Add PR comment intake and the review loop (B2) — step 6b
- 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>
This commit is contained in:
1 parent
3117565928
commit
affdb9e8aa
6 files changed
+540
-1
No files matched your search
@@ -8,6 +8,7 @@ arrives over transport. Every position change is checkpointed through the state
|
||||
store before the loop continues, so a restart lands mid-gate, not mid-air.
|
||||
"""
|
||||
|
||||
import threading
|
||||
import uuid
|
||||
from collections.abc import Iterable, Mapping
|
||||
from dataclasses import dataclass
|
||||
@@ -86,6 +87,12 @@ class MilestonePublisher(Protocol):
|
||||
self, *, task_id: str, milestone: int, project: str, plan: str
|
||||
) -> Any: ...
|
||||
|
||||
def push_followup(
|
||||
self, *, task_id: str, milestone: int, message: str
|
||||
) -> str | None: ...
|
||||
|
||||
def comment(self, index: int, body: str) -> None: ...
|
||||
|
||||
|
||||
class PlanningAgent(Protocol):
|
||||
def plan(
|
||||
@@ -139,6 +146,7 @@ class Orchestrator:
|
||||
roles: RoleMap | None = None,
|
||||
executor: "MilestoneExecutor | None" = None,
|
||||
publisher: "MilestonePublisher | None" = None,
|
||||
gitea_operators: Iterable[str] = (),
|
||||
) -> None:
|
||||
self._store = store
|
||||
self._transport = transport
|
||||
@@ -147,6 +155,13 @@ class Orchestrator:
|
||||
self._executor = executor
|
||||
self._publisher = publisher
|
||||
self._operators = set(operators)
|
||||
# Per-channel allowlists (A4): a Matrix MXID and a Gitea login
|
||||
# are different names for the same person, and neither implies
|
||||
# the other.
|
||||
self._gitea_operators = set(gitea_operators)
|
||||
# Transport and the comment poller deliver on separate threads;
|
||||
# state transitions must not interleave.
|
||||
self._lock = threading.RLock()
|
||||
self._data = _fresh_data()
|
||||
self._project: str | None = None
|
||||
transport.on_message(self._handle)
|
||||
@@ -197,6 +212,10 @@ class Orchestrator:
|
||||
)
|
||||
|
||||
def _handle(self, msg: InboundMessage) -> None:
|
||||
with self._lock:
|
||||
self._handle_locked(msg)
|
||||
|
||||
def _handle_locked(self, msg: InboundMessage) -> None:
|
||||
if msg.sender not in self._operators:
|
||||
return
|
||||
# Every operator message enters the canonical transcript, so planning
|
||||
@@ -376,6 +395,72 @@ class Orchestrator:
|
||||
pending={"kind": "milestone_stopped", "milestone": milestone},
|
||||
)
|
||||
|
||||
def handle_review_comment(self, comment: Any) -> None:
|
||||
"""React to a PR review comment (B2). Called from the poller thread.
|
||||
|
||||
Comment authors are checked against the Gitea operator allowlist:
|
||||
a PR comment is an instruction channel into an agent that writes
|
||||
code, so it is operator-scoped like any other (A4, and the same
|
||||
reasoning C4 applies to tickets).
|
||||
"""
|
||||
with self._lock:
|
||||
self._handle_review_comment_locked(comment)
|
||||
|
||||
def _handle_review_comment_locked(self, comment: Any) -> None:
|
||||
if comment.author not in self._gitea_operators:
|
||||
self._store.oplog.emit(
|
||||
"gitea.comment_ignored",
|
||||
cause=f"author {comment.author!r} not an operator",
|
||||
comment_id=comment.id,
|
||||
)
|
||||
return
|
||||
state = self._store.get_state()
|
||||
if state is None or state.state != PR_OPEN:
|
||||
self._store.oplog.emit(
|
||||
"gitea.comment_ignored",
|
||||
cause=f"not in PR_OPEN (state={state.state if state else None})",
|
||||
comment_id=comment.id,
|
||||
)
|
||||
return
|
||||
if self._executor is None or self._publisher is None:
|
||||
return
|
||||
|
||||
pr = self._data.get("pr") or {}
|
||||
milestone = int(pr.get("milestone", self._data.get("milestone", 1)))
|
||||
# The comment enters the transcript, so the executor sees review
|
||||
# feedback as conversation rather than as an out-of-band instruction.
|
||||
self._store.append_event(
|
||||
UserMessage(text=comment.body, source=f"gitea:comment:{comment.id}")
|
||||
)
|
||||
outcome = self._executor.run(milestone)
|
||||
if outcome.status != "completed":
|
||||
self._await_input(
|
||||
cause=f"review response {outcome.status}",
|
||||
prompt=(
|
||||
f"could not address the review comment ({outcome.status}): "
|
||||
f"{outcome.note}"
|
||||
),
|
||||
pending={"kind": "milestone_stopped", "milestone": milestone},
|
||||
)
|
||||
return
|
||||
try:
|
||||
sha = self._publisher.push_followup(
|
||||
task_id=str(self._data.get("task_id")),
|
||||
milestone=milestone,
|
||||
message=f"Address review comment {comment.id}",
|
||||
)
|
||||
except (GiteaError, GitError) as exc:
|
||||
self._transport.send_message(f"could not push the fix: {exc}")
|
||||
return
|
||||
note = (
|
||||
f"pushed {sha[:8]} addressing the review comment"
|
||||
if sha
|
||||
else "no code change was needed for that comment"
|
||||
)
|
||||
self._transport.send_message(note)
|
||||
if pr.get("number"):
|
||||
self._publisher.comment(int(pr["number"]), note)
|
||||
|
||||
def _publish_milestone(self, milestone: int) -> None:
|
||||
if self._publisher is None:
|
||||
return
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
"""Gitea comment intake (spec B2).
|
||||
|
||||
Polling, not webhooks: the agent VM stays outbound-only (B3), so nothing
|
||||
needs to reach in. Intake is structured JSON straight from the API and never
|
||||
routed through a chat bridge.
|
||||
|
||||
Outage posture, per B2: back off exponentially, cap at a few minutes, retry
|
||||
indefinitely, and log. Polling is pure intake — an outage delays a reaction
|
||||
but must never fail a task or force a state transition.
|
||||
"""
|
||||
|
||||
import threading
|
||||
from collections.abc import Callable, Iterable
|
||||
|
||||
from nisse_dev.gitea import Comment, GiteaClient, GiteaError
|
||||
from nisse_dev.oplog import OpLog
|
||||
|
||||
DEFAULT_INTERVAL_S = 20.0
|
||||
INITIAL_BACKOFF_S = 5.0
|
||||
MAX_BACKOFF_S = 300.0
|
||||
|
||||
|
||||
class CommentPoller:
|
||||
def __init__(
|
||||
self,
|
||||
client: GiteaClient,
|
||||
repo: str,
|
||||
on_comment: Callable[[Comment], None],
|
||||
*,
|
||||
oplog: OpLog | None = None,
|
||||
ignore_authors: Iterable[str] = (),
|
||||
interval_s: float = DEFAULT_INTERVAL_S,
|
||||
initial_backoff_s: float = INITIAL_BACKOFF_S,
|
||||
max_backoff_s: float = MAX_BACKOFF_S,
|
||||
) -> None:
|
||||
self._client = client
|
||||
self._repo = repo
|
||||
self._on_comment = on_comment
|
||||
self._oplog = oplog
|
||||
self._ignore = set(ignore_authors)
|
||||
self._interval_s = interval_s
|
||||
self._initial_backoff_s = initial_backoff_s
|
||||
self._max_backoff_s = max_backoff_s
|
||||
self._seen: set[int] = set()
|
||||
self._stop = threading.Event()
|
||||
self._thread: threading.Thread | None = None
|
||||
|
||||
def prime(self, index: int) -> None:
|
||||
"""Mark existing comments as seen without delivering them.
|
||||
|
||||
A poller that starts by replaying the whole thread would re-act to
|
||||
feedback already addressed — including its own earlier replies.
|
||||
"""
|
||||
try:
|
||||
for comment in self._client.list_comments(self._repo, index):
|
||||
self._seen.add(comment.id)
|
||||
except GiteaError:
|
||||
# Nothing seen means the first successful poll delivers the
|
||||
# backlog once. Better than refusing to start.
|
||||
pass
|
||||
|
||||
def poll_once(self, index: int) -> list[Comment]:
|
||||
"""One pass; returns what was newly delivered. Separated from the
|
||||
loop so behaviour is testable without threads or sleeps."""
|
||||
delivered: list[Comment] = []
|
||||
for comment in self._client.list_comments(self._repo, index):
|
||||
if comment.id in self._seen:
|
||||
continue
|
||||
self._seen.add(comment.id)
|
||||
if comment.author in self._ignore:
|
||||
continue # the agent's own replies are not new input
|
||||
delivered.append(comment)
|
||||
self._on_comment(comment)
|
||||
return delivered
|
||||
|
||||
def start(self, index: int) -> None:
|
||||
self._stop.clear()
|
||||
self._thread = threading.Thread(target=self._loop, args=(index,), daemon=True)
|
||||
self._thread.start()
|
||||
|
||||
def stop(self) -> None:
|
||||
self._stop.set()
|
||||
if self._thread is not None:
|
||||
self._thread.join(timeout=self._interval_s + 5)
|
||||
self._thread = None
|
||||
|
||||
def _loop(self, index: int) -> None:
|
||||
backoff = self._initial_backoff_s
|
||||
while not self._stop.is_set():
|
||||
try:
|
||||
self.poll_once(index)
|
||||
except GiteaError as exc:
|
||||
if self._oplog is not None:
|
||||
self._oplog.emit(
|
||||
"gitea.poll_failed",
|
||||
cause="gitea unreachable",
|
||||
error=str(exc)[:300],
|
||||
retry_in_s=backoff,
|
||||
)
|
||||
self._stop.wait(backoff)
|
||||
backoff = min(backoff * 2, self._max_backoff_s)
|
||||
continue
|
||||
backoff = self._initial_backoff_s
|
||||
self._stop.wait(self._interval_s)
|
||||
@@ -10,7 +10,12 @@ an idempotent transition reconciled against external truth.
|
||||
from dataclasses import dataclass
|
||||
|
||||
from nisse_dev.git import GitRepo, authenticated_url
|
||||
from nisse_dev.gitea import GiteaClient, PullRequest, pull_request_reconciler
|
||||
from nisse_dev.gitea import (
|
||||
GiteaClient,
|
||||
GiteaError,
|
||||
PullRequest,
|
||||
pull_request_reconciler,
|
||||
)
|
||||
from nisse_dev.reconcile import Reconciler
|
||||
from nisse_dev.state import StateStore
|
||||
|
||||
@@ -86,6 +91,32 @@ class MilestonePublisher:
|
||||
self._store.commit_intent(intent_id, cause=f"opened PR #{pr.number}")
|
||||
return PublishResult(pr, f"opened PR #{pr.number}: {pr.url}")
|
||||
|
||||
def comment(self, index: int, body: str) -> None:
|
||||
"""Reply on the PR. Courtesy only — the pushed commit is the real
|
||||
answer — so a failure here must not derail the review loop."""
|
||||
try:
|
||||
self._client.post_comment(self._repo, index, body)
|
||||
except GiteaError:
|
||||
pass
|
||||
|
||||
def push_followup(
|
||||
self, *, task_id: str, milestone: int, message: str
|
||||
) -> str | None:
|
||||
"""Push a fix commit onto an open milestone's branch.
|
||||
|
||||
No intent is recorded: the PR already exists, so a crash mid-push
|
||||
costs a repeated push of the same commit, which is harmless. That is
|
||||
the difference from publish(), where the side effect is a new PR.
|
||||
"""
|
||||
branch = self.branch_for(task_id, milestone)
|
||||
if self._git.current_branch() != branch:
|
||||
self._git.checkout_new_branch(branch)
|
||||
sha = self._git.commit_all(message)
|
||||
if sha is None:
|
||||
return None
|
||||
self._git.push(self._remote, branch, token=self._token)
|
||||
return sha
|
||||
|
||||
|
||||
def _pr_body(milestone: int, plan: str, service_url: str | None) -> str:
|
||||
parts = [
|
||||
|
||||
Reference in new issue
Block a user