fix(apply): revalidate same-author pairs before closing - #1499
Conversation
Revalidate current close policies and known same-author counterparts immediately before close mutations. Preserve pair identity on failed refresh, re-evaluate reopened counterparts, and retain current lease, source, managed-locale and implementation-provenance guards. Supersedes #1042. This guards eligibility without claiming atomic remote closes. Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
@clawsweeper re-review Head The actual compiled apply-decisions CLI was run against a strict synthetic transport with no live-gh fallback. Main closed the parent after counterpart locking, failed refresh, and reopening; this head suppressed all three closes with explanatory pair skips. Both versions closed the stable pair in the same order. The unreadable-counterpart scenario's later nonzero exit is expected and retains the earlier parent skip in its partial report. The executable proof and limits are in the PR body; no real GitHub item was mutated. Exact-head CI: https://github.com/openclaw/clawsweeper/actions/runs/34247581665. The PR body records the full proof and the withdrawn, unreachable author-budget allegation. This is eligibility revalidation, not atomic remote closure. |
|
🦞👀 Re-review progress:
|
|
@clawsweeper re-review The prior review request could not reach review because main's early sparse checkout hid the setup-pnpm local action. #1498 has now reverted that regression. Please review this unchanged prepared head and current body; the runtime proof and clean independent review are already recorded there. This requests review only. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 8, 2026, 12:45 PM ET / 16:45 UTC. ClawSweeper reviewWhat this changesRechecks close eligibility and same-author issue/PR counterparts immediately before closing, with regression coverage and a native GitHub CLI proof harness. Merge readiness✅ Ready for maintainer review This repair remains necessary on current main. No blocking patch defect was found, and the native-client evidence resolves the previous proof blocker. Priority: P2 Review scores
Verification
How this fits togetherClawSweeper’s apply lane turns reviewed closure proposals into GitHub mutations. These guards compare proposals with current item and counterpart state before allowing a close. flowchart TD
A[Reviewed closure proposal] --> B[Candidate eligibility]
B --> C[Fresh counterpart and policy checks]
C --> D[Closeout note]
D --> E[Final mutation guards]
E -->|Eligible| F[GitHub close and archive]
E -->|Changed or unreadable| G[Keep item open]
Before mergeNone. 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. Technical reviewBest possible solution: Keep candidate checks inexpensive while enforcing fresh eligibility at the final close boundary, preserving independent issue archival and documented non-atomic semantics. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source retains cached pair admission, and the contributor records before/after runs injecting counterpart drift after the closeout note; this review did not execute them. Is this the best way to solve the issue? Yes. Reusing the existing policy evaluator and refreshing known counterparts at mutation boundaries addresses the stale decision without adding a competing policy or changing pair ordering. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2690dafa8c33. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
The native-client proof is committed at
The traces contain actual native REST and GraphQL requests, including the successful stable close mutations. Current v2 review-activity cursors are used. The unreadable counterpart's later standalone processing exits nonzero as expected; the parent skip is preserved in the partial report. No live GitHub item was mutated. The earlier full local check passed with 5,639 tests passing and 13 skipped. The repeated proof-update check had one transient maintenance-fixture diagnostic failure; its isolated retry passed unchanged with a fresh private temporary directory. No assertion or timeout was weakened. Exact-head CI: https://github.com/openclaw/clawsweeper/actions/runs/34252522153. The current PR body records the proof, limits, and resolved fixture findings. The current ClawSweeper review is already running; this comment records proof without requesting a duplicate run. |
2 participants
What Problem This Solves
A same-author issue/PR pair can pass candidate admission, then drift before the final close. Main reuses earlier pair and close-policy facts, allowing the parent to close even when the counterpart has locked, reopened, or become unreadable.
Why This Change Was Made
Share the existing close-reason evaluator between candidate and terminal checks. Keep the candidate prefilter, but bypass its policy and generation caches before the closeout note and final close mutation. Revalidate known counterpart identities even when a prior snapshot was closed, and retain known pair context when a refresh fails.
This is a current-main repair of #1042, preserving @vincentkoc's contribution. The original branch does not allow maintainer edits. Existing managed-locale protections, lease guards, source snapshots, and implementation-provenance issue-first/archive sequencing remain in force. Changelog credit is collected in the final notes PR.
User Impact
The current item stays open if its known counterpart is no longer eligible or cannot be revalidated. Stable pairs preserve their existing processing order. This does not make two GitHub mutations atomic: a remote change after the last read or a later independent mutation failure remains possible.
Real Behavior Proof
Claim and surface: the real compiled
apply-decisionsCLI checks the counterpart again after its actual parent closeout-note write and before closing. The candidate is compared with baseline2690dafa8c3382d05e733af9dfdc31c804b7305d(the reverted main tree equals the previously proved baseline) using the same stateful fixture.Command and environment: Node 24.20.0 on macOS;
pnpm run build;node docs/proof/paired-close-drift/run-proof.mjs. Private items, closed, reports, plans, canonical baselines and runtime directories; selected synthetic item numbers;--skip-dashboard. The native GitHub CLI 2.100.0 uses its documentedhttp_unix_socketsetting to send real REST and GraphQL requests to a private local HTTP service. Only a synthetic token is supplied, and an unreachable external proxy prevents fallback network access. Its synthetic repository namespace is only a fixture; no external repository is read or changed.skipped_same_author_pairArtifact: runnable driver, local API service and instructions in
docs/proof/paired-close-drift/; the driver writes per-case mutation traces, apply reports and a compactsummary.json.Limits: native gh and actual HTTP request/response handling over a Unix socket, with a local server modeling GitHub data. This does not prove TLS, live GitHub availability, or production authorization; no live GitHub mutation occurs. The unreadable-counterpart scenario eventually exits nonzero when the CLI independently processes that unreadable issue; its partial report and mutation trace prove that the parent was kept open. Current v2 activity cursors and general pair processing are exercised directly; the implementation-linked issue-first path retains focused regression coverage. This is eligibility revalidation, not a remote transaction or rollback guarantee.
Evidence
The build, all 156 focused policy/pair/apply tests, and the eight baseline/candidate runtime cases pass. Local Codex autoreview is clean through P2. Prepared head:
43db4edcf22e21afab44b81ba15ccd92be91a824. Full localpnpm checkpassed: 5,639 passed, 13 skipped, zero failures, plus static, build, lint and coverage gates. The preceding production head passed full CI: https://github.com/openclaw/clawsweeper/actions/runs/34247581665. Current-head CI is available in this PR’s Checks. The native proof update passed local Codex autoreview through P2; the unchanged production guards were already reviewed clean.The earlier author-budget review allegation was withdrawn: pair admission is PR-to-issue only, the live counterpart must match the issue kind, and the unchanged capacity owner rejects author-budget closes on issues. The parent PR still uses its run-aware gate. No unnecessary budget-policy change was made.
OpenClaw Bay Impact
None. This changes internal apply eligibility and mutation sequencing, with no public status, observer schema, or Bay action controls.
Native Client Proof Upgrade
The replacement
gh.cjsexecutable has been deleted. The proof now launches the unmodified/opt/homebrew/bin/gh2.100.0 and a separate local HTTP API process. Native gh serializes label and PR-close GraphQL mutations and issue-close REST requests, parses server responses, and reports HTTP 422 for the unreadable counterpart. The fixture supplies current v2 review-activity cursors.All eight baseline/candidate scenarios completed with the expected result. In the three drift cases the candidate trace contains zero close mutations and zero native
closePullRequestrequests; the baseline sends the parent close. Both stable runs contain native PR-close GraphQL and issue-close REST requests. The main PR body now describes this stronger proof; the former replacement-client limitation is resolved.The final fixture also rejects unknown GraphQL target/label IDs and non-GET requests to read-only routes. Native negative controls assert failure and unchanged fixture state. Every gh invocation—including version and negative probes—uses a disposable HOME, XDG state and configuration directory, with telemetry disabled. A repeated local full check had one transient maintenance-fixture failure (empty stderr for invalid arguments); that unrelated test passed its isolated retry with a fresh private temporary directory. No assertions or timeouts were weakened.