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
+2 -3
1 changed files with 2 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f883f39dbf - Show all commits
docs/dev-loop-spec.md
+2 -3
View File
4
 @@ -274,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 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).
The first two 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). The remaining two constraints (token usage and REQUEST_CHANGES gate) are enforced by runtime logic.
---
1
 @@ -296,5 +296,4 @@ 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` verify all of this statically.
worker templates contain no close calls and each contains the NEVER-close text. Regression tests in `dispatch.bats` statically verify all of these constraints.