Reduce compaction context while preserving task state - #873
Conversation
WalkthroughCompaction now projects bounded conversation content before summarization. Task state records constraints, tool failures, approvals, changed files, and artifacts. Session replay and the TUI use normalized messages and shared summarization metadata. ChangesCompaction and runtime evidence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentLoop
participant TaskState
participant SessionReplay
participant TUI
participant SummarizeCompactionMessages
AgentLoop->>TaskState: record tool and permission evidence
TUI->>SessionReplay: plan compaction with SkipPrompt
SessionReplay-->>TUI: normalized compaction messages
TUI->>SummarizeCompactionMessages: projected messages
SummarizeCompactionMessages-->>TUI: summary and truncation metadata
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: 4
🧹 Nitpick comments (5)
internal/agent/compaction_projection.go (2)
77-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the nested emptiness check.
The inner
ifis the only effect of the outerif. One condition states the same rule.♻️ Proposed simplification
- if len(sections) == 0 { - if previousSummary == "" { - return nil - } - } + if len(sections) == 0 && previousSummary == "" { + return nil + }🤖 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/agent/compaction_projection.go` around lines 77 - 81, In the surrounding section-handling logic, collapse the nested checks into a single condition that returns nil when sections is empty and previousSummary is also empty. Preserve the existing behavior for nonempty sections or a nonempty previousSummary.
159-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one UTF-8-safe truncation helper.
clipBytesrepeats the logic ofcapTaskEvidenceininternal/agent/task_state.go(lines 289-299). Both trim, reserve three bytes, walk back to a rune boundary, and append"...". Keep one helper with a byte limit parameter.
clipBytesalso computesend := limit - 3without a floor. If a future caller passes a limit below 3,text[:end]panics. A shared helper with a guarded floor removes that risk.♻️ Proposed shared helper
func clipBytes(text string, limit int) string { text = strings.TrimSpace(text) if len(text) <= limit { return text } - end := limit - 3 - for end > 0 && end < len(text) && text[end]&0xc0 == 0x80 { - end-- - } - return strings.TrimSpace(text[:end]) + "..." + end := max(limit-3, 0) + for end > 0 && !utf8.RuneStart(text[end]) { + end-- + } + return strings.TrimSpace(text[:end]) + "..." }Then call
clipBytes(value, maxTaskEvidenceBytes)fromcapTaskEvidenceand delete the duplicated loop.🤖 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/agent/compaction_projection.go` around lines 159 - 169, Consolidate the duplicated UTF-8-safe truncation logic from clipBytes and capTaskEvidence into one shared byte-limit helper, retaining trimming, ellipsis handling, and rune-boundary safety. Guard the reserved ellipsis calculation so limits below three cannot produce a negative slice index. Update capTaskEvidence to reuse clipBytes with maxTaskEvidenceBytes and remove its duplicated truncation loop.internal/agent/task_state.go (1)
325-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid in-place
values[:0]filtering on stored state.
appendBoundedUniqueandremoveTaskFailurerewrite the caller's backing array. Todaysnapshot()copies every evidence slice, so no external holder observes the mutation. The pattern still couples helper behavior to that copy, andmergeTaskFailuresininternal/agent/compaction_preserve.gorepeats it on parsed prior state. Allocate a new slice in these helpers instead.🤖 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/agent/task_state.go` around lines 325 - 360, Update appendBoundedUnique and removeTaskFailure to allocate fresh result slices instead of filtering through values[:0], preserving the existing ordering and bounded behavior. Also replace the same in-place filtering pattern in mergeTaskFailures with newly allocated slices so stored or parsed state is never mutated through shared backing arrays.internal/agent/task_state_test.go (1)
145-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where a different target succeeds.
The retry reuses the same arguments, so this test passes with or without argument identity in the failure key. Add a second failure from a non-command tool, for example
read_fileona.go, then a success onb.go, and assert the failure fora.goremains. That case pins the fix for the key collision reported oninternal/agent/task_state.golines 234-247.As per coding guidelines: "add a regression test for behavior changes".
🤖 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/agent/task_state_test.go` around lines 145 - 151, Extend the test around taskStateEventToolResult to add a read_file failure for a.go, followed by a successful read_file result for b.go with different arguments. Assert that the unresolved failure for a.go remains while the b.go failure is resolved, covering argument identity in the failure key and guarding the behavior in task state failure tracking.Source: Coding guidelines
internal/agent/compaction_preserve_test.go (1)
130-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
slices.Containsinstead of a local helper.
go.moddeclares Go 1.26.5, soslicesis available. RemovecontainsStringand callslices.Contains(state.Task.Constraints, want).🤖 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/agent/compaction_preserve_test.go` around lines 130 - 137, Remove the local containsString helper and replace its call sites with slices.Contains(state.Task.Constraints, want). Add the slices import if needed, preserving the existing containment behavior.
🤖 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/compaction_preserve.go`:
- Around line 577-589: Update mergeTaskFailures to deduplicate entries by
taskFailureState.Key, with newer entries replacing older ones while preserving
maxTaskEvidenceEntries; avoid mutating the input older slice by building
filtered state in newly allocated storage. Add a regression test in
compaction_preserve_test.go covering repeated compaction of the same failing
command with different summaries and asserting only the newest failure remains.
In `@internal/agent/loop.go`:
- Around line 190-196: Update the OnPermission wrapper in the task execution
flow to avoid calling task.observe from parallel read-batch worker goroutines.
Move permission-state observation to a synchronized non-worker path, or protect
the relevant task state with dedicated locking, while preserving invocation of
the original onPermission callback.
In `@internal/agent/task_state.go`:
- Around line 310-323: Update extractExplicitConstraints to collect every
matching line in order instead of returning after the first match, stopping once
maxTaskConstraints is reached. Preserve the existing line filters and return
behavior, and revise the relevant task-state test to assert that multiple
explicit constraints are retained.
- Around line 234-247: Update observeToolEvidence so failures from tools without
a command field remain distinguishable by including a stable, discriminating
argument or target in key; alternatively, only resolve existing failures when
command is non-empty. Preserve separate failure entries for different inputs to
the same tool, and pass the already-capped command directly into
taskFailureState.Command-related data instead of applying capTaskEvidence twice.
---
Nitpick comments:
In `@internal/agent/compaction_preserve_test.go`:
- Around line 130-137: Remove the local containsString helper and replace its
call sites with slices.Contains(state.Task.Constraints, want). Add the slices
import if needed, preserving the existing containment behavior.
In `@internal/agent/compaction_projection.go`:
- Around line 77-81: In the surrounding section-handling logic, collapse the
nested checks into a single condition that returns nil when sections is empty
and previousSummary is also empty. Preserve the existing behavior for nonempty
sections or a nonempty previousSummary.
- Around line 159-169: Consolidate the duplicated UTF-8-safe truncation logic
from clipBytes and capTaskEvidence into one shared byte-limit helper, retaining
trimming, ellipsis handling, and rune-boundary safety. Guard the reserved
ellipsis calculation so limits below three cannot produce a negative slice
index. Update capTaskEvidence to reuse clipBytes with maxTaskEvidenceBytes and
remove its duplicated truncation loop.
In `@internal/agent/task_state_test.go`:
- Around line 145-151: Extend the test around taskStateEventToolResult to add a
read_file failure for a.go, followed by a successful read_file result for b.go
with different arguments. Assert that the unresolved failure for a.go remains
while the b.go failure is resolved, covering argument identity in the failure
key and guarding the behavior in task state failure tracking.
In `@internal/agent/task_state.go`:
- Around line 325-360: Update appendBoundedUnique and removeTaskFailure to
allocate fresh result slices instead of filtering through values[:0], preserving
the existing ordering and bounded behavior. Also replace the same in-place
filtering pattern in mergeTaskFailures with newly allocated slices so stored or
parsed state is never mutated through shared backing arrays.
🪄 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: 3b3c138c-5881-4657-b4ff-accb0f6f9203
📒 Files selected for processing (10)
internal/agent/compaction.gointernal/agent/compaction_metadata_test.gointernal/agent/compaction_preserve.gointernal/agent/compaction_preserve_test.gointernal/agent/compaction_projection.gointernal/agent/compaction_projection_test.gointernal/agent/compaction_test.gointernal/agent/loop.gointernal/agent/task_state.gointernal/agent/task_state_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. |
|
@coderabbitai please review the previously skipped or unchecked areas on the latest head, including the remaining |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and review state =='
gh pr view 873 --repo Gitlawb/zero --json headRefOid,reviewDecision,reviews,comments
echo '== Changed files and diff =='
git diff --stat HEAD^ HEAD
git diff -- internal/agent/loop.go internal/agent/task_state.go internal/agent/compaction.go internal/agent/compaction_projection.go internal/agent/compaction_preserve.go
echo '== Relevant declarations =='
ast-grep outline internal/agent/loop.go --items all --match 'Run|OnPermission|callbackMutex'
ast-grep outline internal/agent/task_state.go --items all --match 'TaskState|ObservePermission|Snapshot|ObserveTool'
ast-grep outline internal/agent/compaction_projection.go --items all
ast-grep outline internal/agent/compaction_preserve.go --items all
echo '== Synchronization and callback call sites =='
rg -n -C 6 --type go 'callbackMutex|OnPermission|ObservePermission|ObserveTool(Result)?|taskState' internal/agent
echo '== Test coverage related to permission and state =='
rg -n -C 4 --type go 'OnPermission|ObservePermission|callbackMutex|TaskStateRecordsDurableRuntimeEvidence|RepeatedCompaction' internal/agent
echo '== Open CodeRabbit review threads/comments =='
gh api repos/Gitlawb/zero/pulls/873/comments --paginate \
--jq '.[] | select(.user.login == "coderabbitai") | {id, path, line, side, body, created_at}'Length of output: 195155 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='Gitlawb/zero'
pr=873
echo '== PR base and exact changed files =='
gh pr view "$pr" --repo "$repo" --json baseRefOid,headRefOid,files
base=$(gh pr view "$pr" --repo "$repo" --json baseRefOid --jq '.baseRefOid')
head=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
printf 'base=%s\nhead=%s\n' "$base" "$head"
git diff --stat "$base" "$head"
git diff --check "$base" "$head"
git diff "$base" "$head" -- \
internal/agent/loop.go \
internal/agent/parallel_tools.go \
internal/agent/task_state.go \
internal/agent/compaction.go \
internal/agent/compaction_projection.go \
internal/agent/compaction_preserve.go \
internal/agent/compaction_projection_test.go \
internal/agent/compaction_preserve_test.go \
internal/agent/task_state_test.go
echo '== Parallel batch lifecycle and callback wrappers =='
sed -n '145,245p' internal/agent/parallel_tools.go
sed -n '180,205p' internal/agent/loop.go
sed -n '1390,1440p' internal/agent/loop.go
sed -n '2348,2492p' internal/agent/loop.go
echo '== Task-state locking and snapshot mutation paths =='
sed -n '115,225p' internal/agent/task_state.go
sed -n '230,375p' internal/agent/task_state.go
sed -n '420,450p' internal/agent/task_state.go
echo '== Unresolved review threads at the current PR head =='
gh api graphql -F owner='Gitlawb' -F name='zero' -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 20) {
nodes { author { login } body url }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| {path, line, comments: [.comments.nodes[] | select(.author.login == "coderabbitai") | {body, url}]}'Length of output: 62968
I found no remaining actionable issue. The The remaining I approve the PR and clear the existing review findings. [resolve] You are interacting with an AI system. |
|
@coderabbitai you confirmed there are no remaining actionable issues and stated that you approve, but GitHub still shows your original |
|
I submit the formal approval and resolve the stale CodeRabbit review thread now. ✅ Action performedComments resolved and changes approved. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Request changes — one blocker, everything else is strong
The architecture is right and the measurement is honest work: projecting the middle to a bounded semantic transcript instead of shipping 617KB to a summarizer that returns 193 tokens either way is clearly correct, and the fail-open path (if len(summaryInput) == 0 { summaryInput = middle }) is exactly the right shape for a projection that could over-trim. I checked that out and ran it rather than reading it; go test ./internal/agent/ is green.
Blocker: isLikelyToolError drops most real failures, including Zero's own
The PR's contract says it retains "concise failures" and "unresolved failures". It largely does not, because isLikelyToolError matches only five exact prefixes (error:, tool error:, failed:, permission denied, command failed) and anything unmatched is treated as success and discarded entirely.
I ran it against real error strings:
"Error: Invalid arguments for view_image" kept=true
"Error viewing shot.png: file is too large" kept=false <-- zero's own tool error
"--- FAIL: TestFoo (0.01s)" kept=false <-- go test
"exit status 1" kept=false
"panic: runtime error: index out of range" kept=false
"fatal: not a git repository" kept=false <-- git
"The model kimi-k2.6 does not exist" kept=false <-- provider
"read_file: no such file or directory" kept=false
The first kept=false is the one that decides it. grep -rh 'errorResult("' internal/tools/ shows this repo emits "Error applying patch: " and "Error editing " — the Error <verb> shape, not Error: — so Zero's own edit and patch failures are dropped as if they had succeeded. A summary that silently loses "the patch did not apply" is worse than a long one, and the loss is invisible: nothing downstream can tell a dropped error from a tool that worked.
Two ways out, your call:
- Cheap: widen the matcher and invert the default. Keep a tool result when it looks like an error or is short — a retained non-error costs a few dozen bytes, a dropped error costs the summary its most important content. Right now the tie goes the expensive way.
- Proper: plumb the structural signal.
tools.Resultalready knowsStatusError;zeroruntime.Messagehas no error field, which is why prose matching is the only option here — so this is a real constraint you worked within, not an oversight. But matching prose for a decision this consequential is the shapeAGENTS.mdinvariant 2 warns about, and aIsError boolon the tool message would end it permanently.
There is also no test asserting an error survives projection — compaction_projection_test.go covers user/assistant/tool-call shaping but nothing pins the [tool_error] path, which is why this gap is invisible to CI.
What I checked and found correct
- Fail-open: an empty projection falls back to the original
middle, so an over-aggressive projection degrades to today's behaviour instead of summarising nothing. - Previous summary carried separately rather than re-summarised, so repeated compaction does not telescope older decisions into noise.
- Per-turn tool-call cap keeps the LAST N and says how many were omitted — the right end to keep, since the recent calls are the ones the summary needs.
- Ordering and budgets are deterministic, so the same transcript projects to the same brief.
One caveat on the benchmark, not a blocker
The 99.75% reduction is real and I do not doubt it. What the numbers do not establish is summary quality: both versions produce ~193 tokens, and the question is whether the 193 tokens produced from a 1,435-byte brief are as good as the 193 from 617KB. The replay assertion checks that goal, decision, failure, plan and tail survive on one frozen corpus — worth saying plainly in the PR that this is a single-corpus structural check rather than a quality comparison across varied sessions, so nobody reads the table as proving more than it does.
Fix the error heuristic and I would approve this.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve — the blocker is fixed, and fixed the right way
Re-reviewed at b01dba28. You took the structural option rather than widening the prose matcher, which is the one that actually ends this class of bug.
The chain is complete and I traced all three links:
zeroruntime.MessagegainsIsError bool— the field whose absence was the reason prose matching existed at allloop.gosets it fromtoolResult.Status == tools.StatusError, i.e. from the same authority the tool itself reported- the projection reads
!message.IsError && !isLikelyToolError(...), keeping the text check only as a fallback for session history written before the field existed — which is the right call, since old transcripts on disk have no flag to read
Both tool-result construction sites are covered. grep MessageRoleTool in internal/agent finds exactly two (loop.go:722 and :3286) and both set IsError, so there is no third path quietly producing an unflagged error message.
Verified against the exact strings from my last review, now carried as structured errors — every one survives:
"Error viewing shot.png: file is too large" kept=true <-- zero's own tool error
"--- FAIL: TestFoo (0.01s)" kept=true
"exit status 1" kept=true
"panic: runtime error: index out of range" kept=true
"fatal: not a git repository" kept=true
"The model kimi-k2.6 does not exist" kept=true
And the projection still projects: a 500-line successful read_file result is dropped, so the 99.75% reduction the PR is for is intact. go test ./internal/agent/ ./internal/zeroruntime/ green.
Everything I approved of last time still holds — the fail-open fallback to middle, the previous summary carried separately, keeping the last N tool calls rather than the first.
My earlier caveat about the benchmark stands unchanged and is still not a blocker: the table measures input size, not summary quality, and the replay assertion is a structural check on one frozen corpus. Worth a sentence in the PR description so the numbers are not read as proving more than they do.
Nice fix — the version that plumbs the status is strictly better than the one I suggested as the cheap option.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes. The direction is right and the benchmark is real, but the projection drops several things that are not reconstructible, and one claim in the description does not hold.
ask_user answers are discarded. executeAskUser returns Status: tools.StatusOK (loop.go:2039), so IsError is false, so the tool branch at compaction_projection.go:70 drops the body. That body is the user's own typed answer. If the user says "Postgres only, the MySQL branch is dead, do not touch it", that sentence is gone after compaction: the summarizer never sees it, and explicitConstraintsFromMessages cannot rescue it because that only scans user-role messages. The question is lost too, since compactionToolCallLine mines only file_path/path/query/pattern and cmd/command/script/shell, so an ask_user call renders as a bare * ask_user. This is the one I would fix first. It is the opposite of reconstructible.
/compact never reaches any of this. The description says the behaviour applies to "automatic context-pressure compaction and manual /compact", and lists manual /compact as validation. projectCompactionInput has exactly one call site, compaction.go:219, and nothing outside internal/agent reaches Compact/CompactMessages. The TUI path goes handleCompactCommand to compactActiveSession to PlanCompaction, a separate summarizer. Either wire it up or drop the claim, but as written the manual verification exercised code this PR does not touch.
apply_patch leaves no trace. Its file path lives only in the patch argument, which is not in the allowlist, and recentEdits matches only write_file/edit_file (compaction_preserve.go:119). So after compaction there is no record of which files were patched, in the brief or in preserved state. The same allowlist gap blanks web_fetch's URL and task's prompt.
The 120-line cap drops the oldest content, which is the part the summary exists to carry. capCompactionBrief keeps the last 120 lines, and the cap binds on essentially every real compaction, so the summarizer input sits around 2 KB regardless of whether the history is 24k or 1.3M tokens. Structured state does survive, so plan, edits, skills, instructions, approvals and the original run prompt are fine. What is never summarized even once is the older action record and assistant reasoning. Pre-PR the whole middle reached the summarizer and summarizeWithFallback halved it recursively when oversized; that path is now unreachable because the projection always returns exactly one message and the fallback bails at len(messages) < 2.
Two smaller ones. ResolvedFailureKeys is never cleared, so once a command has succeeded, any later failure of that same command is filtered out of preserved state for the rest of the run: the fail, fix, change, fail-again shape reports a green suite. And the carried-forward previous summary is clipped to 400 informative words each round, keeping the head, so the newest material in it decays away over repeated compaction.
To be clear about what holds up: the preservation machinery genuinely works. I checked objective, plan, skills, tool schemas and project instructions surviving repeated compaction, and the fourteen tests are real tests, not shape checks. Checking your three unrelated failures against the untouched baseline before claiming they were unrelated is exactly right.
|
@anandh8x that review is long, so here is the order I would take them in. ask_user first. It is the only one where the lost content is the user's own words and nothing can regenerate it, and the fix looks small: that result simply is not reconstructible, so it should not sit in the reconstructible bucket. The The rest can follow. apply_patch and the argument allowlist are mechanical, Worth saying plainly: the preservation machinery holds up. I checked objective, plan, skills, tool schemas and project instructions surviving repeated compaction and they all do, and your fourteen tests are real tests rather than shape checks. Also, my own first finding was wrong and I nearly sent it. I probed |
8b45d99
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Reviewed 8b45d995 ("Unify manual and automatic compaction"), which landed after my approval and dismissed it. This is a much larger commit than the previous two — +467/-77 across 14 files, reaching into internal/sessions and internal/tui for the first time in this PR.
The core of it is right, and it is the fix I'd have asked for if I'd seen the manual path first. /compact used to build its summary from shapedPayloadPreview, which is an allow-list — EventToolCall kept only id/name/toolName, EventToolResult only those plus status. The arguments and output fields never survived. So the manual summarizer was being handed a list of tool names and statuses and asked to write a useful summary of what happened. CompactionMessages + SummarizeCompactionMessages puts the real content through the same projection the automatic path uses, and TestManualCompaction... pins the join end-to-end by asserting the request carries agent.CompactionSummaryInstructions and that the ask_user question and answer both survive. Good test — it asserts the wire content, not struct fields.
I checked the obvious worry and it is not a problem: reading output/content/arguments raw out of the event payload is not a redaction bypass. Redaction already happened at the tool chokepoint before the event was written — loop.go scrubs result.Output and records result.Redacted, and tui/model.go:5439 persists that already-scrubbed value. See the doc note below, though.
One thing I'd fix before merge
plan.Truncated is derived by string-matching a marker that lives in another package — internal/tui/session_controls.go:923:
truncated = truncated || strings.Contains(message.Content, "omitted to fit compaction budget") ||
strings.Contains(message.Content, "[middle omitted]")Both literals are inline strings in internal/agent/compaction_projection.go:104 and :207. Two packages, two copies of one fact, nothing asserting they agree — invariant #5.
I proved the drift is undetected. Rewording only the agent-side marker, exactly as a future refactor would:
compaction_projection.go:104 "...[transcript trimmed to fit the compaction budget]..."
compaction_projection.go:207 "\n...[trimmed]...\n"
go test ./internal/tui/ -count=1 → ok, whole suite green. plan.Truncated is now permanently false, and since RecordCompaction persists it (replay.go:223), the durable session record claims nothing was dropped when material was. The new TestStoreCanPlanCompactionWithoutBuildingLegacyPrompt only asserts !plan.Truncated on a three-event session, so nothing covers the true direction. It fails the other way too: a compacted message whose text happens to contain [middle omitted] flips the flag on.
The fix is the same move you already made elsewhere in this PR. Replacing isLikelyToolError's prose sniffing with the structural IsError field was exactly right; this is the same prose-sniffing pattern one layer up. Have SummarizeCompactionMessages return whether the projection truncated — it is the only code that knows — instead of leaving the caller to recognise its output by sight.
Smaller notes, none blocking
The event log is now read twice per /compact. PlanCompaction calls store.ReadEvents at replay.go:151, then compactActiveSession calls it again at session_controls.go:859 to resolve the same refs. Long sessions pay double I/O and double JSON parse for one compaction. Returning the events from PlanCompaction, or a variant that does, would avoid it. sessionEventsForRefs itself is good — checking Sequence and Type alongside the ID makes it a real clash guard rather than a lookup, and failing with "retry /compact" is the right call.
The two new boundary clippers lack the guard you just added to clipBytes. This commit hardened clipBytes for limit <= 0 and limit <= 3, but clipPrefixAtBoundary and clipSuffixAtBoundary right below it take a negative limit straight into text[:end], which panics. Unreachable today — both callers derive the limit from constants — so this is purely about the next person who lowers compactionBriefMaxBytes. Worth matching the hardening you applied twenty lines up.
PromptChars quietly changed meaning. It used to be len(prompt) for the whole built prompt; it is now the sum of len(message.Content) across projected messages, which excludes the system instruction that StreamCompletion actually sends. Small, but it is a recorded metric, so a comment saying which one it counts would keep later comparisons honest.
The projection budget grew a lot, and the PR title says the opposite. compactionBriefMaxLines = 120 became compactionBriefMaxBytes = 24KB, compactionPreviousSummaryWords = 400 became compactionPreviousSummaryBytes = 16KB, and mustPreserve now admits every successful mutating tool result rather than only errors. Against a raw transcript this is still a large reduction, so I don't think it's wrong — the changed-file evidence is the point of the PR. But whatever numbers the description quotes for "reduce compaction context" were measured before this commit. Worth re-measuring so the claim matches the code.
CompactionMessages' doc comment is inaccurate in a security-relevant way. It says "security-sensitive event payloads use the same redacted previews as the session compaction plan." Only the default branch and ask_user_answers do; the three shaped branches read payload fields directly. As above that is safe, because redaction happened upstream at the tool chokepoint — but the comment states the wrong reason, and someone adding a fourth shaped branch would reasonably conclude they inherit redaction from this function and skip checking their source. Please state the invariant it actually relies on.
Verified
go test ./internal/agent/ ./internal/sessions/ ./internal/tui/ ./internal/zeroruntime/ -count=1— all green at8b45d995.- Mutation-checked the marker coupling as described above; tree restored clean afterward.
- Traced
ChangedFilesfromtools.Resultthroughloop.go:723andcopyMessagesintorecentEdits. Checked the ordering concern raised by the secondforloop over messages — it is not a defect, becausewrite_file/edit_filesetChangedFilestoo, so their paths reappear in the second pass andlastSeenOrderstill lands everything chronologically. The first loop is now near-redundant except forpathByIDand failed edits. removeStringintask_state.go:245is a genuine fix: a recurring failure previously stayed inResolvedFailureKeyswhile also sitting inUnresolvedFailures.
Not blocking on any of this — today's behavior is correct in every case above. The truncation-marker coupling is the one I'd want closed before merge, since it is cheap and it is a named recurring defect class in this repo. Happy to re-approve as soon as that's addressed.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving.
To put a formal verdict on the review above: nothing in 8b45d995 is wrong today. The redaction question I chased turned out clean, the ordering concern in recentEdits dissolved on inspection, removeString is a real fix, and all four affected packages test green. The commit repairs a manual /compact that was genuinely broken — summarizing from an allow-listed preview that had dropped every arguments and output field — and it does so by reusing the automatic path rather than growing a second one. That is the right shape.
The one item I'd still like closed is the truncation marker: plan.Truncated is recognised by string-matching literals that live in internal/agent, and I showed that rewording them leaves the whole tui suite green while the persisted session record silently starts claiming nothing was dropped. It is a latent-drift risk rather than a live bug, which is why it isn't blocking — but it is invariant #5, it is cheap, and SummarizeCompactionMessages returning the fact is strictly less code than sniffing for it downstream. Please take it either in this PR or as an immediate follow-up; if you'd rather not grow this commit further, a follow-up issue is fine by me.
The four smaller notes — the double ReadEvents, the missing limit <= 0 guard on the two new boundary clippers, the changed meaning of PromptChars, and the stale "reduce compaction context" numbers in the description — are all optional. Only the description one has any reader-facing consequence, so it's worth a quick re-measure before merge.
Nice work on the IsError field earlier in this PR, incidentally — that was the structural fix rather than the cheap one, and this commit is better for having it to build on.
fe11979
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/compaction.go`:
- Around line 220-224: Update CompactMessages around SummarizeCompactionMessages
to propagate summaryResult.Truncated and summaryResult.ProjectedChars into the
returned CompactionResult instead of retaining only SummaryText. Add a
regression test exercising CompactMessages with a truncated projection and
asserting both metadata fields, including the non-truncated behavior as
appropriate.
🪄 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: c390003f-d322-4a96-980f-2c50065cdd85
📒 Files selected for processing (6)
internal/agent/compaction.gointernal/agent/compaction_projection.gointernal/agent/compaction_projection_test.gointernal/sessions/replay.gointernal/tui/session_controls.gointernal/tui/session_controls_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tui/session_controls_test.go
- internal/tui/session_controls.go
- internal/agent/compaction_projection.go
- internal/sessions/replay.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving at fe119799. My previous approval was dismissed by this push, which crossed with it by about three minutes.
You took all four notes, and the truncation fix is better than what I proposed. I suggested SummarizeCompactionMessages return the fact; you threaded it properly — compactionProjection{messages, truncated} out of the projection, CompactionSummaryResult{SummaryText, ProjectedChars, Truncated} out of the summarizer — so internal/tui no longer recognises agent's output by sight. grep for either marker in internal/tui now returns nothing.
It also catches a case the string sniffing would only have caught by luck: len(fullSummary) > compactionPreviousSummaryBytes reports truncation of the previous summary independently of the brief. The old sniff would have matched [middle omitted] from clipHeadTailBytes incidentally, and silently stopped the day that marker was reworded.
Re-ran the mutation that proved the gap last time. Flipping capCompactionBrief's new return to false:
--- FAIL: TestManualCompactionPersistsStructuralTruncationMetadata
session_controls_test.go:589: compaction did not persist structural truncation
Same edit left the entire tui suite green at 8b45d995. The cross-package join is now pinned, which was the whole ask.
The other three are done too:
limit <= 0guards on bothclipPrefixAtBoundaryandclipSuffixAtBoundary, matchingclipBytes.PromptCharsis both documented and more honest than before —len(CompactionSummaryInstructions) + ProjectedChars, andcompactionMessageCharscounts tool call names and arguments rather thanContentalone. The test pinning the exact value keeps it that way.- The
CompactionMessagescomment now names the invariant it actually depends on: that the session writer scrubs tool output before persistence, with ask_user answers and generic previews redacted locally. That is the correct statement, and it tells the next person adding a shaped branch what they need to check.
Verified at fe119799: go test ./internal/agent/ ./internal/sessions/ ./internal/tui/ -count=1 all green; mutation as above; working tree clean afterward.
Left open, both optional and neither worth another round: the session event log is still read twice per /compact (PlanCompaction at replay.go:151, then again at session_controls.go:859), and the PR description's "reduce compaction context" figures predate the budget changes in 8b45d995. The second is worth a quick edit to the description before merge so the claim matches the constants, but it is not a code change.
Good sequence of fixes on this PR — IsError, then the retry wording, and now this. Each one took the structural option over the cheap one.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Everything I raised is addressed, and I re-checked each one against the code rather than the commit message.
Both blocking ones are fixed and I proved them: an ask_user answer and an apply_patch file path both survive the projection now (ask_user ANSWER survives: true, apply_patch FILE survives: true). The mustPreserve gate on ask_user and ChangedFiles is the right shape, since those are exactly the two categories that are not reconstructible.
The /compact claim is now true rather than dropped, which is the better of the two options I offered. SummarizeCompactionMessages applies the shared projection and both paths go through it, so the description matches the code. Only the event-to-message conversion is separate, and it carries IsError and ChangedFiles through, with ask_user_answers handled explicitly and redacted.
capCompactionBrief keeping both edges under a byte budget is a better answer than the line cap I complained about, and the comment says why: the oldest material has not necessarily been summarized before. The argument allowlist growing to url, prompt, description and question closes the web_fetch and task gaps too. ResolvedFailureKeys is now cleared on re-failure, so the fail-fix-fail shape reports honestly.
The split-fallback point I raised is effectively moot now rather than fixed: the projection still returns a single message, but the byte cap bounds the input up front, so the oversized-middle case it protected against cannot arise the same way. Worth knowing rather than acting on.
One thing that is not yours: TestEagerToolSchemaTokenBudget fails on this branch at 3588 tokens against a 3550 ceiling. It is pre-existing. I bisected it to cd0eb19 (#843, which added view_image to the eager set); 4ca4fa75 and edf660ab both pass, and current main fails at 3578. So this branch inherits it and adds about 10 tokens. I am raising it separately and it is not a reason to hold this PR.
e450033
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving at e4500339. Third push in forty minutes; each one dismissed the prior approval, so this is a re-approve of the same PR at a newer head rather than a new objection.
e4500339 is small and symmetric: CompactionResult now carries ProjectedChars and Truncated, so the automatic path reports the same two facts SummarizeCompactionMessages already hands the manual path. Before this, the projection computed them and the automatic caller dropped them on the floor. The test covers both directions — exact compactionMessageChars agreement with a non-zero value on the compacting path, and zero values on the no-op path so the fields cannot quietly report stale numbers when nothing was compacted.
One observation, not a request. Nothing in production reads CompactionResult.ProjectedChars or CompactionResult.Truncated yet — grep finds only the two assignments in compaction.go:248-249 and the assertions in compaction_metadata_test.go. The session_controls.go:874 reader is CompactionSummaryResult on the manual path, a different struct. So automatic compaction can still truncate without anything recording it or telling the user, which is the same gap /compact just closed. Surfacing the fields is the right first half and the struct is exported, so this is fine as-is — I mention it only so the second half doesn't get forgotten, since the metadata now exists and costs nothing to record.
Verified at e4500339: go test ./internal/agent/ ./internal/sessions/ ./internal/tui/ ./internal/zeroruntime/ -count=1 all green.
Everything from my fe119799 review still stands — the structural truncation reporting, the clipper guards, the PromptChars accounting, and the corrected CompactionMessages doc comment. The two open items remain optional and unchanged: the doubled ReadEvents per /compact, and the PR description's pre-8b45d995 figures for "reduce compaction context", which are worth correcting before merge so the stated claim matches the constants.
Note that the PR still shows blocked on @Vasanthdev2004's changes-requested from yesterday 18:12, which predates all three of these commits.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/compaction_metadata_test.go (1)
137-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the captured projection is actually bounded.
The test checks
Truncatedand metadata propagation, but it does not prove that projection removed content. A regression could pass by sending the full oversized middle to the summarizer and settingTruncatedtotrue.Compare the captured character count with the original middle input or the production projection limit.
Proposed assertion
if !result.Compacted || !result.Truncated { t.Fatalf("compaction metadata = compacted %t, truncated %t; want both true", result.Compacted, result.Truncated) } + middleChars := compactionMessageChars(messages[1 : len(messages)-2]) if result.ProjectedChars != compactionMessageChars(captured) || result.ProjectedChars == 0 { t.Fatalf("ProjectedChars = %d, want %d", result.ProjectedChars, compactionMessageChars(captured)) } + if result.ProjectedChars >= middleChars { + t.Fatalf("projection was not bounded: got %d characters from %d", result.ProjectedChars, middleChars) + }🤖 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/agent/compaction_metadata_test.go` around lines 137 - 142, Extend the assertions in the compaction metadata test around result.ProjectedChars and captured to verify the captured projection is smaller than the original oversized middle input, or otherwise does not exceed the production projection limit. Preserve the existing checks for Compacted, Truncated, nonzero ProjectedChars, and metadata propagation.
🤖 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.
Nitpick comments:
In `@internal/agent/compaction_metadata_test.go`:
- Around line 137-142: Extend the assertions in the compaction metadata test
around result.ProjectedChars and captured to verify the captured projection is
smaller than the original oversized middle input, or otherwise does not exceed
the production projection limit. Preserve the existing checks for Compacted,
Truncated, nonzero ProjectedChars, and metadata propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 634e8056-d9e4-4f62-b567-ff6ecc87c87c
📒 Files selected for processing (2)
internal/agent/compaction.gointernal/agent/compaction_metadata_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/compaction.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-approving. My earlier approval was dismissed when you pushed, so I read the two new commits rather than just re-stamping it.
fe119799 and e4500339 are an improvement on what I approved. Returning compactionProjection{messages, truncated} and having capCompactionBrief report (string, bool) replaces the TUI sniffing its own summariser output for the literal "omitted to fit compaction budget". I checked and that string match is gone from session_controls.go. Truncation of the carried-forward previous summary is now tracked too, which it was not before, and clipHeadTailBytes picked up a limit <= 0 guard.
Every compaction test passes on this head.
Two failures in the suite, both pre-existing and neither yours. TestEagerToolSchemaTokenBudget is the eager-schema ceiling: this branch reads 3588 because it predates #867's trim to 3578, and #877 raises the ceiling to fix main. TestAltScreenTranscriptScrollKeepsFooterFixed fails on pristine main too; I confirmed that separately against a clean worktree earlier today.
Worth rebasing on main once #877 lands so your CI goes green.
3 participants
Summary
/compactWhy
Long coding sessions can accumulate large file reads and command output. The previous compaction path sent that entire reconstructible payload to the summarizer, increasing token cost and latency even though the final replacement was already small.
Controlled benchmark
The refreshed benchmark uses a deterministic 616,309-byte corpus assembled from eight real Zero source files and the same recorded transcript for both measurements. It measures the raw summarizer input against the current semantic projection over five sequential runs. The temporary harness was removed after the run and no report is committed.
This reduces estimated summarizer input by 99.83% while adding about 0.047 ms of local projection work.
The replay assertions are structural checks on representative sessions: they verify that user intent, decisions, exact failures, active plans, ask-user exchanges, changed files, and the recent tail survive. They do not claim to measure model-summary quality across every provider or workload.
Validation
make fmt-checkgo vet ./...internal/agent,internal/sessions, andinternal/tuigo run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake vulncheckgit diff HEAD --check/compact, continued conversation,/resume, and exact constraint recallgo test ./...still reports three unrelated failures that reproduce outside the changed paths: two CLI doctor tests return exit code 3, and the suggestion-overlay test matches the wordcompactionin the checkout path or branch name.make lint-staticreports two stale.pr867-validationpaths that are not present in this branch.Summary by CodeRabbit