From 922e46e8fc9eefdf7d9ecd392ba8de766022e9d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:41:17 +0000 Subject: [PATCH 1/6] fix(code-reviews): scope manual GitLab merge request sources A manual Code Reviewer job authorizes the pasted project against the connection's cached repository scope before it resolves the connection's credential: the project has to be one of that connection's repositories, and the pasted instance has to be the connected one. The review row records the authorized repository identity (canonical full path and provider id) and the integration it belongs to. Classified provider refusals map to tRPC errors through one shared helper used by the provider-review router, the branch listing and the manual job, and the manual job's provider read failures answer classified client errors. --- .../cloud-agent/provider-branch-listing.ts | 48 +---- .../manual-code-review-jobs.test.ts | 179 ++++++++++++++++++ .../code-reviews/manual-code-review-jobs.ts | 70 +++---- .../src/lib/provider-review/error-mapping.ts | 33 ++++ .../gitlab-authorization.test.ts | 41 ++++ .../provider-review/gitlab-authorization.ts | 26 ++- .../web/src/routers/provider-review-router.ts | 29 +-- 7 files changed, 314 insertions(+), 112 deletions(-) create mode 100644 apps/web/src/lib/provider-review/error-mapping.ts diff --git a/apps/web/src/lib/cloud-agent/provider-branch-listing.ts b/apps/web/src/lib/cloud-agent/provider-branch-listing.ts index 5a52e0494a5..e38afaedfed 100644 --- a/apps/web/src/lib/cloud-agent/provider-branch-listing.ts +++ b/apps/web/src/lib/cloud-agent/provider-branch-listing.ts @@ -35,6 +35,7 @@ import { import { authorizeProject, GitLabReviewError, + gitlabReviewErrorToTrpcError, type GitLabReviewOwner, } from '@/lib/provider-review/gitlab-authorization'; import { @@ -43,6 +44,7 @@ import { BitbucketReviewError, type BitbucketRepositoryAccess, } from '@/lib/provider-review/bitbucket-authorization'; +import { providerReviewErrorToTrpcError } from '@/lib/provider-review/error-mapping'; import { fetchPage, repositoryPathGuard, @@ -108,28 +110,6 @@ async function requireActiveIntegration(owner: Owner, platform: string, label: s return integration; } -/** - * Map a classified Bitbucket refusal onto the router error states. The - * message is the authorization layer's fixed copy — it never embeds a token - * or a workspace identity. - */ -function bitbucketErrorToTrpcError(error: BitbucketReviewError): TRPCError { - switch (error.kind) { - case 'not_found': - return new TRPCError({ code: 'NOT_FOUND', message: error.message }); - case 'forbidden': - return new TRPCError({ code: 'FORBIDDEN', message: error.message }); - case 'reconnect': - return new TRPCError({ code: 'PRECONDITION_FAILED', message: error.message }); - case 'bad_request': - return new TRPCError({ code: 'BAD_REQUEST', message: error.message }); - default: - // stale_head cannot come from a read; retryable surfaces as a - // retryable gateway failure, never as an empty branch list. - return new TRPCError({ code: 'BAD_GATEWAY', message: error.message }); - } -} - type GitHubBranchRef = { name: string; isDefault: boolean }; function toListing(branches: GitHubBranchRef[]): ProviderBranchListing { @@ -265,24 +245,6 @@ async function listGitHubBranches( } } -/** A refusal meaning "this GitLab review call cannot proceed" — router worded. */ -function gitlabErrorToTrpcError(error: GitLabReviewError): TRPCError { - switch (error.kind) { - case 'not_found': - return new TRPCError({ code: 'NOT_FOUND', message: error.message }); - case 'forbidden': - return new TRPCError({ code: 'FORBIDDEN', message: error.message }); - case 'reconnect': - return new TRPCError({ code: 'PRECONDITION_FAILED', message: error.message }); - case 'bad_request': - return new TRPCError({ code: 'BAD_REQUEST', message: error.message }); - default: - // stale_head cannot come from a read; retryable surfaces as a - // retryable gateway failure, never as an empty branch list. - return new TRPCError({ code: 'BAD_GATEWAY', message: error.message }); - } -} - /** * Map a GitLab branches HTTP refusal onto the router error states. Only the * status is carried over; the adapter's message stays out of the copy. @@ -357,7 +319,7 @@ async function listGitLabBranches( ); return toListing(branches.map(branch => ({ name: branch.name, isDefault: branch.default }))); } catch (error) { - if (error instanceof GitLabReviewError) throw gitlabErrorToTrpcError(error); + if (error instanceof GitLabReviewError) throw gitlabReviewErrorToTrpcError(error, reviewOwner); if (error instanceof GitLabBranchesFetchError) throw gitlabBranchesErrorToTrpcError(error, reviewOwner); throw error; @@ -425,7 +387,7 @@ async function listBitbucketBranches( repoSlug ); } catch (error) { - if (error instanceof BitbucketReviewError) throw bitbucketErrorToTrpcError(error); + if (error instanceof BitbucketReviewError) throw providerReviewErrorToTrpcError(error); throw error; } @@ -468,7 +430,7 @@ async function listBitbucketBranches( if (page === MAX_BRANCH_PAGES - 1) truncated = true; } } catch (error) { - if (error instanceof BitbucketReviewError) throw bitbucketErrorToTrpcError(error); + if (error instanceof BitbucketReviewError) throw providerReviewErrorToTrpcError(error); throw error; } diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts index e9ee04e9914..59aaf5be1cf 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts @@ -9,6 +9,27 @@ const mockGetAgentConfigForOwner = jest.fn(); const mockAssertCouncilCreationAllowed = jest.fn(); const mockCreateCodeReview = jest.fn(); const mockTryDispatchPendingReviews = jest.fn(); +const mockGetAllIntegrationsForOwner = jest.fn(); +const mockGetIntegrationForOwner = jest.fn(); +const mockGetValidGitLabToken = jest.fn(); +const mockFetchGitLabMergeRequest = jest.fn(); + +jest.mock('@/lib/integrations/db/platform-integrations', () => ({ + getAllIntegrationsForOwner: (...args: unknown[]) => mockGetAllIntegrationsForOwner(...args), + getIntegrationForOwner: (...args: unknown[]) => mockGetIntegrationForOwner(...args), +})); + +jest.mock('@/lib/integrations/gitlab-service', () => ({ + getValidGitLabToken: (...args: unknown[]) => mockGetValidGitLabToken(...args), +})); + +// The adapter's GitLab transport binds the resolved address and bypasses +// global fetch for self-managed hosts, so the connected path stubs the one +// provider read it makes and asserts the call it was given. +jest.mock('@/lib/integrations/platforms/gitlab/adapter', () => ({ + ...jest.requireActual('@/lib/integrations/platforms/gitlab/adapter'), + fetchGitLabMergeRequest: (...args: unknown[]) => mockFetchGitLabMergeRequest(...args), +})); jest.mock('@kilocode/web-shared/lib/config.server', () => ({ isLocalCodeReviewDevelopmentEnabled: () => mockIsLocalCodeReviewDevelopmentEnabled(), @@ -201,6 +222,164 @@ describe('createManualCodeReviewJob provider failures', () => { }); }); +describe('createManualCodeReviewJob connected GitLab source', () => { + const INSTANCE_URL = 'https://gitlab.example.com'; + const PROJECT_PATH = 'group/project'; + const MR_URL = `${INSTANCE_URL}/${PROJECT_PATH}/-/merge_requests/7`; + + const orgOwner = { type: 'org' as const, id: 'org-1', userId: 'bot-1' }; + + function gitlabIntegration() { + return { + id: 'gitlab-integration-1', + platform: 'gitlab', + integration_status: 'active', + owned_by_user_id: OWNER.id, + owned_by_organization_id: null, + repositories: [ + { id: 501, name: 'project', full_name: PROJECT_PATH, private: true }, + { id: 502, name: 'other', full_name: 'group/other', private: false }, + ], + metadata: { gitlab_instance_url: INSTANCE_URL }, + }; + } + + function mergeRequest(overrides: Record = {}) { + return { + iid: 7, + title: 'Harden the thing', + state: 'opened', + draft: false, + source_branch: 'feature', + target_branch: 'main', + sha: 'sha-7', + web_url: MR_URL, + author: { username: 'gl-user' }, + project_id: 501, + target_project_id: 501, + ...overrides, + }; + } + + function gitlabJob(url: string) { + return { platform: 'gitlab' as const, url, modelSlug: 'test-model' }; + } + + beforeEach(() => { + mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(false); + mockGetAllIntegrationsForOwner.mockResolvedValue([gitlabIntegration()]); + mockGetIntegrationForOwner.mockResolvedValue(gitlabIntegration()); + mockGetValidGitLabToken.mockResolvedValue('glpat-token'); + mockFetchGitLabMergeRequest.mockResolvedValue(mergeRequest()); + mockCreateCodeReview.mockResolvedValue('review-gitlab'); + }); + + it('starts a review for a project inside the connection repository scope', async () => { + await expect( + createManualCodeReviewJob({ owner: OWNER, input: gitlabJob(MR_URL) }) + ).resolves.toEqual({ reviewId: 'review-gitlab', outputMode: 'provider' }); + + expect(mockGetValidGitLabToken).toHaveBeenCalledWith(gitlabIntegration(), { + userId: OWNER.userId, + }); + expect(mockFetchGitLabMergeRequest).toHaveBeenCalledWith({ + accessToken: 'glpat-token', + projectId: PROJECT_PATH, + mrIid: 7, + instanceUrl: INSTANCE_URL, + }); + expect(mockCreateCodeReview).toHaveBeenCalledWith( + expect.objectContaining({ + platform: 'gitlab', + platformIntegrationId: 'gitlab-integration-1', + repoFullName: PROJECT_PATH, + platformProjectId: 501, + prNumber: 7, + prUrl: MR_URL, + prAuthor: 'gl-user', + baseRef: 'main', + headRef: 'refs/merge-requests/7/head', + headSha: 'sha-7', + }) + ); + expect(mockTryDispatchPendingReviews).toHaveBeenCalledWith(OWNER); + }); + + it('refuses a project outside the connection repository scope before any credential or provider call', async () => { + const error = await captureError( + gitlabJob(`${INSTANCE_URL}/other-org/private-project/-/merge_requests/1`) + ); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('NOT_FOUND'); + // The refusal happens before the connection's credential is minted or a + // provider request is made: no token, no read, no review row, no dispatch. + expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); + expect(mockFetchGitLabMergeRequest).not.toHaveBeenCalled(); + expect(mockCreateCodeReview).not.toHaveBeenCalled(); + expect(mockTryDispatchPendingReviews).not.toHaveBeenCalled(); + }); + + it('matches the cached project path case-insensitively and reads only the canonical path', async () => { + const shoutyUrl = `${INSTANCE_URL}/GROUP/Project/-/merge_requests/7`; + + await expect( + createManualCodeReviewJob({ owner: OWNER, input: gitlabJob(shoutyUrl) }) + ).resolves.toEqual({ reviewId: 'review-gitlab', outputMode: 'provider' }); + + expect(mockFetchGitLabMergeRequest).toHaveBeenCalledWith( + expect.objectContaining({ projectId: PROJECT_PATH }) + ); + expect(mockCreateCodeReview).toHaveBeenCalledWith( + expect.objectContaining({ repoFullName: PROJECT_PATH }) + ); + }); + + it('refuses a merge request URL on an instance other than the connected one', async () => { + const error = await captureError( + gitlabJob(`https://gitlab.com/${PROJECT_PATH}/-/merge_requests/7`) + ); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('NOT_FOUND'); + expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); + expect(mockFetchGitLabMergeRequest).not.toHaveBeenCalled(); + }); + + it('keeps the connect-GitLab precondition when the owner has no active GitLab connection', async () => { + mockGetAllIntegrationsForOwner.mockResolvedValue([]); + + const error = await captureError(gitlabJob(MR_URL)); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('PRECONDITION_FAILED'); + expect((error as TRPCError).message).toContain('Connect GitLab'); + expect(mockGetIntegrationForOwner).not.toHaveBeenCalled(); + expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); + }); + + it('passes the organization credential actor for an organization-owned connection', async () => { + mockGetAllIntegrationsForOwner.mockResolvedValue([ + { ...gitlabIntegration(), owned_by_user_id: null, owned_by_organization_id: 'org-1' }, + ]); + mockGetIntegrationForOwner.mockResolvedValue({ + ...gitlabIntegration(), + owned_by_user_id: null, + owned_by_organization_id: 'org-1', + }); + + await expect( + createManualCodeReviewJob({ owner: orgOwner, input: gitlabJob(MR_URL) }) + ).resolves.toEqual({ reviewId: 'review-gitlab', outputMode: 'provider' }); + + expect(mockGetIntegrationForOwner).toHaveBeenCalledWith({ type: 'org', id: 'org-1' }, 'gitlab'); + expect(mockGetValidGitLabToken).toHaveBeenCalledWith(expect.anything(), { + userId: 'bot-1', + organizationId: 'org-1', + }); + }); +}); + describe('createManualCodeReviewJob happy path', () => { afterEach(() => { jest.restoreAllMocks(); diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts index 90fd1cae887..2fd388d39ef 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts @@ -2,7 +2,6 @@ import 'server-only'; import { TRPCError } from '@trpc/server'; import * as z from 'zod'; -import type { PlatformIntegration } from '@kilocode/db/schema'; import { CodeReviewCouncilConfigSchema, MAX_RUNTIME_AGENT_MODEL_LENGTH, @@ -27,9 +26,12 @@ import { PLATFORM } from '@/lib/integrations/core/constants'; import { getAllIntegrationsForOwner } from '@/lib/integrations/db/platform-integrations'; import { generateGitHubInstallationToken } from '@/lib/integrations/platforms/github/adapter'; import { getGitHubPullRequestCheckoutRef } from '@/lib/integrations/platforms/github/webhook-handlers/pull-request-checkout-ref'; -import { getValidGitLabToken } from '@/lib/integrations/gitlab-service'; import { fetchGitLabMergeRequest } from '@/lib/integrations/platforms/gitlab/adapter'; -import { normalizeGitLabInstanceUrl } from '@/lib/integrations/platforms/gitlab/instance-url'; +import { + authorizeProject, + gitlabReviewErrorToTrpcError, + type GitLabReviewOwner, +} from '@/lib/provider-review/gitlab-authorization'; import type { CodeReviewPlatform, Owner } from './core'; import { createCodeReview, findActiveProviderPublishingReview } from './db/code-reviews'; import { tryDispatchPendingReviews } from './dispatch/dispatch-pending-reviews'; @@ -524,38 +526,37 @@ async function resolveConnectedGitLabSource( }); } - for (const integration of integrations) { - const instanceUrl = getGitLabIntegrationInstanceUrl(integration); - if (parsed.origin !== instanceUrl) continue; + const reviewOwner: GitLabReviewOwner = + owner.type === 'org' + ? { type: 'organization', organizationId: owner.id, userId: owner.userId } + : { type: 'user', userId: owner.userId }; - const accessToken = await getValidGitLabToken(integration, { - userId: owner.userId, - ...(owner.type === 'org' ? { organizationId: owner.id } : {}), - }); + try { + // The same boundary GitLab branch listing and the provider-review reads + // enforce: the pasted project has to be one of the connection's cached + // repositories, and the pasted instance has to be the connected one. A + // refusal here happens before the connection's credential is minted, so a + // path outside that scope never reaches the token broker, the provider, a + // review row, or session preparation. + const access = await authorizeProject(reviewOwner, parsed.projectPath, parsed.origin); const rawMergeRequest = await fetchGitLabMergeRequest({ - accessToken, - projectId: parsed.projectPath, + accessToken: access.accessToken, + projectId: access.projectPath, mrIid: parsed.mrIid, - instanceUrl, + instanceUrl: access.instanceUrl, }); const mergeRequest = GitLabMergeRequestApiSchema.parse(rawMergeRequest); validateOpenGitLabMergeRequest(mergeRequest); return buildGitLabSource({ mergeRequest, - projectPath: parsed.projectPath, - integrationId: integration.id, - platformProjectId: - mergeRequest.target_project_id ?? - mergeRequest.project_id ?? - getGitLabRepositoryIdFromIntegration(integration, parsed.projectPath), + projectPath: access.projectPath, + integrationId: access.integrationId, + platformProjectId: access.repositoryId, }); + } catch (error) { + throw gitlabReviewErrorToTrpcError(error, reviewOwner); } - - throw new TRPCError({ - code: 'BAD_REQUEST', - message: 'The merge request URL does not match a connected GitLab instance.', - }); } function buildGitHubSource( @@ -754,27 +755,6 @@ function validateOpenGitLabMergeRequest(mergeRequest: GitLabMergeRequestApi): vo } } -function getGitLabIntegrationInstanceUrl(integration: PlatformIntegration): string { - const metadata = integration.metadata; - const instanceUrl = - typeof metadata === 'object' && - metadata !== null && - !Array.isArray(metadata) && - 'gitlab_instance_url' in metadata - ? metadata.gitlab_instance_url - : undefined; - return normalizeGitLabInstanceUrl(typeof instanceUrl === 'string' ? instanceUrl : undefined); -} - -function getGitLabRepositoryIdFromIntegration( - integration: PlatformIntegration, - projectPath: string -): number | undefined { - const repositories = integration.repositories; - const repository = repositories?.find(candidate => candidate.full_name === projectPath); - return typeof repository?.id === 'number' ? repository.id : undefined; -} - class ProviderFetchError extends Error { readonly status: number; diff --git a/apps/web/src/lib/provider-review/error-mapping.ts b/apps/web/src/lib/provider-review/error-mapping.ts new file mode 100644 index 00000000000..fe554106c11 --- /dev/null +++ b/apps/web/src/lib/provider-review/error-mapping.ts @@ -0,0 +1,33 @@ +/** + * One classified provider refusal → the router's error state. + * + * Every provider surface (the mobile/web provider-review router, the Cloud + * Agent branch listing, and the manual Code Reviewer job) answers the same + * refusal class with the same code, so a client that handles one surface + * handles them all. The provider message is fixed copy that never embeds a + * token, an instance URL, or a workspace identity. + */ +import 'server-only'; + +import { TRPCError } from '@trpc/server'; +import type { ProviderReviewError } from '@kilocode/app-shared/provider-review'; + +export function providerReviewErrorToTrpcError(error: ProviderReviewError): TRPCError { + switch (error.kind) { + case 'not_found': + return new TRPCError({ code: 'NOT_FOUND', message: error.message, cause: error }); + case 'forbidden': + return new TRPCError({ code: 'FORBIDDEN', message: error.message, cause: error }); + case 'reconnect': + // The provider connection must be re-established; the caller's Kilo + // access is not the problem. This is the mobile reconnect state. + return new TRPCError({ code: 'PRECONDITION_FAILED', message: error.message, cause: error }); + case 'stale_head': + return new TRPCError({ code: 'CONFLICT', message: error.message, cause: error }); + case 'bad_request': + return new TRPCError({ code: 'BAD_REQUEST', message: error.message, cause: error }); + case 'retryable': + // A provider-side or network failure to retry, never a 500. + return new TRPCError({ code: 'BAD_GATEWAY', message: error.message, cause: error }); + } +} diff --git a/apps/web/src/lib/provider-review/gitlab-authorization.test.ts b/apps/web/src/lib/provider-review/gitlab-authorization.test.ts index 5ba11cb14a0..81f8629bdac 100644 --- a/apps/web/src/lib/provider-review/gitlab-authorization.test.ts +++ b/apps/web/src/lib/provider-review/gitlab-authorization.test.ts @@ -10,6 +10,7 @@ import { classifyGitLabStatus, GitLabApiStatusError, GitLabReviewError, + gitlabReviewErrorToTrpcError, type GitLabReviewOwner, } from './gitlab-authorization'; @@ -181,6 +182,9 @@ describe('authorizeProject — repository matching', () => { const access = await authorizeProject(SELF_MANAGED_USER, 'GROUP/Sub/REPO'); expect(access.projectPath).toBe('group/sub/repo'); + // The resolved identity travels with the credentials so a caller can + // record which connection and repository an action belongs to. + expect(access).toMatchObject({ integrationId: 'intg_1', repositoryId: 7 }); }); it('refuses a prefix or suffix of a nested path (exact full path only)', async () => { @@ -347,3 +351,40 @@ describe('classifyGitLabErrorForOwner', () => { ); }); }); + +describe('gitlabReviewErrorToTrpcError', () => { + it.each([ + ['not_found', 'NOT_FOUND'], + ['forbidden', 'FORBIDDEN'], + ['reconnect', 'PRECONDITION_FAILED'], + ['stale_head', 'CONFLICT'], + ['bad_request', 'BAD_REQUEST'], + ['retryable', 'BAD_GATEWAY'], + ] as const)('maps the %s kind to %s', (kind, code) => { + const mapped = gitlabReviewErrorToTrpcError( + new GitLabReviewError(kind, 'fixed copy'), + SELF_MANAGED_USER + ); + + expect(mapped.code).toBe(code); + expect(mapped.message).toBe('fixed copy'); + }); + + it('classifies before mapping, so an organization 403 is a reconnect', () => { + const refusal = new GitLabReviewError('forbidden', 'denied', 403); + + expect(gitlabReviewErrorToTrpcError(refusal, SELF_MANAGED_ORG).code).toBe( + 'PRECONDITION_FAILED' + ); + expect(gitlabReviewErrorToTrpcError(refusal, SELF_MANAGED_USER).code).toBe('FORBIDDEN'); + }); + + it('passes an intentional tRPC refusal through unchanged', () => { + const refusal = new TRPCError({ + code: 'BAD_REQUEST', + message: 'Draft merge requests are skipped.', + }); + + expect(gitlabReviewErrorToTrpcError(refusal, SELF_MANAGED_USER)).toBe(refusal); + }); +}); diff --git a/apps/web/src/lib/provider-review/gitlab-authorization.ts b/apps/web/src/lib/provider-review/gitlab-authorization.ts index 24a9a1c5491..c2faf8219df 100644 --- a/apps/web/src/lib/provider-review/gitlab-authorization.ts +++ b/apps/web/src/lib/provider-review/gitlab-authorization.ts @@ -29,6 +29,7 @@ import { normalizeGitLabInstanceUrl, } from '@/lib/integrations/platforms/gitlab/instance-url'; import { logExceptInTest } from '@kilocode/web-shared/lib/utils.server'; +import { providerReviewErrorToTrpcError } from './error-mapping'; /** * The account that owns the GitLab integration. `userId` is the acting user @@ -212,9 +213,25 @@ export type GitLabProjectAccess = { instanceUrl: string; /** The integration's repository full path, matched case-insensitively. */ projectPath: string; + /** The integration the credential belongs to. */ + integrationId: string; + /** The matched repository's provider id from that integration's cache. */ + repositoryId: number; owner: GitLabReviewOwner; }; +/** + * Map one GitLab failure onto the router's error states. Classification is + * owner-aware first (a connection-level 403 is a reconnect, not the reader's + * permission), then every surface answers the kind with the shared code. A + * TRPCError is already an intentional refusal (invalid URL, closed or draft + * merge request) and passes through unchanged. + */ +export function gitlabReviewErrorToTrpcError(error: unknown, owner: GitLabReviewOwner): TRPCError { + if (error instanceof TRPCError) return error; + return providerReviewErrorToTrpcError(classifyGitLabErrorForOwner(error, owner)); +} + function ownerToDbOwner(owner: GitLabReviewOwner): Owner { return owner.type === 'user' ? { type: 'user', id: owner.userId } @@ -334,7 +351,14 @@ export async function authorizeProject( try { const accessToken = await getValidGitLabToken(integration, actorFor(owner)); - return { accessToken, instanceUrl, projectPath: match.full_name, owner }; + return { + accessToken, + instanceUrl, + projectPath: match.full_name, + integrationId: integration.id, + repositoryId: match.id, + owner, + }; } catch (error) { throw classifyGitLabErrorForOwner(error, owner); } diff --git a/apps/web/src/routers/provider-review-router.ts b/apps/web/src/routers/provider-review-router.ts index d882acd7162..85f38dae423 100644 --- a/apps/web/src/routers/provider-review-router.ts +++ b/apps/web/src/routers/provider-review-router.ts @@ -61,6 +61,7 @@ import { unresolveThread as gitlabUnresolveThread, } from '@/lib/provider-review/gitlab-write'; import * as bitbucketRead from '@/lib/provider-review/bitbucket-read'; +import { providerReviewErrorToTrpcError } from '@/lib/provider-review/error-mapping'; import { BITBUCKET_AUTO_MERGE_UNSUPPORTED_REASON, BITBUCKET_PR_REVIEW_CAPABILITIES, @@ -367,32 +368,14 @@ function providerRef(input: { } /** - * Map one classified provider failure onto the mobile error states. - * `retryable` becomes BAD_GATEWAY (the ledger's ambiguous marker), a moved - * head becomes CONFLICT carrying the exact stale-head reason, the rest are - * deterministic rejections. The provider message is fixed copy that never - * embeds a token or an instance URL. + * Map one provider failure onto the mobile error states: every classified + * refusal answers the shared code (retryable → BAD_GATEWAY, a moved head → + * CONFLICT carrying the stale-head reason, the rest deterministic). The + * provider message is fixed copy that never embeds a token or an instance URL. */ function toProviderTrpcError(error: unknown): TRPCError { if (error instanceof TRPCError) return error; - if (error instanceof ProviderReviewError) { - switch (error.kind) { - case 'not_found': - return new TRPCError({ code: 'NOT_FOUND', message: error.message }); - case 'forbidden': - return new TRPCError({ code: 'FORBIDDEN', message: error.message }); - case 'reconnect': - // The provider connection must be re-established; the caller's Kilo - // access is not the problem. This is the mobile reconnect state. - return new TRPCError({ code: 'PRECONDITION_FAILED', message: error.message }); - case 'stale_head': - return new TRPCError({ code: 'CONFLICT', message: error.message }); - case 'bad_request': - return new TRPCError({ code: 'BAD_REQUEST', message: error.message }); - case 'retryable': - return new TRPCError({ code: 'BAD_GATEWAY', message: error.message }); - } - } + if (error instanceof ProviderReviewError) return providerReviewErrorToTrpcError(error); return new TRPCError({ code: 'INTERNAL_SERVER_ERROR', message: 'The review request failed. Please try again.', From b96b1a999253335aebefcfa95d7a9c619295886c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:33:25 +0000 Subject: [PATCH 2/6] refactor(provider-review): share the GitLab review-owner mapping The branch listing and the manual Code Reviewer job each built the same GitLab review owner by hand. Expose one gitlabReviewOwnerFor helper in the review layer and use it in both, so the organization-owner-plus-acting-user pair has a single definition. --- .../src/lib/cloud-agent/provider-branch-listing.ts | 6 ++---- .../src/lib/code-reviews/manual-code-review-jobs.ts | 7 ++----- .../src/lib/provider-review/gitlab-authorization.ts | 12 ++++++++++++ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/apps/web/src/lib/cloud-agent/provider-branch-listing.ts b/apps/web/src/lib/cloud-agent/provider-branch-listing.ts index e38afaedfed..dced3729939 100644 --- a/apps/web/src/lib/cloud-agent/provider-branch-listing.ts +++ b/apps/web/src/lib/cloud-agent/provider-branch-listing.ts @@ -36,6 +36,7 @@ import { authorizeProject, GitLabReviewError, gitlabReviewErrorToTrpcError, + gitlabReviewOwnerFor, type GitLabReviewOwner, } from '@/lib/provider-review/gitlab-authorization'; import { @@ -306,10 +307,7 @@ async function listGitLabBranches( // authorization boundary the Bitbucket path enforces through // authorizeRepository. Without it, any project path the connected token // can reach would be listable. - const reviewOwner: GitLabReviewOwner = - owner.type === 'org' - ? { type: 'organization', organizationId: owner.id, userId: actor.userId } - : { type: 'user', userId: actor.userId }; + const reviewOwner = gitlabReviewOwnerFor(owner, actor.userId); try { const access = await authorizeProject(reviewOwner, repositoryFullName); const branches = await fetchGitLabBranches( diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts index 2fd388d39ef..0c2fc966002 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts @@ -30,7 +30,7 @@ import { fetchGitLabMergeRequest } from '@/lib/integrations/platforms/gitlab/ada import { authorizeProject, gitlabReviewErrorToTrpcError, - type GitLabReviewOwner, + gitlabReviewOwnerFor, } from '@/lib/provider-review/gitlab-authorization'; import type { CodeReviewPlatform, Owner } from './core'; import { createCodeReview, findActiveProviderPublishingReview } from './db/code-reviews'; @@ -526,10 +526,7 @@ async function resolveConnectedGitLabSource( }); } - const reviewOwner: GitLabReviewOwner = - owner.type === 'org' - ? { type: 'organization', organizationId: owner.id, userId: owner.userId } - : { type: 'user', userId: owner.userId }; + const reviewOwner = gitlabReviewOwnerFor(owner, owner.userId); try { // The same boundary GitLab branch listing and the provider-review reads diff --git a/apps/web/src/lib/provider-review/gitlab-authorization.ts b/apps/web/src/lib/provider-review/gitlab-authorization.ts index c2faf8219df..3d3fc80cc5e 100644 --- a/apps/web/src/lib/provider-review/gitlab-authorization.ts +++ b/apps/web/src/lib/provider-review/gitlab-authorization.ts @@ -232,6 +232,18 @@ export function gitlabReviewErrorToTrpcError(error: unknown, owner: GitLabReview return providerReviewErrorToTrpcError(classifyGitLabErrorForOwner(error, owner)); } +/** + * The review-layer owner for a Kilo owner plus the acting user whose + * credential the token broker releases. Organization-owned connections keep + * the organization as the owner and name the acting user separately, so the + * broker can release the organization's credential for that user. + */ +export function gitlabReviewOwnerFor(owner: Owner, actingUserId: string): GitLabReviewOwner { + return owner.type === 'org' + ? { type: 'organization', organizationId: owner.id, userId: actingUserId } + : { type: 'user', userId: actingUserId }; +} + function ownerToDbOwner(owner: GitLabReviewOwner): Owner { return owner.type === 'user' ? { type: 'user', id: owner.userId } From 92e8ea8aee011bca105100dc64501291b54b4381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:49:04 +0000 Subject: [PATCH 3/6] test(code-reviews): cover manual GitLab merge request read refusals Pin the manual job's provider read refusals: a refused merge request read answers NOT_FOUND and a refused credential answers the reconnect state, both without creating a review or dispatching one. --- .../manual-code-review-jobs.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts index 59aaf5be1cf..76ae7571004 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts @@ -358,6 +358,28 @@ describe('createManualCodeReviewJob connected GitLab source', () => { expect(mockGetValidGitLabToken).not.toHaveBeenCalled(); }); + it('maps a refused merge request read to a client error instead of a 500', async () => { + mockFetchGitLabMergeRequest.mockRejectedValue(new Error('GitLab MR fetch failed: 404')); + + const error = await captureError(gitlabJob(MR_URL)); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('NOT_FOUND'); + expect(mockCreateCodeReview).not.toHaveBeenCalled(); + expect(mockTryDispatchPendingReviews).not.toHaveBeenCalled(); + }); + + it('maps a refused credential on the merge request read to the reconnect state', async () => { + mockFetchGitLabMergeRequest.mockRejectedValue(new Error('GitLab MR fetch failed: 401')); + + const error = await captureError(gitlabJob(MR_URL)); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('PRECONDITION_FAILED'); + expect((error as TRPCError).message).toMatch(/reconnect/i); + expect(mockCreateCodeReview).not.toHaveBeenCalled(); + }); + it('passes the organization credential actor for an organization-owned connection', async () => { mockGetAllIntegrationsForOwner.mockResolvedValue([ { ...gitlabIntegration(), owned_by_user_id: null, owned_by_organization_id: 'org-1' }, From 155b6f4cbf2625151119bb8e9b0f28b478575c78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:14:26 +0000 Subject: [PATCH 4/6] fix(cloud-agent-next): budget a preparation attempt for its own steps One preparation attempt runs the sandbox create, the wrapper's first connect and hello, the snapshot restore with its clone fallback, the Kilo runtime start and the session import in sequence. Each step keeps its own bound, so the attempt deadline is derived from their sum instead of a fixed 12 minutes: a shorter deadline fails a slow but valid preparation as preparation_timeout while every step stayed inside its own budget. The step bounds, a permanent create failure and the reconnect window still fail a stuck preparation earlier. --- .../cloud-agent-next/docs/control-plane.md | 4 +- .../src/control-plane/sandbox/routes.ts | 2 +- .../src/control-plane/sandbox/sandbox-do.ts | 2 +- .../src/shared/control-plane-timers.test.ts | 27 +++++++++ .../src/shared/control-plane-timers.ts | 55 ++++++++++++++----- ...control-plane-session-v2-transport.test.ts | 2 +- 6 files changed, 75 insertions(+), 17 deletions(-) diff --git a/services/cloud-agent-next/docs/control-plane.md b/services/cloud-agent-next/docs/control-plane.md index 7d5296f52a8..6d946a51e87 100644 --- a/services/cloud-agent-next/docs/control-plane.md +++ b/services/cloud-agent-next/docs/control-plane.md @@ -309,7 +309,9 @@ so a later report changes nothing. ### Route state (per session) -`preparing` → `ready` → `failed`. Each preparation attempt has a 12-minute deadline. An attempt +`preparing` → `ready` → `failed`. Each preparation attempt has a deadline that outlasts its +own steps in sequence (sandbox create, wrapper first connect and hello, snapshot restore with +the clone fallback, Kilo runtime start, session import). An attempt starts when a route enters `preparing`: the first `prepare`, a `prepare` after `failed`, or a re-prepare after a wrapper restart. A repeated `prepare`, a reconnect or a new allocation inside the attempt keeps its deadline. diff --git a/services/cloud-agent-next/src/control-plane/sandbox/routes.ts b/services/cloud-agent-next/src/control-plane/sandbox/routes.ts index b21988b7a04..14addebae02 100644 --- a/services/cloud-agent-next/src/control-plane/sandbox/routes.ts +++ b/services/cloud-agent-next/src/control-plane/sandbox/routes.ts @@ -500,7 +500,7 @@ export async function failReadyRoutesForScope( } } -/** Fail preparing routes whose 12-minute attempt deadline has passed. */ +/** Fail preparing routes whose preparation attempt deadline has passed. */ export async function failExpiredRoutes(ctx: RouteContext): Promise { const now = ctx.now(); for (const route of await listRoutes(ctx.db)) { diff --git a/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts b/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts index c7d005caccd..7857c747e12 100644 --- a/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts +++ b/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts @@ -5443,7 +5443,7 @@ export class SandboxControlV2 extends DurableObject { private async armAlarm(state: AllocationState): Promise { // The one alarm is the earliest of the allocation timers and the route // preparation deadlines, so a hung wrapper's route cannot outlive its - // 12-minute attempt. + // preparation attempt. const allocationAt = nextAllocationAlarmAt( state.kind === 'connected' && [...this.repositoryCaptures.values()].some( diff --git a/services/cloud-agent-next/src/shared/control-plane-timers.test.ts b/services/cloud-agent-next/src/shared/control-plane-timers.test.ts index 1b895ce5dfe..e4055d0a9ba 100644 --- a/services/cloud-agent-next/src/shared/control-plane-timers.test.ts +++ b/services/cloud-agent-next/src/shared/control-plane-timers.test.ts @@ -35,6 +35,33 @@ describe('control-plane idle timers', () => { }); }); +describe('control-plane preparation attempt budget', () => { + it('outlasts every step of one preparation attempt in sequence', () => { + const { sandbox, wrapper } = CONTROL_PLANE_TIMERS; + const steps = + sandbox.providerCreateMs + + sandbox.wrapperFirstConnectMs + + sandbox.wrapperHelloMs + + wrapper.restoreMs + + wrapper.cloneMs + + wrapper.kiloRuntimeStartMs + + wrapper.kiloSessionMs; + + // A shorter attempt deadline would fail a slow but valid preparation as + // `preparation_timeout` while each step stayed inside its own bound. + expect(sandbox.routePreparationMs).toBeGreaterThanOrEqual(steps); + }); + + it('keeps the attempt budget real when the development divisor shrinks step timers', () => { + const scaled = resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: '10' }); + + expect(scaled.sandbox.routePreparationMs).toBe(CONTROL_PLANE_TIMERS.sandbox.routePreparationMs); + expect(scaled.sandbox.routePreparationMs).toBeGreaterThanOrEqual( + scaled.wrapper.cloneMs + scaled.wrapper.restoreMs + ); + }); +}); + describe('sandbox idle policy for route origins', () => { it('keeps the interactive window for a sandbox without routes', () => { expect(sandboxIdlePolicyForOrigins([])).toBe('interactive'); diff --git a/services/cloud-agent-next/src/shared/control-plane-timers.ts b/services/cloud-agent-next/src/shared/control-plane-timers.ts index 44fe2d18d99..12e8b97f4a4 100644 --- a/services/cloud-agent-next/src/shared/control-plane-timers.ts +++ b/services/cloud-agent-next/src/shared/control-plane-timers.ts @@ -72,23 +72,52 @@ export type ControlPlaneTimers = { }; }; +// External-work bounds for one preparation attempt. They stay real (never +// scaled) because they bound provider and container work. +const PROVIDER_CREATE_MS = 2 * MINUTE_MS; +const PROVIDER_CREATE_RETRY_MS = 10_000; +const WRAPPER_FIRST_CONNECT_MS = 5 * MINUTE_MS; +const WRAPPER_HELLO_MS = 30_000; +const RESTORE_MS = 2 * MINUTE_MS; +const CLONE_MS = 6 * MINUTE_MS; +const KILO_RUNTIME_START_MS = 2 * MINUTE_MS; +const KILO_SESSION_MS = 2 * MINUTE_MS; + +/** + * One preparation attempt runs these steps in order: sandbox create, wrapper + * first connect and hello, snapshot restore (falling back to a clone), Kilo + * runtime start, then the session import. Every step keeps its own bound, so + * the attempt deadline is their sum. A shorter deadline fails a slow but valid + * preparation as `preparation_timeout` while each step stayed inside its own + * budget — the failure names a timeout the plan never allowed to finish. + */ +const ROUTE_PREPARATION_MS = + PROVIDER_CREATE_MS + + WRAPPER_FIRST_CONNECT_MS + + WRAPPER_HELLO_MS + + RESTORE_MS + + CLONE_MS + + KILO_RUNTIME_START_MS + + KILO_SESSION_MS; + // The divisor shortens only internal deadlines whose partner also scales, so // their orderings hold (the 90-s reconnect window stays longer than the 30-s // max backoff). Timers that bound external work stay real: provider create and -// container boot, the 12-min prep attempt that must outlast clone+runtime+ -// session, the pause before a failed create is retried, clone/runtime/session -// bounds, SSE silence (Kilo heartbeats every 10-15 s), health request, the -// real-SSE reconnect window, credential grants and the provider stop ladder. +// container boot, the prep attempt that must outlast create+connect+clone+ +// runtime+session, the pause before a failed create is retried, clone/runtime/ +// session bounds, SSE silence (Kilo heartbeats every 10-15 s), health request, +// the real-SSE reconnect window, credential grants and the provider stop +// ladder. function buildControlPlaneTimers(divisor: number): ControlPlaneTimers { const ms = (value: number): number => Math.max(1, Math.round(value / divisor)); const sandbox = { - providerCreateMs: 2 * MINUTE_MS, - providerCreateRetryMs: 10_000, - wrapperFirstConnectMs: 5 * MINUTE_MS, - wrapperHelloMs: 30_000, + providerCreateMs: PROVIDER_CREATE_MS, + providerCreateRetryMs: PROVIDER_CREATE_RETRY_MS, + wrapperFirstConnectMs: WRAPPER_FIRST_CONNECT_MS, + wrapperHelloMs: WRAPPER_HELLO_MS, heartbeatMs: ms(45_000), reconnectMs: ms(90_000), - routePreparationMs: 12 * MINUTE_MS, + routePreparationMs: ROUTE_PREPARATION_MS, idleMs: ms(10 * MINUTE_MS), unattendedIdleMs: ms(MINUTE_MS), // Bounds one Sandbox -> Session notification; real (not scaled) like the @@ -108,11 +137,11 @@ function buildControlPlaneTimers(divisor: number): ControlPlaneTimers { heartbeatNegotiationMs: ms(1_000), // Scales with the reconnect window it must stay inside. handshakeMs: ms(30_000), - cloneMs: 6 * MINUTE_MS, - restoreMs: 2 * MINUTE_MS, + cloneMs: CLONE_MS, + restoreMs: RESTORE_MS, captureMs: 5 * MINUTE_MS + 10_000, - kiloRuntimeStartMs: 2 * MINUTE_MS, - kiloSessionMs: 2 * MINUTE_MS, + kiloRuntimeStartMs: KILO_RUNTIME_START_MS, + kiloSessionMs: KILO_SESSION_MS, sseSilenceMs: 30_000, healthRequestMs: 5_000, sseReconnectLimit: 6, diff --git a/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts b/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts index bb73f8f69aa..c32bc7cf643 100644 --- a/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts +++ b/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts @@ -125,7 +125,7 @@ describe('SandboxSessionV2 queued transport recovery', () => { const scaled = resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: '10' }); expect(scaled.session.sandboxRpcDeadlineMs).toBe(2_000); expect(scaled.session.transportRecoveryMs).toBe(1_500); - expect(scaled.sandbox.routePreparationMs).toBe(12 * 60_000); + expect(scaled.sandbox.routePreparationMs).toBe(CONTROL_PLANE_TIMERS.sandbox.routePreparationMs); expect(resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: 'invalid' })).toBe( CONTROL_PLANE_TIMERS ); From 70fd47fc2f99ed577a7e6d3854c49e1626dfa50c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:11:51 +0000 Subject: [PATCH 5/6] Revert "fix(cloud-agent-next): budget a preparation attempt for its own steps" This reverts commit 155b6f4cbf2625151119bb8e9b0f28b478575c78. --- .../cloud-agent-next/docs/control-plane.md | 4 +- .../src/control-plane/sandbox/routes.ts | 2 +- .../src/control-plane/sandbox/sandbox-do.ts | 2 +- .../src/shared/control-plane-timers.test.ts | 27 --------- .../src/shared/control-plane-timers.ts | 55 +++++-------------- ...control-plane-session-v2-transport.test.ts | 2 +- 6 files changed, 17 insertions(+), 75 deletions(-) diff --git a/services/cloud-agent-next/docs/control-plane.md b/services/cloud-agent-next/docs/control-plane.md index 6d946a51e87..7d5296f52a8 100644 --- a/services/cloud-agent-next/docs/control-plane.md +++ b/services/cloud-agent-next/docs/control-plane.md @@ -309,9 +309,7 @@ so a later report changes nothing. ### Route state (per session) -`preparing` → `ready` → `failed`. Each preparation attempt has a deadline that outlasts its -own steps in sequence (sandbox create, wrapper first connect and hello, snapshot restore with -the clone fallback, Kilo runtime start, session import). An attempt +`preparing` → `ready` → `failed`. Each preparation attempt has a 12-minute deadline. An attempt starts when a route enters `preparing`: the first `prepare`, a `prepare` after `failed`, or a re-prepare after a wrapper restart. A repeated `prepare`, a reconnect or a new allocation inside the attempt keeps its deadline. diff --git a/services/cloud-agent-next/src/control-plane/sandbox/routes.ts b/services/cloud-agent-next/src/control-plane/sandbox/routes.ts index 14addebae02..b21988b7a04 100644 --- a/services/cloud-agent-next/src/control-plane/sandbox/routes.ts +++ b/services/cloud-agent-next/src/control-plane/sandbox/routes.ts @@ -500,7 +500,7 @@ export async function failReadyRoutesForScope( } } -/** Fail preparing routes whose preparation attempt deadline has passed. */ +/** Fail preparing routes whose 12-minute attempt deadline has passed. */ export async function failExpiredRoutes(ctx: RouteContext): Promise { const now = ctx.now(); for (const route of await listRoutes(ctx.db)) { diff --git a/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts b/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts index 7857c747e12..c7d005caccd 100644 --- a/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts +++ b/services/cloud-agent-next/src/control-plane/sandbox/sandbox-do.ts @@ -5443,7 +5443,7 @@ export class SandboxControlV2 extends DurableObject { private async armAlarm(state: AllocationState): Promise { // The one alarm is the earliest of the allocation timers and the route // preparation deadlines, so a hung wrapper's route cannot outlive its - // preparation attempt. + // 12-minute attempt. const allocationAt = nextAllocationAlarmAt( state.kind === 'connected' && [...this.repositoryCaptures.values()].some( diff --git a/services/cloud-agent-next/src/shared/control-plane-timers.test.ts b/services/cloud-agent-next/src/shared/control-plane-timers.test.ts index e4055d0a9ba..1b895ce5dfe 100644 --- a/services/cloud-agent-next/src/shared/control-plane-timers.test.ts +++ b/services/cloud-agent-next/src/shared/control-plane-timers.test.ts @@ -35,33 +35,6 @@ describe('control-plane idle timers', () => { }); }); -describe('control-plane preparation attempt budget', () => { - it('outlasts every step of one preparation attempt in sequence', () => { - const { sandbox, wrapper } = CONTROL_PLANE_TIMERS; - const steps = - sandbox.providerCreateMs + - sandbox.wrapperFirstConnectMs + - sandbox.wrapperHelloMs + - wrapper.restoreMs + - wrapper.cloneMs + - wrapper.kiloRuntimeStartMs + - wrapper.kiloSessionMs; - - // A shorter attempt deadline would fail a slow but valid preparation as - // `preparation_timeout` while each step stayed inside its own bound. - expect(sandbox.routePreparationMs).toBeGreaterThanOrEqual(steps); - }); - - it('keeps the attempt budget real when the development divisor shrinks step timers', () => { - const scaled = resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: '10' }); - - expect(scaled.sandbox.routePreparationMs).toBe(CONTROL_PLANE_TIMERS.sandbox.routePreparationMs); - expect(scaled.sandbox.routePreparationMs).toBeGreaterThanOrEqual( - scaled.wrapper.cloneMs + scaled.wrapper.restoreMs - ); - }); -}); - describe('sandbox idle policy for route origins', () => { it('keeps the interactive window for a sandbox without routes', () => { expect(sandboxIdlePolicyForOrigins([])).toBe('interactive'); diff --git a/services/cloud-agent-next/src/shared/control-plane-timers.ts b/services/cloud-agent-next/src/shared/control-plane-timers.ts index 12e8b97f4a4..44fe2d18d99 100644 --- a/services/cloud-agent-next/src/shared/control-plane-timers.ts +++ b/services/cloud-agent-next/src/shared/control-plane-timers.ts @@ -72,52 +72,23 @@ export type ControlPlaneTimers = { }; }; -// External-work bounds for one preparation attempt. They stay real (never -// scaled) because they bound provider and container work. -const PROVIDER_CREATE_MS = 2 * MINUTE_MS; -const PROVIDER_CREATE_RETRY_MS = 10_000; -const WRAPPER_FIRST_CONNECT_MS = 5 * MINUTE_MS; -const WRAPPER_HELLO_MS = 30_000; -const RESTORE_MS = 2 * MINUTE_MS; -const CLONE_MS = 6 * MINUTE_MS; -const KILO_RUNTIME_START_MS = 2 * MINUTE_MS; -const KILO_SESSION_MS = 2 * MINUTE_MS; - -/** - * One preparation attempt runs these steps in order: sandbox create, wrapper - * first connect and hello, snapshot restore (falling back to a clone), Kilo - * runtime start, then the session import. Every step keeps its own bound, so - * the attempt deadline is their sum. A shorter deadline fails a slow but valid - * preparation as `preparation_timeout` while each step stayed inside its own - * budget — the failure names a timeout the plan never allowed to finish. - */ -const ROUTE_PREPARATION_MS = - PROVIDER_CREATE_MS + - WRAPPER_FIRST_CONNECT_MS + - WRAPPER_HELLO_MS + - RESTORE_MS + - CLONE_MS + - KILO_RUNTIME_START_MS + - KILO_SESSION_MS; - // The divisor shortens only internal deadlines whose partner also scales, so // their orderings hold (the 90-s reconnect window stays longer than the 30-s // max backoff). Timers that bound external work stay real: provider create and -// container boot, the prep attempt that must outlast create+connect+clone+ -// runtime+session, the pause before a failed create is retried, clone/runtime/ -// session bounds, SSE silence (Kilo heartbeats every 10-15 s), health request, -// the real-SSE reconnect window, credential grants and the provider stop -// ladder. +// container boot, the 12-min prep attempt that must outlast clone+runtime+ +// session, the pause before a failed create is retried, clone/runtime/session +// bounds, SSE silence (Kilo heartbeats every 10-15 s), health request, the +// real-SSE reconnect window, credential grants and the provider stop ladder. function buildControlPlaneTimers(divisor: number): ControlPlaneTimers { const ms = (value: number): number => Math.max(1, Math.round(value / divisor)); const sandbox = { - providerCreateMs: PROVIDER_CREATE_MS, - providerCreateRetryMs: PROVIDER_CREATE_RETRY_MS, - wrapperFirstConnectMs: WRAPPER_FIRST_CONNECT_MS, - wrapperHelloMs: WRAPPER_HELLO_MS, + providerCreateMs: 2 * MINUTE_MS, + providerCreateRetryMs: 10_000, + wrapperFirstConnectMs: 5 * MINUTE_MS, + wrapperHelloMs: 30_000, heartbeatMs: ms(45_000), reconnectMs: ms(90_000), - routePreparationMs: ROUTE_PREPARATION_MS, + routePreparationMs: 12 * MINUTE_MS, idleMs: ms(10 * MINUTE_MS), unattendedIdleMs: ms(MINUTE_MS), // Bounds one Sandbox -> Session notification; real (not scaled) like the @@ -137,11 +108,11 @@ function buildControlPlaneTimers(divisor: number): ControlPlaneTimers { heartbeatNegotiationMs: ms(1_000), // Scales with the reconnect window it must stay inside. handshakeMs: ms(30_000), - cloneMs: CLONE_MS, - restoreMs: RESTORE_MS, + cloneMs: 6 * MINUTE_MS, + restoreMs: 2 * MINUTE_MS, captureMs: 5 * MINUTE_MS + 10_000, - kiloRuntimeStartMs: KILO_RUNTIME_START_MS, - kiloSessionMs: KILO_SESSION_MS, + kiloRuntimeStartMs: 2 * MINUTE_MS, + kiloSessionMs: 2 * MINUTE_MS, sseSilenceMs: 30_000, healthRequestMs: 5_000, sseReconnectLimit: 6, diff --git a/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts b/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts index c32bc7cf643..bb73f8f69aa 100644 --- a/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts +++ b/services/cloud-agent-next/test/integration/control-plane-session-v2-transport.test.ts @@ -125,7 +125,7 @@ describe('SandboxSessionV2 queued transport recovery', () => { const scaled = resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: '10' }); expect(scaled.session.sandboxRpcDeadlineMs).toBe(2_000); expect(scaled.session.transportRecoveryMs).toBe(1_500); - expect(scaled.sandbox.routePreparationMs).toBe(CONTROL_PLANE_TIMERS.sandbox.routePreparationMs); + expect(scaled.sandbox.routePreparationMs).toBe(12 * 60_000); expect(resolveControlPlaneTimers({ CONTROL_PLANE_TIMER_DIVISOR: 'invalid' })).toBe( CONTROL_PLANE_TIMERS ); From e1b496795f7ccc45aeded6a712d5dfbbbd78800e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:47:48 +0000 Subject: [PATCH 6/6] fix(code-reviews): name the remedy when a project is outside the repository list The repository list is the project's authorization scope, so the refusal now tells the user to refresh the integration's repository list. The manual job test fixture keeps the provider's project ids different from the cached repository id, so the assertion pins which id the review row stores. --- .../src/lib/code-reviews/manual-code-review-jobs.test.ts | 6 ++++-- apps/web/src/lib/provider-review/gitlab-authorization.ts | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts index 76ae7571004..20e0c1143a7 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts @@ -255,8 +255,10 @@ describe('createManualCodeReviewJob connected GitLab source', () => { sha: 'sha-7', web_url: MR_URL, author: { username: 'gl-user' }, - project_id: 501, - target_project_id: 501, + // The provider's ids differ from the cached repository id (501) so the + // assertion pins which id the review row stores. + project_id: 999, + target_project_id: 999, ...overrides, }; } diff --git a/apps/web/src/lib/provider-review/gitlab-authorization.ts b/apps/web/src/lib/provider-review/gitlab-authorization.ts index 3d3fc80cc5e..10496bf8d7a 100644 --- a/apps/web/src/lib/provider-review/gitlab-authorization.ts +++ b/apps/web/src/lib/provider-review/gitlab-authorization.ts @@ -355,9 +355,11 @@ export async function authorizeProject( repo => repo.full_name.toLowerCase() === requested.toLowerCase() ); if (!match) { + // The repository list is the project's authorization scope, so this refusal + // names the one remedy that restores it. throw new GitLabReviewError( 'not_found', - 'This project is not part of your connected GitLab repositories.' + 'This project is not in your connected GitLab repositories. Refresh the repository list on the GitLab integration and try again.' ); }