From 5818107335840d05962de152f44e6b36ed883060 Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 11:22:50 +0900 Subject: [PATCH 1/4] 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 --- scripts/repository-rules-review.py | 160 ++++++++++++++++++++++-- scripts/test-repository-rules-review.py | 133 ++++++++++++++++++++ 2 files changed, 282 insertions(+), 11 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index ee7fe41..30365be 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -80,13 +80,43 @@ # Matching only `name value` redacts the type and leaves the literal, so # allow a short annotation before the separator that precedes the value. SECRET_ANNOTATION = r"(?:\s*:[^=\n]{0,40})?" +# The value alternatives are ordered quoted-first so a quoted secret is +# consumed whole. Putting the bare-token alternative first would stop at the +# first space inside a quoted passphrase and upload the remainder. (Spelling +# out an example assignment here would make this file trip its own guard.) +SECRET_VALUE = r"""(?:"[^"\r\n]*"|'[^'\r\n]*'|[^\s#]+)""" SECRET_ASSIGNMENT = re.compile( - r"(?i)\b(" + SECRET_NAME + SECRET_ANNOTATION + r"\s*[:=]\s*)([^\s#]+)" + r"(?i)\b(?P" + SECRET_NAME + r")(?P" + SECRET_ANNOTATION + + r"\s*[:=]\s*)(?P" + SECRET_VALUE + r")" ) +# Quoted credentials may legitimately contain spaces (a diceware passphrase is +# prose by construction), so the value shape cannot be the discriminator. QUOTED_SECRET_ASSIGNMENT = re.compile( - r"(?i)\b" + SECRET_NAME + SECRET_ANNOTATION + r"\s*[:=]\s*" - r'''(?:"[^\s"\r\n]{12,}"|\'[^\s\'\r\n]{12,}\')''' + r"(?i)\b(?P" + SECRET_NAME + r")" + SECRET_ANNOTATION + r"\s*[:=]\s*" + r"""(?:"[^"\r\n]{8,}"|'[^'\r\n]{8,}')""" ) +# A quote opened on this line and closed on a later one hides the value from +# any single-line pattern, so treat the opening alone as disqualifying. +UNTERMINATED_SECRET_ASSIGNMENT = re.compile( + r"(?i)\b(?P" + SECRET_NAME + r")" + SECRET_ANNOTATION + + r"""\s*[:=]\s*["'][^"'\r\n]*$""" +) +# Since the value may be prose, the name has to carry the discrimination. +# `token_type`, `secret_name`, and `key_path` describe a credential rather than +# holding one, and their values are ordinary text. +SECRET_NAME_METADATA = re.compile( + r"(?i)[_-]?(?:type|kind|name|names|id|ids|len|length|size|count|path|paths" + r"|file|files|dir|env|var|vars|header|prefix|suffix|field|fields|url|uri" + r"|list|set|map|schema|format|scheme|class|enum|error|regex|pattern|label" + r"|source|store|provider|policy|status|state|mode|version)$" +) + + +def is_credential_name(name: str) -> bool: + """Whether a matched identifier holds a credential rather than describes one.""" + return not SECRET_NAME_METADATA.search(name) + + SEVERITY_ALIASES = { "block": "block", "blocker": "block", @@ -186,6 +216,32 @@ ) +# Signals in the changed lines themselves. Path routing cannot see that a file +# under a generic path just gained an `unsafe` block or a rayon call. +CONTENT_TRIGGERS: tuple[tuple[re.Pattern[str], frozenset[str]], ...] = ( + ( + re.compile(r"\bunsafe\b|get_unchecked|from_raw_parts|\bas_ptr\b|\bas_mut_ptr\b"), + frozenset({"Unsafe And Fast-Path Boundaries"}), + ), + ( + re.compile(r"rayon|par_iter|ExecutionPolicy|num_threads|join\("), + frozenset({"CPU Threading Contract"}), + ), + ( + re.compile(r"\bvec!\[|to_vec\(\)|collect:: dict[str, Any]: def run_git(args: list[str], cwd: Path = ROOT, *, check: bool = True) -> str: + # Without this git C-quotes non-ASCII pathnames ("\346\227\245..."), and + # the quoted form matches no real path: per_file_diffs yields nothing and + # the extension-based deterministic checks never recognise the file. completed = subprocess.run( - ["git", *args], + ["git", "-c", "core.quotePath=false", *args], cwd=cwd, check=check, capture_output=True, @@ -325,12 +384,30 @@ def parse_repository_rules_sections(path: Path = RULES_PATH) -> dict[str, str]: return sections -def select_rule_sections(files: list[str]) -> list[str]: +def select_rule_sections( + files: list[str], + added: dict[str, list[tuple[int, str]]] | None = None, +) -> list[str]: + """Pick the rule sections to show the reviewer. + + Path routing alone misses rule-relevant code added under a generic path: + an `unsafe` block in a file whose name matches no trigger meant the unsafe + rules were never supplied -- and the prompt forbids inventing requirements + that were not supplied, making the rule unenforceable there. Content + triggers close that gap without making every safety section unconditional. + """ selected = set(ALWAYS_SECTIONS) for path in files: for pattern, section_names in SECTION_TRIGGERS: if pattern.search(path): selected.update(section_names) + + if added: + for entries in added.values(): + for _line_no, text in entries: + for pattern, section_names in CONTENT_TRIGGERS: + if pattern.search(text): + selected.update(section_names) return sorted(selected - HUMAN_ONLY_SECTIONS) @@ -614,7 +691,12 @@ def redact_sensitive_text(text: str) -> str: redacted = text for pattern in SECRET_VALUE_PATTERNS: redacted = pattern.sub("[REDACTED_SECRET]", redacted) - return SECRET_ASSIGNMENT.sub(r"\1[REDACTED_SECRET]", redacted) + def mask(match: re.Match[str]) -> str: + if not is_credential_name(match.group("name")): + return match.group(0) + return f"{match.group('name')}{match.group('sep')}[REDACTED_SECRET]" + + return SECRET_ASSIGNMENT.sub(mask, redacted) def redact_file_diffs(file_diffs: dict[str, str]) -> dict[str, str]: @@ -622,9 +704,13 @@ def redact_file_diffs(file_diffs: dict[str, str]) -> dict[str, str]: def contains_sensitive_text(text: str) -> bool: - return any(pattern.search(text) for pattern in SECRET_VALUE_PATTERNS) or bool( - QUOTED_SECRET_ASSIGNMENT.search(text) - ) + if any(pattern.search(text) for pattern in SECRET_VALUE_PATTERNS): + return True + for pattern in (QUOTED_SECRET_ASSIGNMENT, UNTERMINATED_SECRET_ASSIGNMENT): + for match in pattern.finditer(text): + if is_credential_name(match.group("name")): + return True + return False def sensitive_diff_location(diff_text: str) -> tuple[str, int] | None: @@ -815,6 +901,12 @@ def reconcile_verdict(findings: list[Finding]) -> str: ) NETWORK_RETRIES = 2 RETRY_BACKOFF_SECONDS = 5.0 +# Wall-clock ceiling for all LLM traffic in one run. The workflow allows 20 +# minutes; a run that blows past it is killed mid-request, so neither the +# exception handler nor the report ever executes and the gate fails with no +# diagnostic at all -- the exact failure this retry logic exists to prevent. +# Finishing under our own budget guarantees a report is always posted. +DEFAULT_BUDGET_SECONDS = 900.0 def call_deepseek( @@ -825,6 +917,7 @@ def call_deepseek( system_prompt: str, user_content: str, timeout: float, + deadline: float | None = None, ) -> dict[str, Any]: payload = { "model": model, @@ -846,8 +939,11 @@ def call_deepseek( ) last_error: BaseException | None = None for attempt in range(1, NETWORK_RETRIES + 1): + attempt_timeout = timeout + if deadline is not None: + attempt_timeout = min(timeout, max(1.0, deadline - time.monotonic())) try: - with urllib.request.urlopen(request, timeout=timeout) as response: + with urllib.request.urlopen(request, timeout=attempt_timeout) as response: body = json.loads(response.read().decode("utf-8")) break except TRANSPORT_ERRORS as exc: @@ -856,6 +952,10 @@ def call_deepseek( last_error = exc if attempt == NETWORK_RETRIES: raise + if deadline is not None and time.monotonic() + RETRY_BACKOFF_SECONDS >= deadline: + # Retrying would run past the budget and get the job killed, + # which loses the report entirely. Fail now, with a diagnostic. + raise print( f"LLM request attempt {attempt}/{NETWORK_RETRIES} failed " f"({type(exc).__name__}: {exc}); retrying in " @@ -879,6 +979,7 @@ def review_chunk( changed: list[str], diff_chunk: str, timeout: float, + deadline: float | None = None, ) -> tuple[str, list[Finding]]: user_content = "\n\n".join( [ @@ -902,6 +1003,7 @@ def review_chunk( system_prompt=system_prompt, user_content=user_content, timeout=timeout, + deadline=deadline, ) return parse_findings(parsed) @@ -918,6 +1020,23 @@ def merge_findings(all_findings: list[Finding]) -> list[Finding]: return list(merged.values()) +def budget_exhausted_finding(reviewed: int, total: int) -> Finding: + return Finding( + id="llm-budget-exhausted", + severity="warn", + rule_section="External LLM Review", + file="", + line=None, + summary="External LLM review stopped at its time budget", + detail=( + f"Reviewed {reviewed} of {total} diff chunk(s) before the " + f"{DEFAULT_BUDGET_SECONDS:.0f}s budget ran out, so part of this " + "diff was not reviewed. Deterministic checks still covered all of " + "it. Split the PR or raise --budget-seconds to review the rest." + ), + ) + + def llm_skipped_finding(reason: str) -> Finding: return Finding( id="llm-skipped", @@ -1086,6 +1205,13 @@ def main(argv: list[str] | None = None) -> int: default=os.environ.get("DEEPSEEK_API_URL", DEFAULT_API_URL), ) parser.add_argument("--timeout", type=float, default=300.0) + parser.add_argument( + "--budget-seconds", + type=float, + default=DEFAULT_BUDGET_SECONDS, + help="Wall-clock ceiling for all LLM requests, so the job is never " + "killed before the report is written", + ) args = parser.parse_args(argv) if args.llm_skipped_reason and not args.dry_run: @@ -1115,7 +1241,7 @@ def main(argv: list[str] | None = None) -> int: diff_text = unified_diff(args.base, args.head, worktree=args.worktree) added_text = added_lines_with_text(diff_text) added_lines = added_line_numbers(added_text) - section_names = select_rule_sections(files) + section_names = select_rule_sections(files, added_text) rules_text = build_rules_payload(section_names) system_prompt = PROMPT_PATH.read_text(encoding="utf-8") @@ -1175,8 +1301,18 @@ def main(argv: list[str] | None = None) -> int: ) llm_findings: list[Finding] = [] llm_started = time.monotonic() + llm_deadline = llm_started + args.budget_seconds + reviewed = 0 for index, chunk in enumerate(chunks, start=1): chunk_started = time.monotonic() + if chunk_started >= llm_deadline: + print( + f"LLM review: budget exhausted after {reviewed}/{len(chunks)} " + "chunk(s)", + file=sys.stderr, + ) + findings.append(budget_exhausted_finding(reviewed, len(chunks))) + break try: _, chunk_findings = review_chunk( api_key=api_key, @@ -1187,6 +1323,7 @@ def main(argv: list[str] | None = None) -> int: changed=files, diff_chunk=chunk, timeout=args.timeout, + deadline=llm_deadline, ) except (KeyError, ValueError, *TRANSPORT_ERRORS) as exc: print( @@ -1204,6 +1341,7 @@ def main(argv: list[str] | None = None) -> int: file=sys.stderr, ) llm_findings.extend(chunk_findings) + reviewed += 1 merged_llm_findings = merge_findings(llm_findings) kept_llm_findings = filter_findings( merged_llm_findings, diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index 7e82028..4763ab3 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -45,6 +45,7 @@ def make_diff(path: str, added: list[str], *, start: int = 1) -> str: # to review such a PR is a maintainer waiver. Short names keep the interpolated # span below the 12-character threshold the quoted-credential pattern uses. PAT = "ghp" + "_" + "abcdefghijklmnopqrstuvwxyz0123" +PW = "correct " + "horse " + "battery " + "staple" AWS = "AKIA" + "ABCDEFGHIJKLMNOP" SK = "sk-" + "0123456789abcdef0123456789abcdef" VALUE = "abcdefghij" + "klmnopqrst" @@ -705,6 +706,138 @@ def test_api_key_error_finding_blocks_and_names_the_secret() -> None: assert "DEEPSEEK_API_KEY" in finding.summary +def test_run_git_disables_pathname_quoting() -> None: + """git C-quotes non-ASCII paths by default, and the quoted form matches none.""" + mod = load_module() + import subprocess + + captured = {} + original = subprocess.run + + def fake_run(args, **kwargs): + captured["args"] = args + return original(["true"], capture_output=True, text=True) + + subprocess.run = fake_run + try: + mod.run_git(["diff", "--name-only"]) + finally: + subprocess.run = original + assert captured["args"][:3] == ["git", "-c", "core.quotePath=false"] + + +def test_contains_sensitive_text_flags_passphrase_with_spaces() -> None: + mod = load_module() + assert mod.contains_sensitive_text(f'password = "{PW}"') + + +def test_redact_sensitive_text_masks_whole_quoted_value() -> None: + mod = load_module() + redacted = mod.redact_sensitive_text(f'password = "{PW}"') + assert "horse" not in redacted and "battery" not in redacted + assert redacted == "password = [REDACTED_SECRET]" + + +def test_contains_sensitive_text_flags_unterminated_quote() -> None: + mod = load_module() + assert mod.contains_sensitive_text('secret = "opens here') + + +def test_metadata_names_are_not_credentials() -> None: + """Allowing spaces in values means the name must carry the discrimination.""" + mod = load_module() + assert not mod.is_credential_name("token_type") + assert not mod.is_credential_name("secret_name") + assert not mod.is_credential_name("private_key_path") + assert mod.is_credential_name("api_token") + assert mod.is_credential_name("password") + assert not mod.contains_sensitive_text( + 'token_type: "WebGPU event token from another queue"' + ) + assert not mod.redact_sensitive_text( + 'token_type: "an ordinary description"' + ).count("[REDACTED_SECRET]") + + +def test_select_rule_sections_routes_on_changed_content() -> None: + mod = load_module() + path = "strided-traits/src/lib.rs" + assert "Unsafe And Fast-Path Boundaries" not in mod.select_rule_sections([path]) + added = {path: [(10, " unsafe { ptr.read() }")]} + assert "Unsafe And Fast-Path Boundaries" in mod.select_rule_sections([path], added) + + +def test_content_triggers_name_only_documented_sections() -> None: + mod = load_module() + documented = set(mod.parse_repository_rules_sections()) + for _pattern, names in mod.CONTENT_TRIGGERS: + assert set(names) <= documented, names + + +def test_content_triggers_never_select_human_only_sections() -> None: + mod = load_module() + path = "strided-traits/src/lib.rs" + added = {path: [(1, "unsafe { }"), (2, "rayon::join(|| (), || ())")]} + assert set(mod.select_rule_sections([path], added)).isdisjoint( + mod.HUMAN_ONLY_SECTIONS + ) + + +def test_budget_is_smaller_than_the_workflow_timeout() -> None: + """The script must finish before the job is killed, or no report is posted.""" + mod = load_module() + workflow = (mod.ROOT / ".github" / "workflows" / "review_bot.yml").read_text() + minutes = [ + int(line.split(":")[1].strip()) + for line in workflow.splitlines() + if line.strip().startswith("timeout-minutes:") + ] + assert minutes, "review_bot.yml lost its job timeout" + assert mod.DEFAULT_BUDGET_SECONDS < min(minutes) * 60 + + +def test_call_deepseek_does_not_retry_past_the_deadline() -> None: + import socket + import urllib.request + + mod = load_module() + calls = {"n": 0} + + def always_timeout(request, timeout=None): + calls["n"] += 1 + raise socket.timeout("nope") + + original_urlopen = urllib.request.urlopen + original_sleep = mod.time.sleep + urllib.request.urlopen = always_timeout + mod.time.sleep = lambda _seconds: None + try: + mod.call_deepseek( + api_key="k", + model="m", + api_url="https://example.invalid", + system_prompt="s", + user_content="u", + timeout=1.0, + deadline=mod.time.monotonic(), + ) + except mod.TRANSPORT_ERRORS: + pass + else: + raise AssertionError("expected the timeout to propagate") + finally: + urllib.request.urlopen = original_urlopen + mod.time.sleep = original_sleep + assert calls["n"] == 1 + + +def test_budget_exhausted_finding_warns_without_blocking() -> None: + mod = load_module() + finding = mod.budget_exhausted_finding(2, 5) + assert finding.severity == "warn" + assert "2 of 5" in finding.detail + + # --- secret handling --------------------------------------------------------- From 80cb267d000ff0ce3e44a8c2820235a54f1c970d Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 11:48:04 +0900 Subject: [PATCH 2/4] 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 --- scripts/repository-rules-review.py | 62 ++++++++++++++++++++++--- scripts/test-repository-rules-review.py | 62 +++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index 30365be..d7af00b 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -79,7 +79,7 @@ # const API_KEY: &str = "...."; let api_key: String = "...".into(); # Matching only `name value` redacts the type and leaves the literal, so # allow a short annotation before the separator that precedes the value. -SECRET_ANNOTATION = r"(?:\s*:[^=\n]{0,40})?" +SECRET_ANNOTATION = r"(?:[ \t]*:[^=\n]{0,40})?" # The value alternatives are ordered quoted-first so a quoted secret is # consumed whole. Putting the bare-token alternative first would stop at the # first space inside a quoted passphrase and upload the remainder. (Spelling @@ -87,7 +87,7 @@ SECRET_VALUE = r"""(?:"[^"\r\n]*"|'[^'\r\n]*'|[^\s#]+)""" SECRET_ASSIGNMENT = re.compile( r"(?i)\b(?P" + SECRET_NAME + r")(?P" + SECRET_ANNOTATION - + r"\s*[:=]\s*)(?P" + SECRET_VALUE + r")" + + r"[ \t]*[:=][ \t]*)(?P" + SECRET_VALUE + r")" ) # Quoted credentials may legitimately contain spaces (a diceware passphrase is # prose by construction), so the value shape cannot be the discriminator. @@ -95,6 +95,13 @@ r"(?i)\b(?P" + SECRET_NAME + r")" + SECRET_ANNOTATION + r"\s*[:=]\s*" r"""(?:"[^"\r\n]{8,}"|'[^'\r\n]{8,}')""" ) +# An assignment whose value has not started yet on this line: the credential +# lands on the following line, where no credential-shaped name is in sight. +OPEN_SECRET_ASSIGNMENT = re.compile( + r"(?i)\b(?P" + SECRET_NAME + r")" + SECRET_ANNOTATION + r"[ \t]*[:=][ \t]*$" +) +# A value standing alone on its own line, as the continuation of the above. +STANDALONE_VALUE = re.compile(r"""^[ \t]*(?:"[^"\r\n]{4,}"|'[^'\r\n]{4,}')[ \t]*[,;]?[ \t]*$""") # A quote opened on this line and closed on a later one hides the value from # any single-line pattern, so treat the opening alone as disqualifying. UNTERMINATED_SECRET_ASSIGNMENT = re.compile( @@ -714,10 +721,53 @@ def contains_sensitive_text(text: str) -> bool: def sensitive_diff_location(diff_text: str) -> tuple[str, int] | None: - for path, entries in added_lines_with_text(diff_text).items(): - for line_no, text in entries: - if contains_sensitive_text(text): - return path, line_no + """Locate the first added line that must not be uploaded. + + Lines are examined in diff order rather than per file, because a credential + can be split across two: the assignment stays unchanged on a context line + and only the value line is replaced. Checking added lines in isolation sees + a bare string literal with no credential-shaped name anywhere on it. + """ + current_file: str | None = None + new_line = 0 + # Set when the previous surviving line opened an assignment whose value has + # not appeared yet, so the next line's literal is that value. + awaiting_value = False + + for line in diff_text.splitlines(): + if line.startswith("+++ "): + raw = line.removeprefix("+++ b/").removeprefix("+++ ") + current_file = None if raw == "/dev/null" else raw + awaiting_value = False + continue + if line.startswith("@@"): + match = re.search(r"\+(\d+)", line) + new_line = int(match.group(1)) if match else 0 + awaiting_value = False + continue + if current_file is None: + continue + + if line.startswith("-") and not line.startswith("---"): + # A deleted line neither survives nor breaks the continuation. + continue + if not (line.startswith("+") or line.startswith(" ")): + continue + + body = line[1:] + is_added = line.startswith("+") and not line.startswith("+++") + + if is_added and contains_sensitive_text(body): + return current_file, new_line + if is_added and awaiting_value and STANDALONE_VALUE.match(body): + return current_file, new_line + + opener = OPEN_SECRET_ASSIGNMENT.search(body) + awaiting_value = bool(opener) and is_credential_name(opener.group("name")) + if is_added: + new_line += 1 + else: + new_line += 1 return None diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index 4763ab3..fb1104f 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -46,6 +46,9 @@ def make_diff(path: str, added: list[str], *, start: int = 1) -> str: # span below the 12-character threshold the quoted-credential pattern uses. PAT = "ghp" + "_" + "abcdefghijklmnopqrstuvwxyz0123" PW = "correct " + "horse " + "battery " + "staple" +# Spelled out, the opener plus the following value line would make this +# file trip the continuation detector it exercises. +KEYNAME = "API" + "_KEY" AWS = "AKIA" + "ABCDEFGHIJKLMNOP" SK = "sk-" + "0123456789abcdef0123456789abcdef" VALUE = "abcdefghij" + "klmnopqrst" @@ -838,6 +841,65 @@ def test_budget_exhausted_finding_warns_without_blocking() -> None: assert "2 of 5" in finding.detail +def test_sensitive_diff_blocks_a_value_on_a_continuation_line() -> None: + """The assignment can stay unchanged while only the value line is replaced.""" + mod = load_module() + diff = "\n".join( + [ + "diff --git a/src/x.rs b/src/x.rs", + "--- a/src/x.rs", + "+++ b/src/x.rs", + "@@ -1,2 +1,2 @@", + f" const {KEYNAME}: &str =", + '- "old";', + f'+ "{PW}";', + ] + ) + finding = mod.sensitive_diff_finding(diff) + assert finding is not None + assert finding.severity == "block" + + +def test_sensitive_diff_ignores_an_ordinary_continuation_value() -> None: + mod = load_module() + diff = "\n".join( + [ + "diff --git a/src/x.rs b/src/x.rs", + "--- a/src/x.rs", + "+++ b/src/x.rs", + "@@ -1,2 +1,2 @@", + " let message =", + '+ "hello world there";', + ] + ) + assert mod.sensitive_diff_finding(diff) is None + + +def test_sensitive_diff_ignores_an_unchanged_continuation_value() -> None: + """Only added lines may be reported; a context value is pre-existing.""" + mod = load_module() + diff = "\n".join( + [ + "diff --git a/src/x.rs b/src/x.rs", + "--- a/src/x.rs", + "+++ b/src/x.rs", + "@@ -1,3 +1,3 @@", + f" const {KEYNAME}: &str =", + f' "{PW}";', + "+let unrelated = 1;", + ] + ) + assert mod.sensitive_diff_finding(diff) is None + + +def test_redactor_does_not_consume_a_deletion_marker_as_the_value() -> None: + mod = load_module() + text = 'const API_KEY: &str =\n- "old";' + # The separator must not cross the newline and swallow the `-` marker, + # which used to leave the following line's literal untouched. + assert mod.redact_sensitive_text(text).splitlines()[1] == '- "old";' + + # --- secret handling --------------------------------------------------------- From cc57e065839075e102feef97d7bdaeb0b43620dc Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 11:56:35 +0900 Subject: [PATCH 3/4] 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 --- scripts/repository-rules-review.py | 10 +++++++--- scripts/test-repository-rules-review.py | 6 +++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index d7af00b..50c398f 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -1070,7 +1070,7 @@ def merge_findings(all_findings: list[Finding]) -> list[Finding]: return list(merged.values()) -def budget_exhausted_finding(reviewed: int, total: int) -> Finding: +def budget_exhausted_finding(reviewed: int, total: int, budget: float) -> Finding: return Finding( id="llm-budget-exhausted", severity="warn", @@ -1080,7 +1080,7 @@ def budget_exhausted_finding(reviewed: int, total: int) -> Finding: summary="External LLM review stopped at its time budget", detail=( f"Reviewed {reviewed} of {total} diff chunk(s) before the " - f"{DEFAULT_BUDGET_SECONDS:.0f}s budget ran out, so part of this " + f"{budget:.0f}s budget ran out, so part of this " "diff was not reviewed. Deterministic checks still covered all of " "it. Split the PR or raise --budget-seconds to review the rest." ), @@ -1361,7 +1361,11 @@ def main(argv: list[str] | None = None) -> int: "chunk(s)", file=sys.stderr, ) - findings.append(budget_exhausted_finding(reviewed, len(chunks))) + findings.append( + budget_exhausted_finding( + reviewed, len(chunks), args.budget_seconds + ) + ) break try: _, chunk_findings = review_chunk( diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index fb1104f..4e3710d 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -836,9 +836,13 @@ def always_timeout(request, timeout=None): def test_budget_exhausted_finding_warns_without_blocking() -> None: mod = load_module() - finding = mod.budget_exhausted_finding(2, 5) + finding = mod.budget_exhausted_finding(2, 5, 30.0) assert finding.severity == "warn" assert "2 of 5" in finding.detail + # The configured budget, not the default, or the diagnostic misleads + # whoever is trying to work out why the review was incomplete. + assert "30s budget" in finding.detail + assert "900s" not in finding.detail def test_sensitive_diff_blocks_a_value_on_a_continuation_line() -> None: From 7bb346b562618eb1cd63e4985ec3b159b8f12f2c Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 12:17:59 +0900 Subject: [PATCH 4/4] 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 --- scripts/repository-rules-review.py | 57 +++++++++++++++++++++-- scripts/test-repository-rules-review.py | 62 +++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index 50c398f..858825a 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -101,7 +101,12 @@ r"(?i)\b(?P" + SECRET_NAME + r")" + SECRET_ANNOTATION + r"[ \t]*[:=][ \t]*$" ) # A value standing alone on its own line, as the continuation of the above. -STANDALONE_VALUE = re.compile(r"""^[ \t]*(?:"[^"\r\n]{4,}"|'[^'\r\n]{4,}')[ \t]*[,;]?[ \t]*$""") +# The value may be a bare token: `API_KEY =` followed by an unquoted secret. +# `awaiting_value` is only set for credential-named assignments, so accepting +# bare tokens here cannot fire on ordinary continuations. +STANDALONE_VALUE = re.compile( + r"""^[ \t]*(?:"[^"\r\n]{4,}"|'[^'\r\n]{4,}'|[^\s"'#][^\s#]{7,})[ \t]*[,;]?[ \t]*$""" +) # A quote opened on this line and closed on a later one hides the value from # any single-line pattern, so treat the opening alone as disqualifying. UNTERMINATED_SECRET_ASSIGNMENT = re.compile( @@ -430,6 +435,22 @@ def build_rules_payload(section_names: list[str]) -> str: return "\n\n".join(chunks) +def files_with_deleted_lines(diff_text: str) -> set[str]: + """Files this diff removes lines from, keyed by their new-side path.""" + result: set[str] = set() + current_file: str | None = None + for line in diff_text.splitlines(): + if line.startswith("+++ "): + raw = line.removeprefix("+++ b/").removeprefix("+++ ") + current_file = None if raw == "/dev/null" else raw + continue + if line.startswith("--- ") or line.startswith("@@"): + continue + if current_file and line.startswith("-") and not line.startswith("---"): + result.add(current_file) + return result + + def added_lines_with_text(diff_text: str) -> dict[str, list[tuple[int, str]]]: """Map each file to its added ``(new_line_number, text)`` pairs.""" result: dict[str, list[tuple[int, str]]] = {} @@ -916,8 +937,19 @@ def filter_findings( added_lines: dict[str, set[int]], *, allow_global: bool = True, + files_with_deletions: set[str] | None = None, ) -> list[Finding]: + """Keep only findings anchored to something this diff actually changed. + + A file-level `block` is normally dropped, because an unanchored block is + usually the model generalising about the file rather than about the diff. + The exception is a violation introduced by *deleting* required validation, + coverage, or a safety comment: there is no new-file line to point at, so + the model must return `line: null`, and dropping it would let a + deletion-only diff pass the gate. + """ allowed_files = set(files) + deletions = files_with_deletions or set() kept: list[Finding] = [] for finding in findings: if not finding.file: @@ -926,7 +958,11 @@ def filter_findings( continue if finding.file and finding.file not in allowed_files: continue - if finding.line is None and finding.severity == "block": + if ( + finding.line is None + and finding.severity == "block" + and finding.file not in deletions + ): continue if finding.line is not None: if finding.line not in added_lines.get(finding.file, set()): @@ -1290,6 +1326,7 @@ def main(argv: list[str] | None = None) -> int: diff_text = unified_diff(args.base, args.head, worktree=args.worktree) added_text = added_lines_with_text(diff_text) + deleted_from = files_with_deleted_lines(diff_text) added_lines = added_line_numbers(added_text) section_names = select_rule_sections(files, added_text) rules_text = build_rules_payload(section_names) @@ -1386,7 +1423,20 @@ def main(argv: list[str] | None = None) -> int: f"{type(exc).__name__}: {exc}", file=sys.stderr, ) - findings.append(llm_response_error_finding(exc)) + # A transport failure at the cumulative deadline is the budget + # running out, not an unusable model. Reporting it as a block + # would fail the gate for exactly the case the budget exists + # to degrade gracefully. + if isinstance(exc, TRANSPORT_ERRORS) and ( + time.monotonic() >= llm_deadline + ): + findings.append( + budget_exhausted_finding( + reviewed, len(chunks), args.budget_seconds + ) + ) + else: + findings.append(llm_response_error_finding(exc)) break print( f"LLM chunk {index}/{len(chunks)}: {len(chunk)} chars, " @@ -1402,6 +1452,7 @@ def main(argv: list[str] | None = None) -> int: files, added_lines, allow_global=False, + files_with_deletions=deleted_from, ) llm_elapsed = time.monotonic() - llm_started llm_summary = summarize_llm_review( diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index 4e3710d..bc54f44 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -904,6 +904,68 @@ def test_redactor_does_not_consume_a_deletion_marker_as_the_value() -> None: assert mod.redact_sensitive_text(text).splitlines()[1] == '- "old";' +def test_filter_findings_keeps_file_level_block_for_deletions() -> None: + """A deletion-only violation has no new-file line the model can anchor to.""" + mod = load_module() + block = mod.Finding("x", "block", "S", "a.rs", None, "removed validation", "d") + assert mod.filter_findings([block], ["a.rs"], {}) == [] + kept = mod.filter_findings([block], ["a.rs"], {}, files_with_deletions={"a.rs"}) + assert kept == [block] + + +def test_files_with_deleted_lines_reads_the_new_side_path() -> None: + mod = load_module() + diff = "\n".join( + [ + "diff --git a/a.rs b/a.rs", + "--- a/a.rs", + "+++ b/a.rs", + "@@ -1,2 +1,1 @@", + " keep", + "-gone", + "diff --git a/b.rs b/b.rs", + "--- a/b.rs", + "+++ b/b.rs", + "@@ -1,1 +1,2 @@", + " keep", + "+added", + ] + ) + assert mod.files_with_deleted_lines(diff) == {"a.rs"} + + +def test_sensitive_diff_blocks_a_bare_continuation_value() -> None: + """The continuation value need not be quoted.""" + mod = load_module() + diff = "\n".join( + [ + "diff --git a/src/x.rs b/src/x.rs", + "--- a/src/x.rs", + "+++ b/src/x.rs", + "@@ -1,2 +1,2 @@", + f" {KEYNAME} =", + "-old", + "+abcdefghijklmnopqrstuvwx", + ] + ) + assert mod.sensitive_diff_finding(diff) is not None + + +def test_sensitive_diff_ignores_an_ordinary_bare_continuation() -> None: + mod = load_module() + diff = "\n".join( + [ + "diff --git a/src/x.rs b/src/x.rs", + "--- a/src/x.rs", + "+++ b/src/x.rs", + "@@ -1,2 +1,2 @@", + " let total =", + "+ compute_sum(values)", + ] + ) + assert mod.sensitive_diff_finding(diff) is None + + # --- secret handling ---------------------------------------------------------