From 988f73040b670e2978fb8c992b6a8925a84615e7 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 3 Nov 2026 15:46:55 -0500 Subject: [PATCH 1/2] refactor: Share the agentop argument parser and fix four TUI cells #1578 and #1579 came from an automated audit. Their large claims did not hold on main: the money cells already share the money package, each with its own stated width rule, and most truncate and loopback helpers are needed variants. This change keeps only the parts that are real. agentop configure (#1578): - parseAgentArgs is the shared front half of the five configure commands: help, the verb check, the flags, --yes for enable and disable only, and the home directory. Each agent keeps its own flags and its own work after that. - claude-code now parses as the other four do. It refuses status --yes and a stray argument (exit 2), prints usage for --help after the action (exit 0), and checks the action before it reads the home directory. A flag error prints only the error line, without the default flag list. - askYesNo is the one y/N parser for setup and uninstall. The prompts and the Enter default of each are unchanged. agentop TUI (#1579): - AGENTS pane: a clamped (saturated) total shows the + marker, as the sessions pane does. Before, it read as exact. - AGENTS pane: a negative cost shows the dash. Before, it printed "$-5.0000". - truncStr is removed. It cut by bytes, so a multi-byte model name or host could be cut in half on screen. METHOD, PLUGIN and HOST now use trunc, which counts display columns. ASCII output is unchanged. - The timeline's loopback check matches addresses, not prefixes. Before, localhost.example.com and 127.example.com were hidden as local, and ::1 was shown as egress. profile-tags guard (review of #1655): - TestRegDirImportsPluginsOnlyFromTaggedFiles fails on a plugin import in any core/plugins/all file that is not a tagged plugins_.go file. Before, such an import linked into every binary and no test failed. Fixes #1578 Fixes #1579 Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- cmd/agentop/cmd_bob.go | 79 ++++---------------- cmd/agentop/cmd_claudecode.go | 54 ++++---------- cmd/agentop/cmd_codex.go | 57 +++------------ cmd/agentop/cmd_configure_args.go | 92 ++++++++++++++++++++++++ cmd/agentop/cmd_configure_args_test.go | 37 ++++++++++ cmd/agentop/cmd_opencode.go | 56 +++------------ cmd/agentop/cmd_pi.go | 43 ++--------- cmd/agentop/cmd_setup.go | 12 +--- cmd/agentop/cmd_uninstall.go | 12 +--- cmd/agentop/tui/agents_pane.go | 9 ++- cmd/agentop/tui/app.go | 10 +-- cmd/agentop/tui/events_columns.go | 6 +- cmd/agentop/tui/events_columns_test.go | 4 +- cmd/agentop/tui/events_pane.go | 12 +--- cmd/agentop/tui/money_wire_fixes_test.go | 74 +++++++++++++++++++ cmd/agentop/tui/spend_strip.go | 5 +- cmd/agentop/tui/timeline_layout.go | 4 +- scripts/profile-tags/guards_test.go | 31 +++++++- 18 files changed, 305 insertions(+), 292 deletions(-) create mode 100644 cmd/agentop/cmd_configure_args.go create mode 100644 cmd/agentop/cmd_configure_args_test.go create mode 100644 cmd/agentop/tui/money_wire_fixes_test.go diff --git a/cmd/agentop/cmd_bob.go b/cmd/agentop/cmd_bob.go index 48daf72e5..1516994d3 100644 --- a/cmd/agentop/cmd_bob.go +++ b/cmd/agentop/cmd_bob.go @@ -151,72 +151,21 @@ var bobConfirm = confirm // per-half: one consent covers every write the preview lists, and a refusal // declines both halves together. 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 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 the halves' plans document 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, "agentop: 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, "agentop: 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, "agentop: cannot determine your home directory: %v\n", err) - return 1 + // parseAgentArgs checks the verb before any environment or filesystem work, which + // matters most here: a step below can answer successfully on its own (bobSettingsPath's + // off-macOS arm prints advice), so a late check would report success for a verb that + // does not exist. + var settingsPath, cortexCfgPath *string + a, code, done := parseAgentArgs("bob", bobUsage, args, stdout, stderr, func(fs *flag.FlagSet) { + settingsPath = fs.String("settings", "", "IBM Bob settings file") + // For all three verbs: status reports whether the value still matches the proxy + // the config names now, which needs the config. + cortexCfgPath = fs.String("config", "", "Cortex config file") + }) + if done { + return code } + action, yes, home := a.action, a.yes, a.home if *cortexCfgPath == "" { *cortexCfgPath = filepath.Join(home, cortexCfgRel) } diff --git a/cmd/agentop/cmd_claudecode.go b/cmd/agentop/cmd_claudecode.go index 92ce2cdce..453524aca 100644 --- a/cmd/agentop/cmd_claudecode.go +++ b/cmd/agentop/cmd_claudecode.go @@ -224,42 +224,15 @@ Flags: ` func runClaudeCode(args []string, stdout, stderr io.Writer) int { - if len(args) == 0 { - fmt.Fprint(stderr, claudeCodeUsage) - return 2 - } - action := args[0] - // `agentop claude-code --help` used to be read as an action name and fall through to - // the unknown-action branch below, which sends someone looking for the command - // list to the one place that refuses to print it. Same fix, and same reason, as - // `agentop service --help`. - // - // Answered before the flag set is built rather than through fs.Usage: --help asks - // for the whole command's usage, and a flag set named "claude-code --help" would - // print only the flags of an action that does not exist. - // - // Explicit help goes to stdout with exit 0; a missing or bad action keeps going to - // stderr with exit 2. That split is what makes `--help` pipeable. - switch action { - case "-h", "--help", "help": - fmt.Fprint(stdout, claudeCodeUsage) - return 0 - } - - fs := flag.NewFlagSet("claude-code "+action, flag.ContinueOnError) - fs.SetOutput(stderr) - yes := fs.Bool("yes", false, "do not prompt for confirmation") - settingsPath := fs.String("settings", "", "Claude Code settings file") - cortexCfg := fs.String("config", "", "Cortex config file") - if err := fs.Parse(args[1:]); err != nil { - return 2 - } - - home, err := os.UserHomeDir() - if err != nil || home == "" { - fmt.Fprintf(stderr, "agentop: cannot determine your home directory: %v\n", err) - return 1 - } + var settingsPath, cortexCfg *string + a, code, done := parseAgentArgs("claude-code", claudeCodeUsage, args, stdout, stderr, func(fs *flag.FlagSet) { + settingsPath = fs.String("settings", "", "Claude Code settings file") + cortexCfg = fs.String("config", "", "Cortex config file") + }) + if done { + return code + } + action, yes, home := a.action, a.yes, a.home if *settingsPath == "" { *settingsPath = filepath.Join(home, settingsRel) } @@ -270,14 +243,11 @@ func runClaudeCode(args []string, stdout, stderr io.Writer) int { switch action { case "enable": - return claudeCodeEnable2(*settingsPath, *cortexCfg, statePath, *yes, stdout, stderr) + return claudeCodeEnable2(*settingsPath, *cortexCfg, statePath, yes, stdout, stderr) case "disable": - return claudeCodeDisable2(*settingsPath, statePath, *yes, stdout, stderr) - case "status": - return claudeCodeStatus(*settingsPath, stdout) + return claudeCodeDisable2(*settingsPath, statePath, yes, stdout, stderr) default: - fmt.Fprintf(stderr, "agentop: unknown claude-code action %q (enable, disable, status)\n", action) - return 2 + return claudeCodeStatus(*settingsPath, stdout) } } diff --git a/cmd/agentop/cmd_codex.go b/cmd/agentop/cmd_codex.go index 69c4091e2..87e6c3399 100644 --- a/cmd/agentop/cmd_codex.go +++ b/cmd/agentop/cmd_codex.go @@ -1,7 +1,6 @@ package main import ( - "errors" "flag" "fmt" "io" @@ -187,53 +186,15 @@ Flags: ` func runCodex(args []string, stdout, stderr io.Writer) int { - if len(args) == 0 { - fmt.Fprint(stderr, codexUsage) - return 2 - } - action := args[0] - // Help before the verb check, and the verb before anything touches the - // machine: the same order runClaudeCode and runOpenCode use, for their - // reasons. - switch action { - case "-h", "--help", "help": - fmt.Fprint(stdout, codexUsage) - return 0 - case "enable", "disable", "status": - default: - fmt.Fprintf(stderr, "agentop: unknown codex action %q (enable, disable, status)\n", action) - return 2 - } - - fs := flag.NewFlagSet("configure codex "+action, flag.ContinueOnError) - fs.SetOutput(stderr) - fs.Usage = func() {} - envPath := fs.String("env", "", "Codex's dotenv file") - cortexCfgPath := fs.String("config", "", "Cortex config file") - // Not for status, which changes nothing: accepting --yes there would be a - // flag that parses and does nothing, same reasoning as runOpenCode's. - var yesFlag *bool - if action != "status" { - yesFlag = fs.Bool("yes", false, "do not prompt for confirmation") - } - if err := fs.Parse(args[1:]); err != nil { - if errors.Is(err, flag.ErrHelp) { - fmt.Fprint(stdout, codexUsage) - return 0 - } - return 2 - } - if fs.NArg() > 0 { - fmt.Fprintf(stderr, "agentop: codex %s takes no arguments (got %q)\n", action, fs.Arg(0)) - return 2 - } - yes := yesFlag != nil && *yesFlag - - home, err := os.UserHomeDir() - if err != nil || home == "" { - fmt.Fprintf(stderr, "agentop: cannot determine your home directory: %v\n", err) - return 1 - } + var envPath, cortexCfgPath *string + a, code, done := parseAgentArgs("codex", codexUsage, args, stdout, stderr, func(fs *flag.FlagSet) { + envPath = fs.String("env", "", "Codex's dotenv file") + cortexCfgPath = fs.String("config", "", "Cortex config file") + }) + if done { + return code + } + action, yes, home := a.action, a.yes, a.home if *envPath == "" { *envPath = defaultCodexEnvPath(home) } diff --git a/cmd/agentop/cmd_configure_args.go b/cmd/agentop/cmd_configure_args.go new file mode 100644 index 000000000..eddad1ad9 --- /dev/null +++ b/cmd/agentop/cmd_configure_args.go @@ -0,0 +1,92 @@ +package main + +import ( + "bufio" + "errors" + "flag" + "fmt" + "io" + "os" + "strings" +) + +// agentArgs is what parseAgentArgs read from `agentop configure [flags]`. +type agentArgs struct { + action string + yes bool + home string +} + +// parseAgentArgs is the front half every `configure ` command shares: help, the +// verb check, the flags, and the home directory, in that order, so nothing touches the +// machine before the arguments are known good. flags adds the agent's own flags to the +// set; everything after the home directory stays per-agent. +// +// done reports that the command is finished and code is its exit code: explicit help +// goes to stdout with 0, a missing or bad action or flag to stderr with 2, and no home +// directory with 1. --yes exists only for enable and disable, because status changes +// nothing and a flag that parses and does nothing would mislead. +func parseAgentArgs(agent, usage string, args []string, stdout, stderr io.Writer, flags func(*flag.FlagSet)) (a agentArgs, code int, done bool) { + if len(args) == 0 { + _, _ = fmt.Fprint(stderr, usage) + return a, 2, true + } + a.action = args[0] + switch a.action { + case "-h", "--help", "help": + _, _ = fmt.Fprint(stdout, usage) + return a, 0, true + case "enable", "disable", "status": + default: + _, _ = fmt.Fprintf(stderr, "agentop: unknown %s action %q (enable, disable, status)\n", agent, a.action) + return a, 2, true + } + + fs := flag.NewFlagSet("configure "+agent+" "+a.action, flag.ContinueOnError) + fs.SetOutput(stderr) + fs.Usage = func() {} + if flags != nil { + flags(fs) + } + var yes *bool + if a.action != "status" { + yes = fs.Bool("yes", false, "do not prompt for confirmation") + } + if err := fs.Parse(args[1:]); err != nil { + if errors.Is(err, flag.ErrHelp) { + _, _ = fmt.Fprint(stdout, usage) + return a, 0, true + } + return a, 2, true + } + if fs.NArg() > 0 { + _, _ = fmt.Fprintf(stderr, "agentop: %s %s takes no arguments (got %q)\n", agent, a.action, fs.Arg(0)) + return a, 2, true + } + a.yes = yes != nil && *yes + + home, err := os.UserHomeDir() + if err != nil || home == "" { + _, _ = fmt.Fprintf(stderr, "agentop: cannot determine your home directory: %v\n", err) + return a, 1, true + } + a.home = home + return a, 0, false +} + +// askYesNo prints prompt to w and reads one answer from r. y or yes (any case) is yes, and +// an empty line is enterYes. EOF before any answer is never consent. +func askYesNo(r io.Reader, w io.Writer, prompt string, enterYes bool) bool { + _, _ = fmt.Fprint(w, prompt) + line, err := bufio.NewReader(r).ReadString('\n') + if err != nil && line == "" { + return false + } + switch strings.ToLower(strings.TrimSpace(line)) { + case "y", "yes": + return true + case "": + return enterYes + } + return false +} diff --git a/cmd/agentop/cmd_configure_args_test.go b/cmd/agentop/cmd_configure_args_test.go new file mode 100644 index 000000000..0d482bcb9 --- /dev/null +++ b/cmd/agentop/cmd_configure_args_test.go @@ -0,0 +1,37 @@ +package main + +import ( + "bytes" + "strings" + "testing" +) + +// claude-code used to take `status --yes`, a stray argument, and `--help` after the action, +// and checked the action only after the home directory. It now parses as the other agents do. +func TestRunClaudeCode_ArgumentsParseLikeTheOtherAgents(t *testing.T) { + t.Setenv("HOME", "") // a late action check would exit 1 here, not 2 + for _, tc := range []struct { + name string + args []string + code int + onStdout bool + }{ + {name: "no args", args: nil, code: 2}, + {name: "--help", args: []string{"--help"}, code: 0, onStdout: true}, + {name: "help after the action", args: []string{"status", "--help"}, code: 0, onStdout: true}, + {name: "unknown action", args: []string{"enabel"}, code: 2}, + {name: "status --yes", args: []string{"status", "--yes"}, code: 2}, + {name: "a stray argument", args: []string{"enable", "now"}, code: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + var out, errb bytes.Buffer + code := runClaudeCode(tc.args, &out, &errb) + if code != tc.code { + t.Errorf("exit %d, want %d; stderr %q", code, tc.code, errb.String()) + } + if got := strings.Contains(out.String(), claudeCodeUsage); got != tc.onStdout { + t.Errorf("usage on stdout = %v, want %v:\n%s", got, tc.onStdout, out.String()) + } + }) + } +} diff --git a/cmd/agentop/cmd_opencode.go b/cmd/agentop/cmd_opencode.go index 64eb28a96..a1001f9a6 100644 --- a/cmd/agentop/cmd_opencode.go +++ b/cmd/agentop/cmd_opencode.go @@ -2,7 +2,6 @@ package main import ( "encoding/json" - "errors" "flag" "fmt" "io" @@ -121,52 +120,15 @@ Flags: ` func runOpenCode(args []string, stdout, stderr io.Writer) int { - if len(args) == 0 { - fmt.Fprint(stderr, openCodeUsage) - return 2 - } - action := args[0] - // Help before the verb check, and the verb before anything touches the machine: - // the same order as runBob, for its reasons. - switch action { - case "-h", "--help", "help": - fmt.Fprint(stdout, openCodeUsage) - return 0 - case "enable", "disable", "status": - default: - fmt.Fprintf(stderr, "agentop: unknown opencode action %q (enable, disable, status)\n", action) - return 2 - } - - fs := flag.NewFlagSet("configure opencode "+action, flag.ContinueOnError) - fs.SetOutput(stderr) - cortexCfgPath := fs.String("config", "", "Cortex config file") - bin := fs.String("opencode", "", "the opencode CLI") - // Not for status, which changes nothing: accepting --yes there would be a flag - // that parses and does nothing. - var yesFlag *bool - if action != "status" { - yesFlag = fs.Bool("yes", false, "do not prompt for confirmation") - } - fs.Usage = func() {} - if err := fs.Parse(args[1:]); err != nil { - if errors.Is(err, flag.ErrHelp) { - fmt.Fprint(stdout, openCodeUsage) - return 0 - } - return 2 - } - if fs.NArg() > 0 { - fmt.Fprintf(stderr, "agentop: opencode %s takes no arguments (got %q)\n", action, fs.Arg(0)) - return 2 - } - yes := yesFlag != nil && *yesFlag - - home, err := os.UserHomeDir() - if err != nil || home == "" { - fmt.Fprintf(stderr, "agentop: cannot determine your home directory: %v\n", err) - return 1 - } + var cortexCfgPath, bin *string + a, code, done := parseAgentArgs("opencode", openCodeUsage, args, stdout, stderr, func(fs *flag.FlagSet) { + cortexCfgPath = fs.String("config", "", "Cortex config file") + bin = fs.String("opencode", "", "the opencode CLI") + }) + if done { + return code + } + action, yes, home := a.action, a.yes, a.home if *cortexCfgPath == "" { *cortexCfgPath = filepath.Join(home, cortexCfgRel) } diff --git a/cmd/agentop/cmd_pi.go b/cmd/agentop/cmd_pi.go index 2e8659ab6..10d2130aa 100644 --- a/cmd/agentop/cmd_pi.go +++ b/cmd/agentop/cmd_pi.go @@ -2,7 +2,6 @@ package main import ( "errors" - "flag" "fmt" "io" "os" @@ -142,50 +141,18 @@ type piCommand struct { } func runPi(args []string, stdout, stderr io.Writer) int { - if len(args) == 0 { - fmt.Fprint(stderr, piUsage) - return 2 - } - action := args[0] - switch action { - case "-h", "--help", "help": - fmt.Fprint(stdout, piUsage) - return 0 - case "enable", "disable", "status": - default: - fmt.Fprintf(stderr, "agentop: unknown pi action %q (enable, disable, status)\n", action) - return 2 - } - fs := flag.NewFlagSet("configure pi "+action, flag.ContinueOnError) - fs.SetOutput(stderr) - fs.Usage = func() {} - var yesFlag *bool - if action != "status" { - yesFlag = fs.Bool("yes", false, "do not prompt for confirmation") - } - if err := fs.Parse(args[1:]); err != nil { - if errors.Is(err, flag.ErrHelp) { - fmt.Fprint(stdout, piUsage) - return 0 - } - return 2 - } - if fs.NArg() > 0 { - fmt.Fprintf(stderr, "agentop: pi %s takes no arguments (got %q)\n", action, fs.Arg(0)) - return 2 - } - home, err := os.UserHomeDir() - if err != nil || home == "" { - fmt.Fprintf(stderr, "agentop: cannot determine your home directory: %v\n", err) - return 1 + a, code, done := parseAgentArgs("pi", piUsage, args, stdout, stderr, nil) + if done { + return code } + action, home := a.action, a.home if action == "status" { s := piState(home, os.Getenv("SHELL")) fmt.Fprintf(stdout, "%s: %s\n", s.State, s.Detail) return 0 } c := piCommand{home: home, shell: os.Getenv("SHELL"), ext: piExtensionPath(home), - yes: yesFlag != nil && *yesFlag, stdout: stdout, stderr: stderr} + yes: a.yes, stdout: stdout, stderr: stderr} var readErr error c.rc, c.advise, readErr = rcResolve(home, c.shell) if readErr != nil { diff --git a/cmd/agentop/cmd_setup.go b/cmd/agentop/cmd_setup.go index 762a0da75..88a176b75 100644 --- a/cmd/agentop/cmd_setup.go +++ b/cmd/agentop/cmd_setup.go @@ -1,7 +1,6 @@ package main import ( - "bufio" "errors" "flag" "fmt" @@ -90,16 +89,7 @@ var setupSignals = func() (<-chan os.Signal, func()) { // setupConfirmFrom reads the answer. Enter means yes: the user typed the install // command and the list of changes is right above the prompt. EOF is not consent. func setupConfirmFrom(r io.Reader, w io.Writer) bool { - _, _ = fmt.Fprint(w, " Continue? [Y/n] ") - line, err := bufio.NewReader(r).ReadString('\n') - if err != nil && line == "" { - return false - } - switch strings.ToLower(strings.TrimSpace(line)) { - case "", "y", "yes": - return true - } - return false + return askYesNo(r, w, " Continue? [Y/n] ", true) } // parseSetupFlags parses setup's flags. A request for help, -h or --help as a diff --git a/cmd/agentop/cmd_uninstall.go b/cmd/agentop/cmd_uninstall.go index cc3e006c9..a994cfde1 100644 --- a/cmd/agentop/cmd_uninstall.go +++ b/cmd/agentop/cmd_uninstall.go @@ -1,7 +1,6 @@ package main import ( - "bufio" "bytes" "errors" "flag" @@ -148,16 +147,7 @@ var uninstallConfirm = func(w io.Writer) bool { // uninstallConfirmFrom reads the answer. Only y or yes goes ahead: unlike setup's // question, this one deletes, so Enter and EOF are no. func uninstallConfirmFrom(r io.Reader, w io.Writer) bool { - _, _ = fmt.Fprint(w, " Continue? [y/N] ") - line, err := bufio.NewReader(r).ReadString('\n') - if err != nil && line == "" { - return false - } - switch strings.ToLower(strings.TrimSpace(line)) { - case "y", "yes": - return true - } - return false + return askYesNo(r, w, " Continue? [y/N] ", false) } // parseUninstallFlags parses uninstall's flags. -h or --help is flag.ErrHelp; any diff --git a/cmd/agentop/tui/agents_pane.go b/cmd/agentop/tui/agents_pane.go index 5f3fb273d..b09c44e9d 100644 --- a/cmd/agentop/tui/agents_pane.go +++ b/cmd/agentop/tui/agents_pane.go @@ -402,7 +402,7 @@ func (m *model) rebuildAgentsTable() { // can be priced at a rate of zero. Reading the money field instead would collapse "priced, and // it cost nothing" into "not priced". func agentCostCell(c usage.Counts) string { - if c.PricedRequests == 0 { + if c.PricedRequests == 0 || negativeCost(c.CostMicros) { return emptyCell } // formatUSDTotalMicros, not %.2f over micros/1e6: it does the rounding on the integer, so @@ -426,6 +426,13 @@ func agentCostCellIn(c usage.Counts, units []string, budget int) string { if !ok { return money.Mixed } + // A clamped total is a floor: partialMarker says so, as in sessionMoneyCellIn. + if c.Saturated { + if out := money.Relabel(cell, unit, budget-len(partialMarker)); out != "" { + return out + partialMarker + } + return emptyCell + } if out := money.Relabel(cell, unit, budget); out != "" { return out } diff --git a/cmd/agentop/tui/app.go b/cmd/agentop/tui/app.go index c41cf6938..7e352591e 100644 --- a/cmd/agentop/tui/app.go +++ b/cmd/agentop/tui/app.go @@ -2823,18 +2823,14 @@ func (m *model) sessionHeader(id, suffix string) string { // detail pane's inner width. A rune count is a different number the moment the input is not // ASCII — measured, an 11-column budget returned 21 columns of CJK and 14 of emoji. // -// Three production callers: the sessions table's id cell (twice, both hex-ish ids) and the events -// pane's identity block, whose `line` helper wraps the JWT `subject`, `client` and `scopes` claims -// at events_pane.go:1058. Those claims are remote-controlled, so "ASCII in practice" is an -// observation about today's tokens rather than a guarantee. +// Its callers include the events pane's identity block and its METHOD, PLUGIN and HOST cells. +// The JWT claims, the model name and the host are remote-controlled, so "ASCII in practice" is +// an observation about today's traffic rather than a guarantee. // // Latent rather than observed, which is why the fix is a redirect and not a rewrite: on ASCII // this is byte-for-byte what the old rune-counting version returned, so no current caller // changes behaviour (there is a probe for that equivalence in the sessions title tests). The // column-aware implementation lives in sessions_pane.go beside its left-truncating mirror. -// -// truncStr in events_pane.go is the byte-indexed variant for fixed-width ASCII table cells — if -// that ever needs to handle multi-byte input, point it here too. func trunc(s string, n int) string { return truncRight(s, n) } diff --git a/cmd/agentop/tui/events_columns.go b/cmd/agentop/tui/events_columns.go index eeddf7ef8..ba74b8f8e 100644 --- a/cmd/agentop/tui/events_columns.go +++ b/cmd/agentop/tui/events_columns.go @@ -75,7 +75,7 @@ type cellContext struct { // width is the declared width of the column being filled, set per column in // the row loop. // - // PLUGIN and HOST previously hardcoded 18 and 20 in their truncStr calls, + // PLUGIN and HOST previously hardcoded 18 and 20 in their truncation calls, // duplicating the width two lines above them — the same drift actionColWidth and // methodColWidth were named to prevent, in the one file whose premise is that a // column is a single definition. Reading it from here means changing a width @@ -303,7 +303,7 @@ var eventColumns = []eventColumn{ desc: "which plugin acted; blank when none did", cell: func(c cellContext) string { p := c.plugin - return truncStr(p, c.width) + return trunc(p, c.width) }, full: func(c cellContext) string { return c.plugin }, sortKey: func(c cellContext) sortValue { return strKey(c.plugin) }}, @@ -386,7 +386,7 @@ var eventColumns = []eventColumn{ // to be declared but never visible. {id: colHost, width: 20, defaultOn: true, keep: keepHigh, desc: "host the message was sent to", - cell: func(c cellContext) string { return truncStr(c.row.event.Host, c.width) }, + cell: func(c cellContext) string { return trunc(c.row.event.Host, c.width) }, full: func(c cellContext) string { return c.row.event.Host }, // hostOnly, so "api.example.com:443" and "api.example.com" sort together // rather than the port deciding. Full host, not the truncated cell, for the diff --git a/cmd/agentop/tui/events_columns_test.go b/cmd/agentop/tui/events_columns_test.go index af61ed2af..21d37d8cc 100644 --- a/cmd/agentop/tui/events_columns_test.go +++ b/cmd/agentop/tui/events_columns_test.go @@ -791,7 +791,7 @@ func TestColumnPicker_DoesNotReturnAfterAnAsyncPaneChange(t *testing.T) { // Cells must truncate to their own column's declared width, read from // cellContext rather than hardcoded. // -// PLUGIN and HOST previously repeated 18 and 20 in their truncStr calls, +// PLUGIN and HOST previously repeated 18 and 20 in their truncation calls, // duplicating the width two lines above — the same drift actionColWidth and // methodColWidth were named to prevent, in the file whose premise is that a column // is one definition. This fails if the plumbing regresses to a hardcoded number or @@ -816,7 +816,7 @@ func TestEventColumns_CellsTruncateToTheirOwnWidth(t *testing.T) { t.Errorf("%s cell is %d wide, exceeding its column width %d: %q", c.Title, n, c.Width, cell) } // A cell that came back empty for a column that has content to show would be - // the signature of a zero width reaching truncStr. + // the signature of a zero width reaching trunc. if c.Title == "HOST" && n == 0 { t.Error("HOST truncated to nothing; the column width was not plumbed through") } diff --git a/cmd/agentop/tui/events_pane.go b/cmd/agentop/tui/events_pane.go index 01675839c..89ae45987 100644 --- a/cmd/agentop/tui/events_pane.go +++ b/cmd/agentop/tui/events_pane.go @@ -1036,7 +1036,7 @@ func eventMethodValue(e pipeline.SessionEvent) string { // already 22 and 14 after the column shrank, which bubbles hid by re-truncating // each cell at the column width anyway. func eventMethod(e pipeline.SessionEvent) string { - return truncStr(eventMethodValue(e), methodColWidth) + return trunc(eventMethodValue(e), methodColWidth) } func statusCell(e pipeline.SessionEvent) string { @@ -1152,16 +1152,6 @@ func formatDuration(d time.Duration) string { return fmt.Sprintf("%dh%02dm", mins/60, mins%60) } -func truncStr(s string, n int) string { - if len(s) <= n { - return s - } - if n < 2 { - return s[:n] - } - return s[:n-1] + "…" -} - // padLeft right-aligns s in a field of the given display width. Uses // lipgloss.Width (matching padRight in help_overlay.go) so the saving sign // (U+2212, 3 bytes / 1 column) in TOKENS/COST cells doesn't cause padding to diff --git a/cmd/agentop/tui/money_wire_fixes_test.go b/cmd/agentop/tui/money_wire_fixes_test.go new file mode 100644 index 000000000..1820995e4 --- /dev/null +++ b/cmd/agentop/tui/money_wire_fixes_test.go @@ -0,0 +1,74 @@ +package tui + +import ( + "math" + "strings" + "testing" + "unicode/utf8" + + "github.com/charmbracelet/lipgloss" + + "github.com/rossoctl/cortex/core/cost/usage" + "github.com/rossoctl/cortex/core/pipeline" +) + +// A clamped total is a floor, so the AGENTS cell marks it the way the sessions pane does. +func TestAgentCostCellIn_MarksAClampedTotal(t *testing.T) { + c := usage.Counts{Requests: 1, PricedRequests: 1, CostMicros: math.MaxInt64, Saturated: true} + got := agentCostCellIn(c, nil, agentsCostWidth) + if !strings.HasSuffix(got, partialMarker) { + t.Errorf("agentCostCellIn(saturated) = %q, want a %q suffix: a clamped total must not read as exact", got, partialMarker) + } + unclamped := c + unclamped.Saturated = false + if got := agentCostCellIn(unclamped, nil, agentsCostWidth); strings.HasSuffix(got, partialMarker) { + t.Errorf("agentCostCellIn(not saturated) = %q, want no %q suffix", got, partialMarker) + } +} + +// A negative cost is a broken producer, and every other money surface refuses to print one. +func TestAgentCostCell_RefusesANegativeCost(t *testing.T) { + c := usage.Counts{Requests: 1, PricedRequests: 1, CostMicros: -5_000_000} + if got := agentCostCell(c); got != emptyCell { + t.Errorf("agentCostCell(negative) = %q, want %q", got, emptyCell) + } +} + +// METHOD carries the model name from the request body, so its cut must land on a rune +// boundary and fit the column by display width, not by bytes. +func TestEventMethod_TruncatesByDisplayWidth(t *testing.T) { + for _, model := range []string{ + "モデル名モデル名モデル名モデル名モデル名", // 3 bytes and 2 columns per rune + "héllo-wörld-ünïcode-model-name-long", // 2-byte runes, 1 column each + } { + e := pipeline.SessionEvent{Inference: &pipeline.InferenceExtension{Model: model}} + got := eventMethod(e) + if !utf8.ValidString(got) { + t.Errorf("eventMethod(%q) = %q, not valid UTF-8", model, got) + } + if w := lipgloss.Width(got); w > methodColWidth { + t.Errorf("eventMethod(%q) = %q, %d columns wide, want <= %d", model, got, w, methodColWidth) + } + } +} + +// The timeline hides this machine's own hosts only, by address, not by name prefix. +func TestIsLoopback_MatchesAddressesNotPrefixes(t *testing.T) { + for host, want := range map[string]bool{ + "127.0.0.1:47600": true, + "localhost:8080": true, + "localhost": true, + "[::1]": true, + "[::1]:47600": true, + "::1": true, + "127.0.0.2": true, + "localhost.evil.com": false, + "127.evil.com": false, + "api.anthropic.com": false, + "0.0.0.0:80": false, + } { + if got := isLoopback(host); got != want { + t.Errorf("isLoopback(%q) = %v, want %v", host, got, want) + } + } +} diff --git a/cmd/agentop/tui/spend_strip.go b/cmd/agentop/tui/spend_strip.go index 338d164b2..46c59ef72 100644 --- a/cmd/agentop/tui/spend_strip.go +++ b/cmd/agentop/tui/spend_strip.go @@ -480,9 +480,8 @@ func formatSpendAge(d time.Duration) string { // Width arithmetic is lipgloss.Width throughout, never len() and never a rune // count. footer.go:88-92 records the bug that costs: a budget computed in display // columns and then sliced by rune index overflowed on any wide character, -// rendering 55 columns for a 40-column budget. The package's trunc and truncStr -// helpers are both display-cell-unaware for the same reason, so neither is usable -// here. +// rendering 55 columns for a 40-column budget. trunc is not used here: it clips, +// and a clipped figure is a wrong figure. // // Linear rather than a binary search over n: the list is at most five figures // long, and the loop is the specification — "the widest prefix that fits". diff --git a/cmd/agentop/tui/timeline_layout.go b/cmd/agentop/tui/timeline_layout.go index 9a253f5f5..98482673b 100644 --- a/cmd/agentop/tui/timeline_layout.go +++ b/cmd/agentop/tui/timeline_layout.go @@ -627,8 +627,10 @@ func tlAgentRows(tl *timeline.Timeline, ax tlAxis, from, to, now time.Time) []tl } // isLoopback reports a host on this machine: the proxy's own ports and local tools, not egress. +// By address, not by prefix, so localhost.example.com stays egress. The trim is for a bare +// "[::1]", which HostKey leaves once it drops ":443". func isLoopback(host string) bool { - return strings.HasPrefix(host, "127.") || strings.HasPrefix(host, "localhost") || strings.HasPrefix(host, "[::1]") + return isLoopbackHost(strings.Trim(hostOnly(host), "[]")) } // tlHostRows draws each host's requests in flight per cell, by one sweep over its request diff --git a/scripts/profile-tags/guards_test.go b/scripts/profile-tags/guards_test.go index d1f62d810..264328173 100644 --- a/scripts/profile-tags/guards_test.go +++ b/scripts/profile-tags/guards_test.go @@ -127,8 +127,7 @@ func TestNoExcludePluginTagsRemain(t *testing.T) { // TestEveryPluginFileIsTagged closes the gap that would reintroduce the very bug // this convention removes. A file named plugins_foo.go with a blank import and NO -// build directive is skipped by the unconditional-import guard (it matches the -// plugins_ prefix) and invisible to discoverPlugins (no directive to find), so it +// build directive is invisible to discoverPlugins (no directive to find), so it // links its plugin into every artifact unconditionally and nothing reports it. func TestEveryPluginFileIsTagged(t *testing.T) { files, err := pluginFiles(regDir) @@ -151,6 +150,34 @@ func TestEveryPluginFileIsTagged(t *testing.T) { } } +// TestRegDirImportsPluginsOnlyFromTaggedFiles covers the files in regDir that +// TestEveryPluginFileIsTagged does not read. A plugin import in any other file there, +// such as doc.go or a new extra.go, compiles into every binary that imports regDir, +// whatever tags the build names. +func TestRegDirImportsPluginsOnlyFromTaggedFiles(t *testing.T) { + goFiles, err := filepath.Glob(filepath.Join(regDir, "*.go")) + if err != nil { + t.Fatalf("glob: %v", err) + } + if len(goFiles) == 0 { + t.Fatalf("no .go files in %s — the registration package moved and this guard went blind", regDir) + } + for _, f := range goFiles { + base := filepath.Base(f) + if strings.HasSuffix(base, "_test.go") || strings.HasPrefix(base, "plugins_") { + continue + } + data, err := os.ReadFile(f) + if err != nil { + t.Fatalf("read %s: %v", f, err) + } + for _, m := range blankPluginImport.FindAllStringSubmatch(string(data), -1) { + t.Errorf("%s imports plugin %q outside a tagged plugins_.go file, so every "+ + "binary links it unconditionally", f, m[1]) + } + } +} + // TestNoUnconditionalPluginImports guards the hole that shipped in // cortex-envoy and cortex-cpex: both blank-imported plugin packages // straight from main.go, so those plugins could not be excluded by any tag and From a58d3ea1a5fc5a7b879dc36140456b8509ba87fa Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 3 Nov 2026 16:21:12 -0500 Subject: [PATCH 2/2] fix: Fit the AGENTS cost cell and its clamp marker in the column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes review: a clamped AGENTS total rendered 18 columns in a 12-column cell, so the table cut off the "+" marker, and a foreign unit showed "—". Files: - cmd/agentop/tui/agents_pane.go - cmd/agentop/tui/money_wire_fixes_test.go Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- cmd/agentop/tui/agents_pane.go | 18 ++++++------ cmd/agentop/tui/money_wire_fixes_test.go | 36 +++++++++++++++++------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/cmd/agentop/tui/agents_pane.go b/cmd/agentop/tui/agents_pane.go index b09c44e9d..19a211db6 100644 --- a/cmd/agentop/tui/agents_pane.go +++ b/cmd/agentop/tui/agents_pane.go @@ -414,25 +414,23 @@ func agentCostCell(c usage.Counts) string { // agentsCostWidth is the COST column's declared width, the budget a relabelled figure fits. const agentsCostWidth = 12 -// agentCostCellIn is agentCostCell in the agent's own units: dollars (nil, or USD) exactly as -// before, one foreign unit relabelled to fit the column, and more than one withheld as -// money.Mixed — an agent's figure summed across units is not an amount. +// agentCostCellIn is agentCostCell in the agent's own units: dollars (nil, or USD), one foreign +// unit relabelled to fit the column, and more than one withheld as money.Mixed — an agent's +// figure summed across units is not an amount. func agentCostCellIn(c usage.Counts, units []string, budget int) string { cell := agentCostCell(c) if cell == emptyCell { return cell } + // sessionMoneyCellIn fits a large figure and its clamp marker into budget. Zero stays here, + // because that function shows a zero as unpriced. + if c.CostMicros > 0 { + return sessionMoneyCellIn(c.CostMicros, c.Saturated, budget, units) + } unit, ok := money.WindowUnit(units) if !ok { return money.Mixed } - // A clamped total is a floor: partialMarker says so, as in sessionMoneyCellIn. - if c.Saturated { - if out := money.Relabel(cell, unit, budget-len(partialMarker)); out != "" { - return out + partialMarker - } - return emptyCell - } if out := money.Relabel(cell, unit, budget); out != "" { return out } diff --git a/cmd/agentop/tui/money_wire_fixes_test.go b/cmd/agentop/tui/money_wire_fixes_test.go index 1820995e4..051a1ac5e 100644 --- a/cmd/agentop/tui/money_wire_fixes_test.go +++ b/cmd/agentop/tui/money_wire_fixes_test.go @@ -12,17 +12,33 @@ import ( "github.com/rossoctl/cortex/core/pipeline" ) -// A clamped total is a floor, so the AGENTS cell marks it the way the sessions pane does. +// A clamped total is a floor, so the AGENTS cell marks it the way the sessions pane does. The +// cell must fit the column with its marker, because the table cuts a wider cell at the end. func TestAgentCostCellIn_MarksAClampedTotal(t *testing.T) { - c := usage.Counts{Requests: 1, PricedRequests: 1, CostMicros: math.MaxInt64, Saturated: true} - got := agentCostCellIn(c, nil, agentsCostWidth) - if !strings.HasSuffix(got, partialMarker) { - t.Errorf("agentCostCellIn(saturated) = %q, want a %q suffix: a clamped total must not read as exact", got, partialMarker) - } - unclamped := c - unclamped.Saturated = false - if got := agentCostCellIn(unclamped, nil, agentsCostWidth); strings.HasSuffix(got, partialMarker) { - t.Errorf("agentCostCellIn(not saturated) = %q, want no %q suffix", got, partialMarker) + for _, tc := range []struct { + micros int64 + saturated bool + units []string + }{ + {math.MaxInt64, true, nil}, + {math.MaxInt64, true, []string{"Bobcoins"}}, + {12_345_678, true, nil}, + {1_000_000_000_000_000, false, nil}, + {1_000_000_000_000_000, false, []string{"Bobcoins"}}, + {12_345_678, false, nil}, + } { + c := usage.Counts{Requests: 1, PricedRequests: 1, CostMicros: tc.micros, Saturated: tc.saturated} + got := agentCostCellIn(c, tc.units, agentsCostWidth) + if got == emptyCell { + t.Errorf("agentCostCellIn(%d, %v) = %q, want a figure", tc.micros, tc.units, got) + } + if w := lipgloss.Width(got); w > agentsCostWidth { + t.Errorf("agentCostCellIn(%d, %v) = %q, %d columns, want <= %d", tc.micros, tc.units, got, w, agentsCostWidth) + } + if strings.HasSuffix(got, partialMarker) != tc.saturated { + t.Errorf("agentCostCellIn(%d, saturated=%v, %v) = %q, want the %q suffix only when saturated", + tc.micros, tc.saturated, tc.units, got, partialMarker) + } } }