feat(distill): agent-driven merge architecture - #13
Closed
cad0p wants to merge 68 commits into
Closed
Conversation
The two napkin kb tools did not register a `renderResult` callback, so `ToolExecutionComponent` fell back to `createResultFallback` which dumps the full text output unconditionally. Ctrl+O had nothing to toggle. Add a shared `formatKbResult` helper that truncates to N lines when `!expanded` and appends a "… (N more lines, Ctrl+O to expand)" hint, mirroring the built-in `read` / `grep` / `ls` / `find` / `bash` tools. Collapsed line counts: - kb_search: 15 lines (matches `grep`) - kb_read: 10 lines (matches `read`)
Remove vault-resolve.ts and use napkin-ai's findVault() directly, which now supports $XDG_CONFIG_HOME/napkin/config.json as a global vault config fallback. This eliminates duplicated vault resolution logic and ensures consistent behavior between the napkin CLI and pi-napkin extensions. - Delete extensions/vault-resolve.ts - napkin-context: use new Napkin(cwd) directly - distill: use new Napkin(cwd).vault.configPath for config access Requires napkin-ai >= 0.9.0 (with global config support).
- Update vault resolution section to reflect napkin-ai's native support - Add migration guide from ~/.pi/agent/napkin.json to ~/.config/napkin/config.json
…notes - Read _about.md files to understand folder purposes - Use relevant folder path when creating notes - Append to today's daily note in the relevant namespace - Minor punctuation cleanup
…nfig (#5) Make the distill model config optional. When omitted, the spawned pi subprocess will use whatever model pi resolves by default (the user's configured default), instead of always falling back to a hardcoded anthropic/claude-sonnet-4-6.
Guide the model on what _about.md files should look like — short folder descriptions explaining what kinds of notes belong there. Points the model at existing examples for style reference.
pnpm v10 blocks lifecycle scripts in dependencies by default unless they are listed in onlyBuiltDependencies. Because napkin-ai is a git-hosted dependency (github:cad0p/napkin), pnpm needs to run its 'prepare' script to build dist/ after fetch. Without the allowlist, pi fails to install pi-napkin with: ERR_PNPM_GIT_DEP_PREPARE_NOT_ALLOWED Failed to prepare git-hosted package ... napkin-ai@0.8.1 needs to execute build scripts but is not in the 'onlyBuiltDependencies' allowlist. Declaring napkin-ai in pnpm.onlyBuiltDependencies lets pnpm build it during install. npm users are unaffected (the 'pnpm' key is ignored).
…update README (#8) * docs(readme): point install snippet at the cad0p forks - Napkin link updated from Michaelliv/napkin to cad0p/napkin. - Install snippet updated: napkin from @cad0p scope with both pnpm and npm variants; pi-napkin installed via git from cad0p/pi-napkin rather than npm:pi-napkin, which resolves to the upstream package and would install the wrong fork. - Minor: drop the -ai suffix from prose references ("napkin's built-in vault resolution") since the fork publishes as @cad0p/napkin. * chore: depend on @cad0p/napkin from npm, drop pnpm build-script workaround Now that @cad0p/napkin is published to the npm registry, pi-napkin depends on it by name + semver range rather than as a git URL. The registry tarball ships `dist/` prebuilt, so there's no `prepare` lifecycle to run at install time and the pnpm.onlyBuiltDependencies workaround from #7 is no longer needed. Version range is `*` to match the other peer deps — @cad0p/napkin is under the same owner, so locking pi-napkin behind a range would just create busywork. npm's range matcher excludes prereleases from `*`, so calver builds from the `next` dist-tag don't accidentally get pulled — consumers stay on the stable `latest` until a base release. - package.json: "napkin-ai": "github:cad0p/napkin" → "@cad0p/napkin": "*" - package.json: drop pnpm.onlyBuiltDependencies (no longer applies) - package-lock.json: regenerated — resolves to the registry tarball - extensions/distill/index.ts: import updated to @cad0p/napkin - extensions/napkin-context/index.ts: same - skills/napkin/SKILL.md: install line now `pnpm add -g @cad0p/napkin` (or `npm install -g @cad0p/napkin`)
…re-vault footgun (#9) * docs(skill): document vault resolution and first-time setup The skill skipped over napkin's vault resolution logic, which contains a silent auto-creation footgun: running any vault-resolving command (e.g. `napkin vault`) from a directory with no ancestor `.napkin/` and no global `~/.config/napkin/config.json` makes napkin create a bare vault at cwd \u2014 `.napkin/` + `NAPKIN.md` + `.obsidian/` \u2014 with no prompt. This is easy to hit at first setup: an agent running `napkin vault` from the user's home or a random project directory litters that directory with empty-vault artifacts. Additions: - New "Vault Resolution" section near the top of the skill, listing the full 4-step resolution order (--vault flag -> ancestor .napkin -> global config -> bare-vault auto-creation), with an explicit warning callout about step 4. - "First-time setup" subsection showing the global config file format (`~/.config/napkin/config.json` with a `vault` field) and how to confirm resolution with `napkin vault --json | jq -r .path` before running commands that might trigger the fallback. - Cleanup instructions for when the bare-vault fallback has already fired: delete the stray `.napkin/`, `NAPKIN.md`, `.obsidian/`. - "Config" section rewritten to distinguish the two configs \u2014 per-vault (`<vault>/.napkin/config.json`, managed by `napkin config`) vs. global vault-path (`~/.config/napkin/config.json`, set manually). - `--vault <path>` flag description now cross-references the Vault Resolution section instead of giving a misleading one-line summary. * docs(skill): fix wrong `napkin version` invocation The skill listed `napkin version` as a subcommand, but the CLI exposes version as a flag `--version` (short: `-v`). Running `napkin version` returns "error: unknown command 'version'".
…10) * chore(lint): sort imports in napkin-context after @cad0p/napkin rename Biome's organizeImports flags the @cad0p/napkin import as out of order since PR #8 renamed the dependency from 'napkin-ai'. The old sort order placed 'napkin-ai' after @mariozechner/*, but the new @cad0p/* scope sorts first. Drive-by fix to unblock CI for downstream branches. * feat(distill): add /distill-auto-this-session to toggle auto-distill Adds a per-session toggle for the automatic distillation timer. State is persisted into the session file via a CustomEntry so that turning auto-distill off, quitting pi, and resuming the same session keeps it off. /distill-auto-this-session toggle on <-> off /distill-auto-this-session on turn auto-distill on /distill-auto-this-session off turn auto-distill off /distill-auto-this-session status report state and time to next run Only suppresses the scheduled timer. Manual /distill still works. Status bar shows 'distill: off (session)' when off for the session, deliberately distinct from 'distill: off' which means distill is disabled in the vault config. On resume (turn auto-distill on), the countdown resets so the next run respects the full interval instead of firing immediately. The in-flight poll loop owns the status bar while a distill runs -- renderIdleStatus short-circuits on isRunning to avoid flicker if the user turns auto-distill off mid-run. If distill is disabled in the vault config, the command warns that the session flag has no effect until vault-level distill is enabled. Transitions use 'info' severity since they're user-requested; 'warning' is reserved for the vault-disabled state and bad args. Persistence: - State is stored as a CustomEntry (type: "custom") with customType 'napkin-distill-session-state' and data { suppressed: bool }. CustomEntry is the documented pi mechanism for extension state persistence (session-format.md); unlike CustomMessageEntry it does not participate in LLM context. - session_start reads the latest matching entry on the current branch (via getBranch(), the canonical state-restoration pattern used by todo/summarize/handoff/qna examples) and restores the flag. Branch- scoped reads avoid leaking state across abandoned branches. - Schema is append-only: readers stop at the first matching customType and fall back to default on malformed data, so a newer writer is never silently shadowed by an older valid entry. - The handler only writes on real state changes, so idempotent calls don't bloat the session file. - Forked sessions inherit the state via SessionManager.forkFrom copying all entries; users can turn auto-distill back on in the fork. Session lifecycle: - autoDistillSuppressed is reset at the top of session_start so it doesn't leak across session switches that take early-return paths. - isRunning is reset in session_shutdown so an in-flight distill at shutdown doesn't stick 'true' across in-process session switches. - On session_start with reason resume/fork/startup, an off session emits a one-time notify so the user doesn't miss the status bar after a long gap.
) * chore(test): add bun test infrastructure pi-napkin had zero tests. Add bun test (matches @cad0p/napkin's stack) so upcoming distill work (shouldDistillOnShutdown predicate, worktree lifecycle) has a place to live. Also migrate package manager from npm to pnpm per repo steering: - Drop package-lock.json in favor of pnpm-lock.yaml - Update CI + release workflows to use pnpm + setup-bun for tests The trivial smoke test in extensions/distill/index.test.ts just wires up bun test. Real distill tests land in later phase-a commits. phase-a * feat(distill): add onShutdown config field (default true) Add `distill.onShutdown: boolean` to DistillConfig with default `true`. Export DistillConfig, VaultConfig, DEFAULT_DISTILL, and loadVaultConfig so config parsing is unit-testable from extensions/distill/index.test.ts. Schema + default only \u2014 nothing reads the field yet. Phase B wires it into the shutdown handler via shouldDistillOnShutdown. Tests cover: - Missing config.json \u2192 defaults - Empty distill block \u2192 onShutdown=true (default wins) - Explicit opt-out (onShutdown=false) - Missing onShutdown \u2192 default=true - Malformed values (null, string, 0) \u2192 !== false \u2192 still runs - Malformed JSON \u2192 full default fallback - Other distill fields preserved when setting onShutdown phase-a * feat(distill): add shouldDistillOnShutdown predicate Pure predicate that decides whether to spawn a final distill at session_shutdown. No I/O \u2014 caller assembles all inputs. Ready for the phase B shutdown handler to consume. Guard order matches the spec (NAPKIN_DISTILL_NO_RECURSE, reload, session suppression, enabled, onShutdown opt-out, sessionFile, size=0, lastSpawnedSize, lastSessionSize). The lastSpawnedSize guard dedupes the "shutdown right after interval fire" race without blocking the "shutdown with new content since last spawn" case. Test coverage: one test per guard (proves isolation), short-circuit order tests (proves guard order), and integration scenarios (typical /quit, /reload, just-spawned dedup, fresh session with no content, disabled config, per-vault opt-out, per-session suppression). Known deviation: pi's SessionShutdownEvent has no `reason` field in 0.67.1 (re-checked in node_modules). The predicate accepts an optional `reason` on a local `ShutdownDistillEvent` interface \u2014 phase B will have to detect reload externally (likely by tracking session_start with reason=reload) and pass it in. Flagged for reviewer. phase-a * feat(distill): track lastSpawnedSize for shutdown dedup Record the session file's byte size at the moment of a successful distill spawn \u2014 set BEFORE the completion poll starts. Separate from `lastSessionSize` (which updates on completion) so that a shutdown firing between spawn-and-complete can dedupe against the in-flight distill without waiting for it to finish. Scaffolding only: written in runDistill, not yet read. Phase B wires it into the shutdown handler via shouldDistillOnShutdown guard 8. No unit test \u2014 runDistill isn't exported and has substantial side effects (fork session, spawn subprocess). Testing in isolation needs heavy mocking; phase B integration tests cover the write-then-read path end to end. Per spec allowance. `biome-ignore noUnusedVariables` documents the temporary state explicitly \u2014 phase B removes the suppression when it adds the read. phase-a * test(distill): widen DEFAULTS types so Partial overrides compile Literal-inferred types (`reason: undefined`, `enabled: true`) in the shared DEFAULTS object made `call({ event: { reason: "reload" } })` and similar override overrides fail strict tsc ("Type 'string' is not assignable to type 'undefined'"). bun test runs them fine at runtime \u2014 this is purely a static-typing issue \u2014 but any future tsc-noEmit check (or editor type-check) would flag them. Widen DEFAULTS to an explicit `Inputs` interface matching the predicate's parameter shapes. No behavior change; all 51 tests still green. phase-a * chore(deps): bump pi peer dep to >=0.68.0 for session_shutdown.reason Phase A's `ShutdownDistillEvent` shim worked around pi 0.67.1 lacking `reason` on `SessionShutdownEvent`. The field was added in pi 0.68.0 (2026-04-20). Bump the peer dep floor to `>=0.68.0` and use pi's native type directly. - package.json: peer `@mariozechner/pi-coding-agent` becomes ">=0.68.0" - pnpm-lock.yaml: refreshed (pulls 0.73.1 currently) - should-distill-on-shutdown.ts: drop local `ShutdownDistillEvent`, accept `Pick<SessionShutdownEvent, "reason">` for caller flexibility - should-distill-on-shutdown.test.ts: update test inputs to canonical reason literals; drop "undefined reason" case (impossible post-0.68) 51 \u2192 50 tests (one merged loop no longer tests impossible values). pnpm install --frozen-lockfile, pnpm lint, pnpm test all green. phase-b * feat(distill): add distill-workspace module for worktree layout Introduce `extensions/distill/distill-workspace.ts` \u2014 the module that manages the on-disk layout used by per-distill isolated workspaces. Milestone 2 of Phase B: no git involvement yet, just the directory layout, session fork, and meta.json. Exports: - `createDistillWorkspace(vault, sourceSessionFile)` \u2014 mkdtemp root, fork session into `<wt>/.napkin/distill/session.jsonl`, write meta - `cleanupDistillWorkspace(worktreePath)` \u2014 idempotent rm -rf - `readDistillMeta(worktreePath)` \u2014 parse meta.json or null on any failure (missing, malformed, incomplete schema) - `generateDistillBranchName(now?, nonceHex?)` \u2014 `distill/<hex6>-<epoch>` - `DistillMeta` interface (pid, vault, branch, startedAt, parentSession) Key implementation choices documented inline: - Branch name carries a 24-bit random nonce so two distills firing in the same second never collide (timestamp-only is a latent race). - `readDistillMeta` never throws on I/O absence; only on unexpected system errors. Callers treat null as "stale or gone". - Tmp dir tag is `napkin-distill-<branch-suffix>-<random>` so orphans are traceable. - Milestone 3 will replace the tmp-dir with a real git worktree; the public API stays. Adds `distill-workspace.test.ts` (16 tests): - branch name format + nonce collision-free at scale - workspace layout, meta.json schema - two workspaces from the same source are independent - throws on missing source, cleans up tmp dir on fork failure - cleanup is idempotent on missing paths - readDistillMeta null-paths for missing \/ malformed \/ partial metadata 51 \u2192 66 tests. pnpm lint, pnpm test, pnpm install --frozen-lockfile all green. phase-b * feat(distill): create per-distill git worktree Milestone 3 of Phase B. Replace the tmp-dir placeholder from milestone 2 with a real git worktree rooted at the vault's HEAD. Each distill now gets its own branch (`distill/<hex6>-<epoch>`) and isolated working tree under `<vault>/.napkin/distill-worktrees/<branch-suffix>/`. New exports in distill-workspace.ts: - `createDistillWorktree(vault, branch, path)` \u2014 `git worktree add -b ...` with vault-has-git-repo precondition (throws DistillError otherwise) - `removeDistillWorktree(vault, path, branch)` \u2014 worktree-first, then branch; prunes stale entries on failure; no-ops if vault gone - `DistillError` class \u2014 distinguishable from stdlib errors - `DISTILL_WORKTREES_SUBDIR` constant (`.napkin/distill-worktrees`) `createDistillWorkspace` now: - Chooses a unique branch name and path under the vault - Calls `createDistillWorktree` before forking the session - Rolls back (`removeDistillWorktree`) if the session fork / meta write fails, so failures never leak a worktree or branch - Signature unchanged: still `(vault, sourceSessionFile) \u2192 handle` `cleanupDistillWorkspace` signature shifts to `(vault, workspace)` because cleanup needs both the vault and the branch name (the branch name only lives on the returned handle, not the path). No callers yet (added in milestone 5), so the API shift is free. Tests: adds per-worktree git setup (init, seed commit) and covers: - worktree add / remove happy path - vault-without-.git throws DistillError - branch-collision throws DistillError - workspace rollback on fork failure (no leaked branch) - cleanup idempotency on missing vault / missing paths - worktree path lives under the vault (important for napkin cwd \u2192 vault resolution in the distill subprocess) 66 \u2192 73 tests. pnpm lint + pnpm test + pnpm install --frozen-lockfile all green. phase-b * feat(distill): add LLM merge driver and git_retry wrapper Milestone 4 of Phase B. Ship the two shell-layer pieces that make worktree-based auto-distill safe: `extensions/distill/scripts/napkin-distill-merge` \u2014 LLM-powered git merge driver. Registered per-worktree via `git config --local`: merge.napkin-distill-merge.name = "napkin distill LLM merge driver" merge.napkin-distill-merge.driver = "<abs-path> %O %A %B %P" and activated by a `*.md merge=napkin-distill-merge` line in `.gitattributes` (appended by `registerMergeDriver` when missing). Driver contract: - Reads base (%O), ours (%A), theirs (%B), filename (%P) - Calls `pi -p <prompt>` (prompt preserves both sides, asks for de-dup and frontmatter validity) - Sanity-checks the output: non-empty, length in [0.3, 3.0]x max input, frontmatter-at-top if all three inputs had frontmatter (syntactic) - 3 strikes on any failure (pi exit !=0, empty, sanity fail) - Exit 0: write resolved content to %A - Exit 1: leave %A unmodified (caller's partial-merge salvage handles) - Reusable test mocks via `NAPKIN_DISTILL_MERGE_MOCK` env var `extensions/distill/scripts/git_retry.sh` \u2014 bash function that retries main-mutating git commands on transient index.lock contention: - Up to NAPKIN_GIT_RETRY_MAX (default 5) attempts - Linear backoff base*attempt (default 0.5s; env-overridable) - Max ~5s total wait \u2014 good for transient locks, tiny blast radius for real failures (exits fast after last attempt) - Uses `awk` for float arithmetic (avoids bc dependency) Integration with distill-workspace.ts: - Add `MERGE_DRIVER_SCRIPT` import via new `scripts-paths.ts` (module that resolves script locations via `import.meta.url`, cwd-independent) - `createDistillWorktree` now calls `registerMergeDriver` after creating the worktree, rolls back the worktree + branch on failure (unregistered driver would defeat the whole merge design) - `.gitattributes` append is idempotent: if the line is already present (future: Phase C's auto-init scaffolds it), no change Tests (`scripts.test.ts`, 12 new tests): - napkin-distill-merge: ok, fail, empty, tiny, huge, no-fm, ok-after-2 (3rd-attempt success), ok-after-3 (3-strike give-up) - git_retry: happy path, always-fail hits retry cap, eventual success, REAL index.lock contention with a backgrounded releaser 73 \u2192 85 tests. pnpm lint + pnpm test + pnpm install --frozen-lockfile all green. phase-b * feat(distill): implement worktree-based auto-distill spawn Adds `spawnDistillInWorktree` \u2014 a detached spawn that performs the full auto-distill lifecycle inside a per-distill git worktree so concurrent distills (interval, shutdown, or multiple pi sessions) don't race on vault files. Shipped: - `extensions/distill/scripts/distill-wrapper.sh`: bash orchestrator that runs pi in the worktree, commits, merges main back, squash-merges to main from the vault cwd, and cleans up via EXIT trap. Takes `<vault> <worktree> <branch> <sessionFork> <prompt> <errorDir> [<model>]`. Writes fatal failures to `<vault.configPath>/distill/errors/<ts>-<pid>-<branch>.log` and always completes cleanup regardless of failure path. - `spawnDistillInWorktree({ vault, sessionFile, prompt, model?, spawnFn? })` in `distill-workspace.ts`: creates the workspace, resolves the vault error dir via Napkin, and spawns the wrapper detached with `NAPKIN_DISTILL_NO_RECURSE=1`. Parent returns synchronously with `{ workspace, pid }` and MUST NOT wait. - `DISTILL_WRAPPER_SCRIPT` export in `scripts-paths.ts` so callers and tests resolve the script path via `import.meta.url` (test cwd agnostic). Why: - Existing `spawnDistill` (temp-dir based) isn't safe under concurrency. The worktree design isolates each distill on its own branch so git's own `.git/index.lock` + the LLM merge driver + git_retry handle interleaved main-HEAD writes. See Phase B Item 5 in the shutdown-distill spec. - Kept `spawnDistill` untouched: the manual `/distill` command still uses it (git-optional), and future work (Phase C) can decide when to migrate it. - Error dir lives on the MAIN vault (resolved via `Napkin(vault).vault.configPath`) because worktrees are removed on cleanup, which would otherwise lose the logs. Self-healing scaffolding (temporary until Phase C's auto-init): - The wrapper's `git add -A` excludes `.napkin/distill/` (session fork + meta.json) and `.napkin/distill-worktrees/` (sibling-worktree pool) via pathspec so they never enter the vault's git history. Phase C replaces this with a committed `.gitignore`. Tests (+9, 85 \u2192 94): - Unit: mock `spawn` to verify positional args, detached flags, env, cwd, errorDir creation, return value shape. - Integration: real wrapper against a tmp git repo with pi stubbed via `NAPKIN_DISTILL_SKIP_PI=1`. Covers happy path (squash commit lands on main), empty distill (no commit, clean exit), two concurrent workspaces (both complete without interference), and missing-arg error (exit 2). Next: Item 6 (partial-merge salvage integration test) and Item 7 (route interval distill through worktree). * feat(distill): partial-merge salvage keeps clean files, reverts conflicted ones to main Adds integration-test coverage for the partial-merge salvage path that already lives in distill-wrapper.sh (shipped with Item 5). The salvage code runs after `git merge main` completes: any file still unmerged \u2014 meaning the LLM merge driver 3-struck on it \u2014 is reverted to main's version via `git checkout main -- <file>`, logged, and staged so the merge commit can complete with partial content. Tests (+2, 94 \u2192 96): - "clean file keeps distill's content, conflicted file reverts to main": Seeds main with `clean.md` + `conflict.md`, creates a workspace, then diverges main's `conflict.md`. Distill mutates both. With `NAPKIN_DISTILL_MERGE_MOCK=fail` forcing 3-strikes, asserts that the squash commit on main keeps main's `conflict.md` content and gets distill's `clean.md` content, and that the error log names the conflicted file + "partial-merge" reason. - "all files conflict: salvage reverts everything, no squash commit created": Verifies the empty-squash guard (Item 5's `git diff --cached --quiet` check on main before commit) survives the salvage path \u2014 when every file gets salvaged back to main's version, the squash has no net change and we correctly skip the commit. Why this matters: - LLM driver failures are a normal operating condition (the spec specifies 3 strikes per file). Without salvage, a single bad merge would abort the whole distill and discard all its work. With salvage, the vault gets the work that merged cleanly and loses only the specific files the LLM couldn't handle. - Error-log semantics: each reverted file gets its own line in the log so the forensic trail is file-granular, not distill-granular. Combined with the dangling-SHA log entry (Item 5), a user can `git cat-file -p <sha>` to recover the distill's version of any salvaged file within the git gc grace window (default 2 weeks). Wrapper behavior confirmed by tests: - Salvage loop runs `git checkout main -- <f>` + `git add -- <f>` per unmerged file, then `git commit --no-edit` to finalize the merge. - MERGE_HEAD check (post-salvage) catches the driver-wrote-conflict-markers edge case and bails with a forensic log. - EXIT trap always cleans up worktree + branch regardless of salvage outcome. * feat(distill): route interval distill through worktree spawn Adds `runAutoDistill(ctx)` alongside the existing `runDistill` (untouched) and switches the interval timer to use it. Manual `/distill` command still calls `runDistill` so vaults without git remain supported. `runAutoDistill`: - Pre-flight check: vault must have `.git/`. Missing git surfaces a one-line status hint ("distill: needs git") and returns without throwing. Phase C's auto-init will ensure git is always present. - Spawns via `spawnDistillInWorktree` (Item 5) \u2014 each call creates an isolated git worktree on a fresh `distill/<hex>-<epoch>` branch. - Polls `workspace.worktreePath` for disappearance as the completion signal: the wrapper's EXIT trap removes the worktree on any exit path. - Bookkeeping mirrors `runDistill` exactly: `lastSpawnedSize` on successful spawn (pre-poll dedup for shutdown) `isRunning` while the worktree exists `lastSessionSize` on completion (post-completion dedup) `lastDistillTimestamp` on completion or no-op skip - On 10-min timeout, calls `cleanupDistillWorkspace` to force-remove the stalled worktree + branch. - Catches `DistillError` from the workspace layer separately from generic spawn failures to surface clearer status-bar text. Imports from distill-workspace: `cleanupDistillWorkspace`, `DistillError`, type `DistillWorkspace`, `spawnDistillInWorktree`. Interval wiring (session_start): before: intervalHandle = setInterval(() => runDistill(ctx), intervalMs) after: intervalHandle = setInterval(() => runAutoDistill(ctx), intervalMs) Manual `/distill` command wiring unchanged \u2014 still calls `runDistill`. Tests (+2, 96 \u2192 98): - `runAutoDistill vs runDistill routing (Item 7) > interval callback creates a worktree`: mocks the ExtensionAPI, stubs globalThis.setInterval to capture the interval callback, invokes it synchronously, and asserts `.napkin/distill-worktrees/<hex>-<epoch>` was created. - `/distill command creates a tmp dir, NOT a worktree`: invokes the registered `/distill` command handler, asserts a `napkin-distill-*` tmp dir appeared under `os.tmpdir()` but no worktree was created under the vault. The test ExtensionAPI mock is intentionally minimal \u2014 only `on()` and `registerCommand()` are exercised by the extension during session_start and command invocation. Other methods are stubbed with no-ops. Race-free observation: both `runAutoDistill` and `runDistill` create their respective artifacts (worktree / tmpDir) synchronously before returning the spawned subprocess handle to the caller. The detached wrapper's cleanup trap runs later, so post-return assertions are stable. * feat(distill): wire shutdown handler to spawn auto-distill in worktree Completes Phase B. The session_shutdown handler now consults `shouldDistillOnShutdown` (Phase A predicate) and spawns a final auto-distill via `spawnDistillInWorktree` (Item 5) when all guards pass, capturing any session work added since the last interval distill \u2014 or all of it, for short sessions that finish before the first interval fires. Handler flow (after existing timer/isRunning cleanup, before uiRef reset): 1. Resolve vault via Napkin(ctx.cwd). If no vault, bail. 2. loadVaultConfig to get current distill config (not captured from session_start \u2014 cost of one JSON parse at shutdown is negligible). 3. statSize the current session file. 4. Call shouldDistillOnShutdown with all assembled inputs. 5. If predicate true AND vault has .git/ AND sessionFile exists: spawnDistillInWorktree({ vault, sessionFile, prompt: DISTILL_PROMPT, model: modelStr }). Set lastSpawnedSize = currentSize so any re-entry within the same closure is a no-op (currentSize === lastSpawnedSize guard). 6. Outer try/catch swallows ALL errors and falls through to the final uiRef reset. Shutdown MUST NEVER block; failures log to stderr only. Guards deferred to shouldDistillOnShutdown: NAPKIN_DISTILL_NO_RECURSE (recursion inside distill subprocess) event.reason === "reload" (session continuing) autoDistillSuppressed (per-session /distill-auto-this-session off) !config.enabled (master switch) config.onShutdown === false (per-vault opt-out) !sessionFile (ephemeral session) currentSize === 0 (nothing happened) currentSize === lastSpawnedSize (interval just grabbed this content) currentSize === lastSessionSize (previous distill completed on it) Additional guards in the handler itself: .git/ must exist in vault \u2014 Phase B doesn't auto-init (Phase C will). Silent skip rather than UI hint because shutdown has no UI to surface to. Also: dropped the temporary biome-ignore on `lastSpawnedSize` now that it's genuinely read in the shutdown handler (was a Phase A breadcrumb). Tests (+9, 98 \u2192 107): - spawns on normal exit with enabled config + git vault + content - does NOT spawn when shutdown reason is 'reload' - does NOT spawn when config.onShutdown=false - does NOT spawn when config.enabled=false - does NOT spawn when session file is empty (size === 0) - does NOT spawn when vault is not a git repo (needs-git guard) - does NOT spawn when NAPKIN_DISTILL_NO_RECURSE is set - shutdown never blocks when vault resolution fails (catches throw) - sets lastSpawnedSize so a re-entry on same content is a no-op Tests observe filesystem state (count of `.napkin/distill-worktrees/` entries) as a proxy for "spawnDistillInWorktree was invoked". The workspace is created synchronously before detached exec, so there's no race between the handler return and the assertion. Phase B exit criteria met: - 107 tests pass (85 before Phase B Item 5) - pnpm lint clean - pnpm install --frozen-lockfile clean - 4 new commits (Items 5\u20138), each SSH-signed, Conventional Commits - Each commit builds + tests green between * fix(test): clear NAPKIN_DISTILL_NO_RECURSE in test beforeEach Tests run inside a distill subprocess (env var set to 1), causing all 'should return true' assertions to fail via guard 1 short-circuit. Add top-level beforeEach/afterEach to each affected describe block to clear and restore the env var around each test. * chore(lint): fix biome formatting in test files Pre-existing formatting issues from fb51fb5 that biome format surfaced. No behavior change. * feat(distill): cleanup stale distill worktrees at session_start Add cleanupStaleWorktrees() to distill-workspace.ts: scans 'git worktree list --porcelain', filters to branches under 'distill/', and removes any worktree whose meta.json is missing, whose pid is dead, or whose mtime is older than 60 minutes. Best-effort — failures on one worktree don't abort the sweep. Wire the helper into session_start's distill-enabled branch, before the per-session pause state is restored. Runs inside try/catch so any internal failure never blocks session startup. Exposes parseWorktreeList, STALE_META_AGE_MS, and a minimal StaleCleanupVault interface for testability. Tests (distill-workspace.test.ts): parseWorktreeList covers porcelain format, detached-HEAD skip, missing trailing newline; cleanupStaleWorktrees covers live-worktree preservation, dead-pid removal, missing-meta removal, stale-mtime removal, non-distill-branch preservation, and mixed-state sweep. 10 new tests (117 total, up from 107). * feat(distill): auto-init git + scaffold .gitignore/.gitattributes for auto-distill New module extensions/distill/auto-setup.ts with ensureVaultReadyForAutoDistill({ contentPath }): SetupResult and a small countTrackedFiles helper used by the first-run notify. Contract: - Idempotent: re-running on a fully-set-up vault returns { initialized: false, scaffolded: [] } with no new commits. - Non-destructive: .gitignore / .gitattributes merges only append lines not already present; user content is preserved verbatim. - Fail-soft: every git / fs failure is translated into SetupResult.error instead of throwing, so callers can notify and keep the session alive. Wiring in index.ts's session_start (distill-enabled branch only, before the existing cleanupStaleWorktrees sweep): - On error: notify with the failure + disable auto-distill for the session (in-memory; no persisted state). - On fresh init: multi-line notify with tracked file count + undo hint (rm -rf .git/) + opt-out hint (distill.enabled: false). - On scaffold-only: single-line notify listing the files added. Tests (auto-setup.test.ts): fresh-vault init+commit, existing-repo scaffold commit, idempotent second run, .gitignore user-content preservation, preseeded .gitignore \u2192 only .gitattributes written, partial-setup fill-in, chmod-based init failure (skipped under root), and countTrackedFiles coverage. 9 new tests (126 total, up from 117). * feat(distill): teach auto-distill about supersedes: frontmatter convention Append a short frontmatter instruction to DISTILL_PROMPT so that when a distill run creates a note that replaces an older one, it records the relationship as: supersedes: ["path/to/old/note.md"] A future janitor (not part of this phase) will use that field to archive superseded notes. Standalone notes leave the field empty or omit it. No behavior change beyond the prompt text. No test \u2014 prompt-only change. * refactor(distill): simplify wrapper git add now that .gitignore excludes distill state Phase B's distill-wrapper.sh used pathspec excludes (:(exclude).napkin/distill, etc.) on git add -A as belt-and-braces, because at that point the vault's .gitignore didn't yet cover auto-distill's ephemeral state. Phase C's auto-setup.ts now installs those rules into the vault's .gitignore at session_start, so the wrapper can drop the pathspec form. Replace the multi-line 'git add -A -- :(exclude)...' with a plain 'git add -A' and update the surrounding comment to point at the new source of truth. Test fixture (spawn-distill-in-worktree.test.ts) pre-seeds a minimal .gitignore that mirrors what auto-setup installs so the integration wrapper tests reproduce the production invariant. Existing 11 wrapper tests still pass unchanged. * fix(distill): auto-setup targets ctx.cwd (matches worktree spawn root) spawnDistillInWorktree and cleanupStaleWorktrees both operate on `ctx.cwd` as the git root. 790314e's auto-setup called into napkin's resolved `contentPath`, which can diverge from `ctx.cwd` on legacy bare vaults where Napkin resolves `contentPath` to the `.napkin/` subdir. The mismatch meant auto-setup would `git init` inside `.napkin/` while worktree code looked for git at the vault root, silently breaking auto-distill on those layouts. Realign auto-setup, countTrackedFiles, and cleanupStaleWorktrees calls to `ctx.cwd`. Update the first-run notify's 'To undo' path so the command it prints actually matches where the repo got created. Shutdown-handler test 'does NOT spawn when vault is not a git repo' becomes 'auto-inits git at session_start' \u2014 Phase C1's auto-init makes the needs-git guard dead in the happy path. The new assertion verifies .git was created at the vault root AND a worktree was spawned (= the full auto-distill chain works end-to-end on a bare vault). * refactor(distill): extract STALE_WORKTREE_MINUTES constant Previously the 60-minute stale-worktree threshold was encoded inline as `60 * 60 * 1000`. Lift to a named `STALE_WORKTREE_MINUTES = 60` so the value has a single source of truth callable from tests and documented in one place (the /60 minute/ margin above MAX_DISTILL_DURATION_MS). STALE_META_AGE_MS is kept as a separate export since existing callers already import it by that name. * feat(distill): add /distill-status slash command Exposes the state of active distill worktrees and unmerged distill branches as a slash command. Humans can now answer 'what's going on with auto-distill?' without grepping git worktree list and reading meta.json by hand. The core state lives in new helpers on distill-workspace: - getActiveDistills(vault): scans `git worktree list` for branches matching `distill/*`, reads each worktree's meta.json, reports pid liveness + elapsed time + start SHA. - getUnmergedDistillBranches(vault): surfaces distill branches that exist as refs but have no active worktree (crashed distill breadcrumbs pending gc). createDistillWorkspace now records the vault's HEAD SHA at worktree-creation time in meta.json so downstream consumers (the before_agent_start overlap detector, next commit) can diff exactly what the distill has written since it forked. The /distill-status command formats the result as plain text, routing through ctx.ui.notify when a UI is present and falling back to console.log in headless mode (pi -p /distill-status ...). * feat(distill): add napkin_distill_status pi tool for agent visibility Exposes the same state as /distill-status (active distill worktrees + unmerged distill branches) through a pi tool so the LLM can pull-query before making vault edits. Tool returns compact JSON: { active: [{ pid, branch, elapsedSeconds, session, alive, ... }], unmerged: ["distill/xyz-456", ...] } Shape matches what the human formatter renders: basename of sessionPath, floored elapsed seconds, explicit alive flag. Output is small (tens of tokens in typical cases) and stable \u2014 new keys may be added, existing keys MUST NOT be renamed or repurposed. When the agent is outside any napkin vault, the tool returns { error: "no vault in cwd" } rather than throwing, so an errant call doesn't abort the turn. Serialiser (distillStatusToJson) and formatter (formatDistillStatus) are exported from index.ts as pure functions and unit-tested directly (distill-status.test.ts) without needing a full ExtensionAPI mock. * feat(distill): inject concurrent-distill notice via before_agent_start When the current pi session has written to vault files that a background auto-distill is also editing, append a one-line notice to the per-turn system prompt so the agent knows its recent writes may be clobbered or merged when the distill completes. Per-turn and ephemeral: uses before_agent_start's 'systemPrompt' return rather than 'message' injection, so 0 tokens are spent when there's no overlap, and nothing is persisted to the session file. Implementation: - session-touched-files.ts: reimplements pi's internal extractFileOpsFromMessage (dist/core/compaction/utils.js) to walk assistant messages and collect paths from 'write' / 'edit' tool calls, plus a conservative bash-redirection heuristic (>, >>, tee) for agents that shell out for writes. Not exported from pi's public API, so we reimplement + pin via version-check test. - intersectFiles / formatOverlapNotice in index.ts: pure functions, unit-tested, handle absolute-vs-relative path mismatches via suffix-match. - before_agent_start handler: env-guarded (skips inside distill subprocess), wraps every step in try/catch so failure collapses to 'no overlap detected' — must never block the agent turn. - diffWorktreeSinceStart: drives the distill-side of the intersection. Uses the startSha recorded in meta.json (Phase C2+) to diff exactly what the distill has committed; falls back to 'git status --porcelain' for legacy meta.json files written before startSha was introduced. Tests: 78 new tests across session-touched-files.test.ts, session-touched-files.version-check.test.ts, overlap-injection.test.ts, and diffWorktreeSinceStart tests in distill-workspace.test.ts. The version-check test asserts pi's internal extractFileOpsFromMessage still exists at the expected path, so upstream breakage surfaces as a test failure instead of silent drift. * docs: invert README/SKILL.md \u2014 README is now the comprehensive doc Before: SKILL.md was 811 lines of comprehensive napkin CLI reference, README.md was 75 lines of install + tiny doc stub. Humans found the README first and got almost nothing; agents found SKILL.md via its frontmatter description and got too much (the CLI reference mixed with pi-napkin setup). After: README.md is the comprehensive doc (249 lines) covering the whole pi-napkin surface \u2014 install, extensions, auto-distill config, concurrency (worktree + LLM merge driver + partial-merge salvage), commands (/distill, /distill-auto-this-session, /distill-status), agent tool (napkin_distill_status), agent visibility (before_agent_start overlap injection), vault setup, troubleshooting, and a pointer to the builder-deleter design. SKILL.md shrinks to 44 lines: frontmatter (needed for pi skill discovery), a 2-sentence preamble, a pointer at README, and the vault resolution section retained verbatim. Vault resolution is the one piece of operational info that MUST stay at the skill level \u2014 misresolution silently creates a bare vault (data-loss hazard). For napkin CLI reference, agents now use `napkin --help` / `napkin <cmd> --help` at runtime. Removing the duplicated reference from SKILL.md prevents drift between the skill doc and napkin's actual CLI surface. Concurrency doc includes the partial-merge salvage behavior per spec Item 20 ("Document partial-merge behavior in README"): when the LLM merge driver fails on some files, those files fall back to main's version while the cleanly-merged files keep distill's content. Error logs land in <vault>/.napkin/distill/errors/ with the dangling distill commit SHA for git-gc-grace recovery. * test(distill): use real pi session_shutdown reason values (G3+C5) Shutdown-handler tests used 'exit'|'switch'|'error' for reason, but pi's SessionShutdownEvent.reason union is 'quit'|'reload'|'new'|'resume'|'fork'. Tests passed by accident (guard 2 only short-circuits on 'reload', other values fall through) rather than by exercising real upstream contract. Replace all fabricated reasons with real ones: - 'exit' -> 'quit' (canonical normal-exit case, 8 call sites) - widen the runLifecycle type to the full union - add explicit 'fork' test so the second non-'reload' branch has coverage Addresses coverage-review G3 and correctness-review C5. * test(distill): add end-to-end wrapper test with real-driver conflict resolution (G1) The previous wrapper tests covered happy-path (disjoint files, driver never fires), concurrent worktrees (disjoint files, sequential), and 3-strike salvage (driver always fails). None exercised the core story: driver fires during 'git merge main', resolves the conflict, and its output survives the squash-merge to main. Add a new describe block with two integration tests: 1. LLM-resolved conflict: driver output lands on main - Seeds a baseline, creates worktree, mutates main to force divergence, stages distill's divergent version, runs wrapper with NAPKIN_DISTILL_MERGE_MOCK=ok so the driver concatenates ours+theirs - Asserts the resolved content contains BOTH sides' markers on main (proves driver fired AND its output reached main) - Asserts no error log entries 2. Driver retries: ok-after-2 succeeds on attempt 3 - Verifies the 3-strike retry loop bridges transient failures Addresses coverage-review G1 (BLOCK). * fix(distill): preserve setup-error safety flag after persistence restore (C1) The session_start handler set 'autoDistillSuppressed = true' inside the setup-failure branch (line 316) then ~40 lines later unconditionally overwrote it with 'readPersistedSuppressed(...)' (line 356). For fresh sessions the persisted value is 'false', so the force-suppression was erased before the interval timer armed. Net effect: when 'git init' fails (readonly FS, permission denied, etc.) the user saw the error notify AND auto-distill still tried to run on every interval tick. Fix: track setup outcome in a 'setupFailed' local, and only re-read the persisted state when setup succeeded. When setup failed, force-suppress regardless \u2014 the issue is vault-level, not user intent, and the next session will re-try setup. Add shutdown-handler test that: - Simulates setup failure by placing a directory where '.gitignore' should be a file (mergeLines hits EISDIR) - Asserts no shutdown distill spawns - The previous implementation would have let persisted=false override the safety flag and spawned. Addresses correctness-review C1. * fix(distill): record wrapper pid in meta.json, not parent pi session pid (C2) createDistillWorkspace() wrote meta.json before the wrapper had been spawned, so 'pid' was the parent pi session's pid \u2014 not the wrapper's. Consequences: - /distill-status.alive reflected whether the parent pi was still running, not whether the distill subprocess was alive. A crashed distill showed alive=true as long as the parent lived. - cleanupStaleWorktrees uses isPidAlive(meta.pid) as its primary signal; with a long-lived orchestrator (cr-auto-action spawns many sub-distills from one pi) dead distill worktrees could accumulate for up to 60 min until the mtime fallback kicked in. Fix: the wrapper rewrites meta.json's pid field to its own pid ($$) immediately after installing its cleanup trap, before any work. The JS side's write becomes a pre-spawn placeholder; within ~100ms of spawn the wrapper's own pid lands in meta.json. sed+mv gives atomicity on the same filesystem. Add NAPKIN_DISTILL_HALT_AFTER_META=1 testing hook so integration tests can inspect the updated meta.json before the cleanup trap wipes it. Test: wrapper rewrites meta.json pid to its own pid (C2). - Creates workspace (meta.pid = process.pid) - Runs wrapper with HALT_AFTER_META - Asserts meta.pid is a different positive number, other meta fields unchanged. Update DistillMeta.pid JSDoc to document the pre-spawn-placeholder + wrapper-rewrite sequence. Addresses correctness-review C2. * fix(distill): detect and respect vault default branch instead of hardcoding main (C3) The wrapper hardcoded 'main' in three places: git merge --no-edit main, git checkout main -- <f>, and implicitly in the squash step (squashes into whatever is checked out in VAULT). Two failure modes: 1. Vaults with 'master' as default (older git, init.defaultBranch=master in global config): every 'git merge main' fails with 'not something we can merge' \u2014 salvage block finds nothing, MERGE_HEAD check passes, squash runs against a distill branch never merged with mainline. 2. Main vault with a feature branch checked out at shutdown: squash lands distill commits on the user's feature branch, corrupting history. Fix JS side: add detectDefaultBranch(vault) helper using 'git symbolic-ref refs/remotes/origin/HEAD' (conventional), falling back to current branch via 'git symbolic-ref --short HEAD', then 'main'. Pass the detected value as the 8th argv to the wrapper (optional, defaults to 'main' to preserve backward compat for direct wrapper invocations). Fix wrapper: accept $8 as DEFAULT_BRANCH, use it for merge + salvage checkout. Add a defensive HEAD check before squash \u2014 if vault HEAD isn't on DEFAULT_BRANCH, refuse to squash and log to error dir. Tests: - unit: wrapper spawn call has 'main' at argv[8] for a main-default vault. - integration: detectDefaultBranch returns 'master' for a master-default vault; wrapper squash-merges into master when invoked with argv[8]=master. Addresses correctness-review C3. * refactor(distill): migrate legacy spawnDistill to argv-based spawn (SEC-1) The legacy `spawnDistill` built a shell command string and ran it via spawn('sh', ['-c', cmd]). Today's inputs are safe (constant prompt, shellEscape applied), but the shape was a regression-magnet: any future caller that swapped DISTILL_PROMPT for a user-constructed string, or funneled config.model.id through without shell-escaping, would produce a shell-injection hazard that's easy to miss in review. Extract the 'pi ... >/dev/null 2>&1; rm -rf <tmp>' shell body into scripts/distill-wrapper-legacy.sh and spawn it as spawn('sh', [script, sessionFile, tmpDir, prompt, model]). Every variable is a positional argv entry \u2014 no interpolation, no escaping, no injection surface. Matches the argv style already used by the worktree spawn path. Export spawnDistill (was a module-local function) and accept an optional spawnFn override to support unit testing. Legacy path remains git-optional; behaviour preserved \u2014 manual `/distill` in a non-git vault still works. Tests (spawn-distill-legacy.test.ts): - No "-c" argv - positional argv order: [script, sessionFile, tmpDir, prompt, model] - empty model -> empty string at argv[4] - detached + stdio:ignore + NAPKIN_DISTILL_NO_RECURSE - shell-metacharacter values stay isolated as argv, never concatenated Remove the now-unused shellEscape helper. Addresses security-review SEC-1. * test(distill): integration test for before_agent_start overlap injection (G2) overlap-injection.test.ts only covered the pure helpers (extractFileOpsFromMessage, intersectFiles, formatOverlapNotice). The handler wiring (env guard, vault resolution, session walking, active- distill filtering, diff union, systemPrompt append) had no direct test coverage \u2014 author comment said 'covered indirectly' but none of these code paths were exercised. Add overlap-injection.integration.test.ts with 7 tests: - overlap exists -> returns {systemPrompt: prefix + notice} - no overlap -> handler returns undefined - no active distills -> handler returns undefined - NAPKIN_DISTILL_NO_RECURSE=1 -> handler short-circuits even with overlap - dead distill (pid=999999) -> filtered out by alive check - empty session -> handler returns undefined before touching git - bogus ctx.cwd -> handler returns undefined, no throw The 'overlap exists' test uncovered a latent bug: the handler resolved vaultPath via 'new Napkin(ctx.cwd).vault.contentPath' which points at '<root>/.napkin' for content-layout vaults. getActiveDistills then checked '<root>/.napkin/.git' which doesn't exist, so the handler silently returned no overlap for every real vault. Fix: use ctx.cwd directly (the git root) for vaultPath; keep the 'new Napkin(ctx.cwd)' call for its throw-on-unresolvable semantics. Addresses coverage-review G2. * test(distill): cover wrapper MERGE_HEAD escape-hatch (G4) The wrapper's last-line defense at distill-wrapper.sh (the check for MERGE_HEAD still being present after the driver + salvage) was dead code in CI. No mock mode emitted conflict markers with exit 0, and when I tried that approach git actually cleared MERGE_HEAD on driver exit 0 (the driver's output gets committed even if it contains markers). Add an explicit NAPKIN_DISTILL_FORCE_MERGE_HEAD=1 testing hook that creates MERGE_HEAD immediately before the escape-hatch check. Also add a conflict-markers mock mode for completeness (emits exit 0 output with raw <<<<<<</=======/>>>>>>> markers). Integration test asserts: - wrapper exits 1 when MERGE_HEAD is present at the check - error log contains 'merge did not complete (MERGE_HEAD still present)' - error log contains the dangling SHA for forensic recovery - no distill squash commit lands on main Addresses coverage-review G4. * fix(distill): quote merge-driver path in git config for space-safety (SEC-2+C4) registerMergeDriver stored the driver path as part of a shell command string that git runs via sh -c when a merge fires. Unquoted, any path containing a space (e.g. '/Users/Foo Bar/...' on macOS display-name home dirs, or runner-controlled paths in CI) would word-split at the first space and the driver would silently fail. A malicious path prefix could also inject shell arguments. Fix: single-quote the path and escape any literal single-quote inside it as '\''. Git's sh -c now sees the path as one token. Tests: - config readback confirms the driver value starts with ' and ends with ' %O %A %B %P - integration: worktree creation succeeds when the vault path contains a space Addresses correctness-review C4 and security-review SEC-2. * fix(distill): delimit merge driver inputs to prevent prompt injection (SEC-3) The LLM merge driver fenced file contents between unfenced pseudo- markers ('<<<<<<<< BASE', '<<<<<<<< OURS', '<<<<<<<< THEIRS'). Any file already containing these markers (or a crafted prompt-injection payload that synthesises them) could terminate the section and speak to the LLM as the outer prompt author. Concrete attack path: a note whose body ended with a fake '========' + 'OURS' section could instruct the LLM to emit chosen content. Since auto-distill from one distill becomes input to the next distill's merge driver, content an earlier agent wrote gets fed back \u2014 a worm pattern. Fix: generate per-invocation random 16-hex delimiters for BASE / OURS / THEIRS sections. Regenerate if any delimiter accidentally appears in the inputs (astronomically unlikely). Also add an explicit instruction at the top of the prompt telling the LLM to treat content between markers strictly as data and not to follow instructions inside. Test (scripts.test.ts): - Feed inputs that contain the OLD static marker strings. With random delimiters the merge still succeeds and output is preserved (not replaced by an injected instruction). Addresses security-review SEC-3. * refactor(distill): extract common runner from runDistill/runAutoDistill (CLN-1) runDistill and runAutoDistill were 1:1 duplicates of a ~100-line polling + bookkeeping sequence, differing only in: 1. Pre-flight (runAutoDistill requires .git, runDistill is git-optional) 2. Spawn call (spawnDistill vs spawnDistillInWorktree) 3. Poll-completion target (tmpDir vs worktree path) 4. Timeout cleanup (fs.rmSync vs cleanupDistillWorkspace) The Phase B commit had to remember to update both bookkeeping paths; a future bug fix touching one path risked missing the other. Extract runDistillWith(ctx, strategy) that owns the shared lifecycle: config load, size dedup, status-bar painting, poll loop wiring, completion bookkeeping. The two callers supply a DistillStrategy with: - preflight?: optional short-circuit check - spawnFn: returns { target, cleanup } on success, null on failure runDistill now: 10 lines (strategy setup). runAutoDistill now: ~35 lines (strategy + DistillError-specific status). No behaviour change \u2014 all 226 tests still pass. Visible state mutations (isRunning, lastSessionSize, lastSpawnedSize, lastDistillTimestamp, pollHandle) remain in the closure, so existing closures-over-state contracts (shutdown handler, /distill, interval) are preserved. Addresses security-review CLN-1. * fix(distill): wrapper distinguishes conflict-remaining from real merge failure (C6) The wrapper swallowed ALL non-zero exits from 'git merge main' via '|| true', then let the salvage path decide from the file-level --diff-filter=U output. This masks real merge failures: - exit 128 (corrupt index, invalid ref, refusing to overwrite tracked files, etc.): the merge never started. salvage finds no unmerged files and falls through to squash, which silently lands against a distill branch that was never actually synced with mainline. - Race with concurrent distills: if index.lock contention outlasts the retry budget, exit 128 \u2014 same silent corruption window. Capture the merge exit code explicitly and branch on it: - 0: clean merge, continue. - 1: conflicts remain, proceed to salvage (expected). - other: log 'failed unexpectedly (exit N)' + dangling SHA + exit 1. Also: drop the git_retry wrapper around this one call. Retrying a partially-failed merge yields 'you have unmerged files' (exit 128) on the second attempt, which would now false-positive into the unexpected-exit branch. Transient index.lock on this single call isn't a real concern \u2014 we're the sole writer on the distill branch. Add NAPKIN_DISTILL_FORCE_MERGE_RC=<n> testing hook so tests can exercise the unexpected-exit branch without contriving a real git failure of that shape. Test (C6): force merge_rc=128 via hook, assert wrapper exits 1, error log contains 'failed unexpectedly (exit 128)' + 'aborting' + dangling SHA, no squash commit lands. Addresses correctness-review C6. * fix(distill): add timeout on merge driver's pi invocation (C7) The LLM merge driver shelled out to `pi` with no timeout. If the nested pi hangs (network stall, provider throttle, stuck stdin, infinite model loop), each attempt blocks indefinitely. 3 attempts per conflicted file means a single hung file can exhaust the parent wrapper's 10-min MAX_DISTILL_DURATION_MS budget \u2014 and when that fires, the worktree is yanked while the pi call is still active. Fix: wrap the real-path pi invocation in coreutils `timeout`, falling back to perl's alarm() when timeout isn't installed. Default 60s per attempt, overridable via NAPKIN_DISTILL_MERGE_TIMEOUT_SECS. If neither timeout binary nor perl is present (unusual), the call runs unbounded (acknowledged best-effort). Test (scripts.test.ts, 3s wall time): - Place a stub `pi` in PATH that sleeps 30s - Run driver with NAPKIN_DISTILL_MERGE_TIMEOUT_SECS=1 - Assert driver exits non-zero within 15s (proves timeout fired three times before retry budget exhausted, not that stub's sleep completed) - Skip cleanly if neither timeout nor perl is available on the runner. Addresses correctness-review C7. * feat(distill): add common secret patterns to auto-scaffold .gitignore (SEC-5) On a fresh vault with distill.enabled=true, auto-setup runs 'git add .' on first init. The scaffolded .gitignore covered napkin/Obsidian state and .DS_Store but nothing else \u2014 a .env, id_rsa, cert.pem, or similar in the vault root would be permanently captured in the initial commit (two-week gc grace doesn't help for tracked files). An Obsidian git plugin or manual push would then expose the secret. Extend GITIGNORE_LINES with standard secret patterns: - .env, .env.local, .env.*.local - *.pem, *.key - id_rsa, id_ecdsa, id_ed25519 - secrets.json - .aws/credentials Tests (auto-setup.test.ts): - Fresh vault: .gitignore contains all 10 secret patterns. - Fresh vault with pre-existing .env, id_rsa, cert.pem alongside a note: initial commit tracks the note but NOT the secrets. Addresses security-review SEC-5. * fix(distill): invoke wrapper via bash + propagate env for CI portability Two portability fixes so the distill extension works on Ubuntu runners (and Ubuntu/Debian users in general): 1. Spawn the distill wrappers with `bash` instead of `sh`. The wrappers have a `#!/usr/bin/env bash` shebang and use bash-specific syntax (arrays, `set -o pipefail`). On Ubuntu, `/bin/sh` is `dash`, which parse-errors on bash syntax and exits 2 before any wrapper logic runs. This is a latent production bug, not only a test issue. 2. Pass `env: process.env` explicitly in auto-setup's `runGit`. Unlike Node, Bun's `spawnSync` does not propagate mutations to process.env to the child unless env is passed explicitly — it snapshots env at runtime startup. Passing process.env is a no-op on Node and restores Node-compat semantics on Bun so tests can control git identity via GIT_AUTHOR_* env vars. 3. In shutdown-handler.test.ts, set dummy GIT_AUTHOR_* env vars in beforeEach so the Phase C1 test (production does `git init` + `git commit` on a fresh vault) succeeds on CI runners without a global ~/.gitconfig. Local users with a global gitconfig are unaffected; this only kicks in when no `~/.gitconfig` is present (typical of CI runners). Fixes the 16 failing tests on PR #11 CI: - 11 distill-wrapper.sh integration tests (dash parse error) - 4 ensureVaultReadyForAutoDistill tests (env not propagated on Bun) - 1 session_shutdown Phase C1 test (no identity available) * chore(deps): migrate from @mariozechner to @earendil-works pi-coding-agent scope pi-coding-agent and pi-tui were renamed from @mariozechner to @earendil-works starting with 0.74.0. Migrate all imports, peer dependency declarations, comments, and version-check test paths to the new scope. Pin >=0.74.0 to pick up the session_shutdown.reason field and the SIGTERM handling fix we rely on in shutdown-handler. - package.json peerDeps: @earendil-works/pi-coding-agent >=0.74.0, @earendil-works/pi-tui * - All TS imports updated across extensions/distill and extensions/napkin-context - session-touched-files.version-check.test.ts path updated - Comments / JSDoc references updated Verified: bun test (230 pass), bunx biome check (clean). * refactor(distill): replace any types with precise UIRef / RunCtx types (CLN-2) The distill extension stored `ctx.ui` and the shared runner's narrowed context as `any`, suppressing type-checking at the pi boundary. Replace both with precise types that track pi's public `ExtensionContext` / `ExtensionUIContext` surfaces: - `DistillUIRef.ui` is now `ExtensionUIContext` (gives us setStatus and theme type-checked, and any upstream rename surfaces as a compile error rather than silently widening). - `RunCtx` becomes `Pick<ExtensionContext, 'sessionManager' | 'hasUI' | 'ui' | 'cwd'>` so the four fields we actually read stay in sync with pi at the type level. No behavior change; no test changes. Verified: bun test (230 pass), bunx biome check (clean). * refactor(distill): consolidate worktree enumeration to single git call (CLN-3) `getActiveDistills` and `getUnmergedDistillBranches` both invoked `git worktree list --porcelain` independently. The /distill-status handler (collectDistillStatus) calls them back-to-back, so on every invocation pi ran two identical worktree enumerations. Introduce `getDistillState(vault)` that does exactly one `git worktree list --porcelain` + one `git branch --list 'distill/*'` and returns `{ active, unmerged }` derived from the same snapshot. Retain the two original functions as thin wrappers that call `getDistillState` and destructure — keeps the public API intact for existing tests and downstream callers. - collectDistillStatus (/distill-status + napkin_distill_status tool) migrated to the new call; saves one git invocation per status refresh. - Added describe block with 4 tests exercising mixed state + wrapper agreement. Verified: bun test (234 pass, +4), bunx biome check (clean). * test(distill): assert interval-fires-shortly-before-shutdown race captures delta (G5) Scenario: auto-distill interval ticks, spawns worktree A at size S1, and records lastSpawnedSize = S1. User types for another second, session grows to S2 > S1, then quits. Expected: the shutdown handler spawns a SECOND worktree B to capture the (S1 \u2192 S2) delta \u2014 the shouldDistillOnShutdown guard 8 lets us through because currentSize !== lastSpawnedSize. Add a dedicated describe block with two tests that drive the race end-to-end: - interval fires at S1, content grows to S2>S1, shutdown spawns a second worktree (count goes 0 \u2192 1 \u2192 2) - interval fires at S1, no new content, shutdown is a no-op (count stays at 1) Mechanism: stub setInterval to capture the auto-distill tick callback (rather than swallow it like the existing suite), fire it manually, then drive session_shutdown. Exercises the real runAutoDistill \u2192 lastSpawnedSize wiring instead of relying on the predicate-level should-distill-on-shutdown.test.ts alone. Verified: bun test (236 pass, +2), bunx biome check (clean). * feat(distill): detect conflicting .gitattributes merge rule and refuse to override (G7) Previously, auto-setup appended '*.md merge=napkin-distill-merge' to an existing .gitattributes unconditionally. Because gitattributes is last-match-wins, a user with '*.md merge=union' would end up with our driver quietly overriding theirs the next time a .md file was merged. Detect that case and refuse to scaffold: - `detectConflictingMdMergeRule(gaPath)` (new helper, exported) scans for any `*.md` line with a `merge=<driver>` attribute where <driver> != napkin-distill-merge. Scope is deliberately narrow: exact `*.md` pattern only, no pattern-overlap inference. - `ensureVaultReadyForAutoDistill` checks this right after git init and returns `{ error: 'conflicting merge rule\u2026', conflict: { rule, file, driver } }` WITHOUT touching .gitattributes or .gitignore on conflict. The existing `setupFailed` path in session_start then suppresses auto-distill for the session \u2014 manual /distill still works. - session_start notify explains the options: - remove the conflicting rule, OR - set distill.onShutdown: false in vault config.json Test coverage: - 5 tests in ensureVaultReadyForAutoDistill describe block (fresh / idempotent / conflict / narrower pattern / defensive) - 8 tests in new detectConflictingMdMergeRule describe block (missing / empty / comments / our rule / foreign / extras on same line / different pattern / no merge=) - 1 integration test in shutdown-handler.test.ts verifying the full session_start \u2192 setupFailed \u2192 shutdown-skips-spawn path, and that the user's .gitattributes is preserved byte-for-byte. Exports: - New public: `detectConflictingMdMergeRule`, `NAPKIN_MERGE_DRIVER` (test convenience). - `SetupResult.conflict?: { rule, file, driver }` alongside existing `error`; both are populated on conflict so fail-soft callers keep working. Verified: bun test (250 pass, +14), bunx biome check (clean). * test(distill): exercise pollHandle timeout branch with env-var override (G8) The 10-minute pollHandle timeout in runDistillWith was untested \u2014 a regression in the timeout path (missed cleanup, stuck isRunning, wrong status) would only surface after a real 10-minute hang. Add coverage so the branch is exercised on every CI run. Mechanism: - Convert the private `MAX_DISTILL_DURATION_MS` constant into an exported `getMaxDistillDurationMs()` that checks NAPKIN_DISTILL_MAX_DURATION_MS_OVERRIDE. Production default (10 min) is unchanged; malformed / zero / negative overrides fall back to the default to prevent accidental instant-timeout in production. - New test file `pollhandle-timeout.test.ts` stubs setInterval to capture both the auto-distill tick AND the pollHandle callback, fires the poll callback twice (once before timeout, once after a 150 ms real-time sleep past the 100 ms override), and asserts: - the timeout branch removes the worktree (via spawnCleanup) - isRunning resets so a subsequent auto-tick spawns a fresh distill - the production default is 10 minutes when the override is unset - malformed / zero / negative overrides fall back to the default Verified: bun test (254 pass, +4), bunx biome check (clean). * test(distill): pin pi before_agent_start systemPrompt contract for regression (C8) The distill overlap injector concatenates onto `event.systemPrompt` and returns `{ systemPrompt: <concat> }`. Both halves of that contract live in pi's public types: BeforeAgentStartEvent.systemPrompt: string (we read+concat) BeforeAgentStartEventResult.systemPrompt?: string (we return) If pi ever changes either field (e.g. structured object, required vs optional, renamed), our concat would silently truncate or become a type error without any test in our suite complaining. Add a companion version-check test modelled after session-touched-files.version-check.test.ts that: - verifies the pi types.d.ts is still at the expected path - extracts the BeforeAgentStartEvent body and asserts `systemPrompt: string` - extracts the BeforeAgentStartEventResult body and asserts `systemPrompt?: string` - asserts pi's docstring still documents chaining (so a rewrite to last-wins would be caught) A failure tells us to inspect the new types, update the overlap injector if the contract genuinely changed, or resync the assertions if it was just a rename. Verified: bun test (258 pass, +4), bunx biome check (clean). * ci: mirror cad0p/napkin — bun install + semver-calver-release Migrate CI and local tooling from pnpm to bun to mirror the upstream cad0p/napkin pattern (bun install + biome + tsc + bun test, OIDC release via cad0p/semver-calver-release). Workflows: - ci.yml: single 'test' job on push/PR to main, runs `bun install` then biome check + tsc --noEmit + bun test. Drops the pnpm setup and the split lint/test jobs. - release.yml: auto-release on push to main (and release/* branches with skip_release) via cad0p/semver-calver-release/release@v1, followed by OIDC npm publish via npm-publish@v1. Replaces the softprops/action-gh-release + manual npm publish flow. - validate-package-version.yml (new): runs cad0p/semver-calver-release/validate-package-version@v1 on PRs to catch missing / malformed version bumps. - validate-release-pr.yml (new): runs validate-release-pr@v1 on PRs for release-style checks (changelog, etc.). Local tooling: - package.json scripts: `biome check` → `bunx biome check`, `bun test` unchanged, add `typecheck` script (`bunx tsc --noEmit`). - Add typescript@^5.8.0 devDep so bunx tsc has a pinned compiler. - Add publishConfig/repository/homepage/bugs metadata to match napkin's shape so semver-calver-release and npm publish have everything they need. - New tsconfig.json mirrors napkin's (strict, esnext, bundler moduleResolution, skipLibCheck); excludes *.test.ts because bun test types them at runtime. Lockfiles: - Switch to bun.lock (committed) for reproducibility. Delete the tracked pnpm-lock.yaml and gitignore it (pnpm is no longer used for this project). Content fixes needed to make the new tsc gate green: - extensions/distill/index.ts: ctx.ui.notify expects 'info' | 'warning' | 'error'; 'success' was invalid. Change the distill-complete notify to 'info'. No visible UX change \u2014 the status bar still paints the success glyph via theme.fg. - extensions/napkin-context/index.ts: pi's CustomMessage.content is `string | (TextContent | ImageContent)[]`. Narrow to string for the Markdown renderer (we only ever set string content). - extensions/napkin-context/index.ts: pi's ExtensionContext narrows sessionManager to ReadonlySessionManager, which hides appendCustomMessageEntry. At runtime it's always the full SessionManager; cast through a `SessionManager` type import. Verified locally: - bun install (clean) - bunx biome check extensions/ (clean) - bunx tsc --noEmit (clean) - bun test (258 pass, same as before migration) * fix(distill): match *.md merge rule case-insensitively to honor APFS/NTFS core.ignorecase (R2-1) The G7 detector's regex used case-sensitive matching, which missed `*.MD merge=union` rules. On macOS (APFS default) and Windows (NTFS default) filesystems, git's pattern matching respects `core.ignorecase=true`, so `*.MD merge=union` DOES apply to `foo.md` files. Without this fix we silently layer our driver on top via last-match-wins — exactly the scenario G7 exists to prevent. - Add `i` flag to the regex in `detectConflictingMdMergeRule`. - Document the APFS/NTFS rationale in the function docstring. - Add two new tests: `*.MD merge=union` and mixed-case `*.Md MERGE=Ours`. Total tests: 258 → 260. * fix(napkin-context): guard appendCustomMessageEntry cast with runtime check (R2-2) pi's `ExtensionContext.sessionManager` is typed as `ReadonlySessionManager` \u2014 a `Pick` that omits mutation methods. The cast to the full `SessionManager` worked at runtime because pi hands out the concrete instance, but this is a load-bearing assumption about pi's internals. If pi ever wraps the session manager in a readonly proxy, the cast would silently become a `TypeError` at `appendCustomMessageEntry(...)` call time \u2014 fatal for the extension. Defense-in-depth: - Duck-type check via `typeof sm.appendCustomMessageEntry === 'function'` before calling (handles the "method simply missing" case). - `try/catch` around the call (handles the "proxy throws on write" case). - Graceful degradation: on failure, emit a `notify(..., 'warning')` and continue without the context injection. Worst-case is a session without the vault overview, not a crashed extension. - `Partial<SessionManager>` cast keeps the type-level surface narrow while allowing the runtime probe. * chore(deps): pin @earendil-works/pi-tui peer dep floor to >=0.74.0 (R2-3) Previously `@earendil-works/pi-tui` was an unbounded wildcard (`*`) peer-dep. This allowed a lockfile-less install to resolve to a pre-migration `@mariozechner` version or an incompatible future `@earendil-works` major. Match the floor already set on `@earendil-works/pi-coding-agent` (`>=0.74.0`) so both peer deps track the same migration boundary. - `bun install` refreshed `bun.lock`'s recorded constraint. - No resolved-version changes \u2014 already on `@earendil-works/pi-tui@0.74.x`. * perf(distill): skip branch listing in getActiveDistills when unmerged not needed (R2-4) Post-CLN-3, `getActiveDistills` delegated to `getDistillState`, which unconditionally invoked `git worktree list` AND `git branch --list`. The sole production caller of `getActiveDistills` (the `before_agent_start` overlap injector) doesn't need the unmerged branch list, so the extra git call was wasted work. Split `getDistillState` into three internal primitives: - `listDistillWorktrees(vaultPath)` \u2014 `git worktree list --porcelain` + distill/* filter. - `listDistillBranches(vaultPath)` \u2014 `git branch --list`. - `toActiveDistills(entries)` \u2014 meta.json + pid-liveness hydration (no git). Compose: - `getActiveDistills` = `listDistillWorktrees` + `toActiveDistills` (one git call, no branch listing). - `getUnmergedDistillBranches` = `listDistillWo…
…emplate placeholders Item A1 of PR #12 (agent-driven merge architecture). Introduces the externalised distill prompt as a new artefact pair: `distill-prompt.md` holds the full agent-driven worktree prompt (10 steps + worktree-isolation prefix) with four template placeholders ({{worktreePath}}, {{vaultPath}}, {{branchName}}, {{defaultBranch}}); `distill-prompt.ts` exports `buildDistillPrompt(inputs)` which reads the .md at runtime and substitutes placeholders. Why externalise: easier prompt iteration without touching code, markdown rendering in editors, snapshot-testable against a single source of truth. Per the PR #12 design's "DISTILL_PROMPT location" lock-in. Why not yet wired to production callers: at A1 the wrapper still owns merge/squash/cleanup. Wiring the agent-driven prompt now would have the agent and the wrapper double-execute steps 7-10, which is worse than either end. A2 rewrites the wrapper to invoke the agent for those steps; that is when buildDistillPrompt replaces the inline DISTILL_PROMPT in index.ts. The .md+loader+test ship at A1 as a freestanding architectural piece that A2 builds on. The loader is strict on placeholder coverage and rejects empty inputs — silent corruption (e.g. `git merge ` with empty branch name) is the failure mode to avoid, per the methodology guide's never-deferrable "stale references to renamed concepts" category. package.json `files` now includes `distill-prompt.md` so the .md ships in the npm publish (the loader resolves it relative to its own location via `import.meta.url` + `fileURLToPath`, matching the existing `scripts-paths.ts` pattern). Tests: 15 new (363 -> 378 baseline). Cover happy-path substitution, multi-occurrence replaceAll, empty-input rejection (4), missing-placeholder, empty-template, and 7 .md content invariants (10 step markers, no-force, pull-merge, do-not-loop, isolation prefix).
…gent call + hard timeout Item A2 of PR #12 (agent-driven merge architecture). Rewrites the distill wrapper from a multi-step pipeline (pi -> add -> commit -> merge -> squash -> commit) into a single bounded agent call. The agent now owns content production AND the integration phases (merge, squash, push, cleanup) per the PR #12 design's "Agent contract" section; the wrapper is a thin shell: shim install, cd parentCwd, `timeout(1) pi -p $PROMPT`, write outcome. Wrapper changes: - new positional arg 10: maxDurationSecs (hard agent-task budget; default 600s = 10 minutes from `distill.maxDurationMinutes` config, no per-phase timeouts anymore — folded into this single knob) - replaces git add/commit/merge/squash/commit block (lines 558-680 of the PR #11 wrapper) with a single `timeout --foreground "$MAX_DURATION_SECS" pi --session ... -p "$PROMPT"` invocation - drops merge-driver-specific test hooks: NAPKIN_DISTILL_FORCE_MERGE_HEAD, NAPKIN_DISTILL_FORCE_MERGE_RC (Phase B will drop the merge driver script entirely) - drops the partial-merge log file (no driver to 3-strike on) - retains: shim install, meta.json pid rewrite, startSha extraction, cleanup trap, all HALT_AFTER_* / FORCE_CLEANUP test hooks - leaves TODO(A3) markers for post-validation (markers absent, HEAD on default, commit count, merged-local detection) and TODO(A4) for the salvage path (force-cleanup + failed:<reason> outcome) - on agent exit-0: writes `merged-content` unconditionally (placeholder until A3); on agent non-zero: exits 1 without writing an outcome (placeholder until A4 — JS-side abnormal-termination warning fires) JS-side changes (extensions/distill/distill-workspace.ts + extensions/distill/index.ts): - SpawnDistillOptions drops `prompt` (built internally now via buildDistillPrompt against the .md template introduced in A1) and adds `maxDurationSecs` (positive integer, derived from getMaxDistillDurationMs(config) / 1000) - spawnDistillInWorktree now resolves the agent prompt internally; the worktree-isolation prefix is in distill-prompt.md so buildWorktreeDistillPrompt is no longer called from the spawn path (still exported pending Phase B deletion) - 11th wrapper arg added on the JS side to match the new wrapper signature Test deltas: 363 -> 365 pass, 13 skip, 0 fail. Skipped tests document the pre-existing driver-specific behavior and are either replaced in Phase C (mocked-pi fixtures simulating each agent- behavior class) or deleted in Phase B (driver-specific tests): - 4 in (integration): happy-path, empty-distill, POST-CONV-1, concurrent - 3 in (partial-merge salvage): clean-file, all-conflict, log-co-locate - 2 in (MERGE_HEAD escape-hatch): persists-after-merge, unexpected-rc - 2 in (LLM-resolved conflict, end-to-end): driver-output-on-main, driver-retries-ok-after-2 - 1 in (non-main default branch): wrapper-squash-merges-into-master - 1 in routing: no-content outcome -> warning notify (resumes after A3 wires validate_commit_count for no-content detection) Tests retained and unchanged: shutdown-handler.test.ts (predicate logic), routing.test.ts spawn-routing tests (worktree vs legacy spawn invariant), distill-workspace.test.ts (worktree creation, parentCwd validation), system-prompt.cache-parity.test.ts (POST-R6-CACHE byte-equality), pollhandle-timeout.test.ts (JS-side poller). Tests updated to A2 contract: 5 unit tests in spawn-distill-in-worktree.test.ts (`spawnDistillInWorktree (unit, mocked spawn)`) — drop `prompt:`, add `maxDurationSecs: 600`, assert new arg [10] is the maxDurationSecs string and arg [5] (prompt) contains the steps 1-10 markers + substituted placeholders + isolation prefix. DO NOT YET DELETED (Phase B owns the deletion): the merge driver script `extensions/distill/scripts/napkin-distill-merge`, the `.gitattributes` install in auto-setup.ts, scripts.test.ts, and git_retry.sh.
…on default, merged-local detection Item A3 of PR #12 (agent-driven merge architecture). Replaces the A2 TODO(A3) stub with four post-agent-exit validators that decide the outcome class deterministically, plus a JS-side dispatch update so the new classes surface with the right notification severity. Wrapper helpers added (extensions/distill/scripts/distill-wrapper.sh): - validate_no_markers <vault> — scans tracked *.md files for residual conflict markers (`<<<<<<< `, `======= ` exact, `>>>>>>> ` at line start). Uses `git ls-files -z` so .gitignore'd content (`.napkin/distill/`) is skipped automatically. On hit, logs offending paths and returns 1. - validate_head_on_default <vault> <default> — confirms vault HEAD is the symbolic ref `refs/heads/<default>` via `git symbolic-ref --short HEAD` (V3-locked: detects detached HEAD with a non-zero exit, unlike `branch --show-current` which returns empty silently). - validate_commit_count <vault> <startSha> — prints the count of commits since startSha on the current HEAD; returns non-zero only if rev-list itself fails. The wrapper dispatches `no-content` on 0 and proceeds to the merged-content/local-only dispatch otherwise. - detect_local_only <vault> <default> — returns 0 (true) when origin is configured AND local is ahead of `origin/<default>`. No origin configured means the user never expected a push, so this returns false and the outcome stays merged-content. Wrapper post-agent-exit dispatch (replaces the A2 unconditional `merged-content` placeholder): AGENT_RC == 124 or 137 (timeout SIGTERM or SIGKILL after grace) -> failed:agent-timeout AGENT_RC != 0 -> failed:agent-exit-nonzero HEAD not on default -> failed:head-not-on-default markers found in vault -> failed:markers-after-agent-exit commit_count == 0 -> no-content origin diverges -> merged-local otherwise -> merged-content Magic numbers 124 / 137 are coreutils-defined `timeout(1)` exit codes (SIGTERM exit-within-grace / SIGKILL after grace); named as TIMEOUT_TERM_RC / TIMEOUT_KILL_RC in the wrapper per the methodology guide's never-deferrable magic-number rule. JS-side update (extensions/distill/index.ts formatOutcomeNotification): - new `merged-local` case → warning ⚠ "Distillation complete locally; not pushed to origin (Ns)" - new `failed:<reason>` prefix → error ✗ "Distillation failed: <reason>" (the reason code surfaces in both the message and status text so the user can diagnose without opening the error log first) Salvage at A3 is minimal: failure paths write the `failed:<reason>` outcome and exit 1; the cleanup trap still removes worktree+branch best-effort. A4 will replace this with the explicit salvage helper that force-cleans pre-exit and emits a recovery hint into the outcome sidecar. Tests: 365 -> 376 pass (+11). New file extensions/distill/wrapper-validation.test.ts adds 10 end-to-end tests that drive the wrapper with `NAPKIN_DISTILL_PI_BIN=<bash-stub>` producing each behavior class: - validate_head_on_default PASS / FAIL - validate_no_markers PASS / FAIL - validate_commit_count PASS / FAIL (no-content) - detect_local_only PASS (no origin) / TRIGGERED (merged-local) - agent exit non-zero / agent exceeds maxDurationSecs Plus: routing.test.ts "no-content outcome → warning notify" was skipped in A2 because the wrapper hadn't yet wired no-content detection. A3 ships that detection, so the test is unskipped here and now passes. Skipped tests unchanged from A2 (12 driver-specific): partial-merge salvage, MERGE_HEAD escape-hatch, LLM-resolved conflict, integration happy/empty/POST-CONV-1/concurrent, non-main default branch squash-merges. All marked `// will be deleted in Phase B/C`.
… failed:<reason> outcome Item A4 of PR #12 (agent-driven merge architecture). Replaces the A3 "write outcome + exit 1" failure-path stubs with an explicit `salvage` helper that V3-locked never touches main vault history. Wrapper helper added (extensions/distill/scripts/distill-wrapper.sh): salvage <vault> <worktree_path> <branch_name> <reason> Steps in order: 1. cd to vault (so `git worktree remove --force` doesn't refuse) 2. force-remove worktree (`git worktree remove --force`) 3. rm -rf the leaf if shim residue survives (POST-CONV-3 pattern) 4. `git worktree prune` for stale entries 5. `git branch -D` the distill branch 6. rmdir parent vault-hash dir if empty (POST-CONV-4 pattern) 7. log a critical-error if vault HEAD isn't on the default branch (V3: NEVER `git checkout` on the user's behalf — the salvage path is a janitor for the worktree+branch, not a rollback agent for main) 8. write `failed:<reason>` outcome with reason-specific recovery hint Reason codes (V3-locked): markers-after-agent-exit -> "git revert HEAD --no-edit" + "reflog ~90 days" head-not-on-default -> "git checkout <default>" + "reflog" agent-exit-nonzero -> "see error log" + "reflog" agent-timeout -> "bump distill.maxDurationMinutes" + "reflog" Outcome sidecar format (multi-line for failed:* classes): line 1 = outcome class (machine-readable, drives JS-side dispatch) lines 2+ = optional human-readable recovery hint Happy-path classes (merged-content, merged-local, no-content) stay single-line so the legacy single-line shape continues to parse cleanly. JS-side updates: - findDistillOutcomeForBranch (distill-workspace.ts) parses line 1 as outcomeClass and exposes lines 2+ as `recoveryHint: string | null`. Backward-compatible with single-line legacy sidecars (recoveryHint is null for those). - formatOutcomeNotification (index.ts) renders the recovery hint inline in the failed:* notification: "Distillation failed: <reason> — <hint>" - DistillStrategy.checkOutcome's return type extended with recoveryHint; runDistillWith propagates it through to the dispatch. Tests: 376 -> 381 pass (+5). New file extensions/distill/wrapper-salvage.test.ts: - markers-after-agent-exit: outcome carries reason + 'git revert' hint; main vault HEAD is NOT reset (V3 lockdown — the agent's commit with markers stays; user `git revert`s manually per the hint) - head-not-on-default: outcome carries reason + 'git checkout main' hint - agent-exit-nonzero: outcome carries reason + 'reflog' hint; main vault HEAD is unchanged (agent crashed pre-squash) - agent-timeout: outcome carries reason + 'maxDurationMinutes' hint - happy-path regression: merged-content sidecar stays single-line and `recoveryHint` is null Plus a fix-up to wrapper-validation.test.ts: the A3 helper read the outcome via `readFileSync.trim()` which now collapses the multi-line sidecar into one string. Updated to take only line 1, mirroring the JS-side parser. Salvage operations are best-effort (worktree-remove / branch-D / rmdir all swallow errors). The outcome sidecar write is the user-facing signal; transient cleanup races (concurrent salvage on different distills, ENOENT on already-removed dirs) don't block it. V3 lockdown reaffirmed: NO main vault history mutation under any salvage path. Per case-C analysis in research/v2-v3-verification.md, `git reset --hard $START_SHA` would silently destroy concurrent user commits in the autosave-while-editing scenario. The recovery hint points the user at the forward-only `git revert` path instead.
…-2, CLEAN-A-3, SEC-A-6) Round 1 spec-blind review caught CLEAN-A-2, CLEAN-A-3, SEC-A-6. The prompt's opening line told the agent "You are running in an isolated git worktree at <path>", and step 7 used bare "git merge" / "git add -A" / "git commit -m" commands. But pi spawns the agent at PARENT_CWD per POST-R6-CACHE prompt-cache parity, not the worktree. So bare git in step 7 would either silently corrupt a parent project repo or fail with "not a git repository". Steps 8-10 already used "git -C vaultPath" correctly; step 7 was the inconsistent outlier. Three changes share one .md, so they share one commit: CLEAN-A-2 high: rewrite step 7 to use "git -C worktreePath" for every git operation. Brings step 7's shape into line with steps 8-10. CLEAN-A-3 medium: replace the false opening with a statement of the cwd contract. Self-explaining for future readers and the agent itself, per methodology guide "improve the code, not the comments": the prompt now describes its actual contract instead of relying on reviewer-side context to interpret it. SEC-A-6 medium: add --no-rebase to step 9's pull recovery so a user with global pull.rebase=true cannot silently bypass the spec's pull-merge requirement. Tests: distill-prompt.test.ts gains three assertions; the legacy "isolated git worktree" string assertion is replaced with the new contract-shape assertion. spawn-distill-in-worktree.test.ts updates its prompt-shape assertion to match the new opening.
…t fallback (CLEAN-A-1, SEC-A-3, CI-A-1)
Round 1 cross-reviewer convergence on the timeout(1) invocation:
cleanness-blind, security, and CI-portability lenses all flagged the
same root-cause cluster.
CLEAN-A-1 + SEC-A-3 (high): the wrapper invoked
"timeout --foreground <budget>" without -k/--kill-after. Per
coreutils' man page, without -k, timeout sends a single SIGTERM and
waits indefinitely for the target. A SIGTERM-ignoring agent (a
stuck-in-libc-syscall pi process, a tool subprocess that traps
SIGTERM, an OOM-killing-deadlock scenario) hangs the wrapper forever,
holding the worktree, the gitignored shim, and the .napkin/distill/
cache slot. The header comment claimed SIGKILL escalation happened
after a "10s grace"; that was untrue. The rc-137 (SIGKILL escalation)
arm in the dispatch case was dead code under the bare invocation.
CI-A-1 (high): macOS without "brew install coreutils" doesn't ship
timeout(1) at all; Homebrew installs it as gtimeout by default. The
wrapper's bare "timeout --foreground" expanded to "command not found"
on stock macOS, AGENT_RC=127, routed to failed:agent-exit-nonzero
with no diagnostic naming the missing binary. Every distill on a
stock-macOS user's machine surfaced that misleading outcome.
Fix:
- Detect TIMEOUT_BIN at wrapper start with
"$(command -v timeout || command -v gtimeout || true)". Fail-fast
with "exit 2" + actionable error message (mentions
"brew install coreutils") if neither is on PATH. Solves CI-A-1
without runtime branching.
- Add TIMEOUT_KILL_GRACE_SECS=30 named constant (per the methodology
guide's never-deferrable magic-number rule) and pass it via
-k "${TIMEOUT_KILL_GRACE_SECS}" to the timeout invocation. Now
SIGKILL escalation actually happens, the rc-137 dispatch arm is
reachable, and the comment claims match the code.
- Make the grace period overridable via
NAPKIN_DISTILL_TIMEOUT_KILL_GRACE_SECS so SIGTERM-ignoring stub
tests can complete within the test timeout budget without waiting
the full 30s. Documented as a testing hook alongside the existing
HALT_AFTER_* / FORCE_CLEANUP hooks.
- Replace bare "timeout" with "$TIMEOUT_BIN" in the invocation so the
detected binary (timeout or gtimeout) is what runs.
Tests:
- New regression: "SIGTERM-ignoring agent → SIGKILL escalates within
grace, classifies as agent-timeout". Stub does
"trap '' TERM; sleep 60; touch <marker>". Wrapper runs with
maxDurationSecs=1 + grace=2. Asserts wrapper exits with
failed:agent-timeout, the marker was never created (proving
SIGKILL fired before sleep returned), and elapsed wall-clock is
under 15s (would hang past 30s outer timeout if -k were dropped).
- The pre-existing "agent exceeds maxDurationSecs" test continues to
pass — sleep cooperatively exits on SIGTERM at 1s, doesn't reach
the SIGKILL escalation path. Both paths are now covered.
Verified: bun test green (384 pass, 12 skip, 0 fail; +1 new test),
bunx tsc --noEmit clean, biome check clean.
Round 1 CI-portability lens flagged three workflow-level gaps the four review lenses (correctness, cleanness, security, CI portability) only saw because CI was added as a fourth lens after PR #11's missing-git- identity blind spot. Per the methodology guide's "Known blind spot: CI/environment portability" section, two of these are never-deferrable (CI-A-3 unpinned bun version, CI-A-4 missing git identity). CI-A-2 (high): the matrix ran only ubuntu-latest. Any macOS-only regression — CI-A-1's missing timeout(1), BSD vs GNU grep flag drift, BSD xargs -I quirks, bash 3.2 surface — was invisible until a user reported it. Adds macos-latest to the matrix with fail-fast: false so one platform's failure doesn't mask the other. Combined with Commit a2c8c21 (timeout(1) hardening + gtimeout fallback), the macOS slot now exercises the same wrapper path Linux does. brew install coreutils runs only when runner.os == macOS so Linux jobs are unchanged. CI-A-3 (medium → never-deferrable): bun-version: latest floats with whatever bun release is current on the day CI runs. A bun-internal regression (test runner, fs API, bun:test semantics) could break CI without any code change in this repo. Pinned to 1.3.13, the contributor-machine version. Bump deliberately when a new bun release ships something useful. CI-A-4 (medium → never-deferrable): the workflow had no defensive git identity step. The new wrapper-validation / wrapper-salvage tests do set identity inside their stubs AND in spawnSync env, but a future contributor adding a test that calls git commit without all three layers would silently regress to the PR #11 failure mode. Adds git config --global user.email/user.name + init.defaultBranch=main before bun install. Mirrors the recipe captured in PR #11's deferred.md "Discovered on pi-napkin PR #11 (2026-05-12)" entry. Verified: bun test green locally (384 pass, 12 skip, 0 fail). YAML syntax validates via python yaml. The GH workflow itself can only be verified post-merge against actual runners; the changes are mechanical and the macOS-slot coverage is the new thing to watch on the next CI run.
…ty (SEC-A-1)
The locked spec at "Push behavior: never force" mandates wrapper-side
post-validation:
"Spec the prohibition in the prompt; the wrapper post-validates by
checking that origin/<default> only fast-forwarded (its tip is an
ancestor of the new tip after push)."
Round 1 security review caught that the wrapper implemented only the
first half. The prompt at distill-prompt.md:45 says "NEVER use --force
or --force-with-lease", but detect_local_only's SHA-equality check
couldn't distinguish a fast-forward push from a force-push: in either
case after the agent's push fetches back, local == origin/<default>.
An agent that ignored the prompt prohibition (model error or prompt-
injection from vault content) would land corrupt origin history with
the wrapper writing merged-content checkmark to the user.
The prompt is a soft control. The wrapper's post-validation is the
hard one. Today, the hard one didn't exist.
Fix:
- Replace the rc=0/rc=1 binary in detect_local_only with three states:
rc=0 — local-only (merged-local): legitimate fast-forward state
(origin configured AND remote is ancestor of local) OR
origin configured but origin/<default> never fetched.
rc=1 — in sync OR no origin: proceed to merged-content.
rc=2 — divergent histories: remote is NOT an ancestor of local.
Force-push or remote rewrite happened.
The ancestry check is `git -C <vault> merge-base --is-ancestor
<remote_sha> <local_sha>`. Returns 0 when remote is ancestor of
local — exactly the condition the spec asks for, just inverted to
detect violations.
- Update the dispatch site to capture rc explicitly (because bash
`if cmd; then` collapses rc=1 and rc=2 into the same arm) and route
rc=2 to a new failed:force-push-detected outcome class via the
salvage helper.
- Add a recovery hint for force-push-detected: points the user at
`git log origin/<default>..<default>` and `git log
<default>..origin/<default>` to inspect the divergence, plus reflog
for distill content recovery.
- Update the salvage() docstring and the dispatch-site comment to
list force-push-detected alongside the four existing reason codes.
- Update extensions/distill/index.ts's formatOutcomeNotification
comment to mention the new reason code (the dispatch logic itself
is generic over failed:<reason> and needed no code change).
Tests:
- New regression: "detect_local_only DIVERGENT: origin rewritten
under local → failed:force-push-detected (SEC-A-1)". Builds a
divergent state by giving origin a foreign commit (via a second
clone) and committing a different file on the vault's local main
before the agent runs. Asserts wrapper exits 1 with
failed:force-push-detected.
- The existing detect_local_only TRIGGERED test (origin behind local
in a fast-forwardable way) continues to pass — that's now the
rc=0/merged-local arm of the new tri-state semantics, exercised
exactly the way the spec describes legitimate "didn't push" state.
Verified: bun test green (385 pass, 12 skip, 0 fail; +1 new test),
bunx tsc --noEmit clean, biome check clean.
…trap (SEC-A-2)
Round 1 security review caught defense-in-depth gap. Today's salvage()
and the cleanup trap call:
if [ -d "$worktree" ]; then
rm -rf "$worktree" 2>/dev/null || true
fi
after `git worktree remove --force`. Note: `git worktree remove --force
<path>` REFUSES paths not registered as worktrees of the vault (returns
non-zero, suppressed by `|| true`). The directory then still exists,
the second `[ -d ]` is true, and `rm -rf` runs on whatever path was
passed in.
JS-side `resolveCacheRoot` always builds worktrees under
`<XDG_CACHE_HOME or ~/.cache>/napkin-distill/<vault-hash>/<branch-suffix>/`,
so under normal use the wrapper is safe. But "JS-side construction is
correct" is the only line of defense. An upstream bug (out-of-tree
caller, malformed test fixture, future code path) that passed
`worktree=/etc` or `worktree=$HOME` would silently turn into
`rm -rf /etc` or `rm -rf $HOME`. Per the methodology guide's "Severity
ladder → high" entry, defense-in-depth gaps that could cause silent
data loss are high, not medium — JS-side correctness is the primary
defense, this is the safety net.
Fix:
- Add `safe_rm_worktree` helper. Resolves the input via `cd <path>
&& pwd -P` (canonicalises symlinks portably across BSD/GNU; defeats
symlink-tricks). Refuses to `rm -rf` unless the resolved path
matches `*/napkin-distill/*/*` (i.e. lives at least two levels
deep under a `napkin-distill` directory — matching the production
cache layout's `<cache>/napkin-distill/<hash>/<suffix>/` shape).
On refusal, logs a precise diagnostic naming both the input and
the resolved path and returns 1; caller proceeds (best-effort
cleanup is the contract).
- Replace `rm -rf "$worktree"` in salvage() with
`safe_rm_worktree "$worktree" || true`.
- Replace `rm -rf "$WORKTREE"` in the EXIT trap (cleanup function)
with `safe_rm_worktree "$WORKTREE" || true`.
Tests:
- New describe block `safe_rm_worktree path-safety guard (PR #12
SEC-A-2)` with four assertions:
- refuses to rm-rf a path outside any napkin-distill subtree
(asserts the directory and its sentinel file survive, and the
stderr names "refusing to rm-rf" + "napkin-distill")
- removes a path inside a `napkin-distill/<hash>/<suffix>/`
subtree (asserts the directory is gone)
- refuses an empty path (rc 1, stderr names "empty worktree path")
- returns 0 for a non-existent path (idempotent / already-removed)
- The function is extracted from the wrapper via awk-style range
scan (start at `safe_rm_worktree() {`, end at the next bare `}`)
and sourced into a tiny bash harness with a stub log_error so the
test can call the function with crafted paths the production
wrapper would never construct.
- All five existing salvage scenarios (markers, head-not-on-default,
agent-exit-nonzero, agent-timeout, happy-path) continue to pass —
they hit the `*/napkin-distill/*/*` arm of the new guard and behave
identically to the pre-fix code.
Verified: bun test green (389 pass, 12 skip, 0 fail; +4 new tests),
bunx tsc --noEmit clean, biome check clean.
The pre-fix write_outcome used two printf calls inside a single redirect group. Bash flushes between the two printfs, so a JS-side poller (findDistillOutcomeForBranch) could open the file between the kernel writes of line 1 and line 2, see only the outcome class, and misclassify a failed distill as having no recovery hint. Fix: write to a sidecar.tmp file then mv to the canonical path. POSIX rename(2) is atomic on the same filesystem, so the poller either sees the file absent or sees the complete contents, never a partial write. Standard temp-and-rename pattern. Tests: extract write_outcome from the wrapper into a bash harness (same pattern as the SEC-A-2 safe_rm_worktree extraction) and assert single-line and multi-line outcomes write the exact expected bytes, no .tmp straggler is left behind after the call, idempotent rewrite replaces the file cleanly. Plus an end-to-end test that spawns the real wrapper and asserts the post-run error dir contains no .tmp files. Cites SEC-A-4 (medium).
…buildDistillPrompt inputs (SEC-A-5)
Defense-in-depth at the API contract boundary: validateInput() now rejects (a) non-string / empty, (b) ASCII control chars in [\x00-\x1F] + \x7F, and (c) {{ / }} placeholder-syntax collisions for every field of DistillPromptInputs.
Rationale: a literal newline in any input field would break out of the prompt step it embeds in (classic prompt-injection class). NUL bytes truncate strings on most C-side consumers. Mustache-collision is a future-refactor hazard if the substitution loop ever multi-passes.
Realistic inputs (paths with spaces / hyphens / dots, branch names with slashes like 'feat/foo' or 'release/2026-q1') still pass.
Tests: 42 new in distill-prompt.test.ts cover each control-char representative (NUL, LF, CR, TAB, BEL, ESC, DEL) across all four input fields, three placeholder-syntax shapes across all four fields, and two regression-guard happy-path inputs.
References: SEC-A-5 (medium) per Phase A round-1 security review.
… types co-present (CORR-A-1, SEC-A-7)
Real merge conflicts always emit ALL THREE marker types ('<<<<<<< ', '======= ', '>>>>>>> ') in the same file. The prior validator flagged any one of them, false-positiving on two common shapes: (1) markdown setext H1 underlines ('# title' followed by '=======' on its own line), and (2) documentation prose that quotes a single conflict marker to discuss merges (e.g. README sections that walk the reader through what '<<<<<<< HEAD' means). Either shape would have permanently blocked distills on the affected vault.
Tighten the per-file predicate to require all three markers co-present in the same file via a 'grep -q && grep -q && grep -q' chain. Use a bash 3.2 portable 'while IFS= read -r -d "" file' loop with array '+=' aggregation so the helper still runs on macOS default bash. Continues to enumerate via 'git ls-files -z -- *.md' so .gitignore'd content (e.g. '.napkin/distill/') is skipped.
Tradeoff: a vault that documents a complete '<<<<<<<' / '=======' / '>>>>>>>' example inside a single file (including in a fenced code block — the validator does not parse markdown structure) will trip the check. Users escape with leading whitespace, HTML comments, or by splitting the example across two files. The cost of any-of-three false-positives (block-all-distills-forever on legitimate documentation) is much higher than this rare-explicit-doc case.
Tests: five regression guards in wrapper-validation.test.ts pin the new behavior — setext H1 underline passes, single-marker prose passes, two-marker prose passes, markers split across two files passes, and all-three inside a code block fails (acknowledged tradeoff). Existing all-three real-conflict FAIL test still passes.
References: CORR-A-1 (low→elevated), SEC-A-7 (low→elevated). Elevation rationale: false-positives would permanently block distills on user vaults documenting merge conflicts in notes. CLEAN-A-15 tracks the per-file grep -q optimisation opportunity.
…s (CLEAN-A-6)
wrapper-validation.test.ts and wrapper-salvage.test.ts duplicated ~80 LOC of identical scaffolding (Scaffold interface, makeScaffold, writeStubPi, runWrapper). Move them to _test-helpers.ts as makeWrapperScaffold(prefix), writePiStub, runWrapperWithStub plus the WrapperScaffold interface, and rename the call sites in both files via local aliases so the test bodies stay byte-identical.
runWrapperWithStub returns the union of both prior runWrappers' outputs (outcome/outcomePath from the validation variant + preSha from the salvage variant). Both consumers selectively read what they need; the extra computation is cheap (one rev-parse + one readdir scan).
Side-effect on tsconfig: extensions/**/_test-helpers.ts is now in the tsc exclude list alongside *.test.ts. The previously-included withNapkinOnPath helper passed strict checks because it never touched SessionManager; the SessionManager.create + appendMessage incantation moved here is the same loose pattern every test file uses (already excluded). package.json files already excluded _test-helpers.ts from the published bundle, so this aligns the two configs.
Pure refactor — no behavior change. All 30 wrapper-{validation,salvage} tests still pass; full suite unchanged at 440 pass / 12 skip / 0 fail. Phase C will benefit when adding bash-stub fixtures for ~10 mocked-pi behaviors.
Cites: CLEAN-A-6 (medium, never-deferrable cleanness).
…EAN-A-4, CI-A-6) CLEAN-A-4: distill-wrapper.sh's docstring still carried 'A2 transitional state', 'TODO(A3)', and 'TODO(A4)' markers from the staged-rollout commits. A3 (afed6ae) and A4 (0d0a262) both landed weeks ago, so the markers misrepresent the wrapper's actual state. Replace the multi-paragraph A2/A3 evolution block with a one-line implementation-history reference, drop 'stubbed in this A3 commit' parentheticals from the lifecycle steps, and trim the TODO(A3)/TODO(A4) markers from the agent-responsibilities footer. The numbered Lifecycle section already documents the current shape correctly. CI-A-6: two describe.skip blocks in spawn-distill-in-worktree.test.ts (MERGE_HEAD escape-hatch at L1440, LLM-resolved conflict end-to-end at L1650) lacked the '// will be deleted in Phase B' annotation that the parallel partial-merge-salvage block at L1126 carries. Both target the pre-A2 NAPKIN_DISTILL_MERGE_MOCK driver path that A2 deleted, so they share Phase B fate. Add the same annotation form (suffix on the closing comment-block line) for grep parity. Comment-only changes. bash -n on the wrapper still parses; full test suite unchanged at 440 pass / 12 skip / 0 fail; tsc + lint clean. Cites: CLEAN-A-4 (medium, never-deferrable: stale comments), CI-A-6 (medium, never-deferrable: stale references). Both never-deferrable per release-grade methodology.
…-setup install (B1) PR #12 Phase B item B1 — remove the now-dead per-file LLM merge driver and stop installing it on new vaults. The agent-driven merge architecture (Phase A) made this code unreachable: the distill agent now resolves merges itself in its worktree (steps 7-10 of distill-prompt.md), so there is no driver to register, no .gitattributes rule to install, and no scripts.test.ts to maintain. Deleted: extensions/distill/scripts/napkin-distill-merge (455 LOC bash driver), extensions/distill/scripts.test.ts (driver test suite — would fail at module-load once MERGE_DRIVER_SCRIPT export is removed; pulled forward from B2 to keep tests green between commits per methodology), MERGE_DRIVER_SCRIPT export from scripts-paths.ts, registerMergeDriver() helper + its call site in distill-workspace.ts, GITATTRIBUTES_LINES + NAPKIN_MERGE_DRIVER + detectConflictingMdMergeRule + SetupResult.conflict from auto-setup.ts, the G7 conflict-dispatch arm + comment in index.ts. Test cleanup: remove pre-PR-12 '*.md merge=napkin-distill-merge' fixture writes from routing.test.ts, pollhandle-timeout.test.ts, shutdown-handler.test.ts, spawn-distill-in-worktree.test.ts (lines were inert no-ops post-driver-deletion). Drop the G7 'session_shutdown handler — conflicting .gitattributes blocks setup' integration test from shutdown-handler.test.ts (the conflict path it pinned no longer exists). Drop the two merge-driver-specific tests from distill-workspace.test.ts (shell-quoting + space-in-vault-path). Rewrite auto-setup.test.ts to drop GITATTRIBUTES_LINES / NAPKIN_MERGE_DRIVER / G7 conflict-detection coverage and update the 'fresh vault' / 'partial-setup' / FB-2 tests so .gitignore is the only scaffolded file. Migration policy (locked per design.md 'Migration', user direction 2026-05-15): manual cleanup, no automatic removal. Existing vault .gitattributes files retain the now-orphaned '*.md merge=napkin-distill-merge' line — git falls back to its built-in merge driver once the script is gone, so the line becomes inert, not harmful. Auto-setup change removes only the install path; no remove-old-rule path is added. README will document manual cleanup steps in Phase D (D1). Net delta: -1960 / +58 LOC. Tests: 423 pass / 12 skip / 0 fail (down 31 tests vs Phase A tip 454: -25 from scripts.test.ts deletion, -3 from distill-workspace.test.ts merge-driver tests, -1 from shutdown-handler.test.ts G7, -2 net from auto-setup.test.ts G7 + GITATTRIBUTES rewrites). bunx tsc --noEmit clean. bun run lint clean. Refs: design.md 'PR scope table > Phase B > B1' / 'What gets deleted' / 'Migration'. Closes CLEAN-A-8, CLEAN-5 (R3) from deferred.md.
PR #12 Phase B item B2 — remove tests that exercised the now-deleted napkin-distill-merge driver and the wrapper salvage / merge-driver code paths that B3 will trim. With the agent owning merge resolution end-to-end, these tests pin behaviors that no longer exist. Deleted from extensions/distill/spawn-distill-in-worktree.test.ts: the 'distill-wrapper.sh (partial-merge salvage)' describe.skip block (3 tests, NAPKIN_DISTILL_MERGE_MOCK=fail-driven salvage flow), the 'distill-wrapper.sh (MERGE_HEAD escape-hatch)' describe.skip block (2 tests, FORCE_MERGE_HEAD/FORCE_MERGE_RC testing-hook coverage), the 'distill-wrapper.sh (LLM-resolved conflict, end-to-end)' describe.skip block (2 tests, NAPKIN_DISTILL_MERGE_MOCK=ok happy-path coverage), and the four pending-Phase-C 'happy path' / 'empty distill' / 'POST-CONV-1' / 'concurrent worktrees' test.skip entries plus their now-orphaned helpers (runWrapper, createWorkspaceWithChanges) inside the integration describe. Also dropped the master-default-branch test.skip (Phase B/C-annotated). Phase C will replace the four happy-path / no-content / pi-self-commit / concurrent slots with bash-stub mocked-pi fixtures that exercise the agent's behavior space. Side-effect cleanup: 'distill-wrapper.sh (non-main default branch)' describe block lost its only consumer of sessionFile (the deleted master-default test.skip), so the let-binding was removed; the createSeededSessionFile call is retained for its side-effect of seeding the SessionManager disk fixture. scripts.test.ts deletion was pulled forward into B1 to keep tests green between commits (it imports MERGE_DRIVER_SCRIPT, which B1 removed). FORCE_MERGE_HEAD/FORCE_MERGE_RC search came up empty in wrapper-validation.test.ts — those were already absent. Net delta: -1145 / +44 LOC. Tests: 411 pass / 0 skip / 0 fail (down from 423 pass / 12 skip after B1; -12 from the test.skip / describe.skip deletions). bunx tsc --noEmit clean. bun run lint clean. Refs: design.md 'PR scope table > Phase B > B2' / 'What gets deleted'.
…paths (B3) PR #12 Phase B item B3 — remove the residual merge-driver-specific code paths in the wrapper and the JS-side dispatch arm. With B1 + B2 done, no caller invokes any of these paths; this commit deletes them. Wrapper (extensions/distill/scripts/distill-wrapper.sh): drop the 'source git_retry.sh' line and the HERE script-dir resolution that only existed to load it (CI-A-1 R1, CI-1 R2, CORR-8 R3); refresh the 'single fatal-error log per branch' comment to drop the now-stale 'PR #12 removes the partial-merge log' wording. NAPKIN_DISTILL_MERGE_TIMEOUT_SECS / NAPKIN_DISTILL_MERGE_MOCK / MERGE_HEAD escape-hatch / partial-merge log emission / salvage-via-checkout-main paths were already absent from the post-Phase-A wrapper (Phase A's A2 rewrite never carried them forward; only the source-git_retry stub remained, hence single-line wrapper change here). Helper script: delete extensions/distill/scripts/git_retry.sh (65 LOC) entirely — no callers remain after the wrapper's source line is gone. Also drop the GIT_RETRY_SCRIPT export from extensions/distill/scripts-paths.ts (only referenced by the deleted scripts.test.ts in B1). JS-side cleanup (CLEAN-2 R3 / CORR-7 R2 / CORR-10 R3 — partial-merge JS-arm cleanup): drop the partialMergeLogPath field from the DistillOutcome interface in distill-workspace.ts, drop the partial-merge-specific parser branch in findDistillOutcomeForBranch (the .partial-merge.log path lookup), drop the 'partial-merge' switch arm + readPartialMergeLog plumbing from formatOutcomeNotification in index.ts, and update its docstring/severity-contract comment in index.ts and routing.test.ts to drop the partial-merge row. The merged-local row is now documented alongside merged-content / no-content / failed:<reason>. Test updates: drop the four partial-merge tests + the partialMergeLogPath:null fixture cruft in index.test.ts (formatOutcomeNotification suite). Drop the two partial-merge tests in error-log-surfacing.test.ts (findDistillOutcomeForBranch suite); rename the surviving 'returns class + path, no partial-merge log' tests to 'returns class + path' since the field is gone. Retain the two R8-CC-1 .partial-merge.log filter tests (findDistillErrorLogForBranch / assertNoWrapperFailures) — they pin the defensive filter that excludes orphaned .partial-merge.log files left over from PR #11 vaults. What's NOT deleted (deliberately): the auto-setup.ts doc-comment + .gitignore-only-scaffold comment that explain the migration policy ('PR #12 deleted that driver — the distill agent now resolves merges itself in its worktree...'). These are user-facing migration notes that Phase D's README will mirror; deleting them would lose context. The 'napkin-distill-merge' string only appears as policy explanation, not as a live identifier. Net delta: -249 / +24 LOC. Tests: 405 pass / 0 skip / 0 fail (down from 411 after B2; -6 from the merge-driver dispatch / partialMergeLogPath test deletions). bunx tsc --noEmit clean. bun run lint clean. Refs: design.md 'PR scope table > Phase B > B3' / 'What gets deleted'. Closes CLEAN-A-10, CLEAN-2 (R3), CORR-7 (R2), CORR-10 (R3), CI-A-1, CI-1 (R2), CORR-8 (R3) from deferred.md.
…driver (CORR-1, CLEAN-1, CLEAN-2, CLEAN-3, CORR-2) Phase B Round 1 surfaced 4 medium-severity stale-prose findings across 3 reviewers (cross-reviewer consensus). Per methodology never-deferrable category (stale documentation), comment refresh is mandatory in this PR. Refreshed 5 doc-comment sites in production code: distill-workspace.ts:712 (wrapper-failure-log description), index.ts:996 (forensic log description), index.ts:1125 (runAutoDistill JSDoc), index.ts:1179 (worktreeSpawnFn JSDoc), auto-setup.ts:351 (seedless-init corner-case comment). Each refresh removes references to the deleted merge driver / merge-driver 3-strike / .gitattributes routing and replaces with the agent-driven flow per design.md "Locked decisions". Comment-only changes; behavior unchanged. Authored by Phase B Pass 1 fixer subagent (terminated mid-commit at 7.1m). Orchestrator persisted the pre-authored changes via git add + commit (state management, not authoring) per methodology's "final one-line polish" allowance.
…ixture (CLEAN-4) These env vars were consumed by git_retry.sh which Phase B deleted (B3, fac0cb1). They became inert no-ops in this test fixture. Removed lines 819-820 of spawn-distill-in-worktree.test.ts. Test still passes — the env vars had no observable effect post-deletion. References: CLEAN-4 (B-R1) per Phase B Round 1 cleanness review.
Adds extensions/distill/test-fixtures/agent-stubs/ with 10 executable bash scripts simulating each behavior class from the design's Mocked-pi behaviors testing plan: clean-distill, conflict-resolve-clean, conflict-leave-markers, no-distill, squash-skipped, multiple-commits-on-main, pushed-success, push-fail-merged-local, agent-timeout, agent-crashes. Each fixture is self-contained, reads NAPKIN_STUB_* env vars from the test harness for vault/worktree/branch paths, and produces deterministic filesystem effects (commits, marker files, etc.) so the wrapper's full pipeline (post-validation, salvage, outcome dispatch) can be exercised without burning real LLM tokens. Adds an exclusion to package.json files so the test-fixtures dir stays out of the published bundle (verified via bun pack --dry-run). Phase A's existing inline writePiStub patterns in wrapper-validation.test.ts and wrapper-salvage.test.ts are left in place; this commit only adds the formal fixture surface. C2 wires tests against these fixtures. Per design spec PR scope table > Phase C > C1.
Adds extensions/distill/agent-driven-merge.test.ts with 10 integration tests, one per Mocked-pi behavior class, driving the wrapper end-to-end against the formal fixtures from C1. Each test points NAPKIN_DISTILL_PI_BIN at a fixture file and asserts the resulting outcome class + sidecar contents. Extends runWrapperWithStub in _test-helpers.ts with an opts.fixturePath knob that bypasses the inline writePiStub step and auto-injects NAPKIN_STUB_VAULT/_WORKTREE/_BRANCH/_DEFAULT_BRANCH env vars derived from the workspace, so fixtures can reach the test scaffold without JS-side template-string interpolation. Inline-stub callers ignore these env vars; behavior unchanged for existing tests. Coverage relationship to existing tests: wrapper-validation.test.ts and wrapper-salvage.test.ts already cover most validators/salvage paths in isolation. This file adds a higher-level integration view + fills four genuine gaps that inline-stub tests don't reach: (a) conflict-resolve-clean — actual git-merge-with-conflict + agent resolves + squashes, (b) squash-skipped — agent commits to worktree distill branch but never squashes, (c) multiple-commits-on-main — wrapper accepts >=1 commits per validate_commit_count contract, (d) pushed-success — origin configured + agent pushes successfully (complementing the existing merged-local test that covers the no-push case). Test count: +10 (405 → 415). Per design spec PR scope table > Phase C > C2.
Adds a toMatchSnapshot regression guard over buildDistillPrompt(SAMPLE_INPUTS) to catch unintended drift in the rendered prompt prose. Existing tests in distill-prompt.test.ts assert specific substrings (10 numbered step markers, force-push prohibition, pull-merge-not-rebase, worktree-isolation prefix); the snapshot pins the FULL document at canonical sample inputs so a copy-edit that softens a directive (e.g. "never use --force" → "avoid --force") is caught even if the named substrings still match. Snapshot file lands at extensions/distill/__snapshots__/distill-prompt.test.ts.snap (Bun's default location, sibling to the test file). The snapshot is checked into git; intentional prompt edits will require regenerating with bun test -u and the diff is reviewable per commit. Verified the snapshot file is excluded from the published bundle via bun pack --dry-run (no extensions/**/__snapshots__/** match in package.json files). Test count: +1 (415 → 416). Per design spec PR scope table > Phase C > C3.
Audit of wrapper-validation.test.ts and wrapper-salvage.test.ts post Phase A+B identified two genuine gaps: (1) validate_head_on_default has TWO distinct failure paths (HEAD on a different branch vs HEAD detached). Existing tests covered the feature-branch case via 'git checkout -b'; the detached-HEAD case (which routes the same failed:head-not-on-default outcome class but emits a detached-specific salvage log line) was untested. The new test pins the detached-HEAD diagnostic so a future refactor can't collapse the two paths and silently regress the diagnostic. (2) Multi-validator dispatch ordering — when the agent leaves the vault in multiple invalid states simultaneously (e.g. both wrong HEAD AND markers), the wrapper's documented dispatch order is: head_on_default → no_markers → commit_count → local_only. Earliest validator wins. The new test pins head-vs-markers ordering: an agent that commits a marker-bearing file AND switches off the default branch must report failed:head-not-on-default, not failed:markers-after-agent-exit. This matters for forensic clarity — the outcome class drives the recovery hint, so a misordered dispatch would point users at the wrong recovery action. Other validators (validate_no_markers, validate_commit_count, detect_local_only, safe_rm_worktree) were verified comprehensively covered by existing tests in wrapper-validation.test.ts and wrapper-salvage.test.ts; the seven failed:* reason codes (markers-after-agent-exit, pre-existing-markers, internal-validator-error, head-not-on-default, agent-exit-nonzero, agent-timeout, divergent-history) all have recovery-hint assertions in either suite. The internal-validator-error reason code (Phase A Pass 3) is also covered (wrapper-validation.test.ts:511). Test count: +2 (416 → 418). Per design spec PR scope table > Phase C > C4.
…very flow (PR #12 C-R1 CORR-1) design.md "Mocked-pi behaviors" #8 calls out the recovery branch where the agent's first push fails (origin advanced during distill) and the agent recovers via 'git pull --no-rebase' + push. Phase C's push-fail-merged-local.sh covers the OTHER valid path (agent gives up on push, outcome merged-local) but the recovery-succeeds path was uncovered — the --no-rebase invariant in the prompt was not exercised end-to-end. New fixture push-fail-pull-merge-success.sh: a. agent commits on the worktree distill branch b. squashes onto vault default c. first 'git push' — fails non-ff (test pre-arranges origin advance) d. 'git pull --no-rebase origin <default>' folds origin's commit in e. second push — succeeds The fixture writes a sentinel file recording the exact pull command it ran so the test can assert --no-rebase was used (the design's never-rebase-main invariant; users with pull.rebase=true globally would otherwise rewrite local main silently). New helper advanceOriginFromSideClone in agent-driven-merge.test.ts clones origin to a side directory, commits, pushes back, simulating a teammate landing a commit during the distill window. New test pins: - outcome merged-content (recovery succeeded; not merged-local) - localSha === originSha (final push landed) - sideSha is ancestor of local main (merge actually folded it) - sentinel contains 'pull --no-rebase origin main' (--no-rebase pinned) - distilled file 'pulled-merged.md' is on default push-fail-merged-local.sh is preserved (different valid scenario). Findings addressed: CORR-1 (Phase C Round 1, high). Tests: 418 -> 419 (1 new).
…to default (PR #12 C-R1 CORR-2) design.md "Mocked-pi behaviors" #6 specifies that when the agent commits 2+ times directly to the default branch (rather than commit to its branch + squash-merge to default), the wrapper: - ACCEPTS the outcome as merged-content (the dispatch is no-content vs has-content; the squash invariant is a soft suggestion in the prompt, not a hard wrapper constraint), AND - LOGS A WARNING so the forensic record reflects the violation. The wrapper currently does (a) but not (b). Add (b). Critical design choice: warnings live in a SEPARATE <base>.warning.log file, NOT the fatal .log file. The JS-side findDistillErrorLogForBranch matches only suffix '-<branchShort>.log', so '<base>.warning.log' is safely ignored by the failure-surfacing poller \u2014 a merged-content run with a warning attached stays a success in the UI. The naming mirrors the existing .partial-merge.log precedent (R8-CC-1). Changes: - distill-wrapper.sh: new log_warning helper that lazy-creates a sibling .warning.log with its own header (mirrors log_error structure). Fires when validate_commit_count > 1 right after the no-content early-exit and before merged-local detection. - agent-driven-merge.test.ts: extend multiple-commits-on-main test to assert .warning.log exists with WARNING text + that the fatal .log file does NOT exist (no false-positive failure surface). - error-log-surfacing.test.ts: add parallel test pinning that findDistillErrorLogForBranch ignores .warning.log files. Findings addressed: CORR-2 (Phase C Round 1, medium). Tests: 419 -> 420 (1 new in error-log-surfacing; 1 existing extended).
…s (PR #12 C-R1 CI-1, CLEAN-1) Cross-reviewer consensus (CI portability + cleanness-blind): the test-fixtures/agent-stubs/README.md was shipping in the npm bundle despite a '!.../test-fixtures/**/README.md' exclusion line. Root cause: order in the files array matters when broad include globs and narrow exclude globs interleave. The previous layout was: "extensions/**/scripts/**", "!extensions/**/test-fixtures/**", "extensions/distill/distill-prompt.md", "extensions/**/README.md", "!extensions/**/test-fixtures/**/README.md", The 'extensions/**/README.md' broad include re-pulls the test- fixtures README into the bundle. The narrow '!.../README.md' exclusion that follows is more-specific but doesn't override the broader include because npm pack's matching evaluates them in sequence; verified by 'npm pack --dry-run' showing the README in the bundle. Fix: collapse the two test-fixture exclusion lines into one broad exclusion placed AFTER the README include, so the exclusion is the last directive applied to test-fixture paths: "extensions/**/scripts/**", "extensions/distill/distill-prompt.md", "extensions/**/README.md", "!extensions/**/test-fixtures/**", One exclusion covers all test-fixture files (including README and the bash stubs) without needing the second narrow exclusion. Verification: before fix: npm pack --dry-run | grep -E 'test-fixtures|agent-stubs' \u2192 1 match after fix: npm pack --dry-run | grep -E 'test-fixtures|agent-stubs' \u2192 0 matches Bundle still includes all expected sources (extensions/**/*.ts, scripts, distill-prompt.md, extensions/**/README.md, skills, top- level README, LICENSE). Findings addressed: CI-1 + CLEAN-1 (Phase C Round 1, medium, cross-reviewer consensus). Tests: 420 (no test changes \u2014 config-only fix).
…pperWithFixture (PR #12 C-R1 CI-2, CLEAN-3) Cross-reviewer consensus (CI portability + cleanness-blind): the test-fixtures/agent-stubs/README.md referenced 'runWrapperWithFixture' in _test-helpers.ts, but the actual exported helper is 'runWrapperWithStub' which accepts an 'opts.fixturePath' parameter to point at a formal fixture file. The 'fixture' in the name lives in the option, not the function name. Fix the README's env-var contract section to point at the real helper signature so a developer reading the docs can find the code. Findings addressed: CI-2 + CLEAN-3 (Phase C Round 1, low, cross-reviewer consensus). Tests: 420 (no test changes \u2014 docs-only fix).
Replace merge-driver-centric documentation with content reflecting the locked decisions in features/pi-napkin-distill/pr-12-agent-driven-merge/design.md. The agent now owns the full distill -> merge -> squash -> push -> cleanup lifecycle; the wrapper validates and salvages but never runs a per-file LLM merge driver. Sections rewritten in README.md: - New 'Requirements' subsection (closes CI-A-9 / R1 deferred): bash 4+, timeout(1) / gtimeout, git 2.20+, configured pi provider. - 'Auto-init on first use' and 'How it works' no longer claim a .gitattributes scaffold is written (matches auto-setup.ts:281-283 in PR #12). - 'Concurrency > LLM merge driver' and 'Concurrency > Partial-merge salvage' DELETED. Replaced with 'How distill resolves conflicts' (5-step flow: wrapper sets up, agent distills+merges+squashes+pushes, wrapper validates+salvages) and 'Salvage when validation fails' (V3-locked never-touch-main strategy). - New 'Outcome classes' subsection enumerates merged-content / merged-local (NEW) / no-content / failed:<reason> with the six known reason codes (markers-after-agent-exit, pre-existing-markers, head-not-on-default, divergent-history, agent-exit-nonzero, agent-timeout) and per-class recovery actions. - Troubleshooting > 'Merge driver fails repeatedly' replaced with 'Distill keeps failing (failed:<reason>)' pointing at the new outcome table. - 'Testing hooks' env var table refreshed: removed deleted vars (NAPKIN_DISTILL_MERGE_MOCK, NAPKIN_DISTILL_FORCE_MERGE_HEAD, NAPKIN_DISTILL_FORCE_MERGE_RC); added the surviving Phase A/B set including NAPKIN_DISTILL_TIMEOUT_KILL_GRACE_SECS, NAPKIN_DISTILL_FORCE_CLEANUP, NAPKIN_DISTILL_NO_RECURSE. - New 'Migration from PR #11' top-level section documenting the manual cleanup steps (per design.md 'Migration': single-user PR #11; orphaned .gitattributes rule is inert once driver is gone). - New 'Maintenance > Verifying the agent prompt against a real LLM' subsection naming the bun run verify:agent-prompt entrypoint added in D2. skills/napkin/SKILL.md left unchanged: it documents napkin CLI vault resolution, not the distill merge mechanism, and contains no stale references. Tests: docs-only change, no runtime impact. 420 pass / 0 skip / 0 fail unchanged.
…) (PR #12 D2) Adds scripts/verify-agent-prompt.ts and a 'verify:agent-prompt' npm-script entry. Replays the V2 fixture (research/v2-v3-verification.md) against the configured real LLM and asserts the same 6 post-conditions, exiting 0 on PASS / 1 on FAIL. This is the (b) gate from PR #12's design.md 'CI vs ad-hoc V2 replay': manual on-demand re-validation when the prompt at extensions/distill/distill-prompt.md is edited or a model is rotated. CI uses the bash-stub fixtures from Phase C (extensions/distill/test-fixtures/agent-stubs/) — this script is intentionally NOT wired into CI to avoid token spend on every commit. Behavior: - Creates an ephemeral tmpdir vault at $TMPDIR/napkin-verify-agent-prompt-XXXXXX (cleanup automatic; --keep-tmpdir for forensic inspection). - Sets up two divergent commits on note.md (one on main, one on a distill/verify-<sha> branch) so the agent's step 7 git merge is guaranteed to conflict. - Builds the prompt via the actual buildDistillPrompt(...) export (not a custom V2-only prompt) — exercises the real template + placeholder substitution + bundled .md path resolution. - Resolves the model in priority order: --model flag > vault's distill.model from $XDG_CONFIG_HOME/napkin/config.json's vault > kiro/claude-sonnet-4-6 default. - Spawns 'pi --session ... --model ... -p $PROMPT' under a 300s wall-clock cap (configurable via --timeout-secs) with NAPKIN_DISTILL_NO_RECURSE=1 set. - Asserts 6 post-conditions per V2: (a) no conflict markers in *.md, (b) vault HEAD on default branch, (c) agent's squash commit landed (rev-list count >= 2 after the fixture's 1 setup commit), (d) distill branch removed, (e) worktree removed, (f) note.md resolved with content from at least one source version (sanity-check the agent didn't blank the file). Prints a checklist with ✓/✗ per condition + final RESULT line. Per task spec, no new dependencies. Uses node:child_process spawnSync, node:fs, node:os, node:path. The pi binary is resolved from PATH (or $NAPKIN_DISTILL_PI_BIN if set, matching the wrapper's testing-hook convention). Verification: 'bun run verify:agent-prompt -- --help' prints usage cleanly and exits 0; bunx tsc --noEmit clean; biome check clean (formatting auto-applied). Documentation: README.md's new 'Maintenance > Verifying the agent prompt against a real LLM' section (added in D1, ab89fc6) names this entry-point and points at the .md file to edit when iterating the prompt. Scope: dev-tool only. The script is at scripts/ which is NOT in package.json's 'files' array, so 'npm pack --dry-run' confirms 15 files bundled (unchanged from pre-D2).
…pl (PR #12 D3) design.md's 'Mocked-pi behaviors' #5 originally specified outcome 'failed' for the squash-skipped case (agent commits to the distill branch in the worktree but never squashes to the default branch). The implementation produces 'no-content' instead — and that's the correct answer. Reasoning (decision: align spec to impl): - From main's perspective, no new commits since startSha = no content was integrated. That is exactly what 'no-content' means in the wrapper's outcome dispatch (validate_commit_count returns 0). - Adding a 'failed:squash-skipped' reason code would require new wrapper logic to distinguish 'agent committed in the worktree but skipped squash' from 'agent decided nothing was worth capturing'. The wrapper has no signal to tell those cases apart without inspecting the deleted distill branch in the reflog — which is fragile and racy. - The user-facing notification semantics for 'no-content' (warning: 'distill ran but saved no content') is the right UX for both cases. The forensic distinction is recoverable from the reflog if needed. Changes in this commit: - extensions/distill/test-fixtures/agent-stubs/squash-skipped.sh: header comment expanded to cite CORR-3 and document the alignment decision in-line with the fixture itself. - extensions/distill/agent-driven-merge.test.ts: 'Behavior 5: squash-skipped' comment block expanded to cite CORR-3 / Phase C R1 and pin the impl-side contract. - README.md (already updated in D1, ab89fc6): the 'Outcome classes' table's no-content row already lists 'committed to the distill branch but skipped squash (default branch never moved)' as one of the no-content cases. No further README update needed. Not modified (per task spec — orchestrator's responsibility post-merge): - features/pi-napkin-distill/pr-12-agent-driven-merge/design.md (Mocked-pi behaviors #5 currently says 'failed'; orchestrator will update to 'no-content'). - features/pi-napkin-distill/pr-12-agent-driven-merge/deferred.md (CORR-3 row tracker reconciliation). Tests: 420 pass / 0 skip / 0 fail unchanged. The squash-skipped → no-content assertion was already in place from Phase C; this commit only adds documentation linking it to CORR-3.
Five docs-only fixes flagged by Phase D Round 1 reviewers (correctness + cleanness-blind). No behavior changes; bun test stays at 420 pass / 0 fail.
- CLEAN-1 + CORR-1 (cross-reviewer consensus medium): the maintenance section pointed at scripts/verify-agent-prompt.sh but the actual file shipped in D2 is verify-agent-prompt.ts (TypeScript so it can import buildDistillPrompt from the production module). Fix the extension.
- CLEAN-2 (medium): the failed:<reason> table was missing internal-validator-error, which the wrapper has been emitting since Phase A Pass 3 when post-distill mktemp fails. Add a row describing the condition (full disk / locked-down TMPDIR ⇒ post-distill marker scan never ran) and the recovery action (manually inspect the vault for unresolved markers before relying on the squash; revert HEAD if needed; reflog keeps the distill content for ~90 days).
- CLEAN-3 (low): two README cross-references pointed at features/pi-napkin-distill/.../design.md and features/pi-napkin-distill/builder-deleter — paths that don't exist in the published repo (the design doc lives in the maintainer's vault, not git). Drop the parenthetical at line 428 (the surrounding migration prose stands alone) and trim the dangling 'See [features/...] (design pending)' link at the end of the builder-deleter section.
- CLEAN-5 (nit): the maintenance section claimed verify:agent-prompt asserts 4 post-conditions ('no skipped procedural steps, no conflict markers, HEAD on default, distill branch removed') but the script asserts 6 and 'no skipped procedural steps' isn't one of them. Reword to 'asserts the wrapper's documented post-conditions (see the script header for the full six-item list: no conflict markers, HEAD on default, agent's squash committed, distill branch removed, worktree removed, conflicted note resolved cleanly)'.
All findings: README.md only. No code changes; tsc --noEmit clean; biome check clean.
…-Fix-1) Three docstring-vs-code drift fixes flagged by Phase D Round 1 reviewers (correctness + cleanness-blind). No code changes; bun test stays at 420 pass / 0 fail. - CORR-2 (low): the file-header docstring slot (e) promised the script asserts 'No --force push attempted', but assertPostConditions actually checks 'worktree removed from git worktree list' (the fixture has no remote, so the V2 force-push slot is unverifiable; the substitution picks up an actually-meaningful design.md post-validation requirement). Update the docstring to describe what the code asserts, with a one-line note on why the substitution was made. - CORR-3 (nit): the docstring said 'rev-list --count > 0' for slot (c), but the code requires >= 2 because setupFixture pre-adds one 'main edit' commit before pi runs (so any agent squash lands as the second commit). The inline comment in assertPostConditions already explains this; only the file-header was stale. Update to '>= 2' with the rationale spelled out. - CLEAN-4 (low): the docstring referenced features/pi-napkin-distill/.../design.md (line 8) and research/v2-v3-verification.md (line 39) — paths that don't exist in the published repo. Drop both parentheticals; the surrounding prose stands alone, and the design context is captured in PR #12 itself. All findings: scripts/verify-agent-prompt.ts docstring only. No runtime impact; tsc --noEmit clean; biome check clean.
…e-merge gate) Phase A Pass 2A's CLEAN-5 fix added an explicit no-content branch at step 7 of distill-prompt.md. The verify-agent-prompt fixture invoked pi against an empty --session, and the agent (correctly) entered the no-content branch — skipping merge/squash and failing the post-condition that the agent's squash should land on default. Pre-populate <tmpdir>/session.jsonl with a synthetic 6-message user-assistant conversation about Node spawnSync EPIPE/SIGPIPE handling. The agent now reads a substantive conversation, decides per 'Be selective' that yes-this-merits-capture, and proceeds through step 7's content path. Verified: 3 consecutive runs of bun run verify:agent-prompt all pass 6/6 post-conditions (171s, 166s, 163s). Pre-merge gate now holds.
…ite race Reproduces the production bug where 'Distillation terminated abnormally — no outcome record' fires falsely after a successful merged-content distill. Cause: the agent runs step 10 (`git worktree remove`) before the wrapper writes the outcome sidecar; the JS-side poller in `runDistillWith` ticks during the [worktree-gone, outcome-written) window and `findDistillOutcomeForBranch` returns null, which `formatOutcomeNotification` classifies as 'terminated abnormally'. This test FAILS on current main (race triggered, 6 expects, last assertion observes outcomeAtRaceWindow=null) and will PASS after the fix lands. Do NOT skip this test in CI — the failing assertion pins the regression so a future 'fix' that doesn't actually close the race window cannot silently regress. Test approach: Strategy A — spawn the wrapper detached (mirroring `spawnDistillInWorktree`'s production shape: `detached:true` + `unref()`) and poll worktree disappearance from JS at 50 ms intervals, snapshotting the outcome file's existence at the exact tick where production's poller would call `checkOutcome`. The new fixture `step10-race.sh` widens the race window deterministically by sleeping 0.5 s after `git worktree remove` and before exiting, which keeps the wrapper blocked on the agent subprocess long enough for the JS poller's tick to land inside the window. Why existing bash-stub fixtures miss this: none of them call `git worktree remove`, so the worktree-disappearance signal coincides with the wrapper's EXIT trap (which fires after `write_outcome`), and the race window never opens in tests.
…ies to _test-helpers.ts
Move the fake UI factory (notifyCalls + setStatusCalls capture) and the
spy-style ExtensionAPI factory out of routing.test.ts into the shared
_test-helpers module so other consumers (notably the upcoming verify:e2e
script) can reuse them without duplicating the boilerplate.
The fake UI preserves the existing { msg, severity } notification shape,
keeping the ~10 c.msg.startsWith(...) consumer references in
routing.test.ts unchanged. routing.test.ts collapses three local
definitions (the inline UI fake, the makeUI() factory, and the
makeMockExtensionAPI() function) to a pair of imports.
…ntime gate
verify-e2e exercises the production runtime end-to-end: the manual
/distill command handler triggers runDistillWith, which spawns the real
wrapper subprocess via worktreeSpawnFn, the wrapper invokes pi -p
against a real LLM, and the JS-side setInterval poller observes the
wrapper's outcome sidecar and dispatches a UI notification through the
captured fake UI.
Strict superset of the prompt-only gate it replaces:
- The prompt-only gate spawned pi synchronously via spawnSync, bypassed
the wrapper, and ran no JS-side polling. It asserted on filesystem
post-conditions only.
- verify:e2e covers all those filesystem post-conditions PLUS the
wrapper's outcome class AND the JS-side notification severity AND the
bare-origin push (which the prompt-only harness couldn't validate).
The fixture uses a sibling-layout config with vault.root: '..' and
distill.enabled = true so runDistill routes through worktreeSpawnFn
(rather than legacy-embedded fallback). Origin is bare so the wrapper's
push lands without receive.denyCurrentBranch=ignore — non-bare init
would steer the outcome to merged-local instead of merged-content.
Session ownership: SessionManager.create() allocates the path,
writeSyntheticSession() writes the JSONL content. The wrapper's
session-fork step reads from disk via getSessionFile(); content on disk
before the wrapper runs is what matters.
Trigger: captured.commands.distill.handler('', ctx) — bypasses the
60-minute auto-distill interval; the manual handler invokes runDistill
immediately.
Helper extraction: writeSyntheticSession (and the synthetic 6-message
conversation it embeds) moves to scripts/_e2e-helpers.ts so the on-demand
harness keeps the JSONL format knowledge in one place.
…pper handles it Step 10 instructed the agent to run `git worktree remove` itself, but the wrapper's EXIT trap already removes the worktree unconditionally after writing the outcome sidecar. Two cleanup paths running back-to-back (agent then wrapper) opens a race window: the JS-side poller in `runDistillWith` ticks every ~2s and observes worktree disappearance (caused by the agent's step 10) before the wrapper has finished post-validation and written the outcome. `checkOutcome` then returns null, dispatching a spurious "Distillation terminated abnormally — no outcome record" warning on a successful merged-content distill. Drop step 10 from the prompt. The wrapper has always been the actual cleanup mechanism; the "agent owns cleanup" claim in PR #12's original design was aspirational. Keeping one cleanup path (wrapper-side, after `write_outcome`) closes the race structurally without losing any capability. Test changes match the prompt: - 9 numbered steps, not 10 - Negative assertion: prompt does NOT contain agent-side worktree-remove or branch -D directives - Step 7's no-content branch: 'skip steps 7-9 entirely and exit' - Snapshot regenerated The salvage path has the same race surface (worktree removed before `write_outcome`); a follow-up commit reorders salvage to write outcome first.
…oval The stub at test-fixtures/agent-stubs/step10-race.sh used to mirror the agent's old step 10 (`git worktree remove --force` followed by `git branch -D`) to reproduce the production race. Now that the agent prompt has dropped step 10 (the wrapper's EXIT trap is the sole worktree-removal path on the happy path), the stub no longer needs to remove the worktree itself — leaving it matches post-fix production behaviour exactly (agent commits content + exits; wrapper handles cleanup). Keep `sleep 0.5` after the agent's commit work so the wrapper's post-validation + write_outcome + EXIT-trap cleanup chain happens at a known wall-time offset relative to the JS-side poll. This widens the [agent-exit, worktree-removed] gap deterministically for the happy-path case in the upcoming wrapper-invariant test. Update race-step10-cleanup.test.ts: - Docstring rewritten in timeless framing: regression guard for the wrapper invariant 'write_outcome runs before any worktree removal on the happy path', not 'reproduces the bug'. - Step-10 references retained as historical context (the test originated as the deterministic reproducer for the agent-step-10 race). - Assertion message updated to flag the invariant directly rather than the historic agent-side cause. - Test name no longer says 'FAILS on current main — bug'. The race-step10-cleanup test transitions FAIL→PASS with this commit because the modified stub no longer opens the race window the test was designed to catch. The broader wrapper invariant (covering both happy and salvage paths) is pinned by a new wrapper-invariant.test.ts in the next commit.
…e ordering Pin the wrapper invariant: `write_outcome` runs BEFORE any worktree- removal step anywhere in the wrapper. The JS-side poller in runDistillWith watches the worktree path for disappearance — that's its completion signal. If the wrapper removes the worktree before writing the outcome sidecar, the poller's tick can land in the gap and observe `worktree-gone AND outcome-not-written`, dispatching a spurious 'terminated abnormally' warning on what was actually a successful (or cleanly-failed) distill. This commit covers the happy path (wrapper EXIT trap fires AFTER write_outcome). The salvage path is pinned by a follow-up commit. Mechanism: spawn the wrapper detached (mirrors spawnDistillInWorktree's production shape), poll the filesystem at 50 ms intervals, snapshot the outcome file at the moment the worktree disappears. Note: this test does NOT use runWrapperWithStub from _test-helpers.ts because that helper uses spawnSync, which blocks the test until the wrapper has fully exited — by that time the worktree is long gone, the outcome is long written, and the mid-execution race window is invisible. The raw spawn + child.unref() + concurrent polling pattern (proven in race-step10-cleanup.test.ts) is required to observe the filesystem state at the exact moment the worktree disappears. Reuses test-fixtures/agent-stubs/step10-race.sh as the fixture — it already commits content + sleeps + exits without removing the worktree itself, exactly the shape needed for the happy-path race- window-widening.
…ome ordering
Pin the wrapper invariant 'write_outcome runs before any
worktree-removal step anywhere in the wrapper' for the salvage
path: stub-pi commits content WITH conflict markers + exits 0;
wrapper's `validate_no_markers` fails and routes into
`salvage("markers-after-agent-exit")`. Assert outcome sidecar
exists with class `failed:markers-after-agent-exit` at the
moment worktree disappears.
This test FAILS on the wrapper's pre-reorder salvage code
because `salvage()` currently removes the worktree (lines
~556-575) BEFORE writing the outcome (line ~623). The JS-side
poller can race-observe worktree-gone with no outcome yet,
dispatching 'terminated abnormally' instead of
`failed:markers-after-agent-exit`. The next commit reorders
salvage so write_outcome runs first; this test transitions
RED \u2192 GREEN there.
Mechanism details:
- New stub `test-fixtures/agent-stubs/salvage-race.sh` commits
a vault `*.md` file with a complete conflict-marker triple
(`<<<<<<<` / `=======` / `>>>>>>>`), then exits 0. The
wrapper's post-validation detects the markers and routes into
the salvage path with reason `markers-after-agent-exit`.
- New PATH shim `test-fixtures/slow-git.sh` widens the race
window inside the wrapper. Unlike the happy path (where the
stub's `sleep 0.5` widens the [agent-exit, worktree-removed]
gap), the salvage path's race is INSIDE the wrapper itself
(between `git worktree remove` returning and `write_outcome`
being called). The shim runs the real git FIRST so the
worktree disappears immediately, THEN sleeps 0.5 s before
returning to the wrapper \u2014 widens the [worktree-gone,
write_outcome] interval by ~500 ms.
Ordering matters: exec-then-sleep opens the race window;
sleep-then-exec would NOT (worktree wouldn't disappear until
after the sleep, and the wrapper would resume at normal speed
once it does).
- The shim resolves the real `git` via `NAPKIN_SLOW_GIT_REAL_GIT`
(set by the test before mutating PATH). Avoids
PATH-introspection edge cases (BASH_SOURCE+symlink resolution,
recursive shim lookup) inside the shim itself.
- Test-side: stage the shim as `<tmpdir>/git` and prepend the
tmpdir to PATH so the wrapper's bare `git ...` invocations
resolve through it. Cleanup restores PATH and rm -rf's the
tmpdir.
Test count: 423 pass / 1 fail (the new salvage test fails as
RED gate; the happy-path test from the prior commit + the
race-step10-cleanup test continue to pass). The salvage test
transitions to PASS in the next commit.
The wrapper has two cleanup pathways. The EXIT trap (happy path)
already runs write_outcome BEFORE removing the worktree. The
salvage path (failure recovery) had the opposite order: cd out
of worktree, force-remove worktree, prune, branch -D, rmdir
parent, then verify HEAD, compose recovery hint, and write
outcome.
This opens the same race window as the agent-step-10 bug: the
JS-side poller in runDistillWith watches the worktree path and
calls findDistillOutcomeForBranch as soon as fs.existsSync
returns false. With salvage's old ordering, the poller could
race-observe worktree-gone before write_outcome ran, dispatching
a spurious 'terminated abnormally \u2014 no outcome record' warning
instead of the correct 'failed:<reason>' notification.
Reorder salvage() to mirror the EXIT trap:
1. cd out of the worktree (unchanged)
2. Compose recovery hint (case-block) \u2014 moved up
3. write_outcome 'failed:<reason>' \u2014 moved up (before any
worktree-removal step; this is the function-level invariant)
4. Verify HEAD on default; log_error if not \u2014 moved up
5. THEN remove worktree: git worktree remove --force \u2192
safe_rm_worktree \u2192 git worktree prune \u2192 git branch -D \u2192
rmdir parent
Pure motion, ~30 LOC. All existing invariants preserved:
- Best-effort cleanup (each command tolerates failure with
'|| true')
- Never-touch-main (no git checkout / git reset)
- Recovery-hint formatting per reason code (markers-after-agent-
exit, pre-existing-markers, internal-validator-error,
head-not-on-default, agent-exit-nonzero, agent-timeout,
divergent-history)
- log_error lines for HEAD detached / HEAD not on default
Add a function-level docstring noting the invariant explicitly:
'write_outcome ALWAYS runs before any worktree-removal step in
this function, mirroring the EXIT trap's ordering.'
The wrapper-invariant.test.ts salvage-path case transitions
RED \u2192 GREEN with this commit. Test count: 424 pass / 0 fail.
The maintenance section's ad-hoc real-LLM verification step now points at `bun run verify:e2e` (the full-runtime gate that exercises wrapper subprocess + agent + JS-side polling end-to-end) instead of the prompt-only `verify:agent-prompt` predecessor that the package no longer ships. The new description captures the strict-superset post-conditions (notification severity + message, plus filesystem post-conditions including the bare-origin push) and notes the manual / ~$0.50-per-run nature of the gate.
Contributor
Author
|
Wrong target — opening on cad0p fork instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the per-file LLM merge driver with an agent-driven merge architecture. The distill agent now owns the full distill→merge→squash→push pipeline. Removes the per-file merge driver entirely; the agent has the conversation context the driver always lacked.
Includes the race-step10-fix sub-PR (8 race-fix commits added 2026-05-18) that closes a wrapper↔JS-poller race and replaces the prompt-only
verify:agent-promptgate with the full-runtimeverify:e2egate.Architecture changes
maininto distill branch → squash to main → push). Conflicts resolve in the agent's session with full conversation context, not in a stateless merge driver.salvage()(failure path). Agent doesn't run cleanup commands itself — that was the source of the production race fixed in this PR.git checkout/git reseton the user's vault.distill.maxDurationMinutes(default 10) is the only setting agent-driven merge introduces.Why per-file merge driver was removed
Live-debugging PR #11 (
ed8af2f) surfaced two problems:pi -pcall (no cross-process OAuth cache). N conflicted*.md= N × 95s of dead time. A 5-conflict merge ate 8 minutes of prelude alone.Net change: roughly net-zero LOC (~870 LOC merge driver + tests +
.gitattributesregistration deleted; ~870 LOC wrapper validation + salvage + new prompt + bash-stub fixtures + integration tests added). Structural simplification, not raw line reduction.Race-step10-fix sub-PR (commits
c3cc816..92b6a9c)A production distill emitted
Warning: Distillation terminated abnormally — no outcome recorddespite the underlying distill succeeding. Investigation revealed a race between the agent's step-10 worktree removal and the wrapper'swrite_outcome. Same shape in the salvage path.Fix:
salvage()to write outcome BEFORE any worktree removal.write_outcomealways runs before any worktree-removal anywhere in the wrapper.Test architecture:
verify:agent-prompt(prompt-only, 741 LOC) replaced withverify:e2e(full-runtime, 773 LOC) — exercises wrapper subprocess + real-LLM agent + 2-secondsetIntervalJS poller + bare-origin push.wrapper-invariant.test.ts(422 LOC, 2 tests) — uses subprocess + concurrent filesystem polling (rawspawn, notspawnSync-blocking helpers) to pin the invariant on both happy and salvage paths.Methodology lesson (now codified in the methodology guide as an anti-pattern): real-LLM gates are part of orchestrator self-review, not deferred to maintainer. The previous prompt-only gate passed 3/3 against the unfixed code because its shape never exercised the wrapper↔JS-poller seam.
Verification
bun test— 424 pass / 0 fail (was 420/0 on main; +4 tests added by race fix)bunx tsc --noEmit— cleanbun run lint— cleanHOME=$(mktemp -d) bun test— clean (CI-portability gate)bun run verify:e2e— PASS (140.1s wall, 9/9 post-conditions): severity=info, msg="Distillation complete (140s)", outcome=merged-content, origin/main advancedbun test extensions/distill/system-prompt.cache-parity.test.ts— 9/0 (baseline-regression gate)What was deleted
scripts/merge-driver-*.ts(per-file LLM merge driver + tests)scripts/verify-agent-prompt.ts(prompt-only gate; superseded byverify:e2e).gitattributesmerge-driver registrationSub-PR design artifacts
Full design + 5 rounds of design review for the race fix:
Goldmine/features/pi-napkin-distill/pr-12-agent-driven-merge/design.mdGoldmine/features/pi-napkin-distill/pr-12-agent-driven-merge/race-step10-fix/design.mdTest plan
bun run verify:e2eis the on-demand integration gate; orchestrator runs it before declaring ready, ~$0.50/run, single-run policy. Run shape exercises wrapper + real LLM + JS-side polling end-to-end.Closes
Closes #11 follow-up (the merge-driver scaling + provider-prelude problem PR #11 surfaced). The agent-driven merge architecture supersedes the per-file approach.