polish: cut per-turn overhead, stabilize cache, add /new#478
Conversation
Context economy - Compaction: preserveLast 8->6, triggerRatio 0.8->0.7 (both copies in sync). - System prompt: kill the "ELABORATE summary" directive; scale summary to the work. Comment discipline moved into the core prompt for all model families (was Anthropic-only); Minimax/DeepSeek/Qwen/etc. now get it. - Token-budget regression test ratchets the fixed per-turn overhead (base prompt + eager tool schemas) using the runtime's own estimator, so silent prompt/schema growth fails CI. Response quality - defaultResponseStyle balanced->concise. Reversible via /style. Tool discovery - <available_skills> block in the system prompt lists installed skills (name + one-line description) so the model loads the right one on the first call instead of guessing and reading the failure. Budget-capped at 640 bytes with an "...and N more" fallback; renders nothing when no skills are installed (byte-identical to prior prompt). Session lifecycle - /new starts a fresh session in place; previous session stays on disk and is named in the confirmation for /resume. Guards against mid-run reset. - /clear now notes that context persists and points to /new. Performance & caching - partitionTools no longer alpha-sorts the whole tools array every turn, which inserted a loaded deferred tool mid-array and shifted the cached prefix on every load. It now builds append-only regions [eager sorted] + [tool_search] + [loaded deferred sorted] so a mid-session load grows the tail instead of busting the eager prefix. Regression test asserts the eager block precedes any loaded tool. - /context now shows cache hit rate (flags low rate over >5 turns as a possible unstable prefix) and rolling average turn latency with time-to-first-token. Both reset on /new. Measured baselines (auto-mode, no workspace): base prompt 3162 tokens, eager tool schemas 2433 across 12 tools. Budget ceilings set ~10% above. Deliberately deferred (need a decision): inverted safety-policy rewrite (sandbox doesn't catch ~half the B-tier rules), core-tool/web_fetch deferral (activation-semantics blocker), aggressive prompt slim.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR retunes compaction defaults, stabilizes deferred tool partitioning, adds skills metadata to the system prompt, removes Anthropic-specific prompt addendum text, and updates the TUI with ChangesAgent: compaction, tool partitioning, and prompt updates
TUI: /new command, timing, and cache efficiency
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as cli.app/exec
participant PluginActivation
participant AgentOptions
participant buildSystemPrompt
CLI->>PluginActivation: skillInfos(skillsDir)
PluginActivation-->>CLI: []SkillInfo
CLI->>AgentOptions: Options.Skills = skillInfos
AgentOptions->>buildSystemPrompt: buildSystemPrompt(options)
buildSystemPrompt->>buildSystemPrompt: skillsContext(options.Skills)
sequenceDiagram
participant User
participant Model as tui.model
participant Session as startNewSession
User->>Model: submit "/new"
Model->>Model: check pending / compactInFlight
alt run in progress
Model-->>User: system message to cancel first
else idle
Model->>Session: startNewSession()
Session->>Session: clear session state and transcript
Session-->>Model: reset model state
Model-->>User: new-session transcript note
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
internal/cli/plugin_activate.go (1)
62-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSwallowed error from
MergedSkills.
merged, _ := plugins.MergedSkills(...)discards the error entirely, while every other failure path in this file (activatePlugins) surfaces diagnostics viawritePluginActivationWarning. A broken skill file or unreadable skills dir here fails silently — the<available_skills>block just goes missing with no signal to the user.♻️ Proposed fix
-func (a pluginActivation) skillInfos(defaultDir string) []agent.SkillInfo { - merged, _ := plugins.MergedSkills(defaultDir, a.skillRoots) +func (a pluginActivation) skillInfos(defaultDir string, stderr io.Writer) []agent.SkillInfo { + merged, err := plugins.MergedSkills(defaultDir, a.skillRoots) + if err != nil { + writePluginActivationWarning(stderr, "failed to load skills: "+err.Error()) + } if len(merged) == 0 { return nil }Call site in
exec.gowould becomepluginActivation.skillInfos(deps.skillsDir(), stderr).🤖 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 `@internal/cli/plugin_activate.go` around lines 62 - 81, The skill discovery path in pluginActivation.skillInfos is swallowing the error from plugins.MergedSkills, so broken or unreadable skills fail silently. Update skillInfos to accept stderr and surface any MergedSkills failure through writePluginActivationWarning, matching activatePlugins’ other diagnostics, while still returning nil or filtered skills as appropriate; also update the exec.go call site to pass stderr into skillInfos.
🤖 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/session.go`:
- Around line 43-86: The startNewSession flow is not clearing staged attachments
and related UI state, so a fresh /new session can inherit old files. Update
model.startNewSession to also reset pendingImages, pendingImageLabels,
pendingDocuments, queuedMessage, and any stale file/plan display state alongside
the existing session and usage resets. Use the startNewSession method and the
attachment-related fields on model as the place to make the cleanup consistent
for every new session.
---
Nitpick comments:
In `@internal/cli/plugin_activate.go`:
- Around line 62-81: The skill discovery path in pluginActivation.skillInfos is
swallowing the error from plugins.MergedSkills, so broken or unreadable skills
fail silently. Update skillInfos to accept stderr and surface any MergedSkills
failure through writePluginActivationWarning, matching activatePlugins’ other
diagnostics, while still returning nil or filtered skills as appropriate; also
update the exec.go call site to pass stderr into skillInfos.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57b4ef45-be59-4765-9e05-c9f0c78d35d3
📒 Files selected for processing (28)
internal/agent/compaction.gointernal/agent/loop.gointernal/agent/partition_cache_stable_test.gointernal/agent/prompt_budget_test.gointernal/agent/skills_context_test.gointernal/agent/system_prompt.gointernal/agent/system_prompt.mdinternal/agent/system_prompt_models.gointernal/agent/system_prompt_models_test.gointernal/agent/system_prompt_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/plugin_activate.gointernal/sessions/replay.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/commands.gointernal/tui/flush_test.gointernal/tui/latency_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/new_session_test.gointernal/tui/session.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/usage/cache_efficiency_test.gointernal/usage/tracker.go
Resolve the model struct conflict by keeping both sides: main's keyBindings field (reconfigurable shortcuts, #417) and this branch's turn-latency / TTFT counters. All suites pass on the merged tree.
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. |
CodeRabbit: startNewSession reset conversation/usage/latency state but not the staged input (pendingImages, pendingImageLabels, pendingDocuments, queuedMessage), which is only consumed at prompt-submit. Without clearing it, the fresh session's first prompt silently inherited the previous session's images/documents/queued text. Test now stages all four and asserts they clear.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve — a strong, coherent context-economy + observability pass. Two opinionated defaults are yours to consciously accept.
Worth merging: yes, clearly. This isn't churn — it's exactly the kind of substantive improvement Zero wants: it trims the fixed per-turn overhead, lands a real prompt-cache stability fix, adds the observability to see both, and cleans up session lifecycle. Thoroughly tested (7 new test files + updates), fully commented with rationale, and green on all three platforms including the Windows smoke that's been flaky elsewhere. Thanks @anandh8x.
I ground-truthed the load-bearing pieces rather than taking them on faith:
partitionTools cache-stability rewrite (the heart of it) — correct. The old code alpha-sorted the whole tools array every turn, so a newly-loaded deferred tool got inserted mid-array and shifted every later definition — busting the provider's cached prefix from that point through the system prompt and messages, on every load. The new layout [eager sorted] + [tool_search] + [loaded tail sorted] makes a load grow the tail instead. I verified the active/inactive split preserves the prior behavior: the inactive path returns early byte-identically, and on the active path tool_search is unconditionally exposed exactly as before (it was always skipped in the loop, so the old !exposedNames[...] guard was always true anyway). The comment is honest that tool_search's own discovery text still shrinks as tools load, so the eager block is what stays cached, not the tail — a real, partial win, accurately described.
Cache-rate math — safe. CacheHitRate() divides CachedInputTokens/InputTokens; I checked Normalize (tracker.go:181-183) actually clamps cachedInputTokens to inputTokens per record, so the accumulated summary can't exceed 1.0. FormatCacheEfficiency and the empty-input n/a guard line up with the tests.
Skills block — byte-safe. skillsContext returns "" on no skills and buildSystemPrompt skips the append, so a skill-less run reproduces the prior prompt exactly (test covers it). Budget-capped at 640B with a "…and N more" summary, first skill always listed, nameless entries skipped, descriptions truncated at 100 runes. Wired through both TUI (app.go) and exec (exec.go) via MergedSkills — the same set the skill tool resolves against, so it can't advertise a skill the tool can't load. All helpers exist as used.
/new — data-safe. The prior session is already persisted on disk, so /new only clears in-memory state; it refuses mid-run (m.pending || m.compactInFlight) so it can't strand an in-flight turn, names the previous id for /resume, and correctly clears staged input (images/docs/queued message) so nothing leaks into the fresh session. The /clear→"context still there, use /new" note fixes a genuine, common confusion.
Two conscious defaults — flagging, not blocking
These are product-behavior changes, reversible per-session, but they change Zero's out-of-box feel, so they should be a decision, not a side effect:
defaultResponseStylebalanced → concise+ killing the "ELABORATE summary" directive. Together these make Zero terser by default. I think that's the right call for a coding agent, but it's the most user-visible thing in here.compactionTriggerRatio 0.8 → 0.7is self-described as speculative tuning (the author says validate against the latency/compaction metrics before trusting it). Earlier compaction = more frequent summarization. Honestly flagged; just merge it knowing it's a guess to be validated, not a measured value.preserveLast 8→6is synced across both copies (compaction.go + replay.go) — good.
One non-blocking note
It's a wide PR — ~10 distinct concerns across 28 files under one theme. Coherent, but if the speculative 0.7 ratio or the concise default needs backing out, it's entangled with everything else rather than being a clean revert. Not a reason to hold it; just the tradeoff of shipping the pass as one unit.
Net: correct, well-tested, genuinely improves Zero. Approving. The only things to decide rather than verify are the two defaults above.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving — a lot of well-scoped, well-tested work in one pass (prompt slimming, skills-in-prompt, /new, the budget guard, and latency/cache-efficiency visibility). Reviewed each slice with a regression lens and built/tested locally: no existing behavior breaks, and the tool exposure/deferral semantics are provably unchanged. A few notes, none blocking.
Cache stabilization — the scaffolding is right, but the win isn't delivered yet. The per-turn tool list is now correctly append-only ([eager | tool_search | loadedTail]), and the eager block is genuinely byte-stable across a deferred-tool load — that part is real and regression-free. But the actual cache-hit win doesn't materialize as shipped: the tools cache breakpoint sits on the last tool, and tool_search's description still changes on a load (it drops the now-loaded tool) right after the stable eager block, so the reusable prefix still breaks downstream. The loop.go comment discloses this as a scoped follow-up, which is the right call — just flagging that the "stabilize cache" headline is scaffolding-complete but payoff-pending. Consider softening the title, or landing the loader-description follow-up alongside this.
Strengthen partition_cache_stable_test.go. It currently asserts tool ordering (readPos/searchPos < alphaPos), but the property this slice actually claims is a byte-identical tools prefix across a load. A two-call test — partitionTools with loaded={} vs loaded={alpha:true} — asserting the eager + tool_search prefix bytes are identical would pin the real invariant (and would surface the tool_search-description instability noted above).
Smaller nits (optional):
/newdoesn't reset the plan/specialists UI panels, so a pinned plan can linger until the next run. Not a context leak — the agent's in-context history is fully cleared, and/resumebehaves the same — just cosmetic; am.plan = planPanelState{}(+ specialists) would fully honor the "fresh session" promise.compactionTriggerRatio0.7 is flagged "speculative" in the comment — worth validating against the new avg-turn-latency metric before trusting it. The direction is safe (compacts a bit earlier, never risks overflow).- The swallowed error in
plugin_activate.go:68(merged, _ := plugins.MergedSkills(...)) means a broken/unreadable skills dir makes the<available_skills>block silently vanish, while every other path in that file surfaces a warning — worth matching. (The staged-attachments reset is already handled in 416d0aa.)
Nice work overall — the budget guard is hermetic and the /new state reset is thorough. Approving; the cache follow-up and the test strengthening are the two I'd prioritize.
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
LGTM! thank you so much @anandh8x !
4 participants
What's in this PR
Context economy
preserveLast 8→6,triggerRatio 0.8→0.7(both copies in sync).Response quality
defaultResponseStylebalanced→concise. Reversible via/style.Tool discovery
<available_skills>block in the system prompt lists installed skills (name + one-line description) so the model loads the right one on the first call instead of guessing and reading the failure. Budget-capped at 640 bytes with an "...and N more" fallback; renders nothing when no skills are installed (byte-identical to prior prompt).Session lifecycle
/newstarts a fresh session in place; previous session stays on disk and is named in the confirmation for/resume. Guards against mid-run reset./clearnow notes that context persists and points to/new.Performance & caching
partitionToolsno longer alpha-sorts the whole tools array every turn, which inserted a loaded deferred tool mid-array and shifted the cached prefix on every load. It now builds append-only regions[eager sorted] + [tool_search] + [loaded deferred sorted]so a mid-session load grows the tail instead of busting the eager prefix. Regression test asserts the eager block precedes any loaded tool./contextnow shows cache hit rate (flags low rate over >5 turns as a possible unstable prefix) and rolling average turn latency with time-to-first-token. Both reset on/new.Measured baselines
Auto-mode, no workspace: base prompt 3162 tokens, eager tool schemas 2433 across 12 tools. Budget ceilings set ~10% above.
Verification
go build ./...cleango veton all changed packages cleaninternal/agent,internal/tui,internal/usage,internal/cli,internal/sessionsfull test suites greenSummary by CodeRabbit
/contextfooter with rolling latency, time-to-first-token, and cache-efficiency./newcommand to start a fresh session./clearand/newtranscript/session reset behavior, including guidance and safeguards while a run is active.