[tps-anvil]

Closes #603

The post-merge audit of #576 and #583 found five gaps in the cli restore guard.

1. Discovery. discoverCliTestFiles uses the shared discoverTestFiles in
   scripts/check-test-reports.mjs over the cli package root and returns absolute
   paths. The loaded-set assertion checks files recorded by the preload when
   the assertion runs. Files parse according to their extension, including
   TSX for .tsx and JSX for .jsx.
2. Patch factory. createPatchRegistry (the body of createPatchShared)
   throws PatchAfterCleanupError when a patch is requested after its cleanup
   hook ran. Redundant fetch patches in top-level afterAll hooks are removed
   from agent-seed.test.ts and memory-learn.test.ts.
3. Destructured guarded root. Object bindings with identifier or
   string-literal keys preserve the selected property's identity, including
   nested bindings: const { process: p } = globalThis binds p to process.
   Computed binding keys and array bindings of guarded roots are out of scope.
   Values extracted from process.env are not guarded-root aliases.
4. Lexical scope. Alias maps use declaring scopes. Loop scopes declare
   block-scoped bindings; the existing hoisted-variable handling resolves var.
5. Per-write assertions. Each expected finding in the red multi-write cases is
   asserted separately; removing a corresponding write fails its test.

Evidence, measured on 898e78e against 1477f57:

• Red/green per ask, analysing the same fixtures with the original and corrected
  scanners: all eight new assertions fail on 1477f57 and pass here. They cover
  writes after a var loop and inside a nested function; direct environment
  destructuring; environment writes and p.exit through selected process
  aliases, including nested bindings; JSX followed by a global patch in .tsx
  and .jsx; and discovery from a relative root.
• One mutation per mechanism (apply, run both guard test files, restore):
  • Narrow discovery to .test.ts: 1 failure.
  • Drop the PatchAfterCleanupError refusal: 3 failures, including real afterAll.
  • Skip object-binding aliases: 3 failures.
  • Resolve module objects without scope: 2 failures.
  • Remove one write from a red multi-write fixture: 1 failure.
  • Let loop scopes declare var: 2 failures.
  • Restore the original guarded-root alias resolver: 3 failures.
  • Force TypeScript parsing for JSX files: 2 failures.
  • Preserve a relative discovery root: 1 failure.
• Guard test files, 1477f57 -> 898e78e:
  mock-restore-guard.test.ts 74 -> 82 pass; patch-shared.test.ts 7 -> 7 pass.
  With the new assertions and original scanner: 81 pass / 8 fail; corrected:
  89 pass / 0 fail.
• CLI bun run build (tsc): exit 0. bun run lint:ci: exit 0.
• node scripts/changelog-fragments.mjs check: 87 fragments valid.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved detection of changes to guarded targets, including changes made through aliases, so they can be flagged when they lack proper restoration.
  • Clarified that guarded-target assignments must use the shared patching mechanism, regardless of how the assigned value was created.
  • The restore guard now checks test files across supported JavaScript and TypeScript formats and rejects patches requested after cleanup.
• Tests
  • Expanded coverage for guarded-target aliases, nested scopes, JSX files, and test-file discovery.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: f3080b47-095b-4e83-a6b7-3c686f88b90c

📥 Commits

Reviewing files that changed from the base of the PR and between 8d46173 and cd9ebed.

📒 Files selected for processing (20)

• .changelog/unreleased/fixed-577-guard-decides-by-target.md
• .changelog/unreleased/fixed-603-restore-guard-coverage.md
• packages/cli/test/agent-seed.test.ts
• packages/cli/test/fixtures/mock-restore-guard/destructured-env-exempt.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/destructured-root-alias-leaked.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/direct-env-destructured-exempt.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/jsx-global-leaked.fixture.jsx
• packages/cli/test/fixtures/mock-restore-guard/jsx-global-leaked.fixture.tsx
• packages/cli/test/fixtures/mock-restore-guard/loop-var-after-leaked.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/loop-var-nested-leaked.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/module-alias-outer-scope-leaked.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/module-alias-shadow-cleared.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/module-shadow-param-cleared.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/nested-selected-process-alias.fixture.ts
• packages/cli/test/fixtures/mock-restore-guard/selected-process-alias.fixture.ts
• packages/cli/test/helpers/mock-restore-guard-scan.ts
• packages/cli/test/helpers/patch-shared.ts
• packages/cli/test/memory-learn.test.ts
• packages/cli/test/mock-restore-guard.test.ts
• packages/cli/test/patch-shared.test.ts

💤 Files with no reviewable changes (1)

• packages/cli/test/memory-learn.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The CLI restore-guard scanner now covers shared test discovery, supported file extensions, and scope-aware guarded-root and module-object aliases. The patch registry now restores registered patches during cleanup and rejects patch requests made after cleanup.

Changes: Restore guard scanning

Layer / File(s)	Summary
Scope-aware alias tracking packages/cli/test/helpers/mock-restore-guard-scan.ts, packages/cli/test/fixtures/mock-restore-guard/*, packages/cli/test/mock-restore-guard.test.ts, .changelog/unreleased/fixed-577-guard-decides-by-target.md	The scanner resolves guarded-root and module-object aliases by scope. Fixtures and assertions cover property-derived aliases, destructuring, shadowing, loop variables, and excluded bindings.
Test discovery and parsing packages/cli/test/helpers/mock-restore-guard-scan.ts, packages/cli/test/mock-restore-guard.test.ts, .changelog/unreleased/fixed-603-restore-guard-coverage.md	The scanner uses shared test discovery and selects a parser from the file extension. Tests check discovery coverage, preload-recorded files, and root-path handling.

Patch registry cleanup

Layer / File(s)	Summary
Patch registry lifecycle packages/cli/test/helpers/patch-shared.ts, packages/cli/test/patch-shared.test.ts, packages/cli/test/agent-seed.test.ts, packages/cli/test/memory-learn.test.ts	The patch registry restores registered patches in reverse order and throws PatchAfterCleanupError for later patch requests. Tests cover cleanup and remove separate fetch restoration.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: heskew, tps-sherlock

Merge Risk: ⚪ Minimal · up to cd9eb

No actionable issue remains from this review; the PR is mergeable after normal checks.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 17 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the main change: expanding the CLI restore guard to cover test files discovered by Bun. It is concise and specific.
Linked Issues check		Issue #603 has one active coding target. The changes implement shared discovery from the CLI package root, absolute loaded-file checks, extension-based parsing, post-cleanup patch rejection, scoped gu…
Out of Scope Changes check		The changed scanner, patch helper, tests, fixtures, changelog entries, and removal of redundant top-level cleanup patches directly support issue #603. The stated exclusions for computed keys, array bi…

Full details: Docstring Coverage

Explanation

Docstring coverage is 48.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 17 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[tps-anvil]

Claim sweep — cli#603 (test/603-restore-guard-coverage)

Every sentence this branch adds or changes (prose, comments, fragment, test titles;
git diff origin/main...HEAD), checked against the code. Claim types: (a) outcome/status
on a branch, (b) scope words such as every/all/only/never/always/none, (c) coverage
claims (does the named test assert it?).

Changelog fragment .changelog/unreleased/fixed-603-restore-guard-coverage.md

1. "The cli test guard scans every cli test file bun discovers…" — (b) scope. True: the
   guard scans discoverCliTestFiles(CLI_ROOT) and CLI_ROOT is the cli package root the
   cli suite runs from; discovery is the shared bun-1.3.10 rule. (c) discovered cli test files satisfy the static guard + the scan discovers the forms bun discovers.
   Adjusted from "every test file bun discovers" to "every cli test file bun discovers" —
   the guard is the cli guard, the discovery root is the cli package.
2. "…its patch factory refuses a patch made after cleanup ran." — (a). True:
   createPatchRegistry throws PatchAfterCleanupError once cleanedUp; asserted by
   patch-shared.test.ts (pure + real afterAll).
3. "discovers the cli package's test files by bun's own rule … rather than a hand-kept
   *.test.ts glob" — (a). True: the diff replaces the /\.test\.ts$/ walk with the shared
   discoverTestFiles.
4. "a file in another discovered form is scanned instead of escaping" — (c). Asserted by
   the scan discovers the forms bun discovers.
5. "a run also fails when a file bun loaded is not in the scanned set" — (c). Asserted by
   every cli test file this run loaded through the guard is in the scanned set.
6. "A name destructured out of a guarded global root (const { process } = globalThis) is
   treated as that root" — (a). Red fixture destructured-root-alias-leaked.fixture.ts.
7. "while a name destructured out of process.env is not" — (a). Green fixture
   destructured-env-exempt.fixture.ts.
8. "Module-object and guarded-root aliases are resolved by lexical scope, so a local that
   shadows a name is not the guarded target." — (a). Green fixtures
   module-shadow-param-cleared.fixture.ts, module-alias-shadow-cleared.fixture.ts.
9. "createPatchShared's factory throws PatchAfterCleanupError … instead of leaving it
   unrestored." — (a). patch-shared.test.ts.

Comments — packages/cli/test/helpers/mock-restore-guard-scan.ts

10. "Every test file bun discovers under rootDir, as absolute paths, sorted." — (b).
    True for the given root. (c) the discovery test.
11. "…so the guard scans what a suite runs — …" — (a). Adjusted: dropped "exactly"
    (over-strong: it equals a suite's discovery only when the root is that suite's cwd,
    which CLI_ROOT is).
12. "A directory the walk cannot read throws, so a partial discovery never reads as a
    covered set." — (b),(a). True: discoverTestFiles throws on a failed readdir; asserted
    by discoverTestFiles tests in test-reports.test.ts.
13. "A name destructured from process.env is the env-leak preload's object, not a guarded
    root, so it is not tracked." — (a). Green fixture (7).
14. moduleObjectNames doc: "resolving the name by scope means a local that shadows an import
    or an alias is not the module object, while an alias an outer scope declares is." — (a).
    Green fixtures + red module-alias-outer-scope-leaked.fixture.ts.
15. PatchAfterCleanupError doc: "After that hook runs, a later patch would never be
    restored, so the factory refuses it" — (b),(a). True.
16. Error message "a patch cannot outlive cleanup" — (a). True for the factory's patches
    (before cleanup: restored; after: refused).
17. bindingIdentifiers doc "keeps the identifier nodes (for scope resolution)" — (a). True.
18. moduleObjectAt doc "resolving the name by scope" — (a). True.

Fixture comments

19–23. The five new fixtures' header comments each state the fixture's expected finding and
"Not named *.test.ts, so no suite discovers it." — (a),(c). True: .fixture.ts does
not match TEST_FILE_NAME; shown by the discovery test and by the full-suite file count
(207 files run == 207 discovered).

Test titles

24. "the scan discovers the forms bun discovers, over the cli package" — (b). Adjusted from
    "every form" and the form list extended with .cjs/.jsx so the list covers every
    extension TEST_FILE_NAME accepts ([cm]?[jt]sx?).
25. "every cli test file this run loaded through the guard is in the scanned set" —
    (b). True: the assertion is over the loaded set (loaded.filter(unscanned) == []); it
    asserts nothing about files not yet loaded.
26. "reports each write in global-member-alias-leaked.fixture.ts" — (b). True: two writes,
    two assertions.
27. "clears each write in shadow-nested-scope-cleared.fixture.ts" — (b). The fixture's three
    distinct guarded targets are each checked absent; the two writes to process.exit
    produce the same detail, so the single process.exit check covers both.
28. "refuses a mock identifier bound by a logical assignment, each write on its own" — (b).
    True: both targets asserted.

Fixed

• "scans exactly what a suite runs" → "scans what a suite runs" (over-strong).
• "discovers every form bun discovers" → "the forms bun discovers" + list extended.
• "scans every test file bun discovers" → "every cli test file bun discovers".

[tps-flint]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tps-kern]

APPROVE — head cd9ebed3, reviewed for architecture/verification. Repo visibility checked before writing: tpsdev-ai/cli is public (via repos/tpsdev-ai/cli .visibility).

Discovery matches what Bun actually runs — verified empirically

I ran this host's Bun 1.3.10 (the same version this repo pins) against a sandbox tree containing every form the shared rule [scripts/check-test-reports.mjs TEST_FILE_NAME:137, discoverTestFiles:147] admits and every form it rejects: bun ran exactly the 13 admitted files (.test/_test/.spec/_spec × .ts/.tsx/.js/.jsx/.mjs/.cjs/.mts/.cts, dot-FILES admitted, dot-dirs and node_modules skipped, dist/ walked) and none of the rejected forms (foo-spec.ts, foo.test.helper.ts, plain.ts, dot-dirs). Pattern-for-pattern and exclusion-for-exclusion, measured — not inferred from comments.

• discoverCliTestFiles [packages/cli/test/helpers/mock-restore-guard-scan.ts:528] now delegates to that same shared function over the resolved cli root, and discoverTestFiles throws on any unreadable directory — the guard fails closed on a partial walk.
• Backstops: the forms test [mock-restore-guard.test.ts:39] pins the exact 13-form list; the loaded-set assertion [:58] fails a run when a file the preload recorded (i.e. one bun actually loaded) is outside the scanned set — its stated limit (files loaded after the assertion runs) is acknowledged in the PR; relative/absolute-root equivalence is pinned [:131].
• Census: the guard scans 207 files under packages/cli today, all currently .test.ts — the widened rule is forward coverage for the day a .spec/.tsx file lands.
• Mutation I ran: re-narrowing discovery to .test.ts fails exactly the forms test (1 failure); restore verified, tree clean.

Scope resolution — no case found that reports a shadowed name or misses an outer alias

resolveScope [:208] walks to the nearest enclosing scope that declares the name; guarded-root aliases and module objects are keyed by declaring scope (moduleObjectAt [:241], guardedRootOf). Probes I ran through analyzeSource beyond the fixtures: a shadow nested two functions deep clears (resolves to the param, not the module-scope alias); an outer alias is reported through two nested scopes; a block-hoisted var alias is reported; a value-imported name shadowing a guarded root (import { process }) is reported as a module patch. The new fixtures pin the boundary cases: outer-alias leaked [module-alias-outer-scope-leaked], param shadows cleared for both module objects and their aliases [module-shadow-param-cleared, module-alias-shadow-cleared], loop-var reported both after the loop and from a nested function (the For-scope BlockScoped fix), process.env extraction exempt at every nesting (never an alias), globalThis-rooted destructures selecting process reported.

• Mutation I ran: flattening moduleObjectAt to ignore scope fails exactly the two cleared-fixture tests (2 failures — matching the PR's own evidence for the same mechanism); restore verified.

Per-write assertions (verified by reading)

The multi-write red fixtures assert exact detail arrays (toEqual), so each exempt write's silence and each leaked write's finding are pinned individually — a dropped finding fails and a spurious one fails. Single-write reds assert the finding kind, which write-removal also kills. Matches the PR's claim.

Registry sanity (Sherlock's lane)

createPatchRegistry [packages/cli/test/helpers/patch-shared.ts] restores LIFO everything patched before cleanup and sets the cleanedUp flag before restoring; a patch requested afterwards throws PatchAfterCleanupError before savePatch runs — a post-cleanup patch is never applied and cannot leak. The throw is synchronous at call time, so an async caller surfaces it as a rejection. The two redundant late re-patches removed from agent-seed.test.ts / memory-learn.test.ts are real-world cases of exactly this refusal.

Tests run on this head

Fresh worktree, bun install --frozen-lockfile && bun run build green; suite launcher with isolated HOME and TMPDIR=/tmp: the four touched test files — 98 pass / 0 fail. I characterized no CI lanes.

Findings — non-blocking

1. [mock-restore-guard-scan.ts:161 statementNames vs :335 moduleObjectNames] Type-only imports are counted as scope declarations but skipped as module objects. import type { process } from "./x.js" erases at runtime, yet for the resolver the name shadows the guarded root, so a global write through that name is unreported. Probe-confirmed: analyzeSource on import type { process } from "./x.js"; process.exit = () => {}; returns no findings, while at runtime the write reaches the real global. Exotic — tsc rejects the value-use, so only a file executed by bun alone (bun ignores type errors) could carry it — and compile-refuted in typechecked code. The fix is one line: check importClause.isTypeOnly in statementNames' import branch. Recommend a follow-up.
2. The PR's stated limits (array bindings and computed binding keys of guarded roots untracked; the loaded-set assertion sees only files loaded before it runs) are honest and match the implementation as read.

Not fully seen

hoistedVars and the head of statementNames were pinned by the loop/block-var probes rather than read line-by-line; the guard test's assertion list beyond ~lines 25–140 was sampled; the PR body's evidence section was read through the mutation summary only.

[tps-sherlock]

Verdict: APPROVE — cli#625, head cd9ebed3.

Repo visibility checked before writing: tpsdev-ai/cli is PUBLIC (.visibility: "public"). The single non-blocking observation below is filed by location and severity; its reachability analysis is sent to the maintainer privately.

Security focus — no patch path (sync throw, async rejection, nested registry) can leave a global patched after cleanup: holds.

The guard is in the factory, packages/cli/test/helpers/patch-shared.ts:58:

    if (cleanedUp) throw new PatchAfterCleanupError();
    const restore = savePatch(target, property, value);

The refusal runs before savePatch, so a post-cleanup patch is rejected before the property is ever written. The cleanup hook (lines 53–56) sets cleanedUp = true first, then drains restores in reverse (while (restores.length > 0) restores.pop()!()), which is LIFO and matches savePatch's own undo stack.

• sync throw: test/patch-shared.test.ts:68 asserts the restore-then-refuse order and the error name; the real factory is driven through bun's own afterAll at test/patch-shared.test.ts:106–108. Mutation: deleting the throw line turns 3 tests red (including the afterAll assertion). Reintroducing agent-seed's late afterAll(() => patchShared(globalThis, "fetch", _savedFetch)) is red with PatchAfterCleanupError thrown at patch-shared.ts:58 — so removing the two late re-patches in agent-seed.test.ts and memory-learn.test.ts is load-bearing, not cosmetic.
• async rejection: I drove the factory directly under the isolated launcher — a post-cleanup patch inside both an awaited and an unawaited async continuation surfaces the throw and leaves the target property unchanged (the write never happens). This case has no committed test; the mechanism is the guard-before-savePatch ordering above.
• nested registry: two registries over one target clean up independently, and a patch fn from an already-cleaned registry refuses while a fresh registry still patches — probed, holds.

Non-blocking observation [1] — packages/cli/test/helpers/patch-shared.ts:33 (bare patchShared): it applies the patch (savePatch(...)) and only then registers a per-call afterAll(restore); this path has no cleanedUp guard. A call that runs after the file's hooks have completed applies the write and queues a restore hook bun will not run. Severity: low (a test-authoring footgun, unchanged by this PR; patchShared is imported directly by several test files). Reachability detail sent privately. If you want it closed: route bare patchShared through the registry guard, or document that it must be called from file scope or a suite hook.

Confirmed load-bearing, outside my focus:

• Reverting discoverCliTestFiles (test/helpers/mock-restore-guard-scan.ts:528) to the old *.test.ts-only walk fails "the scan discovers the forms bun discovers, over the cli package".
• Collapsing resolveScope (scan.ts:208) to the immediate parent fails 22 tests (including module-alias-outer-scope-leaked.fixture.ts).
• Making moduleObjectAt (scan.ts:241) name-based instead of scope-based fails the two shadow-cleared fixtures (module-shadow-param-cleared, module-alias-shadow-cleared).

Not seen / environment: the full cli lane on this host ran 3178 tests across 207 files with 10 failures, all in files this PR does not touch and environment-dependent (three "real nono / Landlock·Seatbelt" sandbox cases, validateSshReachable ssh timeout, and a few secrets/nono config cases) — docker is not running here, which explains the sandbox ones. The four files this PR changes pass (98/0). I did not fetch the CI log.