Skip to content

Commit 3afd694

Browse files
committed
fixes
1 parent b02c266 commit 3afd694

8 files changed

Lines changed: 171 additions & 50 deletions

File tree

README.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ It gives org admins a place to manage CLA text and signing history, and gives co
99
- If a contributor has signed a non-current CLA version, they must re-sign before being considered compliant.
1010
- Contributor compliance status is evaluated per org using the contributor's latest signed version for that org.
1111
- If a contributor has open pull requests and their signature becomes outdated after a CLA update, checks may need to be re-opened/re-evaluated and set to failing until re-signing is completed.
12+
- After a contributor signs/re-signs the latest CLA, the bot updates their open PR CLA checks back to success and removes stale CLA prompt comments.
1213
- GitHub is the user-management source of truth for the app.
1314
- The app has no local signup/password user-management system; DB user rows are GitHub-linked identity mirrors only.
1415
- Authentication/session management is stateless JWT-based (HTTP-only cookie + signed JWT with `jti`).
@@ -158,8 +159,8 @@ This section amends your scenario list and adds missing scenarios.
158159
### 6) Contributor signs/re-signs CLA
159160

160161
- Signature is stored with org, user, full CLA hash, accepted hash, assent metadata, immutable GitHub ID at signing time, timestamp, email provenance, and session evidence fields.
161-
- For linked PR context (`repo` + `pr`), failing CLA check/comment can be updated to success/signed status.
162-
- Current behavior note: updates are scoped to known PR context, not guaranteed bulk update of all open PRs for that contributor.
162+
- If `repo` + `pr` is provided, the signer must match that PR author before targeted PR sync is applied.
163+
- After signing/re-signing, open PRs authored by that contributor in the org are auto-synced: the latest CLA check run is updated to success and stale CLA prompt comments are deleted.
163164

164165
### 7) Signed CLA versions cannot be deleted
165166

@@ -222,7 +223,7 @@ This section amends your scenario list and adds missing scenarios.
222223
- Signed current CLA: passing check, no CLA comment.
223224
- Unsigned/outdated signature: failing check + bot comment with signing URL.
224225
- When CLA text changes, contributors on older signatures are marked as requiring re-sign; open PRs may require check re-evaluation and failure until re-signing.
225-
- After signing/re-signing, check/comment can auto-update for the target PR context.
226+
- After signing/re-signing, CLA checks on signer-authored open PRs are auto-updated to success and stale CLA prompt comments are removed.
226227
- Repository maintainers must require `CLA Bot / Contributor License Agreement` in GitHub branch protection/rulesets for merge blocking to be enforced.
227228

228229
## End-to-End Test Coverage Around This Spec

lib/cla/signing.ts

Lines changed: 78 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import {
66
getSignatureStatus,
77
} from "@/lib/db/queries"
88
import { getGitHubClient, type CheckRun } from "@/lib/github"
9-
import { generateSignedComment } from "@/lib/pr-comment-template"
109

1110
const CHECK_NAME = "CLA Bot / Contributor License Agreement"
1211
const DEFAULT_CONSENT_TEXT_VERSION = "v1"
@@ -43,7 +42,7 @@ type SignClaInput = {
4342
type SignClaResult = {
4443
signature: Awaited<ReturnType<typeof createSignature>>
4544
updatedChecks: CheckRun[]
46-
updatedCommentId: number | null
45+
deletedCommentIds: number[]
4746
autoUpdateSkippedReason: string | null
4847
}
4948

@@ -57,7 +56,6 @@ export async function signClaForUser(input: SignClaInput): Promise<SignClaResult
5756
assented,
5857
consentTextVersion,
5958
requestEvidence,
60-
appBaseUrl,
6159
} = input
6260

6361
if (!orgSlug) {
@@ -162,79 +160,97 @@ export async function signClaForUser(input: SignClaInput): Promise<SignClaResult
162160

163161
const versionLabel = org.claTextSha256.slice(0, 7)
164162
const updatedChecks: CheckRun[] = []
165-
let updatedCommentId: number | null = null
163+
const deletedCommentIds: number[] = []
166164
let autoUpdateSkippedReason: string | null = null
167165

168-
if (normalizedRepoName && parsedPrNumber && org.installationId) {
166+
if (!org.installationId) {
167+
autoUpdateSkippedReason = "missing_installation_id"
168+
} else {
169169
try {
170170
const github = getGitHubClient(org.installationId)
171-
const pullRequest = await github.getPullRequest(orgSlug, normalizedRepoName, parsedPrNumber)
172-
173-
if (!pullRequest) {
174-
autoUpdateSkippedReason = "pull_request_not_found"
175-
} else if (
176-
(typeof pullRequest.authorId === "number" &&
177-
String(pullRequest.authorId) !== user.githubId) ||
178-
(typeof pullRequest.authorId !== "number" &&
179-
pullRequest.authorLogin !== user.githubUsername)
180-
) {
181-
autoUpdateSkippedReason = "signer_not_pr_author"
182-
} else {
183-
const existingCheck = await github.getCheckRunForPr(
184-
orgSlug,
185-
normalizedRepoName,
186-
pullRequest.headSha,
187-
CHECK_NAME
171+
const targetPrs = new Map<
172+
string,
173+
{
174+
repoName: string
175+
prNumber: number
176+
headSha: string
177+
}
178+
>()
179+
const addTarget = (repoName: string, prNumber: number, headSha: string) => {
180+
targetPrs.set(`${repoName}#${prNumber}`, { repoName, prNumber, headSha })
181+
}
182+
183+
if (normalizedRepoName && parsedPrNumber) {
184+
const pullRequest = await github.getPullRequest(orgSlug, normalizedRepoName, parsedPrNumber)
185+
if (!pullRequest) {
186+
autoUpdateSkippedReason = "pull_request_not_found"
187+
} else if (!isSignerAuthorForPr(user, pullRequest.authorLogin, pullRequest.authorId)) {
188+
autoUpdateSkippedReason = "signer_not_pr_author"
189+
} else {
190+
addTarget(normalizedRepoName, parsedPrNumber, pullRequest.headSha)
191+
}
192+
}
193+
194+
const openPrs = await github.listOpenPullRequestsForOrganization(orgSlug)
195+
for (const pr of openPrs) {
196+
if (!isSignerAuthorForPr(user, pr.authorLogin, pr.authorId)) continue
197+
addTarget(pr.repoName, pr.number, pr.headSha)
198+
}
199+
200+
if (targetPrs.size === 0 && !autoUpdateSkippedReason) {
201+
autoUpdateSkippedReason = "no_open_prs_for_signer"
202+
}
203+
204+
for (const target of targetPrs.values()) {
205+
const latestClaCheck = (
206+
await github.listCheckRunsForRef(orgSlug, target.repoName, target.headSha)
188207
)
208+
.filter((check) => check.name === CHECK_NAME)
209+
.sort((a, b) => b.id - a.id)[0]
189210

190-
if (existingCheck && existingCheck.conclusion === "failure") {
211+
if (latestClaCheck && latestClaCheck.conclusion !== "success") {
191212
const updated = await github.updateCheckRun({
192213
owner: orgSlug,
193-
repo: normalizedRepoName,
194-
check_run_id: existingCheck.id,
214+
repo: target.repoName,
215+
check_run_id: latestClaCheck.id,
195216
status: "completed",
196217
conclusion: "success",
197218
output: {
198219
title: "CLA: Signed",
199-
summary: `@${user.githubUsername} has signed the CLA. Check updated automatically.`,
220+
summary: `@${user.githubUsername} has signed CLA version \`${versionLabel}\`.`,
200221
},
201222
})
202223
updatedChecks.push(updated)
203224
}
204225

205226
const existingComment = await github.findBotComment(
206227
orgSlug,
207-
normalizedRepoName,
208-
parsedPrNumber
228+
target.repoName,
229+
target.prNumber
209230
)
210-
if (
211-
existingComment &&
212-
(existingComment.body.includes("Contributor License Agreement Required") ||
213-
existingComment.body.includes("Re-signing Required"))
214-
) {
215-
await github.updateComment({
231+
if (existingComment && isRemovableClaPromptComment(existingComment.body)) {
232+
await github.deleteComment({
216233
owner: orgSlug,
217-
repo: normalizedRepoName,
234+
repo: target.repoName,
218235
comment_id: existingComment.id,
219-
body: generateSignedComment({
220-
prAuthor: user.githubUsername,
221-
orgName: org.name,
222-
claVersionLabel: versionLabel,
223-
appBaseUrl: appBaseUrl ?? getAppBaseUrl(),
224-
}),
225236
})
226-
updatedCommentId = existingComment.id
237+
deletedCommentIds.push(existingComment.id)
227238
}
228239
}
240+
241+
if (targetPrs.size > 0) {
242+
autoUpdateSkippedReason = null
243+
}
229244
} catch (err) {
230245
console.error("Failed to auto-update GitHub PR status after signing:", err)
246+
autoUpdateSkippedReason = "auto_update_failed"
231247
}
232248
}
233249

234250
return {
235251
signature,
236252
updatedChecks,
237-
updatedCommentId,
253+
deletedCommentIds,
238254
autoUpdateSkippedReason,
239255
}
240256
}
@@ -314,3 +330,22 @@ function hashIpAddress(source: string | null) {
314330
if (!secret || !source) return null
315331
return createHmac("sha256", secret).update(source).digest("hex")
316332
}
333+
334+
function isSignerAuthorForPr(
335+
user: Pick<SignClaUser, "githubId" | "githubUsername">,
336+
prAuthorLogin: string,
337+
prAuthorId?: number
338+
) {
339+
if (typeof prAuthorId === "number" && user.githubId) {
340+
return String(prAuthorId) === String(user.githubId)
341+
}
342+
return prAuthorLogin.trim().toLowerCase() === user.githubUsername.trim().toLowerCase()
343+
}
344+
345+
function isRemovableClaPromptComment(commentBody: string) {
346+
return (
347+
commentBody.includes("Contributor License Agreement Required") ||
348+
commentBody.includes("Re-signing Required") ||
349+
commentBody.includes("CLA Bot is not configured for this repository")
350+
)
351+
}

lib/github/client.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import type {
1818
IssueComment,
1919
CreateCommentParams,
2020
UpdateCommentParams,
21+
DeleteCommentParams,
2122
ListCommentsParams,
2223
PullRequestRef,
2324
OpenOrganizationPullRequestRef,
@@ -64,6 +65,9 @@ export interface GitHubClient {
6465
/** Update an existing comment. */
6566
updateComment(params: UpdateCommentParams): Promise<IssueComment>
6667

68+
/** Delete an existing comment. */
69+
deleteComment(params: DeleteCommentParams): Promise<void>
70+
6771
/** List all comments on a PR/issue. */
6872
listComments(params: ListCommentsParams): Promise<IssueComment[]>
6973

lib/github/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export type {
2323
UpdateCheckRunParams,
2424
CreateCommentParams,
2525
UpdateCommentParams,
26+
DeleteCommentParams,
2627
ListCommentsParams,
2728
PullRequestRef,
2829
OpenOrganizationPullRequestRef,

lib/github/mock-github-client.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import type {
1818
IssueComment,
1919
CreateCommentParams,
2020
UpdateCommentParams,
21+
DeleteCommentParams,
2122
ListCommentsParams,
2223
PullRequestRef,
2324
OpenOrganizationPullRequestRef,
@@ -239,6 +240,12 @@ export class MockGitHubClient implements GitHubClient {
239240
return { ...rest }
240241
}
241242

243+
async deleteComment(params: DeleteCommentParams): Promise<void> {
244+
const idx = comments.findIndex((c) => c.id === params.comment_id)
245+
if (idx === -1) throw new Error(`Comment ${params.comment_id} not found`)
246+
comments.splice(idx, 1)
247+
}
248+
242249
async listComments(params: ListCommentsParams): Promise<IssueComment[]> {
243250
return comments
244251
.filter(

lib/github/octokit-client.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type {
1616
IssueComment,
1717
CreateCommentParams,
1818
UpdateCommentParams,
19+
DeleteCommentParams,
1920
ListCommentsParams,
2021
PullRequestRef,
2122
OpenOrganizationPullRequestRef,
@@ -179,6 +180,14 @@ export class OctokitGitHubClient implements GitHubClient {
179180
return this.mapComment(data)
180181
}
181182

183+
async deleteComment(params: DeleteCommentParams): Promise<void> {
184+
await this.octokit.rest.issues.deleteComment({
185+
owner: params.owner,
186+
repo: params.repo,
187+
comment_id: params.comment_id,
188+
})
189+
}
190+
182191
async listComments(params: ListCommentsParams): Promise<IssueComment[]> {
183192
const { data } = await this.octokit.rest.issues.listComments({
184193
owner: params.owner,

lib/github/types.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,12 @@ export type UpdateCommentParams = {
9696
body: string
9797
}
9898

99+
export type DeleteCommentParams = {
100+
owner: string
101+
repo: string
102+
comment_id: number
103+
}
104+
99105
export type ListCommentsParams = {
100106
owner: string
101107
repo: string

tests/integration/api-suite.test.ts

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,16 +1042,67 @@ test("Webhook: after signing, check auto-updates to success (no /recheck needed)
10421042
assert(signData.updatedChecks !== undefined, "updatedChecks returned")
10431043
assert(signData.updatedChecks.length > 0, "at least one check was auto-updated")
10441044
assertEqual(signData.updatedChecks[0].conclusion, "success", "check auto-updated to success")
1045+
assert(Array.isArray(signData.deletedCommentIds), "deletedCommentIds returned")
1046+
assert(signData.deletedCommentIds.length > 0, "stale CLA prompt comment was deleted")
10451047

1046-
// Step 5: Verify the bot comment was also updated to show "CLA Signed"
1048+
// Step 5: Verify stale CLA comment is deleted after signing.
10471049
const getRes = await fetch(
10481050
`${baseUrl}/api/webhook/github?orgSlug=fiveonefour&repoName=sdk&prNumber=20`
10491051
)
10501052
const getData = await getRes.json()
1051-
assert(
1052-
getData.comment === null || getData.comment.commentMarkdown.includes("CLA Signed"),
1053-
"comment updated to signed status"
1053+
assertEqual(getData.comment, null, "stale comment deleted")
1054+
})
1055+
1056+
test("Webhook: signing without repo/pr still updates open PR check and deletes stale comment", async (baseUrl) => {
1057+
await resetDb(baseUrl)
1058+
1059+
// Step 1: contributor1 opens PR while signed current -> success
1060+
await sendWebhook(
1061+
baseUrl,
1062+
"pull_request",
1063+
makePrPayload({
1064+
action: "opened",
1065+
prAuthor: "contributor1",
1066+
orgSlug: "fiveonefour",
1067+
repoName: "sdk",
1068+
prNumber: 77,
1069+
})
1070+
)
1071+
1072+
// Step 2: CLA update makes signature stale, /recheck forces failure + prompt comment.
1073+
await updateClaForOrg("fiveonefour", "# Updated CLA v2 for global sign sync")
1074+
const { data: recheckData } = await sendWebhook(baseUrl, "issue_comment", {
1075+
action: "created",
1076+
comment: { body: "/recheck", user: { login: "orgadmin" } },
1077+
issue: {
1078+
number: 77,
1079+
user: { login: "contributor1", id: 1002 },
1080+
pull_request: { url: "https://api.github.com/repos/fiveonefour/sdk/pulls/77" },
1081+
},
1082+
repository: { owner: { login: "fiveonefour" }, name: "sdk" },
1083+
installation: { id: 11111 },
1084+
})
1085+
assertEqual(recheckData.check.status, "failure", "recheck fails after CLA update")
1086+
assert(recheckData.comment?.id, "stale comment is present before signing")
1087+
1088+
// Step 3: Sign WITHOUT repo/pr context.
1089+
await switchRole(baseUrl, "contributor")
1090+
const signRes = await fetch(`${baseUrl}/api/sign`, {
1091+
method: "POST",
1092+
headers: { "Content-Type": "application/json" },
1093+
body: JSON.stringify({ orgSlug: "fiveonefour" }),
1094+
})
1095+
const signData = await signRes.json()
1096+
assertEqual(signRes.status, 200, "sign succeeded")
1097+
assert(signData.updatedChecks.length > 0, "open PR check auto-updated")
1098+
assert(signData.deletedCommentIds.length > 0, "stale comment deleted")
1099+
1100+
// Step 4: Verify no CLA bot comment remains on that PR.
1101+
const getRes = await fetch(
1102+
`${baseUrl}/api/webhook/github?orgSlug=fiveonefour&repoName=sdk&prNumber=77`
10541103
)
1104+
const getData = await getRes.json()
1105+
assertEqual(getData.comment, null, "no CLA comment remains after signing")
10551106
})
10561107

10571108
// -- Scenario 3: Non-member, signed old version -> red check + re-sign comment --
@@ -1113,6 +1164,13 @@ test("Webhook: re-sign flow -- sign auto-updates check + comment", async (baseUr
11131164
assertEqual(signRes.status, 200, "re-sign succeeded")
11141165
assert(signData.updatedChecks.length > 0, "check auto-updated after re-sign")
11151166
assertEqual(signData.updatedChecks[0].conclusion, "success", "check is now success")
1167+
assert(signData.deletedCommentIds.length > 0, "re-sign prompt comment deleted")
1168+
1169+
const getRes = await fetch(
1170+
`${baseUrl}/api/webhook/github?orgSlug=fiveonefour&repoName=sdk&prNumber=30`
1171+
)
1172+
const getData = await getRes.json()
1173+
assertEqual(getData.comment, null, "stale re-sign comment removed")
11161174
})
11171175

11181176
// -- Scenario 4: Non-member, signed latest version -> green check, NO comment --

0 commit comments

Comments
 (0)