gate: run the inert-sync-lane ratchet in test:gate — it was wired to nothing - #3136
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 33 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. 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
force-pushed
the
gate/wire-ratchet-into-test-gate
branch
5 times, most recently
from
July 31, 2026 14:00
2e119cd to
02ce5b7
Compare
…nothing check:inert-sync-lanes has existed since #3062 and was hardened three times (#3068, #3079, #3122 pending). It is invoked by NOTHING — not test:gate, not any workflow. It only runs if a human types it. What that cost, in one sequence: #3108 flagged triage.ts:765 — "the obvious next move is to convert the third with the same helper. That would be wrong twice over", both blockers named, backed by a test. #3114 converted exactly that arm, replaced the warning with its own note, census 45 -> 44. #3126 reverted it, once someone ran the ratchet by hand and found main red. Hours apart, all green in CI, no behaviour change on any board. The ratchet flagged it correctly the whole time; nothing ran it, so the round trip cost three PRs instead of one failed check. Wired beside the other cheap AST guards it belongs with (check-no-nohup, check-capacity-pool-id, check-mock-completeness). Single-pass parse, no measurable cost. Scope reduced: the earlier version of this branch also restored the triage literal. #3126 landed that first, so this is now the wiring alone.
gsxdsm
force-pushed
the
gate/wire-ratchet-into-test-gate
branch
from
July 31, 2026 14:09
02ce5b7 to
81cacca
Compare
This was referenced Jul 31, 2026
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…uards were invisible (#3181) ## The fourth shape: a sync lane handed to a wrapper #3169 taught `unwrapForSyncCall` to walk await, parenthesized, conditional and binary expressions. It still stops at the **call boundary**, so a source call sitting in an *argument* position stays invisible: ```ts const parked = mergeParkedColumns(resolveTaskParkedColumnsSync(store, id), lanes); ``` That prefers the event payload and falls back to the sync answer whenever `lanes` is absent. The callee is `mergeParkedColumns`, not a source — so the walker never looked inside, and **the entire `scheduler.ts` file read as clean**. ``` main today: 9 (triage 7, executor 2, scheduler 0) this PR: 22 (scheduler 13, triage 7, executor 2) ``` Thirteen guards. And `check:inert-sync-lanes` has run in `test:gate` since #3136, so CI is currently enforcing a ratchet that reports a file it cannot see into as fully converted. The green is official, which makes it worse than the version nobody ran. ## Is the fallback still reachable? Yes, which is why these are not retired. #3135 attached lanes at every *live* emitter, but absence remains reachable three ways: the two `lifecycle-ops.ts` emitters on the SQLite-only polling path, any future emitter added without lanes, and the three forwarders (`project-manager.ts`, `remote-node-runtime.ts`, `child-process-runtime.ts`) that reconstruct the event object field-by-field rather than forwarding it. A rarely-exercised fallback is still a fallback. Counting it as clean is how the ledger stops meaning anything. ## The change One line inside your walker, plus its note: ```js if (ts.isCallExpression(n)) { for (const a of n.arguments) walk(a); } ``` Every shape #3169 added is preserved. Still a name match, not dataflow — the limits section still applies. ## Mutation evidence — all three shapes, one tree | Mutant | Result | |---|---| | baseline (22) | exit 0 | | **argument position** (this PR) | **exit 1**, 13 → 14 | | conditional (#3169's) | exit 1, 13 → 14 | | inline (#3062's) | exit 1, 13 → 14 | `scheduler.ts` restored clean after each run. ## Baseline 9 → 22 **Detection, not regression.** No production file changes in this PR. 22 is the exact union I measured before #3169 merged (13 + 7 + 2) and posted on both PRs at the time — it landing unchanged is the confirmation that the two fixes were additive rather than overlapping. ## Census before / after ``` before: COLUMN guards (the backlog): 12 after: COLUMN guards (the backlog): 12 ``` Unchanged — this converts nothing. It restores 13 guards to a ledger that had silently dropped them. ## Supersedes #3122 #3122 carried this fix as a standalone rewrite of `syncLaneLocals` and conflicted with #3169 the moment it landed. This is the six-line version I offered there; #3122 is closed. ## Verification `test:gate` exit 0 · `check:inert-sync-lanes` exit 0 at the re-recorded baseline · plus `fnxc-future-dates`, `lifecycle-columns`, `quarantine-ledger`, `inert-flag-seams`, `lane-wiring`, `sql-column-literals` — all exit 0. Gate script + baseline only. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved detection of synchronous operations nested within wrapper arguments. * Updated synchronization checks to report all currently identified findings, including additional scheduler-related cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…a defect class the census cannot see (#3149) The census backlog is verified-exhausted (12 guards, every blocker re-checked in #3082). This is from the class **the census structurally cannot count**, and it is a real capability loss. ## The defect ```ts const GITHUB_TRACKING_EDITABLE_COLUMNS: Set<ColumnId> = new Set<ColumnId>(["triage", "todo", "in-progress", "in-review", "ideas"]); function canTaskEditGithubTracking(column, workflowId) { return GITHUB_TRACKING_EDITABLE_COLUMNS.has(column) || workflowId === CODING_IDEAS_WORKFLOW_ID; } ``` No resolved branch, no flags fallback. On a board whose lanes are renamed this matched **nothing**, so the helper returned `false` for every task and `showGithubTrackingSection` hid the section outright. **The operator could not turn GitHub tracking on or off** — no error, no explanation, the affordance simply absent. The only thing keeping it reachable was the unrelated `builtin:coding-ideas` escape hatch on the right-hand side. ## Why no gate saw it, and why this class matters now The census counts **comparisons** against legacy ids. This is a **Set literal — a definition** — consulted with `.has()`. Nothing in the backlog ever pointed here. It is the same blind spot that hid `TIME_INDICATOR_COLUMNS` and `BLOCKER_ESCALATION_COLUMNS`, both of which were also found by hand rather than by any gate. I found it by scanning for legacy-id **collections that gate a live column value**, rather than for comparisons: **19 such sites** across the tree. Most are already correct — either `if (!flags) return LEGACY_…has(column)` fallbacks, or seed-then-add resolved sets (`agent-reflection.ts`, `ephemeral-worker-manager.ts`). This one had neither. With the comparison backlog at 12 and every remaining entry blocked or documented, **this is where the remaining renamed-board defects actually live.** ## The fix The set's meaning is "not finished" — every lane except complete and archived — which is what the roles now express: ```ts if (workflowId === CODING_IDEAS_WORKFLOW_ID) return true; if (!columnFlags) return GITHUB_TRACKING_EDITABLE_COLUMNS.has(column); // unchanged pre-fetch return !isCompleteColumnRole(columnFlags, column) && !isArchivedColumnRole(columnFlags, column); ``` The caller passes `detailColumnFlags` — the **task-identity-guarded** value. `workflowMoveMetadata` outlives a task switch, and this file's own `2026-07-30-17:30` note records **six** review findings from consumers that read around that guard. Passing the unguarded value would answer about the previous card's workflow: worse than the legacy fallback, because it is confidently wrong rather than merely stale. ## Verification | | result | |---|---| | suite | **3 passed** | | mutation (restore the literal) | **1 failed \| 2 passed** — the renamed-WIP case only | | dashboard `tsc -p tsconfig.app.json` | **0 errors** | | census `--strict` | exit 0, **unchanged** — this class is invisible to it | The test drives the **production path** (`fetchBoardWorkflows` → `resolveTaskWorkflowMetadata` → `currentColumnFlags`) rather than injecting flags as props, so it covers the producer as well as the consumer. `building` and `shipped` collide with no legacy id, so a surviving `.has(column)` cannot pass by luck; the `todo` control pins that the default vocabulary is unaffected, and the renamed-COMPLETE negative pins that the fix does not hand editability to a finished card. Note: `tsconfig.test-check.json` fails on `main` as well — pre-existing, and **zero** of its errors come from this branch's files. ## Suggested follow-up The remaining 17 collection sites deserve the same pass, and the scan that found this should probably become a gate — a census that counts comparisons will keep reporting zero while this class quietly grows. I have not built that here because the existing gates already need `#3136`'s attention first, and adding a sixth advisory check that nobody blocks on would repeat the pattern this session keeps running into. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…tern (#3252) ## What this is #3251 audits the five lifecycle ratchets with staged probes and claims a gap in mine: > `check-inert-sync-lane-conversions` — does NOT catch: a DIRECT `store.resolveTaskWorkflowIrSync(...)` read feeding `resolveLifecycleColumns` I tested it rather than accepting it, and got a **split result**: a probe inserted into the existing `executor.ts` was **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 function `isReview` was 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: | argument to `resolveLifecycleColumns(...)` | before | after | | --- | --- | --- | | `store.resolveTaskWorkflowIrSync(id)` | caught (20) | caught (20) | | `store.resolveTaskWorkflowIrSync(id) as never` | **MISSED (19)** | caught (20) | | `store.resolveTaskWorkflowIrSync(id)!` | **MISSED (19)** | caught (20) | | `(store.resolveTaskWorkflowIrSync(id) as any)!` | **MISSED (19)** | caught (20) | So: the direct read **is** tracked. The **cast around it** was not. `unwrapForSyncCall` unwrapped `await`, parentheses, conditionals, binaries and (since #3181) call arguments — but stopped at `as`, `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`, `satisfies` and `!` 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 - Mutation-verified in both directions: with the fix reverted all three cast forms read 19; with it applied all read 20. - All eight ratchets exit 0: `inert-sync-lanes`, `lifecycle-columns`, `fnxc-future-dates`, `quarantine-ledger`, `move-target-literals`, `inert-flag-seams`, `lane-wiring`, `sql-column-literals`. - `pnpm test:gate` exit 0 (which runs this script since #3136). - Every probe removed; `git status` clean 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.
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.
A gate that nothing runs
check:inert-sync-laneshas existed since #3062 and been hardened three times (#3068, #3079, and #3122 pending). It is invoked by nothing — nottest:gate, not any workflow. It is apackage.jsonscript that only runs if a human types it.Here is the round trip that cost, on
main:triage.ts:765: "the obvious next move is to convert the third with the same helper. That would be wrong twice over." Both blockers named, backed bysync-workflow-ir-second-blocker.test.ts.mainred.Hours apart. All three green in CI. No behaviour changed on any board at any point. The ratchet flagged it correctly the entire time; nothing ran it, so a caught defect cost three PRs instead of one failed check.
This is not a criticism of #3114's author. A written warning at the exact line, with a test behind it, was overwritten within hours by a well-intentioned change — that is simply what unenforced prose does under fleet pressure. It is the fourth time this class has landed (#3051, the
resolveMoveFanoutColumnsSyncfamily, #3114, and my own--triagedraft that failed to zero).What this does
One line: adds
node scripts/check-inert-sync-lane-conversions.mjstotest:gate, beside the cheap AST guards it belongs with (check-no-nohup,check-capacity-pool-id,check-mock-completeness). Single-pass parse; no measurable cost.Scope reduced from the earlier version
This branch previously also restored the triage literal. #3126 landed that first, so I rebuilt it as the wiring alone rather than carry a half-redundant diff. #3127 is closed with nothing lost.
Census before / after
Unchanged — this converts nothing. It makes an existing check actually run.
Verification
test:gateexit 0 with the ratchet inside it, against currentmain(now green on the ratchet since #3126). Diff is one line ofpackage.json.Related
#3122 restores 13 guards laundered through
mergeParkedColumns— worth landing after this so the ratchet enters CI at full sensitivity. #3135 attaches lanes at the last two live emitters. #3131 corrects a 44% under-report in--triage.