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/__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 b20b73087..74e455360 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,16 @@ 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.email !== ROOMOTE_GIT_AUTHOR.email + ? { ...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..639cc7fbe 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: { @@ -234,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', @@ -269,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`, ); }); @@ -283,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', ); }); @@ -299,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`, ); }); @@ -316,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`, ); }); @@ -333,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', ); }); @@ -350,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', ); }); @@ -366,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', ); }); @@ -383,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', ); }); @@ -401,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', ); }); @@ -417,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', ); }); @@ -431,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', ); }); @@ -470,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', @@ -490,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 e4b3ef6ab..75154586b 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: { @@ -383,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`, ); }); @@ -409,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`, ); }); @@ -433,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`, ); }); 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/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/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 3da2ceed5..487ad1d90 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[] = []; @@ -228,6 +231,65 @@ 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('does not use a legacy null-host row for a stamped host', async () => { + mockRows = [ + { + fullName: 'group/project', + host: null, + sourceControlProvider: 'gitlab', + }, + ]; + + await expect( + resolveWorkspaceRepositoryProviders(dbOrTx, { + type: 'repository', + repo: 'group/project', + sourceControlHost: 'gitlab.example.com', + }), + ).resolves.toEqual({}); + }); + + 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 = [ { @@ -297,3 +359,158 @@ 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('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('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 = [ + { + 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 = [ + { + 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..9ad344892 100644 --- a/packages/db/src/lib/source-control-provider.ts +++ b/packages/db/src/lib/source-control-provider.ts @@ -25,9 +25,43 @@ 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 = 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[], @@ -47,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( @@ -177,6 +210,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-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/__tests__/source-control-pull-requests.test.ts b/packages/sdk/src/server/lib/pull-requests/__tests__/source-control-pull-requests.test.ts index 765008932..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 @@ -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,16 +171,28 @@ 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(); 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 +215,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 +248,7 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { sourceBranch: 'codex/provider-neutral', targetBranch: 'develop', title: '[Feature] Provider neutral PRs', - body: 'Body', + body: attributionBody('Opened on behalf of Private Name.'), labels: ['roomote'], assignees: [], }, @@ -264,7 +287,7 @@ describe('createOrUpdateSourceControlPullRequestForTaskRun', () => { target_branch: 'develop', remove_source_branch: false, title: '[Feature] Provider neutral PRs', - description: 'Body', + description: attributionBody('Created by Roomote.'), labels: 'roomote', }), }), @@ -342,10 +365,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 +416,7 @@ describe('platform-managed draft state', () => { externalRepoId: null, fullName: 'acme/web', htmlUrl: 'https://github.com/acme/web', + private: true, }); return octokit; } @@ -446,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.`, }), ); }); @@ -476,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).', + ), }), ); }); @@ -705,10 +746,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 +812,7 @@ describe('optional targetBranch', () => { externalRepoId: null, fullName: 'acme/web', htmlUrl: 'https://github.com/acme/web', + private: true, }); return octokit; } @@ -937,18 +984,186 @@ 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: attributionBody('Opened on behalf of Participant.'), + }), + ); + }); + + 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: attributionBody('Opened on behalf of Private Name.'), + }, + }); + + expect(octokit.rest.pulls.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: attributionBody('Opened on behalf of @participant.'), + }), + ); + }); + + 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: [], + 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: 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('Created by Roomote.'), }), ); }); - it('preserves the original opener line when updating a pull request', async () => { + 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: 'Preamble\n> Created by Roomote.\n\nDone.', + }), + ); + }); + + it('preserves replacement tokens literally in a private marked opener', async () => { const existing = { number: 11, node_id: 'node-11', @@ -956,7 +1171,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], @@ -971,13 +1186,142 @@ 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: attributionBody('Opened on behalf of Launch $& Owner.'), + }), + ); + }); + + it('does not preserve a private marked 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: attributionBody('Opened on behalf of Private Name.'), + }; + 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: attributionBody('Opened on behalf of Private Name.'), + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: attributionBody('Opened on behalf of @participant.'), + }), + ); + }); + + it('preserves a valid marked 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: attributionBody('Opened on behalf of @launch-owner.'), + }; + 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: attributionBody('Opened on behalf of Private Name.'), + }, + }); + + expect(octokit.rest.pulls.update).toHaveBeenCalledWith( + expect.objectContaining({ + body: attributionBody('Opened on behalf of @launch-owner.'), + }), + ); + }); + + it('ignores attribution in an old unmarked public PR', 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: 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 @participant.'), }), ); }); 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..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 @@ -37,6 +37,7 @@ export type RepositoryRow = { externalRepoId: string | null; fullName: string; htmlUrl: string; + private?: boolean; }; export function resolveSourceControlProviderForRepositoryFromPayload( @@ -96,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 @@ -130,15 +128,12 @@ export async function resolveRepositoryRow({ externalRepoId: true, fullName: true, htmlUrl: true, + private: true, }, }); 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( 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..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 @@ -20,7 +20,10 @@ import { import { buildPullRequestUrl, getSourceControlProviderLabel, + findPrBodyAttributionLine, normalizePrBodyAttributionAppMention, + preservePrBodyAttribution, + rewritePrBodyAttribution, prActions, sourceControlProviderSchema, type PrAction, @@ -218,22 +221,42 @@ 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 rewrittenAttributionBody = rewritePrBodyAttribution( + normalizedMentionBody, + displayName, + ); + const inputWithNormalizedAttribution: SourceControlPullRequestMutationInput = + { + ...input, + body: + repository.private !== true + ? scrubUnmarkedPublicAttribution( + rewrittenAttributionBody, + displayName, + ) + : rewrittenAttributionBody, + }; + + const liveGitHubAttribution = provider === 'github' ? attribution : undefined; const liveGitHubAssigneePlan = liveGitHubAttribution ? await resolveLiveGitHubAssigneePlan({ taskRun, @@ -256,7 +279,6 @@ export async function createOrUpdateSourceControlPullRequestForTaskRun({ repository, provider, createDraft, - attribution: liveGitHubAttribution, staleLaunchAssignee: liveGitHubAssigneePlan?.staleLaunchAssignee, }); case 'gitlab': @@ -415,14 +437,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 +494,7 @@ async function createOrUpdateGitHubPullRequest({ body: preserveExistingPullRequestAttribution( input.body, pullRequest.body, + repository.private === true, ), }); pullRequest = data; @@ -484,7 +505,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,28 +562,44 @@ async function createOrUpdateGitHubPullRequest({ }; } -function replaceCreatedPullRequestAttribution( +function preserveExistingPullRequestAttribution( body: string, - attribution: ResolvedTaskCommitAuthor | undefined, + existingBody: string | null | undefined, + repositoryIsPrivate: boolean, ): string { - if (!attribution) { + const openerLine = existingBody + ? findPrBodyAttributionLine(existingBody) + : null; + 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); + if (!openerLine) { return body; } - return body.replace( - /^(> Opened on behalf of ).+?(\. (?:Follow up by|\[View the task\]))/mu, - `$1${attribution.displayName}$2`, - ); + if (repositoryIsPrivate) { + return preservePrBodyAttribution(body, existingBody ?? ''); + } + + return safePublicOpener + ? rewritePrBodyAttribution(body, `@${publicHandle}`) + : body; } -function preserveExistingPullRequestAttribution( +function scrubUnmarkedPublicAttribution( body: string, - existingBody: string | null | undefined, + displayName: string | null, ): string { - const openerLine = existingBody?.match(/^> Opened on behalf of .+$/mu)?.[0]; - return openerLine - ? body.replace(/^> Opened on behalf of .+$/mu, openerLine) - : body; + 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({ 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..6650453d9 100644 --- a/packages/types/src/__tests__/github-bot-identity.test.ts +++ b/packages/types/src/__tests__/github-bot-identity.test.ts @@ -1,8 +1,10 @@ import { getRoomoteGitHubAppSlugs, getRoomoteManagedGitHubLogins, + formatPrBodyAttribution, matchesRoomoteGitHubLogin, normalizePrBodyAttributionAppMention, + rewritePrBodyAttribution, } from '../constants'; describe('Roomote GitHub bot identity helpers', () => { @@ -67,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.', + ), ); }); @@ -110,15 +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 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( + `${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( + `${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 eae431b5f..ac2d71346 100644 --- a/packages/types/src/constants.ts +++ b/packages/types/src/constants.ts @@ -85,96 +85,69 @@ 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 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 +155,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,26 +173,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 `${body.slice(0, markers.lineStart)}${rewrittenLine}${body.slice(markers.lineEnd)}`; +} + +/** + * 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 markers = findPrBodyAttributionMarkers(body); + if (!markers) { return body; } - return `${prefix}${rewrittenInstruction}${remainder}`; + const normalizedDisplayName = displayName?.trim().replace(/[\r\n]+/g, ' '); + const provenance = normalizedDisplayName + ? `Opened on behalf of ${normalizedDisplayName}.` + : 'Created by Roomote.'; + + return `${body.slice(0, markers.start)}${provenance}${body.slice(markers.end)}`; } /**