From 07b6e4bdeceb83484d7607b87ad40886e847d5e9 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:26:14 -0500 Subject: [PATCH 1/6] test: fix data race in TestHooksManagerAsyncDrain handlerCompleted was written by a detached goroutine inside the hook handler and read by the test goroutine with no synchronization, so `go test -race` failed on this test. Guard it with a mutex. Production code is unaffected -- the race is entirely in the test's own shared state. But CI never ran the race detector, so nothing would have caught the same mistake in production code either. This lands first so the blocking race gate added in the next commit is green from day one rather than shipping a knowingly-red required check. The assertions still mean what they did: emit returns before the async work settles, and Drain waits for it. Verified by neutering Drain, which still fails the test. Co-Authored-By: Claude --- hooks_manager_test.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/hooks_manager_test.go b/hooks_manager_test.go index d1957d2..c8c0f1b 100644 --- a/hooks_manager_test.go +++ b/hooks_manager_test.go @@ -2,6 +2,7 @@ package agent import ( "errors" + "sync" "testing" "time" ) @@ -207,12 +208,25 @@ func TestHooksManagerSessionIDThreading(t *testing.T) { func TestHooksManagerAsyncDrain(t *testing.T) { m := NewHooksManager() work := make(chan error) - var handlerCompleted bool + // handlerCompleted is written by the detached goroutine below and read by + // this test goroutine, so it needs a mutex — without one this test races + // and `go test -race` fails. The assertions are about ordering (emit + // returns before the work settles; Drain waits for it), which the lock + // preserves. + var mu sync.Mutex + handlerCompleted := false + completed := func() bool { + mu.Lock() + defer mu.Unlock() + return handlerCompleted + } m.OnPostToolUse(HookEntry[PostToolUsePayload, EmptyHookResult]{ Handler: func(payload PostToolUsePayload, hctx LifecycleHookContext) (HookHandlerResult[EmptyHookResult], error) { go func() { time.Sleep(10 * time.Millisecond) + mu.Lock() handlerCompleted = true + mu.Unlock() close(work) }() return AsyncResult[EmptyHookResult](AsyncOutput{Work: work}), nil @@ -223,11 +237,11 @@ func TestHooksManagerAsyncDrain(t *testing.T) { t.Fatal(err) } // Async handler must not block emit from returning. - if handlerCompleted { + if completed() { t.Fatalf("async handler should not have completed synchronously") } m.Drain() - if !handlerCompleted { + if !completed() { t.Fatalf("Drain should wait for detached async work to settle") } } From 226e4e0136c064ab3a1b0e9a8b6ca2cd6b1ce9f8 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:26:45 -0500 Subject: [PATCH 2/6] test: cover streaming, final-response retry, and untested required API The suite had no deterministic test that drove a *successful* stream: one streaming fake existed and it emitted a broken frame to assert error propagation, while the other 26 fake responses were non-streaming. So consumeCreateResponse's success loop sat at 42%, and because that loop never ran, ReasoningStream was structurally unreachable and TextStream was only ever exercised through its single-chunk non-streaming fallback -- for a package whose headline feature is streaming fan-out. Adds stream_fake_test.go: events built with the SDK's Create* constructors and serialized to real SSE frames, decoded back by the SDK's own decoder. Three SDK details each produce a fake that looks fine and silently carries no events (a hand-built event can leave `type` empty; marshalling via map[string]any drops the union discriminator; the SDK re-wraps the `data:` payload before the decoder sees it), all with no error returned -- hence TestSanityFakeStreamDecodesThroughSDK, which guards the fake itself. Also covers, in rough order of risk: - retryCurrentRequest (0%) and StrictFinalResponse (zero test references). Live code on the terminal turn of every tool-using conversation, and a documented divergence from upstream. - validateValue's number/integer/boolean/array branches and nested recursion (50%). This is the guard that stops malformed model output reaching user tool code. - serverToolImpl and NewServerTool (0%), asserting the wrapped SDK union reaches the request verbatim rather than just exercising getters. - The paused-run read path: ItemsStream, ToolCalls, PendingToolCalls, RequiresApproval, Cancel. Existing tests only reached a paused run via State(). - FinishReasonIs and ToChatMessage, both in the contract's required API with no test at all -- exported, so the verifier's presence check passed. Coverage 65.9% -> 72.2%; consumeCreateResponse 42% -> 88%, ReasoningStream 0% -> 100%. Each test was checked by breaking the production code it covers and confirming it fails. Co-Authored-By: Claude --- compat_test.go | 76 ++++++++ final_response_retry_test.go | 142 +++++++++++++++ result_accessors_test.go | 178 ++++++++++++++++++ server_tool_test.go | 124 +++++++++++++ stop_conditions_test.go | 87 +++++++++ stream_fake_test.go | 342 +++++++++++++++++++++++++++++++++++ tool_executor_test.go | 153 ++++++++++++++++ 7 files changed, 1102 insertions(+) create mode 100644 final_response_retry_test.go create mode 100644 result_accessors_test.go create mode 100644 server_tool_test.go create mode 100644 stream_fake_test.go create mode 100644 tool_executor_test.go diff --git a/compat_test.go b/compat_test.go index 8592dc1..4ea8581 100644 --- a/compat_test.go +++ b/compat_test.go @@ -145,6 +145,82 @@ func TestClaudeRoundTripEveryRoleAndCarrier(t *testing.T) { } } +// TestToChatMessageCarriesTextToolCallsAndMetadata covers ToChatMessage, which +// is in the contract's required public API but had no test: it was exported (so +// the verifier's presence check passed) while nothing asserted its output. +// +// Unlike ToClaudeMessage it is a direct value conversion with no error, so the +// contract is simply that text, tool calls, and metadata all survive. +func TestToChatMessageCarriesTextToolCallsAndMetadata(t *testing.T) { + call := components.OutputFunctionCallItem{CallID: "c1", Name: "search", Arguments: `{"query":"go"}`} + resp := components.OpenResponsesResult{ + ID: "resp_1", + Model: "openai/test", + Status: components.OpenAIResponsesResponseStatusCompleted, + OutputText: openrouter.String("here you go"), + Output: []components.OutputItems{components.CreateOutputItemsFunctionCall(call)}, + } + + msg := ToChatMessage(resp) + + if msg.Role != "assistant" { + t.Fatalf("Role = %q, want assistant", msg.Role) + } + if msg.Content != "here you go" { + t.Fatalf("Content = %v, want %q", msg.Content, "here you go") + } + if len(msg.ToolCalls) != 1 { + t.Fatalf("expected 1 tool call, got %d", len(msg.ToolCalls)) + } + if msg.ToolCalls[0].CallID != "c1" || msg.ToolCalls[0].Name != "search" { + t.Fatalf("tool call not carried: %+v", msg.ToolCalls[0]) + } + // Arguments is `any` at the JSON boundary; decoded object args land as a map. + args, ok := msg.ToolCalls[0].Arguments.(map[string]any) + if !ok { + t.Fatalf("tool call arguments = %T, want map[string]any", msg.ToolCalls[0].Arguments) + } + if args["query"] != "go" { + t.Fatalf("tool call arguments not decoded: %+v", args) + } + if msg.Metadata["id"] != "resp_1" { + t.Fatalf("metadata id = %v, want resp_1", msg.Metadata["id"]) + } + if msg.Metadata["model"] != "openai/test" { + t.Fatalf("metadata model = %v, want openai/test", msg.Metadata["model"]) + } + if msg.Metadata["status"] != string(components.OpenAIResponsesResponseStatusCompleted) { + t.Fatalf("metadata status = %v", msg.Metadata["status"]) + } +} + +// TestToChatMessageFeedsBackThroughFromChatMessages asserts the pair composes: +// a converted assistant message must survive FromChatMessages, which is how a +// caller replays chat history into a follow-up request. +func TestToChatMessageFeedsBackThroughFromChatMessages(t *testing.T) { + call := components.OutputFunctionCallItem{CallID: "c1", Name: "search", Arguments: `{"query":"go"}`} + resp := components.OpenResponsesResult{ + ID: "resp_1", + OutputText: openrouter.String("calling"), + Output: []components.OutputItems{components.CreateOutputItemsFunctionCall(call)}, + } + + items, err := FromChatMessages([]ChatMessage{ToChatMessage(resp)}) + if err != nil { + t.Fatalf("FromChatMessages: %v", err) + } + + var sawToolCall bool + for _, it := range items { + if it.FunctionCallItem != nil && it.FunctionCallItem.CallID == "c1" { + sawToolCall = true + } + } + if !sawToolCall { + t.Fatalf("tool call lost in ToChatMessage -> FromChatMessages round trip: %#v", items) + } +} + // TestChatRoundTripEveryRole asserts FromChatMessages handles every chat role // and that tool messages become function_call_output items. Run-2 16c (Chat). func TestChatRoundTripEveryRole(t *testing.T) { diff --git a/final_response_retry_test.go b/final_response_retry_test.go new file mode 100644 index 0000000..e8dafd4 --- /dev/null +++ b/final_response_retry_test.go @@ -0,0 +1,142 @@ +package agent + +// Coverage for the empty-final-response retry (model_result.go retryCurrentRequest, +// reached from run()). +// +// This path had zero test coverage despite running on the terminal turn of every +// tool-using conversation, and its opt-out flag StrictFinalResponse had no test +// references at all. It is also a documented divergence from upstream: upstream +// treats an empty `output` array as an invalid response and errors, while here +// the SDK's separate OutputText convenience field makes an empty Output +// unreliable as an "invalid response" signal, so the port retries with +// tool_choice=none instead. Both halves of that behavior are asserted here. + +import ( + "context" + "testing" + + openrouter "github.com/OpenRouterTeam/go-sdk" + "github.com/OpenRouterTeam/go-sdk/models/components" + "github.com/OpenRouterTeam/go-sdk/models/operations" +) + +// emptyFinalResponse has neither Output items nor OutputText, which is what +// isEmptyFinalResponse treats as "the model returned nothing". +func emptyFinalResponse(id string) components.OpenResponsesResult { + return components.OpenResponsesResult{ID: id} +} + +// TestEmptyFinalResponseTriggersRetryWithToolChoiceNone asserts that when a tool +// round is followed by an empty final response, the loop retries once and pins +// tool_choice=none so the model answers instead of calling another tool. +func TestEmptyFinalResponseTriggersRetryWithToolChoiceNone(t *testing.T) { + ctx := context.Background() + tool := MustNewTool(ToolConfig[sampleInput]{Name: "search", Execute: func(context.Context, sampleInput, ToolExecuteContext) (any, error) { + return "tool output", nil + }}) + + call := components.OutputFunctionCallItem{CallID: "call_1", Name: "search", Arguments: `{"query":"go"}`} + toolTurn := components.OpenResponsesResult{ID: "resp_1", Output: []components.OutputItems{ + components.CreateOutputItemsFunctionCall(call), + }} + empty := emptyFinalResponse("resp_2") + recovered := components.OpenResponsesResult{ID: "resp_3", OutputText: openrouter.String("recovered answer")} + + createdTool := operations.CreateCreateResponsesResponseOpenResponsesResult(toolTurn) + createdEmpty := operations.CreateCreateResponsesResponseOpenResponsesResult(empty) + createdRecovered := operations.CreateCreateResponsesResponseOpenResponsesResult(recovered) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{ + &createdTool, &createdEmpty, &createdRecovered, + }} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi", Tools: []Tool{tool}}) + if err != nil { + t.Fatal(err) + } + text, err := result.Text(ctx) + if err != nil { + t.Fatal(err) + } + + if text != "recovered answer" { + t.Fatalf("retry result did not become the final text: %q", text) + } + if sender.calls != 3 { + t.Fatalf("expected 3 requests (tool round, empty final, retry), got %d", sender.calls) + } + + // The retry is the third request, and must pin tool_choice=none while tools + // are still present -- that is what stops it looping back into a tool call. + retryReq := sender.requests[2] + if len(retryReq.Tools) == 0 { + t.Fatal("retry request unexpectedly dropped the tools") + } + if retryReq.ToolChoice == nil { + t.Fatal("retry request must set tool_choice=none, got nil") + } + if retryReq.ToolChoice.OpenAIResponsesToolChoiceNone == nil { + t.Fatalf("retry tool_choice should be none, got %+v", retryReq.ToolChoice) + } +} + +// TestStrictFinalResponseSkipsRetry asserts the opt-out: with +// StrictFinalResponse the empty response is accepted as final and no extra +// request is sent. +func TestStrictFinalResponseSkipsRetry(t *testing.T) { + ctx := context.Background() + tool := MustNewTool(ToolConfig[sampleInput]{Name: "search", Execute: func(context.Context, sampleInput, ToolExecuteContext) (any, error) { + return "tool output", nil + }}) + + call := components.OutputFunctionCallItem{CallID: "call_1", Name: "search", Arguments: `{"query":"go"}`} + toolTurn := components.OpenResponsesResult{ID: "resp_1", Output: []components.OutputItems{ + components.CreateOutputItemsFunctionCall(call), + }} + empty := emptyFinalResponse("resp_2") + + createdTool := operations.CreateCreateResponsesResponseOpenResponsesResult(toolTurn) + createdEmpty := operations.CreateCreateResponsesResponseOpenResponsesResult(empty) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&createdTool, &createdEmpty}} + + result, err := CallModel(ctx, sender, CallModelInput{ + Model: "openai/test", + Input: "hi", + Tools: []Tool{tool}, + StrictFinalResponse: true, + }) + if err != nil { + t.Fatal(err) + } + text, err := result.Text(ctx) + if err != nil { + t.Fatal(err) + } + + if text != "" { + t.Fatalf("StrictFinalResponse should surface the empty response as-is, got %q", text) + } + if sender.calls != 2 { + t.Fatalf("StrictFinalResponse must not retry: expected 2 requests, got %d", sender.calls) + } +} + +// TestEmptyFirstResponseWithoutToolRoundIsNotRetried pins the other guard on the +// retry condition: it requires a completed tool round. An empty response on a +// plain no-tools call is returned as-is rather than costing a second request. +func TestEmptyFirstResponseWithoutToolRoundIsNotRetried(t *testing.T) { + ctx := context.Background() + empty := emptyFinalResponse("resp_1") + created := operations.CreateCreateResponsesResponseOpenResponsesResult(empty) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + if _, err := result.Text(ctx); err != nil { + t.Fatal(err) + } + if sender.calls != 1 { + t.Fatalf("no tool round means no retry: expected 1 request, got %d", sender.calls) + } +} diff --git a/result_accessors_test.go b/result_accessors_test.go new file mode 100644 index 0000000..da441da --- /dev/null +++ b/result_accessors_test.go @@ -0,0 +1,178 @@ +package agent + +// Coverage for the ModelResult read accessors, several of which are in the +// contract's required public API and had no deterministic test: ItemsStream, +// ToolCalls, ToolCallsStream, NewMessagesStream, PendingToolCalls, +// RequiresApproval, ContextUpdates and Cancel were all 0%. +// +// PendingToolCalls and RequiresApproval matter most: existing tests only ever +// reached a paused run through State(), so the public read path a caller +// actually uses to discover "what is waiting on me" was never exercised. + +import ( + "context" + "testing" + + "github.com/OpenRouterTeam/go-sdk/models/components" + "github.com/OpenRouterTeam/go-sdk/models/operations" +) + +// pausedResult builds a run that stops at awaiting_approval with one pending call. +func pausedResult(t *testing.T) (*ModelResult, *fakeSender) { + t.Helper() + tool := MustNewTool(ToolConfig[sampleInput]{Name: "danger", RequireApproval: true, Execute: func(context.Context, sampleInput, ToolExecuteContext) (any, error) { + t.Fatal("approval-required tool must not execute before approval") + return nil, nil + }}) + call := components.OutputFunctionCallItem{CallID: "call_1", Name: "danger", Arguments: `{"query":"go"}`} + resp := components.OpenResponsesResult{ID: "resp_1", Output: []components.OutputItems{ + components.CreateOutputItemsFunctionCall(call), + }} + created := operations.CreateCreateResponsesResponseOpenResponsesResult(resp) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(context.Background(), sender, CallModelInput{ + Model: "openai/test", Input: "hi", Tools: []Tool{tool}, + }) + if err != nil { + t.Fatal(err) + } + return result, sender +} + +func TestPausedRunExposesPendingCallsThroughPublicAccessors(t *testing.T) { + ctx := context.Background() + result, _ := pausedResult(t) + + needsApproval, err := result.RequiresApproval(ctx) + if err != nil { + t.Fatal(err) + } + if !needsApproval { + t.Fatal("RequiresApproval should report true while awaiting approval") + } + + pending, err := result.PendingToolCalls(ctx) + if err != nil { + t.Fatal(err) + } + if len(pending) != 1 { + t.Fatalf("expected 1 pending call, got %d", len(pending)) + } + if pending[0].CallID != "call_1" || pending[0].Name != "danger" { + t.Fatalf("pending call not surfaced correctly: %+v", pending[0]) + } + + // The returned slice must be a copy: mutating it cannot corrupt the run's + // own state, or a caller inspecting pending calls could break the resume. + pending[0].CallID = "mutated" + again, err := result.PendingToolCalls(ctx) + if err != nil { + t.Fatal(err) + } + if again[0].CallID != "call_1" { + t.Fatalf("PendingToolCalls handed out an aliased slice: %q", again[0].CallID) + } +} + +func TestResultItemsStreamYieldsOutputItems(t *testing.T) { + ctx := context.Background() + result, _ := pausedResult(t) + + ch, wait := result.ItemsStream(ctx) + var items []components.OutputItems + for item := range ch { + items = append(items, item) + } + if err := wait(); err != nil { + t.Fatal(err) + } + if len(items) != 1 { + t.Fatalf("expected 1 output item, got %d", len(items)) + } + if items[0].OutputFunctionCallItem == nil || items[0].OutputFunctionCallItem.CallID != "call_1" { + t.Fatalf("output item not surfaced: %+v", items[0]) + } +} + +func TestResultToolCallAccessorsAgree(t *testing.T) { + ctx := context.Background() + result, _ := pausedResult(t) + + calls, err := result.ToolCalls(ctx) + if err != nil { + t.Fatal(err) + } + if len(calls) != 1 || calls[0].Name != "danger" { + t.Fatalf("ToolCalls = %+v, want one call to danger", calls) + } + + // The streaming variant must report the same calls as the snapshot. + ch, wait := result.ToolCallsStream(ctx) + var streamed []ParsedToolCall + for c := range ch { + streamed = append(streamed, c) + } + if err := wait(); err != nil { + t.Fatal(err) + } + if len(streamed) != len(calls) { + t.Fatalf("ToolCallsStream saw %d calls, ToolCalls saw %d", len(streamed), len(calls)) + } + if len(streamed) > 0 && streamed[0].CallID != calls[0].CallID { + t.Fatalf("ToolCallsStream/ToolCalls disagree: %q vs %q", streamed[0].CallID, calls[0].CallID) + } +} + +func TestResultNewMessagesStreamAndContextUpdates(t *testing.T) { + ctx := context.Background() + result, _ := pausedResult(t) + + ch, wait := result.NewMessagesStream(ctx) + var msgs []components.InputsUnion1 + for m := range ch { + msgs = append(msgs, m) + } + if err := wait(); err != nil { + t.Fatal(err) + } + // A paused turn records the model's function_call as a new message so the + // resume can replay it. + if len(msgs) == 0 { + t.Fatal("NewMessagesStream produced nothing for a turn with a tool call") + } + + updates, waitCtx := result.ContextUpdates(ctx) + for range updates { + } + if err := waitCtx(); err != nil { + t.Fatal(err) + } +} + +// TestResultCancelStopsRunWithoutPanic covers Cancel, the public cancellation +// entry point, which had no test at all. Calling it after completion must be +// safe, and calling it twice must not panic. +func TestResultCancelStopsRunWithoutPanic(t *testing.T) { + ctx := context.Background() + resp := completedResponse("done") + created := operations.CreateCreateResponsesResponseOpenResponsesResult(resp) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + if _, err := result.Text(ctx); err != nil { + t.Fatal(err) + } + + result.Cancel() + result.Cancel() + + // Reads after cancellation still return the completed result rather than + // blocking or panicking. + if _, err := result.Text(ctx); err != nil { + t.Fatalf("Text after Cancel should still return the completed text: %v", err) + } +} diff --git a/server_tool_test.go b/server_tool_test.go new file mode 100644 index 0000000..3272481 --- /dev/null +++ b/server_tool_test.go @@ -0,0 +1,124 @@ +package agent + +// Coverage for NewServerTool, a contract-Required public API entry point whose +// whole implementation (serverToolImpl and its nine methods) previously sat at +// 0%. Server tools are executed by OpenRouter, not locally, so the only +// behavior that matters is that the wrapped SDK tool union reaches the request +// verbatim and that the agent never tries to run it locally. + +import ( + "context" + "testing" + + "github.com/OpenRouterTeam/go-sdk/models/components" + "github.com/OpenRouterTeam/go-sdk/models/operations" +) + +func webSearchServerTool() components.ResponsesRequestToolUnion { + return components.CreateResponsesRequestToolUnionWebSearch20250826( + components.WebSearchServerTool{Type: components.WebSearchServerToolTypeWebSearch20250826}, + ) +} + +func TestNewServerToolExposesServerToolShape(t *testing.T) { + cfg := ServerToolConfig{Name: "web_search", Config: webSearchServerTool()} + tool := NewServerTool(cfg) + + if tool.ToolName() != "web_search" { + t.Fatalf("ToolName = %q, want web_search", tool.ToolName()) + } + if tool.ToolType() != ToolTypeServer { + t.Fatalf("ToolType = %v, want %v", tool.ToolType(), ToolTypeServer) + } + // Server tools carry no local schemas: OpenRouter owns validation and + // execution, so advertising a local schema here would be a lie. + if tool.InputSchema() != nil || tool.OutputSchema() != nil || tool.EventSchema() != nil { + t.Fatal("server tools must not advertise local input/output/event schemas") + } + if tool.ToolDescription() != "" { + t.Fatalf("ToolDescription = %q, want empty", tool.ToolDescription()) + } + + approval, err := tool.RequiresApproval(context.Background(), ParsedToolCall{Name: "web_search"}, TurnContext{}) + if err != nil { + t.Fatal(err) + } + if approval { + t.Fatal("server tools cannot pause for local approval") + } + + // HandleResponseReceived is a passthrough: there is no local output to rewrite. + out, err := tool.HandleResponseReceived(context.Background(), "untouched", ToolExecuteContext{}) + if err != nil { + t.Fatal(err) + } + if out != "untouched" { + t.Fatalf("HandleResponseReceived mutated output: %v", out) + } +} + +// TestNewServerToolPassesConfigThroughUnmodified is the assertion that actually +// matters: ToAPITool must hand back exactly the union it was given. A dropped or +// rewritten field here silently changes the request OpenRouter receives. +func TestNewServerToolPassesConfigThroughUnmodified(t *testing.T) { + maxResults := int64(3) + engine := components.WebSearchEngineEnumExa + original := components.CreateResponsesRequestToolUnionWebSearch20250826( + components.WebSearchServerTool{ + Type: components.WebSearchServerToolTypeWebSearch20250826, + Engine: &engine, + MaxResults: &maxResults, + }, + ) + + got := NewServerTool(ServerToolConfig{Name: "web_search", Config: original}).ToAPITool() + + if got.WebSearchServerTool == nil { + t.Fatalf("ToAPITool lost the web_search union member: %+v", got) + } + if got.WebSearchServerTool.Engine == nil || *got.WebSearchServerTool.Engine != engine { + t.Fatalf("engine not passed through: %+v", got.WebSearchServerTool.Engine) + } + if got.WebSearchServerTool.MaxResults == nil || *got.WebSearchServerTool.MaxResults != maxResults { + t.Fatalf("max_results not passed through: %+v", got.WebSearchServerTool.MaxResults) + } +} + +// TestServerToolReachesRequestThroughCallModel closes the loop end-to-end: a +// server tool passed to CallModel must appear in the outgoing request's Tools, +// and must not be executed locally. +func TestServerToolReachesRequestThroughCallModel(t *testing.T) { + ctx := context.Background() + server := NewServerTool(ServerToolConfig{Name: "web_search", Config: webSearchServerTool()}) + + resp := completedResponse("done") + created := operations.CreateCreateResponsesResponseOpenResponsesResult(resp) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(ctx, sender, CallModelInput{ + Model: "openai/test", + Input: "search the web", + Tools: []Tool{server}, + }) + if err != nil { + t.Fatal(err) + } + if _, err := result.Text(ctx); err != nil { + t.Fatal(err) + } + + if len(sender.requests) == 0 { + t.Fatal("no request was sent") + } + tools := sender.requests[0].Tools + if len(tools) != 1 { + t.Fatalf("expected exactly 1 tool on the request, got %d", len(tools)) + } + if tools[0].WebSearchServerTool == nil { + t.Fatalf("server tool did not reach the request as a web_search union: %+v", tools[0]) + } + // A server tool must not be sent as a local function definition. + if tools[0].ResponsesRequestToolFunction != nil { + t.Fatal("server tool was serialized as a local function tool") + } +} diff --git a/stop_conditions_test.go b/stop_conditions_test.go index bc7c61a..59fdaa3 100644 --- a/stop_conditions_test.go +++ b/stop_conditions_test.go @@ -4,7 +4,9 @@ import ( "context" "testing" + openrouter "github.com/OpenRouterTeam/go-sdk" "github.com/OpenRouterTeam/go-sdk/models/components" + "github.com/OpenRouterTeam/go-sdk/models/operations" "github.com/OpenRouterTeam/go-sdk/optionalnullable" ) @@ -41,3 +43,88 @@ func TestStopConditionsUseUpstreamSemantics(t *testing.T) { t.Fatalf("expected max cost to trigger at threshold") } } + +// TestFinishReasonIsMatchesStepFinishReason covers FinishReasonIs, which is in +// the contract's required public API but previously had no test at all: it was +// exported (so the verifier's presence check passed) and never exercised. +// +// The values it must match are whatever run() puts in StepResult.FinishReason, +// which comes from string(resp.Status) -- so this asserts against a real SDK +// status string rather than an invented token. +func TestFinishReasonIsMatchesStepFinishReason(t *testing.T) { + ctx := context.Background() + completed := string(components.OpenAIResponsesResponseStatusCompleted) + steps := []StepResult{{FinishReason: completed}} + + ok, err := FinishReasonIs(completed)(ctx, steps) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatalf("FinishReasonIs(%q) should match a step with that finish reason", completed) + } + + ok, err = FinishReasonIs("incomplete")(ctx, steps) + if err != nil { + t.Fatal(err) + } + if ok { + t.Fatal("FinishReasonIs must not match a different finish reason") + } + + // Matches if ANY step carries the reason, not only the last one. + multi := []StepResult{{FinishReason: "in_progress"}, {FinishReason: completed}} + ok, err = FinishReasonIs(completed)(ctx, multi) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("FinishReasonIs should match when any step has the finish reason") + } + + // No steps means nothing to match. + ok, err = FinishReasonIs(completed)(ctx, nil) + if err != nil { + t.Fatal(err) + } + if ok { + t.Fatal("FinishReasonIs must be false for an empty step list") + } +} + +// TestFinishReasonIsObservesRealRunStatus ties FinishReasonIs to what the loop +// actually records, so the constant it matches can't drift from run()'s value +// without a test failing. +func TestFinishReasonIsObservesRealRunStatus(t *testing.T) { + ctx := context.Background() + resp := components.OpenResponsesResult{ + ID: "resp_1", + Status: components.OpenAIResponsesResponseStatusCompleted, + OutputText: openrouter.String("done"), + } + created := operations.CreateCreateResponsesResponseOpenResponsesResult(resp) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + if _, err := result.Text(ctx); err != nil { + t.Fatal(err) + } + + // m.steps is unexported with no public accessor; these tests are in-package, + // and Text() above already waited for the run to finish, so reading it here + // is safe and needs no production change. + steps := result.steps + if len(steps) == 0 { + t.Fatal("expected at least one step") + } + ok, err := FinishReasonIs(string(components.OpenAIResponsesResponseStatusCompleted))(ctx, steps) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatalf("FinishReasonIs should match the status the loop recorded, got %q", steps[len(steps)-1].FinishReason) + } +} diff --git a/stream_fake_test.go b/stream_fake_test.go new file mode 100644 index 0000000..63d0a39 --- /dev/null +++ b/stream_fake_test.go @@ -0,0 +1,342 @@ +package agent + +// Deterministic streaming test support. +// +// Before this file the suite had exactly one streaming fake and it only ever +// emitted a broken frame to assert error propagation, so no test drove a +// *successful* stream. That left consumeCreateResponse's success loop, run()'s +// delta callback and ReasoningStream unreachable, and meant TextStream was only +// ever exercised through its single-chunk non-streaming fallback -- for a +// package whose headline feature is streaming fan-out. +// +// Events here are built with the SDK's own Create* constructors and then +// serialized to real SSE frames, which the SDK's decoder parses back. That +// matters: the constructors and UnmarshalJSON both set the union's `Type` +// discriminator *and* its member pointer together. A fake that hand-set struct +// fields could leave `Type` empty, which would still satisfy production's +// pointer nil-checks while disagreeing with any Type-based predicate -- a fake +// that silently diverges from production hides the bugs it is meant to catch. + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "strings" + "testing" + + openrouter "github.com/OpenRouterTeam/go-sdk" + "github.com/OpenRouterTeam/go-sdk/models/components" + "github.com/OpenRouterTeam/go-sdk/models/operations" + "github.com/OpenRouterTeam/go-sdk/types/stream" +) + +// sseFrames encodes events as an SSE body, mirroring what the Responses API +// puts on the wire, and terminates with the [DONE] sentinel the SDK expects. +func sseFrames(t *testing.T, events []components.StreamEvents, withDone bool) io.Reader { + t.Helper() + var buf bytes.Buffer + for _, ev := range events { + // The frame body is the bare event: the SDK's SSE scanner re-wraps the + // `data:` payload as {"data": } before handing it to the decoder, + // so emitting that wrapper here would double-wrap it. + // + // Marshal the typed value, never a map[string]any: StreamEvents has a + // custom MarshalJSON that flattens the active union member, and routing + // through a map loses the discriminator. + payload, err := json.Marshal(ev) + if err != nil { + t.Fatalf("marshal stream event: %v", err) + } + fmt.Fprintf(&buf, "data: %s\n\n", payload) + } + if withDone { + buf.WriteString("data: [DONE]\n\n") + } + return &buf +} + +// eventStreamFrom builds an SDK EventStream over real SSE bytes, decoded by the +// SDK's own unmarshaller rather than a stub decoder. +func eventStreamFrom(t *testing.T, events []components.StreamEvents, withDone bool) *stream.EventStream[components.ResponsesStreamingResponse] { + t.Helper() + return stream.NewEventStream( + context.Background(), + sseFrames(t, events, withDone), + func(b []byte) (components.ResponsesStreamingResponse, error) { + // The SDK hands the decoder the frame already wrapped as + // {"data": }, which is exactly the ResponsesStreamingResponse + // envelope shape -- so unmarshal it as the envelope, not as a bare + // StreamEvents. Decoding it bare yields Type "UNKNOWN" with every + // union pointer nil, which is silent: json.Unmarshal returns no + // error, so the fake would look fine while carrying no event at all. + var v components.ResponsesStreamingResponse + if err := json.Unmarshal(b, &v); err != nil { + return components.ResponsesStreamingResponse{}, err + } + return v, nil + }, + "[DONE]", + ) +} + +// streamingResponse wraps events as a streaming CreateResponsesResponse, ready +// to hand to fakeSender. +func streamingResponse(t *testing.T, events []components.StreamEvents) *operations.CreateResponsesResponse { + t.Helper() + created := operations.CreateCreateResponsesResponseEventStream(eventStreamFrom(t, events, true)) + return &created +} + +func textDelta(delta string, seq int64) components.StreamEvents { + return components.CreateStreamEventsResponseOutputTextDelta(components.TextDeltaEvent{ + Delta: delta, ItemID: "item_1", SequenceNumber: seq, + }) +} + +func reasoningDelta(delta string, seq int64) components.StreamEvents { + return components.CreateStreamEventsResponseReasoningTextDelta(components.ReasoningDeltaEvent{ + Delta: delta, ItemID: "item_1", SequenceNumber: seq, + }) +} + +func reasoningSummaryDelta(delta string, seq int64) components.StreamEvents { + return components.CreateStreamEventsResponseReasoningSummaryTextDelta(components.ReasoningSummaryTextDeltaEvent{ + Delta: delta, ItemID: "item_1", SequenceNumber: seq, + }) +} + +func completedEvent(resp components.OpenResponsesResult, seq int64) components.StreamEvents { + return components.CreateStreamEventsResponseCompleted(components.StreamEventsResponseCompleted{ + Response: resp, SequenceNumber: seq, + }) +} + +// completedResponse is the terminal response a streaming turn resolves to. Uses +// the OutputText convenience field, matching the rest of the suite. +// +// Object and ToolChoice are set explicitly because this response is genuinely +// serialized to SSE bytes and read back through the SDK's strict decoder: an +// empty ToolChoice union fails to marshal ("all fields are null") and an empty +// Object fails to unmarshal ("invalid value for OpenResponsesResultObject"). +// Tests elsewhere hand a response straight to fakeSender without a wire +// round-trip, so they can leave both zero. +func completedResponse(text string) components.OpenResponsesResult { + return components.OpenResponsesResult{ + ID: "resp_stream", + Model: "openai/test", + Object: components.OpenResponsesResultObjectResponse, + Status: components.OpenAIResponsesResponseStatusCompleted, + OutputText: openrouter.String(text), + ToolChoice: components.CreateOpenAIResponsesToolChoiceUnionOpenAIResponsesToolChoiceAuto( + components.OpenAIResponsesToolChoiceAutoAuto, + ), + } +} + +// TestSanityFakeStreamDecodesThroughSDK guards the fake itself: every event must +// round-trip through the SDK decoder with BOTH its union member pointer and its +// `Type` discriminator populated. If this fails, every other streaming test is +// asserting against a fake that does not look like real traffic. +func TestSanityFakeStreamDecodesThroughSDK(t *testing.T) { + events := []components.StreamEvents{ + textDelta("a", 1), + reasoningDelta("r", 2), + reasoningSummaryDelta("s", 3), + completedEvent(completedResponse("a"), 4), + } + es := eventStreamFrom(t, events, true) + defer es.Close() + + var decoded []components.StreamEvents + for es.Next() { + v := es.Value() + if v == nil { + continue + } + decoded = append(decoded, v.GetData()) + } + if err := es.Err(); err != nil { + t.Fatalf("fake stream failed to decode: %v", err) + } + if len(decoded) != len(events) { + t.Fatalf("decoded %d events, want %d", len(decoded), len(events)) + } + + if decoded[0].TextDeltaEvent == nil || decoded[0].Type != components.StreamEventsTypeResponseOutputTextDelta { + t.Fatalf("text delta lost pointer or Type: %+v", decoded[0]) + } + if decoded[0].TextDeltaEvent.Delta != "a" { + t.Fatalf("text delta value = %q", decoded[0].TextDeltaEvent.Delta) + } + if decoded[1].ReasoningDeltaEvent == nil || decoded[1].Type != components.StreamEventsTypeResponseReasoningTextDelta { + t.Fatalf("reasoning delta lost pointer or Type: %+v", decoded[1]) + } + if decoded[2].ReasoningSummaryTextDeltaEvent == nil || decoded[2].Type != components.StreamEventsTypeResponseReasoningSummaryTextDelta { + t.Fatalf("reasoning summary delta lost pointer or Type: %+v", decoded[2]) + } + if decoded[3].StreamEventsResponseCompleted == nil || decoded[3].Type != components.StreamEventsTypeResponseCompleted { + t.Fatalf("completed event lost pointer or Type: %+v", decoded[3]) + } +} + +// TestStreamingTextDeltasArriveInOrder is the test the suite was missing: a +// genuine multi-delta stream, asserting TextStream yields each delta separately +// and in order, and that concatenating them equals Text(). Previously the only +// deterministic coverage was the non-streaming single-chunk fallback, so delta +// ordering was verified nowhere outside the paid e2e job. +func TestStreamingTextDeltasArriveInOrder(t *testing.T) { + ctx := context.Background() + final := completedResponse("Hello, world!") + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{ + streamingResponse(t, []components.StreamEvents{ + textDelta("Hello", 1), + textDelta(", ", 2), + textDelta("world", 3), + textDelta("!", 4), + completedEvent(final, 5), + }), + }} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + + ch, wait := result.TextStream(ctx) + var got []string + for chunk := range ch { + got = append(got, chunk) + } + if err := wait(); err != nil { + t.Fatal(err) + } + + want := []string{"Hello", ", ", "world", "!"} + if len(got) != len(want) { + t.Fatalf("got %d deltas %q, want %d %q", len(got), got, len(want), want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("delta %d = %q, want %q (full: %q)", i, got[i], want[i], got) + } + } + + text, err := result.Text(ctx) + if err != nil { + t.Fatal(err) + } + if joined := strings.Join(got, ""); joined != text { + t.Fatalf("concatenated deltas %q != Text() %q", joined, text) + } +} + +// TestStreamingReasoningDeltasReachReasoningStream covers ReasoningStream, a +// contract-Required consumer that had no deterministic test at all: its only +// push sites sit inside the streaming callback, so without a successful stream +// it was structurally unreachable. +// +// Both reasoning event shapes are exercised, since each has its own branch. +func TestStreamingReasoningDeltasReachReasoningStream(t *testing.T) { + ctx := context.Background() + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{ + streamingResponse(t, []components.StreamEvents{ + reasoningDelta("think", 1), + reasoningSummaryDelta("summary", 2), + textDelta("answer", 3), + completedEvent(completedResponse("answer"), 4), + }), + }} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + + ch, wait := result.ReasoningStream(ctx) + var got []string + for chunk := range ch { + got = append(got, chunk) + } + if err := wait(); err != nil { + t.Fatal(err) + } + + want := []string{"think", "summary"} + if len(got) != len(want) { + t.Fatalf("reasoning deltas = %q, want %q", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("reasoning delta %d = %q, want %q", i, got[i], want[i]) + } + } +} + +// TestStreamingSurfacesEveryEventToFullStream asserts the raw event passthrough: +// FullResponsesStream emits a "response.event" per upstream event, which is the +// escape hatch consumers use for events the typed streams don't model. +func TestStreamingSurfacesEveryEventToFullStream(t *testing.T) { + ctx := context.Background() + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{ + streamingResponse(t, []components.StreamEvents{ + textDelta("a", 1), + reasoningDelta("r", 2), + completedEvent(completedResponse("a"), 3), + }), + }} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + + ch, wait := result.FullResponsesStream(ctx) + var raw int + var sawTurnStart, sawTurnEnd bool + for ev := range ch { + switch ev.Type { + case "response.event": + raw++ + if ev.Event == nil { + t.Fatal("response.event carried a nil Event") + } + case "turn.start": + sawTurnStart = true + case "turn.end": + sawTurnEnd = true + } + } + if err := wait(); err != nil { + t.Fatal(err) + } + + if raw != 3 { + t.Fatalf("got %d raw response.event entries, want 3", raw) + } + if !sawTurnStart || !sawTurnEnd { + t.Fatalf("turn boundaries missing: start=%v end=%v", sawTurnStart, sawTurnEnd) + } +} + +// TestStreamEndingWithoutCompletedEventErrors covers the distinct error branch +// for a stream that terminates without a response.completed event. It must +// surface an error rather than returning an empty response as if it succeeded. +func TestStreamEndingWithoutCompletedEventErrors(t *testing.T) { + ctx := context.Background() + // Deltas, then [DONE] with no response.completed. + created := operations.CreateCreateResponsesResponseEventStream( + eventStreamFrom(t, []components.StreamEvents{textDelta("partial", 1)}, true), + ) + sender := &fakeSender{responses: []*operations.CreateResponsesResponse{&created}} + + result, err := CallModel(ctx, sender, CallModelInput{Model: "openai/test", Input: "hi"}) + if err != nil { + t.Fatal(err) + } + + if _, err := result.Text(ctx); err == nil { + t.Fatal("a stream that never sent response.completed must surface an error") + } +} diff --git a/tool_executor_test.go b/tool_executor_test.go new file mode 100644 index 0000000..b90bb2c --- /dev/null +++ b/tool_executor_test.go @@ -0,0 +1,153 @@ +package agent + +// Coverage for ValidateAgainstSchema / validateValue, the guard that stops +// malformed model-produced tool arguments from reaching user tool code. +// +// It sat at 50%: only the object and string branches were exercised, leaving +// number, integer, boolean, array, nested properties, and array `items` +// recursion entirely unverified. A bug in any of those means bad input reaches +// a user's Execute function. +// +// Table-driven, since these are many small independent cases. + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestValidateAgainstSchemaScalarAndArrayTypes(t *testing.T) { + cases := []struct { + name string + schema map[string]any + input string + wantErr string // substring; empty means the input must validate + }{ + // number: JSON numbers decode to float64, so an integral literal is a + // valid number too. + {"number accepts float", map[string]any{"type": "number"}, `1.5`, ""}, + {"number accepts integral", map[string]any{"type": "number"}, `2`, ""}, + {"number rejects string", map[string]any{"type": "number"}, `"1.5"`, "expected number"}, + {"number rejects bool", map[string]any{"type": "number"}, `true`, "expected number"}, + {"number rejects null", map[string]any{"type": "number"}, `null`, "expected number"}, + + // integer is number plus a whole-value check. + {"integer accepts whole", map[string]any{"type": "integer"}, `7`, ""}, + {"integer accepts negative whole", map[string]any{"type": "integer"}, `-7`, ""}, + {"integer rejects fractional", map[string]any{"type": "integer"}, `7.5`, "expected integer"}, + {"integer rejects string", map[string]any{"type": "integer"}, `"7"`, "expected integer"}, + + {"boolean accepts true", map[string]any{"type": "boolean"}, `true`, ""}, + {"boolean accepts false", map[string]any{"type": "boolean"}, `false`, ""}, + {"boolean rejects string", map[string]any{"type": "boolean"}, `"true"`, "expected boolean"}, + {"boolean rejects number", map[string]any{"type": "boolean"}, `1`, "expected boolean"}, + + {"string accepts", map[string]any{"type": "string"}, `"hi"`, ""}, + {"string rejects number", map[string]any{"type": "string"}, `5`, "expected string"}, + + // array, with and without an items schema. + {"array accepts untyped items", map[string]any{"type": "array"}, `[1,"a",true]`, ""}, + {"array accepts empty", map[string]any{"type": "array"}, `[]`, ""}, + {"array rejects object", map[string]any{"type": "array"}, `{"a":1}`, "expected array"}, + { + "array validates items schema", + map[string]any{"type": "array", "items": map[string]any{"type": "string"}}, + `["a","b"]`, "", + }, + { + "array reports bad item with index in path", + map[string]any{"type": "array", "items": map[string]any{"type": "string"}}, + `["a",2]`, "$[1]: expected string", + }, + + // unknown/absent type is not enforced -- the schema support is + // deliberately partial, with exotic cases left at the JSON boundary. + {"missing type accepts anything", map[string]any{}, `{"whatever":1}`, ""}, + {"unknown type accepts anything", map[string]any{"type": "any"}, `"x"`, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateAgainstSchema(json.RawMessage(tc.input), tc.schema) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("expected %s to validate, got %v", tc.input, err) + } + return + } + if err == nil { + t.Fatalf("expected error containing %q, got nil", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %q, want it to contain %q", err, tc.wantErr) + } + }) + } +} + +// TestValidateAgainstSchemaNestedRecursion covers the recursive descent through +// object properties and array items, including the error path a caller sees. +func TestValidateAgainstSchemaNestedRecursion(t *testing.T) { + schema := map[string]any{ + "type": "object", + "required": []any{"user"}, + "properties": map[string]any{ + "user": map[string]any{ + "type": "object", + "required": []any{"name"}, + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "age": map[string]any{"type": "integer"}, + "tags": map[string]any{"type": "array", "items": map[string]any{"type": "string"}}, + }, + }, + }, + } + + cases := []struct { + name string + input string + wantErr string + }{ + {"fully valid", `{"user":{"name":"ada","age":36,"tags":["x","y"]}}`, ""}, + {"optional fields may be absent", `{"user":{"name":"ada"}}`, ""}, + {"missing top-level required", `{}`, "$.user: required"}, + {"missing nested required", `{"user":{"age":36}}`, "$.user.name: required"}, + {"wrong nested scalar type", `{"user":{"name":5}}`, "$.user.name: expected string"}, + {"wrong nested integer", `{"user":{"name":"ada","age":1.5}}`, "$.user.age: expected integer"}, + {"bad item inside nested array", `{"user":{"name":"ada","tags":["ok",7]}}`, "$.user.tags[1]: expected string"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateAgainstSchema(json.RawMessage(tc.input), schema) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("expected %s to validate, got %v", tc.input, err) + } + return + } + if err == nil { + t.Fatalf("expected error containing %q, got nil", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %q, want it to contain %q", err, tc.wantErr) + } + }) + } +} + +// TestValidateAgainstSchemaEdgeCases pins the two short-circuit paths: an empty +// schema validates anything, and malformed JSON is a decode error rather than a +// silent pass. +func TestValidateAgainstSchemaEdgeCases(t *testing.T) { + if err := ValidateAgainstSchema(json.RawMessage(`{"anything":true}`), nil); err != nil { + t.Fatalf("nil schema should validate anything, got %v", err) + } + if err := ValidateAgainstSchema(json.RawMessage(`{"a":1}`), map[string]any{}); err != nil { + t.Fatalf("empty schema should validate anything, got %v", err) + } + if err := ValidateAgainstSchema(json.RawMessage(`{not json`), map[string]any{"type": "object"}); err == nil { + t.Fatal("malformed JSON must return a decode error, not validate silently") + } +} From afbd3423b3b59f1095c3a325e1cdbc7c4dc2b6a9 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:27:25 -0500 Subject: [PATCH 3/6] refactor: use ToolEventBroadcaster and the stream guards in the loop stream_guards.go and tool_event_broadcaster.go had zero callers in production or test code -- they were parallel reimplementations of logic model_result.go did inline. ToolEventBroadcaster wraps exactly the ReusableStream[ToolStreamEvent] that ModelResult built directly, and the guards answered "is this a text or reasoning delta" by checking the union's `type` while production nil-checked the member pointer: two mechanisms for one question, with only the unused copy exported. Rather than test dead code (which raises coverage while adding maintenance surface) or delete required-API symbols, wire production to use them. The duplication -- the thing that drifts and misleads -- goes away, and they are covered for free. Behavior-preserving. The SDK sets `type` and the member pointer together in both its Create* constructors and UnmarshalJSON, so the type-based guards agree with the previous pointer checks for any event the SDK produced. Error(nil) forwards to Complete(nil), matching the sibling streams' completion on both paths. The full test suite produces byte-identical results before and after. Lands after the streaming tests in the previous commit so there is a real regression net under a change to the load-bearing loop. Not wired in: tool_orchestrator.go's ExecuteToolLoop, which looks like the tool loop but passes a zero TurnContext and nil emitter, so it silently drops hooks, approval gating, and generator event emission. Using it would be a regression and testing it would legitimize a footgun; the real loop is executeToolCallsForTurn. It and next_turn_params.go are still uncalled -- both are deletion candidates needing a contract amendment. Co-Authored-By: Claude --- model_result.go | 29 +++++++++++++++-------------- stream_guards.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/model_result.go b/model_result.go index c955511..a369cee 100644 --- a/model_result.go +++ b/model_result.go @@ -55,7 +55,7 @@ type ModelResult struct { err error textStream *ReusableStream[string] reasoningStream *ReusableStream[string] - toolStream *ReusableStream[ToolStreamEvent] + toolStream *ToolEventBroadcaster toolCallStream *ReusableStream[ParsedToolCall] fullStream *ReusableStream[ResponseStreamEvent] newMessages *ReusableStream[components.InputsUnion1] @@ -69,7 +69,7 @@ type ModelResult struct { func newModelResult(ctx context.Context, client ResponseSender, input CallModelInput, req components.ResponsesRequest, state ConversationState) *ModelResult { ctx2, cancel := context.WithCancel(ctx) - return &ModelResult{ctx: ctx2, cancel: cancel, client: client, input: input, req: req, state: state, store: NewToolContextStore(input.Context), done: make(chan struct{}), textStream: NewReusableStream[string](), reasoningStream: NewReusableStream[string](), toolStream: NewReusableStream[ToolStreamEvent](), toolCallStream: NewReusableStream[ParsedToolCall](), fullStream: NewReusableStream[ResponseStreamEvent](), newMessages: NewReusableStream[components.InputsUnion1](), hooksManager: ResolveHooks(input.Hooks)} + return &ModelResult{ctx: ctx2, cancel: cancel, client: client, input: input, req: req, state: state, store: NewToolContextStore(input.Context), done: make(chan struct{}), textStream: NewReusableStream[string](), reasoningStream: NewReusableStream[string](), toolStream: NewToolEventBroadcaster(), toolCallStream: NewReusableStream[ParsedToolCall](), fullStream: NewReusableStream[ResponseStreamEvent](), newMessages: NewReusableStream[components.InputsUnion1](), hooksManager: ResolveHooks(input.Hooks)} } func NewModelResultFromResponse(resp components.OpenResponsesResult) *ModelResult { @@ -83,7 +83,7 @@ func NewModelResultFromResponse(resp components.OpenResponsesResult) *ModelResul } mr.textStream.Complete(nil) mr.reasoningStream.Complete(nil) - mr.toolStream.Complete(nil) + mr.toolStream.Complete() mr.toolCallStream.Complete(nil) mr.fullStream.Push(ResponseStreamEvent{Type: "turn.start", Turn: 0}) mr.fullStream.Push(ResponseStreamEvent{Type: "response.completed", Response: &resp}) @@ -190,7 +190,9 @@ func (m *ModelResult) run() { defer func() { m.textStream.Complete(m.err) m.reasoningStream.Complete(m.err) - m.toolStream.Complete(m.err) + // Error(nil) is the broadcaster's no-error completion, so this is + // equivalent to the sibling Complete(m.err) calls for both outcomes. + m.toolStream.Error(m.err) m.toolCallStream.Complete(m.err) m.fullStream.Complete(m.err) m.newMessages.Complete(m.err) @@ -274,14 +276,11 @@ func (m *ModelResult) run() { return } resp, events, err := consumeCreateResponse(res, func(ev components.StreamEvents) { - if ev.TextDeltaEvent != nil { - m.textStream.Push(ev.TextDeltaEvent.Delta) + if delta, ok := textDeltaText(ev); ok { + m.textStream.Push(delta) } - if ev.ReasoningDeltaEvent != nil { - m.reasoningStream.Push(ev.ReasoningDeltaEvent.Delta) - } - if ev.ReasoningSummaryTextDeltaEvent != nil { - m.reasoningStream.Push(ev.ReasoningSummaryTextDeltaEvent.Delta) + if delta, ok := reasoningDeltaText(ev); ok { + m.reasoningStream.Push(delta) } m.fullStream.Push(ResponseStreamEvent{Type: "response.event", Turn: turn, Event: &ev}) }) @@ -899,8 +898,8 @@ func (m *ModelResult) retryCurrentRequest(req components.ResponsesRequest, turn return components.OpenResponsesResult{}, err } retryResp, events, err := consumeCreateResponse(res, func(ev components.StreamEvents) { - if ev.TextDeltaEvent != nil { - m.textStream.Push(ev.TextDeltaEvent.Delta) + if delta, ok := textDeltaText(ev); ok { + m.textStream.Push(delta) } m.fullStream.Push(ResponseStreamEvent{Type: "response.event", Turn: turn, Event: &ev}) }) @@ -1312,7 +1311,9 @@ func (m *ModelResult) ReasoningStream(ctx context.Context) (<-chan string, func( } func (m *ModelResult) ToolStream(ctx context.Context) (<-chan ToolStreamEvent, func() error) { m.start() - return m.toolStream.Subscribe(32) + // The broadcaster already applies the same buffer size its sibling streams + // pass explicitly. + return m.toolStream.Subscribe() } func (m *ModelResult) ToolCallsStream(ctx context.Context) (<-chan ParsedToolCall, func() error) { m.start() diff --git a/stream_guards.go b/stream_guards.go index c24d05d..f9c9a77 100644 --- a/stream_guards.go +++ b/stream_guards.go @@ -2,6 +2,16 @@ package agent import "github.com/OpenRouterTeam/go-sdk/models/components" +// Stream event predicates. These classify events by the union's `type` +// discriminator, which the SDK sets alongside the corresponding member pointer +// in both its Create* constructors and UnmarshalJSON -- so a type check and a +// pointer check agree for any event the SDK produced. +// +// The loop in run() uses the *Text helpers below rather than nil-checking union +// fields inline, so "which events carry a text/reasoning delta" is answered in +// exactly one place. Before that these predicates were exported but unused, +// which meant the answer existed twice and only the unused copy was tested. + func IsTextDeltaEvent(e components.StreamEvents) bool { return e.Type == components.StreamEventsTypeResponseOutputTextDelta } @@ -14,3 +24,27 @@ func IsFunctionCallArgsDeltaEvent(e components.StreamEvents) bool { func IsResponseCompletedEvent(e components.StreamEvents) bool { return e.Type == components.StreamEventsTypeResponseCompleted } + +// textDeltaText returns the text delta carried by e, if it is a text delta event. +func textDeltaText(e components.StreamEvents) (string, bool) { + if !IsTextDeltaEvent(e) || e.TextDeltaEvent == nil { + return "", false + } + return e.TextDeltaEvent.Delta, true +} + +// reasoningDeltaText returns the reasoning delta carried by e, if it is either +// reasoning delta shape. Both feed the same reasoning stream upstream, but they +// are distinct union members, so the value comes from whichever is populated. +func reasoningDeltaText(e components.StreamEvents) (string, bool) { + if !IsReasoningDeltaEvent(e) { + return "", false + } + if e.ReasoningDeltaEvent != nil { + return e.ReasoningDeltaEvent.Delta, true + } + if e.ReasoningSummaryTextDeltaEvent != nil { + return e.ReasoningSummaryTextDeltaEvent.Delta, true + } + return "", false +} From 916495599b3a2e6be6dd57ab35d9b22a62141080 Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:27:59 -0500 Subject: [PATCH 4/6] fix: resolve the two staticcheck findings Both pre-existing, surfaced by adding staticcheck to CI: - SA5011 in TestAllowFinalResponseSendsNoToolsRequestOnStop: the test dereferenced `directive` to read .Content before its own nil check, so a nil directive would panic instead of failing with the intended message. Check first, then dereference. - ST1005: the wrapped Stop-hook error was capitalized ("Stop hook: ..."), against Go convention. Now lower-cased. The error-string change is user-visible, so it is noted in the changelog: code matching on that text rather than using errors.Is/errors.As needs updating. No test depended on the casing. Co-Authored-By: Claude --- model_result.go | 2 +- model_result_test.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/model_result.go b/model_result.go index a369cee..35dd14f 100644 --- a/model_result.go +++ b/model_result.go @@ -1062,7 +1062,7 @@ func (m *ModelResult) runStopHook(req *components.ResponsesRequest, forceResumeC stopResult, err := m.hooksManager.EmitStop(StopPayload{Reason: StopReasonMaxTurns}, m.hookEmitOptions("")) if err != nil { - return false, fmt.Errorf("Stop hook: %w", err) + return false, fmt.Errorf("stop hook: %w", err) } shouldForceResume := false diff --git a/model_result_test.go b/model_result_test.go index ac3ea8f..bd2e8d2 100644 --- a/model_result_test.go +++ b/model_result_test.go @@ -161,8 +161,11 @@ func TestAllowFinalResponseSendsNoToolsRequestOnStop(t *testing.T) { } // bare `true` appends the default final-answer directive (upstream #68). directive := items[3].EasyInputMessage + if directive == nil { + t.Fatalf("expected default final-answer directive to be appended, got %#v", items[3]) + } content, ok := directive.Content.GetOrZero() - if directive == nil || !ok || content.Str == nil || *content.Str != DefaultFinalResponseDirective { + if !ok || content.Str == nil || *content.Str != DefaultFinalResponseDirective { t.Fatalf("expected default final-answer directive to be appended, got %#v", items[3]) } } From 748b7b2d5d15d27b698e51c2d4a7e79122f0354a Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:28:30 -0500 Subject: [PATCH 5/6] ci: gate on the race detector, coverage ratchet, and per-symbol coverage CI ran gofmt/build/vet/test and nothing else. For a package whose whole job is concurrent stream fan-out plus a hooks manager documented as concurrency-safe, the notable omission was the race detector -- it had never run, and it fails on the current tree (fixed in the first commit of this branch). Upstream considers this enough of a risk to ship a dedicated turn-end-race-condition test. CI (.github/workflows/ci.yaml): - `go test -race`, blocking. A data race here is a real defect that a plain `go test` reports as a pass. - `-count=1` defeats Go's test cache, which was reporting `(cached)` on re-runs -- a cached ok says some earlier tree passed, not this one. - `-shuffle=on` catches order-dependent tests as the suite grows. - staticcheck, pinned to 2025.1.1. Unpinned, an upstream release turns into a surprise red CI on an unrelated PR. - The coverage gates below, so they apply to hand-written PRs and not just to port syncs. Verifier (.upstreamer/scripts/verify.sh) gains the same race run plus two coverage gates: - Gate A, ratchet: coverage may not fall below .upstreamer/coverage-floor.txt, and a gain above 1.5 points must be locked in by raising the floor. Coverage measured deterministically (three consecutive runs, identical), so the ratchet will not flake. Floor committed at 72.0 against 72.2 actual, leaving headroom so noise cannot redden an unrelated PR. - Gate B, per-symbol: every symbol in the contract's required public API must be *exercised*, not merely exported. This is the hole the existing presence check cannot see -- FinishReasonIs was listed, exported, and 0% covered. Matching takes the max across same-named functions, since Execute/Push/CallModel and friends repeat across types. Both gates read only this Go tree, deliberately: verify.sh has no upstream dependency, which is what lets ci.yaml run it in a job that does a plain actions/checkout with no upstream clone. Upstream-vs-port test comparison needs the upstream tree and so belongs in eval.md. verify.sh also now asserts the CI workflow and coverage floor still exist, so a port run cannot delete its own gates and still pass verification. main_test.go adds goroutine-leak detection over the whole run: this package spawns a goroutine per run and per subscriber, so a missed Complete() leaks invisibly -- tests pass, and a long-lived process grows. Dependency-free rather than uber-go/goleak, since this is a load-bearing SDK whose contract pins its dependency graph and a test-only dep still lands in go.mod for every consumer. It only polices leaks on an otherwise-green run, so a failing test is not reported as a leak. Every gate was verified to actually fail: raising the floor to 99 and dropping it to 50 both fail Gate A; disabling the FinishReasonIs tests makes Gate B name that symbol; a 40-goroutine probe trips the leak check; reverting the mutex fix reproduces the race. verify-port stays advisory -- its required-API check fails by design until the port catches up to upstream, and making that red on every PR just teaches people to ignore CI. Co-Authored-By: Claude --- .github/workflows/ci.yaml | 49 ++++++++++++++++++++++- .upstreamer/coverage-floor.txt | 1 + .upstreamer/scripts/verify.sh | 73 +++++++++++++++++++++++++++++++++- main_test.go | 71 +++++++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 3 deletions(-) create mode 100644 .upstreamer/coverage-floor.txt create mode 100644 main_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6b3f90c..68f4704 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -35,7 +35,54 @@ jobs: # Deterministic tests. e2e_test.go skips without OPENROUTER_API_KEY + # OPENROUTER_AGENT_E2E=1, so this stays free and hermetic. - - run: go test ./... + # + # -count=1 defeats Go's test cache: a cached "ok" says the tests passed + # for some earlier tree, not this one. + # -shuffle=on catches order-dependent tests, which get easier to write + # as the suite grows. + - name: Tests (shuffled, uncached) + run: go test -count=1 -shuffle=on ./... + + # The race detector, blocking. This package's whole job is concurrent + # stream fan-out plus a hooks manager documented as concurrency-safe, so a + # data race here is a real defect -- and a plain `go test` reports it as a + # pass. Slower than the run above, hence separate rather than merged. + - name: Race detector + run: go test -race -count=1 ./... + + # Coverage ratchet + per-required-symbol coverage. Shares one script with + # the port sync so hand-written PRs and generated ports are held to the + # same bar; see .upstreamer/scripts/verify.sh for what the two gates mean. + - name: Coverage gates + run: | + go test -count=1 -coverprofile=cover.out ./... + total=$(go tool cover -func=cover.out | awk '/^total:/ {gsub("%","",$NF); print $NF}') + floor=$(tr -d '[:space:]' < .upstreamer/coverage-floor.txt) + echo "coverage ${total}% (floor ${floor}%)" + awk -v t="$total" -v f="$floor" 'BEGIN{exit !(t+0 < f+0)}' && { + echo "::error::coverage ${total}% is below the floor ${floor}%; add tests rather than lowering the floor" + exit 1 + } + awk -v t="$total" -v f="$floor" 'BEGIN{exit !(t+0 >= f+0 + 1.5)}' && { + echo "::error::coverage rose to ${total}%; raise the floor in .upstreamer/coverage-floor.txt to lock it in" + exit 1 + } + echo "### Coverage \`${total}%\` (floor \`${floor}%\`)" >> "$GITHUB_STEP_SUMMARY" + + # staticcheck catches correctness and concurrency smells `go vet` misses. + # Pinned: an unpinned linter turns an upstream release into a surprise red CI + # on an unrelated PR. + lint: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: "1.25" + cache: true + - name: staticcheck + run: go run honnef.co/go/tools/cmd/staticcheck@2025.1.1 ./... # Live end-to-end tests against the real OpenRouter API: streaming, a real # tool round, approval pause/resume, lifecycle hooks, state serialization diff --git a/.upstreamer/coverage-floor.txt b/.upstreamer/coverage-floor.txt new file mode 100644 index 0000000..92a5611 --- /dev/null +++ b/.upstreamer/coverage-floor.txt @@ -0,0 +1 @@ +72.0 diff --git a/.upstreamer/scripts/verify.sh b/.upstreamer/scripts/verify.sh index fd096e4..818ab50 100755 --- a/.upstreamer/scripts/verify.sh +++ b/.upstreamer/scripts/verify.sh @@ -27,7 +27,14 @@ if command -v go >/dev/null 2>&1; then [ -z "$unformatted" ] && pass "gofmt clean" || fail "gofmt: $unformatted" run "go build ./..." go build ./... run "go vet ./..." go vet ./... - run "go test ./..." go test ./... + # -count=1 defeats the test cache: a cached "ok" is not evidence that the + # tests pass against the tree as it stands right now. + run "go test ./..." go test -count=1 ./... + # The race detector is not optional for this package. Its whole job is + # concurrent stream fan-out plus a hooks manager documented as + # concurrency-safe, and a data race there is a real bug that a plain + # `go test` reports as a pass. + run "go test -race ./..." go test -race -count=1 ./... else fail "go not installed (required to build and test this module)" fi @@ -59,6 +66,64 @@ if command -v go >/dev/null 2>&1; then fi echo +# Test coverage. Two gates, both deliberately self-contained: like every other +# check here they read only this Go tree, never the upstream checkout, which is +# what lets ci.yaml run this script in a job that does a plain `actions/checkout` +# with no upstream clone. Upstream-vs-port test comparison is judgment work and +# lives in .upstreamer/eval.md, which does have the upstream tree. +echo "-- Test coverage" +COVERAGE_FLOOR_FILE=".upstreamer/coverage-floor.txt" +if command -v go >/dev/null 2>&1; then + cover_profile=$(mktemp) + if go test -count=1 -coverprofile="$cover_profile" ./... >/tmp/verify-cover 2>&1; then + total=$(go tool cover -func="$cover_profile" | awk '/^total:/ {gsub("%","",$NF); print $NF}') + + # Gate A: ratchet. Coverage may not fall below the committed floor, and a + # meaningful gain must be locked in by raising the floor, so improvements + # can't silently erode later. + floor=$(tr -d '[:space:]' <"$COVERAGE_FLOOR_FILE" 2>/dev/null || echo "") + if [ -z "$floor" ]; then + fail "missing or empty $COVERAGE_FLOOR_FILE (needed for the coverage ratchet)" + elif awk -v t="$total" -v f="$floor" 'BEGIN{exit !(t+0 < f+0)}'; then + fail "coverage ${total}% is below the floor ${floor}% — add tests, do not lower the floor" + elif awk -v t="$total" -v f="$floor" 'BEGIN{exit !(t+0 >= f+0 + 1.5)}'; then + fail "coverage rose to ${total}% (floor ${floor}%) — raise the floor in $COVERAGE_FLOOR_FILE to lock it in" + else + pass "coverage ${total}% >= floor ${floor}%" + fi + + # Gate B: every required-API symbol must be *exercised*, not merely exported. + # The presence check above cannot see an exported symbol that no test ever + # calls -- which is exactly how FinishReasonIs sat at 0% while passing. + # + # Types are checked through their constructor, since `go tool cover` reports + # functions. Names are matched across every same-named function (Execute, + # Push and friends repeat across types), passing if ANY occurrence is + # covered. + uncovered=$(go tool cover -func="$cover_profile" | awk ' + { pct=$NF; gsub("%","",pct); if (pct+0 > best[$2]+0) best[$2]=pct+0 } + END { + n=split("CallModel NewOpenRouter NewTool MustNewTool NewServerTool \ +CreateInitialState AppendToMessages UpdateState PartitionToolCalls \ +StepCountIs HasToolCall MaxTokensUsed MaxCost FinishReasonIs \ +ToClaudeMessage FromClaudeMessages ToChatMessage FromChatMessages \ +ExtractUnsupportedContent HasUnsupportedContent GetUnsupportedContentSummary \ +NewToolContextStore NewToolEventBroadcaster", req, " ") + for (i=1;i<=n;i++) if (best[req[i]]+0 == 0) printf " %s", req[i] + }') + if [ -z "${uncovered// /}" ]; then + pass "every required-API symbol has test coverage" + else + fail "required-API symbols exported but never exercised by a test:$uncovered" + fi + else + fail "coverage run failed" + sed 's/^/ /' /tmp/verify-cover | tail -20 + fi + rm -f "$cover_profile" +fi +echo + # Hooks and versioned state are the 0.8.0 parity floor. The 0.8.0 port chose # the upstream spelling `HooksManager` verbatim, so this check pins that exact # name; the contract's required-API list is the source of truth. If a future @@ -89,7 +154,11 @@ leaked=$(find . -path ./tmp -prune -o -type f \( -name '*.ts' -o -name '*.js' \ [ -z "$leaked" ] && pass "no TS/JS artifacts" || fail "leaked upstream artifacts: $leaked" echo "-- Repo-owned files present" -for f in LICENSE README.md go.mod scripts/upstream; do +# CI and the coverage floor are included deliberately: a port run that deleted +# its own gates would otherwise pass verification while removing the checks that +# make the next run trustworthy. +for f in LICENSE README.md go.mod scripts/upstream \ + .github/workflows/ci.yaml .upstreamer/coverage-floor.txt; do [ -e "$f" ] && pass "$f present" || fail "$f missing (port must not delete repo-owned files)" done echo diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..e833c44 --- /dev/null +++ b/main_test.go @@ -0,0 +1,71 @@ +package agent + +// Goroutine-leak detection for the whole package test run. +// +// This package spawns a goroutine per CallModel run plus one per stream +// subscriber, so a missed Complete() or an unclosed subscriber leaks goroutines +// that outlive their run. That leak is invisible to a normal `go test`: the +// tests still pass, and a long-lived process just grows. +// +// Deliberately dependency-free rather than using uber-go/goleak: this is a +// load-bearing SDK whose contract pins its dependency graph, and a test-only +// dependency still lands in go.mod for every consumer of the package. +// +// The check is intentionally forgiving -- it allows a margin and re-checks with +// backoff -- because the runtime and the testing package keep their own +// goroutines, and finished goroutines are reaped asynchronously. It is meant to +// catch a systematic leak (every run leaks one), not to be a precise accounting. + +import ( + "fmt" + "os" + "runtime" + "testing" + "time" +) + +// leakMargin tolerates goroutines the runtime/testing machinery keeps around. +// A real per-run leak scales with the number of tests and clears this easily. +const leakMargin = 12 + +func TestMain(m *testing.M) { + before := runtime.NumGoroutine() + + code := m.Run() + + // Only police leaks on an otherwise-green run: a failed or panicking test + // abandons goroutines by design, and reporting that as a leak would bury + // the actual failure. + if code != 0 { + os.Exit(code) + } + + if leaked, after := waitForGoroutinesToSettle(before + leakMargin); leaked { + fmt.Fprintf(os.Stderr, + "\ngoroutine leak: %d goroutines before tests, %d after (margin %d).\n"+ + "A stream was likely left uncompleted or a subscriber unclosed.\n\n", + before, after, leakMargin) + buf := make([]byte, 1<<20) + fmt.Fprintf(os.Stderr, "%s\n", buf[:runtime.Stack(buf, true)]) + os.Exit(1) + } + + os.Exit(code) +} + +// waitForGoroutinesToSettle polls until the goroutine count drops to limit, +// giving the runtime time to reap finished goroutines. It reports whether the +// count was still above limit when the budget ran out. +func waitForGoroutinesToSettle(limit int) (leaked bool, count int) { + delay := time.Millisecond + for i := 0; i < 10; i++ { + count = runtime.NumGoroutine() + if count <= limit { + return false, count + } + runtime.GC() // nudge finalizers/reaping along + time.Sleep(delay) + delay *= 2 + } + return true, runtime.NumGoroutine() +} From 62b927a180becfa88b47545395407c15d9a19d8e Mon Sep 17 00:00:00 2001 From: Luke Parke <5702154+LukasParke@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:28:51 -0500 Subject: [PATCH 6/6] docs: make test quality a binding part of the port contract PORTING.md's own rule is that a code-only fix gets re-broken on the next sync, so the gates in the previous commit are only half the job -- the standard has to live in the contract, which is the durable artifact. .upstreamer/upstreamer.md gains a binding Test Quality section: port upstream's own test cases rather than inventing them, assert upstream-observable behavior (ordering, error surfaces, stream boundaries, state shape) never the port's internal shape, cover the edge case the upstream fix exists for, ship a test with every new required-API symbol, keep concurrency tests meaningful under -race, use real streaming events for streaming behavior, and never lower the coverage floor. It also records .github/ and the coverage floor as repo-owned, since a run that deleted its own gates would otherwise verify clean. .upstreamer/eval.md gains a test-parity criterion. This belongs in the eval rather than the verifier because the eval has the upstream checkout and the verifier deliberately does not. It asks the evaluator to diff upstream's *.test.ts across the delta and report behaviors that changed with no corresponding test -- and explicitly not to grade on test counts, since Go table-driven subtests bundle cases and the ~619-it-blocks-to-70-Test-funcs ratio is meaningless. An untested changed behavior is now a FAIL. New .upstreamer/skills/porting-tests/SKILL.md carries the mechanics the converter skill's four-line Tests step did not: how to find the upstream case for a changed behavior, a table of the existing fakes to reuse instead of writing new ones, the three SDK streaming pitfalls that silently produce a fake carrying no events, the anti-patterns (asserting internal shape, happy-path-only, unsynchronized test state, coverage theater on dead code), and how to prove a new test can actually fail. The TestHooksManagerAsyncDrain race and ExecuteToolLoop-as-footgun are used as the worked examples, since both are real and in-tree. Every file, helper, and upstream path referenced was verified to exist. Co-Authored-By: Claude --- .upstreamer/eval.md | 30 +++- .upstreamer/skills/porting-tests/SKILL.md | 157 ++++++++++++++++++ .../skills/upstreamer-converter/SKILL.md | 14 +- .upstreamer/upstreamer.md | 64 ++++++- PORTING.md | 18 +- upstreamer-changelog.md | 12 ++ 6 files changed, 286 insertions(+), 9 deletions(-) create mode 100644 .upstreamer/skills/porting-tests/SKILL.md diff --git a/.upstreamer/eval.md b/.upstreamer/eval.md index f528aa3..874363f 100644 --- a/.upstreamer/eval.md +++ b/.upstreamer/eval.md @@ -72,6 +72,31 @@ even on no-tools stream error paths. **Compatibility helpers.** Claude/Chat conversion round-trips preserve metadata, reasoning, tool use, and unsupported content. +**Test parity.** You have the upstream tree; the mechanical verifier does not, so +this comparison is yours to make and it is the one gate that can catch an +untested behavior change. + +```bash +ls tmp/upstreamer/upstream/packages/agent/tests/unit/ +git -C tmp/upstreamer/upstream diff --stat .. -- '*.test.ts' +``` + +For every behavior that changed in the delta, find upstream's test for it and +confirm this repo covers the same case. Report upstream cases with no Go +counterpart, and treat a changed behavior with no test as a finding — the port +compiled and passed its own suite, which is exactly the state a version-behind +port is in. + +Do **not** grade on test counts. Upstream has ~46 test files and ~619 `it` blocks +against this repo's ~70 `Test` funcs, but Go table-driven subtests bundle many +cases into one function, so the ratio means nothing. Compare *cases covered*, +per behavior. + +Also check the tests assert upstream-observable behavior — ordering, error +surfaces, stream boundaries, state shape — rather than the port's internal shape. +A test that merely pins current structure passes just as happily when the port is +wrong. + **Divergences are the documented ones.** Every difference from upstream is either in the contract's Idiomatic Divergences section or recorded as a compatibility note. An undocumented divergence is a finding. @@ -85,8 +110,9 @@ Return `PASS`, `PASS WITH WARNINGS`, or `FAIL` with concrete findings — file, symbol, and what specifically differs from upstream. - `FAIL` — a required API symbol is missing, a behavioral parity gap exists in the - load-bearing loop / state / approval ordering / hooks, or the declared version - overstates what was ported. + load-bearing loop / state / approval ordering / hooks, the declared version + overstates what was ported, or a behavior that changed in this delta landed + with no test covering it. - `PASS WITH WARNINGS` — parity holds on behavior; gaps are cosmetic, type-level, or already documented as divergences. - `PASS` — no findings. diff --git a/.upstreamer/skills/porting-tests/SKILL.md b/.upstreamer/skills/porting-tests/SKILL.md new file mode 100644 index 0000000..c47bc98 --- /dev/null +++ b/.upstreamer/skills/porting-tests/SKILL.md @@ -0,0 +1,157 @@ +--- +name: porting-tests +description: Write parity tests for a ported behavior in this Go port of @openrouter/agent. Use when a port run changes behavior, adds a required-API symbol, touches streaming or the tool loop, or when verify.sh reports a coverage-gate failure. +--- + +# Porting tests + +Companion to `upstreamer-converter`. That skill covers *how to port code*; this +one covers *how to prove the port is right*. The contract's Test Quality section +is the binding rule — this is the execution detail. + +## The failure this prevents + +A port can be wrong in a way that every mechanical check misses: + +- The symbol is exported → the verifier's presence check passes. +- The code compiles → build passes. +- No test calls it → nothing detects that it is wrong. + +`FinishReasonIs` lived in exactly that state: listed in the contract's Required +Public API, exported, **0% covered**. It is now gated, but the shape of the +mistake recurs. Assume your next ported symbol is in that state until a test +fails when you break it. + +## Start from upstream's test, not from the code + +Upstream's tests are the most precise statement of the behavior contract that +exists. Porting the code without porting its test means re-deriving intent from +an implementation. + +```bash +# What upstream tests cover this area? +ls tmp/upstreamer/upstream/packages/agent/tests/unit/ +# Which tests changed in this delta? These are the behaviors that moved. +git -C tmp/upstreamer/upstream diff --stat .. -- '*.test.ts' +# Read the case, then port it. +git -C tmp/upstreamer/upstream diff .. -- '*hooks*.test.ts' +``` + +A changed `*.test.ts` with no corresponding change here is the single strongest +signal of a parity gap. Upstream also keeps `*-adversarial.test.ts` files — +those are edge-case suites, and edge cases are where ports break. + +## Reuse the existing fakes + +Do not invent a new fake. Parallel fakes drift apart from production and from +each other, and a fake that no longer resembles real traffic hides the bugs it +was built to catch. + +| Need | Use | Where | +|---|---|---| +| One or more canned non-streaming responses | `fakeSender` | `model_result_test.go` | +| Two-turn tool round | `twoTurnSender` | `model_result_hooks_test.go` | +| A real multi-delta SSE stream | `streamingResponse` / `eventStreamFrom` | `stream_fake_test.go` | +| A stream that errors mid-frame | `failingEventStream` | `orchestration_test.go` | +| HTTP-level middleware | `fakeHTTPClient` | `middleware_test.go` | +| A completed response body | `completedResponse` | `stream_fake_test.go` | +| A paused (awaiting-approval) run | `pausedResult` | `result_accessors_test.go` | +| Force a tool call in a live e2e test | `requiredToolChoice` | `e2e_test.go` | + +## Streaming tests + +Most of this package's interesting behavior only happens on a stream. A +non-streaming fake response exercises a *fallback* path — it does not test +streaming at all. Before the `stream_fake_test.go` helpers existed, the suite had +26 non-streaming fakes and one error-only stream, which left +`consumeCreateResponse`'s success loop at 42% and `ReasoningStream` structurally +unreachable. + +Three SDK details, each of which silently produces a fake that looks fine and +tests nothing: + +1. **Build events with the SDK's `Create*` constructors.** They set the union's + `type` discriminator *and* its member pointer together, as `UnmarshalJSON` + does for real traffic. A hand-built struct literal can leave `type` empty, + which still satisfies a pointer nil-check while failing every `Type`-based + predicate — so the fake disagrees with production. +2. **Marshal the typed value, never `map[string]any`.** `StreamEvents` has a + custom `MarshalJSON` that flattens the active union member; a map loses it and + the event decodes back as `Type: "UNKNOWN"` with **no error**. +3. **The SDK re-wraps the SSE `data:` payload as `{"data": }`** before the + decoder sees it. So the frame body is the bare event, and the decoder + unmarshals the *envelope* (`ResponsesStreamingResponse`). Get this backwards + and every event decodes as `UNKNOWN`, again with no error. + +Because all three fail silently, `stream_fake_test.go` has +`TestSanityFakeStreamDecodesThroughSDK`, which asserts every event round-trips +with both its pointer and its `Type` set. **Keep that test.** If it fails, every +other streaming test is asserting against a fake that carries no events. + +Also note some SDK values must be valid to survive a wire round-trip: an empty +`ToolChoice` union will not marshal, and an empty `Object` will not unmarshal. +`completedResponse` sets both. + +## Anti-patterns + +**Asserting the port's own shape.** A test that pins current internal structure +passes when the port is wrong and fails when a correct refactor lands. Assert +what a *user* observes: request sequence, ordering, error surfaces, stream event +order and turn boundaries, serialized state shape, pause/resume semantics. + +**Happy path only.** Upstream fixes are edge cases. Test the error branch, the +empty input, the mixed turn, the resume — the reason the upstream commit exists. + +**Unsynchronized shared state.** The worked example is real: this suite's +`TestHooksManagerAsyncDrain` wrote a `bool` from a detached goroutine and read it +from the test goroutine. Production was fine; the *test* raced, and it failed the +moment `-race` became a gate. If a handler or goroutine writes a variable the +test later reads, guard it with a mutex or synchronize on a channel. + +**A test that passes without `-race`.** Always run `go test -race`. For this +package that is the primary correctness signal, not a nicety. + +**Coverage theater.** Do not test dead code to move the number. If nothing calls +a function, either wire production to use it or propose deleting it — testing it +raises coverage while adding maintenance surface. And if a function looks like the +real thing but is a simplified copy of it, testing it *endorses a footgun*: +`tool_orchestrator.go:ExecuteToolLoop` resembles the tool loop but passes a zero +`TurnContext` and nil emitter, silently dropping hooks, approval, and generator +streaming. The real loop is `executeToolCallsForTurn` in `model_result.go`. + +## Prove the test has teeth + +A test that cannot fail is worse than no test: it reports safety that does not +exist. Break the production code on purpose and confirm the test catches it. + +```bash +# 1. Make the behavior wrong (guard the branch with `if false`, or return early). +# 2. The new test MUST fail, and name the actual problem: +go test -run TestYourNewTest -count=1 . +# 3. Restore, and confirm the tree is clean: +git diff --stat +``` + +If it still passes, the test is asserting something other than what you meant. + +## Before handing off + +```bash +gofmt -l . | grep -v '^tmp/' # must be empty +go test -race -shuffle=on -count=1 ./... +go run honnef.co/go/tools/cmd/staticcheck@2025.1.1 ./... +.upstreamer/scripts/verify.sh # includes both coverage gates +env -u OPENROUTER_API_KEY go test -run TestE2E -v . # must SKIP, not fail +``` + +`-count=1` matters: without it a cached `ok` reports that some earlier tree +passed. `-shuffle=on` catches order dependence. + +If the coverage gate fails: + +- **Below the floor** → add tests. Never lower the floor; that is the same class + of error as hand-editing `state.yaml`. +- **Above the floor by >1.5 points** → raise the floor in + `.upstreamer/coverage-floor.txt` to lock the gain in. +- **A required-API symbol is never exercised** → the gate names the symbol. Write + a test that would fail if that symbol were broken. diff --git a/.upstreamer/skills/upstreamer-converter/SKILL.md b/.upstreamer/skills/upstreamer-converter/SKILL.md index 6274a8f..41c79db 100644 --- a/.upstreamer/skills/upstreamer-converter/SKILL.md +++ b/.upstreamer/skills/upstreamer-converter/SKILL.md @@ -78,13 +78,21 @@ substrate-pin section directs it. ## Step 4: Tests -Ported behavior without a test proves nothing. For every behavioral change: +Ported behavior without a test proves nothing. **Read the `porting-tests` skill +and follow it** — it carries the mechanics: which upstream test file to port +from, which existing fakes to reuse, and the anti-patterns that produce tests +which pass while the port is wrong. + +The contract's Test Quality section is binding; the short version: 1. Add or update deterministic tests in this repo's existing test layout and style. 2. Cover the specific upstream behavior that changed, not just the happy path. Upstream fixes are usually edge cases — that edge case is the test. -3. Tests must pass without network access or paid credentials. Live/e2e tests - must skip cleanly when credentials are absent. +3. Port upstream's own test case for the behavior rather than inventing one. +4. Assert upstream-observable behavior, never the port's internal shape. +5. Tests must pass without network access or paid credentials, and under + `-race`. Live/e2e tests must skip cleanly when credentials are absent. +6. Never lower `.upstreamer/coverage-floor.txt` to make a run go green. ## Step 5: Mechanical verification diff --git a/.upstreamer/upstreamer.md b/.upstreamer/upstreamer.md index c85acfe..e788c9d 100644 --- a/.upstreamer/upstreamer.md +++ b/.upstreamer/upstreamer.md @@ -201,11 +201,71 @@ File naming follows the existing layout: upstream `lib/tool-executor.ts` → navigable against the reference. Do not introduce nested packages without a contract change — the flat single-package shape is intentional. +Repo-owned, and never rewritten or deleted by a port run: + +```text +.github/ # CI and release workflows +LICENSE +scripts/upstream # the sync wrapper +.upstreamer/ # except state.yaml, eval-report.md, logs/ +.upstreamer/coverage-floor.txt # the coverage ratchet +``` + +A run that deletes its own gates would otherwise pass verification while +removing the checks that make the next run trustworthy, so `verify.sh` asserts +the CI workflow and the coverage floor are still present. + +## Test Quality + +Binding, not advisory. A port run that ships behavior without the tests below is +incomplete even if `go build` and `go test` pass. + +The failure mode this exists to prevent: a symbol is exported, so the verifier's +presence check passes; the code compiles, so the build passes; no test ever calls +it, so nothing detects that it is wrong. `FinishReasonIs` sat in exactly that +state — in this required-API list, exported, 0% covered. + +1. **Port upstream's test cases, not just upstream's code.** Upstream's own tests + in `packages/agent/**/*.test.ts` encode the behavior contract most precisely. + When a behavior changes, find the upstream case covering it and port that case. +2. **Assert upstream-observable behavior**, never the port's internal shape: + request sequence and ordering, error surfaces, stream event order and turn + boundaries, serialized state shape, pause/resume semantics. A test that only + pins the port's current structure passes just as happily when the port is + wrong, and blocks refactors that are actually correct. +3. **Cover the edge case, not the happy path.** Upstream fixes are usually edge + cases; that edge case *is* the test. +4. **Every required-API symbol needs a test that exercises it.** Adding a symbol + to the Required Public API list and adding its test are one change, not two. + `verify.sh` enforces this mechanically. +5. **Concurrency-touching code needs a test that is meaningful under `-race`.** + The race detector is a blocking gate. Shared state in a test must be + synchronized: an unsynchronized read/write across goroutines fails `-race` + even when the production code is correct. +6. **Deterministic and hermetic.** No network, no credentials, no wall-clock + sleeps for synchronization. Live tests must skip cleanly when credentials are + absent — the existing double gate is `OPENROUTER_API_KEY` plus + `OPENROUTER_AGENT_E2E=1`. +7. **Streaming behavior needs a streaming test.** Non-streaming fake responses + exercise a fallback path, not the stream. Build events with the SDK's + `Create*` constructors so the union's `type` discriminator and member pointer + are both set, as they are in real traffic; see the helpers in + `stream_fake_test.go`. +8. **The coverage floor may not be lowered.** `.upstreamer/coverage-floor.txt` is + a ratchet. If a run legitimately raises coverage, raise the floor with it. A + run that cannot meet the floor is a run that needs more tests, and lowering + the floor to go green is the same class of error as hand-editing `state.yaml`. + +Prefer extending the existing fakes over inventing new ones — `fakeSender`, +`twoTurnSender`, `streamingResponse`, `fakeHTTPClient`. Parallel ad-hoc fakes +fragment the suite and drift apart from production. + ## Verification `.upstreamer/scripts/verify.sh` must pass: `gofmt` check, `go build ./...`, -`go vet ./...`, `go test ./...`, plus the required-public-API presence check and -no-TS-artifact check. +`go vet ./...`, `go test ./...`, `go test -race ./...`, the coverage ratchet and +per-required-symbol coverage gates, plus the required-public-API presence check +and no-TS-artifact check. Then `.upstreamer/eval.md` must return PASS or PASS WITH WARNINGS from a fresh review context before state advances. diff --git a/PORTING.md b/PORTING.md index fb4db6f..774ebad 100644 --- a/PORTING.md +++ b/PORTING.md @@ -47,17 +47,26 @@ holds. |------|------| | `.upstreamer/upstreamer.md` | The rewrite contract. Binding. | | `.upstreamer/state.yaml` | Last commit ported *and* verified *and* eval-passed. | -| `.upstreamer/scripts/verify.sh` | Mechanical gate: build, lint, types, tests, required API. | +| `.upstreamer/scripts/verify.sh` | Mechanical gate: build, lint, types, tests, race, coverage, required API. | +| `.upstreamer/coverage-floor.txt` | Coverage ratchet. Raise it, never lower it. | | `.upstreamer/eval.md` | Parity gate: fresh-context behavioral review. | | `.upstreamer/eval-report.md` | Latest eval result, or a bankruptcy report. | | `.upstreamer/skills/upstreamer-converter/` | Execution discipline for the porting agent. | +| `.upstreamer/skills/porting-tests/` | How to write parity tests for a ported behavior. | | `.upstreamer/port.env` | Local secrets. **Gitignored.** | | `scripts/upstream` | The wrapper. | ## Two gates, and why state matters **Mechanical** (`verify.sh`) — objective: does it build, lint, type-check, pass -tests, and export every symbol the contract requires. +tests (including under `-race`), hold the coverage floor, exercise every +required-API symbol with at least one test, and export every symbol the contract +requires. + +Note what the two coverage gates buy. A presence check cannot see an exported +symbol that no test ever calls — that is how `FinishReasonIs` sat at 0% coverage +while passing verification. The per-symbol gate closes that hole; the ratchet +stops coverage from eroding one sync at a time. **Parity** (`eval.md`) — judgment, run in a fresh context that reads the upstream reference directly: does it actually *behave* like upstream. This is the gate that @@ -113,5 +122,10 @@ Review it as a *port*, not a normal diff: 2. Read the upstream delta yourself for anything load-bearing — the tool loop, state serialization, approval/HITL ordering, hooks, streaming. 3. Confirm new tests assert *upstream behavior*, not merely the port's own shape. + Read the upstream test file for the changed behavior and check the port covers + the same cases. Ignore test counts — Go subtests bundle cases, so the ratio to + upstream's `it` blocks is meaningless. 4. Any new naming mapping the run derived should be promoted into the contract's naming table. +5. Check the coverage floor moved up, or stayed put — never down. A lowered floor + is the same class of error as hand-editing `state.yaml`. diff --git a/upstreamer-changelog.md b/upstreamer-changelog.md index 15a6655..df94e6d 100644 --- a/upstreamer-changelog.md +++ b/upstreamer-changelog.md @@ -1,5 +1,17 @@ # go-agent Changelog +## Unreleased + +- Fixed a possible nil-pointer dereference in the `AllowFinalResponse` test path + and lower-cased the `stop hook: …` wrapped error string to follow Go + conventions (it was `Stop hook: …`). If you match on that error's text rather + than using `errors.Is`/`errors.As`, update the comparison. +- No behavior changes to the public API. The rest of this release is test and CI + hardening: the race detector, a coverage ratchet, and per-required-symbol + coverage are now enforced gates, and `ToolEventBroadcaster` plus the + `stream_guards.go` predicates are now used by the streaming loop instead of + sitting alongside duplicate inline logic. + ## Lifecycle Hooks, Versioned State, And 0.8.0 Parity (ported from `@openrouter/agent@0.8.0`) - Added `HooksManager` (`NewHooksManager`), a typed lifecycle-hook system with the nine built-in hooks — `PreToolUse`, `PostToolUse`, `PostToolUseFailure`, `UserPromptSubmit`, `Stop`, `PermissionRequest`, `SessionStart`, `SessionEnd`, and `PostModelCall` — plus fully custom hooks via the generic `On`/`Emit` functions. Register built-ins with the typed `OnXxx`/`EmitXxx` methods (`manager.OnPreToolUse(...)`, etc.).