Skip to content

fix: fall back to the status when an HTTP error body cannot be read - #15

Merged
dmccoystephenson merged 2 commits into
mainfrom
fix/timeout-reading-http-error-body
Sep 30, 2026
Merged

dmccoystephenson merged 2 commits into
mainfrom
fix/timeout-reading-http-error-body

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Contributor

Summary

  • GitHubDocsClient._request read the HTTP error body with e.read() inside the except urllib.error.HTTPError handler. An exception raised inside an except block is not routed to that block's sibling clauses, so a socket.timeout / OSError during that read escaped save_file unconverted, contradicting the README's "Everything that stopped an edit from landing raises GitHubDocsError". The read is now wrapped so that such a failure falls back to an empty body, which yields the existing status-only message GitHub API returned HTTP <code> with .status set.
  • The same handler caught only json.JSONDecodeError, but bytes that are not UTF-8 fail earlier as a UnicodeDecodeError (a ValueError, not a JSONDecodeError), so a gateway answering an error with a non-UTF-8 body escaped the same way. The clause is widened to ValueError, matching the success-path handling added in fix: raise GitHubDocsError for a read timeout or a non-JSON 2xx body #13. This second escape has no tracking issue; it was found while implementing A timeout while reading an HTTP error body escapes save_file as something other than GitHubDocsError #14 and sits in the same four lines, under the same README promise.
  • Two TestErrorSurface cases were added, one per escape.

Half touched: python/ only.

Docs: python/README.md's Errors section already states the behavior this fix restores; no README change was needed.

Regression evidence

Local test execution was unavailable in the dispatch sandbox (only Python 3.8 is present, below the 3.9 floor, and running the suite was not permitted), so the local anchor is UNVERIFIED and CI served as the anchor. The tests were pushed as a separate commit ahead of the fix:

  • 88956c9 (tests only): CI red on every python leg, with exactly test_a_timeout_while_reading_an_error_body_falls_back_to_the_status and test_an_error_body_that_is_not_utf8_falls_back_to_the_status erroring (Ran 51 tests … FAILED (errors=2) on 3.9).
  • 158b961 (fix): see the CI result on the PR head.

Test plan

  • New tests fail without the fix (CI run 36686753818)
  • All eight CI legs green on the PR head
  • No new message bypasses _redact; no non-stdlib import; TOKEN sentinel unchanged

Deferred issues

Closes #14

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code


drafted by Claude on behalf of Daniel Stephenson

dmccoystephenson and others added 2 commits September 30, 2026 01:57
Both tests fail against the current handler: a socket.timeout from
e.read() escapes unconverted, and non-UTF-8 bytes raise a
UnicodeDecodeError that the JSONDecodeError clause does not catch.
Pushed ahead of the fix so CI records the failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e.read() ran inside the HTTPError handler, where the sibling OSError
clause cannot catch what it raises, so a timeout or reset while reading
the error body escaped save_file unconverted. The same handler caught
only JSONDecodeError, so a non-UTF-8 body escaped as UnicodeDecodeError.
Both now fall back to the existing status-only message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Contributor Author

Self-review rubric (anchored on CI run 36686820165 at head 158b961, all eight legs green, Ran 51 tests on 3.9):

  • Scope: PASS with a note. Both files are needed for A timeout while reading an HTTP error body escapes save_file as something other than GitHubDocsError #14. The JSONDecodeError → ValueError widening goes beyond A timeout while reading an HTTP error body escapes save_file as something other than GitHubDocsError #14's literal text. It was kept because it is the same README promise, the same handler and one token, and the PR body calls it out as untracked.
  • Tests-new: PASS. No new public symbol was added. Each of the two escapes has its own test.
  • Tests-fix: PASS via fallback-ladder rung 1 (local execution was unavailable: sandbox Python is 3.8 and running the suite was not permitted). The tests-only commit 88956c9 went red in CI run 36686753818, with exactly the two new tests erroring (FAILED (errors=2)). The fix commit 158b961 is green.
  • Sibling structure: PASS. The new cases sit in TestErrorSurface, beside the matching success-path pair from fix: raise GitHubDocsError for a read timeout or a non-JSON 2xx body #13, and follow its naming.
  • Sibling renames: PASS. N/A, nothing was renamed.
  • Docs: PASS. The Errors section of python/README.md (lines 113–116) already promises this behavior. save_file's numbered steps, the config table and the API list are unaffected.
  • Issue resolution: PASS. A timeout while reading an HTTP error body escapes save_file as something other than GitHubDocsError #14 names wrapping e.read() and a TestErrorSurface case where read raises socket.timeout. Both are present.
  • CI: PASS. 8/8 legs are green on the PR head.
  • No-leak (python): PASS. No new message was constructed. The fallback reaches the existing self._redact(message or f"GitHub API returned HTTP {e.code}"), which quotes less upstream content than before, not more.
  • Stdlib-only: PASS. No import was added, and pyproject.toml is untouched.
  • Sentinel intact: PASS. TOKEN is unchanged.
  • No-leak / result-not-exception / export parity (js), support-matrix parity: N/A. The diff does not touch js/, the manifests or the workflows.

Observations outside the diff:

  • client.py:268-273: when allow_404=True and the 404 body read times out, the caller now gets (404, {}). Before, it got an escaped socket.timeout. Every allow_404 caller only tests for existence, so this is the intended outcome.
  • Do-not-auto-merge check: python/src/github_docs/client.py is protected only when the diff touches _redact, the Authorization header or error-message construction. This diff changes none of the three. It only changes what feeds the existing message, and in the direction of quoting less. It is therefore not treated as a protected-path match.

This review comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit e2c7de7 into main Sep 30, 2026
16 checks passed
@dmccoystephenson
dmccoystephenson deleted the fix/timeout-reading-http-error-body branch September 30, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A timeout while reading an HTTP error body escapes save_file as something other than GitHubDocsError

1 participant