fix(core): the in-review stall signal never got the board's review lanes — 0 of 4 call sites - #2956
Conversation
…nes — 0 of 4 call sites Main is red on `unwired-lane-parameter-guard` after #2951, and the red is pointing at a real defect rather than a stale list. #2951 added `reviewColumns?: ReadonlySet<string>` to three signal modules and wired two of them completely. `getInReviewStallReason` was wired at NONE of its four call sites in reads.ts — measured by brace-matching the option literal at each site, not by proximity: getInReviewStallReason L227=NO L390=NO L599=NO L729=NO getInReviewStalledSignal all 4 wired getStalePausedReviewSignal both wired THE USER-VISIBLE CONSEQUENCE. reads.ts computes two adjacent signals for the same card. On a board declaring a separate merge lane beside its human-review lane, `inReviewStall` read the FIRST review column only while `inReviewStalled` — three lines below — read the SET. The same card is "in review" for one signal and not the other. Two signals disagreeing is worse than both being legacy, and it is invisible on every builtin board because there the review set has exactly one element. At three of the four sites the resolve sat BELOW the call, which is why the parameter could not be passed; those are hoisted. ALSO NAMES THE CONTEXT TYPES so the guard stops false-positiving on wired code: it keys an interface member to its OWNER symbol and only counts a mention from a file naming that owner, so passing the property inline reads as unwired. `satisfies InReviewStalledContext` / `satisfies StalePausedReviewContext` on the option literals is real type-checking rather than a decorative import — lint rejected the decorative version, correctly. NEW TEST, because the existing guard cannot see this. Measured: deleting the `reviewColumns:` line from a fixed call site leaves `unwired-lane-parameter-guard` at 9/9 green, since the file still names the type. The new ratchet brace-matches each call site's option literal and fails if any omits the lanes: mutation: remove lanes from one site -> "1 of 4 getInReviewStallReason call sites omit reviewColumns" — 1 failed. It also asserts it FOUND call sites first; a parse that matched nothing would make it vacuous, which is the failure mode this guard family keeps producing. Core 4861 passed / 0 failed · guard 9/9 with KNOWN_UNWIRED unchanged · gate exit 0 · lint clean · core tsc 0 errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI Lint failed on check-fnxc-future-dates: packages/core/src/task-store/reads.ts: 1 future-dated FNXC stamp(s), baseline allows 0 packages/core/src/__tests__/in-review-signals-lane-wiring.test.ts: 1 … I stamped 2026-08-01-04:10 and -04:40 while UTC was 2026-07-31 04:00 — minutes into tomorrow. Re-dated to 2026-07-31. Worth noting why local verification missed it: `pnpm test:gate` passed here before the stamps crossed midnight-relative-to-my-clock, and `pnpm lint` alone does not run this check — CI's Lint job runs the census/FNXC/SQL ratchets too. So a green local gate is not evidence for this particular check if the run predates the edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second attempt at the same stamp. 2026-07-31-03:55 is in the past by UTC but the checker deliberately uses the LOCAL calendar — and this machine is UTC-7, so locally it is still 2026-07-30 21:00 and "today+1" was still future. The script says so at check-fnxc-future-dates.mjs:56-58, in a note added by #2941's own review: "Today in the repo's LOCAL calendar; a stamp for today is fine, tomorrow is not … `toISOString()` is UTC, so for anyone west of it …". I re-dated from UTC on the first pass and reproduced exactly the failure that note exists to prevent. Re-stamped 2026-07-30-20:50 / -20:55. Check now exits 0: "467 known future-dated stamp(s), none added." Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No rebase needed — the CONFLICTING notice is stale. GitHub reports this Test merge of this branch into current The notice was almost certainly generated before #2953 and #2954 merged, both of which touched the same baseline file. Why the raw diff looks alarming and is not. One real interaction, now a no-op. This branch removes the I did not rebase, deliberately. This branch is checked out in another worktree ( |
|
This was my defect in #2951, and the mechanism is worth recording because it will bite the next person the same way. How all four sites got skipped. I wired the call sites with a search-and-replace that swapped an existing key: That edit can only touch a call site that already passed the singular key. Two things I should have caught and did not:
And the irony worth stating plainly: #2951's own PR body warns against 'an optional parameter with no caller' as the anti-pattern this program already removed five of — and then shipped exactly that, in the same change. The guard existing is what turned a silent half-conversion into a red main instead of a board where two adjacent signals quietly disagree about the same card. No action needed from me here; #2956 is correct and main's gate is green (161/487/13/71 verified locally on |
…est that proved it) (#2961) **My defect, shipped in #2951 — and the same family as the one #2956 just fixed.** Found by auditing my own seams after that, not by a failing check. ## What is on `main` right now `surfaceInReviewStalls` reads the project's review columns (converted in #2951), then calls `getInReviewStallReason` **without** `reviewColumns`. The classifier falls back to the literal `in-review`, returns no signal for a renamed-lane card, and the sweep surfaces nothing. That is the textbook **missed pair** this program has a ratchet for: a widened read handing every renamed-board card to a literal classifier. The resolve work happens and is then discarded. On a renamed board an operator sees no stall warnings at all. #2951's conflict resolution dropped two things together: - the per-card `stallLanes` map and the `reviewColumns` argument - **the test that proved the wiring** ## Why nothing caught it **A deleted test cannot fail.** I verified that rebase by comparing the 68 conflict *hunks* — stripping FNXC stamps, confirming 0 of 68 had real content differences — and then ran the gate. The gate passed precisely because the proving test had gone with the code it proved. I verified the conflicts. I did not verify the outcome. Those are different things, and the difference is invisible when the evidence disappears alongside the feature. The `unwired-lane-parameter` guard cannot catch this either, by design: it is deliberately conservative — a mention of the parameter *anywhere* satisfies it — so **partial** wiring is outside its reach. `reviewColumns` is mentioned plenty in `reads.ts`, so the guard is green while this call site goes unwired. ## How I found it The check #2956 used on the sibling defect, applied to every lane seam I have touched: enumerate each function's **call sites** and confirm each one carries the parameter. That enumeration also flags several other call sites without `reviewColumns`/lane arguments (`merger.ts`, `moves.ts`, `auto-merge-finalization.ts`, `merger-ai.ts`, `project-engine.ts`) — I have **not** touched those here; they need per-site judgement about whether the lane answer is even available, and that is a separate change rather than a sweep. ## Revert result | | reverted → | | --- | --- | | `reviewColumns` at the call (i.e. exactly what #2951 shipped) | fails the restored test | ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; blindness suite 71; `self-healing.test.ts` 412; `tsc` engine clean; lint, census `--strict`, FNXC gate, changesets all clean.
…lane (#2963) **Not a degraded message — no task on such a board could be merged at all.** `getTaskMergeBlocker`'s column-identity check *returns a blocker* when the task's column is not a review lane. Both merge entry points called it without `reviewColumns`, so the check ran against the literal `in-review`: ``` Cannot merge FN-1: task is in 'signoff', must be in 'in-review' ``` `aiMergeTask` (`merger.ts`) and `runAiMerge` (`merger-ai.ts`) turn that into a thrown error. Every merge on a renamed board fails, with a message naming a column the board does not have. ## This exact defect was already found once The helper's own FNXC comment records it, in `moves.ts`: > *"so on a renamed board that move threw `Cannot move FN-1 to done: task is in 'signoff', must be in 'in-review'` even though the transition had just been validated as legal. A half-conversion, where the outer question is resolved and the inner one is not."* That fix added the `reviewColumns` option and wired `moves.ts`. **These two callers were missed** — same shape, one layer out. A fix that adds an optional parameter is only as good as the call-site sweep that follows it. ## How it was found By enumerating the call sites of every lane-taking helper, rather than trusting the `unwired-lane-parameter` guard. That guard is deliberately conservative — a mention of the parameter *anywhere* satisfies it — so **partial** wiring is invisible to it, and `reviewColumns` is mentioned plentifully elsewhere. This is the method #2956 used on a sibling defect, applied to every seam I have touched. ## Two sites deliberately unchanged - **`moves.ts`** passes `skipColumnIdentityCheck: true`. It has already proven lane identity from resolved IR traits, so supplying lanes *as well* would be contradictory rather than additive — the helper's comment is explicit that the two options answer different questions. - **`isTaskReadyForMerge`** has **zero** production callers. Adding a parameter there is precisely the unwired-parameter anti-pattern this program keeps removing. ## Revert result | | reverted → | | --- | --- | | `reviewColumns` at either call | reproduces the shipped string exactly | The middle test pins that string deliberately: it is the operator-visible failure, so if the wiring regresses the test says what the operator would have seen. A third case checks that supplying lanes does **not** switch the identity check off — a card in the wip lane is still blocked, and the message names the resolved lanes rather than a column the board lacks. The cases drive `getTaskMergeBlocker` directly: reaching it through the merge entry points needs a real repo, worktree and merge run, while the defect is entirely in *which columns the blocker is asked about*. The wiring itself is covered by tsc and the guard. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `merger` + `merger-ai` + `self-healing` suites 461; `tsc` engine clean; lint, census `--strict`, FNXC gate, changesets all clean.
THE GAP THAT LET THREE DEFECTS THROUGH IN ONE DAY. unwired-lane-parameter.mjs catches a parameter that reaches NO caller. It is deliberately satisfied by a mention anywhere, so PARTIAL wiring is invisible to it: #2956 getInReviewStallReason wired at 0 of 4 call sites, siblings wired #2963 both merge entry points unwired — merging IMPOSSIBLE on a renamed board #2964 merge-confirmed finalization unwired — already-landed work parked `failed` Every one was a fix that added an optional parameter without the call-site sweep that has to follow it. The guard was green throughout. A CENSUS, NOT A GUARD, and the distinction is the point. Auditing the sites this finds showed FOUR of seven were legitimately unwired: skipColumnIdentityCheck callers have proven lane identity by a stronger means, a sentinel-column caller wants the check satisfied by construction, and a dead export has no caller to wire. Failing on those is ~57% false positives — and the sibling guard's own header says why that is worse than a miss: it teaches people to disable the check. So it ratchets like the lifecycle census: 36 known sites across 20 files, may only shrink. A NEW unwired caller raises the count and fails; wiring one lowers it and re-records. Detection is AST-based (TypeScript), not regex: find exported functions accepting a lane-named argument — directly or as an options-bag member — then find call sites passing none of them. The ad-hoc regex scan I used during the audit produced false negatives on multi-line calls, which is exactly how a caller gets missed. Verified to FAIL on the recurrence: injecting one new unwired caller into self-healing.ts reports `9 unwired now, baseline allows 8` and exits 1. A ratchet that cannot fail is worse than none, so that was measured rather than assumed. Fusion-Task-Id: KB-SELF-HEALING-QUERIES
**This is the gap that let three defects reach `main` in one day.** `unwired-lane-parameter.mjs` catches a parameter that reaches **no** caller. It is deliberately satisfied by a mention *anywhere*, so **partial** wiring is invisible to it: | | | | --- | --- | | #2956 | `getInReviewStallReason` wired at **0 of 4** call sites while both siblings were wired | | #2963 | both merge entry points unwired — merging was **impossible** on a renamed board | | #2964 | merge-confirmed finalization unwired — **already-landed work parked `failed`** | Every one was a fix that added an optional parameter without the call-site sweep that has to follow it. The existing guard was green throughout, correctly by its own contract. ## A census, not a guard — and that distinction is the whole design Auditing the sites this finds showed **four of seven were legitimately unwired**: `skipColumnIdentityCheck` callers have already proven lane identity by a stronger means, a sentinel-column caller wants the identity check satisfied by construction, and a dead export has no caller to wire at all. A check that failed on those is ~57% false positives. The sibling guard's own header says why that is worse than a miss — *"it teaches people to disable the check"* — and I agree, so this does not do it. Instead it ratchets like the lifecycle census: **36 known unwired sites across 20 files**, allowed to shrink and not to grow. A new unwired caller raises the count and fails; wiring one lowers it and re-records. The recurrence — adding a caller that forgets the lane answer — is precisely what gets caught, and the legitimate sites cost one baseline line each instead of a permanently red gate. ## Detection is AST-based, deliberately It finds exported functions accepting a lane-named argument — directly *or* as an options-bag member — then finds call sites passing none of them. Not regex: the ad-hoc scan I used during the audit produced false negatives on multi-line calls, which is exactly how a caller gets missed in the first place. Using a heuristic to police a defect caused by a heuristic seemed like a poor trade. ## Verified to fail on the recurrence A ratchet that cannot fail is worse than none, so this was measured rather than assumed. Injecting one new unwired caller into `self-healing.ts`: ``` [check-lane-wiring] call sites not passing a resolved lane argument INCREASED: packages/engine/src/self-healing.ts: 9 unwired now, baseline allows 8 ``` exit 1, naming the file and the delta. ## Placement Runs as a named `check:lane-wiring` step in `pr-checks.yml` beside the lifecycle, SQL, inert-seam and FNXC ratchets — same convention, same failure ergonomics, ~1s. Note the baseline records today's state, which still includes the #2963/#2964 sites because those fixes have not merged yet. When they land the count drops and the baseline is re-recorded downward — the ratchet working as intended rather than a conflict. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `tsc` engine clean; lint, lifecycle census `--strict`, FNXC gate, and the new check all clean.
Detecting named-context functions exposed a second bug that had been hidden
while they were invisible: findUnwiredCallSites tested isObjectLiteralExpression
directly, so { ... } satisfies SomeContext -- the form #2956 used on four wired
reads.ts call sites -- was not recognised as a bag at all.
Six of the fourteen sites the first commit called 'newly visible' were correct
code. Honest baseline is 18 across 14 files, not 24 across 15.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
findLaneAcceptingFunctions matched a lane parameter only when param.type was a TypeLiteralNode, so every *Context INTERFACE was invisible -- including getInReviewStallReason(task, context: InReviewStallContext), which is #2956, the first case in the census's own header. Measured: re-introducing #2956 left the check reporting 'none added'. With the TypeReference arm it fails (reads.ts: 7 unwired, baseline allows 6). Honest baseline moves 10 unwired sites across 8 files -> 24 across 15. No entry was removed; all 14 are newly VISIBLE, not newly broken, and none are audited -- that is what the baseline records. Resolved by name across the corpus rather than via a type-checker Program: these are source scans, and a false shared member only widens what counts as wired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Detecting named-context functions exposed a second bug that had been hidden
while they were invisible: findUnwiredCallSites tested isObjectLiteralExpression
directly, so { ... } satisfies SomeContext -- the form #2956 used on four wired
reads.ts call sites -- was not recognised as a bag at all.
Six of the fourteen sites the first commit called 'newly visible' were correct
code. Honest baseline is 18 across 14 files, not 24 across 15.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…se (#2956) (#2974) #2966 shipped a gate that **cannot detect the defect named first in its own header.** `findLaneAcceptingFunctions` matched a lane parameter only when `param.type` was a `TypeLiteralNode` — an inline `{ reviewColumns?: … }`. But the real code declares these as interfaces: ```ts export function getInReviewStallReason( task: Pick<Task, …>, context: InReviewStallContext = {}, // TypeReference — invisible ): InReviewStallSignal | undefined ``` so the function never entered `accepting` and none of its call sites were examined. ### Measured, both directions | | before | after | |---|---|---| | lane-accepting functions detected | 20 | **30** | | `getInReviewStallReason` detected | no | **yes** | | re-introduce #2956 (drop `reviewColumns` from one call site) | `none added` — **passes** | **fails**: `reads.ts: 7 unwired now, baseline allows 6` | The gate now catches the thing it was built for. ### The baseline moves 10 → 24, and that number needs context `10 unwired call site(s) across 8 files` → `24 across 15`. **No entry was removed** — every previously-recorded file kept its count and 14 sites became visible for the first time: ``` core/task-store/reads.ts 0 -> 6 engine/self-healing.ts 2 -> 4 core/task-store/branch-and-pr-entities.ts 0 -> 1 core/task-store/task-update.ts 0 -> 1 engine/scheduler.ts 0 -> 1 dashboard/routes/register-task-workflow-routes 0 -> 1 cli/commands/dashboard-tui/bucket-mapping.ts 0 -> 1 cli/extension.ts 0 -> 1 ``` **These are newly VISIBLE, not newly broken** — they have been unwired all along. I have **not** audited them, and recording them in the baseline is not a claim that they are fine; it is the ratchet doing what its header describes, since the census's own note says roughly half of the original hits were legitimately unwired (identity proven by a stronger means, sentinel columns, dead exports). Someone should walk the 14. Two stand out as worth a look first: **`reads.ts` at 6** is the file #2956 was about, and **`scheduler.ts`** is a dispatch path. Flagging rather than fixing, because wiring a call site that should not be wired is its own defect and each needs the judgement call the census header describes. ### Regression test `packages/engine/src/__tests__/lane-wiring-census-named-types.test.ts` pins the detector's shape — named interface, type alias, inline literal, positional — against fixtures rather than live counts, so it does not churn when someone legitimately wires a call site. Plus one anti-vacuity case asserting the named-type arm is still load-bearing on real source (`getInReviewStallReason` resolves in the live tree), so the fixtures cannot pass while the tool has quietly stopped applying here. **Mutation:** removing the `TypeReference` arm fails **4 of 5**. ### Also worth knowing `findLaneAcceptingFunctions` still only visits `ts.isFunctionDeclaration` at top level, so `export const fn = (ctx) => …` remains invisible. I checked — no exported arrow function currently takes a lane argument, so nothing is missed today, and I left it rather than widen the surface in the same change. Resolved by **name across the corpus** instead of a type-checker `Program`: these are plain source scans and a checker would cost a full type-resolution pass for one lookup. Two same-named types merge, which only ever widens what counts as wired — safe for a ratchet. **Verified:** 5/5 new tests, `check-lane-wiring` clean at the new baseline, lint clean, FNXC gate exit 0. Core suite on main is green (4923 passed / 0 failed) — unrelated, but I had it running. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved lane-wiring analysis to recognize named interfaces and type aliases. * Added support for wrapped configuration expressions and positional parameters when detecting lane information. * **Tests** * Added comprehensive coverage for lane-wiring detection, including named contexts and live-tree validation. * **Chores** * Updated baseline counts to reflect newly recognized application areas and improved self-healing detection. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…found while testing, not converting) (#3208) **A defect, not a coverage gap** — found while trying to pin `agentParkedColumns` from the #3115 map. `recoverDriftedAgentTaskLinks` enters its preservation branch on a **resolved** question (`isPreWipColumn`) and then decided it on a **literal** one: `evaluateParkedAgentTaskLink` was called without `parkedColumns`, so parked-ness fell back to `todo`/`triage`. The sibling sweep passes the resolved set; this call site did not. On a renamed board: the card is pre-wip, `isParkedTaskColumn` says no, `shouldPreserveParkedLink` is false, and **an agent with a fresh heartbeat run has its task link cleared — while it is working.** `task-agent-sync.ts` predicted this in writing when the parameter was introduced: > *"turning a stale-link bug into a dropped-link bug, since the card would be treated as unparked and its live agent link cleared"* That is what an unpassed optional lane parameter costs — the same missed-pair shape as #2956, #2963 and #3186. ## The test needed two fixture corrections, both caught by failing - the renamed IR had **no hold column**, so no card could be pre-wip at all; - the **per-task selection readers** were missing, so `isPreWipColumn` resolved the default IR and the branch was never entered. Either alone made the case pass while exercising nothing. Third time today a fixture passed for a reason unrelated to the resolver — a pattern, not an anecdote. ## Measured 15 pass; removing `parkedColumns` from the call fails exactly this case. ## Note on how it was found I had discarded a probe at this sweep earlier for failing to discriminate. Coming back with the obstacle understood — the fixture must reach the branch the resolver gates — turned a coverage miss into a defect find. The five discards this session were not wasted; three of them named the obstacle that made a later attempt work. ## Verification `self-healing-agent-link-drift` **15 passed** · `pnpm test:gate` 161 + 13 + 499 + 71 · lint · lane-wiring — green.
Main is red, and the red is pointing at a real defect
unwired-lane-parameter-guardfails onorigin/mainafter #2951. This is not a stale allow-list — the parameter genuinely never reaches the function.#2951 added
reviewColumns?: ReadonlySet<string>to three signal modules and wired two of them completely.getInReviewStallReasonwas wired at none of its four call sites. Measured by brace-matching each call's option literal:The user-visible consequence
reads.tscomputes two adjacent signals for the same card. On a board declaring a separate merge lane beside its human-review lane,inReviewStallread the first review column only, whileinReviewStalled— three lines below — read the set.The same card is "in review" for one signal and not the other. Two signals disagreeing is worse than both being legacy, and it is invisible on every builtin board because there the review set has exactly one element.
At three of the four sites the resolve sat below the call, which is why the parameter could not be passed. Those are hoisted.
I have to correct my own earlier report
On #2951 I said "3 of 4 call sites wired,
reads.ts:227is the gap." That was wrong. I had measured with a 12-line proximity grep, which bled into the adjacentgetInReviewStalledSignalcall and counted itsreviewColumns:as the first call's. Brace-matching the literal shows 0 of 4. The defect was four times larger than I reported, and the cause was exactly the anti-pattern I have spent this session filing against other people's guards — a proximity window standing in for structure.Naming the context types
The guard keys an interface member to its owner symbol and only counts a mention from a file that also names that owner, so passing the property inline reads as unwired even when every site supplies it.
satisfies InReviewStalledContext/satisfies StalePausedReviewContexton the option literals is real type-checking, not a decorative import — lint rejected the decorative version, correctly.New test, because the existing guard cannot see this
Measured: deleting the
reviewColumns:line from a fixed call site leavesunwired-lane-parameter-guardat 9/9 green, because the file still names the type. So the wiring I just fixed had no coverage at all.The new ratchet brace-matches each call site's option literal:
It also asserts it found call sites before checking them — a parse that matched nothing would be vacuous, which is the failure mode this guard family keeps producing. (It caught me mid-change too: an earlier scripted edit left the file syntactically invalid and the source-text test still passed 3/3. It is a wiring ratchet, not a substitute for
tsc.)Verification
Core 4861 passed / 0 failed · guard 9/9 with
KNOWN_UNWIREDunchanged ·pnpm test:gateexit 0 · lint clean · coretsc0 errors.🤖 Generated with Claude Code