Skip to content

Address four Codex review rounds on the review bot - #224

Merged
terasakisatoshi merged 4 commits into
mainfrom
fix-review-bot-continuation-secrets
Aug 4, 2026
Merged

Address four Codex review rounds on the review bot#224
terasakisatoshi merged 4 commits into
mainfrom
fix-review-bot-continuation-secrets

Conversation

@terasakisatoshi

@terasakisatoshi terasakisatoshi commented Aug 4, 2026

Copy link
Copy Markdown
Member

Carries the review-bot fixes raised across four Codex review rounds on tensor4all/tensor4all-rs#568 and tensor4all/tenferro-rs#1603, both of which review this same shared script. Rebased onto main after #223 merged, so it now carries only the work that is not already there.

Two files, both under scripts/. No library code.

What was wrong

Secrets reaching the external model

Escape Detail
Typed declarations const API_KEY: &str = "..." — the redactor treated the type colon as the separator and masked &str, leaving the literal
Quoted values with spaces Only the first word was masked; most of a passphrase was uploaded, and the mask made it look handled
Quoted continuation lines The assignment stays on an unchanged context line while only the value line is replaced, so the added line carries no credential-shaped name
Bare continuation lines Same, with an unquoted value — the first continuation fix only accepted quoted literals

The last two also defeated the redactor: its separator used \s*, crossed the newline, and consumed the - deletion marker as the assignment value.

Widening the value pattern to allow spaces broke tenferro-rs's existing token_type: "WebGPU event token from another queue" regression test — which settles the design question. A diceware passphrase is prose by construction, so no value-shape heuristic separates a credential from a description. The discrimination moved to the name: is_credential_name rejects identifiers ending in _type, _name, _path, _id, and similar metadata suffixes, in both detection and redaction.

Findings silently discarded

  • Deletion-only violations. filter_findings dropped every unanchored block, but a violation introduced by deleting required validation or a // SAFETY: comment has no new-file line to point at, so the model must return line: null. Such a diff passed the gate even when the LLM correctly flagged it. File-level blocks now survive for files the diff deletes lines from.
  • Oversized hunk offsets. Splitting a hunk repeated the original @@ header, so later chunks produced line numbers thousands of lines too small — dropped by the anchor filter, or worse retained against an unrelated added line that happened to collide. Each chunk is renumbered to its own offsets.
  • Untracked files. git diff <base> has no object to compare for a new file, so the documented --worktree preview reviewed brand-new files with nothing and reported a false pass. I had been bitten by this without noticing.
  • Non-ASCII pathnames. git C-quotes them by default and the quoted form matches no real path.
  • Content-blind routing. An unsafe block added under a filename matching no trigger never got the unsafe rules supplied — and the prompt forbids inventing unsupplied requirements, so the rule was unenforceable there, not merely unlikely to fire.

The budget mechanism defeating itself

Retrying once on a transient failure prevents a blocked PR per network blip, but at a 300s per-attempt timeout a three-chunk diff could exceed the job's 20 minutes and be killed mid-request — losing the report entirely, which is the failure the retry existed to prevent. A cumulative 900s budget now clamps each request to the remaining time.

Then the clamped final request's timeout was itself converted into a block, failing the gate at exactly the moment the budget engaged. Deadline-triggered transport failures now emit the warn-only budget-exhausted finding, which also reports the configured --budget-seconds rather than always the default.

Recurring pattern

For four rounds running, the fixtures written to test a new detector tripped that detector — each time caught by running the bot against its own branch, never by the test suite. All secret-shaped fixtures are assembled at runtime now, so the source carries no contiguous secret-shaped literal while the tests still exercise the real shapes.

Verification

  • 80 script tests pass
  • contains_sensitive_text self-scan of both scripts: zero hits
  • The bot reviews its own branch cleanly, and the repository rules review check is green on this PR with the LLM pass actually running (the repository secret was replaced mid-way; earlier runs failed on a non-ASCII key, which this PR also diagnoses precisely instead of blaming the model)

Refs #199

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Repository rules review

Repository rules review (817c1e9fa91bcb1fe2e58bef3fa85f6236ec1873...7bb346b562618eb1cd63e4985ec3b159b8f12f2c)
Verdict: pass
LLM review: 1 chunk(s) (29331 chars) in 10.9s; 0 finding(s) returned, 0 kept, 0 dropped by diff-anchor filtering.
No findings.

terasakisatoshi and others added 4 commits August 4, 2026 12:21
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>
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>
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>
@terasakisatoshi
terasakisatoshi force-pushed the fix-review-bot-continuation-secrets branch from 7409cfa to 7bb346b Compare August 4, 2026 03:21
@terasakisatoshi terasakisatoshi changed the title Block secrets added on a continuation line Address four Codex review rounds on the review bot Aug 4, 2026
@terasakisatoshi
terasakisatoshi enabled auto-merge (squash) August 4, 2026 03:22
@terasakisatoshi
terasakisatoshi merged commit 0082be1 into main Aug 4, 2026
10 checks passed
@terasakisatoshi
terasakisatoshi deleted the fix-review-bot-continuation-secrets branch August 4, 2026 03:23
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.

1 participant