Unify tool output views - #880
Conversation
WalkthroughTool results now expose finalized model, human, artifact, and diagnostic views. Execution paths finalize outcomes after budgeting and rebudgeting. Agent, CLI, and TUI consumers use the appropriate derived view. ChangesTool outcome finalization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ToolRegistry
participant OutcomeFinalizer
participant AgentLoop
participant CLIWriter
participant TUI
ToolRegistry->>OutcomeFinalizer: processed output and boundary output
OutcomeFinalizer-->>ToolRegistry: finalized ToolOutcome
ToolRegistry->>AgentLoop: finalized tool result
AgentLoop->>AgentLoop: select ModelOutput
AgentLoop->>CLIWriter: model and human views
AgentLoop->>TUI: finalized tool result
TUI->>TUI: select HumanDisplay or ModelOutput
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/types.go`:
- Around line 139-154: Persist the finalized state of ToolOutcome across JSON
serialization by exposing a serialized marker or implementing custom
marshaling/unmarshaling for finalized. Ensure restored outcomes make Finalized()
return true so HumanDisplay(), ModelOutput(), and toolResultDetail() retain
reduced error evidence; add a JSON round-trip regression covering a reduced
error outcome.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ab240c0-d033-48f8-b39f-39a06d4a229c
📒 Files selected for processing (10)
internal/agent/loop.gointernal/agent/types.gointernal/cli/exec_writer.gointernal/tools/output_boundary.gointernal/tools/registry.gointernal/tools/tool_outcome.gointernal/tools/tool_outcome_test.gointernal/tools/types.gointernal/tui/model.gointernal/tui/tool_outcome_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/tool_outcome_test.go`:
- Around line 52-54: Update TestToolResultDetailSurvivesOutcomeJSONRoundTrip to
set TMP and TEMP to t.TempDir() in addition to TMPDIR, ensuring os.TempDir uses
the test-owned directory on Windows for exec_command output spills.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 887dc319-f869-40f9-81e8-f05b45d5f520
📒 Files selected for processing (2)
internal/tools/tool_outcome.gointernal/tui/tool_outcome_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed at 22aac8b0. No blocking findings. Traced the parts where a change like this usually goes wrong rather than reading it top to bottom.
The redaction precondition holds. finalizeToolOutcome documents that boundaryOutput must already be redacted, and the human preview is built from it, so if that were ever false this would put unredacted output in front of a person. Both call sites capture it immediately after scrubResultSecrets: registry.go:145-146 and output_boundary.go:101-102. The ordering is right in both, and it is the thing I would have blocked on.
No silent-empty path. ModelOutput() returns Outcome.ModelView only when finalized and falls back to result.Output otherwise, so a direct Tool.Run caller that never crosses the registry boundary still gets its output. That fallback is what makes the four substitutions in loop.go safe.
Nothing mutates output after the view is frozen. finalizeToolOutcome runs after reduction, budgeting and the ceiling, and the only thing after it in the deferred boundary is CommitFileObservation, which writes Meta keys and nothing else. Worth stating explicitly because the whole design rests on it: once ModelView is set, any later write to result.Output would be invisible to the model, and there is currently no such write.
Hook re-budget does not reset the accounting. RebudgetAfterHook carries previous.Diagnostics.OriginalBytes forward when the outcome was already finalized, so a hook that rewrites output cannot make the original look smaller than it was. Easy thing to get wrong, and it is handled.
Byte-based preview trimming is the right call. boundedHumanOutcomePreview splits head and tail with utf8Prefix/utf8Suffix. Line-based retention is what breaks on minified or single-line files, and this avoids it.
Two things, neither blocking:
human.Preview is cleared when the result is an error and output was not command-reduced. The comment explains it as stopping a success preview from hiding the failure, which is right. But a tool whose Preview IS the error detail loses that detail from the human view while it survives in Output. Intended?
HumanView Display carries json:"humanView,omitempty", and omitempty does nothing for a struct in encoding/json. Harmless, just misleading to the next reader.
One note for your own awareness rather than a review point: observeToolResult now takes ModelOutput(). That feeds the repeated-failure signature in the guardrails, which keys on output text. It is equivalent today because ModelView is assigned from result.Output at finalize, so nothing moves. It stops being equivalent the moment anything sets ModelView to something other than the final Output, and the failure would be silent rather than loud.
I read this rather than ran it, so treat the above as a code review and not a verification that the suite passes.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving at 22aac8b0, re-confirmed as the live head before posting.
@Vasanthdev2004 already traced the redaction precondition, the ModelOutput() fallback, the frozen-view invariant, the hook re-budget, and the byte-based preview — and I agree with all of it. He was explicit that he read it rather than ran it, so everything below is the empirical and negative-space half.
The redaction chokepoint is real, and load-bearing under mutation
This is the property the whole design rests on, so I didn't want to take it from a code read. Two things I verified that a read can't:
The on-disk artifact is redacted, not just the in-memory views. TestRegistryOutcomeRedactsModelHumanAndArtifactViews plants a real ghp_-shaped token, and — the part that matters — os.ReadFiles the artifact off disk and asserts the secret is absent from the actual spill file, not only from ModelView/Preview. That is the right assertion for "exact redacted artifacts."
Breaking the scrub ordering is caught. I moved the boundaryOutput capture one line earlier, before scrubResultSecrets — the exact ordering bug the comment at registry.go:137-138 warns about. The human view then leaks the token and the test fails (human view leaked secret). So the chokepoint isn't just correct today, it's pinned against regression.
Negative space: the self-managed spill path, which the redaction test doesn't cover
The redaction test drives a registry-budgeted fake tool. The real risk sits one path over — bash and exec_command are self-budgeting, and applySelfManagedOutputBudget reuses an existing meta["spill_path"] (output_boundary.go:60-61, 70-71). If either tool wrote that spill during its own Run(), before the registry scrub, the artifact would point at a raw, unredacted file while the model and human views were clean. That is the leak this refactor could plausibly have introduced.
It doesn't, and here's why I'm confident:
bash's registry path (prepareBashOutput,directBudget=false) returns bounded capture and does not callspillBashStreamsor setspill_path— it defers, exactly as its comment atbash.go:439-441claims. The only pre-scrub spill (budgetBashCapture,bash.go:471) is thedirectBudget=truepath, which is direct callers who bypass the registry and its scrub entirely — unchanged by this PR and never model-facing.exec_commandis the same shape:truncateExecOutputSpill(the spilling variant) is reachable only fromexecToolResultWithBudget:431, gated ondirectBudget. The registry path skips it and flows full output to the boundary.- Both
exec_command.goandbash.goare untouched by this PR (git diff origin/main --staton them is empty), so this spill timing is pre-existing. The refactor references the post-scrub spill the boundary already created; it doesn't move the write ahead of the scrub.
So the reuse at applySelfManagedOutputBudget only ever reuses a post-scrub spill on the model-facing path. No leak.
Empirical
go build ./...andGOOS=windows go build ./...clean;internal/tools,internal/agent,internal/tuisuites all green.- The two
internal/clifailures the PR calls pre-existing (TestRunDoctorFormatsRedactedProviderDiagnostics,TestRunDoctorConnectivityProbesProvider) fail identically on untouchedorigin/main, and the PR touches none of the doctor/connectivity files. Claim is accurate. - Model context is unchanged by construction:
ModelViewis assigned fromresult.Outputattool_outcome.go:86, andModelOutput()falls back toresult.Outputwhen unfinalized, so what reaches the provider is byte-identical to before. Vasanth's point aboutobserveToolResultnow keying the failure-signature onModelOutput()is the one thing to keep an eye on — equivalent only whileModelView == Output, which is true today. - Mutation-tested a second guard: gutting the error-preview clearing in
finalizeToolOutcomefailsTestErrorOutcomeUsesRawCommandEvidenceButDropsUnrelatedPreview, so the "a success preview must not hide the failure" behaviour Vasanth asked about is genuinely pinned.
Platform
Since this reworks the tool-result path on the agent's hot path, I ran the real binary through full feature runs — file writes, nested paths, sandbox refusal, --add-dir grants, control-character output, specialist child processes, exec_command:
| macOS | Linux | Windows | |
|---|---|---|---|
22aac8b0 |
7/7 | 7/7 | 6/7 |
The single Windows failure is exec_command, and it is not from this PR — it's the #865 PowerShell-under-restricted-token regression (BCrypt.dll init, 0x8007045A) that this branch inherits from main, which fails it identically. Everything that exercises the reworked outcome path — the write and control-character scenarios especially — is green on all three.
Not blocking, already noted by Vasanth
The omitempty on the HumanView struct field is a no-op in encoding/json (harmless), and the human.Preview = "" on a non-reduced error drops detail for a tool whose Preview is the error — his "intended?" is worth a one-line answer in the thread, but the corpus test shows the reduced-error case keeps its evidence, which is the important one. Neither changes my verdict.
Clean, well-tested, and the security-critical seam holds where I tried to break it. Nice work.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Head is still 22aac8b0, the commit I traced in the review above, so that stands rather than needing a fresh pass: same code, all checks green, and zero commits behind main.
For the record, the parts I verified rather than assumed are unchanged: boundaryOutput is captured immediately after scrubResultSecrets at both call sites, so the "must already be redacted" precondition on finalizeToolOutcome genuinely holds; ModelOutput() falls back to result.Output when unfinalized so a direct Tool.Run caller is never handed an empty view; and nothing mutates output after the view is frozen, since CommitFileObservation writes Meta keys only.
My two non-blocking notes are still open, and neither should hold this up:
human.Preview is cleared when the result is an error and output was not command-reduced. Right for a success preview, but a tool whose Preview IS the error detail loses that from the human view while it survives in Output. Worth a look when you are next in there.
HumanView Display carries json:"humanView,omitempty", and omitempty does nothing for a struct in encoding/json. Harmless, just misleading to the next reader.
One thing I flagged for awareness rather than as a finding, repeated here so it does not get lost in the thread: observeToolResult now takes ModelOutput(), which feeds the repeated-failure signature in the guardrails. It is equivalent today because ModelView is assigned from result.Output at finalize. It stops being equivalent the moment anything sets ModelView to something other than the final Output, and that failure would be silent rather than loud.
|
@kevincodex1 this one is ready whenever you have a minute: approved, While checking the approved-but-unmerged pile I found the rest of it is not actually waiting on you, so it is worth writing down rather than leaving as "kevin has a backlog". Six approved PRs are stuck on something neither you nor their authors can clear by deciding anything: #757, #759, #774, #848, #849 and #850. Each is approved, A fresh push on each branch should make the analysis run and let them merge. I can rebase all six onto main and push, which is the natural way to do it and also picks up whatever they have drifted from. Say the word and I will, or tell me if you would rather the authors did it themselves. Two of those are worth not losing to this: #774 cleans up a daemon child after a startup timeout and #759 moves LSP notifications off the read loop. Both have been sitting since 19 and 20 July on a check that was never going to arrive. |
|
Correcting my comment above: the advice in it was wrong, and I tested it rather than leaving you to find out. I said a fresh push would make the missing analysis run and let those six merge. It does not. I rebased #848 onto current main and pushed it as a single-PR test before touching the others. All seven checks re-ran green on a brand new commit, and it still reports only seven checks and still says What I can state as verified rather than inferred:
So something in branch protection is refusing these that did not refuse #857 and #876, and I cannot see which rule from outside. My guess would be that CodeQL became a required check at some point after those merged, which would make every fork PR unmergeable from that day on, but that is a guess and I have been wrong about this three times today already, so I would rather you read the actual setting than act on my theory. The question that would settle it: are Still true from before: #880 is ready whenever you want it, and #774 and #759 have been sitting since 19 and 20 July. |
4 participants
What changed
ToolOutcomethat separates model-facing output, human-facing detail, recoverable artifacts, and reduction diagnosticsWhy
Tool output previously relied on several loosely related fields and metadata conventions. This made it easy for model context, user-visible detail, and recoverable raw output to drift or accidentally use the wrong representation.
The unified boundary keeps model context compact while retaining useful detailed output for users and exact redacted artifacts for recovery and inspection.
Measured behavior
The corpus boundaries differ slightly, so the primary claim is preservation of the existing token reduction with improved human detail and recoverability, rather than a standalone 2-point performance gain.
Validation
make fmt-checkgo vet ./...internal/tools,internal/agent,internal/tui, andinternal/clitestsgo run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-staticmake vulncheckgit diff HEAD --checkThe full local suite also exercised all affected packages successfully. Existing environment/baseline failures remain in doctor connectivity, worktree-trust end-to-end, and daemon end-to-end tests; this change does not touch those paths.
Summary by CodeRabbit
New Features
Bug Fixes