Repository navigation
test: unit tests can no longer write a developer's real agent or Orca settings #23979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b78e50f
2d2f2c3
36ba9bc
f1a6418
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { randomUUID } from 'node:crypto' | ||
| import { existsSync, mkdirSync, mkdtempSync, openSync, rmSync, writeFileSync } from 'node:fs' | ||
| import { writeFile } from 'node:fs/promises' | ||
| import { homedir, tmpdir, userInfo } from 'node:os' | ||
| import { join } from 'node:path' | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { takeRealAgentHomeWriteViolations } from './vitest-real-agent-home-write-guard' | ||
|
|
||
| // Why never-created paths: each sits under a missing folder or is a forced no-op removal, so even | ||
| // with the guard off (the ablation) nothing lands in the real home. | ||
| const missingRealFolder = (entry: string) => | ||
| join(userInfo().homedir, entry, `orca-vitest-guard-${randomUUID()}`) | ||
|
|
||
| afterEach(() => { | ||
| takeRealAgentHomeWriteViolations() | ||
| vi.unstubAllEnvs() | ||
| }) | ||
|
|
||
| describe('vitest real-agent-home write guard', () => { | ||
| it('refuses a named-import sync write under the real ~/.codex', () => { | ||
| const target = join(missingRealFolder('.codex'), 'config.toml') | ||
| expect(() => writeFileSync(target, '[projects."/tmp/x"]\n')).toThrow(/real-agent-home guard/) | ||
| expect(existsSync(target)).toBe(false) | ||
| }) | ||
|
|
||
| it('refuses a promise write under a ~/.claude.json sibling', async () => { | ||
| const target = join(missingRealFolder('.claude.json'), 'x.tmp') | ||
| await expect(writeFile(target, '{}')).rejects.toThrow(/real-agent-home guard/) | ||
| expect(existsSync(target)).toBe(false) | ||
| }) | ||
|
|
||
| it('refuses a removal that would otherwise succeed silently', () => { | ||
| expect(() => rmSync(missingRealFolder('.orca'), { recursive: true, force: true })).toThrow( | ||
| /real-agent-home guard/ | ||
| ) | ||
| }) | ||
|
|
||
| it('still records a refusal the writer swallowed, so the test fails afterwards', () => { | ||
| try { | ||
| mkdirSync(join(missingRealFolder('.cursor'), 'projects')) | ||
| } catch { | ||
| // Trust writers swallow their errors like this. | ||
| } | ||
| expect(takeRealAgentHomeWriteViolations()).toEqual([expect.stringContaining('fs.mkdirSync')]) | ||
| }) | ||
|
|
||
| it('protects the account home even when HOME points elsewhere', () => { | ||
| const tempHome = mkdtempSync(join(tmpdir(), 'orca-vitest-guard-home-')) | ||
| vi.stubEnv('HOME', tempHome) | ||
| vi.stubEnv('USERPROFILE', tempHome) | ||
| try { | ||
| expect(homedir()).not.toBe(userInfo().homedir) | ||
| expect(() => rmSync(missingRealFolder('.copilot'), { force: true })).toThrow( | ||
| /real-agent-home guard/ | ||
| ) | ||
| } finally { | ||
| rmSync(tempHome, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
|
|
||
| it('leaves a sibling that only shares an agent folder prefix alone', () => { | ||
| const sibling = join(userInfo().homedir, `.codex-orca-vitest-guard-${randomUUID()}`) | ||
| expect(() => rmSync(sibling, { force: true })).not.toThrow() | ||
| }) | ||
|
|
||
| it('stands down while an opted-in real-agent suite switch is set', () => { | ||
| vi.stubEnv('ORCA_CODEX_TRUST_CONTRACT_BINARY', '/opt/codex') | ||
| expect(() => rmSync(missingRealFolder('.codex'), { force: true })).not.toThrow() | ||
| }) | ||
|
|
||
| it('allows reads there and writes under a temp home', () => { | ||
| expect(() => openSync(join(missingRealFolder('.codex'), 'config.toml'), 'r')).toThrow(/ENOENT/) | ||
| const tempHome = mkdtempSync(join(tmpdir(), 'orca-vitest-guard-home-')) | ||
| try { | ||
| const target = join(tempHome, '.codex', 'config.toml') | ||
| mkdirSync(join(tempHome, '.codex')) | ||
| writeFileSync(target, 'x') | ||
| expect(existsSync(target)).toBe(true) | ||
| } finally { | ||
| rmSync(tempHome, { recursive: true, force: true }) | ||
| } | ||
| expect(takeRealAgentHomeWriteViolations()).toEqual([]) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| import fs from 'node:fs' | ||
| import { syncBuiltinESMExports } from 'node:module' | ||
| import { userInfo } from 'node:os' | ||
| import { isAbsolute, join, resolve, sep } from 'node:path' | ||
| import { fileURLToPath } from 'node:url' | ||
| import { afterAll, afterEach } from 'vitest' | ||
|
|
||
| /** | ||
| * Why: agent trust and hook writers resolve `~` at write time, so a unit test that reaches one | ||
| * edits the developer's real Codex, Claude or Orca config. Any write there fails the test instead. | ||
| * Child processes a test spawns are outside this guard. | ||
| */ | ||
|
|
||
| type GuardState = { roots: string[]; violations: string[] } | ||
|
|
||
| const AGENT_HOME_ENTRIES = [ | ||
| '.codex', | ||
| '.claude', | ||
| '.orca', | ||
| '.cursor', | ||
| '.copilot', | ||
| '.gemini', | ||
| '.qoder' | ||
| ] | ||
| // Why a bare prefix: Claude's config file and its atomic-write temp siblings sit beside `~/.claude`. | ||
| const AGENT_HOME_FILE_PREFIXES = ['.claude.json'] | ||
| // Why: an Orca terminal exports these pointing at the live app and agent state; guarded, then unset. | ||
| const INHERITED_STATE_ENV = [ | ||
| 'ORCA_USER_DATA_PATH', | ||
| 'CODEX_HOME', | ||
| 'ORCA_CODEX_HOME', | ||
| 'CLAUDE_CONFIG_DIR' | ||
| ] | ||
| // Why: an Orca terminal points this at the live app's CLI, which a test's child shell would run. | ||
| const INHERITED_LIVE_CLI_ENV = ['ORCA_CODEX_LAUNCH_PREFLIGHT'] | ||
| // Why: opted-in real-agent suites point a real CLI at its own home on purpose; each suite's own switch. | ||
| const REAL_AGENT_SUITE_SWITCHES = [ | ||
| 'ORCA_REAL_CLAUDE_SUPERVISED_STOP', | ||
| 'ORCA_CODEX_CONTRACT_BINARY', | ||
| 'ORCA_CODEX_NO_DAEMON_CONTRACT_BINARY', | ||
| 'ORCA_CODEX_TRUST_CONTRACT_BINARY', | ||
| 'ORCA_REPRO_CODEX_BINARY' | ||
| ] | ||
| const realAgentSuiteOptedIn = () => REAL_AGENT_SUITE_SWITCHES.some((name) => process.env[name]) | ||
| const foldCase = process.platform === 'darwin' || process.platform === 'win32' | ||
|
|
||
| function normalize(path: string): string { | ||
| const resolved = resolve(path) | ||
| return foldCase ? resolved.toLowerCase() : resolved | ||
| } | ||
|
|
||
| function realHomes(): string[] { | ||
| const homes = [process.env.HOME, process.env.USERPROFILE] | ||
| try { | ||
| // Why: HOME does not move this one, so a test that swaps HOME cannot hide the real home. | ||
| homes.unshift(userInfo().homedir) | ||
| } catch { | ||
| // No passwd entry (some containers): HOME is all there is. | ||
| } | ||
| return homes.filter((home): home is string => Boolean(home)) | ||
| } | ||
|
|
||
| /** Folder roots end in a separator so `~/.orca` never covers `~/.orca-relay`; file prefixes do not. */ | ||
| function protectedRoots(): string[] { | ||
| const folders = realHomes().flatMap((home) => [ | ||
| ...AGENT_HOME_ENTRIES.map((entry) => join(home, entry)), | ||
| join(home, 'Library', 'Application Support', 'orca'), | ||
| join(home, '.config', 'orca'), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Orca userData roots are hardcoded to the default layouts ( Technical details# Missing XDG_CONFIG_HOME / APPDATA-derived Orca roots
## Affected sites
- `config/scripts/vitest-real-agent-home-write-guard.ts:65-70` — only the default `orca` folders are added.
- `src/main/codex/codex-home-paths.ts:62-75` — `getOrcaUserDataPath()` uses `process.env.XDG_CONFIG_HOME` (linux) and `process.env.APPDATA` (win32) before the home default.
## Required outcome
- The guard protects whichever directory Orca actually uses for userData when those env vars are set, mirroring `getOrcaUserDataPath()`.
## Suggested approach (optional)
- Add roots derived from `process.env.XDG_CONFIG_HOME` and `process.env.APPDATA` (when absolute) alongside the per-home defaults. |
||
| join(home, 'AppData', 'Roaming', 'orca') | ||
|
Comment on lines
+67
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If |
||
| ]) | ||
| for (const name of INHERITED_STATE_ENV) { | ||
| const value = process.env[name] | ||
| if (value && isAbsolute(value)) { | ||
| folders.push(value) | ||
| } | ||
| } | ||
| const filePrefixes = realHomes().flatMap((home) => | ||
| AGENT_HOME_FILE_PREFIXES.map((entry) => normalize(join(home, entry))) | ||
| ) | ||
| return [...new Set([...folders.map((folder) => normalize(folder) + sep), ...filePrefixes])] | ||
| } | ||
|
|
||
| function targetPath(target: unknown): string | null { | ||
| if (typeof target === 'string') { | ||
| return target.startsWith('file:') ? fileURLToPath(target) : target | ||
| } | ||
| if (target instanceof URL) { | ||
| return fileURLToPath(target) | ||
| } | ||
| if (Buffer.isBuffer(target)) { | ||
| return target.toString() | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| function assertOutsideRealHome(state: GuardState, operation: string, target: unknown): void { | ||
| const path = targetPath(target) | ||
| if (path === null || realAgentSuiteOptedIn()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a developer exports any real-agent contract switch before running the full unit suite, this check disables the guard for every test in that process, not just the contract test. An unrelated test can then silently change the developer’s real agent settings. Limit the exemption to the intended contract run. |
||
| return | ||
| } | ||
| const normalized = normalize(path) + sep | ||
| const root = state.roots.find((candidate) => normalized.startsWith(candidate)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Block recursive removal of protected roots through their parents. With the guard enabled, For recursive removal, reject targets that contain a protected root as well as targets inside one. Keep this check operation-specific so unrelated writes under the home remain allowed. Add a regression using a temporary parent and protected child. |
||
| if (!root) { | ||
| return | ||
| } | ||
| const error = new Error( | ||
| `[vitest real-agent-home guard] ${operation}(${path}) would write the real agent home under ${root}; give the test a temp home` | ||
| ) | ||
| // Why the stack: the writer that swallowed this is what the failing test has to fix. | ||
| state.violations.push(error.stack?.split('\n').slice(0, 12).join('\n') ?? error.message) | ||
| throw error | ||
| } | ||
|
|
||
| const WRITE_OPEN_FLAGS = fs.constants.O_WRONLY | fs.constants.O_RDWR | fs.constants.O_CREAT | ||
|
|
||
| function opensForWrite(flags: unknown): boolean { | ||
| if (typeof flags === 'number') { | ||
| return (flags & WRITE_OPEN_FLAGS) !== 0 | ||
| } | ||
| return typeof flags === 'string' && /[wa+]/.test(flags) | ||
| } | ||
|
|
||
| // Argument positions each mutating call writes to; `open` counts only when its flags write. | ||
| const GUARDED_TARGETS: Record<string, readonly number[]> = { | ||
| writeFile: [0], | ||
| appendFile: [0], | ||
| mkdir: [0], | ||
| mkdtemp: [0], | ||
| rename: [0, 1], | ||
| copyFile: [1], | ||
| cp: [1], | ||
| symlink: [1], | ||
| link: [1], | ||
| rm: [0], | ||
| rmdir: [0], | ||
| unlink: [0], | ||
| truncate: [0], | ||
| open: [0] | ||
| } | ||
|
|
||
| function guard( | ||
| state: GuardState, | ||
| api: typeof fs | typeof fs.promises, | ||
| name: string, | ||
| positions: readonly number[], | ||
| label: string | ||
| ): void { | ||
| const descriptor = Object.getOwnPropertyDescriptor(api, name) | ||
| const original: unknown = descriptor?.value | ||
| if (!descriptor || typeof original !== 'function') { | ||
| return | ||
| } | ||
| const target = original | ||
| function guarded(this: unknown, ...args: unknown[]): unknown { | ||
| try { | ||
| if (!name.startsWith('open') || opensForWrite(args[1] ?? 'r')) { | ||
| for (const position of positions) { | ||
| assertOutsideRealHome(state, label, args[position]) | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if (api === fs.promises) { | ||
| return Promise.reject(error) | ||
| } | ||
| throw error | ||
| } | ||
| return target.apply(this, args) | ||
| } | ||
| Object.defineProperty(api, name, { ...descriptor, value: guarded }) | ||
| } | ||
|
|
||
| function install(): GuardState { | ||
| const state: GuardState = { roots: protectedRoots(), violations: [] } | ||
| for (const [name, positions] of Object.entries(GUARDED_TARGETS)) { | ||
| guard(state, fs, name, positions, `fs.${name}`) | ||
| guard(state, fs, `${name}Sync`, positions, `fs.${name}Sync`) | ||
| guard(state, fs.promises, name, positions, `fs.promises.${name}`) | ||
| } | ||
| guard(state, fs, 'createWriteStream', [0], 'fs.createWriteStream') | ||
| // Why: named ESM imports of node:fs keep the originals until the builtin exports are re-synced. | ||
| syncBuiltinESMExports() | ||
| return state | ||
| } | ||
|
|
||
| declare global { | ||
| // Why once per process: re-wrapping per test file would stack wrappers around wrappers. | ||
| var orcaRealAgentHomeWriteGuard: GuardState | undefined | ||
| } | ||
| const state = (globalThis.orcaRealAgentHomeWriteGuard ??= install()) | ||
| // Why after install: the guard keeps the inherited paths as roots; tests that need one set their own. | ||
| const inheritedEnvToUnset = realAgentSuiteOptedIn() | ||
| ? [] | ||
| : [...INHERITED_STATE_ENV, ...INHERITED_LIVE_CLI_ENV] | ||
| for (const name of inheritedEnvToUnset) { | ||
| delete process.env[name] | ||
| } | ||
|
|
||
| /** Drains recorded violations; only the guard's own self-test should need this. */ | ||
| export function takeRealAgentHomeWriteViolations(): string[] { | ||
| return state.violations.splice(0) | ||
| } | ||
|
|
||
| function failOnViolations(): void { | ||
| const violations = takeRealAgentHomeWriteViolations() | ||
| if (violations.length > 0) { | ||
| // Why rethrow here: trust writers swallow errors, so the throw at the call site can vanish. | ||
| throw new Error(`[vitest real-agent-home guard]\n${violations.join('\n')}`) | ||
| } | ||
| } | ||
|
|
||
| afterEach(failOnViolations) | ||
| afterAll(failOnViolations) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve symlink destinations before allowing writes.
A temporary fixture can contain a symlink to the real
~/.codex/config.toml.copyFileSync(source, fixtureLink)passes this lexical check, but Node follows the destination symlink and overwrites the real config. No violation is recorded. (nodejs.org)For operations that follow symlinks, compare canonical targets against canonical protected roots. For new files, resolve the nearest existing parent before checking containment. Add a regression using temporary protected roots.