WARNING: error_description is passed directly through to errorRedirect here, but §6.2 Notes say "don't leak GitHub's raw error string into the URL — map known errors to friendly codes." These two instructions are contradictory within the same pseudocode block.

Line 241 uses error_description ?? error as the redirect value, but the note below says to map to friendly codes. The implementation should only use error_description for internal logging and map to a controlled enum (exchange_failed, access_denied, etc.) before putting anything in the redirect URL.

WARNING: prompt=select_account is not a supported parameter for GitHub's OAuth authorize endpoint. GitHub's OAuth documentation does not recognise prompt; this parameter will be silently ignored. If multi-account selection is desired, this is not achievable via a query parameter on GitHub's standard OAuth flow — it would require the user to be logged out of GitHub first or to use a separate account-switcher flow. Remove this line or note that it has no effect.

WARNING: The webhook revocation update uses WHERE github_user_id = $sender_id, but a single GitHub user ID can map to multiple rows if we ever support both standard and lite app types per user. This is correct in the current schema (the unique index is on (kilo_user_id, github_app_type)), but the WHERE clause is incomplete — it should also filter on the relevant github_app_type values, or explicitly accept that it updates all rows for that GitHub user (which is likely the correct intent). Also, note that the payload field to use is sender.node_id or sender.id (numeric); make sure the stored github_user_id format (numeric string from String(ghUser.id) in §6.2) matches the sender.id payload field format to avoid silent mismatches.

WARNING: The set: { /* all token + identity columns; reset revoked_at */ } comment leaves a critical implementation detail unspecified. The onConflictDoUpdate set clause must explicitly include updated_at: new Date() — updated_at has .defaultNow() which only applies on INSERT, not on UPDATE. Drizzle does not auto-update it on conflict-update. If this is not spelled out, implementers may miss it, leaving updated_at stale after every reconnect.

SUGGESTION: The plan conflates two distinct {ok: false} states under a single reason: 'no_user_token': "row does not exist" and "row exists but is revoked". The UI copy in §8.1 maps these identically, but they have different UX implications — a user who was never connected should see "Connect", while a revoked user should see "Reconnect". Consider splitting into reason: 'not_connected' vs reason: 'revoked', especially since the reason: 'revoked' variant is already used separately at line 339/407.

SUGGESTION: The optimistic-concurrency approach using WHERE access_token_expires_at = $oldExpiry is correct in principle, but relies on the DB driver returning affected-row counts accurately through Drizzle + Hyperdrive. Confirm that Drizzle's update().returning() or affected-rows result works as expected through the Hyperdrive connection pool, and note this in the implementation ticket. The plan says "if 0 rows updated, re-read the row" — make sure the re-read path also checks the new token is not itself expired (another race could have produced a token that is already near-expiry).

WARNING: apps.deleteAuthorization requires the access token (not just the client_id) to identify which authorization to delete — specifically it uses Basic Auth with client_id:client_secret and the token in the request body. If the token has already been revoked (e.g., GitHub webhook arrived first and set revoked_at), the access token may be invalid. The plan should specify: (a) decrypt and pass the stored access token to deleteAuthorization; (b) handle 404/422 gracefully since the grant may already be gone; (c) ensure the decryption key is available in the apps/web context where softDeleteUser runs.

SUGGESTION: The plan says to enable "Expire user authorization tokens" but notes it "needs verification on the existing apps". If this setting is currently disabled on the live Apps, enabling it will immediately invalidate all existing user tokens that were issued without expiry (though since the feature is not yet built, there are no such tokens yet). However, this is worth calling out explicitly as a verification step before Phase 0, so it does not become a surprise during the App config change.

WARNING: Fragile repository_mismatch vs invalid_upstream_request classification

The condition !url.pathname.includes('.git/') returns repository_mismatch for any URL whose path contains .git/ but doesn't start with the repo's project path — e.g. https://gitlab.com/attacker/evil.git/files/something. The intent is to distinguish "right origin, wrong repo" from "right origin, not a repo URL at all", but a URL targeting a completely different .git-bearing path on the same host will be labelled repository_mismatch rather than invalid_upstream_request. Both outcomes fail closed, but the mismatch label is semantically wrong and can confuse incident investigations. Consider checking more strictly: url.pathname === repositoryPath || url.pathname.startsWith(repositoryPath + '/') for the mismatch case.

WARNING: Redundant findGitLabIntegration DB lookup on non-code-review paths

resolveGitLabRuntimeToken already calls findGitLabIntegration internally for non-code-review sessions (see gitlab-runtime-token-resolver.ts:135). Immediately after, issueGitLabSessionCapability issues another findGitLabIntegration call for the same integration ID. The second call is needed to get integrationType/accountId/accountLogin fields that are not part of GetGitLabTokenSuccess, but on the common (non-code-review) path this doubles the DB queries per issuance. Consider enriching GetGitLabTokenSuccess with the identity fields so the second lookup is only needed for the code-review path where findAuthorizedGitLabIntegrations was used instead.

WARNING: Incorrect error reason when integration identity is missing

When getGitLabSessionIdentity returns null (both accountId and accountLogin are null on the integration row), this returns { success: false, reason: 'no_token' }. But no_token semantically means "no credential/access-token found", not "integration row is missing required identity fields". This maps to the same client-facing error message as a legitimately absent token, making the two failure modes indistinguishable in logs and user-facing flows. A dedicated reason like 'no_integration_identity' (or reusing 'no_integration_found') would be more accurate.