Skip to content

fix(autoreview): preserve commit scope at history boundaries - #205

Merged
steipete merged 2 commits into
mainfrom
fix/provenance-boundary-20260829
Aug 29, 2026
Merged

fix(autoreview): preserve commit scope at history boundaries#205
steipete merged 2 commits into
mainfrom
fix/provenance-boundary-20260829

Conversation

@steipete

@steipete steipete commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Autoreview commit inputs currently rely on Git's traversal view of ancestry. At a shallow boundary, rev-list --parents and git show can make carried-forward code look newly added. Local ancestry overlays can also make the reported comparison differ from the underlying commit objects. A review can therefore receive the wrong patch and finding scope.

The provenance instructions also need an explicit evidence gate: blame, commit subjects, PR metadata, and merger identity locate candidates, but do not establish who introduced a regression.

Why This Change Was Made

The repair belongs at the Git-input owner, before the reviewer receives a bundle:

  • Read raw commit bytes, consume only LF-delimited parent records immediately following the tree record, and validate full object IDs. Identity control characters and later extra headers cannot manufacture ancestry. SHA-1, SHA-256, and uppercase hexadecimal IDs remain supported.
  • Ignore replacement refs and legacy grafts consistently. Missing parent objects fail closed with actionable history-completion guidance; the helper does not fetch them automatically.
  • Use the same pinned raw-parent comparison for patch text, statistics, binary/gitlink checks, and captured finding membership. Record the verified commit and parent, or verified parentless root, in the bundle.
  • Preserve the current CapturedBundle / build_bundle contract. review_paths stays deleted; consumers use the captured text and filtered paths rather than performing another membership lookup. Upstream evidence capture/revalidation, preparation progress, ref pinning, and full-tree integrity checks remain intact.
  • Apply patch-backed provenance requirements in both the caller-facing skill and the actual isolated-reviewer prompt. Keep code author, introducing PR author, merger, committer, automation trigger, and current PR author distinct.

This also repairs the smoke harness's stale prompt-builder call and isolates the prompt-limit fixture from the host's Kimi configuration. Provider isolation and credential-scanning protections are preserved.

Both companion repairs have landed: agent-scripts#36 — require patch-backed regression provenance and ClawSweeper#1298 — verify provenance against raw Git history.

User Impact

Commit reviews receive the actual commit delta rather than an inferred root snapshot or a misleading empty comparison. Genuine roots remain reviewable, and shallow checkouts remain usable when the recorded parent is available. When it is unavailable, the helper stops before sending the review and explains what history is needed.

There are no new CLI/configuration options or runtime dependencies. Historical attribution remains explicitly uncertain when the parent-relative behavioral change cannot be verified. This PR does not validate or reassign any original historical introduction claim: an author, merger, or automation trigger alone is not proof of causation.

Evidence

Current candidate: a8466c1d860588a083610fe41fd277c1d88b14e0, based on ae75f60e8d454f1cf44ec4613e10ec9ea7f2ade7. The provenance implementation was validated at eb026f9893a60ba8ff41250b7234f3fd0e9fa715; the current head adds only the two-net-line test-clock repair described below.

  • Focused integration: 20 tests passed in 189.437 seconds. Covers raw ancestry, captured membership, ref pinning, evidence mutation/topology, snapshot/progress behavior, and scanner/isolation guards.
  • Initial full canonical Python suites: 225 tests completed in 171.090 seconds; OK (skipped=1). The single skip is platform-specific.
  • Frozen-source check: all 14 tracked Autoreview/changelog paths retained the same before/after fingerprint during that full run. The initial provenance candidate matches those tested bytes: 90153836997184205b879723434fc0c3579f98e6363ba298b330b4034e82197c.
  • Static gates: all eight canonical skills validated; shell-harness syntax, changed Python syntax, conflict-marker absence, and git diff --check passed.
  • Independent structured reviews: the provenance integration returned scoped-clean, no actionable P0–P2 findings, confidence 0.90. The subsequent isolated clock-fixture repair separately returned scoped-clean, no findings, confidence 0.97. These were static reviews; executed behavioral proof is recorded separately.
  • Current size versus the rebased base: production +50/-64 (net -14); tests +157/-15 (net +142); test-support +1/-1; skill documentation +8/-3; changelog +1.

Regression-first reproductions covered shallow-boundary scope errors, identity separators including CR/VT/FF and Unicode separators, one- and two-parent legacy grafts, and misplaced parent headers. Positive and refusal coverage includes true roots, retained shallow parents, replacement refs, full object-ID formats, malformed parent records, and merge exclusion. The integrated tests inspect the text and membership from one captured result. Upstream capture/evidence/progress owner implementations and their corresponding tests were preserved.

Dependency contracts were checked directly against Git's identity serialization, its consecutive LF-delimited parent parser, its independent graft/replacement setup, and Python's binary versus text-stream behavior, with real-Git regression proof.

Executed commands

Local proof used Python 3.14.7 and Git 2.55.0 on macOS. These are the repository-root forms of the executed commands; only machine-specific executable and checkout prefixes have been omitted. Git selection was process-local, not a change to machine defaults.

Focused integration suite — 20 tests
python3.14 -B skills/autoreview/tests/test_autoreview_hardening.py \
  AutoreviewHardeningTests.test_commit_review_keeps_identity_separators_out_of_parent_records \
  AutoreviewHardeningTests.test_commit_review_follows_only_contiguous_full_parent_ids \
  AutoreviewHardeningTests.test_commit_review_uses_raw_parents_at_history_boundaries \
  AutoreviewHardeningTests.test_commit_bundle_rejects_merge_commits \
  AutoreviewHardeningTests.test_smoke_harness_validates_runtime_prompt_without_provider \
  AutoreviewHardeningTests.test_bundle_ref_pinning_keeps_patch_and_membership_coherent \
  AutoreviewHardeningTests.test_preparation_reuses_untracked_capture_and_keeps_three_full_snapshots \
  AutoreviewHardeningTests.test_dry_run_reuses_capture_without_whole_tree_snapshots \
  AutoreviewHardeningTests.test_evidence_mutations_refuse_stale_publication_and_later_passes \
  AutoreviewHardeningTests.test_evidence_topology_changes_with_identical_bytes_refuse_send \
  AutoreviewHardeningTests.test_duplicate_evidence_keeps_exact_frozen_bytes_across_passes \
  AutoreviewHardeningTests.test_tracked_source_permission_never_authorizes_evidence \
  AutoreviewHardeningTests.test_unrelated_same_size_restored_mtime_mutation_is_still_guarded \
  AutoreviewHardeningTests.test_preparation_ticker_rate_limit_counts_and_cleanup \
  AutoreviewHardeningTests.test_tracked_binary_changes_are_blocked_in_all_modes \
  AutoreviewHardeningTests.test_gitlink_changes_are_blocked_in_all_modes \
  AutoreviewHardeningTests.test_tracked_sensitive_paths_are_omitted_in_all_modes \
  AutoreviewHardeningTests.test_reviewer_scan_refusal_prevents_provider_call \
  AutoreviewHardeningTests.test_codex_isolation_restricts_tool_environment \
  AutoreviewHardeningTests.test_dry_run_flag_exits_zero_for_plain_commit_mode
python3.14 -B -m unittest \
  skills/autoreview/scripts/autoreview_test.py \
  skills.autoreview.tests.test_autoreview_hardening

python -B scripts/validate-skills
bash -n skills/autoreview/scripts/test-review-harness

git diff --check ae75f60e8d454f1cf44ec4613e10ec9ea7f2ade7 -- \
  CHANGELOG.md skills/autoreview/SKILL.md \
  skills/autoreview/scripts/autoreview \
  skills/autoreview/scripts/test-review-harness.py \
  skills/autoreview/tests/test_autoreview_hardening.py

The skill validator ran in a disposable environment containing the existing pinned requirements-dev.txt dependencies. Changed Python files were syntax-checked with ast.parse before the test runs.

These are local results, not a claim of live-provider E2E coverage or GitHub CI completion. Required PR CI remains a separate landing gate.

Windows CI follow-up

The first validation run passed Linux but exposed one existing progress-ticker fixture failure on Windows: test_preparation_ticker_reports_while_caller_is_blocked observed one flush instead of two. The fixture captured the real host clock before mocking the later tick. A reproducible start of 1010.1 makes started + 15 - started equal 14.999999999999886, correctly below the production reporting threshold.

The pre-fix test reproduced the same 1 != 2 failure under that clock. The repaired fixture owns initialization too, using an exactly representable clock origin. The same reproduction now passes, as do all three focused ticker/counting/cleanup tests. The real ticker, its 15-second threshold, thread ordering, and every existing assertion are unchanged; there is no retry, larger timeout, or production workaround. The independent review of this isolated test repair is clean. Current-head validation run 33278532267 passed on both Linux and Windows; CodeQL also passed.

Hosted-review limitation

Hosted ClawSweeper review run 33278765964 stopped at input scanning before review inference. A local reproduction with the production scanner flags identified two URI unknown results for the same unchanged negative proxy-URL fixture in the complete base/head test-file blobs. The fixture uses a reserved .invalid hostname and explicit synthetic username/password words; it tests rejection of credentialed proxies. It exists unchanged in both this PR's base and head, and already exists in OpenClaw's vendor copy outside the downstream diff.

No real credential was identified or exposed, and no scanner override, exemption, fixture obfuscation, or retry-to-green was used. The hosted review remains unavailable; the clean independent reviews used the approved, scanned exact-diff path. A separate follow-up tracks narrowly auditable handling of synthetic URI fixtures in ClawSweeper. Landing proof is the independent review plus the executed regression and exact-head cross-platform CI, not a claimed hosted-bot verdict.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@steipete
steipete marked this pull request as ready for review August 29, 2026 20:16
@steipete

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

The current head is a8466c1. The body now records the Windows CI failure, deterministic fail-before/pass-after clock reproduction, and isolated fixture repair. Production behavior and existing assertions are unchanged. Both the original provenance integration and the follow-up test repair have clean independent reviews; fresh Linux/Windows validation is attached to this exact head.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

@steipete

Copy link
Copy Markdown
Contributor Author

Land-ready technical verification for a8466c1.

The raw-parent comparison now owns both review text and captured finding scope; blame and graph-truncated ancestry no longer stand in for introduction proof. The current CapturedBundle flow and upstream evidence, progress, ref-pinning, and integrity protections remain intact. Production changes are net −14 lines; tests are net +142, counted separately.

Exact-head validation run 33278532267 passed on both Linux and Windows; CodeQL passed too. The earlier Windows failure was reproduced locally as a floating-point fixture error and repaired without changing the ticker, threshold, threading, or assertions. Its identical fail-before case now passes, along with the three focused progress tests. The initial full local run completed 225 tests with one platform skip. A redundant later local full-suite repeat was stopped after the exact-head cross-platform run passed; that repeat is not counted as a pass.

The full provenance integration received a clean independent P0–P2 review (0.90), and the isolated fixture repair received a separate clean review (0.97). The PR body contains the exact full/focused commands and dependency-source evidence. No new configuration, dependency, storage format, or provider-isolation exception was introduced. The historical Browser attribution remains unassigned rather than guessing a replacement introducer.

Hosted ClawSweeper review did not produce a verdict: run 33278765964 refused an unchanged reserved-domain synthetic proxy fixture before review inference. The PR body records the reproduced detector/source evidence and the separate follow-up. No scanner bypass or fixture change was made. The repository has no mandatory hosted-bot or independent GitHub-approval gate; the reviewed patch and exact-head CI are the landing evidence, not the failed hosted review.

@steipete
steipete merged commit 6c32108 into main Aug 29, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant