fix: preserve conversation context in exec prompts#460
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts how zero exec / TUI resume builds the “Previous session context” prompt so conversation turns (messages and other high-signal session events) aren’t pushed out by a long tail of noisy events (e.g., tool results). It adds a regression test to ensure early conversation messages remain present even when many non-conversation events follow.
Changes:
- Replace the fixed “last 20 events” truncation with
promptContextEvents, which filters to high-signal event types and caps prompt context at 80 events. - Preserve early
EventMessageturns even when many later tool-result events exist (by filtering out those noisy events from prompt context). - Add a regression test covering long/noisy exec histories.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/sessions/exec_session.go |
Updates exec/session prompt context selection to filter for high-signal events and use a larger capped window. |
internal/sessions/store_test.go |
Adds a regression test ensuring conversation messages remain in the formatted prompt when many noisy events follow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (1)
Walkthrough
ChangesPrompt context event filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
internal/sessions/exec_session.go (1)
159-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable empty-check given fallback guarantee.
promptContextEventsalways falls back to the raweventsslice when filtering yields nothing (Lines 200-202), andFormatExecPromptalready guardslen(prepared.ContextEvents) == 0at Line 160. Soeventsreturned frompromptContextEventscan never be empty here, making theif len(events) == 0check at Line 164 dead code.🤖 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/sessions/exec_session.go` around lines 159 - 166, The empty-check in FormatExecPrompt is dead code because promptContextEvents already guarantees a non-empty result when prepared.ContextEvents is non-empty and ModeNew is excluded. Remove the redundant len(events) == 0 guard in FormatExecPrompt and keep the existing early return on ModeNew and empty ContextEvents; use promptContextEvents as the single source of truth for event filtering.internal/sessions/store_test.go (1)
558-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the 80-event cap boundary.
This test only produces 3 conversation-type events (well under the new
maxPromptContextEvents = 80cap), so it validates the noise-filtering fix but not the actual truncation logic inpromptContextEvents(Lines 203-205 of exec_session.go). Given the PR's stated motivation ("many later context events"), a test with >80 conversation-type events verifying the earliest ones are dropped (or preserved per PR intent) while the most recent 80 remain would close this gap.🤖 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/sessions/store_test.go` around lines 558 - 582, The new test in TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow only checks noisy-event filtering and does not cover the maxPromptContextEvents truncation behavior in promptContextEvents. Add a companion case in store_test.go that builds more than 80 conversation-type events, then verify FormatExecPrompt keeps the intended most recent 80 context messages and drops the earliest ones according to the resume prompt behavior, while still excluding non-conversation noise. Use the existing symbols FormatExecPrompt, PreparedExec, and promptContextEvents to keep the test aligned with the truncation logic.
🤖 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/sessions/exec_session.go`:
- Around line 159-166: The empty-check in FormatExecPrompt is dead code because
promptContextEvents already guarantees a non-empty result when
prepared.ContextEvents is non-empty and ModeNew is excluded. Remove the
redundant len(events) == 0 guard in FormatExecPrompt and keep the existing early
return on ModeNew and empty ContextEvents; use promptContextEvents as the single
source of truth for event filtering.
In `@internal/sessions/store_test.go`:
- Around line 558-582: The new test in
TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow only checks
noisy-event filtering and does not cover the maxPromptContextEvents truncation
behavior in promptContextEvents. Add a companion case in store_test.go that
builds more than 80 conversation-type events, then verify FormatExecPrompt keeps
the intended most recent 80 context messages and drops the earliest ones
according to the resume prompt behavior, while still excluding non-conversation
noise. Use the existing symbols FormatExecPrompt, PreparedExec, and
promptContextEvents to keep the test aligned with the truncation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e15c7051-61d1-4508-bdca-1d806b2c892c
📒 Files selected for processing (2)
internal/sessions/exec_session.gointernal/sessions/store_test.go
|
hi @pengdst please address coderabbit comments |
|
@kevincodex1 done 👍 |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve — genuine bug fix with regression coverage
Worth it: yes. FormatExecPrompt previously kept only the last 20 events (events[len(events)-20:]) regardless of type, so when a resumed or forked zero exec session had 20+ trailing noisy events (tool_result / tool_call / permission / usage), the window filled with noise and every actual conversation turn was dropped from the "Previous session context" block. The fix filters to high-signal event types before capping, restoring the lost context. Small (+105/−4), self-contained to internal/sessions. Thanks @pengdst.
Verified on Windows (worktree): go build ./internal/sessions/... clean; go test ./internal/sessions/ all pass (incl. the two new regression tests); gofmt clean. Confirmed all whitelisted Event constants exist in store.go, and that ModeNew still early-returns the prompt unchanged — so single-shot zero exec is untouched; only resume/fork paths change.
Design notes (non-blocking):
- The context cap grew 20 → 80 (≈40KB worst case at the 500-char/line cap), but the type-filter strips the bulk of the noise, so real prompts are typically smaller than before. Reasonable.
- The all-noise fallback sensibly keeps the old behavior for sessions with no high-signal events.
- The whitelist excludes all tool_call/tool_result, so a session that ended right after a tool_result the agent hadn't responded to won't surface that output in the resume prompt — a reasonable tradeoff (messages carry the intent).
CodeRabbit's two nitpicks don't apply to the final diff (the "dead code" guard isn't present; the "80-cap coverage" gap was closed by the added truncation test — which is why CodeRabbit later approved). CI is green including Smoke (windows-latest). Approving.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
VERDICT: approve
REGRESSION RISK: low
The change replaces the old "last 20 events regardless of type" truncation in FormatExecPrompt with a new promptContextEvents function that filters to conversation-relevant event types (EventMessage, EventCompaction, EventSessionFork, EventSessionChild, EventSpecialistStart, EventSpecialistStop, EventError) and caps at 80. This alters the content of resumed/forked exec session prompts for existing users — they will now see more conversation context and less tool-result noise. This is an improvement to the prompt quality, not a breaking change: the prompt format (## Previous session context + event lines + ## Current user request) is unchanged, and the function returns early for ModeNew or empty context as before. The fallback (if len(filtered) == 0 { filtered = append(filtered, events...) }) preserves all events when none match the filter, so sessions with only non-conversation events are not silently empty.
BUILD / TEST
go build ./...— passgo vet ./internal/sessions/...— passgofmt -lon changed files — cleango test ./internal/sessions/... -run TestFormatExecPrompt— both new tests pass:TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow: verifies conversation messages survive when followed by 43 tool-result eventsTestFormatExecPromptTruncatesConversationMessagesAfterFilteringNoise: verifies the 80-event cap keeps the most recent conversation events and drops noise
- CI all green (macOS, Ubuntu, Windows, Smoke, Security, Zero Review)
CONTRIBUTING
No linked issue. The author is a community contributor (pengdst). Per CONTRIBUTING.md, community PRs require an approved parent issue with the issue-approved label. No such issue is linked. Process violation, but the change is a legitimate bug fix with regression tests. Maintainer's call.
FINDINGS
- Nit —
internal/sessions/exec_session.go:184-201— fallback path could exceed the cap. When no events match the filter, the fallback appends all events, then the cap is applied. This is correct behavior but means a session with only tool results will still include up to 80 of them. Not a blocker — this preserves existing behavior for unusual sessions.
No other findings.
EXISTING REVIEWS
- Vasanthdev2004 (APPROVED): Assessed as a genuine bug fix with regression coverage. Valid on the current head. Nothing missed.
- coderabbitai (APPROVED + 2 nitpicks): Minor maintainability suggestions on the filter list. Valid, trivial. Nothing critical missed.
- copilot-pull-request-reviewer (COMMENTED): Accurate overview. Nothing missed.
All reviews on the current head (54c3259). No stale reviews.
BOTTOM LINE
A solid bug fix that preserves conversation context in resumed exec sessions instead of dropping it behind tool-result noise; safe to merge.
Summary
Fixes exec/session prompt formatting so older conversation turns are preserved instead of being dropped when many later context events exist.
What changed
Tests
Summary by CodeRabbit
Bug Fixes
Tests