* fix(#2783): address wedged PRs in ship note protocol * chore: acknowledge ship.md growth * fix(#2783): repair ship workflow structure * fix(#2783): gate ship-note recovery on current PR state * fix(#2783): avoid scanner collision in poll loop * fix(#2783): address reviewer feedback on ship-note wedge handling * test: add timeout to spawnSync in ship-notes-wedged-pr.test.cjs to satisfy lint * test: update ghCalls bound expectation in ship-notes-wedged-pr.test.cjs * test: restore ghCalls expected count in ship-notes-wedged-pr.test.cjs
This commit is contained in:
Dennis Kim
7
.changeset/2783-ship-note-wedged-pr.md
Normal file
7
.changeset/2783-ship-note-wedged-pr.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 2818
|
||||
---
|
||||
**`/gsd-ship` now detects and recovers a PR wedged by the ship-note commit** — when the `[ci skip]` ship note leaves required checks unstarted, ship re-triggers CI instead of leaving the PR unmergeable. (#2783)
|
||||
|
||||
*Note: This introduces a latency tradeoff. All `/gsd-ship` invocations now poll GitHub PR state for up to 15 seconds to ensure the commit was processed and check if recovery is needed, even for repositories without required checks.*
|
||||
@@ -455,7 +455,41 @@ would otherwise trigger (GitHub honors `[ci skip]` / `[skip ci]`):
|
||||
|
||||
```bash
|
||||
gsd_run query commit "docs(${padded_phase}): ship phase ${PHASE_NUMBER} — PR #${PR_NUMBER} [ci skip]" --files .planning/STATE.md
|
||||
SHIP_NOTE_SHA=$(git rev-parse HEAD)
|
||||
git push origin ${CURRENT_BRANCH} 2>&1 || echo "⚠ track_shipping: ship-note push failed — it is local-only; rerun: git push origin ${CURRENT_BRANCH}"
|
||||
|
||||
# Preserve the skip-token optimization for repositories without a required-check
|
||||
# wedge; only synthesize a second CI-triggering commit when GitHub reports one (#2783).
|
||||
# Poll mergeStateStatus with backoff to avoid racing GitHub's async state computation.
|
||||
# Note: Skip tokens recognized by GitHub Actions are [skip ci], [ci skip], [no ci], [skip actions], [actions skip], and skip-checks:true.
|
||||
# The recovery commit message MUST NOT contain any of these tokens.
|
||||
|
||||
STATUS="UNKNOWN"
|
||||
CHECKS=0
|
||||
REVIEW_DECISION=""
|
||||
for i in {1..5}; do
|
||||
PR_STATE=$(gh pr view ${PR_NUMBER} --json headRefOid,mergeStateStatus,statusCheckRollup,reviewDecision -q '{head: .headRefOid, status: .mergeStateStatus, checks: ((.statusCheckRollup // []) | length), review: (.reviewDecision // "")}' 2>/dev/null || echo '{"head":"","status":"UNKNOWN","checks":0,"review":""}')
|
||||
HEAD_OID=$(echo "$PR_STATE" | jq -r .head)
|
||||
if [ "$HEAD_OID" = "$SHIP_NOTE_SHA" ]; then
|
||||
STATUS=$(echo "$PR_STATE" | jq -r .status)
|
||||
CHECKS=$(echo "$PR_STATE" | jq -r .checks)
|
||||
REVIEW_DECISION=$(echo "$PR_STATE" | jq -r .review)
|
||||
fi
|
||||
if [ "$HEAD_OID" = "$SHIP_NOTE_SHA" ] && [ "$STATUS" != "UNKNOWN" ]; then
|
||||
break
|
||||
fi
|
||||
sleep 3
|
||||
done
|
||||
|
||||
if [ "$STATUS" = "BLOCKED" ] && [ "$CHECKS" = "0" ] && [ "$REVIEW_DECISION" != "REVIEW_REQUIRED" ] && [ "$REVIEW_DECISION" != "CHANGES_REQUESTED" ] && git log -1 --format=%B "$SHIP_NOTE_SHA" | grep -q '\[ci skip\]'; then
|
||||
echo "⚠ PR is BLOCKED with zero checks. The [ci skip] trailer wedged the PR due to required checks."
|
||||
echo "Pushing an empty commit to trigger the required pipelines..."
|
||||
# gsd_run query commit requires a file list; use git directly for this intentionally empty commit.
|
||||
git commit --allow-empty -m "chore: trigger CI (recover from ship-note skip-token)"
|
||||
git push origin ${CURRENT_BRANCH} 2>&1 || echo "⚠ track_shipping: recovery push failed — rerun: git push origin ${CURRENT_BRANCH}"
|
||||
elif [ "$STATUS" = "UNKNOWN" ]; then
|
||||
echo "⚠ track_shipping: PR mergeStateStatus is UNKNOWN after polling; PR may require manual check re-trigger."
|
||||
fi
|
||||
```
|
||||
</step>
|
||||
|
||||
|
||||
6
tests/emitted-drift-acks/2818-ship-note-wedged-pr.json
Normal file
6
tests/emitted-drift-acks/2818-ship-note-wedged-pr.json
Normal file
@@ -0,0 +1,6 @@
|
||||
{
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"ship.md": "#2783: the ship-note protocol adds a bounded merge-state poll and an empty recovery commit when a skip token leaves a PR blocked with no checks."
|
||||
}
|
||||
}
|
||||
@@ -54,6 +54,13 @@ describe('#2138 ship.md track_shipping pushes the ship-note onto the PR branch',
|
||||
);
|
||||
});
|
||||
|
||||
test('the ship-note step handles wedged PRs caused by skip-token or required status checks', () => {
|
||||
assert.ok(
|
||||
/mergeStateStatus|BLOCKED|trigger CI/.test(step),
|
||||
'track_shipping must include self-healing or check handling for wedged PRs (#2783)',
|
||||
);
|
||||
});
|
||||
|
||||
test('the ship-note commit still records the phase + PR number in STATE', () => {
|
||||
assert.ok(
|
||||
/ship phase \$\{PHASE_NUMBER\}.*PR #\$\{PR_NUMBER\}/.test(step),
|
||||
|
||||
217
tests/ship-notes-wedged-pr.test.cjs
Normal file
217
tests/ship-notes-wedged-pr.test.cjs
Normal file
@@ -0,0 +1,217 @@
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { stripFencedCode } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs');
|
||||
const { cleanup, createTempDir, readFileNormalized } = require('./helpers.cjs');
|
||||
|
||||
const SHIP_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'ship.md');
|
||||
|
||||
function extractStep(name) {
|
||||
const content = readFileNormalized(SHIP_MD);
|
||||
const open = `<step name="${name}">`;
|
||||
const start = content.indexOf(open);
|
||||
assert.notEqual(start, -1, `ship.md must contain a ${name} step`);
|
||||
const end = content.indexOf('</step>', start);
|
||||
assert.notEqual(end, -1, `${name} step must close`);
|
||||
return content.slice(start, end);
|
||||
}
|
||||
|
||||
function extractTrackShippingScript() {
|
||||
const step = extractStep('track_shipping');
|
||||
const blocks = [...step.matchAll(/```bash\r?\n([\s\S]*?)\r?\n```/g)];
|
||||
const match = blocks.find(block => block[1].includes('mergeStateStatus'));
|
||||
assert.ok(match, 'track_shipping must contain an executable merge-state bash block');
|
||||
return match[1];
|
||||
}
|
||||
|
||||
function runTrackShipping(responses) {
|
||||
const tmpDir = createTempDir('gsd-ship-note-');
|
||||
const responsesPath = path.join(tmpDir, 'responses.jsonl');
|
||||
const ghCallsPath = path.join(tmpDir, 'gh-calls.log');
|
||||
const gitCallsPath = path.join(tmpDir, 'git-calls.log');
|
||||
|
||||
try {
|
||||
fs.writeFileSync(
|
||||
responsesPath,
|
||||
responses.map(response => JSON.stringify(response)).join('\n') + '\n',
|
||||
'utf8',
|
||||
);
|
||||
fs.writeFileSync(ghCallsPath, '', 'utf8');
|
||||
fs.writeFileSync(gitCallsPath, '', 'utf8');
|
||||
|
||||
const preamble = [
|
||||
'gsd_run() { :; }',
|
||||
'sleep() { :; }',
|
||||
'git() {',
|
||||
' if [ "$1" = "rev-parse" ] && [ "$2" = "HEAD" ]; then',
|
||||
' printf "%s\\n" "$EXPECTED_HEAD"',
|
||||
' elif [ "$1" = "log" ]; then',
|
||||
' printf "[ci skip]\\n"',
|
||||
' elif [ "$1" = "commit" ]; then',
|
||||
' printf "commit\\n" >> "$GIT_CALLS"',
|
||||
' elif [ "$1" = "push" ]; then',
|
||||
' printf "push\\n" >> "$GIT_CALLS"',
|
||||
' fi',
|
||||
'}',
|
||||
'gh() {',
|
||||
' _call=$(wc -l < "$GH_CALLS")',
|
||||
' _call=$((_call + 1))',
|
||||
' printf "call\\n" >> "$GH_CALLS"',
|
||||
' sed -n "${_call}p" "$GH_RESPONSES"',
|
||||
'}',
|
||||
'jq() {',
|
||||
' "$NODE_BIN" -e "let d=\'\';process.stdin.on(\'data\',c=>d+=c);process.stdin.on(\'end\',()=>{const o=JSON.parse(d);process.stdout.write(String(o[process.argv[1].slice(1)] ?? \'\'));});" "$2"',
|
||||
'}',
|
||||
].join('\n');
|
||||
|
||||
const result = spawnSync('bash', ['-c', `${preamble}\n${extractTrackShippingScript()}`], {
|
||||
cwd: tmpDir,
|
||||
encoding: 'utf8',
|
||||
timeout: 10000,
|
||||
env: {
|
||||
...process.env,
|
||||
CURRENT_BRANCH: 'fix/ship-note',
|
||||
EXPECTED_HEAD: 'current-head',
|
||||
GH_CALLS: ghCallsPath,
|
||||
GH_RESPONSES: responsesPath,
|
||||
GIT_CALLS: gitCallsPath,
|
||||
NODE_BIN: process.execPath,
|
||||
PHASE_NUMBER: '1',
|
||||
PR_NUMBER: '123',
|
||||
padded_phase: '01',
|
||||
},
|
||||
});
|
||||
|
||||
assert.strictEqual(result.status, 0, `track_shipping failed: ${result.stderr}`);
|
||||
return {
|
||||
ghCalls: fs.readFileSync(ghCallsPath, 'utf8').trim().split(/\r?\n/).filter(Boolean),
|
||||
gitCalls: fs.readFileSync(gitCallsPath, 'utf8').trim().split(/\r?\n/).filter(Boolean),
|
||||
};
|
||||
} finally {
|
||||
cleanup(tmpDir);
|
||||
}
|
||||
}
|
||||
|
||||
describe('#2783 ship.md track_shipping self-heals wedged PRs', () => {
|
||||
const step = extractStep('track_shipping');
|
||||
|
||||
test('track_shipping inspects mergeStateStatus post-push', () => {
|
||||
assert.ok(
|
||||
/mergeStateStatus/.test(step),
|
||||
'track_shipping must query mergeStateStatus to detect wedged PRs (#2783)'
|
||||
);
|
||||
});
|
||||
|
||||
test('track_shipping self-heals BLOCKED PRs by pushing a recovery commit without skip token', () => {
|
||||
assert.ok(
|
||||
/BLOCKED/.test(step),
|
||||
'track_shipping must check for BLOCKED merge state (#2783)'
|
||||
);
|
||||
assert.ok(
|
||||
/trigger CI/.test(step) || /allow-empty/.test(step),
|
||||
'track_shipping must push a recovery commit to trigger CI when wedged (#2783)'
|
||||
);
|
||||
});
|
||||
|
||||
test('track_shipping and the following step remain outside balanced code fences', () => {
|
||||
const content = fs.readFileSync(SHIP_MD, 'utf8');
|
||||
const stripped = stripFencedCode(content);
|
||||
assert.strictEqual(stripped.unterminatedFence, false, 'ship.md must not contain an unterminated code fence');
|
||||
assert.match(
|
||||
stripped.text,
|
||||
/<step name="track_shipping">[\s\S]*?<\/step>\s*<step name="ship_post_capability_dispatch">/,
|
||||
'the track_shipping boundary and following step must remain visible after stripping code fences',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2783 ship-note recovery decisions use current GitHub state', { skip: process.platform === 'win32' }, () => {
|
||||
test('ignores a stale BLOCKED response until headRefOid matches the pushed commit', () => {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'stale-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'CLEAN', checks: 0, review: '' },
|
||||
]);
|
||||
|
||||
assert.strictEqual(result.ghCalls.length, 2, 'must poll through the stale PR response');
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
0,
|
||||
'must not recover from merge state attached to an older head',
|
||||
);
|
||||
});
|
||||
|
||||
test('does not recover when review requirements are the BLOCKED reason', () => {
|
||||
for (const review of ['REVIEW_REQUIRED', 'CHANGES_REQUESTED']) {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'current-head', status: 'BLOCKED', checks: 0, review },
|
||||
]);
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
0,
|
||||
`${review} must not create an empty CI-recovery commit`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('still recovers a current BLOCKED head with zero checks and no review gate', () => {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'current-head', status: 'BLOCKED', checks: 0, review: 'APPROVED' },
|
||||
]);
|
||||
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
1,
|
||||
'a confirmed skip-token wedge must create exactly one recovery commit',
|
||||
);
|
||||
});
|
||||
|
||||
test('recovers successfully on the 4th polling attempt', () => {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'stale-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
]);
|
||||
assert.strictEqual(result.ghCalls.length, 4, 'must poll exactly 4 times');
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
1,
|
||||
'must recover after resolving on the 4th attempt',
|
||||
);
|
||||
});
|
||||
|
||||
test('recovers successfully on the 5th (final) polling attempt', () => {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'stale-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
]);
|
||||
assert.strictEqual(result.ghCalls.length, 5, 'must poll exactly 5 times');
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
1,
|
||||
'must recover after resolving on the 5th attempt',
|
||||
);
|
||||
});
|
||||
|
||||
test('exhausts the polling bound after 5 attempts and warns without recovering (attempt 6)', () => {
|
||||
const result = runTrackShipping([
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'UNKNOWN', checks: 0, review: '' },
|
||||
{ head: 'current-head', status: 'BLOCKED', checks: 0, review: '' },
|
||||
]);
|
||||
assert.strictEqual(result.ghCalls.length, 5, 'must exhaust after exactly 5 polling attempts');
|
||||
assert.strictEqual(
|
||||
result.gitCalls.filter(call => call === 'commit').length,
|
||||
0,
|
||||
'must not recover if the status is unresolved when polling exhausts',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user