diff --git a/scripts/lib/upstream-drift.js b/scripts/lib/upstream-drift.js index fede71a..a9513c1 100644 --- a/scripts/lib/upstream-drift.js +++ b/scripts/lib/upstream-drift.js @@ -97,48 +97,63 @@ function formatIssueTitle({ count, lastSyncedShaShort }) { * Body for the rolling tracking issue. Truncates the commit list at * COMMIT_BODY_LIMIT entries and links to the compare URL for the full diff. * + * `totalCount` is the true number of commits upstream is ahead by + * (i.e. `compare.ahead_by` from the GitHub API). `commits.length` + * can be smaller because the compare endpoint caps responses at 250 + * commits regardless of the true delta. Using `commits.length` in + * the intro would silently understate the drift when the API caps + * (e.g. show "250 commits ahead" when the truth is 1521). + * * @param {{ * commits: Array<{ sha: string, author: string, message: string }>, * lastSyncedSha: string, * upstreamHeadSha: string, * upstreamRepo: string, + * totalCount: number, * }} args * @returns {string} */ -function formatIssueBody({ commits, lastSyncedSha, upstreamHeadSha, upstreamRepo }) { +function formatIssueBody({ commits, lastSyncedSha, upstreamHeadSha, upstreamRepo, totalCount }) { if (!Array.isArray(commits)) { throw new Error('formatIssueBody: commits must be an array'); } if (!REPO_RE.test(upstreamRepo)) { throw new Error(`formatIssueBody: upstreamRepo must be "owner/repo" (got: ${upstreamRepo})`); } + if (typeof totalCount !== 'number' || totalCount < 0 || !Number.isInteger(totalCount)) { + throw new Error(`formatIssueBody: totalCount must be a non-negative integer (got: ${totalCount})`); + } const compareUrl = `https://github.com/${upstreamRepo}/compare/${lastSyncedSha}...${upstreamHeadSha}`; const lines = []; - lines.push(`Upstream \`${upstreamRepo}\` has **${commits.length}** commit(s) ahead of the recorded baseline.`); + lines.push(`Upstream \`${upstreamRepo}\` has **${totalCount}** commit(s) ahead of the recorded baseline.`); lines.push(''); lines.push(`- **Baseline**: \`${shortSha(lastSyncedSha)}\` (recorded in [\`upstream/.upstream-sync.json\`](../blob/main/upstream/.upstream-sync.json))`); lines.push(`- **Upstream HEAD**: \`${shortSha(upstreamHeadSha)}\``); lines.push(`- **Full diff**: ${compareUrl}`); lines.push(''); - if (commits.length === 0) { + if (totalCount === 0) { lines.push('_No commits to show._'); lines.push(''); } else { const visible = commits.slice(0, COMMIT_BODY_LIMIT); - const truncated = commits.length - visible.length; + const omitted = totalCount - visible.length; lines.push('
'); - lines.push(`Commits (${commits.length} total${truncated > 0 ? `, showing first ${visible.length}` : ''})`); + if (omitted === 0) { + lines.push(`Commits (${totalCount})`); + } else { + lines.push(`Commits (showing first ${visible.length} of ${totalCount})`); + } lines.push(''); for (const c of visible) { const firstLine = (c.message || '').split('\n')[0].trim(); lines.push(`- \`${shortSha(c.sha)}\` ${c.author ? `(@${c.author}) ` : ''}${firstLine}`); } - if (truncated > 0) { - lines.push(`- _… and ${truncated} more. See the [full diff](${compareUrl})._`); + if (omitted > 0) { + lines.push(`- _… and ${omitted} more. See the [full diff](${compareUrl})._`); } lines.push(''); lines.push('
'); diff --git a/scripts/upstream/check-upstream-drift.js b/scripts/upstream/check-upstream-drift.js index 6baabce..31aa2ae 100644 --- a/scripts/upstream/check-upstream-drift.js +++ b/scripts/upstream/check-upstream-drift.js @@ -179,6 +179,9 @@ function buildIssueContent(state, compare) { lastSyncedSha: state.lastSyncedSha, upstreamHeadSha: headSha, upstreamRepo: state.upstream, + // compare.ahead_by is the true delta even when commits[] is + // capped at 250 by the GitHub compare endpoint. + totalCount: compare.ahead_by, }); return { title, body }; } diff --git a/tests/lib/upstream-drift.test.js b/tests/lib/upstream-drift.test.js index bd0ba5d..4684078 100644 --- a/tests/lib/upstream-drift.test.js +++ b/tests/lib/upstream-drift.test.js @@ -151,6 +151,7 @@ test('formatIssueBody includes baseline, head, compare URL', () => { lastSyncedSha: '9db98673d054f5ed0991ba9d67ff4c883c81a42f', upstreamHeadSha: 'bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222', upstreamRepo: 'affaan-m/everything-claude-code', + totalCount: 1, }); assert.ok(body.includes('9db9867'), 'should contain baseline short sha'); assert.ok(body.includes('bbbb222'), 'should contain head short sha'); @@ -160,19 +161,22 @@ test('formatIssueBody includes baseline, head, compare URL', () => { assert.ok(!body.includes('body'), 'should not include commit message body, only first line'); }); -test('formatIssueBody handles empty commit list', () => { +test('formatIssueBody handles empty commit list (totalCount=0)', () => { const body = lib.formatIssueBody({ commits: [], lastSyncedSha: '0'.repeat(40), upstreamHeadSha: '1'.repeat(40), upstreamRepo: 'a/b', + totalCount: 0, }); assert.ok(body.includes('_No commits to show._')); + assert.ok(body.includes('**0**')); }); -test('formatIssueBody truncates beyond COMMIT_BODY_LIMIT', () => { +test('formatIssueBody truncates display beyond COMMIT_BODY_LIMIT (totalCount === commits.length)', () => { const many = []; - for (let i = 0; i < lib.COMMIT_BODY_LIMIT + 25; i += 1) { + const N = lib.COMMIT_BODY_LIMIT + 25; + for (let i = 0; i < N; i += 1) { many.push({ sha: i.toString(16).padStart(40, '0'), author: 'a', @@ -184,26 +188,78 @@ test('formatIssueBody truncates beyond COMMIT_BODY_LIMIT', () => { lastSyncedSha: '0'.repeat(40), upstreamHeadSha: '1'.repeat(40), upstreamRepo: 'a/b', + totalCount: N, }); assert.ok(body.includes('… and 25 more')); - assert.ok(body.includes(`(${lib.COMMIT_BODY_LIMIT + 25} total`)); - assert.ok(!body.includes(`msg ${lib.COMMIT_BODY_LIMIT + 24}`), 'last commit should not be rendered'); + assert.ok(body.includes(`showing first ${lib.COMMIT_BODY_LIMIT} of ${N}`)); + assert.ok(!body.includes(`msg ${N - 1}`), 'last commit should not be rendered'); +}); + +test('formatIssueBody uses totalCount when API caps commits (totalCount > commits.length)', () => { + // Real-world case: ahead_by=1521 but compare endpoint capped commits[] at 250. + // Intro and summary must report 1521, not 250. + const commits = []; + for (let i = 0; i < 250; i += 1) { + commits.push({ + sha: i.toString(16).padStart(40, '0'), + author: 'a', + message: `msg ${i}`, + }); + } + const body = lib.formatIssueBody({ + commits, + lastSyncedSha: '0'.repeat(40), + upstreamHeadSha: '1'.repeat(40), + upstreamRepo: 'a/b', + totalCount: 1521, + }); + assert.ok(body.includes('**1521** commit(s) ahead'), 'intro should use totalCount, not commits.length'); + assert.ok(body.includes(`showing first ${lib.COMMIT_BODY_LIMIT} of 1521`), 'summary should reference totalCount'); + assert.ok(body.includes(`… and ${1521 - lib.COMMIT_BODY_LIMIT} more`), 'truncation must reflect totalCount'); + assert.ok(!body.includes('250'), 'API window size 250 should never appear in the body'); +}); + +test('formatIssueBody omits the truncation line when total fits in display', () => { + const commits = [ + { sha: 'a'.repeat(40), author: 'x', message: 'm1' }, + { sha: 'b'.repeat(40), author: 'y', message: 'm2' }, + ]; + const body = lib.formatIssueBody({ + commits, + lastSyncedSha: '0'.repeat(40), + upstreamHeadSha: '1'.repeat(40), + upstreamRepo: 'a/b', + totalCount: 2, + }); + assert.ok(body.includes('Commits (2)'), 'summary should be the simple form when nothing is omitted'); + assert.ok(!body.includes('more.'), 'no truncation note when omitted === 0'); }); test('formatIssueBody rejects non-array commits', () => { assert.throws( - () => lib.formatIssueBody({ commits: null, lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'a/b' }), + () => lib.formatIssueBody({ commits: null, lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'a/b', totalCount: 0 }), /must be an array/, ); }); test('formatIssueBody rejects malformed upstreamRepo', () => { assert.throws( - () => lib.formatIssueBody({ commits: [], lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'no-slash' }), + () => lib.formatIssueBody({ commits: [], lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'no-slash', totalCount: 0 }), /owner\/repo/, ); }); +test('formatIssueBody rejects non-integer or negative totalCount', () => { + assert.throws( + () => lib.formatIssueBody({ commits: [], lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'a/b', totalCount: -1 }), + /non-negative integer/, + ); + assert.throws( + () => lib.formatIssueBody({ commits: [], lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'a/b', totalCount: 1.5 }), + /non-negative integer/, + ); +}); + // extractCommitsForBody test('extractCommitsForBody flattens the compare API shape', () => {