From 848be2337188ad7d666827912ea8807e02148e65 Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 10:47:07 +0900 Subject: [PATCH 1/3] 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 --- scripts/repository-rules-review.py | 33 +++++++++-- scripts/test-repository-rules-review.py | 76 +++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index b02f04d..cb1f5d7 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -36,7 +36,7 @@ PROMPT_VERSION = "1" DEFAULT_MODEL = "deepseek-v4-pro" DEFAULT_API_URL = "https://api.deepseek.com/chat/completions" -MAX_DIFF_CHARS = 120_000 +MAX_DIFF_CHARS = 60_000 MAX_FILE_DIFF_CHARS = 40_000 MAX_FINDINGS_PER_CHUNK = 8 @@ -638,6 +638,10 @@ def reconcile_verdict(findings: list[Finding]) -> str: return "fail" if any(item.severity == "block" for item in findings) else "pass" +NETWORK_RETRIES = 2 +RETRY_BACKOFF_SECONDS = 5.0 + + def call_deepseek( *, api_key: str, @@ -665,8 +669,27 @@ def call_deepseek( }, method="POST", ) - with urllib.request.urlopen(request, timeout=timeout) as response: - body = json.loads(response.read().decode("utf-8")) + last_error: OSError | None = None + for attempt in range(1, NETWORK_RETRIES + 1): + try: + with urllib.request.urlopen(request, timeout=timeout) as response: + body = json.loads(response.read().decode("utf-8")) + break + except OSError as exc: + # Timeouts and connection resets are common enough on large diffs + # that one blocked PR per blip is not an acceptable failure mode. + last_error = exc + if attempt == NETWORK_RETRIES: + raise + print( + f"LLM request attempt {attempt}/{NETWORK_RETRIES} failed " + f"({type(exc).__name__}: {exc}); retrying in " + f"{RETRY_BACKOFF_SECONDS:.0f}s", + file=sys.stderr, + ) + time.sleep(RETRY_BACKOFF_SECONDS) + else: # pragma: no cover - the loop either breaks or raises + raise last_error if last_error else RuntimeError("no response") content = body["choices"][0]["message"]["content"] return extract_json_payload(content) @@ -887,7 +910,7 @@ def main(argv: list[str] | None = None) -> int: "--api-url", default=os.environ.get("DEEPSEEK_API_URL", DEFAULT_API_URL), ) - parser.add_argument("--timeout", type=float, default=120.0) + parser.add_argument("--timeout", type=float, default=300.0) args = parser.parse_args(argv) if args.llm_skipped_reason and not args.dry_run: @@ -984,7 +1007,7 @@ def main(argv: list[str] | None = None) -> int: diff_chunk=chunk, timeout=args.timeout, ) - except (KeyError, ValueError, urllib.error.URLError, TimeoutError) as exc: + except (KeyError, ValueError, OSError) as exc: print( f"LLM chunk {index}/{len(chunks)}: failed after " f"{time.monotonic() - chunk_started:.1f}s: " diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index 7d4a52b..4d7297e 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -484,6 +484,82 @@ def test_split_large_file_diff_splits_single_overlong_line() -> None: assert len(chunk) <= mod.MAX_FILE_DIFF_CHARS +def test_call_deepseek_retries_transient_network_errors() -> None: + """socket.timeout is only a TimeoutError alias from Python 3.10 on.""" + import socket + import urllib.request + + mod = load_module() + calls = {"n": 0} + + class FakeResponse: + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def read(self): + return b'{"choices":[{"message":{"content":"{\\"verdict\\":\\"pass\\"}"}}]}' + + def fake_urlopen(request, timeout=None): + calls["n"] += 1 + if calls["n"] == 1: + raise socket.timeout("The read operation timed out") + return FakeResponse() + + original_urlopen = urllib.request.urlopen + original_sleep = mod.time.sleep + urllib.request.urlopen = fake_urlopen + mod.time.sleep = lambda _seconds: None + try: + payload = mod.call_deepseek( + api_key="k", + model="m", + api_url="https://example.invalid", + system_prompt="s", + user_content="u", + timeout=1.0, + ) + finally: + urllib.request.urlopen = original_urlopen + mod.time.sleep = original_sleep + + assert calls["n"] == 2 + assert payload == {"verdict": "pass"} + + +def test_call_deepseek_reraises_after_retries_exhausted() -> None: + import socket + import urllib.request + + mod = load_module() + + def always_timeout(request, timeout=None): + 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, + ) + except OSError: + pass + else: + raise AssertionError("expected the timeout to propagate") + finally: + urllib.request.urlopen = original_urlopen + mod.time.sleep = original_sleep + + # --- secret handling --------------------------------------------------------- From edf7975627f4a7ee538c32f71836be35fc6902b5 Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 10:51:37 +0900 Subject: [PATCH 2/3] 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 --- scripts/repository-rules-review.py | 17 ++++++++++++++--- scripts/test-repository-rules-review.py | 20 ++++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index cb1f5d7..6bda7d4 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -17,6 +17,7 @@ from __future__ import annotations import argparse +import http.client import json import os import re @@ -638,6 +639,16 @@ def reconcile_verdict(findings: list[Finding]) -> str: return "fail" if any(item.severity == "block" for item in findings) else "pass" +# Every way the request can fail below the JSON layer. OSError covers +# socket.timeout, TimeoutError, ConnectionResetError, ssl.SSLError, and +# urllib.error.URLError/HTTPError. http.client.HTTPException is a sibling of +# OSError, not a subclass, so a truncated chunked response (IncompleteRead) +# needs naming separately. socket.timeout only aliases TimeoutError from +# Python 3.10 on, so listing TimeoutError alone is not enough on 3.9. +TRANSPORT_ERRORS: tuple[type[BaseException], ...] = ( + OSError, + http.client.HTTPException, +) NETWORK_RETRIES = 2 RETRY_BACKOFF_SECONDS = 5.0 @@ -669,13 +680,13 @@ def call_deepseek( }, method="POST", ) - last_error: OSError | None = None + last_error: BaseException | None = None for attempt in range(1, NETWORK_RETRIES + 1): try: with urllib.request.urlopen(request, timeout=timeout) as response: body = json.loads(response.read().decode("utf-8")) break - except OSError as exc: + except TRANSPORT_ERRORS as exc: # Timeouts and connection resets are common enough on large diffs # that one blocked PR per blip is not an acceptable failure mode. last_error = exc @@ -1007,7 +1018,7 @@ def main(argv: list[str] | None = None) -> int: diff_chunk=chunk, timeout=args.timeout, ) - except (KeyError, ValueError, OSError) as exc: + except (KeyError, ValueError, *TRANSPORT_ERRORS) as exc: print( f"LLM chunk {index}/{len(chunks)}: failed after " f"{time.monotonic() - chunk_started:.1f}s: " diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index 4d7297e..ae32c43 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -484,8 +484,24 @@ def test_split_large_file_diff_splits_single_overlong_line() -> None: assert len(chunk) <= mod.MAX_FILE_DIFF_CHARS +def test_transport_errors_cover_every_below_json_failure() -> None: + """socket.timeout only aliases TimeoutError from Python 3.10 on.""" + import http.client + import socket + import urllib.error + + mod = load_module() + for exc_type in ( + socket.timeout, + TimeoutError, + ConnectionResetError, + urllib.error.URLError, + http.client.IncompleteRead, + ): + assert issubclass(exc_type, mod.TRANSPORT_ERRORS), exc_type + + def test_call_deepseek_retries_transient_network_errors() -> None: - """socket.timeout is only a TimeoutError alias from Python 3.10 on.""" import socket import urllib.request @@ -551,7 +567,7 @@ def always_timeout(request, timeout=None): user_content="u", timeout=1.0, ) - except OSError: + except mod.TRANSPORT_ERRORS: pass else: raise AssertionError("expected the timeout to propagate") From ebe440e1f369febad618a2a64be291a05d29e3dc Mon Sep 17 00:00:00 2001 From: Satoshi Terasaki Date: Tue, 4 Aug 2026 11:02:02 +0900 Subject: [PATCH 3/3] 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 Refs #199 --- scripts/repository-rules-review.py | 228 +++++++++++++++++++++--- scripts/test-repository-rules-review.py | 148 +++++++++++++-- 2 files changed, 334 insertions(+), 42 deletions(-) diff --git a/scripts/repository-rules-review.py b/scripts/repository-rules-review.py index 6bda7d4..ee7fe41 100644 --- a/scripts/repository-rules-review.py +++ b/scripts/repository-rules-review.py @@ -40,6 +40,9 @@ MAX_DIFF_CHARS = 60_000 MAX_FILE_DIFF_CHARS = 40_000 MAX_FINDINGS_PER_CHUNK = 8 +HUNK_HEADER = re.compile( + r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)$" +) # Crates retired by #199. Contraction moves to tenferro; strided-rs narrows to # the affine strided primitive layer. @@ -68,19 +71,21 @@ re.compile(r"(?i)\bAuthorization:\s*Bearer\s+[A-Za-z0-9._~+/=-]{16,}"), re.compile(r"-----BEGIN [A-Z ]*PRIVATE KEY-----"), ) -SECRET_ASSIGNMENT = re.compile( - r"(?i)\b(" +SECRET_NAME = ( r"[\w.-]*(?:api[_-]?key|token|secret|password|passwd|pwd|client[_-]?secret|" r"private[_-]?key)[\w.-]*" - r"\s*[:=]\s*)" - r"([^\s#]+)" +) +# A typed declaration puts the type between the name and the value: +# 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_ASSIGNMENT = re.compile( + r"(?i)\b(" + SECRET_NAME + SECRET_ANNOTATION + r"\s*[:=]\s*)([^\s#]+)" ) QUOTED_SECRET_ASSIGNMENT = re.compile( - r"(?i)\b" - r"[\w.-]*(?:api[_-]?key|token|secret|password|passwd|pwd|client[_-]?secret|" - r"private[_-]?key)[\w.-]*" - r"\s*[:=]\s*" - r'''(?:"[^\s"\r\n]{12,}"|'[^\s'\r\n]{12,}')''' + r"(?i)\b" + SECRET_NAME + SECRET_ANNOTATION + r"\s*[:=]\s*" + r'''(?:"[^\s"\r\n]{12,}"|\'[^\s\'\r\n]{12,}\')''' ) SEVERITY_ALIASES = { "block": "block", @@ -203,28 +208,53 @@ def to_dict(self) -> dict[str, Any]: } -def run_git(args: list[str], cwd: Path = ROOT) -> str: +def run_git(args: list[str], cwd: Path = ROOT, *, check: bool = True) -> str: completed = subprocess.run( ["git", *args], cwd=cwd, - check=True, + check=check, capture_output=True, text=True, ) return completed.stdout +def untracked_files() -> list[str]: + """Paths git does not track yet, honouring .gitignore.""" + output = run_git(["ls-files", "--others", "--exclude-standard"]) + return [line.strip() for line in output.splitlines() if line.strip()] + + +def untracked_file_diff(path: str) -> str: + """Synthesize an all-added diff for a file with no committed counterpart. + + `git diff ` compares the working tree against a commit, so an + untracked path has no object to compare and is omitted entirely. In + worktree mode -- the documented local preview -- that means a brand new + file is reviewed by nothing at all and the preview reports a false pass. + `--no-index` against /dev/null gives a normal diff without touching the + index; it exits 1 because the inputs differ, which is not an error here. + """ + return run_git( + ["diff", "--unified=3", "--no-index", "--", "/dev/null", path], + check=False, + ) + + def changed_files(base: str, head: str, *, worktree: bool = False) -> list[str]: if worktree: output = run_git(["diff", "--name-only", base]) - else: - output = run_git(["diff", "--name-only", f"{base}...{head}"]) + tracked = [line.strip() for line in output.splitlines() if line.strip()] + return sorted({*tracked, *untracked_files()}) + output = run_git(["diff", "--name-only", f"{base}...{head}"]) return [line.strip() for line in output.splitlines() if line.strip()] def unified_diff(base: str, head: str, *, worktree: bool = False) -> str: if worktree: - return run_git(["diff", "--unified=3", base]) + pieces = [run_git(["diff", "--unified=3", base])] + pieces.extend(untracked_file_diff(path) for path in untracked_files()) + return "\n".join(piece for piece in pieces if piece.strip()) return run_git(["diff", "--unified=3", f"{base}...{head}"]) @@ -236,8 +266,11 @@ def per_file_diffs( worktree: bool = False, ) -> dict[str, str]: diffs: dict[str, str] = {} + untracked = set(untracked_files()) if worktree else set() for path in files: - if worktree: + if path in untracked: + diff = untracked_file_diff(path) + elif worktree: diff = run_git(["diff", "--unified=3", base, "--", path]) else: diff = run_git(["diff", "--unified=3", f"{base}...{head}", "--", path]) @@ -382,6 +415,23 @@ def joined_line_len(lines: list[str]) -> int: return len("\n".join(lines)) +def line_deltas(line: str) -> tuple[int, int]: + """How one diff body line advances the old and new file cursors.""" + if line.startswith("+"): + return (0, 1) + if line.startswith("-"): + return (1, 0) + if line.startswith("\\"): + return (0, 0) + return (1, 1) + + +def format_hunk_header( + old_start: int, old_count: int, new_start: int, new_count: int, suffix: str +) -> str: + return f"@@ -{old_start},{old_count} +{new_start},{new_count} @@{suffix}" + + def split_overlong_diff_line(prefix: list[str], line: str) -> list[str]: prefix_len = joined_line_len(prefix) line_budget = MAX_FILE_DIFF_CHARS - prefix_len - 1 @@ -399,17 +449,88 @@ def split_overlong_diff_line(prefix: list[str], line: str) -> list[str]: def split_oversized_hunk(header: list[str], hunk: list[str]) -> list[str]: - """Split one oversized hunk while repeating file and hunk headers.""" + """Split one oversized hunk, rewriting the header for every emitted chunk. + + Repeating the original header would tell the model that each chunk starts + at the hunk's first line, so findings in later chunks come back with line + numbers thousands of lines too small. `filter_findings` then drops them, or + worse keeps them against an unrelated added line that happens to collide. + """ if not hunk: return [] + parsed = HUNK_HEADER.match(hunk[0]) + if parsed is None: + # Not a header we can renumber; fall back to repeating it verbatim + # rather than inventing offsets. + return split_oversized_hunk_verbatim(header, hunk) + + old_start = int(parsed.group(1)) + new_start = int(parsed.group(3)) + suffix = parsed.group(5) + + chunks: list[str] = [] + body: list[str] = [] + body_old = body_new = 0 + cursor_old, cursor_new = old_start, new_start + + def flush() -> None: + nonlocal body, body_old, body_new, cursor_old, cursor_new + if not body: + return + hunk_header = format_hunk_header( + cursor_old, body_old, cursor_new, body_new, suffix + ) + chunks.append("\n".join([*header, hunk_header, *body])) + cursor_old += body_old + cursor_new += body_new + body = [] + body_old = body_new = 0 + + for line in hunk[1:]: + delta_old, delta_new = line_deltas(line) + single_header = format_hunk_header( + cursor_old + body_old, delta_old, cursor_new + body_new, delta_new, suffix + ) + if joined_line_len([*header, single_header, line]) > MAX_FILE_DIFF_CHARS: + flush() + single_header = format_hunk_header( + cursor_old, delta_old, cursor_new, delta_new, suffix + ) + chunks.extend( + split_overlong_diff_line([*header, single_header], line) + ) + cursor_old += delta_old + cursor_new += delta_new + continue + + candidate_header = format_hunk_header( + cursor_old, body_old + delta_old, cursor_new, body_new + delta_new, suffix + ) + if ( + body + and joined_line_len([*header, candidate_header, *body, line]) + > MAX_FILE_DIFF_CHARS + ): + flush() + body.append(line) + body_old += delta_old + body_new += delta_new + + flush() + if not chunks: + chunks.append("\n".join([*header, hunk[0]])) + return chunks + + +def split_oversized_hunk_verbatim(header: list[str], hunk: list[str]) -> list[str]: + """Fallback for a hunk header this script cannot parse.""" hunk_header = hunk[0] - body = hunk[1:] prefix = [*header, hunk_header] chunks: list[str] = [] current = list(prefix) - for line in body: + for line in hunk[1:]: if joined_line_len([*prefix, line]) > MAX_FILE_DIFF_CHARS: if current != prefix: chunks.append("\n".join(current)) @@ -602,14 +723,57 @@ def llm_response_error_finding(error: BaseException) -> Finding: rule_section="External LLM Review", file="", line=None, - summary="External LLM review did not produce usable JSON", + summary="External LLM review did not complete", + detail=( + "The repository-rules review could not send the request or parse " + f"the model response: {type(error).__name__}: {error}" + ), + ) + + +def api_key_error_finding(reason: str) -> Finding: + return Finding( + id="llm-api-key-invalid", + severity="block", + rule_section="External LLM Review", + file="", + line=None, + summary="DEEPSEEK_API_KEY is unusable", detail=( - "The repository-rules review could not parse or validate the model " - f"response: {type(error).__name__}: {error}" + f"{reason} Re-set the repository secret; the value itself is never " + "printed. Until then the LLM pass cannot run." ), ) +def api_key_problem(api_key: str) -> str | None: + """Describe why a key cannot be sent, without echoing the key. + + HTTP header values are encoded as latin-1, so a key carrying non-ASCII + text -- a pasted ellipsis from a masked console display, or mojibake -- + raises UnicodeEncodeError before any request leaves the machine. That is a + ValueError subclass, so it used to surface as "did not produce usable + JSON", pointing the reader at the model instead of at the secret. + """ + if not api_key: + return "The secret is empty." + if not api_key.isascii(): + offsets = [ + str(index) + for index, char in enumerate(api_key) + if not char.isascii() + ] + return ( + "The secret contains non-ASCII characters at offset(s) " + f"{', '.join(offsets[:10])}, which cannot be sent in an HTTP " + "Authorization header. A masked value copied from a console " + "display is the usual cause." + ) + if any(char.isspace() for char in api_key): + return "The secret contains whitespace." + return None + + def filter_findings( findings: list[Finding], files: list[str], @@ -985,18 +1149,24 @@ def main(argv: list[str] | None = None) -> int: llm_summary: str | None = None llm_stats: dict[str, Any] | None = None if not args.dry_run and not sensitive_finding: - api_key = os.environ.get("DEEPSEEK_API_KEY") + api_key = os.environ.get("DEEPSEEK_API_KEY", "").strip() if not api_key: print("DEEPSEEK_API_KEY is not set", file=sys.stderr) return 1 + key_problem = api_key_problem(api_key) - file_diffs = per_file_diffs( - args.base, - args.head, - files, - worktree=args.worktree, - ) - chunks = split_diff_chunks(redact_file_diffs(file_diffs)) + if key_problem: + print(f"DEEPSEEK_API_KEY is unusable: {key_problem}", file=sys.stderr) + findings.append(api_key_error_finding(key_problem)) + chunks = [] + else: + file_diffs = per_file_diffs( + args.base, + args.head, + files, + worktree=args.worktree, + ) + chunks = split_diff_chunks(redact_file_diffs(file_diffs)) chunk_sizes = [len(chunk) for chunk in chunks] print( f"LLM review: model {args.model}, {len(chunks)} chunk(s), " diff --git a/scripts/test-repository-rules-review.py b/scripts/test-repository-rules-review.py index ae32c43..7e82028 100644 --- a/scripts/test-repository-rules-review.py +++ b/scripts/test-repository-rules-review.py @@ -39,6 +39,18 @@ def make_diff(path: str, added: list[str], *, start: int = 1) -> str: ) +# Secret-shaped fixtures are assembled at runtime so this file contains no +# contiguous secret-shaped literal of its own. Otherwise the guard under test +# blocks the LLM pass on every PR that touches its own tests, and the only way +# 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" +AWS = "AKIA" + "ABCDEFGHIJKLMNOP" +SK = "sk-" + "0123456789abcdef0123456789abcdef" +VALUE = "abcdefghij" + "klmnopqrst" +BEARER = "Authorization: Bearer " + VALUE + + # --- diff parsing ------------------------------------------------------------ @@ -576,22 +588,132 @@ def always_timeout(request, timeout=None): mod.time.sleep = original_sleep +def test_contains_sensitive_text_flags_typed_declaration() -> None: + """A type annotation used to hide the literal from the pre-upload guard.""" + mod = load_module() + for line in ( + f'const API_KEY: &str = "{VALUE}";', + f'let api_key: String = "{VALUE}".into();', + f'client_secret : &\'static str = "{VALUE}"', + f'PASSWORD: str = "{VALUE}"', + ): + assert mod.contains_sensitive_text(line), line + + +def test_redact_sensitive_text_masks_typed_declaration() -> None: + mod = load_module() + redacted = mod.redact_sensitive_text(f'const API_KEY: &str = "{VALUE}";') + assert VALUE not in redacted + assert "[REDACTED_SECRET]" in redacted + + +def test_typed_declaration_guard_keeps_env_lookups_quiet() -> None: + mod = load_module() + for line in ( + 'let key = std::env::var("DEEPSEEK_API_KEY")?;', + "DEEPSEEK_API_KEY: ${{ secrets.DEEPSEEK_API_KEY }}", + "api_key: Option,", + ): + assert not mod.contains_sensitive_text(line), line + + +# --- hunk header renumbering -------------------------------------------------- + + +def test_split_oversized_hunk_renumbers_each_chunk() -> None: + mod = load_module() + header = ["diff --git a/big.rs b/big.rs", "--- a/big.rs", "+++ b/big.rs"] + body = [f"+line {index}" + "y" * 900 for index in range(120)] + chunks = mod.split_oversized_hunk(header, ["@@ -1,0 +1,120 @@ fn ctx()", *body]) + assert len(chunks) > 1 + + starts = [] + for chunk in chunks: + assert len(chunk) <= mod.MAX_FILE_DIFF_CHARS + hunk_line = [line for line in chunk.splitlines() if line.startswith("@@")][0] + parsed = mod.HUNK_HEADER.match(hunk_line) + assert parsed is not None + assert parsed.group(5) == " fn ctx()" + starts.append((int(parsed.group(3)), int(parsed.group(4)))) + + # Every chunk starts where the previous one ended, and the counts sum to + # the original 120 added lines. + assert starts[0][0] == 1 + for (start, count), (next_start, _) in zip(starts, starts[1:]): + assert start + count == next_start + assert sum(count for _, count in starts) == 120 + + +def test_split_oversized_hunk_counts_context_and_removals() -> None: + mod = load_module() + header = ["diff --git a/a.rs b/a.rs", "--- a/a.rs", "+++ b/a.rs"] + hunk = ["@@ -10,3 +20,3 @@", " ctx", "-gone", "+added"] + chunks = mod.split_oversized_hunk(header, hunk) + assert len(chunks) == 1 + hunk_line = [line for line in chunks[0].splitlines() if line.startswith("@@")][0] + parsed = mod.HUNK_HEADER.match(hunk_line) + # context + removal advance old; context + addition advance new. + assert (int(parsed.group(1)), int(parsed.group(2))) == (10, 2) + assert (int(parsed.group(3)), int(parsed.group(4))) == (20, 2) + + +def test_split_oversized_hunk_falls_back_on_unparseable_header() -> None: + mod = load_module() + header = ["diff --git a/a.rs b/a.rs", "--- a/a.rs", "+++ b/a.rs"] + chunks = mod.split_oversized_hunk(header, ["@@ garbage @@", "+one"]) + assert len(chunks) == 1 + assert "@@ garbage @@" in chunks[0] + + +def test_line_deltas_classifies_diff_lines() -> None: + mod = load_module() + assert mod.line_deltas("+added") == (0, 1) + assert mod.line_deltas("-removed") == (1, 0) + assert mod.line_deltas(" context") == (1, 1) + assert mod.line_deltas("\\ No newline at end of file") == (0, 0) + + +# --- API key validation ------------------------------------------------------- + + +def test_api_key_problem_detects_non_ascii_without_echoing_it() -> None: + mod = load_module() + key = SK[:29] + "\u2026" + "tail" + problem = mod.api_key_problem(key) + assert problem is not None + assert "non-ASCII" in problem + assert "29" in problem + assert key not in problem + + +def test_api_key_problem_detects_empty_and_whitespace() -> None: + mod = load_module() + assert "empty" in mod.api_key_problem("") + assert "whitespace" in mod.api_key_problem("sk-abc def") + + +def test_api_key_problem_accepts_a_normal_key() -> None: + mod = load_module() + assert mod.api_key_problem(SK) is None + + +def test_api_key_error_finding_blocks_and_names_the_secret() -> None: + mod = load_module() + finding = mod.api_key_error_finding("The secret is empty.") + assert finding.severity == "block" + assert finding.id == "llm-api-key-invalid" + assert "DEEPSEEK_API_KEY" in finding.summary + + # --- secret handling --------------------------------------------------------- def test_redact_sensitive_text_masks_common_secret_forms() -> None: mod = load_module() - text = "\n".join( - [ - "ghp_abcdefghijklmnopqrstuvwxyz0123", - "AKIAABCDEFGHIJKLMNOP", - "api_key = supersecretvalue", - "Authorization: Bearer abcdefghijklmnopqrst", - ] - ) + text = "\n".join([PAT, AWS, "api_key = supersecretvalue", BEARER]) redacted = mod.redact_sensitive_text(text) - assert "ghp_abcdefghijklmnopqrstuvwxyz0123" not in redacted - assert "AKIAABCDEFGHIJKLMNOP" not in redacted + assert PAT not in redacted + assert AWS not in redacted assert "supersecretvalue" not in redacted assert redacted.count("[REDACTED_SECRET]") >= 4 @@ -606,7 +728,7 @@ def test_contains_sensitive_text_ignores_env_lookup_code() -> None: def test_contains_sensitive_text_flags_quoted_credential() -> None: mod = load_module() - assert mod.contains_sensitive_text('let api_key = "abcdefghijklmnop";') + assert mod.contains_sensitive_text(f'let api_key = "{VALUE}";') def test_sensitive_diff_finding_checks_added_lines_only() -> None: @@ -617,7 +739,7 @@ def test_sensitive_diff_finding_checks_added_lines_only() -> None: "--- a/a.rs", "+++ b/a.rs", "@@ -1,2 +1,2 @@", - " let token = ghp_abcdefghijklmnopqrstuvwxyz0123;", + f" let token = {PAT};", "+let clean = 1;", ] ) @@ -627,7 +749,7 @@ def test_sensitive_diff_finding_checks_added_lines_only() -> None: def test_sensitive_diff_finding_reports_added_match_location() -> None: mod = load_module() diff = make_diff( - "a.rs", ["let clean = 1;", "let t = ghp_abcdefghijklmnopqrstuvwxyz0123;"] + "a.rs", ["let clean = 1;", f"let t = {PAT};"] ) finding = mod.sensitive_diff_finding(diff) assert finding is not None