Add /ship: verify → review → raise orchestrator (#223)

[FrankRay78]

Why

Closes #223. NetPace already had every ingredient a supervised ship flow composes — /raise-pr, /review-slop, /capture-learnings, and the pr-review-toolkit plugin — but nothing wired them behind a single hard test-green gate. Today that's 3+ separate manual invocations. /ship is the orchestrator that chains them so review and PR creation cannot happen against un-verified or red code.

Per the issue, this is the small additive half of IMS#122 — the orchestrator only. NetPace never built IMS's green-ledger/Stop-hook machinery, so none of the "retire the proxy" scope applies here.

What changes

• New /ship command (.claude/commands/ship.md) — steps 0–5 with a structural step‑1→step‑2 gate:
  • 0 up-front precondition guard (stop on main or no-commits-over-main before any suite/review; capture a working-tree baseline hash);
  • 1 always run dotnet build ./src && dotnet test ./src, gate on exit code, red ⇒ STOP;
  • 2 clean-context pr-review-toolkit reviewers + /review-slop; /ship's own loop turns findings into edits;
  • 3 re-verify + commit the fixes, gated on a whole-tracked-tree content-hash diff (not a *.cs porcelain filter);
  • 4 raise via /raise-pr;
  • 5 /capture-learnings on synchronous sources only.
  • Global stop-on-failure.
• /capture-learnings — adds an unconditional, best-effort per-PR review source (gh pr view --json comments,reviews) routed through the existing enforceability triage; a no-op when no PR/review exists, never a wait.
• /raise-pr tidy — drops the spec-folder confirmation AskUserQuestion so the folder is removed and committed without prompting, keeping /raise-pr composable by /ship with no interactive gate.

Non-obvious things a reviewer should know

• Capture-learnings feeds from the synchronous in-/ship review (Review A), never the async @claude PR review (Review B). /ship deliberately does not wait on or prompt about Review B — it's author-gated (claude.yml) and posts minutes later for a human to read at merge. This is the core design decision (issue AC-10).
• No suite-skip for docs-only branches. The always-on gh pr create PreToolUse hook re-runs the suite at PR creation anyway, so a skip would save nothing and could let review run before a late hook-block. /ship always runs step 1 (AC-5).
• Change detection uses a content hash of the whole tracked tree, so it catches build-affecting non-.cs edits (.csproj, Directory.Packages.props, .json) and ignores files already dirty before the review — a *.cs-only git status filter would miss both.
• No hook polices the step-1→step-2 ordering and the gh pr create hook is unchanged — control flow is the gate, by design.

How this branch was produced

This PR was dogfooded through /ship itself: step 1 ran green (616 tests), step 2's code-reviewer + slop review surfaced a real control-flow contradiction in the /capture-learnings edit (an "unconditional" paragraph nested under a conditionally-skipped step) plus two verbosity trims, /ship's loop applied them, step 3 re-verified green and committed them (0a80e4e), and this PR is step 4.

How to verify

• Read ship.md — confirm the step‑1→step‑2 gate is structural (review is downstream of the exit code) with no ordering hook added.
• Confirm /capture-learnings's PR-review source is unconditional and clearly not skipped when has_branch_context is false.
• Confirm /raise-pr no longer prompts to delete the spec folder.
• settings.json and the gh pr create hook are untouched; no Constitution amendment.

Related

• Issue: Build /ship: human-invoked verify → review → raise orchestrator (NetPace adaptation of IMS#122) #223
• Upstream: IMS#122; supersedes the async-review seam of IMS#128.

[FrankRay78]

@claude Review this pull request. Analyse the code changes and provide feedback covering:

• Bugs or correctness issues
• Security concerns (including any flagged by static analysis)
• Adherence to the project conventions in CLAUDE.md
• Test coverage — do the tests adequately cover the new behaviour?
• Any spec/test-plan mismatches

[claude]

Claude finished @FrankRay78's task in 2m 22s —— View job

---

Review

• Gather context (diff, changed files, CLAUDE.md conventions)
• Read .claude/commands/ship.md (new orchestrator)
• Read .claude/commands/capture-learnings.md diff
• Read .claude/commands/raise-pr.md diff
• Analyse for bugs/correctness, security, convention adherence, test coverage, spec/test-plan mismatches
• Post final review

This PR adds three Claude Code slash-command markdown files (agent instructions, not application code), so most of the review is about the procedure they describe rather than compiled correctness. Overall the design is solid — the step‑1→step‑2 structural gate is real (review genuinely sits downstream of the test exit code, no hook needed), and the split of capture-learnings.md step 3 into independently-gated git/PR sources correctly fixes the control-flow contradiction the PR description mentions.

One correctness gap in the step‑3 change-detection gate (.claude/commands/ship.md:33):

compute `POST_HASH = git diff HEAD | git hash-object --stdin` and compare to `PRE_HASH` from step 0.
This covers **all** tracked files (...) and correctly ignores files that were already dirty before step 2.

git diff HEAD only shows changes to already-tracked files — it does not include new, untracked files. If step 2's review loop adds a brand-new file (e.g. pr-test-analyzer flags missing coverage and /ship creates a new test file, or code-simplifier extracts a new helper file), that file stays untracked and invisible to both PRE_HASH and POST_HASH. If it's the only change step 2 makes, POST_HASH == PRE_HASH, so step 3 concludes "nothing changed," skips the re-verify and the commit, and goes straight to step 4. The new file is never staged, never committed, and /raise-pr's git push won't carry an uncommitted/untracked file — so the fix silently never reaches the PR, with no error surfaced anywhere. This directly undercuts the design's own rationale one paragraph up ("a *.cs-only git status filter would miss both" build-affecting non-.cs edits and pre-existing dirt) — the tracked-only hash still misses new files. Worth either running git add -A (or an untracked-file check) before hashing, or hashing git status --short alongside the tracked diff.

Minor — dirty-baseline staging is underspecified (.claude/commands/ship.md:38, "stage only the review edits, not unrelated pre-existing local changes"): the hash mechanism only detects that the tree changed in aggregate when the baseline was already dirty; no procedure is given for isolating which hunks came from step 2 vs. pre-existing dirt (e.g. a stash/diff/restore sequence). Low-impact since a dirty baseline is flagged as unusual, but the instruction currently asks for something the described mechanism can't actually compute.

Note, not a bug — hardcoded main (.claude/commands/ship.md:12,14): git rev-parse --abbrev-ref HEAD/git log main..HEAD hardcode main, while capture-learnings.md resolves the default branch dynamically (git symbolic-ref refs/remotes/origin/HEAD → gh repo view → fallback main). This matches the pre-existing pattern in raise-pr.md, so it's not a regression from this PR, just an inconsistency between sibling commands if main is ever renamed.

Convention adherence: no hard-wrapped markdown (good, per CLAUDE.md), no specs/<NNN>-… references introduced, no Constitution amendment, settings.json and the gh pr create PreToolUse hook are confirmed untouched and match the claims in the PR body (verified the hook at .claude/settings.json:127-129).

raise-pr.md's dropped confirmation (removes the AskUserQuestion before deleting+committing the spec folder): this is called out and justified in the PR body as deliberate, needed for /ship composability. Worth being consciously aware it also removes the safety net for a human invoking /raise-pr directly (not just via /ship) — a destructive git op (delete + commit) that previously asked now runs unconditionally. Given the PR explicitly flags this trade-off and it's an interactive dev-tool used by the maintainer, not blocking.

Test coverage: not applicable in the traditional sense — these are Claude Code agent-instruction files, not testable C#/production code, consistent with sibling commands (review-slop.md, existing raise-pr.md) which also carry no automated test suite. No spec.md/test-plan.md exists for this change (not spec-kit driven), so AC-to-Test Traceability (Constitution IX) doesn't apply here.

Spec/test-plan mismatches: none — no spec artifacts for this PR to check against.

[FrankRay78]

Design revision to #223 (AC-7 / AC-10) — intentional

After dogfooding, we changed two things about /ship so it can drive an automated (ralph) loop, not just a single supervised invocation:

1. Removed the capture-learnings step (was Build /ship: human-invoked verify → review → raise orchestrator (NetPace adaptation of IMS#122) #223 step 5 / AC-10). Reflection needs human curation and batches better across many features. Inside an unattended loop, an in-flow /capture-learnings would either block on its prompt or be auto-skipped to nothing — a category mismatch. /ship now ends at the raised PR with a soft nudge to run /capture-learnings at a later supervised checkpoint. The underlying Build /ship: human-invoked verify → review → raise orchestrator (NetPace adaptation of IMS#122) #223 principle — never wait on the async @claude review (Review B) — is unchanged, and the standalone /capture-learnings PR-review-source fetch (which picks Review B up later) stays.
2. Added a Blocker/Important fix policy to step 2 (strengthens AC beyond Build /ship: human-invoked verify → review → raise orchestrator (NetPace adaptation of IMS#122) #223's discretionary wording). Confirmed Blocker/P1 must be fixed or /ship STOPs (never ship a known blocker); confirmed Important/P2 fixed when in-scope for the branch, else recorded as a follow-up (preserves scope discipline); Suggestion/P3 discretionary. Findings are validated real before acting; every fix is re-verified by step 3.

Since #223's author is driving this, the AC-7/AC-10 revision is deliberate. Commit: 7aed36c.

[FrankRay78]

Addressed Review B's untracked-file finding (ed50165)

Good catch — this was a real P1 that the in-flow synchronous review missed. git diff HEAD doesn't report new untracked files, so if a review's only change was creating a file, step 3 saw POST_HASH == PRE_HASH, skipped the re-verify + commit, and the fix never reached the PR.

Fixed:

• Folded git status --short into the PRE_HASH/POST_HASH snapshot so a new untracked file changes the hash and triggers the re-verify + commit.
• Commit the specific paths the step-2 loop edited or created (git add <paths>, includes new files) rather than git add -A — which also resolves the underspecified dirty-baseline staging you flagged (no hunk-isolation needed).

Deferred, per /ship's own P2 scope policy: the hardcoded-main note (you marked it not-a-bug; it matches raise-pr.md's existing pattern, so changing it here is out-of-scope) and the /raise-pr confirmation drop (the intended tidy, trade-off already flagged in the body).

[FrankRay78]

@claude Please re-review, identify only showstoppers or important issues (P1/P2).

[claude]

Claude finished @FrankRay78's task in 3m 43s —— View job

---

Re-review (P1/P2 only)

• Gather context (latest diff, prior findings, fixes applied since first review)
• Re-examine ship.md for correctness/control-flow issues
• Re-examine capture-learnings.md and raise-pr.md diffs
• Filter to P1 (showstopper) / P2 (important) only
• Post final review

Confirmed the untracked-file fix (ed50165) is correct: folding git status --short into PRE_HASH/POST_HASH does catch a brand-new file, and targeted git add <paths> correctly resolves the dirty-baseline staging concern from the first review (git add <path> also stages deletions of already-tracked paths, so that part is sound too).

P2 — step 3's targeted-path staging has no defined source for code-simplifier's edits (.claude/commands/ship.md:38)

"Stage the specific paths step 2's loop edited or created (git add <those paths> — the loop knows them...)"

code-simplifier is called out one step earlier as the one reviewer that "edits files directly" (line 25) — its changes land in the working tree independently of /ship's own apply loop, which only tracks paths it itself edits in response to aggregated findings (line 33-36). The instruction asserts "the loop knows them" without saying how /ship learns which paths code-simplifier touched.

Worst case: code-simplifier is the only thing that changes anything in step 2 (no other finding gets acted on). Then POST_HASH != PRE_HASH correctly triggers a re-verify — but /ship's own "paths I edited" set is empty, so the described git add <paths> has nothing to stage, and code-simplifier's fix is silently left uncommitted → never reaches the pushed PR. This is the same failure mode ed50165 just fixed (a real change that doesn't reach the PR), reopened via a different code path.

Suggested fix: don't rely on /ship's loop to enumerate touched paths from memory — re-derive the exact changed-path set at commit time from git status --short (diffed against the step-0 baseline), which covers edits from any source (the loop's own fixes and code-simplifier's) rather than only the ones /ship applied itself.

No other P1/P2s found. capture-learnings.md and raise-pr.md are unchanged since the last review pass and no new issues surfaced there.
· branch feature/ship-command

[FrankRay78]

Resolved by simplifying, not patching (a08a9f6)

You're right that code-simplifier's direct edits aren't in /ship's own edited-paths set — a code-simplifier-only change would never get committed. Rather than add machinery to re-derive paths, we removed the machinery that created the problem.

The whole step-3 staging complexity (hash baseline, untracked special-casing, "commit only the paths the loop edited") existed to support one thing: running /ship on a dirty tree and committing only the review's edits. That single ambition produced both this finding and the earlier untracked-file gap.

Fix: step 0 now requires a clean tree (STOP with "commit or stash your changes before shipping" if dirty) — the normal workflow, since you commit your implementation before shipping. With a clean start, whatever git status shows after step 2 is the review's edits and nothing else, so step 3 collapses to: non-empty status → re-verify → git add -A && commit. That's correct for edits from any source (the loop's and code-simplifier's), and it closes both edge cases by deleting the code that caused them.

[FrankRay78]

@claude Please re-review, identify only showstoppers or important issues (P1/P2).

[claude]

Claude finished @FrankRay78's task in 1m 53s —— View job

---

Re-review (P1/P2 only)

• Gather context (latest diff, prior findings, fixes applied since last review)
• Re-examine ship.md after the a08a9f6 simplification (clean-tree requirement)
• Re-examine capture-learnings.md and raise-pr.md diffs
• Filter to P1 (showstopper) / P2 (important) only
• Post final review

No P1/P2 issues found. a08a9f6 fully resolves the P2 I raised in the previous pass.

Confirmed the fix is correct and complete:

• Step 0 (ship.md:17) now hard-gates on git status --porcelain being empty before any suite run or review — /ship STOPs with "Commit or stash your changes before shipping" on a dirty tree.
• Step 3 (ship.md:37-41) collapses to: non-empty git status --porcelain after step 2 ⇒ re-verify ⇒ git add -A && commit. Because the tree is guaranteed clean going into step 2, this is provably correct for any source of change — /ship's own applied fixes, and code-simplifier's direct edits (the exact gap the previous P2 identified) — with no path-enumeration or hash-comparison machinery needed. It also fully subsumes the earlier untracked-file gap (ed50165), since a brand-new file is just another line in git status --porcelain.
• The removed machinery (content-hash PRE_HASH/POST_HASH, targeted git add <paths>) is the machinery that produced both prior findings — deleting it instead of patching it is the right call here, not just a smaller diff.

capture-learnings.md and raise-pr.md are unchanged since the last review pass (confirmed via diff) — no new issues there. The two previously-noted non-blockers (hardcoded main in ship.md, and raise-pr.md's dropped confirmation prompt) remain deliberate, already-flagged trade-offs, not P1/P2.
· branch feature/ship-command