mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-06 21:05:21 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work, and it takes contributions from outside the core team > - Those contributions arrive as PRs, and this repository squash-merges every one of them > - A squash collapses the whole branch into a single commit authored by whoever pressed the button > - So when a maintainer rebases and lands a contributor's stale PR, the contributor's name survives only if the squash message carries a `Co-Authored-By` trailer > - Nothing prompts for that trailer, and the PR page keeps showing the original author either way, so losing it is invisible at the moment it happens > - This pull request has commitperclip detect the situation and print the exact trailers to paste > - The benefit is that keeping an outside contributor's name is a default rather than something a maintainer has to remember ## Linked Issues or Issue Description No public issue exists. The problem follows, and it is not hypothetical. **What happened?** #11370, #11371 and #11379 landed @stubbi's work yesterday. Each of those PRs carries a comment from me telling them their authorship would be preserved. All three squash commits went in without a `Co-Authored-By` trailer, so `git log` credits none of them: | commit | landed from | credited | | --- | --- | --- | | `66515582e` | #9900 | Claude only | | `bc0b5a164` | #9501 | Claude only | | `35a9b9873` | #8982 | Claude only | | `6542ad1f4` | #11259 | ✅ Jannes Stubbemann + Claude | The last one has the trailer because that message was written by hand with the contributor in mind. The only difference between the two outcomes was memory. Master history cannot be rewritten, so those three are now credited by comment on the original PRs — which is a worse record than a commit trailer, and the reason to make this automatic. **Expected behavior** When a branch carries commits by someone other than the PR author, the merger is told what trailers the squash needs. **Paperclip version or commit** `master` at `92047cac4`. ## What Changed - `.github/scripts/check-pr-coauthors.mjs` — new gate. - `.github/scripts/run-quality-gates.mjs` — fetches the PR's commits and runs it. - `.github/scripts/tests/check-pr-coauthors.test.mjs` — 12 cases. - `.github/workflows/pr.yml` — runs `.github/scripts/tests/`. ### Informational, not a failure The squash message does not exist while the PR is open. This can neither be verified there nor fixed there, so failing a PR on it would block work on something its author cannot satisfy. The gate notices that the situation applies and prints the lines to paste. Run against #11370's actual commits it produces exactly what was missing: ``` This branch carries commits by stubbi. Squash-merging drops that authorship unless the squash message carries their trailers, and nothing else will notice if it does not. Add to the squash body when merging: Co-Authored-By: Jannes Stubbemann <stubbi@users.noreply.github.com> ``` ### Edge cases it handles Bots skipped; the PR author's own commits skipped; logins compared case-insensitively (`PR_AUTHOR` does not always arrive in the same case as the commit author login); each contributor listed once however many commits they wrote; and a commit GitHub could not match to an account falls back to its raw git author — that identity being the one most likely to be lost, not least likely. Paging stops at the API's own 250-commit ceiling rather than spinning on full pages of nothing new. ### The test directory was not running `.github/scripts/tests/` held ten test files covering the existing gates, and no workflow ran any of them. Adding an eleventh would have meant adding a test that never executes, so `pr.yml` now runs the directory. All **149** pass, including the 137 that were already there and previously unverified in CI. ## Verification - 149 tests pass via `node --test '.github/scripts/tests/*.test.mjs'` — the exact command CI now runs. - The gate was run against the real commit shape from #11370 and produces the missing trailer verbatim. This PR is its own negative control: the branch carries only my commits, so the new gate should stay silent on it. If commitperclip prints a co-author note below, the gate is wrong. ## Risks Low. Informational output only — it cannot fail a PR, and `allPassed` is unchanged. It adds one API call per gate run (`/pulls/{n}/commits`), fetched in the same `Promise.all` as the existing PR and files calls. Enabling the previously-unrun test directory could in principle surface a pre-existing failure; all 149 pass locally, so it does not. Revert the commit to restore. ## Model Used Claude Opus 5 (`claude-opus-5`), through Claude Code. Extended thinking enabled. Tool use enabled: file read and edit, shell for test runs. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal ticket id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
180 lines
6.7 KiB
JavaScript
180 lines
6.7 KiB
JavaScript
import { test } from 'node:test';
|
|
import assert from 'node:assert/strict';
|
|
import { checkCoauthors, fetchAllPullRequestCommits } from '../check-pr-coauthors.mjs';
|
|
|
|
function commit(login, name = null, email = null) {
|
|
return {
|
|
author: login ? { login } : null,
|
|
commit: { author: { name: name ?? login, email: email ?? `${login}@users.noreply.github.com` } },
|
|
};
|
|
}
|
|
|
|
test('checkCoauthors: says nothing when every commit is the PR author\'s own', () => {
|
|
const result = checkCoauthors(
|
|
[commit('tonio-alucema'), commit('tonio-alucema')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.equal(result.passed, true);
|
|
assert.deepEqual(result.informational, []);
|
|
});
|
|
|
|
test('checkCoauthors: hands over the trailer when the branch carries someone else\'s commit', () => {
|
|
// The case this exists for: a stale contributor PR rebased and landed by a
|
|
// maintainer. Squash-merging drops the contributor unless the squash body
|
|
// carries their trailer.
|
|
const result = checkCoauthors(
|
|
[commit('stubbi', 'Jannes Stubbemann'), commit('tonio-alucema')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.equal(result.informational.length, 1);
|
|
assert.match(result.informational[0], /Jannes Stubbemann/);
|
|
assert.match(
|
|
result.informational[0],
|
|
/Co-Authored-By: Jannes Stubbemann <stubbi@users\.noreply\.github\.com>/
|
|
);
|
|
});
|
|
|
|
test('checkCoauthors: never fails the PR, because the squash message does not exist yet', () => {
|
|
// Informational only. The author of the PR cannot satisfy this from the PR,
|
|
// so failing here would block work on something unfixable at that point.
|
|
const result = checkCoauthors([commit('stubbi')], 'tonio-alucema');
|
|
|
|
assert.equal(result.passed, true);
|
|
});
|
|
|
|
test('checkCoauthors: matches the PR author case-insensitively', () => {
|
|
// GitHub logins are case-insensitive, and PR_AUTHOR does not always arrive
|
|
// in the same case as the commit author login.
|
|
const result = checkCoauthors([commit('Tonio-Alucema')], 'tonio-alucema');
|
|
|
|
assert.deepEqual(result.informational, []);
|
|
});
|
|
|
|
test('checkCoauthors: ignores bots', () => {
|
|
const result = checkCoauthors(
|
|
[commit('github-actions[bot]'), commit('dependabot[bot]')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.deepEqual(result.informational, []);
|
|
});
|
|
|
|
test('checkCoauthors: lists each contributor once, however many commits they wrote', () => {
|
|
const result = checkCoauthors(
|
|
[commit('stubbi', 'Jannes Stubbemann'), commit('stubbi', 'Jannes Stubbemann')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? [];
|
|
assert.equal(trailers.length, 1);
|
|
});
|
|
|
|
test('checkCoauthors: falls back to the raw git author when GitHub matched no account', () => {
|
|
// A commit authored with an email GitHub cannot resolve still deserves a
|
|
// trailer — that is precisely the identity most likely to be lost.
|
|
const result = checkCoauthors(
|
|
[{ author: null, commit: { author: { name: 'Ada Lovelace', email: 'ada@example.com' } } }],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.match(result.informational[0], /Co-Authored-By: Ada Lovelace <ada@example\.com>/);
|
|
});
|
|
|
|
test('checkCoauthors: skips an unattributable commit rather than emitting a broken trailer', () => {
|
|
const result = checkCoauthors(
|
|
[{ author: null, commit: { author: { name: 'Nameless', email: null } } }],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.deepEqual(result.informational, []);
|
|
});
|
|
|
|
test('checkCoauthors: names the count rather than everyone when several contributed', () => {
|
|
const result = checkCoauthors(
|
|
[commit('stubbi', 'Jannes Stubbemann'), commit('elJayAdvisor', 'LJ')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.match(result.informational[0], /2 other contributors/);
|
|
assert.match(result.informational[0], /Jannes Stubbemann/);
|
|
assert.match(result.informational[0], /LJ/);
|
|
});
|
|
|
|
test('checkCoauthors: tolerates a PR with no commits', () => {
|
|
assert.deepEqual(checkCoauthors([], 'tonio-alucema').informational, []);
|
|
assert.deepEqual(checkCoauthors(undefined, 'tonio-alucema').informational, []);
|
|
});
|
|
|
|
test('fetchAllPullRequestCommits: pages until a short batch', async () => {
|
|
const seen = [];
|
|
const commits = await fetchAllPullRequestCommits(async (path) => {
|
|
seen.push(path);
|
|
if (path.endsWith('page=1')) return Array.from({ length: 100 }, () => commit('stubbi'));
|
|
return [commit('tonio-alucema')];
|
|
}, 'paperclipai/paperclip', 9900, 'token');
|
|
|
|
assert.equal(commits.length, 101);
|
|
assert.equal(seen.length, 2);
|
|
});
|
|
|
|
test('fetchAllPullRequestCommits: stops at the API ceiling instead of looping', async () => {
|
|
// `/pulls/{n}/commits` caps at 250 and keeps returning full pages of nothing
|
|
// new past that. A branch that large is not what this gate is about, but it
|
|
// must not spin.
|
|
let calls = 0;
|
|
const commits = await fetchAllPullRequestCommits(async () => {
|
|
calls += 1;
|
|
return Array.from({ length: 100 }, () => commit('stubbi'));
|
|
}, 'paperclipai/paperclip', 9900, 'token');
|
|
|
|
assert.equal(calls, 3);
|
|
assert.equal(commits.length, 300);
|
|
});
|
|
|
|
test('checkCoauthors: counts one person once when their git name varies across commits', () => {
|
|
// People change their git config. Keying the dedup on the rendered trailer
|
|
// would put two lines for the same contributor into the squash body.
|
|
const result = checkCoauthors(
|
|
[commit('stubbi', 'Jannes Stubbemann'), commit('stubbi', 'J. Stubbemann')],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? [];
|
|
assert.equal(trailers.length, 1);
|
|
assert.doesNotMatch(result.informational[0], /other contributors/);
|
|
});
|
|
|
|
test('checkCoauthors: does not credit the PR author as a co-author of themselves', () => {
|
|
// Their own commit, authored with an email GitHub could not match to the
|
|
// account. Without the guard they appear in their own trailer list.
|
|
const byName = checkCoauthors(
|
|
[{ author: null, commit: { author: { name: 'tonio-alucema', email: 'tonio@example.com' } } }],
|
|
'tonio-alucema'
|
|
);
|
|
const byEmail = checkCoauthors(
|
|
[{ author: null, commit: { author: { name: 'Tonio', email: 'tonio-alucema@users.noreply.github.com' } } }],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
assert.deepEqual(byName.informational, []);
|
|
assert.deepEqual(byEmail.informational, []);
|
|
});
|
|
|
|
test('checkCoauthors: counts one person once when some commits matched their account and some did not', () => {
|
|
// The mixed case: GitHub resolved one commit to the login and left another
|
|
// unmatched, both carrying the same email. Keying on login alone emits two
|
|
// trailers for one contributor.
|
|
const result = checkCoauthors(
|
|
[
|
|
commit('stubbi', 'Jannes Stubbemann', 'jannes@example.com'),
|
|
{ author: null, commit: { author: { name: 'Jannes Stubbemann', email: 'jannes@example.com' } } },
|
|
],
|
|
'tonio-alucema'
|
|
);
|
|
|
|
const trailers = result.informational[0].match(/Co-Authored-By:/g) ?? [];
|
|
assert.equal(trailers.length, 1);
|
|
});
|