diff --git a/.changeset/fix-git-context-silent-failure.md b/.changeset/fix-git-context-silent-failure.md new file mode 100644 index 0000000000..416fe9763f --- /dev/null +++ b/.changeset/fix-git-context-silent-failure.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix explore subagents silently losing git context when git commands time out or the directory is not a repository. diff --git a/packages/agent-core/src/session/git-context.ts b/packages/agent-core/src/session/git-context.ts index ada2b1ad2a..7ba1c968bc 100644 --- a/packages/agent-core/src/session/git-context.ts +++ b/packages/agent-core/src/session/git-context.ts @@ -3,15 +3,22 @@ * * `collectGitContext` produces a `` block that is prepended to a * fresh explore subagent's prompt so it can orient itself in the repository - * before searching. Every git command is individually guarded — a single - * failure never aborts the whole collection — and remote URLs are sanitized - * so internal infrastructure is not surfaced to the model. + * before searching. Every git probe is best-effort: probes fail in perfectly + * normal states (no `origin` remote, no commits yet, detached HEAD, older + * Git), so a failed probe is logged and its section omitted rather than + * dropping the whole block. The block is omitted entirely only when nothing + * useful was collected. The one explicit state surfaced to the subagent is + * `reason="not-a-repo"`, so it doesn't waste turns probing git history in a + * non-repo directory. Remote URLs are sanitized so internal infrastructure + * is not surfaced to the model. */ import type { Readable } from 'node:stream'; import type { Kaos, KaosProcess } from '@moonshot-ai/kaos'; +import { log } from '../logging/logger'; + const GIT_TIMEOUT_MS = 5_000; const MAX_DIRTY_FILES = 20; const MAX_COMMIT_LINE_LENGTH = 200; @@ -42,17 +49,50 @@ async function disposeProcess(proc: KaosProcess): Promise { * directory is not a git repository or no useful information was collected. */ export async function collectGitContext(kaos: Kaos, cwd: string): Promise { - // Quick check: is this a git repo? - if ((await runGit(kaos, cwd, ['rev-parse', '--is-inside-work-tree'])) === null) { + // Step 1: is this a git repo? `rev-parse` is the authoritative probe — it + // handles `.git` files (worktrees/submodules), subdirectories, bare repos, + // and `$GIT_DIR` redirection, none of which a plain FS check covers. + const revParseArgs = ['rev-parse', '--is-inside-work-tree'] as const; + const revParse = await runGit(kaos, cwd, revParseArgs); + if (!revParse.ok) { + if (revParse.kind === 'command-failed' && isNotARepo(revParse.stderr)) { + // Definitive "not a repo" — tell the subagent so it doesn't waste turns + // probing git history. All other failures are logged but surface as an + // empty block (the subagent works without git context, same as before): + // a transient `git status` hang shouldn't read as "git is broken". + return ``; + } + logGitFailure(cwd, revParseArgs, revParse); return ''; } - const [remoteUrl, branch, dirtyRaw, logRaw] = await Promise.all([ - runGit(kaos, cwd, ['remote', 'get-url', 'origin']), - runGit(kaos, cwd, ['branch', '--show-current']), - runGit(kaos, cwd, ['status', '--porcelain']), - runGit(kaos, cwd, ['log', '-3', '--format=%h %s']), - ]); + // Step 2: collect context in parallel. Every probe is optional — git + // probes fail in perfectly normal states (no `origin` remote, no commits + // yet, detached HEAD, older Git), so a failed probe never aborts the + // collection. Each failure is logged and its section is simply omitted; if + // nothing useful is collected, the block is dropped entirely below. + // + // Branch is read via `symbolic-ref --short HEAD`, which works in unborn + // repositories and on older Git; it fails in detached-HEAD state, in which + // case the Branch section is just omitted. + const commandArgs = [ + ['remote', 'get-url', 'origin'], + ['symbolic-ref', '--short', 'HEAD'], + ['status', '--porcelain'], + ['log', '-3', '--format=%h %s'], + ] as const; + const [remote, branch, status, gitLog] = (await Promise.all( + commandArgs.map(async (args) => ({ args, result: await runGit(kaos, cwd, args) })), + )) as unknown as [TaggedGitResult, TaggedGitResult, TaggedGitResult, TaggedGitResult]; + + for (const { args, result } of [remote, branch, status, gitLog]) { + if (!result.ok) logGitFailure(cwd, args, result); + } + + const remoteUrl = stdoutOf(remote.result); + const branchName = stdoutOf(branch.result); + const dirtyRaw = stdoutOf(status.result); + const logRaw = stdoutOf(gitLog.result); const sections: string[] = [`Working directory: ${cwd}`]; @@ -67,19 +107,17 @@ export async function collectGitContext(kaos: Kaos, cwd: string): Promise line.trim().length > 0); - if (dirtyLines.length > 0) { - const total = dirtyLines.length; - const shown = dirtyLines.slice(0, MAX_DIRTY_FILES); - let body = shown.map((line) => ` ${line}`).join('\n'); - if (total > MAX_DIRTY_FILES) { - body += `\n ... and ${String(total - MAX_DIRTY_FILES)} more`; - } - sections.push(`Dirty files (${String(total)}):\n${body}`); + const dirtyLines = dirtyRaw.split('\n').filter((line) => line.trim().length > 0); + if (dirtyLines.length > 0) { + const total = dirtyLines.length; + const shown = dirtyLines.slice(0, MAX_DIRTY_FILES); + let body = shown.map((line) => ` ${line}`).join('\n'); + if (total > MAX_DIRTY_FILES) { + body += `\n ... and ${String(total - MAX_DIRTY_FILES)} more`; } + sections.push(`Dirty files (${String(total)}):\n${body}`); } if (logRaw) { @@ -135,7 +173,10 @@ export function parseProjectName(remoteUrl: string): string | null { const scp = /^[^/]+@[^/:]+:(.+)$/.exec(remoteUrl); const rawPath = scp?.[1] ?? tryUrlPath(remoteUrl); if (rawPath === null) return null; - const project = rawPath.replace(/^\/+/, '').replace(/\/+$/, '').replace(/\.git$/, ''); + const project = rawPath + .replace(/^\/+/, '') + .replace(/\/+$/, '') + .replace(/\.git$/, ''); return project.length > 0 ? project : null; } @@ -148,16 +189,61 @@ function tryUrlPath(remoteUrl: string): string | null { } /** - * Run a single `git -C ` command and return its trimmed stdout, - * or `null` on any failure (spawn error, non-zero exit, or timeout). The - * `git -C` form runs in the target directory regardless of the Kaos backend. + * Outcome of a single `git` invocation. + * + * - `ok: true` — exited 0; `stdout` is trimmed. + * - `timeout` — exceeded `GIT_TIMEOUT_MS`; process was SIGKILLed. + * - `spawn-error` — `kaos.exec` itself rejected (git missing / backend error). + * - `command-failed` — git ran but exited non-zero, or its streams errored. + * `exitCode`/`stderr` are populated for the non-zero-exit case. + */ +type GitFailure = + | { readonly kind: 'timeout' } + | { readonly kind: 'spawn-error' } + | { readonly kind: 'command-failed'; readonly exitCode?: number; readonly stderr?: string }; + +type GitResult = + | { readonly ok: true; readonly stdout: string } + | ({ readonly ok: false } & GitFailure); + +type TaggedGitResult = { readonly args: readonly string[]; readonly result: GitResult }; + +function stdoutOf(result: GitResult): string { + return result.ok ? result.stdout : ''; +} + +function isNotARepo(stderr: string | undefined): boolean { + return stderr !== undefined && stderr.includes('not a git repository'); +} + +function logGitFailure(cwd: string, args: readonly string[], failure: GitFailure): void { + const command = `git ${args.join(' ')}`; + if (failure.kind === 'timeout') { + log.debug('git context command timed out', { cwd, command }); + } else if (failure.kind === 'spawn-error') { + log.warn('git context command failed to spawn', { cwd, command }); + } else { + log.debug('git context command failed', { + cwd, + command, + exitCode: failure.exitCode, + stderr: failure.stderr, + }); + } +} + +/** + * Run a single `git -C ` command and return a structured result. + * The `git -C` form runs in the target directory regardless of the Kaos + * backend. Both stdout and stderr are captured so callers can tell "not a + * git repository" (exit 128 + telltale stderr) apart from other failures. */ -async function runGit(kaos: Kaos, cwd: string, args: readonly string[]): Promise { +async function runGit(kaos: Kaos, cwd: string, args: readonly string[]): Promise { let proc: KaosProcess | undefined; try { proc = await kaos.exec('git', '-C', cwd, ...args); } catch { - return null; + return { ok: false, kind: 'spawn-error' }; } try { @@ -166,31 +252,36 @@ async function runGit(kaos: Kaos, cwd: string, args: readonly string[]): Promise /* stdin already closed */ } - const work = Promise.all([collectStream(proc.stdout), proc.wait()]); + const work = Promise.all([collectStream(proc.stdout), collectStream(proc.stderr), proc.wait()]); // Attach a rejection handler up front: if `work` rejects during the // timeout-handling window (before the catch block re-awaits it), Node must // not flag it as an unhandled rejection. work.catch(() => {}); let timer: ReturnType | undefined; + let timedOut = false; try { const timeout = new Promise((_resolve, reject) => { timer = setTimeout(() => { + timedOut = true; reject(new Error(`git ${args.join(' ')} timed out`)); }, GIT_TIMEOUT_MS); }); - const [stdout, exitCode] = await Promise.race([work, timeout]); - if (exitCode !== 0) return null; - return stdout.trim(); + const [stdout, stderr, exitCode] = await Promise.race([work, timeout]); + if (exitCode !== 0) { + return { ok: false, kind: 'command-failed', exitCode, stderr: stderr.trim() }; + } + return { ok: true, stdout: stdout.trim() }; } catch { try { await proc.kill('SIGKILL'); } catch { /* process already gone */ } - // Let the stdout drain settle so the process resources are released, - // even though the timed-out output is discarded. + // Let the streams drain so process resources are released, even though + // the timed-out/errored output is discarded. await work.catch(() => {}); - return null; + if (timedOut) return { ok: false, kind: 'timeout' }; + return { ok: false, kind: 'command-failed' }; } finally { if (timer !== undefined) clearTimeout(timer); if (proc !== undefined) await disposeProcess(proc); diff --git a/packages/agent-core/test/session/git-context.test.ts b/packages/agent-core/test/session/git-context.test.ts index 3bf9c23442..cb135daa88 100644 --- a/packages/agent-core/test/session/git-context.test.ts +++ b/packages/agent-core/test/session/git-context.test.ts @@ -3,14 +3,18 @@ import { Readable } from 'node:stream'; import type { Kaos, KaosProcess } from '@moonshot-ai/kaos'; import { describe, expect, it, vi } from 'vitest'; -import { collectGitContext, parseProjectName, sanitizeRemoteUrl } from '../../src/session/git-context'; +import { + collectGitContext, + parseProjectName, + sanitizeRemoteUrl, +} from '../../src/session/git-context'; import { createFakeKaos } from '../tools/fixtures/fake-kaos'; -function fakeProcess(stdout: string, exitCode = 0): KaosProcess { +function fakeProcess(stdout: string, exitCode = 0, stderr = ''): KaosProcess { return { stdin: { write: () => true, end: () => {} } as never, stdout: Readable.from([stdout]), - stderr: Readable.from(['']), + stderr: Readable.from([stderr]), pid: 1, exitCode, wait: async () => exitCode, @@ -20,22 +24,50 @@ function fakeProcess(stdout: string, exitCode = 0): KaosProcess { } /** Scripted git output keyed by the git subcommand (`args[3]`). */ -type GitScript = Record; +type GitScript = Record; function gitKaos(script: GitScript): Kaos { return createFakeKaos({ exec: async (...args: string[]): Promise => { const subcommand = args[3] ?? ''; - const scripted = script[subcommand]; + // Match the full git invocation first (e.g. `rev-parse --abbrev-ref + // HEAD`) so two commands sharing a subcommand (both `rev-parse`) can be + // scripted distinctly; fall back to the bare subcommand. + const full = args.slice(3).join(' '); + const scripted = script[full] ?? script[subcommand]; if (scripted === undefined) return fakeProcess('', 1); - return fakeProcess(scripted.stdout, scripted.exitCode ?? 0); + return fakeProcess(scripted.stdout, scripted.exitCode ?? 0, scripted.stderr ?? ''); }, }); } describe('collectGitContext', () => { - it('returns an empty string when the directory is not a git repository', async () => { - const kaos = gitKaos({ 'rev-parse': { stdout: '', exitCode: 1 } }); + it('returns an unavailable block when the directory is not a git repository', async () => { + const kaos = gitKaos({ + 'rev-parse': { + stdout: '', + exitCode: 128, + stderr: 'fatal: not a git repository (or any of the parent directories): .git', + }, + }); + expect(await collectGitContext(kaos, '/project')).toBe( + ``, + ); + }); + + it('returns an empty string when rev-parse fails for a reason other than not-a-repo', async () => { + const kaos = gitKaos({ + 'rev-parse': { stdout: '', exitCode: 1, stderr: 'fatal: some other git error' }, + }); + expect(await collectGitContext(kaos, '/project')).toBe(''); + }); + + it('returns an empty string when git fails to spawn', async () => { + const kaos = createFakeKaos({ + exec: async (): Promise => { + throw new Error('spawn failed'); + }, + }); expect(await collectGitContext(kaos, '/project')).toBe(''); }); @@ -43,7 +75,7 @@ describe('collectGitContext', () => { const kaos = gitKaos({ 'rev-parse': { stdout: 'true' }, remote: { stdout: 'https://github.com/acme/widgets.git' }, - branch: { stdout: 'main' }, + 'symbolic-ref --short HEAD': { stdout: 'main' }, status: { stdout: ' M src/a.ts\n?? src/b.ts' }, log: { stdout: 'abc123 first commit\ndef456 second commit' }, }); @@ -66,7 +98,10 @@ describe('collectGitContext', () => { const dirty = Array.from({ length: 25 }, (_, i) => ` M src/f${String(i)}.ts`).join('\n'); const kaos = gitKaos({ 'rev-parse': { stdout: 'true' }, + remote: { stdout: '' }, + 'symbolic-ref --short HEAD': { stdout: '' }, status: { stdout: dirty }, + log: { stdout: '' }, }); const block = await collectGitContext(kaos, '/project'); @@ -84,7 +119,9 @@ describe('collectGitContext', () => { const kaos = gitKaos({ 'rev-parse': { stdout: 'true' }, remote: { stdout: 'git@internal.corp:secret/repo.git' }, - branch: { stdout: 'main' }, + 'symbolic-ref --short HEAD': { stdout: 'main' }, + status: { stdout: '' }, + log: { stdout: '' }, }); const block = await collectGitContext(kaos, '/project'); @@ -95,6 +132,65 @@ describe('collectGitContext', () => { expect(block).toContain('Branch: main'); }); + it('keeps branch and status when the origin remote is absent', async () => { + const kaos = gitKaos({ + 'rev-parse': { stdout: 'true' }, + remote: { stdout: '', exitCode: 2, stderr: "error: No such remote 'origin'" }, + 'symbolic-ref --short HEAD': { stdout: 'main' }, + status: { stdout: ' M src/a.ts' }, + log: { stdout: 'abc123 first commit' }, + }); + + const block = await collectGitContext(kaos, '/project'); + + expect(block).toContain('Branch: main'); + expect(block).toContain('Dirty files (1):'); + expect(block).toContain('Recent commits:'); + expect(block).not.toContain('Remote:'); + expect(block).not.toContain('Project:'); + }); + + it('keeps branch and status when the repository has no commits yet', async () => { + const kaos = gitKaos({ + 'rev-parse': { stdout: 'true' }, + remote: { stdout: 'https://github.com/acme/widgets.git' }, + 'symbolic-ref --short HEAD': { stdout: 'main' }, + status: { stdout: '' }, + log: { + stdout: '', + exitCode: 128, + stderr: "fatal: your current branch 'main' does not have any commits yet", + }, + }); + + const block = await collectGitContext(kaos, '/project'); + + expect(block).toContain('Branch: main'); + expect(block).toContain('Remote: https://github.com/acme/widgets.git'); + expect(block).toContain('Project: acme/widgets'); + expect(block).not.toContain('Recent commits:'); + }); + + it('omits the Branch section in detached HEAD state', async () => { + const kaos = gitKaos({ + 'rev-parse': { stdout: 'true' }, + 'symbolic-ref --short HEAD': { + stdout: '', + exitCode: 128, + stderr: 'fatal: ref HEAD is not a symbolic ref', + }, + remote: { stdout: 'https://github.com/acme/widgets.git' }, + status: { stdout: '' }, + log: { stdout: 'abc123 first commit' }, + }); + + const block = await collectGitContext(kaos, '/project'); + + expect(block).not.toContain('Branch:'); + expect(block).toContain('Remote: https://github.com/acme/widgets.git'); + expect(block).toContain('Recent commits:'); + }); + it('treats a hanging git command as a failure (timeout)', async () => { vi.useFakeTimers(); try {