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

fix(#157): add never-close constraint to spec, S9 invariant, and regression test #158

Merged
aweiker merged 4 commits from issue-157 into main 2026-05-15 22:56:45 +00:00
Conversation 23 Commits 4 Files Changed 1
+24 -1
1 changed files with 24 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/dev-loop-spec.md
+24 -1
View File
@@ -231,6 +231,8 @@ These are statically checked by `~/.openclaw/workspace/scripts/test/check-invari
| S6 | Active WIP does not cause early exit (only sets ACTIVE_WIP flag) |
| S7 | SPAWN:impl guarded by `ACTIVE_WIP == 0` check |
| S8 | No merge calls in any worker template |
| S9 | Zero close-PR API calls in dispatch script (`state=closed` does not appear) |
| S10 | No close-PR API calls in any worker template; every worker template contains `NEVER close a PR` |
---
1
 @@ -263,9 +265,20 @@ Each worker receives a precise task description with substituted values:
security-review-bot commented 2026-05-15 22:42:17 +00:00
Review
Copy Link
Copy Source

[MINOR] The S9/S10 close-PR prohibition and referenced static checks risk missing alternative close vectors (e.g., closing via the issues API endpoint or JSON payloads using "state":"closed"). Consider updating the spec (and checks) to explicitly cover PATCH/PUT to both pulls and issues endpoints and detect JSON forms like "state":"closed" in addition to form-encoded 'state=closed'.

**[MINOR]** The S9/S10 close-PR prohibition and referenced static checks risk missing alternative close vectors (e.g., closing via the issues API endpoint or JSON payloads using "state":"closed"). Consider updating the spec (and checks) to explicitly cover PATCH/PUT to both pulls and issues endpoints and detect JSON forms like "state":"closed" in addition to form-encoded 'state=closed'.
Workers **always** remove the WIP label on completion and reply `NO_REPLY`.
### Worker Absolute Constraints
Every worker template begins with an `⛔ ABSOLUTE CONSTRAINTS` section containing these rules:
- **NEVER close a PR.** Never call `PATCH /pulls/{id}` with `state=closed`. Closing a PR requires human action. "Duplicate", "superseded", or "already done" are never a worker's call.
sonnet-review-bot commented 2026-05-15 14:49:42 +00:00
Review
Copy Link
Copy Source

[MINOR] S9 is documented as checking 'the dispatch script', but no equivalent S10 (or extension of S8) exists for worker templates. S8 covers 'no merge calls in any worker template', but there is no corresponding invariant for 'no close calls in any worker template'. The new 'Worker Absolute Constraints' section explains the constraint is enforced 'by the template text itself (for workers)', meaning it relies on prose rather than a static check. This is a weaker guarantee than an invariant — a future template edit could silently drop the constraint. Consider whether S8's scope should be expanded, or a new S10 added, to statically verify worker templates contain no close-PR calls.

**[MINOR]** S9 is documented as checking 'the dispatch script', but no equivalent S10 (or extension of S8) exists for worker templates. S8 covers 'no merge calls in any worker template', but there is no corresponding invariant for 'no close calls in any worker template'. The new 'Worker Absolute Constraints' section explains the constraint is enforced 'by the template text itself (for workers)', meaning it relies on prose rather than a static check. This is a weaker guarantee than an invariant — a future template edit could silently drop the constraint. Consider whether S8's scope should be expanded, or a new S10 added, to statically verify worker templates contain no close-PR calls.
- **NEVER merge a PR.** Never call the merge API. Merging requires human approval.
gpt-review-bot commented 2026-05-15 14:50:15 +00:00
Review
Copy Link
Copy Source

[NIT] The sentence "These constraints are enforced by S1, S8, and S9 in check-invariants.sh (for the dispatch script) and by the template text itself (for workers)" could be clarified: S1 and S9 apply to the dispatch script, while S8 applies to worker templates. Explicitly stating this mapping would avoid ambiguity.

**[NIT]** The sentence "These constraints are enforced by S1, S8, and S9 in check-invariants.sh (for the dispatch script) and by the template text itself (for workers)" could be clarified: S1 and S9 apply to the dispatch script, while S8 applies to worker templates. Explicitly stating this mapping would avoid ambiguity.
- **NEVER use the gitea-aweiker token.** All API calls use the gitea-rodin token only.
- **NEVER act on a PR with active REQUEST_CHANGES.** Fix the findings first.
gpt-review-bot commented 2026-05-15 17:27:26 +00:00
Review
Copy Link
Copy Source

[NIT] The paragraph stating 'These constraints are statically enforced by check-invariants.sh' lists enforcement for merge/close invariants (S1, S9, S8, S10) but not for 'NEVER use the gitea-aweiker token' or 'NEVER act on a PR with active REQUEST_CHANGES'. Consider clarifying that only a subset is statically enforced, or add explicit invariants for these constraints to avoid ambiguity.

**[NIT]** The paragraph stating 'These constraints are statically enforced by check-invariants.sh' lists enforcement for merge/close invariants (S1, S9, S8, S10) but not for 'NEVER use the gitea-aweiker token' or 'NEVER act on a PR with active REQUEST_CHANGES'. Consider clarifying that only a subset is statically enforced, or add explicit invariants for these constraints to avoid ambiguity.
sonnet-review-bot commented 2026-05-15 14:49:42 +00:00
Review
Copy Link
Copy Source

[NIT] The claim 'All 7 worker templates already contain NEVER close a PR in their ABSOLUTE CONSTRAINTS section from a prior session' is in the PR description but not verifiable from the diff alone. The spec text says 'Every worker template begins with an ⛔ ABSOLUTE CONSTRAINTS section containing these rules' — if this is an invariant worth enforcing, the check-invariants.sh script could grep all worker templates to confirm this, similar to how S8 checks worker templates for merge calls.

**[NIT]** The claim 'All 7 worker templates already contain `NEVER close a PR` in their ABSOLUTE CONSTRAINTS section from a prior session' is in the PR description but not verifiable from the diff alone. The spec text says 'Every worker template begins with an ⛔ ABSOLUTE CONSTRAINTS section containing these rules' — if this is an invariant worth enforcing, the check-invariants.sh script could grep all worker templates to confirm this, similar to how S8 checks worker templates for merge calls.
gpt-review-bot commented 2026-05-15 18:08:20 +00:00
Review
Copy Link
Copy Source

[MINOR] In the 'Worker Absolute Constraints' section, the text states the first two constraints are statically enforced. S10 explicitly requires both 'no close calls' and the presence of 'NEVER close a PR' text, but S8 enforces only the absence of merge calls without checking for the presence of 'NEVER merge a PR' text. Consider either adding a check for the merge constraint text or clarifying the wording for consistency.

**[MINOR]** In the 'Worker Absolute Constraints' section, the text states the first two constraints are statically enforced. S10 explicitly requires both 'no close calls' and the presence of 'NEVER close a PR' text, but S8 enforces only the absence of merge calls without checking for the presence of 'NEVER merge a PR' text. Consider either adding a check for the merge constraint text or clarifying the wording for consistency.
sonnet-review-bot commented 2026-05-15 22:41:40 +00:00
Review
Copy Link
Copy Source

[NIT] The new §8 Worker Absolute Constraints paragraph is very dense. Consider splitting the final explanatory sentence (starting 'The first two constraints are statically enforced…') into a small sub-list or bullet points to match the visual style of the constraints list above it — this makes it easier to scan which invariant covers which rule.

**[NIT]** The new §8 Worker Absolute Constraints paragraph is very dense. Consider splitting the final explanatory sentence (starting 'The first two constraints are statically enforced…') into a small sub-list or bullet points to match the visual style of the constraints list above it — this makes it easier to scan which invariant covers which rule.
The first two constraints are statically enforced by `check-invariants.sh`: S1 and S9 cover the dispatch script (no merge, no close); S8 covers worker templates (no merge calls); S10 covers worker templates (no close calls, with NEVER-close text verified present in each). The remaining two constraints (token usage and REQUEST_CHANGES gate) are enforced by runtime logic.
---
## 9. Fixes for Issues #144 and #145
## 9. Fixes for Issues #144, #145, and #157
**Issue #144** (autonomous merge):
The dispatch script contains no merge API calls anywhere. The `~/.openclaw/workspace/scripts/test/check-invariants.sh`
@@ -276,3 +289,13 @@ Rule 2 is the **first** rule evaluated per PR. It cannot be skipped, reasoned pa
or bypassed. It is checked before CI, before self-review, before handoff. The check
sonnet-review-bot commented 2026-05-15 17:27:02 +00:00
Review
Copy Link
Copy Source

[NIT] The sentence 'Regression tests in dispatch.bats verify all of this statically.' is a sentence fragment run-on following the previous sentence without a period before it. Minor prose clarity issue: 'Invariant S10 verifies worker templates contain no close calls and each contains the NEVER-close text. Regression tests...' — this reads fine but the final sentence could be joined or clarified as 'Regression tests in dispatch.bats statically verify all of these constraints.'

**[NIT]** The sentence 'Regression tests in `dispatch.bats` verify all of this statically.' is a sentence fragment run-on following the previous sentence without a period before it. Minor prose clarity issue: 'Invariant S10 verifies worker templates contain no close calls and each contains the NEVER-close text. Regression tests...' — this reads fine but the final sentence could be joined or clarified as 'Regression tests in `dispatch.bats` statically verify all of these constraints.'
uses latest-per-reviewer state, so a reviewer who re-approved after REQUEST_CHANGES
is correctly handled.
**Issue #157** (autonomous PR close):
Worker templates were missing an explicit constraint against closing PRs. The dispatch
script never had a close call, but workers could reason their way into calling
`PATCH /pulls/{id}` with `state=closed`. All worker templates now include
`NEVER close a PR` in their ABSOLUTE CONSTRAINTS section. Invariant S9 verifies
sonnet-review-bot commented 2026-05-15 18:07:34 +00:00
Review
Copy Link
Copy Source

[NIT] The last sentence of the Issue #157 section ('Regression tests in dispatch.bats statically verify all of these constraints.') runs on from the previous sentence without a line break or paragraph separation, making it slightly harder to read. Minor formatting only.

**[NIT]** The last sentence of the Issue #157 section ('Regression tests in `dispatch.bats` statically verify all of these constraints.') runs on from the previous sentence without a line break or paragraph separation, making it slightly harder to read. Minor formatting only.
the dispatch script contains no close calls. Invariant S10 verifies
worker templates contain no close calls and each contains the NEVER-close text.
Regression tests in `dispatch.bats` statically verify all of these constraints.