From 9385ba0a8864a484f8abdf4d0c35cfe444d8b2d1 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 13:49:30 -0400 Subject: [PATCH 1/8] feat: Add abctl configure bob enable/disable/status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IBM Bob is a VS Code fork, so routing it through Cortex is the VS Code way: the "http.proxy" setting in its user settings.json. abctl could already point Claude Code at Cortex (a settings file) and make "bob" mean "abctl exec -- bob" in a shell (configure bobshell, an rc-file function), but not configure the Bob editor itself — so a Bob user had to find http.proxy and the CA trust step by hand. abctl configure bob enable # write the key, print the CA trust command abctl configure bob disable # remove it, print the optional undo abctl configure bob status # report, and act on nothing The proxy address and the CA path are read from ~/.cortex/config.yaml on every run, never hardcoded, so a moved forward_proxy_addr or ca_dir cannot produce a wrong printed command. enable/disable show the one-line change and prompt, keep a .bak, take --yes, and exit 3 when declined — the same convention as the two sibling agents. Certificate trust is printed, never performed. Installing a root CA is a machine-wide change needing sudo, and a tool that silently escalates to make it is not what anyone wants. The keychain commands name ca.crt, not the bundle.crt the request asked for. bundle.crt holds ~129 certificates and exists only for tools whose CA setting REPLACES the trust store (SSL_CERT_FILE and friends, per core/tlsbridge/bundle.go). The keychain is additive, so "add-trusted-cert -r trustRoot" on the bundle would install explicit machine-wide root trust for ~128 unrelated public CAs, and one "delete-certificate -c authbridge-tls-bridge-ca" would not take it back. The System-keychain + sudo form from the request is kept; only the file differs, and enable's message says so in a line. Ownership is decided by bobIsCortexProxy, a url.Parse check on host and port — deliberately not claude-code's isCortexValue, which is a strings.Contains. That looseness matches "http://corp.example.com/?next= 127.0.0.1:47600" and "localhost:47600.evil.example.com". In claude-code it fails safe, making enable refuse; here the same predicate feeds a delete, so the direction of failure inverts into silently removing someone's corporate proxy. Matching on structure rather than on the configured value also lets disable work when the config is gone, which is when a user uninstalling Cortex needs it most. This reverses part of #1133, which removed "configure bob" reasoning that IBM Bob needs no configuring. That was right about the binary and wrong about the editor. TestConfigure_BobIsNoLongerAnAgent is deleted rather than adapted — its premise is gone — and replaced by tests pinning that the two agents are distinct and neither aliases the other. The README paragraph asserting the old judgment is rewritten for the same reason. Deliberately out of scope: http.proxyStrictSSL is not managed (it disables verification, the opposite of the CA step); there is no state file, so a pre-existing loopback proxy on a 476xx port is indistinguishable from ours and disable would remove it (the prompt names the value, and .bak is written); JSONC is refused rather than parsed, since stripping a user's comments to add one key is the wrong trade; non-macOS settings locations are not guessed, because writing into a file nothing reads is a silent no-op. Signed-off-by: Ed Snible Assisted-By: Claude (Anthropic AI) --- cmd/abctl/README.md | 100 ++++- cmd/abctl/cmd_bob.go | 542 +++++++++++++++++++++++++++ cmd/abctl/cmd_bob_test.go | 632 ++++++++++++++++++++++++++++++++ cmd/abctl/cmd_configure.go | 42 ++- cmd/abctl/cmd_configure_test.go | 85 +++-- cmd/abctl/main.go | 2 +- 6 files changed, 1363 insertions(+), 40 deletions(-) create mode 100644 cmd/abctl/cmd_bob.go create mode 100644 cmd/abctl/cmd_bob_test.go diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index 28176c35c..8727248d5 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -323,12 +323,18 @@ and 3 is what lets an unattended caller tell the two apart — the same code unattended caller that omits `--yes` is still a no-op; it is just no longer a silent one. Scripted callers should pass `--yes`. -`abctl configure bob` no longer exists — it is `bobshell`, because what gets -configured is the Bob Shell integration and not Bob itself. This is a breaking -change and not an alias: the old spelling printed "coming soon" and exited 0, -and now exits 2 with `unknown agent "bob"`, so a script that ran it and checked -the status starts failing rather than silently doing nothing. The error names -`bobshell`, so the fix is visible at the point of failure. +`bob` and `bobshell` are two different agents, not two spellings of one. `bob` +configures the IBM Bob **editor** — a VS Code fork, so the lever is `http.proxy` +in its `settings.json` (below). `bobshell` configures the **shell integration** — +a `bob` function in your rc file, so typing `bob` at a prompt runs through +Cortex. Configuring one does not configure the other, and neither name is an +alias of the other: `abctl configure bob --settings X` is a usage error under +`bobshell`, and vice versa. + +The editor agent briefly did not exist. It was removed on the reasoning that +what needed configuring was the shell integration and "IBM Bob itself needs no +configuring", which was right about the binary and wrong about the editor. It is +back, and the two now stand side by side. Let `enable` write it rather than pasting the block above. What `enable` appends begins with a blank line, which the fence cannot show you: it is invisible when @@ -426,6 +432,88 @@ bash, zsh and dash alike. (`mise doctor` splits `activated:` from Both answers exit 0: "not enabled" is a report, not a failure. +## Routing the IBM Bob editor through Cortex (`abctl configure bob`) + +IBM Bob is a VS Code fork, so it reads the VS Code proxy setting. `abctl +configure bob enable` writes exactly one flat, top-level key into Bob's user +settings: + +```json +{ + "http.proxy": "http://127.0.0.1:47600" +} +``` + +Flat and dotted, not nested under an `"http"` object — that is the shape VS Code +reads, and the shape difference from `configure claude-code`, which writes a +nested `"env"` block. The address is read from `listener.forward_proxy_addr` in +`~/.cortex/config.yaml` on every run, so a moved port or an IPv6 loopback +produces the right value rather than a hardcoded 47600. + +```sh +abctl configure bob enable # write the key, print the CA trust command +abctl configure bob disable # remove the key, print the optional undo +abctl configure bob status # report, and act on nothing +``` + +`enable` and `disable` show the one-line change and ask before writing, keep a +`.bak` of the file as it was first found, and take `--yes` for unattended use. +Declined — or with no terminal to ask on — they write nothing and exit **3**, the +same convention as `configure claude-code` and `configure bobshell`. `--settings +PATH` and `--config PATH` override either file. Restart Bob afterwards: whether +it re-reads a proxy change live is unverified, so the message says restart rather +than guess. + +### Certificate trust is printed, never performed + +The proxy terminates TLS with a forged leaf, so Bob has to trust Cortex's bridge +CA or every HTTPS request fails. Installing a root CA is a machine-wide change +needing `sudo`, and a tool that silently escalates to do it is not what anyone +wants — so `enable` prints the exact command and stops: + +```sh +sudo security add-trusted-cert -d -r trustRoot \ + -k /Library/Keychains/System.keychain ~/.cortex/ca/ca.crt +``` + +`disable` prints the matching `security delete-certificate` and says it is safe +to leave the certificate in place. `status` suggests `security verify-cert` +without running it. Off macOS these become a suggestion to add the file to the +OS trust store, naming the usual Debian and Fedora routes and saying plainly +that the exact step depends on the distribution. + +It is `ca.crt` — the single bridge CA — and deliberately **not** the +`bundle.crt` in the same directory, which holds ~129 certificates and exists for +tools whose CA setting *replaces* the trust store (`SSL_CERT_FILE` and friends, +as `abctl exec` sets). The keychain is additive, so `-r trustRoot` on the bundle +would install explicit machine-wide root trust for ~128 unrelated public CAs, +and one `delete-certificate` would not take it back. + +### What it knows, and what it does not + +"Enabled" means the key is present and its value is a loopback host on a `476xx` +port. That is a structural check on the value, not a record abctl keeps: there is +no state file, so `disable` removes the key only when it still looks like +something `enable` wrote, and reports anything else — a corporate proxy, a +non-string value — while leaving it alone. `enable` refuses rather than +overwriting a foreign value. The cost of having no state file is that a +pre-existing loopback proxy of your own on a `476xx` port is indistinguishable +from Cortex's: `disable` would remove it. The prompt names the exact value first, +and the `.bak` is already written. + +`http.proxy` governs VS Code's core networking and its extension host. An +extension that bundles its own HTTP client can still go around it; this is the +documented lever, not a guarantee of coverage. + +A settings file with comments in it is refused, not rewritten. VS Code permits +them; the strict JSON reader here does not, and silently stripping a user's +comments to add one key is the wrong trade. + +Only macOS's settings location is known (`~/Library/Application Support/IBM +Bob/User/settings.json`). Elsewhere `--settings PATH` is required rather than +guessed — writing a proxy setting into a file nothing reads is a silent no-op, +which is worse than a refusal that names the flag. + ## Panes The UI has these panes. `Enter` drills in; `Esc` backs out. diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go new file mode 100644 index 000000000..a76be9b85 --- /dev/null +++ b/cmd/abctl/cmd_bob.go @@ -0,0 +1,542 @@ +package main + +import ( + "errors" + "flag" + "fmt" + "io" + "net/url" + "os" + "path/filepath" + "runtime" + "strings" + + "github.com/rossoctl/cortex/core/tlsbridge" +) + +const ( + // bobSettingsRel is where IBM Bob keeps its user settings on macOS. Bob is a + // VS Code fork, so this is VS Code's own layout with Bob's product name in it. + bobSettingsRel = "Library/Application Support/IBM Bob/User/settings.json" + + // bobProxyKey is the one setting this command writes. VS Code's settings + // document is FLAT — dotted keys are literal top-level keys, not a nested + // object — so this is the whole path, unlike claude-code's "env" block. + bobProxyKey = "http.proxy" + + // bobCACommonName is the subject every generated bridge CA carries. A literal + // rather than an import: bridgeCACommonName lives in package main of + // cmd/authbridge-proxy and is not importable from here. darwinGoNote already + // carries the same copy for the same reason. + bobCACommonName = "authbridge-tls-bridge-ca" + + // bobSystemKeychain is machine-wide trust, which is the right scope for a + // proxy that every process on the box is pointed at. + bobSystemKeychain = "/Library/Keychains/System.keychain" +) + +const bobUsage = `abctl configure bob — route IBM Bob through Cortex via its settings.json + +Usage: + abctl configure bob enable [--yes] [--settings PATH] [--config PATH] + abctl configure bob disable [--yes] [--settings PATH] [--config PATH] + abctl configure bob status [--settings PATH] [--config PATH] + +Flags: + --yes do not prompt for confirmation + --settings PATH IBM Bob's settings file. Defaults, on macOS only, to + ~/Library/Application Support/IBM Bob/User/settings.json + --config PATH Cortex config to read the proxy address from + (default ~/.cortex/config.yaml) + +IBM Bob is a VS Code fork, so it has a settings file and one flat key decides +where its networking goes: "http.proxy". enable sets it to Cortex's forward +proxy, read from ~/.cortex/config.yaml so the address always matches the proxy +that is actually running. Only that one key is written — every other setting is +left exactly as it was, and the first write copies the original to +settings.json.bak and never overwrites that copy. + +disable removes "http.proxy" ONLY when it points at Cortex. A value that does +not — a corporate proxy, say — is reported and left alone, because a proxy abctl +did not write is not abctl's to delete. + +The proxy is only half of it. Bob's requests are terminated by Cortex's TLS +bridge, so Bob must also trust the bridge CA, and that is an OS trust-store +change abctl does not make for you: it needs sudo, and a tool that silently +escalates to alter machine-wide trust is not one you can audit. enable prints +the command that does it, disable prints how to undo it, status prints how to +check it. + +"abctl configure bobshell" is a different thing and the two are independent. +That one defines a "bob" shell function so typing "bob" runs through Cortex; +this one configures what the editor does on its own. + +Comments: VS Code permits them in settings.json and this command does not. It +reads the file as strict JSON and refuses a commented one by name rather than +rewriting it, because a rewrite would silently delete the comments. + +Exit status: 0 applied, already correct, or reported; 3 declined — at the prompt +or because there was no terminal to ask on; 1 something went wrong; 2 a usage +error. +` + +// bobConfirm prompts before a write. A var so tests can substitute it: `go test` +// inherits the terminal it was launched from, so an unstubbed prompt blocks +// waiting on a human. +// +// Deliberately a separate var from bobShellConfirm rather than a reuse of it. +// They are identical today, but sharing one would mean a test stubbing one +// verb's prompt silently disarms the other command's too — and a test that +// cannot fail is worse than a duplicated three-line closure. +var bobConfirm = func(path, what string, stdout io.Writer) bool { + fmt.Fprintf(stdout, "%s %s\n", what, path) + return confirm(stdout) +} + +// runBob dispatches `abctl configure bob`. Returns the process exit code. +func runBob(args []string, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprint(stderr, bobUsage) + return 2 + } + action := args[0] + // Before anything else: --help asks for this command's usage, and reading it + // as an action name would send someone looking for the command list to the one + // branch that refuses to print it. Same split as bobshell's and claude-code's. + switch action { + case "-h", "--help", "help": + fmt.Fprint(stdout, bobUsage) + return 0 + } + + // The verb is validated HERE, before any environment or filesystem work, for + // the reason runBobShell documents at length: a step below can answer + // successfully on its own — bobSettingsPath's off-macOS arm prints advice — + // so validating the action last would report success for a verb that does not + // exist. Nothing downstream can reach this check, so it comes first. + switch action { + case "enable", "disable", "status": + default: + fmt.Fprintf(stderr, "abctl: unknown bob action %q (enable, disable, status)\n", action) + return 2 + } + + fs := flag.NewFlagSet("configure bob "+action, flag.ContinueOnError) + fs.SetOutput(stderr) + settingsPath := fs.String("settings", "", "IBM Bob settings file") + // Registered for all three verbs, status included: status reports whether the + // value still matches the proxy the config names now, which needs the config. + cortexCfgPath := fs.String("config", "", "Cortex config file") + // --yes for enable and disable ONLY. status writes nothing, so it has nothing + // to confirm; registering it unconditionally would make `status --yes` parse + // and be silently ignored, which is the same "accepted and did nothing" that + // `status extra` is already a usage error for. Read back into a plain bool + // below so the call sites cannot nil-deref. + var yesFlag *bool + if action == "enable" || action == "disable" { + yesFlag = fs.Bool("yes", false, "do not prompt for confirmation") + } + // The FlagSet's own usage would print a bare header and a short flag list; + // this command's usage is the useful answer, and suppressing it here keeps + // -h's single copy on stdout below. + fs.Usage = func() {} + if err := fs.Parse(args[1:]); err != nil { + // -h and --help arrive as flag.ErrHelp, and asking for help is not a usage + // error: stdout and 0. Any other parse failure is real, and Parse has + // already named it on stderr. + if errors.Is(err, flag.ErrHelp) { + fmt.Fprint(stdout, bobUsage) + return 0 + } + return 2 + } + yes := yesFlag != nil && *yesFlag + if fs.NArg() > 0 { + fmt.Fprintf(stderr, "abctl: bob %s takes no arguments (got %q)\n", action, fs.Arg(0)) + return 2 + } + + home, err := os.UserHomeDir() + if err != nil || home == "" { + fmt.Fprintf(stderr, "abctl: cannot determine your home directory: %v\n", err) + return 1 + } + if *cortexCfgPath == "" { + *cortexCfgPath = filepath.Join(home, cortexCfgRel) + } + if *settingsPath == "" { + p, perr := bobSettingsPath(home) + if perr != nil { + // 2, not 1: the missing fact is one the caller can supply on the command + // line, which makes this a usage error rather than a failure. Unlike + // bobshell's off-platform arm there is no block of text to paste that + // would constitute an answer on its own. + fmt.Fprintf(stderr, "abctl: %v.\n Pass --settings PATH to point at it.\n", perr) + return 2 + } + *settingsPath = p + } + + switch action { + case "enable": + // enable derives the CA path itself, from the config it already requires. + return bobEnable(*settingsPath, *cortexCfgPath, yes, stdout, stderr) + case "disable": + return bobDisable(*settingsPath, bobCAPath(*cortexCfgPath, home), yes, stdout, stderr) + default: + return bobStatus(*settingsPath, *cortexCfgPath, bobCAPath(*cortexCfgPath, home), stdout) + } +} + +// bobSettingsPath is where IBM Bob's settings live, for the platforms where that +// is known. +// +// macOS only, deliberately. ~/.config/IBM Bob/User/settings.json is the VS Code +// convention on Linux and would be the obvious guess, but it is unverified for +// this app — and writing a proxy setting into a file nothing reads is a silent +// no-op that leaves the user no reason to look there. bobShellRCPath makes the +// same call for an unrecognised shell: claim only what you know, and say so +// otherwise. --settings covers every platform, so nothing is blocked. +// +// home is a parameter rather than read from the environment, so the mapping is +// testable and runBob reads $HOME exactly once. +func bobSettingsPath(home string) (string, error) { + if runtime.GOOS != "darwin" { + return "", fmt.Errorf("I only know where IBM Bob keeps its settings on macOS, and this is %s", runtime.GOOS) + } + return filepath.Join(home, bobSettingsRel), nil +} + +// bobIsCortexProxy reports whether an http.proxy value is one abctl wrote, +// structurally: loopback host, port in Cortex's 476xx block, http scheme. +// +// Deliberately NOT isCortexValue, which is the same question asked with +// strings.Contains. That form matches a substring anywhere, so +// "http://corp.example.com/?next=127.0.0.1:47600" and the hostname +// "localhost:47600.evil.com" both satisfy it. In claude-code the consequence of +// a false positive is REFUSING to overwrite, so it fails safe; here the +// consequence is delete, and the direction of failure inverts — a false positive +// silently removes someone's corporate proxy. Parsing and comparing Hostname() +// and Port() whole cannot be fooled by a path or a suffixed host. +// +// It also reads nothing from the config, which is what lets disable keep working +// after forward_proxy_addr moves, after wantedFromLoaded rewrites a bind address +// to "localhost", and — the case that decides it — when the config is gone +// entirely. Someone who has uninstalled Cortex and wants Bob working again must +// not find the off switch broken by the absence of the thing being switched off. +// +// The residual false positive is an unrelated loopback proxy of the user's own +// on a 476xx port. Accepted: that is a deliberate collision with Cortex's +// documented port block, disable names the exact value in the prompt before +// removing it, and writeSettings has already saved a .bak. Exact comparison +// against the derived value still earns its place in status, which reports drift +// rather than acting on it. +func bobIsCortexProxy(val string) bool { + u, err := url.Parse(strings.TrimSpace(val)) + if err != nil || u.Host == "" { + return false + } + // http only: it is the only scheme enable ever writes, so anything else is + // someone else's value. + if u.Scheme != "http" { + return false + } + switch u.Hostname() { + case "localhost", "127.0.0.1", "::1": + default: + return false + } + return strings.HasPrefix(u.Port(), "476") +} + +// bobTrustNote is the trust-store step abctl does NOT take, printed for the user +// to run. +// +// Printed rather than executed on purpose. Adding a root to the System keychain +// needs sudo, and a tool that escalates on its own to change machine-wide trust +// is not one anybody can audit after the fact — the user should see the exact +// command before it runs. Same reasoning as darwinGoNote, which prints its +// keychain command too. +// +// caPath is ca.crt — the single bridge CA — and NOT bundle.crt, even though +// bundle.crt is the file most of abctl's other CA messages name. The keychain is +// ADDITIVE, so it wants one certificate; bundle.crt exists for the tools whose CA +// setting REPLACES their trust store, and it holds the bridge CA followed by +// every platform root (129 certificates on this machine). +// `add-trusted-cert -r trustRoot` on that file would install explicit root-trust +// settings for ~128 unrelated public CAs machine-wide, which is both far broader +// than intended and not undone by the single delete-certificate below. See +// core/tlsbridge/bundle.go's header, which states which file is for which job. +func bobTrustNote(caPath string) string { + q := shellQuote(caPath) + if runtime.GOOS == "darwin" { + return "Bob must also trust the bridge CA, or every https request fails verification.\n" + + " That is a machine-wide trust change needing sudo, so abctl does not make it\n" + + " for you — run:\n\n" + + " sudo security add-trusted-cert -d -r trustRoot \\\n" + + " -k " + bobSystemKeychain + " " + q + "\n\n" + + " That is the single bridge CA, not the " + tlsbridge.TrustBundleName + " in the same\n" + + " directory: the keychain adds to what it already trusts, so it wants one\n" + + " certificate. The bundle carries every platform root as well, and trusting it\n" + + " as a root would change how this machine treats certificates that have\n" + + " nothing to do with Cortex.\n\n" + } + // Suggestive by design. The exact route differs per distribution and a + // confidently wrong command is worse than a named one plus a caveat. + return "Bob must also trust the bridge CA, or every https request fails verification.\n" + + " Add " + q + " to this system's trusted certificate store. The exact\n" + + " step depends on the distribution; the usual ones are:\n\n" + + " Debian/Ubuntu: copy it into /usr/local/share/ca-certificates/ (renamed to\n" + + " end in .crt) and run sudo update-ca-certificates\n" + + " Fedora/RHEL: copy it into /etc/pki/ca-trust/source/anchors/ and run\n" + + " sudo update-ca-trust\n\n" + + " Some applications keep their own trust store and need it added there too.\n" + + " That is the single bridge CA, not the " + tlsbridge.TrustBundleName + " beside it, which\n" + + " additionally carries every platform root.\n\n" +} + +// bobUntrustNote is the undo for bobTrustNote, and says plainly that it is +// optional: a CA left in the store signs nothing but Cortex's own forged leaves, +// and removing it is tidiness rather than a fix. +func bobUntrustNote(caPath string) string { + if runtime.GOOS == "darwin" { + // The keychain has to be named: an add to the System keychain is not undone + // by a delete that defaults to the login one. -t drops the trust settings + // add-trusted-cert created along with the certificate itself. + return "Optionally, remove the bridge CA from the keychain as well:\n\n" + + " sudo security delete-certificate -c " + bobCACommonName + " \\\n" + + " -t " + bobSystemKeychain + "\n\n" + + " Safe to leave in place if you expect to re-enable: it only validates\n" + + " certificates Cortex itself issues, so nothing else starts being trusted\n" + + " because it is there.\n\n" + } + return "Optionally, remove " + shellQuote(caPath) + " from this system's\n" + + " trusted certificate store — delete the copy you added (under\n" + + " /usr/local/share/ca-certificates/ or /etc/pki/ca-trust/source/anchors/) and\n" + + " re-run update-ca-certificates / update-ca-trust.\n\n" + + " Safe to leave in place if you expect to re-enable: it only validates\n" + + " certificates Cortex itself issues.\n\n" +} + +// bobVerifyNote is status's suggestion, and is only ever printed — running +// verify-cert here would turn a report into an action, and status writes nothing. +// +// Nothing is printed off macOS: there is no portable equivalent to suggest, and +// inventing one is the confidently-wrong-command failure bobTrustNote's non-mac +// arm already hedges against. +func bobVerifyNote(caPath string) string { + if runtime.GOOS != "darwin" { + return "" + } + return "To check that this machine trusts the bridge CA:\n\n" + + " security verify-cert -c " + shellQuote(caPath) + "\n\n" + + " Not run here — status reports, it does not act. A failure does not by itself\n" + + " mean Bob is misconfigured: the CA only matters for hosts the bridge\n" + + " terminates, and it is trusted separately from the proxy setting above.\n" +} + +// bobCAPath is the bridge CA path for a message, best-effort. +// +// Best-effort because disable and status must keep working when the config does +// not: a user who has uninstalled Cortex still needs the off switch, and +// refusing to print a cleanup hint because the config that named the CA is gone +// would be a worse answer than printing the conventional path. Only enable +// requires the config, and it checks that for itself. +func bobCAPath(cortexCfgPath, home string) string { + if want, _, err := wantedFromConfig(cortexCfgPath); err == nil && want[envCACerts] != "" { + return want[envCACerts] + } + return filepath.Join(home, ".cortex", "ca", "ca.crt") +} + +func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.Writer) int { + want, cfg, err := wantedFromConfig(cortexCfgPath) + if err != nil { + fmt.Fprintf(stderr, "abctl: %v\n", err) + return 1 + } + // Same gate as claude-code and exec, and for the same reason: with the bridge + // off Cortex terminates no TLS, so pointing Bob at the proxy would send every + // https request into something that cannot answer it. + if !bridgeEnabled(cfg) { + fmt.Fprintf(stderr, "abctl: %v\n", errBridgeDisabled(cortexCfgPath)) + return 1 + } + caPath := want[envCACerts] + if caPath == "" { + fmt.Fprintf(stderr, "abctl: %s has no tls_bridge.ca_dir, so there is no CA for IBM Bob to\n"+ + " trust; every https request would fail certificate verification. Enable the\n"+ + " TLS bridge first.\n", cortexCfgPath) + return 1 + } + proxy := want[envProxy] + + doc, err := readSettings(settingsPath) + if err != nil { + // readSettings names the file and says to fix or move it. The clause about + // comments is bob's own: Bob is a VS Code fork, VS Code permits comments in + // settings.json and writes them back, so a commented file is the likeliest + // way to arrive here — and the generic "not valid JSON" does not hint at it. + fmt.Fprintf(stderr, "abctl: %v.\n"+ + " If Bob has saved comments in it, that is why: VS Code allows them and this\n"+ + " command reads strict JSON. Remove them, or point --settings elsewhere —\n"+ + " abctl will not rewrite the file to strip them.\n", err) + return 1 + } + + switch existing := doc[bobProxyKey].(type) { + case nil: + // Not set. Nothing to weigh. + case string: + if existing == proxy { + fmt.Fprintf(stdout, "Already enabled: %s routes IBM Bob through Cortex.\n", settingsPath) + return 0 + } + if !bobIsCortexProxy(existing) { + // Refuse rather than overwrite: the overwhelmingly likely owner of a + // foreign value is a corporate proxy the user needs, and this command + // keeps no record that could restore it. + fmt.Fprintf(stderr, "abctl: %s already sets %q to %q, which is not a Cortex proxy.\n"+ + " Leaving it alone — remove or change it yourself if you want Cortex there\n"+ + " instead.\n", settingsPath, bobProxyKey, existing) + return 1 + } + // Ours but stale — the proxy moved. Falls through to the write. + default: + // A non-string (a number, an object) is not something this command wrote and + // not something it can compare. Same refusal as a foreign string. + fmt.Fprintf(stderr, "abctl: %s sets %q to a non-string value (%T).\n"+ + " Leaving it alone — fix it yourself if you want Cortex there instead.\n", + settingsPath, bobProxyKey, existing) + return 1 + } + + // Enabling before Cortex's first start is legitimate — the proxy generates the + // CA on boot — but the trust command printed below would fail on a missing + // file, so say so now rather than let it be discovered at the sudo prompt. + if _, serr := os.Stat(caPath); serr != nil { + fmt.Fprintf(stdout, "Note: %s does not exist yet.\n"+ + " Cortex creates it on first start. Start Cortex, then run the trust command\n"+ + " below — until the CA is trusted, Bob's https requests fail verification.\n\n", + caPath) + } + + fmt.Fprintf(stdout, "Sets in %s:\n %q: %q\n", settingsPath, bobProxyKey, proxy) + fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak\n\n", settingsPath) + if !yes && !bobConfirm(settingsPath, "Write to", stdout) { + fmt.Fprintln(stdout, "Not changed.") + return exitDeclined + } + + doc[bobProxyKey] = proxy + if werr := writeSettings(settingsPath, doc); werr != nil { + fmt.Fprintf(stderr, "abctl: %v\n", werr) + return 1 + } + + // Deliberately not "all Bob traffic now goes through Cortex": http.proxy is what + // VS Code's own networking and its extension host read, and it is the only + // documented lever — but an extension bundling its own HTTP client can bypass + // it. Claiming coverage abctl cannot deliver is how a user stops looking for the + // real reason something is unparsed. + fmt.Fprintf(stdout, "\nEnabled. %q in %s now points at Cortex — this is the setting\n"+ + "VS Code forks read for their own networking and their extension host.\n\n", + bobProxyKey, settingsPath) + // Restart, rather than a claim either way about live pickup: writeSettings is + // temp+rename so Bob never sees a half-written file, but whether Bob re-reads a + // proxy change without restarting is not something this command has verified. + fmt.Fprint(stdout, "Restart Bob so it re-reads its settings.\n\n") + fmt.Fprint(stdout, bobTrustNote(caPath)) + fmt.Fprintf(stdout, "Undo with: abctl configure bob disable\n") + return 0 +} + +func bobDisable(settingsPath, caPath string, yes bool, stdout, stderr io.Writer) int { + doc, err := readSettings(settingsPath) + if err != nil { + fmt.Fprintf(stderr, "abctl: %v\n", err) + return 1 + } + + existing, ok := doc[bobProxyKey] + if !ok { + fmt.Fprintf(stdout, "Not enabled: %s sets no %q. Nothing to do.\n", settingsPath, bobProxyKey) + return 0 + } + s, isString := existing.(string) + if !isString || !bobIsCortexProxy(s) { + // Exit 0, not 1. "Remove it only if it points at Cortex" is the contract, and + // this file already satisfies it — there is nothing for the user to fix, so + // reporting a failure would be wrong. + fmt.Fprintf(stdout, "%s sets %q to %v, which is not a Cortex proxy.\n"+ + " Left alone — abctl removes only values it would have written.\n", + settingsPath, bobProxyKey, existing) + return 0 + } + + fmt.Fprintf(stdout, "Removes from %s:\n %q: %q\n", settingsPath, bobProxyKey, s) + fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak\n\n", settingsPath) + if !yes && !bobConfirm(settingsPath, "Write to", stdout) { + fmt.Fprintln(stdout, "Not changed.") + return exitDeclined + } + + delete(doc, bobProxyKey) + if werr := writeSettings(settingsPath, doc); werr != nil { + fmt.Fprintf(stderr, "abctl: %v\n", werr) + return 1 + } + + fmt.Fprintf(stdout, "\nDisabled. IBM Bob no longer routes through Cortex.\n") + fmt.Fprint(stdout, "Restart Bob so it re-reads its settings.\n\n") + fmt.Fprint(stdout, bobUntrustNote(caPath)) + return 0 +} + +func bobStatus(settingsPath, cortexCfgPath, caPath string, stdout io.Writer) int { + // Always exit 0: "not enabled" is a successful report, the same call + // claudeCodeStatus and bobShellStatus make. A non-zero status here would make + // `abctl configure bob status` unusable in a shell conditional for anything but + // "is it on". + doc, err := readSettings(settingsPath) + if err != nil { + fmt.Fprintf(stdout, "not enabled (%v)\n", err) + return 0 + } + + switch existing := doc[bobProxyKey].(type) { + case nil: + fmt.Fprintf(stdout, " %q (unset)\nnot enabled in %s\n", bobProxyKey, settingsPath) + case string: + fmt.Fprintf(stdout, " %q=%s\n", bobProxyKey, existing) + switch { + case !bobIsCortexProxy(existing): + fmt.Fprintf(stdout, "not enabled in %s (that is not a Cortex proxy)\n", settingsPath) + default: + // Drift is reported, never acted on. wantedFromConfig is consulted only + // here, and its failure is not this report's failure: the value is still + // a Cortex proxy whatever the config says, so an unreadable config + // downgrades the answer rather than breaking it. + want, _, werr := wantedFromConfig(cortexCfgPath) + switch { + case werr != nil: + fmt.Fprintf(stdout, "enabled in %s (could not read %s to compare: %v)\n", + settingsPath, cortexCfgPath, werr) + case want[envProxy] != existing: + fmt.Fprintf(stdout, "enabled in %s, but %s now names %s —\n"+ + " re-run `abctl configure bob enable` to move it.\n", + settingsPath, cortexCfgPath, want[envProxy]) + default: + fmt.Fprintf(stdout, "enabled in %s\n", settingsPath) + } + } + default: + fmt.Fprintf(stdout, " %q=%v\nnot enabled in %s (that value is a %T, not a string)\n", + bobProxyKey, existing, settingsPath, existing) + } + + if note := bobVerifyNote(caPath); note != "" { + fmt.Fprint(stdout, "\n"+note) + } + return 0 +} diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go new file mode 100644 index 000000000..2adf64db6 --- /dev/null +++ b/cmd/abctl/cmd_bob_test.go @@ -0,0 +1,632 @@ +package main + +import ( + "bytes" + "encoding/json" + "io" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// bobSettings is a plausible IBM Bob settings.json: flat dotted keys, which is the +// whole shape difference from Claude Code's nested "env" block. Deliberately long +// because TestBobEnable_PreservesEverythingElse is the test that matters most here — +// this is a file the user has configured by hand and we are editing it for them. +const bobSettings = `{ + "workbench.colorTheme": "Default Dark Modern", + "editor.fontSize": 13, + "editor.fontFamily": "Menlo, Monaco, monospace", + "editor.tabSize": 4, + "editor.formatOnSave": true, + "editor.rulers": [80, 100], + "files.autoSave": "onFocusChange", + "files.trimTrailingWhitespace": true, + "terminal.integrated.fontSize": 12, + "git.autofetch": true, + "telemetry.telemetryLevel": "off", + "http.proxyAuthorization": "keep-me", + "extensions.autoUpdate": false +}` + +// noPrompt stubs the confirmation for the duration of one test. +// +// Necessary, not tidiness: `go test` inherits the terminal it was launched from, so an +// unstubbed bobConfirm opens /dev/tty and blocks the run waiting for a keystroke. +// Returns the count of prompts so a test can assert one was actually asked. +func noPrompt(t *testing.T, answer bool) *int { + t.Helper() + asked := 0 + saved := bobConfirm + bobConfirm = func(path, what string, stdout io.Writer) bool { + asked++ + return answer + } + t.Cleanup(func() { bobConfirm = saved }) + return &asked +} + +// bobDoc reads the settings file back as a flat map. +func bobDoc(t *testing.T, path string) map[string]any { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var doc map[string]any + if err := json.Unmarshal(b, &doc); err != nil { + t.Fatalf("result is not valid JSON: %v\n%s", err, b) + } + return doc +} + +// The basic claim: the flat key lands, with the value derived from the config. +func TestBobEnable_WritesHTTPProxy(t *testing.T) { + settings, cfg := fixture(t, "{}") + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + if got := bobDoc(t, settings)[bobProxyKey]; got != "http://127.0.0.1:47600" { + t.Errorf("%s = %v, want the config's proxy", bobProxyKey, got) + } + // Top-level and flat, not nested under anything — the VS Code shape. + if _, nested := bobDoc(t, settings)["http"]; nested { + t.Error(`wrote a nested "http" object; VS Code reads the flat dotted key`) + } +} + +// The property that matters most: this is the user's own editor configuration. +func TestBobEnable_PreservesEverythingElse(t *testing.T) { + settings, cfg := fixture(t, bobSettings) + before := bobDoc(t, settings) + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + + after := bobDoc(t, settings) + for k, want := range before { + got, ok := after[k] + if !ok { + t.Errorf("key %q was dropped", k) + continue + } + // Compare through JSON so numbers and arrays compare by value. + wj, _ := json.Marshal(want) + gj, _ := json.Marshal(got) + if string(wj) != string(gj) { + t.Errorf("key %q changed: %s -> %s", k, wj, gj) + } + } + // Exactly one key added, and it is ours. A neighbouring key that merely looks + // related — http.proxyAuthorization — must be left alone, which the loop above + // covers and this states. + if len(after) != len(before)+1 { + t.Errorf("key count %d -> %d, want exactly one added", len(before), len(after)) + } + if after["http.proxyAuthorization"] != "keep-me" { + t.Errorf("a neighbouring http.* key was touched: %v", after["http.proxyAuthorization"]) + } + if _, err := os.Stat(settings + ".bak"); err != nil { + t.Errorf("no backup written: %v", err) + } +} + +// Running enable twice must not prompt, rewrite, or fail the second time. +func TestBobEnable_IsIdempotent(t *testing.T) { + settings, cfg := fixture(t, "{}") + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("first run: exit %d: %s", code, errb.String()) + } + first, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + asked := noPrompt(t, false) // would decline; must not be consulted at all + out.Reset() + errb.Reset() + if code := bobEnable(settings, cfg, false, &out, &errb); code != 0 { + t.Fatalf("second run: exit %d: %s", code, errb.String()) + } + if !strings.Contains(out.String(), "Already enabled") { + t.Errorf("second run does not report the existing state:\n%s", out.String()) + } + if *asked != 0 { + t.Errorf("prompted %d times for a no-op write", *asked) + } + second, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(first, second) { + t.Error("the file was rewritten by a run that had nothing to change") + } +} + +// A corporate proxy is someone's working network configuration. Refuse, name it, and +// change nothing — do not "fix" it. +func TestBobEnable_RefusesAForeignProxy(t *testing.T) { + settings, cfg := fixture(t, `{"http.proxy": "http://proxy.corp.example.com:3128"}`) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 1 { + t.Fatalf("exit = %d, want 1", code) + } + if !strings.Contains(errb.String(), "proxy.corp.example.com:3128") { + t.Errorf("the refusal does not name the value it refused to replace:\n%s", errb.String()) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("a refused enable still wrote to the file") + } + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Error("a refused enable left a .bak, so it opened the file for writing") + } +} + +// Hardcoding 47600 and ~/.cortex/ca would point Bob at nothing the moment someone +// edited their config. The IPv6 form is the one that broke a sibling command: a +// strings.Cut on ":" produced http://[:1]:47600 from [::1]:47600. +func TestBobEnable_DerivesPortAndCAFromConfig(t *testing.T) { + for _, tc := range []struct { + name, addr, wantProxy string + }{ + {"moved port", "127.0.0.1:19999", "http://127.0.0.1:19999"}, + {"ipv6 loopback", "[::1]:47655", "http://[::1]:47655"}, + {"wildcard host becomes loopback", "0.0.0.0:47600", "http://localhost:47600"}, + } { + t.Run(tc.name, func(t *testing.T) { + settings, cfg := fixture(t, "{}") + body, err := os.ReadFile(cfg) + if err != nil { + t.Fatal(err) + } + moved := strings.Replace(string(body), `"127.0.0.1:47600"`, `"`+tc.addr+`"`, 1) + if err := os.WriteFile(cfg, []byte(moved), 0o600); err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + if got := bobDoc(t, settings)[bobProxyKey]; got != tc.wantProxy { + t.Errorf("%s = %v, want %q", bobProxyKey, got, tc.wantProxy) + } + // The CA in the printed trust command comes from ca_dir, so a moved + // config must not produce a command naming the default location. + if strings.Contains(out.String(), filepath.Join(".cortex", "ca")) { + t.Errorf("output names the default CA dir, not the config's:\n%s", out.String()) + } + }) + } +} + +// mode: disabled means nothing terminates TLS, so a written proxy is a broken editor. +// Same gate as claude-code and exec. +func TestBobEnable_RefusesADisabledBridge(t *testing.T) { + settings, cfg := fixture(t, "{}") + body, err := os.ReadFile(cfg) + if err != nil { + t.Fatal(err) + } + off := strings.Replace(string(body), "mode: enabled", "mode: disabled", 1) + if err := os.WriteFile(cfg, []byte(off), 0o600); err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 1 { + t.Fatalf("exit = %d, want 1: %s", code, errb.String()) + } + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Error("a refused enable opened the file for writing") + } +} + +// VS Code permits comments in settings.json; encoding/json does not. Refuse and say +// why — do not rewrite the user's file to strip them. +func TestBobEnable_RejectsCommentedSettings(t *testing.T) { + settings, cfg := fixture(t, "{\n // my theme\n \"editor.fontSize\": 13\n}") + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 1 { + t.Fatalf("exit = %d, want 1", code) + } + if !strings.Contains(errb.String(), "comment") { + t.Errorf("the error does not name the likely cause:\n%s", errb.String()) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("the commented file was rewritten") + } +} + +// Disable removes our value and leaves anyone else's alone. The asymmetry is the +// point: enable refuses a foreign value with exit 1, disable reports it with exit 0 — +// there is nothing wrong with a machine whose Bob uses a different proxy. +func TestBobDisable_RemovesOnlyOurs(t *testing.T) { + ca := filepath.Join(t.TempDir(), "ca.crt") + + t.Run("ours is removed", func(t *testing.T) { + settings, _ := fixture(t, `{"editor.fontSize": 13, "http.proxy": "http://127.0.0.1:47600"}`) + var out, errb bytes.Buffer + if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + doc := bobDoc(t, settings) + if _, ok := doc[bobProxyKey]; ok { + t.Errorf("%s survived disable", bobProxyKey) + } + if doc["editor.fontSize"] == nil { + t.Error("an unrelated key was dropped") + } + }) + + t.Run("a foreign proxy is left in place", func(t *testing.T) { + settings, _ := fixture(t, `{"http.proxy": "http://proxy.corp.example.com:3128"}`) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + var out, errb bytes.Buffer + if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0 — a foreign proxy is not an error", code) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("disable removed a value it did not write") + } + }) + + t.Run("the key being absent is not an error", func(t *testing.T) { + settings, _ := fixture(t, `{"editor.fontSize": 13}`) + var out, errb bytes.Buffer + if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), "Not enabled") { + t.Errorf("output does not say there was nothing to do:\n%s", out.String()) + } + }) + + t.Run("a non-string value is reported, not deleted", func(t *testing.T) { + settings, _ := fixture(t, `{"http.proxy": false}`) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + var out, errb bytes.Buffer + if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("a value abctl cannot have written was deleted anyway") + } + }) +} + +// The off switch must not depend on the thing being switched off. A user who has +// uninstalled Cortex and wants Bob working again must still be able to run disable. +func TestBobDisable_WithUnreadableConfig(t *testing.T) { + settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47600"}`) + if err := os.Remove(cfg); err != nil { + t.Fatal(err) + } + // bobCAPath is the only consumer of the config in the disable path, and it is + // best-effort — this exercises the fallback branch. + ca := bobCAPath(cfg, t.TempDir()) + if ca == "" { + t.Fatal("bobCAPath returned nothing with the config gone") + } + + var out, errb bytes.Buffer + if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + if _, ok := bobDoc(t, settings)[bobProxyKey]; ok { + t.Error("disable failed with the config missing, which is when it is most needed") + } +} + +// Status reports and never acts: three states, all exit 0, nothing written. +func TestBobStatus_ThreeStates(t *testing.T) { + for _, tc := range []struct { + name, settings, want string + }{ + {"absent", `{"editor.fontSize": 13}`, "not enabled"}, + {"ours", `{"http.proxy": "http://127.0.0.1:47600"}`, "enabled in"}, + {"foreign", `{"http.proxy": "http://proxy.corp.example.com:3128"}`, "not enabled"}, + } { + t.Run(tc.name, func(t *testing.T) { + settings, cfg := fixture(t, tc.settings) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out bytes.Buffer + if code := bobStatus(settings, cfg, bobCAPath(cfg, t.TempDir()), &out); code != 0 { + t.Fatalf("exit = %d, want 0 — a report is not a verdict", code) + } + if !strings.Contains(out.String(), tc.want) { + t.Errorf("output does not contain %q:\n%s", tc.want, out.String()) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("status wrote to the settings file") + } + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Error("status left a .bak") + } + }) + } +} + +// Status must distinguish "pointing at Cortex" from "pointing at the port Cortex uses +// now" — reporting drift is useful, silently moving it is not. +func TestBobStatus_ReportsPortDrift(t *testing.T) { + settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47699"}`) + var out bytes.Buffer + if code := bobStatus(settings, cfg, bobCAPath(cfg, t.TempDir()), &out); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), "47699") || !strings.Contains(out.String(), "47600") { + t.Errorf("drift report names neither the stale value nor the current one:\n%s", out.String()) + } +} + +// A declined prompt is the user saying no, and it must cost nothing. +func TestBob_DeclinedPromptWritesNothing(t *testing.T) { + t.Run("enable", func(t *testing.T) { + settings, cfg := fixture(t, `{"editor.fontSize": 13}`) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + asked := noPrompt(t, false) + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, false, &out, &errb); code != exitDeclined { + t.Fatalf("exit = %d, want %d", code, exitDeclined) + } + if *asked != 1 { + t.Errorf("prompted %d times, want 1", *asked) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("a declined enable wrote to the file") + } + }) + + t.Run("disable", func(t *testing.T) { + settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47600"}`) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + noPrompt(t, false) + + var out, errb bytes.Buffer + if code := bobDisable(settings, bobCAPath(cfg, t.TempDir()), false, &out, &errb); code != exitDeclined { + t.Fatalf("exit = %d, want %d", code, exitDeclined) + } + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Error("a declined disable wrote to the file") + } + }) +} + +// Help goes to stdout and exits 0; everything malformed goes to stderr and exits 2. +// The verb check has to come before any filesystem work, or a misspelled verb gets a +// successful-looking answer from a later step. +func TestBob_HelpAndUsageErrors(t *testing.T) { + for _, arg := range []string{"-h", "--help", "help"} { + t.Run("help "+arg, func(t *testing.T) { + var out, errb bytes.Buffer + if code := runBob([]string{arg}, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), "Usage:") { + t.Errorf("usage did not go to stdout:\n%s", out.String()) + } + if errb.Len() != 0 { + t.Errorf("help wrote to stderr: %s", errb.String()) + } + }) + } + + t.Run("no arguments", func(t *testing.T) { + var out, errb bytes.Buffer + if code := runBob(nil, &out, &errb); code != 2 { + t.Fatalf("exit = %d, want 2", code) + } + if !strings.Contains(errb.String(), "Usage:") { + t.Errorf("usage did not go to stderr:\n%s", errb.String()) + } + }) + + t.Run("unknown verb", func(t *testing.T) { + var out, errb bytes.Buffer + if code := runBob([]string{"enabel"}, &out, &errb); code != 2 { + t.Fatalf("exit = %d, want 2", code) + } + // Naming the valid set is the difference between a refusal and a dead end. + for _, verb := range []string{"enable", "disable", "status"} { + if !strings.Contains(errb.String(), verb) { + t.Errorf("the error omits %q: %s", verb, errb.String()) + } + } + }) + + t.Run("status does not take --yes", func(t *testing.T) { + var out, errb bytes.Buffer + if code := runBob([]string{"status", "--yes"}, &out, &errb); code != 2 { + t.Errorf("exit = %d, want 2 — status writes nothing to confirm", code) + } + }) +} + +// A stray operand is usually a mistyped flag, and silently ignoring it means the run +// did something other than what was asked. +func TestBob_RejectsStrayArguments(t *testing.T) { + for _, verb := range []string{"enable", "disable", "status"} { + t.Run(verb, func(t *testing.T) { + var out, errb bytes.Buffer + if code := runBob([]string{verb, "extra"}, &out, &errb); code != 2 { + t.Errorf("exit = %d, want 2", code) + } + if !strings.Contains(errb.String(), `"extra"`) { + t.Errorf("the error does not quote the stray argument: %s", errb.String()) + } + }) + } +} + +// The printed command is the deliverable of the trust step, and it is copy-pasted by +// hand. An unquoted path with a space splits into two arguments: the settings write +// succeeds and the command under it silently does not, which is the worse of the two +// failures. +func TestBob_TrustMessageQuotesPaths(t *testing.T) { + dir := t.TempDir() + spaced := filepath.Join(dir, "Application Support", "ca.crt") + for name, got := range map[string]string{ + "trust": bobTrustNote(spaced), + "untrust": bobUntrustNote(spaced), + "verify": bobVerifyNote(spaced), + } { + if got == "" { + continue // non-darwin verify note + } + if strings.Contains(got, spaced) && !strings.Contains(got, shellQuote(spaced)) { + t.Errorf("%s note contains the bare path, unquoted:\n%s", name, got) + } + } +} + +// The request named bundle.crt; this implementation deliberately prints ca.crt. +// +// bundle.crt holds ~129 certificates and exists only for tools whose CA setting +// REPLACES the trust store. The macOS keychain is additive, so +// `add-trusted-cert -r trustRoot` on the bundle would install machine-wide explicit +// root trust for ~128 unrelated public CAs — far broader than asked, and one +// `delete-certificate -c authbridge-tls-bridge-ca` would not reverse it. This pins the +// deviation so it cannot be "corrected" back to the request's wording. +func TestBob_TrustMessageNamesCaCrtNotBundle(t *testing.T) { + if runtime.GOOS != "darwin" { + t.Skip("the keychain commands are darwin-only") + } + ca := filepath.Join(t.TempDir(), "ca.crt") + for name, got := range map[string]string{ + "trust": bobTrustNote(ca), + "untrust": bobUntrustNote(ca), + "verify": bobVerifyNote(ca), + } { + for _, line := range strings.Split(got, "\n") { + if !strings.Contains(line, "security ") { + continue // prose may mention bundle.crt to explain the choice + } + if strings.Contains(line, "bundle.crt") { + t.Errorf("%s note runs security against the 129-cert bundle:\n%s", name, line) + } + } + } + // And the undo command has to name the keychain it added to, or a System-keychain + // add is not undone by it. + if !strings.Contains(bobUntrustNote(ca), bobSystemKeychain) { + t.Errorf("the untrust command does not name the keychain:\n%s", bobUntrustNote(ca)) + } +} + +// Guessing ~/.config/IBM Bob/... is the VS Code convention but unverified for this +// app, and writing a proxy setting into a file nothing reads is a silent no-op. Refuse +// and name the flag that works instead. +func TestBob_SettingsPathNonDarwin(t *testing.T) { + got, err := bobSettingsPath("/home/someone") + if runtime.GOOS == "darwin" { + if err != nil { + t.Fatalf("darwin should know the path: %v", err) + } + if !strings.Contains(got, filepath.Join("IBM Bob", "User", "settings.json")) { + t.Errorf("path = %q", got) + } + return + } + if err == nil { + t.Fatalf("guessed a path on %s: %q", runtime.GOOS, got) + } + if !strings.Contains(err.Error(), runtime.GOOS) { + t.Errorf("the error does not name the platform it does not know: %v", err) + } +} + +// The ownership test decides what disable deletes, so its false positives cost more +// than claude-code's isCortexValue (a strings.Contains, which feeds a refusal there). +// These are the cases a substring match gets wrong. +func TestBobIsCortexProxy(t *testing.T) { + for _, tc := range []struct { + val string + want bool + }{ + {"http://127.0.0.1:47600", true}, + {"http://localhost:47600", true}, + {"http://[::1]:47600", true}, + {"http://127.0.0.1:47601", true}, // the whole 476xx block is ours + {" http://127.0.0.1:47600 ", true}, + + {"", false}, + {"http://proxy.corp.example.com:3128", false}, + {"http://127.0.0.1:3128", false}, // loopback, not our port + {"http://192.168.1.5:47600", false}, // our port, not loopback + + // A substring match says yes to both of these. That is the reason this + // predicate parses instead. + {"http://corp.example.com/?next=127.0.0.1:47600", false}, + {"http://localhost:47600.evil.example.com", false}, + + // enable only ever writes http://, so anything else is not ours to remove. + {"https://127.0.0.1:47600", false}, + {"socks5://127.0.0.1:47600", false}, + } { + if got := bobIsCortexProxy(tc.val); got != tc.want { + t.Errorf("bobIsCortexProxy(%q) = %v, want %v", tc.val, got, tc.want) + } + } +} diff --git a/cmd/abctl/cmd_configure.go b/cmd/abctl/cmd_configure.go index 77434fa17..d59f8fc76 100644 --- a/cmd/abctl/cmd_configure.go +++ b/cmd/abctl/cmd_configure.go @@ -11,6 +11,9 @@ Usage: abctl configure claude-code enable [--yes] [--settings PATH] [--config PATH] abctl configure claude-code disable [--yes] [--settings PATH] abctl configure claude-code status [--settings PATH] + abctl configure bob enable [--yes] [--settings PATH] [--config PATH] + abctl configure bob disable [--yes] [--settings PATH] [--config PATH] + abctl configure bob status [--settings PATH] [--config PATH] abctl configure bobshell enable | disable | status abctl configure codex | opencode @@ -18,8 +21,12 @@ Agents: claude-code writes the proxy and CA variables into ~/.claude/settings.json, so every session on the machine goes through Cortex. Run "abctl configure claude-code --help" for the detail. + bob writes "http.proxy" into IBM Bob's settings.json, so the Bob + editor itself goes through Cortex. Run + "abctl configure bob --help" for the detail. bobshell defines a "bob" shell function in your shell's rc file, so typing - "bob" runs it through Cortex. Run + "bob" runs it through Cortex. A different thing from "bob" above, + and the two are independent. Run "abctl configure bobshell --help" for the detail. codex not yet persistent — use "abctl exec -- codex" opencode not yet persistent — use "abctl exec -- opencode" @@ -31,11 +38,14 @@ the ones without. The agents that cannot yet be configured persistently say so a name the command that works today, rather than being absent and leaving the reader to conclude Cortex cannot drive them. -Two agents persist, by two different mechanisms: Claude Code reads a settings file, so -its configuration goes there, and Bob Shell gets a shell function written into the rc -file so the routing is applied when you type the command. Codex and OpenCode read the -process environment and nothing else, so their routing lasts exactly as long as the -process — which is what "abctl exec" is for. +Three agents persist, by two different mechanisms. Claude Code and Bob read settings +files, so their configuration goes there — the key differs (Claude Code keeps an "env" +block, Bob is a VS Code fork and reads "http.proxy"), and Bob additionally needs the +bridge CA trusted by the OS, which "configure bob enable" prints rather than performs. +Bob Shell gets a shell function written into the rc file instead, so the routing is +applied when you type the command. Codex and OpenCode read the process environment and +nothing else, so their routing lasts exactly as long as the process — which is what +"abctl exec" is for. "abctl claude-code" is the old spelling of "abctl configure claude-code". It still works, and prints a notice pointing here. @@ -101,10 +111,22 @@ func runConfigure(args []string, stdout, stderr io.Writer) int { // fire here. A user who already typed the current spelling must not be told to // type something else. return runClaudeCode(args[1:], stdout, stderr) + case "bob": + // Two distinct agents, both spelled with "bob", because there are two + // separate things to configure and they persist differently: + // + // bob IBM Bob the editor. A VS Code fork, so it has a settings.json + // and reads "http.proxy" from it. + // bobshell the shell integration — a "bob" function in the rc file, so + // typing "bob" at a prompt runs through Cortex. + // + // This arm reverses part of #1133, which removed "configure bob" reasoning + // that "IBM Bob itself needs no configuring". That was right about the + // binary and wrong about the editor: Bob has a settings file, and without + // this a Bob user had to find "http.proxy" and the CA trust step by hand. + // Configuring one does not configure the other; keep both. + return runBob(args[1:], stdout, stderr) case "bobshell": - // "bobshell", not "bob": what this configures is the Bob Shell integration - // — a function in the user's rc file — and not IBM Bob itself, which needs - // no configuring. The binary it runs is still called "bob". return runBobShell(args[1:], stdout, stderr) case "codex": fmt.Fprint(stdout, comingSoon("Codex", "codex")) @@ -116,7 +138,7 @@ func runConfigure(args []string, stdout, stderr io.Writer) int { // The named list is the answer to a typo; the usage block after it is the // answer to "what else can this do", which is what someone who guessed an // agent name wrong most likely wanted. Same pairing as the no-argument case. - fmt.Fprintf(stderr, "abctl: unknown agent %q (claude-code, bobshell, codex, opencode)\n", agent) + fmt.Fprintf(stderr, "abctl: unknown agent %q (claude-code, bob, bobshell, codex, opencode)\n", agent) fmt.Fprint(stderr, configureUsage) return 2 } diff --git a/cmd/abctl/cmd_configure_test.go b/cmd/abctl/cmd_configure_test.go index e50c73cc8..2dc883e8a 100644 --- a/cmd/abctl/cmd_configure_test.go +++ b/cmd/abctl/cmd_configure_test.go @@ -162,11 +162,16 @@ func TestConfigure_UsageErrors(t *testing.T) { } // Naming the valid set is the difference between a refusal and a dead end. // - // "bobshell" in full, not "bob": the shorter spelling is a substring of the - // longer one, so it stayed green through the rename this PR performs and - // would stay green through the next rename too. An expectation that a - // rename cannot break is not pinning the rename. - for _, agent := range []string{"claude-code", "bobshell", "codex", "opencode"} { + // "bobshell" is spelled in full because the shorter name is a substring of + // it, so an assertion on "bob" alone cannot tell the two apart. + // + // Which makes the "bob" entry below VACUOUS, and it is listed anyway only so + // the set matches the message: drop "bob" from the error text and this loop + // stays green, because "bobshell" still contains it. Do not read a passing + // run here as evidence that the message names the editor agent. What pins + // that arm is TestConfigure_BobReachesTheSameLogic and + // TestConfigure_BobAndBobShellAreDifferentAgents. + for _, agent := range []string{"claude-code", "bob", "bobshell", "codex", "opencode"} { if !strings.Contains(got, agent) { t.Errorf("stderr omits %q: %q", agent, got) } @@ -203,25 +208,59 @@ func TestConfigure_BobShellReachesTheSameLogic(t *testing.T) { } } -// The old spelling must be gone, not silently aliased. Keeping `configure bob` alive -// would preserve the name this change argues is wrong, and the stub it dispatched to -// had no behaviour anyone could depend on. -func TestConfigure_BobIsNoLongerAnAgent(t *testing.T) { - var out, errb bytes.Buffer - if code := runConfigure([]string{"bob"}, &out, &errb); code != 2 { - t.Errorf("exit = %d, want 2", code) +// `configure bob` must be a dispatch arm too, on the same terms as bobshell above. +// +// This replaces TestConfigure_BobIsNoLongerAnAgent, whose premise — that `bob` is not +// an agent — this change reverses. #1133 removed the name reasoning that IBM Bob needs +// no configuring; that was right about the binary and wrong about the editor, which is +// a VS Code fork with a settings.json. The old test is deleted rather than adapted +// because there is nothing left of what it claimed. +// +// `--settings` is not optional here: without it the default path is the real IBM Bob +// settings file on whatever machine runs the test. +func TestConfigure_BobReachesTheSameLogic(t *testing.T) { + settings := filepath.Join(t.TempDir(), "settings.json") + + var viaConfigure, configureErr bytes.Buffer + configureCode := runConfigure([]string{"bob", "status", "--settings", settings}, &viaConfigure, &configureErr) + + var direct, directErr bytes.Buffer + directCode := runBob([]string{"status", "--settings", settings}, &direct, &directErr) + + if configureCode != directCode { + t.Errorf("exit codes differ: configure = %d, bob = %d", configureCode, directCode) + } + if viaConfigure.String() != direct.String() { + t.Errorf("stdout differs:\nconfigure:\n%s\nbob:\n%s", viaConfigure.String(), direct.String()) + } + if configureErr.String() != directErr.String() { + t.Errorf("stderr differs: %q vs %q", configureErr.String(), directErr.String()) + } +} + +// Two agents, both spelled with "bob", and the shorter name is a prefix of the longer. +// So the thing worth pinning is that they are NOT aliases of each other: `bob` +// configures the editor's settings.json, `bobshell` writes a shell function, and a +// dispatch that prefix-matched would silently collapse them into one. +func TestConfigure_BobAndBobShellAreDifferentAgents(t *testing.T) { + settings := filepath.Join(t.TempDir(), "settings.json") + + var bobOut, bobErr bytes.Buffer + runConfigure([]string{"bob", "status", "--settings", settings}, &bobOut, &bobErr) + + t.Setenv(bobShellEnvVar, "1") + var shellOut, shellErr bytes.Buffer + runConfigure([]string{"bobshell", "status"}, &shellOut, &shellErr) + + if bobOut.String() == shellOut.String() { + t.Errorf("the two agents report identically, so one is aliasing the other:\n%s", bobOut.String()) } - // The error has to name the replacement, or someone with `configure bob` in a - // script has no way to find out what to type instead. - // - // Asserted against the FIRST LINE, not the whole stream. The default arm prints - // configureUsage to stderr right after the error, and that usage text names - // bobshell three times — so `Contains(errb.String(), "bobshell")` passes even - // with the agent name stripped out of the error itself, which is the one thing - // this test exists to pin. Splitting first makes the assertion able to fail. - errLine, _, _ := strings.Cut(errb.String(), "\n") - if !strings.Contains(errLine, "bobshell") { - t.Errorf("the error line does not point at the new spelling: %q", errLine) + + // The other direction of the same claim: bobshell has no --settings, so an + // aliasing dispatch would make this succeed instead of failing as a usage error. + var aliasOut, aliasErr bytes.Buffer + if code := runConfigure([]string{"bobshell", "status", "--settings", settings}, &aliasOut, &aliasErr); code != 2 { + t.Errorf("bobshell accepted bob's --settings: exit = %d, want 2", code) } } diff --git a/cmd/abctl/main.go b/cmd/abctl/main.go index 6ff2b9348..876182019 100644 --- a/cmd/abctl/main.go +++ b/cmd/abctl/main.go @@ -59,7 +59,7 @@ Usage: abctl observe open the traffic viewer (TUI) abctl service run Cortex as a service: install, uninstall, status, stop, start, restart - abctl configure point a coding agent at Cortex: claude-code, + abctl configure point a coding agent at Cortex: claude-code, bob, bobshell, codex, opencode abctl exec -- CMD [ARG...] run CMD with Cortex's proxy and CA in its environment, for tools with no settings file From 2419a9693ba2d1567996ea3951cad080b9de8263 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 14:53:23 -0400 Subject: [PATCH 2/8] Fix: Address review of abctl configure bob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four reported problems, all with tests that fail without the fix. 1. Ownership was a port-prefix match on "476", which claimed any loopback proxy in that block regardless of where Cortex actually listens. It now compares host AND port whole against listener.forward_proxy_addr from ~/.cortex/config.yaml, and reports liveness (a 300ms dial) as a separate axis — a stopped Cortex is the normal state of a laptop and is not a verdict on the setting. The predicate became three-state (bobNotOurs/bobOurs/bobUnknown) rather than a bool: an unreadable config cannot answer the question, and a bool forced a guess toward the direction that feeds a delete. bobNotOurs is the zero value so an unassigned value never authorises one. 2. The printed undo did not undo the printed change. enable prints `add-trusted-cert -d`, which writes ADMIN-domain trust settings; disable printed only `delete-certificate -t`, which per its own usage text removes the certificate and USER trust settings, leaving the admin-domain trust behind. disable now prints `remove-trusted-cert -d` (the documented inverse) followed by the delete, with the keychain named. 4. The suite proved bug 2 and passed anyway: one test asserted enable writes http://127.0.0.1:19999 while another asserted that same shape is not ours, and nothing round-tripped enable->disable off the 476xx block. The tests are rewritten for the ownership design above and now carry that round trip. Every assertion added here was mutation-verified (21 mutations, each caught by an assertion written for it). 5. enable promised a backup it did not always make. The note was one unconditional "a copy is kept as .bak", but writeSettings writes one only when the file exists and no .bak is present. Three branches, three statements that are each true. README's bob section is updated where this made it stale: the two-command undo and the ownership description. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/README.md | 65 ++++-- cmd/abctl/cmd_bob.go | 303 ++++++++++++++++++++++------ cmd/abctl/cmd_bob_test.go | 412 ++++++++++++++++++++++++++++++++++---- 3 files changed, 665 insertions(+), 115 deletions(-) diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index 8727248d5..925b61acf 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -476,30 +476,65 @@ sudo security add-trusted-cert -d -r trustRoot \ -k /Library/Keychains/System.keychain ~/.cortex/ca/ca.crt ``` -`disable` prints the matching `security delete-certificate` and says it is safe -to leave the certificate in place. `status` suggests `security verify-cert` -without running it. Off macOS these become a suggestion to add the file to the -OS trust store, naming the usual Debian and Fedora routes and saying plainly -that the exact step depends on the distribution. +`disable` prints the undo, which is **two** commands rather than one, and says +it is safe to leave the certificate in place: + +```sh +sudo security remove-trusted-cert -d ~/.cortex/ca/ca.crt +sudo security delete-certificate -c authbridge-tls-bridge-ca \ + -t /Library/Keychains/System.keychain +``` + +`delete-certificate` alone does not undo the `add-trusted-cert` above it. The add +writes trust settings to the **admin** domain (that is what its `-d` selects); +`delete-certificate -t` removes the certificate and, per its own usage text, +*user* trust settings — a different domain, so the admin-domain trust survives it. +`remove-trusted-cert -d` is the documented inverse of the add, and its `-d` has to +be repeated for the same reason. The keychain is named on the delete because an +add to the System keychain is not undone by a delete that defaults to the login +one. + +`status` suggests `security verify-cert` without running it. Off macOS these +become a suggestion to add or remove the file in the OS trust store, naming the +usual Debian and Fedora routes and saying plainly that the exact step depends on +the distribution. It is `ca.crt` — the single bridge CA — and deliberately **not** the `bundle.crt` in the same directory, which holds ~129 certificates and exists for tools whose CA setting *replaces* the trust store (`SSL_CERT_FILE` and friends, as `abctl exec` sets). The keychain is additive, so `-r trustRoot` on the bundle would install explicit machine-wide root trust for ~128 unrelated public CAs, -and one `delete-certificate` would not take it back. +and the undo above would not take it back. ### What it knows, and what it does not -"Enabled" means the key is present and its value is a loopback host on a `476xx` -port. That is a structural check on the value, not a record abctl keeps: there is -no state file, so `disable` removes the key only when it still looks like -something `enable` wrote, and reports anything else — a corporate proxy, a -non-string value — while leaving it alone. `enable` refuses rather than -overwriting a foreign value. The cost of having no state file is that a -pre-existing loopback proxy of your own on a `476xx` port is indistinguishable -from Cortex's: `disable` would remove it. The prompt names the exact value first, -and the `.bak` is already written. +"Enabled" means the value's **host and port both match** `listener.forward_proxy_addr` +from `~/.cortex/config.yaml` — the address this machine's Cortex actually listens +on, not a port range. Compared whole via `url.Parse`, so neither a path +(`http://corp.example.com/?next=127.0.0.1:47600`) nor a suffixed host +(`localhost:47600.evil.com`) can pass as ours; only `http` counts, since it is the +only scheme `enable` writes. The loopback spellings are folded together +(`localhost` / `127.0.0.1` / `::1`) because `forward_proxy_addr` may bind `0.0.0.0` +while the settings file names `127.0.0.1`, and a hand-typed address must not be +called someone else's proxy. + +That is still not a record abctl keeps — there is no state file. `disable` removes +the key only when it matches, and reports anything else — a corporate proxy, a +loopback proxy on a port the config does not name, a non-string value — while +leaving it alone. `enable` refuses rather than overwriting a foreign value. The +remaining collision is narrow: your own unrelated proxy on *exactly* the address +Cortex is configured for. The prompt names the exact value first, and the `.bak` +is already written. + +Ownership has three answers, not two: a config that cannot be read yields +**cannot tell** rather than "not ours", because a bool would have to guess, and +guessing "not ours" toward a `delete` is the dangerous direction. `status` says so +in those words instead of ruling on it. + +**Whether anything is listening is a separate question**, reported on its own +line. A stopped Cortex is the normal state of a laptop and is not a verdict on the +setting: the setting is right either way, and the answer to "nothing is listening" +is `abctl service start`, not an edit here. `http.proxy` governs VS Code's core networking and its extension host. An extension that bundles its own HTTP client can still go around it; this is the diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index a76be9b85..d1230a062 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -5,11 +5,13 @@ import ( "flag" "fmt" "io" + "net" "net/url" "os" "path/filepath" "runtime" "strings" + "time" "github.com/rossoctl/cortex/core/tlsbridge" ) @@ -182,9 +184,11 @@ func runBob(args []string, stdout, stderr io.Writer) int { // enable derives the CA path itself, from the config it already requires. return bobEnable(*settingsPath, *cortexCfgPath, yes, stdout, stderr) case "disable": - return bobDisable(*settingsPath, bobCAPath(*cortexCfgPath, home), yes, stdout, stderr) + wantProxy, caPath := bobWanted(*cortexCfgPath, home) + return bobDisable(*settingsPath, wantProxy, caPath, yes, stdout, stderr) default: - return bobStatus(*settingsPath, *cortexCfgPath, bobCAPath(*cortexCfgPath, home), stdout) + wantProxy, caPath := bobWanted(*cortexCfgPath, home) + return bobStatus(*settingsPath, *cortexCfgPath, wantProxy, caPath, stdout) } } @@ -207,8 +211,31 @@ func bobSettingsPath(home string) (string, error) { return filepath.Join(home, bobSettingsRel), nil } -// bobIsCortexProxy reports whether an http.proxy value is one abctl wrote, -// structurally: loopback host, port in Cortex's 476xx block, http scheme. +// bobOwnership is how sure abctl is that it wrote an http.proxy value. +// +// Three states rather than a bool, because the third one exists and a bool made a +// caller answer it wrongly. Comparing the settings value against the address the +// config names needs BOTH; with no readable config there is no comparison to make, +// and a bool forced that case to report "not ours" — so `disable` told a user whose +// Cortex was uninstalled that Cortex's own default address was "not a Cortex proxy" +// and left it in the file. The off switch must not be broken by the absence of the +// thing being switched off, so "cannot tell" is a state callers have to handle +// rather than a false they can fall into. +type bobOwnership int + +const ( + // bobNotOurs: parsed fine and is somebody else's — a corporate proxy, a + // different port. Never delete. + bobNotOurs bobOwnership = iota + // bobOurs: matches the address the config names, host and port. + bobOurs + // bobUnknown: the value is a loopback http proxy, but there is no config to + // compare it with. Shaped like something abctl writes and nothing contradicts + // it. disable treats this as removable; status says plainly that it is a guess. + bobUnknown +) + +// bobOwns judges an http.proxy value against the address the config names. // // Deliberately NOT isCortexValue, which is the same question asked with // strings.Contains. That form matches a substring anywhere, so @@ -219,34 +246,111 @@ func bobSettingsPath(home string) (string, error) { // silently removes someone's corporate proxy. Parsing and comparing Hostname() // and Port() whole cannot be fooled by a path or a suffixed host. // -// It also reads nothing from the config, which is what lets disable keep working -// after forward_proxy_addr moves, after wantedFromLoaded rewrites a bind address -// to "localhost", and — the case that decides it — when the config is gone -// entirely. Someone who has uninstalled Cortex and wants Bob working again must -// not find the off switch broken by the absence of the thing being switched off. +// wantProxy comes from the config rather than from a port-range heuristic. The +// heuristic this replaced tested HasPrefix(port, "476") while documenting itself as +// "Cortex's 476xx block", which is not what a prefix match does: it also accepted +// 476 and 4769999. Comparing against forward_proxy_addr needs no port convention at +// all and is right for a user who moved the port. // -// The residual false positive is an unrelated loopback proxy of the user's own -// on a 476xx port. Accepted: that is a deliberate collision with Cortex's -// documented port block, disable names the exact value in the prompt before -// removing it, and writeSettings has already saved a .bak. Exact comparison -// against the derived value still earns its place in status, which reports drift -// rather than acting on it. -func bobIsCortexProxy(val string) bool { +// With no config, the answer is bobUnknown and not bobNotOurs — see bobOwnership. +// A loopback http proxy is then removable-on-a-guess, which disable does while +// naming the value; a non-loopback one is still somebody else's. +func bobOwns(val, wantProxy string) bobOwnership { u, err := url.Parse(strings.TrimSpace(val)) if err != nil || u.Host == "" { - return false + return bobNotOurs } // http only: it is the only scheme enable ever writes, so anything else is // someone else's value. if u.Scheme != "http" { + return bobNotOurs + } + if wantProxy == "" { + // Nothing to compare against. A loopback proxy is shaped like ours and + // nothing contradicts it; anything else plainly is not. + if bobIsLoopbackProxy(val) { + return bobUnknown + } + return bobNotOurs + } + w, err := url.Parse(wantProxy) + if err != nil || w.Host == "" { + if bobIsLoopbackProxy(val) { + return bobUnknown + } + return bobNotOurs + } + // Host AND port, compared whole. Hostname() is compared case-insensitively + // because "LOCALHOST" resolves to the same place, and the loopback spellings are + // folded together because wantedFromLoaded itself produces "localhost" from an + // empty / 0.0.0.0 / :: bind address — so the config can name the same listener a + // different way than the settings file does, and a user who hand-typed + // 127.0.0.1 must not be told it is someone else's proxy. + if u.Port() == w.Port() && bobSameLoopback(u.Hostname(), w.Hostname()) { + return bobOurs + } + return bobNotOurs +} + +// bobSameLoopback reports whether two hostnames name the same listener. +// +// Exact match, or both are loopback spellings. It does NOT resolve names: a DNS +// lookup would make an ownership test depend on the network, and a resolver that +// maps some.corp.host to 127.0.0.1 would then hand a foreign proxy our ownership. +// The three literal spellings are the ones wantedFromLoaded and a hand-edited +// settings file actually produce. +func bobSameLoopback(a, b string) bool { + a, b = strings.ToLower(a), strings.ToLower(b) + if a == b { + return true + } + loopback := func(h string) bool { + switch h { + case "localhost", "127.0.0.1", "::1", "[::1]": + return true + } return false } - switch u.Hostname() { - case "localhost", "127.0.0.1", "::1": - default: + return loopback(a) && loopback(b) +} + +// bobIsLoopbackProxy reports whether val is an http proxy on this machine. +// +// Used ONLY by status, to tell "a local proxy that is not the configured one" — +// almost always a stale Cortex address from before the port moved — apart from a +// corporate proxy somewhere else. It is deliberately NOT an ownership test: it says +// nothing about who wrote the value, so disable must not consult it. +func bobIsLoopbackProxy(val string) bool { + u, err := url.Parse(strings.TrimSpace(val)) + if err != nil || u.Host == "" || u.Scheme != "http" { + return false + } + return bobSameLoopback(u.Hostname(), "localhost") && u.Port() != "" +} + +// bobProxyIsListening reports whether something is accepting connections on the +// host:port val names. Second, independent signal to the address comparison above. +// +// A dial, not a request: the question is "is this address live", and sending an +// HTTP request to someone else's proxy to find out would be a side effect status +// has no business causing. Short timeout because this runs in the path of a +// user-facing report — a firewalled address must not hang the command. +// +// Advisory ONLY, and never part of the ownership decision. Cortex being stopped is +// the normal state of a laptop, so "not listening" cannot be allowed to mean "not +// ours" — that would make disable refuse to clean up exactly when the user has +// already uninstalled the thing. It is reported, not acted on. +func bobProxyIsListening(val string) bool { + u, err := url.Parse(strings.TrimSpace(val)) + if err != nil || u.Host == "" { + return false + } + c, err := net.DialTimeout("tcp", u.Host, 300*time.Millisecond) + if err != nil { return false } - return strings.HasPrefix(u.Port(), "476") + _ = c.Close() + return true } // bobTrustNote is the trust-store step abctl does NOT take, printed for the user @@ -299,11 +403,20 @@ func bobTrustNote(caPath string) string { // optional: a CA left in the store signs nothing but Cortex's own forged leaves, // and removing it is tidiness rather than a fix. func bobUntrustNote(caPath string) string { + q := shellQuote(caPath) if runtime.GOOS == "darwin" { - // The keychain has to be named: an add to the System keychain is not undone - // by a delete that defaults to the login one. -t drops the trust settings - // add-trusted-cert created along with the certificate itself. - return "Optionally, remove the bridge CA from the keychain as well:\n\n" + + // Two commands, because the add did two things and one undo does not cover + // both. `add-trusted-cert -d` writes TRUST SETTINGS to the admin domain, and + // `remove-trusted-cert -d` is its documented inverse — the -d has to be + // repeated or it removes from the user domain, which the add never wrote to. + // `delete-certificate -t` was wrong here on its own: it deletes the + // certificate and, per its own usage text, "user trust settings", leaving the + // admin-domain trust the add created in place. The keychain must be named on + // the delete for the same reason -d is repeated on the remove: an add to the + // System keychain is not undone by a delete that defaults to the login one. + return "Optionally, undo the trust change as well — the trust settings first,\n" + + " then the certificate:\n\n" + + " sudo security remove-trusted-cert -d " + q + "\n" + " sudo security delete-certificate -c " + bobCACommonName + " \\\n" + " -t " + bobSystemKeychain + "\n\n" + " Safe to leave in place if you expect to re-enable: it only validates\n" + @@ -335,18 +448,58 @@ func bobVerifyNote(caPath string) string { " terminates, and it is trusted separately from the proxy setting above.\n" } -// bobCAPath is the bridge CA path for a message, best-effort. +// bobWanted is the best-effort form of wantedFromConfig, for the two verbs that must +// keep working when the config does not. +// +// Returns the proxy URL abctl would write and the CA path it would name, either of +// which may be "" when the config is missing, unreadable or has no forward proxy. +// enable does NOT use this — it needs a readable config and refuses without one. +// disable and status do: a user who has uninstalled Cortex and wants Bob working +// again must not find the off switch broken by the absence of the thing being +// switched off, and a status report is more useful than a parse error. +// +// The CA falls back to the default location because the trust-store note is only +// ever printed, so naming the usual path is better than naming none. The proxy does +// NOT fall back: it feeds the ownership decision, and a guessed address there would +// be a guess about which values are safe to delete. +func bobWanted(cortexCfgPath, home string) (proxy, caPath string) { + caPath = filepath.Join(home, ".cortex", "ca", "ca.crt") + want, _, err := wantedFromConfig(cortexCfgPath) + if err != nil { + return "", caPath + } + if want[envCACerts] != "" { + caPath = want[envCACerts] + } + return want[envProxy], caPath +} + +// bobBackupNote describes what this particular write will and will not preserve. // -// Best-effort because disable and status must keep working when the config does -// not: a user who has uninstalled Cortex still needs the off switch, and -// refusing to print a cleanup hint because the config that named the CA is gone -// would be a worse answer than printing the conventional path. Only enable -// requires the config, and it checks that for itself. -func bobCAPath(cortexCfgPath, home string) string { - if want, _, err := wantedFromConfig(cortexCfgPath); err == nil && want[envCACerts] != "" { - return want[envCACerts] - } - return filepath.Join(home, ".cortex", "ca", "ca.crt") +// Three different true statements, because writeSettings makes three different +// choices and the message used to claim only the first. It writes .bak from +// the file's current contents ONLY when the file exists AND no .bak is there +// already — never overwriting, because a second run would otherwise replace the +// pristine pre-Cortex file with one abctl had already edited. +// +// So "a copy is kept as .bak" was false twice over: on a settings file that +// does not exist yet there is nothing to copy, and when a .bak survives from an +// earlier run the copy kept is that older one, not this run's. Promising a backup +// that is not made is worse than promising none — it is the sentence a user leans on +// before saying yes. +func bobBackupNote(settingsPath string) string { + const unchanged = "Nothing else in the file changes" + bak := settingsPath + ".bak" + if _, err := os.Stat(settingsPath); err != nil { + // Covers a missing file and an unreadable one alike: in both cases this run + // will not produce a .bak, which is the only thing being claimed. + return unchanged + ".\n No backup is made — there is no existing file to copy.\n\n" + } + if _, err := os.Stat(bak); err == nil { + return unchanged + "; " + bak + " already exists and is\n" + + " left as it is, so it still holds the file as first found, not as it is now.\n\n" + } + return unchanged + "; a copy is kept as " + bak + "\n\n" } func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.Writer) int { @@ -392,7 +545,11 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W fmt.Fprintf(stdout, "Already enabled: %s routes IBM Bob through Cortex.\n", settingsPath) return 0 } - if !bobIsCortexProxy(existing) { + // bobNotOurs, not "!= bobOurs": enable reaches here only with a readable + // config (it exits 1 above otherwise), so bobUnknown cannot occur — but if + // that ever changes, a value abctl cannot judge should fall through to the + // write it is about to describe and prompt for, not be refused as foreign. + if bobOwns(existing, want[envProxy]) == bobNotOurs { // Refuse rather than overwrite: the overwhelmingly likely owner of a // foreign value is a corporate proxy the user needs, and this command // keeps no record that could restore it. @@ -422,7 +579,7 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W } fmt.Fprintf(stdout, "Sets in %s:\n %q: %q\n", settingsPath, bobProxyKey, proxy) - fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak\n\n", settingsPath) + fmt.Fprint(stdout, bobBackupNote(settingsPath)) if !yes && !bobConfirm(settingsPath, "Write to", stdout) { fmt.Fprintln(stdout, "Not changed.") return exitDeclined @@ -451,7 +608,7 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W return 0 } -func bobDisable(settingsPath, caPath string, yes bool, stdout, stderr io.Writer) int { +func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr io.Writer) int { doc, err := readSettings(settingsPath) if err != nil { fmt.Fprintf(stderr, "abctl: %v\n", err) @@ -464,7 +621,7 @@ func bobDisable(settingsPath, caPath string, yes bool, stdout, stderr io.Writer) return 0 } s, isString := existing.(string) - if !isString || !bobIsCortexProxy(s) { + if !isString || bobOwns(s, wantProxy) == bobNotOurs { // Exit 0, not 1. "Remove it only if it points at Cortex" is the contract, and // this file already satisfies it — there is nothing for the user to fix, so // reporting a failure would be wrong. @@ -475,7 +632,17 @@ func bobDisable(settingsPath, caPath string, yes bool, stdout, stderr io.Writer) } fmt.Fprintf(stdout, "Removes from %s:\n %q: %q\n", settingsPath, bobProxyKey, s) - fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak\n\n", settingsPath) + if bobOwns(s, wantProxy) == bobUnknown { + // Say it is a guess, because it is: with no readable config there is no + // address to compare against, and what is left is that the value is a + // loopback http proxy — the shape abctl writes. Removing it is still the + // right default (this is the uninstalled-Cortex case, when the off switch + // matters most), but the user should know which of the two answers they are + // getting, and the prompt below names the value before anything is written. + fmt.Fprintf(stdout, " (no readable config to compare against, so this is judged\n"+ + " by shape alone — a loopback proxy, which is what abctl writes)\n") + } + fmt.Fprint(stdout, bobBackupNote(settingsPath)) if !yes && !bobConfirm(settingsPath, "Write to", stdout) { fmt.Fprintln(stdout, "Not changed.") return exitDeclined @@ -493,7 +660,7 @@ func bobDisable(settingsPath, caPath string, yes bool, stdout, stderr io.Writer) return 0 } -func bobStatus(settingsPath, cortexCfgPath, caPath string, stdout io.Writer) int { +func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io.Writer) int { // Always exit 0: "not enabled" is a successful report, the same call // claudeCodeStatus and bobShellStatus make. A non-zero status here would make // `abctl configure bob status` unusable in a shell conditional for anything but @@ -510,25 +677,41 @@ func bobStatus(settingsPath, cortexCfgPath, caPath string, stdout io.Writer) int case string: fmt.Fprintf(stdout, " %q=%s\n", bobProxyKey, existing) switch { - case !bobIsCortexProxy(existing): - fmt.Fprintf(stdout, "not enabled in %s (that is not a Cortex proxy)\n", settingsPath) + case bobOwns(existing, wantProxy) == bobUnknown: + // The value cannot be judged without something to compare it against, and + // saying "not a Cortex proxy" here would be a claim about the settings + // file built on the absence of a config file. Report both facts instead. + fmt.Fprintf(stdout, "cannot tell from %s whether that is this machine's Cortex proxy\n"+ + " (no readable listener.forward_proxy_addr there; it is a loopback proxy,\n"+ + " which is the shape abctl writes, so disable would remove it)\n", cortexCfgPath) + case bobOwns(existing, wantProxy) == bobOurs: + fmt.Fprintf(stdout, "enabled in %s\n", settingsPath) + case wantProxy == "": + // Not loopback and no config: nothing here is ours, and there is still no + // config to name in the drift message below. + fmt.Fprintf(stdout, "not enabled in %s (that is not a local proxy, and %s\n"+ + " is not readable)\n", settingsPath, cortexCfgPath) + case bobIsLoopbackProxy(existing): + // Drift: a loopback proxy that is not the one the config names now. Almost + // always a Cortex address from before the port moved, which is worth + // naming as such — but it is NOT treated as ours by the ownership test, so + // disable will leave it alone and say so. Reporting drift is useful; + // deleting on a guess is not. + fmt.Fprintf(stdout, "not enabled in %s — that is a local proxy, but %s now\n"+ + " names %s. Re-run `abctl configure bob enable` to move it.\n", + settingsPath, cortexCfgPath, wantProxy) default: - // Drift is reported, never acted on. wantedFromConfig is consulted only - // here, and its failure is not this report's failure: the value is still - // a Cortex proxy whatever the config says, so an unreadable config - // downgrades the answer rather than breaking it. - want, _, werr := wantedFromConfig(cortexCfgPath) - switch { - case werr != nil: - fmt.Fprintf(stdout, "enabled in %s (could not read %s to compare: %v)\n", - settingsPath, cortexCfgPath, werr) - case want[envProxy] != existing: - fmt.Fprintf(stdout, "enabled in %s, but %s now names %s —\n"+ - " re-run `abctl configure bob enable` to move it.\n", - settingsPath, cortexCfgPath, want[envProxy]) - default: - fmt.Fprintf(stdout, "enabled in %s\n", settingsPath) - } + fmt.Fprintf(stdout, "not enabled in %s (that is not this machine's Cortex proxy,\n"+ + " which %s puts at %s)\n", settingsPath, cortexCfgPath, wantProxy) + } + // Liveness is a separate axis from ownership and is reported separately. + // Cortex being stopped is the normal state of a laptop, so this must not read + // as a verdict on the setting — the setting is correct either way, and the + // answer to "nothing is listening" is `abctl service start`, not an edit here. + if bobProxyIsListening(existing) { + fmt.Fprintf(stdout, " something is listening on %s\n", existing) + } else { + fmt.Fprintf(stdout, " nothing is listening on %s right now\n", existing) } default: fmt.Fprintf(stdout, " %q=%v\nnot enabled in %s (that value is a %T, not a string)\n", diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 2adf64db6..27e64b2b7 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "io" + "net" "os" "path/filepath" "runtime" @@ -266,12 +267,12 @@ func TestBobEnable_RejectsCommentedSettings(t *testing.T) { // point: enable refuses a foreign value with exit 1, disable reports it with exit 0 — // there is nothing wrong with a machine whose Bob uses a different proxy. func TestBobDisable_RemovesOnlyOurs(t *testing.T) { - ca := filepath.Join(t.TempDir(), "ca.crt") t.Run("ours is removed", func(t *testing.T) { - settings, _ := fixture(t, `{"editor.fontSize": 13, "http.proxy": "http://127.0.0.1:47600"}`) + settings, cfg := fixture(t, `{"editor.fontSize": 13, "http.proxy": "http://127.0.0.1:47600"}`) + want, ca := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } doc := bobDoc(t, settings) @@ -284,13 +285,14 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { }) t.Run("a foreign proxy is left in place", func(t *testing.T) { - settings, _ := fixture(t, `{"http.proxy": "http://proxy.corp.example.com:3128"}`) + settings, cfg := fixture(t, `{"http.proxy": "http://proxy.corp.example.com:3128"}`) + want, ca := bobWanted(cfg, t.TempDir()) before, err := os.ReadFile(settings) if err != nil { t.Fatal(err) } var out, errb bytes.Buffer - if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0 — a foreign proxy is not an error", code) } after, err := os.ReadFile(settings) @@ -303,9 +305,10 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { }) t.Run("the key being absent is not an error", func(t *testing.T) { - settings, _ := fixture(t, `{"editor.fontSize": 13}`) + settings, cfg := fixture(t, `{"editor.fontSize": 13}`) + want, ca := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0", code) } if !strings.Contains(out.String(), "Not enabled") { @@ -314,13 +317,14 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { }) t.Run("a non-string value is reported, not deleted", func(t *testing.T) { - settings, _ := fixture(t, `{"http.proxy": false}`) + settings, cfg := fixture(t, `{"http.proxy": false}`) + want, ca := bobWanted(cfg, t.TempDir()) before, err := os.ReadFile(settings) if err != nil { t.Fatal(err) } var out, errb bytes.Buffer - if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0", code) } after, err := os.ReadFile(settings) @@ -335,25 +339,49 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { // The off switch must not depend on the thing being switched off. A user who has // uninstalled Cortex and wants Bob working again must still be able to run disable. +// +// This is the test that caught the regression in the first version of the fix for +// the exact-match report. Judging ownership by comparing against the config is +// right when there IS a config; making that the only rule meant a missing config +// answered "not a Cortex proxy" for Cortex's own default address, and disable +// then refused to remove it. That is precisely the case where the off switch +// matters most, so bobOwns answers bobUnknown here — a loopback http proxy with +// nothing to compare against — and disable proceeds while saying what it is going +// on. +// +// Both halves are asserted, because removing the value is only half the contract: +// the user is entitled to know the judgment was made by shape rather than by +// matching their config. func TestBobDisable_WithUnreadableConfig(t *testing.T) { settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47600"}`) if err := os.Remove(cfg); err != nil { t.Fatal(err) } - // bobCAPath is the only consumer of the config in the disable path, and it is - // best-effort — this exercises the fallback branch. - ca := bobCAPath(cfg, t.TempDir()) + want, ca := bobWanted(cfg, t.TempDir()) + // The point of the fixture: with the config gone there is nothing to compare + // the value against, so ownership cannot be decided by matching. + if want != "" { + t.Fatalf("bobWanted invented a proxy address from a missing config: %q", want) + } + // The CA path is best-effort and must still produce something printable, or + // the undo command disable prints would name nothing. if ca == "" { - t.Fatal("bobCAPath returned nothing with the config gone") + t.Fatal("bobWanted returned no CA path with the config gone") + } + if got := bobOwns("http://127.0.0.1:47600", want); got != bobUnknown { + t.Errorf("bobOwns with no config = %v, want bobUnknown", got) } var out, errb bytes.Buffer - if code := bobDisable(settings, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } if _, ok := bobDoc(t, settings)[bobProxyKey]; ok { t.Error("disable failed with the config missing, which is when it is most needed") } + if !strings.Contains(out.String(), "shape alone") { + t.Errorf("disable did not disclose that it judged by shape:\n%s", out.String()) + } } // Status reports and never acts: three states, all exit 0, nothing written. @@ -373,7 +401,8 @@ func TestBobStatus_ThreeStates(t *testing.T) { } var out bytes.Buffer - if code := bobStatus(settings, cfg, bobCAPath(cfg, t.TempDir()), &out); code != 0 { + want, ca := bobWanted(cfg, t.TempDir()) + if code := bobStatus(settings, cfg, want, ca, &out); code != 0 { t.Fatalf("exit = %d, want 0 — a report is not a verdict", code) } if !strings.Contains(out.String(), tc.want) { @@ -398,7 +427,8 @@ func TestBobStatus_ThreeStates(t *testing.T) { func TestBobStatus_ReportsPortDrift(t *testing.T) { settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47699"}`) var out bytes.Buffer - if code := bobStatus(settings, cfg, bobCAPath(cfg, t.TempDir()), &out); code != 0 { + want, ca := bobWanted(cfg, t.TempDir()) + if code := bobStatus(settings, cfg, want, ca, &out); code != 0 { t.Fatalf("exit = %d, want 0", code) } if !strings.Contains(out.String(), "47699") || !strings.Contains(out.String(), "47600") { @@ -441,7 +471,8 @@ func TestBob_DeclinedPromptWritesNothing(t *testing.T) { noPrompt(t, false) var out, errb bytes.Buffer - if code := bobDisable(settings, bobCAPath(cfg, t.TempDir()), false, &out, &errb); code != exitDeclined { + want, ca := bobWanted(cfg, t.TempDir()) + if code := bobDisable(settings, want, ca, false, &out, &errb); code != exitDeclined { t.Fatalf("exit = %d, want %d", code, exitDeclined) } after, err := os.ReadFile(settings) @@ -599,34 +630,335 @@ func TestBob_SettingsPathNonDarwin(t *testing.T) { // The ownership test decides what disable deletes, so its false positives cost more // than claude-code's isCortexValue (a strings.Contains, which feeds a refusal there). -// These are the cases a substring match gets wrong. -func TestBobIsCortexProxy(t *testing.T) { +// +// This table replaces TestBobIsCortexProxy, whose predicate was a bool over a port +// PREFIX. Two things were wrong with it and one test cannot have caught both: +// +// - "476" as a prefix is not the 476xx block it documented. It also accepted +// http://127.0.0.1:476 and http://127.0.0.1:4769999, and it accepted 47600 on a +// machine whose Cortex listens on 19999 — the reported bug. +// - A bool has no room for "cannot tell". With the config gone there is nothing to +// compare against, and answering false there broke disable for the uninstalled +// case. bobUnknown is that third answer. +// +// So the cases are grouped by which of the two they pin, and every one names the +// wantProxy it is judged against — the parameter the old predicate did not have. +func TestBobOwns(t *testing.T) { + const want = "http://127.0.0.1:19999" // deliberately NOT in the 476xx block + for _, tc := range []struct { - val string - want bool + name, val, wantProxy string + owns bobOwnership }{ - {"http://127.0.0.1:47600", true}, - {"http://localhost:47600", true}, - {"http://[::1]:47600", true}, - {"http://127.0.0.1:47601", true}, // the whole 476xx block is ours - {" http://127.0.0.1:47600 ", true}, - - {"", false}, - {"http://proxy.corp.example.com:3128", false}, - {"http://127.0.0.1:3128", false}, // loopback, not our port - {"http://192.168.1.5:47600", false}, // our port, not loopback - - // A substring match says yes to both of these. That is the reason this - // predicate parses instead. - {"http://corp.example.com/?next=127.0.0.1:47600", false}, - {"http://localhost:47600.evil.example.com", false}, + // Judged against a config: equality on host and port, nothing looser. + {"exact match", "http://127.0.0.1:19999", want, bobOurs}, + {"surrounding space", " http://127.0.0.1:19999 ", want, bobOurs}, + // wantedFromLoaded itself rewrites an empty/0.0.0.0/:: host, so the two + // loopback spellings have to be one identity or enable and disable disagree. + {"localhost for 127.0.0.1", "http://localhost:19999", want, bobOurs}, + {"127.0.0.1 for localhost", "http://127.0.0.1:19999", "http://localhost:19999", bobOurs}, + {"ipv6 loopback", "http://[::1]:19999", want, bobOurs}, + + // The reported bug. Every one of these passed the old prefix test while the + // configured port was 19999, so disable would have deleted them. + {"the bug: 476 prefix", "http://127.0.0.1:47600", want, bobNotOurs}, + {"the bug: bare 476", "http://127.0.0.1:476", want, bobNotOurs}, + {"the bug: 4769999", "http://127.0.0.1:4769999", want, bobNotOurs}, + + {"empty", "", want, bobNotOurs}, + {"corporate proxy", "http://proxy.corp.example.com:3128", want, bobNotOurs}, + {"loopback, wrong port", "http://127.0.0.1:3128", want, bobNotOurs}, + {"right port, not loopback", "http://192.168.1.5:19999", want, bobNotOurs}, + + // A substring match says yes to both of these. That is why this parses. + {"port in a query string", "http://corp.example.com/?next=127.0.0.1:19999", want, bobNotOurs}, + {"suffixed host", "http://localhost:19999.evil.example.com", want, bobNotOurs}, // enable only ever writes http://, so anything else is not ours to remove. - {"https://127.0.0.1:47600", false}, - {"socks5://127.0.0.1:47600", false}, + {"https", "https://127.0.0.1:19999", want, bobNotOurs}, + {"socks5", "socks5://127.0.0.1:19999", want, bobNotOurs}, + + // No config to compare against: the third state. A loopback http proxy is + // the shape abctl writes, so it is removable; anything else is not. + {"no config, loopback", "http://127.0.0.1:47600", "", bobUnknown}, + {"no config, localhost", "http://localhost:1234", "", bobUnknown}, + {"no config, ipv6", "http://[::1]:47600", "", bobUnknown}, + {"no config, corporate", "http://proxy.corp.example.com:3128", "", bobNotOurs}, + {"no config, routable ip", "http://192.168.1.5:47600", "", bobNotOurs}, + {"no config, https", "https://127.0.0.1:47600", "", bobNotOurs}, + {"no config, empty value", "", "", bobNotOurs}, + + // An unparseable wantProxy must degrade to the no-config answer, not to a + // match: comparing against a host that failed to parse would make every + // port-less value equal to it. + {"unparseable config value", "http://127.0.0.1:47600", "::::", bobUnknown}, } { - if got := bobIsCortexProxy(tc.val); got != tc.want { - t.Errorf("bobIsCortexProxy(%q) = %v, want %v", tc.val, got, tc.want) + t.Run(tc.name, func(t *testing.T) { + if got := bobOwns(tc.val, tc.wantProxy); got != tc.owns { + t.Errorf("bobOwns(%q, %q) = %v, want %v", tc.val, tc.wantProxy, got, tc.owns) + } + }) + } +} + +// The three states must be distinct values, or a switch over them collapses two +// cases into one and every table above still passes. +func TestBobOwnershipStatesAreDistinct(t *testing.T) { + if bobNotOurs == bobOurs || bobOurs == bobUnknown || bobNotOurs == bobUnknown { + t.Fatalf("two states share a value: notOurs=%d ours=%d unknown=%d", + bobNotOurs, bobOurs, bobUnknown) + } + // bobNotOurs must be the zero value: a bobOwnership that was never assigned + // has to mean "do not touch it", never "delete it". + var zero bobOwnership + if zero != bobNotOurs { + t.Errorf("the zero bobOwnership is %d, not bobNotOurs — an unset value would authorise a delete", zero) + } +} + +// Report #4's named gap: no test round-tripped enable then disable on a port outside +// the 476xx block, which is exactly the pair the two halves of the old suite +// contradicted each other about. TestBobEnable_DerivesPortAndCAFromConfig asserted +// enable WRITES http://127.0.0.1:19999, TestBobIsCortexProxy asserted that shape is +// not ours, and nothing crossed between them — so a 50-test suite passed over a +// disable that could not undo its own enable. +// +// Written as a round trip rather than as two assertions about one port, because what +// broke was the relationship between the two commands and not either one alone. +func TestBob_EnableDisableRoundTripOnANon476xxPort(t *testing.T) { + const port = "19999" + + settings, cfg := fixture(t, `{"editor.fontSize": 13}`) + body, err := os.ReadFile(cfg) + if err != nil { + t.Fatal(err) + } + moved := strings.Replace(string(body), "127.0.0.1:47600", "127.0.0.1:"+port, 1) + if moved == string(body) { + t.Fatal("the fixture config no longer names 127.0.0.1:47600, so this test moved nothing") + } + if err := os.WriteFile(cfg, []byte(moved), 0o600); err != nil { + t.Fatal(err) + } + + var enOut, enErr bytes.Buffer + if code := bobEnable(settings, cfg, true, &enOut, &enErr); code != 0 { + t.Fatalf("enable exit %d: %s", code, enErr.String()) + } + got, _ := bobDoc(t, settings)[bobProxyKey].(string) + if want := "http://127.0.0.1:" + port; got != want { + t.Fatalf("enable wrote %q, want %q", got, want) + } + + // The half that was missing. Same config, so disable judges the value against + // the address enable derived it from. + want, ca := bobWanted(cfg, t.TempDir()) + var disOut, disErr bytes.Buffer + if code := bobDisable(settings, want, ca, true, &disOut, &disErr); code != 0 { + t.Fatalf("disable exit %d: %s", code, disErr.String()) + } + if _, ok := bobDoc(t, settings)[bobProxyKey]; ok { + t.Errorf("disable could not undo its own enable on port %s:\n%s", port, disOut.String()) + } + // The failure mode being pinned was a silent refusal, not a crash: disable + // exited 0 and said the value was not Cortex's. Assert the refusal wording is + // absent, so a regression cannot pass by keeping the exit code. + if strings.Contains(disOut.String(), "not a Cortex proxy") { + t.Errorf("disable called its own enable's value foreign:\n%s", disOut.String()) + } + // An unrelated key must survive both writes. + if bobDoc(t, settings)["editor.fontSize"] == nil { + t.Error("a sibling key did not survive the round trip") + } +} + +// bobSameLoopback is what makes localhost and 127.0.0.1 one identity, and it must do +// that WITHOUT resolving names: a DNS lookup in this path would make ownership depend +// on the resolver, and a machine whose "localhost" resolves elsewhere would get a +// different answer for the same settings file. +func TestBobSameLoopback(t *testing.T) { + for _, tc := range []struct { + a, b string + same bool + }{ + {"127.0.0.1", "127.0.0.1", true}, + {"localhost", "127.0.0.1", true}, + {"127.0.0.1", "localhost", true}, + {"::1", "localhost", true}, + {"localhost", "::1", true}, + + {"127.0.0.1", "192.168.1.5", false}, + {"localhost", "proxy.corp.example.com", false}, + // Not a loopback spelling abctl recognises, so it is only equal to itself. + {"127.0.0.2", "127.0.0.1", false}, + {"127.0.0.2", "127.0.0.2", true}, + {"", "", true}, + {"", "127.0.0.1", false}, + } { + if got := bobSameLoopback(tc.a, tc.b); got != tc.same { + t.Errorf("bobSameLoopback(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.same) } } } + +// Liveness is a separate axis from ownership, and conflating them is a bug in both +// directions: a stopped Cortex must not make its own value foreign, and a stranger's +// proxy that happens to be up must not become ours. +func TestBobProxyIsListening(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + live := "http://" + ln.Addr().String() + + if !bobProxyIsListening(live) { + t.Errorf("a listening socket reported as not listening: %s", live) + } + + // Close it and ask again: the same URL, the opposite answer. A predicate that + // ignored its argument would pass the first assertion alone. + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatal(err) + } + if bobProxyIsListening("http://" + addr) { + t.Errorf("a closed socket reported as listening: %s", addr) + } + + // Unparseable and empty values must answer false rather than panicking — this + // runs on whatever is in the user's settings file. + for _, bad := range []string{"", "::::", "not a url", "http://"} { + if bobProxyIsListening(bad) { + t.Errorf("bobProxyIsListening(%q) = true", bad) + } + } + + // Ownership must not move when liveness does. Nothing is listening on this + // port now, and the value is still ours. + if got := bobOwns("http://"+addr, "http://"+addr); got != bobOurs { + t.Errorf("a stopped proxy changed ownership: got %v, want bobOurs", got) + } +} + +// The printed undo must actually undo the printed change. This is the reported bug: +// enable prints `add-trusted-cert -d -r trustRoot`, which writes trust settings to the +// ADMIN domain, and disable printed only `delete-certificate -t`, whose own usage text +// says it removes the certificate and "user trust settings" — a different domain. So a +// user who ran both was left with admin-domain trustRoot settings for a certificate +// that no longer existed, and no printed command had removed them. +// +// Asserted as a pairing rather than as a literal, so the two messages cannot drift +// apart again: whatever flag enable uses to WRITE trust, disable must name the +// command that removes it from the same domain. +func TestBobTrustNoteAndUndoCoverTheSameDomain(t *testing.T) { + if runtime.GOOS != "darwin" { + t.Skipf("the keychain commands are darwin-only; this GOOS prints distro guidance") + } + ca := "/tmp/ca.crt" + trust, untrust := bobTrustNote(ca), bobUntrustNote(ca) + + // What enable writes. + if !strings.Contains(trust, "add-trusted-cert -d") { + t.Fatalf("enable no longer writes admin-domain trust, so this test is judging the wrong thing:\n%s", trust) + } + // The undo for exactly that, with -d repeated. Without the -d, remove-trusted-cert + // works on the user domain, which the add never touched. + if !strings.Contains(untrust, "remove-trusted-cert -d") { + t.Errorf("the undo does not remove admin-domain trust settings:\n%s", untrust) + } + // And the certificate itself, from the keychain the add named. A delete that + // defaults to the login keychain does not undo a System-keychain add. + if !strings.Contains(untrust, "delete-certificate") { + t.Errorf("the undo never deletes the certificate:\n%s", untrust) + } + if !strings.Contains(trust, bobSystemKeychain) || !strings.Contains(untrust, bobSystemKeychain) { + t.Errorf("the two messages do not name the same keychain:\nadd:\n%s\nundo:\n%s", trust, untrust) + } + // The bug's exact signature: delete-certificate -t as the ONLY trust-removing + // step. Pinned directly so a regression names itself. + if strings.Contains(untrust, "delete-certificate") && !strings.Contains(untrust, "remove-trusted-cert") { + t.Error("the undo is delete-certificate alone again, which leaves admin-domain trust behind") + } +} + +// The backup promise must match what writeSettings actually does. It writes .bak +// from the file's current contents only when the file EXISTS and no .bak is there +// already — so a single unconditional "a copy is kept as .bak" was false twice: +// once for a settings file abctl is creating, and once on a second run, where the +// existing .bak is deliberately not overwritten and therefore holds the file as first +// found rather than as it is now. +// +// Three branches, three different true statements. Each asserts its own wording AND +// the absence of the claim that would be wrong there, because a message that says +// everything says nothing. +func TestBobBackupNote_MatchesWhatWriteSettingsDoes(t *testing.T) { + t.Run("no existing file: no backup is made", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "settings.json") + got := bobBackupNote(path) + if !strings.Contains(got, "No backup is made") { + t.Errorf("does not say a backup will not be made:\n%s", got) + } + // The false promise being fixed. + if strings.Contains(got, "a copy is kept") { + t.Errorf("promises a copy of a file that does not exist:\n%s", got) + } + }) + + t.Run("file exists, no .bak yet: a copy is kept", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "settings.json") + if err := os.WriteFile(path, []byte(`{}`), 0o600); err != nil { + t.Fatal(err) + } + got := bobBackupNote(path) + if !strings.Contains(got, "a copy is kept as "+path+".bak") { + t.Errorf("does not promise the backup it will make:\n%s", got) + } + if strings.Contains(got, "No backup is made") || strings.Contains(got, "already exists") { + t.Errorf("claims a different branch's outcome:\n%s", got) + } + }) + + t.Run("a .bak already exists: it is not overwritten", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "settings.json") + if err := os.WriteFile(path, []byte(`{}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path+".bak", []byte(`{"original": true}`), 0o600); err != nil { + t.Fatal(err) + } + got := bobBackupNote(path) + if !strings.Contains(got, "already exists") { + t.Errorf("does not say the existing backup is left alone:\n%s", got) + } + // The distinction that matters to someone deciding whether to trust the + // .bak: it holds the file as FIRST found, not as it is now. + if !strings.Contains(got, "as first found") { + t.Errorf("does not say which state the existing backup holds:\n%s", got) + } + }) + + // The note is a claim about writeSettings, so check it against writeSettings + // rather than only against itself. A message and an implementation that drift + // apart is the whole bug. + t.Run("the claim matches the behaviour", func(t *testing.T) { + for _, exists := range []bool{false, true} { + path := filepath.Join(t.TempDir(), "settings.json") + if exists { + if err := os.WriteFile(path, []byte(`{"pre": 1}`), 0o600); err != nil { + t.Fatal(err) + } + } + promised := strings.Contains(bobBackupNote(path), "a copy is kept") + if err := writeSettings(path, map[string]any{"http.proxy": "http://127.0.0.1:47600"}); err != nil { + t.Fatal(err) + } + _, statErr := os.Stat(path + ".bak") + made := statErr == nil + if promised != made { + t.Errorf("file existed = %v: note promised a copy = %v, writeSettings made one = %v", + exists, promised, made) + } + } + }) +} From 1bdc902c8cb1e0ed0887156871f29aab18c3457b Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 15:50:46 -0400 Subject: [PATCH 3/8] Fix: Splice bob's settings key and lead status with a verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on `abctl configure bob`. Enable/disable no longer round-trip the settings file through writeSettings. json.MarshalIndent over a map[string]any sorts keys, normalizes indentation and explodes inline arrays, so adding one key alphabetized and reflowed the whole document — contradicting the "Nothing else in the file changes" line printed directly above the write. bobWriteKey splices a single member in or out of the original bytes instead, driven by json.Decoder offsets rather than a regex, and re-validates the result as JSON before the atomic rename. Backup and permission behaviour are unchanged. The existing preservation test could not have caught that: it compared through readSettings, which re-parses. Added byte-level tests on a non-alphabetized 4-space fixture — one line added and none removed on enable, byte-for-byte equality on an enable/disable round trip, no stray whitespace or doubled blank line after a removal, and the key name appearing inside a value or a nested object is not mistaken for the member. Status now leads with the verdict — "IBM Bob is configured to use the Cortex proxy" or the *NOT* spelling — with any liveness WARNING on the second line and the detail indented under both. It previously opened with the raw "http.proxy"= key and left the reader to derive the answer. The "Not run here" paragraph is gone. Liveness is probed only for a value this command claims or cannot judge, so a foreign corporate proxy is no longer reported as down. Signed-off-by: Ed Snible Assisted-By: Claude (Anthropic AI) --- cmd/abctl/README.md | 8 + cmd/abctl/cmd_bob.go | 436 ++++++++++++++++++++++++++++++++++---- cmd/abctl/cmd_bob_test.go | 417 +++++++++++++++++++++++++++++++++++- 3 files changed, 814 insertions(+), 47 deletions(-) diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index 925b61acf..dc541fe11 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -450,6 +450,14 @@ nested `"env"` block. The address is read from `listener.forward_proxy_addr` in `~/.cortex/config.yaml` on every run, so a moved port or an IPv6 loopback produces the right value rather than a hardcoded 47600. +Nothing else in the file changes, and that is meant literally: the key is spliced +into the existing bytes rather than re-serialized from a parsed document, so your +key order, your indent width, your inline arrays and your blank lines between +groups all survive untouched. A settings.json is hand-curated and often lives in +a dotfiles repo, where a diff that alphabetizes and reflows the whole file is +worse than the setting is worth. `disable` takes the line back out the same way, +so enable-then-disable returns the file byte-for-byte. + ```sh abctl configure bob enable # write the key, print the CA trust command abctl configure bob disable # remove the key, print the optional undo diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index d1230a062..5ef02263a 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -1,6 +1,8 @@ package main import ( + "bytes" + "encoding/json" "errors" "flag" "fmt" @@ -10,6 +12,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "time" @@ -443,9 +446,9 @@ func bobVerifyNote(caPath string) string { } return "To check that this machine trusts the bridge CA:\n\n" + " security verify-cert -c " + shellQuote(caPath) + "\n\n" + - " Not run here — status reports, it does not act. A failure does not by itself\n" + - " mean Bob is misconfigured: the CA only matters for hosts the bridge\n" + - " terminates, and it is trusted separately from the proxy setting above.\n" + " A failure does not by itself mean Bob is misconfigured: the CA only matters\n" + + " for hosts the bridge terminates, and it is trusted separately from the\n" + + " proxy setting above.\n" } // bobWanted is the best-effort form of wantedFromConfig, for the two verbs that must @@ -487,7 +490,307 @@ func bobWanted(cortexCfgPath, home string) (proxy, caPath string) { // earlier run the copy kept is that older one, not this run's. Promising a backup // that is not made is worse than promising none — it is the sentence a user leans on // before saying yes. +// bobSetKey writes one top-level key into a settings file, changing nothing else +// in it — byte for byte. It is the reason bob does not call writeSettings. +// +// writeSettings round-trips through json.MarshalIndent over a map[string]any, and +// that is lossy in ways JSON does not consider meaningful but a person reading a +// diff does: Go sorts map keys, so a hand-grouped file is alphabetized; indentation +// is normalized to two spaces; and an inline array like [80, 120] is exploded onto +// one line per element. On a real settings file, adding one key moved +// workbench.colorTheme from first to last and rewrote ten lines. These files are +// hand-curated and frequently committed to a dotfiles repo, so a semantically +// equal but textually large diff is a real cost — and it contradicted the +// "Nothing else in the file changes" line printed directly above the write. +// +// So the edit is textual and surgical, and the parser drives it rather than a +// regex: json.Decoder reports byte offsets, which is what makes it safe to splice +// a document this way. A key whose name appears inside some other string value +// cannot be mistaken for the real member, because the offsets come from the +// tokenizer, not from a search. +// +// value == nil means delete the key. Returns the new file content. +func bobSetKey(src []byte, key string, value any) ([]byte, error) { + start, end, found, err := bobFindMember(src, key) + if err != nil { + return nil, err + } + + if value == nil { + if !found { + return src, nil + } + return bobSpliceOut(src, start, end), nil + } + + // Encoded the same way either way, so a replaced value and an inserted one are + // formatted identically. + vb, err := json.Marshal(value) + if err != nil { + return nil, err + } + member := append([]byte(strconv.Quote(key)+": "), vb...) + + if found { + out := make([]byte, 0, len(src)+len(member)) + out = append(out, src[:start]...) + out = append(out, member...) + out = append(out, src[end:]...) + return out, nil + } + return bobInsertMember(src, member) +} + +// bobFindMember locates the byte span of a top-level member, from the opening quote +// of its name through the last byte of its value. Offsets come from json.Decoder, +// so they are the tokenizer's view of the document and not a textual guess. +func bobFindMember(src []byte, key string) (start, end int, found bool, err error) { + dec := json.NewDecoder(bytes.NewReader(src)) + tok, err := dec.Token() + if err != nil { + return 0, 0, false, err + } + if d, ok := tok.(json.Delim); !ok || d != '{' { + return 0, 0, false, fmt.Errorf("top level is not a JSON object") + } + for dec.More() { + // InputOffset before reading the name is the offset just past the previous + // token, so skip whitespace forward to the quote that opens this name. + nameStart := int(dec.InputOffset()) + for nameStart < len(src) && src[nameStart] != '"' { + nameStart++ + } + name, err := dec.Token() + if err != nil { + return 0, 0, false, err + } + // Reading the value advances the offset to just past it, which is the end of + // the whole member. Decoding into json.RawMessage consumes a value of any + // shape — object, array, scalar — in one step. + var raw json.RawMessage + if err := dec.Decode(&raw); err != nil { + return 0, 0, false, err + } + if name == key { + return nameStart, int(dec.InputOffset()), true, nil + } + } + return 0, 0, false, nil +} + +// bobSpliceOut removes a member and exactly one of the commas around it, leaving the +// surrounding layout intact. +// +// The subtlety is which whitespace goes with the member. Taking only the member's own +// bytes leaves its indentation behind as a line of trailing spaces — invisible in a +// terminal, visible in a diff and to every whitespace linter. So the span removed runs +// from the start of the member's own LINE (its leading indentation) to just past the +// newline that ends it. A blank line the user wrote as a grouping separator sits before +// that indentation and is therefore kept. +// +// The comma is the other half: it goes on whichever side has one, preferring the +// FOLLOWING comma so that removing the last member does not leave a trailing comma, +// which is invalid JSON. +func bobSpliceOut(src []byte, start, end int) []byte { + // Is the member alone on its line? Only then can the line be taken whole. + lineStart := bytes.LastIndexByte(src[:start], '\n') + 1 + ownLine := len(bytes.TrimSpace(src[lineStart:start])) == 0 + + // Walk forward past whitespace to a following comma, if there is one. + after := end + for after < len(src) && isBobSpace(src[after]) { + after++ + } + + from, to := start, end + if after < len(src) && src[after] == ',' { + // Not the last member: take the member, the comma, and the rest of its line + // including the newline, so the next member keeps its own indentation. + to = after + 1 + if nl := bytes.IndexByte(src[to:], '\n'); nl >= 0 && len(bytes.TrimSpace(src[to:to+nl])) == 0 { + to += nl + 1 + } else if !ownLine { + // A single-line document: `{"a": 1, "http.proxy": "...", "b": 2}`. There is + // no line to take, and the space that separated this member from the next + // one would be left beside the space after the previous comma, making a + // double space. Take one of them. + for to < len(src) && src[to] == ' ' { + to++ + } + } + if ownLine { + from = lineStart + // A member sitting BETWEEN two separators leaves both behind once it is + // gone, and two blank lines where the user wrote one is a change to the + // file's layout — the thing this function exists not to make. So when the + // bytes on each side of the removed line are both separators of the same + // kind, one of them goes with it. Same reasoning one line down for the + // single-line-document form, where the separator is a space rather than a + // newline and removing a middle member would leave a double space. + from = bobAbsorbSeparator(src, from, to) + } + } else { + // Last member: take the preceding comma and the whitespace between it and us, + // so the member that becomes last does not end with a dangling comma. + for from > 0 && isBobSpace(src[from-1]) { + from-- + } + if from > 0 && src[from-1] == ',' { + from-- + } + } + + out := make([]byte, 0, len(src)-(to-from)) + out = append(out, src[:from]...) + out = append(out, src[to:]...) + return out +} + +// bobInsertMember adds a member after the last existing one, matching that member's +// own indentation so the insertion looks hand-written rather than appended. +func bobInsertMember(src []byte, member []byte) ([]byte, error) { + // The closing brace of the top-level object is the last '}' in the document. + closing := bytes.LastIndexByte(src, '}') + if closing < 0 { + return nil, fmt.Errorf("top level is not a JSON object") + } + + // Is the object empty? Then there is no last member to follow. + trimmed := bytes.TrimSpace(src[:closing]) + empty := bytes.HasSuffix(trimmed, []byte("{")) + + // End of the last member, which is where the comma and the new line go. + insertAt := closing + for insertAt > 0 && isBobSpace(src[insertAt-1]) { + insertAt-- + } + + // The indentation to use: the LAST MEMBER's own, not the closing brace's. Copying + // the brace's line is the obvious-looking choice and it is wrong — in a + // conventionally formatted file that line is column 0, so every inserted key + // landed unindented while every existing one was indented. Read it from where the + // last member starts instead, which is the line the new member will sit beside. + indent := bobIndentOf(src, insertAt) + if indent == "" { + // Either an empty object, or a single-line document with no indentation to + // copy. Two spaces is the only width this function ever invents, and only + // when the file itself shows none. + indent = " " + } + + var ins []byte + if empty { + ins = append(ins, '\n') + ins = append(ins, indent...) + ins = append(ins, member...) + ins = append(ins, '\n') + } else { + ins = append(ins, ',', '\n') + ins = append(ins, indent...) + ins = append(ins, member...) + } + + out := make([]byte, 0, len(src)+len(ins)) + out = append(out, src[:insertAt]...) + out = append(out, ins...) + out = append(out, src[insertAt:]...) + return out, nil +} + +// bobIndentOf returns the leading whitespace of the line containing off. +func bobIndentOf(src []byte, off int) string { + lineStart := bytes.LastIndexByte(src[:off], '\n') + 1 + i := lineStart + for i < off && (src[i] == ' ' || src[i] == '\t') { + i++ + } + return string(src[lineStart:i]) +} + +// bobAbsorbSeparator extends a removal backward over one blank line when the removed +// span has a blank line on both sides, so a grouping separator is not duplicated. +func bobAbsorbSeparator(src []byte, from, to int) int { + // Is what follows the removed span a blank line? + nl := bytes.IndexByte(src[to:], '\n') + if nl < 0 || len(bytes.TrimSpace(src[to:to+nl])) != 0 { + return from + } + // Is what precedes it also one? Walk back over the previous line. + prevStart := bytes.LastIndexByte(src[:from-1], '\n') + 1 + if from == 0 || src[from-1] != '\n' || len(bytes.TrimSpace(src[prevStart:from-1])) != 0 { + return from + } + return prevStart +} + +func isBobSpace(c byte) bool { + return c == ' ' || c == '\t' || c == '\n' || c == '\r' +} + +// bobWriteKey is bob's replacement for writeSettings: same backup and atomic-rename +// behaviour, but the content is the original file with one key edited rather than a +// re-marshal of the whole document. value == nil deletes the key. +func bobWriteKey(path, key string, value any) error { + src, rerr := os.ReadFile(path) //nolint:gosec // operator-supplied path + if rerr != nil { + if !os.IsNotExist(rerr) { + return rerr + } + // No file yet: start from an empty object so the insert path has a document + // to work on. Nothing to back up either. + src = []byte("{}\n") + } else { + // Back the file up ONCE and never overwrite it, matching writeSettings: a + // second enable, or an enable/disable pair, must not replace the pristine + // pre-Cortex file with one abctl already edited. + bak := path + ".bak" + if _, serr := os.Stat(bak); os.IsNotExist(serr) { + if werr := os.WriteFile(bak, src, 0o600); werr != nil { + return fmt.Errorf("writing backup %s: %w", bak, werr) + } + } + } + + // An empty or whitespace-only file is an empty document, the same reading + // readSettings gives it. + if len(bytes.TrimSpace(src)) == 0 { + src = []byte("{}\n") + } + + out, err := bobSetKey(src, key, value) + if err != nil { + return fmt.Errorf("%s: %w", path, err) + } + + // Never hand back something that is not valid JSON, whatever the splicing did. + // Bob reads this file; a malformed one is worse than a reformatted one. + var check map[string]any + if jerr := json.Unmarshal(out, &check); jerr != nil { + return fmt.Errorf("%s: edit would produce invalid JSON (%w); file left unchanged", path, jerr) + } + + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + tmp := path + ".tmp" + // 0600: this file commonly holds API tokens in the same block. + if err := os.WriteFile(tmp, out, 0o600); err != nil { + return err + } + return os.Rename(tmp, path) +} + func bobBackupNote(settingsPath string) string { + // This sentence is a literal claim, and bobWriteKey is what makes it one: it splices + // a single member in or out of the existing bytes, so key order, indentation, inline + // arrays and blank-line grouping all survive. It was NOT true of the first version + // of this command, which re-marshalled the parsed document and so silently + // alphabetized and reformatted the whole file — see bobWriteKey, and + // TestBobEnable_ChangesOneLineAndNoOtherByte, which pins it byte-for-byte. + // + // The same sentence appears in claude-code's enable, where it still goes through the + // shared writeSettings and therefore still overstates what happens. Not changed here: + // that is a different command's behaviour and out of this change's scope. const unchanged = "Nothing else in the file changes" bak := settingsPath + ".bak" if _, err := os.Stat(settingsPath); err != nil { @@ -558,7 +861,17 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W " instead.\n", settingsPath, bobProxyKey, existing) return 1 } - // Ours but stale — the proxy moved. Falls through to the write. + // Reaching here means bobOwns said bobOurs while the value differs from the + // one about to be written — which, now that ownership is an exact host+port + // match, only spellings of the same listener can do: "localhost" against + // "127.0.0.1", or a differing case. Falls through to the write, which + // normalizes it to whatever the config derives. The plan's "ours but stale, + // the port moved" case no longer lands here: a differing port is not ours by + // that definition, so it is refused as foreign above. That is the safe + // direction — enable never overwrites a value it cannot positively claim — + // but it does mean a user whose forward_proxy_addr moved must remove the old + // value by hand. Noted rather than changed: widening ownership back to a port + // range is what the previous review had this command stop doing. default: // A non-string (a number, an object) is not something this command wrote and // not something it can compare. Same refusal as a foreign string. @@ -585,8 +898,7 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W return exitDeclined } - doc[bobProxyKey] = proxy - if werr := writeSettings(settingsPath, doc); werr != nil { + if werr := bobWriteKey(settingsPath, bobProxyKey, proxy); werr != nil { fmt.Fprintf(stderr, "abctl: %v\n", werr) return 1 } @@ -648,8 +960,10 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr return exitDeclined } - delete(doc, bobProxyKey) - if werr := writeSettings(settingsPath, doc); werr != nil { + // nil value means delete. bobWriteKey, not writeSettings: the promise printed + // above is that nothing else in the file changes, and a re-marshal would + // reorder and reindent every other key. + if werr := bobWriteKey(settingsPath, bobProxyKey, nil); werr != nil { fmt.Fprintf(stderr, "abctl: %v\n", werr) return 1 } @@ -660,62 +974,100 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr return 0 } +// The two verdicts status can reach, and the first line of every report it writes. +// +// Package-level so the tests assert the same strings the code prints rather than a +// copy: a wording change that updates only one of the two would otherwise pass. +// +// Note that bobStatusNo CONTAINS the prefix of bobStatusYes up to "is", so neither may +// be checked with a prefix or substring test that could accept the other — see +// TestBobStatus_ThreeStates. +const ( + bobStatusYes = "IBM Bob is configured to use the Cortex proxy" + bobStatusNo = "IBM Bob is *NOT* configured to use the Cortex proxy" +) + func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io.Writer) int { - // Always exit 0: "not enabled" is a successful report, the same call + // The first line answers the question someone ran this to ask, in the words they + // would use to ask it: is IBM Bob going through Cortex or not. Everything else is + // detail under that, and the order is deliberate — this used to open with the raw + // `"http.proxy"=...` key and make the reader derive the verdict from it. + // + // Always exit 0: "not configured" is a successful report, the same call // claudeCodeStatus and bobShellStatus make. A non-zero status here would make // `abctl configure bob status` unusable in a shell conditional for anything but // "is it on". doc, err := readSettings(settingsPath) if err != nil { - fmt.Fprintf(stdout, "not enabled (%v)\n", err) + // An unreadable or non-JSON settings file cannot be configured, so the verdict + // is the same "not" — with the reason, which is the actionable part. + fmt.Fprintf(stdout, "%s\n %v\n", bobStatusNo, err) return 0 } + // Detail lines are collected rather than printed inline, so the verdict and any + // WARNING can go first regardless of which branch produced the detail. + var detail []string + add := func(format string, args ...any) { detail = append(detail, fmt.Sprintf(format, args...)) } + + // Only a value this command recognises as the Cortex proxy is probed for liveness. + // Probing a foreign one warns that someone's corporate proxy is down, which is + // neither true (it is reachable from somewhere, just not here) nor any of Cortex's + // business — and it reads as a complaint about a setting abctl deliberately leaves + // alone. An unjudgeable loopback value IS probed: it is the shape abctl writes, and + // disable would act on it, so its liveness is informative. + verdict, listening := bobStatusNo, "" switch existing := doc[bobProxyKey].(type) { case nil: - fmt.Fprintf(stdout, " %q (unset)\nnot enabled in %s\n", bobProxyKey, settingsPath) + add("%q is unset in %s", bobProxyKey, settingsPath) case string: - fmt.Fprintf(stdout, " %q=%s\n", bobProxyKey, existing) switch { + case bobOwns(existing, wantProxy) == bobOurs: + verdict = bobStatusYes + listening = existing + add("%q=%s in %s", bobProxyKey, existing, settingsPath) case bobOwns(existing, wantProxy) == bobUnknown: + listening = existing // The value cannot be judged without something to compare it against, and - // saying "not a Cortex proxy" here would be a claim about the settings - // file built on the absence of a config file. Report both facts instead. - fmt.Fprintf(stdout, "cannot tell from %s whether that is this machine's Cortex proxy\n"+ - " (no readable listener.forward_proxy_addr there; it is a loopback proxy,\n"+ - " which is the shape abctl writes, so disable would remove it)\n", cortexCfgPath) - case bobOwns(existing, wantProxy) == bobOurs: - fmt.Fprintf(stdout, "enabled in %s\n", settingsPath) + // claiming "not a Cortex proxy" here would be a statement about the + // settings file resting on the absence of a config file. Report both. + add("%q=%s in %s", bobProxyKey, existing, settingsPath) + add("that is a loopback proxy, which is the shape abctl writes, but %s is not", + cortexCfgPath) + add("readable — so whether it is this machine's Cortex proxy cannot be told from here") case wantProxy == "": - // Not loopback and no config: nothing here is ours, and there is still no - // config to name in the drift message below. - fmt.Fprintf(stdout, "not enabled in %s (that is not a local proxy, and %s\n"+ - " is not readable)\n", settingsPath, cortexCfgPath) + add("%q=%s in %s", bobProxyKey, existing, settingsPath) + add("that is not a local proxy, and %s is not readable", cortexCfgPath) case bobIsLoopbackProxy(existing): // Drift: a loopback proxy that is not the one the config names now. Almost // always a Cortex address from before the port moved, which is worth - // naming as such — but it is NOT treated as ours by the ownership test, so - // disable will leave it alone and say so. Reporting drift is useful; - // deleting on a guess is not. - fmt.Fprintf(stdout, "not enabled in %s — that is a local proxy, but %s now\n"+ - " names %s. Re-run `abctl configure bob enable` to move it.\n", - settingsPath, cortexCfgPath, wantProxy) + // naming as such — but it is NOT ours by the ownership test, so disable + // leaves it alone. Reporting drift is useful; deleting on a guess is not. + add("%q=%s in %s", bobProxyKey, existing, settingsPath) + add("that is a local proxy, but %s now names %s", cortexCfgPath, wantProxy) + add("run `abctl configure bob enable` to move it") default: - fmt.Fprintf(stdout, "not enabled in %s (that is not this machine's Cortex proxy,\n"+ - " which %s puts at %s)\n", settingsPath, cortexCfgPath, wantProxy) - } - // Liveness is a separate axis from ownership and is reported separately. - // Cortex being stopped is the normal state of a laptop, so this must not read - // as a verdict on the setting — the setting is correct either way, and the - // answer to "nothing is listening" is `abctl service start`, not an edit here. - if bobProxyIsListening(existing) { - fmt.Fprintf(stdout, " something is listening on %s\n", existing) - } else { - fmt.Fprintf(stdout, " nothing is listening on %s right now\n", existing) + add("%q=%s in %s", bobProxyKey, existing, settingsPath) + add("that is not this machine's Cortex proxy, which %s puts at %s", + cortexCfgPath, wantProxy) } default: - fmt.Fprintf(stdout, " %q=%v\nnot enabled in %s (that value is a %T, not a string)\n", - bobProxyKey, existing, settingsPath, existing) + add("%q=%v in %s", bobProxyKey, existing, settingsPath) + add("that value is a %T, not a string", existing) + } + + fmt.Fprintln(stdout, verdict) + + // Second line, when it applies. Liveness is a separate axis from ownership, and + // this is a WARNING rather than part of the verdict because the setting is correct + // either way: Cortex being stopped is the normal state of a laptop, and the answer + // is `abctl service start`, not an edit to Bob's settings. + if listening != "" && !bobProxyIsListening(listening) { + fmt.Fprintf(stdout, "WARNING: No proxy is listening at %s\n", listening) + } + + for _, line := range detail { + fmt.Fprintf(stdout, " %s\n", line) } if note := bobVerifyNote(caPath); note != "" { diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 27e64b2b7..818980eea 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -80,6 +80,273 @@ func TestBobEnable_WritesHTTPProxy(t *testing.T) { } // The property that matters most: this is the user's own editor configuration. +// bobHandFormatted is deliberately NOT what json.MarshalIndent would emit, in every +// way that matters: +// +// - keys are NOT in alphabetical order (workbench first, editor before files) +// - indentation is FOUR spaces, not two +// - blank lines group related settings +// - "editor.rulers" is an inline array on one line +// +// Every one of those is something a re-marshal destroys, and the fixture the other +// tests use cannot see it: bobSettings is two-space and close enough to sorted that a +// reformat is nearly invisible in it. A settings.json is hand-curated and frequently +// committed to a dotfiles repo, so this shape is the realistic one. +const bobHandFormatted = `{ + "workbench.colorTheme": "Default Dark Modern", + + "editor.rulers": [80, 120], + "editor.fontSize": 13, + "editor.tabSize": 4, + + "files.autoSave": "onFocusChange", + "terminal.integrated.fontSize": 12 +} +` + +// The file must change by exactly the one line that adds the key — asserted on BYTES, +// not through a parse. +// +// This is the test TestBobEnable_PreservesEverythingElse cannot be. That one compares +// through bobDoc, which json.Unmarshals into a map[string]any, and a map has no key +// order and carries no whitespace — so every assertion it makes is blind to the entire +// class of damage a re-marshal does. It passed the whole time enable was alphabetizing +// the file, reindenting it from four spaces to two, and exploding inline arrays onto +// one line per element: on a real settings file that rewrote ten lines to add one key. +// +// So the assertion here is a line diff of the before and after bytes. Any reordering, +// any reindentation, any array reflow shows up as extra changed lines and fails. +func TestBobEnable_ChangesOneLineAndNoOtherByte(t *testing.T) { + settings, cfg := fixture(t, bobHandFormatted) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + added, removed := lineDiff(string(before), string(after)) + got := string(after) + + // Adding a key to the last member's line also puts a comma on it, so the write + // legitimately touches two lines: the new one, and the previous last one gaining + // its comma. Nothing beyond that is this command's business. + if len(added) != 2 || len(removed) != 1 { + t.Errorf("want 1 line replaced by 2 (the comma and the new key), got -%d/+%d:\n--- removed\n%s\n+++ added\n%s", + len(removed), len(added), strings.Join(removed, "\n"), strings.Join(added, "\n")) + } + for _, line := range added { + if !strings.Contains(line, bobProxyKey) && !strings.Contains(line, "terminal.integrated.fontSize") { + t.Errorf("an unrelated line was rewritten: %q", line) + } + } + + // The new line must be indented like its siblings, which is a separate claim from + // "only one line was added" — an unindented new key is still exactly one new line, + // so the count above cannot see it. It is worth its own assertion because it is a + // mistake already made here once: reading the indentation from the line holding + // the closing brace looks right and is wrong, because in a conventionally + // formatted file that line is column 0. + if !strings.Contains(got, "\n \""+bobProxyKey+"\": ") { + t.Errorf("the inserted key is not indented like its siblings:\n%s", got) + } + + // The specific casualties of a re-marshal, each named so a regression reports + // which property it broke rather than just "bytes differ". + if !strings.Contains(got, `"editor.rulers": [80, 120]`) { + t.Errorf("the inline array was reflowed:\n%s", got) + } + if !strings.Contains(got, "\n \"editor.fontSize\": 13,") { + t.Errorf("four-space indentation was not preserved:\n%s", got) + } + if strings.Index(got, "workbench.colorTheme") > strings.Index(got, "editor.fontSize") { + t.Errorf("keys were alphabetized — workbench must still come first:\n%s", got) + } + if !strings.Contains(got, "\"Default Dark Modern\",\n\n \"editor.rulers\"") { + t.Errorf("a blank-line grouping separator was lost:\n%s", got) + } +} + +// enable then disable must return the file to the exact bytes it started with. +// +// A round trip through a re-marshal is stable — it reformats once and then agrees with +// itself — so this cannot be checked with the parsing helpers either. Byte equality is +// the whole claim: a user who enables and changes their mind gets their file back, not +// a reformatted equivalent of it. +func TestBobEnableDisable_RoundTripsByteForByte(t *testing.T) { + settings, cfg := fixture(t, bobHandFormatted) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("enable: exit %d: %s", code, errb.String()) + } + proxy, caPath := bobWanted(cfg, t.TempDir()) + if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + t.Fatalf("disable: exit %d: %s", code, errb.String()) + } + + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if string(before) != string(after) { + t.Errorf("round trip did not restore the file byte for byte:\n--- before\n%s\n+++ after\n%s", before, after) + } +} + +// Removing a key must not leave whitespace nobody wrote, whichever position it held. +// +// Each of these is a distinct splice: a middle member takes its own line and one comma, +// the LAST member has no following comma so it must take the PRECEDING one (a trailing +// comma is invalid JSON), the only member leaves an empty object, and a member between +// two blank-line separators must take one of them or the file gains a blank line. +// +// The assertions are on bytes and on re-parseability, because "valid JSON" and "no +// stray whitespace" are different claims and the early implementations of this splicer +// satisfied the first while failing the second. +func TestBobDisable_LeavesNoStrayWhitespace(t *testing.T) { + for _, tc := range []struct{ name, in, want string }{ + { + name: "middle member", + in: "{\n \"a\": 1,\n \"http.proxy\": \"http://127.0.0.1:47600\",\n \"b\": 2\n}\n", + want: "{\n \"a\": 1,\n \"b\": 2\n}\n", + }, + { + name: "last member takes the preceding comma", + in: "{\n \"a\": 1,\n \"http.proxy\": \"http://127.0.0.1:47600\"\n}\n", + want: "{\n \"a\": 1\n}\n", + }, + { + name: "only member", + in: "{\n \"http.proxy\": \"http://127.0.0.1:47600\"\n}\n", + want: "{\n}\n", + }, + { + name: "between two grouping separators keeps exactly one", + in: "{\n \"a\": 1,\n\n \"http.proxy\": \"http://127.0.0.1:47600\",\n\n \"b\": 2\n}\n", + want: "{\n \"a\": 1,\n\n \"b\": 2\n}\n", + }, + { + name: "single line document", + in: "{\"a\": 1, \"http.proxy\": \"http://127.0.0.1:47600\", \"b\": 2}\n", + want: "{\"a\": 1, \"b\": 2}\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + settings, cfg := fixture(t, tc.in) + proxy, caPath := bobWanted(cfg, t.TempDir()) + + var out, errb bytes.Buffer + if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + + got, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if string(got) != tc.want { + t.Errorf("bytes differ:\n got %q\nwant %q", got, tc.want) + } + // Belt and braces: a splice that produced the right bytes by accident but + // broke the document would be caught here too. + var doc map[string]any + if jerr := json.Unmarshal(got, &doc); jerr != nil { + t.Errorf("result is not valid JSON: %v\n%s", jerr, got) + } + if _, still := doc[bobProxyKey]; still { + t.Errorf("the key survived:\n%s", got) + } + }) + } +} + +// The offsets driving the splice come from json.Decoder, not from a text search, and +// this is the case that tells the two apart: the key's own name appears inside another +// member's string value, and as a nested object's key. A regex or strings.Index +// implementation edits the wrong one; a tokenizer cannot. +func TestBobDisable_IgnoresTheKeyNameInsideValuesAndNesting(t *testing.T) { + in := "{\n" + + " \"some.note\": \"set \\\"http.proxy\\\": \\\"http://127.0.0.1:47600\\\" to use Cortex\",\n" + + " \"nested\": {\"http.proxy\": \"http://inner.example:1\"},\n" + + " \"http.proxy\": \"http://127.0.0.1:47600\",\n" + + " \"z\": 1\n}\n" + + settings, cfg := fixture(t, in) + proxy, caPath := bobWanted(cfg, t.TempDir()) + + var out, errb bytes.Buffer + if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + + got, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + var doc map[string]any + if jerr := json.Unmarshal(got, &doc); jerr != nil { + t.Fatalf("result is not valid JSON: %v\n%s", jerr, got) + } + + if _, still := doc[bobProxyKey]; still { + t.Errorf("the real top-level key survived:\n%s", got) + } + // The decoy in a string value must be untouched, character for character. + if note, _ := doc["some.note"].(string); !strings.Contains(note, `"http.proxy"`) { + t.Errorf("a mention of the key inside another value was edited: %q", note) + } + // The nested one is a different member of a different object. + nested, _ := doc["nested"].(map[string]any) + if nested[bobProxyKey] != "http://inner.example:1" { + t.Errorf("a nested object's same-named key was edited: %v", nested) + } + if doc["z"] != float64(1) { + t.Errorf("an unrelated key was lost: %v", doc["z"]) + } +} + +// lineDiff reports which lines are only in b (added) and only in a (removed), counting +// duplicates. Good enough to assert "exactly these lines changed" without pulling in a +// diff library. +func lineDiff(a, b string) (added, removed []string) { + count := map[string]int{} + for _, l := range strings.Split(a, "\n") { + count[l]++ + } + for _, l := range strings.Split(b, "\n") { + if count[l] > 0 { + count[l]-- + continue + } + added = append(added, l) + } + seen := map[string]int{} + for _, l := range strings.Split(b, "\n") { + seen[l]++ + } + for _, l := range strings.Split(a, "\n") { + if seen[l] > 0 { + seen[l]-- + continue + } + removed = append(removed, l) + } + return added, removed +} + func TestBobEnable_PreservesEverythingElse(t *testing.T) { settings, cfg := fixture(t, bobSettings) before := bobDoc(t, settings) @@ -385,13 +652,25 @@ func TestBobDisable_WithUnreadableConfig(t *testing.T) { } // Status reports and never acts: three states, all exit 0, nothing written. +// +// The verdict is asserted as the WHOLE FIRST LINE, not as a substring anywhere in the +// output, because "leads with the verdict" is the property being claimed. This used to +// open with the raw `"http.proxy"=...` key and leave the reader to derive the answer, +// and a strings.Contains assertion cannot tell that shape from this one: it passes just +// as well with the verdict buried on line six. Equality on line one is what fails when +// something is prepended above it. +// +// The negative spelling is checked too, on the "ours" case. "IBM Bob is *NOT* +// configured..." CONTAINS "IBM Bob is", so a truncated or mis-assembled verdict could +// satisfy a prefix check while saying the opposite of the truth. The two strings are +// each other's trap, so each case pins that the other one is absent. func TestBobStatus_ThreeStates(t *testing.T) { for _, tc := range []struct { name, settings, want string }{ - {"absent", `{"editor.fontSize": 13}`, "not enabled"}, - {"ours", `{"http.proxy": "http://127.0.0.1:47600"}`, "enabled in"}, - {"foreign", `{"http.proxy": "http://proxy.corp.example.com:3128"}`, "not enabled"}, + {"absent", `{"editor.fontSize": 13}`, bobStatusNo}, + {"ours", `{"http.proxy": "http://127.0.0.1:47600"}`, bobStatusYes}, + {"foreign", `{"http.proxy": "http://proxy.corp.example.com:3128"}`, bobStatusNo}, } { t.Run(tc.name, func(t *testing.T) { settings, cfg := fixture(t, tc.settings) @@ -405,9 +684,27 @@ func TestBobStatus_ThreeStates(t *testing.T) { if code := bobStatus(settings, cfg, want, ca, &out); code != 0 { t.Fatalf("exit = %d, want 0 — a report is not a verdict", code) } - if !strings.Contains(out.String(), tc.want) { - t.Errorf("output does not contain %q:\n%s", tc.want, out.String()) + + lines := strings.Split(out.String(), "\n") + if lines[0] != tc.want { + t.Errorf("first line = %q, want %q\nfull output:\n%s", lines[0], tc.want, out.String()) + } + // The other verdict must not appear anywhere: one report, one answer. + other := bobStatusYes + if tc.want == bobStatusYes { + other = bobStatusNo + } + if strings.Contains(out.String(), other) { + t.Errorf("both verdicts present:\n%s", out.String()) } + + // The paragraph the message-simplification request asked to be removed. It + // said status "does not act", which the exit code and this test's own + // byte comparison below already establish. + if strings.Contains(out.String(), "Not run here") { + t.Errorf("the removed paragraph is back:\n%s", out.String()) + } + after, err := os.ReadFile(settings) if err != nil { t.Fatal(err) @@ -422,6 +719,74 @@ func TestBobStatus_ThreeStates(t *testing.T) { } } +// Liveness is a SEPARATE axis from ownership, and the WARNING line must track only the +// first. The distinction is not cosmetic: the naive version of this warned that a +// foreign proxy was down, which is both untrue (a corporate proxy is reachable from +// somewhere, just not from here) and none of abctl's business — it reads as a complaint +// about a setting this command deliberately leaves alone. +// +// Nothing listens on any of these ports in a test, so "nothing is listening" is the +// shared condition and the only variable is whose value it is. That is what makes the +// table a fair comparison: same liveness, different ownership, opposite expectations. +func TestBobStatus_WarnsOnlyForAProxyItClaims(t *testing.T) { + // 47600 is NOT usable for the dead-proxy case: on a developer machine the real + // Cortex proxy is listening on it, so the fixture's hardcoded port would make + // "ours and nothing listening" quietly depend on whether the author had run + // `abctl service stop`. It passed on CI and failed here, which is the wrong way + // round for a test about liveness. So the whole table moves to a port the OS just + // confirmed is free, in both the config and the settings value — ownership is a + // whole host+port match, so the two must move together or the case stops being + // about liveness at all. + dead := freePort(t) + + for _, tc := range []struct { + name, settings string + wantWarning bool + }{ + // Ours and dead: the warning is the whole point — the setting is right and the + // service is stopped, which is the normal state of a laptop. + {"ours and nothing listening", `{"http.proxy": "http://127.0.0.1:` + dead + `"}`, true}, + // Not ours: silent. Judging someone else's proxy is out of scope. + {"foreign", `{"http.proxy": "http://proxy.corp.example.com:3128"}`, false}, + // Drifted but still loopback: silent too. The actionable advice is `enable`, + // which the detail lines give; a liveness complaint about the stale port on top + // of it is noise about a value abctl is already telling the user to replace. + {"drifted port", `{"http.proxy": "http://127.0.0.1:47699"}`, false}, + // Unset: there is no address to probe, so there is nothing to warn about. + {"unset", `{"editor.fontSize": 13}`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + settings, cfg := fixture(t, tc.settings) + movePort(t, cfg, dead) + + var out bytes.Buffer + want, ca := bobWanted(cfg, t.TempDir()) + if code := bobStatus(settings, cfg, want, ca, &out); code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + + got := out.String() + warned := strings.Contains(got, "WARNING: No proxy is listening at ") + if warned != tc.wantWarning { + t.Errorf("warned = %v, want %v:\n%s", warned, tc.wantWarning, got) + } + if !tc.wantWarning { + return + } + // When it does fire it must name the address, so the reader knows which + // thing to start, and it must be the SECOND line — directly under the + // verdict, above the detail — as the request specified. + lines := strings.Split(got, "\n") + if len(lines) < 2 || !strings.HasPrefix(lines[1], "WARNING: No proxy is listening at ") { + t.Errorf("the warning is not on line 2:\n%s", got) + } + if !strings.Contains(lines[1], "127.0.0.1:"+dead) { + t.Errorf("the warning does not name the address: %q", lines[1]) + } + }) + } +} + // Status must distinguish "pointing at Cortex" from "pointing at the port Cortex uses // now" — reporting drift is useful, silently moving it is not. func TestBobStatus_ReportsPortDrift(t *testing.T) { @@ -962,3 +1327,45 @@ func TestBobBackupNote_MatchesWhatWriteSettingsDoes(t *testing.T) { } }) } + +// freePort returns a TCP port on loopback that nothing is listening on, by binding one +// and closing it immediately. +// +// Inherently a race — the port could be taken between the close and the probe — but a +// far smaller one than hardcoding 47600, which on a developer machine is occupied by +// the very service under discussion, deterministically and for the whole session. An +// ephemeral port the kernel just handed out is not reused that quickly. +func freePort(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + _, port, err := net.SplitHostPort(ln.Addr().String()) + if err != nil { + ln.Close() + t.Fatal(err) + } + if err := ln.Close(); err != nil { + t.Fatal(err) + } + return port +} + +// movePort rewrites the fixture config's forward-proxy port, so a test can choose an +// address instead of inheriting 47600 from the fixture. Same strings.Replace trick +// TestClaudeCodeEnable_ReadsAddressesFromConfig uses on the same fixture. +func movePort(t *testing.T, cfgPath, port string) { + t.Helper() + body, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatal(err) + } + moved := strings.Replace(string(body), "127.0.0.1:47600", "127.0.0.1:"+port, 1) + if moved == string(body) { + t.Fatalf("the fixture config no longer names 127.0.0.1:47600, so the port could not be moved:\n%s", body) + } + if err := os.WriteFile(cfgPath, []byte(moved), 0o600); err != nil { + t.Fatal(err) + } +} From 3f77b05c7fb8ad0d93cd07941b2f09403afd244e Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 16:44:43 -0400 Subject: [PATCH 4/8] Fix: Harden bob's http.proxy ownership and splice Four defects in `abctl configure bob`, all on the disable path's "is this value ours" decision or on the textual splice that removes it. 1. A settings.json with `http.proxy` twice is legal JSON, and both Go and VS Code take the last value. bobFindMember returned on the first match, so disable spliced out only that one and exited 0 claiming "IBM Bob no longer routes through Cortex" while Bob was still routed. bobFindMember now reports a duplicate as an error and all three verbs refuse by name rather than act on a file they cannot read unambiguously. The check is byte-level because readSettings decodes to map[string]any, which collapses a duplicate key before any caller can see it. 2. bobOwns ignored userinfo, so `http://user:secret@localhost:47600` was judged ours and disable deleted it, credentials included. A URL carrying credentials is not a value abctl would ever have written. 3. Only Hostname()/Port() were compared, so `.../proxy.pac`, `...?next=` and `...#frag` all passed as ours and were deleted. Path must now be "" or "/" (a hand-typed trailing slash stays removable) with empty RawQuery and Fragment. README already claimed a path cannot pass. 4. bobSpliceOut left a stray blank line or doubled space when the key shared a line with a neighbour, contradicting "Nothing else in the file changes" and the README's byte-for-byte claim. ownLine is now computed from both sides of the member, and the splice absorbs the separator it leaves behind. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/cmd_bob.go | 148 +++++++++++++++++++++-- cmd/abctl/cmd_bob_test.go | 240 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 377 insertions(+), 11 deletions(-) diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index 5ef02263a..a9fb018ab 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -268,6 +268,23 @@ func bobOwns(val, wantProxy string) bobOwnership { if u.Scheme != "http" { return bobNotOurs } + // Everything enable writes is exactly scheme://host:port — no credentials, no + // path, no query, no fragment. So anything carrying one of those was typed by + // somebody else, and this is the check that keeps a delete off it. Without it + // only Hostname() and Port() were compared, and url.Parse puts the rest in + // fields nobody looked at: + // + // http://user:secret@localhost:47600 credentials, deleted with the value + // http://localhost:47600/proxy.pac a PAC script, not our proxy + // http://localhost:47600/?next=evil a redirector that happens to be local + // http://localhost:47600#frag + // + // An empty path and a bare "/" both mean "no path": url.Parse gives "" for + // http://h:p and "/" for http://h:p/, and enable's own value takes the first + // form, so accepting both keeps a hand-typed trailing slash ours. + if u.User != nil || (u.Path != "" && u.Path != "/") || u.RawQuery != "" || u.Fragment != "" { + return bobNotOurs + } if wantProxy == "" { // Nothing to compare against. A loopback proxy is shaped like ours and // nothing contradicts it; anything else plainly is not. @@ -544,6 +561,13 @@ func bobSetKey(src []byte, key string, value any) ([]byte, error) { // bobFindMember locates the byte span of a top-level member, from the opening quote // of its name through the last byte of its value. Offsets come from json.Decoder, // so they are the tokenizer's view of the document and not a textual guess. +// +// A document may legally name the same key twice, and both Go and VS Code take the +// LAST one. Splicing out a single span cannot express "remove both", and removing +// either one alone leaves the key still set while every message this command prints +// says it is gone — so this refuses instead, by name. The whole document is scanned +// rather than returning at the first match, which is what makes the second occurrence +// visible at all. func bobFindMember(src []byte, key string) (start, end int, found bool, err error) { dec := json.NewDecoder(bytes.NewReader(src)) tok, err := dec.Token() @@ -572,10 +596,28 @@ func bobFindMember(src []byte, key string) (start, end int, found bool, err erro return 0, 0, false, err } if name == key { - return nameStart, int(dec.InputOffset()), true, nil + if found { + return 0, 0, false, fmt.Errorf("%q appears more than once at the top level; "+ + "remove the duplicate first — this command edits one member and cannot "+ + "say which of them wins", key) + } + start, end, found = nameStart, int(dec.InputOffset()), true } } - return 0, 0, false, nil + return start, end, found, nil +} + +// bobTailIsOnlySeparator reports whether what follows a member on its own line is +// nothing but a comma and whitespace — i.e. no other key shares the line to the right. +// +// Separate from a plain TrimSpace check because the member's own trailing comma is +// part of the separator and not a neighbour. +func bobTailIsOnlySeparator(tail []byte) bool { + t := bytes.TrimSpace(tail) + if len(t) > 0 && t[0] == ',' { + t = bytes.TrimSpace(t[1:]) + } + return len(t) == 0 } // bobSpliceOut removes a member and exactly one of the commas around it, leaving the @@ -592,9 +634,21 @@ func bobFindMember(src []byte, key string) (start, end int, found bool, err erro // FOLLOWING comma so that removing the last member does not leave a trailing comma, // which is invalid JSON. func bobSpliceOut(src []byte, start, end int) []byte { - // Is the member alone on its line? Only then can the line be taken whole. + // Is the member alone on its line? Only then can the line be taken whole — and + // "alone" means on BOTH sides. A member that is first on its line but shares it + // with a later key still may not have its line's leading indentation removed: + // that indentation belongs to the key that survives. Checking only the left side + // left `{\n "http.proxy": ..., "b": 2,\n` as `{\n "b": 2,` — the indent gone + // and one stray space in its place. lineStart := bytes.LastIndexByte(src[:start], '\n') + 1 - ownLine := len(bytes.TrimSpace(src[lineStart:start])) == 0 + lineEnd := bytes.IndexByte(src[start:], '\n') + if lineEnd < 0 { + lineEnd = len(src) + } else { + lineEnd += start + } + ownLine := len(bytes.TrimSpace(src[lineStart:start])) == 0 && + bobTailIsOnlySeparator(src[end:lineEnd]) // Walk forward past whitespace to a following comma, if there is one. after := end @@ -604,19 +658,44 @@ func bobSpliceOut(src []byte, start, end int) []byte { from, to := start, end if after < len(src) && src[after] == ',' { - // Not the last member: take the member, the comma, and the rest of its line - // including the newline, so the next member keeps its own indentation. + // Not the last member: take the member, the comma, and — only when the member + // had the line to itself — the rest of that line including the newline, so the + // next member keeps its own indentation. + // + // The ownLine guard is load-bearing. A member SHARING a line with another key + // still has a blank line-tail after its comma when the next key is on the line + // below, so without the guard this arm consumed that newline and the following + // line's indentation too, welding two lines together: + // + // {"a": 1,\n "b": 2, "http.proxy": "...",\n "c": 3} + // became ...\n "b": 2, "c": 3\n... + // + // which is exactly the layout change "Nothing else in the file changes" says + // this function does not make. When the member shares its line, only its own + // bytes and its comma may go. to = after + 1 - if nl := bytes.IndexByte(src[to:], '\n'); nl >= 0 && len(bytes.TrimSpace(src[to:to+nl])) == 0 { + if nl := bytes.IndexByte(src[to:], '\n'); ownLine && nl >= 0 && len(bytes.TrimSpace(src[to:to+nl])) == 0 { to += nl + 1 } else if !ownLine { - // A single-line document: `{"a": 1, "http.proxy": "...", "b": 2}`. There is - // no line to take, and the space that separated this member from the next - // one would be left beside the space after the previous comma, making a - // double space. Take one of them. + // The member shares its line — either a single-line document, + // `{"a": 1, "http.proxy": "...", "b": 2}`, or one line of a multi-line one. + // Its own line cannot be taken, and the space that separated this member + // from what follows would be left beside the space after the previous + // comma, making a double space. Take one of them. for to < len(src) && src[to] == ' ' { to++ } + // Nothing but the line's end follows: the space just taken was the only + // thing between the comma and the newline, so the comma we absorbed leaves + // a trailing space behind instead. Walk BACK over the spaces before the + // member so the surviving key ends at its own comma. Trailing whitespace is + // invisible in a terminal and loud in a diff, and it is what + // TestBobDisable_LeavesNoStrayWhitespace forbids. + if to < len(src) && src[to] == '\n' { + for from > 0 && src[from-1] == ' ' { + from-- + } + } } if ownLine { from = lineStart @@ -805,6 +884,34 @@ func bobBackupNote(settingsPath string) string { return unchanged + "; a copy is kept as " + bak + "\n\n" } +// bobDuplicateKey reports whether the settings file names the proxy key more than +// once at the top level, which readSettings cannot show: it decodes into a map, and a +// map keeps only the last of two same-named members. +// +// Called before anything is printed or prompted. bobWriteKey refuses the same file, so +// leaving this out still fails safe — but it fails AFTER asking the user to approve a +// write that then cannot happen, and after status has already reported one of the two +// values as though it were the setting. +func bobDuplicateKey(settingsPath string) error { + src, err := os.ReadFile(settingsPath) + if err != nil { + // Unreadable or absent is not this check's business: the caller's own + // readSettings reports it, with its own wording. + return nil + } + if len(bytes.TrimSpace(src)) == 0 { + return nil + } + _, _, _, ferr := bobFindMember(src, bobProxyKey) + // Only the duplicate verdict is this check's to report. Any other parse failure is + // readSettings' to describe, and bob's readSettings arm says more about it than a + // tokenizer error would. + if ferr != nil && strings.Contains(ferr.Error(), "more than once") { + return ferr + } + return nil +} + func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.Writer) int { want, cfg, err := wantedFromConfig(cortexCfgPath) if err != nil { @@ -827,6 +934,11 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W } proxy := want[envProxy] + if derr := bobDuplicateKey(settingsPath); derr != nil { + fmt.Fprintf(stderr, "abctl: %s: %v\n", settingsPath, derr) + return 1 + } + doc, err := readSettings(settingsPath) if err != nil { // readSettings names the file and says to fix or move it. The clause about @@ -921,6 +1033,11 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W } func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr io.Writer) int { + if derr := bobDuplicateKey(settingsPath); derr != nil { + fmt.Fprintf(stderr, "abctl: %s: %v\n", settingsPath, derr) + return 1 + } + doc, err := readSettings(settingsPath) if err != nil { fmt.Fprintf(stderr, "abctl: %v\n", err) @@ -997,6 +1114,15 @@ func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io. // claudeCodeStatus and bobShellStatus make. A non-zero status here would make // `abctl configure bob status` unusable in a shell conditional for anything but // "is it on". + if derr := bobDuplicateKey(settingsPath); derr != nil { + // Two values, and no way to say which one Bob uses without reimplementing its + // precedence. Reporting either as "the setting" would be a guess, so the + // verdict is "not" — this command cannot confirm the configuration — and the + // reason is the actionable part. Still exit 0: it is a successful report. + fmt.Fprintf(stdout, "%s\n %s: %v\n", bobStatusNo, settingsPath, derr) + return 0 + } + doc, err := readSettings(settingsPath) if err != nil { // An unreadable or non-JSON settings file cannot be configured, so the verdict diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 818980eea..77fc8b56b 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "testing" ) @@ -213,6 +214,16 @@ func TestBobEnableDisable_RoundTripsByteForByte(t *testing.T) { // comma is invalid JSON), the only member leaves an empty object, and a member between // two blank-line separators must take one of them or the file gains a blank line. // +// The SHARED-LINE rows are the ones the single-line row cannot stand in for, and the +// distinction is what a reviewer found broken. A whole-document single line is not +// ownLine on either side, so the splicer's "take the rest of the line" arm never fires +// for it. A member sharing ONE line of a multi-line document is the mixed case: it has +// a newline after it (so that arm did fire, welding the next line onto this one) and it +// may still be first on its line (so the leading-indent arm fired too, eating the +// indentation the surviving key needs). All three placements are pinned — the removed +// key first on the line, in the middle, and last — because each one exercises a +// different pair of those arms. +// // The assertions are on bytes and on re-parseability, because "valid JSON" and "no // stray whitespace" are different claims and the early implementations of this splicer // satisfied the first while failing the second. @@ -243,6 +254,35 @@ func TestBobDisable_LeavesNoStrayWhitespace(t *testing.T) { in: "{\"a\": 1, \"http.proxy\": \"http://127.0.0.1:47600\", \"b\": 2}\n", want: "{\"a\": 1, \"b\": 2}\n", }, + { + // Shares a line to its LEFT, next key on the line below. The reviewer's + // case: the newline-taking arm used to fire here and produce + // `"b": 2, "c": 3` — two of the user's lines welded into one. + name: "shares a line, next key on the following line", + in: "{\n \"a\": 1,\n \"b\": 2, \"http.proxy\": \"http://127.0.0.1:47600\",\n \"c\": 3\n}\n", + want: "{\n \"a\": 1,\n \"b\": 2,\n \"c\": 3\n}\n", + }, + { + // Shares a line to its RIGHT. It IS first on its line, so the arm that + // removes the line's leading indentation fired and left `{\n "b": 2,` — + // the four-space indent replaced by one stray space. + name: "shares a line, first on it", + in: "{\n \"http.proxy\": \"http://127.0.0.1:47600\", \"b\": 2,\n \"c\": 3\n}\n", + want: "{\n \"b\": 2,\n \"c\": 3\n}\n", + }, + { + // Shares a line on BOTH sides, mid-document: neither line arm may fire. + name: "shares a line on both sides", + in: "{\n \"a\": 1,\n \"b\": 2, \"http.proxy\": \"http://127.0.0.1:47600\", \"c\": 3\n}\n", + want: "{\n \"a\": 1,\n \"b\": 2, \"c\": 3\n}\n", + }, + { + // Shares a line and is the document's last member, so there is no following + // comma: the preceding-comma arm runs on a shared line. + name: "shares a line and is last", + in: "{\n \"a\": 1,\n \"b\": 2, \"http.proxy\": \"http://127.0.0.1:47600\"\n}\n", + want: "{\n \"a\": 1,\n \"b\": 2\n}\n", + }, } { t.Run(tc.name, func(t *testing.T) { settings, cfg := fixture(t, tc.in) @@ -1043,6 +1083,35 @@ func TestBobOwns(t *testing.T) { {"https", "https://127.0.0.1:19999", want, bobNotOurs}, {"socks5", "socks5://127.0.0.1:19999", want, bobNotOurs}, + // Everything outside host and port. url.Parse files these under User, Path, + // RawQuery and Fragment, and comparing only Hostname()/Port() reads none of + // them — so each of these matched the configured proxy exactly and disable + // deleted it. The credentials row is the worst of them: the secret goes out + // with the value. + // + // These are NOT the substring rows above. Those fail a whole-value comparison + // on the host; these all carry the right host and the right port, and are + // still somebody else's setting. + {"credentials", "http://user:secret@127.0.0.1:19999", want, bobNotOurs}, + {"username only", "http://user@127.0.0.1:19999", want, bobNotOurs}, + {"a PAC script path", "http://127.0.0.1:19999/proxy.pac", want, bobNotOurs}, + {"a query", "http://127.0.0.1:19999/?next=evil", want, bobNotOurs}, + {"a query with no path", "http://127.0.0.1:19999?next=evil", want, bobNotOurs}, + {"a fragment", "http://127.0.0.1:19999#frag", want, bobNotOurs}, + + // The other edge of that gate, and the reason it tests Path against a set + // rather than for emptiness: url.Parse gives "" for http://h:p and "/" for + // http://h:p/, so treating any non-empty Path as foreign would make a + // hand-typed trailing slash unremovable. enable writes the first form; a + // user who typed the second still wrote our address. + {"trailing slash", "http://127.0.0.1:19999/", want, bobOurs}, + + // Same gate on the no-config path, which reaches it through a different + // branch: there is no value to compare against, so only the shape decides. + {"no config, credentials", "http://user:secret@127.0.0.1:47600", "", bobNotOurs}, + {"no config, a path", "http://127.0.0.1:47600/proxy.pac", "", bobNotOurs}, + {"no config, trailing slash", "http://127.0.0.1:47600/", "", bobUnknown}, + // No config to compare against: the third state. A loopback http proxy is // the shape abctl writes, so it is removable; anything else is not. {"no config, loopback", "http://127.0.0.1:47600", "", bobUnknown}, @@ -1066,6 +1135,177 @@ func TestBobOwns(t *testing.T) { } } +// A settings file may legally name the same key twice, and every JSON reader that +// keeps one value keeps the LAST — Go's encoding/json does, and so does VS Code. That +// makes a duplicate invisible to every decision this command makes: readSettings +// decodes into a map, so the first value is gone before anything looks at it. +// +// The reported failure was disable. It spliced out the FIRST occurrence, left the +// second in place, and printed "Disabled. IBM Bob no longer routes through Cortex." +// while Bob was still routed through it — a false statement with exit 0 behind it. +// +// So all three verbs refuse instead, and all three are asserted here rather than just +// the one that was reported: they share bobDuplicateKey, and a regression in it would +// surface in whichever verb the next reader happens to run. +// +// The file is compared byte-for-byte after each run because "refuses" has to mean the +// file is untouched, not merely that the exit code changed. +func TestBob_RefusesADuplicateProxyKey(t *testing.T) { + // Both occurrences point at Cortex, so nothing here turns on ownership: the + // refusal is about not being able to say which member is the setting. A file + // where only one of them is ours would let a reader think the check is an + // ownership check. + const dup = `{ + "editor.fontSize": 13, + "http.proxy": "http://127.0.0.1:47600", + "http.proxyStrictSSL": false, + "http.proxy": "http://127.0.0.1:47600" +}` + + // The premise. If encoding/json ever started rejecting a duplicate key, or kept + // the first value, the rest of this test would be reasoning about a file shape + // that cannot occur — so the shape is verified before it is relied on. + t.Run("the premise: a decoder hides the duplicate", func(t *testing.T) { + var doc map[string]any + if err := json.Unmarshal([]byte(dup), &doc); err != nil { + t.Fatalf("the fixture is not valid JSON, so the whole test is vacuous: %v", err) + } + if len(doc) != 3 { + t.Errorf("decoded %d keys, want 3 — a duplicate should collapse", len(doc)) + } + }) + + for _, verb := range []string{"enable", "disable", "status"} { + t.Run(verb, func(t *testing.T) { + settings, cfg := fixture(t, dup) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + want, ca := bobWanted(cfg, t.TempDir()) + + var out, errb bytes.Buffer + var code int + switch verb { + case "enable": + code = bobEnable(settings, cfg, true, &out, &errb) + case "disable": + code = bobDisable(settings, want, ca, true, &out, &errb) + case "status": + code = bobStatus(settings, cfg, want, ca, &out) + } + + // status reports; the two writers fail. Both are "refused" — the + // difference is only whether refusing is this verb's answer or its error. + if verb == "status" { + if code != 0 { + t.Errorf("exit = %d, want 0 — a report is not a verdict", code) + } + if lines := strings.Split(out.String(), "\n"); lines[0] != bobStatusNo { + t.Errorf("first line = %q, want %q\n%s", lines[0], bobStatusNo, out.String()) + } + // Reporting either value as "the setting" would be the guess this + // check exists to avoid. + if strings.Contains(out.String(), bobStatusYes) { + t.Errorf("status claimed the configuration it cannot read:\n%s", out.String()) + } + } else if code != 1 { + t.Errorf("exit = %d, want 1", code) + } + + // The key must be NAMED. "this file has a problem" sends the reader + // looking; the key name and the word duplicate tell them what to delete. + said := out.String() + errb.String() + for _, want := range []string{strconv.Quote(bobProxyKey), "more than once"} { + if !strings.Contains(said, want) { + t.Errorf("output does not contain %q:\n%s", want, said) + } + } + + // The reported symptom, asserted directly so a regression names itself: + // disable claimed success over a file it had not fixed. + if strings.Contains(said, "no longer routes through Cortex") { + t.Errorf("%s claimed the proxy was removed:\n%s", verb, said) + } + + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Errorf("%s wrote to a file it refused:\n--- before\n%s\n--- after\n%s", + verb, before, after) + } + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Errorf("%s left a .bak behind for a write it did not make", verb) + } + }) + } +} + +// A duplicate somewhere OTHER than the proxy key is none of this check's business. +// Refusing on any duplicate at all would make the command unusable on a settings file +// whose unrelated keys happen to repeat, and the splice only ever touches one member. +func TestBob_ADuplicateOfAnotherKeyIsNotRefused(t *testing.T) { + settings, cfg := fixture(t, `{"editor.fontSize": 13, "editor.fontSize": 14}`) + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0: %s", code, errb.String()) + } + if got := bobDoc(t, settings)[bobProxyKey]; got == nil { + t.Errorf("enable refused a file whose duplicate is not its key:\n%s%s", out.String(), errb.String()) + } +} + +// The table above pins bobOwns; this pins that disable actually consults it, on the +// one case where getting it wrong destroys something the user cannot recover. +// +// A proxy URL carrying credentials is not a hypothetical shape — it is how an +// authenticating forward proxy is configured in a settings file, and the password is +// usually nowhere else. The old ownership test compared only Hostname() and Port(), so +// `http://user:secret@127.0.0.1:47600` on a machine whose Cortex listens on 47600 was +// judged ours and deleted, secret included. +// +// Written through bobDisable rather than as another bobOwns row because the claim is +// about the file: a correct verdict that the caller ignores loses the value just the +// same. The secret is asserted present in the bytes, not merely the key. +func TestBobDisable_LeavesACredentialBearingProxyAlone(t *testing.T) { + const secret = "s3cr3t-not-ours" + settings, cfg := fixture(t, + `{"http.proxy": "http://corpuser:`+secret+`@127.0.0.1:47600"}`) + + // The port is Cortex's own, and the host is loopback — everything the old check + // looked at says this is ours. Only the userinfo says otherwise, which is the + // point. + want, ca := bobWanted(cfg, t.TempDir()) + + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + var out, errb bytes.Buffer + if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + t.Fatalf("exit = %d, want 0 — a foreign proxy is not an error: %s", code, errb.String()) + } + + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Errorf("disable rewrote a file it does not own:\n--- before\n%s\n--- after\n%s", before, after) + } + if !bytes.Contains(after, []byte(secret)) { + t.Error("the credential is gone from the settings file") + } + // And it must say so: silently leaving the value would look identical to having + // removed it, which is the report the user acts on. + if !strings.Contains(out.String(), "http.proxy") { + t.Errorf("disable did not name the value it left in place:\n%s", out.String()) + } +} + // The three states must be distinct values, or a switch over them collapses two // cases into one and every table above still passes. func TestBobOwnershipStatesAreDistinct(t *testing.T) { From 6d5fa922ccc6fad5cc23ae9658ef8b59a50df8b5 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 17:43:30 -0400 Subject: [PATCH 5/8] Fix: Refuse to delete an unconfirmable proxy under --yes Two review findings. 1. `disable --yes` could delete a foreign proxy. When the Cortex config is missing or unparseable, bobWanted returns an empty proxy address, so no port comparison happens at all and every loopback http proxy classifies as bobUnknown -- Squid on 3128, a corporate agent, a dev tunnel, not just Cortex's 476xx block. Reproduced: `disable --yes --config /nonexistent` silently removed "http.proxy": "http://localhost:3128". Interactively that is survivable, because the prompt prints the value and the user recognizes their own proxy. --yes removes exactly that safeguard, so the unattended path now refuses: exit 1, naming the value and both ways out (re-run without --yes, or pass a readable --config). The prompted path is unchanged -- it still removes the value after printing it and saying it is judging by shape alone, because the off switch has to keep working after Cortex is uninstalled. TestBobDisable_WithUnreadableConfig is narrowed from the unattended to the prompted path, which is where its claim now lives; the collision between the two requirements is documented in the test. 2. bobBackupNote's doc comment was attached to bobSetKey. A missing blank line made godoc render the backup-note prose as bobSetKey's documentation, while the real bobBackupNote had none. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/cmd_bob.go | 47 +++++++--- cmd/abctl/cmd_bob_test.go | 183 +++++++++++++++++++++++++++++++++++++- 2 files changed, 216 insertions(+), 14 deletions(-) diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index a9fb018ab..db5ea25ba 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -494,19 +494,6 @@ func bobWanted(cortexCfgPath, home string) (proxy, caPath string) { return want[envProxy], caPath } -// bobBackupNote describes what this particular write will and will not preserve. -// -// Three different true statements, because writeSettings makes three different -// choices and the message used to claim only the first. It writes .bak from -// the file's current contents ONLY when the file exists AND no .bak is there -// already — never overwriting, because a second run would otherwise replace the -// pristine pre-Cortex file with one abctl had already edited. -// -// So "a copy is kept as .bak" was false twice over: on a settings file that -// does not exist yet there is nothing to copy, and when a .bak survives from an -// earlier run the copy kept is that older one, not this run's. Promising a backup -// that is not made is worse than promising none — it is the sentence a user leans on -// before saying yes. // bobSetKey writes one top-level key into a settings file, changing nothing else // in it — byte for byte. It is the reason bob does not call writeSettings. // @@ -859,6 +846,19 @@ func bobWriteKey(path, key string, value any) error { return os.Rename(tmp, path) } +// bobBackupNote describes what this particular write will and will not preserve. +// +// Three different true statements, because writeSettings makes three different +// choices and the message used to claim only the first. It writes .bak from +// the file's current contents ONLY when the file exists AND no .bak is there +// already — never overwriting, because a second run would otherwise replace the +// pristine pre-Cortex file with one abctl had already edited. +// +// So "a copy is kept as .bak" was false twice over: on a settings file that +// does not exist yet there is nothing to copy, and when a .bak survives from an +// earlier run the copy kept is that older one, not this run's. Promising a backup +// that is not made is worse than promising none — it is the sentence a user leans on +// before saying yes. func bobBackupNote(settingsPath string) string { // This sentence is a literal claim, and bobWriteKey is what makes it one: it splices // a single member in or out of the existing bytes, so key order, indentation, inline @@ -1060,6 +1060,27 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr return 0 } + // A guess plus --yes is not consent. bobUnknown means the config could not be + // read, so there was no address to compare against and all that is known is the + // value's SHAPE: a loopback http proxy. That describes every local proxy anyone + // runs — Squid on 3128, a corporate agent, a dev tunnel — not just Cortex's + // 476xx block, because with no config there is no port to compare at all. + // + // Interactively that is survivable: the prompt prints the value and the user + // recognizes their own proxy. --yes removes exactly that safeguard, so a + // scripted `disable --yes` would silently delete a stranger's proxy. Refuse + // instead, and say which flag turns the guess into an answer. Exit 1, not 0: + // the user asked for a removal that did not happen. + if yes && bobOwns(s, wantProxy) == bobUnknown { + fmt.Fprintf(stderr, "abctl: %s sets %q to %q, which is shaped like a Cortex\n"+ + " proxy but cannot be confirmed as one: the Cortex config could not be read,\n"+ + " so there is no address to compare against — every loopback http proxy looks\n"+ + " like this. Refusing to delete it unattended. Re-run without --yes to see the\n"+ + " value and decide, or pass --config with a readable Cortex config.\n", + settingsPath, bobProxyKey, s) + return 1 + } + fmt.Fprintf(stdout, "Removes from %s:\n %q: %q\n", settingsPath, bobProxyKey, s) if bobOwns(s, wantProxy) == bobUnknown { // Say it is a guess, because it is: with no readable config there is no diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 77fc8b56b..628b343b1 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -659,6 +659,16 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { // Both halves are asserted, because removing the value is only half the contract: // the user is entitled to know the judgment was made by shape rather than by // matching their config. +// +// NARROWED to the PROMPTED path. This test used to pass yes=true, and the refusal +// added for the unattended case broke it — correctly. The two requirements collide +// only there: "the off switch must not depend on the config" (this test) and "a +// guess plus --yes is not consent" +// (TestBobDisable_RefusesToDeleteAGuessUnattended) are both satisfiable, because +// the safeguard --yes removes is the prompt that names the value. So the contract +// this test pins is unchanged in substance and now reads: with the config gone, +// disable still works — it asks first. Removing the prompt is what is refused, not +// removing the value. func TestBobDisable_WithUnreadableConfig(t *testing.T) { settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47600"}`) if err := os.Remove(cfg); err != nil { @@ -679,10 +689,17 @@ func TestBobDisable_WithUnreadableConfig(t *testing.T) { t.Errorf("bobOwns with no config = %v, want bobUnknown", got) } + // Answers yes, standing in for a user who read the printed value and recognized + // it. Asserting the prompt happened is what keeps this row distinct from the + // unattended one: without it, a fix that dropped the prompt entirely would pass. + asked := noPrompt(t, true) var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, want, ca, false, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } + if *asked != 1 { + t.Errorf("prompted %d times, want 1", *asked) + } if _, ok := bobDoc(t, settings)[bobProxyKey]; ok { t.Error("disable failed with the config missing, which is when it is most needed") } @@ -1609,3 +1626,167 @@ func movePort(t *testing.T, cfgPath, port string) { t.Fatal(err) } } + +// --yes must not delete a proxy that is only a GUESS. +// +// The reviewer's fixture, executed literally before anything was reasoned about: +// `disable --yes --config /nonexistent` against `{"http.proxy": +// "http://localhost:3128"}` silently removed Squid's proxy and exited 0. +// +// The mechanism is worth stating precisely, because the PR body first understated +// it. With no readable config, bobWanted returns proxy == "" — so bobOwns has no +// address to compare against and falls back to judging SHAPE alone, which makes the +// verdict bobUnknown. Shape is "http, loopback host, no userinfo/path/query": that +// is EVERY loopback http proxy on ANY port, not just Cortex's 476xx block, because +// with no config there is no port in hand to compare. Squid on 3128 satisfies it. +// +// Interactively that is survivable — bobConfirm prints the value and the user +// recognizes their own proxy. --yes is precisely the removal of that safeguard, so +// the two together are the unsafe combination and the one this refuses. +// +// The three rows after the first are the regression half: each is a path that MUST +// still work, and each differs from the refusing row in exactly one respect (the +// flag, the config, or the value). Without them a fix that simply stopped deleting +// would pass. +func TestBobDisable_RefusesToDeleteAGuessUnattended(t *testing.T) { + // Not Cortex's port, and not in the 476xx block at all — the whole point is that + // the port is irrelevant when there is no config to compare it against. + const foreign = "http://localhost:3128" + + // Subtest names deliberately carry NO flag spellings. t.TempDir() names its + // directory after the subtest, bobDisable interpolates the settings path into its + // message, and an assertion that the message mentions "--yes" then matches the + // PATH instead of the prose. That is not hypothetical: it is what a mutation + // removing "--yes" from the message survived on, until these rows were renamed. + for _, tc := range []struct { + name string + settings string + // readableConfig false means --config pointed at nothing, which is what makes + // bobOwns fall back to shape. + readableConfig bool + yes bool + wantCode int + wantRemoved bool + }{ + // The bug. A guess plus --yes is not consent. + {"guess, unattended", foreign, false, true, 1, false}, + // Same guess, same value, WITHOUT --yes: the prompt is the safeguard, so this + // path must still reach it. noPrompt answers yes below, standing in for a user + // who looked at the printed value and recognized it as Cortex's. + {"guess, prompted", foreign, false, false, 0, true}, + // --yes with a readable config and a matching value: ownership is known, not + // guessed, so the refusal must not touch this. This is the scripted path the + // feature exists for. + {"known ours, unattended", `http://127.0.0.1:47600`, true, true, 0, true}, + // --yes with a readable config and a foreign value: bobNotOurs, which was + // already left alone at exit 0 and must stay that way. Exit 0, not 1 — nothing + // is wrong with this file. + {"known foreign, unattended", foreign, true, true, 0, false}, + } { + t.Run(tc.name, func(t *testing.T) { + settings, cfg := fixture(t, `{"http.proxy": "`+tc.settings+`", "editor.fontSize": 13}`) + + // The config path is the only lever that decides known-vs-guessed, so it is + // what the table varies. An unreadable path is how a user who has + // uninstalled Cortex, or mistyped --config, arrives here. + cfgArg := cfg + if !tc.readableConfig { + cfgArg = filepath.Join(t.TempDir(), "does-not-exist.yaml") + } + want, ca := bobWanted(cfgArg, t.TempDir()) + + // Asserted rather than assumed: if bobWanted ever started guessing a proxy + // address, every "guess" row would silently become a "known" row and this + // test would keep passing while testing nothing. + if tc.readableConfig && want == "" { + t.Fatalf("the fixture config did not yield a proxy address, so this row cannot be about known ownership") + } + if !tc.readableConfig && want != "" { + t.Fatalf("an unreadable config yielded proxy %q, so this row cannot be about a guess", want) + } + + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + asked := noPrompt(t, true) + var out, errb bytes.Buffer + code := bobDisable(settings, want, ca, tc.yes, &out, &errb) + + if code != tc.wantCode { + t.Errorf("exit = %d, want %d\nstdout: %s\nstderr: %s", code, tc.wantCode, out.String(), errb.String()) + } + + after, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + _, stillSet := bobDoc(t, settings)[bobProxyKey] + if removed := !stillSet; removed != tc.wantRemoved { + t.Errorf("removed = %v, want %v\n--- before\n%s\n--- after\n%s", + removed, tc.wantRemoved, before, after) + } + + // A sibling key, to catch a fix that "left the file alone" by rewriting it + // wholesale. Present in every row, removed in none. + if _, ok := bobDoc(t, settings)[`editor.fontSize`]; !ok { + t.Errorf("an unrelated key was lost:\n%s", after) + } + + if !tc.wantRemoved { + // Byte-identical, not merely still-parsing-the-same: a refusal that + // reformatted the file would have written where it said it would not. + if !bytes.Equal(before, after) { + t.Errorf("the file was rewritten despite no removal:\n--- before\n%s\n--- after\n%s", before, after) + } + // writeSettings makes a .bak on its first write, so its absence is + // independent evidence that nothing was written — the assertion above + // would still pass if the file were rewritten to identical bytes. + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Error("a .bak was created, so something was written") + } + } + + if tc.wantCode == 1 { + // The refusal must be on stderr, and ONLY there: exit 1 with the + // explanation on stdout is unreadable in the scripted use this exists + // to protect, and a copy on both streams double-prints it for anyone + // merging them. + // + // Asserted as absence-from-stdout rather than as `errb.Len() != 0`. + // A length check here is SUBSUMED: every state that empties errb also + // fails the three content assertions below, which all read errb — a + // mutation copying the refusal to stdout while leaving stderr intact + // survived all four, which is how this assertion got its present + // shape. + if strings.Contains(out.String(), "cannot be confirmed as one") { + t.Errorf("the refusal is on stdout too: %q", out.String()) + } + // Naming the value is what lets the user tell "my Squid" from "stale + // Cortex" — a bare refusal sends them to the file to find out. + if !strings.Contains(errb.String(), tc.settings) { + t.Errorf("the refusal does not name the value it declined to delete: %q", errb.String()) + } + // Naming the way out is the difference between a refusal and a dead + // end. Both routes are asserted: drop --yes, or supply a config. + for _, want := range []string{"--yes", "--config"} { + if !strings.Contains(errb.String(), want) { + t.Errorf("the refusal does not mention %s: %q", want, errb.String()) + } + } + // And it must not have asked: the refusal replaces the prompt under + // --yes, it does not precede one. + if *asked != 0 { + t.Errorf("prompted %d times under --yes", *asked) + } + } + + // The prompted row is the one that proves the safeguard still exists. If + // the refusal had been written to fire regardless of --yes, this would be 0. + if tc.name == "guess, prompted" && *asked != 1 { + t.Errorf("prompted %d times, want 1 — the guess must still be offered interactively", *asked) + } + }) + } +} From bbd078195a09cffa8321a5c7adc41e81924ea092 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 20:31:01 -0400 Subject: [PATCH 6/8] Fix: Refuse bob enable/disable without a settings document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 review fixes. Code: - enable and disable now refuse when the settings path is missing, empty, or holds only `null`: IBM Bob has not saved settings there, so a write would create a file nothing reads. Previously a null document was prompted for, written, and backed up first. status reports the Cortex status as unknown instead, naming which of the three it found. The probe is bob's own rather than a change to readSettings, which claude-code shares and which cannot tell null from absent — it coerces both to an empty map. This makes bobBackupNote's no-backup arm unreachable from either verb. It is kept, with a comment saying so: the note describes writeSettings truthfully for any path, and its own test still covers that arm. - bobAbsorbSeparator tested from == 0 after the src[:from-1] slice that would panic on it. Reordered. Unreachable today, which is why it is worth fixing before a new caller makes it reachable. - bobIsLoopbackProxy's doc comment described an ownership test. It is a shape test; bobOwns is what rules on ownership. Tests: - Cover bobSetKey's found-and-replace branch, which had none: a settings file naming localhost:47600 against a config deriving 127.0.0.1:47600 is ours by ownership but differs by spelling, so it splices rather than short-circuiting. Verified by replacing the branch body with a panic — the whole suite passed before this test existed. - Pin the new refusal across all three shapes and all three verbs, and pin that the three reasons stay distinguishable, so a --settings typo does not read as a working run against an unconfigured Bob. README: - disable did not remove "only when it matches": a loopback proxy with no comparable address is "cannot tell", which disable removes after asking (and refuses under --yes). Rewritten to describe the three answers rather than to claim a two-way match. - status does not degrade to trust-store guidance off macOS — bobVerifyNote returns nothing there. The claim was the wrong half of a true statement: enable and disable do carry that guidance. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/README.md | 38 +++++--- cmd/abctl/cmd_bob.go | 90 ++++++++++++++++-- cmd/abctl/cmd_bob_test.go | 190 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 297 insertions(+), 21 deletions(-) diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index dc541fe11..d7e443fab 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -502,10 +502,11 @@ be repeated for the same reason. The keychain is named on the delete because an add to the System keychain is not undone by a delete that defaults to the login one. -`status` suggests `security verify-cert` without running it. Off macOS these -become a suggestion to add or remove the file in the OS trust store, naming the -usual Debian and Fedora routes and saying plainly that the exact step depends on -the distribution. +On macOS, `status` suggests `security verify-cert` without running it. Off +macOS it suggests nothing: there is no portable check to name, and `enable` and +`disable` already carry the trust-store guidance for those platforms — naming +the usual Debian and Fedora routes and saying plainly that the exact step +depends on the distribution. It is `ca.crt` — the single bridge CA — and deliberately **not** the `bundle.crt` in the same directory, which holds ~129 certificates and exists for @@ -526,18 +527,19 @@ only scheme `enable` writes. The loopback spellings are folded together while the settings file names `127.0.0.1`, and a hand-typed address must not be called someone else's proxy. -That is still not a record abctl keeps — there is no state file. `disable` removes -the key only when it matches, and reports anything else — a corporate proxy, a -loopback proxy on a port the config does not name, a non-string value — while -leaving it alone. `enable` refuses rather than overwriting a foreign value. The -remaining collision is narrow: your own unrelated proxy on *exactly* the address -Cortex is configured for. The prompt names the exact value first, and the `.bak` -is already written. +That is still not a record abctl keeps — there is no state file, so ownership is +re-decided from the value each time. -Ownership has three answers, not two: a config that cannot be read yields -**cannot tell** rather than "not ours", because a bool would have to guess, and -guessing "not ours" toward a `delete` is the dangerous direction. `status` says so -in those words instead of ruling on it. +Ownership has three answers, not two. A value that matches is **ours**; a +corporate proxy, or a non-string value, is **not ours** and is left alone by both +verbs. The third is **cannot tell**: when there is no address to compare against +— the config is missing or unreadable — any loopback `http` proxy could be this +one, and a bool would have to guess. Guessing "not ours" toward a `delete` is the +dangerous direction, so it is not a bool. `status` reports "cannot tell" in those +words rather than ruling on it. `disable` asks before removing such a value and +refuses under `--yes`, since `--yes` means "do not ask me", not "decide for me". +`enable` refuses rather than overwriting anything it does not own. Whatever is +removed, `writeSettings` has already kept the file as a `.bak`. **Whether anything is listening is a separate question**, reported on its own line. A stopped Cortex is the normal state of a laptop and is not a verdict on the @@ -552,6 +554,12 @@ A settings file with comments in it is refused, not rewritten. VS Code permits them; the strict JSON reader here does not, and silently stripping a user's comments to add one key is the wrong trade. +`enable` and `disable` need a settings document to already exist: a path that is +missing, empty, or holds only `null` means IBM Bob has not saved settings there, +and both refuse rather than creating a file at a path nothing reads. The refusal +names the path, and `--settings` if it is the wrong one. `status` reports the +Cortex status as unknown there, saying which of the three it found. + Only macOS's settings location is known (`~/Library/Application Support/IBM Bob/User/settings.json`). Elsewhere `--settings PATH` is required rather than guessed — writing a proxy setting into a file nothing reads is a silent no-op, diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index db5ea25ba..4e07ce472 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -336,10 +336,9 @@ func bobSameLoopback(a, b string) bool { // bobIsLoopbackProxy reports whether val is an http proxy on this machine. // -// Used ONLY by status, to tell "a local proxy that is not the configured one" — -// almost always a stale Cortex address from before the port moved — apart from a -// corporate proxy somewhere else. It is deliberately NOT an ownership test: it says -// nothing about who wrote the value, so disable must not consult it. +// A shape test, not an ownership test: it says nothing about who wrote the value. Every +// loopback http proxy satisfies it, Cortex's or not. bobOwns treats that shape as +// bobUnknown — never as bobOurs — precisely because it cannot tell them apart. func bobIsLoopbackProxy(val string) bool { u, err := url.Parse(strings.TrimSpace(val)) if err != nil || u.Host == "" || u.Scheme != "http" { @@ -781,9 +780,17 @@ func bobAbsorbSeparator(src []byte, from, to int) int { if nl < 0 || len(bytes.TrimSpace(src[to:to+nl])) != 0 { return from } - // Is what precedes it also one? Walk back over the previous line. + // Is what precedes it also one? The from == 0 test comes first because the walk + // below slices src[:from-1], which panics on from == 0 rather than reporting it. + // Unreachable today — every caller passes a from inside an object, so at least the + // opening brace precedes it — which is exactly why the order is worth fixing now + // rather than after a new caller makes it reachable. + if from == 0 || src[from-1] != '\n' { + return from + } + // Walk back over the previous line. prevStart := bytes.LastIndexByte(src[:from-1], '\n') + 1 - if from == 0 || src[from-1] != '\n' || len(bytes.TrimSpace(src[prevStart:from-1])) != 0 { + if len(bytes.TrimSpace(src[prevStart:from-1])) != 0 { return from } return prevStart @@ -875,6 +882,11 @@ func bobBackupNote(settingsPath string) string { if _, err := os.Stat(settingsPath); err != nil { // Covers a missing file and an unreadable one alike: in both cases this run // will not produce a .bak, which is the only thing being claimed. + // + // Unreachable from enable and disable since bobNoDocument gates both on the file + // existing, so a caller that prints this note has already found one. Kept rather + // than deleted: the note's job is to describe writeSettings truthfully for any + // path handed to it, and its own test still exercises this arm directly. return unchanged + ".\n No backup is made — there is no existing file to copy.\n\n" } if _, err := os.Stat(bak); err == nil { @@ -912,6 +924,51 @@ func bobDuplicateKey(settingsPath string) error { return nil } +// bobNoDocument reports why the settings file cannot be treated as IBM Bob's, or nil. +// +// readSettings cannot answer this: it returns an empty map for a missing file, for an +// empty one, and for a bare `null` alike, so every one of the three reads back as "a +// settings document that happens to set nothing". They are not the same thing. IBM Bob +// writes this file the first time it stores a setting, so no document at all means Bob +// has not run here — most often a --settings typo, or the wrong machine. +// +// That matters because the write path only discovers it late: `null` reaches bobSetKey, +// which rejects a non-object top level, but by then enable has printed what it will set, +// prompted, and copied the file to .bak. Promising a write that cannot happen is the +// failure being closed, so this runs before anything is printed. +// +// It is deliberately NOT fixed inside readSettings, which claude-code shares: coercing +// null to an empty document is the right reading for a command that re-marshals the whole +// file, and changing it there would change that command's behaviour. +// +// An unreadable file is not this check's business — readSettings reports it, with its own +// wording, and "exists but cannot be read" is a different problem from "is not there". +func bobNoDocument(settingsPath string) error { + src, err := os.ReadFile(settingsPath) //nolint:gosec // operator-supplied path + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%s does not exist, so IBM Bob has not saved settings here", settingsPath) + } + return nil + } + if len(bytes.TrimSpace(src)) == 0 { + return fmt.Errorf("%s is empty, so IBM Bob has not saved settings here", settingsPath) + } + if bytes.Equal(bytes.TrimSpace(src), []byte("null")) { + return fmt.Errorf("%s holds only `null`, which sets nothing", settingsPath) + } + return nil +} + +// bobNotInstalled is the refusal enable and disable share. One wording for both, because +// the reason is the same and only the verb differs. +func bobNotInstalled(what string, reason error, stderr io.Writer) int { + fmt.Fprintf(stderr, "abctl: %v.\n"+ + " Nothing to %s. Start IBM Bob and change any setting so it writes the file,\n"+ + " or pass --settings with the path to a settings.json it does use.\n", reason, what) + return 1 +} + func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.Writer) int { want, cfg, err := wantedFromConfig(cortexCfgPath) if err != nil { @@ -934,6 +991,10 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W } proxy := want[envProxy] + if nerr := bobNoDocument(settingsPath); nerr != nil { + return bobNotInstalled("enable", nerr, stderr) + } + if derr := bobDuplicateKey(settingsPath); derr != nil { fmt.Fprintf(stderr, "abctl: %s: %v\n", settingsPath, derr) return 1 @@ -1033,6 +1094,10 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W } func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr io.Writer) int { + if nerr := bobNoDocument(settingsPath); nerr != nil { + return bobNotInstalled("disable", nerr, stderr) + } + if derr := bobDuplicateKey(settingsPath); derr != nil { fmt.Fprintf(stderr, "abctl: %s: %v\n", settingsPath, derr) return 1 @@ -1123,6 +1188,9 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr const ( bobStatusYes = "IBM Bob is configured to use the Cortex proxy" bobStatusNo = "IBM Bob is *NOT* configured to use the Cortex proxy" + // A third answer, not a flavour of bobStatusNo: "not configured" is a claim about + // Bob's settings, and with no settings document there is nothing to make it about. + bobStatusUnknown = "IBM Bob's Cortex status is unknown" ) func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io.Writer) int { @@ -1135,6 +1203,16 @@ func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io. // claudeCodeStatus and bobShellStatus make. A non-zero status here would make // `abctl configure bob status` unusable in a shell conditional for anything but // "is it on". + // No document means no answer. The verdict this used to print was bobStatusNo with + // `"http.proxy" is unset` under it — a positive report about a file that is not + // there, indistinguishable from a real Bob that simply is not routed through Cortex. + // Saying so is the difference between "Bob is not configured" and "abctl cannot tell + // whether Bob is configured", and only the second is true here. + if nerr := bobNoDocument(settingsPath); nerr != nil { + fmt.Fprintf(stdout, "%s\n %v\n", bobStatusUnknown, nerr) + return 0 + } + if derr := bobDuplicateKey(settingsPath); derr != nil { // Two values, and no way to say which one Bob uses without reimplementing its // precedence. Reporting either as "the setting" would be a guess, so the diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 628b343b1..23973267c 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -1790,3 +1790,193 @@ func TestBobDisable_RefusesToDeleteAGuessUnattended(t *testing.T) { }) } } + +// bobSetKey has two ways to write a key: splice a new member in, or replace the span of +// one already there. Every other enable test takes the insert path, because a settings +// file that already names http.proxy is either identical to what we would write (so +// enable stops at "Already enabled") or foreign (so it refuses). Replacing needs the +// narrow middle: ours, and spelled differently. +// +// "localhost:47600" in the file against the "127.0.0.1:47600" the config derives is +// exactly that — bobOwns calls it ours, the strings differ, so enable falls through to +// a write over an existing member. Reached by no other test in this file: replacing the +// branch body with panic() leaves the whole suite green. +func TestBobEnable_ReplacesADifferentSpellingOfTheSameListener(t *testing.T) { + settings, cfg := fixture(t, `{ + "editor.fontSize": 13, + "http.proxy": "http://localhost:47600", + "http.proxyAuthorization": "keep-me" +}`) + noPrompt(t, true) + + var out, errb bytes.Buffer + if code := bobEnable(settings, cfg, true, &out, &errb); code != 0 { + t.Fatalf("exit %d: %s", code, errb.String()) + } + // Not "Already enabled": the values differ, so this must be a write. + if strings.Contains(out.String(), "Already enabled") { + t.Errorf("treated a differing spelling as already correct:\n%s", out.String()) + } + + doc := bobDoc(t, settings) + if got := doc[bobProxyKey]; got != "http://127.0.0.1:47600" { + t.Errorf("%s = %v, want the config's spelling", bobProxyKey, got) + } + // The whole point of replacing rather than inserting: one member, not two. A + // splice that appended instead would leave the key named twice, which bobFindMember + // then refuses outright on the next run — so this also pins that enable stays + // re-runnable. + if n := strings.Count(mustRead(t, settings), `"http.proxy"`); n != 1 { + t.Errorf("the key appears %d times, want 1", n) + } + // Its neighbours must survive, and so must its position: a replace that dropped + // the surrounding spans would take these with it. + for _, want := range []string{`"editor.fontSize": 13`, `"http.proxyAuthorization": "keep-me"`} { + if !strings.Contains(mustRead(t, settings), want) { + t.Errorf("lost %s:\n%s", want, mustRead(t, settings)) + } + } +} + +// mustRead returns the file's bytes as a string, for the assertions that are about +// layout rather than about the parsed document. +func mustRead(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +// No settings document means IBM Bob has not saved settings here, and all three verbs +// must say so rather than act. +// +// The three shapes are one test because readSettings maps every one of them to the same +// empty map, which is what made this worth fixing: before bobNoDocument, `null` and a +// missing file produced byte-identical output, and enable on `null` printed what it +// would set, prompted, wrote a .bak, and only then failed inside bobSetKey. +// +// Subtest names deliberately carry no flag spelling. bobNotInstalled interpolates +// settingsPath into its message, t.TempDir() names its directory after the subtest, so a +// name containing "--yes" would make an assertion on that string match the path instead +// of the prose — which has already silently defeated one mutation in this file's history. +func TestBobVerbs_RefuseWithoutASettingsDocument(t *testing.T) { + for _, tc := range []struct { + name, body string + write bool + }{ + {name: "absent", write: false}, + {name: "empty", body: "", write: true}, + {name: "only the null literal", body: "null\n", write: true}, + } { + t.Run(tc.name, func(t *testing.T) { + // fixture skips the write on "", which is the absent case; the other two + // need the file to exist, so they are written here. + settings, cfg := fixture(t, "") + if tc.write { + if err := os.WriteFile(settings, []byte(tc.body), 0o600); err != nil { + t.Fatal(err) + } + } + + for _, verb := range []struct { + what string + run func(stdout, stderr io.Writer) int + }{ + {"enable", func(o, e io.Writer) int { return bobEnable(settings, cfg, true, o, e) }}, + {"disable", func(o, e io.Writer) int { + want, ca := bobWanted(cfg, t.TempDir()) + return bobDisable(settings, want, ca, true, o, e) + }}, + } { + t.Run(verb.what, func(t *testing.T) { + asked := noPrompt(t, true) + var out, errb bytes.Buffer + if code := verb.run(&out, &errb); code != 1 { + t.Errorf("exit = %d, want 1\n%s%s", code, out.String(), errb.String()) + } + // The refusal must name the verb it is refusing, so the two are + // not one message with the wrong word in it. + if !strings.Contains(errb.String(), "Nothing to "+verb.what) { + t.Errorf("does not say what it will not do: %q", errb.String()) + } + // Naming the way out, on both routes: make Bob write the file, or + // point at one it does use. + for _, want := range []string{"Start IBM Bob", "--settings"} { + if !strings.Contains(errb.String(), want) { + t.Errorf("the refusal omits %q: %q", want, errb.String()) + } + } + // The failure being closed: no prompt, and no .bak. Promising a + // write and then failing inside the writer is the exact shape + // this gate exists to prevent. + if *asked != 0 { + t.Errorf("prompted %d times before refusing", *asked) + } + if _, err := os.Stat(settings + ".bak"); err == nil { + t.Error("wrote a .bak for a refusal") + } + // An error is not an answer: nothing on stdout. + if out.Len() != 0 { + t.Errorf("stdout not empty: %q", out.String()) + } + }) + } + + t.Run("status", func(t *testing.T) { + var out bytes.Buffer + // Exit 0: reporting that it cannot tell is a successful report, the + // same rule the other two status verdicts follow. + want, ca := bobWanted(cfg, t.TempDir()) + if code := bobStatus(settings, cfg, want, ca, &out); code != 0 { + t.Errorf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), bobStatusUnknown) { + t.Errorf("does not report the status as unknown:\n%s", out.String()) + } + // The distinction the whole change turns on: "unknown" must not be + // dressed as a verdict about Bob's settings. Both of the other two + // answers are claims this run cannot make. + for _, wrong := range []string{bobStatusYes, bobStatusNo} { + if strings.Contains(out.String(), wrong) { + t.Errorf("also printed a verdict it cannot support (%q):\n%s", wrong, out.String()) + } + } + }) + }) + } +} + +// The three document shapes must not be reported identically. Their being +// indistinguishable is what made the old behaviour hard to see: a missing --settings +// path and a null document produced the same output, so a typo looked like a working +// run against an unconfigured Bob. +func TestBobStatus_DistinguishesWhyThereIsNoDocument(t *testing.T) { + seen := map[string]string{} + for _, tc := range []struct { + name, body string + write bool + }{ + {name: "absent", write: false}, + {name: "empty", body: "", write: true}, + {name: "only the null literal", body: "null\n", write: true}, + } { + settings, cfg := fixture(t, "") + if tc.write { + if err := os.WriteFile(settings, []byte(tc.body), 0o600); err != nil { + t.Fatal(err) + } + } + var out bytes.Buffer + want, ca := bobWanted(cfg, t.TempDir()) + bobStatus(settings, cfg, want, ca, &out) + // The path varies per subtest, so compare only the reason line's prose. + reason := strings.TrimSpace(strings.TrimPrefix(out.String(), bobStatusUnknown)) + reason = strings.TrimPrefix(reason, settings) + if prev, dup := seen[reason]; dup { + t.Errorf("%s reports identically to %s: %q", tc.name, prev, reason) + } + seen[reason] = tc.name + } +} From 994ee45daa9488a9e297c8736d4675010c223deb Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 21:16:41 -0400 Subject: [PATCH 7/8] Fix: Address round-6 review on configure bob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from review. bobSpliceOut panicked on valid JSON. It measured the member's line end forward from the KEY, so on `"http.proxy":\n "http://..."` — valid JSON, and what VS Code's own formatter produces on a long value — the first newline it found was inside the member, giving a line end below the value's end: `slice bounds out of range [79:46]`. `configure bob disable` died on a settings file that was never malformed, after printing that it was keeping a .bak (and having created it). Indexed from the value's end instead. Two exact-byte table rows cover it, straddling-and-last separately because the preceding-comma arm is reached after the crash site. Three comments credited writeSettings for the backup and atomic rename that bobWriteKey actually does. Corrected; the five references that name writeSettings as a contrast are left alone. README said the same thing and now names no internal function at all — verified by running both verbs with no tty: each prints the change and the .bak line before prompting, and a declined run leaves no .bak. Status told the user to run `enable` on a drifted loopback value, which enable refuses with exit 1 — ownership is an exact host+port match, so a moved port is bobNotOurs, and the arm that falls through to the write is a differing spelling of the same address. Advice replaced with the two steps that work. Pinned as behaviour, not wording: the test runs enable on the value status reported and only objects if status offered a bare `enable` as the fix, so it also stops objecting if enable is ever widened to claim such a value. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/README.md | 5 ++-- cmd/abctl/cmd_bob.go | 27 ++++++++++++++----- cmd/abctl/cmd_bob_test.go | 55 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index d7e443fab..ef026adde 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -538,8 +538,9 @@ one, and a bool would have to guess. Guessing "not ours" toward a `delete` is th dangerous direction, so it is not a bool. `status` reports "cannot tell" in those words rather than ruling on it. `disable` asks before removing such a value and refuses under `--yes`, since `--yes` means "do not ask me", not "decide for me". -`enable` refuses rather than overwriting anything it does not own. Whatever is -removed, `writeSettings` has already kept the file as a `.bak`. +`enable` refuses rather than overwriting anything it does not own. Either verb +prints exactly what it will do to the file, and what it will leave beside it, +before it does it. **Whether anything is listening is a separate question**, reported on its own line. A stopped Cortex is the normal state of a laptop and is not a verdict on the diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index 4e07ce472..b879683ef 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -627,11 +627,18 @@ func bobSpliceOut(src []byte, start, end int) []byte { // left `{\n "http.proxy": ..., "b": 2,\n` as `{\n "b": 2,` — the indent gone // and one stray space in its place. lineStart := bytes.LastIndexByte(src[:start], '\n') + 1 - lineEnd := bytes.IndexByte(src[start:], '\n') + // Scanned from end, the end of the VALUE, not from start, the start of the key. A + // member may straddle a line break — `"http.proxy":\n "http://..."` is valid + // JSON and VS Code's own formatter produces it on a long value — and then the first + // newline after the key is INSIDE the member. Measuring from start there yields a + // lineEnd below end, and the src[end:lineEnd] slice below panicked on a file that + // was never malformed: `slice bounds out of range [79:46]`, from `configure bob + // disable`, after it had already printed that it was keeping a .bak. + lineEnd := bytes.IndexByte(src[end:], '\n') if lineEnd < 0 { lineEnd = len(src) } else { - lineEnd += start + lineEnd += end } ownLine := len(bytes.TrimSpace(src[lineStart:start])) == 0 && bobTailIsOnlySeparator(src[end:lineEnd]) @@ -855,7 +862,7 @@ func bobWriteKey(path, key string, value any) error { // bobBackupNote describes what this particular write will and will not preserve. // -// Three different true statements, because writeSettings makes three different +// Three different true statements, because bobWriteKey makes three different // choices and the message used to claim only the first. It writes .bak from // the file's current contents ONLY when the file exists AND no .bak is there // already — never overwriting, because a second run would otherwise replace the @@ -885,7 +892,7 @@ func bobBackupNote(settingsPath string) string { // // Unreachable from enable and disable since bobNoDocument gates both on the file // existing, so a caller that prints this note has already found one. Kept rather - // than deleted: the note's job is to describe writeSettings truthfully for any + // than deleted: the note's job is to describe bobWriteKey truthfully for any // path handed to it, and its own test still exercises this arm directly. return unchanged + ".\n No backup is made — there is no existing file to copy.\n\n" } @@ -1084,7 +1091,7 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W fmt.Fprintf(stdout, "\nEnabled. %q in %s now points at Cortex — this is the setting\n"+ "VS Code forks read for their own networking and their extension host.\n\n", bobProxyKey, settingsPath) - // Restart, rather than a claim either way about live pickup: writeSettings is + // Restart, rather than a claim either way about live pickup: bobWriteKey is // temp+rename so Bob never sees a half-written file, but whether Bob re-reads a // proxy change without restarting is not something this command has verified. fmt.Fprint(stdout, "Restart Bob so it re-reads its settings.\n\n") @@ -1270,7 +1277,15 @@ func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io. // leaves it alone. Reporting drift is useful; deleting on a guess is not. add("%q=%s in %s", bobProxyKey, existing, settingsPath) add("that is a local proxy, but %s now names %s", cortexCfgPath, wantProxy) - add("run `abctl configure bob enable` to move it") + // NOT "run enable to move it", which is what this line said and what no + // user could do: ownership is an exact host+port match, so enable sees a + // drifted port as bobNotOurs and refuses it with exit 1 — the one arm that + // falls through to the write is a differing SPELLING of the same address + // (localhost vs 127.0.0.1), not a different port. Both halves of that + // refusal are deliberate, so the advice is what changes: name the two + // steps that do work, in the order they work in. + add("to move it: change that value to %s by hand, or remove it and run", wantProxy) + add("`abctl configure bob enable`") default: add("%q=%s in %s", bobProxyKey, existing, settingsPath) add("that is not this machine's Cortex proxy, which %s puts at %s", diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 23973267c..89b0336d5 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -283,6 +283,28 @@ func TestBobDisable_LeavesNoStrayWhitespace(t *testing.T) { in: "{\n \"a\": 1,\n \"b\": 2, \"http.proxy\": \"http://127.0.0.1:47600\"\n}\n", want: "{\n \"a\": 1,\n \"b\": 2\n}\n", }, + { + // The member STRADDLES a line break: valid JSON, and what VS Code's own + // formatter produces when the value is long enough to wrap. This row is not + // about whitespace — it is about not crashing. The splicer measured the + // member's line end forward from the KEY, so the first newline it found was + // the one inside the member, giving a line end BELOW the value's end and a + // `slice bounds out of range [79:46]` panic. `configure bob disable` died on + // a settings file that was never malformed, after printing that it was + // keeping a .bak. + name: "value on the line below its key", + in: "{\n \"a\": 1,\n \"http.proxy\":\n \"http://127.0.0.1:47600\",\n \"b\": 2\n}\n", + want: "{\n \"a\": 1,\n \"b\": 2\n}\n", + }, + { + // Straddling AND last, so the preceding-comma arm runs on a member whose + // own span covers a newline. Separate row because the panic was in code + // reached before that arm, so a crash fixed only for the following-comma + // case would still show up here. + name: "value on the line below its key, and last", + in: "{\n \"a\": 1,\n \"http.proxy\":\n \"http://127.0.0.1:47600\"\n}\n", + want: "{\n \"a\": 1\n}\n", + }, } { t.Run(tc.name, func(t *testing.T) { settings, cfg := fixture(t, tc.in) @@ -805,9 +827,9 @@ func TestBobStatus_WarnsOnlyForAProxyItClaims(t *testing.T) { {"ours and nothing listening", `{"http.proxy": "http://127.0.0.1:` + dead + `"}`, true}, // Not ours: silent. Judging someone else's proxy is out of scope. {"foreign", `{"http.proxy": "http://proxy.corp.example.com:3128"}`, false}, - // Drifted but still loopback: silent too. The actionable advice is `enable`, - // which the detail lines give; a liveness complaint about the stale port on top - // of it is noise about a value abctl is already telling the user to replace. + // Drifted but still loopback: silent too. The detail lines already tell the + // user how to replace the value (see TestBobStatus_ReportsPortDrift), so a + // liveness complaint about the stale port on top of that is noise. {"drifted port", `{"http.proxy": "http://127.0.0.1:47699"}`, false}, // Unset: there is no address to probe, so there is nothing to warn about. {"unset", `{"editor.fontSize": 13}`, false}, @@ -856,6 +878,33 @@ func TestBobStatus_ReportsPortDrift(t *testing.T) { if !strings.Contains(out.String(), "47699") || !strings.Contains(out.String(), "47600") { t.Errorf("drift report names neither the stale value nor the current one:\n%s", out.String()) } + + // And the advice must be followable. This arm used to say "run `abctl configure + // bob enable` to move it" full stop, which enable answers with exit 1: a drifted + // PORT is bobNotOurs, and the arm that falls through to the write is a differing + // spelling of the same address, not a different one. So status was sending the + // user to a command that refuses. + // + // Pinned as behaviour rather than as wording: run enable on the very value status + // just reported, and if it refuses, require that status did not offer a bare + // `enable` as the way out. That keeps passing through a rewording and fails again + // if either side moves — including if someone later lets enable claim a drifted + // value, in which case the bare advice becomes true and this stops objecting. + var enableOut, enableErr bytes.Buffer + noPrompt(t, true) // enable must not block on a prompt if it gets that far + if code := bobEnable(settings, cfg, true, &enableOut, &enableErr); code != 0 { + // enable refuses, so the detail lines must not present it as the whole fix. + // The bare sentence is the exact shape that was wrong. + if strings.Contains(out.String(), "run `abctl configure bob enable` to move it") { + t.Errorf("status sends the user to enable, which exits %d on that value:\nstatus:\n%s\nenable stderr:\n%s", + code, out.String(), enableErr.String()) + } + // Whatever it says instead has to name the value to move to, or the user is + // left guessing which address is current. + if !strings.Contains(out.String(), "by hand") && !strings.Contains(out.String(), "remove it") { + t.Errorf("status offers no followable way to fix the drift:\n%s", out.String()) + } + } } // A declined prompt is the user saying no, and it must cost nothing. From 91b1b908d3bc4d2c5b4bd72430404ce5695ada88 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 27 Sep 2026 22:00:06 -0400 Subject: [PATCH 8/8] Fix: Name the config path in bob disable's shape-only messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `disable` judges an existing `http.proxy` by shape alone when the Cortex config cannot be read. Both messages that say so — the `--yes` refusal and the interactive caveat — said "the Cortex config could not be read" without naming which file, so a typo'd `--config` and an uninstalled Cortex read identically, and only the first is worth retrying. `bobStatus` already named it; `bobDisable` did not have the path. Thread `cortexCfgPath` through and name it in both. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- cmd/abctl/README.md | 10 ++--- cmd/abctl/cmd_bob.go | 81 +++++++++++++++++++++++++---------- cmd/abctl/cmd_bob_test.go | 90 +++++++++++++++++++++++++++++++-------- 3 files changed, 136 insertions(+), 45 deletions(-) diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index ef026adde..4257c2de2 100644 --- a/cmd/abctl/README.md +++ b/cmd/abctl/README.md @@ -509,11 +509,11 @@ the usual Debian and Fedora routes and saying plainly that the exact step depends on the distribution. It is `ca.crt` — the single bridge CA — and deliberately **not** the -`bundle.crt` in the same directory, which holds ~129 certificates and exists for -tools whose CA setting *replaces* the trust store (`SSL_CERT_FILE` and friends, -as `abctl exec` sets). The keychain is additive, so `-r trustRoot` on the bundle -would install explicit machine-wide root trust for ~128 unrelated public CAs, -and the undo above would not take it back. +`bundle.crt` in the same directory, which holds the bridge CA *plus* every +platform root and exists for tools whose CA setting *replaces* the trust store +(`SSL_CERT_FILE` and friends, as `abctl exec` sets). The keychain is additive, +so `-r trustRoot` on the bundle would install explicit machine-wide root trust +for every public CA in it, and the undo above would not take that back. ### What it knows, and what it does not diff --git a/cmd/abctl/cmd_bob.go b/cmd/abctl/cmd_bob.go index b879683ef..52d81617e 100644 --- a/cmd/abctl/cmd_bob.go +++ b/cmd/abctl/cmd_bob.go @@ -188,7 +188,7 @@ func runBob(args []string, stdout, stderr io.Writer) int { return bobEnable(*settingsPath, *cortexCfgPath, yes, stdout, stderr) case "disable": wantProxy, caPath := bobWanted(*cortexCfgPath, home) - return bobDisable(*settingsPath, wantProxy, caPath, yes, stdout, stderr) + return bobDisable(*settingsPath, *cortexCfgPath, wantProxy, caPath, yes, stdout, stderr) default: wantProxy, caPath := bobWanted(*cortexCfgPath, home) return bobStatus(*settingsPath, *cortexCfgPath, wantProxy, caPath, stdout) @@ -385,10 +385,11 @@ func bobProxyIsListening(val string) bool { // bundle.crt is the file most of abctl's other CA messages name. The keychain is // ADDITIVE, so it wants one certificate; bundle.crt exists for the tools whose CA // setting REPLACES their trust store, and it holds the bridge CA followed by -// every platform root (129 certificates on this machine). -// `add-trusted-cert -r trustRoot` on that file would install explicit root-trust -// settings for ~128 unrelated public CAs machine-wide, which is both far broader -// than intended and not undone by the single delete-certificate below. See +// every platform root. `add-trusted-cert -r trustRoot` on that file would install +// explicit root-trust settings for every public CA in it machine-wide — a count +// that varies by machine and by platform, which is the reason not to name one — +// and that is both far broader than intended and not undone by the single +// delete-certificate below. See // core/tlsbridge/bundle.go's header, which states which file is for which job. func bobTrustNote(caPath string) string { q := shellQuote(caPath) @@ -544,6 +545,12 @@ func bobSetKey(src []byte, key string, value any) ([]byte, error) { return bobInsertMember(src, member) } +// errBobDuplicateKey marks the one bobFindMember failure its callers act on: the same +// top-level key present twice. Every other failure is a tokenizer error that readSettings +// describes better. A sentinel rather than a substring of the message, so rewording the +// sentence cannot quietly disable the gate in bobDuplicateKey. +var errBobDuplicateKey = errors.New("duplicate top-level key") + // bobFindMember locates the byte span of a top-level member, from the opening quote // of its name through the last byte of its value. Offsets come from json.Decoder, // so they are the tokenizer's view of the document and not a textual guess. @@ -583,9 +590,12 @@ func bobFindMember(src []byte, key string) (start, end int, found bool, err erro } if name == key { if found { - return 0, 0, false, fmt.Errorf("%q appears more than once at the top level; "+ - "remove the duplicate first — this command edits one member and cannot "+ - "say which of them wins", key) + // Wrapped, not just worded: bobDuplicateKey classifies this verdict and + // used to do it by searching the message text, so rewording the sentence + // below silently turned that gate off. errors.Is cannot drift that way. + return 0, 0, false, fmt.Errorf("%w: %q appears more than once at the top "+ + "level; remove the duplicate first — this command edits one member and "+ + "cannot say which of them wins", errBobDuplicateKey, key) } start, end, found = nameStart, int(dec.InputOffset()), true } @@ -710,6 +720,16 @@ func bobSpliceOut(src []byte, start, end int) []byte { if from > 0 && src[from-1] == ',' { from-- } + // Anything the user left between the value and the line's end goes too. It is + // the member's own tail, but walking back to the preceding comma moves the cut + // ABOVE it, so leaving it behind strands it on the line that survives: + // `"a": 1,\n "http.proxy": "..." \n}` became `"a": 1 \n}` — a trailing space + // on a line the user did not touch. Only spaces and tabs, and only as far as + // the newline: a following blank line is the user's own grouping, and the + // ownLine arm above already owns the newline case. + for to < len(src) && (src[to] == ' ' || src[to] == '\t') { + to++ + } } out := make([]byte, 0, len(src)-(to-from)) @@ -818,6 +838,13 @@ func bobWriteKey(path, key string, value any) error { } // No file yet: start from an empty object so the insert path has a document // to work on. Nothing to back up either. + // + // Unreachable from enable and disable, for the same reason bobBackupNote's + // no-backup arm is: bobNoDocument gates both verbs on the file existing and + // being non-empty, so a caller that gets here has already found one. Kept + // rather than deleted, on the same terms — bobWriteKey's contract is to edit + // whatever path it is handed, and dropping the arm would turn a first write + // into an error for any future caller that does not pre-check. src = []byte("{}\n") } else { // Back the file up ONCE and never overwrite it, matching writeSettings: a @@ -925,7 +952,7 @@ func bobDuplicateKey(settingsPath string) error { // Only the duplicate verdict is this check's to report. Any other parse failure is // readSettings' to describe, and bob's readSettings arm says more about it than a // tokenizer error would. - if ferr != nil && strings.Contains(ferr.Error(), "more than once") { + if ferr != nil && errors.Is(ferr, errBobDuplicateKey) { return ferr } return nil @@ -1100,7 +1127,7 @@ func bobEnable(settingsPath, cortexCfgPath string, yes bool, stdout, stderr io.W return 0 } -func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr io.Writer) int { +func bobDisable(settingsPath, cortexCfgPath, wantProxy, caPath string, yes bool, stdout, stderr io.Writer) int { if nerr := bobNoDocument(settingsPath); nerr != nil { return bobNotInstalled("disable", nerr, stderr) } @@ -1122,7 +1149,11 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr return 0 } s, isString := existing.(string) - if !isString || bobOwns(s, wantProxy) == bobNotOurs { + // One verdict, read three times below. bobOwns is pure and the inputs do not + // change between those reads, so the repetition was only an invitation to let + // two of them drift apart. + owns := bobOwns(s, wantProxy) + if !isString || owns == bobNotOurs { // Exit 0, not 1. "Remove it only if it points at Cortex" is the contract, and // this file already satisfies it — there is nothing for the user to fix, so // reporting a failure would be wrong. @@ -1143,26 +1174,31 @@ func bobDisable(settingsPath, wantProxy, caPath string, yes bool, stdout, stderr // scripted `disable --yes` would silently delete a stranger's proxy. Refuse // instead, and say which flag turns the guess into an answer. Exit 1, not 0: // the user asked for a removal that did not happen. - if yes && bobOwns(s, wantProxy) == bobUnknown { + if yes && owns == bobUnknown { + // Name the config path. Without it the message is the same whether Cortex is + // uninstalled or --config was a typo, and only the second is worth retrying — + // so the reader needs to see WHICH file was not read to tell them apart. + // bobStatus's equivalent arm already names it; this one did not. fmt.Fprintf(stderr, "abctl: %s sets %q to %q, which is shaped like a Cortex\n"+ - " proxy but cannot be confirmed as one: the Cortex config could not be read,\n"+ - " so there is no address to compare against — every loopback http proxy looks\n"+ - " like this. Refusing to delete it unattended. Re-run without --yes to see the\n"+ - " value and decide, or pass --config with a readable Cortex config.\n", - settingsPath, bobProxyKey, s) + " proxy but cannot be confirmed as one: %s could not be read, so there is no\n"+ + " address to compare against — every loopback http proxy looks like this.\n"+ + " Refusing to delete it unattended. Re-run without --yes to see the value and\n"+ + " decide, or pass --config with a readable Cortex config.\n", + settingsPath, bobProxyKey, s, cortexCfgPath) return 1 } fmt.Fprintf(stdout, "Removes from %s:\n %q: %q\n", settingsPath, bobProxyKey, s) - if bobOwns(s, wantProxy) == bobUnknown { + if owns == bobUnknown { // Say it is a guess, because it is: with no readable config there is no // address to compare against, and what is left is that the value is a // loopback http proxy — the shape abctl writes. Removing it is still the // right default (this is the uninstalled-Cortex case, when the off switch // matters most), but the user should know which of the two answers they are // getting, and the prompt below names the value before anything is written. - fmt.Fprintf(stdout, " (no readable config to compare against, so this is judged\n"+ - " by shape alone — a loopback proxy, which is what abctl writes)\n") + fmt.Fprintf(stdout, " (%s\n"+ + " is not readable, so this is judged by shape alone — a loopback\n"+ + " proxy, which is what abctl writes)\n", cortexCfgPath) } fmt.Fprint(stdout, bobBackupNote(settingsPath)) if !yes && !bobConfirm(settingsPath, "Write to", stdout) { @@ -1253,12 +1289,13 @@ func bobStatus(settingsPath, cortexCfgPath, wantProxy, caPath string, stdout io. case nil: add("%q is unset in %s", bobProxyKey, settingsPath) case string: + owns := bobOwns(existing, wantProxy) switch { - case bobOwns(existing, wantProxy) == bobOurs: + case owns == bobOurs: verdict = bobStatusYes listening = existing add("%q=%s in %s", bobProxyKey, existing, settingsPath) - case bobOwns(existing, wantProxy) == bobUnknown: + case owns == bobUnknown: listening = existing // The value cannot be judged without something to compare it against, and // claiming "not a Cortex proxy" here would be a statement about the diff --git a/cmd/abctl/cmd_bob_test.go b/cmd/abctl/cmd_bob_test.go index 89b0336d5..72e64ca5e 100644 --- a/cmd/abctl/cmd_bob_test.go +++ b/cmd/abctl/cmd_bob_test.go @@ -194,7 +194,7 @@ func TestBobEnableDisable_RoundTripsByteForByte(t *testing.T) { t.Fatalf("enable: exit %d: %s", code, errb.String()) } proxy, caPath := bobWanted(cfg, t.TempDir()) - if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, proxy, caPath, true, &out, &errb); code != 0 { t.Fatalf("disable: exit %d: %s", code, errb.String()) } @@ -305,13 +305,53 @@ func TestBobDisable_LeavesNoStrayWhitespace(t *testing.T) { in: "{\n \"a\": 1,\n \"http.proxy\":\n \"http://127.0.0.1:47600\"\n}\n", want: "{\n \"a\": 1\n}\n", }, + { + // A space between the value and the newline, on a LAST member. The + // preceding-comma arm walks the cut back over the comma above, which moves + // it ABOVE this space — so the space belongs to a member that is gone and + // lands on the line that survives: `"a": 1,` became `"a": 1 `, trailing + // whitespace on a line the user never touched. + // + // Not the trigger the review reported, which was whitespace before the + // COLON; the "whitespace everywhere a colon allows it" row below is that + // layout, and it was already clean. The two are worth keeping apart, since + // only one of them can reach this arm. + name: "space between the value and the newline, last member", + in: "{\n \"a\": 1,\n \"http.proxy\": \"http://127.0.0.1:47600\" \n}\n", + want: "{\n \"a\": 1\n}\n", + }, + { + // Same arm, tab instead of space: both are what an editor leaves behind, and + // a fix written against ' ' alone would pass the row above and fail here. + name: "tab between the value and the newline, last member", + in: "{\n \"a\": 1,\n \"http.proxy\": \"http://127.0.0.1:47600\"\t\n}\n", + want: "{\n \"a\": 1\n}\n", + }, + { + // The bound on that absorption, in the opposite direction. Widening it to + // every whitespace byte — isBobSpace, which includes '\n' — swallows the + // newline as well and welds the closing brace onto the surviving line: + // `"a": 1}`. Only the horizontal run, only as far as the line's end. + name: "space then a blank line, last member", + in: "{\n \"a\": 1,\n \"http.proxy\": \"http://127.0.0.1:47600\" \n\n}\n", + want: "{\n \"a\": 1\n\n}\n", + }, + { + // The layout the review named, pinned as already-correct rather than fixed: + // whitespace before the colon is inside the member's own span, so the splice + // takes it with the member and never strands anything. Kept so a future + // change to this arm cannot break it silently. + name: "whitespace before the colon", + in: "{\n \"a\": 1,\n \"http.proxy\" : \"http://127.0.0.1:47600\"\n}\n", + want: "{\n \"a\": 1\n}\n", + }, } { t.Run(tc.name, func(t *testing.T) { settings, cfg := fixture(t, tc.in) proxy, caPath := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, proxy, caPath, true, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } @@ -350,7 +390,7 @@ func TestBobDisable_IgnoresTheKeyNameInsideValuesAndNesting(t *testing.T) { proxy, caPath := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, proxy, caPath, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, proxy, caPath, true, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } @@ -601,7 +641,7 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { settings, cfg := fixture(t, `{"editor.fontSize": 13, "http.proxy": "http://127.0.0.1:47600"}`) want, ca := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } doc := bobDoc(t, settings) @@ -621,7 +661,7 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { t.Fatal(err) } var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0 — a foreign proxy is not an error", code) } after, err := os.ReadFile(settings) @@ -637,7 +677,7 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { settings, cfg := fixture(t, `{"editor.fontSize": 13}`) want, ca := bobWanted(cfg, t.TempDir()) var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0", code) } if !strings.Contains(out.String(), "Not enabled") { @@ -653,7 +693,7 @@ func TestBobDisable_RemovesOnlyOurs(t *testing.T) { t.Fatal(err) } var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0", code) } after, err := os.ReadFile(settings) @@ -716,7 +756,7 @@ func TestBobDisable_WithUnreadableConfig(t *testing.T) { // unattended one: without it, a fix that dropped the prompt entirely would pass. asked := noPrompt(t, true) var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, false, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, false, &out, &errb); code != 0 { t.Fatalf("exit %d: %s", code, errb.String()) } if *asked != 1 { @@ -728,6 +768,13 @@ func TestBobDisable_WithUnreadableConfig(t *testing.T) { if !strings.Contains(out.String(), "shape alone") { t.Errorf("disable did not disclose that it judged by shape:\n%s", out.String()) } + // And WHICH file it could not read. "no readable config to compare against" was + // the same sentence whether Cortex is uninstalled or --config was a typo, and only + // the second is worth retrying — so the path is the part that tells them apart. + // bobStatus's equivalent arm already named it; this one did not. + if !strings.Contains(out.String(), cfg) { + t.Errorf("disable did not name the config it could not read (%s):\n%s", cfg, out.String()) + } } // Status reports and never acts: three states, all exit 0, nothing written. @@ -943,7 +990,7 @@ func TestBob_DeclinedPromptWritesNothing(t *testing.T) { var out, errb bytes.Buffer want, ca := bobWanted(cfg, t.TempDir()) - if code := bobDisable(settings, want, ca, false, &out, &errb); code != exitDeclined { + if code := bobDisable(settings, cfg, want, ca, false, &out, &errb); code != exitDeclined { t.Fatalf("exit = %d, want %d", code, exitDeclined) } after, err := os.ReadFile(settings) @@ -1045,10 +1092,10 @@ func TestBob_TrustMessageQuotesPaths(t *testing.T) { // The request named bundle.crt; this implementation deliberately prints ca.crt. // -// bundle.crt holds ~129 certificates and exists only for tools whose CA setting -// REPLACES the trust store. The macOS keychain is additive, so +// bundle.crt holds the bridge CA plus every platform root, and exists only for tools +// whose CA setting REPLACES the trust store. The macOS keychain is additive, so // `add-trusted-cert -r trustRoot` on the bundle would install machine-wide explicit -// root trust for ~128 unrelated public CAs — far broader than asked, and one +// root trust for every public CA in it — far broader than asked, and one // `delete-certificate -c authbridge-tls-bridge-ca` would not reverse it. This pins the // deviation so it cannot be "corrected" back to the request's wording. func TestBob_TrustMessageNamesCaCrtNotBundle(t *testing.T) { @@ -1066,7 +1113,7 @@ func TestBob_TrustMessageNamesCaCrtNotBundle(t *testing.T) { continue // prose may mention bundle.crt to explain the choice } if strings.Contains(line, "bundle.crt") { - t.Errorf("%s note runs security against the 129-cert bundle:\n%s", name, line) + t.Errorf("%s note runs security against the whole-trust-store bundle:\n%s", name, line) } } } @@ -1256,7 +1303,7 @@ func TestBob_RefusesADuplicateProxyKey(t *testing.T) { case "enable": code = bobEnable(settings, cfg, true, &out, &errb) case "disable": - code = bobDisable(settings, want, ca, true, &out, &errb) + code = bobDisable(settings, cfg, want, ca, true, &out, &errb) case "status": code = bobStatus(settings, cfg, want, ca, &out) } @@ -1351,7 +1398,7 @@ func TestBobDisable_LeavesACredentialBearingProxyAlone(t *testing.T) { } var out, errb bytes.Buffer - if code := bobDisable(settings, want, ca, true, &out, &errb); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &out, &errb); code != 0 { t.Fatalf("exit = %d, want 0 — a foreign proxy is not an error: %s", code, errb.String()) } @@ -1425,7 +1472,7 @@ func TestBob_EnableDisableRoundTripOnANon476xxPort(t *testing.T) { // the address enable derived it from. want, ca := bobWanted(cfg, t.TempDir()) var disOut, disErr bytes.Buffer - if code := bobDisable(settings, want, ca, true, &disOut, &disErr); code != 0 { + if code := bobDisable(settings, cfg, want, ca, true, &disOut, &disErr); code != 0 { t.Fatalf("disable exit %d: %s", code, disErr.String()) } if _, ok := bobDoc(t, settings)[bobProxyKey]; ok { @@ -1761,7 +1808,7 @@ func TestBobDisable_RefusesToDeleteAGuessUnattended(t *testing.T) { asked := noPrompt(t, true) var out, errb bytes.Buffer - code := bobDisable(settings, want, ca, tc.yes, &out, &errb) + code := bobDisable(settings, cfg, want, ca, tc.yes, &out, &errb) if code != tc.wantCode { t.Errorf("exit = %d, want %d\nstdout: %s\nstderr: %s", code, tc.wantCode, out.String(), errb.String()) @@ -1817,6 +1864,13 @@ func TestBobDisable_RefusesToDeleteAGuessUnattended(t *testing.T) { if !strings.Contains(errb.String(), tc.settings) { t.Errorf("the refusal does not name the value it declined to delete: %q", errb.String()) } + // Naming the config path, for the same reason the interactive caveat + // does: "the Cortex config could not be read" does not say which file, + // so a typo'd --config and an uninstalled Cortex read identically. + // Only rows that got here through an unreadable config can assert it. + if !tc.readableConfig && !strings.Contains(errb.String(), cfg) { + t.Errorf("the refusal does not name the config it could not read (%s): %q", cfg, errb.String()) + } // Naming the way out is the difference between a refusal and a dead // end. Both routes are asserted: drop --yes, or supply a config. for _, want := range []string{"--yes", "--config"} { @@ -1936,7 +1990,7 @@ func TestBobVerbs_RefuseWithoutASettingsDocument(t *testing.T) { {"enable", func(o, e io.Writer) int { return bobEnable(settings, cfg, true, o, e) }}, {"disable", func(o, e io.Writer) int { want, ca := bobWanted(cfg, t.TempDir()) - return bobDisable(settings, want, ca, true, o, e) + return bobDisable(settings, cfg, want, ca, true, o, e) }}, } { t.Run(verb.what, func(t *testing.T) {