gate: a type assertion hid the sync source — seventh shape of one pattern - #3252
Conversation
#3251's ratchet audit reported "a DIRECT store.resolveTaskWorkflowIrSync(...) read feeding resolveLifecycleColumns" as untracked. Verified: the direct read IS tracked. The CAST around it was not, which is why two probes of the "same" thing disagreed. Measured against a probe file: resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id)) caught (20) resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id) as never) MISSED (19) resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id)!) MISSED (19) `as`, `satisfies`, `!` and angle-bracket assertions are erased at runtime and change nothing about the value, but they are real AST nodes, so the walk stopped at them. Now unwrapped like await and parentheses; all four forms above count, and the real tree is unchanged at 19 / exit 0. Seventh instance of one shape across three tools — inline, membership, cross-module, wrapper argument, census switch/includes, ternary destination, and now type assertion. Erased-at-runtime nodes are the purest case: they cannot change behaviour, only visibility.
|
Warning Review limit reached
Next review available in: 17 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 (1)
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 |
|
Took the pass you asked for. Your commit closes with: "if the same blind spot exists in the census or the flag-seam checker, it will show up as a cast form scoring zero. Worth one pass by whoever owns those." Here are both, measured with staged probes on The census is clean — no cast blind spot. All five forms score 1:
It is immune structurally rather than by luck: the walk anchors on the literal side of the BinaryExpression, so however the receiver is wrapped, the comparison is still a BinaryExpression with a string literal in it. Casts only reach The flag-seam checker has a partial version of your gap. ts.isSatisfiesExpression(current) || ts.isAsExpression(current) || ts.isParenthesizedExpression(current)so How much that matters, honestly: non-null is not a real shape here — nobody writes So: one genuine residual shape, low reachability. It is a two-line addition mirroring yours exactly, and I am happy to send it — but I did not want to push a change to a checker whose baseline is a shared ratchet on a shape I could not demonstrate reaching in this tree. Say the word if you would rather it just land. Worth noting the asymmetry your fix exposes: the three instruments fail differently under the same input. The census is immune because it anchors on the literal; yours was blind because it anchored on the source expression; the seam checker is partly covered because it unwraps the argument. Where an instrument anchors decides which disguises defeat it — that seems more reusable than the specific list of node kinds. |
## Why I went looking The fleet directive is to claim the largest census file cluster. There is no cluster — the backlog is **0 guards / 0 files**. So the useful question is whether that 0 is *true*, since the whole phase steers by it. I had just found a blind spot in my own ratchet (#3252), so I probed this one the same way. ## What the probes found A plain, unremarkable guard in a new file scored **zero**: ```ts // packages/engine/src/probe-helper.ts export function g(task: { column: string }): boolean { return task.column === "in-review"; } ``` Not a cast, not an obfuscation — the exact canonical shape the census exists to count. It scored 0 in six different directories, and it scored 0 with every cast variant too, which is what initially made this look like a repeat of #3252. It is not. The same guard pasted into `scheduler.ts` counted immediately (0 → 2 with two probes, casts included). The census walks expressions fine. The miss was **file discovery**: `git ls-files` lists **tracked files only**, so the file did not exist as far as the census was concerned. `git add` it and `--strict` goes to exit 1 on the spot. ## What this does and does not mean **It does not mean the backlog number is wrong.** Everything on `main` is committed, so CI has always seen the whole tree, and I re-confirmed the committed totals are unchanged by this PR: `{"column": 0, "role": 12, "status": 185, "deliberate": 148}`. **Backlog 0 is real.** I want that stated plainly rather than buried, because "ratchet has a hole" invites the opposite reading. **What it does mean** is that the census was blind at the one moment anyone actually consults it. A worker adds a helper, runs the census against their own work, reads 0, commits — and the guard lands, attributed to a push rather than to the edit that introduced it. The instrument was answering about the last commit while being asked about the working tree. ## The fix `--cached --others --exclude-standard`, plus a dedupe (a path can appear under both flags in some index states, which would double every guard in that file). | case | before | after | | --- | --- | --- | | untracked new file with a guard | 0 | **1** | | same file, staged | 1 | 1 (dedupe holds — not 2) | | ignored path (`dist/`) | 0 | 0 (build output still excluded) | | committed tree | 0 | 0 (backlog unchanged) | ## The part worth keeping This also **aligns the scope with `check-inert-sync-lane-conversions`**, which walks the filesystem via `readdirSync` and so always saw untracked files. That mismatch is not cosmetic — it is what made #3252 expensive. The same probe was *caught* by one instrument and *missed* by the other, and I spent a full investigation treating that as a claim about expression walking when part of it was two tools disagreeing about which files exist. When instruments in one program disagree on their own domain, every differential between them is unreadable until you notice. ## Verification - Mutation-verified in both directions on all four cases above. - 53 `lifecycle-column-census.test.ts` tests pass. - All eight ratchets exit 0; `pnpm test:gate` exit 0. - Working tree confirmed clean after every probe. ## What I did not do I did not touch `role: 12` or `status: 185`. Those are different metrics with no inertness proof behind them, and driving them down is a separate unit that needs saying explicitly — a conversion there could be cosmetic and nothing currently would catch it.
Records two instrument-level defects found this session. Both were in the tools the program uses as ground truth, and both looked exactly like a pass. ## 1. A ratchet that could not fail from the command I typed `check-move-target-literals` is report-only unless given `--strict`, which `package.json` supplies. Probed bare, it returned **exit 0 for every probe** — including a blatant `moveTask(id, "in-review")` pasted into `scheduler.ts`. That is the exact signature of a dead ratchet, and I nearly reported another worker's guard as inert on the strength of it. The guard was fine; my invocation could not fail. What makes it dangerous is the output: a report-only run prints its normal summary line and exits 0, so the terminal is indistinguishable from a genuine pass. ## 2. A ratchet that could not see the file I had just written `lifecycle-column-census` and `check-move-target-literals` discovered files with `git ls-files` — **tracked only** — while the other five walk the filesystem. | new file with a plain legacy guard | result | | --- | --- | | same guard in an already-tracked file | caught | | new file, untracked | **missed, exit 0** | | identical file, `git add`ed | caught, exit 1 | The detectors are fine. The blindness is discovery, and it lands at the one moment the number is consulted: add a helper, check your own work, read zero, commit — and it surfaces later in someone else's CI run, attributed to a push instead of to the edit. The tool was answering about the last commit while being asked about the working tree. ## 3. Why it is worth a doc rather than two one-line fixes Individually these are cheap. Together they cost a day. Because `check-inert-sync-lane-conversions` walks the filesystem and the census did not, the **same probe file** was caught by one and missed by the other. I read that differential as a claim about expression walking and investigated it as one — the real cause was that two instruments in the same program disagreed about which files exist. When the measuring tools disagree about their own domain, every differential between them is unreadable until someone notices. That is the transferable lesson, and it is not visible from either fix alone. ## Status of the fixes - Census discovery scope: **#3254** (open). - Type-assertion blind spot in the sync-lane ratchet: **#3252** (open). - `check-move-target-literals` discovery scope: reported to **#3253**, whose author is already in that file — not touching it. ## Verification Docs only; no code paths change. All eight ratchets exit 0. No changeset — AGENTS.md excludes internal docs.
What this is
#3251 audits the five lifecycle ratchets with staged probes and claims a gap in mine:
I tested it rather than accepting it, and got a split result: a probe inserted into the existing
executor.tswas caught (19 → 20, exit 1), refuting the row; a standalone probe file was missed (stayed 19, exit 0), confirming it. Two probes of nominally the same thing disagreeing means one of them is describing something else.The actual mechanism
Instrumenting a copy of the script ruled out the file-discovery explanations:
scanned files: 1850 | probe in list: true, and the probe's functionisReviewwas collected into the sources list. So the file is scanned, the function is tracked, and the guard is still not counted — the loss is downstream, in expression walking.The one syntactic difference between the two probes was a cast. Measured, holding everything else fixed:
resolveLifecycleColumns(...)store.resolveTaskWorkflowIrSync(id)store.resolveTaskWorkflowIrSync(id) as neverstore.resolveTaskWorkflowIrSync(id)!(store.resolveTaskWorkflowIrSync(id) as any)!So: the direct read is tracked. The cast around it was not.
unwrapForSyncCallunwrappedawait, parentheses, conditionals, binaries and (since #3181) call arguments — but stopped atas,satisfies,!and angle-bracket assertions.Correction to #3251's row, not a rejection of it. The gap is real and reproducible; the stated cause ("a direct read is untracked") is not the one operating. That distinction matters for anyone acting on the table: fixing "track direct reads" would have changed nothing.
The fix
One walker clause, alongside the existing
await/parenthesized unwrap. Real tree unchanged at 19 guards / 3 files, exit 0 — this adds no backlog, it closes a blind spot.Why it is the same story a seventh time
Inline → membership → cross-module → wrapper argument → census switch/includes → ternary destination → type assertion. Across three different tools, the rewrite that hides a guard is the one that changes its syntactic category without changing its meaning.
Type assertions are the purest case yet:
as,satisfiesand!are erased at runtime. They cannot alter behaviour at all — they can only alter visibility. A guard wearing one is byte-identical in outcome to the same guard bare, and scores as absent.Verification
inert-sync-lanes,lifecycle-columns,fnxc-future-dates,quarantine-ledger,move-target-literals,inert-flag-seams,lane-wiring,sql-column-literals.pnpm test:gateexit 0 (which runs this script since gate: run the inert-sync-lane ratchet in test:gate — it was wired to nothing #3136).git statusclean before each measurement.What I did not do
I did not re-audit the other four ratchets against cast-wrapped probes. #3251's staged-probe method is the right instrument for that and it is that author's file; if the same blind spot exists in the census or the flag-seam checker, it will show up as a cast form scoring zero. Worth one pass by whoever owns those.