ENG-760 - [build] Board and History read one stored fact — the PR's resolution, not druks's opinion of it

[druks-operator]

Linear ticket: ENG-760

Plan

Decision

Implement this as one coherent migration/read-side/wire/UI change. The stored source of truth becomes the PR outcome reported by GitHub, while run state remains the source of liveness. Done is checked first; only unresolved work is eligible for the active Board.

Revision 3 note: the repo owner's review overturned one prior ruling. Re-dispatching a work item onto a genuinely new run now clears any stored pr_merged/pr_resolved_at — the old PR's outcome no longer describes the new attempt. pr.closed remains the only writer of a non-None resolution; dispatch only ever clears the fields back to None. See "Ruled out" below for the superseded line.

The ticket is decision-complete. Its single operator comment confirms the design as understood and adds no new constraint.

Implementation

1. Replace the handoff status schema. (Unchanged from revision 2 — already landed and verified.) A new Alembic revision after d3a5c71f8e40 replaces work_items.status with nullable pr_merged: bool | None and pr_resolved_at: datetime | None, backfilling shipped→(true, updated_at) and cancelled→(false, updated_at), then drops work_items_status_idx and status. Downgrade restores the inverse mapping.

2. pr.closed writes the resolution; Build.dispatch clears it; the milestone event returns.

   In backend/druks/contrib/ship/subscribers.py, pr_close_settles_the_item keeps its dedup guard (if not item or item.pr_merged is not None: return) unchanged, but now:

   • sources pr_resolved_at from GitHub's own timestamp instead of Base.utc_now() (see point 7 below), and
   • restores the timeline milestone the old set_status used to emit, via Ship.record_event(type="shipped", subject=item) when payload["merged"] is true, or Ship.record_event(type="cancelled", subject=item) when false — call it right after the two fields are set and flushed, before ship()/close_external(). These are new literal event-type strings; nothing in the codebase currently emits "shipped"/"cancelled" (HandoffStatus and its call sites are fully gone), so there is no existing string to match against — use these two exactly, matching the enum values set_status used to pass to record_event.
   • Replace the facade-violating imports from druks.database import db_session and from druks.models import Base with from druks.db import Base, db_session (the documented author door; both names are already exported there).

   In backend/druks/contrib/ship/workflows.py, Build.dispatch (around the existing if item.build_run_id != run_id: branch that already clears branch/pr_number on a genuinely new run) also clears the resolution:

   if item.build_run_id != run_id:
       item.update(
           build_run_id=run_id, branch=None, pr_number=None,
           pr_merged=None, pr_resolved_at=None,
       )

   WorkItem.update() uses an explicit _KEEP-sentinel kwarg pattern (backend/druks/contrib/ship/models.py:357) — add pr_merged: bool | None = _KEEP and pr_resolved_at: datetime | None = _KEEP parameters and bodies to that method so both new fields can be cleared through the same call as branch/pr_number, rather than mixing a direct-attribute-assignment style into dispatch. The dedup path (item.build_run_id == run_id, a live-run redispatch) must NOT go through this branch, so an already-stored resolution survives a duplicate dispatch — this is existing behavior, unchanged.

3. Generalize the durable read query, and exclude cancelled/orphaned unresolved items from the Board.

   Run.subject_states() keeps its ranked, unfiltered {subject_id: RunState} query, but add a limit: int = 500 parameter and apply .limit(limit) to the final statement (already ordered by driving.c.created_at.desc()), so the newest-N subjects are bounded in SQL rather than materializing every subject a type has ever had a run for. Ship.list_subjects() passes its existing 500 through instead of relying only on WorkItem.list_recent(limit=500)'s bound. In backend/druks/durable/datastructures.py, Subject.list_open passes its own limit through to subject_states(subject_type, limit=limit) instead of slicing the full dict with [:limit] in Python — that slice becomes an unreachable safety cap once the SQL side is bounded, not the primary limit.

   backend/druks/models.py's StoredSubject.list_open has zero production callers anywhere in backend/druks or backend/tests (Ship.list_subjects never adopted it, and the only caller of any list_open variant is druks/contrib/review/extension.py:34, which calls the identity-only Subject.list_open in datastructures.py, a different method). Remove StoredSubject.list_open from druks/models.py — it is dead code, and its OPEN_STATES-only filtering can't serve Ship's needs anyway (Ship must keep finished-but-unresolved items, which OPEN_STATES excludes). Leave Subject.list_open (datastructures.py) untouched other than the limit threading above; Review's usage is unaffected.

   In backend/druks/contrib/ship/extension.py, fix the facade violation — from druks.durable.models import Run becomes from druks.durable import Run (also import RunState from there). Then close the real gap the repo owner flagged: list_subjects() currently excludes an item only when pr_resolved_at is not None, so a work item whose newest run is cancelled or orphaned — real, reachable RunState values with no resolution ever coming (no GitHub event fires for an internally-cancelled/orphaned run) — stays on the Board forever, and the frontend has no bucket for either state (confirmed: WorkItemsPage.tsx's needsYou/inFlight filters only match parked|failed|finished and running|scheduled respectively; a cancelled/orphaned+unresolved row renders in neither group while still inflating the raw count). Exclude both in the backend composition:

   @classmethod
   def list_subjects(cls) -> list[WorkItemSummary]:
       states = Run.subject_states(cls.subject_type, limit=500)
       items = WorkItem.list_recent(limit=500)
       return [
           WorkItemSummary.from_work_item(item)
           for item in items
           if str(item.id) in states
           and item.pr_resolved_at is None
           and states[str(item.id)] not in (RunState.CANCELLED, RunState.ORPHANED)
       ]

4. Wire contract and DashboardItem hardening. WorkItemSummary.resolution and the /api/ship/work_item / /api/ship/work-items/history shapes are unchanged from revision 2. In backend/druks/contrib/ship/schemas.py, DashboardItem.from_work_item replaces its three bare asserts with raise ValueError(...) — asserts vanish under python -O and this invariant (every DashboardItem is built from an already-resolved WorkItem) must fail loudly in production, not silently:

   @classmethod
   def from_work_item(cls, item: WorkItem) -> "DashboardItem":
       if item.pr_merged is None or item.pr_resolved_at is None:
           raise ValueError(f"work item {item.id} has no stored PR resolution")
       resolution = _pr_resolution(item.pr_merged)
       if resolution is None:
           raise ValueError(f"work item {item.id} resolution could not be derived")
       ...

   (The reviewer also floated replacing pr_merged: bool | None with a single pr_resolution: str | None column as a further simplification. That is optional, not required by any acceptance criterion here — it would re-touch the already-verified migration and ripple through more files than this revision's feedback calls for. Leave it as a follow-up unless you're already touching every call site anyway.)

5. GitHub's own timestamp, not druks's clock, backs pr_resolved_at. The normalized pr.closed signal currently carries only {"branch": ..., "merged": ...} (backend/druks/core/webhooks/github.py, on_pull_request_closed) — it does not forward GitHub's merged_at/closed_at at all, so the fix touches the webhook normalizer, not just the subscriber. Add the resolved timestamp to the published payload there, where merged is already known:

   async def on_pull_request_closed(self) -> Response:
       pull_request = self.data["pull_request"]
       await publish(
           "pr.closed",
           repo=_repo_name(self.data),
           pr_number=pull_request["number"],
           payload={
               "branch": pull_request["head"]["ref"],
               "merged": pull_request["merged"],
               "resolved_at": pull_request["merged_at"] if pull_request["merged"] else pull_request["closed_at"],
           },
       )
       return _accepted()

   In pr_close_settles_the_item, parse it instead of stamping Base.utc_now():

   item.pr_resolved_at = datetime.fromisoformat(payload["resolved_at"].replace("Z", "+00:00"))

   matching the existing ISO-parse idiom used elsewhere (backend/druks/harnesses/codex.py:167). GitHub always populates merged_at or closed_at on a closed action, so no null-handling is needed here. Test fixtures that build a raw GitHub payload (tests/ship/test_webhooks_pull_request.py's _fire_closed helper) need merged_at/closed_at added to their pull_request dict.

6. Render facts in the frontend. Unchanged from revision 2 — WorkItemsPage.tsx filters resolved summaries first, buckets parked/failed/unresolved finished as needs-you and scheduled/running as in-flight, total = needsYou.length + inFlight.length. HistoryPage.tsx/StatusTag.tsx render merged/closed. (Optional nit, not required for any AC: a one-line comment on why unresolved finished lands in needs-you, since a cancelled/orphaned+unresolved row can no longer reach the wire, this bucket only ever holds genuine review-only work now.)

7. Update tests.

   • backend/tests/ship/test_build_dispatch.py: test_dispatch_does_not_clear_a_stored_pr_resolution asserts the exact behavior this revision reverses (pr_merged is False / pr_resolved_at is not None survive a dispatch to a brand-new run) — replace it with test_dispatch_pulls_cancelled_item_back_onto_the_board: seed a work item, close its PR (pr_merged=True, pr_resolved_at set), dispatch a new build (stub Build.start to return a fresh run id), assert pr_merged is None and pr_resolved_at is None, and assert the item is present in Ship.list_subjects(). Add a companion assertion to test_duplicate_dispatch_keeps_the_live_attempt_routing (the dedup path) confirming a stored resolution survives when dispatch hands back the same live run.
   • backend/tests/ship/test_webhooks_pull_request.py: extend the redelivery test coverage and _fire_closed's payload with merged_at/closed_at; assert pr_resolved_at equals the parsed GitHub timestamp, not the webhook-receipt time; add an assertion that pr_close_settles_the_item writes an Event row (type="shipped" or "cancelled") for the item.
   • backend/tests/ship/test_build_board_membership.py: add test_a_cancelled_or_orphaned_build_with_no_resolution_leaves_the_board, parametrized over state in ("cancelled", "orphaned"). "cancelled" seeds directly via seed_build_run(druks_db, work_item_id=item.id, state="cancelled"). "orphaned" is not a state seed_dbos_status supports (backend/druks/testing.py) — seed a run normally, delete its workflow_status row, and backdate Run.created_at past _MISSING_STATUS_GRACE (5 minutes, backend/druks/durable/dbos_state.py) so state_expression derives ORPHANED. Assert str(item.id) not in _board_ids(druks_db) in both cases.
   • Update AGENTS.md's liveness/outcome line only if you find it no longer holds — the current wording ("An externally owned PR outcome is stored from the provider event, never inferred from run lifecycle") already survives this revision unedited: dispatch clears a stale fact, it never infers a new one.

Preserved decisions and scope

• An item with no PR and a run that has failed forever stays needs-you indefinitely unless the operator explicitly closes the ticket. That is intentional unresolved work.
• An item whose newest run is cancelled or orphaned, with no PR resolution, is excluded from the active Board entirely — neither needs-you nor in-flight, since neither bucket can act on it and nothing will ever resolve it except a fresh dispatch or a pr.closed.
• Board and History remain separate routes and pages.
• Out of scope: an inline recent-done tail in the active feed; the ticket explicitly cut it as separable and unnecessary for correctness.
• No compatibility alias for status, shipped, or cancelled is added to the new Ship API contract.

Ruled out

• Keep work_items.status and patch more lifecycle settlement paths — rejected because terminal run events cannot observe a later manual merge, which is the production failure this ticket fixes.
• Use run state alone as Board membership — rejected because a failed run later merged by hand remains terminal-failed even though the PR is resolved.
• Emit server-owned needs-you/in-flight labels — rejected because the established read-side contract emits facts (resolution and SubjectStatus.state) and Ship's frontend owns presentation.
• Merge History into the active feed or add a recent-done tail — rejected because the settled ticket explicitly keeps History as a separate route and cuts the inline tail.
• Ship both old status/shipped/cancelled and new resolution/merged/closed fields — rejected because the repository requires one canonical wire spelling and HandoffStatus must be removed.
• Clear PR resolution during dispatch, workflow start, or pr.opened — overturned in revision 3. The repo owner's review determined that a genuinely new dispatch must clear a stale resolution, since the old PR's outcome no longer describes the new attempt. pr.closed remains the sole writer of a non-None resolution; Build.dispatch only ever clears the fields back to None, and the existing dedup guard in pr_close_settles_the_item is unaffected — a subsequent pr.closed after a clearing dispatch simply writes fresh facts. Clearing during workflow start or pr.opened is still out of scope; only Build.dispatch's genuinely-new-run branch clears.

Acceptance Criteria

• AC1: A new Alembic revision after d3a5c71f8e40 adds nullable work_items.pr_merged and work_items.pr_resolved_at, backfills status='shipped' to (true, updated_at) and status='cancelled' to (false, updated_at), then drops work_items_status_idx and work_items.status; its downgrade restores the inverse mapping.
  • Verification: Inspect the migration's upgrade/downgrade operations and the WorkItem mapping.
• AC2: HandoffStatus, WorkItem.set_status/settle, the run-terminal settlement subscriber, and dispatch/start status clearing (from revision 1) are absent. pr.closed is the only production code that writes a non-None pr_merged/pr_resolved_at (establishes a resolution); Build.dispatch is the only other writer, and it only ever clears both fields to None when pointing an item at a genuinely new run (see AC8). Webhook redelivery does not rewrite an already-stored resolution.
  • Verification: Search the diff for the removed symbols; grep for every assignment to pr_merged/pr_resolved_at and confirm each site is either pr_close_settles_the_item (non-None) or Build.dispatch's new-run branch (None); read the webhook redelivery test.
• AC3: The durable read side exposes every subject's newest driving RunState as a dict[str, RunState], bounded by an explicit limit applied in SQL rather than in Python; Run.open_subject_ids remains removed. Ship's active list combines that map with one WorkItem.list_recent() result, excludes resolved items before considering run state, and preserves finished-but-unresolved items for the needs-you bucket.
  • Verification: Inspect the ranked SQL query and its LIMIT, the Ship list composition, and tests covering newest-run selection plus finished unresolved work.
• AC4: WorkItemSummary serializes resolution as exactly "merged", "closed", or null; History items expose resolution instead of the former handoff status. The Board remains at /api/ship/work_item, History remains at /api/ship/work-items/history, and History is ordered by pr_resolved_at.
  • Verification: Read the Pydantic/TypeScript contracts and API tests asserting the exact camelCase response fields and values.
• AC5: A regression test seeds a failed build, then delivers pr.closed with merged=true, and proves the item has pr_merged=True, is absent from the active Board response, and is present in History with resolution: "merged".
  • Verification: Read the focused backend regression test reproducing the failed-run/later-manual-merge sequence.
• AC6: WorkItemsPage first removes summaries whose resolution is non-null, puts parked, failed, and unresolved finished rows in needs-you, puts scheduled and running rows in in-flight, and computes the displayed active count as needsYou.length + inFlight.length. Frontend tests cover these branches.
  • Verification: Inspect WorkItemsPage and its component test using a mixed snapshot, including a resolved row with an otherwise active run state.
• AC7: History's TypeScript filters, counts, status glyph, labels, and CSS use the canonical merged/closed resolution values. Existing ship() and close_external() run cancellation, ticket synchronization, and branch-cleanup behavior remains covered while neither method writes resolution.
  • Verification: Inspect HistoryPage, StatusTag, styles, and the adapted pull-request webhook tests.
• AC8: Build.dispatch clears pr_merged and pr_resolved_at back to None whenever it points a work item at a genuinely new run (the same branch that already clears branch/pr_number), and leaves them untouched when a dispatch dedups to the same live run. A focused test seeds a resolved item, dispatches a new build, and asserts both fields are None and the item is present in the active Board response; a second test confirms a dedup dispatch preserves a stored resolution.
  • Verification: Read Build.dispatch and WorkItem.update's new parameters; read test_dispatch_pulls_cancelled_item_back_onto_the_board and the dedup companion assertion; confirm the deleted/renamed test_dispatch_does_not_clear_a_stored_pr_resolution no longer asserts the reversed behavior.
• AC9: pr_close_settles_the_item emits a timeline milestone event (type="shipped" when merged, type="cancelled" when closed unmerged) via Ship.record_event(subject=item), restoring the milestone WorkItem.set_status used to produce before its removal.
  • Verification: Read the subscriber for the record_event calls; read a test asserting an Event row with the matching type and subject_id exists after a pr.closed delivery.
• AC10: Ship.list_subjects() excludes any item whose newest run state is cancelled or orphaned and whose pr_resolved_at is None, in addition to excluding resolved items — these rows have no actionable bucket and must not reach the wire. The exclusion happens in the backend list composition, not only in frontend bucketing.
  • Verification: Read Ship.list_subjects()'s filter predicate; read test_a_cancelled_or_orphaned_build_with_no_resolution_leaves_the_board covering both states.
• AC11: DashboardItem.from_work_item raises ValueError instead of using bare assert when a work item lacks a stored resolution, so the invariant fails loudly under python -O.
  • Verification: Read schemas.py for the raise ValueError replacements; confirm no bare assert remains in from_work_item.
• AC12: subscribers.py imports Base/db_session via druks.db, and extension.py imports Run/RunState via druks.durable, matching the documented author-surface doors named in AGENTS.md. StoredSubject.list_open in druks/models.py is removed as dead code (zero production callers); Subject.list_open in datastructures.py, used by the Review extension, is unaffected other than threading its limit into Run.subject_states.
  • Verification: Grep for the corrected import lines; confirm StoredSubject.list_open no longer exists in druks/models.py and the Review extension's active-list test still passes.
• AC13: Run.subject_states() takes a limit parameter and applies it as a SQL LIMIT on the ranked, newest-first query, rather than materializing every subject of a type before Python filters/slices. Ship.list_subjects() and Subject.list_open() both pass their existing limit through instead of slicing an unbounded Python list.
  • Verification: Inspect subject_states's SQL for the LIMIT clause and its two call sites for the threaded limit argument.
• AC14: pr_resolved_at is sourced from GitHub's own merged_at/closed_at timestamp (added to the normalized pr.closed payload by the webhook handler) rather than druks's clock at webhook receipt, so History ordering is stable under redelivery and operator backfill.
  • Verification: Read on_pull_request_closed's payload construction and pr_close_settles_the_item's parsing; read a test asserting pr_resolved_at equals the payload's timestamp, not the time the test ran.

[druks-reviewer]

Contract verification — ENG-760, implementation revision 1

The substance of this change is right. All seven acceptance criteria are satisfied in the code, and the whole backend suite (982 tests) plus the frontend vitest suite (69 tests) pass locally. One thing blocks it: the new WorkItemsPage.test.tsx does not typecheck, so npm --prefix frontend run build fails — locally and in the PR's checks job (run 30277301296).

Blocking

Frontend typecheck fails on the newly added test file.

src/extensions/ship/WorkItemsPage.test.tsx(66,7): error TS2722: Cannot invoke an object which is possibly 'undefined'.
src/extensions/ship/WorkItemsPage.test.tsx(82,11): error TS2488: Type 'NodeListOf<Element>' must have a '[Symbol.iterator]()' method that returns an iterator.

Both are introduced by this diff and both are in the test that covers AC6, so the AC6 evidence itself is what breaks the build. Details and suggested shapes are in the inline comments. After fixing, npm --prefix frontend run build, npm --prefix frontend run lint, and npm --prefix frontend test should all be green.

Verified

• AC1 — a4c9e2f7b1d6 chains off f2c8b81a9d4e (which follows d3a5c71f8e40), single head. Adds nullable pr_merged / pr_resolved_at, backfills shipped → (true, updated_at) and cancelled → (false, updated_at), drops work_items_status_idx then status. Downgrade restores the inverse mapping. DateTime(timezone=True) matches the _UtcDateTime annotation map.
• AC2 — HandoffStatus, set_status, build_end_settles_the_item, any_workflow_start_returns_item_to_board, and the Build.dispatch() status reset are all gone; no stragglers under backend/druks or frontend/src. pr_close_settles_the_item is the sole writer, and test_redelivered_webhook_does_not_rewrite_resolution proves redelivery does not overwrite a stored resolution.
• AC3 — Run.open_subject_ids is replaced by Run.subject_states(), which keeps the ranked driving-run query and drops the OPEN_STATES SQL predicate. Ship.list_subjects joins that map against one WorkItem.list_recent(limit=500) and excludes pr_resolved_at is not None before looking at run state, so finished survives — covered by the parametrized test_an_unresolved_build_holds_the_board.
• AC4 — resolution serializes as merged / closed / null; DashboardItem.resolution replaces status; routes unchanged; list_handoff filters and orders on pr_resolved_at, and updated_at now carries that timestamp.
• AC5 — test_failed_build_later_merged_moves_from_board_to_history seeds a failed build, fires pr.closed(merged=True), and asserts pr_merged is True, absence from /api/ship/work_item, and resolution == "merged" in History with the resolution timestamp on the wire.
• AC6 — WorkItemsPage filters resolved rows first, buckets parked/failed/finished as needs-you and scheduled/running as in-flight, and computes total = needsYou.length + inFlight.length. The component test uses a mixed snapshot including a running row with resolution: 'merged'.
• AC7 — HistoryPage, StatusTag, and styles.css use merged/closed throughout; the pull-request webhook tests still assert run cancellation, ticket sync, and branch cleanup while asserting stored resolution instead of milestones.

Follow-up recommendations

Not blocking, and none of these map to an acceptance criterion — file separately if you agree.

1. Reopen-then-merge is unreachable. pr_close_settles_the_item returns early whenever pr_merged is not None, per the plan's dedup rule. A PR closed unmerged, then reopened and merged, stays closed in History forever. That is the specified behaviour today; worth a ticket if reopen turns out to happen in practice.
2. Facade imports. subscribers.py imports db_session from druks.database and Base from druks.models, and extension.py imports Run from druks.durable.models. Extension code elsewhere goes through druks.db and druks.durable, which AGENTS.md names as the author surface.
3. assert for wire invariants. DashboardItem.from_work_item uses bare assert for pr_merged/pr_resolved_at. Under python -O those vanish; a raised ValueError (or letting Pydantic reject the None) would fail loudly either way.
4. Unbounded state map. Run.subject_states() returns every subject of a type, and StoredSubject.list_open now applies its limit in Python over that full map rather than in SQL. Fine at current scale; worth revisiting if a subject type grows large.

Push back on any of this if you read it differently — particularly the follow-ups, which are judgement calls rather than contract violations.

[druks-reviewer]

tsc -b rejects this call: sse.handlers is typed Record<string, (data: unknown) => void> | null, and under the project's index-access strictness handlers?.snapshot is ((data: unknown) => void) | undefined, so invoking it is TS2722: Cannot invoke an object which is possibly 'undefined'. This fails npm --prefix frontend run build locally and on the PR's checks job.

Two reasonable shapes: narrow explicitly before the call (const handlers = sse.handlers; if (!handlers?.snapshot) throw new Error('snapshot handler not registered')), which also gives a clear failure if the component stops registering snapshot; or type the hoisted mock as a concrete interface with an optional snapshot and assert it non-null. The first is a bit more verbose but keeps the test honest about its precondition — your call.

The test that should cover this is the one already here: WorkItemsPage.test.tsx must pass under tsc -b, not just vitest. Please re-run npm --prefix frontend run build after the fix.

[druks-reviewer]

Second typecheck failure from the same file: TS2488: Type 'NodeListOf<Element>' must have a '[Symbol.iterator]()' method that returns an iterator. The project's lib does not include dom.iterable, so spreading a NodeListOf is not allowed.

Array.from(container.querySelectorAll('.wi-group-count')) is the dominant fix here — it needs no config change and reads the same. Widening lib in tsconfig would also work but changes compiler config for one test assertion, which I'd avoid.

Same verification: npm --prefix frontend run build must go green; the AC6 bucketing assertions in this test are otherwise exactly what was asked for.

Dismissed after implementation revision update.

[druks-reviewer]

Contract verification — ENG-760, implementation revision 2

The round-1 blocker is fixed. All seven acceptance criteria hold and the full verification profile is green.

Prior blocker — resolved

Round 1 blocked on: "Frontend typecheck fails on the newly added test file" — TS2722 on sse.handlers?.snapshot(...) and TS2488 on spreading NodeListOf. Revision 2 narrows the handler explicitly (throwing snapshot handler was not registered if the component stops registering it, which is the stronger of the two fixes I suggested) and swaps the spread for Array.from(...). npm --prefix frontend run build now passes locally and the On Pull Request Frontend check on d9bfb17 is green.

Verified

• AC1 — a4c9e2f7b1d6 (revises f2c8b81a9d4e, itself after d3a5c71f8e40; single head) adds nullable pr_merged / pr_resolved_at, backfills shipped → (true, updated_at) and cancelled → (false, updated_at), then drops work_items_status_idx and status. downgrade() restores the inverse mapping.
• AC2 — HandoffStatus, WorkItem.set_status, build_end_settles_the_item, any_workflow_start_returns_item_to_board, and the Build.dispatch() status reset are all gone; the remaining set_status hits are the unrelated ticketing-provider API. pr_close_settles_the_item is the only writer of both columns, and test_redelivered_webhook_does_not_rewrite_resolution proves redelivery leaves a stored resolution untouched.
• AC3 — Run.open_subject_ids is replaced by Run.subject_states(), which keeps the ranked driving-run query and drops the OPEN_STATES SQL predicate. Ship.list_subjects joins that map against one WorkItem.list_recent(limit=500) and excludes pr_resolved_at is not None before consulting run state, so finished survives. Covered by test_an_unresolved_build_holds_the_board and test_the_newest_run_state_speaks_for_the_item.
• AC4 — resolution serializes as merged / closed / null; DashboardItem.resolution replaces status; both routes unchanged; list_handoff filters and orders on pr_resolved_at, which also backs updated_at on the wire. Tests assert the exact camelCase values and that status is absent.
• AC5 — test_failed_build_later_merged_moves_from_board_to_history seeds a failed build, fires pr.closed(merged=True), then asserts pr_merged is True, absence from /api/ship/work_item, and resolution: "merged" in History with the matching resolution timestamp.
• AC6 — WorkItemsPage filters resolved rows before reading run state, buckets parked / failed / finished as needs-you and scheduled / running as in-flight, and computes total = needsYou.length + inFlight.length. WorkItemsPage.test.tsx uses a mixed snapshot including a running row with resolution: 'merged', and now typechecks.
• AC7 — HistoryPage, StatusTag, and styles.css use merged / closed throughout, with the stale .outcome-shipped / .outcome-cancelled selectors removed. The adapted pull-request webhook tests still assert run cancellation, ticket synchronization, and branch cleanup while ship() / close_external() no longer write resolution.

Verification profile results

• uv run ruff check backend — pass.
• uv run pytest backend/ — 982 passed (local postgres + redis via docker).
• uv pip install -e backend/tests/druks-field_notes — pass.
• proof-extension tests — 5 passed.
• npm --prefix frontend run lint — pass (run in a node:22 container; no node toolchain in this sandbox).
• npm --prefix frontend run build — pass.
• npm --prefix frontend test — 69 passed.
• GitHub checks on d9bfb17: On Pull Request Frontend pass, On Pull Request Backend pass.

Follow-up recommendations

Carried over from round 1 — none maps to an acceptance criterion, so none blocks. File separately if you agree.

1. Reopen-then-merge is unreachable. pr_close_settles_the_item returns early whenever pr_merged is not None, exactly as the plan specified. A PR closed unmerged, then reopened and merged, stays closed in History permanently. Worth a ticket if reopen happens in practice.
2. Facade imports. subscribers.py imports db_session from druks.database and Base from druks.models; extension.py imports Run from druks.durable.models. Extension code elsewhere goes through druks.db and druks.durable, which AGENTS.md names as the author surface.
3. assert for wire invariants. DashboardItem.from_work_item uses bare assert for pr_merged / pr_resolved_at; those vanish under python -O. A raised ValueError, or letting Pydantic reject the None, fails loudly either way.
4. Unbounded state map. Run.subject_states() returns every subject of a type, and StoredSubject.list_open now applies its limit in Python over that full map rather than in SQL. Fine at current scale, worth revisiting as subject counts grow.

Push back on any of these if you read them differently — they are judgement calls, not contract violations.

[druks-operator]

Code review: Minimal, well-targeted fix — both changes correctly address noUncheckedIndexedAccess/DOM.Iterable typecheck failures in the test file, with no scope creep or test-shape concerns.

[czpython]

Reviewed the full branch (25 files, end to end) against main. The core idea is right — GitHub owns the outcome, run state owns liveness — but it ships three behavior regressions and a handful of house-rule violations.

Blocking

1. A redispatched item can never come back to the board. pr_resolved_at is written once and never cleared: the dispatch reset was deleted (workflows.py), any_workflow_start_returns_item_to_board was deleted, and pr.opened doesn't touch it. So: PR closed → operator re-opens the ticket → build re-dispatches → new run running → the item is filtered out at extension.py:143 forever, while History shows it as closed. The deleted test_dispatch_pulls_cancelled_item_back_onto_the_board existed for exactly this strand, and the replacement test now pins the opposite. Same shape on GitHub's side: if item.pr_merged is not None: return (subscribers.py:63) means a close→reopen→merge stores closed permanently.

The resolution belongs to a PR, not to the item forever. Either Build.dispatch clears both columns (a new attempt starting isn't druks stamping an opinion — the old PR's outcome no longer describes the current work), or store the resolved pr_number alongside and only suppress when it matches the item's current one.

2. The shipped/cancelled milestone events are gone, silently. set_status was the only producer of Ship.record_event(type="shipped"|"cancelled"). The feed still renders extension milestones by their own word — events/feed.py:14, extensions/base.py:455, frontend/src/lib/feed.ts. After this PR a subject's timeline ends at the run's last lifecycle event; the merge never appears in it. Nothing in the plan or the ACs mentions dropping the milestone. Keep the record_event in pr_close_settles_the_item (it's one line, and it's the outward-facing half of "the PR resolved"), or say explicitly that the timeline loses it.

3. cancelled / orphaned rows now reach the wire and render nowhere. subject_states drops the OPEN_STATES predicate, so list_subjects returns any item whose newest run is cancelled or orphaned and that has no PR to resolve it. WorkItemsPage buckets only parked|failed|finished and scheduled|running, so those rows are fetched, dropped from both lanes, excluded from total, and flip the empty state from "nothing active" to "no matches". Previously OPEN_STATES excluded them at the source. Filter them in list_subjects or give them a lane.

Should fix before merge
4. Three asserts in DashboardItem.from_work_item (schemas.py:159-162). Banned outright, and stripped under python -O. The root cause is the tri-state pr_merged: bool | None plus a derivation helper. Store the resolution itself — pr_resolution: Mapped[str | None] holding "merged"/"closed". The stored fact then is the wire value (which is this PR's own thesis), _pr_resolution() disappears, DashboardItem reads resolution=item.pr_resolution with no asserts, WorkItemSummary reads it directly, and the migration maps shipped→merged / cancelled→closed just as directly. Shorter diff, one noun, no tri-state bool.

5. Platform plumbing moved into extension code. Ship.list_subjects now calls Run.subject_states directly — checklist #7 names exactly this, and Run is meant to leave author reach. It also leaves StoredSubject.list_open with zero production callers (models.py:106); review uses Subject.list_open, ship abandoned the door instead of adapting it. Keep WorkItem.list_open() and let the platform door take the extra predicate.

6. The read is now unbounded and in Python. subject_states materializes every subject-of-type's newest run into a dict on every board load and SSE snapshot; StoredSubject.list_open then builds an IN (...) from it, and Subject.list_open slices [:limit] after the fact. That grows with total work items forever, where the old query filtered and limited in SQL. Push the filter and limit back down.

7. pr_resolved_at = Base.utc_now() is druks's clock, not GitHub's. The title says "not druks's opinion of it", but History is ordered by webhook receipt time — a redelivery or a backfill sorts wrong. pull_request["closed_at"] / merged_at is right there in on_pull_request; normalize it into the payload next to merged.

Nits
item.pr_resolved_at is None (extension.py:143) → not item.pr_resolved_at; no falsy datetime to distinguish. (pr_merged is not None is a legitimate carve-out only while it stays tri-state — #4 removes it.)
subscribers.py:5-6 imports druks.database / druks.models; extension code goes through the druks.db facade, which ship/models.py already does. Same for from druks.durable.models import Run → from druks.durable import Run.
WorkItemsPage: finished landing in needs-you deserves the why (review-only work with no PR yet); and [...unresolved] copies an array filter already made fresh.
test_failed_build_later_merged_moves_from_board_to_history opens a TestClient inside an async test that has already awaited DBOS work — the poisoning failure mode. Worth confirming it's green in CI rather than assuming.

[druks-reviewer]

@czpython — thanks for the thorough read. Your three blocking issues are real and the implementation will be revised. Here's what changes and why each is a contract change rather than just a fix:

Blocking 1 — re-dispatch clears resolution (design reversal)

The plan's "Ruled out" section explicitly said "Clear PR resolution during dispatch — rejected because the ticket makes pr.closed the single writer." Your review overrides that ruling: a new dispatch attempt means the old PR's outcome no longer describes the current work. Build.dispatch will clear pr_merged and pr_resolved_at before queuing. The deleted test_dispatch_pulls_cancelled_item_back_onto_the_board will be restored. The dedup guard in pr_close_settles_the_item (if pr_merged is not None: return) remains correct for redelivery — dispatch clears the field so a later close writes fresh.

Blocking 2 — restore milestone record_event

set_status was the only caller of Ship.record_event(type="shipped"|"cancelled"). The plan preserved ship()/close_external() operational side effects but omitted the timeline milestone. pr_close_settles_the_item will add the record_event call (one line) so the merge/close event appears in the subject timeline.

Blocking 3 — filter cancelled/orphaned at the source

Dropping OPEN_STATES from the SQL predicate let cancelled/orphaned states reach the wire. Items in those states with no resolution have no bucket in WorkItemsPage and silently corrupt the empty-state signal. These will be excluded in Ship.list_subjects (backend) before they reach the wire — not just frontend-filtered — so the wire contract stays clean.

Should fix before merge:

• DashboardItem.from_work_item bare asserts → replace with raise ValueError (or adopt pr_resolution: Mapped[str | None] to remove the tri-state bool entirely, which also simplifies the migration backfill to shipped→merged/cancelled→closed directly).
• Platform facade imports: subscribers.py will use druks.db instead of druks.database/druks.models; extension.py will use from druks.durable import Run.
• Run.subject_states() unbounded materialization: filter and limit pushed back to SQL, not applied in Python after the fact.
• pr_resolved_at timestamp: use payload["closed_at"] (or merged_at) from the GitHub webhook payload, not Base.utc_now(), so History ordering survives redelivery and backfill.

Nits (also applied): extension.py:143 guard phrasing, redundant [...unresolved] copy in WorkItemsPage, finished-in-needs-you comment, and the async TestClient poisoning concern in the regression test will be confirmed against CI.

The implementer will pick this up as revision 3.

Superseded by implementation revision 3.

[druks-reviewer]

Contract verification — ENG-760, implementation revision 3

All fourteen acceptance criteria hold and the full verification profile is green locally, matching the two green checks runs on 49b39ed. Ticket ENG-760 and its single operator comment were read first; the comment adds no constraint the plan omits.

Prior blockers — all resolved

The owner's CHANGES_REQUESTED review listed three blocking items and four must-fix quality items. Each is addressed in 49b39ed:

1. Re-dispatch clears resolution — workflows.py:130-137 extends the existing genuinely-new-run branch to pass pr_merged=None, pr_resolved_at=None through item.update(), with the two new _KEEP-sentinel parameters added in models.py:365-382. test_dispatch_pulls_cancelled_item_back_onto_the_board seeds a merged item, dispatches a new run, and asserts both fields are None and the item is back in Ship.list_subjects(). The dedup path keeps a stored resolution — asserted in test_duplicate_dispatch_keeps_the_live_attempt_routing. (AC8)
2. Milestone events restored — subscribers.py:69-73 calls Ship.record_event(type="shipped"|"cancelled", subject=item) after the fields are flushed and before ship()/close_external(). frontend/src/lib/feed.ts:44 renders an extension milestone type as its own word, so "shipped"/"cancelled" need no renderer change. Both webhook tests now assert the Event row. (AC9)
3. Cancelled/orphaned rows excluded — extension.py:138-147 filters states[str(item.id)] not in (RunState.CANCELLED, RunState.ORPHANED) in the backend composition. test_a_cancelled_or_orphaned_build_with_no_resolution_leaves_the_board covers both states, deriving ORPHANED by deleting the workflow_status row and backdating Run.created_at past the grace window. (AC10)
4. assert → raise ValueError — schemas.py:159-163; no bare assert remains in from_work_item. (AC11)
5. Facade imports — subscribers.py now imports db_session from druks.db and no longer needs Base; extension.py imports Run, RunState from druks.durable. StoredSubject.list_open is removed from druks/models.py as dead code; Subject.list_open in datastructures.py stays and the Review extension's tests pass. (AC12)
6. Limit pushed into SQL — Run.subject_states(..., limit=500) applies .limit(limit) on the ranked, created_at DESC-ordered query; both call sites thread their limit. (AC13)
7. GitHub's own timestamp — on_pull_request_closed forwards merged_at/closed_at as resolved_at; the subscriber parses it with the ISO idiom. Both webhook tests assert the exact parsed constant rather than "not None". (AC14)

AC1–AC7 are unchanged from revision 2 and re-verified against this head: migration a4c9e2f7b1d6 with its inverse downgrade, HandoffStatus/set_status absent, pr.closed the only writer of a non-None resolution, resolution serialized as merged/closed/null, the failed-then-merged regression test, and the frontend bucketing plus History glyphs.

Verification profile

• uv run ruff check backend — pass
• uv run pytest backend/ — 984 passed
• uv pip install -e backend/tests/druks-field_notes — pass
• proof-extension tests — 5 passed
• npm --prefix frontend run lint — pass (run in a node:22 container; no node toolchain in the sandbox)
• npm --prefix frontend run build — pass
• npm --prefix frontend test — 69 passed
• GitHub checks on 49b39ed — both green

Follow-up recommendations

None of these blocks; file separately if you agree.

1. Subject.list_open's limit now bounds subjects, not open subjects. datastructures.py:59 applies the SQL LIMIT before the Python OPEN_STATES filter, so list_open(limit=50) returns the open subset of the newest 50 subjects — fewer than 50 open subjects when recent ones are closed. The plan directed exactly this, and the Review extension is the only caller, so it ships. If Review's active list ever needs a guaranteed page size, the filter has to move into the ranked SQL rather than the limit. A test in the Review extension's active-list suite asserting page size against a mix of open and closed newest-runs would pin whichever behaviour you want.
2. Stale docstring reference. datastructures.py:54 still points at StoredSubject.list_open, which this diff deletes. One-line docstring fix.
3. Owner nit not applied. WorkItemsPage.tsx:130 keeps the redundant [...unresolved] copy; unresolved is freshly built by filter on the line above.
4. resolved_at assumes the field is present. on_pull_request_closed reads pull_request["merged_at"]/["closed_at"] directly. GitHub always populates one on action == "closed", so this is correct today; a malformed or replayed payload raises KeyError inside the webhook rather than being rejected at the boundary. If you want the boundary to be defensive, a .get() with an explicit 400 plus a test in test_webhooks_pull_request.py for a payload missing both fields would cover it.
5. Reopen-then-merge is still unreachable (carried from rounds 1 and 2). pr_close_settles_the_item returns early whenever pr_merged is not None, so a PR closed unmerged, then reopened and merged, stays closed in History. Note that item 1 above partly softens this: a re-dispatch now clears the resolution, so the item is recoverable through a new attempt.

Push back on any of this — the follow-ups in particular are judgement calls, not contract violations.

[druks-operator]

Code review: Correctly re-implements the PR-resolution-as-stored-fact design (pr.closed now writes GitHub's own merged_at/closed_at, dispatch clears a stale resolution, cancelled/orphaned runs leave the board) with solid test coverage, but threading a limit into Run.subject_states before it filters by run state can silently drop a genuinely open work item off the active board once the system has accumulated >500 more-recently-dispatched subjects — filed as a follow-up.