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())