Add repository rules review bot - #568
Conversation
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>
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>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11fa03bb9
ℹ️ 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".
| r"(?i)\b(" | ||
| r"[\w.-]*(?:api[_-]?key|token|secret|password|passwd|pwd|client[_-]?secret|" | ||
| r"private[_-]?key)[\w.-]*" | ||
| r"\s*[:=]\s*)" | ||
| r"([^\s#]+)" |
There was a problem hiding this comment.
When a PR adds a typed declaration such as const API_KEY: &str = "abcdefghijklmnopqrst", SECRET_ASSIGNMENT treats the type colon as the assignment separator and redacts only &str, while QUOTED_SECRET_ASSIGNMENT does not match through the type annotation. The secret literal therefore survives redact_file_diffs, sensitive_diff_finding does not block the request, and the credential is uploaded to the external LLM despite the documented pre-upload guard. Handle typed declarations conservatively in both detection and redaction.
AGENTS.md reference: AGENTS.md:L285-L292
Useful? React with 👍 / 👎.
| if worktree: | ||
| output = run_git(["diff", "--name-only", base]) |
There was a problem hiding this comment.
When the documented local preview is run before a newly created file has been staged, git diff --name-only <base> omits that untracked file, so neither the deterministic checks nor the LLM review inspect it and the preview can incorrectly report no findings. The official git diff documentation describes this form as comparing the working tree with a commit; an untracked path has no compared Git object. Add untracked paths and their contents to worktree-mode input or clearly require staging first.
AGENTS.md reference: AGENTS.md:L275-L280
Useful? React with 👍 / 👎.
| hunk_header = hunk[0] | ||
| body = hunk[1:] | ||
| prefix = [*header, hunk_header] |
There was a problem hiding this comment.
When one diff hunk exceeds MAX_FILE_DIFF_CHARS, every emitted chunk repeats the original hunk header even though later chunks begin thousands of lines farther into the new file. The model therefore derives incorrect line numbers for findings in later chunks; filter_findings may either discard a real issue or retain it against an unrelated earlier added line. Rewrite each repeated hunk header with the chunk's actual old/new offsets, or otherwise provide explicit source line numbers.
AGENTS.md reference: AGENTS.md:L269-L273
Useful? React with 👍 / 👎.
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
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> Also removes a stale duplicate QUOTED_SECRET_ASSIGNMENT that sat after SEVERITY_ALIASES in this copy and silently overrode the new definition.
|
All three Codex findings were real and are fixed in the latest commit. Ported to the other two copies of this script: tensor4all/strided-rs#223 and tensor4all/tenferro-rs#1603. P1 — typed declarations hid secrets from the pre-upload guardConfirmed. For Both patterns now accept a short type annotation between the name and the value. The existing negative tests ( Second-order problem this exposed: the detector's own test fixtures are secret-shaped, so this file tripped the improved guard and every future PR touching its tests would have been blocked with only a maintainer waiver as recourse. Fixtures are now assembled at runtime from fragments — the source contains no contiguous secret-shaped literal, while the tests still exercise the real shapes. Verified by scanning both scripts with P2 — oversized hunk offsetsConfirmed, including the sharper half of the claim: a wrong line number does not merely get dropped by Each emitted chunk now carries a header rewritten to its own old/new offsets, counting context, removal, and addition lines separately ( This also corrected two tenferro-rs tests that had pinned the old repeat-verbatim behaviour as intended. P2 — untracked files in worktree previewsConfirmed, and I had already been bitten by it without noticing: an earlier Untracked paths are now enumerated with Also fixed: the failure the bot hit on strided-rs#223Not from this review, but the same file. The bot's first live run blocked with: reported as "External LLM review did not produce usable JSON". HTTP header values are encoded latin-1, so a key carrying non-ASCII text raises before any request leaves the machine — and Verification66 script tests pass here (61 in strided-rs, full suite in tenferro-rs), |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bd6b21075
ℹ️ 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".
| TENFERRO_ROUTE_CRATE = "tensor4all-tensorbackend" | ||
| TENFERRO_DEP_TABLES = frozenset({"dependencies", "build-dependencies"}) | ||
| CARGO_TABLE = re.compile(r"^\[([^\]]+)\]\s*$") | ||
| TENFERRO_DEP_KEY = re.compile(r"^\s*(tenferro-[\w-]+)\s*(?:\.|=)") |
There was a problem hiding this comment.
When a crate uses valid Cargo renaming such as linalg = { package = "tenferro-linalg", ... }, a quoted key, or [dependencies.tenferro-linalg], this regex does not identify the package as tenferro, so the advertised deterministic boundary check passes. Because the prompt tells the LLM not to duplicate this deterministic check, such a direct dependency can land entirely undetected; parse TOML package values and dependency subtables rather than relying only on a bare key prefix.
AGENTS.md reference: AGENTS.md:L291-L297
Useful? React with 👍 / 👎.
| output = run_git(["diff", "--name-only", f"{base}...{head}"]) | ||
| return [line.strip() for line in output.splitlines() if line.strip()] |
There was a problem hiding this comment.
When a PR changes a non-ASCII pathname such as 日本.md, Git's default --name-only output is C-quoted (for example, "\346...md"), and this code treats that representation as the literal filesystem path; per_file_diffs therefore produces no LLM chunk and extension-based deterministic checks do not recognize the file. Local git diff -h confirms that -z emits NUL-terminated raw names, so consume that form or otherwise unquote paths to ensure every changed file is reviewed.
AGENTS.md reference: AGENTS.md:L269-L272
Useful? React with 👍 / 👎.
* Fix review bot network timeout handling 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> * 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 tensor4all-rs copies. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address Codex review: secret guard, hunk offsets, untracked previews 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 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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>
Batch of related findings against scripts/repository-rules-review.py: a live run of the ported copy plus the Codex review of tensor4all/tensor4all-rs#568, which reviewed this same shared code. Full classification ledger in the work log. No production Rust changes. Transport errors: the handler enumerated leaves and missed siblings. `socket.timeout` only aliases TimeoutError from Python 3.10 on, and ConnectionResetError, ssl.SSLError, and http.client.IncompleteRead were never caught at all -- the last is a sibling of OSError, not a subclass. The failure mode is worse than the crash: the workflow tees only stdout, so a traceback went to stderr and the PR comment read "did not produce a report" with no diagnostic while the gate failed. Name the two roots instead of a leaf list. Retry budget: retrying once prevents a blocked PR per network blip, but at a 300s per-attempt timeout a three-chunk diff could exceed timeout-minutes: 20 and be killed mid-request, losing the report entirely. Bound all LLM traffic to a 900s budget, clamp each request to the remaining time, skip a retry that would cross the deadline, and warn rather than block when the budget runs out. Secret guard: a typed declaration let the redactor mask the type and leave the literal, and a quoted value containing spaces was neither detected nor fully redacted -- most of a passphrase was uploaded. Widening the value pattern then broke this repository's own token_type regression test, which is the point: a diceware passphrase is prose by construction, so no value-shape heuristic can separate the two. Move the discrimination to the name. The guard's own fixtures then tripped it, so a maintainer waiver would have been the only way to review any PR touching its tests. Fixtures are assembled at runtime now; the source carries no contiguous secret-shaped literal. Chunk headers: splitting an oversized hunk repeated the original @@ header, so findings in later chunks came back with line numbers thousands of lines too small and were dropped, or retained against an unrelated colliding line. Each chunk is renumbered to its own offsets. Routing: path-only routing never supplied the unsafe rules for an unsafe block added under a generic filename, and the prompt forbids inventing requirements that were not supplied, so the rule was unenforceable there. Add content triggers. This exposed that HUMAN_ONLY_SECTIONS was never subtracted here -- the "intentionally not routed to the diff-scoped review bot" guarantee in Performance-Gated Experiment Protocol held only because no trigger named one. Also: git C-quotes non-ASCII pathnames by default, so such a file was reviewed by nothing; and a stale duplicate QUOTED_SECRET_ASSIGNMENT after SEVERITY_ALIASES silently overrode the new definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Second review round: all five findings were real and are fixed, and ported to tensor4all/strided-rs#223 and tensor4all/tenferro-rs#1603. Each was reproduced against the working tree before changing anything. P1 — Cargo package aliases in the dependency gateConfirmed. The bare-key regex saw only one of three valid declaration forms: [features]
tenferro-cpu-faer = ["dep:tenferro-cpu"] # not a dependency
[dependencies]
linalg = { package = "tenferro-linalg" } # renaming — was invisible
"tenferro-tensor" = { workspace = true } # quoted key — was invisible
[dependencies.tenferro-einsum] # subtable — was invisible
P1 — redact complete assigned secretsConfirmed, and worse than described in one respect: the redactor left Widening the value pattern to allow spaces then broke a regression test tenferro-rs already had for P2 — tilde doctest fencesConfirmed; P2 — decode Git pathnamesConfirmed. Rather than switching call sites to P2 — route rules using changed contentConfirmed, and this was the most consequential one: the prompt explicitly forbids inventing requirements that were not supplied, so a rule that never got routed was not merely unlikely to be enforced, it was unenforceable. Added Rejected making the safety sections always-on: that inflates every request with a 463-line rules file where routing is what keeps the cost down. Adding content routing exposed a related latent bug upstream — Also folded inThe tenferro-rs review raised a cumulative-retry problem that applies here identically: at a 300s per-attempt timeout a three-chunk diff could exceed the job timeout and be killed mid-request, losing the report. All LLM traffic is now bounded by a 900s budget, with a Added Verification80 script tests pass, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f59bc3720e
ℹ️ 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".
| for path, entries in added_lines_with_text(diff_text).items(): | ||
| for line_no, text in entries: | ||
| if contains_sensitive_text(text): |
There was a problem hiding this comment.
Fresh evidence beyond the prior multiline fix is a declaration whose assignment line is unchanged, such as const API_KEY: &str =, while the following value line is replaced with a quoted credential. This loop checks each added line in isolation, so the value line has no credential-shaped name and is not blocked; the full-diff redactor then consumes the intervening deletion marker as the assignment value and leaves the new literal available for external upload. Inspect adjacent context for continued assignments or conservatively block standalone quoted values replacing such assignments.
AGENTS.md reference: AGENTS.md:L285-L290
Useful? React with 👍 / 👎.
| if crate is None or crate == TENFERRO_ROUTE_CRATE: | ||
| continue |
There was a problem hiding this comment.
When a workspace package outside crates/—for example docs/tutorial-code/Cargo.toml, tools/api-dump/Cargo.toml, or xtask/Cargo.toml—adds a direct runtime tenferro-* dependency, crate_of() returns None and this branch skips the manifest. The LLM prompt also suppresses this class of finding because the deterministic gate is expected to cover it, so the new dependency can pass entirely undetected; exempt only the tensorbackend package rather than every manifest outside crates/.
AGENTS.md reference: AGENTS.md:L290-L292
Useful? React with 👍 / 👎.
| renamed = TENFERRO_PACKAGE_VALUE.search(line) | ||
| if renamed: | ||
| found[line_no] = renamed.group(1) |
There was a problem hiding this comment.
When an added comment inside a dependency table contains an example such as # linalg = { package = "tenferro-linalg" }, this unanchored search classifies the comment as a real dependency and produces a block-severity finding. The same false positive occurs when package = "tenferro-*" appears in an inline comment after an unrelated dependency, so strip TOML comments or parse the manifest structurally before matching package aliases.
AGENTS.md reference: AGENTS.md:L290-L297
Useful? React with 👍 / 👎.
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>
Ported from the third Codex review round on tensor4all/tensor4all-rs#568, which reviewed this same shared script. 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 and uploads it. sensitive_diff_location now walks the diff in order and tracks an assignment whose value has not appeared yet. The redactor's separator is line-local now. It used to cross the newline and consume the deletion marker as the assignment value, leaving the following line's literal untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Third round: all three findings were real and are fixed. Ported to tensor4all/strided-rs#224 and tensor4all/tenferro-rs#1603. P1 — secrets on a continuation lineConfirmed, and it defeated both halves of the guard. The assignment stays on an unchanged context line while only the value line is replaced: const API_KEY: &str =
- "old";
+ "s3cr3t-passphrase";Detection saw a bare literal with no credential-shaped name. And the redactor's
P1 — workspace packages outside
|
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50abb4db5d
ℹ️ 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".
Deletion-only violations were silently dropped. filter_findings discarded any file-level block finding, but a violation introduced by deleting required validation, coverage, or a safety comment has no new-file line to anchor to, so the model must return line: null. Such a diff passed the gate even when the LLM correctly identified a blocking violation. File-level blocks are now kept for files this diff deletes lines from. Continuation values need not be quoted. An unchanged `API_KEY =` followed by a replaced bare value line was neither detected nor redacted, so the credential was sent verbatim. STANDALONE_VALUE now accepts bare tokens; awaiting_value is only set for credential-named assignments, so this cannot fire on ordinary multi-line expressions. A timeout at the cumulative deadline was reported as a block. The budget clamps the last chunk to the remaining time, and that timeout was converted into llm-review-unusable, failing the gate for exactly the case the budget exists to degrade gracefully. Deadline-triggered transport failures now emit the warn-only budget-exhausted finding. Refs #199 Ported from the Codex review of tensor4all/tensor4all-rs#568. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fourth round: all three findings were real and are fixed, along with the parallel finding on tensor4all/tenferro-rs#1603. Ported to tensor4all/strided-rs#224 as well. P1 — deletion-only changes lost their block findingsConfirmed, and this one was a hole in the gate's premise. The unanchored-block rule still earns its keep against the model generalising about a file, so rather than dropping it I scoped the exception: file-level blocks survive for files this diff deletes lines from. P1 — unquoted continuation valuesConfirmed. My previous round's API_KEY =
-old
+abcdefghijklmnopqrstuvwxwent through untouched. Bare tokens are accepted now. The false-positive risk you'd normally worry about does not apply here: P2 —
|
Ported from the Codex review of tensor4all/tensor4all-rs#568, which reviewed this same shared script. Quoted secrets containing spaces were neither detected nor fully redacted, so most of a passphrase was uploaded. Widening the value pattern needs the name to carry the discrimination, since a diceware passphrase is prose by construction. git C-quoted non-ASCII pathnames, so those files were reviewed by nothing. Path-only routing never supplied the unsafe rules for an unsafe block added under a generic filename, and the prompt forbids inventing unsupplied requirements, so the rule was unenforceable there. Cumulative retries could outlive the job timeout and lose the report entirely. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ported from the third Codex review round on tensor4all/tensor4all-rs#568. 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 and uploads it. sensitive_diff_location now walks the diff in order and tracks an assignment whose value has not appeared yet. The redactor's separator is line-local now. It used to cross the newline and consume the deletion marker as the assignment value, leaving the following line's literal untouched. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deletion-only violations were silently dropped. filter_findings discarded any file-level block finding, but a violation introduced by deleting required validation, coverage, or a safety comment has no new-file line to anchor to, so the model must return line: null. Such a diff passed the gate even when the LLM correctly identified a blocking violation. File-level blocks are now kept for files this diff deletes lines from. Continuation values need not be quoted. An unchanged `API_KEY =` followed by a replaced bare value line was neither detected nor redacted, so the credential was sent verbatim. STANDALONE_VALUE now accepts bare tokens; awaiting_value is only set for credential-named assignments, so this cannot fire on ordinary multi-line expressions. A timeout at the cumulative deadline was reported as a block. The budget clamps the last chunk to the remaining time, and that timeout was converted into llm-review-unusable, failing the gate for exactly the case the budget exists to degrade gracefully. Deadline-triggered transport failures now emit the warn-only budget-exhausted finding. Refs #199 Ported from the Codex review of tensor4all/tensor4all-rs#568. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Address the second Codex review round Ported from the Codex review of tensor4all/tensor4all-rs#568, which reviewed this same shared script. Quoted secrets containing spaces were neither detected nor fully redacted, so most of a passphrase was uploaded. Widening the value pattern needs the name to carry the discrimination, since a diceware passphrase is prose by construction. git C-quoted non-ASCII pathnames, so those files were reviewed by nothing. Path-only routing never supplied the unsafe rules for an unsafe block added under a generic filename, and the prompt forbids inventing unsupplied requirements, so the rule was unenforceable there. Cumulative retries could outlive the job timeout and lose the report entirely. Refs #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Block secrets added on a continuation line Ported from the third Codex review round on tensor4all/tensor4all-rs#568. 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 and uploads it. sensitive_diff_location now walks the diff in order and tracks an assignment whose value has not appeared yet. The redactor's separator is line-local now. It used to cross the newline and consume the deletion marker as the assignment value, leaving the following line's literal untouched. Refs #199 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 #199 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix four more review-bot gaps Deletion-only violations were silently dropped. filter_findings discarded any file-level block finding, but a violation introduced by deleting required validation, coverage, or a safety comment has no new-file line to anchor to, so the model must return line: null. Such a diff passed the gate even when the LLM correctly identified a blocking violation. File-level blocks are now kept for files this diff deletes lines from. Continuation values need not be quoted. An unchanged `API_KEY =` followed by a replaced bare value line was neither detected nor redacted, so the credential was sent verbatim. STANDALONE_VALUE now accepts bare tokens; awaiting_value is only set for credential-named assignments, so this cannot fire on ordinary multi-line expressions. A timeout at the cumulative deadline was reported as a block. The budget clamps the last chunk to the remaining time, and that timeout was converted into llm-review-unusable, failing the gate for exactly the case the budget exists to degrade gracefully. Deadline-triggered transport failures now emit the warn-only budget-exhausted finding. Refs #199 Ported from the Codex review of tensor4all/tensor4all-rs#568. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Deletion-only violations were silently dropped. filter_findings discarded any file-level block finding, but a violation introduced by deleting required validation, coverage, or a safety comment has no new-file line to anchor to, so the model must return line: null. Such a diff passed the gate even when the LLM correctly identified a blocking violation. File-level blocks are now kept for files this diff deletes lines from. Continuation values need not be quoted. An unchanged `API_KEY =` followed by a replaced bare value line was neither detected nor redacted, so the credential was sent verbatim. STANDALONE_VALUE now accepts bare tokens; awaiting_value is only set for credential-named assignments, so this cannot fire on ordinary multi-line expressions. A timeout at the cumulative deadline was reported as a block. The budget clamps the last chunk to the remaining time, and that timeout was converted into llm-review-unusable, failing the gate for exactly the case the budget exists to degrade gracefully. Deadline-triggered transport failures now emit the warn-only budget-exhausted finding. Ported from the Codex review of tensor4all/tensor4all-rs#568. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Implements the Phase 1 item in #566: "port tenferro's
repository-rules-review.py+review_bot.yml(diff-scoped LLM review against REPOSITORY_RULES.md) with the section-routing convention". Same port as tensor4all/strided-rs#222, adapted to this workspace. Part of the environment work inshinaoka/task-management-terasaki#26.What lands
.github/workflows/review_bot.ymlmainanddevelopscripts/repository-rules-review.pyscripts/test-repository-rules-review.pyai/prompts/repository-rules-review.md.github/actions/{post-review-comment,verify-review-label}/scripts/requirements-dev.txtpython-dotenvAGENTS.mddocuments the bot and local usage;CI_rs.ymlgains ascriptsjob running the script's tests, wired intorollup-rs.REPOSITORY_RULES.mdis unchanged — both deterministic checks enforce rules this repository already states.Security model
pull_request_targetchecks out the trusted base revision; the PR head is fetched forgit diffonly and never checked out or executed. External-fork PRs are rejected at the gate. Both label escape hatches require themaintain/adminrole and reapplication after the latest push. Secret-shaped text in added lines blocks the upload before anything reaches the API.Enforcement, not just prose
#566's structural diagnosis is that #552 ported tenferro's rule text but almost none of the enforcement layer. Two rules are exactly machine-checkable, so they run deterministically, before and independently of the LLM:
tenferro-*dependency outsidetensor4all-tensorbackendignore/no_rundoctest fenceno_runsitesBoth are delta-scoped. A repo-wide gate would fail on the backlog today, so these stop new violations from landing while the backlog burns down separately — complementary to, not a replacement for, the
check-crate-boundaries.pyitem in Phase 1.The dependency check parses Cargo.toml table context rather than grepping, because
tenferro-*appears both as feature names and as dependencies:dev-dependenciesare out of scope: the rule is about the runtime route, and test code may reach for tenferro fixtures directly.Verified against the commits that introduced each class — 795c087 (added tenferro deps to core and treetci) and 21c2f07 (converted
ignorefences tono_run) both produce the expected block, while 69a24e7 passes.Section routing
All 26 rule sections are reachable.
Base Branch Synchronizationis 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.test_every_rule_section_is_reachablefails if a section is neither routed, always-on, nor explicitly human-only — so the sections Phase 2 will rewrite cannot go silently unreviewed.A 26-file commit routes to 20 of 26 sections, which is where the cost saving comes from on a 463-line rules file.
Two-stack awareness
The prompt carries the #566 diagnosis that the network stack and the TCI stack differ in naming, option style, and error types by design. A difference between them is not by itself a finding. It also tells the reviewer that existing
anyhow::Resultsurfaces and existing tenferro dependencies are known backlogs, reportable only when this diff adds to them.Three fixes to the ported script
All found by a live run against 69a24e7, and all present in the tenferro original:
socket.timeoutescaped the exception tuple as an unhandled traceback. It only aliasesTimeoutErrorfrom Python 3.10 on, so the ported(KeyError, ValueError, URLError, TimeoutError)missed it on 3.9. Because only stdout is teed into the report, the PR comment would have said "Review step did not produce a report" while the real error went to stderr. CatchingOSErrorcovers every version.Fixes 1 and 2 are worth backporting to strided-rs and tenferro-rs.
Verification
actionlintclean on both workflowsfail(dependency); 21c2f07 →fail(doctest); 69a24e7 →pass; waived →pass; no-LLM → onewarn. Exit codes checked on all four pathspassSetup needed before this is useful
DEEPSEEK_API_KEYsecret (optionally aDEEPSEEK_MODELvariable; the default isdeepseek-v4-pro)rules-review:waiveandrules-review:no-llmreview-bot-gateas a required checkNote that
pull_request_targetresolves the workflow from the base branch, so this PR does not exercise the bot on itself; the first PR after merge is the real test.Refs #566
🤖 Generated with Claude Code