diff --git a/python/robomp/src/host_tools.py b/python/robomp/src/host_tools.py index f72d2ea71..a2239afac 100644 --- a/python/robomp/src/host_tools.py +++ b/python/robomp/src/host_tools.py @@ -23,7 +23,7 @@ from omp_rpc import HostTool, HostToolContext, RpcCommandError, host_tool from robomp import persona from robomp.config import Settings -from robomp.db import Database, issue_key +from robomp.db import Database, IssueState, issue_key from robomp.git_ops import GitCommandError, HeadDriftError from robomp.github_backend import GitHubBackend from robomp.github_client import GitHubError, IssueInfo, PullRequestFileInfo, RepoInfo @@ -49,6 +49,7 @@ _REPO_COMMAND_SCRUBBED_ENV_KEYS: tuple[str, ...] = ( "ROBOMP_REPLAY_TOKEN", "ROBOMP_GH_PROXY_HMAC_KEY", ) +_NEEDS_INFO_LABEL = "needs-info" _AGENT_HOME = Path("/srv/agent-home") _PRE_PR_FIX_TIMEOUT_SECONDS = 600.0 _PRE_PR_CHECK_TIMEOUT_SECONDS = 600.0 @@ -137,6 +138,33 @@ def _run_coro(loop: asyncio.AbstractEventLoop, coro: Any) -> Any: return future.result() +def _issue_needs_info(bindings: ToolBindings) -> bool: + row = bindings.db.get_issue(bindings.issue_key) + return row is not None and row.state == "needs_info" + + +def _remove_needs_info_label(bindings: ToolBindings) -> bool: + try: + _run_coro( + bindings.loop, + bindings.github.remove_issue_label(bindings.repo.full_name, bindings.issue.number, _NEEDS_INFO_LABEL), + ) + except GitHubError as exc: + if exc.status == 404: + return True + log.warning("needs-info label cleanup failed", extra={"issue": bindings.issue_key, "err": str(exc)}) + return False + return True + + +def _advance_needs_info(bindings: ToolBindings, state: IssueState) -> bool: + if not _issue_needs_info(bindings): + return False + label_cleared = _remove_needs_info_label(bindings) + bindings.db.set_issue_state(bindings.issue_key, state) + return label_cleared + + def _audit( bindings: ToolBindings, name: str, args: Mapping[str, Any], result: Any | None = None, error: str | None = None ) -> None: @@ -406,7 +434,6 @@ def _run_pre_publish_bun_check( _AUTOCLOSE_INELIGIBLE_STATES: frozenset[str] = frozenset({"closed", "merged", "needs_info", "abandoned"}) -_NEEDS_INFO_LABEL = "needs-info" def _should_schedule_autoclose(bindings: ToolBindings, target_number: int) -> float | None: @@ -695,6 +722,7 @@ def _build_open_pr(bindings: ToolBindings) -> HostTool[Any, Any]: # Make sure the branch is pushed (idempotent) using the same preflight as gh_push_branch. _guarded_push_branch(bindings, args, "gh_open_pr", bindings.workspace.branch) base = args.get("base") or bindings.repo.default_branch + was_needs_info = _issue_needs_info(bindings) try: pr = _run_coro( bindings.loop, @@ -712,6 +740,7 @@ def _build_open_pr(bindings: ToolBindings) -> HostTool[Any, Any]: _raise_command(f"GitHub rejected PR: {exc.status} {exc.message}") bindings.db.set_issue_pr(bindings.issue_key, pr.number) bindings.db.set_issue_state(bindings.issue_key, "opened") + needs_info_label_cleared = _remove_needs_info_label(bindings) if was_needs_info else False artifact = bindings.workspace.artifacts_dir / "pr.json" artifact.write_text( json.dumps( @@ -726,7 +755,10 @@ def _build_open_pr(bindings: ToolBindings) -> HostTool[Any, Any]: ), encoding="utf-8", ) - _audit(bindings, "gh_open_pr", args, result={"pr_number": pr.number, "url": pr.html_url}) + result: dict[str, Any] = {"pr_number": pr.number, "url": pr.html_url} + if needs_info_label_cleared: + result["cleared_needs_info"] = True + _audit(bindings, "gh_open_pr", args, result=result) return f"opened #{pr.number}: {pr.html_url}" return host_tool( @@ -840,7 +872,10 @@ def _build_repro_record(bindings: ToolBindings) -> HostTool[Any, Any]: if _slot_permissions_active(bindings.slot_uid): assert bindings.slot_uid is not None os.chown(target, bindings.slot_uid, bindings.slot_uid) - _audit(bindings, "repro_record", args, result={"path": str(target.relative_to(bindings.workspace.root))}) + result: dict[str, Any] = {"path": str(target.relative_to(bindings.workspace.root))} + if _advance_needs_info(bindings, "reproducing"): + result["cleared_needs_info"] = True + _audit(bindings, "repro_record", args, result=result) return "recorded" return host_tool( diff --git a/python/robomp/src/tasks.py b/python/robomp/src/tasks.py index 41c1c1b62..d78ff4978 100644 --- a/python/robomp/src/tasks.py +++ b/python/robomp/src/tasks.py @@ -22,7 +22,6 @@ from robomp.sandbox import GitTransport, SandboxManager from robomp.worker import DirectiveInfo, TaskInputs, ThreadMessage, run_task log = logging.getLogger(__name__) -_NEEDS_INFO_LABEL = "needs-info" def _comment_from_payload(payload: Mapping[str, Any]) -> CommentInfo: @@ -499,13 +498,6 @@ async def handle_comment( author_email=settings.git_author_email, slot_uid=slot_uid, ) - if existing.state == "needs_info": - try: - await github.remove_issue_label(repo.full_name, issue.number, _NEEDS_INFO_LABEL) - except GitHubError as exc: - if exc.status != 404: - log.warning("needs-info label cleanup failed", extra={"key": key, "err": str(exc)}) - db.set_issue_state(key, "reproducing") inputs = TaskInputs( settings=settings, db=db, diff --git a/python/robomp/tests/test_host_tools.py b/python/robomp/tests/test_host_tools.py index 5f5c1060e..708796d49 100644 --- a/python/robomp/tests/test_host_tools.py +++ b/python/robomp/tests/test_host_tools.py @@ -382,10 +382,69 @@ def test_repro_record_writes_transcript(db: Database, tmp_path: Path) -> None: _stop_loop(loop, t) +def test_repro_record_clears_needs_info_after_actionable_reply(db: Database, tmp_path: Path) -> None: + removed: list[tuple[str, str]] = [] + + def handler(request: httpx.Request) -> httpx.Response: + removed.append((request.method, request.url.path)) + return httpx.Response(204) + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(handler)) + db.set_issue_state(bindings.issue_key, "needs_info") + try: + tool = next(x for x in build(bindings) if x.name == "repro_record") + result = tool.execute( + { + "title": "panic on empty input", + "command": "bun test foo.test.ts", + "output": "Error: boom", + "exit_code": 1, + }, + _ctx(), + ) + finally: + _stop_loop(loop, t) + + assert result == "recorded" + assert removed == [("DELETE", "/repos/octo/widget/issues/42/labels/needs-info")] + issue = db.get_issue(bindings.issue_key) + assert issue and issue.state == "reproducing" + + +def test_repro_record_advances_needs_info_when_label_is_missing(db: Database, tmp_path: Path) -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "DELETE" + assert request.url.path == "/repos/octo/widget/issues/42/labels/needs-info" + return httpx.Response(404, json={"message": "Label does not exist"}) + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(handler)) + db.set_issue_state(bindings.issue_key, "needs_info") + try: + tool = next(x for x in build(bindings) if x.name == "repro_record") + tool.execute( + { + "title": "panic on empty input", + "command": "bun test foo.test.ts", + "output": "Error: boom", + "exit_code": 1, + }, + _ctx(), + ) + finally: + _stop_loop(loop, t) + + issue = db.get_issue(bindings.issue_key) + assert issue and issue.state == "reproducing" + + def test_repro_record_chowns_to_slot_when_root(db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: chowns: list[tuple[Path, int, int]] = [] monkeypatch.setattr(host_tools, "_slot_permissions_active", lambda slot_uid: slot_uid is not None) - monkeypatch.setattr("robomp.host_tools.os.chown", lambda path, uid, gid: chowns.append((Path(path), uid, gid))) + monkeypatch.setattr( + "robomp.host_tools.os.chown", + lambda path, uid, gid: chowns.append((Path(path), uid, gid)), + raising=False, + ) bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda r: httpx.Response(500)), slot_uid=2001) try: diff --git a/python/robomp/tests/test_server.py b/python/robomp/tests/test_server.py index a6af6f02a..399d7a377 100644 --- a/python/robomp/tests/test_server.py +++ b/python/robomp/tests/test_server.py @@ -2211,10 +2211,10 @@ async def test_handle_comment_finalized_without_directive_still_replies( close_database() -async def test_handle_comment_resumes_needs_info_reply( +async def test_handle_comment_resumes_needs_info_without_preemptive_cleanup( settings: Settings, tmp_path: Path, stub_run_task, monkeypatch ) -> None: - """Reporter details after a needs-info request resume the existing session.""" + """A needs-info reply resumes first; host tools clear state only after actionable work.""" from robomp import tasks from robomp.github_client import GitHubClient, IssueInfo, RepoInfo @@ -2285,10 +2285,10 @@ async def test_handle_comment_resumes_needs_info_reply( assert call["task_kind"] == "handle_comment" assert call["comment"].body == "I am on Bun 1.3.14 and here is the trace" assert sandbox.ensure_calls[0]["existing_branch"] == "farm/old/branch" - assert removed_labels == [("octo/widget", 88, "needs-info")] + assert removed_labels == [] assert post_comment_calls == [], "needs-info replies must not get the finalized-issue notice" row = db.get_issue("octo/widget#88") - assert row is not None and row.state == "reproducing" + assert row is not None and row.state == "needs_info" close_database()