From 200498e812b6ac4455a0dd9ca6020b7aa1fb4bbd Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:32:10 +0900 Subject: [PATCH 1/2] docs(devlog): record the admin-token local-UX unit --- .../000_research.md | 120 ++++++++++++++++++ .../010_suppress_local_prompt.md | 101 +++++++++++++++ .../020_dialog_repair.md | 63 +++++++++ .../030_windows_baseline.md | 82 ++++++++++++ .../040_delivery_record.md | 64 ++++++++++ 5 files changed, 430 insertions(+) create mode 100644 devlog/_plan/260905_admin_token_local_ux/000_research.md create mode 100644 devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md create mode 100644 devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md create mode 100644 devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md create mode 100644 devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md diff --git a/devlog/_plan/260905_admin_token_local_ux/000_research.md b/devlog/_plan/260905_admin_token_local_ux/000_research.md new file mode 100644 index 0000000000..ae938bf82e --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/000_research.md @@ -0,0 +1,120 @@ +# 000 — Research: why a plain local user is shown an admin token box + +Two open issues describe the same wound from opposite ends. + +- **#3483** (juzijia, Windows 10, 2.42.0) — the admin token dialog paints an + empty red error notice the moment it opens, before anything is submitted. +- **#3353** (Tao-Yida) — after upgrading to 2.40.0 a user was locked out of the + dashboard by a bare password box, assumed a config-loss bug, and had to have + an LLM read the source to learn the token was a new security feature. + +The user's framing is stronger than either issue, and it is the one this unit +adopts: **a plain local user should never see that dialog at all.** When it does +appear it is a symptom, and the box asks the user to solve a problem they did +not cause and cannot diagnose. + +## How the dashboard is supposed to authenticate + +A loopback install never needs a typed credential. The server mints a session +and injects it into the served document: + +``` +GET /opencodex-session + -> src/server/index.ts:2074 issueGuiSession(...) + -> src/server/gui-session.ts:166 + -> meta opencodex-session-token / -csrf / -origin / -server-origin + (src/server/gui-static.ts:71-74) +``` + +`gui/src/api.ts:loadInjectedSession()` reads those tags on boot. Verified live +against the running 2.43.0 proxy on port 10100: + +```text +curl -i -H 'Host: 127.0.0.1:10100' http://127.0.0.1:10100/opencodex-session +HTTP/1.1 200 OK + +``` + +So on the happy path the prompt is unreachable. The interesting question is what +happens when that mint fails. + +## The fallback that should not be a fallback + +`gui/src/api.ts:resolveTokenAfter401()` (around line 247) handles a 401 like this: + +```ts +const renewed = await Promise.race([reBootstrapSessionToken(plane), watchdog]); +if (renewed.kind === "minted") return renewed.token; +if (renewed.kind === "failed") return null; +const prompted = await requestAdminToken(token => verifyAdminToken(plane, token)); +``` + +`reBootstrapSessionToken` maps **any 4xx** to `"unavailable"`: + +```ts +if (!response.ok) return response.status >= 400 && response.status < 500 + ? { kind: "unavailable" } : { kind: "failed" }; +``` + +And `"unavailable"` is precisely the branch that raises the password box. + +Now read the mint conditions (`src/server/gui-session.ts:172-183`): + +```ts +if (!isApiAuthRequired(config)) { + if (!isLoopbackHostname(host.hostname) || !isAllowedManagementOrigin(req, config)) return null; + ... +} +``` + +with `isApiAuthRequired(config) === !isLoopbackHostname(config.hostname)` +(`src/server/auth-cors.ts:285`). + +That yields the defect in one sentence: **on a loopback install the only ways to +get a 401 from the bootstrap are a Host or Origin mismatch — a misconfiguration +the admin token cannot fix.** Typing a token there is not a recovery path; it is +a dead end wearing a login form. + +And when the bind genuinely is non-loopback, the token is real and required — +but the dialog explains none of that, which is exactly #3353. + +## Why the notice is already red and empty (#3483) + +`gui/src/admin-token-dialog.ts:76-79` builds the error element up front: + +```ts +validationError.className = "notice notice-err"; +validationError.hidden = true; +``` + +`hidden` works only because the UA stylesheet says `[hidden] { display: none }`, +and that rule is the weakest one in the cascade. `gui/src/styles.css:1307` then +says: + +```css +.notice { ... display: flex; ... } +``` + +An author rule with an explicit `display` beats the UA `[hidden]` rule, so the +element stays laid out. It has `notice-err` borders and padding +(`styles.css:1355-1359`) and no text, which renders as the empty red box in the +screenshot. The bug is a CSS cascade defect, not a logic error — which is why no +existing test caught it: happy-dom asserts `hidden === true` happily while a real +browser paints the box. + +## What this unit changes + +1. Never prompt a standalone/loopback dashboard. Tell the user what is actually + wrong instead. (`010`) +2. When the prompt is legitimate, make it self-explanatory and link to a real + setup guide. Fix the empty notice while in there. (`020`) +3. Triage the Windows baseline and #3320 and land what is provable. (`030`) +4. Deliver as a stacked PR chain, admin-merged to `dev`. (`040`) + +## Constraints carried from the request + +- No repository-wide local suite on this workstation. Focused `bun test ` + plus `bun run typecheck`; heavy probes go to SSH hosts. +- A Windows baseline is already in flight on `desktop-c795oh4` under + `/c/ocxwin` (lock `/c/ocxwin/.suite.lock`, shards `base-1..4`). It is read-only + evidence for this unit and must not be disturbed. diff --git a/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md b/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md new file mode 100644 index 0000000000..e887fa3219 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md @@ -0,0 +1,101 @@ +# 010 — Never prompt a standalone dashboard for an admin token + +Work-phase `wp1`. Depends on 000. Criterion `c-1`. + +## Problem + +`gui/src/api.ts:resolveTokenAfter401()` treats "the server would not mint me a +session" as "ask the human for a token". On a loopback install those are not the +same thing. `issueGuiSession` mints unconditionally for a loopback host with an +allowed origin (`src/server/gui-session.ts:172-183`), so a 401 there means the +request did not look loopback to the server — a Host/Origin/bind problem. No +token the user can type changes that verdict, because the token is not what was +refused. + +## The signal + +The server already states its topology on every served document: + +```ts +// src/server/gui-static.ts:95 +function runtimeRoleMeta(runtimeRole: string): string { + return \`\`; +} +``` + +and the GUI already reads it (`gui/src/api-targets.ts:10-14`). That comment block +is explicit that a missing tag means "standalone / older server / Vite dev", i.e. +the safe default. This unit reuses that exact reader rather than inventing a +second topology signal. + +The rule: **the admin-token prompt is for a deployment that actually requires a +typed credential.** That is the non-loopback bind, which is the `hub` role. Any +other role — `standalone`, `client`, or an absent tag — must not prompt. + +## Change + +In `gui/src/api-targets.ts`, add a sibling to `isConnectedRuntime()`: + +```ts +/** + * May this dashboard ask the user to type an admin token? + * + * Only a hub does. A standalone loopback install mints its own session + * (src/server/gui-session.ts), so a refusal there is a Host/Origin + * misconfiguration that no typed token can repair — prompting for one asks the + * user to answer a question they did not cause and cannot diagnose (#3353). + * A missing tag reads as standalone, matching runtimeRoleFromDocument's + * existing safe default. + */ +export function adminTokenPromptAllowed(): boolean { + return runtimeRoleFromDocument() === "hub"; +} +``` + +In `gui/src/api.ts:resolveTokenAfter401()`, gate the prompt and record why it was +skipped: + +```ts +if (renewed.kind === "failed") return null; +if (!adminTokenPromptAllowed()) { + state.promptCancelled = true; // do not re-ask on every subsequent 401 + reportSessionUnavailable(plane); // surface an actionable notice instead + return null; +} +const prompted = await requestAdminToken(...); +``` + +`promptCancelled = true` matters: without it every failing request re-enters the +resolution path. The existing `storeSession` already resets that flag when a +session is later minted (`gui/src/api.ts:81`), so recovery is automatic once the +misconfiguration is fixed. + +## What the user sees instead + +A dismissible notice, not a form. Copy names the real cause and the real fix: + +> **The dashboard could not start a session.** OpenCodex is running, but this +> page's address is not one it recognises as local. Open the dashboard at the +> address the proxy prints on startup (usually `http://127.0.0.1:`), or see +> the dashboard access guide. + +`reportSessionUnavailable` is a thin, testable seam: it dispatches a +`CustomEvent` the shell renders. It must not be a `alert()` and must not block. + +## Verification + +`bun test gui/tests/api-auth-deadline.test.ts` plus a new case: + +- role `standalone` (and absent tag): after 401 + `unavailable` rebootstrap the + injected `adminTokenPrompt` spy is **not** called, the request resolves, and a + second failing request does not call it either. +- role `hub`: the spy **is** called (the legitimate path stays intact). + +The role must be settable per test — the tests build their own `happy-dom` +document, so the case writes the meta tag before `installApiAuthFetch()`. + +## Out of scope + +No server change. `issueGuiSession`, `requireManagementAuth`, and the CORS +resolvers keep their current semantics; this phase only stops the GUI from +asking a question the server never wanted asked. diff --git a/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md b/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md new file mode 100644 index 0000000000..3e4743988a --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md @@ -0,0 +1,63 @@ +# 020 — Repair the dialog itself + +Work-phase `wp2`. Criteria `c-2`, `c-3`, `c-4`. Landed in #3491 and #3493. + +Two defects in one surface, deliberately split across two PRs because they have +nothing to do with each other beyond sharing a file. + +## #3483 — the empty red notice + +`gui/src/admin-token-dialog.ts` builds the error element up front and hides it +with the `hidden` attribute. That works only because the UA stylesheet says +`[hidden] { display: none }`, and `gui/src/styles.css` overrode it: + +```css +.notice { ... display: flex; ... } +``` + +Author origin beats user-agent origin. **Specificity never enters the +comparison** — which is why this looked like a validation false-positive and why +a `.notice[hidden]` fix would have been the wrong shape (it would still lose to +the three-class `.startup-runtime-notice` rule at `0,3,0`). + +The repository had already solved this exact problem once, in +`gui/src/styles-combos-workspace.css`, where a bare `display: flex` left both +tab panels painted at once. That comment block is the precedent this fix +follows: move the display onto `:not([hidden])`. + +Applied to `.notice`, `.notice-warn` (used without `.notice` in several places), +and `.notice.notice-warn.startup-runtime-notice`. + +### Testing this required two tests, not one + +happy-dom applies no author stylesheet and performs no layout, so +`expect(alert.hidden).toBe(true)` **passes today, unfixed**, while a real browser +paints the box. A DOM assertion cannot see this class of bug. + +So the DOM test asserts the notice is hidden AND empty on open (keeping the two +halves of "no error" from drifting), and a second test reads `styles.css` and +fails any `.notice` rule that sets `display` without the guard. The second was +driven red by reverting the CSS before being accepted. + +## #3353 — the box that explained nothing + +The dialog's only text was a title naming an environment variable. A user who +had never set one had no way in. + +Ground truth, verified in source rather than assumed: + +- the proxy writes the token to `getConfigDir()/admin-api-token` on first start + (`src/lib/admin-secrets.ts`), `0600`, matching `/^ocx_admin_[A-Za-z0-9_-]{43}$/` +- `OPENCODEX_ADMIN_AUTH_TOKEN` replaces it entirely and is not regex-checked +- **no CLI prints it.** `ocx doctor` deliberately reports presence without ever + returning a value + +That last point is worth stating in the docs explicitly rather than omitting: +hunting for `ocx token` is the obvious next move, and silence about it wastes +the user's time. + +The dialog now carries a help paragraph plus a link to a new +"Finding the admin token" anchor, styled like the existing in-app docs links +(`target="_blank"`, `rel="noreferrer"`, accent colour) per +`gui/src/pages/dashboard-dialogs.tsx`. Copy landed in all nine locales; +`gui/tests/i18n-locales.test.ts` enforces key parity. diff --git a/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md b/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md new file mode 100644 index 0000000000..a0371ea812 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md @@ -0,0 +1,82 @@ +# 030 — Windows baseline: what the failures actually were + +Work-phase `wp3`. Criterion `c-5`. Evidence, not a landed fix. + +A full Windows suite was already running on `desktop-c795oh4` when this unit +started (`/c/ocxwin/repo` at `00834d710`, shards `base-1..4`). It was read-only +evidence for this unit and was not disturbed. A second run on Bun `1.4.0` +followed (`v140-1..4`). + +## The headline + +| run | Bun | fail | +|---|---|---:| +| `base-1..4` | 1.3.14 | 179 | +| `v140-1..4` | 1.4.0 | 25 | + +Shard 1 alone went from 100 `(fail)` lines to 2. **The Windows suite was not +telling us about 179 product defects; it was mostly reporting one harness +failure over and over.** + +## Why 161 of them were one bug + +`tests/preload.ts` calls `acquireTestRunLock` (line 42) BEFORE it arms the guard +with `OCX_TEST_HOME_GUARD=1` (line 64). On Windows the lock path needs the +effective account SID, and under 4-shard load that lookup timed out: + +```text +CodexUserIdentityRefusal: Windows effective-account lookup timed out. + at powershellValue (src/codex/user-identity.ts:252) + at resolveWindowsSid (src/codex/user-identity.ts:261) + at resolveDefaultTestRunLockPath (scripts/test-run-lock.ts:227) + at tests/preload.ts:42 +``` + +The worker then ran with the guard permanently off, and every test that asserts +"this helper is only available under the repository test preload" failed as a +cascade: 44 in `codex-reset-credit-operation-ledger`, 68 in +`codex-reset-credit-recovery`, 49 in `lab-fabric-task`. + +**That cascade had teeth.** With the guard down, two suites reached live +machine state instead of being refused: + +- `windows-elevation-spawn.test.ts:89` expected `launcherPid: null` and got + `18144` — a real PowerShell process was launched. +- `service.test.ts:1283`/`:1307` expected "refusing to mutate the + machine-global Windows Task Scheduler from an armed test process" and instead + got real scheduler-registration outcomes. + +So the ordering in `preload.ts` is not only noisy, it is the difference between +a refused test and one that touches the developer's own Task Scheduler. Worth +fixing on its own merits, independently of the Bun version that exposed it. + +## What survives on Bun 1.4.0 + +25 failures in three groups: + +- `multi-account auth store` — 22 of the 25, one file. Not yet diagnosed. +- `ocx v2 keep-native-v1` — 2. Dirac classified the `base` occurrence as a test + artifact: the product CLI exited 0 and the V2 disable took effect; only the + spy compares raw argv, and Windows `.cmd` invocation goes through the ComSpec + wrapper (`src/lib/win-exec.ts:79`), which is correct behaviour. +- `cli wiring > interactiveGuardOk ... when cwd is unlinked` — 1. + +## Not fixed here, and why + +This unit's authority is the admin-token UX. None of the surviving failures are +in that surface, and each needs its own reproduction on Windows before a fix is +more than a guess — the `base` run's evidence is contaminated by the guard +cascade, so a fix written against it would be written against an artifact. + +The honest carry-forward is three separate units: + +1. Arm `OCX_TEST_HOME_GUARD` before `acquireTestRunLock`, or make the SID lookup + fail closed instead of proceeding unguarded. Highest value: it is a safety + defect, not just a flake. +2. Diagnose `multi-account auth store` on Windows. +3. Decide whether `keep-native-v1` should assert on parsed argv rather than the + raw ComSpec string. + +Issue #3320 (non-ASCII account names misclassifying a valid scheduler task) is +adjacent to (1) — both are Windows identity handling — but it is `needs-info` +and was not reproduced here, so it stays open. diff --git a/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md b/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md new file mode 100644 index 0000000000..7c9e486298 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md @@ -0,0 +1,64 @@ +# 040 — Delivery record + +Work-phase `wp4`. All three PRs merged to `dev` with admin authority. + +| PR | merge SHA | what | +|---|---|---| +| [#3491](https://github.com/lidge-jun/opencodex/pull/3491) | `85e42117c` | #3483 empty red notice (CSS cascade) | +| [#3496](https://github.com/lidge-jun/opencodex/pull/3496) | `3e65218ba` | never prompt a local dashboard | +| [#3493](https://github.com/lidge-jun/opencodex/pull/3493) | `8b961b198` | #3353 dialog guidance + docs anchor | + +Ancestry proven for each: `git fetch origin dev` then +`git merge-base --is-ancestor FETCH_HEAD` exit 0. + +Issues #3483 and #3353 closed with landing comments. + +## The stack-parent close race + +#3492 was the original middle PR. Merging #3491 with `--delete-branch` removed +`codex/admin-token-empty-notice`, which was #3492's base, and GitHub auto-closed +it. A closed PR cannot be retargeted, and it could not be reopened because its +base branch no longer existed. #3496 carries the identical commits rebased onto +`dev`; #3492 records the supersession. + +**For the next stack: retarget children to `dev` BEFORE merging the parent, or +merge without `--delete-branch` and delete the branch afterwards.** The order +that looks natural — merge, then restack — is the one that loses the PR. + +## Trailing CI, classified + +Two failures on green-otherwise heads, both proven flakes rather than assumed: + +- `test 3/4` on `6da1269f8`: `tests/responses-state.test.ts:1464` through + `fallbackPendingResponseSpills` (`src/responses/state.ts:714`), which is the + `remaining <= 0` deadline-budget branch in a 2 MB shutdown-spill test — a + loaded-runner timeout. The diff touches no `responses/` file; the file passes + 141/141 locally twice; it passed on rerun. +- `macos` on `87abc9152`: `tests/shutdown-launcher.test.ts:147`. That PR changes + only GUI copy, i18n, and docs — no runtime code at all. Passes 3/3 locally; + passed on rerun. + +Both were rerun to green rather than merged over. + +## Verification actually run + +No repository-wide local suite (prohibited for this task). Instead: + +- `bun run typecheck` — clean +- `cd gui && bun test` — 1366 pass / 0 fail across 220 files +- `bun test tests/gui-static.test.ts tests/server-management-auth.test.ts tests/server-auth.test.ts` — 143 pass / 0 fail +- `bun run lint:gui`, `bun run privacy:scan` — clean +- exact-head CI green on `d9a1afc71`, `6da1269f8`, `87abc9152` before each merge + +The full GUI suite earned its place: it caught 7 tests in `api-auth-memory` +that assumed the prompt always fires, which the focused files did not cover. + +## One correction worth recording + +The first implementation gated the prompt on `runtimeRole === "hub"`. A review +lane checking "would this lock anyone out?" found that it would: `standalone` + +`hostname: "0.0.0.0"` is an operator who deliberately exposed the dashboard and +must type the token, and `tests/server-management-auth.test.ts` already proves +that bind mints no session. The role is topology; the question is the bind. The +shipped predicate is `isApiAuthRequired`, the same one the server gates the mint +with, and a regression now covers the exposed-standalone case explicitly. From f9279351662c426e10e47b6a90411366841ecb4f Mon Sep 17 00:00:00 2001 From: jun Date: Sat, 5 Sep 2026 03:49:12 +0900 Subject: [PATCH 2/2] fix(tests): arm the home guard before the run lock can throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Windows baseline reported 179 failures. 161 of them were this one bug. `tests/preload.ts` acquired the run lock before it armed `OCX_TEST_HOME_GUARD`, with no try/finally between them. Taking the lock resolves a user-scoped path, which on Windows spawns PowerShell for the effective SID; under four-shard load that spawn timed out, the refusal threw straight out of the preload, and every statement below it — the sandbox, the arming, and the assertion that exists to catch exactly this — never ran. The worker then executed its whole file with the guard down, and 161 tests failed asserting "this helper is only available under the repository test preload". The count is the least of it. `src/lib/windows-elevation.ts` and `src/service.ts` refuse live elevation and machine-global Task Scheduler mutation only while the guard is armed, so an unguarded worker launched a real PowerShell process (pid 18144) and reached real scheduler registration on the developer's own machine. The guard is what stands between a test run and the user's Task Scheduler, and it was being skipped by the one failure most likely to happen under load. Reordered to sandbox → arm + assert → lock. Arming earlier is safe because the guard is a deny-list keyed on a path captured at module import, not a "sandbox is present" flag: its worst case when armed early is refusing a write to the real home, which is the direction that fails closed. Putting the lock last is the load-bearing part — arming before the lock but sandboxing after it would still leave a worker that is armed and unsandboxed, able to read the real home. The lock error stays unswallowed: a run that cannot take the lock must still fail, it just must not fail while unprotected. Two regressions. The first asserts the order on the source itself, because nothing else can see it: with a lock that happens to succeed the runtime state is identical either way, and the defect only reproduces when the lock throws. Stashing the reorder was verified to turn it red. The second proves an armed process with a real HOME and no lock still refuses the protected home — the state the timed-out worker was actually in. Evidence and the remaining Windows triage: `devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md`. --- tests/preload.ts | 57 +++++++++++++++++++++++------------ tests/test-home-guard.test.ts | 51 +++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 19 deletions(-) diff --git a/tests/preload.ts b/tests/preload.ts index 6e2baa0ed1..ad64ff6384 100644 --- a/tests/preload.ts +++ b/tests/preload.ts @@ -21,6 +21,44 @@ import { } from "../scripts/test-run-lock"; import { rmSync } from "node:fs"; +// Under `bun run test` the wrapper already handed us a sandbox (and OCX_REAL_HOME so the +// guard could still see the true home). Isolating again is harmless and deliberate: the +// alternative — inferring "already isolated" from path shapes — would trust exactly the +// user-controlled environment state this file exists to distrust. +const isolated = createIsolatedTestEnvironment(); +for (const [key, value] of Object.entries(isolated.env)) { + if (value !== undefined) process.env[key] = value; +} + +// Arm the guard once the sandbox is in place, and BEFORE the run lock. +// +// The order here is sandbox → arm → lock, and the lock being last is the load-bearing +// part. Acquiring the lock resolves a user-scoped path, which on Windows means spawning +// PowerShell for the effective SID (`scripts/test-run-lock.ts` → +// `resolveEffectiveUserIdentity`). Under four-shard load that spawn timed out, the +// refusal threw straight out of this preload, and everything below it — the sandbox, the +// guard, and the assertion that would have caught the omission — never ran. The worker +// then executed its whole file with the guard DOWN. +// +// That is not merely noisy. `src/lib/windows-elevation.ts` and `src/service.ts` refuse +// live elevation and machine-global Task Scheduler mutation only while +// `isTestHomeGuardArmed()`, so an unguarded worker launched a real PowerShell process and +// reached real scheduler registration on the developer's own machine +// (`devlog/_plan/260905_admin_token_local_ux/030`). +// +// Arming earlier is safe because the guard is a deny-list keyed on a path captured at +// module import, not a "sandbox is present" flag: the worst case of arming early is +// refusing a write to the real home, which is the direction that fails closed. The lock +// error is deliberately NOT swallowed — a run that cannot take the lock must still fail, +// it just must not fail while unprotected. +process.env.OCX_TEST_HOME_GUARD = "1"; +// Lets a test assert one preload per process rather than assuming Bun's scheduling. +process.env.OCX_TEST_PRELOAD_PID = String(process.pid); + +if (!isTestHomeGuardArmed() || !protectedHomeForTests()) { + throw new Error("test home guard failed to arm; refusing to run tests unprotected"); +} + // `scripts/test.ts` owns the lock for wrapped runs. A bare `bun test` has no wrapper, // so a single-process runner uses its own PID while true parallel workers rendezvous // on their short-lived controller PID. The first worker acquires the lock and siblings @@ -50,25 +88,6 @@ await acquireTestRunLock({ ), }); -// Under `bun run test` the wrapper already handed us a sandbox (and OCX_REAL_HOME so the -// guard could still see the true home). Isolating again is harmless and deliberate: the -// alternative — inferring "already isolated" from path shapes — would trust exactly the -// user-controlled environment state this file exists to distrust. -const isolated = createIsolatedTestEnvironment(); -for (const [key, value] of Object.entries(isolated.env)) { - if (value !== undefined) process.env[key] = value; -} - -// Arm the guard only after the sandbox is in place, so nothing observes a half-applied -// state. The guard's protected path was already captured at import time above. -process.env.OCX_TEST_HOME_GUARD = "1"; -// Lets a test assert one preload per process rather than assuming Bun's scheduling. -process.env.OCX_TEST_PRELOAD_PID = String(process.pid); - -if (!isTestHomeGuardArmed() || !protectedHomeForTests()) { - throw new Error("test home guard failed to arm; refusing to run tests unprotected"); -} - // Clean up only the root this preload created. The `bun run test` wrapper owns its own. process.on("exit", () => { try { rmSync(isolated.root, { recursive: true, force: true }); } catch { /* best effort at exit */ } diff --git a/tests/test-home-guard.test.ts b/tests/test-home-guard.test.ts index 6334164498..a99b98d7f6 100644 --- a/tests/test-home-guard.test.ts +++ b/tests/test-home-guard.test.ts @@ -286,4 +286,55 @@ const canSymlink = (() => { expect(homedir()).toBeTruthy(); expect(getConfigDir()).not.toBe(protectedHomeForTests()); }); + + /* + * The preload must arm the guard BEFORE it acquires the run lock. + * + * This is not a style preference. Taking the lock resolves a user-scoped path, which on + * Windows spawns PowerShell for the effective SID; under four-shard load that spawn timed + * out, the refusal threw straight out of the preload, and every statement below it — + * including the arming — never ran. The worker executed its whole file unguarded, and + * because `src/lib/windows-elevation.ts` and `src/service.ts` refuse live elevation and + * machine-global Task Scheduler mutation only while armed, one such worker launched a real + * PowerShell process and reached real scheduler registration on the developer's machine. + * + * The ordering is the whole fix, so it is asserted on the source itself: a reader of the + * runtime state cannot tell "armed before the lock" from "armed after a lock that happened + * to succeed", and the failure only reproduces when the lock throws. + */ + test("the preload arms the guard before it can throw on the run lock", async () => { + const source = await Bun.file(new URL("./preload.ts", import.meta.url)).text(); + + const armAt = source.indexOf('process.env.OCX_TEST_HOME_GUARD = "1"'); + const assertAt = source.indexOf("test home guard failed to arm"); + const lockAt = source.indexOf("await acquireTestRunLock("); + const sandboxAt = source.indexOf("createIsolatedTestEnvironment()"); + + expect(armAt).toBeGreaterThan(-1); + expect(assertAt).toBeGreaterThan(-1); + expect(lockAt).toBeGreaterThan(-1); + expect(sandboxAt).toBeGreaterThan(-1); + + // sandbox -> arm -> assert -> lock. Arming before the sandbox would leave a window that + // is merely over-protective, but arming after the lock is the defect above. + expect(sandboxAt).toBeLessThan(armAt); + expect(armAt).toBeLessThan(assertAt); + expect(assertAt).toBeLessThan(lockAt); + }); + + /* + * And the guard has to hold for a process that never reached the lock at all, which is the + * state the timed-out worker was actually in. + */ + test("a process that arms the guard is protected even with no lock and a real HOME", () => { + const { realHome } = sentinelHome(); + const probe = runProbe(` + import { assertNotRealHomeUnderTest, isTestHomeGuardArmed } from "${REPO_ROOT_URL}src/lib/test-home-guard"; + let rejected = false; + try { assertNotRealHomeUnderTest(${JSON.stringify(join(realHome, ".opencodex"))}); } catch { rejected = true; } + console.log(JSON.stringify({ armed: isTestHomeGuardArmed(), rejected })); + `, { OCX_TEST_HOME_GUARD: "1", OCX_REAL_HOME: realHome, HOME: realHome, OPENCODEX_HOME: undefined }); + + expect(JSON.parse(probe.stdout.trim())).toEqual({ armed: true, rejected: true }); + }); });