diff --git a/codeframe/core/adapters/opencode.py b/codeframe/core/adapters/opencode.py index 86ffb2bc..c82d0096 100644 --- a/codeframe/core/adapters/opencode.py +++ b/codeframe/core/adapters/opencode.py @@ -10,39 +10,69 @@ class OpenCodeAdapter(SubprocessAdapter): """Adapter that delegates code execution to OpenCode CLI. - Invokes ``opencode`` with ``--non-interactive`` flag for headless execution. - The prompt is piped via stdin. + Runs ``opencode run `` — the CLI's headless entry point. The + previous invocation was ``opencode --non-interactive`` with the prompt on + stdin, and **no such flag exists**: verified against opencode 1.18.7, + ``--non-interactive`` is absent from the option list and passing it simply + starts the TUI, so the delegated run did no work at all (#913). - Requires OpenCode to be installed: - https://github.com/opencode-ai/opencode + Requires OpenCode to be installed: https://github.com/sst/opencode """ - def __init__(self) -> None: - super().__init__(binary="opencode", cli_args=["--non-interactive"]) + def __init__(self, auto_approve: bool = False) -> None: + """Initialize the OpenCode adapter. + + Args: + auto_approve: Pass ``--auto``, which opencode documents as + "auto-approve permissions that are not explicitly denied + (dangerous!)". **Off by default.** Verified against opencode + 1.18.7 that a plain ``opencode run `` writes files + headlessly under the default permission config, so the flag is + not needed to make the engine work — and turning it on would + auto-approve arbitrary actions for a prompt derived from + repository content, the exposure #905–#907 exist to close. + + Where an operator's opencode config *does* deny writes, the run + produces no file changes and ``require_file_changes`` below turns + that into a loud failure rather than a silent false completion. + """ + cli_args = ["run"] + if auto_approve: + cli_args.append("--auto") + + super().__init__( + binary="opencode", + cli_args=cli_args, + # A coding agent that exits 0 having written nothing is a false + # completion: gates then run on an unchanged tree and the task can be + # marked DONE with no code. Same guard the claude-code adapter got + # in #739/#819. + require_file_changes=True, + ) + self._auto_approve = auto_approve @property def name(self) -> str: # noqa: D102 return "opencode" def build_command(self, prompt: str, workspace_path: Path) -> list[str]: - """Build opencode CLI command. + """Build the opencode CLI command. Args: - prompt: The task prompt (sent via stdin, not in the command). + prompt: The task prompt — a positional argument to ``run``, not + stdin. ``opencode run`` declares ``message`` as a positional + array; it does not read the prompt from stdin. workspace_path: Workspace root (cwd is set by the base class). Returns: Command list for subprocess.Popen. """ - return [self._binary_path, *self._cli_args] + return [self._binary_path, *self._cli_args, prompt] def get_stdin(self, prompt: str) -> str | None: - """Send prompt via stdin. - - Args: - prompt: The task prompt to pipe into the opencode process. + """No stdin: the prompt travels as an argument to ``run``. Returns: - The prompt string. + None — sending it twice would duplicate the instruction. """ - return prompt + return None diff --git a/tests/core/adapters/test_opencode.py b/tests/core/adapters/test_opencode.py index ac4c36e1..da228f31 100644 --- a/tests/core/adapters/test_opencode.py +++ b/tests/core/adapters/test_opencode.py @@ -14,8 +14,17 @@ class TestOpenCodeAdapter: @pytest.fixture(autouse=True) def _no_git(self): - """Prevent _detect_modified_files from calling real git.""" - with patch.object(OpenCodeAdapter, "_detect_modified_files", return_value=[]): + """Prevent _detect_modified_files from calling real git. + + Returns a modified file, i.e. a run that actually did work. With + ``require_file_changes=True`` (#913) an empty list means "wrote + nothing", which is now a *failure* — so an empty default would make + every test here exercise the false-completion guard rather than the + behaviour it names. The no-work case has its own test below. + """ + with patch.object( + OpenCodeAdapter, "_detect_modified_files", return_value=["main.py"] + ), patch.object(OpenCodeAdapter, "_git_head", return_value="abc123"): yield def test_name(self) -> None: @@ -33,17 +42,46 @@ def test_raises_if_opencode_not_installed(self) -> None: with pytest.raises(EnvironmentError, match="not found on PATH"): OpenCodeAdapter() - def test_build_command_includes_non_interactive(self) -> None: + def test_build_command_uses_the_run_subcommand(self) -> None: + """`--non-interactive` does not exist; it silently starts the TUI (#913).""" with patch("shutil.which", return_value="/usr/bin/opencode"): adapter = OpenCodeAdapter() cmd = adapter.build_command("prompt", Path("/tmp")) - assert cmd[0] == "/usr/bin/opencode" - assert "--non-interactive" in cmd - def test_sends_prompt_via_stdin(self) -> None: + assert cmd == ["/usr/bin/opencode", "run", "prompt"] + assert "--non-interactive" not in cmd + + def test_the_prompt_is_an_argument_not_stdin(self) -> None: + """`opencode run` declares `message` as a positional; it reads no stdin. + + Returning the prompt here as well would send the instruction twice. + """ + with patch("shutil.which", return_value="/usr/bin/opencode"): + adapter = OpenCodeAdapter() + + assert adapter.get_stdin("my prompt") is None + assert adapter.build_command("my prompt", Path("/tmp"))[-1] == "my prompt" + + def test_a_zero_work_run_is_not_reported_completed(self) -> None: + """Exit 0 with nothing written is a false completion: gates would then + run on an unchanged tree and the task could be marked DONE with no code + (#739's class, which the claude-code adapter was already patched for).""" with patch("shutil.which", return_value="/usr/bin/opencode"): adapter = OpenCodeAdapter() - assert adapter.get_stdin("my prompt") == "my prompt" + + assert adapter._require_file_changes is True + + def test_auto_approve_is_off_by_default(self) -> None: + """opencode documents --auto as "(dangerous!)" — it auto-approves + anything not explicitly denied, for a prompt derived from repository + content. Verified against 1.18.7 that plain `run` writes files, so the + flag is not needed to make the engine work.""" + with patch("shutil.which", return_value="/usr/bin/opencode"): + default = OpenCodeAdapter() + opted_in = OpenCodeAdapter(auto_approve=True) + + assert "--auto" not in default.build_command("p", Path("/tmp")) + assert "--auto" in opted_in.build_command("p", Path("/tmp")) def test_successful_execution(self) -> None: with patch("shutil.which", return_value="/usr/bin/opencode"): @@ -101,3 +139,51 @@ def test_event_callback_receives_output_lines(self) -> None: assert len(events) == 2 assert events[0].data["line"] == "line one" assert events[1].data["line"] == "line two" + + +class TestZeroWorkIsNotCompleted: + """The false-completion guard, exercised through `run` (#913).""" + + @pytest.fixture(autouse=True) + def _stable_head(self): + """HEAD unchanged, so only the modified-file signal decides.""" + with patch.object(OpenCodeAdapter, "_git_head", return_value="abc123"): + yield + + def _adapter(self) -> OpenCodeAdapter: + with patch("shutil.which", return_value="/usr/bin/opencode"): + return OpenCodeAdapter() + + def _exit_zero_process(self) -> MagicMock: + proc = MagicMock() + proc.stdout = iter(["I reviewed the code and everything looks fine.\n"]) + proc.stderr = MagicMock() + proc.stderr.read.return_value = "" + proc.stdin = MagicMock() + proc.returncode = 0 + proc.wait.return_value = None + return proc + + def test_exit_zero_writing_nothing_is_not_completed(self) -> None: + """The reported failure mode: the CLI exits 0 having done no work, gates + then run on an unchanged tree, and the task can be marked DONE with no + code written.""" + adapter = self._adapter() + + with patch.object(OpenCodeAdapter, "_detect_modified_files", return_value=[]): + with patch("subprocess.Popen", return_value=self._exit_zero_process()): + result = adapter.run("task-1", "implement feature", Path("/tmp/repo")) + + assert result.status != "completed", result.status + + def test_exit_zero_with_changes_is_completed(self) -> None: + """The guard must not fail runs that genuinely did the work.""" + adapter = self._adapter() + + with patch.object( + OpenCodeAdapter, "_detect_modified_files", return_value=["main.py"] + ): + with patch("subprocess.Popen", return_value=self._exit_zero_process()): + result = adapter.run("task-1", "implement feature", Path("/tmp/repo")) + + assert result.status == "completed", result.status diff --git a/tests/core/adapters/test_opencode_smoke_913.py b/tests/core/adapters/test_opencode_smoke_913.py new file mode 100644 index 00000000..aefd62f8 --- /dev/null +++ b/tests/core/adapters/test_opencode_smoke_913.py @@ -0,0 +1,143 @@ +"""Binary-gated smoke test: the OpenCode adapter against the real CLI (#913). + +The adapter shipped invoking ``opencode --non-interactive`` with the prompt on +stdin. **No such flag exists.** Verified against opencode 1.18.7: it is absent +from the option list, and passing it starts the TUI — so the delegated run did +no work, exited non-zero on the help path, and unit tests built entirely from +mocks could not tell. + +That is the gap this file closes. Every assertion here runs the adapter's own +``build_command`` output against the installed binary, so a future change to +the invocation that unit tests would happily mock is caught here instead. + +Skipped when ``opencode`` is not installed, so CI without the binary stays +green — but it is **launch-gating**, not deferred: the engine is one of the +shipped multi-model options. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from codeframe.core.adapters.opencode import OpenCodeAdapter + +pytestmark = [ + pytest.mark.v2, + pytest.mark.skipif( + shutil.which("opencode") is None, reason="opencode CLI not installed" + ), +] + +#: Long enough for a real model round-trip, short enough to fail a hang loudly. +_TIMEOUT_S = 240 + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + workspace = tmp_path / "repo" + workspace.mkdir() + subprocess.run(["git", "init", "-q"], cwd=workspace, check=True) + return workspace + + +def test_the_run_subcommand_exists_and_non_interactive_does_not() -> None: + """The assumption that produced the bug, checked against the binary. + + Asserted on the CLI's own help rather than on our code, so this fails if a + future opencode release moves the headless entry point out from under us. + """ + proc = subprocess.run( + ["opencode", "--help"], capture_output=True, text=True, timeout=60 + ) + # opencode prints help on stderr, not stdout. + help_text = proc.stdout + proc.stderr + + assert "opencode run" in help_text, "the headless entry point moved" + assert "--non-interactive" not in help_text, ( + "--non-interactive now exists; revisit the adapter's invocation" + ) + + +def test_the_adapter_command_actually_writes_a_file(repo: Path) -> None: + """Runs exactly what the adapter builds, and asserts on the file it produces. + + Not "exit 0" — the old invocation could exit 0 and do nothing, which is the + whole false-completion class here. + """ + adapter = OpenCodeAdapter() + cmd = adapter.build_command( + "Create a file named smoke.txt containing exactly the word: works", repo + ) + + try: + proc = subprocess.run( + cmd, cwd=repo, capture_output=True, text=True, timeout=_TIMEOUT_S + ) + except subprocess.TimeoutExpired: + pytest.skip(f"opencode did not complete within {_TIMEOUT_S}s") + + produced = repo / "smoke.txt" + if not produced.exists() and "smoke.txt" in proc.stdout: + # opencode reported writing the file but it is not in the workspace — + # it resolves its project directory from the *parent* process, ignoring + # the subprocess cwd every other adapter relies on. Tracked as + # #1007 [P0.27]; distinct from this issue's contract. + pytest.xfail("opencode ignored cwd and wrote outside the workspace") + + assert produced.exists(), ( + f"the adapter's command wrote nothing.\n" + f"cmd={cmd}\nstdout={proc.stdout[-2000:]}\nstderr={proc.stderr[-2000:]}" + ) + assert "works" in produced.read_text() + + +def test_the_old_invocation_does_no_work(repo: Path) -> None: + """Pins the actual bug, so nobody restores the flag believing it worked. + + ``opencode --non-interactive`` prints the banner/usage and writes nothing. + """ + proc = subprocess.run( + ["opencode", "--non-interactive"], + cwd=repo, + input="Create a file named old.txt containing: works", + capture_output=True, + text=True, + timeout=60, + ) + + assert not (repo / "old.txt").exists(), ( + "the old invocation wrote a file; the premise of #913 no longer holds" + ) + assert proc.returncode != 0 or "Commands:" in proc.stdout + + +def test_a_run_that_writes_nothing_is_not_reported_completed(repo: Path) -> None: + """End to end through `adapter.run`, in a real git repo, with a prompt that + asks for no file changes — the guard must catch it.""" + adapter = OpenCodeAdapter() + + try: + result = adapter.run( + "task-smoke", + "Reply with the single word ACKNOWLEDGED. Do not create, edit or " + "delete any files.", + repo, + ) + except subprocess.TimeoutExpired: + pytest.skip("opencode did not complete in time") + + changed = subprocess.run( + ["git", "status", "--porcelain"], cwd=repo, capture_output=True, text=True + ).stdout.strip() + + if changed: + pytest.skip(f"the model wrote files anyway: {changed!r}") + + assert result.status != "completed", ( + f"a run that wrote nothing was reported {result.status!r} — gates would " + f"then pass on an unchanged tree" + )