Conversation
|
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 Walkthrough📝 Walkthrough🚥 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 unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 10 minutes and 52 seconds. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Confidence Score: 5/5Safe to merge; findings are P2 only and do not affect correctness of the core normalisation feature. All findings are P2: one is dead code that never causes a wrong result (both branches already return the same value), and one is a normalisation gap on a path that was also un-normalised before this PR. The new Go logic is correct, well-tested, and the Vertex path is properly wired up. core/providers/anthropic/utils.go — the Anthropic-native raw-body Responses path does not enable RemapToolVersions; tests/e2e/api/runners/analyze-failures.mjs — dead code in the 404 handler. Important Files Changed
Reviews (4): Last reviewed commit: "test harness for quick checks" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/e2e/api/collections/provider-harness.json (1)
5-6: ⚡ Quick winAdd Files API smoke requests to align harness coverage with stack objective (
#123).This harness validates many provider surfaces but not file-upload/list/retrieve/delete flows, so the Files API objective is still uncovered by the quick-check path.
As per coding guidelines, “always check the stack if there is one for the current PR... see all changes in the light of the whole stack of PRs.”
Also applies to: 248-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/collections/provider-harness.json` around lines 5 - 6, The harness JSON (provider-harness.json) omits Files API flows; add smoke requests exercising file upload, list, retrieve, and delete to the collection so the quick-check path covers the Files API objective; specifically add entries that POST to the Files upload endpoint, GET the files list, GET a file by id, and DELETE a file by id (use the same auth/baseUrl variable pattern used by existing requests like the chat/completions entries) and wire them into the existing run sequence so they run as part of the provider-harness smoke-test.Makefile (1)
1536-1622: ⚡ Quick winRefactor
run-provider-harness-testbody into a script to reduce Makefile complexity.The target is doing orchestration, process lifecycle, and viewer control in one recipe block. Given the current length (and existing lint warning), extracting to a dedicated shell script will be easier to maintain and review.
🧩 Refactor direction
run-provider-harness-test: install-newman ## ... - `@mkdir` -p tmp - @$(EXPOSE_ENV); \ - ...long body... - exit $$NEWMAN_EXIT + `@mkdir` -p tmp + @$(EXPOSE_ENV); \ + $$CMD_PREFIX bash tests/e2e/api/runners/run-provider-harness.sh \ + "$(or $(BASE_URL),http://localhost:8080)" \ + "$(or $(APP_DIR),tests/integrations/python)" \ + "$(or $(VIEWER_PORT),8090)" \ + "$(ENV_FILE)" \ + "$(FOLDER)" \ + "$${CI:-$(CI)}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 1536 - 1622, The run-provider-harness-test target body is too large and should be moved into a dedicated executable shell script; create a script (e.g., scripts/run-provider-harness-test.sh) that implements the current orchestration including BASE_URL/APP_DIR/VIEWER_PORT defaults, the cleanup() and preempt_viewer_port() logic, launching/stopping bifrost, running newman with the same flags (including reporter exports to tmp/newman-report.*), launching tests/e2e/api/runners/harness-viewer.mjs when not CI, and returning newman exit code; then simplify the Makefile target run-provider-harness-test to export any required env, call that script with forwarded arguments/ENV_FILE/INFISICAL vars, and ensure the script is executable and preserves the exact behaviors referenced by names in the diff (cleanup, preempt_viewer_port, harness-viewer.mjs, tmp/bifrost-dev.pid, tmp/harness-viewer.pid, tmp/newman-report.*).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/api/collections/provider-harness.json`:
- Around line 23-30: The test harness currently reuses openaiKey for Azure which
couples providers; add a dedicated azureKey entry and replace any uses of
{{openaiKey}} for Azure auth with {{azureKey}} (update the
azureDeployment/azureApiVersion bindings to reference the new azureKey), and
ensure any other references (including the occurrences noted around lines
179-180) and CI/test env variables are updated to provide the new azureKey
value.
- Around line 14-16: The test name "pm.test('Status code is 2xx', ...)" is
inconsistent with the assertion which allows any status < 400; update the
assertion in the pm.expect call to enforce a strict 2xx range (e.g., assert
pm.response.code is >= 200 and < 300) or alternatively rename the pm.test label
to reflect "< 400" if you intend to allow 3xx; modify the pm.test / pm.expect
block accordingly so the test name and condition match.
In `@tests/e2e/api/runners/harness-viewer.mjs`:
- Around line 287-295: The forwarded fetch call can hang because it has no
timeout; wrap the request with an AbortController (create controller, pass
controller.signal into fetch) and start a timeout (e.g., const timer =
setTimeout(() => controller.abort(), RESEND_TIMEOUT_MS)) before calling fetch;
after fetch completes clearTimeout(timer). Also catch abort/fetch errors and
return a proper timeout response (e.g., status 504 and elapsedMs) instead of
hanging. Update the fetch invocation that uses url, method, headerObj, body and
ensure the timer is cleared on success or failure so the request cannot hang
indefinitely.
- Around line 278-292: The /api/resend handler currently forwards any
client-supplied url/method; restrict it by validating the parsed url and method
against the preloaded harness items and allowed schemes before calling fetch. In
the POST branch that parses raw/JSON (the handler around req.method === "POST"
&& u.pathname === "/api/resend"), check that the parsed url's protocol is
http(s) and that the hostname/path (or a full URL string) exists in the
in-memory harness list (e.g., the collection used to render harness items) and
that the requested method is one of the allowed methods for that harness item;
if validation fails return a 400/403 without calling fetch. Apply the same
header reconstruction (headerObj) only after validation and keep the existing
GET/HEAD body logic.
---
Nitpick comments:
In `@Makefile`:
- Around line 1536-1622: The run-provider-harness-test target body is too large
and should be moved into a dedicated executable shell script; create a script
(e.g., scripts/run-provider-harness-test.sh) that implements the current
orchestration including BASE_URL/APP_DIR/VIEWER_PORT defaults, the cleanup() and
preempt_viewer_port() logic, launching/stopping bifrost, running newman with the
same flags (including reporter exports to tmp/newman-report.*), launching
tests/e2e/api/runners/harness-viewer.mjs when not CI, and returning newman exit
code; then simplify the Makefile target run-provider-harness-test to export any
required env, call that script with forwarded arguments/ENV_FILE/INFISICAL vars,
and ensure the script is executable and preserves the exact behaviors referenced
by names in the diff (cleanup, preempt_viewer_port, harness-viewer.mjs,
tmp/bifrost-dev.pid, tmp/harness-viewer.pid, tmp/newman-report.*).
In `@tests/e2e/api/collections/provider-harness.json`:
- Around line 5-6: The harness JSON (provider-harness.json) omits Files API
flows; add smoke requests exercising file upload, list, retrieve, and delete to
the collection so the quick-check path covers the Files API objective;
specifically add entries that POST to the Files upload endpoint, GET the files
list, GET a file by id, and DELETE a file by id (use the same auth/baseUrl
variable pattern used by existing requests like the chat/completions entries)
and wire them into the existing run sequence so they run as part of the
provider-harness smoke-test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c7fe6ac-d2e3-4273-a53e-12be743ffcbf
📒 Files selected for processing (3)
Makefiletests/e2e/api/collections/provider-harness.jsontests/e2e/api/runners/harness-viewer.mjs
d7bc1b8 to
6d4e381
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
386-392:⚠️ Potential issue | 🟠 MajorAdd trailing backslash to line 391.
Line 391 is missing a trailing backslash, causing line 392 to be treated as a separate shell command. When
make run APP_DIR=...is called, the-app-dirargument won't be passed to the bifrost-http binary.@./tmp/bifrost-http \ -host "$(HOST)" \ -port "$(PORT)" \ -log-style "$(LOG_STYLE)" \ -log-level "$(LOG_LEVEL)" \ - $(if $(PROMETHEUS_LABELS),-prometheus-labels "$(PROMETHEUS_LABELS)") + $(if $(PROMETHEUS_LABELS),-prometheus-labels "$(PROMETHEUS_LABELS)") \ $(if $(APP_DIR),-app-dir "$(abspath $(APP_DIR))")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 386 - 392, The Makefile command invoking ./tmp/bifrost-http treats the $(if $(APP_DIR),-app-dir "$(abspath $(APP_DIR))") token as a separate shell command because the previous line (the $(if $(PROMETHEUS_LABELS)... line) is missing a trailing backslash; fix by adding a trailing backslash to the end of the $(if $(PROMETHEUS_LABELS),-prometheus-labels "$(PROMETHEUS_LABELS)") line so the -app-dir argument is passed as part of the same command to the bifrost-http invocation.
🧹 Nitpick comments (2)
core/providers/anthropic/utils_test.go (2)
2190-2286: ⚡ Quick winAdd at least one non-Anthropic provider case to verify provider gating.
RemapRawToolVersionsForProvideris provider-aware, but this table only exercisesschemas.Anthropic(Line 2285). A Vertex/Bedrock no-remap case would protect against accidental cross-provider rewriting.Diff suggestion
cases := []struct { name string + provider schemas.ModelProvider model string inputBody string expected []expectedTool }{ { name: "sonnet-4-6 with new-gen tools (no-op)", + provider: schemas.Anthropic, model: "claude-sonnet-4-6", inputBody: `{"model":"claude-sonnet-4-6","tools":[ {"type":"computer_20251124","name":"computer","display_width_px":1024,"display_height_px":768}, {"type":"text_editor_20250728","name":"str_replace_based_edit_tool"}, {"type":"bash_20250124","name":"bash"} ]}`, @@ }, + { + name: "vertex provider does not remap anthropic computer-use versions", + provider: schemas.Vertex, + model: "claude-sonnet-4-6", + inputBody: `{"model":"claude-sonnet-4-6","tools":[ + {"type":"text_editor_20250124","name":"str_replace_editor"} + ]}`, + expected: []expectedTool{ + {"text_editor_20250124", "str_replace_editor"}, + }, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - out, err := RemapRawToolVersionsForProvider([]byte(tc.inputBody), schemas.Anthropic, tc.model) + provider := tc.provider + if provider == "" { + provider = schemas.Anthropic + } + out, err := RemapRawToolVersionsForProvider([]byte(tc.inputBody), provider, tc.model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils_test.go` around lines 2190 - 2286, The test table only uses schemas.Anthropic so it doesn't verify provider gating; add at least one case that calls RemapRawToolVersionsForProvider with a non-Anthropic provider (e.g., schemas.Vertex or schemas.Bedrock) and a model string for that provider and an inputBody containing a tools array, and assert the output is unchanged (expected nil or the same tool types/names); place the new case inside the existing cases slice alongside the other entries so the loop will exercise provider-aware behavior and prevent accidental cross-provider remapping.
2289-2293: ⚡ Quick winTighten the “no tools array” assertion to catch silent shape regressions.
On Line 2291, the check currently passes if
toolsis present but empty (or present with wrong shape). For a true no-op expectation, assert absence oftoolsexplicitly.Diff suggestion
- if tc.expected == nil { - if toolsResult.Exists() && toolsResult.IsArray() && len(toolsResult.Array()) > 0 { - t.Fatalf("expected no tools array, got %s", toolsResult.Raw) - } - return - } + if tc.expected == nil { + if toolsResult.Exists() { + t.Fatalf("expected no tools field for no-op case, got %s", toolsResult.Raw) + } + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/utils_test.go` around lines 2289 - 2293, The test currently allows a present-but-empty or wrongly-shaped "tools" field to pass; change the assertion in the providerUtils.GetJSONField(out, "tools") branch so it explicitly fails if toolsResult.Exists() (i.e., require the field to be absent when tc.expected == nil). Update the check around toolsResult (used with providerUtils.GetJSONField, toolsResult.Exists(), toolsResult.IsArray(), toolsResult.Array()) to call t.Fatalf when toolsResult.Exists() is true, ensuring the test catches any silent presence or shape regressions for the "tools" field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/utils.go`:
- Line 1172: The test call to RemapRawToolVersionsForProvider now mismatches the
function signature: update the call in provider_feature_support_test (where it
currently passes []byte(tt.inputJSON), tt.provider) to include the model
argument as the third parameter, i.e. pass tt.model so the call becomes
RemapRawToolVersionsForProvider([]byte(tt.inputJSON), tt.provider, tt.model);
this ensures the test matches the function signature of
RemapRawToolVersionsForProvider.
In `@Makefile`:
- Around line 48-55: The Makefile block guarded by USE_INFISICAL_RESOLVED
currently sources the output of "infisical export" via process substitution
which can silently succeed with an empty environment if the export fails; change
it to run "infisical export --path \"$${INFISICAL_PATH_VAL}\" --format dotenv"
first, capture its exit status (or write its output to a temporary file), and if
the command fails print a clear error and exit 1; only when the export succeeds,
run "set -a; . <( ... )" or "set -a; . /tmp/<tempfile>" to source the variables
and then set +a and clean up the temp file; reference USE_INFISICAL_RESOLVED and
INFISICAL_PATH_VAL to locate the block to modify and ensure downstream targets
that rely on EXPOSE_ENV fail fast on export errors.
In `@tests/e2e/api/runners/analyze-failures.mjs`:
- Around line 555-570: renderMissingPerModel currently treats any feature with
cells[f].total === 0 as "missing" even when that feature is not applicable to
the model/provider; update the computation of tested and missing so they first
filter COVERAGE_FEATURES by applicability to the model (e.g., use an existing
feature-to-provider map or a per-cell flag like cells[f].applicable, or add a
helper isFeatureApplicableToModel(feature, key)), then compute tested =
applicableFeatures.filter(f => cells[f].total > 0) and missing =
applicableFeatures.filter(f => cells[f].total === 0); keep the rest of the
message logic but base counts and the shown/rest lists on those filtered arrays
so provider-inapplicable features are excluded from per-model gap counts.
In `@tests/integrations/python/config.json`:
- Around line 243-250: The config enables Bedrock batch usage
("use_for_batch_api": true) but the expected role ARN is missing; fix by either
setting "use_for_batch_api" to false for this provider or reintroducing the role
ARN fields the tests and loader expect (add a role_arn and/or batch_role_arn
entry under bedrock_key_config with the appropriate env reference so tests like
test_bedrock.py and utilities that read config["role_arn"] can find the batch
role).
---
Outside diff comments:
In `@Makefile`:
- Around line 386-392: The Makefile command invoking ./tmp/bifrost-http treats
the $(if $(APP_DIR),-app-dir "$(abspath $(APP_DIR))") token as a separate shell
command because the previous line (the $(if $(PROMETHEUS_LABELS)... line) is
missing a trailing backslash; fix by adding a trailing backslash to the end of
the $(if $(PROMETHEUS_LABELS),-prometheus-labels "$(PROMETHEUS_LABELS)") line so
the -app-dir argument is passed as part of the same command to the bifrost-http
invocation.
---
Nitpick comments:
In `@core/providers/anthropic/utils_test.go`:
- Around line 2190-2286: The test table only uses schemas.Anthropic so it
doesn't verify provider gating; add at least one case that calls
RemapRawToolVersionsForProvider with a non-Anthropic provider (e.g.,
schemas.Vertex or schemas.Bedrock) and a model string for that provider and an
inputBody containing a tools array, and assert the output is unchanged (expected
nil or the same tool types/names); place the new case inside the existing cases
slice alongside the other entries so the loop will exercise provider-aware
behavior and prevent accidental cross-provider remapping.
- Around line 2289-2293: The test currently allows a present-but-empty or
wrongly-shaped "tools" field to pass; change the assertion in the
providerUtils.GetJSONField(out, "tools") branch so it explicitly fails if
toolsResult.Exists() (i.e., require the field to be absent when tc.expected ==
nil). Update the check around toolsResult (used with providerUtils.GetJSONField,
toolsResult.Exists(), toolsResult.IsArray(), toolsResult.Array()) to call
t.Fatalf when toolsResult.Exists() is true, ensuring the test catches any silent
presence or shape regressions for the "tools" field.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46c03a79-f207-432a-b6d9-a87cdd1c4a6f
📒 Files selected for processing (18)
Makefilecore/providers/anthropic/chat.gocore/providers/anthropic/chatservertools_test.gocore/providers/anthropic/payloadordering_test.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/validatechattools_test.gocore/providers/vertex/vertex.godocs/docs.jsondocs/providers/test-harness-coverage.mdxtests/e2e/api/HARNESS_COVERAGE_BACKLOG.mdtests/e2e/api/collections/provider-harness.jsontests/e2e/api/runners/analyze-failures.mjstests/e2e/api/runners/filter-collection.mjstests/e2e/api/runners/harness-viewer.mjstests/integrations/python/config.json
✅ Files skipped from review due to trivial changes (1)
- docs/providers/test-harness-coverage.mdx
6d4e381 to
6d08e1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 1532-1534: The Makefile currently computes BASE_URL_VAL but does
not propagate the resolved host/port into the auto-start path, causing `make
dev` to start on the default HOST/PORT (8080) while callers may have set
BASE_URL to another host/port; update the logic around BASE_URL_VAL and the
auto-start invocation of `make dev` so you parse BASE_URL_VAL to extract host
and port (fallback to localhost:8080 when absent) and export those as HOST and
PORT (or pass HOST=$(HOST_VAL) PORT=$(PORT_VAL)) into the `make dev` command;
additionally, if BASE_URL_VAL is a non-local/remote URL, skip the auto-start and
fail fast with a clear message; apply the same change to the repeated block that
computes VIEWER_PORT_VAL (the other occurrence mentioned).
- Line 397: The Makefile currently breaks the ./tmp/bifrost-http command because
the previous line does not end with a line-continuation, so the $(if
$(APP_DIR),-app-dir "$(abspath $(APP_DIR))") argument is treated as a separate
shell command; fix it by adding a trailing backslash to the end of the preceding
Makefile command so that the APP_DIR conditional (the expression referencing
APP_DIR and abspath) is passed as an argument to ./tmp/bifrost-http.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7569032-8cc1-4485-9bd8-09d660bbd6f9
📒 Files selected for processing (19)
Makefilecore/internal/llmtests/provider_feature_support_test.gocore/providers/anthropic/chat.gocore/providers/anthropic/chatservertools_test.gocore/providers/anthropic/payloadordering_test.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/requestbuilder_test.gocore/providers/anthropic/utils.gocore/providers/anthropic/utils_test.gocore/providers/anthropic/validatechattools_test.gocore/providers/vertex/vertex.godocs/docs.jsondocs/providers/test-harness-coverage.mdxtests/e2e/api/HARNESS_COVERAGE_BACKLOG.mdtests/e2e/api/collections/provider-harness.jsontests/e2e/api/runners/analyze-failures.mjstests/e2e/api/runners/filter-collection.mjstests/e2e/api/runners/harness-viewer.mjstests/integrations/python/config.json
✅ Files skipped from review due to trivial changes (4)
- core/providers/vertex/vertex.go
- tests/e2e/api/runners/filter-collection.mjs
- docs/providers/test-harness-coverage.mdx
- core/providers/anthropic/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/providers/anthropic/chat.go
- core/providers/anthropic/utils.go
- tests/e2e/api/runners/harness-viewer.mjs
- tests/e2e/api/runners/analyze-failures.mjs
- docs/docs.json
- tests/integrations/python/config.json
6d08e1c to
5f06bd8
Compare
Merge activity
|

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 guidelines