fix(review): stop showing zero stats for local ranges - #1312
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 6:25 PM ET / 22:25 UTC. ClawSweeper reviewWhat this changesThe PR obtains whole-range Git line counts for local-range reviews and renders unavailable or incomplete statistics as explicit unknowns instead of verified zero totals. Merge readinessKeep open: this active PR fixes the source-proven local-range zero-count defect, resolves the prior availability concern, and has sufficient native behavior proof; no correctness or security blocker was found. Priority: P2 Review scores
Verification
How this fits togetherClawSweeper's local-range review converts a committed Git range into review metadata and a saved Markdown report. That metadata feeds the OpenClaw PR-surface summary shown to reviewers and any reloaded local history. flowchart LR
A[Committed Git range] --> B[Local range review]
B --> C[Complete file enumeration]
C --> D[Optional Git line counts]
D -->|available| E[Report metadata]
D -->|unavailable| F[Unknown count metadata]
E --> G[PR surface summary]
F --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain authoritative whole-range counts when available and the documented explicit-unknown representation when complete statistics cannot be collected. Do we have a high-confidence way to reproduce the issue? Yes—current source and the PR base establish the deterministic omission-and-zero-coercion path, while the captured body supplies an after-fix native CLI trace; this reviewer did not rerun it. Is this the best way to solve the issue? Yes—the change uses Git's complete range metadata as the count authority and preserves uncertainty rather than inventing numeric totals. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 995ab70f113a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
What Problem This Solves
Fixes an issue where users running
review --local-rangewould see a PR surface table claiming zero additions, deletions, and net change despite reviewing a nonempty committed range. The confirmed six-file reproduction rendered +0/−0 while Git measured +1,933/−509. The model's separatereviewMetricstext did not repair this deterministic metadata defect.This is separate from the scanner fix in #1309 and proof-applicability work in #1310. The defect was observed on
74b0f8552fde46842a933ca360ed272f0212193e; this PR does not attribute its introduction to those changes.Why This Change Was Made
The producer omitted additions/deletions, then both report adapters converted missing values to zero. The fix obtains authoritative whole-range Git numstat and joins it to NUL-framed name/status using exact rename/copy identities. Both local file representations receive the same counts, independently of truncated patches, HEAD-only metadata, per-commit churn, or model-authored metrics.
Required complete file enumeration preserves the verified existing runtime contract: 128 MiB capture capacity and no per-read deadline. Optional numstat remains 1 MiB/five-second bounded best-effort metadata. An unavailable, oversized, timed-out, malformed, incomplete, duplicate, or mismatched stats response discards the entire statistics map and produces null counts without stopping review. Required unreadable or invalid file lists still refuse; they are never replaced with a supposedly complete empty list. The raw-Git reader's defaults and all scanner/provenance caller deadlines and hardening remain unchanged.
Reports preserve unknown counts as JSON nulls. Unknown/binary counts or incomplete lists render an explicit unavailable explanation, not invented zeros or a partial numeric total. Genuine pure-rename/mode-only zero counts remain valid. There are no new test-only production seams.
Review Disposition
The review of
43c6be92944189bf960c3d84b846f64ca5033558accepted the native proof and found no actionable correctness/security findings, but requested a maintainer decision on the newly introduced oversized-metadata refusal. The maintainer-selected outcome is preserve review completion, not accept that new failure mode.Addressed in
8a4dc4675a7f0d409a34babc4c658bfc0ee56cea: optional statistics now degrade to unavailable, and required enumeration retains its actual prior capacity and timeout behavior. This also addresses the review's rank-up request to record that policy. No compatibility-risk override or merge-policy change is being requested. Fresh current-head normal and controlled-fault native proof is below; request re-review against this head and body.User Impact
Reviewers get accurate whole-range surface counts when available, including files and lines beyond display-evidence limits. A failure to collect optional statistics no longer prevents an otherwise valid local review from completing. Historical reports/comments, scanner/provenance enforcement, proof applicability, and merge policy are unchanged by this PR.
OpenClaw Bay Impact
Unaffected: these local file statistics and ordinary report/comment presentation do not change Bay's observer data, routes, or controls.
Documentation Impact
Reviewed the canonical local CLI guidance in
README.mdand updated the active local-review referencedocs/commit-sweeper.md, owned by ClawSweeper maintainers. It documents authoritative counts, mandatory enumeration with its existing limits, bounded optional statistics, unknown/completeness semantics, and scope limits. The local producer, report adapters, and surface renderer own the behavior. The existing one-line changelog entry was updated rather than adding a duplicate note.Real Behavior Proof
Head and claim: on
8a4dc4675a7f0d409a34babc4c658bfc0ee56cea, the real nativereview --local-rangeCLI carries complete whole-range Git counts through local context, persistedpr_surface_files, report reload, the production renderer, and the CLI-written history comment; a bounded optional-statistics failure preserves review completion and explicitly unknown counts.Environment: macOS, Node 24.20.0, pnpm 11.10.0, real native Codex and unchanged host TruffleHog admission. Both runs used fresh clean copies of the supplied six-file synthetic repository, base
38f05fcba9bedbcfc67df41c2164dde77d5e0a8f, head9f92a6cd1689b685f3ad427e7c7b980b0a6d57dc. These are local synthetic identities, not hosted-commit claims. The OpenClaw target profile activates the existing PR surface table. Target-bundled skills/helpers were treated as data and never executed. Original inputs and both proof checkouts remain clean.Normal native Git path
Executed with unmodified Git for every operation; no fake provider or scanner. The configured model identifier and machine-specific paths are omitted below:
Exit 0, 2026-08-30 22:06:10–22:11:55 UTC. Independent whole-range Git, every persisted per-file count, native saved history, and fresh rendering after report reload agree:
Exact production table total:
Native application with controlled optional-statistics overflow
The second run kept the real application, Codex, scanner, and Git operations, with one explicitly controlled external-process fault: a trusted task-local Git wrapper padded only the first host numstat response with leading zeros. The original Git counts, paths, and NUL framing remained mathematically unchanged, but the response grew from 335 bytes to 2,097,487 bytes, beyond the optional 1,048,576-byte limit. All other Git operations executed the real Git binary. This is fault injection, not a claim that this six-file fixture naturally creates oversized metadata, and it adds no production test hook.
The same CLI invocation ran with that wrapper prepended to
PATH, in a separate fresh fixture. Exit 0, 2026-08-30 22:06:10–22:12:00 UTC. Verified persisted/runtime results:Exact visible PR surface:
Artifact/trace: the normal and unavailable cases retain separate
0.md,comment.md, sanitized invocation/diagnostics, and machine-readable comparisons under ignoredartifacts/local-range-stats-compatibility-20260830/. The fault receipt records the exact range, one injected call, transformation, and byte sizes. Comparisons assert the exact implementation head and fixture endpoints, complete path preservation, clean checkouts, persisted values, and native-history/reloaded-renderer agreement. Observable results are reproduced inline; local artifacts are not claimed as public download links. Model metrics are never the oracle.Limits: this proves the local metadata/persistence/rendering and controlled metadata-failure paths, not behavior of the fixture's own helper, hosted publication, scanner detection completeness, or merging. No historical comment was edited.
Evidence
The retained real-Git regression uses two commits after base, an earlier-only change and later reversal, 87 changed paths, A/M/D plus an edited rename, and a patch beyond 512 KiB. It verifies +6,087/−4 through both representations, real CLI persistence/reload, and saved review history while separate introduction evidence is capped at 80 paths/24K characters. Binary counts, real zeros, copies, special paths, failed/malformed reads, invalid values, and incomplete lists are covered.
Compatibility regressions cover graceful CLI completion with failed/oversized/malformed numstat, required-list refusal, synthetic required enumeration beyond 2 MiB through existing production factories, and valid required enumeration delayed 5.5 seconds while optional numstat still times out to unknown. The capacity test asserts the shared existing 128 MiB contract; it does not allocate 128 MiB or claim a native filesystem tree at that limit. These fault-injected tests use the existing fake provider/scanner harness and support, rather than replace, the native proofs above.
On the current head,
build:all, static/documentation/format/lint gates, and 172 focused tests passed on Node 24. Fresh precommit and committed-branch Codex autoreviews were scoped-clean at their default P0 threshold; broader findings coverage is not claimed from those passes.Earlier validation included a full coverage pass of 4,099 passed, zero failed, nine skipped at supported concurrency 4 after four unchanged repair-toolchain setup deadlines at default concurrency. A Node 26 post-rebase local run hit one outer CLI timeout; that test passed unchanged on Node 24. No test deadlines or coverage thresholds were raised. Hosted CI on the previous PR head passed all required jobs; updated-head CI and current-head/body ClawSweeper review remain landing gates.