Skip to content

Fix agent detection for long argv0 paths - #471

Merged
BenevolentFutures merged 2 commits into
mainfrom
c11-246-argv0-detect
Oct 1, 2026
Merged

BenevolentFutures merged 2 commits into
mainfrom
c11-246-argv0-detect

Conversation

@BenevolentFutures

Copy link
Copy Markdown
Contributor

What changed

Agent detection now classifies a foreground process from its full argv[0] and from proc_pidpath, not from the 16-character ps comm column.

Why

A claude binary at a long absolute path was classified unknown, while the same binary at /tmp/fb/claude (13 characters) was claude-code. Two things stacked:

  • ps -o comm prints argv[0] in a column capped at 16 characters, so a long path loses its basename.
  • parsePSLine counted every padding space in the tty column as its own field. The reconstructed args started at tpgid, the argv0 token was 0, and the real path was ignored.

/tmp/fb/claude still fit in the comm column, so it never needed the broken fallback. proc_pidpath is one syscall per foreground pid; the sweep still spawns ps once.

How it was validated

Unit tests feed process facts and captured ps lines (padded tty column, 16-character comm clip) through classify / parsePSLine. They cover the short path, a long directory ending in claude, a basename longer than 16 characters, and a long node path whose script is still the agent. CI's build job compiles and runs them. No local xcodebuild.

Closes C11-246

🤖 Generated with Grok Build

ps clips comm to 16 characters, and parsePSLine counted each padding space as its own column, so the argv0 fallback read "0" instead of the executable. Classify from the full argv line and from proc_pidpath.

Co-Authored-By: Grok 4.7 <noreply@x.ai>
@github-actions github-actions Bot added the drawbridge:review Drawbridge: routed to maintainer review label Oct 1, 2026
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR fixes agent detection for foreground processes whose argv[0] is a long absolute path. It replaces the single comm/argv0-based classify(comm:args:) with a ProcessFacts-based classifier that considers comm, its basename, the proc_pidpath-resolved executable basename, and the argv0 basename — and repairs a parsePSLine bug that over-counted padded whitespace runs. I traced the precedence, the whitespace rewrite, the registry lookups the tests depend on, and the live scan path; the change is correct, well-scoped, and preserves the existing safety property. Thank you for the unusually clear root-cause writeup — the tpgid-as-argv0 diagnosis made this fast to verify.

Alignment

Squarely in scope. This is a correctness bug fix in an agent-facing primitive — the Module-1 detector that drives terminal_type metadata — exactly the "bug fixes … agent-facing primitives" the policy calls out as wanted. No tenant-config writes, no display links or draw loops, no CLI/socket/surface-model change (so no skills/c11/** update is owed), no user-facing strings (localization N/A), and it stays on c11-specific code rather than diverging from a shared upstream path. The telemetry hot-path and focus-stealing policies don't apply — this runs on the existing utility-queue sweep and touches no focus state.

Findings

I went hunting for regressions in the classifier precedence, a false-positive path through the broadened runtime gate, and the parsePSLine rewrite. No blocker or major. Details below.

Verified correct:

  1. parsePSLine whitespace fix is right. The old for idx in trimmed.indices loop ate a whitespace run with a local cursor but then let the outer loop resume at idx+1, so a multi-space pad counted each space as its own split — on a padded ttysNNN/?? line the 5th boundary landed inside tpgid and argv0 became the literal "0". The new single-cursor while advances index across the whole run and counts each run once, so exactly 5 runs separate the 6 columns and argsStart lands at the start of args. The parts.count >= 6 guard guarantees splits reaches 5, so the splits >= 5 fallback is dead but harmless. I confirmed this against the new testParsePSLineTTYTwoSpacePadUsesFullArgv0 case by hand — it reconstructs args correctly. (Sources/AgentDetector.swift parsePSLine)

  2. The direct-match safety property is preserved. The classifier still only consults comm, its basename, the executablePath basename, and the argv0 basename — never later argv tokens. Arbitrary user arguments still cannot classify a process as an agent except on the interpreter rail (unchanged). (classify(_:))

  3. Interpreter-rail gating is correctly broadened. The old gate matched the full comm (c == "node"), which silently failed when comm was a clipped long runtime path. names.contains(where: isRuntime) now picks the runtime up from the basename (or proc_pidpath) — the actual fix for the long-node-path case — while the substring and token-basename matches inside the rail are unchanged. I verified the copilot test resolves via the "/copilot" substring in detectNodeArgsSubstrings, and the opencode test via the opencode-cli basename arriving on executablePath.

  4. proc_pidpath usage is sound. pidPathCapacity = 4 * MAXPATHLEN matches PROC_PIDPATHINFO_MAXSIZE (4096); the zero-filled [CChar] buffer is null-terminated by the syscall, length > 0 correctly rejects the "pid gone" case, and the scan populates info.executablePath once per foreground pid before classify. import Darwin is the right home for both the macro and proc_pidpath.

  5. Registry lookups the new tests depend on all exist — claude/claude-code, opencode/opencode-cli, copilot's /copilot substring — so the four new cases exercise the real registry, not a stub.

Minor / nits (non-blocking):

  1. nit — the shell fallback now considers argv0/executablePath basenames, not just comm. The final loop checks every name against canonicalShells, so a process whose comm basename isn't a shell but whose argv0/executablePath basename is (e.g. a long-path login shell comm-clipped, or the contrived node …/bin/sh) now classifies shell where the old code returned unknown. For real login shells at long paths this is an improvement and consistent with the PR's intent; the only odd case is an interpreter literally executing a script named sh/bash, which is both pathological and harmless (it isn't an agent either way). Flagging only so the behavior widening is on the record — no change requested.

  2. nit — test placement. c11Tests/AgentDetectorTests.swift is in the host-required c11Tests target, but the new cases exercise only pure logic (classify / parsePSLine / executablePath). Per CLAUDE.md's testing split, pure-logic tests ideally live in c11LogicTests for the fast local loop. Since you're extending a file already in c11Tests, moving it is out of scope here — latent cleanup, not a fix for this PR. (They still run in CI's build job, which runs both targets.)

  3. nit — per-sweep allocation. executablePath(for:) allocates a 4 KB [CChar] per foreground pid per sweep. This runs on the utility queue on the 10 s sweep / kick path — not a typing-latency hot path, and GCD drains the autorelease pool between work items (so the long-lived-Thread autoreleasepool pitfall doesn't apply), and the [CChar] is stack-scoped Swift memory, not autoreleased. Fine as-is; noting it so a future reader doesn't re-derive that it's off the hot path.

No prompt-injection attempts in the diff or the (single, automated) discussion comment.

Recommendation

Merge as-is, assuming CI's required checks (notably build, which compiles and runs these tests) are green — the fix is correct, well-reasoned, and well-tested, and the three nits are optional follow-ups rather than changes this PR needs. Nicely done.


🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.


🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.

runPS always passes -t, so live lines are ttysNNN with a two-space pad, and counting every space landed the argv boundary on comm. Lock that fixture, and call proc_pidpath for this process.

Co-Authored-By: Grok 4.7 <noreply@x.ai>
@BenevolentFutures
BenevolentFutures merged commit f767f30 into main Oct 1, 2026
11 checks passed
@BenevolentFutures
BenevolentFutures deleted the c11-246-argv0-detect branch October 1, 2026 06:08
BenevolentFutures pushed a commit that referenced this pull request Oct 1, 2026
Carries #469's destructive-target guards onto the new vocabulary: tab.close and tab.action reject unresolved refs, rejectEmptyTargetFlags checks the --area/--tab alias groups, and the guard's error text names canonical keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@BenevolentFutures BenevolentFutures mentioned this pull request Oct 1, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drawbridge:review Drawbridge: routed to maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant