fix(coding-agent): handle reftable footer branch detection (#2300)
* fix(coding-agent): handle reftable footer branch detection - Fall back to git commands when HEAD contains refs/heads/.invalid - Treat unresolved .invalid as detached instead of showing literal .invalid - Watch reftable directory for branch change updates - Resolve commonGitDir for worktree setups fixes #2123 * fix(coding-agent): improve reftable branch detection and reduce false-positive rerenders - Re-resolve branch on watcher events instead of invalidating cache; only notify listeners when the branch string actually changed - Add regression tests: regular repo fast path (no spawnSync), nested cwd upward walk, unchanged-branch reftable churn suppression
This commit is contained in:
Aaron Soules
@@ -1,11 +1,18 @@
|
||||
import { spawnSync } from "child_process";
|
||||
import { existsSync, type FSWatcher, readFileSync, statSync, watch } from "fs";
|
||||
import { dirname, join, resolve } from "path";
|
||||
|
||||
type GitPaths = {
|
||||
repoDir: string;
|
||||
commonGitDir: string;
|
||||
headPath: string;
|
||||
};
|
||||
|
||||
/**
|
||||
* Find the git HEAD path by walking up from cwd.
|
||||
* Find git metadata paths by walking up from cwd.
|
||||
* Handles both regular git repos (.git is a directory) and worktrees (.git is a file).
|
||||
*/
|
||||
function findGitHeadPath(): string | null {
|
||||
function findGitPaths(): GitPaths | null {
|
||||
let dir = process.cwd();
|
||||
while (true) {
|
||||
const gitPath = join(dir, ".git");
|
||||
@@ -15,13 +22,19 @@ function findGitHeadPath(): string | null {
|
||||
if (stat.isFile()) {
|
||||
const content = readFileSync(gitPath, "utf8").trim();
|
||||
if (content.startsWith("gitdir: ")) {
|
||||
const gitDir = content.slice(8);
|
||||
const headPath = resolve(dir, gitDir, "HEAD");
|
||||
if (existsSync(headPath)) return headPath;
|
||||
const gitDir = resolve(dir, content.slice(8).trim());
|
||||
const headPath = join(gitDir, "HEAD");
|
||||
if (!existsSync(headPath)) return null;
|
||||
const commonDirPath = join(gitDir, "commondir");
|
||||
const commonGitDir = existsSync(commonDirPath)
|
||||
? resolve(gitDir, readFileSync(commonDirPath, "utf8").trim())
|
||||
: gitDir;
|
||||
return { repoDir: dir, commonGitDir, headPath };
|
||||
}
|
||||
} else if (stat.isDirectory()) {
|
||||
const headPath = join(gitPath, "HEAD");
|
||||
if (existsSync(headPath)) return headPath;
|
||||
if (!existsSync(headPath)) return null;
|
||||
return { repoDir: dir, commonGitDir: gitPath, headPath };
|
||||
}
|
||||
} catch {
|
||||
return null;
|
||||
@@ -33,6 +46,17 @@ function findGitHeadPath(): string | null {
|
||||
}
|
||||
}
|
||||
|
||||
/** Ask git for the current branch. Returns null on detached HEAD or if git is unavailable. */
|
||||
function resolveBranchWithGit(repoDir: string): string | null {
|
||||
const result = spawnSync("git", ["--no-optional-locks", "symbolic-ref", "--quiet", "--short", "HEAD"], {
|
||||
cwd: repoDir,
|
||||
encoding: "utf8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
});
|
||||
const branch = result.status === 0 ? result.stdout.trim() : "";
|
||||
return branch || null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Provides git branch and extension statuses - data not otherwise accessible to extensions.
|
||||
* Token stats, model info available via ctx.sessionManager and ctx.model.
|
||||
@@ -40,28 +64,21 @@ function findGitHeadPath(): string | null {
|
||||
export class FooterDataProvider {
|
||||
private extensionStatuses = new Map<string, string>();
|
||||
private cachedBranch: string | null | undefined = undefined;
|
||||
private gitWatcher: FSWatcher | null = null;
|
||||
private gitPaths: GitPaths | null | undefined = undefined;
|
||||
private headWatcher: FSWatcher | null = null;
|
||||
private reftableWatcher: FSWatcher | null = null;
|
||||
private branchChangeCallbacks = new Set<() => void>();
|
||||
private availableProviderCount = 0;
|
||||
|
||||
constructor() {
|
||||
this.gitPaths = findGitPaths();
|
||||
this.setupGitWatcher();
|
||||
}
|
||||
|
||||
/** Current git branch, null if not in repo, "detached" if detached HEAD */
|
||||
getGitBranch(): string | null {
|
||||
if (this.cachedBranch !== undefined) return this.cachedBranch;
|
||||
|
||||
try {
|
||||
const gitHeadPath = findGitHeadPath();
|
||||
if (!gitHeadPath) {
|
||||
this.cachedBranch = null;
|
||||
return null;
|
||||
}
|
||||
const content = readFileSync(gitHeadPath, "utf8").trim();
|
||||
this.cachedBranch = content.startsWith("ref: refs/heads/") ? content.slice(16) : "detached";
|
||||
} catch {
|
||||
this.cachedBranch = null;
|
||||
if (this.cachedBranch === undefined) {
|
||||
this.cachedBranch = this.resolveGitBranch();
|
||||
}
|
||||
return this.cachedBranch;
|
||||
}
|
||||
@@ -103,37 +120,73 @@ export class FooterDataProvider {
|
||||
|
||||
/** Internal: cleanup */
|
||||
dispose(): void {
|
||||
if (this.gitWatcher) {
|
||||
this.gitWatcher.close();
|
||||
this.gitWatcher = null;
|
||||
if (this.headWatcher) {
|
||||
this.headWatcher.close();
|
||||
this.headWatcher = null;
|
||||
}
|
||||
if (this.reftableWatcher) {
|
||||
this.reftableWatcher.close();
|
||||
this.reftableWatcher = null;
|
||||
}
|
||||
this.branchChangeCallbacks.clear();
|
||||
}
|
||||
|
||||
private setupGitWatcher(): void {
|
||||
if (this.gitWatcher) {
|
||||
this.gitWatcher.close();
|
||||
this.gitWatcher = null;
|
||||
}
|
||||
private notifyBranchChange(): void {
|
||||
for (const cb of this.branchChangeCallbacks) cb();
|
||||
}
|
||||
|
||||
const gitHeadPath = findGitHeadPath();
|
||||
if (!gitHeadPath) return;
|
||||
private refreshGitBranch(): void {
|
||||
const nextBranch = this.resolveGitBranch();
|
||||
if (this.cachedBranch !== undefined && this.cachedBranch !== nextBranch) {
|
||||
this.cachedBranch = nextBranch;
|
||||
this.notifyBranchChange();
|
||||
return;
|
||||
}
|
||||
this.cachedBranch = nextBranch;
|
||||
}
|
||||
|
||||
private resolveGitBranch(): string | null {
|
||||
try {
|
||||
if (!this.gitPaths) return null;
|
||||
const content = readFileSync(this.gitPaths.headPath, "utf8").trim();
|
||||
if (content.startsWith("ref: refs/heads/")) {
|
||||
const branch = content.slice(16);
|
||||
return branch === ".invalid" ? (resolveBranchWithGit(this.gitPaths.repoDir) ?? "detached") : branch;
|
||||
}
|
||||
return "detached";
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
private setupGitWatcher(): void {
|
||||
if (!this.gitPaths) return;
|
||||
|
||||
// Watch the directory containing HEAD, not HEAD itself.
|
||||
// Git uses atomic writes (write temp, rename over HEAD), which changes the inode.
|
||||
// fs.watch on a file stops working after the inode changes.
|
||||
const gitDir = dirname(gitHeadPath);
|
||||
|
||||
try {
|
||||
this.gitWatcher = watch(gitDir, (_eventType, filename) => {
|
||||
if (filename === "HEAD") {
|
||||
this.cachedBranch = undefined;
|
||||
for (const cb of this.branchChangeCallbacks) cb();
|
||||
this.headWatcher = watch(dirname(this.gitPaths.headPath), (_eventType, filename) => {
|
||||
if (!filename || filename.toString() === "HEAD") {
|
||||
this.refreshGitBranch();
|
||||
}
|
||||
});
|
||||
} catch {
|
||||
// Silently fail if we can't watch
|
||||
}
|
||||
|
||||
// In reftable repos, branch switches update files in the reftable directory
|
||||
// instead of HEAD. Watch it separately so the footer picks up those changes.
|
||||
const reftableDir = join(this.gitPaths.commonGitDir, "reftable");
|
||||
if (existsSync(reftableDir)) {
|
||||
try {
|
||||
this.reftableWatcher = watch(reftableDir, () => {
|
||||
this.refreshGitBranch();
|
||||
});
|
||||
} catch {
|
||||
// Silently fail if we can't watch
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
191
packages/coding-agent/test/footer-data-provider.test.ts
Normal file
191
packages/coding-agent/test/footer-data-provider.test.ts
Normal file
@@ -0,0 +1,191 @@
|
||||
import { spawnSync } from "child_process";
|
||||
import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs";
|
||||
import { tmpdir } from "os";
|
||||
import { join } from "path";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
let resolvedBranch = "main";
|
||||
|
||||
vi.mock("child_process", () => ({
|
||||
spawnSync: vi.fn((_command: string, args: readonly string[]) => {
|
||||
if (args[1] === "symbolic-ref") {
|
||||
return { status: resolvedBranch ? 0 : 1, stdout: resolvedBranch ? `${resolvedBranch}\n` : "", stderr: "" };
|
||||
}
|
||||
return { status: 1, stdout: "", stderr: "" };
|
||||
}),
|
||||
}));
|
||||
|
||||
import { FooterDataProvider } from "../src/core/footer-data-provider.js";
|
||||
|
||||
type WorktreeFixture = {
|
||||
worktreeDir: string;
|
||||
reftableDir: string;
|
||||
};
|
||||
|
||||
function createPlainReftableRepo(tempDir: string): string {
|
||||
const repoDir = join(tempDir, "repo");
|
||||
mkdirSync(join(repoDir, ".git", "reftable"), { recursive: true });
|
||||
writeFileSync(join(repoDir, ".git", "HEAD"), "ref: refs/heads/.invalid\n");
|
||||
return repoDir;
|
||||
}
|
||||
|
||||
function createPlainRepo(tempDir: string): string {
|
||||
const repoDir = join(tempDir, "repo");
|
||||
mkdirSync(join(repoDir, ".git"), { recursive: true });
|
||||
writeFileSync(join(repoDir, ".git", "HEAD"), "ref: refs/heads/main\n");
|
||||
return repoDir;
|
||||
}
|
||||
|
||||
function createReftableWorktree(tempDir: string): WorktreeFixture {
|
||||
const repoDir = join(tempDir, "repo");
|
||||
const commonGitDir = join(repoDir, ".git");
|
||||
const gitDir = join(commonGitDir, "worktrees", "src");
|
||||
const worktreeDir = join(tempDir, "worktree");
|
||||
const reftableDir = join(commonGitDir, "reftable");
|
||||
|
||||
mkdirSync(gitDir, { recursive: true });
|
||||
mkdirSync(reftableDir, { recursive: true });
|
||||
mkdirSync(worktreeDir, { recursive: true });
|
||||
|
||||
writeFileSync(join(worktreeDir, ".git"), `gitdir: ${gitDir}\n`);
|
||||
writeFileSync(join(gitDir, "HEAD"), "ref: refs/heads/.invalid\n");
|
||||
writeFileSync(join(gitDir, "commondir"), "../..\n");
|
||||
writeFileSync(join(reftableDir, "tables.list"), "0\n");
|
||||
|
||||
return { worktreeDir, reftableDir };
|
||||
}
|
||||
|
||||
async function waitFor(condition: () => boolean, timeoutMs = 2000): Promise<void> {
|
||||
const startedAt = Date.now();
|
||||
while (!condition()) {
|
||||
if (Date.now() - startedAt > timeoutMs) {
|
||||
throw new Error("Timed out waiting for condition");
|
||||
}
|
||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||
}
|
||||
}
|
||||
|
||||
describe("FooterDataProvider reftable branch detection", () => {
|
||||
let originalCwd: string;
|
||||
let tempDir: string;
|
||||
|
||||
beforeEach(() => {
|
||||
originalCwd = process.cwd();
|
||||
tempDir = mkdtempSync(join(tmpdir(), "footer-data-provider-"));
|
||||
resolvedBranch = "main";
|
||||
vi.mocked(spawnSync).mockClear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
process.chdir(originalCwd);
|
||||
if (tempDir && existsSync(tempDir)) {
|
||||
rmSync(tempDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("uses HEAD directly in a regular repo from a nested directory", () => {
|
||||
const repoDir = createPlainRepo(tempDir);
|
||||
const nestedDir = join(repoDir, "src", "nested");
|
||||
mkdirSync(nestedDir, { recursive: true });
|
||||
process.chdir(nestedDir);
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
expect(vi.mocked(spawnSync)).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
|
||||
it("resolves the branch via git when HEAD is .invalid in a reftable repo", () => {
|
||||
const repoDir = createPlainReftableRepo(tempDir);
|
||||
process.chdir(repoDir);
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
expect(vi.mocked(spawnSync)).toHaveBeenCalledWith(
|
||||
"git",
|
||||
["--no-optional-locks", "symbolic-ref", "--quiet", "--short", "HEAD"],
|
||||
expect.objectContaining({
|
||||
cwd: expect.stringMatching(/repo$/),
|
||||
encoding: "utf8",
|
||||
stdio: ["ignore", "pipe", "ignore"],
|
||||
}),
|
||||
);
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
|
||||
it("resolves the branch via git in a reftable-backed worktree", () => {
|
||||
const { worktreeDir } = createReftableWorktree(tempDir);
|
||||
process.chdir(worktreeDir);
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
|
||||
it("treats an unresolved .invalid reftable HEAD as detached", () => {
|
||||
const repoDir = createPlainReftableRepo(tempDir);
|
||||
process.chdir(repoDir);
|
||||
resolvedBranch = "";
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("detached");
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
|
||||
it("does not notify listeners when reftable updates keep the same branch", async () => {
|
||||
const { worktreeDir, reftableDir } = createReftableWorktree(tempDir);
|
||||
process.chdir(worktreeDir);
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
vi.mocked(spawnSync).mockClear();
|
||||
const onBranchChange = vi.fn();
|
||||
provider.onBranchChange(onBranchChange);
|
||||
|
||||
writeFileSync(join(reftableDir, "tables.list"), "1\n");
|
||||
await waitFor(() => vi.mocked(spawnSync).mock.calls.length > 0);
|
||||
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
expect(onBranchChange).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
|
||||
it("updates the cached branch when the reftable directory changes", async () => {
|
||||
const { worktreeDir, reftableDir } = createReftableWorktree(tempDir);
|
||||
process.chdir(worktreeDir);
|
||||
|
||||
const provider = new FooterDataProvider();
|
||||
try {
|
||||
expect(provider.getGitBranch()).toBe("main");
|
||||
resolvedBranch = "foo";
|
||||
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
const timeout = setTimeout(() => reject(new Error("Timed out waiting for branch change")), 2000);
|
||||
provider.onBranchChange(() => {
|
||||
clearTimeout(timeout);
|
||||
resolve();
|
||||
});
|
||||
writeFileSync(join(reftableDir, "tables.list"), "1\n");
|
||||
});
|
||||
|
||||
expect(provider.getGitBranch()).toBe("foo");
|
||||
} finally {
|
||||
provider.dispose();
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user