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..19a211db6 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 @@ -414,14 +414,19 @@ 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 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..051a1ac5e --- /dev/null +++ b/cmd/agentop/tui/money_wire_fixes_test.go @@ -0,0 +1,90 @@ +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. 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) { + 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) + } + } +} + +// 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