feat: add post-PR lifecycle phases and ship method#1153
Conversation
…thod Extends the issue-lifecycle model with intermediate states between PR open and done, giving visibility into CI failures and release builds. New methods: pr_merged (pr_open → releasing), pr_failed (pr_open → pr_failed), ship (releasing → done). Adds pr-cooldown check enforcing 3-minute wait after link_pr before checking PR status. Recovery paths allow link_pr and implement from pr_failed. Skill guidance updated to require human confirmation before opening PRs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Blocking Issues
-
plan-approvedcheck blocks thepr_failed → implementrecovery path (extensions/models/issue_lifecycle.ts:270)
TheTRANSITIONStable now allowsimplementfrompr_failed, but theplan-approvedcheck hard-codesstate.phase !== "approved"— so callingimplementfrompr_failedwill always be rejected by the check, even though thevalid-transitioncheck passes. The recovery path documented in the skill and implementation reference is broken at runtime.Fix: update the
plan-approvedcheck to also allowpr_failedas a valid phase, e.g.:if (state.phase !== "approved" && state.phase !== "pr_failed") {
Suggestions
-
No tests for the
pr-cooldowncheck — The cooldown check has non-trivial time-based logic (readinglinkedAt, computing elapsed time, formatting remaining seconds). A test with a recently-linked PR and a test with an old-enoughlinkedAtwould give confidence this works. The check can be tested via the model's check infrastructure or by calling itsexecutedirectly. -
No test for
link_prfrompr_failedrecovery path —link_prnow acceptspr_failedas a source phase, and the PullRequestSchema documents thatfailedAt/failureReasonare "cleared on next link_pr." Adding a test that callslink_prafter seeding apullRequest-mainresource with failure fields would verify the recovery path and that those fields are indeed absent in the overwritten resource. -
link_prdescription is stale (issue_lifecycle.ts:1170) — The description says "Transitions the phase to pr_open if currently implementing" but it now also handles recovery frompr_failed. Consider updating to mention both source phases. -
starttransition test doesn't cover new phases (schemas_test.ts:44-47) — The test only assertspr_openis included instart's allowed phases. The two new phases (pr_failed,releasing) are also in the list. A minor gap — thePhaseordering test catches their existence, but an explicit assertion onstartwould document the intent.
- Fix plan-approved check to allow implement from pr_failed (recovery path was broken at runtime because the check hard-coded phase !== "approved") - Add pr-cooldown check tests (recently linked, old enough, no PR linked) - Add test for link_pr from pr_failed verifying failure fields are cleared - Update start transition test to cover pr_failed and releasing phases - Update link_pr description to mention pr_failed source phase Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds an `attempt` counter to the pullRequest resource that starts at 1 on the first link_pr call and increments on each subsequent call. The attempt number is carried through pr_failed and pr_merged lifecycle entries so swamp-club shows numbered steps like plan iterations do: "PR linked (attempt 1)", "PR failed (attempt 1)", "PR linked (attempt 2)", "PR merged (attempt 2)". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Well-structured PR that adds post-PR lifecycle phases with clean state machine design.
Blocking Issues
None.
Suggestions
-
as PullRequestDatacast in pr-cooldown check (schemas.ts:310-312): The rawJSON.parse+ type assertion bypasses Zod validation. If the stored data is malformed (e.g.,linkedAtmissing),new Date(pr.linkedAt).getTime()would silently produceNaN, and the cooldown check would always pass. Consider parsing throughPullRequestSchema.parse()for defense-in-depth — though this follows the existing pattern in the codebase, so not blocking. -
Context type duplication across methods: The context type for
pr_merged,pr_failed,link_pr, andshipis repeated inline. A shared type alias (e.g.,MethodContext) could reduce the surface area for drift. Low priority — the current approach is explicit and works.
DDD Assessment
Good domain modeling:
- State transitions via explicit domain methods (
pr_merged,pr_failed,ship) rather than generic setters — proper ubiquitous language. - The
pr-cooldowncheck encapsulates a domain policy as a named concept. - Recovery paths (
pr_failed → link_pr,pr_failed → implement) model real-world scenarios explicitly. - Attempt tracking on
PullRequestDatagives the aggregate history awareness without separate event sourcing.
Checklist
- No
anytypes - All promises awaited (no fire-and-forget)
- License headers present on all
.tsfiles - No imports from internal libswamp paths
- Test coverage for all new methods and the cooldown check
- Schema tests updated for new phases and transitions
- Upgrade entry added for version bump
- Skill docs updated to reflect new phases and human-confirmation rule
Summary
pr_failed,releasing) betweenpr_openanddonefor visibility into CI failures and release buildspr_merged(pr_open → releasing),pr_failed(pr_open → pr_failed),ship(releasing → done)pr-cooldowncheck enforcing a 3-minute wait afterlink_prbefore checking PR status, giving CI time to runpr_failedvialink_pr(re-link) orimplement(major rework)Test Plan
deno check— type checking passesdeno lint— no lint errorsdeno fmt— all files formatteddeno run test— all 4260 tests pass (26 in lifecycle + schema tests)deno run compile— binary compiles successfullylink_pr → pr_merged → shipflowpr_failed → link_prrecovery loop works🤖 Generated with Claude Code