From 7f55b0eb3a3e55b1a821c3ffb45203d50ef8818e Mon Sep 17 00:00:00 2001 From: dbarr5 Date: Wed, 19 Aug 2026 06:27:26 -0400 Subject: [PATCH] fix(repo): validate and fetch a --repo mirror instead of reusing it blind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane SC-A2, slice 1 of the project-continuity work. `aether agent --repo owner/name` reused an existing local mirror on the strength of one `existsSync` check: if (existsSync(join(dir, ".git"))) return { dir, cloned: false }; There is no `git fetch` anywhere in this codebase, so that mirror was never refreshed. `createWorktree` then branched off it with no start-point argument, meaning a task could silently begin from a tip that was current days ago, and `prCreateHint` would go on to invite a PR from it. The mirror path is derived from the slug alone (~/.aether-agent/repos/-), so the remote was never validated either: any directory sitting at that path was accepted as the requested repo. Adds `refreshMirror`, called by `ensureLocalClone` before an existing mirror is returned. It guarantees three things: 1. the mirror's origin really is the repo that was asked for — compared through parseRepoSpec so https/ssh/.git/trailing-slash forms normalize rather than needing a second, subtly different URL parser 2. the mirror is fetched, and the resulting tip is reported 3. when the fetch cannot happen — offline, auth expired, remote gone — the result is "unknown" with the git error attached. It never degrades to "fresh" as a convenience Read-only with respect to the user's tree: it runs remote get-url, fetch and rev-parse. Never checkout, reset, clean, merge, pull or rebase. A test asserts each of those six verbs is absent from the recorded argv. Auth stays the user's own git/gh configuration, inherited from the environment. A test asserts no Aether credential shape (aek_, Authorization, http.extraheader, GIT_ASKPASS, x-access-token) reaches the git argv. `ensureLocalClone` now takes an injected Runner, defaulting to the existing `defaultRunner()` from worktree.ts. It previously called spawnSync directly and was therefore untestable; this is the same seam the gated-worktree flow already uses, so no second runner abstraction is introduced. The user-facing line stops rounding off. "(reusing local clone)" was equally true of a mirror last fetched a week ago: ⎇ repo octocat/hello-world (fetched) @ a1b2c3d ⎇ repo octocat/hello-world (NOT REFRESHED — Could not resolve host: github.com) ! this worktree will branch off whatever the mirror already had; its base is not known to match the remote. Written test-first: the six tests were added and confirmed failing to compile against the missing export before `refreshMirror` existed. Mutation-checked: rewriting every `state: "unknown"` to `state: "fresh"` fails "a failed fetch reports unknown, never fresh" (11 pass / 1 fail); restoring gives 12 / 12. Gates at this commit: npm run typecheck exit 0 npm test 928 pass / 0 fail (922 on clean 41a7e261) Noted, not fixed here: test/diagnostics.test.ts "a hanging backend cannot stall the fast report" failed once under full-suite load at 1818ms and passes in isolation at 82ms. Its 50ms budget is load-sensitive. Pre-existing fragility, unrelated to this change, left for the lane that owns that file. --- src/commands/code.ts | 19 +++++-- src/core/repo.ts | 118 +++++++++++++++++++++++++++++++++++++++++-- test/repo.test.ts | 97 ++++++++++++++++++++++++++++++++++- 3 files changed, 227 insertions(+), 7 deletions(-) diff --git a/src/commands/code.ts b/src/commands/code.ts index 11382d7..70b8a24 100644 --- a/src/commands/code.ts +++ b/src/commands/code.ts @@ -143,9 +143,22 @@ export async function cmdCode(ctx: AppContext, task: string, opts: CodeOpts): Pr repoSpec = parseRepoSpec(opts.repo); const co = ensureLocalClone(repoSpec); repoRoot = co.dir; - process.stderr.write( - `⎇ repo ${repoSpec.full} ${co.cloned ? "(cloned)" : "(reusing local clone)"}\n ${co.dir}\n`, - ); + // Say what actually happened to the mirror. "reusing local clone" was + // equally true of a mirror last fetched a week ago, which is exactly the + // case a user needs told rather than hidden behind a reassuring word. + const tip = co.freshness.remoteTip ? ` @ ${co.freshness.remoteTip.slice(0, 7)}` : ""; + const how = co.cloned + ? "(cloned)" + : co.freshness.state === "fresh" + ? "(fetched)" + : `(NOT REFRESHED — ${co.freshness.reason ?? "reason unknown"})`; + process.stderr.write(`⎇ repo ${repoSpec.full} ${how}${tip}\n ${co.dir}\n`); + if (co.freshness.state !== "fresh") { + process.stderr.write( + " ! this worktree will branch off whatever the mirror already had;\n" + + " its base is not known to match the remote.\n", + ); + } } catch (err) { process.stderr.write(`✗ ${err instanceof Error ? err.message : String(err)}\n`); return 1; diff --git a/src/core/repo.ts b/src/core/repo.ts index bef0428..4376117 100644 --- a/src/core/repo.ts +++ b/src/core/repo.ts @@ -13,6 +13,7 @@ import { spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; +import { defaultRunner, type Runner } from "./worktree.js"; export interface RepoSpec { owner: string; @@ -60,6 +61,8 @@ export function cloneArgs(spec: RepoSpec, dir: string, useGh: boolean): { cmd: s } export interface RepoCheckout { + /** How current the mirror is. Never assumed — always measured or reported unknown. */ + freshness: MirrorFreshness; /** Local git dir for the repo (the mirror). */ dir: string; /** True when this call performed the clone (vs reusing an existing mirror). */ @@ -71,9 +74,15 @@ export interface RepoCheckout { * its dir. Reuses an existing mirror. Throws with an actionable message on a * clone failure (private repo + no auth is the common case). */ -export function ensureLocalClone(spec: RepoSpec): RepoCheckout { +export function ensureLocalClone(spec: RepoSpec, run: Runner = defaultRunner()): RepoCheckout { const dir = localMirrorDir(spec); - if (existsSync(join(dir, ".git"))) return { dir, cloned: false }; + // A mirror that already exists is validated and fetched before anything + // branches off it. Reusing it on the strength of its path alone is how a + // task silently starts from a days-old tip. + if (existsSync(join(dir, ".git"))) { + const { freshness } = refreshMirror(spec, dir, run, { exists: true }); + return { dir, cloned: false, freshness }; + } const useGh = ghAvailable(); const { cmd, args } = cloneArgs(spec, dir, useGh); const r = spawnSync(cmd, args, { encoding: "utf8" }); @@ -84,7 +93,110 @@ export function ensureLocalClone(spec: RepoSpec): RepoCheckout { : "gh CLI not found — install it or set up git credentials for github.com"; throw new Error(`could not clone ${spec.full}: ${why}\n ${hint}`); } - return { dir, cloned: true }; + // A clone just came from the remote, so its tip is the remote tip by + // construction. Read it back rather than asserting it. + const tip = run("git", ["-C", dir, "rev-parse", "HEAD"]); + return { + dir, + cloned: true, + freshness: { + state: "fresh", + remoteTip: tip.status === 0 ? tip.stdout.trim() || null : null, + checkedAt: new Date().toISOString(), + }, + }; +} + + +/** How current a local mirror is, relative to its GitHub remote. */ +export type MirrorFreshnessState = "fresh" | "stale" | "unknown"; + +export interface MirrorFreshness { + state: MirrorFreshnessState; + /** Commit the remote default branch resolved to, when the fetch succeeded. */ + remoteTip: string | null; + checkedAt: string; + /** Why the state is not "fresh". Present whenever it is not. */ + reason?: string; +} + +export interface MirrorResult { + dir: string; + freshness: MirrorFreshness; +} + +/** + * Validate and refresh an existing mirror before anything branches off it. + * + * Three properties this function exists to guarantee: + * + * 1. The directory really is the repo that was asked for. The mirror path is + * derived from the slug alone, so any directory sitting at that path would + * otherwise be accepted as "octocat/hello-world" on the strength of its name. + * 2. The mirror is fetched, so a task worktree does not branch off a tip that + * was current days ago. + * 3. When step 2 cannot happen — offline, auth expired, remote gone — the + * result says so. It never degrades to "fresh" as a convenience. + * + * Read-only with respect to the user's working tree: it fetches into the object + * store and reads refs. It never checks out, resets, merges, pulls or cleans. + * + * Auth is the user's own git/gh configuration, inherited from the environment. + * No Aether credential is passed, and none is available to this function. + */ +export function refreshMirror( + spec: RepoSpec, + dir: string, + run: Runner, + options: { exists: boolean; now?: string }, +): MirrorResult { + const checkedAt = options.now ?? new Date().toISOString(); + if (!options.exists) { + return { dir, freshness: { state: "unknown", remoteTip: null, checkedAt, reason: "no local mirror yet" } }; + } + + const remote = run("git", ["-C", dir, "remote", "get-url", "origin"]); + if (remote.status !== 0) { + return { + dir, + freshness: { + state: "unknown", + remoteTip: null, + checkedAt, + reason: remote.stderr.trim() || "could not read the mirror's origin remote", + }, + }; + } + // parseRepoSpec already normalizes https/ssh/.git/trailing-slash forms, so + // comparing through it avoids a second, subtly different URL parser. + let actual: string; + try { + actual = parseRepoSpec(remote.stdout.trim()).full; + } catch { + throw new Error( + `local mirror at ${dir} does not point at ${spec.full} — its origin is "${remote.stdout.trim()}"`, + ); + } + if (actual !== spec.full) { + throw new Error(`local mirror at ${dir} does not point at ${spec.full} — its origin is ${actual}`); + } + + const fetched = run("git", ["-C", dir, "fetch", "--prune", "origin"]); + if (fetched.status !== 0) { + return { + dir, + freshness: { + state: "unknown", + remoteTip: null, + checkedAt, + reason: (fetched.stderr || fetched.stdout).trim() || "git fetch failed", + }, + }; + } + + const tip = run("git", ["-C", dir, "rev-parse", "FETCH_HEAD"]); + const remoteTip = tip.status === 0 ? tip.stdout.trim() || null : null; + return { dir, freshness: { state: "fresh", remoteTip, checkedAt } }; } /** One-line "open a PR" footer for a finished repo run. Pure. */ diff --git a/test/repo.test.ts b/test/repo.test.ts index 5cb2702..52be4e9 100644 --- a/test/repo.test.ts +++ b/test/repo.test.ts @@ -1,6 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { parseRepoSpec, cloneArgs, prCreateHint } from "../src/core/repo.js"; +import { parseRepoSpec, cloneArgs, prCreateHint, refreshMirror } from "../src/core/repo.js"; +import type { Runner, RunResult } from "../src/core/worktree.js"; test("parseRepoSpec accepts owner/name", () => { const s = parseRepoSpec("octocat/hello-world"); @@ -44,3 +45,97 @@ test("prCreateHint targets the repo + branch", () => { const hint = prCreateHint(parseRepoSpec("octocat/hello-world"), "aether/fix-1"); assert.match(hint, /gh pr create -R octocat\/hello-world --head aether\/fix-1 --fill/); }); + +// ── mirror freshness (SC-A2) ──────────────────────────────────────────────── +// An existing --repo mirror must never be reused as-is. Before a task worktree +// branches off it, the mirror's remote is validated and the mirror is fetched +// through the user's own git/gh auth. When that cannot happen, the result says +// stale or unknown — never fresh. + +function recordingRunner(table: Record): { run: Runner; calls: string[][] } { + const calls: string[][] = []; + const run: Runner = (cmd, args) => { + calls.push([cmd, ...args]); + const key = [cmd, ...args].join(" "); + for (const [pattern, result] of Object.entries(table)) { + if (key.startsWith(pattern)) return result; + } + return { status: 0, stdout: "", stderr: "" }; + }; + return { run, calls }; +} + +const OK = (stdout = ""): RunResult => ({ status: 0, stdout, stderr: "" }); + +test("an existing mirror is fetched, not silently reused", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const { run, calls } = recordingRunner({ + "git -C": OK("https://github.com/octocat/hello-world.git\n"), + }); + const result = refreshMirror(spec, "/mirror", run, { exists: true }); + assert.equal(result.freshness.state, "fresh"); + const fetched = calls.some((call) => call.includes("fetch")); + assert.equal(fetched, true, "an existing mirror must be fetched before use"); +}); + +test("a mirror pointing at a different repo is rejected, never used", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const { run } = recordingRunner({ + "git -C": OK("https://github.com/somebody-else/other-repo.git\n"), + }); + assert.throws( + () => refreshMirror(spec, "/mirror", run, { exists: true }), + /does not point at octocat\/hello-world/, + ); +}); + +test("a failed fetch reports unknown, never fresh", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const run: Runner = (_cmd, args) => { + if (args.includes("fetch")) return { status: 1, stdout: "", stderr: "Could not resolve host: github.com" }; + return OK("https://github.com/octocat/hello-world.git\n"); + }; + const result = refreshMirror(spec, "/mirror", run, { exists: true }); + assert.notEqual(result.freshness.state, "fresh"); + assert.equal(result.freshness.state, "unknown"); + assert.match(result.freshness.reason ?? "", /Could not resolve host/); +}); + +test("refreshing a mirror never checks out, resets or cleans the user's tree", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const { run, calls } = recordingRunner({ + "git -C": OK("https://github.com/octocat/hello-world.git\n"), + }); + refreshMirror(spec, "/mirror", run, { exists: true }); + for (const mutation of ["checkout", "reset", "clean", "merge", "pull", "rebase"]) { + assert.equal( + calls.some((call) => call.includes(mutation)), + false, + `refresh must not run git ${mutation} on the user's mirror`, + ); + } +}); + +test("no Aether credential is ever handed to git or gh", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const { run, calls } = recordingRunner({ + "git -C": OK("https://github.com/octocat/hello-world.git\n"), + }); + refreshMirror(spec, "/mirror", run, { exists: true }); + const flat = calls.flat().join(" "); + for (const leak of ["aek_", "Authorization", "http.extraheader", "GIT_ASKPASS", "x-access-token"]) { + assert.equal(flat.includes(leak), false, `credential material reached the git argv: ${leak}`); + } +}); + +test("a fresh mirror reports the exact base commit a worktree would branch from", () => { + const spec = parseRepoSpec("octocat/hello-world"); + const tip = "a".repeat(40); + const run: Runner = (_cmd, args) => { + if (args.includes("rev-parse")) return OK(tip + "\n"); + return OK("https://github.com/octocat/hello-world.git\n"); + }; + const result = refreshMirror(spec, "/mirror", run, { exists: true }); + assert.equal(result.freshness.state, "fresh"); + assert.equal(result.freshness.remoteTip, tip); +});