From 4f298d0ac0ce86d996f68df1295b108ab33d1c41 Mon Sep 17 00:00:00 2001 From: Ankit Verma Date: Sat, 3 Oct 2026 13:12:26 +0530 Subject: [PATCH] fix(staleness): detect rollback with one Git query (#3445) --- gitnexus/src/cli/group-status-format.ts | 7 +- gitnexus/src/core/git-staleness.ts | 94 +++++++--- gitnexus/src/core/staleness-status.ts | 42 +++-- gitnexus/src/mcp/resources.ts | 2 +- .../context-resource-staleness.test.ts | 22 +++ .../test/unit/cli-group-status-format.test.ts | 23 +++ gitnexus/test/unit/group/service.test.ts | 73 +++++++- gitnexus/test/unit/staleness-fallback.test.ts | 29 +++- gitnexus/test/unit/staleness-timeout.test.ts | 4 +- .../test/unit/staleness-zero-count.test.ts | 162 ++++++++++++++++++ gitnexus/test/unit/staleness.test.ts | 49 ++++++ 11 files changed, 453 insertions(+), 54 deletions(-) create mode 100644 gitnexus/test/unit/staleness-zero-count.test.ts diff --git a/gitnexus/src/cli/group-status-format.ts b/gitnexus/src/cli/group-status-format.ts index f5d09c47e..bd94ccc9a 100644 --- a/gitnexus/src/cli/group-status-format.ts +++ b/gitnexus/src/cli/group-status-format.ts @@ -4,16 +4,18 @@ * against a real group, which is how `STALE (-1 commits behind)` went unnoticed * (#3256). */ +import type { StalenessStatus } from '../core/staleness-status.js'; /** The fields of a `groupStatus` repo row the index column reads. */ export interface GroupRepoIndexRow { indexStale: boolean; commitsBehind?: number; + status?: StalenessStatus; } /** - * The index column of a `group status` row. The output is unchanged except - * for one case: a count that is not a real count renders as `?`. + * The index column of a `group status` row. Diverged indexes differ from HEAD + * without a forward commit count; an unavailable count renders as `?`. * * `group/service.ts` has always reported a repo with no recorded commit as * `{ indexStale: true, commitsBehind: -1 }`. The previous `?? '?'` fallback @@ -21,6 +23,7 @@ export interface GroupRepoIndexRow { */ export const formatIndexStatusCell = (row: GroupRepoIndexRow): string => { if (!row.indexStale) return 'OK '; + if (row.status === 'diverged') return 'STALE (index differs from HEAD)'; const n = row.commitsBehind; const count = typeof n === 'number' && n >= 0 ? String(n) : '?'; return `STALE (${count} commits behind)`; diff --git a/gitnexus/src/core/git-staleness.ts b/gitnexus/src/core/git-staleness.ts index ece63a87c..e224b2319 100644 --- a/gitnexus/src/core/git-staleness.ts +++ b/gitnexus/src/core/git-staleness.ts @@ -17,8 +17,8 @@ export type { StalenessInfo, StalenessStatus } from './staleness-status.js'; const execFileAsync = promisify(execFile); /** - * Ceiling for one `git rev-list` staleness probe. Generous for the local - * history walk this is, and short enough that an unresponsive working tree + * Per-command ceiling for async `git rev-list` and both HEAD probes. + * Generous for local Git queries, and short enough that an unresponsive working tree * degrades to "not stale" quickly rather than holding a request open. */ const STALENESS_TIMEOUT_MS = 5_000; @@ -33,12 +33,23 @@ const behindHint = (n: number): string => const DIVERGED_HINT = "⚠️ Index is not at HEAD and the commit gap could not be counted — the recorded commit may no longer be in this clone's history. Run analyze tool to update."; +// `rev-list --count lastCommit..HEAD` answering 0 does NOT mean "HEAD is the +// indexed commit" — it means "HEAD has no commits lastCommit lacks", which is +// also true when HEAD is an *ancestor* of lastCommit (the working tree checked +// out an older commit than the one indexed, or switched to a line of history +// behind it). That read a rollback as `current` until this hint existed. +const REGRESSED_HINT = + '⚠️ Index is not at HEAD — the indexed commit is not reachable from the checked-out commit (the working tree may have checked out an older commit, or a different line of history). Run analyze tool to update.'; + const unknown = (): StalenessInfo => ({ isStale: false, commitsBehind: 0, status: 'unknown' }); -const fromCount = (commitsBehind: number): StalenessInfo => - commitsBehind > 0 - ? { isStale: true, commitsBehind, hint: behindHint(commitsBehind), status: 'behind' } - : { isStale: false, commitsBehind: 0, status: 'current' }; +// Called only once a positive HEAD-only count is in hand. +const behind = (commitsBehind: number): StalenessInfo => ({ + isStale: true, + commitsBehind, + hint: behindHint(commitsBehind), + status: 'behind', +}); /** * `rev-list` could not answer. Asking for HEAD alone needs no history walk and @@ -53,6 +64,26 @@ const fromHead = (head: string | null, lastCommit: string): StalenessInfo => { return { isStale: false, commitsBehind: 0, hint: DIVERGED_HINT, status: 'diverged' }; }; +/** + * `rev-list --left-right --count lastCommit...HEAD` measures both sides in + * one process, so a later HEAD change cannot mix two snapshots. The left + * count identifies a rollback even when the HEAD-only (right) count is 0. + * Positive right counts keep the existing `behind` behavior, including when + * both sides have commits (divergent branches or a re-shallowed clone). + */ +const fromCounts = (output: string): StalenessInfo => { + const counts = /^(\d+)\s+(\d+)$/.exec(output.trim()); + if (!counts) return unknown(); + const indexedOnly = Number(counts[1]); + const headOnly = Number(counts[2]); + if (!Number.isSafeInteger(indexedOnly) || !Number.isSafeInteger(headOnly)) return unknown(); + if (headOnly > 0) return behind(headOnly); + if (indexedOnly > 0) { + return { isStale: true, commitsBehind: 0, hint: REGRESSED_HINT, status: 'diverged' }; + } + return { isStale: false, commitsBehind: 0, status: 'current' }; +}; + const readHeadSync = (repoPath: string): string | null => { try { return ( @@ -61,6 +92,7 @@ const readHeadSync = (repoPath: string): string | null => { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], windowsHide: true, + timeout: STALENESS_TIMEOUT_MS, }).trim() || null ); } catch { @@ -89,14 +121,18 @@ export function checkStaleness(repoPath: string, lastCommit: string): StalenessI // No recorded commit is not "at HEAD": there is nothing to measure against. if (!lastCommit) return unknown(); try { - const result = execFileSync('git', ['rev-list', '--count', `${lastCommit}..HEAD`], { - cwd: repoPath, - encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true, - }).trim(); + const result = execFileSync( + 'git', + ['rev-list', '--left-right', '--count', `${lastCommit}...HEAD`], + { + cwd: repoPath, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + }, + ); - return fromCount(parseInt(result, 10) || 0); + return fromCounts(result); } catch { return fromHead(readHeadSync(repoPath), lastCommit); } @@ -115,21 +151,25 @@ export async function checkStalenessAsync( try { // Note: promisified execFile captures stdout/stderr by default (no stdio option needed, // unlike the sync variant which requires explicit stdio: ['pipe','pipe','pipe']). - const { stdout } = await execFileAsync('git', ['rev-list', '--count', `${lastCommit}..HEAD`], { - cwd: repoPath, - encoding: 'utf-8', - windowsHide: true, - // The catch below fails closed on every git ERROR, but a hang is not an - // error — it is silence, and without a bound this await never settles. - // A working tree on a disconnected network mount or behind a stuck lock - // does exactly that, and `/api/repos` fans this out once per registered - // repo, so one unreachable mount could hold the whole listing open - // (#3232 review). The timeout kills the child and rejects, and the catch - // below reports it as `unknown` — still the fail-closed `isStale: false`. - timeout: STALENESS_TIMEOUT_MS, - }); + const { stdout } = await execFileAsync( + 'git', + ['rev-list', '--left-right', '--count', `${lastCommit}...HEAD`], + { + cwd: repoPath, + encoding: 'utf-8', + windowsHide: true, + // The catch below fails closed on every git ERROR, but a hang is not an + // error — it is silence, and without a bound this await never settles. + // A working tree on a disconnected network mount or behind a stuck lock + // does exactly that, and `/api/repos` fans this out once per registered + // repo, so one unreachable mount could hold the whole listing open + // (#3232 review). The timeout kills the child and rejects, and the catch + // below reports it as `unknown` — still the fail-closed `isStale: false`. + timeout: STALENESS_TIMEOUT_MS, + }, + ); - return fromCount(parseInt(stdout.trim(), 10) || 0); + return fromCounts(stdout); } catch (err) { // A rev-list that timed out means the working tree is not answering. Asking // it again for HEAD would only double the bound #3232 put on a hung mount. diff --git a/gitnexus/src/core/staleness-status.ts b/gitnexus/src/core/staleness-status.ts index 97a8cce94..40f9d3b51 100644 --- a/gitnexus/src/core/staleness-status.ts +++ b/gitnexus/src/core/staleness-status.ts @@ -18,21 +18,35 @@ * provably stale index indistinguishable from a fresh one. `status` is the * additive channel that separates them for a caller that wants to act on it: * - * - `current` — the index is at HEAD: `rev-list` answered 0, or it could not - * answer but HEAD alone resolved to the indexed commit. - * - `behind` — `rev-list` answered N > 0; `commitsBehind` is N. - * - `diverged` — `rev-list` could not answer, but HEAD resolved and is not the - * indexed commit. The index is provably not at HEAD; only the count is - * unknown. A branch-pinned `serve` clone reaches this once git prunes the - * commit a failed re-index left behind — the pinned update is a - * `fetch --depth 1`, which orphans it — and a rewritten history reaches it - * directly. It is the rule the Claude hook already applies: - * HEAD !== lastCommit. - * - `unknown` — HEAD could not be resolved at all: not a git repository, git - * timed out, or no commit was recorded. + * The successful probe is `rev-list --left-right --count lastCommit...HEAD`: + * the left count is indexed-only commits, and the right is HEAD-only commits. + * Both counts come from one HEAD snapshot, without a follow-up process. * - * `isStale` and `commitsBehind` keep their historical values in every case, so - * no existing consumer changes behaviour unless it reads `status`. + * - `current` — both counts are 0, or `rev-list` could not answer but a + * fallback `rev-parse HEAD` resolved to the indexed commit. + * - `behind` — the right count is N > 0; `commitsBehind` is N, including + * when the left count is positive too (divergent or shallow history). + * - `diverged` — the index is provably not at HEAD, reached two different ways: + * - The left count is positive and the right is 0: HEAD is an ancestor + * of the indexed commit (#3127). The working tree checked out an older + * commit than the one indexed, or a release branch behind the indexed + * tip. The mismatch is established: `isStale` is `true` and + * `commitsBehind` stays 0 (there is no forward count to report). + * - `rev-list` could not answer at all, but HEAD resolved and is not the + * indexed commit: only the count is unknown. A branch-pinned `serve` + * clone reaches this once git prunes the commit a failed re-index left + * behind — the pinned update is a `fetch --depth 1`, which orphans it — + * and a rewritten history reaches it directly. This arm keeps the + * historical fail-open `isStale: false` (see below). + * - `unknown` — the probe could not establish the relationship: no readable + * HEAD, a timeout, no recorded commit, or malformed count output. + * + * `isStale` and `commitsBehind` keep their historical fail-open values + * (`false` / `0`) whenever the check could not fully answer — every `unknown`, + * and the `rev-list`-failure arm of `diverged` — so no existing consumer + * changes behaviour there unless it reads `status`. The other arm of + * `diverged` (the confirmed rollback) is a successful, computed + * answer rather than a failure, so `isStale` reflects it (`true`) instead. */ export type StalenessStatus = 'current' | 'behind' | 'diverged' | 'unknown'; diff --git a/gitnexus/src/mcp/resources.ts b/gitnexus/src/mcp/resources.ts index 8436629e8..49687fa9b 100644 --- a/gitnexus/src/mcp/resources.ts +++ b/gitnexus/src/mcp/resources.ts @@ -351,7 +351,7 @@ async function getContextResource(backend: LocalBackend, repoName?: string): Pro // Check staleness using the current on-disk lastCommit (not the cached handle) const repoPath = repo.repoPath; - const lastCommit = freshMeta?.lastCommit ?? repo.lastCommit ?? 'HEAD'; + const lastCommit = freshMeta?.lastCommit ?? repo.lastCommit ?? ''; const staleness = repoPath ? checkStaleness(repoPath, lastCommit) : { isStale: false, commitsBehind: 0 }; diff --git a/gitnexus/test/integration/context-resource-staleness.test.ts b/gitnexus/test/integration/context-resource-staleness.test.ts index ab94fcab8..c5f58454d 100644 --- a/gitnexus/test/integration/context-resource-staleness.test.ts +++ b/gitnexus/test/integration/context-resource-staleness.test.ts @@ -207,4 +207,26 @@ describe('context resource freshness — out-of-process analyze (#2438)', () => expect(result).toContain('symbols: 333'); expect(result).toContain('processes: 4'); }); + + it('does not claim divergence when disk metadata and the cached handle have no indexed commit', async function missingIndexedCommitDoesNotClaimDivergence() { + writeFileSync(path.join(repoPath, 'a.ts'), 'export const a = 1;\n'); + runGit(repoPath, 'add', 'a.ts'); + runGit(repoPath, 'commit', '-m', 'c1'); + + // A legacy metadata/registry entry can omit lastCommit despite RepoMeta's + // required field; seed that persisted shape rather than a symbolic ref. + await seedIndexedRepo(repoPath, storagePath, { + repoPath, + indexedAt: '2024-01-01T00:00:00Z', + stats: { files: 1, nodes: 1, processes: 0 }, + } as RepoMeta); + + const backend = new LocalBackend(); + await backend.init(); + expect((await backend.resolveRepo('test-repo')).lastCommit).toBeUndefined(); + + const result = await readResource('gitnexus://repo/test-repo/context', backend); + expect(result).not.toContain('staleness:'); + expect(result).toContain(' commit: ""'); + }); }); diff --git a/gitnexus/test/unit/cli-group-status-format.test.ts b/gitnexus/test/unit/cli-group-status-format.test.ts index 32278dd04..962efd536 100644 --- a/gitnexus/test/unit/cli-group-status-format.test.ts +++ b/gitnexus/test/unit/cli-group-status-format.test.ts @@ -27,6 +27,29 @@ describe('formatIndexStatusCell', () => { ); }); + it.each([0, 3])( + 'renders a stale diverged row without a commits-behind count of %i', + (commitsBehind) => { + const row = { indexStale: true, commitsBehind, status: 'diverged' as const }; + expect(formatIndexStatusCell(row)).toBe('STALE (index differs from HEAD)'); + }, + ); + + it('renders an explicit behind status with its counted gap', () => { + const row = { indexStale: true, commitsBehind: 3, status: 'behind' as const }; + expect(formatIndexStatusCell(row)).toBe('STALE (3 commits behind)'); + }); + + it('renders an explicit current status as OK', () => { + const row = { indexStale: false, commitsBehind: 0, status: 'current' as const }; + expect(formatIndexStatusCell(row)).toBe('OK '); + }); + + it('preserves the fail-open cell when a diverged probe did not confirm staleness', () => { + const row = { indexStale: false, commitsBehind: 0, status: 'diverged' as const }; + expect(formatIndexStatusCell(row)).toBe('OK '); + }); + it('keeps the OK cell byte-identical, padding included', () => { expect(formatIndexStatusCell({ indexStale: false, commitsBehind: 0 })).toBe('OK '); }); diff --git a/gitnexus/test/unit/group/service.test.ts b/gitnexus/test/unit/group/service.test.ts index f56522ef9..770e6889f 100644 --- a/gitnexus/test/unit/group/service.test.ts +++ b/gitnexus/test/unit/group/service.test.ts @@ -2,13 +2,17 @@ import { describe, it, expect, vi } from 'vitest'; import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; +import { execFileSync } from 'node:child_process'; import { GroupService, type GroupToolPort, type GroupRepoHandle, } from '../../../src/core/group/service.js'; import { writeContractRegistry } from '../../../src/core/group/storage.js'; -import { formatIndexStatusCell } from '../../../src/cli/group-status-format.js'; +import { + formatIndexStatusCell, + type GroupRepoIndexRow, +} from '../../../src/cli/group-status-format.js'; import type { ContractRegistry, StoredContract, CrossLink } from '../../../src/core/group/types.js'; function makeTmpGroup(): { tmpDir: string; groupDir: string; cleanup: () => void } { @@ -717,10 +721,7 @@ repos: const svc = new GroupService(port); const result = (await svc.groupStatus({ name: 'test-group' })) as { - repos: Record< - string, - { indexStale: boolean; commitsBehind?: number; status?: string; missing: boolean } - >; + repos: Record; }; const row = result.repos['app/backend']; @@ -736,5 +737,67 @@ repos: cleanup(); } }); + + it('renders a real rollback as an index that differs from HEAD', async () => { + const { cleanup, tmpDir } = makeTmpGroup(); + try { + vi.stubEnv('GITNEXUS_HOME', tmpDir); + const repoPath = path.join(tmpDir, 'rollback'); + fs.mkdirSync(repoPath); + const git = (...args: string[]): string => + execFileSync( + 'git', + [ + '-c', + 'user.email=t@example.com', + '-c', + 'user.name=T', + '-c', + 'commit.gpgsign=false', + ...args, + ], + { cwd: repoPath, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }, + ).trim(); + git('init', '-q', '--initial-branch=main'); + git('commit', '--allow-empty', '-qm', 'first'); + const firstCommit = git('rev-parse', 'HEAD'); + git('commit', '--allow-empty', '-qm', 'indexed'); + const indexedCommit = git('rev-parse', 'HEAD'); + git('checkout', '-q', '--detach', firstCommit); + + const storagePath = path.join(repoPath, '.gitnexus'); + fs.mkdirSync(storagePath); + fs.writeFileSync( + path.join(storagePath, 'gitnexus.json'), + JSON.stringify({ lastCommit: indexedCommit, indexedAt: '2026-01-01T00:00:00.000Z' }), + ); + const port = makePort({ + resolveRepo: vi.fn( + async (name?: string): Promise => ({ + id: name || 'test', + name: name || 'test', + repoPath, + storagePath, + }), + ), + }); + + const svc = new GroupService(port); + const result = (await svc.groupStatus({ name: 'test-group' })) as { + repos: Record; + }; + const row = result.repos['app/backend']; + expect(row).toMatchObject({ + missing: false, + indexStale: true, + commitsBehind: 0, + status: 'diverged', + }); + expect(formatIndexStatusCell(row)).toBe('STALE (index differs from HEAD)'); + } finally { + vi.unstubAllEnvs(); + cleanup(); + } + }); }); }); diff --git a/gitnexus/test/unit/staleness-fallback.test.ts b/gitnexus/test/unit/staleness-fallback.test.ts index 427c52b40..baed8e13b 100644 --- a/gitnexus/test/unit/staleness-fallback.test.ts +++ b/gitnexus/test/unit/staleness-fallback.test.ts @@ -3,7 +3,7 @@ * OTHER than a timeout. `staleness.test.ts` reaches `diverged` and `unknown` * against real repositories, but not the third arm of `fromHead`: HEAD still * resolves to the indexed commit, so the index is at HEAD however `rev-list` - * failed. Real git cannot fail `..HEAD` while HEAD prints that same SHA + * failed. Real git cannot fail `...HEAD` while HEAD prints that same SHA * without a corrupted object store, so this drives it through a mock. * * Its own file for the same reason as `staleness-timeout.test.ts`: the mock @@ -12,14 +12,21 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const { plan, spawnedArgs } = vi.hoisted(() => ({ - plan: { head: null as string | null }, + plan: { head: null as string | null, enoent: false }, spawnedArgs: [] as string[][], })); // `rev-list` exits 128 the way git does for a missing object; `rev-parse HEAD` -// answers `plan.head`, or fails when it is null. +// answers `plan.head`, or fails when it is null. `plan.enoent` instead fails +// every invocation the way Node reports a missing executable (git not on +// PATH) — no exit code, `code: 'ENOENT'` — to check that shape of failure is +// caught by the same `catch` as a present-but-failing git (#3127). const answer = (args: readonly string[]): { error: Error | null; stdout: string } => { spawnedArgs.push([...args]); + if (plan.enoent) { + const failure = Object.assign(new Error('spawn git ENOENT'), { code: 'ENOENT' }); + return { error: failure, stdout: '' }; + } if (args[0] === 'rev-parse' && plan.head) return { error: null, stdout: `${plan.head}\n` }; const failure = Object.assign(new Error(`Command failed: git ${args.join(' ')}`), { code: 128, @@ -54,7 +61,7 @@ vi.mock('node:child_process', async (importOriginal) => { import { checkStaleness, checkStalenessAsync } from '../../src/core/git-staleness.js'; const INDEXED_COMMIT = 'a'.repeat(40); -const REV_LIST = ['rev-list', '--count', `${INDEXED_COMMIT}..HEAD`]; +const REV_LIST = ['rev-list', '--left-right', '--count', `${INDEXED_COMMIT}...HEAD`]; const REV_PARSE = ['rev-parse', 'HEAD']; const bothHelpers = { @@ -65,6 +72,7 @@ const bothHelpers = { describe('staleness after a failed (not timed-out) rev-list (#3256)', () => { beforeEach(() => { plan.head = null; + plan.enoent = false; spawnedArgs.length = 0; }); @@ -95,6 +103,19 @@ describe('staleness after a failed (not timed-out) rev-list (#3256)', () => { expect(result).toEqual({ isStale: false, commitsBehind: 0, status: 'unknown' }); expect(spawnedArgs).toEqual([REV_LIST, REV_PARSE]); }); + + it('reports unknown — never current — when git itself is not on PATH', async () => { + // nikolai-vysotskyi (#3127): "git not on PATH" specifically, as a + // distinct failure shape (ENOENT, no exit code) from a present git + // that fails against a bad ref. Same requirement either way: it must + // never be silently read as fresh. + plan.enoent = true; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toEqual({ isStale: false, commitsBehind: 0, status: 'unknown' }); + expect(spawnedArgs).toEqual([REV_LIST, REV_PARSE]); + }); }); } }); diff --git a/gitnexus/test/unit/staleness-timeout.test.ts b/gitnexus/test/unit/staleness-timeout.test.ts index 1b80df5b8..6f4670135 100644 --- a/gitnexus/test/unit/staleness-timeout.test.ts +++ b/gitnexus/test/unit/staleness-timeout.test.ts @@ -43,6 +43,8 @@ describe('checkStalenessAsync — timed-out rev-list (#3256)', () => { expect(result).toEqual({ status: 'unknown', isStale: false, commitsBehind: 0 }); // Exactly one spawn. A `rev-parse HEAD` follow-up here is the regression: // it doubles the hung-mount bound and can flip this answer. - expect(spawnedArgs).toEqual([['rev-list', '--count', `${INDEXED_COMMIT}..HEAD`]]); + expect(spawnedArgs).toEqual([ + ['rev-list', '--left-right', '--count', `${INDEXED_COMMIT}...HEAD`], + ]); }); }); diff --git a/gitnexus/test/unit/staleness-zero-count.test.ts b/gitnexus/test/unit/staleness-zero-count.test.ts new file mode 100644 index 000000000..4e2b510b7 --- /dev/null +++ b/gitnexus/test/unit/staleness-zero-count.test.ts @@ -0,0 +1,162 @@ +/** + * Count both sides in one Git process to distinguish freshness from rollback + * without mixing HEAD snapshots (#3127). + * Keep the child-process mock isolated from tests that use real repositories. + */ +import type { ExecFileOptions } from 'node:child_process'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { plan, invocations } = vi.hoisted(() => ({ + plan: { counts: '0\t0\n' as string | null, head: 'failure', advanceHead: false }, + invocations: [] as { args: string[]; options: ExecFileOptions }[], +})); + +const answer = (args: readonly string[], options: ExecFileOptions): string => { + invocations.push({ args: [...args], options }); + if (args[0] === 'rev-list') { + if (plan.counts === null) { + throw Object.assign(new Error('Command failed: git rev-list'), { code: 128, killed: false }); + } + // Simulate a checkout/commit after Git measured the relationship. A second + // process would see this different HEAD and could misclassify the result. + if (plan.advanceHead) plan.head = 'b'.repeat(40); + return plan.counts; + } + if (plan.head === 'empty') return ' \n'; + if (plan.head === 'timeout') { + throw Object.assign(new Error('Command failed: git rev-parse HEAD'), { + killed: true, + signal: 'SIGTERM', + }); + } + if (plan.head === 'failure') { + throw Object.assign(new Error('Command failed: git rev-parse HEAD'), { + code: 128, + killed: false, + }); + } + return `${plan.head}\n`; +}; + +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + const { promisify } = await import('node:util'); + const execFile = ( + _file: string, + args: readonly string[], + options: ExecFileOptions, + callback: (error: Error | null, stdout: string, stderr: string) => void, + ): void => { + try { + callback(null, answer(args, options), ''); + } catch (error) { + callback(error as Error, '', ''); + } + }; + // Real execFile has a custom promisifier returning both output streams. + Object.defineProperty(execFile, promisify.custom, { + value: async (_file: string, args: readonly string[], options: ExecFileOptions) => ({ + stdout: answer(args, options), + stderr: '', + }), + }); + const execFileSync = (_file: string, args: readonly string[], options: ExecFileOptions): string => + answer(args, options); + return { + ...actual, + execFile: execFile as unknown as typeof actual.execFile, + execFileSync: execFileSync as unknown as typeof actual.execFileSync, + }; +}); + +import { checkStaleness, checkStalenessAsync } from '../../src/core/git-staleness.js'; + +const INDEXED_COMMIT = 'a'.repeat(40); +const REV_LIST = ['rev-list', '--left-right', '--count', `${INDEXED_COMMIT}...HEAD`]; +const REV_PARSE = ['rev-parse', 'HEAD']; + +const bothHelpers = { + checkStaleness: async (repo: string, lastCommit: string) => checkStaleness(repo, lastCommit), + checkStalenessAsync, +}; + +describe('staleness from one relationship query (#3127)', () => { + beforeEach(() => { + plan.counts = '0\t0\n'; + plan.head = 'failure'; + plan.advanceHead = false; + invocations.length = 0; + }); + + for (const [name, check] of Object.entries(bothHelpers)) { + describe(name, () => { + it.each([ + { counts: '0\t0\n', status: 'current', isStale: false, commitsBehind: 0 }, + { counts: '2\t0\n', status: 'diverged', isStale: true, commitsBehind: 0 }, + { counts: '0\t3\n', status: 'behind', isStale: true, commitsBehind: 3 }, + { counts: '2\t3\n', status: 'behind', isStale: true, commitsBehind: 3 }, + ])('reports $status from $counts in one process', async ({ counts, ...expected }) => { + plan.counts = counts; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toMatchObject(expected); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST]); + }); + + it('keeps the count snapshot when HEAD advances after the query', async () => { + plan.head = INDEXED_COMMIT; + plan.advanceHead = true; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(plan.head).not.toBe(INDEXED_COMMIT); + expect(result).toEqual({ status: 'current', isStale: false, commitsBehind: 0 }); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST]); + }); + + it.each(['', '0\n', '0\tbogus\n'])( + 'reports unknown for malformed counts %j', + async (counts) => { + plan.counts = counts; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toEqual({ status: 'unknown', isStale: false, commitsBehind: 0 }); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST]); + }, + ); + + it('reports unknown when the follow-up HEAD command fails', async () => { + plan.counts = null; + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toEqual({ status: 'unknown', isStale: false, commitsBehind: 0 }); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST, REV_PARSE]); + }); + + it('reports unknown when the follow-up HEAD command returns no commit', async () => { + plan.counts = null; + plan.head = 'empty'; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toEqual({ status: 'unknown', isStale: false, commitsBehind: 0 }); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST, REV_PARSE]); + }); + + it('bounds the HEAD command and reports unknown without retrying after its timeout', async () => { + plan.counts = null; + plan.head = 'timeout'; + + const result = await check('/repo', INDEXED_COMMIT); + + expect(result).toEqual({ status: 'unknown', isStale: false, commitsBehind: 0 }); + expect(invocations.map(({ args }) => args)).toEqual([REV_LIST, REV_PARSE]); + const timeout = invocations[1].options.timeout; + expect(Number.isFinite(timeout)).toBe(true); + expect(timeout).toBeGreaterThan(0); + }); + }); + } +}); diff --git a/gitnexus/test/unit/staleness.test.ts b/gitnexus/test/unit/staleness.test.ts index ef4a6a517..0e6084d7e 100644 --- a/gitnexus/test/unit/staleness.test.ts +++ b/gitnexus/test/unit/staleness.test.ts @@ -335,3 +335,52 @@ describe('branch-pinned serve clone once git prunes the indexed commit (#3256)', }); }); }); + +// ── #3127: a 0 forward count is not "the index matches this tree" ─────────── +// +// nikolai-vysotskyi (issue #3127, comment on the `--stale-policy` proposal): +// `git rev-list --count lastCommit..HEAD` answers "commits reachable from +// HEAD but not lastCommit", which is also 0 when HEAD is an *ancestor* of +// lastCommit — i.e. the working tree checked out an older commit than the one +// indexed, or a release branch behind the indexed tip. Before this fix that +// read as `current`; a `--stale-policy error`-style caller would exit 0 +// exactly when the index is provably wrong about the checked-out tree. +describe('staleness when the checkout has regressed behind the indexed commit (#3127)', () => { + let root: string; + let fixture: ReturnType; + + beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'gitnexus-staleness-regressed-')); + fixture = makeRepo(root, 'repo'); + // Roll the working tree back to c1. `rev-list --count c3..HEAD` alone + // answers 0 here (HEAD/c1 has no commits c3 lacks) — exactly the count a + // pre-fix caller read as "index matches HEAD". + git(fixture.repo, 'checkout', '-q', fixture.c1); + }); + afterAll(() => removeTree(root)); + + for (const [name, check] of Object.entries(bothHelpers)) { + describe(name, () => { + it('reports diverged, not current, when HEAD is behind the indexed commit', async () => { + const result = await check(fixture.repo, fixture.c3); + + expect(result.status).toBe('diverged'); + // Established by a positive indexed-only count and a zero HEAD-only + // count (not a `rev-list` failure), so — unlike the failure-path + // `diverged` above — `isStale` reflects the mismatch instead of the + // historical fail-open `false`. + expect(result.isStale).toBe(true); + expect(result.commitsBehind).toBe(0); + expect(result.hint).toContain('not reachable from the checked-out commit'); + }); + + it('still reports current for the commit actually checked out', async () => { + expect(await check(fixture.repo, fixture.c1)).toMatchObject({ + status: 'current', + isStale: false, + commitsBehind: 0, + }); + }); + }); + } +});