From 852ac8a836e695ac2fd5eb7bc3cefbd1ebc74edf Mon Sep 17 00:00:00 2001 From: Andrey Avtomonov Date: Tue, 9 Jun 2026 14:55:08 +0200 Subject: [PATCH] fix(git): scope the dirty-main squash guard to staged changes only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard rejected any uncommitted tracked change (git status --untracked-files=no), which also caught unstaged edits like a ktx.yaml that setup writes during the flow and commits only after the context build — so the guard wrongly blocked setup context builds and ingest (8 local-bundle-ingest cases failed with 'uncommitted changes (ktx.yaml)'). The actual hazard is the index, not the working tree: 'git commit' captures only staged changes, and the auto_commit:false residue is staged by 'git merge --squash'. Narrow the check to 'git diff --cached' so only pre-existing staged changes are refused; unstaged working-tree edits proceed untouched (never committed by the squash). Adds a regression test that an unstaged tracked change does not block the merge and is neither committed nor discarded. --- packages/cli/src/context/core/git.service.ts | 20 +++++++------ .../context/ingest/ingest-bundle.runner.ts | 4 +-- .../context/memory/memory-agent.service.ts | 4 +-- .../cli/test/context/core/git.service.test.ts | 30 +++++++++++++++++++ 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/context/core/git.service.ts b/packages/cli/src/context/core/git.service.ts index 932a5b81..9343eb61 100644 --- a/packages/cli/src/context/core/git.service.ts +++ b/packages/cli/src/context/core/git.service.ts @@ -744,19 +744,21 @@ export class GitService { | { ok: false; conflict: true; conflictPaths: string[] } | { ok: false; dirty: true; dirtyPaths: string[] } > { - // The squash flow commits the whole index and, on conflict, `reset --hard`s the tree, so - // it must start from a clean target. Refuse if the target has uncommitted tracked changes - // (e.g. residue from a prior `auto_commit: false` run) rather than committing them under - // this run's message or discarding them. Untracked files (gitignored .ktx state, stray - // files) are not committed by the squash and are intentionally ignored here. - const dirtyPaths = (await this.git.raw(['status', '--porcelain', '--untracked-files=no'])) + // `git commit` after the squash commits the WHOLE index, and conflict cleanup `reset + // --hard`s the tree. So pre-existing *staged* changes are the hazard: they would be + // committed under this run's message, or discarded on conflict. The residue from a prior + // `auto_commit: false` run is exactly that — `git merge --squash` stages without + // committing. Refuse when the index already differs from HEAD. Unstaged working-tree edits + // (e.g. a `ktx.yaml` written by setup and committed later, or user edits) are NOT captured + // by `git commit`, so they must not block the squash; untracked files are likewise ignored. + const dirtyPaths = (await this.git.raw(['diff', '--cached', '--name-only'])) .split('\n') - .map((line) => line.slice(3).trim()) + .map((line) => line.trim()) .filter(Boolean); if (dirtyPaths.length > 0) { this.logger.warn( - `applySquashToIndex: target worktree has ${dirtyPaths.length} uncommitted change(s) ` + - `(${dirtyPaths.slice(0, 5).join(', ')}); refusing to squash ${branch} to avoid contaminating or discarding them`, + `applySquashToIndex: target index has ${dirtyPaths.length} staged change(s) ` + + `(${dirtyPaths.slice(0, 5).join(', ')}); refusing to squash ${branch} to avoid committing or discarding them`, ); return { ok: false, dirty: true, dirtyPaths }; } diff --git a/packages/cli/src/context/ingest/ingest-bundle.runner.ts b/packages/cli/src/context/ingest/ingest-bundle.runner.ts index a1d05aed..90207c5e 100644 --- a/packages/cli/src/context/ingest/ingest-bundle.runner.ts +++ b/packages/cli/src/context/ingest/ingest-bundle.runner.ts @@ -2700,8 +2700,8 @@ export class IngestBundleRunner { await this.deps.runs.markFailed(runRow.id); if ('dirty' in squashResult.merge) { throw new Error( - 'The project working tree has uncommitted changes ' + - `(${squashResult.merge.dirtyPaths.slice(0, 5).join(', ')}); commit or discard them before ingesting ` + + 'The project has staged but uncommitted changes ' + + `(${squashResult.merge.dirtyPaths.slice(0, 5).join(', ')}); commit or unstage them before ingesting ` + '(this typically means a previous run with storage.git.auto_commit: false was left staged).', ); } diff --git a/packages/cli/src/context/memory/memory-agent.service.ts b/packages/cli/src/context/memory/memory-agent.service.ts index a33cce4d..6d649203 100644 --- a/packages/cli/src/context/memory/memory-agent.service.ts +++ b/packages/cli/src/context/memory/memory-agent.service.ts @@ -276,8 +276,8 @@ export class MemoryAgentService { sessionOutcome = 'conflict'; if ('dirty' in mergeResult) { this.logger.warn( - `[memory-agent] chat=${chatId} not landed: project working tree has uncommitted changes ` + - `(${mergeResult.dirtyPaths.slice(0, 5).join(', ')}); commit or discard them and re-run ` + + `[memory-agent] chat=${chatId} not landed: project has staged but uncommitted changes ` + + `(${mergeResult.dirtyPaths.slice(0, 5).join(', ')}); commit or unstage them and re-run ` + '(a previous run with auto_commit disabled may have been left staged).', ); } else { diff --git a/packages/cli/test/context/core/git.service.test.ts b/packages/cli/test/context/core/git.service.test.ts index 3b670c94..89b2f080 100644 --- a/packages/cli/test/context/core/git.service.test.ts +++ b/packages/cli/test/context/core/git.service.test.ts @@ -496,6 +496,36 @@ describe('GitService', () => { await rm(wtDir, { recursive: true, force: true }).catch(() => undefined); }); + it('allows the squash when main has only unstaged (not staged) changes', async () => { + // e.g. setup writes ktx.yaml during the flow and commits it only after the context + // build, leaving it modified-but-unstaged. `git commit` never captures unstaged changes, + // so the squash must proceed and leave them untouched. + const { commitHash: baseSha } = await writeAndCommit('config.yaml', 'a: 1\n'); + const parent = await realpath(join(tempDir, '..')); + const wtDir = join(parent, `wt-${Date.now()}-unstaged`); + await service.addWorktree(wtDir, 'session/unstaged', baseSha); + const scoped = service.forWorktree(wtDir); + await writeFile(join(wtDir, 'from-branch.yaml'), 'b: 1\n', 'utf-8'); + await scoped.commitFile('from-branch.yaml', 'wip', 'System User', 'system@example.com'); + + // Modify a tracked file on main WITHOUT staging it. + await writeFile(join(tempDir, 'config.yaml'), 'a: 2\n', 'utf-8'); + + const result = await service.squashMergeIntoMain('session/unstaged', 'System User', 'system@example.com', 'msg'); + + expect(result.ok).toBe(true); + if (!result.ok || !('squashSha' in result)) { + throw new Error('expected the squash to land'); + } + expect(result.touchedPaths).toEqual(['from-branch.yaml']); + // The branch landed, and the unstaged edit was neither committed nor discarded. + await expect(readFile(join(tempDir, 'from-branch.yaml'), 'utf-8')).resolves.toBe('b: 1\n'); + await expect(readFile(join(tempDir, 'config.yaml'), 'utf-8')).resolves.toBe('a: 2\n'); + + await service.removeWorktree(wtDir).catch(() => undefined); + await rm(wtDir, { recursive: true, force: true }).catch(() => undefined); + }); + it('stageSquashMergeIntoMain also refuses on a dirty main', async () => { const { commitHash: baseSha } = await writeAndCommit('seed.md', 'seed'); const parent = await realpath(join(tempDir, '..'));