feat: add post-PR lifecycle phases, ship method, and attempt tracking#63
feat: add post-PR lifecycle phases, ship method, and attempt tracking#63
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. Also includes resumption and close-out guidance from swamp#1152. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix plan-approved check to allow implement from pr_failed - Add pr-cooldown check tests - Add link_pr from pr_failed recovery test - Track PR attempt number across link/fail/merge cycles - Update link_pr description for pr_failed source phase - Update start transition test to cover new phases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
-
Schema description vs. implementation discrepancy for
attempt: ThePullRequestSchemadescribesattemptas "incremented on each subsequent link_pr call after a pr_failed cycle", but the implementation increments it on everylink_prcall regardless — including URL corrections frompr_open. The test confirms this behavior (link_pr: is idempotent — second call increments attempt), so it is clearly intentional. Worth aligning the description to say "incremented on each subsequent link_pr call" to avoid confusion. -
Test fixture missing required
attemptfield: The pr-cooldown tests construct raw JSON without the requiredattemptfield (e.g.,{ url: "...", linkedAt: "..." }). The tests still pass because the check only accessespr.linkedAtand the data bypasses Zod validation viaJSON.parse(...) as PullRequestData. Addingattempt: 1to those fixtures would keep the test data consistent with the schema. -
Comment in test uses "backwards compat" for a new phase:
schemas_test.tsdescribescompleteacceptingreleasingas "backwards compat for new flow", butreleasingis introduced in this very PR. "Fallback for the full flow" or just dropping the qualifier would be more accurate.
Otherwise the implementation is clean: all three new methods (pr_merged, pr_failed, ship) are correctly implemented and well-tested; the pr-cooldown check is properly gated on the right methods; env vars are restored in finally blocks; no any types in hand-written code; no live cloud services in tests; named exports only. The test coverage is comprehensive with 22 new lifecycle tests covering happy paths, error paths, and recovery flows.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found.
Medium
-
pr-cooldowncheck silently passes on corruptedlinkedAttimestamps (issue-lifecycle/extensions/models/issue_lifecycle.ts:313-316)The cooldown computation is:
const linkedAt = new Date(pr.linkedAt).getTime(); const elapsed = now - linkedAt; if (elapsed < PR_COOLDOWN_MS) { /* reject */ }
If
pr.linkedAtis an invalid date string (data corruption, hand-edited record),getTime()returnsNaN.Date.now() - NaN = NaN, andNaN < 180000evaluates tofalsein JavaScript, so the check passes — the cooldown is silently bypassed.Breaking example: A pullRequest-main record with
linkedAt: "not-a-date"would letpr_mergedorpr_failedexecute immediately after linking.Mitigating factor:
linkedAtis always set bylink_prusingnew Date().toISOString(), so this requires external data corruption. Unlikely in practice.Suggested fix (if you want defense-in-depth):
const linkedAt = new Date(pr.linkedAt).getTime(); if (Number.isNaN(linkedAt)) { return { pass: false, errors: ["pullRequest linkedAt is invalid — re-link the PR."] }; }
Low
-
pr_mergedre-execution after partial failure overwritesmergedAt(issue_lifecycle.ts:1287)If
pr_mergedsucceeds writing the pullRequest resource but fails writing the state, a retry would re-read the PR (now containingmergedAt), then overwritemergedAtwith a newnow(sinceargs.mergedAtdefaults tonow). The original merge timestamp would be lost. In practice the timestamps would be seconds apart, so this is cosmetic. -
No escape from
releasingon failed release builds (state machine design)If the release build fails after
pr_merged, there's no method to go fromreleasingback toimplementingorpr_open. The only exits areship(done),complete(done), orstart(destroys all progress). This is a design gap rather than a code bug — flagging for awareness.
Verdict
PASS — The state machine logic is well-structured, transition checks are consistent with the TRANSITIONS table, attempt tracking handles pre-upgrade records gracefully, and the test coverage is thorough (cooldown boundary cases, idempotent re-link, failure recovery paths). The NaN bypass in pr-cooldown is the only substantive finding and it requires data corruption to trigger.
Summary
Port of systeminit/swamp#1152 and systeminit/swamp#1153 to the extensions repo.
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 statusattemptnumber across link/fail/merge cycles — lifecycle entries show numbered steps like plan iterationspr_failedvialink_pr(re-link) orimplement(major rework)Test Plan
deno check— type checking passesdeno lint— no lint errorsdeno fmt— all files formatted🤖 Generated with Claude Code