fix(cli): keep the deployed policy authoritative over a legacy app-file one

Review follow-up. `deployedPolicyBase` spread the app file's policy over the
deployed one, and older pulls wrote the whole policy into that file — so a repo
still carrying `on_behalf_of` (a stale value, or the explicit `null` a pull used
to write) or a legacy `triggerables` map fed both back into the deploy. The
first re-permissioned the app from checked-in content, the second restored
grants for runnables the deploy had removed. `pushRawApp` compounded it by
gating `preserve_on_behalf_of` on the merged policy rather than the deployed
one, which also let a create bypass the folder default.

The run-as identity and every derived grant (`triggerables`, `s3_inputs`,
`allowed_s3_keys`) now come off the deployed policy or not at all, and the raw
path claims an identity only on update, matching the low-code path.

Also honour a file-stated `execution_mode: viewer`, which had no other way to
survive a first deploy, and warn a non-deployer that a raw-app push reassigns
ownership — `preCheckPermissionedAs` covered `app` but not `raw_app`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MaJ9KHDeaWrWUdVsHeHQ8L

cli/src/commands/app/app.ts

@@ -144,31 +144,59 @@ export function executionModeFromAppFile(
   if (app?.["guests"] ?? isExecutionModeGuest(app)) {
     return "guest";
   }
-  // `public`/`guests` are the only modes the file records, so "neither" covers
-  // both publisher and viewer: keep the deployed one rather than demoting a
-  // viewer app to publisher on every push.
-  if (deployedPolicy?.execution_mode === "viewer") {
+  // A pull records the mode as `public`/`guests` and nothing else, so a file
+  // saying neither may still be a viewer app: honour an explicit `viewer`, and
+  // otherwise keep the deployed mode rather than demoting one on every push. A
+  // file stating `publisher` does not demote a deployed viewer — that block is
+  // vestigial (only older pulls wrote it), and viewer is the stricter of the
+  // two, running each viewer's own identity instead of the publisher's.
+  if (
+    app?.["policy"]?.["execution_mode"] === "viewer" ||
+    deployedPolicy?.execution_mode === "viewer"
+  ) {
     return "viewer";
   }
   return "publisher";
 }
 
+/** The policy fields a deploy derives from the app it is deploying. Carrying one
+ * over from either side would leave a grant keyed to sources this deploy just
+ * replaced: legacy `triggerables` are folded into `triggerables_v2` on read, and
+ * `s3_inputs` / `allowed_s3_keys` are what the backend enforces S3 access
+ * against. A low-code deploy recomputes these; a raw one produces none of them.
+ * `triggerables_v2` is not listed because both deploys overwrite it outright. */
+const DERIVED_POLICY_FIELDS = [
+  "triggerables",
+  "s3_inputs",
+  "allowed_s3_keys",
+] as const;
+
 /**
  * What a push's regenerated policy starts from. The app file states only the
  * access mode, so regenerating from the local sources alone silently drops every
- * other policy field the deployed app carries — its run-as identity, sandbox
- * isolation, SDK scopes. Anything the file does state still wins.
+ * other policy field the deployed app carries — its sandbox isolation, SDK
+ * scopes. Anything else the file does state still wins.
  *
- * Legacy `triggerables` are dropped: the backend folds them into
- * `triggerables_v2` on read, so carrying them over would keep granting runnables
- * this deploy no longer contains.
+ * The run-as identity is never taken from the file: the deployed policy owns it,
+ * and a push claims a different one only through `preserve_on_behalf_of`. Older
+ * pulls wrote the whole policy into the app file, so repos still carry
+ * `on_behalf_of` keys — a stale one, or an explicit `null`, would otherwise
+ * re-permission the app from checked-in content.
  */
 export function deployedPolicyBase(
   deployedPolicy: Policy | undefined,
   localPolicy: Policy | undefined,
 ): Policy {
-  const { triggerables: _legacy, ...deployed } = deployedPolicy ?? {};
-  return { ...deployed, ...(localPolicy ?? {}) } as Policy;
+  const {
+    on_behalf_of: _obo,
+    on_behalf_of_email: _oboEmail,
+    ...local
+  } = localPolicy ?? {};
+  const base: Record<string, any> = { ...(deployedPolicy ?? {}), ...local };
+  for (const field of DERIVED_POLICY_FIELDS) {
+    delete base[field];
+  }
+  return base as Policy;
 }
 export async function pushApp(
   workspace: string,

cli/src/commands/app/raw_apps.ts

@@ -447,16 +447,14 @@ export async function pushRawApp(
 
   // Submitting a policy is how the backend reads a claim on the app's execution
   // identity; without the flag it rewrites on_behalf_of to whoever ran the push.
+  // Only ever claims the deployed identity — on create the backend applies the
+  // folder default instead, as on the low-code path.
   const preserveFields: { preserve_on_behalf_of?: boolean } = {};
-  if (
-    permissionedAsContext?.userIsAdminOrDeployer &&
-    appForPolicy.policy.on_behalf_of
-  ) {
+  if (permissionedAsContext?.userIsAdminOrDeployer && remotePolicy?.on_behalf_of) {
     preserveFields.preserve_on_behalf_of = true;
     log.info(
       `Preserving ${
-        appForPolicy.policy.on_behalf_of_email ??
-          appForPolicy.policy.on_behalf_of
+        remotePolicy.on_behalf_of_email ?? remotePolicy.on_behalf_of
       } as permissioned_as for app ${remotePath}`,
     );
   }
@@ -537,7 +535,6 @@ export async function pushRawApp(
           summary: localApp.summary,
           policy: appForPolicy.policy,
           deployment_message: message,
-          ...preserveFields,
           // Preserve any user draft at this path (see backend skip_draft_deletion).
           skip_draft_deletion: true,
           ...(localApp.custom_path

cli/src/core/permissioned_as.ts

@@ -130,7 +130,7 @@ export async function preCheckPermissionedAs(
         const label =
           typeStr === "script" ? "(script owner)" : "(flow owner)";
         wouldChangeItems.push({ path: change.path, currentOwner: label });
-      } else if (typeStr === "app") {
+      } else if (typeStr === "app" || typeStr === "raw_app") {
         wouldChangeItems.push({
           path: change.path,
           currentOwner: "(app policy owner)",
@@ -177,7 +177,7 @@ export async function preCheckPermissionedAs(
         }
       }
       continue;
-    } else if (typeStr === "app") {
+    } else if (typeStr === "app" || typeStr === "raw_app") {
       wouldChangeItems.push({
         path: change.path,
         currentOwner: "(app policy owner)",

cli/test/app_access_mode_unit.test.ts

@@ -1,5 +1,6 @@
 import { expect, test } from "bun:test";
 import {
+  deployedPolicyBase,
   executionModeFromAppFile,
   generatingPolicy,
   markAccessFromPolicy,
@@ -26,3 +27,59 @@ test("the access mode survives the app.yaml round trip", async () => {
   expect(executionModeFromAppFile({ policy: { execution_mode: "publisher" } })).toBe("publisher");
   expect(executionModeFromAppFile({})).toBe("publisher");
 });
+
+// `viewer` has no representation in the app file, so a push that read the mode
+// off the file alone demoted every viewer app to publisher — running the
+// publisher's identity for viewers instead of each viewer's own.
+test("a deployed viewer mode is not demoted by a file that cannot express it", () => {
+  expect(executionModeFromAppFile({}, { execution_mode: "viewer" } as any)).toBe("viewer");
+  expect(executionModeFromAppFile({ policy: { execution_mode: "viewer" } })).toBe("viewer");
+  // Widening still has to come from the file, and dropping `public` still demotes.
+  expect(executionModeFromAppFile({}, { execution_mode: "anonymous" } as any)).toBe("publisher");
+  expect(
+    executionModeFromAppFile({ public: true }, { execution_mode: "viewer" } as any),
+  ).toBe("anonymous");
+});
+
+// A push regenerates the policy, so whatever `deployedPolicyBase` does not carry
+// over is dropped from the deployed app. Older pulls wrote the whole policy into
+// the app file, so repos still ship `on_behalf_of` keys — letting one win would
+// hand the app's execution identity to checked-in content, which is what
+// `preserve_on_behalf_of` exists to prevent.
+test("deployedPolicyBase: the file never sets the run-as identity", () => {
+  const deployed: any = {
+    on_behalf_of: "u/svc",
+    on_behalf_of_email: "svc@corp",
+    sandbox: true,
+  };
+  for (
+    const local of [
+      undefined,
+      { on_behalf_of: null, on_behalf_of_email: null },
+      { on_behalf_of: "u/stale", on_behalf_of_email: "stale@corp" },
+    ] as any[]
+  ) {
+    expect(deployedPolicyBase(deployed, local)).toMatchObject({
+      on_behalf_of: "u/svc",
+      on_behalf_of_email: "svc@corp",
+      sandbox: true,
+    });
+  }
+});
+
+// Grants keyed to the sources a deploy replaces must not survive it, from either
+// side of the merge: the backend folds legacy `triggerables` into
+// `triggerables_v2` on read and enforces S3 access against `s3_inputs`.
+test("deployedPolicyBase: derived grants are never carried over", () => {
+  const stale: any = {
+    triggerables: { "gone:script/f/gone": {} },
+    s3_inputs: [{ allowed_resources: ["u/gone"] }],
+    allowed_s3_keys: [{ s3_path: "gone" }],
+  };
+  for (const [deployed, local] of [[stale, undefined], [{}, stale]] as any[]) {
+    const base = deployedPolicyBase(deployed, local) as any;
+    expect(base.triggerables).toBeUndefined();
+    expect(base.s3_inputs).toBeUndefined();
+    expect(base.allowed_s3_keys).toBeUndefined();
+  }
+});

cli/test/raw_app_sync.test.ts

@@ -812,14 +812,39 @@ syncBehavior: v1`, "utf-8");
       expect(pushResult2.code).toEqual(0);
       await waitForDeploymentJobs(backend);
 
-      const getResp = await backend.apiRequest!(
-        `/api/w/${backend.workspace}/apps/get/p/f/test/policy_app`
-      );
-      expect(getResp.status).toEqual(200);
-      const policy = (await getResp.json()).policy;
+      async function readPolicy() {
+        const resp = await backend.apiRequest!(
+          `/api/w/${backend.workspace}/apps/get/p/f/test/policy_app`
+        );
+        expect(resp.status).toEqual(200);
+        return (await resp.json()).policy;
+      }
+      const policy = await readPolicy();
       expect(policy.on_behalf_of).toEqual("u/svc");
       expect(policy.on_behalf_of_email).toEqual("svc@windmill.dev");
       expect(policy.sandbox).toEqual(true);
       expect(policy.execution_mode).toEqual("viewer");
+
+      // The carry-over must not make the access mode sticky in the widening
+      // direction: `public: true` promotes, and dropping it demotes again.
+      const yamlPath = path.join(appDir, "raw_app.yaml");
+      const baseYaml = await readFileContent(yamlPath);
+      await writeFile(yamlPath, `${baseYaml}public: true\n`, "utf-8");
+      expect((await backend.runCLICommand(
+        ["sync", "push", "--yes"], tempDir, "raw_app_policy_test"
+      )).code).toEqual(0);
+      await waitForDeploymentJobs(backend);
+      expect(await readPolicy()).toMatchObject({ execution_mode: "anonymous" });
+
+      await writeFile(yamlPath, baseYaml, "utf-8");
+      expect((await backend.runCLICommand(
+        ["sync", "push", "--yes"], tempDir, "raw_app_policy_test"
+      )).code).toEqual(0);
+      await waitForDeploymentJobs(backend);
+      expect(await readPolicy()).toMatchObject({
+        execution_mode: "publisher",
+        sandbox: true,
+        on_behalf_of: "u/svc",
+      });
     });
 });