Skip to content

feat(agents): add OpenCode harness with opt-in [opencode] extra - #115

Open
mohsen-uipath wants to merge 14 commits into
mainfrom
feat/opencode-harness
Open

feat(agents): add OpenCode harness with opt-in [opencode] extra#115
mohsen-uipath wants to merge 14 commits into
mainfrom
feat/opencode-harness

Conversation

@mohsen-uipath

@mohsen-uipath mohsen-uipath commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Adds a new agent kind opencode (registered via the existing plugin SPI, selectable with agent.type: opencode or -D agent.type=opencode) that drives opencode run --format json and reduces its nd-JSON stream into the standard event protocol — EventCollector builds the TurnRecord, no hand-assembled telemetry. Since OpenCode is model-agnostic, this is the cheap path for evaluating open-weight models (DeepSeek, Kimi, GLM, …) through one harness.

Highlights

  • Full Agent contract: pending-turn crash/timeout semantics, cooperative should_stop, one terminal event per turn; max_turns counts OpenCode's native steps (documented in the run-limit parity table).
  • Trustworthy telemetry: per-step token-bucket convention arbitrated from the stream's own total; the reconciliation invariant holds exactly; tool names normalized to the canonical vocabulary.
  • Cost never understated: provider-reported cost wins when non-zero; rate card covers omitted or $0-for-priced-tokens cases; openrouter/ model ids normalize to bare rate-card keys (mirrored in evalboard).
  • Loud failure over fake pass: a clean exit with zero recognized events crashes the turn instead of scoring an empty success.
  • Robust teardown: post-EOF reap bounded by the turn deadline; each invocation runs in its own process group and is swept on kill/stop (no leaked server children).
  • Opt-in install: the [opencode] extra is deliberately empty — OpenCode is a Node CLI; a missing binary fails at start() with the install command.

Model: everything is standardized on DeepSeek V4 Pro (openrouter/deepseek/deepseek-v4-pro) — the smoke task, docs examples, and tests — so the checked-in task runs out of the box. DeepSeek V4 Flash 0731 was dropped: every OpenRouter provider serving it is excluded by the account's data-policy settings (an account-level restriction, not a code issue), so no reference to it ships.

Validation (all on DeepSeek V4 Pro via OpenRouter): make verify green (4,149 tests, coverage gate met); live end-to-end runs confirm real telemetry with exact reconciliation (including cache-heavy traffic), zero warnings, zero leaked processes, --session resume across turns, and both tasks/run_limits/ parity fixtures behaving per contract (clean cap stop vs. timeout failure).

Docs: docs/agents/OPENCODE.md (wired into nav + generated indexes) + an OpenCode column in docs/agents/HARNESS_PARITY.md.


Results — the same 174 tasks on two models

174 tasks from skills/tests/tasks/ across 10 skill directories, run through the OpenCode harness. coder_eval @ fcb7dad, skills @ 1e16eedb, -e tests/experiments/default.yaml --type opencode -j 4 — tempdir driver, max_turns: 200, task_timeout: 1200, turn_timeout: 900. Identical task list, experiment, harness and concurrency for both; only -m differs.

DeepSeek V4 Pro GPT-5.6 Luna
Route Azure AI Foundry Bedrock (global. profile, /v1/responses)
Pass rate 140/174 = 80.5% 149/174 = 85.6%
SUCCESS 140 149
FAILURE 26 24
TIMEOUT 6 0
ERROR 2 1
Wall clock 127 min 49 min
Skill calls 166 189
Tasks invoking a skill 155/174 172/174
Total tokens 63,833,519 54,160,671
Cost $54.32 $3.17
Cost per passing task $0.388 $0.021

GPT-5.6 Luna wins on every axis: +5.1 pp pass rate, 2.6× faster, and ~17× cheaper — $0.021 vs $0.388 per passing task. It also produced no timeouts at all, where DeepSeek lost 6 tasks to the 1200s task cap.

By skill directory

Directory DeepSeek V4 Pro GPT-5.6 Luna
uipath-admin 56/63 (89%) 58/63 (92%)
uipath-coded-apps 27/30 (90%) 27/30 (90%)
uipath-human-in-the-loop 16/21 (76%) 20/21 (95%)
uipath-test 8/18 (44%) 8/18 (44%)
uipath-planner 14/15 (93%) 14/15 (93%)
uipath-solution 10/13 (77%) 10/13 (77%)
uipath-tasks 5/9 (56%) 8/9 (89%)
uipath-automation-discovery 2/2 (100%) 2/2 (100%)
uipath-maestro-case 1/2 (50%) 2/2 (100%)
uipath-platform 1/1 (100%) 0/1 (0%)
Total 140/174 (80.5%) 149/174 (85.6%)

The two models agree on most of the suite: 133 tasks pass on both, 18 fail on both, 7 pass only on DeepSeek and 16 only on Luna. uipath-test is the shared floor at 8/18 for both — a suite-side gap rather than a model gap. Luna's gains concentrate in uipath-human-in-the-loop (+19 pp) and uipath-tasks (+33 pp); the single uipath-platform regression is a one-task directory.

Tokens and cost basis

Bucket DeepSeek V4 Pro GPT-5.6 Luna
Input (uncached) 27,345,771 3,444
Output 433,988 403,799
Cache read 36,053,760 48,024,513
Cache write 0 5,728,915

Neither model has a rate-card entry in src/coder_eval/pricing.py, so both runs report $0.00. Costs above are computed by hand from the models.dev catalog entry for the provider each run actually used, so the two are priced from one source:

  • DeepSeek V4 Pro (azure): $1.74/M in, $3.48/M out. Azure publishes no cache-read rate; every provider at this same tier lists ~$0.145/M, used here as an estimate.
  • GPT-5.6 Luna (amazon-bedrock / openai.gpt-5.6-luna): $0.22/M in, $1.32/M out, $0.022/M cache read, $0.275/M cache write (base tier; a higher tier applies above 272k context).

Caveat on the cost gap: part of it is billing shape, not just the rate card. The Bedrock route cached the prompt prefix (5.7M cache writes, 48M cache reads, almost no uncached input), while the Azure route billed 27.3M tokens at full input rate — that single bucket is $47.58 of DeepSeek's $54.32.

New agent kind `opencode`, registered through the existing plugin SPI and
selectable from task YAML (agent.type: opencode) or the CLI
(-D agent.type=opencode). Drives `opencode run --format json`
non-interactively and reduces its nd-JSON event stream into the standardized
event protocol; EventCollector builds the TurnRecord, so no telemetry is
assembled by hand.

Telemetry and cost:
- Per-step token buckets with the input convention arbitrated per step from
  the stream's own `total` (flat vs nested); unverifiable or contradictory
  shapes warn once per turn instead of silently mis-booking a bucket.
- Real per-call cost from step_finish.cost; the rate card fills the gaps
  (cost omitted, or $0 reported for tokens the card prices above zero) so
  run totals are never understated. openrouter/ model prefixes normalize to
  the bare rate-card keys (mirrored in evalboard/lib/pricing.ts).
- The reconciliation invariant holds by construction: summing the four
  buckets across TurnRecord.messages equals token_usage exactly.
- Tool names normalize to the canonical vocabulary (bash -> Bash, ...) so
  one criterion scores identically across harnesses.

Failure paths per the Agent contract: AgentCrashError with a crashed=True
partial TurnRecord on pending_turn, TurnTimeoutError on deadline, cooperative
should_stop honored at event granularity, and a clean exit that recognized no
events crashes loudly instead of scoring as an empty success. stderr is
drained concurrently and every post-exit read is bounded (the CLI's server
child holds the inherited pipes open).

Opt-in install: the [opencode] extra is deliberately empty — OpenCode is a
Node CLI (npm install -g opencode-ai) and the harness imports no third-party
Python package; a missing binary fails at start() with the install command.

Validated live end-to-end (SUCCESS backed by real telemetry: turns, tokens,
tools, cost, exact reconciliation). Documented at docs/agents/OPENCODE.md and
wired into the docs nav and generated index surfaces.
… test every failure path

Closes the three deliberately-deferred hardening items:

- The post-EOF reap in _settle_turn was unbounded: a CLI that closed its
  stream but never exited hung the turn past its deadline, the one window
  where turn_timeout went unenforced. The reap now gets the deadline's
  remainder (TurnTimeoutError on expiry) or a fixed grace when no deadline is
  configured (AgentCrashError naming the wedge).

- kill()/kill_sync() signaled only the CLI pid, orphaning the server child
  that opencode run leaves holding the pipes — a slow process leak across a
  batch. Each invocation now runs in its own session (start_new_session), and
  teardown sweeps the spawned process groups with SIGKILL. OpenCode persists
  sessions on disk, so --session continuity survives the sweep. Verified
  live: zero leftover opencode processes after a real run.

- The failure paths were the least-tested code in the file. Eleven new tests
  cover: deadline expiry mid-stream and post-EOF (TurnTimeoutError, partial
  parked, single TIMEOUT terminal event, iteration rollback), the
  no-deadline wedge (AgentCrashError), external CancelledError (partial
  parked, CRASHED terminal event, cancellation re-raised), kill_sync from the
  watchdog thread, process-group sweep on stop/cooperative-stop, tool
  error/permission-denied capture, and the orphan-result branch.

Live smoke re-run under the new teardown: SUCCESS with exact reconciliation,
normalized tools, heavy cache traffic booked correctly, zero warnings, zero
leaked processes.
…F exit grace

The constant gained a second consumer in the bounded-reap change (the exit
grace in _settle_turn when no turn deadline is configured); the comment still
described only the SIGTERM->SIGKILL role. Comment text only.
…rate entry

deepseek-v4-flash-0731 is unusable on this OpenRouter account (every serving
provider is excluded by the account's data policy), so the checked-in smoke
task failed out of the box while all real validation ran on deepseek-v4-pro
anyway. Standardize every reference — smoke task, docs examples, config
docstring, tests — on v4-pro, and drop the now-orphaned flash-0731 rate-card
entry plus its evalboard parity listing (v4-pro was already priced on main).

The smoke task now passes as checked in, with no model override. Verified
live: SUCCESS 1.000, no data-policy error.
# Conflicts:
#	README.md
#	docs/index.md
#	docs/llms.txt
#	mkdocs.yml
Comment thread src/coder_eval/agents/opencode_agent.py Fixed
Comment thread tests/test_opencode_agent.py Fixed
- signal.SIGKILL does not exist on Windows, so pyright failed the Windows
  Smoke job on kill_sync. Resolve it once as _SIGKILL (SIGTERM fallback) and
  use it in kill_sync and the group sweep; the sweep itself was already a
  runtime no-op off POSIX.
- The Windows job also runs pytest: install the os.killpg test stub with
  raising=False (the attribute is absent there) and skip the
  process-group-teardown test class off POSIX, since the sweep it asserts is
  POSIX-only by design.
- CodeQL py/mixed-returns on communicate(): the final except ends in
  _crash_turn, whose NoReturn CodeQL cannot see — add an explicit unreachable
  raise so no path looks like an implicit None return.
- CodeQL py/ineffectual-statement on the cancellation test's bare
  'await task': bind the (never-produced) value so the statement's effect is
  explicit.
The E2E job runs --tags smoke-pass on Bedrock runners that have neither
the opencode CLI nor OpenRouter credentials, and pins the bucket at
exactly 7 tasks; the new task's smoke-pass tag made it an 8th,
un-runnable entry. Drop the tag (the task keeps smoke/opencode for
local runs) — live opencode coverage needs its own credentialed job,
the way Codex has one.
@uipreliga

Copy link
Copy Markdown
Collaborator

Let's wait for merging until we get some results from this code.

`plugins:` is how a task ships the skills under test, but the OpenCode agent
listed it among the fields it silently drops. A skill-injection run therefore
looked entirely normal while measuring the bare model: the only loadable skill
was OpenCode's built-in `customize-opencode`, and an attempt to load a real one
returned an error.

Map each local plugin root to OpenCode's `skills.paths`:

- Read the `skills` field of `<root>/.claude-plugin/plugin.json` (string or
  list), the same field Claude Code reads, so one `plugins:` line means the same
  thing on both harnesses. Fall back to the convention default `<root>/skills`,
  or to the root itself when it is already a bare skills directory.
- Hand the paths over via OPENCODE_CONFIG_CONTENT, which the CLI merges as a
  final local-scope layer. Chosen over writing `<sandbox>/.opencode/skills/`:
  it writes nothing into the sandbox that is later preserved as a run artifact
  and inspected by file criteria, and it does not depend on how the CLI resolves
  a project root from `--dir`. An inherited value is merged into, not clobbered;
  with no `plugins:` block the variable is untouched, so runs without one are
  byte-for-byte unchanged.
- Never point at a plugin root that has a skills subdir. `skills.paths` is
  scanned recursively and a root can hold a self-referential symlink, which
  resolves skills through an arbitrary path and drops duplicate names.

`--pure` skips external *plugins*, not configured skill paths, so the default
`pure: true` is unaffected.

Also make the engagement observable, without which the injection cannot be
told from the old behavior: map OpenCode's lowercase `skill` tool to the
canonical `Skill`, and read the skill name from `parameters["name"]` (OpenCode)
as well as `parameters["skill"]` (Claude).

Every way this can resolve to nothing — unset env var, missing directory, no
SKILL.md under the root — is warned at `start()`, and the resolved paths are
recorded per task under `environment_info.opencode_skill_paths`.

Verified against the real CLI: 1 -> 27 loadable skills, and a live smoke task
goes from an invented command at score 0.0 to `Skill` engagement plus the
correct invocation at score 1.000.
uipreliga

This comment was marked as outdated.

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Duplicate submission — see the full review comment immediately above. Body replaced to avoid double-posting.)

…contract

Closes the two review blockers on the OpenCode harness.

communicate()'s `finally` cancelled the stderr drain and dropped the process
handle without reaping the child. Two exits reach it with the CLI still
RUNNING — the `except Exception` crash (a StreamReader ValueError on an
over-long line, a malformed-payload TypeError in a handler) and an external
cancellation — and neither passes through the graceful `await self.kill()`
that the intentional cuts and _settle_turn use. That is not merely a leak:
AgentCrashError is categorized AGENT_CRASH (max_retries=2) and the
orchestrator's attempt-failure hook only drains pending_turn, so attempt 2
spawned a SECOND `opencode --dir <sandbox> --session <same id>` while attempt
1 was still editing the files the criteria were about to score — whichever
writer won decided the task's result. Clearing the handle first also meant
neither kill() nor kill_sync() could signal it afterwards, so off POSIX (where
the group sweep is a no-op by design) nothing reaped it at all.

_reap_orphaned_cli() now runs before the handle is dropped. It is deliberately
synchronous: it executes while a CancelledError is propagating, where an await
can itself be cut short and leave the child alive after all, so it uses
Process.kill() plus the group sweep — no suspension point. Skipping the SIGTERM
courtesy is right for a turn that is already lost; the graceful escalation in
kill() still owns every path with something left to flush. A clean turn is
untouched (the CLI has already exited, so the guard is a no-op and the server
child survives for the next turn's --session resume).

_build_env's mock-shadowing contract had zero assertions on it, though
start(env_path_prepend=..., plugin_tools_dir=...) is the abstract Agent.start()
contract and the orchestrator always supplies both. An inverted PATH join would
leave sandbox mock CLIs un-shadowed, so a task grading a mocked CLI exercises
the real binary, writes no invocation log, and scores 0 on every row with the
suite still green. Both lines were in the coverage-missing list; all three
sibling agents pin exactly this.

Nine new tests, each verified to FAIL against the unfixed code: four teardown
assertions (read-loop crash, external cancel, the POSIX group sweep on a
crashed turn, and the clean turn that must kill nothing) plus five on the
sandbox environment (ordered PATH prepend, PLUGIN_TOOLS_DIR exported,
inherited PLUGIN_TOOLS_DIR never clobbered, neither key touched without the
kwargs, host credentials inherited whole). Two mutations were confirmed caught:
inverting the PATH join order, and letting the sandbox value override an
inherited PLUGIN_TOOLS_DIR.

_ExplodingRunningProcess is new because _ExplodingProcess could not model the
case that matters: it inherits the plain fake's wait(), which reports an exit
code the instant it is awaited, so the read loop never died with the CLI still
alive. The teardown was extracted to a named method rather than inlined —
communicate() was one statement over ruff's PLR0915 cap — which also gives the
rationale a better home than a wall of comment inside a finally.

_build_env gains a docstring recording why it may hardcode "PATH" where
CodexAgent may not: it seeds from os.environ, whose keys CPython upper-cases on
Windows, instead of handing the SDK a partial dict merged over the real
environment.

make verify green: 4,171 tests, coverage gate met (91.76% on the module).
…ax_turns

Three measurement-integrity items from the review, all of which could change a
task's score or final_status for identical agent output.

Zero-telemetry guard keys on the TELEMETRY, not the event vocabulary.
`recognized_events == 0` left the identical outcome reachable one layer down: a
`step_finish` carrying no `tokens` key (a provider or auth mode that omits usage)
recognizes three events, books an all-zero TokenUsage, and EventCollector maps
that to `token_usage=None` — a COMPLETED turn with no tokens, no cost and no
warning, which a file-based criterion can still score SUCCESS, which is absent
from every token aggregate, and whose run_limits.max_total_tokens / max_usd gates
could never trip no matter what the run really billed. The second arm keys on a
step the CLI reported as FINISHED — its own claim that a generation completed —
rather than on `usage.is_empty()` alone, which would also condemn a stream cut
before any step could finish. Intentional cuts stay exempt.

Tool ARGUMENT keys are now normalized alongside tool names. _TOOL_NAME_MAP did
half the job: `command_executed` serializes `parameters` to JSON for every tool
but Bash, so `{tool_name: Read, command_pattern: 'file_path.*app\.py'}` matched
on Claude and scored 0 on OpenCode for identical behaviour. _OPENCODE_ARG_RENAME
mirrors antigravity's _ANTIGRAVITY_ARG_RENAME and is applied at the one seam
where `parameters=` is built.

The review proposed mapping `filePath` -> `file_path`, from the PR's own live
capture. Reading the tool schemas the installed CLI actually registers shows it
now uses `path` for read/write/edit (`{path, oldString, newString, replaceAll}`),
so that map alone would have renamed nothing on a current build. Both spellings
are accepted; the search tools' `path` and Bash's `command` already match
Claude's names and are left alone, which is why the map is keyed per canonical
tool rather than applied globally.

With `Skill: {name -> skill}` at the agent boundary, skill_triggered reverts to
the agent-agnostic `parameters.get("skill")`. Carrying a per-harness alternative
in a criterion that must know nothing about harnesses would make every future
harness edit it, and `parameters` is substring-scanned two lines below, so
widening the key set there was the riskier of the two places.

on_tool_use also stopped freezing the first event's view of a call. The CLI may
emit pending/running before completed for one callID, and the first event
routinely carries no `input` — so `parameters` stayed `{}` permanently and every
command_executed row scored 0 while the run looked normal. Later evidence now
wins; absent evidence never clears what is already held.

max_turns is pinned in both directions. The sole test asserted only that
`max_turns=1` sets the flag, which a `>` -> `>=` mutation survives while turning
a normal 2-step run under `max_turns: 2` into FinalStatus.MAX_TURNS_EXHAUSTED —
and a spurious exhaustion also suppresses the non-zero-exit and zero-telemetry
crash guards, so such a run would score silently instead of failing loudly. Added
the cap-not-reached case, the uncapped case, and a deciding-step-kept-whole case
asserting step 1's exact token buckets survive the cut.

Every fix was mutation-checked rather than trusted green: `>` -> `>=` and
counting finished instead of started steps (both now caught, the first
previously survived the whole suite); reverting the guard to event-vocabulary
only; disabling the arg rename; and applying it tool-agnostically.

docs/agents/OPENCODE.md documents the argument-key table (with the CLI version
drift), the widened guard's two shapes, and the matching troubleshooting entry.
No evalboard mirror is needed: pickArgText already falls through file_path ->
filePath -> path -> skill, so canonical keys hit its preferred entry earlier and
existing run artifacts still render.

make verify green: 4,189 tests, coverage gate met.
Seven medium findings; the docker one takes the documentation route.

Factory contract (A2). __init__ declared `**_: Any`, which swallowed the
always-passed `route=` undeclared. create_agent calls
`agent_class(config, route=route, **kwargs)` through a `cast(Any, ...)`, so
pyright checks nothing at the call site — with a sink on this side, nothing
checked it at runtime either, and the TypeError the orchestrator deliberately
relies on (it gates cost_log_tags on supports_cost_log_tags precisely so an
ungated forward crashes loudly) was silently absorbed instead. Every parameter
is now declared; `route` is kept and documented as unused, since the CLI owns
its own provider configuration.

Types (A2). `_plugin_skill_dirs(plugins=...)` was annotated
`list[dict[str, Any]] | None` while the config supplies
`list[LocalPluginConfig] | None`, and the mismatch was papered over with a
`# type: ignore[arg-type]` — which pyright treats as BLANKET, suppressing every
diagnostic on its only call site. Widened to `Sequence[Mapping[str, Any]] | None`
and the suppression removed: 0 errors, no ignore.

Turn events (A6). finalize() emitted no TurnEndEvent for a step left open by a
crash, timeout, cancel, or either clean cut, so the last TurnStartEvent was never
closed — a task.log with `>>> Turn start` and no matching `--- Turn end`, and a
violation of Agent.communicate's one-pair-per-inner-turn contract that all three
siblings honor. Unlike them, completed steps here already close themselves in
on_step_finish, so a `step_open` flag makes finalize fire for the straggler only.
Nothing in the suite asserted this balance before; six tests now do.

Token casts (A6). on_step_finish's five bare int() casts were the module's only
unguarded field reads, contradicting _handle_line's advertised "Never raises on
bad input" — and every neighbouring field already warns-and-continues on drift.
Raising here is expensive: AGENT_CRASH retries twice, so ONE mistyped bucket
burned three attempts and landed the task as ERROR. `_as_int` warns once and
counts 0 instead, keeps numeric strings and floats, and rejects bool (int(True)
would book a phantom token).

Dispatch (A1). _handle_line inlined the error branch while its four siblings
delegated; moved to `_OpenCodeTurnState.on_error`, making the dispatch uniform
and the payload-shape handling directly testable (7 shapes pinned).

Skill-injection coverage (A3). Six previously-untested branches: list-form
manifest (incl. non-string entries), all four unusable-manifest fallbacks, the
non-local plugin entry, the missing-SKILL.md warning, and the malformed inherited
OPENCODE_CONFIG_CONTENT paths. Two review assumptions did not survive contact:
the missing-SKILL.md case still INJECTS (the CLI scans recursively; the warning
is advisory), and the non-local branch is unreachable through the typed config
(LocalPluginConfig pins `type: Literal["local"]`), so it is driven against
_plugin_skill_dirs directly and the test says why.

Docker (A7). Documented rather than implemented, as agreed: docs/agents/OPENCODE.md
gains a "Running in Docker" section stating the driver is unsupported and naming
both gaps — the CLI is absent from the image (adding it needs a pinned version
that travels with the release tag, as CLAUDE_CODE_VERSION does) and no OpenCode
credentials are in SandboxConfig.env_passthrough, so even a custom image would
authenticate against nothing — plus the build-your-own workaround, a Known-
limitations bullet, and a correction to the Dockerfile's own comment, which
claimed "all built-in agents ship in every build" and this agent made false.

Agent roster (A7). Ten hand-written "Claude Code, Codex, and Gemini" sentences
across README.md, docs/index.md, docs/llms.txt and docs/USER_GUIDE.md's `--type`
table still omitted OpenCode; `make docs-indexes` cannot repair them (all sit
outside the generated markers) and was re-run to confirm no drift. README's
`coder-eval[codex,antigravity]` install line is deliberately left alone — the
`[opencode]` extra is empty by design, so listing it would imply pip installs a
Node CLI.

Mutation-checked, not trusted green: finalize never closing the open step,
accepting bool as a token count, and casting via str() (which caught a genuine
gap — no test fed a float, though _as_int admits one) all now fail.

make verify green: 4,223 tests, coverage gate met (91.73%).
…zero-token guard

The zero-token guard makes a turn that finished steps without booking any tokens
a hard crash. That is right by default — such a turn is absent from every token
aggregate and its max_total_tokens / max_usd gates can never trip — but it has no
override, so a provider or auth mode that genuinely reports no usage would fail
EVERY turn and make the harness unusable rather than merely imprecise. (The docs
already note OpenCode reports `cost: 0` under subscription-style auth, so a usage-
omitting mode is not hypothetical.)

`agent.require_token_telemetry: false` downgrades that arm to a warn-and-score.
Reachable from YAML, an experiment variant, or `-D agent.require_token_telemetry=false`
like any other agent field, with the resolver's did-you-mean on a typo.

Deliberately scoped to the missing-token arm only: a stream with NO recognized
events still fails even with the hatch open. That arm is event-vocabulary drift,
which has silently zeroed a whole run once already, and no provider quirk explains
a renamed vocabulary — so one flag must not reopen both holes.

Implementation is one field plus one branch at the single existing guard site; the
message is unchanged and merely hoisted to a variable so both paths share it.

make verify green: 4,225 tests, coverage gate met.
…by criteria

OpenCode exposes a provider-specific tool set, so the tool vocabulary varies by
MODEL within this one harness — not just across harnesses, which is what
_TOOL_NAME_MAP was built for. A live 174-task run makes it concrete:

  DeepSeek V4 Pro : write=144  edit=55  apply_patch=0
  GPT-5.6 Luna    : write=0    edit=0   apply_patch=120

`apply_patch` was unmapped, so it reached CommandTelemetry under its raw name.
Every `command_executed` criterion keyed on `tool_name: Write` or `tool_name:
Edit` therefore scores 0 on a GPT-family model that edited the file correctly —
and that suite carries 33 Write and 30 Edit such criteria.

Mapped to `Write`, matching codex_agent's `_TOOL_ITEM_NAMES["apply_patch"]`, so
one criterion reads the same on either harness.

Measured blast radius on the run that surfaced it: zero. All 76 Luna tasks that
used apply_patch had their failures elsewhere (llm_judge 2, run_command 4) and
not one command_executed criterion among them — so the published comparison was
not distorted. This is a latent scoring gap being closed, not a correction to
those numbers.

Its argument is a patch envelope (`patchText`), not a file path, so no arg
rename applies and a `command_pattern` written against `file_path` still will
not match it. Documented in OPENCODE.md alongside the guidance to assert on
`tool_name` alone or on the resulting file.

Mutation-checked: removing the entry fails the new test.

make verify green: 4,226 tests, coverage gate met.
@mohsen-uipath

mohsen-uipath commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Every required finding is addressed, and almost all of the non-required ones, mapped to commits below. Head is now 82c0695.

Blockers — both closed (7b5cf67)

1. _build_env mock-CLI PATH contract untested. Added TestSandboxEnvironment mirroring the three sibling agents: order-preserving prepend ahead of the parent PATH, PLUGIN_TOOLS_DIR exported, an inherited value winning over the sandbox one, and neither key touched when start() gets no kwargs — all asserting on captured["kwargs"]["env"].

2. CLI never reaped in communicate()'s finally. Added _reap_orphaned_cli(proc), called before the handle is cleared and guarded on proc is None or proc.returncode is not None. It is deliberately synchronous: it runs while a CancelledError is propagating, where any await can itself be cut short and leave the child alive. Four tests pin it — the read-loop crash kills the CLI, an external cancel kills it, a clean turn kills nothing (killpg == []), and the spawn-failure path does not raise over the unbound proc.

Non-blocking

# Finding Where
1 OpenCode key hardcoded in the agnostic skill_triggered 839e818 — criterion reverted to parameters.get("skill"); renamed at the agent seam instead
3 Blanket # type: ignore[arg-type] 839e818 / ad0a559 — widened _plugin_skill_dirs' parameter type; the module now has zero type: ignore
4 __init__ broke the SPI factory contract ad0a559 — now (config, route=None, *, task_id="unknown"), with tests that an undeclared kwarg raises instead of vanishing
5 max_turns tested one direction only 839e818 — the >/>= mutation is now killed: max_turns=2 on the 2-step stream stays False, =1 cuts at the start of step 2 and keeps step 1 whole
6 Two-event tool lifecycle untested 839e818 — added the else branch refreshing parameters/execution_started_at ("later evidence wins; absent evidence never clears"), plus three tests including the inverse case
7 Five untested skill-injection branches 839e818 / ad0a559TestSkillInjection covers list-form manifest, unusable manifest, non-local entry, missing SKILL.md, and the inherited OPENCODE_CONFIG_CONTENT merge
8 finalize() emits no TurnEndEvent ad0a559 — added a step_open flag and the emission; the suite now carries TurnStartEvent/TurnEndEvent assertions
9 Five unguarded int() casts ad0a559 — added _as_int(), rejecting bool and accepting numeric str/float
10 opencode unusable under the docker driver ad0a559 — docs route: a "Running in Docker" section plus a Known-limitations bullet, and the Dockerfile comment that wrongly claimed every built-in ships in the image is corrected
11 Stale agent roster ad0a559 — updated across README.md, docs/index.md, docs/llms.txt and docs/USER_GUIDE.md, including the --type flag reference
12 Tool ARG keys un-normalized 839e818 — added _OPENCODE_ARG_RENAME mirroring _ANTIGRAVITY_ARG_RENAME
13 Zero-telemetry guard keys on event names 839e818 — now keys on captured telemetry (steps_finished > 0 and usage.is_empty()), with intentional cuts exempt; 5d492ab adds require_token_telemetry as an escape hatch for a provider that reports no usage at all

Two review premises turned out stale

  • The review notes OpenCode emits filePath. Current builds register path (verified against the binary), so _OPENCODE_ARG_RENAME accepts both.
  • Two of the five "untested branches" are unreachable through the typed config — LocalPluginConfig pins type: Literal["local"]. Those are driven against _plugin_skill_dirs directly instead.

One gap found outside the review

apply_patch — the GPT-family edit tool — was missing from _TOOL_NAME_MAP, so GPT-family edits were invisible to criteria. e0487de adds "apply_patch": "Write", mirroring codex_agent._TOOL_ITEM_NAMES.

Finding 2 — required part done, optional part open

The one change the finding asked for is in: the _ERROR branch moved to _OpenCodeTurnState.on_error(part), so the dispatch is uniform and _handle_line is D22 to C16, under the 20 you asked for.

Current state of the five functions in scope:

Function At review Now
_handle_line D 22 C 16
on_tool_use C 18 C 20
_settle_turn C 18 D 24
on_step_finish C 17 C 17
_plugin_skill_dirs C 14 C 14

Two of them grew as a direct side effect of other fixes in this round — on_tool_use absorbed finding 6's second-event branch, and _settle_turn absorbed finding 13's second guard arm. So the cluster is not smaller overall, and _settle_turn in particular is worse than when you flagged it.

Your optional suggestion is still open: _settle_turn still takes both deadline and timeout. That one is cheap — timeout survives only to give _timeout_turn a value for its error message, one test touches _settle_turn and none passes timeout=. Taking _settle_turn back under D would mean lifting the 44-line zero-telemetry guard into its own method, the same extraction you already accepted for _handle_line.

Happy to do both here if you would like them in this PR. My mild preference is to land them with the ruff C90 ratchet on main rather than grow a PR that is already +3957/-40, but it is a small change either way and I do not feel strongly.

Verification at 82c0695

ruff clean · pyright 0 errors · custom lint 344 passed · 4296 passed, 5 skipped · all CI checks green.

opencode_agent.py specifically: 131 tests (65 at review time) and 95.08% coverage. Both lines called out as coverage-missing are now covered.

Results

I repeated the experiment from the PR body against this head — same suite and harness, GPT-5.6 Luna on Bedrock against DeepSeek V4 Pro on Azure AI Foundry — on 36 tasks instead of 174. The results are very close to the published run:

GPT-5.6 Luna DeepSeek V4 Pro
174 tasks (PR body) 85.6% 80.5%
36 tasks (this head) 86.1% 80.6%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants