diff --git a/README.md b/README.md index a3429f95f..69a31ee53 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,10 @@ flowchart LR 1. The advisor creates a falsifiable assignment with the exact required research-base SHA, baseline metrics, expected mechanism, implementation scope, and stopping rules. 2. `create_assignment` creates the student branch and draft PR, embeds a typed assignment record, and applies the routing labels. 3. The assigned student receives one OpenHands conversation for that assignment revision. New PR comments and reviews are queued durably even while a turn is active, then delivered in the next bounded turn. -4. The student commits the exact implementation, launches supervised training, and records every referenced run in W&B. +4. The student commits the exact implementation, launches supervised training, + records every referenced run in W&B, and uses `post_assignment_comment` for + material progress, questions, blockers, or replies. Each typed comment wakes + the advisor without changing the PR's branch, draft state, or labels. 5. The student calls `submit_experiment_result`; the tool validates and publishes the branch before changing the PR to `status:review`. 6. The advisor compares the evidence, then uses the corresponding operation-specific tool to merge a reproducible winner, close a useful negative result, request a new revision, or send non-revision feedback. diff --git a/SPEC.md b/SPEC.md index 75c54a851..1638e4bce 100644 --- a/SPEC.md +++ b/SPEC.md @@ -296,7 +296,7 @@ the model-facing schema. It also canonicalizes every Senpai-authored comment to an `ADVISOR:` or `STUDENT:` prefix from that trusted role; models supply plain comment text and cannot impersonate the other role through a payload. -Advisor operations that act on an assignment share this object: +Assignment-scoped advisor and student operations share this object: ```json { @@ -313,6 +313,7 @@ Advisor operations that act on an assignment share this object: | `publish_advisor_branch` | advisor | `remote_branch_sha_before_push`, `local_commit_sha` | | `repair_assignment_routing` | advisor | `working_state` (`wip` or `review`) and a `blockers` list containing only `blocked`, `hold`, or `needs-rebase` | | `send_assignment_feedback` | advisor | `feedback_id`, `comment` | +| `post_assignment_comment` | student | `comment_id`, `comment` | | `request_assignment_revision` | advisor | `new_revision_id`, `required_base_sha`, `comment` | | `accept_result_on_current_base` | advisor | `expected_current_base_sha`, `reason` | | `merge_experiment` | advisor | `expected_current_base_sha`, `merge_method` | @@ -320,11 +321,18 @@ Advisor operations that act on an assignment share this object: | `respond_to_human_issue` | advisor or student | `issue_number`, `human_message_id`, `response` | | `submit_experiment_result` | student | `branch`, `remote_branch_sha_before_push`, `result` | -Student publication happens only inside `submit_experiment_result`, which -derives the PR and proposed local head from the structured result, then validates -repository, assignment, revision, student, and current remote head before it can -push. Marker comments are trusted only when authored by the authenticated token -actor. +Interim student communication happens through `post_assignment_comment`. The +runtime binds the configured student identity and validates the exact open WIP +or review assignment, revision, and PR head before posting an immutable typed comment. +Exact replay is a no-op; changed text uses a new `comment_id`. The comment does +not push, change routing, or end the assignment, and its trusted marker wakes the +advisor without replaying into the student's own feedback inbox. + +Terminal student publication happens only inside `submit_experiment_result`, +which derives the PR and proposed local head from the structured result, then +validates repository, assignment, revision, student, and current remote head +before it can push. Marker comments are trusted only when authored by the +authenticated token actor. Assignment creation checks the remote base SHA, creates an isolated empty assignment commit with `git commit-tree`, publishes with force-with-lease, diff --git a/plugins/senpai/README.md b/plugins/senpai/README.md index ad6da0c70..d26cc674b 100644 --- a/plugins/senpai/README.md +++ b/plugins/senpai/README.md @@ -21,7 +21,7 @@ the current role: `repair_assignment_routing`, `send_assignment_feedback`, `request_assignment_revision`, `accept_result_on_current_base`, `merge_experiment`, and `close_experiment`; -- students receive `submit_experiment_result`; and +- students receive `post_assignment_comment` and `submit_experiment_result`; and - both roles receive `get_prs` and `respond_to_human_issue`. Each tool has one operation-specific schema without a union wrapper and a diff --git a/senpai_agent/controller.py b/senpai_agent/controller.py index f0419f584..84569fa2d 100644 --- a/senpai_agent/controller.py +++ b/senpai_agent/controller.py @@ -53,7 +53,9 @@ from senpai_agent.workspace import StudentWorkspaceReconciler, WorkspaceDivergence -_EDGE_TRIGGERED_EVENT_KINDS = frozenset({"research_base_changed"}) +_EDGE_TRIGGERED_EVENT_KINDS = frozenset( + {"research_base_changed", "student_assignment_comment"} +) _PROMPT_TEMPLATE_VARIABLES = frozenset( { "ADVISOR_BRANCH", diff --git a/senpai_agent/github/mailbox/advisor.py b/senpai_agent/github/mailbox/advisor.py index fd1cbc603..20818aea8 100644 --- a/senpai_agent/github/mailbox/advisor.py +++ b/senpai_agent/github/mailbox/advisor.py @@ -29,7 +29,7 @@ result_matches_assignment, versioned_event, ) -from .issues import human_issue_events +from .communications import advisor_communication_events if TYPE_CHECKING: from .core import GitHubMailbox @@ -136,7 +136,7 @@ def advisor_events( ) events.extend(_research_base_events(mailbox, active_assignments)) - events.extend(human_issue_events(mailbox, issues)) + events.extend(advisor_communication_events(mailbox, active_assignments, issues)) return tuple(events) diff --git a/senpai_agent/github/mailbox/communications.py b/senpai_agent/github/mailbox/communications.py new file mode 100644 index 000000000..2b50122f3 --- /dev/null +++ b/senpai_agent/github/mailbox/communications.py @@ -0,0 +1,26 @@ +"""Advisor events derived from PR and Issue communication.""" + +from __future__ import annotations + +from collections.abc import Sequence +from typing import TYPE_CHECKING + +from senpai_agent.mailbox import ControllerEvent +from senpai_agent.models import AssignmentRecord + +from .issues import human_issue_events +from .student_comments import student_assignment_comment_events + +if TYPE_CHECKING: + from .core import GitHubMailbox + + +def advisor_communication_events( + mailbox: GitHubMailbox, + assignments: Sequence[tuple[dict[str, object], AssignmentRecord]], + issues: Sequence[dict[str, object]], +) -> list[ControllerEvent]: + return [ + *student_assignment_comment_events(mailbox, assignments), + *human_issue_events(mailbox, issues), + ] diff --git a/senpai_agent/github/mailbox/student_comments.py b/senpai_agent/github/mailbox/student_comments.py new file mode 100644 index 000000000..d2184113d --- /dev/null +++ b/senpai_agent/github/mailbox/student_comments.py @@ -0,0 +1,152 @@ +"""Trusted student assignment comments delivered to advisor controllers.""" + +from __future__ import annotations + +import sys +from collections.abc import Sequence +from typing import TYPE_CHECKING + +from senpai_agent.github.http import GitHubReadError +from senpai_agent.mailbox import ControllerEvent +from senpai_agent.models import ( + AssignmentCommentRecord, + AssignmentRecord, + authoritative_marker_line, + parse_assignment_comment_markers, + render_assignment_comment_marker, +) + +from .values import ( + FEEDBACK_EXCERPT_BYTES, + bounded_text, + github_datetime, + object_value, + payload_digest, +) + +if TYPE_CHECKING: + from .core import GitHubMailbox + + +def student_assignment_comment_events( + mailbox: GitHubMailbox, + assignments: Sequence[tuple[dict[str, object], AssignmentRecord]], +) -> list[ControllerEvent]: + """Wake the advisor for trusted typed messages from assigned students.""" + + with_comments = [ + (pull, assignment) + for pull, assignment in assignments + if pull.get("comments_url") + ] + if not with_comments: + return [] + try: + actor = mailbox._github.actor() + except (GitHubReadError, TypeError) as error: + _report_read_error(f"actor {type(error).__name__}: {error}") + return [] + + events_by_key: dict[str, ControllerEvent] = {} + for pull, assignment in with_comments: + comments_url = str(pull["comments_url"]) + number = int(pull["number"]) + try: + comments = mailbox._github.objects(f"{comments_url}?per_page=100") + except (GitHubReadError, TypeError) as error: + _report_read_error(f"pr={number} {type(error).__name__}: {error}") + continue + for item in comments: + event = _comment_event( + mailbox, + pull, + assignment, + item, + actor=actor, + ) + if event is not None: + previous = events_by_key.get(event.dedupe_key) + if previous is None or int(event.payload["github_comment_id"]) < int( + previous.payload["github_comment_id"] + ): + events_by_key[event.dedupe_key] = event + events = list(events_by_key.values()) + events.sort( + key=lambda event: ( + github_datetime(str(event.payload["created_at"])), + int(event.payload["github_comment_id"]), + ) + ) + return events + + +def _comment_event( + mailbox: GitHubMailbox, + pull: dict[str, object], + assignment: AssignmentRecord, + item: dict[str, object], + *, + actor: str, +) -> ControllerEvent | None: + try: + author = str(object_value(item["user"])["login"]) + body = str(item.get("body") or "") + records = parse_assignment_comment_markers(body) + github_comment_id = int(item["id"]) + comment_url = str(item["html_url"]) + created_at = str(item["created_at"]) + except (KeyError, TypeError, ValueError): + return None + if author.casefold() != actor.casefold() or len(records) != 1: + return None + record: AssignmentCommentRecord = records[0] + number = int(pull["number"]) + if ( + record.repo != mailbox.repo + or record.pr_number != number + or record.assignment_id != assignment.assignment_id + or record.revision_id != assignment.revision_id + or record.student != assignment.student + ): + return None + marker = render_assignment_comment_marker(record) + if authoritative_marker_line(body) != marker: + return None + message = "\n".join(body.splitlines()[1:]).strip() + if not message: + return None + payload = { + "number": number, + "pr_url": str(pull["html_url"]), + "comment_url": comment_url, + "github_comment_id": github_comment_id, + "comment_id": record.comment_id, + "assignment_id": record.assignment_id, + "revision_id": record.revision_id, + "student": record.student, + "message": bounded_text(message, limit=FEEDBACK_EXCERPT_BYTES), + "created_at": created_at, + } + semantic_payload = { + "number": number, + "assignment_id": record.assignment_id, + "revision_id": record.revision_id, + "student": record.student, + "comment_id": record.comment_id, + "message": message, + } + return ControllerEvent( + kind="student_assignment_comment", + dedupe_key=( + "student_assignment_comment:v2:" + payload_digest(semantic_payload) + ), + payload=payload, + ) + + +def _report_read_error(message: str) -> None: + print( + f"SENPAI_STUDENT_COMMENT_READ_ERROR {message}", + file=sys.stderr, + flush=True, + ) diff --git a/senpai_agent/github/tools/__init__.py b/senpai_agent/github/tools/__init__.py index 7f4c7c8d0..f3bf088e1 100644 --- a/senpai_agent/github/tools/__init__.py +++ b/senpai_agent/github/tools/__init__.py @@ -8,6 +8,7 @@ GitHubMutationObservation, MergeExperimentAction, PublishAdvisorBranchAction, + PostAssignmentCommentAction, RepairAssignmentRoutingAction, RequestAssignmentRevisionAction, RespondToHumanIssueAction, @@ -20,6 +21,7 @@ CreateAssignmentTool, MergeExperimentTool, PublishAdvisorBranchTool, + PostAssignmentCommentTool, RepairAssignmentRoutingTool, RequestAssignmentRevisionTool, RespondToHumanIssueTool, @@ -58,6 +60,8 @@ "MergeExperimentAction", "MergeExperimentTool", "PRManifestObservation", + "PostAssignmentCommentAction", + "PostAssignmentCommentTool", "PublishAdvisorBranchAction", "PublishAdvisorBranchTool", "RepairAssignmentRoutingAction", diff --git a/senpai_agent/github/tools/contracts.py b/senpai_agent/github/tools/contracts.py index dacd862fe..d045a1d29 100644 --- a/senpai_agent/github/tools/contracts.py +++ b/senpai_agent/github/tools/contracts.py @@ -127,6 +127,30 @@ class SendAssignmentFeedbackAction(Action): ) +class PostAssignmentCommentAction(Action): + """Post one interim student message to the current assignment PR.""" + + assignment: AssignmentVersion = Field( + description="Current student assignment revision and PR-head precondition.", + ) + comment_id: str = Field( + min_length=1, + max_length=256, + description=( + "Stable ID for this message. Exact replay is a no-op; a new message " + "must use a new ID." + ), + ) + comment: str = Field( + min_length=1, + max_length=50_000, + description=( + "Interim progress, question, blocker, evidence, or response to post on " + "the assigned pull request." + ), + ) + + class RequestAssignmentRevisionAction(Action): """Start a new revision of an existing assignment on an exact research base.""" diff --git a/senpai_agent/github/tools/definitions.py b/senpai_agent/github/tools/definitions.py index 54aa5acb5..5a9289424 100644 --- a/senpai_agent/github/tools/definitions.py +++ b/senpai_agent/github/tools/definitions.py @@ -24,6 +24,7 @@ GitHubMutationObservation, MergeExperimentAction, PublishAdvisorBranchAction, + PostAssignmentCommentAction, RepairAssignmentRoutingAction, RequestAssignmentRevisionAction, RespondToHumanIssueAction, @@ -32,6 +33,7 @@ ) from .runtime import ( GitHubToolRuntime, + PostAssignmentCommentExecutor, SubmitExperimentResultExecutor, tool_annotations, ) @@ -228,3 +230,21 @@ def create(cls, runtime: GitHubToolRuntime) -> Sequence[Self]: "make the PR review-ready.", SubmitExperimentResultExecutor(runtime), ) + + +class PostAssignmentCommentTool( + ToolDefinition[PostAssignmentCommentAction, GitHubMutationObservation] +): + """Post one student-authored comment to its active assignment PR.""" + + @classmethod + def create(cls, runtime: GitHubToolRuntime) -> Sequence[Self]: + return _tool( + cls, + PostAssignmentCommentAction, + "Post assignment comment", + "Post or exactly replay one meaningful interim progress update, " + "question, blocker, evidence item, or response on this student's " + "current active assignment without pushing or changing workflow state.", + PostAssignmentCommentExecutor(runtime), + ) diff --git a/senpai_agent/github/tools/runtime.py b/senpai_agent/github/tools/runtime.py index 6b5a72fde..718fedf5f 100644 --- a/senpai_agent/github/tools/runtime.py +++ b/senpai_agent/github/tools/runtime.py @@ -22,7 +22,11 @@ ) from senpai_agent.models import ExperimentResult -from .contracts import GitHubMutationObservation, SubmitExperimentResultAction +from .contracts import ( + GitHubMutationObservation, + PostAssignmentCommentAction, + SubmitExperimentResultAction, +) if TYPE_CHECKING: from openhands.sdk.conversation import LocalConversation @@ -121,6 +125,13 @@ def require_current_student(self, student: str) -> None: f"student {self.student_name!r}" ) + def current_student(self) -> str: + """Return the configured student identity for a student-owned mutation.""" + + if self.role != "student" or not self.student_name: + raise RuntimeError("student GitHub tools require a student name") + return self.student_name + def human_issue_audience(self) -> set[str]: """Return the only Issue audience labels this role may answer.""" @@ -210,6 +221,32 @@ def _submit_after_push( ) +class PostAssignmentCommentExecutor( + ToolExecutor[PostAssignmentCommentAction, GitHubMutationObservation] +): + """Post one durable interim message to the student's current assignment.""" + + def __init__(self, runtime: GitHubToolRuntime): + self.runtime = runtime + + def __call__( + self, + action: PostAssignmentCommentAction, + conversation: LocalConversation | None = None, + ) -> GitHubMutationObservation: + version = action.assignment + result = self.runtime.workflow.post_assignment_comment( + version.pr_number, + assignment_id=version.assignment_id, + revision_id=version.revision_id, + expected_head_sha=version.expected_pr_head_sha, + student=self.runtime.current_student(), + comment_id=action.comment_id, + comment=action.comment, + ) + return GitHubMutationObservation.from_result(result) + + def configured_student_names(value: Sequence[str] | str | None) -> frozenset[str]: """Normalize an explicit or environment-provided launch allowlist.""" diff --git a/senpai_agent/github/tools/toolset.py b/senpai_agent/github/tools/toolset.py index eddcda82a..6824931f5 100644 --- a/senpai_agent/github/tools/toolset.py +++ b/senpai_agent/github/tools/toolset.py @@ -17,6 +17,7 @@ CreateAssignmentTool, MergeExperimentTool, PublishAdvisorBranchTool, + PostAssignmentCommentTool, RepairAssignmentRoutingTool, RequestAssignmentRevisionTool, RespondToHumanIssueTool, @@ -101,7 +102,11 @@ def create( *RespondToHumanIssueTool.create(runtime), ) if role == "student": - return (*common, *SubmitExperimentResultTool.create(runtime)) + return ( + *common, + *PostAssignmentCommentTool.create(runtime), + *SubmitExperimentResultTool.create(runtime), + ) return ( *common, *CreateAssignmentTool.create(runtime), diff --git a/senpai_agent/github/workflow/student_comments.py b/senpai_agent/github/workflow/student_comments.py new file mode 100644 index 000000000..03bb8249b --- /dev/null +++ b/senpai_agent/github/workflow/student_comments.py @@ -0,0 +1,137 @@ +"""Student-authored progress and question comments on active assignments.""" + +from senpai_agent.github.workflow.errors import ( + ReconciliationError, + WorkflowPreconditionError, +) +from senpai_agent.github.workflow.responses import MutationResult, PullRequestSnapshot +from senpai_agent.github.workflow.text import marker_body, role_prefixed_comment +from senpai_agent.github.workflow.validation import ( + require_current_revision, + require_open, +) +from senpai_agent.models import ( + AssignmentRecord, + AssignmentCommentRecord, + render_assignment_comment_marker, +) + + +def _require_assigned_routing( + snapshot: PullRequestSnapshot, + assignment: AssignmentRecord, +) -> None: + labels = set(snapshot.labels) + student_labels = {label for label in labels if label.startswith("student:")} + if student_labels != {f"student:{assignment.student}"}: + raise WorkflowPreconditionError( + "pull request must retain exactly its assigned student label" + ) + if len(labels & {"status:wip", "status:review"}) != 1: + raise WorkflowPreconditionError( + "pull request must retain exactly one active assignment status" + ) + + +class StudentCommentMixin: + __slots__ = () + + def post_assignment_comment( + self, + number: int, + *, + assignment_id: str, + revision_id: str, + expected_head_sha: str, + student: str, + comment_id: str, + comment: str, + ) -> MutationResult: + """Post one idempotent student message to its current assignment.""" + + if self._role != "student": + raise PermissionError("post_assignment_comment requires a student workflow") + with self._assignment_lifecycle_lock: + return self._post_assignment_comment( + number, + assignment_id=assignment_id, + revision_id=revision_id, + expected_head_sha=expected_head_sha, + student=student, + comment_id=comment_id, + comment=comment, + ) + + def _post_assignment_comment( + self, + number: int, + *, + assignment_id: str, + revision_id: str, + expected_head_sha: str, + student: str, + comment_id: str, + comment: str, + ) -> MutationResult: + before, assignment = self._assigned_pull_at_head( + number, + assignment_id=assignment_id, + expected_head_sha=expected_head_sha, + ) + require_open(before) + require_current_revision(assignment, revision_id) + _require_assigned_routing(before, assignment) + if assignment.student != student: + raise PermissionError( + f"assignment student {assignment.student!r} does not match this " + f"runtime's student {student!r}" + ) + + comment_id = comment_id.strip() + content = comment.strip() + if not comment_id or not content: + raise ValueError("comment_id and comment must not be empty") + marker = render_assignment_comment_marker( + AssignmentCommentRecord( + repo=self._repo, + pr_number=number, + assignment_id=assignment.assignment_id, + revision_id=assignment.revision_id, + student=assignment.student, + comment_id=comment_id, + ) + ) + rendered = marker_body(marker, content) + existing = self._marker_comments(number, marker) + if len(existing) > 1: + raise ReconciliationError( + f"GitHub contains multiple comments for marker {marker!r}" + ) + desired_body = role_prefixed_comment(rendered, self._role) + if existing and existing[0].body != desired_body: + raise WorkflowPreconditionError( + "comment_id already identifies a different message; " + "use a new comment_id" + ) + + changed, verified = self._upsert_marker_comment( + number, + marker=marker, + body=rendered, + ) + after, current_assignment = self._assigned_pull_at_head( + number, + assignment_id=assignment_id, + expected_head_sha=expected_head_sha, + ) + require_open(after) + require_current_revision(current_assignment, revision_id) + _require_assigned_routing(after, current_assignment) + if current_assignment.student != student: + raise ReconciliationError("assignment student changed while posting comment") + return MutationResult( + changed=changed, + resource_url=verified.url, + state="assignment_comment_posted", + version=after.head_sha, + ) diff --git a/senpai_agent/github/workflow/workflow.py b/senpai_agent/github/workflow/workflow.py index 7f928e9aa..850ec9ec4 100644 --- a/senpai_agent/github/workflow/workflow.py +++ b/senpai_agent/github/workflow/workflow.py @@ -9,6 +9,7 @@ from senpai_agent.github.workflow.results import ResultMixin from senpai_agent.github.workflow.review import ReviewMixin from senpai_agent.github.workflow.revisions import RevisionMixin +from senpai_agent.github.workflow.student_comments import StudentCommentMixin class GitHubWorkflow( @@ -16,6 +17,7 @@ class GitHubWorkflow( MergeMixin, ReviewMixin, ResultMixin, + StudentCommentMixin, RevisionMixin, AssignmentMixin, LookupMixin, diff --git a/senpai_agent/models.py b/senpai_agent/models.py index f32407d88..5a72c439b 100644 --- a/senpai_agent/models.py +++ b/senpai_agent/models.py @@ -98,6 +98,18 @@ class AssignmentFeedbackRecord(Contract): feedback_id: _NonEmptyString +class AssignmentCommentRecord(Contract): + """One immutable student message on its exact active assignment.""" + + schema_version: Literal[1] = 1 + repo: _NonEmptyString + pr_number: int = Field(gt=0) + assignment_id: _NonEmptyString + revision_id: _NonEmptyString + student: _NonEmptyString + comment_id: _NonEmptyString + + class ResearchBaseAcceptanceRecord(Contract): """Durable approval of one exact result against a changed research base.""" @@ -229,6 +241,11 @@ class ResultMarkerError(ValueError): r"" ) +_ASSIGNMENT_COMMENT_PREFIX = "" +) _RESEARCH_BASE_ACCEPTANCE_PREFIX = "" +def render_assignment_comment_marker(comment: AssignmentCommentRecord) -> str: + return f"" + + +def parse_assignment_comment_markers( + body: str, +) -> tuple[AssignmentCommentRecord, ...]: + comments: list[AssignmentCommentRecord] = [] + for line_number, line in enumerate(body.splitlines(), start=1): + if not line.startswith(_ASSIGNMENT_COMMENT_PREFIX): + continue + marker = _ASSIGNMENT_COMMENT_MARKER.fullmatch(line) + if marker is None or marker.group("version") != "1": + raise ValueError( + "malformed or unsupported Senpai assignment comment marker " + f"on line {line_number}" + ) + try: + comments.append( + AssignmentCommentRecord.model_validate_json(marker.group("payload")) + ) + except (ValidationError, ValueError) as error: + raise ValueError( + f"invalid Senpai assignment comment marker on line {line_number}" + ) from error + return tuple(comments) + + def parse_assignment_feedback_markers( body: str, ) -> tuple[AssignmentFeedbackRecord, ...]: diff --git a/system_instructions/SENPAI-HARNESS.md b/system_instructions/SENPAI-HARNESS.md index 071551c96..c283d4fc0 100644 --- a/system_instructions/SENPAI-HARNESS.md +++ b/system_instructions/SENPAI-HARNESS.md @@ -84,7 +84,7 @@ only when its named tool is present in your schema: `repair_assignment_routing`, `send_assignment_feedback`, `request_assignment_revision`, `accept_result_on_current_base`, `merge_experiment`, and `close_experiment`. Students may receive - `submit_experiment_result`. Both roles may receive + `post_assignment_comment` and `submit_experiment_result`. Both roles may receive `respond_to_human_issue`. Do not reproduce these operations with `gh`, raw REST calls, or `git push`. diff --git a/system_instructions/STUDENT.md b/system_instructions/STUDENT.md index 811f0f61c..0e6af1b13 100644 --- a/system_instructions/STUDENT.md +++ b/system_instructions/STUDENT.md @@ -20,8 +20,10 @@ allowed files, metric contract, run limits, and any requested revision. - Modify only files allowed by `program.md`, the assignment, and the target contract. Ask the advisor when they conflict. - Do not mutate GitHub workflow state or push through shell commands. Use - `submit_experiment_result` so the branch lease, result identity, draft state, - and labels are verified together. + `post_assignment_comment` for a meaningful interim question, blocker, + progress update, evidence item, or reply on the assigned PR. Use + `submit_experiment_result` for the terminal result so the branch lease, + result identity, draft state, and labels are verified together. - If no assignment is present, finish. The controller owns work polling. ## Implement @@ -92,6 +94,11 @@ advisor or human feedback before spending another allocation. ## Report and submit +Post material interim updates with `post_assignment_comment`, especially when +the advisor asked for a response or the experiment is blocked or delayed. Use a +fresh `comment_id` for each new message; exact retries reuse the same ID. Do not +mirror routine internal narration—the PR should remain a concise research log. + Report: - the terminal structured Senpai result; diff --git a/tests/test_controller.py b/tests/test_controller.py index 7fcefca42..d3b7ab343 100644 --- a/tests/test_controller.py +++ b/tests/test_controller.py @@ -604,6 +604,40 @@ def poll(self): ] +def test_student_assignment_comment_does_not_repeat_on_reminder_cadence( + monkeypatch, +): + event = ControllerEvent( + kind="student_assignment_comment", + dedupe_key="student_assignment_comment:v2:message-1", + payload={"comment_id": "message-1"}, + ) + clock = [0.0] + monkeypatch.setattr(controller_module.time, "monotonic", lambda: clock[0]) + + class PersistentMailbox(Mailbox): + def poll(self): + self.calls += 1 + return (event,) + + turns = Turns() + controller_module.Controller( + role="advisor", + mailbox=PersistentMailbox([]), + turns=turns, + conversation_id=CONVERSATION_ID, + full_prompt="programme", + sleep=lambda seconds: clock.__setitem__(0, clock[0] + seconds), + poll_interval_seconds=600, + jitter_seconds=0, + event_reminder_seconds=600, + ).run(max_cycles=3) + + assert [call[2] for call in turns.calls] == [ + frozenset({event.dedupe_key}), + ] + + def test_changed_research_base_sha_wakes_immediately(): first = research_base_event("def") changed = research_base_event("fed") diff --git a/tests/test_controller_github_advisor.py b/tests/test_controller_github_advisor.py index 562f59c94..57375914f 100644 --- a/tests/test_controller_github_advisor.py +++ b/tests/test_controller_github_advisor.py @@ -3,6 +3,7 @@ from senpai_agent.github.mailbox import GitHubMailbox from senpai_agent.models import ( + AssignmentCommentRecord, AssignmentKey, AssignmentRecord, ExperimentResult, @@ -10,6 +11,7 @@ ResultStatus, experiment_result_digest, render_assignment_marker, + render_assignment_comment_marker, render_research_base_acceptance_marker, render_result_comment, ) @@ -73,6 +75,143 @@ def assignment( ) +def assignment_comment( + *, + comment_id: int = 501, + message_id: str = "paired-run-started", + student: str = "student-1", + revision_id: str = "revision-2", +): + marker = render_assignment_comment_marker( + AssignmentCommentRecord( + repo="acme/widgets", + pr_number=17, + assignment_id="assignment-17", + revision_id=revision_id, + student=student, + comment_id=message_id, + ) + ) + return { + "id": comment_id, + "body": f"{marker}\n\nSTUDENT: The paired run has started.", + "html_url": f"https://github.test/acme/widgets/pull/17#issuecomment-{comment_id}", + "created_at": "2026-08-11T07:20:00Z", + "updated_at": "2026-08-11T07:20:00Z", + "user": {"login": "senpai-bot", "type": "Bot"}, + "author_association": "MEMBER", + } + + +def test_student_assignment_comment_wakes_the_advisor_once(monkeypatch): + comments_url = "https://api.github.test/repos/acme/widgets/issues/17/comments" + assigned = pull( + labels=("research", "student:student-1", "status:wip"), + body=render_assignment_marker(assignment()), + head_sha="7" * 40, + comments_url=comments_url, + ) + advisor = mailbox(monkeypatch, [assigned], students=("student-1",)) + monkeypatch.setattr(advisor._github, "actor", lambda: "senpai-bot") + monkeypatch.setattr(advisor._github, "objects", lambda _url: [assignment_comment()]) + monkeypatch.setattr( + advisor._github, + "get", + lambda _path: {"object": {"sha": "b" * 40}}, + ) + + first = next( + event for event in advisor.poll() if event.kind == "student_assignment_comment" + ) + replay = next( + event for event in advisor.poll() if event.kind == "student_assignment_comment" + ) + assigned["head"]["sha"] = "8" * 40 + assigned["labels"] = [ + {"name": label} + for label in ("research", "student:student-1", "status:review") + ] + after_submission = next( + event for event in advisor.poll() if event.kind == "student_assignment_comment" + ) + + assert first.dedupe_key == replay.dedupe_key == after_submission.dedupe_key + assert first.payload == { + "number": 17, + "pr_url": "https://github.test/acme/widgets/pull/17", + "comment_url": "https://github.test/acme/widgets/pull/17#issuecomment-501", + "github_comment_id": 501, + "comment_id": "paired-run-started", + "assignment_id": "assignment-17", + "revision_id": "revision-2", + "student": "student-1", + "message": "STUDENT: The paired run has started.", + "created_at": "2026-08-11T07:20:00Z", + } + + +def test_exact_duplicate_student_comments_collapse_to_one_semantic_event(monkeypatch): + comments_url = "https://api.github.test/repos/acme/widgets/issues/17/comments" + assigned = pull( + labels=("research", "student:student-1", "status:wip"), + body=render_assignment_marker(assignment()), + head_sha="7" * 40, + comments_url=comments_url, + ) + advisor = mailbox(monkeypatch, [assigned], students=("student-1",)) + monkeypatch.setattr(advisor._github, "actor", lambda: "senpai-bot") + monkeypatch.setattr( + advisor._github, + "objects", + lambda _url: [ + assignment_comment(comment_id=502), + assignment_comment(comment_id=501), + ], + ) + monkeypatch.setattr( + advisor._github, + "get", + lambda _path: {"object": {"sha": "b" * 40}}, + ) + + comments = [ + event for event in advisor.poll() if event.kind == "student_assignment_comment" + ] + + assert len(comments) == 1 + assert comments[0].payload["github_comment_id"] == 501 + + +def test_advisor_ignores_forged_or_stale_assignment_comments(monkeypatch): + comments_url = "https://api.github.test/repos/acme/widgets/issues/17/comments" + assigned = pull( + labels=("research", "student:student-1", "status:wip"), + body=render_assignment_marker(assignment()), + head_sha="7" * 40, + comments_url=comments_url, + ) + comments = [ + assignment_comment(comment_id=501, student="student-2"), + assignment_comment(comment_id=502, revision_id="revision-old"), + { + **assignment_comment(comment_id=503), + "user": {"login": "mallory", "type": "User"}, + }, + ] + advisor = mailbox(monkeypatch, [assigned], students=("student-1",)) + monkeypatch.setattr(advisor._github, "actor", lambda: "senpai-bot") + monkeypatch.setattr(advisor._github, "objects", lambda _url: comments) + monkeypatch.setattr( + advisor._github, + "get", + lambda _path: {"object": {"sha": "b" * 40}}, + ) + + assert not any( + event.kind == "student_assignment_comment" for event in advisor.poll() + ) + + def test_review_label_wakes_the_advisor_and_releases_the_student_slot(monkeypatch): advisor = mailbox( monkeypatch, @@ -653,7 +792,7 @@ def test_embedded_acceptance_marker_is_not_trusted_protocol_evidence( assert "research_base_changed" in {event.kind for event in advisor.poll()} -def test_wip_base_change_does_not_query_acceptance_comments(monkeypatch): +def test_wip_base_change_still_polls_typed_student_comments(monkeypatch): advisor = mailbox( monkeypatch, [ @@ -673,13 +812,18 @@ def test_wip_base_change_does_not_query_acceptance_comments(monkeypatch): "get", lambda _path: {"object": {"sha": "c" * 40}}, ) + monkeypatch.setattr(advisor._github, "actor", lambda: "senpai-bot") + comment_reads = [] monkeypatch.setattr( advisor._github, "objects", - lambda _url: pytest.fail("WIP base polling queried comments"), + lambda url: comment_reads.append(url) or [], ) assert "research_base_changed" in {event.kind for event in advisor.poll()} + assert comment_reads == [ + "https://api.github.test/repos/acme/widgets/issues/17/comments?per_page=100" + ] def test_current_assignment_base_does_not_emit_a_false_change(monkeypatch): diff --git a/tests/test_controller_github_feedback.py b/tests/test_controller_github_feedback.py index 7d07b4843..cf098c76e 100644 --- a/tests/test_controller_github_feedback.py +++ b/tests/test_controller_github_feedback.py @@ -9,9 +9,11 @@ from senpai_agent.inbox import PersistentInbox from senpai_agent.mailbox import ControllerEvent from senpai_agent.models import ( + AssignmentCommentRecord, AssignmentFeedbackRecord, AssignmentRecord, render_assignment_feedback_marker, + render_assignment_comment_marker, render_assignment_marker, ) from senpai_agent.state import ( @@ -518,6 +520,37 @@ def test_untrusted_people_bots_and_automation_comments_are_not_feedback( assert [event.payload["feedback_id"] for event in events] == [101] +def test_student_assignment_comment_is_not_replayed_to_its_student(monkeypatch): + marker = render_assignment_comment_marker( + AssignmentCommentRecord( + repo="acme/widgets", + pr_number=17, + assignment_id="assignment-17", + revision_id="revision-2", + student="student-1", + comment_id="paired-run-started", + ) + ) + mailbox = student_mailbox( + monkeypatch, + feedback_responses( + issue_comments=[ + feedback( + 105, + f"{marker}\n\nSTUDENT: The paired run has started.", + author="morganmcg1", + association="OWNER", + user_type="User", + ) + ] + ), + ) + + assert not any( + event.kind == "student_pr_feedback" for event in mailbox.poll() + ) + + def test_typed_actor_feedback_keeps_its_marked_revision_and_hides_other_protocol( monkeypatch, ): diff --git a/tests/test_github_workflow_student_comments.py b/tests/test_github_workflow_student_comments.py new file mode 100644 index 000000000..2d3ff2cef --- /dev/null +++ b/tests/test_github_workflow_student_comments.py @@ -0,0 +1,205 @@ +import pytest + +from github_workflow_support import ( + ASSIGNMENT_ID, + HEAD_SHA, + AmbiguousMutationGitHub, + FakeGitHub, + pull_request, + workflow, +) +from senpai_agent.github.workflow import ( + PullHeadMismatchError, + StaleAssignmentRevisionError, + WorkflowPreconditionError, +) +from senpai_agent.models import ( + AssignmentCommentRecord, + render_assignment_comment_marker, +) + + +def post_comment( + client, + *, + assignment_id: str = ASSIGNMENT_ID, + revision_id: str = "revision-1", + expected_head_sha: str = HEAD_SHA, + student: str = "student-one", + comment_id: str = "compile-started", + comment: str = "The candidate compiles; paired timing is running now.", +): + return client.post_assignment_comment( + 7, + assignment_id=assignment_id, + revision_id=revision_id, + expected_head_sha=expected_head_sha, + student=student, + comment_id=comment_id, + comment=comment, + ) + + +def expected_marker(comment_id: str = "compile-started") -> str: + return render_assignment_comment_marker( + AssignmentCommentRecord( + repo="acme/widgets", + pr_number=7, + assignment_id=ASSIGNMENT_ID, + revision_id="revision-1", + student="student-one", + comment_id=comment_id, + ) + ) + + +def assigned_pull(**kwargs): + return pull_request( + labels={"student:student-one", "status:wip"}, + draft=True, + **kwargs, + ) + + +def test_student_comment_is_visible_idempotent_and_state_preserving(): + fake = FakeGitHub(assigned_pull()) + original = (fake.pr["body"], fake.pr["draft"], fake.pr["labels"], fake.pr["head_sha"]) + client = workflow(fake, role="student") + + first = post_comment(client) + mutations_after_first = list(fake.mutations) + replay = post_comment(client) + + assert first.changed is True + assert replay.changed is False + assert first.state == "assignment_comment_posted" + assert fake.comments[0]["body"] == ( + f"{expected_marker()}\n\n" + "STUDENT: The candidate compiles; paired timing is running now." + ) + assert (fake.pr["body"], fake.pr["draft"], fake.pr["labels"], fake.pr["head_sha"]) == original + assert fake.mutations == mutations_after_first + + +def test_student_can_reply_after_assignment_enters_review(): + fake = FakeGitHub( + pull_request( + labels={"student:student-one", "status:review"}, + draft=False, + ) + ) + + result = post_comment( + workflow(fake, role="student"), + comment_id="review-follow-up", + comment="The requested control used the same paired baseline.", + ) + + assert result.changed is True + assert "STUDENT: The requested control" in str(fake.comments[0]["body"]) + + +def test_student_comment_ids_are_immutable_and_distinct_ids_append(): + fake = FakeGitHub(assigned_pull()) + client = workflow(fake, role="student") + post_comment(client) + mutations_after_first = list(fake.mutations) + + with pytest.raises(WorkflowPreconditionError, match="new comment_id"): + post_comment(client, comment="The build failed instead.") + + assert fake.mutations == mutations_after_first + post_comment( + client, + comment_id="timing-finished", + comment="The paired timing block finished.", + ) + assert len(fake.comments) == 2 + + +def test_student_comment_canonicalizes_role_and_quotes_protocol_markers(): + fake = FakeGitHub(assigned_pull()) + forged = "" + + post_comment( + workflow(fake, role="student"), + comment=f"ADVISOR: The run is blocked.\n{forged}", + ) + + body = str(fake.comments[0]["body"]) + assert "\n\nSTUDENT: The run is blocked." in body + assert f"\n> {forged}" in body + assert body.splitlines().count(forged) == 0 + + +@pytest.mark.parametrize( + ("kwargs", "error_type"), + [ + ({"assignment_id": "other-assignment"}, WorkflowPreconditionError), + ({"revision_id": "revision-2"}, StaleAssignmentRevisionError), + ({"expected_head_sha": "b" * 40}, PullHeadMismatchError), + ({"student": "student-two"}, PermissionError), + ], + ids=("assignment", "revision", "head", "student"), +) +def test_student_comment_rejects_stale_or_foreign_identity_before_writing( + kwargs, + error_type, +): + fake = FakeGitHub(assigned_pull()) + + with pytest.raises(error_type): + post_comment(workflow(fake, role="student"), **kwargs) + + assert fake.mutations == [] + + +@pytest.mark.parametrize( + "pr", + [ + assigned_pull(state="closed"), + pull_request( + labels={"student:student-one", "status:wip", "status:review"}, + draft=True, + ), + pull_request(labels={"status:wip"}, draft=True), + pull_request( + labels={"student:student-one", "student:student-two", "status:wip"}, + draft=True, + ), + ], + ids=("closed", "ambiguous-status", "missing-student-label", "multiple-student-labels"), +) +def test_student_comment_requires_one_open_current_assignment(pr): + fake = FakeGitHub(pr) + + with pytest.raises(WorkflowPreconditionError): + post_comment(workflow(fake, role="student")) + + assert fake.mutations == [] + + +def test_student_comment_rejects_advisor_workflow_before_writing(): + fake = FakeGitHub(assigned_pull()) + + with pytest.raises(PermissionError, match="student workflow"): + post_comment(workflow(fake, role="advisor")) + + assert fake.mutations == [] + + +def test_student_comment_retry_recovers_a_lost_post_response_without_duplication(): + path = "/repos/acme/widgets/issues/7/comments" + fake = AmbiguousMutationGitHub( + assigned_pull(), + fail_method="POST", + fail_path=path, + ) + client = workflow(fake, role="student") + + recovered = post_comment(client) + replay = post_comment(client) + + assert recovered.changed is True + assert replay.changed is False + assert len(fake.comments) == 1 diff --git a/tests/test_model_markers.py b/tests/test_model_markers.py index fc3a18b2b..966076b19 100644 --- a/tests/test_model_markers.py +++ b/tests/test_model_markers.py @@ -8,16 +8,19 @@ from model_test_support import HEAD_SHA, assignment, result from senpai_agent.models import ( + AssignmentCommentRecord, ResearchBaseAcceptanceRecord, ResultMarkerError, ResultStatus, WandbRunRef, authoritative_marker_line, experiment_result_digest, + parse_assignment_comment_markers, parse_assignment_markers, parse_research_base_acceptance_markers, parse_result_markers, render_assignment_marker, + render_assignment_comment_marker, render_research_base_acceptance_marker, render_result_comment, render_result_marker, @@ -42,6 +45,37 @@ def test_assignment_marker_round_trips_as_one_line(): assert parse_assignment_markers(marker) == (original,) +def test_assignment_comment_marker_round_trips_as_one_line(): + original = AssignmentCommentRecord( + repo="acme/widgets", + pr_number=17, + assignment_id="assignment-17", + revision_id="revision-2", + student="student-1", + comment_id="paired-run-started", + ) + + marker = render_assignment_comment_marker(original) + + assert "\n" not in marker + assert marker.startswith("", + "", + " trailing", + "", + ], +) +def test_assignment_comment_parser_rejects_invalid_markers(line: str): + with pytest.raises(ValueError, match="assignment comment marker"): + parse_assignment_comment_markers(line) + + @pytest.mark.parametrize( "line", [ diff --git a/tests/test_openhands_conversation.py b/tests/test_openhands_conversation.py index 9e805dfe1..fe2093c5f 100644 --- a/tests/test_openhands_conversation.py +++ b/tests/test_openhands_conversation.py @@ -1103,6 +1103,7 @@ def close(self): assert captured["resolved"] == { "senpai_github": { "get_prs", + "post_assignment_comment", "respond_to_human_issue", "submit_experiment_result", }, diff --git a/tests/test_openhands_tools_and_agents.py b/tests/test_openhands_tools_and_agents.py index 0eac3f1df..a282d15b9 100644 --- a/tests/test_openhands_tools_and_agents.py +++ b/tests/test_openhands_tools_and_agents.py @@ -435,6 +435,7 @@ def test_file_agent_definitions_keep_bounded_tools_and_no_github_mutations( "get_prs", "create_assignment", "publish_advisor_branch", + "post_assignment_comment", "repair_assignment_routing", "send_assignment_feedback", "request_assignment_revision", diff --git a/tests/test_tools_github_access.py b/tests/test_tools_github_access.py index 37b9bdb6e..9676a0e73 100644 --- a/tests/test_tools_github_access.py +++ b/tests/test_tools_github_access.py @@ -37,6 +37,7 @@ } STUDENT_GITHUB_TOOLS = { "get_prs", + "post_assignment_comment", "respond_to_human_issue", "submit_experiment_result", } diff --git a/tests/test_tools_github_schema.py b/tests/test_tools_github_schema.py index 09fa0b7c5..3588ddf81 100644 --- a/tests/test_tools_github_schema.py +++ b/tests/test_tools_github_schema.py @@ -11,6 +11,7 @@ GitHubToolRuntime, MergeExperimentTool, PublishAdvisorBranchTool, + PostAssignmentCommentTool, RepairAssignmentRoutingTool, RequestAssignmentRevisionTool, RespondToHumanIssueTool, @@ -35,6 +36,7 @@ "remote_branch_sha_before_push", "local_commit_sha", }, + "post_assignment_comment": {"assignment", "comment_id", "comment"}, "repair_assignment_routing": {"assignment", "working_state", "blockers"}, "send_assignment_feedback": {"assignment", "feedback_id", "comment"}, "request_assignment_revision": { @@ -81,6 +83,7 @@ def github_tools(tmp_path: Path): tool_types = ( CreateAssignmentTool, PublishAdvisorBranchTool, + PostAssignmentCommentTool, RepairAssignmentRoutingTool, SendAssignmentFeedbackTool, RequestAssignmentRevisionTool, diff --git a/tests/test_tools_github_transitions.py b/tests/test_tools_github_transitions.py index 677007c93..709253445 100644 --- a/tests/test_tools_github_transitions.py +++ b/tests/test_tools_github_transitions.py @@ -15,6 +15,8 @@ GitHubToolRuntime, MergeExperimentAction, MergeExperimentTool, + PostAssignmentCommentAction, + PostAssignmentCommentTool, PublishAdvisorBranchAction, PublishAdvisorBranchTool, RepairAssignmentRoutingAction, @@ -67,6 +69,23 @@ def runtime(workflow: RecordingWorkflow, workspace: Path) -> GitHubToolRuntime: ) +def student_runtime( + workflow: RecordingWorkflow, + workspace: Path, + *, + student_name: str | None = "student-one", +) -> GitHubToolRuntime: + return GitHubToolRuntime( + workflow=workflow, + workspace=workspace, + git_token=None, + role="student", + advisor_branch=None, + student_names=frozenset(), + student_name=student_name, + ) + + def assignment() -> AssignmentVersion: return AssignmentVersion( pr_number=17, @@ -76,6 +95,59 @@ def assignment() -> AssignmentVersion: ) +def test_student_comment_binds_the_runtime_student_and_exact_assignment( + tmp_path: Path, +): + workflow = RecordingWorkflow() + tool = PostAssignmentCommentTool.create( + student_runtime(workflow, tmp_path) + )[0] + + observation = tool( + PostAssignmentCommentAction( + assignment=assignment(), + comment_id="paired-run-started", + comment="The paired run has started.", + ) + ) + + assert observation.state == "post_assignment_comment" + assert workflow.calls == [ + ( + "post_assignment_comment", + 17, + { + "assignment_id": "assignment-17", + "revision_id": "revision-1", + "expected_head_sha": "a" * 40, + "student": "student-one", + "comment_id": "paired-run-started", + "comment": "The paired run has started.", + }, + ) + ] + + +def test_student_comment_requires_a_configured_student_before_mutation( + tmp_path: Path, +): + workflow = RecordingWorkflow() + tool = PostAssignmentCommentTool.create( + student_runtime(workflow, tmp_path, student_name=None) + )[0] + + with pytest.raises(RuntimeError, match="student name"): + tool( + PostAssignmentCommentAction( + assignment=assignment(), + comment_id="blocked", + comment="The experiment is blocked.", + ) + ) + + assert workflow.calls == [] + + def test_create_assignment_uses_the_created_branch_head_for_the_pr( monkeypatch: pytest.MonkeyPatch, tmp_path: Path,