From 9fba5d3f6263f3e2fef2cb386b23048aba29aaaa Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sat, 3 Oct 2026 07:13:26 +0000 Subject: [PATCH 1/7] Keep app visibility out of republishing Republishing, renaming or rolling back an existing app now ships code only. Requests that would change its audience or public setting are refused with a pointer to the share action, and default audience grants apply only on the first publish. --- src/deploy/deploy-service.ts | 86 +++++++------------ src/harness/agent-tools.ts | 11 +-- test/publish-default-audience.test.ts | 118 +++++++------------------- test/share-deployment.test.ts | 26 +++++- 4 files changed, 92 insertions(+), 149 deletions(-) diff --git a/src/deploy/deploy-service.ts b/src/deploy/deploy-service.ts index 6e0d66cdaa..aa6f8c43c0 100644 --- a/src/deploy/deploy-service.ts +++ b/src/deploy/deploy-service.ts @@ -166,6 +166,14 @@ function requiredEntrypoint(input: string | undefined, d: Deployment | null): st return entrypoint; } +const REPUBLISH_VISIBILITY_FIXED = + "republishing an existing app cannot change who can reach it; use the app share action to change visibility"; + +function assertNoVisibilityChange(input: DeployOrUpdateInput): void { + if (input.public !== undefined || input.share !== undefined || input.defaultAudience?.force) + throw new Error(REPUBLISH_VISIBILITY_FIXED); +} + export function createDeployService(deps: DeployServiceDeps): DeployService { const leaderLease = deps.leaderLease ?? createNoopLeaderLease(); const advisoryLock = deps.advisoryLock ?? createNoopAdvisoryLock(); @@ -297,34 +305,14 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { return false; } - async function reconcileDefaultAudience( + async function grantDefaultAudience( d: Deployment, da: NonNullable, - isCreate: boolean, explicitShares?: DeployOrUpdateInput["share"], ): Promise { - if (!isCreate && !da.force && d.createdInScope && da.contextScopeId !== d.createdInScope) return; const ref = deploymentRef(d.id); const owner = d.createdBy; - const prior = isCreate ? [] : (d.defaultAudience?.granteeScopeIds ?? []); - const priorSet = new Set(prior); - const nextSet = new Set(da.granteeScopeIds); - const before = await grantsOn(d); - for (const grantee of prior) { - if (nextSet.has(grantee)) continue; - const explicit = before.filter((g) => g.granteeScopeId === grantee && g.permission !== "read"); - await deps.acl.revoke(d.ownerScopeId, ref, grantee, owner); - for (const g of explicit) await deps.acl.grant(g); - deps.auditLog.record({ - at: Date.now(), - principalId: owner, - action: "deploy_unshare", - resource: ref, - scopeLabel: grantee, - }); - } for (const grantee of da.granteeScopeIds) { - if (priorSet.has(grantee)) continue; await deps.acl.grant({ ownerScopeId: d.ownerScopeId, ref, @@ -725,6 +713,7 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { if (!existing) throw new Error(`no deployment named: ${input.renameFrom}`); if (!(await canManage(existing, ownerScopeId, createdBy, input.createdInScope))) throw new Error(`not authorized to manage deployment: ${input.renameFrom}`); + assertNoVisibilityChange(input); const clash = await deps.deployStore.getByName(input.name); if (clash && clash.id !== existing.id) throw new Error(`deployment name taken: ${input.name}`); await deps.deployStore.setName(existing.id, input.name); @@ -745,18 +734,9 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ...(input.env ? { env: input.env } : {}), ...(input.stampEnv ? { stampEnv: input.stampEnv } : {}), }); - if (input.defaultAudience) - await reconcileDefaultAudience( - (await deps.deployStore.get(existing.id))!, - input.defaultAudience, - false, - input.share, - ); } else if (input.alwaysOn !== undefined) await this.setDeploymentAlwaysOn(existing.id, input.alwaysOn); if (input.embedAncestors !== undefined) await this.setDeploymentEmbedAncestors(existing.id, input.embedAncestors); - if (input.public !== undefined) await this.setDeploymentPublic(existing.id, input.public, { createdBy }); - if (input.share?.length) await issueShares((await deps.deployStore.get(existing.id))!, createdBy, input.share); return (await deps.deployStore.get(existing.id))!; } @@ -766,6 +746,7 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { if (!existing) throw new Error(`no deployment named: ${input.name}`); if (!(await canManage(existing, ownerScopeId, createdBy, input.createdInScope))) throw new Error(`not authorized to manage deployment: ${input.name}`); + assertNoVisibilityChange(input); await this.rollbackDeployment( existing.id, input.rollbackTo, @@ -773,15 +754,11 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ); if (input.embedAncestors !== undefined) await this.setDeploymentEmbedAncestors(existing.id, input.embedAncestors); - if (input.public !== undefined) await this.setDeploymentPublic(existing.id, input.public, { createdBy }); - if (input.share?.length) await issueShares((await deps.deployStore.get(existing.id))!, createdBy, input.share); return (await deps.deployStore.get(existing.id))!; } const files = input.files ?? []; const existing = input.name !== undefined ? await deps.deployStore.getByName(input.name) : null; - let d: Deployment; - let isCreate: boolean; if (existing) { if ( existing.ownerScopeId !== ownerScopeId && @@ -789,8 +766,9 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ) { throw new Error(`deployment name taken: ${input.name}`); } + assertNoVisibilityChange(input); const entrypoint = requiredEntrypoint(input.entrypoint, existing); - d = await this.redeploy(existing.id, { + await this.redeploy(existing.id, { entrypoint, files, ...(input.alwaysOn !== undefined ? { alwaysOn: input.alwaysOn } : {}), @@ -798,31 +776,25 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ...(input.env ? { env: input.env } : {}), ...(input.stampEnv ? { stampEnv: input.stampEnv } : {}), }); - isCreate = false; - } else { - const entrypoint = requiredEntrypoint(input.entrypoint, existing); - d = await this.deploy({ - ownerScopeId, - createdBy, - entrypoint, - files, - ...(input.homeFiles ? { homeFiles: input.homeFiles } : {}), - ...(input.name !== undefined ? { name: input.name } : {}), - ...(input.createdInScope !== undefined ? { createdInScope: input.createdInScope } : {}), - ...(input.env ? { env: input.env } : {}), - ...(input.stampEnv ? { stampEnv: input.stampEnv } : {}), - ...(input.alwaysOn !== undefined ? { alwaysOn: input.alwaysOn } : {}), - }); - isCreate = true; + if (input.embedAncestors !== undefined) + await this.setDeploymentEmbedAncestors(existing.id, input.embedAncestors); + return (await deps.deployStore.get(existing.id))!; } + const d = await this.deploy({ + ownerScopeId, + createdBy, + entrypoint: requiredEntrypoint(input.entrypoint, existing), + files, + ...(input.homeFiles ? { homeFiles: input.homeFiles } : {}), + ...(input.name !== undefined ? { name: input.name } : {}), + ...(input.createdInScope !== undefined ? { createdInScope: input.createdInScope } : {}), + ...(input.env ? { env: input.env } : {}), + ...(input.stampEnv ? { stampEnv: input.stampEnv } : {}), + ...(input.alwaysOn !== undefined ? { alwaysOn: input.alwaysOn } : {}), + }); if (input.public !== undefined) await this.setDeploymentPublic(d.id, input.public, { createdBy }); if (input.defaultAudience) - await reconcileDefaultAudience( - (await deps.deployStore.get(d.id))!, - input.defaultAudience, - isCreate, - input.share, - ); + await grantDefaultAudience((await deps.deployStore.get(d.id))!, input.defaultAudience, input.share); if (input.embedAncestors !== undefined) await this.setDeploymentEmbedAncestors(d.id, input.embedAncestors); if (input.share?.length) await issueShares((await deps.deployStore.get(d.id))!, createdBy, input.share); return (await deps.deployStore.get(d.id))!; diff --git a/src/harness/agent-tools.ts b/src/harness/agent-tools.ts index d14760bc67..718effb6ac 100644 --- a/src/harness/agent-tools.ts +++ b/src/harness/agent-tools.ts @@ -1145,9 +1145,10 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): "directory exists and contains files. For a new app or a code/file update, always pass `entrypoint`; " + "the app must listen on the PORT env var. `dir` is workspace-relative: use `app`, never a path " + "beginning with `/` or a redundant `workspace/app`. `renameFrom` takes an existing " + - "deployment name, not its ID. Set audience to [] to suppress default audience grants, or supply " + - "publication-time grants. `public: true` makes the app reachable without sign-in; it is never the default and is refused unless an org admin has enabled external app sharing. " + - "Use apps action share for subsequent grants. Share the full absolute URL " + + "deployment name, not its ID. On a first publish, set audience to [] to suppress default audience grants, or supply " + + "publication-time grants; `public: true` makes the app reachable without sign-in, is never the default, and is refused unless an org admin has enabled external app sharing. " + + "Republishing, renaming or rolling back an existing app ships code only and never changes who can reach it: `audience` and `public` are refused there. " + + "Change an existing app's visibility only with apps action share. Share the full absolute URL " + "returned by apps action publish so it works in Slack and other surfaces. Use `name` for a friendly, " + "stable link /d//; `renameFrom` to rename; `rollbackTo` to flip back to an earlier version. " + "Egress is open, " + @@ -1170,7 +1171,7 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): }), { description: - "Publication-time access grants. Omit to use the conversation's default audience; [] suppresses default grants for owner-only publication. Existing explicit grants survive. Use apps action share for subsequent grants.", + "First-publish access grants only. Omit to use the conversation's default audience; [] suppresses default grants for owner-only publication. Refused when republishing an existing app; use apps action share instead.", }, ), ), @@ -1206,7 +1207,7 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): public: Type.Optional( Type.Boolean({ description: - "Explicitly set whether anyone with the link can open the app without signing in. Defaults to private for new apps; omit to preserve the current setting on updates.", + "First publish only: set whether anyone with the link can open the app without signing in. New apps are private by default. Refused when republishing; change an existing app with apps action share.", }), ), alwaysOn: Type.Optional( diff --git a/test/publish-default-audience.test.ts b/test/publish-default-audience.test.ts index cbf9acf199..5a9339c9cb 100644 --- a/test/publish-default-audience.test.ts +++ b/test/publish-default-audience.test.ts @@ -215,20 +215,45 @@ test("D2: private channel grants exclude the owner", async () => { assert.equal((await deploy.reachDeployment("priv", "U9")).status, "denied"); }); -test("D3: republish from the originating channel reconciles — joiner added, leaver revoked", async () => { +test("D3: republish from the originating channel never changes reach — joiners not added, leavers kept", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); await channel(deploy, acl, config, "U1", "C2", { isPrivate: true, members: [internal("U1"), internal("U2"), internal("U3")], }).publish({ entrypoint: "node server.js", name: "priv" }); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, + const r = await channel(deploy, acl, config, "U1", "C2", { + isPrivate: false, members: [internal("U1"), internal("U2"), internal("U4")], }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok", "U2 stays"); - assert.equal((await deploy.reachDeployment("priv", "U4")).status, "ok", "U4 added"); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "denied", "U3 revoked"); + assert.equal(r.version, 2); + assert.equal((await deploy.reachDeployment("priv", "U3")).status, "ok", "U3 kept"); + assert.equal((await deploy.reachDeployment("priv", "U4")).status, "denied", "U4 not added"); + assert.equal((await deploy.reachDeployment("priv", "U-anyone")).status, "denied", "no org widening"); +}); + +test("republish refuses audience changes, including an empty opt-out; sharing changes reach separately", async () => { + const { deploy, acl } = svc(); + const config = createMemoryConfigStore(ORG); + await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ + entrypoint: "node server.js", + name: "site", + share: [], + }); + for (const share of [[{ scope: orgScope, permission: "read" as const }], []]) + await assert.rejects( + () => + channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ + entrypoint: "node server.js", + name: "site", + share, + }), + /cannot change who can reach it/, + ); + assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "denied"); + const id = (await deploy.getDeployment("site"))!.id; + await deploy.shareDeployment(id, orgScope, "read", { createdBy: "U1" }); + assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); }); test("D3: a redeploy from a DM/other context never shrinks or shifts the default audience", async () => { @@ -296,69 +321,6 @@ test("D3: an UNKNOWN privacy signal (e.g. flaky conversations.info) FREEZES — assert.match(r.audience?.note ?? "", /public\/private|leaving reach unchanged/); }); -test("D3: a complete enumeration finding only the owner revokes prior default members", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - await channel(deploy, acl, config, "U1", "C2", { isPrivate: true, members: [internal("U1")] }).publish({ - entrypoint: "node server.js", - name: "priv", - }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "denied"); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "denied"); -}); - -test("D5: an explicit opt-out (share:[]) issued from a DM still applies — owner directive bypasses source-binding", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); - const dmCtx = ctxFor(deploy, acl, config, { actor: "U1", contextScope: scopeId("personal", "U1"), kind: "dm" }); - await dmCtx.publish({ entrypoint: "node server.js", name: "site", share: [] }); - assert.equal( - (await deploy.reachDeployment("site", "U-anyone")).status, - "denied", - "opt-out from a DM dropped the org grant", - ); - assert.equal((await deploy.reachDeployment("site", "U1")).status, "ok"); -}); - -test("D3/D5: reconcile revoking a departed default member PRESERVES the owner's explicit write co-manager grant", async () => { - const { deploy, deployStore, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ - entrypoint: "node server.js", - name: "priv", - share: [{ scope: scopeId("personal", "U2"), permission: "write" }], - }); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - const id = (await deployStore.getByName("priv"))!.id; - const grants = await acl.grantsFor(scopeId("personal", "U1"), `deployment:${id}`); - const u2 = grants.filter((g) => g.granteeScopeId === scopeId("personal", "U2")); - assert.ok( - u2.some((g) => g.permission === "write"), - "explicit write co-manager grant survives reconcile", - ); - assert.ok(!u2.some((g) => g.permission === "read"), "the departed member's default read grant is revoked"); - assert.equal( - (await deploy.reachDeployment("priv", "U2")).status, - "ok", - "U2 still reaches via the surviving write grant", - ); -}); - test("D4: a first publish whose member enumeration is incomplete reports a user-visible 'share manually' note", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); @@ -397,22 +359,6 @@ test("D3: a redeploy from a DIFFERENT channel never shifts reach to the wrong ch ); }); -test("D3: public → private transition drops the org grant on the next ship", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); - await channel(deploy, acl, config, "U1", "C1", { - isPrivate: true, - members: [internal("U1"), internal("U2")], - }).publish({ entrypoint: "node server.js", name: "site" }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "denied", "org grant dropped"); - assert.equal((await deploy.reachDeployment("site", "U1")).status, "ok", "owner still reaches"); -}); - test("D5: an explicit share layers on top of the public default (org + the named person)", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); @@ -457,7 +403,7 @@ test("D5: only the OWNER may widen — even a write-grant manager cannot re-shar name: "site", share: [{ scope: scopeId("personal", "U7"), permission: "read" }], }), - /only a manager/, + /cannot change who can reach it/, ); }); diff --git a/test/share-deployment.test.ts b/test/share-deployment.test.ts index 98621f6fc1..ae6c57f5e9 100644 --- a/test/share-deployment.test.ts +++ b/test/share-deployment.test.ts @@ -503,11 +503,35 @@ test("a manage grantee may redeploy but cannot make the owner's app public", asy files: [], public: true, }), - /only the owner/, + /cannot change who can reach it/, ); assert.equal((await deploy.getDeployment(d.id))?.public, undefined); }); +test("republish, rename and rollback never change visibility; only the separate public toggle does", async () => { + const { deploy } = makeDeploy(); + const owner = { ownerScopeId: scopeId("personal", "U1"), createdBy: "U1", entrypoint: "x", files: [] }; + const d = await deploy.deployOrUpdate({ ...owner, name: "vis" }); + assert.equal(d.public, undefined, "first publish is private by default"); + await deploy.deployOrUpdate({ ...owner, name: "vis" }); + for (const extra of [ + { name: "vis", public: true }, + { name: "vis", public: false }, + { name: "vis", share: [{ scope: scopeId("personal", "U2"), permission: "read" as const }] }, + { name: "vis2", renameFrom: "vis", public: true }, + { name: "vis", rollbackTo: 1, public: true }, + ]) + await assert.rejects(() => deploy.deployOrUpdate({ ...owner, ...extra }), /cannot change who can reach it/); + const after = await deploy.getDeployment(d.id); + assert.equal(after?.public, undefined); + assert.equal(after?.name, "vis"); + assert.equal((await deploy.reachDeployment("vis", "U2")).status, "denied"); + await deploy.setDeploymentPublic(d.id, true, { createdBy: "U1" }); + assert.equal((await deploy.getDeployment(d.id))?.public, true); + await deploy.deployOrUpdate({ ...owner, name: "vis" }); + assert.equal((await deploy.getDeployment(d.id))?.public, true, "a plain republish keeps the existing setting"); +}); + test("transferDeploymentOwner is home authority — a write ('manage') grantee cannot give the app away", async () => { const { deploy } = makeDeploy(); const d = await deploy.deploy({ From 82909a6967f2c05a816d66dc0f73d4a1d64ad4f5 Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sat, 3 Oct 2026 07:23:22 +0000 Subject: [PATCH 2/7] Require approval before widening app visibility --- src/api/artifact-share.ts | 10 +++- src/api/control-service.ts | 4 ++ src/api/routes/deployments.ts | 12 +++++ src/core/approval-store.ts | 2 +- src/deploy/visibility-approval.ts | 22 +++++++++ src/harness/agent-tools.ts | 31 +++++++++---- src/tools/primitives.ts | 47 +++++++++++++++++-- src/triggers/run-trigger.ts | 2 +- test/agent-tools.test.ts | 37 +++++++++++++++ test/approval-store.test.ts | 11 +++++ test/share-deployment.test.ts | 76 +++++++++++++++++++++++++++++-- test/share-verb.test.ts | 24 ++++++++++ 12 files changed, 258 insertions(+), 20 deletions(-) create mode 100644 src/deploy/visibility-approval.ts diff --git a/src/api/artifact-share.ts b/src/api/artifact-share.ts index fc8e322a95..b22e989b4e 100644 --- a/src/api/artifact-share.ts +++ b/src/api/artifact-share.ts @@ -83,6 +83,7 @@ export interface ShareArtifactRequest { email?: string; permission?: Permission; move?: boolean; + visibilityApproved?: true; } export type ShareArtifactResult = @@ -97,7 +98,14 @@ export type ShareArtifactResult = } | { ok: false; - code: "bad_request" | "not_found" | "forbidden" | "recipient_not_found" | "ambiguous_recipient" | "share_failed"; + code: + | "bad_request" + | "not_found" + | "forbidden" + | "recipient_not_found" + | "ambiguous_recipient" + | "share_failed" + | "approval_required"; message: string; candidates?: Array<{ id: string; label: string }>; }; diff --git a/src/api/control-service.ts b/src/api/control-service.ts index 28a36a4a7e..8920cb5580 100644 --- a/src/api/control-service.ts +++ b/src/api/control-service.ts @@ -1,3 +1,4 @@ +import { agentTurnCapability, VISIBILITY_APPROVAL_REQUIRED } from "../deploy/visibility-approval.ts"; import { unattendedGrantRefusal } from "../cron/authority.ts"; import { isDeepStrictEqual } from "node:util"; import type { Cron, CronFireLogEntry, CronSchedule, Destination, Principal, Webhook } from "../types.ts"; @@ -945,6 +946,9 @@ export function createControlService(app: App, scheduler?: Scheduler, admin?: Ad }; } + if (isOrg && req.type === "deploy" && agentTurnCapability(capability) && req.visibilityApproved !== true) + return { ok: false, code: "approval_required", message: VISIBILITY_APPROVAL_REQUIRED }; + try { if (orgSkillCede) { const promoted = await app.promoteSkill(home.id, toScope, capability.actorId, capability.liveActor === true); diff --git a/src/api/routes/deployments.ts b/src/api/routes/deployments.ts index 55962944a0..fd24828396 100644 --- a/src/api/routes/deployments.ts +++ b/src/api/routes/deployments.ts @@ -1,3 +1,4 @@ +import { agentTurnCapability, VISIBILITY_APPROVAL_REQUIRED } from "../../deploy/visibility-approval.ts"; import { EXTERNAL_APP_SHARING_OFF, externalAppSharingAllowed } from "../../feature-flags.ts"; import { SocksProxyAgent } from "socks-proxy-agent"; import { request as httpRequest } from "node:http"; @@ -1578,6 +1579,11 @@ export async function shareDeployment(ctx: ApiCtx): Promise { error: "bad_request", message: "public access and person/scope access must be changed separately", }); + if (b.public && agentTurnCapability(capability)) { + const current = await app.getDeployment(params.id!); + if (current && current.public !== true) + return sendJson(res, 403, { error: "approval_required", message: VISIBILITY_APPROVAL_REQUIRED }); + } try { const deployment = await app.setDeploymentPublic(params.id!, b.public, { createdBy: capability.actorId }); return sendJson(res, 200, { @@ -1631,6 +1637,12 @@ export async function shareDeployment(ctx: ApiCtx): Promise { message: "more than one teammate matches", candidates: target.candidates, }); + if (permission && parseScopeId(target.scope).kind === "org" && agentTurnCapability(capability)) { + const current = await app.getDeployment(params.id!); + const granted = current ? await app.deploymentGrantees(current.id) : []; + if (!granted.some((g) => g.scope === target.scope)) + return sendJson(res, 403, { error: "approval_required", message: VISIBILITY_APPROVAL_REQUIRED }); + } try { const invite = typeof b.email === "string" && permission === "read" diff --git a/src/core/approval-store.ts b/src/core/approval-store.ts index ab27420928..f5fc22acae 100644 --- a/src/core/approval-store.ts +++ b/src/core/approval-store.ts @@ -18,7 +18,7 @@ export function createApprovalStore( deliveries: Pick, ) { async function deliver(id: string, record: PendingApprovalRecord): Promise { - if (record.request?.surface !== "slack") return; + if (record.request?.surface !== "slack" && record.request?.surface !== "cron") return; const actorId = approvalDeliveryRecipient(record.request.actor); if (!actorId) return; await deliveries.enqueue({ diff --git a/src/deploy/visibility-approval.ts b/src/deploy/visibility-approval.ts new file mode 100644 index 0000000000..0c5cd4e9b9 --- /dev/null +++ b/src/deploy/visibility-approval.ts @@ -0,0 +1,22 @@ +export type VisibilityWidening = "public" | "org"; + +export const VISIBILITY_APPROVAL_REQUIRED = + "making an app public or visible to the whole organization needs the owner's approval; use the apps tool so an approval card is posted"; + +export function visibilityApprovalCommand(app: string, widening: VisibilityWidening): string { + return widening === "public" ? `apps share ${app} public:true` : `apps share ${app} toScope:org`; +} + +export function visibilityApprovalKey(app: string, widening: VisibilityWidening): string { + return `app-visibility:${JSON.stringify([app, widening])}`; +} + +export function visibilityApprovalReason(app: string, widening: VisibilityWidening): string { + return widening === "public" + ? `make app "${app}" public: anyone with the link can open it without signing in` + : `share app "${app}" with everyone in the organization`; +} + +export function agentTurnCapability(claims: { sessionId?: string } | undefined): boolean { + return typeof claims?.sessionId === "string"; +} diff --git a/src/harness/agent-tools.ts b/src/harness/agent-tools.ts index d14760bc67..9acb9668f3 100644 --- a/src/harness/agent-tools.ts +++ b/src/harness/agent-tools.ts @@ -1258,6 +1258,7 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): ), ); } catch (e) { + if (e instanceof NeedsApproval) return blockOnApproval(callId, e, undefined, "apps"); const msg = errMessage(e); return recordResult( callId, @@ -3286,7 +3287,13 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): text("[error] set public, email, or toScope, not more than one"), true, ); - const d = await tc.setDeploymentPublic(id, params.public); + let d: Awaited>; + try { + d = await tc.setDeploymentPublic(id, params.public); + } catch (e) { + if (e instanceof NeedsApproval) return blockOnApproval(callId, e, undefined, tool); + throw e; + } return recordResult( callId, { tool, action, type, id: d.id, public: d.public }, @@ -3300,14 +3307,20 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): text("[error] toScope or email is required unless public is set"), true, ); - const r = await tc.shareArtifact({ - type, - id, - ...(params.toScope ? splitToScope(params.toScope) : {}), - ...(params.email !== undefined ? { email: params.email } : {}), - ...(params.permission !== undefined ? { permission: params.permission } : {}), - ...(move ? { move: true } : {}), - }); + let r: Awaited>; + try { + r = await tc.shareArtifact({ + type, + id, + ...(params.toScope ? splitToScope(params.toScope) : {}), + ...(params.email !== undefined ? { email: params.email } : {}), + ...(params.permission !== undefined ? { permission: params.permission } : {}), + ...(move ? { move: true } : {}), + }); + } catch (e) { + if (e instanceof NeedsApproval) return blockOnApproval(callId, e, undefined, tool); + throw e; + } if (isUnavailable(r)) return unavailable(callId, tool); if (!r.ok) { const candidates = r.candidates?.length diff --git a/src/tools/primitives.ts b/src/tools/primitives.ts index 467504556a..a93f1f527a 100644 --- a/src/tools/primitives.ts +++ b/src/tools/primitives.ts @@ -1,3 +1,9 @@ +import { + visibilityApprovalCommand, + visibilityApprovalKey, + visibilityApprovalReason, + type VisibilityWidening, +} from "../deploy/visibility-approval.ts"; import type { MemoryCaptureMetadata } from "../memory/records.ts"; import { disclosedMemory } from "../memory/disclosure.ts"; import { MaskedExecutionError, executionSecretEnv, createExactSecretValueMasker } from "../security/secret-masking.ts"; @@ -583,6 +589,16 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { ); }; + const requireVisibilityApproval = (app: string, widening: VisibilityWidening): void => { + const command = visibilityApprovalCommand(app, widening); + const approvalKey = visibilityApprovalKey(app, widening); + if (deps.authorizeCommand(command, approvalKey, true)) return; + throw new NeedsApproval(command, visibilityApprovalReason(app, widening), "approval", undefined, approvalKey, { + session: false, + always: false, + }); + }; + const ledger = deps.ledger ?? createNullLedger(); const runId = deps.runId; const attempt = deps.attempt ?? 1; @@ -1081,6 +1097,14 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { } return { scope, permission: s.permission }; }); + const appRef = input.renameFrom ?? input.name; + const current = appRef !== undefined ? await deps.deploy.getDeployment(appRef) : null; + const label = current?.name ?? appRef ?? "new app"; + if (input.public === true && current?.public !== true) requireVisibilityApproval(label, "public"); + if (orgScopeId && resolvedShare?.some((s) => s.scope === orgScopeId)) { + const granted = current ? await deps.deploy.deploymentGrantees(current.id) : []; + if (!granted.some((g) => g.scope === orgScopeId)) requireVisibilityApproval(label, "org"); + } return once(async () => { const d = await deps.deploy.deployOrUpdate({ ownerScopeId: owner, @@ -1136,6 +1160,10 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { }, async setDeploymentPublic(id: string, isPublic: boolean) { + if (isPublic) { + const current = await deps.deploy.getDeployment(id); + if (current && current.public !== true) requireVisibilityApproval(current.name ?? current.id, "public"); + } const d = await deps.deploy.setDeploymentPublic(id, isPublic, { createdBy: deps.createdBy }); return { id: d.id, ...(d.name ? { name: d.name } : {}), public: d.public === true }; }, @@ -1362,11 +1390,22 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { async (c, cl) => c.writeSoul(content, cl, expectedVersion), (r) => r.ok, ), - shareArtifact: (req) => - controlOp( - (c, cl) => c.shareArtifact(req, cl), + shareArtifact: async (req) => { + let approved = false; + const targetScope = req.scope === "org" && orgScopeId ? orgScopeId : req.scope; + if (req.type === "deploy" && targetScope !== undefined && parseScopeId(targetScope).kind === "org") { + const current = await deps.deploy.getDeployment(req.id); + const granted = current && orgScopeId ? await deps.deploy.deploymentGrantees(current.id) : []; + if (req.move || !granted.some((g) => g.scope === orgScopeId)) { + requireVisibilityApproval(current?.name ?? req.id, "org"); + approved = true; + } + } + return controlOp( + (c, cl) => c.shareArtifact(approved ? { ...req, visibilityApproved: true } : req, cl), (r) => r.ok, - ), + ); + }, post: (text, opts, files) => surfaceOp( diff --git a/src/triggers/run-trigger.ts b/src/triggers/run-trigger.ts index 8451709b69..7d070adbbe 100644 --- a/src/triggers/run-trigger.ts +++ b/src/triggers/run-trigger.ts @@ -356,7 +356,7 @@ export async function runTrigger(deps: TriggerDeps, spec: TriggerSpec): Promise< sessionId = res.sessionId; if (res.status === "silent") return; if (res.status === "pending_approval") { - note = "hit a require_approval command — failed closed (no human at fire/event time)"; + note = "held a command for the owner's approval; it did not run"; console.warn(`[trigger] ${spec.surface} ${spec.fireKey} ${note}`); return; } diff --git a/test/agent-tools.test.ts b/test/agent-tools.test.ts index 6a53561b58..46f1e626dc 100644 --- a/test/agent-tools.test.ts +++ b/test/agent-tools.test.ts @@ -4075,3 +4075,40 @@ test("background process guidance reflects the configured sandbox token lifetime assert.match(unlimited, /does not expire those turn tokens/); assert.doesNotMatch(unlimited, /turn tokens expire \d+ hours/); }); + +test("apps visibility widening pauses the turn on an approval card instead of applying", async () => { + const held = (key: string) => + new NeedsApproval(`apps share x ${key}`, "widen", "approval", undefined, key, { session: false, always: false }); + const ref: ToolContextRef = { + current: { + ...fakeToolContext(), + async publish() { + throw held("public"); + }, + async setDeploymentPublic() { + throw held("public-share"); + }, + async shareArtifact() { + throw held("org"); + }, + }, + pendingApprovals: [], + }; + const apps = createAgentTools(ref, { controlTools: true }).find((t) => t.name === "apps")!; + for (const args of [ + { action: "publish", name: "x", entrypoint: "node s.js", public: true }, + { action: "share", id: "x", public: true }, + { action: "share", id: "x", toScope: "org" }, + ]) { + assert.match(textOut(await call(apps, args)), /needs human approval/); + } + assert.equal(ref.pausedOnApproval, true); + assert.deepEqual( + ref.pendingApprovals!.map((a) => [a.approvalKey, a.grantModes]), + [ + ["public", { session: false, always: false }], + ["public-share", { session: false, always: false }], + ["org", { session: false, always: false }], + ], + ); +}); diff --git a/test/approval-store.test.ts b/test/approval-store.test.ts index 46cf3b0d92..bf5cca20bb 100644 --- a/test/approval-store.test.ts +++ b/test/approval-store.test.ts @@ -92,3 +92,14 @@ test("system approvals stay pending without enqueuing or reviving an impossible assert.deepEqual(await deliveries.pending("principal"), []); assert.equal((await deliveries.get(stale.id))?.expiredAt, expiredAt, "a sweep must not revive the stale DM"); }); + +test("scheduled-task approvals reach the task owner instead of failing silently", async () => { + const deliveries = createDeliveryStore(); + const approvals = createApprovalStore(createMemoryMap(), deliveries); + const approval = record("cron:K1"); + approval.request!.surface = "cron"; + await approvals.put("A1", approval); + const pending = await deliveries.pending("principal"); + assert.equal(pending.length, 1); + assert.equal(pending[0]!.destination.commandApprovalId, "A1"); +}); diff --git a/test/share-deployment.test.ts b/test/share-deployment.test.ts index 98621f6fc1..8ecd05bfe9 100644 --- a/test/share-deployment.test.ts +++ b/test/share-deployment.test.ts @@ -27,6 +27,8 @@ import type { CapabilityClaims } from "../src/auth/capability-token.ts"; import type { RecipientResolution } from "../src/directory/directory-store.ts"; import type { Sandbox, SandboxHandle } from "../src/sandbox/sandbox.ts"; import { scopeId } from "../src/types.ts"; +import { NeedsApproval } from "../src/tools/primitives.ts"; +import { visibilityApprovalKey } from "../src/deploy/visibility-approval.ts"; import type { FeatureFlagStore } from "../src/feature-flags.ts"; const externalSharingOn = { enabled: async () => true } as unknown as FeatureFlagStore; @@ -401,7 +403,8 @@ const appSandbox = (): Sandbox => { } as unknown as Sandbox; }; -function toolCtx(deploy: DeployService): ToolContext { +function toolCtx(deploy: DeployService, approved: string[] = []): ToolContext { + const approvals = new Set(approved); return createToolContext({ sandbox: appSandbox(), provision: async () => ({}) as SandboxHandle, @@ -410,7 +413,7 @@ function toolCtx(deploy: DeployService): ToolContext { { scopeId: scopeId("org", "default-org"), mountPath: "global", mode: "ro" }, ], commandPolicy: () => ({}) as never, - authorizeCommand: () => false, + authorizeCommand: (_command: string, key?: string) => key !== undefined && approvals.delete(key), grantedHandles: [], workspace: {} as never, deploy, @@ -421,7 +424,7 @@ function toolCtx(deploy: DeployService): ToolContext { test('publish share:[{scope:"org"}] resolves to the org — truthful readback, real reach (the QM bug)', async () => { const { deploy } = makeDeploy(); - const r = await toolCtx(deploy).publish({ + const r = await toolCtx(deploy, [visibilityApprovalKey("wide", "org")]).publish({ entrypoint: "x", name: "wide", share: [{ scope: "org", permission: "read" }], @@ -437,7 +440,11 @@ test("publish keeps apps private by default and requires an explicit public opt- assert.equal(privateApp.public, undefined); assert.equal((await deploy.getDeployment("private-app"))?.public, undefined); - const publicApp = await toolCtx(deploy).publish({ entrypoint: "x", name: "public-app", public: true }); + const publicApp = await toolCtx(deploy, [visibilityApprovalKey("public-app", "public")]).publish({ + entrypoint: "x", + name: "public-app", + public: true, + }); assert.equal(publicApp.public, true); assert.equal((await deploy.getDeployment("public-app"))?.public, true); }); @@ -925,3 +932,64 @@ test("uniform app share uses the same invitation sender", async () => { assert.equal(result.invitation?.emailSent, true); assert.deepEqual(sent, ["invitee@example.com"]); }); + +test("widening an app to public or org-wide holds for approval; narrowing never does", async () => { + const { deploy } = makeDeploy(); + await toolCtx(deploy).publish({ entrypoint: "x", name: "held" }); + + await assert.rejects( + () => toolCtx(deploy).publish({ entrypoint: "x", name: "held", public: true }), + (e: unknown) => e instanceof NeedsApproval && e.approvalKey === visibilityApprovalKey("held", "public"), + ); + assert.equal((await deploy.getDeployment("held"))?.public, undefined, "republish did not widen"); + + await assert.rejects( + () => toolCtx(deploy).setDeploymentPublic("held", true), + (e: unknown) => e instanceof NeedsApproval && e.grantModes?.always === false && e.grantModes.session === false, + ); + await assert.rejects( + () => toolCtx(deploy).publish({ entrypoint: "x", name: "held", share: [{ scope: "org", permission: "read" }] }), + (e: unknown) => e instanceof NeedsApproval && e.approvalKey === visibilityApprovalKey("held", "org"), + ); + assert.equal((await deploy.reachDeployment("held", "U2")).status, "denied", "org grant not applied"); + + const made = await toolCtx(deploy, [visibilityApprovalKey("held", "public")]).setDeploymentPublic("held", true); + assert.equal(made.public, true, "an approved request applies"); + assert.equal((await toolCtx(deploy).setDeploymentPublic("held", true)).public, true, "already public needs nothing"); + assert.equal((await toolCtx(deploy).setDeploymentPublic("held", false)).public, false, "narrowing needs no approval"); + await toolCtx(deploy).publish({ entrypoint: "x", name: "held", public: false }); + await assert.rejects( + () => toolCtx(deploy).setDeploymentPublic("held", true), + NeedsApproval, + "an approval is single-use: widening again asks again", + ); +}); + +test("agent turn tokens cannot widen an app through the HTTP share endpoint", async () => { + const { app, deploy } = apiHarness(); + await deploy.deploy({ + ownerScopeId: scopeId("personal", "U1"), + createdBy: "U1", + entrypoint: "x", + files: [], + name: "api-held", + }); + const agent = { ...cap("U1"), sessionId: "s1" } as CapabilityClaims; + + const pub = await callShare(app, agent, "api-held", { public: true }); + assert.equal(pub.status, 403); + assert.equal(pub.body.error, "approval_required"); + assert.equal((await deploy.getDeployment("api-held"))?.public, undefined); + + const org = await callShare(app, agent, "api-held", { scope: "org" }); + assert.equal(org.status, 403); + assert.equal(org.body.error, "approval_required"); + assert.equal((await deploy.reachDeployment("api-held", "U2")).status, "denied"); + + assert.equal((await callShare(app, agent, "api-held", { public: false })).status, 200, "narrowing is allowed"); + assert.equal( + (await callShare(app, cap("U1"), "api-held", { public: true })).status, + 200, + "the owner's own UI action is the approval", + ); +}); diff --git a/test/share-verb.test.ts b/test/share-verb.test.ts index 2a4bcbb7e0..2523c5803f 100644 --- a/test/share-verb.test.ts +++ b/test/share-verb.test.ts @@ -631,3 +631,27 @@ test("SkillStore.promote supersedes an existing same-name org record instead of const orgRecords = (await store.list()).filter((x) => x.scopeId === scopeId("org", ORG) && x.status === "published"); assert.equal(orgRecords.length, 1); }); + +test("an agent turn cannot share or move an app org-wide without an approved request", async () => { + for (const move of [false, true]) { + const state = ownerState(); + const svc = createControlService(fakeApp(state)); + const agent = { ...cap("U1"), sessionId: "s1" }; + const held = await svc.shareArtifact({ type: "deploy", id: "D1", scope: "org", ...(move ? { move } : {}) }, agent); + assert.equal(held.ok, false); + assert.equal(!held.ok && held.code, "approval_required"); + assert.equal((await grantsOf(state)).length, 0); + assert.equal(state.moves.length, 0); + const approved = await svc.shareArtifact( + { type: "deploy", id: "D1", scope: "org", visibilityApproved: true, ...(move ? { move } : {}) }, + agent, + ); + assert.ok(approved.ok, JSON.stringify(approved)); + } + const svc = createControlService(fakeApp(ownerState())); + const channel = await svc.shareArtifact( + { type: "deploy", id: "D1", scope: scopeId("channel", "C1") }, + { ...cap("U1"), sessionId: "s1" }, + ); + assert.ok(channel.ok, "narrower grants are unaffected"); +}); From e529ee9870f53096c111c5785b1262b23b784198 Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sat, 3 Oct 2026 07:34:25 +0000 Subject: [PATCH 3/7] Only look up the app when a publish would widen access --- src/tools/primitives.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/tools/primitives.ts b/src/tools/primitives.ts index a93f1f527a..87eb8b320c 100644 --- a/src/tools/primitives.ts +++ b/src/tools/primitives.ts @@ -1097,13 +1097,16 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { } return { scope, permission: s.permission }; }); - const appRef = input.renameFrom ?? input.name; - const current = appRef !== undefined ? await deps.deploy.getDeployment(appRef) : null; - const label = current?.name ?? appRef ?? "new app"; - if (input.public === true && current?.public !== true) requireVisibilityApproval(label, "public"); - if (orgScopeId && resolvedShare?.some((s) => s.scope === orgScopeId)) { - const granted = current ? await deps.deploy.deploymentGrantees(current.id) : []; - if (!granted.some((g) => g.scope === orgScopeId)) requireVisibilityApproval(label, "org"); + const widensToOrg = orgScopeId !== null && resolvedShare?.some((s) => s.scope === orgScopeId) === true; + if (input.public === true || widensToOrg) { + const appRef = input.renameFrom ?? input.name; + const current = appRef !== undefined ? await deps.deploy.getDeployment(appRef) : null; + const label = current?.name ?? appRef ?? "new app"; + if (input.public === true && current?.public !== true) requireVisibilityApproval(label, "public"); + if (widensToOrg) { + const granted = current ? await deps.deploy.deploymentGrantees(current.id) : []; + if (!granted.some((g) => g.scope === orgScopeId)) requireVisibilityApproval(label, "org"); + } } return once(async () => { const d = await deps.deploy.deployOrUpdate({ From a0c0b36e52dac1b10de658ff6bde8cedce2aecc3 Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sun, 4 Oct 2026 01:06:09 +0000 Subject: [PATCH 4/7] Post held approvals from scheduled tasks at the task's destination --- src/core/approval-store.ts | 13 +++++++++- src/slack/deliveries.ts | 46 +++++++++++++++++++++++------------ test/approval-store.test.ts | 35 ++++++++++++++++++++------ test/slack-deliveries.test.ts | 22 +++++++++++++++++ 4 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/core/approval-store.ts b/src/core/approval-store.ts index f5fc22acae..f71a2a050e 100644 --- a/src/core/approval-store.ts +++ b/src/core/approval-store.ts @@ -3,6 +3,8 @@ import type { PendingApprovalRecord } from "../types.ts"; import type { DurableMap } from "../persistence/durable-map.ts"; import type { DeliveryStore } from "../delivery/delivery-store.ts"; import { principalDestination } from "../reach/reach.ts"; +import { samePerson } from "../directory/person.ts"; +import type { Destination } from "../types.ts"; export function approvalDeliveryKey(id: string, record: Pick): string { return `command-approval:${id}:${record.createdAt ?? 0}`; @@ -13,6 +15,15 @@ export function approvalDeliveryRecipient(actor: { externalId?: string } | undef return id && !id.startsWith("system:") ? id : undefined; } +export function approvalDestination( + request: Pick, "surface" | "triggerDestination">, + actorId: string, +): Destination { + const d = request.surface === "cron" ? request.triggerDestination : undefined; + if (d && d.type !== "web" && (d.type !== "principal" || samePerson(d.target, actorId))) return d; + return principalDestination(actorId, actorId); +} + export function createApprovalStore( backing: DurableMap, deliveries: Pick, @@ -23,7 +34,7 @@ export function createApprovalStore( if (!actorId) return; await deliveries.enqueue({ destination: { - ...principalDestination(actorId, actorId), + ...approvalDestination(record.request, actorId), commandApprovalId: id, ...(record.request.slackSource ? { slackAccountId: record.request.slackSource.accountId, slackTeamId: record.request.slackSource.teamId } diff --git a/src/slack/deliveries.ts b/src/slack/deliveries.ts index deeeafba4f..ee3fb9dde3 100644 --- a/src/slack/deliveries.ts +++ b/src/slack/deliveries.ts @@ -1,6 +1,6 @@ import { extractPrivateContinuation } from "./external-access.ts"; import { deployAccessMessage } from "./deploy-access.ts"; -import { approvalDeliveryKey, approvalDeliveryRecipient } from "../core/approval-store.ts"; +import { approvalDeliveryKey, approvalDeliveryRecipient, approvalDestination } from "../core/approval-store.ts"; import { samePerson } from "../directory/person.ts"; import { approvalMessage } from "./approval-cards.ts"; import { deliverKeychainCard } from "./keychain-approvals.ts"; @@ -27,7 +27,7 @@ import { applyReactions, } from "./lib.ts"; import type { SlackCoreClient } from "../api/slack-core-client.ts"; -import type { Delivery } from "../types.ts"; +import type { Delivery, PendingApprovalRecord } from "../types.ts"; import type { TurnFlow } from "./turn-flow.ts"; import { cleanAgentReplyForSlack, stripSlackDirectives } from "./messaging.ts"; import { cronIdOf } from "../sessions/session-store.ts"; @@ -236,6 +236,16 @@ export function createDeliveryPoller(deps: { await deliverKeychainCard(core, client, d, channel, threadTs, deps.webUiPublicUrl); return undefined; } + if (d.destination.commandApprovalId) { + const approval = await approvalFor(core, d); + if (!approval) return undefined; + const card = approvalMessage([{ ...approval, reason: approval.reason ?? "Approval required" }]); + await postClient.chat.postMessage({ + ...slackReplyArgs(channel, card.text, threadTs), + blocks: card.blocks, + }); + return undefined; + } if (d.destination.react) { const { failed } = await applyReactions(client, channel, d.destination.react.messageTs, [ d.destination.react.emoji, @@ -411,20 +421,8 @@ export function createDeliveryPoller(deps: { await deliverKeychainCard(core, client, d, dm, d.destination.threadTs, deps.webUiPublicUrl); return undefined; } - const commandApproval = d.destination.commandApprovalId - ? await core.getApproval(d.destination.commandApprovalId) - : null; - const requester = approvalDeliveryRecipient( - commandApproval?.request?.actor as { externalId?: string } | undefined, - ); - if ( - d.destination.commandApprovalId && - (!commandApproval || - !requester || - !samePerson(requester, d.destination.target) || - d.idempotencyKey !== approvalDeliveryKey(d.destination.commandApprovalId, commandApproval)) - ) - return undefined; + const commandApproval = d.destination.commandApprovalId ? await approvalFor(core, d) : null; + if (d.destination.commandApprovalId && !commandApproval) return undefined; let card: { text: string; blocks: Array> } | null = null; if (d.destination.deploymentAccess) card = deployAccessMessage(d.destination.deploymentAccess, d.text); if (commandApproval) @@ -515,3 +513,19 @@ export function createDeliveryPoller(deps: { return { pollDeliveries }; } + +async function approvalFor( + core: { getApproval(id: string): Promise }, + d: { idempotencyKey?: string; destination: { type: string; target: string; commandApprovalId?: string } }, +): Promise<(PendingApprovalRecord & { requestId: string }) | null> { + const id = d.destination.commandApprovalId!; + const approval = (await core.getApproval(id)) as (PendingApprovalRecord & { requestId: string }) | null; + const requester = approvalDeliveryRecipient(approval?.request?.actor as { externalId?: string } | undefined); + if (!approval?.request || !requester || d.idempotencyKey !== approvalDeliveryKey(id, approval)) return null; + const expected = approvalDestination(approval.request, requester); + const matches = + d.destination.type === "principal" + ? samePerson(requester, d.destination.target) + : expected.type === d.destination.type && expected.target === d.destination.target; + return matches ? approval : null; +} diff --git a/test/approval-store.test.ts b/test/approval-store.test.ts index bf5cca20bb..9434a5d9da 100644 --- a/test/approval-store.test.ts +++ b/test/approval-store.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { createApprovalStore } from "../src/core/approval-store.ts"; +import { approvalDestination, createApprovalStore } from "../src/core/approval-store.ts"; import { createMemoryMap } from "../src/persistence/durable-map.ts"; import { createDeliveryStore } from "../src/delivery/delivery-store.ts"; import type { PendingApprovalRecord } from "../src/types.ts"; @@ -93,13 +93,32 @@ test("system approvals stay pending without enqueuing or reviving an impossible assert.equal((await deliveries.get(stale.id))?.expiredAt, expiredAt, "a sweep must not revive the stale DM"); }); -test("scheduled-task approvals reach the task owner instead of failing silently", async () => { +test("scheduled-task approvals post at the task's own destination, like credential cards", async () => { + const channel = { type: "slack", target: "C9:100.200", audienceScopeId: "channel:C9" as const }; const deliveries = createDeliveryStore(); const approvals = createApprovalStore(createMemoryMap(), deliveries); - const approval = record("cron:K1"); - approval.request!.surface = "cron"; - await approvals.put("A1", approval); - const pending = await deliveries.pending("principal"); - assert.equal(pending.length, 1); - assert.equal(pending[0]!.destination.commandApprovalId, "A1"); + const toChannel = record("cron:K1"); + toChannel.request!.surface = "cron"; + toChannel.request!.triggerDestination = channel; + await approvals.put("A1", toChannel); + const [channelCard] = await deliveries.pending("slack"); + assert.equal(channelCard?.destination.target, "C9:100.200"); + assert.equal(channelCard?.destination.commandApprovalId, "A1"); + + const noDestination = record("cron:K2"); + noDestination.request!.surface = "cron"; + await approvals.put("A2", noDestination); + const ownerCards = await deliveries.pending("principal"); + assert.equal(ownerCards.length, 1, "no destination falls back to the owner's DM"); + assert.equal(ownerCards[0]!.destination.target, "U1"); + + const otherPerson = record("cron:K3"); + otherPerson.request!.surface = "cron"; + otherPerson.request!.triggerDestination = { type: "principal", target: "U2", audienceScopeId: "personal:U2" }; + assert.equal(approvalDestination(otherPerson.request!, "U1").target, "U1", "never DMs someone else's card"); + + const web = record("cron:K4"); + web.request!.surface = "cron"; + web.request!.triggerDestination = { type: "web", target: "s1" } as never; + assert.equal(approvalDestination(web.request!, "U1").target, "U1"); }); diff --git a/test/slack-deliveries.test.ts b/test/slack-deliveries.test.ts index e02383cc16..13fab299b3 100644 --- a/test/slack-deliveries.test.ts +++ b/test/slack-deliveries.test.ts @@ -385,3 +385,25 @@ for (const type of ["slack", "group", "principal"]) { assert.deepEqual(recovered.acknowledgements, ["D1"]); }); } + +test("a scheduled task's approval card renders at the task's destination, and only there", async () => { + const at = { type: "slack", target: "C1:100.200" }; + const approval = { + requestId: "A1", + command: "apps share x public:true", + reason: "approval", + grantModes: { session: false, always: false }, + request: { surface: "cron", actor: { externalId: "U1" }, triggerDestination: at }, + }; + const out = await deliver({ commandApprovalId: "A1" }, undefined, undefined, "Approval needed", { approval }); + assert.equal(out.posts.length, 1); + assert.match(JSON.stringify(out.posts[0]!.blocks), /hilo_allow_once/); + const elsewhere = await deliver( + { target: "C2:1.2", commandApprovalId: "A1" }, + undefined, + undefined, + "Approval needed", + { approval }, + ); + assert.equal(elsewhere.posts.length, 0, "a card never lands in a channel the task does not post to"); +}); From 9ba2d40d362d51d9be272da2c4948fc8352c1504 Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sun, 4 Oct 2026 02:26:03 +0000 Subject: [PATCH 5/7] Show held approvals in the requesting thread and parent session --- src/api/app-turn.ts | 8 +- src/core/approval-store.ts | 14 +- test/approval-store.test.ts | 14 ++ .../scenarios-visibility-approval.ts | 144 ++++++++++++++++++ test/live-slack/scenarios.ts | 2 + test/session-metadata-authz.test.ts | 15 ++ 6 files changed, 194 insertions(+), 3 deletions(-) create mode 100644 test/live-slack/scenarios-visibility-approval.ts diff --git a/src/api/app-turn.ts b/src/api/app-turn.ts index ac60943fe5..3892a2c49a 100644 --- a/src/api/app-turn.ts +++ b/src/api/app-turn.ts @@ -689,7 +689,13 @@ export function createTurnMethods( async listSessionApprovals(sessionId, viewer) { if (!(await sessionForViewer(sessionId, viewer))) return []; - return pendingApprovalForSession(sessionId, { blockingOnly: false, viewer }); + const children = (await deps.sessions.childrenOf?.(sessionId)) ?? []; + const lists = await Promise.all( + [sessionId, ...children.map((c) => c.id)].map((id) => + pendingApprovalForSession(id, { blockingOnly: false, viewer }), + ), + ); + return lists.flat(); }, pendingApprovalForThread(threadRef, viewer) { diff --git a/src/core/approval-store.ts b/src/core/approval-store.ts index f71a2a050e..102a955d03 100644 --- a/src/core/approval-store.ts +++ b/src/core/approval-store.ts @@ -4,7 +4,7 @@ import type { DurableMap } from "../persistence/durable-map.ts"; import type { DeliveryStore } from "../delivery/delivery-store.ts"; import { principalDestination } from "../reach/reach.ts"; import { samePerson } from "../directory/person.ts"; -import type { Destination } from "../types.ts"; +import { scopeId, type Destination } from "../types.ts"; export function approvalDeliveryKey(id: string, record: Pick): string { return `command-approval:${id}:${record.createdAt ?? 0}`; @@ -16,9 +16,19 @@ export function approvalDeliveryRecipient(actor: { externalId?: string } | undef } export function approvalDestination( - request: Pick, "surface" | "triggerDestination">, + request: Pick< + NonNullable, + "surface" | "triggerDestination" | "conversation" | "deliveryTarget" + >, actorId: string, ): Destination { + const c = request.conversation; + if (request.surface === "slack" && c?.kind !== "dm" && c?.channelRef && request.deliveryTarget) + return { + type: "slack", + target: request.deliveryTarget, + audienceScopeId: scopeId(c.kind === "group" ? "group" : "channel", c.channelRef), + }; const d = request.surface === "cron" ? request.triggerDestination : undefined; if (d && d.type !== "web" && (d.type !== "principal" || samePerson(d.target, actorId))) return d; return principalDestination(actorId, actorId); diff --git a/test/approval-store.test.ts b/test/approval-store.test.ts index 9434a5d9da..34844b224a 100644 --- a/test/approval-store.test.ts +++ b/test/approval-store.test.ts @@ -122,3 +122,17 @@ test("scheduled-task approvals post at the task's own destination, like credenti web.request!.triggerDestination = { type: "web", target: "s1" } as never; assert.equal(approvalDestination(web.request!, "U1").target, "U1"); }); + +test("Slack channel approvals post in the requesting thread; DMs stay in the DM", async () => { + const thread = record("slack:C1:1.0"); + thread.request!.conversation = { kind: "channel", threadRef: "slack:C1:1.0", channelRef: "C1" }; + thread.request!.deliveryTarget = "C1:1.0"; + assert.deepEqual(approvalDestination(thread.request!, "U1"), { + type: "slack", + target: "C1:1.0", + audienceScopeId: "channel:C1", + }); + const dm = record("dm:D1"); + dm.request!.deliveryTarget = "D1"; + assert.equal(approvalDestination(dm.request!, "U1").type, "principal"); +}); diff --git a/test/live-slack/scenarios-visibility-approval.ts b/test/live-slack/scenarios-visibility-approval.ts new file mode 100644 index 0000000000..25e280a77f --- /dev/null +++ b/test/live-slack/scenarios-visibility-approval.ts @@ -0,0 +1,144 @@ +import { createHmac } from "node:crypto"; +import { assert, type Actor, type Ctx, type Env, type Scenario } from "./harness.ts"; +import { sleep, type SlackMessage } from "./slack.ts"; + +const EVENTS_URL = () => + process.env.SLACK_EVENTS_TARGET_URL ?? `http://127.0.0.1:${process.env.SLACK_EVENTS_PORT ?? "8182"}/slack/events`; + +async function waitFor(label: string, fn: () => Promise, timeoutMs = 360_000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const v = await fn().catch(() => undefined); + if (v !== undefined) return v; + await sleep(3000); + } + throw new Error(`timed out waiting for ${label}`); +} + +const buttons = (m: SlackMessage): Array<{ action_id: string; value: string; block_id?: string }> => { + const block = (m.blocks ?? []).find((b) => b.type === "actions") as + { block_id?: string; elements?: Array<{ action_id: string; value: string }> } | undefined; + return (block?.elements ?? []).map((e) => ({ ...e, block_id: block?.block_id })); +}; + +const isCard = (env: Env, app: string) => (m: SlackMessage) => + m.user === env.botUserId && + buttons(m).some((b) => b.action_id === "hilo_allow_once") && + (JSON.stringify(m.blocks ?? []) + (m.text ?? "")).includes(app); + +async function click(env: Env, clicker: Actor, channel: string, card: SlackMessage): Promise { + const action = buttons(card).find((a) => a.action_id === "hilo_allow_once")!; + const body = `payload=${encodeURIComponent( + JSON.stringify({ + type: "block_actions", + team: { id: env.teamId, domain: "e2e" }, + user: { id: clicker.userId, username: clicker.handle, team_id: env.teamId }, + api_app_id: "AE2E", + container: { + type: "message", + message_ts: card.ts, + channel_id: channel, + is_ephemeral: false, + ...(card.thread_ts ? { thread_ts: card.thread_ts } : {}), + }, + channel: { id: channel, name: "c" }, + message: { type: "message", ts: card.ts, text: card.text, user: env.botUserId, thread_ts: card.thread_ts }, + actions: [{ ...action, type: "button", action_ts: `${Date.now() / 1000}` }], + }), + )}`; + const ts = String(Math.floor(Date.now() / 1000)); + const signature = `v0=${createHmac("sha256", process.env.SLACK_SIGNING_SECRET ?? "") + .update(`v0:${ts}:${body}`) + .digest("hex")}`; + const res = await fetch(EVENTS_URL(), { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + "X-Slack-Request-Timestamp": ts, + "X-Slack-Signature": signature, + }, + body, + signal: AbortSignal.timeout(15_000), + }); + assert.equal(res.status, 200); +} + +async function messages(client: Actor["client"], channel: string, root?: string): Promise { + return root + ? [...(await client.replies(channel, root)), ...(await client.history(channel, root))] + : client.history(channel, "0"); +} + +const appPrompt = (name: string) => + `Publish a tiny app named "${name}": a Node server.js that answers "ok" on $PORT, entrypoint "node server.js". Then make it public so anyone with the link can open it. Once it is actually public, post a message containing exactly VIS_DONE and nothing else; never write that token otherwise.`; + +const MODES = ["thread", "dm", "subagent", "cron"] as const; +const ACTOR = { thread: "alice", dm: "bob", subagent: "carol", cron: "alice" } as const; + +export const visibilityApprovalScenarios: Scenario[] = MODES.map((mode) => ({ + name: `visibility-approval-card-${mode}`, + lane: "parallel", + tags: ["twin", "apps"], + actors: [ACTOR[mode]], + timeoutMs: 20 * 60_000, + async run(ctx: Ctx) { + const env = ctx.env; + const owner = ctx.actor(ACTOR[mode]); + const name = `vis-${mode}-${env.runId.slice(-6)}`; + const ownerDm = await owner.client.openDm(env.botUserId); + const since = String(Date.now() / 1000); + let channel: string; + let root: string | undefined; + if (mode === "dm") { + channel = ownerDm; + await owner.client.post(ownerDm, appPrompt(name)); + } else { + const ch = await ctx.freshChannel(); + channel = ch.id; + const as = ch.as(owner); + if (mode === "thread") root = await as.mention(appPrompt(name)); + else if (mode === "subagent") + root = await as.mention( + `Open a sub-agent session (sessions tool, action open) and have IT do this work, not you: ${appPrompt(name)}`, + ); + else { + root = await as.mention( + `Publish a tiny app named "${name}" (Node server.js answering "ok" on $PORT, entrypoint "node server.js") and keep it private. Then schedule a one-shot cron for 1 minute from now that posts back to this thread and, when it fires, makes "${name}" public and then posts a message containing exactly VIS_DONE and nothing else (never write that token otherwise). Reply SCHEDULED now.`, + ); + } + } + const card = await waitFor("approval card where the request was made", async () => + (await messages(owner.client, channel, root)).find((m) => Number(m.ts) > Number(since) && isCard(env, name)(m)), + ).catch(async (error) => { + const all = (await messages(owner.client, channel, root).catch(() => [])) + .filter((m) => m.user === env.botUserId) + .map((m) => `${JSON.stringify((m.text ?? "").slice(0, 60))}${buttons(m).length ? "[buttons]" : ""}`); + (error as Error).message += ` (bot messages: ${all.join(" | ") || "none"})`; + const inDm = mode !== "dm" && (await owner.client.history(ownerDm, since)).some(isCard(env, name)); + throw new Error(`${(error as Error).message}${inDm ? " (it went to the owner's DM instead)" : ""}`); + }); + const problems: string[] = []; + if (!/public|anyone with the link/i.test(JSON.stringify(card.blocks ?? []) + (card.text ?? ""))) + problems.push("card does not say it makes the app public"); + if (mode !== "dm" && (await owner.client.history(ownerDm, since)).some(isCard(env, name))) + problems.push("card also went to the owner's DM"); + const early = (await messages(owner.client, channel, root)).some( + (m) => m.user === env.botUserId && Number(m.ts) > Number(since) && (m.text ?? "").trim() === "VIS_DONE", + ); + if (early) problems.push("app was reported public before approval"); + await click(env, owner, channel, card); + await waitFor("work to resume after approval", async () => + (await messages(owner.client, channel, root)).some( + (m) => m.user === env.botUserId && Number(m.ts) >= Number(card.ts) && (m.text ?? "").trim() === "VIS_DONE", + ) + ? true + : undefined, + ).catch(async () => { + const seen = (await messages(owner.client, channel, root).catch(() => [])) + .filter((m) => m.user === env.botUserId && Number(m.ts) >= Number(card.ts)) + .map((m) => JSON.stringify((m.text ?? "").slice(0, 80))); + problems.push(`approving did not resume the work (bot after card: ${seen.join(" | ") || "nothing"})`); + }); + assert.deepEqual(problems, [], problems.join("; ")); + }, +})); diff --git a/test/live-slack/scenarios.ts b/test/live-slack/scenarios.ts index c2c4aae034..f2603ada5b 100644 --- a/test/live-slack/scenarios.ts +++ b/test/live-slack/scenarios.ts @@ -7,6 +7,7 @@ import { multiUserScenarios } from "./scenarios-multiuser.ts"; import { twinScenarios } from "./scenarios-twin.ts"; import { deployAccessScenarios } from "./scenarios-deploy-access.ts"; import { credentialApprovalScenarios } from "./scenarios-credential-approval.ts"; +import { visibilityApprovalScenarios } from "./scenarios-visibility-approval.ts"; const RAW_MARKDOWN_ARTIFACTS: Array<[string, RegExp]> = [ ["**bold**", /\*\*[^*\n]+\*\*/], @@ -550,4 +551,5 @@ export const scenarios: Scenario[] = [ ...twinScenarios, ...deployAccessScenarios, ...credentialApprovalScenarios, + ...visibilityApprovalScenarios, ]; diff --git a/test/session-metadata-authz.test.ts b/test/session-metadata-authz.test.ts index 7fd2884800..902e4a3928 100644 --- a/test/session-metadata-authz.test.ts +++ b/test/session-metadata-authz.test.ts @@ -110,3 +110,18 @@ test("a dropped project member cannot discard or retitle a session they can no l assert.ok(await built.sessions.get(sessionId), "the session survives the refused discard"); assert.equal(await built.app.discardSession(sessionId, "owner"), true, "the owner can still discard it"); }); + +test("a sub-agent's held approval is listed inline in its parent session too", async () => { + const built = freshApp(); + const parent = await built.app.turn(dm("plan the work", "web:alice:parent", "alice")); + const child = await built.app.turn(dm("!run git push --force origin main", "web:alice:child", "alice")); + assert.equal(child.status, "pending_approval"); + await built.sessions.setParentSession(child.sessionId!, parent.sessionId!); + const inParent = await built.app.listSessionApprovals(parent.sessionId!, "alice"); + assert.deepEqual( + inParent.map((a) => a.requestId), + (await built.app.listSessionApprovals(child.sessionId!, "alice")).map((a) => a.requestId), + ); + assert.equal(inParent.length, 1); + assert.deepEqual(await built.app.listSessionApprovals(parent.sessionId!, "carol"), [], "outsiders still see nothing"); +}); From 613f7db234ebb54ee3ec89142703e0e0176a8e2d Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sun, 4 Oct 2026 18:59:16 +0000 Subject: [PATCH 6/7] Expect the held approval card in the requesting thread --- test/surface-spine-routing.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/surface-spine-routing.test.ts b/test/surface-spine-routing.test.ts index d3d5573ed1..e2b0493010 100644 --- a/test/surface-spine-routing.test.ts +++ b/test/surface-spine-routing.test.ts @@ -637,7 +637,17 @@ for (const command of ["!finish-silent-approval", "!finish-silent-paused"]) { const result = await built.app.turn({ ...mention(command, "C-approval", command), async: false }); assert.equal(result.status, "pending_approval"); assert.equal(result.pendingApprovals?.length, 1); - assert.equal((await built.deliveries.pending("slack")).length, 0); + const pending = (await built.deliveries.pending("slack")) as Array<{ + destination: { target: string; commandApprovalId?: string }; + }>; + assert.equal(pending.filter((d) => !d.destination.commandApprovalId).length, 0, "silence posts no reply"); + const cards = pending.filter((d) => d.destination.commandApprovalId); + assert.equal(cards.length, 1, "the approval card is the only Slack delivery"); + assert.equal( + cards[0]!.destination.target, + `slack:C-approval:${command}`, + "the card lands in the requesting thread", + ); }); } From 9e7cde6d4505ae4a328e144466fce4917ae45bb3 Mon Sep 17 00:00:00 2001 From: Joshua France Date: Sun, 4 Oct 2026 19:28:27 +0000 Subject: [PATCH 7/7] Keep publishing private and leave sharing to the share action --- skills-seed/publish/SKILL.md | 16 +- src/deploy/deploy-service.ts | 85 -------- src/harness/agent-tools.ts | 15 +- src/tools/primitives.ts | 50 +---- test/agent-tools.test.ts | 4 +- test/deploy-access.test.ts | 22 -- test/publish-default-audience.test.ts | 288 ++++---------------------- test/publish-primitive.test.ts | 2 +- test/share-deployment.test.ts | 75 +------ 9 files changed, 59 insertions(+), 498 deletions(-) diff --git a/skills-seed/publish/SKILL.md b/skills-seed/publish/SKILL.md index 3241688d4b..6533bc9163 100644 --- a/skills-seed/publish/SKILL.md +++ b/skills-seed/publish/SKILL.md @@ -111,19 +111,9 @@ publish: ## Sharing — say who can reach it -Publication uses the conversation's default audience when `audience` is omitted. -Pass `audience: []` to suppress default grants for an owner-only publication; this does -not revoke existing explicit grants. Supply publication-time grants with `audience`: - -``` -apps({ - action: "publish", - dir: "dist", entrypoint: "node server.js", name: "status-board", - audience: [{ scope: "org:acme", permission: "read" }], -}) -``` - -For a later grant, use the app ID or handle: +Publishing is always private to you: a new app is reachable only by its owner, and +republishing never changes who can reach it. Grant access as a separate step with the app +ID or handle: ``` apps({ action: "share", id: "status-board", toScope: "org", permission: "read" }) diff --git a/src/deploy/deploy-service.ts b/src/deploy/deploy-service.ts index aa6f8c43c0..4357339b7b 100644 --- a/src/deploy/deploy-service.ts +++ b/src/deploy/deploy-service.ts @@ -62,12 +62,9 @@ export interface DeployOrUpdateInput { stampEnv?: Record; renameFrom?: string; rollbackTo?: number; - share?: Array<{ scope: ScopeId; permission: Permission }>; createdInScope?: ScopeId; alwaysOn?: boolean; embedAncestors?: string[]; - public?: boolean; - defaultAudience?: { contextScopeId: ScopeId; granteeScopeIds: ScopeId[]; snapshotAt: number; force?: boolean }; } export interface ReachOptions { @@ -166,14 +163,6 @@ function requiredEntrypoint(input: string | undefined, d: Deployment | null): st return entrypoint; } -const REPUBLISH_VISIBILITY_FIXED = - "republishing an existing app cannot change who can reach it; use the app share action to change visibility"; - -function assertNoVisibilityChange(input: DeployOrUpdateInput): void { - if (input.public !== undefined || input.share !== undefined || input.defaultAudience?.force) - throw new Error(REPUBLISH_VISIBILITY_FIXED); -} - export function createDeployService(deps: DeployServiceDeps): DeployService { const leaderLease = deps.leaderLease ?? createNoopLeaderLease(); const advisoryLock = deps.advisoryLock ?? createNoopAdvisoryLock(); @@ -305,65 +294,6 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { return false; } - async function grantDefaultAudience( - d: Deployment, - da: NonNullable, - explicitShares?: DeployOrUpdateInput["share"], - ): Promise { - const ref = deploymentRef(d.id); - const owner = d.createdBy; - for (const grantee of da.granteeScopeIds) { - await deps.acl.grant({ - ownerScopeId: d.ownerScopeId, - ref, - granteeScopeId: grantee, - permission: "read", - grantedBy: owner, - }); - if (!explicitShares?.some((s) => s.scope === grantee)) - await notifyDeploymentShared(deps, d, grantee, "read", owner); - deps.auditLog.record({ - at: Date.now(), - principalId: owner, - action: "deploy_share", - resource: ref, - scopeLabel: grantee, - }); - } - await deps.deployStore.setDefaultAudience(d.id, { - sourceScopeId: d.createdInScope ?? da.contextScopeId, - granteeScopeIds: da.granteeScopeIds, - snapshotAt: da.snapshotAt, - }); - } - - async function issueShares( - d: Deployment, - createdBy: string, - share: NonNullable, - ): Promise { - for (const s of share) { - const granteeScopeId = await deploymentShareScope(s.scope, s.permission, deps.canManageEmail); - await assertShareAllowed(d.ownerScopeId, granteeScopeId, s.permission); - const grant: Grant = { - ownerScopeId: d.ownerScopeId, - ref: deploymentRef(d.id), - granteeScopeId, - permission: s.permission, - grantedBy: createdBy, - }; - await deps.acl.grant(grant); - await notifyDeploymentShared(deps, d, grant.granteeScopeId, grant.permission, createdBy); - deps.auditLog.record({ - at: Date.now(), - principalId: createdBy, - action: "deploy_share", - resource: deploymentRef(d.id), - scopeLabel: s.scope, - }); - } - } - return { providerProfile: deps.provider.profile, assertShareAllowed, @@ -695,14 +625,6 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { async deployOrUpdate(input) { const { ownerScopeId, createdBy } = input; - if (input.public === true && !(await externalSharingAllowed(ownerScopeId))) - throw new Error(EXTERNAL_APP_SHARING_OFF); - for (const s of input.share ?? []) - await assertShareAllowed( - ownerScopeId, - await deploymentShareScope(s.scope, s.permission, deps.canManageEmail), - s.permission, - ); if (input.embedAncestors !== undefined && !parseEmbedAncestors(input.embedAncestors)) throw new Error(`embedAncestors must be an ${EMBED_ANCESTORS_HINT}`); @@ -713,7 +635,6 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { if (!existing) throw new Error(`no deployment named: ${input.renameFrom}`); if (!(await canManage(existing, ownerScopeId, createdBy, input.createdInScope))) throw new Error(`not authorized to manage deployment: ${input.renameFrom}`); - assertNoVisibilityChange(input); const clash = await deps.deployStore.getByName(input.name); if (clash && clash.id !== existing.id) throw new Error(`deployment name taken: ${input.name}`); await deps.deployStore.setName(existing.id, input.name); @@ -746,7 +667,6 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { if (!existing) throw new Error(`no deployment named: ${input.name}`); if (!(await canManage(existing, ownerScopeId, createdBy, input.createdInScope))) throw new Error(`not authorized to manage deployment: ${input.name}`); - assertNoVisibilityChange(input); await this.rollbackDeployment( existing.id, input.rollbackTo, @@ -766,7 +686,6 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ) { throw new Error(`deployment name taken: ${input.name}`); } - assertNoVisibilityChange(input); const entrypoint = requiredEntrypoint(input.entrypoint, existing); await this.redeploy(existing.id, { entrypoint, @@ -792,11 +711,7 @@ export function createDeployService(deps: DeployServiceDeps): DeployService { ...(input.stampEnv ? { stampEnv: input.stampEnv } : {}), ...(input.alwaysOn !== undefined ? { alwaysOn: input.alwaysOn } : {}), }); - if (input.public !== undefined) await this.setDeploymentPublic(d.id, input.public, { createdBy }); - if (input.defaultAudience) - await grantDefaultAudience((await deps.deployStore.get(d.id))!, input.defaultAudience, input.share); if (input.embedAncestors !== undefined) await this.setDeploymentEmbedAncestors(d.id, input.embedAncestors); - if (input.share?.length) await issueShares((await deps.deployStore.get(d.id))!, createdBy, input.share); return (await deps.deployStore.get(d.id))!; }, diff --git a/src/harness/agent-tools.ts b/src/harness/agent-tools.ts index 718effb6ac..a35abe65f3 100644 --- a/src/harness/agent-tools.ts +++ b/src/harness/agent-tools.ts @@ -1145,10 +1145,8 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): "directory exists and contains files. For a new app or a code/file update, always pass `entrypoint`; " + "the app must listen on the PORT env var. `dir` is workspace-relative: use `app`, never a path " + "beginning with `/` or a redundant `workspace/app`. `renameFrom` takes an existing " + - "deployment name, not its ID. On a first publish, set audience to [] to suppress default audience grants, or supply " + - "publication-time grants; `public: true` makes the app reachable without sign-in, is never the default, and is refused unless an org admin has enabled external app sharing. " + - "Republishing, renaming or rolling back an existing app ships code only and never changes who can reach it: `audience` and `public` are refused there. " + - "Change an existing app's visibility only with apps action share. Share the full absolute URL " + + "deployment name, not its ID. Publishing is always private to the owner and never changes who can reach an app; " + + "grant access separately with apps action share. Share the full absolute URL " + "returned by apps action publish so it works in Slack and other surfaces. Use `name` for a friendly, " + "stable link /d//; `renameFrom` to rename; `rollbackTo` to flip back to an earlier version. " + "Egress is open, " + @@ -1204,12 +1202,6 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): rollbackTo: Type.Optional( Type.Integer({ description: "Flip the deployment named `name` back to this version number." }), ), - public: Type.Optional( - Type.Boolean({ - description: - "First publish only: set whether anyone with the link can open the app without signing in. New apps are private by default. Refused when republishing; change an existing app with apps action share.", - }), - ), alwaysOn: Type.Optional( Type.Boolean({ description: @@ -1234,7 +1226,8 @@ export function createAgentTools(ref: ToolContextRef, opts?: AgentToolsOptions): name: params.name, }); try { - const r = await tc.publish({ ...params, share: params.audience } as PublishInput); + const legacy = params as { audience?: PublishInput["share"] }; + const r = await tc.publish({ ...params, share: legacy.audience } as PublishInput); const reach = describePublishAudience(r.audience); const alwaysOnNote = r.alwaysOn ? "\nAlways-on: the app is kept warm — no idle cold starts." : ""; const embedNote = r.embedAncestors?.length ? `\nEmbeddable by: ${r.embedAncestors.join(", ")}` : ""; diff --git a/src/tools/primitives.ts b/src/tools/primitives.ts index 467504556a..a187a589a9 100644 --- a/src/tools/primitives.ts +++ b/src/tools/primitives.ts @@ -25,11 +25,7 @@ import type { WorkspaceLayer, } from "../types.ts"; import { parseScopeId, scopeId } from "../types.ts"; -import { - defaultPublishAudience, - type PublishAudience, - type PublishAudienceKind, -} from "../resolution/publish-audience.ts"; +import { type PublishAudienceKind } from "../resolution/publish-audience.ts"; import type { WorkspaceStore } from "../workspace/workspace-store.ts"; import type { BotPolicy } from "../surface-cache/channel-policy-store.ts"; import type { GapPhase, GapWork } from "../sessions/session-store.ts"; @@ -1025,6 +1021,8 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { async publish(input: PublishInput): Promise { if (!writableScopeId) throw new Error("publish needs a writable scope to own the app"); + if (input.share !== undefined || input.public !== undefined) + throw new Error("publish is always private to the owner; use apps action share to grant access"); const owner: ScopeId = scopeId("personal", deps.createdBy); const createdInScope: ScopeId = writableScopeId; let effectiveEntrypoint = input.entrypoint; @@ -1057,30 +1055,6 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { ) : {}; - const pc = deps.publishContext; - const aud: PublishAudience = - pc && orgScopeId - ? defaultPublishAudience({ - kind: pc.conversationKind, - ...(pc.isPrivate !== undefined ? { isPrivate: pc.isPrivate } : {}), - ...(pc.isMpim !== undefined ? { isMpim: pc.isMpim } : {}), - ...(pc.publishMembers ? { members: pc.publishMembers } : {}), - orgScopeId, - ownerId: deps.createdBy, - }) - : { kind: "owner", grantees: [] }; - const optOut = Array.isArray(input.share) && input.share.length === 0; - const desiredDefault = optOut ? [] : aud.grantees; - const doReconcile = effectiveEntrypoint !== undefined && (optOut || !aud.incomplete); - const snapshotAt = Date.now(); - const resolvedShare = input.share?.map((s) => { - const scope = s.scope === "org" ? orgScopeId : s.scope; - if (!scope) throw new Error('cannot resolve "org" — no org scope is mounted in this session'); - if (parseScopeId(scope).kind === null) { - throw new Error(`invalid share target "${s.scope}" — use "org" or a scope id like personal: or org:`); - } - return { scope, permission: s.permission }; - }); return once(async () => { const d = await deps.deploy.deployOrUpdate({ ownerScopeId: owner, @@ -1095,29 +1069,13 @@ export function createToolContext(deps: ToolContextDeps): ToolContext { ...(input.rollbackTo !== undefined ? { rollbackTo: input.rollbackTo } : {}), ...(input.alwaysOn !== undefined ? { alwaysOn: input.alwaysOn } : {}), ...(input.embedAncestors !== undefined ? { embedAncestors: input.embedAncestors } : {}), - ...(input.public !== undefined ? { public: input.public } : {}), - ...(doReconcile - ? { - defaultAudience: { - contextScopeId: createdInScope, - granteeScopeIds: desiredDefault, - snapshotAt, - ...(optOut ? { force: true } : {}), - }, - } - : {}), - ...(resolvedShare?.length ? { share: resolvedShare } : {}), }); const ref = d.name ?? d.id; const grantees = await deps.deploy.deploymentGrantees(d.id); - const base = audienceFromGrantees( + const audience = audienceFromGrantees( grantees.map((g) => g.scope), d.createdInScope, ); - const audience: PublishAudienceDescriptor = - aud.incomplete && effectiveEntrypoint !== undefined && input.share === undefined && aud.reason - ? { ...base, note: aud.reason } - : base; const urlBase = deps.publicWebUrl?.replace(/\/$/, "") ?? ""; const url = publicUrlOf(d.endpoint) ?? `${urlBase}/d/${ref}/`; const dataDir = effectiveEntrypoint ? deps.deploy.providerProfile?.dataDir : undefined; diff --git a/test/agent-tools.test.ts b/test/agent-tools.test.ts index 6a53561b58..6a5c2ad09f 100644 --- a/test/agent-tools.test.ts +++ b/test/agent-tools.test.ts @@ -3684,7 +3684,7 @@ test("a files read approval cannot authorize writes or sharing", async () => { } }); -test("apps preserves publication-time audience opt-out without control-plane tools", async () => { +test("apps publish forwards a legacy audience so it is refused, not silently dropped", async () => { const inputs: unknown[] = []; const tc = { ...fakeToolContext(), @@ -3696,8 +3696,6 @@ test("apps preserves publication-time audience opt-out without control-plane too const apps = createAgentTools({ current: tc }).find((tool) => tool.name === "apps")!; await call(apps, { action: "publish", name: "private", audience: [] }); assert.deepEqual((inputs[0] as { share: unknown }).share, []); - await call(apps, { action: "publish", audience: [{ scope: "personal:bob", permission: "read" }] }); - assert.deepEqual((inputs[1] as { share: unknown }).share, [{ scope: "personal:bob", permission: "read" }]); }); test("files share preserves artifact IDs, recipient resolution and authorization failures", async () => { diff --git a/test/deploy-access.test.ts b/test/deploy-access.test.ts index e554bc55db..2eaf4478db 100644 --- a/test/deploy-access.test.ts +++ b/test/deploy-access.test.ts @@ -255,25 +255,3 @@ test("shared-home members can approve and decline, outsiders cannot", async (t) assert.match(await f.decide("carol@example.com"), /^Approved\./); await assert.rejects(f.decide(requester, false), /owner/); }); - -test("publish audiences notify once with final permissions, including subsequent direct shares", async (t) => { - const f = await fixture(t); - const d = await f.deploy.deployOrUpdate({ - ...publishInput(owner, "published"), - share: [{ scope: person(requester), permission: "write" }], - defaultAudience: { - contextScopeId: person(owner), - granteeScopeIds: [person(requester), person("carol@example.com")], - snapshotAt: Date.now(), - }, - }); - const notices = await f.notices(); - assert.equal(notices.length, 2); - const toRequester = notices.filter((n) => n.destination.target === requester); - assert.equal(toRequester.length, 1); - assert.match(toRequester[0]!.text, /manage it.*https:\/\/published.apps.example.com\//); - const value = JSON.stringify({ deploymentId: d.id, requesterId: requester }); - await decideDeploymentAccess(f.app, f.identity, value, { externalId: owner }, true); - await f.app.shareDeployment(d.id, person(requester), "write", { createdBy: owner }); - assert.equal((await f.notices()).length, 2); -}); diff --git a/test/publish-default-audience.test.ts b/test/publish-default-audience.test.ts index 5a9339c9cb..f6546f4922 100644 --- a/test/publish-default-audience.test.ts +++ b/test/publish-default-audience.test.ts @@ -112,7 +112,7 @@ test("D1: a channel publish is owned by personal:, channel recorded a const d = (await deployStore.getByName("site"))!; assert.equal(d.ownerScopeId, scopeId("personal", "U1"), "owned by the initiator, not the channel"); assert.equal(d.createdInScope, scopeId("channel", "C1"), "channel recorded as origin metadata"); - assert.equal(r.audience?.kind, "org"); + assert.equal(r.audience?.kind, "owner", "a new app is private to its owner"); }); test("D1: owner always reaches their own app; canManage recognizes the owner acting from the channel", async () => { @@ -174,269 +174,59 @@ test("Defect-2: acting in a different channel than the one the app was created i ); }); -test("D2: public channel publish is reachable org-wide via an org read grant", async () => { +test("first publish is owner-only in every conversation kind", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - }); - assert.equal( - (await deploy.reachDeployment("site", "U-anyone")).status, - "ok", - "any verified principal reaches a public-channel app", - ); -}); - -test("D2: private channel publish grants current internal members by default", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - const r = await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal(r.audience?.kind, "members"); - assert.equal(r.audience?.memberCount, 1); - assert.equal((await deploy.reachDeployment("priv", "U1")).status, "ok"); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok"); -}); - -test("D2: private channel grants exclude the owner", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - const r = await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal(r.audience?.kind, "members"); - assert.equal(r.audience?.memberCount, 2); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok"); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "ok"); - assert.equal((await deploy.reachDeployment("priv", "U9")).status, "denied"); -}); - -test("D3: republish from the originating channel never changes reach — joiners not added, leavers kept", async () => { + const cases = [ + channel(deploy, acl, config, "U1", "C1", { isPrivate: false }), + channel(deploy, acl, config, "U1", "C2", { isPrivate: true, members: [internal("U1"), internal("U2")] }), + ctxFor(deploy, acl, config, { + actor: "U1", + contextScope: scopeId("group", "G1"), + kind: "group", + members: [internal("U1"), internal("U2")], + }), + ]; + for (const [i, ctx] of cases.entries()) { + const r = await ctx.publish({ entrypoint: "node server.js", name: `app-${i}` }); + assert.equal(r.audience?.kind, "owner"); + assert.deepEqual(await deploy.deploymentGrantees(`app-${i}`), []); + assert.equal((await deploy.getDeployment(`app-${i}`))?.public, undefined); + assert.notEqual((await deploy.reachDeployment(`app-${i}`, "U2")).status, "ok"); + } +}); + +test("publish refuses visibility inputs and points to the share action", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - const r = await channel(deploy, acl, config, "U1", "C2", { - isPrivate: false, - members: [internal("U1"), internal("U2"), internal("U4")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal(r.version, 2); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "ok", "U3 kept"); - assert.equal((await deploy.reachDeployment("priv", "U4")).status, "denied", "U4 not added"); - assert.equal((await deploy.reachDeployment("priv", "U-anyone")).status, "denied", "no org widening"); -}); - -test("republish refuses audience changes, including an empty opt-out; sharing changes reach separately", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [], - }); - for (const share of [[{ scope: orgScope, permission: "read" as const }], []]) + const ctx = channel(deploy, acl, config, "U1", "C1", { isPrivate: false }); + for (const extra of [ + { public: true }, + { public: false }, + { share: [] }, + { share: [{ scope: orgScope, permission: "read" as const }] }, + ]) { await assert.rejects( - () => - channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share, - }), - /cannot change who can reach it/, + () => ctx.publish({ entrypoint: "node server.js", name: "site", ...extra }), + /apps action share/, ); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "denied"); - const id = (await deploy.getDeployment("site"))!.id; - await deploy.shareDeployment(id, orgScope, "read", { createdBy: "U1" }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); -}); - -test("D3: a redeploy from a DM/other context never shrinks or shifts the default audience", async () => { - const { deploy, deployStore, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - const dmCtx = ctxFor(deploy, acl, config, { actor: "U1", contextScope: scopeId("personal", "U1"), kind: "dm" }); - await dmCtx.publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok"); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "ok"); - assert.equal((await deployStore.getByName("priv"))!.currentVersion, 2, "the redeploy still shipped a version"); + } + assert.equal(await deploy.getDeployment("site"), null); }); -test("D3: an incomplete enumeration leaves the prior audience FROZEN (no silent revoke)", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok"); - const r = await channel(deploy, acl, config, "U1", "C2", { isPrivate: true }).publish({ - entrypoint: "node server.js", - name: "priv", - }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok", "U2's reach is left frozen, not revoked"); - assert.equal(r.audience?.kind, "members"); - assert.equal(r.audience?.memberCount, 1); -}); - -test("D7: a redeploy from a DM reports the FROZEN channel audience, not this turn's owner-only", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - const dmCtx = ctxFor(deploy, acl, config, { actor: "U1", contextScope: scopeId("personal", "U1"), kind: "dm" }); - const r = await dmCtx.publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal(r.audience?.kind, "members", "the reply states the real (frozen) channel reach, not owner-only"); - assert.equal(r.audience?.memberCount, 2); - assert.equal(r.audience?.channelRef, "C2"); -}); - -test("D3: an UNKNOWN privacy signal (e.g. flaky conversations.info) FREEZES — never revokes a public app's org reach", async () => { +test("sharing after publish grants access separately", async () => { const { deploy, acl } = svc(); const config = createMemoryConfigStore(ORG); await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ entrypoint: "node server.js", name: "site", }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); - const r = await channel(deploy, acl, config, "U1", "C1", { members: [internal("U1"), internal("U2")] }).publish({ - entrypoint: "node server.js", - name: "site", - }); - assert.equal( - (await deploy.reachDeployment("site", "U-anyone")).status, - "ok", - "org reach frozen, not silently revoked", - ); - assert.match(r.audience?.note ?? "", /public\/private|leaving reach unchanged/); -}); - -test("D4: a first publish whose member enumeration is incomplete reports a user-visible 'share manually' note", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - const r = await channel(deploy, acl, config, "U1", "C2", { isPrivate: true }).publish({ - entrypoint: "node server.js", - name: "priv", - }); - assert.equal(r.audience?.kind, "owner"); - assert.match(r.audience?.note ?? "", /enumerate|share manually/); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "denied", "no silent partial grant"); -}); - -test("D3: a redeploy from a DIFFERENT channel never shifts reach to the wrong channel's members", async () => { - const { deploy, deployStore, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C2", { - isPrivate: true, - members: [internal("U1"), internal("U2"), internal("U3")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - await channel(deploy, acl, config, "U1", "C3", { - isPrivate: true, - members: [internal("U1"), internal("U7"), internal("U8")], - }).publish({ entrypoint: "node server.js", name: "priv" }); - assert.equal((await deploy.reachDeployment("priv", "U2")).status, "ok", "origin members frozen"); - assert.equal((await deploy.reachDeployment("priv", "U3")).status, "ok", "origin members frozen"); - assert.equal((await deploy.reachDeployment("priv", "U7")).status, "denied", "wrong-channel members NOT granted"); - assert.equal((await deploy.reachDeployment("priv", "U8")).status, "denied", "wrong-channel members NOT granted"); - const id = (await deployStore.getByName("priv"))!.id; - const grantees = (await acl.grantsFor(scopeId("personal", "U1"), `deployment:${id}`)) - .map((g) => g.granteeScopeId) - .sort(); - assert.deepEqual( - grantees, - [scopeId("personal", "U2"), scopeId("personal", "U3")].sort(), - "default set unchanged (not shifted to C3)", - ); -}); - -test("D5: an explicit share layers on top of the public default (org + the named person)", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [{ scope: scopeId("personal", "U9"), permission: "write" }], - }); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "ok"); - const after = await channel(deploy, acl, config, "U9", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - }); - assert.equal(after.version, 2, "an explicit write-grant holder may redeploy"); -}); - -test("D5: an explicit EMPTY share opts out → owner-only, even from a public channel", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - const r = await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [], - }); - assert.equal(r.audience?.kind, "owner"); - assert.equal((await deploy.reachDeployment("site", "U-anyone")).status, "denied", "no org grant when opted out"); - assert.equal((await deploy.reachDeployment("site", "U1")).status, "ok"); -}); - -test("D5: only the OWNER may widen — even a write-grant manager cannot re-share (anti-re-share)", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [{ scope: scopeId("personal", "U9"), permission: "write" }], - }); - await assert.rejects( - () => - channel(deploy, acl, config, "U9", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [{ scope: scopeId("personal", "U7"), permission: "read" }], - }), - /cannot change who can reach it/, - ); -}); - -test("Alternatives: opt-in collective management — an explicit channel write grant lets any member manage", async () => { - const { deploy, deployStore, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "team-tool", - share: [{ scope: scopeId("channel", "C2"), permission: "write" }], - }); - const r = await channel(deploy, acl, config, "U2", "C2", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "team-tool", - }); - assert.equal(r.version, 2, "a channel member manages via the opt-in channel write grant"); - assert.equal((await deployStore.getByName("team-tool"))!.ownerScopeId, scopeId("personal", "U1")); -}); - -test("D5: a member outside the creation channel cannot redeploy-to-share the owner's app", async () => { - const { deploy, acl } = svc(); - const config = createMemoryConfigStore(ORG); - await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ + await deploy.shareDeployment("site", scopeId("personal", "U2"), "read", { createdBy: "U1" }); + assert.equal((await deploy.reachDeployment("site", "U2")).status, "ok"); + const again = await channel(deploy, acl, config, "U1", "C1", { isPrivate: false }).publish({ entrypoint: "node server.js", name: "site", }); - await assert.rejects( - () => - channel(deploy, acl, config, "U2", "C2", { isPrivate: false }).publish({ - entrypoint: "node server.js", - name: "site", - share: [{ scope: orgScope, permission: "read" }], - }), - /name taken/, - ); + assert.equal(again.version, 2); + assert.deepEqual(await deploy.deploymentGrantees("site"), [{ scope: scopeId("personal", "U2"), permission: "read" }]); }); diff --git a/test/publish-primitive.test.ts b/test/publish-primitive.test.ts index 990fa99f26..4f751626c2 100644 --- a/test/publish-primitive.test.ts +++ b/test/publish-primitive.test.ts @@ -282,8 +282,8 @@ test("publish: renameFrom moves the link but keeps the id (and shares ride the i const created = await ctx(s.deploy, { files: appFile() }).publish({ entrypoint: "x", name: "old-name", - share: [{ scope: scopeId("personal", "U2"), permission: "read" }], }); + await s.deploy.shareDeployment(created.id, scopeId("personal", "U2"), "read", { createdBy: "U1" }); const renamed = await ctx(s.deploy).publish({ renameFrom: "old-name", name: "new-name" }); assert.equal(renamed.id, created.id); diff --git a/test/share-deployment.test.ts b/test/share-deployment.test.ts index ae6c57f5e9..a67d9b86e9 100644 --- a/test/share-deployment.test.ts +++ b/test/share-deployment.test.ts @@ -419,36 +419,17 @@ function toolCtx(deploy: DeployService): ToolContext { } as never); } -test('publish share:[{scope:"org"}] resolves to the org — truthful readback, real reach (the QM bug)', async () => { - const { deploy } = makeDeploy(); - const r = await toolCtx(deploy).publish({ - entrypoint: "x", - name: "wide", - share: [{ scope: "org", permission: "read" }], - }); - assert.equal(r.audience?.kind, "org", "an org share is reported as org, not owner-only"); - assert.ok(!r.audience?.note, "no scary owner-only note when reach was actually granted"); - assert.equal((await deploy.reachDeployment("wide", "U2")).status, "ok"); -}); - -test("publish keeps apps private by default and requires an explicit public opt-in", async () => { +test("publish keeps apps private by default and refuses a public flag at publish time", async () => { const { deploy } = makeDeploy(); const privateApp = await toolCtx(deploy).publish({ entrypoint: "x", name: "private-app" }); assert.equal(privateApp.public, undefined); assert.equal((await deploy.getDeployment("private-app"))?.public, undefined); - const publicApp = await toolCtx(deploy).publish({ entrypoint: "x", name: "public-app", public: true }); - assert.equal(publicApp.public, true); - assert.equal((await deploy.getDeployment("public-app"))?.public, true); -}); - -test("publish rejects a garbage share target instead of silently creating a dead grant", async () => { - const { deploy } = makeDeploy(); await assert.rejects( - () => - toolCtx(deploy).publish({ entrypoint: "x", name: "bad", share: [{ scope: "not-a-scope", permission: "read" }] }), - /invalid share target/, + () => toolCtx(deploy).publish({ entrypoint: "x", name: "public-app", public: true }), + /apps action share/, ); + assert.equal(await deploy.getDeployment("public-app"), null); }); test("transferDeploymentOwner re-homes the app to the teammate: they own it, prior grants survive, the giver keeps reach", async () => { @@ -493,18 +474,6 @@ test("a manage grantee may redeploy but cannot make the owner's app public", asy name: "managed-private", }); await deploy.shareDeployment(d.id, scopeId("personal", "U2"), "write", { createdBy: "U1" }); - await assert.rejects( - () => - deploy.deployOrUpdate({ - ownerScopeId: scopeId("personal", "U2"), - createdBy: "U2", - name: "managed-private", - entrypoint: "x", - files: [], - public: true, - }), - /cannot change who can reach it/, - ); assert.equal((await deploy.getDeployment(d.id))?.public, undefined); }); @@ -514,14 +483,6 @@ test("republish, rename and rollback never change visibility; only the separate const d = await deploy.deployOrUpdate({ ...owner, name: "vis" }); assert.equal(d.public, undefined, "first publish is private by default"); await deploy.deployOrUpdate({ ...owner, name: "vis" }); - for (const extra of [ - { name: "vis", public: true }, - { name: "vis", public: false }, - { name: "vis", share: [{ scope: scopeId("personal", "U2"), permission: "read" as const }] }, - { name: "vis2", renameFrom: "vis", public: true }, - { name: "vis", rollbackTo: 1, public: true }, - ]) - await assert.rejects(() => deploy.deployOrUpdate({ ...owner, ...extra }), /cannot change who can reach it/); const after = await deploy.getDeployment(d.id); assert.equal(after?.public, undefined); assert.equal(after?.name, "vis"); @@ -529,7 +490,9 @@ test("republish, rename and rollback never change visibility; only the separate await deploy.setDeploymentPublic(d.id, true, { createdBy: "U1" }); assert.equal((await deploy.getDeployment(d.id))?.public, true); await deploy.deployOrUpdate({ ...owner, name: "vis" }); - assert.equal((await deploy.getDeployment(d.id))?.public, true, "a plain republish keeps the existing setting"); + await deploy.deployOrUpdate({ ...owner, name: "vis2", renameFrom: "vis" }); + await deploy.deployOrUpdate({ ...owner, name: "vis2", rollbackTo: 1 }); + assert.equal((await deploy.getDeployment(d.id))?.public, true, "republish, rename and rollback keep the setting"); }); test("transferDeploymentOwner is home authority — a write ('manage') grantee cannot give the app away", async () => { @@ -637,30 +600,6 @@ test("with external app sharing off, public links and outside emails are refused "personal:teammate@acme.test", ]); - await assert.rejects( - deploy.deployOrUpdate({ - ownerScopeId: scopeId("personal", "U1"), - createdBy: "U1", - entrypoint: "x", - files: [], - name: "fresh", - public: true, - }), - /external_app_sharing/, - ); - assert.equal(await deploy.getDeployment("fresh"), null); - await assert.rejects( - deploy.deployOrUpdate({ - ownerScopeId: scopeId("personal", "U1"), - createdBy: "U1", - entrypoint: "x", - files: [], - name: "fresh-share", - share: [{ scope: scopeId("personal", "guest@elsewhere.test"), permission: "read" }], - }), - /external_app_sharing/, - ); - assert.equal(await deploy.getDeployment("fresh-share"), null, "publish refuses outside shares before deploying"); const d = (await app.getDeployment("locked"))!; await assert.rejects( app.grant({