The active board follows each item's driving run#80
Conversation
There was a problem hiding this comment.
Two CI regressions introduced by this revision — both actionable in-repo.
Blocking
-
Backend test —
Run.cancelmock signature is now stale.
test_subject_lifecycle.py::test_workflow_cancel_takes_its_own_kind_and_passes_over_idle_subjectsmonkeypatchesRun.cancelwithasync def cancel(self, *, failure=None).BuildWorkflow.cancelnow forwardsfailure_code=to whateverRun.cancelit finds, so the mock raisesTypeError: got an unexpected keyword argument 'failure_code'. Constraint: the mock must accept the same kwargsRun.cancelnow accepts — either addfailure_code=Noneexplicitly (mirrors the real signature and documents the contract) or absorb it with**_(looser, but resilient to future kwargs). Extend the same test to assert the forwardedfailure_codewhen relevant so future signature drift is caught. -
Frontend build — spread over
NodeListOf<Element>is rejected by strict TS config.
frontend/src/extensions/build/WorkItemsPage.test.tsx:64does[...document.querySelectorAll('.wi-group-count')].tsc -bfails withTS2488: Type 'NodeListOf<Element>' must have a '[Symbol.iterator]()' method that returns an iterator, breakingnpm --prefix frontend run build. Constraint: iterate without relying on the iterator protocol onNodeListOf.Array.from(document.querySelectorAll('.wi-group-count'))is the least invasive fix and keeps behaviour identical; if you prefer to keep the spread everywhere, addingdom.iterabletotsconfig'slibalso works but widens the type surface for the whole app — the localArray.fromis the more conservative choice unless there's a project-wide preference. The new test in the same file covers the count assertion; no additional test needed once the build passes.
Push back on either if you see a cleaner approach — the constraint is only "backend tests pass" and "tsc -b && vite build succeeds," not the exact one-liner.
Superseded by 6f1afd0.
There was a problem hiding this comment.
Round 2 verification — approving.
Both prior-round blockers are addressed exactly as flagged:
backend/tests/test_subject_lifecycle.py:84— theRun.cancelmonkeypatch now acceptsfailure_code=None, matching the extended signature.frontend/src/extensions/build/WorkItemsPage.test.tsx:64— swapped spread overNodeListOfforArray.from(...), unblockingtsc -b.
Frontend CI is green. Backend CI was still in progress at review time; the diff since round 1 is limited to the two lines above (both test-only, both correctly resolving the prior TypeError / TS2488 blockers), so I'm not blocking on the pending check.
Follow-up recommendations (post-merge, non-blocking)
- Consider a targeted unit test for
list_subject_ids_on_board's "active scope + terminated build" driving-run ordering so futureROW_NUMBERchanges don't silently regress. - Product may want scope-terminal signals (
Scope.kind+run.cancelled/run.failed) surfaced somewhere; today they leave the item invisible to both board and History by design.
Push back if any of these read differently on your end.
|
Code review: Board membership, ACTIVE count, and handoff writes correctly unified onto run-derived state; two medium follow-up items filed as a sub-issue on ENG-740. |
Membership read work_items.status, a column only the shipped/closed paths wrote, so a build that crashed or was cancelled left the item on the board forever — counted by the header but matching neither lane. The board now derives from the same run state the lanes bucket by, and the header counts the lanes it renders. The newest run speaks for its subject: dedup gives a subject one active run per kind and the next starts only once the last is terminal, so preferring an active run over a newer one decided nothing once scope folded into build. created_at holds whole seconds, so the uuid7 id settles same-second ties. Subject.list_open() composes Run.open_subject_ids(), so an extension asks its own subject rather than the platform read side. Run.cancel announces run.cancelled — the body re-raises without emitting, so nothing had observed an operator cancel before.
Linear ticket: ENG-740
The bug
ACTIVE (2)above a single rendered row, and cancelled items that never left the board.Membership came from
work_items.status, a column only the shipped/closed paths wrote —list_subjects()returnedWorkItem.list_recent() if not item.status. The lanes came from somewhere else: the run-derivedSubjectStatus.state. Two sources of truth that had to agree, maintained separately.A build that crashed or was cancelled ended its run but wrote no status, so the item stayed
NULL— counted as active by the header while its derived state wascancelledand matched neither lane. Because membership was defined by the absence of a value, the failure mode was always lingering.The change
The board derives from the same run state the lanes bucket by, and the header counts the lanes it renders.
statuskeeps only its History meaning. A run that ends with nothing merged settles the item cancelled, so an abandoned attempt still lands somewhere instead of vanishing;ship()andclose_external()keep overwriting, because GitHub announced those outcomes.Run.cancelnow announcesrun.cancelled. The workflow body re-raisesDBOSWorkflowCancelledErrorwithout emitting, so nothing had ever observed an operator cancel.Simplifications found on the way
The newest run speaks for its subject. The old rule was "prefer an active run, else the newest". Queue dedup gives a subject at most one active run per kind, and the next starts only once the last is terminal — so within a kind, active is newest. The preference only decided anything when two kinds overlapped on one subject, which stopped being possible when scope folded into build (#81).
Removing it exposed a latent weakness it had been hiding:
created_atholds whole seconds, so two runs starting in the same second tie. Theuuid7id orders by real creation time and settles them. It also fixed a live inconsistency —list_for_subjectordered byupdated_atwhile the board query usedcreated_at, andupdated_atmoves, so a touched older run could out-rank a newer one.A subject lists its own open rows.
Subject.list_open()composesRun.open_subject_ids()as a subquery — one statement, one round trip — so an extension asks its own subject instead of the platform read side for a set of ids to filter against.list_subjectsis one line. The run query lives onRunbesidelist_for_subjectandget_latest_for_subject;reads.pyis for queries that compose rows into response shapes, and this composes none.Run.get_latest_for_subject()replacesruns[0]at every call site.Verification
ruff check/ruff formatclean, imports clean, frontendtsc/eslint/ vitest green. Backend CI green.test_build_board_membership.pycovers: live and failed runs hold the board; finished and cancelled ones leave it whilestatusis still NULL (the original strand); a re-dispatched item returns; an ended build settles cancelled; a shipped item keeps its outcome; and the bulk board query and per-subject status name the same driving run.Not in this PR
work_items.statusstill encodes two things — whether it ended (now derivable) and how. Collapsing it tois_shippedis ENG-760; it's a migration plus a wire change plus the History page's filter.