Repository navigation
Refactor: Share the agentop argument parser and fix four TUI cells - #1659
Conversation
rossoctl#1578 and rossoctl#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 (rossoctl#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 (rossoctl#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 rossoctl#1655): - TestRegDirImportsPluginsOnlyFromTaggedFiles fails on a plugin import in any core/plugins/all file that is not a tagged plugins_<name>.go file. Before, such an import linked into every binary and no test failed. Fixes rossoctl#1578 Fixes rossoctl#1579 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
This is a well-scoped refactor. parseAgentArgs and askYesNo remove five copies of the argument-parsing front half and two copies of the y/N reader, and they keep the per-agent flags and prompts as they were. The claude-code behavior change is documented and covered by tests. The four TUI fixes and the new profile-tags guard each have a test that fails on main. I found one gap: in the dollar case at the clamp value, the AGENTS pane's new + marker does not reach the screen (inline). I also have one documentation nit, below. Neither blocks the merge.
Nit, outside the diff: claudeCodeUsage (cmd_claudecode.go ~L221) and bobUsage (cmd_bob.go L52) still list --yes without the (enable and disable) qualifier that codex, opencode and pi show. status --yes is now a usage error for claude-code, so its help text should say so.
Verification: I checked out the head commit locally. go test passes in cmd/agentop/tui, in the configure, setup and uninstall tests in cmd/agentop, and in scripts/profile-tags. TestReportSessionInterruption fails in my sandbox on both this branch and main, so the failure comes from the environment and not from this PR. CI is green.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop CLI, TUI, profile-tags guard), tests
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing
| return money.Mixed | ||
| } | ||
| // A clamped total is a floor: partialMarker says so, as in sessionMoneyCellIn. | ||
| if c.Saturated { |
There was a problem hiding this comment.
suggestion: In the dollar case, the marker is cut off at the value a saturated total actually has. money.Relabel returns dollars unchanged when the unit is the default, so budget-len(partialMarker) does not shrink anything. At CostMicros = math.MaxInt64, this function returns "$9223372036854.78+" (18 columns). The COST column is agentsCostWidth = 12 wide, and table.go truncates the cell with runewidth.Truncate(value, 12, "…"), so the screen shows $9223372036… and the + is gone. Saturation pins the total at MaxInt64, so this is the main case and not an edge case. The new test passes because it checks the function's return value and not the rendered cell.
sessionMoneyCellIn avoids this with its rung ladder: the compact humanizeCount rung fits the budget. You could reuse that ladder here, or fall back to a compact rendering when lipgloss.Width(out+partialMarker) > budget. A test that asserts lipgloss.Width(cell) <= agentsCostWidth on the saturated case would catch a regression. This is not a regression, because main also overflowed the cell, just without a marker.
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
This is a re-review after a58d3ea1 ("Fit the AGENTS cost cell and its clamp marker in the column"). My earlier review found one problem: the AGENTS pane's + saturation marker was cut off in the 12-column COST cell. That is now fixed. agentCostCellIn sends every positive figure through sessionMoneyCellIn's precision ladder, so the dollar case and the foreign-unit case both fit the column and keep the marker. At math.MaxInt64 the cell reads $9.2T+ in dollars and 9.2T Bobco…+ in credits. On the previous head the credit case showed —, which hid a known cost. Zero stays on the old path, because the ladder treats zero as unpriced. Negative and unpriced figures still return — from agentCostCell before the ladder runs. I found no must-fix issues.
Verification:
- I checked out
a58d3ea1. I ranagentCostCellInover 6 unit sets (nil,USD,Bobcoins, a 22-character unit, a 1-character unit, and mixed), 13 magnitudes from 0 toMaxInt64, and both saturation states, and I rendered each result in the realagentsColumnstable. Every cell is at most 12 columns wide (lipgloss.Width), and the table cut none of them. Every positive saturated figure keeps its+, and figures under$100krender exactly as before. - The rewritten
TestAgentCostCellIn_MarksAClampedTotalnow asserts the display width. When I restored the previous head'sagents_pane.go, it failed in 5 ways: an 18-column$9223372036854.78+, a 14-column$1000000000.00, and—in place of the credit figures. So it guards both gaps. go vetis clean.go testpasses in everycmd/agentopsubpackage and inscripts/profile-tags. In thecmd/agentoproot package,TestReportSessionInterruptionfails in my sandbox, as it did on the last head and onmain, so the cause is the environment.- CI: every check is green except
Go CI (core peerproc, macOS). That job was cancelled at its 15-minute limit, it covers code this PR does not touch, and it passes onmain. A re-run should clear it.
Still open, not blocking (the PR body defers them): a saturated total whose cost is zero shows $0.00 without +. The cell is budgeted against the declared width (12), not the fitted width. The claude-code and bob usage text still lists --yes without "(enable and disable)".
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI AGENTS cost cell, incremental diff since 988f7304; full PR reviewed previously)
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: passing, except the macOS peerproc job, cancelled at its timeout and unrelated to this PR
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
Summary
#1578 and #1579 came from an automated audit. I checked each claim against
main. Most of the large claims did not hold:moneypackage, and each cell has a stated width and precision rule.This PR fixes the real problems only. It also closes the guard gap that the review of #1655 found.
Changes
agentop configure (#1578)
New
parseAgentArgsis the shared front half of the 5configure <agent>commands: help, the verb check, flags,--yesfor enable/disable only, and the home directory. Each agent keeps its own flags and its own work after that.Behavior change for
claude-code. It now parses as the other 4 commands do:status --yesand a stray argument exit 2. Before, they were accepted and ignored.--helpafter the action prints usage and exits 0. Before, it exited 2.No script or doc uses the old behavior. The documented usage does not show
--yesforstatus.New
askYesNois the one y/N parser for setup and uninstall. The prompts and the Enter default of each command are the same as before.agentop TUI (#1579)
+marker, as the sessions pane does. Before, it read as exact.—. Before, it printed$-5.0000. Every other money surface already refused it.truncStris removed. It cut by bytes, so a multi-byte model name or host could be cut in half (invalid UTF-8 on screen). METHOD, PLUGIN and HOST now usetrunc, which counts display columns. ASCII output does not change.localhost.example.comand127.example.comwere hidden as local, and::1showed as egress.profile-tags guard (review of #1655)
TestRegDirImportsPluginsOnlyFromTaggedFilesfails on a plugin import in anycore/plugins/allfile that is not a taggedplugins_<name>.gofile. Before, such an import linked into every binary and no test failed. The reviewer'sextra.gocase now fails with a clear message.Not changed: the sessions pane marks a clamped total with a
+suffix, and the strip, band and drawer use a!prefix. Code comments defend both choices, so this PR does not change them.Test plan
mainand passes with the fix:money_wire_fixes_test.go: the 4 TUI bugsTestRunClaudeCode_ArgumentsParseLikeTheOtherAgents: 4 of its cases fail onmainextra.gocasego test ./...passes incmd/agentop(all packages) andscripts/profile-tags, after a rebase onmaingolangci-lint run --new-from-rev=upstream/mainreports 0 new issuesgofmt,go vet,go mod tidy -diffand pre-commit are clean on the changed filesDeferred (review nits, not fixed in this PR):
$0.00without+. This needs a clamped token count and a zero-rate cost together.+can be lost. This was already true onmain.claude-codeandbobusage text lists--yeswithout "(enable and disable)".Fixes #1578
Fixes #1579
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com