fix(release): adapt gpt-5 chat params and batch aliases#199
Merged
SantiagoDePolonia merged 5 commits intomainfrom Apr 1, 2026
Merged
fix(release): adapt gpt-5 chat params and batch aliases#199SantiagoDePolonia merged 5 commits intomainfrom
SantiagoDePolonia merged 5 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 52 seconds. ⌛ 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds documentation for exact-match response caching (new feature page, nav entry, quickstart link), tweaks alias resolution to normalize and respect explicit providers, extends OpenAI provider handling to detect GPT‑5 and unify reasoning-chat parameter adaptation, and adds related tests and expanded E2E scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Comment on lines
-125
to
-130
| if !requested.ExplicitProvider { | ||
| if resolution, ok := s.resolveAlias(requested.Model); ok { | ||
| return resolution, true, nil | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
@coderabbitai what are the consequences of removing it?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and display the Normalize method on RequestedModelSelector
ast-grep --pattern 'func ($_ RequestedModelSelector) Normalize() $$$'
# Find normalizeName function in the aliases package
ast-grep --pattern 'func normalizeName($_) string {
$$$
}'
# Also look at how normalizeAlias is implemented
ast-grep --pattern 'func normalizeAlias($_) $$$'
Length of output: 1538
🏁 Script executed:
#!/bin/bash
# Find ParseModelSelector implementation
ast-grep --pattern 'func ParseModelSelector($$$) $$$'
Length of output: 1515
@SantiagoDePolonia Here's a breakdown of the consequences of removing that block:
What was removed
// OLD — ran BEFORE Normalize()
if !requested.ExplicitProvider {
if resolution, ok := s.resolveAlias(requested.Model); ok {
return resolution, true, nil // early return, skipped normalization entirely
}
}Consequences
1. Redundant double alias lookup eliminated (main effect)
In the old code, for a non-explicit-provider request that does not match an alias, resolveAlias was called twice: once in the removed early block, and again at the bottom of the function (which still exists). Removing the early block means the lookup happens exactly once. This is purely a correctness / clarity improvement.
2. Normalization now always runs before alias resolution
In the old code, if an alias was found early, Normalize() was never called. In the new code, Normalize() (which calls ParseModelSelector) runs first. Theoretically this could surface errors that were previously swallowed, but in practice it's a no-op: the only error ParseModelSelector can return is "model is empty", and resolveAlias already guards against that (via normalizeName → strings.TrimSpace, returning false for empty names). So there's no reachable input where the old code succeeded but the new code returns an error.
3. Resolution.Requested field is unaffected
Neither old nor new code overrides resolution.Requested in the non-explicit-provider path (unlike the ExplicitProvider path which does resolution.Requested = selector). Both paths rely on resolveAlias to populate it as core.ModelSelector{Model: normalizedName}, so the returned value is identical.
4. Provider prefix stripping is unaffected
ParseModelSelector auto-extracts a provider prefix (e.g., "openai/gpt-4" → model="gpt-4", provider="openai"), but resolveAlias is still called with requested.Model (the original string) in both old and new code — not with selector.Model — so alias lookup behavior is unchanged.
Summary
The removal is functionally safe. The only observable change is eliminating the redundant duplicate alias lookup for non-explicit-provider requests, making the code path consistent and easier to follow.
🧠 Learnings used
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/auditlog_test.go:272-279
Timestamp: 2026-01-15T23:07:26.646Z
Learning: Go code guideline: Avoid using interface{} and map[string]interface{} for API request/response payload types. This should be restricted to public API surfaces and data payloads. It is acceptable to use interface{} in internal infrastructure packages (e.g., storage abstractions) only to break import cycles, and only when type assertions occur at initialization (not on every request) to minimize runtime type checks and improve readability. Apply this rule broadly to files under internal/ that deal with payload-like types, but not to internal components that are pure infrastructure or have runtime-per-request type assertions.
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/aliases/service_test.go`:
- Around line 238-265: The test
TestServiceResolveAliasWithExplicitProviderFallbackWhenConcreteSelectorMissing
assumes ResolveModel will silently override an explicit provider; instead,
change the assertions so the resolver leaves an explicit provider selector
unchanged: call
service.ResolveModel(core.NewRequestedModelSelector("smart","openai")) and
assert that changed is false and that selector.QualifiedModel() still reflects
the original explicit provider/name (e.g., "openai/smart"), rather than
expecting "anthropic/claude-3-7-sonnet". Update expectations in the test (not
resolver code) and keep NewService, Refresh, and ResolveModel calls as-is.
In `@internal/aliases/service.go`:
- Around line 130-137: When requested.ExplicitProvider is true and
s.catalog.Supports(selector.QualifiedModel()) is false, do NOT call
s.resolveAlias(requested.Model); instead keep and return the normalized selector
unchanged so higher validation can reject provider/model mismatches. Update the
explicit-provider branch in the ResolveModel path: remove the
s.resolveAlias(requested.Model) fallback and return Resolution{Requested:
selector, Resolved: selector}, false, nil (preserving selector and not resolving
aliases) so explicit provider requests are honored.
In `@internal/providers/openai/openai_test.go`:
- Around line 1835-1894: The test
TestStreamChatCompletion_GPT5Model_AdaptsParameters is missing an assertion that
streaming GPT‑5 requests do not include a temperature field; update the server
handler in that test (the httptest http.HandlerFunc that inspects the parsed
request `raw`) to assert that `temperature` is not present (similar to the
existing `max_tokens` check) so the test fails if provider.StreamChatCompletion
or the core.ChatRequest accidentally forwards Temperature for streaming gpt‑5
models.
In `@tests/e2e/release-e2e-scenarios.md`:
- Line 674: The markdown headings (e.g., "S63 Auth-enabled dashboard runtime
config" and the other flagged headings listed) are missing a required blank line
underneath which triggers markdownlint MD022; to fix, insert a single blank line
immediately after each heading line so the heading is separated from the
following content, and apply this same blank-line pattern to all other flagged
headings (lines referenced in the comment) throughout the document to satisfy
MD022.
- Around line 693-695: The jq command is currently outputting the secret field
`value`, risking secret leakage; update the pipeline that prints
`"$AUTH_KEY_JSON"` so the jq filter omits `value` and only emits non-secret
fields (e.g., `id`, `name`, `user_path`, `active`, `redacted_value`), ensuring
any logging or CI output never includes the raw managed API key value.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9ce04750-9aff-46b6-b7d2-157d8bdc2316
📒 Files selected for processing (11)
docs/advanced/configuration.mdxdocs/docs.jsondocs/features/cache.mdxdocs/getting-started/quickstart.mdxinternal/aliases/batch_preparer_test.gointernal/aliases/service.gointernal/aliases/service_test.gointernal/providers/openai/openai.gointernal/providers/openai/openai_test.gointernal/server/handlers_test.gotests/e2e/release-e2e-scenarios.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/providers/openai/openai_test.go (1)
1878-1882:⚠️ Potential issue | 🟡 MinorRequest is missing
Temperaturefield, making the temperature assertion ineffective.The server handler correctly asserts that
temperatureshould not be present (lines 1852-1854), but the request never setsTemperature. This means the assertion is dead code that cannot catch a regression where streaming GPT-5 requests accidentally forward temperature.Compare with
TestChatCompletion_GPT5Model_AdaptsParameters(lines 1608-1613) which correctly sets both fields.🔧 Proposed fix
func TestStreamChatCompletion_GPT5Model_AdaptsParameters(t *testing.T) { maxTokens := 2000 + temp := 0.7 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ... req := &core.ChatRequest{ - Model: "gpt-5-nano", - Messages: []core.Message{{Role: "user", Content: "Hello"}}, - MaxTokens: &maxTokens, + Model: "gpt-5-nano", + Messages: []core.Message{{Role: "user", Content: "Hello"}}, + MaxTokens: &maxTokens, + Temperature: &temp, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/openai/openai_test.go` around lines 1878 - 1882, The test builds a core.ChatRequest (req) without setting the Temperature field so the server-side assertion that temperature should be absent is never exercised; update the test that constructs req in internal/providers/openai/openai_test.go to set the Temperature pointer (e.g., a local temperature variable and assign &temperature to req.Temperature) just like in TestChatCompletion_GPT5Model_AdaptsParameters so the handler's temperature assertion becomes effective and will catch regressions where GPT-5 streaming requests forward temperature.
🤖 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/release-e2e-scenarios.md`:
- Around line 755-756: The script writes sensitive managed API key files
($AUTH_KEY_JSON -> $QA_AUTH_KEY_JSON and extracted value ->
$QA_AUTH_KEY_VALUE_FILE) to /tmp without restricting permissions or guaranteed
cleanup; change the write flow to create the files with secure permissions
(e.g., use a temp file or set umask/chmod to owner-only such as 600 immediately
after creation) and ensure both $QA_AUTH_KEY_JSON and $QA_AUTH_KEY_VALUE_FILE
are removed in the cleanup path; update any teardown/trap logic to always unlink
these two variables to avoid leaving secrets on disk.
- Around line 35-37: Add fail-fast shell guards before executing the
auth-enabled block: enable strict mode (set -euo pipefail) and verify sourcing
.env succeeded (or that required env vars are set) so a failed `source .env` or
subsequent API call cannot silently cascade; update the snippet containing `set
-a` / `source .env` / `set +a` and replicate the same guards for the other
blocks referenced (the blocks around lines 751-783 and 917-945) to ensure early
exit on errors and missing required variables.
- Around line 734-946: The PR adds new auth-enabled runtime and cache behaviors
(managed API keys, scoped execution plans, cache analytics, and new audit
fields/headers) but CLAUDE.md was not updated; update CLAUDE.md to document the
new admin endpoints and payloads (POST /admin/api/v1/auth-keys, POST
/admin/api/v1/execution-plans, deactivate endpoints), the managed API key
lifecycle and one-time secret handling, the scoped workflow semantics
(scope_user_path, effective_features including cache:false), the
X-GoModel-User-Path header handling and precedence when using managed keys,
expected audit log fields (auth_method, auth_key_id, user_path,
execution_plan_version_id, cache_type), and the cache analytics/usage endpoints
and query params shown in the tests so readers can reproduce S63–S79 flows.
---
Duplicate comments:
In `@internal/providers/openai/openai_test.go`:
- Around line 1878-1882: The test builds a core.ChatRequest (req) without
setting the Temperature field so the server-side assertion that temperature
should be absent is never exercised; update the test that constructs req in
internal/providers/openai/openai_test.go to set the Temperature pointer (e.g., a
local temperature variable and assign &temperature to req.Temperature) just like
in TestChatCompletion_GPT5Model_AdaptsParameters so the handler's temperature
assertion becomes effective and will catch regressions where GPT-5 streaming
requests forward temperature.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c1e60900-3f2d-4472-a122-c80af70cb3ef
📒 Files selected for processing (6)
internal/aliases/batch_preparer_test.gointernal/aliases/service.gointernal/aliases/service_test.gointernal/providers/openai/openai_test.gointernal/server/handlers_test.gotests/e2e/release-e2e-scenarios.md
Comment on lines
+734
to
+946
| ## 12. Authenticated runtime features | ||
|
|
||
| ### S63 Auth-enabled dashboard runtime config | ||
|
|
||
| Reads the allowlisted runtime flags for the live auth-enabled gateway. | ||
|
|
||
| ```bash | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/dashboard/config" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq '.' | ||
| ``` | ||
|
|
||
| ### S64 Create managed API key | ||
|
|
||
| Creates one managed API key scoped to a release-specific user path and stores the one-time secret under `/tmp`. | ||
|
|
||
| ```bash | ||
| AUTH_KEY_JSON=$(curl -sS -X POST "$AUTH_BASE_URL/admin/api/v1/auth-keys" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"name\":\"$QA_AUTH_KEY_NAME\",\"description\":\"Release e2e managed key\",\"user_path\":\"$QA_USER_PATH\"}") | ||
| printf '%s\n' "$AUTH_KEY_JSON" > "$QA_AUTH_KEY_JSON" | ||
| printf '%s\n' "$AUTH_KEY_JSON" | jq -r '.value' > "$QA_AUTH_KEY_VALUE_FILE" | ||
| printf '%s\n' "$AUTH_KEY_JSON" \ | ||
| | jq '{id,name,user_path,active,redacted_value}' | ||
| ``` | ||
|
|
||
| ### S65 Verify managed API key list | ||
|
|
||
| Checks that the newly issued managed API key is visible and active. | ||
|
|
||
| ```bash | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/auth-keys" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq ".[] | select(.name==\"$QA_AUTH_KEY_NAME\") | {id,name,user_path,active,expires_at,redacted_value}" | ||
| ``` | ||
|
|
||
| ### S66 Create user-path-scoped workflow with cache disabled | ||
|
|
||
| Creates a scoped workflow for `openai/gpt-4.1-nano` that disables cache for the managed-key user path. | ||
|
|
||
| ```bash | ||
| WORKFLOW_JSON=$(curl -sS -X POST "$AUTH_BASE_URL/admin/api/v1/execution-plans" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"scope_provider\":\"openai\",\"scope_model\":\"gpt-4.1-nano\",\"scope_user_path\":\"$QA_USER_PATH\",\"name\":\"$QA_WORKFLOW_NAME\",\"description\":\"Disable cache for managed-key release e2e scope\",\"plan_payload\":{\"schema_version\":1,\"features\":{\"cache\":false,\"audit\":true,\"usage\":true,\"guardrails\":false,\"fallback\":false},\"guardrails\":[]}}") | ||
| printf '%s\n' "$WORKFLOW_JSON" > "$QA_WORKFLOW_JSON" | ||
| printf '%s\n' "$WORKFLOW_JSON" | jq -r '.id' > "$QA_WORKFLOW_ID_FILE" | ||
| printf '%s\n' "$WORKFLOW_JSON" \ | ||
| | jq '{id,name,scope,plan_payload}' | ||
| ``` | ||
|
|
||
| ### S67 Verify scoped workflow detail | ||
|
|
||
| Reads the created workflow back and confirms the normalized scope and effective feature projection. | ||
|
|
||
| ```bash | ||
| WORKFLOW_ID=$(cat "$QA_WORKFLOW_ID_FILE") | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/execution-plans/$WORKFLOW_ID" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq '{id,name,scope,plan_payload,effective_features}' | ||
| ``` | ||
|
|
||
| ### S68 Managed-key request through scoped workflow | ||
|
|
||
| Sends a request with the managed API key while also sending a conflicting `X-GoModel-User-Path` header. | ||
|
|
||
| ```bash | ||
| API_KEY=$(cat "$QA_AUTH_KEY_VALUE_FILE") | ||
| curl -sS -D - "$AUTH_BASE_URL/v1/chat/completions" \ | ||
| -H "Authorization: Bearer $API_KEY" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -H "X-Request-ID: $QA_AUTH_REQ1" \ | ||
| -H 'X-GoModel-User-Path: /team/should-be-overridden' \ | ||
| -d '{"model":"openai/gpt-4.1-nano","messages":[{"role":"user","content":"Reply with exactly QA_AUTH_CACHE_OFF_OK"}],"max_tokens":16}' \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S69 Repeated managed-key request should still bypass cache | ||
|
|
||
| Repeats the same request and expects another live provider response rather than `X-Cache: HIT`. | ||
|
|
||
| ```bash | ||
| API_KEY=$(cat "$QA_AUTH_KEY_VALUE_FILE") | ||
| curl -sS -D - "$AUTH_BASE_URL/v1/chat/completions" \ | ||
| -H "Authorization: Bearer $API_KEY" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -H "X-Request-ID: $QA_AUTH_REQ2" \ | ||
| -H 'X-GoModel-User-Path: /team/should-be-overridden' \ | ||
| -d '{"model":"openai/gpt-4.1-nano","messages":[{"role":"user","content":"Reply with exactly QA_AUTH_CACHE_OFF_OK"}],"max_tokens":16}' \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S70 Audit evidence for managed-key scoped workflow | ||
|
|
||
| Confirms that auth method, managed auth key ID, normalized user path, workflow ID, and no cache hit are all recorded together. | ||
|
|
||
| ```bash | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/audit/log?request_id=$QA_AUTH_REQ2&limit=5" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq '{total,entries:(.entries|map({request_id,status_code,auth_method,auth_key_id,user_path,execution_plan_version_id,cache_type,answer:.data.response_body.choices[0].message.content}))}' | ||
| ``` | ||
|
|
||
| ### S71 Global cache warm request with explicit user path | ||
|
|
||
| Warms the global cache-enabled workflow using the master key and a cache-specific user path. | ||
|
|
||
| ```bash | ||
| curl -sS -D - "$AUTH_BASE_URL/v1/chat/completions" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -H "X-Request-ID: $QA_CACHE_REQ1" \ | ||
| -H "X-GoModel-User-Path: $QA_CACHE_USER_PATH" \ | ||
| -d "{\"model\":\"openai/gpt-4.1-nano\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly $QA_CACHE_REPLY\"}],\"max_tokens\":16}" \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S72 Repeated global cache request should hit exact cache | ||
|
|
||
| Repeats the same request and expects `X-Cache: HIT (exact)`. | ||
|
|
||
| ```bash | ||
| curl -sS -D - "$AUTH_BASE_URL/v1/chat/completions" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -H "X-Request-ID: $QA_CACHE_REQ2" \ | ||
| -H "X-GoModel-User-Path: $QA_CACHE_USER_PATH" \ | ||
| -d "{\"model\":\"openai/gpt-4.1-nano\",\"messages\":[{\"role\":\"user\",\"content\":\"Reply with exactly $QA_CACHE_REPLY\"}],\"max_tokens\":16}" \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S73 Cache overview filtered by user path | ||
|
|
||
| Checks cache analytics after the exact-cache hit using the same tracked user path. | ||
|
|
||
| ```bash | ||
| sleep 6 | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/cache/overview?days=1&user_path=$QA_CACHE_USER_PATH" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq '.' | ||
| ``` | ||
|
|
||
| ### S74 Cached usage log filtered by user path | ||
|
|
||
| Reads cached-only usage entries for the same exact-hit request path. | ||
|
|
||
| ```bash | ||
| curl -sS "$AUTH_BASE_URL/admin/api/v1/usage/log?days=1&user_path=$QA_CACHE_USER_PATH&cache_mode=cached&limit=5" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq '{total,entries:(.entries|map({request_id,cache_type,model,provider,endpoint,user_path,total_tokens}))}' | ||
| ``` | ||
|
|
||
| ### S75 Invalid managed API key user path (negative) | ||
|
|
||
| Verifies user-path validation for managed API key creation. | ||
|
|
||
| ```bash | ||
| curl -sS -i -X POST "$AUTH_BASE_URL/admin/api/v1/auth-keys" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d '{"name":"qa-invalid-user-path","user_path":"/team/../alpha"}' \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S76 Invalid workflow scope user path (negative) | ||
|
|
||
| Verifies user-path validation for workflow creation. | ||
|
|
||
| ```bash | ||
| curl -sS -i -X POST "$AUTH_BASE_URL/admin/api/v1/execution-plans" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d '{"scope_provider":"openai","scope_model":"gpt-4.1-nano","scope_user_path":"/team/../alpha","name":"qa-invalid-workflow-path","plan_payload":{"schema_version":1,"features":{"cache":true,"audit":true,"usage":true,"guardrails":false},"guardrails":[]}}' \ | ||
| | sed -n '1,24p' | ||
| ``` | ||
|
|
||
| ## 13. Authenticated cleanup | ||
|
|
||
| ### S77 Deactivate managed API key | ||
|
|
||
| Deactivates the managed key created for the auth-enabled release run. | ||
|
|
||
| ```bash | ||
| AUTH_KEY_ID=$(curl -sS "$AUTH_BASE_URL/admin/api/v1/auth-keys" \ | ||
| -H "$ADMIN_AUTH_HEADER" \ | ||
| | jq -r ".[] | select(.name==\"$QA_AUTH_KEY_NAME\") | .id") | ||
| curl -sS -i -X POST "$AUTH_BASE_URL/admin/api/v1/auth-keys/$AUTH_KEY_ID/deactivate" \ | ||
| -H "$ADMIN_AUTH_HEADER" | ||
| ``` | ||
|
|
||
| ### S78 Deactivated managed API key is rejected | ||
|
|
||
| Confirms that the same managed key can no longer authenticate requests. | ||
|
|
||
| ```bash | ||
| API_KEY=$(cat "$QA_AUTH_KEY_VALUE_FILE") | ||
| curl -sS -i "$AUTH_BASE_URL/v1/chat/completions" \ | ||
| -H "Authorization: Bearer $API_KEY" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -H "X-Request-ID: $QA_DEACTIVATED_REQ" \ | ||
| -d '{"model":"openai/gpt-4.1-nano","messages":[{"role":"user","content":"Reply with exactly QA_AUTH_DEACTIVATED"}],"max_tokens":16}' \ | ||
| | sed -n '1,20p' | ||
| ``` | ||
|
|
||
| ### S79 Deactivate scoped workflow | ||
|
|
||
| Deactivates the workflow created for the scoped managed-key release run. | ||
|
|
||
| ```bash | ||
| WORKFLOW_ID=$(cat "$QA_WORKFLOW_ID_FILE") | ||
| curl -sS -i -X POST "$AUTH_BASE_URL/admin/api/v1/execution-plans/$WORKFLOW_ID/deactivate" \ | ||
| -H "$ADMIN_AUTH_HEADER" | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify CLAUDE.md was updated for these new auth/cache/runtime behaviors.
This section adds/changes documented release validation behavior around auth-enabled runtime config, managed keys, scoped workflows, and cache analytics. Please confirm corresponding CLAUDE.md updates are included in this PR (or a linked follow-up).
Based on learnings: Update CLAUDE.md documentation whenever behavior, configuration, providers, supported commands, or public API contracts change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/release-e2e-scenarios.md` around lines 734 - 946, The PR adds new
auth-enabled runtime and cache behaviors (managed API keys, scoped execution
plans, cache analytics, and new audit fields/headers) but CLAUDE.md was not
updated; update CLAUDE.md to document the new admin endpoints and payloads (POST
/admin/api/v1/auth-keys, POST /admin/api/v1/execution-plans, deactivate
endpoints), the managed API key lifecycle and one-time secret handling, the
scoped workflow semantics (scope_user_path, effective_features including
cache:false), the X-GoModel-User-Path header handling and precedence when using
managed keys, expected audit log fields (auth_method, auth_key_id, user_path,
execution_plan_version_id, cache_type), and the cache analytics/usage endpoints
and query params shown in the tests so readers can reproduce S63–S79 flows.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/release-e2e-scenarios.md (1)
35-37:⚠️ Potential issue | 🟠 MajorAdd strict shell guards before running auth-enabled scenarios.
Without fail-fast mode, a bad
.envload or failed command can silently cascade through later scenarios.Suggested fix
set -a source .env set +a +set -euo pipefail +: "${GOMODEL_MASTER_KEY:?GOMODEL_MASTER_KEY is required}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/release-e2e-scenarios.md` around lines 35 - 37, Enable strict shell failure handling before sourcing the environment and ensure .env is present: add fail-fast and strict-unset/pipefail options before the existing set -a / source .env / set +a sequence, and add an explicit existence/readability check for ".env" that prints a clear error and exits nonzero if the file is missing or unreadable; keep the existing "set -a", "source .env", and "set +a" semantics but surround them with the strict options and the pre-check.
🤖 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/release-e2e-scenarios.md`:
- Around line 60-62: The cleanup_release_auth_artifacts function currently
removes only $QA_AUTH_KEY_JSON and $QA_AUTH_KEY_VALUE_FILE; update it to also
remove the workflow temp files by adding $QA_WORKFLOW_JSON and
$QA_WORKFLOW_ID_FILE to the rm -f list so those /tmp files are not left stale
(also apply the same addition wherever similar cleanup logic appears, e.g., the
other occurrences noted around the same tests).
- Around line 758-766: The script writes AUTH_KEY_JSON and then unconditionally
extracts .value into QA_AUTH_KEY_VALUE_FILE which can become "null" if creation
failed; update the logic around AUTH_KEY_JSON / QA_AUTH_KEY_VALUE_FILE / jq
usage to validate the response before persisting the key value: after obtaining
AUTH_KEY_JSON, verify the JSON indicates success (e.g., .value exists and is not
null/empty or check HTTP success) and only then write AUTH_KEY_JSON to
QA_AUTH_KEY_JSON and the extracted .value to QA_AUTH_KEY_VALUE_FILE; if
validation fails, emit an error and abort so downstream checks aren’t misled.
---
Duplicate comments:
In `@tests/e2e/release-e2e-scenarios.md`:
- Around line 35-37: Enable strict shell failure handling before sourcing the
environment and ensure .env is present: add fail-fast and strict-unset/pipefail
options before the existing set -a / source .env / set +a sequence, and add an
explicit existence/readability check for ".env" that prints a clear error and
exits nonzero if the file is missing or unreadable; keep the existing "set -a",
"source .env", and "set +a" semantics but surround them with the strict options
and the pre-check.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3524a43c-ebf8-42a2-8eb6-3ff80e9adfa0
📒 Files selected for processing (1)
tests/e2e/release-e2e-scenarios.md
Contributor
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 `@tests/e2e/release-e2e-scenarios.md`:
- Around line 941-945: The script currently may call the deactivate endpoint
with an empty AUTH_KEY_ID; update the command that extracts AUTH_KEY_ID (the jq
invocation used to populate AUTH_KEY_ID) to use strict mode (jq -er) so it fails
when no match is found, then add an explicit guard immediately after setting
AUTH_KEY_ID that checks if the variable is empty (e.g., test -z "$AUTH_KEY_ID")
and, if so, emit a clear error message and exit non-zero before reaching the
curl POST to /admin/api/v1/auth-keys/$AUTH_KEY_ID/deactivate; reference
AUTH_KEY_ID and the deactivate curl call when locating where to add the jq flag
and the early-fail check.
- Around line 800-806: The script writes WORKFLOW_JSON to QA_WORKFLOW_JSON and
then extracts the ID into QA_WORKFLOW_ID_FILE without validating the response;
update the sequence around WORKFLOW_JSON / jq -r '.id' so you parse and validate
the id (from WORKFLOW_JSON), ensure it is non-empty and not "null" (and
optionally check the HTTP response/status), and if validation fails emit a clear
error to stderr and exit non-zero instead of writing a bogus ID; locate and
change the block that sets WORKFLOW_JSON, the jq extraction to
QA_WORKFLOW_ID_FILE, and the follow-up logic that uses QA_WORKFLOW_ID_FILE to
implement this guard and fail-fast behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: de14fdf9-ded5-438c-9753-69c4e796e1c9
📒 Files selected for processing (1)
tests/e2e/release-e2e-scenarios.md
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
1 participant
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 by CodeRabbit
New Features
Documentation
Bug Fixes
Tests