-
Notifications
You must be signed in to change notification settings - Fork 460
fix: detect Claude Code 401 auth errors in claude_harness #45982
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.
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
Changes from all commits
a40fcca
eddfe99
bdba5ac
8945696
502ad71
3e3df49
c578de5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,6 +190,23 @@ describe("claude_harness.cjs", () => { | |
| expect(isAuthenticationFailedError("Authentication failed (Request ID: C818:3ED713:19D401B:1C446B7:69D653CA)")).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for Claude Code stream-JSON "error":"authentication_failed" field', () => { | ||
| const jsonLine = JSON.stringify({ | ||
| type: "assistant", | ||
| error: "authentication_failed", | ||
| message: { content: [{ type: "text", text: "Not logged in · Please run /login" }] }, | ||
|
|
||
| }); | ||
| expect(isAuthenticationFailedError(jsonLine)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for Claude Code "Not logged in" message', () => { | ||
| expect(isAuthenticationFailedError("Not logged in · Please run /login")).toBe(true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test on line 203 exercises a JSON-serialised line containing 💡 Suggested negative testit("does not confuse a different JSON error field with auth failure", () => {
const jsonLine = JSON.stringify({ type: "error", error: "rate_limit_error" });
expect(isAuthenticationFailedError(jsonLine)).toBe(false);
});This guards against regex drift that might accidentally widen the @copilot please address this. |
||
| }); | ||
|
|
||
| it('returns true for "not logged in" (case-insensitive)', () => { | ||
| expect(isAuthenticationFailedError("NOT LOGGED IN")).toBe(true); | ||
| }); | ||
|
|
||
| describe("isInvalidModelError", () => { | ||
| it("returns true for model-not-supported errors", () => { | ||
| expect(isInvalidModelError("Execution failed: CAPIError: 400 The requested model is not supported.")).toBe(true); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test does not isolate the
"error"field branch: the JSON payload also embeds"Not logged in"in thetextfield, so this test passes even if the"error":"authentication_failed"regex arm were deleted.💡 Suggested fix
Use a neutral string for the
textfield so only theerrorkey triggers the match:Without this fix there is zero actual coverage for the
"error"\s*:\s*"authentication_failed"branch — it can be deleted and all tests still pass.