refactor(runtime): register opencode as a stub runtime - #6035
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by Qodoruntime: register opencode as a stub runtime backend
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
waynesun09
left a comment
There was a problem hiding this comment.
Review-only findings (see inline comments). No approval/change-request action taken.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3e45cab to
a768a59
Compare
waynesun09
left a comment
There was a problem hiding this comment.
Review-only findings (see inline comments, plus a reply on the existing ValidRuntimes thread rather than a duplicate comment). No approval/change-request action taken.
Findings without a diff anchor:
- LOW — PR body claim "fail-fast at bootstrap, before sandbox setup" doesn't match the code path. Verified empirically on a768a59: sandbox creation (internal/cli/run.go:1022) precedes
rt.Bootstrap(run.go:1245), which is the last step of the "Bootstrapping sandbox" phase, afterscanRuntimeContent/bootstrapCommon/bootstrapEnv. The failure is fail-fast relative toRun()only. Suggest correcting the description to "fails during sandbox bootstrap, before agent execution". - LOW —
Runtime.System()interface godoc drift. internal/runtime/runtime.go:48-51 still defines the return as "the model vendor … e.g. "anthropic""; with this PR two of three implementations return runtime identifiers (fullsend.dummy,opencode). Worth amending the godoc ("the model vendor where the runtime is vendor-specific, or the runtime identifier for multi-provider runtimes") here or in the follow-up. - LOW — validation behavior unpinned by tests.
TestOrgConfigValidateRuntime(internal/config/config_test.go:389) never exercisesopencode, so the "registered but unusable validates" behavior — whichever way theValidRuntimes()decision lands — has no explicit test to catch a silent revert.
Overall: solid scaffolding PR — registration, interface conformance, fail-fast gate, and an honest security-matrix row. The substantive open question is selectability-before-implementability (see the ValidRuntimes thread); the ConfigDir() provisional marker is the one other change worth making before merge so follow-ups don't inherit an unverified path as settled.
Site previewPreview: https://5be208d2-site.fullsend-ai.workers.dev Commit: |
Register "opencode" in ValidRuntimes() and Resolve() with a stub OpenCodeRuntime that satisfies both Runtime and TranscriptHandler interfaces. All methods are no-ops or return not-implemented errors. This is the first step toward OpenCode support tracked by fullsend-ai#1260. Subsequent PRs will add stream parsing, bootstrap, run execution, and transcript extraction. Closes: n/a Refs: fullsend-ai#1260, fullsend-ai#1935 Signed-off-by: sonupreetam <spreetam@redhat.com>
- Fail fast in Bootstrap() instead of only in Run(), avoiding unnecessary sandbox setup work before the not-implemented error - Soften System() comment: remove unverified InitEvent reference, note event schema is TBD pending fullsend-ai#1935 - Fill in security feature matrix in docs/runtimes.md with OpenCode column (all N/A — stub; does not implement ClaudeHooksBootstrap) - Add dedicated TestOpenCodeRuntimeBootstrap_NotImplemented test - Remove Bootstrap from TestOpenCodeRuntimeNoopMethods (no longer a no-op) Signed-off-by: sonupreetam <spreetam@redhat.com>
- Remove opencode from ValidRuntimes() — keep only in Resolve() so users cannot select it via fullsend github/admin install until the runtime is functional. Re-add when Run() is implemented. - Mark ConfigDir() as provisional with comment noting the agent-writable workspace security concern (fullsend-ai#1260). - Change ExtractTranscripts/ExtractDebugLog from no-op (nil) to not-implemented errors referencing fullsend-ai#1935, preventing silent success claims in CI logs. - Update runtimes.md table to clarify opencode is resolved by Resolve() but not in ValidRuntimes() until implemented. - Add TestOpenCodeRuntimeExtractStubs_NotImplemented test. Signed-off-by: sonupreetam <spreetam@redhat.com>
- Test that config validation rejects 'opencode' in both org and per-repo configs (not in ValidRuntimes until implemented) - Test ResolveFromPerRepoConfig with opencode (hand-written config bypassing validation can reach the stub) - Verify Transcripts type assertion in TestResolve (ensures Resolve() returns OpenCodeRuntime for both Runtime and Transcripts) Signed-off-by: sonupreetam <spreetam@redhat.com>
f21eb27 to
202e0a8
Compare
waynesun09
left a comment
There was a problem hiding this comment.
All review threads from both rounds are addressed and verified against the code at 202e0a8: opencode removed from ValidRuntimes() (locked in by config validation tests) while staying resolvable via Resolve(), Bootstrap()/extract stubs fail fast with explicit not-implemented errors referencing #1935, docs matrix and provisional ConfigDir() marker in place. Built the branch and ran internal/runtime + internal/config suites locally — all PR-related tests pass.
|
@waynesun09 Thank you for the reviews. The merge queue ejection was a transient GitHub API timeout during e2e scaffold setup, |
|
🤖 Finished Retro · ✅ Success · Started 3:08 PM UTC · Completed 3:23 PM UTC Commit: |
Retro: PR #6035 — register opencode as a stub runtimeWorkflow shape: Human-authored PR by an external contributor ( Actionable finding: Issue #1935 prematurely closedIssue #1935 ("TranscriptHandler assumes file-based JSONL traces — needs format-neutral contract before second runtime") was manually closed by
#1935 tracks a hard prerequisite — the format-neutral TranscriptHandler contract needed before the opencode runtime can be fully implemented. It should likely be reopened. Existing issues corroborated by this PR
Review quality assessmentNo agent review ran, so no agent-vs-human comparison is possible. The third-party Proposals filed
|
Add PiRuntime as a stub implementation of the Runtime and
TranscriptHandler interfaces for the pi agent runtime
(earendil-works/pi), following the same pattern as
OpenCodeRuntime. The runtime is resolvable via
runtime.Resolve("pi") but intentionally excluded from
ValidRuntimes() until Bootstrap/Run are functional (per
the #6035 precedent).
Key additions:
- PiRuntime stub (pi.go): implements Runtime and
TranscriptHandler with not-implemented errors, mirroring
OpenCodeRuntime.
- parsePiStream (pi_progress.go): maps pi's --mode json
NDJSON event stream to AgentEvent values. Handles session
header (InitEvent), text, thinking, tool_result,
message_end (TokensEvent), agent_end (ResultEvent), and
error events. Detects stop_reason=error/aborted for the
exit-0-override since --mode json exits 0 on model error.
Falls back to synthesized ResultEvent on truncated streams.
- Test fixtures (testdata/pi/): recorded from pi 0.84.2
--mode json output covering basic run, error run,
reasoning, multi-step, malformed input, empty input, and
truncated stream scenarios.
- Registry wiring: "pi" case in Resolve() switch.
- Documentation: pi column in docs/runtimes.md security
feature matrix, config key support table, registered
runtimes row, and pi-specific known constraints section.
Note: pre-commit could not run (sandbox network policy
blocked git fetch during hook environment init).
golangci-lint not available in sandbox. go vet passes.
Related to #6464
Add PiRuntime as a stub implementation of the Runtime and
TranscriptHandler interfaces for the pi agent runtime
(earendil-works/pi), following the same pattern as
OpenCodeRuntime. The runtime is resolvable via
runtime.Resolve("pi") but intentionally excluded from
ValidRuntimes() until Bootstrap/Run are functional (per
the #6035 precedent).
Key additions:
- PiRuntime stub (pi.go): implements Runtime and
TranscriptHandler with not-implemented errors, mirroring
OpenCodeRuntime.
- parsePiStream (pi_progress.go): maps pi's --mode json
NDJSON event stream to AgentEvent values. Handles session
header (InitEvent), text, thinking, tool_result,
message_end (TokensEvent), agent_end (ResultEvent), and
error events. Detects stop_reason=error/aborted for the
exit-0-override since --mode json exits 0 on model error.
Falls back to synthesized ResultEvent on truncated streams.
- Test fixtures (testdata/pi/): recorded from pi 0.84.2
--mode json output covering basic run, error run,
reasoning, multi-step, malformed input, empty input, and
truncated stream scenarios.
- Registry wiring: "pi" case in Resolve() switch.
- Documentation: pi column in docs/runtimes.md security
feature matrix, config key support table, registered
runtimes row, and pi-specific known constraints section.
Note: pre-commit could not run (sandbox network policy
blocked git fetch during hook environment init).
golangci-lint not available in sandbox. go vet passes.
Related to #6464
Add PiRuntime as a stub implementation of the Runtime and
TranscriptHandler interfaces for the pi agent runtime
(earendil-works/pi), following the same pattern as
OpenCodeRuntime. The runtime is resolvable via
runtime.Resolve("pi") but intentionally excluded from
ValidRuntimes() until Bootstrap/Run are functional (per
the #6035 precedent).
Key additions:
- PiRuntime stub (pi.go): implements Runtime and
TranscriptHandler with not-implemented errors, mirroring
OpenCodeRuntime.
- parsePiStream (pi_progress.go): maps pi's --mode json
NDJSON event stream to AgentEvent values. Handles session
header (InitEvent), text, thinking, tool_result,
message_end (TokensEvent), agent_end (ResultEvent), and
error events. Detects stop_reason=error/aborted for the
exit-0-override since --mode json exits 0 on model error.
Falls back to synthesized ResultEvent on truncated streams.
- Test fixtures (testdata/pi/): recorded from pi 0.84.2
--mode json output covering basic run, error run,
reasoning, multi-step, malformed input, empty input, and
truncated stream scenarios.
- Registry wiring: "pi" case in Resolve() switch.
- Documentation: pi column in docs/runtimes.md security
feature matrix, config key support table, registered
runtimes row, and pi-specific known constraints section.
Note: pre-commit could not run (sandbox network policy
blocked git fetch during hook environment init).
golangci-lint not available in sandbox. go vet passes.
Related to #6464
Register PiRuntime (earendil-works/pi, CLI `pi`) in runtime.Resolve() following the OpenCode stub precedent (fullsend-ai#6035): resolvable internally for dev/testing, deliberately NOT added to config.ValidRuntimes() — a selectable stub would burn pre-script side effects and a sandbox before failing — so it is not user-selectable via `fullsend admin install --runtime` until the runtime is functional. - internal/runtime/pi.go: Name "pi", System "pi" (multi-provider, OpenCode precedent), ConfigDir /sandbox/pi-config (new sandbox.SandboxPiConfig, outside the agent-writable workspace), EnvExports pinning PI_CODING_AGENT_DIR / PI_CODING_AGENT_SESSION_DIR to runner-owned paths plus PI_OFFLINE=1 and PI_SKIP_VERSION_CHECK=1; Bootstrap/Run and transcript extraction return explicit not-implemented errors (fullsend-ai#6464). - images/sandbox/Containerfile: ARG PI_VERSION=0.84.2, npm install --ignore-scripts, with a renovate customManagers regex tracking the pin; the install comment records that pin bumps must re-verify the upcoming stream-parser fixtures (pi changed its --mode json shape within 0.84). - docs/runtimes.md: registered-runtimes row, pi (stub) column in the security feature matrix and the config-key support matrix. - Tests: pi_test.go (metadata, env exports, not-implemented, no-ops, capability defaults — no CLAUDE.md bridge, default debug-log name); registry and config tests extended with the resolvable-but-not-selectable cases. Next steps tracked in fullsend-ai#6464: stream parser with recorded fixtures, Bootstrap/Run with the ADR 0090 hook-adapter extension, transcript extraction, then ValidRuntimes() enable. Refs fullsend-ai#6464 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Summary
Register "opencode" as an agent runtime backend via
runtime.Resolve()with a stubOpenCodeRuntimethat satisfies bothRuntimeandTranscriptHandlerinterfaces. The stub is not added toconfig.ValidRuntimes()— it is resolvable internally for dev/testing but not user-selectable viafullsend github --runtimeorfullsend admin install --runtimeuntil the runtime is functional.Note: This stub does not resolve #1935. The format-neutral TranscriptHandler contract (#1935) remains a hard prerequisite before
Run()and TranscriptHandler methods can be implemented for real. Extract methods return explicit not-implemented errors referencing #1935 to prevent silent success claims.Related Issue
Refs #1260
Refs #1935
Changes
case "opencode"toruntime.Resolve()returning a Backend withOpenCodeRuntimeinternal/runtime/opencode.go— stub struct with:Bootstrap()andRun()returningfmt.Errorf("opencode runtime is not yet implemented")(fails during sandbox bootstrap, before agent execution)ExtractTranscripts()/ExtractDebugLog()returning not-implemented errors referencing TranscriptHandler assumes file-based JSONL traces — needs format-neutral contract before second runtime #1935 (prevents silent success in CI logs)ConfigDir()marked provisional with security note about agent-writable workspace (Track opencode as a supported agent runtime #1260)RuntimeandTranscriptHandleropencodecolumn todocs/runtimes.mdsecurity feature matrix (all N/A — stub; does not implementClaudeHooksBootstrap)opencoderow todocs/runtimes.mdregistered runtimes table (noted as not inValidRuntimes()until implemented)config.ValidRuntimes()— avoids exposing a non-functional runtime on user setup paths and avoids updating ~5 doc/CLI surfaces that enumerate valid runtimesTesting
make lintpasses (stage changes first, then run)Checklist
!for breaking changes)