Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a first “guardrails” packaged plugin MVP for OpenCode, wires it into the packaged profile via config, and updates the guardrails migration canon/docs to explicitly treat Anthropic’s skills guide PDF as a primary reference. It also adds scenario tests to validate plugin loading, env injection, file protection, lifecycle logging, and compaction-context behavior.
Changes:
- Add a packaged guardrail plugin and load it through the guardrails profile config.
- Improve plugin bus-event hook execution to catch/log failures instead of letting one hook break processing.
- Expand scenario tests + documentation to validate and describe the new guardrail behavior and canon.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/test/scenario/guardrails.test.ts | Adds scenario coverage for the guardrail profile plugin behavior (env injection, blocking, lifecycle logging, compaction context). |
| packages/opencode/src/plugin/index.ts | Changes plugin bus-event dispatch to run hooks via Effect.tryPromise with error logging. |
| packages/guardrails/README.md | Updates package documentation to include the packaged guardrail plugin and expanded scenario coverage. |
| packages/guardrails/profile/plugins/guardrail.ts | Introduces the guardrail plugin MVP (protected file rules, shell env injection, lifecycle logging, compaction context). |
| packages/guardrails/profile/opencode.json | Loads the guardrail plugin from the packaged profile via plugin config. |
| packages/guardrails/profile/AGENTS.md | Updates operational guidance to include progressive disclosure and guardrail state ownership. |
| docs/ai-guardrails/README.md | Updates canon/operating principles and tracking to align with the guardrail plugin MVP and Anthropic guide PDF. |
| docs/ai-guardrails/migration/claude-code-skills-inventory.md | Adds Anthropic PDF and progressive-disclosure principles to the migration inventory inputs. |
| docs/ai-guardrails/issues/README.md | Adds requirement to respect the Anthropic skill guide PDF as part of issue completion criteria. |
| docs/ai-guardrails/issues/003-guardrail-plugin-mvp.md | Adds the local issue brief for the guardrail plugin MVP deliverables/acceptance/sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const root = path.join(input.directory, ".opencode", "guardrails") | ||
| const log = path.join(root, "events.jsonl") | ||
| const state = path.join(root, "state.json") | ||
|
|
||
| await mkdir(root, { recursive: true }) | ||
|
|
||
| async function mark(data: Record<string, unknown>) { | ||
| const prev = await stash(state) | ||
| await save(state, { ...prev, ...data, mode, updated_at: new Date().toISOString() }) | ||
| } | ||
|
|
||
| async function seen(type: string, data: Record<string, unknown>) { | ||
| await line(log, { type, time: new Date().toISOString(), ...data }) | ||
| } | ||
|
|
||
| function note(props: Record<string, unknown> | undefined) { | ||
| return { | ||
| sessionID: typeof props?.sessionID === "string" ? props.sessionID : undefined, | ||
| permission: typeof props?.permission === "string" ? props.permission : undefined, | ||
| patterns: Array.isArray(props?.patterns) ? props.patterns : undefined, | ||
| } | ||
| } | ||
|
|
||
| function hidden(file: string) { | ||
| return rel(input.worktree, file).startsWith(".opencode/guardrails/") | ||
| } | ||
|
|
||
| function deny(file: string, kind: "read" | "edit") { | ||
| const item = rel(input.worktree, file) | ||
| if (kind === "read" && has(item, sec)) return "secret material is outside the allowed read surface" | ||
| if (hidden(file)) return "guardrail runtime state is plugin-owned" | ||
| if (kind === "edit" && has(item, cfg)) return "linter or formatter configuration is policy-protected" | ||
| } |
There was a problem hiding this comment.
hidden()/deny() compute relative paths from input.worktree, but guardrail state is written under path.join(input.directory, ".opencode/guardrails"). If directory is a subfolder of worktree (common when launching from a nested path), .opencode/guardrails/* will not start with .opencode/guardrails/ relative to the worktree, so protected state files may not be blocked by the read/write checks. Consider basing the hidden/state checks on the actual root path (e.g., compare normalized absolute paths to root) or use input.directory consistently for rel()/hidden()/deny().
| async function line(file: string, data: Record<string, unknown>) { | ||
| const prev = await Bun.file(file).text().catch(() => "") | ||
| await Bun.write(file, prev + JSON.stringify(data) + "\n") | ||
| } |
There was a problem hiding this comment.
line() appends by reading the entire existing log file into memory and rewriting it (prev + ...). This is O(n) per event and can become very slow as events.jsonl grows; it also risks lost updates if writes ever overlap. Prefer an actual append (e.g., fs.appendFile / Bun.write(..., { append: true })) so each event write is constant-time and atomic at the file level.
| if (item.tool === "bash") { | ||
| const cmd = typeof out.args?.command === "string" ? out.args.command : "" | ||
| const file = cmd.replaceAll("\\", "/") | ||
| if (!cmd) return | ||
| if (has(file, sec) || file.includes(".opencode/guardrails/")) { | ||
| await mark({ last_block: "bash", last_command: cmd, last_reason: "shell access to protected files" }) | ||
| throw new Error(text("shell access to protected files")) | ||
| } | ||
| if (!bash(cmd)) return | ||
| if (!cfg.some((rule) => rule.test(file)) && !file.includes(".opencode/guardrails/")) return | ||
| await mark({ last_block: "bash", last_command: cmd, last_reason: "protected runtime or config mutation" }) | ||
| throw new Error(text("protected runtime or config mutation")) |
There was a problem hiding this comment.
The bash protected-file check runs has(file, sec) where file is the full shell command string (e.g. cat .env). The sec regexes are written for path-like strings ((^|/)\.env..., .*\.pem$, etc.), so common commands containing whitespace/flags won’t match and will bypass the intended protection. Consider parsing the command/args (or at least scanning for protected path tokens with boundaries that include whitespace/quotes) before applying the secret/config patterns.
| const hook = (await Plugin.list()).find((item) => typeof item.event === "function") | ||
| expect(hook?.event).toBeDefined() | ||
|
|
There was a problem hiding this comment.
This test picks the first plugin hook that has an event function. That’s currently likely the guardrail plugin, but it can become flaky if another built-in/external plugin later adds an event hook. Consider selecting the hook more specifically (e.g., by checking it also implements experimental.session.compacting or tool.execute.before, or by matching the plugin origin if that metadata is available in the hook).
| }, | ||
| }, | ||
| } as any) | ||
| await wait() |
There was a problem hiding this comment.
wait() is likely unnecessary here because each hook.event invocation is already awaited and the plugin’s event handler awaits its file writes. Keeping the extra sleep can add flakiness (timing-dependent) and slows CI; consider removing it or replacing it with a deterministic wait (e.g., poll for the log/state file to exist).
| await wait() |
| catch: (err) => { | ||
| log.error("plugin event hook failed", { error: err, type: input.type }) | ||
| }, | ||
| }).pipe(Effect.ignore), |
There was a problem hiding this comment.
This change alters event delivery semantics: previously hook.event?.(...) was invoked inside Effect.sync without awaiting any returned promise, so async event handlers ran fire-and-forget (and could produce unhandled rejections). The new Effect.tryPromise awaits each hook’s event promise, which can slow or backpressure bus event processing if a plugin does I/O in event. If the intention is to prevent a single plugin from stalling the event stream while still capturing/logging failures, consider forking each hook invocation (scoped) or using bounded concurrency.
| }).pipe(Effect.ignore), | |
| }).pipe(Effect.ignore, Effect.forkScoped), |
| @@ -0,0 +1,48 @@ | |||
| # Issue 003: Guardrail Plugin MVP | |||
There was a problem hiding this comment.
The local brief is titled “Issue 003”, but the PR description and docs/ai-guardrails/README.md tracking refer to GitHub issue #4 as the current issue. To avoid confusion for readers, consider adding an explicit cross-reference in the header (e.g., “(GitHub #4)”) or aligning the numbering scheme in the title.
| # Issue 003: Guardrail Plugin MVP | |
| # Issue 003 (GitHub #4): Guardrail Plugin MVP |
Summary
Verification
Closes #4