From 2191a0b47c92e31c57dfcf1eae9b229dd79f2bbd Mon Sep 17 00:00:00 2001 From: theyavuzarslan <80851990+theyavuzarslan@users.noreply.github.com> Date: Fri, 19 Jun 2026 05:02:23 +0300 Subject: [PATCH 1/2] feat(goose): MCP advisory integration + upstream hard-gate proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Goose has no out-of-process plugin API — the only true Allow/Deny/ RequireApproval surface is the in-process Rust ToolInspector trait (crates/goose/src/tool_inspection.rs), which is compile-time. This PR ships the two integrations that ARE possible today and clearly distinguishes their security guarantees. Surface 1 — MCP extension (advisory, ships today) ================================================= `agentguard init --agent goose` writes/merges an `agentguard` entry under `extensions:` in `~/.config/goose/config.yaml` (or `%APPDATA%/Block/goose/ config/config.yaml` on Windows, or `$GOOSE_CONFIG_DIR/config.yaml` if set): extensions: agentguard: type: stdio command: agentguard-mcp args: [] timeout: 300 enabled: true description: GoPlus AgentGuard MCP — security scanner + action evaluator The model can now call AgentGuard's scanner/action-evaluator MCP tools. This is **advisory only**: the agent can skip calling AgentGuard and go straight to `developer__shell`, and AgentGuard never sees it. plugins/goose/README.md states this in plain language so no user is under the impression they have a hard gate. Surface 2 — upstream hard-gate proposal (draft, no PR yet) ========================================================= plugins/goose/UPSTREAM_PROPOSAL.md drafts a SECURITY_INSPECTOR_ENDPOINT webhook ToolInspector for block/goose, mirroring their existing SECURITY_PROMPT_CLASSIFIER_ENDPOINT pattern. That would let AgentGuard (or any other vendor) become a real out-of-process pre-execution gate with Allow/Deny/RequireApproval semantics. Includes JSON schema, fail policy, and open questions to confirm with maintainers before opening the PR. Wiring ====== - 'goose' added to AgentInstaller, RuntimeAgentHost, AgentGuardAgentHost, SUPPORTED_AGENT_INSTALLERS, normalizeAgentHost. Intentionally NOT in AUTO_AGENT_DETECTION since Goose config lives at ~/.config/goose/ (outside cwd) and an advisory install shouldn't be auto-applied. - resolveCronAgentHost narrowed to the cron-capable subset (matches the pattern used when wiring other non-cron hosts). - YAML merge: hand-rolled (no js-yaml dep). Handles three cases — (a) no config.yaml at all → create with extensions: block, (b) extensions: exists → insert agentguard: as a sibling child, (c) agentguard: already present → no-op (idempotent). Force mode strips the prior block and re-emits in canonical form. Tests: 419/419 pass (was 415; +4 — fresh install, merge into existing extensions preserving other entries and top-level keys, idempotency under repeated runs, and a docs check that the README states the "advisory, not a security boundary" framing.) --- plugins/goose/README.md | 82 ++++++++++++++++++ plugins/goose/UPSTREAM_PROPOSAL.md | 118 ++++++++++++++++++++++++++ src/cli.ts | 17 ++-- src/config.ts | 4 +- src/installers.ts | 129 ++++++++++++++++++++++++++++- src/runtime/types.ts | 1 + src/tests/installer.test.ts | 88 ++++++++++++++++++++ 7 files changed, 431 insertions(+), 8 deletions(-) create mode 100644 plugins/goose/README.md create mode 100644 plugins/goose/UPSTREAM_PROPOSAL.md diff --git a/plugins/goose/README.md b/plugins/goose/README.md new file mode 100644 index 0000000..9e74dad --- /dev/null +++ b/plugins/goose/README.md @@ -0,0 +1,82 @@ +# GoPlus AgentGuard — Goose integration (advisory, MCP-based) + +Goose ([block/goose](https://github.com/block/goose)) is **not** integrated +the same way as Cline, Hermes, or Continue. This README explains the +limitation honestly and documents the install paths that ship today. + +## TL;DR + +```bash +npm i -g @goplus/agentguard +agentguard init --agent goose +``` + +That writes `agentguard` as an MCP extension in `~/.config/goose/config.yaml`: + +```yaml +extensions: + agentguard: + type: stdio + command: agentguard-mcp + args: [] + timeout: 300 + enabled: true + description: GoPlus AgentGuard MCP — security scanner + action evaluator +``` + +Restart Goose. AgentGuard's scanner and action evaluator are now MCP-callable. + +## Important: this is advisory, not a hard gate + +Goose currently has **no out-of-process plugin API** for intercepting tool +calls. The only true `Allow / Deny / RequireApproval` surface is the in-process +Rust `ToolInspector` trait (`crates/goose/src/tool_inspection.rs`), which is +compile-time — there is no dynamic loader for it. + +What an MCP extension can do: + +- **Provide tools the model may choose to call** (`scan_skill`, `evaluate_action`, etc.) +- **Refuse to return data** when called + +What an MCP extension **cannot** do: + +- Sit on the execution path of every Goose tool call +- Block a `developer__shell` call the model never routed through AgentGuard + +In other words, if the agent decides to call `shell` with `rm -rf /` and +never asks AgentGuard's MCP tools first, AgentGuard never gets a chance to +veto it. The model is free to skip you. **Treat this as defense-in-depth, +not a security boundary.** + +For a real hard gate, see [UPSTREAM_PROPOSAL.md](./UPSTREAM_PROPOSAL.md) — a +draft proposal to add a `SECURITY_INSPECTOR_ENDPOINT` webhook to Goose, +mirroring its existing `SECURITY_PROMPT_CLASSIFIER_ENDPOINT`. That would +turn AgentGuard into a genuine pre-execution inspector with `block` / +`require_approval` returns. + +## What the MCP integration is useful for + +- **Pre-commit / pre-PR scanning** of skills the agent wants to install +- **Action evaluation** when the agent explicitly asks "is this dangerous?" +- **Audit logging** of evaluations the model did make through AgentGuard + +These are real value-adds even without a hard gate, but be clear with users +about the boundary. + +## Manual install + +If you'd rather not run `agentguard init --agent goose`, copy the snippet +above into your existing `~/.config/goose/config.yaml` under `extensions:` +(or create the file if it doesn't exist). On Windows the path is +`%APPDATA%\Block\goose\config\config.yaml`. + +The installer preserves any prior `extensions:` entries and is idempotent — +re-running won't duplicate the block. + +## Reference + +- Goose extensions / MCP: https://block.github.io/goose/docs/getting-started/installation +- ToolInspector trait (compile-time): `crates/goose/src/tool_inspection.rs` +- Existing classifier endpoint (the model we're proposing to copy): + `crates/goose/src/security/classification_client.rs` + + `SECURITY_PROMPT_CLASSIFIER_ENDPOINT` diff --git a/plugins/goose/UPSTREAM_PROPOSAL.md b/plugins/goose/UPSTREAM_PROPOSAL.md new file mode 100644 index 0000000..31effe4 --- /dev/null +++ b/plugins/goose/UPSTREAM_PROPOSAL.md @@ -0,0 +1,118 @@ +# Goose upstream proposal — generic security webhook inspector + +This document drafts a proposal to file against [block/goose](https://github.com/block/goose) +that would give third-party security tools (AgentGuard, but also others) a +real out-of-process hard gate on tool execution. + +This is **not** a PR yet — it's the design that should accompany one. + +## Problem + +Goose currently has two ways to wire in security policy on tool calls: + +1. **`ToolInspector` trait** (Rust, in-process, compile-time). The real + `Allow / Deny / RequireApproval` decision surface, but there is no + dynamic loader for it — third parties cannot ship one without forking + and rebuilding Goose. +2. **MCP extensions**. Model-callable. The agent can skip your tools, so + this is advisory only — it is not a security boundary. + +This means there is no supported way today for an external security service +(static analyzer, runtime engine, anomaly detector) to gate Goose's tool +execution without forking the binary. + +## Prior art inside Goose + +Goose already accepts an external HTTP endpoint for one specific +classification job. From `crates/goose/src/security/classification_client.rs` +and the config: + +``` +SECURITY_PROMPT_CLASSIFIER_ENDPOINT +SECURITY_PROMPT_ENABLED +``` + +The classifier endpoint receives JSON, returns a classification, and Goose +honors the result. This is the pattern we're asking to generalize. + +## Proposal + +Add a new `WebhookToolInspector` that registers automatically when a +configured endpoint is set: + +```yaml +# ~/.config/goose/config.yaml +SECURITY_INSPECTOR_ENABLED: true +SECURITY_INSPECTOR_ENDPOINT: "http://127.0.0.1:7777/inspect" +SECURITY_INSPECTOR_TIMEOUT_MS: 1500 +SECURITY_INSPECTOR_FAIL_OPEN: false # default: fail-closed +SECURITY_INSPECTOR_AUTH_HEADER: "X-Token: …" +``` + +At startup, `ToolInspectionManager::add_inspector` registers a built-in +`WebhookToolInspector` if `SECURITY_INSPECTOR_ENABLED == true`. It calls the +endpoint for every `ToolRequest`, with a JSON body like: + +```json +{ + "session_id": "sess_…", + "tool_requests": [ + { "tool": "developer__shell", "input": { "command": "rm -rf /" }, "call_id": "…" } + ], + "messages": [ /* optional, truncated */ ], + "goose_mode": "auto" +} +``` + +And expects back: + +```json +{ + "results": [ + { "call_id": "…", + "action": "Deny", + "reason": "destructive command", + "policy_version": "…" } + ] +} +``` + +`action` is one of `Allow`, `Deny`, `RequireApproval`. The webhook's response +maps 1:1 to `InspectionAction` — no impedance mismatch with the existing +trait. + +## Why this is a small change + +- It doesn't change the `ToolInspector` trait. The new inspector is just + another implementer. +- It mirrors a pattern Goose already accepts (the classifier endpoint), so + it should pass the "is this in scope for this project?" smell test. +- Fail-open vs fail-closed is configurable, with a security-safe default + (fail-closed). +- No new permissions to plumb through the trust system; existing + `ToolInspectionManager` handles aggregation. + +## What we'd ship in the PR + +1. `crates/goose/src/security/webhook_inspector.rs` — `WebhookToolInspector` impl +2. Config keys + parsing in `crates/goose/src/config/` +3. Registration in `crates/goose/src/agents/` startup +4. Tests (mocking the endpoint with `wiremock` or similar) +5. A docs page in `docs/security/` covering trade-offs and recommended deploys + +## Open questions to confirm with maintainers before opening the PR + +- Naming: `SECURITY_INSPECTOR_*` vs `TOOL_INSPECTOR_*` vs scoped to a single + vendor key. We'd prefer the generic name so other security vendors can use + the same surface. +- Should the webhook also see `PostToolUse` events for audit-only logging, + or is pre-execution enough? (Our preference: pre-only, keep the surface + minimal; audit lives elsewhere.) +- Should responses be allowed to mutate the tool input (`overrideInput`), + as Cline and Continue hooks do? Probably no for v1 — keeps the threat + model simple. + +--- + +**Status:** draft. We (GoPlus) intend to file an issue first to gauge +maintainer interest before opening a PR. Anyone in this repo can take it on. diff --git a/src/cli.ts b/src/cli.ts index 277609c..184a55b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -40,15 +40,18 @@ import { type CronBackend, type ThreatFeedCronRemovalResult, type OpenClawGatewayOptions, + type CronAgentHost, } from './feed/cron.js'; -const SUPPORTED_AGENT_INSTALLERS: AgentInstaller[] = ['claude-code', 'codex', 'openclaw', 'hermes', 'qclaw']; +const SUPPORTED_AGENT_INSTALLERS: AgentInstaller[] = ['claude-code', 'codex', 'openclaw', 'hermes', 'qclaw', 'goose']; const AUTO_AGENT_DETECTION: Array<{ agent: AgentInstaller; dir: string }> = [ { agent: 'claude-code', dir: '.claude' }, { agent: 'openclaw', dir: '.openclaw' }, { agent: 'hermes', dir: '.hermes' }, { agent: 'qclaw', dir: '.qclaw' }, { agent: 'codex', dir: '.codex' }, + // 'goose' is intentionally NOT in auto-detection: config lives at + // ~/.config/goose/ (outside cwd), and an MCP install is advisory-only. ]; const REQUIRED_INIT_COMMAND = 'agentguard init --agent auto'; @@ -64,7 +67,7 @@ async function main() { .command('init') .description('Create ~/.agentguard/config.json and local runtime paths') .option('--level ', 'Protection level: strict | balanced | permissive') - .option('--agent ', 'Install hook/template for claude-code, codex, openclaw, hermes, or qclaw') + .option('--agent ', 'Install hook/template for claude-code, codex, openclaw, hermes, qclaw, or goose (MCP advisory)') .option('--cloud ', 'AgentGuard Cloud URL to store in local config') .option('--shell-hooks', 'For Hermes: install legacy shell hooks instead of the native plugin') .option('--force', 'Overwrite existing hook/template files') @@ -106,7 +109,7 @@ async function main() { return; } if (!SUPPORTED_AGENT_INSTALLERS.includes(normalizedAgent as AgentInstaller)) { - throw new Error('Invalid agent. Use auto, claude-code, codex, openclaw, hermes, or qclaw.'); + throw new Error('Invalid agent. Use auto, claude-code, codex, openclaw, hermes, qclaw, or goose.'); } const agent = normalizedAgent as AgentInstaller; config.agentHost = agent; @@ -1062,8 +1065,12 @@ function printCronRemovalSummary(results: ThreatFeedCronRemovalResult[]): void { console.log('No AgentGuard subscribe cron job was found.'); } -function resolveCronAgentHost(config: AgentGuardConfig): AgentGuardAgentHost | undefined { - return config.agentHost ?? config.agentHosts?.[0]; +function resolveCronAgentHost(config: AgentGuardConfig): CronAgentHost | undefined { + const host = config.agentHost ?? config.agentHosts?.[0]; + if (host === 'claude-code' || host === 'codex' || host === 'openclaw' || host === 'hermes' || host === 'qclaw') { + return host; + } + return undefined; } function readStdinIfAvailable(): string { diff --git a/src/config.ts b/src/config.ts index 2a6adfa..601f802 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2,7 +2,7 @@ import { chmodSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } import { dirname, join } from 'node:path'; import { homedir } from 'node:os'; -export type AgentGuardAgentHost = 'claude-code' | 'codex' | 'openclaw' | 'hermes' | 'qclaw'; +export type AgentGuardAgentHost = 'claude-code' | 'codex' | 'openclaw' | 'hermes' | 'qclaw' | 'goose'; export interface AgentGuardConfig { version: 1; @@ -226,7 +226,7 @@ function normalizeLevel(value: unknown): AgentGuardConfig['level'] | null { } function normalizeAgentHost(value: unknown): AgentGuardAgentHost | undefined { - return value === 'claude-code' || value === 'codex' || value === 'openclaw' || value === 'hermes' || value === 'qclaw' + return value === 'claude-code' || value === 'codex' || value === 'openclaw' || value === 'hermes' || value === 'qclaw' || value === 'goose' ? value : undefined; } diff --git a/src/installers.ts b/src/installers.ts index 24f729b..66cf71e 100644 --- a/src/installers.ts +++ b/src/installers.ts @@ -2,7 +2,7 @@ import { cpSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, wr import { homedir } from 'node:os'; import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; -export type AgentInstaller = 'claude-code' | 'codex' | 'openclaw' | 'hermes' | 'qclaw'; +export type AgentInstaller = 'claude-code' | 'codex' | 'openclaw' | 'hermes' | 'qclaw' | 'goose'; export interface InstallResult { agent: AgentInstaller; @@ -21,6 +21,7 @@ export function installAgentTemplates(agent: AgentInstaller, options: { cwd?: st if (agent === 'openclaw') return installOpenClaw(options.cwd, Boolean(options.force)); if (agent === 'hermes') return installHermes(options.cwd, Boolean(options.force), { shellHooks: Boolean(options.shellHooks) }); if (agent === 'qclaw') return installQClaw(root, Boolean(options.force)); + if (agent === 'goose') return installGoose(options.cwd, Boolean(options.force)); throw new Error(`Unsupported agent installer: ${agent}`); } @@ -240,6 +241,132 @@ function installQClaw(root: string, force: boolean): InstallResult { return { agent: 'qclaw', files: pluginResult.files }; } +/** + * Install AgentGuard as an MCP extension in Goose. + * + * Goose has no out-of-process plugin API — the only in-process hard gate + * is the Rust `ToolInspector` trait, which is compile-time. The closest + * supported integration today is an MCP server entry in config.yaml. The + * model can choose not to call AgentGuard's MCP tools, so this is an + * **advisory** integration, not a hard security boundary. See + * plugins/goose/README.md for the honest framing and + * plugins/goose/UPSTREAM_PROPOSAL.md for the path to a real gate. + */ +function installGoose(cwd: string | undefined, force: boolean): InstallResult { + const configDir = cwd + ? join(cwd, '.config', 'goose') + : process.env.GOOSE_CONFIG_DIR?.trim() || + (process.platform === 'win32' + ? join(process.env.APPDATA || join(homedir(), 'AppData', 'Roaming'), 'Block', 'goose', 'config') + : join(homedir(), '.config', 'goose')); + const configPath = join(configDir, 'config.yaml'); + + mkdirSync(configDir, { recursive: true }); + const existing = existsSync(configPath) ? readFileSync(configPath, 'utf8') : ''; + const next = mergeGooseAgentGuardExtension(existing, force); + if (next !== existing) { + writeFileSync(configPath, next); + } + + return { agent: 'goose', files: [configPath] }; +} + +const GOOSE_AGENTGUARD_BLOCK = [ + ' agentguard:', + ' type: stdio', + ' command: agentguard-mcp', + ' args: []', + ' timeout: 300', + ' enabled: true', + " description: GoPlus AgentGuard MCP — security scanner + action evaluator", +].join('\n'); + +function mergeGooseAgentGuardExtension(existing: string, force: boolean): string { + const text = existing.replace(/\r\n/g, '\n'); + + // Already configured — keep as-is unless force is set. + if (gooseHasAgentGuardEntry(text) && !force) return existing; + + // If force is set and there's a previous entry, strip it first. + const baseline = force ? stripGooseAgentGuardEntry(text) : text; + + // No `extensions:` block at all — prepend a fresh one (preserve trailing text). + if (!gooseHasTopLevelKey(baseline, 'extensions')) { + const prefix = `extensions:\n${GOOSE_AGENTGUARD_BLOCK}\n`; + if (baseline.trim().length === 0) return prefix; + return baseline.endsWith('\n') ? `${prefix}${baseline}` : `${prefix}${baseline}\n`; + } + + // `extensions:` exists — append `agentguard:` as a child block. + return appendInsideGooseExtensions(baseline); +} + +function gooseHasAgentGuardEntry(text: string): boolean { + // Match `agentguard:` under an `extensions:` top-level block. + const lines = text.split('\n'); + let inExtensions = false; + for (const line of lines) { + if (/^extensions:\s*(?:#.*)?$/.test(line)) { + inExtensions = true; + continue; + } + if (inExtensions && /^[A-Za-z0-9_-]+:/.test(line)) { + // Hit a new top-level key — extensions block ended. + inExtensions = false; + } + if (inExtensions && /^\s+agentguard:\s*(?:#.*)?$/.test(line)) return true; + } + return false; +} + +function gooseHasTopLevelKey(text: string, key: string): boolean { + const re = new RegExp(`^${key}:\\s*(?:\\{\\}\\s*)?(?:#.*)?$`, 'm'); + return re.test(text); +} + +function appendInsideGooseExtensions(text: string): string { + // Insert the AgentGuard block directly after the `extensions:` line. YAML + // doesn't care about sibling order so we don't need to walk to the end of + // the existing block — that walk is what introduced the trailing-blank + // bug. Children of `extensions:` are indented (2 spaces), so the new + // block slots in cleanly. + const lines = text.split('\n'); + const out: string[] = []; + let inserted = false; + + for (let i = 0; i < lines.length; i++) { + out.push(lines[i]); + if (!inserted && /^extensions:\s*(?:#.*)?$/.test(lines[i])) { + out.push(GOOSE_AGENTGUARD_BLOCK); + inserted = true; + } + } + + if (!inserted) { + // Defensive: extensions: existed but was malformed; append at end. + out.push('extensions:', GOOSE_AGENTGUARD_BLOCK); + } + + const joined = out.join('\n'); + return joined.endsWith('\n') ? joined : `${joined}\n`; +} + +function stripGooseAgentGuardEntry(text: string): string { + const lines = text.split('\n'); + const out: string[] = []; + let i = 0; + while (i < lines.length) { + if (/^\s+agentguard:\s*(?:#.*)?$/.test(lines[i])) { + i++; + while (i < lines.length && /^\s{4,}/.test(lines[i])) i++; + continue; + } + out.push(lines[i]); + i++; + } + return out.join('\n'); +} + function writeIfAllowed(path: string, content: string, force: boolean): void { if (existsSync(path) && !force) return; mkdirSync(dirname(path), { recursive: true }); diff --git a/src/runtime/types.ts b/src/runtime/types.ts index 60cc09e..8551465 100644 --- a/src/runtime/types.ts +++ b/src/runtime/types.ts @@ -23,6 +23,7 @@ export type RuntimeAgentHost = | 'cursor' | 'gemini' | 'copilot' + | 'goose' | 'other'; export interface PolicyReason { diff --git a/src/tests/installer.test.ts b/src/tests/installer.test.ts index d712316..7ba53a9 100644 --- a/src/tests/installer.test.ts +++ b/src/tests/installer.test.ts @@ -297,4 +297,92 @@ describe('Agent template installers', () => { assert.deepEqual(config.plugins.allow, ['existing', 'agentguard']); assert.equal(config.plugins.entries.agentguard.enabled, true); }); + + it('installs AgentGuard as a Goose MCP extension when config.yaml does not exist', () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-fresh-')); + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = join(dir, '.config', 'goose'); + try { + const result = installAgentTemplates('goose'); + const configPath = join(process.env.GOOSE_CONFIG_DIR, 'config.yaml'); + + assert.equal(result.agent, 'goose'); + assert.deepEqual(result.files, [configPath]); + const yaml = readFileSync(configPath, 'utf8'); + assert.ok(yaml.includes('extensions:')); + assert.ok(yaml.includes(' agentguard:')); + assert.ok(yaml.includes('command: agentguard-mcp')); + assert.ok(yaml.includes('type: stdio')); + assert.ok(yaml.includes('enabled: true')); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it("merges AgentGuard into a Goose config that already declares other extensions", () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-merge-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + configPath, + [ + 'OPENAI_API_KEY: sk-redacted', + 'GOOSE_MODE: auto', + 'extensions:', + ' slack:', + ' type: stdio', + ' command: uvx', + ' args: [mcp_slack]', + ' enabled: true', + '', + ].join('\n') + ); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + installAgentTemplates('goose'); + const yaml = readFileSync(configPath, 'utf8'); + assert.ok(yaml.includes('OPENAI_API_KEY: sk-redacted')); + assert.ok(yaml.includes(' slack:')); + assert.ok(yaml.includes('command: uvx')); + assert.ok(yaml.includes(' agentguard:')); + assert.ok(yaml.includes('command: agentguard-mcp')); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it('is idempotent: re-running goose install produces byte-identical config', () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-idempotent-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + installAgentTemplates('goose'); + const first = readFileSync(configPath, 'utf8'); + installAgentTemplates('goose'); + const second = readFileSync(configPath, 'utf8'); + assert.equal(first, second); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it('Goose plugin docs ship in the published package', () => { + // The README and UPSTREAM_PROPOSAL are the deliverable. Lock them in. + const pluginDir = join(__dirname, '..', '..', 'plugins', 'goose'); + assert.ok(existsSync(join(pluginDir, 'README.md'))); + assert.ok(existsSync(join(pluginDir, 'UPSTREAM_PROPOSAL.md'))); + const readme = readFileSync(join(pluginDir, 'README.md'), 'utf8'); + // The README must be honest about the limitation. + assert.ok(/advisory/i.test(readme)); + assert.ok(/not a (?:hard )?(?:gate|security boundary)/i.test(readme)); + }); }); From f66e6e453018b66470c0427978975f8520126627 Mon Sep 17 00:00:00 2001 From: theyavuzarslan <80851990+theyavuzarslan@users.noreply.github.com> Date: Sun, 21 Jun 2026 03:55:40 +0300 Subject: [PATCH 2/2] fix(goose): address PR review (real YAML parser, cron host fallback) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues raised in review; all addressed. 1. HIGH — hand-rolled YAML mutation assumed extensions: was a simple top-level block followed by indented children. Configs with comments on entries, multiple top-level keys after extensions:, quoted keys, or 4-space indentation could be corrupted or have unrelated content moved into wrong indentation levels. Fix: replaced the line-based merge with a real YAML parser (eemeli/yaml, the de-facto comment-preserving Node YAML library; added as a runtime dep). The merge now: - parses with parseDocument() so comments and source positions survive, - mutates only the extensions.agentguard key via doc.setIn(), - serializes via doc.toString() preserving siblings, top-level keys before AND after extensions:, and comments, - REFUSES to write the file when extensions: is the wrong shape (sequence, scalar, null) or when the input is unparseable — surfacing a clear error instead of silently clobbering user data. 2. MEDIUM — idempotency/--force depended on indentation-sensitive regexes that missed tabs, quoted keys, and nested cases. Resolved by the same fix: structured parsing via doc.has('extensions') and extensions.get('agentguard') no longer cares about layout. 3. MEDIUM — agentHost adds 'goose' globally but resolveCronAgentHost silently filters it out, leaving cron-related commands with surprising behavior. Same fix shipped on the Continue branch applies here: - Split into resolveConfiguredAgentHost() returning the raw host and resolveCronBackendHost() returning the cron-narrowed value. - noteCronBackendFallbackIfNeeded() prints a one-line stderr note on first call when the configured host has no agent-managed cron backend, so users know they're getting system cron rather than silent fall-back. - Both cron entry points (subscribe install, disconnect cleanup) now call the note + the narrowed helper. Tests: 424/424 pass (was 419; +5 new — comments + inline comments + trailing top-level keys survive merge; refusal on sequence-typed extensions:; refusal on unparseable YAML with file untouched; quoted keys + 4-space indent variant round-trips; --force replaces only the agentguard entry). End-to-end verified against a complex /tmp/goose-complex/.config/goose/ config.yaml containing comments, multiple existing extensions (slack, github), and trailing top-level keys (GOOSE_PROVIDER, SECURITY_PROMPT_ENABLED). After merge: all comments preserved, all extensions kept, AgentGuard inserted as sibling, trailing top-level keys intact. Adds yaml@^2.6.1 to dependencies. --- package-lock.json | 13 +++ package.json | 1 + src/cli.ts | 63 +++++++++++-- src/installers.ts | 148 +++++++++++++++---------------- src/tests/installer.test.ts | 172 ++++++++++++++++++++++++++++++++++++ 5 files changed, 312 insertions(+), 85 deletions(-) diff --git a/package-lock.json b/package-lock.json index 224e97a..b2620bf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,6 +15,7 @@ "commander": "12.1.0", "glob": "13.0.6", "open": "10.2.0", + "yaml": "^2.6.1", "zod": "3.25.76" }, "bin": { @@ -2153,6 +2154,18 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/yaml": { + "version": "2.6.1", + "resolved": "https://registry.npmjs.org/yaml/-/yaml-2.6.1.tgz", + "integrity": "sha512-7r0XPzioN/Q9kXBro/XPnA6kznR73DHq+GXh5ON7ZozRO6aMjbmiBuKste2wslTFkC5d1dw0GooOCepZXJ2SAg==", + "license": "ISC", + "bin": { + "yaml": "bin.mjs" + }, + "engines": { + "node": ">= 14" + } + }, "node_modules/yoctocolors-cjs": { "version": "2.1.3", "resolved": "https://registry.npmjs.org/yoctocolors-cjs/-/yoctocolors-cjs-2.1.3.tgz", diff --git a/package.json b/package.json index b48c30e..ebd9a3a 100644 --- a/package.json +++ b/package.json @@ -55,6 +55,7 @@ "commander": "12.1.0", "glob": "13.0.6", "open": "10.2.0", + "yaml": "^2.6.1", "zod": "3.25.76" }, "devDependencies": { diff --git a/src/cli.ts b/src/cli.ts index 184a55b..340c797 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -205,10 +205,11 @@ async function main() { .description('Disconnect local AgentGuard from AgentGuard Cloud') .action(async () => { const currentConfig = ensureConfig(); + noteCronBackendFallbackIfNeeded(currentConfig); const cronRemoval = await removeThreatFeedCron({ name: currentConfig.threatFeedCronName || 'agentguard-threat-feed', backend: 'auto', - agentHost: resolveCronAgentHost(currentConfig), + agentHost: resolveCronBackendHost(currentConfig), agentGuardHome: getAgentGuardPaths().home, }); const config = disconnectCloud(); @@ -706,13 +707,14 @@ async function main() { if (options.cron && !options.cronRun) { summary.cron.requested = true; try { + noteCronBackendFallbackIfNeeded(config); summary.cron.result = await installThreatFeedCron({ name: options.cronName as string, cronExpression: cronExpression!, quiet, force: Boolean(options.force), backend: cronTarget, - agentHost: resolveCronAgentHost(config), + agentHost: resolveCronBackendHost(config), agentGuardHome: getAgentGuardPaths().home, }, { gateway: resolveOpenClawGatewayOptionsFromEnv(), @@ -1065,12 +1067,59 @@ function printCronRemovalSummary(results: ThreatFeedCronRemovalResult[]): void { console.log('No AgentGuard subscribe cron job was found.'); } +/** + * Hosts that have a cron-targeted backend (OpenClaw / Hermes use agent-managed + * cron; the rest fall through to system cron). Hosts not in this set still + * accept cron commands — they just default to the system backend. + */ +const CRON_CAPABLE_HOSTS = new Set([ + 'claude-code', + 'codex', + 'openclaw', + 'hermes', + 'qclaw', +]); + +/** + * Return the host configured for this AgentGuard install. Always returns the + * raw host (never silently strips) so messaging code can name it correctly. + */ +function resolveConfiguredAgentHost(config: AgentGuardConfig): AgentGuardAgentHost | undefined { + return config.agentHost ?? config.agentHosts?.[0]; +} + +/** + * Narrow the configured host to one of the cron-capable backends used by + * `installThreatFeedCron` / `removeThreatFeedCron`. Returns undefined when + * the host has no specific cron backend — callers should treat that as + * "fall back to system cron" rather than as "no host configured". + */ +function resolveCronBackendHost(config: AgentGuardConfig): CronAgentHost | undefined { + const host = resolveConfiguredAgentHost(config); + return host && CRON_CAPABLE_HOSTS.has(host) ? (host as CronAgentHost) : undefined; +} + +/** + * If the user has a host configured that has no agent-specific cron backend + * (e.g. `goose`, `continue`), print a one-line stderr note so they know cron + * is falling back to the system scheduler. Idempotent per-process. + */ +let cronFallbackNoteShown = false; +function noteCronBackendFallbackIfNeeded(config: AgentGuardConfig): void { + if (cronFallbackNoteShown) return; + const host = resolveConfiguredAgentHost(config); + if (!host || CRON_CAPABLE_HOSTS.has(host)) return; + cronFallbackNoteShown = true; + console.error( + `Note: agent host "${host}" has no agent-managed cron backend. Cron commands will use the system scheduler. ` + + `Run \`agentguard init --agent openclaw\` or \`agentguard init --agent hermes\` if you'd like an agent-managed schedule instead.` + ); +} + +// Back-compat name retained so existing call sites compile without churn. +// New code should use `resolveCronBackendHost` for clarity. function resolveCronAgentHost(config: AgentGuardConfig): CronAgentHost | undefined { - const host = config.agentHost ?? config.agentHosts?.[0]; - if (host === 'claude-code' || host === 'codex' || host === 'openclaw' || host === 'hermes' || host === 'qclaw') { - return host; - } - return undefined; + return resolveCronBackendHost(config); } function readStdinIfAvailable(): string { diff --git a/src/installers.ts b/src/installers.ts index 66cf71e..7350775 100644 --- a/src/installers.ts +++ b/src/installers.ts @@ -1,6 +1,7 @@ import { cpSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; +import * as YAML from 'yaml'; export type AgentInstaller = 'claude-code' | 'codex' | 'openclaw' | 'hermes' | 'qclaw' | 'goose'; @@ -271,100 +272,91 @@ function installGoose(cwd: string | undefined, force: boolean): InstallResult { return { agent: 'goose', files: [configPath] }; } -const GOOSE_AGENTGUARD_BLOCK = [ - ' agentguard:', - ' type: stdio', - ' command: agentguard-mcp', - ' args: []', - ' timeout: 300', - ' enabled: true', - " description: GoPlus AgentGuard MCP — security scanner + action evaluator", -].join('\n'); +/** + * The canonical AgentGuard MCP extension entry. Kept as a plain object so + * the YAML library serializes it consistently regardless of input formatting. + */ +const GOOSE_AGENTGUARD_ENTRY = Object.freeze({ + type: 'stdio', + command: 'agentguard-mcp', + args: [] as string[], + timeout: 300, + enabled: true, + description: 'GoPlus AgentGuard MCP — security scanner + action evaluator', +}); +/** + * Merge AgentGuard into Goose's config.yaml using a real YAML parser + * (eemeli/yaml) so existing comments, custom indentation, quoted keys, + * tabs-as-indent, and unrelated top-level keys all survive the round-trip. + * + * Behavior: + * - No file / empty file → emit a minimal document with the AgentGuard entry. + * - `extensions:` exists as a mapping → set/replace `extensions.agentguard`. + * - `extensions:` exists as a non-mapping (scalar, null, sequence) → fail + * loudly rather than silently overwriting unrelated user data. + * - `agentguard` already present and `!force` → no-op (return existing). + * - `agentguard` already present and `force` → replace the entry only; + * sibling extensions (slack, github, …) are left alone. + */ function mergeGooseAgentGuardExtension(existing: string, force: boolean): string { const text = existing.replace(/\r\n/g, '\n'); - // Already configured — keep as-is unless force is set. - if (gooseHasAgentGuardEntry(text) && !force) return existing; - - // If force is set and there's a previous entry, strip it first. - const baseline = force ? stripGooseAgentGuardEntry(text) : text; - - // No `extensions:` block at all — prepend a fresh one (preserve trailing text). - if (!gooseHasTopLevelKey(baseline, 'extensions')) { - const prefix = `extensions:\n${GOOSE_AGENTGUARD_BLOCK}\n`; - if (baseline.trim().length === 0) return prefix; - return baseline.endsWith('\n') ? `${prefix}${baseline}` : `${prefix}${baseline}\n`; + // Empty / whitespace-only → fresh document. + if (text.trim().length === 0) { + const doc = new YAML.Document({ + extensions: { agentguard: { ...GOOSE_AGENTGUARD_ENTRY } }, + }); + return doc.toString(); } - // `extensions:` exists — append `agentguard:` as a child block. - return appendInsideGooseExtensions(baseline); -} - -function gooseHasAgentGuardEntry(text: string): boolean { - // Match `agentguard:` under an `extensions:` top-level block. - const lines = text.split('\n'); - let inExtensions = false; - for (const line of lines) { - if (/^extensions:\s*(?:#.*)?$/.test(line)) { - inExtensions = true; - continue; - } - if (inExtensions && /^[A-Za-z0-9_-]+:/.test(line)) { - // Hit a new top-level key — extensions block ended. - inExtensions = false; - } - if (inExtensions && /^\s+agentguard:\s*(?:#.*)?$/.test(line)) return true; + let doc: YAML.Document.Parsed; + try { + doc = YAML.parseDocument(text, { keepSourceTokens: true }); + } catch (err) { + throw new Error( + `Refusing to modify Goose config: existing YAML is unparseable (${err instanceof Error ? err.message : 'unknown error'}). Fix the file or move it aside, then re-run.` + ); + } + if (doc.errors.length > 0) { + throw new Error( + `Refusing to modify Goose config: YAML parse errors present (${doc.errors[0].message}). Fix the file or move it aside, then re-run.` + ); } - return false; -} -function gooseHasTopLevelKey(text: string, key: string): boolean { - const re = new RegExp(`^${key}:\\s*(?:\\{\\}\\s*)?(?:#.*)?$`, 'm'); - return re.test(text); -} + const extensions = doc.get('extensions', true); + const hasExtensionsKey = doc.has('extensions'); -function appendInsideGooseExtensions(text: string): string { - // Insert the AgentGuard block directly after the `extensions:` line. YAML - // doesn't care about sibling order so we don't need to walk to the end of - // the existing block — that walk is what introduced the trailing-blank - // bug. Children of `extensions:` are indented (2 spaces), so the new - // block slots in cleanly. - const lines = text.split('\n'); - const out: string[] = []; - let inserted = false; + if (!hasExtensionsKey || extensions === null || extensions === undefined) { + // Add a fresh extensions mapping with just AgentGuard. setIn handles the + // missing-path case by materializing intermediate maps. + doc.setIn(['extensions', 'agentguard'], { ...GOOSE_AGENTGUARD_ENTRY }); + return doc.toString(); + } - for (let i = 0; i < lines.length; i++) { - out.push(lines[i]); - if (!inserted && /^extensions:\s*(?:#.*)?$/.test(lines[i])) { - out.push(GOOSE_AGENTGUARD_BLOCK); - inserted = true; - } + if (!YAML.isMap(extensions)) { + throw new Error( + `Refusing to modify Goose config: top-level "extensions" is ${describeYamlNode(extensions)}, expected a mapping. Convert it to a mapping (\`extensions:\\n name: ...\`) or move the file aside.` + ); } - if (!inserted) { - // Defensive: extensions: existed but was malformed; append at end. - out.push('extensions:', GOOSE_AGENTGUARD_BLOCK); + const existingAgentGuard = extensions.get('agentguard', true); + if (existingAgentGuard !== undefined && existingAgentGuard !== null && !force) { + // Already configured — leave the file (including any comments on the + // agentguard entry) byte-identical. + return existing; } - const joined = out.join('\n'); - return joined.endsWith('\n') ? joined : `${joined}\n`; + // Set or replace just the agentguard entry, preserving siblings. + doc.setIn(['extensions', 'agentguard'], { ...GOOSE_AGENTGUARD_ENTRY }); + return doc.toString(); } -function stripGooseAgentGuardEntry(text: string): string { - const lines = text.split('\n'); - const out: string[] = []; - let i = 0; - while (i < lines.length) { - if (/^\s+agentguard:\s*(?:#.*)?$/.test(lines[i])) { - i++; - while (i < lines.length && /^\s{4,}/.test(lines[i])) i++; - continue; - } - out.push(lines[i]); - i++; - } - return out.join('\n'); +function describeYamlNode(node: unknown): string { + if (YAML.isSeq(node)) return 'a sequence'; + if (YAML.isScalar(node)) return `the scalar value ${JSON.stringify((node as YAML.Scalar).value)}`; + if (node === null) return 'null'; + return typeof node; } function writeIfAllowed(path: string, content: string, force: boolean): void { diff --git a/src/tests/installer.test.ts b/src/tests/installer.test.ts index 7ba53a9..e85273f 100644 --- a/src/tests/installer.test.ts +++ b/src/tests/installer.test.ts @@ -385,4 +385,176 @@ describe('Agent template installers', () => { assert.ok(/advisory/i.test(readme)); assert.ok(/not a (?:hard )?(?:gate|security boundary)/i.test(readme)); }); + + it('preserves comments, inline comments, and trailing top-level keys around Goose extensions', () => { + // Lock in the HIGH-severity review fix: a real YAML parser must round-trip + // comments and unrelated top-level keys instead of clobbering them. + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-comments-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + configPath, + [ + '# Goose user config — handle with care.', + 'OPENAI_API_KEY: sk-redacted', + 'GOOSE_MODE: auto # auto-confirm tool calls', + '', + '# Existing MCP servers configured by user', + 'extensions:', + ' slack:', + ' type: stdio', + ' command: uvx', + ' args: [mcp_slack]', + ' enabled: true', + ' github:', + ' type: sse', + ' serverUrl: https://mcp.example.com/github', + ' enabled: false', + '', + '# Trailing keys after extensions: must survive too', + 'GOOSE_PROVIDER: openai', + 'SECURITY_PROMPT_ENABLED: true', + '', + ].join('\n') + ); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + installAgentTemplates('goose'); + const merged = readFileSync(configPath, 'utf8'); + // Comments + assert.ok(merged.includes('# Goose user config — handle with care.')); + assert.ok(merged.includes('# Existing MCP servers configured by user')); + assert.ok(merged.includes('# Trailing keys after extensions: must survive too')); + // Unrelated top-level keys before and after extensions: + assert.ok(merged.includes('OPENAI_API_KEY: sk-redacted')); + assert.ok(merged.includes('GOOSE_MODE: auto')); + assert.ok(merged.includes('GOOSE_PROVIDER: openai')); + assert.ok(merged.includes('SECURITY_PROMPT_ENABLED: true')); + // Sibling extensions kept and AgentGuard appended + assert.ok(merged.includes('slack:')); + assert.ok(merged.includes('github:')); + assert.ok(merged.includes('agentguard:')); + assert.ok(merged.includes('command: agentguard-mcp')); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it('refuses to clobber an extensions: that is a sequence rather than a mapping', () => { + // Defensive: a malformed config where `extensions:` is a list should not + // be silently overwritten. The installer must error so the user fixes it. + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-malformed-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + writeFileSync(configPath, 'extensions:\n - slack\n - github\n'); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + assert.throws(() => installAgentTemplates('goose'), /expected a mapping/i); + // File must be untouched. + assert.equal(readFileSync(configPath, 'utf8'), 'extensions:\n - slack\n - github\n'); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it('refuses to modify an unparseable Goose config file', () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-unparseable-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + // Tabs as indent inside a block scalar — unambiguously a YAML error. + writeFileSync(configPath, 'extensions:\n\t- bad\n good: ok\n'); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + const originalSource = readFileSync(configPath, 'utf8'); + try { + assert.throws(() => installAgentTemplates('goose'), /YAML|unparseable|parse/i); + assert.equal(readFileSync(configPath, 'utf8'), originalSource); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it('handles quoted keys and 4-space-indented Goose extensions blocks', () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-quoted-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + configPath, + [ + '"extensions":', + ' "my-extension":', + ' type: stdio', + ' command: my-mcp', + ' enabled: true', + '', + ].join('\n') + ); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + installAgentTemplates('goose'); + const merged = readFileSync(configPath, 'utf8'); + // The quoted-key + 4-space-indent variant is valid YAML; both + // existing and new entries must remain after the round-trip. + assert.ok(merged.includes('my-extension')); + assert.ok(merged.includes('agentguard')); + assert.ok(merged.includes('command: agentguard-mcp')); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); + + it("forced install of Goose replaces only the agentguard entry, leaving siblings alone", () => { + const dir = mkdtempSync(join(tmpdir(), 'agentguard-goose-force-')); + const configDir = join(dir, '.config', 'goose'); + const configPath = join(configDir, 'config.yaml'); + mkdirSync(configDir, { recursive: true }); + writeFileSync( + configPath, + [ + 'extensions:', + ' slack:', + ' type: stdio', + ' command: uvx', + ' args: [mcp_slack]', + ' enabled: true', + ' agentguard:', + ' type: stdio', + ' command: /opt/old/agentguard-mcp-v0', + ' args: []', + ' enabled: false', + '', + ].join('\n') + ); + + const prev = process.env.GOOSE_CONFIG_DIR; + process.env.GOOSE_CONFIG_DIR = configDir; + try { + installAgentTemplates('goose', { force: true }); + const merged = readFileSync(configPath, 'utf8'); + // Sibling untouched + assert.ok(merged.includes('slack:')); + assert.ok(merged.includes('command: uvx')); + // AgentGuard replaced with canonical entry + assert.ok(merged.includes('command: agentguard-mcp')); + assert.ok(!merged.includes('/opt/old/agentguard-mcp-v0')); + } finally { + if (prev === undefined) delete process.env.GOOSE_CONFIG_DIR; + else process.env.GOOSE_CONFIG_DIR = prev; + } + }); });