smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(robomp): defer needs-info cleanup

Browse Source
This commit is contained in:
oldschoola
2026-06-15 20:27:16 -07:00
parent abd5e71d06
commit f2015a0a21
4 changed files with 103 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
python/robomp/src/host_tools.py
+39 -4
View File
@@ -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(
python/robomp/src/tasks.py
-8
View File
@@ -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,
python/robomp/tests/test_host_tools.py
+60 -1
View File
@@ -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:
python/robomp/tests/test_server.py
+4 -4
View File
@@ -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()