Repository navigation
feat(lifecycle): link authoritative PR merges and expose claim-to-merge timelines (#1203) - #1296
Conversation
…ge timelines (#1203) Record pr_merged through one shared write path (recordPrMergedEvent) with a canonical dedupe identity shared by every merge observer: reconcile now records one merge event per linked merged PR with authoritative mergedAt, head/merge-commit shas, and linkSource provenance instead of first-PR-wins with number-only metadata. Add the source-neutral read contract (dispatch.issue-lifecycle-timeline/v1): a bounded projection and GET /api/issues/[issueId]/lifecycle exposing first/subsequent claims, done/close/reopen milestones, all linked merged PRs, and claim-to-done / claim-to-merge intervals in milliseconds with explicit provenance. A missing first claim is unknown, never zero; a close without an observed merge is closed-unmerged, never shipped. Add a bounded, idempotent, operator-only merge backfill (POST /api/issues/[issueId]/lifecycle) deriving pr_merged solely from a run record naming the PR plus that PR's persisted GitHub merge time; it never fabricates claim evidence.
…R evidence, honest link provenance (#1203) - prMergeStamp canonicalizes the identity so a Date and GitHub's second-precision string form of one merge produce the same dedupe key; reconcile and the backfill can no longer split one physical merge onto two pr_merged rows. - Wire mergedPrLinkEvidence into recordReconciledAction: production reconcile now records one merge event per explicitly linked PR. - Drop title-derived link evidence entirely; the close-driver PR records with reconcile-close-trigger provenance instead. - Present-but-unparseable mergedAt stays visible as metadata.mergedAtRaw.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M3.1-Flash-Preview (anthropic) — primary route · pr-reviewer-action v3.5.0
Findings (2 major)
| Severity | Location | Finding |
|---|---|---|
src/app/api/issues/reconcile/route.ts:271 |
Title-only matches still enter the close decision and are subsequently persisted as authoritative issue-linked merges, violating PR 1203's prohibition on inferring links from PR titles. | |
src/app/api/issues/reconcile/route.ts:396 |
Multi-PR evidence is only persisted while the issue remains open, so a linked PR merged after the first PR closes the issue is never recorded. |
Request changes: the new lifecycle API and bounded backfill are internally consistent, but reconcile still treats a title-derived association as an authoritative issue link and cannot record additional PR merges after the first merge closes the issue. Both behaviors contradict PR 1203's explicit linkage and multi-PR acceptance requirements.
Decisive findings
-
Title inference still creates authoritative merge linkage.
extractFixingIssueNumbers(pr.body ?? pr.title ?? null)puts title-only matches intomergedFixingIssues, which drivesreconcileIssue's close action. That action then feedsmergedPrintorecordPrMergedEventasreconcile-close-trigger. Relabeling a title-derived association as the observed close trigger does not prevent the title inference; it only hides its origin. The linked issue explicitly says “Do not infer links from PR titles.” Title-only PRs must not drive the close or be recorded as issue-linked merge evidence. -
A later linked PR is never recorded after the issue has closed. The route builds merge evidence for every currently fetched linked PR, but its reconciliation loop still skips every issue whose current GitHub state is not
open. Thus, if PR PR 7 closes an issue and PR PR 9 is linked and merged later, subsequent runs never callrecordReconciledActionfor that issue and PR 9 never enters the lifecycle stream. This misses PR 1203's explicit multi-PR and late/out-of-order merge behavior. Recording multi-PR evidence only while several PRs happen to be visible during the same open-state run is insufficient.
Linked Issue Fit
- Authoritative merge time: implemented by
prMergeTimestamp, and the backfill correctly requires persistedGithubPullRequest.mergedAt. - Missing claim remains unknown: implemented and directly tested.
- Closed-unmerged is not shipped: implemented and tested.
- Reopen/reclaim preserve milestones: implemented and tested.
- Typed, bounded read contract: implemented with a 500-event query window and
truncatedoutput. - Backfill uses verifiable records: implemented through issue-attached
AgentRunURLs and persisted PR merge timestamps; it does not synthesize claims. - Do not infer links from PR titles: violated by the title-to-close-to-
reconcile-close-triggerpath. - Multiple linked PRs and late/out-of-order merges: violated after the issue reaches closed state.
- Idempotency across observers/restarts: the canonical second-precision merge stamp and database unique dedupe key support replay when observers provide the same instant.
Claim Falsification
Violated: 2 of 2 PR-body claims. The pure projection, backfill, provenance, and same-instant replay tests largely hold, but each high-level claim is contradicted by a concrete production path: title-only linkage remains authoritative, and reconcile does not capture every linked merge over the issue lifecycle.
Standards Compliance
- API responses are JSON with appropriate 401/403/404/500 handling.
authorizeRequestandrejectAgentCallerenforce authenticated reads and operator-only mutation.- The backfill does not read or persist secrets.
- The change follows the existing Prisma and lifecycle-module organization.
Lint,Typecheck,Build,Tests, and the database checks are reported successful at the reviewed head.
Requirement Coverage
Lint/typecheck blocks CI; must pass: satisfied by the successful Lint and Typecheck results for1c8b9a5, with no contrary implementation finding.DISPATCH_AGENT_TOKENandGITHUB_TOKENmust never be logged, echoed, or persisted: satisfied within the available diff and inspected lifecycle/backfill paths; no changed code reads, logs, or stores either value.
Tool Harness Findings
Native inspection confirmed that recordPrMergedEvent is the shared recorder, backfill writes use it, GithubPullRequest.mergedAt is the persisted authoritative merge source, operator mutation rejects bearer callers, and the lifecycle dedupe key has a database unique index. Inspection also confirmed the title fallback and the issue.state !== "open" skip that produce the two findings. The 24-call investigation budget was exhausted before all potentially related callers and test integrations could be reviewed.
Unknowns or Needs Verification
smokeis still pending in the supplied CI results; this review does not infer that it failed.- Linked example GitHub URLs could not be fetched, but they are fixture URLs rather than required upstream behavioral evidence.
- Additional historical lifecycle writers beyond the searched call sites were not exhaustively reviewed before the tool budget was exhausted; the two reported defects are directly established from the production paths read above.
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head 1c8b9a5. Verified two correctness defects in the code, consistent with the automated review; all completed exact-head CI checks are green but smoke is queued. CI green does not address these behavioral gaps.
-
Title-only association becomes an authoritative merge link (major).
src/app/api/issues/reconcile/route.ts:267-276usesextractFixingIssueNumbers(pr.body ?? pr.title ?? null)to fillmergedFixingIssues, whilebodyKeywordNumbersintentionally excludes title matches frommergedPrLinkEvidence. HoweverreconcileIssue(...mergedFixingIssues)still closes the issue for a title-only match, andrecordReconciledAction:90-109promotes the close driver into apr_mergedevent withlinkSource: "reconcile-close-trigger". This is still title inference, merely relabeled as an observed action, contrary to #1203's explicit no-title-link requirement. Remove title-only matching from the authoritative close/link path (or separate legacy close policy from trusted linkage without writing apr_mergedclaim). Add a no-body, title-"Fixes #42" merged PR regression: no inferred authoritativepr_mergedfor #42 and no incorrect closure if not explicitly linked. -
Later linked PR merges are invisible once an issue is closed (major).
src/app/api/issues/reconcile/route.ts:335-336skips non-open issues. The onlymergedPrLinkEvidencewrite is insiderecordReconciledAction, and that function writespr_mergedonly for a successfulclose_issueaction. After PR #7 closes issue #42, a second explicitly linked PR #9 merged on a later run produces no event; even several linked merges in the same run are lost unless that run executes a successful close. The POST backfill is a manual, issue-specific path requiring an issue-attachedAgentRun.touchedIssueUrlsreference, not a complete substitute. Decouple read-only authoritative merge evidence recording from issue-close actions and open-state gating, retaining bounded scans, idempotency keys and no GitHub mutation of closed issues. Cover sequential linked PR merges (#7 closes first; #9 merges later), out-of-order observations, and repeated reconciles yielding exactly one event per PR.
Please push a new head with regression tests for both. Preserve the bounded timeline API, unknown provenance, and existing status-transition semantics.
Note: requeue from queue page Updated automatically by Dispatch on 2026-10-20T14:23:24.405Z |
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (2 major, 1 minor)
| Severity | Location | Finding |
|---|---|---|
src/app/api/issues/reconcile/route.ts:90 |
A title-only matched PR that drives a close is still recorded as an authoritative pr_merged event (reconcile-close-trigger), contrary to PR 1203's no-title-link requirement. | |
src/app/api/issues/reconcile/route.ts:335 |
Merge evidence recording is coupled to close actions and open-state gating, so linked PRs merging after the issue closes produce no pr_merged event. | |
| Minor | src/app/api/issues/reconcile/route.test.ts:330 |
No regression test for a no-body, title-'Fixes PR 42' merged PR asserting no inferred pr_merged and no incorrect closure. |
Request changes. The outstanding change request (review 5563081926) identifies two correctness defects that the current head still exhibits; the diff explicitly embraces the first rather than fixing it.
- Title-only match still yields an authoritative
pr_merged(major).src/app/api/issues/reconcile/route.tsstill fillsmergedFixingIssuesfromextractFixingIssueNumbers(pr.body ?? pr.title ?? null), andrecordReconciledActionnow records the close-driver PR aspr_mergedwithlinkSource: "reconcile-close-trigger"even whenmergedPrsis null ("A close whose only link was a title match therefore still gets its (authoritative) merge recorded"). Issue PR 1203 says "Do not infer links from PR titles"; relabeling the title-driven close as an observed merge link does not satisfy that, and the new test only asserts the label is nottitle-keyword— it does not cover a no-body, title-"Fixes PR 42" PR as the reviewer requested. - Later linked merges after close are still invisible (major).
mergedPrLinkEvidenceis only consumed viarecordReconciledAction, which writespr_mergedonly on a successful close, and reconcile still skips non-open issues; a second linked PR merging after the issue closes records nothing. The POST backfill requires an issue-attachedAgentRunand is not a substitute.
The read contract, bounded window, unknown-provenance handling, and operator-only backfill look well built and well tested; the defects are confined to the reconcile write path.
Linked Issue Fit: acceptance criteria for multi-PR/late-merge timelines are met on the read side, but the no-title-link and multi-PR write criteria are not fully met per above.
Unknowns or Needs Verification: the truncated diff hides the reconcile open-state skip and the tail of issue-lifecycle-read.ts; the second defect's line numbers are from the reviewer and should be re-confirmed at head, though the diff's structure supports the concern.
Outstanding Human Change Requests
- @joryirving's change request (1c8b9a5, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
joryirving
left a comment
There was a problem hiding this comment.
Request changes on exact head e4f43d4bfd0f. CI, Security Audit, Build Dispatch Image, PR Smoke, and AI PR Review all pass, but two major lifecycle-correctness findings from the prior review remain in this head.
Major 1: title-only matches still close issues and become authoritative pr_merged links
src/app/api/issues/reconcile/route.ts still calls extractFixingIssueNumbers(pr.body ?? pr.title ?? null) when building mergedFixingIssues. When a PR has no body and its title says Fixes #42, that title-derived match enters the map used by reconcileIssue() to decide whether to close issue 42. bodyKeywordNumbers correctly excludes that match from mergedPrLinkEvidence, but when the close succeeds, recordReconciledAction() falls back to the close-driving mergedPr and writes it as pr_merged with linkSource: "reconcile-close-trigger".
Renaming the provenance does not make a title inference authoritative. The linked #1203 contract explicitly says title-derived matches are not links. A title-only match must not drive the authoritative close/link path.
Required: remove title-only matching from the close decision and merge-link evidence path. Add a route-level regression with a merged PR whose body is null/empty and title is Fixes #42; assert it neither closes issue 42 nor emits a pr_merged event for it.
Major 2: merge evidence is still coupled to a successful close action and open-issue processing
mergedPrLinkEvidence is only consumed by recordReconciledAction(), which is called from the action loop. That function writes pr_merged events only inside the successful close_issue branch. The outer reconcile loop also skips every issue whose current GitHub state is not open. Consequently, after PR #7 closes issue #42, a later explicitly linked PR #9 can merge and subsequent reconciliation will never record #9's merge. Even for an open issue, a linked merge is lost if that reconciliation pass has no successful close action.
The documented backfill is not equivalent to continuous reconciliation: it requires an AgentRun attached to the issue whose touchedIssueUrls names the PR. Explicit body-keyword/branch evidence for a later merge does not guarantee that such a run record exists. This contradicts #1203's multi-PR requirement and the goal of recording every linked authoritative merge.
Required: decouple idempotent pr_merged event recording from issue-close execution and open-state gating. Reconcile the bounded linked-merge evidence for already-closed issues too, without mutating their GitHub status; keep the shared canonical dedupe key so repeated observations upsert one event. Add regression coverage for sequential merges (#7 closes the issue, #9 merges later), a closed issue, and repeated reconciliation producing exactly one event per PR.
These are behavioral gaps, not CI/traceability gaps. Please push fixes and request re-review on the new head.
…oncile (#1203) Review finding on #1296: reconcile only iterates open issues, so a linked PR that merges after its issue closed was never recorded and the timeline lost its authoritative merge — violating #1203's late/out-of-order merge acceptance. A bounded late-merge pass inside the same reconcile run now records pr_merged for linked numbers the open loop did not process, via the shared recordPrMergedEvent path with no close/status writes. Evidence is re-derived from the already-fetched merged-PR window (no extra GitHub calls) and replays idempotently onto the canonical merge dedupe key.
|
Responding to the two majors in the AI review at head 1. "Title inference still creates authoritative merge linkage" — not changed; rationale below. The title fallback feeding the close decision ( #1203's "Do not infer links from PR titles" governs link evidence, and link evidence now excludes title matches entirely — only 2. "A later linked PR is never recorded after the issue has closed" — fixed in
|
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (1 major, 1 minor, 1 info)
| Severity | Location | Finding |
|---|---|---|
src/app/api/issues/reconcile/route.ts:331 |
Title-derived matches still feed the close decision (`pr.body ?? pr.title`) and the close-trigger PR is still recorded as an authoritative `pr_merged` event, contrary to the outstanding human change request and PR 1203's no-title-links rule. | |
| Minor | src/app/api/issues/reconcile/route.test.ts:314 |
The requested regression (merged PR with null body and title `Fixes #42` asserting no close and no pr_merged) is not present. |
| Info | src/app/api/issues/reconcile/route.ts:538 |
Merges outside the fetched PR window (maxItems 100) are only recoverable via the operator backfill; confirm downstream PR 1183/PR 1184 consumers accept that bound. |
Recommendation: request changes. The outstanding human change request (review 5577078043) is half-addressed: the late-merge gap is fixed with solid tests, but title-only matching still drives the close decision and the close-trigger PR is still written as an authoritative pr_merged row, which the reviewer explicitly required removed.
Change-by-change findings
- Major (not addressed):
src/app/api/issues/reconcile/route.tsstill callsextractFixingIssueNumbers(pr.body ?? pr.title ?? null)intomergedFixingIssues, andrecordReconciledActionstill records the close-driving PR aspr_mergedwithlinkSource: "reconcile-close-trigger"when no explicit evidence exists. The requested regression (merged PR with null body, titleFixes #42, asserting no close and nopr_merged) is absent. The PR description argues this is pre-existing PR 1202 behavior and that the provenance label is honest; that is a defensible design position, but the human reviewer decides whether their request stands, and it has not been withdrawn or satisfied. - Addressed: late merges after close are now recorded via
recordClosedIssueMergeEvidence(reconcile/route.ts), re-deriving links from the already-fetched PR window with no extra GitHub calls, no status writes, and replay idempotence; covered by helper-level and POST-level tests including repo scoping and processed-number skip.
The read contract (GET /api/issues/[issueId]/lifecycle), operator-only backfill (agent bearer 403, rate-limited, verifiable records only), and docs match PR 1203's acceptance criteria: unknown first claim stays unknown, closed-unmerged is never "shipped", intervals are milliseconds with provenance, and the read is bounded at 500 events.
Outstanding human change requests
- joryirving (5577078043), Major 1 — not addressed (see above).
- joryirving (5577078043), Major 2 — addressed by
recordClosedIssueMergeEvidenceand its tests.
Unknowns or Needs Verification
- Merges outside the fetched PR window (maxItems 100) remain backfill-only; confirm that window is acceptable for PR 1183/PR 1184 consumers.
- CI (Tests, Typecheck, Lint, Database integration) passes at head
253c2a9, but the title-match regression the reviewer asked for does not exist in the diff.
Outstanding Human Change Requests
- @joryirving's change request (e4f43d4, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
…m close execution (#1203) Review-required changes on #1296: - Major 1: PR titles no longer feed mergedFixingIssues. A merged PR whose only correlation to an issue is its title neither closes the issue nor records a pr_merged event for it; only body keywords and branch-name patterns are link/close evidence (#1203). - Major 2: recordClosedIssueMergeEvidence becomes recordLinkedMergeEvidence and drops the processed-issue-number skip: pr_merged recording is decoupled from close execution and open-state gating. The pass records every linked merged PR in the fetched window whether the issue is open or closed and whether the close action executed; rows the close path wrote converge on the same canonical dedupe key (upsert is a no-op on conflict). - Regressions: title-only PR closes/records nothing (route level); merge recorded even when the close action fails (route level); one event per PR across sequential merges and repeated reconciliation.
|
Both majors from the re-review (head Major 1 — title-only matches no longer drive close or link (implemented as required). Major 2 — merge evidence decoupled from close execution and open-state gating. Also added on review: a warn when a linked number has no cached issue row so dropped evidence is visible instead of silent. Local validation at |
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (1 minor, 1 info)
| Severity | Location | Finding |
|---|---|---|
| Minor | src/app/api/issues/[issueId]/lifecycle/route.ts:152 |
The backfill's githubPullRequest.findMany loads every PR for the repo without a take bound; consider limiting to merged PRs or a window. |
| Info | Merges outside the fetched 100-PR window are intentionally left to the operator backfill; confirm operators know to run it for older issues. |
Recommendation: approve. Both outstanding human change requests are resolved at head 4d5e078 with route-level regressions in the diff.
- Major 1 (title-only matches):
src/app/api/issues/reconcile/route.tsnow buildsmergedFixingIssuesfromextractFixingIssueNumbers(pr.body ?? null)only — the?? pr.titlefallback is gone — and the new test "never closes or records a merge from a title-only match" assertsissuesClosed: 0, zero lifecycle rows, and no executed actions for a body-null, title-Fixes #42PR. - Major 2 (merge evidence coupled to close/open state):
recordLinkedMergeEvidenceruns after the action loop over every linked number in the fetched window, records merges regardless of close success or issue state, and converges on the sharedprMergedDedupeKey. Regressions cover failed close, same-run convergence, and replay idempotence.
The implementation matches PR 1203's acceptance criteria: no title inference, no worker-report merges, unknown first claim stays { at: null, provenance: "unknown" }, closed-unmerged is never "shipped", multi-PR and out-of-order fixtures are covered, and the read path is bounded (500 events, truncated flag). The backfill is operator-only (agent bearer 403, tested) and derives merges only from persisted GithubPullRequest.mergedAt.
Linked Issue Fit
Satisfies PR 1203's implementation and acceptance items as evidenced by the new tests in issue-lifecycle-read.test.ts, lifecycle-backfill.test.ts, and the reconcile route tests; docs updated in docs/issue-lifecycle-events.md.
Unknowns or Needs Verification
- CI rows shown are for head
4d5e078and all pass; the author's own local counts are consistent with that. No gaps blocking merge. POSTbackfill loads all PRs for the repo without atakebound — minor, noted in findings.
Outstanding Human Change Requests
- @joryirving's change request (e4f43d4, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
…overy (#1203) The operator backfill loaded every PR row of the repo with no bound. It now loads only the rows for the PR numbers the issue's runs touch, and takes the repo's merged-PR total from an aggregate count, so no unbounded row set is materialized. recorded / skippedUnmerged / skippedNoLink keep their documented meanings: skippedNoLink is the repo's merged-PR total minus the merges this issue recorded. Documents the reconciler's bounded merged-PR window (100 most recently updated closed PRs; merged PRs come from the closed side) and the operator path for merges outside it, including the precondition the backfill cannot bypass — a merge time already persisted on the PR row (#1313 tracks the sync gap that leaves it unpersisted for PRs the sync saw while open).
|
Addresses the two findings left open by the last AI review on Minor — the backfill's PR read is bounded. Info — the window and the operator path are documented. One precondition the docs now state plainly (and #1313 tracks): the automation sync upserts open PRs only, and an open PR never carries a merge, so a row it wrote holds no Local validation at Review of this delta: an independent same-model reviewer returned GO-with-minors — a docs over-promise about the backfill's reachable coverage and two missing regression cases. Both are fixed in this commit (the docs now state the persistence precondition; the new cases pin a rejected-row count and a scoped-but-unmerged row). Same-model limitation noted: no model-family diversity was available for that review. Remaining external verification: CI on |
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (1 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/lifecycle-backfill.ts |
Backfill coverage depends on the PR 1313 sync gap (open-PR-only upserts never persist mergedAt); confirm the documented operator-recovery limitation is acceptable before relying on backfill for older merges. |
Recommendation: approve. Both majors from the outstanding human change request are implemented at this head and pinned by route-level regressions; CI (typecheck, tests, coverage, database integration, migrations, lint) is green on e1dd146.
Change-by-change findings
- Major 1 (title-only matches): the close/link path now uses
extractFixingIssueNumbers(pr.body ?? null)— the?? pr.titlefallback is gone — and the regression "never closes or records a merge from a title-only match" asserts zero closes, zero lifecycle rows, and no executed actions for a body-less PR titledFixes #42. - Major 2 (merge evidence coupled to close):
recordLinkedMergeEvidenceruns a bounded pass over every linked number in the fetched merged-PR window regardless of issue state or close outcome; regressions cover merge recorded on failed close, convergence to onepr_mergedper PR when the close also succeeds, and idempotent replay across sequential merges. - The backfill's PR read is bounded to numbers the issue's runs touch, with the repo merged-PR total from an aggregate
count(); the sync-gap precondition (open-PR-only upserts never carrymergedAt) is documented and tracked as PR 1313, out of scope here.
Human review dispositions: both requests from joryirving are addressed at this head (see human_review_dispositions); the human reviewer decides whether they still stand.
Sources: PR diff (src/app/api/issues/reconcile/route.ts, route.test.ts, src/lib/lifecycle-backfill.ts, src/app/api/issues/[issueId]/lifecycle/route.ts), linked issue PR 1203, PR description, CI check results on e1dd146.
Outstanding Human Change Requests
- @joryirving's change request (e4f43d4, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
|
Courier: already addressed (run courier-3c38bf48fc3d63f0c3d00645351ef931) PR #1296 (misospace/dispatch) is already complete and published: head e1dd146 on courier/misospace/dispatch/issue-1203 is both local HEAD and the PR head (verified via GitHub API; PR open, non-draft). All assigned review feedback is addressed: joryirving's two CHANGES_REQUESTED majors were implemented at 4d5e078 (title-only matches removed from close/link; merge evidence decoupled from close execution via recordLinkedMergeEvidence) and the follow-up minor (bounded backfill PR read) at e1dd146, each with on-thread responses; the latest AI review at this head is APPROVED with only an info-level item tracked as issue #1313 (out of scope, filed). Coordinator-owned local validation at this worktree/revision: npm run typecheck exit 0; npm run lint 0 errors (1 pre-existing warning in src/app/login/page.tsx, untouched); npx vitest run of the 5 changed-file test files (lifecycle-backfill, lifecycle route, issue-lifecycle-read, issue-lifecycle, reconcile route) -> 100 passed; npm test -> 4783 passed / 24 skipped / 1 todo. No commits or pushes were needed. Remaining external verification explicitly left pending: CI on e1dd146 (previous head 4d5e078 was green; this head's checks pending, not claimed green) and joryirving's human re-review. |
joryirving
left a comment
There was a problem hiding this comment.
REQUEST CHANGES on exact head e1dd146fbe252a41f88182f9602b96648a618818. The two prior major findings are addressed: title-only matches no longer drive closure/linkage, and the bounded merge-evidence pass is decoupled from close execution and issue-open state. All five reported workflows pass. I found two remaining correctness gaps in the new contract:
- The timeline reports
delivery: "unknown"for a closed-unmerged issue that was closed, reopened, and then closed again.closedAtintentionally preserves the first close milestone, but delivery compares that first close to the last reopen instead of considering a subsequent close. Keep the first milestone if that is the contract, but derive current delivery from the latest close/reopen ordering; add a close → reopen → close regression expectingclosed-unmergedwhen the current issue state is closed and no PR merged. - Branch-name merge evidence is conditionally skipped when
mergedFixingIssuesalready has a key equal topr.number. That map is keyed by issue number, so an unrelated body reference to issue #42 can suppress a merged PR #42's valid branch link to issue #99. The new multi-PR evidence pass then misses #99's merge entirely. Branch evidence collection should not be gated on a key collision in the close-decision map; keep first-PR-wins for closure separately and add a collision regression.
Both are behavioral correctness issues in the timeline/linkage this PR introduces, so I would fix them before merging.
…ce ungated (#1203) Addresses the two open findings on PR #1296 (exact head e1dd146): - delivery now follows the latest close/reopen ordering, so close -> reopen -> close reports closed-unmerged again instead of unknown. `closedAt` still reports the first close milestone. - branch-name merge evidence is collected independently of the close-decision map, so a PR whose own number collides with an issue number in that map still records its branch link. The close decision stays first-PR-wins per issue number.
…each (#1203) Follow-up to the review findings on PR #1296: - The issue cache's closedAt now counts toward the latest close only when the issue is closed, the same gate the `closedAt` milestone uses, so delivery never certifies a close the milestone refuses to. - The delivery ladder is state-based when there is no close evidence at all, so a reopen without a close no longer turns a closed issue unknown. - The close-after-reopen note stays silent for a merged delivery. - The reconciler's widened close reach in the branch-collision case is stated in the route comment and pinned by a test.
|
Run handoff — head Took over PR #1296 and closed the two open findings from @joryirving's review of What changed
Regressions added — Local validation at
The 4 skipped files are the Postgres-backed integration suites: they need a live database, which this environment has none of, so they are CI-only here. Review of this delta — an independent same-model reviewer returned GO-with-minors (no blocker, no major). All five minors it raised are fixed at Remaining external verification (explicitly pending)
Nothing here was merged — the PR is left open for operator verification and human review. |
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: glm-5.3-flash (openai) — fallback (primary failed) · pr-reviewer-action v3.6.0
Findings (1 info)
| Severity | Location | Finding |
|---|---|---|
| Info | src/lib/lifecycle-backfill.ts |
Backfill coverage depends on PR 1313 (sync persisting mergedAt for closed PRs); confirm the tracked follow-up lands before relying on backfill for legacy issues. |
Approve. Both findings in joryirving's change request are resolved at head d789ae1 with pinned regressions: the branch-link collision fix is visible in the reconcile route diff and covered by the new collision tests in src/app/api/issues/reconcile/route.test.ts (branch link survives a PR/issue number collision; branch-linked issue still closed), and the delivery fix is specified in the PR description (latest close = later of last observed close and the cache's closedAt, cache gated on closed state) with regressions added in src/lib/issue-lifecycle-read.test.ts. CI is green on this head (tests, typecheck, lint, migrations, build, database integration). The known sync gap — open-PR-only upserts mean GithubPullRequest.mergedAt may be missing for older merges, limiting backfill coverage — is documented in docs/issue-lifecycle-events.md and tracked as PR 1313; it is a pre-existing limitation, not a defect this PR introduces.
Outstanding Human Change Requests
- @joryirving's change request (e1dd146, head moved since) is not shown addressed at this head; it needs the reviewer's own re-review.
|
Courier: already addressed (run courier-811388e2b0cba6868633fc2417a65c24) PR #1296 (misospace/dispatch) is open, non-draft, head d789ae1 on courier/misospace/dispatch/issue-1203 == local HEAD (clean tree). Both of joryirving's change-request findings are fixed at b82754e/d789ae1 with pinned regressions and on-thread replies; both review threads are resolved. AI reviewer (its-saffron[bot]) APPROVED at this exact head; its single info finding (backfill coverage depends on #1313 sync persisting mergedAt) is pre-existing, documented in docs/issue-lifecycle-events.md and the PR body, and tracked externally. All 13 CI checks are green at d789ae1 (Tests, Database integration, Typecheck, Lint, Database migrations, Build, Workflow lint, Coverage, review, Docker Build, Docker Build (MCP), npm audit, smoke). Fresh local validation this run at d789ae1 after npm ci (exit 0): npx vitest run of the 5 lifecycle suites (issue-lifecycle-read, reconcile route, issue-lifecycle, lifecycle-backfill, lifecycle route) -> 5 files / 106 tests passed; npm run typecheck -> exit 0; npm run lint -> 0 errors (1 pre-existing warning in src/app/login/page.tsx, untouched). Remaining blocker is external only: joryirving's CHANGES_REQUESTED review requires the reviewer's own re-review to dismiss (mergeable_state blocked); no code work is outstanding for this run. |
joryirving
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d789ae1 after prior REQUEST_CHANGES. Both blocking correctness issues are fixed: the lifecycle projector preserves the original first-close milestone but derives current delivery from the most recent close/reopen order (including the cache-only second close), and merged-PR branch evidence is now collected independently of close-decision-map key collisions. Dedicated close→reopen→close and PR-number/issue-number collision regressions cover the formerly broken outcomes. Title-only matches remain excluded; first-PR-wins close selection and idempotent event recording remain. All required workflows passed on this head. No remaining blocking findings.
…s an authoritative merge time (#1320) * fix(automation-sync): persist merged PRs so lifecycle backfill has merge times (#1313) The automation sync sourced PR rows only from the state=open listing, so every GithubPullRequest row it wrote carried mergedAt: null, and a PR that later merged dropped out of the listing without ever being updated. The lifecycle backfill added in #1296 correlates a run's touched PR against the persisted mergedAt and therefore found no merge evidence for any PR the sync had seen. Also fetch the recently updated closed listing (one added call per repo per sync, degrading gracefully like packages on failure) and upsert the rows that carry merged_at, reusing the existing upsert write path. A PR that merges between the two concurrent listings is upserted once with the merged row winning, never written back to mergedAt: null. * test: restore full @/lib/github mocks so route tests exercise the real automation sync (#1313) The scheduled/automation sync route tests mock @/lib/github wholesale; the new unconditional closed-PR fetch would resolve to a missing export and fail the sync inside a catch path, so the happy-path tests passed without ever running a successful sync. Add the missing mock to both factories and pin the scheduled route's automation result to {synced: 1, failed: 0}. Also pin state: closed on the both-listings upsert so 'merged row wins' is fully asserted. --------- Co-authored-by: Courier <courier@localhost>
Summary
Builds on the durable
IssueLifecycleEventstream from #1202 and delivers the source-neutral lifecycle read contract for #1183/#1184: authoritative PR-merge linkage (multi-PR, explicit provenance), a typed timeline export, and a bounded operator-only backfill. Closes #1203.recordPrMergedEvent+ canonicalprMergedDedupeKey/prMergeStampinsrc/lib/issue-lifecycle.ts. Reconcile records onepr_mergedper linked merged PR with GitHub'smergedAt,mergeCommitSha/headSha, and alinkSource(body-keyword,branch-name).linkSource: "reconcile-close-trigger"(the observed action, never a text guess).recordLinkedMergeEvidenceruns a bounded pass inside the same reconcile run over every linked number in the fetched merged-PR window (no extra GitHub calls): a linked merge records whether the issue is open or closed and whether the close action executed. Rows the close path wrote converge on the same canonical dedupe key (upsert is a no-op on conflict) — repeated reconciliation yields exactly one event per PR. Merges outside the window remain the operator-only backfill's territory.dispatch.issue-lifecycle-timeline/v1: pure projectionsrc/lib/issue-lifecycle-read.ts+GET /api/issues/[issueId]/lifecycle— first/subsequent claims, done/close/reopen milestones, all linked merged PRs,claimToDoneMs/claimToMergeMsin milliseconds with per-timestamp provenance (observed/backfilled/unknown). Missing first claim is{ at: null, provenance: "unknown" }— never zero, never issue-creation time. A close without an observed merge isclosed-unmerged, never "shipped".deliveryfollows the latest close/reopen ordering — the latest close is the later of the last observed close and the issue cache'sclosedAt(the cache counts only for a closed issue) — so a close after a reopen reportsclosed-unmergedagain, whileclosedAtkeeps reporting the first close milestone. Timeline is invariant to event input order; duplicateapi:*-vs-webhookobservations collapse with a note. Read is bounded (500-event window,truncatedflag).POST /api/issues/[issueId]/lifecycle(operator auth only; agent bearer 403): derivespr_mergedsolely from verifiable records — anAgentRunattached to the issue naming a PR URL, correlated with that PR's persisted GitHub merge time (GithubPullRequest.mergedAt). Handles the html-URL vs API-URL shape split. Never fabricates claim evidence, skips PRs without an authoritative merge time, idempotent across replays.count()instead of a row scan — no unbounded row set is materialized.recorded/skippedUnmerged/skippedNoLinkkeep their documented meanings;skippedNoLinkis the repo's merged-PR total minus the merges this issue recorded.mergedAtand a PR that later merges is not updated either — the backfill recovers a merge only where the merge time is already in the table. fix(automation-sync): persist merged pull requests so the lifecycle backfill has an authoritative merge time #1313 tracks closing that sync gap; the reconcile path is unaffected because it readsmerged_atstraight from GitHub.docs/issue-lifecycle-events.mdgains the read-contract and merge-linkage sections, the reconciler's bounded merged-PR window, and the operator recovery path for merges outside it.Invariants this change keeps (and the other paths enforcing the same rule)
pr_mergedonly ever from an authoritative merge observation — never a worker report, never a title inferencetasks/report/report-close.ts(writescompleted/attempt_failedonly); sync/webhook (recordSyncedIssueEventswrites nopr_merged)mergedAt. Title-derived matches produce no close and no link evidence.reconciler|<repo>|<n>|pr_merged|<pr>|<raw merged_at>|"unknown"prMergeStampcanonicalizes.000Z→Zso aDateand GitHub's second-precision string of one instant produce byte-identical pre-#1203 keys; reconcile (close path and decoupled pass) and backfill sharerecordPrMergedEvent. Tests pin the exact legacy key string and Date↔string convergence.move,status,groom, claim/unclaim, groomer, webhookrecordStatusTransitionand the unchangedissue_closed/status_changeddedupe keys; the read path, backfill, and merge-evidence pass write no status events.reconcileIssuefed bymergedFixingIssuesmergedFixingIssuesis body-keyword/branch-name only; title-only matches cannot close. The branch claim still feeds the close decision under the same first-PR-wins-per-issue-number rule, now that the collision no longer suppresses it.pr_mergedwithbackfilled: trueand never readsstartedAt/createdAt; tests assert zeroclaimrows from backfill.admission-override(samerejectAgentCallergate); lane route bearer authrejectAgentCaller+ rate limit (tested 403); GET usesauthorizeRequestandtake-boundedfindMany; the backfill's PR read is scoped to the touched PR numbers.Local validation (at head
d789ae1)npx vitest run src/lib/issue-lifecycle-read.test.ts src/app/api/issues/reconcile/route.test.ts src/lib/issue-lifecycle.test.ts src/lib/lifecycle-backfill.test.ts "src/app/api/issues/[issueId]/lifecycle/route.test.ts"— 5 files, 106 passed.npm test— 194 files passed / 4 skipped; 4791 tests passed / 24 skipped / 1 todo (vitest). The 4 skipped files are the Postgres-backed integration suites, which need a live database and run in CI.npm run typecheck— exit 0.npm run lint— 0 errors (1 pre-existing warning insrc/app/login/page.tsx, untouched).CI was green on the previous head
e1dd146(all 13 checks). CI on this head (d789ae1) is pending — not claimed green.Two findings from @joryirving's review of
e1dd146— thedeliveryderivation for close → reopen → close, and the branch-evidence collision with the close-decision map — are implemented atb82754e, with the follow-ups atd789ae1(the issue cache'sclosedAtcounts toward the latest close only for a closed issue, the delivery ladder is state-based when no close evidence exists, the close-after-reopen note stays silent for a merged delivery, and the reconciler's widened close reach in the collision case is stated and pinned by a test). An independent same-model review of that delta returned GO-with-minors; all five minors it raised are fixed. Same-model review limitation noted — no model-family diversity available.Known limits (deliberate, documented)
POST /api/issues/{issueId}/lifecyclefor that issue. Both preconditions must hold: anAgentRunattached to the issue whosetouchedIssueUrlsname the PR, and that PR's persistedGithubPullRequest.mergedAt— which is only there if it was persisted before the PR dropped out of the sync's open-PR listing (fix(automation-sync): persist merged pull requests so the lifecycle backfill has an authoritative merge time #1313).truncated: true; full export is follow-up work.mergedAtRawpreserves present-but-unparseable source stamps instead of dropping provenance.Handoff
Settled decisions: shared
reconciler\|…merge-key namespace (historical, deliberate); title-derived matches removed from the close decision per review (a deliberate #1202-behavior change, demanded and now implemented); merge evidence decoupled from close execution and open-state gating per review; issue-cache fallback reads keepprovenance: "observed"; the backfill's PR read is scoped to the touched PR numbers with the repo merged total taken as an aggregate count, preserving the response contract;deliveryfollows the latest close/reopen ordering whileclosedAtkeeps the first close milestone; branch-name link evidence is collected independently of the close-decision map, with the close decision unchanged in kind (first-PR-wins per issue number).Remaining external verification: CI on this head (
d789ae1) — green one1dd146, pending here and not claimed green — and @joryirving's re-review of the two findings addressed atb82754e/d789ae1.