Add PR comment intake and the review loop (B2) — step 6b #17

Merged

dev-agent merged 5 commits from review-loop into main 2026-09-07 22:55:12 +00:00

Conversation 2 Commits 5 Files Changed 13

+1036 -9

dev-agent commented 2026-09-07 22:00:35 +00:00

Collaborator

Copy link

Copy source

Continues step 6. (This began stacked on #16; now that #16 is merged it applies cleanly to main as a single commit.)

• poller.py — polls PR comments instead of taking webhooks, so the VM stays outbound-only per B3. On an outage it backs off exponentially, caps at five minutes, and retries indefinitely while logging: polling is pure intake, so an outage delays a reaction but must never fail a task or force a transition.
• prime() marks the existing thread as seen without delivering it. Without that, starting a watch replays the whole history and the agent re-addresses feedback it already handled — including its own earlier replies.
• The review loop: a comment arriving in PR_OPEN enters the canonical transcript before the executor runs, so feedback reaches it as conversation rather than as an out-of-band instruction. The fix is pushed to the same branch and a reply posted on the PR.
• No intent is recorded for a follow-up push, deliberately — the PR already exists, so a crash mid-push costs a repeated push of the same commit, which is harmless. That is the distinction from publish(), where the side effect is a whole new PR.
• Comment authors are checked against a separate Gitea operator allowlist. A PR comment is an instruction channel into an agent that writes code, so it is operator-scoped for the same reason C4 scopes tickets — and a Gitea login is not a Matrix MXID, which is why A4 specifies per-channel allowlists rather than one list.

One thing this fixes that predates it: the orchestrator had no lock. Until now only the transport thread delivered messages; the poller makes that two threads mutating state-machine position concurrently. Orchestrator now holds an RLock across message handling.

I also made the backoff timings injectable — the outage path was otherwise untestable without a 15-second wait, and a test that slow tends to get deleted rather than kept.

16 new unit tests. 173 unit + 10 integration green locally.

Still to come for step 6: Gitea as an approval source (approve rev-N comments) and plan externalisation to issue comments. Both are cleaner once the sandbox repo is owned by dongar with the agent as a write collaborator, since that is what makes "the agent cannot approve or merge its own work" testable rather than merely true by omission.

🤖 Generated with Claude Code

Continues step 6. (This began stacked on #16; now that #16 is merged it applies cleanly to `main` as a single commit.) - **`poller.py`** — polls PR comments instead of taking webhooks, so the VM stays outbound-only per B3. On an outage it backs off exponentially, caps at five minutes, and retries indefinitely while logging: polling is pure intake, so an outage delays a reaction but must never fail a task or force a transition. - **`prime()`** marks the existing thread as seen without delivering it. Without that, starting a watch replays the whole history and the agent re-addresses feedback it already handled — including its own earlier replies. - **The review loop**: a comment arriving in `PR_OPEN` enters the canonical transcript *before* the executor runs, so feedback reaches it as conversation rather than as an out-of-band instruction. The fix is pushed to the same branch and a reply posted on the PR. - **No intent is recorded for a follow-up push**, deliberately — the PR already exists, so a crash mid-push costs a repeated push of the same commit, which is harmless. That is the distinction from `publish()`, where the side effect is a whole new PR. - **Comment authors are checked against a separate Gitea operator allowlist.** A PR comment is an instruction channel into an agent that writes code, so it is operator-scoped for the same reason C4 scopes tickets — and a Gitea login is not a Matrix MXID, which is why A4 specifies per-channel allowlists rather than one list. **One thing this fixes that predates it:** the orchestrator had no lock. Until now only the transport thread delivered messages; the poller makes that two threads mutating state-machine position concurrently. `Orchestrator` now holds an `RLock` across message handling. I also made the backoff timings injectable — the outage path was otherwise untestable without a 15-second wait, and a test that slow tends to get deleted rather than kept. 16 new unit tests. 173 unit + 10 integration green locally. Still to come for step 6: Gitea as an approval source (`approve rev-N` comments) and plan externalisation to issue comments. Both are cleaner once the sandbox repo is owned by `dongar` with the agent as a write collaborator, since that is what makes "the agent cannot approve or merge its own work" testable rather than merely true by omission. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dev-agent added 1 commit 2026-09-07 22:00:35 +00:00

Add PR comment intake and the review loop (B2) — step 6b ... affdb9e8aa

- poller.py: CommentPoller polls PR comments rather than taking
  webhooks, keeping the VM outbound-only (B3). On an outage it backs
  off exponentially, caps, and retries indefinitely while logging:
  polling is pure intake, so an outage delays a reaction but never
  fails a task or forces a transition
- prime() marks the existing thread as seen without delivering it, so
  starting a watch does not re-address feedback already handled
- Review loop: a comment in PR_OPEN enters the canonical transcript,
  the executor responds, and the fix is pushed to the same branch with
  a reply on the PR. No intent is recorded for a follow-up push — the
  PR already exists, so a repeated push is harmless, unlike opening a
  second PR
- Comment authors are checked against a separate Gitea operator
  allowlist: a PR comment is an instruction channel into an agent that
  writes code, so it is operator-scoped, and a Gitea login is not a
  Matrix MXID (A4 per-channel allowlists)
- Orchestrator now holds a lock: transport and the poller deliver on
  separate threads, and state transitions must not interleave
- Backoff timings are injectable, so the outage path is tested in
  milliseconds instead of being untestable

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

dev-agent referenced this pull request 2026-09-07 22:15:08 +00:00

Externalise plans to Gitea and accept approvals there (A4, Part H) #18

dongar added 3 commits 2026-09-07 22:47:42 +00:00

Externalise plans to Gitea and accept approvals there (A4, Part H) ... d508e9a9b7

- approvals.py: PlanBoard opens one issue per project and posts each
  plan revision as a comment carrying its rev tag. The state store
  keeps only a pointer, so gate state can be re-derived from Gitea
  after a state-volume loss — external truth wins
- Gitea comments are a second approval source: revision binding, the
  typed word approve, and first-approval-wins work exactly as over
  transport, with a per-channel allowlist because a Gitea login is
  not a Matrix MXID
- Outcomes are echoed to the other channel, so the two records cannot
  silently disagree about what was approved
- A Gitea outage degrades durability of the record, not the ability
  to approve: planning proceeds and Matrix remains a complete channel
- Live test now asserts the server refuses the agent's own merge and
  its push to the protected branch. That property was previously
  true only because our code did not attempt it; branch protection
  makes it structural, and this test fails if the protection is ever
  relaxed
- Fixed a race in the process-group test: killing a group is
  asynchronous and the grandchild is an orphan, so its exit is not
  synchronised with our return; the test now waits for death

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Do not believe a completion claim before any work was done ... acee8b96f0

CI caught the executor recording an empty milestone as a success: the
model answered with prose on its first turn, and the loop reads "no
tool call" as "the plan is done". It had therefore finished a
milestone having touched nothing, and the milestone would have gone
on to publish with no changes.

A completion claim is now challenged once when no tool has been
called yet — the same shape as the planner's nudge — and a model that
persists escalates instead of reporting success. After the first tool
call the claim is taken at face value, as before.

The live milestone test failed roughly one run in five on this; four
consecutive runs pass now.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Merge pull request 'Externalise plans to Gitea and accept approvals there (A4, Part H)' (#18) from gitea-approvals into review-loop ... 8f365263c4

Reviewed-on: dongar/nisse#18
Reviewed-by: Don Garrett <dgarrett@acm.org>

dongar approved these changes 2026-09-07 22:48:42 +00:00

Dismissed

dev-agent added 1 commit 2026-09-07 22:53:22 +00:00

Count only mutating tools as evidence of work ... 886cb80583

The previous fix challenged a completion claim made before any tool
call, but treated every tool as work — so a model that ran one
list_dir and then declared the milestone done was believed, and CI
caught the same empty milestone again.

Reading a file or listing a directory is the model orienting itself.
Only write_file and run_command now count, so a completion claim must
be backed by something that changed or verified the tree.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

dev-agent dismissed dongar's review 2026-09-07 22:53:22 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dongar approved these changes 2026-09-07 22:55:08 +00:00

dev-agent merged commit f03c201729 into main 2026-09-07 22:55:12 +00:00

dev-agent deleted branch review-loop 2026-09-07 22:55:12 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

dongar

Labels

Clear labels

bug

Reproduce first; the fix goes ahead after a grace period unless the operator replies

design

Agree the design (ARCHITECTURE.md + build plan PR) before any code

discuss

Talk it through with the operator before restating

e2e:full

CI runs all 48 property walks and the whole end-to-end suite on this PR

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

dev-agent dongar (Don Garrett) nisse-review

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dev/nisse#17