fix: report local scan failures without connectivity advice - #154
fix: report local scan failures without connectivity advice#154mldangelo wants to merge 1 commit into
Conversation
classifyConnectionFailure matches bare words anywhere in a message, and the unauthorized, forbidden and rate-limited branches of scanFailureMessage replace the message entirely with fixed advice. A local failure whose text merely contained one of those words was therefore reported as a credential problem, and its own text reached neither stderr nor the JSON error field. The reported trigger is "permission denied", the standard strerror for EACCES: a read-only TMPDIR surfaced as "The stored ChatGPT credentials cannot access the configured model." Skip classification for failures that cannot originate from the model transport -- InvalidTargetError, OutputDirectoryError, ConfigurationError, PluginPythonUnavailableError, and filesystem syscall errno codes -- so they keep their own message. Network errno codes are deliberately excluded from that set because they are genuine transport failures. Advice still replaces the underlying text for real transport failures. Upstream authentication and authorization errors can name the organization or project, and cli-authentication.test.ts pins that they must not reach stderr. Fixes openai#36
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec01cfe904
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| error instanceof InvalidTargetError || | ||
| error instanceof OutputDirectoryError || | ||
| error instanceof ConfigurationError || | ||
| error instanceof PluginPythonUnavailableError |
There was a problem hiding this comment.
Recognize PluginBootstrapError as local
When --plugin names an invalid local path containing 403 or forbidden (for example, a regular file at /tmp/403), resolvePluginPath throws the base PluginBootstrapError at runtime.ts:1007-1009. This guard recognizes only its PluginPythonUnavailableError subclass, and the wrapper has no syscall code, so classifyConnectionFailure interprets the path as a model authorization failure and discards the actionable plugin-path message. Treat the full PluginBootstrapError family as local.
Useful? React with 👍 / 👎.
|
Thanks for the thoughtful work on making local scan failures easier to understand. The substantive error handling from this branch is already on |
Fixes #36.
Scope correction — two of the three reported reproductions are already fixed
I re-ran all three reproductions from the issue against current
main(9c7634b) before writing anything. The issue was filed againstf22d4a36, and thenetwork_error/timeoutbranches ofscanFailureMessagenow already fall through tocliErrorMessage:mainPath target does not exist: src/network/client.tsunknown Git ref: origin/connection-fixEACCES: permission denied, mkdtemp '/tmp/...'So this PR addresses the one surviving case, which is the
unauthorized/forbidden/rate_limitedbranches — the three that still discard the original message. The practical trigger ispermission denied, the standardstrerrorforEACCES, which theforbiddenpattern matches verbatim:A read-only
TMPDIRtherefore surfaces as "The stored ChatGPT credentials cannot access the configured model. Use an account or API key with model access." — sending the user to reconfigure credentials for achmodproblem, with the real path never printed.What this changes
Classification is skipped for failures that cannot originate from the model transport, so they keep their own message:
InvalidTargetError— path and Git ref validationOutputDirectoryError— output directory checksConfigurationError— local configPluginPythonUnavailableError— interpreter discoveryEACCES,EPERM,ENOSPC,EROFS,ENOENT, …)Network errno codes (
ECONNRESET,ENOTFOUND,ETIMEDOUT, …) are deliberately excluded from that set — they are genuine transport failures and must keep classifying. The set is an explicit allowlist rather than a/^E[A-Z]+$/test for exactly that reason.Why I did not take the issue's alternative suggestion
The issue offers a second option: "always include the original message alongside the advice so no failure is reported without its cause."
I implemented that first, and the existing suite caught it.
tests-ts/cli-authentication.test.ts→ "identifies overriding API keys in noninteractive scan auth failures" deliberately asserts that upstream text must not reach stderr:Upstream authentication and authorization errors can name the organization or project, and suppressing that is intentional. Appending the cause would leak it into both stderr and the JSON
errorfield. So the advice branches still replace the underlying text, and I added a comment recording why, since the next person to read this will have the same idea I did.That trade-off means a genuinely misclassified transport error still loses its text. Narrowing the
forbiddenpattern itself (for instance requiringpermission deniedto co-occur with model/auth context) would close that too, but it changes retry-versus-fatal behavior in the event stream and felt out of scope here.Also out of scope
The issue notes the same classifier gates retry-versus-fatal in the
api.tsevent-streamerrorbranch. Those messages come from the Codex transport, where classification is the intended behavior, and changing it risks turning fatal auth failures into retry loops. Worth a separate look, not folded in here.Testing / QA instructions
Baseline before this branch: 470 pass / 6 skip / 0 fail. After: 472 pass / 6 skip / 0 fail (two added tests).
Confirm the test reproduces the bug
New coverage
reports local input and filesystem failures without connectivity advice— drives realmain()through thecli-fixturesharness with six failures:EACCESfrommkdtemp,EPERMfrommkdir, anOutputDirectoryError, anInvalidTargetErrornaming asrc/403/path, anInvalidTargetErrornaming anorigin/forbidden-pathsref, and aPluginPythonUnavailableError. Each asserts the real message reaches stderr and that no credential, authentication, or rate-limit advice appears.The
403andforbiddencases are deliberate: they cover the other two words in theforbiddenpattern that a repository path or branch name can plausibly contain, not justpermission denied.keeps model authorization advice for genuine transport failures— the guard against over-correcting. ACodexSecurityErrorcarrying401 invalid API key for org-private/403 model access denied for org-privatemust still produce advice and must still not leakorg-private.Regression surface
The pre-existing tests that pin this area are unmodified and still pass —
cli-authentication.test.ts"identifies overriding API keys…" (theorg-privatesuppression above) and the threecli.test.tscases asserting sqlite/database/sandbox errors do not claim a network failure.