diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index 1c665e668..5d9eede19 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -885,4 +885,6 @@ For Vibe (mistral-vibe), this generates per-tool `[tools.]` tables in the For Takt, this generates the `default_permission_mode` under `provider_profiles.` in the shared `.takt/config.yaml` (project mode) or `~/.takt/config.yaml` (global mode). Takt does not have per-tool / per-pattern rules; tool gating is a single coarse mode per provider profile, ordered `readonly` < `edit` < `full` (`readonly` may only read, `edit` may also edit/write files, `full` may also run shell commands). The active provider is named by the top-level `provider:` key (defaulting to `claude`). The mapping is therefore **lossy**: on generate, a single mode is derived with this precedence — (1) any `deny` rule anywhere ⇒ `readonly` (conservative — keep the narrowest mode whenever the user expressed any restriction); (2) else any `edit`/`write` category `allow` rule ⇒ `edit`; (3) else any `bash` category `allow` rule ⇒ `full`; (4) else ⇒ `readonly` (safe default). On import, `full` ⇄ `bash: { "*": "allow" }`, `edit` ⇄ `edit: { "*": "allow" }`, and `readonly` (or an unset/unknown mode) ⇄ `bash: { "*": "deny" }`. `config.yaml` is shared with other Takt settings, so the mode is merged in place — the active provider's other keys (e.g. `step_permission_overrides`), every other provider profile, and all other top-level keys are preserved — and the file is never deleted. See the [Takt configuration docs](https://github.com/nrslib/takt/blob/main/docs/configuration.md). +For Amp, this writes to the shared `.amp/settings.json` (project mode) or `~/.config/amp/settings.json` (global mode), using **two** permission surfaces. In rulesync's canonical model the category name **is** the Amp tool name. A **whole-tool deny** (pattern `*`) is written to the bare `amp.tools.disable` array (the tool name is pushed verbatim, preserving `builtin:` prefixes and the `*` glob) for backwards compatibility. Every **lossy** case is written to the ordered `amp.permissions` array instead of being dropped: an **argument-specific deny** (pattern `!== "*"`) becomes `{ tool, action: "reject", matches: { cmd: } }`, and every `allow` / `ask` rule becomes `{ tool, action, matches?: { cmd } }` (the `matches` object is omitted for the `*` catch-all). Amp evaluates `amp.permissions` **first-match-wins**, so generated entries are ordered deterministically and fail-closed: sorted by tool name, then entries **with** `matches.cmd` (more specific) before catch-alls, then by action priority **`reject` < `ask` < `allow`**, then by `cmd`. `amp.permissions` is Amp's documented **legacy / backwards-compatibility** surface — it remains functional and is the only place to express `allow`/`ask` and argument-specific `reject` rules. **Ownership:** rulesync OWNS and wholesale-replaces the `allow`/`ask`/`reject` entries on every generate, but **preserves any existing `action: "delegate"` entry** (rulesync's canonical model has no `delegate` equivalent); preserved `delegate` entries are placed **after** the rulesync-generated entries (so the regenerated rules take precedence under first-match-wins). On **import**, both keys are read and merged into one canonical config: `amp.tools.disable[tool]` → `{ tool: { "*": "deny" } }`, and each `amp.permissions` entry → `{ tool: { (matches?.cmd ?? "*"): mapped } }` (`reject` → `deny`, `allow` → `allow`, `ask` → `ask`; `delegate` is skipped). When both sources target the same tool+pattern, the **most restrictive action wins** (`deny` > `ask` > `allow`). The settings file is shared with the MCP feature (`amp.mcpServers`), so all other keys are preserved on round-trip and the file is never deleted. Tool names and `cmd` patterns that are prototype-pollution keys (`__proto__`, `constructor`, `prototype`) are skipped defensively. See the [Amp manual](https://ampcode.com/manual). + > **Note: Interaction with ignore feature.** Both the ignore feature and the permissions feature can manage `Read` tool deny entries in `.claude/settings.json`. When both features configure the `Read` tool, the **permissions feature takes precedence** and a warning is emitted. If you only need to restrict file reads based on glob patterns, use the ignore feature (`.rulesync/.aiignore`). Use permissions only when you need fine-grained `allow`/`ask`/`deny` control over the `Read` tool. diff --git a/skills/rulesync/file-formats.md b/skills/rulesync/file-formats.md index 1c665e668..5d9eede19 100644 --- a/skills/rulesync/file-formats.md +++ b/skills/rulesync/file-formats.md @@ -885,4 +885,6 @@ For Vibe (mistral-vibe), this generates per-tool `[tools.]` tables in the For Takt, this generates the `default_permission_mode` under `provider_profiles.` in the shared `.takt/config.yaml` (project mode) or `~/.takt/config.yaml` (global mode). Takt does not have per-tool / per-pattern rules; tool gating is a single coarse mode per provider profile, ordered `readonly` < `edit` < `full` (`readonly` may only read, `edit` may also edit/write files, `full` may also run shell commands). The active provider is named by the top-level `provider:` key (defaulting to `claude`). The mapping is therefore **lossy**: on generate, a single mode is derived with this precedence — (1) any `deny` rule anywhere ⇒ `readonly` (conservative — keep the narrowest mode whenever the user expressed any restriction); (2) else any `edit`/`write` category `allow` rule ⇒ `edit`; (3) else any `bash` category `allow` rule ⇒ `full`; (4) else ⇒ `readonly` (safe default). On import, `full` ⇄ `bash: { "*": "allow" }`, `edit` ⇄ `edit: { "*": "allow" }`, and `readonly` (or an unset/unknown mode) ⇄ `bash: { "*": "deny" }`. `config.yaml` is shared with other Takt settings, so the mode is merged in place — the active provider's other keys (e.g. `step_permission_overrides`), every other provider profile, and all other top-level keys are preserved — and the file is never deleted. See the [Takt configuration docs](https://github.com/nrslib/takt/blob/main/docs/configuration.md). +For Amp, this writes to the shared `.amp/settings.json` (project mode) or `~/.config/amp/settings.json` (global mode), using **two** permission surfaces. In rulesync's canonical model the category name **is** the Amp tool name. A **whole-tool deny** (pattern `*`) is written to the bare `amp.tools.disable` array (the tool name is pushed verbatim, preserving `builtin:` prefixes and the `*` glob) for backwards compatibility. Every **lossy** case is written to the ordered `amp.permissions` array instead of being dropped: an **argument-specific deny** (pattern `!== "*"`) becomes `{ tool, action: "reject", matches: { cmd: } }`, and every `allow` / `ask` rule becomes `{ tool, action, matches?: { cmd } }` (the `matches` object is omitted for the `*` catch-all). Amp evaluates `amp.permissions` **first-match-wins**, so generated entries are ordered deterministically and fail-closed: sorted by tool name, then entries **with** `matches.cmd` (more specific) before catch-alls, then by action priority **`reject` < `ask` < `allow`**, then by `cmd`. `amp.permissions` is Amp's documented **legacy / backwards-compatibility** surface — it remains functional and is the only place to express `allow`/`ask` and argument-specific `reject` rules. **Ownership:** rulesync OWNS and wholesale-replaces the `allow`/`ask`/`reject` entries on every generate, but **preserves any existing `action: "delegate"` entry** (rulesync's canonical model has no `delegate` equivalent); preserved `delegate` entries are placed **after** the rulesync-generated entries (so the regenerated rules take precedence under first-match-wins). On **import**, both keys are read and merged into one canonical config: `amp.tools.disable[tool]` → `{ tool: { "*": "deny" } }`, and each `amp.permissions` entry → `{ tool: { (matches?.cmd ?? "*"): mapped } }` (`reject` → `deny`, `allow` → `allow`, `ask` → `ask`; `delegate` is skipped). When both sources target the same tool+pattern, the **most restrictive action wins** (`deny` > `ask` > `allow`). The settings file is shared with the MCP feature (`amp.mcpServers`), so all other keys are preserved on round-trip and the file is never deleted. Tool names and `cmd` patterns that are prototype-pollution keys (`__proto__`, `constructor`, `prototype`) are skipped defensively. See the [Amp manual](https://ampcode.com/manual). + > **Note: Interaction with ignore feature.** Both the ignore feature and the permissions feature can manage `Read` tool deny entries in `.claude/settings.json`. When both features configure the `Read` tool, the **permissions feature takes precedence** and a warning is emitted. If you only need to restrict file reads based on glob patterns, use the ignore feature (`.rulesync/.aiignore`). Use permissions only when you need fine-grained `allow`/`ask`/`deny` control over the `Read` tool. diff --git a/src/features/permissions/amp-permissions.test.ts b/src/features/permissions/amp-permissions.test.ts index 3c2e11066..4f8af8161 100644 --- a/src/features/permissions/amp-permissions.test.ts +++ b/src/features/permissions/amp-permissions.test.ts @@ -47,7 +47,7 @@ describe("AmpPermissions", () => { }); describe("fromRulesyncPermissions", () => { - it("maps deny rules to amp.tools.disable, skipping allow/ask", async () => { + it("keeps whole-tool deny in amp.tools.disable and emits allow/ask as amp.permissions", async () => { const rulesyncPermissions = makeRulesyncPermissions(testDir, { edit_file: { "*": "deny" }, read_file: { "*": "allow" }, @@ -60,7 +60,80 @@ describe("AmpPermissions", () => { }); const json = JSON.parse(instance.getFileContent()); + // Whole-tool deny stays on the legacy disable surface. expect(json["amp.tools.disable"]).toEqual(["edit_file"]); + // allow/ask are no longer dropped: they become amp.permissions entries. + // Ordering is globally fail-closed (ask before allow). + expect(json["amp.permissions"]).toEqual([ + { tool: "web", action: "ask" }, + { tool: "read_file", action: "allow" }, + ]); + }); + + it("emits an argument-specific deny as a reject entry with matches.cmd", async () => { + const rulesyncPermissions = makeRulesyncPermissions(testDir, { + bash: { "*": "deny", "git *": "deny" }, + }); + + const instance = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + const json = JSON.parse(instance.getFileContent()); + + // The whole-tool deny stays in disable; the argument-specific deny becomes reject. + expect(json["amp.tools.disable"]).toEqual(["bash"]); + expect(json["amp.permissions"]).toEqual([ + { tool: "bash", action: "reject", matches: { cmd: "git *" } }, + ]); + }); + + it("orders amp.permissions specific-before-catch-all and reject { + const rulesyncPermissions = makeRulesyncPermissions(testDir, { + bash: { + "*": "allow", + "rm *": "deny", + "sudo *": "ask", + "git *": "allow", + }, + }); + + const instance = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + const json = JSON.parse(instance.getFileContent()); + + expect(json["amp.tools.disable"]).toEqual([]); + // Entries with matches.cmd come first (sorted reject { + // `mcp__*` is a glob tool whose catch-all allow would, under Amp's + // first-match-wins, shadow the specific `mcp__github` reject if emitted + // first. Global fail-closed ordering puts all rejects ahead. + const rulesyncPermissions = makeRulesyncPermissions(testDir, { + "mcp__*": { "*": "allow" }, + mcp__github: { "deploy *": "deny" }, + }); + + const instance = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + const json = JSON.parse(instance.getFileContent()); + + expect(json["amp.permissions"]).toEqual([ + { tool: "mcp__github", action: "reject", matches: { cmd: "deploy *" } }, + { tool: "mcp__*", action: "allow" }, + ]); }); it("preserves builtin: prefixes and the * glob verbatim, sorted and deduped", async () => { @@ -111,6 +184,56 @@ describe("AmpPermissions", () => { expect(instance.getRelativeFilePath()).toBe("settings.jsonc"); }); + + it("preserves a pre-existing delegate entry, placing it after generated entries", async () => { + await writeFileContent( + join(testDir, ".amp", "settings.json"), + JSON.stringify({ + "amp.permissions": [ + { tool: "bash", action: "delegate", matches: { cmd: "deploy *" } }, + // A user-authored allow that rulesync owns and should regenerate (wholesale-replace). + { tool: "bash", action: "allow", matches: { cmd: "stale *" } }, + ], + }), + ); + const rulesyncPermissions = makeRulesyncPermissions(testDir, { + bash: { "git *": "allow" }, + }); + + const instance = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + const json = JSON.parse(instance.getFileContent()); + + expect(json["amp.permissions"]).toEqual([ + // Regenerated rulesync entry first. + { tool: "bash", action: "allow", matches: { cmd: "git *" } }, + // Pre-existing delegate survives, placed after generated entries. + { tool: "bash", action: "delegate", matches: { cmd: "deploy *" } }, + ]); + }); + + it("removes amp.permissions when nothing is generated and no delegate is preserved", async () => { + await writeFileContent( + join(testDir, ".amp", "settings.json"), + JSON.stringify({ + "amp.permissions": [{ tool: "bash", action: "allow", matches: { cmd: "old *" } }], + }), + ); + const rulesyncPermissions = makeRulesyncPermissions(testDir, { + edit_file: { "*": "deny" }, + }); + + const instance = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + const json = JSON.parse(instance.getFileContent()); + + expect(json["amp.tools.disable"]).toEqual(["edit_file"]); + expect("amp.permissions" in json).toBe(false); + }); }); describe("fromFile", () => { @@ -140,6 +263,90 @@ describe("AmpPermissions", () => { expect(config.permission["builtin:Bash"]).toEqual({ "*": "deny" }); expect(config.permission["*"]).toEqual({ "*": "deny" }); }); + + it("imports amp.permissions entries back into canonical actions", async () => { + await writeFileContent( + join(testDir, ".amp", "settings.json"), + JSON.stringify({ + "amp.permissions": [ + { tool: "read_file", action: "allow" }, + { tool: "web", action: "ask" }, + { tool: "bash", action: "reject", matches: { cmd: "rm *" } }, + { tool: "bash", action: "allow", matches: { cmd: "git *" } }, + ], + }), + ); + + const instance = await AmpPermissions.fromFile({ outputRoot: testDir }); + const config = JSON.parse(instance.toRulesyncPermissions().getFileContent()); + + expect(config.permission.read_file).toEqual({ "*": "allow" }); + expect(config.permission.web).toEqual({ "*": "ask" }); + expect(config.permission.bash).toEqual({ "rm *": "deny", "git *": "allow" }); + }); + + it("skips delegate entries on import (no canonical equivalent)", async () => { + await writeFileContent( + join(testDir, ".amp", "settings.json"), + JSON.stringify({ + "amp.permissions": [ + { tool: "bash", action: "delegate", matches: { cmd: "deploy *" } }, + { tool: "bash", action: "allow", matches: { cmd: "git *" } }, + ], + }), + ); + + const instance = await AmpPermissions.fromFile({ outputRoot: testDir }); + const config = JSON.parse(instance.toRulesyncPermissions().getFileContent()); + + expect(config.permission.bash).toEqual({ "git *": "allow" }); + }); + + it("merges both sources and lets deny/reject win on conflict (fail-closed)", async () => { + await writeFileContent( + join(testDir, ".amp", "settings.json"), + JSON.stringify({ + "amp.tools.disable": ["bash"], + // amp.permissions has a catch-all allow for the same tool+pattern. + "amp.permissions": [{ tool: "bash", action: "allow" }], + }), + ); + + const instance = await AmpPermissions.fromFile({ outputRoot: testDir }); + const config = JSON.parse(instance.toRulesyncPermissions().getFileContent()); + + // disable → bash:{"*":"deny"}; the allow on the same key loses to deny. + expect(config.permission.bash).toEqual({ "*": "deny" }); + }); + }); + + describe("round-trip", () => { + it("round-trips allow/ask/reject and whole-tool deny through Amp and back", async () => { + const original = { + bash: { "*": "deny", "git *": "allow", "rm *": "deny", "sudo *": "ask" }, + read_file: { "*": "allow" }, + web: { "*": "ask" }, + }; + const rulesyncPermissions = makeRulesyncPermissions(testDir, original); + + const exported = await AmpPermissions.fromRulesyncPermissions({ + outputRoot: testDir, + rulesyncPermissions, + }); + // Re-read the generated settings file shape into a fresh instance. + await writeFileContent(join(testDir, ".amp", "settings.json"), exported.getFileContent()); + const reimported = await AmpPermissions.fromFile({ outputRoot: testDir }); + const config = JSON.parse(reimported.toRulesyncPermissions().getFileContent()); + + expect(config.permission.bash).toEqual({ + "*": "deny", + "git *": "allow", + "rm *": "deny", + "sudo *": "ask", + }); + expect(config.permission.read_file).toEqual({ "*": "allow" }); + expect(config.permission.web).toEqual({ "*": "ask" }); + }); }); describe("isDeletable", () => { @@ -186,5 +393,27 @@ describe("AmpPermissions", () => { }); expect(instance.validate().success).toBe(true); }); + + it("rejects a non-array amp.permissions", () => { + const instance = new AmpPermissions({ + outputRoot: testDir, + relativeDirPath: ".amp", + relativeFilePath: "settings.json", + fileContent: JSON.stringify({ "amp.permissions": "nope" }), + }); + expect(instance.validate().success).toBe(false); + }); + + it("accepts a valid amp.permissions array", () => { + const instance = new AmpPermissions({ + outputRoot: testDir, + relativeDirPath: ".amp", + relativeFilePath: "settings.json", + fileContent: JSON.stringify({ + "amp.permissions": [{ tool: "bash", action: "allow" }], + }), + }); + expect(instance.validate().success).toBe(true); + }); }); }); diff --git a/src/features/permissions/amp-permissions.ts b/src/features/permissions/amp-permissions.ts index 09a3ea029..dc4b6dd57 100644 --- a/src/features/permissions/amp-permissions.ts +++ b/src/features/permissions/amp-permissions.ts @@ -13,7 +13,8 @@ import type { AiFileParams, ValidationResult } from "../../types/ai-file.js"; import type { PermissionAction, PermissionsConfig } from "../../types/permissions.js"; import { formatError } from "../../utils/error.js"; import { readFileContentOrNull } from "../../utils/file.js"; -import { isRecord } from "../../utils/type-guards.js"; +import { isPrototypePollutionKey } from "../../utils/prototype-pollution.js"; +import { isPlainObject } from "../../utils/type-guards.js"; import { RulesyncPermissions } from "./rulesync-permissions.js"; import { ToolPermissions, @@ -33,6 +34,38 @@ import { */ const AMP_TOOLS_DISABLE_KEY = "amp.tools.disable"; +/** + * Amp's `"amp.permissions"` setting is an ordered array of per-tool rules with + * argument-level matching. Each entry is + * `{ tool: string, action: "allow" | "reject" | "ask" | "delegate", matches?: { cmd?: string } }`. + * Tool names support globs (`*`, `mcp__*`); `matches.cmd` is a per-argument + * glob. Rules are evaluated **first-match-wins**. It lives in the same shared + * Amp settings file as `amp.tools.disable`. + * + * `amp.permissions` is Amp's documented legacy/backwards-compat surface — it + * remains functional and is the only place to express `allow`/`ask` and + * argument-specific `reject` rules (the simpler `amp.tools.disable` array can + * only disable whole tools). + * + * Reference: https://ampcode.com/manual ("amp.permissions"). + */ +const AMP_PERMISSIONS_KEY = "amp.permissions"; + +/** + * Actions rulesync owns and regenerates wholesale in `amp.permissions`. + * `delegate` is intentionally excluded: it has no canonical equivalent, so any + * existing `delegate` entry is preserved verbatim rather than regenerated. + */ +type AmpManagedAction = "allow" | "reject" | "ask"; +type AmpAction = AmpManagedAction | "delegate"; + +type AmpPermissionEntry = { + tool: string; + action: AmpAction; + matches?: { cmd?: string }; + [key: string]: unknown; +}; + function parseAmpSettings(fileContent: string): Record { const errors: ParseError[] = []; const parsed: unknown = parseJsonc(fileContent || "{}", errors, { allowTrailingComma: true }); @@ -44,7 +77,9 @@ function parseAmpSettings(fileContent: string): Record { throw new Error(`Failed to parse Amp settings: ${details}`); } - if (!isRecord(parsed)) { + // `isPlainObject` (not `isRecord`) rejects class instances for + // prototype-pollution hardening; the JSONC parser always yields a plain object. + if (!isPlainObject(parsed)) { throw new Error("Amp settings must be a JSON object"); } @@ -56,15 +91,53 @@ function toDisableList(value: unknown): string[] { return value.filter((entry): entry is string => typeof entry === "string"); } +/** + * Read an `amp.permissions` array from untrusted parsed settings. Only entries + * whose shape rulesync understands are retained; the original objects are kept + * by reference (with a normalized `action`) so unknown sibling keys survive a + * round-trip when the entry is preserved. + */ +function toPermissionsList(value: unknown): AmpPermissionEntry[] { + if (!Array.isArray(value)) return []; + const entries: AmpPermissionEntry[] = []; + for (const raw of value) { + if (!isPlainObject(raw)) continue; + const { tool, action } = raw; + if (typeof tool !== "string" || typeof action !== "string") continue; + if (action !== "allow" && action !== "reject" && action !== "ask" && action !== "delegate") { + continue; + } + const matches = raw.matches; + let normalizedMatches: { cmd?: string } | undefined; + if (isPlainObject(matches) && typeof matches.cmd === "string") { + normalizedMatches = { cmd: matches.cmd }; + } + entries.push({ + ...raw, + tool, + action, + ...(normalizedMatches ? { matches: normalizedMatches } : {}), + } as AmpPermissionEntry); + } + return entries; +} + /** * Permissions generator for Amp (ampcode). * - * Amp only supports disabling tools (there is no allow/ask surface for the - * tools list), so rulesync `deny` rules are mapped onto the - * `"amp.tools.disable"` array. Each disabled tool name is modeled as a rulesync - * category with a single `{ "*": "deny" }` rule, so glob (`*`) entries and - * `builtin:` prefixes round-trip verbatim. `allow`/`ask` rules are skipped with - * a warning since Amp's tools list cannot represent them. + * Amp exposes two permission surfaces in the same shared settings file: + * + * - `amp.tools.disable` — a bare list of tool names to disable. This can only + * express "disable the whole tool", which maps cleanly to a rulesync + * whole-tool `deny` (`{ "*": "deny" }`). + * - `amp.permissions` — an ordered, first-match-wins array of + * `{ tool, action, matches?: { cmd } }` rules that can express `allow`, + * `reject` (deny), and `ask` with optional per-argument (`cmd`) matching. + * + * Export strategy (resolves issue #2000): a whole-tool `deny` (pattern `*`) + * stays in `amp.tools.disable` for backwards compatibility, while every lossy + * case — argument-specific `deny`, and all `allow`/`ask` rules — is emitted as + * `amp.permissions` entries instead of being silently dropped. * * The settings file is shared with the MCP feature (`amp.mcpServers`), so reads * and writes merge into the existing JSON rather than overwriting it, and the @@ -80,7 +153,8 @@ export class AmpPermissions extends ToolPermissions { /** * The settings file may carry other Amp settings (e.g. `amp.mcpServers`), so - * we never delete it; only the managed `amp.tools.disable` key is rewritten. + * we never delete it; only the managed `amp.tools.disable` / + * `amp.permissions` keys are rewritten. */ override isDeletable(): boolean { return false; @@ -142,7 +216,6 @@ export class AmpPermissions extends ToolPermissions { outputRoot = process.cwd(), rulesyncPermissions, global = false, - logger, }: ToolPermissionsFromRulesyncPermissionsParams): Promise { const basePaths = AmpPermissions.getSettablePaths({ global }); const jsonDir = join(outputRoot, basePaths.relativeDirPath); @@ -151,9 +224,24 @@ export class AmpPermissions extends ToolPermissions { const json = fileContent ? parseAmpSettings(fileContent) : {}; const config = rulesyncPermissions.getJson(); - const disable = convertRulesyncToAmpDisable(config, logger); + const { disable, permissions } = convertRulesyncToAmp(config); + + // Preserve user-authored `delegate` entries (no canonical equivalent), and + // place them AFTER the rulesync-generated entries. Amp is first-match-wins, + // so rulesync's regenerated allow/ask/reject rules take precedence; the + // surviving delegate entries act as later fallbacks. rulesync OWNS and + // wholesale-replaces the allow/ask/reject entries. + const existingPermissions = toPermissionsList(json[AMP_PERMISSIONS_KEY]); + const preservedDelegates = existingPermissions.filter((entry) => entry.action === "delegate"); - const newJson = { ...json, [AMP_TOOLS_DISABLE_KEY]: disable }; + const newJson: Record = { ...json, [AMP_TOOLS_DISABLE_KEY]: disable }; + + const mergedPermissions = [...permissions, ...preservedDelegates]; + if (mergedPermissions.length > 0) { + newJson[AMP_PERMISSIONS_KEY] = mergedPermissions; + } else { + delete newJson[AMP_PERMISSIONS_KEY]; + } return new AmpPermissions({ outputRoot, @@ -166,7 +254,10 @@ export class AmpPermissions extends ToolPermissions { toRulesyncPermissions(): RulesyncPermissions { const json = parseAmpSettings(this.getFileContent()); - const config = convertAmpDisableToRulesync(toDisableList(json[AMP_TOOLS_DISABLE_KEY])); + const config = convertAmpToRulesync({ + disable: toDisableList(json[AMP_TOOLS_DISABLE_KEY]), + permissions: toPermissionsList(json[AMP_PERMISSIONS_KEY]), + }); return this.toRulesyncPermissionsDefault({ fileContent: JSON.stringify(config, null, 2), @@ -183,6 +274,13 @@ export class AmpPermissions extends ToolPermissions { error: new Error(`"${AMP_TOOLS_DISABLE_KEY}" must be a JSON array of strings`), }; } + const permissions = json[AMP_PERMISSIONS_KEY]; + if (permissions !== undefined && !Array.isArray(permissions)) { + return { + success: false, + error: new Error(`"${AMP_PERMISSIONS_KEY}" must be a JSON array of objects`), + }; + } return { success: true, error: null }; } catch (error) { return { @@ -208,47 +306,168 @@ export class AmpPermissions extends ToolPermissions { } /** - * Convert a rulesync permissions config into Amp's `amp.tools.disable` array. + * Fail-closed action priority used to order generated `amp.permissions` entries + * for the same tool. Because Amp is first-match-wins, a more restrictive action + * must appear before a more permissive one: `reject` < `ask` < `allow`. + */ +const ACTION_PRIORITY: Record = { + reject: 0, + ask: 1, + allow: 2, +}; + +/** + * Convert a rulesync permissions config into Amp's two permission surfaces. * - * Each tool category is treated as a tool name; a `deny` rule disables it. The - * category name (including any `builtin:` prefix or `*` glob) is preserved - * verbatim. `allow`/`ask` rules are skipped with a warning since Amp's tools - * list can only express "disabled". + * For each `(category, pattern, action)` — where the category name IS the Amp + * tool name: + * - `deny` + pattern `*` → push `category` onto `amp.tools.disable` (whole-tool + * disable; legacy-compatible). + * - `deny` + pattern `!== "*"` → `amp.permissions` `{ tool, action: "reject", + * matches: { cmd: pattern } }` (argument-specific deny keeps its specificity). + * - `allow` / `ask` → `amp.permissions` `{ tool, action, matches?: { cmd } }` + * (`matches` omitted for the `*` catch-all). + * + * Generated `amp.permissions` entries are ordered deterministically so the + * first-match-wins evaluation is fail-closed and stable: sorted by `tool`, then + * by entries WITH `matches.cmd` (more specific) before catch-alls, then by + * action priority (`reject` < `ask` < `allow`), then by `cmd`. + * + * Prototype-pollution-unsafe tool names / cmd patterns (`__proto__`, + * `constructor`, `prototype`) are skipped defensively — they would otherwise be + * used as object keys on the import side. */ -function convertRulesyncToAmpDisable( - config: PermissionsConfig, - logger?: ToolPermissionsFromRulesyncPermissionsParams["logger"], -): string[] { +function convertRulesyncToAmp(config: PermissionsConfig): { + disable: string[]; + permissions: AmpPermissionEntry[]; +} { const disable: string[] = []; + const permissions: AmpPermissionEntry[] = []; for (const [category, rules] of Object.entries(config.permission)) { + if (isPrototypePollutionKey(category)) continue; for (const [pattern, action] of Object.entries(rules)) { - if (action !== "deny") { - logger?.warn( - `Amp's "${AMP_TOOLS_DISABLE_KEY}" only supports disabling tools (deny). ` + - `Skipping ${action} rule for tool "${category}" (pattern "${pattern}").`, - ); + if (pattern !== "*" && isPrototypePollutionKey(pattern)) continue; + + if (action === "deny") { + if (pattern === "*") { + // Whole-tool disable stays on the legacy `amp.tools.disable` surface. + disable.push(category); + } else { + permissions.push({ + tool: category, + action: "reject", + matches: { cmd: pattern }, + }); + } continue; } - // The tool name is the category; the `*` pattern means "disable the whole - // tool". Any non-`*` pattern is forwarded as-is onto the category name so - // it round-trips, but Amp itself matches on tool name only. - disable.push(category); + + // allow / ask → amp.permissions (omit `matches` for the catch-all). + const ampAction: AmpManagedAction = action === "allow" ? "allow" : "ask"; + permissions.push({ + tool: category, + action: ampAction, + ...(pattern !== "*" ? { matches: { cmd: pattern } } : {}), + }); } } - return uniq(disable.toSorted()); + return { + disable: uniq(disable.toSorted()), + permissions: sortAmpPermissions(permissions), + }; } /** - * Convert Amp's `amp.tools.disable` array back into a rulesync permissions - * config. Each disabled tool name becomes a category with a single - * `{ "*": "deny" }` rule. + * Stable, deterministic ordering for generated `amp.permissions` entries. + * Order: fail-closed action priority (`reject` < `ask` < `allow`) FIRST — so the + * ordering is globally fail-closed even across glob tool names — then specific + * (has `matches.cmd`) before catch-all, then tool name, then `cmd`. */ -function convertAmpDisableToRulesync(disable: string[]): PermissionsConfig { +function sortAmpPermissions(entries: AmpPermissionEntry[]): AmpPermissionEntry[] { + const decorated = entries.map((entry, index) => ({ entry, index })); + decorated.sort((a, b) => { + // Action priority is the PRIMARY key so the ordering is fail-closed + // *globally*, not just within one tool string. Amp tool names can be globs + // (`*`, `mcp__*`), so a catch-all `allow` on a glob tool (e.g. `mcp__*`) + // could otherwise be emitted before — and, under first-match-wins, shadow — + // a more specific `reject` on a concrete tool (e.g. `mcp__github`). Emitting + // every `reject` before every `ask` before every `allow` guarantees no + // allow can shadow a deny regardless of tool-glob matching. + const ap = ACTION_PRIORITY[a.entry.action as AmpManagedAction] ?? 0; + const bp = ACTION_PRIORITY[b.entry.action as AmpManagedAction] ?? 0; + if (ap !== bp) return ap - bp; + + // Within the same action, specific (with `matches.cmd`) before catch-all. + const aHasCmd = a.entry.matches?.cmd !== undefined ? 0 : 1; + const bHasCmd = b.entry.matches?.cmd !== undefined ? 0 : 1; + if (aHasCmd !== bHasCmd) return aHasCmd - bHasCmd; + + const at = a.entry.tool; + const bt = b.entry.tool; + if (at !== bt) return at < bt ? -1 : 1; + + const ac = a.entry.matches?.cmd ?? ""; + const bc = b.entry.matches?.cmd ?? ""; + if (ac !== bc) return ac < bc ? -1 : 1; + + return a.index - b.index; + }); + return decorated.map((d) => d.entry); +} + +/** + * Convert Amp's two permission surfaces back into a rulesync permissions + * config, merging both sources into one canonical model. + * + * - `amp.tools.disable[tool]` → `{ tool: { "*": "deny" } }`. + * - `amp.permissions` entry `{ tool, action, matches }` → + * `{ tool: { (matches?.cmd ?? "*"): mapped } }` where `reject → deny`, + * `allow → allow`, `ask → ask`. `delegate` is skipped (no canonical + * equivalent). + * + * When both sources target the same `(tool, pattern)`, the more restrictive + * action wins (fail-closed): `deny` > `ask` > `allow`. + * + * Tool names and cmd patterns that are prototype-pollution keys are skipped + * defensively before being used as object keys. + */ +function convertAmpToRulesync({ + disable, + permissions, +}: { + disable: string[]; + permissions: AmpPermissionEntry[]; +}): PermissionsConfig { + const actionPriority: Record = { + deny: 2, + ask: 1, + allow: 0, + }; const permission: Record> = {}; + + const assign = (tool: string, pattern: string, action: PermissionAction): void => { + if (isPrototypePollutionKey(tool)) return; + if (isPrototypePollutionKey(pattern)) return; + const bucket = (permission[tool] ??= {}); + const existing = bucket[pattern]; + if (existing === undefined || actionPriority[action] > actionPriority[existing]) { + bucket[pattern] = action; + } + }; + for (const tool of disable) { - permission[tool] = { "*": "deny" }; + assign(tool, "*", "deny"); } + + for (const entry of permissions) { + if (entry.action === "delegate") continue; + const pattern = entry.matches?.cmd ?? "*"; + const action: PermissionAction = + entry.action === "reject" ? "deny" : entry.action === "ask" ? "ask" : "allow"; + assign(entry.tool, pattern, action); + } + return { permission }; }