Skip to content

Stop tests from starting a real agent CLI unless asked - #23824

Closed
brennanb2025 wants to merge 5 commits into
mainfrom
brennanb2025/real-cli-tests-opt-in
Closed

brennanb2025 wants to merge 5 commits into
mainfrom
brennanb2025/real-cli-tests-opt-in

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​104 $\color{#cf222e}{\Huge{\mathbf{−}}}$​12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​92
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​14 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​12

ELI5

Some of Orca's tests start the real Claude, Codex, Hermes or OMP program installed on your computer. Until now the Claude and Hermes ones ran automatically whenever that program was installed and signed in, so any broad local test run started your real agent against your own config and account. After this PR, every test that starts a real agent stays off unless you turn it on with ORCA_RUN_REAL_AGENT_TESTS=1.

What Changed

Problem. The two Claude "real CLI" test files decided whether to run by asking claude --version and claude auth status. On any machine with Claude installed and signed in, a broad vitest run (a directory, or a command whose file list came out empty and so ran the whole suite) started the real Claude against the developer's real ~/.claude, wrote session folders there and made real API calls on their account. This has happened six times on the maintainer's machine in two days, from agents whose instructions explicitly banned it; the latest was a shell word-split that left vitest with no file arguments, so it ran the full suite for 13 minutes. Instructions cannot fix this, so the tests themselves must be opt-in.

What is different for developers. Tests that start a real agent now skip by default, through the same skip conditions they already had. To run them on purpose, set the variable, e.g. ORCA_RUN_REAL_AGENT_TESTS=1 pnpm exec vitest run --config config/vitest.config.ts src/main/claude/claude-structured-real-cli.test.ts.

Mechanism. One new test-only module, src/main/real-agent-tests-opt-in-test-support.ts, owns the variable name and the check. The shared Claude availability helper checks it before it runs any probe, so without the variable nothing is spawned, not even claude --version, and its existing realClaudeAvailable / realClaudeAuthenticated exports come out false. The Claude suites' existing describe.skipIf / it.skipIf gates then skip them with no edits to those files. Every other suite gets the check added to the condition it already skips on.

Every test that starts a real agent CLI, and how each is now gated:

  • src/main/claude/claude-structured-real-cli.test.ts (real Claude, real account): was ungated; now skips through the shared Claude helper (file unchanged).
  • src/main/claude/claude-structured-real-cli-fold.test.ts (real Claude, real account): was ungated; now skips through the shared Claude helper (file unchanged).
  • src/main/hermes/hook-service.test.ts, test "is visible to the real hermes CLI when hermes is installed" (runs the real hermes binary from PATH): was ungated; now returns early without probing, the same way it already did when Hermes is not installed.
  • src/main/claude/claude-supervised-stop-real-cli.test.ts (real Claude): already needed ORCA_REAL_CLAUDE_SUPERVISED_STOP=1 plus ORCA_REAL_CLAUDE_BIN; now also needs the new variable.
  • src/main/codex/codex-index-heal-binary-contract.test.ts (real Codex binary, throwaway CODEX_HOME): already needed ORCA_CODEX_CONTRACT_BINARY; now also needs the new variable.
  • src/main/runtime/orchestration-codex-real-pty.integration.test.ts (real Codex in a PTY): already needed ORCA_REPRO_CODEX_BINARY; now also needs the new variable.
  • tests/tools/omp-close-lifecycle.test.mjs and tests/tools/omp-relay-close-lifecycle.test.mjs (real OMP binary in a PTY, throwaway HOME; collected by the unit Vitest config): already needed ORCA_OMP_PROBE_BINARY; now also need the new variable, and their run instructions (omp-close-lifecycle.md, omp-relay-close-lifecycle.md) include it.

CI. Only one workflow runs a real agent on purpose: the Codex index-heal contract job in .github/workflows/pr.yml. It now sets ORCA_RUN_REAL_AGENT_TESTS: '1', so its behaviour is unchanged. Its existing "required" guard (ORCA_CODEX_CONTRACT_REQUIRED=1) now also fails red if the opt-in is missing, so the job cannot pass by silently skipping. No CI job runs the Claude or Hermes real-CLI tests.

Why

Six incidents show that telling agents "never run these" does not hold; a missing file argument or a directory run is enough. Making the tests opt-in removes the failure at its source instead of relying on every caller.

One variable checked in a shared module, rather than a file-name exclude in the Vitest config: an exclude would also block deliberate named runs, and it only catches files whose names match the pattern, so the Hermes test (inside an ordinary hook-service.test.ts) would still run. The check sits in the Claude helper before its probes, so the gate also stops the --version and auth status spawns, not just the sessions.

The three tests that already had their own switch keep it, since it also supplies the binary path; the new variable is added on top so there is one switch that covers every real-agent test.

Skips carry no per-test message naming the variable: adding one meant moving the Claude suites to per-test skip hooks, which re-indented about 500 lines for a message. The variable is named in the helper comments and in this PR instead.

Linked Issue

None: found by the maintainer; no issue was opened.

Visual Proof

N/A: test-only change with no app behaviour or UI difference.

Testing

  • New src/main/claude/claude-real-cli-availability-test-support.test.ts: with the variable unset, the Claude helper reports realClaudeAvailable and realClaudeAuthenticated false and node:child_process spawn/spawnSync are never called; with it set, the (mocked) probe runs --version once. The command resolver is mocked to a nonexistent path so no real CLI can start even if a mock failed. Ablation: removing the check from the helper makes the first test fail.

  • hook-service.test.ts, codex-index-heal-binary-contract.test.ts and orchestration-codex-real-pty.integration.test.ts run by name with all opt-in and binary variables unset: real-agent tests skip, the rest pass.

  • Contract guard: ORCA_CODEX_CONTRACT_REQUIRED=1 with a nonexistent binary and no opt-in fails red ("needs ORCA_CODEX_CONTRACT_BINARY and ORCA_RUN_REAL_AGENT_TESTS=1"); the contract cases skip.

  • The two OMP probes, run by name with ORCA_OMP_PROBE_BINARY pointed at a nonexistent path and no opt-in: both skip.

  • Not run locally, on purpose: the three *real-cli* Claude files themselves, because running them is the hazard this PR removes. Two of them are unchanged and skip through the helper covered above; tc:node passes.

  • tc:node, oxlint on changed files, and pnpm run check:code-quality:changed -- origin/main pass. macOS only.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Scope: 12 files, +118/−14: test files, one test-only module, the two OMP probes' run instructions and one CI environment line; no app code changes, so no effect on SSH, Windows, Linux, mobile or wire compatibility.

Not changed: the five Playwright end-to-end specs that start a real agent TUI already have their own switch and are not reachable from a Vitest run: terminal-windows-codex-multiline-paste and terminal-codex-local-typing-latency (ORCA_E2E_REAL_CODEX), chinese-ime-chat-input-repro (ORCA_E2E_REAL_CODEX_IME), terminal-opencode-emoji-table-rendering (ORCA_E2E_REAL_OPENCODE), and ssh-codex-display-artifacts-repro (ORCA_E2E_REAL_REMOTE_CODEX, Codex inside a Docker SSH host). config/scripts/run-codex-real-account-validation.mjs is a manual script; its test only checks the harness and starts no agent.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Tests that start the real Claude, Codex or Hermes CLI now skip unless
ORCA_RUN_REAL_AGENT_TESTS=1, checked before any availability probe runs,
so a broad local vitest run no longer starts the developer's real agent
against their own config and account. The Codex contract CI job sets the
variable and turns a missing opt-in into a red test.
…-tests-opt-in

# Conflicts:
#	src/main/codex/codex-index-heal-binary-contract.test.ts
Revert the Claude real-CLI suites to main and make the helper's existing
realClaudeAvailable/realClaudeAuthenticated false without the opt-in, so
their gates skip with zero edits. The other suites get a one-condition
change instead of new hooks.
The two tests/tools OMP probes start an installed OMP binary and are collected
by the unit Vitest config, but skipped only on ORCA_OMP_PROBE_BINARY. Require
ORCA_RUN_REAL_AGENT_TESTS=1 as well, like the other binary-gated real-agent
tests, and add it to their documented run commands.
@brennanb2025 brennanb2025 changed the title Stop tests from starting the real Claude, Codex or Hermes CLI unless asked Stop tests from starting a real agent CLI unless asked Sep 29, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status: ready for maintainer review (still a draft; not merged)

What this PR fixes. Tests that start a real agent program (Claude, Codex, Hermes, OMP) used to run whenever that program was installed and signed in. A broad local test run could therefore start the real Claude against the developer's own ~/.claude and make real API calls on their account. After this PR, all of those tests skip unless ORCA_RUN_REAL_AGENT_TESTS=1 is set, and the check happens before any probe (even claude --version) is started.

Review loops: 2; the second found nothing.

  • Loop 1 found two tests the PR had missed, both starting a real OMP binary: tests/tools/omp-close-lifecycle.test.mjs and tests/tools/omp-relay-close-lifecycle.test.mjs. The unit test config collects both, and their only guard was ORCA_OMP_PROBE_BINARY. Fixed in d2a924c: both now also require the opt-in, and their run instructions say so.
  • Loop 2 re-checked the whole repo a different way. It expanded every file the unit test config collects (10,152), then traced each one that starts a process without mocking it. It also checked production code that finds and starts an agent, and every variable that points at an agent binary. It found no other test that starts a real agent, so it changed no code. It corrected the PR title and description only: the title now says "agent CLI" rather than naming three agents, and the Playwright list is complete.
  • The architecture challenge was not triggered: there were only 2 loops and no repeated issue.

Checked

  • Test coverage. Every test that can start a real agent in a Vitest run is gated: the two structured Claude suites (through the shared Claude helper), Claude supervised-stop, the Hermes hook test, the Codex index-heal contract, the Codex real-terminal integration test, and the two OMP probes. The five Playwright specs that start a real agent already had their own switch, and Vitest never collects them.
  • CI still runs the real agent where it should. The "Codex index-heal contract" job in pr.yml sets the opt-in, and on d2a924c its log shows Tests 4 passed | 1 skipped (5). The 4 are the real contract cases; the skip is the missing-prerequisites guard, which only fires when setup is missing. That guard now also fails red if the opt-in is missing, so the job cannot pass by silently skipping.
  • No regressions. All 24 CI checks pass (7 skipped by design), including all 5 unit-test shards, typecheck, lint and package (windows). Locally, every changed test file was run by name with the opt-in unset: the new helper test passes 7/7, and the real-agent cases skip.
  • Mocking. The helper test's node:child_process mock also covers code that imports the bare child_process name.
  • Performance. No impact: nothing new runs when a module is imported, and the Claude helper now skips the probe it used to start at import.
  • Scope and approach. 12 files, +118/−14; test files, one test-only module, two run-instruction files and one CI environment line. No app code changes, so no effect on SSH, Windows, Linux, mobile or compatibility between app versions. A shared opt-in variable, checked before any probe runs and added to each suite's existing skip condition, is a standard way to gate tests that start a real CLI.

Not run on purpose

  • Electron and mobile QA: N/A. The PR changes no app behaviour or UI, so there is nothing to screenshot.
  • The real-agent tests themselves: not run locally, since running them is the hazard this PR removes. Their gating is covered by the new helper test and by running each file by name with the opt-in unset.

Not caused by this PR

Possible follow-up (out of scope): nothing stops a future test from starting a real agent without calling the opt-in check. A small ratchet test, or a *.real-agent.test.ts naming convention excluded from the default unit run, would catch that structurally.

@brennanb2025
brennanb2025 marked this pull request as ready for review September 29, 2026 09:13
@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Adds opt-in gate for tests that start real agent CLIs.

The PR appears safe to merge; the remaining concern is non-blocking coverage for the new CI setting.

Findings

  1. P2 CI opt-in lacks test coverage ▶

Summary

The PR makes Vitest tests that launch real agent CLIs opt-in, adds coverage for the Claude availability gate, and enables the Codex contract in its dedicated CI step.

  • The gates prevent the reviewed real-agent tests and Claude availability probes from starting by default.
  • The Codex CI step retains a fail-closed prerequisite guard.

Reviews (1) · Last reviewed commit: "test: gate the real OMP close probes beh..."

Comment thread .github/workflows/pr.yml
# instead of a green no-op.
ORCA_CODEX_CONTRACT_REQUIRED: '1'
# Why: real-agent tests skip unless opted in; this job exists to run one.
ORCA_RUN_REAL_AGENT_TESTS: '1'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 CI opt-in lacks test coverage The workflow test checks the Codex contract job’s binary, version, and required guard, but not this new opt-in. If a later workflow edit removes it, that test will still pass; the missing setting will surface only when the conditional Codex job runs and fails. Please assert verify.env.ORCA_RUN_REAL_AGENT_TESTS alongside the other required settings.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@coderabbitai

coderabbitai Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fcd7f253-abaa-4b32-8cc4-cbd33837696a

📥 Commits

Reviewing files that changed from the base of the PR and between 36c473e and d2a924c.

📒 Files selected for processing (12)
  • .github/workflows/pr.yml
  • src/main/claude/claude-real-cli-availability-test-support.test.ts
  • src/main/claude/claude-real-cli-availability-test-support.ts
  • src/main/claude/claude-supervised-stop-real-cli.test.ts
  • src/main/codex/codex-index-heal-binary-contract.test.ts
  • src/main/hermes/hook-service.test.ts
  • src/main/real-agent-tests-opt-in-test-support.ts
  • src/main/runtime/orchestration-codex-real-pty.integration.test.ts
  • tests/tools/omp-close-lifecycle.md
  • tests/tools/omp-close-lifecycle.test.mjs
  • tests/tools/omp-relay-close-lifecycle.md
  • tests/tools/omp-relay-close-lifecycle.test.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

A shared helper enables real-agent tests only when ORCA_RUN_REAL_AGENT_TESTS is exactly 1. Claude availability probing and selected real-agent tests now require this opt-in alongside their existing conditions. The Codex contract workflow and OMP probe commands set the opt-in.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d2a92

Ordinary test runs avoid starting real agent CLIs, while the documented intentional runs remain enabled. No merge-blocking issue is identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d2a92

The new opt-in reduces accidental use of developers’ agent accounts during ordinary test runs. The dedicated CI contract test remains enabled. No expansion of application privileges was found, but not every possible test path was verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant credential exposure is a test runner’s access to installed agent CLIs and their account context. The examined default-off gates reduce accidental local exposure; the dedicated CI step deliberately retains its prior real-binary check.

Trust Boundaries and Controls

  • observed — Possession of an installed binary or signed-in CLI is no longer sufficient for the examined tests: the exact environment opt-in is also required before their real-agent execution paths.

Resilience and Maintainability Implications

  • observed — The required Codex contract fails if either its binary or the new opt-in is absent, preserving a visible failure rather than a passing-but-skipped CI check.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 9 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: real agent CLI tests now require explicit opt-in.
Description check ✅ Passed The description is complete and structured according to the template. It explains the problem, changes, rationale, testing, scope, compatibility considerations, and checklist status. The absence of a …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 9 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This test-only PR adds a single opt-in switch, ORCA_RUN_REAL_AGENT_TESTS=1, and threads it through every Vitest test that starts a real agent CLI, so a broad local run no longer touches a developer's installed Claude/Codex/Hermes/OMP.

  • New shared switch — src/main/real-agent-tests-opt-in-test-support.ts exports REAL_AGENT_TESTS_ENV and realAgentTestsEnabled(), which treats only '1' as enabled.
  • Claude availability gate — claude-real-cli-availability-test-support.ts short-circuits before its --version probe, so realClaudeAvailable / realClaudeAuthStatus / realClaudeAuthenticated all come out false and the two claude-structured-real-cli* suites skip unchanged through their existing describe.skipIf.
  • Per-test gates — supervised-stop, the Codex binary contract, the Codex real-PTY integration test, and the Hermes real-CLI test each add the check to their existing condition; both OMP probes and their .md run instructions do too.
  • CI contract — the Codex index-heal job sets the opt-in, and its ORCA_CODEX_CONTRACT_REQUIRED=1 guard now fails red when either the binary or the opt-in is missing.

I verified the new helper test passes, that the gated suites skip without the variable, and that the CI guard fails red under ORCA_CODEX_CONTRACT_REQUIRED=1 with no opt-in. An independent sweep of Vitest-collected tests found no other test that starts a real agent CLI, so the enumeration in the description holds.

ℹ️ Nitpicks

  • config/scripts/codex-index-heal-contract-workflow.test.mjs asserts the job's ORCA_CODEX_CONTRACT_REQUIRED but not the new ORCA_RUN_REAL_AGENT_TESTS, so that workflow-contract test alone would not catch the opt-in being dropped from CI. Not blocking — the contract's own REQUIRED guard already fails red if the opt-in disappears.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #23702, which gates the real Claude CLI suites (the ones that were starting the real claude against developers' ~/.claude) behind ORCA_REAL_CLAUDE_CLI_TEST=1 in the shared availability helper.

The other tests this PR touched were already opt-in behind their own variables (Claude supervised-stop, the Codex index-heal contract, the Codex real-PTY repro, and the OMP lifecycle tests via ORCA_OMP_PROBE_BINARY). One small gap remains: src/main/hermes/hook-service.test.ts runs the real hermes from PATH when it is installed. That can be a follow-up if it ever bites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant