From 1b637a5b2dc04cf02275357a80c044b517d0ccac Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 12 May 2026 10:04:21 +0900 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20upstream=20drift=20detector=20(Phas?= =?UTF-8?q?e=201=20=E2=80=94=20log=20only)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the upstream-drift tracker (see #56 for the policy this implements). Reads upstream/.upstream-sync.json, calls the GitHub compare API for affaan-m/everything-claude-code, and logs whether upstream is ahead of the recorded baseline. Phase 2 will add issue management on a weekly cron; for now the workflow only runs on manual dispatch. What this lands: - scripts/lib/upstream-drift.js — pure functions, JSDoc-typed: parseUpstreamState (strict schema validation) shortSha formatIssueTitle (used in Phase 2; tested now) formatIssueBody (with COMMIT_BODY_LIMIT truncation; tested now) compareUrl - scripts/upstream/check-upstream-drift.js — entry script. Shells out to gh api compare. Tolerates 404 (upstream rename/archive/ deletion) with a warning and exit 0 so weekly cron does not produce red runs that nobody triages. maxBuffer is bumped to 50 MB because compare responses easily exceed Node's default 1 MB when the delta is large. - tests/lib/upstream-drift.test.js — 20 unit tests covering schema validation (good, malformed JSON, bad upstream, bad SHA, bad date, bad notes), pluralization, truncation, URL construction. - tests/run-all.js — registers the new test file. - .github/workflows/upstream-drift.yml — workflow_dispatch only, permissions: contents: read, no issues:write yet (Phase 2 will add it together with the cron trigger and label). Local smoke run against live upstream confirmed: ECC is currently 1517 commits ahead of the 2026-02-09 baseline; script prints the delta and the compare URL cleanly. Lint + tests both green (242/242). Workflow will be scanned by the existing validate-workflow-security.js — uses neither workflow_run nor pull_request_target, so it passes trivially. --- .github/workflows/upstream-drift.yml | 27 +++ scripts/lib/upstream-drift.js | 178 ++++++++++++++++++ scripts/upstream/check-upstream-drift.js | 136 ++++++++++++++ tests/lib/upstream-drift.test.js | 225 +++++++++++++++++++++++ tests/run-all.js | 1 + 5 files changed, 567 insertions(+) create mode 100644 .github/workflows/upstream-drift.yml create mode 100644 scripts/lib/upstream-drift.js create mode 100644 scripts/upstream/check-upstream-drift.js create mode 100644 tests/lib/upstream-drift.test.js diff --git a/.github/workflows/upstream-drift.yml b/.github/workflows/upstream-drift.yml new file mode 100644 index 0000000..99fa4b1 --- /dev/null +++ b/.github/workflows/upstream-drift.yml @@ -0,0 +1,27 @@ +name: Upstream Drift + +on: + workflow_dispatch: + +permissions: + contents: read + +jobs: + check: + name: Check upstream drift + runs-on: ubuntu-latest + timeout-minutes: 5 + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '20.x' + + - name: Check upstream drift + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: node scripts/upstream/check-upstream-drift.js diff --git a/scripts/lib/upstream-drift.js b/scripts/lib/upstream-drift.js new file mode 100644 index 0000000..b1ddd59 --- /dev/null +++ b/scripts/lib/upstream-drift.js @@ -0,0 +1,178 @@ +/** + * Upstream drift engine: pure functions used by the upstream-drift Action. + * + * `scripts/upstream/check-upstream-drift.js` calls these helpers and drives + * the `gh` CLI. Functions here have no I/O so they can be unit-tested in + * isolation against `tests/lib/upstream-drift.test.js`. + * + * State of record: + * - Human: upstream/README.md (Upstream baseline section) + * - Machine: upstream/.upstream-sync.json + * + * Schema for the JSON state file: + * { + * "upstream": "owner/repo", + * "lastSyncedSha": "<40-char SHA>", + * "lastSyncedAt": "", + * "notes": "optional free text" + * } + */ + +const SHA_RE = /^[0-9a-f]{40}$/; +const REPO_RE = /^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/; +const COMMIT_BODY_LIMIT = 50; + +/** + * Parse and validate the contents of `upstream/.upstream-sync.json`. + * + * Throws on any structural problem — the workflow should fail fast rather + * than silently treat a malformed state file as "no drift". + * + * @param {string} jsonString + * @returns {{ upstream: string, lastSyncedSha: string, lastSyncedAt: string, notes?: string }} + */ +function parseUpstreamState(jsonString) { + let parsed; + try { + parsed = JSON.parse(jsonString); + } catch (err) { + throw new Error(`upstream-sync state is not valid JSON: ${err.message}`, { cause: err }); + } + + if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new Error('upstream-sync state must be a JSON object'); + } + + const { upstream, lastSyncedSha, lastSyncedAt, notes } = parsed; + + if (typeof upstream !== 'string' || !REPO_RE.test(upstream)) { + throw new Error(`upstream must be a "owner/repo" string (got: ${JSON.stringify(upstream)})`); + } + + if (typeof lastSyncedSha !== 'string' || !SHA_RE.test(lastSyncedSha)) { + throw new Error(`lastSyncedSha must be a 40-char hex string (got: ${JSON.stringify(lastSyncedSha)})`); + } + + if (typeof lastSyncedAt !== 'string' || lastSyncedAt.length === 0) { + throw new Error(`lastSyncedAt must be a non-empty string (got: ${JSON.stringify(lastSyncedAt)})`); + } + + if (notes !== undefined && typeof notes !== 'string') { + throw new Error(`notes, when present, must be a string (got: ${typeof notes})`); + } + + return { upstream, lastSyncedSha, lastSyncedAt, notes }; +} + +/** + * Short SHA (first 7 chars) for display in titles and URLs. + * + * @param {string} sha + * @returns {string} + */ +function shortSha(sha) { + if (typeof sha !== 'string') return ''; + return sha.slice(0, 7); +} + +/** + * Title shown on the rolling tracking issue. Recomputed every run so the + * title itself encodes the current delta count. + * + * @param {{ count: number, lastSyncedShaShort: string }} args + * @returns {string} + */ +function formatIssueTitle({ count, lastSyncedShaShort }) { + if (typeof count !== 'number' || count < 0 || !Number.isInteger(count)) { + throw new Error(`formatIssueTitle: count must be a non-negative integer (got: ${count})`); + } + if (typeof lastSyncedShaShort !== 'string' || lastSyncedShaShort.length === 0) { + throw new Error('formatIssueTitle: lastSyncedShaShort must be a non-empty string'); + } + const noun = count === 1 ? 'commit' : 'commits'; + return `Upstream sync: ${count} new ${noun} in ECC since ${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. + * + * @param {{ + * commits: Array<{ sha: string, author: string, message: string }>, + * lastSyncedSha: string, + * upstreamHeadSha: string, + * upstreamRepo: string, + * }} args + * @returns {string} + */ +function formatIssueBody({ commits, lastSyncedSha, upstreamHeadSha, upstreamRepo }) { + 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})`); + } + + 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(''); + 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) { + lines.push('_No commits to show._'); + lines.push(''); + } else { + const visible = commits.slice(0, COMMIT_BODY_LIMIT); + const truncated = commits.length - visible.length; + + lines.push('
'); + lines.push(`Commits (${commits.length} total${truncated > 0 ? `, showing first ${visible.length}` : ''})`); + 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})._`); + } + lines.push(''); + lines.push('
'); + lines.push(''); + } + + lines.push('---'); + lines.push(''); + lines.push('To close this issue, advance the baseline by following the procedure in [`upstream/README.md`](../blob/main/upstream/README.md#recording-a-sync). This issue is maintained automatically by `.github/workflows/upstream-drift.yml`.'); + + return lines.join('\n'); +} + +/** + * Compare URL for the GitHub UI. Convenience export — also used by the + * entry script for the Phase 1 log output. + * + * @param {string} upstreamRepo + * @param {string} baseSha + * @param {string} headSha + * @returns {string} + */ +function compareUrl(upstreamRepo, baseSha, headSha) { + if (!REPO_RE.test(upstreamRepo)) { + throw new Error(`compareUrl: upstreamRepo must be "owner/repo" (got: ${upstreamRepo})`); + } + return `https://github.com/${upstreamRepo}/compare/${baseSha}...${headSha}`; +} + +module.exports = { + COMMIT_BODY_LIMIT, + parseUpstreamState, + shortSha, + formatIssueTitle, + formatIssueBody, + compareUrl, +}; diff --git a/scripts/upstream/check-upstream-drift.js b/scripts/upstream/check-upstream-drift.js new file mode 100644 index 0000000..9fa1ec0 --- /dev/null +++ b/scripts/upstream/check-upstream-drift.js @@ -0,0 +1,136 @@ +#!/usr/bin/env node + +/** + * Upstream drift detector — Phase 1 (log-only). + * + * Reads `upstream/.upstream-sync.json`, asks the GitHub API how many + * commits the upstream repo is ahead of the recorded baseline, and logs + * the result. Does NOT create or update GitHub issues yet — that lands + * in Phase 2. + * + * Designed to run in `.github/workflows/upstream-drift.yml`: + * - Uses `gh api` for upstream reads (public repo, no PAT needed). + * - Exits 0 on success and on tolerated upstream errors (rename, + * archive, deletion) so weekly cron does not produce red runs that + * nobody triages. + * - Exits non-zero only on programmer errors (malformed state file). + */ + +const fs = require('fs'); +const path = require('path'); +const { execSync } = require('child_process'); + +const lib = require('../lib/upstream-drift'); + +const REPO_ROOT = path.join(__dirname, '..', '..'); +const STATE_PATH = path.join(REPO_ROOT, 'upstream', '.upstream-sync.json'); + +function log(message) { + console.log(`[upstream-drift] ${message}`); +} + +function warn(message) { + console.error(`[upstream-drift] ${message}`); +} + +function readState() { + let raw; + try { + raw = fs.readFileSync(STATE_PATH, 'utf8'); + } catch (err) { + throw new Error(`Could not read state file at ${STATE_PATH}: ${err.message}`, { cause: err }); + } + return lib.parseUpstreamState(raw); +} + +/** + * Invoke `gh api` and return the parsed JSON response, or null when the + * upstream is unreachable (rename/archive/deletion). Other errors throw. + * + * @param {string} endpoint - path like "repos/owner/repo/compare/A...B" + * @returns {object|null} + */ +function ghApi(endpoint) { + let stdout; + try { + stdout = execSync(`gh api ${endpoint}`, { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + // The compare endpoint can return well over 1 MB of JSON when + // the delta includes hundreds of commits. Default maxBuffer is + // 1 MB and the process is killed with SIGTERM on overflow. + maxBuffer: 50 * 1024 * 1024, + }); + } catch (err) { + const stderr = (err.stderr || '').toString(); + if (stderr.includes('HTTP 404') || stderr.includes('Not Found')) { + warn(`Upstream unreachable for endpoint "${endpoint}". Treating as tolerated failure.`); + return null; + } + throw new Error(`gh api failed for "${endpoint}": ${stderr || err.message}`, { cause: err }); + } + try { + return JSON.parse(stdout); + } catch (err) { + throw new Error(`gh api returned non-JSON for "${endpoint}": ${err.message}`, { cause: err }); + } +} + +function summarize(state, compare) { + const status = compare.status; + const ahead = compare.ahead_by; + const behind = compare.behind_by; + const headSha = compare.commits && compare.commits.length > 0 + ? compare.commits[compare.commits.length - 1].sha + : state.lastSyncedSha; + + log(`Status: ${status} (ahead_by=${ahead}, behind_by=${behind})`); + log(`Baseline: ${lib.shortSha(state.lastSyncedSha)} (recorded ${state.lastSyncedAt})`); + log(`Upstream HEAD: ${lib.shortSha(headSha)}`); + log(`Compare: ${lib.compareUrl(state.upstream, state.lastSyncedSha, headSha)}`); + + if (status === 'identical' || ahead === 0) { + log('No drift — baseline matches upstream HEAD.'); + return; + } + + if (status === 'ahead' || status === 'diverged') { + log(`Drift detected: upstream is ${ahead} commit(s) ahead.`); + return; + } + + if (status === 'behind') { + log('Baseline is ahead of upstream HEAD — likely a manual cherry-pick or upstream reset.'); + return; + } + + log(`Unknown compare status "${status}" — surfacing as drift for review.`); +} + +function main() { + let state; + try { + state = readState(); + } catch (err) { + console.error(err.message); + process.exit(1); + } + + log(`Upstream: ${state.upstream}`); + const endpoint = `repos/${state.upstream}/compare/${state.lastSyncedSha}...HEAD`; + const compare = ghApi(endpoint); + + if (compare === null) { + // Tolerated: rename/archive/deletion. Phase 2 will optionally + // post a comment on the existing tracking issue if there is one. + process.exit(0); + } + + summarize(state, compare); +} + +if (require.main === module) { + main(); +} + +module.exports = { readState, ghApi, summarize }; diff --git a/tests/lib/upstream-drift.test.js b/tests/lib/upstream-drift.test.js new file mode 100644 index 0000000..f962bc1 --- /dev/null +++ b/tests/lib/upstream-drift.test.js @@ -0,0 +1,225 @@ +/** + * Tests for scripts/lib/upstream-drift.js + * + * Run with: node tests/lib/upstream-drift.test.js + */ + +const assert = require('assert'); +const lib = require('../../scripts/lib/upstream-drift'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + passed += 1; + } catch (err) { + console.log(` ✗ ${name}`); + console.log(` Error: ${err.message}`); + failed += 1; + } +} + +const VALID_STATE = JSON.stringify({ + upstream: 'affaan-m/everything-claude-code', + lastSyncedSha: '9db98673d054f5ed0991ba9d67ff4c883c81a42f', + lastSyncedAt: '2026-02-09', + notes: 'optional', +}); + +console.log('\n=== Testing upstream-drift.js ===\n'); + +// parseUpstreamState + +test('parseUpstreamState accepts a well-formed state file', () => { + const state = lib.parseUpstreamState(VALID_STATE); + assert.strictEqual(state.upstream, 'affaan-m/everything-claude-code'); + assert.strictEqual(state.lastSyncedSha, '9db98673d054f5ed0991ba9d67ff4c883c81a42f'); + assert.strictEqual(state.lastSyncedAt, '2026-02-09'); + assert.strictEqual(state.notes, 'optional'); +}); + +test('parseUpstreamState accepts state without notes', () => { + const json = JSON.stringify({ + upstream: 'a/b', + lastSyncedSha: '0'.repeat(40), + lastSyncedAt: '2026-01-01', + }); + const state = lib.parseUpstreamState(json); + assert.strictEqual(state.notes, undefined); +}); + +test('parseUpstreamState throws on malformed JSON', () => { + assert.throws(() => lib.parseUpstreamState('{not json'), /not valid JSON/); +}); + +test('parseUpstreamState throws on non-object root', () => { + assert.throws(() => lib.parseUpstreamState('[]'), /must be a JSON object/); + assert.throws(() => lib.parseUpstreamState('null'), /must be a JSON object/); + assert.throws(() => lib.parseUpstreamState('"a string"'), /must be a JSON object/); +}); + +test('parseUpstreamState rejects bad upstream values', () => { + const bad = JSON.stringify({ + upstream: 'no-slash', + lastSyncedSha: '0'.repeat(40), + lastSyncedAt: '2026-01-01', + }); + assert.throws(() => lib.parseUpstreamState(bad), /owner\/repo/); +}); + +test('parseUpstreamState rejects short or non-hex SHAs', () => { + const short = JSON.stringify({ + upstream: 'a/b', + lastSyncedSha: 'abc', + lastSyncedAt: '2026-01-01', + }); + assert.throws(() => lib.parseUpstreamState(short), /40-char hex/); + + const nonHex = JSON.stringify({ + upstream: 'a/b', + lastSyncedSha: 'z'.repeat(40), + lastSyncedAt: '2026-01-01', + }); + assert.throws(() => lib.parseUpstreamState(nonHex), /40-char hex/); +}); + +test('parseUpstreamState rejects empty lastSyncedAt', () => { + const bad = JSON.stringify({ + upstream: 'a/b', + lastSyncedSha: '0'.repeat(40), + lastSyncedAt: '', + }); + assert.throws(() => lib.parseUpstreamState(bad), /non-empty string/); +}); + +test('parseUpstreamState rejects non-string notes', () => { + const bad = JSON.stringify({ + upstream: 'a/b', + lastSyncedSha: '0'.repeat(40), + lastSyncedAt: '2026-01-01', + notes: 42, + }); + assert.throws(() => lib.parseUpstreamState(bad), /notes.*must be a string/); +}); + +// shortSha + +test('shortSha returns the first 7 chars', () => { + assert.strictEqual(lib.shortSha('9db98673d054f5ed0991ba9d67ff4c883c81a42f'), '9db9867'); +}); + +test('shortSha returns "" for non-string input', () => { + assert.strictEqual(lib.shortSha(undefined), ''); + assert.strictEqual(lib.shortSha(null), ''); + assert.strictEqual(lib.shortSha(123), ''); +}); + +// formatIssueTitle + +test('formatIssueTitle pluralizes correctly for 0/1/many', () => { + assert.strictEqual( + lib.formatIssueTitle({ count: 0, lastSyncedShaShort: '9db9867' }), + 'Upstream sync: 0 new commits in ECC since 9db9867', + ); + assert.strictEqual( + lib.formatIssueTitle({ count: 1, lastSyncedShaShort: '9db9867' }), + 'Upstream sync: 1 new commit in ECC since 9db9867', + ); + assert.strictEqual( + lib.formatIssueTitle({ count: 5, lastSyncedShaShort: '9db9867' }), + 'Upstream sync: 5 new commits in ECC since 9db9867', + ); +}); + +test('formatIssueTitle rejects negative or non-integer counts', () => { + assert.throws(() => lib.formatIssueTitle({ count: -1, lastSyncedShaShort: 'x' }), /non-negative integer/); + assert.throws(() => lib.formatIssueTitle({ count: 1.5, lastSyncedShaShort: 'x' }), /non-negative integer/); +}); + +test('formatIssueTitle rejects empty shortSha', () => { + assert.throws(() => lib.formatIssueTitle({ count: 1, lastSyncedShaShort: '' }), /non-empty/); +}); + +// formatIssueBody + +test('formatIssueBody includes baseline, head, compare URL', () => { + const body = lib.formatIssueBody({ + commits: [{ sha: 'aaaa1111aaaa1111aaaa1111aaaa1111aaaa1111', author: 'octocat', message: 'fix: something\n\nbody' }], + lastSyncedSha: '9db98673d054f5ed0991ba9d67ff4c883c81a42f', + upstreamHeadSha: 'bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222', + upstreamRepo: 'affaan-m/everything-claude-code', + }); + assert.ok(body.includes('9db9867'), 'should contain baseline short sha'); + assert.ok(body.includes('bbbb222'), 'should contain head short sha'); + assert.ok(body.includes('https://github.com/affaan-m/everything-claude-code/compare/9db98673d054f5ed0991ba9d67ff4c883c81a42f...bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222')); + assert.ok(body.includes('@octocat')); + assert.ok(body.includes('fix: something')); + assert.ok(!body.includes('body'), 'should not include commit message body, only first line'); +}); + +test('formatIssueBody handles empty commit list', () => { + const body = lib.formatIssueBody({ + commits: [], + lastSyncedSha: '0'.repeat(40), + upstreamHeadSha: '1'.repeat(40), + upstreamRepo: 'a/b', + }); + assert.ok(body.includes('_No commits to show._')); +}); + +test('formatIssueBody truncates beyond COMMIT_BODY_LIMIT', () => { + const many = []; + for (let i = 0; i < lib.COMMIT_BODY_LIMIT + 25; i += 1) { + many.push({ + sha: i.toString(16).padStart(40, '0'), + author: 'a', + message: `msg ${i}`, + }); + } + const body = lib.formatIssueBody({ + commits: many, + lastSyncedSha: '0'.repeat(40), + upstreamHeadSha: '1'.repeat(40), + upstreamRepo: 'a/b', + }); + 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'); +}); + +test('formatIssueBody rejects non-array commits', () => { + assert.throws( + () => lib.formatIssueBody({ commits: null, lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'a/b' }), + /must be an array/, + ); +}); + +test('formatIssueBody rejects malformed upstreamRepo', () => { + assert.throws( + () => lib.formatIssueBody({ commits: [], lastSyncedSha: '0', upstreamHeadSha: '1', upstreamRepo: 'no-slash' }), + /owner\/repo/, + ); +}); + +// compareUrl + +test('compareUrl builds the expected GitHub URL', () => { + assert.strictEqual( + lib.compareUrl('a/b', 'base', 'head'), + 'https://github.com/a/b/compare/base...head', + ); +}); + +test('compareUrl rejects malformed repo', () => { + assert.throws(() => lib.compareUrl('no-slash', 'b', 'h'), /owner\/repo/); +}); + +console.log('\n=== Test Results ==='); +console.log(`Passed: ${passed}`); +console.log(`Failed: ${failed}`); +console.log(`Total: ${passed + failed}\n`); + +process.exit(failed === 0 ? 0 : 1); diff --git a/tests/run-all.js b/tests/run-all.js index 121da6e..f7e71c1 100644 --- a/tests/run-all.js +++ b/tests/run-all.js @@ -18,6 +18,7 @@ const testFiles = [ 'lib/gemini-tools.test.js', 'lib/grok-engine.test.js', 'lib/grok-cli.test.js', + 'lib/upstream-drift.test.js', 'hooks/hooks.test.js', 'hooks/block-no-verify.test.js', 'ci/validate-workflow-security.test.js', From cf359a568037d9b4a74eaf4f72b4468b8fc936c1 Mon Sep 17 00:00:00 2001 From: Jamkris Date: Tue, 12 May 2026 10:17:34 +0900 Subject: [PATCH 2/2] fix: address PR #58 review feedback (execFileSync + 404 disambiguation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2 — execSync command-injection surface (CodeRabbit, cubic): Replaced execSync with a template string by execFileSync('gh', ['api', endpoint], opts). The endpoint argument now bypasses shell parsing entirely. While the input is regex-validated upstream, the defense-in-depth change costs nothing and removes a class of risk from the audit-tool surface. P2 — 404 disambiguation (cubic): The previous code treated every 404 from `gh api` as a tolerable "upstream unreachable" warning. That silently masks the much more common failure mode of a malformed `lastSyncedSha` in upstream/.upstream-sync.json — the compare endpoint also returns 404 when the baseline ref doesn't exist in a repo that does. Split into two checks: 1. Pre-flight `gh api repos/` — 404 here is genuine rename/archive/deletion. Warn, exit 0, weekly cron stays green. 2. After pre-flight succeeds, compare-endpoint 404 is a hard error. Print a specific "Check upstream/.upstream-sync.json" message and exit 1. ghApi() now surfaces 404 as a typed error (`err.is404 === true`) so callers can act on it. Added isUpstreamReachable() as the pre-flight helper. P2 — node: prefix suggestion (CodeRabbit): Not applying — repo convention is 23 plain `require('fs')` vs. 1 `require('node:fs')` (see scripts/hooks/run.js). Switching only the new files would create inconsistency; a repo-wide refactor is out of scope for this PR. Will reply on the thread. Local smoke run: - Happy path: ECC 1518 commits ahead, clean output, exit 0. - Bad-baseline path: forced lastSyncedSha to 'deadbeef...' x 5, pre-flight passes, compare returns 404, script prints "Baseline SHA ... is not reachable" and exits 1. Lint clean. 242/242 tests pass. --- scripts/upstream/check-upstream-drift.js | 93 ++++++++++++++++++------ 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/scripts/upstream/check-upstream-drift.js b/scripts/upstream/check-upstream-drift.js index 9fa1ec0..7b708bb 100644 --- a/scripts/upstream/check-upstream-drift.js +++ b/scripts/upstream/check-upstream-drift.js @@ -9,16 +9,18 @@ * in Phase 2. * * Designed to run in `.github/workflows/upstream-drift.yml`: - * - Uses `gh api` for upstream reads (public repo, no PAT needed). - * - Exits 0 on success and on tolerated upstream errors (rename, - * archive, deletion) so weekly cron does not produce red runs that - * nobody triages. - * - Exits non-zero only on programmer errors (malformed state file). + * - Uses `gh api` via execFileSync (argv array — no shell parsing). + * - Exits 0 on success and on tolerated upstream errors (the repo + * itself was renamed/archived/deleted) so weekly cron does not + * produce red runs nobody triages. + * - Exits non-zero on programmer/config errors: a malformed state + * file, or a baseline SHA that doesn't exist in an otherwise + * reachable upstream repo (almost always a typo). */ const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); +const { execFileSync } = require('child_process'); const lib = require('../lib/upstream-drift'); @@ -44,16 +46,22 @@ function readState() { } /** - * Invoke `gh api` and return the parsed JSON response, or null when the - * upstream is unreachable (rename/archive/deletion). Other errors throw. + * Invoke `gh api` for the given endpoint and return the parsed JSON. + * Uses execFileSync with an argv array so the endpoint never reaches + * a shell — defense-in-depth against future inputs that might bypass + * the JSON-schema validation upstream. + * + * 404s are surfaced as a typed error (`err.is404 === true`) so callers + * can distinguish tolerable cases (upstream repo gone) from hard + * failures (invalid baseline SHA against a repo that does exist). * * @param {string} endpoint - path like "repos/owner/repo/compare/A...B" - * @returns {object|null} + * @returns {object} */ function ghApi(endpoint) { let stdout; try { - stdout = execSync(`gh api ${endpoint}`, { + stdout = execFileSync('gh', ['api', endpoint], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], // The compare endpoint can return well over 1 MB of JSON when @@ -63,11 +71,11 @@ function ghApi(endpoint) { }); } catch (err) { const stderr = (err.stderr || '').toString(); - if (stderr.includes('HTTP 404') || stderr.includes('Not Found')) { - warn(`Upstream unreachable for endpoint "${endpoint}". Treating as tolerated failure.`); - return null; - } - throw new Error(`gh api failed for "${endpoint}": ${stderr || err.message}`, { cause: err }); + const is404 = stderr.includes('HTTP 404') || stderr.includes('Not Found'); + const apiError = new Error(`gh api failed for "${endpoint}": ${stderr || err.message}`, { cause: err }); + apiError.stderr = stderr; + apiError.is404 = is404; + throw apiError; } try { return JSON.parse(stdout); @@ -76,6 +84,29 @@ function ghApi(endpoint) { } } +/** + * Pre-flight: check whether the upstream repo itself is reachable. A + * 404 here genuinely means "rename / archive / deletion" — a tolerable + * config drift the workflow should warn about, not fail on. A 404 on + * the *compare* endpoint after this check passes is a hard error + * (almost certainly a malformed `lastSyncedSha`). + * + * @param {string} repo - "owner/repo" + * @returns {boolean} true if reachable; false if the repo returns 404 + */ +function isUpstreamReachable(repo) { + try { + ghApi(`repos/${repo}`); + return true; + } catch (err) { + if (err.is404) { + warn(`Upstream repo "${repo}" returned 404. Treating as tolerated upstream-unreachable (rename, archive, or deletion).`); + return false; + } + throw err; + } +} + function summarize(state, compare) { const status = compare.status; const ahead = compare.ahead_by; @@ -117,15 +148,35 @@ function main() { } log(`Upstream: ${state.upstream}`); - const endpoint = `repos/${state.upstream}/compare/${state.lastSyncedSha}...HEAD`; - const compare = ghApi(endpoint); - if (compare === null) { - // Tolerated: rename/archive/deletion. Phase 2 will optionally - // post a comment on the existing tracking issue if there is one. + // Pre-flight: if the repo itself is 404, treat it as tolerable + // (rename/archive/deletion) and exit 0. Phase 2 will optionally post + // a comment on the existing tracking issue in this branch. + if (!isUpstreamReachable(state.upstream)) { process.exit(0); } + // Repo exists. A 404 on the compare endpoint now means the recorded + // baseline SHA is not reachable in the upstream repo — almost always + // a typo in upstream/.upstream-sync.json. Fail loudly so the + // operator notices. + const endpoint = `repos/${state.upstream}/compare/${state.lastSyncedSha}...HEAD`; + let compare; + try { + compare = ghApi(endpoint); + } catch (err) { + if (err.is404) { + console.error( + `Baseline SHA "${state.lastSyncedSha}" is not reachable in ${state.upstream}. ` + + `Check upstream/.upstream-sync.json — the SHA may be malformed or refer to a commit ` + + `that was force-removed from the upstream history.`, + ); + process.exit(1); + } + console.error(err.message); + process.exit(1); + } + summarize(state, compare); } @@ -133,4 +184,4 @@ if (require.main === module) { main(); } -module.exports = { readState, ghApi, summarize }; +module.exports = { readState, ghApi, isUpstreamReachable, summarize };