[iscekic]

Evidence

Evidence: demonstrated — the running web app completed both providers' connect and callback flows with real application authentication, real PostgreSQL integration rows and real Redis installation storage; only provider network responses were simulated. Focused database and route regressions also passed.

Change

Slack and Linear callback persistence uses primary-PostgreSQL advisory locks, acquired in provider-owner then provider-workspace order. Each transaction sets a five-second lock timeout and a thirty-second SQL statement timeout, matching the existing integration convention. Ownership reads and integration writes use the lock transaction. Adapter installation operations and restoration complete within the protected operation. Organization model defaults are resolved before entering the transaction. Slack credential mirroring uses a savepoint and retains its best-effort behavior. The organization-management configuration from main is preserved.

The coordination contract is documented in the existing security architecture overview.

Verification

Running-app proof head: b69d978fa2.

• Focused callback, authority, integration-service and credential-store regressions: seven suites, 99 tests passed. Both success-path route tests invoke the real upsert service and assert the persisted integration row; organization workspace updates are checked against real rows.
• pnpm --filter web run typecheck passed; direct web tsgo passed after the test migration.
• Oxlint and formatting checks on the changed integration files passed.
• Running-app verification uses real application authentication, provider SDKs, PostgreSQL integration rows and Redis installation storage. Only provider network responses come from a local fixture. Installation, reconnect, ownership and concurrent-callback outcomes were checked for both providers.
• Ownership handling was exercised for both providers: a connect for a workspace that already had a recorded owner returned the standard workspace-already-connected redirect, and that workspace's integration row and stored installation were unchanged afterwards.
• An organization-owned Slack install was exercised end to end: the callback redirect reached the organization integration page, stored an active row, and recorded the model resolved from the organization's policy before the transaction.
• Observed provider calls: Slack OAuth exchange; Linear OAuth token exchange and GraphQL identity/user lookup.
• Real provider consent and upstream token behavior remain unproven. The runtime run covers personal and organization-scoped installs; organization connection migration and enterprise-wide installations were not exercised.

This is nonvisual HTTP and storage behavior; no screenshot is attached.

Production prerequisites

None.

Proxy answers

The owner's proxy gave these answers in the owner's place.

1. Question: Installation design — Answer: Pre-flight ownership check, then persist in the callback. Confidence: 0.75 · Precedent: Address the root cause.
2. Question: Token lifecycle — Answer: Do not revoke the newly issued token on this outcome. Confidence: 0.80 · Precedent: None.
3. Question: Restoration contract — Answer: Restore the prior installation; delete only state created by the callback. Confidence: 0.75 · Precedent: Apply the shared lifecycle rule.
4. Question: Provider coverage — Answer: Include Slack using the same ownership contract; a separate pull request was initially preferred. Confidence: 0.50 · Precedent: Apply the class of fix across affected surfaces.
5. Question: Verification scope — Answer: Route tests plus a real-Postgres integration test. Confidence: 0.60 · Precedent: Require end-to-end evidence.
6. Question: Pull request placement — Answer: Include Slack in this pull request under the work item's one-pull-request limit. Confidence: 0.75 · Precedent: Apply the shared contract across affected surfaces.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental commit resolves the owner's model policy before the advisory-lock transaction and honors an explicit null as a resolved no-model value, and the main merge retains the provider-bound OAuth state checks; I found no bug, security, or data-integrity issue in the changed production or test lines.

Assumptions: [INFERENCE] The sandbox is read-only, so the database-backed callback suites and the nested-savepoint credential write were verified by inspection only; their passing status is taken from the PR description. [INFERENCE] node_modules is unavailable, so the hand-rolled Slack installation shape and the Linear viewer { organization } lookup could not be compared against the Chat SDK packages, and enterprise-wide installs remain unexercised as the PR states.

Files Reviewed (14 files)

• apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts
• apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts
• apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts
• apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts
• apps/web/src/lib/integrations/slack-service.ts
• apps/web/src/lib/integrations/slack-service.test.ts
• apps/web/src/lib/integrations/linear-service.ts
• apps/web/src/lib/integrations/linear-service.test.ts
• apps/web/src/lib/integrations/platforms/slack/credential-store.ts
• apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts
• apps/web/src/app/api/integrations/slack/callback/route.test.ts
• apps/web/src/app/api/integrations/linear/callback/route.test.ts
• apps/web/src/app/api/integrations/oauth-install-conflict.test.ts
• docs/security-architecture-overview.md

Verification notes

• The merge of main keeps fix(integrations): bind signed OAuth state to its integration flow #8055's provider-bound state coverage in route.test.ts (provider mismatch, missing provider claim, wrong purpose), and both callbacks still reject states whose verified.provider differs from the platform.
• The new defaultModel branch treats undefined as "resolve here" and null as "policy allowed no model", matching the explicit value the callbacks now pass after resolving outside the lock; metadata still preserves an existing model_slug.
• writeSlackCredential now accepts the install transaction and opens a nested transaction, which Drizzle implements as a savepoint, so a credential failure is rolled back without aborting the installation write.
• The lock transaction sets lock_timeout = '5s' and statement_timeout = '30s' before acquiring the owner and workspace advisory locks.

Previous Review Summaries (4 snapshots, latest commit 4a7ef18)

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

Previous review (commit 4a7ef18)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change bounds advisory-lock acquisition with the same five-second lock_timeout and thirty-second statement_timeout used by the other integration transactions, and replaces the weakened route assertions with real-Postgres integration-row checks; both earlier findings are resolved and no new defects were found in the changed lines.

Assumptions: [INFERENCE] The sandbox is read-only, so the new database-backed route tests were verified by inspection only, not executed; their passing status is taken from the PR description. [INFERENCE] Organization and enterprise-install paths remain unexercised, as the PR states.

Files Reviewed (4 files)

• apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts
• apps/web/src/app/api/integrations/linear/callback/route.test.ts
• apps/web/src/app/api/integrations/slack/callback/route.test.ts
• docs/security-architecture-overview.md

Carried-forward findings re-verified during this pass

• The advisory-lock transaction now sets lock_timeout and statement_timeout before the owner and workspace lock acquisitions, so the earlier missing-bound warning is resolved.
• Both callback route tests now execute the real upsert service and assert the persisted platform_integrations row (including platform, installation id, account login and metadata) instead of forwarding a mocked call, so the earlier test-coverage suggestion is resolved. Cascade deletes on platform_integrations clean the row up with the test user in finally, and the per-worker database keeps the shared fixture user id isolated.

Previous review (commit 88d8746)

Status: 2 Issues Found | Recommendation: Address before merge

Executive Summary

The advisory locks close the install/rollback race the previous review flagged, but the new lock helper omits the lock and statement timeouts every other advisory-lock transaction in this area sets, so waiting installs can pin the primary connection pool.

Assumptions: [INFERENCE] node_modules is unavailable in this sandbox, so the hand-rolled Slack code exchange could not be diffed against @chat-adapter/slack; its equivalence to the adapter's installation shape — especially enterprise-install keying on enterprise.id, which the PR says was not exercised — is unverified. [INFERENCE] The manually stored Linear expiresAt/refreshToken shape is likewise unverified from package source, though the PR reports exercising it through the real adapter.

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	1

Issue Details (click to expand)

WARNING

File	Line	Issue
apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts	13	Advisory-lock transaction has no lock_timeout/statement_timeout; it spans Redis and outbound Linear calls, so blocked installs hold primary pool connections until the holder finishes

SUGGESTION

File	Line	Issue
apps/web/src/app/api/integrations/linear/callback/route.test.ts	299	Test no longer asserts the upsertLinearInstallation row write; the Slack route test has the same gap

Files Reviewed (14 files)

• apps/web/src/app/api/integrations/linear/callback/route.test.ts - 1 suggestion
• apps/web/src/app/api/integrations/oauth-install-conflict.test.ts
• apps/web/src/app/api/integrations/slack/callback/route.test.ts - same test gap
• apps/web/src/lib/integrations/linear-service.test.ts
• apps/web/src/lib/integrations/linear-service.ts
• apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts
• apps/web/src/lib/integrations/oauth/platforms/slack-callback.test.ts
• apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts
• apps/web/src/lib/integrations/oauth/workspace-installation-lock.ts - 1 warning
• apps/web/src/lib/integrations/platforms/slack/credential-store.test.ts
• apps/web/src/lib/integrations/platforms/slack/credential-store.ts
• apps/web/src/lib/integrations/slack-service.test.ts
• apps/web/src/lib/integrations/slack-service.ts
• docs/security-architecture-overview.md

Previous review (commit 3560b6c)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The follow-up commit resolves the Linear workspace via viewer.organization, matching the Chat SDK adapter's lookup, and the updated test mock mirrors that shape; I found no correctness, security, or memory-leak issue in the changed production or test lines.

Files Reviewed (7 files)

• apps/web/src/app/api/integrations/linear/callback/route.test.ts
• apps/web/src/app/api/integrations/oauth-install-conflict.test.ts
• apps/web/src/app/api/integrations/slack/callback/route.test.ts
• apps/web/src/lib/integrations/linear-service.ts
• apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts
• apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts
• apps/web/src/lib/integrations/slack-service.ts

Previous review (commit f3ad69a)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The install callbacks now resolve the target workspace from the OAuth code and reject a workspace held by another Kilo owner before any Chat SDK installation state is written; the changed production and test code contains no correctness, security, or memory-leak issues I could substantiate.

Files Reviewed (7 files)

• apps/web/src/app/api/integrations/linear/callback/route.test.ts
• apps/web/src/app/api/integrations/oauth-install-conflict.test.ts
• apps/web/src/app/api/integrations/slack/callback/route.test.ts
• apps/web/src/lib/integrations/linear-service.ts
• apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts
• apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts
• apps/web/src/lib/integrations/slack-service.ts

---

_{Reviewed by deepseek-flash-latest · Input: 132.2K · Output: 24K · Cached: 2.9M}

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

[iscekic]

Changes required before merge: the new Slack rollback can remove another callback’s successful installation.

At apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts:163-182, the ownership preflight, shared adapter write, database upsert, and rollback do not form an atomic workspace operation. Two different owners can both pass the preflight. One callback can snapshot no prior installation, another can successfully commit its installation and owner row, and the losing callback can then delete the winner’s shared installation when its upsert detects the conflict. This is a source-derived interleaving, not an executed concurrent callback reproduction.

Serialize the entire workspace ownership transition or use an equivalent generation/ownership protocol so rollback cannot overwrite or delete another callback’s installation. Cover concurrent different-owner callbacks and verify the winner remains usable. Also provide separate running-app callback evidence; the recorded route/database suites use provider and installation-store doubles.

[iscekic]

bot: Callback persistence now uses provider-owner and provider-workspace transaction advisory locks, with ownership reads and integration writes on that transaction. Installation operations and restoration finish under the locks. Controlled callback regressions pass, and a separate running-app HTTP exercise uses real PostgreSQL and Redis with provider-only network fixtures. Stored-credential usability checks pass for both providers. The PR body records the exact proof head and verification boundaries.

[iscekic]

bot: Callback persistence now runs as one serialized workspace transition: the ownership read, the adapter installation write, the integration upsert, and its restoration all execute inside provider-owner then provider-workspace advisory locks with bounded acquisition. A second different-owner callback for the same workspace therefore reaches its ownership check only after the first transition has committed, and receives the existing conflict response without touching shared installation state.

Running-app evidence at 4a7ef184ed: the Next dev server with real PostgreSQL application rows and real Redis installation storage, the real Slack WebClient and Linear SDK, and only provider responses served by a local recording stub. Two simultaneous callbacks for one workspace were observed queued on the same advisory lock; one reported success and the other the conflict response, with a single active integration row for the workspace. A sequential different-owner repeat behaved the same way. With the key held past the lock timeout, the callback reported the installation-failure response after about five seconds and wrote no integration row or installation; a fresh attempt then installed normally with the expected owner and stored credential. Stored-credential usability passed for both providers: the Slack connection check returned success, and the Linear installation token resolved its viewer and workspace.

Real provider consent and enterprise-wide installations remain unproven; the pull request body records the proof head and these limits.

[iscekic]

Changes requested: merge main into the branch. #8055 merged into main and changed apps/web/src/app/api/integrations/linear/callback/route.test.ts, which now conflicts. Keep #8055's provider-bound OAuth state checks together with this PR's locked install flow. Re-run the Slack and Linear callback suites on the merged tree.

[iscekic]

bot: Merged main into the branch and re-ran the callback suites on the merged tree.

• Merge commit c45389ae2 resolves the apps/web/src/app/api/integrations/linear/callback/route.test.ts conflict by keeping the provider-bound OAuth state coverage from fix(integrations): bind signed OAuth state to its integration flow #8055 alongside this branch's locked install flow: provider claim mismatch, missing provider claim, wrong purpose, and already-consumed attempt are all still asserted, and both callbacks still compare the verified provider with the platform.
• On the merged tree at b69d978fa2, the Linear and Slack callback route suites pass together (2 suites, 37 tests), and the focused set — install-conflict, Linear service, Slack service, Slack credential store, Slack callback — passes (5 suites, 62 tests). Both route success paths execute the real upsert service and assert the persisted integration row against PostgreSQL.
• The running-app evidence in the pull request body was recorded at the same head: Next dev server with real PostgreSQL rows and real Redis installation storage, real provider SDKs, and only provider HTTP responses served by a local fixture.