From f087e699af32fed9ad6df8a345627adc9f49493a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Sat, 7 Nov 2026 14:44:43 +0800 Subject: [PATCH] fix(core): register promoted initial commits in unborn repos (#16081) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(core): register promoted initial commits in unborn repos A promoted command captured its pre-spawn HEAD as null both when the repository was unborn and when the probe failed. The registration path then skipped both cases, so amending the agent's own initial commit was blocked.\n\nCapture the pre-spawn state as either a SHA or an explicitly unborn HEAD. The promoted lineage check now requires a null predecessor only for the unborn case, while repository or probe failures still skip registration. Add a real-git regression for a promoted initial commit. Co-authored-by: Qwen-Coder * fix(core): fail closed when the pre-spawn HEAD probe is terminated Only a genuine git non-zero answer (status 128, no signal) proceeds to the repository probe. A killed, timed-out, or failed-to-spawn HEAD lookup now returns null instead of asserting a verified unborn repository, which the promoted path could otherwise pair with a single-entry reflog to register a pre-existing root commit the agent never made. Co-authored-by: Qwen-Coder * style(core): prettier formatting for probe regression test Co-authored-by: Qwen-Coder * docs(core): scope the lineage guard's fail-closed claim to born HEAD R1-1: the docblock stated "a foreign commit never earns an exemption" as a fail-closed property, which the unborn arm makes false — `null` is a sentinel every root commit satisfies, so a root commit somebody else lands during the promoted window is indistinguishable from this command's own. Say that, and scope the five-star guarantee to a born HEAD. The behaviour is the trade tracked on #12523; only the written invariant was wrong. R1-2: pin that the PATH `git` shim is live before relying on it. Both closing assertions also hold when the shim is bypassed (the lineage check then rejects on a non-null predecessor), so the row could green without exercising the probe-termination path it exists to pin. Verified by mutation: changing the shim's match from `--verify` to `--bypass` turns this row red. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmw1dkg4zbx * docs(core): scope the promoted-lineage claim to what the predecessor proves The `requirePredecessor` paragraph said "a foreign commit never earns an exemption", and the promoted call site said the guard keeps the registered SHA one this command created. The lineage check only proves the newest reflog `commit:` entry moved HEAD from `preHead`; a foreign commit landed directly on `preHead` while this command's own commit never landed has that same predecessor and still registers, so both statements overclaimed. Scope them: born-HEAD narrowing is not closing, and the fail-closed guarantee holds only once this command's own commit landed first. Co-authored-by: Qwen-Coder Patrol-Run: qwen-pr-closeout/jmw2g5c3hdf --------- Co-authored-by: Qwen-Coder --- .../shell.session-commit-tracking.test.ts | 63 ++++++- packages/core/src/tools/shell.ts | 161 ++++++++++++------ 2 files changed, 168 insertions(+), 56 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 b2d066c12b3..ff97a99a936 100644 --- a/packages/core/src/tools/shell.session-commit-tracking.test.ts +++ b/packages/core/src/tools/shell.session-commit-tracking.test.ts @@ -17,7 +17,8 @@ * `git commit` runs through `ShellToolInvocation.execute()` in a real temp * repo, then the guard is asked whether the follow-up amend is allowed. The * fake `ShellExecutionService` really runs commands via `spawnSync`, so - * `getGitHeadSync` / `getGitHead` see genuine HEADs, not stubbed SHAs. + * `getGitHeadBeforeCommand` / `getGitHead` see genuine HEADs, not stubbed + * SHAs. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -139,6 +140,16 @@ describe.skipIf(process.platform === 'win32')( return dir; } + /** An initialized repository with no commit yet (unborn HEAD). */ + function initUnbornRepo(): void { + fs.rmSync(repoDir, { recursive: true, force: true }); + fs.mkdirSync(repoDir, { recursive: true }); + rawGit('init -q --initial-branch=main', repoDir); + rawGit('config user.email agent@example.com'); + rawGit('config user.name Agent'); + rawGit('config commit.gpgsign false'); + } + /** A temp dir that is not a repository; removed in `afterEach`. */ function makeScratchDir(prefix: string): string { const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); @@ -672,6 +683,56 @@ describe.skipIf(process.platform === 'win32')( await vi.waitFor(() => expect(amendVerdict()).toBeNull()); }); + it('registers a promoted initial commit in a repository with an unborn HEAD (issue #12514-D)', async () => { + // An empty repository gives both a failed `rev-parse HEAD` and an + // unborn branch the same null SHA. The promoted path used to skip + // registration for both, so the next amend of the agent's root commit + // was falsely blocked. Capture must distinguish "verified repo with no + // HEAD" from "probe failed", and lineage must expect a null predecessor. + initUnbornRepo(); + fs.writeFileSync(path.join(repoDir, 'initial.txt'), 'initial\n'); + + await runPromoted( + 'git add initial.txt && git commit -m "initial feature"', + ); + + expect(lastLog('%s')).toBe('initial feature'); + expect(amendVerdict()).toBeNull(); + }); + + it('does not register an existing root commit when the pre-spawn HEAD probe is terminated', async () => { + initUnbornRepo(); + rawCommit('human.txt', 'human\n', 'human root'); + const humanSha = headSha(); + const gitPath = execSync('command -v git', { encoding: 'utf-8' }).trim(); + const probeBinDir = makeScratchDir('qwen-12514-probe-'); + const probeGitPath = path.join(probeBinDir, 'git'); + fs.writeFileSync( + probeGitPath, + '#!/bin/sh\n' + + 'if [ "$1" = rev-parse ] && [ "$2" = --verify ] && [ "$3" = HEAD ]; then\n' + + ' kill -TERM "$$"\n' + + 'fi\n' + + `exec ${JSON.stringify(gitPath)} "$@"\n`, + ); + fs.chmodSync(probeGitPath, 0o755); + vi.stubEnv( + 'PATH', + `${probeBinDir}${path.delimiter}${process.env['PATH']}`, + ); + // Load-bearing: this row's closing assertions also hold when the shim is + // bypassed (the lineage check then rejects on a non-null predecessor), so + // pin that the interception is live before relying on it. + expect(() => rawGit('rev-parse --verify HEAD')).toThrow(); + + await runPromoted('git commit -m "nothing staged"', true); + firePromoteSettle!(); + await new Promise((resolve) => setTimeout(resolve, 500)); + + expect(headSha()).toBe(humanSha); + expect(amendVerdict()?.blocked).toBe(true); + }); + it('does not adopt a commit somebody else landed while a promoted child ran (issue #12514)', async () => { // The promoted window is the child's whole lifetime, which #12523's // accepted foreground window does not cover: at settle the newest diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 1717bbbd2eb..11284d7ce90 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -120,6 +120,10 @@ const DEFAULT_SHELL_OUTPUT_THRESHOLD = 30_000; // append loop share it so the two cannot drift. const APPENDED_METADATA_SEPARATOR = '\n\n'; +type GitHeadBeforeCommand = + | { sha: string; unborn: false } + | { sha: null; unborn: true }; + export const SHELL_RECORD_ARTIFACT_REMINDER = 'If this command created or rewrote a deliverable file (csv, html, sql, py, md, json, notebook, image, pdf, or an office document), call record_artifact for each one. workspacePath is relative to the current execution directory, and title is required. Do not call it for files already recorded by write_file, edit, or notebook_edit.'; @@ -2560,22 +2564,22 @@ export class ShellToolInvocation extends BaseToolInvocation< // and so attribution still runs after a `git commit && cd ..` // chain (which would have failed an "any cd anywhere" gate). 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 - // `attachCommitAttribution` and `trackSessionCommit`). For - // non-attributable hasCommit cases (`cd /elsewhere && git - // commit`, `git -C /other commit`), no consumer reads preHead and - // the ~10–50 ms execFileSync is dead work that just blocks the - // event loop before the user's real command spawns. - // Registration must never run against a null preHead — - // `head.sha !== preHead` would be trivially true and exempt - // whatever HEAD happens to be there (the fail-open #12463's - // regression row ② pins). - const preHead: string | null = + // Capture HEAD only when the commit will actually be attributed in our + // cwd: that's the only consumer (the `attributableInCwd` branch below + // feeds `preHead` into `attachCommitAttribution` and passes the full + // state to promoted-path registration). For non-attributable hasCommit + // cases (`cd /elsewhere && git commit`, `git -C /other commit`), no + // consumer reads HEAD and the ~10–50 ms execFileSync is dead work that + // just blocks the event loop before the user's real command spawns. + // Registration must never run against a failed probe — + // `head.sha !== preHead` would be trivially true and exempt whatever + // HEAD happens to be there (the fail-open #12463's regression row ② + // pins). An unborn HEAD is represented separately from that failure. + const headBeforeCommand: GitHeadBeforeCommand | null = !this.config.getShellExecutionSandbox?.() && commitCtx.attributableInCwd - ? this.getGitHeadSync(cwd) + ? this.getGitHeadBeforeCommand(cwd) : null; + const preHead = headBeforeCommand?.sha ?? null; // Snapshot the attribution inputs BEFORE spawn so bindGhPrCreate can // tell a run that CREATED a PR from one that merely RESOLVED the @@ -3007,7 +3011,7 @@ export class ShellToolInvocation extends BaseToolInvocation< commandToExecute, promoteAbortController, promoteArtifacts, - preHead, + headBeforeCommand, ); return promotedToolResult; } @@ -3691,11 +3695,11 @@ export class ShellToolInvocation extends BaseToolInvocation< * pre-allocated buffer/stream slots that absorb the race between * service-side promote-time data flush and this finalizer running. * - * `preHead` is the HEAD captured synchronously before spawn (null - * unless the command carried a commit recognisable as landing in our - * cwd, unsandboxed). A promoted command settles after `execute()` - * has returned, so the foreground registration block never runs for - * it — registration happens here, at settle (#12514). + * `headBeforeCommand` is the HEAD state captured synchronously before + * spawn (null unless the command carried a commit recognisable as + * landing in our cwd, unsandboxed). A promoted command settles after + * `execute()` has returned, so the foreground registration block never + * runs for it — registration happens here, at settle (#12514). */ private async handlePromotedForeground( result: ShellExecutionResult, @@ -3703,7 +3707,7 @@ export class ShellToolInvocation extends BaseToolInvocation< commandToExecute: string, abortController: AbortController, promoteArtifacts: PromoteArtifacts, - preHead: string | null, + headBeforeCommand: GitHeadBeforeCommand | null, ): Promise { // Mirror executeBackground's outputPath layout so /tasks-on-disk and // ReadFileTool's auto-allow rules treat foreground-promoted shells @@ -4091,13 +4095,14 @@ export class ShellToolInvocation extends BaseToolInvocation< // Session-commit registration for the promoted path (#12514 A.3). // `execute()` returns the promote ToolResult without reaching the // foreground registration block, so a commit landed by a promoted - // command would never earn the amend exemption. `preHead` arrives - // from the caller's pre-spawn capture, gated there on - // `attributableInCwd` (and non-sandboxed), so a - // null here means the command carried no commit recognisable as - // landing in our cwd — `cd /elsewhere && git commit` stays - // unregistered exactly as the foreground path leaves it, and - // `head.sha !== preHead` can never be trivially true. + // command would never earn the amend exemption. The pre-spawn HEAD + // state arrives from the caller, gated there on + // `attributableInCwd` (and non-sandboxed), so a null here means the + // command carried no commit recognisable as landing in our cwd — + // `cd /elsewhere && git commit` stays unregistered exactly as the + // foreground path leaves it. An unborn HEAD is a distinct value: + // `rev-parse --git-dir` succeeded but `HEAD` had no commit yet, so a + // root commit from this command is still attributable (#12514-D). // // Registration is settle-only by design: the commit can land at any // point in the backgrounded child's life, and the only moment this @@ -4119,7 +4124,9 @@ export class ShellToolInvocation extends BaseToolInvocation< // registry. const registrationGeneration = getSessionCommitGeneration(); const registerPromotedCommit = (): void => { - if (preHead === null || promotedCommitRegistration !== null) return; + if (headBeforeCommand === null || promotedCommitRegistration !== null) { + return; + } if (getSessionCommitGeneration() !== registrationGeneration) { // Fail-closed: the registry this decision was made against no // longer exists, so the exemption it would grant is stale. The @@ -4135,7 +4142,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } promotedCommitRegistration = this.trackSessionCommit( cwd, - preHead, + headBeforeCommand.sha, // Lineage, not mere difference. This runs when the backgrounded // child exits — arbitrarily later than the caller's pre-spawn // capture — so a commit anybody else lands in this repository @@ -4143,9 +4150,11 @@ export class ShellToolInvocation extends BaseToolInvocation< // and `head.sha !== preHead` alone would adopt it: the foreign // commit would earn the amend exemption and the promoted one // would lose its own. Requiring that entry to have moved HEAD - // *from* `preHead` keeps the registered SHA one this command - // created (see `trackSessionCommit`'s `requirePredecessor`). - preHead, + // *from* `preHead` rejects the foreign one unless this command's + // own commit landed first; a foreign commit landed straight on + // `preHead` still passes — the #12523 residue disclosed on + // `trackSessionCommit`'s `requirePredecessor`. + headBeforeCommand.unborn ? null : headBeforeCommand.sha, ).catch((err: unknown) => { // Registration must never break the settle path — a failure // here costs the exemption (fail-closed), nothing more. @@ -4592,30 +4601,60 @@ export class ShellToolInvocation extends BaseToolInvocation< } /** - * Synchronous companion to {@link getGitHead}. Captured BEFORE the - * user's shell command spawns so a fast `git commit` (hot-cached, - * no hooks) cannot move HEAD before our async rev-parse has a chance - * to read it — a real race seen on slow filesystems / heavy contention - * where preHead would otherwise resolve to the new SHA, postHead would - * match, and `attachCommitAttribution` would silently skip writing the - * attribution note even though the commit succeeded. + * Synchronous pre-spawn HEAD probe. Captured BEFORE the user's shell + * command spawns so a fast `git commit` (hot-cached, no hooks) cannot + * move HEAD before our async rev-parse has a chance to read it — a real + * race seen on slow filesystems / heavy contention where preHead would + * otherwise resolve to the new SHA, postHead would match, and + * `attachCommitAttribution` would silently skip writing the attribution + * note even though the commit succeeded. + * + * A non-repository, missing git binary, or failed `--git-dir` probe + * returns `null`. A verified repository whose HEAD has no commit returns + * `{ sha: null, unborn: true }` instead: callers that need a SHA still + * see null, while session-commit registration can require an explicitly + * empty predecessor rather than conflating the unborn state with a + * failed probe. * * Worst case is ~10–50 ms of event-loop block per commit-shaped shell * command; acceptable trade for correctness of the post-command HEAD * comparison. */ - private getGitHeadSync(cwd: string): string | null { + private getGitHeadBeforeCommand(cwd: string): GitHeadBeforeCommand | null { try { - const stdout = childProcess.execFileSync('git', ['rev-parse', 'HEAD'], { + const stdout = childProcess.execFileSync( + 'git', + ['rev-parse', '--verify', 'HEAD'], + { + cwd, + timeout: 2000, + windowsHide: true, + stdio: ['ignore', 'pipe', 'ignore'], + }, + ); + const sha = String(stdout).trim(); + if (sha.length > 0) return { sha, unborn: false }; + return null; + } catch (error) { + if ( + !(error instanceof Error) || + !('status' in error) || + error.status !== 128 || + !('signal' in error) || + error.signal !== null + ) { + return null; + } + // A failed HEAD lookup is either an unborn branch or no repository. + } + try { + childProcess.execFileSync('git', ['rev-parse', '--git-dir'], { cwd, timeout: 2000, windowsHide: true, - // Discard stderr noise (e.g. "fatal: not a git repository") — - // the catch-or-empty-output path already covers failure. - stdio: ['ignore', 'pipe', 'ignore'], + stdio: ['ignore', 'ignore', 'ignore'], }); - const sha = String(stdout).trim(); - return sha.length > 0 ? sha : null; + return { sha: null, unborn: true }; } catch { return null; } @@ -4668,17 +4707,25 @@ export class ShellToolInvocation extends BaseToolInvocation< * lands in this repository meanwhile is not a narrow race but the * expected shape, and difference-from-`preHead` alone would adopt it. * Passing the captured `preHead` here requires the entry that created - * HEAD to have moved it *from* that SHA, which only this command's own - * commit does. Fail-closed both ways: a foreign commit never earns an - * exemption, and a multi-commit promoted chain (`commit a && commit b`, - * whose newest entry descends from `a`) loses one it had earned rather - * than guessing. The foreground call site passes nothing and keeps the - * disclosed window unchanged. + * HEAD to have moved it *from* that SHA. For an unborn repository the + * expected predecessor is explicitly `null`, which lets the initial + * commit register without treating a failed probe as a valid empty + * history. That sentinel is not provenance: every root commit satisfies + * it, so a root commit somebody else lands during the promoted window is + * indistinguishable from this command's own and registers too (#12523). + * A born HEAD narrows that window but does not close it: a foreign commit + * landed directly on `preHead` while this command's own commit never + * landed has the same predecessor and registers too, the same #12523 + * residue. Fail-closed once this command's own commit landed first: a + * foreign commit on top of it never earns an exemption, and a multi-commit + * promoted chain (`commit a && commit b`, whose newest entry descends + * from `a`) loses one it had earned rather than guessing. The foreground + * call site passes nothing and keeps the disclosed window unchanged. */ private async trackSessionCommit( cwd: string, preHead: string | null, - requirePredecessor?: string, + requirePredecessor?: string | null, ): Promise { const head = await this.getGitHeadOrigin(cwd); if ( @@ -4690,10 +4737,14 @@ export class ShellToolInvocation extends BaseToolInvocation< // only other output of this path is a block reason asserting the // commit was not the agent's, so say which side of the lineage // check failed. + const predecessorLabel = + requirePredecessor === null + ? '(unborn)' + : requirePredecessor.slice(0, 12); debugLogger.warn( `Session commit not registered in ${cwd}: the newest HEAD reflog entry descends from ` + `${head.predecessor ?? '(no predecessor)'}, not from the pre-spawn HEAD ` + - `${requirePredecessor.slice(0, 12)}, so it was not created by this command.`, + `${predecessorLabel}, so it was not created by this command.`, ); return; }