Merge pull request #2043 from dyoshikawa/resolve-scrap-issue-2000-amp-permissions

feat(amp): emit amp.permissions for allow/ask/argument-specific deny

Author: dyoshikawa

docs/reference/file-formats.md

@@ -885,4 +885,6 @@ For Vibe (mistral-vibe), this generates per-tool `[tools.<tool>]` tables in the
 
 For Takt, this generates the `default_permission_mode` under `provider_profiles.<provider>` 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: <pattern> } }`, 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.

skills/rulesync/file-formats.md

@@ -885,4 +885,6 @@ For Vibe (mistral-vibe), this generates per-tool `[tools.<tool>]` tables in the
 
 For Takt, this generates the `default_permission_mode` under `provider_profiles.<provider>` 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: <pattern> } }`, 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.

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<ask<allow per tool", async () => {
+      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<ask<allow then cmd),
+      // and the catch-all allow comes last.
+      expect(json["amp.permissions"]).toEqual([
+        { tool: "bash", action: "reject", matches: { cmd: "rm *" } },
+        { tool: "bash", action: "ask", matches: { cmd: "sudo *" } },
+        { tool: "bash", action: "allow", matches: { cmd: "git *" } },
+        { tool: "bash", action: "allow" },
+      ]);
+    });
+
+    it("emits every reject before any allow so a glob-tool allow cannot shadow a specific reject", async () => {
+      // `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);
+    });
   });
 });