From d81fdf8a42b3d913bd6f141b266248dfd421dc67 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 15 May 2026 01:53:50 +0200 Subject: [PATCH] test(tests): added tests for proxy-mode, transport and shutdown behavior - Test. - Test. --- tests/conftest.py | 30 ++- tests/test_config.py | 46 ++++- tests/test_db.py | 13 ++ tests/test_host_tools.py | 385 ++++++++++++++++++++++++++++++++--- tests/test_persona.py | 15 ++ tests/test_queue_cancel.py | 35 ++++ tests/test_queue_shutdown.py | 141 +++++++++++++ tests/test_sandbox.py | 8 + tests/test_server.py | 5 + tests/test_worker.py | 227 +++++++++++++++++++++ tests/test_worker_smoke.py | 7 +- 11 files changed, 876 insertions(+), 36 deletions(-) create mode 100644 tests/test_queue_shutdown.py create mode 100644 tests/test_worker.py diff --git a/tests/conftest.py b/tests/conftest.py index d782d6893..e1d437baf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,7 +12,9 @@ from robomp.db import Database, close_database def _baseline_env(tmp_path: Path) -> dict[str, str]: return { - "GITHUB_TOKEN": "ghp_test_token_value_xxxxxxxxxxxxxxxx", + # Orchestrator-mode: no PAT in this container; talk to gh-proxy instead. + "ROBOMP_GH_PROXY_URL": "http://gh-proxy.invalid:8081", + "ROBOMP_GH_PROXY_HMAC_KEY": "test-hmac-key-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "GITHUB_WEBHOOK_SECRET": "test-webhook-secret", "ROBOMP_BOT_LOGIN": "robomp-bot", "ROBOMP_GIT_AUTHOR_NAME": "robomp-bot", @@ -31,6 +33,11 @@ def env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> dict[str, str]: env = _baseline_env(tmp_path) for key, value in env.items(): monkeypatch.setenv(key, value) + # Defensive: a stray `.env` or shell export must not flip us into PAT mode. + # `monkeypatch.delenv` would let pydantic_settings fall back to the .env + # file; setenv("") is what actually shadows the file value, and the + # `_blank_token_disables` validator treats empty strings as unset. + monkeypatch.setenv("GITHUB_TOKEN", "") monkeypatch.delenv("ROBOMP_PROVIDER", raising=False) monkeypatch.setenv("ROBOMP_REPLAY_TOKEN", "") reset_settings_cache() @@ -39,6 +46,27 @@ def env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> dict[str, str]: close_database() +@pytest.fixture +def proxy_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> dict[str, str]: + """Baseline env for the gh-proxy container: holds the PAT, no proxy vars.""" + baseline = _baseline_env(tmp_path) + baseline.pop("ROBOMP_GH_PROXY_URL", None) + baseline.pop("ROBOMP_GH_PROXY_HMAC_KEY", None) + baseline["GITHUB_TOKEN"] = "ghp_test_token_value_xxxxxxxxxxxxxxxx" + for key, value in baseline.items(): + monkeypatch.setenv(key, value) + # Same defense-in-depth as `env`: setenv("") rather than delenv so + # pydantic_settings doesn't fall back to the on-disk `.env` file. + monkeypatch.setenv("ROBOMP_GH_PROXY_URL", "") + monkeypatch.setenv("ROBOMP_GH_PROXY_HMAC_KEY", "") + monkeypatch.delenv("ROBOMP_PROVIDER", raising=False) + monkeypatch.setenv("ROBOMP_REPLAY_TOKEN", "") + reset_settings_cache() + yield baseline + reset_settings_cache() + close_database() + + @pytest.fixture def settings(env: dict[str, str]) -> Settings: cfg = Settings() # type: ignore[call-arg] diff --git a/tests/test_config.py b/tests/test_config.py index 1b0fce058..49528e26e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -15,20 +15,48 @@ def test_settings_load_from_env(env: dict[str, str]) -> None: assert not cfg.allows("other/widget") -def test_settings_missing_required(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: # type: ignore[no-untyped-def] - monkeypatch.chdir(str(tmp_path)) - for key in ( - "GITHUB_TOKEN", - "GITHUB_WEBHOOK_SECRET", - "ROBOMP_BOT_LOGIN", - "ROBOMP_REPO_ALLOWLIST", - ): - monkeypatch.delenv(key, raising=False) +def test_settings_missing_required(monkeypatch: pytest.MonkeyPatch, env: dict[str, str]) -> None: + """Empty out every credential source: validator MUST trip the + 'no GitHub access configured' branch. The `env` fixture keeps the other + required fields satisfied so we isolate the credential-validator path.""" + monkeypatch.setenv("GITHUB_TOKEN", "") + monkeypatch.setenv("ROBOMP_GH_PROXY_URL", "") + monkeypatch.setenv("ROBOMP_GH_PROXY_HMAC_KEY", "") + reset_settings_cache() + with pytest.raises(ValidationError, match="no GitHub access configured"): + Settings() # type: ignore[call-arg] + + +def test_orchestrator_mode_loads_proxy_config(env: dict[str, str]) -> None: + cfg = Settings() # type: ignore[call-arg] + assert cfg.github_token is None + assert cfg.gh_proxy_url == "http://gh-proxy.invalid:8081" + assert cfg.gh_proxy_hmac_key is not None + assert cfg.gh_proxy_hmac_key.get_secret_value().startswith("test-hmac-key") + + +def test_rejects_token_and_proxy_together(monkeypatch: pytest.MonkeyPatch, env: dict[str, str]) -> None: + monkeypatch.setenv("GITHUB_TOKEN", "x") reset_settings_cache() with pytest.raises(ValidationError): Settings() # type: ignore[call-arg] +def test_rejects_proxy_url_without_key(monkeypatch: pytest.MonkeyPatch, env: dict[str, str]) -> None: + monkeypatch.delenv("ROBOMP_GH_PROXY_HMAC_KEY", raising=False) + reset_settings_cache() + with pytest.raises(ValidationError): + Settings() # type: ignore[call-arg] + + +def test_proxy_mode_loads_pat(proxy_env: dict[str, str]) -> None: + cfg = Settings() # type: ignore[call-arg] + assert cfg.github_token is not None + assert cfg.github_token.get_secret_value() == "ghp_test_token_value_xxxxxxxxxxxxxxxx" + assert cfg.gh_proxy_url is None + assert cfg.gh_proxy_hmac_key is None + + def test_allowlist_csv_parsing(monkeypatch: pytest.MonkeyPatch, env: dict[str, str]) -> None: monkeypatch.setenv("ROBOMP_REPO_ALLOWLIST", " alpha/one ,beta/two, ,gamma/three ") reset_settings_cache() diff --git a/tests/test_db.py b/tests/test_db.py index f4ec88a22..5cd08aad0 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -90,10 +90,23 @@ def test_reset_stuck_running_recovers(db: Database) -> None: ) row = db.claim_next_event() assert row is not None + # Capture `started_at` set by the claim so we can prove the recovery flip preserves it. + with db._lock: # noqa: SLF001 + before = db._conn.execute( # noqa: SLF001 + "SELECT started_at FROM events WHERE delivery_id=?", ("d1",) + ).fetchone() + assert before is not None + assert before["started_at"] is not None # Simulate crash: row still running. recovered = db.reset_stuck_running() assert recovered == 1 assert db.get_event("d1").state == "queued" + with db._lock: # noqa: SLF001 + after = db._conn.execute( # noqa: SLF001 + "SELECT started_at FROM events WHERE delivery_id=?", ("d1",) + ).fetchone() + assert after is not None + assert after["started_at"] == before["started_at"] def test_upsert_issue_round_trip(db: Database) -> None: diff --git a/tests/test_host_tools.py b/tests/test_host_tools.py index 1b678ce48..b91e054de 100644 --- a/tests/test_host_tools.py +++ b/tests/test_host_tools.py @@ -15,7 +15,7 @@ from omp_rpc import HostToolContext, RpcCommandError from robomp.db import Database from robomp.github_client import GitHubClient, IssueInfo, RepoInfo from robomp.host_tools import ToolBindings, build -from robomp.sandbox import Workspace +from robomp.sandbox import LocalGitTransport, Workspace def _stub_workspace(tmp_path: Path) -> Workspace: @@ -81,6 +81,7 @@ def _bindings( bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=_stub_issue(), workspace=_stub_workspace(tmp_path), @@ -151,6 +152,7 @@ def test_gh_post_comment_defaults_to_inbound_pr_thread(db: Database, tmp_path: P bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=_stub_issue(), # issue #42 workspace=_stub_workspace(tmp_path), @@ -182,6 +184,7 @@ def test_gh_post_comment_explicit_number_overrides_inbound(db: Database, tmp_pat bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=_stub_issue(), workspace=_stub_workspace(tmp_path), @@ -493,6 +496,7 @@ def test_gh_push_branch_rejects_wrong_identity(db: Database, tmp_path: Path) -> bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -610,6 +614,7 @@ def test_gh_open_pr_rejects_wrong_identity_before_push_or_pr(db: Database, tmp_p bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -729,6 +734,7 @@ def test_gh_push_branch_rejects_invalid_identity_scan_range(db: Database, tmp_pa bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -836,9 +842,9 @@ def test_gh_open_pr_refuses_failed_bun_check_before_push_or_pr( _stop_loop(loop, t) msg = str(exc.value) - assert "`bun check` failed before PR creation" in msg + assert "refusing to open PR" in msg + assert "`bun check` failed before open PR" in msg assert "TypeError: property missing" in msg - assert "retry `gh_open_pr`" in msg assert not opened_pr row = db._conn.execute("SELECT error FROM tool_calls WHERE tool='gh_open_pr' ORDER BY id DESC LIMIT 1").fetchone() assert row is not None @@ -924,6 +930,7 @@ def test_gh_push_branch_rejects_dirty_worktree(db: Database, tmp_path: Path) -> bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -964,10 +971,12 @@ def test_gh_push_branch_rejects_dirty_worktree(db: Database, tmp_path: Path) -> _stop_loop(loop, thread) -def test_gh_push_branch_does_not_run_repository_bun_scripts( +def test_gh_push_branch_runs_fix_and_check_before_pushing( db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """A clean push must not execute repository-controlled package scripts during preflight.""" + """gh_push_branch must run `bun run fix` then `bun check` (when defined) + before the push reaches the remote. Same gate as `gh_open_pr` so a + follow-up commit can't break CI.""" import os import subprocess @@ -1009,36 +1018,41 @@ def test_gh_push_branch_does_not_run_repository_bun_scripts( ws = mgr.ensure_workspace( repo="octo/widget", number=42, - title="no repo scripts during push", + title="push gate", clone_url=str(bare), default_branch="main", author_name="robomp-bot", author_email="robomp-bot@example.invalid", ) - marker = tmp_path / "bun-invoked" + fix_calls = tmp_path / "fix-calls" + check_calls = tmp_path / "check-calls" fakebin = tmp_path / "fakebin" fakebin.mkdir() fake_bun = fakebin / "bun" - fake_bun.write_text(f"#!/bin/sh\nprintf invoked > {marker}\nprintf dirty > formatter-output.txt\nexit 0\n") + fake_bun.write_text( + "#!/bin/sh\n" + 'if [ "$1" = "run" ] && [ "$2" = "fix" ]; then\n' + f" printf called >> {fix_calls}\n" + ' printf "formatted\\n" > src.txt\n' + " exit 0\n" + "fi\n" + 'if [ "$1" = "check" ]; then\n' + f" printf called >> {check_calls}\n" + " exit 0\n" + "fi\n" + 'printf "unexpected bun call: %s\\n" "$*" >&2\n' + "exit 2\n" + ) fake_bun.chmod(0o755) monkeypatch.setenv("PATH", f"{fakebin}{os.pathsep}{os.environ['PATH']}") (ws.repo_dir / "package.json").write_text( - json.dumps( - { - "scripts": { - "fix:tools": "printf dirty > formatter-output.txt", - "fix": "printf dirty > formatter-output.txt", - }, - } - ) - + "\n" - ) - (ws.repo_dir / "feature.txt").write_text("feature\n") - subprocess.run( - ["git", "-C", str(ws.repo_dir), "add", "package.json", "feature.txt"], check=True, capture_output=True + json.dumps({"scripts": {"fix": "...", "check": "..."}}) + "\n", + encoding="utf-8", ) + (ws.repo_dir / "src.txt").write_text("original\n") + subprocess.run(["git", "-C", str(ws.repo_dir), "add", "package.json", "src.txt"], check=True, capture_output=True) subprocess.run( [ "git", @@ -1050,7 +1064,7 @@ def test_gh_push_branch_does_not_run_repository_bun_scripts( "user.name=robomp-bot", "commit", "-m", - "ok", + "feat: follow-up", ], check=True, capture_output=True, @@ -1063,6 +1077,7 @@ def test_gh_push_branch_does_not_run_repository_bun_scripts( bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -1092,9 +1107,20 @@ def test_gh_push_branch_does_not_run_repository_bun_scripts( finally: _stop_loop(loop, thread) + # Both gates ran, and fix preceded check (both have one call recorded). + assert fix_calls.read_text() == "called" + assert check_calls.read_text() == "called" + # The formatter's diff was committed by the bot as a `style: bun run fix` commit. + log = subprocess.run( + ["git", "-C", str(ws.repo_dir), "log", "--format=%an <%ae> %s", "-n", "2"], + capture_output=True, + text=True, + check=True, + ) + lines = log.stdout.strip().splitlines() + assert lines[0].startswith("robomp-bot style: bun run fix"), lines + # And the branch ended up on the remote at the new head. assert result.startswith(f"pushed {ws.branch} ") - assert not marker.exists() - assert not (ws.repo_dir / "formatter-output.txt").exists() refs = subprocess.run( ["git", "-C", str(bare), "for-each-ref", "--format=%(refname)"], capture_output=True, @@ -1104,6 +1130,315 @@ def test_gh_push_branch_does_not_run_repository_bun_scripts( assert f"refs/heads/{ws.branch}" in refs.stdout.splitlines() +def test_gh_push_branch_force_with_lease_recovers_after_amend(db: Database, tmp_path: Path) -> None: + """A divergent local history (amended commit) must push successfully. + + Plain `git push` rejects this as non-fast-forward, leaving the agent stuck. + `--force-with-lease` accepts the rewrite because origin still matches the + ref we last fetched.""" + import os + import subprocess + + bare = tmp_path / "upstream.git" + bare.mkdir() + subprocess.run(["git", "init", "--bare", "--initial-branch=main", str(bare)], check=True, capture_output=True) + seed = tmp_path / "seed" + seed.mkdir() + env = os.environ | { + "GIT_AUTHOR_NAME": "robomp-bot", + "GIT_AUTHOR_EMAIL": "robomp-bot@example.invalid", + "GIT_COMMITTER_NAME": "robomp-bot", + "GIT_COMMITTER_EMAIL": "robomp-bot@example.invalid", + } + subprocess.run(["git", "init", "--initial-branch=main", str(seed)], check=True, capture_output=True) + (seed / "README.md").write_text("init\n") + for cmd in ( + ["git", "-C", str(seed), "add", "."], + [ + "git", + "-C", + str(seed), + "-c", + "user.email=robomp-bot@example.invalid", + "-c", + "user.name=robomp-bot", + "commit", + "-m", + "init", + ], + ["git", "-C", str(seed), "remote", "add", "origin", str(bare)], + ["git", "-C", str(seed), "push", "origin", "main"], + ): + subprocess.run(cmd, check=True, capture_output=True, env=env) + + from robomp.sandbox import SandboxManager + + mgr = SandboxManager(tmp_path / "workspaces") + ws = mgr.ensure_workspace( + repo="octo/widget", + number=42, + title="amend recover", + clone_url=str(bare), + default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + # First commit + push — fast-forward path. + (ws.repo_dir / "feature.txt").write_text("original\n") + subprocess.run(["git", "-C", str(ws.repo_dir), "add", "feature.txt"], check=True, capture_output=True) + subprocess.run( + [ + "git", + "-C", + str(ws.repo_dir), + "-c", + "user.email=robomp-bot@example.invalid", + "-c", + "user.name=robomp-bot", + "commit", + "-m", + "feat: original", + ], + check=True, + capture_output=True, + env=env, + ) + + github = GitHubClient("tok", transport=httpx.MockTransport(lambda r: httpx.Response(500))) + loop, thread = _make_loop_in_background() + try: + bindings = ToolBindings( + db=db, + github=github, + git_transport=LocalGitTransport(token=None), + repo=_stub_repo(), + issue=IssueInfo( + repo="octo/widget", + number=42, + title="t", + body="", + state="open", + author="alice", + labels=(), + is_pull_request=False, + ), + workspace=ws, + loop=loop, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + db.upsert_issue( + key=bindings.issue_key, + repo="octo/widget", + number=42, + state="reproducing", + branch=ws.branch, + session_dir=str(ws.session_dir), + ) + tool = next(x for x in build(bindings) if x.name == "gh_push_branch") + tool.execute({}, _ctx()) + + # Confirm origin received the original commit. + first_remote = subprocess.run( + ["git", "-C", str(bare), "rev-parse", f"refs/heads/{ws.branch}"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + # Now amend the commit (simulates an agent reset-author rebase, or a + # code change applied via `git commit --amend`). + (ws.repo_dir / "feature.txt").write_text("amended\n") + subprocess.run(["git", "-C", str(ws.repo_dir), "add", "feature.txt"], check=True, capture_output=True) + subprocess.run( + [ + "git", + "-C", + str(ws.repo_dir), + "-c", + "user.email=robomp-bot@example.invalid", + "-c", + "user.name=robomp-bot", + "commit", + "--amend", + "--no-edit", + ], + check=True, + capture_output=True, + env=env, + ) + new_local = subprocess.run( + ["git", "-C", str(ws.repo_dir), "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + assert new_local != first_remote, "amend must rewrite the SHA" + + # Second push — divergent. Plain `git push` would reject; we expect success. + result = tool.execute({}, _ctx()) + finally: + _stop_loop(loop, thread) + + assert result.startswith(f"pushed {ws.branch} ") + final_remote = subprocess.run( + ["git", "-C", str(bare), "rev-parse", f"refs/heads/{ws.branch}"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + assert final_remote == new_local, (final_remote, new_local) + + +def test_gh_push_branch_aborts_on_failed_bun_check( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failing `bun check` aborts the push and leaves the remote untouched.""" + import os + import subprocess + + bare = tmp_path / "upstream.git" + bare.mkdir() + subprocess.run(["git", "init", "--bare", "--initial-branch=main", str(bare)], check=True, capture_output=True) + seed = tmp_path / "seed" + seed.mkdir() + env = os.environ | { + "GIT_AUTHOR_NAME": "robomp-bot", + "GIT_AUTHOR_EMAIL": "robomp-bot@example.invalid", + "GIT_COMMITTER_NAME": "robomp-bot", + "GIT_COMMITTER_EMAIL": "robomp-bot@example.invalid", + } + subprocess.run(["git", "init", "--initial-branch=main", str(seed)], check=True, capture_output=True) + (seed / "README.md").write_text("init\n") + for cmd in ( + ["git", "-C", str(seed), "add", "."], + [ + "git", + "-C", + str(seed), + "-c", + "user.email=robomp-bot@example.invalid", + "-c", + "user.name=robomp-bot", + "commit", + "-m", + "init", + ], + ["git", "-C", str(seed), "remote", "add", "origin", str(bare)], + ["git", "-C", str(seed), "push", "origin", "main"], + ): + subprocess.run(cmd, check=True, capture_output=True, env=env) + + from robomp.sandbox import SandboxManager + + mgr = SandboxManager(tmp_path / "workspaces") + ws = mgr.ensure_workspace( + repo="octo/widget", + number=42, + title="push aborted", + clone_url=str(bare), + default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + + fakebin = tmp_path / "fakebin" + fakebin.mkdir() + fake_bun = fakebin / "bun" + fake_bun.write_text( + "#!/bin/sh\n" + 'if [ "$1" = "check" ]; then\n' + ' printf "TypeError: property missing\\n" >&2\n' + " exit 1\n" + "fi\n" + "exit 0\n" + ) + fake_bun.chmod(0o755) + monkeypatch.setenv("PATH", f"{fakebin}{os.pathsep}{os.environ['PATH']}") + + (ws.repo_dir / "package.json").write_text( + json.dumps({"scripts": {"check": "tsc --noEmit"}}) + "\n", + encoding="utf-8", + ) + (ws.repo_dir / "feature.txt").write_text("feature\n") + subprocess.run( + ["git", "-C", str(ws.repo_dir), "add", "package.json", "feature.txt"], check=True, capture_output=True + ) + subprocess.run( + [ + "git", + "-C", + str(ws.repo_dir), + "-c", + "user.email=robomp-bot@example.invalid", + "-c", + "user.name=robomp-bot", + "commit", + "-m", + "ok", + ], + check=True, + capture_output=True, + env=env, + ) + + github = GitHubClient("tok", transport=httpx.MockTransport(lambda r: httpx.Response(500))) + loop, thread = _make_loop_in_background() + try: + bindings = ToolBindings( + db=db, + github=github, + git_transport=LocalGitTransport(token=None), + repo=_stub_repo(), + issue=IssueInfo( + repo="octo/widget", + number=42, + title="t", + body="", + state="open", + author="alice", + labels=(), + is_pull_request=False, + ), + workspace=ws, + loop=loop, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + db.upsert_issue( + key=bindings.issue_key, + repo="octo/widget", + number=42, + state="reproducing", + branch=ws.branch, + session_dir=str(ws.session_dir), + ) + tool = next(x for x in build(bindings) if x.name == "gh_push_branch") + with pytest.raises(RpcCommandError) as exc: + tool.execute({}, _ctx()) + finally: + _stop_loop(loop, thread) + + msg = str(exc.value) + assert "refusing to push" in msg + assert "`bun check` failed before push" in msg + assert "TypeError: property missing" in msg + # The branch must not have reached the remote. + refs = subprocess.run( + ["git", "-C", str(bare), "for-each-ref", "--format=%(refname)"], + capture_output=True, + text=True, + check=True, + ) + assert not any(r.startswith("refs/heads/farm/") for r in refs.stdout.splitlines()), refs.stdout + # Audit row attributes the failure to gh_push_branch, not gh_open_pr. + row = db._conn.execute( + "SELECT tool, error FROM tool_calls WHERE tool='gh_push_branch' ORDER BY id DESC LIMIT 1" + ).fetchone() + assert row is not None + assert "TypeError: property missing" in row["error"] + + def test_gh_open_pr_runs_fix_then_check_and_commits_fixup( db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1227,6 +1562,7 @@ def test_gh_open_pr_runs_fix_then_check_and_commits_fixup( bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", @@ -1401,6 +1737,7 @@ def test_gh_open_pr_skips_fix_when_no_script(db: Database, tmp_path: Path, monke bindings = ToolBindings( db=db, github=github, + git_transport=LocalGitTransport(token=None), repo=_stub_repo(), issue=IssueInfo( repo="octo/widget", diff --git a/tests/test_persona.py b/tests/test_persona.py index 7e4fb2d31..b6bef08e6 100644 --- a/tests/test_persona.py +++ b/tests/test_persona.py @@ -115,3 +115,18 @@ def test_kickoff_directive_prompt_embeds_thread_and_classify_instruction() -> No assert "reproduce + fix" in out # The kickoff variant must still tell the agent to classify first. assert "Classify first" in out + + +def test_resume_triage_renders_branch_and_issue() -> None: + out = persona.resume_triage( + repo=_Repo(), + issue=_Issue(), + workspace=_Workspace(), + ) + # Working branch surfaces literally so the agent sees what it's on. + assert "farm/abc/test" in out + # Issue identity surfaces with the title. + assert "octo/widget#1080" in out + assert "broken thing" in out + # The prompt instructs the agent to reconcile drift via fetch_issue_thread. + assert "fetch_issue_thread" in out diff --git a/tests/test_queue_cancel.py b/tests/test_queue_cancel.py index 57cb837cc..6332319c1 100644 --- a/tests/test_queue_cancel.py +++ b/tests/test_queue_cancel.py @@ -31,12 +31,17 @@ class _StubSandbox: """Sentinel; queue tests don't touch the workspace pool.""" +class _StubGitTransport: + """Sentinel; queue tests don't push.""" + + def _make_pool(settings: Settings, db: Database) -> WorkerPool: return WorkerPool( settings=settings, db=db, github=_StubGitHub(), # type: ignore[arg-type] sandbox=_StubSandbox(), # type: ignore[arg-type] + git_transport=_StubGitTransport(), # type: ignore[arg-type] ) @@ -176,6 +181,36 @@ async def test_non_cancelled_failure_keeps_real_traceback( assert "cancelled by operator" not in stored.last_error +@pytest.mark.asyncio +async def test_run_event_marks_failed_when_not_shutting_down( + settings: Settings, db: Database, monkeypatch: pytest.MonkeyPatch +) -> None: + """When `_shutting_down` is False, a dispatch failure still marks the row failed.""" + pool = _make_pool(settings, db) + assert pool._shutting_down is False # noqa: SLF001 + db.record_event( + delivery_id="d5", + event_type="issues", + repo="octo/widget", + issue_key="octo/widget#1", + payload={"action": "opened"}, + state="running", + ) + row = _row("d5") + + async def fake_dispatch(self: WorkerPool, r: EventRow) -> None: + raise RuntimeError("regular failure") + + monkeypatch.setattr(WorkerPool, "_dispatch", fake_dispatch) + await pool._run_event(row) # noqa: SLF001 + + stored = db.get_event("d5") + assert stored is not None + assert stored.state == "failed" + assert stored.last_error is not None + assert "regular failure" in stored.last_error + + @pytest.mark.asyncio async def test_cancel_unknown_delivery_returns_false(settings: Settings, db: Database) -> None: """Cancelling an unknown delivery is a no-op that returns False.""" diff --git a/tests/test_queue_shutdown.py b/tests/test_queue_shutdown.py new file mode 100644 index 000000000..21db29926 --- /dev/null +++ b/tests/test_queue_shutdown.py @@ -0,0 +1,141 @@ +"""Graceful shutdown drain + kill behavior on WorkerPool. + +These tests poke `WorkerPool` directly: they don't spin up a dispatcher loop +or omp subprocess. The contract under test is `stop()`'s drain-then-kill +sequence and `_run_event`'s shutting-down branch that leaves the DB row in +`running` so `reset_stuck_running()` can requeue it. +""" + +from __future__ import annotations + +import asyncio +from contextlib import suppress + +import pytest + +from robomp.config import Settings +from robomp.db import Database, EventRow +from robomp.queue import WorkerPool + + +class _StubGitHub: + """Sentinel; queue tests don't talk to GitHub.""" + + +class _StubSandbox: + """Sentinel; queue tests don't touch the workspace pool.""" + + +class _StubGitTransport: + """Sentinel; queue tests don't push.""" + + +def _make_pool(settings: Settings, db: Database) -> WorkerPool: + return WorkerPool( + settings=settings, + db=db, + github=_StubGitHub(), # type: ignore[arg-type] + sandbox=_StubSandbox(), # type: ignore[arg-type] + git_transport=_StubGitTransport(), # type: ignore[arg-type] + ) + + +def _row(delivery: str = "d1") -> EventRow: + return EventRow( + delivery_id=delivery, + event_type="issues", + repo="octo/widget", + issue_key="octo/widget#1", + payload={"action": "opened"}, + received_at="2026-01-01T00:00:00Z", + state="running", + attempts=1, + last_error=None, + ) + + +@pytest.mark.asyncio +async def test_stop_drains_inflight_within_timeout(settings: Settings, db: Database) -> None: + """A short in-flight task finishes during the drain window; no kill hook needed.""" + pool = _make_pool(settings, db) + + async def short_coro() -> None: + await asyncio.sleep(0.05) + + task = asyncio.create_task(short_coro()) + pool._inflight_tasks[task] = "d-short" # noqa: SLF001 + + await pool.stop(drain_timeout=1.0, kill_timeout=0.1) + + assert pool._shutting_down is True # noqa: SLF001 + assert task.done() + + +@pytest.mark.asyncio +async def test_stop_fires_kill_hook_when_drain_exceeds_timeout(settings: Settings, db: Database) -> None: + """When drain times out, stop() pops and runs the registered cancel hook. + + The DB row stays `running` because `_run_event` (not exercised here) is + the only path that mutates state, and even when triggered post-kill the + shutting_down flag suppresses `mark_event(..., 'failed')`. + """ + pool = _make_pool(settings, db) + db.record_event( + delivery_id="d-blocked", + event_type="issues", + repo="octo/widget", + issue_key="octo/widget#1", + payload={"action": "opened"}, + state="running", + ) + + hook_called = asyncio.Event() + pool._cancel_hooks["d-blocked"] = hook_called.set # noqa: SLF001 + + never = asyncio.Event() + + async def _park() -> None: + await never.wait() + + blocked = asyncio.create_task(_park()) + pool._inflight_tasks[blocked] = "d-blocked" # noqa: SLF001 + + await pool.stop(drain_timeout=0.05, kill_timeout=0.05) + + assert hook_called.is_set() + stored = db.get_event("d-blocked") + assert stored is not None + assert stored.state == "running" + + blocked.cancel() + with suppress(asyncio.CancelledError): + await blocked + + +@pytest.mark.asyncio +async def test_run_event_skips_mark_event_when_shutting_down( + settings: Settings, db: Database, monkeypatch: pytest.MonkeyPatch +) -> None: + """During shutdown, a dispatch exception MUST leave the row untouched.""" + pool = _make_pool(settings, db) + pool._shutting_down = True # noqa: SLF001 + + db.record_event( + delivery_id="d-shutdown", + event_type="issues", + repo="octo/widget", + issue_key="octo/widget#1", + payload={"action": "opened"}, + state="running", + ) + + async def fake_dispatch(self: WorkerPool, r: EventRow) -> None: + raise RuntimeError("omp died") + + monkeypatch.setattr(WorkerPool, "_dispatch", fake_dispatch) + await pool._run_event(_row("d-shutdown")) # noqa: SLF001 + + stored = db.get_event("d-shutdown") + assert stored is not None + assert stored.state == "running" + assert stored.last_error is None diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index c1eef78f2..e94caa834 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -60,6 +60,8 @@ def test_ensure_workspace_creates_worktree(tmp_path: Path, upstream_repo: Path) title="something is wrong", clone_url=str(upstream_repo), default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", ) assert ws.repo_dir.is_dir() assert (ws.repo_dir / "README.md").read_text() == "hello\n" @@ -87,6 +89,8 @@ def test_ensure_workspace_is_idempotent(tmp_path: Path, upstream_repo: Path) -> title="t", clone_url=str(upstream_repo), default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", ) ws2 = mgr.ensure_workspace( repo="octo/widget", @@ -94,6 +98,8 @@ def test_ensure_workspace_is_idempotent(tmp_path: Path, upstream_repo: Path) -> title="t", clone_url=str(upstream_repo), default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", ) assert ws1.repo_dir == ws2.repo_dir assert ws1.branch == ws2.branch @@ -107,6 +113,8 @@ def test_remove_workspace(tmp_path: Path, upstream_repo: Path) -> None: title="t", clone_url=str(upstream_repo), default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", ) assert ws.repo_dir.exists() mgr.remove_workspace(repo="octo/widget", number=12) diff --git a/tests/test_server.py b/tests/test_server.py index 65f321abc..39fc0590f 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -16,6 +16,7 @@ from robomp.dashboard import tail_jsonl from robomp.db import close_database, get_database, issue_key from robomp.github_client import GitHubClient from robomp.manual_triage import InvalidIssueRef, parse_issue_ref +from robomp.sandbox import LocalGitTransport from robomp.server import create_app @@ -1300,6 +1301,7 @@ async def test_handle_comment_directive_bootstraps_untriaged_issue( settings=settings, db=db, github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), sandbox=sandbox, payload=payload, delivery_id="test-delivery-1", @@ -1369,6 +1371,7 @@ async def test_handle_comment_directive_reopens_finalized_issue( settings=settings, db=db, github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), sandbox=sandbox, payload=payload, delivery_id="test-delivery-2", @@ -1442,6 +1445,7 @@ async def test_handle_comment_finalized_without_directive_still_replies( settings=settings, db=db, github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), sandbox=sandbox, payload=payload, delivery_id="test-delivery-3", @@ -1519,6 +1523,7 @@ async def test_directive_handler_attaches_thread_from_github( settings=settings, db=db, github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), sandbox=sandbox, payload=payload, delivery_id="test-delivery-4", diff --git a/tests/test_worker.py b/tests/test_worker.py new file mode 100644 index 000000000..1b012ee0d --- /dev/null +++ b/tests/test_worker.py @@ -0,0 +1,227 @@ +"""Resume-aware behavior of `worker._run_rpc_blocking`. + +These tests swap `robomp.worker.RpcClient` for a recording fake so we can +observe the `extra_args` and `set_todos` decisions the driver takes based on +whether the workspace's omp session directory already holds a JSONL transcript. +""" + +from __future__ import annotations + +import asyncio +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from robomp import worker +from robomp.config import Settings + + +class _FakeRpcClient: + instances: list[_FakeRpcClient] = [] + + def __init__(self, **kwargs): + self.kwargs = kwargs + self.set_todos_calls: list[list[dict]] = [] + self.get_todos_calls = 0 + _FakeRpcClient.instances.append(self) + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def install_headless_ui(self) -> None: + pass + + def on_tool_execution_end(self, _cb) -> None: + pass + + def on_message_update(self, _cb) -> None: + pass + + def stop(self) -> None: + pass + + def set_todos(self, phases): + self.set_todos_calls.append(phases) + + def get_todos(self): + self.get_todos_calls += 1 + return () + + def prompt_and_wait(self, prompt, timeout): + class _Turn: + messages: list = [] + events: list = [] + assistant_text: str = "ok" + + return _Turn() + + +_SEEDED_PHASES = [ + { + "id": "p1", + "name": "Reproduce", + "tasks": [ + { + "id": "t1", + "content": "do it", + "status": "pending", + "notes": "", + "details": "", + } + ], + } +] + + +def _make_inputs( + tmp_path: Path, settings: Settings, *, session_has_jsonl: bool +) -> tuple[worker.TaskInputs, SimpleNamespace]: + session_dir = tmp_path / "session" + session_dir.mkdir() + if session_has_jsonl: + (session_dir / "foo.jsonl").write_text("{}\n", encoding="utf-8") + repo_dir = tmp_path / "repo" + repo_dir.mkdir() + + workspace = SimpleNamespace( + session_dir=session_dir, + repo_dir=repo_dir, + branch="robomp/issue-1", + ) + repo = SimpleNamespace(full_name="acme/widgets", owner="acme", name="widgets") + issue = SimpleNamespace(repo="acme/widgets", number=1, title="bug") + + db = SimpleNamespace(set_event_model=lambda _did, _model: None) + github = SimpleNamespace() + + inputs = worker.TaskInputs( + settings=settings, + db=db, # type: ignore[arg-type] + github=github, # type: ignore[arg-type] + git_transport=SimpleNamespace(), # type: ignore[arg-type] + repo=repo, # type: ignore[arg-type] + issue=issue, # type: ignore[arg-type] + workspace=workspace, # type: ignore[arg-type] + delivery_id="d-test", + attempts=0, + ) + bindings = SimpleNamespace( + workspace=workspace, + repo=repo, + issue=issue, + issue_key=f"{repo.full_name}#{issue.number}", + ) + return inputs, bindings + + +@pytest.fixture(autouse=True) +def _reset_fake() -> None: + _FakeRpcClient.instances.clear() + + +@pytest.fixture(autouse=True) +def _patch_worker(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("robomp.worker.RpcClient", _FakeRpcClient) + monkeypatch.setattr("robomp.worker.host_tools.build", lambda _b: ()) + monkeypatch.setattr( + "robomp.worker.persona.system_append", + lambda *, repo, issue, workspace: "SYS", + ) + monkeypatch.setattr( + "robomp.worker.persona.seed_phases", + lambda _kind: [dict(p) for p in _SEEDED_PHASES], + ) + + +@pytest.mark.asyncio +async def test_run_rpc_passes_continue_when_session_jsonl_present(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=True) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="triage_issue", + prompt="x", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + assert _FakeRpcClient.instances[0].kwargs["extra_args"] == ("--continue",) + + +@pytest.mark.asyncio +async def test_run_rpc_omits_continue_when_session_empty(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="triage_issue", + prompt="x", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + assert _FakeRpcClient.instances[0].kwargs["extra_args"] == () + + +@pytest.mark.asyncio +async def test_run_rpc_skips_set_todos_on_resumed_triage(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=True) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="triage_issue", + prompt="x", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + assert _FakeRpcClient.instances[0].set_todos_calls == [] + + +@pytest.mark.asyncio +async def test_run_rpc_seeds_todos_on_fresh_triage(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="triage_issue", + prompt="x", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + calls = _FakeRpcClient.instances[0].set_todos_calls + assert len(calls) == 1 + assert calls[0] == _SEEDED_PHASES + + +@pytest.mark.asyncio +async def test_run_rpc_merges_todos_on_followup_with_resume(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=True) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="handle_comment", + prompt="x", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + client = _FakeRpcClient.instances[0] + assert client.get_todos_calls == 1 + assert len(client.set_todos_calls) == 1 + assert len(client.set_todos_calls[0]) == len(_SEEDED_PHASES) diff --git a/tests/test_worker_smoke.py b/tests/test_worker_smoke.py index d3f65f298..2d281af0e 100644 --- a/tests/test_worker_smoke.py +++ b/tests/test_worker_smoke.py @@ -68,12 +68,14 @@ def test_triage_end_to_end(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> N from robomp.config import Settings, reset_settings_cache from robomp.db import Database from robomp.github_client import GitHubClient - from robomp.sandbox import SandboxManager + from robomp.sandbox import LocalGitTransport, SandboxManager from robomp.tasks import triage_issue bare = _seed_failing_repo(tmp_path) - monkeypatch.setenv("GITHUB_TOKEN", "ghp_test") + monkeypatch.setenv("ROBOMP_GH_PROXY_URL", "http://gh-proxy.invalid:8081") + monkeypatch.setenv("ROBOMP_GH_PROXY_HMAC_KEY", "test-hmac-key-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + monkeypatch.delenv("GITHUB_TOKEN", raising=False) monkeypatch.setenv("GITHUB_WEBHOOK_SECRET", "secret") monkeypatch.setenv("ROBOMP_BOT_LOGIN", "robomp-bot") monkeypatch.setenv("ROBOMP_REPO_ALLOWLIST", "octo/widget") @@ -171,6 +173,7 @@ def test_triage_end_to_end(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> N settings=cfg, db=db, github=github, + git_transport=LocalGitTransport(token=None), sandbox=sandbox, payload=payload, delivery_id="smoke-test",