Repository navigation
feat(#7824)!: add OpenCode sandbox hook adapter - #8852
yvonnedevlinrh wants to merge 14 commits into
Conversation
Install Go-embedded native hooks at bootstrap using the pi deployment pattern. Enforce Bash and tool policies, sanitize successful and failed tool output, and keep hook configuration and digests on the runner. Pin utilities for standalone bootstrap and agent-restore hook guards. Add native OpenCode 1.19.1 regression coverage and scoped image smoke checks. Document the remaining same-user verification race. BREAKING CHANGE: OpenCode requires security.enabled: true, OpenCode 1.19.1 and the pinned plugin SDK. Remove the security-disabled workaround and additional OPENCODE_CONFIG_CONTENT.plugin entries. Assisted-by: gpt-5.6-terra Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
E2E tests did not runE2E tests run automatically for pull requests authored by users with write access or higher. For other contributors, someone with write access must add the Reason: See E2E testing guide for details. |
PR Summary by QodoAdd OpenCode 1.19.1 sandbox security-hook adapter
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Preserve the native hook adapter while adopting the shared hook environment and trusted interpreter APIs from main. Keep the data-only .env loader, trusted binary launch, and both hook integrity checks. Verify the pinned hook PATH during bootstrap and retain override-reset ordering checks. Update the runtime documentation for data-only loading. Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Site previewPreview: https://3da24be3-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review notes on head 2347294: one failing required check and two smaller points, all inline.
Honor nonzero exit_code on structured OpenCode errors so context suppression cannot replace failure details with a passing summary. Cover failure passthrough and successful suppression. Addresses PR fullsend-ai#8852 review feedback from @qodo-code-review[bot]. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Permit single-quoted Bash literals without hiding executable syntax. Cache successful error sanitization by identity and complete payload, and distinguish missing LS targets from hook integrity failures. Addresses PR fullsend-ai#8852 review feedback from @qodo-code-review[bot]. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Link the term directly from the runtime-selection table. Addresses PR fullsend-ai#8852 review feedback from @qodo-code-review[bot]. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Commit the SDK manifest and transitive integrity hashes, install with npm ci, and reject SDK pins that differ from the OpenCode binary pin. Document lockfile regeneration when updating the runtime. Addresses PR fullsend-ai#8852 review feedback from @qodo-code-review[bot]. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Publish base platform digests and a run-scoped queue index. Pass its immutable digest to both code-image builds and smoke tests without updating normal release tags. Fail closed if that digest is missing. Addresses PR fullsend-ai#8852 review feedback from @qodo-code-review[bot]. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review notes on head 624928d: one critical and three medium findings, all inline. The critical one: OPENCODE_PURE set from .env stops the hook plugin from loading, and the digest guards still pass.
Use printf so shell escape handling cannot alter recorded guard commands. Addresses PR fullsend-ai#8852 review feedback from @waynesun09. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Keep the hook plugin enabled after hostile environment loads. Normalize Bash exit metadata, patch targets, and path-scoped permission inputs. Cover the native failure and permission paths with the pinned runtime. Addresses PR fullsend-ai#8852 review feedback from @waynesun09. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Explain the stronger Bash default and the retained pi-style skills layout. Reference agents#1821 for the scribe tool-declaration follow-up. Addresses PR fullsend-ai#8852 review feedback from @waynesun09. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Keep local PR feedback and triage records out of commits. Addresses PR fullsend-ai#8852 feedback workflow requirements. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review notes on head 85d7e75: one medium finding, inline.
Run the existing pre-block probe in each candidate sandbox image as the sandbox user. Include runtime and security changes in the image workflow triggers. Addresses PR fullsend-ai#8852 review feedback from @waynesun09. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Preserve the native OpenCode hook description alongside upstream's human_min_role overlay guidance. Assisted-by: gpt-6.1-sol Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review notes on head 3171a19: three medium findings, all inline.
| // Scan every string field, not just stdout: native metadata, titles | ||
| // and attachments can otherwise carry an alternate copy of a secret. | ||
| const original = { ...output }; | ||
| if (input.tool === "bash" && Number.isInteger(output.metadata?.exit)) { |
There was a problem hiding this comment.
MEDIUM: Bash timeout or abort (metadata.exit null) still looks like success to context suppression
The fix in dcd6839 sets exit_code only when Number.isInteger(output.metadata?.exit). In OpenCode v1.19.1 packages/opencode/src/tool/shell.ts:542-557, the race between exitCode, abort and timeout returns code: null for the abort and timeout cases. The tool still returns normally with metadata: { exit: null }, and the termination notice is appended only as a <shell_metadata> block in the output text (lines 561-584). No exit_code reaches the hooks.
hook_io.looks_failed (hook_io.py:147-152) checks only an Exit code prefix, interrupted, or an integer non-zero exit_code, so it reports success. suppress_scan_secrets matches by substring ("no secrets" in lower), and the go test and pytest matchers look for positive evidence in the same way. A verification command that prints partial success text and is then killed by timeout or user abort is rewritten to ...: passed, and the timeout notice is dropped. This is a different case from the integer non-zero exit thread on hook_io.py:151, which is resolved.
Suggestion: in tool.execute.after, when input.tool === "bash" and metadata.exit is not an integer, mark the payload as failed, for example with original.interrupted = true or a non-zero sentinel exit_code. Add a regression test: bash output containing "No secrets found" followed by the shell_metadata timeout notice, with exit: null, must not be suppressed.
| function checkPermission(cfg, tool, args, cwd, worktree, targets) { | ||
| const id = ["write", "apply_patch", "multiedit"].includes(tool) ? "edit" : tool; | ||
| const rule = cfg.permission[id] ?? cfg.permission["*"]; | ||
| const match = (pattern, text) => new RegExp("^" + pattern.replace(/[.+^${}()|[\]\\]/g, "\\$&").replace(/\*/g, ".*").replace(/\?/g, ".") + "$", "s").test(text); |
There was a problem hiding this comment.
MEDIUM: the plugin's trusted-permission ceiling uses different wildcard rules and match values than OpenCode for bash, skill, glob/grep and websearch
The earlier thread on line 91 was fixed only for path tools (read/edit/write/apply_patch). Other tools still differ from native OpenCode 1.19.1:
- Native
Wildcard.match(packages/opencode/src/util/wildcard.ts) turns a trailing*into( .*)?, so"git *"matches a baregit. The localmatchhere does not, so it denies that command. - Native bash checks each parsed command node separately (shell.ts
collect, which skips cwd commands such ascd). The plugin matches the whole command string, so a trusted map like{"*":"deny","git *":"allow"}deniescd repo && git status, which native OpenCode allows. - For skill, websearch and glob/grep, the plugin tests
args.pathor the literal"*"instead of native's pattern, name or query, so a trusted skill or websearch allow-list by name denies every call. - Rule selection uses
cfg.permission[id] ?? cfg.permission["*"]instead of native's ordered evaluation.
Native checks still run, so this over-denies and is not a bypass. But it breaks valid trusted pattern maps that the docs say the plugin keeps as a ceiling.
Suggestion: either port OpenCode's Wildcard.match (including the trailing * rule) and per-tool value selection, with table tests against native results, or limit the plugin-side ceiling to the path tools that are already verified and document that native evaluation is authoritative for bash, skill, glob, grep and websearch.
| try { | ||
| const tool = validate(input, input.args); | ||
| const sanitized = chain("PostToolUse", tool, input.args, original); | ||
| output.output = sanitized.output; |
There was a problem hiding this comment.
MEDIUM: MCP tool results bypass sanitization. Only output.output is rewritten, but OpenCode builds the model text from result.content[]
In OpenCode v1.19.1 packages/opencode/src/session/tools.ts:419-458, MCP tools trigger tool.execute.after with the raw CallToolResult. After the hook, OpenCode builds the model-visible text and attachments from result.content[] (text items, resource.text, image and blob data). This hook writes back only output.output = sanitized.output and, on rewrite, clears metadata and attachments. It never touches content, so the redaction, canary and unicode rewrites the chain makes on content[].text are thrown away.
For an unrestricted agent (openCodePermissionRecord(nil) returns an empty record and cfg.tools is null), MCP tools are allowed. validateOpenCodeTrustedEnv (opencode_integrity.go:89-114) rejects a non-empty plugin but accepts an mcp block in OPENCODE_CONFIG_CONTENT. Any MCP server configured there therefore returns results to the model without the shared post-tool sanitization.
Suggestion: fail closed on result shapes the adapter does not handle: deny when the after payload has a content array or no string output, or when the tool is not a known built-in. Also reject a non-empty mcp key in validateOpenCodeTrustedEnv next to plugin. Add a test for each.
Signed-off-by: Yvonne Devlin <ydevlin@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review notes on head f866ac4: one medium finding, inline.
| // call. The native hook adapter additionally checks file-tool write paths. | ||
| func (r OpenCodeRuntime) EnvExports() []string { | ||
| return []string{ | ||
| "unset " + strings.Join(openCodeDangerousEnvVars, " "), |
There was a problem hiding this comment.
MEDIUM: EnvExports() writes a non-assignment unset ... line into .env, so every OpenCode .env read logs an "ignored .env line" warning
This PR adds "unset " + strings.Join(openCodeDangerousEnvVars, " ") as the first entry of OpenCodeRuntime.EnvExports(). bootstrapEnv (internal/cli/run.go:3802) appends rt.EnvExports() line by line into /sandbox/workspace/.env. That file is now read as data by envDataLoad. In env_data.go, statement() matches unset as a NAME. The next character is not = and the line has no export prefix, so it raises "not a NAME=value assignment". read_file then calls warn() and prints fullsend: ignored '/sandbox/workspace/.env' line N: not a NAME=value assignment.
This happens on every OpenCode launch and on every probe that loads .env. That warning channel is how operators see tampered or malformed .env lines, so a warning on every run makes the real ones easy to miss.
The unset has no effect in .env. It is also redundant at launch: opencode_run.go joins EnvExports() into the prelude after the .env load, and openCodeTrustedEnvRestore() already unsets the same openCodeDangerousEnvVars. No other runtime's EnvExports returns a non-assignment line, and main's opencode.go had none, so this is new in the diff.
Suggestion: make EnvExports() return only export or assignment lines. Keep the unset in the launch-time shell fragment (openCodeTrustedEnvRestore already covers it) and list the names in openCodeEnvDataDrop. Add a test that runs EnvExports() through envDataLoad and checks that no "ignored" warning is printed.
Summary
Adds the sandbox security adapter for OpenCode 1.19.1, enabling the shared hook controls for write-capable agents.
security.HookPlanthrough native before/after tool hooks.OPENCODE_PUREvalues from disabling the hook plugin.Deployment decision
The adapter follows pi's existing bootstrap deployment pattern. The runner uploads the Go-embedded adapter and selected shared scripts into
/sandbox/opencode-config, retains their expected digests, and verifies them before loading.This keeps the deployment consistent with the existing runtime architecture. Hash checks detect modifications, but the same-user verification-to-load race remains explicitly unresolved. Stronger OS-backed storage is left for separate cross-runtime review.
Config-directory skills remain agent-writable, persist across iterations, and are not covered by the adapter's integrity checks. This behavior predates the adapter and is consistent with pi's existing deployment model. The trust model for inherited skill changes is tracked separately in #8899.
Breaking changes
security.enabled: false. Existing harnesses using that workaround must enable security.OPENCODE_CONFIG_CONTENT.pluginentries are rejected.The SDK is installed in the image; the adapter and selected scripts are supplied during bootstrap.
Validation
runtime-opencodeE2E validation pass with the matching runner and candidate image.Raw session/transcript artifact redaction remains tracked separately in #7320.
Closes #7824.
Related: unbound-force/unbound-force#515.
Runtime implementation guide
Consulted
docs/contributing/runtime-implementation.md. This PR updates the OpenCode security matrix, shared hook contract, and sandbox workspace layout to describe the native adapter and selected shared scripts. Deployment follows pi's bootstrap-uploaded, runner-digest-checked pattern. The same-user verification-to-load race remains explicitly documented as unresolved.