Merge pull request #2291 from dyoshikawa/resolve-scrap-issue-2283-codex-rw-merge

fix: merge per-path Codex filesystem rules across read/edit/write categories

Author: dyoshikawa

src/features/permissions/codexcli-permissions.test.ts

@@ -64,6 +64,75 @@ describe("CodexcliPermissions", () => {
     expect(fileContent).toContain('"example.com" = "deny"');
   });
 
+  it("should merge per-path rules across read/edit/write categories instead of last-category-wins", async () => {
+    const logger = createMockLogger();
+    const rulesyncPermissions = new RulesyncPermissions({
+      outputRoot: testDir,
+      relativeDirPath: ".rulesync",
+      relativeFilePath: "permissions.json",
+      fileContent: JSON.stringify({
+        permission: {
+          read: {
+            "/data/readable/**": "allow",
+            "/data/full/**": "allow",
+            "/data/blocked/**": "deny",
+            "/data/asked/**": "allow",
+          },
+          write: {
+            // read allow + write deny → "read" (this was the last-wins bug).
+            "/data/readable/**": "deny",
+            // read allow + write allow → "write".
+            "/data/full/**": "allow",
+            // read deny + write allow → contradiction, warn + "deny".
+            "/data/blocked/**": "allow",
+            // read allow + write ask → "read" (ask maps to the deny side).
+            "/data/asked/**": "ask",
+          },
+        },
+      }),
+    });
+
+    const codexPermissions = await CodexcliPermissions.fromRulesyncPermissions({
+      outputRoot: testDir,
+      rulesyncPermissions,
+      logger,
+    });
+
+    const fileContent = codexPermissions.getFileContent();
+    expect(fileContent).toContain('"/data/readable/**" = "read"');
+    expect(fileContent).toContain('"/data/full/**" = "write"');
+    expect(fileContent).toContain('"/data/blocked/**" = "deny"');
+    expect(fileContent).toContain('"/data/asked/**" = "read"');
+    expect(logger.warn).toHaveBeenCalledWith(
+      expect.stringContaining('Codex CLI cannot express "writable but not readable"'),
+    );
+  });
+
+  it("should collapse edit and write for the same path with the more restrictive action winning", async () => {
+    const logger = createMockLogger();
+    const rulesyncPermissions = new RulesyncPermissions({
+      outputRoot: testDir,
+      relativeDirPath: ".rulesync",
+      relativeFilePath: "permissions.json",
+      fileContent: JSON.stringify({
+        permission: {
+          edit: { "/data/mixed/**": "allow", "/data/open/**": "allow" },
+          write: { "/data/mixed/**": "deny" },
+        },
+      }),
+    });
+
+    const codexPermissions = await CodexcliPermissions.fromRulesyncPermissions({
+      outputRoot: testDir,
+      rulesyncPermissions,
+      logger,
+    });
+
+    const fileContent = codexPermissions.getFileContent();
+    expect(fileContent).toContain('"/data/mixed/**" = "deny"');
+    expect(fileContent).toContain('"/data/open/**" = "write"');
+  });
+
   it("should preserve unmanaged config.toml params on regeneration", async () => {
     const logger = createMockLogger();
     const codexDir = join(testDir, ".codex");

src/features/permissions/codexcli-permissions.ts

@@ -328,18 +328,13 @@ function convertRulesyncToCodexProfile({
   const workspaceRootFilesystem: CodexFilesystemRuleTable = {};
   const domains: Record<string, "allow" | "deny"> = {};
 
+  const filesystemCategoryRules: Partial<
+    Record<"read" | "edit" | "write", Record<string, PermissionAction>>
+  > = {};
+
   for (const [toolName, rules] of Object.entries(config.permission)) {
     if (toolName === "read" || toolName === "edit" || toolName === "write") {
-      const mapAction = toolName === "read" ? mapReadAction : mapWriteAction;
-      for (const [pattern, action] of Object.entries(rules)) {
-        addFilesystemRule({
-          filesystem,
-          workspaceRootFilesystem,
-          pattern,
-          access: mapAction(action),
-          logger,
-        });
-      }
+      filesystemCategoryRules[toolName] = rules;
       continue;
     }
 
@@ -353,6 +348,23 @@ function convertRulesyncToCodexProfile({
     );
   }
 
+  // Codex has a single access level per path (deny < read < write), so rules
+  // for the same pattern across the canonical read/edit/write categories are
+  // merged instead of last-category-wins overwriting (e.g. `read: allow` +
+  // `write: deny` emits `"read"`, not `"deny"`).
+  for (const [pattern, access] of mergeFilesystemCategoryRules({
+    categoryRules: filesystemCategoryRules,
+    logger,
+  })) {
+    addFilesystemRule({
+      filesystem,
+      workspaceRootFilesystem,
+      pattern,
+      access,
+      logger,
+    });
+  }
+
   applyDefaultGitWriteRules({ config, filesystem, workspaceRootFilesystem });
 
   if (Object.keys(workspaceRootFilesystem).length > 0) {
@@ -956,6 +968,94 @@ function mapWriteAction(action: PermissionAction): "write" | "deny" {
   return action === "allow" ? "write" : "deny";
 }
 
+/**
+ * Merge the canonical read/edit/write category rules into one Codex access
+ * level per path pattern (Codex models a single `deny` < `read` < `write`
+ * level, with no `ask`).
+ *
+ * - `edit` and `write` collapse onto Codex's write side; when both carry the
+ *   same pattern, the more restrictive action wins (`deny` > `ask` > `allow`).
+ * - `read: allow` + write-side `allow` → `"write"`.
+ * - `read: allow` + write-side non-allow → `"read"` (readable but not
+ *   writable — exactly what Codex's `"read"` level expresses).
+ * - `read` non-allow → `"deny"` regardless of the write side; a contradictory
+ *   write-side `allow` (unreadable but writable is not expressible in Codex)
+ *   is warned about.
+ * - Single-category patterns keep the existing mapReadAction/mapWriteAction
+ *   mappings.
+ *
+ * Iteration order is read → edit → write with first-seen pattern order, so
+ * the emitted table is stable regardless of the authored category order.
+ * Note the merge is one-way: `"{path}" = "read"` imports back as
+ * `read: allow` only (the explicit write-side deny is implied by Codex's
+ * access level and not re-materialized).
+ */
+function mergeFilesystemCategoryRules({
+  categoryRules,
+  logger,
+}: {
+  categoryRules: Partial<Record<"read" | "edit" | "write", Record<string, PermissionAction>>>;
+  logger?: ToolPermissionsFromRulesyncPermissionsParams["logger"];
+}): Array<[string, "read" | "write" | "deny"]> {
+  const readRules = categoryRules.read ?? {};
+  const writeSideRestrictiveness: Record<PermissionAction, number> = {
+    deny: 2,
+    ask: 1,
+    allow: 0,
+  };
+
+  // Collapse edit/write onto Codex's single write side, restrictive-wins.
+  const writeSideRules: Record<string, PermissionAction> = {};
+  for (const category of ["edit", "write"] as const) {
+    for (const [pattern, action] of Object.entries(categoryRules[category] ?? {})) {
+      const existing = writeSideRules[pattern];
+      if (
+        existing === undefined ||
+        writeSideRestrictiveness[action] > writeSideRestrictiveness[existing]
+      ) {
+        writeSideRules[pattern] = action;
+      }
+    }
+  }
+
+  const patterns: string[] = [];
+  const seen = new Set<string>();
+  for (const pattern of [...Object.keys(readRules), ...Object.keys(writeSideRules)]) {
+    if (!seen.has(pattern)) {
+      seen.add(pattern);
+      patterns.push(pattern);
+    }
+  }
+
+  const merged: Array<[string, "read" | "write" | "deny"]> = [];
+  for (const pattern of patterns) {
+    const readAction = readRules[pattern];
+    const writeAction = writeSideRules[pattern];
+
+    if (readAction === undefined) {
+      merged.push([pattern, mapWriteAction(writeAction as PermissionAction)]);
+      continue;
+    }
+    if (writeAction === undefined) {
+      merged.push([pattern, mapReadAction(readAction)]);
+      continue;
+    }
+
+    if (readAction === "allow") {
+      merged.push([pattern, writeAction === "allow" ? "write" : "read"]);
+      continue;
+    }
+
+    if (writeAction === "allow") {
+      logger?.warn(
+        `Codex CLI cannot express "writable but not readable": pattern "${pattern}" has read: ${readAction} and a write-side allow. Emitting "deny".`,
+      );
+    }
+    merged.push([pattern, "deny"]);
+  }
+  return merged;
+}
+
 function buildCodexBashRulesContent(config: PermissionsConfig): string {
   const bashRules = config.permission.bash ?? {};
   const entries = Object.entries(bashRules);