[jatmn]

Summary

Fixes order-dependent failures seen in the serial single-process run (bun run scripts/run-tests.ts --randomize --seed=101 --max-concurrency=1, no shards). Each test passed alone.

• "unknown openai-compatible model fallback logs one debug warning and no console errors" (src/utils/context.test.ts) and "suspicious terminal columns can be flagged again after returning to normal width":
  • Cause: LSPDiagnosticRegistry.test.ts and attachments.lspDiagnostics.test.ts mocked debug.js (and slowOperations.js / LSPDiagnosticRegistry.js) without restoring them. Later files' spies inherited the earlier warning calls.
  • Fix: both files now snapshot the real exports before mock.module and restore them in afterAll. slowOperations.js keeps its real exports and overrides only jsonStringify.
• "records the panel stop source and causal input before aborting an agent" (teammateViewHelpers.interruptionTrace.test.ts):
  • Cause: trace entries left behind by claude.lifecycle.test.ts made the test pick up an older claude_stream_cleanup abort.
  • Fix: the victim test now resets trace state in its existing locked beforeEach and uses snapshotEnv, the same pattern as test: reset teammate interruption trace state #2350. claude.lifecycle.test.ts is not touched, because resetting in its own afterEach/afterAll crashes Bun under --isolate.
• "third-party provider branch opens the first-run provider manager": this one is already fixed on main by test: restore real ColorPicker and CustomSelect exports after wizard step tests (TA-001 round 2) #2340, which restores the leaked CustomSelect stub. I confirmed it reproduces without that restore, so this PR makes no change for it.
• attachments.lspDiagnostics.test.ts now also uses snapshotEnv for its env handling.
• Test-only. Assertions and production code are unchanged, and no tests are skipped.

Impact

• user-facing impact: none (test-only).
• developer/maintainer impact: the default serial randomized run (no feature flags) is clean for the seeds below.

Testing

• I ran the required local preflight.
• exact commands and results: bun run build, bun run check, bun run typecheck, bun run typecheck:type-tests, bun run lint (0 errors) and bun run security:pr-scan pass; git status clean.
• serial runs: bun run scripts/run-tests.ts --randomize --seed=<S> --max-concurrency=1 (no shards, no --feature flags) for seeds 101, 4242 and 2027: no failures. This claim covers only that command.
• eight-shard randomized runs for seeds 101, 4242 and 2027: no failures.
• the CI --isolate commands for the touched shards (1 and 4): pass, with no crash.
• documented skipped checks, platform limitations, or verified pre-existing failures: none.

Notes

• provider/model path tested: n/a (test-only).
• screenshots attached (if UI changed): n/a.
• follow-up work or known limitations: the same serial run with --feature=UNATTENDED_RETRY still has order-dependent failures (for example seeds 4242 and 2027). They also occur on main, are not caused or fixed by this PR, and are being fixed in a separate PR.

Summary by CodeRabbit

• Tests
  • Updated diagnostic, interruption-trace, and attachment-related test setup and cleanup to better restore environment and module state between tests.
  • Added consistent trace resets and cleanup of shared test state, helping prevent changes in one test from affecting others.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Twigpine/openclaude
• Branch: test/serial-run-order-leaks
• File: .coderabbit.yaml
• Commit: 09bdb3c
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Essentials
• Run ID: 45b1d0a8-a02f-4b0e-8caf-597628db385c

📥 Commits

Reviewing files that changed from the base of the PR and between 40297b4 and 09bdb3c.

📒 Files selected for processing: 3

• src/services/lsp/LSPDiagnosticRegistry.test.ts
• src/state/teammateViewHelpers.interruptionTrace.test.ts
• src/utils/attachments.lspDiagnostics.test.ts

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

📜 Recent review details

⏰ Context from checks skipped due to timeout: 17

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

🧰 Additional context used

📚 Code guidelines: 1

AGENTS.md — configured

📓 Path-based instructions: 4

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/utils/attachments.lspDiagnostics.test.ts
• src/state/teammateViewHelpers.interruptionTrace.test.ts
• src/services/lsp/LSPDiagnosticRegistry.test.ts

Apply the OpenClaude maintainer review rubric from AGENTS.md.

⚙️ CodeRabbit configuration file

Files:

• src/utils/attachments.lspDiagnostics.test.ts
• src/state/teammateViewHelpers.interruptionTrace.test.ts
• src/services/lsp/LSPDiagnosticRegistry.test.ts

Source excerpt: Keep changes focused on one problem.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• src/utils/attachments.lspDiagnostics.test.ts
• src/state/teammateViewHelpers.interruptionTrace.test.ts
• src/services/lsp/LSPDiagnosticRegistry.test.ts

Source excerpt: No hidden policy change: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup.

📄 CodeRabbit inference engine (Custom checks)

Files:

• src/utils/attachments.lspDiagnostics.test.ts
• src/state/teammateViewHelpers.interruptionTrace.test.ts
• src/services/lsp/LSPDiagnosticRegistry.test.ts

🧠 Learnings: 1

📚 Learning: 2026-10-22T17:13:07.516Z

Learnt from: CR
Repo: Twigpine/openclaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-10-22T17:13:07.516Z
Learning: Applies to ** : Source excerpt:
# AGENTS.md - AI Agent Coding Guide

## Stack And Conventions

- TypeScript with strict mode and ESM imports.

Applied to files:

• src/state/teammateViewHelpers.interruptionTrace.test.ts

🔇 Additional comments: 3

src/services/lsp/LSPDiagnosticRegistry.test.ts: 1

1-1: LGTM!

Also applies to: 6-16, 27-27

src/utils/attachments.lspDiagnostics.test.ts: 1

1-2: LGTM!

Also applies to: 9-19, 57-57, 145-148, 160-160

src/state/teammateViewHelpers.interruptionTrace.test.ts: 1

2-2: LGTM!

Also applies to: 16-16, 22-23, 29-29

📝 Walkthrough

The tests now restore mocked modules and environment variables, and reset shared interruption-trace state. The changes cover LSP diagnostics, attachment diagnostics, and teammate view interruption-trace tests.

Changes: Test isolation

Layer / File(s)	Summary
Restore mocked modules src/services/lsp/LSPDiagnosticRegistry.test.ts, src/utils/attachments.lspDiagnostics.test.ts	The tests preserve real module exports when mocking and restore the debug, slow-operations, and LSP registry modules after testing.
Restore environment and trace state src/state/teammateViewHelpers.interruptionTrace.test.ts, src/utils/attachments.lspDiagnostics.test.ts	The tests use snapshotEnv to restore environment variables. The interruption-trace test resets trace state before and after each test.

Priority: ⬇️ Low

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

Change: Bug fix

Possibly related PRs

• Twigpine/openclaude#2342: Restores cache-busted real exports in tests with leaked mock.module registrations.
• Twigpine/openclaude#2345: Resets shared interruption-trace state in another test.
• Twigpine/openclaude#2346: Updates the runner and documentation for shared-process test runs without --isolate.

Suggested reviewers: chioarub

Merge Risk: ⚪ Minimal · up to 09bdb

The test-isolation changes are mergeable after normal checks; no actionable issue was identified.

Pre-merge checks: 7

Check name	Status	Explanation
Title check		The title is concise, scoped to tests, and accurately describes restoring diagnostic mocks and resetting trace state.
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
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		PASS — The pull request changes only three test files. The diff restores test mocks, resets interruption-trace state, and snapshots test environment variables. It does not touch auth, provider routing…
No Hidden Policy Change		The PR changes only three .test.ts files. The diff adds mock restoration, environment snapshots, and interruption-trace reset logic. No product, trust-model, routing-default, telemetry/network, or p…
Description check		The pull request description follows the required template. It explains the changes and reasons, documents user-facing and maintainer impact, lists preflight and focused test results, identifies no sk…

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files.

Full details: Risk Surface Disclosed

Explanation

PASS — The pull request changes only three test files. The diff restores test mocks, resets interruption-trace state, and snapshots test environment variables. It does not touch auth, 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

The PR changes only three .test.ts files. The diff adds mock restoration, environment snapshots, and interruption-trace reset logic. No product, trust-model, routing-default, telemetry/network, or permission-policy behavior changes are present.

Full details: Description check

Explanation

The pull request description follows the required template. It explains the changes and reasons, documents user-facing and maintainer impact, lists preflight and focused test results, identifies no skipped checks, and records notes and known limitations.

✨ Finishing Touches

📝 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

Merge readiness

• Draft PR: GitHub still marks this pull request as a draft (mergeable_state: blocked until marked ready).
• Live main drift: behind_default_count is 1 (live base 40297b49792f4b4cbde34d9b3b08d56fcf0e92c4 vs three-dot merge-base 7141ba29f3b85769491da65ab42567a1cee1d1d1). Rebase onto current main before merge so CI and serial-order fixes run on the latest tree.
• Merge-base decision: live-base-three-dot reviewing 7141ba29..09bdb3c8 (single commit 09bdb3c8).
• Required CI on head 09bdb3c87f344d0a4cf166e2e80218045532b0be: all completed check runs success (including unit-tests (1–8), typecheck, smoke-and-tests, dedicated-tests, CodeQL, CodeQL (javascript-typescript), pr-intent-scan). CodeRabbit commit status success (“Review completed”).
• Validation I ran (review-owned checkout, TMPDIR under run runtime/tmp):
  • Serial command (single process, no shards): bun run scripts/run-tests.ts --randomize --seed=<seed> --max-concurrency=1 --feature=UNATTENDED_RETRY.
  • On merge-base 7141ba29: seed 101 exit 0; seed 4242 exit 1 (4 failures); seed 2027 exit 1 (76 failures).
  • On head 09bdb3c8 (same command, --shard-timeout-ms=1200000): seed 101 exit 0; seed 4242 exit 1 (4 failures — getAgentModel provider-aware fallback haiku alias cases, not the three files in this diff); seed 2027 exit 1 (76 failures).
  • Targeted repro on merge-base: bun test --max-concurrency=1 on LSPDiagnosticRegistry.test.ts → attachments.lspDiagnostics.test.ts → context.test.ts → 1 fail (unknown openai-compatible model fallback logs one debug warning and no console errors); same chain on head → 0 fail.
  • CI shard command (3× each, shards 1 and 4 per author): bun run scripts/run-tests.ts --shards=8 --shard-index=<K> --feature=UNATTENDED_RETRY --max-concurrency=1 → 9/9 exit 0, no exit 132. (Workflow does not pass --isolate; CI isolates via per-shard child processes in run-tests.ts.)
  • PR testing bullets vs full serial: the body claims seeds 4242 and 2027 are clean on head; full serial runs above still fail with the same failure counts as merge-base for those seeds. The in-scope leak class this PR targets is verified via the targeted chain and seed 101; consider narrowing the PR testing section so it does not claim whole-serial cleanliness for seeds that still surface unrelated order-dependent failures.

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