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/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/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/.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/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/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") } } 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() +} diff --git a/model_result.go b/model_result.go index c955511..35dd14f 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}) }) @@ -1063,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 @@ -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/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]) } } 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/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 +} 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") + } +} 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.).