Skip to content

B7b: every access resolver answers with one org-aware membership model - #2673

Merged
2witstudios merged 7 commits into
pu/org-walletsfrom
pu/ow-b7b
Sep 21, 2026
Merged

2witstudios merged 7 commits into
pu/org-walletsfrom
pu/ow-b7b

Conversation

@2witstudios

@2witstudios 2witstudios commented Sep 17, 2026

Copy link
Copy Markdown
Owner

B7b: every access resolver answers with one org-aware membership model

Leaf gdk2ok994er5or61l6crpklc (Phase 1 task yn7155euqcungw4qoht18jio). Base pu/org-wallets at de988390e (#2669, B7); merge-base equals that tip. Wave F (orgs on in staging) waits on this PR, and on B7c for the apps/web inline gates.

What changed

1. One membership model for every sibling resolver.

  • resolveEffectiveDriveMemberships (permissions/org-drive-membership.ts) is the one IO edge over B7's pure resolveEffectiveDriveMembership. It handles one or many (user, drive) pairs. While dark, or for personal drives, it runs no query. When enabled it adds one org-roles query and one default-roles query per batch. loadEffectiveDriveMembership is now a single-pair call into it.
  • These now read membership only through that edge:
    • Named siblings: getUserDriveAccess, isUserDriveMember, isDriveOwnerOrAdmin, getUserDrivePermissions, getUserAccessiblePagesInDrive, getUserAccessiblePagesInDriveWithDetails, getBatchPagePermissions, getUsersWhoCanViewPage, getDriveIdsForUser. hasAppDriveMembership and hasScopedDriveMembership follow through isUserDriveMember.
    • Lib gates added by point-guard ruling: checkDriveAccess, checkDriveAccessForRoles, resolveGranterAccess, getPageIfCanShare, getMemberCustomRoleId, isUserMemberOfAnyEventDrive, resolveDriveMembership (agent workspaces).
  • Batch resolvers. withEffectiveMembership swaps each row's joined drive_members row for the effective membership. It resolves each (user, drive) pair once. It also loads the permissions of a default custom role the query did not join.
  • getDriveIdsForUser. While enabled it returns exactly the drive set of listAccessibleDrives({ includeTrash: true }), decided by decideListedDriveRole. A RESTRICTED or PRIVATE org drive is included only once joined, and never through a page share alone. Its callers (mentions search, calendar, sessions, principal drive ids) aggregate only over drives the user has listed.
  • Token rule.
    • An inheriting key or OAuth scope resolves with the person's effective access, including org access.
    • An explicit key role means only that role.
    • Agents hold only their own drive_agent_members rows.
    • All three are shown in the matrix suite.

2. Former lead's OWNER row.

  • reassignLedOrgDrives deletes the former lead's OWNER row on every drive it reassigns, in the reassignment transaction.
  • leaveOrganization deletes the leaver's OWNER rows on all of the org's drives, reported as revoked.formerLeadOwnerRows.
  • updateDriveLastAccessed writes the self-heal row only for a personal drive.
  • Data check. The query (drive_members with role = 'OWNER' joined to drives with orgId IS NOT NULL) returns 0 rows on a freshly migrated database. The e2e Northwind seed creates no org drive, and every route that sets drives.orgId (api/drives POST, api/drives/[driveId]/org) returns 404 while ORGS_ENABLED is false. So no environment can hold such a row today. I did not run it against production.

3. ORG-4 audit dedupe.

  • permissions/org-admin-access-audit.ts writes authz.access.granted at most once per (user, PRIVATE org drive) per 15-minute UTC window.
  • How: a guarded insert on a unique key. A claim row goes into rate_limit_buckets, keyed by audit:org-admin-private-drive:<user>:<drive> plus the window start truncated to the 15-minute UTC boundary. That pair is the table's primary key, and the insert uses ON CONFLICT DO NOTHING. Only the caller whose insert lands writes the event.
    • No migration: the table already exists, and the webhook-delivery idempotency claim uses it the same way.
    • The key cannot live in the audit store: the dedicated Admin PG ingest grant is INSERT-only.
    • An in-process memo skips Postgres for repeats inside the window.
    • Claim rows expire with their window and are swept by the existing bucket sweep.
    • If the claim store is unreachable, it fails open and writes the event.
    • The write is awaited. If it fails, the claim row and the memo entry are released, so the next access in the same window writes the record (review 5237996587).
  • Every resolver path writes through this one function: page views, realtime per-event re-checks (getUserAccessLevel), AI tool checks, drive room joins, search's drive gate and per-hit checks, and batch checks. Audience computations (getUsersWhoCanViewPage) pass audit: false: nobody accessed anything.

4. Removal and demotion.

  • Removal. removeMember already runs the leave cascade (2ba9e10). That cascade is now revokeOrgDriveGrants(tx, userId, driveIds): agent grants, drive and page share links, MCP key drive rows, and OAuth grants.
    • An OAuth drive scope has no row, and its explicit role is never re-checked. So every OAuth token family (access and refresh tokens) whose scopes name one of the drives is revoked, with reason org_access_revoked.
  • Demotion. changeMemberRole runs revokeForDemotion in the role-change transaction. Pure planDemotionRevocation compares effective access before and after, per org drive:
    • Access gone (a drive org power alone opened): the full revokeOrgDriveGrants.

    • ADMIN became MEMBER (an OPEN drive): only what a Member could not create:

      • drive share links (owner or admin only);
      • page share links on pages the member cannot share (memberCouldSharePage mirrors getUserAccessLevel);
      • agent grants above the Member cap (recapAgentMembershipsGrantedBy, now transaction-aware with a known cap).

      Inheriting key and OAuth scopes stay, because they follow the person.

    • Otherwise: nothing. This covers a row still backing the access, a drive they lead, Owner to Admin, and promotion.

Fixes the consistency matrix and review surfaced. All four change behaviour while orgs are dark. Master already has the last three (#2672); only the expired-share fix is missing from master.

  • listAccessibleDrives (both the dark and org bodies) ignored pagePermissions.expiresAt. An expired page share still listed its drive in the picker, /api/drives and MCP list_drives, while getDriveIdsForUser and every access resolver refused it. Both bodies now filter expired shares. Master has the same unfiltered query (drive-service.ts permissionDrives), so it differs there too. Matrix fixture: Lu's expired share on Marcus Notes; test an expired page share lists no drive, dark or enabled.
  • checkDriveAccessForRoles read pending rows, so a pending ADMIN invitation could manage roles. It now uses the shared loader, which gates on acceptedAt. The point-guard ruled: routed functions apply the gate, with a pending-invite test on each. A separate lane, fix-accepted-invite-gate, fixes master.
  • resolvePagePermissionRow (behind getBatchPagePermissions and getUsersWhoCanViewPage: search, sidebar badges, inbox, permissions/batch, channel fan-out) let a custom role's driveWidePermissions.canView open a PRIVATE page.
    • getUserAccessLevel, the app-token resolver and the agent resolver all refuse that.
    • resolve-page-permission-row.test.ts had pinned the leaking behaviour; that test is changed here.
    • With orgs enabled, every implicit OPEN member holds the default role, so every org member would have seen Product's private pages in search.
  • resolvePagePermissionRow also returned a custom-role entry whose canView is false, so {canView:false, canEdit:true} granted edit through getBatchPagePermissions while getUserAccessLevel returned null. It now takes master's fix(permissions): pending drive invites grant nothing; a custom role's drive-wide grant never opens a private page #2672 line (return resolved.canView ? { ...resolved, canDelete: false } : null;) and its unit test grants nothing when the custom role entry denies view, whatever else it sets. Matrix fixture: Product's default role hides a Board deck page while setting canEdit on it.

Requirement IDs and tests

ID Tests Claim
ORG-4 ORG-4 (partial) DRV-5 (partial) DRV-6 (partial) X-6 (partial) the consistency matrix… · ORG-4 (partial) agent and app identities never gain org-derived access… (org-drive-sibling-resolvers.integration) · ORG-4 (partial) five resolver calls in one window write one audit row… · ORG-4 (partial) the claim is a unique key in Postgres… (org-admin-access-audit.integration) · 4× ORG-4 (partial) (org-admin-access-audit.test) · removal and demotion 2× (org-member-revocation.integration) · 4× (demotion.test) partial (see below)
DRV-5 matrix · DRV-5 (partial) an implicit Open member holds the drive default role in every custom-role reader… partial: sidebar and picker UI are Wave F
DRV-6 matrix partial: the join request and approval flow are not here
DRV-7 DRV-7 (partial) resolveGranterAccess follows the shared membership… partial
DRV-9 not claimed. getDriveIdsForUser now equals the accessible-drives listing (matrix asserts it); the picker grouping UI is Wave F not claimed
X-6 matrix (non-member Dana with a stale org row, guest Chris with a Finance page share, former lead Fred) partial: the policy, wallet and automation clauses belong to other lanes
ORG-6 ORG-6 (partial) a lead who leaves the org keeps no OWNER row on its drives · ORG-6 (partial) reassigning a lead's org drives … removes their OWNER row… · ORG-6 (partial) updateDriveLastAccessed never writes an OWNER self-heal row for an org drive… (leave.integration) partial
dark while ORGS_ENABLED is false every routed sibling returns exactly the pre-B7b result, except the three named fixes… · a pending invitation counts for nothing in any routed resolver, dark or enabled see below

ORG-4 is still (partial), so the allowlist is unchanged.

  • The audit clause is now proven: one row per access window on every path, including a second process. B7 proved the access clause for its four resolvers, and this PR extends it to every lib sibling.
  • Two clauses are still unproven:
    • "full access on every org-owned drive" is not true yet for the apps/web inline gates (B7c, below);
    • drive-owner-only actions (isOwner, e.g. rename or restore drive) stay closed to the org Owner.
  • Reviewer: please confirm that ORG-4 stays (partial) until B7c lands.

Dark-flag equivalence.

  • __tests__/fixtures/pre-b7b-sibling-resolvers.ts is a frozen copy of all 16 routed function bodies at de988390e, extracted mechanically. Only the names are prefixed legacy.
  • The test compares live against legacy for 13 people × 6 drives × every page, with ORGS_ENABLED false. That is more than 1000 comparisons, and the test asserts that count.
  • The set of differences must equal exactly five intended differences:
    • checkDriveAccessForRoles for Tomás's two pending invitations;
    • Marcus's view of the private Hiring page, in getBatchPagePermissions and getUsersWhoCanViewPage;
    • Marcus's edit-without-view on the Board deck, in getBatchPagePermissions.
  • The expired-share fix is not in that set: listAccessibleDrives is not a routed sibling. Its own test, an expired page share lists no drive, dark or enabled, covers it dark.
  • Non-vacuity: while dark, Marcus's stale Finance org row still opens Finance.

Mutation checks (line index at 2b0c4bf7d unless noted; each with a no-op control GREEN; git status clean after)

Line mutated Result
permissions.ts:433 isUserDriveMember reads drive_members directly instead of the shared loader RED: consistency matrix
permissions.ts:1097 withEffectiveMembership always returns the joined rows RED: matrix
permissions.ts:1047 private-page guard in resolvePagePermissionRow removed RED: matrix and dark equivalence
permissions.ts:156 if (listed !== null) removed in getDriveIdsForUserWithOrgs RED: matrix
permissions.ts:50 if (ORGS_ENABLED) gate removed RED: dark equivalence
drive-role-service.ts:122 back to db.query.driveMembers.findFirst without the acceptedAt gate RED: matrix, dark equivalence, pending invitation
org-admin-access-audit.ts:93 if (won) write(access)write(access) RED: dedupe integration (second process) and unit (second process)
org-admin-access-audit.ts:105 claim always wins RED: both dedupe integration tests
org-admin-access-audit.ts:44 no window truncation RED: both dedupe integration tests
org-drive-membership.ts:138 drop options.audit && RED: dedupe integration (the audience writes a row)
drive-service.ts:656 drop && drive.orgId === null RED: ORG-6 (partial) updateDriveLastAccessed…
leave.ts:118 reassign deletes the OWNER row of '__nobody__' RED: ORG-6 (partial) reassigning…
leave.ts:119 reassign deletes role MEMBER instead of OWNER RED: ORG-6 (partial) reassigning…
leave.ts:285 leave deletes role MEMBER instead of OWNER RED: ORG-6 (partial) a lead who leaves…, O-8 guest survives leave
leave.ts:189 OAuth family revocation skipped RED: removal and demotion
leave.ts:225 scopesNameDrive matches any parseable scope RED: removal (own-drive grant survives) and demotion
membership.ts:147 demotion revocation never runs RED: demotion
demotion.ts:73 access-gone drives not revoked RED: demotion
demotion.ts:74 ADMIN→MEMBER drives not capped RED: demotion
demotion.ts:165 page-link check skipped RED: demotion
demotion.ts:98 explicit canShare ignored RED: demotion (a shareable link is revoked)
demotion.ts:129 (d1e6edd) accepted rows → empty map RED: demotion
demotion.ts:129 (d1e6edd) planted tx.select().from(driveMembers) RED: drive-members-enumeration seam guard
org-admin-access-audit.ts:116 (7d094f2) claim release removed RED: failed-write integration and unit tests
org-admin-access-audit.ts:114 (7d094f2) memo entry kept after a failed write RED: failed-write integration test
drive-service.ts:120 (7d094f2) expiry filter removed, dark body RED: expired page share test
drive-service.ts:217 (7d094f2) expiry filter removed, org body RED: matrix and expired page share test
permissions.ts:1048 (ef594e1) back to the ungated return { ...resolved, canDelete: false }; RED: matrix (Board deck [false,true,false,false] for Eve, Kai, Nina, Tomás and Marcus), dark equivalence, resolve-page-permission-row.test.ts
org-admin-access-audit.ts:123 (ef594e1) a lost claim memoized for the whole window (current.expiresAt) RED: both new unit tests and the new integration test

Review follow-ups

  • d1e6edd9e: CI Unit Tests caught the seam guard on demotion.ts's drive_members read. It now goes through loadAcceptedRowsInDrives in the permissions seam. Seam test: 3 passed (3).

  • 7d094f204: review 5237996587 P2s (audit write/claim release; the ORG-4 (partial) audit dedupe describe title) and the expired-share listing fix. drive-service.test.ts + acceptedAt-gate.test.ts: 64 passed (64).

  • ef594e127: review 5262822286. P2: resolvePagePermissionRow gates the custom-role result on canView (fix(permissions): pending drive invites grant nothing; a custom role's drive-wide grant never opens a private page #2672's line), with the Board deck shape in the matrix fixture. P3-1: a lost audit claim is memoized for ORG_ADMIN_AUDIT_LOSS_TTL_MS (30s) instead of the whole window, so a process that lost while the winner's write was in flight, and then failed, writes the record on a later access in the same window. Wins stay memoized for the window. P3-2: this body's dark-flag sentences.

    • At ef594e127 (throwaway homebrew postgresql@17 on 127.0.0.1:5499, session TZ America/Chicago, stopped and deleted after): org-drive-sibling-resolvers.integration.test.ts 7 passed (7) · org-admin-access-audit.integration.test.ts 4 passed (4) · org-admin-access-audit.test.ts 8 passed (8) · resolve-page-permission-row.test.ts 15 passed (15) · bunx tsc --noEmit -p tsconfig.json (packages/lib) 0 errors, with a probe error caught · bunx eslint on the two touched sources: 0 errors.

Commands and results (local; build slot respected, CI is the gate)

Throwaway homebrew postgresql@17 on 127.0.0.1:5498 (session TZ America/Chicago), migrated with bun run --filter @pagespace/db db:migrate. The worktree source harness (vitest.wt*.config.ts, a @pagespace/db source alias) is git-excluded.

  • Integration: DATABASE_URL=postgresql://user@127.0.0.1:5498/pagespace_test bunx vitest run --config vitest.wt-int.config.ts <file> (packages/lib), at 2b0c4bf7d:
    • org-drive-sibling-resolvers.integration.test.ts: 7 passed (7) at 7d094f204
    • org-admin-access-audit.integration.test.ts: 3 passed (3) at 7d094f204
    • org-admin-access-audit.test.ts (unit): 6 passed (6) at 7d094f204
    • org-member-revocation.integration.test.ts: 3 passed (3)
    • leave.integration.test.ts: 12 passed (12)
    • org-services.integration.test.ts: 21 passed (21)
    • org-drive-resolvers.integration.test.ts (B7, now on the shared fixture): 8 passed (8)
    • org-membership-sync.integration.test.ts: 10 passed (10)
    • page-viewers.integration.test.ts: 9 passed (9)
  • Lib unit: bunx vitest run --config vitest.wt.config.ts src/permissions src/services src/organizations src/__tests__/acceptedAt-gate.test.ts src/agent-workspaces src/auth --exclude '**/*.integration.test.ts' gives 245 files, 5792 passed | 1 todo.
  • Web (apps/web source harness): bunx vitest run --config vitest.wt.config.ts src/app/api/orgs "src/app/api/drives/[driveId]/roles" "src/app/api/drives/[driveId]/members" src/app/api/calendar src/app/api/agent-workspaces "src/app/api/drives/[driveId]/agents" "src/app/api/pages/[pageId]/permissions" src/app/api/__tests__/security-audit-coverage.test.ts src/lib/dev-preview/__tests__/manage-decision.test.ts gives 41 files, 755 passed.
  • Types: bunx tsc -p tsconfig.wt.json (packages/lib, tests and fixtures included) reports 0 errors. A probe with an injected type error was caught, so the check is live.
  • bunx eslint on every touched lib source file: 0 errors.
  • Left to CI: monorepo typecheck, apps/web and realtime suites beyond those above, knip (lib __tests__ are knip-ignored; every new export is used in its own file or by lib code), and E2E.
  • New integration files are added to lib vitest.config.ts excludes (unit run and coverage), so they run in ci.yml's lib test:integration step.

Callers of changed functions and what each now sees

Unless stated, the effects apply only while ORGS_ENABLED is on. While dark, every caller sees exactly today's result, except the four fixes above (pending invitations, the private-page drive-wide guard, the view-denying role entry, and expired shares in listAccessibleDrives).

  • Page-level views: an org Owner/Admin is ADMIN, an implicit OPEN member holds the default role, stale org and former-lead rows count for nothing.
    • getBatchPagePermissions: api/search, api/sidebar/badges, api/inbox, api/permissions/batch, api/agent-workspaces/conversations, api/storage/info and lib/storage/storage-info-core, lib/commands/available-commands, lib/auth/principal-permissions, lib tags/tag-service. Cross-drive aggregates remain per-page filtered.
    • getUsersWhoCanViewPage: api/channels/[pageId]/messages (channel fan-out). It writes no audit.
    • getUserAccessiblePagesInDrive(+WithDetails): api/pages/tree, lib/ai/core/page-tree-context, lib/ai/tools/actor-permissions, lib/auth/principal-permissions, lib app-permissions (inheriting tokens).
  • Drive-level checks, same rules:
    • getUserDriveAccess: realtime index.ts (drive room joins), app-logs/app-log-handler; web api/drives/[driveId]/assignees, api/mentions/search, api/storage/info, lib/ai/core/agent-awareness, page-tree-context, actor-permissions, principal-permissions; lib drive-search-service (search gate), drive-agent-service. (hooks/usePageContentSocket only names it in a comment.)
    • isUserDriveMember: api/commands/{resolve,suggest}, api/drives/[driveId]/apps/[tokenId], api/workflows/agents, lib/ai/core/command-resolver; AI tools activity, calendar-read, calendar-write, skill, trigger; principal-permissions, lib/commands/{available-commands,command-catalog-loader}, zoom/page-webhook/calendar trigger executors, workflow-executor; lib app-permissions (hasAppDriveMembership, hasScopedDriveMembership), file-access, share-link-service, calendar-event-drive-service, tag-service.
    • isDriveOwnerOrAdmin: backups routes (backups/[backupId]/pages, drives/[driveId]/backups/**, schedule), api/commands and [commandId], drives/[driveId]/history, lib/ai/tools/command-tools, principal-permissions, lib/dev-preview/manage-decision, services/api/{backup-export-service,drive-backup-service,page-service}; lib rollback-permissions, share-link-service (drive share links), calendar-event-drive-service, drive-agent-service, drive-envs/local-env-gate.
    • getUserDrivePermissions: processor services/{authorization,rbac}; web api/ai/page-agents/multi-drive (+ sandbox-eligibility-by-drive), api/auth/key, api/upload/{complete,presign}, lib/agent-workspaces/principal-code-exec-access, principal-permissions; lib sandbox/can-run-code, validated-service-token. An implicit member's drive-wide edit follows the default role.
    • checkDriveAccess: api/drives/[driveId]/agents/**, apps/**, members/**, api/integrations/zoom/triggers, api/workflows/**, lib/ai/tools/{actor-permissions,agent-tools,member-tools}, lib/repositories/drive-invite-repository.
    • checkDriveAccessForRoles: api/drives/[driveId]/roles/**, lib/ai/tools/role-management-tools. A pending invitation no longer manages roles (dark too).
    • resolveDriveMembership: realtime index.ts and dev-preview/preview-runtime; web api/agent-workspaces/conversations, lib/agent-workspaces/agent-workspaces-runtime, lib/ai/tools/session-tools-runtime, lib/dev-preview/preview-runtime; lib agent-workspace-access, sandbox/preview/{preview-access,dev-preview-status}.
    • isUserMemberOfAnyEventDrive: api/calendar/events/[eventId]/**, lib/ai/tools/calendar-read-tools.
    • getMemberCustomRoleId: lib/auth/oauth-grant-authority, principal-permissions, lib drive-service (validateDriveScopeAccess). An implicit member's own custom role is the default role.
  • getPageIfCanShare (via grantPagePermission and revokePagePermission): api/pages/[pageId]/permissions, lib revocation-kick. An org Admin can grant or revoke page permissions on org drives.
  • resolveGranterAccess (via addAgentToDrive and recapAgentMembershipsGrantedBy): api/ai/page-agents/[agentId]/drives, api/drives/[driveId]/agents, api/drives/[driveId]/members/[userId]. Org power grants agents up to ADMIN; an implicit member is capped to MEMBER with the default role.
  • getDriveIdsForUser: api/auth/key, api/mentions/search, lib/ai/tools/{calendar-read-tools,session-tools-runtime,session-tools}, principal-permissions. It returns the accessible-drives set (see above).
  • updateDriveLastAccessed: api/drives/[driveId]/access. On an org drive it no longer writes an OWNER row (orgs dark: no org drives exist).
  • leaveOrganization, leaveAllOrganizations, reassignLedOrgDrives, removeMember, changeMemberRole: api/orgs/[orgId]/members/[userId], lib repositories/account-repository (account deletion). They revoke OAuth families that name the org's drives, and OWNER rows. Demotion revokes as described.
  • CLI / MCP: they reach all of the above only over HTTP (/api/drives, MCP routes, OAuth). Inheriting MCP and OAuth scopes follow the person. An explicit key role is unchanged.

Not in this PR

  • B7c (point-guard ruling, page kxgdqsjqyrlyvcytnhlbdllz): apps/web inline drive_members / drives.ownerId gates. They fail closed for org power but honour a stale source='org' row:
    • drives/[driveId]/{trash,permissions-tree,pages,members/invite}, pages/{tree,bulk-copy,bulk-move}, account/handle-drive;
    • services/api/{page-reorder-service,permission-management-service}, lib/repositories/drive-invite-repository.findAdminMembership, lib/auth/revoke-adapters;
    • messages/threads (fetchChannelsWithLastMessage), lib/users/visibility, commands and command-tools getMemberDriveIds;
    • activity/pulse/memory scoping, admin/global-prompt, users/messageable, sidebar/badges and inbox pre-filters (their decision is getBatchPagePermissions, routed here).
  • Owner-only checks (drives.ownerId only, correct for a lead and closed to the org Owner): drives/[driveId]/restore, trash/drives/[driveId], ai/page-agents/create, drive-tools rename_drive, drive-repository.findByIdAndOwner.
  • Lib audience and scoping helpers not routed: getDriveRecipientUserIds and getDriveMemberUserIds (drive broadcast recipients), app-shell-service.fetchShellDriveIds, usersShareDrive (DM eligibility, deliberately without the acceptedAt gate). While orgs are on, an implicit member gets no drive-level broadcast or shell entry (fail closed), and a stale org row still receives drive broadcasts until the sync removes it. These belong with B7c.
  • No changelog entry: nothing is user-visible while ORGS_ENABLED=false on the integration branch. On master, the acceptedAt gate is fixed by lane fix-accepted-invite-gate; the private-page leak is reported in #org-wallets for a master ruling.

🤖 Generated with Claude Code

https://claude.ai/code/session_011VHYwVCUxjqUhx9Z5yaxwn

2witstudios and others added 4 commits September 17, 2026 09:51
…red org-aware membership (B7b)

getUserDriveAccess, isUserDriveMember, isDriveOwnerOrAdmin, getUserDrivePermissions,
getUserAccessiblePagesInDrive(+WithDetails), getBatchPagePermissions, getUsersWhoCanViewPage,
getDriveIdsForUser (and through isUserDriveMember hasAppDriveMembership/hasScopedDriveMembership),
plus the lib gates checkDriveAccess, checkDriveAccessForRoles, resolveGranterAccess,
getPageIfCanShare, getMemberCustomRoleId, isUserMemberOfAnyEventDrive and resolveDriveMembership
now read membership only through loadEffectiveDriveMembership / resolveEffectiveDriveMemberships,
the one IO edge over B7's pure resolveEffectiveDriveMembership. getDriveIdsForUser lists exactly
what listAccessibleDrives lists (decideListedDriveRole).

ORG-4 audit dedupe: org-admin-access-audit.ts writes authz.access.granted at most once per
(user, PRIVATE drive) per 15-minute UTC window, via a guarded ON CONFLICT DO NOTHING claim on the
(key, windowStart) primary key of rate_limit_buckets (no migration; the Admin PG ingest grant is
INSERT-only), with an in-process memo in front and fail-open on a claim-store error.

Two fixes the consistency matrix surfaced, both live on master:
- checkDriveAccessForRoles read pending rows (a pending ADMIN invitation managed roles).
- resolvePagePermissionRow let a custom role's drive-wide view open a PRIVATE page in batch
  permissions and the page-viewer fan-out; every other resolver already refused that.

Tests: org-drive-sibling-resolvers.integration.test.ts (consistency matrix, dark-flag equivalence
against a frozen de98839 copy, pending invitations, agent/app identities, granter caps),
org-admin-access-audit.test.ts; the Northwind fixture is shared with the B7 suite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011VHYwVCUxjqUhx9Z5yaxwn
…paths and processes (B7b)

Five resolver calls in one 15-minute UTC window (page view, AI tool check, drive room join, drive
route, batch check), then search's drive gate and per-hit checks, then a second process with an
empty memo, all write one authz.access.granted row; 09:14:59.999 is the same window and 09:15:00
writes a second. An audience computation (getUsersWhoCanViewPage) writes none. The claim itself is
a Postgres unique key: five concurrent claims, one winner.

claimOrgAdminAuditWindow is exported for that last test.

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

- reassignLedOrgDrives deletes the former lead's OWNER row on every drive it reassigns, inside the
  reassignment transaction (a rollback keeps it).
- leaveOrganization deletes the leaver's OWNER rows on all of the org's drives (led now, or led
  once and already reassigned), reported as revoked.formerLeadOwnerRows; personal drives and other
  orgs' drives keep theirs.
- updateDriveLastAccessed writes the owner self-heal row only for a personal drive; on an org
  drive the lead reaches it through drives.ownerId and a row would outlive a reassignment.

Data check (the query in leave.integration.test.ts ownerRowsOnOrgDrives, unfiltered): 0 rows on a
freshly migrated database; the e2e Northwind seed creates no org drive, and every route that sets
drives.orgId is 404 while ORGS_ENABLED is false, so no environment can hold one today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011VHYwVCUxjqUhx9Z5yaxwn
…t, OAuth grants included (B7b)

- revokeOrgDriveGrants (leave.ts) is the one per-drive revocation: agent memberships granted,
  drive and page share links created, MCP key drive rows, and now every OAuth token family whose
  scopes name one of the drives (an explicit OAuth drive role lives in the scope list and is never
  re-checked, so the family is revoked with reason org_access_revoked). leaveOrganization, and
  through it removeMember and account deletion, run it over all the org's drives.
- changeMemberRole runs revokeForDemotion (demotion.ts) in the same transaction. Pure
  planDemotionRevocation compares effective access before and after through
  resolveEffectiveDriveMembership: drives the lower role cannot open get the full revocation;
  drives where ADMIN became MEMBER lose drive share links, page share links the member could not
  share (memberCouldSharePage mirrors getUserAccessLevel), and agent grants above the member cap
  (recapAgentMembershipsGrantedBy, now transaction-aware with a known cap). Inheriting key scopes
  and grants on such drives stay: they follow the person.

Tests: org-member-revocation.integration.test.ts (removal: MCP key scopes and an OAuth grant on
org drives stop resolving, own-drive ones survive; demotion: Finance fully revoked, Product loses
only admin-only artifacts, an invited ADMIN row keeps Omar's; promotion revokes nothing) and
demotion.test.ts.

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

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c22c68aa-ed44-49c5-befb-b3e89b0ee9a1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@2witstudios

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

…m (B7b)

CI Unit Tests: drive-members-enumeration.seam.test.ts flagged a drive_members read in
organizations/demotion.ts. The read moves to loadAcceptedRowsInDrives in
permissions/org-drive-membership.ts, which accepts the role change's transaction.

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

@2witstudios 2witstudios left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[independent-review] agent:ow-irv-2673

Spec review of #2673 (B7b) at head 2b0c4bf7da3e19dd7326537ccb50c44b3341dfa9. I read Spec drc7x34u, leaf gdk2ok99 (all sections), Sequence Spec, Retro 0, and the full diff. I also verified on a throwaway pg17 (127.0.0.1:5641, session TZ America/Chicago, freshly migrated), since stopped.

Verdict: changes requested (posted as COMMENT: GitHub refuses REQUEST_CHANGES on a PR by the same account). One P1 (CI is red on this head) and two P2s. The access model itself holds up: the extended matrix agrees everywhere, the frozen legacy copy is mechanical, and query counts are constant.

Per-ID verdicts

ID Claim Verdict
ORG-4 (partial) in 16 titles (matrix, agent/app identities, dedupe unit ×5 and integration ×2, demotion unit ×4, removal/demotion integration ×2) partial, correctly marked in it titles. I agree it must stay partial: the apps/web inline gates (B7c) and the owner-only actions closed to the org Owner leave "full access on every org-owned drive" unproven. But describe('ORG-4 audit dedupe (integration)') names ORG-4 plainly (P2-1 below).
DRV-5 (partial) matrix + an implicit Open member holds the drive default role… (org-drive-sibling-resolvers.integration.test.ts) partial, correct. The sidebar and picker are Wave F.
DRV-6 (partial) matrix partial, correct. There is no join or approve flow here.
DRV-7 (partial) resolveGranterAccess follows the shared membership… partial, correct
X-6 (partial) matrix partial, correct. Two negatives are tested: Dana, from another org, is denied Northwind's OPEN Product, and Chris, a guest, gets no listing of a second drive through a page share. The policy, wallet and automation clauses belong to other lanes.
ORG-6 (partial) ×3 new (leave.integration.test.ts) partial, correct. The titles are the ones the leaf mandated.
DRV-9 not claimed correct
  • The allowlist is unchanged.
  • No migration.
  • No real Spec ID appears in a gate self-test.
  • Every partial marker is well formed.

CI on this head (run 35238300575)

  • ci / Spec ID coverage: FAILED, log lines 246 to 313:
    • ORG-4 allowlisted partial: …
    • ORG-6 MISSING
    • covered 9/87, allowlisted-missing 77, MISSING 1
    • FAIL: no passing test names these IDs: ORG-6
  • On #2669's run (base) ORG-6 was covered (10/87, MISSING 0). The regression is a cascade of the P1: lib test:coverage exited 1, so later results never reached the gate.
  • Lint & TypeScript, and E2E: green.

P1

P1-1: ci / Unit Tests fails on the lib seam guard (inline at packages/lib/src/organizations/demotion.ts:130).

  • src/__tests__/seams/drive-members-enumeration.seam.test.ts > no NEW file outside the membership seams reads drive_members fails with packages/lib/src/organizations/demotion.ts:130 .from(driveMembers).
  • Result: Test Files 1 failed | 617 passed, and @pagespace/lib#test:coverage exited (1) (unit log ~7885 and 12214).
  • The PR's local unit command (src/permissions src/services src/organizations src/__tests__/acceptedAt-gate.test.ts …) never ran src/__tests__/seams.
  • Fix: move the row read into a services/permissions function (the seam's rule). Only pre-existing moved code may use the allowlist.

P2

P2-1: a plain ORG-4 claim through a describe title (inline at org-admin-access-audit.integration.test.ts:42).

  • hitsFromTestOutcomes (scripts/check-spec-coverage.ts ~425-433) counts an ID named in an ancestor title as covered once every test under that describe passes. nameCarriesId sees ORG-4 audit dedupe as plain.
  • Once the lib integration results reach the gate (after P1 is fixed), ORG-4 becomes STALE-ALLOW and the gate fails. Worse, the file claims ORG-4 in full, which this PR says it does not prove.
  • Fix: rename the describe (e.g. org-admin audit dedupe (integration)) or mark it ORG-4 (partial).

P2-2: the dedupe claim is consumed before an audit write that can fail, and is never released (inline at org-admin-access-audit.ts:93).

  • write is audit(), which fires securityAudit.logEvent(...) and only logs a rejection ([Audit] audit write failed). repository.appendEvent can reject, for example when the Admin PG ingest is unreachable while the main DB, which holds the claim, is up.
  • In that case the (user, PRIVATE drive) claim row already landed, and every later access in the 15-minute window is suppressed. So one failed write loses the chain record for the whole window, including accesses after the audit store recovers.
  • The module's own contract says "Over-auditing a PRIVATE drive is acceptable; losing the record of an access is not."
  • Fix: let the writer return the append promise, and on rejection delete the claim row (and drop the memo entry) so the next access retries. Add a unit test with a rejecting writer.

Rest of the brief, verified

(a) Consistency matrix. The lane's suites reproduce at 2b0c4bf7d: sibling-resolvers 6/6, audit 2/2, revocation 3/3, leave 12/12, B7 resolvers 8/8.

  • I extended the matrix fixture (git-excluded probe, not committed) with shapes the lane did not have:
    1. a trashed PRIVATE org drive that the org Admin reaches only through org power;
    2. an OPEN drive whose default role names a private page explicitly, plus drive-wide view;
    3. a pending ADMIN invitation on an OPEN drive for an org MEMBER;
    4. a RESTRICTED drive where a row's custom role has drive-wide view, next to a private page;
    5. explicit canView=false page permissions, for an implicit OPEN member and for an org Admin;
    6. an expired page share.
  • Every resolver agreed with getDriveAccess and getUserAccessLevel for shapes 1 to 4, and for the org Admin's deny in shape 5. The remaining disagreements are not caused by this PR (P3 notes below).
  • Mutation 1 (by line index, with a no-op control): drive-member-service.ts:96, where checkDriveAccess loads membership as { id: drive.id } so the shared loader treats it as a personal drive. The matrix went RED: 1 failed | 5 passed. Control: 6/6.

(b) Routing.

  • All 9 named siblings and the 7 extra lib gates call loadEffectiveDriveMembership or resolveEffectiveDriveMemberships. Every call site passes a full drive row, or a select that includes orgId and orgVisibility.
  • The lib functions left reading drive_members or drives.ownerId are:
    • listings or audiences: getDriveMemberUserIds, getDriveRecipientUserIds, getAllMemberUserIdsForEvent, app-shell-service, usersShareDrive, listAccessibleDrives (B7);
    • mutations: share-link accept upserts, org-membership-sync, deletion, leave, demotion.
  • All of these are named for B7c or are not access decisions.

(c) Dark flag.

  • I diffed the 15 functions in pre-b7b-sibling-resolvers.ts against git show de988390e with whitespace normalised. They are identical except for the LEGACY_VIEWER_BATCH_SIZE constant name, so the frozen copy is mechanical and not self-graded.
  • By code reading, the only other behaviour changes are for a nonexistent drive id: isDriveOwnerOrAdmin, isUserDriveMember and getUserAccessiblePagesInDrive no longer query membership, which the FK makes unreachable.
  • The two declared diffs:
    • the private-page guard in resolvePagePermissionRow;
    • the acceptedAt gate in checkDriveAccessForRoles, by point-guard ruling.

(d) Former-lead OWNER row.

  • reassignLedOrgDrives deletes the row per drive inside the transaction. The rollback test proves it is in the same transaction.
  • leaveOrganization deletes it on every org drive.
  • updateDriveLastAccessed is guarded by orgId === null.
  • Data check on my freshly migrated pg17: drive_members with role OWNER joined to drives with orgId not null returns 0. There are 0 org drives.

(e) Dedupe on rate_limit_buckets. The choice is sound apart from P2-2.

  • Key namespace. Every checkDistributedRateLimit or reset… caller in apps and packages uses a fixed literal prefix (oauth-token:, magic_link:, authfail:, webhook-seen:, …). None is caller-controlled or starts with audit:, so nothing can pre-claim or reset an audit key.
  • Types. window_start and expires_at are timestamptz. The window is epoch-ms arithmetic, which is UTC.
  • Sweep. It deletes expires_at < now(), and expiresAt is the window end, so a live dedupe row is never swept early. After the window ends the key's windowStart differs, so a sweep cannot cause a duplicate.
  • Growth. At most one row per (admin, PRIVATE drive, 15 minutes), bounded by the existing cron sweep.
  • Concurrency. Covered by the integration test: 5 racers with 1 winner, and a fresh module graph as a second process.
  • Mutation 2 was not on this file. See (f).

(f) Removal and demotion.

  • Removal runs the leave cascade. It revokes mcp_token_drives rows and OAuth families whose scopes name org drives; the integration test resolves the key and the OAuth token before and after.
  • Demotion revokes everything on access-gone drives, and only drive links, unshareable page links and the agent recap on OPEN drives. Inheriting scopes stay. Omar's invited ADMIN row is untouched.
  • Mutation 2 (by line index, with control): demotion.ts:102, removing memberCouldSharePage's private-page guard. demotion.test.ts went RED: 1 failed | 4 passed. Control: 5/5. git status was clean after both mutations.

(g) acceptedAt.

  • The shared loader, getDriveIdsForUserWithOrgs, both batch joins and the demotion row read all filter acceptedAt.
  • The pending-invite test covers every routed function, dark and enabled. My extension adds a pending ADMIN invitation on an OPEN drive, which resolves as the implicit MEMBER.

(h) N+1. I spied on pool.query with orgs enabled:

Call Queries
getBatchPagePermissions, 40 pages 4
getBatchPagePermissions, 120 pages across 3 org drives 4
getUsersWhoCanViewPage, 1 candidate 4
getUsersWhoCanViewPage, 13 candidates 4
getDriveIdsForUser 6
…WithDetails 7

No N+1. revokeForDemotion loops per capped drive inside the transaction, which is bounded by the org's drives and is a mutation, not a listing.

(i) ORG-4 stays partial. Yes, for the reasons in the table.

Rules.

  • The decision logic is pure (planDemotionRevocation, memberCouldSharePage, createOrgAdminAccessAuditor with injected IO).
  • Permissions go through permissions/.
  • No any.
  • ORGS_ENABLED is a code constant.
  • No new now().
  • IN lists are chunked at 500.
  • No new @pagespace/lib subpath is imported by apps.
  • Scope matches the leaf.

P3 (no thread needed)

  1. The PR body says getDriveIdsForUser returns "exactly" the listAccessibleDrives({includeTrash}) set. Not quite.
    • listAccessibleDrives ignores pagePermissions.expiresAt, so a user whose only link to a drive is an expired page share gets it listed, while getDriveIdsForUser (correctly) omits it.
    • Reproduced on pg17: getDriveIdsForUser: pat answered [], listAccessibleDrives ["…"].
    • The bug is pre-existing in listAccessibleDrives on master (it lists a drive name through an expired share). Worth a master ticket; not this lane.
  2. Pre-existing, dark and enabled alike: a drive member with an explicit canView=false page permission still gets that page from getUserAccessiblePagesInDrive(+WithDetails), while getUserAccessLevel denies it.
    • Shown for Nina (implicit Product member) and for Dana on a personal drive. The legacy copy is identical.
    • The tree leaks the title of an explicitly revoked page. This is for master, not this lane.
  3. getMemberCustomRoleId does not skip the drive's lead. On an OPEN org drive, a lead who is an org MEMBER now gets the default role's id (null before). All 3 callers check owner first, so this is latent only.
  4. MembershipDrive.orgId is optional, and undefined means a personal drive.
    • A future caller that selects only { id, ownerId } silently skips the org model. Mutation 1 shows exactly that.
    • Consider making orgId and orgVisibility required.
  5. The new ORG-6 tests in leave.integration.test.ts keep that file's if (!dbAvailable) return; pattern, so they pass without doing anything when no DB is available. CI has a DB; the pattern is pre-existing.
  6. The audit writes in the name of the wrong person on some paths. When recapAgentMembershipsGrantedBy resolves the granter on another actor's action, it audits org-admin access by the granter, who accessed nothing. This errs toward over-auditing.

Comment thread packages/lib/src/organizations/demotion.ts Outdated
Comment thread packages/lib/src/permissions/__tests__/org-admin-access-audit.integration.test.ts Outdated
Comment thread packages/lib/src/permissions/org-admin-access-audit.ts Outdated
@2witstudios

Copy link
Copy Markdown
Owner Author

CI fix at d1e6edd: Unit Tests failed on drive-members-enumeration.seam.test.ts (a drive_members read in organizations/demotion.ts). The read now goes through loadAcceptedRowsInDrives in permissions/org-drive-membership.ts, which accepts the transaction. Local: seam test 3 passed (3); organizations unit + seams 107 passed; org-member-revocation.integration.test.ts 3 passed (3) on pg17. Mutation demotion.ts:129 (rows → empty map): control 3/3 GREEN, mutant RED (demotion test). Spec ID coverage failed with ORG-6 MISSING, most likely because the red unit job never produced the integration results that carry the full ORG-6 titles; this push re-runs it.

…; expired shares list no drive (B7b)

Review 5237996587:
- P2: the dedupe claim was consumed before a fire-and-forget audit write. The write is now awaited
  (same structured log + securityAudit.logEvent as audit()), and a failed write removes the
  in-process memo entry and deletes the claim row, so the next access in the window writes the
  record. record() still never rejects.
- P2: the dedupe describe title made a plain ORG-4 claim; retitled 'ORG-4 (partial) audit dedupe'.
- Point-guard: listAccessibleDrives (dark and org bodies) ignored pagePermissions.expiresAt, so an
  expired page share still listed its drive while getDriveIdsForUser and every access resolver
  refused it. Both bodies now filter expired shares; master has the same unfiltered query.

Tests: unit 'a failed audit write releases the claim...', integration 'a failed audit write
releases the window...' (securityAudit.logEvent rejected once), matrix fixture gains Lu's expired
share on Marcus Notes plus 'an expired page share lists no drive, dark or enabled'.

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

@2witstudios 2witstudios left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[independent-review] agent:ow-irv-2673b

Re-review of #2673 (B7b) at 7d094f204841cbdc5d64264c1add45d5de23ed0a, covering the delta 2b0c4bf7d..7d094f204 (d1e6edd, 7d094f2) against review 5237996587.

I read the leaf gdk2ok99 (all sections), Spec drc7x34u, reviews 5235984739, 5236485572 and 5237996587, and the three thread replies. All my evidence comes from a throwaway homebrew pg17 (127.0.0.1:5673, session TZ America/Chicago, freshly migrated), since stopped. I changed no code; probes were git-excluded and removed, and git status was clean after every mutant.

Verdict: all three findings of 5237996587 are FIXED. One new P2 (inline), 0 P1.

Previous findings

Finding Status Evidence
P1: demotion read failed the drive-members seam guard FIXED See (1) below
P2-1: plain ORG-4 through the describe title FIXED See (2) below
P2-2: dedupe claim consumed before a write that can fail FIXED See (3) below
Orchestrator add: expired page share listed its drive FIXED See (4) below

(1) The demotion read now goes through the permissions seam, not the allowlist.

  • demotion.ts:129 calls loadAcceptedRowsInDrives(tx, …) in permissions/org-drive-membership.ts. It runs the same select as before, with the same acceptedAt filter, on the same transaction; an empty id list yields an empty map. The seam test file is untouched in the delta.
  • My plant: a new file src/organizations/zz-review-plant.ts with db.select().from(driveMembers). The seam test reported 1 failed | 2 passed, naming zz-review-plant.ts:6. After deleting the plant: 3 passed.
  • CI on this head (Test Suite run 35242406812):
    • Unit Tests is green, and lib reports Test Files 618 passed | 5 skipped (was 1 failed | 617).
    • Spec ID coverage is green: ORG-6 covered, covered 10/87, allowlisted-missing 77, MISSING 0, spec-coverage: OK.
    • Security Tests run 35242406569 is green.

(2) The describe title is fixed and ORG-4 is not counted as covered.

  • The describe reads exactly ORG-4 (partial) audit dedupe (integration).
  • Spec job: ORG-4 allowlisted partial: …, listing org-admin-access-audit.integration.test.ts among the partial files. ORG-4 is still on scripts/spec-coverage-allowlist.txt:68, not counted as covered.

(3) The claim is now released when the audit write fails. write is awaited. On rejection the auditor drops the memo entry and deletes the claim row (key, windowStart). record() still never rejects. My own probe on pg17, beyond the lane's test:

  • securityAudit.logEvent throws synchronously twice in a row: first on the drive route (getDriveAccess), then on a page view. After each failure the claim row is gone and there are 0 audit rows.
  • A third access at 10:59:59 in the same window writes exactly 1 row, and a fourth writes none.
  • The claim row reads windowStart 10:45:00Z and expiresAt 11:00:00Z, so the window end is correct on a Chicago session.
  • The lane's suites: audit integration 3/3 and unit 6/6.

The rate_limit_buckets choice still holds.

  • Key namespace. I re-checked every check/reset…DistributedRateLimit caller in apps and packages. All use a fixed literal prefix (export:user:, passkey_auth:, signup:, integration:, page-webhook*:, email:, oauth*:, …). None can produce audit:, and only this module writes audit:org-admin-private-drive: keys.
  • No early sweep. The sweep is expires_at < now() on timestamptz, which does not depend on the session TZ. expiresAt is the window end.
  • No unbounded growth. There is at most one row per (user, PRIVATE drive, 15 minutes), and a failed write deletes rather than adds.

(4) An expired share no longer lists its drive. Both listAccessibleDrives bodies filter expiresAt IS NULL OR expiresAt > now. Probe on the Chicago session, with shares expired 2h ago and 60s ago, live for another 2h, never expiring, and one expired on a personal drive:

  • Dark: listAccessibleDrives = getDriveIdsForUser = {personal, research}.
  • Enabled: both = {personal}; a page share alone lists no org drive.
  • Mutants, with the control green:
    • Line 120, dark body filter removed: RED. It lists handbook, finance and the expired personal drive, which is exactly master's behaviour.
    • Line 217, org body filter removed: RED. It lists the expired personal drive.
  • Master differs: origin/master drive-service.ts:110 still has the unfiltered permissionDrives query. It needs its own master lane, as the PR body says.

(5) Consistency matrix.

  • I copied the lane's matrix and added my own people:
    • Ola: org ADMIN with an accepted invite MEMBER row on PRIVATE Finance.
    • Pia: org MEMBER with no row and only a page share (view+edit) on Finance's private page. getUserDriveAccess is true; she is not listed.
    • Rex: removed from the org, still holding a stale source:'org' row on OPEN Product and an invite ADMIN row on Research.
  • With 16 people × 6 drives, every resolver agrees, and the non-vacuity asserts hold.
  • Mutants, by line index with an anchor guard and the control green; neither function was mutated by the lane or the previous reviewer:
    • permissions.ts:414: isDriveOwnerOrAdmin reads the raw drive_members row. RED, including isDriveOwnerOrAdmin: ola on Finance answered false, canonical true.
    • permissions.ts:453: getUserAccessiblePagesInDrive reads the raw row. RED.
  • A fourth shape of my own fails the matrix: P2 below.

(6) Dark flag. The lane's dark-equivalence test passes at head (exactly 4 diffs against the frozen de988390e copy).

  • Both declared fixes are now on master via #2672 (merged 2026-09-18, 6d5d088c8): the resolvePagePermissionRow private-page guard and the checkDriveAccessForRoles acceptedAt gate.
  • The expired-share filter is a third dark-visible change and is not on master. It is declared in the "Fixes" list, but the callers section still says "While dark, every caller sees exactly today's result, except the two fixes above" (P3).
  • The delta's audit and demotion code runs only on org power or org role changes.

(7) Partial markers. Every Spec ID in an added test title is (partial): ORG-4 ×19, ORG-6 ×3, DRV-5 ×2, DRV-6, DRV-7 and X-6. The only plain mentions are in code comments. ORG-4 must stay partial until B7c, because the apps/web inline gates and the owner-only actions closed to the org Owner are unproven.

P2 (new, inline at permissions.ts:1048)

A role entry that denies view still grants edit in batch permissions.

  • resolvePagePermissionRow returns { ...resolved, canDelete: false } even when resolved.canView is false. getUserAccessLevel (line 326 here) returns null in that case.
  • The shape: Product's default role holds a per-page entry {canView:false, canEdit:true}. With orgs on, the matrix goes RED for nina, kai, eve, marcus, tomas and pia: getBatchPagePermissions … answered [false,true,false,false], canonical [false,false,false,false].
  • Every implicit OPEN member holds the default role, so enabling orgs spreads this to the whole org.
  • #2672 closed exactly this on master (return resolved.canView ? { ...resolved, canDelete: false } : null;, with the test grants nothing when the custom role entry denies view, whatever else it sets). This PR carries only the private-page half of that hunk.
  • With master's line applied at 1048, my extended matrix goes green, and the lane's suite (including dark equivalence at exactly 4 diffs) plus resolve-page-permission-row.test.ts pass: 22 passed.
  • While dark this matches the pu/org-wallets base, so it is not a regression. But it breaks the leaf's "every resolver agrees", and permissions.ts conflicts between master and pu/org-wallets. Taking this side of the conflict would silently undo #2672.

P3 (no thread needed)

  1. A lost claim can suppress audits after a failed write. A process that loses the claim while another process's write is in flight and then fails memoizes the loss, and writes nothing more in that window. Probe: process B lost during A's failing write, and B's re-access in the same window left 0 rows and 0 claim rows. Only a later access in another process (A did) restores the record. This is a narrow race (a failing write overlapping a concurrent access in another process), not the deterministic loss P2-2 was. A short TTL on memoized losses would close it.
  2. The PR body's dark-flag sentence is stale. It says "the two fixes above"; there are three dark-visible changes, and only the expired-share one is still missing from master.
  3. The merge with master will conflict in permissions.ts, drive-role-service.ts and drive-service.ts, all touched by #2672. Resolve toward master's lines. git merge-tree origin/master origin/pu/org-wallets already conflicts there, so this belongs to the integration branch, not this lane.

Suites at head, my runs: sibling-resolvers 7/7, B7 resolvers 8/8, audit integration 3/3 and unit 6/6, revocation 3/3, demotion 5/5, drive-service 40/40, seams 3 files.

Comment thread packages/lib/src/permissions/permissions.ts Outdated
… in the batch resolver; a lost audit claim is trusted only briefly (B7b)

- resolvePagePermissionRow takes master's #2672 line: resolved.canView gates the custom-role result,
  so {canView:false, canEdit:true} no longer grants edit through getBatchPagePermissions while
  getUserAccessLevel returns null. The matrix fixture gives Product's default role that entry on a
  Board deck page; the dark-equivalence test names it as the third intended difference.
- The ORG-4 auditor memoizes a lost claim for ORG_ADMIN_AUDIT_LOSS_TTL_MS (30s), not the whole
  window: a loss observed while the winner's write is in flight, and then fails, no longer
  suppresses this process's audit for the rest of the window. Wins stay memoized for the window.

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

@2witstudios 2witstudios left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[independent-review] agent:ow-irv-2673b

Re-review of #2673 (B7b) at ef594e12767e55d8d9693dd27b98d816c9eeca09, covering the delta 7d094f204..ef594e127 (one commit) against my review 5262822286 and the point-guard rulings.

Evidence comes from a throwaway homebrew pg17 (127.0.0.1:5673, session TZ America/Chicago, freshly migrated), since stopped and deleted. I changed no code; probes were git-excluded and removed, and git status was clean after every mutant.

Verdict: every ruled item is FIXED. No new findings: 0 P1, 0 P2, 0 P3.

Finding (5262822286) Ruling Status Evidence
P2: a role entry with canView:false still grants edit in resolvePagePermissionRow fix on #2673 with #2672's line, plus the shape in the matrix FIXED See (1)
P3-1: a memoized claim loss suppresses later audits short TTL on losses, with the probe as a test FIXED See (2)
P3-2: stale dark-flag sentence in the PR body correct the body FIXED See (3)
P3-3: master vs pu/org-wallets conflicts point-guard's master-sync PR out of scope here not reviewed

(1) P2 is fixed.

  • permissions.ts:1048 is now return resolved.canView ? { ...resolved, canDelete: false } : null;, byte-identical to master's #2672 line.
  • The matrix fixture gives Product's default role a Board-deck entry {canView:false, canEdit:true}. The enabled matrix asserts DENY for nina, kai and marcus through the default role.
  • Dark equivalence now expects exactly five differences. The new one is Marcus's edit-without-view on the Board deck, which master also no longer grants.
  • resolve-page-permission-row.test.ts carries #2672's unit case.
  • My mutant, by line index with the control green (22/22): line 1048 back to return { ...resolved, canDelete: false }; turns three tests RED:
    • org-drive-sibling-resolvers.integration.test.ts:313 (dark equivalence);
    • :341 (the enabled consistency matrix);
    • resolve-page-permission-row.test.ts:132.
  • My extended matrix (Ola, Pia and Rex from my last review) on the new fixture: 16 people × 6 drives, every resolver agrees.

(2) P3-1 is fixed. settled is now a Map from key to the time the settlement stays trusted. A win is trusted until the window's end; a loss only for ORG_ADMIN_AUDIT_LOSS_TTL_MS (30s), after which the next access asks the claim store again.

  • My probe, through the real resolvers (getUserAccessLevel, two module graphs):
    1. A wins and its logEvent hangs; B loses.
    2. A's write fails, and the claim is released.
    3. B at TTL−1s writes 0 rows.
    4. B at TTL+1s writes exactly 1.
    5. Further B and A accesses in the window write none, and the next window writes a second row.
  • Mutants, with the control green:
    • A loss trusted for the whole window (current.expiresAt for both outcomes): RED, in 2 unit tests and the new integration test (:198).
    • A loss never trusted (TTL 0): RED, in the unit test that checks the claim store is not asked again inside the TTL (:132).
  • Load stays bounded: at most one claim-store round trip per (user, PRIVATE drive) per 30s per process for a loser, and none for a winner inside its window. The unit test asserts 11 claims across 10 lapsed re-checks.
  • The residual is inherent: a loser whose only access falls inside the TTL, with no later access anywhere, cannot know the winner failed. That is now bounded to 30s instead of 15 minutes.

(3) P3-2 is fixed. The body now names four dark-visible fixes: pending invitations, the private-page drive-wide guard, the view-denying role entry and expired shares. It says dark equivalence has exactly five intended differences, and that the expired-share fix is covered by its own test because listAccessibleDrives is not a routed sibling. The first three are on master through #2672; the expired-share filter is not.

Other checks.

  • Every Spec ID in the new test titles is (partial); the only plain mentions are in code comments. ORG-4 stays partial until B7c.
  • The allowlist is unchanged, and there is no migration.

Suites and CI.

  • My runs at head: sibling-resolvers 7/7, B7 resolvers 8/8, audit integration 4/4 and unit 8/8, revocation 3/3, demotion 5/5, drive-service 40/40, batch-page-permissions 22/22, resolve-page-permission-row 15/15, seams 3 files. Total: 12 files, 145/145.
  • CI on this head:
    • Test Suite run 35558047032 is green: Unit Tests (lib 618 passed | 5 skipped), Lint & TypeScript, E2E, and Spec ID coverage (ORG-4 allowlisted, ORG-6 covered, covered 10/87 … MISSING 0, spec-coverage: OK).
    • Security Tests run 35558046814 is green.

I have not resolved any threads.

@2witstudios
2witstudios merged commit d1f0c0d into pu/org-wallets Sep 21, 2026
12 checks passed
@2witstudios
2witstudios deleted the pu/ow-b7b branch September 21, 2026 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant