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 +``` 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/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/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/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index b78717560e..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, }; @@ -261,6 +265,33 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { 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 ef3ccda453..d1faf27699 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -12,6 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { handleManagementAPI } from "../src/server/management-api"; import { LAYER_INVENTORY, readPromptLayers } from "../src/codex/prompt-layers"; +import type { ManagementPrincipal } from "../src/server/management-auth"; import type { OcxConfig } from "../src/types"; const MARKER = "# Auto-injected by opencodex"; @@ -69,6 +70,7 @@ async function call( pathname: string, fx: Fixture, body?: unknown, + principal: ManagementPrincipal | undefined = "gui-session", ): Promise<{ status: number; body: any; routed: boolean }> { 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; @@ -436,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 () => { @@ -565,6 +573,7 @@ describe("020 coverage completions", () => { store_unreadable: 409, invalid_characters: 400, write_superseded: 409, + write_failed: 500, recovery_required: 409, locked: 409, }); @@ -583,7 +592,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 +603,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 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", 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"); 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 = {