fix(ai-detection): match binary tokens, not loose substrings

[agent-era-ai]

Summary

• Fixes a misdetection in AIToolService.detectToolFromArgs where Codex (or Gemini) sessions whose ps args= contained the substring claude — a worktree slug like agent-shows-claude-not-codex, a prompt mentioning Claude, or an install path under ~/.claude* — were rendered as Claude on the kanban / status UI.
• Two-pass detection: a strict pass strips shell-quoted argument bodies and matches each tool name only at a token boundary ((?:^|[\s/])name(?=\s|$)); the original .includes() chain is kept as a final fallback for legacy invocation shapes. Per-tool token regexes are built once at module load from AI_TOOLS, so a new tool added to the config is detected automatically.
• Adds 13 table-driven unit cases covering the broken cases (codex/gemini × claude-in-slug / claude-in-prompt / claude-in-install-path), the bash-wrapper resume-or-fresh shape, case-insensitive variants, and the legacy fallback path. No changes to isAIPaneCommand, status detection, or launch logic.

Test plan

• npx jest tests/unit/AIToolService.test.ts — 38/38 (13 new) pass
• npx jest (full suite) — 78 suites, 801/801 pass
• npx tsc -p tsconfig.test.json — clean
• Spot-check: launch Codex on a worktree whose slug contains "claude" and confirm the agent column reports Codex

🤖 Generated with Claude Code

[claude]

Code Review — PR #239: fix(ai-detection): match binary tokens, not loose substrings

Overview

Fixes a real, reproducible misdetection bug in AIToolService.detectToolFromArgs: the old includes() chain matched the first tool name found anywhere in the full ps args output, so a Codex or Gemini session with "claude" in its prompt, slug, or install path was mis-labeled as Claude. The fix is a strict-then-loose two-pass: strip shell-quoted spans, match each tool name only as a standalone token via a pre-compiled regex, fall back to the original includes() for legacy shapes. 13 new table-driven unit tests cover every AC.

---

Code quality and correctness

src/services/AIToolService.ts

• Regex safety — (?:^|[\s/])${tool}(?=\s|$) correctly handles the three realistic prefix forms (start of string, after a path separator, after whitespace) and ensures no suffix char other than whitespace or EOS can follow. A name like claude-code after / would have claude followed by -, which fails the lookahead — correct. No concern here.

• Quote-stripping — /'[^']*'/g and /"[^"]*"/g are ungreedy only in the sense that [^']* stops at the first closing quote. This handles the common shellQuote single-quote form correctly. It would mis-strip something like it\'s in a single-quoted span, but ps args output doesn't typically include escaped quotes in the span; this is pragmatic and fine for the use case.

• Implicit ordering dependency — The comment "Object.keys preserves insertion order, which today places claude first" is accurate but fragile: if someone reorders AI_TOOLS in constants.ts, the loose fallback path changes behavior silently. Consider either:

  • Adding a note in constants.ts that insertion order affects fallback detection priority, or
  • Sorting TOOL_NAMES explicitly if a deterministic order is desirable.
    (Minor — the risk is low since AI_TOOLS rarely changes.)

• Object.fromEntries type assertion — as Record<keyof typeof AI_TOOLS, RegExp> is a necessary workaround for TypeScript's weak inference on Object.fromEntries. Fine as-is.

• Multi-paragraph JSDoc — AGENTS.md says multi-paragraph JSDoc "should be rare." The new docblock on detectToolFromArgs is informative and explains a non-obvious invariant, which is exactly the stated exception, so it passes the spirit of the guideline. If you want to trim it, the first paragraph (the one ending "the bug behind worktrees like…") is the load-bearing sentence; the second is restating the fallback which is visible in code. Not a blocker.

• Module-level comment block — Same observation: the 4-line comment above TOOL_NAMES / TOOL_TOKEN_RES includes one genuinely non-obvious note (insertion order / historical bias). The other lines ("Built once at module load so a new tool added to the config is detected automatically") describe what the code does, which AGENTS.md advises against. Could be trimmed to just the ordering note.

tests/unit/AIToolService.test.ts

• Table-driven approach is clean and matches project style.
• Tests drive through the public detectAllSessionAITools API rather than breaking the private boundary — correct and consistent with the existing fixture pattern.
• All 8 ACs with concrete args are covered. The legacy-fallback case (someweirdtoolnameclaudethingembedded) is important and present.
• import type {AITool} is the right form for a type-only import.

---

Potential issues

1. Empty-string edge in the mock — When the mock returns '' for the ps call, result.get('dev-p-f') would be undefined, not 'none'. The tests that expect 'none' pass a valid argsLine ('bash', 'vim'), so this isn't a bug in the tests, but it's worth noting that a truly empty args line isn't directly tested. Low risk.

2. (?=\s|$) in strict pass, but $ matches end of the stripped string, not end of a token within it — After quote-stripping, something like bash -c codex in the stripped form is bash -c codex (no quotes to strip), and codex is at end-of-string. The regex matches. That's the intended behavior and it's correct; just flagging it's been validated.

3. No test for double-quoted prompts — The cases cover single-quoted prompts (the default shellQuote form) and bare tokens. Double-quoted prompts are handled by the second replace (/"[^"]*"/g), but there's no explicit test case. Worth adding if you want full coverage of the quote-stripping branch, though the risk is very low in practice.

---

Performance

No concerns. Regexes are pre-compiled once at module load. The two sequential loops over TOOL_NAMES (3 entries today) are negligible.

---

Security

No new attack surface. The function reads an already-retrieved ps args string and applies pure string matching. No shell execution, file I/O, or untrusted interpolation added.

---

Test coverage

• 13 new cases, all mapping directly to the AC list in requirements.md. ✅
• The full suite (801 tests) and typecheck are reported green in the test plan. ✅
• The one manual step (spot-check with a live Codex session on a claude-bearing slug) is correctly noted as not yet done.

---

Summary

Solid fix. Root cause is correctly identified, the implementation is minimal and doesn't over-engineer, and the test coverage directly matches the requirements. The two minor style notes (trimming the what-describing lines from the module comment, and noting the insertion-order dependency in constants.ts) are worth a quick look before merge but aren't blockers. No correctness, security, or performance concerns.

[claude]

Code Review — PR #239: fix(ai-detection): match binary tokens, not loose substrings

Overview

Fixes a real bug where Codex/Gemini sessions were misidentified as Claude when the process args string contained the substring claude anywhere — in a worktree slug, a quoted prompt, or an install path. The two-pass strict-then-loose approach is well-reasoned and the test suite is thorough.

---

What's good

• Root cause is correctly identified and fixed. The strict regex (?:^|[\s/])${name}(?=\s|$) cleanly distinguishes binary tokens from substrings inside slugs/prompts.
• Quote stripping is sound. Stripping '...' and "..." spans before the strict pass handles the shellQuote wrapping correctly.
• Module-level regex compilation avoids repeated regex construction on every detection call.
• Auto-extending to new tools. Building TOOL_TOKEN_RES from AI_TOOLS means a new tool entry in constants is picked up automatically — no separate maintenance.
• Test coverage is excellent. 13 table-driven cases cover the exact scenarios from the bug report plus regression coverage for legacy paths.
• Scope is tightly bounded. isAIPaneCommand, status detection, launch logic, and AI_TOOLS config are all left untouched.

---

Issues and suggestions

Latent regex injection risk (low severity, worth a one-line guard)

TOOL_TOKEN_RES builds regexes directly from Object.keys(AI_TOOLS) without escaping:

new RegExp(`(?:^|[\\s/])${name}(?=\\s|$)`)

All current tool names (claude, codex, gemini, auggie) are regex-safe, but if someone ever adds a tool with a name containing ., +, (, etc., the regex silently changes semantics. Consider a minimal guard:

const safeName = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
new RegExp(`(?:^|[\\s/])${safeName}(?=\\s|$)`)

Doesn't affect any current tool; future-proofs against a silent footgun.

Load-order coupling comment could be sharper

The existing comment accurately describes the risk, but it points to constants.ts as the hazard while the actual enforcement point is the loop order here. A small addition would make it actionable:

// Iteration order is load-bearing for the loose fallback: first hit wins when args
// contains substrings of multiple tool names. If you need explicit priority, enforce
// it here rather than relying on AI_TOOLS insertion order.

Unbalanced quotes in the stripped string (edge case)

The quote-stripping step:

argsLower.replace(/'[^']*'/g, '').replace(/"[^"]*"/g, '')

… assumes balanced, non-nested quotes. If ps args= ever surfaces an unbalanced ' (e.g. from a malformed command line), the regex matches nothing and the full prompt text leaks through to the strict pass. This is unlikely in practice, but it's the failure mode if it does occur — the strict pass still won't match unless the leaked text contains a bare binary token, so the fallback behaviour is safe. No action required, but worth a comment if this function is ever revisited.

Minor: as Record<…> cast on Object.fromEntries

The cast is necessary given current TypeScript limitations with Object.fromEntries. No change needed — just noting it's intentional and correct.

---

Test coverage

The table-driven test suite is comprehensive and covers all acceptance criteria from the requirements doc. The decision to test through the public detectAllSessionAITools rather than exposing the private method is correct per the project's testing philosophy.

One optional addition that would increase confidence: a test case for an args string with an unbalanced single-quote to confirm it degrades safely (returns 'none' rather than throwing or misdetecting). Not blocking.

---

Verdict

Approve with suggestions. The fix is correct, the tests are thorough, and the scope is tight. The regex-escaping guard is the only suggestion worth acting on before merge; the rest are minor.

[claude]

Code Review — PR #239: fix(ai-detection): match binary tokens, not loose substrings

Overview

Fixes a real misdetection in AIToolService.detectToolFromArgs: the old .includes('claude') matched anywhere in the full ps args= string, so a Codex session whose args contained a worktree slug, prompt, or install path with "claude" in it was labelled as Claude in the UI. The fix introduces a two-pass detection scheme: strict word-boundary regex on the lowercased-and-quote-stripped args first, then the original .includes() chain as a legacy fallback. Module-level regex compilation avoids re-building the regexes on every call.

---

Code Quality & Style

Strengths:

• The fix is minimal and scoped exactly to the broken function — no unrelated changes.
• Pre-building TOOL_TOKEN_RES at module load is the right call: one-time cost, no per-call allocation.
• The escapeRe helper is simple and correct.
• Tests are correctly driven through the public detectAllSessionAITools path, consistent with how the existing tests were written.
• The two-pass design is a good backwards-compatibility tradeoff: strict detection fixes the real cases, loose fallback avoids silently breaking any unusual-but-historically-correct invocation shapes.

Style nit — comments reference the current task/branch:

Per AGENTS.md: "Don't reference the current task, fix, or callers — those belong in PR descriptions and rot as the codebase evolves."

Two places violate this:

1. AIToolService.ts (new function comment):

   // Strict pass first to prevent "claude" inside a prompt, slug, or install path from
   // outranking the actually-running binary (the bug behind worktrees like
   // `agent-shows-claude-not-codex` rendering as Claude when Codex is attached).

   The "the bug behind worktrees like agent-shows-claude-not-codex" part will read as dead context in six months. Suggest trimming to just the invariant:

   // Strict pass first: a tool name inside a prompt, slug, or install path must not
   // outrank the actually-running binary. Falls back to loose .includes() for legacy
   // invocation shapes the strict pass may not recognise.

2. Test case inline comments:

   // The bug this work item targets: codex on a worktree whose slug contains "claude".
   // ...
   // Same shape, but with a quoted prompt...
   // ...
   // this is the live scenario for this branch.

   These are fine for the PR description (where they are!) but will be noise in the test file. Prefer short descriptions that stand alone: e.g. 'codex: claude in worktree slug', 'codex: claude in prompt', 'codex: claude in install path', etc. The table-driven case name is already the right place for that context.

---

Potential Issues

1. Quote stripping regex is greedy and not nested-aware (minor, low risk)

const stripped = argsLower.replace(/'[^']*'/g, '').replace(/"[^"]*"/g, '');

shellQuote always produces well-formed single-quoted spans so this is fine in practice. But if ps args= ever returns malformed output (e.g. an unclosed quote from a process that was mid-exec), the greedy match can eat more than intended and the strict pass would miss the binary. The fallback handles this gracefully, so it's not a correctness risk — just worth noting.

2. TOOL_NAMES ordering is load-bearing and fragile (worth a brief note)

The existing comment is good:

// Iteration order is load-bearing for the loose fallback below: when args contains
// substrings of multiple tool names, the first hit wins.

Worth extending this note to call out the specific risk: if a future tool name is a substring of another (e.g. a hypothetical tool named code alongside codex), the loose pass would silently misdetect. This is an existing limitation carried forward, but documenting the risk in the comment helps whoever adds the next tool.

3. Module-level escapeRe constant — should be a named function or marked private-by-convention

escapeRe is a module-level arrow function living in a module that only exports AIToolService. It's not exported, so this is fine. Just noting that it sits outside the class, which is consistent with TOOL_NAMES and TOOL_TOKEN_RES. No change needed.

---

Test Coverage

The 13 new cases are comprehensive and cover exactly the acceptance criteria from requirements.md:

• Codex: claude in slug, claude in quoted prompt, claude in install path ✓
• Gemini: claude in prompt, claude in install path ✓
• Claude: quoted display name, bash-wrapper resume/fresh shape ✓
• Case-insensitive variants ✓
• Non-tool processes (bash, vim) → 'none' ✓
• Legacy fallback path (someweirdtoolnameclaudethingembedded) ✓

One case that isn't covered: what if the tool name appears in both the strict and loose positions — e.g. bash -c codex --prompt 'fix codex bug'. This would be detected correctly by the strict pass, but it's a harmless gap.

---

Security & Performance

No concerns. The regex matching is O(n) on a short string; the regexes are compiled once. No external input reaches any privileged operation through this path.

---

Summary

This is a solid, focused fix. The main actionable feedback:

1. Remove branch/task references from the function comment and test case comments — replace with timeless descriptions of the invariant (per AGENTS.md conventions).
2. Consider extending the TOOL_NAMES ordering comment to mention the substring-collision risk.

Everything else looks correct, well-tested, and consistent with project conventions. ✓

[claude]

Code Review — PR #239: fix(ai-detection): match binary tokens, not loose substrings

Overview

Fixes a real bug: detectToolFromArgs used bare .includes('claude') so any process whose args contained the string "claude" (in a slug, prompt, or install path) would be misidentified as Claude regardless of the actual binary. The fix introduces a strict token-boundary pass followed by the legacy substring fallback.

The core logic is correct and well-tested. A few points below.

---

Issues

Minor — comment verbosity vs. project style

AGENTS.md says: "prefer a single line … don't explain WHAT the code does."

The module-level block comment on TOOL_NAMES is genuinely warranted — the iteration-order dependency is non-obvious. But the detectToolFromArgs method comment repeats what the code already says:

// Strict pass first: a tool name inside a prompt, slug, or install path must not
// outrank the actually-running binary. Falls back to loose `.includes()` for legacy
// invocation shapes the strict pass may not recognize.

The first sentence is fine (the WHY). The second sentence explains the WHAT of the fallback, which a reader can already see in the code. Consider trimming to just the WHY.

Similarly, the inline comment on stripped:

// shellQuote uses single quotes for non-safe args; strip those plus double-quoted spans
// so prompt/display text can't be mistaken for a binary token.

Two lines; the second line restates what the regex does. One line would suffice.

Minor — the loose fallback preserves a wrong detection

The test case 'legacy embedded substring', 'someweirdtoolnameclaudethingembedded', 'claude' explicitly validates that someweirdtoolnameclaudethingembedded still resolves to 'claude'. That was the original bug — it was never a correct detection, just an incidental one. The requirements doc says to preserve it for safety, and that's a reasonable call, but the comment calling it "historically-correct" may mislead future readers who assume this was intentional behavior rather than a deliberate choice to keep a safety net. Consider: "fallback: matches accidentally-correct legacy forms; latent bug, kept for safety" or just note it's intentional in the comment.

Suggestion — missing ambiguous-token test case

There's no test for args that contain two different tools as valid tokens (e.g., claude --help codex). Per the ordering comment, claude would win. This edge case is unlikely in practice but would serve as a regression guard for the load-bearing ordering invariant.

---

Strengths

• Strict-then-loose approach is exactly right: new cases are fixed, nothing regresses.
• TOOL_TOKEN_RES compiled once at module load — good.
• Tests exercise the private method through the public API, matching established test-file patterns.
• 13 table-driven cases cover all acceptance criteria.
• Tracker docs are thorough and the decision rationale is well-explained.

---

Summary

Approve with the above as optional suggestions. The fix is correct, minimal, and well-tested. The comment style is the only thing that pushes against project conventions, and that's a minor point.