Fix review bot network timeout handling - #223
Merged
Merged
Conversation
A live run in the tensor4all-rs port of this script surfaced three problems, all inherited from the tenferro original. `socket.timeout` escaped the exception tuple as an unhandled traceback. It only aliases `TimeoutError` from Python 3.10 on, so `(KeyError, ValueError, URLError, TimeoutError)` misses it on 3.9. This matters beyond the version gap: the workflow tees only stdout into the report, so the traceback went to stderr and the PR comment would have read "Review step did not produce a report" with no diagnostic. Catching `OSError` covers socket.timeout, TimeoutError, and URLError on every version, so the existing `llm-review-unusable` block finding reaches the comment instead. Transient network failures now retry once before blocking a PR. One blocked PR per network blip is not an acceptable failure mode for a required-adjacent check. A 108k single chunk timed out against DeepSeek at the 120s default. Aggregate chunks are capped at 60k and the default timeout is 300s. Per-chunk latency measured here was 26-150s depending on size, so 300s leaves real margin. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Repository rules reviewRepository rules review (9c165edd761dd0190cbe9587b8459deb88e31fc6...ebe440e1f369febad618a2a64be291a05d29e3dc) Verdict: fail LLM review: 1 chunk(s) (26068 chars) in 0.0s; 0 finding(s) returned, 0 kept, 0 dropped by diff-anchor filtering. Findings: - [block] llm-review-unusable (External LLM Review) <unknown>: External LLM review did not produce usable JSON The repository-rules review could not parse or validate the model response: UnicodeEncodeError: 'latin-1' codec can't encode characters in position 35-39: ordinal not in range(256) |
The first pass caught OSError, which covers socket.timeout, TimeoutError, ConnectionResetError, ssl.SSLError, and urllib.error.URLError. http.client.IncompleteRead is a sibling of OSError rather than a subclass, so a truncated chunked response still escaped. Name both roots in one TRANSPORT_ERRORS tuple and assert the coverage in a test. Keeps this file identical to the tenferro-rs and tensor4all-rs copies. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
terasakisatoshi
added a commit
to tensor4all/tensor4all-rs
that referenced
this pull request
Aug 4, 2026
Three findings from the Codex review of this PR, plus the root cause of the failure the bot itself hit on tensor4all/strided-rs#223. **Typed declarations hid secrets from the pre-upload guard (P1).** For `const API_KEY: &str = "..."`, `SECRET_ASSIGNMENT` treated the type colon as the separator and redacted `&str`, leaving the literal intact, while `QUOTED_SECRET_ASSIGNMENT` did not match through the annotation at all. The credential therefore survived redaction and was uploaded to the external LLM. Both patterns now allow a short type annotation between the name and the value. The detector's own fixtures were themselves secret-shaped, so this file tripped the improved guard and would have blocked the LLM pass on every PR touching its tests. Fixtures are now assembled at runtime; the file contains no contiguous secret-shaped literal while the tests still exercise the real shapes. **Oversized hunks reported wrong line numbers (P2).** Every chunk repeated the original hunk header even though later chunks start thousands of lines further into the file, so `filter_findings` either dropped a real finding or kept it against an unrelated added line that happened to collide. Each chunk now carries a header rewritten to its own old/new offsets, counting context, removal, and addition lines separately. An unparseable header falls back to the previous verbatim behaviour rather than inventing offsets. **Worktree previews skipped untracked files (P2).** `git diff <base>` compares the working tree against a commit, so a newly created file has no object to compare and was omitted from the documented local preview entirely -- a brand new file was reviewed by nothing and the preview reported a false pass. Untracked paths are now enumerated with `ls-files --others --exclude-standard` and diffed against /dev/null with `--no-index`, which needs no staging and does not touch the index. **An unusable API key was reported as unparseable JSON.** HTTP header values are latin-1, so a key carrying non-ASCII text raises UnicodeEncodeError before any request leaves the machine. That is a ValueError subclass, so it surfaced as "External LLM review did not produce usable JSON" and pointed the reader at the model instead of at the secret. The key is now validated up front with a diagnostic that names the offending offsets without echoing the value, and the generic wording no longer claims the model responded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ported from the Codex review of tensor4all/tensor4all-rs#568, which reviewed the same script. All three findings apply verbatim here. Typed declarations hid secrets from the pre-upload guard. For `const API_KEY: &str = "..."`, SECRET_ASSIGNMENT treated the type colon as the separator and redacted the type, leaving the literal, while QUOTED_SECRET_ASSIGNMENT did not match through the annotation at all, so the credential was uploaded. Both patterns now allow a short type annotation. The detector fixtures were themselves secret-shaped and tripped the improved guard, so they are assembled at runtime now. Oversized hunks reported wrong line numbers: every chunk repeated the original header, so findings in later chunks came back with line numbers thousands of lines too small. Each chunk now carries a header rewritten to its own offsets. Worktree previews skipped untracked files entirely, so a brand new file was reviewed by nothing and the documented preview reported a false pass. Untracked paths are diffed against /dev/null with --no-index. An unusable API key was reported as unparseable JSON. This is the failure the bot hit on strided-rs#223: a non-ASCII key raises UnicodeEncodeError while encoding the latin-1 Authorization header, and that is a ValueError subclass, so it surfaced as "did not produce usable JSON". The key is validated up front now, with a diagnostic that names the offending offsets without echoing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Refs #199
terasakisatoshi
added a commit
to tensor4all/tensor4all-rs
that referenced
this pull request
Aug 4, 2026
* Add repository rules review bot Port tenferro-rs's delta-scoped REPOSITORY_RULES review, per #566 Phase 1. The workflow runs from the trusted base revision and treats PR contents as data: the PR head is fetched for `git diff` only, never checked out or executed. Findings post as a single updating PR comment; only block-severity findings fail the check. It covers PRs to both main and develop. #566's structural diagnosis is that #552 ported tenferro's rule text as prose but almost none of the enforcement layer. Two rules are exactly machine checkable, so they run deterministically, before and independently of the LLM: - New direct `tenferro-*` dependency outside tensor4all-tensorbackend (Dense Layout And Linear Algebra). Cargo.toml names `tenferro-*` both as feature names and as dependencies, so the check tracks table context rather than grepping; dev-dependencies are out of scope. - Added `ignore` / `no_run` doctest fence (Documentation Examples). Both are delta-scoped. #566 records existing violations of each (Phase 3 for the dependency route, Phase 1 for the two `no_run` sites), so a repo-wide gate would fail on the backlog. These stop new violations while the backlog burns down separately. Verified against the commits that introduced each class: 795c087 and 21c2f07 both produce the expected block. Section routing covers all 26 rule sections. Base Branch Synchronization is marked human-only: it is a git-workflow protocol, so nothing inside a diff can satisfy or violate it, and showing it to a diff-scoped reviewer only invites invented findings. A coverage test fails if a section is neither routed, always-on, nor explicitly human-only, so the sections Phase 2 will rewrite cannot go silently unreviewed. The prompt carries the two-stack context from #566: the network stack and the TCI stack differ in naming, option style, and error types by design, so a difference between them is not by itself a finding. Three fixes to the ported script, all found by a live run against 69a24e7: - `socket.timeout` escaped the exception tuple as an unhandled traceback. It only aliases TimeoutError from Python 3.10 on, so the ported `(KeyError, ValueError, URLError, TimeoutError)` missed it on 3.9. Catching OSError covers every version, and the report reaches the PR comment instead of a stack trace reaching stderr. - Transient network failures now retry once before blocking a PR. - A 108k single chunk timed out at the 120s default. Aggregate chunks are capped at 60k and the default timeout is 300s; the same commit now reviews as two chunks in 235s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover ConnectionResetError, SSLError, and IncompleteRead The first pass caught OSError, which covers socket.timeout, TimeoutError, ConnectionResetError, ssl.SSLError, and urllib.error.URLError. http.client.IncompleteRead is a sibling of OSError rather than a subclass, so a truncated chunked response still escaped. Name both roots in one TRANSPORT_ERRORS tuple and assert the coverage in a test. Keeps this file identical to the tenferro-rs and strided-rs copies. Refs #566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address Codex review: secret guard, hunk offsets, untracked previews Three findings from the Codex review of this PR, plus the root cause of the failure the bot itself hit on tensor4all/strided-rs#223. **Typed declarations hid secrets from the pre-upload guard (P1).** For `const API_KEY: &str = "..."`, `SECRET_ASSIGNMENT` treated the type colon as the separator and redacted `&str`, leaving the literal intact, while `QUOTED_SECRET_ASSIGNMENT` did not match through the annotation at all. The credential therefore survived redaction and was uploaded to the external LLM. Both patterns now allow a short type annotation between the name and the value. The detector's own fixtures were themselves secret-shaped, so this file tripped the improved guard and would have blocked the LLM pass on every PR touching its tests. Fixtures are now assembled at runtime; the file contains no contiguous secret-shaped literal while the tests still exercise the real shapes. **Oversized hunks reported wrong line numbers (P2).** Every chunk repeated the original hunk header even though later chunks start thousands of lines further into the file, so `filter_findings` either dropped a real finding or kept it against an unrelated added line that happened to collide. Each chunk now carries a header rewritten to its own old/new offsets, counting context, removal, and addition lines separately. An unparseable header falls back to the previous verbatim behaviour rather than inventing offsets. **Worktree previews skipped untracked files (P2).** `git diff <base>` compares the working tree against a commit, so a newly created file has no object to compare and was omitted from the documented local preview entirely -- a brand new file was reviewed by nothing and the preview reported a false pass. Untracked paths are now enumerated with `ls-files --others --exclude-standard` and diffed against /dev/null with `--no-index`, which needs no staging and does not touch the index. **An unusable API key was reported as unparseable JSON.** HTTP header values are latin-1, so a key carrying non-ASCII text raises UnicodeEncodeError before any request leaves the machine. That is a ValueError subclass, so it surfaced as "External LLM review did not produce usable JSON" and pointed the reader at the model instead of at the secret. The key is now validated up front with a diagnostic that names the offending offsets without echoing the value, and the generic wording no longer claims the model responded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address the second Codex review round Cargo renaming and dependency subtables bypassed the tenferro route check; tilde doctest fences bypassed the doctest check; quoted secrets containing spaces were neither detected nor fully redacted; git C-quoted non-ASCII pathnames so those files were reviewed by nothing; path-only routing never supplied the unsafe rules for a generic filename; and cumulative retries could outlive the job timeout and lose the report. Adds the work log this repository's Work Logs And Design Records rule requires, with the classification ledger for both review rounds. Refs #566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address the third Codex review round Continuation-line secrets: an assignment can stay unchanged on a context line while only the value line is replaced, so checking added lines in isolation sees a bare literal with no credential-shaped name. sensitive_diff_location now walks the diff in order and tracks an assignment whose value has not appeared yet. The redactor's separator is also line-local now; it used to cross the newline and consume the deletion marker as the value, leaving the new literal untouched. Cargo manifests outside crates/ were skipped entirely because crate_of() returned None. xtask, tools/api-dump, docs/tutorial-code, and docs/book-tests are workspace members, so a direct tenferro dependency there passed undetected. Scope is now every Cargo.toml, exempting the route package by its [package] name rather than its directory. TOML comments were treated as declarations, so a commented example or a trailing comment produced a block finding. Comments are stripped, respecting quoted hashes. The new fixtures tripped the new detector, as with the earlier rounds; the opener is assembled at runtime so the file no longer trips its own guard. Refs #566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Report the configured review budget, not the default budget_exhausted_finding interpolated DEFAULT_BUDGET_SECONDS into its message regardless of --budget-seconds, so `--budget-seconds 30` produced a diagnostic claiming a 900s budget ran out. That misleads exactly the reader who is trying to work out why a review came back incomplete. Pass the configured value through and assert both halves in the test. Refs #566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes to
scripts/repository-rules-review.py, all surfaced by a live run while porting the same script to tensor4all-rs (tensor4all/tensor4all-rs#568), and all inherited from the tenferro original.1.
socket.timeoutescaped the exception tupleThe ported handler caught
(KeyError, ValueError, urllib.error.URLError, TimeoutError).socket.timeoutonly became an alias ofTimeoutErrorin Python 3.10, so on 3.9 it propagated as an unhandled traceback:This matters beyond the version gap. The workflow pipes only stdout into the report (
| tee review-report.txt), so the traceback went to stderr and the PR comment would have read "Review step did not produce a report" with no diagnostic at all.Catching
OSErrorcoverssocket.timeout,TimeoutError, andurllib.error.URLErroron every version, so the existingllm-review-unusableblock finding reaches the PR comment as designed.2. No retry on transient failures
One blocked PR per network blip is not an acceptable failure mode. Requests now retry once with a 5s backoff before the block finding is raised.
3. Chunk size and timeout were mismatched
A 108k single chunk timed out at the 120s default. Aggregate chunks are now capped at 60k (
MAX_DIFF_CHARS) and the default timeout is 300s. Measured per-chunk latency in this repository was 26–150s depending on size, so 300s leaves real margin, and the tensor4all-rs commit that timed out now completes as two chunks in 235s.Verification
49 script tests pass, including two new regression tests that inject
socket.timeoutand assert the retry succeeds and that an exhausted retry still propagates.Refs #199
🤖 Generated with Claude Code