From 6e821890cc08226fc6b9c2cd4822f1ae7e8ee435 Mon Sep 17 00:00:00 2001 From: Francesco Falanga Date: Sat, 19 Sep 2026 17:31:42 +0200 Subject: [PATCH] =?UTF-8?q?fix(core,metrics):=20renames=20follow=20the=20f?= =?UTF-8?q?ile=20=E2=80=94=20no=20phantom=20`old=20=3D>=20new`=20path,=20l?= =?UTF-8?q?ifecycle=20carried=20across=20the=20move?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by an external review and reproduced. `old.ts` → `git mv new.ts` → edit `new.ts` produced THREE paths in the stream: `old.ts`, `old.ts => new.ts` and `new.ts`, the rename filed as a modification of a file that never existed. Every rename was one more eligible file nothing could retouch, and the real file's edits landed under a name its first touch was never recorded against. Rate biased down: it flatters. - core: `git log --numstat` names a renamed file as `old => new` or `dir/{old => new}/file` (1,911 brace-form and 45 plain-form on react-router). `splitRenamePath` parses both; the stream records the file under its new name with `previousPath` and status `renamed` (additive field, no schema bump). `git show` in PR scope gets the same. - metrics: persistence is one chronological pass; a lifecycle follows the file across a rename. A pure rename (0/0) is not a touch — only the address changed. A rename with edits is. Hotfix antecedents move with the file the same way. Fixture: 0/3 (0%) → 1/1 (100%). AI-Mode: agent --- .changeset/renames-follow-the-file.md | 13 +++++ packages/core/src/git/collect.test.ts | 39 +++++++++++++ packages/core/src/git/diff.ts | 7 ++- packages/core/src/git/log.ts | 9 ++- packages/core/src/git/paths.test.ts | 55 ++++++++++++++++++ packages/core/src/git/paths.ts | 42 ++++++++++++++ packages/core/src/schema/commit.ts | 4 ++ .../metrics/src/outcome-correlation.test.ts | 37 ++++++++++++ packages/metrics/src/outcome-correlation.ts | 11 ++++ packages/metrics/src/persistence.test.ts | 55 ++++++++++++++++++ packages/metrics/src/persistence.ts | 56 ++++++++++++------- 11 files changed, 303 insertions(+), 25 deletions(-) create mode 100644 .changeset/renames-follow-the-file.md create mode 100644 packages/core/src/git/paths.test.ts create mode 100644 packages/core/src/git/paths.ts diff --git a/.changeset/renames-follow-the-file.md b/.changeset/renames-follow-the-file.md new file mode 100644 index 0000000..3bf8bb4 --- /dev/null +++ b/.changeset/renames-follow-the-file.md @@ -0,0 +1,13 @@ +--- +'@evidtrail/core': patch +'@evidtrail/metrics': patch +--- + +Renames follow the file: a moved file is one file, not a phantom path plus a stranger + +Found by an external review, reproduced, and it flatters: `old.ts` → `git mv new.ts` → edit `new.ts` produced three paths in the stream — `old.ts`, `old.ts => new.ts` and `new.ts` — with the rename filed as a *modification* of a file that never existed. Every rename in a repository was one more eligible file that could never be retouched, and the real file's later edits were filed under a name its first touch had never been recorded against. On react-router, 1,956 renames were doing this. + +- **core** — `git log --numstat` names a renamed file as `old => new` or `dir/{old => new}/file`; both forms are now parsed (`splitRenamePath`). The stream records the file under its new name with `previousPath` set and status `renamed` (additive field, no schema bump). PR-scoped collection (`git show`) gets the same parsing. +- **metrics** — a file's lifecycle follows it across a rename. A pure rename (no lines changed) is not a touch: nothing about the code happened, only its address. A rename that also edits is one. Hotfix linking carries the touch history across the move the same way. + +Direction of the change on real repositories: eligible files go down, retouches go up, and the rate rises — the phantom paths had been padding the denominator with files nothing could ever touch. Exact figures in the pull request. diff --git a/packages/core/src/git/collect.test.ts b/packages/core/src/git/collect.test.ts index f60f5cd..0251a5d 100644 --- a/packages/core/src/git/collect.test.ts +++ b/packages/core/src/git/collect.test.ts @@ -460,3 +460,42 @@ describe('window bounds in the stream', () => { expect(relative.since).not.toBe('1d'); }); }); + +describe('renamed files', () => { + // Found by an external review: `old.ts` → `git mv new.ts` → edit `new.ts` + // produced THREE paths in the stream — `old.ts`, `old.ts => new.ts` and + // `new.ts` — with the rename filed as a modification of a file that never + // existed. Every rename in a repository was one more eligible file that + // could never be retouched. + it('records a rename as one file under its new name, carrying the old one', async () => { + const dir = mkdtempSync(join(tmpdir(), 'evidtrail-rename-')); + const run = (cmd: string, date: string) => + execSync(cmd, { + cwd: dir, + env: { ...process.env, GIT_AUTHOR_DATE: date, GIT_COMMITTER_DATE: date, EVIDTRAIL_MODE: '', CLAUDECODE: '' }, + }); + try { + run('git init -q -b main', '2026-01-01T10:00:00Z'); + run('git config user.name test && git config user.email test@example.com', '2026-01-01T10:00:00Z'); + writeFileSync(join(dir, 'old.ts'), 'export const a = 1;\nexport const b = 2;\n'); + run('git add -A && git commit -q -m "add old"', '2026-01-01T10:00:00Z'); + run('git mv old.ts new.ts && git commit -q -m "rename"', '2026-01-02T10:00:00Z'); + writeFileSync(join(dir, 'new.ts'), 'export const a = 1;\nexport const b = 3;\n'); + run('git add -A && git commit -q -m "modify new"', '2026-01-04T10:00:00Z'); + + const stream = await collectCommits({ repoPath: dir }); + const byMessage = Object.fromEntries(stream.commits.map((c) => [c.message, c.stats.files])); + + expect(byMessage['rename']).toEqual([ + { path: 'new.ts', previousPath: 'old.ts', status: 'renamed', additions: 0, deletions: 0 }, + ]); + expect(byMessage['modify new']).toEqual([ + { path: 'new.ts', status: 'modified', additions: 1, deletions: 1 }, + ]); + const everyPath = stream.commits.flatMap((c) => c.stats.files.map((f) => f.path)); + expect(everyPath).not.toContain('old.ts => new.ts'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/core/src/git/diff.ts b/packages/core/src/git/diff.ts index 63a6272..89f4492 100644 --- a/packages/core/src/git/diff.ts +++ b/packages/core/src/git/diff.ts @@ -1,5 +1,6 @@ import { simpleGit, type SimpleGit } from 'simple-git'; import { FileChange } from '../schema/commit.js'; +import { splitRenamePath } from './paths.js'; import type { z } from 'zod'; type FileChangeType = z.infer; @@ -57,7 +58,8 @@ export async function getDiffStats(repoPathOrGit: string | SimpleGit, commitHash if (parts.length >= 3) { const additions = parts[0] === '-' ? 0 : parseInt(parts[0], 10) || 0; const deletions = parts[1] === '-' ? 0 : parseInt(parts[1], 10) || 0; - const path = parts[parts.length - 1]; + // Same rename form as `git log --numstat`: see paths.ts + const { path, previousPath } = splitRenamePath(parts.slice(2).join('\t')); // Skip binary files (marked with -) if (parts[0] === '-' && parts[1] === '-') { @@ -69,7 +71,8 @@ export async function getDiffStats(repoPathOrGit: string | SimpleGit, commitHash files.push({ path, - status: statusMap.get(path) || 'modified', + status: statusMap.get(path) || (previousPath ? 'renamed' : 'modified'), + ...(previousPath ? { previousPath } : {}), additions, deletions, }); diff --git a/packages/core/src/git/log.ts b/packages/core/src/git/log.ts index 9582a77..269f57f 100644 --- a/packages/core/src/git/log.ts +++ b/packages/core/src/git/log.ts @@ -1,5 +1,6 @@ import { SimpleGit } from 'simple-git'; import { FileChange } from '../schema/commit.js'; +import { splitRenamePath } from './paths.js'; import type { z } from 'zod'; type FileChangeType = z.infer; @@ -96,13 +97,15 @@ export async function logWithStats(git: SimpleGit, rangeArgs: string[]): Promise if (match[1] === '-' && match[2] === '-') continue; const additions = match[1] === '-' ? 0 : parseInt(match[1], 10) || 0; const deletions = match[2] === '-' ? 0 : parseInt(match[2], 10) || 0; - const pathParts = match[3].split('\t'); - const path = pathParts[pathParts.length - 1]; + // A rename is one numstat line whose path field names both sides; + // the name-status pass keyed its status by the new name. + const { path, previousPath } = splitRenamePath(match[3]); totalAdditions += additions; totalDeletions += deletions; files.push({ path, - status: statusMap?.get(path) || 'modified', + status: statusMap?.get(path) || (previousPath ? 'renamed' : 'modified'), + ...(previousPath ? { previousPath } : {}), additions, deletions, }); diff --git a/packages/core/src/git/paths.test.ts b/packages/core/src/git/paths.test.ts new file mode 100644 index 0000000..cfa5961 --- /dev/null +++ b/packages/core/src/git/paths.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect } from 'vitest'; +import { splitRenamePath } from './paths.js'; + +// The forms are copied from real `git log --numstat` output on react-router, +// where the brace form outnumbers the plain one forty to one. +describe('splitRenamePath', () => { + it('leaves an ordinary path alone', () => { + expect(splitRenamePath('src/index.ts')).toEqual({ path: 'src/index.ts' }); + // an arrow inside a name that is not a rename is not a rename + expect(splitRenamePath('docs/a=>b.md')).toEqual({ path: 'docs/a=>b.md' }); + }); + + it('splits the plain form', () => { + expect(splitRenamePath('old.ts => new.ts')).toEqual({ path: 'new.ts', previousPath: 'old.ts' }); + expect( + splitRenamePath( + 'integration/helpers/app/entry.server.tsx => packages/react-router-dev/config/defaults/entry.server.web.tsx' + ) + ).toEqual({ + path: 'packages/react-router-dev/config/defaults/entry.server.web.tsx', + previousPath: 'integration/helpers/app/entry.server.tsx', + }); + }); + + it('expands the brace form with a shared prefix and suffix', () => { + expect(splitRenamePath('.github/workflows/{shared-integration.yml => integration-run.yml}')).toEqual({ + path: '.github/workflows/integration-run.yml', + previousPath: '.github/workflows/shared-integration.yml', + }); + expect(splitRenamePath('src/{old => new}/index.ts')).toEqual({ + path: 'src/new/index.ts', + previousPath: 'src/old/index.ts', + }); + }); + + it('expands the brace form at the start of the path', () => { + expect( + splitRenamePath('{packages/react-router-dev/config/defaults => integration/helpers/app}/entry.server.tsx') + ).toEqual({ + path: 'integration/helpers/app/entry.server.tsx', + previousPath: 'packages/react-router-dev/config/defaults/entry.server.tsx', + }); + }); + + it('expands a brace form with an empty side without leaving a double slash', () => { + expect(splitRenamePath('src/{ => lib}/util.ts')).toEqual({ + path: 'src/lib/util.ts', + previousPath: 'src/util.ts', + }); + expect(splitRenamePath('src/{lib => }/util.ts')).toEqual({ + path: 'src/util.ts', + previousPath: 'src/lib/util.ts', + }); + }); +}); diff --git a/packages/core/src/git/paths.ts b/packages/core/src/git/paths.ts new file mode 100644 index 0000000..d819959 --- /dev/null +++ b/packages/core/src/git/paths.ts @@ -0,0 +1,42 @@ +// How git names a renamed file in `--numstat` output. +// +// With rename detection on (the default), a moved file is one line, not a +// delete plus an add, and its path field carries both names: +// +// old.ts => new.ts plain form, whole path differs +// src/{old => new}/index.ts brace form, shared prefix/suffix +// {packages/a => integration/b}/entry.tsx brace form at the start +// src/{ => lib}/util.ts brace form with an empty side +// +// Taking "the last tab-separated field" as the path — what this parser did +// before — kept the whole `old => new` string as a file name. Every rename +// then became a path that never existed, counted as one more eligible file, +// while the real file's later edits were filed under a name the first touch +// had never been recorded against. Found by an external review on a +// three-commit fixture; on react-router, 1,911 brace-form and 45 plain-form +// renames were doing this. +export interface SplitPath { + path: string; + previousPath?: string; +} + +const BRACE = /^(.*)\{(.*) => (.*)\}(.*)$/; +const PLAIN = /^(.+) => (.+)$/; + +function join(prefix: string, middle: string, suffix: string): string { + // `src/{ => lib}/util.ts` → old side is `src//util.ts` before cleanup + return `${prefix}${middle}${suffix}`.replace(/\/{2,}/g, '/'); +} + +export function splitRenamePath(raw: string): SplitPath { + const brace = raw.match(BRACE); + if (brace) { + const [, prefix, from, to, suffix] = brace; + return { path: join(prefix, to, suffix), previousPath: join(prefix, from, suffix) }; + } + const plain = raw.match(PLAIN); + if (plain) { + return { path: plain[2], previousPath: plain[1] }; + } + return { path: raw }; +} diff --git a/packages/core/src/schema/commit.ts b/packages/core/src/schema/commit.ts index 428b58a..8ff27cd 100644 --- a/packages/core/src/schema/commit.ts +++ b/packages/core/src/schema/commit.ts @@ -3,6 +3,10 @@ import { z } from 'zod'; export const FileChange = z.object({ path: z.string(), status: z.enum(['added', 'modified', 'deleted', 'renamed']).optional(), // best-effort + // Set on a rename: the name the file had before this commit. Lets a + // consumer that tracks files over time carry a file's history across + // the move instead of starting a new one under the new name. + previousPath: z.string().optional(), additions: z.number().int().nonnegative(), deletions: z.number().int().nonnegative(), }); diff --git a/packages/metrics/src/outcome-correlation.test.ts b/packages/metrics/src/outcome-correlation.test.ts index 36fa4d9..a9aeff7 100644 --- a/packages/metrics/src/outcome-correlation.test.ts +++ b/packages/metrics/src/outcome-correlation.test.ts @@ -285,3 +285,40 @@ describe('outcome rates against the base rate', () => { expect(result.reverts.rates.ai.count).toBe(0); }); }); + +describe('hotfixes across a rename', () => { + it('links a hotfix on the new name to the last touch under the old one', () => { + // a1 writes app.ts; a move-only commit renames it; a fix lands on the new + // name two days after the write. The antecedent is a1, not the move. + const result = calculateOutcomeCorrelation( + makeStream([ + makeCommit({ + hash: 'a1', + tags: aiTags, + authorDate: '2025-01-01T00:00:00.000Z', + stats: { totalAdditions: 1, totalDeletions: 0, files: [{ path: 'app.ts', additions: 1, deletions: 0 }] }, + }), + makeCommit({ + hash: 'mv', + tags: humanTags, + authorDate: '2025-01-02T00:00:00.000Z', + stats: { + totalAdditions: 0, + totalDeletions: 0, + files: [{ path: 'core/app.ts', previousPath: 'app.ts', status: 'renamed', additions: 0, deletions: 0 }], + }, + }), + makeCommit({ + hash: 'f1', + message: 'fix: null pointer', + authorDate: '2025-01-03T00:00:00.000Z', + stats: { totalAdditions: 1, totalDeletions: 0, files: [{ path: 'core/app.ts', additions: 1, deletions: 0, status: 'modified' }] }, + }), + ]), + { hotfixWindowDays: 7 } + ); + expect(result.hotfixes.linked).toBe(1); + expect(result.hotfixes.byAttribution.ai).toBe(1); + expect(result.hotfixes.byAttribution.human).toBe(0); + }); +}); diff --git a/packages/metrics/src/outcome-correlation.ts b/packages/metrics/src/outcome-correlation.ts index 79b1f1a..f227ab8 100644 --- a/packages/metrics/src/outcome-correlation.ts +++ b/packages/metrics/src/outcome-correlation.ts @@ -142,6 +142,17 @@ function calculateHotfixes( // antecedent, but it does become one for a later hotfix (a chain of // fixes attributes to the immediately preceding touch, not the origin). for (const file of commit.stats.files) { + if (file.previousPath !== undefined) { + // The file moved: its touch history moves with it. A pure rename + // leaves the prior touch as the antecedent; a rename with edits + // becomes the touch, like any other edit. + const carried = lastTouch.get(file.previousPath); + lastTouch.delete(file.previousPath); + if (carried && file.additions + file.deletions === 0) { + lastTouch.set(file.path, carried); + continue; + } + } if (file.status === 'deleted') { lastTouch.delete(file.path); } else { diff --git a/packages/metrics/src/persistence.test.ts b/packages/metrics/src/persistence.test.ts index f71a065..411338b 100644 --- a/packages/metrics/src/persistence.test.ts +++ b/packages/metrics/src/persistence.test.ts @@ -593,3 +593,58 @@ describe('observation end under --until', () => { expect(relative.avgDays).toBe(plain.avgDays); }); }); + +describe('renames follow the file', () => { + const aiTags: CommitStream['commits'][0]['tags'] = { attribution: 'ai', automated: false, mode: 'agent', evidence: 'declared', level: 'explicit', sources: [] }; + const file = (path: string, additions: number, deletions: number, previousPath?: string) => ({ + path, + additions, + deletions, + ...(previousPath ? { previousPath, status: 'renamed' as const } : {}), + }); + const commit = (hash: string, date: string, files: ReturnType[]) => + makeCommit({ hash, authorDate: date, tags: aiTags, stats: { totalAdditions: 0, totalDeletions: 0, files } }); + const week = (stream: CommitStream) => + calculatePersistence(stream).rapidRetouch.find((r) => r.windowDays === 7)!; + + it('counts an edit after a rename as a retouch of the original file', () => { + // old.ts (1 Jan) → new.ts (2 Jan, pure rename) → edited (4 Jan). Before: + // three eligible files and no retouch — 0%. The truth is one file, + // retouched on day 3 — 100%. + const stream = makeStream([ + commit('a', '2024-01-01T00:00:00.000Z', [file('old.ts', 2, 0)]), + commit('b', '2024-01-02T00:00:00.000Z', [file('new.ts', 0, 0, 'old.ts')]), + commit('c', '2024-01-04T00:00:00.000Z', [file('new.ts', 1, 1)]), + ]); + expect(week(stream)).toMatchObject({ retouched: 1, eligible: 1, tooRecent: 0, rate: 1 }); + expect(calculatePersistence(stream).filesConsidered).toBe(1); + }); + + it('does not count a pure rename as a touch, and does not open a lifecycle with one', () => { + const stream = makeStream([ + commit('a', '2024-01-01T00:00:00.000Z', [file('old.ts', 2, 0)]), + commit('b', '2024-01-02T00:00:00.000Z', [file('new.ts', 0, 0, 'old.ts')]), + // nothing else: the file survives under its new name + ]); + expect(week(stream)).toMatchObject({ retouched: 0, eligible: 1, tooRecent: 0, rate: 0 }); + + // a target commit that only moves a file it never wrote starts no clock + const moveOnly = makeStream([ + makeCommit({ + hash: 'h', + authorDate: '2024-01-01T00:00:00.000Z', + stats: { totalAdditions: 2, totalDeletions: 0, files: [file('old.ts', 2, 0)] }, + }), // not a target commit + commit('b', '2024-01-02T00:00:00.000Z', [file('new.ts', 0, 0, 'old.ts')]), + ]); + expect(calculatePersistence(moveOnly).filesConsidered).toBe(0); + }); + + it('counts a rename that also edits as a touch', () => { + const stream = makeStream([ + commit('a', '2024-01-01T00:00:00.000Z', [file('old.ts', 2, 0)]), + commit('b', '2024-01-03T00:00:00.000Z', [file('new.ts', 1, 1, 'old.ts')]), + ]); + expect(week(stream)).toMatchObject({ retouched: 1, eligible: 1 }); + }); +}); diff --git a/packages/metrics/src/persistence.ts b/packages/metrics/src/persistence.ts index 09042b5..3488924 100644 --- a/packages/metrics/src/persistence.ts +++ b/packages/metrics/src/persistence.ts @@ -100,33 +100,49 @@ export function calculatePersistence( // graph. Committer time remains the elapsed-time proxy for integration. const sortedCommits = [...commitStream.commits].reverse(); - // First target-cohort touch per file + // One chronological pass: a file's lifecycle opens at its first + // target-cohort touch and closes at the first later touch by anyone. + // + // A lifecycle is keyed by path, and paths move. When a commit renames a + // file, the lifecycle follows it to the new name — otherwise the move + // opened a second, empty lifecycle under the new name while the old one + // sat untouched forever, and the file's real edits counted against + // neither. A pure rename (no lines changed) is not a touch: nothing about + // the code happened, only its address. A rename that also edits is. const lifecycles = new Map(); let filesExcluded = 0; sortedCommits.forEach((commit, index) => { - if (!isTarget(commit)) return; + const target = isTarget(commit); for (const file of commit.stats.files) { - if (lifecycles.has(file.path)) continue; - const category = categorizeFile(file.path); - const isExcluded = onlyCategory ? category !== onlyCategory : excluded.has(category); - if (isExcluded) { - filesExcluded++; - lifecycles.set(file.path, { firstTargetIndex: -1, firstTargetDate: new Date(0), eventDate: null }); - continue; + let isTouch = true; + if (file.previousPath !== undefined) { + const carried = lifecycles.get(file.previousPath); + if (carried) { + lifecycles.delete(file.previousPath); + if (!lifecycles.has(file.path)) lifecycles.set(file.path, carried); + } + isTouch = file.additions + file.deletions > 0; } - lifecycles.set(file.path, { - firstTargetIndex: index, - firstTargetDate: new Date(commit.committerDate), - eventDate: null, - }); - } - }); + if (!isTouch) continue; - // First subsequent modification (or deletion) ends the survival clock - sortedCommits.forEach((commit, index) => { - for (const file of commit.stats.files) { const lifecycle = lifecycles.get(file.path); - if (!lifecycle || lifecycle.firstTargetIndex < 0) continue; // excluded category + if (!lifecycle) { + if (!target) continue; // not yet in the cohort's story + const category = categorizeFile(file.path); + const isExcluded = onlyCategory ? category !== onlyCategory : excluded.has(category); + if (isExcluded) { + filesExcluded++; + lifecycles.set(file.path, { firstTargetIndex: -1, firstTargetDate: new Date(0), eventDate: null }); + continue; + } + lifecycles.set(file.path, { + firstTargetIndex: index, + firstTargetDate: new Date(commit.committerDate), + eventDate: null, + }); + continue; + } + if (lifecycle.firstTargetIndex < 0) continue; // excluded category if (index <= lifecycle.firstTargetIndex) continue; // not later than first touch if (lifecycle.eventDate === null) { lifecycle.eventDate = new Date(commit.committerDate);