Skip to content

fixes missing stream termination for custom providers #7115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
akshaydeo merged 1 commit into from
Sep 12, 2026
Merged

fixes missing stream termination for custom providers #7115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 56 additions & 2 deletions .claude/skills/investigate-issue/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,8 +761,62 @@ Once all approved changes are applied:
make run-e2e FLOW=<feature>
```

2. Report results to the user, including the red-then-green transcript/summary for Bug issues (what failed before, what passes now)
3. If tests fail, investigate and propose fixes (with approval)
2. Run the provider harness (mandatory for every non-exempt wire-visible change, always scoped;
the exemptions are AGENTS.md's: no wire-visible effect, or behaviour no HTTP request can
reach, and an exempt change must say so in the report). It is a paid live sweep
against real provider accounts: the unfiltered collection is ~1,900 requests. The scope keeps
the run small and `HARNESS_MAX_REQUESTS` (below) is the enforced ceiling. Always use the shared
integration config via `APP_DIR=tests/integrations/python` (that is
`tests/integrations/python/config.json`) and scope the run to the change with `PROVIDER` and
`FEATURE`, or `SMOKE=1` for a cross-cutting change. Never run the unscoped sweep, and never
widen the scope beyond the change, without a separate explicit yes from the user that names
the scope.

Before launching, resolve the exact request count for the chosen scope (the same filter the
recipe applies) and put the command, the scope, and that count in the plan the user approves:
```bash
node tests/e2e/api/runners/augment-provider-harness.mjs --source tests/e2e/api/collections/provider-harness.json --out tmp/harness-augmented.json
# PROVIDER/FEATURE scope: one pass
node tests/e2e/api/runners/filter-collection.mjs --source tmp/harness-augmented.json --out tmp/harness-preflight.json --provider <provider> --feature "<keyword>"
# SMOKE=1: the recipe runs a parallel main pass plus a deferred cache-parity pass, so count both
node tests/e2e/api/runners/filter-collection.mjs --source tmp/harness-augmented.json --out tmp/harness-smoke-main.json --smoke tests/e2e/api/collections/smoke-manifest.json --exclude-feature-any cache-parity
node tests/e2e/api/runners/filter-collection.mjs --source tmp/harness-augmented.json --out tmp/harness-smoke-cache.json --smoke tests/e2e/api/collections/smoke-manifest.json --feature-any cache-parity
# stderr of each command ends with: [filter-collection] wrote ... with N requests after filter
```
Comment thread
coderabbitai[bot] marked this conversation as resolved.
State N for a scoped run, or N_main + N_cache for SMOKE=1, in the plan. When SMOKE=1 is
combined with PROVIDER, FEATURE or FOLDER, apply the same filters to both smoke commands.
The preflight is an estimate: the main pass forks one newman per provider and a producer
shared by several forks runs once per fork, so the live total can exceed the preflight sum
(observed: 102 preflight, 122 live for SMOKE=1). The enforced bound is
`HARNESS_MAX_REQUESTS`: always pass it with the ceiling the user approved. The recipe checks
every newman launch against its exact filtered count before it starts (main shards, 429
replays, the cache-parity pass, sequential mode); a launch that would cross the cap is
refused and the run exits 3, so the live total can never exceed the approved number. The
stream-cancellation probes are never sent under a cap because their count is not known up
front; if they are wanted, run them as a separately approved `SKIP_STREAM_CANCEL=` run
without the cap. After the run, quote the provider table's Total column as the actual.

Port 8080 is a blocking precondition. The recipe reuses any server whose `/health` answers
and then never starts the `APP_DIR` one, so a stale listener silently tests old code. Run
`lsof -nP -iTCP:8080 -sTCP:LISTEN` first: if it reports a listener you did not start on the
current working tree in this session, stop, ask the user to shut it down (never kill a
process you did not start), and recheck; do not run the target while `lsof` still reports
it. The one acceptable listener is Bifrost you started yourself from the code under test,
which is also the reliable way to run it, because a cold `make dev` from this config can
take longer than the recipe's 60s health wait:
```bash
make dev APP_DIR=tests/integrations/python # in the background; wait for /health = 200
```
```bash
make run-provider-harness-test APP_DIR=tests/integrations/python CI=1 HARNESS_MAX_REQUESTS=<approved ceiling> PROVIDER=<provider> FEATURE="<keyword>"
# cross-cutting change: the curated smoke set instead
make run-provider-harness-test APP_DIR=tests/integrations/python CI=1 HARNESS_MAX_REQUESTS=<approved ceiling> SMOKE=1
```
Report the provider status table and `tmp/harness-failures.md` findings, and state exactly
which scope ran. See AGENTS.md "Every fix ends with a provider-harness run".

3. Report results to the user, including the red-then-green transcript/summary for Bug issues (what failed before, what passes now)
4. If tests fail, investigate and propose fixes (with approval)

## Error Handling

Expand Down
16 changes: 16 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,22 @@ Insert into the collection surgically (a script that splices the new object in,

The narrow exemptions: changes with no wire-visible effect (comments, internal renames, log lines) and behaviour no HTTP request can reach. If a change is exempt, say so explicitly in the PR rather than leaving the omission unexplained.

### Every non-exempt wire-visible fix ends with a provider-harness run against `tests/integrations/python/config.json`

Unit tests and `make test-core` are not the finish line. The exemptions are the ones in the previous section: a change with no wire-visible effect (comments, internal renames, log lines, test-only or guidance-only edits) or behaviour no HTTP request can reach is exempt, and the report must say so explicitly. For everything else, after the Go-level red/green loop and the regression reruns, run the live provider harness with the shared integration config, scoped to the change with `PROVIDER` and `FEATURE` so the paid sweep stays small, and report the provider table from the run. `APP_DIR=tests/integrations/python` is the config directory the harness starts Bifrost from; pass it explicitly so a stale server or another config never answers for the code under test.

```bash
# Scoped to the change (preferred): the provider and a keyword from the affected cases
make run-provider-harness-test APP_DIR=tests/integrations/python CI=1 HARNESS_MAX_REQUESTS=<approved ceiling> PROVIDER=<provider> FEATURE="<keyword>"

# Curated ~100-request smoke set across all providers, when the change is cross-cutting
make run-provider-harness-test APP_DIR=tests/integrations/python CI=1 HARNESS_MAX_REQUESTS=<approved ceiling> SMOKE=1
```

`HARNESS_MAX_REQUESTS` is the enforced spend bound: the recipe checks every newman launch against its exact filtered request count before it starts and refuses any launch that would cross the cap (exit 3), so the live total never exceeds the approved number. Always pass it; the preflight count from `filter-collection.mjs` is only an estimate because shared producers repeat per provider fork. Stream-cancellation probes are never sent under a cap.

Port 8080 is a blocking precondition: the recipe reuses any server whose `/health` answers and never starts the `APP_DIR` one, so a stale listener silently tests old code. Run `lsof -nP -iTCP:8080 -sTCP:LISTEN` first; if it reports a listener you did not start from the current working tree, stop, have it shut down (never kill a process you did not start), and recheck before running the target. The one acceptable listener is Bifrost you started yourself from the code under test (`make dev APP_DIR=tests/integrations/python` in the background, then wait for `/health`), which is also the reliable pattern since a cold start can outlast the recipe's 60s health wait. For non-exempt changes, do not skip the run because it is paid or slow; scope it instead, and report exactly which scope ran.

### Always prefer `make test-core` over raw `go test` for provider-level tests

The `make test-core` target is the canonical harness for provider tests — it wires up env vars from `.env` (provider API keys), invokes the per-provider `{provider}_test.go` entrypoint in `core/providers/<provider>/`, and routes through the shared `core/internal/llmtests/` scenario suite that validates end-to-end behavior (including streaming).
Expand Down
63 changes: 59 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,12 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
printf ' %-18s %s\n' "HARNESS_JOBS=N" "Cap on concurrently running newman shards (default 100). The grid is ~168 live cells with sub-shards"; \
printf ' %-18s %s\n' "" " on, so the cap does block - which is why HARNESS_CLASSES is ordered slowest-first, to keep the long"; \
printf ' %-18s %s\n' "" " shards holding slots from the start. Lower it if a provider starts returning 429s."; \
printf ' %-18s %s\n' "HARNESS_MAX_REQUESTS=N" ""; \
printf ' %-18s %s\n' "" " Approved ceiling on paid requests for this run. Checked with each newman launch's EXACT filtered"; \
printf ' %-18s %s\n' "" " request count (main shards, 429 replays, the deferred cache-parity pass, sequential mode) before it"; \
printf ' %-18s %s\n' "" " starts: a launch that would cross the cap is refused, so the live total never exceeds N. Running"; \
printf ' %-18s %s\n' "" " shards drain and the report merges as usual; the target exits 3. Stream-cancellation probes are"; \
printf ' %-18s %s\n' "" " never sent under a cap (their count is not known up front). Unset = no cap (default)."; \
printf ' %-18s %s\n' "RETRY_429=N" "Max transient-failure retry attempts per shard (default 3; 0 disables). Covers 429 plus the two"; \
printf ' %-18s %s\n' "" " overload codes - 503 (OpenAI 'engine is currently overloaded') and 529 (Anthropic"; \
printf ' %-18s %s\n' "" " overloaded_error, which is its whole equivalent of 503; its error table has no 503). Other 5xx"; \
Expand Down Expand Up @@ -2157,6 +2163,26 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
if [ "$$HARNESS_QUIET" = "1" ] || [ "$$MONITOR_LIVE" = "1" ]; then "$$@" >> "$$QUIET_LOG" 2>&1; \
else "$$@"; fi; \
}; \
: "HARNESS_MAX_REQUESTS is the approved paid-request ceiling for this run. Every newman"; \
: "launch below passes its exact filtered request count here first; a launch that would"; \
: "cross the cap is refused and later launches are refused too, so the live total is"; \
: "bounded by construction rather than estimated from the preflight. Shards already"; \
: "running drain normally and the report still merges; the target then exits 3."; \
BUDGET_USED=0; BUDGET_EXCEEDED=0; \
: "tmp/harness-budget-used mirrors BUDGET_USED so a COMPAT=both parent can hand the"; \
: "remaining budget to its second sub-run instead of letting both spend the full cap."; \
mkdir -p tmp; printf '0' > tmp/harness-budget-used; \
budget_ok() { \
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if [ -z "$(HARNESS_MAX_REQUESTS)" ]; then return 0; fi; \
if [ "$$BUDGET_EXCEEDED" = "1" ] || [ $$((BUDGET_USED + $$1)) -gt "$(HARNESS_MAX_REQUESTS)" ]; then \
BUDGET_EXCEEDED=1; \
say "$(RED)[$$2] not launched: $$1 request(s) would push the total to $$((BUDGET_USED + $$1)), past HARNESS_MAX_REQUESTS=$(HARNESS_MAX_REQUESTS) (launched so far: $$BUDGET_USED)$(NC)"; \
return 1; \
fi; \
BUDGET_USED=$$((BUDGET_USED + $$1)); \
printf '%s' "$$BUDGET_USED" > tmp/harness-budget-used; \
return 0; \
}; \
start_monitor() { \
if [ -f tmp/harness-monitor.pid ]; then return 0; fi; \
if [ "$$HARNESS_QUIET" != "1" ] && [ ! -t 1 ]; then return 0; fi; \
Expand Down Expand Up @@ -2200,10 +2226,20 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
if [ "$(COMPAT)" = "both" ]; then \
mkdir -p tmp; \
say "$(CYAN)COMPAT=both: running harness with compat OFF then ON (sub-runs forced CI=1 to skip the interactive viewer)...$(NC)"; \
: "Both sub-runs share one HARNESS_MAX_REQUESTS: the second gets what the first left."; \
BOTH_REMAINING="$(HARNESS_MAX_REQUESTS)"; BOTH_USED=0; \
for mode in off on; do \
if [ -n "$$BOTH_REMAINING" ] && [ "$$BOTH_REMAINING" -le 0 ]; then \
say "$(RED)compat $$mode not run: HARNESS_MAX_REQUESTS=$(HARNESS_MAX_REQUESTS) already spent by the earlier sub-run ($$BOTH_USED launched)$(NC)"; \
BOTH_RC=3; continue; \
fi; \
say "$(CYAN)=== Harness run: compat $$mode ===$(NC)"; \
$(MAKE) run-provider-harness-test COMPAT=$$mode CI=1; \
$(MAKE) run-provider-harness-test COMPAT=$$mode CI=1 HARNESS_MAX_REQUESTS="$$BOTH_REMAINING"; \
RC=$$?; \
if [ -n "$$BOTH_REMAINING" ]; then \
SUB_USED="$$(cat tmp/harness-budget-used 2>/dev/null || echo 0)"; \
BOTH_USED=$$((BOTH_USED + SUB_USED)); BOTH_REMAINING=$$((BOTH_REMAINING - SUB_USED)); \
fi; \
mv -f tmp/newman-report.json "tmp/newman-report-compat-$$mode.json" 2>/dev/null || true; \
mv -f tmp/newman-report.html "tmp/newman-report-compat-$$mode.html" 2>/dev/null || true; \
mv -f tmp/harness-failures.md "tmp/harness-failures-compat-$$mode.md" 2>/dev/null || true; \
Expand Down Expand Up @@ -2587,6 +2623,9 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
rm -f "tmp/harness-filtered-$$SHARD.json"; \
continue; \
fi; \
: "A refused shard must not leave its filtered file behind: the status table counts"; \
: "every tmp/harness-filtered-*.json as planned work, so it would show rows that never ran."; \
budget_ok "$$P_ITEM_COUNT" "$$SHARD" || { rm -f "tmp/harness-filtered-$$SHARD.json"; continue; }; \
: "Block until a slot frees. 'wait -n' reaps one arbitrary child, which is why shard"; \
: "exit codes are recorded by the subshell into tmp/parallel-exit-<shard> instead of"; \
: "being collected later with 'wait <pid>' - that pid may already have been reaped here."; \
Expand All @@ -2603,6 +2642,10 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
done; \
done; \
if [ "$$LAUNCHED" -eq 0 ]; then \
if [ "$$BUDGET_EXCEEDED" = "1" ]; then \
say "$(RED)Aborted before any launch: every shard would exceed HARNESS_MAX_REQUESTS=$(HARNESS_MAX_REQUESTS). Raise the cap or narrow PROVIDER/FEATURE.$(NC)"; \
exit 3; \
fi; \
say "$(RED)No provider runs were launched. Check PROVIDER/FEATURE/FOLDER filters.$(NC)"; \
exit 1; \
fi; \
Expand Down Expand Up @@ -2663,7 +2706,9 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
say "$(YELLOW)[$$rs] retry filter failed - keeping the original verdict$(NC)"; \
continue; \
fi; \
[ "$$(grep -c '"request":' "$$RETRY_COLL" 2>/dev/null || echo 0)" -eq 0 ] && continue; \
RETRY_COUNT="$$(grep -c '"request":' "$$RETRY_COLL" 2>/dev/null || true)"; RETRY_COUNT="$${RETRY_COUNT:-0}"; \
[ "$$RETRY_COUNT" -eq 0 ] && continue; \
budget_ok "$$RETRY_COUNT" "$$rs-retry$$RETRY_ATTEMPT" || { rm -f "$$RETRY_COLL"; continue; }; \
while [ "$$(shard_jobs)" -ge "$$JOBS_CAP" ]; do wait -n 2>/dev/null || true; done; \
( \
newman_shard "$$rs-retry$$RETRY_ATTEMPT" "$$RETRY_COLL" "tmp/newman-report-$$rs-retry$$RETRY_ATTEMPT.json" "$$rp"; \
Expand Down Expand Up @@ -2733,6 +2778,8 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
else \
SEQ_PROVIDERS="$(or $(PROVIDER),$(HARNESS_PROVIDERS))"; \
: > tmp/newman-cli.log; \
SEQ_COUNT="$$(grep -c '"request":' "$$COLLECTION_FILE" 2>/dev/null || true)"; SEQ_COUNT="$${SEQ_COUNT:-0}"; \
if budget_ok "$$SEQ_COUNT" main; then \
add_pass "$$(printf '{"t":"pass","id":"main","mode":"sequential","log":"tmp/newman-cli.log","collection":"%s"}' "$$COLLECTION_FILE")"; \
newman run "$$COLLECTION_FILE" \
--env-var "baseUrl=$$BASE_URL_VAL" \
Expand Down Expand Up @@ -2762,6 +2809,7 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
--reporter-htmlextra-darkTheme > tmp/newman-cli.log 2>&1; \
NEWMAN_EXIT=$$?; \
end_pass main; \
else NEWMAN_EXIT=0; fi; \
if [ "$$HARNESS_MONITORED" != "1" ] && [ "$$HARNESS_QUIET" != "1" ]; then cat tmp/newman-cli.log; fi; \
if command -v jq >/dev/null 2>&1 && [ -f tmp/newman-report.json ]; then \
say "$(CYAN)Sanitizing tmp/newman-report.json (newman embeds the whole parent folder in every failure)...$(NC)"; \
Expand All @@ -2782,7 +2830,8 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
$(if $(FOLDER),--folder "$(FOLDER)",) \
$${SMOKE_MANIFEST:+--smoke "$$SMOKE_MANIFEST"} \
$(if $(PROVIDER),--provider $(PROVIDER),) || { say "$(RED)Cache parity filter step failed$(NC)"; }; \
if [ -f tmp/harness-cache-filtered.json ]; then \
CACHE_COUNT="$$(grep -c '"request":' tmp/harness-cache-filtered.json 2>/dev/null || true)"; CACHE_COUNT="$${CACHE_COUNT:-0}"; \
if [ -f tmp/harness-cache-filtered.json ] && budget_ok "$$CACHE_COUNT" cache-parity; then \
CACHE_PROVIDERS="$(or $(PROVIDER),$(HARNESS_PROVIDERS))"; \
: > tmp/newman-cli-cache-parity.log; \
add_pass '{"t":"pass","id":"cache-parity","mode":"sequential","log":"tmp/newman-cli-cache-parity.log","collection":"tmp/harness-cache-filtered.json"}'; \
Expand Down Expand Up @@ -2825,7 +2874,9 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
: "prints normally to a restored main screen."; \
say "$(GREEN)Newman finished. Reports: tmp/newman-report.{json,html} + tmp/newman-cli.log$(NC)"; \
STREAM_CANCEL_EXIT=0; \
if [ -z "$(SKIP_STREAM_CANCEL)" ] && [ -z "$(RERUN_FAILED)" ] && [ "$(PROVIDER)" != "passthrough" ] && { [ -z "$(FOLDER)" ] || printf '%s' "$(FOLDER)" | grep -qi 'stream'; }; then \
if [ -n "$(HARNESS_MAX_REQUESTS)" ]; then \
say "$(YELLOW)Skipping stream cancellation probes: their request count is not known before they run, so they are never sent under HARNESS_MAX_REQUESTS.$(NC)"; \
elif [ -z "$(SKIP_STREAM_CANCEL)" ] && [ -z "$(RERUN_FAILED)" ] && [ "$(PROVIDER)" != "passthrough" ] && { [ -z "$(FOLDER)" ] || printf '%s' "$(FOLDER)" | grep -qi 'stream'; }; then \
say "$(CYAN)Running stream cancellation probes...$(NC)"; \
$(USE_NODE); node tests/e2e/api/runners/run-stream-cancellation.mjs \
--base-url "$$BASE_URL_VAL" \
Expand Down Expand Up @@ -2884,5 +2935,9 @@ run-provider-harness-test: $(if $(HELP),,install-newman) ## Run the Bifrost prov
say "$(GREEN)Viewer closed.$(NC)"; \
fi; \
fi; \
if [ "$$BUDGET_EXCEEDED" = "1" ]; then \
say "$(RED)Aborted: HARNESS_MAX_REQUESTS=$(HARNESS_MAX_REQUESTS) reached; $$BUDGET_USED request(s) were launched. The provider table above is the actual total.$(NC)"; \
exit 3; \
fi; \
if [ "$$NEWMAN_EXIT" -ne 0 ]; then exit $$NEWMAN_EXIT; fi; \
exit $$STREAM_CANCEL_EXIT
Loading
Loading