Fix nil update dereference in tool approval middleware#472
Fix nil update dereference in tool approval middleware#472PratikDhanave wants to merge 1 commit into
Conversation
The tool approval middleware passed every update from the inner run directly to splitApprovalRequestContents, which dereferences update.Contents. Nil updates are legal in a response stream: the agent run loop skips them and the toolautocall middleware explicitly forwards them, so composing toolapproval with toolautocall (or any provider that yields nil updates) panicked with a nil pointer dereference. Forward nil updates downstream, matching toolautocall's behavior, and add a regression test that drives the middleware with a nil update.
|
@PratikDhanave please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the tool approval middleware (agent/harness/toolapproval) when the inner run yields a nil update by forwarding nil updates downstream instead of attempting to classify them, aligning behavior with other middleware and the core run loop.
Changes:
- Add a
nil-update guard in the tool approval middleware’s streaming loop and forwardnilupdates downstream. - Add a regression test to ensure
nilupdates are tolerated and passed through. - Apply gofmt/gofumpt-style formatting to existing test callsites.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
agent/harness/toolapproval/toolapproval.go |
Forwards nil updates downstream to avoid dereferencing u.Contents on a nil update. |
agent/harness/toolapproval/toolapproval_test.go |
Adds a regression test for nil update passthrough and reformats helper callsites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tc, ok := u.Contents[0].(*message.TextContent); ok && tc.Text == "done" { | ||
| textUpdates++ | ||
| } |
| yield(&agent.ResponseUpdate{ | ||
| Role: message.RoleAssistant, | ||
| Contents: []message.Content{&message.TextContent{Text: "done"}}, | ||
| }, nil) | ||
| } |
Summary
The tool approval middleware (
agent/harness/toolapproval) dereferences every update from the inner run without a nil check: the main loop passes each update straight intosplitApprovalRequestContents, which iteratesu.Contents.Nil updates are legal in a response stream — the agent run loop skips them (
agent.go), and thetoolautocallmiddleware explicitly forwards them downstream (autocall.go).toolapprovalis the only consumer in the codebase missing this guard, so composing it withtoolautocall(or any provider that yields a nil update) panics:Fix
Forward nil updates downstream (matching
toolautocall's behavior) instead of classifying them, preserving stream semantics for outer consumers.No public API changes.
Tests
TestToolApproval_NilUpdatePassthrough, which drives the middleware with an inner run that yields a nil update followed by a text update. Onmainit fails with the SIGSEGV above; with the fix it passes and asserts the nil update is forwarded and the text update is delivered.go test -race -shuffle=on ./...passes.gofmt/gofumptclean,go vetclean.