From bdeaf5da6e5dab7d2762b5b442150eff8ca81a84 Mon Sep 17 00:00:00 2001 From: Ratnesh Maurya <85143283+ratnesh-maurya@users.noreply.github.com> Date: Tue, 6 Oct 2026 11:35:58 +0530 Subject: [PATCH] fix(forge-cli): never record the optimizer's own URL as the value to restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `optimizer start` captured whatever ANTHROPIC_BASE_URL was in ~/.claude/settings.json as prev_base_url, with no check that forge had written it. A second start (including the adopt path, where a proxy is already running and the settings are already wired) therefore saved http://127.0.0.1:8787 — the optimizer's own address — as the thing to revert to. `stop` then restored it faithfully, leaving every `claude` pointed at a dead proxy: API Error: Connection refused (ECONNREFUSED) ENABLE_TOOL_SEARCH had the same flaw: prev_tool_search became forge's own "true", so stop left tool search enabled instead of removing the key. A user whose real base URL was a gateway lost it outright — the second start overwrote the saved gateway, so stop could never put it back. sanitizePrevWiring now discards a captured value that forge wrote itself — matching our current listen addr, or the one the prior daemon state recorded, so a start on a different --listen still spots stale wiring — and carries the prior state's genuine originals forward instead. Because a self-written base URL means the tool-search value is ours too, both are replaced together rather than trusted independently. Fixes #539 --- forge-cli/cmd/optimizer_daemon.go | 52 ++++++++++-- forge-cli/cmd/optimizer_daemon_test.go | 106 +++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 5 deletions(-) diff --git a/forge-cli/cmd/optimizer_daemon.go b/forge-cli/cmd/optimizer_daemon.go index 0c5e1c24..12f15e0a 100644 --- a/forge-cli/cmd/optimizer_daemon.go +++ b/forge-cli/cmd/optimizer_daemon.go @@ -202,8 +202,14 @@ func runOptimizerStart(_ *cobra.Command, _ []string) error { } } - // Wire Claude Code settings so every `claude` inherits the base URL. - prevBase, prevTS, err := wireClaudeSettings(listen) + // Wire Claude Code settings so every `claude` inherits the base URL. A prior + // state file carries the genuine pre-forge values; a re-start must hand them + // over rather than recapture settings forge itself wrote. + var prior *optDaemonState + if p, perr := readOptDaemonState(); perr == nil { + prior = &p + } + prevBase, prevTS, err := wireClaudeSettings(listen, prior) if err != nil { logger.Warn("could not update ~/.claude/settings.json; set ANTHROPIC_BASE_URL yourself", "error", err.Error()) } else { @@ -450,9 +456,42 @@ func captureStr(env map[string]any, key string) *string { return nil } +// forgeWiredBaseURL reports whether raw is a base URL an earlier `start` wrote: +// our current listen addr, or the one the prior daemon state recorded (starting +// on a different --listen leaves the old address behind in settings.json). +func forgeWiredBaseURL(raw, listen string, prior *optDaemonState) bool { + s := strings.TrimSuffix(strings.TrimSpace(raw), "/") + if s == "" { + return false + } + if s == "http://"+listen { + return true + } + return prior != nil && prior.Listen != "" && s == "http://"+prior.Listen +} + +// sanitizePrevWiring keeps `stop` honest. A value read back from settings.json +// is only a genuine "previous" one when forge did not write it itself; when it +// did, the real pre-forge value is whatever the prior state file already holds +// (nil = the key was absent). Without this, a second `start` records the +// optimizer's own URL as prev_base_url and `stop` dutifully restores it, +// leaving every `claude` pointed at a dead proxy with ECONNREFUSED. +func sanitizePrevWiring(prevBase, prevTS *string, listen string, prior *optDaemonState) (*string, *string) { + if prevBase == nil || !forgeWiredBaseURL(*prevBase, listen, prior) { + return prevBase, prevTS + } + // Already wired by us, so the ENABLE_TOOL_SEARCH we read back is ours too — + // neither value may be trusted. Carry the prior originals forward instead. + if prior != nil { + return prior.PrevBaseURL, prior.PrevToolSearch + } + return nil, nil +} + // wireClaudeSettings sets ANTHROPIC_BASE_URL + ENABLE_TOOL_SEARCH under `env`, // returning the prior values (nil = key was absent) so stop can revert exactly. -func wireClaudeSettings(listen string) (*string, *string, error) { +// prior is the state a previous `start` left behind, or nil on a clean start. +func wireClaudeSettings(listen string, prior *optDaemonState) (*string, *string, error) { m, err := readClaudeSettings() if err != nil { return nil, nil, err @@ -461,8 +500,11 @@ func wireClaudeSettings(listen string) (*string, *string, error) { if env == nil { env = map[string]any{} } - prevBase := captureStr(env, "ANTHROPIC_BASE_URL") - prevTS := captureStr(env, "ENABLE_TOOL_SEARCH") + prevBase, prevTS := sanitizePrevWiring( + captureStr(env, "ANTHROPIC_BASE_URL"), + captureStr(env, "ENABLE_TOOL_SEARCH"), + listen, prior, + ) env["ANTHROPIC_BASE_URL"] = "http://" + listen env["ENABLE_TOOL_SEARCH"] = "true" m["env"] = env diff --git a/forge-cli/cmd/optimizer_daemon_test.go b/forge-cli/cmd/optimizer_daemon_test.go index ad60089a..189e0287 100644 --- a/forge-cli/cmd/optimizer_daemon_test.go +++ b/forge-cli/cmd/optimizer_daemon_test.go @@ -148,3 +148,109 @@ func TestResolveChildUpstream_EnvChainingCaptured(t *testing.T) { t.Errorf("self-pointing settings should fall back to env, got %q", got) } } + +func strptr(s string) *string { return &s } + +func TestSanitizePrevWiring(t *testing.T) { + gateway := "https://gw.corp.example" + tests := []struct { + name string + prevBase *string + prevTS *string + listen string + prior *optDaemonState + wantBase *string + wantTS *string + }{ + { + name: "absent key stays absent", + listen: "127.0.0.1:8787", + wantBase: nil, wantTS: nil, + }, + { + name: "real gateway is captured untouched", + prevBase: strptr(gateway), prevTS: strptr("false"), + listen: "127.0.0.1:8787", + wantBase: strptr(gateway), wantTS: strptr("false"), + }, + { + name: "our own listen is never recorded as previous", + prevBase: strptr("http://127.0.0.1:8787"), prevTS: strptr("true"), + listen: "127.0.0.1:8787", + wantBase: nil, wantTS: nil, + }, + { + name: "trailing slash on our own listen still detected", + prevBase: strptr("http://127.0.0.1:8787/"), prevTS: strptr("true"), + listen: "127.0.0.1:8787", + wantBase: nil, wantTS: nil, + }, + { + name: "re-start carries the prior state's real gateway forward", + prevBase: strptr("http://127.0.0.1:8787"), prevTS: strptr("true"), + listen: "127.0.0.1:8787", + prior: &optDaemonState{Listen: "127.0.0.1:8787", PrevBaseURL: strptr(gateway), PrevToolSearch: strptr("false")}, + wantBase: strptr(gateway), wantTS: strptr("false"), + }, + { + name: "start on a new port still spots the old wiring", + prevBase: strptr("http://127.0.0.1:8787"), prevTS: strptr("true"), + listen: "127.0.0.1:9000", + prior: &optDaemonState{Listen: "127.0.0.1:8787", PrevBaseURL: nil, PrevToolSearch: nil}, + wantBase: nil, wantTS: nil, + }, + { + name: "a different localhost service is a real previous value", + prevBase: strptr("http://127.0.0.1:3000"), prevTS: strptr("true"), + listen: "127.0.0.1:8787", + prior: &optDaemonState{Listen: "127.0.0.1:8787"}, + wantBase: strptr("http://127.0.0.1:3000"), wantTS: strptr("true"), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotBase, gotTS := sanitizePrevWiring(tc.prevBase, tc.prevTS, tc.listen, tc.prior) + if !eqStrPtr(gotBase, tc.wantBase) { + t.Errorf("base = %s, want %s", showStrPtr(gotBase), showStrPtr(tc.wantBase)) + } + if !eqStrPtr(gotTS, tc.wantTS) { + t.Errorf("tool search = %s, want %s", showStrPtr(gotTS), showStrPtr(tc.wantTS)) + } + }) + } +} + +// TestSanitizePrevWiring_StopNeverRestoresDeadProxy is the regression for the +// reported break: start -> start -> stop must not leave `claude` pointed at the +// optimizer's own address once the proxy is gone. +func TestSanitizePrevWiring_StopNeverRestoresDeadProxy(t *testing.T) { + const listen = "127.0.0.1:8787" + + // First start: nothing was set before, so nothing is recorded. + base, ts := sanitizePrevWiring(nil, nil, listen, nil) + first := optDaemonState{Listen: listen, PrevBaseURL: base, PrevToolSearch: ts} + + // Second start reads back what the first one wrote. + base, ts = sanitizePrevWiring(strptr("http://"+listen), strptr("true"), listen, &first) + + if base != nil { + t.Fatalf("prev_base_url = %q, want nil; stop would restore a dead proxy", *base) + } + if ts != nil { + t.Fatalf("prev_tool_search = %q, want nil; stop would leave ENABLE_TOOL_SEARCH on", *ts) + } +} + +func eqStrPtr(a, b *string) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + +func showStrPtr(p *string) string { + if p == nil { + return "" + } + return `"` + *p + `"` +}