fix(scripts): the soft-delete reconciler wrote a literal archived into boards that do not have one - #2999
Conversation
…nto boards that do not have one `reconcileLeakedSoftDeletes` ran `UPDATE project."tasks" SET "column" = 'archived'` under `--apply`, against an operator's live database. 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 and made the write 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. The SQL pre-filter carried the same literal, so the query and the planner each imposed the legacy vocabulary and fixing one would leave the other silently deciding. 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. 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. 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 (`archived: "archived"`), so a legacy board repairs exactly as before. Needed no new plumbing — `openBackend` already returns both `core` and `store`, which corrects what I wrote on #2994 about this needing the `importCore` seam. Reverted, 2 of the 4 new cases fail. The other two 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. Verified: 17 passed across the three script suites; node --check and eslint clean; sql-literal, census, lane-wiring and fnxc ratchets green. No changeset — root `scripts/` is repo tooling, not part of the published package.
|
Verified — the selection fix is well pinned, and the write is the right shape. One coverage gap worth naming before merge. Mutations:
The second is not a defect in this PR — That is worth stating explicitly because of what the line does: const target = (await core.resolveTaskLifecycleColumns(store, row.id, irCache))?.archived;
if (!target) { summary.skipped.push({ … reason: "unresolved-archived-lane" }); continue; }
await tx.execute(sql`UPDATE project."tasks" SET "column" = ${target} …`);The most dangerous statement in the script is the one with no test. The refusal on Cheap close, matching how the selection was made testable: lift the target choice into a pure helper ( Not blocking: this is strictly better than what it replaces, and the selection fix alone stops the mass rewrite. Flagging because "the repair is the damage" is exactly the failure mode this PR fixes, and the guard preventing its return is currently held by nothing. Stamps on this branch are clean — the impossible-hour ones visible in the tree are inherited from |
…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.
#2999 removed both raw-SQL lane literals from reconcile-leaked-soft-deletes.mjs, so the population this branch recorded (28) is two higher than the tree. That reads as a DECREASE, which this gate fails on by design. Re-recorded to 26: audit-branch-cross-contamination.mjs keeps its 4, the leaked-soft- deletes entry is gone. This is the merge-order hand-off flagged on the PR, done rather than left for the operator.
|
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)
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 |
…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.
…laimed it had (#3005) ## An audit that scanned four legacy lanes — and claimed it had Two halves of the same wrong answer. **The query allowlisted the lanes:** ```sql WHERE deleted_at IS NULL AND "column" IN ('triage','todo','in-progress','in-review') ``` On a board whose lanes are named anything else that matches **nothing**, so the audit scans zero rows and reports zero contamination — a clean bill of health from a scan that never happened. `triage` is in that list too, a lane U11 (#2515) deleted. **And the report asserted the coverage it did not have:** ```js scannedColumns: ["triage", "todo", "in-progress", "in-review"], ``` printed regardless of what the query returned. When I first surveyed this script I called that field "the one thing keeping it from being fully silent" — it turns out it was a **claim, not an observation**, so it was not keeping it honest at all. It is now derived from the rows that came back. ## Fix: exclude finished lanes instead of allowlisting active ones Inverted so the default is the safe one — an unrecognised lane is active work by assumption and **is** audited; only lanes that genuinely mean finished drop out. An allowlist fails **closed** (skip everything unknown), a denylist fails **open** (look at it), and for an audit one extra finished branch is a far smaller error than auditing nothing. Filtered in JS rather than by building a dynamic SQL exclusion: it keeps **one** place deciding what "finished" means, and removes the last raw-SQL lane literal from this file. ## Revert proof ``` ✖ scannedColumns reports the board's real lanes, not a fixed legacy claim ✖ reports each scanned lane once, and nothing at all for an empty board ℹ pass 1 ℹ fail 2 ``` ## A demonstration of #3000, for free This PR removes a 4-literal raw-SQL clause, and `check-sql-column-literals` here reports **22, unchanged and green** — because this branch predates #3000 and the gate still walks `packages/` only. That is precisely the blind spot #3000 closes, reproduced a second time. ## Merge order This removes the 4 literals #3000 baselines. Landing this **after** #3000 drops that count and its gate fails on DECREASE — that gate auto-rewrites the baseline and asks for the commit, unlike `check-lane-wiring` which needs an explicit `--update-baseline`. Either order works; one of them needs a re-record, and I am happy to push it. ## Verification (measured) - `node --test` — **3 passed / 0 failed** (1 pre-existing + 2 new) - `node --check`, `eslint` — clean - `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. ## Territory status This was the last item I know of in `scripts/`. The four operator scripts holding lane assumptions — `recover-stale-blocked-by` (#2992), `reconcile-task-state-consistency` (#2994), `reconcile-leaked-soft-deletes` (#2999) and this one — are now either resolved or, where a script genuinely cannot resolve lanes, made loud rather than silent.
… on them `main` is red on `check-fnxc-future-dates`. The cause is four stamps reading `2026-07-30-26:10` — an hour that cannot exist. They are exactly what #2995 taught this gate to catch. That PR landed the hour validation (`00-23`) after #2999 had already merged these four, so the gate started reporting a defect that was sitting there rather than one anybody introduced afterwards. The guard is working; nothing had been checking before it. Corrected by literal normalisation — 26:10 on the 30th IS 02:10 on the 31st — which keeps the chronology the author was recording rather than flattening the stamps to an arbitrary in-range hour. AGENTS.md specifies `yyyy-MM-dd-hh:mm`, and the stamp's whole purpose is a readable why-does-this-exist trail, so the ordering is the part worth preserving. THE BASELINE TIGHTENING RIDES ALONG, and it is a date rollover rather than anything anyone did. Stamps written yesterday as `2026-07-31` were future then and were baselined as such; today they are past, so 176 files ratchet to zero. The gate rewrites the file as a side effect and exits 0, so leaving it uncommitted dirties the tree on every subsequent run for everyone — which is why it belongs in this commit rather than a later one. Re-recording on a decrease is the rule this gate and its siblings already state. Worth knowing about the design: this churn recurs whenever a day boundary passes with future-dated stamps in the baseline, and it shrinks only as people stop writing them — which is the behaviour the gate exists to produce. 93 files still carry a non-zero allowance, so the drain is not finished. MEASURED - gate red before, exit 0 after, and stable across two consecutive runs - baseline -176/+25 entries, all date-rollover - inert-seam, sql-literal, lane-wiring gates and the census: all green - the reconciler's own suite: green Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… on them (#3010) ## `main` is currently red on `check-fnxc-future-dates` Four stamps read `2026-07-30-26:10` — an hour that cannot exist. They're exactly what #2995 taught this gate to catch. That PR landed the hour validation (`00-23`) *after* #2999 had already merged these four, so the gate started reporting a defect that was already sitting there rather than one introduced afterwards. **The guard is working**; nothing was checking before it. ``` scripts/lib/backend-db.mjs:41 scripts/reconcile-task-state-consistency.mjs:8, :51 scripts/__tests__/reconcile-task-state-consistency.test.mjs:109 ``` Corrected by **literal normalisation** — 26:10 on the 30th *is* 02:10 on the 31st — rather than flattening them to an arbitrary in-range hour. AGENTS.md specifies `yyyy-MM-dd-hh:mm`, and the stamp exists to give a readable why-does-this-exist trail, so the ordering is the part worth preserving. ## The baseline tightening rides along, and it's a date rollover Stamps written yesterday as `2026-07-31` were future *then* and were baselined as such. Today they're past, so **176 files ratchet to zero**. Nobody did anything. The gate rewrites the baseline as a side effect and exits 0, so leaving it uncommitted dirties the tree on every subsequent run **for everyone** — which is why it belongs in this commit rather than a later one. Re-recording on a decrease is the rule this gate and its siblings already state. Worth knowing about the design, since I wrote it: this churn recurs whenever a day boundary passes with future-dated stamps in the baseline, and it shrinks only as people stop writing them — which is the behaviour the gate exists to produce. **93 files still carry a non-zero allowance**, so the drain isn't finished. If it stays noisy once those clear, the gate's fail-on-tighten contract is the thing to revisit, not the stamps. ## Measured | check | result | |---|---| | gate | red before, **exit 0 after**, stable across two consecutive runs | | baseline | −176/+25 entries, all date-rollover | | inert-seam · sql-literal · lane-wiring · census | all green | | reconciler's own suite | green | ## One correction to a claim I made earlier this session While investigating I reported the gate as hanging for 600s. It wasn't — the harness killed the process (exit 144) and the empty output made it look like a stall. The gate completes in seconds. Noting it because I nearly filed a performance bug against a healthy script. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…3025 fixed (#3027) **Stacked on #3025** (its commit is the parent here) — that PR fixes two producers of a dock/plugin-rendered `TaskCard`, and this adds the coverage for the second one. ## The gap #3025 correctly fixes **both** producers, which is the Surface Enumeration discipline working. Its test covers only `MainContent`. Measured by deleting the identical line from `useRightDockController`: ``` MainContent.graph-popout 6 passed RightDock 33 passed TaskCard.host-inventory 1 passed ``` All green with the dock's wiring gone. I checked every suite that touches that hook; none observes the prop. Its own test comment says: > REVERT CHECK: drop `taskColumnFlags` from **either** `renderTaskCard` and this reads "none". For this producer that is not true, and it is the producer that draws cards into the **right dock**, where an operator actually sees them. So the pair could quietly become a single again with every test still green. ## The test | state | result | |---|---| | #3025 as merged | 2/2 pass | | delete the dock's `taskColumnFlags={…}` line | **1 failed / 1 passed** | Driven through the real `renderTaskCard`, captured off the `renderProps` the controller hands `RightDock` — it is not on the returned controller object. `RightDock` itself is stubbed so the assertion cannot fail for unrelated dock plumbing. The paired negative asserts an unresolved task receives `undefined` rather than a fabricated object. That direction matters: inventing flags would make a card claim traits its board never declared, which is worse than the legacy fallback it replaces — the same *report, don't guess* reasoning as #2999's `!target` refusal. ## One harness note My first mock replaced `../RightDock` wholesale and the hook died on `No "readStoredRightDockOpen" export is defined on the mock` — the controller imports its persistence helpers from that module. Spreading `importOriginal()` and overriding only the two components fixes it. Recorded in the file because the next person stubbing this module will hit the same thing. **Verified:** 2/2, `tsc -p tsconfig.app.json` 0 errors in the new file, lint clean, FNXC gate exit 0. If #3025 lands first this rebases to a single test commit; if the two are taken together the stack applies as-is. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A repair script that wrote a column the board does not have
Under
--apply, against an operator's live database: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:
so a legacy board repairs exactly as before.
Correcting myself
On #2994 I wrote that this follow-up "needs the same
importCoreseam". It doesn't:openBackendalready returns{ core, store, … }and this script already destructurescore. No new plumbing was required. I posted that correction on #2994 too, since acting on it would have wasted someone's time.Revert proof
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 --testacross all three script suites — 17 passed / 0 failednode --check,eslint— cleancheck-sql-column-literals,lifecycle-column-census --strict,check-lane-wiring,check-fnxc-future-dates— greenNo 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-literalsto 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 walksPACKAGESonly and never scansscripts/.That is the same shape as the lane-wiring gap #2978 closed (it scanned neither
pluginsnordashboard/app).scripts/audit-branch-cross-contamination.mjs:185still holdsWHERE … "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.