docs(solutions): probe the instrument the way CI runs it - #3255
Merged
Conversation
Two instrument-level defects found this session, each visually identical to a pass: 1. A ratchet invoked without its failure flag. `check-move-target-literals` is report-only unless given `--strict`, which package.json supplies. Probed bare, it returned exit 0 for EVERY probe including an obvious `moveTask(id, "in-review")` in scheduler.ts — indistinguishable from a dead guard, and nearly reported as one. A report-only run prints its normal summary and exits 0, so the terminal looks the same as a real pass. 2. A ratchet that cannot see an uncommitted file. Two tools discovered files via `git ls-files` (tracked only) while the rest walk the filesystem, so a new file was invisible until `git add` — blind at precisely the moment the number is consulted. The compounding cost is the real lesson: because the two tools disagreed about WHICH FILES EXIST, the same probe was caught by one and missed by the other, and that differential was investigated as a claim about expression walking before the cause turned out to be discovery scope. Docs only — no changeset (AGENTS.md excludes internal docs).
Contributor
|
Warning Review limit reached
Next review available in: 2 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 |
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…p cannot fool (#3257) ## What Adds one technique to #3255. Docs only. #3255 records that probing a ratchet **by exit code** can read green because the tool is report-only without `--strict` — a real trap that nearly got a healthy gate reported as dead. There is a second technique that sidesteps it entirely and is strictly more informative: **parse the tool's own per-file count.** ```bash node scripts/check-move-target-literals.mjs 2>&1 | grep -a "my-probe-tmp" \ | grep -aoE "^ +[0-9]+" | tr -d ' ' ``` **Immune to the report-only trap** — a report-only run still *prints* the count, so the number moves 0 → 1 whether or not `--strict` was passed. **It measures which shapes, not just whether something fired.** An exit code is one bit for the whole run. Auditing a detector means asking *"of these five spellings, which are seen?"*, and five separate binary runs cannot distinguish **partial** detection from a probe file that failed to compile. The move-target audit read `direct 1 / backtick 1 / ternary 0 / const 0` in a single run, which named the gap immediately. ## Both belong | question | technique | |---|---| | **can this ratchet fail at all?** | `pnpm check:*` — ask this first (#3255 §1) | | **what can it see?** | per-file counts — an exit code is too coarse | I also added a caveat that applies to both: confirm the probe is actually being scanned by watching the tool's **scanned-file total** move. A probe that never compiled and a probe the tool never discovered both report zero hits, and neither is a finding — that one cost me a wasted measurement before I noticed the total had stayed at 1961. ## Why this is worth a follow-up rather than a comment #3255's rule as written — *"use `pnpm check:*`, not a bare `node scripts/...`"* — would have made the shape-coverage audits impossible, since `--strict` collapses five distinct per-form answers into one bit. The rule is right for its question and wrong for the other one, and the distinction is easy to lose once only the rule survives in someone's memory. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added guidance for evaluating ratchets using per-file output counts. * Documented report-only and shape-coverage limitations, count-based versus failure-based checks, and verifying that probe files were scanned. * Included a command example for probing ratchet behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Aug 1, 2026
… question (#3273) Extends the doc merged in #3255 with two more instances of the same pattern, both found this session, **neither involving a ratchet**. Four instances now, from four unrelated directions: | what was read as "pass" | what the green actually meant | | --- | --- | | `node scripts/check-*.mjs` exits 0 | report-only mode — the failure path needs `--strict` | | a census reports 0 for a new file | the file is untracked, so it was never scanned | | a backgrounded `cmd > log; grep …` reports exit 0 | that is `grep`'s status; the suite inside had 8 failures | | a rebased branch's tests pass | the rebase never started, so it ran on the **old** base | The two new ones are worth writing down because they are not about tooling anyone built here — they are about how results are read. **Exit codes belong to the last command in the pipeline.** A backgrounded `run_tests > log 2>&1; echo done; grep X log` exits with `grep`'s status, so the harness reported "completed, exit code 0" for a dashboard suite that had 8 failures. I nearly recorded that suite as green. Read the summary out of the log; never infer a suite's result from a wrapper's exit code. **A failed rebase leaves you on the old base, and the tests still pass there.** `git rebase` refused with `cannot rebase: You have unstaged changes`, so the branch never moved. `git diff origin/main` then listed 20+ files including other workers' commits — which reads exactly like my branch had reverted their work — and a full test run on that tree came back green. Both signals were true about a tree nobody cared about. ``` git merge-base --is-ancestor origin/main HEAD ``` said STALE while the tests said pass. That is the only check that separates the two, and it belongs before any claim of "verified on current main". The shared tell, stated once: **a result too clean, or too alarming, for what changed.** Every probe shape passing including ones that obviously should not; a two-file branch appearing to revert twenty. When the answer does not fit the size of the question, find out what was actually measured before believing it. ## Verification Docs only; no code paths change. `lifecycle-columns`, `move-target-literals`, `inert-sync-lanes`, `quarantine-ledger` all exit 0. No changeset — AGENTS.md excludes internal docs. **Pre-existing red, not from this branch:** `check:fnxc-future-dates` currently fails on main from a `2026-08-01-00:50` stamp in `packages/core/src/task-store/lifecycle-ops.ts` (commit `e52da740a5`) — a timezone-ahead clock writing tomorrow's date, at 23:45 UTC. Already claimed by **#3269 and #3270**, so I have not touched it; flagging only so this branch's CI result is not misattributed. It is the same recurring class this doc's sibling rule addresses: take the stamp from `date -u`, not the local clock. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added guidance for identifying misleadingly successful CI and test results. * Documented checks for report-only runs, untracked files, masked failures, and tests running on an outdated code base. * Included recommendations for reviewing logs and verifying branch ancestry. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Aug 1, 2026
…you report (#3291) Extends the doc from #3255/#3273 with the failure that cost the most in a single session: **one stale install produced five wrong reports on one issue** (#3264). ## What happened A `node_modules` that had drifted from the lockfile — `jsdom@29.0.1` installed, `29.1.1` pinned — generated failures that existed on no CI machine and no other checkout. They were not subtle: deterministic, reproducible on demand, with plausible stack traces and real-looking assertion diffs. Each round of triage got **more precise about the wrong data**: | round | claim | why it was wrong | | --- | --- | --- | | 1 | "4 deterministic failures" | measured in a 4-file batch, called it isolation | | 2 | "3 deterministic, 2 order-dependent" | isolated correctly, but a race is not deterministic | | 3 | "TaskCard is broken" | stale jsdom; the CSS assertion was correct | | 4 | "no contamination" | true of four app files; published unqualified | | 5 | "quarantine these two" | never read the failure text — both were timeouts | The through-line is not carelessness about the code. **The environment was never treated as part of the claim**, so no amount of care about the analysis could recover it. ## The checks, in the order they cost the most ```bash pnpm install --frozen-lockfile # node_modules is not evidence until it matches the lockfile <run the file ALONE, 3+ times> # isolation and repetition answer different questions <read the failure TEXT> # a timeout and an assertion failure need opposite responses uptime # a loaded box manufactures timeouts that mean nothing ``` ## Why the load check earned its place Two tests "failing" in a full-suite run were `Test timed out in 15000ms` on a box at **load average 9.7 with 84 users**. Under AGENTS.md's quarantine-on-sight rule that reads as a flake to quarantine — and the ledger's **14-day deletion ratchet would have made the lost coverage permanent**. The rule presumes the failure is a property of the test, not of the machine. A wall-clock budget crossed under local contention is evidence about the hardware. I was one comment away from deleting healthy coverage on that basis. ## The tell A finding is environment-derived when it is **local, recent, and unshared**: nobody else has reported it, CI is green, and it appeared without a commit that could explain it. Any two of those should stop a report before it is written. All three applied here, and the report went out anyway — five times. ## Verification Docs only; no code paths change. `fnxc-future-dates`, `lifecycle-columns`, `quarantine-ledger` exit 0. No changeset — internal docs are excluded. Context: the one finding in #3264 that survived all five rounds is #3286 (merged), and it survived because it was verified by **reverting the product change** rather than by trusting a red — 3/3/2 failures without the fix, 27/27 across four runs with it.
gsxdsm
added a commit
that referenced
this pull request
Aug 1, 2026
…ed three defects (#3293) ## What My blind-spot table in #3251 audited **one axis**. Adds the one that missed three defects. Docs only. That table records what each of the five lifecycle ratchets can and cannot **see**. I probed that carefully — several spellings per tool — and then wrote *"nothing found; sound"* for two of them. Within a day, three of those same tools turned out to share a completely different defect: **they wrote to the tree they were checking**, auto-tightening their own baseline during a plain check run. | gate | wrote during a check | fixed by | |---|---|---| | `check-fnxc-future-dates` | yes | #3287 | | `lifecycle-column-census` | yes, under `--strict` | #3289 | | `check-sql-column-literals` | yes | #3292 | **No number of detection probes could have surfaced that.** The table asserted one property carefully and said nothing about the other *while reading as comprehensive* — which is precisely the failure it documents in the tools it audits. ## The rule it adds 1. **What can it see?** — probe each spelling of the thing it claims to catch. 2. **Can it fail at all?** — invoke it as `package.json` does; a report-only run exits 0 forever (#3255). 3. **Does it write?** — `git status --porcelain` before and after, on a clean tree. With the trap on the third spelled out: these gates write only when a tightening is **available**, so a clean tree after a run proves the *trigger* is absent, not that the tool is read-only. Inflate a baseline entry first, then run it. I hit exactly this while reviewing #3292 — ran all three gates on main, saw a clean tree, and had to stop myself concluding the SQL gate was fine. ## Why the pattern, not the people Three tools converged on write-during-check independently. That argues the design is **attractive**, not that three authors were careless: the tightening is correct, the write saves a step, and the message even tells you to commit it. It only becomes a defect at the moment a second person runs the same gate — which is invisible from inside any one of them. What it cost, measured: #3283 and #3285 are the same `+0/-1`, five minutes apart, by two authors, **neither of whom wrote that line**. ``` lint clean; fnxc-future-dates clean ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-literalsis report-only unless given--strict, whichpackage.jsonsupplies. Probed bare, it returned exit 0 for every probe — including a blatantmoveTask(id, "in-review")pasted intoscheduler.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-censusandcheck-move-target-literalsdiscovered files withgit ls-files— tracked only — while the other five walk the filesystem.git addedThe 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-conversionswalks 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
check-move-target-literalsdiscovery scope: reported to fix(gate): catch cast move-targets, keep??fallbacks unflagged, and pin all of it #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.