fix(opencode): address gadfly's dogfood review

Gadfly's own swarm reviewed PR #26 and reached consensus (3/3 models) on a real bug, plus flagged security/maintainability items. Fixes: - Pass-through auth (BLOCKING, 3/3 agreement): openCodeEnv() stripped every provider key except OLLAMA_API_KEY, so the documented opencode/<provider>/<model> escape hatch (e.g. opencode/anthropic/...) had no way to authenticate — the reusable workflow forwards ANTHROPIC_API_KEY/OPENAI_API_KEY into the container and the allowlist discarded them. Now forward ANTHROPIC_*/OPENAI_*/GOOGLE_*/ GEMINI_* so OpenCode's built-in providers can authenticate, while still withholding gadfly's own secrets (Gitea/findings tokens, claude-code OAuth). - Read-only hardening (security lens): the generated config denied only edit/bash. Using OpenCode's documented permission schema, also deny webfetch/websearch/ external_directory — the network + out-of-sandbox tools — closing the exfiltration surface a prompt-injected review could otherwise reach. Permission is now a map so the deny set is extensible. - Dedup (maintainability lens, 3/3): extract shared filterEnv() and killGroupOnCancel() helpers in engine.go, used by both shell-out engines' runPass/env builders instead of the copy-pasted blocks. - Cosmetic: split the const block so defaultOpenCodeBaseURL's doc comment no longer visually misattaches to the agent-name const. README updated: the read-only note and the reduced-env note now reflect the broader deny set and the forwarded provider keys. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-18 01:29:29 -04:00
parent 463aa01ddb
commit 2477e50230
4 changed files with 106 additions and 79 deletions
@@ -176,13 +176,16 @@ secret in config). Tuning knobs (all optional):
 | `GADFLY_OPENCODE_BIN` | `opencode` | CLI binary path |

 > **Read-only is enforced through config, not a flag.** OpenCode has no `--append-system-prompt`, so
-> Gadfly generates a per-lens config — the lens system prompt as a `gadfly` agent's prompt, with
-> `edit`/`bash` denied at both the global and agent level — and injects it via `OPENCODE_CONFIG_CONTENT`.
-> That env var is the highest-precedence config source in the container, so it **outranks any
-> `opencode.json` a reviewed repo ships** — a repo can't re-enable edits on the reviewer. The
-> subprocess runs with a **minimal environment** (`OLLAMA_API_KEY` + `PATH`/`HOME`/locale/`OPENCODE_*`/
-> `GADFLY_OPENCODE_*`), not the runner's full env; the Gitea token, Anthropic/Claude keys, and findings
-> token aren't handed to the CLI.
+> Gadfly generates a per-lens config — the lens system prompt as a `gadfly` agent's prompt, with the
+> mutating and network tools (`edit`/`bash`/`webfetch`/`websearch`/`external_directory`) denied at both
+> the global and agent level — and injects it via `OPENCODE_CONFIG_CONTENT`. That env var is the
+> highest-precedence config source in the container, so it **outranks any `opencode.json` a reviewed
+> repo ships** — a repo can't re-enable edits on the reviewer. The subprocess runs with a **reduced
+> environment**: the provider keys OpenCode needs to authenticate (`OLLAMA_API_KEY` for the primary
+> path, plus `ANTHROPIC_*`/`OPENAI_*`/`GOOGLE_*`/`GEMINI_*` for the `opencode/<provider>/<model>`
+> pass-through) alongside `PATH`/`HOME`/locale/`OPENCODE_*`/`GADFLY_OPENCODE_*` — but **not** gadfly's
+> own secrets (the Gitea token, the findings token, or the claude-code subscription token), which the
+> CLI has no use for.

 > **Newly wired, lightly tested.** Like the claude-code engine, `auto` specialist selection and the
 > `delegate_investigation` worker are majordomo-only and are skipped here (OpenCode does its own
@@ -160,16 +160,7 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i
 		// Force an extended-thinking budget for this run (a "...:max" spec).
 		cmd.Env = append(cmd.Env, "MAX_THINKING_TOKENS="+strconv.Itoa(e.thinkingTokens))
 	}
-	// Put the CLI and the Node children it spawns in their own process group and
-	// kill the WHOLE group on context cancel, so a timed-out lens can't leave
-	// orphaned claude/node processes behind in the container.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-	cmd.Cancel = func() error {
-		if cmd.Process != nil {
-			_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
-		}
-		return nil
-	}
+	killGroupOnCancel(cmd)
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
@@ -220,13 +211,39 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i
 	return "", fmt.Errorf("claude -p produced no parseable output")
 }

+// killGroupOnCancel puts cmd in its own process group and, on context cancel,
+// SIGKILLs the whole group — so a timed-out shell-out CLI (claude/opencode) can't
+// leave orphaned Node children behind in the container. Call before cmd.Run.
+func killGroupOnCancel(cmd *exec.Cmd) {
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	cmd.Cancel = func() error {
+		if cmd.Process != nil {
+			_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
+		}
+		return nil
+	}
+}
+
+// filterEnv returns the current process environment reduced to the variables for
+// which keep returns true. Shared by the shell-out engines' minimal-env builders
+// (claudeEnv, openCodeEnv), which differ only in their keep predicate.
+func filterEnv(keep func(string) bool) []string {
+	var env []string
+	for _, kv := range os.Environ() {
+		if k, _, ok := strings.Cut(kv, "="); ok && keep(k) {
+			env = append(env, kv)
+		}
+	}
+	return env
+}
+
 // claudeEnv builds a minimal environment for the `claude` subprocess: only what
 // the CLI needs (PATH/HOME, its auth tokens, locale, Node/XDG/GADFLY_CLAUDE_*
 // knobs), deliberately dropping the rest of the runner's secrets — GITEA_TOKEN,
 // GADFLY_FINDINGS_TOKEN, provider keys — so they never reach the third-party
 // CLI. Defense in depth: the parent already holds them, but the CLI has no need.
 func claudeEnv() []string {
-	keep := func(k string) bool {
+	return filterEnv(func(k string) bool {
 		switch k {
 		case "PATH", "HOME", "USER", "LOGNAME", "TMPDIR", "LANG", "TERM", "SHELL", "MAX_THINKING_TOKENS":
 			return true
@@ -237,14 +254,7 @@ func claudeEnv() []string {
 			strings.HasPrefix(k, "GADFLY_CLAUDE_") ||
 			strings.HasPrefix(k, "NODE_") ||
 			strings.HasPrefix(k, "XDG_")
-	}
-	var env []string
-	for _, kv := range os.Environ() {
-		if k, _, ok := strings.Cut(kv, "="); ok && keep(k) {
-			env = append(env, kv)
-		}
-	}
-	return env
+	})
 }

 // truncateForErr caps CLI error detail so a stderr dump can't bloat the comment,
@@ -8,7 +8,6 @@ import (
 	"os"
 	"os/exec"
 	"strings"
-	"syscall"
 )

 // openCodeEngine reviews by shelling out to the `opencode` CLI (opencode.ai) in
@@ -35,16 +34,17 @@ type openCodeEngine struct {
 	extraArgs     []string // appended verbatim (GADFLY_OPENCODE_EXTRA_ARGS)
 }

+// openCodeProviderName / openCodeAgentName are the internal names of the provider
+// and agent gadfly generates in the injected config. "gadfly" won't collide with
+// OpenCode's models.dev provider registry.
 const (
-	// openCodeProviderName / openCodeAgentName are the internal names of the
-	// provider and agent gadfly generates in the injected config. "gadfly" won't
-	// collide with OpenCode's models.dev provider registry.
 	openCodeProviderName = "gadfly"
 	openCodeAgentName    = "gadfly"
-	// defaultOpenCodeBaseURL is ollama-cloud's OpenAI-compatible endpoint.
-	defaultOpenCodeBaseURL = "https://ollama.com/v1"
 )

+// defaultOpenCodeBaseURL is ollama-cloud's OpenAI-compatible endpoint.
+const defaultOpenCodeBaseURL = "https://ollama.com/v1"
+
 // isOpenCodeSpec reports whether a GADFLY_MODEL spec selects the opencode engine:
 // the bare id "opencode"/"open-code" or an "opencode/<model>" form (both
 // spellings accepted; "opencode" is canonical).
@@ -122,9 +122,24 @@ type openCodeConfig struct {
 	Agent      map[string]openCodeAgent    `json:"agent"`
 }

-type openCodePermission struct {
-	Edit string `json:"edit"`
-	Bash string `json:"bash"`
+// openCodePermission is OpenCode's permission map: tool key → "allow"|"ask"|"deny".
+type openCodePermission map[string]string
+
+// denyMutations denies every OpenCode permission that could change the repo, run
+// commands, or reach the network / the filesystem outside the checked-out tree —
+// keeping the reviewer strictly read-only. The read/search tools OpenCode gates
+// separately (read/glob/grep/list/lsp) stay at their default so the agent can
+// still verify findings against the code. OpenCode's permission keys are
+// enumerated at https://opencode.ai/docs/agents; a key it doesn't recognize is
+// simply ignored, so listing extras is safe.
+func denyMutations() openCodePermission {
+	return openCodePermission{
+		"edit":               "deny",
+		"bash":               "deny",
+		"webfetch":           "deny",
+		"websearch":          "deny",
+		"external_directory": "deny",
+	}
 }

 type openCodeProvider struct {
@@ -147,13 +162,13 @@ type openCodeAgent struct {
 }

 // config builds the OpenCode config JSON for one pass. The system prompt becomes
-// the "gadfly" agent's prompt; edit/bash are denied at BOTH the global and agent
-// level (defense in depth — OpenCode's read-only tools stay available). For a
-// bare or pass-through spec the generated ollama-cloud provider block is omitted.
-// The API key is always the "{env:OLLAMA_API_KEY}" reference, never a literal
-// secret baked into the config.
+// the "gadfly" agent's prompt; the mutating/network tools are denied at BOTH the
+// global and agent level (defense in depth — OpenCode's read tools stay
+// available). For a bare or pass-through spec the generated ollama-cloud provider
+// block is omitted. The API key is always the "{env:OLLAMA_API_KEY}" reference,
+// never a literal secret baked into the config.
 func (e *openCodeEngine) config(system string) ([]byte, error) {
-	deny := openCodePermission{Edit: "deny", Bash: "deny"}
+	deny := denyMutations()
 	cfg := openCodeConfig{
 		Schema:     "https://opencode.ai/config.json",
 		Permission: deny,
@@ -194,16 +209,7 @@ func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int
 	// outranks any opencode.json the reviewed repo itself ships. NO_COLOR keeps the
 	// captured stdout free of ANSI decoration.
 	cmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+string(cfg), "NO_COLOR=1")
-	// Put the CLI and the Node children it spawns in their own process group and
-	// kill the WHOLE group on context cancel, so a timed-out lens can't leave
-	// orphaned opencode/node processes behind in the container.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-	cmd.Cancel = func() error {
-		if cmd.Process != nil {
-			_ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
-		}
-		return nil
-	}
+	killGroupOnCancel(cmd) // don't orphan the CLI's Node children on a timed-out lens
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
@@ -233,15 +239,20 @@ func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int
 	return "", fmt.Errorf("opencode run returned no output")
 }

-// openCodeEnv builds a minimal environment for the `opencode` subprocess: only
-// what the CLI needs (PATH/HOME, locale, Node/XDG/OPENCODE_*/GADFLY_OPENCODE_*
-// knobs) plus OLLAMA_API_KEY — which the generated provider references and which
-// claudeEnv deliberately DROPS. The runner's other secrets (GITEA_TOKEN,
-// GADFLY_FINDINGS_TOKEN, Anthropic/Claude keys) are withheld — the CLI has no need
-// for them. OPENCODE_CONFIG_CONTENT is never inherited: runPass sets it, and a
-// duplicate key would be ambiguous (getenv returns the first occurrence).
+// openCodeEnv builds a minimal environment for the `opencode` subprocess. It
+// forwards what the CLI needs to reach a model provider: OLLAMA_API_KEY for the
+// generated ollama-cloud provider (the primary opencode/<model> path), PLUS the
+// standard provider keys — ANTHROPIC_*, OPENAI_*, GOOGLE_*, GEMINI_* — so the
+// opencode/<provider>/<model> pass-through form can authenticate against
+// OpenCode's own built-in providers (those keys are otherwise stripped, which
+// broke the documented escape hatch). It still withholds gadfly's OWN secrets —
+// GITEA_TOKEN, GADFLY_API_KEY, GADFLY_FINDINGS_TOKEN, and the claude-code
+// subscription token (CLAUDE_CODE_OAUTH_TOKEN, which OpenCode can't use anyway) —
+// so they never reach the third-party CLI. OPENCODE_CONFIG_CONTENT is never
+// inherited: runPass sets it, and a duplicate key would be ambiguous (getenv
+// returns the first occurrence).
 func openCodeEnv() []string {
-	keep := func(k string) bool {
+	return filterEnv(func(k string) bool {
 		if k == "OPENCODE_CONFIG_CONTENT" {
 			return false // set explicitly by runPass; never inherit a competing value
 		}
@@ -253,13 +264,10 @@ func openCodeEnv() []string {
 			strings.HasPrefix(k, "OPENCODE_") ||
 			strings.HasPrefix(k, "GADFLY_OPENCODE_") ||
 			strings.HasPrefix(k, "NODE_") ||
-			strings.HasPrefix(k, "XDG_")
-	}
-	var env []string
-	for _, kv := range os.Environ() {
-		if k, _, ok := strings.Cut(kv, "="); ok && keep(k) {
-			env = append(env, kv)
-		}
-	}
-	return env
+			strings.HasPrefix(k, "XDG_") ||
+			strings.HasPrefix(k, "ANTHROPIC_") ||
+			strings.HasPrefix(k, "OPENAI_") ||
+			strings.HasPrefix(k, "GOOGLE_") ||
+			strings.HasPrefix(k, "GEMINI_")
+	})
 }
@@ -138,12 +138,15 @@ func TestOpenCodeConfig(t *testing.T) {
 	if ag.Prompt != sys {
 		t.Errorf("agent prompt = %q, want it to round-trip the system prompt", ag.Prompt)
 	}
-	if ag.Permission.Edit != "deny" || ag.Permission.Bash != "deny" {
-		t.Errorf("agent permission = %+v, want edit/bash deny", ag.Permission)
-	}
-	// Global permission also denies (defense in depth).
-	if cfg.Permission.Edit != "deny" || cfg.Permission.Bash != "deny" {
-		t.Errorf("global permission = %+v, want edit/bash deny", cfg.Permission)
+	// Mutating/network tools are denied at BOTH the agent and global level (defense
+	// in depth); the read/search tools stay at OpenCode's default.
+	for _, k := range []string{"edit", "bash", "webfetch", "websearch", "external_directory"} {
+		if ag.Permission[k] != "deny" {
+			t.Errorf("agent permission[%q] = %q, want deny", k, ag.Permission[k])
+		}
+		if cfg.Permission[k] != "deny" {
+			t.Errorf("global permission[%q] = %q, want deny", k, cfg.Permission[k])
+		}
 	}

 	// Provider block: correct npm, default baseURL, env-ref apiKey, model in map.
@@ -199,7 +202,8 @@ func TestOpenCodeEnvFilters(t *testing.T) {
 	t.Setenv("OLLAMA_API_KEY", "keep-ollama")
 	t.Setenv("GADFLY_API_KEY", "secret-gadfly")
 	t.Setenv("GADFLY_FINDINGS_TOKEN", "secret-findings")
-	t.Setenv("ANTHROPIC_API_KEY", "secret-anthropic")
+	t.Setenv("ANTHROPIC_API_KEY", "keep-anthropic") // pass-through provider auth
+	t.Setenv("OPENAI_API_KEY", "keep-openai")       // pass-through provider auth
 	t.Setenv("CLAUDE_CODE_OAUTH_TOKEN", "secret-claude")
 	t.Setenv("GADFLY_OPENCODE_MODEL", "keep-knob")
 	t.Setenv("OPENCODE_CONFIG_CONTENT", "should-not-inherit")
@@ -213,14 +217,16 @@ func TestOpenCodeEnvFilters(t *testing.T) {
 		}
 		return false
 	}
-	// kept: the ollama key the provider references + opencode knobs + PATH
-	for _, k := range []string{"OLLAMA_API_KEY", "GADFLY_OPENCODE_MODEL", "PATH"} {
+	// kept: the ollama key + the standard provider keys the opencode/<provider>/<model>
+	// pass-through form needs + opencode knobs + PATH
+	for _, k := range []string{"OLLAMA_API_KEY", "ANTHROPIC_API_KEY", "OPENAI_API_KEY", "GADFLY_OPENCODE_MODEL", "PATH"} {
 		if !has(k) {
 			t.Errorf("openCodeEnv dropped %s, but it should be kept", k)
 		}
 	}
-	// dropped: the runner's other secrets + the claude engine's auth
-	for _, k := range []string{"GITEA_TOKEN", "GADFLY_API_KEY", "GADFLY_FINDINGS_TOKEN", "ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"} {
+	// dropped: gadfly's own secrets + the claude engine's subscription token
+	// (OpenCode's anthropic provider uses ANTHROPIC_API_KEY, not this OAuth token).
+	for _, k := range []string{"GITEA_TOKEN", "GADFLY_API_KEY", "GADFLY_FINDINGS_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"} {
 		if has(k) {
 			t.Errorf("openCodeEnv leaked %s into the subprocess env", k)
 		}