From 2ab0e95d11396a2b294e43b812806199996f9ad4 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:39:11 +0000 Subject: [PATCH 1/2] Fix PR #351 review issues for GitLab, Gitea, Docker, and PR funnel Use OAuth-aware GitLab listing headers, ignore Gitea deployment-token bots, clean nested Docker daemons on failed standby resume, prefer public OAuth callback URLs, stop source-control form resets on status refetch, and scope instance-report PR funnel keys by host. --- .../gitea/__tests__/handleComment.test.ts | 25 ++++++++ apps/api/src/handlers/gitea/handleComment.ts | 27 +++++++- .../compute-providers/spawn-docker-worker.ts | 8 +++ .../gitlab/oauth/authorize/route.ts | 5 +- .../gitlab/oauth/callback/route.ts | 7 ++- .../settings/SourceControlConfigForm.tsx | 11 ++-- .../src/lib/__tests__/instance-report.test.ts | 63 ++++++++++++++++--- packages/db/src/lib/instance-report.ts | 22 +++++-- .../source-control-pull-request-reads.test.ts | 42 ++++++++++++- .../source-control-pull-request-reads.ts | 2 +- 10 files changed, 184 insertions(+), 28 deletions(-) diff --git a/apps/api/src/handlers/gitea/__tests__/handleComment.test.ts b/apps/api/src/handlers/gitea/__tests__/handleComment.test.ts index a8bfb000..8455165a 100644 --- a/apps/api/src/handlers/gitea/__tests__/handleComment.test.ts +++ b/apps/api/src/handlers/gitea/__tests__/handleComment.test.ts @@ -3,6 +3,7 @@ const { mockGetTaskUrl, mockGetGiteaAutomationTargets, mockCreateGiteaPullRequestComment, + mockGetGiteaDeploymentUser, mockFindActiveGitHubPrReviewTask, mockFindReusableGitHubPrFollowUpOwner, mockSendMessageToTask, @@ -12,6 +13,7 @@ const { mockGetTaskUrl: vi.fn(), mockGetGiteaAutomationTargets: vi.fn(), mockCreateGiteaPullRequestComment: vi.fn(), + mockGetGiteaDeploymentUser: vi.fn(), mockFindActiveGitHubPrReviewTask: vi.fn(), mockFindReusableGitHubPrFollowUpOwner: vi.fn(), mockSendMessageToTask: vi.fn(), @@ -25,6 +27,7 @@ vi.mock('@roomote/cloud-agents/server', () => ({ vi.mock('@roomote/gitea', () => ({ createGiteaPullRequestComment: mockCreateGiteaPullRequestComment, + getGiteaDeploymentUser: mockGetGiteaDeploymentUser, })); vi.mock('@roomote/db/server', () => ({ @@ -107,11 +110,13 @@ describe('handleGiteaComment', () => { mockGetTaskUrl.mockReset(); mockGetGiteaAutomationTargets.mockReset(); mockCreateGiteaPullRequestComment.mockReset(); + mockGetGiteaDeploymentUser.mockReset(); mockFindActiveGitHubPrReviewTask.mockReset(); mockFindReusableGitHubPrFollowUpOwner.mockReset(); mockSendMessageToTask.mockReset(); mockSteerMessageToTask.mockReset(); + mockGetGiteaDeploymentUser.mockResolvedValue(null); mockGetGiteaAutomationTargets.mockResolvedValue({ status: 'ok', targets: [ @@ -380,6 +385,26 @@ describe('handleGiteaComment', () => { expect(mockEnqueueTask).not.toHaveBeenCalled(); }); + it('ignores comments from the deployment token identity even without a roomote username prefix', async () => { + mockGetGiteaDeploymentUser.mockResolvedValue({ + id: 99, + login: 'deploy-bot', + }); + + const result = await handleGiteaComment( + makeCommentPayload({ + sender: { id: 99, login: 'deploy-bot' }, + comment: { user: { id: 99, login: 'deploy-bot' } }, + }), + ); + + expect(result).toEqual({ + status: 'ok', + message: 'roomote_authored_comment', + }); + expect(mockEnqueueTask).not.toHaveBeenCalled(); + }); + it('requires linked commenter attribution for explicit mentions', async () => { await handleGiteaComment(makeCommentPayload()); diff --git a/apps/api/src/handlers/gitea/handleComment.ts b/apps/api/src/handlers/gitea/handleComment.ts index 1050f8be..10e5add6 100644 --- a/apps/api/src/handlers/gitea/handleComment.ts +++ b/apps/api/src/handlers/gitea/handleComment.ts @@ -3,7 +3,10 @@ import { findActiveGitHubPrReviewTask, findReusableGitHubPrFollowUpOwner, } from '@roomote/db/server'; -import { createGiteaPullRequestComment } from '@roomote/gitea'; +import { + createGiteaPullRequestComment, + getGiteaDeploymentUser, +} from '@roomote/gitea'; import { type TaskPayload, TaskPayloadKind, @@ -43,6 +46,23 @@ function isGiteaMention(commentBody: string): boolean { return commentBody.toLowerCase().includes(GITEA_MENTION_HANDLE); } +async function isDeploymentTokenAuthor(username: string): Promise { + try { + const deploymentUser = await getGiteaDeploymentUser(); + + return ( + !!deploymentUser && + deploymentUser.login.toLowerCase() === username.toLowerCase() + ); + } catch (error) { + console.warn( + `[handleGiteaComment] failed to resolve Gitea deployment token identity: ${error instanceof Error ? error.message : String(error)}`, + ); + + return false; + } +} + async function postMentionResponseComment({ repositoryFullName, pullRequestNumber, @@ -208,7 +228,10 @@ export async function handleGiteaComment( return { status: 'ok', message: 'no_comment_author' }; } - if (isRoomoteGiteaUsername(commenter)) { + if ( + isRoomoteGiteaUsername(commenter) || + (await isDeploymentTokenAuthor(commenter)) + ) { return { status: 'ok', message: 'roomote_authored_comment' }; } diff --git a/apps/controller/src/compute-providers/spawn-docker-worker.ts b/apps/controller/src/compute-providers/spawn-docker-worker.ts index 9c898529..36077d20 100644 --- a/apps/controller/src/compute-providers/spawn-docker-worker.ts +++ b/apps/controller/src/compute-providers/spawn-docker-worker.ts @@ -434,6 +434,14 @@ export async function spawnDockerWorker( await docker(['stop', '--time', '10', containerName], { allowFailure: true, }); + // Nested Docker project daemons are started on standby resume. On a + // failed resume, the retained worker is only stopped for reuse; the + // privileged `-docker` daemon must still be removed. + if (usesDockerProjects) { + await docker(['rm', '-f', taskDaemonContainerName], { + allowFailure: true, + }); + } } else if (resolveAppEnv(process.env) === 'development' && containerId) { console.error( `[spawnDockerWorker] Preserving failed Docker worker container ${containerName} for local debugging`, diff --git a/apps/web/src/app/api/source-control/gitlab/oauth/authorize/route.ts b/apps/web/src/app/api/source-control/gitlab/oauth/authorize/route.ts index 5d6fac49..fa5e5d2d 100644 --- a/apps/web/src/app/api/source-control/gitlab/oauth/authorize/route.ts +++ b/apps/web/src/app/api/source-control/gitlab/oauth/authorize/route.ts @@ -28,7 +28,8 @@ export async function GET() { { status: 400 }, ); - const redirectUri = buildGitLabOAuthRedirectUri(webEnv.R_APP_URL); + const publicAppUrl = webEnv.R_PUBLIC_URL ?? webEnv.R_APP_URL; + const redirectUri = buildGitLabOAuthRedirectUri(publicAppUrl); const { url, state } = createGitLabOAuthAuthorizationUrl({ baseUrl, clientId, @@ -38,7 +39,7 @@ export async function GET() { response.cookies.set('roomote-gitlab-oauth-state', state, { httpOnly: true, sameSite: 'lax', - secure: webEnv.R_APP_URL.startsWith('https://'), + secure: publicAppUrl.startsWith('https://'), path: '/api/source-control/gitlab/oauth', maxAge: 600, }); diff --git a/apps/web/src/app/api/source-control/gitlab/oauth/callback/route.ts b/apps/web/src/app/api/source-control/gitlab/oauth/callback/route.ts index 1f096e30..dd63facb 100644 --- a/apps/web/src/app/api/source-control/gitlab/oauth/callback/route.ts +++ b/apps/web/src/app/api/source-control/gitlab/oauth/callback/route.ts @@ -14,7 +14,8 @@ export const dynamic = 'force-dynamic'; export async function GET(request: NextRequest) { const webEnv = await bootstrapWebRuntimeEnv(); - const redirect = new URL('/setup', webEnv.R_APP_URL); + const publicAppUrl = webEnv.R_PUBLIC_URL ?? webEnv.R_APP_URL; + const redirect = new URL('/setup', publicAppUrl); redirect.searchParams.set('step', 'source-control-connect'); const response = () => NextResponse.redirect(redirect); const authResult = await authorize(); @@ -46,7 +47,7 @@ export async function GET(request: NextRequest) { clientId, clientSecret, code, - redirectUri: buildGitLabOAuthRedirectUri(webEnv.R_APP_URL), + redirectUri: buildGitLabOAuthRedirectUri(publicAppUrl), }); redirect.searchParams.set('gitlab', 'connected'); redirect.searchParams.set('sync', '1'); @@ -58,7 +59,7 @@ export async function GET(request: NextRequest) { result.cookies.set('roomote-gitlab-oauth-state', '', { httpOnly: true, sameSite: 'lax', - secure: webEnv.R_APP_URL.startsWith('https://'), + secure: publicAppUrl.startsWith('https://'), path: '/api/source-control/gitlab/oauth', maxAge: 0, }); diff --git a/apps/web/src/components/settings/SourceControlConfigForm.tsx b/apps/web/src/components/settings/SourceControlConfigForm.tsx index 607eeb00..3b3e6191 100644 --- a/apps/web/src/components/settings/SourceControlConfigForm.tsx +++ b/apps/web/src/components/settings/SourceControlConfigForm.tsx @@ -173,13 +173,10 @@ export function SourceControlConfigForm({ : 'pat', ); } - }, [ - provider, - nonSecretInitialValues, - isAdo, - nonSecretInitialValuesKey, - providerStatus?.fields, - ]); + // providerStatus.fields array identity is intentionally omitted; content + // key and derived non-secret values drive resets instead of query refetches. + // eslint-disable-next-line react-hooks/exhaustive-deps -- content-keyed + }, [provider, nonSecretInitialValues, isAdo, nonSecretInitialValuesKey]); if (!providerStatus) { return null; diff --git a/packages/db/src/lib/__tests__/instance-report.test.ts b/packages/db/src/lib/__tests__/instance-report.test.ts index d01317d9..e63f8194 100644 --- a/packages/db/src/lib/__tests__/instance-report.test.ts +++ b/packages/db/src/lib/__tests__/instance-report.test.ts @@ -44,6 +44,7 @@ describe('instance-report pure helpers', () => { const deduped = dedupeAuthoredPullRequests([ { sourceControlProvider: 'github', + host: 'github.com', repository: 'Acme/App', repositoryId: 'repo-1', prNumber: 7, @@ -54,6 +55,7 @@ describe('instance-report pure helpers', () => { }, { sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'repo-1', prNumber: 7, @@ -81,7 +83,9 @@ describe('instance-report pure helpers', () => { const result = summarizePullRequestCohort( [ { - key: 'github:acme/app#1', + key: 'github:github.com:acme/app#1', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 1, @@ -89,7 +93,9 @@ describe('instance-report pure helpers', () => { firstDetectedAt: inWindow, }, { - key: 'github:acme/app#2', + key: 'github:github.com:acme/app#2', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 2, @@ -97,7 +103,9 @@ describe('instance-report pure helpers', () => { firstDetectedAt: inWindow, }, { - key: 'github:acme/app#3', + key: 'github:github.com:acme/app#3', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 3, @@ -105,7 +113,9 @@ describe('instance-report pure helpers', () => { firstDetectedAt: inWindow, }, { - key: 'github:acme/app#4', + key: 'github:github.com:acme/app#4', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 4, @@ -113,7 +123,9 @@ describe('instance-report pure helpers', () => { firstDetectedAt: inWindow, }, { - key: 'github:acme/app#5', + key: 'github:github.com:acme/app#5', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 5, @@ -121,7 +133,9 @@ describe('instance-report pure helpers', () => { firstDetectedAt: inWindow, }, { - key: 'github:acme/app#old', + key: 'github:github.com:acme/app#old', + sourceControlProvider: 'github', + host: 'github.com', repository: 'acme/app', repositoryId: 'r1', prNumber: 99, @@ -131,8 +145,8 @@ describe('instance-report pure helpers', () => { ], since, new Map([ - ['github:acme/app#4', 100], - ['github:acme/app#5', 300], + ['github:github.com:acme/app#4', 100], + ['github:github.com:acme/app#5', 300], ]), ); @@ -144,6 +158,38 @@ describe('instance-report pure helpers', () => { medianTimeToMergeSeconds: 200, }); }); + + it('keeps same-named repos on different hosts as distinct PR keys', () => { + const now = new Date('2026-07-10T12:00:00.000Z'); + + const deduped = dedupeAuthoredPullRequests([ + { + sourceControlProvider: 'gitlab', + host: 'gitlab.a.example', + repository: 'acme/app', + repositoryId: 'repo-a', + prNumber: 3, + prUrl: 'https://gitlab.a.example/acme/app/-/merge_requests/3', + status: 'open', + detectedAt: now, + updatedAt: now, + }, + { + sourceControlProvider: 'gitlab', + host: 'gitlab.b.example', + repository: 'acme/app', + repositoryId: 'repo-b', + prNumber: 3, + prUrl: 'https://gitlab.b.example/acme/app/-/merge_requests/3', + status: 'merged', + detectedAt: now, + updatedAt: now, + }, + ]); + + expect(deduped).toHaveLength(2); + expect(new Set(deduped.map((entry) => entry.key)).size).toBe(2); + }); }); describe('collectInstanceReportStats pullRequests7d isolation', () => { @@ -315,6 +361,7 @@ describe('collectInstanceReportStats pullRequests7d isolation', () => { const productOpenedRows = await db .select({ sourceControlProvider: taskPullRequests.sourceControlProvider, + host: taskPullRequests.host, repository: taskPullRequests.repository, repositoryId: taskPullRequests.repositoryId, prNumber: taskPullRequests.prNumber, diff --git a/packages/db/src/lib/instance-report.ts b/packages/db/src/lib/instance-report.ts index f4f18e75..eef76ffe 100644 --- a/packages/db/src/lib/instance-report.ts +++ b/packages/db/src/lib/instance-report.ts @@ -127,6 +127,7 @@ export type InstanceReportStats = { type AuthoredPullRequestRow = { sourceControlProvider: string; + host: string | null; repository: string | null; repositoryId: string | null; prNumber: number | null; @@ -138,6 +139,8 @@ type AuthoredPullRequestRow = { type DedupedAuthoredPullRequest = { key: string; + sourceControlProvider: string; + host: string | null; repository: string | null; repositoryId: string | null; prNumber: number | null; @@ -155,15 +158,18 @@ function toNumber(value: string | number | null | undefined): number { function getAuthoredPullRequestKey(row: { sourceControlProvider: string; + host: string | null; repository: string | null; prNumber: number | null; prUrl: string; }): string { + const host = (row.host ?? '').toLowerCase(); + if (row.repository && row.prNumber != null) { - return `${row.sourceControlProvider}:${row.repository.toLowerCase()}#${row.prNumber}`; + return `${row.sourceControlProvider}:${host}:${row.repository.toLowerCase()}#${row.prNumber}`; } - return `${row.sourceControlProvider}:url:${row.prUrl.toLowerCase()}`; + return `${row.sourceControlProvider}:${host}:url:${row.prUrl.toLowerCase()}`; } /** @@ -224,6 +230,8 @@ export function dedupeAuthoredPullRequests( if (!existing) { byKey.set(key, { key, + sourceControlProvider: row.sourceControlProvider, + host: row.host, repository: row.repository, repositoryId: row.repositoryId, prNumber: row.prNumber, @@ -241,6 +249,8 @@ export function dedupeAuthoredPullRequests( if (row.updatedAt >= existing.latestUpdatedAt) { existing.latestUpdatedAt = row.updatedAt; existing.status = row.status; + existing.sourceControlProvider = row.sourceControlProvider; + existing.host = row.host; existing.repository = row.repository; existing.repositoryId = row.repositoryId; existing.prNumber = row.prNumber; @@ -354,11 +364,14 @@ async function collectPullRequestFactsDurations( .select({ repositoryId: pullRequestFacts.repositoryId, repositoryFullName: pullRequestFacts.repositoryFullName, + sourceControlProvider: pullRequestFacts.sourceControlProvider, + host: repositories.host, prNumber: pullRequestFacts.prNumber, createdAtRemote: pullRequestFacts.createdAtRemote, mergedAtRemote: pullRequestFacts.mergedAtRemote, }) .from(pullRequestFacts) + .innerJoin(repositories, eq(repositories.id, pullRequestFacts.repositoryId)) .where( and( inArray(pullRequestFacts.prNumber, prNumbers), @@ -383,7 +396,7 @@ async function collectPullRequestFactsDurations( factsByRepoIdPr.set(`${fact.repositoryId}#${fact.prNumber}`, seconds); factsByNamePr.set( - `${fact.repositoryFullName.toLowerCase()}#${fact.prNumber}`, + `${fact.sourceControlProvider}:${(fact.host ?? '').toLowerCase()}:${fact.repositoryFullName.toLowerCase()}#${fact.prNumber}`, seconds, ); } @@ -399,7 +412,7 @@ async function collectPullRequestFactsDurations( } if (seconds == null && entry.repository) { seconds = factsByNamePr.get( - `${entry.repository.toLowerCase()}#${entry.prNumber}`, + `${entry.sourceControlProvider}:${(entry.host ?? '').toLowerCase()}:${entry.repository.toLowerCase()}#${entry.prNumber}`, ); } @@ -413,6 +426,7 @@ async function collectPullRequestFactsDurations( const authoredPullRequestSelect = { sourceControlProvider: taskPullRequests.sourceControlProvider, + host: taskPullRequests.host, repository: taskPullRequests.repository, repositoryId: taskPullRequests.repositoryId, prNumber: taskPullRequests.prNumber, diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-reads.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-reads.test.ts index f89550aa..eecdd2e4 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-reads.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-reads.test.ts @@ -39,7 +39,7 @@ vi.mock('@roomote/github', () => ({ vi.mock('@roomote/gitlab', () => ({ resolveGitLabToken: (...args: unknown[]) => mockResolveGitLabToken(...args), - isGitLabOAuthAccessToken: () => false, + isGitLabOAuthAccessToken: (token: string) => token === 'oauth-token', resolveGitLabBaseUrl: async () => 'https://gitlab.com', buildGitLabApiBaseUrl: (baseUrl: string) => `${baseUrl.replace(/\/+$/, '')}/api/v4`, @@ -1251,6 +1251,46 @@ describe('listMergedSourceControlPullRequestsForRepository', () => { ]); }); + it('lists GitLab MRs with Authorization Bearer when the deployment token is OAuth-backed', async () => { + mockResolveGitLabToken.mockResolvedValue('oauth-token'); + const fetchImpl = vi.fn().mockResolvedValueOnce( + jsonResponse([ + { + id: 992, + iid: 43, + title: 'OAuth listed MR', + state: 'merged', + web_url: 'https://gitlab.com/acme/backend/-/merge_requests/43', + source_branch: 'feature/oauth', + target_branch: 'main', + created_at: '2026-06-30T00:00:00Z', + updated_at: '2026-07-10T00:00:00Z', + merged_at: '2026-07-10T00:00:00Z', + closed_at: null, + author: { id: 8, username: 'oauth-user' }, + }, + ]), + ); + + await listMergedSourceControlPullRequestsForRepository({ + repository: makeRepositoryRow({ + sourceControlProvider: 'gitlab', + externalRepoId: '101', + }), + provider: 'gitlab', + fetchImpl, + }); + + expect(fetchImpl).toHaveBeenCalledWith( + expect.stringContaining('/merge_requests?'), + expect.objectContaining({ + headers: expect.objectContaining({ + Authorization: 'Bearer oauth-token', + }), + }), + ); + }); + it('lists Gitea merged PRs from the closed list, dropping unmerged rows', async () => { const fetchImpl = vi.fn().mockResolvedValueOnce( jsonResponse([ diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-reads.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-reads.ts index 9259a791..cba785bf 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-reads.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-reads.ts @@ -2100,7 +2100,7 @@ async function listGitLabMergeRequests({ : {}), }, ), - tokenHeader: { name: 'PRIVATE-TOKEN', value: token }, + tokenHeader: buildGitLabTokenHeader(token), schema: gitLabMergeRequestListPageSchema, }); From ea365abf35b78eb6e48aee07ab37fffb6ee31e1e Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:49:23 +0000 Subject: [PATCH 2/2] Stop nested Docker daemon on failed standby resume Preserve the retained daemon container so later resumes can restart it; only stop it so a failed resume does not leave the privileged daemon running. --- .../src/compute-providers/spawn-docker-worker.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apps/controller/src/compute-providers/spawn-docker-worker.ts b/apps/controller/src/compute-providers/spawn-docker-worker.ts index 36077d20..2ea6d2c8 100644 --- a/apps/controller/src/compute-providers/spawn-docker-worker.ts +++ b/apps/controller/src/compute-providers/spawn-docker-worker.ts @@ -434,11 +434,12 @@ export async function spawnDockerWorker( await docker(['stop', '--time', '10', containerName], { allowFailure: true, }); - // Nested Docker project daemons are started on standby resume. On a - // failed resume, the retained worker is only stopped for reuse; the - // privileged `-docker` daemon must still be removed. + // Nested Docker project daemons are started on standby resume. Stop the + // privileged `-docker` daemon so it does not keep running after a + // failed resume, but keep the container retained for later `docker start` + // (resume never recreates the daemon). if (usesDockerProjects) { - await docker(['rm', '-f', taskDaemonContainerName], { + await docker(['stop', '--time', '10', taskDaemonContainerName], { allowFailure: true, }); }