ci(studio): gate new useEffect call sites behind a per-file ratchet

[miguel-heygen]

CLAUDE.md bans useEffect in studio, but nothing enforced it, so the call sites kept
accumulating while the rule read as absolute. This adds check:no-use-effect to the root lint
chain (.github/workflows/ci.yml runs bun run lint at line 160). No existing call site is
changed.

What the gate does

scripts/check-no-use-effect.mjs walks packages/studio/src, parses each .ts/.tsx file with
the TypeScript compiler, and counts calls to useEffect and useLayoutEffect. It compares each
file against a per-file budget seeded from what exists today: 281 call sites across 154 files,
plus the one sanctioned caller. Three ways to fail:

• a file not in the budget has an effect (NEW),
• a budgeted file grows (OVER BUDGET),
• a budgeted file shrinks and the number was not lowered (STALE).

The last one is what makes it a debt register rather than a permanent allowance: the budget can
only ever go down, and the script is the only place it lives.

Counting is done on the AST rather than with grep, because docblocks quote the banned pattern
while explaining it, and prose about a rule must not count as breaking it. Aliasing is resolved
rather than assumed away: import { useEffect as x }, React.useEffect and a namespace import
all resolve to the same call and all count. Both hooks share one budget, so neither can be
smuggled in by spelling it the other way.

How to shrink a budget

Remove the effect, then lower that file's number in BUDGET; delete the entry when it reaches
zero. The check tells you the number to write. When every entry is gone, delete BUDGET and the
script becomes a flat ban.

The allowed escape

useMountEffect() (packages/studio/src/hooks/useMountEffect.ts) is the one sanctioned way to
sync once with an external system on mount. That file is excluded outright rather than budgeted,
because the budget must be able to reach zero and this entry never will. Everything else derives
during render, does the work in an event handler, uses a data-fetching library, or resets with a
key.

Verification

Full bun run lint is green with the step wired in:

```
$ node scripts/check-no-use-effect.mjs
no-use-effect: no new useEffect or useLayoutEffect in packages/studio/src. 281 budgeted call site(s) across 154 file(s) remain.
```

Proved non-vacuous by adding one useEffect(() => {}, []) to a studio file and re-running, then
reverting:

```
OVER BUDGET: packages/studio/src/hooks/useFileTree.ts has 2, budget allows 1
Lines: 18, 88. The budget is debt, not headroom.

1 problem(s). Budgeted debt is 281 across 154 files.
```

A new unbudgeted file using the aliased, namespaced and layout spellings fails the same way:

```
NEW banned effect hook: packages/studio/src/probeGate.ts:4, :5, :6
The ban is absolute. Use useMountEffect() for a one-time external sync, or derive
the value during render. See CLAUDE.md > React Rules.
```

Since review

The Fallow audit flagged four functions in the new script on CRAP score, which weighs cyclomatic
complexity by the share of the function no test reaches. Coverage is not the payable half here:
fallow's static estimator scores every .mjs file in this repo at 0% whether or not it has a test
(check-cli-process-ownership.mjs and check-package-cycles.mjs sit at 42 and 132 today, and pass
only because --gate new-only grandfathers them). So the score came down on complexity, and the
audit is untouched: no suppression comment, no ignore entry, no raised --max-crap.

Each function now makes one decision. Resolving a react import, reading a namespace name,
following a useEffect as x alias and matching a callee are four named predicates instead of one
nested condition, and the budget comparison splits into "what is wrong with this file" and "what is
wrong with this entry". main() also hoists the source walk out of its loop rather than rebuilding
the file list once per sanctioned entry.

The gate itself is unchanged: same rule, same messages, same budget numbers, same exit codes. The
helpers are now exported and scripts/check-no-use-effect.test.mjs drives them the way the sibling
check scripts are tested (registered in test:scripts, 15 cases). Break-and-revert still holds
after the split, on both failure shapes:

```
$ node scripts/check-no-use-effect.mjs # a new aliased effect in an unbudgeted file
NEW banned effect hook: packages/studio/src/__probe.tsx:2
The ban is absolute. Use useMountEffect() for a one-time external sync, or derive
the value during render. See CLAUDE.md > React Rules.
exit=1

$ node scripts/check-no-use-effect.mjs # App.tsx budget nudged 1 -> 9
STALE budget entry: packages/studio/src/App.tsx now has 1, budget still says 9
Lower it to 1. Thanks for paying the debt down.
exit=1
```

The tests are non-vacuous: breaking alias resolution, the stale-budget comparison, and the module
specifier check each turns cases red (2, 1 and 2 failures), and all 15 pass on revert.

Local runs of the audit exactly as CI invokes it, before and after:

```
$ bunx fallow audit --base origin/main --fail-on-issues --format pr-comment-github

before: exit 1, 4 findings (reactBindings 210.0, listBudgetIssues 90.0, visit 56.0, walk 30.0)

after: exit 0, "No GitHub PR/MR findings."

```

bun run lint, bun run test:scripts (203 pass) and bun run format:check are green.

Since review

Head d378a3908. Points 2 and 3 of the review, plus the branch-protection question.

Every spelling the review named is now resolved, and nothing is silently unhandled. The script
header carries the full list, each entry either detected or out of scope with its reason:

Spelling	Status
dynamic import of the module, awaited or not, bound or destructured	detected
require of the module, bound or destructured	detected
computed namespace access, React["useEffect"]	detected
barrel re-export written under packages/studio/src	detected, in the barrel itself
.js / .jsx sources	detected, same terms as .ts / .tsx
local alias, transitively (const a = useEffect; const b = a;), any declaration order	detected
barrel re-export written outside the scanned tree	out of scope: needs cross-package module resolution, and no file under packages/ re-exports react today
runtime indirection a static pass cannot follow (React[flag ? "useEffect" : "useMemo"], eval, a hook read out of a data structure)	out of scope: a ratchet resists drift, not an author defeating it, who can equally edit the budget map

The barrel case is closed where it is written rather than where it is imported: the re-export is
itself the file the laundering route needs, and it is in scope, so export { useEffect } from "react" and export * from "react" fail in the barrel. That is a root fix, not a per-consumer one.

The sanctioned escape is no longer skipped wholesale. Skipping the file made it the one place
any effect could hide. useMountEffect.ts must now contain exactly one banned-hook call and that
call must pass an empty dependency array; a second effect, a non-empty array, or no array at all
fails the check. Three tests cover those three shapes, and one holds the real file to it.

Branch protection: no ruleset change required. Lint is not a required context, so the gate
also runs in the required Typecheck job (bun run check:no-use-effect, one added step). A
failing rule now blocks a merge without a repo-admin action.

Break-and-revert, once per new detection, each mutation reverted afterwards:

dynamic import / require     -> exit 1, 2 failures
computed namespace access    -> exit 1, 1 failure
local alias chain            -> exit 1, 3 failures
barrel re-export             -> exit 1, 1 failure
.js sources                  -> exit 1, 1 failure
sanctioned shape             -> exit 1, 3 failures
reverted: 29 pass, 0 fail

The baseline is unchanged by the wider resolution: still 281 calls across 154 files, so the new
spellings add no previously-missed debt.

$ bunx fallow audit --base origin/main --fail-on-issues
Audit scope: 5 changed files vs origin/main (0562d9258..HEAD)
✓ No issues in 5 changed files

bun run lint, bun run test:scripts (217 node cases + 42 vitest) and bun run format:check are
green.

One thing worth flagging for whoever reads the script next: the header deliberately writes the
module specifier as a <react> placeholder inside require(...) / import(...) in prose. Spelling
it literally in a comment makes the dependency audit read the comment as a real import and report
react as an unlisted dependency of the repo root.

[jrusso1020]

Approving at 830f1b677e07e50d820976f8cbfac716acaa2ca3. The ratchet is the strong form of this pattern, not the usual weak one, and the reasoning in the header about why neither oxlint shape would do is correct on both counts.

What I verified rather than took on trust

• The seed is real. 154 entries summing to 281, which is the number in the description. Counted off the file, not off the body.
• The gate actually runs. check:no-use-effect is in the lint script and ci.yml:160 runs bun run lint. It is wired, not just written.
• The ratchet is two-sided, which is the part that matters. Over budget fails, and if (actual >= allowed) continue means a file with slack also fails and is told to lower its number. So a budgeted file cannot quietly bank headroom, and effects cannot be shuffled from a tight file into a loose one -- the source drops to actual and the total allowance falls with it. That is a genuine debt register rather than an allowance list, and it is the specific thing most ratchets get wrong.
• Aliasing really is resolved. import { useEffect }, import { useEffect as x }, import React from "react" and import * as React all land in direct/namespaces, and BANNED covering both hooks in one budget closes the obvious dodge of swapping one for the other. Counting CallExpression nodes off the TS AST is also what makes the header's own quoting of the banned name safe, which a grep would have failed on immediately.
• Deletion and rename are handled. A budgeted file that disappears reports actual 0 and asks for the entry to go; a rename reports both halves. SANCTIONED is existence-checked in main so the escape hatch cannot rot into a permanent exemption for a file nobody has.
• The scope has no gap inside studio. SCANNED is packages/studio/src, and packages/studio/frontend holds only package.json and bun.lock while packages/studio/tests has no banned call. So "the studio package" and "packages/studio/src" are the same set today.

The gate runs but does not gate

bun run lint is the only path to this script, and the Lint job is not in main's required status checks. Required today: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest. Lint is also conditional on needs.changes.outputs.code == 'true'.

So a PR that adds an effect gets a red Lint job and can still merge. Every other root check -- package cycles, tracked artifacts, workspace contracts -- sits in exactly the same position, so this is not a regression and not something to fix inside this PR. I raise it because this PR's own premise is that prose alone never failed a build, and there is a second version of that gap one level up: a check that fails a job nobody is required to pass. Worth closing on purpose rather than inheriting by default.

Fallow, which is not required either but is saying something specific

Four findings, all high-crap-score, and the numbers are worth reading rather than dismissing:

function	cyclomatic	CRAP
walk	5	30
visit	7	56
listBudgetIssues	9	90
reactBindings	14	210

CRAP is comp^2 * (1 - coverage)^3 + comp. Every one of those four is exactly comp^2 + comp, which is the value at coverage zero. So Fallow is not really complaining about complexity here -- it is reporting, precisely, that this file has no tests.

That is the note I would act on. The script is already built for them: callSites(file, text = readFileSync(...)) takes the source as a parameter and listBudgetIssues(found, budget = BUDGET) takes the register as one, and main is behind an import.meta.url guard so the module imports cleanly. Those seams only exist for a caller that does not exist yet. A handful of cases over callSites with inline sources -- the alias, the namespace call, the docblock that must not count, the .d.ts skip -- plus two over listBudgetIssues for the over-budget and stale-slack directions, would pin the behaviour this whole gate rests on and would take the critical finding with them. This file is now load-bearing for a rule the repo intends to enforce forever; it deserves the same standard it imposes.

Two narrow bypasses, worth knowing rather than fixing

• Only from "react" counts. A hook imported through a local barrel or a compat shim resolves to nothing and is not counted. There is no such barrel in studio today, so this is latent, but it is the shape a future refactor could walk into without noticing.
• const { useEffect } = React is not a call site the walker sees. Destructuring off a namespace import produces an identifier that is not in direct. Obscure, and nobody writes it deliberately, but it is the one spelling the alias handling does not cover.

Neither is worth code today. Both are worth a line in the header beside the aliasing paragraph, so the next reader knows where the edges are rather than assuming the resolution is total.

The CLAUDE.md section reads well and, importantly, says what to do instead rather than only what not to do -- derive during render, work in handlers, key to reset a subtree. A ban that does not name the alternative is how the alternative becomes useMountEffect for everything.

Review by Rames

[jerrai-bot-heygen]

Request changes at 830f1b677e07e50d820976f8cbfac716acaa2ca3.

The per-file baseline is exact (281 calls across 154 files), and the NEW/OVER/STALE behavior is non-vacuous. Two requirements remain before this can serve as an enforcement gate:

1. The diff causes a red Fallow audit: the new scanner contains CRAP findings, including reactBindings at 210/30. Refactor the scanner enough to clear the repository's maintainability gate, rather than landing a knowingly red check.
2. check:no-use-effect currently runs only through the non-required Lint job. A failing rule therefore need not block a merge. Run it in an existing required CI job (or supply the branch-protection change that makes its job required) and add adversarial tests for the resolved React call forms. In particular, the current scanner passes violations using dynamic import("react"), require("react"), computed namespace access, barrel re-export, .js, and local aliases.

The allowed useMountEffect escape should also be constrained to its intended implementation rather than skipping the file wholesale, or be explicitly tested/justified as a supported boundary.

— Jerrai

[jerrai-bot-heygen]

Request changes at exact head d378a390884c954c92afa926f1e4f28e679918bc.

The scanner coverage and required-Typecheck wiring are now materially stronger, but the sanctioned-wrapper invariant still has a bypass. sanctionedProblem() accepts exactly one banned hook call and then calls isMountEffectCall(), which checks only the argument count and an empty dependency array. It never verifies that the callee is useEffect.

Consequently, replacing the real wrapper's useEffect(effect, []) with useLayoutEffect(effect, []) passes:

• bannedCalls() returns that one call because both hooks are in BANNED.
• isMountEffectCall() accepts its two arguments and empty array.

That violates the stated sanctioned boundary—one mount useEffect—and permits a paint-blocking layout effect exactly where the escape hatch is meant to be narrow.

Require the one call to resolve specifically to useEffect (not merely any BANNED member) and add a regression that mutates the wrapper to useLayoutEffect(effect, []) and expects the gate to fail. The dynamic-import/require/computed/barrel/JS-alias coverage and Typecheck gate can remain as implemented.

Review by Jerrai

[jrusso1020]

At d378a390884c954c92afa926f1e4f28e679918bc. Holding the re-stamp. My 830f1b67 approval is stale anyway under this repo's last-push-approval rule, so treat it as withdrawn rather than merely old.

I confirmed the finding jerrai-bot-heygen raised, at source, rather than seconding it. It is right, and here is the exact mechanism so you do not have to take two reviewers' word for it:

const BANNED = new Set(["useEffect", "useLayoutEffect"]);   // :68

function isMountEffectCall(node) {                          // :477
  const [, deps] = node.arguments;
  return node.arguments.length === 2
    && ts.isArrayLiteralExpression(deps)
    && deps.elements.length === 0;
}

isMountEffectCall reads node.arguments and never touches node.expression. So the callee is unconstrained beyond having already passed bannedCalls, which admits either member of BANNED. Swap the wrapper's body to useLayoutEffect(effect, []) and: bannedCalls returns exactly one node, the count check passes, isMountEffectCall sees two arguments and an empty array, and sanctionedProblem returns null.

The message the function would have printed is the tell -- must call useEffect(effect, []) and nothing else -- so the code does not enforce what its own error text claims. On a ratchet that is the defect that matters most, because the whole value of the file is that it enforces exactly what it says.

The new suite covers the sanctioned file four ways (baseline, a second useEffect(other, []), [effect] deps, and the missing array) and none of them is a callee swap, which is consistent with the hole being in the callee check.

A second hiding place of the same shape, which I do not think has been named

scan() skips the sanctioned file wholesale:

for (const file of sources()) {
  if (SANCTIONED.has(file)) continue;      // :504
  const lines = violations(file);
  ...
}

and sanctionedProblem calls bannedCalls alone. But violations() -- the path that was just skipped -- checks two things, not one:

const found = [...bannedCalls(root), ...root.statements.filter(isReactReExport)];

So export { useEffect } from "react" inside the sanctioned file is checked by nobody. Your own suite proves the gate catches that statement in an ordinary file (assert.deepEqual(violations("a.ts", 'export { useEffect } from "react";'), [2])), and the one file exempt from that path is the one whose whole job is to be the narrow exception.

It is the same bypass class and worth fixing in the same pass, because sanctionedProblem's own docblock is the thing that argues against it: "Skipping the file wholesale would make it a hiding place: any effect, any dependency array, unchecked." Re-exports are the third item that sentence does not list, and they are strictly worse than a stray effect -- a re-export hands the banned hook to every other file under SCANNED through an import the scanner has no reason to distrust.

What I would want before re-stamping

1. Require the one sanctioned call to resolve specifically to useEffect, not merely to a BANNED member.
2. Have sanctionedProblem also reject a react re-export, so the exemption is from the BUDGET and not from the whole check.
3. Two regressions, each mutating the real wrapper: useLayoutEffect(effect, []), and a export { useEffect } from "react" added to it. Both should fail the gate.

Everything else in this revision holds up and I am not asking you to revisit it. The expanded spelling coverage is real -- dynamic import, require, computed namespace access, in-tree barrel re-export, .js/.jsx sources and local alias chains -- with a break-and-revert per spelling, and documenting the cross-package barrel as out of scope with the observation that none exists in the repo is the right way to bound it. useMountEffect.ts no longer being skipped is the change that makes the sanctioned entry mean anything at all, which is why these two holes are worth closing now rather than later: they are the residue of exactly the gap this revision set out to close.

Moving the gate into the required Typecheck job so it blocks without a ruleset change is the right lever, and worth saying out loud that it is now load-bearing: a gate that runs and does not block is a lint, and this one is meant to be a ratchet.

Not a second block -- jerrai-bot-heygen's request-changes at this head is live and gating, and this is deliberately additive to it. Point 2 is the part I would not want lost between the two reviews.

No merge or enqueue action taken.

Review by Rames