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
+5 -3
1 changed files with 5 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit fb7d8d5e3b - Show all commits
docs/dev-loop-spec.md
+5 -3
View File
@@ -232,6 +232,7 @@ These are statically checked by `~/.openclaw/workspace/scripts/test/check-invari
| 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` |
---
4
 @@ -273,7 +274,7 @@ Every worker template begins with an `⛔ ABSOLUTE CONSTRAINTS` section containi
- **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.
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).
These constraints are statically enforced by `check-invariants.sh`: S1 and S9 cover the dispatch script (no merge, no close); S8 and S10 cover worker templates (no merge calls, no close calls, and NEVER-close text present in each).
---
1
 @@ -294,5 +295,6 @@ Worker templates were missing an explicit constraint against closing PRs. The di
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. The regression test in `dispatch.bats`
verifies the same statically.
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` verify all of this statically.