fix(engines): invoke opencode run and require file changes (#913) - #1008
fix(engines): invoke opencode run and require file changes (#913)#1008frankbria wants to merge 1 commit into
Conversation
|
Claude finished @frankbria's task in 8m 4s —— View job Precision bug-hunting review — PR #1008 (opencode run invocation)
Findings (2)
Delivered as 2 inline comments (with a committable suggestion on the minor) + one summary comment. No code was modified; no builds/tests run. Did not approve or request changes. |
| Command list for subprocess.Popen. | ||
| """ | ||
| return [self._binary_path, *self._cli_args] | ||
| return [self._binary_path, *self._cli_args, prompt] |
There was a problem hiding this comment.
[major] The task prompt is passed as a positional argv element, but CodeFrame prompts routinely exceed the OS single-argument limit (~128 KB on Linux), so large-prompt opencode runs fail at process start.
Failure scenario: TaskContextPackager.build assembles the prompt from to_prompt_context() — up to 10 KB of PRD (context.py:179) plus the full contents of every relevant file (context.py:230, each capped at DEFAULT_FILE_TOKENS * CHARS_PER_TOKEN), within a 100K-token budget. A prompt > ~131072 bytes hits Linux's MAX_ARG_STRLEN, so execve returns E2BIG and subprocess.Popen raises OSError: [Errno 7] Argument list too long. SubprocessAdapter.run catches it (subprocess_adapter.py:203) and returns AgentResult(status="failed", error="Failed to start 'opencode': ...") on every run. The prior stdin transport (this PR's old get_stdin returned the prompt) and the sibling ClaudeCodeAdapter (still stdin) have no such cap — this is a regression for the normal large-prompt task, not an edge case.
Fix needs CLI verification (the discipline this PR correctly followed, so I am not asserting it here): confirm whether opencode run with no positional reads the message from stdin, or accepts a prompt-file, and pipe the prompt that way; otherwise explicitly guard oversize prompts. I deliberately did not write a suggestion that assumes stdin support — that is exactly the unverified-contract mistake this issue exists to stop.
| def repo(tmp_path: Path) -> Path: | ||
| workspace = tmp_path / "repo" | ||
| workspace.mkdir() | ||
| subprocess.run(["git", "init", "-q"], cwd=workspace, check=True) |
There was a problem hiding this comment.
[minor] The repo fixture never makes an initial commit, so HEAD stays unborn and require_file_changes cannot fire — test_a_run_that_writes_nothing_is_not_reported_completed fails on its intended success path.
Failure scenario: with no commit, _git_head runs git rev-parse HEAD (exit 128) and returns None; in the guard in_git_repo = head_after is not None is False (subprocess_adapter.py:233), so the downgrade branch is skipped and a zero-work exit-0 run stays "completed". When opencode obeys "do not write files", result.status is "completed" and the assert result.status != "completed" on line 140 raises. The unit TestZeroWorkIsNotCompleted only passes because it patches _git_head to a sha — this smoke fixture does not, so the guard it exists to exercise never engages.
| subprocess.run(["git", "init", "-q"], cwd=workspace, check=True) | |
| subprocess.run(["git", "init", "-q"], cwd=workspace, check=True) | |
| subprocess.run( | |
| [ | |
| "git", "-c", "user.email=smoke@test", "-c", "user.name=smoke", | |
| "commit", "-q", "--allow-empty", "-m", "init", | |
| ], | |
| cwd=workspace, | |
| check=True, | |
| ) |
GLM precision review — PR #1008
Scope: concrete defects only (logic / data-handling / regressions). Style, architecture, naming, and test coverage are covered by the general-purpose review and intentionally not repeated here. |
Closes #913.
Problem
The adapter ran
opencode --non-interactivewith the prompt on stdin. No such flag exists. Verified against opencode 1.18.7:--non-interactiveis absent from the option listopencode run [message..], with the message as a positional, not stdinSo the delegated run did no work. And because
OpenCodeAdapterdid not setrequire_file_changes=True, an exit-0 no-op was reportedcompleted, gates ran on an unchanged tree, and the task could be marked DONE with no code written — the #739 false-completion class the claude-code adapter was already patched for.Fix
opencode --non-interactiveopencode run <message>get_stdinreturnsNone, so it is not sent twice)completedrequire_file_changes=True→failed--autois off by defaultI first added
--autoon the reasoning that a headless coding agent needs permission auto-approval. Testing showed that is not true — a plainopencode runwrites files fine under the default permission config. Since opencode documents--autoas "auto-approve permissions that are not explicitly denied (dangerous!)", and the prompt is derived from repository content, defaulting it on would add exactly the exposure #905–#907 closed. It is now opt-in (OpenCodeAdapter(auto_approve=True)).Where an operator's opencode config does deny writes, the run produces no file changes and
require_file_changesturns that into a loud failure rather than a silent false completion — so declining the flag degrades safely.Tests
Unit (
tests/core/adapters/test_opencode.py) — the--non-interactiveassertion is gone; new tests pin["opencode", "run", prompt],get_stdin() is None,require_file_changes, and the--autodefault.The autouse fixture returned
[]modified files, which under the new guard made three unrelated tests exercise the false-completion path instead of the behaviour they name. It now represents a run that did work, and the no-work case has its own tests (both directions: zero changes → notcompleted; changes →completed).Binary-gated smoke (
tests/core/adapters/test_opencode_smoke_913.py, criterion 4) — runs the adapter's ownbuild_commandoutput against the installed CLI, skipped whenopencodeis absent:opencode runexists and--non-interactivedoes not — asserted against the CLI's own help, so it fails if a future release moves the headless entry pointcompletedThe smoke test found a defect the issue does not cover — filed as #1007 [P0.27]
opencode ignores the subprocess
cwd. EverySubprocessAdaptertargets a workspace viacwd=workspace_path; opencode resolves its project directory from the parent process instead. Running the smoke test from the repo root, opencode reportedCreated \smoke.txt`and the file appeared in **/home/frankbria/projects/codeframe/`** — the pytest process's cwd — not the temp workspace. Reproduced twice.Under
codeframe servethat means a delegated task can edit CodeFRAME's own checkout while the task's workspace is untouched.require_file_changesdoes not save it: the guard inspects the workspace, finds nothing, and fails the run — correctly — while the edits have already landed elsewhere.opencode run --dir <workspace>looks like the remedy, but two attempts to verify it timed out (240s, 280s) and wrote nothing — and a plainopencode runtimed out immediately afterwards too, so the timeouts track opencode instability today, not--dir. I did not ship an unverified flag: assuming a CLI contract without checking is precisely what produced this issue. Filed as #1007 with the reproduction, and the smoke testxfails on exactly that condition so it converts into the regression test when #1007 is fixed.Known limitation
The two model-dependent smoke tests
skipon timeout rather than fail, so an upstream opencode outage does not red the suite. The CLI-contract assertions (help text, old invocation) are hard and deterministic.