-
Notifications
You must be signed in to change notification settings - Fork 351
fix(copilot-driver): handle auth failures in --continue attempts #26146
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
8aaa0a0
df9662c
bd9d744
541fcc2
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,7 +34,7 @@ describe("copilot_driver.cjs", () => { | |||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| describe("retry policy: resume on partial execution", () => { | ||||||||||||||||||
| describe("retry policy: continue on partial execution", () => { | ||||||||||||||||||
| // Inline the same retry-eligibility logic as the driver for unit testing. | ||||||||||||||||||
| // The driver retries whenever the session produced output (hasOutput), regardless | ||||||||||||||||||
| // of the specific error type. CAPIError 400 is just the well-known case. | ||||||||||||||||||
|
Comment on lines
39
to
40
|
||||||||||||||||||
| // The driver retries whenever the session produced output (hasOutput), regardless | |
| // of the specific error type. CAPIError 400 is just the well-known case. | |
| // The driver generally retries when the session produced output (hasOutput), | |
| // except for known non-retryable cases such as MCP policy and no-auth-info. | |
| // CAPIError 400 is just the well-known retryable case. |
Copilot
AI
Apr 14, 2026
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.
In the "auth fails on a --continue attempt" test, the variable is still named resumeResult, which is confusing now that the driver uses --continue. Renaming it (e.g., continueResult) would align terminology and reduce ambiguity.
| const resumeResult = { exitCode: 1, hasOutput: true, output: "Error: No authentication information found." }; | |
| expect(shouldRetry(resumeResult, 1)).toBe(false); | |
| expect(shouldRetry(resumeResult, 2)).toBe(false); | |
| expect(shouldRetry(resumeResult, 3)).toBe(false); | |
| const continueResult = { exitCode: 1, hasOutput: true, output: "Error: No authentication information found." }; | |
| expect(shouldRetry(continueResult, 1)).toBe(false); | |
| expect(shouldRetry(continueResult, 2)).toBe(false); | |
| expect(shouldRetry(continueResult, 3)).toBe(false); |
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.
The retry-policy doc comment says auth errors can be “no output” cases, but the reported auth failure prints a message to stderr (so
hasOutputis true) and is now handled by the explicit auth-error pattern below. Consider removing “auth error” from the “no output produced” bullet to avoid misleading readers about how auth failures are detected.