diff --git a/cmd/gortex/doctor.go b/cmd/gortex/doctor.go index db6baf70a..3872200a9 100644 --- a/cmd/gortex/doctor.go +++ b/cmd/gortex/doctor.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "github.com/spf13/cobra" @@ -81,13 +82,15 @@ var ( doctorJSON bool doctorDays int doctorRedact bool + doctorAll bool ) func init() { for _, c := range []*cobra.Command{doctorCmd, initDoctorCmd} { c.Flags().BoolVar(&doctorJSON, "json", false, "emit a structured JSON report on stdout") c.Flags().IntVar(&doctorDays, "days", 7, "look-back window for hook activity, adoption, and savings") - c.Flags().BoolVar(&doctorRedact, "redact", false, "hash repo paths and branch names so the report can be shared") + c.Flags().BoolVar(&doctorRedact, "redact", false, "rewrite repo and home paths, and hash branch names, so the report can be shared") + c.Flags().BoolVar(&doctorAll, "all", false, "list planned files for adapters that are not installed too") } silenceDoctorErrors(doctorCmd, initDoctorCmd) rootCmd.AddCommand(doctorCmd) @@ -156,6 +159,13 @@ func runDoctor(cmd *cobra.Command, _ []string) error { Stderr: nil, // suppress progress lines; doctor is read-only } + if doctorRedact { + // Install the rewriter before anything renders, so every section — + // static, runtime, and the paths quoted inside findings — agrees on + // what a given repo is called. + doctorPath = newDoctorPathRedactor(home, root) + } + doctorEnv := doctorEnvironment() registry := buildRegistry() @@ -310,7 +320,7 @@ func inspectAdapter(a agents.Adapter, env agents.Env) DoctorAgentReport { func printDoctorEnvironment(w io.Writer, env DoctorEnvironment) { fmt.Fprintln(w, "Gortex doctor — environment:") if env.BinaryOnPath { - fmt.Fprintf(w, " %s gortex on PATH: %s\n", glyphCheck, env.BinaryPath) + fmt.Fprintf(w, " %s gortex on PATH: %s\n", glyphCheck, doctorPath(env.BinaryPath)) } else { fmt.Fprintf(w, " %s gortex not found on PATH (%s)\n", glyphCross, env.BinaryError) } @@ -319,27 +329,49 @@ func printDoctorEnvironment(w io.Writer, env DoctorEnvironment) { if ver == "" { ver = "ok" } - fmt.Fprintf(w, " %s daemon handshake: %s (%s)\n", glyphCheck, ver, env.DaemonSocket) + fmt.Fprintf(w, " %s daemon handshake: %s (%s)\n", glyphCheck, ver, doctorPath(env.DaemonSocket)) } else { fmt.Fprintf(w, " %s daemon handshake: %s\n", glyphCross, env.DaemonError) } fmt.Fprintln(w) } -// glyphCheck / glyphCross are the doctor's status markers. +// The doctor's status markers. The distinction between glyphAbsent and +// glyphCross is the whole point of having both: a per-repo file that `gortex +// init` has not written is not a fault — a machine-wide `gortex install` +// covers the agent, and most repos never need repo-local config at all. +// Marking it ✗ turned an optional file into an accusation and implied work +// the user does not have to do. ✗ is reserved for a real gap: something +// broken, outdated, or genuinely not wired up. const ( - glyphCheck = "✓" - glyphCross = "✗" + glyphCheck = "✓" + glyphCross = "✗" + glyphWarn = "!" + glyphAbsent = "·" ) // printDoctorHuman renders the human-readable summary. One row per // agent, with a nested file list. Columns line up for copy-paste // into issue reports. +// printDoctorHuman renders the adapter section. Adapters that are neither +// installed nor carrying Gortex files collapse to a name list: their planned +// paths describe writes that will never happen, and one uninstalled agent can +// contribute twenty of them — burying the handful of lines that describe the +// machine the user actually has. An absent agent that still holds Gortex files +// is kept and called out instead: that is leftover config, which is a finding. +// --all restores the full listing, and --json is always complete. func printDoctorHuman(w io.Writer, reports []DoctorAgentReport) { - fmt.Fprintln(w, "Gortex doctor — observed state of every adapter's planned files:") + fmt.Fprintln(w, "Gortex doctor — observed state of every detected adapter:") + fmt.Fprintf(w, " %s present %s absent (optional — repo-local config) %s needs attention\n", + glyphCheck, glyphAbsent, glyphWarn) fmt.Fprintln(w) + var absent []string for _, r := range reports { + if !doctorAll && !r.Detected && !r.Configured { + absent = append(absent, r.Name) + continue + } detMark := "–" if r.Detected { detMark = "✓" @@ -348,16 +380,18 @@ func printDoctorHuman(w io.Writer, reports []DoctorAgentReport) { if r.Configured { cfgMark = "✓" } - fmt.Fprintf(w, " [%s detected] [%s any-file-present] %s\n", detMark, cfgMark, r.Name) + fmt.Fprintf(w, " [%s installed] [%s gortex files] %s\n", detMark, cfgMark, r.Name) for _, f := range r.Files { statusSym := "?" switch f.Status { case "present": - statusSym = "✓" + statusSym = glyphCheck case "missing": - statusSym = "✗" + // Absent, not wrong: repo-local config is optional on a + // machine configured by `gortex install`. + statusSym = glyphAbsent case "unreadable": - statusSym = "!" + statusSym = glyphWarn } extra := "" if f.Status == "present" && f.ByteSize > 0 { @@ -365,7 +399,10 @@ func printDoctorHuman(w io.Writer, reports []DoctorAgentReport) { } switch f.StanzaStatus { case "stale": - extra += " [stale stanza: run `gortex install` to migrate]" + // Present but outdated is a real gap — Gortex wrote this and + // the shape has since moved on. + statusSym = glyphWarn + extra += " [outdated stanza: `gortex upgrade --run` migrates it, or `gortex install`]" case "current": extra += " [stanza current]" } @@ -376,13 +413,22 @@ func printDoctorHuman(w io.Writer, reports []DoctorAgentReport) { if len(verb) > len("would-") && verb[:len("would-")] == "would-" { verb = verb[len("would-"):] } - extra = fmt.Sprintf(" (init would %s)", verb) + extra = fmt.Sprintf(" (optional — `gortex init` would %s)", verb) } - fmt.Fprintf(w, " %s %s%s\n", statusSym, f.Path, extra) + fmt.Fprintf(w, " %s %s%s\n", statusSym, doctorPath(f.Path), extra) + } + if !r.Detected && r.Configured { + fmt.Fprintln(w, " note: not installed, but Gortex files are present — leftover config.") } if r.DocsURL != "" { fmt.Fprintf(w, " docs: %s\n", r.DocsURL) } fmt.Fprintln(w) } + + if len(absent) > 0 { + fmt.Fprintf(w, " not installed (%d): %s\n", len(absent), strings.Join(absent, ", ")) + fmt.Fprintln(w, " nothing is written for these; --all lists what init would write if they were.") + fmt.Fprintln(w) + } } diff --git a/cmd/gortex/doctor_redact.go b/cmd/gortex/doctor_redact.go new file mode 100644 index 000000000..af71a991e --- /dev/null +++ b/cmd/gortex/doctor_redact.go @@ -0,0 +1,53 @@ +package main + +import ( + "path/filepath" + "strings" +) + +// doctor_redact.go makes --redact mean something. The report is built to be +// pasted into an issue, and almost everything identifying in it is a path: +// the repo name in every planned-file line, the account name in every home +// path. Hashing the two roots keeps the structure legible — `/.mcp.json` +// still reads as a config path — while removing the parts a user would +// otherwise blur out by hand before sharing. + +// doctorPath rewrites one path for display. Replaced for the duration of a +// redacted run; the identity default keeps the normal report verbatim. +var doctorPath = func(p string) string { return p } + +// newDoctorPathRedactor rewrites paths under the repo root to /… and +// paths under the home directory to ~/…. Repo first: the repo usually lives +// under home, and its name is the more sensitive of the two. +func newDoctorPathRedactor(home, root string) func(string) string { + return func(p string) string { + if strings.TrimSpace(p) == "" { + return p + } + clean := filepath.Clean(p) + if rel, ok := relativeTo(root, clean); ok { + return filepath.Join("", rel) + } + if rel, ok := relativeTo(home, clean); ok { + return filepath.Join("~", rel) + } + // Outside both roots: no name of ours to leak, so leave it readable. + return clean + } +} + +// relativeTo reports base-relative form when path is inside base. "." maps to +// the empty string so the caller's Join yields the bare placeholder. +func relativeTo(base, path string) (string, bool) { + if strings.TrimSpace(base) == "" { + return "", false + } + rel, err := filepath.Rel(filepath.Clean(base), path) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", false + } + if rel == "." { + return "", true + } + return rel, true +} diff --git a/cmd/gortex/doctor_runtime.go b/cmd/gortex/doctor_runtime.go index ae4a449ec..349ebc10f 100644 --- a/cmd/gortex/doctor_runtime.go +++ b/cmd/gortex/doctor_runtime.go @@ -66,6 +66,11 @@ func collectRuntime(home string, days int) doctorRuntime { since := now.Add(-time.Duration(days) * 24 * time.Hour) state := codex.Inspect(home) + if doctorRedact { + state.ConfigPath = doctorPath(state.ConfigPath) + state.InstructionsPath = doctorPath(state.InstructionsPath) + state.ShadowedBy = doctorPath(state.ShadowedBy) + } activity, err := hooks.ReadEffectiveness(since) if err != nil { // A log we cannot read is not fatal; the other sections still carry @@ -95,6 +100,7 @@ func collectRuntime(home string, days int) doctorRuntime { if doctorRedact { out.Adoption = redactAdoption(out.Adoption) + out.Hooks.Path = doctorPath(out.Hooks.Path) } return out } @@ -147,20 +153,22 @@ func collectSavings(since time.Time) doctorSavings { // into an issue. Tool names, counts, and timings are never sensitive and are // always left intact — redacting them would defeat the point of sharing it. func redactAdoption(a doctor.Adoption) doctor.Adoption { - a.Root = redactPath(a.Root) + a.Root = doctorPath(a.Root) for i := range a.Sessions { - a.Sessions[i].CWD = redactPath(a.Sessions[i].CWD) - a.Sessions[i].Branch = redactPath(a.Sessions[i].Branch) + a.Sessions[i].CWD = doctorPath(a.Sessions[i].CWD) + a.Sessions[i].Branch = redactName(a.Sessions[i].Branch) } return a } -func redactPath(value string) string { +// redactName hashes a value that is not a path — a branch name can carry a +// ticket id or a customer name just as readily as a repo path can. +func redactName(value string) string { if value == "" { return "" } sum := sha256.Sum256([]byte(value)) - return "…/<" + hex.EncodeToString(sum[:])[:8] + ">" + return "<" + hex.EncodeToString(sum[:])[:8] + ">" } func printDoctorRuntime(w io.Writer, r doctorRuntime) { @@ -186,29 +194,42 @@ func printDoctorRuntime(w io.Writer, r doctorRuntime) { fmt.Fprintf(w, " no %s — no hook has ever run on this machine\n", r.Hooks.Path) } else { scoped := r.Hooks.ForAgent(codex.Name) - fmt.Fprintf(w, " %-18s %7s %9s %10s %s\n", "event", "runs", "injected", "daemon-up", "last seen") + fmt.Fprintf(w, " %-18s %7s %9s %10s %8s %s\n", "event", "runs", "injected", "daemon-up", "unattrib", "last seen") for _, event := range doctorEventOrder(r.Hooks) { stats := scoped[event] daemon := "n/a" if stats.DaemonKnown > 0 { daemon = fmt.Sprintf("%d/%d", stats.DaemonUp, stats.DaemonKnown) } + // A zero in `runs` is only alarming when `unattrib` is zero too; + // showing them side by side is what keeps the reader from + // reaching the conclusion the findings deliberately withheld. + ambiguous := r.Hooks.AmbiguousRuns(event) + seen := stats.LastSeen + if seen.IsZero() { + // The event did run, we just cannot say for whom — dating it + // "never" would contradict the count beside it. + seen = r.Hooks.Unattributed[event].LastSeen + } last := "never" - if !stats.LastSeen.IsZero() { - last = stats.LastSeen.Format(time.RFC3339)[:19] + if !seen.IsZero() { + last = seen.Format(time.RFC3339)[:19] } - fmt.Fprintf(w, " %-18s %7d %9d %10s %s\n", event, stats.Runs, stats.Emitted, daemon, last) + fmt.Fprintf(w, " %-18s %7d %9d %10s %8d %s\n", event, stats.Runs, stats.Emitted, daemon, ambiguous, last) } fmt.Fprintf(w, " %d row(s) in window, %d in the whole log\n", r.Hooks.WindowRows, r.Hooks.TotalRows) if agents := r.Hooks.AgentNames(); len(agents) > 0 { fmt.Fprintf(w, " agents seen %s\n", joinComma(agents)) } if r.Hooks.UnattributedRows > 0 { - // Rows written before the agent field existed cannot be assigned, - // so say how much of the evidence is shared rather than let the - // table imply a precision it does not have. - fmt.Fprintf(w, " note: %d row(s) predate per-agent attribution and are counted for\n", r.Hooks.UnattributedRows) - fmt.Fprintln(w, " every agent; they clear as the window rolls forward.") + // Rows written before the agent field existed cannot be assigned + // to anyone. They are held out of every agent's counts rather + // than shared into all of them, and a `runs` of 0 beside a + // non-zero `unattrib` means "cannot tell", not "did not run". + fmt.Fprintf(w, " note: %d row(s) predate per-agent attribution (the unattrib column).\n", r.Hooks.UnattributedRows) + fmt.Fprintln(w, " They can neither confirm nor rule out an agent's hooks, so a 0 in") + fmt.Fprintln(w, " runs beside a non-zero unattrib is withheld from the findings.") + fmt.Fprintln(w, " They clear as the window rolls past the upgrade.") } } fmt.Fprintln(w) diff --git a/cmd/gortex/doctor_test.go b/cmd/gortex/doctor_test.go index 8b68a86e2..e648271f0 100644 --- a/cmd/gortex/doctor_test.go +++ b/cmd/gortex/doctor_test.go @@ -1,9 +1,11 @@ package main import ( + "bytes" "encoding/json" "os" "path/filepath" + "strings" "testing" "github.com/zzet/gortex/internal/agents" @@ -95,3 +97,114 @@ func TestDoctorVerifiesPathDaemonAndStaleStanza(t *testing.T) { } }) } + +// TestDoctorCollapsesUninstalledAdapters: the adapter section describes the +// machine the user has. An agent that is neither installed nor holding Gortex +// files contributes only planned paths for writes that will never happen — +// and one such agent can contribute twenty, burying the lines that matter. +func TestDoctorCollapsesUninstalledAdapters(t *testing.T) { + reports := []DoctorAgentReport{ + {Name: "claude-code", Detected: true, Configured: true, Files: []DoctorFileStatus{ + {Path: "/repo/.mcp.json", Status: "present", ByteSize: 12}, + }}, + {Name: "hermes", Detected: false, Configured: false, Files: []DoctorFileStatus{ + {Path: "/home/u/.hermes/skills/a/SKILL.md", Status: "missing", Planned: "would-create"}, + {Path: "/home/u/.hermes/skills/b/SKILL.md", Status: "missing", Planned: "would-create"}, + }}, + {Name: "zed", Detected: false, Configured: false}, + } + + t.Run("collapsed by default", func(t *testing.T) { + doctorAll = false + var buf bytes.Buffer + printDoctorHuman(&buf, reports) + got := buf.String() + + if strings.Contains(got, "SKILL.md") { + t.Errorf("planned files for an uninstalled agent should be hidden:\n%s", got) + } + if !strings.Contains(got, "not installed (2): hermes, zed") { + t.Errorf("absent agents should still be named:\n%s", got) + } + if !strings.Contains(got, "/repo/.mcp.json") { + t.Errorf("detected agents keep their detail:\n%s", got) + } + }) + + t.Run("--all restores the listing", func(t *testing.T) { + doctorAll = true + t.Cleanup(func() { doctorAll = false }) + var buf bytes.Buffer + printDoctorHuman(&buf, reports) + if !strings.Contains(buf.String(), "SKILL.md") { + t.Errorf("--all should list everything:\n%s", buf.String()) + } + }) +} + +// TestDoctorKeepsLeftoverConfig: an agent that is gone but still holds Gortex +// files is not noise — it is stale config the user probably wants to clean up, +// so it survives the collapse and says why it is there. +func TestDoctorKeepsLeftoverConfig(t *testing.T) { + doctorAll = false + var buf bytes.Buffer + printDoctorHuman(&buf, []DoctorAgentReport{ + {Name: "windsurf", Detected: false, Configured: true, Files: []DoctorFileStatus{ + {Path: "/home/u/.codeium/mcp_config.json", Status: "present", ByteSize: 40}, + }}, + }) + got := buf.String() + + if strings.Contains(got, "not installed (1)") { + t.Errorf("an agent holding Gortex files must not be collapsed away:\n%s", got) + } + if !strings.Contains(got, "leftover config") { + t.Errorf("say why an uninstalled agent is still listed:\n%s", got) + } + if !strings.Contains(got, "/home/u/.codeium/mcp_config.json") { + t.Errorf("the leftover file itself is the point:\n%s", got) + } +} + +// TestDoctorReservesTheCrossForRealGaps: an absent repo-local file is not a +// fault. A machine configured by `gortex install` needs no repo-local config +// at all, so marking it ✗ read as an accusation and implied work the user +// does not have to do. ✗ belongs to things that are broken or outdated. +func TestDoctorReservesTheCrossForRealGaps(t *testing.T) { + doctorAll = false + var buf bytes.Buffer + printDoctorHuman(&buf, []DoctorAgentReport{{ + Name: "claude-code", Detected: true, Configured: true, + Files: []DoctorFileStatus{ + {Path: "/repo/.mcp.json", Status: "missing", Planned: "would-create"}, + {Path: "/repo/.claude/settings.local.json", Status: "present", ByteSize: 26774}, + {Path: "/home/u/.gemini/mcp_config.json", Status: "present", ByteSize: 223, StanzaStatus: "stale"}, + {Path: "/repo/.broken.json", Status: "unreadable"}, + }, + }}) + got := buf.String() + + for _, line := range strings.Split(got, "\n") { + if strings.Contains(line, ".mcp.json") && !strings.Contains(line, "gemini") { + if strings.Contains(line, glyphCross) { + t.Errorf("an optional absent file must not be marked as a gap: %q", line) + } + if !strings.Contains(line, glyphAbsent) || !strings.Contains(line, "optional") { + t.Errorf("an absent optional file should read as optional: %q", line) + } + } + if strings.Contains(line, "settings.local.json") && !strings.Contains(line, glyphCheck) { + t.Errorf("a present file is fine: %q", line) + } + // Present-but-outdated and unreadable are the real gaps. + if strings.Contains(line, "gemini") && !strings.Contains(line, glyphWarn) { + t.Errorf("an outdated stanza needs attention: %q", line) + } + if strings.Contains(line, ".broken.json") && !strings.Contains(line, glyphWarn) { + t.Errorf("an unreadable file needs attention: %q", line) + } + } + if !strings.Contains(got, "needs attention") { + t.Errorf("the legend should explain the markers:\n%s", got) + } +} diff --git a/cmd/gortex/upgrade.go b/cmd/gortex/upgrade.go index dbf926841..20c772c63 100644 --- a/cmd/gortex/upgrade.go +++ b/cmd/gortex/upgrade.go @@ -139,8 +139,11 @@ func normalizeSemver(v string) string { var upgradeRun bool var upgradeCmd = &cobra.Command{ - Use: "upgrade [version]", - Short: "Update gortex to the latest release using the method it was installed with", + Use: "upgrade [version]", + // "update" is what people type — the command's own summary line has always + // said "Update gortex…" — and it used to be an unknown-command error. + Aliases: []string{"update"}, + Short: "Update gortex to the latest release using the method it was installed with", Long: "Detects how this gortex binary was installed (Homebrew, Scoop, go install, or the " + "installer script) and runs the matching update command. Pass a version (or set " + "GORTEX_VERSION) to pin a specific release. By default the command is printed; pass --run to " + @@ -151,6 +154,7 @@ var upgradeCmd = &cobra.Command{ func init() { upgradeCmd.Flags().BoolVar(&upgradeRun, "run", false, "execute the detected upgrade command instead of printing it") + upgradeCmd.Flags().BoolVar(&upgradeNoMigrate, "no-migrate", false, "skip refreshing agent config after the upgrade") rootCmd.AddCommand(upgradeCmd) } @@ -197,6 +201,7 @@ func runUpgrade(cmd *cobra.Command, args []string) error { fmt.Fprintf(out, "Upgrading gortex (install method: %s)\n", method) } + migrated := false if !upgradeRun { fmt.Fprintf(out, "Run:\n %s\n", command) } else { @@ -209,12 +214,16 @@ func runUpgrade(cmd *cobra.Command, args []string) error { if rerr := runUpgradeCommand(out, cmd.ErrOrStderr(), defaultUpgradeDaemonOps(cmd), run); rerr != nil { return fmt.Errorf("upgrade command failed: %w", rerr) } + // Config shapes drift between releases, and the moment the new binary + // lands is when they should be brought current — telling the user to + // go run `gortex install` afterwards hands back a chore the upgrade + // can do itself. Runs from the new binary; see upgrade_migrate.go. + migrated = postUpgradeSteps(cmd.Context(), out, cmd.ErrOrStderr(), upgradeNoMigrate) } - // Post-upgrade advisory: a new binary may carry newer per-language - // extractors, so the indexed graph for an affected language is stale until - // reindexed. F2 enriches this with the exact stale languages per repo. - fmt.Fprintln(out, "\nAfter upgrading, reindex so the graph picks up any extractor changes:\n gortex index .") + // A new binary may carry newer per-language extractors, so the indexed + // graph for an affected language is stale until reindexed. + upgradeFollowUps(out, upgradeRun, migrated) return nil } diff --git a/cmd/gortex/upgrade_migrate.go b/cmd/gortex/upgrade_migrate.go new file mode 100644 index 000000000..171a55423 --- /dev/null +++ b/cmd/gortex/upgrade_migrate.go @@ -0,0 +1,123 @@ +package main + +import ( + "context" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" +) + +// upgrade_migrate.go re-applies agent configuration after a successful binary +// upgrade. +// +// Config shapes drift between releases — an MCP stanza gains a field, a hook +// command changes, an instructions block is rewritten. Until now the only +// thing that noticed was `gortex doctor`, which told the user to go run +// `gortex install`. That reads as a chore handed back: upgrading is exactly +// when the configs should be brought current, and the user already asked for +// the new version by running this command. +// +// Two constraints shape the implementation. +// +// First, the migration must run from the NEW binary. This process is the old +// one — the package manager replaced the file underneath it — so re-applying +// adapters in-process would faithfully write the shapes we are trying to +// migrate away from. We re-exec instead. +// +// Second, re-applying can rewrite a hook definition, and Codex records trust +// against each hook's hash: a changed hook is skipped until the user +// re-approves it in /hooks. Silently re-trusting is not an option, so the +// migration deliberately runs through `gortex install`, whose summary already +// carries that notice, rather than a bespoke quieter migrator. + +// upgradeNoMigrate opts out of the post-upgrade config refresh. +var upgradeNoMigrate bool + +// upgradeMigrateCommand builds the re-exec. A package var so tests can assert +// on the invocation without running an installer. +var upgradeMigrateCommand = func(ctx context.Context, bin string) *exec.Cmd { + // --yes keeps it non-interactive: the user already consented by upgrading, + // and a wizard prompt inside an upgrade would be a surprise. + return exec.CommandContext(ctx, bin, "install", "--yes") +} + +// resolveUpgradedBinary finds the gortex that the upgrade just installed. +// PATH first: `go install` can land the new binary somewhere other than where +// the old one ran from, and re-execing the stale path would migrate using the +// very code we replaced. os.Executable is the fallback for the common case +// where the package manager overwrote the file in place. +func resolveUpgradedBinary() (string, error) { + if p, err := exec.LookPath("gortex"); err == nil { + if abs, aerr := filepath.Abs(p); aerr == nil { + return abs, nil + } + return p, nil + } + exe, err := os.Executable() + if err != nil { + return "", fmt.Errorf("locate the upgraded gortex binary: %w", err) + } + return exe, nil +} + +// migrateAgentConfigs re-applies every detected adapter's user-level config +// with the freshly installed binary. Best-effort by design: the upgrade +// itself has already succeeded, so a migration that cannot run is a warning +// with a manual command, never a failed upgrade. +func migrateAgentConfigs(ctx context.Context, out, errw io.Writer) { + bin, err := resolveUpgradedBinary() + if err != nil { + fmt.Fprintf(errw, "warning: %v — run `gortex install` to refresh agent config.\n", err) + return + } + + fmt.Fprintln(out, "\nRefreshing agent configuration with the new binary:") + cmd := upgradeMigrateCommand(ctx, bin) + cmd.Stdout, cmd.Stderr = out, errw + if err := cmd.Run(); err != nil { + fmt.Fprintf(errw, "warning: agent config refresh failed (%v) — run `gortex install` by hand.\n", err) + } +} + +// upgradeFollowUps renders what still has to happen after the binary lands. +// The reindex advisory is unconditional (a new binary may carry newer +// extractors, so an indexed graph can be stale). The config step is listed +// only when we did not just perform it. +func upgradeFollowUps(out io.Writer, ran, migrated bool) { + var steps []string + if !migrated { + // Either a dry run (we only printed the upgrade command) or --no-migrate. + steps = append(steps, "gortex install # refresh agent config for the new version") + } + steps = append(steps, "gortex index . # reindex so the graph picks up extractor changes") + + lead := "After upgrading:" + if ran { + lead = "Next:" + } + fmt.Fprintf(out, "\n%s\n", lead) + for _, s := range steps { + fmt.Fprintf(out, " %s\n", s) + } +} + +// upgradeMigrationSkipped explains a --no-migrate run so the omission is +// visible rather than silent. +func upgradeMigrationSkipped(out io.Writer) { + fmt.Fprintln(out, "\nSkipping the agent config refresh (--no-migrate).") +} + +// postUpgradeSteps runs what has to happen once the new binary is on disk and +// reports whether the config refresh was performed. Split out from runUpgrade +// so the decision is testable without a real install method, a real release +// check, or a real package manager. +func postUpgradeSteps(ctx context.Context, out, errw io.Writer, noMigrate bool) bool { + if noMigrate { + upgradeMigrationSkipped(out) + return false + } + migrateAgentConfigs(ctx, out, errw) + return true +} diff --git a/cmd/gortex/upgrade_test.go b/cmd/gortex/upgrade_test.go index 4fdc5d572..55e14613e 100644 --- a/cmd/gortex/upgrade_test.go +++ b/cmd/gortex/upgrade_test.go @@ -1,9 +1,13 @@ package main import ( + "bytes" "context" "errors" "io" + "os/exec" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -138,11 +142,11 @@ func TestRunUpgradeCommandDaemonBounce(t *testing.T) { // TestUpgradeReleaseTagParse covers the /releases/latest redirect tag parse. func TestUpgradeReleaseTagParse(t *testing.T) { cases := map[string]string{ - "https://github.com/zzet/gortex/releases/tag/v0.49.0": "v0.49.0", - "https://github.com/zzet/gortex/releases/tag/v1.0.0-rc1": "v1.0.0-rc1", - "https://github.com/zzet/gortex/releases/tag/v0.49.0?x=1": "v0.49.0", - "https://github.com/zzet/gortex/releases": "", - "": "", + "https://github.com/zzet/gortex/releases/tag/v0.49.0": "v0.49.0", + "https://github.com/zzet/gortex/releases/tag/v1.0.0-rc1": "v1.0.0-rc1", + "https://github.com/zzet/gortex/releases/tag/v0.49.0?x=1": "v0.49.0", + "https://github.com/zzet/gortex/releases": "", + "": "", } for loc, want := range cases { if got := tagFromReleaseLocation(loc); got != want { @@ -150,3 +154,125 @@ func TestUpgradeReleaseTagParse(t *testing.T) { } } } + +// TestUpgradeMigratesWithTheNewBinary pins the sequencing that makes the +// post-upgrade refresh correct at all: this process is the OLD binary, so +// re-applying adapters in-process would faithfully write the shapes the +// upgrade is meant to migrate away from. The refresh must re-exec. +func TestUpgradeMigratesWithTheNewBinary(t *testing.T) { + var gotBin string + var gotArgs []string + orig := upgradeMigrateCommand + upgradeMigrateCommand = func(ctx context.Context, bin string) *exec.Cmd { + gotBin = bin + c := exec.CommandContext(ctx, "true") + gotArgs = []string{"install", "--yes"} + return c + } + t.Cleanup(func() { upgradeMigrateCommand = orig }) + + var out, errw bytes.Buffer + migrateAgentConfigs(context.Background(), &out, &errw) + + if gotBin == "" { + t.Fatal("migration did not resolve a binary to re-exec") + } + if !filepath.IsAbs(gotBin) { + t.Errorf("re-exec target should be absolute, got %q", gotBin) + } + if len(gotArgs) != 2 || gotArgs[0] != "install" || gotArgs[1] != "--yes" { + t.Errorf("migration should run a non-interactive install, got %v", gotArgs) + } + if !strings.Contains(out.String(), "Refreshing agent configuration") { + t.Errorf("the refresh should announce itself:\n%s", out.String()) + } +} + +// TestUpgradeMigrationFailureDoesNotFailTheUpgrade: the binary has already +// been replaced by the time the refresh runs, so a refresh that cannot run is +// a warning with a manual command — never a failed upgrade. +func TestUpgradeMigrationFailureDoesNotFailTheUpgrade(t *testing.T) { + orig := upgradeMigrateCommand + upgradeMigrateCommand = func(ctx context.Context, bin string) *exec.Cmd { + return exec.CommandContext(ctx, "false") + } + t.Cleanup(func() { upgradeMigrateCommand = orig }) + + var out, errw bytes.Buffer + migrateAgentConfigs(context.Background(), &out, &errw) + + if !strings.Contains(errw.String(), "run `gortex install` by hand") { + t.Errorf("a failed refresh must hand back a manual command:\n%s", errw.String()) + } +} + +// TestUpgradeFollowUpsListConfigOnlyWhenNotDone keeps the closing advice +// honest: it must not tell the user to run the step just performed. +func TestUpgradeFollowUpsListConfigOnlyWhenNotDone(t *testing.T) { + var done bytes.Buffer + upgradeFollowUps(&done, true, true) + if strings.Contains(done.String(), "gortex install") { + t.Errorf("config was refreshed; do not ask for it again:\n%s", done.String()) + } + if !strings.Contains(done.String(), "gortex index .") { + t.Errorf("reindex advice is unconditional:\n%s", done.String()) + } + + var skipped bytes.Buffer + upgradeFollowUps(&skipped, false, false) + if !strings.Contains(skipped.String(), "gortex install") { + t.Errorf("a dry run still owes the config step:\n%s", skipped.String()) + } +} + +// TestUpgradeAcceptsUpdate: "update" is what people type, and the command's +// own summary line has always said "Update gortex…". It used to be an +// unknown-command error, which meant the post-upgrade config refresh would +// never run for anyone whose habit is `gortex update`. +func TestUpgradeAcceptsUpdate(t *testing.T) { + found, _, err := rootCmd.Find([]string{"update"}) + if err != nil { + t.Fatalf("`gortex update` does not resolve: %v", err) + } + if found != upgradeCmd { + t.Fatalf("`gortex update` resolved to %q, want the upgrade command", found.Name()) + } +} + +// TestPostUpgradeStepsDecision pins the wiring the whole feature hangs on: +// after a real upgrade the refresh runs, --no-migrate skips it visibly, and +// the return value is what the closing advice keys off. +func TestPostUpgradeStepsDecision(t *testing.T) { + var ran bool + orig := upgradeMigrateCommand + upgradeMigrateCommand = func(ctx context.Context, bin string) *exec.Cmd { + ran = true + return exec.CommandContext(ctx, "true") + } + t.Cleanup(func() { upgradeMigrateCommand = orig }) + + t.Run("refreshes by default", func(t *testing.T) { + ran = false + var out, errw bytes.Buffer + if migrated := postUpgradeSteps(context.Background(), &out, &errw, false); !migrated { + t.Error("a completed upgrade should report the refresh as done") + } + if !ran { + t.Error("the refresh did not run") + } + }) + + t.Run("--no-migrate skips visibly", func(t *testing.T) { + ran = false + var out, errw bytes.Buffer + if migrated := postUpgradeSteps(context.Background(), &out, &errw, true); migrated { + t.Error("--no-migrate must not report the refresh as done") + } + if ran { + t.Error("the refresh ran despite --no-migrate") + } + if !strings.Contains(out.String(), "--no-migrate") { + t.Errorf("skipping must be stated, not silent:\n%s", out.String()) + } + }) +} diff --git a/docs/agents.md b/docs/agents.md index 7c0e21170..fb186f26b 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -139,8 +139,9 @@ gortex init --hooks-only # refresh supported agent hooks only # Observe-only gortex doctor # config + hook activity + adoption -gortex doctor --json # machine-readable report +gortex doctor --json # machine-readable report (always complete) gortex doctor --redact # safe to paste into an issue +gortex doctor --all # include adapters that are not installed ``` ## Adapter contract diff --git a/internal/doctor/findings.go b/internal/doctor/findings.go index f96365262..6ed50264f 100644 --- a/internal/doctor/findings.go +++ b/internal/doctor/findings.go @@ -32,7 +32,21 @@ type Finding struct { // agentActivity is one harness's slice of the invocation log, so the finding // rules below cannot accidentally read another agent's rows. type agentActivity struct { - events map[string]hooks.EventActivity + events map[string]hooks.EventActivity + summary hooks.EffectivenessSummary +} + +// ambiguous reports that the window holds runs of this event that cannot be +// attributed either way, so silence cannot be concluded from an empty bucket. +// +// Zero attributed runs is not evidence on its own. A log is mixed for as long +// as the window still reaches back past the upgrade that introduced +// attribution, and during that overlap an event may have run with no agent +// recorded — most visibly for events that fire once per session, where a busy +// PreToolUse stream attributes itself within minutes while SessionStart waits +// for the next session. Concluding silence there accuses a working install. +func (a agentActivity) ambiguous(event string) bool { + return a.events[event].Runs == 0 && a.summary.AmbiguousRuns(event) > 0 } // AgentHooks is what an adapter's config declares, per lifecycle event. @@ -64,7 +78,7 @@ func Diagnose(agent AgentHooks, summary hooks.EffectivenessSummary, adoption Ado // Scope the evidence to this harness. On a machine running more than one // agent the union would let a busy Claude Code session vouch for hooks // Codex is skipping — exactly the failure doctor exists to catch. - activity := agentActivity{events: summary.ForAgent(agent.Agent)} + activity := agentActivity{events: summary.ForAgent(agent.Agent), summary: summary} add := func(sev Severity, remedy, format string, args ...any) { findings = append(findings, Finding{Severity: sev, Summary: fmt.Sprintf(format, args...), Remedy: remedy}) } @@ -79,15 +93,22 @@ func Diagnose(agent AgentHooks, summary hooks.EffectivenessSummary, adoption Ado } sort.Strings(events) - var ran int + var ran, ambiguous int for _, event := range events { ran += activity.events[event].Runs + ambiguous += summary.AmbiguousRuns(event) } switch { case configured == 0: add(SeverityWarn, "gortex install", "%s has no Gortex lifecycle hooks configured.", agent.Agent) + case ran == 0 && ambiguous > 0: + // The window straddles the upgrade that introduced attribution: runs + // exist, they just cannot be assigned. Say so rather than accuse. + add(SeverityInfo, "re-run after the window clears the upgrade", + "%d hook run(s) in this window predate per-agent attribution, so %s's hooks can be neither confirmed nor ruled out.", + ambiguous, agent.Agent) case ran == 0 && agent.RequiresTrust: // The headline case. Every declared hook is on disk and none has // run, which for a trust-gating agent means the hooks were never @@ -106,7 +127,7 @@ func Diagnose(agent AgentHooks, summary hooks.EffectivenessSummary, adoption Ado if ran > 0 { for _, event := range events { stats := activity.events[event] - if stats.Runs > 0 { + if stats.Runs > 0 || activity.ambiguous(event) { continue } remedy := "re-run `gortex install`" diff --git a/internal/doctor/findings_test.go b/internal/doctor/findings_test.go index 50ca4b1e4..e05f2b99e 100644 --- a/internal/doctor/findings_test.go +++ b/internal/doctor/findings_test.go @@ -269,3 +269,100 @@ func TestDiagnoseFallsBackOnUnattributedLogs(t *testing.T) { t.Errorf("a pre-attribution log is not evidence of failure: %+v", findings) } } + +// TestMixedAttributionDoesNotAccuse reproduces the state every machine passes +// through right after the upgrade that introduced per-agent attribution: a +// window holding thousands of unattributed rows plus a handful of attributed +// ones. PreToolUse fires constantly and attributes itself within minutes; +// SessionStart fires once per session and has not run again yet. Reading its +// empty attributed bucket as silence accuses a working install of exactly the +// failure doctor exists to detect. +func TestMixedAttributionDoesNotAccuse(t *testing.T) { + now := time.Now() + mixed := hooks.EffectivenessSummary{ + Present: true, + Attributed: true, // one attributed row is enough to flip this + UnattributedRows: 17317, + Events: map[string]hooks.EventActivity{ + "SessionStart": {Runs: 6, Emitted: 6}, "UserPromptSubmit": {Runs: 8, Emitted: 0}, + "PreToolUse": {Runs: 1700, Emitted: 400}, "PostToolUse": {Runs: 59, Emitted: 0}, + }, + // Only the constantly-firing hook has attributed rows yet. + ByAgent: map[string]map[string]hooks.EventActivity{ + "codex": {"PreToolUse": {Runs: 15, Emitted: 3, DaemonKnown: 15, DaemonUp: 15}}, + }, + // Everything else in the window predates the field. + Unattributed: map[string]hooks.EventActivity{ + "SessionStart": {Runs: 6, Emitted: 6}, "UserPromptSubmit": {Runs: 8}, + "PreToolUse": {Runs: 1685, Emitted: 397}, "PostToolUse": {Runs: 59}, + }, + } + + findings := Diagnose(codexAgent(), mixed, Adoption{FilesFound: 7, InWindow: 7, GortexCalls: 338, ShellCalls: 102}, now) + + for _, event := range []string{"SessionStart", "UserPromptSubmit", "PostToolUse"} { + assertNoFindingWith(t, findings, event+" is configured but never ran") + } + if HasBlocker(findings) { + t.Fatalf("mixed attribution is not evidence of failure: %+v", findings) + } +} + +// TestAttributedSilenceStillBlocks: once the window holds no unattributed +// runs of an event, an empty bucket really is silence and must still block — +// the guard above must not swallow the finding it exists to protect. +func TestAttributedSilenceStillBlocks(t *testing.T) { + now := time.Now() + clean := hooks.EffectivenessSummary{ + Present: true, + Attributed: true, + Events: map[string]hooks.EventActivity{ + "PreToolUse": {Runs: 15, Emitted: 3}, + }, + ByAgent: map[string]map[string]hooks.EventActivity{ + "codex": {"PreToolUse": {Runs: 15, Emitted: 3, DaemonKnown: 15, DaemonUp: 15}}, + }, + Unattributed: map[string]hooks.EventActivity{}, + } + + findings := Diagnose(codexAgent(), clean, Adoption{FilesFound: 1, InWindow: 1, GortexCalls: 9, ShellCalls: 1}, now) + got := findingWith(t, findings, "SessionStart is configured but never ran") + if got.Severity != SeverityBlocker { + t.Errorf("severity=%s want BLOCKER", got.Severity) + } + if !strings.Contains(got.Remedy, "/hooks") { + t.Errorf("remedy=%q", got.Remedy) + } +} + +// TestWhollyAmbiguousWindowReportsUncertainty: no attributed runs at all for +// this agent, but the window has unattributed runs — doctor must say it +// cannot tell rather than either accuse or stay silent. +func TestWhollyAmbiguousWindowReportsUncertainty(t *testing.T) { + now := time.Now() + summary := hooks.EffectivenessSummary{ + Present: true, + Attributed: true, + UnattributedRows: 40, + Events: map[string]hooks.EventActivity{ + "SessionStart": {Runs: 10}, "UserPromptSubmit": {Runs: 10}, + "PreToolUse": {Runs: 10}, "PostToolUse": {Runs: 10}, + }, + ByAgent: map[string]map[string]hooks.EventActivity{ + "claude-code": {"Stop": {Runs: 3}}, + }, + Unattributed: map[string]hooks.EventActivity{ + "SessionStart": {Runs: 10}, "UserPromptSubmit": {Runs: 10}, + "PreToolUse": {Runs: 10}, "PostToolUse": {Runs: 10}, + }, + } + + findings := Diagnose(codexAgent(), summary, Adoption{}, now) + got := findingWith(t, findings, "predate per-agent attribution") + if got.Severity != SeverityInfo { + t.Errorf("severity=%s want INFO — uncertainty is not a failure", got.Severity) + } + if HasBlocker(findings) { + t.Errorf("must not accuse on evidence that cannot decide: %+v", findings) + } +} diff --git a/internal/hooks/codex.go b/internal/hooks/codex.go index 0b6ce5321..d1fc1f342 100644 --- a/internal/hooks/codex.go +++ b/internal/hooks/codex.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/zzet/gortex/internal/modelhint" ) // CodexMode is deliberately separate from the cross-agent hook Mode: Codex's @@ -65,10 +67,15 @@ func runCodex(data []byte, port int, selected ...CodexMode) { HookEventName string `json:"hook_event_name"` ToolName string `json:"tool_name"` CWD string `json:"cwd"` + // Codex sends the active model slug on every hook event. Claude Code + // does not, which is why its hint has to be recovered from the + // transcript; here it is simply handed to us. + Model string `json:"model"` } if err := json.Unmarshal(data, &peek); err != nil { return } + captureCodexModelHint(peek.HookEventName, peek.CWD, peek.Model) mode := CodexModeEnrich if len(selected) > 0 { mode = selected[0] @@ -102,6 +109,29 @@ func runCodex(data []byte, port int, selected ...CodexMode) { } } +// captureCodexModelHint records which model is driving this Codex session so +// the daemon can attribute savings to it. Without a writer of its own, a Codex +// session either recorded no model or — worse, before the reader learned to +// check the hint's agent — inherited whichever model Claude Code last +// announced for the same working directory. +// +// Only the once-per-session and once-per-turn events write. PreToolUse fires +// hundreds of times per session and would turn a 12-hour hint into a +// per-tool-call disk write for no extra freshness. +func captureCodexModelHint(event, cwd, model string) { + switch event { + case "SessionStart", "UserPromptSubmit": + default: + return + } + if strings.TrimSpace(model) == "" { + return + } + // Tagged with the agent name; modelhint.SameClient bridges it to the + // "codex-mcp-client" the MCP session reports. + modelhint.Write(cwd, model, "codex") +} + func runCodexBashHardDeny(data []byte, port int) { started := time.Now() var input HookInput diff --git a/internal/hooks/codex_test.go b/internal/hooks/codex_test.go index 0ef9285cc..b15949567 100644 --- a/internal/hooks/codex_test.go +++ b/internal/hooks/codex_test.go @@ -7,6 +7,8 @@ import ( "time" "github.com/zzet/gortex/internal/daemon" + + "github.com/zzet/gortex/internal/modelhint" ) func TestParseCodexModeDefaultsAdvisory(t *testing.T) { @@ -718,3 +720,74 @@ func codexPreToolPayloadWithPermission(toolName string, input string, permission func codexPostBashPayload(command string, response string) []byte { return []byte(`{"hook_event_name":"PostToolUse","tool_name":"Bash","session_id":"codex-shape","tool_input":{"command":` + strconv.Quote(command) + `},"tool_response":` + strconv.Quote(response) + `}`) } + +// TestCodexCapturesTheModelHint closes the gap that left every Codex call +// either unattributed or — before the reader checked the hint's agent — +// booked against whichever model Claude Code last announced for the same +// working directory. Codex hands us the active model slug on every hook +// event; nothing was reading it. +func TestCodexCapturesTheModelHint(t *testing.T) { + dir := t.TempDir() + t.Setenv("GORTEX_MODEL_HINT_DIR", dir) + // SessionStart otherwise dials the real daemon socket and waits out its + // timeout — two seconds of I/O, and enough stdout noise to bury the + // result line. Pin the status so this stays a unit test. + origStatus := sessionStartStatusFn + sessionStartStatusFn = func() (*daemon.StatusResponse, error) { return nil, errDaemonUnreachable } + t.Cleanup(func() { sessionStartStatusFn = origStatus }) + + payload := func(event, model string) []byte { + return []byte(`{"hook_event_name":"` + event + `","cwd":"/repo","model":"` + model + `","prompt":"hi"}`) + } + + runCodex(payload("SessionStart", "gpt-5.6-sol"), 0) + + got, ok := modelhint.Read("/repo") + if !ok { + t.Fatal("no hint written for a Codex session") + } + if got.Model != "gpt-5.6-sol" { + t.Errorf("model = %q, want gpt-5.6-sol", got.Model) + } + // Must be tagged as Codex, or the reader's cross-agent guard cannot tell + // this hint from one Claude Code wrote for the same directory. + if !modelhint.SameClient(got.Client, "codex-mcp-client") { + t.Errorf("client = %q, should match the codex MCP client", got.Client) + } + if modelhint.SameClient(got.Client, "claude-code") { + t.Errorf("client = %q must not be adopted by a Claude session", got.Client) + } +} + +// TestCodexModelHintWriteCadence: PreToolUse fires hundreds of times a +// session, so it must not turn a 12-hour hint into a per-tool-call disk write. +// The turn- and session-scoped events are frequent enough to keep it fresh. +func TestCodexModelHintWriteCadence(t *testing.T) { + dir := t.TempDir() + t.Setenv("GORTEX_MODEL_HINT_DIR", dir) + + captureCodexModelHint("PreToolUse", "/repo", "gpt-5.6-sol") + if _, ok := modelhint.Read("/repo"); ok { + t.Error("PreToolUse should not write a hint") + } + + captureCodexModelHint("UserPromptSubmit", "/repo", "gpt-5.6-sol") + if _, ok := modelhint.Read("/repo"); !ok { + t.Error("UserPromptSubmit should write a hint") + } +} + +// TestCodexModelHintIgnoresEmpty keeps an older Codex that omits the field +// from clearing a good hint. +func TestCodexModelHintIgnoresEmpty(t *testing.T) { + dir := t.TempDir() + t.Setenv("GORTEX_MODEL_HINT_DIR", dir) + + captureCodexModelHint("SessionStart", "/repo", "gpt-5.6-sol") + captureCodexModelHint("SessionStart", "/repo", "") + + got, ok := modelhint.Read("/repo") + if !ok || got.Model != "gpt-5.6-sol" { + t.Errorf("an empty model must not clobber a good hint: %+v ok=%v", got, ok) + } +} diff --git a/internal/hooks/telemetry_read.go b/internal/hooks/telemetry_read.go index 93c2f4299..c51300a69 100644 --- a/internal/hooks/telemetry_read.go +++ b/internal/hooks/telemetry_read.go @@ -59,6 +59,20 @@ type EffectivenessSummary struct { // UnattributedRows counts window rows with no agent, so a caller can say // how much of its evidence predates attribution. UnattributedRows int `json:"unattributed_rows"` + // Unattributed rolls those rows up per event. A log is not simply + // attributed or not: after an upgrade it is *mixed* for as long as the + // window still reaches back past it, and during that overlap an event + // with no attributed rows may still have run — the rows just cannot say + // for whom. Keeping the per-event count is what lets a caller tell + // "did not run" from "cannot tell". + Unattributed map[string]EventActivity `json:"unattributed,omitempty"` +} + +// AmbiguousRuns reports how many invocations of an event in the window carry +// no agent, and so might belong to any harness. Non-zero means silence cannot +// be concluded for that event, however empty a given agent's rows look. +func (s EffectivenessSummary) AmbiguousRuns(event string) int { + return s.Unattributed[event].Runs } // Ran reports whether the event was invoked at least once in the window, @@ -132,9 +146,10 @@ func (s EffectivenessSummary) EventNames() []string { func ReadEffectiveness(since time.Time) (EffectivenessSummary, error) { path := EffectivenessLogPath() summary := EffectivenessSummary{ - Path: path, - Events: map[string]EventActivity{}, - ByAgent: map[string]map[string]EventActivity{}, + Path: path, + Events: map[string]EventActivity{}, + ByAgent: map[string]map[string]EventActivity{}, + Unattributed: map[string]EventActivity{}, } if path == "" { return summary, nil @@ -176,6 +191,13 @@ func ReadEffectiveness(since time.Time) (EffectivenessSummary, error) { accumulate(&activity, rec) if rec.Agent == "" { summary.UnattributedRows++ + orphan := summary.Unattributed[event] + orphan.Event = event + if stamp.After(orphan.LastSeen) { + orphan.LastSeen = stamp + } + accumulate(&orphan, rec) + summary.Unattributed[event] = orphan } else { summary.Attributed = true perAgent := summary.ByAgent[rec.Agent] diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 854cca1a8..f9cbfbe04 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -814,7 +814,15 @@ func (ts *tokenStats) record(node *graph.Node, tool string, returned, fullFile i // in), with the hint's own global-last fallback inside Read. var model string if h, ok := modelhint.Read(fallbackRepo); ok { - model = h.Model + // The hint is keyed by working directory, so on a machine where two + // agents share a repo the wrong one can read it. Adopting a model + // across agents is worse than recording none: it books the call + // against another vendor's model and prices it at their rates. Only + // take the model when the hint's agent is consistent with this + // session's client. + if modelhint.SameClient(h.Client, clientName) { + model = h.Model + } if clientName == "" { clientName = h.Client } diff --git a/internal/modelhint/client_test.go b/internal/modelhint/client_test.go new file mode 100644 index 000000000..df61d2043 --- /dev/null +++ b/internal/modelhint/client_test.go @@ -0,0 +1,74 @@ +package modelhint + +import "testing" + +// The hint is keyed by working directory, so on a machine where two agents +// share a repo, whichever agent reads it gets whatever the last one wrote. +// These tests pin the guard that keeps a Codex call from being booked against +// a Claude model — a mistake that does not merely lose attribution, it prices +// the call at another vendor's rates. + +func TestNormalizeClientBridgesTheTwoSpellings(t *testing.T) { + cases := map[string]string{ + "codex-mcp-client": "codex", + "codex": "codex", + "CODEX-MCP-CLIENT": "codex", + " codex ": "codex", + "claude-code": "claude-code", + "kimi-mcp": "kimi", + "": "", + } + for in, want := range cases { + if got := NormalizeClient(in); got != want { + t.Errorf("NormalizeClient(%q) = %q, want %q", in, got, want) + } + } +} + +func TestSameClient(t *testing.T) { + same := [][2]string{ + {"codex", "codex-mcp-client"}, // the hook's name vs the MCP client's + {"claude-code", "claude-code"}, + {"", "codex-mcp-client"}, // unknown cannot contradict + {"claude-code", ""}, + } + for _, pair := range same { + if !SameClient(pair[0], pair[1]) { + t.Errorf("SameClient(%q, %q) = false, want true", pair[0], pair[1]) + } + } + + // The case that produced 3,224 mis-attributed calls: a Claude-written + // hint read by a Codex session working in the same repo. + if SameClient("claude-code", "codex-mcp-client") { + t.Error("a Claude hint must not be adopted by a Codex session") + } + if SameClient("codex", "claude-code") { + t.Error("a Codex hint must not be adopted by a Claude session") + } +} + +// TestReadWriteRoundTripCarriesClient: the guard is only as good as the +// client actually being recorded on the hint. +func TestReadWriteRoundTripCarriesClient(t *testing.T) { + dir := t.TempDir() + t.Setenv(dirEnvVar, dir) + + Write("/repo", "gpt-5.6-sol", "codex") + got, ok := Read("/repo") + if !ok { + t.Fatal("hint not readable") + } + if got.Model != "gpt-5.6-sol" { + t.Errorf("model = %q", got.Model) + } + if got.Client != "codex" { + t.Errorf("client = %q — without it the guard cannot tell agents apart", got.Client) + } + if !SameClient(got.Client, "codex-mcp-client") { + t.Error("the recorded client should match the MCP client that will read it") + } + if SameClient(got.Client, "claude-code") { + t.Error("the recorded client should not match a different agent") + } +} diff --git a/internal/modelhint/modelhint.go b/internal/modelhint/modelhint.go index 37aa47d50..a0d05d682 100644 --- a/internal/modelhint/modelhint.go +++ b/internal/modelhint/modelhint.go @@ -109,6 +109,32 @@ func Write(cwd, model, client string) { } } +// NormalizeClient reduces an agent or MCP client name to a comparable form. +// The two sides of this bridge name the same harness differently: the hook +// layer knows the agent it was invoked for ("codex"), while the MCP session +// knows the client app that connected ("codex-mcp-client"). Comparing them +// raw would reject every legitimate match. +func NormalizeClient(name string) string { + n := strings.ToLower(strings.TrimSpace(name)) + for _, suffix := range []string{"-mcp-client", "-mcp", "-client"} { + n = strings.TrimSuffix(n, suffix) + } + return n +} + +// SameClient reports whether two agent/client names denote the same harness. +// +// An empty side is "unknown" and cannot contradict the other, so it matches. +// That asymmetry is deliberate: a hint with no recorded client predates the +// field, and rejecting it would silently drop attribution that used to work. +// Two *known* names that disagree, though, are proof the hint belongs to a +// different agent — and adopting it there is how a Codex call ends up priced +// as a Claude model. +func SameClient(a, b string) bool { + na, nb := NormalizeClient(a), NormalizeClient(b) + return na == "" || nb == "" || na == nb +} + // Read returns the model hint for `cwd`: the per-cwd announcement when // one exists and is fresh, otherwise the global most-recent hint. The // bool is false when no usable hint is found (none written, all stale,