fix(review): preserve non-executable live-proof reports - #1277
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 automation
|
Codex review: needs maintainer review before merge. Reviewed August 28, 2026, 3:06 AM ET / 07:06 UTC. ClawSweeper reviewWhat this changesThe PR writes empty live-proof step lists as Merge readinessKeep open: this is a coherent, narrow correction to persisted live-proof report parsing, with current-head terminal evidence and no actionable introduced defect found. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherClawSweeper renders review decisions into persisted Markdown reports, then parses those reports before deciding whether a live-proof plan should be skipped or prepared for execution. This change sits at that renderer/parser boundary and affects review-job automation rather than target-repository code. flowchart LR
A[Review decision] --> B[Report renderer]
B --> C[Persisted Markdown report]
C --> D[Report parser]
D --> E{Runnable plan?}
E -->|No| F[Skip target execution]
E -->|Yes| G[Inspect and drive proof]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the canonical empty-array format with narrowly scoped legacy support while retaining strict malformed-payload and raw-marker rejection. Do we have a high-confidence way to reproduce the issue? Yes. The prior renderer represented an empty list as Is this the best way to solve the issue? Yes. Correcting the renderer/parser codec while retaining one explicitly constrained legacy representation is the narrowest maintainable fix. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against af8f89fe69e1. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles) |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
1 participant
What Problem This Solves
Fixes an issue where a valid non-executable live-proof plan caused ClawSweeper's review job to fail before it could finish publishing the review. Both
declined_suspiciousandnot_applicableplans legitimately have an empty entry and no steps. The report renderer wrote that empty list as- none, but the report parser tried to JSON-parsenoneand marked the plan invalid before the inspector reached its normal no-execution skip.This was observed in the retained report from the earlier PR 1269 review run. That PR's unrelated findings were subsequently fixed and merged; this follow-up does not alter PR 1269 or the separate single-line generation fix.
Why This Change Was Made
The fix belongs at the report renderer/parser boundary. New reports encode empty
Steps:as bare[]; nonempty steps keep their existing JSON-object-per-bullet encoding. The namedlegacy-empty-list-v1contract preserves already-produced reports containing a solitary- none, with surrounding whitespace allowed.The dedicated payload parser accepts exactly one complete representation. Missing, malformed, mixed, duplicated, or noncanonical empty payloads fail closed. Only exact standalone production verification/recording markers delimit an attached suffix; its validation remains with the existing owner. The decision parser still validates the result, so recommended plans with empty steps remain invalid. The inspector's invalid-plan-before-skip guard, execution gates, strict command parsing, terminal completion, one-shot sequencing, and attached-verification guards are unchanged.
User Impact
Valid declined or non-applicable plans survive persisted-report inspection and execution orchestration without launching anything. Their review results can proceed through the existing publication path rather than failing on an empty-list formatting mismatch. Malformed plans still fail before target work starts; this does not create an execution fallback or permission.
OpenClaw Bay Impact
None. Observer schemas, public APIs, lifecycle states, dashboard projections, and publication ownership do not change. This is an internal persisted-report codec correction.
Documentation Impact
Updated the active
docs/live-proof.mdrunbook with the canonical empty representation, the narrowly scoped legacy contract, strict rejection rules, and attachment boundaries; added a one-line changelog entry. ReviewedAGENTS.mdandCONTRIBUTING.md. No policy, schema-version, dependency, configuration, or workflow changes.Review Finding Disposition
Accepted and corrected P2: Match attachment-marker parsing exactly. At the previous head
fae183ab38e015259231bc762420e7efe112c9c2, the plan parser trimmed marker lines (and the outer section), while attached verification required exact raw lines. Parent reproduction confirmed three malformed reports were admitted as candidate 42 even though verification reported the marker absent.The correction preserves raw section boundaries, including EOF and CRLF pairs, and matches raw marker lines before trimming only scalar/Steps payload data. The existing verifier, inspector and execution guards are unchanged. Added regression coverage spans leading/trailing spaces, tabs and Unicode whitespace; both marker types; LF/CRLF; report EOF and next-section boundaries; exact-marker positive controls; and separate malformed-attachment validation ownership. The original three report fixtures were replayed unchanged and now fail closed in both real CLI modes. No bot autofix/automerge command or policy override was used.
Evidence
pnpm run checkof the pre-correction source later committed asfae183ab38e015259231bc762420e7efe112c9c2failed: 3,910 passed, 12 failed, 9 skipped (3,931 total; separate changed-surface coverage tests passed 12/12). The failures are deadline/dispatch/setup-budget assertions and terminal cleanup failures in four unchanged test files. Source/path diagnosis found no codec rejection behind them: the retry/deadline fixtures do not invoke this codec, direct terminal cases bypass it, and the sanitized-child case accepted the report and failed later in terminal cleanup.Focused command:
Real Behavior Proof
Claim and source. Valid non-executable plans must complete the actual ClawSweeper report/CLI path with no candidates and no target execution, while malformed plans still fail. The tested source is
9e784ab9bc5767288dd9eceabb65ca852654a441, integrated onto9bad4750e84b4666c0b5e616d0f8c7f191744496; continuity from the tested pre-commit files to the committed tree is verified before publication. Before-fix proof ran atf3883a32631b930965fbeaeae4dc70761f2b7647; the pre-patch renderer, report parser, and inspector are byte-identical at that baseline and the integration base.Environment. Local macOS arm64, Node 24.20.0, pnpm 11.10.0; installed Bash 5.3.15 for workflow fixtures.
AGENTS.md's Docker-backed Crabbox clause explicitly applies to a Windows host; this proof ran on macOS. This is not a proof waiver: the general requirement for actual controlled runtime behavior is satisfied below, and no policy or gate has been changed. No container/image/lease claim is made.Actual production path, not a mocked substitute. The production decision parser and renderer generate the reports, then separate real
dist/clawsweeper.js live-proof-reviewprocesses run in both--inspectand execution-orchestration modes. The repository profile is enabled. The independent child processes have Node filesystem-read permission only and no filesystem-write or child-process grant, with a minimal environment and no injected hooks or mocks. For this change, launching a target would be a failure of the claim; correct behavior is completing ClawSweeper's own runtime path without target execution.Before the fix, the original producer's empty reports failed with
live proof plan for 42 is invalid. After the empty-plan fix and raw-marker correction:{"candidates":[],"recordMedia":false,"requiresBrowser":false,"requiresTerminal":false}No target checkout or output directory was created. The absence of target execution is also enforced by the child permission boundary, not just inferred from an empty result.
Normalized CLI commands below use fixture-path variables. The records directory contains the generated synthetic
42.md; the target/output paths do not exist. Each case uses its own fresh fixture directory. The production build was created through the repository's build wrapper:Original incident artifact. After the marker correction, the parent also reran the fixed built CLI in
--inspectmode against the actual retained1269.md, without changing it, granting write/child permissions, or executing any command contained in it. It returned exit 0 and the exact empty inspection object above. The retained report and failed-step log hashes matched before and after. This was read-only local artifact inspection, not a PR review rerun or live operator action.Artifact fingerprints. Full per-invocation reports, arguments, stdout/stderr, and receipts are retained locally. The public proof above contains the observed outputs without publishing agent transcripts.
7663b497752bdf9a1408fcb788b46dc8d8635fc5c94688de70228553c2b413da591232e076430f432d51e049c29d78120975df94310aa6e35dc3d48fc6efc77a304a0927bf3fb209150223c8aa67fc864495ab52d54ed9582aadae7a10007be23bdd6d23880fa53727357260739c6347caa9f04be43dff121712e256f5db99c1ccc572167ab409ea645bfee810cf1b42d1fb773d3146564b1b91cbd816afe486f0ef09cb76c1b918401845a79d0f2bce82ce9c4c0fdb89a38975609aa744521d6248238a1726b78d24cde54a10b4e9313776719bc8e08971615b539a8d77b53fScope and Limits
The proof exercises actual local ClawSweeper report parsing and CLI inspection/orchestration. It does not claim target execution, hosted publication, media/UI behavior, a live queue operation, or a deployed rollout. Existing unit/integration tests exercise their own synthetic fixtures. Historical reports and logs were not rewritten, and no live apply/close, repair, or workflow dispatch was used for this proof.