ci: add first-party CLA action (replaces archived contributor-assistant) - #6
Conversation
Deps pinned to the CommonJS majors (@actions/core@^1.11.1, @actions/github@^6.0.0): the current 3.x/9.x majors are ESM-only (type: module), which conflicts with the plan's node24 CommonJS constraint. Known npm-audit findings via @actions/http-client@2.x -> undici@5 are accepted for now (client talks only to api.github.com); the ESM/dep-modernization is an explicit follow-up. Advances: IS-580 🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
Serialization mirrors the live cla.json byte format (2-space indent, no trailing newline — verified against production). 🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
… check - Wrap the CLI entry in try/catch so a missing required input reaches core.setFailed instead of a raw stack trace. - dist-freshness check uses git status --porcelain (catches new untracked files under dist/, not only modified tracked ones). - Kept 422 in the signature-write retry deliberately: a concurrent bootstrap create returns 422 (sha wasn't supplied) and the refetch+retry path is exactly what recovers it (spec §4.5). 🤖 Generated by [robots](https://vyos.io)
🤖 Generated by [robots](https://vyos.io)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (5)
📜 Recent review details🧰 Additional context used🔍 Remote MCP Context7Relevant PR review context
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a GitHub Action for CLA checks with webhook parsing, signature matching and updates, status comment management, rerun triggering, test coverage, and a pull-request CI workflow. ChangesCLA Action Implementation
Related PRs: None identified. Suggested labels: ci, github-actions, new-feature Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/actions/cla/src/signatures.js (1)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable
throw lastErrorat line 47.With
attempt <= 3and the retry guardattempt < 3, the third attempt's failure always falls tothrow e;inside thecatch(line 44) — the loop never exits normally, sothrow lastError;after the loop can never execute. Harmless today but confusing for future maintainers reasoning about the retry-limit enforcement.🧹 Optional cleanup
- let lastError; for (let attempt = 1; attempt <= 3; attempt += 1) { const cur = await loadSignatures(octokit, cfg); if (cur.entries.some((e) => e.id === entry.id)) return { written: false, reason: 'already-signed' }; const next = { signedContributors: [...cur.entries, entry] }; const params = { owner: cfg.owner, repo: cfg.repo, path: cfg.path, branch: cfg.branch, message: `@${entry.name} has signed the CLA in ${calling.owner}/${calling.repo}#${entry.pullRequestNo}`, content: Buffer.from(JSON.stringify(next, null, 2)).toString('base64'), }; if (cur.sha) params.sha = cur.sha; try { await octokit.rest.repos.createOrUpdateFileContents(params); return { written: true }; } catch (e) { - lastError = e; if ((e.status === 409 || e.status === 422) && attempt < 3) continue; throw e; } } - throw lastError; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/cla/src/signatures.js around lines 22 - 48, The final throw in appendSignature is unreachable because the retry loop always either returns or throws inside the catch on the last attempt. Simplify the retry flow in signatures.js by removing the dead post-loop `throw lastError` and keeping the `octokit.rest.repos.createOrUpdateFileContents` error handling in the existing `catch` block, with `attempt`/`lastError` only used if you still need retry state..github/actions/cla/src/committers.js (1)
25-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAllowlist matching is case-sensitive.
parseAllowlist/filterAllowlistedcompare logins with exact-caseSet.has. GitHub logins are case-preserving but effectively case-insensitive at signup, so a casing mismatch between the configured allowlist and the API-reported login silently defeats the exemption — the test fixture at.github/actions/cla/test/committers.test.js:58already has to list bothcopilotandCopilotto work around this. Normalizing both sides to lower case removes the need for duplicate entries and avoids future misses when a new bot login is added with only one casing.♻️ Suggested normalization
function parseAllowlist(raw) { - return new Set(String(raw || '').split(',').map((s) => s.trim()).filter(Boolean)); + return new Set(String(raw || '').split(',').map((s) => s.trim().toLowerCase()).filter(Boolean)); } function filterAllowlisted(accounts, allow) { const out = new Map(); for (const [id, login] of accounts) { - if (!allow.has(login)) out.set(id, login); + if (!allow.has(login.toLowerCase())) out.set(id, login); } return out; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/cla/src/committers.js around lines 25 - 35, The allowlist check in parseAllowlist/filterAllowlisted is case-sensitive, so matching GitHub logins can be missed when casing differs. Normalize allowlist entries and account logins to a consistent case, ideally lower case, before storing and comparing them in the Set/Map logic. Update the matching flow in parseAllowlist and filterAllowlisted so login exemptions work regardless of casing and duplicate entries like Copilot/copilot are unnecessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/cla/src/context.js:
- Around line 8-23: The recheck path in context parsing currently accepts any
exact RECHECK_PHRASE comment, which lets untrusted commenters trigger reruns.
Update context.js in the issue_comment handling to gate the `recheck` action by
trusted actor checks, such as PR author or approved collaborator association,
before returning `{ kind: 'recheck' }`. Make sure main.js only reaches
triggerPrHeadRerun for recheck events that passed this authorization in
context.js.
In @.github/actions/cla/src/signatures.js:
- Around line 1-10: The loadSignatures() helper is assuming repos.getContent
always returns base64-encoded file content, which breaks once the signatures
file exceeds the contents API threshold. Update loadSignatures() to use the raw
media type or the blobs API when fetching the file, and keep the
JSON.parse(Buffer.from(...)) path only for confirmed base64 content. Use the
existing loadSignatures, octokit.rest.repos.getContent, and
res.data.content/res.data.sha flow to locate the fix, and make the non-base64
case fail explicitly instead of silently parsing bad data.
---
Nitpick comments:
In @.github/actions/cla/src/committers.js:
- Around line 25-35: The allowlist check in parseAllowlist/filterAllowlisted is
case-sensitive, so matching GitHub logins can be missed when casing differs.
Normalize allowlist entries and account logins to a consistent case, ideally
lower case, before storing and comparing them in the Set/Map logic. Update the
matching flow in parseAllowlist and filterAllowlisted so login exemptions work
regardless of casing and duplicate entries like Copilot/copilot are unnecessary.
In @.github/actions/cla/src/signatures.js:
- Around line 22-48: The final throw in appendSignature is unreachable because
the retry loop always either returns or throws inside the catch on the last
attempt. Simplify the retry flow in signatures.js by removing the dead post-loop
`throw lastError` and keeping the
`octokit.rest.repos.createOrUpdateFileContents` error handling in the existing
`catch` block, with `attempt`/`lastError` only used if you still need retry
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 693ad824-807b-43c6-b1b0-e146c3406d46
⛔ Files ignored due to path filters (3)
.github/actions/cla/dist/index.jsis excluded by!**/dist/**,!**/dist/**.github/actions/cla/dist/licenses.txtis excluded by!**/dist/**,!**/dist/**.github/actions/cla/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (19)
.github/actions/cla/.gitignore.github/actions/cla/action.yml.github/actions/cla/package.json.github/actions/cla/src/comment.js.github/actions/cla/src/committers.js.github/actions/cla/src/constants.js.github/actions/cla/src/context.js.github/actions/cla/src/main.js.github/actions/cla/src/rerun.js.github/actions/cla/src/signatures.js.github/actions/cla/test/comment.test.js.github/actions/cla/test/committers.test.js.github/actions/cla/test/constants.test.js.github/actions/cla/test/context.test.js.github/actions/cla/test/fixtures/legacy-comment.md.github/actions/cla/test/main.test.js.github/actions/cla/test/rerun.test.js.github/actions/cla/test/signatures.test.js.github/workflows/action-ci.yml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Rollout 1c (Phorge T8943, 2026-05-30) renamed default branches fleet-wide: release-train reposcurrent->rolling;vyos/.githuband other non-release-train reposcurrent->production. (1) Reusable-workflow refs of the formvyos/.github/.github/workflows/<name>.yml@productionare CORRECT and canonical. Do NOT suggest changing@productionto@current:currentis the OLD name, retained only as a temporary bake-period alias and slated for removal. Any AGENTS.md still saying "reusables pinned to@current" or "current (rolling)" is stale post-1c and must not drive review suggestions. (2) In the mirror callerpr-mirror-repo-sync.yml,permissions: contents: readis INTENTIONAL: the central reusable workflow performs all push/PR writes with thevyos-botGitHub App installation token (minted via theget-tokenaction), not the inheritedGITHUB_TOKEN. Do NOT suggest broadening the caller'spermissionstocontents: write/pull-requests: write.
Files:
.github/workflows/action-ci.yml
🪛 ast-grep (0.44.0)
.github/actions/cla/test/comment.test.js
[warning] 7-7: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(__dirname, 'fixtures', 'legacy-comment.md'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🔍 Remote MCP
Relevant docs-backed context:
- GitHub metadata syntax explicitly supports JavaScript actions with
runs.using: 'node24', andruns.mainis the file executed by that runtime. (docs.github.com) - The PR commits endpoint is
GET /repos/{owner}/{repo}/pulls/{pull_number}/commits;per_pageis capped at 100, so pagination is needed for large PRs. The example response includesauthor/committerobjects withloginandidfields. (docs.github.com) - GitHub’s Contents API
Create or update file contentsrequiresshawhen updating an existing file, uses base64 content, defaultsbranchto the repo default branch, and can return409or422; GitHub also warns that parallel create/update and delete requests conflict. (docs.github.com) - Workflow runs can be re-run, including failed jobs, up to 30 days after the original run; the
rerun-failed-jobsendpoint requiresrun_idand returns201 Created. (docs.github.com) - GitHub’s JavaScript action guide says to install
@actions/coreand@actions/github, and not to commitnode_modules/. (docs.github.com)
🔇 Additional comments (24)
.github/actions/cla/action.yml (2)
27-29: Summary says "composite Action"; manifest defines a JavaScript action.
runs.using: 'node24'withruns.main: 'dist/index.js'is a JavaScript action, not a composite action (runs.using: 'composite'+steps). The AI-generated summary's description doesn't match the actual manifest shape.
1-30: LGTM!.github/actions/cla/package.json (1)
1-19: LGTM!.github/actions/cla/.gitignore (1)
1-2: LGTM!.github/actions/cla/src/constants.js (1)
1-8: LGTM!.github/actions/cla/test/constants.test.js (1)
1-12: LGTM!.github/actions/cla/src/context.js (1)
3-25: LGTM!.github/actions/cla/test/context.test.js (1)
1-53: LGTM!.github/workflows/action-ci.yml (2)
1-21: LGTM!Also applies to: 28-38
22-27: 🩺 Stability & AvailabilityNo action needed.
.github/actions/cla/package-lock.jsonis present, sonpm ciandcache-dependency-pathpoint at a real lockfile.> Likely an incorrect or invalid review comment..github/actions/cla/src/committers.js (1)
1-23: LGTM!.github/actions/cla/test/committers.test.js (1)
1-71: LGTM!.github/actions/cla/src/signatures.js (1)
12-20: LGTM!.github/actions/cla/test/signatures.test.js (1)
1-99: LGTM!.github/actions/cla/src/comment.js (1)
36-83: Marker/legacy dedup and TOCTOU cleanup logic looks correct.Bot-authorship check on Line 40 correctly guards against spoofed markers; newest-marked-wins with duplicate cleanup (Lines 47-49, 75-81) and legacy adoption fallback (Lines 50-55) match the documented spec (§4.1-§4.4) and are covered by the test suite. Non-404 errors on delete are surfaced via
warnrather than swallowed (Line 79), and 404s on both update/delete are treated as already-resolved states.One edge case: if
updateCommentthrows a non-404 error (Line 62-63), the function re-throws before reaching the duplicate-cleanup loop (Lines 75-81), so stale duplicates aren't pruned on that run. This is self-healing on the next successful run, so not flagging as a blocking issue..github/actions/cla/test/comment.test.js (1)
1-133: LGTM!.github/actions/cla/test/fixtures/legacy-comment.md (1)
1-1: LGTM!.github/actions/cla/src/rerun.js (2)
4-6: 🎯 Functional CorrectnessConfirm
event: 'pull_request_target'filter matches the reusable workflow's actual trigger once wired in.Octokit method names/params (
getWorkflowRun,listWorkflowRunswithworkflow_id/event/head_sha,reRunWorkflowFailedJobswith fallback toreRunWorkflow) match the documented REST API surface. However, candidate lookup is hardcoded toevent: 'pull_request_target'(Line 5), coupling correctness tocla-reusable.yml's trigger config for the CLA check job — that file is intentionally unchanged/out of scope for this PR. If the workflow that ultimately calls this action triggers the CLA check job under a different event name (or via a reusable-workflow indirection that changes the recordedevent), no candidate run will ever be found andsign/recheckunblock reruns will silently no-op (status: 'skipped').Worth a final check against
cla-reusable.ymlbefore the planned workflow swap.
1-23: LGTM on the rest of the implementation — clean fallback and error-containment logic (never throws, matches test coverage for 409 fallback and unexpected API exceptions)..github/actions/cla/test/rerun.test.js (1)
1-79: LGTM!.github/actions/cla/src/main.js (3)
67-68: 🎯 Functional CorrectnessConfirm
repoOctokit's token carriesactions: writeandpull-requests: writeonce wired into the real workflow.
triggerPrHeadRerun(called here withrepoOctokit) invokesreRunWorkflowFailedJobs/reRunWorkflow, andupsertStatusCommentinvokesissues.createComment/updateComment/deleteComment— both require write-scoped permissions on the token (actions: writefor reruns per the REST API docs,pull-requests/issues: writefor comments). Sincecla-reusable.ymlisn't part of this PR and itspermissions:block isn't visible here, this can't be confirmed from the provided files. If the reusable workflow only grantscontents: read(as intentionally done for the mirror-sync workflow per repo convention), the sign/recheck rerun and comment-posting paths will silently degrade to warnings/status:'failed'(Lines 53-55, 68) rather than hard-failing — worth validating during the canary end-to-end pass mentioned in the PR objectives.
78-79: 📐 Maintainability & Code Quality | 💤 Low valueVerify
GITHUB_TOKENsourcing convention matchesaction.ymlinputs.
ghTokenis read directly fromprocess.env.GITHUB_TOKENrather than viacore.getInput(...), whilesignature-token(Line 89) uses the input mechanism. This is a valid pattern if the calling workflow step setsGITHUB_TOKENas an env var (matching the archivedcontributor-assistantaction's convention), butaction.yml's declared inputs aren't in this review cohort to confirm consistency.
1-70: Orchestration logic is sound.Context-kind branching (noop/check/sign/recheck), sign-then-reload sequencing (Lines 33-45), fail/pass determination (Line 47), non-fatal comment-upsert handling (Lines 49-55), and scoped rerun triggering only for
sign/recheck(Lines 66-69) all match the documented spec and are exercised by the accompanying test suite (all-signed no-comment, unsigned-fails, sign-flow write+rerun, closed/noop, comment-failure resilience)..github/actions/cla/test/main.test.js (1)
1-198: LGTM!
…eview rec) 🤖 Generated by [robots](https://vyos.io)
Adversarial review —
|
… case-insensitive allowlist, dead-code cleanup - loadSignatures fails with an explicit error if the contents API stops inlining base64 (file-size threshold) instead of a confusing parse crash — the raw/blob-API switch stays a future change (file is ~16 KB). - Allowlist matching normalized to lower case (GitHub logins are case-insensitively unique; the live list's copilot/Copilot duplication was a workaround for case-sensitive matching). - Removed unreachable throw after the append retry loop. 🤖 Generated by [robots](https://vyos.io)
|
@coderabbitai — disposition of the two 🧹 nitpicks from the review body (no threads to reply in), both addressed in
Same commit also carries the loud large-file guard from the inline thread. Suite: 54/54. |
|
Head-map update for the adversarial-review record above: reviewed head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show 3190c7f --stat 2>/dev/null || echo "commit not found locally"
echo "---signatures.js---"
sed -n '1,60p' .github/actions/cla/src/signatures.js
echo "---committers.js---"
sed -n '1,50p' .github/actions/cla/src/committers.jsLength of output: 5964
Also noted the added 54/54 noted. No further action needed on these two items. |
evgsentrium
left a comment
evgsentrium
left a comment
There was a problem hiding this comment.
Replacement of the archived contributor-assistant
2 participants
What
First-party node24 JS action at
.github/actions/cla/implementing the exact CLA-check subset the fleet uses (spec + plan both dual-gated Codex+agy). Does NOT changecla-reusable.yml— the swap is a follow-up PR after a canary e2e.Covers: committer collection with per-role unlinked-identity rules (
web-flow/noreply@github.comexemptions), trim-safe allowlist, signature match by numeric id againstsignatures/version1/cla.json(schema + byte format unchanged — 2-space indent, no trailing newline, verified against production), single marker+bot-author status comment with legacy contributor-assistant adoption + non-blocking dedupe, byte-exact sign/recheck comment flows with idempotent SHA-retried signature writes, workflow-scoped PR-head unblock re-run (the mechanism behind today'srun_attempt: 2flips), file bootstrap, closed/non-PR no-ops. All-signed PRs get NO bot comment (edit-only-if-exists, matching upstream UX).Tests: 52,
node:test, mocked octokit, no network. CI:action-ci.yml(tests + dist freshness viagit status --porcelain, SHA-pinned checkout/setup-node,persist-credentials: false).Notable decisions
@actions/core@^1.11.1+@actions/github@^6.0.0: current 3.x/9.x majors are ESM-only (type: module), conflicting with the CJS design constraint. Known npm-audit findings via@actions/http-client@2.x→undici@5accepted (client talks only to api.github.com); ESM/dep modernization is an explicit follow-up.Why
contributor-assistant/github-actionis archived (read-only since 2026-03) — frozen code on a merge-blocking fleet-wide check across ~56 repos.Rollout (after this PR)
Canary e2e on a throwaway branch +
vyos/gh-action-test, then a one-line swap PR incla-reusable.yml. Rollback at any point = revert to the SHA-pinned archived action.Advances: IS-580
🤖 Generated by robots