feat: add display hint - #56
Conversation
📝 WalkthroughWalkthroughChangesDisplay hint API and dispatch flow
Sequence Diagram(s)sequenceDiagram
participant Tool
participant BareDispatch
participant ToolDispatchResult
participant LoopObserver
participant ToolPostContext
Tool->>BareDispatch: return ToolOutput with optional display_hint
BareDispatch->>ToolDispatchResult: populate display_hint
BareDispatch->>LoopObserver: invoke on_tool_post
LoopObserver->>ToolPostContext: receive display_hint
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/bare/dispatch.rs (1)
746-755: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce the no-hint invariant for error dispatch results.
Both dispatch paths can propagate
display_hint: Some(...)whenis_erroris true, contradicting the public contract and potentially misdirecting presentation layers.
src/engine/bare/dispatch.rs#L746-L755: setdisplay_hinttoNonewheneverresult.is_erroris true.src/engine/bare/dispatch.rs#L1021-L1032: apply the same filtering when copyingdispatch_resultfrom the middleware pipeline.🤖 Prompt for 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. In `@src/engine/bare/dispatch.rs` around lines 746 - 755, Enforce the no-hint invariant in both dispatch result construction paths: at src/engine/bare/dispatch.rs:746-755, set display_hint to None when result.is_error is true; at src/engine/bare/dispatch.rs:1021-1032, apply the same filtering when copying dispatch_result from the middleware pipeline, while preserving hints for non-error results.
🧹 Nitpick comments (2)
src/engine/bare.rs (1)
2088-2095: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for display-hint propagation.
This fixture only verifies construction with
None; add a test using a non-Nonehint and assert that it survives successful direct/pipeline dispatch and reachesToolPostContext, while error results remainNone.🤖 Prompt for 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. In `@src/engine/bare.rs` around lines 2088 - 2095, Add regression coverage near the existing ToolDispatchResult fixture for a non-None display hint. Exercise both successful direct and pipeline dispatch, assert the hint reaches ToolPostContext unchanged, and verify error results still have display_hint set to None.src/tool.rs (1)
190-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the per-variant
serde(rename)attributes
rename_all = "snake_case"already gives these variants the same tags, so the explicitrename = "..."attrs are redundant and can be removed to reduce maintenance surface.🤖 Prompt for 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. In `@src/tool.rs` around lines 190 - 248, Remove the per-variant serde rename attributes from the DisplayHint enum and rely on its existing #[serde(rename_all = "snake_case")] configuration for serialization tags. Keep all variants, documentation, and enum-level serde attributes unchanged.
🤖 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 `@src/tool.rs`:
- Around line 2064-2065: Remove the duplicate #[cfg(feature = "testing")]
attribute from hinted_dispatch_setup, leaving a single testing configuration
guard.
---
Outside diff comments:
In `@src/engine/bare/dispatch.rs`:
- Around line 746-755: Enforce the no-hint invariant in both dispatch result
construction paths: at src/engine/bare/dispatch.rs:746-755, set display_hint to
None when result.is_error is true; at src/engine/bare/dispatch.rs:1021-1032,
apply the same filtering when copying dispatch_result from the middleware
pipeline, while preserving hints for non-error results.
---
Nitpick comments:
In `@src/engine/bare.rs`:
- Around line 2088-2095: Add regression coverage near the existing
ToolDispatchResult fixture for a non-None display hint. Exercise both successful
direct and pipeline dispatch, assert the hint reaches ToolPostContext unchanged,
and verify error results still have display_hint set to None.
In `@src/tool.rs`:
- Around line 190-248: Remove the per-variant serde rename attributes from the
DisplayHint enum and rely on its existing #[serde(rename_all = "snake_case")]
configuration for serialization tags. Keep all variants, documentation, and
enum-level serde attributes unchanged.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7622e6b2-ff14-4820-a893-354e27a3ff34
📒 Files selected for processing (9)
CHANGELOG.mdsrc/engine/bare.rssrc/engine/bare/dispatch.rssrc/middleware.rssrc/middleware/memoize.rssrc/middleware/unknown_tool.rssrc/middleware/verify.rssrc/observer/context.rssrc/tool.rs
No description provided.