diff --git a/devlog/_plan/260930_local_test_stability/000_README.md b/devlog/_plan/260930_local_test_stability/000_README.md new file mode 100644 index 00000000000..c1f44b0ede9 --- /dev/null +++ b/devlog/_plan/260930_local_test_stability/000_README.md @@ -0,0 +1,7 @@ +# 260930 Local test stability in Codex-managed worktrees + +Status: open. Loop session `01a0f144-fb4d-7c32-940c-8b8a99874ba2` (wp2, after #6304 closed wp1), branch `codex/test-guard-managed-worktree` from `origin/dev` `2405624f39`. + +| Doc | Work-phase | Content | +|---|---|---| +| 010_diagnosis_and_plan.md | wp2 | Two host-dependent failure classes, their causes, the diff, verification | diff --git a/devlog/_plan/260930_local_test_stability/010_diagnosis_and_plan.md b/devlog/_plan/260930_local_test_stability/010_diagnosis_and_plan.md new file mode 100644 index 00000000000..10087195ca2 --- /dev/null +++ b/devlog/_plan/260930_local_test_stability/010_diagnosis_and_plan.md @@ -0,0 +1,37 @@ +# 010 Diagnosis and diff-level plan + +## Symptoms (2026-09-30, while verifying #6304) + +1. `bun run test:changed` from the Codex-managed worktree `~/.codex/worktrees/77b7/opencodex`: 863 failures. A sample file (`tests/codex-integration/model-visibility-management-api.test.ts`) fails 23/23 on its own with `refusing to remove a path inside the real Codex home (~/.codex) from a test process: ".../tests/codex-integration/.tmp-model-visibility-management-" resolves there`. The same head cloned to `/private/tmp` drops to 56 failures. +2. `tests/service/shutdown-launcher.test.ts` fails SIGINT/SIGTERM/SIGHUP at both `dev` `6538001e2f` and the #6304 head: `The proxy answered /healthz but did not inject Codex config ... Proxy already running on port 10100; requested a second instance on port `. + +## Causes + +1. `protectedRemovalReason` (`src/lib/test-home-guard.ts`) refuses any removal target inside `~/.codex`. Forty test files keep their fixture directory next to the test (`join(import.meta.dir, ".tmp-...")`) and clean it through `removeTreeWithRetry`. When the checkout itself is a Codex-app worktree under `~/.codex/worktrees/`, every such cleanup is refused, although the directory is repository content the test created, not Codex state. +2. The launcher test gives the child a fresh `OPENCODEX_HOME` with no `config.json`. `handleStart` probes the configured port (`findProxyOwnerBeforeJournalRecovery({ probeConfiguredPort: true })`), which defaults to 10100. On a developer machine running ocx there, the child finds that live owner, takes the sibling path, and by design never injects Codex config, so the test's precondition never arrives. CI has no proxy on 10100, which is why it only fails locally. + +## Diff + +- `src/lib/test-home-guard.ts`: a removal target strictly inside the checkout that loaded the guard (`resolve(import.meta.dir, "../..")`, both canonical and lexical spellings) is not a protected location. Equality and ancestor checks still run first for every protected tree, so the checkout root itself and anything above it stay refused; outside a checkout that lives under a protected tree the exemption changes nothing. Computed lazily so production module load does no extra work. +- `tests/ci-workflows/test-home-guard.test.ts`: a probe with a sentinel real home whose `.codex` is a symlink to the checkout's parent, so the checkout sits inside the protected Codex home. Asserts a fixture path under `tests/` is allowed, while the checkout root, its parent and a sibling of the checkout are refused. Skipped where symlinks are unavailable, like the existing symlink probe. +- `tests/service/shutdown-launcher.test.ts`: write `{ "port": }` to the fresh home's `config.json` so the configured-port probe targets the test's own port, not the host's 10100. +- `structure/`: record the checkout exemption where the guard is described, if a doc owns it. + +## Verification + +- `bun test tests/ci-workflows/test-home-guard.test.ts tests/service/shutdown-launcher.test.ts` from the managed worktree, with the host proxy live on 10100. +- `bun test ./tests/codex-integration/model-visibility-management-api.test.ts` from the managed worktree (23/23 failing before). +- `bun run test:changed` from the managed worktree: failures must fall to the `/private/tmp` level and contain no `refusing to remove`. +- `bun run typecheck`, `bun run structure:check`, `bun run privacy:scan`; exact-head CI; after merge, the `dev` push CI for both merge commits. + +## Residual, out of scope + +The `/private/tmp` full run still showed 5 s timeouts in the Claude picker/intercept TLS suites and service-state suites when 1566 files share one machine; each passes in isolation and on the sharded CI. Recorded, not changed here. + + +## Audit fold (NEAR-PASS, two blockers) + +1. The exemption is tied to one protected tree. It lifts only the "inside `protectedPath`" refusal, and only when a checkout root spelling is strictly inside that same `protectedPath` and the candidate is strictly inside that checkout root. Equality, ancestor and real-home checks stay unconditional. A bunfs build (`import.meta.dir` under `/$bunfs/root`, root `/`) or a checkout at `$HOME` never satisfies "checkout inside the tree", so the guard is unchanged there. Probe: `OCX_REAL_HOME=` still refuses `/.codex/x`. +2. Each spelling (canonical, lexical) is judged on its own and any refusal wins. Probe: a symlink inside the checkout that points at the protected tree outside the checkout is refused through its canonical form. + +Notes taken: the launcher test also pins `GROK_HOME` and `OCX_OWNER_REGISTRY_DIR` into the fixture home (the `sibling-home-client-sync` pattern) so inherited real state cannot mark the child a sibling; the `protectedRemovalReason` doc comment is updated; verification adds a run from a checkout outside `~/.codex`. diff --git a/src/lib/test-home-guard.ts b/src/lib/test-home-guard.ts index 63537742d01..c7c0e28f60b 100644 --- a/src/lib/test-home-guard.ts +++ b/src/lib/test-home-guard.ts @@ -22,6 +22,7 @@ import { homedir } from "node:os"; import { dirname, isAbsolute, join, relative, resolve } from "node:path"; import { realpathSync } from "node:fs"; +import { fileURLToPath } from "node:url"; const GUARD_ENV = "OCX_TEST_HOME_GUARD"; /** @@ -185,16 +186,57 @@ function isInside(parent: string, child: string): boolean { return rel !== "" && !rel.startsWith("..") && !isAbsolute(rel); } +let checkoutRootSpellings: readonly string[] | undefined; + +/** + * Both spellings of the checkout that loaded this module (`src/lib` -> repository root). + * + * Lazy so a production process, which never removes anything through this guard, does no work + * for it at module load. In a compiled binary the module URL sits under a virtual root, so the + * result names no real checkout and the exemption below can never apply. + */ +function checkoutRoots(): readonly string[] { + if (checkoutRootSpellings === undefined) { + let lexical: string | null = null; + try { + lexical = resolve(fileURLToPath(new URL("../..", import.meta.url))); + } catch { + // A non-file module URL names no checkout: no exemption. + } + checkoutRootSpellings = lexical === null ? [] : [...new Set([canonicalize(lexical), lexical])]; + } + return checkoutRootSpellings; +} + +/** + * Whether `candidate` is repository content of a checkout that itself lives inside `protectedPath`. + * + * A Codex-app worktree is a checkout under `~/.codex/worktrees/`, and forty suites keep their + * fixture directory beside the test file. Refusing every path inside `~/.codex` refused those + * suites' own cleanup, 863 failures in one run, although nothing there is Codex state. The lift + * is deliberately narrow: it applies to one tree, only when a checkout spelling is strictly + * inside that tree and the candidate is strictly inside that checkout. The checkout root itself, + * its ancestors and every sibling stay refused, and a checkout that merely CONTAINS a protected + * tree (one at `$HOME`, or the virtual root of a compiled build) never qualifies. + */ +function isOwnCheckoutContent(protectedPath: string, candidate: string): boolean { + return checkoutRoots().some(root => isInside(protectedPath, root) && isInside(root, candidate)); +} + /** * Why removing `target` is refused, or `null` when it is not a protected location. * * Three relations are refused, not one. Equality alone would still permit * `rmSync(getConfigPath())` against a live `config.json`, and it would permit * `rmSync(homedir())`, which takes the protected tree with it. So a target is refused when it - * IS a protected tree, when it sits INSIDE one, or when it is an ANCESTOR of one. + * IS a protected tree, when it sits INSIDE one, or when it is an ANCESTOR of one. The one lift is + * content of the running checkout when that checkout lives inside the tree + * ({@link isOwnCheckoutContent}); it never applies to equality or ancestry. * * Canonicalization is what makes a symlink useless as a bypass: a temp path that merely points - * at the real home resolves to the real home before any comparison happens. + * at the real home resolves to the real home before any comparison happens. Each spelling is + * judged on its own and any refusal wins, so a link inside the checkout that resolves into the + * protected tree is refused through its canonical form. */ export function protectedRemovalReason(target: string): string | null { // Both spellings are judged, not just the canonical one. Canonicalization is what defeats a @@ -209,7 +251,9 @@ export function protectedRemovalReason(target: string): string | null { for (const tree of PROTECTED_TREES) { for (const protectedPath of [tree.path, tree.lexical]) { if (candidate === protectedPath) return `${tree.label} (${protectedPath})`; - if (isInside(protectedPath, candidate)) return `a path inside ${tree.label} (${protectedPath})`; + if (isInside(protectedPath, candidate) && !isOwnCheckoutContent(protectedPath, candidate)) { + return `a path inside ${tree.label} (${protectedPath})`; + } if (isInside(candidate, protectedPath)) return `an ancestor of ${tree.label} (${protectedPath})`; } } diff --git a/tests/ci-workflows/test-home-guard.test.ts b/tests/ci-workflows/test-home-guard.test.ts index aa89f907e78..8be98541ed0 100644 --- a/tests/ci-workflows/test-home-guard.test.ts +++ b/tests/ci-workflows/test-home-guard.test.ts @@ -10,7 +10,7 @@ * Incident: devlog/_fin/260730_codex_rs_upstream_v2_live_handoff/070. */ import { describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdtempSync, mkdirSync, readFileSync, statSync, symlinkSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, mkdirSync, readFileSync, statSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; @@ -692,6 +692,64 @@ const canSymlink = (() => { expect(JSON.parse(probe.stdout.trim())).toEqual({ alias: true, plain: true }); }); + test.skipIf(!canSymlink)("content of a checkout inside the Codex home may be removed; the checkout and its neighbours may not", async () => { + // A Codex-app worktree is a checkout under ~/.codex/worktrees/. The sentinel's .codex points + // at this checkout's parent, which puts the running checkout inside the protected tree the + // same way. Only repository content is lifted; a link inside the checkout that resolves out + // of it is still judged by its canonical form. + const probeId = beginProbe("14-checkout-content"); + const realHome = mkdtempSync(join(tmpdir(), "ocx-sentinel-home-")); + const codexLink = join(realHome, ".codex"); + symlinkSync(dirname(REPO_ROOT), codexLink); + mkdirSync(join(realHome, ".opencodex"), { recursive: true }); + try { + const probe = await runProbe(probeId, ` + import { mkdirSync, symlinkSync, unlinkSync } from "node:fs"; + import { dirname, join } from "node:path"; + import { protectedRemovalReason } from "${REPO_ROOT_URL}src/lib/test-home-guard"; + const root = ${JSON.stringify(REPO_ROOT)}; + const linkDir = join(root, ".tmp"); + mkdirSync(linkDir, { recursive: true }); + const link = join(linkDir, "guard-link-" + process.pid); + symlinkSync(dirname(root), link); + try { + console.log(JSON.stringify({ + fixture: protectedRemovalReason(join(root, "tests", ".tmp-guard-fixture")) === null, + checkout: protectedRemovalReason(root) !== null, + parent: protectedRemovalReason(dirname(root)) !== null, + sibling: protectedRemovalReason(join(dirname(root), "another-worktree")) !== null, + link: protectedRemovalReason(link) !== null, + })); + } finally { + unlinkSync(link); + } + `, { OCX_REAL_HOME: realHome, OCX_TEST_HOME_GUARD: "1" }); + + expect(JSON.parse(probe.stdout.trim())).toEqual({ + fixture: true, checkout: true, parent: true, sibling: true, link: true, + }); + } finally { + unlinkSync(codexLink); + } + }); + + test("a checkout that contains a protected tree gains no exemption", async () => { + // The lift requires the checkout to sit INSIDE the tree. A checkout at the home directory, or + // the virtual root a compiled build reports, contains ~/.codex instead and must stay guarded. + const probeId = beginProbe("15-checkout-contains-tree"); + const probe = await runProbe(probeId, ` + import { join } from "node:path"; + import { protectedRemovalReason } from "${REPO_ROOT_URL}src/lib/test-home-guard"; + const root = ${JSON.stringify(REPO_ROOT)}; + console.log(JSON.stringify({ + codex: protectedRemovalReason(join(root, ".codex", "sessions")) !== null, + opencodex: protectedRemovalReason(join(root, ".opencodex", "config.json")) !== null, + })); + `, { OCX_REAL_HOME: REPO_ROOT, OCX_TEST_HOME_GUARD: "1" }); + + expect(JSON.parse(probe.stdout.trim())).toEqual({ codex: true, opencodex: true }); + }); + test("removeTreeWithRetry refuses a protected tree before it calls through", () => { const attempted: string[] = []; expect(() => removeTreeWithRetry(protectedHomeForTests(), { remove: path => { attempted.push(path); } })) diff --git a/tests/service/shutdown-launcher.test.ts b/tests/service/shutdown-launcher.test.ts index 9f7dca0b8c2..b6045bf18a0 100644 --- a/tests/service/shutdown-launcher.test.ts +++ b/tests/service/shutdown-launcher.test.ts @@ -108,6 +108,11 @@ describe.skipIf(!runnable)("ocx launcher graceful shutdown", () => { // no-ops when no config.toml exists) — this lets us prove the config is RESTORED. const codexConfig = join(home, "config.toml"); writeFileSync(codexConfig, 'model = "gpt-5.1"\n'); + // Pin the configured port to this test's own port. `ocx start` probes the CONFIGURED + // port for a live owner even with no state files, and a fresh home defaults to 10100. + // On a developer machine running ocx there, the child found that proxy, took the sibling + // path and by design never injected Codex config, so this test could not pass locally. + writeFileSync(join(home, "config.json"), JSON.stringify({ port })); // stdout/stderr are CAPTURED, not discarded. // @@ -126,6 +131,10 @@ describe.skipIf(!runnable)("ocx launcher graceful shutdown", () => { USERPROFILE: identity.userProfile, OPENCODEX_HOME: home, CODEX_HOME: home, + // Inherited real state must not make the child a sibling of the host's proxy either + // (same pinning as tests/cli/sibling-home-client-sync.test.ts). + GROK_HOME: join(home, "grok"), + OCX_OWNER_REGISTRY_DIR: join(identity.homeDir, ".opencodex", "ocx-homes"), ...identity.serviceManagerEnv, }, });