diff --git a/cmd/abctl/README.md b/cmd/abctl/README.md index 28176c35c..4257c2de2 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,140 @@ 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. + +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 +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 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. + +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 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 + +"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, so ownership is +re-decided from the value each time. + +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. 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 +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 +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. + +`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, +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..52d81617e --- /dev/null +++ b/cmd/abctl/cmd_bob.go @@ -0,0 +1,1354 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "net" + "net/url" + "os" + "path/filepath" + "runtime" + "strconv" + "strings" + "time" + + "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": + wantProxy, caPath := bobWanted(*cortexCfgPath, home) + return bobDisable(*settingsPath, *cortexCfgPath, wantProxy, caPath, yes, stdout, stderr) + default: + wantProxy, caPath := bobWanted(*cortexCfgPath, home) + return bobStatus(*settingsPath, *cortexCfgPath, wantProxy, caPath, 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 +} + +// 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 +// "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. +// +// 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. +// +// 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 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 + } + // 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. + 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 + } + return loopback(a) && loopback(b) +} + +// bobIsLoopbackProxy reports whether val is an http proxy on this machine. +// +// 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" { + 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 + } + _ = c.Close() + return true +} + +// 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. `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) + 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 { + q := shellQuote(caPath) + if runtime.GOOS == "darwin" { + // 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" + + " 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" + + " 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 +// 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 +} + +// 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) +} + +// 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. +// +// 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() + 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 { + if found { + // 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 + } + } + 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 +// 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 — 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 + // 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 += end + } + 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 + 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 — 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'); ownLine && nl >= 0 && len(bytes.TrimSpace(src[to:to+nl])) == 0 { + to += nl + 1 + } else if !ownLine { + // 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 + // 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-- + } + // 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)) + 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? 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 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. + // + // 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 + // 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) +} + +// bobBackupNote describes what this particular write will and will not preserve. +// +// 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 +// 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 + // 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 { + // 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 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" + } + 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" +} + +// 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 && errors.Is(ferr, errBobDuplicateKey) { + return ferr + } + 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 { + 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] + + 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 + } + + 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 + } + // 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. + 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 + } + // 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. + 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.Fprint(stdout, bobBackupNote(settingsPath)) + if !yes && !bobConfirm(settingsPath, "Write to", stdout) { + fmt.Fprintln(stdout, "Not changed.") + return exitDeclined + } + + if werr := bobWriteKey(settingsPath, bobProxyKey, proxy); 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: 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") + fmt.Fprint(stdout, bobTrustNote(caPath)) + fmt.Fprintf(stdout, "Undo with: abctl configure bob disable\n") + return 0 +} + +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) + } + + 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) + 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) + // 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. + 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 + } + + // 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 && 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: %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 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, " (%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) { + fmt.Fprintln(stdout, "Not changed.") + return exitDeclined + } + + // 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 + } + + 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 +} + +// 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" + // 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 { + // 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". + // 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 + // 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 + // 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: + add("%q is unset in %s", bobProxyKey, settingsPath) + case string: + owns := bobOwns(existing, wantProxy) + switch { + case owns == bobOurs: + verdict = bobStatusYes + listening = existing + add("%q=%s in %s", bobProxyKey, existing, settingsPath) + 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 + // 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 == "": + 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 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) + // 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", + cortexCfgPath, wantProxy) + } + default: + 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 != "" { + 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..72e64ca5e --- /dev/null +++ b/cmd/abctl/cmd_bob_test.go @@ -0,0 +1,2085 @@ +package main + +import ( + "bytes" + "encoding/json" + "io" + "net" + "os" + "path/filepath" + "runtime" + "strconv" + "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. +// 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, cfg, 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 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. +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", + }, + { + // 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", + }, + { + // 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", + }, + { + // 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, cfg, 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, cfg, 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) + + 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) { + + t.Run("ours is removed", func(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, cfg, want, 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, 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, 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) + 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, cfg := fixture(t, `{"editor.fontSize": 13}`) + want, ca := bobWanted(cfg, t.TempDir()) + var out, errb bytes.Buffer + 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") { + 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, 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, cfg, want, 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. +// +// 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. +// +// 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 { + t.Fatal(err) + } + 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("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) + } + + // 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, cfg, 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") + } + 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. +// +// 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}`, 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) + before, err := os.ReadFile(settings) + if err != nil { + t.Fatal(err) + } + + 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 — a report is not a verdict", code) + } + + 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) + } + 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") + } + }) + } +} + +// 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 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}, + } { + 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) { + settings, cfg := fixture(t, `{"http.proxy": "http://127.0.0.1:47699"}`) + 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) + } + 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. +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 + want, ca := bobWanted(cfg, t.TempDir()) + 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) + 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 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 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) { + 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 whole-trust-store 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). +// +// 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 { + name, val, wantProxy string + owns bobOwnership + }{ + // 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", "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}, + {"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}, + } { + 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) + } + }) + } +} + +// 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, cfg, 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, 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()) + } + + 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) { + 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, cfg, 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) + } + } + }) +} + +// 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) + } +} + +// --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, 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()) + } + + 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 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"} { + 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) + } + }) + } +} + +// 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, cfg, 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 + } +} 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