diff --git a/.devflow/features/external-model-routing/KNOWLEDGE.md b/.devflow/features/external-model-routing/KNOWLEDGE.md index a5a9e202..687aa303 100644 --- a/.devflow/features/external-model-routing/KNOWLEDGE.md +++ b/.devflow/features/external-model-routing/KNOWLEDGE.md @@ -1,11 +1,11 @@ --- feature: external-model-routing name: External Model Routing & Per-Agent Model Config -description: "Use when working on the proxy lifecycle (enable/disable/status/preflight), the ensure-proxy hook, per-agent model mapping, agent frontmatter rewriting, or the agents TUI. Keywords: proxy, external-model-routing, GPT, agent-models, ensure-proxy, frontmatter, devflow proxy, devflow agents, subswitch, ANTHROPIC_BASE_URL, CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT, dormancy, reapplyAgentMapping, runTui, flags-view, tui." +description: "Use when working on the proxy lifecycle (enable/disable/status/preflight), the ensure-proxy hook, per-agent model mapping, agent frontmatter rewriting, or the agents TUI. Keywords: proxy, external-model-routing, GPT, agent-models, ensure-proxy, frontmatter, devflow proxy, devflow agents, subswitch, ANTHROPIC_BASE_URL, CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT, dormancy, reapplyAgentMapping, runTui, flags-view, tui, proxyJsonExists, applyProxyTeardownToSettings, D-STRIP-1, mergeDevflowSettingsTemplate." category: architecture directories: [src/core/proxy-state.ts, src/core/external-models.ts, src/core/agent-models.ts, src/core/agent-state.ts, src/core/agent-frontmatter.ts, src/core/codex-auth-inspect.ts, src/core/model-discovery.ts, src/core/cache.ts, src/core/proxy-log.ts, src/cli/commands/proxy.ts, src/cli/commands/agents.ts, src/cli/agents-view, src/cli/tui, src/assets/scripts/hooks/ensure-proxy] created: 2026-07-24 -updated: 2026-08-25 +updated: 2026-09-01 --- # External Model Routing & Per-Agent Model Config @@ -18,7 +18,7 @@ Two authority sources govern the proxy at different points in its lifecycle. `ma ## System Context -The routing runtime is an internal package (`subswitch@0.2.0`, exact-pinned in `package.json`). Its name is a **hard branding constraint** — it must never appear in user-visible strings, error messages, CLI output, or agent context injections. User-facing vocabulary is always "external model routing" / "Devflow proxy". The one exception is internal code: health-check body comparisons (`body['name'] === 'subswitch'`), `SUBSWITCH_CONFIG` env var, and hook log lines are fine. +The routing runtime is an internal package (`subswitch@0.4.0`, exact-pinned in `package.json`). Its name is a **hard branding constraint** — it must never appear in user-visible strings, error messages, CLI output, or agent context injections. User-facing vocabulary is always "external model routing" / "Devflow proxy". The one exception is internal code: health-check body comparisons (`body['name'] === 'subswitch'`), `SUBSWITCH_CONFIG` env var, and hook log lines are fine. ## Proxy Lifecycle @@ -27,15 +27,17 @@ The routing runtime is an internal package (`subswitch@0.2.0`, exact-pinned in ` | File | Role | |------|------| | `~/.devflow/proxy.json` | Runtime authority. Tolerant-parsed by `readProxyState()`. ENOENT → default disabled state (not an error). Fields: `enabled`, `port`, `binPath`, `configPath`, `resolvedAt`, `devflowVersion`. | -| `~/.devflow/proxy-routing.json` | Routing config written by `buildRoutingConfigJson(port)`. Shape: bare `{port}` plus trailing newline. 0.2.0 rejects unrecognised keys — including a `codex` block breaks the runtime. Written before preflight runs on enable. | +| `~/.devflow/proxy-routing.json` | Routing config written by `buildRoutingConfigJson(port, existingContent?)`. Strict 5-key shape accepted by subswitch 0.4.0: `port`, `logLevel`, `anthropic`, `providers`, `limits`. Port-only on a fresh write — no `anthropic` block injected; the relay's own default governs (D-EFR-4). User-set `anthropic` sub-keys are preserved. Strips legacy sub-keys that are hard startup errors in 0.4.0: `anthropic.streamIdleTimeoutMs`, `limits.connectTimeoutMs`, `limits.maxConcurrentRequests`, `limits.maxBodyBytes` (see `ROUTING_CONFIG_REJECTED_SUBKEYS`). Written before preflight runs on enable. | | `manifest.features.proxy` | Init/uninstall authority. Seeds from prior manifest on re-init (ADR-014). Never in `config.json` — manifest-group by design, same as `ambient`/`hud`/`rules`. | **`isProxyEnabled()` is the sole dormancy authority**: it reads only `proxy.json` (never manifest). This is load-bearing: on preflight failure, `init.ts` converges `proxy.json` to `enabled:false` alongside manifest, hooks, and env — all four artifacts must agree (avoids PF-015). Any path that forces `proxyEnabled=false` must write `proxy.json enabled:false` so that a subsequent `isProxyEnabled()` call in the same process returns false correctly. +**`proxyJsonExists()` is the evidence discriminator** (D-STRIP-1): `readProxyState()` returns `Ok(defaultState)` on ENOENT — it cannot distinguish "file absent" from "file present with `DEFAULT_PROXY_PORT`". Callers that must gate on Devflow-managed evidence (init.ts env strip, uninstall cleanup phase) use `proxyJsonExists()` instead of inferring file presence from `readProxyState()`. + ### Enable path (crash-safe) 1. Read `proxy.json` for the remembered port; `resolvePort(portOption, priorPort)` picks the effective port. `--port` has **no commander default** — omission leaves `portOption` as `undefined` and the remembered port from `proxy.json` wins (TS-1 fix). -2. Write `proxy-routing.json` with the effective port (bare `{port}` JSON, no model list). +2. Write `proxy-routing.json` with the effective port via `buildRoutingConfigJson(port, existingContent)`, which reads the existing file and merges user customisations (logLevel, providers, anthropic sub-keys) while stripping rejected 0.3.0 sub-keys. No `anthropic` block is injected — the relay's own default governs. 3. Run `runProxyPreflight()` (4 ordered checks — ①–④: bin, codex auth, port probe/adoption, settings — see Preflight section). Doctor excluded: a pre-spawn gate is always unsatisfiable on a cold path (D-EFR-2; see Anti-Patterns). 4. On success: write `proxy.json` `enabled:true`. 5. Spawn relay via `spawnRelayAndWaitForPort()` (exported): bounded ≤50×100ms probe loop (5s max). `SpawnRelayResult.spawnedPid` is set when this process spawned the relay; absent on the adopted path. If relay never accepts, `rollbackProxyState` closure writes `proxy.json enabled:false` and returns error. @@ -51,7 +53,7 @@ Hard failures at any step (steps 1–9) set `process.exitCode = 1` and return The relay process is intentionally left running on `--disable` for any live Claude Code sessions. The disable path: 1. Read `proxy.json` first to determine `managedPort` for the URL strip. -2. `applyDisableToSettings(parsedSettings, managedPort)` — removes hooks AND strips `ANTHROPIC_BASE_URL` (port-scoped) and `CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT` (unconditional) (see invariant below). +2. `applyProxyTeardownToSettings(parsedSettings, managedPort)` — the unified teardown point: removes hooks always; when `managedPort` is defined delegates to `applyDisableToSettings` (hooks + port-scoped URL strip + unconditional window-var strip). 3. Writes `proxy.json` `enabled:false` — **keeps** `port`, `binPath`, `configPath`, `resolvedAt`, `devflowVersion` for the next enable. 4. Syncs manifest to `proxy: false`. 5. `revertExternalAgents()` — rewrites installed agent files to shipped default models. @@ -59,6 +61,30 @@ The relay process is intentionally left running on `--disable` for any live Clau Hard failures (e.g., malformed `settings.json`) set `process.exitCode = 1` and return early. +### `applyProxyTeardownToSettings` — unified teardown point + +```typescript +// Single teardown decision point for disable and uninstall. +// D-STRIP-1: hooks are always removed (Devflow's own artifacts); +// env vars are stripped only when managedPort is defined — the caller +// derives managedPort from an existing proxy.json, which is evidence +// that Devflow wrote those vars. When proxy.json is absent, passing +// undefined skips the env strip rather than clobbering a foreign gateway. +export function applyProxyTeardownToSettings( + settings: Settings, + managedPort: number | undefined, +): boolean { + if (managedPort === undefined) return removeProxyHooks(settings); + return applyDisableToSettings(settings, managedPort); +} +``` + +Uninstall's cleanup phase calls `proxyJsonExists()` to decide whether to pass a port or `undefined`: +- `proxy.json` present → read `proxy.json.port`, pass it as `managedPort` (strip env). +- `proxy.json` absent → pass `undefined` (hooks only). + +`applyDisableToSettings`'s **both-operations invariant** is unchanged when called via the managed path: `removeProxyHooks(s)` and `_stripProxyEnvFromObject(s, port)` both always evaluate — no short-circuit (avoids PF-015). + ### `applyDisableToSettings` — both-operations invariant ```typescript @@ -85,11 +111,17 @@ The regression that this guards against: `removeProxyHooks(s) || _stripProxyEnvF ④ readSettingsJson parseable; ANTHROPIC_BASE_URL not 'foreign'; API key warn (non-fatal) ``` +**Check ④ is extracted as `checkSettingsEnv(deps, port)` (D-EFR-5)**, shared by the adopted-relay path and the free-port path. The old ordering ran ④ only on the free-port branch — a healthy relay on the target port was erroneously buying an exemption from the foreign-gateway refusal. Now both branches call `checkSettingsEnv` before any early-return on adoption. + `spawnDoctor` is on `ProxyPreflightDeps` and built by `buildRealPreflightDeps` so `runEnable` can pass it into `runPostSpawnVerification` (step 6) via the same deps instance (`spawnDoctor: preflightDeps.spawnDoctor`). Preflight itself never calls `spawnDoctor`. **Init never runs doctor and never spawns.** `devflow init` calls `runProxyPreflight` (the same 4-check function) then writes `proxy.json enabled:true`. The relay is started by the first session's `ensure-proxy` hook. Deeper diagnostics (doctor, spawn) live in `devflow proxy --enable` and `devflow proxy --status`. -All four checks are injectable via `ProxyPreflightDeps`. **`buildRealPreflightDeps(opts)`** (exported) builds the production implementation and is shared between `runEnable` and `init.ts`. Key option: `swallowSettingsReadError: true` for init.ts (which writes `settings.json` itself); `false` for `runEnable` (propagates read errors to the user). +All four checks are injectable via `ProxyPreflightDeps`. **`buildRealPreflightDeps(opts)`** (exported) builds the production implementation and is shared between `runEnable` and `init.ts`. Key option: `swallowSettingsReadError: true` for init.ts (which writes `settings.json` itself); `false` for `runEnable` (propagates read errors to the user). `swallowSettingsReadError` semantics are preserved by the caller-supplied dep: when set, `readSettingsJson()` resolves to `'{}'` on I/O failure instead of rejecting. + +### init.ts proxy strip gating (D-STRIP-1) + +`init.ts` gates the proxy env strip on `proxyJsonExists()` — not on the state returned by `readProxyState()`. The guard is explicit in the source as a `D-STRIP-1` comment. When the file is absent on a machine that never had the proxy, no env stripping runs and no foreign gateway is clobbered. When the file is present, the port from `proxy.json` is passed to `_stripProxyEnvFromObject` as `managedPort`. ### Exported seams in proxy.ts @@ -101,7 +133,8 @@ All four checks are injectable via `ProxyPreflightDeps`. **`buildRealPreflightDe | `resolvePort(portOption, priorPort)` | Port resolution with remembered-port fallback | | `isOurRelayBody(body)` | Health-check identity check (`name === 'subswitch'`) | | `applyProxyEnv`, `stripProxyEnv` | Settings JSON string transforms (pure, no mutation) | -| `applyDisableToSettings` | Unconditional hooks-remove + URL-strip on parsed Settings object | +| `applyDisableToSettings` | Both-operations invariant: unconditional hooks-remove + URL-strip on parsed Settings object | +| `applyProxyTeardownToSettings` | Unified teardown for disable + uninstall: hooks always removed; env stripped only when `managedPort` defined (D-STRIP-1) | | `addProxyHooks`, `removeProxyHooks`, `hasProxyHooks` | Hook mutation helpers | | `runProxyPreflight`, `ProxyPreflightDeps`, `PreflightResult` | Preflight contract (4 checks) | | `PostSpawnDoctorDeps` | Injectable interface for post-spawn doctor verification | @@ -111,7 +144,7 @@ All four checks are injectable via `ProxyPreflightDeps`. **`buildRealPreflightDe | `formatExternalModelsLine(catalog, logPath)` | External models `--status` line; renders selectable model names or `unavailable` with the log path | | `realHttpGet(url, timeoutMs)` | Exported HTTP GET with three bounds: wall-clock deadline, 64KB body cap, `res.on('error')` handler. | -Internal named functions (not exported): `applyEnableSettingsPass`, `resolveProcessState`, `formatProcessLine`, `readPidFile`, `PROBE_TIMEOUT_MS`, `DOCTOR_TIMEOUT_MS`, `RELAY_SPAWN_*` constants. +Internal named functions (not exported): `applyEnableSettingsPass`, `checkSettingsEnv`, `resolveProcessState`, `formatProcessLine`, `readPidFile`, `PROBE_TIMEOUT_MS`, `DOCTOR_TIMEOUT_MS`, `RELAY_SPAWN_*` constants. ## ensure-proxy Hook Contract @@ -129,7 +162,7 @@ esac | UserPromptSubmit | any | **immediate exit 0 before any proxy-state reads** (silent; SessionStart handles all state) | | SessionStart | UP + correct identity | exit 0, no output | | SessionStart | UP + wrong identity | exit 0 + `json_session_output` warning ("port occupied by another application") | -| SessionStart | DOWN + missing bin/config | exit 0 + `json_session_output` warning ("relay binary not found" / "routing config not found") | +| SessionStart | DOWN + missing bin/config after re-resolution fails | exit 0 + `json_session_output` warning ("relay binary not found" / "routing config not found") | | SessionStart | DOWN + prerequisites ok | acquire spawn lock → nohup spawn → write `proxy.pid` (best-effort) → wait 80×0.1s = 8s → exit 0 [+warning if never up] | **`proxy.pid` is written immediately after spawn (best-effort)**, mirroring the CLI enable path. `devflow proxy --status` reads this file to display the process line for hook-started relays. A stale pid from a relay that never came up is harmless — `--status` liveness-checks it before display (`process.kill(pid, 0)`). @@ -138,6 +171,12 @@ esac **binPath/configPath are read only in the SessionStart-down branch** — deferred so the enabled+port check path pays zero additional json_field_file cost. +**binPath re-resolution on stale/missing path (D-FIX4)**: when the persisted `binPath` is absent or the file no longer exists (e.g., npx cache GC, devflow upgrade), the hook attempts two resolution strategies before emitting a warning: +- **Strategy (a)**: bounded walk from the resolved `devflow` CLI up to its nearest `node_modules/subswitch/package.json` ancestor. Walk guard: `_WALK_GUARD < 6` — at most 6 `dirname` steps. The `bin` field is read via `node -p` (env-var pass avoids quoting issues). Stops at the first `subswitch` dir found regardless of bin result. +- **Strategy (b)**: `command -v subswitch` — for globally-installed CLI installations. + +The healed path is used for the current session only and is never written back to `proxy.json` from the hook. The next `devflow proxy --enable` persists the corrected path. On re-resolution failure the original "relay binary not found" warning is emitted (avoids PF-009 — always exits 0). + **curl is guarded** with `command -v curl >/dev/null 2>&1` before the health-check identity call. When curl is absent, the hook assumes the relay is ours and exits 0 (no spurious warning). The CLI `--status` command is the authoritative identity check. **Health body parsed via `json_field`** — key-order-independent. The old substring match `*'"name":"subswitch"'*` was order-dependent; the current code pipes `$HEALTH_BODY` into `json_field "name" ""` (sourced from json-parse) and compares the extracted value. `json_field` is always available at this call site because line 33 exits the hook if `_JSON_AVAILABLE=false`. @@ -154,6 +193,35 @@ The spawn wait uses **80×0.1s = 8s** (hook) vs the CLI's **50×100ms = 5s**. Th **Hook spawn path is covered by tests** (tests/shell-hooks.test.ts): a stub relay reads `SUBSWITCH_CONFIG` and binds the port, asserting silent exit (exit 0, no stdout/stderr), a live pid recorded in `proxy.pid`, and spawn lock released. The failure branch (full 8s wait) is intentionally not unit-tested for duration reasons. +## post-install.ts: Settings Merge (D-SETTINGS-1) + +`installSettings` in `src/targets/claude-code/post-install.ts` **merges** the Devflow template into an existing `settings.json` rather than overriding it wholesale: + +- **Fresh file**: write the template directly. +- **Existing file**: call `mergeDevflowSettingsTemplate(existingParsed, templateParsed)` — adds Devflow hook entries absent from the file (idempotent by exact command string), sets `statusLine`/`attribution` only when the user has no existing value. +- **Parse failure**: warn and skip — the file is left byte-identical. A broken `settings.json` is never clobbered. +- **Foreign/unexpected hook shapes**: a per-event value that is not an array is left entirely untouched rather than overwritten or thrown on (applies PF-023). + +The old "override confirm" prompt is gone. The merge is purely additive (Devflow entries only), so no prompt is needed — declining could not protect user keys that the merge never touches, and the prompt rendered on top of the init spinner. + +`mergeDevflowSettingsTemplate` is exported for unit testing. Every shape check is at the mutation sink (not upstream) per PF-023. + +## subswitch 0.4.0 Routing Config Contract (D-EFR-4) + +`buildRoutingConfigJson(port, existingContent?)` builds `proxy-routing.json` with a strict 5-key shape (`port`, `logLevel`, `anthropic`, `providers`, `limits`) accepted by subswitch 0.4.0's `z.strictObject` schema. Unknown top-level keys cause a hard relay startup error — never emit them. + +**`ROUTING_CONFIG_REJECTED_SUBKEYS`** strips sub-keys that were valid in prior pinned versions but are hard startup errors in 0.4.0: +- `anthropic.streamIdleTimeoutMs` — valid in 0.2.0, removed in 0.3.0 +- `limits.connectTimeoutMs` — moved to `anthropic.connectTimeoutMs` in 0.3.0 +- `limits.maxConcurrentRequests` — valid in 0.2.0, removed in 0.3.0 +- `limits.maxBodyBytes` — valid through 0.3.0, renamed `limits.maxBufferedBodyBytes` in 0.4.0 + +**`anthropic.connectTimeoutMs`**: bounds only the DNS+TCP connect — armed on the socket, disarmed on the `'connect'` event. It never bounds the headers or stream phase, so it cannot cap a long-running request that has already connected. The relay's own 10 s default applies when the user has not set a value; `buildRoutingConfigJson` no longer injects one. A user-specified value is preserved as-is. + +**Body handling in 0.4.0**: bodies over the buffering window are streamed to Anthropic rather than rejected (route labels `anthropic:streamed` / `anthropic:streamed:unsniffed`, log field `bodyMode`). Only translated (Codex) routes still return 413 `request_too_large`. The 0.2.0-era synthesized 503 `overloaded_error` no longer exists — devflow never depended on it. + +`buildRoutingConfigJson` preserves user customisations from the existing file (logLevel, providers, anthropic sub-keys except rejected ones, limits sub-keys except rejected ones) so an upgrade from an older pinned version cannot leave the relay unable to boot. + ## Model Discovery (model-discovery.ts) `src/core/model-discovery.ts` provides live model catalog access via the relay. @@ -266,16 +334,7 @@ type AgentState = 'active' | 'saved-inactive' | 'not-installed' | 'unknown'; **`AGENT_STATE_LABELS`** — `Readonly>` mapping each state to its bare display text (no color applied; color is the call site's responsibility). Shared by `render.ts` (TUI STATE column) and `formatListOutput` in `agents.ts` (`--list` STATE column) so the two surfaces cannot drift from each other. -**`classifyAgentState(opts)`** — pure function with no I/O: -```typescript -interface ClassifyAgentStateOptions { - configured: string; // 'default' or a model name - proxyEnabled: boolean; - installed: boolean; // agent .md present in install dir - inRegistry: boolean; // agent name in plugin registry -} -``` -Classification rules (evaluated in order): +**`classifyAgentState(opts)`** — pure function with no I/O. Classification rules (evaluated in order): 1. `!inRegistry` → `'unknown'` (orphan rows from `agent-models.json` not in the registry) 2. `!installed` → `'not-installed'` 3. `isDormantExternalModel(configured, proxyEnabled)` → `'saved-inactive'` @@ -321,7 +380,7 @@ The TUI follows a pure-reducer / pure-renderer / thin-terminal-shell split (appl ## writeFileAtomicExclusive — Mode Preservation -`writeFileAtomicExclusive` (in `src/core/fs-atomic.ts`) now preserves the target file's permission mode across atomic replace: +`writeFileAtomicExclusive` (in `src/core/fs-atomic.ts`) preserves the target file's permission mode across atomic replace: 1. Write to `.tmp` with O_EXCL (crash-safe). 2. `stat(filePath)` to read the existing mode (permission bits only, masked with `0o777`). @@ -342,25 +401,30 @@ A user who hardened `settings.json` to `0600` (to protect `ANTHROPIC_API_KEY`) n - **D-EFR-3: Never mock the routing-runtime subprocess without a paired real-binary test**: any test that mocks the routing-runtime subprocess must be paired with at least one CI-executed test that does not. The specific trap (PF-016 reproduced exactly): `tests/integration/**` is excluded from `npm test` by `vitest.config.ts` while CI runs only `npm run build && npm test` — a real-binary test placed in `tests/integration/` would never execute in CI. Place real-binary tests in `tests/` (not `tests/integration/`). - **Calling model discovery from `validateSetArgs` or `--list`/`--reset`**: `validateSetArgs` uses `isValidModelName` (from agent-frontmatter.ts, pure regex) — not model discovery. The zero-spawn constraint for `--list`, `--set`, and `--reset` is a firm requirement pinned by module-boundary spy tests in `tests/agents-command.test.ts`. Importing or calling `discoverExternalModels`/`getExternalModelsCached` from the validation path breaks these tests and violates the configure-first-then-enable flow. - **Putting the agent-state classifier in external-models.ts**: `external-models.ts` is a leaf module with a single responsibility (dormancy predicate + Claude alias set). Agent state classification belongs in `src/core/agent-state.ts` (applies ADR-013). Adding classifier logic to external-models.ts would give it two reasons to change and break its leaf-module contract. +- **Emitting legacy sub-keys into proxy-routing.json**: `anthropic.streamIdleTimeoutMs`, `limits.connectTimeoutMs`, `limits.maxConcurrentRequests`, `limits.maxBodyBytes`, `limits.maxUpstreamSockets`, `limits.streamIdleTimeoutMs`, `limits.requestTimeoutMs`, and `limits.maxSseEventBytes` are hard startup errors in subswitch 0.4.0. Always build the config via `buildRoutingConfigJson` — never construct the JSON manually. +- **Gating env strip on `isProxyEnabled()` instead of `proxyJsonExists()`**: `readProxyState()` returns `Ok(defaultState)` on ENOENT — it cannot distinguish absence from a present file at the default port. Env strip must be gated on `proxyJsonExists()` (D-STRIP-1) to avoid clobbering a foreign gateway on machines that never had the proxy. +- **Skipping check ④ on the adopted-relay path**: a healthy relay on the port does not exempt the caller from the foreign-gateway refusal. `checkSettingsEnv(deps, port)` must run before any early-return on adoption (D-EFR-5). ## Gotchas - **`proxy.json` ENOENT is not an error**: `readProxyState()` returns a default disabled state when the file is missing. Callers that treat ENOENT as an error will get a false negative on fresh installs. +- **`proxyJsonExists()` vs `readProxyState()`**: `readProxyState()` returns `Ok(defaultState)` on ENOENT — it cannot differentiate "file absent" from "file present with `DEFAULT_PROXY_PORT`". Callers that must gate on managed-evidence (init env strip, uninstall cleanup) must call `proxyJsonExists()` separately (D-STRIP-1). - **Port adoption path**: if a relay is already accepting connections on the target port and the health check confirms our identity (`name === 'subswitch'`), preflight returns `adopted: true` and `spawnRelayAndWaitForPort` skips spawning. `spawnedPid` will be absent from `SpawnRelayResult` on this path — `runPostSpawnVerification` must never kill an adopted relay. -- **`stripProxyEnv` is port-scoped for the URL, unconditional for the window var (REG-1)**: `stripProxyEnv(settingsJson, managedPort)` removes `ANTHROPIC_BASE_URL` **only when its value exactly matches `http://127.0.0.1:`** (protecting foreign gateways on any other port), but removes `CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT` unconditionally — Devflow is its sole producer, so there is no foreign value to protect. A localhost URL on any other port classifies as `'ours-other-port'` or `'foreign'` and is never touched. Callers must pass the port Devflow owns (from `proxy.json.port` or `DEFAULT_PROXY_PORT`). `readProxyEnvState` uses the pattern `^http://127\.0\.0\.1:\d+$` to classify any localhost URL as `'ours-other-port'` for display purposes only — the strip never uses that broad pattern. +- **`stripProxyEnv` window var is unconditional when managedPort is defined (REG-1)**: `_stripProxyEnvFromObject(settings, managedPort)` removes `CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT` unconditionally (Devflow is its sole producer) and removes `ANTHROPIC_BASE_URL` only when its value exactly matches `http://127.0.0.1:`. This unconditional removal only runs when the caller has verified `proxyJsonExists()` — the evidence gate is the outer guard (D-STRIP-1), not the inner strip. - **Remembered port on re-enable**: `--port` has no commander default. When `--port` is omitted, `portOption` is `undefined` and `resolvePort(undefined, priorPort)` returns the remembered port from `proxy.json`. - **Dormant TUI rows**: when proxy is off and an agent has a saved GPT model, `buildRow()` calls `isDormantExternalModel()` and sets `configuredModel='default'` with the GPT name in `dormantModel`. `persistedModelFor(row)` returns `dormantModel` for an untouched dormant row, so `mergeTuiRowsIntoMapping` preserves the GPT mapping entry byte-identical on save even though `configuredModel` shows `'default'`. - **`binPath` must be spawned with `node `**: npm does not guarantee executable bits on installed package binaries. Always spawn as `node `, never `` directly. - **Leaked stub relays**: proxy tests that spawn stub relays must reap them on the failure path too — not only the happy path. Use `afterEach`/`onTestFinished` with SIGTERM→SIGKILL escalation and confirm death via `process.kill(pid, 0)`. Real incident: three orphaned stub relays accumulated over ~3 weeks; a full run stretched from ~24 seconds to 40+ minutes and produced 13–21 spurious failures in unrelated files (memory pipeline, capture hooks) that were repeatedly misdiagnosed as product defects. - **`resolveProxyBin()` uses `createRequire(import.meta.url)`**: ESM-safe way to resolve CommonJS package paths. The `require.resolve('subswitch/package.json')` approach finds the package relative to devflow's own `node_modules`, not the user's project. -- **env -i corporate-TLS**: `NODE_EXTRA_CA_CERTS` is now included in both the hook's `_RELAY_ENV` array and `scrubChildEnv()` in `proxy-log.ts`, so corporate-TLS deployments that supply a CA bundle via this var will have it forwarded to the relay. It is included only when non-empty (an empty path causes TLS errors). `NODE_OPTIONS` remains excluded from both allowlists — it permits arbitrary code execution via `--require`/`--import`. A drift-guard test in `tests/proxy-log.test.ts` asserts the hook's conditional append exists, so a future one-sided change is caught early rather than silently breaking one spawn path. +- **env -i corporate-TLS**: `NODE_EXTRA_CA_CERTS` is included in both the hook's `_RELAY_ENV` array and `scrubChildEnv()` in `proxy-log.ts`, so corporate-TLS deployments that supply a CA bundle via this var will have it forwarded to the relay. It is included only when non-empty (an empty path causes TLS errors). `NODE_OPTIONS` remains excluded from both allowlists — it permits arbitrary code execution via `--require`/`--import`. A drift-guard test in `tests/proxy-log.test.ts` asserts the hook's conditional append exists, so a future one-sided change is caught early rather than silently breaking one spawn path. - **`classifyCodexAuthReadError` is directly testable**: the ENOENT→`{kind:'absent'}` vs other-error→`{kind:'unreadable'}` classification that used to live inside `runStatus` is now exported from `codex-auth-inspect.ts`. Tests can import and call it with a synthetic error object without needing to mock the filesystem. - **`isProxyEnabled()` is the sole dormancy authority**: it reads only `proxy.json`. On preflight failure in `init.ts`, `proxy.json` is explicitly written to `enabled:false` so `isProxyEnabled()` returns the correct value for the `reapplyAgentMapping` call that follows. Any new code path that forces the proxy off must write this file — relying on manifest alone is insufficient. - **`readInstalledAgentNames` degrades on any error, not just ENOENT**: the catch block is bare (`catch {}`) — EPERM, ENOTDIR, and any other OS error all return an empty set rather than throwing (avoids PF-009). A misconfigured install path must not crash the TUI or `--list`. +- **subswitch 0.3.0 `connectTimeoutMs` semantics changed**: in 0.2.0 it was an inactivity/socket timeout that killed long requests; in 0.3.0 it is strictly a DNS+TCP connect budget (armed on socket, disarmed on `'connect'`). It cannot cap a long-running request that has already connected. `buildRoutingConfigJson` no longer injects a default — the relay's own 10 s budget governs, and a user-set value is preserved. ## Key Files -- `src/core/proxy-state.ts` — ProxyState schema, read/write, `isProxyEnabled()`, `resolveProxyBin()`, `buildRoutingConfigJson()` +- `src/core/proxy-state.ts` — ProxyState schema, read/write, `isProxyEnabled()`, `proxyJsonExists()` (evidence discriminator, D-STRIP-1), `resolveProxyBin()`, `buildRoutingConfigJson()` (0.4.0 5-key shape, `ROUTING_CONFIG_REJECTED_SUBKEYS`, user-set anthropic keys preserved; no injection) - `src/core/external-models.ts` — `CLAUDE_MODEL_ALIASES` (as const), `ClaudeModelAlias` literal union, `isClaudeModelName()`, `isDormantExternalModel()` (leaf module, no project imports) - `src/core/agent-state.ts` — `AgentState` type, `AGENT_STATE_LABELS` record, `classifyAgentState()` — single vocabulary for the STATE column shared by `--list` and the TUI (applies ADR-013; leaf module, imports only external-models.ts) - `src/core/agent-frontmatter.ts` — pure frontmatter rewriter, `readFrontmatterModel()`, `rewriteAgentFrontmatter()`, `isValidModelName()` (MODEL_NAME_RE — imported by validateSetArgs for zero-spawn charset validation) @@ -370,24 +434,27 @@ A user who hardened `settings.json` to `0600` (to protect `ANTHROPIC_API_KEY`) n - `src/core/cache.ts` — `modelCacheDir(devflowDir)` and `hudCacheDir(devflowDir)` path accessors (single source of truth for cache layout — avoids PF-013); `parseRawEnvelope()` (single canonical envelope parser, rejects non-finite ttl); `readCache()`, `writeCache()` with 0700/0600 permissions; `pruneOldEntries()` (keeps 3 entries by timestamp, reaps `.json.tmp.*` orphans) - `src/core/proxy-log.ts` — `scrubChildEnv()` (allowlist-based env for relay spawn; paired with hook's `env -i` allowlist), `openProxyLog()`, `rotateProxyLogIfLarge()` - `src/core/fs-atomic.ts` — `writeFileAtomicExclusive()` — mode-preserving atomic write -- `src/cli/commands/proxy.ts` — `proxyCommand`; exported seams: `buildRealPreflightDeps`, `spawnRelayAndWaitForPort`, `runPostSpawnVerification`, `resolvePort`, `isOurRelayBody`, `runProxyPreflight`, `applyProxyEnv`, `stripProxyEnv`, `applyDisableToSettings`, `addProxyHooks`, `removeProxyHooks`, `hasProxyHooks`, `readProxyEnvState`, `formatCodexAuthLine` (returns `{level,msg}`), `realHttpGet`, `PostSpawnDoctorDeps` +- `src/cli/commands/proxy.ts` — `proxyCommand`; exported seams: `buildRealPreflightDeps`, `spawnRelayAndWaitForPort`, `runPostSpawnVerification`, `resolvePort`, `isOurRelayBody`, `runProxyPreflight`, `applyProxyEnv`, `stripProxyEnv`, `applyDisableToSettings`, `applyProxyTeardownToSettings` (unified teardown, D-STRIP-1), `addProxyHooks`, `removeProxyHooks`, `hasProxyHooks`, `readProxyEnvState`, `formatCodexAuthLine`, `realHttpGet`, `PostSpawnDoctorDeps` - `src/cli/commands/agents.ts` — `agentsCommand`, `validateSetArgs()` (calls `isValidModelName`, zero-spawn), `applySetMapping()`, `buildListRows()`, `mergeTuiRowsIntoMapping()` (consumes `persistedModelFor`/`persistedEffortFor`) - `src/cli/agents-view/state.ts` — pure reducer, `buildRow()`, `isDirtyModel()`, `isDirtyEffort()`, `persistedModelFor()`, `persistedEffortFor()`, `rowState()` (delegates to `classifyAgentState`), `unsavedCount()` - `src/cli/agents-view/render.ts` — pure frame renderer; `COL_STATE = 14`; exports `FIXED_ROWS`, `computeViewportHeight` - `src/cli/agents-view/terminal.ts` — thin adapter over the shared `runTui` driver (`src/cli/tui/terminal.ts`); exports `runAgentsTui()`, re-exports `TuiIO` and `MAX_KEYPRESSES` from tui/ - `src/cli/tui/terminal.ts` — generic `runTui` driver (`RunTuiSpec`: `signalAction: Exclude`, `continueIntent: C`, `screen?: 'alt'|'inline'`), `normalizeKey`, `TuiIO`, `MAX_KEYPRESSES`, `RenderDims`, `INLINE_MARGIN`; agents-view uses `signalAction='cancel'` + default `'alt'` screen; flags-view uses `signalAction='abort'` + `'inline'` screen - `src/cli/tui/cells.ts` — cell helper utilities (shared across TUI modules) -- `src/assets/scripts/hooks/ensure-proxy` — SessionStart + UserPromptSubmit hook; writes `proxy.pid` after spawn; UserPromptSubmit exits before proxy-state reads; relay spawned via `env -i` 6-var allowlist -- `src/cli/commands/init.ts` — proxy preflight block (4-check, no doctor, no spawn); `reapplyAgentMapping` guard after preflight; convergence writes `proxy.json enabled:false` on preflight failure +- `src/assets/scripts/hooks/ensure-proxy` — SessionStart + UserPromptSubmit hook; binPath re-resolution (bounded walk max 6 + `command -v subswitch`); writes `proxy.pid` after spawn; UserPromptSubmit exits before proxy-state reads; relay spawned via `env -i` 6-var allowlist +- `src/targets/claude-code/post-install.ts` — `mergeDevflowSettingsTemplate()` (D-SETTINGS-1: merge not overwrite; idempotent by exact command string; shape-guarded; exported for testing); `installSettings()` — no override confirm prompt; parse failure writes nothing +- `src/cli/commands/init.ts` — proxy preflight block (4-check, no doctor, no spawn); `proxyJsonExists()` gates env strip (D-STRIP-1); `reapplyAgentMapping` guard after preflight; convergence writes `proxy.json enabled:false` on preflight failure +- `src/cli/commands/uninstall.ts` — imports `applyProxyTeardownToSettings`; cleanup phase calls `proxyJsonExists()` to determine `managedPort` (present → pass port; absent → pass `undefined`) ## Related - **ADR-013**: src/core vs src/cli boundary — all state I/O and pure logic in `src/core/`; CLI orchestration and user-facing action handlers in `src/cli/`. The proxy feature is the canonical multi-module example of this split. `src/core/agent-state.ts` is a new application: moving the agent-state classifier out of `external-models.ts` into its own core leaf module gives it a single responsibility and a clear home (applies ADR-013). - **ADR-014**: state-aware re-init — `proxy` is seeded from `manifest?.features.proxy ?? FEATURE_DEFAULTS.proxy` in `resolveSeedFeatures`. On `--reset`, seeds as `false`. Never read from `config.json`. -- **PF-009**: all proxy artifact removals in uninstall/disable are non-fatal; preflight failure warns but never aborts `devflow init` — `proxyEnabled` is simply forced to `false`. Also: `readInstalledAgentNames` degrades to empty set on any error; `writeFileAtomicExclusive` chmod step is non-fatal. +- **PF-009**: all proxy artifact removals in uninstall/disable are non-fatal; preflight failure warns but never aborts `devflow init` — `proxyEnabled` is simply forced to `false`. Also: `readInstalledAgentNames` degrades to empty set on any error; `writeFileAtomicExclusive` chmod step is non-fatal; ensure-proxy binPath re-resolution is best-effort (always exits 0). - **PF-013**: cache path accessors (`modelCacheDir`, `hudCacheDir`) in `cache.ts` prevent write-site/removal-site drift — the canonical PF-013 shape applied to the model-discovery cache. - **PF-014**: no `process.exit()` inside finally-guarded scopes — TUI cleanup wired via Promise `resolve()`; hard failures in CLI commands set `process.exitCode = 1` and return. -- **PF-015**: every path that forces proxy off must converge `proxy.json enabled:false` alongside manifest/hooks/env — `isProxyEnabled()` reads only `proxy.json`, so this file is the load-bearing convergence point. +- **PF-015**: every path that forces proxy off must converge `proxy.json enabled:false` alongside manifest/hooks/env — `isProxyEnabled()` reads only `proxy.json`, so this file is the load-bearing convergence point. Also: both-operations invariant in `applyDisableToSettings` — neither `removeProxyHooks` nor `_stripProxyEnvFromObject` may be short-circuited. - **PF-017**: relay spawn uses `env -i` 6-var allowlist (hook) + `scrubChildEnv()` (CLI spawn) — both allowlists must be kept in sync for corporate-TLS users. +- **PF-023**: validation at the sink that mutates — `mergeDevflowSettingsTemplate` shape-guards every `hooks` value before touching it; foreign/unexpected hook shapes left untouched rather than overwritten. - **PF-001**: port digit-validated before /dev/tcp interpolation in `ensure-proxy`. - Feature knowledge: `installer-shadowing` — covers `resolveSeedFeatures`, manifest-group feature seeding, and uninstall artifact cleanup patterns that proxy extends. diff --git a/.devflow/features/index.md b/.devflow/features/index.md index 5e9bc21d..cf1948ce 100644 --- a/.devflow/features/index.md +++ b/.devflow/features/index.md @@ -4,5 +4,5 @@ - **resolve-pipeline** — src/assets/commands/resolve.mds, src/assets/agents/triage.md, src/assets/agents/code.md, src/core/plugins.ts, src/assets/commands/code-review.mds — Use when modifying /resolve or /code-review convergence logic, adding or changing Triage disposition rules (including DUPLICATE collapsing), adjusting Code-agent operating modes (issue-fix/validation-fix), touching the resolution-summary.md parser contract, changing the Verification Gate retry loop, understanding how DIFF_FILES flows from git validate-branch into blast-radius triage, or working on traceability operations (fetch-review-threads, resolve-review-threads, post-resolution-summary, check-merge-readiness, THREAD_MAP). Keywords: resolve, triage, disposition matrix, blast-radius, FIX_NOW, FIX_SEPARATE, TECH_DEBT, FALSE_POSITIVE, BY_DESIGN, ESCALATED, DUPLICATE, duplicate-grouping, duplicates-collapse, duplicate_of, resolution-summary, convergence parser, DIFF_FILES, issue-fix, validation-fix, Verification Gate, manage-debt, COMPLIANCE_SKILL_INSTALLED, TRACEABILITY DEGRADED, fetch-review-threads, THREAD_MAP, post-resolution-summary, Third-Party Threads, check-merge-readiness, ext-N, D7, D9, PF-024. - **installer-shadowing** — src/targets/claude-code/installer.ts, src/targets/claude-code/legacy.ts, src/cli/commands/init.ts, src/cli/commands/init-seed.ts, src/cli/commands/uninstall.ts, src/cli/commands/rules.ts, src/cli/commands/skills.ts, src/cli/commands/flags.ts, src/cli/flags-view, src/cli/tui, src/core/plugins.ts, src/core/assets.ts, src/core/paths.ts, src/core/manifest.ts, src/core/flags.ts, src/core/feature-config.ts, src/core/orphan-sweep.ts, src/core/migrations.ts, src/cli/commands/compliance-prompts.ts — Use when modifying the install pipeline (installViaFileCopy, installAllRules, composeScripts, InstallReport), adding or changing skill/rule shadow override logic, touching uninstall scope (enumerateUserDevFlowContent, removeDevFlowInstallArtifacts, resolveDevflowDirCleanup, installArtifactPaths, sweepDevflowNamespaces, resolveProjectDataCleanup) or install-artifact cleanup, extending the CLI skills/rules/flags management commands, working with asset directory accessors (rulesDir, skillsDir, commandsDir) and package-root resolution, modifying the init seeding layer (resolveInitSeed, resolveSeedFeatures, resolveSeedFlags, resolveSeedPlugins, --reset, FlagsRecord, knownPlugins, readConfigIfPresent, resolveExistingViewMode, getAllCommandNames, proxy), working on the flags TUI (FlagsViewState, FlagRow, buildFlagRows, collectFlagRecord, effectiveDisplay, blurb, inline mode, RunTuiSpec screen) or the flags CLI (createFlagsCommand, lookupFlag, persistFlagConfig, formatFlagValue), or working on the compliance wizard step (shouldRunComplianceStep, runComplianceStep, modePromptShown, CompliancePromptIO). Keywords: installViaFileCopy, installAllRules, composeScripts, InstallReport, RuleInstallOutcome, SkillShadowState, RuleShadowState, shadow, unshadow, validateSkillShadow, validateRuleShadow, seedRuleShadow, prefixSkillName, unprefixSkillName, devflow:, skills, rules, uninstall, EISDIR, enumerateUserDevFlowContent, removeDevFlowInstallArtifacts, resolveDevflowDirCleanup, installArtifactPaths, enumerateDryRunExtras, sweepDevflowNamespaces, resolveProjectDataCleanup, runDryRunPhase, runSelectivePhaseForScope, runFullPhaseForScope, runCleanupPhase, getPackageRoot, isContainedIn, rulesDir, skillsDir, agentsDir, commandsDir, scriptsDir, LEGACY_SKILL_NAMES, sweepOrphanedAssets, SweepResult, sweepOrphans, sweepFailures, SweepFailure, mdFileName, mdEntryName, orphan sweep, getAllSkillNames, getAllCommandNames, getAllAgentNames, DELETED_PLUGIN_NAMES, EXCLUDED, resolveInitSeed, resolveSeedFeatures, resolveSeedFlags, resolveSeedPlugins, resolveResetGatedInputs, applyCliToggles, FlagsRecord, FlagsRecordValue, getDefaultFlagsRecord, parseManifestFlags, migrateLegacyFlagsToRecord, sanitizeFlagsRecord, coerceFlagValue, parseFlagValueInput, neutralValueOf, isNeutral, countActiveFlags, readViewMode, knownPlugins, readConfigIfPresent, resolveExistingViewMode, resolveFinalViewMode, reset, init-seed, proxy, reapplyAgentMapping, revertExternalAgents, agent-models.json, proxy.json, proxy-routing.json, proxy.pid, applyDisableToSettings, buildRealPreflightDeps, canonicalise-agent-keys-v1, AnyMigration, migrations.json, compliance-prompts, shouldRunComplianceStep, CompliancePromptIO, runComplianceStep, modePromptShown, createFlagsCommand, lookupFlag, persistFlagConfig, FlagsViewState, FlagRow, buildFlagRows, collectFlagRecord, buildStops, cycleForward, cycleBackward, sanitizeCell, padToVisible, truncateVisible, effectiveDisplay, EffectiveDisplay, formatFlagValue, blurb, FlagDefCommon, INLINE_MARGIN, cursorUp, RunTuiSpec, screen, inline. - **learning-capture-system** — src/assets/scripts/hooks, src/assets/agents/learning.md, src/cli/commands/learning.ts, src/core/feature-config.ts, src/core/learning-tuning-config.ts, src/hud/components/learning-counts.ts, src/assets/commands/_partials — Use when modifying capture hooks (capture-prompt/capture-turn/capture-question), the learning or memory pending-turns queues, the Learning agent (src/assets/agents/learning.md), the session-start-context learning directive, the feature-config toggles, the learning tuning config, the decisions content files (decisions.md/pitfalls.md/index.md) or their ledger ops, or the devflow learning CLI. Keywords: capture-prompt, capture-turn, capture-question, queue-append, pending-turns, memory-worker, Learning agent, learning directive, LEARNING MAINTENANCE, DEVFLOW_BG_UPDATER, learning-lock, queue_read_gates, decisions_load, DECISIONS_CONTEXT, feature-config, config.json, learning.json, decisions-ledger, assign-anchor, retire-anchor, refresh-anchor, render-decisions, staged-write CAS, WORKING-MEMORY.md.new, segmentDetails, amendments, is-hex-sha, verify_and_swap, compute_commits_since_note, divergence guard, isSafeRawBody. -- **external-model-routing** — src/core/proxy-state.ts, src/core/external-models.ts, src/core/agent-models.ts, src/core/agent-state.ts, src/core/agent-frontmatter.ts, src/core/codex-auth-inspect.ts, src/core/model-discovery.ts, src/core/cache.ts, src/core/proxy-log.ts, src/cli/commands/proxy.ts, src/cli/commands/agents.ts, src/cli/agents-view, src/cli/tui — Use when working on the proxy lifecycle (enable/disable/status/preflight), the ensure-proxy hook, per-agent model mapping, agent frontmatter rewriting, or the agents TUI. Keywords: proxy, external-model-routing, GPT, agent-models, ensure-proxy, frontmatter, devflow proxy, devflow agents, subswitch, ANTHROPIC_BASE_URL, dormancy, reapplyAgentMapping. +- **external-model-routing** — src/core/proxy-state.ts, src/core/external-models.ts, src/core/agent-models.ts, src/core/agent-state.ts, src/core/agent-frontmatter.ts, src/core/codex-auth-inspect.ts, src/core/model-discovery.ts, src/core/cache.ts, src/core/proxy-log.ts, src/cli/commands/proxy.ts, src/cli/commands/agents.ts, src/cli/agents-view, src/cli/tui — Use when working on the proxy lifecycle (enable/disable/status/preflight), the ensure-proxy hook, per-agent model mapping, agent frontmatter rewriting, or the agents TUI. Keywords: proxy, external-model-routing, GPT, agent-models, ensure-proxy, frontmatter, devflow proxy, devflow agents, subswitch, ANTHROPIC_BASE_URL, dormancy, reapplyAgentMapping, proxyJsonExists, applyProxyTeardownToSettings, D-STRIP-1, mergeDevflowSettingsTemplate, subswitch 0.4.0. - **compliance-feature** — src/core/compliance.ts, src/targets/claude-code/compliance-install.ts, src/cli/commands/compliance.ts, src/assets/skills/compliance, src/assets/rules/compliance.md, src/assets/agents/git.md, src/assets/commands/code-review.mds, src/assets/commands/plan.mds, src/assets/commands/implement.mds, src/assets/commands/resolve.mds, src/assets/commands/release.md — Use when adding or modifying the compliance feature (framework registry, converge contract, CLI, rule stamping), changing how host commands resolve COMPLIANCE_SKILL_INSTALLED, modifying traceability operations in the Git agent (learn-conventions, issue-first, thread resolution, shipped markers, release evidence), or extending the D4 DEGRADED contract. Keywords: compliance, COMPLIANCE_SKILL_INSTALLED, convergeComplianceArtifacts, convergeFromManifest, frameworks, FEATURE_OWNED_SKILLS, traceability, D4, D9, gather-release-evidence, conventions.md, resolve-review-threads, ensure-traceable-issue, stamper, manifest-group, ComplianceFeatureState. diff --git a/CHANGELOG.md b/CHANGELOG.md index 697e9f74..4fe9ebb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **Routing runtime pinned to `subswitch@0.4.0`** (from `0.2.0`). Over-window Anthropic-bound request bodies are now streamed upstream instead of being rejected by the relay, so long prompts no longer fail at the proxy; only translated (Codex) routes still return `413 request_too_large`. `buildRoutingConfigJson` no longer injects `anthropic.connectTimeoutMs` — the relay's own default (10 s, connect-only) governs; user-set values are preserved as before. The injection was a 0.2.0-era workaround artifact that outlived its purpose once the key's semantics were narrowed to DNS+TCP connect only. + +### Fixed +- **`proxy-routing.json` upgrade safety**: `buildRoutingConfigJson` now strips all seven `limits.*` sub-keys that `subswitch@0.4.0` promotes to hard startup errors: `limits.maxBodyBytes` (renamed `limits.maxBufferedBodyBytes`), `limits.maxUpstreamSockets` (moved to `anthropic.maxUpstreamSockets`), `limits.streamIdleTimeoutMs` (moved to `providers.codex.streamIdleTimeoutMs`), `limits.requestTimeoutMs` (moved to `providers.codex.requestTimeoutMs`), and `limits.maxSseEventBytes` (moved to `providers.codex.maxSseEventBytes`), joining the existing strips for `anthropic.streamIdleTimeoutMs`, `limits.connectTimeoutMs`, and `limits.maxConcurrentRequests`. A hand-edited config carrying any of these keys would have killed the relay on every session start — spawned by the `ensure-proxy` hook, with no route back. The `limits` block is now omitted entirely when all its sub-keys are stripped (matching the `anthropic` block's existing omit-when-empty behaviour). + +**Upgrade**: no action required. If you hand-edited `~/.devflow/proxy-routing.json` to set any of the seven stripped `limits.*` keys, move their values to the 0.4.0 target paths (`limits.maxBufferedBodyBytes`, `anthropic.maxUpstreamSockets`, or the appropriate `providers.codex.*` key); the next `devflow proxy --enable` drops the stale keys for you. + --- ## [2.3.0] - 2026-08-31 diff --git a/CLAUDE.md b/CLAUDE.md index 3b89ddca..6a2fa177 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,7 +63,7 @@ Debug logs stored at `~/.devflow/logs/{project-slug}/`. Knowledge write-back is in-command (not a background pipeline): gated by `devflow knowledge --enable/--disable` (flips `knowledge` in feature config); Knowledge agent writes directly at workflow end. -**External Model Routing (Devflow Proxy)**: Routes Devflow agents through GPT models via an OpenAI/Codex subscription using a local relay. Feature state is manifest-gated (like ambient/hud/rules, per ADR-001): `manifest.features.proxy` is the source of truth; `~/.devflow/proxy.json` holds runtime authority (enabled, port, binPath, configPath, resolvedAt, devflowVersion). `~/.devflow/proxy-routing.json` holds the routing config (port only — bare `{port}` object; the 0.2.0 routing runtime rejects unrecognised keys). The `ensure-proxy` hook (SessionStart + UserPromptSubmit, registered/removed by `addProxyHooks`/`removeProxyHooks`) auto-starts the relay when a session begins. `ANTHROPIC_BASE_URL` and `CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT` are injected into (and stripped from) `settings.json` at CLI enable/disable time via `applyProxyEnv`/`stripProxyEnv`, not by the hook (the window var is removed unconditionally on strip; the URL delete is port-scoped to avoid clobbering foreign gateways). Toggle via `devflow proxy --enable/--disable/--status` or via the Advanced init wizard. Enabling runs `runProxyPreflight` (4 checks: bin, codex auth, port, settings), spawns the relay, then gates on a post-spawn doctor verification against the live relay (doctor requires a running relay to pass); on doctor failure the enable rolls back, killing the relay only if it spawned it. Init runs the same preflight but never spawns or runs doctor — the first session's ensure-proxy hook starts the relay; on init preflight failure: warning + force-disabled, init never aborted (avoids PF-009). Disabling reverts agent frontmatter to Claude defaults but preserves the model mapping for re-enable. Default OFF; Advanced-only — never part of Recommended defaults. +**External Model Routing (Devflow Proxy)**: Routes Devflow agents through GPT models via an OpenAI/Codex subscription using a local relay. Feature state is manifest-gated (like ambient/hud/rules, per ADR-001): `manifest.features.proxy` is the source of truth; `~/.devflow/proxy.json` holds runtime authority (enabled, port, binPath, configPath, resolvedAt, devflowVersion). `~/.devflow/proxy-routing.json` holds the routing config; `buildRoutingConfigJson` writes port-only on a fresh write (no `anthropic` block injected — the relay's own default governs); user-set keys in `logLevel`/`anthropic`/`providers`/`limits` blocks are preserved from existing content; the 0.4.0 routing runtime rejects unrecognised top-level keys and hard-fails on registered legacy keys — `anthropic.streamIdleTimeoutMs`, `limits.connectTimeoutMs`, `limits.maxConcurrentRequests`, `limits.maxBodyBytes`, `limits.maxUpstreamSockets`, `limits.streamIdleTimeoutMs`, `limits.requestTimeoutMs`, `limits.maxSseEventBytes` — which `buildRoutingConfigJson` strips. The `ensure-proxy` hook (SessionStart + UserPromptSubmit, registered/removed by `addProxyHooks`/`removeProxyHooks`) auto-starts the relay when a session begins; hook removal is unconditional on disable. `ANTHROPIC_BASE_URL` and `CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT` are injected into (and stripped from) `settings.json` at CLI enable/disable time via `applyProxyEnv`/`applyProxyTeardownToSettings`, not by the hook; env stripping is evidence-gated on `~/.devflow/proxy.json` existing (`proxyJsonExists()`); the URL delete is port-scoped to avoid clobbering foreign gateways; preflight refuses a foreign `ANTHROPIC_BASE_URL` before adopting a healthy relay; the `ensure-proxy` hook re-resolves a missing `binPath` before warning. Toggle via `devflow proxy --enable/--disable/--status` or via the Advanced init wizard. Enabling runs `runProxyPreflight` (4 checks: bin, codex auth, port, settings), spawns the relay, then gates on a post-spawn doctor verification against the live relay (doctor requires a running relay to pass); on doctor failure the enable rolls back, killing the relay only if it spawned it. Init runs the same preflight but never spawns or runs doctor — the first session's ensure-proxy hook starts the relay; on init preflight failure: warning + force-disabled, init never aborted (avoids PF-009). Disabling reverts agent frontmatter to Claude defaults but preserves the model mapping for re-enable. Default OFF; Advanced-only — never part of Recommended defaults. **Per-Agent Model Configuration**: User overrides to agent model assignments persist in `~/.devflow/agent-models.json` (deviations only — absent entry = shipped default). `reapplyAgentMapping` runs after every `devflow init` post-install to re-apply user overrides to freshly copied agent files. `revertExternalAgents` reverts all agents to shipped defaults (called on proxy disable and before agent removal on uninstall). GPT model assignments are **dormant** when routing is off — they are stored in `agent-models.json` but not written to agent frontmatter until routing is enabled. Manage via `devflow agents` TUI or `devflow agents --list/--set/--reset`. Core source files: `src/core/agent-frontmatter.ts` (pure rewrite engine), `src/core/agent-models.ts` (schema + apply/revert), `src/core/external-models.ts` (CLAUDE_MODEL_ALIASES, isClaudeModelName, isDormantExternalModel — leaf module), `src/core/model-discovery.ts` (discoverExternalModels, getExternalModelsCached, cache-warming), `src/core/cache.ts` (cache read/write, 0700/0600 permissions, parseRawEnvelope), `src/core/proxy-log.ts` (scrubChildEnv, openProxyLog, relay env allowlisting), `src/core/proxy-state.ts` (state I/O), `src/cli/commands/proxy.ts` (CLI + hook wiring), `src/cli/commands/agents.ts` (CLI), `src/cli/agents-view/` (TUI — state, render, terminal; thin adapter over the shared `src/cli/tui/` driver). @@ -199,7 +199,7 @@ Per-project runtime files live under `.devflow/`: ~/.devflow/ ├── proxy.json # Proxy runtime state (enabled, port, binPath, configPath, resolvedAt, devflowVersion) — global, not per-project -├── proxy-routing.json # Routing config (port only — bare {port} object) read by the ensure-proxy hook +├── proxy-routing.json # Routing config (port-only by default; user-set anthropic/logLevel/providers/limits preserved) read by the ensure-proxy hook ├── proxy.pid # Relay PID — written by CLI enable and by the ensure-proxy hook spawn (transient) ├── .proxy-spawn.lock/ # Hook spawn lock dir — prevents concurrent session double-spawn (transient) ├── agent-models.json # Per-agent model overrides (deviations only; absent = shipped default) diff --git a/package-lock.json b/package-lock.json index 9283dc93..bb4ca712 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,7 +12,7 @@ "@clack/prompts": "^0.9.1", "commander": "^12.0.0", "picocolors": "^1.1.1", - "subswitch": "0.2.0" + "subswitch": "0.4.0" }, "bin": { "devflow": "dist/cli.js" @@ -1522,9 +1522,9 @@ "license": "MIT" }, "node_modules/subswitch": { - "version": "0.2.0", - "resolved": "https://registry.npmjs.org/subswitch/-/subswitch-0.2.0.tgz", - "integrity": "sha512-x07ACfkE495sBTZ6bV/O9jdARWdPcE09Jtaq0rg1o0vHgFKx+zc1nv4tZvxIWRyfZPH1f0XgKf+0njBi1Ylkcw==", + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/subswitch/-/subswitch-0.4.0.tgz", + "integrity": "sha512-z/lLW59foN60NdNIdCb7obz0jWvQoAQ2WbOI5edtiqyTjCfeI7hrgi6Rnbdytx3M4qQ8to7haPYYCVoLUBSeFA==", "license": "MIT", "dependencies": { "@clack/prompts": "^1.7.0", diff --git a/package.json b/package.json index 6d6760c5..9bcfacbf 100644 --- a/package.json +++ b/package.json @@ -63,7 +63,7 @@ "@clack/prompts": "^0.9.1", "commander": "^12.0.0", "picocolors": "^1.1.1", - "subswitch": "0.2.0" + "subswitch": "0.4.0" }, "devDependencies": { "@mdscript/mds": "0.2.0", diff --git a/src/assets/scripts/hooks/ensure-proxy b/src/assets/scripts/hooks/ensure-proxy index 856add49..be3125c8 100644 --- a/src/assets/scripts/hooks/ensure-proxy +++ b/src/assets/scripts/hooks/ensure-proxy @@ -181,12 +181,71 @@ PROXY_BIN=$(json_field_file "$PROXY_STATE_FILE" "binPath" "") PROXY_CONFIG=$(json_field_file "$PROXY_STATE_FILE" "configPath" "") dbg "PROXY_BIN=$PROXY_BIN PROXY_CONFIG=$PROXY_CONFIG" -# Validate prerequisites before spawning +# Validate prerequisites before spawning. +# D-FIX4: when the persisted binPath is missing or stale (e.g. npx cache GC, upgrade), +# attempt dependency-light re-resolution before warning. Two strategies: +# a) Walk up from `devflow` CLI to find devflow's node_modules/subswitch. +# b) `command -v subswitch` — globally installed CLI. +# Re-resolution is best-effort: on failure the existing warning is emitted and the +# hook exits 0 as before. The healed path is used for this session only; the next +# successful `devflow proxy --enable` persists it to proxy.json. (avoids PF-001, +# avoids PF-009: non-fatal on failure, always exits 0) if [ -z "$PROXY_BIN" ] || [ ! -f "$PROXY_BIN" ]; then - log "prereq fail: binPath missing or not a file: $PROXY_BIN" - CONTEXT="[Devflow proxy] Warning: relay binary not found. Run 'devflow proxy --enable' to restore external model routing." - json_session_output "$CONTEXT" - exit 0 + log "prereq: binPath missing or not a file (${PROXY_BIN:-}) — attempting re-resolution" + _RESOLVED_BIN="" + + # Resolve node for re-resolution strategies. + _NODE_FOR_RESOLVE=$(command -v node 2>/dev/null || true) + + # Strategy a: locate devflow CLI → walk up to find node_modules/subswitch + if [ -n "$_NODE_FOR_RESOLVE" ]; then + _DF_CMD=$(command -v devflow 2>/dev/null || true) + if [ -n "$_DF_CMD" ]; then + _DF_REAL=$(realpath "$_DF_CMD" 2>/dev/null || readlink -f "$_DF_CMD" 2>/dev/null || echo "$_DF_CMD") + _WALK_DIR=$(dirname "$_DF_REAL") + _WALK_GUARD=0 + while [ "$_WALK_GUARD" -lt 6 ] && [ -n "$_WALK_DIR" ] && [ "$_WALK_DIR" != "/" ]; do + _SW_PKG="$_WALK_DIR/node_modules/subswitch/package.json" + if [ -f "$_SW_PKG" ]; then + # Read bin field from package.json using node (already confirmed present). + # Env-var pass avoids shell-quoting issues with paths containing spaces. + _BIN_REL=$(SUBSWITCH_PKG_JSON="$_SW_PKG" \ + "$_NODE_FOR_RESOLVE" -p \ + "try{var p=JSON.parse(require('fs').readFileSync(process.env.SUBSWITCH_PKG_JSON,'utf-8'));var b=p.bin;typeof b==='string'?b:b&&b.subswitch?b.subswitch:''}catch(e){''}" \ + 2>/dev/null) || _BIN_REL="" + if [ -n "$_BIN_REL" ] && [ "$_BIN_REL" != "undefined" ]; then + _CAND="$(dirname "$_SW_PKG")/$_BIN_REL" + if [ -f "$_CAND" ]; then + _RESOLVED_BIN="$_CAND" + log "re-resolved binPath via devflow walk: $_RESOLVED_BIN" + fi + fi + break # found subswitch dir — stop walking regardless of bin result + fi + _WALK_DIR=$(dirname "$_WALK_DIR") + _WALK_GUARD=$(( _WALK_GUARD + 1 )) + done + fi + fi + + # Strategy b: subswitch globally installed as a CLI + if [ -z "$_RESOLVED_BIN" ]; then + _SW_GLOBAL=$(command -v subswitch 2>/dev/null || true) + if [ -n "$_SW_GLOBAL" ] && [ -f "$_SW_GLOBAL" ]; then + _RESOLVED_BIN="$_SW_GLOBAL" + log "re-resolved binPath via command -v subswitch: $_RESOLVED_BIN" + fi + fi + + if [ -n "$_RESOLVED_BIN" ]; then + PROXY_BIN="$_RESOLVED_BIN" + log "binPath healed for this session: $PROXY_BIN" + else + log "re-resolution failed — binPath not found" + CONTEXT="[Devflow proxy] Warning: relay binary not found. Run 'devflow proxy --enable' to restore external model routing." + json_session_output "$CONTEXT" + exit 0 + fi fi NODE_BIN=$(command -v node 2>/dev/null || true) diff --git a/src/cli/commands/init.ts b/src/cli/commands/init.ts index e89d0fca..a1fa1c4c 100644 --- a/src/cli/commands/init.ts +++ b/src/cli/commands/init.ts @@ -35,7 +35,7 @@ import { addCaptureHooks, removeCaptureHooks } from './capture.js'; import { removeDreamHook } from './legacy-hooks.js'; import { addProxyHooks, removeProxyHooks, applyProxyEnv, stripProxyEnv, runProxyPreflight, buildRealPreflightDeps } from './proxy.js'; import { reapplyAgentMapping, readAgentMapping } from '../../core/agent-models.js'; -import { readProxyState, writeProxyState, buildProxyState, buildRoutingConfigJson, DEFAULT_PROXY_PORT } from '../../core/proxy-state.js'; +import { readProxyState, writeProxyState, buildProxyState, buildRoutingConfigJson, DEFAULT_PROXY_PORT, proxyJsonExists } from '../../core/proxy-state.js'; import type { Settings } from '../../targets/claude-code/hooks.js'; import { stripDevflowTeammateModeFromJson } from '../../core/teammate-mode-cleanup.js'; // Settings/HookMatcher types used by hook utilities — each in their own module @@ -1635,14 +1635,18 @@ export const initCommand = new Command('init') content = JSON.stringify(parsedSettings, null, 2) + '\n'; } // Proxy env: ANTHROPIC_BASE_URL strip-then-add, scoped to managed port. - // Read proxy.json to learn which port we own — only that URL is stripped. - // A user's own localhost gateway on any other port is preserved. + // D-STRIP-1: only strip when proxy.json exists — evidence that Devflow previously + // wrote ANTHROPIC_BASE_URL. Without this gate, a fresh init on a machine where + // DEFAULT_PROXY_PORT (4141) happens to be a user's own gateway (LiteLLM etc.) + // would silently delete both their URL and the window-enforcement var. // Invariant: proxy.json always reflects the final settled state after the // preflight block above — all paths that force proxyEnabled=false also write // proxy.json enabled:false (avoids PF-015), so managedPort == effectivePort. - const proxyStateForStrip = await readProxyState(devflowDir); - const managedPort = proxyStateForStrip.ok ? proxyStateForStrip.value.port : DEFAULT_PROXY_PORT; - content = stripProxyEnv(content, managedPort); + if (await proxyJsonExists(devflowDir)) { + const proxyStateForStrip = await readProxyState(devflowDir); + const managedPort = proxyStateForStrip.ok ? proxyStateForStrip.value.port : DEFAULT_PROXY_PORT; + content = stripProxyEnv(content, managedPort); + } if (proxyEnabled) content = applyProxyEnv(content, effectivePort); if (content !== original) { diff --git a/src/cli/commands/proxy.ts b/src/cli/commands/proxy.ts index ddb91b83..c1ab9271 100644 --- a/src/cli/commands/proxy.ts +++ b/src/cli/commands/proxy.ts @@ -29,6 +29,7 @@ import { buildProxyState, buildRoutingConfigJson, proxyBaseUrl, + proxyJsonExists, resolveProxyBin, DEFAULT_PROXY_PORT, } from '../../core/proxy-state.js'; @@ -187,9 +188,11 @@ function _stripProxyEnvFromObject(settings: Settings, managedPort: number): bool const env = s.env as Record | undefined; if (!env) return false; - // Devflow is the only producer of this var — always remove it, regardless of whether - // the URL is still ours. Port-scoping protects a FOREIGN url value; there is no - // foreign value of this key to protect. (applies PF-015, ADR-003) + // D-STRIP-1: every REMOVAL caller (init, uninstall, runDisable) first proves Devflow + // managed the proxy — `proxyJsonExists()` — and the enable caller is taking ownership + // of the key anyway, so reaching this line means the value is Devflow's to remove. + // Removal is therefore unconditional: unlike ANTHROPIC_BASE_URL (port-scoped below), + // this key has no foreign value to protect. (applies PF-015, ADR-003) const hadWindowVar = env[UNKNOWN_MODEL_WINDOW_ENV] !== undefined; delete env[UNKNOWN_MODEL_WINDOW_ENV]; @@ -335,6 +338,34 @@ export function applyDisableToSettings(settings: Settings, managedPort: number): return removedHooks || strippedEnv; } +/** + * Settings teardown for every path that turns the proxy off: `devflow proxy --disable` + * and `devflow uninstall`. + * + * D-STRIP-1: the two removals answer to different evidence, so they are gated + * separately. + * - Hooks are Devflow's own artifacts — removed unconditionally. Leaving them behind + * points later sessions at a hook script that no longer exists, and makes a re-run + * of an interrupted uninstall unable to clean up. + * - The env vars (ANTHROPIC_BASE_URL, CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT) + * are stripped only against `managedPort`, which the caller supplies from an existing + * `proxy.json` — the one piece of evidence that Devflow ever wrote them. Pass + * `undefined` when that file is absent: the values then belong to a gateway Devflow + * never managed (a user's own LiteLLM on 4141, say) and must survive untouched. + * + * When `managedPort` is defined this delegates to `applyDisableToSettings`, so the + * both-operations invariant documented there still holds. + * + * Mutates settings in place. Returns true when any change was made. + */ +export function applyProxyTeardownToSettings( + settings: Settings, + managedPort: number | undefined, +): boolean { + if (managedPort === undefined) return removeProxyHooks(settings); + return applyDisableToSettings(settings, managedPort); +} + /** * Check whether the ensure-proxy hook is registered on at least one event. * Returns true if present on either SessionStart or UserPromptSubmit. @@ -410,6 +441,59 @@ export interface PreflightResult { adopted: boolean; } +/** + * Preflight check ④ — settings.json readable and parseable, no foreign + * ANTHROPIC_BASE_URL, plus the non-fatal ANTHROPIC_API_KEY warning. + * + * Extracted so the adopted-relay path and the free-port path run the identical + * check (D-EFR-5): a healthy relay on the port must not buy an exemption from the + * foreign-gateway refusal. Keeping it in one function is what stops the two paths + * from drifting on which sub-checks they apply or which message they return. + * + * `swallowSettingsReadError` semantics are preserved by the caller-supplied dep: + * when that flag is set (init), readSettingsJson() resolves to '{}' on I/O failure + * instead of rejecting, so the read-failure branch is reachable only from runEnable. + */ +async function checkSettingsEnv( + deps: ProxyPreflightDeps, + port: number, +): Promise> { + let settingsJson: string; + try { + settingsJson = await deps.readSettingsJson(); + } catch { + return Err('Could not read settings.json — check file permissions'); + } + + let parsedSettings: Record; + try { + parsedSettings = JSON.parse(settingsJson) as Record; + } catch { + return Err('settings.json is malformed — fix it before enabling the proxy'); + } + + if (readProxyEnvState(settingsJson, port) === 'foreign') { + return Err( + 'An existing ANTHROPIC_BASE_URL in settings.json points to a different gateway — Devflow will not overwrite it', + ); + } + + // API key warning (non-fatal) + const envBlock = parsedSettings.env; + if ( + typeof envBlock === 'object' && + envBlock !== null && + !Array.isArray(envBlock) && + typeof (envBlock as Record).ANTHROPIC_API_KEY === 'string' + ) { + deps.onWarn?.( + 'ANTHROPIC_API_KEY is set in settings.json — requests will use that key through the local relay', + ); + } + + return Ok(undefined); +} + /** * Run preflight checks before enabling the Devflow proxy. * @@ -418,6 +502,8 @@ export interface PreflightResult { * ② ~/.codex/auth.json exists. * ③ Port probe: free → OK; accepting → health check → adopt or fail. * ④ settings.json parseable; ANTHROPIC_BASE_URL not pointing elsewhere; API key warn. + * Runs on BOTH outcomes of ③ that can proceed — adopted relay and free port + * (D-EFR-5) — via the shared `checkSettingsEnv` helper. * * Doctor is deliberately excluded from preflight: the relay's doctor subcommand * probes the relay port — a not-yet-started relay makes that probe fail (exit 1). A @@ -453,6 +539,11 @@ export async function runProxyPreflight( PROBE_TIMEOUT_MS, ); if (healthResult.ok && isOurRelayBody(healthResult.value)) { + // D-EFR-5: run check ④ BEFORE the adopted early-return. The old order + // (adopted-return first, settings check only when the port was free) let a + // healthy relay on the port silently skip the foreign-gateway refusal entirely. + const settingsCheck = await checkSettingsEnv(deps, port); + if (!settingsCheck.ok) return Err(settingsCheck.error); return Ok({ binPath, npxWarning, adopted: true }); } // Port accepting but health timed out, failed, or not our relay @@ -463,39 +554,8 @@ export async function runProxyPreflight( // Port refused — free to proceed // ④ Settings.json check - let settingsJson: string; - try { - settingsJson = await deps.readSettingsJson(); - } catch { - return Err('Could not read settings.json — check file permissions'); - } - - let parsedSettings: Record; - try { - parsedSettings = JSON.parse(settingsJson) as Record; - } catch { - return Err('settings.json is malformed — fix it before enabling the proxy'); - } - - const envState = readProxyEnvState(settingsJson, port); - if (envState === 'foreign') { - return Err( - 'An existing ANTHROPIC_BASE_URL in settings.json points to a different gateway — Devflow will not overwrite it', - ); - } - - // API key warning (non-fatal) - const envBlock = parsedSettings.env; - if ( - typeof envBlock === 'object' && - envBlock !== null && - !Array.isArray(envBlock) && - typeof (envBlock as Record).ANTHROPIC_API_KEY === 'string' - ) { - deps.onWarn?.( - 'ANTHROPIC_API_KEY is set in settings.json — requests will use that key through the local relay', - ); - } + const settingsCheck = await checkSettingsEnv(deps, port); + if (!settingsCheck.ok) return Err(settingsCheck.error); return Ok({ binPath, npxWarning, adopted: false }); } @@ -1709,6 +1769,13 @@ async function runDisable(): Promise { // applyDisableToSettings strips ANTHROPIC_BASE_URL only when the URL // port matches the port Devflow manages — callers must supply it. Reading // proxy.json here also consolidates state for Step 2 below. + // + // D-STRIP-1: readProxyState() cannot distinguish "file absent" from "file present + // with the default port", so the env strip is gated on the file's existence — the + // only evidence that Devflow ever wrote those vars. Must be read before Step 2 + // creates the file. Hook removal is NOT gated: our hooks are ours to remove on + // every path. + const proxyManaged = await proxyJsonExists(devflowDir); const priorStateResult = await readProxyState(devflowDir); const priorState = priorStateResult.ok ? priorStateResult.value : null; const managedPort = priorState?.port ?? DEFAULT_PROXY_PORT; @@ -1730,7 +1797,10 @@ async function runDisable(): Promise { return; } - const changed = applyDisableToSettings(parsedSettings, managedPort); + const changed = applyProxyTeardownToSettings( + parsedSettings, + proxyManaged ? managedPort : undefined, + ); if (changed) { // Guard ENOSPC/EACCES — unhandled rejection leaves proxy in partial state try { diff --git a/src/cli/commands/uninstall.ts b/src/cli/commands/uninstall.ts index c4c5b210..66245d12 100644 --- a/src/cli/commands/uninstall.ts +++ b/src/cli/commands/uninstall.ts @@ -15,8 +15,8 @@ import { removeCaptureHooks } from './capture.js'; import { removeDreamHook } from './legacy-hooks.js'; import { removeHudStatusLine } from './hud.js'; import { removeContextHook } from './context.js'; -import { applyDisableToSettings } from './proxy.js'; -import { readProxyState, DEFAULT_PROXY_PORT } from '../../core/proxy-state.js'; +import { applyProxyTeardownToSettings } from './proxy.js'; +import { readProxyState, proxyJsonExists } from '../../core/proxy-state.js'; import { hudCacheDir } from '../../core/cache.js'; import { revertExternalAgents } from '../../core/agent-models.js'; import type { Settings } from '../../targets/claude-code/hooks.js'; @@ -717,7 +717,10 @@ export async function runCleanupPhase(opts: { /** Pre-captured managed proxy ports per scope, read BEFORE artifact removal. * proxy.json is deleted by removeDevFlowInstallArtifacts (inside runFullPhaseForScope), * so reading it inside this phase would always yield the DEFAULT_PROXY_PORT fallback - * and leave a non-default ANTHROPIC_BASE_URL (e.g. port 4200) unstripped. (F6) */ + * and leave a non-default ANTHROPIC_BASE_URL (e.g. port 4200) unstripped. (F6) + * + * A scope with no entry means proxy.json did not exist — the env vars are not + * Devflow's to remove and only the hooks come out (D-STRIP-1). */ managedProxyPorts?: ReadonlyMap; }): Promise { const { scopesToUninstall, keepDocs, verbose, cwd, isTTY } = opts; @@ -814,14 +817,16 @@ export async function runCleanupPhase(opts: { settingsContent = stripFlags(settingsContent); // also strips viewMode via view-mode registry entry settingsContent = stripDevflowTeammateModeFromJson(settingsContent); // Remove proxy hooks and ANTHROPIC_BASE_URL env in a single parse-mutate-serialize pass. - // REG-1: scope the URL strip to the port Devflow manages — use the pre-captured port - // (from opts.managedProxyPorts) because proxy.json is already deleted by - // removeDevFlowInstallArtifacts before this phase runs. A user's own localhost - // gateway on any other port is left in settings untouched. + // D-STRIP-1: an absent entry in managedProxyPorts means proxy.json did not exist + // before artifact removal — no evidence Devflow ever wrote the env vars, so + // applyProxyTeardownToSettings removes only our hooks and leaves the user's + // ANTHROPIC_BASE_URL / CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT alone. + // Hook removal is never gated: our hooks must not survive an uninstall. + // REG-1: the port-scoped URL strip protects any foreign localhost gateway on a + // different port — the managedPort value is from the pre-captured proxy.json port. { - const managedPort = opts.managedProxyPorts?.get(scope) ?? DEFAULT_PROXY_PORT; const parsedSettings = JSON.parse(settingsContent) as Settings; - applyDisableToSettings(parsedSettings, managedPort); + applyProxyTeardownToSettings(parsedSettings, opts.managedProxyPorts?.get(scope)); settingsContent = JSON.stringify(parsedSettings, null, 2) + '\n'; } @@ -1047,14 +1052,22 @@ export const uninstallCommand = new Command('uninstall') // Pre-capture managed proxy ports for each scope BEFORE artifact removal. // proxy.json is deleted by removeDevFlowInstallArtifacts (inside runFullPhaseForScope), // so reading it after the full phase would always yield DEFAULT_PROXY_PORT. (F6) + // + // D-STRIP-1: only populate managedProxyPorts when proxy.json exists — evidence that + // Devflow has managed the proxy on this machine. runCleanupPhase gates the + // applyProxyTeardownToSettings call on the port being present in the map, so when the file + // is absent (never managed), no env strip is attempted and a user's own gateway vars + // are left untouched. const managedProxyPorts = new Map(); if (!isSelectiveUninstall) { for (const scope of scopesToUninstall) { try { const paths = await getInstallationPaths(scope); - const proxyState = await readProxyState(paths.devflowDir); - if (proxyState.ok) managedProxyPorts.set(scope, proxyState.value.port); - } catch { /* non-fatal: if we can't read, we fall back to DEFAULT_PROXY_PORT */ } + if (await proxyJsonExists(paths.devflowDir)) { + const proxyState = await readProxyState(paths.devflowDir); + if (proxyState.ok) managedProxyPorts.set(scope, proxyState.value.port); + } + } catch { /* non-fatal */ } } } diff --git a/src/core/proxy-log.ts b/src/core/proxy-log.ts index 7823c493..dbf6d2bf 100644 --- a/src/core/proxy-log.ts +++ b/src/core/proxy-log.ts @@ -33,8 +33,10 @@ export const PROXY_LOG_TAIL_BYTES = 1_048_576; * (e.g. SUBSWITCH_CONFIG for the relay spawn and doctor spawn). * * applies ADR-003: the prior denylist rationale is gone — the routing runtime - * reads exactly three env vars (ANTHROPIC_API_KEY, FORCE_COLOR, SUBSWITCH_CONFIG). - * Verified by whole-dist grep of the 0.2.0 package. An allowlist is the correct + * reads exactly four env vars (ANTHROPIC_API_KEY, FORCE_COLOR, NO_COLOR, + * SUBSWITCH_CONFIG — NO_COLOR read in dist/tty.js with presence semantics; CI is + * read only by the `init` subcommand which devflow never invokes). + * Verified by whole-dist grep of the 0.4.0 package. An allowlist is the correct * shape: 61 inherited vars → 6. * * HOME is retained: the runtime's loadConfig resolves ~ paths via homedir(). diff --git a/src/core/proxy-state.ts b/src/core/proxy-state.ts index 9ff4861e..5a68aa9a 100644 --- a/src/core/proxy-state.ts +++ b/src/core/proxy-state.ts @@ -126,49 +126,74 @@ export async function writeProxyState( // --------------------------------------------------------------------------- /** - * Accepted top-level keys for the subswitch 0.2.0 FileConfigSchema (z.strictObject). + * Accepted top-level keys for the subswitch 0.4.0 FileConfigSchema (z.strictObject). * Unknown top-level keys cause a hard relay startup error — never emit them. * - * @D-EFR-4 Subswitch 0.2.0 routing config contract: + * @D-EFR-4 Subswitch 0.4.0 routing config contract: * FileConfigSchema is a z.strictObject with exactly 5 accepted top-level keys: * port, logLevel, anthropic, providers, limits. Unknown keys cause a hard startup * error — the relay refuses to start. anthropic and limits are themselves * strictObject + prefault({}), so they may be partially specified. - * - * connectTimeoutMs default: subswitch 0.2.0 applies anthropic.connectTimeoutMs - * as upstream.setTimeout() at anthropic-passthrough.js:86 — a socket *inactivity* - * timeout, not a connect timeout. The 10s default kills any Anthropic request - * whose upstream takes >10s to emit the first response byte — routine for long - * opus requests (confirmed: 99 spurious 504s clustered at 10004-10098ms while - * successful requests ran 25s-99s). Default overridden to 120 000ms; a - * user-specified value always wins. - * - * limits.connectTimeoutMs is a registered LEGACY KEY that causes a hard startup - * error. Strip it from the limits block when preserving existing config. */ const ROUTING_CONFIG_ALLOWED_TOP_KEYS = new Set([ 'port', 'logLevel', 'anthropic', 'providers', 'limits', ]); -/** Default anthropic.connectTimeoutMs injected when not specified by the user (ms). */ -export const DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS = 120_000; +/** + * Sub-keys of a preserved `anthropic` / `limits` block that the pinned relay rejects + * outright — each is a registered LEGACY KEY, and a legacy key is a hard startup + * error naming its replacement, not a warning. They are stripped when carrying a + * user's existing config forward so an upgrade cannot leave the relay unable to boot. + * + * Every entry was valid under a version devflow previously pinned, which is what makes + * it reachable in a real user's file: + * anthropic.streamIdleTimeoutMs — valid in 0.2.0, removed in 0.3.0 (relay no longer + * bounds the stream-idle phase on a connected client). + * limits.connectTimeoutMs — moved to anthropic.connectTimeoutMs in 0.3.0; + * stripping it loses nothing (relay default governs). + * limits.maxConcurrentRequests — valid in 0.2.0, removed in 0.3.0 (admission gate + * removed). + * limits.maxBodyBytes — valid through 0.3.0, renamed to + * limits.maxBufferedBodyBytes in 0.4.0. The old spelling is a registered legacy + * key, so leaving it in place would stop the relay from booting. + * limits.maxUpstreamSockets — valid in 0.3.0, moved to anthropic.maxUpstreamSockets + * in 0.4.0's providers.* restructure. + * limits.streamIdleTimeoutMs — valid in 0.3.0, moved to + * providers.codex.streamIdleTimeoutMs in 0.4.0. + * limits.requestTimeoutMs — valid in 0.3.0, moved to + * providers.codex.requestTimeoutMs in 0.4.0. + * limits.maxSseEventBytes — valid in 0.3.0, moved to + * providers.codex.maxSseEventBytes in 0.4.0. + * + * Keys retired before the 0.2.0 baseline are deliberately absent: only keys reachable + * in a config written by a previously pinned devflow version belong on this list. + */ +const ROUTING_CONFIG_REJECTED_SUBKEYS: Readonly> = { + anthropic: ['streamIdleTimeoutMs'], + limits: [ + 'connectTimeoutMs', + 'maxConcurrentRequests', + 'maxBodyBytes', + 'maxUpstreamSockets', + 'streamIdleTimeoutMs', + 'requestTimeoutMs', + 'maxSseEventBytes', + ], +}; /** * Build the routing config JSON for ~/.devflow/proxy-routing.json. * * Authoritatively sets `port`. Preserves any existing valid `anthropic`, - * `limits`, `logLevel`, and `providers` blocks from `existingContent`, - * filtering out unknown top-level keys and the legacy `limits.connectTimeoutMs` - * field (which causes a hard relay startup error in 0.2.0). - * - * Injects a default `anthropic.connectTimeoutMs` of 120 000ms when the user - * has not set one — see @D-EFR-4 for the rationale. + * `limits`, `logLevel`, and `providers` blocks from `existingContent`, filtering + * out unknown top-level keys and every sub-key in ROUTING_CONFIG_REJECTED_SUBKEYS + * (each one a hard relay startup error). No `anthropic` block is injected when + * the user has not set one — the relay's own default governs (D-EFR-4). * - * If `existingContent` is missing or malformed, falls back to a clean config - * with only `port` and `anthropic.connectTimeoutMs`. + * If `existingContent` is missing or malformed, falls back to a port-only config. * * applies ADR-013: pure core-layer function — no I/O. - * @D-EFR-4: see note above for the strict top-key constraint and timeout rationale. + * @D-EFR-4: see note above for the strict top-key constraint. */ export function buildRoutingConfigJson(port: number, existingContent?: string): string { // Parse existing config tolerantly; a malformed file falls back to clean defaults. @@ -196,26 +221,29 @@ export function buildRoutingConfigJson(port: number, existingContent?: string): config.logLevel = preserved.logLevel; } - // Anthropic block: preserve user settings; inject connectTimeoutMs default when absent. - // @D-EFR-4: the 10s upstream inactivity timeout kills long opus requests. - const existingAnthropic = + // Anthropic block: preserve user settings; strip legacy rejected sub-keys. + // No default is injected — the relay's own default governs (D-EFR-4). + if ( typeof preserved.anthropic === 'object' && preserved.anthropic !== null && !Array.isArray(preserved.anthropic) - ? { ...(preserved.anthropic as Record) } - : {}; - if (!Object.prototype.hasOwnProperty.call(existingAnthropic, 'connectTimeoutMs')) { - existingAnthropic.connectTimeoutMs = DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS; + ) { + const existingAnthropic = { ...(preserved.anthropic as Record) }; + for (const key of ROUTING_CONFIG_REJECTED_SUBKEYS.anthropic) { + delete existingAnthropic[key]; + } + if (Object.keys(existingAnthropic).length > 0) { + config.anthropic = existingAnthropic; + } } - config.anthropic = existingAnthropic; // Preserve providers block if present. if (preserved.providers !== undefined) { config.providers = preserved.providers; } - // Preserve limits block if present; strip legacy limits.connectTimeoutMs that - // causes a hard startup error in 0.2.0 (use anthropic.connectTimeoutMs instead). + // Preserve limits block if present, minus the sub-keys the relay hard-errors on + // (see ROUTING_CONFIG_REJECTED_SUBKEYS). if (preserved.limits !== undefined) { if ( typeof preserved.limits === 'object' && @@ -223,8 +251,12 @@ export function buildRoutingConfigJson(port: number, existingContent?: string): !Array.isArray(preserved.limits) ) { const limitsObj = { ...(preserved.limits as Record) }; - delete limitsObj['connectTimeoutMs']; - config.limits = limitsObj; + for (const key of ROUTING_CONFIG_REJECTED_SUBKEYS.limits) { + delete limitsObj[key]; + } + if (Object.keys(limitsObj).length > 0) { + config.limits = limitsObj; + } } else { config.limits = preserved.limits; } @@ -267,6 +299,29 @@ export function proxyBaseUrl(port: number): string { // isProxyEnabled — the primary contract other modules consume // --------------------------------------------------------------------------- +/** + * Returns true when ~/.devflow/proxy.json exists on disk — i.e. Devflow has + * previously managed the proxy on this machine. + * + * This is the evidence gate used by init and uninstall before stripping + * ANTHROPIC_BASE_URL / CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT: + * we only strip those vars when we can prove Devflow wrote them. + * + * D-STRIP-1: gate proxy env stripping on devflow-managed evidence. + * readProxyState() returns Ok(defaultState) on ENOENT — it cannot distinguish + * "file absent" from "file present with DEFAULT_PROXY_PORT". Callers that need + * to differentiate must use proxyJsonExists() rather than checking the result + * of readProxyState(). + */ +export async function proxyJsonExists(devflowDir: string): Promise { + try { + await fs.access(join(devflowDir, 'proxy.json')); + return true; + } catch { + return false; + } +} + /** * Check whether the Devflow proxy is currently enabled. * Returns false when the proxy state file is missing, unreadable, or malformed. diff --git a/src/targets/claude-code/post-install.ts b/src/targets/claude-code/post-install.ts index 0c8bda58..83650e0f 100644 --- a/src/targets/claude-code/post-install.ts +++ b/src/targets/claude-code/post-install.ts @@ -791,10 +791,103 @@ export async function stripUserSecurityDenyList( return { removed }; } +/** True for a non-null, non-array object literal. */ +function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** + * Every `command` string carried by a hook matcher, tolerating foreign shapes. + * Returns an empty array for anything that is not `{ hooks: [{ command: string }] }`. + */ +function hookCommandsOf(matcher: unknown): string[] { + if (!isPlainObject(matcher) || !Array.isArray(matcher.hooks)) return []; + return matcher.hooks + .filter(isPlainObject) + .map((h) => h.command) + .filter((c): c is string => typeof c === 'string'); +} + +/** + * Merge Devflow's template hook entries and top-level fields into an existing + * parsed settings object. Idempotent by exact command string — a hook that is + * already present is skipped. Sets `statusLine` and `attribution` only when the + * user has no existing value for them. Mutates `existing` in place. + * + * D-SETTINGS-1: merge strategy — never replace; only add devflow entries that are absent. + * Returns { changed: true } when any field was added. + * + * `existing` comes from a hand-editable file, so every branch is shape-guarded: + * a `hooks` value (or per-event value) that is not the expected object/array shape + * is left untouched rather than overwritten or thrown on (applies PF-023 — validate + * at the sink that mutates). + * + * Exported for testing. + */ +export function mergeDevflowSettingsTemplate( + existing: Record, + template: Record, +): { changed: boolean } { + let changed = false; + + // Merge hook entries — idempotent by exact command string + const tmplHooks = isPlainObject(template.hooks) ? template.hooks : {}; + const existingHooksRaw = existing.hooks; + if (existingHooksRaw === undefined || isPlainObject(existingHooksRaw)) { + const existingHooks: Record = existingHooksRaw ?? {}; + let hooksChanged = false; + for (const [event, matchers] of Object.entries(tmplHooks)) { + if (!Array.isArray(matchers)) continue; + for (const matcher of matchers) { + const cmd = hookCommandsOf(matcher)[0]; + if (!cmd) continue; + const current = existingHooks[event]; + if (current !== undefined && !Array.isArray(current)) break; // foreign shape — leave the event alone + const eventArr: unknown[] = current ?? []; + if (eventArr.some((m) => hookCommandsOf(m).includes(cmd))) continue; + eventArr.push(matcher); + existingHooks[event] = eventArr; + hooksChanged = true; + } + } + // Attach only when something was actually added — never introduce an empty + // `hooks` key into a settings.json that had none. + if (hooksChanged) { + existing.hooks = existingHooks; + changed = true; + } + } + + // Set statusLine only if the user has none + if (existing.statusLine === undefined && template.statusLine !== undefined) { + existing.statusLine = template.statusLine; + changed = true; + } + + // Set attribution only if the user has none + if (existing.attribution === undefined && template.attribution !== undefined) { + existing.attribution = template.attribution; + changed = true; + } + + return { changed }; +} + /** * Install or update settings.json with Devflow configuration. - * Prompts interactively in TTY mode when settings already exist. - * In non-TTY mode, skips override (safe default). + * + * Strategy (D-SETTINGS-1: merge, never overwrite wholesale): + * - Fresh file: write the template directly. + * - Existing file: MERGE devflow hook entries and fields into the parsed object. + * Idempotent by exact command string — existing hooks are never duplicated or removed. + * Preserves every user key (env, permissions, apiKeyHelper, model, etc.) untouched. + * - Parse failure: warn and skip; file left byte-identical (never clobber a broken file). + * + * The merge is additive only, so it runs without a confirmation prompt: init is called + * from inside an active spinner (a prompt would render on top of it), and declining + * could not protect the file anyway — init's own settings pass rewrites the hook set + * unconditionally right afterwards. A prompt whose answer changes nothing is worse + * than no prompt. * * The deny list is handled by init's dedicated security step * (applyUserSecurityDenyList / installManagedSettings) after installSettings completes. @@ -828,39 +921,35 @@ export async function installSettings( return; } - // Settings exist — check if they already have hooks - let hasHooks = false; + // Settings exist — parse and merge (never overwrite wholesale) + let existingParsed: Record; try { - const existing = JSON.parse(await fs.readFile(settingsPath, 'utf-8')); - hasHooks = !!existing.hooks; - } catch { /* parse error = treat as no hooks */ } - - if (hasHooks) { - // Settings already configured with hooks — nothing to do + existingParsed = JSON.parse(await fs.readFile(settingsPath, 'utf-8')) as Record; + } catch { + // Parse failure — warn and skip; file left byte-identical (D-SETTINGS-1) + if (verbose) { + p.log.warn( + 'settings.json could not be parsed — Devflow hooks not added. ' + + 'Fix the JSON manually and re-run devflow init.', + ); + } return; } - // Settings exist without hooks — prompt in TTY, warn in non-TTY - if (process.stdin.isTTY) { - const confirmed = await p.confirm({ - message: 'settings.json exists without hooks (Working Memory needs hooks). Override?', - initialValue: true, - }); + const templateParsed = JSON.parse(settingsContent) as Record; - if (p.isCancel(confirmed)) { - p.cancel('Installation cancelled.'); - process.exit(0); - } + // Merge template into existing (mutates existingParsed in place). + // If nothing needs to be added the write is skipped entirely. + const { changed } = mergeDevflowSettingsTemplate(existingParsed, templateParsed); - if (confirmed) { - await fs.writeFile(settingsPath, settingsContent, 'utf-8'); - p.log.success('Settings overridden'); - } else { - p.log.info('Keeping existing settings'); - } - } else { - p.log.warn('Settings exist without hooks. Working Memory requires hooks.'); - p.log.info('Re-run interactively to configure, or manually add hooks to settings.json'); + if (!changed) { + // Already fully configured — nothing to do + return; + } + + await writeFileAtomicExclusive(settingsPath, JSON.stringify(existingParsed, null, 2) + '\n'); + if (verbose) { + p.log.success('Settings updated with Devflow hooks and HUD'); } } catch (error: unknown) { if (verbose) { diff --git a/tests/external-models.test.ts b/tests/external-models.test.ts index c63bb13f..544d4dc5 100644 --- a/tests/external-models.test.ts +++ b/tests/external-models.test.ts @@ -20,7 +20,7 @@ import { } from '../src/core/external-models.js'; // Literal GPT model IDs — independent of the deleted hardcoded registry. -// These reflect the subswitch@0.2.0 catalog used throughout Phase D tests. +// These reflect the subswitch@0.4.0 catalog used throughout Phase D tests. // applies ADR-003: end-state only — no compatibility imports from deleted exports. const KNOWN_GPT_IDS = ['gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5']; import { diff --git a/tests/init-proxy.test.ts b/tests/init-proxy.test.ts index c7cf88e1..b97c1ca2 100644 --- a/tests/init-proxy.test.ts +++ b/tests/init-proxy.test.ts @@ -42,7 +42,7 @@ function makeAgentFrontmatter(model: string): string { /** * Known GPT model IDs — literal list so this test file does not depend on the * hardcoded registry in external-models.ts (which is deleted in Commit 9). - * These match the subswitch@0.2.0 catalog used throughout the Phase D tests. + * These match the subswitch@0.4.0 catalog used throughout the Phase D tests. * applies ADR-003: end-state only — no externalModelIds() import. */ const GPT_IDS = ['gpt-5.6-sol', 'gpt-5.6-terra', 'gpt-5.6-luna', 'gpt-5.5']; diff --git a/tests/model-discovery.test.ts b/tests/model-discovery.test.ts index d0a63493..0175a95b 100644 --- a/tests/model-discovery.test.ts +++ b/tests/model-discovery.test.ts @@ -776,7 +776,7 @@ describe('T1: Real-binary — discoverExternalModels with live runtime', () => { const result = await discoverExternalModels(cacheDir, logPath); - // subswitch models --json (0.2.0) is a static registry dump: no auth, no relay, + // subswitch models --json (0.4.0) is a static registry dump: no auth, no relay, // no config file required (verified: exit 0, configFileFound:false). The per-test // cacheDir is freshly created (beforeEach), so no cache hit is possible — source // must be 'live'. Guarding these behind `if (result.known)` would let the test diff --git a/tests/packaging.test.ts b/tests/packaging.test.ts index dc3d4567..fcc83b64 100644 --- a/tests/packaging.test.ts +++ b/tests/packaging.test.ts @@ -25,7 +25,7 @@ const ROOT = path.resolve(import.meta.dirname, '..'); * Expected exact-pinned version of the routing runtime. * Hoisted so the next bump is a one-line change. */ -const SUBSWITCH_VERSION = '0.2.0'; +const SUBSWITCH_VERSION = '0.4.0'; // --------------------------------------------------------------------------- // Guard 3: dependency pin integrity diff --git a/tests/post-install-merge.test.ts b/tests/post-install-merge.test.ts new file mode 100644 index 00000000..223ee4bd --- /dev/null +++ b/tests/post-install-merge.test.ts @@ -0,0 +1,362 @@ +/** + * Tests for FIX 1 (issue #313): mergeDevflowSettingsTemplate in post-install.ts. + * + * D-SETTINGS-1: installSettings must MERGE devflow hook entries into an existing + * settings.json rather than replacing it. A user's env block, permissions, apiKeyHelper, + * or model assignment must never be destroyed. Parse failure must leave the file + * byte-identical. Idempotent by exact command string. + * + * Coverage: + * - Hooks already present → changed:false (no duplicate insertion) + * - Hooks absent → changed:true (added) + * - Preserves user-owned keys untouched (env, permissions, model, etc.) + * - statusLine set only when absent + * - attribution set only when absent + * - statusLine preserved when already set by the user + * - Idempotent — double merge is the same as single merge + * - Template hook with no command string is skipped silently + * - Empty template → changed:false + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mergeDevflowSettingsTemplate, installSettings } from '../src/targets/claude-code/post-install.js'; +import type { HookMatcher } from '../src/targets/claude-code/hooks.js'; +import * as os from 'node:os'; +import * as fsp from 'node:fs/promises'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// Resolved repo root — installSettings needs it to locate the settings template. +const REPO_ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), '..'); + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function makeHookMatcher(command: string, timeout = 10): HookMatcher { + return { hooks: [{ type: 'command', command, timeout }] }; +} + +function makeTemplate(commands: string[], statusLine = 'devflow: {branch}', attribution = 'Devflow'): Record { + const matchers = commands.map((cmd) => makeHookMatcher(cmd)); + return { + hooks: { + 'SessionStart': matchers, + }, + statusLine, + attribution, + }; +} + +// ─── mergeDevflowSettingsTemplate ──────────────────────────────────────────── + +describe('mergeDevflowSettingsTemplate — FIX 1 (issue #313)', () => { + + it('returns changed:true and adds hooks when existing has none', () => { + const existing: Record = {}; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook memory-worker']); + const { changed } = mergeDevflowSettingsTemplate(existing, template); + expect(changed).toBe(true); + const hooks = (existing.hooks as Record)['SessionStart'] ?? []; + expect(hooks.length).toBe(1); + expect(hooks[0]?.hooks[0]?.command).toBe('/devflow/scripts/hooks/run-hook memory-worker'); + }); + + it('returns changed:false when all template hooks are already present', () => { + const cmd = '/devflow/scripts/hooks/run-hook memory-worker'; + const existing: Record = { + hooks: { + 'SessionStart': [makeHookMatcher(cmd)], + }, + statusLine: 'devflow: {branch}', + attribution: 'Devflow', + }; + const template = makeTemplate([cmd]); + const { changed } = mergeDevflowSettingsTemplate(existing, template); + expect(changed).toBe(false); + }); + + it('does not duplicate a hook that already exists (exact command match)', () => { + const cmd = '/devflow/scripts/hooks/run-hook capture-turn'; + const existing: Record = { + hooks: { + 'SessionStart': [makeHookMatcher(cmd)], + }, + }; + const template = makeTemplate([cmd]); + mergeDevflowSettingsTemplate(existing, template); + mergeDevflowSettingsTemplate(existing, template); // second pass + const hooks = (existing.hooks as Record)['SessionStart'] ?? []; + const count = hooks.filter((m) => m.hooks.some((h) => h.command === cmd)).length; + expect(count).toBe(1); + }); + + it('preserves user keys: env block is never touched', () => { + const existing: Record = { + env: { ANTHROPIC_API_KEY: 'sk-test-keep-me', CUSTOM_VAR: 'value' }, + }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook preamble']); + mergeDevflowSettingsTemplate(existing, template); + // env block must survive unchanged + const env = existing.env as Record; + expect(env.ANTHROPIC_API_KEY).toBe('sk-test-keep-me'); + expect(env.CUSTOM_VAR).toBe('value'); + }); + + it('preserves user keys: permissions block is never touched', () => { + const existing: Record = { + permissions: { deny: ['Bash(*)', 'Edit(*)'] }, + }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook preamble']); + mergeDevflowSettingsTemplate(existing, template); + const perms = existing.permissions as Record; + expect(perms.deny).toEqual(['Bash(*)', 'Edit(*)']); + }); + + it('preserves user keys: model field is never touched', () => { + const existing: Record = { + model: 'claude-opus-4-5', + }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook preamble']); + mergeDevflowSettingsTemplate(existing, template); + expect(existing.model).toBe('claude-opus-4-5'); + }); + + it('preserves user keys: apiKeyHelper is never touched', () => { + const existing: Record = { + apiKeyHelper: '/usr/local/bin/get-api-key', + }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook preamble']); + mergeDevflowSettingsTemplate(existing, template); + expect(existing.apiKeyHelper).toBe('/usr/local/bin/get-api-key'); + }); + + it('sets statusLine from template when absent on existing', () => { + const existing: Record = {}; + const template = makeTemplate([], 'devflow v2'); + mergeDevflowSettingsTemplate(existing, template); + expect(existing.statusLine).toBe('devflow v2'); + }); + + it('does NOT overwrite statusLine when user already has one', () => { + const existing: Record = { statusLine: 'my-custom-status-line' }; + const template = makeTemplate([], 'devflow v2'); + mergeDevflowSettingsTemplate(existing, template); + expect(existing.statusLine).toBe('my-custom-status-line'); + }); + + it('sets attribution from template when absent on existing', () => { + const existing: Record = {}; + const template = { statusLine: 's', attribution: 'Devflow' }; + mergeDevflowSettingsTemplate(existing, template); + expect(existing.attribution).toBe('Devflow'); + }); + + it('does NOT overwrite attribution when user already has one', () => { + const existing: Record = { attribution: 'my-org' }; + const template = { statusLine: 's', attribution: 'Devflow' }; + mergeDevflowSettingsTemplate(existing, template); + expect(existing.attribution).toBe('my-org'); + }); + + it('adds only the missing hooks when some are present and some are not', () => { + const cmd1 = '/devflow/scripts/hooks/run-hook memory-worker'; + const cmd2 = '/devflow/scripts/hooks/run-hook capture-turn'; + const existing: Record = { + hooks: { 'SessionStart': [makeHookMatcher(cmd1)] }, // cmd1 present, cmd2 absent + }; + const template: Record = { + hooks: { 'SessionStart': [makeHookMatcher(cmd1), makeHookMatcher(cmd2)] }, + }; + const { changed } = mergeDevflowSettingsTemplate(existing, template); + expect(changed).toBe(true); + const hooks = (existing.hooks as Record)['SessionStart'] ?? []; + const cmds = hooks.flatMap((m) => m.hooks.map((h) => h.command)); + expect(cmds).toContain(cmd1); + expect(cmds).toContain(cmd2); + // cmd1 appears exactly once — no duplicate + expect(cmds.filter((c) => c === cmd1).length).toBe(1); + }); + + it('handles multiple hook events independently', () => { + const cmd = '/devflow/scripts/hooks/run-hook ensure-proxy'; + const existing: Record = {}; + const template: Record = { + hooks: { + 'SessionStart': [makeHookMatcher(cmd)], + 'UserPromptSubmit': [makeHookMatcher(cmd)], + }, + }; + mergeDevflowSettingsTemplate(existing, template); + const ss = (existing.hooks as Record)['SessionStart'] ?? []; + const up = (existing.hooks as Record)['UserPromptSubmit'] ?? []; + expect(ss.some((m) => m.hooks.some((h) => h.command === cmd))).toBe(true); + expect(up.some((m) => m.hooks.some((h) => h.command === cmd))).toBe(true); + }); + + it('skips a template hook entry that has no command string', () => { + const existing: Record = {}; + const template: Record = { + hooks: { + 'SessionStart': [{ hooks: [] } as unknown as HookMatcher], // no command + }, + }; + const { changed } = mergeDevflowSettingsTemplate(existing, template); + // No command → nothing to add → not changed from hooks (hooks key initialized but no entry pushed) + expect(changed).toBe(false); + }); + + it('empty template results in changed:false', () => { + const existing: Record = { model: 'claude-opus-4-5' }; + const template: Record = {}; + const { changed } = mergeDevflowSettingsTemplate(existing, template); + expect(changed).toBe(false); + expect(existing.model).toBe('claude-opus-4-5'); + }); + + it('is idempotent — double merge produces same result as single merge', () => { + const cmd = '/devflow/scripts/hooks/run-hook memory-worker'; + const existing: Record = {}; + const template = makeTemplate([cmd], 'devflow v2', 'Devflow'); + + mergeDevflowSettingsTemplate(existing, template); + const snapshotAfterFirst = JSON.stringify(existing); + + // D-SETTINGS-1: second merge must be a no-op + const { changed: changedOnSecond } = mergeDevflowSettingsTemplate(existing, template); + expect(changedOnSecond).toBe(false); + expect(JSON.stringify(existing)).toBe(snapshotAfterFirst); + }); + + // ── Shape guards (PF-023): `existing` is a hand-editable file ────────────── + // + // The merge mutates a user-authored object, so every branch validates shape at + // the sink. A settings.json that is valid JSON but structurally odd must neither + // throw (init would swallow it and skip hook installation entirely) nor lose the + // user's own entries. + + it('leaves a non-object hooks value untouched instead of throwing', () => { + const existing: Record = { hooks: 'not-an-object' }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook memory-worker']); + expect(() => mergeDevflowSettingsTemplate(existing, template)).not.toThrow(); + expect(existing.hooks).toBe('not-an-object'); + }); + + it('leaves an array hooks value untouched instead of writing keys onto it', () => { + const existing: Record = { hooks: [] }; + const template: Record = { + hooks: { SessionStart: [makeHookMatcher('/devflow/scripts/hooks/run-hook memory-worker')] }, + }; + const { changed } = mergeDevflowSettingsTemplate(existing, template); + // JSON.stringify would silently drop keys attached to an array — never touch it. + expect(Array.isArray(existing.hooks)).toBe(true); + expect((existing.hooks as unknown[]).length).toBe(0); + expect(changed).toBe(false); + }); + + it('leaves a non-array per-event value untouched instead of throwing', () => { + const existing: Record = { hooks: { SessionStart: 'oops' } }; + const template = makeTemplate(['/devflow/scripts/hooks/run-hook memory-worker']); + expect(() => mergeDevflowSettingsTemplate(existing, template)).not.toThrow(); + expect((existing.hooks as Record).SessionStart).toBe('oops'); + }); + + it('tolerates malformed existing matcher entries when deduping', () => { + const cmd = '/devflow/scripts/hooks/run-hook memory-worker'; + const existing: Record = { + hooks: { SessionStart: ['a-string', { noHooksKey: true }, null] }, + }; + const template = makeTemplate([cmd]); + expect(() => mergeDevflowSettingsTemplate(existing, template)).not.toThrow(); + const arr = (existing.hooks as Record).SessionStart; + // Foreign entries survive; the devflow hook is appended after them. + expect(arr.length).toBe(4); + expect(arr[0]).toBe('a-string'); + }); + + it('does not introduce an empty hooks key when the template adds no hooks', () => { + const existing: Record = { model: 'claude-opus-4-5' }; + const template: Record = { statusLine: 's' }; + mergeDevflowSettingsTemplate(existing, template); + expect('hooks' in existing).toBe(false); + }); + + it('preserves existing hook order — devflow hooks appended, not prepended', () => { + const existingCmd = '/user/custom-hook'; + const devflowCmd = '/devflow/scripts/hooks/run-hook memory-worker'; + const existing: Record = { + hooks: { 'SessionStart': [makeHookMatcher(existingCmd)] }, + }; + const template = makeTemplate([devflowCmd]); + mergeDevflowSettingsTemplate(existing, template); + const hooks = (existing.hooks as Record)['SessionStart'] ?? []; + expect(hooks[0]?.hooks[0]?.command).toBe(existingCmd); // user hook comes first + expect(hooks[1]?.hooks[0]?.command).toBe(devflowCmd); // devflow hook appended + }); +}); + +// ─── installSettings — parse-failure and merge wiring ──────────────────────── +// +// These tests exercise installSettings(claudeDir, rootDir, devflowDir, verbose) +// against the real filesystem via real tmpdirs. They complement the pure +// mergeDevflowSettingsTemplate unit tests above by pinning the I/O wiring and +// the parse-failure bail path (post-install.ts:926-935). + +describe('installSettings — parse-failure and wiring (issue #313)', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'devflow-settings-test-')); + }); + + afterEach(async () => { + await fsp.rm(tmpDir, { recursive: true, force: true }); + }); + + it('(a) leaves settings.json byte-identical on invalid JSON and leaves no .tmp.* residue', async () => { + // Arrange — write a syntactically invalid JSON (trailing comma before closing brace) + const invalidJson = '{ "hooks": { "SessionStart": [] } , }'; + const settingsPath = path.join(tmpDir, 'settings.json'); + await fsp.writeFile(settingsPath, invalidJson, 'utf-8'); + const bytesBefore = await fsp.readFile(settingsPath); + + // Act — parse failure must bail without touching the file + await installSettings(tmpDir, REPO_ROOT, '/fake/devflow', false); + + // Assert — file bytes are identical (no byte was changed) + const bytesAfter = await fsp.readFile(settingsPath); + expect(bytesAfter.equals(bytesBefore)).toBe(true); + + // Assert — writeFileAtomicExclusive writes to a `.tmp.` sibling then renames; + // on the parse-failure path it must never be created. + const entries = await fsp.readdir(tmpDir); + const tmpFiles = entries.filter((e) => e.includes('.tmp.')); + expect(tmpFiles).toHaveLength(0); + }); + + it('(b) adds hooks while preserving env and permissions blocks byte-for-byte (wiring pin)', async () => { + // Arrange — valid settings with env + permissions but no hooks + const existingSettings: Record = { + env: { ANTHROPIC_BASE_URL: 'https://my-gateway.example.com', MY_VAR: 'keep-me' }, + permissions: { allow: ['Read(*)'], deny: ['Bash(rm -rf *)'] }, + }; + const settingsPath = path.join(tmpDir, 'settings.json'); + await fsp.writeFile(settingsPath, JSON.stringify(existingSettings, null, 2) + '\n', 'utf-8'); + + // Act — installSettings should merge the template hooks in + await installSettings(tmpDir, REPO_ROOT, '/fake/devflow', false); + + // Assert — hooks were added (template carries hooks for SessionStart, Stop, etc.) + const result = JSON.parse(await fsp.readFile(settingsPath, 'utf-8')) as Record; + expect(result.hooks).toBeDefined(); + const hooks = result.hooks as Record; + const hookEventCount = Object.values(hooks).filter( + (arr) => Array.isArray(arr) && arr.length > 0, + ).length; + expect(hookEventCount).toBeGreaterThan(0); + + // Assert — env block survived byte-for-byte + expect(result.env).toEqual(existingSettings.env); + + // Assert — permissions block survived byte-for-byte + expect(result.permissions).toEqual(existingSettings.permissions); + }); +}); diff --git a/tests/proxy-state.test.ts b/tests/proxy-state.test.ts index fa4cb4b7..27ecefb4 100644 --- a/tests/proxy-state.test.ts +++ b/tests/proxy-state.test.ts @@ -10,7 +10,8 @@ * - readProxyState: malformed JSON → tolerant default, no throw (TEST-2) * - writeProxyState → readProxyState round-trip (TEST-2) * - Tolerant field parsing: wrong-typed fields self-heal to defaults (TEST-2) - * - buildRoutingConfigJson: exact {port} shape only — no codex key (AC-C4) + * - buildRoutingConfigJson: port-only shape — no injected anthropic block (AC-C4) + * - buildRoutingConfigJson: user-set anthropic.connectTimeoutMs is preserved * - Pre-existing proxy.json with models loads cleanly; key absent after write (AC-C5) * - RUNTIME_VERSION_RE: path-traversal and length-limit rejection (AC-S4) */ @@ -24,9 +25,9 @@ import { writeProxyState, buildRoutingConfigJson, buildProxyState, + proxyJsonExists, DEFAULT_PROXY_PORT, RUNTIME_VERSION_RE, - DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS, } from '../src/core/proxy-state.js'; // --------------------------------------------------------------------------- @@ -215,7 +216,7 @@ describe('readProxyState — wrong-typed fields self-heal to defaults', () => { }); // --------------------------------------------------------------------------- -// buildRoutingConfigJson — port + anthropic.connectTimeoutMs default (AC-C4 / D-EFR-4) +// buildRoutingConfigJson — port-only shape; user-set anthropic keys preserved (AC-C4) // --------------------------------------------------------------------------- describe('buildRoutingConfigJson — base behavior', () => { @@ -242,12 +243,9 @@ describe('buildRoutingConfigJson — base behavior', () => { expect(() => JSON.parse(json)).not.toThrow(); }); - it('injects default anthropic.connectTimeoutMs when no existing config supplied (D-EFR-4)', () => { - const json = buildRoutingConfigJson(4141); - const obj = JSON.parse(json) as Record; - const anthropic = obj.anthropic as Record; - expect(anthropic).toBeDefined(); - expect(anthropic.connectTimeoutMs).toBe(DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS); + it('emits no anthropic block when no existing config is supplied (relay default governs)', () => { + const obj = JSON.parse(buildRoutingConfigJson(4141)) as Record; + expect(Object.prototype.hasOwnProperty.call(obj, 'anthropic')).toBe(false); }); it('only emits allowed top-level keys (strictObject constraint D-EFR-4)', () => { @@ -260,19 +258,22 @@ describe('buildRoutingConfigJson — base behavior', () => { }); describe('buildRoutingConfigJson — existing config preservation', () => { - it('user-specified anthropic.connectTimeoutMs wins over default', () => { + it('user-specified anthropic.connectTimeoutMs is preserved (relay default not injected)', () => { const existing = JSON.stringify({ port: 4141, anthropic: { connectTimeoutMs: 30_000 } }); const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; const anthropic = obj.anthropic as Record; expect(anthropic.connectTimeoutMs).toBe(30_000); }); - it('preserves other anthropic fields alongside injected default', () => { - const existing = JSON.stringify({ port: 4141, anthropic: { streamIdleTimeoutMs: 60_000 } }); + it('preserves other anthropic fields from existing config', () => { + // maxUpstreamSockets is a live key in the pinned runtime's AnthropicSchema — + // a preservation fixture has to use a shape the relay actually accepts, or it + // pins behaviour that would break the relay at startup (avoids PF-043). + const existing = JSON.stringify({ port: 4141, anthropic: { maxUpstreamSockets: 64 } }); const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; const anthropic = obj.anthropic as Record; - expect(anthropic.streamIdleTimeoutMs).toBe(60_000); - expect(anthropic.connectTimeoutMs).toBe(DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS); + expect(anthropic.maxUpstreamSockets).toBe(64); + expect(Object.prototype.hasOwnProperty.call(anthropic, 'connectTimeoutMs')).toBe(false); }); it('preserves logLevel from existing config', () => { @@ -296,6 +297,108 @@ describe('buildRoutingConfigJson — existing config preservation', () => { expect(limits.maxConcurrent).toBe(10); }); + // Keys that were valid under a previously pinned runtime and are registered legacy + // keys in the current one — a hard startup error, not a warning. Carrying a user's own + // proxy-routing.json forward across the upgrade must drop them, or the relay that the + // ensure-proxy hook spawns dies on boot every session with no route back. + it('strips anthropic.streamIdleTimeoutMs (removed in the pinned runtime)', () => { + const existing = JSON.stringify({ + port: 4141, + anthropic: { streamIdleTimeoutMs: 60_000, maxUpstreamSockets: 64 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const anthropic = obj.anthropic as Record; + expect(Object.prototype.hasOwnProperty.call(anthropic, 'streamIdleTimeoutMs')).toBe(false); + // Neighbouring valid keys survive the strip; no default is injected. + expect(anthropic.maxUpstreamSockets).toBe(64); + expect(Object.prototype.hasOwnProperty.call(anthropic, 'connectTimeoutMs')).toBe(false); + }); + + it('strips limits.maxConcurrentRequests (removed in the pinned runtime)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxConcurrentRequests: 8, maxConcurrent: 10 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'maxConcurrentRequests')).toBe(false); + expect(limits.maxConcurrent).toBe(10); + }); + + it('strips limits.maxBodyBytes (renamed maxBufferedBodyBytes in the pinned runtime)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxBodyBytes: 10_485_760, maxConcurrent: 10 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'maxBodyBytes')).toBe(false); + expect(limits.maxConcurrent).toBe(10); + }); + + it('preserves the current limits.maxBufferedBodyBytes spelling', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxBufferedBodyBytes: 10_485_760 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(limits.maxBufferedBodyBytes).toBe(10_485_760); + }); + + it('strips limits.maxUpstreamSockets (moved to anthropic.maxUpstreamSockets in 0.4.0)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxUpstreamSockets: 128, maxBufferedBodyBytes: 10_485_760 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'maxUpstreamSockets')).toBe(false); + expect(limits.maxBufferedBodyBytes).toBe(10_485_760); + }); + + it('strips limits.streamIdleTimeoutMs (moved to providers.codex.streamIdleTimeoutMs in 0.4.0)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { streamIdleTimeoutMs: 30_000, maxBufferedBodyBytes: 10_485_760 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'streamIdleTimeoutMs')).toBe(false); + expect(limits.maxBufferedBodyBytes).toBe(10_485_760); + }); + + it('strips limits.requestTimeoutMs (moved to providers.codex.requestTimeoutMs in 0.4.0)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { requestTimeoutMs: 60_000, maxBufferedBodyBytes: 10_485_760 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'requestTimeoutMs')).toBe(false); + expect(limits.maxBufferedBodyBytes).toBe(10_485_760); + }); + + it('strips limits.maxSseEventBytes (moved to providers.codex.maxSseEventBytes in 0.4.0)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxSseEventBytes: 65_536, maxBufferedBodyBytes: 10_485_760 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + const limits = obj.limits as Record; + expect(Object.prototype.hasOwnProperty.call(limits, 'maxSseEventBytes')).toBe(false); + expect(limits.maxBufferedBodyBytes).toBe(10_485_760); + }); + + it('omits limits key from output when all sub-keys are stripped (mirrors anthropic omit-when-empty)', () => { + const existing = JSON.stringify({ + port: 4141, + limits: { maxUpstreamSockets: 128, streamIdleTimeoutMs: 30_000 }, + }); + const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; + expect(Object.prototype.hasOwnProperty.call(obj, 'limits')).toBe(false); + }); + it('preserves providers block from existing config', () => { const existing = JSON.stringify({ port: 4141, providers: { openai: { baseUrl: 'https://api.openai.com' } } }); const obj = JSON.parse(buildRoutingConfigJson(4141, existing)) as Record; @@ -315,19 +418,17 @@ describe('buildRoutingConfigJson — existing config preservation', () => { expect(obj.port).toBe(4141); }); - it('malformed existing content falls back cleanly — no throw, port + anthropic default emitted', () => { + it('malformed existing content falls back cleanly — no throw, port-only config emitted', () => { expect(() => buildRoutingConfigJson(4141, '{ bad json !!!')).not.toThrow(); const obj = JSON.parse(buildRoutingConfigJson(4141, '{ bad json !!!')) as Record; expect(obj.port).toBe(4141); - const anthropic = obj.anthropic as Record; - expect(anthropic.connectTimeoutMs).toBe(DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS); + expect(Object.prototype.hasOwnProperty.call(obj, 'anthropic')).toBe(false); }); - it('undefined existingContent falls back to clean defaults', () => { + it('undefined existingContent falls back to port-only config', () => { const obj = JSON.parse(buildRoutingConfigJson(4141, undefined)) as Record; expect(obj.port).toBe(4141); - const anthropic = obj.anthropic as Record; - expect(anthropic.connectTimeoutMs).toBe(DEFAULT_ANTHROPIC_CONNECT_TIMEOUT_MS); + expect(Object.prototype.hasOwnProperty.call(obj, 'anthropic')).toBe(false); }); }); @@ -406,3 +507,58 @@ describe('RUNTIME_VERSION_RE — version string validation (AC-S4)', () => { expect(RUNTIME_VERSION_RE.test(v)).toBe(expected); }); }); + +// --------------------------------------------------------------------------- +// FIX 2 (issue #313): proxyJsonExists — D-STRIP-1 gate discriminator +// +// readProxyState() returns Ok(defaultState) on ENOENT — callers cannot use its +// success to prove proxy.json exists. proxyJsonExists() is the correct gate for +// stripping managed env vars: strip only when the file exists (proving devflow +// previously wrote it). +// --------------------------------------------------------------------------- + +describe('proxyJsonExists — FIX 2 (issue #313)', () => { + it('returns false when proxy.json does not exist', async () => { + const result = await proxyJsonExists(tmpDir); + expect(result).toBe(false); + }); + + it('returns true when proxy.json exists (even with only a port)', async () => { + await fs.writeFile(path.join(tmpDir, 'proxy.json'), JSON.stringify({ port: DEFAULT_PROXY_PORT })); + const result = await proxyJsonExists(tmpDir); + expect(result).toBe(true); + }); + + it('returns true for a full proxy.json written by writeProxyState', async () => { + const state = buildProxyState(DEFAULT_PROXY_PORT, '/path/relay.js', '/path/config.json', '0.2.0'); + await writeProxyState(tmpDir, state); + const result = await proxyJsonExists(tmpDir); + expect(result).toBe(true); + }); + + it('returns true even when the file contains malformed JSON (file-exists ≠ valid JSON)', async () => { + await fs.writeFile(path.join(tmpDir, 'proxy.json'), 'not-valid-json{{'); + const result = await proxyJsonExists(tmpDir); + expect(result).toBe(true); + }); + + it('returns false when devflowDir itself does not exist', async () => { + const nonexistent = path.join(tmpDir, 'does-not-exist'); + const result = await proxyJsonExists(nonexistent); + expect(result).toBe(false); + }); + + it('discriminates absent-file from Ok(defaultState) returned by readProxyState', async () => { + // readProxyState() returns Ok({enabled:false,port:DEFAULT_PROXY_PORT,...}) on ENOENT, + // which is indistinguishable from a file present with those exact values. + // proxyJsonExists() must correctly report false for the absent-file case. + const readResult = await readProxyState(tmpDir); + expect(readResult.ok).toBe(true); // readProxyState returns Ok even on ENOENT + if (readResult.ok) { + expect(readResult.value.port).toBe(DEFAULT_PROXY_PORT); // same as a real file with default port + } + // proxyJsonExists is the correct discriminator — file is absent + const exists = await proxyJsonExists(tmpDir); + expect(exists).toBe(false); + }); +}); diff --git a/tests/proxy.test.ts b/tests/proxy.test.ts index 8fc4534f..e1516a26 100644 --- a/tests/proxy.test.ts +++ b/tests/proxy.test.ts @@ -22,6 +22,7 @@ import { removeProxyHooks, hasProxyHooks, applyDisableToSettings, + applyProxyTeardownToSettings, runProxyPreflight, runPostSpawnVerification, isOurRelayBody, @@ -1755,3 +1756,196 @@ describe('Phase 4 / T7-extended: fully-enabled state includes UNKNOWN_MODEL_WIND expect(env[WINDOW_ENV]).toBe('1'); }); }); + +// ─── FIX 3 (issue #313): runProxyPreflight — D-EFR-5 ordering ─────────────── +// +// Foreign-env check (check ④) must evaluate BEFORE the adopted early-return +// (check ③). Previously, a healthy relay on the port caused an early-return that +// silently skipped the foreign-gateway refusal entirely. +// +// Regression tests: +// REG-EFR-1: foreign URL + healthy relay → Err (refusal), not adopted +// REG-EFR-2: no foreign URL + healthy relay → Ok(adopted:true) (normal adopt) +// REG-EFR-3: foreign URL + port free → Err (check ④ on the free-port path, unchanged) + +describe('runProxyPreflight — FIX 3 D-EFR-5 foreign-env check ordering (issue #313)', () => { + const port = DEFAULT_PORT; + const codexAuthPath = '/home/test/.codex/auth.json'; + const configPath = '/home/test/.devflow/proxy-routing.json'; + const logPath = '/home/test/.devflow/logs/proxy.log'; + + const ourHealthBody = '{"name":"subswitch","version":"0.2.0","providers":[{"id":"anthropic","configured":true,"modelCount":0}]}'; + + // REG-EFR-1: healthy relay + foreign URL → must be refused (not adopted silently) + it('REG-EFR-1: returns Err when relay is healthy but ANTHROPIC_BASE_URL is a foreign gateway', async () => { + const deps = makeDeps({ + tcpConnectable: vi.fn().mockResolvedValue(true), // port up + httpGet: vi.fn().mockResolvedValue({ ok: true, value: ourHealthBody }), // our relay + readSettingsJson: vi.fn().mockResolvedValue(JSON.stringify({ + env: { ANTHROPIC_BASE_URL: 'https://litellm.mycompany.internal' }, // foreign URL + })), + }); + const result = await runProxyPreflight(port, codexAuthPath, configPath, logPath, deps); + // Must be refused — not adopted silently + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain('ANTHROPIC_BASE_URL'); + } + }); + + // REG-EFR-2: healthy relay + no foreign URL → normal adopt + it('REG-EFR-2: returns Ok(adopted:true) when relay is healthy and no foreign URL', async () => { + const deps = makeDeps({ + tcpConnectable: vi.fn().mockResolvedValue(true), + httpGet: vi.fn().mockResolvedValue({ ok: true, value: ourHealthBody }), + readSettingsJson: vi.fn().mockResolvedValue('{}'), // no foreign URL + }); + const result = await runProxyPreflight(port, codexAuthPath, configPath, logPath, deps); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.adopted).toBe(true); + } + }); + + // REG-EFR-3: port free + foreign URL → check ④ on the free-port path (already tested above, + // reconfirmed here to show both paths refuse a foreign URL) + it('REG-EFR-3: returns Err when port is free but ANTHROPIC_BASE_URL is a foreign gateway', async () => { + const deps = makeDeps({ + tcpConnectable: vi.fn().mockResolvedValue(false), // port free + readSettingsJson: vi.fn().mockResolvedValue(JSON.stringify({ + env: { ANTHROPIC_BASE_URL: 'https://litellm.mycompany.internal' }, + })), + }); + const result = await runProxyPreflight(port, codexAuthPath, configPath, logPath, deps); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain('ANTHROPIC_BASE_URL'); + } + }); + + // REG-EFR-1b: healthy relay + our own relay URL on the port → adopted (not foreign) + it('REG-EFR-1b: adopted when relay is healthy and ANTHROPIC_BASE_URL is already our relay URL', async () => { + const deps = makeDeps({ + tcpConnectable: vi.fn().mockResolvedValue(true), + httpGet: vi.fn().mockResolvedValue({ ok: true, value: ourHealthBody }), + readSettingsJson: vi.fn().mockResolvedValue(JSON.stringify({ + env: { ANTHROPIC_BASE_URL: `http://127.0.0.1:${port}` }, // our URL → not foreign + })), + }); + const result = await runProxyPreflight(port, codexAuthPath, configPath, logPath, deps); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.adopted).toBe(true); + } + }); + + // Verify that readSettingsJson is called in the adopt path (D-EFR-5 gate is wired) + it('calls readSettingsJson in the adopted path to evaluate foreign-env state', async () => { + const readSettingsJson = vi.fn().mockResolvedValue('{}'); + const deps = makeDeps({ + tcpConnectable: vi.fn().mockResolvedValue(true), + httpGet: vi.fn().mockResolvedValue({ ok: true, value: ourHealthBody }), + readSettingsJson, + }); + await runProxyPreflight(port, codexAuthPath, configPath, logPath, deps); + expect(readSettingsJson).toHaveBeenCalledTimes(1); + }); +}); + +// ─── FIX 2 (issue #313): applyProxyTeardownToSettings — D-STRIP-1 gate ──────── +// +// Every proxy-off path (devflow proxy --disable, devflow uninstall) routes its +// settings teardown through this one function so the two removals cannot drift: +// - hooks → always removed (they are Devflow's own artifacts) +// - ANTHROPIC_BASE_URL + window var → removed only when the caller supplies a +// managedPort, which it may do only when proxy.json exists (the sole evidence +// that Devflow ever wrote them). +// +// Falsification: gating the whole call on the evidence (the shape this replaced) +// leaves Devflow's hooks in settings.json after uninstall — REG-TEARDOWN-1 fails. + +describe('applyProxyTeardownToSettings — FIX 2 D-STRIP-1 (issue #313)', () => { + const DEVFLOW_DIR = '/home/test/.devflow'; + + /** Settings in the fully-enabled shape: proxy hooks + both env vars. */ + function enabledSettings(port: number): Settings { + const s = { + env: { + ANTHROPIC_BASE_URL: `http://127.0.0.1:${port}`, + [WINDOW_ENV]: '1', + MY_VAR: 'keep', + }, + } as unknown as Settings; + addProxyHooks(s, DEVFLOW_DIR); + return s; + } + + // REG-TEARDOWN-1: hooks go regardless of evidence — an uninstall that leaves them + // behind points every later session at a hook script that no longer exists. + it('REG-TEARDOWN-1: removes proxy hooks even with no managed port (unmanaged)', () => { + const settings = enabledSettings(DEFAULT_PORT); + expect(hasProxyHooks(settings)).toBe(true); + + const changed = applyProxyTeardownToSettings(settings, undefined); + + expect(changed).toBe(true); + expect(hasProxyHooks(settings)).toBe(false); + }); + + it('leaves both env vars untouched when no managed port is supplied', () => { + const settings = enabledSettings(DEFAULT_PORT); + + applyProxyTeardownToSettings(settings, undefined); + + const env = (settings as unknown as { env: Record }).env; + // Not ours to remove: without proxy.json there is no evidence Devflow wrote these. + expect(env.ANTHROPIC_BASE_URL).toBe(`http://127.0.0.1:${DEFAULT_PORT}`); + expect(env[WINDOW_ENV]).toBe('1'); + expect(env.MY_VAR).toBe('keep'); + }); + + it('removes hooks AND both env vars when a managed port is supplied', () => { + const settings = enabledSettings(DEFAULT_PORT); + + const changed = applyProxyTeardownToSettings(settings, DEFAULT_PORT); + + expect(changed).toBe(true); + expect(hasProxyHooks(settings)).toBe(false); + const env = (settings as unknown as { env?: Record }).env; + expect(env?.ANTHROPIC_BASE_URL).toBeUndefined(); + expect(env?.[WINDOW_ENV]).toBeUndefined(); + expect(env?.MY_VAR).toBe('keep'); + }); + + it('keeps the both-operations invariant: env is stripped even when hooks are present', () => { + // The regression applyDisableToSettings guards against — hooks present must not + // short-circuit the env strip. Re-pinned through the teardown entry point. + const settings = enabledSettings(DEFAULT_PORT); + + applyProxyTeardownToSettings(settings, DEFAULT_PORT); + + const env = (settings as unknown as { env?: Record }).env; + expect(env?.ANTHROPIC_BASE_URL).toBeUndefined(); + expect(env?.[WINDOW_ENV]).toBeUndefined(); + }); + + it('REG-1 preserved: a foreign gateway on another port survives a managed teardown', () => { + const settings = { + env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:9999' }, + } as unknown as Settings; + addProxyHooks(settings, DEVFLOW_DIR); + + applyProxyTeardownToSettings(settings, DEFAULT_PORT); + + const env = (settings as unknown as { env: Record }).env; + expect(env.ANTHROPIC_BASE_URL).toBe('http://127.0.0.1:9999'); + expect(hasProxyHooks(settings)).toBe(false); + }); + + it('returns false when there is nothing to remove (unmanaged, no hooks)', () => { + const settings = { env: { ANTHROPIC_BASE_URL: 'https://gw.example.com' } } as unknown as Settings; + expect(applyProxyTeardownToSettings(settings, undefined)).toBe(false); + const env = (settings as unknown as { env: Record }).env; + expect(env.ANTHROPIC_BASE_URL).toBe('https://gw.example.com'); + }); +}); diff --git a/tests/shell-hooks.test.ts b/tests/shell-hooks.test.ts index 9ac2d485..f5b99f82 100644 --- a/tests/shell-hooks.test.ts +++ b/tests/shell-hooks.test.ts @@ -2553,4 +2553,125 @@ describe('ensure-proxy behavioral tests', () => { ).toBe(false); }); }); + + // ── FIX 4 (issue #313): stale binPath re-resolution ───────────────────────── + // + // When proxy.json exists with enabled:true but the stored binPath is absent + // (e.g., npx cache GC cleared the subswitch package), the hook must attempt + // re-resolution before emitting the "relay binary not found" warning. + // + // Strategy b (command -v subswitch) is the easiest to test in a controlled env: + // create a fake subswitch script in a shadow PATH directory, set binPath to a + // non-existent path, and verify the hook heals silently (exits 0 with no warning). + // + // Strategy a (devflow walk) is exercised implicitly by the presence of a real + // devflow binary + node_modules/subswitch in the running test environment — + // but we can't rely on that structure in CI, so we keep strategy-b tests here. + + describe('FIX 4 — stale binPath re-resolution (issue #313)', () => { + + it('always exits 0 when binPath is stale (regression: no crash)', async () => { + // Base case: stale path + re-resolution fails (no devflow/subswitch in PATH) + // → hook falls through to warning and exits 0 (same as before fix). + writeProxyJson({ + enabled: true, + port: await allocateFreePort(), + binPath: '/this/stale/path/does/not/exist/relay.js', + }); + const result = spawnSync('bash', [PROXY_HOOK], { + input: JSON.stringify(SESSION_INPUT), + env: { + ...process.env, + HOME: homeDir, + PATH: '/usr/bin:/bin', // restrict PATH so devflow/subswitch not found + }, + encoding: 'utf-8', + }); + expect(result.status).toBe(0); // must always exit 0 + }); + + it('emits relay-binary-not-found warning when re-resolution fails', async () => { + writeProxyJson({ + enabled: true, + port: await allocateFreePort(), + binPath: '/stale/relay.js', + }); + const result = spawnSync('bash', [PROXY_HOOK], { + input: JSON.stringify(SESSION_INPUT), + env: { + ...process.env, + HOME: homeDir, + PATH: '/usr/bin:/bin', + }, + encoding: 'utf-8', + }); + expect(result.status).toBe(0); + // Warning emitted for SessionStart + if (result.stdout) { + try { + const parsed = JSON.parse(result.stdout) as Record; + const output = parsed['hookSpecificOutput'] as Record | undefined; + if (output) { + expect((output['additionalContext'] as string)).toContain('[Devflow proxy]'); + } + } catch { + // stdout not JSON (e.g., empty) — still acceptable as long as exit=0 + } + } + }); + + it('heals silently (exits 0, no warning) when strategy-b finds subswitch via PATH', async () => { + // Create a fake subswitch binary in a shadow bin directory + const fakeBinDir = path.join(tmpDir, 'shadow-bin'); + fs.mkdirSync(fakeBinDir, { recursive: true }); + + // The hook will call `subswitch serve` on the resolved binary — we need a + // script that does not actually start a relay (it will exit immediately). + // The hook only spawns in the UserPromptSubmit+port-up path, which doesn't + // trigger here since port is down. So we just need the binary to exist. + const fakeSubswitch = path.join(fakeBinDir, 'subswitch'); + fs.writeFileSync(fakeSubswitch, '#!/bin/sh\nexec true\n'); + fs.chmodSync(fakeSubswitch, '0755'); + + writeProxyJson({ + enabled: true, + port: await allocateFreePort(), + binPath: '/stale/relay.js', // non-existent stored path + }); + + // PATH must find the fake subswitch (for `command -v subswitch`) as well as + // every binary the hook itself needs (bash, node, cat, ...), so prepend the + // shadow dir to the inherited PATH rather than replacing it. + const result = spawnSync('bash', [PROXY_HOOK], { + input: JSON.stringify(SESSION_INPUT), + env: { + ...process.env, + HOME: homeDir, + PATH: `${fakeBinDir}:${process.env.PATH}`, + }, + encoding: 'utf-8', + }); + + // After re-resolution, binPath is healed — the hook continues past the + // binPath check. On SessionStart with port down, it tries to spawn the relay. + // The fake subswitch exits immediately → relay fails to start → hook emits a + // different warning (port not accepting) and exits 0. The key assertion is + // that exit code is 0 and the "relay binary not found" warning is NOT emitted. + expect(result.status).toBe(0); + + // Should NOT emit the "relay binary not found" message + if (result.stdout) { + try { + const parsed = JSON.parse(result.stdout) as Record; + const output = parsed['hookSpecificOutput'] as Record | undefined; + if (output) { + const ctx = output['additionalContext'] as string ?? ''; + expect(ctx).not.toContain('relay binary not found'); + } + } catch { + // Not JSON — also acceptable (empty stdout = healed past binPath check) + } + } + }); + }); });