fix(review): attribute a finding to the patch before filing it - #1075
fix(review): attribute a finding to the patch before filing it#1075masatohoshino wants to merge 3 commits into
Conversation
A `reviewFinding` is scored against the contributor's patch, but the prompt never says how to establish that this patch caused the condition. It names no base and asks only that the location "overlap the PR diff when possible", so a condition that predates the branch can land in the channel that scores the patch. `lateFinding` — the other provenance property in the same section — already gets a named command and a fail-closed default. Add the missing step in that same shape, with every check anchored on one snapshot so two reads of the same file cannot disagree: the merge base of `base.sha` and `head.sha`. `base.sha` alone is the base-branch tip, so a two-dot diff against it reports base-branch commits made after the branch diverged as this patch's work, and `HEAD` is not guaranteed to be the PR head in every review lane. A renamed file needs its previous path on the base side and both paths in the diff, or it reads as wholly added. Findings stay strict. Introduced, materially worsened, and newly-reachable conditions remain findings, and so does a condition the PR claimed to cover but did not — a patch that says it migrates every existing record while handling only new ones is incomplete against its own claim. When the comparison cannot be made the finding stays: an unfair finding costs an argument, a suppressed one ships a defect. A genuine pre-existing prerequisite routes to `risks` plus `maintainerDecision.required`, which pauses automated landing without touching patch quality. Merge-risk fields do not reach that gate, and `fix_before_merge` is the only category permitted to carry `automergeInstruction`, so using it as a blocker would invite the repair lane to widen the PR into the pre-existing debt; both are called out. Whether the prerequisite is repaired here or tracked as follow-up stays the target repository's scope call. Prompt prose and one policy test pinning fourteen clauses. No production TypeScript, no schema change, no second review pass, and no additional model call.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 22, 2026, 5:10 PM ET / 21:10 UTC. ClawSweeper reviewWhat this changesThe PR adds review-prompt guidance and tests for attributing PR findings using pull-request diffs and hydrated file blobs rather than shallow-clone ancestry. Merge readiness⛔ Blocked until stronger real behavior proof is added - 7 items remain Keep open: the branch remains useful, but its unresolved-attribution path still creates contributor-facing P0–P2 work, and the required current-head container proof is absent. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherClawSweeper hydrates PR metadata and file blobs, then supplies them to the review prompt that emits findings, ratings, and routing decisions. This change affects whether a detected condition is charged to a contributor or presented as a maintainer decision. flowchart LR
A[Pull request metadata] --> B[Review context]
C[Hydrated file blobs] --> B
B --> D[Review prompt]
D --> E[Finding attribution]
E --> F[Verdict and routing]
F --> G[Contributor and maintainer output]
Decision needed
Why: The branch deliberately selects the former despite existing routing semantics, making this a policy tradeoff between defect retention and contributor attribution. 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: Keep definite patch defects as findings, but route unresolved attribution to risks plus a required maintainer scope decision so it cannot downgrade or block the contributor by itself. Do we have a high-confidence way to reproduce the issue? Yes: current source shows that any retained P0–P2 finding is contributor work regardless of overallCorrectness, and the PR includes a shallow-cache fixture for the local-cache premise. Is this the best way to solve the issue? No: retaining an unestablished-attribution finding still blocks contributor readiness, so the stated guard does not achieve the intended routing outcome. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 648ad3538d98. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (23 earlier review cycles; latest 8 shown)
|
…lly has The first live ClawSweeper scan of this branch found that its own procedure could not run. It told the reviewer to use `git merge-base` and a three-dot `git diff a...b`, but the review cache materializes a missing base or head with `--depth=1` (`ensureReviewTreeCommit`), which leaves both trees present and their shared history absent. Reproduced against this PR's refs with the production fetches: both commits resolve, `merge-base` exits 1, three-dot exits 128 with "no merge base". The rule would then take its own fail-closed branch and file the base-state finding it exists to prevent. Nothing about the hydration is wrong. Its contract is the two commits plus the base-side and head-side blob of every changed file, including a renamed file's previous path, and it keeps that contract. The prompt asked for something outside it, so the prompt is what changes here — no production TypeScript, no new fetch, no widened cache. Attribution now reads off the pull request diff first, which is computed against the merge base, so its added lines are the branch's work and its context lines are not. That diff is bounded — each file's patch is truncated and the file list is capped — so both sides of the file are readable directly with `GIT_NO_LAZY_FETCH=1 git show`, head side included, since a truncated patch otherwise leaves new code with no source at all. The environment variable is load-bearing: the checkout is a blobless promisor clone and hydration deliberately skips blobs over its size budget, so a plain `git show` would pull an oversized one back and walk around that budget. Most of the review rounds pushed against suppression rather than against over-filing, which is worth stating because this change exists to reduce findings. `base.sha` is the base branch's current tip, not the merge base, so on a branch behind its base the read cuts both ways: a difference may be the base branch's own work, and matching content may be something the base branch introduced independently after divergence. It may clear the patch only when the context positively shows the branch is not behind, and an absent or unknown `mergeableState` is not that showing. The missing-base-path exception is rename-only: when the diff reports a file as added, absence is exactly what added means and the patch owns its contents. Blob hydration is best-effort, so the base read can be missing for a condition that genuinely predates the branch. Keeping a defect visible and scoring the contributor for it are separate acts: the finding stays, the reviewer must say provenance could not be established, and that unestablished attribution may not by itself set `overallCorrectness` to `patch is incorrect`. Retaining it still counts as contributor work, which the prompt says plainly rather than implying the guard makes retention free. Two tests, because a prompt assertion cannot speak for the review environment: the policy test pins every clause and pins all three superseded command forms absent, and a new shallow-cache integration test builds the cache the way `ensureReviewTreeCommit` does, runs the real `hydratePullRequestReviewBlobs`, points the remote at an invalid URL, and proves offline that the instructed reads succeed — including a renamed file through its previous path — while `merge-base` and the three-dot diff do not.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Reading provenance off the pull request diff settles who wrote the lines. That is not the same question as whether the condition is this patch's: a defect in untouched lines is still the patch's when the patch worsened it, newly made its bad path reachable, or claimed to cover it. Left as written, "lines it leaves as context are not this branch's work" would have suppressed exactly the newly-reachable case the surrounding rules preserve. Say it in the prompt: the diff answers authorship, the rules below answer attribution. The policy test pins both sentences.
|
The P1 was correct and is fixed. Reproduced against this PR's own refs, using the fetches So in that cache state the two commands the procedure named — Hydration is unchanged: same bounded set of objects, same best-effort behaviour,
Head The routing-policy decision remains open. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Added the structured-result proof the last review asked for. Head is unchanged Three controlled cases were run through A — pre-existing prerequisite. Base compares stored rows byte for byte; the B — patch-introduced defect (control). Two P1 findings in the file the patch C — latent base defect the patch newly reaches. Filed as a finding whose own Boundaries, stated rather than blurred:
This validates execution of the current policy. It does not resolve the separate @clawsweeper re-review |
1 participant
Closes #1074
What Problem This Solves
Resolves a problem where a condition that already existed in the PR base can be
filed as a defect in the contributor's patch, turning it into contributor-facing
Before mergework.The review prompt already states the rule twice — findings are defects
introduced by the patch — but it gives the reviewer no way to establish that.
Nothing in the prompt names the PR base, and the location guidance is only that
the location should overlap the PR diff when possible. ComparelateFinding,the other provenance property in the same section: it gets a named verification
command, an instruction not to infer from a similar title or line location, and
an explicit fail-closed default.
That gap has a cost for contributors who follow OpenClaw's own rules.
CONTRIBUTING.mdasks them to keep PRs focused (one thing per PR), and the rootAGENTS.mdaccepts a one-sided fix that recordsexplicit follow-up work.A contributor who does exactly that, stating the boundary and naming the
follow-up, can still have the untouched surface charged to their patch as
contributor-facing rework.
Why This Change Was Made
The change is prose in the
reviewFindingssection of the review prompt, in thesame shape
lateFindingalready landed in: a named check, a stated rule, and afail-closed default. There is no schema change, no parser change, no rating
change, no second review pass, and no additional model call.
What the first live review changed
The first ClawSweeper scan of this PR found a real defect in it, and the
procedure below is the repair. The original wording told the reviewer to run
git merge-baseand a three-dotgit diff a...b. ClawSweeper observed that thereview cache materializes a missing base or head with
--depth=1(
ensureReviewTreeCommit), which leaves both trees present and their sharedhistory absent — so both commands fail there, the rule falls into its own
fail-closed branch, and it files the base-state finding it exists to prevent.
Reproduced against this PR's own refs, with the exact production fetches and
lazy fetching disabled:
Reproducible from the review comment on this PR, which names
src/clawsweeper-context-hydration.tsand the--depth=1fetch, and from theharness below, which re-runs those exact fetches against this PR's own refs:
The finding was accepted in full — the observed cache behaviour, the diagnosis,
and the conclusion that the procedure had to change. A separate recommendation
from a later local round was declined on the record; see the closeout. Nothing about the hydration is wrong: its contract is
the two commits plus the base-side and head-side blob of every changed file,
including a renamed file's previous path, and it keeps that contract exactly.
The prompt was asking for something outside it, so the prompt is what changed —
no production TypeScript, no new fetch, no widened cache.
The procedure that replaced it
It now names only sources the review actually has, in the order it should trust
them.
pullFiles[].patch, fetched from the GitHub API during hydration — not assomething the checkout computes. GitHub derives it against the merge base on
its own side, where the full history exists, so the shallow cache cannot
affect it: lines it shows as added or changed are this branch's work and
lines it leaves as context are not. That settles authorship, which is not
the whole test: a condition in untouched lines is still the patch's when the
patch worsened it, newly made its bad path reachable, or claimed to cover it.
The prompt says so explicitly, so the diff rule cannot suppress the
newly-reachable case the scope preserves. This is also why the repair needs
no local merge base at all.
GIT_NO_LAZY_FETCH=1 git show <head-sha>:<file>for the code as the branch leaves it — the read touse when a truncated patch hides the change being reviewed — and
GIT_NO_LAZY_FETCH=1 git show <base-sha>:<file>for the base side, usingprevious_filenamewhen the patch renamed it. The environment variable is load-bearing: thecheckout is a blobless promisor clone and hydration deliberately skips blobs
over its size budget, so a plain
git showwould pull an oversized one backover the network and walk around the budget it depends on. Failing the read
is the correct outcome — it falls through to the unresolved case. Needed because that diff is truncated per file and its file list is
capped, so it does not always reach the cited lines. The prompt states plainly
that
base.shais the base branch's current tip, not the merge base. On abranch behind its base that read cuts both ways: a difference may be the base
branch's own work rather than the contributor's, and matching content may be
something the base branch introduced independently after divergence rather
than code the patch inherited. So the fallback may clear the patch only when
the context positively shows the branch is not behind — an absent or unknown
mergeableStateis not that showing — and otherwise raises doubt only.git merge-baseand the three-dot diff — forbidden, with the--depth=1reason stated inline so the instruction cannot drift back.
Eight later review rounds sharpened each of those, and it is worth being
explicit that most of them pushed in the direction of not suppressing:
an unscoped rename exception would have cleared findings on added files,
base-side parity on a stale branch would have cleared findings the branch
actually introduced, and an unknown merge state would have defaulted to the
permissive reading. Each is now closed on the conservative side. Reading
base.shaalonecarries no merge-base semantics: on a stale branch a fix that landed on main
after divergence is present at the base and absent from the head, which reads as
the patch introduced it. The in-context diff is not complete. And blob
hydration is best-effort — it skips oversized or unresolvable files and only
warns — so the base read can be missing for a condition that genuinely predates
the branch.
The stale-base problem also has two directions, and the first repair only closed
one: a difference at the base tip being read as the patch's work. The mirror —
matching content being read as inherited when the base branch added it
independently — would suppress a real finding, which is the worse direction,
and it is now closed too.
The hydration finding forced a distinction the earlier wording had collapsed. Keeping a
defect visible and scoring the contributor for it are separate acts. When
neither source settles provenance the finding stays, so no real defect is
dropped, but the reviewer must say provenance could not be established, and that
unestablished attribution may not by itself set
overallCorrectnesstopatch is incorrect.The incremental cost of this change is zero network operations. Blob hydration
already runs on every PR review and is unchanged here; the procedure adds at
most one
git showper side of the file being attributed, only when the boundeddiff does not already answer the question, and
GIT_NO_LAZY_FETCH=1guaranteesthose reads never fetch. When hydration skipped the blob the read fails rather
than reaching for it — that is the point of the flag, and the proof shows both
sides of it: the same read is rc=128 before hydration and rc=0 after.
One review finding was not adopted, and it is worth naming. A later round
argued that an unproven attribution should leave
reviewFindingsentirely andlive in
risksplus a maintainer decision. Its factual half is right and is nowreflected in the prompt:
hasBlockingReviewFindingscounts any P0-P2 finding ascontributor work, so the
overallCorrectnessguard does not spare thecontributor a blocker, and the wording no longer implies it does. Its
recommendation is declined. Hydration skips a file because the file is large or
unresolvable, which is uncorrelated with whether the patch introduced the
defect — so routing on that signal would let real patch defects stop being
contributor work exactly when the evidence is thinnest. The defect itself is
established by reading the head; only its author is not. Keeping it costs the
contributor one reply, which the maintainer-decision route then resolves.
Whether ClawSweeper should prefer the opposite trade is a product call — which
is the same call the live review raised in its own decision packet.
The paragraph is one rule with the parts a rule needs: how to establish
provenance, what stays a finding, where a non-attributable prerequisite goes,
and what to do when the check cannot be completed. The exact decision sequence
is the diff itself, and the linked issue states it as an ordered rule.
Two things worth separating, since they are easy to conflate:
automerge logic.
maintainerDecision.requiredalready routes to a needs-humanverdict today and
fix_before_mergealready authorises an automergeinstruction today; neither contract is edited here.
a genuine pre-existing prerequisite, the same condition should now arrive as a
maintainer decision rather than as contributor rework — so that PR gains a
human-decision pause it might not have had, and loses a finding it should not
have had. Fewer findings and more decision packets on that class of PR is the
intended effect, not a side effect.
Deliberate boundaries:
newly-reachable conditions all remain
reviewFindings.handles only new ones is still incomplete against its own claim, even though
the untouched records predate it. That case stays a finding.
inconclusive, the condition is treated as patch-attributable and the finding is
filed. Blob hydration is best-effort, so a fail-open rule would silently
suppress real defects.
prerequisite goes to
risksplusmaintainerDecision.required, not tomergeRiskOptions. Read off the current source rather than measured in a liverun, so treat these three as maintainer verification points:
isRepairLoopPassReportreadsreviewFindings.lengthbut no merge-riskfield, so a prerequisite parked in merge risk alone still yields
clawsweeper-verdict:passon an automerge-labelled PR;maintainerDecisionis not an input toderivedPrRatingorprStatusLabelKind, so it cannot move patch quality; andfix_before_mergeis the only category permitted to carry
automergeInstruction. The prompt also warns againstfix_before_mergespecifically, because that is the only category permitted to carry
automergeInstruction— it invites the repair lane to widen the PR into thepre-existing debt.
repaired in the same PR or tracked as follow-up is left to the target
AGENTS.md, per the root instruction that generic ClawSweeper prompts stayrepo-agnostic.
prompts/review-commit.mdalso emits findings, but its reviewed unit is one commit against the parent SHA
the prompt is handed directly, so there is no base-tip-versus-merge-base
question to get wrong and no
patchTierfor a misattributed finding to lower.The gap this fixes is specific to PR review.
User Impact
When a prerequisite is established as pre-existing, it stops being contributor
patch debt and becomes a maintainer decision that pauses automated landing, so a
contributor who keeps the focused-PR shape OpenClaw asks for is not scored for
debt their branch did not create.
That qualifier is the honest part. Where provenance cannot be established — the
branch is behind its base, the patch is truncated past the cited lines, or the
base blob was skipped as oversized — the finding stays contributor-facing by
design. The change narrows when a contributor is charged; it does not promise
they never are.
No configuration, schema, or public interface changes.
patchTierFromReview,hasBlockingReviewFindings,isReadyForMaintainerLook,parseReviewFindingand the decision schema are untouched — but that is a statement about their
implementation, not their output. They keep computing what they compute today;
what reaches them changes, which is the whole point, and their results move
accordingly on PRs carrying a pre-existing prerequisite.
OpenClaw Bay: schema unchanged, distributions shift. No decision-schema field is added, removed
or retyped, and the durable report and comment renderers are untouched, so Bay
reads exactly the fields it reads today and no data contract moves. Any view
that aggregates finding counts will see the distribution shift, which is the
intended effect of the change rather than a contract break.
Evidence
Prompt prose plus two tests. No production TypeScript — the hydration path
is unchanged, because what it undertakes was never the problem.
What rests on what, since the claims have different strengths: the two tests are
durable and re-runnable, and carry the procedure's wording and the shallow-cache
behaviour. The Real Behavior Proof below is recorded output from a one-off local harness
run against this PR's live refs. The harness is not a shipped file, but every
git command it issues is quoted in that section, so the measurement can be
repeated by hand. The routing claims — that a prerequisite belongs in
risksplusmaintainerDecision.requiredand notfix_before_merge— are read off currentsource rather than exercised, and are flagged as maintainer verification points
rather than presented as measured.
Root cause
The prompt states that findings are patch-introduced defects but
supplies no procedure for establishing it, so provenance is left to inference
from line numbers.
Fix
This PR changes one paragraph pair in the
reviewFindingssection. Itnames the base and head SHAs the review context already carries, names the two
commands that read the base side, keeps as findings every condition this patch
introduced, worsened, newly reached, or claimed to cover but did not, routes a
genuine pre-existing prerequisite to
risksplusmaintainerDecision.required,warns off
fix_before_merge, and fails closed.Tests added
Two, because a prompt assertion alone cannot speak for the review environment.
test/review-prompt-policy.test.ts— pins every clause of the procedure inthe file's existing
readFileSync+assert.matchidiom, and carriesdoesNotMatchassertions on all three superseded command forms (themerge-base read, the three-dot diff, and the older two-dot
HEADdiff) sonone of them can return. Reverting only the prompt paragraph fails exactly
this test.
test/review-blob-hydration.test.ts— a shallow-cache integration test thatbuilds the review cache the way
ensureReviewTreeCommitdoes (one--depth=1 --filter=blob:nonefetch per side into the production ref names),runs the real
hydratePullRequestReviewBlobs, points the remote at aninvalid URL, and then asserts the boundary in both directions: both commits
resolve,
git merge-baseand the three-dot diff do not, the instructedgit showreads succeed including a renamed file through its previous path,and the head path is absent on the base side. The fixture is deliberately
offline; it is a statement about hydration, not about the network.
The second test is not vacuous: building the same fixture without
--depth=1makes
git merge-baseand the three-dot diff succeed (rc=0 both), so theassertions track the shallow boundary rather than git in general.
Non-scope
This change is limited to the
reviewFindingssection of thereview prompt, its policy test, and one shallow-cache integration test in the
existing review-blob-hydration suite. It carries no rating redesign, no schema
change, no new label or status, no second review pass, and no change to
patchTierFromReview,hasBlockingReviewFindings,isReadyForMaintainerLook,or
parseReviewFinding.Whether a prerequisite is repaired in the same PR or tracked as follow-up stays
the target repository's call.
Limitations
This changes reviewer behavior, and nothing mechanically
enforces that the reviewer performs the check — the same level of assurance
lateFindinghas today. The proof below shows the instruction is delivered andexecutable; it does not and cannot show that the model will comply, which is only
observable from live review runs after this lands.
Review closeout
Six findings changed this patch. Every one of them was a case where the
paragraph would have caused the misattribution it exists to prevent, which is
worth saying plainly rather than presenting the result as if it arrived clean.
From the first live ClawSweeper scan (the reason this PR moved):
git merge-baseand a three-dot diff; the review cache can materialize amissing base or head with
--depth=1, leaving both trees present and theirshared history absent, so both commands fail and the fail-closed branch files
the base-state finding. Accepted. Reproduced against this PR's own refs
with the production fetches (
merge-baserc=1, three-dot rc=128, both commitsrc=0), then repaired by removing ancestry from the procedure entirely rather
than by widening the cache. Covered by a new shallow-cache integration test.
From the local
codex reviewrounds before submission:base.shais the base-branch tip rather than the merge base, andHEADis notreliably the PR head, so the original two-dot
git diff <base-sha>..HEADcouldread base drift as the patch's work.
must use
previous_filename— otherwise the fail-closed rule charges untouchedcode to the contributor.
the new name, so a renamed file diffed as wholly added.
HEAD" contradicted the re-review paragraph inthe same file, which legitimately requires
git diff <earlier-sha>..HEAD.behind its base the two checks could disagree about the same file.
The last four are now moot by construction rather than by instruction: with the
diff gone from the procedure, there is one git read against one snapshot, and
the only path question left is the rename, which the prompt and both tests
cover. The superseded command forms are all pinned absent.
Real Behavior Proof
The previous version of this section proved only that the procedure reaches the
model's composed input. The first live ClawSweeper review was right that this was
not enough: delivery says nothing about whether the instructed commands can run.
This proof now carries both claims, and the second one is measured against this
PR's own refs on the real remote.
Claim 1 — delivery. The attribution procedure reaches the review model's
composed input through the production prompt-composition path, and the base SHA
it refers to travels in the same payload.
Claim 2 — executability. After the review cache materializes the base and
head commits the way it actually does, every git operation the prompt instructs
runs with lazy fetching disabled, and the two operations the prompt forbids are
exactly the ones that cannot run there.
Surface: for claim 1, the built head's
compactPullRequestForTestandreviewPromptForTest, which calls the productionbuildReviewPrompt— nothingstubbed. For claim 2, a repository holding neither commit, then the exact fetch
ensureReviewTreeCommitissues (--force --filter=blob:none <src>:<dst> --depth=1) per side into the production destination refs, againstthis repository's remote and this pull request's own head ref. Blob
hydration then issues the same
fetch --filter=blob:none --stdinthathydratePullRequestReviewBlobsissues for missing bounded blobs; every read isrun with
GIT_NO_LAZY_FETCH=1so nothing can be rescued by the network.Command / environment: Docker-backed Crabbox
local-container, imagenode:24-bookworm, on a disposable container lease, Linux host, againstcommitted head
9b52677592436dd2c7459f28b8ffe7f5717d8181. The sandbox syncs the branch, builds itfrom source, and runs the harness. Crabbox's
--no-hydratebelow is its ownsandbox-provisioning flag and is unrelated to review blob hydration.
Observed result: exit
0, command phase 6.511s.Read: all twenty-seven clauses of the procedure are present in the prompt the
production composer hands the model, none are present in the base copy, and
none of the three superseded command forms survives. The cache really is
shallow; both commits resolve;
git merge-baseand the three-dot diff failthere; and the instructed
git showfails before blob hydration and succeedsafter it — which is what makes it a statement about hydration rather than
about network access. The SHAs in claim 1 are a synthetic PR fixture; the SHAs
in claim 2 are this PR's real base and head.
Rename coverage: carried by
test/review-blob-hydration.test.tsratherthan the harness, because it needs a rename to exist in the fixture. That test
builds the same shallow cache, runs the real
hydratePullRequestReviewBlobs,and proves offline that the base side reads through
previous_filenamewhilethe head path is absent on the base side — the case where a naive reader would
conclude the patch introduced the whole file.
Artifact / trace: the durable fixtures are the two tests. The harness is a
short local script rather than a shipped file, so the tests — not the harness —
are what future readers re-run.
Freshness: this proof and the review closeout describe commit
9b52677592436dd2c7459f28b8ffe7f5717d8181. If the branch is rebased or gains a commit, both areregenerated against the new head rather than carried forward.
Claim 3 — the structured review result (three behavioural examples)
The review asked for the changed structured outcome through the production
review path, which the earlier claims did not carry. Three controlled cases are
now measured. Read them as behavioural examples of what the procedure produces,
not as a reliability measurement.
Path exercised.
clawsweeper review --local-rangeon this head:buildLocalRangeReviewsynthesises the review item and its context from a localgit range →
buildReviewPrompt(the production composer, using this branch'sprompts/review-item.md) →runCodex→ the decision schema → the productionparser and report writer.
--local-rangeis the repository's own GitHub-free review mode: it scrubsGitHub credentials, points
ghat an empty config dir and skips the startcomment, so there are no public side effects. It is not offline in the model
sense — each case still invokes the configured reviewer session.
Coverage boundary. These runs are limited to the production composition,
model invocation, decision schema, parser and report writer. GitHub context
hydration and publication are bypassed, so the inputs the procedure names —
pullFiles[].patch,base.sha,previous_filename, merge state — reach thereviewer from the local range here rather than from GitHub. Claims 1 and 2
cover those inputs; Claim 3 validates the routing decision the policy asks for;
claims 1 and 2 validate that its inputs arrive and are readable. Reported
runtime:
review_model: internal,review_reasoning_effort: high, one modelcall per case — the ordinary review call, with no additional invocation added
by the attribution procedure itself. The proof as a whole made three such
calls, one per case.
Case A — a pre-existing prerequisite, conclusive within the fixture. Base
compares stored rows
against upstream text byte for byte. The patch is limited to adding a projection
match for rows the importer marked, leaving the unmarked path exactly as it was,
and the supplied PR body states that boundary and names the backfill as separate
follow-up work.
The prerequisite is visible, it pauses automation, and it is not charged to the
contributor's patch. That is the routing this PR exists to produce — shown for
this fixture, which is built so the answer is unambiguous. It is not a claim
that provenance is always establishable.
Case B — a patch-introduced defect (control). Same base; the patch adds a new
fork anchor with a real bug of its own.
The control matters: the rule does not simply suppress findings, and a maintainer
decision is not substituted for a defect the patch introduced.
Case C — a latent base defect the patch newly reaches. An unchanged parsing
helper in the base converts its input with no guard; the patch routes an
operator-supplied value into it through the existing compaction entry point.
The defective lines are the base's and the finding is still the patch's, which is
the authorship-versus-attribution distinction stated above, executed.
asked. The model call needs this host's configured Codex session; putting
those credentials inside a container is not something I will do for a proof.
Claims 1 and 2 are container-run; claim 3 is host-run through the same
production entrypoint. If a Docker-backed structured run matters, it needs
to happen on infrastructure that already holds the reviewer credentials.
the intended structured result. They do not measure how reliably a model
obeys it across many runs; that is a separate question from whether the
instruction is correct and executable.
A real PR carries more signal and more noise than any of them.
ensureReviewTreeCommitbuilds it whena commit is missing. When the commit is already present with full ancestry no
shallow fetch happens, and the forbidden commands would work — the prompt
still forbids them, because the instruction must hold in the worse state.
upstream/main's copy of theprompt, not any particular PR's base. That shows the paragraph is new; it is
not evidence about a specific review.
unreadable base side keeps the finding instead of clearing it.
Validation
node --test test/review-prompt-policy.test.ts— exit0, 36/36 pass.node --test test/review-blob-hydration.test.ts— exit0, 6/6 pass, thesixth being the new shallow-cache case. It also runs inside
pnpm run check;the local non-zero exit below comes from a different suite in that command's
coverage phase, not from this test.
The test has teeth: reverting only the prompt paragraph and re-running the same
command leaves 35/36, failing exactly the new test and nothing else.
pnpm exec oxfmt --check test/review-prompt-policy.test.ts— exit0.pnpm run check— exits non-zero (1) on my machine. Stating that plainlyrather than burying it:
check:static,build:alland all fourlint:*targets pass, and the failure is entirely in the coverage test run, which
reports 13 failures out of 3207, all in
test/repair/target-validation.test.ts(pinned-base git reproduction,replacement-branch plumbing, and dependency-setup process reaping under bun
and npm).
Those 13 are pre-existing on this Linux host and not attributable to this
change, established mechanically rather than asserted: stashing the diff and
re-running the same test file on an otherwise unmodified checkout reproduces
the identical 13 failure names, and injecting one failure name that passes on
the base tree correctly reports it as new. The visible causes are
environmental — a coverage profile directory the sandboxed child cannot
create, and a detached package-manager entry point it cannot resolve, with
bunabsent on this host entirely.I have not touched that suite: this PR is limited to the review prompt and its
tests. Upstream CI settles it. The
pnpm checkjob on this head — the samecommand, including the coverage phase that holds all 13 of those cases —
passes:
https://github.com/openclaw/clawsweeper/actions/runs/31284806440/job/93171649120.
CodeQL (https://github.com/openclaw/clawsweeper/runs/93171859965) and the
sparse repair build smoke
(https://github.com/openclaw/clawsweeper/actions/runs/31284806440/job/93171649127)
pass too. So the 13 failures are not reproduced in the upstream CI
environment; treat them as local to my host, not as a property of the change
or of the suite.