fix(distill): use pi's default model when none configured by cad0p · Pull Request #12 · Michaelliv/pi-napkin

cad0p · 2026-05-04T21:44:56Z

Make the distill model config optional. When omitted, the spawned pi subprocess uses whatever model pi resolves by default, instead of always falling back to a hardcoded anthropic/claude-sonnet-4-6. Changes: DistillConfig.model is now optional, DEFAULT_DISTILL no longer hardcodes a model, spawnDistill only passes --model when config explicitly specifies one.

…nfig 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.

…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.

…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.

…-3, CLEAN-2) CLEAN-3: rename the legacy DISTILL_PROMPT constant in extensions/distill/index.ts to LEGACY_DISTILL_PROMPT. The constant is used only by the legacy spawnDistill path (argv-based, pre-PR-12); the canonical agent-driven prompt lives in distill-prompt.md and loads via buildDistillPrompt. Both share roughly steps 1-5 (overview/templates/identify/search/daily-note). Naming the legacy one explicitly makes the drift hazard visible — adding a step here without cross-checking distill-prompt.md is now a code-review red flag rather than a hidden invariant. Add a docblock pointing maintainers at the canonical source. Updated: index.ts (constant declaration + the spawnDistill arg site that passes it). spawn-distill-legacy.test.ts comment that references args[6] as 'the DISTILL_PROMPT positional'. The 'DISTILL_PROMPT location' citation in distill-prompt.ts:10 is left intact — it's a quoted design-doc section title, not a code reference. (DISTILL_PROMPT_PATH in distill-prompt.ts is a different constant — path to the .md file — and is unaffected.) CLEAN-2: drop the two NAPKIN_GIT_RETRY_MAX / NAPKIN_GIT_RETRY_DELAY env-var lines from distill-wrapper.sh's Environment header docs. The docs claimed those vars were 'forwarded to git_retry (cleanup paths only)' but PR #12 Phase A removed every git_retry invocation from the wrapper — the agent owns merge/squash/push retries now. The vars are inert in the wrapper. Phase B's CI-1 deletes the lingering source line for git_retry.sh entirely. Both findings are never-deferrable per the methodology guide (stale references / drift hazard). Tests: 455 pass, 12 skip, 0 fail. tsc --noEmit clean. lint clean.

…EAN-1) buildWorktreeDistillPrompt was the worktree-isolation prompt prefix helper from PR #11 (POST-R6-CACHE) but PR #12 moved that framing into distill-prompt.md. The function had zero production callers; only its own unit tests and a misleading comment in spawn-distill-legacy.test.ts referenced it. Originally scoped for Phase B (dead-code cleanup) per the deferred.md tracker, but pulled into Phase A per the methodology guide's re-flagged-findings rule: spec-blind reviewers re-flagged this same dead code 3 times across Round 1 (CLEAN-A-5/SEC-A-11/CORR-A-4 — 3-way consensus), Round 2 (CLEAN-1 R2), and Round 3 (CLEAN-1 R3). The 3rd flag is the signal to improve the code, not to re-decline. Changes: delete buildWorktreeDistillPrompt from distill-workspace.ts; remove the describe("buildWorktreeDistillPrompt (POST-R6-CACHE)", ...) block and import from spawn-distill-in-worktree.test.ts; update the worktree-isolation pin in spawn-distill-legacy.test.ts to reference distill-prompt.md (the actual home of that framing now) and check against the current signature phrase "lives in the git worktree at" instead of the obsolete "isolated git worktree at". Tests: 453 pass / 12 skip / 0 fail (down from 455 by the 2 deleted assertions in the dead-code describe block). tsc clean. lint clean.

…-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.

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.

…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.

cad0p marked this pull request as ready for review May 4, 2026 21:45

Navigation Menu

fix(distill): use pi's default model when none configured - #12

fix(distill): use pi's default model when none configured#12
cad0p wants to merge 1 commit into
Michaelliv:mainfrom
cad0p:feat/distill-default-model-upstream

cad0p commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cad0p commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant