Skip to content

fix(bg): match background-session command args on token boundaries - #1834

Merged
kevincodex1 merged 4 commits into
Twigpine:mainfrom
Pablosinyores:fix/1770-bg-command-token-boundary
Jul 7, 2026
Merged

kevincodex1 merged 4 commits into
Twigpine:mainfrom
Pablosinyores:fix/1770-bg-command-token-boundary

Conversation

@Pablosinyores

@Pablosinyores Pablosinyores commented Jul 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • commandLineContainsArgs (src/cli/bgRegistry.ts) matched each stored launch arg as a raw substring via indexOf, so a selector like 1642 was satisfied by an unrelated live token 16420 in the same position. A reused PID whose command line merely contains the stored digits could keep a dead background session classified as running, and the kill path could then signal the wrong process.
  • Match each stored arg against a whole whitespace-delimited token, in order, instead of a substring. This is the focused token-boundary hardening from the roadmap in Harden background process identity checks before killing sessions #1770 (step 1).

This is a partial/regression fix, not a full resolution — hence Refs #1770 rather than Fixes #1770. The broader process-identity work (pre-signal revalidation, durable argv marker, legacy-session fallback) is deliberately out of scope here and left for its own PRs.

Impact

  • user-facing impact: openclaude kill <id-or-name> and background-session liveness are less likely to act on an unrelated process that merely reuses a PID with a superset-token command line. Legitimate sessions are still recognized by exact command tokens or by the session id on the command line.
  • developer/maintainer impact: commandLineContainsArgs now tokenizes on whitespace and requires whole-token matches; behavior is unchanged for real invocations whose argv tokens match exactly.

Testing

  • bun run build
  • bun run smoke
  • bun run check
  • focused tests: bun test src/cli/bgRegistry.test.ts (27 pass)

Added four cases through the exported isBackgroundSessionProcessAlive (injecting isProcessAlive/getProcessCommand): the 1642 vs 16420 collision (now false), exact-token match (true), session-id match (true), and dead process (false). The collision case is proven to fail against the previous substring implementation and pass after the change.

Notes

  • provider/model path tested: n/a (background-session registry only; no provider/model surface touched).
  • follow-up work or known limitations: this only closes the deterministic substring collision. Per the roadmap in Harden background process identity checks before killing sessions #1770, the higher-value durable pieces (re-reading process identity immediately before signalling kill, and a persisted opaque argv marker for new sessions) belong in follow-up PRs. Happy to take a run at the pre-signal revalidation next once this lands, following maintainer sequencing.

Summary by CodeRabbit

  • Bug Fixes
    • Improved background session liveness detection by matching command-line content using contiguous, whitespace-delimited token sequences rather than substring searches, reducing false positives from reused identifiers.
    • Session IDs are now checked as whole tokens, with safer handling of quotes/whitespace and correct behavior when the process is reported dead.
  • Tests
    • Added expanded coverage for token boundaries, multi-word argv matching, contiguity constraints, quote-related parsing, substring-collision rejection, and dead-process scenarios.

commandLineContainsArgs matched each stored launch arg as a raw
substring, so a selector like "1642" was satisfied by an unrelated live
token "16420" in the same position. A reused PID whose command line
merely contained the stored digits could therefore keep a dead session
classified as running, and the kill path could target the wrong
process.

Match each stored arg against a whole whitespace-delimited token, in
order, instead of a substring. Add regression coverage through
isBackgroundSessionProcessAlive for the collision, the exact-token
match, the session-id match, and the dead-process case.

Refs Twigpine#1770
@coderabbitai

coderabbitai Bot commented Jul 1, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6870ef75-71ad-4352-b6a5-5f00438741b1

📥 Commits

Reviewing files that changed from the base of the PR and between 529de15 and 8810e22.

📒 Files selected for processing (2)
  • src/cli/bgRegistry.test.ts
  • src/cli/bgRegistry.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: smoke-and-tests (24.11.x)
  • GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,tsx}: Use TypeScript with strict mode
Use ESM imports in TypeScript source files

Files:

  • src/cli/bgRegistry.test.ts
  • src/cli/bgRegistry.ts
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md - AI Agent Coding Guide

This guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.

Project Snapshot

OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.

The installed CLI runs on Node.js >=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.

Work Style

  • Keep changes focused on one problem.
  • Prefer existing patterns in the file or nearby module.
  • Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
  • Add or update tests when behavior changes.
  • Update docs when setup, commands, provider behavior, or user-facing behavior changes.
  • For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.

Stack And Conventions

  • TypeScript with strict mode and ESM imports.
  • React + Ink for terminal UI.
  • Bun lockfile and Bun scripts for development workflows.
  • Node runtime for the built CLI.

Common libraries and patterns:

  • chalk for terminal color.
  • commander for CLI argument parsing.
  • execa for child processes.
  • Existing service, provider, settings, permission, and UI patterns over new abstractions.

Repository Map

  • src/commands/ - slash and CLI command implementations.
  • src/components/ - React/Ink UI components.
  • src/services/ - API, MCP, OAuth, wiki, voice, and other service integrations.
  • src/tools/ - tool implementations.
  • src/utils/ - shared utilities.
  • src/integrations/ - provider and model integration metadata.
  • src/entrypoints/ - CLI, MCP, SDK, and generated public types.
  • src/tasks/ - local, remote, workflow, and monitor tas...

Files:

  • src/cli/bgRegistry.test.ts
  • src/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/bgRegistry.test.ts
  • src/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/bgRegistry.test.ts
🔇 Additional comments (3)
src/cli/bgRegistry.ts (2)

507-521: LGTM!


523-557: 🎯 Functional Correctness

No issue: BackgroundSession.command is written from [process.execPath, ...execArgv, entrypoint, ...childArgs], so the first token is the exact executable path that ps/Get-CimInstance should report for the spawned process.
The bare node values in tests are fixtures, not the runtime shape.

			> Likely an incorrect or invalid review comment.
src/cli/bgRegistry.test.ts (1)

9-15: LGTM!

Also applies to: 681-808


📝 Walkthrough

Walkthrough

Background session liveness checks now use whole-token command matching instead of substring scans, and tests cover exact matches, collision cases, multi-word argv handling, and dead-process rejection.

Changes

Background session identity matching fix

Layer / File(s) Summary
Whole-token matching implementation
src/cli/bgRegistry.ts
commandLineContainsArgs now tokenizes command lines and matches stored args as ordered whole tokens; sessionId matching uses the same whole-token logic.
Process identity tests
src/cli/bgRegistry.test.ts
Imports are updated and a new suite exercises substring rejection, exact token/sessionId matches, multi-word argv matching, and dead-process handling.

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

Suggested labels: bug

Suggested reviewers: jatmn, kevincodex1

🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR fixes the substring collision, but it does not implement marker-based identity, fail-closed handling, or pre-kill revalidation from #1770. Implement the remaining #1770 identity-hardening requirements or narrow the linked issue scope to the token-boundary fix only.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the token-boundary hardening in the diff.
Description check ✅ Passed The description follows the required template and covers summary, impact, testing, and notes.
Out of Scope Changes check ✅ Passed The changes stay within background-session identity hardening and related tests, with no unrelated code paths added.
Risk Surface Disclosed ✅ Passed PASS: The PR explicitly calls out the background-kill misidentification risk and marks this as a partial fix with follow-ups out of scope; no blocker is introduced.
No Hidden Policy Change ✅ Passed Scoped to process-identity matching only; no telemetry, routing, permission, or product-policy changes are present or hidden.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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.test.ts`:
- Around line 681-732: Add a companion regression test in
isBackgroundSessionProcessAlive process identity that covers the sessionId
matching path, since it is still vulnerable to substring collisions. Use the
existing BackgroundSession fixture and verify a command line containing a longer
unrelated token like sess-100 does not match sessionId sess-1, while keeping the
exact-token case positive. Refer to isBackgroundSessionProcessAlive and the
sessionId/commandLine matching branch in bgRegistry.ts so the new test guards
the hardened tokenized behavior.

In `@src/cli/bgRegistry.ts`:
- Around line 507-529: The `commandLineMatchesBackgroundSession` path still uses
a raw `includes` check for `session.sessionId`, so it can misclassify a process
when the session ID appears as a substring inside an unrelated token. Update
that sessionId lookup to use the same whole-token matching approach as
`commandLineContainsArgs` (or share the tokenization logic) so `sessionId`
matching is consistent with the fixed `session.command` path and avoids the
`1642` vs `16420` collision.
- Around line 514-526: The argument matching in bgRegistry.ts’s session liveness
check is splitting commandLine on whitespace, which breaks multi-word argv
values like refactor auth. Update the matching logic in the arg/commandLine
comparison block to preserve quoted or free-text argv entries as single
arguments, and make sure the loop in the relevant matcher still compares each
arg against the correct token sequence without treating spaces inside a value as
separators.
🪄 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

Run ID: d944096c-2aa3-4b18-86c8-d7ee280cc3b6

📥 Commits

Reviewing files that changed from the base of the PR and between bb61d84 and 2c7b0f3.

📒 Files selected for processing (2)
  • src/cli/bgRegistry.test.ts
  • src/cli/bgRegistry.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: smoke-and-tests
  • GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript with strict mode and ESM imports

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Follow the existing code style in the touched files

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx,js,jsx,py}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Keep comments useful and concise

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Follow TypeScript strict mode and type safety practices by running typecheck before submitting

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md - AI Agent Coding Guide

This guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.

Project Snapshot

OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.

The installed CLI runs on Node.js >=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.

Work Style

  • Keep changes focused on one problem.
  • Prefer existing patterns in the file or nearby module.
  • Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
  • Add or update tests when behavior changes.
  • Update docs when setup, commands, provider behavior, or user-facing behavior changes.
  • For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.

Stack And Conventions

  • TypeScript with strict mode and ESM imports.
  • React + Ink for terminal UI.
  • Bun lockfile and Bun scripts for development workflows.
  • Node runtime for the built CLI.

Common libraries and patterns:

  • chalk for terminal color.
  • commander for CLI argument parsing.
  • execa for child processes.
  • Existing service, provider, settings, permission, and UI patterns over new abstractions.

Repository Map

  • src/commands/ - slash and CLI command implementations.
  • src/components/ - React/Ink UI components.
  • src/services/ - API, MCP, OAuth, wiki, voice, and other service integrations.
  • src/tools/ - tool implementations.
  • src/utils/ - shared utilities.
  • src/integrations/ - provider and model integration metadata.
  • src/entrypoints/ - CLI, MCP, SDK, and generated public types.
  • src/tasks/ - local, remote, workflow, and monitor tas...

Files:

  • src/cli/bgRegistry.ts
  • src/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/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Add or update tests when the change affects behavior

Files:

  • src/cli/bgRegistry.test.ts
**/*.test.{ts,tsx,js}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Test the exact provider/model path you changed when possible

Files:

  • src/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/bgRegistry.test.ts
🔇 Additional comments (2)
src/cli/bgRegistry.ts (1)

531-539: LGTM! (unchanged; see the sessionId note attached to lines 507-529 for the follow-up)

src/cli/bgRegistry.test.ts (1)

9-15: LGTM!

Comment thread src/cli/bgRegistry.test.ts
Comment thread src/cli/bgRegistry.ts
Comment thread src/cli/bgRegistry.ts Outdated

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

CodeRabbit's requested changes have been open for more than five hours, and this repository's CONTRIBUTING.md says PR authors should address CodeRabbit findings instead of waiting for a maintainer override. I verified the current patch, and the session-id and multi-word argv comments are still valid, so please complete those review items before this moves forward.

Findings

  • [P1] Complete CodeRabbit's request to harden the session-id match
    src/cli/bgRegistry.ts:535
    The new matcher fixes substring collisions for session.command, but the first branch still returns true when the live command merely contains session.sessionId as a substring. For example, a stored id like sess-1 would still match an unrelated live command containing sess-100, leaving that reused PID classified as the background session before the kill path decides whether to signal it. Please complete CodeRabbit's request by routing the session-id check through the same exact/whole-token identity matching instead of includes.

  • [P2] Complete CodeRabbit's request to preserve multi-word argv values
    src/cli/bgRegistry.ts:514
    Background prompts are stored as argv entries, and --from-pr/resume launches can rely on session.command because the resolved session id is not present on argv. A real prompt such as refactor auth is stored as one arg, but ps renders the live command as words like ... --from-pr 1642 --print refactor auth; after this change split(/\s+/) produces refactor and auth, so it can no longer match the stored arg refactor auth. The scanner also keeps skipping later tokens until it finds a match, so it still does not compare the stored argv sequence to the live command sequence reliably. That can mark a live background PR-resume session stale, or keep trusting a command where the matching token appears later in the wrong argument position. Please update the matcher so it preserves argv boundaries and compares each stored arg against the correct live argument position.

… args

Address review on Twigpine#1770:

- commandLineMatchesBackgroundSession matched session.sessionId with a raw
  includes(), so a short id like 'sess-1' matched an unrelated live command
  containing 'sess-100' and kept a reused-PID session classified as running.
  Route it through the same whole-token matcher.
- commandLineContainsArgs required each stored arg to equal a single token, so
  a prompt stored as one argv entry (e.g. 'refactor auth') never matched the
  words ps renders it as, breaking --from-pr/resume launches that rely on the
  stored command. Expand each arg into its own tokens and match the flattened
  sequence as an ordered subsequence.

Add regression tests: session-id substring collision stays dead, exact id
token is alive, and a multi-word prompt arg matches across command tokens
(all proven fail-on-old).
@Pablosinyores

Copy link
Copy Markdown
Contributor Author

Thanks — both addressed in 34fa3a5.

  • [P1] The session-id check no longer uses includes(). commandLineMatchesBackgroundSession now routes session.sessionId through the same whole-token matcher, so sess-1 no longer matches an unrelated live command containing sess-100. withGeneratedSessionId writes --session-id <id> as its own token, so the id is always a standalone token when present — the whole-token match is exact there.
  • [P2] commandLineContainsArgs now expands each stored arg into its own tokens and matches the flattened sequence as an ordered subsequence, so a prompt stored as one argv entry like refactor auth matches the words ps renders (... --print refactor auth). --from-pr/resume launches that rely on session.command keep matching.

Added regression tests: session-id substring collision stays dead, the exact id token is alive, and a multi-word prompt arg matches across command tokens. Verified all three fail on the pre-fix matcher and pass after. bun test src/cli/bgRegistry.test.ts 30 pass, typecheck clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts (1)

507-538: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Non-contiguous subsequence matching reopens a collision gap this PR is meant to close.

commandLineContainsArgs matches argTokens as an ordered subsequence, not a contiguous block — the inner while loop skips forward over any non-matching live tokens between matches. That means a live command like node x openclaude y 1642 would satisfy stored args ['node', 'openclaude', '1642'], because each token is still found in order, just with unrelated tokens interspersed. A reused PID whose command line happens to contain the same ordered tokens with extra junk between them will still be misclassified as the owning session — the same class of false-positive collision #1770 targets, just via token insertion instead of digit-substring overlap.

None of the added tests cover this interspersed-token case, so it isn't caught by the new regression suite either.

🔧 Suggested fix: require contiguous match
-  if (argTokens.length === 0) return false
-  let tokenIndex = 0
-  for (const argToken of argTokens) {
-    let matched = false
-    while (tokenIndex < tokens.length) {
-      const token = tokens[tokenIndex]
-      tokenIndex += 1
-      if (token === argToken) {
-        matched = true
-        break
-      }
-    }
-    if (!matched) return false
-  }
-  return true
+  if (argTokens.length === 0 || argTokens.length > tokens.length) return false
+  outer: for (let start = 0; start <= tokens.length - argTokens.length; start++) {
+    for (let i = 0; i < argTokens.length; i++) {
+      if (tokens[start + i] !== argTokens[i]) continue outer
+    }
+    return true
+  }
+  return false
🤖 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 507 - 538, The `commandLineContainsArgs`
helper in `bgRegistry` still allows non-contiguous ordered subsequence matches,
which can misclassify unrelated live processes as matching a stored session.
Change the matching logic so `args` must appear as one contiguous run of whole
tokens in `commandLine` rather than skipping over intervening tokens, and keep
the whitespace-tokenization behavior for multi-word args. Update or add a
regression test around `commandLineContainsArgs` to cover an interspersed-token
case that should not match.
🤖 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.ts`:
- Around line 507-538: The `commandLineContainsArgs` helper in `bgRegistry`
still allows non-contiguous ordered subsequence matches, which can misclassify
unrelated live processes as matching a stored session. Change the matching logic
so `args` must appear as one contiguous run of whole tokens in `commandLine`
rather than skipping over intervening tokens, and keep the
whitespace-tokenization behavior for multi-word args. Update or add a regression
test around `commandLineContainsArgs` to cover an interspersed-token case that
should not match.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 54185d9e-e17b-4a7b-80ad-c17bf45cfa87

📥 Commits

Reviewing files that changed from the base of the PR and between 2c7b0f3 and 34fa3a5.

📒 Files selected for processing (2)
  • src/cli/bgRegistry.test.ts
  • src/cli/bgRegistry.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: typecheck
  • GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript with strict mode and ESM imports

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx,js,jsx,py,json,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Follow the existing code style in the touched files

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx,js,jsx,py}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Keep comments useful and concise

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Follow TypeScript strict mode and type safety practices by running typecheck before submitting

Files:

  • src/cli/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md - AI Agent Coding Guide

This guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.

Project Snapshot

OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.

The installed CLI runs on Node.js >=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.

Work Style

  • Keep changes focused on one problem.
  • Prefer existing patterns in the file or nearby module.
  • Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
  • Add or update tests when behavior changes.
  • Update docs when setup, commands, provider behavior, or user-facing behavior changes.
  • For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.

Stack And Conventions

  • TypeScript with strict mode and ESM imports.
  • React + Ink for terminal UI.
  • Bun lockfile and Bun scripts for development workflows.
  • Node runtime for the built CLI.

Common libraries and patterns:

  • chalk for terminal color.
  • commander for CLI argument parsing.
  • execa for child processes.
  • Existing service, provider, settings, permission, and UI patterns over new abstractions.

Repository Map

  • src/commands/ - slash and CLI command implementations.
  • src/components/ - React/Ink UI components.
  • src/services/ - API, MCP, OAuth, wiki, voice, and other service integrations.
  • src/tools/ - tool implementations.
  • src/utils/ - shared utilities.
  • src/integrations/ - provider and model integration metadata.
  • src/entrypoints/ - CLI, MCP, SDK, and generated public types.
  • src/tasks/ - local, remote, workflow, and monitor tas...

Files:

  • src/cli/bgRegistry.ts
  • src/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/bgRegistry.ts
  • src/cli/bgRegistry.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Add or update tests when the change affects behavior

Files:

  • src/cli/bgRegistry.test.ts
**/*.test.{ts,tsx,js}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Test the exact provider/model path you changed when possible

Files:

  • src/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/bgRegistry.test.ts
🔇 Additional comments (4)
src/cli/bgRegistry.ts (2)

540-547: LGTM! sessionId now correctly routed through whole-token matching, closing the gap flagged in the prior review round.


553-577: LGTM!

src/cli/bgRegistry.test.ts (2)

6-16: LGTM!


725-771: LGTM! Good regression coverage for the sessionId whole-token and multi-word prompt cases requested in prior review. Note: the interspersed-token/non-contiguous matching gap flagged in bgRegistry.ts (lines 507-538) isn't covered by this suite either — worth adding once that's addressed.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 2, 2026

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

CodeRabbit's latest requested change is still valid, and it has been open for more than five hours. This repository's CONTRIBUTING.md says PR authors should address CodeRabbit findings instead of waiting for a maintainer override, so please complete that automated review item before this moves forward.

Findings

  • [P1] Complete CodeRabbit's request to require a contiguous command match
    src/cli/bgRegistry.ts:527
    The new matcher still treats the stored background command as an ordered subsequence of the live command line, so it skips over unrelated tokens between matches. For example, a stored command like ['node', 'openclaude', '1642'] is still classified as alive for a reused PID whose command line is node attacker openclaude extra 1642 --serve, because the loop advances past attacker and extra and eventually finds each stored token. That leaves the same wrong-process kill risk open for token insertion collisions, and the added tests only cover 1642 vs 16420, not this interspersed-token case. Please complete CodeRabbit's request by requiring the flattened stored argv tokens to appear as one contiguous whole-token run in the live command line, and add a regression test where unrelated tokens inserted between the stored tokens are rejected.

…en run

The token-boundary matcher for background-session process identity treated
the stored argv tokens as an ordered subsequence of the live command line, so
unrelated tokens between matches were skipped. A reused PID whose command
interleaves the stored tokens (e.g. stored [node, openclaude, 1642] against
"node attacker openclaude extra 1642 --serve") was still classified alive,
keeping the wrong-process kill risk open for token-insertion collisions.

Match the flattened stored argv as one contiguous whole-token run instead.
Leading interpreter path and trailing flags still match; interspersed tokens
no longer do. Add a regression test asserting an interspersed-token command
line is rejected (fails on the prior subsequence matcher, passes now).

Refs Twigpine#1770
@Pablosinyores

Copy link
Copy Markdown
Contributor Author

Addressed in 529de15. The matcher now requires the flattened stored argv tokens to appear as one contiguous whole-token run, not an ordered subsequence — so the interspersed case you called out (node attacker openclaude extra 1642 --serve against stored [node, openclaude, 1642]) is now rejected. Leading interpreter path and trailing flags still match; tokens wedged between the stored ones do not.

Added a regression test (does not treat interspersed stored tokens as alive) with exactly that interleaved command line. Verified it fails on the previous subsequence matcher (Received: true) and passes now. bun test bgRegistry.test.ts 31 pass, typecheck clean on the file.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 6, 2026

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an issue that needs to be addressed before this is ready.

Findings

  • [P2] Preserve Windows background sessions that rely on command fallback
    src/cli/bgRegistry.ts:526
    The new matcher splits both the live command line and the stored argv on whitespace, but Windows Get-CimInstance ... CommandLine returns the raw command line with quoted paths and quoted arguments, while session.command stores unquoted argv entries such as C:\Program Files\nodejs\node.exe or refactor auth. For non-forked --from-pr resumes, buildBackgroundSessionLaunch() deliberately does not add the resolved session id to argv, so this command fallback is the only identity path. A real Windows command like "C:\Program Files\nodejs\node.exe" C:\repo\dist\cli.mjs --from-pr 1642 --print "refactor auth" no longer matches stored ['C:\Program Files\nodejs\node.exe', 'C:\repo\dist\cli.mjs', '--from-pr', '1642', '--print', 'refactor auth'], so refreshBackgroundSessionStatuses() marks a live session stale and openclaude kill <id> can report it killed without signalling the process. Please make the token-boundary fix handle quoted Windows command lines, or avoid requiring the quoted executable/entrypoint/prompt tokens before falling back to the argv selector tokens.

Get-CimInstance CommandLine returns the raw, quoted Windows command line, so
a whitespace split fuses quotes onto the edge tokens ("C:\Program,
node.exe", "refactor, auth") while session.command stores those values
unquoted. The contiguous whole-token run therefore never matched, and a live
non-forked --from-pr resume (whose only identity path is the stored command)
was marked stale, letting kill <id> report success without signalling the
process. Trim a single surrounding quote from each token before comparing;
POSIX ps output is unquoted so this is a no-op there and preserves the Twigpine#1770
token-boundary guard.
@Pablosinyores

Copy link
Copy Markdown
Contributor Author

Good catch on the Windows path — took the quote-handling route.

Get-CimInstance ... CommandLine hands back the raw quoted command line, so a whitespace split fuses the quotes onto the edge tokens ("C:\Program, node.exe", "refactor, auth") while session.command stores those same values unquoted — the contiguous run never lined up, so a non-forked --from-pr resume (only identity path is the stored command) got marked stale and kill <id> would report success without signalling anything.

Fix trims a single surrounding quote from each token before comparing (tokenizeCommandLine), applied to both the live line and the stored argv. POSIX ps output carries no quotes so it's a no-op there, and it doesn't loosen the token boundary — a quoted "16420" still won't satisfy stored 1642.

Added two tests: the exact quoted Windows line from your example (spaced exe path + quoted --print "refactor auth") now resolves alive, and a quoted-collision guard to lock the boundary. Verified the Windows test fails on the prior commit and passes now; full bgRegistry suite green, tsc clean.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I rechecked the previously discussed background-session identity paths and do not see any remaining actionable issues from my side.

@kevincodex1 LGTM

@kevincodex1
kevincodex1 merged commit 67bebbd into Twigpine:main Jul 7, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants