diff --git a/scripts/__tests__/check-i18n-en-drift.test.ts b/scripts/__tests__/check-i18n-en-drift.test.ts index 334ef5fd5..a67be8353 100644 --- a/scripts/__tests__/check-i18n-en-drift.test.ts +++ b/scripts/__tests__/check-i18n-en-drift.test.ts @@ -138,8 +138,17 @@ function repoWithCommits(commits: Array>): { root: string return { root, shas }; } -/** Runs the real gate as CI runs it. Returns the exit code and the merged output. */ -function runGate(root: string, args: string[]): { code: number; output: string } { +/** + * Runs the real gate as CI runs it. Returns the exit code and the merged output. + * + * `envOverrides` re-supplies one of the two vars the gate reads, for the tests + * that are ABOUT those vars; everything else keeps them cleared. + */ +function runGate( + root: string, + args: string[], + envOverrides: Record = {}, +): { code: number; output: string } { const result = execFileSync( process.execPath, [gateScript, '--root', root, ...args], @@ -149,7 +158,7 @@ function runGate(root: string, args: string[]): { code: number; output: string } cwd: root, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], - env: { ...process.env, OS_I18N_DRIFT_BASE: '', GITHUB_BASE_REF: '' }, + env: { ...process.env, OS_I18N_DRIFT_BASE: '', GITHUB_BASE_REF: '', ...envOverrides }, }, // `execFileSync` throws on a non-zero exit; the status and both streams are // on the error, so the two paths are unified here rather than at each site. @@ -157,9 +166,13 @@ function runGate(root: string, args: string[]): { code: number; output: string } return { code: 0, output: result }; } -function runGateExpectingFailure(root: string, args: string[]): { code: number; output: string } { +function runGateExpectingFailure( + root: string, + args: string[], + envOverrides: Record = {}, +): { code: number; output: string } { try { - runGate(root, args); + runGate(root, args, envOverrides); } catch (error) { const failure = error as { status: number; stdout: string; stderr: string }; return { code: failure.status, output: `${failure.stdout}${failure.stderr}` }; @@ -564,6 +577,128 @@ describe('resolving the commit to compare against', () => { }); }); + // ── an explicitly named base is authoritative (objectui#3766) ───────────── + + /** Well-formed, forty hex digits, and in no clone anywhere. */ + const ABSENT_SHA = '0123456789abcdef0123456789abcdef01234567'; + + it('FAILS on an explicitly named --base that does not exist — it does NOT fall back', () => { + // The defect objectui#3766 filed, in the shape it was measured. `--base` used + // to be merely the FIRST link of the candidate chain, so a sha missing from + // this clone (a shallow checkout, an unfetched sha, a typo) fell through to + // `merge-base with main` — which in this fixture repo resolves to HEAD. The + // gate then compared the packs at HEAD against the identical working tree, + // found no changed en value, and printed a confident green (measured: exit 0, + // with `(--base …)` in the summary line replaced by the candidate it actually + // used, so the log gave the reader nothing to notice). Restoring the + // fallthrough turns this case green again — exit 0 and the "followed by all + // nine" line below — which is the reverse verification for the fix. + // + // "The base you named is missing" and "you named no base" are different + // facts, and only the second may be answered by guessing. Ported verbatim in + // spirit from `check-changeset-presence.test.ts`, where the sibling gate's + // own tests caught this first (objectui#3762). + const { root } = reconstructed3625(); + + const { code, output } = runGateExpectingFailure(root, [...FIXTURE_FLOOR, '--base', ABSENT_SHA]); + expect(code).toBe(1); + expect(output).toContain('Cannot resolve the commit to compare against'); + expect(output).toContain(`--base ${ABSENT_SHA} (unresolved)`); + expect(output).toContain('named EXPLICITLY'); + expect(output).toContain('a diff gate with no diff would pass while'); + // And specifically NOT the pass it used to produce by comparing against main. + expect(output).not.toContain('Every changed en value was followed'); + }); + + it('FAILS the same way on an OS_I18N_DRIFT_BASE that does not exist', () => { + // The env var is the other documented explicit entrypoint — same authority, + // same failure, and it is the one CI-adjacent callers reach for. Driven + // through the real CLI with the var actually set, because `runGate` clears it + // for every other test in this file. + const { root } = reconstructed3625(); + + const { code, output } = runGateExpectingFailure(root, FIXTURE_FLOOR, { + OS_I18N_DRIFT_BASE: ABSENT_SHA, + }); + expect(code).toBe(1); + expect(output).toContain(`OS_I18N_DRIFT_BASE=${ABSENT_SHA} (unresolved)`); + expect(output).toContain('named EXPLICITLY'); + expect(output).not.toContain('Every changed en value was followed'); + }); + + it('consults NOTHING but the named base — not even a candidate that would resolve', () => { + // The mechanism, asserted directly rather than through an exit code: `tried` + // holds exactly one entry. `GITHUB_BASE_REF=main` plus a real `origin/main` + // here means the discovery candidate WOULD have produced a base, so a single + // `tried` entry is the proof that it was never asked. + const { root, shas } = repoWithCommits([ + packFiles(treesFor((lang) => ({ a: `x-${lang}` }))), + packFiles(treesFor((lang) => ({ a: `y-${lang}` }))), + ]); + execFileSync('git', ['remote', 'add', 'origin', root], { cwd: root }); + execFileSync('git', ['update-ref', 'refs/remotes/origin/main', shas[0]], { cwd: root }); + + const outcome = resolveBaseRef(root, { explicit: ABSENT_SHA, env: { GITHUB_BASE_REF: 'main' } }); + expect(outcome.ok).toBe(false); + if (!outcome.ok) { + expect(outcome.named).toBe(true); + expect(outcome.tried).toEqual([`--base ${ABSENT_SHA} (unresolved)`]); + } + // Sanity: the same repo, same env, WITHOUT a named base still finds one. + expect(resolveBaseRef(root, { env: { GITHUB_BASE_REF: 'main' } })).toMatchObject({ ok: true }); + }); + + it('still resolves a named base that DOES exist, and --base outranks the env var', () => { + const { root, shas } = repoWithCommits([ + packFiles(treesFor((lang) => ({ a: `x-${lang}` }))), + packFiles(treesFor((lang) => ({ a: `y-${lang}` }))), + ]); + + expect(resolveBaseRef(root, { explicit: shas[0], env: {} })).toMatchObject({ + ok: true, + ref: shas[0], + how: `--base ${shas[0]}`, + }); + expect(resolveBaseRef(root, { env: { OS_I18N_DRIFT_BASE: shas[0] } })).toMatchObject({ + ok: true, + ref: shas[0], + how: `OS_I18N_DRIFT_BASE=${shas[0]}`, + }); + // `--base` outranks the env var: both name a real commit, and the flag's is + // the one compared against. + const both = resolveBaseRef(root, { explicit: shas[0], env: { OS_I18N_DRIFT_BASE: shas[1] } }); + expect(both).toMatchObject({ ok: true, ref: shas[0], how: `--base ${shas[0]}` }); + }); + + it('keeps guessing through the whole chain when NO base was named', () => { + // The other half of objectui#3766, and the half CI depends on: only "you + // named one and it is missing" became a failure. With nothing named, an + // unresolvable `GITHUB_BASE_REF` still falls through to `origin/main` exactly + // as before — `pnpm check:i18n-drift` passes no `--base`, so this is the path + // every real CI run takes. + const { root, shas } = repoWithCommits([ + packFiles(treesFor((lang) => ({ a: `x-${lang}` }))), + packFiles(treesFor((lang) => ({ a: `y-${lang}` }))), + ]); + execFileSync('git', ['remote', 'add', 'origin', root], { cwd: root }); + execFileSync('git', ['update-ref', 'refs/remotes/origin/main', shas[0]], { cwd: root }); + + // `GITHUB_BASE_REF` names a branch this clone does not have; the chain moves + // on and `origin/main` answers. + expect(resolveBaseRef(root, { env: { GITHUB_BASE_REF: 'no-such-branch' } })).toMatchObject({ + ok: true, + ref: shas[0], + how: 'merge-base with origin/main', + }); + // And in a clone with no `origin` at all, the last link — the local `main` — + // still answers, so the chain is intact end to end and not just at its head. + const solo = repoWithCommits([packFiles(treesFor((lang) => ({ a: `x-${lang}` })))]); + expect(resolveBaseRef(solo.root, { env: {} })).toMatchObject({ + ok: true, + how: 'merge-base with main', + }); + }); + it('reads the working tree as the after side when no head is named', () => { // So an agent editing en.ts locally gets the answer before committing. const { root, shas } = repoWithCommits([packFiles(treesFor((lang) => ({ a: `x-${lang}` })))]); diff --git a/scripts/check-changeset-presence.mjs b/scripts/check-changeset-presence.mjs index f15153702..df626af46 100644 --- a/scripts/check-changeset-presence.mjs +++ b/scripts/check-changeset-presence.mjs @@ -185,8 +185,9 @@ function gitQuiet(root, args) { * silently judged the change against `main`'s tip instead and printed a * confident green. "The base you named is missing" and "you named no base" are * different facts, and only the second one may be answered by guessing. (The - * sibling gate `check-i18n-en-drift.mjs` still has the fallthrough shape — filed - * separately, not fixed here.) + * sibling gate `check-i18n-en-drift.mjs` inherited the fallthrough shape from + * this gate's first draft; it was fixed to match under objectui#3766, so the two + * resolvers are the same shape again — keep them that way.) * * @returns {{ ok: true, ref: string, how: string } | { ok: false, tried: string[], shallow: boolean, named: boolean }} */ diff --git a/scripts/check-i18n-en-drift.mjs b/scripts/check-i18n-en-drift.mjs index e69189539..e0bea426a 100644 --- a/scripts/check-i18n-en-drift.mjs +++ b/scripts/check-i18n-en-drift.mjs @@ -340,16 +340,52 @@ export function readPacks(root, ref) { /** * Which commit this branch should be judged against. * - * Order: an explicit `--base`, then `OS_I18N_DRIFT_BASE`, then the merge base - * with the PR's own base branch (`GITHUB_BASE_REF`), then `origin/main`, then a - * local `main`. A base that cannot be resolved is a HARD FAILURE, never a skip: - * a diff gate that quietly finds nothing to diff reports green while checking - * nothing, which is the failure this whole family of gates exists to stop. + * An explicitly NAMED base (`--base`, `OS_I18N_DRIFT_BASE`) is authoritative and + * is the only thing consulted when given. Otherwise the base is DISCOVERED: the + * merge base with the pull request's own base branch (`GITHUB_BASE_REF`), then + * `origin/main`, then a local `main`. The merge base — rather than the target + * branch's tip — is what makes a branch answer for the edits IT made and not for + * whatever landed on `main` after it forked. + * + * A base that cannot be resolved is a HARD FAILURE, never a skip: a diff gate + * that quietly finds nothing to diff reports green while checking nothing, which + * is the failure this whole family of gates exists to stop. And an explicit one + * that cannot be resolved does NOT fall through to the discovery candidates — + * with the fallthrough, a `--base` sha absent from this clone (a shallow clone, + * an unfetched sha, a typo) silently judged the packs against whatever the + * discovery chain answered next instead (`merge-base with origin/main` on this + * repo, as measured) and printed a confident green, with the `(--base …)` in the + * summary line replaced by the candidate actually used so the log gave the + * reader nothing to notice (objectui#3766, measured: exit 0 → exit 1). "The base + * you named is missing" and "you named no base" are different facts, and only + * the second one may be answered by guessing. `check-changeset-presence.mjs` + * resolves its base from the same sources and had this same defect caught by its + * own tests first (objectui#3762) — the two `resolveBaseRef`s are deliberately + * the same shape. * - * @returns {{ ok: true, ref: string, how: string } | { ok: false, tried: string[], shallow: boolean }} + * @param {string} root + * @param {{ explicit?: string | null, env?: Record }} [options] + * @returns {{ ok: true, ref: string, how: string } | { ok: false, tried: string[], shallow: boolean, named: boolean }} */ export function resolveBaseRef(root, { explicit = null, env = process.env } = {}) { const tried = []; + const verify = (ref) => gitQuiet(root, ['rev-parse', '--verify', `${ref}^{commit}`]); + const shallow = () => gitQuiet(root, ['rev-parse', '--is-shallow-repository']) === 'true'; + + const named = explicit + ? { how: `--base ${explicit}`, ref: explicit } + : env.OS_I18N_DRIFT_BASE + ? { how: `OS_I18N_DRIFT_BASE=${env.OS_I18N_DRIFT_BASE}`, ref: env.OS_I18N_DRIFT_BASE } + : null; + + if (named) { + const resolved = verify(named.ref); + tried.push(`${named.how}${resolved ? '' : ' (unresolved)'}`); + return resolved + ? { ok: true, ref: resolved, how: named.how } + : { ok: false, tried, shallow: shallow(), named: true }; + } + const attempt = (how, compute) => { const value = compute(); tried.push(`${how}${value ? '' : ' (unresolved)'}`); @@ -357,10 +393,6 @@ export function resolveBaseRef(root, { explicit = null, env = process.env } = {} }; const candidates = [ - explicit ? () => attempt(`--base ${explicit}`, () => gitQuiet(root, ['rev-parse', '--verify', `${explicit}^{commit}`])) : null, - env.OS_I18N_DRIFT_BASE - ? () => attempt(`OS_I18N_DRIFT_BASE=${env.OS_I18N_DRIFT_BASE}`, () => gitQuiet(root, ['rev-parse', '--verify', `${env.OS_I18N_DRIFT_BASE}^{commit}`])) - : null, env.GITHUB_BASE_REF ? () => attempt(`merge-base with origin/${env.GITHUB_BASE_REF}`, () => gitQuiet(root, ['merge-base', 'HEAD', `origin/${env.GITHUB_BASE_REF}`])) : null, @@ -372,7 +404,7 @@ export function resolveBaseRef(root, { explicit = null, env = process.env } = {} const hit = candidate(); if (hit) return hit; } - return { ok: false, tried, shallow: gitQuiet(root, ['rev-parse', '--is-shallow-repository']) === 'true' }; + return { ok: false, tried, shallow: shallow(), named: false }; } // -- the comparison ----------------------------------------------------------- @@ -536,10 +568,15 @@ if (invokedDirectly) { console.error( 'Cannot resolve the commit to compare against, so there is nothing to diff.\n' + ` tried: ${base.tried.join(', ')}\n` + - (base.shallow - ? ' This clone is SHALLOW. In CI, give the checkout `fetch-depth: 0`; locally,\n' + - ' run `git fetch --no-tags origin main` (or `git fetch --unshallow`).\n' - : ' Fetch the base branch (`git fetch --no-tags origin main`) and re-run.\n') + + (base.named + ? ' That base was named EXPLICITLY, so it is not guessed around: comparing the packs\n' + + ' against some other commit instead would answer a question nobody asked, and\n' + + ' answer it confidently. Name a commit that exists in this clone, or pass none and\n' + + ' let the merge base with the target branch be found.\n' + : base.shallow + ? ' This clone is SHALLOW. In CI, give the checkout `fetch-depth: 0`; locally,\n' + + ' run `git fetch --no-tags origin main` (or `git fetch --unshallow`).\n' + : ' Fetch the base branch (`git fetch --no-tags origin main`) and re-run.\n') + ' This is a failure, not a skip: a diff gate with no diff would pass while\n' + ' checking nothing. See the header of scripts/check-i18n-en-drift.mjs.', );