From 4fd2b3f77c0176fedc881bb04e5cc88d79e25080 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 19:05:12 +0300 Subject: [PATCH 1/3] fix(engine): a refusal says where its base came from, not just what it was MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 116 worktrees and 5.4 GB sat unreclaimable behind this refusal: kild/triage-1101 carries 365 commits not in main (tip ace0a41) — land it, or retry with force Every word of it true, and the whole of it useless. That repo's default branch is `dev`; `main` is its release branch, permanently behind. So every tree read as hundreds of commits ahead of a branch it had never forked from, by construction, forever. The guard was right that the trees differed from `main` and wrong that this meant unlanded work, and it had no way to know. The refusal DID name the base — `main` is right there. What it could not say is that nobody chose it. `resolveDefaultBase` returned a bare string, so one layer down a guess and a configured fact were the same value, and a refusal built on either read as equally authoritative. Four measurement passes went into rediscovering that from outside. So the resolution now carries its provenance: `explicit` when a caller named the branch, `origin-head` when it came from that symref, `fallback` when nothing did. It rides `KildGitStatus`, `ReviewCommitsResult` and the refusal itself, so a client can render "measured against a guess" rather than a bare number. The wording earns its length. `origin/HEAD` is not the remote's default — it is a symref git writes at CLONE TIME and never refreshes, so it goes stale silently when the default moves. The refusal says that, because the obvious fix on reading "base came from origin/HEAD" is to trust it. What this deliberately does NOT do: query the remote (a network call inside a local guard), run `git remote set-head` (kild rewriting state in a repo it does not own), or infer a base from commit counts (the heuristic that produced the 116 in the first place). The base is knowable from explicit configuration or not at all — `--base`, project `.kild/config.json`, global config, three working levels — and where none exists the engine says it is guessing rather than pretending it is not. Refusing stays correct; being uncheckable was the defect. Gates: 457 tests, typecheck, lint, e2e 70/70. --- engine/src/kild/git-review.ts | 18 ++++++-- engine/src/kild/kild-disposal.test.ts | 65 +++++++++++++++++++++++++++ engine/src/kild/kild-disposal.ts | 22 ++++++++- engine/src/kild/worktree-status.ts | 42 ++++++++++++++--- 4 files changed, 136 insertions(+), 11 deletions(-) diff --git a/engine/src/kild/git-review.ts b/engine/src/kild/git-review.ts index e6a86afe..d1f84a56 100644 --- a/engine/src/kild/git-review.ts +++ b/engine/src/kild/git-review.ts @@ -3,7 +3,7 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import { promisify } from 'node:util'; -import { resolveDefaultBase } from './worktree-status.ts'; +import { type BaseSource, resolveDefaultBase } from './worktree-status.ts'; /** * Review intelligence — the git drill-down behind a review surface. Where @@ -56,6 +56,9 @@ export interface ReviewFile { export interface ReviewCommitsResult { base: string; + /** Where {@link base} came from — see BaseSource. A commit count measured against a guessed + * base is not evidence of unlanded work, and a caller refusing on it must be able to say so. */ + baseSource: BaseSource | 'explicit'; commits: ReviewCommit[]; error?: string; // any git failure captured here, NEVER thrown } @@ -222,8 +225,15 @@ export function parsePorcelainZ(stdout: string): { /** Commits on the kild's branch that base doesn't have (`base..HEAD`), newest * first, each with its own diff stats. Never throws — failures land in `error`. */ export async function reviewCommits(dir: string, base?: string): Promise { - const resolvedBase = base ?? (await resolveDefaultBase(dir)); - const result: ReviewCommitsResult = { base: resolvedBase, commits: [] }; + // An explicit `base` is a caller's assertion; anything else is resolved AND labelled, so a + // refusal built on it can say whether a human chose the branch or the engine guessed it. + const resolved = base ? { base, source: 'explicit' as const } : await resolveDefaultBase(dir); + const resolvedBase = resolved.base; + const result: ReviewCommitsResult = { + base: resolvedBase, + baseSource: resolved.source, + commits: [], + }; const invalid = await verifyRepoAndBase(dir, resolvedBase); if (invalid) { result.error = invalid; @@ -270,7 +280,7 @@ async function countLines(dir: string, file: string): Promise { /** Per-file diff stats vs base — committed (branch vs merge-base) and uncommitted * (working tree, incl. untracked files) combined into one list. Never throws. */ export async function reviewFiles(dir: string, base?: string): Promise { - const resolvedBase = base ?? (await resolveDefaultBase(dir)); + const resolvedBase = base ?? (await resolveDefaultBase(dir)).base; const result: ReviewFilesResult = { base: resolvedBase, files: [] }; const invalid = await verifyRepoAndBase(dir, resolvedBase); if (invalid) { diff --git a/engine/src/kild/kild-disposal.test.ts b/engine/src/kild/kild-disposal.test.ts index 9cc1e286..3ad776c2 100644 --- a/engine/src/kild/kild-disposal.test.ts +++ b/engine/src/kild/kild-disposal.test.ts @@ -209,3 +209,68 @@ test('removal frees the tree and keeps the branch — disposal never deletes wor expect(existsSync(wt.path)).toBe(false); expect((await git('branch')).stdout).toContain('kild/reclaim'); }); + +test('a refusal built on a GUESSED base says so, and one on a chosen base does not', async () => { + // The measured failure: 116 trees held behind "carries 365 commits not in main". True, and + // useless — that repo's default is `dev`, so every tree was hundreds of commits ahead of a + // branch it had never forked from, by construction. The refusal named the base. What it + // could not say was that nobody had chosen it, and that cost four measurement passes and + // 5.4 GB. The count is only evidence of unlanded work if the base is. + const wt = await ensureWorktree(repo, 'guessed', 'main'); + writeFileSync(path.join(wt.path, 'a.txt'), 'authored\n'); + await gitIn(wt.path, 'add', '.'); + await gitIn(wt.path, 'commit', '-q', '-m', 'real work'); + + // No `base` given, and this repo has no origin/HEAD → the literal fallback. + const guessed = await assessDisposal({ + repo, + dir: wt.path, + branch: 'kild/guessed', + inUse: false, + }); + expect(guessed.ok).toBe(false); + if (guessed.ok) return; + expect(guessed.baseSource).toBe('fallback'); + expect(guessed.base).toBe('main'); + expect(guessed.message).toContain('nothing configured a base'); + expect(guessed.message).toContain('.kild/config.json'); + + // Named explicitly: the same refusal, with no hedge, because somebody chose the branch. + const chosen = await assessDisposal({ + repo, + dir: wt.path, + branch: 'kild/guessed', + base: 'main', + inUse: false, + }); + expect(chosen.ok).toBe(false); + if (chosen.ok) return; + expect(chosen.baseSource).toBe('explicit'); + expect(chosen.message).not.toContain('this is a guess'); + expect(chosen.message).toContain('carries 1 commit not in main'); +}); + +test('a base cached in origin/HEAD is labelled as the cache it is', async () => { + // origin/HEAD is written at clone time and never refreshed, so it goes stale silently when + // the remote's default moves. Better than a literal guess; still not a fact anyone asserted. + await git('remote', 'add', 'origin', repo); + await git('symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/main'); + await git('update-ref', 'refs/remotes/origin/main', 'main'); + + const wt = await ensureWorktree(repo, 'cached', 'main'); + writeFileSync(path.join(wt.path, 'a.txt'), 'authored\n'); + await gitIn(wt.path, 'add', '.'); + await gitIn(wt.path, 'commit', '-q', '-m', 'real work'); + + const assessment = await assessDisposal({ + repo, + dir: wt.path, + branch: 'kild/cached', + inUse: false, + }); + expect(assessment.ok).toBe(false); + if (assessment.ok) return; + expect(assessment.baseSource).toBe('origin-head'); + expect(assessment.message).toContain('origin/HEAD'); + expect(assessment.message).toContain('caches at clone time'); +}); diff --git a/engine/src/kild/kild-disposal.ts b/engine/src/kild/kild-disposal.ts index ca070c80..5959001b 100644 --- a/engine/src/kild/kild-disposal.ts +++ b/engine/src/kild/kild-disposal.ts @@ -1,5 +1,6 @@ import { reviewCommits } from './git-review.ts'; import { changedFiles, forceRemoveWorktree, registeredWorktree } from './worktree.ts'; +import type { BaseSource } from './worktree-status.ts'; /** * Disposal — the verb that reclaims a kild's worktree, and the one guard it answers to. @@ -31,6 +32,10 @@ export interface DisposalRefusal { commits?: number; /** For `authored`: short sha of the newest of them. */ tip?: string; + /** For `authored`: the branch the count was measured against, and whether anybody chose it. + * On the wire so a client can render "measured against a guess" rather than a bare number. */ + base?: string; + baseSource?: BaseSource | 'explicit'; } /** A disposal the guard allows, and the cost of going ahead with it. */ @@ -107,15 +112,30 @@ export async function assessDisposal(req: DisposalRequest): Promise 0 && !req.force) { + // Say where the base came from when nobody chose it. "carries 365 commits not in main" is + // a true sentence and a useless one if `main` was a guess — that exact refusal held 116 + // trees and 5.4 GB, because a repo whose default was `dev` read as hundreds of commits + // ahead of a branch it never forked from. The count is not evidence of unlanded work + // unless the base is, and only the engine knows which it had. + const guessed = + review.baseSource === 'origin-head' + ? ` (base ${review.base} came from origin/HEAD, which git caches at clone time and ` + + 'never refreshes — set baseBranch in .kild/config.json if it is wrong)' + : review.baseSource === 'fallback' + ? ` (nothing configured a base, so this is a guess — set baseBranch in ` + + '.kild/config.json, or pass --base)' + : ''; return { ok: false, code: 'authored', message: `${where} carries ${commits} commit${commits === 1 ? '' : 's'} not in ` + - `${review.base}${tip ? ` (tip ${tip})` : ''} — land it, or retry with force ` + + `${review.base}${tip ? ` (tip ${tip})` : ''}${guessed} — land it, or retry with force ` + '(the branch and its commits survive either way)', commits, tip, + base: review.base, + baseSource: review.baseSource, }; } diff --git a/engine/src/kild/worktree-status.ts b/engine/src/kild/worktree-status.ts index a5674747..d2f23ad4 100644 --- a/engine/src/kild/worktree-status.ts +++ b/engine/src/kild/worktree-status.ts @@ -17,6 +17,10 @@ export interface KildGitStatus { path: string; // the dir inspected branch: string | null; base: string; // base branch compared against (default: main) + /** Where {@link base} came from. `explicit` is an assertion by the caller; the others are + * the engine's own resolution, and a client that renders ahead/behind should say so — an + * ahead-count measured against a guessed base is not a fact about anybody's work. */ + baseSource: BaseSource | 'explicit'; ahead: number; // commits on branch not in base behind: number; // commits on base not in branch dirty: boolean; // uncommitted changes present @@ -38,16 +42,40 @@ async function runGit(dir: string, args: string[]): Promise { } } +/** + * Where a base branch came from. Carried alongside the branch itself because a GUESS and a + * CONFIGURED FACT are different claims, and everything downstream — a git summary, a disposal + * refusal — reads as authoritative unless told otherwise. + * + * This is not hypothetical. 116 worktrees sat unreclaimable behind a refusal reading "carries + * 365 commits not in main", which was true and meaningless: that repo's default is `dev`, so + * every tree was hundreds of commits ahead of a branch it had never forked from, by + * construction. The refusal named the base. What it could not say was that nobody had chosen + * it. Four measurement passes went into rediscovering that, and 5.4 GB went into not knowing. + */ +export type BaseSource = + /** `origin/HEAD` — the remote's default AS CACHED LOCALLY. Git writes this symref at clone + * time and never refreshes it, so it goes stale silently when the remote's default moves. + * Better than a literal guess; still not a fact anyone asserted. */ + | 'origin-head' + /** Nothing said otherwise, so `main`. A pure guess, and the one that cost the 116. */ + | 'fallback'; + +export interface ResolvedBase { + base: string; + source: BaseSource; +} + /** The base branch to compare against when the caller doesn't name one: the remote's - * default (`origin/HEAD`, minus the `origin/` prefix) if set, else `main`. Shared - * with git-review so summary status and review drill-down agree on the baseline. */ -export async function resolveDefaultBase(dir: string): Promise { + * default (`origin/HEAD`, minus the `origin/` prefix) if set, else `main` — and WHICH of + * those it was. Shared with git-review so summary status and review drill-down agree. */ +export async function resolveDefaultBase(dir: string): Promise { const head = await runGit(dir, ['symbolic-ref', '--short', 'refs/remotes/origin/HEAD']); if (head.ok) { const branch = head.stdout.trim().replace(/^origin\//, ''); - if (branch) return branch; + if (branch) return { base: branch, source: 'origin-head' }; } - return 'main'; + return { base: 'main', source: 'fallback' }; } /** Inspect one kild directory's git state relative to `base` (default: the @@ -55,11 +83,13 @@ export async function resolveDefaultBase(dir: string): Promise { * ref, or any git error returns a well-formed object with `error` set and safe * defaults so a driving agent can surface the state without crashing. */ export async function kildGitStatus(dir: string, base?: string): Promise { - const resolvedBase = base ?? (await resolveDefaultBase(dir)); + const resolved = base ? { base, source: 'explicit' as const } : await resolveDefaultBase(dir); + const resolvedBase = resolved.base; const status: KildGitStatus = { path: dir, branch: null, base: resolvedBase, + baseSource: resolved.source, ahead: 0, behind: 0, dirty: false, From 0c4450775e21494753d17fe78772e5c81892a4dc Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 19:22:47 +0300 Subject: [PATCH 2/3] fix(engine): the base and its provenance are one fact, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found this PR made the common case WORSE than before it, which is the failure it was written to fix, introduced by the fix. `resolveBaseBranch` was left returning a bare string. Its result is stored as `Kild.base` at creation and is always set, so every downstream caller passed it as the `base` argument — and the new code read any truthy base as `explicit`. The result: every live kild reported `baseSource: 'explicit'` with no hedge, including the ones whose base was `currentBranch() ?? 'main'` and nobody chose. Before this branch an unconfigured base was undifferentiated and honestly uncertain; after it, it asserted a choice that was never made. The new tests passed because they only covered orphan trees, which have no stored base. Fixed by making it impossible to write. `kildGitStatus`, `reviewCommits`, `reviewFiles`, `reviewDiff`, `landPlan`, `landMerge`, `collectLedgerFacts` and the disposal request take a `ResolvedBase`, never a string, so a base cannot travel without its source. `resolveBaseBranch` reports which of flag, config, current-branch or fallback produced it; `Kild` stores both; and `storedBase()` is the one way to read a record back into a comparison. The compiler found all eleven call sites, which is the point of the type carrying it. `BaseSource` gained `unrecorded` for a kild written before any of this. Not folded into `fallback`: claiming we know it was guessed is the same overreach as claiming we know it was chosen. The refusal's `base`/`baseSource` now actually reach the wire. They were documented as being on it and the DELETE handler dropped them, so a client could only recover the hedge by parsing the free-text message — exactly what the structured fields exist to avoid. `reviewFiles`/`reviewDiff` report provenance too. They resolve the base identically to `reviewCommits` one function away and reported nothing, so `git/commits` hedged while `git/files` and `git/diff` did not, measuring against the same branch. One thing worth naming: test files are excluded from `tsc --noEmit`, so every fixture passing a bare `'main'` compiled fine and failed only at runtime. The signature change was invisible to the type checker in precisely the files whose job is to notice. Fixtures are updated; whether the exclusion should stay is its own question. Gates: 457 tests, typecheck, lint, e2e 70/70. --- engine/src/kild/git-review.test.ts | 24 +++++++++++------------ engine/src/kild/git-review.ts | 25 ++++++++++++++---------- engine/src/kild/kild-close.ts | 3 ++- engine/src/kild/kild-disposal.test.ts | 16 +++++++-------- engine/src/kild/kild-disposal.ts | 4 ++-- engine/src/kild/kild-land.test.ts | 18 ++++++++--------- engine/src/kild/kild-land.ts | 11 ++++++++--- engine/src/kild/kild-manager.ts | 16 +++++++++++++-- engine/src/kild/kild-types.ts | 22 ++++++++++++++++++++- engine/src/kild/memory.test.ts | 12 ++++++++---- engine/src/kild/memory.ts | 5 +++-- engine/src/kild/worktree-status.test.ts | 10 +++++----- engine/src/kild/worktree-status.ts | 22 +++++++++++++++++---- engine/src/kild/worktree.ts | 26 +++++++++++++++++++------ engine/src/server.ts | 17 ++++++++-------- 15 files changed, 154 insertions(+), 77 deletions(-) diff --git a/engine/src/kild/git-review.test.ts b/engine/src/kild/git-review.test.ts index 61d98130..16143e57 100644 --- a/engine/src/kild/git-review.test.ts +++ b/engine/src/kild/git-review.test.ts @@ -72,7 +72,7 @@ test('commits vs base come newest-first with per-commit stats', async () => { await git(dir, ['add', '.']); await commit(dir, 'trim a, add b'); - const result = await reviewCommits(dir, 'main'); + const result = await reviewCommits(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeUndefined(); expect(result.base).toBe('main'); @@ -93,21 +93,21 @@ test('commits vs base come newest-first with per-commit stats', async () => { test('no commits ahead of base yields an empty list, no error', async () => { const dir = await initRepo(); - const result = await reviewCommits(dir, 'main'); + const result = await reviewCommits(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeUndefined(); expect(result.commits).toEqual([]); }); test('commits: a missing base ref is an error object, not a crash', async () => { const dir = await initRepo(); - const result = await reviewCommits(dir, 'does-not-exist'); + const result = await reviewCommits(dir, { base: 'does-not-exist', source: 'explicit' }); expect(result.error).toBe('base ref not found: does-not-exist'); expect(result.commits).toEqual([]); }); test('commits: a non-git directory is an error object, not a crash', async () => { const dir = mkTmp('kild-git-review-nogit-'); - const result = await reviewCommits(dir, 'main'); + const result = await reviewCommits(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeDefined(); expect(result.commits).toEqual([]); }); @@ -123,7 +123,7 @@ test('files combine committed, uncommitted, and untracked changes vs base', asyn fs.writeFileSync(path.join(dir, 'README.md'), 'hello\nedited\n'); // uncommitted edit fs.writeFileSync(path.join(dir, 'untracked.txt'), 'u1\nu2\nu3\n'); // never added - const result = await reviewFiles(dir, 'main'); + const result = await reviewFiles(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeUndefined(); const byPath = new Map(result.files.map((file) => [file.path, file])); @@ -160,7 +160,7 @@ test('files report deletions and renames with the pre-rename path', async () => await git(dir, ['mv', 'README.md', 'RENAMED.md']); await commit(dir, 'delete + rename'); - const result = await reviewFiles(dir, 'main'); + const result = await reviewFiles(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeUndefined(); const byPath = new Map(result.files.map((file) => [file.path, file])); @@ -184,7 +184,7 @@ test("files never include the base's own advances (merge-base semantics)", async await commit(dir, 'theirs'); await git(dir, ['checkout', 'feature']); - const result = await reviewFiles(dir, 'main'); + const result = await reviewFiles(dir, { base: 'main', source: 'explicit' }); expect(result.error).toBeUndefined(); expect(result.files.map((file) => file.path)).toEqual(['mine.txt']); @@ -192,7 +192,7 @@ test("files never include the base's own advances (merge-base semantics)", async test('files: a missing base ref is an error object, not a crash', async () => { const dir = await initRepo(); - const result = await reviewFiles(dir, 'does-not-exist'); + const result = await reviewFiles(dir, { base: 'does-not-exist', source: 'explicit' }); expect(result.error).toBe('base ref not found: does-not-exist'); expect(result.files).toEqual([]); }); @@ -207,7 +207,7 @@ test('diff returns one unified patch covering committed + working-tree changes', await commit(dir, 'committed line'); fs.writeFileSync(path.join(dir, 'README.md'), 'hello\ncommitted\nuncommitted\n'); - const result = await reviewDiff(dir, 'main', 'README.md'); + const result = await reviewDiff(dir, { base: 'main', source: 'explicit' }, 'README.md'); expect(result.error).toBeUndefined(); expect(result.truncated).toBe(false); @@ -220,7 +220,7 @@ test('diff covers an untracked file via no-index', async () => { const dir = await initRepo(); fs.writeFileSync(path.join(dir, 'fresh.txt'), 'brand new\n'); - const result = await reviewDiff(dir, 'main', 'fresh.txt'); + const result = await reviewDiff(dir, { base: 'main', source: 'explicit' }, 'fresh.txt'); expect(result.error).toBeUndefined(); expect(result.patch).toContain('+brand new'); @@ -229,7 +229,7 @@ test('diff covers an untracked file via no-index', async () => { test('diff refuses a path git did not report (traversal guard)', async () => { const dir = await initRepo(); for (const evil of ['../../etc/passwd', '/etc/passwd', 'nope.txt']) { - const result = await reviewDiff(dir, 'main', evil); + const result = await reviewDiff(dir, { base: 'main', source: 'explicit' }, evil); expect(result.unknownPath).toBe(true); expect(result.error).toContain('not reported by git'); expect(result.patch).toBe(''); @@ -240,7 +240,7 @@ test('diff larger than the cap is truncated and flagged', async () => { const dir = await initRepo(); fs.writeFileSync(path.join(dir, 'big.txt'), 'x-line-of-payload\n'.repeat(20_000)); // ~360 KB - const result = await reviewDiff(dir, 'main', 'big.txt'); + const result = await reviewDiff(dir, { base: 'main', source: 'explicit' }, 'big.txt'); expect(result.error).toBeUndefined(); expect(result.truncated).toBe(true); diff --git a/engine/src/kild/git-review.ts b/engine/src/kild/git-review.ts index d1f84a56..d721b256 100644 --- a/engine/src/kild/git-review.ts +++ b/engine/src/kild/git-review.ts @@ -3,7 +3,7 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import { promisify } from 'node:util'; -import { type BaseSource, resolveDefaultBase } from './worktree-status.ts'; +import { type BaseSource, type ResolvedBase, resolveDefaultBase } from './worktree-status.ts'; /** * Review intelligence — the git drill-down behind a review surface. Where @@ -58,13 +58,16 @@ export interface ReviewCommitsResult { base: string; /** Where {@link base} came from — see BaseSource. A commit count measured against a guessed * base is not evidence of unlanded work, and a caller refusing on it must be able to say so. */ - baseSource: BaseSource | 'explicit'; + baseSource: BaseSource; commits: ReviewCommit[]; error?: string; // any git failure captured here, NEVER thrown } export interface ReviewFilesResult { base: string; + /** Where {@link base} came from — same contract as ReviewCommitsResult. Stats measured + * against a guessed base are not facts about anybody's work either. */ + baseSource: BaseSource; files: ReviewFile[]; error?: string; // any git failure captured here, NEVER thrown } @@ -224,10 +227,11 @@ export function parsePorcelainZ(stdout: string): { /** Commits on the kild's branch that base doesn't have (`base..HEAD`), newest * first, each with its own diff stats. Never throws — failures land in `error`. */ -export async function reviewCommits(dir: string, base?: string): Promise { - // An explicit `base` is a caller's assertion; anything else is resolved AND labelled, so a - // refusal built on it can say whether a human chose the branch or the engine guessed it. - const resolved = base ? { base, source: 'explicit' as const } : await resolveDefaultBase(dir); +export async function reviewCommits( + dir: string, + base?: ResolvedBase, +): Promise { + const resolved = base ?? (await resolveDefaultBase(dir)); const resolvedBase = resolved.base; const result: ReviewCommitsResult = { base: resolvedBase, @@ -279,9 +283,10 @@ async function countLines(dir: string, file: string): Promise { /** Per-file diff stats vs base — committed (branch vs merge-base) and uncommitted * (working tree, incl. untracked files) combined into one list. Never throws. */ -export async function reviewFiles(dir: string, base?: string): Promise { - const resolvedBase = base ?? (await resolveDefaultBase(dir)).base; - const result: ReviewFilesResult = { base: resolvedBase, files: [] }; +export async function reviewFiles(dir: string, base?: ResolvedBase): Promise { + const resolved = base ?? (await resolveDefaultBase(dir)); + const resolvedBase = resolved.base; + const result: ReviewFilesResult = { base: resolvedBase, baseSource: resolved.source, files: [] }; const invalid = await verifyRepoAndBase(dir, resolvedBase); if (invalid) { result.error = invalid; @@ -368,7 +373,7 @@ async function noIndexDiff(dir: string, file: string): Promise { * and no git/fs call ever receives the raw caller path otherwise. Never throws. */ export async function reviewDiff( dir: string, - base: string | undefined, + base: ResolvedBase | undefined, file: string, ): Promise { const files = await reviewFiles(dir, base); diff --git a/engine/src/kild/kild-close.ts b/engine/src/kild/kild-close.ts index c9576994..ab1173d5 100644 --- a/engine/src/kild/kild-close.ts +++ b/engine/src/kild/kild-close.ts @@ -3,6 +3,7 @@ import path from 'node:path'; import { configuredCloseHook, configuredMemoryDir } from './config.ts'; import { type HookAgentSpawn, type KildCloseEvent, runCloseHook } from './hooks.ts'; import type { Kild } from './kild-types.ts'; +import { storedBase } from './kild-types.ts'; import { appendKildLog, collectLedgerFacts, kildTranscriptPath } from './memory.ts'; import { worktreePath } from './worktree.ts'; @@ -47,7 +48,7 @@ export async function closeKild(kild: Kild, deps: CloseDeps): Promise { appendKildLog( kild, memoryDir, - await collectLedgerFacts(dir, kild.base, kild.landedSha, kild.landed), + await collectLedgerFacts(dir, storedBase(kild), kild.landedSha, kild.landed), ); } catch (err) { console.error(`kild: ledger append failed for '${kild.name}': ${errText(err)}`); diff --git a/engine/src/kild/kild-disposal.test.ts b/engine/src/kild/kild-disposal.test.ts index 3ad776c2..aea28e5d 100644 --- a/engine/src/kild/kild-disposal.test.ts +++ b/engine/src/kild/kild-disposal.test.ts @@ -53,7 +53,7 @@ test('a tree with ONLY untracked litter is disposable, and the litter is named', repo, dir: wt.path, branch: 'kild/litter', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, }); expect(assessment.ok).toBe(true); @@ -77,7 +77,7 @@ test('uncommitted changes to TRACKED files are not a refusal either', async () = repo, dir: wt.path, branch: 'kild/tracked', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, }); expect(assessment.ok).toBe(true); @@ -98,7 +98,7 @@ test('an undeterminable discard list says so, instead of reporting nothing lost' repo, dir: wt.path, branch: 'kild/unreadable', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, force: true, // past the commits guard, which refuses `undetermined` on its own }); @@ -119,7 +119,7 @@ test('a branch carrying commits base does not have is REFUSED, with the count an repo, dir: wt.path, branch: 'kild/authored', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, }); expect(assessment.ok).toBe(false); @@ -143,7 +143,7 @@ test('force overrides the authored refusal — the branch and its commits surviv repo, dir: wt.path, branch: 'kild/forced', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, force: true, }); @@ -183,7 +183,7 @@ test('a base that cannot be resolved is undetermined, and says force is the way repo, dir: wt.path, branch: 'kild/nobase', - base: 'no-such-base', + base: { base: 'no-such-base', source: 'explicit' }, inUse: false, }); expect(assessment).toMatchObject({ ok: false, code: 'undetermined' }); @@ -195,7 +195,7 @@ test('a base that cannot be resolved is undetermined, and says force is the way repo, dir: wt.path, branch: 'kild/nobase', - base: 'no-such-base', + base: { base: 'no-such-base', source: 'explicit' }, inUse: false, force: true, }), @@ -240,7 +240,7 @@ test('a refusal built on a GUESSED base says so, and one on a chosen base does n repo, dir: wt.path, branch: 'kild/guessed', - base: 'main', + base: { base: 'main', source: 'explicit' }, inUse: false, }); expect(chosen.ok).toBe(false); diff --git a/engine/src/kild/kild-disposal.ts b/engine/src/kild/kild-disposal.ts index 5959001b..27d7e74f 100644 --- a/engine/src/kild/kild-disposal.ts +++ b/engine/src/kild/kild-disposal.ts @@ -1,6 +1,6 @@ import { reviewCommits } from './git-review.ts'; import { changedFiles, forceRemoveWorktree, registeredWorktree } from './worktree.ts'; -import type { BaseSource } from './worktree-status.ts'; +import type { BaseSource, ResolvedBase } from './worktree-status.ts'; /** * Disposal — the verb that reclaims a kild's worktree, and the one guard it answers to. @@ -70,7 +70,7 @@ export interface DisposalRequest { /** Branch the tree is on (`kild/`), for the message. */ branch?: string; /** Base branch authored commits are measured against. Absent → the repo's default. */ - base?: string; + base?: ResolvedBase; /** True when a live agent process is working in this tree. */ inUse: boolean; /** Dispose even when the branch carries authored commits. The branch — and every commit diff --git a/engine/src/kild/kild-land.test.ts b/engine/src/kild/kild-land.test.ts index 40d8d45c..ca1d5be1 100644 --- a/engine/src/kild/kild-land.test.ts +++ b/engine/src/kild/kild-land.test.ts @@ -68,7 +68,7 @@ test('the dry run reports what would land and TOUCHES NOTHING', async () => { const beforeTree = await snapshot(wt.path); const beforeRepo = await snapshot(repo); - const plan = await landPlan(wt.path, 'main'); + const plan = await landPlan(wt.path, { base: 'main', source: 'explicit' }); expect(plan.wouldMerge).toBe(true); expect(plan.merged).toBe(false); expect(plan.sha).toBeUndefined(); @@ -89,7 +89,7 @@ test('the dry run names the colliding files and would not merge', async () => { await git('commit', '-q', '-am', 'main moves'); const beforeRepo = await snapshot(repo); - const plan = await landPlan(wt.path, 'main'); + const plan = await landPlan(wt.path, { base: 'main', source: 'explicit' }); expect(plan.wouldMerge).toBe(false); expect(plan.collides).toEqual(['README.md']); expect(plan.error).toContain('conflicts in 1 file'); @@ -99,20 +99,20 @@ test('the dry run names the colliding files and would not merge', async () => { test('a kild with nothing committed would not land, and says exactly that', async () => { const wt = await ensureWorktree(repo, 'empty', 'main'); writeFileSync(path.join(wt.path, 'only-litter.txt'), 'x'); - const plan = await landPlan(wt.path, 'main'); + const plan = await landPlan(wt.path, { base: 'main', source: 'explicit' }); expect(plan.wouldMerge).toBe(false); expect(plan.error).toBe('nothing committed on kild/empty vs main'); }); test('a kild that ran in the checkout has no branch to land', async () => { - const plan = await landPlan(repo, 'main'); + const plan = await landPlan(repo, { base: 'main', source: 'explicit' }); expect(plan.wouldMerge).toBe(false); expect(plan.error).toContain('no branch to land'); }); test('landing merges into base and reports the merge sha', async () => { const wt = await kildWithWork('ship'); - const result = await landMerge(repo, wt.path, 'main'); + const result = await landMerge(repo, wt.path, { base: 'main', source: 'explicit' }); expect(result.merged).toBe(true); expect(result.sha).toMatch(/^[0-9a-f]{40}$/); // The base really carries the work now, at exactly the sha reported. @@ -122,7 +122,7 @@ test('landing merges into base and reports the merge sha', async () => { ); expect((await git('branch', '--merged', 'main')).stdout).toContain('kild/ship'); // And a dry run afterwards agrees there is nothing left to land. - expect((await landPlan(wt.path, 'main')).wouldMerge).toBe(false); + expect((await landPlan(wt.path, { base: 'main', source: 'explicit' })).wouldMerge).toBe(false); }); test('landing refuses when base is not checked out in the repo, and merges nothing', async () => { @@ -130,7 +130,7 @@ test('landing refuses when base is not checked out in the repo, and merges nothi await git('checkout', '-q', '-b', 'side'); const before = await snapshot(repo); - const result = await landMerge(repo, wt.path, 'main'); + const result = await landMerge(repo, wt.path, { base: 'main', source: 'explicit' }); expect(result.merged).toBe(false); expect(result.error).toContain('is on side, not main'); expect(await snapshot(repo)).toEqual(before); @@ -141,7 +141,7 @@ test('landing refuses on a dirty main checkout rather than entangling the merge' writeFileSync(path.join(repo, 'README.md'), 'edited in the checkout\n'); const before = await snapshot(repo); - const result = await landMerge(repo, wt.path, 'main'); + const result = await landMerge(repo, wt.path, { base: 'main', source: 'explicit' }); expect(result.merged).toBe(false); expect(result.error).toContain('uncommitted changes'); expect(await snapshot(repo)).toEqual(before); @@ -153,7 +153,7 @@ test('a conflicting land is refused before git is asked to merge', async () => { await git('commit', '-q', '-am', 'main moves'); const before = await snapshot(repo); - const result = await landMerge(repo, wt.path, 'main'); + const result = await landMerge(repo, wt.path, { base: 'main', source: 'explicit' }); expect(result.merged).toBe(false); expect(result.collides).toEqual(['README.md']); // No half-finished merge left behind for someone else to discover. diff --git a/engine/src/kild/kild-land.ts b/engine/src/kild/kild-land.ts index 22d2bc7f..e1c98f3a 100644 --- a/engine/src/kild/kild-land.ts +++ b/engine/src/kild/kild-land.ts @@ -3,6 +3,7 @@ import { promisify } from 'node:util'; import { type ReviewCommit, reviewCommits } from './git-review.ts'; import { currentBranch } from './worktree.ts'; +import type { ResolvedBase } from './worktree-status.ts'; import { kildGitStatus } from './worktree-status.ts'; /** @@ -91,7 +92,7 @@ export async function conflictingPaths( * `dir` is the kild's effective directory (its worktree, else its cwd); `base` its base * branch (absent → the repo's default). */ -export async function landPlan(dir: string, base?: string): Promise { +export async function landPlan(dir: string, base?: ResolvedBase): Promise { const status = await kildGitStatus(dir, base); const result: LandResult = { base: status.base, @@ -115,7 +116,7 @@ export async function landPlan(dir: string, base?: string): Promise return result; } - const review = await reviewCommits(dir, status.base); + const review = await reviewCommits(dir, { base: status.base, source: status.baseSource }); if (review.error) { result.error = review.error; return result; @@ -147,7 +148,11 @@ export async function landPlan(dir: string, base?: string): Promise * report the merge sha. Runs the same plan first and refuses on anything it flagged, so a * caller never has to interpret two different verdicts. */ -export async function landMerge(repo: string, dir: string, base?: string): Promise { +export async function landMerge( + repo: string, + dir: string, + base?: ResolvedBase, +): Promise { const plan = await landPlan(dir, base); if (!plan.wouldMerge || plan.branch === null) return plan; diff --git a/engine/src/kild/kild-manager.ts b/engine/src/kild/kild-manager.ts index 767c4090..5dce80da 100644 --- a/engine/src/kild/kild-manager.ts +++ b/engine/src/kild/kild-manager.ts @@ -33,9 +33,18 @@ import { type SendOut, type SpawnContext, type StopOut, + storedBase, } from './kild-types.ts'; import { GENERAL_PERSONA, listPersonas } from './personas.ts'; import { resolveBaseBranch, worktreePath } from './worktree.ts'; + +/** The base and its provenance as stored fields — one call, so the two can never be recorded + * apart. A `base` without its `baseSource` is a guess wearing a decision's clothes. */ +async function resolvedBaseFields(cwd: string, flag?: string) { + const { base, source } = await resolveBaseBranch(cwd, flag); + return { base, baseSource: source }; +} + import { kildGitStatus } from './worktree-status.ts'; /** Soft cap on kild size — a cheap loop/scale guard in v1 (loop control is otherwise @@ -179,7 +188,7 @@ export class KildManager { // Resolve the base once here — the single chokepoint every creator (CLI, REST, WS) // flows through: explicit `base` wins, else the cwd's configured `baseBranch`, else // its current branch, else `main`. - base: await resolveBaseBranch(spec.cwd, spec.base), + ...(await resolvedBaseFields(spec.cwd, spec.base)), agents: [], log: [], }; @@ -265,7 +274,10 @@ export class KildManager { landedSha: kild.landedSha, agents: kild.agents.map(agentView), totals: costTotals(kild.agents), - git: await kildGitStatus(kild.worktree ? worktreePath(kild.worktree) : kild.cwd, kild.base), + git: await kildGitStatus( + kild.worktree ? worktreePath(kild.worktree) : kild.cwd, + storedBase(kild), + ), }; } diff --git a/engine/src/kild/kild-types.ts b/engine/src/kild/kild-types.ts index c1145897..8c12913d 100644 --- a/engine/src/kild/kild-types.ts +++ b/engine/src/kild/kild-types.ts @@ -1,7 +1,7 @@ import type { UiEvent } from './events.ts'; import type { KildCloseEvent } from './hooks.ts'; import type { Inbox } from './inbox.ts'; -import type { KildGitStatus } from './worktree-status.ts'; +import type { BaseSource, KildGitStatus, ResolvedBase } from './worktree-status.ts'; /** * Kild domain — the operator-facing primitive: a set of agents exchanging directed @@ -281,6 +281,11 @@ export interface NewKildSpec { /** Base branch for the worktree + git-status baseline (default: the checkout's current * branch). Editable via `.kild/config.json` `baseBranch` or the `--base` CLI flag. */ base?: string; + /** Where {@link base} came from. Stored with it because the two are one fact: a base + * resolved by fallback and a base a human named produce identical strings, and everything + * measured against them — ahead/behind, a disposal refusal — is only as certain as the + * base is. Recording the string alone is what let a guess be reported as a choice. */ + baseSource?: BaseSource; } /** Lightweight kild descriptor for client lists. Its presence in a `{kilds}` broadcast @@ -496,3 +501,18 @@ export interface CommandAck { requestId: string; result: CommandResult; } + +/** + * A kild's recorded base, read as the pair it is. + * + * The ONE way to turn a stored `base` back into something a git comparison accepts, so no + * caller can hand a stored string to a function expecting a resolved base and have it treated + * as an assertion. That laundering is what made every live kild report its base as `explicit` + * — including the ones whose base was `currentBranch() ?? 'main'` at creation. + */ +export function storedBase(kild: { + base?: string; + baseSource?: BaseSource; +}): ResolvedBase | undefined { + return kild.base ? { base: kild.base, source: kild.baseSource ?? 'unrecorded' } : undefined; +} diff --git a/engine/src/kild/memory.test.ts b/engine/src/kild/memory.test.ts index ffdac9a8..b4324661 100644 --- a/engine/src/kild/memory.test.ts +++ b/engine/src/kild/memory.test.ts @@ -170,7 +170,11 @@ test('a RECORDED merge sha wins over inference — the ledger names the commit', test('collectLedgerFacts treats a recorded sha as landed without asking git', async () => { const dir = await repoWithBranch(); // 2 commits ahead of main, nothing merged - const collected = await collectLedgerFacts(dir, 'main', 'abcdef1234567890'); + const collected = await collectLedgerFacts( + dir, + { base: 'main', source: 'explicit' }, + 'abcdef1234567890', + ); expect(collected.landed).toBe(true); expect(collected.landedSha).toBe('abcdef1234567890'); // The code facts are still measured, not overwritten by the land record. @@ -256,7 +260,7 @@ test('collectLedgerFacts reports real commits, files and an unlanded branch', as const dir = await repoWithBranch(); fs.writeFileSync(path.join(dir, 'c.ts'), 'x\n'); // uncommitted - const collected = await collectLedgerFacts(dir, 'main'); + const collected = await collectLedgerFacts(dir, { base: 'main', source: 'explicit' }); expect(collected.gitError).toBeUndefined(); expect(collected.base).toBe('main'); expect(collected.branch).toBe('kild/work'); @@ -273,14 +277,14 @@ test('collectLedgerFacts reports landed once the branch is contained in base', a await git(dir, ['merge', '--no-ff', '-m', 'land', 'kild/work']); await git(dir, ['checkout', 'kild/work']); - const collected = await collectLedgerFacts(dir, 'main'); + const collected = await collectLedgerFacts(dir, { base: 'main', source: 'explicit' }); expect(collected.commits).toBe(0); expect(collected.landed).toBe(true); }); test('collectLedgerFacts on a non-repo yields an error, not a throw', async () => { const dir = fs.mkdtempSync(path.join(tmp, 'norepo-')); - const collected = await collectLedgerFacts(dir, 'main'); + const collected = await collectLedgerFacts(dir, { base: 'main', source: 'explicit' }); expect(collected.gitError).toBeDefined(); expect(collected.landed).toBe(false); expect(collected.commits).toBe(0); diff --git a/engine/src/kild/memory.ts b/engine/src/kild/memory.ts index 22c7e5f6..a64c96ce 100644 --- a/engine/src/kild/memory.ts +++ b/engine/src/kild/memory.ts @@ -4,6 +4,7 @@ import path from 'node:path'; import { kildHome } from './config.ts'; import { reviewCommits } from './git-review.ts'; import type { Kild } from './kild-types.ts'; +import type { ResolvedBase } from './worktree-status.ts'; import { kildGitStatus } from './worktree-status.ts'; /** @@ -115,12 +116,12 @@ export interface KildLedgerFacts { */ export async function collectLedgerFacts( dir: string, - base?: string, + base?: ResolvedBase, landedSha?: string, carried?: { commits: number; files: number }, ): Promise { const status = await kildGitStatus(dir, base); - const review = await reviewCommits(dir, status.base); + const review = await reviewCommits(dir, { base: status.base, source: status.baseSource }); const gitError = status.error ?? review.error; const tip = review.commits[0]?.sha.slice(0, 7); return { diff --git a/engine/src/kild/worktree-status.test.ts b/engine/src/kild/worktree-status.test.ts index 88dff973..7e763c8c 100644 --- a/engine/src/kild/worktree-status.test.ts +++ b/engine/src/kild/worktree-status.test.ts @@ -73,7 +73,7 @@ test('a branch one commit ahead reports ahead 1 and the changed file', async () await git(dir, ['add', '.']); await commit(dir, 'add feature'); - const status = await kildGitStatus(dir, 'main'); + const status = await kildGitStatus(dir, { base: 'main', source: 'explicit' }); expect(status.error).toBeUndefined(); expect(status.branch).toBe('feature'); @@ -88,7 +88,7 @@ test('an uncommitted edit marks the kild dirty', async () => { const dir = await initRepo(); fs.writeFileSync(path.join(dir, 'README.md'), 'changed\n'); - const status = await kildGitStatus(dir, 'main'); + const status = await kildGitStatus(dir, { base: 'main', source: 'explicit' }); expect(status.error).toBeUndefined(); expect(status.dirty).toBe(true); @@ -119,7 +119,7 @@ test('a branch that merges cleanly into base reports conflictsWithBase false', a await git(dir, ['add', '.']); await commit(dir, 'add feature'); // new file, no overlap with base - const status = await kildGitStatus(dir, 'main'); + const status = await kildGitStatus(dir, { base: 'main', source: 'explicit' }); expect(status.error).toBeUndefined(); expect(status.ahead).toBe(1); @@ -139,7 +139,7 @@ test('a branch that edits the same line as base reports conflictsWithBase true', await commit(dir, 'main edit'); await git(dir, ['checkout', 'feature']); - const status = await kildGitStatus(dir, 'main'); + const status = await kildGitStatus(dir, { base: 'main', source: 'explicit' }); expect(status.ahead).toBe(1); expect(status.behind).toBe(1); @@ -149,7 +149,7 @@ test('a branch that edits the same line as base reports conflictsWithBase true', test('a missing base ref is reported as an error, not a crash', async () => { const dir = await initRepo(); - const status = await kildGitStatus(dir, 'does-not-exist'); + const status = await kildGitStatus(dir, { base: 'does-not-exist', source: 'explicit' }); expect(status.error).toBeDefined(); expect(status.base).toBe('does-not-exist'); diff --git a/engine/src/kild/worktree-status.ts b/engine/src/kild/worktree-status.ts index d2f23ad4..599fa70a 100644 --- a/engine/src/kild/worktree-status.ts +++ b/engine/src/kild/worktree-status.ts @@ -20,7 +20,7 @@ export interface KildGitStatus { /** Where {@link base} came from. `explicit` is an assertion by the caller; the others are * the engine's own resolution, and a client that renders ahead/behind should say so — an * ahead-count measured against a guessed base is not a fact about anybody's work. */ - baseSource: BaseSource | 'explicit'; + baseSource: BaseSource; ahead: number; // commits on branch not in base behind: number; // commits on base not in branch dirty: boolean; // uncommitted changes present @@ -54,12 +54,22 @@ async function runGit(dir: string, args: string[]): Promise { * it. Four measurement passes went into rediscovering that, and 5.4 GB went into not knowing. */ export type BaseSource = + /** A caller named it — `--base`, or a query param. The only one that is an assertion. */ + | 'explicit' + /** `baseBranch` in project or global config. Chosen deliberately, just not in this call. */ + | 'configured' + /** The checkout's current branch at creation time. Plausible, nobody chose it. */ + | 'current-branch' /** `origin/HEAD` — the remote's default AS CACHED LOCALLY. Git writes this symref at clone * time and never refreshes it, so it goes stale silently when the remote's default moves. * Better than a literal guess; still not a fact anyone asserted. */ | 'origin-head' /** Nothing said otherwise, so `main`. A pure guess, and the one that cost the 116. */ - | 'fallback'; + | 'fallback' + /** A kild recorded before provenance was tracked. Not a guess and not a choice — an + * absence. Named rather than folded into `fallback`, because claiming we know it was + * guessed is the same overreach as claiming we know it was chosen. */ + | 'unrecorded'; export interface ResolvedBase { base: string; @@ -82,8 +92,12 @@ export async function resolveDefaultBase(dir: string): Promise { * remote default branch, else `main`). Never throws: a non-git dir, a missing base * ref, or any git error returns a well-formed object with `error` set and safe * defaults so a driving agent can surface the state without crashing. */ -export async function kildGitStatus(dir: string, base?: string): Promise { - const resolved = base ? { base, source: 'explicit' as const } : await resolveDefaultBase(dir); +export async function kildGitStatus(dir: string, base?: ResolvedBase): Promise { + // Takes a ResolvedBase, never a bare string. A string parameter meant any caller holding a + // stored base passed it as though a human had named it, so `Kild.base` — itself resolved by + // a fallback chain at creation — was reported as `explicit` for every live kild. The base and + // where it came from travel together or the provenance is decoration. + const resolved = base ?? (await resolveDefaultBase(dir)); const resolvedBase = resolved.base; const status: KildGitStatus = { path: dir, diff --git a/engine/src/kild/worktree.ts b/engine/src/kild/worktree.ts index 9aa7bbe6..0e9bd53a 100644 --- a/engine/src/kild/worktree.ts +++ b/engine/src/kild/worktree.ts @@ -4,6 +4,7 @@ import path from 'node:path'; import { promisify } from 'node:util'; import { configuredBaseBranch, kildHome } from './config.ts'; +import type { ResolvedBase } from './worktree-status.ts'; // execFile (no shell) + a branch-name allowlist: the brain's create_worktree tool // and UI clients' worktree selectors feed a (possibly LLM-generated) name in here, @@ -82,12 +83,25 @@ export async function currentBranch(repo: string): Promise { return branch || undefined; } -/** Resolve the base branch for a worktree/kild in `cwd`: explicit `flag` wins, else the - * configured `baseBranch` (project over global), else the checkout's current branch, else - * `main`. This is the branch new worktrees fork from and that git status is measured - * against, so ahead/behind + collisions reflect this kild's own work. */ -export async function resolveBaseBranch(cwd: string, flag?: string): Promise { - return flag ?? (await configuredBaseBranch(cwd)) ?? (await currentBranch(cwd)) ?? 'main'; +/** + * Resolve the base branch for a worktree/kild in `cwd`, AND say where it came from: explicit + * `flag` wins, else the configured `baseBranch` (project over global), else the checkout's + * current branch, else `main`. + * + * The provenance is not decoration. This result is stored as `Kild.base` at creation and then + * handed to every git comparison for the rest of the kild's life. Returned as a bare string it + * was indistinguishable from a base a human named, so a guess laundered into an assertion the + * moment it was stored — and every refusal measured against it claimed a certainty nobody had. + * That is the failure that held 116 worktrees; storing the string without its source is how it + * would have survived the fix. + */ +export async function resolveBaseBranch(cwd: string, flag?: string): Promise { + if (flag) return { base: flag, source: 'explicit' }; + const configured = await configuredBaseBranch(cwd); + if (configured) return { base: configured, source: 'configured' }; + const current = await currentBranch(cwd); + if (current) return { base: current, source: 'current-branch' }; + return { base: 'main', source: 'fallback' }; } /** Create a fresh isolated worktree on a `kild/` branch, force-resetting any diff --git a/engine/src/server.ts b/engine/src/server.ts index eff13b4d..685cc5e3 100644 --- a/engine/src/server.ts +++ b/engine/src/server.ts @@ -20,6 +20,7 @@ import { landMerge, landPlan } from './kild/kild-land.ts'; import { kildManager } from './kild/kild-manager.ts'; import { type KildTree, kildTrees, orphanTrees } from './kild/kild-trees.ts'; import type { AgentSpec, CommandResult, KildIdentity, KildStatus } from './kild/kild-types.ts'; +import { storedBase } from './kild/kild-types.ts'; import { listPersonas } from './kild/personas.ts'; import { findProject, loadProjects } from './kild/projects.ts'; import { @@ -465,7 +466,7 @@ app.get('/api/kilds/:id', async (c) => { path: target.value.dir, repo: target.value.repo, }), - git: await kildGitStatus(target.value.dir, target.value.base), + git: await kildGitStatus(target.value.dir, storedBase(target.value)), }); }); @@ -829,7 +830,7 @@ app.delete('/api/kilds/:id', async (c) => { if (!target.ok) { return c.json({ error: target.message, code: target.code }, kildResultStatus(target)); } - const { worktree, repo, dir, base, live, name } = target.value; + const { worktree, repo, dir, live, name } = target.value; if (!worktree) { return c.json( { @@ -844,7 +845,7 @@ app.delete('/api/kilds/:id', async (c) => { repo, dir, branch: `kild/${worktree}`, - base, + base: storedBase(target.value), inUse: worktreesInUse().has(worktree), force, }); @@ -902,7 +903,7 @@ app.get('/api/kilds/:id/land', async (c) => { if (!target.ok) { return c.json({ error: target.message, code: target.code }, kildResultStatus(target)); } - return c.json({ ...(await landPlan(target.value.dir, target.value.base)), dryRun: true }); + return c.json({ ...(await landPlan(target.value.dir, storedBase(target.value))), dryRun: true }); }); app.post('/api/kilds/:id/land', async (c) => { const id = c.req.param('id'); @@ -910,7 +911,7 @@ app.post('/api/kilds/:id/land', async (c) => { if (!target.ok) { return c.json({ error: target.message, code: target.code }, kildResultStatus(target)); } - const result = await landMerge(target.value.repo, target.value.dir, target.value.base); + const result = await landMerge(target.value.repo, target.value.dir, storedBase(target.value)); // A land that did not happen is NOT a success — the caller must be able to tell. if (!result.merged) return c.json({ ...result, dryRun: false }, 409); if (target.value.live && result.sha) { @@ -933,14 +934,14 @@ app.get('/api/kilds/:id/git/commits', async (c) => { if (!located.ok) { return c.json({ error: located.message, code: located.code }, kildResultStatus(located)); } - return c.json(await reviewCommits(located.value.dir, located.value.base)); + return c.json(await reviewCommits(located.value.dir, storedBase(located.value))); }); app.get('/api/kilds/:id/git/files', async (c) => { const located = kildManager.kildDir(c.req.param('id')); if (!located.ok) { return c.json({ error: located.message, code: located.code }, kildResultStatus(located)); } - return c.json(await reviewFiles(located.value.dir, located.value.base)); + return c.json(await reviewFiles(located.value.dir, storedBase(located.value))); }); app.get('/api/kilds/:id/git/diff', async (c) => { const file = c.req.query('path'); @@ -949,7 +950,7 @@ app.get('/api/kilds/:id/git/diff', async (c) => { if (!located.ok) { return c.json({ error: located.message, code: located.code }, kildResultStatus(located)); } - const diff = await reviewDiff(located.value.dir, located.value.base, file); + const diff = await reviewDiff(located.value.dir, storedBase(located.value), file); // The traversal guard: only a path git itself reported may be diffed. if (diff.unknownPath) return c.json({ error: diff.error }, 404); return c.json(diff); From cb7c03ba756731555d5c5c1e705ae70f169d4ce6 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 19:40:35 +0300 Subject: [PATCH 3/3] build(engine): typecheck the tests too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tsconfig.json` excluded `src/**/*.test.ts`, so the files whose job is to notice drift were the only ones nothing checked. That is not theoretical: this branch changed a signature from `string` to `ResolvedBase`, every fixture kept passing a bare `'main'`, tsc was silent, and they failed at runtime instead — in tests that had been asserting against a shape the code no longer had. Two thirds of the errors this surfaced were one missing dependency: `bun:test` had no type declarations, so 34 of 67 were `cannot find module`. `@types/bun` plus `types: ["node", "bun"]` clears all of them and is a straightforward gap, not drift. The remaining 33 were real, and the interesting ones are the fixtures that had quietly stopped modelling their types: - `AgentView` literals with no `ownership` — the very field this codebase refuses to synthesise for archived agents, absent from fixtures claiming to BE an AgentView. A test asserting on that shape was not testing what it said. - `Message` literals with no `seq` — the cursor the whole log is ordered by. - `KildGitStatus` literals with no `baseSource`, from this branch's own change. `CompactGitStatus` gained `baseSource` because the type checker caught it riding that wire undeclared: present in the JSON, typed nowhere, rendered nowhere. It is declared and rendered now — `formatCompactGitSummary` marks the comparison when nobody chose the base, since an ahead/behind count is only a fact about somebody's work if the base is one. `changedFiles` is dropped there deliberately; this is kept deliberately, and now the type says which. One cast survives, at the pi SDK boundary in agent.fork.test.ts: pi's exported `Message` union is narrower than what `appendMessage` takes at runtime. Cast with the reason written down, rather than loosening our own types around it. Verified the net actually catches things rather than assuming: removing a single `ownership` from one fixture fails typecheck. Gates: 457 tests, typecheck (now including tests), lint, e2e 70/70. --- engine/bun.lock | 5 +++ engine/package.json | 1 + engine/src/agent.fork.test.ts | 6 +++- engine/src/kild/attachment.test.ts | 2 +- engine/src/kild/kild-land.test.ts | 2 +- engine/src/kild/kild-registry.test.ts | 2 +- engine/src/kild/kilds-status.test.ts | 45 ++++++++++++++++++++------- engine/src/kild/kilds-status.ts | 14 +++++++-- engine/src/kild/memory.test.ts | 2 ++ engine/tsconfig.json | 4 +-- 10 files changed, 63 insertions(+), 20 deletions(-) diff --git a/engine/bun.lock b/engine/bun.lock index e5595f37..ed8dcb31 100644 --- a/engine/bun.lock +++ b/engine/bun.lock @@ -14,6 +14,7 @@ }, "devDependencies": { "@biomejs/biome": "^2.0.0", + "@types/bun": "^1.3.14", "@types/node": "^22.0.0", "typescript": "^5.6.0", }, @@ -196,6 +197,8 @@ "@tokenizer/token": ["@tokenizer/token@0.3.0", "", {}, "sha512-OvjF+z51L3ov0OyAU0duzsYuvO01PH7x4t6DJx+guahgTnBHkhJdG7soQeTSFLWN3efnHyibZ4Z8l2EuWwJN3A=="], + "@types/bun": ["@types/bun@1.3.14", "", { "dependencies": { "bun-types": "1.3.14" } }, "sha512-h1hFqFVcvAvD9j9K7ZW7vd82aSA+rTdznZa+5bwvCwqSB1jmmfLcbIWhOLx1/+boy/xmjgCs/OMUL8hRJSmnPw=="], + "@types/node": ["@types/node@22.19.19", "", { "dependencies": { "undici-types": "~6.21.0" } }, "sha512-dyh/xO2Fh5bYrfWaaqGrRQQGkNdmYw6AmaAUvYeUMNTWQtvb796ikLdmTchRmOlOiIJ1TDXfWgVx1QkUlQ6Hew=="], "@types/retry": ["@types/retry@0.12.0", "", {}, "sha512-wWKOClTTiizcZhXnPY4wikVAwmdYHp8q6DmC+EJUzAMsycb7HB32Kh9RN4+0gExjmPmZSAQjgURXIGATPegAvA=="], @@ -226,6 +229,8 @@ "buffer-equal-constant-time": ["buffer-equal-constant-time@1.0.1", "", {}, "sha512-zRpUiDwd/xk6ADqPMATG8vc9VPrkck7T07OIx0gnjmJAnHnTVXNQG3vfvWNuiZIkwu9KrKdA1iJKfsfTVxE6NA=="], + "bun-types": ["bun-types@1.3.14", "", { "dependencies": { "@types/node": "*" } }, "sha512-4N0ig0fEomHt5R0KCFWjovxow98rIoRwKolrYdCcknNwMekCXRnWEUvgu5soYV8QXtVsrUD8B95MBOZGPvr6KQ=="], + "bytes": ["bytes@3.1.2", "", {}, "sha512-/Nf7TyzTx6S3yRJObOAV7956r8cr2+Oj8AC5dt8wSP3BQAoeX58NoHyCU8P8zGkNXStjTSi6fzO6F0pBdcYbEg=="], "call-bind-apply-helpers": ["call-bind-apply-helpers@1.0.2", "", { "dependencies": { "es-errors": "^1.3.0", "function-bind": "^1.1.2" } }, "sha512-Sp1ablJ0ivDkSzjcaJdxEunN5/XvksFJ2sMBFfq6x0ryhQV/2b/KwFe21cMpmHtPOSij8K99/wSfoEuTObmuMQ=="], diff --git a/engine/package.json b/engine/package.json index 076ccd44..f8e933e0 100644 --- a/engine/package.json +++ b/engine/package.json @@ -28,6 +28,7 @@ }, "devDependencies": { "@biomejs/biome": "^2.0.0", + "@types/bun": "^1.3.14", "@types/node": "^22.0.0", "typescript": "^5.6.0" } diff --git a/engine/src/agent.fork.test.ts b/engine/src/agent.fork.test.ts index cc08e9be..c2bd3d29 100644 --- a/engine/src/agent.fork.test.ts +++ b/engine/src/agent.fork.test.ts @@ -70,10 +70,14 @@ test('the source session file is never written — even when the fork is appende fs.mkdtempSync(path.join(tmp, 'target-')), sessionDir, ); + // Cast at the SDK boundary, not around our own types. pi's exported `Message` union is + // narrower than what `appendMessage` accepts at runtime — this shape is what the agent + // actually sends and what the assertions below verify the SDK does with it. The cast + // documents that mismatch rather than hiding a shape our code got wrong. forked.appendMessage({ role: 'user', content: [{ type: 'text', text: 'a question against the frozen snapshot' }], - }); + } as Parameters[0]); expect(fs.readFileSync(source, 'utf8')).toBe(before); // byte-identical expect(fs.readFileSync(forked.getSessionFile() as string, 'utf8')).toContain( diff --git a/engine/src/kild/attachment.test.ts b/engine/src/kild/attachment.test.ts index c5a9f0f0..04efce6d 100644 --- a/engine/src/kild/attachment.test.ts +++ b/engine/src/kild/attachment.test.ts @@ -161,7 +161,7 @@ describe('concurrent attaches', () => { path.join(home, 'attached', 'claims', 'kild-w', 'kild'), 'utf8', ); - expect(claim.trim()).toBe(winner); + expect(claim.trim()).toBe(winner as string); expect(await findAttachment(winner as string)).toMatchObject({ kildId: 'kild-w' }); }); diff --git a/engine/src/kild/kild-land.test.ts b/engine/src/kild/kild-land.test.ts index ca1d5be1..772f2725 100644 --- a/engine/src/kild/kild-land.test.ts +++ b/engine/src/kild/kild-land.test.ts @@ -116,7 +116,7 @@ test('landing merges into base and reports the merge sha', async () => { expect(result.merged).toBe(true); expect(result.sha).toMatch(/^[0-9a-f]{40}$/); // The base really carries the work now, at exactly the sha reported. - expect((await git('rev-parse', 'HEAD')).stdout.trim()).toBe(result.sha); + expect((await git('rev-parse', 'HEAD')).stdout.trim()).toBe(result.sha as string); expect((await git('log', '--format=%s', '-1')).stdout.trim()).toBe( 'kild: land kild/ship into main', ); diff --git a/engine/src/kild/kild-registry.test.ts b/engine/src/kild/kild-registry.test.ts index b35303f7..e5f17a36 100644 --- a/engine/src/kild/kild-registry.test.ts +++ b/engine/src/kild/kild-registry.test.ts @@ -32,7 +32,7 @@ function kild(id: string): Kild { } function msg(kildId: string, text: string): Message { - return { id: `${kildId}-1`, kildId, from: 'human', to: ['agent'], text, ts: 1 }; + return { id: `${kildId}-1`, kildId, from: 'human', to: ['agent'], text, ts: 1, seq: 1 }; } test('appendMessage write-throughs the kild log; an empty kild leaves no file', () => { diff --git a/engine/src/kild/kilds-status.test.ts b/engine/src/kild/kilds-status.test.ts index 71e59e3c..b34da018 100644 --- a/engine/src/kild/kilds-status.test.ts +++ b/engine/src/kild/kilds-status.test.ts @@ -18,6 +18,7 @@ test('formatCompactGitSummary preserves clean known-branch divergence', () => { uncommittedFiles: 0, changedFileCount: 0, conflictsWithBase: null, + baseSource: 'explicit' as const, }), ).toEqual(' · feature-x +2/-1'); }); @@ -34,6 +35,7 @@ test('formatCompactGitSummary renders a null branch as unknown', () => { uncommittedFiles: 0, changedFileCount: 0, conflictsWithBase: null, + baseSource: 'explicit' as const, }), ).toEqual(' · ? +0/-0'); }); @@ -50,6 +52,7 @@ test('formatCompactGitSummary appends dirty and conflict markers', () => { uncommittedFiles: 1, changedFileCount: 1, conflictsWithBase: true, + baseSource: 'explicit' as const, }), ).toEqual(' · feature-x +2/-0 dirty CONFLICTS'); }); @@ -60,11 +63,15 @@ test('a compacted kild carries NO log — the thread is its own cursored resourc id: 'kild-1', name: 'ops', cwd: '/tmp/ops', - agents: [{ handle: 'brain', persona: 'brain' }], + agents: [{ handle: 'brain', ownership: 'owned' as const, persona: 'brain' }], }, ]); expect(compact).toEqual([ - { id: 'kild-1', name: 'ops', agents: [{ handle: 'brain', persona: 'brain' }] }, + { + id: 'kild-1', + name: 'ops', + agents: [{ handle: 'brain', ownership: 'owned' as const, persona: 'brain' }], + }, ]); // Neither the whole log nor a "last couple messages" teaser: a listing that carries messages // is a listing whose size is unbounded in the conversation. @@ -83,13 +90,14 @@ test('git compacts to a summary: changed-file COUNT, not the list (pull discipli uncommittedFiles: 1, changedFiles: ['src/a.ts', 'src/b.ts'], conflictsWithBase: null, + baseSource: 'explicit' as const, }; const compact = compactLiveKilds([ { id: 'kild-1', name: 'ops', cwd: '/tmp/ops', - agents: [{ handle: 'brain', persona: 'brain' }], + agents: [{ handle: 'brain', ownership: 'owned' as const, persona: 'brain' }], git, }, ]); @@ -103,6 +111,7 @@ test('git compacts to a summary: changed-file COUNT, not the list (pull discipli uncommittedFiles: 1, changedFileCount: 2, conflictsWithBase: null, + baseSource: 'explicit' as const, }); // The full list is NOT in the director's compact view. expect(compact[0]?.git).not.toHaveProperty('changedFiles'); @@ -110,7 +119,12 @@ test('git compacts to a summary: changed-file COUNT, not the list (pull discipli test('a live kild without git status has no git key', () => { const compact = compactLiveKilds([ - { id: 'kild-1', name: 'ops', cwd: '/tmp/ops', agents: [{ handle: 'brain', persona: 'brain' }] }, + { + id: 'kild-1', + name: 'ops', + cwd: '/tmp/ops', + agents: [{ handle: 'brain', ownership: 'owned' as const, persona: 'brain' }], + }, ]); expect(compact[0]).not.toHaveProperty('git'); }); @@ -120,7 +134,7 @@ test('collisions: two kilds that touch the same file each name the other', () => id, name, cwd: '/tmp/repo', - agents: [{ handle: 'coder', persona: 'coder' }], + agents: [{ handle: 'coder', ownership: 'owned' as const, persona: 'coder' }], git: { path: `/tmp/${name}`, branch: name, @@ -131,6 +145,7 @@ test('collisions: two kilds that touch the same file each name the other', () => uncommittedFiles: 0, changedFiles, conflictsWithBase: null, + baseSource: 'explicit' as const, }, }); const compact = compactLiveKilds([ @@ -149,22 +164,28 @@ test('per-agent attention + cost ride the compact view, with a kild totals rollu { id: 'kild-1', name: 'ops', + cwd: '/tmp/ops', agents: [ - { handle: 'coder', idle: true, tokens: 3400, cost: 1.25 }, - { handle: 'reviewer', tokens: 600, cost: 0.25 }, + { handle: 'coder', ownership: 'owned' as const, idle: true, tokens: 3400, cost: 1.25 }, + { handle: 'reviewer', ownership: 'owned' as const, tokens: 600, cost: 0.25 }, ], }, ]); expect(compact[0]?.agents).toEqual([ - { handle: 'coder', idle: true, tokens: 3400, cost: 1.25 }, - { handle: 'reviewer', tokens: 600, cost: 0.25 }, + { handle: 'coder', ownership: 'owned' as const, idle: true, tokens: 3400, cost: 1.25 }, + { handle: 'reviewer', ownership: 'owned' as const, tokens: 600, cost: 0.25 }, ]); expect(compact[0]?.totals).toEqual({ tokens: 4000, cost: 1.5 }); }); test('a kild whose agents have no stats gets no totals key', () => { const compact = compactLiveKilds([ - { id: 'kild-1', name: 'ops', cwd: '/tmp/ops', agents: [{ handle: 'coder' }] }, + { + id: 'kild-1', + name: 'ops', + cwd: '/tmp/ops', + agents: [{ handle: 'coder', ownership: 'owned' as const }], + }, ]); expect(compact[0]).not.toHaveProperty('totals'); }); @@ -175,7 +196,7 @@ test('server-computed totals on the live status are preferred over recomputing', id: 'kild-1', name: 'ops', cwd: '/tmp/ops', - agents: [{ handle: 'coder', tokens: 100, cost: 0.1 }], + agents: [{ handle: 'coder', ownership: 'owned' as const, tokens: 100, cost: 0.1 }], totals: { tokens: 4000, cost: 1.5 }, }, ]); @@ -188,7 +209,7 @@ test('compaction copies the agent array without mutating the source kild', () => id: 'kild-1', name: 'ops', cwd: '/tmp/ops', - agents: [{ handle: 'brain', persona: 'brain' }], + agents: [{ handle: 'brain', ownership: 'owned' as const, persona: 'brain' }], }, ]; diff --git a/engine/src/kild/kilds-status.ts b/engine/src/kild/kilds-status.ts index 8ea18f2b..61831168 100644 --- a/engine/src/kild/kilds-status.ts +++ b/engine/src/kild/kilds-status.ts @@ -1,5 +1,5 @@ import { type AgentView, type CostTotals, costTotals, type KildStatus } from './kild-types.ts'; -import type { KildGitStatus } from './worktree-status.ts'; +import type { BaseSource, KildGitStatus } from './worktree-status.ts'; /** The director's compact view of a kild's git state: a summary, not the full * changed-file list. Per the pull-not-push discipline, the director sees a COUNT plus @@ -15,13 +15,23 @@ export interface CompactGitStatus { uncommittedFiles: number; changedFileCount: number; conflictsWithBase: boolean | null; + /** Where {@link base} came from. Declared rather than left riding the spread: it was + * reaching this wire already, typed nowhere and rendered nowhere, which is how a field + * ends up load-bearing by accident. `changedFiles` is dropped here deliberately; this is + * kept deliberately. */ + baseSource: BaseSource; error?: string; } /** One-line git summary shared by kild list and detail displays. */ export function formatCompactGitSummary(git?: CompactGitStatus): string { if (!git) return ''; - return ` · ${git.branch ?? '?'} +${git.ahead}/-${git.behind}${git.dirty ? ' dirty' : ''}${git.conflictsWithBase ? ' CONFLICTS' : ''}`; + // An ahead/behind count is only a fact about somebody's work if the base is. When nobody + // chose the base, the numbers still print — refusing to show them would be worse — but they + // print marked, so a reader knows what the comparison rests on. + const guessed = git.baseSource === 'explicit' || git.baseSource === 'configured'; + const against = guessed ? '' : ` vs ${git.base}?`; + return ` · ${git.branch ?? '?'} +${git.ahead}/-${git.behind}${against}${git.dirty ? ' dirty' : ''}${git.conflictsWithBase ? ' CONFLICTS' : ''}`; } /** One overlap: `kild` also changed `files`. The specific overlapping files ARE the diff --git a/engine/src/kild/memory.test.ts b/engine/src/kild/memory.test.ts index b4324661..fdf351fc 100644 --- a/engine/src/kild/memory.test.ts +++ b/engine/src/kild/memory.test.ts @@ -55,6 +55,7 @@ function kild(cwd: string, overrides: Partial = {}): Kild { to: ['agent'], text: 'Fix the auth bug', ts: 1, + seq: 1, }, { id: 'm1', @@ -63,6 +64,7 @@ function kild(cwd: string, overrides: Partial = {}): Kild { to: ['human'], text: 'second message', ts: 2, + seq: 2, }, ], worktree: 'fix-auth', diff --git a/engine/tsconfig.json b/engine/tsconfig.json index 6813f48c..03fc9cfc 100644 --- a/engine/tsconfig.json +++ b/engine/tsconfig.json @@ -16,8 +16,8 @@ "verbatimModuleSyntax": true, "forceConsistentCasingInFileNames": true, "noEmit": true, - "types": ["node"] + "types": ["node", "bun"] }, "include": ["src/**/*.ts"], - "exclude": ["dist", "node_modules", "src/**/*.test.ts"] + "exclude": ["dist", "node_modules"] }