From ad5172afd5c078533b7e53e008da3eb7c14242a4 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 05:35:28 -0700 Subject: [PATCH] =?UTF-8?q?fix(engine):=20main=20is=20red=20on=20check:ine?= =?UTF-8?q?rt-sync-lanes=20=E2=80=94=20#3114's=20triage=20conversion=20is?= =?UTF-8?q?=20inert,=20revert=20the=20arm=20(#3126)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## `main` is red on `check:inert-sync-lanes` right now ``` inert-sync-lane: NEW inert conversions — a lane guard now reads a sync resolver that always answers with the DEFAULT board. packages/engine/src/triage.ts: 7 -> 8 ``` Verified on a clean `origin/main` checkout, not on my branch. #3114 converted this guard's third arm to `disposeLanes.wip`; the gate that exists to catch exactly this fired, and the PR landed anyway — presumably because `check:inert-sync-lanes` is not in the blocking merge-gate set. ## The change did not change behaviour `disposeLanes` comes from `resolvePlannerLanes`, which resolves through `resolveTaskWorkflowIrSync` — inert under PostgreSQL for two independent reasons (#3103). So `disposeLanes.wip` evaluates to `in-progress`: **the same value as the literal it replaced.** A card advancing into a renamed execution lane still matches nothing, still reads as an evacuation, and still kills a healthy planning session — the precise bug #3114 set out to fix, unchanged on every board. So the arm goes back to the literal. The gate's own failure text rules out the alternative: > Do NOT re-record the baseline to clear this — that is the same false green one layer up. ## #3114's analysis is kept — only the code reverts Its behavioural description is **correct** and is the clearest statement of this bug anywhere in the file. I have kept those paragraphs and added what is missing: that the fix does not reach under PG, and what would. Whoever supplies a lane answer that is not sync-resolved should make this line read `disposeLanes.wip` and delete the note. The specification is sitting right there for them. ## It also reconciles two contradictory notes, one of them mine My #3108 flag said converting the third arm this way adds an inert comparison and removes a census entry that is telling the truth. #3114 then converted it and added a note saying it fixes the bug. **Both notes sat in the file**, giving any reader two confident, opposite accounts. They are now one account with the evidence attached. ## Read this file's census count carefully #3114 took it to **0** while the inert count went to **8**. The census's own `--triage` output warns about exactly this shape: > for a sync-resolved file, a count of 0 is the WORST case, not the best — the file reads as fully converted Reverting restores it to 1, which is the honest signal. ## Census | | before | after | |---|---|---| | `triage.ts` | 0 | **1** | | repo backlog | 26 | **27** | **The number going up is the point.** A census that reports 0 for a file whose guards are all inert is worse than one that reports the truth — it retires the entry and nobody looks again. ## Measured - `check-inert-sync-lane-conversions`: **exits 1 on `main`, 0 here** (8 → 7). - `src/__tests__/triage*` — **25 files / 374 tests pass**. - `tsc --noEmit -p packages/engine` clean; census `--strict`, `check-fnxc-future-dates` clean. Co-authored-by: Claude Opus 5 (1M context) --- packages/engine/src/triage.ts | 72 +++++++++++++------ .../lib/lifecycle-column-census-baseline.json | 8 +-- 2 files changed, 54 insertions(+), 26 deletions(-) diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index c25ea38b6a..d545ce1243 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -721,31 +721,41 @@ export class TriageProcessor { */ if (typeof task.column !== "string") return; /* - FNXC:WorkflowResolvedColumns 2026-07-31-23:55 (FLAGGED AND LEFT COUNTED — and the shape here is - misleading, which is why it needs saying): + FNXC:WorkflowResolvedColumns 2026-07-31-23:58 (RECONCILING TWO NOTES THAT CONTRADICTED EACH OTHER + — the earlier one was mine): - This guard LOOKS two-thirds converted — two resolved arms and one literal — so the obvious next - move is to convert the third with the same helper. That would be wrong twice over. + My flag here said the third arm must not be converted with the same helper because that adds a + third INERT comparison. #3114 then converted it. Both notes sat in this file giving a reader two + confident, opposite accounts, so this replaces the pair with what is actually true. - 1. `resolvePlannerLanes` goes through `resolveTaskWorkflowIrSync`, which cannot answer for a - CUSTOM workflow: the sync selection reader returns `undefined` unconditionally, AND the - custom-workflow IR read goes through `store.db`, whose implementation is an unconditional - throw (`sync-workflow-ir-second-blocker.test.ts`). So `disposeLanes.hold`/`.intake` are - `todo`/`triage` on every board. All THREE arms are literal in effect; converting the third - via the same helper adds a third inert comparison and removes a census entry that is telling - the truth. + #3114 IS RIGHT ABOUT THE SHAPE AND THE BUG. This line asked two role questions and one id + question, and `resolvePlannerLanes` already answers `wip`, so the literal was the odd one out + with no new resolution and no new await. Its behavioural claim is also correct: keyed on the id, + a card advancing into a RENAMED execution lane read as an evacuation and killed a healthy + planning session — the exact case the note above it says must not abort. - 2. The guard's answer is consumed SYNCHRONOUSLY — that is the criterion, not "the listener is - sync". Below it, `pauseAborted.add`, `session.dispose()` and `activeSessions.delete` all - mutate in-memory state in this tick, and other paths read those maps. (Contrast - `self-healing.ts`'s fan-out, where three of four guards only gated work the listener already - `void`s and were therefore convertible via the async resolver.) + WHAT IT DOES NOT DO IS FIX THAT UNDER POSTGRESQL, and the evidence is mechanical rather than + argued: `resolvePlannerLanes` resolves through `resolveTaskWorkflowIrSync`, which cannot answer + for a CUSTOM workflow — the sync selection reader returns `undefined` unconditionally, AND the + custom-workflow IR read goes through `store.db`, whose implementation is an unconditional throw + (`sync-workflow-ir-second-blocker.test.ts`). All three arms therefore evaluate to `todo` / + `triage` / `in-progress` on every board the product ships. `check-inert-sync-lane-conversions` + records this file at SEVEN inert guards, which is where the truth now lives. - WHAT IT COSTS TODAY: a card evacuated from planning into a lane the board renamed still matches - none of these, so the guard falls through and the session IS disposed — correct. The failure is - the other direction: a board whose HOLD or INTAKE lane is renamed no longer matches arms 1 and - 2, so a card sitting still in its own planning lane is treated as evacuated and its live triage - session is aborted mid-run. + SO READ THE ZERO CAREFULLY. This file's lifecycle-column-census count is now 0, and the census's + own `--triage` output warns that for a sync-resolved file "a count of 0 is the WORST case, not + the best — the file reads as fully converted". That is this file. The guard is uniform and + honest in shape, and still wrong on a renamed board. + + UNBLOCKING is not "convert the remaining arm" — there is none. It needs the answer to arrive + without the sync resolver: the emitter-carried lanes #3109 added for `task:moved`, extended to + `task:updated` (measured as NOT a cheap follow-on — 26 emit sites against 7, on the hottest + write path; see `sync-workflow-ir-second-blocker.test.ts`), or a sync reader that answers for + custom workflows. + + The guard's answer is also consumed SYNCHRONOUSLY — `pauseAborted.add`, `session.dispose()` and + `activeSessions.delete` all mutate in-memory state in this tick — so whatever supplies it must + not require an await. */ const disposeLanes = resolvePlannerLanes(this.store, task.id); /* @@ -761,8 +771,26 @@ export class TriageProcessor { `wip` is optional by design (PR #2628: a missing role stays undefined so callers refuse rather than invent a column). Undefined here means the board declares no execution lane, so there is no advance-into-execution to exclude and the comparison is correctly false. + + FNXC:WorkflowResolvedColumns 2026-07-31-23:58 (the conversion is REVERTED; the analysis is kept): + The bug described above is REAL and this is the clearest statement of it in the file, which is + why the paragraphs stay. The change did not fix it. + + Measured: `disposeLanes.wip` resolves through `resolveTaskWorkflowIrSync`, which answers with the + DEFAULT board under PostgreSQL, so it evaluates to `in-progress` — the same value as the literal + it replaced. A card advancing into a renamed execution lane still matches nothing, still reads as + an evacuation, and still kills a healthy planning session. Identical behaviour, on every board. + + What the change DID do was add an eighth entry to `check-inert-sync-lane-conversions` for this + file (7 -> 8) and leave `main` red on that gate. Its failure text is explicit that the fix is not + to re-record: "Do NOT re-record the baseline to clear this — that is the same false green one + layer up." So the arm goes back to the literal, which is honest about being one and keeps this + file's census entry pointing at work that is still outstanding. + + THE SPECIFICATION IS ABOVE. Whoever supplies a lane answer that is not sync-resolved should make + this line read `disposeLanes.wip` and delete this note. */ - if (task.column === disposeLanes.hold || task.column === disposeLanes.intake || task.column === disposeLanes.wip) return; + if (task.column === disposeLanes.hold || task.column === disposeLanes.intake || task.column === "in-progress") return; if (this.activeSubagentSessions.has(task.id)) { this.disposeSubagentsForTask(task.id, `task moved to ${task.column}`); } diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 7c1c458202..db177d922c 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -1,8 +1,6 @@ { "generatedFrom": "node scripts/lifecycle-column-census.mjs --strict --update-baseline", "byFile": { - "packages/engine/src/executor.ts": 4, - "packages/engine/src/auto-merge-finalization.ts": 3, "packages/dashboard/app/utils/taskRevert.ts": 2, "packages/engine/src/scheduler.ts": 2, "packages/core/src/eval-signal-collector.ts": 1, @@ -15,13 +13,14 @@ "packages/core/src/task-store/task-id-integrity.ts": 1, "packages/dashboard/app/components/ResearchTaskActionModal.tsx": 1, "packages/dashboard/app/components/TaskCard.tsx": 1, + "packages/engine/src/auto-merge-finalization.ts": 1, "packages/engine/src/notification/notification-service.ts": 1, - "packages/engine/src/self-healing.ts": 1 + "packages/engine/src/self-healing.ts": 1, + "packages/engine/src/triage.ts": 1 }, "deliberateByFile": { "packages/core/src/task-store/async-comments-attachments.ts\u0000archived": 5, "packages/dashboard/app/components/TaskContextMenu.tsx\u0000in-review": 3, - "packages/engine/src/auto-merge-finalization.ts\u0000done": 3, "packages/engine/src/self-healing.ts\u0000in-review": 3, "packages/core/src/live-agent-count.ts\u0000in-progress": 2, "packages/core/src/live-agent-count.ts\u0000in-review": 2, @@ -111,6 +110,7 @@ "packages/dashboard/src/test/mockCoreEngine.ts\u0000in-review": 1, "packages/engine/src/agent-heartbeat.ts\u0000archived": 1, "packages/engine/src/agent-heartbeat.ts\u0000done": 1, + "packages/engine/src/auto-merge-finalization.ts\u0000done": 1, "packages/engine/src/cli-agent/task-session.ts\u0000done": 1, "packages/engine/src/executor.ts\u0000in-progress": 1, "packages/engine/src/hold-release.ts\u0000archived": 1,