fix(autoreview): redact secrets without blocking review - #132
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 30, 2026, 5:41 PM ET / 21:41 UTC. ClawSweeper reviewWhat this changesThis PR substantially expands the Autoreview skill’s secret-redaction parser and hardening tests so credential-like diff content is sanitized while executable source remains usable for review. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 8 items remain Keep this PR open: the reported templated-URL regression remains an actionable P1 blocker on the unchanged head, and the PR still lacks after-fix real validator output. The patch also cannot currently merge cleanly, so it needs a focused repair, refreshed merge state, and proof before landing. Priority: P1 Review scores
Verification
How this fits togetherAutoreview receives a proposed diff, detects credential-like source fragments, rewrites sensitive values to stable placeholders, then sends the sanitized patch to the review workflow. This change sits at the redaction boundary, where false positives can block a review and false negatives can expose sensitive material. flowchart LR
A[Proposed code diff] --> B[Autoreview secret detector]
B --> C[Language-aware parsing]
C --> D[Secret redaction and identity mapping]
D --> E[Patch safety validation]
E --> F[Sanitized review input]
F --> G[Automated code review]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Narrow the known-secret-fragment collection so URL userinfo interpolation slots and bare usernames remain reviewable, retain password redaction, add focused regressions, then provide redacted live validator output for both acceptance and masking cases on a freshly rebased branch. Do we have a high-confidence way to reproduce the issue? Yes, from the supplied current-head probe: pass a one-line added hunk containing Is this the best way to solve the issue? No. The intended redaction goal is appropriate, but registering interpolation syntax as a secret fragment turns valid URL templates into a whole-patch refusal; a narrow exclusion with regression coverage is safer than accepting this behavior. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fe588b1a6267. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
|
@steipete Measured this branch against the case class from #133 (URL userinfo). Two results worth separating: the four over-masking cases are unchanged, and one of them regresses from masking into a hard refusal, which runs against this PR's stated goal of redacting without blocking review. To keep main's drift out of it, both columns are this branch's own history — merge-base
Password redaction is intact on both sides — the last row is the control, and it is the one case in the set that should be redacted. MechanismThe refusal is not a new pattern match. So the interpolation slot text itself — Blast radiusBecause that check is a substring scan over the entire patch, one scheme-bearing f-string poisons every other occurrence of the same slot in the diff: url = f"ssh://{ssh_user}@git.example.invalid/org/repo.git"
log.info(f"connecting as {ssh_user}")The second line has no userinfo position at all; it is collateral from the first. Scope of the claimThe boundary is specifically scheme-URL userinfo. A bare Reproduction, if useful: probe scriptimport importlib.util, importlib.machinery, sys, pathlib
mod_path = pathlib.Path(sys.argv[1]) # skills/autoreview/scripts/autoreview
spec = importlib.util.spec_from_loader(
"ar", importlib.machinery.SourceFileLoader("ar", str(mod_path))
)
ar = importlib.util.module_from_spec(spec)
sys.modules["ar"] = ar
spec.loader.exec_module(ar)
CASES = [
("fixture.py", 'url = f"ssh://{ssh_user}@git.example.invalid/org/repo.git"'),
("fixture.py", 'url = f"https://{user}@example.com/repo"'),
("fixture.py", 'target = f"{account}@mail.example.invalid"'),
("fixture.ts", "const url = `ssh://${sshUser}@git.example.invalid/org/repo.git`;"),
("fixture.py", 'u = "https://alice@github.com/example/repo"'),
("fixture.py", 'dsn = "postgres://user:hunter2pass@db.example/app"'),
]
for name, line in CASES:
patch = (
f"diff --git a/{name} b/{name}\n--- a/{name}\n+++ b/{name}\n"
f"@@ -0,0 +1 @@\n+{line}\n"
)
try:
out = ar.validate_review_patch("diff", [name], patch)
got = next(
l[1:] for l in out.splitlines()
if l.startswith("+") and not l.startswith("+++")
)
status = "same" if got == line else "masked"
except SystemExit as error:
got, status = str(error).splitlines()[0], "REFUSED"
print(f"[{status}] {line}\n -> {got}")Run against each checkout: Not opening a competing fix — this branch owns the subsystem. Flagging it here because the refusal path is the one failure mode that costs more than a false positive: a contributor whose diff contains a templated remote URL cannot run autoreview at all, and the error text points at "move or remove the ambiguous occurrence" for source that has no secret in it. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
2 participants
Summary
Proof
python3 -m unittest skills.autoreview.tests.test_autoreview_hardening -k review_patch(281 passed)python3 -m unittest skills.autoreview.tests.test_autoreview_hardening -k secret_detector(86 passed)PYTHONPATH=/tmp/agent-skills-validate-deps scripts/validate-skills(6 skills validated)git diff --check(0, 8)Local full-suite note
The full 520-test run passed 515 tests and skipped 1. Four JVM-home tests failed only because this Mac has no Java runtime; GitHub CI runs on Java-equipped Ubuntu and Windows images.