Repository navigation
test(claude): run the real Claude CLI tests only when opted in - #23702
Conversation
An installed, signed-in claude is not consent to spend turns on that account. The gate skips before probing the binary unless ORCA_REAL_CLAUDE_CLI_TEST=1.
The suite used to run whenever `claude --version` succeeded and `claude auth status` reported a login, so any broad local vitest run spent real turns on the developer's account and wrote transcripts into their Claude home. It now also requires ORCA_REAL_CLAUDE_CLI_TEST=1, and the skipped suite's title names the variable.
 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAdds an opt-in gate for tests that use the real Claude CLI. The gate skips without probing unless Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The two documented structured CLI suites do not use Claude unless explicitly opted in, and the separate supervised-stop suite has its own opt-in. No actionable merge-blocking risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The real-account tests now require an explicit opt-in before checking or using the Claude CLI. No new production entrypoint or broader account access was identified. The assessment is limited to the inspected test paths and does not establish runtime behavior for every test configuration. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Opt-in gate for the real Claude CLI suite —
resolveRealClaudeCliGatechecksORCA_REAL_CLAUDE_CLI_TEST=1before any subprocess, so a broad local run no longer probes or drives the developer's signed-inclaudeaccount. - Strict account report parsing —
parseAuthStatusdropsloggedIn/projectsDirectoryfields of the wrong type instead of casting, so an unrecognized shape degrades to a skip rather than a runtime throw. - Real suite rebased on the gate — the inline
spawnSyncprobes are replaced with the gate, anddescribe.skipIf(gate.skipReason !== null)plus a title that names the variable make an unset opt-in self-explanatory in test output. - Unit coverage for the gate — a fake
claudepins the skip-without-probing cases, probe order, missing-binary skip, failed/unparseable auth, and wrong-typed field dropping.
I ran the two files through --config config/vitest.config.ts: 9 gate tests pass and all 6 real-CLI cases report skipped with the variable unset, confirming the gate fires before any probe. I also confirmed no other suite under src/main/claude/ spawns the real binary (the connection and exit-proof suites use scripted/fake CLIs), so the scope claim holds.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Opt-in gate extracted — new
src/main/claude/claude-real-cli-test-gate.tsreturns a skip reason before any probe unlessORCA_REAL_CLAUDE_CLI_TEST === '1', then checksclaude --versionandclaude auth status --jsonwith a strict field parse. - Real suite gated —
claude-structured-real-cli.test.tsnow resolves the gate at module load and usesdescribe.skipIf(gate.skipReason !== null)with a reason-bearing title; signed-in cases still requireloggedIn === true. - Gate unit tests — 9 cases cover unset/
''/'0'/'true'skipping without probing, the opt-in probe order and payloads, a missing binary, failed/unparseable auth, and wrong-typed account fields. - AGENTS.md — records when to run the opt-in suite under Verifying Changes.
Verified locally: the gate suite passes (9/9), the change is test-only in practice (the gate has no production importer), and no central env-var doc lists the sibling ORCA_REAL_* variables, so no doc update is implied.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…3614) into #23059 Conflicts, resolved to keep main's mechanism and #23059's turn facts: - Codex turn boundaries and translator: main's #23682 wins; the error-driven `fail` end and the oversized-notification settler are gone, and only `turn/completed` ends a turn, a command's turn included. - Codex adapter: the compaction tracker stays deleted; #23618's settlement of sends in an ended turn is kept, and `codexDispatchRejection` is imported from its new home. - Journal reducer: main's per-item creating fence (#23456) moves into the item and submission folds this branch split out of the reducer. - Stale-turn verdict: main's proof-of-death revision; every turn revision states the thread scope. - Failure words: main's retry-control rules (#23608) with this branch's any-command retry step; `commandRefused` drops its retry step beside a Retry, as hostFault does. - Delivery: both the commit wake and main's death-evidence resettle and dispose. - Fold row and transcript slots: both the turn-outcome flag and main's `agentId`. - pr.yml and pnpm-lock.yaml taken from main. Beyond the conflicts, the merged files needed: - use-structured-agent-session: both sides added the same `submissions` key; one kept. - structured-agent-session-projection: the root-item import main still used is unused here. - The desktop message list and the mobile chat view each grew past the line limit from both sides' additions: the turn-membership memo moves into its own hook, and the mobile list footer (live status, then waiting rows) into its own helper.
main's #23621 moved the real-Claude probe into a shared support module that both real-CLI suites import, probing on import with no opt-in. That module now takes its availability from this PR's opt-in gate, so neither suite probes or runs the CLI unless ORCA_REAL_CLAUDE_CLI_TEST=1, and AGENTS.md names both suites.
Comments Outside DiffThese findings could not be posted inline.
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (79bc8f6), the branch merged main twice and reconciled with main's newer shared real-CLI support module:
- Shared probe now routes through the opt-in gate —
claude-real-cli-availability-test-support.tsderivesrealClaudeAvailableandrealClaudeAuthStatusfromresolveRealClaudeCliGate, so the newer fold suite, which consumes that module, also skips without ever probing whenORCA_REAL_CLAUDE_CLI_TESTis unset. - Real-suite title hoisted to a module const —
suiteTitlekeeps the skip reason on the main suite while avoiding a whole-file re-indent of the mergeddescribecall. - Opt-in gate unchanged —
claude-real-cli-test-gate.tsand its test are byte-identical to the prior review (git diff 79bc8f6...HEADon both is empty).
I ran the gate suite plus both real-CLI suites with the variable unset: 9 gate tests pass, the structured suite (7) and fold suite (1) report skipped, 1.2s total — the gate short-circuits before any claude spawn even through the shared support module. resolveClaudeCommand() still runs at import but only does filesystem path resolution; the already-opt-in claude-supervised-stop-real-cli suite is unaffected.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
ELI5
One test file talks to the real Claude command-line tool on your computer, using your own Claude account. Until now it ran by itself whenever it found Claude installed and signed in. Now it only runs when you ask for it by setting
ORCA_REAL_CLAUDE_CLI_TEST=1.What Changed
Before: running a broad local test command (a folder, the whole suite, or a file list that accidentally came out empty) also ran
src/main/claude/claude-structured-real-cli.test.tson any machine whereclaude --versionworked andclaude auth statusreported a login. That spent real turns on the developer's Claude account and left real transcripts in their Claude home folder. It has happened four times.After: that suite is skipped unless
ORCA_REAL_CLAUDE_CLI_TEST=1is set. The skipped suite's title says so:Claude structured real CLI handshake (skipped: set ORCA_REAL_CLAUDE_CLI_TEST=1 to run against the real claude CLI). With the variable set it behaves as before, except that it reads the account report strictly (a field of the wrong type is dropped instead of cast), including skipping whenclaudeis missing and skipping the signed-in cases when there is no login.Mechanism: a small gate,
resolveRealClaudeCliGateinsrc/main/claude/claude-real-cli-test-gate.ts, checks the variable first and returns before it runs anyclaudeprobe. Only when opted in does it runclaude --versionand thenclaude auth status --json, and it reads the account report with a checked parse instead of a cast. The test file passes in the process-spawning probe, so the gate itself spawns nothing and is unit-tested with a fake.Since this PR opened,
mainadded a second real-CLI suite (claude-structured-real-cli-fold.test.ts) and moved the probe both suites use intoclaude-real-cli-availability-test-support.ts, which ranclaudeon import with no opt-in. That shared module now takes its answer from the same gate, so neither suite runs, or even probes, the real CLI unlessORCA_REAL_CLAUDE_CLI_TEST=1is set, andAGENTS.mdnames both suites. Checked with the process runner mocked: with the variable unset nothing is probed and the suites report the CLI unavailable; with it set, the two probes run as before.The variable follows the naming the repo already uses for suites that touch real resources on the developer's machine:
ORCA_REAL_PROCESS_SKILL_TEST,ORCA_REAL_WSL_SKILL_TEST,ORCA_REAL_WINDOWS_SKILL_TEST, each checked as=== '1'. No Claude variable existed, so this adds one in that family.What the suite covers
It is the only test that talks to a real Claude CLI: the structured-session handshake (a session id minted before the first message), the settings and model each turn reports, a follow-up withdrawn when the turn ahead of it is stopped (as the real CLI confirms it), sign-in guidance on an unauthenticated start, and where the transcript file lands.
No CI job ran it before this change (no workflow installs Claude, so every case skipped), and none runs it after. CI coverage is unchanged; the only thing that stops is accidental local runs on a developer's account.
AGENTS.mdnow says when to run it on purpose.Will real problems still get caught?
Turning off the default run does not remove a safety net, because the default run was never a reliable one:
.claude/commandsno longer appears in the command list. It had failed the same way in earlier accidental runs today, and nobody acted on it, because a failure buried in a broad run looks like noise. That failure needs its own fix, which is tracked separately.How agents will know to run it:
AGENTS.md, which every agent working in this repo reads at the start of a session, now says: when you change Claude structured-session code (src/main/claude/claude-structured-*), runORCA_REAL_CLAUDE_CLI_TEST=1 pnpm test src/main/claude/claude-structured-real-cli.test.ts src/main/claude/claude-structured-real-cli-fold.test.ts.What is still not covered, stated plainly: an agent that changes Claude code without reading that line, or ignores it, will not run these tests, and CI will not catch a regression they would have caught. That was equally true before this change, because CI never ran them. The fix for that gap is a follow-up: record real Claude sessions once and replay them in a normal unit test, so the Orca side of these cases is checked on every pull request without a Claude account, leaving only changes in Claude itself to the opt-in run.
Where these real-Claude tests came from
These suites were added, and extended, by these PRs:
src/main/claude/claude-structured-real-cli.test.ts: added in feat(claude): move structured native chat onto the Claude Agent SDK and enable it on macOS and Linux #18560 (structured Claude moved onto the Claude Agent SDK). Cases were later added or changed by fix(native-chat): list the slash commands and skills a structured Claude session actually loaded #19127 (the slash-command and skill list), fix(claude): open structured chat without a startup deadline, and make Retry start fresh #22364 (open without a startup deadline), fix(native-chat): a message is accepted, then delivered #22821 (accept, then deliver), fix(claude): withdraw a follow-up queued behind a stopped turn instead of dropping it silently #23553 (a queued follow-up withdrawn behind a stopped turn), test(claude): fix queued-send cancellation CI expectations #23675 (typed cancellation in queued-send settlements) and fix(native-chat): Stop is there from the moment a message is sent #23026 (Stop from the moment a message is sent).src/main/claude/claude-structured-real-cli-fold.test.tsand the sharedclaude-real-cli-availability-test-support.ts: added in fix(native-chat): a message Claude folds into its running turn no longer splits the turn #23621 (a message Claude folds into its running turn no longer splits the turn). The shared support probedclaude --versionandauth statuswhen it was imported, with no opt-in. This PR puts both suites behind the one switch.src/main/claude/claude-supervised-stop-real-cli.test.ts: added in fix(claude): run structured Claude under the POSIX provider supervisor #23476. It already needed its ownORCA_REAL_CLAUDE_SUPERVISED_STOP=1, so this PR doesn't change it.Why
Having Claude installed and signed in is not consent to spend that account's turns or write into its home folder. The old gate inferred consent from credentials, so every broad run on a developer machine was an opt-in by accident.
An explicit variable is the rule the other real-binary tests already follow: the Codex binary contract needs
ORCA_CODEX_CONTRACT_BINARY, the real-Codex orchestration test needsORCA_REPRO_CODEX_BINARY, and the real-agent end-to-end specs needORCA_E2E_REAL_CODEX,ORCA_E2E_REAL_CODEX_IME,ORCA_E2E_REAL_OPENCODEorORCA_E2E_REAL_REMOTE_CODEX. This was the one real-agent test that used the developer's signed-in account without being asked.Alternatives considered: excluding the file from the default vitest include list would stop it running even when a developer asks for it by path, and would hide it from the suite entirely. Gating only the signed-in cases would still spawn the real CLI on every broad run. The gate checks the variable before any probe, so a broad run no longer touches
claudeat all.CI jobs and the variable: no CI job ran this suite's tests before, and none does now, so no workflow needs the variable.
Unit tests / test(called fromPR Checksonubuntu-24.04-arm, and fromnode-next-compaton Node 24 and 26): collects the file in its shards, but no workflow installsclaude, so every case skipped before on the missing binary (the recorded shard timing for the whole file is 257 ms, far too short for a real session) and skips now on the missing variable. Variable not set; not needed.PR Checks / Codex index-heal contract: installs the Codex CLI only and runs one named Codex file. Not affected.Git compatibility,shell contracts,orcad browser provider,cross-version wire compatibility,package,package (windows),release-ref-validation, the skill-sharing release lanes, and the Bun profile lanes). Not affected.Linked Issue
N/A: internal test-safety fix, no issue.
Visual Proof
N/A: test-only change, no UI or runtime behavior change.
Testing
src/main/claude/claude-real-cli-test-gate.test.tscovers the gate both ways with a fakeclaudethat is installed and signed in. Unset, empty,0andtrueall skip with the message naming the variable, and the fake is never called.1runs, returns the account report, and probes--versionthenauth status --jsonin that order. Opted in with no binary still skips. A failed or unreadable auth report still runs, with no report, so only the signed-out case runs. Account fields of the wrong type are dropped.With the variable check removed from the gate, the four "skips without probing" cases fail, and the other five still pass.
Ran the gate test plus
claude-structured-real-cli.test.tswith the variable unset: 9 passed, and all 6 real-CLI cases were reported as skipped under the title naming the variable. Also ran the real-CLI suite once withORCA_REAL_CLAUDE_CLI_TEST=1, against the real signed-in Claude CLI (2.1.280) with its login in place, on this branch:.claude/commandsappears in the command list. Claude 2.1.280 listed 72 commands without it. This case failed the same way in two earlier local runs today, before this change, so it is drift between the test and the current Claude release, not something this gate causes. It needs its own fix.oxlinton the changed files, andpnpm run check:code-quality:changed -- origin/main: clean.Platforms: macOS locally. The change is a plain environment check and has no platform-specific paths.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security: a developer's real account is no longer used by default. Cross-platform: the Windows spawn path used by the probes is unchanged. SSH, mobile, backward compatibility and performance: not affected; this only changes which tests run.
To run the real suites on purpose:
ORCA_REAL_CLAUDE_CLI_TEST=1 pnpm exec vitest run --config config/vitest.config.ts src/main/claude/claude-structured-real-cli.test.ts src/main/claude/claude-structured-real-cli-fold.test.tsAuthor X: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)