[iscekic]

Evidence

Evidence: partially demonstrated — the follow-up verification ran the organization PostgreSQL suites on this head and observed both operations complete under a deliberately overlapped run; no visual surface changed.

Verification

• A new concurrent test holds the settings-path policy lock on one connection and then updates organization settings, while invitation acceptance for the same organization runs on a second connection. It waits on PostgreSQL's own lock-wait state until the two transactions overlap, then asserts that the settings write and the membership acceptance both complete. The same scenario surfaces a PostgreSQL deadlock error when the two paths take those locks in the opposite order, so the test fails without the ordering change.
• The settings row is created before the overlapped run, so its first insert cannot mask the ordering under test.
• NODE_ENV=test pnpm exec jest --runInBand over packages/web-shared/src/lib/organizations/organizations.test.ts, organization-groups.test.ts, organization-group-policy-context.server.test.ts, and apps/web/src/routers/organizations/organization-settings-router.test.ts — 4 suites passed, 156 tests passed and 2 skipped, against a fresh local PostgreSQL with all migrations applied.
• pnpm --filter @kilocode/web-shared typecheck and the repository lint configuration over the changed files — passed.
• No visual change was introduced; no screenshots were captured.

Change

Invitation acceptance now takes its membership-mutation advisory lock and the organization group-policy advisory lock before the organization row lock, the same order the settings mutations and SSO membership joins use. The policy lock helper is shared rather than duplicated, so every caller requests the same lock.

Concurrent invitation acceptance and settings writes for one organization therefore queue instead of holding each other's locks, and the policy revision still advances in the transaction that inserts the membership.

Production prerequisites

None.

Proxy answers

1. Question: Transaction structure — Answer: Keep membership insertion and the policy-revision update in the same transaction. Confidence: 0.72. Precedent: Prefer the smallest correct implementation.
2. Question: Preferences persistence — Answer: Persist and return the schema-parsed value. Confidence: 0.75. Precedent: Use a consistent stored representation.
3. Question: Membership no-op — Answer: Leave the policy revision unchanged when no membership is created. Confidence: 0.70. Precedent: Avoid work for unchanged state.
4. Question: Earlier publication recovery — Answer: Temporary local credential routing through the existing control credential, with the publication gate unchanged and local configuration restored afterward. Confidence: 0.55. Precedent: Complete the task through the required checks.
5. Question: Follow-up publication recovery — Answer: Temporarily use the existing control credential for local provider lookups, retain the unchanged publication gate, and restore the provider configuration afterward. Confidence: 0.60. Precedent: Approve narrowly scoped infrastructure recovery.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)

• packages/web-shared/src/lib/organizations/organizations.ts
• packages/web-shared/src/lib/organizations/organization-groups.ts
• packages/web-shared/src/lib/organizations/organizations.test.ts

Incremental review of dc527501e verified that invitation acceptance now takes the membership advisory lock, then the group-policy advisory lock, then the organization row lock, matching the SSO join (addUserToOrganization), removal, and settings/updateAllowLists paths, so the reported row-lock/policy-lock inversion is removed. The new PostgreSQL regression test meaningfully distinguishes the old order (deadlock/timeout) from the new one, and the lockOrganizationGroupPolicy export has no remaining references to the old name.

Remaining invite-acceptance vs. member-removal/invite ordering around the invitation-row lock is unchanged pre-existing behavior and is outside this diff.

Previous Review Summary (commit 9ca5eb8)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 9ca5eb8)

Status: No Issues Found | Recommendation: Merge

Executive Summary

All three hardening changes in packages/web-shared/src/lib/organizations/organizations.ts are correct: the invite token no longer reaches the logger, the group-policy revision bump matches the SSO/removal lock order after the membership insert, and updateOrganizationSettings now parses through OrganizationSettingsSchema exactly like mutateOrganizationSettings; verified callers all spread schema-declared keys so the parse-and-strip change does not drop data.

Files Reviewed (2 files)

• packages/web-shared/src/lib/organizations/organizations.ts
• packages/web-shared/src/lib/organizations/organizations.test.ts

Notes on verification

• Lock ordering: acceptOrganizationInvite takes the membership advisory lock (lockOrganizationMembershipMutation, line 717) before the policy lock via bumpOrganizationGroupPolicyRevision (line 757), matching addUserToOrganization/SSO (organizations.ts:354-367) and the removal path (organization-member-removal.ts:19-20) — no new deadlock class.
• Bump placement: only the real-insert branch bumps, after the insert, inside the same transaction; the already-a-member branch returns early without bumping, consistent with addUserToOrganization bumping only on an actual insert.
• invitation.id is in scope at the changed log line (row selected with .for('update') at line 640) and is a non-secret identifier.
• Settings validation: every non-test caller spreads previously-stored settings, and all jsonb keys read elsewhere (is_sales_demo, oss_sponsorship_tier, recommendations_digest_enabled, code_indexing_enabled, oss_*, suppress_trial_messaging) are declared in OrganizationSettingsSchema (packages/db/src/schema-types.ts:997), so unknown-key stripping is safe.

---

_{Reviewed by deepseek-flash-latest · Input: 102K · Output: 28.3K · Cached: 1.4M}

_{Review guidance: REVIEW.md from base branch main}

[iscekic]

Changes required before merge: the new policy revision bump creates a lock-order inversion.

At packages/web-shared/src/lib/organizations/organizations.ts:699, acceptOrganizationInvite locks the organization row with FOR UPDATE. The new call at line 801 then requests the organization policy advisory lock.

The settings path takes the opposite order: apps/web/src/routers/organizations/organization-settings-router.ts:330 acquires the policy advisory lock, then mutateOrganizationSettings locks the organization row at organizations.ts:882.

An isolated PostgreSQL reproduction completed with the base lock sequence and raised SQLSTATE 40P01 with the added policy lock. This reproduces the lock sequence, not the full application flow.

Use a consistent lock order across invitation acceptance and settings mutations. Add a concurrent PostgreSQL regression test for these paths before merging.

[iscekic]

bot: Addressed the lock-order finding.

• acceptOrganizationInvite now acquires the membership-mutation advisory lock and the organization group-policy advisory lock before the organization row lock. The settings mutations take the policy lock first and the row lock second, and SSO membership joins take the membership lock before the policy lock, so every path requests these three locks in one order.
• The policy lock helper in organization-groups.ts is exported and reused instead of duplicated, so callers cannot drift.
• Regression coverage: organizations.test.ts runs invitation acceptance and a settings mutation concurrently for one organization. One connection holds the policy lock and then updates settings; the test waits on PostgreSQL's lock-wait state until the acceptance transaction is blocked, releases the settings write, and asserts both operations complete. On commit 7af586e287 (previous head) the same test observed SQLSTATE 40P01 and acceptance failed; on dc527501e0 it passes.
• Repro detail for anyone re-running it: the settings row is seeded before the overlapped run, because creating it inside the transaction holds a key-share lock on the organization row and hides the ordering under test.

Suites run on dc527501e0 against migrated local PostgreSQL: organizations.test.ts, organization-groups.test.ts, organization-group-policy-context.server.test.ts, organization-settings-router.test.ts (4 suites, 156 passed, 2 skipped), plus pnpm --filter @kilocode/web-shared typecheck and the root lint config on the changed files.

Thanks for the precise reproduction — it pointed straight at the ordering rather than the revision bump itself.

[iscekic]

bot: Correction to the evidence line in my previous comment, so the record matches what actually ran.

• The deadlock observation was not produced by commit 7af586e287. That commit predates the regression test.
• What ran: on dc527501e0, with acceptOrganizationInvite's lock acquisition temporarily reverted to the previous order in the working tree (uncommitted), the concurrent test observed DrizzleQueryError … cause: error: deadlock detected, code: '40P01' and acceptance returned failure. With the branch as committed, the same test passes.
• The test exists only from dc527501e0 onward; the suites, typecheck, and lint results reported earlier were all run on that head.

Scope note on ordering, to avoid overstating the guarantee: the single order now covered is membership-mutation advisory lock, then organization group-policy advisory lock, then organization row lock. Invitation acceptance, SSO membership joins, member removal, and the settings mutations all follow it for those three locks. The invitation row lock that acceptance takes before the membership advisory lock is unchanged and not part of that order; member removal deletes pending invitation rows after taking the membership advisory lock, so that pair keeps whatever behaviour it had before this branch.