Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughclassifyValidationFailure() now detects credential-related message patterns earlier and returns Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/validation.ts`:
- Around line 41-43: The credential-detection regex used on the normalized
string (the /.../.test(normalized) expression in validation.ts) misses messages
like "API key not valid"; update that regex to include a pattern matching "api
key not valid" (and a snake/underscore variant if you want consistency with the
existing api[_ ]key[_ ]invalid pattern) so such messages are caught by the
credential branch rather than falling through to the model 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72006602-5990-4589-a01e-9aaaee89a1a9
📒 Files selected for processing (2)
src/lib/validation.test.tssrc/lib/validation.ts
|
✨ Thanks for submitting this PR that proposes a fix for the issue where an expired API key is not properly classified as a credential error during the Gemini onboard process. The changes and testing you provided will help us review this further. Possibly related open PRs: Possibly related open issues: |
|
Extended the credential regex to |
|
@latenighthackathon if you can in the future, please mark your fork/branches so that we can merge commits in. Currently we cannot merge in the latest main into your branch as it's marked protected. Thanks! |
|
@brandonpelfrey thanks for the heads up — updated those permissions on the fork ruleset (flipped |
24ee88a to
4bc7dce
Compare
|
/ok to test 4bc7dce |
|
Heads up — the |
) When a Gemini onboard validates with an expired API key, the provider returns HTTP 400 with the message "API key expired. Please renew the API key.". The current classifier sees httpStatus === 400 first and returns { kind: "model", retry: "model" }, so the expired-key case never reaches the credential-message regex. In the Gemini validation flow which does not allowModelRetry, kind: "model" falls through to unknown, and the onboard wizard loops back to provider selection without offering to re-enter the key. Fix: check credential-bearing error messages BEFORE the HTTP 400 model default, and extend the credential regex to include 'api key expired' and API_KEY_INVALID forms so Gemini's API_KEY_INVALID status classification also lands as credential. Preserves existing behavior: HTTP 400 without a credential-bearing message still classifies as model (regression guard test added). Tests - src/lib/validation.test.ts: 41 tests pass, +3 for the new paths (expired key, API_KEY_INVALID, regression guard) - src/lib/validation-recovery.test.ts: 5 tests pass Prior fix attempt (#1944) was closed by its author on 2026-04-18 to reduce PR volume. This patch replays the same core reorder with a slightly broader credential regex and preserves the existing test style. Closes #1942 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Addresses CodeRabbit nit on #2132: > Credential regex misses "API key not valid" wording. Line 41 patterns > won't match messages like "API key not valid. Please pass a valid API > key." unless API_KEY_INVALID is also present. When NemoClaw extracts only the Gemini error .message field (without the API_KEY_INVALID status prefix), the bare wording slipped through to the httpStatus === 400 -> model default. Extend the credential regex to also match "api key not valid": /api key (expired|not valid)|api[_ ]key[_ ]invalid|.../ Tests - src/lib/validation.test.ts: 42 tests pass (+1 for the bare wording) Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
4bc7dce to
b3d7fa4
Compare
|
@iamsh4 @jyaunches thanks — rebased onto current main so |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Clean reorder — credential message check before the 400→model default. Regression guard test for non-credential 400s is the right call. LGTM.
wscurran
added
area: providers
7 participants
Summary
When a Gemini onboard validates with an expired API key, the provider returns HTTP 400 with
"API key expired. Please renew the API key.". The current classifier checkshttpStatus === 400first and returns{ kind: "model", retry: "model" }, so the expired-key case never reaches the credential-message regex. In the Gemini validation flow (noallowModelRetry),kind: "model"falls through tounknown, and the onboard wizard loops back to provider selection without offering to re-enter the key.Related Issue
Closes #1942
Changes
src/lib/validation.ts— move the credential-bearing message check before thehttpStatus === 400 → modeldefault. Extend the credential regex to includeapi key expiredandapi[_ ]key[_ ]invalidso Gemini'sAPI_KEY_INVALIDstatus classification also lands ascredential.model— regression guard test added.Testing
src/lib/validation.test.ts— 41 tests pass (+3 new: expired key, API_KEY_INVALID, regression guard)src/lib/validation-recovery.test.ts— 5 tests passExecuted:
npx vitest run src/lib/validation.test.ts src/lib/validation-recovery.test.tsin thenemoclaw-testDocker environmentPrior art
Prior fix attempt #1944 was closed by its author on 2026-04-18 to reduce PR volume — the issue remained open. This PR replays the same core reorder with a slightly broader credential regex (covering both
API key expiredandAPI_KEY_INVALIDforms Gemini uses) and preserves the existing test style.Checklist
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests