Skip to content

fix(review): stop rejecting the approved autoreview fixture - #1309

Merged
steipete merged 2 commits into
mainfrom
steipete/strange-khorana-5cfb9c
Aug 30, 2026
Merged

fix(review): stop rejecting the approved autoreview fixture#1309
steipete merged 2 commits into
mainfrom
steipete/strange-khorana-5cfb9c

Conversation

@steipete

@steipete steipete commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Resolves a problem where exact reviews stop before model execution when an unrelated edit touches the autoreview hardening test file containing an unchanged, reviewed synthetic credential-rejection fixture.

The confirmed example is agent-skills PR205, with exact-review job 99170200224 failing with AgentInputScanError: findings at host source 8d11c21558ba90ac6812b80b95efd0dd9b0aa936. Complete before/after blob coverage includes that unchanged fixture even outside patch context; the clean independent exact-diff scan was not contradictory.

Why This Change Was Made

Register the maintainer-approved full-URI digest 662a886a0fd7447dad0acda3aeccc9eb539fc90438b453de7e2f523ca7ee6c83 only for mode 100644 Git blobs at:

  • skills/autoreview/tests/test_autoreview_hardening.py
  • .agents/skills/autoreview/tests/test_autoreview_hardening.py

The existing action-ledger entry remains supported. Each staged OID retains every scanned logical endpoint's path and mode, including ineligible references. Every reference must satisfy the same digest/path/mode policy before classification; a valid base cannot authorize an executable head/index/worktree or an unapproved alias. Raw bytes remain staged once per OID. Unique audit sources are derived only after this validation.

This is not a domain, credential-word, test-name or unchanged-line exemption; no neighboring URI is implicitly approved. Full before/after/deleted/index/raw-worktree and prompt/schema/diff/additional coverage, scanner flags, budgets, strict output/version/verification/literal-line checks, source fences and cleanup remain intact. One unmatched finding refuses the whole scan, with no success notice. No original fixture literal was added to repository test sources, rewritten upstream, or obfuscated.

Review Finding Disposition

Accepted and fixed the P1 endpoint-mode finding. Published head e8dfeaf52a8fd6c29d5ced427c80a0ce1207df8b inherited approval across a same-OID 100644→100755 transition because it retained only eligible references. Real TruffleHog reproduced that admission at both approved paths. The legacy classifier at production 8d11c21558 had the same inheritance pattern, so the repair consistently enforces the fence for both entries rather than adding a legacy exception.

All three rank-up requests were applied: retain complete endpoint eligibility context; add mode/alias/snapshot regressions; publish a redacted after-fix rejection trace. New hermetic regressions produced 75 passed / 7 failed against the prior code, then 82 passed / 0 failed after the repair. They cover both mode-transition directions, executable head/index/worktree snapshots, ineligible shared aliases, and valid repeated regular snapshot OIDs.

User Impact

Reviews containing this exact approved fixture can proceed, while executable or otherwise ineligible source references remain blocking. TruffleHog findings are not hidden: accepted records emit digest/source/blob/line/decoder/count notices. Shared-blob counts are per-source references and must not be summed as unique findings.

OpenClaw Bay Impact

Unaffected: no lifecycle, queue, workflow, publication, observer API or dashboard data-contract change. The raw-Git provenance repair and historical review comments are untouched.

Documentation Impact

Updated the active README Safety Model and existing unreleased changelog entry. Documentation now states that every scanned endpoint association must qualify and distinguishes classification from detector coverage. No new documentation page or lifecycle category.

Evidence

  • Current head: c8f03ea935473ee2702435f0bb6ca1f36e825918.
  • Required pre-commit Codex review, including P1 blockers: scoped-clean, no P0/P1 findings. The separate committed-base Codex review of the complete PR at this head is also scoped-clean with no findings.
  • Fresh canonical validation: Node 24.20.0 / Bash 5.3.15 complete pnpm run check passed: 4,090 passed, zero failed, nine skipped, plus all 13 changed-coverage tests; focused scanner/raw-Git suite 82 passed. All coverage gates ran unchanged.
  • CI and the committed-base review are checked separately against the exact head before landing; neither replaces runtime proof.

Real Behavior Proof

Surface: the real scanAgentInput admission API and native TruffleHog. A counter immediately after admission is an inert downstream canary; no review model or target helper is invoked.

Environment: macOS, Node 24.20.0, external TruffleHog 3.97.1, binary SHA-256 f6899e5521ff060634a9599e02215195d29d330e9a902feb3f5322eedcb7ba21. The original baseline scanner was extracted from production 8d11c21558ba90ac6812b80b95efd0dd9b0aa936. A separate frozen copy of published head e8dfeaf52a8fd6c29d5ced427c80a0ce1207df8b establishes the endpoint bug before this repair.

Every run keeps the production flags:

filesystem <private-input-dir> --results=verified,unknown --fail --fail-on-scan-errors --no-update --json --no-color

Original false-positive regression

The pinned target range is ae75f60e8d454f1cf44ec4613e10ec9ea7f2ade7..a8466c1d860588a083610fe41fd277c1d88b14e0: five changed paths, ten complete before/after blobs, plus raw patch/metadata. The fixture and named test AST are unchanged; neither literal occurs in patch context.

Case Production baseline Fixed candidate
Full pinned candidate findings, canary 0 admitted, canary 1, audit
Exact blobs at vendored path findings, canary 0 admitted, canary 1, audit
Both approved paths share blobs findings, canary 0 admitted, two notices / two unique occurrences
Existing regular action-ledger fixture admitted admitted
Twelve negative source/surface cases findings, canary 0 findings, canary 0, no success notice

The negatives cover wrong path, executable Git mode, altered password/host, mixed findings, prompt, schema, additional input, raw patch context, deleted bytes, the wrong digest at an otherwise approved path, and real Base64-decoded URI findings. All sixteen original scenarios were requalified; the production-before/current-after comparison preserves 32 matching admission assertions.

Primary scans still report the same two URI findings and 112 chunks / 1,388,714 processed bytes. The vendored source/path replay reports 53 chunks / 654,618 bytes. Both findings are URI detector 17, filesystem source 15, PLAIN, unknown verification state, equal Raw/RawV2 matching the approved digest, null extras, and complete error-free scanner logs. Their value-free identities are:

[
  {"blob":"3430fd935c089a5f4c5c1cc4b5422d1d43773778","line":3424,"occurrences":1},
  {"blob":"58dc0fbb4309a9b02a31be69bbcc65f7bddd244f","line":3563,"occurrences":1}
]

Endpoint-boundary regression

The canonical/vendored mode fixtures reuse exact candidate blob 58dc0fbb4309a9b02a31be69bbcc65f7bddd244f. The legacy case reuses its existing approved literal in a small fixture at its approved path. Each mode-only commit preserves its blob OID, and raw patches contain no URI literal, so rejection cannot be attributed to a patch finding.

Case Published head before repair Fixed candidate
Canonical 100644→100755, same OID admitted, canary 1 findings, canary 0, no success notice
Vendored 100644→100755, same OID admitted, canary 1 findings, canary 0, no success notice
Canonical 100755→100644, same OID admitted, canary 1 findings, canary 0, no success notice
Legacy 100644→100755, same OID admitted, canary 1 findings, canary 0, no success notice
Eligible and ineligible shared-path aliases admitted, canary 1 findings, canary 0, no success notice
Both approved aliases, all references regular admitted admitted
Ordinary regular-source control admitted admitted

Each mode-transition case has one actual native URI finding. The normalized after-fix record for the canonical case is:

{
  "source":"skills/autoreview/tests/test_autoreview_hardening.py",
  "fixtureSha256":"662a886a0fd7447dad0acda3aeccc9eb539fc90438b453de7e2f523ca7ee6c83",
  "endpointModes":["100644","100755"],
  "endpointBlobOids":["58dc0fbb4309a9b02a31be69bbcc65f7bddd244f","58dc0fbb4309a9b02a31be69bbcc65f7bddd244f"],
  "rawPatchContainsFixture":false,
  "nativeUriFindings":1,
  "outcome":"findings",
  "downstreamCanaryStarts":0,
  "successAuditNotices":0,
  "modelInvoked":false
}

Current-head binding: Re-executed against committed head c8f03ea935473ee2702435f0bb6ca1f36e825918: thirteen original admission cases, seven endpoint cases, the remaining three original cases, and the exact minimal replay below all met their stated outcomes. The numeric-HTML control remains separately recorded as unchanged nondetection, not refusal proof. Mode-transition findings reject with canary 0 and no success notice; ordinary canonical/vendored cases admit with the same native findings and audits. All five committed source fingerprints match the executed code. Compiled scanner dependency-closure SHA-256: 7957d23b1c0fb097dacc1db6db9264959ddf6b2894d042090393ce8a30c28e42. No review model ran.

Minimal replay: after building either host scanner version, set SCAN_MODULE to its absolute compiled agent-input-scan.js path and SCAN_TARGET_DIR to the clean pinned agent-skills head with its base available. This command was executed for the candidate:

node --input-type=module <<'NODE'
import { pathToFileURL } from 'node:url';
const { scanAgentInput, AgentInputScanError } = await import(pathToFileURL(process.env.SCAN_MODULE));
let outcome = 'accepted';
let downstreamCanaryStarts = 0;
try {
  scanAgentInput({
    cwd: process.env.SCAN_TARGET_DIR,
    prompt: 'Controlled scanner admission proof; no review model.',
    source: {
      kind: 'committed',
      baseSha: 'ae75f60e8d454f1cf44ec4613e10ec9ea7f2ade7',
      headSha: 'a8466c1d860588a083610fe41fd277c1d88b14e0',
    },
    timeoutMs: 180000,
  });
  downstreamCanaryStarts++;
} catch (error) {
  outcome = error instanceof AgentInputScanError ? error.reason : 'unexpected_failure';
}
console.log(JSON.stringify({ outcome, downstreamCanaryStarts, modelInvoked: false }));
NODE

Limits: original production prompt/schema bytes were unavailable; replay uses harmless text with real source bytes. Vendored and mode-transition cases are controlled source/path replays, not a claimed full historical OpenClaw range. The local scanner binary is macOS, not the original Linux binary. A fully numeric-HTML-encoded Python-literal probe yields zero findings on both old and fixed code: that unsuccessful detection assumption is retained and not counted as refusal proof. Reported encoded findings still reject; real BASE64 URI findings matching the approved digest were observed and refused. No detector option, coverage check, or threshold was weakened. Raw URI values and verification diagnostics remain undisclosed; these normalized receipts are not agent transcripts.

@clawsweeper

clawsweeper Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 30, 2026
@clawsweeper

clawsweeper Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 1:47 PM ET / 17:47 UTC.

ClawSweeper review

What this changes

This PR expands the host-side reviewed-fixture registry and makes secret-scan classification require every Git endpoint associated with a scanned blob to match that fixture’s exact digest, approved path, and regular-file mode.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep this PR open for normal landing review. The endpoint-association fix resolves the earlier P1 mode-inheritance flaw while preserving fail-closed scanning for every unapproved digest, path, or mode.

Priority: P1
Reviewed head: c8f03ea935473ee2702435f0bb6ca1f36e825918

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) Strong native after-fix admission evidence and focused fail-closed regression coverage support the narrow repair.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.
Evidence reviewed 5 items Endpoint eligibility fence: The classifier hashes each reported full URI, requires the strict URI detector contract and literal-line match, then rejects classification when any staged reference lacks the same digest/path/mode approval.
All scanned associations retained: The scanner records both sides of every raw Git endpoint transition for each blob OID, including mode-only transitions, and passes the complete reference list to the classifier.
Focused regression coverage: The added suite covers aliases, both mode-transition directions, snapshot states, malformed scanner output, and fail-closed source surfaces; accepted cases remain limited to eligible fixture associations.
Findings None None.
Security None None.

How this fits together

ClawSweeper stages review inputs and Git diff endpoints for TruffleHog before any model review begins. This change affects the narrow post-scan classifier that may recognize explicitly approved synthetic test fixtures; all other findings still stop the review.

flowchart TD
A[Review inputs and Git endpoints] --> B[Stage source blobs]
B --> C[Native TruffleHog scan]
C --> D{Exact fixture and detector match?}
D -->|No| E[Refuse review]
D -->|Yes| F{Every path and mode approved?}
F -->|No| E
F -->|Yes| G[Emit value-free audit]
G --> H[Continue to review model]
Loading

Before merge

  • Resolve merge risk (P1) - This intentionally changes the pre-model secret-scan boundary; an error in the approved fixture digest, source path, or mode could classify a finding that should block review.
  • Complete next step (P2) - No discrete repair remains after the prior endpoint-mode finding was addressed; this PR needs ordinary maintainer landing review of its security-sensitive exception.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and regression delta production +84/-36, tests +181/-15 Most code growth is targeted endpoint-provenance coverage for a security-sensitive admission boundary.
Changed files 5 files affected The implementation, its regression suite, and the documented scanner contract change together.

Merge-risk options

Maintainer options:

  1. Accept the bounded fixture policy (recommended)
    Accept the explicitly reviewed digest/path/mode exception after confirming the two approved source paths remain the intended synthetic-fixture boundary.
  2. Pause if approval scope changed
    Pause the PR if either approved fixture source or its synthetic test purpose no longer has maintainer approval.

Technical review

Best possible solution:

Land only the narrowly documented exact-digest/path/mode exception while retaining the existing fail-closed behavior for every other scanner finding.

Do we have a high-confidence way to reproduce the issue?

Yes. The PR supplies current-head native TruffleHog traces for the original false rejection and the previously accepted mode/alias boundary cases, and the focused source coverage matches those scenarios.

Is this the best way to solve the issue?

Yes. Retaining every endpoint association and requiring all of them to satisfy the same exact fixture policy is the narrowest way to admit the approved synthetic URI without introducing a broad scanner exemption.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c179a466606f.

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P1: This repairs a review-admission failure and protects a secret-scanning boundary used before model execution.
  • merge-risk: 🚨 security-boundary: The PR intentionally permits a narrowly identified scanner finding to pass the admission gate under exact source constraints.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the post-TruffleHog fixture classifier; the PR body provides redacted native TruffleHog 3.97.1 traces at the reviewed head showing approved source replays proceed while executable-mode and unapproved-alias cases reject before the downstream canary and emit no success audit.

Evidence

What I checked:

  • Endpoint eligibility fence: The classifier hashes each reported full URI, requires the strict URI detector contract and literal-line match, then rejects classification when any staged reference lacks the same digest/path/mode approval. (src/agent-input-scan-fixtures.ts:118, c8f03ea93547)
  • All scanned associations retained: The scanner records both sides of every raw Git endpoint transition for each blob OID, including mode-only transitions, and passes the complete reference list to the classifier. (src/agent-input-scan.ts:340, c8f03ea93547)
  • Focused regression coverage: The added suite covers aliases, both mode-transition directions, snapshot states, malformed scanner output, and fail-closed source surfaces; accepted cases remain limited to eligible fixture associations. (test/agent-input-scan.test.ts:455, c8f03ea93547)
  • Current-head real behavior evidence: The PR body records native TruffleHog 3.97.1 runs at the reviewed head, showing approved sources admit while mode and alias negatives reject before the downstream canary, with no success audit notice. (c8f03ea93547)
  • Feature history: The original reviewed-fixture classifier was introduced as a new file by the merged commit below; the current change is a narrow follow-up to that scanner boundary. (src/agent-input-scan-fixtures.ts:4, 0a46047ef6a6)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-08-30T16:34:11.020Z sha e8dfeaf :: needs real behavior proof before merge. :: [P1] Reject fixture approval after a regular-to-executable mode change

@steipete

Copy link
Copy Markdown
Contributor Author

Additional reproduction for the same source-association repair requested in the current review: the fence must preserve path/digest associations as well as endpoint modes. This is verified against published head e8dfeaf52a8fd6c29d5ced427c80a0ce1207df8b, not a verdict on the in-flight rewrite.

I reproduced two cases with real TruffleHog 3.97.1 and the immutable production scanner owner. Both associated source files remain mode 100644 at both endpoints; no executable transition is involved.

  1. Start with the approved autoreview fixture file and an identical copy at unapproved/fixture.py. Make the same harmless edit in both files, outside the URI's diff context. The before blobs match each other, and the after blobs match each other.
  2. Repeat with the second copy at test/action-ledger-runtime.test.ts, which is registered for a different approved fixture digest, not the autoreview digest.

Current main c179a466606f782ae73404357a0c94e6bead5807 refuses both cases with findings and downstream canary 0. Both are admitted at the published head: outcome=accepted, downstreamCanaryStarts=1, modelInvoked=false. The host emits only the approved autoreview source notice, omitting the ineligible shared association. Raw URI values remain undisclosed. This is controlled production-owner admission proof using the exact source modules transformed by Node 24.20.0's TypeScript API and the real installed scanner; no scanner output or provider is mocked, and no model is invoked.

The reason is visible across the two owners: the producer retains only qualifying source associations, while the classifier accepts a nonempty intersection instead of requiring every association to match that digest's policy. Blob deduplication therefore lets the approved occurrence lend eligibility to a different source.

Please cover these cases in the same endpoint-provenance repair: retain all scanned logical source/mode associations and reject classification if any association is ineligible for the exact digest, including associations supplied by the index/snapshot path. Preserve positive sharing of the actual autoreview digest at its two approved paths, with the existing per-source audit notices. The existing shared approved path OIDs unit case uses the ledger digest at the autoreview paths and should not expect that combination to be accepted.

The existing owner retains branch, push, and merge control. This note adds concrete pre-merge evidence; it requests no policy broadening, fixture rewrite, historical-comment edit, or scanner bypass.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 30, 2026
@steipete
steipete merged commit 74b0f85 into main Aug 30, 2026
20 checks passed
@steipete
steipete deleted the steipete/strange-khorana-5cfb9c branch August 30, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant