fix(acp): warn on session persistence failures#487
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses ACP session persistence failures that were previously ignored by surfacing them as non-fatal warning “thought” chunks, while preserving in-memory continuity so turns still work even when on-disk history cannot be read or appended.
Changes:
- Add a
notifier.warninghelper that emits warnings viaagent_thought_chunk. - Return and propagate persistence/load errors from
persistTurnandloadHistory, emitting warnings instead of silently ignoring failures. - Add end-to-end ACP tests covering warning emission and continued prompt/load behavior under persistence corruption/failure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/acp/translate.go | Adds a notifier warning helper that routes warnings through thought chunks. |
| internal/acp/agent.go | Stops silently swallowing store errors; surfaces load/append failures as non-fatal warnings while keeping in-memory history updated. |
| internal/acp/agent_test.go | Adds tests asserting persistence/load failures produce warning thought chunks and do not abort prompt/load flows. |
💡 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughSession history persistence and loading now return errors instead of discarding them, and those failures are surfaced as warning thought updates. The ACP test harness now separates thought chunks, and new end-to-end tests cover persistence and history-read failures. ChangesPersistence and history-load error surfacing
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 (1)
internal/acp/agent_test.go (1)
163-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid end-to-end coverage for the two warning paths.
Both tests correctly force real failures (directory-as-file, invalid JSON) and assert the RPC still succeeds while a warning thought chunk is emitted — this directly validates the persistTurn/loadHistory → notifier.warning contract from
internal/acp/agent.go.One optional gap:
TestACPPromptWarnsWhenPersistenceFailschecksStopReasonbut doesn't verify the assistant's actual message text still reachesh.updates, which would more fully confirm the "in-memory continuity" guarantee the PR objectives call out (persistence failing should not affect the delivered response). Consider adding that assertion for stronger coverage.🤖 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/acp/agent_test.go` around lines 163 - 224, TestACPPromptWarnsWhenPersistenceFails covers the warning path but does not assert that the assistant response still reaches the in-memory update stream when persistence fails. Update this test to also verify the delivered assistant text via h.updates, using the existing harness and MethodSessionPrompt flow, so it confirms continuity alongside the StopReason and warning thought chunk checks.
🤖 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/acp/agent_test.go`:
- Around line 163-224: TestACPPromptWarnsWhenPersistenceFails covers the warning
path but does not assert that the assistant response still reaches the in-memory
update stream when persistence fails. Update this test to also verify the
delivered assistant text via h.updates, using the existing harness and
MethodSessionPrompt flow, so it confirms continuity alongside the StopReason and
warning thought chunk checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53633b53-10e8-4661-8c0a-2c72f15c8fb5
📒 Files selected for processing (3)
internal/acp/agent.gointernal/acp/agent_test.gointernal/acp/translate.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Clean and well-scoped. ACP persistence errors that were getting swallowed now surface as non-fatal warning chunks, and the in-memory history is kept unconditionally so the turn keeps working when the store fails. Traces correct, the tests cover both the load and the append failure paths, and there's nothing platform-specific. Approving.
| } | ||
|
|
||
| // Serve runs the connection read loop until the stream closes or ctx is done. | ||
| func (a *Agent) Serve(ctx context.Context) error { return a.conn.Serve(ctx) } |
There was a problem hiding this comment.
can you check this? its flagged by CI security as unreachable function
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
please check agent.go changes there is un unreachable function
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Note
GitHub currently reports mergeStateStatus: BLOCKED even though the branch is mergeable from a conflict standpoint. The apparent blocker is the open requested-changes review about internal/acp/agent.go:87 being flagged as unreachable. I checked that line: Agent.Serve is unchanged from main, is a thin wrapper around Conn.Serve, and is used by the ACP test harness. The repo's CI security workflow documents the deadcode step as advisory/non-blocking, and the current Security & code health job passed. I do not see a code or security fix needed for that specific flag, but the outstanding review state still needs to be resolved before the PR can proceed.
Findings
- [P3] Strengthen the persistence-failure test to verify in-memory continuity
internal/acp/agent_test.go:163
TestACPPromptWarnsWhenPersistenceFailscorrectly checks that the prompt succeeds and that a warning thought chunk is emitted, but it does not assert that the assistant's message text still reachesh.updateswhen persistence fails. Since the PR's stated goal includes keeping prompt turns and in-memory continuity working when persistence fails, please add an assertion such asdrainContains(t, h.updates, "Hello from ZERO")so the test proves the warning path does not interrupt streamed assistant output.
Ashwinhegde19
force-pushed
the
fix-acp-persistence-warnings
branch
from
4e19a38 to
4a04b73
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving.
The persistence-failure test now asserts in-memory continuity the way you asked — drainContains(h.updates, "Hello from ZERO") proves the assistant's output still streams when the append fails, alongside the two warning-thought assertions for the load and append paths. That was the one open finding.
The deadcode flag on agent.go:87 is a non-issue — Serve is unchanged from main and that step is advisory, as you already found; nothing to fix there.
Marked the linked issue #471 issue-approved — silent persistence errors dropping session history is a real bug, and surfacing them as non-fatal warnings while keeping the turn and in-memory continuity alive is the right fix. CI is green.
Good to merge — over to kevin.
Ashwinhegde19
force-pushed
the
fix-acp-persistence-warnings
branch
from
4a04b73 to
3253cd1
Compare
Fixes #471
Summary
Tests
go test ./internal/acp -count=1make lintmake test-quickSummary by CodeRabbit