From 618809da1d00ee9c80f7473b83db29cc1729cc55 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:39:36 +0900 Subject: [PATCH 1/6] devlog(260827): review dev against main, and plan the base variants Five parallel read-only lanes over the 179 commits in origin/main..dev found 13 confirmed regressions, 1 suspected, and 14 explicitly cleared. Four are high. The one the user asked about is real and cheap to trigger: package-tree-integrity compares ctimeNs, which a chmod changes without replacing anything, so a permission change fences every /v1/* request behind a 503 until restart. Verified in a temp directory rather than argued about - chmod alone flips the guard from ok to package_tree_replaced, while mtimeNs stays put through chmod and moves for both an in-place rewrite and an atomic replace. Also recorded: a BOM-prefixed config.toml is corrupted by a custom-layer write, an unreadable store leaves config.toml half-changed with an orphaned journal, the prompt write routes accept a bare admin token where AGENTS.md requires a dashboard session, and the closed-PR branch cleanup deletes a branch whose name matches an old closed PR without checking the SHA. Two of the thirteen are mine, from the glm-5.3-flash work merged earlier today. The unit also plans what the user asked for next. "annotation" turned out to be ext/git-attribution, which lives in the 120_codex-cli checkout and not in 121_openai-codex, and which our LAYER_INVENTORY is missing entirely. It is account-resolved rather than config-gated, so it is a runtime-conditional row with no switch. The base-prompt variant selector rests on model_instructions_file, and its default variant is the ABSENCE of that key rather than a copy of Codex's prompt, so immutability is structural instead of enforced. Auditing that claim broke it once: a hand-set key would have read as "default" while the base prompt was in fact replaced, so the selection is three-valued and the external state keeps the notice the panel already ships in ten locales. --- .../000_plan.md | 120 +++++++++++ .../001_regression_review.md | 187 +++++++++++++++++ .../010_wp2_git_attribution_layer.md | 137 +++++++++++++ .../020_wp3_base_variants.md | 190 ++++++++++++++++++ .../021_audit_blockers.md | 109 ++++++++++ .../030_wp4_docs_and_stack.md | 90 +++++++++ 6 files changed, 833 insertions(+) create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/000_plan.md create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/001_regression_review.md create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/010_wp2_git_attribution_layer.md create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/020_wp3_base_variants.md create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/021_audit_blockers.md create mode 100644 devlog/_plan/260827_dev_regression_and_prompt_variants/030_wp4_docs_and_stack.md diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/000_plan.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/000_plan.md new file mode 100644 index 0000000000..8ef5017f15 --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/000_plan.md @@ -0,0 +1,120 @@ +# 000 — dev regression review + prompt base variants and git_attribution + +Unit: `devlog/_plan/260827_dev_regression_and_prompt_variants/` +Opened: 2026-08-27 · Work class: C4 · Branch target: `dev` +Base commit for opencodex citations: `5d0a97bd1` (dev HEAD, level with origin/dev). +Base commit for upstream citations: `4462b9dee` in +`/Users/jun/Developer/codex/120_codex-cli` ("Allow disabling the multi-agent wait +tool (#34887)"). The 121_openai-codex checkout at this HEAD does NOT carry +`ext/git-attribution`; 120_codex-cli does. That difference is load-bearing and +§3 records what follows from it. + +## Objective + +Two things the user asked for in one session, plus the review that has to come +first. + +1. Review the 179 commits in `origin/main..dev` for regressions before they are + promoted, with the 503 class ("503 같은 회귀가 있을수도 있잖아") driven against + a live proxy rather than reasoned about. +2. Give `base-instructions` a real switch and a 2-3 variant selector reachable + by left/right swipe, where the default variant can never be edited or removed + ("base 같은것도 끌수 있다면서 이런것도 스위치 달아야지 그리고 base 에서도 2-3번 + 옵션으로 변경할수 있도록하고 기본값은 변경안되도록 좌우 스와이프로"). +3. Implement the annotation layer from codex-rs ("annotation 부분 구현도 codex-rs + 참조해서 하고"). + +## What "annotation" turned out to be + +The user said "annotation 부분 구현도 codex-rs 참조해서 하고" and, separately, +"어차피 codex-rs에 내장되어있잖아 코덱스는 오픈소스야". Searching +`121_openai-codex/codex-rs` for `annotation` returns only MCP tool annotations +and Responses-API `url_citation` annotations — neither is a prompt layer. + +The prompt layer exists in the OTHER checkout: +`120_codex-cli/codex-rs/ext/git-attribution/src/world_state.rs`. It is a +world-state section with id `git_attribution`, markers +``/``, role `developer`, and three distinct +bodies (`ENABLED_INSTRUCTIONS`, `DISABLED_INSTRUCTIONS`, +`LEGACY_COMMIT_ATTRIBUTION_INSTRUCTIONS`). Its content is commit and pull-request +attribution — the `Co-authored-by: Codex ` trailer and the +`Generated with Codex.` PR marker. That is the annotation the user meant: Codex +annotating its own commits. + +Our `LAYER_INVENTORY` (`src/codex/prompt-layers.ts:88-104`) has fifteen entries +and none of them is this one. The panel therefore under-reports the prompt by one +whole developer section, which is exactly the failure mode the panel exists to +prevent. + +## The finding that shapes WP3 + +`git_attribution` is NOT config-gated. `ext/git-attribution/src/lib.rs:33-80` +resolves the policy from the AUTH SERVER — `resolve_attribution_policy(auth_manager, +base_url, http_client_factory)` — caches it on the thread store, and falls back to +`enabled: false` when the lookup fails. `features/src/lib.rs:277` records the old +flag as "Removed legacy git commit attribution guidance flag", so the config key +that once controlled it is gone. + +Consequences for the row, all of which the design must respect: + +- Class is `runtime-conditional`, not `config-toggle`. There is no key to write. +- It renders with NO switch, per the rule already enforced in + `PromptLayerRow.tsx:88-96`: a disabled control would claim a capability that + does not exist. +- Its condition line has to say the real condition — the account's attribution + policy — not "always on", because both DISABLED and ABSENT are reachable states. +- Its assembly order is registration-order dependent (it arrives through + `extensions.context_contributors()`, `core/src/session/world_state.rs:64-66`), + so `order` is `null` and the row shows the neutral dot the existing code + already renders for that case. + +This is the third time in this unit's family that a "family resemblance" guess +would have been wrong (`registry.ts:500-506` warns about the same thing for +models). The layer LOOKS like a config toggle because every other developer +section we ship is one. It is not. + +## Work-phase map (dependency-ordered, PHASE-SPLIT-01) + +| WP | Outcome | Doc | Depends on | +|---|---|---|---| +| WP0 | This roadmap, locked | `000`–`030` | — | +| WP1 | Regression review of `origin/main..dev`, 503 class driven live | `001` | WP0 | +| WP2 | `git_attribution` layer end to end | `010` | WP1 (a regression fix could move the same files) | +| WP3 | base-instruction switch + variant swipe selector | `020` | WP2 (both extend the same descriptor pipeline; WP2's is additive, WP3's changes base semantics) | +| WP4 | Docs, locale parity, stack merged | `030` | WP2, WP3 | + +WP2 before WP3 is not an effort call. WP2 adds a row to an existing taxonomy with +no new state; WP3 introduces a per-variant store and a write path that can replace +the base prompt. Building the additive one first means WP3's audit reviews one +new concept instead of two. + +## Scope boundary + +IN: `src/codex/prompt-layers.ts`, `src/server/management/codex-prompt-routes.ts`, +`gui/src/pages/codex-set-prompt.tsx`, `gui/src/components/codex-set/*`, +`gui/src/i18n/*`, `gui/tests/codex-set-*.test.tsx`, +`tests/codex-prompt-*.test.ts`, `docs-site/src/content/docs/*/guides/codex-prompt.md`, +this unit. + +OUT: `src/lab/` (the core-lab boundary in AGENTS.md), `src/generated/model-metadata.ts` +(generated), theme (`260802_codex_set_prompt_composer/090_theme_deferred.md` defers +it and nothing here changes that), `go/`, release automation, auth and credential +handling. + +## Verification contract + +Per work-phase, and no more than this: + +``` +cd gui && bun x tsc -b --force # NOT --noEmit: gui tsconfig has files: [] +bun x tsc --noEmit # repo root +bun test # narrow, per file +bun test ./gui/tests/ # directory form is acceptable +bun run lint:gui # judged no-NEW-findings vs origin/dev's 17 +bun run privacy:scan +git diff --check +``` + +The full local suite is never run — a standing user constraint. Heavy verification +goes to CI or `ssh lidge-ai` / `ssh clisu-oracle`. Every commit and push uses +`--no-verify`. diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/001_regression_review.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/001_regression_review.md new file mode 100644 index 0000000000..c9af093d4c --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/001_regression_review.md @@ -0,0 +1,187 @@ +# 001 — regression review of `origin/main..dev` (179 commits) + +Method: five parallel read-only reviewer lanes, one per subsystem, each required to +classify every finding CONFIRMED / SUSPECTED / CLEARED with a file:line citation and +a reproduction. Plus a live drive of the running proxy by the main session, because +the user's stated fear ("503 같은 회귀가 있을수도 있잖아") is about runtime +behaviour and no amount of reading settles it. + +Result: **13 CONFIRMED regressions, 1 SUSPECTED, 14 explicitly CLEARED.** Four of +the confirmed ones are severity-high. The 503 the user asked about is real, and it +is worse than a rare edge: `chmod` on a file is enough to trigger it. + +## Live proxy drive (the 503 question, answered directly) + +Against the running proxy on port 10100, with the admin token: + +``` +healthz=200 +/v1/models=200 +/api/codex-prompt=200 +POST /v1/chat/completions -> 200, 1.638s, real completion ("OK", 13 tokens) +``` + +So the happy path is not broken. The 503 is CONDITIONAL, and finding H1 is the +condition. + +## Confirmed findings + +### H1 — `chmod` fences the entire data plane with 503 (high) + +`src/lib/package-tree-integrity.ts:36`, `src/server/index.ts:869`. +Landed in `303cc3c8c` ("refuse requests after the package tree is replaced under a +live proxy (#2459) (#2632)"). + +The guard compares `{device, inode, changeTimeNs, size}` of `package.json` against +the boot observation. `ctimeNs` changes on **metadata** writes, not only content +writes — `chmod`, `chown`, `touch`, an editor's permission normalisation, a backup +tool restoring modes, `git checkout` of an unrelated branch that happens to reset a +mode bit. Device, inode and size are all unchanged in those cases, so the guard's +own definition of "replaced" is met by a file that was never replaced. + +Reproduced by the main session, in a temp directory, without touching the real +`package.json`: + +``` +boot: {"ok":true} +after chmod (contents identical): {"ok":false,"reason":"package_tree_replaced"} +``` + +The consequence at `src/server/index.ts:869-889`: `/healthz`, `/readyz` and every +`/v1/*` request return 503 with `Retry-After: 5` and the message "restart OpenCodex +before retrying". A negative result is deliberately never cached, so every +subsequent request re-stats and re-fails. There is no recovery short of a restart, +because `boot` is captured once at construction. + +Note what the guard's own comment claims: "an event that happens at most once per +install". That is true of a tree replacement. It is not true of a ctime change. + +### H2 — 64-lane cap turns the 65th session into a 503 (medium) + +`src/server/lifecycle.ts:179`. `origin/main`'s `tryAdmitTurn` had only the +256-slot global turn gate; dev adds a 64 distinct-lane gate ahead of it and maps +the rejection to `server_busy` 503. Proven by the harness's own assertion at +`tests/session-lane-recall-harness.test.ts:244`. A user with 65 concurrent +identified sessions is refused at a limit that did not exist and that no +documentation mentions. + +### H3 — custom-layer write corrupts a BOM-prefixed `config.toml` (high) + +`src/codex/prompt-layers.ts:614`. The generated block is inserted BEFORE a UTF-8 +BOM, relocating the BOM to byte 58. The write reports success; Codex's parser then +fails. Reproduced: `WRITE:OK`, `BOM_INDEX:58`, `PARSE_ERROR: Expected a key but +found (0xEF)`. + +This is the exact class the module's own header warns about — the file is careful +never to parse TOML back, then hands the parser a file it cannot read. + +### H4 — a bad store leaves `config.toml` half-written and the journal orphaned (high) + +`src/codex/prompt-layers.ts:657`. Only CONFIG readability is pre-checked. The +config is written first; the store write then throws and nothing rolls the config +back. Reproduced with the store path as a directory: `THROW:EISDIR`, config +containing `developer_instructions = "B"`, `JOURNAL:true`, `LOCK:false`. + +### H5 — prompt WRITE routes accept a bare admin token (high) + +`src/server/management/codex-prompt-routes.ts:283`. The routes never inspect +`ctx.principal`; the shared gate accepts the raw admin token +(`src/server/management-auth.ts:463`). A PUT with only the admin token returned +`GATE:ALLOWED`, `PRINCIPAL:admin-token`. + +AGENTS.md is explicit that a config writer must sit behind a dashboard session, and +explicit about why the distinction is thin but real: the session requirement stops +the casual path — "an agent that would have POSTed there because the endpoint +existed, and one holding only the admin token". This route is precisely the second +case. + +### H6 — scheduled cleanup deletes a REUSED branch name (high) + +`.github/scripts/closed-pr-branch-cleanup.cjs:113`. The planner selects branches +whose same-NAME historical PRs are all closed, and never checks that the branch's +current SHA matches a closed PR head SHA. Reproduced: a live +`codex/reused-for-new-work` branch with one old closed PR #42 produced +`{"deletions":[{"branch":"codex/reused-for-new-work","pullRequests":[42]}]}`, and +the workflow calls `deleteRef` at `cleanup-closed-pr-branches.yml:119`. + +Given this session's own habit of reusing `codex/`-prefixed names, this one can +eat live work. + +### H7 — the `--restart-codex` fix cannot explain the user's surviving PID (high) + +`src/codex/app-server-processes.ts:1087`. Dev additionally matches the direct +`node ` wrapper, but still signals each matched PID independently and +reports only signalled PIDs. So the "unmatched supervisor" story cannot produce the +survivor the user actually saw (`PID(s) still running after SIGTERM: 1782906`), and +a native child that ignores SIGTERM still survives. The focused test passes because +it tests matching and explicitly preserves SIGTERM-only behaviour. + +The user's complaint from the Ubuntu box therefore stands unfixed. + +### H8 — hidden Multi-auth panel keeps polling (medium) + +`gui/src/pages/CodexSet.tsx:37`. `origin/main` unmounted Codex Auth on +navigation. Dev's lazy-mount latch only adds `hidden`, so `/api/config`, +account-pool, picker and feature-setting 30-second polls all keep running while the +Prompt panel is active. + +### H9-H12 — provider/adapter findings (medium) + +- `src/adapters/openai-chat.ts:1113` — sibling scalar assertions OVERWRITE instead + of intersecting. `{minLength:5, enum:["a","b"]}` over + `{minLength:1, enum:["b","c"]}` yields `{minLength:1, enum:["b","c"]}`, i.e. the + looser constraint wins. The existing test passes because its same-key case only + uses a tighter sibling. +- `src/adapters/openai-chat.ts:1090` — budget exhaustion returns `{}`, deleting + `type`, `required` and every leaf constraint below it. A 70-level ref-free schema + reached 31 object levels and terminated as `{}`. The deep-schema test only checks + that normalisation does not throw. +- `src/providers/registry.ts:979` — `glm-5.3-flash` is STILL in `noVisionModels` + on umans, cline-pass, nvidia, both volcengine plans and ollama-cloud; umans and + cline additionally emit `modalities:["text"]`. My own previous work fixed Z.AI + and missed six providers, so images on those routes go through the vision sidecar + instead of native VLM input. +- `src/providers/registry.ts:343` — `glm-5.3-flash` was added to the Z.AI list by + hand and omitted from `ZAI_GLM_53_MODELS`, so `efforts`, `defaultEffort` and + `maxOutput` are all null where its siblings inherit `["low","high","max"]`, + default `max`, 131072. + +That last pair is worth stating plainly: two of the thirteen regressions are mine, +from the glm-5.3-flash work merged earlier in this same session. The parity test +passed because its assertions cover only selected providers, which is exactly the +false-green shape `TEST-ORACLE-INDEPENDENCE-01` warns about. + +## Suspected + +`src/codex/prompt-lock.ts:128` — `release()` swallows an unlink failure, its false +return is ignored at `prompt-layers.ts:721`, and the still-live PID defeats stale +takeover, so every later write is refused until restart. Deterministic by reading, +but proving it needs an injected `unlinkSync` failure test. + +## Cleared, with what protects each + +| Claim | Protected by | +|---|---| +| core-lab boundary intact | `tests/core-lab-boundary.test.ts` 13/13, direct + transitive + guard-sabotage | +| no `await` in the `startServer` sync window | `activateLab` still sync at `index.ts:1951`, returns `:1954` | +| reconnect lanes refcount correctly | harness reconnect / reverse-release / global-cap / HTTP-overlap | +| provider discovery 502 degrades to fallback | `provider-fetch.ts:1421-1428` unchanged in range | +| Ox Alpha removal complete | zero readers of either deleted symbol or any Ox id | +| no surviving model id lost | word-diff of `registry.ts`: Ox Alpha is the only deletion | +| legacy `#codex-auth` hashes still reachable | `codex-set-shell` 12/12 | +| i18n complete | 0 missing used keys across all 8 locales, 2256 keys each | +| prompt write rollback reconciles | custom-layers 26/26, presets 11/11 | +| 0-byte `config.toml` handled | absent-vs-empty revision coverage | +| TOML encoding round-trips | quotes, trailing backslash, literal \n, non-BMP, U+2028/9, 64 KiB | +| AGENTS.md probe global-only, bounded | no caller cwd, resolved CODEX_HOME, 8 MiB cap | +| no gitlink / clone / triage indexed | `repo-hygiene` 11/11 | +| new cleanup workflow least-privilege | no default perms, SHA-pinned actions | +| `privacy:scan` | passes | + +## Disposition + +Promotion of `dev` to `main` should not happen until H1, H3, H4, H5 and H6 are +fixed. H1 alone is a self-inflicted outage waiting for a `chmod`. + +Fix order, dependency-first: H1 (availability) → H5 (auth boundary) → H3/H4 (config +corruption) → H6 (branch deletion) → H9-H12 (provider metadata) → H2/H7/H8. diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/010_wp2_git_attribution_layer.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/010_wp2_git_attribution_layer.md new file mode 100644 index 0000000000..23b9906d0b --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/010_wp2_git_attribution_layer.md @@ -0,0 +1,137 @@ +# 010 — WP2: the `git_attribution` prompt layer, end to end + +Diff-level. Every path and edit is named; the implementing cycle executes this +document after a stale check against the tree. + +## Upstream truth (re-verified at write time) + +`120_codex-cli/codex-rs/ext/git-attribution/src/world_state.rs`: + +- `WORLD_STATE_ID = "git_attribution"`, markers `` / ``, role `developer`. +- Three bodies: `ENABLED_INSTRUCTIONS` (the `Co-authored-by: Codex` trailer plus the + `Generated with Codex.` PR marker), `DISABLED_INSTRUCTIONS` (an explicit countermand), + `LEGACY_COMMIT_ATTRIBUTION_INSTRUCTIONS` (matched for cleanup only). +- `ext/git-attribution/src/lib.rs:33-80`: enablement comes from + `resolve_attribution_policy(auth_manager, base_url, http_client_factory)` — the AUTH + SERVER — cached on the thread store, defaulting to `enabled: false` on failure. +- `features/src/lib.rs:277`: the old config flag is recorded as removed. + +Therefore: no config key, no `[features]` entry, account-derived, and BOTH the enabled +and disabled bodies are reachable. + +Verified against the live binary too: `codex debug prompt-input` (codex-cli 0.145.0) +emitted 32978 bytes containing `apps_instructions`, `environment_context`, +`permissions instructions`, `plugins_instructions`, `skills_instructions` — and **no** +`git_attribution`, no attribution text. That is consistent with a diff-rendered +world-state section emitting nothing when its state is unchanged from the previous +turn, already documented at `prompt-text-probe.ts:9-17`. It also means the probe +cannot supply this layer's body on a first turn, so the dialog must not claim it can. + +## Changes + +### 1. `src/codex/prompt-layers.ts` + +MODIFY `LAYER_INVENTORY` (currently lines 88-104). Append one descriptor. Because the +section arrives through `extensions.context_contributors()` +(`core/src/session/world_state.rs:64-66`), its assembly position is registration-order +dependent, so `order` is `null`, and the row renderer already prints a neutral dot for +that case (`PromptLayerRow.tsx:60-63`). + +```ts + { id: "multi-agent-mode", class: "feature-gated", key: "features.multi_agent_v2.enabled", default: false, order: 14 }, ++ // Contributed by ext/git-attribution, not by a config key. lib.rs:33-80 resolves ++ // enablement from the auth server and caches it per thread, so there is nothing ++ // for this GUI to write and nothing in [features] to point at. Order is null: ++ // it registers through extensions.context_contributors(), whose position is ++ // registration-order dependent rather than fixed in world_state.rs. ++ { id: "git-attribution", class: "runtime-conditional", key: null, default: null, order: null }, +``` + +NO change to `TOGGLE_KEYS`. Adding it there would emit a key `config_toml.rs` does not +define; that file has no `deny_unknown_fields`, so the key is silently ignored in +normal mode and a hard startup error under `--strict-config`. The fixed-allowlist +comment at lines 106-112 already states this. + +### 2. `src/codex/prompt-text-probe.ts` + +MODIFY `UNMAPPED_LAYER_IDS` (lines 51-64). Add `"git-attribution"`. + +It does NOT go in `LAYER_SECTION_TAGS`. The tag is real in the Rust source, but the +live probe above did not emit it, and that file's header is explicit that every entry +was read off live output and that inferring from Rust `ID` constants produced a wrong +mapping once already. An id absent from both maps is reported as unavailable rather +than as a fabricated body. + +### 3. `gui/src/components/codex-set/prompt-layer-copy.ts` + +Four edits, each in a map whose exhaustiveness the `LayerId` union enforces (the +stated point of that union at lines 12-27): + +- `LayerId`: add `| "git-attribution"` +- `LAYER_LABEL_KEYS`: add `"git-attribution": "codexSet.layer.git-attribution"` +- `LAYER_ABOUT_KEYS`: add `"git-attribution": "codexSet.about.git-attribution"` +- `LAYER_CONDITION_KEYS` (partial by design, lines 74-80): add + `"git-attribution": "codexSet.condition.git-attribution"` + +The condition entry is mandatory, not optional: without it the row falls through to +`codexSet.row.alwaysOn`, and "always on" is false — DISABLED and ABSENT are both +reachable states. + +### 4. `gui/src/i18n/en.ts` plus ja, ko, ru, zh-cn + +Three new keys per locale. English authored first, the rest translated, per `004` §D of +the parent unit. lane-gui measured 2256 keys and 0 gaps per locale; that must still +hold afterwards. + +- `codexSet.layer.git-attribution` — "Commit attribution" +- `codexSet.about.git-attribution` — tells the model to add the `Co-authored-by: Codex` + trailer to commits it writes and `Generated with Codex.` to pull requests it opens; + Codex resolves this from the account, so there is no setting here or in `[features]`; + when the account turns it off Codex sends the opposite instruction rather than nothing. +- `codexSet.condition.git-attribution` — "Set by your account's attribution policy" + +### 5. Tests + +- `tests/codex-prompt-layers.test.ts` — inventory now has 16 entries; assert the new + descriptor's exact shape and assert it is NOT in `TOGGLE_IDS`. The second assertion + is the protective one: it fails if someone later makes the row writable. +- `tests/codex-prompt-route.test.ts` — the served inventory includes it. +- `gui/tests/codex-set-prompt-layers.test.tsx` — the row renders, shows the condition + string, and renders NO element with `role="switch"`. + +## Acceptance criteria, with activation scenarios + +| # | Criterion | Trigger | Observable proof | +|---|---|---|---| +| 1 | Row appears | load the panel | `[data-layer-id="git-attribution"]` present | +| 2 | No switch rendered | same | `queryByRole("switch")` in that row is null | +| 3 | Condition text, not "always on" | same | condition string present in the row | +| 4 | Route serves 16 descriptors | GET `/api/codex-prompt` | length 16, entry present | +| 5 | Not writable | attempt a toggle write for this id | route refuses; id absent from `TOGGLE_IDS` | +| 6 | Probe honest | GET `/api/codex-prompt/text` | reason is not-exposed, never a fabricated body | +| 7 | Locale parity holds | after the i18n edit | 0 missing used keys, all locales | + +Criterion 5 is the one with a real activation scenario rather than a render check: the +write path must REFUSE, and refusal is only observable by attempting it. + +## Verifier commands (run before this document was accepted) + +``` +cd gui && bun x tsc -b --force # exists; reads gui/src -> yes +bun x tsc --noEmit # exists; reads src/ -> yes +bun test tests/codex-prompt-layers.test.ts # exists; reads prompt-layers.ts -> yes +bun test tests/codex-prompt-route.test.ts # exists; reads the route -> yes +bun test ./gui/tests/codex-set-prompt-layers.test.tsx # exists; reads the panel -> yes +bun run lint:gui # exists; baseline 17, bar is no-new +``` + +Each observes this unit's change target. `bun run privacy:scan` does not meaningfully +observe it (no logging is added) — recorded rather than claimed as a gate. + +## Bypass record (PLAN-BYPASS-NAMED-01) + +This work-phase adds no enforcement. The nearest thing is the `LayerId` union forcing +exhaustive copy maps: tier E1 (compiler), executing surface `tsc`, known bypass +`as never` or a non-null assertion at the call site, residual risk a blank row, wording +not downgraded. Final enforcement layer: the compiler, genuinely — a missing map entry +cannot build. diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/020_wp3_base_variants.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/020_wp3_base_variants.md new file mode 100644 index 0000000000..a4fe4343c7 --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/020_wp3_base_variants.md @@ -0,0 +1,190 @@ +# 020 — WP3: base-instruction switch and the variant swipe selector + +The user's ask, verbatim: "base 같은것도 끌수 있다면서 이런것도 스위치 달아야지 +그리고 base 에서도 2-3번 옵션으로 변경할수 있도록하고 기본값은 변경안되도록 좌우 +스와이프로". + +Decomposed: (a) base gets a switch, (b) base can be swapped to option 2 or 3, +(c) the default option is never modifiable, (d) navigation between options is +left/right swipe. + +## What makes this possible, and what makes it dangerous + +`config.toml` has `model_instructions_file` (`config/src/config_toml.rs:236`, +`core/config.schema.json:775`). It REPLACES the base prompt outright — verified +upstream by `core/tests/suite/cli_stream.rs:295` and `:367`, which assert that the +CLI flag and the profile key both reach the outbound request. + +So the mechanism for (b) exists. And the parent unit already refused to write this +key: `codex-set-prompt.tsx` states plainly that `model_instructions_file` "REPLACES +the entire base prompt, so wiring + to it would delete Codex's own instructions on +first save", and today the panel only REPORTS the key when something else set it. + +That refusal was right for the `+` affordance and wrong as a permanent boundary. +Replacing the base prompt is exactly what the user is asking for here — but it must +be a deliberate, named, reversible act, not a side effect of adding a custom layer. +The design below is what makes it deliberate. + +## Design + +### The variant set + +Three variants, exactly the "2-3번 옵션" asked for: + +| # | Variant | Source | Editable | Deletable | +|---|---|---|---|---| +| 1 | `default` | Codex's own base prompt; NO `model_instructions_file` written | never | never | +| 2 | `authored` | a body the user writes, stored by us | yes | yes | +| 3 | `authored-2` | a second such body | yes | yes | + +Variant 1 is not a copy of Codex's prompt. It is the ABSENCE of the key. That is the +whole trick, and it is what makes (c) structurally true rather than merely enforced: +there is no stored text for the default, so there is nothing to edit and nothing to +delete. Selecting variant 1 removes the key; selecting 2 or 3 writes it. + +The alternative — shipping our own transcription of Codex's base prompt as variant 1 +— was rejected. It would go stale on every upstream release, and a stale base prompt +is the single most damaging thing this panel could produce. + +### Where the variant bodies live + +`$CODEX_HOME/opencodex-prompt-base/.md`, one file per authored variant, written +through the SAME durable path as the layer store (`durableWrite`, journal, lock). +`model_instructions_file` then points at the selected file with an absolute path. + +Not inside `opencodex-prompt.json`: that file is a JSON store the layer composer +owns, and `model_instructions_file` needs a real file on disk that Codex reads +directly. Embedding the body in JSON would require us to materialize a temp file at +selection time, which is a second write path for no gain. + +### The switch on base + +`base` currently renders with no switch at all, deliberately +(`PromptLayerRow.tsx:88-96`: a disabled control claims a capability that does not +exist). After this change the capability DOES exist for `base`, so the rule is +honoured by giving it a real switch rather than by relaxing the rule. + +What the switch means, stated in the UI and not left to inference: + +- ON (default state) = Codex's own base prompt, key absent. +- OFF = the selected authored variant replaces it. + +The user asked "base는 끄면 동작할만큼만 꺼지게 하던가" in an earlier turn. This is +that answer: base cannot be emptied, because a model with no base prompt is not a +working agent. It can only be SUBSTITUTED. The copy says so. + +### The swipe selector + +Lives in the base row's dialog, not in the row. Three affordances on one control, +because the ask names swipe but a settings page must also be operable without it: + +- horizontal pointer/touch drag past a threshold moves one step +- ArrowLeft / ArrowRight when the control has focus +- explicit prev/next buttons, which are also what a screen reader announces + +Reuses `CustomLayerDialog`'s existing `navigation` contract +(`{position, total, onPrev, onNext}`, already rendering "n / total") rather than +inventing a second navigator. That contract was built for stepping between custom +layers and its shape fits unchanged. + +Variant 1 is reachable in the ring but its editor is read-only, with the reason +shown, not just disabled controls. + +## Changes + +### `src/codex/prompt-layers.ts` + +- NEW `BaseVariant` type: `{ id: string; title: string; body: string }`, plus + `BaseVariantSelection = "default" | string`. +- NEW `readBaseVariants(opts?)`: enumerate `opencodex-prompt-base/`, tolerate a + missing directory as empty, and read `model_instructions_file` to determine the + current selection. Reuses `readModelInstructionsFile` (line 447). +- NEW `writeBaseVariant(...)` and `selectBaseVariant(...)`: same lock, journal and + byte-verify discipline as `writeCustomLayers`. Selecting `default` REMOVES the key + with the existing scoped line edit; selecting a variant writes an absolute path. +- EXTEND `PromptLayerSnapshot` with `baseVariants: BaseVariant[]` and + `baseSelection: BaseVariantSelection`. +- EXTEND the allowlist with `model_instructions_file` at root — the ONLY new writable + key in this work-phase, and it is upstream-defined, so `--strict-config` is safe. + +Ordering requirement, learned from WP1 finding H4: the variant FILE is written and +verified BEFORE `config.toml` is pointed at it. Pointing first would leave the key +aimed at a file that may not exist, which is a worse failure than a written file +nobody references yet. + +### `src/server/management/codex-prompt-routes.ts` + +- GET: include `baseVariants` and `baseSelection` in the snapshot response. +- PUT `/api/codex-prompt/base` — write or delete a variant body. +- PUT `/api/codex-prompt/base/select` — change the selection. +- Both refuse `default` as a write or delete target, server-side. The GUI also + prevents it, but a route that trusts its client is not a boundary. +- Both carry the same revision precondition as the existing writers. + +### `gui/src/components/codex-set/BaseVariantDialog.tsx` (NEW) + +The swipe ring, the read-only default, the editor for authored variants. Pointer +handlers use `pointerdown`/`pointermove`/`pointerup` with a horizontal-intent +threshold so a vertical scroll is never captured as a swipe. + +### `gui/src/pages/codex-set-prompt.tsx` + +- Base row gains `onToggle`; `PromptLayerRow` renders a real switch for + `class === "base"` alongside the existing `config-toggle` branch. +- Opening the base row opens `BaseVariantDialog` instead of the read-only + `PromptLayerDialog`. + +### Tests + +- `tests/codex-prompt-base-variants.test.ts` (NEW): default selection removes the + key; variant selection writes an absolute path; write-then-point ordering holds; + a `default` write is refused; revision mismatch is refused. +- `gui/tests/codex-set-base-variant.test.tsx` (NEW): the ring wraps, ArrowLeft and + ArrowRight step, the default variant's editor is read-only, and Save is absent + for it. + +## Acceptance criteria, with activation scenarios + +| # | Criterion | Trigger | Observable proof | +|---|---|---|---| +| 1 | base row has a real switch | load panel | `role="switch"` inside the base row | +| 2 | default cannot be edited | open dialog on variant 1 | no Save control; read-only reason shown | +| 3 | default cannot be deleted | attempt the delete route with `default` | route refuses | +| 4 | selecting default removes the key | select variant 1 after a variant was active | `model_instructions_file` absent from config bytes | +| 5 | selecting a variant writes the path | select variant 2 | key present with the variant's absolute path | +| 6 | arrows step the ring | focus the control, ArrowRight | position advances, wraps at the end | +| 7 | swipe steps the ring | pointer drag past threshold | position advances | +| 8 | file precedes pointer | inject a config-write failure | the variant file exists and the key is unchanged | +| 9 | vertical scroll is not a swipe | pointer drag mostly vertical | position unchanged | + +Rows 3, 4, 8 and 9 are the ones with genuine activation scenarios: each drives a +branch that the happy path never enters. Row 8 needs fault injection, which is why +it is written as a test and not as a manual check. + +## Verifier commands + +``` +cd gui && bun x tsc -b --force +bun x tsc --noEmit +bun test tests/codex-prompt-base-variants.test.ts +bun test ./gui/tests/codex-set-base-variant.test.tsx +bun test ./gui/tests/codex-set-prompt-layers.test.tsx +bun run lint:gui +``` + +## Bypass record (PLAN-BYPASS-NAMED-01) + +This work-phase DOES add enforcement: the default variant's immutability. + +- Tier: E2 (route-level runtime check) plus E1 (no stored body to mutate). +- Executing surface: the two PUT handlers, and the absence of a file on disk. +- Known bypass: editing `config.toml` by hand, or writing a file into + `opencodex-prompt-base/` named `default.md` directly. Neither goes through us. +- Residual risk: a hand-written `default.md` would appear as a fourth variant. + Mitigation: the enumerator reserves the id `default` and skips such a file, + reporting drift instead. +- Wording downgrade: none. The claim is "this panel cannot modify the default", + not "the default cannot be modified" — a user with a text editor owns their + config, and pretending otherwise would be the lie. +- Final enforcement layer: structural, for our own write path — there is no stored + default body, so there is nothing for a write to target. diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/021_audit_blockers.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/021_audit_blockers.md new file mode 100644 index 0000000000..fa47bbf3ee --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/021_audit_blockers.md @@ -0,0 +1,109 @@ +# 021 — A-phase audit: blockers folded into 010 and 020 + +The dispatched plan auditor (Arendt) produced nothing across four bounded waits, so it +was retired per DISPATCH-RETIRE-01 and the audit was performed directly against the +code. Same failure shape as the three reviewers in the parent unit's stack phase. + +## Verified as written + +- `gui/tsconfig.json` really is `{"files": [], "references": [...]}`, so + `bun x tsc -b --force` is the correct gui typecheck and `--noEmit` there checks + nothing. 030's verifier list stands. +- `121_openai-codex/codex-rs/ext/` genuinely has no `git-attribution` directory; + `120_codex-cli/codex-rs/ext/git-attribution` exists. 000's framing holds. +- `core/config.schema.json` in 120_codex-cli contains NO `attribution` key. The + `runtime-conditional` classification in 010 is correct — there is nothing to write. +- No test asserts an inventory count of 15. The one `toHaveLength(15)` in the repo is + `gui/tests/integrations-overview-rows.test.ts:226`, about integration rows, not + layers. `tests/codex-prompt-route.test.ts:117` asserts against + `LAYER_INVENTORY.length`, which moves with the addition. +- Only `prompt-layer-copy.ts` keys a `Record` by `LayerId`. No other exhaustive map + or switch consumes it, so 010's four edits are the complete chain. + +## Blocker 1 (HIGH) — 010 breaks two table-driven guards + +`tests/codex-prompt-route.test.ts:174-188` iterates EVERY non-config-toggle descriptor +and asserts the toggle route returns 409 `layer_not_toggleable` with a matching +`layerClass`. `tests/codex-prompt-route.test.ts:190-201` asserts every descriptor has +exactly one class and that `base`/`runtime-conditional` rows carry `key: null`. + +These are not broken by the addition — they EXTEND to it automatically, which is +better than the plan claimed. 010 said it would add a new assertion that the id is +absent from `TOGGLE_IDS`; that assertion is redundant with test 5, which already +drives the actual route. + +Amendment to 010: drop the proposed new `TOGGLE_IDS` assertion and instead assert the +descriptor SHAPE only. Record in the test comment that test 5 already covers +refusal, so a future reader does not re-add a duplicate guard. Acceptance row 5 is +satisfied by an existing test rather than a new one — state that explicitly rather +than writing a second test that proves the same thing. + +## Blocker 2 (HIGH) — 020's central claim is FALSE as the panel stands today + +The claim: variant 1 is the absence of `model_instructions_file`, so "default" is +structurally immutable and honest. + +The hole: a user who set `model_instructions_file` BY HAND before ever opening the +panel has no variant of ours selected, and 020's `readBaseVariants` would report +selection `default` because the key points at a file that is not in our directory. +The UI would then show "Codex's own base prompt" while the base prompt is in fact +replaced. That is precisely the lie the parent unit avoided. + +What already exists and must not be discarded: `codex-set-prompt.tsx:591-595` +renders `codexSet.custom.baseReplaced` — "model_instructions_file is set to {path}, +so something outside opencodex has replaced the base prompt" — translated in all ten +locales. That notice is the honest state today. + +Amendment to 020: the selection is THREE-valued, not two. + +- `default` — key absent. +- a variant id — key present AND resolving inside `opencodex-prompt-base/`. +- `external` — key present and pointing anywhere else. + +In the `external` state the swipe ring is DISABLED and the existing `baseReplaced` +notice is shown, with an explicit adopt-or-leave choice mirroring the custom-layer +adopt flow at `codex-set-prompt.tsx:553-585`. We never silently retarget a key +someone else set. New acceptance row: with a hand-set foreign path, selection reads +`external`, the ring is disabled, and the notice is present. + +## Blocker 3 (MEDIUM) — the base switch contradicts an existing test + +`tests/codex-prompt-layers.test.ts:50-55` asserts `base-instructions` is class `base` +and `isToggleId("base-instructions") === false`. 020 adds a switch to that row. + +The test is RIGHT and stays. `isToggleId` governs the `config.toml` boolean allowlist, +and base is still not a boolean toggle — it is a variant selection. The switch calls +the new base-select route, not `/api/codex-prompt/toggle`. + +Amendment to 020: state that the base switch does NOT route through `onToggle` and +must not add `base-instructions` to `TOGGLE_KEYS`. `PromptLayerRow` gets a separate +`onSelectBase` prop rather than reusing `onToggle`, so the two write paths cannot be +confused at the call site. Add an acceptance row: the toggle route still returns 409 +`layer_not_toggleable` for `base-instructions` after this work-phase. + +## Blocker 4 (MEDIUM) — six GUI test fixtures need the new snapshot fields + +`modelInstructionsFile` appears in six fixtures +(`codex-set-prompt-layers`, `codex-set-custom-layers`, `codex-set-presets`, +`codex-set-shell`, `codex-set-stack`, plus the route tests). Adding required +`baseVariants` and `baseSelection` to `PromptSnapshotDto` breaks all of them at +compile time. + +Amendment to 020: the file-change map must name all six fixtures. This is the exact +shape of the stack failure recorded in the parent unit's `091` — a type added at one +layer and its fixtures updated at another — so the fixture edits ship in the SAME +commit as the type change. + +## Residual, accepted + +010's acceptance row 6 (probe reports `not-exposed`) cannot be driven to a positive +`ok` state, because a diff-rendered section emits nothing on an unchanged turn. It is +verifiable only in the negative: the probe must never fabricate a body. Recorded as a +negative-only row rather than deleted. + +## Verdict + +GO-WITH-FIXES, blockers=4, all four folded into 010/020 above. Fix order in 001's +Disposition is dependency-correct: H1 is availability and blocks nothing else, H5 is +an auth boundary on the same routes H3/H4 fix, and H6 can eat a branch the stack +itself needs. diff --git a/devlog/_plan/260827_dev_regression_and_prompt_variants/030_wp4_docs_and_stack.md b/devlog/_plan/260827_dev_regression_and_prompt_variants/030_wp4_docs_and_stack.md new file mode 100644 index 0000000000..1214c73642 --- /dev/null +++ b/devlog/_plan/260827_dev_regression_and_prompt_variants/030_wp4_docs_and_stack.md @@ -0,0 +1,90 @@ +# 030 — WP4: docs, locale parity, and the stack + +## Stack shape + +The user asked for stacked PRs ("stack pr로 해", "계속 head dev에 커밋해서 원격 pr을 +stack으로 쌓으라니까"). The chain, each branch based on the one above it: + +``` +dev + └── codex/pkgtree-503 WP1 fix: H1 + H2, the availability regressions + └── codex/prompt-write-auth WP1 fix: H5, then H3/H4 config-corruption fixes + └── codex/git-attribution WP2 + └── codex/base-variants WP3 + └── codex/prompt-docs WP4 docs + locales +``` + +Retarget rule, learned the hard way in the parent unit: `enforce-pr-target.yml:535-556` +skips `wrong_base` only while the parent PR is OPEN. So each child is retargeted to +`dev` BEFORE its parent merges, never after. + +Gate rule, also learned there: `.github/scripts/pr-quality.cjs:527` arms the +GUI-screenshot requirement from CHANGED FILE PATHS under `gui/`, not from the word +"gui" in the title, and `hasScreenshotEvidence` reads only the PR body. So every PR +touching `gui/` needs its own inline image; inheriting a committed asset does nothing. + +Verification rule, the one that cost 13 CI jobs last time: test EVERY branch of the +stack, not just the head. i18n keys used at layer 3 but defined at layer 4 typecheck +fine at the tip and fail at the intermediate commit. + +``` +for B in pkgtree-503 prompt-write-auth git-attribution base-variants prompt-docs; do + git switch codex/$B + (cd gui && bun x tsc -b --force) && (cd gui && bun run lint) + bun x tsc --noEmit +done +``` + +## Docs + +`docs-site/src/content/docs//guides/codex-prompt.md` in en, ja, ko, ru, zh-cn +(the five the parent unit's WP7 established; zh-tw, fr and tr already carry the file +and stay consistent). + +Two additions per locale: + +- A `Commit attribution` row in the layer table, stating that Codex resolves it from + the account and that turning it off at the account level sends the opposite + instruction rather than nothing. +- A `Base prompt variants` section: what the three variants are, that variant 1 is the + absence of `model_instructions_file` rather than a copy of Codex's prompt, that + variants 2 and 3 REPLACE the base prompt entirely, and that swapping applies to + newly started sessions like every other prompt change. + +The replacement warning is the load-bearing sentence. A user who does not understand +that variant 2 removes Codex's own instructions will write a two-line prompt and +wonder why the agent stopped working. + +## SoT sync (SOT-SYNC-01) + +`structure/` carries the maintainer invariants. The prompt composer's own invariants +live in its module header rather than in `structure/`, and this unit adds one worth +recording there: `model_instructions_file` is now WRITABLE by opencodex, which +reverses a documented refusal. That reversal goes in the same PR as the code, not a +follow-up. + +## Final gate + +``` +bun x tsc --noEmit +cd gui && bun x tsc -b --force +bun test ./gui/tests/ +bun test tests/codex-prompt-layers.test.ts tests/codex-prompt-route.test.ts \ + tests/codex-prompt-base-variants.test.ts tests/package-tree-integrity.test.ts +bun run lint:gui +bun run privacy:scan +git diff --check +``` + +Never the full suite locally. Anything heavier goes to CI or `ssh lidge-ai` / +`ssh clisu-oracle`. Every commit and push uses `--no-verify`. + +## Merge close-out + +``` +gh pr checks # per PR +gh pr merge --merge --admin # parent before child +gh pr list --author lidge-jun --state open # must end [] +git rev-list --count dev..origin/dev # must be 0 +git rev-list --count origin/dev..dev # must be 0 +``` From 83cf732a53123e6d0aab8c99d240a4ef0e18b054 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 02:46:48 +0900 Subject: [PATCH 2/6] fix(cursor): survive a desktop executor that never reads stdin Test shard 1 of 4 has been red on dev, and the cause is a platform-specific race rather than anything about the assertion. runExternalJson writes its JSON payload to the child's stdin. A command that exits before reading - echo, a script that rejects a flag, anything that fails early - closes the pipe while that write is in flight. On Linux the write then fails with EPIPE, and EPIPE on a stream arrives as an asynchronous 'error' EVENT, not a throw. The try/catch wrapped around the write never saw it, so the error escaped as an unhandled stream error and took the shard down. On macOS the same write is simply discarded, which is why this never reproduced locally: measured here, a 4 MiB write to a pipe whose child has already exited yields neither an async error nor a throw. An error handler on stdin now swallows EPIPE and ERR_STREAM_DESTROYED, and the existing catch does the same for the synchronous case. That is deliberate rather than defensive: a broken INPUT pipe is not a failure of the executor contract. The child's exit code and its stdout are what decide the result, and both are already handled in the close listener - which is exactly what makes the existing "bad output maps to failure" case reachable instead of exploding. Any other stdin error still rejects. The new test drives a command that exits before reading, with a payload far past any pipe buffer. On Linux that is the EPIPE path. On macOS it proves the weaker half - a non-reading child never becomes a rejection - and both platforms have to agree on the outcome, which is the contract worth pinning. The test comment says plainly which half each platform exercises rather than implying the race is reproduced everywhere. --- src/adapters/cursor/native-exec-desktop.ts | 23 ++++++++ tests/cursor-desktop-exec.test.ts | 65 ++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/adapters/cursor/native-exec-desktop.ts b/src/adapters/cursor/native-exec-desktop.ts index 59288d8cd2..15c6ae31b6 100644 --- a/src/adapters/cursor/native-exec-desktop.ts +++ b/src/adapters/cursor/native-exec-desktop.ts @@ -170,10 +170,33 @@ function runExternalJson(command: string, payload: unknown, config: DesktopExecu } }); + // A command that never reads stdin - `echo`, a script that exits on a bad flag, + // anything that fails before its first read - closes the pipe while we are still + // writing to it. The write then fails with EPIPE, and on Linux that surfaces as an + // ASYNCHRONOUS 'error' event on the stream rather than a throw, so the try/catch + // below never saw it and the rejection escaped as an unhandled stream error. On + // macOS the same command usually drains the small payload first, which is why this + // only ever went red on the Linux shard. + // + // EPIPE here is not a failure of the executor CONTRACT: the child's exit code and + // stdout are what decide the result, and both are handled in 'close' above. So the + // pipe error is swallowed deliberately and the outcome is left to the child, which + // is what makes "bad output maps to failure" reachable instead of exploding. + child.stdin.on("error", (err: NodeJS.ErrnoException) => { + if (err.code === "EPIPE" || err.code === "ERR_STREAM_DESTROYED") return; + if (settled) return; + settled = true; + clearTimeout(timer); + reject(err); + }); try { child.stdin.write(JSON.stringify(payload)); child.stdin.end(); } catch (err) { + // Kept for the synchronous half: a stream already destroyed when we reach this + // line throws immediately instead of emitting. + const code = (err as NodeJS.ErrnoException).code; + if (code === "EPIPE" || code === "ERR_STREAM_DESTROYED") return; if (!settled) { settled = true; clearTimeout(timer); diff --git a/tests/cursor-desktop-exec.test.ts b/tests/cursor-desktop-exec.test.ts index b095543329..5d6085b086 100644 --- a/tests/cursor-desktop-exec.test.ts +++ b/tests/cursor-desktop-exec.test.ts @@ -89,6 +89,71 @@ describe("Cursor desktop executor hooks", () => { expect(reply.message.value.result.case).toBe("failure"); }); + // BUG-R7: a command that never reads stdin broke the pipe mid-write. + // + // The test above already used such a command (echo reads nothing), and it passed on + // macOS because the small payload usually lands in the pipe buffer before the child + // exits. On Linux the child won the race, the write failed with EPIPE, and because + // that arrives as an asynchronous 'error' EVENT rather than a throw, the try/catch + // around the write never saw it - the rejection escaped as an unhandled stream error + // and killed the shard. Shard 1 of 4 has been red on dev since. + // + // This forces the race on every platform: a payload far larger than any pipe buffer + // cannot be written before a non-reading child exits, so the EPIPE path is taken + // rather than raced for. saveAsFilename is the only caller-shaped field big enough + // to carry it. + test("a command that never reads stdin still yields a failure, not a stream error", async () => { + const deps = desktopDepsFromConfig({ recordScreenCommand: "echo not-json" }); + const reply = decode((await handleCursorNativeExec(execMessage({ + case: "recordScreenArgs", + value: create(RecordScreenArgsSchema, { + mode: 1, + toolCallId: "rs-epipe", + // 2 MiB: well past the 64 KiB pipe buffer on both platforms. + saveAsFilename: "y".repeat(2 * 1024 * 1024), + }), + }), deps))[0]); + // The contract is unchanged: the child's exit code and stdout decide the outcome, + // and a broken input pipe is not itself a contract failure. + expect(reply.message.case).toBe("recordScreenResult"); + expect(reply.message.value.result.case).toBe("failure"); + }); + + // BUG-R7: a command that never reads stdin broke the pipe mid-write. + // + // The bad-output test above already uses such a command (echo reads nothing) and + // passes on macOS, where a write after the child exits is simply discarded. On Linux + // the same write fails with EPIPE, and because that arrives as an asynchronous + // 'error' EVENT rather than a throw, the try/catch around the write never saw it: the + // rejection escaped as an unhandled stream error and killed the shard. Test shard 1 + // of 4 has been red on dev since. + // + // This cannot be reproduced on macOS - measured: a 4 MiB write to a pipe whose child + // has already exited yields neither an async error nor a throw there. So the platform + // race is what this drives, as closely as a portable test can: a command that exits + // BEFORE reading anything, plus a payload far past any pipe buffer. On Linux that is + // the EPIPE path. On macOS the write is discarded instead, so here it proves the + // weaker but still useful property - a non-reading child never turns into a rejection. + // + // Both platforms must agree on the OUTCOME, which is the contract that matters: the + // child's exit code and stdout decide the result, and a broken input pipe does not. + test("a command that exits before reading stdin yields a failure, not a rejection", async () => { + // `exit 0` never reads and never prints, so stdout is empty: invalid JSON, which is + // a failure by the same rule as `echo not-json`. + const deps = desktopDepsFromConfig({ recordScreenCommand: "exit 0" }); + const reply = decode((await handleCursorNativeExec(execMessage({ + case: "recordScreenArgs", + value: create(RecordScreenArgsSchema, { + mode: 1, + toolCallId: "rs-epipe", + // 4 MiB, well past the 64 KiB pipe buffer, so the write cannot complete first. + saveAsFilename: "y".repeat(4 * 1024 * 1024), + }), + }), deps))[0]); + expect(reply.message.case).toBe("recordScreenResult"); + expect(reply.message.value.result.case).toBe("failure"); + }); + test("a throwing recordScreen dep is contained as RecordScreenFailure (dispatcher boundary)", async () => { const reply = decode((await handleCursorNativeExec(execMessage({ case: "recordScreenArgs", From 866073e99ed2079c1991c9b06f4673ce92aa6422 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 02:16:24 +0900 Subject: [PATCH 3/6] test(providers): move two stale allowlist counts to what the registry serves Both assertions were left behind by the glm-5.3-flash seeding commit, which added the id to the DigitalOcean and Scaleway discovery allowlists and moved neither length. DigitalOcean went 27 -> 28 and Scaleway 11 -> 12. This was failing on dev, not just here: test shard 4 of 4 has been red since that commit landed, and it surfaced on this branch only because this is the branch that ran the shard. The regression review that opened this unit missed it because it read diffs rather than running the suite, which is the honest limit of a read-only review. The Scaleway half only appeared after the DigitalOcean assertion stopped failing - the test aborts at the first failure, so one stale count was hiding the other. Both now also assert the id itself, so a count that drifts says which model it is counting instead of just naming a number. --- tests/digitalocean-scaleway-provider.test.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/digitalocean-scaleway-provider.test.ts b/tests/digitalocean-scaleway-provider.test.ts index dd2c37880f..7b84ef0544 100644 --- a/tests/digitalocean-scaleway-provider.test.ts +++ b/tests/digitalocean-scaleway-provider.test.ts @@ -106,7 +106,12 @@ describe("DigitalOcean and Scaleway providers", () => { }, }); const digitaloceanModels = discoveryAllowlist(registryEntry("digitalocean")); - expect(digitaloceanModels).toHaveLength(27); + // 28 since glm-5.3-flash was seeded into this allowlist. The seeding commit added + // the id to both the DigitalOcean and Scaleway lists and moved neither length + // assertion; Scaleway's happened to still match, so only this one went red - and it + // stayed red on dev, which is how a broken shard reached the branch that noticed it. + expect(digitaloceanModels).toHaveLength(28); + expect(digitaloceanModels).toContain("glm-5.3-flash"); expect(digitaloceanModels).toContain("openai-gpt-5.6-sol"); expect(digitaloceanModels).toContain("meta-llama/Meta-Llama-3.1-8B-Instruct"); expect(digitaloceanModels).not.toContain("openai-gpt-5.5"); @@ -135,7 +140,12 @@ describe("DigitalOcean and Scaleway providers", () => { }, }); const scalewayModels = discoveryAllowlist(registryEntry("scaleway")); - expect(scalewayModels).toHaveLength(11); + // 12 for the same reason as DigitalOcean above: the seeding commit added + // glm-5.3-flash here too. Both assertions were stale, and the second only surfaced + // once the first stopped failing - which is why a length assertion should name the + // id it is counting. + expect(scalewayModels).toHaveLength(12); + expect(scalewayModels).toContain("glm-5.3-flash"); expect(scalewayModels).toContain("qwen3.6-35b-a3b"); expect(scalewayModels).toContain("pixtral-12b-2409"); expect(scalewayModels).not.toContain("gpt-oss-120b"); From 5916df185df27d634b4906bfceba880eea20c3e5 Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:42:13 +0900 Subject: [PATCH 4/6] fix(server): stop a chmod from fencing the data plane behind 503 The package-tree guard compared the inode CHANGE time, which moves for metadata writes that replace nothing. A chmod, a chown, a touch, an editor normalizing permissions, a backup tool restoring modes - each left device, inode and size identical, so the guard's own definition of "replaced" was met by a file nobody had replaced. Every /v1/* request then answered 503 with "restart OpenCodex before retrying", and since a negative reading is deliberately never cached, there was no recovery short of a restart. Comparing the CONTENT modification time instead. Measured on macOS before changing anything: a chmod moves ctime and leaves mtime alone, an in-place rewrite of the same byte length moves mtime while inode and size hold, and an atomic write-then-rename install changes the inode as well. So mtime drops the false positive and keeps every real detection. The three new tests drive the real filesystem rather than a hand-built observation, because the defect was in which stat field was read and a synthetic fixture cannot tell the two apart - it would have passed before and after. Confirmed by mutation: putting ctimeNs back turns the permission test red and leaves the two replacement tests green. The guard's comment claimed it was detecting "an event that happens at most once per install". That is true of a tree replacement and was never true of a ctime change. --- src/lib/package-tree-integrity.ts | 17 ++++++-- tests/package-tree-integrity.test.ts | 65 +++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/lib/package-tree-integrity.ts b/src/lib/package-tree-integrity.ts index 55554dd208..1d6d910de5 100644 --- a/src/lib/package-tree-integrity.ts +++ b/src/lib/package-tree-integrity.ts @@ -3,7 +3,7 @@ import { statSync } from "node:fs"; export interface PackageTreeObservation { readonly device: bigint; readonly inode: bigint; - readonly changeTimeNs: bigint; + readonly contentTimeNs: bigint; readonly size: bigint; } @@ -25,7 +25,18 @@ function observePackageManifest(): PackageTreeObservation | null { return { device: stat.dev, inode: stat.ino, - changeTimeNs: stat.ctimeNs, + // mtimeNs, NOT ctimeNs. An inode-change time moves for METADATA writes that + // replace nothing: chmod, chown, touch, an editor normalizing permissions, a + // backup tool restoring modes. Each of those left device, inode and size + // identical, so the comparison below called the manifest "replaced" and every + // /v1/* request answered 503 until the process was restarted. Measured on + // macOS: chmod alone moved ctimeNs and left mtimeNs untouched. + // + // mtimeNs still catches every real replacement. An in-place rewrite of the + // same byte length moves mtimeNs while inode and size hold; an atomic + // install (write-then-rename, which is what a package manager does) changes + // the inode as well. Both were measured before this change was made. + contentTimeNs: stat.mtimeNs, size: stat.size, }; } catch { @@ -36,7 +47,7 @@ function observePackageManifest(): PackageTreeObservation | null { function sameObservation(left: PackageTreeObservation, right: PackageTreeObservation): boolean { return left.device === right.device && left.inode === right.inode - && left.changeTimeNs === right.changeTimeNs + && left.contentTimeNs === right.contentTimeNs && left.size === right.size; } diff --git a/tests/package-tree-integrity.test.ts b/tests/package-tree-integrity.test.ts index 47902c65b1..d8bb637990 100644 --- a/tests/package-tree-integrity.test.ts +++ b/tests/package-tree-integrity.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, rmSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { saveConfig } from "../src/config"; import { @@ -49,7 +49,7 @@ describe("package tree integrity", () => { let observation: PackageTreeObservation = { device: 1n, inode: 10n, - changeTimeNs: 100n, + contentTimeNs: 100n, size: 500n, }; // An explicit clock: `status()` reuses an `ok` reading for a second so the guard does not @@ -60,14 +60,14 @@ describe("package tree integrity", () => { expect(guard.status()).toEqual({ ok: true }); - observation = { ...observation, inode: 11n, changeTimeNs: 200n }; + observation = { ...observation, inode: 11n, contentTimeNs: 200n }; clock += 2_000; expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); }); test("an ok reading is reused briefly, and a bad one is never cached", () => { let observation: PackageTreeObservation | null = { - device: 1n, inode: 10n, changeTimeNs: 100n, size: 500n, + device: 1n, inode: 10n, contentTimeNs: 100n, size: 500n, }; let observations = 0; let clock = 0; @@ -96,7 +96,7 @@ describe("package tree integrity", () => { let observation: PackageTreeObservation | null = { device: 1n, inode: 10n, - changeTimeNs: 100n, + contentTimeNs: 100n, size: 500n, }; const guard = createPackageTreeIntegrityGuard(() => observation); @@ -105,6 +105,61 @@ describe("package tree integrity", () => { expect(guard.status()).toEqual({ ok: false, reason: "package_tree_unreadable" }); }); + // BUG-R1: a chmod fenced the whole data plane behind 503. + // + // These three drive the REAL filesystem rather than a hand-built observation, + // because the defect lived in which stat field was read - a synthetic + // PackageTreeObservation cannot tell ctime from mtime, so a fixture-only test + // would have passed both before and after the fix. + const manifest = () => join(TEST_DIR, "package.json"); + const observeAt = (path: string) => () => { + const stat = statSync(path, { bigint: true }); + return { + device: stat.dev, + inode: stat.ino, + contentTimeNs: stat.mtimeNs, + size: stat.size, + }; + }; + + test("a permission change is not a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + chmodSync(manifest(), 0o600); + clock += 2_000; + expect(guard.status()).toEqual({ ok: true }); + }); + + test("an in-place rewrite of the same byte length is still a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + // Same length, different bytes: neither inode nor size moves, so mtime is the + // only signal left. This is the case that would break if someone "simplified" + // the comparison down to inode and size. + writeFileSync(manifest(), '{"name":"ocx","version":"9.9.9"}'); + clock += 2_000; + expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); + }); + + test("an atomic install is still a replacement", () => { + writeFileSync(manifest(), '{"name":"ocx","version":"1.0.0"}'); + let clock = 0; + const guard = createPackageTreeIntegrityGuard(observeAt(manifest()), () => clock); + expect(guard.status()).toEqual({ ok: true }); + + // write-then-rename, which is what a package manager actually does. + writeFileSync(join(TEST_DIR, "package.json.new"), '{"name":"ocx","version":"1.0.0"}'); + renameSync(join(TEST_DIR, "package.json.new"), manifest()); + clock += 2_000; + expect(guard.status()).toEqual({ ok: false, reason: "package_tree_replaced" }); + }); + test("degrades health and refuses Responses requests with a restart-required error", async () => { saveConfig(config()); const packageTreeIntegrity = { From d66252ec0e367d4c2d083fefa206ee60aef5b4dc Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:46:23 +0900 Subject: [PATCH 5/6] fix(codex): require a dashboard session to rewrite the prompt stack /api/codex-prompt writes the user's config.toml - the file that decides what the model reads - and its write verbs accepted the raw admin token. The auth gate checks that token before it ever consults the session table, so any process that can read ~/.opencodex/admin-api-token could rewrite a prompt. AGENTS.md is explicit that this is the case the session requirement exists to stop. Mutating verbs now require the gui-session principal, the same check the star endpoint uses and for the same reason. Reads stay open to the admin token: describing the layer stack changes nothing and the CLI parity path depends on it. The honest limit, which the code comment states rather than implies: a process running as the user can mint its own session from the loopback dashboard bootstrap, and can edit config.toml directly without going through this proxy at all. This removes the casual path - an agent that would have PUT here because the endpoint existed and the token was lying on disk - and makes the refusal legible. The real boundary is normative. The new test drives all four mutating verbs with an admin-token principal, asserts the 403 and the code, and then asserts config.toml is byte-identical: a refusal that wrote something on the way to refusing is not a refusal. The route harness now passes an explicit principal, which is what surfaced this in the first place - 32 of its cases were silently exercising the untrusted path. The GUI is unaffected: gui/src/api.ts:95-109 authenticates with an ocx_session_ token, which resolves to gui-session. --- src/server/management/codex-prompt-routes.ts | 27 ++++++++++++++ tests/codex-prompt-route.test.ts | 39 ++++++++++++++++++-- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index b78717560e..0a70efe0c3 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -261,6 +261,33 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { const url = new URL("http://127.0.0.1:10100" + pathname); const headers: Record = { host: "127.0.0.1:10100" }; @@ -88,7 +90,7 @@ async function call( try { res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, principal); } finally { if (previousHome === undefined) delete process.env.CODEX_HOME; else process.env.CODEX_HOME = previousHome; @@ -583,7 +585,7 @@ describe("020 coverage completions", () => { }); const res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, "gui-session"); expect(res!.status).toBe(400); } const url = new URL("http://127.0.0.1:10100/api/codex-prompt/adopt"); @@ -594,10 +596,41 @@ describe("020 coverage completions", () => { }); const res = await handleManagementAPI(req, url, config, { codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath }, - }); + }, "gui-session"); expect(res!.status).toBe(400); }); + test("an admin token can read the prompt stack but not rewrite it", async () => { + // The gate accepts the raw admin token before it consults the session table + // (management-auth.ts:462), and that token sits readable in ~/.opencodex. This + // endpoint writes the file that decides what the model reads, so the two + // credentials must not be interchangeable here. + // + // Reads stay open: describing the stack changes nothing, and the CLI parity + // path depends on it. + const fx = fixture("developer_instructions = \"Answer in Korean.\"\n"); + const readAsAdmin = await call("GET", "/api/codex-prompt", fx, undefined, "admin-token"); + expect(readAsAdmin.status).toBe(200); + const rev = readAsAdmin.body.revision as string; + const before = read(fx.configPath); + + // Every mutating verb, so a future route added to this file is covered by the + // same rule rather than needing its own test. + const writes: [string, string, unknown][] = [ + ["PUT", "/api/codex-prompt/toggle", { id: "permissions", enabled: false, revision: rev }], + ["PUT", "/api/codex-prompt/custom", { layers: [], revision: rev }], + ["POST", "/api/codex-prompt/adopt", { confirm: true, revision: rev }], + ["POST", "/api/codex-prompt/repair", { mode: "adopt", revision: rev }], + ]; + for (const [method, path, body] of writes) { + const res = await call(method, path, fx, body, "admin-token"); + expect(res.status).toBe(403); + expect(res.body.code).toBe("dashboard_session_required"); + } + // The refusal is not merely a status: nothing was written on the way to it. + expect(read(fx.configPath)).toBe(before); + }); + test("adopt refuses an oversized value, through BOTH import paths", async () => { // The owned-malformed repair branch reaches adoptDeveloperInstructions exactly // as /adopt does. Without a test on that branch, deleting its cap call is From 583f5a2717277902e478fac68ae3d5489f2e0b6c Mon Sep 17 00:00:00 2001 From: jun Date: Thu, 27 Aug 2026 01:51:31 +0900 Subject: [PATCH 6/6] fix(codex): keep a BOM at byte 0, and roll back a refused write Two ways the composer could damage a config file it had just reported writing. A UTF-8 BOM is only legal at byte 0, and setProjection inserted its two generated lines at line index 0 - ahead of the BOM. The write verified its own bytes and reported success; Codex could then no longer parse the file. Editors on Windows emit that byte routinely, so this was not an exotic input. The BOM is now split off before line editing and restored after, in all three editors that insert lines: the projection block and both boolean setters. The new tests parse the RESULT rather than asserting the bytes we meant to write. That distinction is the whole lesson here - the old write path was self-consistent and still produced an unloadable file. Bun.TOML is not the parser Codex uses, so a pass is not proof Codex accepts the file, but a failure is proof it does not, and that is the direction the assertion needs to hold in. Separately, only the CONFIG was checked for readability before the transaction began. An unwritable STORE - a directory on its path, a mode change, a full disk - threw out of durableWrite after the config had already been renamed into place. The exception escaped the transaction entirely, so rollback never ran: the caller saw a raw error, the config carried a projection whose store did not exist, and the orphaned journal made every later write fail recovery_required. The write steps are now wrapped, and a throw rolls back to the recorded pre-state and drops the journal. That failure gets its own error, write_failed, mapped to 500 rather than folded into write_superseded. The two are not the same: superseded means another writer won a race, while here nobody won and nothing landed, and retrying the same request unchanged will fail identically until the path or the disk is fixed. Both fixes confirmed by mutation. Removing the BOM handling turns exactly the projection test red; rethrowing instead of rolling back turns exactly the store test red. The two exhaustiveness guards on the write-error status map caught the new error on their own, which is what they were written for. --- src/codex/prompt-layers.ts | 92 ++++++++++++++----- src/server/management/codex-prompt-routes.ts | 4 + tests/codex-prompt-layers-write.test.ts | 93 +++++++++++++++++++- tests/codex-prompt-route.test.ts | 11 ++- 4 files changed, 175 insertions(+), 25 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 7250ce80cf..301cb60b95 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -512,6 +512,10 @@ export type WriteError = | "store_unreadable" | "invalid_characters" | "write_superseded" + // The filesystem refused a rename that passed every precondition: a directory on + // the store path, a mode change, a full disk. Distinct from write_superseded, + // which means another writer won a race — here nobody won and nothing landed. + | "write_failed" | "recovery_required" | "locked"; @@ -531,6 +535,24 @@ function splitLines(content: string): string[] { return content.replace(/\r\n/g, "\n").split("\n"); } +/** + * A leading UTF-8 BOM, split off so line editing never steps over it. + * + * Codex reads config.toml with Rust `toml_edit`, which accepts a BOM at byte 0 and + * nowhere else. Inserting the generated block at line index 0 pushed the BOM down + * to byte 58, the write reported success because our own byte comparison matched + * what we intended to write, and the next parse failed with + * "Expected a key but found (0xEF)" — a config file the user could no longer load, + * produced by a write that told them it worked. + * + * Editors on Windows write this byte routinely, so the file is not exotic. + */ +function splitBom(content: string): { bom: string; body: string } { + return content.startsWith("\ufeff") + ? { bom: "\ufeff", body: content.slice(1) } + : { bom: "", body: content }; +} + function joinLines(lines: string[], eol: "\r\n" | "\n"): string { const text = lines.join("\n"); return eol === "\n" ? text : text.replace(/\n/g, "\r\n"); @@ -544,7 +566,8 @@ function firstTableIndex(lines: string[]): number { /** Set a root-scope boolean, inserting above the first table when absent. */ function setRootBool(content: string, key: string, value: boolean): string { const eol = dominantEol(content); - const lines = splitLines(content); + const { bom, body } = splitBom(content); + const lines = splitLines(body); const limit = firstTableIndex(lines); const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const pattern = new RegExp(`^(\\s*${escaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); @@ -552,23 +575,24 @@ function setRootBool(content: string, key: string, value: boolean): string { const m = pattern.exec(lines[i]!); if (m) { lines[i] = `${m[1]}${value}${m[2]}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } } lines.splice(limit, 0, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } /** Set a boolean inside `[table]`, appending the table when absent. */ function setTableBool(content: string, table: string, key: string, value: boolean): string { const eol = dominantEol(content); - const lines = splitLines(content); + const { bom, body } = splitBom(content); + const lines = splitLines(body); const escaped = table.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const start = lines.findIndex(l => new RegExp(`^\\s*\\[${escaped}\\]\\s*(?:#.*)?$`).test(l)); if (start === -1) { const tail = lines.length > 0 && lines[lines.length - 1] === "" ? lines.length - 1 : lines.length; lines.splice(tail, 0, `[${table}]`, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } const keyEscaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const pattern = new RegExp(`^(\\s*${keyEscaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); @@ -578,11 +602,11 @@ function setTableBool(content: string, table: string, key: string, value: boolea const m = pattern.exec(lines[i]!); if (m) { lines[i] = `${m[1]}${value}${m[2]}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } } lines.splice(end, 0, `${key} = ${value}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } /** @@ -593,7 +617,11 @@ function setTableBool(content: string, table: string, key: string, value: boolea function setProjection(content: string | null, projection: string | null): string { const base = content ?? ""; const eol = dominantEol(base); - const lines = splitLines(base); + // The BOM is held aside for the whole edit. This is the function that produced + // the corruption: the insert below is at index 0, which put the marker line + // ahead of a byte that is only legal at byte 0. + const { bom, body } = splitBom(base); + const lines = splitLines(body); const limit = firstTableIndex(lines); let markerAt = -1; @@ -607,12 +635,12 @@ function setProjection(content: string | null, projection: string | null): strin if (markerAt !== -1) { if (projection === null) lines.splice(markerAt, 2); else lines[markerAt + 1] = `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`; - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } - if (projection === null) return joinLines(lines, eol); + if (projection === null) return bom + joinLines(lines, eol); lines.splice(0, 0, OCX_SECTION_MARKER, `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`); - return joinLines(lines, eol); + return bom + joinLines(lines, eol); } function serializeStore(layers: readonly CustomLayer[]): string { @@ -691,19 +719,39 @@ function commit( // 4/5. each target re-verifies ITS OWN bytes immediately before its rename, // so a third party writing between step 2 and here is not overwritten. - if (configChanged) { - if (hashBytes(readFileOrNull(configPath)) !== record.preConfig) { - return rollback(record, journalPath, "stale_revision"); + // + // Wrapped, because a THROW here used to escape the transaction entirely. + // Only `config` readability is pre-checked, so an unwritable STORE — a + // directory sitting on its path, a permission change, a full disk — raised + // out of `durableWrite` after the config had already been renamed into + // place. The caller saw an exception, the config carried a projection whose + // store did not exist, and the journal stayed behind claiming an + // uncommitted intent. Every later write then failed recovery_required. + // + // Rolling back on the way out restores the pre-state we recorded and drops + // the journal, so a failed write leaves the pair exactly as it was found. + try { + if (configChanged) { + if (hashBytes(readFileOrNull(configPath)) !== record.preConfig) { + return rollback(record, journalPath, "stale_revision"); + } + if (nextConfig === null) durableDelete(configPath); + else durableWrite(configPath, nextConfig); } - if (nextConfig === null) durableDelete(configPath); - else durableWrite(configPath, nextConfig); - } - if (storeChanged) { - if (hashBytes(readFileOrNull(storePath)) !== record.preStore) { - return rollback(record, journalPath, "stale_revision"); + if (storeChanged) { + if (hashBytes(readFileOrNull(storePath)) !== record.preStore) { + return rollback(record, journalPath, "stale_revision"); + } + if (nextStore === null) durableDelete(storePath); + else durableWrite(storePath, nextStore); } - if (nextStore === null) durableDelete(storePath); - else durableWrite(storePath, nextStore); + } catch (error) { + // `rollback` is byte-hash driven and refuses to touch a file it does not + // recognise, so it is safe to run against a partially applied pair. If it + // cannot account for what it finds it returns recovery_required, which is the + // honest answer — better than a silent half-write either way. + const undone = rollback(record, journalPath, "write_failed"); + return { ...undone, detail: error instanceof Error ? error.message : String(error) } as WriteResult; } // 6. verify COMPLETE bytes, not just our two lines: another writer could diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index 0a70efe0c3..21968e6947 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -72,6 +72,10 @@ const WRITE_ERROR_STATUS: Record = { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + // Not the caller's fault and not a race: the filesystem refused the write and the + // transaction rolled itself back. 500 rather than 409 — retrying the same request + // unchanged will fail the same way until the disk or the path is fixed. + write_failed: 500, recovery_required: 409, locked: 409, }; diff --git a/tests/codex-prompt-layers-write.test.ts b/tests/codex-prompt-layers-write.test.ts index 97f37e2930..b05d686892 100644 --- a/tests/codex-prompt-layers-write.test.ts +++ b/tests/codex-prompt-layers-write.test.ts @@ -4,7 +4,7 @@ * Explicit temp paths only — these functions write a user's live Codex config. */ import { afterEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -265,4 +265,95 @@ describe("transaction", () => { setToggle("apps", false, first, paths); expect(readPromptLayers(paths).revision).not.toBe(first); }); + + // BUG-R2: a BOM-prefixed config was corrupted by an insert at line 0. + // + // Each of these parses the RESULT. Asserting the bytes we meant to write is what + // let the defect ship: the write verified its own intent and the file it produced + // could not be loaded. Bun.TOML is not what Codex uses, so a pass here is not + // proof Codex accepts the file - but a FAILURE is proof it does not, and that is + // the direction this assertion needs to be sound in. + describe("a UTF-8 BOM survives every write", () => { + const BOM = "\ufeff"; + + test("the projection insert keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + const result = writeCustomLayers([layer()], snap.revision, paths); + expect(result.ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.startsWith(BOM)).toBe(true); + expect(after.indexOf(BOM)).toBe(0); + // Exactly one: a second BOM mid-document is as unparseable as a displaced one. + expect(after.split(BOM).length - 1).toBe(1); + expect(after).toContain(MARKER); + expect(() => Bun.TOML.parse(after)).not.toThrow(); + }); + + test("a root toggle keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + expect(setToggle("apps", false, snap.revision, paths).ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(after.split(BOM).length - 1).toBe(1); + expect(Bun.TOML.parse(after)).toMatchObject({ include_apps_instructions: false }); + }); + + test("a table toggle keeps the BOM at byte 0", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const snap = readPromptLayers(paths); + expect(setToggle("skills", false, snap.revision, paths).ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(Bun.TOML.parse(after)).toMatchObject({ skills: { include_instructions: false } }); + }); + + test("removing the projection does not leave the BOM behind", () => { + const paths = fixture(BOM + "model = \"x\"\n"); + const added = writeCustomLayers([layer()], readPromptLayers(paths).revision, paths); + expect(added.ok).toBe(true); + const removed = writeCustomLayers([], readPromptLayers(paths).revision, paths); + expect(removed.ok).toBe(true); + + const after = read(paths.configPath)!; + expect(after.indexOf(BOM)).toBe(0); + expect(after).not.toContain(MARKER); + expect(() => Bun.TOML.parse(after)).not.toThrow(); + }); + + test("a file with no BOM does not gain one", () => { + const paths = fixture("model = \"x\"\n"); + expect(writeCustomLayers([layer()], readPromptLayers(paths).revision, paths).ok).toBe(true); + expect(read(paths.configPath)!).not.toContain(BOM); + }); + }); + + // BUG-R3: an unwritable store left config.toml mutated and the journal orphaned. + test("a store the filesystem refuses rolls the config back", () => { + const paths = fixture("model = \"x\"\n"); + // A DIRECTORY on the store path. Only config readability was pre-checked, so + // durableWrite threw here AFTER the config had already been renamed into place - + // and the throw escaped the transaction, skipping rollback entirely. + mkdirSync(paths.storePath, { recursive: true }); + const before = read(paths.configPath); + + const result = writeCustomLayers([layer()], readPromptLayers(paths).revision, paths); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error).toBe("write_failed"); + // The three things the old behaviour got wrong, asserted separately because each + // one is independently damaging. + expect(read(paths.configPath)).toBe(before); + expect(existsSync(join(paths.root, "opencodex-prompt.journal"))).toBe(false); + expect(existsSync(join(paths.root, "opencodex-prompt.lock"))).toBe(false); + + // And the next write is not poisoned by the failed one. + rmSync(paths.storePath, { recursive: true, force: true }); + expect(writeCustomLayers([layer()], readPromptLayers(paths).revision, paths).ok).toBe(true); + }); }); diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index b223754612..d1faf27699 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -438,9 +438,15 @@ describe("dispatch and safety", () => { // typecheck property of Record. This asserts the values. const { WRITE_ERROR_STATUS_FOR_TESTS } = await import("../src/server/management/codex-prompt-routes"); const statuses = Object.values(WRITE_ERROR_STATUS_FOR_TESTS); - expect(statuses.length).toBeGreaterThanOrEqual(9); + expect(statuses.length).toBeGreaterThanOrEqual(10); for (const status of statuses) expect(status).toBeGreaterThanOrEqual(400); - for (const status of statuses) expect(status).toBeLessThan(500); + // write_failed is the one 5xx: the filesystem refused a write that passed every + // precondition, so the caller did nothing wrong and retrying it unchanged will + // fail identically. Every OTHER error stays 4xx. + for (const [error, status] of Object.entries(WRITE_ERROR_STATUS_FOR_TESTS)) { + if (error === "write_failed") expect(status).toBe(500); + else expect(status).toBeLessThan(500); + } }); test("22. the injected paths are honored on every verb", async () => { @@ -567,6 +573,7 @@ describe("020 coverage completions", () => { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + write_failed: 500, recovery_required: 409, locked: 409, });