feat: administered workspace RBAC and app connections - #5081
Conversation
steve8708
left a comment
steve8708
left a comment
There was a problem hiding this comment.
Watchdog review of the RBAC slice (9fd0846). Verified OK: agent roles/permissions are context only (no authorize path reads them); explain-access is gated self vs admin; Applications writes are owner/admin gated and org-scoped in the WHERE; new SQL parameterized with LOWER(email); migration additive; all 8 new catalog keys in all 12 files; Forms reference wired end to end; 328/328 in org+authorization+client/org specs; 76/76 guards. Two SECURITY items are inline (checkAction resource-scope fall-through; Applications Disabled unenforced on the hosted path). Also, not blocking:
- Audit events on set-app-member-roles / set-app-permission-roles record only the requested (after) state; read the prior assignment/override before mutating and put before/after in the audit summary.
- No distinct "no app access" state: AppRoleControl shows the default role's label for an unassigned member; render an explicit not-assigned/no-access indicator when a default role is configured.
- Bulk app-role assignment (multi-select rows, apply roles) was in the polish list and is absent.
- resolveAppAuthorizationContext runs twice per turn (production-agent.ts and agent-chat-plugin.ts); share one memoized resolution.
- useAppPermissions issues its own list-app-permissions query instead of reusing the Team page payload; fine if cached, otherwise dedupe.
Reply on each thread with fixed / reason.
| } | ||
|
|
||
| return { | ||
| allowed: true, |
There was a problem hiding this comment.
SECURITY (watchdog): access: { scope: "resource" } with no resource block falls through every branch (line 94 app/permission, 124 org, 174 if (config.resource)) and reaches this allowed: true for any caller, including a non-member. ActionAccessScope allows "resource" but nothing requires resource for it. Fix: make resource required when scope is "resource" (type-level) and deny at runtime with a clear reason when scope is "resource" and config.resource is absent; add the spec case (no such case exists in check-action.spec.ts).
There was a problem hiding this comment.
Fixed: ActionAccessConfig now requires a resource block when scope is resource at the type level, and checkAction denies malformed runtime configs. Added the regression spec.
|
|
||
| if (ownerEmail === email && (!resourceOrgId || sameOrg)) return true; | ||
| if ((!sameOrg && !canClaimCallerOrg) || !orgId) return false; | ||
| if (sameOrg && !orgEnabled) return false; |
There was a problem hiding this comment.
SECURITY (watchdog): this org_enabled check lives only in the local fallback path. When a workspace directory is configured, hostedWorkspaceAppAccess (line 88, called at 343) answers from the registry and never reads org_enabled, so an org admin who sets an app to Disabled in the new Applications surface changes nothing for hosted deployments. Fix: consult the local workspace_apps.org_enabled row for the caller's org before returning the hosted decision (a local Disabled must win), or surface the flag through the registry ACL; add a hosted-path spec.
There was a problem hiding this comment.
Fixed: hosted authorization now consults the caller org local workspace_apps.org_enabled row before accepting the hosted registry decision; an explicit local disable wins. Added hosted-path coverage.
| @@ -0,0 +1,42 @@ | |||
| import { | |||
There was a problem hiding this comment.
GAP (watchdog): the guard's heuristic keys on literal resourceType: / resourceId: object keys, which no real action in the repo uses (calls are positional assertAccess(type, id, role) and this PR's own convention is resource: { type, idFrom, level }), so it only fires on the synthetic fixtures in its spec and is inert against real code. Either detect assertAccess( / resolveAccess( / accessFilter( calls and the access: block directly, or drop the guard rather than ship a check that reports PASS having matched nothing.
There was a problem hiding this comment.
Fixed: the resource-action guard now detects declarative resource/access blocks plus assertAccess, resolveAccess, and accessFilter calls, with focused heuristic specs.
steve8708
left a comment
steve8708
left a comment
There was a problem hiding this comment.
Watchdog review of the offboarding (9ff03f1) and CI hygiene (66b452f) slices.
CI hygiene: verified OK. The relay check now warns and exits 0 only for deploy-preview and stays fatal for production/branch-deploy; the baseline shrink warning (>3 MB) does not change the size gate's pass/fail; both new unit tests pass (67/67 workflow, 5/5 baseline); no secrets echoed.
Offboarding: real transaction wiring (DbExec.transaction, no getDbExec inside the callback, PGlite bridge safe), owner/admin gate, self-removal handling, case-insensitive matching and safe identifier quoting are all correct. Three inline threads block: the owner sweep ignores the identity registry, the successor is not verified as an org member, and the audit row drops the counts. Two more, not inline:
- BUG: SCIM deprovisioning still goes through removeMembershipIfOwned (scim-provisioning.ts ~258, called ~364) and never calls offboardMember, so directory-driven offboarding gets no ownership transfer, no connection-grant cleanup and no audit event. Route it through offboardMember with a configured default successor (org owner) or leave the member's memberships in a pending-offboard state that an admin resolves; do not silently skip.
- GAP: no agent-callable org action for offboarding was added (packages/core/src/org/actions/); the spec asked for an action so the agent has the same capability as the Team page. Add it with the same owner/admin gate and successor validation.
- NOTE: the cross-system handoff (other app databases, external IdP) is documented as skipped rather than queued; state that limit in the docs and the PR description explicitly.
- NIT: offboard.spec.ts's transaction mock is a passthrough, so rollback is never exercised; one PGlite-bridge test would prove atomicity.
Reply on each thread with fixed / reason.
| throw new Error("Transfer target does not exist"); | ||
|
|
||
| const tables = await tx.execute({ | ||
| sql: `SELECT table_name, column_name FROM information_schema.columns |
There was a problem hiding this comment.
DATA-LOSS (watchdog): this is a second hand-written sweep that only catches columns literally named owner_email. IDENTITY_REKEY_COLUMNS in identity/rekey.ts registers ~70 identity-bearing columns across modes (resources.owner, the four *_shares.principal_id user shares, the analytics email-user-id tables, app_secrets/custom scope ids, author_email/actor_email/created_by, workspace_user_groups JSON). After offboarding, every one of those keeps a live reference to the departed member and the transferred owner never reaches them. Import and reuse the registry plus rekey's information_schema sweep (extract a shared helper), mapping each mode to transfer-to-successor or delete, and add a spec that seeds one row per registry entry and asserts none reference the old email afterward. Same rule as the rekey review: absent and unhandled must be different outcomes.
There was a problem hiding this comment.
One fix, not four: the bot's three offboarding findings (grants transferred before deletion, transfer scope inferred from an org_id column's presence, no retry after a failed pending removal) and this thread share a root cause. Drive offboarding from IDENTITY_REKEY_COLUMNS with an explicit per-entry disposition (transfer to successor / delete / leave provenance) and an explicit org scope per entry, in this order: revoke grants and delete role/group/token rows first, then transfer ownership rows scoped to the org, then org_members, sessions, audit. Tables not in the registry are refused, not guessed. For the pending-removal marker: set it only after local cleanup succeeds, or make the whole handler idempotent so an authorized retry completes cleanup. Please keep the successor check as an active org membership.
There was a problem hiding this comment.
Fixed: offboarding now reuses IDENTITY_REKEY_COLUMNS and a single information_schema snapshot, with explicit cleanup/transfer handling per entry and org-safe scope checks. The PGlite spec covers registry-backed ownership, secrets, shares, sessions, grants, and audit cleanup.
There was a problem hiding this comment.
Converged: offboarding is registry-driven with explicit cleanup and transfer ordering plus org scope, grants are revoked first, successor membership is verified, and pending cleanup is retryable. Focused PGlite coverage exercises the shared registry path.
| const body: { transferTo?: string } = await readBody<{ | ||
| transferTo?: string; | ||
| }>(event).catch(() => ({}) as { transferTo?: string }); | ||
| if (!body.transferTo) { |
There was a problem hiding this comment.
SECURITY (watchdog): transferTo is only checked for non-empty here and for account existence in offboard.ts:41; nothing verifies the successor is a member of THIS org. An owner/admin (or a typo) can transfer every owned row in the org to an account with no access to the org. Require isOrgMember(orgId, transferTo) (case-insensitive) before calling offboardMember, and restrict the TeamPage transferTo field to org members (a member picker, not free text). Add the spec case.
There was a problem hiding this comment.
Fixed: remove-member and offboard-member both require an active, non-pending successor in the same org. TeamPage now uses a member Select instead of free text.
| args: orgId ? [oldEmail, oldEmail, orgId] : [oldEmail, oldEmail], | ||
| }); | ||
| const memberships = await tx.execute({ | ||
| sql: `DELETE FROM org_members WHERE LOWER(email) = ?${ |
There was a problem hiding this comment.
GAP (watchdog): the audit row written below records only {oldEmail, transferTo}; removedMemberships, removedAppRoles, transferredRows and revokedSessions are computed in this function and returned to the caller but never stored. Put the counts in the audit summary/input so the compliance record says what actually moved. Also the org_members delete here runs before session revocation and the audit insert; keep it last per the contract (harmless inside the transaction, but the order is the documented promise).
There was a problem hiding this comment.
Fixed: the immutable offboarding audit event now includes membership, role, transfer, session, and cleanup counts; grants, roles, sessions, and cleanup run before the membership delete. Transaction coverage runs through the PGlite bridge.
steve8708
left a comment
steve8708
left a comment
There was a problem hiding this comment.
Watchdog review of the labs Connect slice (d0412b8).
Verified OK: labs.connectApps is server-evaluated, defaults off, single definition, nav gated; PKCE/state/return-path validation is reused unmodified from server/identity-sso.ts; the manual-URL path verifies card origin against the fetched origin and always hands off to the typed URL (no open redirect via card fields); no wildcard cookie or shared secret; feed names/descriptions render as text and icons come from a fixed internal set; translations present in all 11 Dispatch catalogs; 22/22 new specs pass.
Inline: marketplace Connect bypasses card verification; feed hardcodes capabilities; client fetch hardening; core changeset. Not inline:
- Flag off leaves the /connect route mounted and rendering empty; the spec said no route mounts. A loader-level 404 (or not registering the route) when the flag is off closes it.
- The design doc landed in the RBAC slice (cross-app-connect-design.mdx); make sure its "real vs deferred" section matches this commit exactly: feed + card validation + existing PKCE handoff are real, hub-side registration and revocation are deferred, marketplace rows are pointers until verified.
Reply on each thread with fixed / reason.
| function MarketplaceAppRow({ app }: { app: MarketplaceApp }) { | ||
| const t = useT(); | ||
| const appUrl = normalizeConnectUrl(app.url); | ||
| const canConnect = app.capabilities.includes("connect") && appUrl; |
There was a problem hiding this comment.
GAP (watchdog): the marketplace Connect button trusts the feed's self-declared capabilities and never calls fetchConnectAgentCard / parseConnectAgentCard, so a listed app gets less scrutiny than one typed by hand: no card fetch, no origin check, no "what will be granted" screen. Route marketplace rows through the same card flow before offering Connect (fetch the card from the listed URL, verify origin, show grants), and treat a feed entry as a pointer, never as proof.
There was a problem hiding this comment.
Fixed: catalog entries are treated as pointers. fetchMarketplaceApps fetches and validates each target agent card and only returns a Connect-capable entry after exact-origin validation.
| name: app.name, | ||
| description: `Agent-Native ${app.name} app`, | ||
| url: app.demoUrl, | ||
| capabilities: ["identity-sso", "connect"], |
There was a problem hiding this comment.
BUG (watchdog): capabilities is hardcoded (["identity-sso","connect"] for every template, ["connect"] for every community app) rather than derived from each app's real agent card, so the feed asserts a capability nothing verified. Either derive it at generation time by fetching each app's card (cache the result) or omit capabilities from the feed and let the client verify per app (which the thread on connect.tsx asks for anyway).
There was a problem hiding this comment.
Fixed: apps.json now omits capability claims (empty arrays); Dispatch derives the Connect capability only from the verified target card.
| source?: "first-party" | "community"; | ||
| } | ||
|
|
||
| export function normalizeConnectUrl(value: string): URL | null { |
There was a problem hiding this comment.
GAP (watchdog, low severity because these fetches run in the user's browser): normalizeConnectUrl accepts any https host including IP literals and link-local/metadata ranges, and neither fetch has a timeout or a response-size cap. Add a private/loopback/link-local/metadata denylist for non-localhost hosts, an AbortController timeout (~10 s) and a size cap on the card and feed fetches; keep the localhost allowance for development only.
There was a problem hiding this comment.
Fixed: Connect URL, card, and feed fetches now enforce HTTPS (localhost-only HTTP), credential-free/private-network checks, 5-second timeouts, and response-size caps, with focused tests.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
NIT (watchdog): this changeset covers only @agent-native/dispatch, but the commit adds public surface to @agent-native/core (AgentCapabilities.connect / A2AConfig.connect in a2a/types.ts, wired in agent-card.ts). Add the core entry (minor).
There was a problem hiding this comment.
Fixed: the changeset now includes minor entries for both @agent-native/core and @agent-native/dispatch.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rbac-batch-2 # Conflicts: # packages/core/src/a2a/types.ts # scripts/guard-netlify-prebuilt-workflow.test.ts # templates/forms/actions/update-form.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: Repair changed too much of targeted file plan.mdx; expected a localized parser fix. Agent output: Repaired recap-source.json: escaped the malformed Mermaid expression and related multiline code strings. JSON and source preservation checks passed; nothing was published. --- ⠀ 🟢 Recap source repaired and validated locally stderr: Reading additional input from stdin... 2026-09-16T03:59:32.317876Z ERROR codex_core::tools::router: error=apply_patch verification failed: invalid patch: The first line of the patch must be '*** Begin Patch' 2026-09-16T03:59:38.237244Z ERROR codex_core::tools::router: error=apply_patch verification failed: Failed to find context '-2,6 +2,6 @@' in /home/runner/work/agent-native/agent-native/recap-source.json |
…rbac-batch-2 # Conflicts: # packages/core/src/client/org/TeamPage.spec.tsx # packages/core/src/client/org/TeamPage.tsx
steve8708
left a comment
steve8708
left a comment
There was a problem hiding this comment.
Watchdog: one CI item. The required Build check is red on this head because of a Rolldown panic in the desktop-app build; details and the bisect recipe are inline on packages/core/src/index.ts. Everything else on required checks is green; the Android debug build failure is an ffmpeg-static download flake, not this PR.
| // Import everything from "@agent-native/core". | ||
|
|
||
| export * from "./config.js"; | ||
| export * from "./authorization/index.js"; |
There was a problem hiding this comment.
BLOCKING CI (watchdog): the required Build check fails on head c744239 at "Build all packages": packages/desktop-app build panics inside Rolldown (crates/rolldown/src/module_finalizers/mod.rs:469, "Panic in async function"), run 34990481296 job 104465529713. desktop-app is untouched by this PR and only imports @agent-native/core client subpaths, so the trigger is a module-graph change in core. I checked this barrel for duplicate names against the rest of the core index and found none, so it is not an ambiguous export * on its own. Reproduce locally: pnpm --filter @agent-native/desktop-app build on this branch, then on origin/main; if only the branch panics, bisect the new re-exports (this line, the client/org barrel changes, the feature-flags registry split) by replacing export * with explicit named exports; if main panics too it is a Rolldown/Vite toolchain regression from the last dependency bump and needs a pin, not code. The plan/tasks BETTER_AUTH_SECRET lines in the same log are the usual production-env warnings, not the failure.
There was a problem hiding this comment.
Fixed and verified. I reproduced the Rolldown panic on the old 329d1f9 ref; bisect isolated it to the dynamic import in action.ts, not the barrel exports. Replaced it with a static import. The desktop-app build now passes all main, preload, renderer, code-agent-runner (29,997 KB), and packaged smoke stages.
steve8708
left a comment
steve8708
left a comment
There was a problem hiding this comment.
Re-verified 329d1f916d (349 tests green, 76 guards green). Three of the nine review items are still open and one required check is still red, so this is not shippable yet. Do not admin-merge around the Build check.
Still open
- Hosted registry ignores
org_enabled(thread onworkspace-app-access.ts). offboardMembersilently skips unregistered identity columns (thread onoffboard.ts).- SCIM deprovisioning bypasses offboarding with no marker, audit, or docs (thread on
scim-provisioning.ts). pnpm --filter @agent-native/desktop-app buildstill panics on this exact ref, so the required Build check will fail again (thread oncore/src/index.ts). A bisect is running on my side; I will post the result in that thread.
Screenshots: the PR body says captures are "attached inline in the task thread", but nothing is visible on GitHub and the thread text only names them. Before merge, post the actual images (a PR comment via the GitHub web UI is fine) for: Team page multi-role + permission overrides, bulk role assignment, explain-access, Applications (workspace app access modes), offboarding dialog with successor, account email change, Connect page with labs.connectApps on (manual URL + marketplace feed) and the 404 with it off. The Applications page was missing from the list you captured.
Verified fixed: check-action resource contract, guard heuristic, not-assigned indicator, bulk assignment, Connect SSRF/timeout/size caps, feed capabilities, 404 loader, changeset.
| normalizedAppId, | ||
| context.orgId?.trim() || null, | ||
| ); | ||
| if (locallyEnabled === false) return false; |
There was a problem hiding this comment.
Still open. This local check only fires when the calling app mirrors a workspace_apps row; hostedWorkspaceAppAccess (lines 88-175) never reads org_enabled, and the Dispatch registry it calls never returns or filters on it: packages/dispatch/src/server/lib/app-creation-store.ts listWorkspaceApps -> ensureWorkspaceAppRecords selects only owner_email, org_id, visibility, name, description, path, WorkspaceAppSummary has no such field, and filterWorkspaceAppsByAccess gates on ownership/sharing only (grep -rn org_enabled packages/dispatch/src is empty). set-workspace-app-access writes the flag into the DB of whichever app the admin is in, which in hosted mode is the Dispatch DB, so the one app that owns the row is the one that never enforces it for others.
Fix: make the registry the enforcement point. In listWorkspaceApps/filterWorkspaceAppsByAccess, exclude rows with org_enabled = false for the caller's org (owner can still see it, flagged), and return orgEnabled in WorkspaceAppSummary so hostedWorkspaceAppAccess can deny explicitly. Add a spec that exercises the hosted path with a disabled app (the only test today, workspace-app-access.spec.ts "honors a local organization disable before hosted registry access", covers the local path).
There was a problem hiding this comment.
Verified fixed on e5849995fb: registry selects and enforces org_enabled (WorkspaceAppSummary.orgEnabled, filterWorkspaceAppsByAccess), hostedWorkspaceAppAccess denies disabled apps, owner fast path checks it first; specs cover hosted, owner, and local-wins cases. Resolving.
There was a problem hiding this comment.
Fixed in bf8de81. WorkspaceAppSummary now carries orgEnabled from the registry; list/filter preserves a disabled row only for owners, hosted authorization denies disabled apps, and local disablement wins before the owner fast path. Dispatch registry and core hosted/owner specs cover it.
|
|
||
| // The registry is shared with identity rekey so new owner columns cannot | ||
| // silently bypass offboarding. The information_schema sweep retains the | ||
| // extension escape hatch used by rekey for app-owned owner_email tables. |
There was a problem hiding this comment.
Still open. assertIdentityColumnsRegistered (rekey.ts:191) is only called from rekeyIdentity (rekey.ts:589); offboardMember never calls it. owner_email tables are auto-adopted here, but any other identity-shaped column an app adds without registering it in IDENTITY_REKEY_COLUMNS (*_email, user_id, created_by, ...) is silently skipped: the departed member's rows stay attached, no throw, no log. That is the silent-coercion class the repo bans.
Fix: export the assertion from rekey.ts and call it at the top of offboardMember (inside the transaction, before any delete) so an unregistered column fails the offboarding loudly with the column name, same as rekey does. Add one spec: an unregistered *_email column makes offboardMember throw and leaves the roster untouched.
There was a problem hiding this comment.
Verified fixed on bf8de813b3: registry assertion runs before any mutation and the unregistered-column spec throws. Nit only: it re-queries information_schema instead of calling assertIdentityColumnsRegistered; fine to leave. Resolving.
There was a problem hiding this comment.
Fixed in bf8de81. Offboarding snapshots information_schema inside its transaction and calls the shared identity assertion before any delete/update; the unregistered created_by spec proves the roster remains unchanged when the guard throws.
| // A directory deactivation revokes local and connected-app sessions only | ||
| // when no active SCIM organization remains for the identity. The Better | ||
| // Auth transaction owns the session rows, so this stays atomic with the | ||
| // membership mapping cleanup without pretending to cover other app DBs. |
There was a problem hiding this comment.
Still open. removeMembershipIfOwned (lines 258-300) does its own raw deletes of orgMember/appMemberRole/orgScimMembership: no ownership transfer, no grant/token revocation beyond this session delete, no audit row. A SCIM-driven removal therefore behaves differently from the manual and agent offboarding paths, and nothing in code comments or administered-deployments.mdx says so.
SCIM cannot choose a successor, so do not route it through offboardMember blindly. Instead: (1) when SCIM removes a membership it created, set the same pending marker the federated path uses (federation_removal_pending_at) and write an audit row, so the Team page shows the member as pending offboarding and an admin finishes it with a successor through the existing retryPendingFederatedRemoval path; (2) document the boundary in administered-deployments.mdx (SCIM deprovisions access immediately; ownership transfer is completed by an admin) and update the locale copies. Add a spec for the marker.
There was a problem hiding this comment.
Verified fixed on e5849995fb: pending marker + audit row on SCIM removal, spec, docs paragraph in source and 10 locales. Resolving.
There was a problem hiding this comment.
Fixed in bf8de81. SCIM-created removal now keeps the membership with federation_removal_pending_at, removes app roles, writes an immutable pending audit event in the Better Auth transaction, and clears the marker on reactivation. Admin completion remains the existing successor/retry path and is documented with locales.
| // Import everything from "@agent-native/core". | ||
|
|
||
| export * from "./config.js"; | ||
| export * from "./authorization/index.js"; |
There was a problem hiding this comment.
Blocker: the required Build check is still red for the same reason. On this exact ref (329d1f916d, clean tree):
pnpm --filter @agent-native/desktop-app build
Rolldown panicked... "VAULT_SYNC_DESCRIPTION_PREFIX" is not in any chunk, which is unexpected
Rolldown panicked... "BAGGAGE_META_KEY" is not in any chunk...
Rolldown panicked... "cryptoRuntime" is not in any chunk...
Rolldown panicked... "BUILDER_AUTH_FAILURE_TTL_MS" is not in any chunk...
Build failed in 195ms
ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL @agent-native/desktop-app@0.1.150 build
No commit in the PR touches Rolldown, a dependency pin, or desktop-app, so the trigger is a core module-graph change this branch introduced (this barrel export and the new authorization/, identity/, org/actions/ modules are the suspects; a self-import of the core barrel from inside core would do it). I am bisecting now and will post the minimal revert here. Until then: run pnpm -r --filter '@agent-native/desktop-app...' run build locally before every push and paste the passing tail in this thread; do not mark this fixed from CI status alone.
There was a problem hiding this comment.
Bisect done (clean detached worktree, full pnpm -r --filter '@agent-native/desktop-app...' run build):
origin/main@ 08324bc: passes.- this ref @ 329d1f9: panics (same four symbols). The failure is in the electron-vite main-process build, which statically bundles nearly all of core's server surface; that is why unrelated secrets/otel/crypto symbols are named.
- Removing the
authorizationbarrel export alone: still panics. Removing the four new action registrations inserver/action-discovery.ts: still panics. - Root cause: the dynamic
import()added topackages/core/src/action.tsinwrapRunWithAccess(const { assertActionAccess } = await import("./authorization/check-action.js")).action.tssits at the root of almost every static import chain in core, andcheck-action.tsis also reachable eagerly through other paths, so Rolldown's chunk finalizer loses the symbols (module_finalizers/mod.rs:469). Upstream bug, newly triggered by this edge.
Minimal fix, verified (build passes clean with only this diff): replace the inline await import(...) and the import type { ActionAccessConfig } with one static import:
import { assertActionAccess, type ActionAccessConfig } from "./authorization/check-action.js";
// ...
if (access) await assertActionAccess(access, args, ctx);No behavior change; check-action.ts only import types action.ts, so no runtime cycle. After the change run the core action specs and the desktop build locally and paste the passing tail here.
There was a problem hiding this comment.
Verified fixed on e5849995fb: static import in action.ts, pnpm -r --filter '@agent-native/desktop-app...' run build exits 0 in a clean worktree. Resolving.
There was a problem hiding this comment.
Fixed and verified on the current tip. The Rolldown panic was caused by the dynamic import in action.ts; the static import is now in place. The desktop-app build passes all stages, including code-agent-runner (29,997 KB) and packaged smoke.
There was a problem hiding this comment.
Separate from the Rolldown panic (fixed): CI Build on 1f84997208 failed in templates/chat build with ReferenceError: React is not defined at packages/core/src/client/ocean/OceanBackground.tsx:65, inside react-dom's legacy renderToString. I bisected it in a clean worktree. pnpm -r --filter 'chat...' run build passes on both origin/main and 1f84997208. The render chain (server/onboarding-html.ts -> AuthPage -> OceanBackground) is untouched by this PR, and none of the last 60 repo-wide Build jobs show this crash. Treat it as a flake, not a regression: do not change code for it. If Build on 3a7541f68f fails with the same signature, rerun just the failed job once (gh run rerun <run-id> --failed). Only escalate if it fails the same way twice. Lint & format stays red until #5106 merges and you merge main. Do not admin-merge past either check.
There was a problem hiding this comment.
Lint & format is red again on 92a2b59149, and again it is not your change. The refresh worked: this run checked out Merge 92a2b59149 into e2834b3c6f (post-#5135 main), and the two plans docs are gone from the failure list. One different inherited file fails now:
packages/core/docs/content/integrations.mdx (117ms)
Format issues found in above 1 files.
It is byte-identical between your branch and main, it fails oxfmt --check on origin/main itself, and it landed in main commit 9c18df90de (#5075, 18:49 PT). Third inherited formatting break on main today.
Do not format it here and do not let pnpm prep fold it in; carrying an independent main add is what made the PR conflicting last time. I'm clearing it on main again and will confirm here. Your other required checks are running on this head.
Root cause worth a follow-up after this ships: docs-only PRs skip the Lint & format lane via the change-scope gate (it showed skipping on #5135), so unformatted docs land on main freely, and then every code PR fails the tree-wide check. The fix belongs in the scope gate, not in individual PRs.
There was a problem hiding this comment.
Main is clean again. #5141 merged as baa0f0ecde (now main's head), and I verified packages/core/docs/content/integrations.mdx passes oxfmt --check at that commit. That was the only file failing the tree-wide check.
Lint & format on this PR clears on its next CI run. As before, do not rerun the failed job: a rerun replays the original merge commit (Merge 92a2b59149 into e2834b3c6f), which predates this fix. If your current run has already finished that lane, push anything (git merge origin/main or git commit --allow-empty) so GitHub recomputes the merge ref against main >= baa0f0ecde.
Nothing else outstanding from my side: the review is complete and the merge hold is lifted. Ship once the run on your head is green.
There was a problem hiding this comment.
Two findings in the new slice 7892e9509a (37 files, organization provider keys). Flagging before you merge, since I had only signed off through 470263004f.
1. Silent skip in the personal-key cleanup — packages/core/src/server/agent-engine-api-key-route.ts:380. The cleanup is const session = await getSession(event); if (session?.email) { ...deleteAppSecret... } with no else and no log. If the session read fails or carries no email, the organization key is written, the shadowing personal row survives, and the caller still gets { ok: true }. Because the resolver is user-first, that member keeps silently using the old personal credential while the UI reports success — the exact absent-vs-unreadable coercion the repo bans. Your own sibling path at :271 gets this right (console.warn("[agent-engine] could not read session for delete")). Mirror that: warn, and either report a partial result or fail loudly, so "saved" never means "still shadowed".
2. Docs promise more than the code guarantees, and the translations don't carry the new copy. administered-deployments.mdx now says personal rows "are removed when an organization key is saved", which is untrue in the skip case above. Also the English file gained 8 lines (including the new provider-model-field paragraph), while all ten locales changed exactly 1 line each — so the new paragraph exists only in English. guard:i18n-changed-copy passed because a locale file was touched, not because the content landed.
Ordering itself is right: the cleanup runs after writeAppSecret, so a failed save can't erase the old credential first. Everything I verified earlier is still intact on this head.
There was a problem hiding this comment.
Fixed in 7320659. Organization-key cleanup now catches a missing or failed session read, logs the failure, returns HTTP 503 with an explicit partial-save error, and never reports success while a legacy personal row may still shadow the org key. Added a regression spec covering the unavailable-session path. The model guidance was already translated in all ten locale files; this commit splits that guidance into its own paragraph in each locale so the curated-suggestions and free-form-ID meaning is explicit and structurally visible. Both i18n guards pass.
There was a problem hiding this comment.
Both findings are fixed in 7320659b8a, verified:
1. Silent skip → loud failure. The cleanup now wraps getSession in try/catch with console.warn("[agent-engine] could not read session for legacy-key cleanup"), and a missing email returns 503 with "Organization key saved, but the legacy personal key could not be cleared. Retry this save before using the organization key." instead of { ok: true }. The caller can no longer read "saved" while a shadowing personal row survives. The new spec ("reports a partial save when the legacy-key cleanup session is unavailable") pins exactly the right pair: writeAppSecret was called with the org target, and deleteAppSecret was not called.
2. Translations carry the meaning. All ten locales gained the real content (+3 lines each) with English unchanged, including the provider-model paragraph — e.g. de-DE "Das Modellfeld listet aktuelle Modelle als Vorschläge..." and ja-JP "モデル欄には現在のモデルが候補として表示され...". Not placeholder touches.
That closes everything I raised. Review complete, merge hold lifted: ship once CI is green on this head.
One follow-up for a later PR, not this one: guard:i18n-changed-copy passed on the previous push when each locale file had only a one-line edit and the new English paragraph existed nowhere else. The guard verifies a locale file changed, not that the changed English content landed in it, so it can be satisfied by a trivial touch.
| return [{ title: "Connect apps — Dispatch" }]; | ||
| } | ||
|
|
||
| function MarketplaceAppRow({ app }: { app: MarketplaceApp }) { |
There was a problem hiding this comment.
Minor: MarketplaceAppRow renders name, description, and a bare Connect link, while the manual URL flow's ConnectCard shows dispatch.pages.connectAppGrant (what will be granted) before the click. One-click connect from the feed must show the same grant text so the user knows what they are approving. Reuse the same string, no new copy.
There was a problem hiding this comment.
Fixed in bf8de81. Connect-capable marketplace rows now reuse dispatch.pages.connectAppGrant, matching the manual URL flow before the Connect action.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers the latest SCIM lifecycle, RBAC, offboarding, and Connect changes (130 files, 4,427 changed lines). The earlier reported action-contract, app-disable, marketplace validation, SSR flag, closed-org, ownership, and session concerns were not reposted; agents found the relevant fixes or treated them as existing threads.
New findings
- 🔴 HIGH — SCIM deprovisioning writes to the lazily-created audit table without ensuring that table exists, so a fresh serverless deployment can roll back membership removal entirely.
- 🟡 MEDIUM — SCIM session cleanup considers only active SCIM sources and can log out users who still have manually managed organization memberships.
- 🟡 MEDIUM — After completed SCIM removal, reactivation can attempt to insert a duplicate
(org_id, user_id)mapping and permanently fail reprovisioning.
The new SCIM audit and lifecycle behavior is directionally useful and the focused SCIM tests pass, but the transaction and reactivation edge cases need coverage before merge.
🧪 Browser testing: Attempted FULL verification across 19 planned cases, but Chrome MCP tools were unavailable to every executor and retry. The dev server was healthy; no visual evidence could be captured.
| }, | ||
| ], | ||
| }); | ||
| await database.create({ |
There was a problem hiding this comment.
🔴 Ensure the SCIM audit table exists before deprovisioning
This SCIM callback writes agentAuditLog inside Better Auth’s transaction, but it does not call ensureAuditTables(). The audit table is lazily created and boot database work can be disabled in the normal serverless path, so the first SCIM deprovision on a fresh deployment can fail and roll back the membership and role cleanup. Provision this table through guaranteed schema setup before the transaction or otherwise use a schema that is guaranteed to exist.
Additional Info
Reported by 1 of 4 code-review agents; confirmed by tracing the lazy audit initialization path.
There was a problem hiding this comment.
Confirmed. agent_audit_log only exists after ensureAuditTables() (audit/store.ts:24, CREATE TABLE IF NOT EXISTS), which runs lazily from the audit store functions, from offboardMember (offboard.ts:42), and as a swallowed best-effort at boot. On a deployment whose first audit write is this SCIM deprovisioning, database.create({ model: "agentAuditLog" }) fails inside Better Auth's transaction and the whole deprovision rolls back, so the directory removal never lands. Do not call ensureAuditTables() from inside the callback: it uses the framework pool, and on PGlite dev a second connection inside a Better Auth transaction deadlocks. Fix at the boundary instead: add the audit table DDL to org/migrations.ts (next id, same idempotent statement shared with ensureAuditTables) so it exists wherever org_members exists, and keep ensureAuditTables for older databases. Spec: run the SCIM deprovision against a fresh test database with no prior audit write.
There was a problem hiding this comment.
Unrelated to this thread, FYI for the ship loop: the required Lint & format check is red on 1f84997208 because oxfmt --check rejects 40 Plans docs files (template-plan*.mdx and locales/*/{plan-plugin,pr-visual-recap,template-content-local-files,template-plan}.mdx). Those files are identical to main; they came in unformatted with #3903 (14372c1), so every PR is red right now. Do not reformat them in this PR. A formatting-only fix PR against main is being opened; once it merges, merge main here and Lint goes green.
There was a problem hiding this comment.
Fixed in 3a7541f. The full idempotent agent_audit_log DDL is now shared by ensureAuditTables and org migration 1032, so the table exists wherever org_members migrations have run; the SCIM path does not open a second connection inside Better Auth. The migration spec asserts the shared statement, and the first SCIM audit-event fixture requires the declared audit table.
There was a problem hiding this comment.
Verified fixed on 3a7541f68f: migration 1032 owns the table via the shared AGENT_AUDIT_LOG_CREATE_SQL, ensureAuditTables and rekey reuse it, and the fresh-database spec covers the first SCIM write. Resolving. One ask: the new org/migrations.ts -> audit/store.ts import edge is fine by inspection, but re-run pnpm -r --filter '@agent-native/desktop-app...' run build on this head and note the result in the Build thread, since CI's Build is currently masked by an unrelated chat SSR failure.
There was a problem hiding this comment.
| // when no active SCIM organization remains for the identity. The Better | ||
| // Auth transaction owns the session rows, so this stays atomic with the | ||
| // membership mapping cleanup without pretending to cover other app DBs. | ||
| if (activeOrgIds.size === 0) { |
There was a problem hiding this comment.
🟡 Preserve sessions when manual organization memberships remain
activeOrgIds contains only active SCIM source organizations. If the user still belongs to a manually managed organization, deprovisioning their last SCIM source still deletes every Better Auth session. Query remaining active org_members memberships (excluding pending removals) before revoking sessions, so SCIM deactivation does not log the user out of an unrelated organization.
Additional Info
Reported independently by 1 of 4 code-review agents and confirmed from the session deletion branch.
There was a problem hiding this comment.
Agree, and it should mirror what offboardMember now does (bf8de81): before deleting sessions, count remaining active org_members rows for the email (excluding rows with federation_removal_pending_at set) rather than only SCIM source orgs; revoke only when that count is zero. One spec: SCIM removes the last source org while a manual membership in another org remains, sessions stay.
There was a problem hiding this comment.
Fixed in 3a7541f. SCIM session revocation now checks all remaining active org_members rows for the identity and excludes pending removals, so a manual membership in another organization preserves the account session. The focused SCIM spec covers that multi-org case.
There was a problem hiding this comment.
Verified fixed on 3a7541f68f: remaining active orgMember rows (pending excluded) gate the session delete; multi-org spec added. Resolving.
| } | ||
| } | ||
| if (member) { | ||
| if (mapping) return; |
There was a problem hiding this comment.
🟡 Reuse stale SCIM mappings during reactivation
After local offboarding removes the org_members row, the existing org_scim_memberships mapping remains. On reactivation, mappedMember is absent and member is absent, so this branch returns without reconciling the stale mapping; the later member-creation path attempts another mapping for the same (org_id, user_id), violating the unique constraint and rolling back reprovisioning. Update the existing mapping with the new memberId or reuse it before inserting.
Additional Info
Reported by 1 of 4 code-review agents; confirmed against the mapping and member recreation branches.
There was a problem hiding this comment.
Agree, lower severity than the two above. When mapping.memberId points at a row that local offboarding already deleted, the code falls through and creates a second org_scim_memberships row (createdMembership true) while the stale one keeps its dangling memberId; if a manual membership exists by then, if (mapping) return leaves the stale mapping in place. Fix: in ensureMembership, when the mapping's member row is missing, delete that mapping before the fall-through (or update it in place with the new memberId) so there is exactly one mapping per (org, user). One spec: deprovision, local offboard, reactivate; expect one mapping and one membership.
There was a problem hiding this comment.
Fixed in 3a7541f. Reactivation deletes a dangling org_scim_memberships mapping before creating a replacement, preserving the unique (org_id, user_id) invariant after local offboarding. The focused spec exercises deprovision, local member deletion, and reactivation with one mapping and one membership.
There was a problem hiding this comment.
Verified fixed on 3a7541f68f: dangling mapping is deleted before the replacement is created; spec covers deprovision -> local offboard -> reactivate. Resolving.
|
Visual proof from the running local Dispatch template on the current PR head:
|
…rbac-batch-2 # Conflicts: # packages/core/src/scripts/runner.ts
…d, packages/core (37 files)
Provider setup now saves organization-scoped keys only. The model field accepts curated suggestions and custom model IDs. |
…rbac-batch-2 # Conflicts: # packages/core/src/client/org/TeamPage.tsx
1 participant
Summary
This ready-for-review PR delivers the administered-workspace RBAC, offboarding,
access-contract, Applications, CI hygiene, identity SSO documentation, and
labs Connect Apps batch in reviewable commits.
overrides, shared
checkActioncontracts, explain-access, Applicationsaccess administration, Forms reference pattern, and audit metadata.
registry coverage, ownership transfer, session/app-role cleanup, delegated
grant revocation, immutable audit counts, active-successor validation, and
an agent-callable action. SCIM remains a separate Better Auth transaction;
its bridge removes only SCIM-owned memberships and local sessions, while
cross-database ownership transfer is an explicit pending/retry boundary.
shrink warnings.
labs.connectApps, public catalog feed, exact-originagent-card validation, URL fetch hardening, existing identity-hub handoff,
and Dispatch Connect Apps UI.
silent identity probe remain opt-in and backward compatible; this PR adds the
rollout runbook but does not flip Dispatch's browser flag.
and Connect Apps docs with all configured locales.
Security and rollout notes
context for the agent only. The production handler passes one prepared
authorization snapshot into the loop so it is not resolved twice per turn.
assertions. No shared wildcard cookie or community-app secret is introduced.
registry-backed cleanup run before the membership delete; external IdP and
other-app effects remain durable retry work rather than a false atomicity
claim.
sessions for unrelated active org memberships, and safely repairs stale
source mappings during reactivation.
The harness already waits on its migration/index setup and no safe small fix
was identified, so no unrelated harness change is included.
render pulled into the CLI graph. Removing that dead export keeps the public
getOnboardingHtml()API and avoids classic-JSX evaluation during builds;OceanBackground and the chat SSR path remain unchanged.
runner.tsregistration import) failed withAction authorization runtime is not available.; Green (the import restored) passed all 17 runner specs andreached the expected
An authenticated user is required.decision.shim, while production-agent, action-discovery/CLI, MCP, A2A, and HTTP server
entrypoints register the real checker. This preserves declarative access
enforcement without pulling database modules into browser bundles and keeps
every server execution surface fail-closed.
formatter command passes locally, so it is treated as baseline runner drift.
handoff. The repository has no hub-side per-app connection registry, dynamic
identity-client registration endpoint, or disconnect/session-revocation API
to reuse, so those product/backend pieces remain a documented follow-up.
Verification
access contracts, feature flags, SSO/SCIM, connect card/feed validation, and
the app feed route.
pnpm guardsand both i18n guards pass after the latest review fixes.pnpm fmt:check, core/Dispatch typechecks, andgit diff --checkpass.http://127.0.0.1:8095/dispatch):Organization settings shows Applications and per-member roles/permissions;
the multi-select role picker and explicit Not assigned state are visible;
the permission matrix and explain-access popover work; Remove member uses an
active-member ownership-transfer picker; Connect Apps shows feed failure,
manual URL validation, and a verified local agent-card handoff. Captures are
attached inline in the task thread and summarized here for review.
The SSO/SCIM settings controls remain opt-in and therefore are not rendered in
the default-off browser session; their focused specs cover the enabled path.
Dispatch browser identity SSO remains flag-gated and was not flipped here.
Screenshots
The task thread includes captures from the running Dispatch template for:
labs.connectAppsenabled, feed/manual URL states, and theflag-off 404 state.