From a0a7364917d5a582a103e0e953357ba00ab79d63 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 31 Oct 2026 08:47:20 +0800 Subject: [PATCH] fix(core): keep session-commit registration for an absolute cd into the same repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cdTargetMayChangeRepo` returned true for ANY target starting with '/', with no reference to the shell's cwd. The product-recommended spelling `cd /abs/path/to/this/repo && git commit -m "..."` therefore latched `cwdShifted`, `attributableInCwd` went false, `trackSessionCommit` never ran, `sessionCommitShas` stayed empty, and the agent's own next `git commit --amend` was hard-blocked by the amend guard. Resolve an absolute target against the shell cwd and treat the cwd itself or a subdirectory of it as in-repo. Sibling and out-of-tree targets still shift, including the `-sibling` case that a bare `startsWith` would wrongly admit. Relative targets are untouched, and resolution stays static (`path.resolve`) — no `git rev-parse` in the walk. `gitCommitContext`, `isAmendCommit` and `findAttributableCommitSegment` now take the cwd; all four module-private call sites thread it through. Co-authored-by: Qwen-Coder Patrol-Run: qwen-issue-patrol/jmvvn3bibqk --- .../shell.session-commit-tracking.test.ts | 53 ++++++++++++++++ packages/core/src/tools/shell.ts | 61 ++++++++++++++----- 2 files changed, 98 insertions(+), 16 deletions(-) diff --git a/packages/core/src/tools/shell.session-commit-tracking.test.ts b/packages/core/src/tools/shell.session-commit-tracking.test.ts index 49eab48d138..70c9f2eab24 100644 --- a/packages/core/src/tools/shell.session-commit-tracking.test.ts +++ b/packages/core/src/tools/shell.session-commit-tracking.test.ts @@ -462,6 +462,59 @@ describe.skipIf(process.platform === 'win32')( await expectOtherRepoCommitBlocked(runShellCommand); }); + it('registers a commit after an absolute `cd` into the same repo root (issue #12514)', async () => { + // #12514-A: `cdTargetMayChangeRepo` returns true for ANY target starting + // with '/', with no reference to the shell cwd. So the + // product-recommended spelling `cd /abs/path/to/this/repo && git commit` + // latched `cwdShifted`, `attributableInCwd` went false, + // `trackSessionCommit` never ran and `sessionCommitShas` stayed empty — + // which is why the agent's own next `git commit --amend` was hard-blocked + // by the guard (reported by bainmariehk, 2026-10-07). + await expectRegisters(`cd ${repoDir} && ${COMMIT_FEATURE}`, false); + }); + + it('registers a commit after an absolute `cd` into a subdirectory of the same repo (issue #12514)', async () => { + // #12514-A, subdirectory arm: an absolute target inside the cwd tree must + // count as in-repo, not only an exact-equal path. `git` resolves the + // toplevel from any subdirectory, so the commit still lands in our repo + // and must still be registered (and carry the trailer). + const subDir = path.join(repoDir, 'sub'); + fs.mkdirSync(subDir, { recursive: true }); + const preHead = headSha(); + fs.writeFileSync(path.join(subDir, 'feature.txt'), 'work\n'); + await runShellCommand( + `cd ${subDir} && git add feature.txt && git commit -m "feature"`, + ); + expectFeatureCommitLanded(preHead); + expect(amendVerdict()).toBeNull(); + }); + + it('keeps the cwd-shift latch for an absolute `cd` into a sibling directory that shares the repo path prefix (issue #12514)', async () => { + // Guards the obvious wrong fix: a bare `target.startsWith(cwd)` without a + // separator boundary. `-sibling` shares the repo path as a *string* + // prefix but is a different repository, so admitting it would stamp our + // trailer onto a foreign repo's commit — the corruption `cwdShifted` + // exists to prevent. + const siblingDir = `${repoDir}-sibling`; + tempDirs.push(siblingDir); + fs.mkdirSync(siblingDir, { recursive: true }); + initRepo(siblingDir, { subject: 'sibling seed' }); + fs.writeFileSync(path.join(siblingDir, 'work.txt'), 'sibling work\n'); + const repoHeadBefore = headSha(); + + await runShellCommand( + `cd ${siblingDir} && git add work.txt && git commit -m "sibling work"`, + ); + + // The commit really landed — in the SIBLING repository. + expect(lastLog('%s', siblingDir)).toBe('sibling work'); + expect(headSha()).toBe(repoHeadBefore); + expect(lastLog('%B', siblingDir)).not.toContain( + 'Co-authored-by: Qwen-Coder', + ); + expect(amendVerdict()?.blocked).toBe(true); + }); + // R1-11: `COMMIT_RECOGNITION_LEADING_NOISE` feeds the one helper both // recognisers call, so a row per token pins registration and the trailer // rewrite at once. Each token leads the segment carrying `git commit` diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 36b8a63783c..a5bb2801b16 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -762,7 +762,10 @@ function programBasename(program: string): string { * attribution. The trade-off matches the wrong-repo guard's intent * (better miss than corrupt unrelated repos). */ -function gitCommitContext(command: string): { +function gitCommitContext( + command: string, + cwd: string, +): { hasCommit: boolean; attributableInCwd: boolean; } { @@ -793,7 +796,7 @@ function gitCommitContext(command: string): { // --show-toplevel` after the cd, which would require a synchronous // fs/exec call that the rest of this walk avoids — the heuristic // covers the common case and stays conservative on the rest. - if (!hasCommit && cdTargetMayChangeRepo(tokens)) cwdShifted = true; + if (!hasCommit && cdTargetMayChangeRepo(tokens, cwd)) cwdShifted = true; continue; } if (program === 'popd') { @@ -864,20 +867,37 @@ function parseGhInvocation(tokens: string[]): string[] { * whether a subsequent `git commit` in the same chain is still * attributable in our cwd. * - * Returns true (conservative — assume shift) when the target is - * absolute, escapes upward (`..`), goes to `$HOME` / `~`, contains an - * env-var (we can't resolve it statically), or is missing entirely - * (`cd` alone goes to `$HOME`). Plain relative paths like `cd src`, - * `cd ./packages/foo`, or `cd subdir/nested` are treated as in-repo. + * Returns true (conservative — assume shift) when the target escapes + * upward (`..`), goes to `$HOME` / `~`, contains an env-var (we can't + * resolve it statically), or is missing entirely (`cd` alone goes to + * `$HOME`). Plain relative paths like `cd src`, `cd ./packages/foo`, + * or `cd subdir/nested` are treated as in-repo. + * + * An absolute target is resolved against `cwd` (the directory the shell + * command runs in, #12514-A): landing on that directory or in a + * subdirectory of it cannot change the repository, so the very common + * `cd /abs/path/to/this/repo && git commit -m "..."` spelling keeps + * attribution. Anything else absolute — a sibling, an unrelated tree — + * still shifts. Resolution is static (`path.resolve`) on purpose: the + * walk must not spawn `git rev-parse` (see `gitCommitContext`), and a + * symlinked spelling that resolves elsewhere is still caught by the + * equality/prefix test because both sides go through the same `cwd`. */ -function cdTargetMayChangeRepo(tokens: string[]): boolean { +function cdTargetMayChangeRepo(tokens: string[], cwd: string): boolean { // tokens[0] is 'cd'. The next non-flag token is the target. let i = 1; while (i < tokens.length && tokens[i]!.startsWith('-')) i++; const target = tokens[i]; // `cd` with no argument goes to $HOME. if (target === undefined) return true; - if (target.startsWith('/')) return true; + if (target.startsWith('/')) { + // Same directory, or a subdirectory of it: still this repo. The + // `path.sep` in the prefix test is load-bearing — `-sibling` + // shares the string prefix but is a different repository. + const root = path.resolve(cwd); + const resolved = path.resolve(cwd, target); + return !(resolved === root || resolved.startsWith(root + path.sep)); + } if (target.startsWith('~')) return true; // Env-var reference (e.g. `$HOME`, `$REPO`) — can't resolve here. if (target.includes('$')) return true; @@ -910,14 +930,14 @@ function cdTargetMayChangeRepo(tokens: string[]): boolean { * must not flip the diff range for the second (fresh) commit, since * `preHead` would be the inner repo's SHA there, not ours. */ -function isAmendCommit(command: string): boolean { +function isAmendCommit(command: string, cwd: string): boolean { let cwdShifted = false; for (const sub of splitCommands(command)) { const tokens = tokeniseSegment(sub); if (!tokens || tokens.length === 0) continue; const program = tokens[0]!; if (program === 'cd' || program === 'pushd') { - if (!cwdShifted && cdTargetMayChangeRepo(tokens)) cwdShifted = true; + if (!cwdShifted && cdTargetMayChangeRepo(tokens, cwd)) cwdShifted = true; continue; } if (program === 'popd') { @@ -985,6 +1005,7 @@ function isAmendCommit(command: string): boolean { */ function findAttributableCommitSegment( command: string, + cwd: string, ): { start: number; end: number } | null { let cursor = 0; let cwdShifted = false; @@ -1013,7 +1034,7 @@ function findAttributableCommitSegment( // Mirror gitCommitContext's cd/pushd heuristic: relative paths // that don't escape upward are treated as in-repo, so // `cd subdir && git commit ...` still finds the segment. - if (!cwdShifted && cdTargetMayChangeRepo(looseTokens)) { + if (!cwdShifted && cdTargetMayChangeRepo(looseTokens, cwd)) { cwdShifted = true; } continue; @@ -2502,7 +2523,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // text like `echo "git commit"` doesn't trigger snapshot/notes, // and so attribution still runs after a `git commit && cd ..` // chain (which would have failed an "any cd anywhere" gate). - const commitCtx = gitCommitContext(strippedCommand); + const commitCtx = gitCommitContext(strippedCommand, cwd); // Capture preHead only when the commit will actually be // attributed in our cwd: that's the only consumer (the // `attributableInCwd` branch below feeds preHead into @@ -3065,7 +3086,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // — `preHead` was captured synchronously before spawn and is // the pre-amend SHA, so this range captures only the amend // delta. - const isAmend = isAmendCommit(strippedCommand); + const isAmend = isAmendCommit(strippedCommand, cwd); attributionWarning = await this.attachCommitAttribution( cwd, preHead, @@ -4225,7 +4246,12 @@ export class ShellToolInvocation extends BaseToolInvocation< // Use the broader `hasCommit` flag rather than `attributableInCwd`: // `cd /elsewhere && git commit` should still be refused even // though we wouldn't attribute it. - if (gitCommitContext(strippedCommand).hasCommit) { + if ( + gitCommitContext( + strippedCommand, + this.params.directory || this.config.getTargetDir(), + ).hasCommit + ) { return { llmContent: 'Refusing to run `git commit` in background mode: AI-attribution notes ' + @@ -5388,7 +5414,10 @@ export class ShellToolInvocation extends BaseToolInvocation< // (with the trailer mid-string) back to the executor. The // in-cwd check is what we want here: only inject the // trailer when we're confident the commit lands in our cwd. - const segmentRange = findAttributableCommitSegment(command); + const segmentRange = findAttributableCommitSegment( + command, + this.params.directory || this.config.getTargetDir(), + ); if (!segmentRange) { return command; }