From 68a199cf5a9dce455fef925511a7e63c2fc68a19 Mon Sep 17 00:00:00 2001 From: Dennis Kim Date: Tue, 11 Aug 2026 17:10:36 -0400 Subject: [PATCH] fix(#2783): address wedged PRs in ship note protocol (#2818) * 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 --- .changeset/2783-ship-note-wedged-pr.md | 7 + gsd-core/workflows/ship.md | 34 +++ .../2818-ship-note-wedged-pr.json | 6 + .../fix-2138-ship-note-lost-on-merge.test.cjs | 7 + tests/ship-notes-wedged-pr.test.cjs | 217 ++++++++++++++++++ 5 files changed, 271 insertions(+) create mode 100644 .changeset/2783-ship-note-wedged-pr.md create mode 100644 tests/emitted-drift-acks/2818-ship-note-wedged-pr.json create mode 100644 tests/ship-notes-wedged-pr.test.cjs diff --git a/.changeset/2783-ship-note-wedged-pr.md b/.changeset/2783-ship-note-wedged-pr.md new file mode 100644 index 000000000..bb096cc4c --- /dev/null +++ b/.changeset/2783-ship-note-wedged-pr.md @@ -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.* diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index 754c6e8a0..d1d6b2c12 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -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 ``` diff --git a/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json b/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json new file mode 100644 index 000000000..344d90c1a --- /dev/null +++ b/tests/emitted-drift-acks/2818-ship-note-wedged-pr.json @@ -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." + } +} diff --git a/tests/fix-2138-ship-note-lost-on-merge.test.cjs b/tests/fix-2138-ship-note-lost-on-merge.test.cjs index c6e75a264..20d768e73 100644 --- a/tests/fix-2138-ship-note-lost-on-merge.test.cjs +++ b/tests/fix-2138-ship-note-lost-on-merge.test.cjs @@ -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), diff --git a/tests/ship-notes-wedged-pr.test.cjs b/tests/ship-notes-wedged-pr.test.cjs new file mode 100644 index 000000000..17e35a5d6 --- /dev/null +++ b/tests/ship-notes-wedged-pr.test.cjs @@ -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 = ``; + const start = content.indexOf(open); + assert.notEqual(start, -1, `ship.md must contain a ${name} step`); + const end = content.indexOf('', 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, + /[\s\S]*?<\/step>\s*/, + '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', + ); + }); +});