Skip to content

fix(preflight): consume exact bot comment states - #311

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/exact-merge-informational-comments-20260807
Draft

fix(preflight): consume exact bot comment states#311
vincentkoc wants to merge 1 commit into
mainfrom
fix/exact-merge-informational-comments-20260807

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

Summary

  • accept the exact-head trusted Dependency Guard informational state without treating its advisory security sentence as an unresolved finding
  • consume a versioned ClawSweeper review-state tuple, with full-comment checks for transitional output and fail-closed handling for malformed, unknown, partial, or duplicate state
  • preserve blocking behavior for stale or blocked dependency states, findings, security concerns, nonempty Before Merge sections, appended objections, and human requests

Root cause

OpenClaw PR openclaw/openclaw#120232 was blocked before checkout and validation:

The dependency classifier understood authorization and clearance states, but not the producer's exact-head trusted informational state.

The ClawSweeper classifier depended on legacy prose and ignored everything after <details>. Current output is structured differently, and the observed comment is internally contradictory: it says there are no findings and Before Merge is empty, then asks maintainers to confirm migration or upgrade compatibility before merge inside the details section.

State contract

Clownfish now accepts the existing clawsweeper-review-version marker as the versioned producer/consumer boundary. The producer-ready v1 shape is:

<!-- clawsweeper-review-version item=<pr> sha=<40-hex-head> v=1 readiness=ready findings=none security=none before_merge=none ... -->

Rules:

  • item, sha, and v=1 must match the current pull request and exact head
  • all four state fields must be present together
  • readiness is ready or blocked
  • findings, security, and before_merge are none or actionable
  • unknown versions or values, partial tuples, and duplicate markers fail closed
  • a v1 marker with none of the state fields is transitional and is accepted only when the full comment has clean readiness, Findings, Security, and Before Merge evidence
  • structured state never overrides contradictory visible prose, nonempty sections, or explicit merge objections

The current openclaw/openclaw#120232 (comment) therefore remains blocking until the producer removes the contradictory pre-merge instruction and emits a consistent state tuple.

Proof

  • node --test test/preflight-external-pr-merge.test.mjs: 172 passed
  • npm run validate: 6,707 jobs validated
  • node --check scripts/preflight-external-pr-merge.mjs
  • git diff --check
  • local autoreview: clean, no accepted/actionable findings
  • TruffleHog pre-review scan: clean

The focused regressions cover the two observed comments plus stale, blocked, unknown-role, duplicate-marker, appended-objection, unknown-version, missing-version, partial-state, findings, security, nonempty Before Merge, and human-request variants.

LOC

  • production: +190 / -11
  • tests: +354 / -2

The production increase implements the requested versioned comment-state contract and fail-closed security boundary; it does not add a second merge path or compatibility fallback.

Punchcard-Session: amber-timber-river-t1
@vincentkoc vincentkoc self-assigned this Aug 7, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Aug 7, 2026
@clawsweeper

clawsweeper Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 7, 2026, 12:15 PM ET / 16:15 UTC.

ClawSweeper review

What this changes

This PR updates external-merge preflight to consume exact Dependency Guard and ClawSweeper review states while rejecting contradictory bot comments.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked by patch quality or review findings - 6 items remain

Keep open: the new transitional v1 path can accept a review that omits Findings and Security evidence, contradicting the stated fail-closed contract. Likely related person: github-actions[bot] (current-main parser provenance; low confidence).

Priority: P1
Reviewed head: d9e138fd1d45569c3f02e9cc1bbd4646d5992398

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch has focused tests and a clear goal, but its new transitional path leaves a high-impact fail-open case.
Proof confidence 🌊 off-meta tidepool Not applicable: This MEMBER-authored internal preflight change is covered by focused fixture-based validation; no external contributor proof gate applies.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This MEMBER-authored internal preflight change is covered by focused fixture-based validation; no external contributor proof gate applies.
Evidence reviewed 3 items Transitional-state bypass: A v1 marker with none of its four state fields is classified transitional, and the ready-review path returns true after checks that do not require Findings or Security evidence to exist.
Missing-field check: The current-format ready predicate requires Merge readiness and Before merge only; subsequent table parsing rejects non-none cells but accepts absent Findings and Security rows.
Current-main provenance: The existing preflight review parser on current main is attributed by blame to the current automation-recorded source snapshot; the proposed parser changes are new to this PR.
Findings 1 actionable finding [P1] Require complete transitional review evidence
Security Needs attention Incomplete transitional state can pass preflight: A malformed producer comment can omit its Findings and Security evidence yet be accepted as a benign current-head review, weakening the intended fail-closed review gate.

How this fits together

Clownfish preflight reads pull-request comments before an external pull request reaches checkout and validation. It classifies trusted automation and human objections into benign or blocking evidence for the merge gate.

flowchart LR
A[Pull request comments] --> B[Preflight classifier]
B --> C[Dependency Guard state]
B --> D[ClawSweeper review state]
B --> E[Human objections]
C --> F[Merge gate]
D --> F
E --> F
Loading

Before merge

  • Require complete transitional review evidence (P1) - A v1 marker with no state fields takes the transitional branch here. hasCurrentClawSweeperReadyReviewSignal requires only ready prose and an empty Before merge section, while the table scan only rejects present non-None Findings/Security cells. Thus a truncated producer comment that omits both fields is accepted as benign, despite this PR's full-comment fail-closed contract. Require exactly one clean result for each field and add omission fixtures.
  • Resolve security concern: Incomplete transitional state can pass preflight - A malformed producer comment can omit its Findings and Security evidence yet be accepted as a benign current-head review, weakening the intended fail-closed review gate.
  • Resolve merge risk (P1) - An incomplete transitional ClawSweeper comment can be treated as benign, allowing external-merge preflight to proceed without explicit clean Findings and Security evidence.
  • Complete next step (P2) - A focused parser and fixture repair can close the fail-open transitional-state gap without changing the intended contract.
  • Improve patch quality - Require explicit clean Findings and Security evidence for transitional v1 comments.
  • Improve patch quality - Add fixtures that omit each required evidence field and assert that preflight blocks.

Findings

  • [P1] Require complete transitional review evidence — scripts/preflight-external-pr-merge.mjs:2629
  • [high] Incomplete transitional state can pass preflight — scripts/preflight-external-pr-merge.mjs:2629
Agent review details

Security

Needs attention: The transitional parser can fail open on missing review-security evidence in a pre-merge authorization boundary.

Review metrics

Metric Value Why it matters
Change size production +190/-11, tests +354/-2 The parser contract is substantially expanded, so complete fail-closed coverage matters more than the passing happy-path fixtures.

Merge-risk options

Maintainer options:

  1. Require complete transitional evidence (recommended)
    Require one explicit clean Findings result and one explicit clean Security result for transitional v1 comments, and add omission fixtures before merge.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Require complete transitional Findings and Security evidence and add omission regression fixtures before merge.

Technical review

Best possible solution:

Require exactly one clean Findings result and one clean Security result before accepting a transitional v1 review, then add omission regressions alongside the existing malformed-state cases.

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

Yes—source-reproducible: a current-head v1 marker with no state fields, ready prose, and an empty Before merge section is accepted even if the Findings and Security evidence is omitted.

Is this the best way to solve the issue?

No. The proposed full-comment validation must require the clean Findings and Security evidence it claims to consume, rather than treating their absence as clean.

Full review comments:

  • [P1] Require complete transitional review evidence — scripts/preflight-external-pr-merge.mjs:2629
    A v1 marker with no state fields takes the transitional branch here. hasCurrentClawSweeperReadyReviewSignal requires only ready prose and an empty Before merge section, while the table scan only rejects present non-None Findings/Security cells. Thus a truncated producer comment that omits both fields is accepted as benign, despite this PR's full-comment fail-closed contract. Require exactly one clean result for each field and add omission fixtures.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 67e43efa9094.

Labels

Label changes:

  • add P1: This changes an external merge-preflight safety gate and currently permits an incomplete producer review to pass it.
  • add merge-risk: 🚨 security-boundary: The changed parser determines whether security and review evidence can be treated as benign before merge authorization.
  • add merge-risk: 🚨 automation: The defect can cause the automated preflight workflow to advance an external pull request on incomplete review state.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This MEMBER-authored internal preflight change is covered by focused fixture-based validation; no external contributor proof gate applies.

Label justifications:

  • P1: This changes an external merge-preflight safety gate and currently permits an incomplete producer review to pass it.
  • merge-risk: 🚨 security-boundary: The changed parser determines whether security and review evidence can be treated as benign before merge authorization.
  • merge-risk: 🚨 automation: The defect can cause the automated preflight workflow to advance an external pull request on incomplete review state.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This MEMBER-authored internal preflight change is covered by focused fixture-based validation; no external contributor proof gate applies.

Evidence

Security concerns:

  • [high] Incomplete transitional state can pass preflight — scripts/preflight-external-pr-merge.mjs:2629
    A malformed producer comment can omit its Findings and Security evidence yet be accepted as a benign current-head review, weakening the intended fail-closed review gate.
    Confidence: 0.97

Acceptance criteria:

  • [P1] node --test test/preflight-external-pr-merge.test.mjs.
  • [P1] npm run validate.
  • [P1] git diff --check.

What I checked:

Likely related people:

  • github-actions[bot]: The available current-main blame and feature-history record attributes the existing preflight review-parser snapshot to this automation commit; deeper individual ownership is not preserved in local history. (role: current-main parser provenance author; confidence: low; commits: e47a61ceddf9; files: scripts/preflight-external-pr-merge.mjs, test/preflight-external-pr-merge.test.mjs)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant