Centralize agent context planning - #879
Conversation
WalkthroughThe agent now uses a shared ChangesContext planning and diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Run
participant contextPlanner
participant Provider
participant Compaction
participant OnContext
participant Trace
Run->>contextPlanner: Plan messages, tools, and settings
contextPlanner->>Provider: Return completion request
Run->>OnContext: Report context breakdown
Run->>Trace: Emit prefix hash and invalidation reason
Provider-->>Run: Return context-limit error
Run->>Compaction: Summarize and reduce context
Compaction->>contextPlanner: Plan replacement request
contextPlanner->>Provider: Stream replacement request
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: 3
🤖 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/agent/context_planner.go`:
- Around line 46-50: Make prefix planning request-specific while retaining the
configured run planner and its state: update the context-planning path around
buildPromptSubstringsFromParts to accept or safely derive prompt parts from the
current request before computing the fingerprint. In
internal/agent/context_planner.go lines 46-50, use those request-specific parts;
in internal/agent/compaction.go lines 627-631, pass the configured run planner
with compaction-specific prompt parts and record its context evidence before
streaming. Add regression coverage for differing configured/request system
content and compaction planning.
In `@internal/agent/loop.go`:
- Line 323: Update each request-planning path around planner.Plan calls at lines
323, 407, 465, and 966 to retain the returned plan and invoke recordContextPlan
for that plan before streaming or sending the provider request. Ensure
reactive-compaction retries record the retained post-compaction plan, so
OnContext and PrefixHashes correspond to the request actually sent.
In `@internal/trace/emit.go`:
- Line 108: Update the custom encoder containing the PrefixHash
invalidation_reason field to omit that key when p.InvalidationReason is empty,
preserving the omitempty wire contract; retain it when a non-empty reason
exists. Add a regression test covering encoding with an empty invalidation
reason and verifying the field is absent.
🪄 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: 4b1eef45-0317-4db3-bb97-e283e8d1ae88
📒 Files selected for processing (11)
internal/agent/compaction.gointernal/agent/context_measurement.gointernal/agent/context_measurement_test.gointernal/agent/context_planner.gointernal/agent/context_planner_test.gointernal/agent/loop.gointernal/agent/loop_test.gointernal/trace/emit.gointernal/trace/parse.gointernal/trace/trace.gointernal/trace/trace_test.go
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/agent/context_planner.go`:
- Around line 50-54: Deep-copy tool definitions, including each nested
Parameters map, before assigning them to CompletionRequest.Tools in the Plan
flow. Ensure the request owns an independent schema from toolDefs so later
caller mutations cannot diverge from PrefixFingerprint and Breakdown.
🪄 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: e877666c-d646-4079-a646-70d84a965dcd
📒 Files selected for processing (9)
internal/agent/compaction.gointernal/agent/compaction_test.gointernal/agent/context_planner.gointernal/agent/context_planner_test.gointernal/agent/loop.gointernal/agent/types.gointernal/trace/emit.gointernal/trace/recorder.gointernal/trace/trace_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/agent/loop.go
- internal/trace/trace_test.go
- internal/trace/emit.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed as a draft. No blocking findings, and the claims hold where I checked them rather than took them.
The seam is genuinely complete. grep zeroruntime.CompletionRequest{ across internal/agent returns exactly one construction site, inside the planner itself, with every other path going through planner.Plan. That is the strongest evidence the centralization is real rather than partial, and it is the thing most likely to be half-done in a change like this.
Request identity is properly tested. TestContextPlannerPreservesProviderRequest uses reflect.DeepEqual on the whole request, not substring checks, and then verifies deep-copy semantics three levels into the nested map[string]any schema plus the image byte slice. Nested-map aliasing is the classic Go trap in exactly this shape and you tested for it, including that a genuinely changed schema does refresh the snapshot and the fingerprint while leaving the earlier plan's request untouched.
The trace stays content-free. explainPrefixChange returns only initial, unchanged, base_instructions, confirmation_policy, project_context and skills, every one derived from a hash comparison rather than from content. That matters because the trace vocabulary is a published coordination surface and the same property was the review axis on #761.
Planner lifetime checks out too: created per Run at loop.go:186, so previousPrefix starts nil and the first plan reports initial rather than inheriting another session's state.
internal/trace passes. internal/agent fails only on TestEagerToolSchemaTokenBudget, which is the ceiling issue #877 fixes, inherited from main and not yours.
Also worth saying: describing the token and tool-call differences as run variation rather than a guaranteed saving is the right call, and rarer than it should be.
Ready for approval as far as I am concerned once it leaves draft.
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. |
|
@anandh8x reviewed above, and there is nothing blocking in it. If you were holding it in draft waiting on review feedback, take it out whenever you like and I will approve. One thing worth knowing: your only failing test is If you are holding it in draft for a reason of your own, ignore that and carry on. Two things I checked and am confirming back, since they are the claims a reviewer would usually just take on trust. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Re-checked on bea0c214 rather than standing on the earlier read: same commit, now out of draft, up to date with main, and all three smoke jobs green.
Two things I re-verified on this head rather than trusting my own earlier note.
zeroruntime.CompletionRequest{} is constructed in exactly one place in internal/agent now, context_planner.go:61, and everything else goes through planner.Plan. Partial adoption is the usual way a centralization lands half-done, and this one is complete.
explainPrefixChange can only emit fixed literals: initial, unchanged, base_instructions, confirmation_policy, project_context, skills, system_prompt, tools, schema, prefix_changed, comma-joined. No content, no hashes, no paths. That was the review axis on #761 and it holds.
One caveat that is not yours. TestEagerToolSchemaTokenBudget fails on main and therefore on your branch, and CI does not catch it, which is why these checks are green. #877 raises the ceiling and is still waiting on a human approval. Nothing for you to do, but do not read the green tick as that test passing.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving. Reviewed bea0c214, re-confirmed as the live head before posting.
@Vasanthdev2004 already covered seam completeness, the reflect.DeepEqual request-identity test, deep-copy semantics, trace vocabulary, and planner lifetime, and I agree with all of it — I won't restate it. Everything below is what I checked that his review did not.
The central claim holds on the wire, not just in a test
The description says the provider-visible prefix is identical before and after. That is the claim a refactor like this lives or dies on, and a test asserting it is not the same as it being true, so I captured the actual bytes.
I built ff608c78 (the merge base) and bea0c214, pointed both at a recording HTTP provider, and ran the same prompt from the same workspace path and same isolated config dir:
base sha256: 383f7d26920969d1eb67...
pr sha256: 383f7d26920969d1eb67...
byte-identical
Worth noting the first attempt showed a large diff, and it was my harness at fault, not the patch — the two runs used different workspace directories and the system prompt embeds that path. Equal paths, identical bytes. The claim is real.
Main finding: one load-bearing part of the freeze cache key is untested
snapshotTools keys the frozen schemas on ToolsHash + "\x00" + SchemaHash (context_planner.go:87). I checked the key is complete first — ToolDefinition has exactly Name, Description, Parameters, and toolSubstrings fingerprints all three — so invariant #10 is satisfied. No issue there.
But the SchemaHash half is not pinned by any test. Dropping it:
key := fingerprint.ToolsHash // SchemaHash removedleaves go test ./internal/agent/ -run 'Planner|Context' green. That component is load-bearing: with name and description unchanged and only Parameters differing between turns, ToolsHash alone is stable, so the planner would serve the previous turn's frozen schemas and the provider would receive a stale tool surface.
The code is correct — I wrote a throwaway probe and it passes as-written, and fails with the mutation applied, printing the stale schema. So this is a missing guard, not a defect. Twelve lines close it:
func TestSchemaOnlyChangeRefreshesFrozenTools(t *testing.T) {
planner := newContextPlanner(contextPlannerConfig{})
messages := []zeroruntime.Message{{Role: zeroruntime.MessageRoleUser, Content: "hi"}}
base := map[string]any{"type": "object", "properties": map[string]any{"path": map[string]any{"type": "string"}}}
grown := map[string]any{"type": "object", "properties": map[string]any{"path": map[string]any{"type": "string"}, "limit": map[string]any{"type": "number"}}}
// Name and description identical: only the schema moves, so ToolsHash alone
// cannot notice and the freeze must consult SchemaHash.
_ = planner.Plan(messages, []zeroruntime.ToolDefinition{{Name: "read_file", Description: "read a file", Parameters: base}}, "")
plan := planner.Plan(messages, []zeroruntime.ToolDefinition{{Name: "read_file", Description: "read a file", Parameters: grown}}, "")
properties, _ := plan.Request.Tools[0].Parameters["properties"].(map[string]any)
if _, ok := properties["limit"]; !ok {
t.Fatalf("second plan served a stale frozen schema: %#v", plan.Request.Tools[0].Parameters)
}
}For contrast, the deep-copy guard is pinned: replacing copyToolDefinitions(toolDefs) with toolDefs fails TestContextPlannerPreservesProviderRequest immediately. The freeze commit landed five minutes before the first review, so this is the least-examined part of the PR and worth the extra guard.
I also checked the specific risk the freeze introduces — the snapshot is returned by reference and shared across stable turns, so a downstream mutation would corrupt every later request. The only .Tools[...] write anywhere is providers/anthropic/provider.go:406, and it mutates that provider's own mapped.Tools, not the zeroruntime slice. No in-place Parameters[...] writes in any provider. Safe.
Negative space: the seam is agent-loop-scoped, and that boundary is worth stating
zeroruntime.CompletionRequest{ has exactly one construction site inside internal/agent, as noted. Repo-wide there are five more:
internal/tui/session_title.go:147
internal/tui/plan_step_detail.go:376
internal/tui/session_controls.go:925
internal/tui/recap.go:68
internal/cli/workflows.go:980
The description scopes the change to "main agent-loop and automatic-compaction request construction," so this is consistent rather than a gap. I raise it because the stated purpose is a seam for future "relevance selection, deduplication, pruning, and cache-aware context policies" — and when those land in the planner, these five paths will not get them.
The session_controls.go:925 one is the interesting one: #873 unified manual and automatic compaction summarization so both share agent.SummarizeCompactionMessages, while this PR centralizes request construction. Manual compaction still builds its own request in internal/tui. The two unifications stop just short of meeting. Not this PR's job, but with both in flight it is worth someone writing down which side owns that request.
Minor: compaction's fallback planners lose the context window
compaction.go:576 and :591 construct newContextPlanner(contextPlannerConfig{}), while :432 passes the real options.ContextWindow. contextWindow only feeds breakdown.UsedFraction (context_measurement.go:80) and gates no decision, so the blast radius is accounting: any breakdown produced through those paths reports UsedFraction 0. :591 passes a nil recorder so nothing is emitted, and :576 is defensive against a planner that the constructor already sets. Reporting-only and probably unreachable — flagging so it is a deliberate choice rather than a surprise if the accounting is ever consumed.
Verified
- Provider request byte-identical vs merge base, as above.
go test ./internal/agent/ ./internal/trace/— both fully green atbea0c214, includingTestEagerToolSchemaTokenBudget, which was failing when this PR was first reviewed.- The pre-existing-failure claim is accurate.
TestRunDoctorFormatsRedactedProviderDiagnosticsandTestRunDoctorConnectivityProbesProviderfail identically on untouchedff608c78. Checked rather than taken. - Mutation-tested the deep copy (caught) and the schema cache key (survived — the finding above).
- Platform coverage, which the description does not claim: I ran the built binary through complete feature runs on all three OSes — file writes, nested paths, sandbox refusal,
--add-dirgrants, control characters, specialist child processes, andexec_command.
| macOS | Linux | Windows | |
|---|---|---|---|
bea0c214 |
7/7 | 7/7 | 6/7 |
The single Windows failure is exec_command, and it is not from this PR: PowerShell cannot start under the sandbox's WRITE_RESTRICTED token (.NET fails BCrypt.dll init, 0x8007045A), and main fails it identically. Since this PR touches the agent loop's hot path, I wanted real three-OS execution rather than an assumption, and it is clean.
Nothing here blocks. The schema-key test is the one thing I would add before merge, and it is twelve lines.
3 participants
Summary
Why
Zero needs one safe seam for future relevance selection, deduplication, pruning, and cache-aware context policies. This change establishes that seam without filtering context or reducing model capability. It does not claim deterministic token savings by itself.
Evaluation
Validation
make fmt-checkgo vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-static(0 issues)make vulncheck(no vulnerabilities found)git diff HEAD --checkgo test ./...passes except for the existingTestRunDoctorFormatsRedactedProviderDiagnosticsandTestRunDoctorConnectivityProbesProviderfailures ininternal/cli; both reproduce unchanged from the untouched base commit.Summary by CodeRabbit
New Features
Bug Fixes