fix: harden install/init against destroying proxy/env config (#313) - #314
Open
dean0x wants to merge 12 commits into
Open
fix: harden install/init against destroying proxy/env config (#313)#314dean0x wants to merge 12 commits into
dean0x wants to merge 12 commits into
Conversation
0.2.0's connect timer capped time-to-first-byte and synthesized 504s during upstream slow periods, killing Claude Code sessions machine-wide (subswitch#42). 0.3.0 (ADR-010 relay hardening) removes the TTFB cap and other proxy-side policy. Relay on this machine already restarted onto 0.3.0.
…#313) installSettings now uses a merge strategy (D-SETTINGS-1): hook entries are added idempotently to an existing settings.json by exact command string; statusLine and attribution are set only when absent; user-owned keys (env, permissions, model, apiKeyHelper) are never touched. Parse failure emits a warning and leaves the file byte-identical. Adds exported mergeDevflowSettingsTemplate helper for testing.
Adds proxyJsonExists() to proxy-state.ts — a discriminator helper that distinguishes "file absent" from readProxyState()'s tolerant Ok(defaultState) on ENOENT, which was indistinguishable from a file present with DEFAULT_PROXY_PORT. Gates ANTHROPIC_BASE_URL and CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT stripping in init.ts and uninstall.ts on proxyJsonExists() (D-STRIP-1): strip only when proxy.json exists, proving devflow previously wrote those env vars. Prevents destroying a user's independently-managed ANTHROPIC_BASE_URL during devflow init.
) In runProxyPreflight, the foreign ANTHROPIC_BASE_URL refusal (check ④) now runs BEFORE the adopted early-return (check ③). Previously, a healthy relay on the target port triggered an early-return that silently skipped the foreign-gateway guard entirely — enabling devflow's proxy while a user's custom gateway URL was set. D-EFR-5: readSettingsJson is called in the healthy-relay branch before returning Ok(adopted:true). swallowSettingsReadError semantics preserved: when that flag is true, readSettingsJson returns '{}' on I/O failure instead of throwing.
When the persisted binPath is missing or stale (e.g. npx cache GC cleared the
subswitch package dir), the ensure-proxy hook now attempts dependency-light
re-resolution before emitting the "relay binary not found" warning:
Strategy a: walk up from `command -v devflow` → find node_modules/subswitch →
read bin.subswitch from package.json via node (already present).
Strategy b: `command -v subswitch` — globally installed CLI.
Both strategies are best-effort; the hook falls through to the existing warning
and always exits 0 when re-resolution fails (avoids PF-001, PF-009). The healed
path is used for the current session only; `devflow proxy --enable` persists it.
Loop is bounded at 6 iterations (avoids PF-017).
packaging.test.ts hardcoded SUBSWITCH_VERSION='0.2.0'; the dependency was bumped to 0.3.0 in d087565. Align the guard constant with the current lockfile.
…313) - post-install.ts: hoist existingHooksObj and existing.hooks assignment out of the mergeDevflowSettingsTemplate loop — was re-evaluated on every iteration but always yielded the same reference after the first - ensure-proxy: remove "(moved here from below)" tombstone from node re-resolution comment; describe current state, not the transition
…313) The D-STRIP-1 gate was applied to the whole teardown instead of to the env strip alone, and it was missing entirely from `devflow proxy --disable`. - uninstall: with no proxy.json, `removeProxyHooks` stopped running too, so an uninstall could leave SessionStart/UserPromptSubmit entries pointing at a deleted hook script — and a re-run of an interrupted uninstall (proxy.json already removed by the artifact loop) could never clean them up. - `runDisable`: `applyDisableToSettings` still ran unconditionally with the DEFAULT_PROXY_PORT fallback, so `devflow proxy --disable` on a machine where Devflow never managed the proxy deleted a user's own ANTHROPIC_BASE_URL on 4141 plus CLAUDE_CODE_DISABLE_UNKNOWN_MODEL_WINDOW_ENFORCEMENT — the exact hazard #313 set out to fix. Both paths now route through `applyProxyTeardownToSettings(settings, port?)`: hooks always come out, the env strip runs only against a port the caller read from an existing proxy.json. `applyDisableToSettings` keeps its both-operations invariant and is still what runs on the managed path. Also folds the duplicated D-EFR-5 foreign-env block into a shared `checkSettingsEnv` helper, so the adopted-relay path and the free-port path apply the same check ④ — the adopted path was skipping the malformed-JSON refusal and the ANTHROPIC_API_KEY warning.
The subswitch 0.2.0 -> 0.3.0 bump on this branch retires two keys that were valid under 0.2.0 and are now registered legacy keys — a hard startup error, not a warning: `anthropic.streamIdleTimeoutMs` and `limits.maxConcurrentRequests`. `buildRoutingConfigJson` preserves a user's existing `anthropic` and `limits` blocks wholesale, so anyone who hand-tuned ~/.devflow/proxy-routing.json under 0.2.0 would have had those keys written straight back into the config the 0.3.0 relay reads — the relay refuses to boot, and the ensure-proxy hook just reports "relay failed to start" every session. Both keys join `limits.connectTimeoutMs` in a named ROUTING_CONFIG_REJECTED_SUBKEYS table so the two strip sites cannot drift. The list is scoped to keys a config that worked against the version Devflow actually shipped could contain — keys retired before 0.2.0 are unreachable. Also refreshes the @D-EFR-4 contract comment: in 0.3.0 anthropic.connectTimeoutMs is a genuine DNS+TCP connect budget (armed on the socket, disarmed on 'connect'), not the socket-inactivity timeout that motivated the 120s override under 0.2.0. The `preserves other anthropic fields` fixture used streamIdleTimeoutMs — a shape the pinned runtime now rejects — so it pinned behaviour that would break the relay at startup (avoids PF-043). Retargeted at maxUpstreamSockets.
…apes (#313) The confirm in `installSettings` was harmless while the old `hasHooks` check made it nearly unreachable. With the template merge it fires on every re-init for anyone with a feature turned off (HUD off -> no statusLine, memory off -> no memory hooks), and it fires from inside init's running spinner — the same place init deliberately calls `s.stop()` before prompting for sudo. Declining protected nothing either: init's own settings pass rewrites the whole hook set immediately afterwards. A prompt whose answer changes nothing is worse than no prompt, so the merge (additive only) now runs silently in both TTY and non-TTY. This also removes a `process.exit(0)` that would have fired under that spinner. `mergeDevflowSettingsTemplate` also walked a hand-editable file's shape unguarded: `hooks: []` silently swallowed the devflow entries (JSON.stringify drops keys attached to an array), and a matcher without a `hooks` array threw, which the outer catch turned into "could not configure settings" — hooks never installed. Every branch now validates shape at the sink and leaves foreign shapes untouched (applies PF-023). An empty `hooks` key is no longer introduced into a settings.json that had none.
…vior (#313) Add two tmpdir integration tests to tests/post-install-merge.test.ts: (a) parse-failure bail: write invalid JSON settings.json (trailing comma), call installSettings, assert file bytes identical and no .tmp.* residue. (b) merge wiring: write valid settings.json with env+permissions but no hooks, call installSettings, assert hooks are added and env/permissions survive byte-for-byte. Also correct a stale comment in src/cli/commands/uninstall.ts:1058 that still named applyDisableToSettings for a call that is now applyProxyTeardownToSettings. Co-Authored-By: Claude <noreply@anthropic.com>
Reflects proxy lifecycle changes from issue #313 / PR #314: proxyJsonExists() evidence discriminator, applyProxyTeardownToSettings unified teardown, D-STRIP-1 init.ts env-strip gating, checkSettingsEnv D-EFR-5 shared helper, ensure-proxy binPath re-resolution, post-install mergeDevflowSettingsTemplate merge strategy, and subswitch 0.3.0 routing config contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
installSettingsnow merges devflow hook entries into an existingsettings.jsoninstead of replacing it wholesale. User-owned keys (env,permissions,model,apiKeyHelper, custom hooks) are never touched. Parse failure warns and skips — file left byte-identical. Idempotent by exact command string (D-SETTINGS-1). The interactive confirm prompt was removed entirely — the merge is additive/non-destructive, init's own settings pass rewrote hooks regardless of the answer anyway, and the prompt fired mid-spinner; the non-TTY "Settings exist without hooks" warning is gone too; foreign/unexpected hook shapes are left untouched (PF-023).applyProxyTeardownToSettings(the new exported helper that both uninstall and disable now route through) is unconditional, while env-var stripping remains evidence-gated on~/.devflow/proxy.jsonexistence viaproxyJsonExists().applyDisableToSettings's both-operations invariant (removeProxyHooks+_stripProxyEnvFromObject) is untouched — only the uninstall/disable wiring changed. PreviouslystripProxyEnvusedreadProxyState()'s tolerantOk(defaultState)on ENOENT, which caused stripping to run even when devflow had never managed the env vars (D-STRIP-1).buildRoutingConfigJsonnow strips sub-keys the 0.3.0 runtime hard-rejects on startup —anthropic.streamIdleTimeoutMs,limits.connectTimeoutMs, andlimits.maxConcurrentRequests— collected in the newROUTING_CONFIG_REJECTED_SUBKEYSconstant. The allowed top-key set is unchanged (port,logLevel,anthropic,providers,limits). The packaging test'sSUBSWITCH_VERSIONpin was updated to0.3.0.runProxyPreflight, the foreign-ANTHROPIC_BASE_URLrefusal (check ④) now evaluates before the adopted early-return (check ③). Previously, a healthy relay on the port triggered an early-return that silently skipped the foreign-gateway guard entirely (D-EFR-5).ensure-proxyshell hook now attempts dependency-light re-resolution ofbinPathbefore emitting the "relay binary not found" warning: walk fromcommand -v devflow→node_modules/subswitch/→ readbin.subswitchfrompackage.jsonvianode; fall back tocommand -v subswitch. Loop bounded at 6 iterations. Always exits 0.Changes
src/targets/claude-code/post-install.ts— newmergeDevflowSettingsTemplate()exported helper;installSettings()rewritten to use merge strategy; interactive confirm prompt removedsrc/core/proxy-state.ts— newproxyJsonExists()exported helpersrc/cli/commands/proxy.ts— newapplyProxyTeardownToSettings()exported helper (unconditional hook removal + evidence-gated env strip); D-EFR-5: foreign-env check before adopted early-return inrunProxyPreflight;buildRoutingConfigJsonstripsROUTING_CONFIG_REJECTED_SUBKEYSfor 0.3.0 runtimesrc/cli/commands/init.ts— import + D-STRIP-1 gate forstripProxyEnvsrc/cli/commands/uninstall.ts— import + route throughapplyProxyTeardownToSettings(D-STRIP-1 gate); stale comment correctedsrc/assets/scripts/hooks/ensure-proxy— FIX 4 re-resolution block with bounded walktests/post-install-merge.test.ts— 22 unit tests formergeDevflowSettingsTemplate+ 2 tmpdir integration tests pinninginstallSettingsparse-failure and wiring (24 total)tests/proxy-state.test.ts— +8 tests forproxyJsonExiststests/proxy.test.ts— +11 tests (D-EFR-5 ordering REG-EFR-1/2/3 +ROUTING_CONFIG_REJECTED_SUBKEYSstripping +applyProxyTeardownToSettings)tests/shell-hooks.test.ts— +3 tests for FIX 4 re-resolutiontests/packaging.test.ts— bumpSUBSWITCH_VERSIONconstant to0.3.0Behavior Changes
settings.jsonexists: the interactive confirm asking whether to add Devflow hooks was removed. The merge is additive and non-destructive; declining could not protect any content the subsequent hooks-settings pass would not rewrite anyway. Non-TTY silent-skip path ("Settings exist without hooks" warning) is also removed.Breaking Changes
None.
Reviewer Focus Areas
applyProxyTeardownToSettings:proxy.ts— unconditionalremoveProxyHookscall, then port-gated_stripProxyEnvFromObject; confirm theapplyDisableToSettingstwo-operation invariant remains intactinit.tsline ~1645 anduninstall.tsmanagedProxyPortsmap — both useproxyJsonExists()before touching env varsROUTING_CONFIG_REJECTED_SUBKEYS:proxy.tsbuildRoutingConfigJson— stripped sub-keys must not include any key the 0.3.0 relay still readsproxy.tsrunProxyPreflight—readSettingsJson()is now called inside theisOurRelayBody()branch beforereturn Ok({ adopted: true })mergeDevflowSettingsTemplateuses exact command string matching; a second merge on an already-configured file produceschanged: falsewithout mutating anythingexit 0Related Issues
Closes #313