fix: main is red on the lifecycle ratchet — re-record the census baseline - #2811
Conversation
|
Warning Review limit reached
Next review available in: 23 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 lifecycle census now classifies deliberate-literal increases as reclassifications when guard counts do not rise, reports them differently in strict mode, updates baseline mappings, and adds tests for reclassification, genuine guard increases, and unchanged baselines. ChangesLifecycle census ratchet
Estimated code review effort: 3 (Moderate) | ~20 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 |
Greptile SummaryThe PR refines lifecycle-census diagnostics and adds regression coverage.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/lifecycle-column-census.mjs | Classifies deliberate-count changes and selects more specific strict-mode diagnostics. |
| packages/engine/src/tests/census-reclassification-message.test.ts | Adds subprocess coverage for unchanged baselines, deliberate reclassification, and genuine guard growth. |
Reviews (4): Last reviewed commit: "fix(census): "did NOT increase", not "we..." | Re-trigger Greptile
Added the follow-up I flagged: the failure now says what actually happenedSecond commit on this PR. The first unblocks The message was actively misleading. Now a marker-only diff reports: This does not loosen the ratchet. The baseline still must be re-recorded and the run still exits 1. Whether a marker-only change should fail at all is a policy question for the ratchet's owner — I raised both options on the first commit and am not deciding it here. Making an existing failure legible is not a policy change. Two bugs in my own detection, caught by its test before it shippedBoth would have made the guard silently never fire — the exact failure mode this program exists to find, in the code meant to find it:
The honest condition is weaker and correct: markers rose and guards did not. It cannot mask real regrowth, because a file whose guard count also rose is still reported as a rise — asserted by a case that keeps the 3 tests: the reclassification message, the genuine-rise message, and a control proving the whole thing still passes against the unmodified baseline. The control matters here because the suite shells out to the real script; without it every assertion could pass on a script that always fails. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/lifecycle-column-census.mjs (1)
352-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect and shorten the duplicated FNXC records.
Both comments are dated 2026-07-31, which is in the future relative to July 30, 2026, and duplicate lengthy narrative. Retain a concise invariant/rationale with a current timestamp.
scripts/lifecycle-column-census.mjs#L352-L362: replace the future-dated multi-paragraph FNXC block.packages/engine/src/__tests__/census-reclassification-message.test.ts#L1-L23: keep the test-focused FNXC record concise and current.As per coding guidelines, “Add concise FNXC comments … with a timestamp in
yyyy-MM-dd-hh:mm”; retrieved learnings also require current, accurate timestamps.🤖 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/lifecycle-column-census.mjs` around lines 352 - 362, Replace the duplicated future-dated FNXC comments at scripts/lifecycle-column-census.mjs lines 352-362 and packages/engine/src/__tests__/census-reclassification-message.test.ts lines 1-23 with concise, accurate invariant/rationale comments using the current timestamp in yyyy-MM-dd-hh:mm format; preserve the explanation relevant to the census reclassification message and test behavior at each site.Sources: Coding guidelines, Learnings
🤖 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/lifecycle-column-census.mjs`:
- Around line 506-525: Update scripts/lifecycle-column-census.mjs lines 506-525
so the reclassification tag and remediation message describe guard counts that
did not increase, including equality, rather than claiming debt went down or
calling it not new debt; retain the baseline update command. Update
packages/engine/src/__tests__/census-reclassification-message.test.ts lines
62-79 to assert the accurate equality-case wording.
---
Nitpick comments:
In `@scripts/lifecycle-column-census.mjs`:
- Around line 352-362: Replace the duplicated future-dated FNXC comments at
scripts/lifecycle-column-census.mjs lines 352-362 and
packages/engine/src/__tests__/census-reclassification-message.test.ts lines 1-23
with concise, accurate invariant/rationale comments using the current timestamp
in yyyy-MM-dd-hh:mm format; preserve the explanation relevant to the census
reclassification message and test behavior at each site.
🪄 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: b253a1f6-3041-4b28-8894-ebb386f1751e
📒 Files selected for processing (3)
packages/engine/src/__tests__/census-reclassification-message.test.tsscripts/lib/lifecycle-column-census-baseline.jsonscripts/lifecycle-column-census.mjs
…is the common one #2811 review. `reclassified` is `guardsNow <= guardsBefore`, so it is TRUE when the guard count is UNCHANGED — which is exactly what adding a DELIBERATE-LITERAL marker to a site the parser already excluded produces, and exactly what the fixture in this PR's own test creates. Claiming a decrease there puts a second wrong number in a message whose entire purpose is to stop the reader chasing one. The test's comment was wrong in the same way — it said the fixture makes "the same file's guard count fall", when deleting only deliberate entries leaves it unchanged. Corrected, and the test now also asserts the decrease claim is absent. Mutation-verified: restoring "went DOWN" fails the case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Real, and the equality case is the common one — fixed in
Now reads "Unconverted debt did NOT increase", which is accurate for both The test's comment was wrong in the same way, which is why the assertion looked right: it said the fixture makes "the same file's guard count fall". It does not — deleting deliberate entries leaves the count untouched. Corrected, and the test now also asserts the decrease claim is absent, so the wording cannot drift back. Mutation-verified: restoring "went DOWN" fails it. A correction I owe this PR. I initially measured On a clean checkout of this branch: I have also closed #2813, which I had opened for the same red main before finding this one. Two PRs editing the same ratchet file would conflict and risk one silently reverting the other's entries — precisely the failure a ratchet exists to prevent. #2811 is the better change: it fixes the misleading message and moves One thing from #2813 worth carrying regardless: |
Second query-class conversion, and it needed TWO fixes — the second only visible once the first landed
The finding: fixing the query moved the failure one frame deeperWith the query widened, the case still reported zero. Converting only the query would have looked like a fix and changed nothing — The two halves need different shapes, which is the reusable point:
Using the project union for the per-card question would have marked a card stale because another workflow calls its column wip. That is the flat-set mistake this program has made four times, and the helper's own header warns against exactly this. A test premise of mine that was simply wrongI first asserted that a card in What the union actually buys is that the row is fetched; whether it is stale is then a per-task question. The case now covers the guarantee that genuinely matters — a store with no workflow selection, where both halves fall back together. That is the fourth fixture/premise error I have made in this sweep ( Verification7 cases in the reporter suite, 8 on the shared helper. Revert proof: restoring either literal read makes the renamed case surface zero. Still not done
|
… count ROSE' Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…is the common one #2811 review. `reclassified` is `guardsNow <= guardsBefore`, so it is TRUE when the guard count is UNCHANGED — which is exactly what adding a DELIBERATE-LITERAL marker to a site the parser already excluded produces, and exactly what the fixture in this PR's own test creates. Claiming a decrease there puts a second wrong number in a message whose entire purpose is to stop the reader chasing one. The test's comment was wrong in the same way — it said the fixture makes "the same file's guard count fall", when deleting only deliberate entries leaves it unchanged. Corrected, and the test now also asserts the decrease claim is absent. Mutation-verified: restoring "went DOWN" fails the case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3c41669 to
6fe1f6c
Compare
Rebased —
|
Third query-class conversion: restart recovery did not run at all on a renamed board
Three layers, and naming them is the pointThe previous two conversions in this class each hid a second layer behind the first, so I looked for all of them before touching anything:
Layer 3 is worth calling out separately: a stale warning comment is its own hazard. It told the next reader a defect existed where none did, and I nearly re-fixed it before checking the call. A mistake I made building thisI pulled Same root cause as the missing-test-file slip earlier: taking a file from another branch takes its whole contents, including whatever is now out of date. For a barrel file that is nearly always wrong; the fix is to re-apply the single edit on top of the current version. Verification12 cases in the suite (2 new), revert-proven: restoring the literal query requeues nothing on the renamed board. Remaining in this class
|
…ert (#2825) ## Red on main ``` lifecycle-column-census > the baseline can always be re-recorded > writes the baseline BEFORE the rise check can exit AssertionError: expected 19345 to be greater than 26374 ``` Read literally, that says the CLI now runs its rise check *before* the `--update-baseline` write — which would break the one command whose entire job is re-recording, and would be a genuine bug worth stopping for. **It does not.** The order in code is correct and unchanged: | | line | |---|---| | `if (updateBaseline) { … writeBaseline() … process.exit(0)` | `scripts/lifecycle-column-census.mjs:487` | | `"column-guard count ROSE"` + `process.exit(1)` | `:510` | ## What actually moved was a comment Line **359** explains this exact failure mode and quotes the marker verbatim: > …`"column-guard count ROSE"`, which is the opposite of what happened and sends the reader looking for… So `cli.indexOf("column-guard count ROSE")` found the **prose**, 7000 characters before the branch it was meant to locate. A guard that a comment can invert is not measuring control flow. And the honest-looking fix — reword the comment — silently re-arms the same trap for whoever explains this next. `cliSource()` now strips comments before indexing. The same defence is already used by `archived-column-gate-parity.test.ts`, for the same reason: notes documenting *why* a literal is dangerous have to mention the literal. ## Kept, not deleted The end-to-end block below these does cover the contract — it drives the real CLI and asserts exit code, baseline content and printed output, and its own comment names the ordering bug. It would have been defensible to delete the two source-text cases as redundant. I kept them because two guards at different levels is the point: **e2e proves the behaviour, these locate the branch that provides it.** They only needed to stop being defeated by prose. ## Evidence The real ordering bug — make a rise exit before the update branch writes — fires **all three**: | guard | failure | |---|---| | source order | `expected 9454 to be greater than 9505` | | slice / uniqueness | `expected 10433 to be -1` | | end-to-end | `expected 1 to be +0` (exit code) | **My first mutation attempt was invalid** and I nearly reported it as evidence: it moved the block by line range, mangled the file, and both markers disappeared — the resulting `-1`s look like a firing guard but prove nothing. A mutation that corrupts its target is not evidence that a guard works. Engine **10991 passed / 0 failed** · gate **732 green** · lint clean. Test-only; the CLI is restored clean. ## Note on duplicated effort #2811 and my #2814 both re-recorded the census baseline for #2783's rise, concurrently. No harm done — but this file is now a fleet-wide contention point, and the per-file baseline shape exists precisely to avoid that. Worth one owner for census/ratchet fixes rather than whoever notices first. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#2818) Three sweeps that **never ran at all** on a renamed board, plus the shared answer the rest of the class needs. Consolidated from three handoff branches so the helper appears once. #2811 merged, so this is my only open PR. `#2800` measured this class and shipped evidence deliberately without conversions: `listTasks({ column: "<literal>" })` filters in the store, so on a renamed board the read returns an **empty array** and the sweep it feeds does nothing. The census scores the comparison *inside* the loop, never the query above it. ## What was broken | file | census count | what actually happened on a renamed board | |---|---|---| | `backlog-pressure-reporter.ts` | **0** | both reads empty, ratio computed as 0/0 — **the alert never fired**, on a board that may be under exactly the pressure it reports | | `stale-task-reporter.ts` | **0** | both reads empty — **no stale-task signal ever raised**, where work is most likely sitting unnoticed | | `restart-recovery-coordinator.ts` | flagged | sweep never ran — **an engine restart left interrupted tasks stuck with no requeue** | Two of the three have a census count of **zero**. They contain no lifecycle comparison at all, so they have never appeared in the backlog, in a per-file list, or in any "N → 0" claim — and were completely inert. **A file at zero is not evidence of anything.** ## The shared answer, and what it is not Every existing resolver answers a **per-task** question. A query has no task in hand, so it needs the project-level one: every column any workflow declares for a role, unioned with the legacy ids so a board mid-rename still finds rows under the old ones. The set is never empty, so a caller cannot accidentally query nothing. The header states what it is **not**: answering a per-card question from the union would mark a card as review because some *other* workflow calls its column review — the flat-set mistake this program has made four times. ## The finding that generalises: the query is rarely the whole defect `stale-task-reporter` **still reported zero after the query was fixed** — `getTaskAgeStalenessSignal` defaults to the legacy pair, so a card the query now returned was refused inside the signal. Converting only the query would have looked like a fix and changed nothing. That is a caveat on #2800's approach, offered as refinement rather than correction: **asserting the query ARGUMENT is right when pinning a known defect** (the outcome is 0 either way) **and insufficient when proving a fix**, because the outcome is the only thing that distinguishes a real conversion from a deeper one. All three conversions here assert outcomes. `restart-recovery` had three layers — query, a redundant re-assertion (deleted; a test pins the `paused` guard it did contribute), and a move destination that was **already** resolved but whose warning comment was stale. A stale warning is its own hazard: it told the next reader a defect existed where none did. ## Verification - helper **8 passed** · three reporter/coordinator suites **29 passed** - `pnpm test:gate` **161 / 13 / 487 / 71** · lint clean · `--strict` exits 0 · four `tsc` targets clean - each conversion revert-proven independently; the failing case is named in each test header ## Two mistakes worth recording **The helper's own test caught a bug in it.** My first draft wrapped the definition loop in one `try`, and `parseWorkflowIr` **validates** rather than parses — one malformed row would have returned legacy-only lanes for *every* workflow, indistinguishable from the bug it exists to fix. Now isolated per definition. **I clobbered the core barrel** by taking `index.ts` wholesale from a handoff branch, dropping two exports `main` had added since; three packages stopped compiling. Taking a file from another branch takes its whole contents, including what is now stale — for a barrel that is nearly always wrong. Re-applied as a single edit on top of `main`. ## Not included `self-healing.ts`'s 49 — actively owned and mid-conversion; an outside refactor there produces conflicting halves of one sweep. `project-engine.ts` (7) and `executor.ts` (2) need their own read of what each sweep does with the rows, which these three are the argument for. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…conversion (#2856) ## This is the cause of four main reds today, not a fifth instance of them I have now fixed the census baseline on `main` three times (#2814, plus a withdrawn branch, plus watching #2811 and #2844 do the same). Rather than do it a fourth time, here is why it keeps happening. `census-baseline-corruption-guard` asserted: ```ts expect(result).toContain("every file matches its baseline exactly"); ``` That demands the **committed baseline be byte-in-step with the tree at all times**. **It is not, by design.** A conversion PR that removes guards leaves the tree holding *fewer* than the baseline allows, and the CLI treats that as the good case — it tightens the pin and exits 0. Measured directly: ``` simulated drop → EXIT ON DROP: 0 "The baseline file has been rewritten downward. COMMIT IT … in CI this write is discarded with the runner, which is why the gate is green and not silent." ``` So the ratchet was already happy while this test went red. Every conversion that did not *also* re-record the baseline turned `main` red for a condition that was never a defect. That is the mechanism behind **#2783's markers, #2837's query split and two more** — plus three collisions between workers racing to re-record the same file (#2811/#2814, #2844, and a branch of mine I deleted rather than open as a duplicate). ## The fix matches the guard's own stated intent Its comment says: *"a guard that always fails is no guard"* — its job is to prove the **corruption** diagnosis does not false-positive on a healthy file. **A tightened baseline is healthy.** So it now asserts what that needs: - the run **succeeds** — `execFileSync` throws on a non-zero exit, so a **rise still fails before any assertion runs**; a rise is real debt and must stay loud - the corruption diagnosis is **absent** - the outcome is one of the two healthy shapes the CLI can report ## Measured discrimination — all four cases | scenario | before | after | |---|---|---| | **drop** (legitimate conversion) | ❌ 1 failed — *the false red* | ✅ 3 passed | | **rise** (real debt) | ❌ fails | ❌ **still fails** | | **corrupt JSON** | ❌ fails | ❌ still fails (case unchanged) | | **unreadable file** | ❌ fails | ❌ still fails (case unchanged) | Only *"somebody converted guards and has not re-recorded the pin yet"* stops being a red. ## Scope Engine **11022 passed / 0 failed** · gate **732 green** · lint clean. Test-only. **Does not change** the CLI, the ratchet, or what `--strict` reports. Re-recording the baseline on a drop is still the right thing to do — it just stops being an emergency that reddens main and blocks everyone else while three people race to fix it. The baseline file is restored byte-clean after every simulation above (`git diff` verified). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Expanded baseline validation coverage to detect unreadable or invalid baseline data. - Added support for both exact baseline matches and successfully tightened baselines. - Improved health checks for baseline verification outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
mainis RED on the lifecycle ratchet right now.node scripts/lifecycle-column-census.mjs --strictexits 1 on pristineorigin/main, which is theLintjob's Lifecycle-column ratchet step — so every open PR fails Lint until this lands, regardless of its own contents.Verified on a detached checkout of
origin/main, not on a branch of mine.Cause
Eight
DELIBERATE-LITERALmarkers were added across seven files without re-recording the baseline:Adding a marker RECLASSIFIES a site (column-guard → deliberate), so the tracked deliberate totals move and
--strictfails until the baseline records the new shape. It is the same mechanism that turned #2775 red earlier today — a marker landing without its baseline — which is worth noting because it has now happened twice from different PRs.The fix
Baseline re-recorded, nothing else. Zero source changes; the diff is one derived file.
--strictexits 0pnpm test:gate— 161 / 13 / 487 / 71pnpm lintcleanWorth a follow-up by whoever owns the ratchet
The failure is structural rather than careless: a PR that adds a marker is doing the right thing, and the baseline requirement is only discovered when CI goes red — after merge, for everyone else. Two options, neither of which I am taking unilaterally on a red-main fix:
--stricttreat a marker-only reclassification as an accepted rise (it is not new debt — the count of unconverted guards goes down);I would take (1): a marker is the documented way to close a site, and requiring a second mechanical step to record it is a trap that catches good behaviour.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests