From 7212cfd4ded294891927b49044e44f0821b3cfb4 Mon Sep 17 00:00:00 2001 From: Fernando Castillo <8614435+fr1j0@users.noreply.github.com> Date: Wed, 22 Apr 2026 23:11:31 +0700 Subject: [PATCH] feat(#2473): ship refuses to open PR when HANDOFF.json declares in-progress work (#2553) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(#2473): ship refuses to open PR when HANDOFF.json declares in-progress work Add a preflight step to /gsd-ship that parses .planning/HANDOFF.json and refuses to run git push + gh pr create when any remaining_tasks[].status is not in the terminal set {done, cancelled, deferred_to_backend, wont_fix}. Refusal names each blocking task and lists four resolutions (finish, mark terminal, delete stale file, --force). Missing HANDOFF.json is a no-op so projects that do not use /gsd-pause-work see no behavior change. Also documents the terminal-statuses contract in references/artifact-types.md and adds tests/ship-handoff-preflight.test.cjs to lock in the contract. Closes #2473 * fix(#2473): capture node exit from $() so malformed HANDOFF.json hard-stops Command substitution BLOCKING=$(node -e "...") discards the inner process exit code, so a corrupted HANDOFF.json that fails JSON.parse would yield empty BLOCKING and fall through silently to push_branch — the opposite of what preflight is supposed to do. Capture node's exit into HANDOFF_EXIT via $? immediately after the assignment and branch on it. A non-zero exit is now a hard refusal with the parser error printed on the preceding stderr line. --force does not bypass this branch: if the file exists and can't be parsed, something is wrong and the user should fix it (option 3 in the refusal message — "Delete HANDOFF.json if it's stale" — still applies). Verified with a tmp-dir simulation: captured exit 2, hard-stop fires correctly on malformed JSON. Added a test case asserting the capture ($?) + branch (-ne 0) + parser exit (process.exit(2)) are all present, so a future refactor can't silently reintroduce the bug. Reported by @coderabbitai on PR #2553. --- CHANGELOG.md | 3 + get-shit-done/references/artifact-types.md | 4 +- get-shit-done/workflows/ship.md | 61 +++++++- tests/ship-handoff-preflight.test.cjs | 156 +++++++++++++++++++++ 4 files changed, 219 insertions(+), 5 deletions(-) create mode 100644 tests/ship-handoff-preflight.test.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2407e8a69..a02cccb95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased](https://github.com/gsd-build/get-shit-done/compare/v1.37.1...HEAD) +### Changed +- **`/gsd-ship` refuses to open a PR when `HANDOFF.json` declares in-progress work** — preflight now reads `.planning/HANDOFF.json` and blocks `gh pr create` when any `remaining_tasks[].status` is not in the terminal set `{done, cancelled, deferred_to_backend, wont_fix}`. The refusal names each blocking task and lists four resolutions (finish / mark terminal / delete stale file / `--force`). Missing `HANDOFF.json` remains a no-op (#2473) + ### SDK query layer — Phase 3 (what you get) If you use GSD **as a workflow**—milestones, phases, `.planning/` artifacts, bundled workflows, and `**/gsd:`** commands—Phase 3 is about **behavior matching what the docs and steps promise**, and **a bit less overhead** when the framework advances a phase or bootstraps a new project for you. diff --git a/get-shit-done/references/artifact-types.md b/get-shit-done/references/artifact-types.md index 9684e8cff..20eda5942 100644 --- a/get-shit-done/references/artifact-types.md +++ b/get-shit-done/references/artifact-types.md @@ -48,7 +48,9 @@ reads is inert — the consumption mechanism is what gives an artifact meaning. - **Shape**: Structured pause state (JSON machine-readable + Markdown human-readable) - **Lifecycle**: Created on pause → Consumed on resume → Replaced by next pause - **Location**: `.planning/HANDOFF.json` + `.planning/phases/XX-name/.continue-here.md` (or spike/deliberation path) -- **Consumed by**: `resume-project` workflow +- **Consumed by**: `resume-project` workflow; `ship` workflow (preflight refuses to open a PR when `remaining_tasks[]` holds non-terminal entries) + +**`remaining_tasks[].status` terminal contract:** an entry is "terminal" — i.e. not blocking further work on this branch — when `status` is one of `done`, `cancelled`, `deferred_to_backend`, or `wont_fix`. Any other value (`not_started`, `in_progress`, `paused`, `blocked`, etc.) signals work-in-progress. `/gsd-ship` consumes this contract in preflight; `/gsd-pause-work` writes the entries; `/gsd-resume-work` reads them as the authoritative source of truth over `.continue-here.md`. --- diff --git a/get-shit-done/workflows/ship.md b/get-shit-done/workflows/ship.md index 5a6550625..528e85ff4 100644 --- a/get-shit-done/workflows/ship.md +++ b/get-shit-done/workflows/ship.md @@ -36,7 +36,9 @@ fi -Verify the work is ready to ship: +Verify the work is ready to ship. + +Detect the `--force` override once, before the checks. Set `FORCE=true` if `--force` appears in `$ARGUMENTS`, otherwise `FORCE=false`. The override applies to the "Pending handoff tasks?" check below; it does not skip the other preflight steps. 1. **Verification passed?** ```bash @@ -51,20 +53,71 @@ Verify the work is ready to ship: ``` If uncommitted changes exist: ask user to commit or stash first. -3. **On correct branch?** +3. **Pending handoff tasks?** + + If `.planning/HANDOFF.json` exists, parse `remaining_tasks[]` and refuse when any entry's `status` is not in the terminal set `{done, cancelled, deferred_to_backend, wont_fix}`. The canonical pause/resume contract treats non-terminal statuses as "work still in progress on this branch" — `/gsd-ship` must not create a public PR while that signal is active. + + Missing `HANDOFF.json` is a no-op (preserves existing behavior for projects that don't use `/gsd-pause-work`). A malformed `HANDOFF.json` (non-parseable JSON) is a hard stop — the exit code from `node` is captured explicitly so a bad file can never fall through to a silent pass. + + ```bash + HANDOFF_PATH=.planning/HANDOFF.json + if [ -f "${HANDOFF_PATH}" ]; then + BLOCKING=$(node -e " + const fs = require('fs'); + const TERMINAL = new Set(['done','cancelled','deferred_to_backend','wont_fix']); + let h; + try { h = JSON.parse(fs.readFileSync('${HANDOFF_PATH}','utf8')); } + catch (e) { console.error('HANDOFF.json is not valid JSON: ' + e.message); process.exit(2); } + const tasks = Array.isArray(h.remaining_tasks) ? h.remaining_tasks : []; + const blocking = tasks.filter(t => t && !TERMINAL.has(t.status)); + blocking.forEach(t => console.log(' • [' + (t.status || 'unknown') + '] ' + (t.name || ('task ' + t.id)))); + ") + HANDOFF_EXIT=$? + if [ "${HANDOFF_EXIT}" -ne 0 ]; then + echo "" + echo "✗ Cannot ship: .planning/HANDOFF.json could not be parsed (node exited ${HANDOFF_EXIT})." + echo " Fix the JSON or delete the file, then retry. The parser error is printed above." + exit 1 + fi + if [ -n "${BLOCKING}" ]; then + if [ "${FORCE}" = "true" ]; then + echo "⚠ HANDOFF.json declares in-progress work — shipping anyway because --force was passed:" + echo "${BLOCKING}" + else + cat <&1 ``` diff --git a/tests/ship-handoff-preflight.test.cjs b/tests/ship-handoff-preflight.test.cjs new file mode 100644 index 000000000..5eea87c2f --- /dev/null +++ b/tests/ship-handoff-preflight.test.cjs @@ -0,0 +1,156 @@ +/** + * Regression / contract test for enhancement #2473 + * + * /gsd-ship preflight must refuse to open a PR when .planning/HANDOFF.json + * declares in-progress work. A task is "terminal" (non-blocking) when its + * status is one of {done, cancelled, deferred_to_backend, wont_fix}; any + * other value signals work-in-progress that should block `gh pr create`. + * + * These assertions validate the workflow text itself — not a runtime + * simulation — matching the style of bug-2334-quick-gsd-sdk-preflight.test.cjs. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const WORKFLOW_PATH = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'ship.md'); +const ARTIFACTS_PATH = path.join(__dirname, '..', 'get-shit-done', 'references', 'artifact-types.md'); + +const TERMINAL_STATUSES = ['done', 'cancelled', 'deferred_to_backend', 'wont_fix']; + +describe('enhancement #2473: /gsd-ship refuses to open PR when HANDOFF.json declares in-progress work', () => { + const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const preflightStart = workflow.indexOf(''); + const preflightEnd = workflow.indexOf('', preflightStart); + assert.ok(preflightStart !== -1 && preflightEnd !== -1, 'preflight_checks step must exist'); + const preflight = workflow.slice(preflightStart, preflightEnd); + + test('preflight checks the HANDOFF.json path', () => { + assert.match( + preflight, + /\.planning\/HANDOFF\.json/, + 'preflight must reference .planning/HANDOFF.json so the check runs against the canonical pause-work artifact' + ); + }); + + test('preflight parses remaining_tasks[] and inspects status', () => { + assert.match( + preflight, + /remaining_tasks/, + 'preflight must parse the remaining_tasks[] array from HANDOFF.json' + ); + assert.match( + preflight, + /status/, + 'preflight must inspect per-task status to distinguish terminal from in-progress entries' + ); + }); + + test('preflight enumerates all four terminal statuses', () => { + for (const status of TERMINAL_STATUSES) { + assert.ok( + preflight.includes(status), + `preflight must list "${status}" as terminal so tasks with this status don't block shipping` + ); + } + }); + + test('preflight refuses (exits non-zero) when a blocking task is found', () => { + assert.match( + preflight, + /exit 1/, + 'preflight must exit non-zero on a blocking task so the workflow stops before push/PR creation' + ); + }); + + test('refusal message names the blocking tasks and lists resolution options', () => { + assert.ok( + /Cannot ship/i.test(preflight) || /blocking tasks?/i.test(preflight), + 'refusal must explicitly say shipping is blocked and list the offending tasks' + ); + assert.match( + preflight, + /--force/, + 'refusal must mention the --force escape hatch so the user knows how to override' + ); + }); + + test('--force override is detected from $ARGUMENTS and bypasses the check', () => { + assert.match( + preflight, + /\$ARGUMENTS/, + '--force must be read from $ARGUMENTS, matching the existing --text convention in this workflow' + ); + // Both the detection and the bypass branch must live inside preflight_checks + const mentionsForceMoreThanOnce = (preflight.match(/--force|FORCE/g) || []).length >= 2; + assert.ok( + mentionsForceMoreThanOnce, + 'preflight must both detect --force and branch on it (set + check)' + ); + }); + + test('missing HANDOFF.json is a no-op (preserves existing behavior)', () => { + assert.match( + preflight, + /if \[ -f "?\$\{?HANDOFF_PATH\}?"? \]|if \[ -f "?\.planning\/HANDOFF\.json"? \]/, + 'preflight must guard on HANDOFF.json existence — absent file means no check, preserving pre-#2473 behavior' + ); + }); + + test('malformed HANDOFF.json is a hard stop (node exit code is captured, not swallowed by $())', () => { + // Command substitution $() discards the inner exit code, so the node parser + // exit must be captured explicitly via $? and branched on. Without this, + // a corrupted HANDOFF.json would yield empty BLOCKING and ship silently. + assert.match( + preflight, + /HANDOFF_EXIT=\$\?/, + 'preflight must capture $? from the node invocation so a non-zero exit is visible to the shell' + ); + assert.match( + preflight, + /HANDOFF_EXIT.*-ne 0/, + 'preflight must branch on the captured node exit and refuse when parsing failed' + ); + // And the parser itself must still signal failure on bad JSON + assert.match( + preflight, + /process\.exit\(2\)/, + 'node parser must exit non-zero on invalid JSON so the captured exit code is meaningful' + ); + }); + + test('pending-handoff check is placed before push_branch and create_pr (cannot fall through to gh pr create)', () => { + const pushStart = workflow.indexOf(''); + const createStart = workflow.indexOf(''); + const handoffInPreflight = preflight.search(/HANDOFF\.json/); + assert.ok(handoffInPreflight !== -1, 'HANDOFF.json check must live inside preflight_checks'); + assert.ok( + preflightEnd < pushStart && pushStart < createStart, + 'preflight_checks must run before push_branch and create_pr so a refusal prevents any public action' + ); + }); + + test('artifact-types.md documents the terminal-statuses contract', () => { + const artifacts = fs.readFileSync(ARTIFACTS_PATH, 'utf-8'); + const handoffSection = artifacts.slice( + artifacts.indexOf('### HANDOFF.json'), + artifacts.indexOf('---', artifacts.indexOf('### HANDOFF.json')) + ); + assert.ok(handoffSection.length > 0, 'HANDOFF.json section must exist in artifact-types.md'); + for (const status of TERMINAL_STATUSES) { + assert.ok( + handoffSection.includes(status), + `artifact-types.md HANDOFF.json entry must enumerate terminal status "${status}"` + ); + } + assert.match( + handoffSection, + /ship/i, + 'artifact-types.md HANDOFF.json entry must name /gsd-ship as a consumer' + ); + }); +});