rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

bug: dev-loop merged PR #138 despite active REQUEST_CHANGES from security-review-bot #145

New Issue
Closed
opened 2026-05-15 05:03:36 +00:00 by rodin · 2 comments
rodin commented 2026-05-15 05:03:36 +00:00
Owner
Copy Link
Copy Source

Summary

The dev-loop merged PR #138 into main despite the security-review-bot having submitted a REQUEST_CHANGES review containing a MAJOR security finding. This violates the explicit rule: never merge with active REQUEST_CHANGES.

What happened

  • PR #138 had a REQUEST_CHANGES review from security-review-bot with a MAJOR finding (doc-map loaded from untrusted PR branch)
  • The dev-loop handoff check should have blocked on this
  • PR was merged anyway at 2026-05-15T03:39:37Z
  • The security bug (see issue #143) is now in main

Expected behavior

The dev-loop skill (step 1.3.6) requires ALL of:

  • Zero unresolved findings from ANY review
  • No REQUEST_CHANGES state from any reviewer

If either condition fails, the PR must NOT be handed off or merged.

Root cause (suspected)

The handoff check in the dev-loop skill may not be correctly parsing REQUEST_CHANGES state from review responses, or the check was skipped/bypassed. The Haiku model running the dispatcher may have reasoned past the check rather than executing it as a shell command.

Fix needed

  1. Audit the dev-loop handoff check — verify it actually queries review states via API and hard-fails on REQUEST_CHANGES
  2. The check must be a shell command (not reasoned about) per the skill’s own rules
  3. Consider adding explicit state != APPROVED detection separate from findings parsing
## Summary The dev-loop merged PR #138 into `main` despite the security-review-bot having submitted a REQUEST_CHANGES review containing a MAJOR security finding. This violates the explicit rule: **never merge with active REQUEST_CHANGES**. ## What happened - PR #138 had a REQUEST_CHANGES review from `security-review-bot` with a MAJOR finding (doc-map loaded from untrusted PR branch) - The dev-loop handoff check should have blocked on this - PR was merged anyway at 2026-05-15T03:39:37Z - The security bug (see issue #143) is now in `main` ## Expected behavior The dev-loop skill (step 1.3.6) requires ALL of: - [ ] Zero unresolved findings from ANY review - [ ] No REQUEST_CHANGES state from any reviewer If either condition fails, the PR must NOT be handed off or merged. ## Root cause (suspected) The handoff check in the dev-loop skill may not be correctly parsing REQUEST_CHANGES state from review responses, or the check was skipped/bypassed. The Haiku model running the dispatcher may have reasoned past the check rather than executing it as a shell command. ## Fix needed 1. Audit the dev-loop handoff check — verify it actually queries review states via API and hard-fails on REQUEST_CHANGES 2. The check must be a shell command (not reasoned about) per the skill’s own rules 3. Consider adding explicit `state != APPROVED` detection separate from findings parsing
rodin commented 2026-05-15 08:16:32 +00:00
Author
Owner
Copy Link
Copy Source

🔍 Triage note: The fix for this issue lives in the dev-loop skill (an OpenClaw workspace skill), not in review-bot Go code. Should this issue remain here as a tracking issue, or be moved/closed in favor of a fix tracked elsewhere?

The domain docs for review-bot describe the review posting/editing strategy but not how consumers (like dev-loop) should interpret review states. The dev-loop skill owns that logic.

**🔍 Triage note:** The fix for this issue lives in the dev-loop skill (an OpenClaw workspace skill), not in review-bot Go code. Should this issue remain here as a tracking issue, or be moved/closed in favor of a fix tracked elsewhere? The domain docs for review-bot describe the review posting/editing strategy but not how consumers (like dev-loop) should interpret review states. The dev-loop skill owns that logic.
rodin commented 2026-05-15 08:35:23 +00:00
Author
Owner
Copy Link
Copy Source

Addressed by issue #148 / PR #149 (pure shell dispatch redesign), merged to main.

The REQUEST_CHANGES check is now a pure shell command:

curl ... /reviews | jq -e 'any(.[]; .state == "REQUEST_CHANGES")'

If that jq command returns true, the script exits immediately — no model reasoning can override it. The shell cannot rationalize around a hard exit.

Addressed by issue #148 / PR #149 (pure shell dispatch redesign), merged to main. The REQUEST_CHANGES check is now a pure shell command: ```bash curl ... /reviews | jq -e 'any(.[]; .state == "REQUEST_CHANGES")' ``` If that jq command returns true, the script exits immediately — no model reasoning can override it. The shell cannot rationalize around a hard exit.
rodin closed this issue 2026-05-15 08:35:23 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#145
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?