Repository navigation
[Cross-machine spend 5/6] Collect By machine detail on demand - #4841
salmonumbrella wants to merge 8 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex review: needs real behavior proof before merge. What this changesAdds opt-in cross-machine Codex spend to Usage & Spend, including shared-request deduplication, collector-side pricing and partial-source reporting. Example: The supplied synthetic fixture includes local requests, an SSH source, copied requests and an unavailable machine.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Partial Merge readiness⛔ Blocked before merge - 4 items remain Keep open for a maintainer scope decision: this PR adds useful cross-machine accounting, but expands the deliberately token-only contract into pricing, persisted host consent and app presentation. Priority: P2 Decision needed
Before merge
FindingsNone. Provenance
Tests
Agent review detailsHow this fits togetherSelected SSH hosts and configured local Codex histories feed the shared native parser and identity union, which produce priced totals and machine groups for Usage & Spend. flowchart LR
Settings[Saved host selections] --> Controller[Spend dashboard controller]
Controller --> Local[Local native histories]
Controller --> SSH[Existing SSH cost transport]
Local --> Union[Validated identity union]
SSH --> Union
Union --> Pricing[Collector pricing snapshot]
Pricing --> Panel[Machine spend panel]
Technical reviewBest possible solution: Agree and land the prerequisite accounting contracts, then deliver the opt-in app panel through the existing parser, SSH transport and refresh owners. Do we have a high-confidence way to reproduce the issue? This is a capability extension rather than a reproduced bug: pinned main intentionally provides token-only identified subtotals and has no cross-machine spend panel. Is this the best way to solve the issue? Deduplicating before pricing and reusing the existing parser and SSH transport are sound; the expanded accounting and consent contracts need owner direction before this becomes the accepted implementation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5f93dc3068b3. Merge-risk optionsMaintainer options:
Provenance checked
TestingProof path: shipped entry point. SecurityNeeds attention: Existing SSH hardening is preserved, while the new persisted consent boundary needs final-effect verification. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 29, 2026, 7:56 AM ET / 11:56 UTC. |
|
Thanks @salmonumbrella. Here's the product direction for the app side: start with an on-demand "by machine" detail in Usage & Spend that runs the accepted collector (#4836/#4834) when you open or refresh it, with no separate background refresh lifecycle, no persisted automatic SSH collection, and no enlarged local admission path. Counting shared requests once and showing unknown prices honestly are the right pieces to keep. On consent: removing a host from the shared list should clear its cost consent, so re-adding the same alias needs a fresh selection (today |
db29f30 to
9540b18
Compare
|
I cut this down to on-demand By machine detail: open it, press its Refresh, or change the period while it's open. It has no background SSH lifecycle and keeps the collector's local limits. Removing a host clears its consent, with a regression test. Production is now +473/−111 over the pricing head. The body has current-head CLI and app proof, including the full-history mini run with local unavailable and the result labelled partial. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9540b18 to
2ffd6ec
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Commit shared host edits on submit, focus loss, or exit so intermediate keystrokes do not revoke retained machine-spend consent. Request source subtotals only for app consumers and compare compact JSON semantically.
Keep the shared host editor available with monitoring off and show existing validator guidance for invalid committed aliases. Trim duplicate test coverage, exercise the real input path, and add the PR changelog credit.
Consolidate source-resolution cases and keep layer-specific assertions. Wait for cancelled controller tasks to finish before checking late results.
Render the collector's sanitized error in each source row's existing caption. Keep unavailable costs and partial totals while showing the next action.
2ffd6ec to
9b7a0ed
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
What changed
Sources/, including localization.Why
#4833 needs per-machine Codex spend without double counting requests that show up on more than one machine. A background SSH lifecycle and persisted auto-collection made the earlier version much bigger than the feature needs. This version only collects when you're looking at the detail, and shows missing coverage as missing.
Usage
Add SSH aliases under Settings → Menu → Additional SSH hosts. Then open Settings → Usage & Spend → By machine, tick the hosts you want, and press Refresh. The CLI equivalent:
Recording
Historical, from the retired 9540b18 head.
on-demand-walkthrough.mp4
Screenshots
Historical 9540b18 captures, kept for reference:
Current 9b7a0ed captures, using generated fixtures:
Previous 2ffd6ec captures, from before the rebase onto main and the recovery-message fix:
Proof
Current head 9b7a0ed, on fully generated accounting data only. The packaged Mac CLI and app talked over real SSH to an exact-head Linux CLI built separately on the OrbStack Linux VM. Same command shape as below.
--identified-only. Total shows 3 / 33K / $0.12, Partial data. After I restored the current peer and pressed Refresh, Total showed 4 / 44K / $0.16 and the message was gone. Local-missing still shows This Mac Unavailable with its caption and a partial Total. Unknown model shows This Mac 1 / 11K / Unpriced, each host 2 / 22K / $0.08, Total 4 / 44K / Unpriced. App startup stayed collapsed with no SSH before opening the detail.Previous proof on 2ffd6ec, not rerun on 9b7a0ed. The fix only passes the failed source's message into the existing row and doesn't change the collection lifecycle. Same generated-data setup.
Historical proof from the retired head, run on 9540b18 with the packaged CLI on a Mac mini and two dedicated SSH profiles using distinct accounting extracts on the same OrbStack Linux VM:
CodexBar.app/Contents/Helpers/CodexBarCLI cost --provider codex --identified-only --format json --days 7 --identified-end <pinned-ms> --remote <ssh-a> --remote <ssh-b>Redacted app child-process observations and remote CLI receipts:
Tests at 9b7a0ed: 86 tests in 5 suites pass. That's
MachineSpendTests(4) andProviderArchitectureGatekeeperTests(48) natively, plus the portableCodexRequestSubtotalTests(31) and 3 documented/strict CLI invocation tests.make checkis clean (SwiftLint, 0 violations across 3163 files). Codex review was unavailable due to its usage quota.Earlier, at 2ffd6ec:
MachineSpendTests(3) and the portableCodexRequestSubtotalTests(30) pass on macOS, and the portable 30 also pass on Linux aarch64 with Swift 6.3.3. Before the test-only cleanup (+72/−222 in two test files, no production change after), the broader 6 suites / 78 tests passed, includingPreferencesPaneSmokeTests, Fast pricing, Codex session tier and coverage compatibility.Refs #4833, slice 5 (on-demand machine detail)
Follow-up: #4876 adds CLI setup for these SSH hosts and their spend selection.