Repository navigation
fix: align detection parity across TS, Rust and Python - #7
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (4)Rust source lives in `crates/agenthint/src/`📄 CodeRabbit inference engine (AGENTS.md) Files:
If editing `install.sh`, run `sh -n install.sh` and keep it POSIX `sh` compatible📄 CodeRabbit inference engine (AGENTS.md) Files:
TypeScript source lives in `src/` Keep detection results explainable: include `confidence` and `signals` Prefer explicit `AI_AGENT` support over heuristics Never print environment variable values that may contain secrets; signal names are e...📄 CodeRabbit inference engine (AGENTS.md) Files:
Prefix shell commands with `rtk` unless the command genuinely needs raw shell behavior📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 ast-grep (0.45.3)test/python_test.py[info] 99-99: Do not hardcode temporary file or directory names (hardcoded-tmp-file) src/index.ts[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🪛 Ruff (0.16.4)test/python_test.py[error] 100-100: Probable insecure usage of temporary file or directory: "/tmp/codex" (S108) 🔇 Additional comments (12)
📝 SummarySummary by CodeRabbit
WalkthroughThe change updates environment signal matching across Rust, TypeScript, Python, and shell code. It adds cowork signal reporting, whitespace filtering, deterministic ordering, case-insensitive overrides, and tie-breaking. It also rejects non-leading ChangesDetection and CLI behaviour
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The detection and CLI behavior updates are covered by matching implementation changes, documentation, fixtures, and regression tests across supported languages. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.93% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 9 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
F1: prefer earliest rule on confidence ties in Rust F2: sort prefix signals in TS and Python to match Rust F3: document stdio hints as opt-in library signals only F4: fix Python .exe stripping to be case-insensitive F5: make install.sh truthy check case-insensitive F6: fix Rust CLI non-leading init error path F7: ignore whitespace-only heuristic env values F8: include cowork classifier signal alongside trigger
41640b7 to
df9618d
Compare
Code review
Build, lint and all three test suites pass on this branch: TS (31 tests), Rust (20 unit + 3 CLI, plus fmt and clippy) and Python (13 tests). Bug
This PR adds a SPEC rule: the Reproduction (clean env): deno run -A dist/cli.js --json </dev/null | cat
# => confidence: 0.2, signals: ["stdio:stdout-not-tty", "stdio:stdin-not-tty"]Calling Suggested fix: drop the Minor (not bugs)
|
Review — PR #7
|
Review — PR #7
|
| code point | TS | Rust | Python |
|---|---|---|---|
| U+FEFF (BOM) | ignored | detected: agent=U+FEFF, 0.98 |
detected: same |
| U+0085 (NEL) | detected: agent=U+0085, 0.98 |
ignored | ignored |
| all others tested* | agree | agree | agree |
* TAB, VT, FF, SPACE, NBSP, OGHAM SPACE, EN QUAD, FIG SPACE, LS, PS, NNBSP, MMSP, IDEO all ignored by all three. ZWSP (U+200B) and U+180E are treated as normal characters by all three, so the earlier note about ZWSP does not reproduce — the JS-only whitespace is U+FEFF, and U+0085 diverges in the opposite direction.
Heuristic example: CODEX_HOME=\uFEFF → TS reports no agent; Rust/Python report codex at 0.92. Cowork example: CLAUDE_CODE=1 + CLAUDE_CODE_IS_COWORK=\uFEFF → TS reports claude-code with one signal; Rust/Python report cowork with the classifier signal. This is exactly the kind of edge F7 is trying to pin down, so either fix the set in SPEC (for example ASCII \t\n\v\f\r) and implement it explicitly, or add shared fixtures so the difference is deliberate.
2. Python doctor capitalizes the "no agent" hint (minor, pre-existing)
AGENTHINT_DISABLE=1 python3 -m agenthint.cli doctor | grep hint
hint: Agents should set AI_AGENT=<agent-name> before invoking tools.
AGENTHINT_DISABLE=1 node dist/cli.js doctor | grep hint
hint: agents should set AI_AGENT=<agent-name> before invoking tools.
python/agenthint/__init__.py:114 reuses the JSON hint from :284 verbatim; src/doctor.ts:40 and format_doctor in crates/agenthint/src/lib.rs hardcode the lowercase human variant. doctor --json agrees across all three (capital A), so only the human output differs. fixtures/cli-cases.json has no "doctor, no agent" case, which is why nothing catches it.
3. Nit: --help output differs by one trailing blank line (pre-existing)
TS emits ...Show this help\n\n because printHelp console.logs a template literal that already ends in a newline; Rust and Python emit a single \n.
Confirming prior review notes
- Deno TTY fallback: independently reproduced on
deno 2.9.6withenv -i PATH="$PATH" deno run -A dist/cli.js --json→confidence: 0.2,signals: ["stdio:stdout-not-tty","stdio:stdin-not-tty"]. Node and Bun emit no signals.src/index.ts:256still falls back toprocess.stdout.isTTY, so the newSPEC.md:104sentence anddocs/signals.md:82-83overstate current TS behavior. Dropping the?? process.stdout.isTTY/?? process.stdin.isTTYfallbacks would make the docs true and align TS with Rust/Python. - Test duplication: the new ties, sorted-prefix and cowork tests restate fixture cases already run by all three suites (
fixtures/detection-cases.json:51,67,83). The genuinely new edges (whitespace-only,.EXE, mixed-case truthy) have no shared fixture and are only covered per language. Adding those three todetection-cases.jsonwould enforce parity rather than document it, and the duplicate unit tests could then go.
Verified correct
- F4:
name.lower().removesuffix(".exe")matches TS\.exe$/iand Rust lower-then-strip for ASCII names. - F5:
tr '[:upper:]' '[:lower:]'is POSIX and matches the librarylower()behavior. - F6:
foo init bar→ exit 2,invalid usage: foo init barin all three CLIs (verified). - F8: cowork promotion and the classifier signal are identical in all three, including
CLAUDECODEas the triggering signal. - F7 for the common ASCII cases: all three treat empty and
\t\n\rvalues as absent. - Exit codes 0/1/2 preserved.
No blocking bugs in the diff itself. The SPEC/TS mismatch from the Deno fallback is the only item I would want resolved or explicitly accepted before merge.
- TS: drop the process.stdout/stdin.isTTY fallbacks so stdio hints are truly opt-in (Deno reports isTTY=false for piped stdio). - Define a portable ASCII whitespace set in SPEC and trim with it in all three implementations; native trims disagreed on U+FEFF, U+0085 and U+001C-U+001F. - Python doctor: lowercase the human "no agent" hint to match TS/Rust. - TS --help: remove the extra trailing blank line. - Move whitespace, cowork, truthy and .EXE edge cases into shared fixtures (with parentProcessName support) and drop duplicate unit tests. - Add CLI fixtures for --help, doctor with no agent and whitespace-only init names. - Remove comments that restated the spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TYKH18k7c9ZUzJcWB3vMfq
1 participant
Aligns detection and CLI behaviour across the TypeScript, Rust and Python implementations, and addresses the review feedback on this PR.
Behaviour changes in this PR
process.stdout.isTTY/process.stdin.isTTYfallbacks. Deno reportsisTTY === falsefor piped stdio, sodetectAgent()and the CLI were emittingstdio:*-not-ttyhints unasked, contrary to SPEC and unlike Rust/Python.\t,\n,\v,\f,\r. All three implementations trim exactly that set forAI_AGENT,AGENTHINT_AGENT, heuristic env values, parent process names andinit <name>. Native trims disagreed: JS trims U+FEFF, Rust/Python trim U+0085, Python also trims U+001C–U+001F..exestripping is case-insensitive (Codex.EXE→codex).install.shtruthy check is case-insensitive viatr.doctornow prints the lowercase human "no agent" hint, matching TS/Rust.--helpno longer prints an extra trailing blank line.Tie-break (earliest rule wins), sorted prefix signals, leading-only
initand the cowork classifier signal already landed onmainin 87fd407. This PR only locks them in with shared fixtures.Tests
fixtures/detection-cases.jsoncover ASCII vs non-ASCII whitespace, the cowork classifier, case-insensitive truthy overrides, and.EXEparent names (fixtures now acceptparentProcessName).fixtures/cli-cases.jsoncover--help,doctorwith no agent, and whitespace-onlyinitnames.processwithout explicit options.npm run checkpasses: 26 node, 15+3 rust, 7 python, plus fmt/clippy/biome.sh -n install.shpasses.Review items not changed
LC_ALL=Cfortrininstall.sh: the reported failure did not reproduce with GNU tr 9.11 under C, UTF-8, ISO8859-1 or Turkish locales, and no truthy value containsI.loweredas a global ininstall.sh: matches the rest of the script, where all helpers use globals;localis not POSIX.🤖 Generated with Claude Code
https://claude.ai/code/session_01TYKH18k7c9ZUzJcWB3vMfq