feat(gate): freeze the SQL column-literal surface — 30 sites, none may be added - #2841
Conversation
…y be added A legacy column id inside a query string is invisible to every existing check. The lifecycle census parses TypeScript COMPARISONS and a SQL literal is string data; the inert-seam gate reasons about parameters and call sites. The surface was uninstrumented. What it cost: cleanupStaleMergeQueueRowsImpl filtered on `t.column != 'in-review'`, so on a renamed board every queued card looked stale, its merge_queue row was deleted, and the card became unleaseable. Found by the operator on #2819, in SQL I had already read past during that same work. The quieter half is analytics: five sites count `"column" = 'done'`, so throughput, cycle time, and team dashboards report zero completed work on a renamed board. Nothing errors. This does NOT fix the sites — `resolveProjectColumnsForRoles` is the mechanism and its migration has an owner (#2839). It freezes the population: a new file or higher count fails, and a LOWER count fails too, so the baseline ratchets down as sites migrate instead of leaving slots to regrow into. AST-based because a line grep for the same pattern reports 37 hits, 25 of them prose quoting `column === "done"` in explanatory notes. A guard that is 68% false positives trains readers to skip it — a lesson already paid for in this program. TWO CORRECTIONS FOUND BY MUTATION-TESTING MY OWN GATE: - Requiring a SQL keyword in the same literal missed CLAUSE FRAGMENTS. team-analytics.ts builds ["assignedAgentId IS NOT NULL", `"column" = 'done'`, ...] and joins them into a WHERE later; the fragment is as vocabulary-bound as any query but contains no keyword. Fixing that took the population from 12 to 14, so the count reported on #2839 was low. - My first mutation test "proved" a direction it had not: I replaced the first textual occurrence in a file, which was in a COMMENT, and read the unchanged count as the scanner being wrong. The scanner was right. All three directions are now driven against real SQL: adding a full query fails, adding a bare fragment fails, and migrating one away fails until the baseline is re-recorded. Wired into test:gate and both pretest hooks. Runtime is a single AST pass. 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)
📝 WalkthroughWalkthroughAdds an AST-based audit for legacy SQL column literals, records existing per-file counts, and integrates the audit into pre-test and gated test commands. ChangesSQL column literal audit
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds an AST-based gate that freezes the per-file population of legacy SQL column literals.
Confidence Score: 4/5The PR is not yet safe to merge because deeply nested IN predicates can still add legacy SQL literals without changing the recorded population. The current IN_BODY expression balances only two nested groups, so a predicate such as Files Needing Attention: scripts/check-sql-column-literals.mjs, scripts/tests/check-sql-column-literals.test.mjs
|
| Filename | Overview |
|---|---|
| scripts/check-sql-column-literals.mjs | Implements the AST scanner and ratcheting baseline check, but the prior nested-IN bypass remains reachable beyond the hard-coded two-level nesting bound. |
| scripts/tests/check-sql-column-literals.test.mjs | Adds focused regression coverage for the previously reported matcher blind spots, with nested-IN coverage limited to the implementation's two-level bound. |
| scripts/lib/sql-column-literals-baseline.json | Records the current per-file legacy SQL comparison counts used by the gate. |
| package.json | Integrates the new scanner into both pretest hooks and the gated test workflow. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Walk packages TypeScript files] --> B[Parse TypeScript AST]
B --> C[Decode string and template literals]
C --> D[Recognize column interpolations]
D --> E[Match legacy SQL comparisons]
E --> F[Count matches per file]
F --> G{Counts equal baseline?}
G -->|Yes| H[Gate passes]
G -->|Increase or decrease| I[Gate fails and requests migration or baseline update]
Reviews (9): Last reviewed commit: "fix(gate): IS DISTINCT FROM was missing,..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-sql-column-literals.mjs`:
- Line 75: Remove the file-level SQL_SHAPE guard that continues before
BARE_CLAUSE runs. Ensure every file reaches BARE_CLAUSE validation, including
files containing only SQL fragments such as column comparisons, while preserving
the existing checks and reporting behavior.
- Around line 83-89: Update the literal-matching logic around node.getText(sf)
to use decoded literal content: use the node’s text for StringLiteral and
NoSubstitutionTemplateLiteral nodes, and handle TemplateExpression nodes using
their decoded text and span text values. Preserve quote stripping and comparison
matching behavior, then re-record the baseline.
🪄 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: 2b4be9f7-8131-4ce6-b769-bd2f80dc2340
📒 Files selected for processing (3)
package.jsonscripts/check-sql-column-literals.mjsscripts/lib/sql-column-literals-baseline.json
…es, so they are gone #2841 review — two findings from greptile, two from coderabbit, against three lines, and all four reduce to one mistake: deciding whether to RUN the comparison regex, using cheaper patterns that disagree with it. - A file-level SQL_SHAPE.test(source) skipped whole files. A file holding only a clause fragment has no SELECT/WHERE anywhere, so a new forbidden site could be added to it and the gate passed — the exact shape the fragment carve-out existed for, reintroduced one level up. - BARE_CLAUSE is anchored ^...$, so a qualified or compound fragment (`t."column" = 'done'`, `("column" = 'done' OR active = 1)`) matched neither pattern and was never compared. - node.getText() returns SOURCE text, where a double-quoted TS string spells the identifier \"column\" with backslashes intact, while every pattern expects the decoded "column". A gate whose false NEGATIVES are this easy to construct is worse than no gate: the baseline it prints reads as coverage. So the pre-filters are deleted — one unanchored pattern over the DECODED text of every string and template literal, nothing left to disagree with. SQL_SHAPE and BARE_CLAUSE are removed rather than left unused, since an unused pattern invites a filter that uses it. THE FALSE-POSITIVE ARGUMENT IS UNAFFECTED, which is what makes this safe: comments are not AST nodes, so walking literals cannot match prose however permissive the pattern gets. Pinned as a case. IT IMMEDIATELY FOUND A LIVE SITE the old version missed — workflow-analytics.ts builds `t."column" = 'done'` as a clause fragment, qualified, so 1 -> 2. Baseline re-recorded; that is better detection, not new debt. Mutation-verified: restoring getText() fails 1 of 7. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four findings are real, they are one mistake, and the fix is deletion. 09400db. Two from greptile and two from coderabbit, against three lines. Every one reduces to the same thing: the gate decided whether to run its comparison regex, using cheaper patterns that disagreed with it.
A gate whose false negatives are this easy to construct is worse than no gate, because the baseline it prints reads as coverage. So the pre-filters are gone: one unanchored pattern over the decoded text of every string and template literal, with nothing left to disagree with. The false-positive argument is untouched, which is what makes this safe. It never depended on the pre-filters: comments are not AST nodes, so walking literals cannot match prose however permissive the pattern gets. That property is now pinned as its own case. It immediately found a live site the old version missed. Seven cases added ( Mutation-verified: restoring |
… 14 sites to 31 #2841 review, second round. Two more false negatives, and the second one matters most. IN PREDICATES. The operator list was = != <>, so `"column" IN ('in-progress','in-review')` counted nothing and a second one could be added with the baseline green. team-analytics.ts and workflow-analytics.ts each held one, unseen. Each legacy ELEMENT counts, matching how the = arm counts two comparisons in one query. INTERPOLATED COLUMNS — the serious one. literalText joined only the STATIC template spans, on the reasoning that an interpolated expression cannot be part of a comparison. That is backwards for the dominant production shape: a Drizzle template puts the COLUMN in the hole and the legacy id in the static text, so `${schema.project.tasks.column} != 'archived'` joined to text with no column identifier and matched nothing. The merge-queue and self-healing queries this gate exists to freeze are written exactly that way — it was blind on the files it was built for. Enabling the shape revealed FIVE previously invisible files. A non-column interpolation now renders as a NUL sentinel, not a space: a space lets `= ${expr}'done'` join into `= 'done'`, a comparison not in the source. Pinned as a case, which is how I found it. ALSO GUARDED THE ENTRY POINT. The test imports the matcher, and importing ran the whole gate and called process.exit(1) — so the test file failed for the gate's reasons, and while the gate was green it passed for reasons unrelated to what it asserts. Baseline re-recorded 14 -> 31. That is detection catching up with reality, not new debt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both real, and the second one means this gate was blind on the exact files it was written for. f788d83.
Interpolated columns — the serious one. Enabling that shape revealed five previously invisible files:
Detection catching up with reality, not new debt. Two things I found while fixing these, both worth recording:
12 cases now ( |
#2841 review, third round. `[^)]*` stops at the first `)`, and any nested call supplies one — so `"column" IN (COALESCE(x, y), 'done')` never reached its legacy id and the predicate counted zero. Third false negative of the same family as the previous two rounds: a shape the pattern did not describe. The IN body now tolerates nested groups two levels deep, which covers the realistic worst case here (`LOWER(COALESCE(a, b))`). THE BOUND IS STATED RATHER THAN HIDDEN. A regex cannot balance arbitrary nesting, and the alternative — matching the predicate head and extracting the balanced region programmatically — buys a depth nobody writes at the cost of a second scanner to keep correct. At three levels the gate under-counts again; that is a known limit now, not an unknown one. Repo count unchanged at 31 (no current site nests), so this closes a hole rather than finding one. Mutation-verified: restoring `[^)]*` fails 2 of 15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correct — third false negative of the same family. 96a6ac4.
The The bound is stated rather than hidden, which is the part I want on the record. A regex cannot balance arbitrary nesting. The alternative — match the predicate head and extract the balanced region programmatically — buys a depth nobody writes here, at the cost of a second scanner to keep correct. At three levels this under-counts again. That is now a known limit with a comment naming it, rather than an unknown one; if a site ever nests that deep, the fix is the extraction approach, not a third regex tier. Repo count is unchanged at 31 — no current site nests — so this closes a hole rather than finding one, unlike the previous two rounds. Three cases added including the paired negative (tolerating nesting must not turn every Pattern worth noting across the three rounds of this PR: every finding has been a false negative, and each arrived only after the previous one was fixed — pre-filters, then |
…e-coordination 31 -> 30. main's lifecycle work removed the last legacy comparison from task-store/async-merge-coordination.ts, and the check failed because the file's baseline entry still allowed 1. That is the downward direction firing in real use rather than in a mutation test: a migrated site must re-record, or the slot it left behind can be silently regrown into later. Also merges current main into the branch (no conflicts). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Updating this PR's own description, because it no longer matched its code. The population is 30, not the 14 in the original title. Two rounds of review found five classes of false negative in my scanner, all fixed on this branch:
I am spelling these out rather than quietly updating a number: a gate whose false negatives are this easy to construct is worse than no gate, because the baseline it prints reads as coverage. My mutation tests all passed because I only ever mutated shapes my own scanner already understood — the blind spots were outside the space I probed. The pre-filters are gone entirely now: one unanchored pattern over the DECODED text of every string and template literal, with nothing left to disagree with it. The false-positive argument is unaffected, since comments are still not AST nodes. Also in this push: baseline 31 → 30. main migrated the last legacy comparison out of |
This gate reported "14 sites" for days while the real population was 31. The gap was five classes of
false negative, and the one that mattered was found by asking "why is the merge-queue query — the
reason this check exists — not in the output?". That question is unanswerable against a count.
Reading the 26 matches it now prints immediately splits the population, which the count concealed:
- 11 are "column" != 'archived' / <> 'archived' exclusions. Archiving sets task.column='archived'
unconditionally as a real STATE rather than a board lane, so these are probably correct and
vocabulary-independent — the same conclusion reached by hand for archive-lifecycle-2.ts earlier.
- 14 are "column" = 'done' and "column" IN ('in-progress','in-review') in analytics and merge
paths. Those are genuinely lane-bound: on a renamed board they silently count zero.
So the frozen population is NOT a work list, and issue #2839 presented it as one. Corrected there.
A tool that freezes a population has to be able to show it, or its own number is the only evidence
anyone has for what it covers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ef6af91 to
e6f3293
Compare
#2841 review, fourth round. `schema.project.tasks["column"]` is the same reference as `schema.project.tasks.column`, and only the dot form was matched — so an element-access reference fell through to the NUL sentinel and its whole predicate vanished. The identical blindness the static-span join had, reachable by changing punctuation. Drizzle accepts both spellings, and a formatter or a reserved-word column name can produce the bracket one, so this is not a hypothetical shape. Both quote styles are tolerated, for the same reason the rest of this scanner is permissive: a false positive costs one baseline entry, a false negative costs the gate its meaning. The paired negative is covered — a bracket access to a different property must not read as a column, or every interpolation becomes one. Repo count unchanged (no current site uses brackets), so this closes a hole rather than finding one. 18 cases now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fourth round, same conclusion: correct. be50b65.
Both quote styles are now tolerated, for the same reason the rest of this scanner is permissive: a false positive costs one baseline entry, a false negative costs the gate its meaning. The paired negative is covered — a bracket access to a different property must not read as a column, or every interpolation becomes one. 18 cases; repo count unchanged, so this closes a hole rather than finding one. Four rounds in, the pattern in the findings is worth naming, because it is a property of this gate's design and not of any one bug. Every finding has been a false negative, each arrived only after the previous was fixed, and each was a text shape I had not modelled: pre-filters, then The 18 cases mean the next unmodelled shape fails a specific named test rather than widening the blind spot invisibly. But the structural fix, if a fifth round arrives, is to stop matching text and resolve the column reference through the TypeScript type-checker — which is a materially bigger instrument than a script, and worth doing only if the shapes keep coming. Flagging that as the escalation path rather than adding a fifth regex tier by reflex. |
… to tolerate #2841 review, fifth round. The last commit's comment said "both quote styles and an optional trailing `!`/`?` are tolerated" and the regex implemented only the quote styles. The comment described the intent, the regex described the behaviour, and only one of them runs — so a non-null-asserted column reference still fell through to the NUL sentinel and its predicate vanished. Worth stating plainly: a comment that overstates a guard is worse than no comment, because it is the thing a reader checks INSTEAD of the code. Four cases now pin the behaviour rather than describing it, including the negative — an assertion on a different property is still not a column. 22 cases. Repo count unchanged; this closes a hole rather than finding one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correct, and this one is squarely my error rather than an unmodelled shape. Pushed. The previous round's comment says "both quote styles and an optional trailing A comment that overstates a guard is worse than no comment, because it is the thing a reader checks instead of the code. That is the same failure this whole PR is about, one layer up: a claim of coverage that nothing executes. Four cases now pin the behaviour instead of describing it — bracket-with- Fifth round, and the escalation note from the last comment now has a concrete trigger. Four of the five findings were unmodelled text shapes; this one was a documented-but-unimplemented shape. Both are symptoms of specifying a reference by its spelling. If a sixth arrives, the fix is to resolve the column reference through the TypeScript type-checker rather than adding a sixth pattern tier — I would rather that decision be made deliberately than reached by attrition. |
…ge-queue predicate #2841 review, sixth round — and unlike rounds four and five this one was not hypothetical. `async-merge-coordination.ts` writes `${schema.project.tasks.column} IS DISTINCT FROM 'in-review'`, the merge-queue stale sweep, one of the very queries this gate exists to freeze. The operator list held `= != <>` and `IN`, so the predicate counted zero and the file's entry read 0 instead of 1. `IS` and `IS NOT` are covered in the same change rather than left for a seventh round. Enumerating operators one review round at a time is how the previous five holes happened, and the marginal cost of including the neighbouring spellings now is a line of regex. Baseline 30 -> 31, which is detection catching up rather than new debt. 26 cases, including the negative: IS DISTINCT FROM a NON-legacy id must not match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Sixth round, and unlike four and five this one was hiding a live site. fbf91fe.
sql`${schema.project.tasks.column} IS DISTINCT FROM 'in-review'`That is the merge-queue stale sweep — one of the very queries this gate exists to freeze. The operator list held
Six rounds is enough evidence to state the conclusion rather than keep patching. The tally: one pre-filter defect, and five cases where the pattern simply did not enumerate a spelling — That is not bad luck, it is what a regex-shaped specification of a type-level fact does. The gate is asking "does this text look like a column compared to a legacy id" when the real question is "does this expression resolve to I am not doing that inside this PR — it is a materially different instrument and this one is now green with 26 cases pinning the shapes we know. But I would rather that decision be made deliberately than arrived at by a seventh round. |
This branch removes legacy comparisons, and the gate (#2841, merged after this was opened) fails on a LOWERED count so a migrated slot cannot be silently regrown into. Without this the branch turns main red on merge, which is what #2864 did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch removes legacy comparisons, and the gate (#2841, merged after this was opened) fails on a LOWERED count so a migrated slot cannot be silently regrown into. Without this the branch turns main red on merge, which is what #2864 did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch removes legacy comparisons, and the gate (#2841, merged after this was opened) fails on a LOWERED count so a migrated slot cannot be silently regrown into. Without this the branch turns main red on merge, which is what #2864 did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2877 review (greptile P2) — right, and it is the worst kind of docs defect: the table contradicted the rule stated four paragraphs below it, and it would have told the next reader that three genuine defects were intentional. Lines 213/332/345 read `task.column` off a row `select`. That is a board lane by exactly the test this document gives — "look at where the compared value came from" — and I put them in the sentinel bucket anyway. The real split is 5 sentinels, 4 convertible. The consequence is the point of the document, so it is now stated: on a renamed archived lane, 213 keeps a card's documents WRITABLE, and 332/345 reject a legitimate archived-document publication as `parent-not-archived` / `archived-state-inconsistent`. The headline finding survives the correction and is sharper for it — a file's census count is an upper bound on convertible sites, not a work estimate, and this one file holds BOTH classes spelled identically. That, rather than the ratio, is the lesson. The second P2 on this PR is stale rather than wrong: `scripts/check-sql-column-literals.mjs` did not exist when the passage was written, and #2841 has since merged it. Verified present on this branch after merging main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2877 review (greptile P2) — right, and it is the worst kind of docs defect: the table contradicted the rule stated four paragraphs below it, and it would have told the next reader that three genuine defects were intentional. Lines 213/332/345 read `task.column` off a row `select`. That is a board lane by exactly the test this document gives — "look at where the compared value came from" — and I put them in the sentinel bucket anyway. The real split is 5 sentinels, 4 convertible. The consequence is the point of the document, so it is now stated: on a renamed archived lane, 213 keeps a card's documents WRITABLE, and 332/345 reject a legitimate archived-document publication as `parent-not-archived` / `archived-state-inconsistent`. The headline finding survives the correction and is sharper for it — a file's census count is an upper bound on convertible sites, not a work estimate, and this one file holds BOTH classes spelled identically. That, rather than the ratio, is the lesson. The second P2 on this PR is stale rather than wrong: `scripts/check-sql-column-literals.mjs` did not exist when the passage was written, and #2841 has since merged it. Verified present on this branch after merging main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the real count #2877 review, both findings. DETECTOR REFERENCE: not missing, just ahead of this branch. `scripts/check-sql-column-literals.mjs` landed on main with #2841 (ef50244); rebasing brings it in and the citation resolves. BASELINE WAS RED AFTER THE REBASE, which is the gate working rather than a problem: team-analytics.ts went 6 -> 3 as sites were converted on main, and a DROP fails deliberately so a migrated site cannot leave a slot the surface silently regrows into. Re-recorded: 31 -> 28. The doc cited "30 sites" — already stale when written, and a fixed number in prose is a claim that rots. It now cites the current 28 AND says the number moves as sites convert, with the reason the gate catches drift in both directions. The sentinel-count finding was corrected in an earlier commit on this branch: lines 213/332/345 read `task.column` off a row select and ARE board lanes, so calling them sentinels would have marked three genuine defects intentional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**`pnpm test:gate` and both `pretest` hooks fail on `main` right now.** Merge this first. ## What happened #2841 (the SQL gate) merged, then #2864 merged. #2864 removed three legacy comparisons from `team-analytics.ts`, but its baseline entry still allows six — and this gate **fails on a lowered count by design**, so a migrated slot cannot be silently regrown into later. Baseline 30 → 28. ## This is my sequencing error The four analytics conversions were branched and reviewed **before** the gate existed, so none of them carries a baseline update. The gate then landed first, which means **each of them breaks `main` as it merges**. I opened all five without thinking about the order they would land in. The three still open — #2866, #2870, #2871 — will each do this again. I am adding baseline updates to them next so they land clean. ## Note on the downward check The "count went down" failure looks like pedantry until it fires. It exists so a migrated site cannot leave an unused allowance behind for the surface to regrow into — the same rot as an allow-list entry for a deleted function. The real cost is that a conversion and its gate have to land in a known order, which is a coupling I created and did not plan for. ## Verification `pnpm test:gate` green with the re-recorded baseline · lint 0 · `node scripts/check-sql-column-literals.mjs` exit 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n see (#2877) Docs only. Two findings from this unit that cost real time to derive and would otherwise be re-derived by whoever reaches these files next. ## 1. `=== "archived"` is usually a SENTINEL `packages/core/src/task-store/async-comments-attachments.ts` carries **9** census guards — the second-largest single-file count outside `self-healing.ts`. Reading all nine: **exactly one** is a board-column comparison. The other eight compare against a value `getLiveTaskColumn` *manufactures*: ```ts if (row.column === "archived" || row.deletedAt != null) return "archived"; // ← fabricated return row.column; ``` Converting those eight to `isArchivedColumnRole` would keep passing on the built-in board and start **failing** on a renamed one — a soft-deleted parent's documents would become readable. **The conversion makes the renamed board worse**, which is the opposite of what the census count implies. The rule that separates them: look at where the compared value *came from*, not at its type. From `task.column` or a DB field → a board lane. From a function that *returns* `"archived"` as a documented outcome → a sentinel. Consequence worth stating plainly: **a file's census count is an upper bound on convertible sites, not a work estimate.** ## 2. Lane literals inside raw `sql` are in no total at all The Reliability panel had three inputs. Two were call arguments and converted routinely (#2861). The third encoded its lanes in a `sql` fragment: ```sql metadata->>'to' = 'in-review' OR (metadata->>'from' = 'in-review' AND metadata->>'to' = 'done') ``` The census scans `===`/`!==` comparisons; the unwired-lane-parameter guard scans declarations. **Neither can see a string inside a `sql` template**, so this class is not in the backlog number — a second, independent reason the total is a floor. Second known instance after the archived gate in PR #2724, which makes it a pattern rather than an accident. Fixed in #2875, and the doc says so rather than leaving it described as outstanding — a learnings doc that reports a fixed defect as open sends the next reader to a dead end. `scripts/check-sql-column-literals.mjs` (#2841) is the detector for the class and freezes the surface at 30 sites; the two are complementary. ## 3. Sibling files The GitLab importer's `column: "triage"` was fixed in #2843. The Linear importer — written from the same template, with **two tests pinning the bug** — still had it, and was found only by re-grepping an area I had already declared clean (#2860). When a defect is found in a file that has a sibling, the sibling is the next place to look, and no tool will tell you that. ## Verification `pnpm lint` clean. No source change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mplate (#2875) The Reliability panel's **third and last** blind input — and my own loose end. #2861 fixed the two counts beside it, so the panel went from uniformly wrong to **partially** wrong: entries and bounces populated, duration reporting `no-in-review-entries` forever. Partial blindness is harder to notice than total, which is why finishing it matters more than one site suggests. ```sql metadata->>'to' = 'in-review' OR (metadata->>'from' = 'in-review' AND metadata->>'to' = 'done') ``` ## The class, not just the site **This shape is invisible to every check we have.** The lifecycle census scans `===`/`!==` comparisons; the unwired-lane-parameter guard scans declarations. Neither sees a lane id inside a `sql` template, so this class is **not in the backlog total at all** — the number is a floor for this reason as well as the usual one. `scripts/check-sql-column-literals.mjs` (#2841, in flight) is the detector for exactly this: it freezes the surface at 30 sites rather than converting any, so this one was unowned. That PR and this one are complementary — it stops the surface growing, this shrinks it by one. ## The fix Lanes resolve **once per call** via `resolveProjectColumnsForRoles` and arrive as parameterised equality fragments, one branch per id — no interpolated list, no string building. Resolution lives in `getInReviewDurationEventsImpl` because that is where the store is; `async-audit.ts` takes a bare `db` handle and cannot resolve anything. Best-effort, defaulting to the legacy pair, so a caller that cannot resolve keeps exactly today's query. **The union is correct rather than a widening hack**, for the same reason as #2861: these are *move records*, and a past move recorded the column name as it was at the time. A board renamed last month has rows under both ids, so the honest query covers both — which is precisely what `resolveProjectColumnsForRoles` returns. ## Tested against real PostgreSQL, deliberately This is a **SQL predicate** change. A mocked store would assert the arguments and prove nothing about the query that actually runs — which is the entire risk when the literal lives inside `sql`. The new case inserts real `activity_log` rows on a renamed board and reads them back through the real store method. The legacy-lane case in the same file stays green, which is the compatibility half. **Revert proof, measured:** restore the hardcoded fragments and the new case fails with ``` expected [] to deeply equal [ 'renamed-entered', 'renamed-done' ] ``` ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/core`) — clean - `activity-log-parity.pg.test.ts` — 5 passed against real PostgreSQL With this, all three Reliability inputs read the board's own lanes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Reliability duration metrics now work correctly with renamed workflow lanes. * Completion tracking recognizes configured completion lanes instead of relying on fixed defaults. * Improved handling of transitions between multiple review lanes and review-to-work-in-progress movements. * Legacy lane behavior remains supported when configured lane information is unavailable. * **Tests** * Added coverage for renamed lanes, historical lane IDs, and transition edge cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Instruments a surface no existing check can see. Follows #2839, and corrects the count I reported there (12 → 14).
Why it was invisible
The lifecycle census parses TypeScript comparisons; a legacy id inside a SQL string is string data. The inert-seam gate reasons about parameters and call sites. Neither has ever looked here.
What it cost:
cleanupStaleMergeQueueRowsImplfiltered ont.column != 'in-review', so on a renamed board every queued card looked stale, itsmerge_queuerow was deleted, and the card became unleaseable. The operator found it reviewing #2819 — in SQL I had already read past during that same work.The quieter half is analytics: five sites count
"column" = 'done', so throughput, cycle time, and team dashboards report zero completed work on a renamed board. Nothing errors, which is why nobody files it.What this does, and does not do
It does not fix the sites.
resolveProjectColumnsForRolesis the mechanism and its migration has an owner (#2839). This freezes the population so the surface cannot grow underneath that migration: a new file or a higher count fails, and a lower count fails too — so the baseline ratchets down as sites migrate rather than leaving slots to silently regrow into. That is the same rot as an allow-list entry for a deleted function, which this repo already hit once.AST-based, deliberately: a line grep for the same pattern reports 37 hits, 25 of them prose quoting
column === "done"in explanatory notes. A guard that is 68% false positives trains its readers to skip it — a lesson this program has already paid for.Two corrections found by mutation-testing my own gate
1. Clause fragments were missed. Requiring a SQL keyword in the same literal skipped
team-analytics.ts, which builds["assignedAgentId IS NOT NULL","column" = 'done', ...]and joins them into aWHERElater. That fragment is as vocabulary-bound as any full query but contains no keyword. Fixing it took the population 12 → 14, so the number I put on #2839 was low.2. My first mutation test proved a direction it had not. I replaced the first textual occurrence in a file — which was inside a comment — and read the unchanged count as the scanner being broken. The scanner was right; my test was wrong. All three directions are now driven against real SQL:
3 SQL column literal(s), baseline allows 21 site(s) now, baseline still allows 2 — re-record itI am flagging that second one because it is the exact failure mode this program keeps finding: a green result read as evidence when the experiment was invalid.
Verification
pnpm test:gategreen with the new check in it · lint 0 · single AST pass. Wired intotest:gateand bothpretesthooks.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests