Skip to content

Commit 850232f

Browse files
agent-era-aiclaude
andauthored
fix(ai-detection): match binary tokens, not loose substrings (#239)
* fix(ai-detection): match binary tokens, not loose substrings `AIToolService.detectToolFromArgs` was using `.includes('claude'|'codex'|'gemini')` on the full `ps args=` string, with claude checked first, which mis-detected codex (or gemini) as claude whenever the args string contained "claude" anywhere — a quoted prompt, a worktree slug like `agent-shows-claude-not-codex`, or an install path under `~/.claude*`. Strict pass first: strip shell-quoted spans (single + double-quoted argument bodies), then match each tool name only at a token boundary (`(?:^|[\s/])name(?=\s|$)`). The original `.includes()` chain is kept as a fallback for legacy invocation shapes the strict regex may not recognize. Token regexes are built once at module load from `AI_TOOLS` keys, so a new tool added to the config is detected automatically. Adds 13 table-driven cases under `detectAllSessionAITools`: codex/gemini on a claude-bearing slug, codex/gemini with a claude-bearing prompt (quoted by shellQuote), codex/gemini under a claude-bearing install path, the bash-wrapper resume/fresh shape, case-insensitive variants, and the legacy fallback path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review comments Trim multi-paragraph JSDoc on `detectToolFromArgs` and the `what`-describing lines from the module-level comment block, per AGENTS.md ("Multi-paragraph JSDoc... should be rare", "Don't explain WHAT the code does"). Keep the load-bearing notes: the bug context, the shellQuote/quote-stripping rationale, and the iteration-order dependency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review: escape tool names in regex builder Defensive: regex-escape the tool name before interpolation in TOOL_TOKEN_RES. All current tool names (claude, codex, gemini) are regex-safe, but a future addition with `.`, `+`, `(`, etc. would silently change the strict-match semantics without it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review: drop task/branch references in comments Per AGENTS.md: don't reference the current task, fix, or callers in code comments. Trim function comment to the timeless invariant; rename test cases to short stand-alone labels and consolidate two WHY notes into one header covering shellQuote behaviour for the table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ea4930d commit 850232f

5 files changed

Lines changed: 204 additions & 13 deletions

File tree

src/services/AIToolService.ts

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,15 @@ const CLAUDE_WAITING_RE = /❯\s+\d+\.\s+\w+/m;
1212
// for long-running operations, broaden to `/…\s*\(\d+(s|m)/`.
1313
const CLAUDE_WORKING_RE = /\s*\(\d+s/;
1414

15+
// Iteration order is load-bearing for the loose fallback below: when args contains
16+
// substrings of multiple tool names, the first hit wins. Object.keys preserves insertion
17+
// order, so reordering AI_TOOLS in constants.ts changes that priority silently.
18+
const TOOL_NAMES = Object.keys(AI_TOOLS) as Array<keyof typeof AI_TOOLS>;
19+
const escapeRe = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
20+
const TOOL_TOKEN_RES: Record<keyof typeof AI_TOOLS, RegExp> = Object.fromEntries(
21+
TOOL_NAMES.map(name => [name, new RegExp(`(?:^|[\\s/])${escapeRe(name)}(?=\\s|$)`)])
22+
) as Record<keyof typeof AI_TOOLS, RegExp>;
23+
1524
export class AIToolService {
1625
/**
1726
* Get tool name for display
@@ -80,22 +89,21 @@ export class AIToolService {
8089
return toolsMap;
8190
}
8291

83-
/**
84-
* Detect AI tool from process arguments
85-
*/
92+
// Strict pass first: a tool name inside a prompt, slug, or install path must not
93+
// outrank the actually-running binary. Falls back to loose `.includes()` for legacy
94+
// invocation shapes the strict pass may not recognize.
8695
private detectToolFromArgs(args: string): AITool {
8796
const argsLower = args.toLowerCase();
88-
89-
if (argsLower.includes('/claude') || argsLower.includes('claude')) {
90-
return 'claude';
91-
}
92-
if (argsLower.includes('/codex') || argsLower.includes('codex')) {
93-
return 'codex';
97+
// shellQuote uses single quotes for non-safe args; strip those plus double-quoted spans
98+
// so prompt/display text can't be mistaken for a binary token.
99+
const stripped = argsLower.replace(/'[^']*'/g, '').replace(/"[^"]*"/g, '');
100+
101+
for (const tool of TOOL_NAMES) {
102+
if (TOOL_TOKEN_RES[tool].test(stripped)) return tool;
94103
}
95-
if (argsLower.includes('/gemini') || argsLower.includes('gemini')) {
96-
return 'gemini';
104+
for (const tool of TOOL_NAMES) {
105+
if (argsLower.includes(tool)) return tool;
97106
}
98-
99107
return 'none';
100108
}
101109

tests/unit/AIToolService.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {AIToolService} from '../../src/services/AIToolService.js';
22
import {AI_TOOLS} from '../../src/constants.js';
3+
import type {AITool} from '../../src/models.js';
34

45
// Mock the command execution functions
56
jest.mock('../../src/shared/utils/commandExecutor.js', () => ({
@@ -82,11 +83,48 @@ random-session:33333`);
8283
});
8384

8485
const result = await aiToolService.detectAllSessionAITools();
85-
86+
8687
expect(result.get('dev-project-feature')).toBe('claude');
8788
expect(result.has('other-session')).toBe(false);
8889
expect(result.has('random-session')).toBe(false);
8990
});
91+
92+
describe('disambiguates tool when args mention another tool name', () => {
93+
// shellQuote leaves chars like `[A-Za-z0-9_\-./=:]+` bare; anything with spaces or
94+
// special chars is wrapped in single quotes. Both shapes appear in the cases below.
95+
const cases: Array<[string, string, AITool]> = [
96+
['codex: claude in worktree slug (unquoted)', `bash -c codex resume --last agent-shows-claude-not-codex || codex agent-shows-claude-not-codex`, 'codex'],
97+
['codex: claude in quoted prompt', `bash -c codex resume --last 'fix the claude bug' || codex 'fix the claude bug'`, 'codex'],
98+
['codex: claude in install path', `node /home/user/.claude-tools/codex/bin/codex resume --last`, 'codex'],
99+
['gemini: claude in quoted prompt', `bash -c gemini --resume latest 'tame the claude noise' || gemini 'tame the claude noise'`, 'gemini'],
100+
['gemini: claude in install path', `node /home/user/.claude-tools/gemini/bin/gemini --resume latest`, 'gemini'],
101+
['claude: quoted display name', `claude -n 'feature - project' --dangerously-skip-permissions`, 'claude'],
102+
['claude: bash-wrapper resume/fresh', `bash -c claude --continue -n 'foo' || claude -n 'foo'`, 'claude'],
103+
['case-insensitive: uppercase CLAUDE', 'CLAUDE', 'claude'],
104+
['case-insensitive: uppercase path codex', '/USR/BIN/CODEX', 'codex'],
105+
['non-tool: bash', 'bash', 'none'],
106+
['non-tool: vim', 'vim', 'none'],
107+
// Loose fallback path: no token boundary present, but the substring still resolves
108+
// the way it did before strict matching was introduced.
109+
['legacy embedded substring', 'someweirdtoolnameclaudethingembedded', 'claude'],
110+
];
111+
112+
for (const [name, argsLine, expected] of cases) {
113+
test(name, async () => {
114+
(runCommandQuickAsync as jest.Mock).mockImplementation((cmdArgs: string[]) => {
115+
if (cmdArgs.includes('list-panes') && cmdArgs.includes('-a')) {
116+
return Promise.resolve('dev-p-f:99999');
117+
}
118+
if (cmdArgs.includes('-p') && cmdArgs.includes('99999')) {
119+
return Promise.resolve(` 99999 ${argsLine}`);
120+
}
121+
return Promise.resolve('');
122+
});
123+
const result = await aiToolService.detectAllSessionAITools();
124+
expect(result.get('dev-p-f')).toBe(expected);
125+
});
126+
}
127+
});
90128
});
91129

92130
describe('getStatusForTool', () => {
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Implementation — agent-shows-claude-not-codex
2+
3+
## What was built
4+
5+
Tightened `AIToolService.detectToolFromArgs` (`src/services/AIToolService.ts:86-117`) so that tool names inside a prompt, slug, or install path no longer outrank the actually-running binary.
6+
7+
**Two-pass detection:**
8+
1. **Strict pass** — strip shell-quoted spans (`'...'`, `"..."`) from the lowercased args, then for each tool match a word-boundary regex `(?:^|[\s/])${tool}(?=\s|$)`. The binary is recognized only when it appears as a standalone token (start of string, after `/`, or after whitespace, and followed by whitespace or end of string).
9+
2. **Loose fallback** — if the strict pass returns `'none'`, fall back to the original `.includes()` chain. Preserves any historically-correct legacy detection forms; latent bugs in the loose path were already there before this change.
10+
11+
**Non-changes:**
12+
- `isAIPaneCommand` left as-is — it sees only the process command name (not full args), and its coarseness is intentional for pane shortlisting.
13+
- `getStatusForTool`, `isWorking`, `isWaitingForTool`, and `AI_TOOLS` config unchanged.
14+
- No tmux launch, fallback chain, or `aiSessionMemory` changes.
15+
16+
## Tests
17+
18+
Added 13 table-driven cases under `detectAllSessionAITools › disambiguates tool when args mention another tool name` in `tests/unit/AIToolService.test.ts`:
19+
20+
- Codex on a worktree slug containing "claude" (the live repro for this branch).
21+
- Codex with a quoted prompt containing "claude" (shellQuote single-quotes any prompt with spaces).
22+
- Codex installed under a path containing "claude".
23+
- Gemini parallels (claude-bearing prompt; claude-bearing install path).
24+
- Claude with quoted display name.
25+
- Claude bash-wrapper resume/fresh shape.
26+
- Case-insensitive variants (`CLAUDE`, `/USR/BIN/CODEX`).
27+
- Non-tool processes (`bash`, `vim`) → `'none'`.
28+
- Legacy fallback path (`someweirdtoolnameclaudethingembedded`) still resolves to `'claude'`.
29+
30+
Existing cases (`claude`, `/usr/bin/claude`, `node /usr/bin/codex`, `node /usr/bin/gemini`, the `detects AI tools across multiple sessions` fixture) all continue to pass.
31+
32+
## Key decisions
33+
34+
- **Quote stripping in the strict pass.** `shellQuote` always wraps args containing spaces in single quotes, so a prompt like `'fix the claude bug'` would otherwise space-border "claude" and the strict regex would still match it. Stripping `'...'` (and defensively `"..."`) in the strict-pass input cleanly removes prompt/display content from tool detection while leaving the binary tokens intact.
35+
- **Strict-then-loose, not strict-only.** Per requirements decision: any quietly-correct legacy detection form (e.g. `someweirdtoolnameclaudethingembedded`) keeps resolving as it did. The fallback also gives us a safety net if the args shape ever changes in unexpected ways across platforms or tmux versions.
36+
- **Test through `detectAllSessionAITools`.** `detectToolFromArgs` is private; rather than expose it for tests, the new cases drive it via the public path with mocked `tmux list-panes` + `ps` output. The existing test file already established that pattern.
37+
38+
## Test/typecheck status
39+
40+
- `npx jest tests/unit/AIToolService.test.ts` → 38/38 pass (13 new).
41+
- `npx jest` (full suite) → 78 suites, 801/801 pass.
42+
- `npx tsc -p tsconfig.test.json` → clean.
43+
44+
## Notes for cleanup
45+
46+
- Comment in the new function explains *why* (the bug) without restating *what* (the regex). Should survive a review pass.
47+
- No documentation files reference `detectToolFromArgs`. README/AGENTS.md don't need updates.
48+
- No new exports or public API surface — change is internal to `AIToolService`.
49+
50+
## Stage review
51+
52+
Strict-then-loose detection in `AIToolService.detectToolFromArgs`, single fix point. 13 new table-driven cases plus the original 9 still green; full suite (801 tests) and typecheck clean. No commits made yet — leaving that for cleanup so the user can decide on commit granularity.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# Discovery — agent-shows-claude-not-codex
2+
3+
## Problem
4+
5+
When a worktree is running Codex (or Gemini), the kanban / status UI sometimes labels the agent as "Claude" instead of the tool that is actually attached. This was hit on this very branch: a Codex session launched with the tracker prompt for slug `agent-shows-claude-not-codex` is misdetected as `claude`.
6+
7+
## Findings
8+
9+
**Root cause**`AIToolService.detectToolFromArgs` uses a loose substring match:
10+
11+
```ts
12+
// src/services/AIToolService.ts:86-100
13+
const argsLower = args.toLowerCase();
14+
if (argsLower.includes('/claude') || argsLower.includes('claude')) return 'claude';
15+
if (argsLower.includes('/codex') || argsLower.includes('codex')) return 'codex';
16+
if (argsLower.includes('/gemini') || argsLower.includes('gemini')) return 'gemini';
17+
```
18+
19+
The bare `.includes('claude')` matches any occurrence of the literal string anywhere in the process command line — not just the binary name. Claude is also checked first, so any string containing "claude" wins over "codex" / "gemini".
20+
21+
**Where args comes from**`detectAllSessionAITools` reads `pane_pid` for each `dev-*` tmux session, then `ps -p <pid> -o args=` returns that process's command line (`AIToolService.ts:37-81`). For the resume-or-fresh chain (`WorktreeCore.launchAISessionWithFallback`, line 620), `tmux new-session ... '<resume> || <fresh>'` causes the pane's first process to be `/bin/sh -c "<resume> || <fresh>"`. So the args string includes the full resume + fresh commands, all flags, and `initialPrompt` if any — and any of those substrings can contain "claude".
22+
23+
**Reproduction** — verified with the exact detection function:
24+
25+
```
26+
detectToolFromArgs("bash -c codex resume --last 'fix claude bug' || codex 'fix claude bug'")
27+
→ "claude" (BUG: actual tool is codex)
28+
29+
detectToolFromArgs("bash -c codex resume --last 'agent-shows-claude-not-codex' || codex 'agent-shows-claude-not-codex'")
30+
→ "claude" (BUG: this is the live scenario for this branch)
31+
32+
detectToolFromArgs("node /home/user/.claude-tools/codex/bin/codex")
33+
→ "claude" (BUG: codex installed under a path containing 'claude')
34+
```
35+
36+
The opposite direction (claude misdetected as codex) doesn't trigger, because the `claude` binary name virtually always appears in a Claude pane's args before any `codex`/`gemini` substring.
37+
38+
**Triggers in practice**
39+
1. `initialPrompt` passed to `launchAISessionWithFallback` (line 615) when the prompt text contains "claude" — common for tracker items whose slug or description mentions Claude (e.g. this branch).
40+
2. `displayName` for Claude is set to `${feature} - ${project}` (`WorktreeCore.ts:403`); it's only attached for claude (`-n` flag), so it can't poison codex args. But fragments of feature/project names that contain "claude" can still arrive via `initialPrompt`.
41+
3. Codex/Gemini installed under a path that contains "claude" (less common but possible — e.g. a `~/.claude*` shared tools directory).
42+
43+
**Test coverage gap**`tests/unit/AIToolService.test.ts` only exercises clean fixtures (`node /usr/bin/codex`, `claude`, `node /usr/bin/gemini`). No test covers args strings that mix tool names or include prompt text.
44+
45+
## Recommendation
46+
47+
Tighten `detectToolFromArgs` to match the *binary*, not any substring. Two viable approaches:
48+
49+
**A. Word-boundary regex on the executable basename.** Match `claude`, `codex`, `gemini` only when they appear as a standalone token (start of string, after `/`, after whitespace) and are followed by whitespace or end-of-string. That excludes occurrences inside `agent-shows-claude-not-codex` or `--prompt 'fix claude'`.
50+
51+
**B. Parse the executable token explicitly.** Strip a leading `bash -c "..."` / `sh -c "..."` wrapper, take the first non-`node` token, and `path.basename()` it. Compare against the known set.
52+
53+
Recommendation: **A**. It's a one-line change to a single regex per tool, keeps the existing structure, and is robust against the `bash -c "<resume> || <fresh>"` shape without us having to faithfully tokenize the inner command line. A quote-unaware parser (B) gets brittle around prompt text with embedded quotes, whereas a regex like `/(?:^|[\s/])claude(?=\s|$)/` cleanly says "the binary, anywhere it might appear, but never as a substring inside another token". Order independence falls out of this for free.
54+
55+
Tests to add alongside the fix: codex launched with claude-bearing prompt; codex installed at a claude-bearing path; the bash-wrapper resume/fallback shape; gemini in the same scenarios.
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# Requirements — agent-shows-claude-not-codex
2+
3+
## Problem
4+
5+
When a worktree is running Codex (or Gemini), the kanban / status UI sometimes labels the agent as "Claude" instead of the tool that is actually attached. This was hit on this very branch: a Codex session launched with the tracker prompt for slug `agent-shows-claude-not-codex` is misdetected as `claude`.
6+
7+
## Why
8+
9+
`AIToolService.detectToolFromArgs` (`src/services/AIToolService.ts:86-100`) decides the tool by `argsLower.includes('claude' | 'codex' | 'gemini')` against the full `ps -o args=` output, with `claude` checked first. The args string for a fallback-chain pane is `/bin/sh -c "<resume> || <fresh>"`, so any `initialPrompt`, slug, install-path, or display fragment containing "claude" wins over the actually-running binary. Triggers seen: tracker prompts that quote a claude-bearing slug, codex installed under a path containing "claude", or any prompt text mentioning Claude.
10+
11+
## Summary
12+
13+
Replace the loose substring match in `detectToolFromArgs` with a binary-aware match — a word-boundary regex anchored on the executable token, so `claude` / `codex` / `gemini` are recognized only when they appear as a standalone token (start of string, after `/`, or after whitespace) and are followed by whitespace or end-of-string. Preserve the existing `.includes()` behaviour as a final fallback only when the strict pass returns `'none'`, so any quietly-correct legacy detections aren't regressed. Keep `isAIPaneCommand` unchanged. Add unit tests covering the broken cases and the bash-wrapper fallback shape.
14+
15+
## Acceptance criteria
16+
17+
### Detection correctness
18+
19+
1. Given `ps args` of `bash -c "codex resume --last 'agent-shows-claude-not-codex' || codex 'agent-shows-claude-not-codex'"`, `detectToolFromArgs` returns `'codex'`.
20+
2. Given `ps args` of `bash -c "codex resume --last 'fix the claude bug' || codex 'fix the claude bug'"`, `detectToolFromArgs` returns `'codex'`.
21+
3. Given `ps args` of `node /home/user/.claude-tools/codex/bin/codex resume --last`, `detectToolFromArgs` returns `'codex'`.
22+
4. Given `ps args` of `bash -c "gemini --resume latest 'tame the claude noise' || gemini 'tame the claude noise'"`, `detectToolFromArgs` returns `'gemini'`.
23+
5. Existing positive cases keep returning the same tool: `claude`, `/usr/bin/claude`, `node /usr/bin/codex`, `node /usr/bin/gemini` (matching today's `AIToolService.test.ts:43-67`).
24+
6. When the strict pass finds no binary token, the function falls back to today's `.includes()` behaviour (claude → codex → gemini → none) and returns the same answer it does today, so any legacy invocation forms still resolve.
25+
7. Args that match no tool — `bash`, `vim`, empty string — return `'none'`.
26+
8. Matching is case-insensitive (e.g. `CLAUDE`, `/USR/BIN/CODEX` resolve correctly).
27+
28+
### Scope and non-changes
29+
30+
9. `isAIPaneCommand` is unchanged — its substring behaviour is intentional and used only for coarse pane shortlisting.
31+
10. `getStatusForTool`, `isWorking`, `isWaitingForTool`, and `AI_TOOLS` config are unchanged.
32+
11. No change to tmux launch, fallback chain, or `aiSessionMemory` behaviour.
33+
34+
### Tests
35+
36+
12. `tests/unit/AIToolService.test.ts` adds table-driven cases for ACs 1–4 and 7–8, plus an explicit case asserting that the legacy fallback path still resolves a "weird but historically-OK" args string (AC 6). Gemini gets parallel coverage to Codex: claude-in-prompt, claude-in-install-path, and the bash-wrapper resume-or-fresh shape.
37+
13. The existing `detectAllSessionAITools` test continues to pass with no fixture changes.
38+
14. `npm run typecheck` and `npm test` are green.

0 commit comments

Comments
 (0)