Repository navigation
feat(cli): add local background sessions - #1642
Conversation
Add local detached background sessions backed by an OpenClaude-owned registry under the resolved config directory. - implement --bg spawning plus ps, logs, logs -f, kill, and an explicit attach limitation - harden registry metadata validation, atomic writes, ID/name collision handling, and terminal-name reuse - precreate child log files with precise ownership cleanup and register metadata only after spawn succeeds - verify live PIDs against the session command before treating registry entries as running - wait for process-tree termination and escalate to SIGKILL before marking sessions killed - skip live local background sessions during --continue transcript selection - preserve Node heap flags for detached children while avoiding stale launcher relaunch state - handle -- separators so dash-prefixed prompts remain positional - document storage, safety model, name reuse, and the current attach limitation Validation: - bun test - bun run typecheck - bun run smoke - isolated built-CLI --bg/ps/logs/kill smoke - CodeRabbit review findings addressed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughImplements the complete ChangesBackground Sessions Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Restore complete bg registry and UDS module mocks after conversation recovery tests so Bun's process-global mock.module registry cannot leak partial module exports into later CLI tests. CI exposed this under Bun 1.3.13 when conversationRecovery.test ran before the bgRegistry and bg CLI test files.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/bg.ts`:
- Around line 370-380: Add a clarifying comment in the treeKillAsync function
that explains the error-suppression logic in the callback. Specifically,
document the pattern where the error is only rejected if both the error exists
AND the process is still running (checked via isProcessRunning), explaining that
this handles the race condition where the process may already be terminated by
the time treeKill attempts to kill it, making the error safe to ignore.
In `@src/cli/bgRegistry.ts`:
- Around line 175-183: The pid validation in the isValidRegistryEntry check does
not verify that the process ID is strictly positive. Add a check to ensure
candidate.pid is greater than 0 alongside the existing checks for typeof
candidate.pid === 'number' and Number.isInteger(candidate.pid). Apply this same
validation to the create-input boundary code mentioned in the comment, wherever
pid values are accepted or validated, to prevent non-positive PIDs that could
cause unintended process targeting in termination flows.
- Around line 217-275: The session name availability check in
`assertBackgroundSessionNameAvailable` is separated from the actual session
write in `createBackgroundSession` (at the `writeNewSession` call), creating a
race condition where two concurrent calls can both pass the availability check
and then both write sessions with the same name. To fix this, make the name
reservation atomic by either handling naming conflicts at the point of
persistence in `writeNewSession` with proper error detection and retry logic, or
by deferring the availability check until immediately before the atomic write
operation to minimize the window for concurrent duplicates.
In `@src/entrypoints/cli.tsx`:
- Around line 161-188: Split the bg-management dispatch into two separate code
paths based on command type. Move the `ps|logs|attach|kill` command handling to
an earlier location in the CLI entry point (before provider/config/profile
setup) by creating a separate fast-path condition that only checks for these
four command names. Keep the `--bg/--background` flag dispatch at the current
location or later since it requires environment preparation for child process
spawning. Update the condition at the current location to only check for the
presence of the `--bg` or `--background` flags, removing the checks for the four
command names from this path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7d2d023-3b22-49b9-9313-5ef9405e0e15
📒 Files selected for processing (10)
README.mdscripts/build.tssrc/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/utils/conversationRecovery.test.tssrc/utils/conversationRecovery.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/entrypoints/cli.test.tssrc/utils/conversationRecovery.test.tsREADME.mdsrc/cli/bg.test.tssrc/cli/bgRegistry.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.tssrc/utils/conversationRecovery.tssrc/cli/bgRegistry.tsscripts/build.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/entrypoints/cli.test.tssrc/utils/conversationRecovery.test.tssrc/cli/bg.test.tssrc/cli/bgRegistry.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.tssrc/utils/conversationRecovery.tssrc/cli/bgRegistry.tsscripts/build.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/entrypoints/cli.test.tssrc/utils/conversationRecovery.test.tsREADME.mdsrc/cli/bg.test.tssrc/cli/bgRegistry.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.tssrc/utils/conversationRecovery.tssrc/cli/bgRegistry.tsscripts/build.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/entrypoints/cli.test.tssrc/entrypoints/cli.tsxscripts/build.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/entrypoints/cli.test.tssrc/utils/conversationRecovery.test.tssrc/cli/bg.test.tssrc/cli/bgRegistry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/entrypoints/cli.test.tssrc/utils/conversationRecovery.test.tsREADME.mdsrc/cli/bg.test.tssrc/cli/bgRegistry.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.tssrc/utils/conversationRecovery.tssrc/cli/bgRegistry.tsscripts/build.ts
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update docs when setup, commands, or user-facing behavior changes
Files:
README.md
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
README.md
🪛 GitHub Actions: PR Checks / 2_smoke-and-tests.txt
src/cli/bgRegistry.test.ts
[error] 2-2: Test runner failed with SyntaxError: Export named 'resolveBackgroundSession' not found in module '/home/runner/work/openclaude/openclaude/src/cli/bgRegistry.ts'. (Unhandled error between tests)
src/cli/bgRegistry.ts
[error] 1-1: Module export mismatch: 'resolveBackgroundSession' is imported/exported incorrectly. Named export 'resolveBackgroundSession' is missing.
🪛 GitHub Actions: PR Checks / smoke-and-tests
src/cli/bgRegistry.test.ts
[error] 1-2: Unhandled error between tests. SyntaxError: Export named 'resolveBackgroundSession' not found in module '/home/runner/work/openclaude/openclaude/src/cli/bgRegistry.ts'.
🔇 Additional comments (26)
src/cli/bg.ts (15)
1-20: LGTM!
21-52: LGTM!
54-100: LGTM!
102-134: LGTM!
136-166: LGTM!
168-225: LGTM!
227-268: LGTM!
270-316: LGTM!
318-359: LGTM!
361-368: LGTM!
382-445: LGTM!
447-471: LGTM!
473-485: LGTM!
487-505: LGTM!
507-608: LGTM!src/cli/bg.test.ts (4)
1-34: LGTM!
36-78: LGTM!
80-107: LGTM!
109-129: LGTM!src/cli/bgRegistry.test.ts (1)
27-441: LGTM!src/utils/conversationRecovery.ts (1)
361-391: LGTM!Also applies to: 641-642
src/utils/conversationRecovery.test.ts (1)
215-241: LGTM!src/cli/bgRegistry.ts (1)
286-311: No issue here. TheresolveBackgroundSessionexport is present at line 286, and the TypeScript configuration (moduleResolution: "bundler"withallowImportingTsExtensions: true) correctly resolves imports of.tsfiles via.jsextensions. The test file already imports this function successfully. This does not block merge.src/entrypoints/cli.test.ts (1)
89-99: LGTM!scripts/build.ts (1)
34-35: LGTM!Also applies to: 176-177
README.md (1)
103-126: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/conversationRecovery.test.ts`:
- Around line 97-98: Add an inline comment above the mock.module calls (for
'../cli/bgRegistry.js' and './udsClient.js') to explain the Bun 1.3.13-specific
behavior being addressed. The comment should clarify that this explicit module
restoration is necessary to prevent partial module exports from leaking into
subsequent test files, which is a known issue in Bun 1.3.13 that requires this
workaround pattern after calling mock.restore().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 619e7a63-0a38-47a3-ad99-9a02512dc414
📒 Files selected for processing (1)
src/utils/conversationRecovery.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/conversationRecovery.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/utils/conversationRecovery.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/conversationRecovery.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/conversationRecovery.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/conversationRecovery.test.ts
🔇 Additional comments (2)
src/utils/conversationRecovery.test.ts (2)
9-10: LGTM!
219-247: LGTM!
Replace the conversation recovery bgRegistry module mock with real registry metadata backed by a short-lived live child process. This keeps UDS as the only mocked boundary and avoids leaking a mocked registry module into later CLI registry tests under Bun 1.3.13.
Stop the conversation recovery test from using process-wide bgRegistry mocks or real child processes by injecting the live-session dependencies directly. Pin and serialize the bg registry test config directory through the shared env mutation lock so path/cache state cannot leak from neighboring tests under Bun CI ordering.
Explain why conversation recovery tests re-register full module exports after mock.restore(), matching the CodeRabbit-requested Bun 1.3.13 isolation workaround.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/conversationRecovery.test.ts`:
- Around line 217-240: The test for collectLiveBackgroundSessionIds only covers
the scenario where UDS returns empty but the registry provides data. Add two
additional test cases to cover the failure resilience contract: one where
listAllLiveSessions throws an error but refreshBackgroundSessionStatuses still
returns valid session data (testing that the function falls back to registry
data), and another where refreshBackgroundSessionStatuses throws an error but
listAllLiveSessions still returns valid session data (testing that the function
falls back to UDS data). These tests ensure the aggregator continues functioning
when either the UDS or registry endpoints fail independently, properly
demonstrating the fault-tolerance behavior of the
collectLiveBackgroundSessionIds function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad0518b5-e3f0-4d3d-b82e-58147a7fb929
📒 Files selected for processing (3)
src/cli/bgRegistry.test.tssrc/utils/conversationRecovery.test.tssrc/utils/conversationRecovery.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/utils/conversationRecovery.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/utils/conversationRecovery.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/utils/conversationRecovery.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/utils/conversationRecovery.ts
🔇 Additional comments (2)
src/cli/bgRegistry.test.ts (1)
5-12: LGTM!Also applies to: 23-33, 37-51
src/utils/conversationRecovery.ts (1)
361-409: LGTM!
Avoid relying on process-wide CLAUDE_CONFIG_DIR state in bgRegistry tests. Use a registry-local test root override so CI file ordering and mocked path modules cannot redirect background session metadata into another test's temp directory.
Add focused coverage for collectLiveBackgroundSessionIds when UDS discovery fails but registry data remains available, and when registry refresh fails but UDS data remains available.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/bgRegistry.test.ts (1)
105-125: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a concurrent duplicate-name regression test (not just sequential collision).
This suite currently proves sequential rejection, but not the concurrent same-name create race. Add a focused test that starts two
createBackgroundSessioncalls concurrently for the same name and asserts at most one live session is created.As per coding guidelines, “Review tests for meaningful coverage of the changed behavior … Block when risky runtime changes lack focused regression coverage…”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/bgRegistry.test.ts` around lines 105 - 125, The existing test in the 'rejects duplicate names and reports ambiguous names' test case only validates sequential rejection (first session created, then second rejected). Add a new test case that validates concurrent duplicate-name handling by starting two createBackgroundSession calls with the same name simultaneously (using Promise.all or similar concurrent execution pattern) and asserting that at most one session is created and persisted, ensuring the race condition is properly handled and covered by regression tests.Source: Coding guidelines
♻️ Duplicate comments (3)
src/utils/conversationRecovery.test.ts (1)
219-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd fallback-failure regression cases for the resilience contract.
This test covers the "UDS empty + registry provides data" path, but the function's independent try/catch blocks (lines 361-409 in conversationRecovery.ts) are designed to tolerate failures from either source. Missing test cases:
- UDS throws → registry still contributes
- Registry throws → UDS still contributes
These scenarios are part of the advertised contract (PR objectives: "independent try/catch fallbacks") and need regression coverage before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/conversationRecovery.test.ts` around lines 219 - 242, The test for collectLiveBackgroundSessionIds currently only covers the case where UDS is empty and registry provides data, but the function implements independent try/catch blocks (referenced in conversationRecovery.ts) designed to handle failures from either source independently. Add two additional test cases to ensure the resilience contract is properly covered: one test where listAllLiveSessions throws an error but refreshBackgroundSessionStatuses still provides valid session data, and another test where refreshBackgroundSessionStatuses throws an error but listAllLiveSessions still provides valid session data. Each test should verify that the function successfully returns the contributing session even when the other source fails.src/cli/bgRegistry.ts (2)
185-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate PID as strictly positive at both registry-read and create boundaries.
Line 189/190 accepts
pid <= 0, and Line 250 persists it. That permits unsafe signal targets in downstream kill flows. Reject non-positive PIDs in bothisBackgroundSessionandcreateBackgroundSession.Suggested fix
function isBackgroundSession( value: unknown, expectedId: string, ): value is BackgroundSession { @@ typeof candidate.pid === 'number' && Number.isInteger(candidate.pid) && + candidate.pid > 0 && @@ export async function createBackgroundSession( input: CreateBackgroundSessionInput, ): Promise<BackgroundSession> { + if (!Number.isInteger(input.pid) || input.pid <= 0) { + throw new Error(`Invalid background session pid: ${input.pid}`) + } await assertBackgroundSessionNameAvailable(input.name)Also applies to: 241-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/bgRegistry.ts` around lines 185 - 191, The code currently allows non-positive PIDs (zero or negative) which is unsafe for signal operations. In the `isBackgroundSession` function, add a validation to ensure the PID is strictly positive (greater than zero) alongside the existing `Number.isInteger(candidate.pid)` check. Additionally, in the `createBackgroundSession` function around lines 241-251 where the session is persisted, validate that the provided PID is strictly positive before storing it. Both locations must enforce that PID > 0 to prevent unsafe signal targeting in downstream kill flows.
227-245:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake session-name uniqueness atomic at write time.
Line 244 checks availability, but Line 284 performs persistence later. Two concurrent creates with the same name can both pass the check and register duplicate live names.
Also applies to: 284-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/bgRegistry.ts` around lines 227 - 245, The assertBackgroundSessionNameAvailable function checks for name uniqueness before session creation, but the actual persistence happens later in createBackgroundSession, creating a race condition where two concurrent requests can pass the availability check and both persist sessions with the same name. Fix this by moving the uniqueness check into the persistence layer (where the session is actually written) so that the check and write operations are atomic, ensuring no duplicate names can be registered even under concurrent creates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cli/bgRegistry.test.ts`:
- Around line 105-125: The existing test in the 'rejects duplicate names and
reports ambiguous names' test case only validates sequential rejection (first
session created, then second rejected). Add a new test case that validates
concurrent duplicate-name handling by starting two createBackgroundSession calls
with the same name simultaneously (using Promise.all or similar concurrent
execution pattern) and asserting that at most one session is created and
persisted, ensuring the race condition is properly handled and covered by
regression tests.
---
Duplicate comments:
In `@src/cli/bgRegistry.ts`:
- Around line 185-191: The code currently allows non-positive PIDs (zero or
negative) which is unsafe for signal operations. In the `isBackgroundSession`
function, add a validation to ensure the PID is strictly positive (greater than
zero) alongside the existing `Number.isInteger(candidate.pid)` check.
Additionally, in the `createBackgroundSession` function around lines 241-251
where the session is persisted, validate that the provided PID is strictly
positive before storing it. Both locations must enforce that PID > 0 to prevent
unsafe signal targeting in downstream kill flows.
- Around line 227-245: The assertBackgroundSessionNameAvailable function checks
for name uniqueness before session creation, but the actual persistence happens
later in createBackgroundSession, creating a race condition where two concurrent
requests can pass the availability check and both persist sessions with the same
name. Fix this by moving the uniqueness check into the persistence layer (where
the session is actually written) so that the check and write operations are
atomic, ensuring no duplicate names can be registered even under concurrent
creates.
In `@src/utils/conversationRecovery.test.ts`:
- Around line 219-242: The test for collectLiveBackgroundSessionIds currently
only covers the case where UDS is empty and registry provides data, but the
function implements independent try/catch blocks (referenced in
conversationRecovery.ts) designed to handle failures from either source
independently. Add two additional test cases to ensure the resilience contract
is properly covered: one test where listAllLiveSessions throws an error but
refreshBackgroundSessionStatuses still provides valid session data, and another
test where refreshBackgroundSessionStatuses throws an error but
listAllLiveSessions still provides valid session data. Each test should verify
that the function successfully returns the contributing session even when the
other source fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c18b6cdd-0125-401c-ada1-6545a4f8a9dc
📒 Files selected for processing (3)
src/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/utils/conversationRecovery.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/conversationRecovery.test.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
🔇 Additional comments (3)
src/utils/conversationRecovery.test.ts (1)
96-99: LGTM!src/cli/bgRegistry.ts (1)
68-74: LGTM!Also applies to: 77-79
src/cli/bgRegistry.test.ts (1)
6-7: LGTM!Also applies to: 19-24
Validate persisted and newly-created background session PIDs before exposing them to management commands. Reserve named live sessions with an atomic registry write, release reservations when sessions become terminal, and cover concurrent duplicate-name attempts. Split local session management dispatch from background spawning so ps/logs/attach/kill avoid provider startup while --bg still inherits profile routing.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve the prompt when
--bgis combined with--debug
src/cli/bg.ts:57
The background parser treats every option inOPTION_VALUE_FLAGSas requiring the next token, but the root CLI defines--debug [filter]as an optional-value flag. As a result,openclaude --bg --debug "fix tests"consumes"fix tests"as the debug filter, leavesparsed.promptempty, and exits with the background usage error instead of starting a background print run. The same foreground command reaches normal startup, andopenclaude --bg --debug -- "fix tests"works only because the delimiter forces the prompt path. Please handle optional flags like--debugwithout swallowing the prompt. -
[P3] Ignore orphaned name reservations when no live session owns them
src/cli/bgRegistry.ts:224
Name reservations are written as separatebg-sessions/names/<hash>.jsonfiles and are treated as authoritative even when the referenced session metadata does not exist. If the parent process is interrupted after reserving a name but before writingsessions/<id>.json, or the metadata is later removed/corrupted, futureopenclaude --bg --name <name> ...attempts fail withalready exists, whileopenclaude pshas no session to show or clear. Please reconcile reservations with the session metadata, or release/ignore reservations whose owner session is missing or terminal.
Preserve positional prompts when --bg is combined with optional-value flags such as --debug. Recover stale name reservations whose owner metadata is missing or terminal while preserving in-flight reservations from live creators. Cover both reviewer findings with focused parser and registry regression tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/bg.ts (1)
57-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--from-prand--resumeappear in both flag sets, breaking prompt parsing.Lines 72 and 86 place
--from-prand--resumeinREQUIRED_OPTION_VALUE_FLAGS, but lines 102-103 also add them toOPTIONAL_OPTION_VALUE_FLAGS. Since theREQUIREDcheck infindPromptIndex(line 183) fires first, theOPTIONALentries are dead code. This causes the parser to consume the next token as the flag's value:openclaude --bg --from-pr "fix the bug" # Expected prompt: "fix the bug" # Actual: parser treats "fix the bug" as --from-pr's value, no prompt foundPer the library context, these flags should accept only inline values (
--from-pr=123), so they should be removed fromREQUIRED_OPTION_VALUE_FLAGS.Proposed fix
const REQUIRED_OPTION_VALUE_FLAGS = new Set([ '--add-dir', '--agent', '--agents', '--allowed-tools', '--allowedTools', '--append-system-prompt', '--append-system-prompt-file', '--betas', '--debug-file', '--disallowed-tools', '--disallowedTools', '--effort', '--fallback-model', '--file', - '--from-pr', '--input-format', '--json-schema', '--max-budget-usd', '--max-turns', '--mcp-config', '--model', '--name', '--output-format', '--permission-mode', '--permission-prompt-tool', '--plugin-dir', '--prefill', '--provider', - '--resume', '--resume-session-at', '--rewind-files', '--session-id', '--setting-sources', '--settings', '--system-prompt', '--system-prompt-file', '--task-budget', '--tools', '--workload', '-n', ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/bg.ts` around lines 57 - 106, The flags `--from-pr` and `--resume` are duplicated across both REQUIRED_OPTION_VALUE_FLAGS and OPTIONAL_OPTION_VALUE_FLAGS sets. Since the REQUIRED check fires first in the parsing logic, the OPTIONAL entries become unreachable dead code, causing these flags to incorrectly consume the next token as their value. Remove both `--from-pr` and `--resume` from the REQUIRED_OPTION_VALUE_FLAGS set, keeping them only in OPTIONAL_OPTION_VALUE_FLAGS, since these flags should accept only inline values like `--from-pr=value` and not positional arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cli/bg.ts`:
- Around line 57-106: The flags `--from-pr` and `--resume` are duplicated across
both REQUIRED_OPTION_VALUE_FLAGS and OPTIONAL_OPTION_VALUE_FLAGS sets. Since the
REQUIRED check fires first in the parsing logic, the OPTIONAL entries become
unreachable dead code, causing these flags to incorrectly consume the next token
as their value. Remove both `--from-pr` and `--resume` from the
REQUIRED_OPTION_VALUE_FLAGS set, keeping them only in
OPTIONAL_OPTION_VALUE_FLAGS, since these flags should accept only inline values
like `--from-pr=value` and not positional arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 556a9584-8e77-4c96-902c-3f17dd3cbd9c
📒 Files selected for processing (4)
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.tssrc/cli/bgRegistry.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.tssrc/cli/bgRegistry.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.tssrc/cli/bgRegistry.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/cli/bg.test.tssrc/cli/bgRegistry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.tssrc/cli/bgRegistry.test.ts
🔇 Additional comments (16)
src/cli/bg.ts (1)
1-52: LGTM!Also applies to: 108-622
src/cli/bg.test.ts (1)
1-155: LGTM!src/cli/bgRegistry.ts (8)
19-75: LGTM!
77-116: LGTM!
118-150: LGTM!
152-183: LGTM!
185-303: LGTM!
305-371: LGTM!
373-434: LGTM!
436-534: LGTM!src/cli/bgRegistry.test.ts (6)
1-46: LGTM!
48-124: LGTM!
126-266: LGTM!
268-336: LGTM!
338-478: LGTM!
480-629: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.
Findings
- [P2] Ignore flags that appear after
--when detecting bg/print mode
src/cli/bg.ts:219
The background parser still scans the full argument list for mode flags, including positional prompt text after the--delimiter. For example,openclaude --bg -- --printoropenclaude --bg -- -pis supposed to run a background print-mode child with that literal prompt, buthasPrintMode()sees the positional token and skips injecting the real--print; the child is then spawned asopenclaude -- --print, which starts the interactive CLI detached from the terminal instead of a non-interactive background run. The entrypoint has the same delimiter issue for dispatch (src/entrypoints/cli.tsx:189usesargs.includes('--bg')/--background), soopenclaude -- --bgis routed into the background path even though--bgis only prompt text. Please make the bg/print/background flag checks delimiter-aware and add regression coverage for flag-looking prompts after--.
Limit background and print-mode flag detection to arguments before the -- delimiter so flag-shaped prompts remain positional. Keep optional resume/from-pr flags out of the required-value table and add regressions for delimiter and optional-flag prompt handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/entrypoints/cli.tsx`:
- Around line 70-73: The function argsBeforeDelimiter is duplicated between
src/entrypoints/cli.tsx (lines 70-73) and src/cli/bg.ts (lines 166-169). While
this duplication is currently acceptable due to the lazy-load architecture where
cli.tsx minimizes imports for fast paths, if maintenance concerns arise in the
future, extract this helper function to a small shared module that both files
can import without pulling in the full dependency graph of bg.js. This would
eliminate the duplication while preserving the performance benefits of the
current lazy-loading approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a716d5a-8b7d-4ba0-99ea-e33a17b58309
📒 Files selected for processing (4)
src/cli/bg.test.tssrc/cli/bg.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/cli/bg.test.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/cli/bg.test.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/cli/bg.test.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/cli/bg.test.tssrc/entrypoints/cli.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/cli/bg.test.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/entrypoints/cli.test.tssrc/entrypoints/cli.tsx
🔇 Additional comments (5)
src/entrypoints/cli.tsx (2)
89-110: LGTM!
194-206: LGTM!src/entrypoints/cli.test.ts (1)
89-119: LGTM!Also applies to: 121-138
src/cli/bg.ts (1)
218-219: LGTM!src/cli/bg.test.ts (1)
79-109: LGTM!Also applies to: 118-126
Move args-before-delimiter handling into the existing dependency-free CLI args utility. Use a dynamic import from the entrypoint so background flag routing shares the helper without adding top-level module load to version and management fast paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/entrypoints/cli.test.ts`:
- Around line 121-142: Replace this test that validates source-code string
layout with execution-level tests that actually call the main() function with
controlled arguments and mocks. Remove the string-search assertions for
optionArgs, helperImportIndex, bgFlagIndex, and backgroundFlagIndex. Instead,
create tests that execute main() with various argument combinations (ps, logs,
attach, kill with and without --bg flag, and -- --bg), mock the necessary
dependencies and handlers, and assert which handlers actually run and which
startup steps are skipped based on the argument parsing. This ensures the test
validates real runtime behavior and the delegation of background spawn flags to
argsBeforeDelimiter, rather than just verifying code layout in the source file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d49507c0-6fb9-4da1-b399-33f5853936cd
📒 Files selected for processing (4)
src/cli/bg.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/utils/cliArgs.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/cliArgs.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/utils/cliArgs.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/cliArgs.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/cliArgs.tssrc/entrypoints/cli.test.tssrc/entrypoints/cli.tsxsrc/cli/bg.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/entrypoints/cli.test.tssrc/entrypoints/cli.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/entrypoints/cli.test.ts
🔇 Additional comments (13)
src/utils/cliArgs.ts (1)
31-34: LGTM!src/entrypoints/cli.tsx (1)
189-200: LGTM!src/cli/bg.ts (11)
1-53: LGTM!
54-106: LGTM!
107-139: LGTM!
141-211: LGTM!
213-275: LGTM!
277-313: LGTM!
315-366: LGTM!
368-454: LGTM!
456-494: LGTM!
496-514: LGTM!
516-617: LGTM!
Export the CLI entrypoint for controlled tests and add isolated importer injection so runtime routing tests do not leak global module mocks. Replace the delimiter source-layout assertion with execution-level coverage for management commands, real background flags, and flag-shaped prompt text after --.
Keep upstream provider startup profile handling behind the local CLI importer seam. Update the entrypoint routing test double to cover the new startup profile helper before background dispatch.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve space-separated resume/from-pr values in bg parsing
src/cli/bg.ts:182
The background parser treats every token after--resume,-r, and--from-pras a positional prompt because the comment assumes Commander only accepts optional option values inline. Commander does consume space-separated optional values, though, so a normal invocation likeopenclaude --bg --resume 550e8400-e29b-41d4-a716-446655440000is rewritten to child args--resume --print 550e8400-e29b-41d4-a716-446655440000. In print mode that leavesoptions.resume === true, so the child hits the existing "--resumerequires a valid session ID" path instead of resuming the requested session.--from-pr 1642is similarly rewritten as a prompt instead of a PR selector. Please keep these optional flag values attached to the option, and add regression coverage for the space-separated forms. -
[P2] Do not trust reused PIDs when command-line identity cannot be verified
src/cli/bgRegistry.ts:504
The PID identity check returnstruewhengetProcessCommand(pid)/readCommand()returnsnull, so a registry entry remainsrunningwhenever the process exists but its command line cannot be read. That is risky becauseopenclaude kill <session>refreshes statuses and then, if the session is still non-terminal andisProcessRunning(session.pid)is true, callsterminateBackgroundProcessTree(session.pid). If the original background process exited and the PID was reused, and command-line lookup fails because of platform tooling, permissions, or a transientps/PowerShell failure, this can terminate an unrelated process. The safer fallback for a claimed session-specific PID is to mark it stale/unknown unless the command line can be positively matched to the session id, with a regression test forgetProcessCommand: () => null.
Keep space-separated --resume, -r, and --from-pr values attached when building background child args. Mark live background sessions stale when PID command identity cannot be read, avoiding termination of reused unrelated PIDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/bgRegistry.ts`:
- Line 504: The condition on line 504 in bgRegistry.ts incorrectly treats a null
return from getProcessCommand() as an indicator that the session is dead, when
null could represent a transient read failure rather than an actual dead
process. This causes live sessions to be incorrectly marked as stale, breaking
downstream logic in collectLiveBackgroundSessionIds and killHandler. Change the
condition to only return false when you can definitively confirm the session is
dead, rather than when the command read fails. Additionally, update the test
cases in src/cli/bgRegistry.test.ts at lines 550-571 to reflect the corrected
behavior where transient command-read failures do not cause sessions to be
marked as stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c507ac5-555a-4d95-ba4c-223842819ea6
📒 Files selected for processing (4)
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise in code
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/cli/bg.test.tssrc/cli/bgRegistry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.ts
🔇 Additional comments (17)
src/cli/bg.ts (12)
350-391: 💤 Low valueNo session-exit detection in
followLogFile.This function polls indefinitely until SIGINT/SIGTERM. When the background session terminates, the user must manually interrupt. The
attachHandleralready documents that full reattach isn't implemented, so this is expected behavior—just noting for visibility.
1-49: LGTM!
50-117: LGTM!
119-142: LGTM!
144-169: LGTM!
171-257: LGTM!
259-310: LGTM!
312-348: LGTM!
393-434: LGTM!
436-479: LGTM!
481-539: LGTM!
541-642: LGTM!src/cli/bg.test.ts (5)
1-34: LGTM!
36-78: LGTM!
79-135: LGTM!
137-172: LGTM!
174-223: LGTM!
Represent unreadable live PID identity as a non-terminal unknown state so active sessions stay excluded from resume selection. Refuse to terminate unknown live PIDs because the process command cannot be positively matched to the background session.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and found an issue that still needs to be addressed.
Findings
-
[P2] Complete the resume-selector fix without injecting a conflicting session id
src/cli/bg.ts:560
The latest parser fix preserves--bg --resume <id>as child args, buthandleBgFlag()then adds a fresh--session-idwhenever the caller did not provide one. That child command reaches the existing root CLI validation, where--session-idis rejected with--resumeunless--fork-sessionis also present, so the documentedopenclaude --bg --resume <session-id>path still exits before print-mode resume can run. Please complete the previous resume-selector fix by avoiding the generated--session-idon non-forking resume invocations, or by otherwise making the spawned child satisfy the existing resume/session-id contract. -
[P2] Wire
--from-prthrough the background print path
src/main.tsx:2743
The background parser now keeps--bg --from-pr 1642attached to the option instead of turning1642into a prompt, but the detached child is always launched in--printmode and thisrunHeadless()call only passescontinueandresume, notfromPr. As a result, the preserved--from-prselector is ignored by the headless resume loader instead of selecting the PR-linked session the PR body says is supported. Please passfromPrthrough to the print/resume path, or keep--from-prout of the accepted background-resume forms until headless mode can actually consume it.
Avoid adding a generated --session-id to non-forked background resume launches so the spawned print-mode child satisfies the existing resume/session-id contract. Pass --from-pr through headless print mode and resolve PR-linked sessions through the shared conversation recovery path. Add regression coverage for background resume launch args and PR selector matching.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.tsx (1)
2519-2519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip startup hooks for headless
--from-prresumes.Line 2519 omits
options.fromPrfrom the suppression guard, so startup hooks can run on PR-based resume paths even though those flows are resume-like.Suggested fix
- const sessionStartHooksPromise = options.continue || options.resume || teleport || setupTrigger ? undefined : processSessionStartHooks('startup'); + const sessionStartHooksPromise = + options.continue || options.resume || options.fromPr || teleport || setupTrigger + ? undefined + : processSessionStartHooks('startup');As per coding guidelines,
src/main.tsxstartup/entrypoint changes should be blocked when they can alter startup flow safety or behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.tsx` at line 2519, The sessionStartHooksPromise assignment on line 2519 of src/main.tsx is missing the options.fromPr check in its suppression guard. Add options.fromPr to the condition alongside options.continue, options.resume, teleport, and setupTrigger so that startup hooks are correctly skipped for PR-based resume flows, which are resume-like paths that should not trigger startup initialization.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/bg.test.ts`:
- Around line 138-169: Add a new test case in the test file for the explicit
--session-id passthrough scenario in buildBackgroundSessionLaunch. This test
should verify that when an explicit --session-id argument is provided in the
input, the function preserves that session ID without injecting the generated
one, and passes the explicit ID through to childArgs unchanged. This completes
coverage of all three branches of the buildBackgroundSessionLaunch function
logic.
In `@src/cli/print.ts`:
- Around line 5044-5046: The resume mode gates in print.ts need to recognize
both options.resume and options.fromPr as valid resume sources. While line 5046
already includes both conditions, the earlier validation gates at lines 568-576
and 776-788 still only check for options.resume, causing --from-pr to be
rejected in those contexts. Update the conditional checks at the resume
validation gate (around lines 568-576) and the input check gate (around lines
776-788) to include options.fromPr alongside options.resume so that --from-pr is
treated consistently as a valid resume path throughout the codebase.
---
Outside diff comments:
In `@src/main.tsx`:
- Line 2519: The sessionStartHooksPromise assignment on line 2519 of
src/main.tsx is missing the options.fromPr check in its suppression guard. Add
options.fromPr to the condition alongside options.continue, options.resume,
teleport, and setupTrigger so that startup hooks are correctly skipped for
PR-based resume flows, which are resume-like paths that should not trigger
startup initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 301437c6-7e12-4ef8-920a-1b028c528040
📒 Files selected for processing (6)
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/print.tssrc/main.tsxsrc/utils/conversationRecovery.test.tssrc/utils/conversationRecovery.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise in code
Files:
src/main.tsxsrc/utils/conversationRecovery.test.tssrc/cli/print.tssrc/utils/conversationRecovery.tssrc/cli/bg.test.tssrc/cli/bg.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/main.tsxsrc/utils/conversationRecovery.test.tssrc/cli/print.tssrc/utils/conversationRecovery.tssrc/cli/bg.test.tssrc/cli/bg.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/main.tsx
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/main.tsxsrc/utils/conversationRecovery.test.tssrc/cli/print.tssrc/utils/conversationRecovery.tssrc/cli/bg.test.tssrc/cli/bg.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/conversationRecovery.test.tssrc/cli/bg.test.ts
🔇 Additional comments (24)
src/cli/bg.ts (15)
1-47: LGTM!
49-52: LGTM!
54-107: LGTM!
109-141: LGTM!
143-168: LGTM!
170-206: LGTM!
208-248: LGTM!
250-281: LGTM!
283-324: LGTM!
326-362: LGTM!
364-415: LGTM!
417-438: LGTM!
440-503: LGTM!
505-568: LGTM!
570-672: LGTM!src/cli/bg.test.ts (6)
1-8: LGTM!
10-78: LGTM!
80-136: LGTM!
171-206: LGTM!
208-235: LGTM!
237-257: LGTM!src/utils/conversationRecovery.ts (1)
86-87: LGTM!Also applies to: 228-258, 795-801
src/utils/conversationRecovery.test.ts (1)
197-240: LGTM!Also applies to: 264-336
src/cli/print.ts (1)
70-73: LGTM!Also applies to: 456-460, 687-693, 4910-4916
Include --from-pr in print-mode resume guards so PR-linked headless resumes can run without a prompt and share resume-only options. Skip eager startup hooks for headless PR resumes and add explicit --session-id launch coverage.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Keep
--bg --from-prregistered under the resumed transcript id
src/cli/bg.ts:654
handleBgFlag()records the session id returned bybuildBackgroundSessionLaunch()in the background registry, but the--from-prcase always gets a freshly generated id and then the child later resumes the PR-linked transcript and switches toresult.sessionIdinrunHeadless(). That leaves the live background registry pointing at the generated id while the active child is writing to the resumed PR transcript id, so the new--continuefilter will not skip that active detached transcript and can resume work that is still running. Please either register/update the background entry with the actual resumed session id for--from-pr, or make the spawned child fork/use the generated id consistently so the registry id matches the transcript being written.
Resolve non-forked --from-pr background launches to the selected transcript id before writing registry metadata. Preserve PID identity refresh for PR-resume children by matching the stored invocation when argv does not carry the transcript id. Add regressions for launch registration and registry refresh.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/bg.test.ts`:
- Around line 189-208: Add a new test case in the test file to cover the error
path in buildBackgroundSessionLaunch when PR resolution fails. Create a test
that calls buildBackgroundSessionLaunch with a resolvePrResumeSessionId callback
that returns null, and verify that it throws an error with the message "No
conversation found linked to PR selector: [selector]". This will ensure the
error handling logic (which currently exists but is untested) is properly
validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3b9e6218-c5fc-4b2a-802d-7a6f8106887a
📒 Files selected for processing (5)
src/cli/bg.test.tssrc/cli/bg.tssrc/cli/bgRegistry.test.tssrc/cli/bgRegistry.tssrc/utils/conversationRecovery.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise in code
Files:
src/utils/conversationRecovery.tssrc/cli/bgRegistry.test.tssrc/cli/bg.test.tssrc/cli/bgRegistry.tssrc/cli/bg.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/conversationRecovery.tssrc/cli/bgRegistry.test.tssrc/cli/bg.test.tssrc/cli/bgRegistry.tssrc/cli/bg.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/conversationRecovery.tssrc/cli/bgRegistry.test.tssrc/cli/bg.test.tssrc/cli/bgRegistry.tssrc/cli/bg.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: 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. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/cli/bgRegistry.test.tssrc/cli/bg.test.ts
🔇 Additional comments (10)
src/cli/bgRegistry.ts (2)
507-526: LGTM!
528-542: LGTM!src/cli/bgRegistry.test.ts (1)
528-549: LGTM!src/utils/conversationRecovery.ts (1)
802-809: LGTM!src/cli/bg.ts (5)
49-56: LGTM!
262-276: LGTM!
278-284: LGTM!Also applies to: 286-297
299-330: LGTM!
632-637: LGTM!src/cli/bg.test.ts (1)
138-149: LGTM!Also applies to: 151-167, 169-187
Add regression coverage for non-forked background --from-pr launches when the selector cannot be resolved. Verify the launch planner returns the same clear error used by handleBgFlag().
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
Migrate fork from the bg.js daemon-based bg architecture (commit 98d8715, T12.2 of bg-agent-view) to upstream's full bg.ts + bgRegistry.ts + bgFinalizer.ts + bgRouting.ts + backgroundSessionTermination.ts architecture (introduced upstream in Twigpine#1642, hardened by Twigpine#2133). New files (from upstream/main): src/cli/bg.ts (1136 lines) src/cli/bg.test.ts (1324 lines) src/cli/bgRegistry.ts (1125 lines) src/cli/bgRegistry.test.ts (1555 lines) src/cli/bgFinalizer.ts ( 238 lines) src/cli/bgFinalizer.test.ts ( 541 lines) src/cli/bgFinalizer.fixture.ts ( 36 lines) src/cli/bgRouting.ts ( 4 lines) src/utils/backgroundSessionTermination.ts ( 27 lines) Modified (apply Twigpine#2133's hunks): README.md, src/cli/print.ts, src/entrypoints/cli.tsx, src/utils/gracefulShutdown.ts Deleted (fork's old daemon-based bg, replaced by upstream's session-registry model): src/cli/bg.js (was fork's bg.js; 205 lines) src/cli/bg.test.js (was fork's bg.test.js; 484 lines) Fork-local additions to support the migrated bg.ts without dragging in upstream's full CLI machinery (which depends on envFile, flagSettings, githubModelsCredentials, interruptionTrace, daemon, templateJobs, etc. that the fork doesn't carry): src/utils/cliArgs.ts — add argsBeforeDelimiter() (mirrors upstream helper for handling `--` delimiter in argv) src/utils/conversationRecovery.ts — add findResumeSessionIdByPrSelector() (used by bg.ts's --bg pr-resume path) Minimal cli.tsx wiring (instead of upstream's full CliEntrypointImporters machinery, the fork keeps its existing main() and just installs the bg finalizer at the top when the private BACKGROUND_SESSION_ID_ENV / BACKGROUND_SESSION_LAUNCHER_PID_ENV vars are set): src/entrypoints/cli.tsx — at start of main(), import bgRouting + bgFinalizer and call prepareBackgroundSessionFinalizer() if either env var is set src/utils/gracefulShutdown.ts — add noteBackgroundSessionTerminationSignal() to SIGINT / SIGTERM / SIGHUP handlers so the finalizer can record the real exit signal Verification: bun run typecheck → 0 errors bun test src/cli/bg.test.ts → 51 pass / 0 fail bun test src/cli/bgRegistry.test.ts → 60 pass / 0 fail bun test src/cli/bgFinalizer.test.ts → 9 fail (integration tests; see "Known limitations" below) bun run build → OK bun test (full suite) → 5078 pass / 167 skip / 16 fail (7 fail are origin/main-opencc pre-existing baseline: modelRouteOverrides × 3, filesystem permissions × 2, client baseURL × 2; 9 fail are bgFinalizer integration tests) Known limitations: 1. bgFinalizer.test.ts has 9 integration test failures because the test fixtures spawn real detached CLI subprocesses (using OPENCLAUDE_BG_FINALIZER_FIXTURE_READY env var and a per-test config dir), which depend on infrastructure the fork doesn't carry (the full envFile / flagSettings / githubModelsCredentials / interruptionTrace / daemon stack that upstream's cli.tsx has). The tests are kept as placeholders for the future port of that machinery. 2. Fork's --bg now goes through upstream's session-registry model (--bg-session-id env var, registry under ~/.claude/sessions/) instead of the old daemon request socket flow. The fork's previous T12.2 daemon command surface (handleBgAgentsCommand → /bg-agents subcommand) is still available but the new `--bg <short-id>` re-launch path is the one bg.js callers used to use. Co-Authored-By: Claude <noreply@anthropic.com>
Fork-local adapter of the upstream helper added in Twigpine#1642 alongside the local background sessions feature. Bridges the PR-selector → session-id mapping for `bg.ts`'s --bg pr-resume flow without pulling the upstream `loadMessageLogs` / `getSessionIdFromLog` API surface through more wiring.
The v1 daemon-mailbox data path (BG_PROTO list/kill ops over the bg daemon socket) was inherited from the original bg-agent-view plan (T8/T9). When fork commit 1439579 ported `--bg` to the v2 session-registry model (upstream Twigpine#1642 hardened by Twigpine#2133), the dialog was left reading the v1 mailbox — which is empty for every session that came through the v2 path. `opencc ps` showed 7+ sessions; `/background` showed "No background agents". Replace the data layer with direct bgRegistry calls: - `loadJobs` → `loadSessions` (uses refreshBackgroundSessionStatuses + listBackgroundSessions; sessions are sorted startedAt desc) - `killJob` → `killSession` (uses killBackgroundSession from bg.ts) - `useBackgroundAgentJobs` → `useBackgroundAgentSessions` - `setBackgroundAgentSockPathForTesting` → `setBackgroundAgentRegistryRootForTesting` (wraps `_setBackgroundSessionsRootForTesting`) The BackgroundAgentRow renderer is unchanged — `sessionToJob` adapts the v2 BackgroundSession shape into the v1 JobRecord shape (session.id → job.short, isTerminal(session) → job.dying, etc.) so the pointer + short + cwd + status + isolation row layout is preserved across the v1 → v2 switch. Tests: - Removed the fake-daemon-server harness (loopback unix socket + FrameReader/encoder + ~150 lines of net.createServer glue) - Switched to mocking `_setBackgroundSessionsRootForTesting` and writing sessions directly to `~/.claude/bg-sessions/sessions/`, matching the pattern in `bgRegistry.test.ts`. - 8/8 tests pass. Default pid in fixtures is `process.pid` so the refresh path doesn't demote `running` sessions to `stale`. Ported from opencc-release `93029117`. Out of scope: `claude bg-agents` CLI subcommand still reads the v1 daemon mailbox (CLI surface, not the TUI dialog).
3 participants
Summary
openclaude --bgsessions withps,logs,logs -f,kill, and an explicitattachlimitation.--continuetranscript selection so active detached work is not resumed accidentally.Design and safety
ps,logs,attach, andkill.--bg --debug "prompt"preserves the prompt while still supporting inline debug filters such as--debug=api,hooks.--bg --resume <id>,--bg -r <id>, and--bg --from-pr <selector>keep their space-separated selector values attached to the option instead of treating them as prompts.--, including--print,-p, and--bg, remains positional and does not change background routing or child print-mode detection.unknownstate, keeping resume filtering conservative while avoiding unsafe termination of a reused unrelated PID.killwaits for verified running process-tree exit and escalates toSIGKILLbefore marking a session killed; liveunknownsessions are not terminated because their PID identity cannot be verified.Validation
bunx bun@1.3.13 test --max-concurrency=1 src/entrypoints/cli.test.ts src/cli/bg.test.ts(23 pass, 0 fail)bunx bun@1.3.13 test --max-concurrency=1 src/commands/provider/provider.test.tsx src/utils/providerProfile.test.ts src/services/api/providerValidation.test.ts(101 pass, 0 fail)bunx bun@1.3.13 run typecheckgit diff --checkbunx bun@1.3.13 test --max-concurrency=1 src/cli/bg.test.ts src/cli/bgRegistry.test.ts(34 pass, 0 fail)bunx bun@1.3.13 run check(4067 pass, 0 fail)0996e7d7: preserved upstream provider-profile startup handling behind the local CLI importer seam.0996e7d7:smoke-and-tests,typecheck, andwebpassed0996e7d7: approved, no actionable comments in the latest review953aabc2: preserved background resume/from-PR selector values and added conservative PID identity handling.03ecd3ed: represented unreadable live PID identity as non-terminalunknownand blocked unsafekilltermination for unverified live PIDs.03ecd3ed:smoke-and-tests,typecheck, andwebpassed03ecd3ed: approved, no unresolved review threadsKnown limitation
attachcurrently points users toopenclaude logs <id> -f; full terminal reattach is not implemented in this change.Summary by CodeRabbit
Release Notes
New Features
--bg/--background) with persistent session tracking and dedicated commands:ps,logs(supports-ffor follow; stdout/stderr),kill, andattach(redirects tologs -f).--from-pr.Documentation
Bug Fixes