gate: the lane-wiring ratchet did not scan plugins, dashboard/app, or any .tsx - #2978
Conversation
|
Warning Review limit reached
Next review available in: 31 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 (2)
📝 WalkthroughWalkthroughThe lane-wiring census now scans dashboard and plugin sources, including TSX files. The baseline records counts for the newly covered source files. ChangesLane-wiring census
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Rebased onto #2976 wired Resolved by regenerating from the actual scan rather than hand-merging the conflict — the script is the source of truth for this file, and hand-editing a generated baseline is how conflict markers get committed into one: 19 → 18, exactly the CLI entry leaving; |
dd663f3 to
e9d1a5e
Compare
|
Verified this composes with #2974, and measured the combined result — the two conflict, and the number neither PR states is the one that will actually land. Both regenerate Not a problem in itself — it's a generated file — but it must be regenerated, never hand-merged. A hand-merged baseline is a number nobody measured, which is the failure the whole ratchet exists to prevent. Measured after merging #2974 into this branch and regenerating:
Neither PR's number survives. That's expected and correct — widening the scan roots (yours) and widening what the detector can see (mine) multiply rather than add, since your new roots contain named-context call sites mine can now resolve. The invariant still holds on the combined tree, which is the part I actually wanted to confirm: re-introducing #2956 fails with So: whoever merges second should run Separately — your framing that the new gate re-opened a blind spot the sibling guard had already learned about is the right read, and it's the same shape as the two blind spots found in my aria matcher today: a new tool encodes the cases in front of its author instead of the ones the codebase actually writes. Scanning six roots because the older guard's FNXC note already records why six is beating that by reading the note. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/check-lane-wiring.mjs (1)
17-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the FNXC comment.
The block contains historical narrative and implementation detail. Keep only the technical decision: scan
packages/dashboard/appandplugins, include.tsxfiles, and retain the audited baseline. A shorter comment is easier to update.As per coding guidelines, FNXC comments must be concise, use the
FNXC:Area-of-productprefix with ayyyy-MM-dd-hh:mmtimestamp, and stay current as requirements change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-lane-wiring.mjs` around lines 17 - 30, Shorten the leading FNXC comment in check-lane-wiring to state only that packages/dashboard/app and plugins are scanned, .tsx files are included, and the audited baseline is retained. Preserve the FNXC:Area-of-product prefix and update the timestamp to the required yyyy-MM-dd-hh:mm format.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-lane-wiring.mjs`:
- Around line 31-33: Update the audited call-site count in the note within
scripts/check-lane-wiring.mjs from 9 to 10, matching the generated baseline and
PR objective; leave the surrounding audit explanation unchanged.
- Around line 35-42: Update the root-scanning logic in check-lane-wiring so
failures for every required entry in ROOTS do not silently become an empty file
list. Remove the fallback catch or rethrow an error that includes the specific
root path, ensuring missing or unreadable roots fail pnpm check:lane-wiring.
---
Nitpick comments:
In `@scripts/check-lane-wiring.mjs`:
- Around line 17-30: Shorten the leading FNXC comment in check-lane-wiring to
state only that packages/dashboard/app and plugins are scanned, .tsx files are
included, and the audited baseline is retained. Preserve the
FNXC:Area-of-product prefix and update the timestamp to the required
yyyy-MM-dd-hh:mm format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4651a97f-7413-4c5e-bc56-d1729ec105a2
📒 Files selected for processing (2)
scripts/check-lane-wiring.mjsscripts/lib/lane-wiring-baseline.json
… any .tsx `check-lane-wiring.mjs` scanned four roots and only `.ts`. The older `unwired-lane-parameter-guard.test.ts` scans six INCLUDING `packages/dashboard/app` and `plugins`, and its note records why: an unwired `completeColumnsByTaskId` sat on `main` unreported because the glasses plugin was not in the list. That lesson was re-opened by the newer gate. Two changes, and either alone still misses most of it — those trees are overwhelmingly `.tsx`, which the file filter excluded, so adding the roots without the extension would have scanned a handful of files and reported a reassuring near-zero. Measured on the widened scan: 10 further call sites that no gate could see, in 8 files. Recorded in the baseline rather than fixed — they span three other batches — but audited rather than blind-baselined; the classification is in the PR body. Two are real defects (the graph's stuck badge never fires on a renamed board; the dashboard sort/duplicate sites), three are a deprecated plugin that depends on `@fusion/plugin-sdk` alone and cannot resolve anything, and one is harmless by construction. Note for merge order: a DECREASE also exits 1, and #2976 wires `packages/cli/src/commands/task-lifecycle.ts` (present in this baseline). Landing that after this one turns main's gate red until the baseline is re-recorded. Loudly, not silently — but it needs the re-record. Verified: gate green at 19 known / none added; the older guard's suite 9 passed; census, sql-literal and fnxc ratchets green.
Rebased again onto #2974, and the audit above needs correcting — it is now 14 sites, not 10#2974 improved the census's declaration detection, so the widened scan sees more than when I opened this. Regenerated rather than hand-merged; baseline is 30 in 22 files (main's narrow scan reads 16 in 12). Gate green, Newly visible beyond the original table — audited, not assumed:
The false positives are worth naming, because they are a new interactionBoth take a single But the lane answer is carried on the entity and correctly consumed inside ( This is the same class I flagged on #2966 before #2974 landed: resolving named parameter types closes one gap and opens this one. I am not attempting a heuristic fix — I tried three variants of exactly that on the older guard and each traded the false positive for worse false negatives (the widest hid twelve genuine entries). Baselining them keeps the ratchet catching new real sites; the cost is that a future genuine unwiring in those two files is masked, which I would rather state than hide. Worth the census owner's call whether a parameter whose type is a domain entity (rather than an options bag) should count as lane-accepting at all — that is the distinction that separates these two from the |
e9d1a5e to
e687422
Compare
…it count
Review findings on the widened lane-wiring scan, plus the rebase onto
main.
1. `catch { return [] }` turned a missing or unreadable root into an
empty file list, so a typo'd path or a permissions failure produced a
smaller scan and a confident "none added". Every root is part of the
coverage contract -- plugins and dashboard/app were added BECAUSE they
were unscanned -- so silently dropping one restores exactly the
blindness this change removes. Now throws with the root path.
2. The audit note said 9 further call sites; the review said 10 from the
pre-rebase baseline. Both are now wrong: main gained sites in these
same trees while the PR sat, so the widening currently uncovers 15.
Re-measured and stamped so the next reader re-measures rather than
trusts a hand-written number beside a generated baseline.
Rebase conflict was on the generated baseline: took main's version and
regenerated with --update-baseline rather than hand-merging the JSON,
which would produce a number matching neither branch.
Mutation-verified: a typo'd root exits 1 naming the root; restoring it
exits 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e687422 to
841456e
Compare
|
Rebased onto main and both findings fixed in Fail closed on an unscannable root — real, and the more important oneFixed as suggested, with the root path in the message. That matters more here than in most places: Mutation-verified: pointing a root at a nonexistent path exits and restoring it exits The audited count — correct finding, but both numbers are now staleThe note said 15, not 9 or 10. Main gained call sites in these very trees while the PR sat, so both figures were accurate when written and neither is now. Updated to 15 and stamped with the reason, so the next reader re-measures instead of trusting a hand-written number sitting beside a generated file. That drift is the underlying problem — the count and the baseline age at different rates. The rebase conflictOn Verification
|
Follow-up on the one real site in my own area, now traced to its exact blockerI listed
So a plugin-rendered card has no route to the board's trait flags. Fixing it means extending the dashboard's plugin API to expose resolved flags to plugin views, then threading them — a public-surface change to the plugin contract, not a one-line edit in my plugin. Consequence while it stands: on a renamed board, Left for whoever owns the plugin dashboard API. Recorded here rather than in a stale TODO because this PR is what made the site visible in the first place. |
Handoff for the three
|
|
Post-merge confirmation: the combined baseline landed at exactly 30 sites / 22 files on Both gates are clean and self-consistent on main, with no self-write — running the check leaves the tree clean, so the baseline was committed rather than regenerated-on-read. That was the failure mode that let a stale baseline reach main earlier today via More importantly, the invariant survived all three merges landing in sequence:
|
Addendum: the practical obstacle, and why I decided against a Board-only fixTracing the three consumers:
So in I decided against doing Board alone anyway, and the reason is this program's own lesson rather than reluctance: wiring one of three call sites produces exactly the partial-wiring shape that motivated the gate in the first place — #2956 was "wired at 0 of 4 call sites while both siblings were wired", and a half-converted fanout would read as converted while two surfaces still answer with legacy literals. The other two need a per-task flags source over the whole task list that neither component currently has, which is a real design step, not a mechanical edit. Whoever picks this up gets the whole set or a deliberately-scoped subset with the remainder named — not an accidental one. |
…nto boards that do not have one (#2999) ## A repair script that wrote a column the board does not have Under `--apply`, against an operator's live database: ```js await tx.execute(sql`UPDATE project."tasks" SET "column" = 'archived' WHERE id = ${row.id}`); ``` On a board that does not declare `archived`, that is not a mislabel — it parks the row in a column the workflow does not have, **manufacturing exactly the undeclared-column state this migration keeps repairing elsewhere**. The selection was wrong in the same direction, which made the write far worse. "Leaked" meant `column !== "archived"`, so on a renamed board **every** soft-deleted row looked leaked — including the ones resting correctly in that board's own archived lane. The repair then rewrote them. The tool's fix *was* the damage. ## Three changes, because fixing one would have left the others deciding **The SQL pre-filter carried the same literal** (`AND "column" != 'archived'`), so the query and the planner each imposed the legacy vocabulary independently. Dropped it — soft-deleted rows are a small set, so selecting them all and filtering in the pure planner costs nothing and leaves **one** place that decides what "archived" means. **The filter takes the set**; a row resting in *any* of the board's archived lanes is not leaked. **The write resolves per task**, because the destination must be that card's own lane, not a board-wide pick. A row whose archived lane cannot be resolved is **skipped and reported**, never written with a guessed id. A recovery script that declines to act on rows it does not understand is recoverable; one that writes a plausible wrong value is not. Verified rather than assumed — a store that answers nothing resolves to the default lifecycle: ``` lifecycle from unanswering store: {"intake":"todo",…,"archived":"archived"} ``` so a legacy board repairs exactly as before. ## Correcting myself On #2994 I wrote that this follow-up "needs the same `importCore` seam". It doesn't: `openBackend` already returns `{ core, store, … }` and this script already destructures `core`. No new plumbing was required. I posted that correction on #2994 too, since acting on it would have wasted someone's time. ## Revert proof ``` ✖ a soft-deleted row already in the board's RENAMED archived lane is not leaked ✖ a board with several archived lanes treats all of them as resting places ℹ pass 5 ℹ fail 2 ``` The other two new cases pass both ways by design — they guard the legacy meaning and the still-catches-a-real-leak direction — so I am not counting them as coverage of the defect. ## Verification (measured) - `node --test` across all three script suites — **17 passed / 0 failed** - `node --check`, `eslint` — clean - `check-sql-column-literals`, `lifecycle-column-census --strict`, `check-lane-wiring`, `check-fnxc-future-dates` — green No changeset: root `scripts/` is repo tooling, not part of the published package. ## Gate blind spot found while verifying this, NOT fixed here I removed a raw-SQL lane literal and expected `check-sql-column-literals` to drop from 22 — its own header says *"a LOWER count fails too so the baseline is ratcheted down"*. It stayed at **22 and green**, because it walks `PACKAGES` only and **never scans `scripts/`**. That is the same shape as the lane-wiring gap #2978 closed (it scanned neither `plugins` nor `dashboard/app`). `scripts/audit-branch-cross-contamination.mjs:185` still holds `WHERE … "column" IN ('triage','todo','in-progress','in-review')`, invisible to the gate. Left as a separate follow-up rather than bundled into a product fix.
…e raw SQL actually is `check-sql-column-literals` walked `packages/` only and took `.tsx?`, while every operator script is a repo-root `.mjs`. So the one place in the tree that writes raw SQL by hand was the one place this gate could not see. Found by removing a raw-SQL lane literal in #2999 and watching this gate report "22 known, none added" — unchanged and green. Its own header promises the opposite ("a LOWER count fails too so the baseline is ratcheted down"), so the silence was the tell. TWO changes, and either alone still sees nothing: the root and the extension. Adding one without the other scans nothing new and reports a reassuring zero — the same trap #2978 hit widening the lane-wiring census. Newly visible, audited rather than blind-baselined: audit-branch-cross-contamination.mjs:182 "column" IN ('triage','todo','in-progress','in-review') Real: the contamination audit scans only the legacy active lanes, so on a renamed board it scans nothing and reports no contamination. Read-only, and it does print its `scannedColumns`, which is the one thing keeping that from being silent. reconcile-leaked-soft-deletes.mjs:53,73 Both already fixed by #2999, which is the PR that exposed this gap. Proven able to fail, not just to count: a temporary `.mjs` holding one forbidden comparison was reported ("baseline allows 0") and the gate returned to green when it was removed. The ScriptKind move to JS for `.mjs` is DEFENSIVE and I could not demonstrate it was necessary — three JSX-ambiguous shapes all recovered under TSX with identical counts. Said plainly in the code, because the opposite claim would be easy to make and wrong. MERGE ORDER: #2999 removes both literals in reconcile-leaked-soft-deletes.mjs. Landing it after this one drops the count and this gate fails on DECREASE, needing a re-record. Merge #2999 first, or ping me and I will re-record here. Verified: gate green at 28 known / none added; its own suite 32 passed; eslint clean; census, lane-wiring and fnxc ratchets green.
…e raw SQL actually is (#3000) ## The gate could not see the one place raw SQL is actually written by hand `check-sql-column-literals` walked `packages/` only and took `.tsx?`. Every operator script is a repo-root `.mjs`. I found it by removing a raw-SQL lane literal in #2999 and watching this gate report: ``` [check-sql-column-literals] 22 known SQL column literal(s), none added. ``` Unchanged, and green. Its own header promises the opposite — *"a LOWER count fails too so the baseline is ratcheted down"* — so the silence was the tell. **Two changes, and either alone still sees nothing:** the root and the extension. Adding one without the other scans nothing new and reports a reassuring zero — the same trap #2978 hit when widening the lane-wiring census. ## Newly visible: 6 sites, audited not blind-baselined | site | verdict | | --- | --- | | `audit-branch-cross-contamination.mjs:182` — `"column" IN ('triage','todo','in-progress','in-review')` | **real** — the contamination audit scans only the legacy active lanes, so on a renamed board it scans nothing and reports no contamination. Read-only, and it does print its `scannedColumns`, which is the one thing keeping that from being fully silent. | | `reconcile-leaked-soft-deletes.mjs:53, :73` | already fixed by **#2999** — the PR that exposed this gap | ## Proven able to fail, not just to count A guard that has only ever printed a number is a number. A temporary `.mjs` holding one forbidden comparison: ``` scripts/zz-probe-tmp.mjs: 1 SQL column literal(s), baseline allows 0 ``` and the gate returned to green once removed. ## One claim I withdrew I initially wrote that the `ScriptKind` move to `JS` for `.mjs` was needed because *"TSX treats `<` as JSX and would misparse an ordinary comparison"*. I could not demonstrate it. I tried three JSX-ambiguous shapes — `x <div> y`, `f<b, c>(d)`, and a literal sandwiched between `<` and `>` comparisons — and TSX recovered from all three with counts identical to JS. So `JS` is used because it is the correct kind for the file, **not** because a miss was observed, and the code now says exactly that. The opposite claim would have been easy to make and wrong, and this gate's whole value is that its statements about its own coverage are true. ## Merge order **#2999 removes both literals in `reconcile-leaked-soft-deletes.mjs`.** Landing it *after* this PR drops the count, and this gate fails on DECREASE (by design), needing a re-record. Merge #2999 first, or say the word and I will re-record here. Note the widening is self-protecting afterwards: if someone narrows the walk back to `packages/`, the recorded `scripts/` entries vanish from the scan and the gate goes red on decrease. ## Verification (measured) - gate — green, **28 known / none added** (was 22 across `packages/` only) - its own suite — **32 passed** - `eslint` — clean - `lifecycle-column-census --strict`, `check-lane-wiring`, `check-fnxc-future-dates` — green Gate/tooling only; no product file touched.
The new gate re-opened a blind spot the old one had already learned about
check-lane-wiring.mjs(#2966) scanned four roots and only.ts:unwired-lane-parameter-guard.test.tsscans six, includingpackages/dashboard/appandplugins, and its FNXC note records exactly why:That is the same trap here, and it needed two changes. Those trees are overwhelmingly
.tsx, which the file filter excluded — so adding the roots without the extension would have scanned a handful of files and reported a reassuring near-zero.What the widened scan found: 10 sites, in 8 files, audited not blind-baselined
dependency-graph/GraphTaskNode.tsx(isTaskStuck)isTaskStucktakes an optional 4thcolumnFlags; omitted,isWipColumnRolefalls back to the literal, so no card on a renamed board is ever shown stuck in the graphdashboard/app/Lane.tsx,ListView.tsx(sortTasksForDisplayColumn)dashboard/app/ModelSelectorTab.tsx×2 (resolveEffectiveExecutor/Validator)dashboard/app/TaskDetailModal.tsx(isNearDuplicateCanonicalInactive)even-cards/routes/board-routes.ts×3 (boardToDeck)@fusion/plugin-sdkalone, with no resolution sourceeven-realities-glasses/routes/board-routes.ts:141(boardToDeck){ maxCards: 1 }summary call slicesactiveto empty, soterminalColumnscannot change its output; documented incards.tsThey are baselined rather than fixed because they span three other batches. I did not fix the graph one despite it being my area: wiring it needs the plugin prop contract to carry column flags, and the plugin's own
dashboard-interop.d.tsdeclaresisTaskStuckwith only three parameters — so it crosses the dashboard↔plugin API boundary rather than being a local change.Merge-order hazard, stated precisely
A decrease also exits 1 (
process.exit(1)on thedecreasedbranch), and #2976 wirespackages/cli/src/commands/task-lifecycle.ts, which is present in this baseline. If #2976 lands after this PR, main's gate goes red until the baseline is re-recorded.It fails loudly rather than silently, so it is a chore not a risk. Merging #2976 first and letting me re-record here is the cleanest order — say the word and I will push the re-record.
Verification (measured)
check-lane-wiring— green, 19 known / none added (was 9 across 4 roots)unwired-lane-parameter-guard.test.ts— 9 passed, the older guard is unaffectedlifecycle-column-census --strict,check-sql-column-literals,check-fnxc-future-dates— greenGate/tooling only; no product file is touched.
Summary by CodeRabbit
.tsxfiles while excluding declarations, tests, specs, and ignored directories.