ci: bulletproof test gates + binding test-quality contract for the port

.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

.upstreamer/coverage-floor.txt

@@ -0,0 +1 @@
+72.0

.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 <last>..<target> -- '*.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.

.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

.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 <last>..<target> -- '*.test.ts'
+# Read the case, then port it.
+git -C tmp/upstreamer/upstream diff <last>..<target> -- '*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": <event>}`** 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.

.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