From dd45e1720138f489551dcef46eddda2f79d50ee3 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 6 May 2026 08:48:47 +0800 Subject: [PATCH] =?UTF-8?q?fix(attribution):=20runGit=20null-on-failure,?= =?UTF-8?q?=20versionless=20v3=E2=86=92v4=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit z54M (Copilot): runGit returned '' on both successful-empty-output and silent failure, so a `--name-only` that errored mid-way through the diff fan-out aliased to a real `--allow-empty` commit. The empty-commit branch then preserved pending attributions, leaving the just-committed file's tracked AI edit alive to re-attribute on the next commit. Switch runGit to `Promise`, distinguishing exit code 0 (any output, including '') from non-zero (null). The diff-stage fan-out and ancillary probes now treat null as analysis failure and bail with `return null` instead of falling into the empty-commit path. z539 (Copilot): the v3→v4 `shouldMigrate` only fired on `$version === 3`. A versionless settings file carrying the legacy `general.gitCoAuthor: false` boolean would skip every migration (gitCoAuthor isn't in V1_INDICATOR_KEYS — it post-dates V2), get its `$version` normalized to 4 by the loader, and leave the boolean in place. The settings dialog then reads the V4 `{commit, pr}` shape, sees missing keys, defaults both to true, and silently overwrites the user's opt-out on the next save. Also fire when `$version` is absent AND the value at `general.gitCoAuthor` is a boolean. Tests cover the new path and confirm the existing versioned/object-shape paths are untouched. --- .../migration/versions/v3-to-v4.test.ts | 29 +++++++++++ .../src/config/migration/versions/v3-to-v4.ts | 20 ++++++- packages/core/src/tools/shell.ts | 52 ++++++++++++++++--- 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/config/migration/versions/v3-to-v4.test.ts b/packages/cli/src/config/migration/versions/v3-to-v4.test.ts index 8b3b25c60..65fa13fab 100644 --- a/packages/cli/src/config/migration/versions/v3-to-v4.test.ts +++ b/packages/cli/src/config/migration/versions/v3-to-v4.test.ts @@ -39,6 +39,35 @@ describe('V3ToV4Migration', () => { expect(migration.shouldMigrate('x')).toBe(false); expect(migration.shouldMigrate(42)).toBe(false); }); + + // `gitCoAuthor` post-dates the V1 indicator-key list, so a settings + // file that has ONLY this legacy boolean shape (no `$version`, + // no other migration-triggering keys) wouldn't fire any earlier + // migration. The v3→v4 step must catch it directly so the dialog + // doesn't silently overwrite the user's stored opt-out with the + // schema defaults on next save. + it('returns true for versionless settings with legacy boolean gitCoAuthor', () => { + expect( + migration.shouldMigrate({ + general: { gitCoAuthor: false }, + }), + ).toBe(true); + }); + + it('returns false for versionless settings without gitCoAuthor', () => { + expect(migration.shouldMigrate({ general: {} })).toBe(false); + expect(migration.shouldMigrate({})).toBe(false); + }); + + it('returns false for versionless settings with already-object gitCoAuthor', () => { + // User who hand-edited to the v4 shape — let the loader's + // version normalization handle it without rewriting. + expect( + migration.shouldMigrate({ + general: { gitCoAuthor: { commit: false, pr: true } }, + }), + ).toBe(false); + }); }); describe('migrate', () => { diff --git a/packages/cli/src/config/migration/versions/v3-to-v4.ts b/packages/cli/src/config/migration/versions/v3-to-v4.ts index 34795303a..7f8ca7578 100644 --- a/packages/cli/src/config/migration/versions/v3-to-v4.ts +++ b/packages/cli/src/config/migration/versions/v3-to-v4.ts @@ -39,7 +39,25 @@ export class V3ToV4Migration implements SettingsMigration { return false; } const s = settings as Record; - return s['$version'] === 3; + if (s['$version'] === 3) { + return true; + } + // Versionless settings file (no $version key) with the legacy + // boolean `gitCoAuthor` shape: the V1/V2 migrations don't list + // `gitCoAuthor` as an indicator key (it post-dates them), so a + // settings file that has ONLY this shape wouldn't trigger any + // earlier migration and would land here at the v3→v4 boundary + // without being rewritten. Handle the boolean directly so the + // settings dialog (which reads the v4 `{commit, pr}` shape) can + // surface the user's prior choice instead of silently overwriting + // their opt-out with the schema defaults on first save. + if (s['$version'] === undefined) { + const value = getNestedProperty(s, GIT_CO_AUTHOR_PATH); + if (typeof value === 'boolean') { + return true; + } + } + return false; } migrate( diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 50c066585..883366a22 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -2122,7 +2122,15 @@ export class ShellToolInvocation extends BaseToolInvocation< deletedFiles: new Set(), }; - const runGit = async (args: string): Promise => { + // Distinguish a successful git command with no output (e.g. + // `--allow-empty` -> empty `--name-only` listing) from a failed + // git command (silenced by ShellExecutionService) so the caller + // can choose between the empty-commit sentinel and the analysis- + // failure sentinel. Returning the same `''` for both used to + // alias `--allow-empty` to a `--name-only` failure, which left + // pending attributions tracked across the just-committed file + // and re-attributed it on the next commit. + const runGit = async (args: string): Promise => { const handle = await ShellExecutionService.execute( `git ${args}`, cwd, @@ -2132,7 +2140,7 @@ export class ShellToolInvocation extends BaseToolInvocation< {}, ); const r = await handle.result; - return r.exitCode === 0 ? r.output : ''; + return r.exitCode === 0 ? r.output : null; }; try { @@ -2161,7 +2169,19 @@ export class ShellToolInvocation extends BaseToolInvocation< runGit('log -1 --pretty=%P HEAD'), runGit('rev-parse --show-toplevel'), ]); - const hasParent = hasParentOutput.length > 0; + // `rev-parse --verify HEAD~1` is allowed to fail (shallow + // clone, true root commit) — treat null and '' uniformly. + const hasParent = hasParentOutput !== null && hasParentOutput.length > 0; + // `log -1 --pretty=%P HEAD` MUST succeed; if git can't read the + // current HEAD's metadata we have no way to tell shallow apart + // from a real root commit. Bail. + if (parentShaOutput === null) { + debugLogger.warn( + 'getCommittedFileInfo: log -1 --pretty=%P HEAD failed; ' + + 'cannot distinguish shallow clone from true root commit.', + ); + return null; + } const isTrueRootCommit = parentShaOutput.trim().length === 0; // Shallow clone: HEAD has a parent recorded but the object // isn't local. Bail rather than over-attribute via --root. @@ -2177,8 +2197,9 @@ export class ShellToolInvocation extends BaseToolInvocation< // reconcile paths from `git diff` (relative to the toplevel) // against absolute paths recorded by the edit/write tools. // Using the configured target directory as base would zero out - // attribution for any file outside it. - const repoRoot = repoRootOutput.trim(); + // attribution for any file outside it. Tolerate failure (null + // -> empty string -> caller falls back to targetDir). + const repoRoot = (repoRootOutput ?? '').trim(); // Choose the diff range: // - amend: `HEAD@{1}..HEAD` — the actual amend delta. The @@ -2191,8 +2212,8 @@ export class ShellToolInvocation extends BaseToolInvocation< let diffArgs: { name: string; status: string; numstat: string }; if (isAmend) { // Verify HEAD@{1} actually exists; reflogs can be GC'd. - const hasReflog = - (await runGit('rev-parse --verify HEAD@{1}')).length > 0; + const reflogProbe = await runGit('rev-parse --verify HEAD@{1}'); + const hasReflog = reflogProbe !== null && reflogProbe.length > 0; if (!hasReflog) { // Without a pre-amend snapshot we can't compute the amend // delta; emitting `HEAD~1..HEAD` would over-attribute. @@ -2226,6 +2247,23 @@ export class ShellToolInvocation extends BaseToolInvocation< runGit(diffArgs.numstat), ]); + // ANY of the three diffs failing (null) is an analysis failure, + // NOT an empty commit. Without this check, a `--name-only` that + // failed silently used to alias to `--allow-empty`, leaving the + // just-committed file's tracked AI edit in the singleton and + // re-attributing it to the next commit. + if ( + nameOutput === null || + statusOutput === null || + numstatOutput === null + ) { + debugLogger.warn( + 'getCommittedFileInfo: one or more diff calls failed; ' + + 'cannot distinguish empty commit from analysis failure.', + ); + return null; + } + const files = nameOutput .split('\n') .map((f) => f.trim())