fix: detect Claude Code 401 auth errors in claude_harness#45982
Conversation
…th errors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…tication_failed) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…group Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=0). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Extends Claude harness authentication detection to stop retries on Claude Code 401 failures.
Changes:
- Detects proxy and missing-login authentication signals.
- Adds regression tests for the new patterns.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/claude_harness.cjs |
Expands authentication-error matching. |
actions/setup/js/claude_harness.test.cjs |
Tests new authentication signals. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| const jsonLine = JSON.stringify({ | ||
| type: "assistant", | ||
| error: "authentication_failed", | ||
| message: { content: [{ type: "text", text: "Not logged in · Please run /login" }] }, |
There was a problem hiding this comment.
The regex extension and tests look correct. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 7.72 AIC · ⌖ 6.66 AIC · ⊞ 5K
|
@copilot generalize this detection to common harness code |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 26.9 AIC · ⌖ 4.46 AIC · ⊞ 6.7K
Comment /matt to run again
| const RATE_LIMIT_ERROR_PATTERN = /rate_limit_error|429 Too Many Requests|"api_error_status"\s*:\s*429|request rejected \(429\)|rate limit/i; | ||
| const AUTHENTICATION_FAILED_PATTERN = /Authentication failed(?:\s*\(Request ID:[^)]+\))?/i; | ||
| // Matches: | ||
| // - "Authentication failed (Request ID: ...)" — Anthropic-direct auth error |
There was a problem hiding this comment.
[/diagnosing-bugs] The not logged in branch is quite generic — it could produce false positives if any tool, test, or user script that Claude Code runs emits this phrase as part of its own output (e.g. a git command, a test assertion, or a CLI tool).
💡 Suggestion: tighten the match or add a negative test
Consider anchoring it more tightly, e.g.:
/Not logged in(?:\s*[·•]\s*Please run)?/iThis still matches the known Claude Code message ("Not logged in · Please run /login") while avoiding accidental matches on arbitrary application output that happens to contain the phrase.
If you intentionally want the broad match, add an explicit negative test that confirms a message like "User is not logged in to the database" does not trigger auth abort — so any future accidental broadening is caught.
@copilot please address this.
| }); | ||
|
|
||
| it('returns true for Claude Code "Not logged in" message', () => { | ||
| expect(isAuthenticationFailedError("Not logged in · Please run /login")).toBe(true); |
There was a problem hiding this comment.
[/tdd] The test on line 203 exercises a JSON-serialised line containing "error":"authentication_failed" — great. However, it does not test the negative/boundary case: a JSON line where "error" has a different value (e.g. "rate_limit_error") should not match this pattern.
💡 Suggested negative test
it("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 authentication_failed branch to other JSON error values.
@copilot please address this.
There was a problem hiding this comment.
Two issues block merge
The fix correctly targets a real problem (missed auth failure signals burning retry budget), but introduces a false-positive risk and ships a test that doesn't actually cover what it claims.
Blocking findings
1. pattern is unanchored (high)
The regex matches any substring anywhere in the full accumulated stdout+stderr. Task content being processed (web pages, files, commit messages) could contain this phrase and trigger a non-retryable abort on attempt 0. The "error":"authentication_failed" arm is much safer because it is a JSON key-value match. The not logged in arm needs anchoring (start-of-line, word boundaries, or requiring the Claude Code /login suffix).
2. JSON test provides no coverage of the "error" arm (high)
The test embeds "Not logged in" inside the JSON text field, so the not logged in branch alone makes it pass. Deleting the "error":"authentication_failed" arm entirely leaves all tests green. The test must use a neutral text value to isolate the branch it claims to test.
🔎 Code quality review by PR Code Quality Reviewer · 37.4 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
| // - "Authentication failed (Request ID: ...)" — Anthropic-direct auth error | ||
| // - `"error":"authentication_failed"` — Claude Code stream-JSON error field (e.g. 401 via AWF proxy) | ||
| // - "Not logged in" — Claude Code message when no credentials are available | ||
| const AUTHENTICATION_FAILED_PATTERN = /(?:Authentication failed(?:\s*\(Request ID:[^)]+\))?|"error"\s*:\s*"authentication_failed"|not logged in)/i; |
There was a problem hiding this comment.
False-positive risk: not logged in matches any substring in accumulated stdout/stderr and will abort retries on attempt 0 if task output happens to contain that phrase.
💡 Details and suggested fix
The pattern /not logged in/i has no anchoring, word boundaries, or structural requirements. The harness collects all stdout+stderr from the subprocess — including tool responses, task descriptions, and user-provided content being processed. If any line in that output contains "not logged in" (e.g., a web-scraping result, a git commit message, a markdown file being read), isAuthenticationFailedError returns true and the harness hits the early-abort path at attempt 0 (non-retryable), silently killing the job.
Since this signal originates from Claude Code itself, consider anchoring it more tightly — e.g., requiring it to appear as a standalone line or within a JSON event field:
// Option A: require it to appear at the start of a line (common for Claude Code terminal messages)
/(?:Authentication failed(?:\s*\(Request ID:[^)]+\))?|"error"\s*:\s*"authentication_failed"|^not logged in\b)/im
// Option B: require word boundaries and limit to short strings
/(?:Authentication failed(?:\s*\(Request ID:[^)]+\))?|"error"\s*:\s*"authentication_failed"|(?<![a-z])not logged in(?![a-z])\s*[·—·]?\s*(?:please run \/login)?)/iAt minimum add a test that confirms output containing "not logged in" buried inside unrelated content (e.g., "Error: user not logged in to external service") does not trigger the pattern unintentionally — or acknowledge the risk in a comment.
| it('returns true for Claude Code stream-JSON "error":"authentication_failed" field', () => { | ||
| const jsonLine = JSON.stringify({ | ||
| type: "assistant", | ||
| error: "authentication_failed", |
There was a problem hiding this comment.
Test does not isolate the "error" field branch: the JSON payload also embeds "Not logged in" in the text field, so this test passes even if the "error":"authentication_failed" regex arm were deleted.
💡 Suggested fix
Use a neutral string for the text field so only the error key triggers the match:
it('returns true for Claude Code stream-JSON "error":"authentication_failed" field', () => {
const jsonLine = JSON.stringify({
type: "assistant",
error: "authentication_failed",
// Neutral text — must NOT independently satisfy the pattern
message: { content: [{ type: "text", text: "Please authenticate to continue." }] },
});
expect(isAuthenticationFailedError(jsonLine)).toBe(true);
});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.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ss_retry_guard Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in c578de5.
|
🤖 PR Triage
Rationale: Targeted Claude Code auth error detection fix (3 files, +29/-3). AI bot APPROVED. Prevents wasted retry cycles on 401 auth failures. Batch with #45985 (similar small bug fixes).
|
|
🎉 This pull request is included in a new release. Release: |
AUTHENTICATION_FAILED_PATTERNonly matched Anthropic-direct auth text ("Authentication failed (Request ID: ...)"), missing the signals Claude Code emits when auth fails via the AWF API proxy — causing the harness to burn all 4 retry attempts instead of aborting immediately.Changes
claude_harness.cjs— extendAUTHENTICATION_FAILED_PATTERNto cover the two additional auth failure signals:"error":"authentication_failed"— JSON field in Claude Code stream output on 401not logged in— text content ("Not logged in · Please run /login") emitted when no credentials are presentclaude_harness.test.cjs— add three test cases for the new pattern branches.