Skip to content

Fix PR status routing for security owner review#264

Open
brokemac79 wants to merge 1 commit into
openclaw:mainfrom
brokemac79:fix-issue-263-security-review-status
Open

Fix PR status routing for security owner review#264
brokemac79 wants to merge 1 commit into
openclaw:mainfrom
brokemac79:fix-issue-263-security-review-status

Conversation

@brokemac79

Copy link
Copy Markdown
Contributor

Summary

Fixes #263.

This changes PR status label routing so securityReview.status: needs_attention is not always treated as contributor-facing work.

When the durable review has:

  • sufficient/override/not-applicable proof,
  • a correct patch,
  • no P0-P2 review findings, and
  • a recommended merge-risk option of accept_risk,

ClawSweeper now routes the PR to the existing status: 👀 ready for maintainer look label instead of status: ⏳ waiting on author.

Security reviews that recommend fix_before_merge, or security reviews with no structured recommended accept_risk option, still route as unresolved contributor work. That preserves the safer default for real author-remediable security defects.

Validation

  • pnpm run build:all
  • node --test --test-name-pattern "ClawSweeper PR status" test/clawsweeper.test.ts
  • pnpm exec oxfmt --check src/clawsweeper.ts test/clawsweeper.test.ts
  • pnpm run check:active-surface
  • pnpm run check:limits
  • pnpm run lint:src
  • pnpm run lint:scripts

I also attempted the full node --test test/clawsweeper.test.ts locally, but this Windows desktop environment hits unrelated existing subprocess assumptions around Codex proof checks and /usr/bin/git. The focused status-label tests pass after build:all.

@clawsweeper

clawsweeper Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 13, 2026, 4:39 PM ET / 20:39 UTC.

Summary
The PR updates ClawSweeper PR status routing so securityReview.status: needs_attention with a recommended accept_risk merge-risk option can route to maintainer look, and adds focused status-label tests.

Reproducibility: yes. Source inspection on current main shows every security needs_attention review is treated as unresolved contributor work and routed to waiting-on-author before maintainer readiness is considered.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 2 files changed, 63 additions, 2 deletions. The diff is small, but it touches live PR status-label automation.
  • Routing cases added: 3 cases covered. The new test exercises accept_risk, fix_before_merge, and ambiguous security-review behavior before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted live output, terminal output, or logs showing a real or dry-run ClawSweeper status-label sync for the security-owner acceptance case.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists checks and focused tests, but no after-fix real or dry-run status-label sync output; add redacted proof and update the PR body to trigger review, or ask for @clawsweeper re-review.

Risk before merge

  • [P1] This PR changes status-label automation; without real or dry-run label-sync proof, the report parsing plus apply-side label-sync path has not been demonstrated after the fix.

Maintainer options:

  1. Prove the label-sync path (recommended)
    Before merge, add redacted terminal output, copied live output, or logs from a real or dry-run ClawSweeper status-label sync covering the security-owner acceptance case.
  2. Accept focused test-only evidence
    A maintainer can intentionally waive the external real-behavior proof gate based on the small diff and focused tests, but that owns the automation proof gap.

Next step before merge

  • [P1] Automerge is opted in, but the remaining blocker is contributor-provided proof or an explicit maintainer proof waiver, not a branch repair ClawSweeper can make.

Security
Cleared: The diff changes internal status-label routing and focused tests only; no new secret handling, dependency, workflow permission, or supply-chain concern was found.

Review details

Best possible solution:

Land the focused routing fix after redacted live or dry-run output shows a durable review with needs_attention plus recommended accept_risk receives maintainer-look status while repairable or ambiguous security findings still wait on the author.

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

Yes. Source inspection on current main shows every security needs_attention review is treated as unresolved contributor work and routed to waiting-on-author before maintainer readiness is considered.

Is this the best way to solve the issue?

Yes, likely. Reusing the existing recommended merge-risk option category is a narrow fix that preserves waiting-on-author for ambiguous or author-remediable security findings.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7d76b7996ffb.

Label changes

Label changes:

  • add status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Needs real behavior proof before merge: The PR body lists checks and focused tests, but no after-fix real or dry-run status-label sync output; add redacted proof and update the PR body to trigger review, or ask for @clawsweeper re-review.
  • remove status: 📣 needs proof: Current PR status label is status: 🚀 automerge armed.

Label justifications:

  • P2: This is a normal-priority automation routing bug with limited blast radius to ClawSweeper PR status labels.
  • merge-risk: 🚨 automation: Merging the PR changes how durable review reports are converted into PR workflow status labels.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Needs real behavior proof before merge: The PR body lists checks and focused tests, but no after-fix real or dry-run status-label sync output; add redacted proof and update the PR body to trigger review, or ask for @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully and applied; it requires conservative, evidence-backed, automation-safe review and a read-only checkout for this task. (AGENTS.md:1, 7d76b7996ffb)
  • Current main still has the routing bug: Current main treats every securityReview.status === "needs_attention" as unresolved contributor work, then maps unresolved work to waiting_on_author before maintainer readiness can apply. (src/clawsweeper.ts:9022, 7d76b7996ffb)
  • PR implementation is narrow: PR head 7256492 adds recommendedMergeRiskOptionCategory, securityReviewNeedsContributorWork, and passes parsed merge-risk options into PR status routing rather than weakening all security-review blockers. (src/clawsweeper.ts:8893, 7256492ad862)
  • Focused regression coverage: The added test covers the intended split: recommended accept_risk reaches ready-for-maintainer-look, while recommended fix_before_merge and ambiguous security reviews still wait on the author. (test/clawsweeper.test.ts:15623, 7256492ad862)
  • Real behavior proof is still absent: The PR body lists build, lint, focused tests, and the comments show automerge was requested, but there is no terminal output, log, screenshot, or artifact from a real or dry-run status-label sync after the fix. (7256492ad862)
  • No safer canonical replacement found: Search found only this open PR and its linked open issue for the central security-owner status-routing problem, so this PR remains the active implementation candidate.

Likely related people:

  • steipete: Current blame for the status-routing helper block points at eb2f304, and recent GitHub commit history shows repeated maintenance of src/clawsweeper.ts and test/clawsweeper.test.ts. (role: recent area contributor; confidence: high; commits: eb2f30434661, 214bb5474d07, 4018a41e0fcc; files: src/clawsweeper.ts, test/clawsweeper.test.ts)
  • momothemage: Recent merged work stabilized workflow status publishing in the same ClawSweeper automation surface touched by this PR. (role: adjacent status automation contributor; confidence: medium; commits: 429a73be116e; files: src/clawsweeper.ts, test/clawsweeper.test.ts)
  • brokemac79: Beyond opening this PR, prior merged commits from this contributor touched proof nudges, related review context, and advisory label sync in src/clawsweeper.ts. (role: prior adjacent review automation contributor; confidence: medium; commits: 9b08258b9c64, 68065f816acf, 5400cbd5e004; files: src/clawsweeper.ts, test/clawsweeper.test.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.

@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 Jun 6, 2026
@Takhoffman

Copy link
Copy Markdown
Contributor

@clawsweeper automerge

@clawsweeper

clawsweeper Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper automerge is enabled.

  • Head: 7256492ad862
  • Label: clawsweeper:automerge
  • Action: exact-head review queued (workflow sweep.yml, event repository_dispatch).
  • Flow: review this head, repair/rebase only if needed, then re-review the exact repaired head before merge.

Draft PRs stay fix-only until GitHub marks them ready for review. Pause with /clawsweeper stop.

Automerge progress:

  • 2026-06-13 20:33:44 UTC review queued 7256492ad862 (queued)

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. clawsweeper:human-review ClawSweeper automerge is paused for maintainer review and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 13, 2026
@clawsweeper

clawsweeper Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: - [P1] Automerge is opted in, but the remaining blocker is contributor-provided proof or an explicit maintainer proof waiver, not a branch repair ClawSweeper can make.; Cleared: The diff changes internal status-label routing and focused tests only; no new secret handling, dependency, workflow permission, or supply-chain concern was found. (sha=7256492ad8625aa3edaa156fe5aba02d9a5d3639)

Why human review is needed:
This item has security-sensitive risk. ClawSweeper is pausing instead of making an autonomous change that could affect trust, credentials, permissions, or exposure.

What the maintainer can do as a next step:
If the maintainer accepts the current risk and wants ClawSweeper to continue merge gates, comment @clawsweeper approve. If the security-sensitive detail still needs changes, describe the safe path or push the fix, then comment @clawsweeper automerge. If the risk should not be automated, keep the PR paused for manual review or comment @clawsweeper stop.

I added clawsweeper:human-review and left the final call with a maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge clawsweeper:human-review ClawSweeper automerge is paused for maintainer review 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR status label misroutes security-owner review as waiting on author

2 participants