feat(permission-groups): enforce every config key server-side - #7347
Merged
Conversation
Adds a golden corpus and a seeded fuzz loop over `parsePermissionGroupConfig`, written against the current hand-written implementation so a derived one has something to prove itself against. Every row states what a stored `jsonb` value coerces to today; a row that changes in a later diff is a decision someone has to defend rather than a silent regression. Two properties the corpus pins are easy to break by accident: - `typeof [] === 'object'`, so an array-valued column coerces to defaults rather than throwing. A parser built on `z.object()` throws here unless it guards `Array.isArray`. - An emptied allowlist denies everything while `null` allows everything, so the two must never collapse into one another. It also pins a live defect. `allowedIntegrations` and `allowedModelProviders` are the only keys that skip element validation, so a corrupted row coerces to a value `permissionGroupFullConfigSchema` then refuses — the route reading it fails response validation instead of returning a usable allowlist. Filtering non-strings on the way in is fail-closed and removes the class; the test inverts when that lands. Adds the two coverage guards that were missing: the write schema, the defaults and the read schema declare the same keys, and every boolean config key is registered as a platform feature. Neither was asserted, so a key omitted from the write schema would have made an admin checkbox silently no-op on save. Drops the `@/lib/permission-groups/types` mock in the permission-check suite and its hand-copied `DEFAULT_PERMISSION_GROUP_CONFIG`. That module imports only zod and a type, so there was nothing to mock, and the mock's permissive merge was strictly looser than the real parser — those 63 tests were asserting against a fake that could not reproduce a coercion bug. The factory also returned two exports, leaving `FILE_SHARE_AUTH_TYPES` and `PERMISSION_GROUP_CONSTRAINTS` undefined for that module graph.
The config shape was maintained by hand in five parallel places — the write schema, the `PermissionGroupConfig` interface, the defaults, the tolerant parser, and the contract's read schema — plus the platform-feature list. Key order was load-bearing across all of them, because the group editor's dirty check compares stringified configs, so a key added at a different index read as an unsaved change forever. Nothing checked that the write schema had the same keys as the rest, and a key missing there made an admin checkbox silently no-op on save. `PERMISSION_GROUP_FIELDS` now declares each key once, carrying its schema, its default, whether it is server-enforced, and (for a boolean) its editor descriptor. Everything else is projected from it, so declaration order is the wire order by construction rather than by agreement. Two things this could have broken quietly, both pinned by the corpus added in the previous commit: - `z.object().parse([])` throws, but `typeof [] === 'object'`, so an array-valued jsonb column used to coerce to defaults. The guard keeps `Array.isArray` for exactly that row. - `.catch(default)` is whole-value tolerant while the old parser was element-wise. On an allowlist that would have been fail-open: one bad member would yield `null`, and `null` means unrestricted. `tolerantArray` filters instead, so a corrupt member narrows the allowlist. One deliberate behavior change. `allowedIntegrations` and `allowedModelProviders` were the only keys that skipped element validation, so a corrupted row produced a config the read schema then refused — the route reading it failed response validation instead of returning a usable allowlist. They now filter like every other array, which is fail-closed. Type-level assertions live in the source rather than a test because type-check excludes test files; a zod generic degrading to `unknown` would otherwise be invisible, since the runtime values would still be correct while every call site lost its narrowing.
… funnel A permission-group key only means something if a server refuses when it is set. Twelve did not: `hideCopilot`, `hideSecretsTab`, `hideDeployChatbot` and the rest were read in a sidebar filter and nowhere else, so an organization that set one had hidden a nav item, not withheld a capability. The gap was structural — nothing connected "this key is offered to admins" to "something refuses when it is set" — so this adds the connection rather than one more check. Operations already declare their policy as frozen data, so capability joins it: `defineWorkspaceOperation` takes a capability id, and `authorizeWorkspaceOperation` refuses when the caller's group withholds it. One insert covers every surface — internal routes, v2 routes, the Copilot adapter, trusted tools — because all of them funnel through it, including the HEAD probe, which has to answer the same question its GET would or it becomes an existence oracle. Capability is checked after the role check. `requirePermission` throws the refusal the v2 surface conceals as a 404, so asking about capability first would tell a complete outsider which capabilities the organization withholds. It is also the cheaper check, and it names the remedy the caller can actually act on. Two principals pass through, as policy rather than oversight. A workspace API key authorizes as the workspace and has no user, so no group resolves; substituting the key's creator would apply a bystander's group to every caller and break the key when that person left. A deployment run has no subject either, and denying there would 403 every schedule and webhook in the organization the moment a group withheld anything — a deployed workflow runs with the workspace's authority, like any service account, and what it *does* is still gated by the executor. Capabilities are ids with a rule registry, not predicates on the operation: a closure cannot be logged, compared, or read by an audit, and fifty table operations naming one capability should be fifty strings rather than fifty identical functions. Rules split on whether the decision needs a request value; a parameterized one is refused at definition time, because declared on an operation it would silently never fire. `check:permission-group-enforcement` is what stops this recurring. It asserts every capability is reachable, every key claiming capability enforcement is read by a rule, and no key claiming something weaker is — so a key cannot reach the admin editor while still documented as cosmetic. It runs in count-down mode (0/233) until the operations are annotated, and refuses to report success if its own parsers come back empty, since an audit that goes quiet when it breaks is worse than none. Config resolution is memoized per request, keyed on user and workspace and caching the promise so concurrent callers share one query. The gate returns before touching the database when the operation declares nothing or the workspace has no organization, so a personal workspace pays nothing.
Every capability in the registry now has something that refuses when the key behind it is set. An organization that hides Tables, Knowledge Bases, Files, Secrets, Integrations, API keys, the inbox, Chat, or any of the three deploy surfaces gets a 403 from the API rather than a hidden nav item. Most of it is declaration. 163 operations across tables, knowledge, files, secrets, credentials, workflows, MCP and API keys name their capability, and the funnel does the rest — the gate added in the previous commit already covered every surface those operations reach. The table domain fixes its capability in the factories rather than repeating it at ten call sites, since every operation in that module is one capability; the audit reads both that form and the positional one. Four sites are not workspace operations and are gated where they actually run: - Chat is a raw handler, checked after the request parses and before the send is claimed. That also settles the resume stream: with no run created there is nothing for it to replay. - `hideTraceSpans` becomes a projection rather than a refusal — the log stays readable, its trace spans, block I/O and final output do not. Applied before child traces hydrate, so a withheld view does not pay for a cross-workspace join it discards, and by deleting rather than omitting, because the execution-data schema is a passthrough and would otherwise let the fields through. - The inbox routes are raw handlers with inline queries; a shared guard keeps the six of them from drifting. - The auth-mode and tool-kind capabilities are annotated at the validators that already enforce them, which is what lets the audit prove every capability is reachable rather than assuming it. The audit now reports no pending enforcement: nothing can reach the admin editor claiming a restriction it does not apply. 120 operations remain unannotated and are counted, not enforced — they are the domains whose capabilities land with the creation-versus-invocation work. Three test files fail to load on this branch (`create-credential-connection`, `add-workspace-files`, `upload-sessions`); they fail identically on the base commit, and are unrelated to this change.
Two controls were configured and applied to nothing. **Legacy blocks defeated the integration allowlist.** Any block marked `hideFromToolbar` was exempt from access control, which covered 44 blocks — including fully functional superseded versions of Slack, GitHub, Notion, SharePoint and Google Sheets. Legacy `slack` talks to Slack exactly as `slack_v2` does, so an allowlist naming `slack_v2` was satisfied by `slack`, reachable through workflow import, the API, or a Copilot-built workflow. The admin editor filtered out exactly those blocks, so the hole was invisible to the person configuring the allowlist. A superseded block is now judged as the successor its `sunset.replacedBy` names, transitively, so allowing or denying an integration covers every version of it and the editor's single row means what it appears to. The exemption narrows to what it was for: the universal entry point, and a retired block with no successor — that one has no row to be permitted on and nothing to be permitted *as*, so denying it would break older workflows an admin could not rescue. **Enrichments sent row data with the tool denylist not applied.** The per-tool gate keys off the acting user and skips entirely when a call carries none; enrichment runs passed only a workspace. So `deniedTools` blocked a provider when a workflow called it and not when a table enrichment did — the same tool, the same row data, one path governed. The user is now threaded from all three callers, each of which already had one: the table run resolves it for billing attribution, the internal tool surface validated it and then dropped it, and the Copilot tool context carries it. `EnrichmentRunContext.userId` documents why it is load-bearing rather than attribution, since an omission fails open and silently.
`allowPersonalApiKeys` was a workspace column and nothing else, so the policy was all-or-nothing for the whole workspace: an organization could not let one team hold personal keys while another could not. `disablePersonalApiKeys` adds that, and the two combine with AND rather than one overriding the other. The column stays the coarse switch every workspace has, including the ones no group governs; the group key narrows it for one cohort inside an enterprise organization. Either saying no is a no, which is also why the column is checked first — it costs nothing and skips the group lookup entirely. It is not declarable on an operation, and the capability registry says so. Every other capability withholds something about a resource, so an operation opts into it; this one refuses a *principal kind*, and applies to every operation a personal key could reach. It is asserted in the funnel's personal-key branch instead, annotated so the audit can still prove the key is enforced. v1 authorizes in its own middleware rather than through the funnel, so the check is repeated there. Without it the same key v2 refused would keep working against v1 — which is the shape of the coverage gap this whole change exists to remove, so leaving it would have been the same mistake in a smaller place. The settings toggle now reads both layers, so the UI never offers a key type the server will refuse. The key is appended last in the field registry: declaration order is the wire order, and moving an existing key would read as an unsaved change in every open group editor.
The CSV export hands over every execution log the workspace ever recorded, including a column holding the full trace spans, to any member with read access. It was the widest read in the product and the only one with no control of its own — an organization could withhold a single log's trace spans in the UI and still have the whole history downloaded in bulk. `disableLogExport` withholds the export separately from reading a log, because those are different exposures: one payload someone is looking at, versus the entire history in a file. The export also applies the same trace-span projection the detail view does, so the two agree — a group that withholds spans no longer discloses them here. Checked inline rather than through an application use case: this route queries the log tables directly and predates that boundary. Migrating it is worth doing on its own, and is not a reason to leave the export ungoverned until then.
`FORBIDDEN_DETAIL_CODES` generates the v2 `403` description, so the new code has to reach the OpenAPI documents or `check:openapi` fails. That coupling is the point: a refusal a caller can branch on is published rather than left to be discovered by matching on prose.
Adds the thirteen keys the coverage audit found missing, and the rules that give each one meaning. Nothing enforces them yet — the enforcement audit lists all thirteen as pending, which is the point: the registry cannot quietly ship a key that refuses nothing. They divide into the two exposures the audit kept turning up. Extraction: table export, bulk file download, execution cost. Provenance and scope: which connectors may pull an external corpus in, whether a member may create a knowledge base or a table rather than only use one, whether they may attach personal credentials, approve a CLI login, or create a workspace that no existing group would govern. `allowedKnowledgeConnectors` is parameterized on the connector id, which required widening the parameterized rule from an auth mode to any request value. That is the right shape for it: an organization that sanctions Drive rarely sanctions the other sixty, and a connector is the one integration that copies a whole external corpus into the workspace. `maxLoopIterations` is deliberately not here. It is a resource ceiling rather than an access control — nothing is withheld from anyone — so it needs a numeric control the group editor has no affordance for, and folding it in as a boolean would misrepresent it.
`POST /api/mcp/workflow-servers` calls `performCreateWorkflowMcpServer` directly rather than going through `createWorkflowDeploymentServer`, so the `deploy.mcp` capability declared on that operation never fired here. A group that hid MCP deployment stopped the v2 route and not this one. Gating both is what keeps the two doors agreeing. Migrating this handler onto the use case is the better end state and is worth doing on its own; it is not a reason to leave the second door open until then.
…e-key escape
The key CRUD routes are raw handlers with inline queries, so `hideApiKeysTab`
hid the settings tab while `POST /api/workspaces/{id}/api-keys` and
`POST /api/users/me/api-keys` still minted keys.
This is also the mitigation the capability gate's design depends on. A
workspace API key authorizes as the workspace and resolves no permission
group, so the funnel's capability check does not apply to it — deliberately,
since substituting the key's creator would apply a bystander's group to every
caller and break the key when that person left. That leaves one escape: a
governed member minting themselves a workspace key that outranks their own
group. Gating the minting closes it at the door.
Keys that already exist keep working. Revoking those is an admin decision,
not something a policy change should do silently to live integrations.
Personal keys are user-global and belong to no workspace, so they resolve the
organization's default group — the same resolution invitations already use for
an organization-level action.
…ce operations
Annotates 71 `defineWorkspaceOperation` declarations across eleven domains so
the permission-group audit can tell an unreviewed operation from a deliberately
ungoverned one.
Capability mappings:
mcp_servers.* mcp_tools.use registering an MCP server and
storing its credentials was a
side door around the key that
blocks calling MCP tools
mcp_servers.workflow_* deploy.mcp reads included, so a group with
the surface hidden is not still
told what is published on it
skills.* skills.use
custom_tools.* custom_tools.use
chat_deployments.* deploy.chat
chat.send copilot.use
catalog.connector_types.list knowledge.use it enumerates knowledge-base
connectors and nothing else
Declared `'none'` with a reason: the block and tool catalogs (the set the editor
renders at all), memory and function execution (the executor's own per-run work,
where a gate fails runs the group permits rather than withholding anything),
credential groups (an admin-only, entitlement-gated section no key names), the
BYOK inherited-status read, and platform context.
Adds funnel-level refusal tests for MCP, skills, and the catalog split, so a
capability cannot be declared on an operation and then read by nothing.
…nd CLI access Three capabilities reached the admin editor with a checkbox, a hint, and no server gate: an organization that set `disableWorkspaceCreation`, `hideOrgMemberDirectory` or `disableCliAccess` believed it had withheld something while every route still answered. None is workspace-operation shaped, so each is wired at an annotated call site rather than through the declarative funnel. workspace.create — extended `getWorkspaceCreationPolicy` rather than the POST route, so forking and the sidebar's "can I create?" signal are covered by the same decision with no route changes. A new workspace carries no `permissionGroupWorkspace` row, so a scoped-group member creating one lands outside every group targeting them — the cleanest escape from the regime today. The gate resolves the caller's organization even when the resulting workspace would be personal, because a personal workspace is precisely that escape. `blockedReasonCode` gains `'permission-group-denied'`; the one caller that switches on it (`app/workspace/page.tsx`) gets matching copy, since the existing organization branch would have told a blocked member to ask for workspace access they already have. organization.member_directory — `/api/organizations/[id]/members` and `/api/organizations/[id]/roster` gated on bare organization membership, so every member could list every colleague's name and email. Both now consult the organization's default group. No role exemption: the default group governs owners and admins for every other capability, and carving one out here would make this the only key whose meaning depended on who was asking. cli.use — gated at `/api/cli/auth/approve`, the only moment a human is present in the device-auth handoff. The poll route that redeems the approval for an API key is deliberately unauthenticated and is left alone: it has no session to resolve a group against, and re-deciding there would duplicate this check while racing a config change between the two calls. `workspaceId` is set only for platform scope, so a personal-scope login falls back to the organization's default group instead of being the unguarded path. The workspace-level member list (`/api/workspaces/[id]/members`) is deliberately NOT gated. It returns id, name and image for people who already share a workspace with the caller — identities the collaboration surfaces publish continuously anyway (presence cursors, canvas avatars, log actors, the sharing dialog), and it exposes no email at all. Hiding it would blank those surfaces while disclosing the same names over the realtime channel, so the restriction would read as breakage rather than policy. An organization-wide roster of colleagues with their email addresses is a materially different disclosure from "who is in this room with me", and that is what the capability is named for.
`knowledge.create`, `knowledge.upload` and `knowledge.connectors` shipped as declared capabilities with nothing reading them, so an admin who set the matching keys got a checkbox and no refusal. - knowledge.create governs the one operation that opens a knowledge base, so a group may query, populate and organize the bases it has without creating new ones. - knowledge.upload governs every path carrying caller-supplied bytes: the single-request document upload and the four upload-session operations. The connector sync path is untouched — a connector's documents are the sanctioned source, which is the point of the key. - Both rules also read `hideKnowledgeBaseTab`. An operation declares exactly one capability, so moving these off `knowledge.use` would otherwise have let a group that withheld the whole module still reach them through the API. - knowledge.connectors is parameterized on the connector id, which the authorization funnel never sees, so it is asserted inside `createKnowledgeConnector` ahead of the write, through `CAPABILITY_RULES`. Update is not gated: it cannot change a connector's type, and re-asserting would strand an existing connector the moment an admin narrowed the allowlist. - The admin editor grows a nested connector picker under Knowledge Base, keyed off the client-safe connector meta registry.
…and credential groups
…es.bulk_download Three capabilities shipped with an admin checkbox and no server gate: an organization that set disableTableCreation, disableTableExport or disableBulkFileDownload believed it had withheld something while every path still answered. tables.create The table operation factories that mint more than one kind of operation now take the capability as an argument with no default — a default would let a new operation inherit tables.use without anyone deciding it should, which is the unreviewed omission this gate exists to prevent. tables.create and the copilot create-from-workspace-file import declare it. An import targeting `new` also creates a table, but one targeting `existing` only fills one and the operation cannot tell them apart: the target is request input the funnel never sees. Asserted inside the use case instead. tables.export Declared on createExport and downloadExport. Generating the file is the extraction and handing over its bytes completes it; readExport carries no rows and cancelExport stops an extraction rather than performing one, so gating either would strand a member with an export they can neither watch nor stop after the group changed. Both raw export routes — the synchronous CSV/JSON stream and the async job — bypass the use case entirely and query directly, so they gate inline after their existing access check and before any data moves. files.bulk_download files.download serves both a single file and a zipped folder tree, so declaring the capability on the operation would also take away saving one file, which is not what the key means. The use case asserts it only when the request is actually bulk, reusing the same single-file predicate the resource authorization already resolved so the two cannot drift. Every gate decides through CAPABILITY_RULES rather than a config key spelled out at the call site, so a renamed key cannot silently stop denying anything.
…ersist-time block bypass Every `defineWorkspaceOperation` in the workflow registry now declares what a permission group withholds, so an unfilled field can no longer be mistaken for an unreviewed one. Most workflow CRUD is honestly `'none'` — the workflow module has no hide key, and its reads and writes are governed by workspace role — and each of those carries a reason naming why. `workflows.versions.activate` gains `deploy.api`: activating a different deployed version changes what the deployed API serves, so a group that withholds API deployment must withhold it too. `workflows.public_api.update` stays exempt on purpose: `public_api.use` is asserted inside the use case and only for the enabling direction, because a group that withholds public execution must still let an admin withdraw execution a workflow already has. Closes a real bypass on the two paths that persist a whole graph. Import and the graph replace never went through the editing operations, so a member could save or import a workflow containing a block their group's `allowedIntegrations` denies; the allowlist was then a property of one authoring route rather than of what is stored, and the block was refused only by the executor mid-run — after the workflow had been saved, shared, and possibly deployed. Both paths now resolve the caller's permission config and refuse with 403 before anything is written, so there is nothing to roll back, and `importErrorCode` maps 403 to `forbidden` instead of letting a refusal surface as a 500.
Closes the persist-time bypass: a whole-graph write hands over finished blocks naming whatever types it likes, so a member could import or save a workflow containing an integration their group denies. It was caught only by the executor mid-run, after the workflow had been saved, shared and possibly deployed. Narrowed the guard from the editor's `isBlockTypeAllowed` to the allowlist alone. That helper also refuses blocks hidden from the current viewer, which is right when *adding* a preview block but wrong when *storing* a graph that already contains one: it would reject an export taken before the block was gated, and fail a save for a reason no permission group set. The guard also resolves a superseded block to its successor, so it agrees with the runtime gate rather than letting a legacy type through. Left `updatePublicApi` on `'none'` rather than declaring `public_api.use`, against the original brief. The use case already asserts the capability inside `execute`, guarded on the enabling direction only; declaring it on the operation would close nothing new while newly trapping a workspace that has public execution it could no longer withdraw.
…lities logs.cost, credentials.personal, triggers.webhook and copilot.tool_auto_approval each shipped with an admin checkbox and no server check, so an organization that set one believed it had withheld a capability while every surface still answered. logs.cost is a projection rather than a refusal, following the logs.trace_spans precedent: the log stays readable and its spend does not. Applied to the detail (run total, itemized ledger, per-block and per-span cost and tokens), to the list summaries, and to the export CSV — a hidden detail cost still printed in the list or downloaded in bulk withholds nothing. The withheld detail still satisfies the wire contract: `cost` is nullable and `costLedger` optional. credentials.personal is operation-shaped for the three OAuth connection operations, which can only ever produce a personal account-linked grant, and request-shaped for `credentials.create`, whose `type` decides scope — so that one asserts inside the use case through the capability rule rather than reimplementing the predicate. triggers.webhook gates creation only. An already-created webhook must keep firing: inbound delivery has no session to resolve a group against, and refusing there would silently break live integrations. copilot.tool_auto_approval is honoured at read time as well as at write time, so a stored auto-allow saved before the key was set stops silencing the prompt immediately rather than only for new entries.
… auto-approval Every capability the registry declares is now enforced somewhere. `hideCostInfo` follows the trace-span precedent as a projection, and goes further than the brief in one way that matters: it strips per-block and per-span cost too, since a viewer could otherwise sum the spans back to the total that was withheld. It reaches the list and the CSV export as well, because a cost hidden on the detail view and printed in the list withholds nothing. `disableToolAutoApproval` refuses at read time, not only at write. An entry saved before the policy changed would otherwise keep silencing the prompt forever, so turning the key on would take effect for nobody who had already clicked "always allow" — which is exactly the population it is for.
…ation All 287 operations now declare one, so the field becomes required and omitting it is a compile error rather than an unreviewed gap. That is the whole point: an absent field could not be told apart from an operation nobody had looked at, which is how twelve config keys shipped with an admin checkbox and no server gate. The last seven are logs reads and the public-API workspace reads, all `'none'` with reasons. The logs ones say the thing worth remembering: a group withholds *fields* inside a run — trace spans, cost — not the fact that it ran, so refusing the read would be a different restriction from the one the admin set. Also made the connector allowlist actorless-safe. It required a human subject, so a scheduled sync would have hit a 500 instead of a refusal; it now passes through with no user, exactly as the funnel treats an actorless caller. `check:actorless-executor-operations` caught that — one audit catching the other's blind spot is the argument for having both.
…ithheld Four had accumulated: the authorization funnel, a separate assertions module, five bespoke per-domain helpers, and raw config-key reads at call sites. Two of them resolved the config through different helpers, so the assertions module silently bypassed the per-request memo the funnel uses, and the two refusal messages were built independently and could drift. `capability-assertions.ts` is now the single API — workspace-scoped and organization-scoped, throwing and non-throwing — and the funnel delegates to it. Everything reads `CAPABILITY_RULES` rather than a config key spelled out locally, so a renamed key cannot quietly stop denying anything. `resolvePermissionGroupConfig` takes an optional organization id: a caller that already loaded the workspace passes it, a raw route omits it, and both share one memo keyed on user and workspace. Previously the second case had no memo at all. `PermissionGroupCapabilityError` moved into the permission-groups module. The assertions need to throw it and the funnel needs to call them, so leaving it beside the funnel made the two import each other. The personal-API-key check now reads its rule instead of the config key directly, so it cannot disagree with the capability of the same name.
…roup-item Two skills for the enterprise permission-group system: the end-to-end procedure for wiring a new governed item (field registry -> capability rule -> operation declaration or use-case assertion -> golden corpus), and the procedure for auditing an existing one by proving the refusal rather than assuming it.
…e module's wording
`capability` became a required field, so the audit's count-down mode could no
longer be reached by an operation that had simply not been annotated yet — and
while it lingered, an un-annotated `capability: 'none'` silently suppressed the
check that every declared capability is enforced. Both causes now fail:
- a capability the parsers cannot read, which the type system guarantees is a
declaration form this text-based audit does not follow, not an omission
- `'none'` without a `permission-group-exempt:` reason
The unreached-capability assertion runs unconditionally as a result.
Also collapses `capabilityRefusalMessage` into its only caller, drops the
callerless `isStaticCapability`, un-exports the compile-time assertion aliases
in `fields.ts` and `capabilities.ts` (the constraint is checked at the
declaration; the export implied a consumer that never existed), and moves the
`logs.trace_spans` rule into `CAPABILITY_IDS` order.
Wording, all of it user- or admin-facing:
- sixteen capability `describe` strings now agree with the verb in the shared
refusal sentence ("Knowledge bases is not available" -> "The Knowledge Base
module is not available"; "Skills" -> "Loading skills")
- five field hints stated what stayed permitted rather than what the key
withholds, which reads backwards where the same string is reported as an
active restriction
…ave loop An unrestricted group is the common case, so every workflow save in every ungoverned workspace was paying two registry lookups per block to reach an answer that could not change. The allowlist becomes a Set via the existing `toAllowedIntegrationTypes`, and the null case returns before the loop. Also corrects `isBlockTypeAllowed`'s doc, which restated its signature and had gone stale: it asks two questions, and the second — deployment visibility — is exactly why the persist-time guard cannot reuse it.
stripSpanCosts also runs inside backfill-trace-spans.ts, which stores what it returns, so extending it to clear tokens turned a read-time projection into permanent data loss for every authorized reader of a migrated run. Split the two: stripSpanCosts keeps its persistence contract (dollars live in the ledger, spans keep structure, timing and tokens), and the joined cross-workspace child projection gets stripJoinedChildTraceSpend, which is never written back.
…cache Three ways a run's governed subject was lost after being resolved correctly once: - serializePauseSnapshot enumerates the metadata it rebuilds and did not list capabilityGovernedUserId, so a paused run resumed gating on the billing actor (governedSubjectUserId reads absence as 'not declared'). - continueCascadeAfterResume carried the paused cell's subject into the next group even when that group held another dispatch's unclaimed pre-stamp, which both drain points in workflow-column-execution already read off the stamp. - getPermissionConfig memoizes on the ExecutionContext with no subject key, while validateModelProvider/validateBlockType passed the actor positionally - so the agent handler's model check cached the billing actor's group and every later assertPermissionsAllowed silently reused it. Derive the subject inside getPermissionConfig so the memo is correct by construction.
…e what it stopped A dispatcher that passed its status read could stamp a new cell marker after the bulk cancel and before the user delete; ON DELETE SET NULL then made it indistinguishable from an actorless request and a sibling worker drained it with no per-tool gate. Take FOR UPDATE on the departing user's row first: the foreign key a stamp checks needs FOR KEY SHARE on that same row, so every concurrent stamp either commits where the marker cancel still sees it, or blocks and is refused. Both cancels also bypassed the ordinary cancel path and so published nothing, leaving collaborators watching a dispatch that will never advance. Return what each stopped and publish the same terminal dispatch and cell events after the commit.
A payload enqueued before capabilityGovernedUserId existed does not simply reproduce the pre-existing behavior: before the field, the cascaded cells gated on actorUserId. Record what null actually costs (one deploy's worth of in-flight jobs, loosened for a session-made change) and why the alternatives are worse - actorUserId is the billed account for a workspace-key change and indistinguishable from a human on the payload, and failing closed abandons the writes the schema change promised.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 456 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
…ot its kind
A workspace-kind invitation whose granted workspace belongs to an
organization joins the invitee to that organization exactly as an
organization-kind one does — acceptance derives the member row from the
workspace's LIVE organization — so keying the organization capability
check on `kind === 'organization'` left every organization-backed
workspace invitation performing an ungated organization admission.
`resolveInvitationAdmissionOrganizationId` answers the question the gate
actually has, from acceptance's own derivation: the live organization of
the granted workspace for a workspace invitation, the stamped one
otherwise, and nobody at all for an external intent or an escalation the
stamped organization refuses — the three cases where acceptance creates
no member row. Acceptance and the accept-screen preview now read the join
target through the same helper, so the gate cannot drift from what the
accept does.
The refusal converges on `capabilityRefusalResponse('invitations.send')`
so a client sees the same sentence and `details.code` as every other
withheld capability instead of bare prose.
The last raw `/api/table/**` capability gate still reading `authResult.userId` bare. An internal executor JWT presents the run's actor, not somebody asking for a file, so this refused a delegation the executor exemption passes ungated — and refused the download of an export the same run was allowed to start (`export-async`) and to list (`jobs`), both of which already derive `capabilityGovernedAuthUserId`. The role check is untouched: it still runs on the id the credential presents.
`copyTraceSpansWithoutCosts` dropped the span's own `cost` and nothing else, so every completed run persisted the same dollars itemized underneath it in `providerTiming.segments` — the duplicate the ledger owns, and exactly what the legacy backfill had already learned to clear. Both writers now run one removal rule: the copy isolates the nodes the strip writes to (span, children, providerTiming, its segments) and hands them to `stripSpanCosts`, so persistence and backfill cannot answer differently about what a stored span may carry. Tokens survive on both paths — they are trace detail, not dollars.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
No issues found across 458 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
Five files staging reformatted under the upgraded biome came back to their pre-upgrade bytes in the merge; all five hunks are formatting only (ASI-guard blank lines, CSS font-family wrapping). Without this, format:check fails on the merge commit.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
No issues found across 458 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hide*) only hid UI — the API answered normally. All are now enforced server-side, and ~15 new keys were added (log/table/file export, knowledge connectors, workspace/CLI/webhook creation, cost/trace-span withholding, personal API keys moved into groups)lib/permission-groups/fields.ts) derives the write schema, read schema, config type, defaults, tolerant parser, and admin feature list — adding a key is one entry, and each key declares its scope so org-only toggles render inert on workspace-scoped groupscapability(or explicit annotated'none'), enforced centrally inauthorizeWorkspaceOperationafter the role check; raw routes, v1, and the MCP middleware gate through shared helpers so refusal wording and detail codes never driftdeploy.mcp/mcp_tools.usethread through the MCP middleware for all 20 handlers, andhideCostInfo/hideTraceSpansproject fields (including run files and cost-selective query refusal) on every log/run surfacenulla producer must state — threaded through the whole table dispatch pipeline including pause/resume and account deletioncheck:audits: enforcement (every operation/registry accounted for, unparseable declarations are findings), application-graph (the funnel and route wrapper cannot pull heavy module trees, dynamic imports included), capability-subject (v1 sinks only take the governed subject; a??fallback is a finding), and block-successors (the generated retired-id map cannot drift)/add-permission-group-itemand/validate-permission-group-itemskills document the procedure and audit checklistType of Change
Testing
Two consecutive full-suite runs green (39,187 tests, 0 failures), 44 audits pass, type-check clean,
check:migrationsbackward-compatible (both migrations restructured for zero-downtime: real commit boundaries around constraint validation, replay-safe statements, concurrent index build). Every capability gate has tests verified to fail with the gate removed; workspace-key pass-through, executor exemption, Copilot governance, and check ordering are pinned by dedicated tests.Note:
bun auditin CI currently fails on a repo-wide dependency advisory wave that predates and is unrelated to this PR.Checklist