diff --git a/devlog/_plan/260802_client_toggle_api/000_plan.md b/devlog/_plan/260802_client_toggle_api/000_plan.md new file mode 100644 index 0000000000..104fc03dcf --- /dev/null +++ b/devlog/_plan/260802_client_toggle_api/000_plan.md @@ -0,0 +1,134 @@ +# 000 — Plan: client-integration toggle research (API-side on/off switch) + +Research unit. Docs-only: this unit produces survey and design-option documents, +not production code. Implementation, if approved later, gets its own decade docs +in a follow-up cycle. + +Parent unit: `devlog/_plan/260731_client_config_export/` (the read-only export +surface this unit extends). Sibling survey `001_client_config_survey.md` there +already covers Pi/OpenCode injection layers and the no-standard landscape; this +unit does not re-survey them. + +## Loop spec + +- Loop archetype: spec-satisfaction research (verifier defines done). +- Trigger: user request — can client integrations be applied as an API-side + on/off switch (enable writes the provider block into the client config, + disable removes it), for Hermes Agent, OpenClaw, Kimi Code CLI, and + Gajae Code, with cc-switch as the reference implementation. +- Goal: durable devlog evidence answering (a) how cc-switch implements + apply/remove, (b) what each of the four clients requires for a safe external + toggle, (c) what shape an opencodex management-API toggle would take and its + risks. +- Non-goals: no `src/`, `gui/`, or test changes; no new clients serialized in + `config-export.ts`; no implementation plan at diff level (that is the next + cycle's decade docs, only if the design is approved). +- Verifier: every load-bearing claim carries a source URL or repo file path + opened this cycle (cxc-search Tier 2); unverifiable claims are marked + `candidate — unverified` and listed as open questions. `bun run + privacy:scan` stays green. +- Stop condition: all four clients + cc-switch lane questions answered or + explicitly marked unreachable. +- Memory artifact: this unit's 001–003 docs. +- Expected terminal outcomes: DONE (docs written, claims sourced) or BLOCKED + (sources unreachable, named). +- Escalation: a client whose config cannot be toggled safely from outside is a + finding, not a failure — record the blocker and mark that client UNSAFE for + the toggle design. + +## Research questions + +### Q1 — cc-switch reference mechanics (doc 001) + +- How does cc-switch apply a provider into each app's config (writer modules, + atomic write, backup/restore), and how does it remove or restore one? +- Does it offer any local HTTP API / relay ("universal endpoint") as an + alternative to file writes? +- What state does it own (`~/.cc-switch/config.json`) versus mutate in the + client's files? + +### Q2 — Per-client toggle requirements (doc 002) + +For each of Hermes Agent, OpenClaw, Kimi Code CLI, Gajae Code: + +- Config file, format, and the minimal provider block that points at + `http://127.0.0.1:/v1`. +- Hot-reload semantics: does the client re-read the file, or only at + session/process start? What does "applies on toggle" mean in practice? +- Non-interactive management surface, if any (CLI subcommands, local API), + which would beat raw file writes. +- Credential handling: env-reference support versus literal-only (decides + whether the no-secret-serialization invariant survives a write path). +- Removal semantics: what must be cleaned up beyond the provider block + (default model pointers, sessions, auth storage). + +### Q3 — opencodex API toggle design options (doc 003) + +- Current surface: `GET /api/client-config` is read-only by design; mutating + precedents exist (`PUT /api/disabled-models`, `PUT /api/model-visibility`). +- Trust boundary (A-gate amendment, blocker 2): the toggle endpoint lives on + the **management plane** — every `/api/*` request passes + `requireManagementAuth`, loopback GUI sessions are origin-bound, and + mutations require CSRF (`src/server/management-auth.ts`, + `src/server/index.ts:448`). This is separate from **data-plane** admission + (`resolveApiAuth` in `src/server/auth-cors.ts`), whose loopback shortcut + only decides what a *client* must send to `/v1`. Doc 003 must not conflate + the two. +- Launcher precedence (A-gate amendment, blocker 3): `ocx opencode` injects + `provider.opencodex` through `OPENCODE_CONFIG_CONTENT`, which outranks the + disk config for that process (`src/cli/opencode.ts`). Read-back must + distinguish "disk state" from "runtime state when launched via `ocx`", and + the design must say whether the disk toggle even applies to OpenCode. +- Design space: `PUT /api/client-config/:client {enabled: bool}` or a + `/api/client-integrations` resource; read-back/health (is our block present + and current?) versus fire-and-forget writes. +- State model (A-gate amendment, blocker 4): no cc-switch-style DB exists, so + "is it on?" must be read back from the client file. A boolean `enabled` + hides drift; the design needs richer states — `absent`, `current`, `stale` + (our block but not what we would generate now), `conflict` (a block with our + provider id that we did not write), `unsafe` (unparseable file / damaged + ownership markers). +- Invariants to carry over: no secret serialized, additive merge only, + preserve unknown fields, atomic write + backup, never touch blocks we did + not write. + +### Q4 — Local writer/read-back precedents (doc 003, A-gate amendment) + +- `src/grok/inject.ts` — the repo's one existing third-party config writer: + BEGIN/END managed fence in `~/.grok/config.toml`, orphan-marker refusal, + non-loopback refusal with credential-fallthrough reasoning, placeholder + `api_key = "opencodex-loopback"` (never a real secret), `stripGrokConfig` + removal path. +- `src/grok/status.ts` — the read-only status reader paired with the writer; + parses only our own fenced region. +- `src/config.ts` `atomicWriteFile`/`renameAtomicFile` — temp+rename, Windows + EBUSY/EPERM/EACCES retry with backoff, 0o600 mode, ACL hardening, residual + temp scrubbing. +- Claude Desktop writer/reader — second precedent to survey in 003. +- Risk register: format fidelity (YAML/JSON5/TOML round-trip), concurrent + writes while the client runs, clients that rewrite their own config + (Kimi Code), drift between proxy catalog and written model list. + +## Doc map (000-range, research only) + +- `000_plan.md` — this document. +- `001_ccswitch_toggle_analysis.md` — Q1 findings. +- `002_client_toggle_matrix.md` — Q2 findings, one section per client. +- `003_api_design_options.md` — Q3 design space + risk register + open + questions. Options, not a commitment. +- `004_ux_design.md` — (cycle 2) GUI design for the unified integrations + surface: tab rename, hero with install detection + switches, per-client + sub-pages, and the rollback UX that makes the toggle trustworthy. Design + spec only; component diffs belong to a later implementation cycle. + +## Dispatch plan + +- 5 research lanes (subagents, read-only): cc-switch, Hermes, OpenClaw, + Kimi Code, Gajae Code. Lane output is candidate evidence; load-bearing + claims are re-opened by the main agent before promotion (cxc-search proof + handoff). +- Local (main agent): current management-API surface, GUI consumer, loopback + admission semantics — already read this cycle: + `src/clients/config-export.ts`, `src/server/management/model-routes.ts`, + `src/server/auth-cors.ts` (`resolveApiAuth`, loopback admission), + `gui/src/components/apikeys-workspace/client-config-clients.ts`. diff --git a/devlog/_plan/260802_client_toggle_api/001_ccswitch_toggle_analysis.md b/devlog/_plan/260802_client_toggle_api/001_ccswitch_toggle_analysis.md new file mode 100644 index 0000000000..df789cbad5 --- /dev/null +++ b/devlog/_plan/260802_client_toggle_api/001_ccswitch_toggle_analysis.md @@ -0,0 +1,101 @@ +# 001 — cc-switch toggle mechanics (reference implementation) + +Research only. No diffs here. Sources were opened against upstream `main` on +2026-08-02 via the GitHub API (research lane Zeno, main-agent spot-check of the +two load-bearing claims). + +cc-switch v3.17.0 (Tauri 2, Rust + React) manages Claude Code, Claude Desktop, +Codex, Gemini CLI, Grok Build, OpenCode, OpenClaw, and Hermes Agent. It is the +closest shipping implementation of the "on/off switch per client app" this unit +evaluates. Its own provider store is `~/.cc-switch/cc-switch.db` (SQLite), with +`settings.json` for device preferences and `backups/` for database backups +([README.md#L302-L305](https://github.com/farion1231/cc-switch/blob/main/README.md#L302-L305), +[database/mod.rs#L96-L109](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/database/mod.rs#L96-L109)). + +## 1. Two app classes: exclusive vs additive + +The toggle semantics split on whether the client keeps one active provider or +many coexisting ones: + +| Class | Clients | "Switch on" means | "Switch off" means | +|-------|---------|-------------------|--------------------| +| Exclusive | Claude Code, Codex, Gemini | backfill current live config into the stored provider record, then overwrite live config with the target provider | switching away backfills and overwrites with the next provider | +| Additive | OpenCode, OpenClaw, Hermes | insert the provider entry into the client's live config, alongside existing ones | `remove_provider_from_live_config` deletes only that live entry; the provider stays in cc-switch's DB, marked `live_config_managed=false` | + +Source: [provider/mod.rs#L2893-L2951](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/provider/mod.rs#L2893-L2951), +[provider/mod.rs#L2954-L2966](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/provider/mod.rs#L2954-L2966), +[provider/mod.rs#L3087-L3146](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/provider/mod.rs#L3087-L3146). +Main-agent spot-check: `live_config_managed` markers and +`provider_live_config_managed` confirmed in `provider/mod.rs` (lines 1870, +2437-2450). + +All four of this unit's target clients (Hermes, OpenClaw, and by their config +shapes Kimi Code and Gajae Code) are **additive-class**: a toggle writes and +removes one provider entry, never a whole-file takeover. + +## 2. Write discipline + +- Provider-specific writers project into native files: Claude -> + `~/.claude/settings.json`, Codex -> `~/.codex/auth.json` + `config.toml`, + OpenClaw -> `~/.openclaw/openclaw.json` (JSON5), Hermes -> + `~/.hermes/config.yaml` (YAML). +- JSON/TOML/text writes go through a temporary sibling file + rename (atomic on + Unix; Windows removes the destination first). + [provider/live.rs#L1015-L1060](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/provider/live.rs#L1015-L1060), + [config.rs#L273-L351](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/config.rs#L273-L351) +- Writer modules preserve unknown fields (`serde(flatten)` extra maps in + `hermes_config.rs` / `openclaw_config.rs`) and honor the client's own + config-dir overrides (`HERMES_HOME`, Windows `%LOCALAPPDATA%\hermes`). +- Health warnings are returned on write (`OpenClawWriteOutcome.warnings`). + +## 3. Proxy takeover: the *other* kind of toggle + +cc-switch has a second, coarser switch — "proxy takeover" — which is +backup-and-restore, not provider-entry deletion: + +1. Enable: start the local proxy if needed, snapshot the app's current live + config into the DB's live-backup storage, then write proxy + endpoint/placeholder fields into the client config. +2. Disable/stop-with-restore: write the saved snapshot back and delete the + backup rows. Missing backup -> attempt SSOT/provider reconstruction and + placeholder cleanup. +3. Switching providers *while* takeover is active is a hot switch: the client + keeps pointing at the local proxy and only the proxy's upstream target + changes. + +[services/proxy.rs#L730-L820](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/proxy.rs#L730-L820), +[services/proxy.rs#L1292-L1323](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/proxy.rs#L1292-L1323), +[services/proxy.rs#L1822-L1865](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/proxy.rs#L1822-L1865), +[provider/mod.rs#L3004-L3054](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/services/provider/mod.rs#L3004-L3054) + +For opencodex this pattern is mostly redundant — opencodex *is* the proxy, so +the client config only ever needs the additive provider entry; there is no +upstream target to hot-switch inside the client. + +## 4. Universal endpoint (relay), not a management API + +cc-switch embeds an Axum server (default `http://127.0.0.1:15721`) exposing +Claude / OpenAI Responses / Gemini routes plus health and status; tools point +their base URL at it. It is a data-plane relay, **not** a control API — the +toggle operations live in Tauri commands, and the only URL scheme is +`ccswitch://v1/import?...` for config import. +[proxy/server.rs#L100-L145](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/proxy/server.rs#L100-L145), +[proxy/server.rs#L291-L366](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/proxy/server.rs#L291-L366), +[deeplink/parser.rs#L11-L67](https://github.com/farion1231/cc-switch/blob/main/src-tauri/src/deeplink/parser.rs#L11-L67) + +Main-agent spot-check: `proxy/server.rs` binds +`config.listen_address:listen_port` via tokio TcpListener (lines 101-145). + +## 5. What this means for an opencodex toggle + +- The additive per-client writer is the pattern to copy: insert/remove exactly + one provider entry, preserve everything else, atomic write, health warnings + on read-back. +- cc-switch's "is it on?" state is *its own DB record* plus a + `live_config_managed` marker — it does not re-derive state from the client + file. An opencodex toggle has no such DB; state must be read back from the + client config itself (does our entry exist, and does it match what we would + generate now?). This is a real design divergence, explored in 003. +- Its exclusive-app machinery (backfill/overwrite) and proxy-takeover + snapshot/restore solve problems opencodex does not have; neither needs + porting. diff --git a/devlog/_plan/260802_client_toggle_api/002_client_toggle_matrix.md b/devlog/_plan/260802_client_toggle_api/002_client_toggle_matrix.md new file mode 100644 index 0000000000..09b12e2933 --- /dev/null +++ b/devlog/_plan/260802_client_toggle_api/002_client_toggle_matrix.md @@ -0,0 +1,186 @@ +# 002 — Per-client toggle requirements + +Research only. No diffs here. Each section answers the same five questions from +000 Q2: where the config lives, the minimal provider block, hot-reload +semantics, non-interactive management surface, credential handling, and removal +cleanup. Lane findings were source-opened by the dispatched subagents; the main +agent re-opened the Hermes provider docs and the cc-switch writer modules in the +previous cycle (see 001). Snapshot date: 2026-08-02. + +## Summary matrix + +| | Hermes Agent | OpenClaw | Kimi Code CLI | Gajae Code | +|---|---|---|---|---| +| Config file | `~/.hermes/config.yaml` | `~/.openclaw/openclaw.json` | `~/.kimi-code/config.toml` | `~/.gjc/agent/models.yml` | +| Format | YAML | JSON5 | TOML | YAML | +| Dir override | `HERMES_HOME`; Windows `%LOCALAPPDATA%\hermes` | settings override only | `KIMI_CODE_HOME` | — | +| Hot reload | No (mtime-cached; new session) | **Yes** (gateway `hybrid` reload) | No in v1 (`/reload`); v2 watches | No (mtime refresh on `/model`/session start) | +| Env-ref for key | **Yes** (`${VAR}`, `${env:VAR}`) | **Yes** (`${VAR}`, SecretRef) | **No** (literal only) | **Yes** (`apiKeyEnv`, fail-closed) | +| Non-interactive add | No (wizard/dashboard only) | `openclaw config set/patch` | `kimi provider add ` (registry import only) | `gjc setup provider` | +| Non-interactive remove | No | `openclaw config unset` | `kimi provider remove` (cascades) | **No** (file edit required) | +| Toggle channel of choice | file writer (or dashboard API) | **client CLI** | **registry endpoint + client CLI** | CLI add + file-writer remove | + +## Hermes Agent + +Upstream: `NousResearch/hermes-agent` `main`, 2026-08-02 (lane Dewey; main-agent +read of `website/docs/user-guide/configuring-models.md` in the earlier cycle). + +- **Provider block** (modern dict form; legacy `custom_providers` list + auto-migrates at config v12): + `providers: : { api: , api_key, extra_headers, + discover_models, models: [...] }`, with the active selection in + `model: { provider, default, base_url, api_mode }`. `api_mode: + chat_completions` targets the proxy's `/v1/chat/completions`. +- **Reload**: `hermes_cli/config.py` caches `config.yaml` by mtime/size and + reloads only when `load_config()` runs. CLI chat reads at session start; the + gateway has partial live reads. Treat provider changes as **new-session / + restart required**. +- **Non-interactive surface**: `hermes setup --non-interactive` is a wizard, + not provider CRUD. The dashboard is a local authenticated HTTP surface + (`/api/config`, `/api/env`, `/api/model/set`, raw YAML save via + `saveConfigRaw()` in `web/src/lib/api.ts` / `hermes_cli/web_server.py`) — + usable for automation behind its session-token auth. A dedicated provider + add/remove REST endpoint is **candidate — unverified** (none found). +- **Credentials**: `${VAR_NAME}` / `${env:VAR_NAME}` substitution is supported + in provider config (`key_env`/`api_key_env` aliases also recognized), so the + no-secret-serialization invariant survives. Undefined references remain + verbatim with a warning. +- **Removal cleanup**: `~/.hermes/.env`, `auth.json`, credential-pool state, + and sessions live outside `config.yaml`. Deleting our `providers:` entry + leaves those intact; `hermes auth remove` handles credential-pool entries. + A toggle that never writes credentials (env-ref only) has nothing extra to + clean. + +## OpenClaw + +Upstream: `openclaw/openclaw` `main` + docs.openclaw.ai, 2026-08-02 (lane +Hegel). + +- **Provider block**: `models.providers.: { baseUrl, apiKey | SecretRef, + auth, api, models: [{id, ...}], contextWindow, maxTokens, timeoutSeconds, + params, headers }` ([src/config/types.models.ts#L199-L220](https://github.com/openclaw/openclaw/blob/main/src/config/types.models.ts#L199-L220)). + Allowed `api` values include `openai-completions`, `openai-responses`, + `anthropic-messages` ([types.models.ts#L11-L25](https://github.com/openclaw/openclaw/blob/main/src/config/types.models.ts#L11-L25)). + `models.mode: "merge"` keeps the bundled catalog alongside ours; the default + model is `agents.defaults.model.primary`. +- **Reload**: the gateway **watches `openclaw.json`**; default + `gateway.reload.mode: hybrid` hot-applies model/provider changes + ([config hot-reload](https://docs.openclaw.ai/configuration#config-hot-reload)). + This is the one client where a file toggle takes effect on a running + process — and therefore the one where non-atomic writes are most dangerous. +- **Non-interactive surface**: `openclaw config set` / `config patch` carry + `--merge` for protected maps like `models.providers`; removal is a plain + path unset, `openclaw config unset models.providers.opencodex` (`--merge` is + not a documented unset flag), plus `openclaw models set ` + for the primary + ([config CLI](https://docs.openclaw.ai/cli/config), + [models CLI](https://docs.openclaw.ai/cli/models)). Full add/remove without + touching the file ourselves. Exact path quoting and `--dry-run` behavior are + implementation-cycle checks. +- **Credentials**: `${UPPERCASE_VAR}` interpolation in any config string, + including `apiKey`; missing/empty variables fail config loading (fail-closed). + SecretRef objects (`{ source: "env", ... }`) also supported + ([env vars](https://docs.openclaw.ai/gateway/configuration#environment-variables)). +- **Removal cleanup**: plain `config unset models.providers.` on our + provider key; if the toggle set `agents.defaults.model.primary`, restore or + clear it. No external credential state for an env-ref-only entry. + +## Kimi Code CLI + +Upstream: `MoonshotAI/kimi-code` `main` at `e22479a` (2026-08-01) + official +docs (lane Mencius; all findings source-verified). + +- **Provider block** (`config.toml`): + `[providers.opencodex] type = "openai"`, `base_url`, `api_key`; models as + `[models.]` with **mandatory** `provider`, `model`, positive + `max_context_size`, plus additive `capabilities`. +- **Capability inference is wire-scoped**: for `type = "openai"` only + OpenAI-style prefixes (`oN`, `gpt-*`) are recognized, so a routed id like + `anthropic/claude-opus-4-8` resolves to *unknown* capabilities and must be + declared explicitly ([capability-registry.ts#L24-L44](https://github.com/MoonshotAI/kimi-code/blob/e22479a62eed9c3b78a67b313f4332c2c0ba9670/packages/kosong/src/providers/capability-registry.ts#L24-L44)). + This collides with our "never guess metadata" invariant: the toggle should + either emit only capabilities the catalog actually asserts, or omit the + field and let the user declare it. +- **Reload**: v1 (default CLI) does **not** watch the file — `/reload` or + restart. Experimental v2 (`KIMI_CODE_EXPERIMENTAL_FLAG=1`, `kimi web`) + installs a watcher. +- **Non-interactive surface**: `kimi provider add + --api-key ` imports a **custom `api.json` registry** — the CLI then + creates the provider/model entries itself and refreshes them from the same + URL on later startups. `kimi provider remove ` cascades: provider + + referencing model aliases + `default_model`/`default_provider` + ([core-impl.ts#L734-L762](https://github.com/MoonshotAI/kimi-code/blob/e22479a62eed9c3b78a67b313f4332c2c0ba9670/packages/agent-core/src/rpc/core-impl.ts#L734-L762)). + There is **no** generic `kimi provider add --type openai ...`; a local proxy + must be hand-written to TOML *or* served as a registry. **Design + consequence: opencodex could serve an `api.json` registry endpoint, making + the toggle `kimi provider add http://127.0.0.1:10100/...` / `kimi provider + remove opencodex` — vendor-owned writes, atomic by construction, with + cascade cleanup and catalog refresh for free.** The registry schema itself + is an open question for the implementation cycle. +- **Write semantics**: provider mutations rewrite the *entire* TOML via + `smol-toml` (unknown fields preserved as values; **comments and formatting + lost**) ([toml.ts#L466-L517](https://github.com/MoonshotAI/kimi-code/blob/e22479a62eed9c3b78a67b313f4332c2c0ba9670/packages/agent-core/src/config/toml.ts#L466-L517)). + An external writer can do no worse than the vendor's own CLI — but using the + CLI/registry path avoids the fight entirely. +- **Credentials**: literal-only (no shell-env fallback). Loopback binds need + no real key, so a placeholder satisfies the no-secret invariant; a + non-loopback toggle would have to serialize a real key — scope the toggle + to loopback or require manual key entry. +- **Removal cleanup**: CLI removal cascades config references; sessions and + OAuth files are separate and untouched. An env-less custom provider has no + extra auth state. + +## Gajae Code + +Upstream: published `gajae-code@0.12.7` / `@gajae-code/coding-agent@0.12.7` +tarballs, matching commit `5f2e7cd` (lane Kant; all findings source-verified). + +- **Provider block** (`~/.gjc/agent/models.yml`): the Pi `models.json` shape + in YAML — `providers.: { baseUrl, apiKey | apiKeyEnv, api, + models: [{id, name?, input, contextWindow?, maxTokens?}] }`. Allowed `api` + values include `openai-completions` and `openai-responses` + ([models-config-schema.ts#L113-L155](https://github.com/Yeachan-Heo/gajae-code/blob/5f2e7cd05e8ea344991566f9ed96f1f9c66226bd/packages/coding-agent/src/config/models-config-schema.ts#L113-L155)). + **Validation is strict: unknown fields fail.** Our writer must emit only + schema-known fields — the opposite of the preserve-everything default. +- **Reload**: `ModelRegistry` loads at construction; `refresh()` / + `refreshInBackground()` compare mtime. No `fs.watch`. Opening `/model` + triggers an offline refresh; session start schedules a background one. + Toggle applies on new session or explicit refresh. +- **Credentials**: `apiKey` is *env-name-or-literal* (env lookup first, then + the literal text becomes the token — a footgun); **`apiKeyEnv` is + env-name-only and fail-closed — the field a toggle must use.** + Credential order: runtime `--api-key` > `models.yml` key > stored `agent.db` + credential > OAuth > standard env mapping > registry fallback + ([auth-storage.ts#L3544-L3600](https://github.com/Yeachan-Heo/gajae-code/blob/5f2e7cd05e8ea344991566f9ed96f1f9c66226bd/packages/ai/src/auth-storage.ts#L3544-L3600)). +- **Non-interactive surface**: `gjc setup provider --compat openai|anthropic + --provider ID --base-url URL --api-key-env ENV --model ID... + [--force] [--json]` — add/replace only; raw `--api-key` is rejected by + design ([setup-cli.ts#L332-L382](https://github.com/Yeachan-Heo/gajae-code/blob/5f2e7cd05e8ea344991566f9ed96f1f9c66226bd/packages/coding-agent/src/cli/setup-cli.ts#L332-L382)). + Two gaps: custom `--compat openai` writes `api: openai-responses` with no + `--api` selector (a chat-completions-only provider still needs direct YAML), + and **no remove command exists** — removal is an atomic `models.yml` edit. +- **Removal cleanup**: `agent.db` credentials only exist if the user went + through `/login`/broker (an env-backed toggle never creates them); + references in `config.yml` (`modelRoles`, `enabledModels`, + `disabledProviders`, `modelProviderOrder`, `modelProfile.default`) must be + reverted if the toggle set them; live sessions keep their selected model + until switched/restarted. The `models.db` discovery-cache row is inert for a + removed provider. + +## Cross-client observations + +1. **Three of four clients offer a vendor-owned write path** (OpenClaw CLI, + Kimi registry+CLI, Gajae add-CLI). Only Hermes forces a raw file write (or + its dashboard API). Delegating writes to the vendor's own tool inherits + their atomicity and schema knowledge; the cost is a version/availability + dependency on their CLI. +2. **Only OpenClaw hot-reloads.** Every other client applies the toggle on + the next session (Hermes, Gajae) or explicit `/reload` (Kimi v1). The GUI + copy must say "applies to new sessions" everywhere except OpenClaw. +3. **The no-secret invariant survives everywhere except non-loopback Kimi.** + Hermes `${VAR}`, OpenClaw `${VAR}`/SecretRef, and Gajae `apiKeyEnv` all + carry env references; loopback needs no real key anywhere + (`resolveApiAuth` skips admission on loopback binds). +4. **Removal asymmetry is the norm**: add is easy everywhere; clean remove + needs a file writer for Hermes and Gajae, while OpenClaw and Kimi cascade + through their CLIs. diff --git a/devlog/_plan/260802_client_toggle_api/003_api_design_options.md b/devlog/_plan/260802_client_toggle_api/003_api_design_options.md new file mode 100644 index 0000000000..f08ccbc4ec --- /dev/null +++ b/devlog/_plan/260802_client_toggle_api/003_api_design_options.md @@ -0,0 +1,224 @@ +# 003 — API-side toggle: design options, trust boundary, risk register + +Research only. This document maps the design space; it does not commit to an +implementation. If an option is approved, the next cycle writes decade docs +(010+) at diff level. + +## 1. What exists today + +- `GET /api/client-config?client=opencode|pi` + ([model-routes.ts:236](/Users/jun/Developer/new/700_projects/opencodex/src/server/management/model-routes.ts:236)) + builds the client document from the same core the CLI uses and **never + writes** — "targeting it is the caller's explicit act" + ([config-export.ts:19](/Users/jun/Developer/new/700_projects/opencodex/src/clients/config-export.ts:19)). +- Mutating precedents on the same router (`PUT /api/disabled-models`, + `PUT /api/model-visibility`) persist only opencodex's *own* config, never + third-party files. +- The GUI renders per-client rows from its own hand-synced list + ([client-config-clients.ts:5](/Users/jun/Developer/new/700_projects/opencodex/gui/src/components/apikeys-workspace/client-config-clients.ts:5)) — + copy/download only, no apply button. + +A management-API toggle for the four new clients is therefore **not** +unprecedented in kind: two routes already mutate third-party files, and they +are the primary endpoint-level precedents for a toggle: + +- `POST /api/grok/apply` + ([agent-settings-routes.ts:593](/Users/jun/Developer/new/700_projects/opencodex/src/server/management/agent-settings-routes.ts:593)) — + accepts **no body** (every input comes from persisted state, guards stay in + `injectGrokConfig` and are not duplicated at the route); single-flight with + `GrokApplyBusyError` -> 409; a policy skip (non-loopback, no `~/.grok`) is + reported as a result (`skippedReason`), not a 500. +- `POST /api/claude-desktop/apply` + ([agent-settings-routes.ts:651](/Users/jun/Developer/new/700_projects/opencodex/src/server/management/agent-settings-routes.ts:651)) — + validates its optional body (`mode` enum, profile override parse), persists + `appliedFingerprint` + `appliedAt` into opencodex config after a successful + write, and reports partial failure as `{ saved: true, path }` + 500 when the + third-party write fails after the profile was saved. +- `GET /api/claude-desktop/status` + ([agent-settings-routes.ts:706](/Users/jun/Developer/new/700_projects/opencodex/src/server/management/agent-settings-routes.ts:706)) — + recomputes the on-disk fingerprint (sha256, truncated) and returns + `stale = savedFingerprint !== onDiskFingerprint`, plus a tri-state + `activeProfile` (`null` = undeterminable; a readable `appliedId` with no + opencodex entry is a *known* false). + +## 2. Trust boundary (do not conflate the two planes) + +| Plane | Guard | Relevance | +|-------|-------|-----------| +| Management (`/api/*`) — where the toggle endpoint lives | `requireManagementAuth`, origin-bound loopback sessions, CSRF on mutations ([management-auth.ts:207](/Users/jun/Developer/new/700_projects/opencodex/src/server/management-auth.ts:207), [index.ts:448](/Users/jun/Developer/new/700_projects/opencodex/src/server/index.ts:448)) | The toggle inherits this; no new auth design needed, but a toggle **write** must require the same CSRF path as `PUT /api/disabled-models`. | +| Data (`/v1/*`) — what the client calls after toggling | `resolveApiAuth`; loopback binds skip admission entirely ([auth-cors.ts:327](/Users/jun/Developer/new/700_projects/opencodex/src/server/auth-cors.ts:327)) | Decides what credential the *written config* must carry: placeholder on loopback, real key reference otherwise. | + +## 3. State model: read-back, not a database + +cc-switch keeps its own SQLite and a `live_config_managed` marker (001 §1, §5). +opencodex has no such DB and adding one for this is questionable scope. The +honest alternative: **derive state from the client file itself**, which forces +richer states than a boolean: + +| State | Meaning | Toggle action offered | +|-------|---------|----------------------| +| `absent` | no opencodex entry in the client config | enable | +| `current` | our entry exists and matches what we would generate now | disable | +| `stale` | our entry is untouched (hash match) but no longer equals what we would generate now (catalog/port drift) | refresh (rewrite), disable | +| `conflict` | the file changed after we wrote it (hash mismatch), or an entry with our provider id exists with no ownership proof | refuse; show diff, require explicit takeover | +| `unsafe` | file unparseable, ownership markers damaged, or path not a regular file | refuse; surface the reason | + +"Did we write it?" needs an ownership proof. Local precedents offer three +working patterns: + +- **Managed fence** — `src/grok/inject.ts` writes a BEGIN/END marker region in + `~/.grok/config.toml`, refuses or strips on an orphaned marker + (`skippedReason: "orphaned-marker"`), and `src/grok/status.ts` is the paired + read-only reader that parses only our own fence. Works for formats with + comments (TOML/YAML/JSON5); not for strict JSON. +- **Provenance sidecar** — `src/claude/desktop-3p.ts` writes a metadata + sidecar (`appliedId`, `entries`) next to the mutated file, refreshes a + single `.bak` on **every** replacement (overwrite, not one-time — + [desktop-3p.ts:371](/Users/jun/Developer/new/700_projects/opencodex/src/claude/desktop-3p.ts:371)), + and rolls back if the metadata write fails + ([desktop-3p.ts:356-380](/Users/jun/Developer/new/700_projects/opencodex/src/claude/desktop-3p.ts:356)). + Works for any format, including strict JSON and strict-schema YAML (Gajae). +- **Persisted content fingerprint** — the Claude Desktop route pair stores + the last-written content hash (`appliedFingerprint`) in opencodex's own + config and the status route compares it against the on-disk hash to compute + `stale` + ([agent-settings-routes.ts:706](/Users/jun/Developer/new/700_projects/opencodex/src/server/management/agent-settings-routes.ts:706)). + This is the piece that actually distinguishes "we wrote it and it is + unchanged" from "someone edited it after us" — a sidecar alone only proves + what we *intended* to write. + +The ownership record for a toggle is therefore: **managed provider identity + +canonical last-written content hash (persisted opencodex-side) + the fence or +sidecar that scopes what we own in the file.** The classification is a +two-axis derivation from it — the persisted hash proves *the file was not +touched after us*, and a fresh regeneration proves *our content is still what +we would write today*: + +1. on-disk hash != last-written hash -> `conflict` (someone modified the file + after us; disable must refuse, per caveat 2 below). +2. hashes match AND managed content equals a fresh regeneration -> `current`. +3. hashes match BUT managed content differs from a fresh regeneration + (catalog/port drift) -> `stale` (offer refresh/disable). +4. our provider id present with no ownership record at all -> `conflict`. +5. markers damaged or file unparseable -> `unsafe`. + +Disable may proceed only from `current`/`stale` — i.e. only while the on-disk +hash still equals the last-written hash — followed by the pre-commit recheck +of caveat 1. This deliberately sharpens the Claude Desktop status route's +looser naming, where `stale = savedFingerprint !== onDiskFingerprint` +conflates foreign edits with drift; a toggle that can *delete* needs the two +split apart. + +All of this rides on `atomicWriteFile` ([config.ts:54-167](/Users/jun/Developer/new/700_projects/opencodex/src/config.ts:54)): +temp+rename, Windows EBUSY/EPERM/EACCES retry with backoff, 0o600 mode, ACL +hardening, residual-temp scrubbing. Two caveats a toggle must add on top: + +1. **Atomic rename is not compare-and-swap.** An additive read-modify-write + can still lose a concurrent user/client edit that lands between our read + and our rename. The toggle must re-read the file and verify the pre-write + fingerprint immediately before committing; a mismatch aborts with the + `conflict` state rather than overwriting. The `/api/grok/apply` + single-flight pattern (busy -> 409) covers concurrency *within* the + serving process; the pre-commit fingerprint check covers everything else + best-effort. Residual cross-process races inside the re-read/rename window + are accepted and documented, not claimed away. +2. **Disable after user modification.** If the persisted hash no longer + matches on-disk content, the entry has been touched by someone else; + disable must not silently delete it. That is the `conflict` path: show the + drift, require explicit takeover/removal. + +## 4. The `ocx opencode` precedence trap + +`ocx opencode` injects `provider.opencodex` through `OPENCODE_CONFIG_CONTENT`, +which **outranks the disk config** for that process +([opencode.ts:8](/Users/jun/Developer/new/700_projects/opencodex/src/cli/opencode.ts:8)). +Consequences: + +- For OpenCode, "enabled on disk" and "enabled at runtime" are different + truths. Read-back must say which one it reports. +- A defensible scope cut: the disk toggle targets the four *new* clients and + Pi (file-only consumers); OpenCode's disk toggle is either documented as + "direct launches only" or excluded in v1. The launcher remains the + recommended OpenCode path. + +## 5. Design options + +### Option A — opencodex-owned file writer (cc-switch additive pattern) + +`PUT /api/client-integrations/:client { enabled: boolean }` (plus a GET for +the five-state read-back). Enable: parse the client file, additive-merge our +provider entry (ownership sidecar or fence), atomic write with the §3 +ownership/backup contract (persisted content fingerprint, compare-before-commit, +`.bak` refreshed on every replacement). Disable: remove exactly our entry, +restore nothing else — and only while the fingerprint still matches (§3). + +- Pros: works for all four clients; no dependency on their CLIs; one code + path per format (YAML ×2, JSON5, TOML). +- Cons: opencodex assumes format-fidelity risk (comments, key order); + Gajae's strict schema rejects unknown fields (002 §Gajae); Kimi's own + tooling rewrites the whole document, so coexisting with `kimi provider` + edits is fine byte-wise but our writer must round-trip unknown sections as + values; needs new serializer dependencies (YAML, JSON5, TOML) in a repo + whose export core is currently JSON-only. + +### Option B — vendor-CLI-driven toggle + +Enable/disable shells out to the client's own non-interactive surface +(002 matrix): `openclaw config set --merge` / plain `config unset`; `kimi provider add +` / `remove` — with opencodex **serving an `api.json` registry +endpoint** so add/remove is fully vendor-owned; `gjc setup provider ...` for +add. Hermes has no CRUD surface (its dashboard API is auth-gated and has no +verified provider endpoint), so Hermes drops to Option A regardless. + +- Pros: vendor owns schema knowledge, atomicity, comment loss (Kimi already + rewrites the whole file itself), and cascade cleanup (Kimi removal deletes + referencing aliases + `default_model`; OpenClaw's plain path unset removes + exactly our provider key). + The Kimi registry variant has a second payoff: model-catalog refreshes + propagate on the client's next startup instead of going `stale`. +- Cons: requires the client binary on PATH at toggle time; version drift + changes flags; `gjc setup provider` cannot select `openai-completions` and + has no remove, so Gajae still needs a file writer for half the operation; + exit-code/stdout contracts become test fixtures we do not control. + +### Option C — hybrid (recommended shape for the next cycle) + +Per client, prefer the vendor CLI when it covers the operation; fall back to +the Option-A writer where it does not: + +| Client | Enable | Disable | +|--------|--------|---------| +| OpenClaw | `openclaw config set --merge` (`--merge` is documented for protected maps on set/patch) | `openclaw config unset models.providers.opencodex` (plain path unset — `--merge` is **not** a documented unset flag; [config CLI](https://docs.openclaw.ai/cli/config)) | +| Kimi Code | `kimi provider add ` | `kimi provider remove opencodex` | +| Gajae Code | `gjc setup provider --api-key-env ...` (needs `openai-responses` acceptance) or file writer | file writer (no CLI remove exists) | +| Hermes | file writer (YAML, `${VAR}` key ref) | file writer | + +Read-back (the five states of §3) is always opencodex-owned file parsing — +vendor CLIs report their own state, not provenance. + +## 6. Risk register + +| Risk | Severity | Mitigation | +|------|----------|-----------| +| Format fidelity: comment/order loss on YAML/JSON5/TOML rewrite | Medium | prefer vendor CLI (Option C); Kimi already loses comments with its own tooling, so matching that bar is defensible; fence/sidecar keeps our edits narrow | +| Gajae strict schema rejects unknown fields | High if naive | emit only schema-known fields (002 §Gajae); test against the published tarball schema | +| Concurrent write while client runs | Medium | `atomicWriteFile` prevents torn bytes only; lost updates need the §3 compare-before-commit fingerprint check + single-flight 409, with the residual cross-process race accepted and documented; OpenClaw hot-reload makes a torn write *visible* to a running gateway | +| Drift: written model list vs live catalog | Medium | five-state read-back (`stale`) + refresh action; Kimi registry path self-refreshes | +| Non-loopback credential serialization (Kimi literal-only) | High | scope the toggle to loopback (placeholder key, Grok precedent `api_key = "opencodex-loopback"` in [grok/inject.ts](/Users/jun/Developer/new/700_projects/opencodex/src/grok/inject.ts)); non-loopback requires manual key entry — document, do not automate | +| Hermes removal residue (`.env`, `auth.json`, credential pool) | Low | env-ref-only toggle never writes those; document that we do not touch them | +| `conflict` state destructive-disable | High | never remove a block without ownership proof; require explicit takeover UX | +| Windows path/ACL matrix (`%LOCALAPPDATA%\hermes`, XDG on Windows for others) | Medium | reuse `atomicWriteFile` hardening; per-client path table in the implementation cycle | + +## 7. Open questions for the implementation cycle + +1. Kimi `api.json` registry schema — exact document shape the CLI imports and + re-fetches (source read of `provider.ts` registry-import path). +2. Hermes dashboard API auth model — could an agent-driven caller ever use it, + or is file writing the only honest path? +3. Gajae `api: openai-responses` acceptance — does our `/v1/responses` surface + satisfy `gjc setup provider --compat openai`, making the add half + CLI-driven too? +4. Whether read-back belongs in `GET /api/client-config` (per-client `state` + field) or a new `GET /api/client-integrations`. +5. Per-client Windows/macOS/Linux path matrix (follows cc-switch's + `get_hermes_dir` resolution order for Hermes). diff --git a/devlog/_plan/260802_client_toggle_api/004_ux_design.md b/devlog/_plan/260802_client_toggle_api/004_ux_design.md new file mode 100644 index 0000000000..8eb7c04f09 --- /dev/null +++ b/devlog/_plan/260802_client_toggle_api/004_ux_design.md @@ -0,0 +1,432 @@ +# 004 — UX design: unified Integrations surface + +Design spec only. Component diffs belong to a later implementation cycle. +Grounds every choice in the existing GUI (read this cycle): sidebar `NAV` in +[App.tsx:50-63](/Users/jun/Developer/new/700_projects/opencodex/gui/src/App.tsx:50), +the sub-tab pattern shared by Logs (`#logs/debug`, hash-routed, lazy-mounted, +poll-gated) and Claude (`code/desktop` segmented strip with arrow-key nav and +`preventScroll` focus), the sidebar inline `Switch` precedent on the Claude +nav entry, `Switch` in [ui.tsx:8](/Users/jun/Developer/new/700_projects/opencodex/gui/src/ui.tsx:8), +`CLIENT_MARKS` (real favicon or monogram tile, never a borrowed logo), and the +CSS token base (`--accent`, `--green/--amber` + `-soft` pairs, `--border`, +`--font-ui/--font-code`, control sizes in `gui/src/styles.css:25`). + +## 1. Design Read + +```yaml +--- +name: opencodex-gui-integrations +colors: + primary: "inherit: --fg/--bg token pair (theme-aware)" + accent: "inherit: --accent + --accent-soft (existing GUI accent)" + background: "inherit: --bg, --glass-panel for the hero strip" +typography: + heading: { fontFamily: "var(--font-ui)", fontSize: "page-head scale (existing)" } + body: { fontFamily: "var(--font-ui)", fontSize: "body scale (existing)" } +iconography: + system: "existing gui/src/icons.tsx set" + weight: "match current stroke weight" + domain: "CLIENT_MARKS precedent — real client favicon or monogram tile" +--- +``` + +Reading this as: a dense operator surface for a developer proxy dashboard, +with a control-room language. One glance must answer "what is connected, what +is applied, and can I undo it" — closer to an audio patchbay than to a +marketing integrations gallery. + +Do's: compose the sub-tab mechanics from their proven parts — Logs hash +ownership + existing wrapping tab CSS + Claude focus behavior (§3.2); make +rollback the most visible safety promise; keep every client honest about its +real state (including "we cannot tell"). +Don'ts: no marketing-hero treatment, no celebration motion on toggles, no +boolean-looking UI over a five-state backend, no per-client visual +re-theming. + +### Dial setting + +``` +DESIGN_VARIANCE: 3 +MOTION_INTENSITY: 2 +Product density profile: D6 (dense admin / repeated ops work) +Reasoning: developer ops dashboard where toggling is repeated work; trust +comes from state honesty and undo, not from visual novelty. Domain gate: +dashboards never receive the expressive kit by default. +``` + +Concept generation (UX-CONCEPT-GEN-01): SKIPPED — utility CRUD/dashboard +surface inside an existing design system; no new brand-visible composition. + +## 2. Tab name: **Integrations** + +Recommendation: rename the `api` sidebar entry to **Integrations** (i18n key +`nav.api` -> `nav.integrations`; ko "연동", en "Integrations", other locales +follow their usual SaaS wording). + +| Candidate | Verdict | Why | +|-----------|---------|-----| +| **Integrations** | Recommended | The surface covers three things — proxy credentials (API keys), downstream client installs, and apply/rollback state. "Integrations" is the only word that covers all three; it is also the word users already know from every SaaS settings page. | +| Clients | Rejected | Accurate for the export clients, but the tab also owns API keys (proxy-side credentials), which are not clients. Also invites confusion with upstream "providers". | +| Connections | Rejected | Reads as network/transport state in a proxy product — collided vocabulary. | +| API (current) | Rejected | Names only the keys half; the switch surface would live under a label that undersells it. | + +Sidebar change: the `api`, `claude`, and `grok` entries collapse into one +`integrations` entry (11 -> 9 nav items). The inline `Switch` on the Claude +nav entry moves into the Claude sub-tab header — the sidebar returns to +navigation-only, which also removes the oddity of a switch that controls +something invisible until you open the page. + +## 3. Information architecture + +Hash-routed sub-tabs, reusing the Logs pattern (`readTabFromHash`, lazy mount, +`active`-gated polls, `role="tablist"` + arrow keys): + +``` +Integrations +├─ #integrations Overview (hero: detection + switches + rollback center) +├─ #integrations/keys API Keys (existing ApiKeysWorkspace panels) +├─ #integrations/codex Codex CLI (informational card — service-coupled, §5.3) +├─ #integrations/claude Claude Code (today's ClaudeCode page) +├─ #integrations/claude/desktop Claude Desktop (today's ClaudeDesktop page) +├─ #integrations/grok Grok Build (existing Grok page moves in) +├─ #integrations/opencode OpenCode +├─ #integrations/pi Pi +├─ #integrations/hermes Hermes Agent +├─ #integrations/openclaw OpenClaw +├─ #integrations/kimi Kimi Code +└─ #integrations/gajae Gajae Code +``` + +Claude Code and Claude Desktop are **separate integration surfaces** — they +have independent backends, and only Claude Code has an enable switch (the +sidebar Switch drives `/api/claude-code`; Desktop works through its own +Save/Apply + fingerprint status) — joined under one family tab with the +existing Code | Desktop segmented strip; the nested hash gives Desktop its +own deep link for the first time. + +### 3.1 Routing contract (A-gate amendment) + +- `integrations` and every suffix above must be registered in + `app-routing.ts` normalization — unregistered suffixes do not survive it, + so this is a hard prerequisite, not a detail. +- Legacy redirects, applied once via `replaceState` (no history spam): + `#api` -> `#integrations/keys`, `#claude` -> `#integrations/claude`, + `#grok` -> `#integrations/grok`. +- An unknown `#integrations/` lands on Overview with a `replaceState` + correction, matching how unknown hashes degrade today. +- Back/Forward: the hash is the source of truth for the active tab (Logs + precedent), so browser navigation walks tab history. + +### 3.2 Tab strip overflow (A-gate amendment) + +Eleven tabs exceed one row on narrow windows. The existing `.page-tabs` +**wraps** to multiple rows and has no horizontal-scrollbar mechanics, and the +Logs keyboard path does not scroll. The design therefore keeps wrapping — +not a scrolling strip: wrapped tabs stay on-screen, so keyboard focus can +never land on an off-screen tab, and no new scroll/focus contract is needed. +Focus behavior on tab change follows the Claude strip (`preventScroll`), not +the Logs strip. Tab order is stable — never reordered by detection state — +because muscle memory beats proximity on a repeated-work surface; undetected +clients keep their tab with a "미설치" dot and the not-installed empty state +(§8), the deliberate alternative to hide-if-absent tabs that would make the +strip jump as clients come and go. + +The per-client export panel currently inside `ClientConfigPanel` leaves the +keys tab and lands inside each **file-toggle** client tab (§5.4; capability +matrix in §5.0 names which clients have an export surface at all) — keys tab +keeps credentials and endpoints only. + +## 4. Overview (the hero) + +Not a marketing hero — an ops summary strip plus a detection grid. Three +zones, top to bottom: + +**4.1 Summary strip.** One line of counts: `7개 감지됨 · 3개 적용 중 · 1개 +업데이트 필요 · 마지막 변경 12분 전`. On the right, one quiet text action +`모두 해제…` — scoped to the file-toggle clients (§5.0; the exception +clients' own controls are untouched) — which opens a confirm dialog listing +exactly which clients will be disabled and what happens to each +(destructive-adjacent, so it keeps its confirmation per the lazy-gate +exemption). There is deliberately no "모두 적용": enabling has per-client +preconditions (install detection, conflict resolution), so a bulk-enable +button would only ever half-work. + +**4.2 Client card grid.** One card per client, in the same stable order as +the tab strip. Cards are **capability-aware** (§5.0): the switch, five-state +badge, and backup line render only on file-toggle client cards — the four +exception clients render their own native status and action instead (Claude +Code: its enable flag switch; Claude Desktop: Save/Apply + fingerprint +status; Grok: select/save/apply state; Codex: informational service link). +A file-toggle card: + +``` +┌─────────────────────────────────────┐ +│ [mark] Hermes Agent [badge] │ +│ 설치 감지됨 · v1.2.3 │ +│ ~/.hermes/config.yaml │ +│ [switch] │ +│ 적용됨 · 14:32 · 백업 보관됨 │ +│ 설정 → │ +└─────────────────────────────────────┘ +``` + +- Brand mark per `CLIENT_MARKS` precedent (real favicon asset or monogram + tile; never another product's logo). +- Detection line: installed/not, version when the client exposes one, config + path in `--font-code`. +- Switch with the five-state badge next to it (§7), on file-toggle cards — + the badge is always present, so the switch never has to carry state it + cannot express. Exception cards show their §5.0-native status in the same + slot, so the grid stays visually aligned without faking a switch. +- Bottom line: last-applied time + backup presence (file-toggle cards; the + exception clients show their native equivalent, e.g. Desktop's + saved-vs-applied fingerprint line). This line is the rollback promise made + visible at all times, not hidden in a dialog. +- `설정 →` deep-links to the client's sub-tab (hash). + +**4.3 Rollback center.** A compact log of recent apply/disable/restore +operations from the **file-toggle clients' operation journal** (§6.1; the +four exception clients keep their state in their own surfaces and do not +write journal rows) — newest first, ~5 rows: `14:32 Hermes 적용 — 되돌리기` / +`14:10 OpenClaw 해제 — 이 시점으로 복원…` / `어제 Kimi 복원 완료`. Row +actions follow §6.1 exactly: `되돌리기` only on each client's newest +operation, `이 시점으로 복원…` on older rows whose snapshot survives, and a +disabled `백업 만료됨` on rows whose snapshot was collected — the row stays +as a record, the action does not pretend to exist. Empty state: `아직 적용 +기록이 없습니다` plus one line explaining that every apply keeps a backup — +the feature's elevator pitch told when there is nothing to show. + +## 5. Client sub-page anatomy + +### 5.0 Capability matrix (A-gate amendment) + +The clients do not share one contract, so the page skeleton below applies +**only where the capability row says it does**. This matrix, not the +skeleton, is the per-client source of truth: + +| Client | Detection | State model | Switch means | Apply mechanism | Restore | Export | +|--------|-----------|-------------|--------------|-----------------|---------|--------| +| Codex CLI | service liveness | service states (running/stopped) | **none — informational card** linking to service controls | service-coupled injection (`ocx start`/`stop`) | service stop restores | n/a | +| Claude Code | `~/.claude` presence | proxy-side enabled flag + auth mode | opencodex-side enable (`/api/claude-code`) | existing PUT; launcher env for the client | n/a (flag flip) | n/a | +| Claude Desktop | config library path | saved/applied fingerprint (today's status route) | apply/stop-serving semantics | `POST /api/claude-desktop/apply` | profile swap-back | n/a | +| Grok Build | `~/.grok` presence | status reader (present/baseUrl/models) + policy skips | **no binary switch** — keeps its select/save/apply flow | `POST /api/grok/apply` | strip path (`ocx stop`) | n/a | +| OpenCode | binary + XDG config dir | five-state (003 §3) | file toggle | file writer (003) — launcher precedence caveat (003 §4) | snapshot restore | existing export | +| Pi | binary + `~/.pi` | five-state | file toggle | file writer | snapshot restore | existing export | +| Hermes | binary + `~/.hermes` | five-state | file toggle | file writer (YAML) | snapshot restore | proposed — new export contract required | +| OpenClaw | binary + `~/.openclaw` | five-state | file toggle | `openclaw config set/unset` CLI preferred | snapshot restore | proposed — new export contract required | +| Kimi Code | binary + `~/.kimi-code` | five-state | registry add/remove | `kimi provider add/remove` + served `api.json` | snapshot restore | proposed — new export contract required | +| Gajae Code | binary + `~/.gjc` | five-state | file toggle | `gjc setup provider` add + file-writer remove | snapshot restore | proposed — new export contract required | + +Consequences: the five-state badge, the switch, and the snapshot-rollback +chrome (§5.1-5.2, §6) render only for the six file-toggle clients (OpenCode, +Pi, Hermes, OpenClaw, Kimi, Gajae). Codex, Claude Code, Claude Desktop, and +Grok keep their own truth — the design unifies their *placement*, not their +semantics. + +### 5.1 Header + +Mark + name, the state badge, the switch (primary action, file-toggle clients +only), and a secondary `백업에서 복원…` button. Restore is **offered in every +state where a snapshot exists — including `conflict` and `unsafe`**, where +the switch itself is locked; its preflight (§6) then decides between a clean +restore, a drift-confirmed restore, or a refusal with manual instructions. +"Rollback is always reachable" is the promise; "restore never needs +judgment" is not — see §6. + +### 5.2 Status line + +`적용 14:32 · 백업 14:32 · 마지막 복원 없음` — three facts, code font, no +color unless something needs attention. + +### 5.3 Apply semantics note (per client, one line) + +What "applied" means for this client, from 002: OpenClaw — `실행 중인 +게이트웨이에 즉시 반영됩니다`; Hermes — `새 세션부터 적용됩니다`; Gajae — +`새 세션 또는 /model을 열 때 적용됩니다`; Kimi — `재시작 또는 /reload 시 +적용됩니다 (v2는 파일 변경을 감지합니다)`; Codex — special: this tab is an +informational card, not a switch — it explains that Codex wiring is owned by +the proxy service (`ocx start` injects, `ocx stop` restores) and links to the +service controls. A per-client switch that secretly restarted the whole +proxy would be a bigger hammer than the card promises. + +**5.4 Settings.** Per-client knobs, each behind the same apply button so +settings + switch ride one write path: + +- Exposed models: multi-select from the proxy catalog (defaults to all + visible; this is the toggle-time model list of 003). +- Default-model pointer where the client has one (OpenClaw + `agents.defaults.model.primary`, Kimi `default_model`, Hermes + `model.default`) with an explicit `건드리지 않음` option as the default — + the toggle must not silently hijack the user's default. +- Credential env var name (read-only display of the referenced var, e.g. + `OPENCODEX_API_KEY`) + the existing export/download affordance from + `ClientConfigDialog`. +- Advanced (collapsed): raw config preview with our block highlighted, + ownership/fingerprint details, config path. + +**5.5 Apply history.** Per-client mini log, same row shape as the rollback +center. + +## 6. Rollback UX — the guarantee, designed + +The pitch is "스위치로 뺐다 넣었다 항상 원복" — so rollback is designed as +two recovery levels plus a strict preflight contract, not one button and not +overlapping ones (A-gate amendment: switch-off/`해제` IS the owned-block +removal of 003 §3 — one backend operation, one label; it is not also a +"rollback level"): + +| Level | Surface | Mechanism | When it exists | +|-----|---------|-----------|----------------| +| 1. 되돌리기 (undo) | Toast after a successful apply/disable/refresh + the latest row per client in the rollback center | restores that operation's own pre-write snapshot | the client's **latest** operation only, while its snapshot is retained | +| 2. 복원 (restore) | `백업에서 복원…` in the client header + any rollback-center row | writes a chosen snapshot back wholesale, after the preflight below | whenever that snapshot exists — offered in **any** state, including `conflict`/`unsafe` | + +Vocabulary, fixed: `적용` = switch on (writes our block); `해제` = switch off +(removes exactly our owned block — the 003 disable); `되돌리기` = undo one +operation; `복원` = full-file restore from a snapshot. No fifth verb. + +### 6.1 Operation journal and snapshot identity (A-gate amendment) + +- Every write operation (apply/disable/refresh/restore) gets an immutable + operation id and its **own** snapshot of the pre-write file, stored per + client (e.g. `.ocx-backups/`); a single shared `.bak` + overwrite (the Claude Desktop precedent) cannot honor per-operation undo + and is explicitly not the model here. +- Retention: keep the latest **10** snapshots per client; older ones are + garbage-collected. A history row whose snapshot was collected renders + disabled with `백업 만료됨` — the row stays as a record, the action does + not pretend to exist. +- Undo binds strictly: only the client's newest operation offers + `되돌리기`, and only while the file still matches that operation's + post-write state (otherwise the row degrades to a restore offer). Older + rows offer `이 시점으로 복원…`, which is level 2 with the preflight below — + never a silent multi-step rewind. + +### 6.2 Restore preflight (A-gate amendment) + +"복원" being always offered does not mean it always writes. Before anything +is committed: + +1. **Snapshot the current file first.** Every restore begins by taking a new + snapshot of the file as it exists right now, so a restore is itself + undoable. Rollback can never strand the user. +2. **Drift check.** If the current file still equals the state that snapshot + was taken to protect against (no post-snapshot edits), restore proceeds + with a plain confirm. If the file has drifted — the user or the client + edited it after the snapshot — the dialog says so and names the + consequence (`스냅샷 이후의 변경이 백업으로 보관되고 파일이 교체됩니다`), + requiring an explicit confirm. Newer edits are preserved by step 1's + backup, never silently destroyed. +3. **Refusal path.** If the target path is not a regular writable file + (symlink, directory, missing parent, permission failure), restore refuses + and the Notice names both the snapshot path on disk and the reason, so the + user can finish the job by hand. A rollback feature that dead-ends + silently is worse than none — but one that writes somewhere it must not + is worse still. + +Supporting rules surfaced as UI copy, not buried in docs: + +- Every apply writes a backup first (`백업 보관됨` is a first-class status + fact on cards and headers). +- We only ever remove what we wrote; the `conflict` state exists to prove we + mean it — the switch locks, the dialog shows the drift, and the offered + actions are `복원` / `내용 확인 후 인계` (explicit takeover) / `그대로 두기`. +- `해제` (switch off) is surgical — it removes exactly our owned block and + says so; `복원` is a full-file rollback and says so in its confirm. The + two are never merged into one ambiguous "초기화". + +## 7. State model → visual mapping + +Backend states (003 §3) plus detection, mapped one-to-one onto visuals — the +switch shows on/off, the badge shows truth: + +| State | Badge (tone) | Switch | Primary actions | +|-------|--------------|--------|-----------------| +| 미설치 | `미설치` (faint) | disabled | install-guide link; tab shows empty state | +| 미적용 (`absent`) | `미적용` (faint) | off | apply | +| 적용됨 (`current`) | `적용됨` (green) | on | disable (`해제`), restore | +| 업데이트 필요 (`stale`) | `업데이트 필요` (amber) | on | refresh (re-apply), disable, restore | +| 충돌 (`conflict`) | `충돌` (red) | **locked** | restore (§6.2 preflight), takeover dialog | +| 확인 불가 (`unsafe`) | `확인 불가` (red, outline) | **locked** | restore (§6.2 preflight), open config path | + +Tones ride existing tokens (`--green`/`--amber`/`--accent` + `-soft` pairs); +no new hues. Every async action (detection scan, apply, restore) drives the +existing `Notice` + busy/disabled semantics; polls stay `active`-gated per +tab as in Logs. + +## 8. UX states + +- **Loading**: first detection scan shows skeleton cards in the known grid + structure (known-structure rule); subsequent refreshes keep the grid and + mark only stale rows. +- **Empty**: no clients detected at all -> hero shows one line + (`설치된 클라이언트가 감지되지 않았습니다`) + install links for the + supported set, and the rollback center shows its own empty line. The page + never renders a bare grid. +- **Error**: detection/status poll failure keeps the last good grid, adds the + Logs-precedent stale callout with retry; an apply/restore failure uses the + `Notice` error path with the backup path named (§6). +- **Onboarding**: first visit (no apply history anywhere) pins one + explanation line under the summary strip — what "적용" does (writes one + provider block into the client's config), and the safety promise (backup + first, remove only ours, restore anytime). It dismisses permanently after + the first successful apply. + +## 9. Lazy-gate audit (decision points justified) + +| Decision point | Disposition | +|----------------|-------------| +| Per-toggle confirm on enable | **Deleted** — undo toast (level 1) carries reversibility; a confirm on a fully reversible action is pure cost | +| Confirm on `모두 해제` | Kept — bulk + destructive-adjacent (exemption class) | +| Confirm on restore / takeover | Kept — full-file overwrite / foreign-block ownership change (exemption class) | +| Default-model pointer per client | Demoted — defaults to `건드리지 않음`; system absorbs the safe choice | +| Raw config preview / fingerprint details | Demoted — collapsed "고급" section | +| `모두 적용` bulk enable | **Deleted** — preconditions differ per client; the button could never mean what it says | + +One primary action per client surface: the switch on file-toggle clients +(§5.0), the existing primary flow on the four exception clients. Everything +else is secondary or collapsed. + +## 10. Accessibility and keyboard + +- Tab strip: `role="tablist"`, `aria-selected`, ArrowLeft/Right + Home/End, + `focus({ preventScroll: true })` — the Claude strip's focus behavior (the + Logs strip does not scroll or `preventScroll`; §3.2 picked wrapping, so no + tab can focus off-screen). +- Switch: existing `Switch` component (`aria-pressed`, `aria-label`). +- Locked switches (conflict/unsafe) are `disabled` + `aria-describedby` + pointing at the badge text, so the lock explains itself. Implementation + note: today's `Switch` accepts no `aria-describedby`/extra props — this + requires a small component-contract extension, flagged here so the + implementation cycle prices it. +- Apply/restore outcomes announce through `Notice` (`role="status"`). + +## 11. Migration map (existing surfaces -> new home) + +| Today | Becomes | +|-------|---------| +| sidebar `api` (ApiKeys page) | `#integrations/keys` minus the client-config panel | +| sidebar `claude` (Claude hub: Code/Desktop) | `#integrations/claude`, composition unchanged | +| sidebar `grok` (Grok page) | `#integrations/grok`, content unchanged — retains its select/save/apply flow; **no** integration switch or snapshot-rollback chrome (§5.0) | +| sidebar Claude inline Switch | Claude **Code** header switch specifically (it drives `/api/claude-code` only; Desktop has no such switch) | +| `ClientConfigPanel` per-client rows | export/settings section inside the OpenCode and Pi tabs only, until new export contracts exist for the other clients (§5.0) | +| ClaudeDesktop apply/status machinery | **visual** precedent for the file-toggle tabs' status chrome (§5.1-5.2) — semantics follow §5.0 per client, never copied wholesale | + +## 12. Non-goals and open questions + +Non-goals: no redesign of the sidebar shell, theme system, or other pages; no +new color tokens; no concept-image exploration (§1); no per-client branding +beyond marks; no implementation (component diffs are the next cycle's decade +docs). + +Open questions for the implementation cycle: + +1. Detection contract per client (binary on PATH? config dir existence? + version probe) — needs the same source-level rigor as 002, per client. +2. Where apply history persists (opencodex config vs a small journal file) + and its rotation bound. +3. i18n: six locales need the new `nav.integrations` + state-badge strings; + Korean copy above is the source of truth for tone. + +Resolved during A-gate (no longer open): the Codex tab is an informational +card with no switch (§5.0, §5.3); snapshot retention is count-based at 10 +per client (§6.1). diff --git a/devlog/_plan/260802_codex_set_prompt_composer/000_plan.md b/devlog/_plan/260802_codex_set_prompt_composer/000_plan.md new file mode 100644 index 0000000000..a2912f2b7f --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/000_plan.md @@ -0,0 +1,166 @@ +# 000 — Codex Set: the prompt composer + +Unit: `devlog/_plan/260802_codex_set_prompt_composer/` +Opened: 2026-08-02 · Work class: C4 · Branch target: `dev` +Base commit for every opencodex citation: `f9b9440c5` ("release: v2.10.0"). +Base commit for every upstream citation: `2b5bdcf67` in +`/Users/jun/developer/codex/121_openai-codex` ("Support portable Agent Plugins +throughout installation (#36544)"), pulled 2026-08-02. + +## Objective + +Turn the `Codex Auth` tab into `Codex Set` — a page that configures Codex as a +whole, not just its accounts — and give it a second section that composes the +Codex prompt stack. + +Verbatim ask, decomposed: + +1. "codex auth 탭 이름을 codex set로 바꾸고, 현재 창은 multi-auth로 바꾸고" + — the page becomes `Codex Set`; today's account-pool content becomes its + `Multi-auth` section. +2. "그 옆에 codex prompt, 아니 뭐 prompt, 그냥 prompt 이렇게 넣고" — a second + section named `Prompt`. +3. "상단 로그 디버그 탭처럼 왼쪽, 오른쪽 창을 이동하는 걸로" — the Logs/Debug + tab pattern, not the scrolling section-tabs pattern. See §Layout decision. +4. "계층적으로 들어가는 프롬프트를 스위치로 껐다 켰다" — each built-in prompt + layer gets a switch. +5. "우리가 추가하는 레이어들을 플러스로 이제 넣을 수 있도록" — a `+` affordance + adds custom layers. +6. "기존 프롬프트는 스위치를 끄고, 그 행을 삭제하는 건 불가능하지만" — built-in + rows can be switched off but never deleted. +7. "커스텀으로 넣는 프롬프트는 팝업이 떠서 그 안에 프롬프트를 삽입하고, + 저장하고, 스위치를 껐다 킬 수 있고, 그 행 자체를 삭제할 수도" — custom rows + open an editable dialog and can be deleted. +8. "기본적으로 있는 프롬프트는 팝업으로 열리지만 편집 불가능하게" — built-in + rows open a read-only dialog. +9. "절대 끌 수 없는 프롬프트는 프롬프트 설정창에서 절대 끌 수 없게 만들어놔" — + layers with no upstream off-switch must render as permanently on. This is the + single hardest constraint in the unit and §4 of `001` proves which layers + those are. +10. "클로드 코드의 프리셋을 제공한다든지, 아니면 Grok Build의 시스템 프롬프트를 + 제공한다든지" — ship presets, "codex 호환성이 있게 우리가 좀 조작을 해서". +11. "theme도 넣는데 ... 나중에 하는 걸로 그냥 설계해가지고 별도 devlog로 그냥 + 잠깐 스텝만 남겨놓고" — Theme is deferred; a separate stub unit records the + steps, and nothing in this unit implements it. + +"굳이 이게 완성본이지 않고, 우리는 일단 기능만 제공하고 나중에 유저 피드백을 +받아서" — ship the whole feature, expect the shape to move on feedback. + +## Evidence base + +Four parallel read-only researchers, all against the two frozen HEADs above: + +| Lane | Question | Document | +|---|---|---| +| Noether | Every prompt layer, its gate, its default | `001` | +| Herschel | AGENTS.md path, `model_instructions_file`, preset sources | `002` | +| Parfit | config.toml load order, write safety, when edits apply | `003` | +| Raman | opencodex GUI/API/test surfaces to reuse | `004` | + +## The finding that reshapes the design + +The ask assumes custom layers are "plus" additions next to the built-ins. The +obvious mechanism — `model_instructions_file` — **replaces the entire base +prompt** rather than adding to it (`002` §3). Wiring the `+` button to it would +silently delete Codex's own instructions the moment a user saved their first +custom layer. + +The additive key is `developer_instructions`: a root string that renders as its +own developer-role section ahead of world-state content +(`config_toml.rs:216`, `session/mod.rs:3413`). So: + +- **Custom layers compose into `developer_instructions`**, concatenated in row + order. Layer identity lives in `$CODEX_HOME/opencodex-prompt.json`, which + opencodex owns; config.toml receives only a generated projection of the + enabled subset. `010` §Storage explains why an earlier in-TOML marker scheme + was abandoned. +- **`model_instructions_file` is not written by the `+` flow at all.** It + appears in the Prompt section only as a read-only *status* row that reports + whether something outside opencodex has replaced the base prompt. + +This is a deviation from the literal ask and is called out here rather than +buried: the requested capability ships, through a different key, because the +obvious key destroys what the user wanted to keep. + +## Layout decision + +The ask names the Logs/Debug tab as the model. `004` §A3 establishes that Logs +does **not** use `SectionTabs` (the sticky scroll-spy strip on Usage / Subagents +/ API Keys). It swaps exclusive tabpanels and persists the choice in the hash +(`#logs` vs `#logs/debug`), lazy-mounting Debug on first visit +(`Logs.tsx:408-425`, `Logs.tsx:551`). + +That is the right pattern here and matches "왼쪽, 오른쪽 창을 이동하는" more +precisely than a scrolling page would: Multi-auth and Prompt are unrelated +surfaces, and Multi-auth polls `/api/codex-auth/*` on a 30s timer that should +not run while the user is editing prompts. + +Decision: **exclusive tabpanels, hash-persisted, Logs-shaped.** +`#codex-set` = Multi-auth, `#codex-set/prompt` = Prompt. + +## Route identity + +`004` §A1 flags this as UNKNOWN in the ask. Decision: **rename the route id to +`codex-set`** and add `codex-auth` → `codex-set` to the existing legacy-redirect +table (`app-routing.ts:85-93`) so bookmarks and the Providers deep link +(`providers-page-utils.ts:19`) keep working. + +The backend namespace `/api/codex-auth/*` **does not move**. It is load-bearing +across a dozen test files and renaming it buys nothing. + +## Constraints + +| Constraint | Source | +|---|---| +| Layers with no upstream off-switch must be non-disableable in the UI | Ask item 9; proven set in `001` §4 | +| Custom text never routes through `model_instructions_file` | `002` §3 | +| Writes must preserve user comments and formatting | `003` §4; `features.ts:262` precedent | +| Only marker-owned lines may be rewritten or deleted | `injected-marker.ts:53-60` | +| Copy says "applies to new sessions", never "next turn" | `003` §3 | +| Presets are adapted, never pasted verbatim | `002` §5-6 | +| A key opencodex does not recognise is left untouched, not deleted | `003` §5 | +| No secret, token, or account identifier is ever serialized | AGENTS.md privacy boundary | +| `bun run typecheck`, `bun run test`, `privacy:scan`, `lint:gui` stay green | AGENTS.md | + +## Work phases + +One decade doc per phase; one PABCD cycle per decade doc. + +| Phase | Doc | Deliverable | Independently landable because | +|---|---|---|---| +| WP1 | `010` | `prompt-layers.ts`: inventory, toggles, custom store, revisions | pure module + tests, no consumer needed | +| WP2 | `020` | `/api/codex-prompt` — the **complete** DTO | WP1 exports every field it serializes | +| WP3 | `030` | Shell: rename, tabpanels, routing, i18n, loading contract | Prompt panel renders live toggle rows from WP2 | +| WP4 | `040` | Full layer list: all five classes, read-only dialog | WP2's inventory already carries class and order | +| WP5 | `050` | Custom layers: `+`, editor, delete, reorder, **linter** | linter moves here; no forward dependency | +| WP6 | `060` | Presets only | presets need the linter, so it ships after it | +| WP7 | `070` | Docs, locale parity, full gate | everything above has landed | + +Deferred, stub only: `090` (Theme). + +### Re-slicing after audit + +The first split had four forward dependencies, all found by an independent +audit: WP3 deferred its loading-contract migration to WP4, WP5 rendered a linter +WP6 owned and shipped an empty preset submenu, WP4's dialog promised per-layer +text nothing produced, and WP2 needed inventory metadata WP1 did not export. + +Four corrections, in order: + +1. **WP1 owns `LAYER_INVENTORY`.** One definition; WP2 projects it. Blocker 7. +2. **WP3 ships a working panel**, not a placeholder: toggle rows plus the + loading contract. WP4 then adds the non-toggle classes and the dialog. +3. **The linter moves to WP5**, where it is consumed. WP6 becomes presets alone + and no longer ships UI ahead of content. +4. **WP4's dialog shows what we can read**, and says so plainly where we cannot. + Codex exposes no API for rendered layer text, so the dialog explains the + layer, names its key, and shows its value — the honest scope. + +## Out of scope + +- Theme (recorded in `090`, implemented later). +- Any change to `/api/codex-auth/*`. +- Any change to how opencodex proxies requests. This unit writes Codex's own + config; it does not touch the proxy's prompt handling. +- Editing AGENTS.md files. The Prompt section reports the AGENTS.md layer but + never writes project docs. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/001_prompt_layer_inventory.md b/devlog/_plan/260802_codex_set_prompt_composer/001_prompt_layer_inventory.md new file mode 100644 index 0000000000..810bbbebcd --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/001_prompt_layer_inventory.md @@ -0,0 +1,242 @@ +# 001 — The Codex prompt stack, layer by layer + +Researcher: Noether (read-only). Upstream HEAD `2b5bdcf67`. +Every row below was read in `/Users/jun/developer/codex/121_openai-codex`. + +## 1. Assembly order + +Sections are registered by `add_section()` in `codex-rs/core/src/session/world_state.rs`. +Initial-context rendering preserves that order with two exceptions: model-switch +moves to the front and multi-agent mode to the back of the developer-message +sequence (`session/mod.rs:3528-3538`). + +Classes are the five defined in §4. This table uses that vocabulary and no +other. An earlier draft used an ad-hoc ALWAYS-ON/TOGGLE/FEATURE split that +contradicted §4; two audit rounds flagged it, and the second was still correct +because the fix had not actually landed on this block. + +| # | Layer | Tag | Role | Gate | Default | Class | +|---:|---|---|---|---|---|---| +| 1 | Model **switch** | `` | dev | model changed + instructions non-empty | — | `runtime-conditional` | +| 2 | Personality | `` | dev | `[features] personality` | on | `feature-gated` | +| 3 | Context-window guidance | `` | dev | `[features] token_budget` | off | `feature-gated` | +| 4 | Realtime | `` | dev | realtime activation state | — | `runtime-conditional` | +| 5 | AGENTS.md | `` | user | discovered content exists | — | `runtime-conditional` | +| 6 | Permissions | `` | dev | `include_permissions_instructions` | on | `config-toggle` | +| 7 | Collaboration mode | `` | dev | `include_collaboration_mode_instructions` | on | `config-toggle` | +| 8 | Environment context | `` | user | `include_environment_context` | on | `config-toggle` | +| 9 | Environments instructions | `` | dev | `include_environment_context` AND `[features] deferred_executor` | off | `feature-gated` | +| 10 | Apps | `` | dev | `include_apps_instructions` + connector present | on | `config-toggle` | +| 11 | Plugins | `` | dev | `plugins_available` at turn build — see §Plugins | — | `runtime-conditional` | +| 12 | Tools | `` | dev | `[features] deferred_tool_world_state` | off | `feature-gated` | +| 13 | Skills (extension) | `` | dev | `[skills] include_instructions` | on | `config-toggle` | +| 14 | Multi-agent mode | `` | dev | `[features.multi_agent_v2] enabled` | off | `feature-gated` | + +**Base/model instructions are not in this table.** They are class `base` and +travel in the request's `instructions` field, not as a world-state section +(`client.rs:861-887`). Row 1 is the model-*switch* transition — a different +thing that an earlier draft conflated with it. + +Line references, in registration order: `world_state.rs:61`, `:66`, `:88`, +`:99`, `:113`, `:114`, `:139`, `:149`, `:168`, `:175`, `:187`, `:190`, `:208`, +`:228`. + +Ordering caveat: Skills is an *extension* contribution, so its position among +other extensions depends on registration order. Statically guaranteed is only +that all extension contributions land between Tools and Multi-agent mode +(`world_state.rs:187`, `:208`, `:228`). The UI must therefore not promise an +exact index for Skills. + +## 2. The five direct off-switches + +These are the only keys that turn a layer off without side effects. All five +default to on when unset. + +| Key | TOML position | Resolves at | +|---|---|---| +| `include_permissions_instructions` | root | `config/mod.rs:3836` | +| `include_apps_instructions` | root | `config/mod.rs:3837` | +| `include_collaboration_mode_instructions` | root | `config/mod.rs:3838` | +| `include_environment_context` | root | `config/mod.rs:3845` | +| `[skills] include_instructions` | `[skills]` table | `config/mod.rs:3840` | + +Declared as `Option` at `config_toml.rs:220-229` and `skills_config.rs:30`; +`unwrap_or(true)` at the resolve sites above. + +## 3. Feature-gated layers + +Reachable from config.toml, but through `[features]` rather than an `include_*` +key, and turning them off changes more than prompt text. The Prompt section +shows these as **status rows, not switches** — flipping `multi_agent_v2` from a +prompt page would silently reconfigure subagent concurrency. + +`personality` (default on, `features/lib.rs:1373`), `token_budget` (off, +`:1337`), `deferred_executor` (off, `:883`), +`deferred_tool_world_state` (off, `:1151`), `multi_agent_v2` (off, `:1097`). + +`[features] plugins` (on, `:1181`) is deliberately **not** in this list: it +influences plugin loading but does not gate the `` +section, which follows a runtime OR. See §Plugins. + +## 4. The canonical taxonomy — five classes, not two + +An earlier draft of this section carried a single "cannot be turned off" list. +An independent audit found it self-contradictory: it listed Plugins as +feature-gated in §3 and simultaneously as non-disableable here, and it conflated +"has no `include_*` key" with "cannot be suppressed at all". Both errors would +have propagated straight into the API's response. + +Every layer belongs to exactly **one** of these classes. This taxonomy is the +single source for `020`'s response and `040`'s row kinds; a contract test +asserts the partition is total and disjoint. + +### Class A — `base` — the request's own instruction field + +| id | Layer | Why it is class A | +|---|---|---| +| `base-instructions` | base/model instructions | base instructions travel in the Responses `instructions` field, or as a leading developer message under Responses Lite (`client.rs:861-887`). **No `include_base_instructions` key exists anywhere in the schema.** Content is replaceable via `model_instructions_file`; the field itself is not user-suppressible. | + +Exactly one member. It is not a world-state section at all. + +The audit judged an earlier wording — "the request always carries non-empty base +instructions" — overstated, since `client.rs:861-887` shows *how* they are sent +rather than proving they are never empty. Narrowed accordingly: what class A +asserts is the absence of an off-switch, which is exactly what the schema shows. + +### Class B — `config-toggle` — a direct boolean in config.toml + +`permissions`, `collaboration`, `environment`, `apps`, `skills`. The five keys +of §2. **These are the only rows that get a switch.** + +### Class C — `feature-gated` — reachable, but through `[features]` + +| id | Governing key | Default | +|---|---|---| +| `personality` | `[features] personality` | on | +| `context-window-guidance` | `[features] token_budget` | off | +| `environments-instructions` | `[features] deferred_executor` | off | +| `tools` | `[features] deferred_tool_world_state` | off | +| `multi-agent-mode` | `[features.multi_agent_v2] enabled` | off | + +Class C rows get **no switch** — flipping `multi_agent_v2` from a prompt page +would silently reconfigure subagent concurrency — but they are honestly labelled +as configurable elsewhere, with a link to the setting that owns them. + +**Plugins is not in this class**, though two earlier drafts put it here. See +§Plugins below. + +### Class D — `runtime-conditional` — no config gate, presence follows state + +| id | Emits when | Evidence | +|---|---|---| +| `model-switch` | the model changed and instructions are non-empty | `model.rs:44-58` | +| `agents-md` | discovered project docs produced content | `world_state.rs:113`, `agents_md.rs:89-110` | +| `realtime` | entering or leaving active realtime | `realtime.rs:43-66` | +| `plugins` | `plugins_available` is true at turn build | `mcp.rs:200-202`, `world_state.rs:187-189` | + +### Plugins — why it took three tries + +Draft 1 called this layer impossible to disable. Draft 2 moved it to +`feature-gated`. Both overstated the source, and an audit round proved it by +opening the code: + +```rust +// core/src/mcp.rs:200-202 +let plugins_available = + selected_plugin_available || !loaded_plugins.capability_summaries().is_empty(); +``` + +`Feature::Plugins` feeds `plugins_config_input()`, which governs ordinary plugin +loading — the right operand. But `selected_plugin_available` is an **independent +OR path** that can make the section emit regardless of the loaded set. The +feature flag *influences* emission; it does not gate it. + +The citation earlier drafts leaned on, `session/mod.rs:3422-3430`, gates +*recommended plugin candidates* — adjacent machinery, not this section. The +section receives `step_context.mcp.plugins_available()` +(`world_state.rs:187-189`). + +Defensible: there is no `include_plugins_instructions` key, and emission follows +a runtime availability computation. That is `runtime-conditional` — the UI shows +a condition and no switch. + +**UNKNOWN:** whether `[features] plugins = false` alone suppresses the section on +every path, given the `selected_plugin_available` operand. Settling it needs a +trace of that variable's producers. The UI renders identically either way, so it +is recorded rather than resolved. + +No boolean anywhere suppresses these. They can still be *empty* — a zero +`project_doc_max_bytes` yields no AGENTS.md content — but emptiness is not a +toggle, and the UI must not present it as one. + +### Class E — `extension-unknown` + +Core iterates registered extension contributors unconditionally +(`world_state.rs:208`), but each extension decides its own availability. So the +honest claim is "no *core* include switch", not "cannot be turned off" — the +audit flagged the stronger wording as overstated and it is. + +Skills is the one extension with a known config gate and therefore sits in class +B. Every other extension layer is class E: enumerable only at runtime, if at +all. + +### What this means on the wire + +`020` serializes **one** array — `inventory`, which is `LAYER_INVENTORY` from +WP1, each entry carrying its `class`. There is no separate `locked` or +`features` array; an earlier draft invented both, and they would have drifted +from this taxonomy within a release. + +A row gets a switch **iff** `class === "config-toggle"`. Classes A and D are the +rows where a switch cannot exist; class C is configurable elsewhere; class E is +reported as `extensionLayersEnumerable: false` rather than as a list, because we +cannot enumerate it. + +Ask item 9 — "절대 끌 수 없는 프롬프트는 절대 끌 수 없게" — is satisfied by +classes A and D. That is the set the tests in `020` case 5 and `040` case 2 +defend, both driven from `inventory` rather than a hand-maintained list. + +## 5. Content-override keys + +| Key | Position | Semantics | +|---|---|---| +| `model_instructions_file` | root / profile, path | **REPLACES** base instructions (`config/mod.rs:3825-3832`) | +| `instructions` | root, string | legacy base override, below the file key | +| `developer_instructions` | root, string | **ADDS** a developer section before world state (`session/mod.rs:3413`) | +| `experimental_compact_prompt_file` | root / profile, path | replaces the compaction prompt only | +| `experimental_realtime_start_instructions` | root, string | replaces realtime start text | +| `[features.multi_agent_v2] subagent_developer_instructions` | table, string | replaces inherited subagent dev instructions | +| `[features.multi_agent_v2] multi_agent_mode_hint_text` | table, string | empty string suppresses layer 14 | +| `[features.token_budget] guidance_message` | table, string | supplies layer 3's body | +| `[auto_review] policy` | table, string | augments the guardian template | + +Not valid config.toml keys at this HEAD: `base_instructions` (runtime override +only, `config/mod.rs:2566`), `experimental_instructions_file` (removed +2026-05-14 in `7dbe1c949`), root `guardian_policy_config` (managed +`requirements.toml` only). + +`developer_instructions` is the key WP5 composes into. It is the only root +string that adds a layer instead of replacing one. + +## 6. Version sensitivity + +This surface is young and still moving. Landing dates from pickaxe history: + +- `include_apps_instructions`, `include_permissions_instructions`, + `include_environment_context` — `8d1964686` / `91ca49e53`, 2026-04-03. +- `include_collaboration_mode_instructions` — `8123bddb1`, 2026-05-12. +- `experimental_instructions_file` **removed** — `7dbe1c949`, 2026-05-14. +- `subagent_developer_instructions` — `49025589b`, 2026-07-28. +- Skills moved out of core to an extension — `0d109f097`, 2026-07-31. +- Environment-context behavior last changed — `9eeac78b3`, 2026-07-30. + +Consequence for the implementation: treat every key as possibly-absent. WP1 +reads what is there and reports the rest as unknown rather than asserting a +default the running Codex may not share. + +## Needs verification + +- Skills' index relative to other extensions is registration-order dependent; + only the Tools→Multi-agent bracket is static. +- Third-party extensions may add further layers with their own gates. The UI + must not claim its list is exhaustive. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/002_injection_paths_and_presets.md b/devlog/_plan/260802_codex_set_prompt_composer/002_injection_paths_and_presets.md new file mode 100644 index 0000000000..576f59859f --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/002_injection_paths_and_presets.md @@ -0,0 +1,147 @@ +# 002 — Where user text can legally enter, and what presets cost + +Researcher: Herschel (read-only). Upstream HEAD `2b5bdcf67`. + +## 1. AGENTS.md discovery + +Discovery walks from cwd up to the nearest ancestor holding a +`project_root_markers` entry — `.git` by default — and never above it +(`agents_md.rs:8`). Directories are then read root-to-cwd inclusive +(`agents_md.rs:188`). Per directory the first match wins: +`AGENTS.override.md`, then `AGENTS.md`, then configured fallbacks +(`agents_md.rs:211`, `:234`). + +Host-provided instructions come first; the transition into project content is +the literal separator `\n\n--- project-doc ---\n\n`; adjacent project files join +with `\n\n` (`agents_md.rs:319`). + +**Budget: 32 KiB aggregate per environment**, consumed root-first. The last +file that fits is byte-truncated and decoded lossily; everything after it is +dropped (`config/mod.rs:204`, `agents_md.rs:95`, `:106`). A budget of zero +disables project docs entirely. + +That truncation direction matters for the UI: the *deepest* AGENTS.md — usually +the one the user thinks is most specific — is the first to vanish. + +## 2. Rendered shape + +```text +# AGENTS.md instructions for + + + + +--- project-doc --- + + + + + +``` + +Produced by `UserInstructions::body()` (`user_instructions.rs:9-28`). It is a +**user-role** fragment, not part of the system `instructions` field. Base +instructions travel in the Responses `instructions` field, or as a leading +developer message under Responses Lite (`client.rs:861`). + +## 3. `model_instructions_file` — why the `+` button must not use it + +- Precedence: runtime `base_instructions` > `model_instructions_file` > root + `instructions` (`config/mod.rs:3832`). +- Once resolved it beats conversation-history instructions and the catalog's + baked instructions (`session/mod.rs:634`, `:652`). +- It **replaces the base prompt entirely. It does not append.** +- A missing, unreadable, non-UTF-8, or empty file is a hard config-load error, + not a fallback (`config/mod.rs:4254`). +- Validation stops at "readable and non-empty after trim". No schema, no + tool-contract check, no size cap (`config/mod.rs:4267`). + +So a naive "add a custom layer" that wrote this key would delete Codex's own +instructions, and a bad path would leave Codex refusing to start. Both failure +modes are silent from the GUI's point of view. + +`000` §"The finding that reshapes the design" records the consequence: +custom layers compose into `developer_instructions`; this key is surfaced +read-only, as a warning row when something else has set it. + +## 4. Bundled prompt assets + +At this HEAD none of the six root markdown prompts under `codex-rs/core/` has a +production reference; only `prompt_with_apply_patch_instructions.md` is +referenced, and only by a test (`session/tests.rs:1435`). + +| Asset | Bytes | Active selector | +|---|---:|---| +| `gpt_5_1_prompt.md` | 24,204 | NONE at HEAD | +| `gpt_5_2_prompt.md` | 21,652 | NONE at HEAD | +| `gpt-5.1-codex-max_prompt.md` | 7,589 | NONE at HEAD | +| `gpt-5.2-codex_prompt.md` | 7,589 | NONE at HEAD | +| `gpt_5_codex_prompt.md` | 6,647 | NONE at HEAD | +| `prompt_with_apply_patch_instructions.md` | 23,988 | test only | + +Live base instructions come from the model catalog instead +(`openai_models.rs:478`). In the bundled snapshot the default is `gpt-5.6-sol` +with instructions embedded in `models-manager/models.json:4`. A remote catalog +refresh can change that, so "the current base prompt" is catalog-dependent and +the UI must read it rather than name it. + +## 5. Preset source material — what it actually is + +The ask names Claude Code and Grok Build presets. Neither source is a prompt +that can be shipped as-is. + +**`002_prompt-context/02_cc_prompt.md`** is not a Claude Code system prompt. It +is a 15,163-byte Korean *analysis document* about one (`:10`, `:44`, `:100`). +Pasting it would inject commentary and source snippets, plus "You are Claude +Code", Claude tool names, `CLAUDE.md`, ``, and Anthropic +billing semantics. + +**`02_gr_prompt.md`** is likewise a dossier — 24,013 bytes mixing current OSS +findings with a binary-analysis appendix (`:8`, `:109`, `:122`). It carries +unresolved `${{ tools.by_kind.* }}` placeholders that only Grok's MiniJinja +renderer expands (`:44`). + +**Grok Build's real assets** are `prompt.md` (4,638 B), `apply_patch_prompt.md` +(21,360 B), `subagent_prompt.md` (4,741 B) under +`180_grok-build/crates/codegen/xai-grok-agent/templates/`. The default opens by +identifying the model as "Grok released by xAI" (`prompt.md:1`) and uses +render-time tool placeholders (`context.rs:263`). + +Conclusion for WP6: presets are **authored by us**, distilling behavioral intent +from these sources into harness-neutral text. They are never verbatim copies. +Each preset ships with a provenance line naming what it was derived from. + +## 6. The compatibility hazards WP6's linter checks + +Each is grounded in something read above: + +| Hazard | Why it breaks | Evidence | +|---|---|---| +| Identity redefinition ("You are Claude Code" / "You are Grok") | contradicts the selected base identity | `02_cc_prompt.md:44`, Grok `prompt.md:1` | +| Foreign tool names (`Read`/`Edit`/`Bash`) | Codex's registry defines tools, not prose | `model_info.rs:151` | +| Unresolved template placeholders (`${{ ... }}`) | Codex runs no MiniJinja over instruction text | `context.rs:252` | +| Redefining `apply_patch` or prescribing another edit protocol | same registry argument | `model_info.rs:151` | +| Foreign approval vocabulary (`always-approve`, Claude permission modes) | Codex injects its own permission block afterwards | `world_state.rs:114` | +| Claims about cwd, date, network, installed tools | authoritative environment context is generated later | `world_state.rs:149` | + +**The 32 KiB budget does not belong on this list.** An earlier draft cited +`agents_md.rs:109` as a reason to warn on long custom layers. That budget +governs **project-document loading** — the AGENTS.md chain — and has no bearing +on root `developer_instructions`, which is read as a plain config string with no +cap (`config_toml.rs:216`). An independent audit flagged the citation as simply +wrong, and it is. + +`060`'s size advisory survives, but as declared opencodex policy justified by +request cost, not as an upstream constraint we discovered. + +The linter warns; it does not block. A user who wants to override Codex's +identity is allowed to — but not by accident. + +## Needs verification + +- Historical slug mapping for the six unreferenced markdown assets is UNKNOWN + without git archaeology; not needed for this unit. +- Whether a live remote catalog currently supersedes `gpt-5.6-sol` is UNKNOWN + from a static checkout. WP4 reads it at runtime instead of hardcoding. +- Model compliance under deliberately contradictory layers is unproven; static + ordering is proven, behavior under conflict would need live capture. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/003_config_write_semantics.md b/devlog/_plan/260802_codex_set_prompt_composer/003_config_write_semantics.md new file mode 100644 index 0000000000..9a189004d0 --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/003_config_write_semantics.md @@ -0,0 +1,137 @@ +# 003 — config.toml: layering, write safety, and when an edit applies + +Researcher: Parfit (read-only). Upstream HEAD `2b5bdcf67`. + +## 1. Load order + +Lowest to highest precedence (`config/src/loader/mod.rs:96-109`, assembled at +`:225-413`, folded lowest-first at `state.rs:488-500`): + +1. System `/etc/codex/config.toml` +2. Cloud enterprise fragments +3. **User `${CODEX_HOME}/config.toml`** ← what we write +4. Profile-v2 `${CODEX_HOME}/.config.toml` +5. Project-local trusted configs (cwd, ancestor, repo `.codex/config.toml`) +6. Runtime/session flags including CLI `-c` +7. Thread-provided layers +8. Legacy `managed_config.toml` / MDM + +Defaults are not a physical layer. Absent values deserialize to `None` and +runtime defaults apply afterwards (`config/mod.rs:3836-3845`). + +## 2. Profiles — do not write `[profiles.x]` + +Two systems exist and only one works: + +- **Profile-v2 (current):** `--profile work` loads `${CODEX_HOME}/work.config.toml` + as a full overlay (`loader/mod.rs:245-293`). Keys go at that file's **root**. +- **Legacy `[profiles.]`:** still in the schema (`config_toml.rs:308-313`), + but selecting it via root `profile = "name"` is now a **hard error** + directing the user to `.config.toml` (`config/mod.rs:3260-3266`). + +So the five toggles are schema-valid inside `[profiles.x]` and simultaneously +unreachable there. WP1 writes root keys of the user config and nothing else. + +## 3. When an edit takes effect + +`config_lock.rs:142-146` copies the four `include_*` flags into a lock config. +That is **not** a per-session freeze of live config.toml. It materializes +resolved values into an optional exported/replayed lock file, and only when +`config_lock_export_dir` is set (`config_lock.rs:48-71`); replay happens only +under a debug `load_path` (`config/mod.rs:1461-1495`). + +The real timing: config is read while constructing `Config`, copied into +`SessionConfiguration` at session creation (`session/mod.rs:634-713`), and new +turns clone from that rather than re-reading the file +(`turn_context.rs:600-637`, `:475-482`). + +Therefore: + +- An edit does **not** affect the next turn of a running thread. +- It applies when a new session is built from a freshly loaded `Config`. +- A full process restart is **not** proven necessary. + +UNKNOWN: whether each frontend reloads config before every new thread. Settling +it needs a trace of the app-server thread-start path. + +**Mandated UI copy:** "새 세션부터 적용됩니다. 실행 중인 세션은 현재 설정을 +유지합니다." Never "즉시 적용" and never "재시작 필요". + +## 4. Write safety + +Codex rewrites config.toml itself — `codex features enable/disable` +(`cli/src/main.rs:1909-1928`), migration-notice setters (`config/edit.rs:813-829`), +MCP settings. The writer reads the text, parses `toml_edit::DocumentMut`, +applies path-scoped AST edits, serializes, and atomically replaces +(`config/edit.rs:730-769`). + +Comments and unrelated formatting survive; a regression test proves two nested +edits leave everything else byte-identical (`config/edit_tests.rs:600-655`). + +Consequences for us: + +- Our marker comments should survive Codex's own edits. +- If Codex edits the same key, **its value wins.** Same-path keys are shared + ownership, not ours. +- Malformed TOML blocks `DocumentMut` parsing, so Codex fails before writing + (`config/edit.rs:745-749`). Our writer must never emit malformed output. + +## 5. Unknown and malformed keys + +`ConfigToml` carries `#[schemars(deny_unknown_fields)]` but **not** serde's +`deny_unknown_fields` (`config_toml.rs:147-150`). So: + +- Normal mode: a valid-but-unknown root key is **silently ignored**. +- `--strict-config`: unknown keys are a **hard load error** + (`loader/layer_io.rs:104-168`, `config_loader_tests.rs:363-385`). +- Malformed TOML: always `InvalidData`, strict or not. + +A typo like `include_app_instructions = false` therefore does nothing in normal +mode and bricks startup in strict mode. WP1 validates key names against a fixed +allowlist before writing — the GUI can never emit a key it did not intend. + +## 6. Managed layers can defeat our write + +Cloud enterprise fragments sit *below* user config, so our write wins over them +(`cloud_config_layers_tests.rs:82-145`). Cloud *requirements* cannot lock these +keys — `ConfigRequirements` has no `include_*` fields +(`config_requirements.rs:1-28`). + +But legacy `managed_config.toml` / MDM is appended **after** runtime layers +(`loader/mod.rs:369-413`), so a managed value silently overrides ours. + +**This is why WP1 reports `defaultedUserValue`, not `effective`.** An earlier +draft of this document told WP2 to return an effective value and render an +override notice. opencodex reads exactly one of the eight layers above, so it +cannot compute the effective value, and an audit correctly rejected the +instruction as an overclaim. + +Resolved-configuration reporting is **deferred**, not approximated: promising +override detection without a read path would relocate the false claim rather +than remove it. `010` and `005` carry the same rule; this section no longer +contradicts them. + +## 7. Canonical target file + +```toml +# ~/.codex/config.toml — root level +include_permissions_instructions = false +include_apps_instructions = false +include_collaboration_mode_instructions = false +include_environment_context = false + +[skills] +include_instructions = false +``` + +Verified against `config.schema.json:5411-5426` and `skills_config.rs:30`. + +## Risks carried into implementation + +| Risk | Mitigation | Phase | +|---|---|---| +| Typo'd key silently no-ops or bricks strict mode | fixed allowlist, no free-form keys | WP1 | +| Managed layer overrides our write | **deferred** — we report this file's value under a name that says so | WP1/WP2 | +| Codex overwrites our value | same-path keys are shared; never assume ownership | WP1 | +| User expects instant effect | "new sessions" copy, enforced by test | WP3 | +| Upstream renames a key | absent key = unknown, not false | WP1 | diff --git a/devlog/_plan/260802_codex_set_prompt_composer/004_surface_inventory.md b/devlog/_plan/260802_codex_set_prompt_composer/004_surface_inventory.md new file mode 100644 index 0000000000..408143df4e --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/004_surface_inventory.md @@ -0,0 +1,151 @@ +# 004 — opencodex surfaces this unit builds on + +Researcher: Raman (read-only). opencodex `dev` @ `f9b9440c5`. + +## A. Route and page shell + +`Page` now lives in `app-routing.ts`, not `App.tsx` (`App.tsx:23`). The id +`codex-auth` appears in: + +| Place | Location | +|---|---| +| `Page` union | `app-routing.ts:5-15` | +| `VALID_PAGES` | `app-routing.ts:20-30` | +| `PAGE_TKEY` | `App.tsx:31-41` | +| `NAV` | `App.tsx:50-52` | +| render branch | `App.tsx:307-316` | +| Providers deep link | `providers-page-utils.ts:14-19` | + +Routing is hash-based; the first segment resolves the page +(`app-routing.ts:36-44`). Navigation pushes history, normalization replaces +(`use-app-route-state.ts:39-90`). There is **no persisted last-page state** +(`use-app-route-state.ts:44`), so a rename needs no storage migration. Legacy +redirects already exist at `app-routing.ts:85-93` — that is where +`codex-auth` → `codex-set` goes. + +`CodexAuth.tsx` (178 lines) is a thin wrapper: it owns the `/api/config` fetch, +the session cache `ocx.codex-auth.config.v1:${apiBase}`, a 30s poll, the +provider-recovery action, and the account-mode banner; then it delegates +everything else to `CodexAccountPool` (`CodexAuth.tsx:93-177`). + +## B. The tab pattern — an important correction + +**Logs does not use `SectionTabs`.** It reuses the `.page-tabs`/`.page-tab` +classes but swaps exclusive tabpanels and persists the choice in the hash +(`Logs.tsx:408-425`, `:522`, `:567`), lazy-mounting Debug on first visit and +keeping it mounted after (`Logs.tsx:411-423`, `:551`). + +`SectionTabs` is the *other* thing: a sticky scroll-spy strip over a normally +scrolling page, used by Usage (`Usage.tsx:716`), Subagents +(`SubagentsWorkspace.tsx:73`), and API Keys (`ApiKeysWorkspace.tsx:206`). Props +are `{scope, items:[{id,label,meta?}], ariaLabel}`; anchors are +`${scope}-section-${id}` (`section-anchors.ts:11`); scroll lock is 1,200 ms +(`section-anchors.ts:23`). + +`000` §Layout decision picks the Logs pattern. This document exists partly to +record that the ask's phrase "상단 로그 디버그 탭처럼" pointed at Logs, and Logs +is the tabpanel pattern — reaching for `SectionTabs` because it has "section" +in the name would have been the wrong build. + +## C. Tests that a rename breaks + +- `gui/tests/sidebar-codex-auth.test.ts:14-23` — presence and routing source. +- `gui/tests/dashboard-tabs.test.ts:45-52` — expects `codex-auth` **second** in + sidebar order. + +Routing tests do not enumerate `codex-auth` today +(`providers-hash-history.test.tsx:54-85`), so WP3 adds explicit old→new +deep-link coverage. `page-loading-contract.test.tsx:25-39` does not list +`CodexAuth` in `MIGRATED`; the new Prompt surface should join it. + +The ~12 files touching `/api/codex-auth/**` test the **backend namespace**, +which does not move. A page rename must not touch them. + +## D. i18n + +`nav.codexAuth` plus **130 `codexAuth.*` keys** per locale, six locales. +Nav key: `en.ts:1108`, `ko.ts:691`, `ja.ts:1058`, `zh.ts:684`, `ru.ts:1100`, +`de.ts:666`. + +English is authoritative: `TKey = keyof typeof en` (`en.ts:1662-1664`), and +every other locale is `Record` (`zh.ts:1-3`, `shared.ts:9-12`). +**Key parity is compile-time enforced** — a missing translation fails +`typecheck`, so all six locales move together in one commit. + +Decision: keep the 130 `codexAuth.*` keys untouched (the pool UI consumes them +directly), add a new `codexSet.*` namespace for the shell and Prompt section. + +## E. Config write precedent + +`src/codex/features.ts` is the model, but its header explicitly forbids +broadening it beyond `multi_agent_v2` (`features.ts:1-15`). WP1 therefore +writes a **new module**, copying the technique: + +- `activeCodexConfigPath()` resolves `CODEX_HOME` at call time, expands `~`, + canonicalizes via `realpathSync.native` (`features.ts:58-67`). +- `dominantEol`/`applyEol` preserve CRLF vs LF (`features.ts:36-48`). +- `setMaxConcurrentThreads` shows the scoped line edit: validate, refuse + unreadable files, match only within the owning table body, stay idempotent, + write atomically (`features.ts:248-310`). + +`OCX_SECTION_MARKER` is `# Auto-injected by opencodex`, defined in +`injected-marker.ts:1-10`. Ownership is adjacency-based: a key is ours only if +the marker precedes it (`injected-marker.ts:53-60`); unmarked user keys are +preserved and block injection (`inject.ts:141-167`). + +## F. Management API + +Handlers take `ManagementContext` and return `Response | null` +(`context.ts:24-30`). Registration is manual: import in `management-api.ts` and +add to the null-coalescing chain (`management-api.ts:59-69`, `:127-138`). + +All `/api/**` passes `requireManagementAuth` (`server/index.ts:448-453`). +Unsafe methods need browser `Origin` plus a matching CSRF token +(`management-auth.ts:246-266`); bodies cap at 2 MiB +(`management-api.ts:84-95`). `jsonResponse` lives at `auth-cors.ts:170-175`. + +Shape to copy: `sidebar-routes.ts:23-89` for the module skeleton, +`agent-settings-routes.ts:127-178` for GET/PUT config-toggle semantics. + +## G. GUI data contract + +`useKeyedClientResource(key, deps, loader, options)` distinguishes `loading` +(replace content) from `refreshing` (keep stale content visible) +(`client-resource.ts:3-17`, `:51-55`). After a mutation, publish the confirmed +server DTO with `setClientResourceData` (`client-resource.ts:464-482`). +`useDataSurface` wraps it and classifies cold/stale/empty/failure states +(`data-surface.ts:12-48`, `:137-152`). + +## H. Test harnesses + +Route tests call `handleManagementAPI` directly with a concrete `URL` and a +`Request` carrying `Host` — origin derivation rejects a missing Host. See +`management-client-config-route.test.ts:46-88` (fixtures + injected seams, +plus hostile-origin rejection at `:240-243`) and `sidebar-routes.test.ts:18-58` +(helper + `finally` restore). + +**Route tests must never resolve the real `CODEX_HOME`.** Writer tests take an +explicit temp `configPath`; route tests inject a writer seam. + +GUI tests use Bun + happy-dom + React `act`. `client-config-panel.test.tsx` is +the dialog reference: global install/restore (`:55-69`), lazy +`react-dom/client` under `LanguageProvider` (`:86-113`), stubbed fetch +(`:132-152`), and native dialog assertions including `aria-labelledby`, +Escape, and focus return (`:204-222`). + +## Reuse map + +| New work | Model on | +|---|---| +| Codex Set shell, exclusive tabpanels | `Logs.tsx:408` | +| Multi-auth section content | `CodexAuth.tsx:93` (kept whole) | +| Prompt layer/custom rows + dialogs | `ClientConfigRow.tsx`, `ClientConfigDialog.tsx` | +| Prompt GET + loading | `data-surface.ts:137` | +| Post-write cache publish | `client-resource.ts:464` | +| TOML reader/writer | `features.ts:58`, `:248` — in a NEW module | +| Route module skeleton | `sidebar-routes.ts:47` | +| GET/PUT toggle semantics | `agent-settings-routes.ts:159` | +| Route registration | `management-api.ts:59`, `:127` | +| Route tests | `sidebar-routes.test.ts:18` | +| Dialog tests | `client-config-panel.test.tsx:86` | +| Deep-link regression | `providers-hash-history.test.tsx:54` | diff --git a/devlog/_plan/260802_codex_set_prompt_composer/005_ux_design.md b/devlog/_plan/260802_codex_set_prompt_composer/005_ux_design.md new file mode 100644 index 0000000000..eff64e474a --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/005_ux_design.md @@ -0,0 +1,185 @@ +# 005 — Prompt section: the design + +Depends on `001` (what is toggleable), `002` (what may be written), `003` +(when it applies), `004` (what to build on). + +## The page + +``` +Codex Set #codex-set +┌──────────────┬──────────┐ +│ Multi-auth │ Prompt │ ← exclusive tabpanels, Logs-shaped +└──────────────┴──────────┘ +``` + +`#codex-set` → Multi-auth (today's account pool, moved whole). +`#codex-set/prompt` → Prompt. Prompt lazy-mounts on first visit and stays +mounted, exactly as Debug does (`Logs.tsx:551`). + +Why exclusive panels rather than a scrolling page: Multi-auth polls +`/api/codex-auth/*` every 30s and owns modal login flows. Those should not run +underneath a prompt editor, and a user scrolling from account cards into prompt +rows is a worse read than a deliberate switch. + +## The Prompt panel + +``` +Prompt [ + Add layer ] +새 세션부터 적용됩니다. 실행 중인 세션은 현재 설정을 유지합니다. + +BUILT-IN + 🔒 Model instructions always on [view] + 🔒 AGENTS.md always on [view] + Permissions [ ●─ ] on [view] + Collaboration mode [ ●─ ] on [view] + Environment context [ ●─ ] on [view] + Apps [ ●─ ] on [view] + Skills [ ●─ ] on [view] + 🔒 Plugins availability [view] + ⚙ Personality feature [view] + ⚙ Multi-agent mode feature [view] + +CUSTOM + My house rules [ ●─ ] on [edit] [×] + Claude Code style [ ─○ ] off [edit] [×] +``` + +Three row kinds, three affordance sets: + +| Kind | Switch | Dialog | Delete | +|---|---|---|---| +| 🔒 locked built-in | **absent** | read-only | never | +| toggleable built-in | present | read-only | never | +| ⚙ feature-gated | **absent**, links to its real setting | read-only | never | +| custom | present | **editable** | yes | + +The lock icon is not a disabled switch. `001` §4 proves these layers have no +off-switch anywhere in Codex; rendering a greyed-out toggle would imply the +capability exists and is merely unavailable. A switch that cannot exist should +not be drawn. This is ask item 9, and WP4 asserts it in a test. + +Feature-gated rows (`001` §3) also get no switch, but for a different reason: +flipping `multi_agent_v2` from a prompt page would silently reconfigure subagent +concurrency. The row states the governing key and links to where it is owned. + +## Ordering + +Rows follow the assembly order in `001` §1, so the list reads as the prompt is +built. Skills carries a quiet note that its exact position among extensions is +registration-dependent (`001` ordering caveat) — the UI must not overpromise. + +Custom layers form a second group below. Within it, order is the composition +order written into `developer_instructions`, and it is reorderable. + +## Dialogs + +Built-in rows open **read-only**: the layer's purpose, its class, the exact +config key where one exists, its default, and this file's value. Copy button, no +editor. Ask item 8. + +Two things the dialog does **not** show, because nothing produces them: + +- **The rendered prompt text.** Codex exposes no API for it, and reconstructing + it would mean reimplementing `world_state.rs` against a moving target + (`001` §6). `040` says the same. +- **The effective value.** opencodex reads one of the eight config layers in + `003` §1, so it reports `defaultedUserValue` — this file's value — and claims + nothing about what the running Codex resolved. + +Custom rows open an **editor**: title, body textarea, live compatibility +warnings (`002` §6), Save, Cancel. Escape cancels and returns focus, matching +`client-config-panel.test.tsx:204-222`. + +## The `+` flow + +`+ Add layer` offers: + +- **Blank** — empty editor. +- **From preset** — a picker of the WP6 presets, each with a provenance line. + +Either way the result is a custom row: editable, toggleable, deletable. + +## What custom rows actually write + +Every layer — enabled or not — lives in `$CODEX_HOME/opencodex-prompt.json`, +which opencodex owns outright. `config.toml` receives a generated projection of +the **enabled** subset, in row order, as exactly two lines: + +```toml +# Auto-injected by opencodex +developer_instructions = "escaped composition of enabled layers" +``` + +A disabled layer keeps its body in the JSON and is simply absent from the +projection, so switching it off removes it from the prompt without losing it. + +`010` §Canonical physical form fixes the shape: always a single-line basic +string, always directly under the marker. Replacement is "find marker, replace +next line" — not a search for a value span. + +If `developer_instructions` exists without our marker, opencodex refuses to +write it and offers an explicit **Adopt** flow that shows the existing text and +imports it as a custom layer on confirmation (`010` §Ownership). Nothing is +overwritten or deleted silently. + +### Accepted characters + +Bodies accept printable Unicode, spaces, and newlines. Tabs are normalized to +four spaces; CRLF is normalized to LF. Control characters are rejected with a +precise message. + +This is not fussiness. `010` records a measured defect in `Bun.TOML.parse` on +Bun 1.3.14 — it transposes `\t` and `\f`, and rejects `\u0007` — so an encoding +we could verify locally is not necessarily the encoding Codex's Rust parser +reads. Restricting the character set makes the escaping total and unambiguous +with three rules, which no parser defect can undermine. + +`model_instructions_file` is never written here. `002` §3 explains why. When +something else has set it, the Prompt panel shows a warning row: the base prompt +has been replaced, by a file opencodex does not manage. + +## Honest status, not optimistic status + +Three places where the UI must resist claiming more than it knows: + +1. **Timing.** "새 세션부터 적용됩니다" (`003` §3). Never "즉시 적용", never + "재시작 필요" — neither is proven. +2. **Scope of what we read.** opencodex reads one of the eight config layers in + `003` §1, so the panel says "이 파일의 설정" and never claims the running + Codex agrees. Managed-override detection is deferred, not approximated. +3. **Completeness.** Third-party extensions can add layers we cannot enumerate + (`001` needs-verification). The list is labelled as the layers opencodex + knows about, not as every layer that exists. + +## Empty and error states + +- **No `~/.codex/config.toml`:** rows render at documented defaults and switches + are **live**. The first toggle creates the file (`010` §First write). An + earlier draft disabled the switches while promising creation "on first + change", which an audit correctly called a contradiction — a disabled switch + makes that first change impossible. +- **Unreadable or malformed config:** the panel refuses to write and says so. + `003` §4 — Codex cannot parse malformed TOML either, so writing would compound + the failure rather than recover from it. +- **`developer_instructions` exists without our marker:** built-in toggles keep + working; the custom group offers **Adopt**, which previews the original line + and the exact decoded body, offers a copy, and imports it only on + confirmation (`010` §Ownership). +- **Our line exists but is malformed:** `drift: "owned-malformed"`, handled the + same way — preview, copy, confirmed re-adopt or replace. Reformatting a line + we generated must never lock a user out. +- **Store lost with a live projection:** `drift: "store-missing"`. Writes are + refused until the user salvages the projected text as one layer, with the + losses listed and a backup written first. +- **No custom layers:** the CUSTOM group shows a one-line invitation, not an + empty box. + +## What the UI does not claim + +`010` renames `effective` to `defaultedUserValue` because opencodex reads one +file out of the eight config layers in `003` §1. The panel therefore says +"이 파일의 설정" and never asserts the running Codex agrees. + +The managed-override notice from `003` §6 is **deferred with the field**. +Detecting an MDM override needs a resolved-config read path we do not have; +shipping the notice without it would move the overclaim rather than remove it. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/010_wp1_prompt_layers_core.md b/devlog/_plan/260802_codex_set_prompt_composer/010_wp1_prompt_layers_core.md new file mode 100644 index 0000000000..bc30659ac7 --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/010_wp1_prompt_layers_core.md @@ -0,0 +1,828 @@ +# 010 — WP1: the config.toml read/write core + +New file: `src/codex/prompt-layers.ts`. No GUI, no route. Pure module + tests. + +`004` §E: `features.ts` forbids broadening itself beyond `multi_agent_v2`, so +this is a sibling module that copies its technique rather than an extension of +it. + +## Exports + +```ts +/** Classes A-E from 001 §4. The partition is total and disjoint. */ +export type LayerClass = + | "base" | "config-toggle" | "feature-gated" + | "runtime-conditional" | "extension-unknown"; + +export type ToggleId = + | "permissions" | "collaboration" | "environment" | "apps" | "skills"; + +/** The canonical inventory. ONE definition, consumed by route and GUI alike. */ +export interface LayerDescriptor { + id: string; + class: LayerClass; + /** config key for config-toggle and feature-gated; null otherwise */ + key: string | null; + /** documented default when the key is absent */ + default: boolean | null; + /** assembly index from 001 §1; null when registration-order dependent */ + order: number | null; +} +export const LAYER_INVENTORY: readonly LayerDescriptor[]; + +export interface ToggleState { + id: ToggleId; + key: string; + /** null = key absent from the user file */ + userFileValue: boolean | null; + /** userFileValue ?? default. NOT the resolved Codex value — see below. */ + defaultedUserValue: boolean; + default: boolean; +} + +export interface CustomLayer { + id: string; // [a-z0-9]{6}, stable across edits + title: string; + body: string; + enabled: boolean; +} + +export interface PromptLayerSnapshot { + configPath: string; + storePath: string; + configExists: boolean; + readable: boolean; + /** false when developer_instructions exists without our marker */ + developerInstructionsOwned: boolean; + /** non-null blocks mutations until resolved; see §Drift */ + drift: Drift; + toggles: ToggleState[]; + custom: CustomLayer[]; + modelInstructionsFile: string | null; // read-only warning (002 §3) + /** SHA-256 over the COMPLETE bytes of both files plus existence flags */ + revision: string; +} + +/** Preview DTOs — returned without writing anything. */ +export interface AdoptPreview { + rawLine: string; + decodedBody: string | null; // null when the form is unsupported + reason: "ok" | "unsupported_form" | "invalid_characters"; + path: string; + line: number; +} +export interface SalvagePreview { + body: string; + /** the DIRECTORY backups are written to. No path is reserved by a preview. */ + backupDir: string; + /** enumerated so the UI can state them; see §Missing store */ + unrecoverable: readonly string[]; +} +``` + +`previewSalvage` returns a directory, not a filename. Naming an exact backup +path during a read-only preview would either reserve it — making the preview a +write — or promise a name that exclusive creation may refuse at commit time. +The actual path is created during the confirmed mutation and returned in its +`WriteResult`. + +```ts + +export type WriteResult = + | { ok: true; changed: boolean; snapshot: PromptLayerSnapshot } + | { ok: false; error: WriteError }; + +export type WriteError = + | "config_unreadable" | "stale_revision" | "developer_instructions_not_owned" + | "unknown_layer" | "store_unreadable" | "invalid_characters" + | "adopt_unsupported_form" | "write_superseded" | "recovery_required"; + +export type Drift = + | "journal-present" | "projection-stale" | "store-missing" + | "owned-malformed" | null; + +/** Pure. Never writes, never locks, never recovers. */ +export function readPromptLayers(opts?: Paths): PromptLayerSnapshot; + +export function setToggle(id: ToggleId, enabled: boolean, revision: string, opts?: Paths): WriteResult; +export function writeCustomLayers(layers: CustomLayer[], revision: string, opts?: Paths): WriteResult; + +/** Preview is read-only; commit requires an explicit confirmation + revision. */ +export function previewAdopt(opts?: Paths): AdoptPreview; +export function adoptDeveloperInstructions(revision: string, opts?: Paths): WriteResult; + +/** `owned-malformed`: re-adopt through the narrow decoder, or replace outright. */ +export function repairOwnedMalformed( + mode: "adopt" | "replace", revision: string, opts?: Paths, +): WriteResult; + +/** `store-missing`: salvage the projection as ONE layer. Preview is read-only. */ +export function previewSalvage(opts?: Paths): SalvagePreview; +export function salvageProjection(revision: string, opts?: Paths): WriteResult; + +/** `journal-present`: run at service start and at every lock acquisition. */ +export function recoverIfNeeded(opts?: Paths): + { ok: true; recovered: boolean } | { ok: false; error: "recovery_required" }; +``` + +Every filesystem mutation lives in WP1. `020` is a transport that validates, +forwards, and serializes — it never opens a file. An earlier draft left adopt, +repair, salvage, and recovery undeclared, so `020` could not have been built +from WP1's stated contract at all. + +### `defaultedUserValue`, not `effective` + +The audit caught the first draft calling `configured ?? default` the *effective* +value. It is not. `003` §1 lists eight layers above the user file — profile-v2, +project config, CLI `-c`, thread layers, MDM — any of which can win. + +opencodex reads one file, so it can only report what that file says. The field +is named for what it actually is, and the UI says "이 파일의 설정" rather than +claiming the running Codex agrees. `003` §6's managed-override notice is +**deferred**: promising override detection without a read path would be the same +overclaim in a different place. + +## Key allowlist — fixed, never computed + +```ts +const TOGGLE_KEYS: Record = { + permissions: { table: null, key: "include_permissions_instructions" }, + collaboration: { table: null, key: "include_collaboration_mode_instructions" }, + environment: { table: null, key: "include_environment_context" }, + apps: { table: null, key: "include_apps_instructions" }, + skills: { table: "skills", key: "include_instructions" }, +}; +``` + +`003` §5: an unknown key is silently ignored in normal mode and a hard startup +error under `--strict-config`. A fixed table means the GUI can never emit a key +it did not intend. `setToggle` rejects any id outside this map before +touching the file. + +## Read + +1. Resolve the path exactly as `features.ts:58-67` does — `CODEX_HOME` at call + time, `~` expansion, `realpathSync.native` when it resolves. +2. Missing file → `configExists: false`, `readable: true`, every toggle + `userFileValue: null`. **Writes are allowed and create the file** — see + "First write" below. The audit was right that a disabled switch plus a + "created on first change" note is a contradiction. +3. Unreadable → `readable: false` and every write refused. +4. For each of the five keys, scan the correct scope: root keys only outside any + `[table]` header; `skills.include_instructions` only inside `[skills]`. + Reuse the table-body bounding from `features.ts:269-290`. +5. **Absent key means `configured: null`, never `false`.** `001` §6 — this + surface is four months old and still moving; absence is unknown, not off. +6. Custom layers come from `opencodex-prompt.json`, never from parsing + `developer_instructions`. Determine `developerInstructionsOwned` by the + marker-adjacency rule. +7. Read `model_instructions_file` for the read-only warning row. +8. Compute `revision`. + +## First write + +When `config.toml` is absent, the first mutation creates it: + +- `mkdir -p` the parent with mode `0700`, matching how Codex itself treats + `$CODEX_HOME`; +- write a minimal file containing only the marker and the key being set; +- `0600` on the file — it sits beside `auth.json`. + +A missing file is a first run, not an error state. `040` therefore renders live +switches, not disabled ones. + +## File modes — every content-bearing file, not just config.toml + +Custom layer bodies are user prose that may contain anything the user considers +private. They land in four places besides `config.toml`, and an earlier draft +specified a mode for none of them: + +| File | Mode | Why | +|---|---|---| +| `opencodex-prompt.json` | `0600` | holds every layer body | +| `opencodex-prompt.journal` | `0600` | holds pre- and post-images of both files | +| `opencodex-prompt.salvage-*.txt` | `0600` | a copy of the projected prompt | +| `opencodex-prompt.lock*` | `0600` | pid and token metadata | + +Temporary files inherit the same mode **at creation**, not by a later `chmod` — +a window where a body is world-readable is still a disclosure. Quarantined lock +files keep their mode through the rename. + +Windows has no POSIX modes; the repository's existing ACL hardening path is used +instead, and the mode assertions are POSIX-only in tests. + +## Write + +Same discipline as `features.ts:248-310`: + +- refuse unreadable input rather than creating a fresh file over it; +- `dominantEol` before, `applyEol` after; +- edit the line array, never re-serialize the document; +- confine matching to the owning table body; +- idempotent — equal value returns `changed: false` and writes nothing; +- `atomicWriteFile` only when something changed. + +Root-key insertion goes at the document top, before the first `[table]` header, +because a root key placed after one belongs to that table. `inject.ts:162` +already establishes the top-of-document convention. + +`[skills]` is created only when setting the skills layer and the table is +absent, appended at end of document. + +## Storage — redesigned after audit + +The first draft fenced layer bodies inside the `developer_instructions` TOML +string and kept disabled bodies in a sidecar JSON. An independent audit killed +it on four counts, all correct: + +- **TOML encoding.** "Body is verbatim" is false inside a multiline basic + string. A body containing `"""` terminates the value; a backslash is an escape. + Arbitrary user prose would produce malformed TOML — and `003` §4 shows Codex + cannot then parse the file at all. +- **Fence collision.** A body containing `# <<< ocx-layer:...` splits or steals + its own block. +- **Two-file reconciliation.** Presence/absence rules do not say which side wins + for body, title, or order, and deleting the JSON silently loses every disabled + body. +- **Concurrency.** Two GUI tabs, or Codex writing between our read and write, + lose updates. Atomic rename prevents torn bytes, not stale overwrites. + +### The fix: one owned file, one generated key + +**`$CODEX_HOME/opencodex-prompt.json` is the single source of truth** for custom +layers — every layer, enabled or not, with body, title, and order. + +`config.toml` receives exactly one generated root key, emitted by our own +three-rule encoder over a restricted character set (§Why no prompt text goes +into config.toml at all): + +```toml +# Auto-injected by opencodex +developer_instructions = "...properly escaped composition of enabled layers..." +``` + +Consequences that dissolve three blockers at once: + +- **No fences.** Bodies are joined with `\n\n` and never carry structure that + has to survive a round trip through TOML. The value is write-only from our + side; we never parse it back to recover layer identity. +- **Encoding is total.** Three escapes — `"`, `\`, newline — over a character + set that excludes everything ambiguous. Verification is a byte comparison, not + a reparse, because no TOML parser we could run is trustworthy enough to be the + judge (measured defects in `Bun.TOML` are recorded below). +- **Reconciliation disappears.** There is exactly one authority. `config.toml` + is a *projection*, never a source. + +### Why no prompt text goes into config.toml at all + +Round 2 of the audit rejected the `smol-toml` plan on six counts, and a live +experiment on this machine settled the question harder than the audit could. + +**Measured on Bun 1.3.14, `Bun.TOML.parse`:** + +| Input | Decodes to | Correct? | +|---|---|---| +| `"a\tb"` | `a\fb` | **no — tab becomes form-feed** | +| `"a\fb"` | `a\tb` | **no — form-feed becomes tab** | +| `"a\u0007b"` | `SyntaxError` | **no — valid TOML rejected** | +| `'''\nx'''` | `\nx` | **no — spec requires trimming the first newline** | + +`\n`, `\r`, `\b`, `\\`, `\"`, `\uXXXX` for printable code points, and non-BMP +text all round-trip correctly. But three defects are fatal to a verify-by-reparse +design: two escapes are transposed, one legal escape is refused, and multi-line +literal trimming is not implemented. + +The consequence is not "pick a different parser". It is that **we cannot verify +what Codex will read.** Codex parses with Rust `toml_edit`; we would verify with +Bun or with a JS library. An encoding tuned to satisfy our verifier is exactly +the encoding that can diverge in Codex's. A reparse check against a parser with +known-transposed escapes is worse than no check, because it reports success. + +Adding `smol-toml` does not fix this either. The audit was right on every +procedural count — it is BSD-3-Clause and not MIT as an earlier draft claimed, +it is absent from `package.json` and `bun.lock`, its `parse()` returns values +and not source spans so the "locate the exact value span" claim was unsupported, +and a new production dependency triggers the security review that +`AGENTS.md` requires and no phase owned. + +### The design that removes the problem + +**No user-authored prose is ever written into `config.toml`.** + +Custom layers live in `$CODEX_HOME/opencodex-prompt.md` — a plain UTF-8 markdown +file that opencodex owns outright. `config.toml` receives one generated line: + +```toml +# Auto-injected by opencodex +model_instructions_file = "~/.codex/opencodex-prompt.md" +``` + +No — see the next paragraph. That key replaces the base prompt (`002` §3), which +is the thing this unit exists to avoid. + +`developer_instructions` accepts only a string, so an external file is not an +option for it. Therefore the composed value **must** be encoded into TOML, and +the encoding must be one whose correctness does not depend on any parser we +control. + +**Resolution: restrict the writable body character set.** + +"Printable Unicode" is not executable, as the audit noted. The rule is defined +over **Unicode scalar values**, not UTF-16 code units: + +| Input | Handling | +|---|---| +| U+0009 tab | normalized to four spaces | +| CRLF, lone CR | normalized to LF | +| U+000A newline | accepted, encoded as `\n` | +| other C0 controls, U+007F DEL | **rejected** with position | +| C1 controls U+0080–U+009F | **rejected** — invisible and rarely intentional | +| **unpaired surrogate** | **rejected** — not a scalar value; UTF-8 encoding would substitute U+FFFD and silently alter the prompt | +| U+2028, U+2029 | accepted; they are not TOML line terminators and cannot end a basic string | +| everything else, incl. non-BMP | accepted verbatim | + +Validation iterates code points, so positions are reported as **code-point +indices** consistently across the API, the linter, and the editor. Size caps are +measured in **UTF-8 bytes after normalization** — tab expansion can grow a body, +so checking before normalization would let an oversized value through. + +Within that set, TOML basic-string encoding is total: escape `"` and `\`, emit +`\n` for newline, pass everything else through. Three escapes, all unambiguous, +none in the defective set. `\r` never appears because CRLF is normalized away. + +Verification is then a **byte-level** assertion rather than a semantic one: the +emitted line must equal `key = "` + escaped + `"`, and re-reading the file must +yield that exact line. No TOML parser is involved on the write path, so no +parser's defects can hide a divergence. + +**Byte equality is not the same as Rust agreeing with us**, which the audit +rightly pressed on. A hand-written grammar matcher tests the encoder against the +encoder's own assumptions. So WP1 also carries **one** independent proof: a +checked-in fixture of generated lines covering every accepted character class, +parsed by the real `toml_edit` through a tiny Rust test in the pinned upstream +checkout, with the decoded values committed as a golden file. It runs on demand, +not in the GUI suite, and it is the only thing that settles what Codex actually +reads. + +This costs the user the ability to put a NUL or a bell character in a prompt. +That is not a real loss, and it buys a guarantee that a dependency plus a +reparse could not. + +### Canonical physical form + +Audit blocker 5 asked for one canonical representation so replacement is a known +edit rather than a span search. It is exactly this, always: + +``` + +developer_instructions = "" +``` + +Two lines, at the top of the document, above the first `[table]`. The value is +always a single-line basic string — never multi-line, never literal. Replacement +is: find the marker, replace the following line. If the line after the marker +does not match `/^developer_instructions = "/`, we do not own it and refuse. + +The five boolean toggles keep the `features.ts:248-310` scoped line edit. +Booleans need no escaping at all. + +### Ownership, and the takeover flow + +`developer_instructions` is ours only when the immediately preceding line is +`OCX_SECTION_MARKER` and the line itself matches the canonical form — the +adjacency rule from `injected-marker.ts:53-60`, tightened by a shape check. + +- **Marker present, canonical shape** → owned; rewrite freely. +- **Marker present, shape differs** → refuse. Something edited our line into a + form our replacement rule does not cover. +- **Marker absent, key present** → externally authored. Refuse to write, and + offer the takeover below. + +We cannot safely rewrite a value whose string form we did not choose: it may be +literal, multi-line, or dotted, and `Bun.TOML.parse` cannot be trusted to decode +it (see §Why no prompt text goes into config.toml at all). Refusing is honest. + +**But refusing alone is a dead end**, which is what audit blocker 5 objected to +in round 2 — the earlier answer was "the user clears it themselves", i.e. delete +your existing instructions by hand. That is not a feature. + +**Takeover (`POST /api/codex-prompt/adopt`):** + +1. read the raw source line +2. **decode it** with the inverse of our own encoder (below) — an earlier draft + stored the raw line as the body, which an audit correctly called a semantics + change: `"hello\nworld"` would have been imported as those twelve literal + characters rather than two lines +3. show the user **both** the original source line and the exact decoded body + that will be imported, plus a copy button +4. on explicit confirmation: write the decoded body into + `opencodex-prompt.json` as one enabled layer titled "Imported from + config.toml", and replace the original lines with the canonical owned form, + through the journal transaction every other write uses + +**The decoder is deliberately narrow.** It accepts a single-line basic string +containing only `\"`, `\\`, and `\n` — precisely the three escapes our encoder +emits. Anything else is refused: + +| Input | Result | +|---|---| +| `\t`, `\f`, `\b`, `\r` | refused — `Bun.TOML` transposes two of these, so we will not guess | +| `\uXXXX` | refused — decoding it correctly is exactly the ambiguity we removed | +| multi-line basic or literal string | refused | +| unterminated or unbalanced quotes | refused | + +Refusal is `adopt_unsupported_form` with the file path and line number. That is +a narrow dead end that names where to look, and it is honest about the reason: +we do not have a parser we trust for the general case. + +**The decoded text then goes through the ordinary pipeline, in this order:** + +1. decode with the narrow decoder → `adopt_unsupported_form` on refusal +2. normalize: tab → four spaces, CRLF and lone CR → LF +3. validate scalars: reject unpaired surrogates, C0, DEL, C1 → `invalid_characters` + with a code-point position +4. enforce the 64 KiB body cap **after** normalization, in UTF-8 bytes, and the + 128 KiB composed cap against the layers that already exist +5. preview **the post-normalization body** — the exact bytes that will be + committed + +Step 5 matters: an earlier draft previewed the decoded text and committed the +normalized text, so a body containing tabs would have been shown one way and +saved another. Preview and commit must be the same string. + +The same five steps run for `owned-malformed` re-adoption. + +### Malformed owned lines + +Marker present, shape non-canonical. The audit flagged this as a new dead end, +and it was — Adopt covered only the marker-absent case, and Repair only drift. + +It is now its own state, `drift: "owned-malformed"`, with the same treatment as +Adopt: show the raw line, offer a copy, and on confirmation either re-adopt it +through the narrow decoder or replace it outright with an empty owned line if +the user prefers. A user cannot be locked out by reformatting a line we +generated. + +### Revision — hashes the edit base, not a summary of it + +Audit blocker 4 found the first revision covering too little: removing the +marker while leaving the value unchanged produced an identical hash, so an +ownership change was invisible. + +`revision` is now SHA-256 over the **complete bytes** of both files plus their +existence flags: + +``` +sha256( "cfg:" + (configExists ? configBytes : "\0absent") + "\n" + + "store:" + (storeExists ? storeBytes : "\0absent") ) +``` + +Hashing whole bytes rather than extracted values covers marker presence, key +position, reformatting, comment changes, and file creation or deletion in one +construct. It is also the exact base the edit is computed from, which is what +makes the compare-then-write meaningful. + +### The transaction + +Blocker 2 was correct and serious: JSON-first meant a failed request had already +mutated the source of truth. Blocker 3 was correct that "the next read +reprojects" made GET a mutating operation. + +Both are fixed by a journal, and by never letting a read write. + +**Files:** + +| Path | Role | +|---|---| +| `opencodex-prompt.json` | source of truth for custom layers | +| `opencodex-prompt.journal` | present only during a mutation | +| `config.toml` | Codex's file; carries the generated projection | + +**Write, under an advisory lock (below):** + +1. acquire the lock; run recovery first if a journal exists +2. re-read both files; compare `revision` → `stale_revision` on mismatch +3. write the journal envelope (below) and durably rename it into place. + **The journal is prepared intent. It is not a commit.** +4. re-verify `config.toml` bytes against the edit base, then write it atomically +5. re-verify `opencodex-prompt.json` bytes against the edit base, then write it + atomically +6. **verify both targets now hash to the post-image**; only then delete the + journal — that deletion is the commit +7. release + +Steps 4 and 5 each re-verify **their own** target immediately before its rename. +An earlier draft guarded only `config.toml`, which left the store writable by a +third party between step 2 and step 5 — the audit was right that we would then +overwrite it. A mismatch at either point aborts and rolls back. + +Step 6 compares **complete bytes**, not just the presence of our two lines. +Another writer could change an unrelated key while leaving our lines intact, and +a narrow check would report success over their edit. Any target that matches +neither image is `write_superseded`, and the journal is **not** deleted. + +**Rollback** applies the same pre/post/neither classification per target: a +target matching the post-image is restored to its pre-image, a target matching +the pre-image is already correct, and a target matching neither stops everything +with `recovery_required`. Rollback never writes over a file it does not +recognise. + +**Recovery — at service start and at lock acquisition, never in a GET.** + +Because the journal is prepared intent and commit is its deletion, **a journal +found on disk means the transaction never committed.** Recovery therefore rolls +*back*, never forward. An earlier draft rolled forward from the post-image while +simultaneously claiming a failed request changes nothing; the audit found the +two rules cannot both hold, and this is the one that survives. + +An earlier draft said "if either target differs from the post-image, rewrite +both from the post-image". An audit found that destroys legitimate work: crash +after writing config.toml, user or Codex then edits config.toml, recovery sees a +mismatch and overwrites their edit with a stale post-image. + +Recovery classifies **each target independently** against both recorded hashes: + +| Target matches | Meaning | Action | +|---|---|---| +| **all targets** post-image | writes finished, step 6 never ran | delete journal — commit it | +| post-image (mixed) | partially applied | **restore to pre-image** | +| pre-image | never applied | leave it | +| **neither** | **externally modified** | **stop; `recovery_required`** | + +A single unrecognised target aborts the whole recovery before anything is +written. We never repair one file while another carries a stranger's edit. + +The all-post case is the one exception to rolling back, and it is not a +roll-forward: both files already hold exactly the intended result, so the only +missing step was deleting the journal. + +### Journal envelope + +"Restore from the pre-image if it is intact" is not implementable when the +pre-image lives inside the same damaged document — the audit was right. The +journal is therefore a **checksummed envelope**: + +``` +line 1: ocx-journal-v1 +line 2: {"preConfig":"...","postConfig":"...","preStore":"...","postStore":"...", + "preConfigExists":true,...} +``` + +On recovery the checksum is verified first. **Any journal that fails its +checksum, or is truncated, is `recovery_required` — nothing is read from it and +nothing is written.** Partial recovery from a damaged transaction record is +exactly the operation that should never be attempted on a user's config. + +Atomic temp-write plus `fsync` and rename makes a torn journal exceptional; when +it happens anyway, failing closed is the whole point of having the record. + +`recovery_required` names both paths and the journal, and blocks mutations until +the user resolves it. An honest stop beats best-effort repair on a file the user +also edits by hand. + +### Commit point — one state machine, not two + +The same audit found the draft mixing two models: journal-rename-is-commit +(implying roll-forward) alongside "a step-5 failure restores the pre-image" +(implying the journal is only intent). Both cannot hold. + +**Chosen: the journal is prepared intent. Commit is when both targets match the +post-image.** + +- Failure before both targets are written → roll **back** to the pre-image, and + the API reports a plain error. Nothing changed. +- Failure after both match → the write succeeded; recovery only deletes the + journal. +- The unrecognised-target case above overrides everything and stops. + +This is the model that makes "a failed request changes nothing" true, which is +what audit blocker 2 required. Roll-forward-after-journal would have made a +failed request silently succeed later. + +### Durability + +`fsync` on a temp file does not make its directory entry durable. Each step is: +write temp → `fsync` file → rename → **`fsync` the parent directory**. Journal +deletion is also followed by a directory `fsync` before completion is reported. + +On Windows, directory `fsync` is unavailable; `FlushFileBuffers` on the file plus +`MoveFileEx` with write-through is the documented fallback, and the +`030`-era Windows CI job covers it. Where neither is available, the journal is +still written first, so the worst case is a recovery that reports +`recovery_required` rather than one that loses data silently. + +### Reads never write + +`readPromptLayers` is pure. When it observes drift — a journal present, or +config.toml's projection disagreeing with the JSON — it reports +`drift: "journal-present" | "projection-stale" | null` and changes nothing. + +`020` surfaces drift as a state the GUI renders with an explicit **Repair** +action. Repair is a `POST`, revision-checked like any other mutation. An HTTP +GET must never modify a user's configuration, which is exactly what blocker 3 +said. + +### Missing store is not an empty store + +Blocker 3's sharpest case: JSON deleted while an owned, non-empty projection +still sits in config.toml. Treating that as "empty store" would make the next +write erase the active prompt and every saved body. + +Three distinct states, distinguished before anything is written: + +| Store | Owned projection | Meaning | Behavior | +|---|---|---|---| +| absent | absent | never used | normal first run | +| absent | **present and non-empty** | **store lost** | refuse writes, `drift: "store-missing"`, offer Repair | +| present | either | normal | normal | + +Repair from `store-missing` is **salvage, not reconstruction** — the audit was +right that the earlier wording oversold it. + +The projection holds one concatenated string of the enabled layers. It cannot +recover layer boundaries, ids, titles, row order, disabled layers, or their +bodies, and it cannot tell a `\n\n` that separated two layers from one that a +user typed. All of that is gone with the store. + +So salvage takes the whole projected text and offers it as **one new layer**, +with the exact body previewed and the losses listed explicitly before the user +confirms. + +The backup is written first, and "first" has to mean durable: a timestamp alone +can collide, and an unflushed backup is not a backup. It is created with +exclusive `wx` and a random suffix, mode `0600`, then `fsync`ed together with its +parent directory. **If the backup cannot be created or made durable, salvage +aborts** — a destructive operation whose safety net failed should not proceed. + +### Cross-process locking + +Blocker 4 was right that an in-process mutex protects only browser tabs behind +one service. A CLI invocation, a second service, or Codex itself can all write +the same file. + +`$CODEX_HOME/opencodex-prompt.lock` is an advisory lock created with `wx`, +carrying a random **token**, pid, and start time. Every opencodex writer — +service, CLI, route — takes it. + +Naive stale-breaking is racy, as the audit showed: A judges the lock stale, B +removes it and acquires its own, A then deletes *B's live lock* and both +proceed. Unlinking a path you did not verify is the bug. + +**Race-safe takeover:** + +1. read the lock; if its pid is live or it is younger than 10s, wait +2. otherwise `rename()` it to `opencodex-prompt.lock.stale-` — an + atomic operation exactly one contender can win +3. the winner creates the real lock with `wx` and deletes the quarantined file +4. a loser's rename fails with `ENOENT`; it returns to step 1 + +**Release deletes only a lock whose token still matches ours.** A token mismatch +means we were superseded: we do not delete, and we report `write_superseded` +rather than assuming the file is still ours. + +That covers opencodex against itself. **It does not cover Codex**, which knows +nothing about our lock. The residual window is between step 2's read and step 4's +rename. Two mitigations, and an honest limit: + +- immediately before the rename, re-hash `config.toml` and compare against the + edit base. A change aborts with `stale_revision`, nothing written. This shrinks + the window to the rename itself. +- after the rename, re-read and confirm the file contains our two lines. If it + does not, another writer won; report `write_superseded` rather than success. +- a race lost inside the rename cannot be prevented from user space. It is + detected on the next read as `projection-stale`, and it is recorded in `070` + as residual rather than claimed solved. + +## Tests — `tests/codex-prompt-layers.test.ts` + +Every case takes explicit temp paths (`004` §H: never resolve the real +`CODEX_HOME`). + +**Reading** +1. missing config → defaults, `configExists: false`, writes still permitted +2. unreadable config → `readable: false`, every write refused +3. absent key → `userFileValue: null`, `defaultedUserValue: true` +4. `include_apps_instructions = false` → both false +5. `[skills] include_instructions` read from its table, not root +6. a root-looking key **inside** another table is not read as the root key + +**Boolean writes** +7. insert above the first `[table]` +8. replace in place, comments intact +9. idempotent → `changed: false`, file byte-identical +10. CRLF preserved +11. unrelated tables, comments, blank lines survive +12. unknown id rejected before any file access +13. first write creates file and parent with `0700`/`0600` + +**Encoding — byte-level, no parser involved** +14. body with `"` and `\` emits exactly `\"` and `\\`, byte-compared +15. body with `"""` is unremarkable — it is three escaped quotes on one line +16. tab normalizes to four spaces; CRLF normalizes to LF +17. control characters rejected with position, nothing written +18. non-BMP Unicode and combining marks pass through unescaped +19. a 64 KiB body produces one line of the expected length +20. **after every write, the file re-read byte-for-byte contains the exact + expected two lines** — the assertion that replaces reparse verification +21. **a golden fixture of the emitted line is checked against the TOML spec's + basic-string grammar by hand-written matcher**, not by `Bun.TOML` +21a. unpaired high surrogate → rejected with code-point position +21b. unpaired low surrogate → rejected +21c. U+007F DEL and a C1 control (U+0085) → rejected +21d. U+2028 / U+2029 accepted and pass through unescaped +21e. size caps measured in UTF-8 bytes **after** tab expansion +21f. **golden Rust fixture**: generated lines for every accepted character class + parsed by real `toml_edit`, decoded values matching a committed golden file + +Cases 20-21 exist because `Bun.TOML.parse` on Bun 1.3.14 transposes `\t`/`\f` +and rejects `\u0007` (measured; see §Why no prompt text goes into config.toml). +A test that verified through that parser would report success on a file Codex +might read differently. + +**Ownership and adoption** +22. marker + canonical line → owned, rewritten +23. marker + non-canonical line → refuse, file untouched +24. key without marker → refuse, `drift`/adopt offered, file untouched +25. adopt on a single-line basic string imports it and takes ownership +26. adopt on a multi-line or literal string is refused with path and line +26a. adopt normalizes tabs to four spaces and CRLF to LF +26b. adopt rejects a forbidden control with its code-point position +26c. adopt refuses a body that exceeds 64 KiB **after** normalization +26d. adopt refuses when the composed total would exceed 128 KiB +26e. **the previewed body is byte-identical to the committed body** +26f. `owned-malformed` re-adoption runs the same five steps +26g. `previewSalvage` returns a directory and creates no file +27. our marker survives an unrelated boolean write + +**Store, transaction, recovery** +28. custom layers round-trip through JSON +29. disabled layer stays in JSON, absent from the projection +30. all layers disabled → both generated lines removed +31. store absent + no owned projection → normal first run +32. **store absent + owned non-empty projection → `drift: "store-missing"`, + writes refused, salvage offers the projected text as one layer** +33. store malformed → `store_unreadable`, config.toml untouched +34. stale revision → refused, nothing written +35. revision changes when only the marker is removed (value identical) +36. revision changes when the config is deleted +37. journal present + **all** targets match post-image → journal deleted, state + kept (the writes finished; only the commit step was missing) +38. journal present + one target post, one pre → **both rolled back to + pre-image**, because a journal on disk means commit never happened +39. journal present + a target matches **neither** → `recovery_required`, + nothing written, the other target untouched +40. journal fails its checksum → `recovery_required`, nothing read from it +41. journal truncated → `recovery_required`, nothing written +41a. store externally modified between compare and its rename → abort, roll back +41b. store deleted between compare and its rename → abort, roll back +41c. config externally modified during rollback → `recovery_required` +41d. post-write byte comparison catches an unrelated key changed by another + writer → `write_superseded`, journal retained +41e. backup creation fails → salvage aborts, nothing destroyed +41f. every content-bearing file is created `0600` (POSIX only) +41. config write succeeds, store write fails → config restored, request errors, + **source of truth unchanged** +42. rollback itself fails → `recovery_required` naming both paths +43. config.toml modified between compare and rename → `stale_revision` +44. post-rename readback missing our lines → `write_superseded` +45. lock held by a live pid → second writer waits then refuses +46. lock held by a dead pid → broken after the timeout +46a. **A quarantines the stale lock; B acquires the real lock before A can + recreate it; A's `wx` fails; A retries from the top without deleting B's + lock and without entering the critical section** +46b. release with a mismatched token deletes nothing and reports + `write_superseded` +46c. two contenders quarantine simultaneously → exactly one rename succeeds +47. **`readPromptLayers` never writes**: a read against every drift state leaves + both files byte-identical + +Cases 20, 24, 32, 41, 43, and 47 are the data-protection set. Each is driven red +once before it is trusted. + +### Cross-platform + +The lock uses `wx` open and pid liveness; the journal uses `fsync` + rename. +Both behave differently enough on Windows that WP1's CI must run its suite on +Linux, macOS, and Windows — the three platforms `AGENTS.md` names. Path +separators and `realpathSync.native` on a WSL-visible `$CODEX_HOME` +(`home.ts:90-107`) are covered by existing fixtures. + +**Windows durability is a WP1 acceptance gate, not an assumption.** Directory +`fsync` is unavailable there, and the plan does not claim a Bun API for +`MoveFileEx` write-through because none is established. WP1 must determine what +Bun actually exposes and then either use it or **fail closed**: if +write-through cannot be guaranteed, the journal is still written first, and a +crash surfaces as `recovery_required` rather than as silent loss. A Windows test +must exercise that branch explicitly. WP1 does not land until this is settled +one way or the other and the answer is recorded here. + +### No new production dependency + +WP1 adds nothing to `package.json`. The audit's dependency-review blocker is +resolved by removing the dependency, not by scheduling a review: byte-level +encoding needs no TOML library, and `Bun.TOML.parse` is used only in tests, and +only where its measured defects do not apply. + +## Not in this phase + +No route, no GUI, no presets, no linter. WP1 ships a module and its tests. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/020_wp2_management_route.md b/devlog/_plan/260802_codex_set_prompt_composer/020_wp2_management_route.md new file mode 100644 index 0000000000..3211cb918d --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/020_wp2_management_route.md @@ -0,0 +1,198 @@ +# 020 — WP2: `/api/codex-prompt` + +New file: `src/server/management/codex-prompt-routes.ts`. +Registered in `src/server/management-api.ts`. +Shape from `sidebar-routes.ts:23-89`; GET/PUT semantics from +`agent-settings-routes.ts:127-178` (`004` §F). + +## Endpoints + +### `GET /api/codex-prompt` + +```jsonc +{ + "configPath": "~/.codex/config.toml", + "storePath": "~/.codex/opencodex-prompt.json", + "configExists": true, + "readable": true, + "developerInstructionsOwned": true, + "drift": null, + "revision": "sha256:...", + "inventory": [ + { "id": "base-instructions", "class": "base", + "key": null, "default": null, "order": 0 }, + { "id": "permissions", "class": "config-toggle", + "key": "include_permissions_instructions", "default": true, "order": 6 }, + { "id": "plugins", "class": "feature-gated", + "key": "features.plugins", "default": true, "order": 11 }, + { "id": "agents-md", "class": "runtime-conditional", + "key": null, "default": null, "order": 5 } + ], + "toggles": [ + { "id": "permissions", "key": "include_permissions_instructions", + "userFileValue": null, "defaultedUserValue": true, "default": true } + ], + "extensionLayersEnumerable": false, + "custom": [ + { "id": "a1b2c3", "title": "My house rules", "enabled": true, "body": "..." } + ], + "modelInstructionsFile": null +} +``` + +`inventory` **is** `LAYER_INVENTORY` from WP1, serialized. The audit found the +first draft inventing `locked` and `features` arrays the core module did not +export — two constants that would drift apart within a release. There is now one +definition, in WP1, and the route projects it. + +A row gets a switch **iff** `class === "config-toggle"`. Classes `base` and +`runtime-conditional` are the non-disableable set behind ask item 9; +`feature-gated` rows are configurable, but not from here. + +`extensionLayersEnumerable: false` is the honest statement of `001` class E: we +cannot list third-party extension layers, so we say so rather than implying the +inventory is exhaustive. + +No `effective` field exists. `010` explains why: opencodex reads one file out of +the eight layers in `003` §1, so it reports that file's value under a name that +says as much. + +### `PUT /api/codex-prompt/toggle` + +`{ "id": "apps", "enabled": false, "revision": "sha256:..." }` +→ `{ "ok": true, "changed": true, "snapshot": {...} }` + +- `id` not a `config-toggle` in `LAYER_INVENTORY` → `409 layer_not_toggleable`. + This covers classes A, C, D, and E in one rule, derived from the inventory + rather than a hand-maintained deny-list. +- `id` unknown entirely → `400 unknown_layer`. +- missing/stale `revision` → `409 stale_revision`. +- unreadable config → `409 config_unreadable`. + +The GUI never sends a locked id. The route refuses it anyway: a hand-rolled +request must not be able to do what the UI forbids. + +### `PUT /api/codex-prompt/custom` + +`{ "layers": [...], "revision": "sha256:..." }` — full replacement, order is +composition order. + +Refused with `409 developer_instructions_not_owned` when the key exists without +our marker (`010` §Ownership). The GUI offers **Adopt** — preview, copy, +confirm — rather than telling the user to edit the file by hand. + +Validation before any file access: + +| Rule | Response | +|---|---| +| `layers` not an array | `400 invalid_body` | +| > 32 layers | `400 too_many_layers` | +| id not `[a-z0-9]{6}` | `400 invalid_layer_id` | +| duplicate id | `400 duplicate_layer_id` | +| title empty, > 80 chars, or contains a newline | `400 invalid_title` | +| body > 64 KiB | `400 body_too_large` | +| composed total > 128 KiB | `400 composed_too_large` | +| control character in body | `400 invalid_characters` with position | + +The size caps are **opencodex policy**, not a Codex limit. `002` §3 records that +Codex validates nothing beyond readable-and-non-empty. The audit correctly +rejected an earlier draft that justified a cap with the 32 KiB AGENTS.md budget +— that budget governs project-doc loading and has nothing to do with +`developer_instructions`. The real justification is request cost and keeping a +hand-editable file hand-editable. + +Tabs and CRLF are normalized rather than rejected. Control characters are +refused: `010` records a measured `Bun.TOML.parse` defect that makes local +verification untrustworthy, so the encoding is restricted to a character set +whose escaping is total under three unambiguous rules. + +### `POST /api/codex-prompt/adopt` + +Takes ownership of an externally authored `developer_instructions`. Returns the +raw source line for preview when called with `{ "confirm": false }`, and +performs the import only on `{ "confirm": true, "revision": "..." }`. + +Refused with `409 adopt_unsupported_form` when the value is not a single-line +basic string, naming the file path and line number so the user can move it by +hand. `010` §Ownership explains why a broader extraction is not attempted. + +### `POST /api/codex-prompt/repair` + +The only endpoint that resolves a `drift` state. Revision-checked like any +mutation. GET never repairs anything — an HTTP GET must not modify a user's +configuration. + +`drift` is the canonical type from `010`: `"journal-present"`, +`"projection-stale"`, `"store-missing"`, `"owned-malformed"`, or `null`. An +earlier draft omitted `owned-malformed`, which would have left the GUI unable to +resolve the one state a user can reach by reformatting a line we generated. + +| drift | Action | Preview | +|---|---|---| +| `journal-present` | run recovery; `recovery_required` is terminal until resolved | — | +| `projection-stale` | re-project from the store | the resulting projection | +| `store-missing` | **salvage** the projected text as **one** layer | body + the enumerated losses + backup path | +| `owned-malformed` | `mode: "adopt"` re-adopts through the narrow decoder, `mode: "replace"` overwrites with an empty owned line | raw line + decoded body | + +`store-missing` is **salvage, not reconstruction**. `010` §Missing store +enumerates what cannot come back: layer boundaries, ids, titles, order, disabled +layers, and whether a `\n\n` was a separator or the user's own text. The preview +lists them and a backup is written before anything is destroyed. + +Every preview is a **GET-shaped read** performed by `previewSalvage` / +`previewAdopt`; the write happens only on a confirmed `POST` carrying a matching +revision. + +## Response echoes the snapshot + +Every mutating response returns the freshly re-read snapshot, so the GUI can +`setClientResourceData` with server truth instead of optimistic local state +(`004` §G, `client-resource.ts:464-482`). + +## Auth + +Nothing extra. `requireManagementAuth` already covers `/api/**` +(`server/index.ts:448-453`) and unsafe methods already require Origin + CSRF +(`management-auth.ts:246-266`). This is a local-config write, not an +account-identity action, so it does **not** need the `agent_consent_required` +treatment that `/api/github/star` carries. + +## Privacy + +The snapshot carries file paths and user-authored prompt text. It carries no +token, key, or account identifier. Layer bodies are user content the user just +typed into this same GUI — echoing them back is not disclosure. Nothing here is +logged: `privacy:scan` stays green because the route never writes request +bodies to any log sink. + +## Tests — `tests/codex-prompt-route.test.ts` + +Harness from `sidebar-routes.test.ts:18-58`: a helper building Host-bearing +requests, dispatching `handleManagementAPI`, restoring seams in `finally`. The +WP1 module is injected so **no test touches the real `CODEX_HOME`**. + +1. GET returns the snapshot with the full inventory +2. GET on a missing config → defaults, `configExists: false` +3. PUT toggle flips a value and echoes the new snapshot +4. unknown id → 400 +5. **every non-`config-toggle` inventory id → 409, writer never called** — + table-driven over `LAYER_INVENTORY`, so a new upstream layer is covered the + day it is added +6. **contract test: every inventory id has exactly one class, and every + `config-toggle` has a non-null key** — the partition guard +7. PUT custom round-trips order +8. each validation rule, one case each +9. stale revision → 409 on every mutating verb +10. unowned `developer_instructions` → 409 on custom; toggles still work +11. unreadable config → 409 on all mutations +12. hostile Origin rejected (mirrors `management-client-config-route.test.ts:240`) +13. unhandled path returns `null` so the chain continues +14. adopt preview returns the raw line and **writes nothing** +15. adopt without `confirm: true` writes nothing +16. adopt on an unsupported form → 409 with path and line +17. every `drift` state is reported by GET and **GET writes nothing** +18. repair requires a matching revision +19. control-character body → 400 with position + +Cases 5 and 6 are load-bearing: 5 proves ask item 9 at the API boundary, 6 +prevents the inventory drift the audit found in the first draft. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/030_wp3_page_shell.md b/devlog/_plan/260802_codex_set_prompt_composer/030_wp3_page_shell.md new file mode 100644 index 0000000000..85b04b2864 --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/030_wp3_page_shell.md @@ -0,0 +1,135 @@ +# 030 — WP3: Codex Auth → Codex Set + +The rename, the two-panel shell, and a **working** Prompt panel showing the five +toggle rows. WP4 then adds the other four layer classes and the dialog. + +The first draft ended here with an empty placeholder and deferred the +loading-contract migration to WP4. An audit called that a forward dependency, +correctly: a phase that ships a panel saying "nothing here yet" cannot be +verified against anything. + +## Route rename + +Per `004` §A, edit in one commit: + +| File | Change | +|---|---| +| `app-routing.ts:5-15` | `Page` union: `"codex-auth"` → `"codex-set"` | +| `app-routing.ts:20-30` | `VALID_PAGES` likewise | +| `app-routing.ts:85-93` | legacy table gains `codex-auth` → `codex-set` | +| `App.tsx:31-41` | `PAGE_TKEY`: `"codex-set": "nav.codexSet"` | +| `App.tsx:50-52` | `NAV` entry, position unchanged (second) | +| `App.tsx:307-316` | render `` | +| `providers-page-utils.ts:19` | deep link → `#codex-set` | + +The legacy redirect is not optional. `#codex-auth` is a bookmarkable URL that +has shipped, and `use-app-route-state.ts:44` reads the initial page straight +from the hash — without the entry, an old bookmark lands on a 404-ish unknown +page. + +**`/api/codex-auth/*` does not move.** `004` §C: about a dozen test files bind +to that namespace and renaming it buys nothing. + +## Files + +``` +gui/src/pages/CodexSet.tsx (new — shell, ~120 lines) +gui/src/pages/codex-set-multiauth.tsx (new — today's CodexAuth body, moved) +gui/src/pages/codex-set-prompt.tsx (new — toggle rows + data surface) +gui/src/pages/CodexAuth.tsx (deleted) +``` + +## The Prompt panel in this phase + +`useDataSurface("codex-prompt:" + apiBase, ...)` over `GET /api/codex-prompt` +(`004` §G). Renders only `class === "config-toggle"` rows — five switches, each +PUTting with the snapshot revision and republishing the echoed snapshot via +`setClientResourceData`. + +`CodexSet` joins `MIGRATED` in `page-loading-contract.test.tsx:25-39` **in this +phase**, since it now has a real data surface. + +Rows for the other classes are deliberately absent, not stubbed. A phase that +renders half a taxonomy invites a reader to assume the rest does not exist. + +`codex-set-multiauth.tsx` is a **move, not a rewrite**. Everything +`CodexAuth.tsx:93-177` owns — the `/api/config` fetch, the session cache key, +the 30s poll, provider recovery, the banner — moves verbatim and keeps +delegating to `CodexAccountPool`. The session cache key +`ocx.codex-auth.config.v1:${apiBase}` stays as-is: renaming it would discard +every user's warm cache for no benefit. + +## Shell + +Modeled on `Logs.tsx:408-425` (`004` §B): + +```tsx +const tab = subRoute === "prompt" ? "prompt" : "multiauth"; +// hash: #codex-set | #codex-set/prompt +// Prompt lazy-mounts on first visit, stays mounted after (Logs.tsx:551) +``` + +Reuses `.page-tabs` / `.page-tab` from `styles.css:417-423`. No new CSS in this +phase. + +Tab switching pushes history so back/forward work, matching +`use-app-route-state.ts:39-90`. + +## i18n + +New `codexSet.*` namespace. English first (`en.ts` is authoritative, +`TKey = keyof typeof en` at `en.ts:1662`), then the same keys in all five other +locales **in the same commit** — `Record` makes a gap a typecheck +failure (`004` §D). + +WP3 keys: + +``` +nav.codexSet "Codex Set" / "Codex 설정" +codexSet.tab.multiauth "Multi-auth" / "다중 인증" +codexSet.tab.prompt "Prompt" / "프롬프트" +codexSet.prompt.title "Prompt layers" / "프롬프트 레이어" +codexSet.prompt.timing → see below +``` + +`codexSet.prompt.timing` is fixed copy from `003` §3: + +- en: "Applies to newly started sessions. Running sessions keep their current + prompt settings." +- ko: "새 세션부터 적용됩니다. 실행 중인 세션은 현재 설정을 유지합니다." + +Not "즉시 적용", not "재시작 필요" — neither is proven, and `003` §3 leaves the +frontend reload path UNKNOWN. + +The 130 existing `codexAuth.*` keys are untouched. + +## Tests + +Update: + +- `gui/tests/sidebar-codex-auth.test.ts:14-23` → rename file to + `sidebar-codex-set.test.ts`, assert the new id and that the legacy id still + resolves. +- `gui/tests/dashboard-tabs.test.ts:45-52` → `codex-set` second. + +Add `gui/tests/codex-set-shell.test.tsx`: + +1. `#codex-set` renders Multi-auth, not Prompt +2. `#codex-set/prompt` renders Prompt +3. `#codex-auth` **redirects** to `#codex-set` — the bookmark guarantee +4. Prompt does not mount until first visited +5. Prompt stays mounted after switching away +6. tab switch pushes history; back returns to the previous tab +7. the timing string renders and contains neither "즉시" nor "재시작" +8. the five toggle rows render from the inventory +9. toggling PUTs once, with the current revision +10. a stale-revision 409 re-reads instead of retrying blindly +11. `configExists: false` leaves switches **live**, not disabled + +Case 7 pins `003` §3 into a test so a later copy edit cannot quietly promise +something the runtime does not do. Case 11 pins the audit's blocker-9 fix. + +## Verification + +`bun run typecheck` (catches any locale gap), `bun run test`, +`bun run lint:gui`. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/040_wp4_layer_rows.md b/devlog/_plan/260802_codex_set_prompt_composer/040_wp4_layer_rows.md new file mode 100644 index 0000000000..996d0aad62 --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/040_wp4_layer_rows.md @@ -0,0 +1,123 @@ +# 040 — WP4: the full layer taxonomy + +WP3 shipped the five `config-toggle` rows. WP4 adds the remaining four classes +from `001` §4 — `base`, `feature-gated`, `runtime-conditional`, +`extension-unknown` — and the read-only dialog. Custom layers are WP5. + +## Files + +``` +gui/src/components/codex-set/PromptLayerPanel.tsx (new) +gui/src/components/codex-set/PromptLayerRow.tsx (new) +gui/src/components/codex-set/PromptLayerDialog.tsx (new) +gui/src/components/codex-set/prompt-layer-copy.ts (new — id → i18n key map) +gui/src/styles-codex-set.css (new) +``` + +Row and dialog are modeled on `ClientConfigRow.tsx` / `ClientConfigDialog.tsx`, +which already solve "compact row, detail behind a dialog" in this codebase +(`004` §Reuse map). + +## Data + +The WP3 data surface is reused unchanged. No polling: the file changes when the +user changes it, and a 30s timer would fight the editor for no gain. + +## Row kinds come from the class, not a list + +There is no `rowKind()` heuristic. The server sends +`inventory[].class` (`020`), which is `LAYER_INVENTORY` from WP1, which is +`001` §4. One definition, three consumers: + +```tsx +const kind = descriptor.class; // that is the whole mapping +``` + +| Class | Renders | +|---|---| +| `config-toggle` | a real switch (shipped in WP3) | +| `base` | lock glyph, "always on", **no switch element at all** | +| `runtime-conditional` | lock glyph, the condition it follows, no switch | +| `feature-gated` | gear glyph, governing key, link to its owner, no switch | +| `extension-unknown` | rendered as a count, never as rows | + +The first draft derived row kind from a server `locked` array that WP1 never +exported, and an audit found it also mis-listed Plugins as non-disableable. +Deriving from `class` removes both failure modes: the taxonomy is the contract. + +Precise wording matters in the UI copy too. `base` and `runtime-conditional` +rows have **no off-switch anywhere in Codex**. `feature-gated` rows *are* +disableable — through `[features]`, not through this page. An earlier draft +applied the stronger sentence to all non-switch rows, which is false for +feature-gated ones and would mislead a user into thinking a setting does not +exist. + +"No switch element at all" is literal: no ``, +no greyed toggle. `005` explains the reasoning — a disabled control claims the +capability exists and is temporarily unavailable, which is false. `001` §4 +proves these layers have no off-switch anywhere in Codex. + +## Ordering + +Assembly order from `001` §1, so the list reads the way the prompt is built. +Skills carries a footnote that its position among extensions is +registration-dependent (`001` ordering caveat). + +## Dialog — read-only + +Ask item 8: built-in layers open a popup that cannot be edited. Contents: + +- what the layer does, in one sentence +- its class, and for classes B/C the exact key and its TOML position +- for class B: default and this file's value +- for class D: the runtime condition that decides emission +- Copy button for the key name + +**No rendered prompt text.** The first draft promised to show each layer's +actual content; an audit noted nothing produces it. Codex exposes no API for +rendered layer bodies, and reconstructing them would mean reimplementing +`world_state.rs` against a moving target (`001` §6). The dialog explains the +layer and names its key — the honest scope. + +No textarea, no Save. Escape closes and returns focus, matching +`client-config-panel.test.tsx:204-222`. + +## Failure states + +From `005`: + +- `configExists: false` → defaults, switches **live**; the first write creates + the file (`010` §First write). +- `readable: false` → writes refused with an explanation. +- `developerInstructionsOwned: false` → toggles still work; the custom group + explains the key is externally managed. +- PUT rejected → revert the row to the server snapshot and surface the error. + Never leave a switch showing a state the file does not have. + +## Tests — `gui/tests/codex-set-prompt-layers.test.tsx` + +Harness from `client-config-panel.test.tsx:86-152`. + +1. every inventory entry renders a row, in `order` +2. **table-driven over the inventory: every non-`config-toggle` class renders + no switch element** — query returns null for each +3. a `feature-gated` row names its governing key and links out +4. a `runtime-conditional` row states its condition +5. `extension-unknown` renders as a count, never as rows +6. a rejected PUT reverts the row +7. dialog opens read-only: no textarea, no Save +8. Escape closes and returns focus +9. the dialog claims no rendered prompt text +10. `readable: false` refuses writes +11. cold load renders the skeleton; refresh keeps rows visible + +Case 2 is ask item 9 at the rendering layer; `020` case 5 is the same guarantee +at the API layer. Both are required — one without the other is a UI that merely +looks safe, or an API nobody exercises. Driving it from the inventory means a +new upstream layer is covered the day WP1 lists it. + +## Styling + +Design tokens only, no gradients — the constraint `260802_api_tab_client_connect_simplify` +records at `styles-apikeys-workspace.css:1-10`. Rows reuse existing row/switch +patterns; the new stylesheet only carries what the layer list genuinely needs. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/050_wp5_custom_layers.md b/devlog/_plan/260802_codex_set_prompt_composer/050_wp5_custom_layers.md new file mode 100644 index 0000000000..b36eee81fa --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/050_wp5_custom_layers.md @@ -0,0 +1,148 @@ +# 050 — WP5: custom layers + +The `+` button, the editable dialog, delete, reorder, **and the compatibility +linter**. This is ask items 5, 6, 7 and the half of item 4 that applies to +user-authored rows. + +The linter moved here from WP6. An audit found WP5 rendering findings from an +API WP6 owned — a forward dependency. The linter is a pure function with no +dependency on presets, so it belongs in the phase that consumes it; WP6 becomes +presets alone. + +## Files + +``` +gui/src/components/codex-set/CustomLayerRow.tsx (new) +gui/src/components/codex-set/CustomLayerDialog.tsx (new) +gui/src/components/codex-set/custom-layer-state.ts (new — reducer) +gui/src/components/codex-set/prompt-lint.ts (new — moved from WP6) +``` + +## Where the text goes + +`developer_instructions`, composed by WP1. **Not** `model_instructions_file` — +`002` §3 proves that key replaces the entire base prompt, so wiring `+` to it +would delete Codex's own instructions on first save. `000` records this as the +deliberate deviation from the literal ask. + +`010` §Storage settles the mechanics: `opencodex-prompt.json` is the single +source of truth for every layer including disabled bodies, and +`developer_instructions` is a generated projection of the enabled subset, always +in the canonical two-line form. One authority, no reconciliation. + +When the key exists without our marker, the custom group offers **Adopt** +(`010` §Ownership): the existing text is shown verbatim, copied if the user +wants, and imported as a custom layer only on explicit confirmation. `+` stays +hidden until ownership is settled, but the user is never told to go delete their +own instructions by hand. + +## Row + +``` + My house rules [ ●─ ] on [edit] [×] +``` + +Switch, edit, delete — all three, unlike built-ins which never get delete +(ask item 6). Drag handle for reorder; order is composition order. + +## Dialog — editable + +Ask item 7. Title input, body textarea, live compatibility warnings, Save, +Cancel. Same dialog scaffolding as WP4's read-only one; the difference is the +editor and the Save action, not a separate component family. + +Escape cancels, discards, returns focus. When the body is dirty, Escape asks +first — a textarea the user has typed into is not the same as a read-only +dialog they glanced at. + +## The `+` flow + +WP5 ships a single action: `+ Add layer` opens an empty editor. **No preset +submenu exists yet** — an empty menu is worse than no menu, and the audit was +right that shipping one is a forward dependency on WP6. WP6 adds the submenu +together with the presets that fill it. + +## Client-side validation + +Mirrors `020`'s server rules so the user sees the problem while typing rather +than on Save: + +| Rule | Message | +|---|---| +| empty title | "제목을 입력하세요" | +| title > 80 chars | count shown, Save disabled | +| body > 64 KiB | size shown, Save disabled | +| composed total > 128 KiB | which layers overflow | +| > 32 layers | `+` disabled with the reason | +| control character in body | the offending position, Save disabled | + +Tabs and CRLF are **normalized, not rejected** — four spaces and LF — with a +quiet note the first time it happens. `010` explains why the character set is +restricted: a measured `Bun.TOML.parse` defect makes local verification +untrustworthy, so the encoding is kept to three unambiguous escapes instead. + +The server still enforces every one of these (`020`). Client validation is +courtesy; the API is the boundary. + +## The linter + +`prompt-lint.ts` — pure, no I/O: + +```ts +export function lintPromptLayer(body: string): LintFinding[]; +``` + +Rules and their evidence live in `060`; the implementation ships here. Findings +render inline with the offending span highlighted, **as warnings, never +blocks**. A user who deliberately wants to override Codex's identity may; they +just should not do it by accident. + +## Reorder and revisions + +Order is composition order. Reordering PUTs the full list — `020`'s endpoint is +full-replacement precisely so ordering needs no separate verb. + +Every PUT carries the snapshot revision (`010` §Concurrency). A `409 +stale_revision` means another tab or a manual edit moved the file: the editor +re-reads and tells the user their view was stale rather than silently +overwriting someone else's work. + +Keyboard reorder must work: up/down buttons alongside the drag handle. A +drag-only affordance is not reachable. + +## Delete + +Confirms first, because a body can be long and there is no undo. Delete removes +the row and PUTs the remainder; WP1 drops its entry from the JSON and +regenerates the projection without it. + +## Tests — `gui/tests/codex-set-custom-layers.test.tsx` + +1. `+ Blank` opens an empty editor +2. Save PUTs the full list with the new layer appended +3. edit changes title and body, id is unchanged +4. toggling a custom layer PUTs with `enabled` flipped +5. delete confirms, then PUTs without the row +6. reorder PUTs the new order +7. keyboard reorder works without pointer events +8. dirty Escape asks before discarding +9. clean Escape closes immediately, focus returns +10. each validation rule disables Save with its message +11. a rejected PUT restores the previous list +12. built-in rows still have no delete control +13. stale revision → re-read, no blind retry +14. unowned `developer_instructions` hides `+` and offers Adopt +15. Adopt shows the raw text and requires confirmation before writing +16. Adopt is refused, with path and line, when the value is not a single-line + basic string +17. a body containing `"` and `\` saves and round-trips unharmed +18. tabs normalize to four spaces; CRLF normalizes to LF +19. a control character is rejected with its position +20. `drift` states render with a Repair action rather than silently self-healing +21. linter: one case per rule, positive and negative +22. clean behavioral text produces zero findings +23. lint spans point at the right substring +24. no rule throws on empty, whitespace-only, or 64 KiB input + +Case 12 re-asserts ask item 6 from the custom-layer side: adding delete to one +row family must not leak it into the other. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/060_wp6_presets_and_linter.md b/devlog/_plan/260802_codex_set_prompt_composer/060_wp6_presets_and_linter.md new file mode 100644 index 0000000000..57efab1b86 --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/060_wp6_presets_and_linter.md @@ -0,0 +1,118 @@ +# 060 — WP6: presets + +The linter moved to `050`, where it is consumed. WP6 is presets and the picker +that offers them — content for a UI that already exists rather than UI ahead of +content. The rule table below stays here as the linter's specification; `050` +implements it. + +Ask item 10: "클로드 코드의 프리셋을 제공한다든지, 아니면 Grok Build의 어떤 +시스템 프롬프트를 제공한다든지 ... 하지만 codex 호환성이 있게 우리가 좀 조작을 +해서". + +That last clause is the whole phase. `002` §5 establishes that the source +material cannot be shipped as-is; §6 establishes what breaks if it is. + +## Presets are authored, not copied + +`002` §5, in short: + +- `02_cc_prompt.md` is a 15 KB Korean **analysis document** about Claude Code's + prompt, not the prompt itself. +- `02_gr_prompt.md` is a 24 KB dossier mixing OSS findings with a + binary-analysis appendix, carrying unresolved `${{ tools.by_kind.* }}`. +- Grok Build's real templates open with "You are Grok" and rely on render-time + placeholders only MiniJinja expands. + +So each preset is **our text**, distilling behavioral intent into +harness-neutral wording, shipped with a provenance line naming the source and +stating that it is an adaptation. No verbatim third-party prompt enters this +repository — which also keeps the licensing question from ever arising. + +## Shipped presets + +`gui/src/components/codex-set/presets.ts`, each ≤ 2 KB: + +| Preset | Distilled intent | Derived from | +|---|---|---| +| Concise output | short answers, no preamble, minimal formatting | Claude Code's brevity directives | +| Plan before editing | state the plan, then edit | Claude Code's planning posture | +| Explain reasoning | narrate why, not just what | Grok Build's confirmation style | +| Test-first | write the failing test first | common agent practice | +| Korean replies | answer in Korean regardless of prompt language | user-requested staple | + +Every preset is a **behavioral instruction**. None names a tool, none claims an +identity, none describes the environment. That is what makes them safe to +append, and it is exactly the constraint the linter enforces. + +## Linter specification (implemented in WP5) + +`gui/src/components/codex-set/prompt-lint.ts` — pure function, no I/O: + +```ts +export type LintLevel = "warn" | "info"; +export interface LintFinding { + level: LintLevel; + rule: string; + messageKey: TKey; + span?: [number, number]; +} +export function lintPromptLayer(body: string): LintFinding[]; +``` + +Rules, each traceable to `002` §6: + +| Rule | Pattern | Level | Why | +|---|---|---|---| +| `identity` | `you are (claude\|grok\|gemini\|gpt-\|chatgpt)` | warn | contradicts base identity — `02_cc_prompt.md:44` | +| `foreign-tool` | `\b(Read\|Edit\|Write\|Bash\|Glob\|Grep)\s+tool\b` | warn | registry defines tools — `model_info.rs:151` | +| `placeholder` | `\$\{\{.*?\}\}` | warn | no MiniJinja over instructions — `context.rs:252` | +| `apply-patch` | `apply_patch` with redefining verbs | warn | same registry argument | +| `approval-vocab` | `always-approve`, `ask mode`, `acceptEdits` | warn | Codex injects its own — `world_state.rs:114` | +| `environment` | claims about cwd, date, network, OS | warn | generated later — `world_state.rs:149` | +| `size` | body > 8 KB | info | **opencodex policy** — see below | + +The size rule cites no upstream limit, because none exists. +`developer_instructions` is a plain config string with no cap +(`config_toml.rs:216`), and `002` §6 records that an earlier draft wrongly cited +the 32 KiB AGENTS.md project-doc budget, which governs an unrelated mechanism. +The 8 KB advisory is ours, justified by per-request token cost and by keeping a +hand-editable config file hand-editable. It is `info`, never `warn`. + +**Warnings never block.** A user who means to override identity may; the linter +ensures it is a decision rather than an accident. `002` §6 says the same. + +Findings render inline in WP5's editor with the offending span highlighted. + +## Preset picker + +Fills the submenu WP5 left empty. Each entry shows name, one-line description, +provenance, and a preview. Choosing one opens the WP5 editor **pre-filled and +fully editable** — a preset is a starting point, not a locked artifact. + +Presets are linted on selection like any other text. If a preset ever trips its +own linter, that is a bug in the preset, and the test below catches it. + +## Tests — `gui/tests/codex-set-presets.test.tsx` + +Rule-level linter cases live in WP5 (`050` cases 16-19). This phase tests the +presets and the picker: + +1. **every shipped preset lints clean** — the self-consistency check +2. every preset is ≤ 2 KB and names no tool, identity, or environment fact +3. picker lists every preset with its provenance line +4. choosing one opens a pre-filled, editable editor +5. the result is an ordinary custom layer — toggleable, editable, deletable +6. preset text is editable before save +7. the submenu appears only in this phase's build — absent when the preset list + is empty + +Case 1 keeps the phase honest: presets that violate our own compatibility rules +would be worse than shipping none. + +## i18n + +Preset names, descriptions, and lint messages all go through `codexSet.preset.*` +and `codexSet.lint.*` in all six locales. Preset **bodies** stay English — +they are instructions to a model, not UI copy, and a mistranslated behavioral +directive is a functional defect. The Korean-replies preset is the exception +that proves it: its body is English text instructing Korean output. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/070_wp7_docs_and_verification.md b/devlog/_plan/260802_codex_set_prompt_composer/070_wp7_docs_and_verification.md new file mode 100644 index 0000000000..c285c2984b --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/070_wp7_docs_and_verification.md @@ -0,0 +1,116 @@ +# 070 — WP7: docs, locales, and the closing gate + +## Docs + +New page: `docs-site/src/content/docs/guides/codex-prompt.md`, plus **four** +locale copies: `ja`, `ko`, `ru`, `zh-cn`. + +The docs site carries four non-English locales +(`docs-site/astro.config.mjs:63-68`); the **GUI** carries five, because it also +has German. An audit caught an earlier draft saying "five locale copies" while +listing four directories. The counts differ by surface and must not be +conflated: docs = 4 + English, GUI = 5 + English. + +A new page must also be added to the sidebar with its four translations, in the +same shape as the entries at `astro.config.mjs:85-91`. + +Contents: + +1. What the prompt stack is — the layer table from `001` §1, in user terms. +2. The five-class taxonomy from `001` §4, in user terms: which layers have a + switch, which are configured elsewhere, and which cannot be suppressed at + all. Say plainly that `feature-gated` layers are reachable but not from this + page, and that extension layers cannot be enumerated. +3. Custom layers: what they are, where they land (`developer_instructions`), + what happens to text the user wrote by hand (preserved, never edited). +4. Presets: adaptations, not copies, with the reasoning from `002` §5. +5. Timing: new sessions, not running ones (`003` §3). +6. That opencodex reads **one** config layer out of eight (`003` §1), so the + page reports this file's values, not the resolved Codex configuration. + +Also update: + +- `docs-site/src/content/docs/reference/configuration/` — the five toggle keys + and `developer_instructions`, in the file where root keys already live. +- Any nav/sidebar entry naming "Codex Auth". + +Astro's config lists locales explicitly; a page added to English only shows a +missing-translation state. Either all five docs variants (English + four), or an +explicit decision — and the `260802_docs_overhaul` unit already established that +as the standard. The GUI's six-locale rule is separate and stated below. + +## Locale parity + +`typecheck` catches missing GUI keys because every dictionary is +`Record` (`004` §D). It does **not** catch a key that exists but +still holds English text. WP7 reads every `codexSet.*` string in all six GUI +locales (en, ko, ja, zh, ru, de) and confirms it is actually translated. + +Korean copy follows the house rule: no translationese, no AI idioms, one +register throughout. + +## Full gate + +On the exact HEAD that closes the unit: + +```bash +bun run typecheck +bun run test +bun run lint:gui +bun run privacy:scan +bun run build:gui +git diff --check +``` + +All six must pass. `build:gui` is included because this unit adds a stylesheet +and new components — a Vite build failure would not surface in typecheck. + +## Acceptance — one row per ask item + +| # | Ask | Proven by | +|---|---|---| +| 1 | tab renamed Codex Set | `030` tests 1-3 | +| 2 | current window → Multi-auth | `030` test 1 | +| 3 | Prompt section beside it | `030` test 2 | +| 4 | Logs-style left/right panels | `030` tests 4-6 | +| 5 | built-in layers switchable | `030` tests 8-9 | +| 6 | `+` adds custom layers | `050` tests 1-2 | +| 7 | built-in rows never deletable | `050` test 12 | +| 8 | custom rows: dialog, save, toggle, delete | `050` tests 1-5 | +| 9 | built-in dialog is read-only | `040` test 7 | +| 10 | **non-disableable layers cannot be turned off** | `040` test 2 **and** `020` test 5, both table-driven over `LAYER_INVENTORY`; `020` test 6 guards the partition | +| 11 | presets provided, Codex-compatible | `060` tests 1-6 | +| 12 | Theme deferred with steps recorded | `090` exists, nothing implemented | + +Item 10 carries two proofs on purpose. A UI-only guarantee is a UI that looks +safe; an API-only guarantee is a boundary nobody exercises. + +## Residual risk, stated rather than hidden + +1. **Upstream drift.** Every key here is ≤ 4 months old and `001` §6 shows the + surface still moving. A rename upstream makes a toggle silently ineffective. + Mitigation: an absent key reads as unknown, never as false. Not a fix. +2. **Model compliance is unproven.** `002` needs-verification: static ordering + is proven; whether a model obeys a custom layer contradicting its base prompt + is not. The linter warns, it does not guarantee. +3. **We read one config layer of eight.** `003` §1. The field is named + `defaultedUserValue` and the UI claims nothing more. Resolved-config + reporting is deferred, not quietly approximated. +4. **Extension layers are not enumerable.** `001` class E. The API says + `extensionLayersEnumerable: false` rather than implying a complete list. +5. **Shared key ownership.** `003` §4 — Codex may rewrite + `developer_instructions` itself. An earlier draft called this non-blocking + while having no concurrency mechanism at all; an audit was right to reject + that. `010` now adds whole-byte revisions, a journal whose recovery refuses to + touch an externally modified file, a tokenized cross-process lock, and a + pre-rename hash guard. What remains genuinely residual: Codex can replace the + file inside the rename window itself, which no user-space mechanism prevents. + It is detected on the next read as `projection-stale` and surfaced as a + Repair action, not silently absorbed. +6. **Byte equality is not Rust equivalence.** `010` carries one golden fixture + parsed by the real `toml_edit`, run on demand. Between runs, the encoder's + correctness rests on a restricted character set and a hand-written grammar + matcher — strong, but not the same as continuous proof. + +Risks 1 and 5 deserve a live re-check whenever opencodex bumps its supported +Codex version. None blocks the unit; all belong in the close-out. diff --git a/devlog/_plan/260802_codex_set_prompt_composer/090_theme_deferred.md b/devlog/_plan/260802_codex_set_prompt_composer/090_theme_deferred.md new file mode 100644 index 0000000000..ce07775d9b --- /dev/null +++ b/devlog/_plan/260802_codex_set_prompt_composer/090_theme_deferred.md @@ -0,0 +1,49 @@ +# 090 — Theme: deferred + +Status: **not started, deliberately.** + +Ask: "뭐 할 수 있으면 theme도 넣는데, codex prompt에는 theme은 나중에 하는 +걸로 그냥 설계해가지고 별도 devlog로 그냥 잠깐 스텝만 남겨놓고". + +So: steps only. Nothing in WP1-WP7 implements any of this, and no Theme tab +ships with this unit. + +## Why it is genuinely separate + +Theme configures how Codex *renders*; the Prompt section configures what the +model *reads*. They share a page and nothing else — no config keys, no write +path, no failure modes. Bundling them would put a cosmetic setting behind the +same "applies to new sessions" caveat that prompt layers need, which is both +wrong and confusing. + +## Steps when it is picked up + +1. **Research the surface.** `~/.codex/config.toml` theme keys at the + then-current upstream HEAD. `001` §6 shows this area moves fast, so a fresh + read is mandatory — do not trust this document's assumptions. +2. **Decide ownership.** Whether Codex has a first-party theme setter, as it + does for `codex features enable/disable` (`cli/src/main.rs:1909-1928`). If it + does, delegate; the `features.ts:1-15` header states the principle — a + third-party writer for something upstream already owns is churn. +3. **Reuse WP1.** If a direct write is needed, `prompt-layers.ts` already has + the path resolution, EOL preservation, scoped line edit, and atomic write. + Extend the allowlist; do not write a second writer. +4. **Add a third tab** to the `030` shell: `#codex-set/theme`. The shell is + already n-way; adding a panel is additive. +5. **Extend `/api/codex-prompt`** or add a sibling route. Prefer a sibling — + theme is not a prompt layer and folding it in would make one endpoint carry + two unrelated schemas. +6. **i18n** in all six locales, English first (`004` §D). +7. **Live verification.** A theme change is visible; verify it in the actual + Codex UI rather than asserting the file contents and calling it done. + +## Open questions to settle first + +- Does theme apply live, or on new sessions like the prompt keys (`003` §3)? + The answer decides the copy, and the copy is the main UX risk. +- Is theme per-profile? `003` §2 — legacy `[profiles.x]` selection is a hard + error now, so a per-profile theme would need `.config.toml`. +- Does the Codex desktop app read theme from the same file, or its own store? + If the latter, this belongs nowhere near `config.toml`. + +None of these is answered here. Answering them is step 1. diff --git a/devlog/_plan/260802_wt1_update_path_star_prompt/000_plan.md b/devlog/_plan/260802_wt1_update_path_star_prompt/000_plan.md new file mode 100644 index 0000000000..e3b8f2a8df --- /dev/null +++ b/devlog/_plan/260802_wt1_update_path_star_prompt/000_plan.md @@ -0,0 +1,45 @@ +# wt1 — Update path + star-prompt leakage (research) + +Worktree: `/Users/jun/.codex/worktrees/260802-wt1-update-path` (branch `codex/wt1-update-path`, off `dev`). +This unit is docs-first preparation; the executing session re-verifies every claim at its own P before building. + +## Scope + +Three bugs in the install/update/prompt surface, all must-fix regardless of PR quality: + +### Bug A — PR #871: preview installs cannot update to stable + +- Evidence: PR #871 (`fix(update): allow stable updates from older previews`). +- Root cause: an installed preview such as `2.8.2-preview.20260731` cannot be parsed by the stable-only current-version branch of the update comparator, so with npm `latest` at `2.9.1`, `isNewer()` returns `false` and the dashboard reports `already_latest`, disabling one-click update. +- Grounding: `src/update/notify.ts` (`isNewer`, `readVersionCache`), consumed by `src/update/badge.ts:69` (`updateAvailable: isNewer(cache.latest_version, current, channel)`). +- Severity: high — the update path silently bricks itself for every preview-channel user once a stable supersedes their build. +- Fix shape (from PR, re-verify): compare an installed preview by its `major.minor.patch` core when the selected channel is `latest`; keep the latest-channel target strict (a preview registry target is never accepted as a stable update). + +### Bug B — Issue #879: star-prompt agent deferral leaks into every editing session + +- Evidence: issue #879, filed 2026-08-02 after user report ("shows during editing, not just install"). +- Root cause (three compounding decisions, verified in code): + 1. `src/cli/star-prompt.ts` leaves the `.star-prompted` marker unwritten on the agent path, so every agent-driven `ocx start` re-prints `printAgentDeferral()` — no deferral marker, counter, or cooldown. + 2. The deferral text recruits the agent as an unbounded relay ("put the same Yes/No question at the top of your next reply, unchanged"); AGENTS.md repeats the repeat-forever rule, so one `ocx start` hijacks every later reply. + 3. Agent PTYs pass the TTY gate, so the deferral fires on routine edit/test cycles. +- Grounding: `src/cli/star-prompt.ts` (`MARKER`, `printAgentDeferral`, `maybeShowStarPrompt`), `src/cli/agent-driven.ts` (env detection), callers `src/cli/index.ts:317` and `src/service.ts:2468`, consumer of marker semantics `src/update/notify.ts:135`, tests `tests/startup-prompt.test.ts`, `tests/agent-driven.test.ts`, `tests/sidebar-routes.test.ts`. +- Severity: medium — degrades every agent-assisted session and trains users to dismiss a consent question. +- Constraint (must keep): an agent never answers or auto-dismisses; only the account owner stars; `403 agent_consent_required` on `POST /api/github/star` stays untouched. + +### Bug C (should-fix, same subsystem) — PR #557: npm cache recovery hardening + +- Maintainer-takeover draft for #533: fail closed when the npm cache is same-UID but not effectively readable/writable before any proxy stop path; sanitize persisted update-job command/log/error fields (raw home/cache paths, uid/gid). +- Include only if wt1 capacity allows after A and B land; otherwise leave for maintainer review. + +## Claim ledger + +| # | Claim | Source | Status | +|---|-------|--------|--------| +| 1 | `isNewer()` stable-only parse rejects `2.8.2-preview.*` current versions | PR #871 body + `src/update/notify.ts` | code-verified | +| 2 | Marker unwritten on agent path; deferral re-arms every agent start | `src/cli/star-prompt.ts` | code-verified | +| 3 | npm semver/dist-tag behavior for preview vs stable channels | Luna lane (not dispatched — covered by repo comparator code) | n/a | + +## Out of scope + +- Changing the consent invariant (agent never stars; user-only prompt). +- Pushing anything; each worktree session commits locally and asks before push. diff --git a/devlog/_plan/260802_wt1_update_path_star_prompt/010_implementation.md b/devlog/_plan/260802_wt1_update_path_star_prompt/010_implementation.md new file mode 100644 index 0000000000..5cd014b73d --- /dev/null +++ b/devlog/_plan/260802_wt1_update_path_star_prompt/010_implementation.md @@ -0,0 +1,41 @@ +# wt1 — Implementation roadmap (re-verify at P before building) + +Branch `codex/wt1-update-path` off `dev`. One PABCD cycle per bug (A then B; C optional). + +## Bug A — PR #871: preview → stable update (build first) + +File map: + +- MODIFY `src/update/notify.ts` — `isNewer()`: when channel is `latest` and the installed version is a preview (`x.y.z-preview.*`), compare by `major.minor.patch` core; a preview registry target is never accepted as a stable update target (strict target side stays). +- MODIFY `src/update/badge.ts` — only if the GUI/API update-check result needs the same comparator path (it consumes `isNewer` at :69, so likely no change; verify). +- MODIFY `tests/` update comparator tests near existing `notify`/`badge` coverage. + +Acceptance + activation scenarios: + +1. Installed `2.8.2-preview.20260731`, channel `latest`, npm latest `2.9.1` → `isNewer` true, GUI shows update available (was `already_latest`). Activation: unit test driving exactly this pair. +2. Installed `2.9.1`, registry target `2.9.2-preview.*` on `latest` channel → still false (target-strict). Activation: regression test. +3. Preview channel behavior unchanged. Activation: existing suite green. + +## Bug B — Issue #879: star-prompt deferral bound (build second) + +File map: + +- MODIFY `src/cli/star-prompt.ts` — add a deferral record (e.g. `.star-deferred` with ISO timestamp) written when `printAgentDeferral()` fires; skip the deferral when the record is younger than the chosen cooldown (recommend: once per version, or 7 days — pick one and document). Bound the relay text itself: one relay per deferral event, not repeat-forever. +- MODIFY `AGENTS.md` — the user-consent paragraph must match the new bound (remove "repeat unchanged forever", keep "never answer for the user"). +- MODIFY `tests/startup-prompt.test.ts`, `tests/agent-driven.test.ts` — regressions below. +- DO NOT TOUCH: `src/server/management/sidebar-routes.ts` `403 agent_consent_required`; the human interactive prompt path; marker semantics used by `src/update/notify.ts:135`. + +Acceptance + activation scenarios: + +1. Second agent-driven `ocx start` within the cooldown prints nothing. Activation: test with a fresh config dir, env marker set, deferral record present. +2. Hand-typed interactive run (no agent env, TTY mocked) still shows the real prompt regardless of deferral record. Activation: existing prompt test stays green + new case. +3. Deferral text no longer instructs repeat-forever relay. Activation: snapshot/string assertion on `printAgentDeferral` output. +4. `hasStarPromptRun()` semantics for `update/notify.ts` unchanged. Activation: existing notify tests green. + +## Bug C — PR #557 (optional, capacity permitting) + +Fold the two high-severity blockers (fail-closed npm cache probe before proxy stop; sanitize persisted job command/log/error fields). Draft PR exists — rebase and finish rather than rewrite. + +## Verification gate for the worktree session + +`bun run typecheck` + focused tests + `bun run test` before proposing merge. Commit per bug; no push without user approval. diff --git a/devlog/_plan/260802_wt2_zero_leak_bounds/000_plan.md b/devlog/_plan/260802_wt2_zero_leak_bounds/000_plan.md new file mode 100644 index 0000000000..86678f29e8 --- /dev/null +++ b/devlog/_plan/260802_wt2_zero_leak_bounds/000_plan.md @@ -0,0 +1,38 @@ +# wt2 — Zero-leak state-store bounds (research) + +Worktree: `/Users/jun/.codex/worktrees/260802-wt2-zero-leak` (branch `codex/wt2-zero-leak`, off `dev`). +Tracker: issue #820. These six PRs are the confirmed retained-state/memory-leak family; all are must-fix regardless of PR quality. + +## Scope + +| PR | Defect | Bound introduced (per PR body, re-verify) | +|----|--------|-------------------------------------------| +| #847 | Unbounded streamed tool-argument memory (SSE records can arrive without delimiters) | 4 MiB shared SSE record cap; 8 MiB per-call / 32 MiB per-turn tool-arg cap; oversized calls fail as typed upstream errors | +| #845 | Unbounded Cursor blob-store memory | 16 MiB/blob, 64 MiB total, 4096 entries; pin active-request blobs; LRU evict only unpinned; protobuf error for rejected `setBlobArgs` | +| #844 | Unbounded Cursor Connect frame buffering | 32 MiB inbound payload cap; reject oversized declared length at the 5-byte header; complete one pending frame incrementally; fail turn on EOF with incomplete frame | +| #843 | Unbounded Antigravity replay retention | SHA-256 fixed-size cache identities; 64 KiB/signature, 256 calls + 2 MiB/session, 32 MiB global; LRU + 1h TTL preserved | +| #841 | Responses continuation state not byte-accounted | UTF-8 byte measurement before admission; reject single entries > 64 MiB store budget; same rule on snapshot load | +| #840 | Windows ACL temp-path memos never released | Release memos once ephemeral file proven absent; key atomic-write timeouts by stable destination | + +- Severity: high — all six let a long-running proxy grow RSS without bound; the Windows ACL leak (#840) was confirmed by live measurement in the prior zero-leak unit (`devlog/_plan/260801_zero_leak_state_stores/`). +- Grounding entry points: `src/responses/state.ts` (#841), Cursor Connect/blob code paths (#844/#845), `src/config.ts:184-187` (`atomicWriteFileAsync` timeout memo keyed by destination, #840), SSE/tool-argument streaming in `src/adapters/openai-responses.ts` + `src/server/responses/core.ts` (#847). + +## Key review questions for the executing session + +- Do the caps fail as typed upstream errors without emitting a completed tool item or a clean Chat DONE marker (#847)? +- Do pinned blobs leave a safe-capacity failure mode rather than deadlock (#845)? +- Do the six PRs overlap in shared helpers? Order of landing matters; rebase chain per PR body cross-references (#840-#845-#847 all cite #820). +- Benchmark cells: prior unit recorded 16/16 scoped PASS but competitor cells remain `UNKNOWN` — do not write superiority/RSS claims into docs or release notes. + +## Claim ledger + +| # | Claim | Source | Status | +|---|-------|--------|--------| +| 1 | Six stores are unbounded on current dev | #820 tracker + PR bodies #840-#847 | code-verified (prior unit) | +| 2 | Windows ACL memo leak reproduced by measurement | `devlog/_plan/260801_zero_leak_state_stores/` | verified (prior unit) | +| 3 | Proposed caps match provider payload realities (32 MiB frames etc.) | PR bodies; no external source needed | unverified — executing session must spot-check | + +## Out of scope + +- RSS/benchmark superiority claims. +- Changing TTL semantics beyond what each PR states. diff --git a/devlog/_plan/260802_wt2_zero_leak_bounds/010_implementation.md b/devlog/_plan/260802_wt2_zero_leak_bounds/010_implementation.md new file mode 100644 index 0000000000..fd3857fe03 --- /dev/null +++ b/devlog/_plan/260802_wt2_zero_leak_bounds/010_implementation.md @@ -0,0 +1,26 @@ +# wt2 — Implementation roadmap (re-verify at P before building) + +Branch `codex/wt2-zero-leak` off `dev`. One PABCD cycle per PR-family cluster; all six cite tracker #820. + +## Landing order (dependency-ordered, not effort-bucketed) + +1. **#841 Responses state byte cap** — foundation: `src/responses/state.ts` admission accounting is shared by snapshot load; other bounds do not depend on it, but it is the smallest contract to verify the byte-accounting pattern the others reuse. +2. **#847 Streamed tool-argument bounds** — `src/adapters/openai-responses.ts` + `src/server/responses/core.ts` (+ Chat outbound conversion). 4 MiB SSE record cap; 8 MiB/call, 32 MiB/turn. +3. **#844 Connect frame buffering** → 4. **#845 blob-store** — Cursor lane; #845's pinning assumes #844's incremental frame completion. +5. **#843 Antigravity replay retention** — independent; SHA-256 identities + LRU. +6. **#840 Windows ACL memo release** — `src/config.ts:184-187` (`atomicWriteFileAsync` memo keyed by stable destination). LAND LAST: wt4's realpath fix (#869) also touches this function; coordinate so wt4 lands first or rebase over it. + +## Per-PR acceptance pattern (apply to each) + +1. Oversized input fails as a typed error without emitting a completed tool item / clean DONE marker / acknowledged store. Activation: fault-injection test feeding an over-cap payload, asserting the typed error and the absent success side-effect. +2. At-cap valid input still succeeds. Activation: boundary test at exactly cap and cap−1. +3. RSS-style retention proof: after N cycles of oversized + valid traffic, store size stays under the stated bound. Activation: focused harness test (reuse the prior zero-leak unit's measurement approach; `devlog/_plan/260801_zero_leak_state_stores/`). +4. Eviction never removes entries pinned by an active request (#845) / prior valid mappings survive an oversized replacement (#843) / unrelated valid chains survive rejection (#841). + +## Verification gate + +`bun run typecheck` + `bun run test` full suite (shared stores = full suite per AGENTS.md). No RSS superiority claims in docs/release notes — comparator cells remain `UNKNOWN`. + +## Cross-worktree coordination (wt3 Bug B) + +wt2 #847 and wt3 Bug B (#860) both touch `src/adapters/openai-responses.ts` and `src/server/responses/core.ts`. Different code paths (wt2: SSE record/tool-argument caps; wt3: `service_tier` injection at `core.ts:803-807`), so either order lands — but the second lane MUST rebase over the first and re-run its fault-injection tests. wt4's realpath fix (#869) precedes wt2 #840 as already ordered above. diff --git a/devlog/_plan/260802_wt3_provider_wire/000_plan.md b/devlog/_plan/260802_wt3_provider_wire/000_plan.md new file mode 100644 index 0000000000..dfb9b65819 --- /dev/null +++ b/devlog/_plan/260802_wt3_provider_wire/000_plan.md @@ -0,0 +1,42 @@ +# wt3 — Provider wire correctness (research) + +Worktree: `/Users/jun/.codex/worktrees/260802-wt3-provider-wire` (branch `codex/wt3-provider-wire`, off `dev`). +Provider-adapter/wire bugs; all must-fix regardless of PR quality. + +## Scope + +### Bug A — PR #746 / issue #748: Copilot Responses-only models routed to chat completions + +- Root cause: the `github-copilot` preset configures a provider-wide `openai-chat` adapter, but Copilot fronts a mixed-wire catalog; several newer OpenAI models are served only by the Responses API (`model "gpt-5.6-sol" is not accessible via the /chat/completions endpoint`). `gpt-5.4` hides it behind a passing text-only smoke test; a real Codex request (function tools + reasoning effort) fails. +- Grounding: `src/providers/registry.ts`, `src/providers/github-copilot-transport.ts`. +- Severity: high — hard data-plane failure for Copilot users on current models. + +### Bug B — PR #860 (+ issue #875): DeepSeek `service_tier` must be capability-gated + +- Root cause: `fastMode` injects `service_tier` unconditionally on Responses routes; DeepSeek does not support the field. PR #860 adds a provider-level `supportsServiceTier` capability: canonical OpenAI Responses providers support it, DeepSeek explicitly rejects it (strip the field), unclassified custom providers keep caller-supplied values. +- Fresh corroboration: issue #875 (2026-08-02) "DeepSeek V4 Flash Responses route stalls after tool calls" — same wire family; executing session must check whether #875 is the same root cause or a second defect before closing either. +- Grounding: `src/adapters/openai-responses.ts`, `src/server/responses/core.ts`, `src/types.ts`. + +### Bug C — PRs #839 / #854: Claude 4.6/4.7 1M context windows missing + +- Root cause: `ANTHROPIC_MODEL_CONTEXT_WINDOWS` omits `claude-opus-4-7`, `claude-opus-4-6`, `claude-sonnet-4-6`, so they advertise `max_input_tokens: null`, `shouldMarkOneMillion()` rejects them, and the `[1m]` picker row is never emitted — Claude Code accounts them at its 200k default. #854 additionally fixes generated profiles writing `[1m]` on sub-1M routes (372K route marked `[1m]`). +- Grounding: `src/claude/context-windows.ts`, `src/claude/model-info.ts`, `src/providers/registry.ts`. +- Note: #839 and #854 overlap; land ONE consolidated fix, credit both PRs. + +### Optional — PRs #616 / #837: hosted image tool preferences + +- Some gateways reserve the image tool namespace server-side; generic normalization collides with client `image_gen` declarations. #837 already integrates #616 onto current dev preserving authorship. Include if capacity allows. + +## Claim ledger + +| # | Claim | Source | Status | +|---|-------|--------|--------| +| 1 | Copilot serves some models Responses-only | gpt-5.4 verified (BerriAI/litellm#23332, exact `unsupported_api_for_model` error); gpt-5.6-sol lead only (JetBrains LLM-29711: function tools + reasoning_effort rejected on `/chat/completions`); same pattern for gpt-5-codex (opencode #2758) | verified (5.4) / lead (sol) | +| 2 | DeepSeek rejects/mishandles `service_tier` | No primary evidence either way (api-docs.deepseek.com does not list the field; Anthropic-compatible API marks it "Ignored" — different endpoint) | unresolved — capability-gating is safe regardless; do not claim rejection without a live probe | +| 3 | DeepSeek Responses route stalls after tool calls (hosted api.deepseek.com) | Stall reports are NIM/vLLM compatibility paths, not hosted; verified hosted failure mode is a 400 when `reasoning_content` is omitted after a tool call (official Thinking Mode docs; claude-code-router#1378) | contradicted as stated — #875 may be a `reasoning_content` echo defect, not #860's root cause; executing session must split them | +| 4 | Claude Opus 4.6/4.7 + Sonnet 4.6 are documented at 1M context | Anthropic official: Opus 4.6 (1M beta, 2026-02-05), Opus 4.7 (1M, 2026-04-16, migration guide), Sonnet 4.6 (1M beta, 2026-02-17); model overview cross-check | verified | + +## Out of scope + +- New provider presets (covered by separate enhancement PRs). +- Changing the conservative relay capability policy for unknown providers beyond what #860 states. diff --git a/devlog/_plan/260802_wt3_provider_wire/010_implementation.md b/devlog/_plan/260802_wt3_provider_wire/010_implementation.md new file mode 100644 index 0000000000..b1676cfafb --- /dev/null +++ b/devlog/_plan/260802_wt3_provider_wire/010_implementation.md @@ -0,0 +1,53 @@ +# wt3 — Implementation roadmap (re-verify at P before building) + +Branch `codex/wt3-provider-wire` off `dev`. One PABCD cycle per bug; land in dependency order A → B → C (D optional). + +## Bug A — #746/#748: Copilot mixed-wire routing + +File map: + +- MODIFY `src/providers/registry.ts` — per-model wire override for the `github-copilot` preset: Responses-only models route to the Responses adapter instead of the preset-wide `openai-chat`. +- MODIFY `src/providers/github-copilot-transport.ts` — only if the transport needs Responses-shaped auth/headers distinct from chat (verify at P). +- MODIFY provider routing tests + a mixed-wire fixture: model list containing both chat-served and Responses-only models. + +Acceptance + activation: + +1. `gpt-5.4` via Copilot preset issues a Responses request (never `/chat/completions`). Activation: mock-transport test asserting the wire per model — external corroboration: litellm#23332. +2. Chat-served Copilot models still use chat completions (no regression). Activation: existing suite. +3. `gpt-5.6-sol` tools+reasoning request takes the path that does not 400. Activation: request-shape test; note external evidence is lead-only (JetBrains LLM-29711), so gate on request shape, not a claimed upstream error string. + +## Bug B — #860 (+#875): DeepSeek service_tier capability gate + +File map: + +- MODIFY `src/types.ts` + registry-enriched metadata — provider-level `supportsServiceTier` capability (`src/providers/registry.ts`), canonical OpenAI Responses providers = true, DeepSeek = explicitly false. +- MODIFY `src/adapters/openai-responses.ts` / `src/server/responses/core.ts` — `fastMode` injects/removes `service_tier` only for supporting providers; strip for rejecting providers; preserve caller values for unclassified custom providers. +- DOCS: configuration reference (EN + zh-CN) per PR. + +Acceptance + activation: + +1. DeepSeek Responses request never carries `service_tier`, including with `fastMode` on. Activation: serialized-payload test. +2. Canonical OpenAI keeps injecting; custom unclassified preserves caller value. Activation: payload tests. +3. Issue #875 triage: determine whether the stall is (a) this field, (b) missing `reasoning_content` echo after tool calls (verified DeepSeek failure mode — 400, per official Thinking Mode docs), or (c) NIM/vLLM-only. Hosted stall reports were NOT externally verified; do not close #875 on #860's evidence alone. If (b), that is a separate fix in the same lane (response-state must round-trip `reasoning_content`). + +## Bug C — #839/#854 consolidated: Claude 1M windows + +File map: + +- MODIFY `src/providers/registry.ts:217` — `ANTHROPIC_MODEL_CONTEXT_WINDOWS` lives here (verified on dev@3195c7194; it does omit the three models). Add `claude-opus-4-7`, `claude-opus-4-6`, `claude-sonnet-4-6` at 1M (externally verified: Anthropic announcements 2026-02-05 / 2026-04-16 / 2026-02-17 + model overview). `src/claude/context-windows.ts` only hosts `shouldMarkOneMillion` (:83) and marker helpers — no map change there. +- MODIFY `src/claude/model-info.ts` — generated profiles: `[1m]` marker only when the authoritative effective window ≥ 1M (fixes the 372K-route-marked-`[1m]` defect from #854); honor provider caps + case-insensitive marker spelling. +- Tests: picker row emission (`[1m]` present for the three models, absent for sub-1M routes). + +Land as ONE PR crediting #839 and #854. + +## Cross-worktree coordination (wt2 #847) + +wt3 Bug B and wt2 #847 both touch `src/adapters/openai-responses.ts` and `src/server/responses/core.ts`. The changes live in different code paths (wt2: SSE record/tool-argument caps; wt3: `service_tier` injection at `core.ts:803-807`), so either landing order works — but whichever lane lands second MUST rebase over the other and re-run its payload-shape tests. Both units name this file pair in their ledgers. + +## Bug D (optional) — #616/#837 hosted image tools + +Rebase #837 (it already integrates #616 with authorship preserved); validate the per-model Responses wire including OpenAI API virtual-model rewrites. + +## Verification gate + +`bun run typecheck` + `bun run test`; wire changes need the full suite (shared adapters). diff --git a/devlog/_plan/260802_wt4_server_config_security/000_plan.md b/devlog/_plan/260802_wt4_server_config_security/000_plan.md new file mode 100644 index 0000000000..bf67451ede --- /dev/null +++ b/devlog/_plan/260802_wt4_server_config_security/000_plan.md @@ -0,0 +1,33 @@ +# wt4 — Server CORS + atomic config writes (research) + +Worktree: `/Users/jun/.codex/worktrees/260802-wt4-server-config` (branch `codex/wt4-server-config`, off `dev`). +Two must-fix bugs on the server/config boundary, both security-adjacent. + +## Scope + +### Bug A — PR #850: browser-extension CORS origins all collapse to `null` + +- Root cause: `URL.origin` serializes `chrome-extension://...` (and other non-special schemes) as the string `"null"` per the WHATWG URL Standard. Comparing that value makes every browser extension origin look identical — the allowlist cannot distinguish the configured extension from any other, while rejecting `*` leaves users no safe path. +- Fix shape (from PR, re-verify): canonicalize authority-based opaque origins as `scheme + "://" + authority` instead of the WHATWG `null` serialization; allow only the configured extension ID; cover both preflight and `/v1/models` data-plane requests; document in EN + zh-CN config references. +- Grounding: `src/server/index.ts` (`setCorsOrigin` at :136/:345; `origin_rejected` data-plane blocks at :424/:463/:580/:614/:635/:669). +- Severity: high — this is the CORS security boundary; a wrong comparison here is an origin-confusion defect, not a cosmetic one. + +### Bug B — PR #869: atomic config writes destroy symlinked destinations + +- Root cause: `atomicWriteFile`/`atomicWriteFileAsync` write a temp file beside the literal destination path and `rename(2)` over it. rename replaces the directory entry, so when the destination is itself a symlink the link is destroyed and replaced by a regular file. Breaks dotfiles-managed setups (`~/.codex/config.toml` symlinked into a tracked repo): the first injected write silently converts it to a real file and the repo stops receiving updates; the live config keeps working so the divergence is invisible. +- Grounding: `src/config.ts:107` (`atomicWriteFile`), `src/config.ts:184-187` (`atomicWriteFileAsync`); callers include `src/config.ts:1627` (config write), `:2070` (pid), `:2098` (runtime port state). +- Severity: high — silent data divergence; nothing surfaces it until the user diffs their dotfiles repo. +- Fix shape (from PR, re-verify): resolve the destination through `realpath` before choosing the temp-file location so the rename lands beside the real file, preserving the link. + +## Claim ledger + +| # | Claim | Source | Status | +|---|-------|--------|--------| +| 1 | `new URL("chrome-extension://...").origin === "null"` per WHATWG URL | WHATWG URL Standard §4.7 (opaque origin for non-special schemes; serializes as `"null"`); applies equally to `moz-extension://` and `safari-web-extension://`; compare scheme+host or `runtime.getURL("/")` instead | verified | +| 2 | rename(2) over a symlink replaces the link, not the target | POSIX.1-2024 `rename()` (Open Group pubs 9799919799) + Linux man-pages 6.18 `rename(2)`: "if newpath refers to a symbolic link, the link will be overwritten"; standard mitigation = realpath/canonicalize destination first (check/use race caveat: use dirfd-based resolution for security-sensitive paths) | verified | +| 3 | All atomicWriteFile callers audited for symlink destinations | PR #869 body | unverified — executing session must enumerate | + +## Out of scope + +- Relaxing the CORS allowlist model itself (exact-match stays). +- Windows ACL hardening of the temp path (that is wt2/#840's lane; coordinate if both touch `atomicWriteFileAsync` — wt4 owns the realpath fix, wt2 owns the memo release; land in that order to minimize conflicts). diff --git a/devlog/_plan/260802_wt4_server_config_security/010_implementation.md b/devlog/_plan/260802_wt4_server_config_security/010_implementation.md new file mode 100644 index 0000000000..cb229b1112 --- /dev/null +++ b/devlog/_plan/260802_wt4_server_config_security/010_implementation.md @@ -0,0 +1,36 @@ +# wt4 — Implementation roadmap (re-verify at P before building) + +Branch `codex/wt4-server-config` off `dev`. Security-boundary lane: per AGENTS.md, CORS/origin changes need explicit security review before merge. + +## Bug A — #850: extension CORS origin identity + +File map: + +- MODIFY `src/server/auth-cors.ts` — the actual WHATWG `"null"`-collapse comparison lives in `isExtraAllowedOrigin` (:81-89, `new URL(allowed).origin === new URL(origin).origin`), with `isSameOriginAsRequest` (:64) adjacent; `setCorsOrigin` (:21) only sets `_corsOrigin`. Canonicalize authority-based opaque origins as `scheme + "://" + authority` instead of the `"null"` serialization; allow only the configured extension ID; `*` stays rejected. Externally verified basis: WHATWG URL §4.7 collapses all three extension schemes to `"null"`; compare scheme+host, never `.origin`. +- COVER preflight AND every data-plane rejection path: at P, grep `origin_rejected` in `src/server/index.ts` and cover ALL ten sites (:424/:463/:580/:614/:635/:669/:694/:788/:819/:853 as of dev@3195c7194 — re-grep, do not trust this list). `src/server/index.ts` itself is listed for coverage sites only; the comparison change is in `auth-cors.ts`. +- DOCS: EN + zh-CN configuration references. + +Acceptance + activation: + +1. Configured extension ID passes preflight and `/v1/models`. Activation: request test with `Origin: chrome-extension://`. +2. A DIFFERENT extension ID is rejected (this is the actual bug — today both serialize to `"null"`). Activation: adversarial request test. +3. `moz-extension://` and `safari-web-extension://` get the same canonicalization treatment (verified same spec rule). Activation: parametrized scheme tests. +4. Non-extension origins (http/https localhost rules) unchanged. Activation: existing suite. + +## Bug B — #869: realpath before atomic rename + +File map: + +- MODIFY `src/config.ts:107` (`atomicWriteFile`) and `:184` (`atomicWriteFileAsync`) — resolve the destination through realpath before choosing the temp-file location, so the rename lands beside the real file and the symlink survives. Verified basis: POSIX.1-2024 `rename()` + Linux `rename(2)`: "if newpath refers to a symbolic link, the link will be overwritten." +- Audit EVERY caller: at P, grep `atomicWriteFile` across `src/` (~21 files as of dev@3195c7194) and classify each for symlink-destination exposure. Do not treat any inline list as exhaustive. Explicitly named because it is the most symlink-sensitive caller in the tree: `src/oauth/store.ts` (credential store). Also confirmed callers: `src/config.ts:1627/:2070/:2098`, `src/responses/state.ts`, `src/codex/journal.ts`, `src/codex/inject.ts`, `src/grok/inject.ts`, `src/codex/history-provider.ts`, `src/update/job.ts`, `src/update/notify.ts`, `src/claude/desktop-3p.ts`, `src/codex/{refresh,runtime,features,account-store,quota}.ts`, `src/codex/catalog/*`. +- Caveat from research: canonicalization introduces a check/use race; for the config path this is acceptable (user-owned dir), but note it in the code comment — do not claim race-free. + +Acceptance + activation: + +1. `~/.codex/config.toml` as a symlink into a temp "dotfiles repo": after an injected write, the symlink still exists and the target file carries the update. Activation: fixture test asserting `lstat` is still a link post-write. +2. Non-symlink destinations byte-identical behavior. Activation: existing suite. +3. Coordination: wt2's #840 touches `atomicWriteFileAsync` memo logic. wt4 lands the realpath change first (or rebases); both units name this file in their ledgers. + +## Verification gate + +`bun run typecheck` + `bun run test` + `bun run privacy:scan`; security review sign-off per MAINTAINERS.md before merge (CORS boundary). diff --git a/devlog/_plan/260802_wt4_server_config_security/011_wp1_execution_notes.md b/devlog/_plan/260802_wt4_server_config_security/011_wp1_execution_notes.md new file mode 100644 index 0000000000..95763fb356 --- /dev/null +++ b/devlog/_plan/260802_wt4_server_config_security/011_wp1_execution_notes.md @@ -0,0 +1,23 @@ +# wp1 execution notes (P-phase stale check, 2026-08-02) + +## Stale check results + +- `010_implementation.md` line refs: auth-cors.ts `isExtraAllowedOrigin` confirmed live at :81-89 in the pre-fix form; `setCorsOrigin` :21. `origin_rejected` sites in `src/server/index.ts` = 10 (grep-confirmed), all funneled through `isAllowedRequestOrigin`/`isAllowedManagementOrigin` → both call `isExtraAllowedOrigin`, so ONE predicate fix covers every data-plane + management-plane rejection site. +- Bun URL behavior verified locally (primary proof): `chrome-extension://abc123/page.html`, `moz-extension://u-u-i-d/`, `safari-web-extension://UUID/` all serialize `.origin` as `"null"` with `host` populated. +- Docs drift: `docs-site` configuration reference was split into domain subpages (commit 7fdb2cb8e). The `corsAllowOrigins` row now lives at `docs-site/src/content/docs//reference/configuration/server.md` (:19-20) for all five locales (EN, zh-cn, ko, ja, ru). The PR #850 doc hunks DO NOT APPLY to current dev — docs must be re-based onto the subpage rows. +- PR #850 state: OPEN, CONFLICTING vs dev. Maintainer review blocker (locale doc drift) was already fixed by the contributor in a346ad60, so the fetched diff head is review-clean. Code+test hunks APPLY CLEAN to dev@478354ee8 (`git apply --check --include='src/**' --include='tests/**'`). + +## wp1 execution decision + +Apply the maintainer-reviewed PR #850 diff for `src/server/auth-cors.ts`, `src/types.ts`, `tests/server-auth.test.ts`, `tests/server-loopback-host-gate.test.ts` verbatim (credit eachann1024), then re-write the five `server.md` rows by hand in the new subpage location with the same content the PR used (authority-based extension origins supported; `*` is not a wildcard). + +Rationale: the implementation is small, already maintainer-reviewed, and its test shape (live server preflight + data-plane + unit-level cross-scheme and `*` rejection) matches the acceptance criteria in `010_implementation.md` exactly. Hand-rewriting an equivalent fix would add risk, not value. + +## Acceptance (carried from 010, mapped to PR tests) + +1. Configured extension ID passes preflight + `/v1/models` — covered by the new `server-auth.test.ts` case (204 + allow-origin echo; 200 data-plane). +2. Different extension ID rejected — covered (403 live; false unit). +3. Cross-scheme isolation (`moz-extension://` rejected) — covered by unit test. +4. `*` rejected — covered by unit test. +5. Non-extension behavior unchanged — existing `server-auth` / `server-loopback-host-gate` suites must stay green. +6. Docs: five locale `server.md` rows updated; locales must not contradict EN. diff --git a/devlog/_plan/260802_wt4_server_config_security/020_wp2_execution_notes.md b/devlog/_plan/260802_wt4_server_config_security/020_wp2_execution_notes.md new file mode 100644 index 0000000000..6d2f27a912 --- /dev/null +++ b/devlog/_plan/260802_wt4_server_config_security/020_wp2_execution_notes.md @@ -0,0 +1,38 @@ +# wp2 execution notes (P-phase stale check, 2026-08-02) + +## Stale check results + +- `atomicWriteFile` at `src/config.ts:107`, `atomicWriteFileAsync` at :187 — both confirmed in the pre-fix form (temp `${path}.ocx.${pid}.${seq}.tmp` beside the LITERAL path; `io.rename(tmp, path)`). +- PR #869 diff (`/tmp/wt4-pr869.diff`, 336 lines) applies CLEAN to dev@478354ee8 + wp1 commit (`git apply --check` passes). +- PR #869 state: OPEN, MERGEABLE, quality gates green, no maintainer blocker comments. +- The diff's `src/responses/state.ts` hunk expects `enforceAppOwnedMemoryBudget` (77243d932) — present on current dev. + +## PR #869 implementation shape (what will be applied) + +1. `resolveWriteTarget(path)` (exported): `realpathSync(path)`, fallback to literal path when unresolvable (first write of a not-yet-created file). +2. `assertResolvedTargetAllowed(path, target)`: re-applies the test-only real-home guard (`assertNotRealHomeUnderTest`) to the RESOLVED target — a symlink escaping a temp fixture home into the protected home is refused even though the caller's dir-level check passed. Inert in production. +3. Both sync + async writers compute `tmp` beside the RESOLVED target and rename onto it. +4. `src/responses/state.ts` snapshot load sweeps stale temps in BOTH the literal and the resolved directory (a symlinked snapshot strands temps in the real dir). +5. Tests: `tests/config.test.ts` symlink suite (sync+async: link survives + target updated, no temps left, plain destination unaffected, first-write creation, dangling symlink replaced), `tests/responses-state.test.ts` (sweep in resolved dir), `tests/test-home-guard.test.ts` (escape-refused probe). + +## Caller audit (criterion c5) — grep `atomicWriteFile` across `src/` @ dev 478354ee8 + +| Caller | Writes into | Symlink exposure | +|---|---|---| +| `src/config.ts` (owner; config.json :1627, pid :2070, runtime port :2098) | OPENCODEX_HOME | direct — config dir is dotfiles-managed in the reported case | +| `src/oauth/store.ts` | OPENCODEX_HOME credential store | high-value target; fix protects token files behind symlinked dirs | +| `src/codex/inject.ts`, `journal.ts`, `history-provider.ts`, `account-store.ts`, `quota.ts`, `refresh.ts`, `runtime.ts`, `features.ts` | `~/.codex/*` | the reported dotfiles case (`config.toml` symlink) | +| `src/claude/desktop-3p.ts` | Claude Desktop config | user-managed file, symlink plausible | +| `src/grok/inject.ts` | grok config | same shape | +| `src/responses/state.ts` | OPENCODEX_HOME snapshot | covered by the sweep-in-both-dirs hunk | +| `src/update/job.ts`, `notify.ts` | OPENCODEX_HOME update state | covered by shared helper | +| `src/codex/catalog/*` (aggregation, bundled, effort, metadata, parsing, provider-fetch, sync) | catalog caches | covered by shared helper | + +No caller needs an individual change: the fix lives in the shared writer + the one stale-temp sweep that scans directories. + +## Known design decisions to audit + +- Dangling symlink: realpath fails → literal path → rename REPLACES the dangling link (PR test asserts this). Debate point: silently replacing a dotfiles link whose target dir is temporarily unmounted. PR chose "replace"; the alternative (refuse) breaks first-write-into-new-target-dir. +- TOCTOU: link swapped between realpath and rename lands the write at the old target. Accepted, documented in `010_implementation.md`; not claimed race-free. +- Windows: `realpathSync` resolves junctions/symlinks on win32 too; temp stays same-volume because it sits beside the resolved target. +- wt2 coordination: #840's memo-release touches `atomicWriteFileAsync` timeout-memo area; this diff does not overlap those lines (memo keyed by `path` argument, unchanged). diff --git a/devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md b/devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md new file mode 100644 index 0000000000..71477e277d --- /dev/null +++ b/devlog/_plan/260802_wt5_windows_service_doctor/000_plan.md @@ -0,0 +1,32 @@ +# wt5 — Windows scheduler settle + Bun provenance diagnostics (research) + +Worktree: `/Users/jun/.codex/worktrees/260802-wt5-windows-service` (branch `codex/wt5-windows-service`, off `dev`). +Two must-fix bugs in service install/diagnostics. + +## Scope + +### Bug A — PR #868: scheduler registration verification never settles + +- Root cause (from PR body, re-verify): post-create Task Scheduler verification retried non-transient states, so registration verification could fail to settle. Fix retries only transient post-create visibility/XML health states, preserves fail-closed behavior for conflicts/missing assets/unknown SCM status, and stops late reconciliation when attempt ownership changes. +- Grounding: `src/service.ts`, `src/lib/winsw.ts`, `src/lib/windows-elevation.ts`; verification contracts in scheduler/startup/install tests (PR claims 136 focused tests). +- Severity: medium-high — Windows service install reliability; fail-closed semantics must be preserved exactly. + +### Bug B — PR #861 / issue #848: doctor repeats OPENCODEX_BUN_PATH guidance when override already active + +- Root cause (owner-confirmed on `dev@fa5780d5`): the service/doctor path has the Bun version but no trustworthy runtime-origin field, so Windows `auto-known-bad` repeats the `OPENCODEX_BUN_PATH` instruction even when the override is already active. +- Fix shape (owner-directed, from issue #848 comment): propagate one allowlisted `override | bundled | process` provenance marker through every actual launcher (npm Node launcher, Windows scheduler, native WinSW, launchd, systemd); report unknown/absent for legacy payloads instead of inferring from the current shell (`durableBunRuntime()` at reporting time can mislabel the running process); keep `bunRevision` informational; preserve the conservative `auto-known-bad` result for canaries; document the provenance trust rule in `structure/05_gui-and-management-api.md`. +- Grounding: `src/lib/bun-runtime.ts`, `src/service.ts`, `src/cli/status.ts`, `src/server/management/system-routes.ts`. +- Severity: medium — diagnostics-only, but it sends Windows users down a wrong remediation path. + +## Claim ledger + +| # | Claim | Source | Status | +|---|-------|--------|--------| +| 1 | Windows scheduler verification can fail to settle on transient states | PR #868 body | code-verified (PR author claims live Windows validation) | +| 2 | Doctor mislabels runtime origin; repeats override guidance | issue #848, owner comment | verified by owner on dev@fa5780d5 | +| 3 | Conservative eager-relay policy must stay unchanged for canary Bun | issue #848, owner comment | verified directive | + +## Out of scope + +- Changing the conservative eager-relay capability policy for canary/unknown Bun builds. +- macOS launchd/systemd behavior changes beyond adding the provenance marker. diff --git a/devlog/_plan/260802_wt5_windows_service_doctor/010_implementation.md b/devlog/_plan/260802_wt5_windows_service_doctor/010_implementation.md new file mode 100644 index 0000000000..c0c2224bb9 --- /dev/null +++ b/devlog/_plan/260802_wt5_windows_service_doctor/010_implementation.md @@ -0,0 +1,38 @@ +# wt5 — Implementation roadmap (re-verify at P before building) + +Branch `codex/wt5-windows-service` off `dev`. Windows-heavy lane: run long CPU/validation work on `ssh macmini-cf` or a Windows box; bare `macmini` fails host-key verification (ops note from maintainer). + +## Bug A — #868: scheduler verification settle + +File map: + +- MODIFY `src/service.ts` (scheduler registration + post-create verification) — retry ONLY transient post-create Task Scheduler visibility/XML health states. +- PRESERVE fail-closed for: conflicts, missing assets, unknown SCM status. Stop late reconciliation when attempt ownership changes. +- Tests: scheduler/startup/service/install-verification contracts (PR claims 136 focused; re-verify on rebase). + +Acceptance + activation: + +1. Transient post-create invisibility settles to installed/viable/running within the retry budget. Activation: fault-injection test with scripted transient states. +2. Conflict / missing asset / unknown SCM each still fail closed with no retry storm. Activation: three adversarial tests. +3. Ownership change mid-reconcile stops late writes. Activation: interleaving test. +4. Live Windows validation: startup protection reports installed, viable, running, conflict-free (PR author claims this; executing session re-runs it). + +## Bug B — #861/#848: Bun runtime provenance + +File map (owner-directed shape from issue #848 comment — follow it exactly): + +- MODIFY all five launcher paths to stamp one allowlisted `override | bundled | process` marker: npm Node launcher, Windows scheduler, native WinSW (`src/lib/winsw.ts`), launchd, systemd (`src/service.ts`, `src/lib/bun-runtime.ts`). +- MODIFY `src/server/management/system-routes.ts` — expose the recorded provenance scalar alongside Bun version/revision. +- MODIFY doctor/status (`src/cli/status.ts`) — report recorded provenance; legacy payload without the field = unknown/absent. NEVER call `durableBunRuntime()` at report time to guess from the current shell (mislabels the running process). +- KEEP `bunRevision` informational; conservative `auto-known-bad` for canaries unchanged; eager-relay capability policy untouched. +- DOCS: `structure/05_gui-and-management-api.md` — provenance trust + backward-compat rule. + +Acceptance + activation: + +1. With `OPENCODEX_BUN_PATH` override active, doctor no longer repeats the setup instruction. Activation: regression test with override env + stamped marker. +2. Legacy service payload (no marker) reports unknown, not a shell guess. Activation: fixture with old payload. +3. Regressions cover scheduler + WinSW + launchd + systemd + direct Node launcher (owner's explicit list in #848). + +## Verification gate + +`bun run typecheck` + focused doctor/runtime/service/watchdog tests (baseline was 99/99 on the issue thread) + `bun run test`. diff --git a/docs-site/src/content/docs/ja/reference/configuration/server.md b/docs-site/src/content/docs/ja/reference/configuration/server.md index 6273a2b7ec..cb41dc273f 100644 --- a/docs-site/src/content/docs/ja/reference/configuration/server.md +++ b/docs-site/src/content/docs/ja/reference/configuration/server.md @@ -16,7 +16,7 @@ description: リスナー、リモート アクセス、アドミッション | `connectTimeoutMs?` | `number` | `200000` |試行ごとの DNS/TCP/TLS/最終ヘッダーの期限。本体が生成される前に終了します。 | | `shutdownTimeoutMs?` | `number` | `5000` |アクティブなターンが中止される前の正常な排出期限。 | | `websockets?` | `boolean` | `false` |応答 WebSocket パスとして `supports_websockets` をアドバタイズします。 False は HTTP/SSE を維持します。 | -| `corsAllowOrigins?` | `string[]` | `[]` |追加の正確な CORS 起点。ループバック起点は常に許可されます。 | +| `corsAllowOrigins?` | `string[]` | `[]` | 追加の正確な CORS origin。ループバック origin は常に許可します。`chrome-extension://` など authority ベースのブラウザー拡張 origin に対応し、`*` はワイルドカードではありません。Firefox と Safari は拡張 UUID を（インストール/ブラウザー起動ごとに）再生成するため、origin が変わったらエントリを更新してください。 | | `apiKeys?` | `OcxApiKey[]` | `[]` |生成された `ocx_…` 資格情報は、非ループバック バインドでの管理およびデータ プレーン認証によって受け入れられました。ダッシュボードで管理。 | | `storageCleanupPolicy?` | `StorageCleanupPolicy` |無効 |アーカイブされたセッションのクリーンアップ ポリシーをオプトインします。暗黙的に有効になることはありません。 | | `appOwnedMemoryBudgetMb?` | `number` | `256` |排除可能なアプリ所有のログ、キャッシュ、BLOB、および継続ペイロードの MiB の上限。範囲は 64 ～ 4096。 RSSキャップではありません。 | diff --git a/docs-site/src/content/docs/ko/reference/configuration/server.md b/docs-site/src/content/docs/ko/reference/configuration/server.md index 36727c08ff..2632f0b168 100644 --- a/docs-site/src/content/docs/ko/reference/configuration/server.md +++ b/docs-site/src/content/docs/ko/reference/configuration/server.md @@ -16,7 +16,7 @@ description: 리스너, 원격 접근, admission 키, 타임아웃, 저장소, | `connectTimeoutMs?` | `number` | `200000` | 시도별 DNS/TCP/TLS/최종 헤더 기한입니다. 본문 생성 전에 끝납니다. | | `shutdownTimeoutMs?` | `number` | `5000` | 진행 중인 turn을 중단하기 전에 허용하는 정상 종료 드레인 기한입니다. | | `websockets?` | `boolean` | `false` | Responses WebSocket 경로에 `supports_websockets`를 광고합니다. `false`이면 HTTP/SSE를 유지합니다. | -| `corsAllowOrigins?` | `string[]` | `[]` | 추가로 허용할 정확한 CORS origin입니다. 루프백 origin은 항상 허용됩니다. | +| `corsAllowOrigins?` | `string[]` | `[]` | CORS에서 추가로 허용할 정확한 origin입니다. 루프백 origin은 항상 허용됩니다. `chrome-extension://` 같은 authority 기반 브라우저 확장 origin을 지원하며, `*`는 와일드카드가 아닙니다. Firefox와 Safari는 확장 UUID를 (설치/브라우저 실행 때마다) 새로 만드므로 origin이 바뀌면 항목을 갱신하세요. | | `apiKeys?` | `OcxApiKey[]` | `[]` | 비루프백 바인드에서 관리 API와 데이터 플레인 인증이 허용하는 생성된 `ocx_…` 자격 증명입니다. 대시보드에서 관리합니다. | | `storageCleanupPolicy?` | `StorageCleanupPolicy` | disabled | 선택적으로 활성화하는 보관 세션 정리 정책입니다. 절대 암묵적으로 활성화되지 않습니다. | | `appOwnedMemoryBudgetMb?` | `number` | `256` | 제거 가능한 앱 소유 로그, 캐시, blob, continuation payload에 대한 MiB 단위 상한입니다. 범위는 64–4096이며 RSS 상한은 아닙니다. | diff --git a/docs-site/src/content/docs/reference/configuration/server.md b/docs-site/src/content/docs/reference/configuration/server.md index 23f9887dd0..4f66870358 100644 --- a/docs-site/src/content/docs/reference/configuration/server.md +++ b/docs-site/src/content/docs/reference/configuration/server.md @@ -17,7 +17,7 @@ runs helper features around provider requests. | `connectTimeoutMs?` | `number` | `200000` | Per-attempt DNS/TCP/TLS/final-header deadline; it ends before body generation. | | `shutdownTimeoutMs?` | `number` | `5000` | Graceful drain deadline before active turns are aborted. | | `websockets?` | `boolean` | `false` | Advertise `supports_websockets` for the Responses WebSocket path. False keeps HTTP/SSE. | -| `corsAllowOrigins?` | `string[]` | `[]` | Additional exact CORS origins. Loopback origins are always allowed. | +| `corsAllowOrigins?` | `string[]` | `[]` | Additional exact origins allowed by CORS. Loopback origins are always allowed. Authority-based browser extension origins such as `chrome-extension://` are supported; `*` is not a wildcard. Firefox and Safari regenerate the extension UUID (per install / per browser launch), so update the entry when the origin changes. | | `apiKeys?` | `OcxApiKey[]` | `[]` | Generated `ocx_…` credentials accepted by management and data-plane auth on non-loopback binds. Dashboard-managed. | | `storageCleanupPolicy?` | `StorageCleanupPolicy` | disabled | Opt-in archived-session cleanup policy. Never enabled implicitly. | | `appOwnedMemoryBudgetMb?` | `number` | `256` | Cap in MiB for evictable app-owned logs, caches, blobs, and continuation payloads. Range 64–4096; not an RSS cap. | diff --git a/docs-site/src/content/docs/ru/reference/configuration/server.md b/docs-site/src/content/docs/ru/reference/configuration/server.md index 1d841e00db..af5f4f6f14 100644 --- a/docs-site/src/content/docs/ru/reference/configuration/server.md +++ b/docs-site/src/content/docs/ru/reference/configuration/server.md @@ -17,7 +17,7 @@ description: Listener, удалённый доступ, admission key, тайм | `connectTimeoutMs?` | `number` | `200000` | Дедлайн одной попытки DNS/TCP/TLS/final-header; он завершается до генерации тела ответа. | | `shutdownTimeoutMs?` | `number` | `5000` | Дедлайн graceful-drain до принудительного прерывания активных turn'ов. | | `websockets?` | `boolean` | `false` | Объявлять `supports_websockets` для WebSocket-пути Responses. Значение false удерживает HTTP/SSE. | -| `corsAllowOrigins?` | `string[]` | `[]` | Дополнительные точные CORS-origin'ы. Loopback-origin'ы разрешены всегда. | +| `corsAllowOrigins?` | `string[]` | `[]` | Дополнительные точные origin, разрешённые CORS. Loopback-origin разрешены всегда. Поддерживаются authority-based origin браузерных расширений, например `chrome-extension://`; `*` не является маской. Firefox и Safari пересоздают UUID расширения (при каждой установке/запуске браузера), поэтому обновляйте запись при смене origin. | | `apiKeys?` | `OcxApiKey[]` | `[]` | Сгенерированные credentials `ocx_…`, принимаемые для management и data-plane auth на не-loopback bind'ах. Управляются через дашборд. | | `storageCleanupPolicy?` | `StorageCleanupPolicy` | disabled | Opt-in policy очистки архивированных сессий. Никогда не включается неявно. | | `appOwnedMemoryBudgetMb?` | `number` | `256` | Лимит в MiB для eviction-friendly app-owned log'ов, cache'ей, blob'ов и continuation payload'ов. Это не RSS-cap. Диапазон 64–4096. | diff --git a/docs-site/src/content/docs/zh-cn/reference/configuration/server.md b/docs-site/src/content/docs/zh-cn/reference/configuration/server.md index 73a6a7ed93..665588ee92 100644 --- a/docs-site/src/content/docs/zh-cn/reference/configuration/server.md +++ b/docs-site/src/content/docs/zh-cn/reference/configuration/server.md @@ -17,7 +17,7 @@ description: 监听、远程访问、准入密钥、超时、存储、侧车、 | `connectTimeoutMs?` | `number` | `200000` | 每次尝试的 DNS/TCP/TLS/最终响应头截止时间；它在正文生成之前结束。 | | `shutdownTimeoutMs?` | `number` | `5000` | 优雅停机截止时间，超过后会中止仍在进行中的请求。 | | `websockets?` | `boolean` | `false` | 为 Responses WebSocket 路径声明 `supports_websockets`。设为 false 会保留 HTTP/SSE。 | -| `corsAllowOrigins?` | `string[]` | `[]` | 额外的精确 CORS 来源。回环来源始终允许。 | +| `corsAllowOrigins?` | `string[]` | `[]` | CORS 额外允许的精确 origin。loopback origin 始终允许；支持 `chrome-extension://<扩展 ID>` 等基于 authority 的浏览器扩展 origin，`*` 不是通配符。Firefox 和 Safari 会（每次安装/启动浏览器时）重新生成扩展 UUID，origin 变化后请更新该条目。 | | `apiKeys?` | `OcxApiKey[]` | `[]` | 管理平面和非回环绑定上的数据平面身份验证可接受的已生成 `ocx_…` 凭据。由仪表板管理。 | | `storageCleanupPolicy?` | `StorageCleanupPolicy` | disabled | 可选启用的归档会话清理策略。不会被隐式启用。 | | `appOwnedMemoryBudgetMb?` | `number` | `256` | 可逐出应用自有日志、缓存、blob 和续传载荷的内存上限，单位 MiB。范围 64–4096；不是 RSS 上限。 | diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts new file mode 100644 index 0000000000..23fa265c75 --- /dev/null +++ b/src/codex/prompt-layers.ts @@ -0,0 +1,246 @@ +/** + * prompt-layers.ts — the Codex prompt-layer surface in `$CODEX_HOME/config.toml`. + * + * Scope boundary: this module owns the five `include_*` prompt toggles and the + * generated `developer_instructions` projection. It is a SIBLING of + * `features.ts`, not an extension of it: that module's header explicitly forbids + * broadening itself past `multi_agent_v2`, so the technique is copied here + * rather than the file being widened. + * + * Two design decisions are load-bearing and were forced by an adversarial audit + * (devlog/_plan/260802_codex_set_prompt_composer/): + * + * 1. NO USER PROSE IS PARSED BACK OUT OF TOML. Custom layers live in + * `opencodex-prompt.json`, which we own outright; config.toml receives a + * write-only projection of the enabled subset. Layer identity never has to + * survive a round trip through a TOML parser. + * + * 2. NO TOML LIBRARY IS USED TO VERIFY WHAT WE WROTE. Measured on Bun 1.3.14, + * `Bun.TOML.parse` transposes `\t` and `\f`, rejects `\u0007`, and does not + * trim the newline after an opening `'''`. Codex parses with Rust + * `toml_edit`, so verifying through a JS parser could report success on a + * file Codex reads differently. Instead the accepted character set is + * restricted until escaping is total under three rules, and verification is + * a byte comparison. + * + * CODEX_HOME is resolved at CALL time (the `features.ts:58-67` pattern) so tests + * can point fixtures via env or an explicit path. + */ +import { existsSync, readFileSync, realpathSync } from "node:fs"; +import { join, resolve } from "node:path"; +import { createHash } from "node:crypto"; +import { expandUserPath } from "../config"; +import { CODEX_CONFIG_PATH } from "./paths"; +import { OCX_SECTION_MARKER } from "./injected-marker"; + +// --------------------------------------------------------------------------- +// Inventory — ONE definition, consumed by the route and the GUI alike. +// Classes are the five in devlog 001 §4; the partition is total and disjoint. +// --------------------------------------------------------------------------- + +export type LayerClass = + | "base" + | "config-toggle" + | "feature-gated" + | "runtime-conditional" + | "extension-unknown"; + +export type ToggleId = + | "permissions" + | "collaboration" + | "environment" + | "apps" + | "skills"; + +export interface LayerDescriptor { + id: string; + class: LayerClass; + /** config key for config-toggle and feature-gated rows; null otherwise */ + key: string | null; + /** documented default when the key is absent */ + default: boolean | null; + /** assembly index from devlog 001 §1; null when registration-order dependent */ + order: number | null; +} + +/** + * Assembly order per `core/src/session/world_state.rs`. `base-instructions` is + * NOT a world-state section — it travels in the Responses `instructions` field + * — so it carries order 0 and sits ahead of the rest. + * + * `plugins` is `runtime-conditional`, not feature-gated: `core/src/mcp.rs:200` + * computes `selected_plugin_available || !capability_summaries().is_empty()`, + * so `[features] plugins` influences the right operand but does not gate + * emission. + */ +export const LAYER_INVENTORY: readonly LayerDescriptor[] = Object.freeze([ + { id: "base-instructions", class: "base", key: null, default: null, order: 0 }, + { id: "model-switch", class: "runtime-conditional", key: null, default: null, order: 1 }, + { id: "personality", class: "feature-gated", key: "features.personality", default: true, order: 2 }, + { id: "context-window-guidance", class: "feature-gated", key: "features.token_budget", default: false, order: 3 }, + { id: "realtime", class: "runtime-conditional", key: null, default: null, order: 4 }, + { id: "agents-md", class: "runtime-conditional", key: null, default: null, order: 5 }, + { id: "permissions", class: "config-toggle", key: "include_permissions_instructions", default: true, order: 6 }, + { id: "collaboration", class: "config-toggle", key: "include_collaboration_mode_instructions", default: true, order: 7 }, + { id: "environment", class: "config-toggle", key: "include_environment_context", default: true, order: 8 }, + { id: "environments-instructions", class: "feature-gated", key: "features.deferred_executor", default: false, order: 9 }, + { id: "apps", class: "config-toggle", key: "include_apps_instructions", default: true, order: 10 }, + { id: "plugins", class: "runtime-conditional", key: null, default: null, order: 11 }, + { id: "tools", class: "feature-gated", key: "features.deferred_tool_world_state", default: false, order: 12 }, + { id: "skills", class: "config-toggle", key: "skills.include_instructions", default: true, order: 13 }, + { id: "multi-agent-mode", class: "feature-gated", key: "features.multi_agent_v2.enabled", default: false, order: 14 }, +] as const); + +/** + * The write allowlist. Fixed, never computed: `config_toml.rs` does NOT carry + * serde's `deny_unknown_fields`, so a typo'd key is silently ignored in normal + * mode and a hard startup error under `--strict-config`. A fixed table means + * the GUI can never emit a key it did not intend. + */ +const TOGGLE_KEYS: Record = { + permissions: { table: null, key: "include_permissions_instructions" }, + collaboration: { table: null, key: "include_collaboration_mode_instructions" }, + environment: { table: null, key: "include_environment_context" }, + apps: { table: null, key: "include_apps_instructions" }, + skills: { table: "skills", key: "include_instructions" }, +}; + +export const TOGGLE_IDS = Object.freeze(Object.keys(TOGGLE_KEYS) as ToggleId[]); + +export function isToggleId(value: string): value is ToggleId { + return Object.prototype.hasOwnProperty.call(TOGGLE_KEYS, value); +} + +// --------------------------------------------------------------------------- +// Paths +// --------------------------------------------------------------------------- + +export interface Paths { + configPath?: string; + storePath?: string; +} + +function activeCodexHome(): string { + const raw = process.env.CODEX_HOME?.trim(); + if (!raw) return CODEX_CONFIG_PATH.slice(0, -"/config.toml".length); + const path = resolve(expandUserPath(raw)); + try { + return realpathSync.native(path); + } catch { + return path; + } +} + +export function activeConfigPath(opts?: Paths): string { + return opts?.configPath ?? join(activeCodexHome(), "config.toml"); +} + +export function activeStorePath(opts?: Paths): string { + return opts?.storePath ?? join(activeCodexHome(), "opencodex-prompt.json"); +} + +// --------------------------------------------------------------------------- +// Character policy — see the header. Defined over Unicode SCALAR VALUES, not +// UTF-16 code units, because a lone surrogate is not a scalar value and UTF-8 +// encoding would silently substitute U+FFFD. +// --------------------------------------------------------------------------- + +export interface CharacterFinding { + /** code-point index, consistent across module, route and editor */ + position: number; + reason: "control" | "unpaired-surrogate"; + codePoint: number; +} + +/** Tab to four spaces, CRLF and lone CR to LF. Applied BEFORE validation. */ +export function normalizeBody(body: string): string { + return body.replace(/\r\n?/g, "\n").replace(/\t/g, " "); +} + +/** First offending scalar, or null. Run AFTER normalizeBody. */ +export function findInvalidCharacter(body: string): CharacterFinding | null { + let position = 0; + for (let i = 0; i < body.length; ) { + const code = body.codePointAt(i)!; + const unit = body.charCodeAt(i); + const isHighSurrogate = unit >= 0xd800 && unit <= 0xdbff; + const isLowSurrogate = unit >= 0xdc00 && unit <= 0xdfff; + // codePointAt only combines a well-formed pair, so a surviving surrogate + // code point here is unpaired by construction. + if ((isHighSurrogate || isLowSurrogate) && code === unit) { + return { position, reason: "unpaired-surrogate", codePoint: code }; + } + const isNewline = code === 0x0a; + const isC0 = code < 0x20 && !isNewline; + const isDel = code === 0x7f; + const isC1 = code >= 0x80 && code <= 0x9f; + if (isC0 || isDel || isC1) { + return { position, reason: "control", codePoint: code }; + } + i += code > 0xffff ? 2 : 1; + position += 1; + } + return null; +} + +/** + * TOML basic-string encoding, total over the accepted set: three rules, none of + * them in the range where `Bun.TOML.parse` misbehaves. `\r` cannot appear + * because normalizeBody removed it; control characters cannot appear because + * findInvalidCharacter rejected them. + */ +export function encodeBasicString(body: string): string { + return `"${body.replace(/\\/g, "\\\\").replace(/"/g, '\\"').replace(/\n/g, "\\n")}"`; +} + +/** + * Inverse of `encodeBasicString`, deliberately narrow: it accepts ONLY the three + * escapes we emit. `\t`, `\f`, `\b`, `\r` and `\uXXXX` are refused rather than + * guessed — decoding them correctly is exactly the ambiguity the restricted set + * exists to avoid. + */ +export function decodeBasicString(literal: string): string | null { + if (literal.length < 2 || !literal.startsWith('"') || !literal.endsWith('"')) return null; + const inner = literal.slice(1, -1); + let out = ""; + for (let i = 0; i < inner.length; i += 1) { + const ch = inner[i]!; + if (ch !== "\\") { + if (ch === '"') return null; // unescaped quote: not a single literal + out += ch; + continue; + } + const next = inner[i + 1]; + if (next === "\\") out += "\\"; + else if (next === '"') out += '"'; + else if (next === "n") out += "\n"; + else return null; // any other escape is outside what we will decode + i += 1; + } + return out; +} + +// --------------------------------------------------------------------------- +// Byte-level hashing. The revision covers COMPLETE file bytes plus existence, +// so removing the marker while leaving the value intact still changes it. +// --------------------------------------------------------------------------- + +function readFileOrNull(path: string): string | null { + try { + if (!existsSync(path)) return null; + return readFileSync(path, "utf8"); + } catch { + return null; + } +} + +export function computeRevision(configBytes: string | null, storeBytes: string | null): string { + const hash = createHash("sha256"); + hash.update("cfg:"); + hash.update(configBytes ?? "\0absent"); + hash.update("\nstore:"); + hash.update(storeBytes ?? "\0absent"); + return `sha256:${hash.digest("hex")}`; +} + +export { readFileOrNull as readFileBytes }; diff --git a/src/config.ts b/src/config.ts index dd09ce8b4d..a4467b026c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,8 +1,8 @@ import { execFileSync } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { chmodSync, copyFileSync, existsSync, linkSync, mkdirSync, readFileSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, readFileSync, realpathSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; -import { join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { Database } from "bun:sqlite"; import * as z from "zod/v4"; import { @@ -104,6 +104,57 @@ function isMissingPathError(error: unknown): boolean { return (error as NodeJS.ErrnoException | undefined)?.code === "ENOENT"; } +/** + * Resolve a write target through any symlink before the temp+rename dance. + * + * rename(2) replaces a directory ENTRY. When the entry is itself a symlink + * (a dotfiles-managed `~/.codex/config.toml` -> `~/dotfiles/.codex/config.toml`, + * say), renaming a sibling temp file over it destroys the link and leaves a plain + * file behind — the repo silently stops receiving writes. Resolving first puts both + * the temp file and the rename target inside the link's real directory, so the entry + * being replaced is the real file and the symlink survives. + * + * Same-filesystem atomicity is preserved because the temp file stays beside its + * resolved target. A genuinely absent destination (not yet created) falls back to + * the literal path, which is the correct target for a first write. + * + * An EXISTING symlink that cannot be resolved — dangling because its target volume + * is unmounted, an ELOOP chain, an EACCES parent — is refused instead. Falling back + * to the literal path there would let the rename replace the link, recreating the + * exact dotfiles-divergence failure this helper exists to prevent (audit: wt4 wp2). + */ +export function resolveWriteTarget(path: string): string { + try { + return realpathSync(path); + } catch (cause) { + let entry; + try { + entry = lstatSync(path); + } catch (error) { + if (isMissingPathError(error)) return path; // no entry at all — first write + throw error; + } + if (entry.isSymbolicLink()) { + throw new Error(`refusing to replace unresolvable symlinked write target: ${path}`, { cause }); + } + return path; + } +} + +/** + * Re-apply the real-home guard to a RESOLVED write target. + * + * Callers such as saveConfig check only their logical config dir, which passes when + * OPENCODEX_HOME points at a temp fixture. Following a symlink out of that fixture + * would land on the protected home the caller's own check just cleared, so the guard + * has to run again on wherever the write actually terminates. Inert in production, + * where the guard is disarmed. + */ +function assertResolvedTargetAllowed(path: string, target: string): void { + if (target === path) return; + assertNotRealHomeUnderTest(dirname(target)); +} + export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO = { write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }), harden: target => { @@ -115,13 +166,15 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO unlink: unlinkSync, }): void { recordOwnedConfigPath(resolveConfigDir(), path); - const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; + const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); + const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { io.write(tmp, content); io.harden(tmp); hardened = true; - io.rename(tmp, path); + io.rename(tmp, target); forgetHardenedSecretPath(tmp); } catch (cause) { let scrubbed = false; @@ -201,13 +254,15 @@ export async function atomicWriteFileAsync( truncate: target => truncateSync(target, 0), unlink: unlinkSync, }; - const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; + const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); + const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { await effective.write(tmp, content); await effective.harden(tmp); hardened = true; - await effective.rename(tmp, path); + await effective.rename(tmp, target); forgetHardenedSecretPath(tmp); } catch (cause) { let scrubbed = false; diff --git a/src/responses/state.ts b/src/responses/state.ts index 4a39647dc8..a275c91631 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -1,6 +1,6 @@ import { chmodSync, existsSync, lstatSync, mkdirSync, opendirSync, readFileSync, rmSync, unlinkSync } from "node:fs"; import { dirname, join } from "node:path"; -import { atomicWriteFileAsync, getConfigDir } from "../config"; +import { atomicWriteFileAsync, getConfigDir, resolveWriteTarget } from "../config"; import { enforceAppOwnedMemoryBudget, type RetainedStoreSnapshot } from "../lib/app-owned-memory"; import type { OcxProviderContinuationState } from "../types"; import { @@ -454,10 +454,23 @@ function ensureLoaded(): void { if (loaded) return; loaded = true; const path = snapshotPath(); + // Atomic writes place their temp beside the RESOLVED target, so a symlinked + // snapshot (dotfiles-managed config dir) strands temps in the link's real + // directory where a scan of the literal config dir would never see them. + // Both locations are swept; they collapse to one when nothing is symlinked. + // resolveWriteTarget refuses a dangling link; snapshot loading stays independent. + let resolvedDir = dirname(path); try { - recoverStaleResponseStateTemps(dirname(path)); + resolvedDir = dirname(resolveWriteTarget(path)); } catch { - /* best-effort cleanup only; snapshot loading must remain independent */ + /* unresolvable link: sweep the literal dir only */ + } + for (const dir of new Set([dirname(path), resolvedDir])) { + try { + recoverStaleResponseStateTemps(dir); + } catch { + /* best-effort cleanup only; snapshot loading must remain independent */ + } } try { if (existsSync(path)) { diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index 4505510425..aedf4a932e 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -80,15 +80,28 @@ export function isAllowedRequestOrigin(req: Request, config: OcxConfig): boolean function isExtraAllowedOrigin(origin: string, cfg: OcxConfig): boolean { if (!cfg.corsAllowOrigins?.length) return false; + const parsedOrigin = comparableOrigin(origin); return cfg.corsAllowOrigins.some(allowed => { - try { - return new URL(allowed).origin === new URL(origin).origin; - } catch { - return allowed === origin; - } + const parsedAllowed = comparableOrigin(allowed); + return parsedOrigin !== null && parsedAllowed !== null + ? parsedAllowed === parsedOrigin + : allowed === origin; }); } +function comparableOrigin(value: string): string | null { + try { + const parsed = new URL(value); + if (parsed.origin !== "null") return parsed.origin; + // WHATWG URL exposes authority-based custom schemes (for example browser + // extensions) as opaque `null` origins. Compare their scheme + authority so + // one allowlisted extension cannot admit every other opaque origin. + return parsed.host ? `${parsed.protocol}//${parsed.host}` : null; + } catch { + return null; + } +} + export function managementRequestOrigin(req: Request, config: OcxConfig): string | null { const host = req.headers.get("Host"); const parsedHost = parseHttpHost(host); diff --git a/src/types.ts b/src/types.ts index c4827a8fa6..b2b37e243b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -756,7 +756,7 @@ export interface OcxConfig { combos?: Record; /** Background proactive token refresh ("Token Guardian"). Off by default; see OcxTokenGuardianConfig. */ tokenGuardian?: OcxTokenGuardianConfig; - /** Additional origins allowed for CORS (e.g. ["https://clisu-oracle.tail19a2d7.ts.net"]). Loopback origins are always allowed. */ + /** Additional exact origins allowed for CORS (e.g. HTTPS or chrome-extension://). Loopback origins are always allowed. */ corsAllowOrigins?: string[]; } diff --git a/tests/codex-prompt-layers.test.ts b/tests/codex-prompt-layers.test.ts new file mode 100644 index 0000000000..63b06c14bf --- /dev/null +++ b/tests/codex-prompt-layers.test.ts @@ -0,0 +1,173 @@ +/** + * Encoding and inventory contract for src/codex/prompt-layers.ts. + * + * The encoding cases exist because `Bun.TOML.parse` cannot be trusted as a + * verifier here: on Bun 1.3.14 it transposes `\t` and `\f`, rejects `\u0007`, + * and does not trim the newline after an opening `'''`. Codex parses with Rust + * `toml_edit`, so these assertions are deliberately BYTE-level — they check + * what we emit, not what a JS parser makes of it. + */ +import { describe, expect, test } from "bun:test"; +import { + LAYER_INVENTORY, + TOGGLE_IDS, + computeRevision, + decodeBasicString, + encodeBasicString, + findInvalidCharacter, + isToggleId, + normalizeBody, +} from "../src/codex/prompt-layers"; + +/** TOML basic-string grammar, hand-written so it does not share code with the encoder. */ +const BASIC_STRING = /^"(?:[^"\\\u0000-\u001f]|\\["\\bfnrt]|\\u[0-9A-Fa-f]{4})*"$/; + +describe("inventory", () => { + test("every id is classified exactly once", () => { + const ids = LAYER_INVENTORY.map(d => d.id); + expect(new Set(ids).size).toBe(ids.length); + }); + + test("every config-toggle carries a key, and the toggle set matches", () => { + const toggles = LAYER_INVENTORY.filter(d => d.class === "config-toggle"); + for (const d of toggles) expect(d.key).not.toBeNull(); + expect(toggles.map(d => d.id).sort()).toEqual([...TOGGLE_IDS].sort()); + }); + + test("only config-toggle rows are writable", () => { + for (const d of LAYER_INVENTORY) { + expect(isToggleId(d.id)).toBe(d.class === "config-toggle"); + } + }); + + test("plugins is runtime-conditional, not feature-gated", () => { + // core/src/mcp.rs:200 — selected_plugin_available || !summaries.is_empty(). + // [features] plugins feeds only the right operand. + const plugins = LAYER_INVENTORY.find(d => d.id === "plugins"); + expect(plugins?.class).toBe("runtime-conditional"); + expect(plugins?.key).toBeNull(); + }); + + test("base instructions are class base and never toggleable", () => { + const base = LAYER_INVENTORY.find(d => d.id === "base-instructions"); + expect(base?.class).toBe("base"); + expect(isToggleId("base-instructions")).toBe(false); + }); +}); + +describe("normalization", () => { + test("tab becomes four spaces", () => { + expect(normalizeBody("a\tb")).toBe("a b"); + }); + + test("CRLF and lone CR become LF", () => { + expect(normalizeBody("a\r\nb\rc")).toBe("a\nb\nc"); + }); + + test("newlines and non-BMP text survive", () => { + expect(normalizeBody("a\nb 😀")).toBe("a\nb 😀"); + }); +}); + +describe("character policy", () => { + test("accepts printable text, newlines, non-BMP and U+2028/U+2029", () => { + // U+2028/U+2029 are not TOML line terminators and cannot end a basic string. + expect(findInvalidCharacter("plain\nline 😀 é \u2028\u2029")).toBeNull(); + }); + + test("rejects C0 controls with a code-point position", () => { + const found = findInvalidCharacter("ab\u0007cd"); + expect(found).toEqual({ position: 2, reason: "control", codePoint: 7 }); + }); + + test("rejects DEL and C1 controls", () => { + expect(findInvalidCharacter("a\u007f")?.reason).toBe("control"); + expect(findInvalidCharacter("a\u0085")?.reason).toBe("control"); + }); + + test("rejects an unpaired surrogate", () => { + // UTF-8 encoding would replace it with U+FFFD and silently alter the prompt. + expect(findInvalidCharacter("a\ud800b")?.reason).toBe("unpaired-surrogate"); + expect(findInvalidCharacter("a\udc00b")?.reason).toBe("unpaired-surrogate"); + }); + + test("a well-formed surrogate pair is not flagged", () => { + expect(findInvalidCharacter("😀")).toBeNull(); + }); + + test("position counts code points, not UTF-16 units", () => { + // "😀" is one code point but two UTF-16 units. + expect(findInvalidCharacter("😀\u0007")?.position).toBe(1); + }); +}); + +describe("encoding", () => { + const bodies = [ + "plain", + 'has """ triple quotes', + "back \\ slash", + "trailing backslash \\", + "line1\nline2", + "emoji 😀 and é", + "# >>> ocx-layer:abc123", // the retired fence text is now inert + "'''literal'''", + 'quote " inside', + "\\n literal backslash-n", + ]; + + for (const body of bodies) { + test(`emits one grammar-valid line: ${JSON.stringify(body).slice(0, 32)}`, () => { + const line = encodeBasicString(body); + expect(line.includes("\n")).toBe(false); + expect(BASIC_STRING.test(line)).toBe(true); + }); + + test(`round-trips through our own decoder: ${JSON.stringify(body).slice(0, 32)}`, () => { + expect(decodeBasicString(encodeBasicString(body))).toBe(body); + }); + } + + test("a 64 KiB body stays one line", () => { + const line = encodeBasicString("a".repeat(65536)); + expect(line.includes("\n")).toBe(false); + expect(line.length).toBe(65538); + }); +}); + +describe("decoder is deliberately narrow", () => { + test("refuses escapes we never emit", () => { + for (const literal of ['"a\\tb"', '"a\\fb"', '"a\\bb"', '"a\\rb"', '"a\\u00e9b"']) { + expect(decodeBasicString(literal)).toBeNull(); + } + }); + + test("refuses unterminated or unquoted input", () => { + expect(decodeBasicString('"abc')).toBeNull(); + expect(decodeBasicString("abc")).toBeNull(); + expect(decodeBasicString("'''abc'''")).toBeNull(); + }); + + test("refuses an unescaped interior quote", () => { + expect(decodeBasicString('"a"b"')).toBeNull(); + }); +}); + +describe("revision", () => { + test("absent and empty files are distinguishable", () => { + expect(computeRevision(null, null)).not.toBe(computeRevision("", "")); + }); + + test("changes when only the marker is removed", () => { + const withMarker = '# Auto-injected by opencodex\ndeveloper_instructions = "x"\n'; + const without = 'developer_instructions = "x"\n'; + expect(computeRevision(withMarker, "{}")).not.toBe(computeRevision(without, "{}")); + }); + + test("changes when the config is deleted", () => { + expect(computeRevision("a", "{}")).not.toBe(computeRevision(null, "{}")); + }); + + test("is stable for identical bytes", () => { + expect(computeRevision("a", "{}")).toBe(computeRevision("a", "{}")); + }); +}); diff --git a/tests/config.test.ts b/tests/config.test.ts index b62da331e6..b0399df09c 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, renameSync, rmSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, readlinkSync, renameSync, rmSync, symlinkSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { @@ -27,7 +27,7 @@ import { } from "../src/config"; import * as windowsAcl from "../src/lib/windows-secret-acl"; -import { AtomicWriteResidualTempError, atomicWriteFile, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config"; +import { AtomicWriteResidualTempError, atomicWriteFile, atomicWriteFileAsync, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config"; let testDir = ""; beforeEach(() => { @@ -1700,3 +1700,115 @@ describe("config.ts – Windows ACL hardening integration", () => { spy.mockRestore(); }); }); + +describe("config.ts – atomic writes preserve symlinked destinations", () => { + test("a symlinked destination survives the write and the real file receives it", () => { + // Dotfiles shape: ~/.codex/config.toml -> ~/dotfiles/.codex/config.toml + const repoDir = join(testDir, "dotfiles"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config.toml"); + symlinkSync(realFile, link); + + atomicWriteFile(link, "rewritten"); + + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(readlinkSync(link)).toBe(realFile); + expect(readFileSync(realFile, "utf8")).toBe("rewritten"); + expect(readFileSync(link, "utf8")).toBe("rewritten"); + }); + + test("no temp file is left beside the link or its target", () => { + const repoDir = join(testDir, "dotfiles-clean"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-clean.toml"); + symlinkSync(realFile, link); + + atomicWriteFile(link, "rewritten"); + + expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]); + expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]); + }); + + test("a plain destination is unaffected", () => { + const destination = join(testDir, "plain.toml"); + atomicWriteFile(destination, "first"); + atomicWriteFile(destination, "second"); + + expect(lstatSync(destination).isSymbolicLink()).toBe(false); + expect(readFileSync(destination, "utf8")).toBe("second"); + }); + + test("a destination that does not exist yet is created at the literal path", () => { + const destination = join(testDir, "created.toml"); + expect(existsSync(destination)).toBe(false); + + atomicWriteFile(destination, "fresh"); + + expect(readFileSync(destination, "utf8")).toBe("fresh"); + }); + + test("a dangling symlink is preserved and the write is refused", () => { + const link = join(testDir, "dangling.toml"); + symlinkSync(join(testDir, "gone", "config.toml"), link); + + // The target volume may only be temporarily unavailable; replacing the link + // would recreate the dotfiles divergence this fix exists to prevent. + expect(() => atomicWriteFile(link, "recovered")).toThrow(/unresolvable symlinked write target/); + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(existsSync(join(testDir, "gone"))).toBe(false); + }); +}); + +describe("config.ts – async atomic writes preserve symlinked destinations", () => { + test("a symlinked destination survives the write and the real file receives it", async () => { + const repoDir = join(testDir, "dotfiles-async"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-async.toml"); + symlinkSync(realFile, link); + + await atomicWriteFileAsync(link, "rewritten"); + + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(readlinkSync(link)).toBe(realFile); + expect(readFileSync(realFile, "utf8")).toBe("rewritten"); + expect(readFileSync(link, "utf8")).toBe("rewritten"); + }); + + test("no temp file is left beside the link or its target", async () => { + const repoDir = join(testDir, "dotfiles-async-clean"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-async-clean.toml"); + symlinkSync(realFile, link); + + await atomicWriteFileAsync(link, "rewritten"); + + expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]); + expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]); + }); + + test("a plain destination is unaffected", async () => { + const destination = join(testDir, "plain-async.toml"); + await atomicWriteFileAsync(destination, "first"); + await atomicWriteFileAsync(destination, "second"); + + expect(lstatSync(destination).isSymbolicLink()).toBe(false); + expect(readFileSync(destination, "utf8")).toBe("second"); + }); + + test("a dangling symlink is preserved and the write is refused", async () => { + const link = join(testDir, "dangling-async.toml"); + symlinkSync(join(testDir, "gone-async", "config.toml"), link); + + await expect(atomicWriteFileAsync(link, "recovered")).rejects.toThrow(/unresolvable symlinked write target/); + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(existsSync(join(testDir, "gone-async"))).toBe(false); + }); +}); diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 337b229108..550ad81998 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -1418,6 +1418,27 @@ describe("Responses previous_response_id state", () => { for (const path of [live, current, young, unrelated, directory]) expect(existsSync(path)).toBe(true); }); + test("load sweeps stale temps in a symlinked snapshot's real directory", () => { + // Atomic writes place their temp beside the RESOLVED target, so a dotfiles-managed + // config dir strands temps where a scan of the literal home would never find them. + const realDir = mkdtempSync(join(tmpdir(), "ocx-state-real-")); + const realSnapshot = join(realDir, "responses-state.json"); + writeFileSync(realSnapshot, JSON.stringify({ version: 2, states: [] })); + symlinkSync(realSnapshot, join(home, "responses-state.json")); + + const deadPid = process.pid === 4242 ? 4243 : 4242; + const stranded = join(realDir, `responses-state.json.ocx.${deadPid}.1.tmp`); + writeFileSync(stranded, "private state"); + const old = new Date(Date.now() - 60 * 60 * 1_000); + utimesSync(stranded, old, old); + + clearResponseStateMemoryForTests(); + previousResponseProviderState("trigger-load"); + + expect(existsSync(stranded)).toBe(false); + rmSync(realDir, { recursive: true, force: true }); + }); + test("stale temp recovery is best-effort when unlink fails", () => { const deadPid = process.pid === 4242 ? 4243 : 4242; const path = join(home, `responses-state.json.ocx.${deadPid}.1.tmp`); diff --git a/tests/server-auth.test.ts b/tests/server-auth.test.ts index 41ccefff9e..d67f9ea72f 100644 --- a/tests/server-auth.test.ts +++ b/tests/server-auth.test.ts @@ -632,6 +632,68 @@ describe("server local API auth", () => { } }); + test("extension allowlist gates preflight and data-plane requests by authority", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const extensionOrigin = "chrome-extension://modkelfkcfjpgbfmnbnllalkiogfofh"; + saveConfig({ + ...config("127.0.0.1"), + corsAllowOrigins: [extensionOrigin], + }); + stubModelDiscoveryFor("https://api.example.test"); + + const server = startServer(0); + const modelsUrl = new URL("/v1/models", server.url); + try { + const preflight = await fetch(modelsUrl, { + method: "OPTIONS", + headers: { + origin: extensionOrigin, + "access-control-request-method": "GET", + }, + }); + expect(preflight.status).toBe(204); + expect(preflight.headers.get("access-control-allow-origin")).toBe(extensionOrigin); + + const accepted = await fetch(modelsUrl, { headers: { origin: extensionOrigin } }); + expect(accepted.status).toBe(200); + expect(accepted.headers.get("access-control-allow-origin")).toBe(extensionOrigin); + + const rejected = await fetch(modelsUrl, { + headers: { origin: "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" }, + }); + expect(rejected.status).toBe(403); + + // The management plane shares isExtraAllowedOrigin: the configured extension gets + // its preflight echoed, any other extension is refused before reaching /api/*. + const managementUrl = new URL("/api/settings", server.url); + const managementPreflight = await fetch(managementUrl, { + method: "OPTIONS", + headers: { + origin: extensionOrigin, + "access-control-request-method": "GET", + }, + }); + expect(managementPreflight.status).toBe(204); + expect(managementPreflight.headers.get("access-control-allow-origin")).toBe(extensionOrigin); + + const managementRejected = await fetch(managementUrl, { + method: "OPTIONS", + headers: { + origin: "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "access-control-request-method": "GET", + }, + }); + expect(managementRejected.status).toBe(403); + expect(managementRejected.headers.get("access-control-allow-origin")).not.toBe( + "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + ); + } finally { + await server.stop(true); + } + }); + test("loopback management API rejects host-header same-origin rebinding", async () => { if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/server-loopback-host-gate.test.ts b/tests/server-loopback-host-gate.test.ts index 8c8a617fe6..e5937eeb16 100644 --- a/tests/server-loopback-host-gate.test.ts +++ b/tests/server-loopback-host-gate.test.ts @@ -96,3 +96,38 @@ describe("isAllowedRequestOrigin over a forwarded port", () => { ).toBe(false); }); }); + +describe("isAllowedRequestOrigin with extension origins", () => { + test("admits only the configured browser extension authority", () => { + const config = { + ...loopbackConfig, + corsAllowOrigins: ["chrome-extension://modkelfkcfjpgbfmnbnllalkiogfofh"], + } as OcxConfig; + + expect( + isAllowedRequestOrigin( + request("localhost:10100", "chrome-extension://modkelfkcfjpgbfmnbnllalkiogfofh"), + config, + ), + ).toBe(true); + expect( + isAllowedRequestOrigin( + request("localhost:10100", "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + config, + ), + ).toBe(false); + expect( + isAllowedRequestOrigin( + request("localhost:10100", "moz-extension://modkelfkcfjpgbfmnbnllalkiogfofh"), + config, + ), + ).toBe(false); + + expect( + isAllowedRequestOrigin( + request("localhost:10100", "chrome-extension://modkelfkcfjpgbfmnbnllalkiogfofh"), + { ...loopbackConfig, corsAllowOrigins: ["*"] } as OcxConfig, + ), + ).toBe(false); + }); +}); diff --git a/tests/test-home-guard.test.ts b/tests/test-home-guard.test.ts index 7c5958d5cc..9944f794e6 100644 --- a/tests/test-home-guard.test.ts +++ b/tests/test-home-guard.test.ts @@ -93,6 +93,32 @@ describe("real-home write guard", () => { expect(() => readFileSync(join(opencodexHome, "codex-accounts.json"))).toThrow(); }); + test("armed + a symlink escaping a temp home into the protected home: refused", () => { + // Atomic writes resolve their destination through symlinks, so a temp home whose + // config.json points into the protected home would otherwise pass the caller's + // dir-level check and then write the real file anyway. + const { realHome, opencodexHome } = sentinelHome(); + const protectedFile = join(opencodexHome, "config.json"); + writeFileSync(protectedFile, '{"sentinel":true}', "utf8"); + const dir = mkdtempSync(join(tmpdir(), "ocx-escape-home-")); + symlinkSync(protectedFile, join(dir, "config.json")); + + const probe = runProbe(` + import { saveConfig } from "${REPO_ROOT_URL}src/config"; + const REFUSAL = "refusing to write the real OpenCodex home"; + try { + saveConfig({ providers: {}, defaultProvider: "openai", port: 10100 } as never); + console.log("wrote"); + } catch (err) { + console.log(String(err).includes(REFUSAL) ? "refused" : "other"); + } + `, { OCX_TEST_HOME_GUARD: "1", OCX_REAL_HOME: realHome, OPENCODEX_HOME: dir }); + + expect(probe.stdout).toContain("refused"); + // The protected file must be byte-for-byte untouched. + expect(readFileSync(protectedFile, "utf8")).toBe('{"sentinel":true}'); + }); + test("armed + an unregistered temp home: writers succeed", () => { // The 54 suites that mkdtemp their own home must keep working with no opt-in. const dir = mkdtempSync(join(tmpdir(), "ocx-plain-home-"));