Skip to content

fix: checkpoint malformed maintainer decisions during apply#416

Merged
steipete merged 1 commit into
openclaw:mainfrom
brokemac79:codex/fix-maintainer-decision-apply-checkpoint
Jul 5, 2026
Merged

fix: checkpoint malformed maintainer decisions during apply#416
steipete merged 1 commit into
openclaw:mainfrom
brokemac79:codex/fix-maintainer-decision-apply-checkpoint

Conversation

@brokemac79

@brokemac79 brokemac79 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Stamp malformed maintainer_decision apply records with apply_checked_at before recording the kept-open result.
  • Preserve the no-GitHub-mutation behavior for malformed decision frontmatter.
  • Strengthen the regression test so two malformed records with --processed-limit 1 advance from the first record to the second on the next apply run.

Problem

The decision-packet rewrite in #358 added a maintainer_decision parse step in the apply loop. When frontmatter contained malformed maintainer decision JSON, the catch path returned a kept-open result and advanced processedCount, but did not persist apply_checked_at. That let the same malformed report keep sorting as unchecked and repeatedly consume bounded apply queue slots.

Implementation

  • In the malformed maintainer_decision catch path, write apply_checked_at directly to the report markdown when not in dry-run mode.
  • Avoid writeReportMarkdown in this specific path because decision-packet sync reparses the malformed frontmatter and would fail again.
  • Leave the result action and reason as kept_open / invalid maintainer_decision: ... and do not call GitHub.

Validation

  • pnpm run build:all passed.
  • node --test test\clawsweeper.test.ts passed: 69 tests.
  • pnpm run lint:src passed.
  • pnpm run lint:scripts passed.
  • pnpm exec oxfmt --check src\clawsweeper.ts test\clawsweeper.test.ts passed.
  • git diff --check passed.
  • $env:Path = 'C:\msys64\usr\bin;' + $env:Path; node --test --test-skip-pattern "dispatch receipt gate|--local-range" test\*.test.ts passed: 663 tests, 662 pass, 1 skipped.
  • Live CLI proof below passed with node dist/clawsweeper.js apply-decisions against two malformed reports and a fatal mock gh.

Proof

The updated malformed-decision test creates 321.md and 322.md, both with invalid maintainer_decision JSON, and runs apply twice with --processed-limit 1 while the mock gh exits if called. The first run stamps 321.md with apply_checked_at; the second run processes 322.md, proving the malformed first record no longer monopolizes the bounded apply slot and no GitHub mutation is attempted.

Live apply-run proof from a local built checkout, with temp paths redacted:

Proof root: $PROOF_ROOT
Command: node dist/clawsweeper.js apply-decisions --target-repo openclaw/clawsweeper --items-dir $PROOF_ROOT/items --closed-dir $PROOF_ROOT/closed --plans-dir $PROOF_ROOT/plans --report-path $PROOF_ROOT/apply-report.json --processed-limit 1 --min-age-days 0 --close-delay-ms 0
GitHub mock: GH_BIN=node, GH_BIN_ARGS=[$PROOF_ROOT/bin/gh.js]; mock appends to $PROOF_ROOT/gh-called.log and exits 1 if called.

Run 1 output:
[apply] 2026-07-05T11:27:58.263Z starting apply: files=2 dry_run=false apply_kind=issue min_age=0 days apply_close_reasons=all stale_min_age_days=60 close_delay_ms=0 sync_comments_only=false comment_sync_min_age_days=0 max_runtime_ms=0 item_numbers=all closed=0/20 processed=0/1 counts={}
[apply] 2026-07-05T11:27:58.268Z finished apply closed=0/20 processed=1/1 counts={"kept_open":1}
[
  {
    "number": 321,
    "action": "kept_open",
    "reason": "invalid maintainer_decision: maintainer_decision must contain valid JSON"
  }
]

Run 1 apply-report.json:
[
  {
    "number": 321,
    "action": "kept_open",
    "reason": "invalid maintainer_decision: maintainer_decision must contain valid JSON"
  }
]
Run 1 checkpoints:
321: apply_checked_at: 2026-07-05T11:27:58.267Z
322: <none>
Run 1 GitHub mock called: False

Run 2 output:
[apply] 2026-07-05T11:27:58.385Z starting apply: files=2 dry_run=false apply_kind=issue min_age=0 days apply_close_reasons=all stale_min_age_days=60 close_delay_ms=0 sync_comments_only=false comment_sync_min_age_days=0 max_runtime_ms=0 item_numbers=all closed=0/20 processed=0/1 counts={}
[apply] 2026-07-05T11:27:58.388Z finished apply closed=0/20 processed=1/1 counts={"kept_open":1}
[
  {
    "number": 322,
    "action": "kept_open",
    "reason": "invalid maintainer_decision: maintainer_decision must contain valid JSON"
  }
]

Run 2 apply-report.json:
[
  {
    "number": 322,
    "action": "kept_open",
    "reason": "invalid maintainer_decision: maintainer_decision must contain valid JSON"
  }
]
Run 2 checkpoints:
321: apply_checked_at: 2026-07-05T11:27:58.267Z
322: apply_checked_at: 2026-07-05T11:27:58.387Z
Run 2 GitHub mock called: False

I also attempted unfiltered pnpm run test:unit. On this Windows host it fails in environment-sensitive tests unrelated to this change: dispatch-receipt-owner needs jq in the shell path, and two local-range-review tests do not produce their fake Codex marker files. The skip-pattern run above covers the rest of the top-level suite.

Risks / Rollout

  • Low risk: the change is limited to the malformed maintainer-decision validation catch path.
  • No workflow files are touched.
  • Dry-run still avoids writing report markdown.

Links

Codex Review Closeout

  • Command: C:\msys64\usr\bin\bash.exe C:/Users/marti/.codex/skills/codex-review/scripts/codex-review --mode branch --base origin/main --codex-bin C:/Users/marti/AppData/Local/Temp/codex-fast-wrapper.sh --output C:\Users\marti\AppData\Local\Temp\codex-review-pr416.txt
  • Nested review target: codex -c 'service_tier="fast"' review --base origin/main from codex/fix-maintainer-decision-apply-checkpoint against actual PR base origin/main (44d3b656300f647a20e1c82a48e84f8ad24225f9). The wrapper only corrected the local CLI config's rejected service_tier=default; model remained gpt-5.5 with xhigh reasoning.
  • Result: codex-review clean: no accepted/actionable findings reported. Codex review summary: malformed decision records now stamp apply_checked_at, processed-limit runs can advance to later items, and the focused test covers that rotation.
  • Accepted findings: none.
  • Rejected findings: none.
  • Rerun validation after review: pnpm run build:all passed; node --test test\clawsweeper.test.ts passed (69 tests); direct node dist/clawsweeper.js apply-decisions ... --processed-limit 1 --min-age-days 0 --close-delay-ms 0 proof passed twice. The proof showed run 1 processed/stamped only item 321, run 2 processed/stamped item 322, and the fatal gh mock was not called.

@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 9:12 AM ET / 13:12 UTC.

Summary
The PR stamps malformed maintainer_decision apply reports with apply_checked_at and extends regression coverage for processed-limit rotation without GitHub calls.

Reproducibility: yes. Current main sorts missing apply_checked_at as unchecked, and the malformed maintainer_decision catch path does not write that checkpoint; I did not run the mutating apply harness in this read-only checkout.

Review metrics: 3 noteworthy metrics.

  • Patch Size: 2 files changed, +36/-1. The diff is small enough to audit directly but touches live apply bookkeeping.
  • Apply Surface: 1 malformed-decision catch path changed. The behavior is scoped before any GitHub mutation path.
  • Runtime Proof Shape: 2 apply runs, 2 malformed reports, 0 GitHub mock calls. The posted transcript covers bounded-slot rotation and mutation safety.

Root-cause cluster
Relationship: canonical
Canonical: #416
Summary: This PR is the canonical current fix for the malformed maintainer-decision apply checkpoint bug; related PRs either introduced the broader decision-packet path or cover adjacent apply automation.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] The diff intentionally bypasses writeReportMarkdown for malformed decision frontmatter, so maintainers should confirm the direct write remains limited to apply_checked_at and dry-run guarded.
  • [P1] This changes live apply queue bookkeeping; a regression could affect ClawSweeper automation throughput even though the malformed path still avoids GitHub mutation.

Maintainer options:

  1. Accept The Scoped Checkpoint Fix (recommended)
    Merge after maintainer review because the patch is small, proved against the apply entrypoint, and preserves fail-closed GitHub behavior.
  2. Request Maintainer Apply Smoke
    Ask for a maintainer-run two-record apply smoke if maintainers want independent confirmation before landing an apply-state write.
  3. Pause For A Different Malformed-Record Policy
    Pause only if maintainers prefer quarantine or another malformed-report policy instead of timestamping and keeping the report open.

Next step before merge

  • [P2] No repair job is needed; review found no concrete patch defect, and the remaining action is ordinary maintainer merge review for apply automation.

Security
Cleared: No concrete security or supply-chain concern found; the diff only timestamps an existing report file in a malformed-input apply path and updates tests.

Review details

Best possible solution:

Land the narrow catch-path checkpoint with regression coverage and live proof if maintainers accept the intentionally direct report write for malformed frontmatter.

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

Yes. Current main sorts missing apply_checked_at as unchecked, and the malformed maintainer_decision catch path does not write that checkpoint; I did not run the mutating apply harness in this read-only checkout.

Is this the best way to solve the issue?

Yes. The direct timestamp write is the narrowest maintainable fix because the normal report writer would reparse the malformed maintainer_decision frontmatter.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: Malformed generated reports can repeatedly consume bounded apply slots, which is a normal-priority automation bug with limited blast radius.
  • merge-risk: 🚨 automation: The PR changes live apply report writes and queue rotation behavior, so maintainers should notice automation risk despite the small scope.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes redacted live apply-decisions output proving the after-fix two-run behavior and no GitHub mock invocation.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted live apply-decisions output proving the after-fix two-run behavior and no GitHub mock invocation.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its read-only review, apply-lane safety, and generated-state guidance affected this review. (AGENTS.md:1, 44d3b656300f)
  • Current-main bug path: Current main catches malformed maintainer_decision, records a kept-open result, and increments processedCount without writing apply_checked_at. (src/clawsweeper.ts:18045, 44d3b656300f)
  • Queue ordering impact: Reports with missing or invalid apply_checked_at sort as unchecked because applyQueueSortFields falls back to 0. (src/clawsweeper.ts:5440, 44d3b656300f)
  • Patch implementation: The PR head adds a direct apply_checked_at frontmatter update and writes the report only when not in dry-run mode. (src/clawsweeper.ts:18050, 9ef9e68f1f18)
  • Regression coverage: The updated test creates two malformed reports, runs apply twice with --processed-limit 1, and asserts each report is stamped in turn while the fatal gh mock is not reached. (test/clawsweeper.test.ts:449, 9ef9e68f1f18)
  • Real behavior proof and checks: The PR body includes redacted live apply-decisions output showing two bounded runs, item 321 then item 322, with no GitHub mock call; GitHub reports the PR clean with CI and CodeQL checks successful. (9ef9e68f1f18)

Likely related people:

  • brokemac79: The malformed maintainer_decision apply parser was introduced by the merged decision-packet work, and nearby apply_checked_at queue rotation helper work also traces to this contributor. (role: introduced behavior and recent area contributor; confidence: high; commits: 232898fb6edc, 5bdf6f31c904; files: src/clawsweeper.ts, test/clawsweeper.test.ts)
  • Vincent Koc: Blame shows the surrounding apply skip/result bookkeeping and queue sort behavior tracing to earlier exact-review admission and apply-loop work. (role: original apply-loop contributor; confidence: medium; commits: f92fbfebbe8f; files: src/clawsweeper.ts)
  • Peter Steinberger: Recent current-main commits changed adjacent apply-state and stale-proof paths around the same apply command surface. (role: recent adjacent contributor; confidence: medium; commits: 44d3b656300f, cdcb851924a7; files: src/clawsweeper.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (5 earlier review cycles)
  • reviewed 2026-07-05T11:21:02.191Z sha 9ef9e68 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-05T11:24:52.476Z sha 9ef9e68 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-05T11:31:53.742Z sha 9ef9e68 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T11:36:00.778Z sha 9ef9e68 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T13:07:08.265Z sha 9ef9e68 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 5, 2026
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 5, 2026
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@steipete steipete merged commit f62724d into openclaw:main Jul 5, 2026
17 checks passed
steipete added a commit that referenced this pull request Jul 5, 2026
Keep automatic apply windows responsive by running fast candidates first and admitting at most one expensive PR close-coverage proof per window.

Fast and proof cursors advance independently from the exact executor trace, so interrupted or partial windows preserve unexamined work. Coverage proof, freshness checks, protected-label gates, and explicit targeted applies remain unchanged.

Includes the maintainer changelog credit for #416#419.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary 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.

2 participants