From ac0c46f6a645e2d0ad5e50f958f5f367c026a36c Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 16:28:10 +0000 Subject: [PATCH 1/8] fix: use public-safe source control attribution --- .changeset/safe-public-attribution.md | 8 + apps/docs/source-control.mdx | 17 ++ .../cloud-agents/src/server/commit-author.ts | 24 ++- .../requestUserInputGuidance.test.ts | 1 + .../__tests__/slackAppMention.test.ts | 1 + .../src/server/workflows/standardTask.ts | 1 + .../__tests__/source-control-provider.test.ts | 95 +++++++++++ .../db/src/lib/source-control-provider.ts | 139 ++++++++++++++++ .../source-control-pull-requests.test.ts | 156 +++++++++++++++++- .../source-control-pull-request-shared.ts | 2 + .../source-control-pull-requests.ts | 68 ++++---- .../__tests__/resolve-git-author.test.ts | 138 +++++++++++++--- .../server/lib/task-runs/dequeue-helpers.ts | 31 +++- .../src/__tests__/github-bot-identity.test.ts | 18 ++ packages/types/src/constants.ts | 25 +++ 15 files changed, 657 insertions(+), 67 deletions(-) create mode 100644 .changeset/safe-public-attribution.md diff --git a/.changeset/safe-public-attribution.md b/.changeset/safe-public-attribution.md new file mode 100644 index 000000000..2c0749ebc --- /dev/null +++ b/.changeset/safe-public-attribution.md @@ -0,0 +1,8 @@ +--- +'@roomote/cloud-agents': patch +'@roomote/db': patch +'@roomote/sdk': patch +'@roomote/types': patch +--- + +Use linked source-control usernames instead of account names when attributing Roomote changes in public repositories. diff --git a/apps/docs/source-control.mdx b/apps/docs/source-control.mdx index 3a6c5cf86..e722e4e24 100644 --- a/apps/docs/source-control.mdx +++ b/apps/docs/source-control.mdx @@ -40,6 +40,23 @@ After setup, verify that Roomote can: - clone the repository inside a task sandbox - push a branch or open a reviewable change when the task finishes +## Attribution on pull requests and commits + +Roomote keeps human-readable attribution inside private repositories. For +public repositories, it uses the task participant's linked source-control +username when one is available. If Roomote cannot resolve a linked username, +the pull request or merge request says only that it was created by Roomote. + +Roomote never derives public attribution from an account email address. Commit +emails use the source-control provider's `noreply` identity when available, or +the Roomote identity otherwise. A workspace containing any public or +unresolved repository uses the public-safe identity for all new commits because +Git author configuration applies across the workspace. + +Changing a repository from private to public does not rewrite existing Git +history. Roomote sanitizes a legacy named attribution line the next time it +updates an open public pull request. + ## Pull request review comments When **Review Code** finds an issue on a changed line, Roomote posts the finding diff --git a/packages/cloud-agents/src/server/commit-author.ts b/packages/cloud-agents/src/server/commit-author.ts index b20b73087..7ef75e84e 100644 --- a/packages/cloud-agents/src/server/commit-author.ts +++ b/packages/cloud-agents/src/server/commit-author.ts @@ -1,7 +1,6 @@ import { type CommitAuthorKind, type TaskInitiator, - getUserDisplayName, PRODUCT_NAME, } from '@roomote/types'; import { @@ -69,6 +68,8 @@ export type ResolvedTaskCommitAuthor = { kind: CommitAuthorKind; /** Human-readable display name; PRODUCT_NAME for roomote authorship. */ displayName: string; + /** Source-control handle safe to publish, including its leading `@`. */ + publicDisplayName: string | null; githubLogin: string | null; prAssigneeLogin: string | null; gitAuthor: ResolvedGitAuthor; @@ -77,6 +78,7 @@ export type ResolvedTaskCommitAuthor = { export const DEFAULT_ROOMOTE_COMMIT_AUTHOR: ResolvedTaskCommitAuthor = { kind: 'roomote', displayName: PRODUCT_NAME, + publicDisplayName: null, githubLogin: null, prAssigneeLogin: null, gitAuthor: ROOMOTE_GIT_AUTHOR, @@ -212,7 +214,6 @@ export async function resolveTaskCommitAuthor( columns: { id: true, name: true, - email: true, }, }); @@ -224,14 +225,14 @@ export async function resolveTaskCommitAuthor( githubIdentity.githubLogin ?? normalizeNullableString(task.commitAuthorLogin); const displayName = - normalizeNullableString(getUserDisplayName(user)) ?? - githubLogin ?? - PRODUCT_NAME; + normalizeNullableString(user?.name) ?? githubLogin ?? PRODUCT_NAME; + const publicDisplayName = githubLogin ? `@${githubLogin}` : null; if (!githubIdentity.githubLogin || !githubIdentity.githubUserId) { return { kind: 'user', displayName, + publicDisplayName, githubLogin, prAssigneeLogin: null, gitAuthor: ROOMOTE_GIT_AUTHOR, @@ -241,6 +242,7 @@ export async function resolveTaskCommitAuthor( return { kind: 'user', displayName, + publicDisplayName, githubLogin, prAssigneeLogin: githubIdentity.githubLogin, gitAuthor: { @@ -257,11 +259,13 @@ export async function resolveTaskCommitAuthor( normalizeNullableString(task.actorDisplayName) ?? githubLogin ?? PRODUCT_NAME; + const publicDisplayName = githubLogin ? `@${githubLogin}` : null; if (!githubLogin || !externalId) { return { kind: 'external', displayName, + publicDisplayName, githubLogin, prAssigneeLogin, gitAuthor: ROOMOTE_GIT_AUTHOR, @@ -271,6 +275,7 @@ export async function resolveTaskCommitAuthor( return { kind: 'external', displayName, + publicDisplayName, githubLogin, prAssigneeLogin, gitAuthor: { @@ -286,6 +291,15 @@ export async function resolveTaskCommitAuthor( }; } +/** Use the provider noreply identity only when its public handle is available. */ +export function resolvePublicGitAuthor( + attribution: ResolvedTaskCommitAuthor, +): ResolvedGitAuthor { + return attribution.publicDisplayName + ? { ...attribution.gitAuthor, name: attribution.publicDisplayName } + : ROOMOTE_GIT_AUTHOR; +} + /** * Resolves attribution for a live run. A linked participant owns their turns; * all ownerless or unlinked runs use the Roomote app identity. diff --git a/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts index 58314db4a..ec6c592b8 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts @@ -33,6 +33,7 @@ const teamSlackPermalink = buildSlackThreadPermalink({ const matchedUserAttribution: ResolvedTaskCommitAuthor = { kind: 'user', displayName: 'Jane Doe', + publicDisplayName: null, githubLogin: null, prAssigneeLogin: null, gitAuthor: { diff --git a/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts index e4b3ef6ab..6a85d5524 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts @@ -24,6 +24,7 @@ const exactSlackPermalink = const matchedSlackAttribution: ResolvedTaskCommitAuthor = { kind: 'user', displayName: 'Jane Doe', + publicDisplayName: null, githubLogin: null, prAssigneeLogin: null, gitAuthor: { diff --git a/packages/cloud-agents/src/server/workflows/standardTask.ts b/packages/cloud-agents/src/server/workflows/standardTask.ts index f03c9783a..a494b4863 100644 --- a/packages/cloud-agents/src/server/workflows/standardTask.ts +++ b/packages/cloud-agents/src/server/workflows/standardTask.ts @@ -24,6 +24,7 @@ import { buildGitHubMessageInstructions } from '../github-message-instructions'; const DEFAULT_ATTRIBUTION: ResolvedTaskCommitAuthor = { kind: 'roomote', displayName: PRODUCT_NAME, + publicDisplayName: null, githubLogin: null, prAssigneeLogin: null, gitAuthor: { diff --git a/packages/db/src/lib/__tests__/source-control-provider.test.ts b/packages/db/src/lib/__tests__/source-control-provider.test.ts index 3da2ceed5..111613ac7 100644 --- a/packages/db/src/lib/__tests__/source-control-provider.test.ts +++ b/packages/db/src/lib/__tests__/source-control-provider.test.ts @@ -3,6 +3,8 @@ import type { DatabaseOrTransaction } from '../../db'; import { resolveWorkspaceRepositoryProviders, resolveWorkspaceSourceControlProvider, + workspaceAllowsPrivateAttribution, + workspaceUsesOnlySourceControlProvider, } from '../source-control-provider'; const mockWhere = vi.fn(); @@ -11,6 +13,7 @@ let mockRows: Array<{ fullName: string; host: string | null; isActive?: boolean; + private?: boolean; sourceControlProvider: 'github' | 'gitlab' | 'gitea' | 'ado' | 'bitbucket'; }> = []; let mockEnvironmentRepositories: string[] = []; @@ -297,3 +300,95 @@ describe('resolveWorkspaceSourceControlProvider', () => { warn.mockRestore(); }); }); + +describe('workspaceAllowsPrivateAttribution', () => { + beforeEach(() => { + mockRows = []; + mockEnvironmentRepositories = []; + }); + + it('allows account names only when every selected repository is private', async () => { + mockRows = [ + { + fullName: 'octo/api', + host: 'github.com', + private: true, + sourceControlProvider: 'github', + }, + { + fullName: 'octo/web', + host: 'github.com', + private: true, + sourceControlProvider: 'github', + }, + ]; + + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository_set', + repositories: ['octo/api', 'octo/web'], + }), + ).resolves.toBe(true); + }); + + it('uses public-safe attribution for mixed-visibility workspaces', async () => { + mockRows = [ + { + fullName: 'octo/private', + host: 'github.com', + private: true, + sourceControlProvider: 'github', + }, + { + fullName: 'octo/public', + host: 'github.com', + private: false, + sourceControlProvider: 'github', + }, + ]; + + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository_set', + repositories: ['octo/private', 'octo/public'], + }), + ).resolves.toBe(false); + }); + + it('uses public-safe attribution when a repository cannot be resolved', async () => { + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository', + repo: 'octo/missing', + }), + ).resolves.toBe(false); + }); + + it('requires every repository to match before using a provider handle', async () => { + mockRows = [ + { + fullName: 'octo/api', + host: 'github.com', + private: true, + sourceControlProvider: 'github', + }, + { + fullName: 'group/web', + host: 'gitlab.com', + private: false, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + workspaceUsesOnlySourceControlProvider( + dbOrTx, + { + type: 'repository_set', + repositories: ['octo/api', 'group/web'], + }, + 'github', + ), + ).resolves.toBe(false); + }); +}); diff --git a/packages/db/src/lib/source-control-provider.ts b/packages/db/src/lib/source-control-provider.ts index 9a1e22eb1..0a85fe2db 100644 --- a/packages/db/src/lib/source-control-provider.ts +++ b/packages/db/src/lib/source-control-provider.ts @@ -25,9 +25,44 @@ type RepositoryProviderRow = { fullName: string; host: string | null; isActive?: boolean; + private?: boolean; sourceControlProvider: SourceControlProvider; }; +function selectRepositoryRows( + rows: RepositoryProviderRow[], + repositoryOrder: string[], + sourceControlHost?: string, +): RepositoryProviderRow[] | null { + const rowsByFullName = new Map(); + + for (const row of rows) { + const matches = rowsByFullName.get(row.fullName) ?? []; + matches.push(row); + rowsByFullName.set(row.fullName, matches); + } + + const selected: RepositoryProviderRow[] = []; + + for (const fullName of [...new Set(repositoryOrder)]) { + const matches = rowsByFullName.get(fullName) ?? []; + const activeMatches = matches.filter((row) => row.isActive === true); + const candidates = activeMatches.length > 0 ? activeMatches : matches; + const hostMatches = + candidates.length > 1 && sourceControlHost + ? candidates.filter((row) => row.host === sourceControlHost) + : candidates; + + if (hostMatches.length !== 1) { + return null; + } + + selected.push(hostMatches[0]!); + } + + return selected.length > 0 ? selected : null; +} + function toRepositoryProviderMap( rows: RepositoryProviderRow[], repositoryOrder: string[], @@ -177,6 +212,110 @@ export async function resolveWorkspaceRepositoryProviders( } } +async function resolveWorkspaceRepositoryRows( + dbOrTx: DatabaseOrTransaction, + workspace: TaskWorkspace, +): Promise { + if (workspace.type === 'environment') { + const environment = await dbOrTx.query.environments.findFirst({ + where: eq(environments.id, workspace.environmentId), + columns: { config: true }, + }); + if (!environment) { + return null; + } + + const rows = await dbOrTx + .select({ + fullName: repositories.fullName, + host: repositories.host, + isActive: repositories.isActive, + private: repositories.private, + sourceControlProvider: repositories.sourceControlProvider, + }) + .from(environmentRepositoryMappings) + .innerJoin( + repositories, + eq(environmentRepositoryMappings.repositoryId, repositories.id), + ) + .where( + and( + eq( + environmentRepositoryMappings.environmentId, + workspace.environmentId, + ), + eq(repositories.isActive, true), + ), + ); + const selected = selectRepositoryRows( + rows, + environment.config.repositories.map( + (repository) => repository.repository, + ), + ); + return selected; + } + + if (workspace.type === 'all_repositories') { + const rows = await dbOrTx + .select({ + fullName: repositories.fullName, + host: repositories.host, + isActive: repositories.isActive, + private: repositories.private, + sourceControlProvider: repositories.sourceControlProvider, + }) + .from(repositories) + .where(eq(repositories.isActive, true)); + return rows.length > 0 ? rows : null; + } + + const fullNames = + workspace.type === 'repository' ? [workspace.repo] : workspace.repositories; + const rows = await dbOrTx + .select({ + fullName: repositories.fullName, + host: repositories.host, + isActive: repositories.isActive, + private: repositories.private, + sourceControlProvider: repositories.sourceControlProvider, + }) + .from(repositories) + .where(inArray(repositories.fullName, fullNames)); + const selected = selectRepositoryRows( + rows, + fullNames, + workspace.sourceControlHost, + ); + return selected; +} + +/** + * Whether every repository in a workspace is known private. Missing or + * ambiguous repository rows return false so attribution fails toward privacy. + */ +export async function workspaceAllowsPrivateAttribution( + dbOrTx: DatabaseOrTransaction, + workspace: TaskWorkspace, +): Promise { + const rows = await resolveWorkspaceRepositoryRows(dbOrTx, workspace); + return rows?.every((repository) => repository.private === true) ?? false; +} + +/** Whether every known repository can use a handle from the same provider. */ +export async function workspaceUsesOnlySourceControlProvider( + dbOrTx: DatabaseOrTransaction, + workspace: TaskWorkspace, + provider: SourceControlProvider, +): Promise { + const rows = await resolveWorkspaceRepositoryRows(dbOrTx, workspace); + return ( + rows?.every( + (repository) => repository.sourceControlProvider === provider, + ) ?? false + ); +} + /** * Resolve the single source-control provider a launch's workspace belongs to, * so the task payload can carry an explicit `sourceControlProvider`. Handles diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index 765008932..6a6d4f724 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts @@ -173,10 +173,15 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { mockGetDeploymentGitHubRoomoteMentionEnabled.mockResolvedValue(true); mockResolveConfiguredGitHubAppSlugIfConfigured.mockResolvedValue(null); mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'roomote', displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockResolveLaunchTaskCommitAuthor.mockResolvedValue({ + kind: 'roomote', + displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockGetDeploymentPrAction.mockResolvedValue('draft'); @@ -199,6 +204,13 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { externalRepoId: '101', fullName: 'acme/backend', htmlUrl: 'https://gitlab.com/acme/backend', + private: false, + }); + mockResolveLaunchTaskCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@github-login', + prAssigneeLogin: null, }); const fetchImpl = vi .fn() @@ -225,7 +237,7 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { sourceBranch: 'codex/provider-neutral', targetBranch: 'develop', title: '[Feature] Provider neutral PRs', - body: 'Body', + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', labels: ['roomote'], assignees: [], }, @@ -264,7 +276,8 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { target_branch: 'develop', remove_source_branch: false, title: '[Feature] Provider neutral PRs', - description: 'Body', + description: + '> Created by Roomote. Follow up by mentioning @roomote.', labels: 'roomote', }), }), @@ -342,10 +355,15 @@ describe('platform-managed draft state', () => { vi.clearAllMocks(); mockResolveConfiguredGitHubAppSlugIfConfigured.mockResolvedValue(null); mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'roomote', displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockResolveLaunchTaskCommitAuthor.mockResolvedValue({ + kind: 'roomote', + displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockGetDeploymentPrAction.mockResolvedValue('draft'); @@ -388,6 +406,7 @@ describe('platform-managed draft state', () => { externalRepoId: null, fullName: 'acme/web', htmlUrl: 'https://github.com/acme/web', + private: true, }); return octokit; } @@ -705,10 +724,15 @@ describe('optional targetBranch', () => { vi.clearAllMocks(); mockResolveConfiguredGitHubAppSlugIfConfigured.mockResolvedValue(null); mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'roomote', displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockResolveLaunchTaskCommitAuthor.mockResolvedValue({ + kind: 'roomote', + displayName: 'Roomote', + publicDisplayName: null, prAssigneeLogin: null, }); mockGetDeploymentPrAction.mockResolvedValue('draft'); @@ -766,6 +790,7 @@ describe('optional targetBranch', () => { externalRepoId: null, fullName: 'acme/web', htmlUrl: 'https://github.com/acme/web', + private: true, }); return octokit; } @@ -948,6 +973,90 @@ describe('optional targetBranch', () => { ); }); + it('uses only the linked handle in a public GitHub pull request body', async () => { + const octokit = makeOctokit({ + list: [], + created: { + number: 13, + node_id: 'node-13', + html_url: 'https://github.com/acme/web/pull/13', + title: '[Feature] X', + draft: true, + base: { ref: 'develop' }, + }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + targetBranch: 'develop', + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + }), + ); + }); + + it('uses generic provenance in a public GitHub pull request without a linked handle', async () => { + const octokit = makeOctokit({ + list: [], + created: { + number: 13, + node_id: 'node-13', + html_url: 'https://github.com/acme/web/pull/13', + title: '[Feature] X', + draft: true, + base: { ref: 'develop' }, + }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: null, + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + targetBranch: 'develop', + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Created by Roomote. Follow up by mentioning @roomote.', + }), + ); + }); + it('preserves the original opener line when updating a pull request', async () => { const existing = { number: 11, @@ -982,6 +1091,49 @@ describe('optional targetBranch', () => { ); }); + it('does not preserve a private display name when a public pull request is updated', async () => { + const existing = { + number: 11, + node_id: 'node-11', + html_url: 'https://github.com/acme/web/pull/11', + title: 'Old title', + draft: false, + base: { ref: 'develop' }, + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }; + const octokit = makeOctokit({ + list: [existing], + updated: { ...existing, title: '[Feature] X' }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + }), + ); + }); + it('removes the stale launch-owner assignment when updating a pull request', async () => { const existing = { number: 11, diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts index 3d17ee81a..7be5d0368 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts @@ -37,6 +37,7 @@ export type RepositoryRow = { externalRepoId: string | null; fullName: string; htmlUrl: string; + private?: boolean; }; export function resolveSourceControlProviderForRepositoryFromPayload( @@ -130,6 +131,7 @@ export async function resolveRepositoryRow({ externalRepoId: true, fullName: true, htmlUrl: true, + private: true, }, }); diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts index 09f16d51b..f5f29a596 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts @@ -21,6 +21,7 @@ import { buildPullRequestUrl, getSourceControlProviderLabel, normalizePrBodyAttributionAppMention, + rewritePrBodyAttribution, prActions, sourceControlProviderSchema, type PrAction, @@ -218,22 +219,32 @@ export async function createOrUpdateSourceControlPullRequestForTaskRun({ resolveConfiguredGitHubAppSlugIfConfigured(), getDeploymentGitHubRoomoteMentionEnabled(), ]); - const inputWithNormalizedAttribution: SourceControlPullRequestMutationInput = - configuredGitHubAppSlug - ? { - ...input, - body: normalizePrBodyAttributionAppMention( - input.body, - configuredGitHubAppSlug, - roomoteMentionEnabled, - ), - } - : input; - - const liveGitHubAttribution = + const attribution = provider === 'github' ? await resolveRunCommitAuthor(db, taskRun) - : undefined; + : await resolveLaunchTaskCommitAuthor(db, taskRun.taskId); + const normalizedMentionBody = configuredGitHubAppSlug + ? normalizePrBodyAttributionAppMention( + input.body, + configuredGitHubAppSlug, + roomoteMentionEnabled, + ) + : input.body; + const displayName = + attribution.kind === 'roomote' + ? null + : repository.private === true + ? attribution.displayName + : provider === 'github' + ? attribution.publicDisplayName + : null; + const inputWithNormalizedAttribution: SourceControlPullRequestMutationInput = + { + ...input, + body: rewritePrBodyAttribution(normalizedMentionBody, displayName), + }; + + const liveGitHubAttribution = provider === 'github' ? attribution : undefined; const liveGitHubAssigneePlan = liveGitHubAttribution ? await resolveLiveGitHubAssigneePlan({ taskRun, @@ -256,7 +267,6 @@ export async function createOrUpdateSourceControlPullRequestForTaskRun({ repository, provider, createDraft, - attribution: liveGitHubAttribution, staleLaunchAssignee: liveGitHubAssigneePlan?.staleLaunchAssignee, }); case 'gitlab': @@ -415,14 +425,12 @@ async function createOrUpdateGitHubPullRequest({ repository, provider, createDraft, - attribution, staleLaunchAssignee, }: { input: SourceControlPullRequestMutationInput; repository: RepositoryRow; provider: 'github'; createDraft: boolean; - attribution?: ResolvedTaskCommitAuthor; staleLaunchAssignee?: string; }): Promise { if (!repository.installationId) { @@ -474,6 +482,7 @@ async function createOrUpdateGitHubPullRequest({ body: preserveExistingPullRequestAttribution( input.body, pullRequest.body, + repository.private === true, ), }); pullRequest = data; @@ -484,7 +493,7 @@ async function createOrUpdateGitHubPullRequest({ owner, repo, title: input.title, - body: replaceCreatedPullRequestAttribution(input.body, attribution), + body: input.body, head: input.sourceBranch, base: targetBranch, draft: createDraft, @@ -541,27 +550,20 @@ async function createOrUpdateGitHubPullRequest({ }; } -function replaceCreatedPullRequestAttribution( - body: string, - attribution: ResolvedTaskCommitAuthor | undefined, -): string { - if (!attribution) { - return body; - } - - return body.replace( - /^(> Opened on behalf of ).+?(\. (?:Follow up by|\[View the task\]))/mu, - `$1${attribution.displayName}$2`, - ); -} - function preserveExistingPullRequestAttribution( body: string, existingBody: string | null | undefined, + repositoryIsPrivate: boolean, ): string { const openerLine = existingBody?.match(/^> Opened on behalf of .+$/mu)?.[0]; + const safePublicOpener = openerLine?.startsWith('> Opened on behalf of @'); return openerLine - ? body.replace(/^> Opened on behalf of .+$/mu, openerLine) + ? repositoryIsPrivate || safePublicOpener + ? body.replace( + /^(?:> Opened on behalf of .+|> Created by Roomote\..*)$/mu, + openerLine, + ) + : body : body; } diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/resolve-git-author.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/resolve-git-author.test.ts index c431ce6e2..28f4e75e1 100644 --- a/packages/sdk/src/server/lib/task-runs/__tests__/resolve-git-author.test.ts +++ b/packages/sdk/src/server/lib/task-runs/__tests__/resolve-git-author.test.ts @@ -1,10 +1,12 @@ import { db, githubUserMappings, + repositoryFactory, taskFactory, + type TaskRun, userFactory, } from '@roomote/db/server'; -import { PRODUCT_NAME } from '@roomote/types'; +import { ALL_REPOSITORIES, PRODUCT_NAME } from '@roomote/types'; import { resolveGitAuthor } from '../dequeue-helpers'; @@ -14,6 +16,22 @@ function uniqueGitHubUserId(): number { return githubUserIdSeed; } +function runContext( + taskId: string, + actingUserId: string | null, + repo = 'Roomote/example-app', +) { + return { + id: 1, + taskId, + actingUserId, + payload: { + repo, + sourceControlProvider: 'github', + } as TaskRun['payload'], + }; +} + /** * resolveGitAuthor resolves a linked live acting user and falls back to * Roomote when a run has no current actor. @@ -23,7 +41,7 @@ describe('resolveGitAuthor', () => { const task = await taskFactory.create({}); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: null }), + resolveGitAuthor(tx, runContext(task.id, null)), ); expect(result).toEqual({ @@ -38,7 +56,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: null }), + resolveGitAuthor(tx, runContext(task.id, null)), ); expect(result).toEqual({ @@ -47,7 +65,7 @@ describe('resolveGitAuthor', () => { }); }); - it('resolves a user commit author to their noreply email via the GitHub mapping', async () => { + it('uses the linked handle for an unknown-visibility GitHub workspace', async () => { const user = await userFactory.create({ name: 'Mona Lisa' }); const githubUserId = uniqueGitHubUserId(); @@ -64,7 +82,43 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: user.id }), + resolveGitAuthor(tx, runContext(task.id, user.id)), + ); + + expect(result).toEqual({ + name: '@octocat', + email: `${githubUserId}+octocat@users.noreply.github.com`, + }); + }); + + it('keeps the account name for a known private workspace', async () => { + const user = await userFactory.create({ name: 'Mona Lisa' }); + const githubUserId = uniqueGitHubUserId(); + const repository = await repositoryFactory.create({ + fullName: `octo/private-${githubUserId}`, + linkedByUserId: user.id, + private: true, + sourceControlProvider: 'gitlab', + }); + await db.insert(githubUserMappings).values({ + userId: user.id, + githubLogin: 'octocat', + githubUserId, + }); + const task = await taskFactory.create({ + initiatorUserId: user.id, + commitAuthorKind: 'user', + commitAuthorUserId: user.id, + }); + + const result = await db.transaction(async (tx) => + resolveGitAuthor(tx, { + ...runContext(task.id, user.id, repository.fullName), + payload: { + repo: repository.fullName, + sourceControlProvider: 'gitlab', + } as TaskRun['payload'], + }), ); expect(result).toEqual({ @@ -73,6 +127,52 @@ describe('resolveGitAuthor', () => { }); }); + it('uses Roomote for a public mixed-provider workspace', async () => { + const user = await userFactory.create({ name: 'Mona Lisa' }); + const githubUserId = uniqueGitHubUserId(); + const privateRepository = await repositoryFactory.create({ + fullName: `group/private-${githubUserId}`, + linkedByUserId: user.id, + private: true, + sourceControlProvider: 'gitea', + }); + const publicRepository = await repositoryFactory.create({ + fullName: `group/public-${githubUserId}`, + linkedByUserId: user.id, + private: false, + sourceControlProvider: 'gitlab', + }); + await db.insert(githubUserMappings).values({ + userId: user.id, + githubLogin: 'octocat', + githubUserId, + }); + const task = await taskFactory.create({ + initiatorUserId: user.id, + commitAuthorKind: 'user', + commitAuthorUserId: user.id, + }); + + const result = await db.transaction(async (tx) => + resolveGitAuthor(tx, { + ...runContext(task.id, user.id), + payload: { + repo: ALL_REPOSITORIES, + selectedRepositories: [ + privateRepository.fullName, + publicRepository.fullName, + ], + sourceControlProvider: 'github', + } as TaskRun['payload'], + }), + ); + + expect(result).toEqual({ + name: PRODUCT_NAME, + email: 'roomote@roomote.dev', + }); + }); + it('falls back to Roomote when the user commit author has no GitHub mapping', async () => { const user = await userFactory.create({ name: 'Unmapped User' }); @@ -83,7 +183,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: user.id }), + resolveGitAuthor(tx, runContext(task.id, user.id)), ); expect(result).toEqual({ @@ -101,7 +201,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: null }), + resolveGitAuthor(tx, runContext(task.id, null)), ); expect(result).toEqual({ @@ -118,7 +218,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: null }), + resolveGitAuthor(tx, runContext(task.id, null)), ); expect(result).toEqual({ @@ -135,7 +235,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { id: 1, taskId: task.id, actingUserId: null }), + resolveGitAuthor(tx, runContext(task.id, null)), ); expect(result).toEqual({ @@ -146,11 +246,7 @@ describe('resolveGitAuthor', () => { it('does not require a task lookup when the run has no acting user', async () => { const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { - id: 1, - taskId: 'missing-task-id', - actingUserId: null, - }), + resolveGitAuthor(tx, runContext('missing-task-id', null)), ); expect(result).toEqual({ @@ -174,15 +270,11 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { - id: 1, - taskId: task.id, - actingUserId: participant.id, - }), + resolveGitAuthor(tx, runContext(task.id, participant.id)), ); expect(result).toEqual({ - name: 'Participant', + name: '@participant', email: `${githubUserId}+participant@users.noreply.github.com`, }); }); @@ -198,11 +290,7 @@ describe('resolveGitAuthor', () => { }); const result = await db.transaction(async (tx) => - resolveGitAuthor(tx, { - id: 1, - taskId: task.id, - actingUserId: participant.id, - }), + resolveGitAuthor(tx, runContext(task.id, participant.id)), ); expect(result).toEqual({ diff --git a/packages/sdk/src/server/lib/task-runs/dequeue-helpers.ts b/packages/sdk/src/server/lib/task-runs/dequeue-helpers.ts index 4a027dc80..bd8346894 100644 --- a/packages/sdk/src/server/lib/task-runs/dequeue-helpers.ts +++ b/packages/sdk/src/server/lib/task-runs/dequeue-helpers.ts @@ -22,6 +22,8 @@ import { markTaskStartParallelCountEndedAt, resolveSandboxModelRuntimeEnv, resolveWorkspaceSourceControlProvider, + workspaceAllowsPrivateAttribution, + workspaceUsesOnlySourceControlProvider, stringifyDecryptedEnvVarValue, syncTaskStateFromRuns, eq, @@ -35,7 +37,9 @@ import { createTaskRunBitbucketCredentials } from '@roomote/bitbucket'; import { createTaskRunGiteaCredentials } from '@roomote/gitea'; import { createTaskRunAdoCredentials } from '@roomote/ado'; import { + DEFAULT_ROOMOTE_COMMIT_AUTHOR, releaseTaskRun, + resolvePublicGitAuthor, resolveRunCommitAuthor, } from '@roomote/cloud-agents/server'; @@ -758,9 +762,32 @@ export function reportBootstrapFailure({ export async function resolveGitAuthor( tx: DbTx, - taskRun: Pick, + taskRun: Pick, ): Promise { const commitAuthor = await resolveRunCommitAuthor(tx, taskRun); - return commitAuthor.gitAuthor; + if (commitAuthor.kind === 'roomote') { + return commitAuthor.gitAuthor; + } + + const workspace = resolveTaskWorkspace(taskRun.payload); + if (await workspaceAllowsPrivateAttribution(tx, workspace)) { + return commitAuthor.gitAuthor; + } + + const usesOnlyGitHub = await workspaceUsesOnlySourceControlProvider( + tx, + workspace, + 'github', + ); + const provider = await resolveWorkspaceSourceControlProvider(tx, workspace); + const singleUnknownGitHubRepository = + workspace.type === 'repository' && + !provider && + resolveSourceControlProviderFromPayload(taskRun.payload) === 'github'; + if (!usesOnlyGitHub && !singleUnknownGitHubRepository) { + return DEFAULT_ROOMOTE_COMMIT_AUTHOR.gitAuthor; + } + + return resolvePublicGitAuthor(commitAuthor); } diff --git a/packages/types/src/__tests__/github-bot-identity.test.ts b/packages/types/src/__tests__/github-bot-identity.test.ts index 88457d08e..43abcfc08 100644 --- a/packages/types/src/__tests__/github-bot-identity.test.ts +++ b/packages/types/src/__tests__/github-bot-identity.test.ts @@ -3,6 +3,7 @@ import { getRoomoteManagedGitHubLogins, matchesRoomoteGitHubLogin, normalizePrBodyAttributionAppMention, + rewritePrBodyAttribution, } from '../constants'; describe('Roomote GitHub bot identity helpers', () => { @@ -121,4 +122,21 @@ describe('Roomote GitHub bot identity helpers', () => { ); }); }); + + describe('rewritePrBodyAttribution', () => { + const body = + '> Opened on behalf of Private Name. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.'; + + it('uses a public handle without changing the attribution tail', () => { + expect(rewritePrBodyAttribution(body, '@octocat')).toBe( + '> Opened on behalf of @octocat. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.', + ); + }); + + it('uses generic Roomote provenance when no public identity exists', () => { + expect(rewritePrBodyAttribution(body, null)).toBe( + '> Created by Roomote. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.', + ); + }); + }); }); diff --git a/packages/types/src/constants.ts b/packages/types/src/constants.ts index eae431b5f..e72daecf3 100644 --- a/packages/types/src/constants.ts +++ b/packages/types/src/constants.ts @@ -222,6 +222,31 @@ export function normalizePrBodyAttributionAppMention( return `${prefix}${rewrittenInstruction}${remainder}`; } +/** + * Rewrite the leading Roomote PR provenance sentence while preserving its + * task/conversation links and follow-up instructions. + */ +export function rewritePrBodyAttribution( + body: string, + displayName: string | null, +): string { + const firstNewline = body.indexOf('\n'); + const firstLine = firstNewline === -1 ? body : body.slice(0, firstNewline); + const remainder = firstNewline === -1 ? '' : body.slice(firstNewline); + const match = matchPrBodyAttributionLine(firstLine); + + if (!match) { + return body; + } + + const normalizedDisplayName = displayName?.trim().replace(/[\r\n]+/g, ' '); + const provenance = normalizedDisplayName + ? `> Opened on behalf of ${normalizedDisplayName}. ` + : '> Created by Roomote. '; + + return `${provenance}${match.instruction}${remainder}`; +} + /** * Hosted-product GitHub App slugs Roomote always treats as its own bots. * Custom deployments still add their configured slug via helpers below. From 7ee4d8a83773b7f4d693af0ad57210e027a77d3c Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 16:38:05 +0000 Subject: [PATCH 2/8] fix: validate public attribution identities --- .../__tests__/source-control-provider.test.ts | 37 ++++++++ .../db/src/lib/source-control-provider.ts | 14 ++- .../source-control-pull-requests.test.ts | 89 +++++++++++++++++++ .../source-control-pull-requests.ts | 7 +- 4 files changed, 138 insertions(+), 9 deletions(-) diff --git a/packages/db/src/lib/__tests__/source-control-provider.test.ts b/packages/db/src/lib/__tests__/source-control-provider.test.ts index 111613ac7..9ced11b53 100644 --- a/packages/db/src/lib/__tests__/source-control-provider.test.ts +++ b/packages/db/src/lib/__tests__/source-control-provider.test.ts @@ -231,6 +231,24 @@ describe('resolveWorkspaceSourceControlProvider', () => { ).resolves.toEqual({ 'group/project': 'gitea' }); }); + it('rejects a single repository row from a different host', async () => { + mockRows = [ + { + fullName: 'group/project', + host: 'gitlab.example.com', + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + resolveWorkspaceRepositoryProviders(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'git.example.com', + }), + ).resolves.toEqual({}); + }); + it('prefers active rows over stale inactive rows with the same name', async () => { mockRows = [ { @@ -364,6 +382,25 @@ describe('workspaceAllowsPrivateAttribution', () => { ).resolves.toBe(false); }); + it('uses public-safe attribution when the selected host does not match', async () => { + mockRows = [ + { + fullName: 'group/project', + host: 'gitlab.example.com', + private: true, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'git.example.com', + }), + ).resolves.toBe(false); + }); + it('requires every repository to match before using a provider handle', async () => { mockRows = [ { diff --git a/packages/db/src/lib/source-control-provider.ts b/packages/db/src/lib/source-control-provider.ts index 0a85fe2db..9ad344892 100644 --- a/packages/db/src/lib/source-control-provider.ts +++ b/packages/db/src/lib/source-control-provider.ts @@ -48,10 +48,9 @@ function selectRepositoryRows( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const hostMatches = - candidates.length > 1 && sourceControlHost - ? candidates.filter((row) => row.host === sourceControlHost) - : candidates; + const hostMatches = sourceControlHost + ? candidates.filter((row) => row.host === sourceControlHost) + : candidates; if (hostMatches.length !== 1) { return null; @@ -82,10 +81,9 @@ function toRepositoryProviderMap( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const hostMatches = - candidates.length > 1 && sourceControlHost - ? candidates.filter((row) => row.host === sourceControlHost) - : candidates; + const hostMatches = sourceControlHost + ? candidates.filter((row) => row.host === sourceControlHost) + : candidates; if (candidates.length > 1 && hostMatches.length !== 1) { console.warn( diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index 6a6d4f724..3e8c9ae59 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts @@ -1134,6 +1134,95 @@ describe('optional targetBranch', () => { ); }); + it.each(['@Jane Doe', '@jane@example.com'])( + 'does not preserve invalid legacy public attribution %s', + async (legacyAttribution) => { + const existing = { + number: 11, + node_id: 'node-11', + html_url: 'https://github.com/acme/web/pull/11', + title: 'Old title', + draft: false, + base: { ref: 'develop' }, + body: `> Opened on behalf of ${legacyAttribution}. Follow up by mentioning @roomote.`, + }; + const octokit = makeOctokit({ + list: [existing], + updated: { ...existing, title: '[Feature] X' }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + }), + ); + }, + ); + + it('preserves a valid handle in existing public attribution', async () => { + const existing = { + number: 11, + node_id: 'node-11', + html_url: 'https://github.com/acme/web/pull/11', + title: 'Old title', + draft: false, + base: { ref: 'develop' }, + body: '> Opened on behalf of @launch-owner. Follow up by mentioning @roomote.', + }; + const octokit = makeOctokit({ + list: [existing], + updated: { ...existing, title: '[Feature] X' }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Opened on behalf of @launch-owner. Follow up by mentioning @roomote.', + }), + ); + }); + it('removes the stale launch-owner assignment when updating a pull request', async () => { const existing = { number: 11, diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts index f5f29a596..84bc38410 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts @@ -556,7 +556,12 @@ function preserveExistingPullRequestAttribution( repositoryIsPrivate: boolean, ): string { const openerLine = existingBody?.match(/^> Opened on behalf of .+$/mu)?.[0]; - const safePublicOpener = openerLine?.startsWith('> Opened on behalf of @'); + const publicHandle = openerLine?.match( + /^> Opened on behalf of @([^\s.]+)\. /u, + )?.[1]; + const safePublicOpener = + publicHandle !== undefined && + /^[A-Za-z0-9](?:[A-Za-z0-9]|-(?=[A-Za-z0-9])){0,38}$/u.test(publicHandle); return openerLine ? repositoryIsPrivate || safePublicOpener ? body.replace( From 9bbb03bf939696435a7ceac8f03ccd5b57e6b864 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 16:46:28 +0000 Subject: [PATCH 3/8] fix: rebuild preserved public attribution --- .../source-control-pull-requests.test.ts | 43 +++++++++++++++++++ .../source-control-pull-requests.ts | 20 ++++++--- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index 3e8c9ae59..d436915a6 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts @@ -1223,6 +1223,49 @@ describe('optional targetBranch', () => { ); }); + it('discards trailing legacy text after a valid public handle', async () => { + const existing = { + number: 11, + node_id: 'node-11', + html_url: 'https://github.com/acme/web/pull/11', + title: 'Old title', + draft: false, + base: { ref: 'develop' }, + body: '> Opened on behalf of @octocat. Private Name. Follow up by mentioning @roomote.', + }; + const octokit = makeOctokit({ + list: [existing], + updated: { ...existing, title: '[Feature] X' }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: '> Opened on behalf of @octocat. Follow up by mentioning @roomote.', + }), + ); + }); + it('removes the stale launch-owner assignment when updating a pull request', async () => { const existing = { number: 11, diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts index 84bc38410..40cc8bf7b 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts @@ -562,13 +562,19 @@ function preserveExistingPullRequestAttribution( const safePublicOpener = publicHandle !== undefined && /^[A-Za-z0-9](?:[A-Za-z0-9]|-(?=[A-Za-z0-9])){0,38}$/u.test(publicHandle); - return openerLine - ? repositoryIsPrivate || safePublicOpener - ? body.replace( - /^(?:> Opened on behalf of .+|> Created by Roomote\..*)$/mu, - openerLine, - ) - : body + if (!openerLine) { + return body; + } + + if (repositoryIsPrivate) { + return body.replace( + /^(?:> Opened on behalf of .+|> Created by Roomote\..*)$/mu, + openerLine, + ); + } + + return safePublicOpener + ? rewritePrBodyAttribution(body, `@${publicHandle}`) : body; } From 6defb75377fd4cb9b359fffaee3f98eaa88b8626 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:44:03 +0000 Subject: [PATCH 4/8] refactor: delimit public attribution metadata --- .../server/__tests__/commit-author.test.ts | 41 +++++ .../cloud-agents/src/server/commit-author.ts | 3 +- .../workflows/__tests__/utilsAppSlug.test.ts | 6 + .../src/server/workflows/utils.ts | 8 +- .../__tests__/source-control-provider.test.ts | 66 +++++++ .../db/src/lib/source-control-provider.ts | 14 +- .../source-control-pull-requests.test.ts | 168 +++++++++-------- .../source-control-pull-requests.ts | 40 +++- .../src/__tests__/github-bot-identity.test.ts | 90 +++++++-- packages/types/src/constants.ts | 171 +++++++----------- 10 files changed, 402 insertions(+), 205 deletions(-) create mode 100644 packages/cloud-agents/src/server/__tests__/commit-author.test.ts diff --git a/packages/cloud-agents/src/server/__tests__/commit-author.test.ts b/packages/cloud-agents/src/server/__tests__/commit-author.test.ts new file mode 100644 index 000000000..c79ef3787 --- /dev/null +++ b/packages/cloud-agents/src/server/__tests__/commit-author.test.ts @@ -0,0 +1,41 @@ +import { + DEFAULT_ROOMOTE_COMMIT_AUTHOR, + resolvePublicGitAuthor, + type ResolvedTaskCommitAuthor, +} from '../commit-author'; + +describe('resolvePublicGitAuthor', () => { + it('does not combine an unverified handle with the Roomote email', () => { + const attribution: ResolvedTaskCommitAuthor = { + kind: 'external', + displayName: 'Private Name', + publicDisplayName: '@octocat', + githubLogin: 'octocat', + prAssigneeLogin: null, + gitAuthor: DEFAULT_ROOMOTE_COMMIT_AUTHOR.gitAuthor, + }; + + expect(resolvePublicGitAuthor(attribution)).toEqual( + DEFAULT_ROOMOTE_COMMIT_AUTHOR.gitAuthor, + ); + }); + + it('uses the handle with a verified noreply identity', () => { + const attribution: ResolvedTaskCommitAuthor = { + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@octocat', + githubLogin: 'octocat', + prAssigneeLogin: 'octocat', + gitAuthor: { + name: 'Private Name', + email: '123+octocat@users.noreply.github.com', + }, + }; + + expect(resolvePublicGitAuthor(attribution)).toEqual({ + name: '@octocat', + email: '123+octocat@users.noreply.github.com', + }); + }); +}); diff --git a/packages/cloud-agents/src/server/commit-author.ts b/packages/cloud-agents/src/server/commit-author.ts index 7ef75e84e..74e455360 100644 --- a/packages/cloud-agents/src/server/commit-author.ts +++ b/packages/cloud-agents/src/server/commit-author.ts @@ -295,7 +295,8 @@ export async function resolveTaskCommitAuthor( export function resolvePublicGitAuthor( attribution: ResolvedTaskCommitAuthor, ): ResolvedGitAuthor { - return attribution.publicDisplayName + return attribution.publicDisplayName && + attribution.gitAuthor.email !== ROOMOTE_GIT_AUTHOR.email ? { ...attribution.gitAuthor, name: attribution.publicDisplayName } : ROOMOTE_GIT_AUTHOR; } diff --git a/packages/cloud-agents/src/server/workflows/__tests__/utilsAppSlug.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/utilsAppSlug.test.ts index e0daa161a..62a6aa2d4 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/utilsAppSlug.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/utilsAppSlug.test.ts @@ -16,6 +16,10 @@ import { setGitHubRoomoteMentionSettingCache, type Schemas, } from '@roomote/github'; +import { + PR_BODY_ATTRIBUTION_END_MARKER, + PR_BODY_ATTRIBUTION_START_MARKER, +} from '@roomote/types'; import { DEFAULT_ROOMOTE_COMMIT_AUTHOR } from '../../commit-author'; import { @@ -73,6 +77,8 @@ describe('getPrBodyAttributionLine', () => { expect(line).toContain('@roomote'); expect(line).not.toContain('@octomote'); + expect(line).toContain(PR_BODY_ATTRIBUTION_START_MARKER); + expect(line).toContain(PR_BODY_ATTRIBUTION_END_MARKER); }); it('mentions @roomote with a database-configured app slug', () => { diff --git a/packages/cloud-agents/src/server/workflows/utils.ts b/packages/cloud-agents/src/server/workflows/utils.ts index fae10326d..8e9bf2000 100644 --- a/packages/cloud-agents/src/server/workflows/utils.ts +++ b/packages/cloud-agents/src/server/workflows/utils.ts @@ -6,6 +6,7 @@ import { buildTelegramMessagePermalink, buildDiscordMessagePermalink, getGitHubFollowUpMention, + formatPrBodyAttribution, resolveTaskWorkspace, } from '@roomote/types'; import { @@ -273,12 +274,15 @@ function buildPrBodyAttributionLine({ : defaultFollowUpInstruction; if (attribution.kind === 'roomote') { - return `> Created by Roomote. ${instruction}`; + return formatPrBodyAttribution('Created by Roomote.', instruction); } const safeUserName = escapeValue(attribution.displayName || PRODUCT_NAME); - return `> Opened on behalf of ${safeUserName}. ${instruction}`; + return formatPrBodyAttribution( + `Opened on behalf of ${safeUserName}.`, + instruction, + ); } export function getWorkspaceInstructions( repoFullNames?: string[], diff --git a/packages/db/src/lib/__tests__/source-control-provider.test.ts b/packages/db/src/lib/__tests__/source-control-provider.test.ts index 9ced11b53..20d333dec 100644 --- a/packages/db/src/lib/__tests__/source-control-provider.test.ts +++ b/packages/db/src/lib/__tests__/source-control-provider.test.ts @@ -249,6 +249,47 @@ describe('resolveWorkspaceSourceControlProvider', () => { ).resolves.toEqual({}); }); + it('falls back to a legacy null-host repository row', async () => { + mockRows = [ + { + fullName: 'group/project', + host: null, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + resolveWorkspaceRepositoryProviders(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'gitlab.example.com', + }), + ).resolves.toEqual({ 'group/project': 'gitlab' }); + }); + + it('prefers an exact host match over a legacy null-host row', async () => { + mockRows = [ + { + fullName: 'group/project', + host: null, + sourceControlProvider: 'github', + }, + { + fullName: 'group/project', + host: 'gitlab.example.com', + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + resolveWorkspaceRepositoryProviders(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'gitlab.example.com', + }), + ).resolves.toEqual({ 'group/project': 'gitlab' }); + }); + it('prefers active rows over stale inactive rows with the same name', async () => { mockRows = [ { @@ -401,6 +442,31 @@ describe('workspaceAllowsPrivateAttribution', () => { ).resolves.toBe(false); }); + it('uses legacy null-host visibility only when no exact host exists', async () => { + mockRows = [ + { + fullName: 'group/project', + host: null, + private: true, + sourceControlProvider: 'gitlab', + }, + { + fullName: 'group/project', + host: 'gitlab.example.com', + private: false, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'gitlab.example.com', + }), + ).resolves.toBe(false); + }); + it('requires every repository to match before using a provider handle', async () => { mockRows = [ { diff --git a/packages/db/src/lib/source-control-provider.ts b/packages/db/src/lib/source-control-provider.ts index 9ad344892..8a7e47c6d 100644 --- a/packages/db/src/lib/source-control-provider.ts +++ b/packages/db/src/lib/source-control-provider.ts @@ -48,8 +48,13 @@ function selectRepositoryRows( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const hostMatches = sourceControlHost + const exactHostMatches = sourceControlHost ? candidates.filter((row) => row.host === sourceControlHost) + : []; + const hostMatches = sourceControlHost + ? exactHostMatches.length > 0 + ? exactHostMatches + : candidates.filter((row) => row.host === null) : candidates; if (hostMatches.length !== 1) { @@ -81,8 +86,13 @@ function toRepositoryProviderMap( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const hostMatches = sourceControlHost + const exactHostMatches = sourceControlHost ? candidates.filter((row) => row.host === sourceControlHost) + : []; + const hostMatches = sourceControlHost + ? exactHostMatches.length > 0 + ? exactHostMatches + : candidates.filter((row) => row.host === null) : candidates; if (candidates.length > 1 && hostMatches.length !== 1) { diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index d436915a6..ab38e5b3c 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts @@ -1,6 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { RunStatus, TaskPayloadKind } from '@roomote/types'; +import { + RunStatus, + TaskPayloadKind, + formatPrBodyAttribution, +} from '@roomote/types'; import type { TaskRun } from '@roomote/db/server'; const { @@ -167,6 +171,13 @@ function jsonResponse(body: unknown, status = 200): Response { }); } +function attributionBody( + provenance: string, + instruction = 'Follow up by mentioning @roomote.', +): string { + return formatPrBodyAttribution(provenance, instruction); +} + describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { beforeEach(() => { vi.clearAllMocks(); @@ -237,7 +248,7 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { sourceBranch: 'codex/provider-neutral', targetBranch: 'develop', title: '[Feature] Provider neutral PRs', - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), labels: ['roomote'], assignees: [], }, @@ -276,8 +287,7 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { target_branch: 'develop', remove_source_branch: false, title: '[Feature] Provider neutral PRs', - description: - '> Created by Roomote. Follow up by mentioning @roomote.', + description: attributionBody('Created by Roomote.'), labels: 'roomote', }), }), @@ -465,13 +475,19 @@ describe('platform-managed draft state', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...githubInput, - body: '> Created by Roomote. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).\n\n## What changed\n\nDone.', + body: `${attributionBody( + 'Created by Roomote.', + 'Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).', + )}\n\n## What changed\n\nDone.`, }, }); expect(octokit.rest.pulls.create).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Created by Roomote. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).\n\n## What changed\n\nDone.', + body: `${attributionBody( + 'Created by Roomote.', + 'Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).', + )}\n\n## What changed\n\nDone.`, }), ); }); @@ -495,13 +511,19 @@ describe('platform-managed draft state', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...githubInput, - body: '> Created by Roomote. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).', + body: attributionBody( + 'Created by Roomote.', + 'Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).', + ), }, }); expect(octokit.rest.pulls.create).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Created by Roomote. Follow up by mentioning @roomote-roomote or in [the web UI](https://example.com/task/1).', + body: attributionBody( + 'Created by Roomote.', + 'Follow up by mentioning @roomote-roomote or in [the web UI](https://example.com/task/1).', + ), }), ); }); @@ -962,13 +984,13 @@ describe('optional targetBranch', () => { input: { ...baseInput, targetBranch: 'develop', - body: '> Opened on behalf of Launch Owner. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Launch Owner.'), }, }); expect(octokit.rest.pulls.create).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of Participant. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Participant.'), }), ); }); @@ -1004,13 +1026,13 @@ describe('optional targetBranch', () => { input: { ...baseInput, targetBranch: 'develop', - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), }, }); expect(octokit.rest.pulls.create).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @participant.'), }), ); }); @@ -1046,18 +1068,60 @@ describe('optional targetBranch', () => { input: { ...baseInput, targetBranch: 'develop', - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), + }, + }); + + expect(octokit.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: attributionBody('Created by Roomote.'), + }), + ); + }); + + it('scrubs an unmarked public attribution line without parsing the name', async () => { + const octokit = makeOctokit({ + list: [], + created: { + number: 13, + node_id: 'node-13', + html_url: 'https://github.com/acme/web/pull/13', + title: '[Feature] X', + draft: true, + base: { ref: 'develop' }, + }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Jane R. Doe', + publicDisplayName: null, + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + targetBranch: 'develop', + body: 'Preamble\n> Opened on behalf of Jane R. Doe. Follow up by mentioning @roomote.\n\nDone.', }, }); expect(octokit.rest.pulls.create).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Created by Roomote. Follow up by mentioning @roomote.', + body: 'Preamble\n> Created by Roomote.\n\nDone.', }), ); }); - it('preserves the original opener line when updating a pull request', async () => { + it('preserves replacement tokens literally in a private marked opener', async () => { const existing = { number: 11, node_id: 'node-11', @@ -1065,7 +1129,7 @@ describe('optional targetBranch', () => { title: 'Old title', draft: false, base: { ref: 'develop' }, - body: '> Opened on behalf of Launch Owner. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Launch $& Owner.'), }; const octokit = makeOctokit({ list: [existing], @@ -1080,18 +1144,18 @@ describe('optional targetBranch', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...baseInput, - body: '> Opened on behalf of Participant. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Participant.'), }, }); expect(octokit.rest.pulls.update).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of Launch Owner. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Launch $& Owner.'), }), ); }); - it('does not preserve a private display name when a public pull request is updated', async () => { + it('does not preserve a private marked name when a public pull request is updated', async () => { const existing = { number: 11, node_id: 'node-11', @@ -1099,7 +1163,7 @@ describe('optional targetBranch', () => { title: 'Old title', draft: false, base: { ref: 'develop' }, - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), }; const octokit = makeOctokit({ list: [existing], @@ -1123,64 +1187,18 @@ describe('optional targetBranch', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...baseInput, - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), }, }); expect(octokit.rest.pulls.update).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @participant.'), }), ); }); - it.each(['@Jane Doe', '@jane@example.com'])( - 'does not preserve invalid legacy public attribution %s', - async (legacyAttribution) => { - const existing = { - number: 11, - node_id: 'node-11', - html_url: 'https://github.com/acme/web/pull/11', - title: 'Old title', - draft: false, - base: { ref: 'develop' }, - body: `> Opened on behalf of ${legacyAttribution}. Follow up by mentioning @roomote.`, - }; - const octokit = makeOctokit({ - list: [existing], - updated: { ...existing, title: '[Feature] X' }, - }); - mockRepositoriesFindFirst.mockResolvedValue({ - installationId: 555, - externalRepoId: null, - fullName: 'acme/web', - htmlUrl: 'https://github.com/acme/web', - private: false, - }); - mockResolveRunCommitAuthor.mockResolvedValue({ - kind: 'user', - displayName: 'Private Name', - publicDisplayName: '@participant', - prAssigneeLogin: null, - }); - - await createOrUpdateSourceControlPullRequestForTaskRun({ - taskRun: makeTaskRun({ repo: 'acme/web' }), - input: { - ...baseInput, - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', - }, - }); - - expect(octokit.rest.pulls.update).toHaveBeenCalledWith( - expect.objectContaining({ - body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', - }), - ); - }, - ); - - it('preserves a valid handle in existing public attribution', async () => { + it('preserves a valid marked handle in existing public attribution', async () => { const existing = { number: 11, node_id: 'node-11', @@ -1188,7 +1206,7 @@ describe('optional targetBranch', () => { title: 'Old title', draft: false, base: { ref: 'develop' }, - body: '> Opened on behalf of @launch-owner. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @launch-owner.'), }; const octokit = makeOctokit({ list: [existing], @@ -1212,18 +1230,18 @@ describe('optional targetBranch', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...baseInput, - body: '> Opened on behalf of Private Name. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of Private Name.'), }, }); expect(octokit.rest.pulls.update).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of @launch-owner. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @launch-owner.'), }), ); }); - it('discards trailing legacy text after a valid public handle', async () => { + it('ignores attribution in an old unmarked public PR', async () => { const existing = { number: 11, node_id: 'node-11', @@ -1255,13 +1273,13 @@ describe('optional targetBranch', () => { taskRun: makeTaskRun({ repo: 'acme/web' }), input: { ...baseInput, - body: '> Opened on behalf of @participant. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @participant.'), }, }); expect(octokit.rest.pulls.update).toHaveBeenCalledWith( expect.objectContaining({ - body: '> Opened on behalf of @octocat. Follow up by mentioning @roomote.', + body: attributionBody('Opened on behalf of @participant.'), }), ); }); diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts index 40cc8bf7b..fbdc45cc0 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts @@ -20,7 +20,10 @@ import { import { buildPullRequestUrl, getSourceControlProviderLabel, + findPrBodyAttributionLine, + hasMarkedPrBodyAttribution, normalizePrBodyAttributionAppMention, + preservePrBodyAttribution, rewritePrBodyAttribution, prActions, sourceControlProviderSchema, @@ -238,10 +241,21 @@ export async function createOrUpdateSourceControlPullRequestForTaskRun({ : provider === 'github' ? attribution.publicDisplayName : null; + const rewrittenAttributionBody = rewritePrBodyAttribution( + normalizedMentionBody, + displayName, + ); const inputWithNormalizedAttribution: SourceControlPullRequestMutationInput = { ...input, - body: rewritePrBodyAttribution(normalizedMentionBody, displayName), + body: + repository.private !== true && + !hasMarkedPrBodyAttribution(normalizedMentionBody) + ? scrubUnmarkedPublicAttribution( + rewrittenAttributionBody, + displayName, + ) + : rewrittenAttributionBody, }; const liveGitHubAttribution = provider === 'github' ? attribution : undefined; @@ -555,9 +569,11 @@ function preserveExistingPullRequestAttribution( existingBody: string | null | undefined, repositoryIsPrivate: boolean, ): string { - const openerLine = existingBody?.match(/^> Opened on behalf of .+$/mu)?.[0]; + const openerLine = existingBody + ? findPrBodyAttributionLine(existingBody) + : null; const publicHandle = openerLine?.match( - /^> Opened on behalf of @([^\s.]+)\. /u, + /^> Opened on behalf of @([^\s.]+)\.(?: |$)/u, )?.[1]; const safePublicOpener = publicHandle !== undefined && @@ -567,10 +583,7 @@ function preserveExistingPullRequestAttribution( } if (repositoryIsPrivate) { - return body.replace( - /^(?:> Opened on behalf of .+|> Created by Roomote\..*)$/mu, - openerLine, - ); + return preservePrBodyAttribution(body, existingBody ?? ''); } return safePublicOpener @@ -578,6 +591,19 @@ function preserveExistingPullRequestAttribution( : body; } +function scrubUnmarkedPublicAttribution( + body: string, + displayName: string | null, +): string { + const provenance = displayName + ? `> Opened on behalf of ${displayName}.` + : '> Created by Roomote.'; + return body.replace( + /^[ \t]*>[ \t]*(?:Opened on behalf of|Created by Roomote).*$/gmu, + () => provenance, + ); +} + async function createOrUpdateGitLabMergeRequest({ input, repository, diff --git a/packages/types/src/__tests__/github-bot-identity.test.ts b/packages/types/src/__tests__/github-bot-identity.test.ts index 43abcfc08..6650453d9 100644 --- a/packages/types/src/__tests__/github-bot-identity.test.ts +++ b/packages/types/src/__tests__/github-bot-identity.test.ts @@ -1,6 +1,7 @@ import { getRoomoteGitHubAppSlugs, getRoomoteManagedGitHubLogins, + formatPrBodyAttribution, matchesRoomoteGitHubLogin, normalizePrBodyAttributionAppMention, rewritePrBodyAttribution, @@ -68,31 +69,48 @@ describe('Roomote GitHub bot identity helpers', () => { describe('normalizePrBodyAttributionAppMention', () => { it('rewrites a hardcoded @roomote mention to the configured app slug', () => { - const body = - '> Created by Roomote. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).\n\n## What changed\n\nDone.'; + const body = `${formatPrBodyAttribution( + 'Created by Roomote.', + 'Follow up by mentioning @roomote or in [the web UI](https://example.com/task/1).', + )}\n\n## What changed\n\nDone.`; expect( normalizePrBodyAttributionAppMention(body, 'roomote-roomote'), ).toBe( - '> Created by Roomote. Follow up by mentioning @roomote-roomote or in [the web UI](https://example.com/task/1).\n\n## What changed\n\nDone.', + `${formatPrBodyAttribution( + 'Created by Roomote.', + 'Follow up by mentioning @roomote-roomote or in [the web UI](https://example.com/task/1).', + )}\n\n## What changed\n\nDone.`, ); }); it('keeps the shorthand when it is enabled', () => { - const body = - '> Created by Roomote. Follow up by mentioning @roomote-roomote.'; + const body = formatPrBodyAttribution( + 'Created by Roomote.', + 'Follow up by mentioning @roomote-roomote.', + ); expect( normalizePrBodyAttributionAppMention(body, 'roomote-roomote', true), - ).toBe('> Created by Roomote. Follow up by mentioning @roomote.'); + ).toBe( + formatPrBodyAttribution( + 'Created by Roomote.', + 'Follow up by mentioning @roomote.', + ), + ); }); it('rewrites Opened on behalf of attribution mentions', () => { - const body = - '> Opened on behalf of Matt Rubens. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.'; + const body = formatPrBodyAttribution( + 'Opened on behalf of Matt Rubens.', + '[View the task](https://example.com/task/1) or mention @roomote for follow-up asks.', + ); expect(normalizePrBodyAttributionAppMention(body, 'openmote')).toBe( - '> Opened on behalf of Matt Rubens. [View the task](https://example.com/task/1) or mention @openmote for follow-up asks.', + formatPrBodyAttribution( + 'Opened on behalf of Matt Rubens.', + '[View the task](https://example.com/task/1) or mention @openmote for follow-up asks.', + ), ); }); @@ -111,32 +129,70 @@ describe('Roomote GitHub bot identity helpers', () => { expect(normalizePrBodyAttributionAppMention(body, 'openmote')).toBe(body); }); - it('rewrites historical unlinked-attribution mentions', () => { + it('leaves unmarked historical attribution alone', () => { const body = '> Created by Roomote from an unlinked Slack user. Follow up by mentioning @roomote or in the web UI.'; expect( normalizePrBodyAttributionAppMention(body, 'roomote-roomote'), - ).toBe( - '> Created by Roomote from an unlinked Slack user. Follow up by mentioning @roomote-roomote or in the web UI.', - ); + ).toBe(body); }); }); describe('rewritePrBodyAttribution', () => { - const body = - '> Opened on behalf of Private Name. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.'; + const instruction = + '[View the task](https://example.com/task/1) or mention @roomote for follow-up asks.'; + const body = `${formatPrBodyAttribution( + 'Opened on behalf of Private Name.', + instruction, + )}\n\n## What changed\n\nDone.`; it('uses a public handle without changing the attribution tail', () => { expect(rewritePrBodyAttribution(body, '@octocat')).toBe( - '> Opened on behalf of @octocat. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.', + `${formatPrBodyAttribution('Opened on behalf of @octocat.', instruction)}\n\n## What changed\n\nDone.`, ); }); it('uses generic Roomote provenance when no public identity exists', () => { expect(rewritePrBodyAttribution(body, null)).toBe( - '> Created by Roomote. [View the task](https://example.com/task/1) or mention @roomote for follow-up asks.\n\n## What changed\n\nDone.', + `${formatPrBodyAttribution('Created by Roomote.', instruction)}\n\n## What changed\n\nDone.`, ); }); + + it('handles periods in marked display names without parsing them', () => { + const marked = formatPrBodyAttribution( + 'Opened on behalf of Jane R. Doe.', + instruction, + ); + + expect(rewritePrBodyAttribution(marked, null)).toBe( + formatPrBodyAttribution('Created by Roomote.', instruction), + ); + }); + + it('rewrites a marked attribution line after a preamble', () => { + const marked = `Preamble\n${formatPrBodyAttribution( + 'Opened on behalf of Private Name.', + instruction, + )}`; + + expect(rewritePrBodyAttribution(marked, null)).toBe( + `Preamble\n${formatPrBodyAttribution('Created by Roomote.', instruction)}`, + ); + }); + + it('ignores markers outside an attribution blockquote', () => { + const unquoted = formatPrBodyAttribution( + 'Opened on behalf of Private Name.', + instruction, + ).slice(2); + expect(rewritePrBodyAttribution(unquoted, null)).toBe(unquoted); + }); + + it('does not parse or upgrade unmarked attribution', () => { + const legacy = + '> Opened on behalf of Jane R. Doe. Follow up by mentioning @roomote.'; + expect(rewritePrBodyAttribution(legacy, null)).toBe(legacy); + }); }); }); diff --git a/packages/types/src/constants.ts b/packages/types/src/constants.ts index e72daecf3..239cbde61 100644 --- a/packages/types/src/constants.ts +++ b/packages/types/src/constants.ts @@ -85,96 +85,73 @@ export function getGitHubFollowUpMention( return roomoteMentionEnabled ? '@roomote' : getGitHubAppMention(slug); } -/** - * Leading Roomote PR provenance blockquote: - * `> Created by Roomote. ...` or `> Opened on behalf of . ...` - * (including the historical "from an unlinked ..." attribution form). - * - * Parsed with linear string scans so untrusted PR bodies cannot trigger - * polynomial regular-expression matching. - */ -function matchPrBodyAttributionLine( - firstLine: string, -): { prefix: string; instruction: string } | null { - if (!firstLine.startsWith('>')) { +export const PR_BODY_ATTRIBUTION_START_MARKER = + ''; +export const PR_BODY_ATTRIBUTION_END_MARKER = + ''; + +type PrBodyAttributionMarkerMatch = { + start: number; + end: number; + lineStart: number; + lineEnd: number; +}; + +function findPrBodyAttributionMarkers( + body: string, +): PrBodyAttributionMarkerMatch | null { + const startMarker = body.indexOf(PR_BODY_ATTRIBUTION_START_MARKER); + if (startMarker === -1) { return null; } - let index = 1; - while ( - index < firstLine.length && - (firstLine.charCodeAt(index) === 32 /* space */ || - firstLine.charCodeAt(index) === 9) /* tab */ - ) { - index += 1; + const start = startMarker + PR_BODY_ATTRIBUTION_START_MARKER.length; + const end = body.indexOf(PR_BODY_ATTRIBUTION_END_MARKER, start); + if (end === -1 || body.slice(start, end).includes('\n')) { + return null; } - const contentStart = index; - const content = firstLine.slice(contentStart); - - const createdByPrefix = 'Created by Roomote'; - if (content.startsWith(createdByPrefix)) { - let sentenceEnd = createdByPrefix.length; - - if (content.startsWith(' from an unlinked ', sentenceEnd)) { - sentenceEnd += ' from an unlinked '.length; - while ( - sentenceEnd < content.length && - content.charCodeAt(sentenceEnd) !== 46 /* . */ - ) { - sentenceEnd += 1; - } - } - - if (content.charCodeAt(sentenceEnd) !== 46 /* . */) { - return null; - } - - sentenceEnd += 1; - while ( - sentenceEnd < content.length && - (content.charCodeAt(sentenceEnd) === 32 || - content.charCodeAt(sentenceEnd) === 9) - ) { - sentenceEnd += 1; - } - - return { - prefix: firstLine.slice(0, contentStart + sentenceEnd), - instruction: content.slice(sentenceEnd), - }; + const lineStart = body.lastIndexOf('\n', startMarker - 1) + 1; + if (!/^[ \t]*>[ \t]*$/u.test(body.slice(lineStart, startMarker))) { + return null; } - const openedPrefix = 'Opened on behalf of '; - if (content.startsWith(openedPrefix)) { - let sentenceEnd = openedPrefix.length; - while ( - sentenceEnd < content.length && - content.charCodeAt(sentenceEnd) !== 46 /* . */ - ) { - sentenceEnd += 1; - } + return { + start, + end, + lineStart, + lineEnd: + body.indexOf('\n', end) === -1 ? body.length : body.indexOf('\n', end), + }; +} - if (content.charCodeAt(sentenceEnd) !== 46 /* . */) { - return null; - } +export function formatPrBodyAttribution( + provenance: string, + instruction: string, +): string { + return `> ${PR_BODY_ATTRIBUTION_START_MARKER}${provenance}${PR_BODY_ATTRIBUTION_END_MARKER} ${instruction}`; +} - sentenceEnd += 1; - while ( - sentenceEnd < content.length && - (content.charCodeAt(sentenceEnd) === 32 || - content.charCodeAt(sentenceEnd) === 9) - ) { - sentenceEnd += 1; - } +export function findPrBodyAttributionLine(body: string): string | null { + const markers = findPrBodyAttributionMarkers(body); + return markers ? `> ${body.slice(markers.start, markers.end)}` : null; +} - return { - prefix: firstLine.slice(0, contentStart + sentenceEnd), - instruction: content.slice(sentenceEnd), - }; +export function hasMarkedPrBodyAttribution(body: string): boolean { + return findPrBodyAttributionMarkers(body) !== null; +} + +export function preservePrBodyAttribution( + body: string, + existingBody: string, +): string { + const current = findPrBodyAttributionMarkers(body); + const existing = findPrBodyAttributionMarkers(existingBody); + if (!current || !existing) { + return body; } - return null; + return `${body.slice(0, current.start)}${existingBody.slice(existing.start, existing.end)}${body.slice(current.end)}`; } /** @@ -182,8 +159,8 @@ function matchPrBodyAttributionLine( * the deployment's current follow-up handle: the configured GitHub App slug, * or the shorter `@roomote` alias when that setting is enabled. * - * Only the leading attribution blockquote is rewritten; other body text that - * happens to mention `@roomote` is left unchanged. + * Only the marker-containing line is rewritten; other body text that happens + * to mention `@roomote` is left unchanged. */ export function normalizePrBodyAttributionAppMention( body: string, @@ -200,51 +177,43 @@ export function normalizePrBodyAttributionAppMention( normalizedSlug, roomoteMentionEnabled, ); - const firstNewline = body.indexOf('\n'); - const firstLine = firstNewline === -1 ? body : body.slice(0, firstNewline); - const remainder = firstNewline === -1 ? '' : body.slice(firstNewline); - const match = matchPrBodyAttributionLine(firstLine); - - if (!match) { + const markers = findPrBodyAttributionMarkers(body); + if (!markers) { return body; } - const { prefix, instruction } = match; - const rewrittenInstruction = instruction.replace( + const line = body.slice(markers.lineStart, markers.lineEnd); + const rewrittenLine = line.replace( /(mention(?:ing)?\s+)@([A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?)/g, `$1${mention}`, ); - if (rewrittenInstruction === instruction) { + if (rewrittenLine === line) { return body; } - return `${prefix}${rewrittenInstruction}${remainder}`; + return `${body.slice(0, markers.lineStart)}${rewrittenLine}${body.slice(markers.lineEnd)}`; } /** - * Rewrite the leading Roomote PR provenance sentence while preserving its - * task/conversation links and follow-up instructions. + * Rewrite only the server-owned provenance text between attribution markers. + * Unmarked bodies are deliberately left alone. */ export function rewritePrBodyAttribution( body: string, displayName: string | null, ): string { - const firstNewline = body.indexOf('\n'); - const firstLine = firstNewline === -1 ? body : body.slice(0, firstNewline); - const remainder = firstNewline === -1 ? '' : body.slice(firstNewline); - const match = matchPrBodyAttributionLine(firstLine); - - if (!match) { + const markers = findPrBodyAttributionMarkers(body); + if (!markers) { return body; } const normalizedDisplayName = displayName?.trim().replace(/[\r\n]+/g, ' '); const provenance = normalizedDisplayName - ? `> Opened on behalf of ${normalizedDisplayName}. ` - : '> Created by Roomote. '; + ? `Opened on behalf of ${normalizedDisplayName}.` + : 'Created by Roomote.'; - return `${provenance}${match.instruction}${remainder}`; + return `${body.slice(0, markers.start)}${provenance}${body.slice(markers.end)}`; } /** From c87e5f188bf0886f889e8a69dc794021fa6ed092 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:51:18 +0000 Subject: [PATCH 5/8] fix: scrub duplicate public attribution --- .../source-control-pull-requests.test.ts | 42 +++++++++++++++++++ .../source-control-pull-requests.ts | 4 +- packages/types/src/constants.ts | 4 -- 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index ab38e5b3c..04ee59e82 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts @@ -1037,6 +1037,48 @@ describe('optional targetBranch', () => { ); }); + it('scrubs duplicated unmarked attribution in an otherwise marked public body', async () => { + const octokit = makeOctokit({ + list: [], + created: { + number: 13, + node_id: 'node-13', + html_url: 'https://github.com/acme/web/pull/13', + title: '[Feature] X', + draft: true, + base: { ref: 'develop' }, + }, + }); + mockRepositoriesFindFirst.mockResolvedValue({ + installationId: 555, + externalRepoId: null, + fullName: 'acme/web', + htmlUrl: 'https://github.com/acme/web', + private: false, + }); + mockResolveRunCommitAuthor.mockResolvedValue({ + kind: 'user', + displayName: 'Private Name', + publicDisplayName: '@participant', + prAssigneeLogin: null, + }); + + await createOrUpdateSourceControlPullRequestForTaskRun({ + taskRun: makeTaskRun({ repo: 'acme/web' }), + input: { + ...baseInput, + targetBranch: 'develop', + body: `${attributionBody('Opened on behalf of Private Name.')}\n\n> Opened on behalf of Duplicated Private Name.`, + }, + }); + + expect(octokit.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: `${attributionBody('Opened on behalf of @participant.')}\n\n> Opened on behalf of @participant.`, + }), + ); + }); + it('uses generic provenance in a public GitHub pull request without a linked handle', async () => { const octokit = makeOctokit({ list: [], diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts index fbdc45cc0..b9a48a530 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-requests.ts @@ -21,7 +21,6 @@ import { buildPullRequestUrl, getSourceControlProviderLabel, findPrBodyAttributionLine, - hasMarkedPrBodyAttribution, normalizePrBodyAttributionAppMention, preservePrBodyAttribution, rewritePrBodyAttribution, @@ -249,8 +248,7 @@ export async function createOrUpdateSourceControlPullRequestForTaskRun({ { ...input, body: - repository.private !== true && - !hasMarkedPrBodyAttribution(normalizedMentionBody) + repository.private !== true ? scrubUnmarkedPublicAttribution( rewrittenAttributionBody, displayName, diff --git a/packages/types/src/constants.ts b/packages/types/src/constants.ts index 239cbde61..ac2d71346 100644 --- a/packages/types/src/constants.ts +++ b/packages/types/src/constants.ts @@ -137,10 +137,6 @@ export function findPrBodyAttributionLine(body: string): string | null { return markers ? `> ${body.slice(markers.start, markers.end)}` : null; } -export function hasMarkedPrBodyAttribution(body: string): boolean { - return findPrBodyAttributionMarkers(body) !== null; -} - export function preservePrBodyAttribution( body: string, existingBody: string, From 8fa8d302b8573662ca2af7e4e6588378b7151ccb Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:55:00 +0000 Subject: [PATCH 6/8] fix: require exact host for attribution --- .../__tests__/source-control-provider.test.ts | 23 +++++++++++++++++-- .../db/src/lib/source-control-provider.ts | 14 ++--------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/packages/db/src/lib/__tests__/source-control-provider.test.ts b/packages/db/src/lib/__tests__/source-control-provider.test.ts index 20d333dec..487ad1d90 100644 --- a/packages/db/src/lib/__tests__/source-control-provider.test.ts +++ b/packages/db/src/lib/__tests__/source-control-provider.test.ts @@ -249,7 +249,7 @@ describe('resolveWorkspaceSourceControlProvider', () => { ).resolves.toEqual({}); }); - it('falls back to a legacy null-host repository row', async () => { + it('does not use a legacy null-host row for a stamped host', async () => { mockRows = [ { fullName: 'group/project', @@ -264,7 +264,7 @@ describe('resolveWorkspaceSourceControlProvider', () => { repo: 'group/project', sourceControlHost: 'gitlab.example.com', }), - ).resolves.toEqual({ 'group/project': 'gitlab' }); + ).resolves.toEqual({}); }); it('prefers an exact host match over a legacy null-host row', async () => { @@ -442,6 +442,25 @@ describe('workspaceAllowsPrivateAttribution', () => { ).resolves.toBe(false); }); + it('uses public-safe attribution for a legacy null-host row', async () => { + mockRows = [ + { + fullName: 'group/project', + host: null, + private: true, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + workspaceAllowsPrivateAttribution(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'gitlab.example.com', + }), + ).resolves.toBe(false); + }); + it('uses legacy null-host visibility only when no exact host exists', async () => { mockRows = [ { diff --git a/packages/db/src/lib/source-control-provider.ts b/packages/db/src/lib/source-control-provider.ts index 8a7e47c6d..9ad344892 100644 --- a/packages/db/src/lib/source-control-provider.ts +++ b/packages/db/src/lib/source-control-provider.ts @@ -48,13 +48,8 @@ function selectRepositoryRows( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const exactHostMatches = sourceControlHost - ? candidates.filter((row) => row.host === sourceControlHost) - : []; const hostMatches = sourceControlHost - ? exactHostMatches.length > 0 - ? exactHostMatches - : candidates.filter((row) => row.host === null) + ? candidates.filter((row) => row.host === sourceControlHost) : candidates; if (hostMatches.length !== 1) { @@ -86,13 +81,8 @@ function toRepositoryProviderMap( const matches = rowsByFullName.get(fullName) ?? []; const activeMatches = matches.filter((row) => row.isActive === true); const candidates = activeMatches.length > 0 ? activeMatches : matches; - const exactHostMatches = sourceControlHost - ? candidates.filter((row) => row.host === sourceControlHost) - : []; const hostMatches = sourceControlHost - ? exactHostMatches.length > 0 - ? exactHostMatches - : candidates.filter((row) => row.host === null) + ? candidates.filter((row) => row.host === sourceControlHost) : candidates; if (candidates.length > 1 && hostMatches.length !== 1) { From 9f8019db2fa0944d8abb7a72c655952fe0a4df9e Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:06:51 +0000 Subject: [PATCH 7/8] test: align attribution prompt expectations --- .changeset/safe-public-attribution.md | 8 ------ .../requestUserInputGuidance.test.ts | 28 +++++++++---------- .../__tests__/slackAppMention.test.ts | 6 ++-- 3 files changed, 17 insertions(+), 25 deletions(-) delete mode 100644 .changeset/safe-public-attribution.md diff --git a/.changeset/safe-public-attribution.md b/.changeset/safe-public-attribution.md deleted file mode 100644 index 2c0749ebc..000000000 --- a/.changeset/safe-public-attribution.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -'@roomote/cloud-agents': patch -'@roomote/db': patch -'@roomote/sdk': patch -'@roomote/types': patch ---- - -Use linked source-control usernames instead of account names when attributing Roomote changes in public repositories. diff --git a/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts index ec6c592b8..639cc7fbe 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/requestUserInputGuidance.test.ts @@ -235,7 +235,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. [View the task](https://example.com/task/123) or mention @', + 'prepend `> Opened on behalf of Jane Doe. [View the task](https://example.com/task/123) or mention @', ); expect(harnessInstructions).toContain( 'for follow-up asks.` at the top of the PR body file before creating or refreshing the pull request', @@ -270,7 +270,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${sharedSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${sharedSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); @@ -284,7 +284,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -300,7 +300,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${sharedSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${sharedSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); @@ -317,7 +317,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); @@ -334,7 +334,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Telegram](https://t.me/c/456789/7/42).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Telegram](https://t.me/c/456789/7/42).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -351,7 +351,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Telegram](https://t.me/roomote_bot).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Telegram](https://t.me/roomote_bot).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -367,7 +367,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -384,7 +384,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Teams](https://teams.microsoft.com/l/message/19%3Achannel%40thread.v2/1647012345678?tenantId=tenant-abc).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Teams](https://teams.microsoft.com/l/message/19%3Achannel%40thread.v2/1647012345678?tenantId=tenant-abc).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -402,7 +402,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Teams](https://teams.microsoft.com/l/app/bot-app-id?tenantId=tenant-abc).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Teams](https://teams.microsoft.com/l/app/bot-app-id?tenantId=tenant-abc).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -418,7 +418,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -432,7 +432,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', + 'prepend `> Opened on behalf of Jane Doe. Follow up by mentioning @roomote or in [the web UI](https://example.com/task/123).` at the top of the PR body file before creating or refreshing the pull request', ); }); @@ -471,7 +471,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Opened on behalf of Jane Doe. [View the task](https://example.com/task/123) or mention @', + 'prepend `> Opened on behalf of Jane Doe. [View the task](https://example.com/task/123) or mention @', ); expect(harnessInstructions).toContain( 'for follow-up asks.` at the top of the PR body file before creating or refreshing the pull request', @@ -491,7 +491,7 @@ describe('request_user_input guidance in workflow prompts', () => { }); expect(harnessInstructions).toContain( - 'prepend `> Created by Roomote. [View the task](https://example.com/task/123) or mention @', + 'prepend `> Created by Roomote. [View the task](https://example.com/task/123) or mention @', ); expect(harnessInstructions).toContain( 'for follow-up asks.` at the top of the PR body file before creating or refreshing the pull request', diff --git a/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts index 6a85d5524..75154586b 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/slackAppMention.test.ts @@ -384,7 +384,7 @@ describe('slackAppMention', () => { }); expect(result.harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSpecificAppSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSpecificAppSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); @@ -410,7 +410,7 @@ describe('slackAppMention', () => { }); expect(result.harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${exactSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${exactSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); @@ -434,7 +434,7 @@ describe('slackAppMention', () => { }); expect(result.harnessInstructions).toContain( - `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSpecificAppSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, + `prepend \`> Opened on behalf of Jane Doe. Follow up by mentioning @roomote, in [the web UI](https://example.com/task/123), or in [Slack](${teamSpecificAppSlackPermalink}).\` at the top of the PR body file before creating or refreshing the pull request`, ); }); From f2923f8ea709b832de503502d5a06f6485329c27 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:20:22 +0000 Subject: [PATCH 8/8] fix: require exact host in PR repository lookup --- ...ource-control-pull-request-shared.host.test.ts | 15 ++++++++------- .../source-control-pull-request-shared.ts | 11 ++--------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-shared.host.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-shared.host.test.ts index 33e9d3473..d6eb3a63b 100644 --- a/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-shared.host.test.ts +++ b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-request-shared.host.test.ts @@ -76,11 +76,10 @@ describe('resolveRepositoryRow host scoping', () => { ).resolves.toEqual(exactRow); }); - it('falls back to a legacy null-host row when no row matches the host exactly', async () => { - const legacyRow = repositoryRow({ id: 'repo-legacy', host: null }); + it('rejects a legacy null-host row when no row matches the host exactly', async () => { mockRepositoriesFindMany.mockResolvedValue([ repositoryRow({ id: 'repo-other-host', host: 'gitlab.other.example' }), - legacyRow, + repositoryRow({ id: 'repo-legacy', host: null }), ]); await expect( @@ -89,7 +88,9 @@ describe('resolveRepositoryRow host scoping', () => { repositoryFullName: 'acme/backend', host: 'gitlab.example.com', }), - ).resolves.toEqual(legacyRow); + ).rejects.toThrow( + 'GitLab repository not found or inactive on gitlab.example.com: acme/backend', + ); }); it('reports the host in the not-found error when no row qualifies for it', async () => { @@ -108,10 +109,10 @@ describe('resolveRepositoryRow host scoping', () => { ); }); - it('rejects multiple candidates within the chosen host tier', async () => { + it('rejects multiple exact matches within the chosen host', async () => { mockRepositoriesFindMany.mockResolvedValue([ - repositoryRow({ id: 'repo-legacy-1', host: null }), - repositoryRow({ id: 'repo-legacy-2', host: null }), + repositoryRow({ id: 'repo-exact-1', host: 'gitlab.example.com' }), + repositoryRow({ id: 'repo-exact-2', host: 'gitlab.example.com' }), ]); await expect( diff --git a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts index 7be5d0368..b6c48f62d 100644 --- a/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts +++ b/packages/sdk/src/server/lib/pull-requests/source-control-pull-request-shared.ts @@ -97,10 +97,7 @@ export function resolveSourceControlHostForRepositoryFromPayload( * optionally narrowed by source-control instance host. * * When `host` is provided (typically from the task payload's - * `sourceControlHost`), rows whose `host` matches exactly are preferred; - * when none match, rows with a NULL host still qualify so legacy rows - * written before the host backfill keep resolving (mirroring the scoping in - * `upsertSourceControlPullRequestFactFromWebhook`). + * `sourceControlHost`), only rows whose `host` matches exactly qualify. * * Without a `host`, a (provider, fullName) identity active on more than one * row is an error rather than an arbitrary pick: same-name repositories on @@ -136,11 +133,7 @@ export async function resolveRepositoryRow({ }); if (host !== undefined) { - const exactMatches = rows.filter((row) => row.host === host); - const candidates = - exactMatches.length > 0 - ? exactMatches - : rows.filter((row) => row.host === null); + const candidates = rows.filter((row) => row.host === host); if (candidates.length === 0) { throw new Error(