tests: add e2e model-catalog wiring tests for openai, anthropic, and gemini - #4250
Merged
Pratham-Mishra04 merged 1 commit intoJun 11, 2026
Conversation
Collaborator
Author
This was referenced Jun 10, 2026
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Node.js CLI that generates a Postman collection of provider-scoped model-catalog wiring scenarios and a Bash Newman runner that parses a seed env, forwards select vars, runs the collection against a live Bifrost instance, and emits an HTML report. ChangesModel Catalog Wiring E2E Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Jun 10, 2026
|
|
Contributor
Confidence Score: 5/5Safe to merge — only new test infrastructure is added, no production code is touched. All three files are new test-only additions. The generator, collection, and runner are self-contained and do not affect any production path. The tests/e2e/api/runners/build-model-catalog-wiring.mjs — the Important Files Changed
Reviews (3): Last reviewed commit: "tests: add e2e model-catalog wiring test..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/api/runners/individual/run-newman-model-catalog-wiring-tests.sh`:
- Around line 118-122: The conditional that appends "--reporter-cli-no-failures
false" is misleading and a no-op for controlling execution flow; remove the
entire if block that checks ci_normalized and the
cmd+=(--reporter-cli-no-failures false) line, or if you actually intended to
change Newman’s stop-on-failure behavior replace that append with the correct
flag "--bail" (or "--bail <count>") instead; locate the check using the
ci_normalized variable and the cmd+=(--reporter-cli-no-failures false)
expression to make the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01eb7e0b-6d31-4265-a8d2-899213878d67
📒 Files selected for processing (3)
tests/e2e/api/collections/bifrost-model-catalog-wiring.postman_collection.jsontests/e2e/api/runners/build-model-catalog-wiring.mjstests/e2e/api/runners/individual/run-newman-model-catalog-wiring-tests.sh
Comment on lines
+118
to
+122
| # In CI keep going past failures so every scenario's cleanup folder runs. | ||
| ci_normalized="$(printf '%s' "${CI:-}" | tr '[:upper:]' '[:lower:]')" | ||
| if [ "$ci_normalized" = "1" ] || [ "$ci_normalized" = "true" ]; then | ||
| cmd+=(--reporter-cli-no-failures false) | ||
| fi |
Contributor
There was a problem hiding this comment.
The --reporter-cli-no-failures false flag doesn't achieve the stated intent.
The comment says "keep going past failures so cleanup runs," but:
--reporter-cli-no-failurescontrols whether failure details are printed in CLI output, not execution flow- Setting it to
falseis already the default (failures are shown) - To control whether Newman stops on first failure, use
--bail(default isfalse, i.e., continue)
Newman already continues past failures by default, so the code works—but the explicit flag is a no-op and the comment is misleading. Either remove this block (since the default behavior is what you want) or correct the flag if different behavior was intended.
🛠️ Suggested fix: remove the no-op flag or clarify intent
Option A: Remove the block entirely (Newman continues past failures by default):
-# In CI keep going past failures so every scenario's cleanup folder runs.
-ci_normalized="$(printf '%s' "${CI:-}" | tr '[:upper:]' '[:lower:]')"
-if [ "$ci_normalized" = "1" ] || [ "$ci_normalized" = "true" ]; then
- cmd+=(--reporter-cli-no-failures false)
-fiOption B: If you want to suppress failure noise in CI (opposite intent):
# In CI keep going past failures so every scenario's cleanup folder runs.
+# Suppress verbose failure details in CI logs; failures are still reported in summary.
ci_normalized="$(printf '%s' "${CI:-}" | tr '[:upper:]' '[:lower:]')"
if [ "$ci_normalized" = "1" ] || [ "$ci_normalized" = "true" ]; then
- cmd+=(--reporter-cli-no-failures false)
+ cmd+=(--reporter-cli-no-failures)
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/api/runners/individual/run-newman-model-catalog-wiring-tests.sh`
around lines 118 - 122, The conditional that appends "--reporter-cli-no-failures
false" is misleading and a no-op for controlling execution flow; remove the
entire if block that checks ci_normalized and the
cmd+=(--reporter-cli-no-failures false) line, or if you actually intended to
change Newman’s stop-on-failure behavior replace that append with the correct
flag "--bail" (or "--bail <count>") instead; locate the check using the
ci_normalized variable and the cmd+=(--reporter-cli-no-failures false)
expression to make the change.
Pratham-Mishra04
force-pushed
the
06-10-tests_add_e2e_routing_wiring_test_harness
branch
from
June 11, 2026 06:32
0e59caa to
d742bbb
Compare
Pratham-Mishra04
force-pushed
the
06-10-tests_add_e2e_model-catalog_wiring_tests_for_openai_anthropic_and_gemini
branch
from
June 11, 2026 06:32
a58e9c7 to
bdee3c5
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/api/runners/build-model-catalog-wiring.mjs`:
- Around line 79-98: The listModelsAssertLines helper currently treats
expectSubset and expectSuperset identically (only presence checks); update
listModelsAssertLines so that after the presence checks it performs an
exhaustiveness check when expectSuperset.length > 0: compute the filtered
provider names array (names) and verify names.length === expectSuperset.length
and that every element of names exists in expectSuperset (or compare sorted JSON
representations) to ensure no extra models are present; keep the existing
expectSubset presence checks but ensure you reference expectSubset,
expectSuperset, names, and expectEmpty in the new logic.
- Around line 152-157: The updateKey case is passing undefined as the name to
keyBody which results in a name: undefined property; either preserve the
original key name by passing step.key.name into keyBody in the update branch
(case "updateKey") or change keyBody to omit the name property when its
parameter is undefined (use conditional property addition inside keyBody) so PUT
payloads do not include name: undefined; update the case that calls keyBody
and/or the keyBody implementation accordingly (symbols: the "updateKey" case,
step.key, keyBody, keyId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f61ee0d-9bff-43af-846d-1acf879427a1
📒 Files selected for processing (3)
tests/e2e/api/collections/bifrost-model-catalog-wiring.postman_collection.jsontests/e2e/api/runners/build-model-catalog-wiring.mjstests/e2e/api/runners/individual/run-newman-model-catalog-wiring-tests.sh
Comment on lines
+79
to
+98
| function listModelsAssertLines(sid, { subset = [], superset = [], absent = [], empty = false }) { | ||
| return [ | ||
| `var providerName = ${jsProviderName(sid)};`, | ||
| `var expectSubset = ${JSON.stringify(subset)};`, | ||
| `var expectSuperset = ${JSON.stringify(superset)};`, | ||
| `var expectAbsent = ${JSON.stringify(absent)};`, | ||
| `var expectEmpty = ${empty ? "true" : "false"};`, | ||
| "if (pm.response.code !== 200) { throw new Error('list models status ' + pm.response.code); }", | ||
| "var body = pm.response.json();", | ||
| "var names = (body.models || []).filter(function (m) { return m.provider === providerName; })", | ||
| " .map(function (m) { return m.name; });", | ||
| "if (expectEmpty && names.length !== 0) { throw new Error('expected no models for ' + providerName + ' but got ' + JSON.stringify(names)); }", | ||
| "expectSubset.concat(expectSuperset).forEach(function (m) {", | ||
| " if (names.indexOf(m) < 0) { throw new Error('expected model ' + m + ' in ' + JSON.stringify(names)); }", | ||
| "});", | ||
| "expectAbsent.forEach(function (m) {", | ||
| " if (names.indexOf(m) >= 0) { throw new Error('expected model ' + m + ' absent but found in ' + JSON.stringify(names)); }", | ||
| "});", | ||
| ]; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Clarify subset vs superset semantics or implement exhaustiveness check.
Both subset and superset parameters are treated identically (line 91) — they only verify the listed models are present, without checking whether additional models exist. A true superset assertion should validate that the listed models are the only models present (exhaustiveness check).
Current usage (line 268) labels it "both keys' models present", which suggests a presence check rather than exclusivity. Either:
- Rename
supersetto something likeexpectedModelsto match actual behavior - Implement true exhaustiveness: after validating presence, check
names.length === expectSuperset.lengthwhenexpectSuperset.length > 0
♻️ Option 2: Implement exhaustive superset check
"expectSubset.concat(expectSuperset).forEach(function (m) {",
" if (names.indexOf(m) < 0) { throw new Error('expected model ' + m + ' in ' + JSON.stringify(names)); }",
"});",
+"if (expectSuperset.length > 0 && names.length !== expectSuperset.length) {",
+" throw new Error('expected exactly ' + expectSuperset.length + ' models but found ' + names.length + ': ' + JSON.stringify(names));",
+"}",
"expectAbsent.forEach(function (m) {",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/api/runners/build-model-catalog-wiring.mjs` around lines 79 - 98,
The listModelsAssertLines helper currently treats expectSubset and
expectSuperset identically (only presence checks); update listModelsAssertLines
so that after the presence checks it performs an exhaustiveness check when
expectSuperset.length > 0: compute the filtered provider names array (names) and
verify names.length === expectSuperset.length and that every element of names
exists in expectSuperset (or compare sorted JSON representations) to ensure no
extra models are present; keep the existing expectSubset presence checks but
ensure you reference expectSubset, expectSuperset, names, and expectEmpty in the
new logic.
Comment on lines
+152
to
+157
| case "updateKey": { | ||
| const name = uniq("update key " + step.key.id); | ||
| items.push(item(nextId("update-key"), name, | ||
| request("PUT", url(["api", "providers", seg, "keys", keyId(sid, step.key.id)]), keyBody(step.key, sid, undefined, provider.keyEnv)), | ||
| events(null, mutationTest(name, [200], cleanupName)))); | ||
| break; |
Contributor
There was a problem hiding this comment.
Clarify key name handling on updates.
The updateKey step (line 155) passes undefined as the name parameter to keyBody(), which will include name: undefined in the resulting object. This is unclear:
- The comment on lines 48-50 says updates "resend the full intended state (value, models, enabled, aliases)" but doesn't mention
name - If name is immutable and should be omitted on updates, use conditional property addition in
keyBody()instead of passingundefined - If name should be included, it needs to be preserved from the original key
Verify the API contract for key updates:
#!/bin/bash
# Check how key updates are handled elsewhere in the codebase
echo "=== Checking key update patterns ==="
rg -n "PUT.*providers.*keys" --type ts --type js --type go -C3 -g '!node_modules'
echo -e "\n=== Checking key update payloads ==="
ast-grep --pattern $'request("PUT", url([$$$, "keys", $$$]), $BODY)'♻️ Suggested clarification if name should be omitted
function keyBody(k, sid, name, keyEnv) {
const out = {
id: keyId(sid, k.id),
- name,
value: `env.${keyEnv}`,
models: [...k.models],
enabled: k.enabled,
};
+ if (name !== undefined) out.name = name;
if (k.blacklisted.length) out.blacklisted_models = [...k.blacklisted];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/api/runners/build-model-catalog-wiring.mjs` around lines 152 - 157,
The updateKey case is passing undefined as the name to keyBody which results in
a name: undefined property; either preserve the original key name by passing
step.key.name into keyBody in the update branch (case "updateKey") or change
keyBody to omit the name property when its parameter is undefined (use
conditional property addition inside keyBody) so PUT payloads do not include
name: undefined; update the case that calls keyBody and/or the keyBody
implementation accordingly (symbols: the "updateKey" case, step.key, keyBody,
keyId).
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
06-10-tests_add_e2e_routing_wiring_test_harness
to
graphite-base/4250
June 11, 2026 07:18
Pratham-Mishra04
force-pushed
the
06-10-tests_add_e2e_model-catalog_wiring_tests_for_openai_anthropic_and_gemini
branch
from
June 11, 2026 07:20
bdee3c5 to
a6736d3
Compare
Pratham-Mishra04
deleted the
06-10-tests_add_e2e_model-catalog_wiring_tests_for_openai_anthropic_and_gemini
branch
June 11, 2026 07:22
akshaydeo
pushed a commit
that referenced
this pull request
Jun 12, 2026
…gemini (#4250) ## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added a comprehensive end-to-end test suite for model catalog wiring across OpenAI, Anthropic, and Gemini: generates Postman collections for isolated provider scenarios, validates provider/key lifecycle, model allowlists/blacklists and alias resolution, and verifies catalog and inference endpoints. * Added a Bash test runner that executes the collection with robust env handling, CI-friendly behavior, and HTML reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
…gemini (maximhq#4250) ## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added a comprehensive end-to-end test suite for model catalog wiring across OpenAI, Anthropic, and Gemini: generates Postman collections for isolated provider scenarios, validates provider/key lifecycle, model allowlists/blacklists and alias resolution, and verifies catalog and inference endpoints. * Added a Bash test runner that executes the collection with robust env handling, CI-friendly behavior, and HTML reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
…gemini (maximhq#4250) ## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added a comprehensive end-to-end test suite for model catalog wiring across OpenAI, Anthropic, and Gemini: generates Postman collections for isolated provider scenarios, validates provider/key lifecycle, model allowlists/blacklists and alias resolution, and verifies catalog and inference endpoints. * Added a Bash test runner that executes the collection with robust env handling, CI-friendly behavior, and HTML reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit