[jatmn]

Summary

• BashTool.sandboxAnalysis.test.ts now snapshots the real utils/Shell.js and services/analytics/growthbook.js modules (nonce-qualified imports, the same REAL_* pattern as model.test.tsx) and re-registers them for every mocked specifier in afterAll.
• Why: mock.restore() does not undo mock.module, so the stubbed exec (always { code: 0, interrupted: false }) leaked into later files in the same process. Any order that ran this file before BashTool.errorOutput.test.ts made errorOutput fail ("expected ShellError").

Impact

• user-facing impact: none (test-only).
• developer/maintainer impact: BashTool tests are order-independent in a single process, which removing --isolate needs (TA-001).

Testing

• I ran the required local preflight.
• exact commands and results: bun run build ok, bun run typecheck ok, bun run check ok (0 failures in every shard).
• focused tests: bun test --randomize --seed=$s src/tools/BashTool/*.test.ts for seeds 1-20: 20/20 pass with this change; on current main seeds 1, 4, 6, 11 and 13 all fail (10/20 seeds failed overall before the fix). BashTool.errorOutput.test.ts alone: 50/50 pass on main, 20/20 pass while another test run loads the machine, so its sleep 5 abort tests have no remaining timing race.
• documented skipped checks, platform limitations, or verified pre-existing failures: none.

Notes

• provider/model path tested: n/a
• screenshots attached (if UI changed): n/a
• follow-up work or known limitations: none.

Summary by CodeRabbit

• Tests
  • Updated test module loading and mock setup for shell execution and feature-flag behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Twigpine/openclaude
• Branch: test/bashtool-sandboxanalysis-restore-mocks
• File: .coderabbit.yaml
• Commit: 8eadf37
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: 44acec45-a007-471b-ae24-c4567468d081

📥 Commits

Reviewing files that changed from the base of the PR and between c823efe and 8eadf37.

📒 Files selected for processing: 1

• src/tools/BashTool/BashTool.sandboxAnalysis.test.ts

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

📜 Recent review details

⏰ Context from checks skipped due to timeout: 17

• GitHub Check: unit-tests (2)
• GitHub Check: launcher-compatibility (22)
• GitHub Check: unit-tests (6)
• GitHub Check: unit-tests (1)
• GitHub Check: unit-tests (7)
• GitHub Check: unit-tests (4)
• GitHub Check: unit-tests (3)
• GitHub Check: unit-tests (8)
• GitHub Check: unit-tests (5)
• GitHub Check: launcher-compatibility (24.11.x)
• GitHub Check: dedicated-tests
• GitHub Check: smoke-and-tests
• GitHub Check: typecheck
• GitHub Check: launcher-node-floor
• GitHub Check: CodeQL (javascript-typescript)
• GitHub Check: CodeQL (actions)
• GitHub Check: Analyze (javascript-typescript)

🧰 Additional context used

📓 Path-based instructions: 3

Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive.

⚙️ CodeRabbit configuration file

Files:

• src/tools/BashTool/BashTool.sandboxAnalysis.test.ts

Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.

⚙️ CodeRabbit configuration file

Files:

• src/tools/BashTool/BashTool.sandboxAnalysis.test.ts

Apply the OpenClaude maintainer review rubric from AGENTS.md.

⚙️ CodeRabbit configuration file

Files:

• src/tools/BashTool/BashTool.sandboxAnalysis.test.ts

🪛 ast-grep (0.45.3)

src/tools/BashTool/BashTool.sandboxAnalysis.test.ts

[warning] 10-10: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.

(insecure-random-security-token-typescript)

🔇 Additional comments: 1

src/tools/BashTool/BashTool.sandboxAnalysis.test.ts: 1

1-1: LGTM!

Also applies to: 10-25, 124-124, 128-128, 132-132, 136-136, 154-154, 158-158

📝 Walkthrough

The sandbox analysis test file replaces static Shell and Growthbook imports with nonce-qualified dynamic imports. Its mocks spread the loaded modules before applying test overrides, and an afterAll hook restores the real modules under both supported import paths.

Changes: Sandbox analysis test setup

Layer / File(s)	Summary
Dynamic module setup and mocks src/tools/BashTool/BashTool.sandboxAnalysis.test.ts	The test file loads the real Shell and Growthbook modules with nonce-qualified dynamic imports, uses them as the base for mocks, and restores them in afterAll.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: chioarub

Merge Risk: ⚪ Minimal · up to 8eadf

The test-only module restoration change presents no identified merge-blocking risk.

Pre-merge checks: 6 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 6 passed

Check name	Status	Explanation
Title check		The title is concise, scoped to BashTool tests, and accurately describes restoring Shell and GrowthBook module mocks.
Description check		The description includes all required sections, explains the cause and impact, and records comprehensive validation results.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Risk Surface Disclosed		The pull request changes only src/tools/BashTool/BashTool.sandboxAnalysis.test.ts. The diff updates test module mocking and cleanup for Shell and GrowthBook. It does not touch authentication, provid…
No Hidden Policy Change		PASS. The pull request changes only src/tools/BashTool/BashTool.sandboxAnalysis.test.ts. The added code snapshots real Shell and GrowthBook modules, restores mocked specifiers in afterAll, and…

Full details: Risk Surface Disclosed

Explanation

The pull request changes only src/tools/BashTool/BashTool.sandboxAnalysis.test.ts. The diff updates test module mocking and cleanup for Shell and GrowthBook. It does not touch authentication, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts. No risk-surface disclosure is required.

Full details: No Hidden Policy Change

Explanation

PASS. The pull request changes only src/tools/BashTool/BashTool.sandboxAnalysis.test.ts. The added code snapshots real Shell and GrowthBook modules, restores mocked specifiers in afterAll, and preserves existing test-only overrides for exec and getFeatureValue_CACHED_MAY_BE_STALE. No product behavior, trust model, routing default, telemetry/network behavior, or permission policy changes are present.

✨ 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

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

[jatmn]

— jatmn's PR Review agent

🟢 Thanks for the contribution. I do not see any actionable issues from my review.

Merge readiness

Merge-base decision is live-base-three-dot on main at c823efe56ae380e5c479855da22300d829451d48 through head 8eadf37b2cc65df5d372c0e8e7f20b67aa63b6bb. The branch is not behind main (behind_default_count 0). GitHub reports mergeable with mergeable_state blocked only while required checks were finishing; on the pinned head, PR Checks (smoke-and-tests, unit-tests shards 1–8, dedicated-tests, typecheck, launcher jobs, pr-intent-scan) and CodeQL (aggregate, javascript-typescript, actions) all completed successfully.

This is a surgical TA-001 test-hygiene slice: one file, no dependency on other open PRs, and scripts/run-tests.ts still passes --isolate on head. The PR body correctly describes a prerequisite for later isolate removal, not TA-001 completion.

Findings

No verified defects in the review range.