diff --git a/.cfgcaddy.yml b/.cfgcaddy.yml index 25ba8b3d..ac4f8770 100644 --- a/.cfgcaddy.yml +++ b/.cfgcaddy.yml @@ -46,6 +46,9 @@ links: os: "Linux Darwin" - src: .claude/CLAUDE.md os: "Linux Darwin" + - src: .claude/CLAUDE.md + dest: .pi/agent/AGENTS.md + os: "Linux Darwin" - src: .claude/agents os: "Linux Darwin" - src: .claude/agents.md diff --git a/.claude/skills/pi-extension-review/SKILL.md b/.claude/skills/pi-extension-review/SKILL.md new file mode 100644 index 00000000..b2df8927 --- /dev/null +++ b/.claude/skills/pi-extension-review/SKILL.md @@ -0,0 +1,99 @@ +--- +name: pi-extension-review +description: Fork-pin-review gate for adding any third-party Pi extension, package, or MCP source. Use before running fork_pin_extension.py, before hand-editing .config/pi/extensions-manifest.json, or whenever asked to add/enable a Pi extension not already wired via a first-party fragment. +--- + +# Pi Extension Fork-Pin-Review Gate + +No third-party Pi extension, package, or MCP source may render into +`settings.json` without an approved manifest entry. This is enforced in +code, not just convention: `verify_pinned_sources_reviewed()` in +`stapler-scripts/llm-sync/src/sources/review_gate.py` cross-references every +rendered `tstapler`-fork-shaped source against +`.config/pi/extensions-manifest.json` and raises `PiConfigError` unless a +matching entry exists at the exact pinned commit with `disposition == +"approved"`. + +**Only a human hand-edits `disposition`, `approved_by`, and `approved_date`.** +No script or tool in this repo writes the literal string `"approved"` — +`fork_pin_extension.py` always writes `disposition: "candidate"` with +`approved_by`/`approved_date` left `null`, and `ManifestEntry.__post_init__` +in `stapler-scripts/llm-sync/src/sources/extension_manifest.py` raises +`ManifestError` if `disposition == "approved"` is ever constructed without +both approval fields present. Approval is a diff Tyler authors by hand, not +a flag any automation can set. + +## Required gate before enabling any candidate + +Reproduced from `project_plans/pi-dotfiles/extension-audit.md`'s "Required +gate before enabling any candidate" — complete all steps, in order, before +changing `disposition` to `"approved"`: + +1. Fork the exact upstream repository to the Tyler-owned GitHub namespace. +2. Record upstream URL, upstream commit, fork commit, license, package + paths, and reviewer notes. +3. Inspect lifecycle scripts, network destinations, subprocess execution, + filesystem writes, environment access, secret handling, and telemetry. +4. Run upstream tests from the fork commit and add threat-focused + regression tests for the capabilities actually enabled. +5. Pin the Pi package source to the exact fork commit; mutable + branches/tags and upstream npm packages are rejected by `PiConfigSource`. +6. Introduce candidates disabled or in an opt-in fragment first, then + validate in a temporary Pi home before current macOS and Linux rollout. +7. Upgrades repeat this process; no automated upstream advancement. +8. Check maintenance liveness — last commit date, release cadence, and + contributor count — and record the finding in the manifest entry's + `notes` field. An unmaintained upstream must be caught here, not only + during the later capability review. + +## How to run the helper + validator + +`fork_pin_extension.py` scaffolds the mechanical parts of step 1 and 2 — +forking via `gh repo fork` and writing a `candidate` manifest entry — but +has no code path that can write `"approved"`: + +```bash +# Preview the plan; makes no gh calls, writes nothing. +uv run --directory stapler-scripts/llm-sync scripts/fork_pin_extension.py fork \ + / \ + --id \ + --capability "" \ + --dry-run + +# Actually fork and write the candidate entry. +uv run --directory stapler-scripts/llm-sync scripts/fork_pin_extension.py fork \ + / \ + --id \ + --capability "" +``` + +This creates `github.com/tstapler/` and appends an entry to +`.config/pi/extensions-manifest.json` with `disposition: "candidate"`. From +there, steps 2-8 above are manual review work: hand-edit the entry's +`license`, `package_paths`, `reviewer`, `review_date`, and `notes` fields as +you complete each step, citing real file paths as evidence. + +After any manifest or review-gate change, validate with: + +```bash +make llm-sync-test +``` + +This runs every `stapler-scripts/llm-sync/test_*.py` module, including +`test_extension_manifest.py`, `test_review_gate.py`, and +`test_fork_pin_extension.py` — covering the approval-field invariant, the +gate's rejection of unreviewed/mismatched-commit/non-approved sources, and +the no-automated-approval guarantee. + +## When a candidate fails review + +Not every candidate reaches `approved`. If the review at any step above +turns up a blocker — unmaintained upstream, a capability that can't be +scoped safely, a security finding, or a reviewer decision to wait — set +`disposition` to `hold` (revisit later, evidence not yet conclusive) or +`rejected` (reviewed and declined). Both are legal terminal-for-now states +the manifest schema already supports; leaving a rejected or stalled +candidate at `disposition: "candidate"` misrepresents it as still pending +review. `verify_pinned_sources_reviewed()` blocks sync for `hold` and +`rejected` exactly as it does for `candidate` — only `approved` passes the +gate. diff --git a/.config/pi/README.md b/.config/pi/README.md index 95be0925..230e239b 100644 --- a/.config/pi/README.md +++ b/.config/pi/README.md @@ -39,6 +39,10 @@ overlay's own tracked fragment can add its scope there because its internally managed update channel owns that package's cadence. Local package paths are also allowed. +Before forking, pinning, or enabling any third-party extension, follow the +fork-pin-review gate documented in +[`.claude/skills/pi-extension-review/SKILL.md`](../../.claude/skills/pi-extension-review/SKILL.md). + Never put credentials, OAuth state, sessions, trust data, or other mutable Pi runtime state in these files. @@ -85,3 +89,79 @@ uv run --directory stapler-scripts/llm-sync main.py --target pi --dry-run A Pi-only target does not install Claude/Antigravity plugins or rewrite their MCP settings. + +## Package lifecycle + +VERIFIED via a manual temp-`HOME` spike (pi 0.84.4, 2026-09-21), using the +real npm-hosted extension `@gotgenes/pi-permission-system` referenced in +`project_plans/pi-dotfiles/full-featured-profile-research.md`: + +```bash +TMPHOME=$(mktemp -d) +mkdir -p "$TMPHOME/.pi/agent" +find "$TMPHOME" -type f | sort > before.txt # empty + +HOME="$TMPHOME" pi install npm:@gotgenes/pi-permission-system --no-approve +find "$TMPHOME" -type f | sort > after-install.txt +``` + +**Artifact path**: an npm `packages` entry lands under a dedicated npm +workspace at `~/.pi/agent/npm/` — `package.json` (declares the package as a +`dependencies` entry), `package-lock.json`, a `.gitignore` (`*` / +`!.gitignore`), and the installed files under +`~/.pi/agent/npm/node_modules//` plus its own transitive +dependencies as npm siblings under the same `node_modules/`. `pi install` +also rewrites `~/.pi/agent/settings.json`'s `packages` key as a flat array of +source strings (e.g. `{"packages": ["npm:@gotgenes/pi-permission-system"]}`), +not the map-keyed-by-id object shape this file's own layered-config examples +above show — the map shape is this repo's authoring format, and `llm-sync` +must render it down to Pi's actual array-of-strings runtime shape. Installing +also seeds an ordinary npm cache at `~/.npm/` (`_cacache`, `_logs`), which is +npm's own global cache, not Pi-specific state. + +**Removal via `pi remove`**: `HOME="$TMPHOME" pi remove +npm:@gotgenes/pi-permission-system --no-approve` fully prunes the artifact — +it empties `settings.json`'s `packages` array, updates +`~/.pi/agent/npm/package.json`/`package-lock.json`, and deletes +`~/.pi/agent/npm/node_modules//` along with any transitive +dependency no longer needed by another installed package. Nothing was left +behind or errored. + +**Removal by hand-editing `settings.json` (the path `llm-sync` actually +takes) does *not* prune anything.** Given the artifact installed above, then +directly rewriting `settings.json`'s `packages` array to `[]` (simulating +`PiSettingsTarget.save()`) and re-running Pi — tried as `pi --version`, +`pi list --no-approve` (which reports "No packages installed" while the +files are still on disk), and a full non-interactive startup attempt +(`pi -p "hi" --offline --no-approve`, which fails only on missing API +credentials) — `~/.pi/agent/npm/node_modules/@gotgenes/pi-permission-system/` +remained on disk in every case. Pi does not garbage-collect installed +packages based on `settings.json` content alone; pruning only happens +through the explicit `pi remove ` (or `pi uninstall `) CLI +path. This confirms the risk this plan's Rabbit Holes/Unresolved Questions +sections flagged as unverified: **Epic 2.2's `PiPackageLedger` is required** +for stale-artifact cleanup — Pi will not do it on its own from a rendered +`settings.json`. + +**Caution for Epic 2.2 tooling**: bare `pi update` (no source argument) +self-updates the `pi` binary itself via `npm --prefix ~/.local`, outside any +temp `HOME` sandboxing (it updated the real system-wide install during this +spike, from the repo-pinned 0.84.4 to 0.86.1; reverted with `npm install +--global --prefix ~/.local --no-audit --no-fund +"@earendil-works/pi-coding-agent@0.84.4"`). Any future reconciliation +tooling must never invoke bare `pi update` — use `pi update --extensions` or +target specific sources. + +## Package ownership ledger + +`llm-sync` tracks which Pi package artifacts it installed in a ledger at +`~/.config/llm-sync/pi-package-state.json` (override with +`--pi-package-ledger-state-file`), since hand-editing `settings.json` alone +doesn't prune anything (see Package lifecycle above). + +- `--prune-stale-pi-packages` — a package the ledger owns but that's no + longer enabled in the rendered config is always reported as stale; this + flag actually deletes its on-disk artifact. +- `--reconcile-pi-package-ledger` — rebuilds ledger entries missing from a + prior sync that was interrupted between writing `settings.json` and + writing the ledger, from the current config's enabled packages. diff --git a/.config/pi/config.d/40-claude-compat.json b/.config/pi/config.d/40-claude-compat.json new file mode 100644 index 00000000..d046408b --- /dev/null +++ b/.config/pi/config.d/40-claude-compat.json @@ -0,0 +1,7 @@ +{ + "extensions": { + "claude-compat": { + "path": "~/dotfiles/plugins/pi-claude-compat/pi/index.ts" + } + } +} diff --git a/.config/pi/extensions-manifest.json b/.config/pi/extensions-manifest.json new file mode 100644 index 00000000..7a29348c --- /dev/null +++ b/.config/pi/extensions-manifest.json @@ -0,0 +1 @@ +{"extensions": {}} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c5786ad..e3474a54 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,8 @@ on: - 'bootstrap/**' - 'install.sh' - '.github/workflows/ci.yml' + - 'stapler-scripts/llm-sync/**' + - '.config/pi/**' pull_request: paths: - '**/*.py' @@ -19,6 +21,8 @@ on: - 'bootstrap/**' - 'install.sh' - '.github/workflows/ci.yml' + - 'stapler-scripts/llm-sync/**' + - '.config/pi/**' jobs: ansible-lint: @@ -54,6 +58,8 @@ jobs: run: | cd stapler-scripts/slack-emoji-export uv run pytest test_extract_slack_emoji.py + - name: Run llm-sync tests + run: make llm-sync-test - name: Run ruff run: | cd stapler-scripts/slack-emoji-export diff --git a/bootstrap-pyinfra/deploys/llm_sync.py b/bootstrap-pyinfra/deploys/llm_sync.py index 3f5eda72..8936e2d6 100644 --- a/bootstrap-pyinfra/deploys/llm_sync.py +++ b/bootstrap-pyinfra/deploys/llm_sync.py @@ -34,4 +34,8 @@ def llm_sync() -> None: if code != 0: raise DeployError(f"llm-sync failed: {output}") + # shell_capture returns main.py's combined stdout+stderr verbatim (see + # its docstring), so this already reproduces PiPackageLedger's + # `stale Pi package: ...` report lines unmodified -- no pyinfra-side + # change needed to surface them (Epic 2.2, Task 2.2.2a). print(output) diff --git a/bootstrap-pyinfra/test_llm_sync_deploy.py b/bootstrap-pyinfra/test_llm_sync_deploy.py new file mode 100644 index 00000000..38642191 --- /dev/null +++ b/bootstrap-pyinfra/test_llm_sync_deploy.py @@ -0,0 +1,149 @@ +"""Verifies the pass-through claim in deploys/llm_sync.py's docstring: that +main.py's `stale Pi package: ...` report line survives unmodified through +`common.shell_capture`, the mechanism `llm_sync()` uses to invoke main.py. + +`llm_sync()` itself can't be called directly here: it's a pyinfra +`@deploy`-decorated function that calls `shell_capture`, which calls +`host.get_fact()`, and that only resolves inside a connected pyinfra run -- +not a plain `uv run` test process (the same reason test_pi_install.py only +tests the pure functions in deploys/pi.py, never the `@deploy("Pi")`-decorated +`pi()` itself). So these tests reproduce llm_sync()'s actual invocation shape +via subprocess instead: driving main.py's real CLI entrypoint through `uv +run` (the same tool llm_sync()'s shell command uses), against a fixture +matching stapler-scripts/llm-sync/test_pi_package_ledger.py's +`_seed_environment`/`_base_args` pattern for a single stale ledger entry. + +Run directly: uv run test_llm_sync_deploy.py +""" + +import json +import subprocess +import tempfile +from pathlib import Path + +LLM_SYNC_DIR = Path(__file__).parent.parent / "stapler-scripts" / "llm-sync" + +_STALE_LINE = ( + "stale Pi package: old-extension (not pruned; run with --prune-stale-pi-packages)" +) + +# Marker string common.shell_capture uses to split captured output from the +# trailing exit code it appends via `printf "%s{marker}%d" "$OUT" "$CODE"`. +_SHELL_CAPTURE_MARKER = "__PYINFRA_EXIT__" + + +def _write_json(path: Path, value: object) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value), encoding="utf-8") + + +def _seed_environment(root: Path) -> Path: + """Same fixture shape as test_pi_package_ledger.py's `_seed_environment`: + no packages enabled in config, plus a pre-existing stale ledger entry + named `old-extension` whose recorded artifact path is a real directory. + """ + _write_json(root / "config.d" / "10-empty.json", {"packages": {}}) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + + artifact_dir = root / "agent" / "npm" / "node_modules" / "old-extension" + artifact_dir.mkdir(parents=True) + (artifact_dir / "index.js").write_text("// stale", encoding="utf-8") + + _write_json(root / "package-state.json", {"old-extension": str(artifact_dir)}) + return artifact_dir + + +def _main_py_args(root: Path) -> list[str]: + """The subset of main.py's fixture-pointing flags needed to reach the + stale-report code path without touching this machine's real ~/.claude, + ~/.pi, or ~/.config/pi state.""" + return [ + "main.py", + "--target", + "pi", + "--source-dir", + str(root / "claude_src"), # empty/nonexistent: no real Claude assets synced + "--state-file", + str(root / "state.json"), + "--pi-dir", + str(root / "agent"), + "--pi-config-file", + str(root / "config.json"), + "--pi-config-dir", + str(root / "config.d"), + "--pi-local-config", + str(root / "config.local.json"), + "--pi-local-config-dir", + str(root / "config.local.d"), + "--pi-extensions-manifest", + str(root / "extensions-manifest.json"), + "--pi-settings-file", + str(root / "agent" / "settings.json"), + "--pi-settings-state-file", + str(root / "settings-state.json"), + "--pi-package-ledger-state-file", + str(root / "package-state.json"), + ] + + +def test_llm_sync_deploy_prints_stale_pi_package_report_line() -> None: + """main.py, invoked through its real CLI entrypoint via `uv run` (the + same tool llm_sync()'s shell command uses) rather than calling + sync_pi_settings() in-process, reaches the stale-report code path and + prints the byte-exact line.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _seed_environment(root) + + result = subprocess.run( + ["uv", "run", *_main_py_args(root)], + cwd=LLM_SYNC_DIR, + capture_output=True, + text=True, + timeout=60, + ) + + assert result.returncode == 0, result.stdout + result.stderr + assert _STALE_LINE in result.stdout.splitlines() + + +def test_bootstrap_llm_sync_reproduces_exact_stale_report_line() -> None: + """Reproduces `common.shell_capture`'s exact command shape -- + `OUT=$(command 2>&1); CODE=$?; printf "%s{marker}%d" "$OUT" "$CODE"`, + then splitting on the marker the same way shell_capture does -- to prove + the specific pass-through mechanism llm_sync() relies on (combining + stdout+stderr through command substitution) doesn't truncate, reorder, + or otherwise mangle the stale-report line.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _seed_environment(root) + + inner_command = "uv run " + " ".join(_main_py_args(root)) + wrapped = ( + f'OUT=$({inner_command} 2>&1); CODE=$?; ' + f'printf "%s{_SHELL_CAPTURE_MARKER}%d" "$OUT" "$CODE"' + ) + + result = subprocess.run( + ["bash", "-c", wrapped], + cwd=LLM_SYNC_DIR, + capture_output=True, + text=True, + timeout=60, + ) + + assert result.returncode == 0, result.stdout + result.stderr + captured_output, _, exit_code = result.stdout.rpartition( + _SHELL_CAPTURE_MARKER + ) + + assert exit_code == "0", result.stdout + assert _STALE_LINE in captured_output.splitlines() + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed") diff --git a/bootstrap-pyinfra/test_pi_install.py b/bootstrap-pyinfra/test_pi_install.py index 811e1e2d..f5cbb4f1 100644 --- a/bootstrap-pyinfra/test_pi_install.py +++ b/bootstrap-pyinfra/test_pi_install.py @@ -55,6 +55,19 @@ def test_pi_hook_install_uses_the_supported_stapler_squad_installer() -> None: assert "ssq-hooks install pi" in command +def test_external_install_mode_dry_run_states_skip_reason_explicitly() -> None: + """`pi()` prints `f"Pi installation: {plan.reason}"` unconditionally -- + that line runs at deploy-definition time, not as a queued operation, so + it appears under `--dry` too. Assert the plan this print consumes + actually states why nothing will be installed, not just that it won't be. + """ + plan = plan_pi_install( + "external", pi_present=False, installed_version=None, target_version="0.84.4" + ) + assert plan.action == "external" + assert "externally managed" in plan.reason + + def test_rejects_invalid_mode_and_unsafe_version() -> None: invalid = ( ("surprise", "0.84.4"), diff --git a/plugins/pi-claude-compat/pi/index.ts b/plugins/pi-claude-compat/pi/index.ts new file mode 100644 index 00000000..44d24002 --- /dev/null +++ b/plugins/pi-claude-compat/pi/index.ts @@ -0,0 +1,108 @@ +import { Type, type Static } from "typebox"; +import type { ExtensionAPI, ExtensionContext, ToolDefinition } from "@earendil-works/pi-coding-agent"; +import { + createFindToolDefinition, + createGrepToolDefinition, + createLsToolDefinition, +} from "@earendil-works/pi-coding-agent"; + +type AnyToolDefinition = ToolDefinition; + +/** + * Registers `name` as a full alias of a Pi-native tool. + * + * Pi's extension API has no "forward this tool call to another registered + * tool" primitive (tool_call handlers can only mutate input or block), so + * this re-derives the native tool definition from its own factory — using + * the live `ctx.cwd` at call time rather than a cwd frozen at extension + * load — and delegates straight to its `execute`. Everything else (schema, + * description, rendering) comes from the native definition unchanged. + */ +function aliasNativeTool( + name: string, + createDefinition: (cwd: string) => AnyToolDefinition, +): AnyToolDefinition { + const template = createDefinition(process.cwd()); + return { + ...template, + name, + execute: (toolCallId, params, signal, onUpdate, ctx) => + createDefinition(ctx.cwd).execute(toolCallId, params, signal, onUpdate, ctx), + }; +} + +const askUserQuestionOptionSchema = Type.Object({ + label: Type.String(), + description: Type.Optional(Type.String()), +}); + +const askUserQuestionQuestionSchema = Type.Object({ + header: Type.Optional(Type.String()), + question: Type.String(), + multiSelect: Type.Optional(Type.Boolean()), + options: Type.Array(askUserQuestionOptionSchema), +}); + +const askUserQuestionSchema = Type.Object({ + questions: Type.Array(askUserQuestionQuestionSchema), +}); + +type AskUserQuestionInput = Static; + +interface AskUserQuestionAnswer { + header?: string; + question: string; + answer: string; +} + +// Pi's native ask-user primitive is ctx.ui.select() — a single-choice +// dialog. There is no native multi-select prompt, so a `multiSelect: true` +// question still only returns one answer here; that is a known gap, not a +// silent no-op (see project_plans/pi-dotfiles/implementation/plan.md's Task +// 3.4.1a). +async function executeAskUserQuestion( + _toolCallId: string, + params: AskUserQuestionInput, + signal: AbortSignal | undefined, + _onUpdate: unknown, + ctx: ExtensionContext, +) { + if (!ctx.hasUI) { + throw new Error("AskUserQuestion: no interactive UI available in this Pi run mode."); + } + + const answers: AskUserQuestionAnswer[] = []; + for (const q of params.questions) { + const title = q.header ? `${q.header}: ${q.question}` : q.question; + const labels = q.options.map((option) => + option.description ? `${option.label} — ${option.description}` : option.label, + ); + const choice = await ctx.ui.select(title, labels, { signal }); + answers.push({ header: q.header, question: q.question, answer: choice ?? "(no answer)" }); + } + + const text = answers + .map((a) => `${a.header ? `[${a.header}] ` : ""}${a.question} -> ${a.answer}`) + .join("\n"); + + return { content: [{ type: "text" as const, text }], details: answers }; +} + +const askUserQuestionTool: ToolDefinition = { + name: "AskUserQuestion", + label: "Ask User Question", + description: + "Ask the user one or more multiple-choice questions and wait for their answers. " + + "Compat shim for Claude Code's AskUserQuestion tool, backed by Pi's native " + + "single-choice ui.select() prompt.", + promptSnippet: "Ask the user a multiple-choice question", + parameters: askUserQuestionSchema, + execute: executeAskUserQuestion, +}; + +export default function piClaudeCompat(pi: ExtensionAPI): void { + pi.registerTool(aliasNativeTool("Grep", (cwd) => createGrepToolDefinition(cwd))); + pi.registerTool(aliasNativeTool("Glob", (cwd) => createFindToolDefinition(cwd))); + pi.registerTool(aliasNativeTool("LS", (cwd) => createLsToolDefinition(cwd))); + pi.registerTool(askUserQuestionTool); +} diff --git a/project_plans/pi-dotfiles/decisions/ADR-002-credential-provider-approach.md b/project_plans/pi-dotfiles/decisions/ADR-002-credential-provider-approach.md index f45f2225..f315542d 100644 --- a/project_plans/pi-dotfiles/decisions/ADR-002-credential-provider-approach.md +++ b/project_plans/pi-dotfiles/decisions/ADR-002-credential-provider-approach.md @@ -33,12 +33,15 @@ wired in. `.config/pi/config.d/90-credential-1password.json` with `enabled: false` — activation is a machine-local override the user makes deliberately, never a tracked default. -3. Treat the extension as an **Adapter** (GoF) around Pi's existing - `packages`/`extensions` registry mechanism, not as a bespoke Tyler-owned - credential system. Dotfiles never generate, copy, or touch - `~/.pi/agent/auth.json`; setup of the actual `!op read ...` reference - remains an explicit local action the user performs outside any tracked - config. +3. Treat the extension as a **Registry entry, disabled by default** — the same + mechanism as every other opt-in package in the Pattern Decisions table + (`packages`/`extensions` registry), not as a bespoke Tyler-owned credential + system. No adapter class is built; there is no interface translation for a + future reader to find, just a disabled-by-default registration like the + permission system, plan mode, and the compat shim. Dotfiles never generate, + copy, or touch `~/.pi/agent/auth.json`; setup of the actual `!op read ...` + reference remains an explicit local action the user performs outside any + tracked config. 4. Require the manifest entry's review `notes` to record, specifically: vault item least-privilege scoping, per-tool/per-project scoping, output redaction, and confirmation that resolved values are not inherited into diff --git a/project_plans/pi-dotfiles/design/ux.md b/project_plans/pi-dotfiles/design/ux.md index 09a84c10..3e08ba07 100644 --- a/project_plans/pi-dotfiles/design/ux.md +++ b/project_plans/pi-dotfiles/design/ux.md @@ -26,12 +26,32 @@ Would write to: .config/pi/extensions-manifest.json No changes made (dry run). Re-run without --dry-run to fork and write the manifest entry. ``` + +**Representative output/sample (real run, no `--dry-run`):** +``` +$ uv run fork_pin_extension.py fork gotgenes/pi-packages \ + --id gotgenes-pi-packages --capability "permission-system,subagents" + +Forking: github.com/gotgenes/pi-packages -> github.com/tstapler/pi-packages +Forked. fork_commit = e64946b5ce96ca004b753d98932c8b13106dd132 +Wrote: .config/pi/extensions-manifest.json + + extensions.gotgenes-pi-packages: + disposition: "candidate" + capability: "permission-system,subagents" + upstream_repo: "github.com/gotgenes/pi-packages" + fork_repo: "github.com/tstapler/pi-packages" + approved_by: null + approved_date: null + +Manifest entry written. Next: complete the review checklist at +.claude/skills/pi-extension-review/SKILL.md, then hand-edit disposition to "approved". +``` **Acceptance criteria:** - Running `fork_pin_extension.py fork --dry-run` shows the planned fork target and the exact manifest diff without calling `gh repo fork` or writing `.config/pi/extensions-manifest.json` — matching Task 1.3.1d. - The dry-run and real-run output both show `disposition: "candidate"` and `approved_by`/`approved_date` as `null` — never any other disposition value, since no flag on this command can write `"approved"` (Task 1.3.1c's static-inspection test enforces this at the code level; the CLI output must not contradict it by implying an approval flag exists). - On a real run, the printed `fork_commit` matches the SHA written to the manifest entry — the terminal output and the file are never allowed to diverge (read the mutation back, don't just claim it). - Re-running `fork` for an `--id` that already has a manifest entry fails with a message naming the existing entry id and its current `disposition`, not a generic "already exists" — so Tyler knows whether it's safe to re-scaffold (e.g. blocked on an existing `approved` entry) without opening the JSON file. -- No dead end: the dry-run's closing line states the next concrete action ("Re-run without --dry-run..."). +- No dead end: the dry-run's closing line states the next concrete action ("Re-run without --dry-run..."), and the real run's closing line states the next concrete action in the fork -> review -> approve chain ("Manifest entry written. Next: complete the review checklist..., then hand-edit disposition to \"approved\"") — so the two samples together read as one continuous flow, not two disconnected commands. --- @@ -47,12 +67,26 @@ Next step: run the pi-extension-review checklist (.claude/skills/pi-extension-re then hand-edit .config/pi/extensions-manifest.json to set disposition: "approved". Sync aborted; no changes were written to settings.json. ``` + +**Representative output/sample (entry exists but is `hold`/`rejected`):** +``` +$ uv run --directory stapler-scripts/llm-sync main.py --target pi + +PiConfigError: unreviewed fork source in rendered config: + git:github.com/tstapler/pi-permission-system@abc1234 +existing entry has disposition: rejected — re-approval requires a fresh review, +not silently flipping the field. +Next step: run the pi-extension-review checklist (.claude/skills/pi-extension-review/SKILL.md), +then hand-edit .config/pi/extensions-manifest.json to set disposition: "approved". +Sync aborted; no changes were written to settings.json. +``` **Acceptance criteria:** - A blocked sync's error message names the exact unreviewed source string (e.g. `git:github.com/tstapler/pi-permission-system@abc1234`), not a generic "validation failed" — matching Story 1.2.1's three GWT cases (missing entry, stale/mismatched-commit approval). - The stale-approval case (approved at a different commit than the one now configured) produces a message that names both the configured commit and the approved commit, so Tyler can tell at a glance whether the fragment or the manifest is out of date. - The message states explicitly that no file was written (`settings.json` unchanged) — a partial or ambiguous write state is never implied. - Work-owned and `@tstapler`-scoped sources exempted via `trustedPackageScopes` never appear in this error path, even with an empty manifest — confirming Story 1.2.2's exemption holds in the actual CLI output, not just in unit tests. - No dead end: the message's last line names the concrete next action — run the review skill, then hand-edit `disposition` to `"approved"` — exactly as Task 1.4.1a's skill documents. +- The "no entry at all" case and the "entry exists but is `hold`/`rejected`" case are distinguishable in the message text, not collapsed into the same generic wording — the no-entry case never claims a disposition, and the hold/rejected case names the entry's actual current disposition verbatim (e.g. `existing entry has disposition: rejected`) plus the explicit warning that re-approval requires a fresh review rather than flipping the field. This matters because someone re-approving a previously-rejected extension under time pressure, without noticing it was already rejected, is a real risk this doc and pre-mortem.md's Failure #1 both flag. --- @@ -92,12 +126,32 @@ $ uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry --> Loaded 1 host [Pi] Would skip install (pi_install_mode=external; deferring to externally managed Pi) ``` + +**Representative output/sample (piped to a non-TTY, e.g. `| tee sync.log` or CI):** +``` +$ uv run --directory stapler-scripts/llm-sync main.py --target pi --dry-run --pi-dir /tmp/pi-staging | tee sync.log + +Syncing pi_config -> pi... +Detected 3/12 modified items. +Would write settings.json keys: packages.gotgenes-pi-permission-system, extensions.claude-compat +Would delete legacy agent old-agent.md +No changes made to /tmp/pi-staging (dry run). +``` + +**Representative output/sample (happy path — fully approved, clean sync, nothing stale, nothing to prune):** +``` +$ uv run --directory stapler-scripts/llm-sync main.py --target pi + +Synced pi_config -> pi. 0 changes. All sources reviewed. +``` **Acceptance criteria:** - Every dry-run invocation names the destination it would have written to (`/tmp/pi-staging`, or the real `~/.pi/agent` path when not overridden), so Tyler can visually confirm a staging run never touched the real machine — per requirements' "Dry-run output must show intended changes without exposing credentials or secret values." - Dry-run output enumerates each changed key/resource individually (e.g. `packages.gotgenes-pi-permission-system`), not just a count — matching the observability requirement that bootstrap output identify "the effective configuration layers, extension/package actions, and whether each item was installed, updated, disabled, skipped, or already converged." - No credential-shaped value ever appears in dry-run output — this is testable by grepping the printed text for common secret-key patterns (`apikey`, `token`, `auth`) after a run against a fixture containing one; `PiConfigSource._reject_credential_material` should have already errored before any dry-run print occurs. - `pi_install_mode=external` dry-run output states explicitly that install was skipped and why (deferring to externally managed Pi) — never a silent no-op with no explanation, since a work machine must never look like a failed run when it correctly did nothing. - The closing line of every dry-run always states plainly that no changes were made and to what path — no dead end, since the human's next action (re-run without `--dry-run`/`--dry` once satisfied) is either implied by convention already established elsewhere in this doc or stated directly. +- When stdout is not a TTY (piped to a log file, CI, `grep`), Rich markup (`[yellow]...[/yellow]`-style color codes) must not appear in the output — the underlying text/line format is byte-for-byte identical to the TTY case either way, per Surface 3's greppable-line requirement; only the color wrapping is conditional on TTY detection. +- A fully-approved, clean sync (every source reviewed, nothing stale, nothing to prune) prints a single-line success summary naming the target and resource, the change count, and that all sources are reviewed (e.g. `Synced pi_config -> pi. 0 changes. All sources reviewed.`) — so a clean run has a recognizable "nothing to do, and nothing to worry about" signal distinct from a run that happened to make zero changes because it was blocked. --- @@ -136,8 +190,23 @@ $ uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry + "approved_date": "2026-09-14" + "disposition": "approved" ``` + +**Representative error sample (notes-evidence check fails — fewer than 5 real file paths cited):** +``` +$ uv run --directory stapler-scripts/llm-sync main.py --target pi + +ManifestError: entry "gotgenes-pi-packages" has disposition: "approved" but its +notes cite only 2 of the required 5 distinct file paths verified in the fork +tree at fork_commit e64946b5ce96ca004b753d98932c8b13106dd132. +Missing evidence for gate sub-area(s): filesystem, secrets, telemetry. +This does not judge whether the review was competent — it only checks that +notes name real files an evidence-based reviewer would have looked at. +Fix: re-open .config/pi/extensions-manifest.json and add cited paths (one per +missing sub-area) that exist at that commit, per the pi-extension-review checklist. +``` **Acceptance criteria:** - `ExtensionManifestSource.load()` rejects an `approved` entry missing `approved_by`/`approved_date` with a `ManifestError` naming the entry id and the missing field(s) — so a partially-completed hand-edit is caught immediately on the next sync, not silently accepted (Story 1.1.2). +- When the mechanical notes-evidence check fails (fewer than 5 distinct real file paths verified against the fork tree at `fork_commit`, per Tasks 1.1.2c/1.1.2d), the raised `ManifestError` names the entry id and lists which gate sub-area(s) (network/subprocess/filesystem/secrets/telemetry) lack cited evidence — not a generic "notes insufficient" — and states plainly that this is a mechanical floor-check, not a judgment on review quality, matching the same honest framing as Story 4.1.1's AC. - The `notes` field, once approved, records specific review findings (fail-closed parser paths, deny/ask defaults, no-secret-inheritance, worktree cleanup, or per-extension equivalents like scoping/redaction/no-inheritance for the credential provider) — a generic string like `"reviewed, looks fine"` is a process failure this doc flags but cannot mechanically block; the `pi-extension-review` skill (Task 1.4.1a) is the documented control, not code, since content quality of prose is not machine-checkable the way `disposition` state is. - No tool, script, or automation in this codebase (`fork_pin_extension.py` included) writes the literal string `"approved"` to any `disposition` field — verified structurally by Task 1.3.1c's inspection test, and this workflow's diff is the only sanctioned path to that value. - The diff is a normal, reviewable git change — `git diff .config/pi/extensions-manifest.json` shows exactly the fields above changed, nothing else in the file touched, so the approval is auditable in `git log`/`git blame` the same way any other code change is. @@ -147,5 +216,5 @@ $ uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry ## Summary -- **Surfaces designed**: 5 (`fork_pin_extension.py fork` + `--dry-run`; `verify_pinned_sources_reviewed()` blocked-sync error; `--prune-stale-pi-packages` + stale-package report; bootstrap/pyinfra dry-run output; manual manifest-approval hand-edit workflow). -- **UX acceptance criteria written**: 25 (5 per surface). +- **Surfaces designed**: 5 (`fork_pin_extension.py fork` + `--dry-run` + real-run; `verify_pinned_sources_reviewed()` blocked-sync error, covering both no-entry and hold/rejected dispositions; `--prune-stale-pi-packages` + stale-package report; bootstrap/pyinfra dry-run output, including non-TTY and happy-path samples; manual manifest-approval hand-edit workflow, including the notes-evidence-failure error). +- **UX acceptance criteria written**: 29 (5, 6, 5, 7, 6 across the five surfaces respectively). diff --git a/project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md b/project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md new file mode 100644 index 00000000..e5df3d84 --- /dev/null +++ b/project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md @@ -0,0 +1,94 @@ +# Claude Tool Compatibility Matrix (Pi) + +Maps every Claude-specific tool name referenced by this repo's synced +`.claude/skills/**/SKILL.md` corpus to its status under the Pi coding agent +(https://pi.dev). Built for Epic 5.3 (`project_plans/pi-dotfiles/implementation/plan.md`, +Stories 5.3.1-5.3.2) so a future coding agent syncing a new skill to Pi +doesn't assume parity that doesn't exist. Corresponds to validation rows +`SM-7`/`SM-7 (error)` (`project_plans/pi-dotfiles/implementation/validation.md`). + +**Method**: `find .claude/skills -iname SKILL.md -print0 | xargs -0 grep -lE '\b\b'`, +run 2026-09-21 against 398 `SKILL.md` files. Counts below are the number of +`SKILL.md` files containing at least one occurrence of the exact word, not +total occurrences. + +**Status values**: +- `native` — Pi has an equivalent built in; no extension needed. +- `shimmed via pi-claude-compat` — a first-party Tyler-owned extension aliases + the Claude name onto a native or extension-provided capability. **Not yet + built** (Epic 3.4, `plugins/pi-claude-compat/pi/index.ts` does not exist on + disk as of this writing) — noted per-row as "planned, not yet shimmed." +- `extension-provided` — requires a third-party Pi extension; noted per-row + whether that extension is forked/approved in this plan's scope or merely a + named research candidate. +- `unsupported` — no native equivalent and no extension planned in this + plan's scope. +- `unknown — needs verification` — could not be confirmed from this repo's + own verified research or from Pi's docs; not guessed. + +## Basis for Pi's native tool set + +This repo has not independently re-verified Pi's full native tool list +against `pi.dev` docs during this task (no web access was used here beyond +what earlier phases already verified). The claims below are grounded in two +already-VERIFIED sources already in this repo: + +- `project_plans/pi-dotfiles/full-featured-profile-research.md` (lines 18-44): + "Pi already has core read/write/edit/bash and file discovery tools, + instructions and skills, prompt templates, model/provider switching, + session persistence, branching, compaction, themes, keybindings, package + filtering, and an extension API," and separately: "The existing skills + materially use Claude tool names, especially `AskUserQuestion`, `WebSearch`, + `WebFetch`, task tools, and `Agent`. We therefore need either a selective + compatibility extension or a deliberate source translation layer." +- `project_plans/pi-dotfiles/requirements.md:114`: "Pi deliberately omits + native MCP, subagents, permission popups, plan mode, and background bash; + parity may require evaluating or building extensions rather than copying + Claude configuration." + +Where a row's status isn't directly supported by one of those two sources or +by an in-scope plan.md epic, it is marked `unknown — needs verification` +rather than inferred. + +## Matrix + +| Tool name | Found in corpus | Pi status | Basis | +|---|---|---|---| +| `AskUserQuestion` | 41 files | `shimmed via pi-claude-compat` | Shipped (Epic 3.4, commit `14a4adc`). `plugins/pi-claude-compat/pi/index.ts` registers a custom `AskUserQuestion` tool backed by Pi's native single-choice `ctx.ui.select()` UI primitive, wired into Pi via `.config/pi/config.d/40-claude-compat.json`'s `extensions.claude-compat` entry. Known gap: Pi's native UI is single-choice only, so a `multiSelect: true` question still returns just one answer — documented in-code at `plugins/pi-claude-compat/pi/index.ts:58-61`. | +| `Grep` | 64 files | `shimmed via pi-claude-compat` | Shipped (Epic 3.4, commit `14a4adc`). `plugins/pi-claude-compat/pi/index.ts` re-derives Pi's native `createGrepToolDefinition` under the `Grep` name via its `aliasNativeTool` helper, wired into Pi via `.config/pi/config.d/40-claude-compat.json`. | +| `Glob` | 54 files | `shimmed via pi-claude-compat` | Shipped (Epic 3.4, commit `14a4adc`). `plugins/pi-claude-compat/pi/index.ts` re-derives Pi's native `createFindToolDefinition` (Pi's find tool) under the `Glob` name via its `aliasNativeTool` helper, wired into Pi via `.config/pi/config.d/40-claude-compat.json`. | +| `LS` | 0 files (searched, none found in current corpus) | `shimmed via pi-claude-compat` | Shipped (Epic 3.4, commit `14a4adc`), despite no current `SKILL.md` using the exact word `LS`. `plugins/pi-claude-compat/pi/index.ts` re-derives Pi's native `createLsToolDefinition` under the `LS` name via its `aliasNativeTool` helper, wired into Pi via `.config/pi/config.d/40-claude-compat.json`. | +| `Agent` | 105 files | `extension-provided` — not yet forked/approved, and its `pi-claude-compat` facade is also not yet built | requirements.md:114: Pi "deliberately omits native ... subagents." `gotgenes/pi-subagents` (Tier 1, `full-featured-profile-research.md:56-90`) is the candidate, tracked in Epic 3.2 with disposition `candidate` (`extension-audit.md:17`) — not yet forked/approved per Task 3.2.1's precondition. research.md:139 additionally plans `Agent` "as a compatibility facade over the selected subagent package" via `pi-claude-compat` (Epic 3.4) — also not yet built. Two dependent gaps, neither closed today. | +| `TaskCreate` | 3 files | `unsupported` — no extension scheduled in this plan's scope | research.md:165-177 (Tier 3) names `juicesharp/rpiv-mono`'s `rpiv-todo` package as the closest candidate for Claude `TaskCreate`/`TaskUpdate` compatibility, but no epic in `plan.md` forks or schedules it — Tier 3 is out of scope except where Epic 3.4's compat shim explicitly covers it (it doesn't cover task tools; see plan.md:722's required-name list, which omits task-store aliasing from Epic 3.4's own scope at plan.md:537-554). | +| `WebSearch` | 14 files | `unsupported` — no extension scheduled in this plan's scope | research.md:199-207 (Tier 3) names `nicobailon/pi-web-access` as the non-browser search/content-extraction candidate, but no epic in this plan forks or approves it. Not covered by Epic 3.4's compat shim (scoped to `AskUserQuestion`/`Grep`/`Glob`/`LS`/`Agent` only, plan.md:537-554). | +| `WebFetch` | 30 files | `unsupported` — no extension scheduled in this plan's scope | Same basis as `WebSearch` — `nicobailon/pi-web-access` is the named Tier 3 candidate; not forked/scheduled in this plan. | +| `Read` | 166 files | `native` | research.md:20-21: Pi already has "core read/write/edit/bash" tools. Exact tool-name casing on Pi's side is not verified here, but the capability gap list (research.md:24-38) does not list read/write/edit/bash as a gap, so no compat shim is planned or needed. | +| `Write` | 173 files | `native` | Same basis as `Read`. | +| `Edit` | 71 files | `native` | Same basis as `Read`. | +| `Bash` | 64 files | `native` (synchronous only) | research.md:20-21 confirms native bash. Caveat: requirements.md:114 states Pi "deliberately omits ... background bash" — background/interactive execution is a separate Tier 3 gap (research.md:226-241, `99percentpeople/pi-background-tasks` candidate), not forked or scheduled in this plan. Ordinary synchronous `Bash` calls are native; long-running/background bash is `unsupported` in this plan's scope. | +| `TodoWrite` | 7 files | `unknown — needs verification` | Distinct from `TaskCreate`/`TaskUpdate`. research.md's closest lead is the Tier 3 `rpiv-todo` package (line 171), evaluated there only in the context of `TaskCreate`/`TaskUpdate`, not `TodoWrite` specifically. No verified statement in this repo confirms or denies a native Pi equivalent for session-scoped todo tracking. Not guessed. | +| `MultiEdit` | 3 files | `native` (functional equivalent, not name-for-name) | Legacy Claude Code tool for the same multi-file text substitution Pi's native edit/bash tools already cover (research.md:20-21). Not called out as a gap in research.md's gap list (lines 24-38), so no dedicated shim is planned. | +| `SlashCommand` | 2 files | `unknown — needs verification` | Pi has "prompt templates" natively (research.md:21), but whether Pi exposes a tool-callable mechanism to invoke one programmatically from within another tool call (the `SlashCommand` tool's actual behavior in Claude Code) is not confirmed by any source in this repo. Not guessed. | + +## Tier 4 — explicitly deferred (session/workflow utilities) + +Per `plan.md`'s "Explicitly deferred" section (plan.md:27): Tier 4 +session/workflow-utility parity is deliberately deferred to a follow-up SDD +project, not delivered by this plan. These rows are recorded here so the gap +is tracked, not silently absent from the matrix. + +| Category | Pi status | Basis | +|---|---|---| +| Session search/handoff | `deferred` — follow-up candidate named, not evaluated or forked | `full-featured-profile-research.md:245-255` (Tier 4) names `thurstonsand/pi-sessions` (review anchor `8f2f3d444cc65255bfc61dbb6173e69c21eb5e2c`) as the candidate a follow-up project would evaluate. | +| Async compaction | `deferred` — follow-up candidate named, not evaluated or forked | `full-featured-profile-research.md:257-265` (Tier 4) names `almogdepaz/pi-async-compaction` (review anchor `5b9a70b678f23b7250f66b97789cb2777061cf3c`) as the candidate a follow-up project would evaluate. | + +## Workflow category: global/project instructions + +Not a tool name found by the grep — recorded separately per Story 5.3.2 +(plan.md:735-751), since "global and project instructions" parity is named +explicitly in requirements.md's In-Scope list and needed its own row rather +than being silently covered by the glossary alone. + +| Category | Pi status | Basis | +|---|---|---| +| Global/project instructions | **Project-level: `native`.** **Global-level: closed via symlink (this epic, Task 5.3.2a).** | Project-level: Pi natively loads project instructions by walking up from the current directory looking for `AGENTS.md` or `CLAUDE.md` (`AGENTS.override.md` takes precedence per-directory) — VERIFIED against `pi.dev/docs/latest/quickstart` and `github.com/earendil-works/pi/blob/main/AGENTS.md` (plan.md:21). This repo's project-level `CLAUDE.md` files (e.g. `/home/tstapler/CLAUDE.md`) already exist, so Pi already reads them with no dotfiles change. Global-level: `~/.claude/CLAUDE.md`'s equivalent is `~/.pi/agent/AGENTS.md`; `.cfgcaddy.yml` now has a `dest: .pi/agent/AGENTS.md` entry (Task 5.3.2a) mirroring the existing `.claude/CLAUDE.md` entry's `src`, giving `~/.pi/agent/AGENTS.md` the same tracked content Claude already reads globally. | diff --git a/project_plans/pi-dotfiles/implementation/plan.md b/project_plans/pi-dotfiles/implementation/plan.md index 34a041f9..3be22b47 100644 --- a/project_plans/pi-dotfiles/implementation/plan.md +++ b/project_plans/pi-dotfiles/implementation/plan.md @@ -18,9 +18,14 @@ Before scoping new work, this is what the codebase already does — verified by - **Skills/prompts sync**: `PiTarget` (`stapler-scripts/llm-sync/src/targets/pi.py`) — regression anchor per requirements, untouched by this plan. - **Tiered-config-pattern skill**: `.claude/skills/tiered-configuration/SKILL.md` already documents and enforces the config.d model (success metric 9 is already met). - **First-party extensions**: kibitzer, ponytail, dotfiles-hooks are already wired via `.config/pi/config.d/{10,20,30}-*.json`. +- **Project-level instructions (partial parity, zero new work)**: Pi natively loads project instructions by walking up from the current directory looking for `AGENTS.md` or `CLAUDE.md` (with `AGENTS.override.md` taking precedence per-directory) — VERIFIED against Pi's own docs (`pi.dev/docs/latest/quickstart`; `github.com/earendil-works/pi/blob/main/AGENTS.md`). Since this repo's project-level `CLAUDE.md` files (e.g. this repo's own `/home/tstapler/CLAUDE.md`) already exist, Pi already reads them with no dotfiles changes required. Global-level parity (`~/.claude/CLAUDE.md`'s equivalent, `~/.pi/agent/AGENTS.md`) is the genuine gap — scoped as new Story 5.3.2. What is genuinely missing, and what this plan builds: a **fork-pin-review manifest and enforcement gate** (no third-party extension may render without one), an **ownership ledger for on-disk package artifacts** (today's ledger only covers `settings.json` top-level keys), the **sequenced, human-gated fork-and-enable work** for the Tier 1+ candidates from the research docs, **credential-provider wiring**, and the **rollout/rollback runbook** the requirements' Risk Control section calls for. +### Explicitly deferred + +- **Tier 4 session/workflow-utility parity** (session search/handoff, async compaction) is deliberately deferred to a follow-up SDD project, not delivered by this plan. Phase 3 scopes itself to Tier 0-2 extensions only (Tier 3/MCP is partially covered by Epic 3.4's compat shim, which stays in scope). `full-featured-profile-research.md`'s Tier 4 section names the candidates a follow-up would evaluate: session search/handoff via `thurstonsand/pi-sessions` (review anchor `8f2f3d444cc65255bfc61dbb6173e69c21eb5e2c`) and async compaction via `almogdepaz/pi-async-compaction` (review anchor `5b9a70b678f23b7250f66b97789cb2777061cf3c`). This is a stated scope boundary, not a silently dropped requirement — matching this plan's existing practice of recording deferrals explicitly (see Unresolved Questions). + --- ## Step 0.5 — Creative pass: shape of the fork-review-pin pipeline @@ -53,7 +58,7 @@ What is genuinely missing, and what this plan builds: a **fork-pin-review manife | Disposition | A Manifest Entry's review status: `candidate`, `hold`, `rejected`, or `approved`. | | | Approval Gate | The rule that `disposition` can only become `approved` by a human hand-edit — no tool writes that value. | Enforced by Epic 1.3's helper never emitting it. | | Fork-and-Pin Helper | The scripted scaffolding tool (`fork_pin_extension.py`) that creates the GitHub fork and a `candidate` Manifest Entry, but cannot approve it. | | -| Review Gate Specification | The validation rule (`verify_pinned_sources_reviewed`) that blocks any rendered fork source lacking an `approved` Manifest Entry at the matching commit. | | +| Review Gate Specification | The validation rule (`verify_pinned_sources_reviewed`) that blocks any rendered fork source lacking an `approved` Manifest Entry at the matching commit. | Lives in `stapler-scripts/llm-sync/src/sources/review_gate.py`, not `extension_manifest.py` — it cross-references `PiConfigSource`'s rendered output and the manifest registry, so it's kept out of the manifest-schema-parsing module. | | Credential Provider | A Pi extension that resolves secrets at runtime from an OS/desktop credential store without the value passing through tracked config. | e.g. forked `pi-1password`. | | Credential Material | Any literal secret value (API key, token, password). | Already rejected by `PiConfigSource._reject_credential_material`. | | Package Ledger | New state file (`PiPackageLedger`) extending the ownership-ledger concept to installed package artifacts on disk, not just `settings.json` keys. | Phase 2. | @@ -74,8 +79,8 @@ What is genuinely missing, and what this plan builds: a **fork-pin-review manife | Fork/pin manifest storage | Registry — stable-ID keyed map, same shape as the existing `packages`/`extensions` registries | PoEAA (Registry) | One manifest file per extension under `project_plans/` | Splitting review state across many files defeats a single grep/validate point; matching the existing registry shape keeps the validator and mental model consistent with `PiConfigSource`. | | Review Gate Specification | Specification pattern — validate-before-render, extending `PiConfigSource`'s existing credential/pin checks | GoF/DDD Specification | Convention-only documentation (checklist as prose in `extension-audit.md` with no enforcement) | `PiConfigSource` already proves this codebase's answer to "must never happen" is executable validation, not policy text; a prose-only gate is exactly the silent-install failure mode requirements forbid. | | Config-fragment renderer / merge engine | Existing tiered override chain (`TieredJsonConfig`) — reused unchanged | Existing implementation | A second, extension-specific merge engine | The current engine already deep-merges objects, replaces arrays, and honors `null`-deletes, fully tested; a second engine would fork behavior for no functional gain. | -| Ownership/cleanup tracking for installed packages | External Ownership Ledger (Terraform-state-inspired snapshot), a sibling to the existing `managedKeys` ledger for a different resource class | PoEAA (Memento/Snapshot) | Assume Pi's own package manager garbage-collects unused installs | Unverified — Rabbit Holes explicitly flags this as a risk; treating it as self-cleaning without confirming (Epic 2.1's spike) risks the exact stale-entry problem requirements call out. | -| Credential-provider integration | Adapter — forked `pi-1password` wrapped behind the existing `packages` registry, disabled by default | GoF Adapter | Build a custom Tyler-owned credential extension from scratch | The research doc identifies `jmcombs/pi-1password` as the closest fit to the desired desktop-store model already; building new re-solves native-keyring/OAuth-adjacent risk the fork review already scopes. | +| Ownership/cleanup tracking for installed packages | External Ownership Ledger (Repository), a sibling to the existing `managedKeys` ledger for a different resource class | Repository — a small ownership-record store, same shape as `PiSettingsTarget`'s `managedKeys` mechanism | Assume Pi's own package manager garbage-collects unused installs | Unverified — Rabbit Holes explicitly flags this as a risk; treating it as self-cleaning without confirming (Epic 2.1's spike) risks the exact stale-entry problem requirements call out. | +| Credential-provider integration | Registry entry, disabled by default — same mechanism as the other opt-in packages in this table | Registry (matches this table's other opt-in entries) | Build a custom Tyler-owned credential extension from scratch | The research doc identifies `jmcombs/pi-1password` as the closest fit to the desired desktop-store model already; building new re-solves native-keyring/OAuth-adjacent risk the fork review already scopes. No adapter class is built — the extension registers exactly like every other opt-in `packages` entry, so "Adapter" mislabeled what's actually implemented (architecture-review.md Concern #5). | | Staged rollout / dry-run | Feature toggle via the existing `enabled` registry flag + tier-ordered promotion checklist | Existing implementation / Fowler feature toggle | A separate environment-variable rollout flag mechanism | A second, untyped flag would bypass the schema validation `PiConfigSource` already performs on the registry it would duplicate. | --- @@ -102,6 +107,7 @@ Not N/A: the current macOS machine's `~/.pi/agent/settings.json` already contain - **Logs**: `llm_sync()` (`bootstrap-pyinfra/deploys/llm_sync.py`) already prints `main.py`'s full stdout, which itself reports per-resource install/update/disable/skip decisions via `rich.Console`. Phase 1/2 additions (manifest rejections, stale-package reports) extend the same stream with entry-id-named messages, matching the existing `PiConfigError`/`PiSettingsTargetError` style of naming the offending key. - **Metrics**: none — this is local bootstrap automation, not an online service (per requirements.md Observability Requirements). - **Alerts**: none. Failures surface as a non-zero-exit `DeployError` that halts the pyinfra run, exactly as `deploys/pi.py` already does for install failures. +- **Pre-bump gate (required, not optional)**: any future `pi_install_version` change (`bootstrap-pyinfra/group_data/all.py`) must first pass a smoke test — start `pi` against a temp `HOME` with every currently-`approved` manifest entry's fork/commit enabled, assert each extension loads without error, and re-run Task 3.3.1c-2's plan-mode/permission-system composition integration test as a regression check (Story 5.1.1, Task 5.1.1c). A core-Pi version bump can silently break an unchanged, already-approved extension's runtime behavior (e.g. fail-soft extension loading masking a broken permission gate), which none of the install/update/skip logging above would surface. ## Risk Control @@ -134,8 +140,12 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership v Phase 5: Validation, Rollback, Documentation (runbook, backup/rollback, compat matrix, doc sync) + +Epic 5.2 (Story 5.2.2) ──(must complete first)──▶ Task 3.2.2b ``` +**Back-edge not drawn above**: Story 5.2.2 (Phase 5, classify the current machine's existing `settings.json` keys) must complete *before* Story 3.2.2's temp-dir validation/real-machine adoption step runs — see Story 3.2.2's and Task 3.2.2b's `Dependencies:` lines. The diagram shows Phase 3 feeding Phase 5 for simplicity; in practice this one Phase-5 story gates a Phase-3 real-machine step and must land first. + --- ## Phase 1: Fork-Pin-Review Governance @@ -178,17 +188,28 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - *Given* a `ManifestEntry` for id `narumiruna-pi-plan-mode` with `disposition: "approved"` and `approved_by: null`, *When* `ExtensionManifestSource.load()` parses it, *Then* it raises `ManifestError` stating approved entries require `approved_by` and `approved_date`. - A `candidate` entry with no `approved_by` loads successfully. - *Given* the same entry with `disposition: "candidate"` and `approved_by: null`, *When* loaded, *Then* it returns a `ManifestEntry` with `disposition == "candidate"`. +- An `approved` entry's `notes` must enumerate real evidence, not a rubber stamp — mechanical check only, not a quality judgment. + - *Given* a `ManifestEntry` with `disposition: "approved"`, *When* `ExtensionManifestSource.load()` parses it, *Then* it raises `ManifestError` unless `notes` enumerates at least 5 distinct file paths (one per gate-checklist sub-area: network, subprocess, filesystem, secrets, telemetry) that all exist in the fork tree at `fork_commit`, verified via `gh api repos/tstapler//git/trees/?recursive=1`. This raises the floor by catching empty or copy-pasted notes; it does not and cannot verify that the review itself was competent (see Story 4.1.1's AC for the same honest framing). **Files**: `stapler-scripts/llm-sync/src/sources/extension_manifest.py`, `stapler-scripts/llm-sync/test_extension_manifest.py` ##### Task 1.1.2a: Add the invariant check (~3 min) +- Enforce the "approved requires approved_by/approved_date" invariant in `ManifestEntry.__post_init__` (a frozen dataclass's `__post_init__` still runs on construction), not only in `ExtensionManifestSource.load()` — this makes the type itself reject an illegal state regardless of call site (e.g. `fork_pin_extension.py` or a test constructing a `ManifestEntry` directly, bypassing `.load()`), addressing architecture-review.md Concern #4. Keep the existing load()-time GWT tests as regression coverage of the same invariant, now enforced at the type level. - Files: `stapler-scripts/llm-sync/src/sources/extension_manifest.py` ##### Task 1.1.2b: Add the two GWT-derived tests (~4 min) - Files: `stapler-scripts/llm-sync/test_extension_manifest.py` +##### Task 1.1.2c: Add the mechanical notes-evidence check for `approved` entries (~5 min) +- On `disposition: "approved"`, parse `notes` for ≥5 distinct file paths and confirm each exists in `repos/tstapler//git/trees/?recursive=1` via `gh api`; raise `ManifestError` naming which sub-area(s) are missing evidence if fewer than 5 are found. Checks existence only — it cannot and does not judge whether the cited paths were meaningfully reviewed. +- Files: `stapler-scripts/llm-sync/src/sources/extension_manifest.py` + +##### Task 1.1.2d: Add the notes-evidence GWT tests (~4 min) +- Covers: an `approved` entry with <5 enumerated paths is rejected; an `approved` entry with 5 real paths (mocked `gh api` tree response) passes; a path not present in the tree response is rejected by name. +- Files: `stapler-scripts/llm-sync/test_extension_manifest.py` + ### Epic 1.2: Manifest Enforcement in llm-sync -**Goal**: Wire the manifest into the existing Pi settings sync so a pinned fork source can never render into `settings.json` without a matching `approved` Manifest Entry at the exact same commit. +**Goal**: Wire the manifest into the existing Pi settings sync so a pinned fork source can never render into `settings.json` without a matching `approved` Manifest Entry at the exact same commit. The cross-referencing gate itself lives in a new sibling module, `review_gate.py`, kept separate from `extension_manifest.py`'s narrower manifest-schema-parsing responsibility. #### Story 1.2.1: Sync refuses unreviewed fork sources **As** Tyler, **I want** `sync_pi_settings` to refuse to write settings when a config.d fork source has no approved manifest entry at that exact commit, **so that** no third-party extension activates without recorded human approval. @@ -208,19 +229,19 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - A duplicate manifest entry id is rejected at load time, before the gate ever runs. - *Given* `.config/pi/extensions-manifest.json` with two entries that resolve to the same id `gotgenes-pi-packages`, *When* `ExtensionManifestSource.load()` parses it, *Then* it raises `ManifestError` naming the duplicate id. -**Files**: `stapler-scripts/llm-sync/src/sources/extension_manifest.py`, `stapler-scripts/llm-sync/src/cli.py`, `stapler-scripts/llm-sync/test_extension_manifest.py`, `.github/workflows/ci.yml` +**Files**: `stapler-scripts/llm-sync/src/sources/review_gate.py`, `stapler-scripts/llm-sync/src/sources/extension_manifest.py`, `stapler-scripts/llm-sync/src/sources/pi_config.py`, `stapler-scripts/llm-sync/src/cli.py`, `stapler-scripts/llm-sync/test_review_gate.py`, `stapler-scripts/llm-sync/test_extension_manifest.py`, `.github/workflows/ci.yml` ##### Task 1.2.1a: Add `verify_pinned_sources_reviewed()` (~5 min) -- Scans rendered `packages`/`extensions` values for `github.com/tstapler/` fork sources and checks each against the manifest. -- Files: `stapler-scripts/llm-sync/src/sources/extension_manifest.py` +- Scans rendered `packages`/`extensions` values for `github.com/tstapler/` fork sources and checks each against the manifest. Lives in `review_gate.py`, which imports `ExtensionManifestSource`/`ManifestEntry` from `extension_manifest.py` and `LoadedPiConfig`/`PiConfigError` from `pi_config.py` — kept out of `extension_manifest.py` itself since this cross-references a sibling module's aggregate, not the manifest's own schema. +- Files: `stapler-scripts/llm-sync/src/sources/review_gate.py` ##### Task 1.2.1b: Call it from `sync_pi_settings()` (~4 min) -- After `PiConfigSource.load()`, before `PiSettingsTarget.save()`. +- After `PiConfigSource.load()`, before `PiSettingsTarget.save()`; imports `verify_pinned_sources_reviewed()` from `review_gate.py`. - Files: `stapler-scripts/llm-sync/src/cli.py` ##### Task 1.2.1c: Add the three baseline GWT tests (~5 min) - No-entry blocks; exact-match passes; commit-mismatch blocks. -- Files: `stapler-scripts/llm-sync/test_extension_manifest.py` +- Files: `stapler-scripts/llm-sync/test_review_gate.py` ##### Task 1.2.1d: Add `--pi-extensions-manifest` CLI override (~4 min) - Mirrors the existing `--pi-config-file`-style flags; default `.config/pi/extensions-manifest.json`. @@ -228,16 +249,21 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership ##### Task 1.2.1e: Add the hold/rejected-present and case-insensitive/exact-match commit comparison GWT tests (~5 min) - Covers: entry present with `disposition: "hold"` blocks; entry present with `disposition: "rejected"` blocks; uppercase manifest `fork_commit` vs. lowercase config pin passes; lowercase manifest `fork_commit` vs. uppercase config pin passes; a full 40-char manifest `fork_commit` vs. its own 7-char prefix as the config pin is correctly treated as a MISMATCH (proves the comparison is exact-after-case-fold, not prefix-tolerant). -- Files: `stapler-scripts/llm-sync/test_extension_manifest.py` +- Files: `stapler-scripts/llm-sync/test_review_gate.py` ##### Task 1.2.1f: Add the duplicate-manifest-id-at-load-time GWT test (~3 min) -- Extends `ExtensionManifestSource.load()`'s existing required-field validation (Task 1.1.1c) with a duplicate-id check; test lives alongside the other `verify_pinned_sources_reviewed()` coverage since it protects the same gate. +- Extends `ExtensionManifestSource.load()`'s existing required-field validation (Task 1.1.1c) with a duplicate-id check; this is a manifest-parsing concern (load-time schema validation), not part of the `review_gate.py` extraction, so it stays in `extension_manifest.py`/`test_extension_manifest.py` alongside Epic 1.1's other schema checks even though it protects the same overall gate. - Files: `stapler-scripts/llm-sync/src/sources/extension_manifest.py`, `stapler-scripts/llm-sync/test_extension_manifest.py` ##### Task 1.2.1g: Wire `make llm-sync-test` into the CI gate (~4 min) - Add a step running `make llm-sync-test` to `.github/workflows/ci.yml`'s `test` job (or a sibling job), and add `stapler-scripts/llm-sync/**` and `.config/pi/**` to the workflow's `paths` triggers, so a regression in `verify_pinned_sources_reviewed()` or the manifest loader fails CI instead of only a local run. `make llm-sync-test` (Makefile:40) already exists and runs every `test_*.py` under `stapler-scripts/llm-sync`; it is not currently invoked by any workflow (VERIFIED: no match for `llm-sync` in `.github/workflows/ci.yml` as of this plan). - Files: `.github/workflows/ci.yml` +##### Task 1.2.1h: Extract a shared `parse_fork_source()` used by both the config-source pin check and the review gate (~5 min) +- Addresses architecture-review.md Concern #3 (pinned-fork-source parsing duplicated between `PiConfigSource._is_allowed_package_source` and `verify_pinned_sources_reviewed()`, with no shared parser, risking silent drift between the two). Add `parse_fork_source(source: str) -> SourceRef(repo, commit)` and have both `_is_allowed_package_source` (in `pi_config.py`) and `verify_pinned_sources_reviewed()` (in `review_gate.py`) call it instead of each re-deriving repo/commit independently. +- Add a metamorphic test asserting both call sites agree on accept/reject and on the extracted repo/commit over the same fixture corpus, so the two can no longer silently diverge if the fork-URL shape is loosened in one and not the other (pre-mortem Failure #2). +- Files: `stapler-scripts/llm-sync/src/sources/pi_config.py`, `stapler-scripts/llm-sync/src/sources/review_gate.py`, `stapler-scripts/llm-sync/test_review_gate.py` + #### Story 1.2.2: Work/Tyler-owned exemptions carry through **As** Tyler, **I want** work-owned and Tyler-owned package sources exempted from the manifest gate, matching the existing `trustedPackageScopes`/`@tstapler` exemptions, **so that** the gate only ever blocks third-party forks, per requirements' explicit carve-out. @@ -247,13 +273,13 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - A `@tstapler`-scoped source is exempted the same way. - *Given* `npm:@tstapler/pi-claude-compat@1.0.0`, *When* checked, *Then* it is not required to appear in the manifest. -**Files**: `stapler-scripts/llm-sync/src/sources/extension_manifest.py`, `stapler-scripts/llm-sync/test_extension_manifest.py` +**Files**: `stapler-scripts/llm-sync/src/sources/review_gate.py`, `stapler-scripts/llm-sync/test_review_gate.py` ##### Task 1.2.2a: Restrict the scan to `github.com/tstapler/`-fork-shaped sources only (~3 min) -- Files: `stapler-scripts/llm-sync/src/sources/extension_manifest.py` +- Files: `stapler-scripts/llm-sync/src/sources/review_gate.py` ##### Task 1.2.2b: Add exemption tests (~4 min) -- Files: `stapler-scripts/llm-sync/test_extension_manifest.py` +- Files: `stapler-scripts/llm-sync/test_review_gate.py` ### Epic 1.3: Fork-and-Pin Helper Script **Goal**: Script the mechanical, safe parts of forking and pinning — never the approval itself. @@ -270,7 +296,7 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Files**: `stapler-scripts/llm-sync/scripts/fork_pin_extension.py`, `stapler-scripts/llm-sync/test_fork_pin_extension.py` ##### Task 1.3.1a: Write the `fork` subcommand (~5 min) -- uv inline-script, typer CLI per the `python-scripting` skill's conventions; calls `gh repo fork --org tstapler --default-branch-only` and captures the resulting commit via `gh api`. +- uv inline-script, typer CLI per the `python-scripting` skill's conventions; calls `gh repo fork --default-branch-only` (target namespace flag TBD — see Unresolved Questions: org-vs-personal `tstapler` namespace is still Tyler's open decision, not to be hardcoded ahead of it) and captures the resulting commit via `gh api`. - Files: `stapler-scripts/llm-sync/scripts/fork_pin_extension.py` ##### Task 1.3.1b: Add manifest-skeleton writing (~5 min) @@ -285,6 +311,10 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - Prints the planned fork + manifest diff without calling `gh`. - Files: `stapler-scripts/llm-sync/scripts/fork_pin_extension.py` +##### Task 1.3.1e: Add the dry-run-matches-real-run test (~5 min) +- Per architecture-review.md's remediation for its `fork_pin_extension.py` concern: mock the `gh` subprocess calls (fork creation, commit lookup) and assert that `--dry-run`'s computed manifest-entry diff is identical to the manifest entry a real (non-dry-run) run actually persists to `.config/pi/extensions-manifest.json`. Without this, the no-approval-path guarantee (Task 1.3.1c) could hold for the dry-run code path while the actually-shipped real-run path diverges undetected. +- Files: `stapler-scripts/llm-sync/test_fork_pin_extension.py` + ### Epic 1.4: Review Process Skill **Goal**: Turn `extension-audit.md`'s "Required gate" checklist into a discoverable, reusable skill so no agent (or Tyler, in a hurry) skips a step. @@ -293,13 +323,14 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Acceptance Criteria**: - The skill names the code that enforces the gate, not just convention. - - *Given* `.claude/skills/pi-extension-review/SKILL.md`, *When* read, *Then* it names `verify_pinned_sources_reviewed()` in `stapler-scripts/llm-sync/src/sources/extension_manifest.py` as the enforcement point and states that only a human edits `disposition`/`approved_by`/`approved_date`. + - *Given* `.claude/skills/pi-extension-review/SKILL.md`, *When* read, *Then* it names `verify_pinned_sources_reviewed()` in `stapler-scripts/llm-sync/src/sources/review_gate.py` as the enforcement point and states that only a human edits `disposition`/`approved_by`/`approved_date`. - The skill's checklist matches `extension-audit.md`'s seven-step gate. - *Given* the skill file, *When* its checklist section is compared to `project_plans/pi-dotfiles/extension-audit.md`'s "Required gate before enabling any candidate" list, *Then* all seven steps are present. **Files**: `.claude/skills/pi-extension-review/SKILL.md` ##### Task 1.4.1a: Write frontmatter + the seven-step checklist (~5 min) +- The seven-step checklist must include a maintenance-liveness check (last commit date, release cadence, contributor count) recorded in the manifest entry's `notes`, so an unmaintained upstream is caught by the gate itself rather than surfacing only during Task 3.2.1c's review (pre-mortem Failure #3). - Files: `.claude/skills/pi-extension-review/SKILL.md` ##### Task 1.4.1b: Add "how to run the helper + validator" section (~4 min) @@ -355,6 +386,7 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Files**: `stapler-scripts/llm-sync/src/targets/pi_package_ledger.py`, `stapler-scripts/llm-sync/test_pi_package_ledger.py`, `stapler-scripts/llm-sync/src/cli.py` ##### Task 2.2.1a: Write `PiPackageLedger` (~5 min) +- Dependencies: Task 2.1.1b - `save`/`find_stale`/`prune`, modeled on `PiSettingsTarget`'s atomic-write pattern. Blocked on Epic 2.1's Task 2.1.1b per this story's precondition AC — do not start until that subsection exists. - Files: `stapler-scripts/llm-sync/src/targets/pi_package_ledger.py` @@ -402,7 +434,7 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **As** Tyler, **I want** every fork-and-enable story to declare "a human has recorded approval" as its first acceptance criterion, **so that** an agent can't skip it. **Acceptance Criteria**: -- Each story in Epics 3.2-3.5 and 4.1 states the approval precondition before any fork action. +- Each story in Epics 3.2, 3.3, 3.5, and 4.1 that forks a third-party repo states the approval precondition before any fork action; Epic 3.4 is exempt because it forks nothing (Tyler-owned, local-path extension). - *Given* Story 3.2.1, *When* its Acceptance Criteria are read, *Then* the first bullet is the approval precondition, not a fork action. - No task in Phase 3 or Phase 4 programmatically sets `disposition: "approved"`. - *Given* every task in Phase 3 and Phase 4, *When* scanned for `gh repo fork` invocations or `"approved"`-literal edits, *Then* none appear outside a task explicitly marked `[Tyler, manual]`. @@ -413,7 +445,7 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - No files; this story is satisfied structurally by the stories that follow and by Task 1.3.1c's regression test. ### Epic 3.2: Permission System & Subagents Fork (`gotgenes/pi-packages`) -**Goal**: Once approved, fork, review, and pin `gotgenes/pi-packages` for the permission system and subagents — the strongest foundation per `full-featured-profile-research.md` — wired disabled by default. +**Goal**: Once approved, fork, review, and pin `gotgenes/pi-packages` for the permission system and subagents — the strongest foundation per `full-featured-profile-research.md` — wired disabled by default. If `gotgenes/pi-packages` is rejected at Task 3.2.1c's review or found unmaintained (per the new maintenance-liveness check), Tyler records the decision as a new Unresolved Question and either names an alternate Tier-1 candidate from `full-featured-profile-research.md` or scopes a Tyler-owned minimal replacement before Phase 3/4 continue (pre-mortem Failure #3). #### Story 3.2.1: Fork, review, and pin `gotgenes/pi-packages` **As** Tyler, **I want** `@gotgenes/pi-permission-system` and `@gotgenes/pi-subagents` forked, reviewed, and pinned, **so that** plan/subagent workflows have a deny-by-default safety boundary before anything else in the profile is enabled. @@ -428,13 +460,14 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Files**: `.config/pi/extensions-manifest.json`, `.config/pi/config.d/50-gotgenes-permissions.json` ##### Task 3.2.1a: PRECONDITION — Tyler only, manual — Approve and fork (~5 min) -- `gh repo fork gotgenes/pi-packages --org tstapler` at the review anchor; record approval date/notes. +- `gh repo fork gotgenes/pi-packages` (target namespace per Tyler's resolution of the org-vs-personal Unresolved Question) at the review anchor; record approval date/notes. ##### Task 3.2.1b: Scaffold via the helper (~2 min) - `fork_pin_extension.py fork gotgenes/pi-packages --id gotgenes-pi-packages --capability permission-system,subagents` -##### Task 3.2.1c: Manual review, not code — Complete the checklist (~5 min) +##### Task 3.2.1c: Manual review, not code — Complete the checklist (~2-4 hours, not a code task — a real security review session) - Includes: verify fail-closed parser paths, deny/ask defaults for writes/external paths/credentials/git publication/package installation/destructive commands, children don't inherit secrets/capabilities by default, worktree cleanup — per `full-featured-profile-research.md`'s "Required review" list. Record findings in the manifest entry's `notes`. +- This plan's general "~2-5 min" sizing convention is for mechanical code tasks; it does not apply here — reviewing an unfamiliar codebase's fail-closed paths and deny/ask coverage realistically takes hours, and under-budgeting it risks the rubber-stamp failure mode Step 0.5 and pre-mortem Failure #1 already warn against. ##### Task 3.2.1d: Approve the manifest entry (~2 min) - Hand-edit `disposition` to `"approved"` with `approved_by`/`approved_date`. @@ -447,6 +480,8 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership #### Story 3.2.2: Validate on the current machine via a temp Pi home first **As** Tyler, **I want** the permission system enabled with a conservative deny/ask policy on my current macOS machine only, validated in a temp Pi home first, **so that** I can catch problems before wider rollout. +**Dependencies**: Story 5.2.2 — Tyler's real-machine adoption step (Task 3.2.2b) must not proceed until Story 5.2.2's classification table exists, so a managed sync can never overwrite an unclassified work-only or machine-generated key on the real machine. + **Acceptance Criteria**: - A machine-local fragment enables the permission system with deny defaults. - *Given* `~/.config/pi/config.local.d/10-permissions.json` (untracked) with `packages.gotgenes-pi-permission-system.enabled: true` and a conservative policy object, *When* rendered in a temp `HOME`, *Then* the resulting `settings.json` shows the package enabled with that policy. @@ -456,9 +491,11 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Files**: none tracked (machine-local by design) — validated via existing `--dry-run`/`--pi-dir` flags. ##### Task 3.2.2a: Tyler, manual — Author the machine-local fragment (~5 min) +- Dependencies: Task 3.2.1d - Deny/ask defaults per the research doc's required review list. ##### Task 3.2.2b: Tyler, manual — Validate via temp `--pi-dir`, then adopt (~3 min) +- Dependencies: Task 3.2.1d, Story 5.2.2 — the "adopt" half of this task (enabling on the real machine, not the temp `--pi-dir` validation) must wait until Story 5.2.2's classification table exists, so the first managed sync on the real machine never overwrites an unclassified key. - Per Phase 5's runbook. ### Epic 3.3: Plan Mode Fork (`narumiruna/pi-extensions`) @@ -480,8 +517,15 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership ##### Task 3.3.1b: Scaffold via the helper (~2 min) -##### Task 3.3.1c: Manual review, not code — Complete the checklist, including an integration test proving plan mode and the permission system compose most-restrictively (~5 min) -- Per `full-featured-profile-research.md`'s explicit instruction: "add integration tests proving that the two independent gates compose most-restrictively and cannot reactivate a tool denied by the other." +##### Task 3.3.1c: Manual review, not code — Complete the checklist (~2-4 hours, not a code task — a real security review session) +- Dependencies: Task 3.2.1d +- Fail-closed parser paths, deny/ask defaults, children-don't-inherit-secrets/capabilities, worktree cleanup — per `full-featured-profile-research.md`'s "Required review" list, same shape as Task 3.2.1c. Record findings in the manifest entry's `notes`. +- This plan's general "~2-5 min" sizing convention is for mechanical code tasks; it does not apply here — reviewing an unfamiliar codebase's fail-closed paths and deny/ask coverage realistically takes hours, matching Task 3.2.1c's and 4.1.1c's sizing convention. + +##### Task 3.3.1c-2: Write the integration test proving plan mode and the permission system compose most-restrictively (~10-15 min) +- Dependencies: Task 3.2.1d +- A separate code-writing deliverable from Task 3.3.1c's manual review — per `full-featured-profile-research.md`'s explicit instruction: "add integration tests proving that the two independent gates compose most-restrictively and cannot reactivate a tool denied by the other." Requires Task 3.2.1's permission-system fork to be approved first, since this test exercises composition with it. Sized separately from the manual-review budget because it's real code (an integration test spanning two forks), not review time. +- Files: `stapler-scripts/llm-sync/test_review_gate.py` (or a new `stapler-scripts/llm-sync/test_plan_mode_permission_composition.py` if the fixture setup warrants its own file) ##### Task 3.3.1d: Approve the manifest entry (~2 min) - Files: `.config/pi/extensions-manifest.json` @@ -514,7 +558,7 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership ##### Task 3.4.1d: Add a regression test for the local-path exemption (~3 min) - Confirms `verify_pinned_sources_reviewed()`'s scan already exempts local `path` sources by construction. -- Files: `stapler-scripts/llm-sync/test_extension_manifest.py` +- Files: `stapler-scripts/llm-sync/test_review_gate.py` ### Epic 3.5: Command-Hook Bridge Fork (`hsingjui/pi-hooks`) — blocked on license **Goal**: Fork and pin the Claude-hook-compatible bridge once its missing-license concern is resolved (Unresolved Questions). @@ -559,7 +603,8 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership ##### Task 4.1.1b: Scaffold via the helper (~2 min) -##### Task 4.1.1c: Manual review, not code — Complete the checklist with explicit scoping/redaction/no-inheritance notes (~5 min) +##### Task 4.1.1c: Manual review, not code — Complete the checklist with explicit scoping/redaction/no-inheritance notes (~2-4 hours, not a code task — a real security review session) +- This plan's general "~2-5 min" sizing convention is for mechanical code tasks; it does not apply here — verifying vault-item scoping, output redaction, and no-subagent-inheritance in an unfamiliar codebase realistically takes hours. ##### Task 4.1.1d: Approve the manifest entry (~2 min) - Files: `.config/pi/extensions-manifest.json` @@ -579,10 +624,15 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - *Given* the full set of files this plan adds or edits, *When* grepped for `auth.json`, *Then* no match exists outside prose documentation. - A redaction dry-run never prints a resolved secret value. - *Given* a machine-local test with a dummy 1Password reference, *When* the extension resolves it in dry-run/log mode, *Then* the log shows the reference string (e.g. `!op read op://vault/item/field`), not the resolved value — VERIFIED manually by Tyler on his own machine (requires a real 1Password session, not reproducible in CI). +- A resolved secret does not appear in the model-context transcript, not just stdout logs. + - *Given* the same dummy 1Password reference resolved during a live Pi session, *When* the actual tool-call transcript/session-history entry the extension produces is inspected (not stdout), *Then* it shows the reference string, not the resolved value — VERIFIED manually by Tyler (same non-CI-reproducible constraint as the log check, since it requires a real 1Password session). +- A resolved secret is not inherited by a child/subagent process's environment. + - *Given* a subagent spawned (via the Epic 3.2 permission-system/subagents fork) from a session where the credential provider has resolved a value, *When* the subagent process's environment is inspected, *Then* the resolved value is absent — cross-referencing and re-verifying, specifically for the credential provider, Task 3.2.1c's "children don't inherit secrets by default" finding, which as reviewed there is scoped only to the permission-system fork. **Files**: `.config/pi/config.d/90-credential-1password.json`, `.config/pi/README.md` ##### Task 4.2.1a: Add the opt-in fragment, disabled by default (~3 min) +- Dependencies: Task 4.1.1d - Files: `.config/pi/config.d/90-credential-1password.json` ##### Task 4.2.1b: Document the `auth.json`-grep check (~3 min) @@ -591,6 +641,15 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership ##### Task 4.2.1c: Tyler, manual — Perform the redaction dry-run and record the VERIFIED result in the manifest entry's `notes` (~5 min) - Files: `.config/pi/extensions-manifest.json` +##### Task 4.2.1d: Tyler, manual — Inspect the tool-call transcript/session-history entry for the resolved value and record the VERIFIED result (~5 min) +- Same dummy-reference setup as Task 4.2.1c, but inspects the session-history/transcript file Pi writes for the tool call, not stdout — the channel requirements.md's Rabbit Holes names as "model context." +- Files: `.config/pi/extensions-manifest.json` + +##### Task 4.2.1e: Tyler, manual — Confirm no subagent-environment inheritance and record the VERIFIED result (~5 min) +- Dependencies: Task 3.2.1d — this task needs the Epic 3.2 permission-system/subagents fork approved and available to actually spawn a subagent against; testing subagent-environment inheritance has nothing to test without it. Approval alone is not sufficient to exercise the check: the subagents package is wired disabled-by-default in tracked config (Story 3.2.1e), so this task also requires a temporary/sandbox enablement of the package — reuse Story 3.2.2's temp-`--pi-dir` validation pattern rather than inventing a new mechanism — to actually spawn a subagent against. +- Spawns a subagent from a session with a resolved credential and inspects its process environment; re-verifies Task 3.2.1c's finding specifically for the credential provider rather than assuming it carries over from the permission-system fork. +- Files: `.config/pi/extensions-manifest.json` + --- ## Phase 5: Validation, Rollback, Documentation @@ -605,6 +664,8 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - *Given* `project_plans/pi-dotfiles/implementation/rollout-runbook.md`, *When* compared to requirements.md's Risk Control list, *Then* each of the five steps has a corresponding runbook section with a runnable command. - Step 1 names the exact commands already available. - *Given* the runbook's step 1 section, *When* read, *Then* it shows `uv run --directory stapler-scripts/llm-sync main.py --target pi --dry-run --pi-dir /tmp/pi-staging` and `uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry`. +- The runbook states the pre-`pi_install_version`-bump smoke-test gate as required, not optional. + - *Given* the runbook, *When* read, *Then* it has a section stating that any `pi_install_version` change must first pass a temp-`HOME` smoke test of every `approved` manifest entry plus a re-run of Task 3.3.1c-2's composition integration test, before the version pin in `bootstrap-pyinfra/group_data/all.py` is edited. **Files**: `project_plans/pi-dotfiles/implementation/rollout-runbook.md` @@ -616,6 +677,10 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - Adopt+verify macOS; verify idempotency/rollback; roll out Linux families. - Files: `project_plans/pi-dotfiles/implementation/rollout-runbook.md` +##### Task 5.1.1c: Write the pre-`pi_install_version`-bump smoke-test runbook section (~5 min) +- Documents the gate as a required step before editing `pi_install_version` in `bootstrap-pyinfra/group_data/all.py`: start `pi` against a temp `HOME` with every `approved` manifest entry's fork/commit enabled, assert each loads without error, and re-run Task 3.3.1c-2's plan-mode/permission-system composition integration test. Mirrors the Observability Plan's pre-bump gate line item. +- Files: `project_plans/pi-dotfiles/implementation/rollout-runbook.md` + ### Epic 5.2: Backup/Rollback Procedure #### Story 5.2.1: A tested rollback that never touches credentials/sessions/trust @@ -655,6 +720,8 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership **Acceptance Criteria**: - The matrix covers every Claude tool name referenced by the synced skill corpus. - *Given* `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` and a grep of `.claude/skills/**/SKILL.md` for Claude tool names (`AskUserQuestion`, `WebSearch`, `WebFetch`, `Agent`, `TaskCreate`, `Grep`, `Glob`, `LS`), *When* compared, *Then* every found tool name has a matrix row stating its Pi status (native, shimmed via `pi-claude-compat`, extension-provided, or unsupported). +- Deferred workflow categories are marked, not omitted. + - *Given* the matrix's category rows for session/workflow utilities (Tier 4, see "Explicitly deferred" above), *When* read, *Then* they are marked `deferred` with a pointer to the follow-up project, not silently absent from the matrix. **Files**: `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` @@ -662,7 +729,25 @@ Phase 3: Gated Phase 4: Credential Phase 2: Ownership - No file write; Bash/Grep only. ##### Task 5.3.1b: Write the matrix file (~5 min) -- One row per discovered tool name and its Pi status. +- One row per discovered tool name and its Pi status; mark Tier 4 session/workflow-utility categories `deferred` per the "Explicitly deferred" section. +- Files: `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` + +#### Story 5.3.2: Global and project instructions map to Pi's native loader +**As** a future coding agent or Tyler setting up a new machine, **I want** the instructions workflow category's Pi mapping documented and the global-level gap closed, **so that** "global and project instructions" parity (requirements.md In-Scope) is actually delivered, not just named in the glossary. + +**Acceptance Criteria**: +- Project-level parity is documented as already-native, with its source cited. + - *Given* `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md`'s instructions row, *When* read, *Then* it states Pi natively loads `AGENTS.md`/`CLAUDE.md` walking up from the current directory (citing `pi.dev/docs/latest/quickstart`), so this repo's existing project-level `CLAUDE.md` files already apply with no dotfiles change. +- Global-level parity is closed via the existing symlink tool, not new tooling. + - *Given* `.cfgcaddy.yml`'s existing `.claude/CLAUDE.md` entry (no `dest`, mirrors `$HOME/.claude/CLAUDE.md`), *When* a second entry is added for `dest: .pi/agent/AGENTS.md`, *Then* `~/.pi/agent/AGENTS.md` resolves to the same tracked content Claude already reads globally, giving global-instructions parity without a new mechanism. + +**Files**: `.cfgcaddy.yml`, `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` + +##### Task 5.3.2a: Add the `.pi/agent/AGENTS.md` cfgcaddy entry (~3 min) +- Files: `.cfgcaddy.yml` + +##### Task 5.3.2b: Document the mapping in the compat matrix (~3 min) +- One row: "global/project instructions" — native (project-level, via Pi's own `AGENTS.md`/`CLAUDE.md` loader) + symlinked (global-level, via the new cfgcaddy entry). - Files: `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` ### Epic 5.4: Documentation Sync diff --git a/project_plans/pi-dotfiles/implementation/pre-mortem.md b/project_plans/pi-dotfiles/implementation/pre-mortem.md new file mode 100644 index 00000000..81898a23 --- /dev/null +++ b/project_plans/pi-dotfiles/implementation/pre-mortem.md @@ -0,0 +1,18 @@ +# Pre-mortem: pi-dotfiles +**Date**: 2026-09-15 + +## Failure Modes + +| # | Failure | First Symptom | Prevention | Severity | +|---|---------|--------------|------------|----------| +| 1 | The `notes` field becomes a rubber stamp because nothing forces evidence that step 3 of the seven-step gate ("inspect lifecycle scripts, network destinations, subprocess execution, filesystem writes, environment access, secret handling, telemetry") was actually done against `gotgenes/pi-packages`'s real surface — a large, unfamiliar codebase reviewed under normal end-of-day time pressure, with the plan's own architecture review already conceding no code can verify note *quality*. | A manifest entry gets `disposition: "approved"` with generic `notes` ("reviewed, looks fine" or a one-line paraphrase) that doesn't reference any specific file/function in the fork at the reviewed commit. | Add a mechanical (not content-quality) schema check: `notes` must enumerate ≥N distinct file paths that exist in the fork tree at `fork_commit` (verified via `gh api repos/tstapler//git/trees/?recursive=1`), one per gate-checklist item 3 sub-area (network/subprocess/filesystem/secrets/telemetry). This can't verify understanding, but it makes a copy-pasted or empty note fail a check instead of silently passing — raises the floor without pretending to raise the ceiling. | +| 2 | `verify_pinned_sources_reviewed()` independently re-derives repo+commit from rendered `packages`/`extensions` source strings instead of reusing `_is_allowed_package_source`'s parser (architecture-review.md's Lens 2/Reuse Check finding — flagged, not yet incorporated into plan.md's task text). If the fork-URL shape is ever loosened in one parser and not the other, an unreviewed source can render into `settings.json` with no error at all. | None — by construction this fails silently; the earliest real signal is Tyler noticing an extension behaving in a way he doesn't remember approving, which could be weeks after the divergence was introduced. | Extract the shared `parse_fork_source(source) -> SourceRef(repo, commit)` architecture-review.md already recommends, used by both `_is_allowed_package_source` and `verify_pinned_sources_reviewed()`, plus a metamorphic test asserting both functions agree on accept/reject and repo/commit extraction over the same fixture corpus — not just parallel example-based tests that can drift independently. | +| 3 | `gotgenes/pi-packages` — the plan's own "strongest foundation" for Epic 3.2, and an implicit prerequisite for Epic 3.3's required "plan mode composes most-restrictively with the permission system" integration test — fails Task 3.2.1c's review (e.g., a fail-open parser path) or the upstream repo goes unmaintained before/after the fork anchor commit. The seven-step gate has no maintenance-liveness check (last-commit date, issue backlog, bus factor), only security/license review, so this risk isn't even surfaced by the gate that's supposed to catch it. | Tyler starts the Task 3.2.1c checklist, finds a fail-open code path or an upstream repo with no commits in 6+ months, and there is no documented fallback candidate or decision point in the plan — Phase 3 and the Phase 3.3 integration-test dependency both stall indefinitely with no stated next step. | Add a maintenance-liveness sub-step to the seven-step gate (last commit date, release cadence, contributor count) as an explicit `notes` field, and record one alternate Tier-1 candidate per capability in `full-featured-profile-research.md` so a rejected/abandoned foundational fork has a documented fallback instead of silently stalling the plan's dependency chain. | +| 4 | The credential provider's only verification (Story 4.2.1, Task 4.2.1c) checks that *logs* show the reference string instead of the resolved secret — but requirements.md's own Rabbit Holes list names three leak channels (logs, **model context**, **subprocess environments**), and only one of the three is tested, and only once, manually, non-reproducibly ("requires a real 1Password session, not reproducible in CI"). Nothing checks whether a resolved secret ends up in the Pi session transcript sent to the model provider or inherited by a subagent process — which matters *especially* here because this same profile is adding subagent support via `gotgenes/pi-packages` in the same rollout. | Months after rollout, someone (or a future coding agent) inspects a Pi session history/transcript file and finds a plaintext resolved 1Password value in what was sent to the model as tool-call context — undetectable by the existing log-only check, since the value never touched a log line. | Extend Task 4.2.1c's redaction verification to explicitly test the two untested channels named in requirements.md: (a) inspect the actual tool-call transcript/session-history entry the extension produces when invoked, not just stdout logs, for the resolved value; (b) confirm the credential provider does not export the resolved value into a child/subagent process's environment (tie to Task 3.2.1c's "children don't inherit secrets by default" finding, which is currently scoped only to the permission-system fork, not cross-checked against the credential provider). | +| 5 | Extension forks are reviewed and pinned against Pi's extension API as of `pi_install_version = "0.84.4"`. Nothing in this plan ties a future `pi_install_version` bump to re-validating already-`approved` manifest entries — gate step 7 ("upgrades repeat this process") covers *extension* version upgrades, not core-Pi upgrades invalidating an unchanged, already-approved extension commit. If Pi's extension-loading behavior fails soft (ignores a broken extension rather than erroring), a `pi_install_version` bump could silently disable the permission-system's deny-by-default enforcement while `settings.json` still shows it as enabled — the opposite of what the whole governance effort exists to guarantee. | Tyler bumps `pi_install_version` for an unrelated reason, bootstrap converges with no reported error, but a later mutating action that should have been denied by the (now-broken) permission extension executes anyway — discovered only by the bad action's consequences, not by any check in this plan's Observability Plan (which covers install/update/skip decisions, not runtime extension-load success). | Add a smoke-test step gating any `pi_install_version` change: start `pi` against a temp `HOME` with every `approved` manifest entry's fork/commit enabled, assert each extension loads without error, and re-run Task 3.3.1c-2's plan-mode/permission-system composition integration test — turning core-Pi version bumps into a gated action analogous to extension re-pins, and adding this as a line item in the Observability Plan. | + +## P1 Items (address before implementation) + +- [x] Failure #1 — Resolved in `implementation/plan.md`: Tasks 1.1.2c/1.1.2d add the mechanical notes-evidence check (≥5 distinct file paths verified against the fork tree at `fork_commit` via the GitHub trees API) to Story 1.1.2, ahead of Phase 3/4 approval work. +- [x] Failure #4 — Resolved in `implementation/plan.md`: Tasks 4.2.1d/4.2.1e extend Story 4.2.1 to cover the model-context/session-transcript and subagent-environment-inheritance leak channels, not just log output. +- [x] Failure #5 — Resolved in `implementation/plan.md`: Task 5.1.1c adds the pre-`pi_install_version`-bump smoke test to the runbook, and a new Observability Plan line item ("Pre-bump gate (required, not optional)") makes it a required gate, not optional. diff --git a/project_plans/pi-dotfiles/implementation/rollout-runbook.md b/project_plans/pi-dotfiles/implementation/rollout-runbook.md new file mode 100644 index 00000000..6aa0b88d --- /dev/null +++ b/project_plans/pi-dotfiles/implementation/rollout-runbook.md @@ -0,0 +1,204 @@ +# Pi Staged-Rollout & Rollback Runbook + +Maps requirements.md's five-step Risk Control staged-adoption list 1:1 to +runnable commands. Covers Epic 5.1 (Story 5.1.1) and Epic 5.2's Story 5.2.1. +Story 5.2.2 (classifying the real machine's existing `settings.json` keys) +is **out of scope for this document as written** — see the note at the end +of Step 2. + +All `llm-sync` commands below assume the repo root as the working directory +and are run via `uv run --directory stapler-scripts/llm-sync main.py ...`. +Flag names are verified against `stapler-scripts/llm-sync/src/cli.py`'s +`main()` argparse block as of this writing; re-check that file if a flag +below fails with `unrecognized arguments`. + +## Step 1: Dry-run / temp-dir validation + +Validate merge and provisioning behavior without touching any real Pi +install. + +Config-sync dry run against a scratch directory instead of `~/.pi/agent`: + +```sh +uv run --directory stapler-scripts/llm-sync main.py \ + --target pi --dry-run --pi-dir /tmp/pi-staging +``` + +Pi *installation* dry run (separate concern from config sync — this +exercises `bootstrap-pyinfra/deploys/pi.py`'s install-mode logic without +installing anything, by forcing `external` mode for the run): + +```sh +cd bootstrap-pyinfra +uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry +``` + +`--dry` previews pyinfra's queued operations without executing them +(pyinfra's analog to `ansible-playbook --check`); `--data +pi_install_mode=external` overrides `pi_install_version`/`pi_install_mode` +for this run only, per `bootstrap-pyinfra/deploys/pi.py`'s `plan_pi_install` +(`external` mode always returns action `"external"` — "installation is +externally managed" — regardless of what's already on `PATH`). + +Both commands are non-destructive: the first writes nothing outside +`/tmp/pi-staging`, and the second's `--dry` withholds all queued pyinfra +operations, including the Pi install itself. + +## Step 2: Backup / inventory the current unmanaged Pi configuration + +Before any managed sync touches the real machine, back up the current +`settings.json` and record what's currently designated as dotfiles-owned. +This step is the same action as Task 5.2.1a below — see **"Backup before +adopting"** for the exact commands; it isn't duplicated here. + +**Scope note (Story 5.2.2, explicitly out of scope for this runbook as +written):** requirements.md's step 2 also implies classifying *which* +existing `settings.json` keys are universal, work-only, or +machine-generated, so a managed sync never silently overwrites an +unclassified key. That classification is plan.md's Story 5.2.2 +(Task 5.2.2a), a Tyler-only manual step against his real machine's +`~/.pi/agent/settings.json` — it requires data not available in this +environment and is **not done by this document**. Per plan.md's +Dependency Visualization ("back-edge" note) and Task 3.2.2b's +`Dependencies:` line, Story 5.2.2's classification table must exist +*before* Story 3.2.2's real-machine adoption step (Step 3 below) runs on +the actual machine — backing up and dry-running (this step and Step 1) can +proceed without it, but adopting on the real machine cannot. + +## Steps 3-5: Adopt, verify, and roll out + +### Step 3: Adopt and verify on the current macOS machine + +Once Step 2's backup exists and Story 5.2.2's classification table is +recorded (see the scope note above — this is a precondition, not something +this runbook performs), run the real sync against the real Pi settings +location: + +```sh +uv run --directory stapler-scripts/llm-sync main.py --target pi +``` + +This uses `cli.py`'s defaults: `--pi-dir` defaults to `~/.pi/agent`, and the +managed-key ownership state defaults to +`~/.config/llm-sync/pi-settings-state.json` (see `main()` in `cli.py`, +around the `PiSettingsTarget` construction). Verify by inspecting +`~/.pi/agent/settings.json` and confirming Pi itself starts and loads +config without error. + +### Step 4: Verify idempotent re-run and rollback + +Re-run the same command from Step 3 a second time with no config changes in +between; `PiSettingsTarget.save()` (`stapler-scripts/llm-sync/src/targets/pi_settings.py`) +diffs desired vs. existing state and reports no change when converged — a +second run should print that settings are already converged rather than +rewriting the file. Then rehearse rollback per **"Rollback"** below (Task +5.2.1b) — do not defer rollback verification to an actual incident; prove it +works while the blast radius is a scratch/staging copy or an easily +re-adoptable real machine. + +### Step 5: Roll out to supported Linux families + +No separate mechanism: re-run Steps 1, 3, and 4 unchanged on each target +Linux family (`bootstrap-pyinfra`'s deploys are OS-branching internally, +e.g. via `common.is_macos`/`is_wsl`, not via a different entrypoint). This +runbook's commands and the underlying `llm-sync`/`pyinfra` tooling are the +same regardless of platform; only the machine backup taken in Step 2 +differs per host. + +## Pre-`pi_install_version`-bump smoke-test gate (Task 5.1.1c) — REQUIRED + +This gate is **required, not optional**, and applies independently of the +staged-rollout steps above. It must run before any future edit to +`pi_install_version` in `bootstrap-pyinfra/group_data/all.py`: + +1. Start `pi` against a temporary `HOME` with every manifest entry currently + at `disposition: "approved"` in `.config/pi/extensions-manifest.json` + enabled with its pinned fork/commit. +2. Assert each such extension loads without error. +3. Re-run plan.md's Task 3.3.1c-2 plan-mode/permission-system composition + integration test as a regression check. + +**Forward reference — this test does not exist yet.** As of this plan's +current implementation state, Task 3.3.1c-2's composition integration test +is blocked on Epics 3.2/3.3, which are themselves blocked on Tyler's manual +fork-and-review steps (see plan.md's Unresolved Questions and Dependency +Visualization). Do not treat step 3 above as already satisfied by an +existing test — confirm the test exists and passes at the time of the +version bump, not by reference to this runbook. + +Rationale (mirrors the Observability Plan's pre-bump gate line item in +plan.md): a core-Pi version bump can silently break an unchanged, +already-approved extension's runtime behavior — for example, fail-soft +extension loading masking a broken permission gate — which none of the +existing install/update/skip logging surfaces on its own. + +## Backup before adopting (Task 5.2.1a) + +Before the first managed sync touches the real machine's `settings.json`, +take both a file backup and a record of the `managedKeys` state at that +time: + +```sh +cp ~/.pi/agent/settings.json ~/.pi/agent/settings.json.pre-dotfiles-backup +cp ~/.config/llm-sync/pi-settings-state.json \ + ~/.config/llm-sync/pi-settings-state.json.pre-dotfiles-backup +``` + +The second copy may not exist yet on a machine that has never been synced +(`PiSettingsTarget._read_object` treats a missing state file as `{}`, +i.e. no `managedKeys` yet) — in that case skip it; there's nothing to +back up. + +`~/.config/llm-sync/pi-settings-state.json` is `cli.py`'s confirmed default +for `--pi-settings-state-file` (`main()`'s `PiSettingsTarget` construction: +`args.pi_settings_state_file or Path.home() / ".config" / "llm-sync" / +"pi-settings-state.json"`). + +## Rollback (Task 5.2.1b) + +Manual restore, then re-run without managed mode: + +```sh +cp ~/.pi/agent/settings.json.pre-dotfiles-backup ~/.pi/agent/settings.json +cp ~/.config/llm-sync/pi-settings-state.json.pre-dotfiles-backup \ + ~/.config/llm-sync/pi-settings-state.json +``` + +If the state-file backup didn't exist in the previous step (pre-managed +machine), remove `~/.config/llm-sync/pi-settings-state.json` instead of +restoring it, returning to the "never synced" starting state. + +**Scope guarantee, per Story 5.2.1's acceptance criteria:** this rollback +restores `settings.json` and the state file to their exact backup-time +contents — a whole-file copy, not a `managedKeys`-scoped patch. In the +intended workflow (nothing but `PiSettingsTarget` touches these files +between backup and rollback), that has the same effect as reverting only +the `managedKeys`-tracked keys, because `PiSettingsTarget.save()` never +writes a key outside `managedKeys`. But if anything else — Pi itself, a +manual edit — changes a non-managed key in that window, the whole-file copy +reverts that too; it is not preserved. Reading `PiSettingsTarget.save()` +(`stapler-scripts/llm-sync/src/targets/pi_settings.py`) confirms why the +part of this restore that *is* unconditional is safe and scoped: + +- `PiSettingsTarget` only ever reads and writes two paths: `settings_path` + (`~/.pi/agent/settings.json`) and `state_path` + (`~/.config/llm-sync/pi-settings-state.json`, or their overrides). It has + no code path that touches any other file under `~/.pi/agent/`. +- `auth.json` and session files under `~/.pi/agent/` are therefore **never + touched by rollback** — the restore procedure only copies the two files + above, so it has no path that could reach them, regardless of + `managedKeys` state. This part of the guarantee holds unconditionally. +- Do not re-run `llm-sync main.py --target pi` immediately after a + rollback copy without first confirming you want managed mode back — the + restored state file's `managedKeys` will cause the next real sync to + resume managing exactly those same keys. + +This is the manual equivalent of the automated check +`test_rollback_restores_only_managed_keys_and_preserves_auth_and_sessions` +in `stapler-scripts/llm-sync/test_pi_settings_target.py`, referenced in +`project_plans/pi-dotfiles/implementation/validation.md`'s Migration Note +section. That test exercises this section's guarantee against a `tmp_path` +fixture with dummy `auth.json`/session files, asserting they are +byte-identical before and after — and also exercises the whole-file-copy +caveat above: a non-`managedKeys` key changed by something other than +`PiSettingsTarget` between backup and rollback is reverted too. diff --git a/project_plans/pi-dotfiles/implementation/validation.md b/project_plans/pi-dotfiles/implementation/validation.md new file mode 100644 index 00000000..8a4e0c7d --- /dev/null +++ b/project_plans/pi-dotfiles/implementation/validation.md @@ -0,0 +1,133 @@ +# Validation Plan: pi-dotfiles + +**Date**: 2026-09-15 + +## Happy Path Scenario + +Given a personal macOS/Linux machine with no Pi installed and a clean clone of the dotfiles repo (the Baseline in `requirements.md`), when a bootstrap run executes `pi()` followed by `llm_sync()` — installing Pi, rendering `.config/pi/config.json` + `config.d/*` + machine-local overrides through `PiConfigSource`, checking every third-party fork source against an `approved` `.config/pi/extensions-manifest.json` entry via `verify_pinned_sources_reviewed()`, and writing only `managedKeys`-tracked keys via `PiSettingsTarget.save()` — then the machine ends with a working Pi install, a rendered `~/.pi/agent/settings.json` containing exactly the approved/pinned/enabled packages and synced Claude skills/prompts, no credential material anywhere in the written files, and a second identical run makes no further changes. + +## Requirement → Test Mapping + +Tests marked **(existing)** already exist in the codebase and are cited, not re-specified, per the reconciliation instruction. Tests without that marker are net-new, named to match this repo's existing convention (`test__[_when_]`, plain functions self-discovered by each file's `if __name__ == "__main__"` block — see Test Stack). Where plan.md's own Task-level GWT list already enumerates a test, the Scenario column cites the Task id. + +| Requirement | Test File | Test Name | Type | Scenario | +|---|---|---|---|---| +| SM-1: bootstrap installs Pi on a personal machine; work overlay can declare `external` | `bootstrap-pyinfra/test_pi_install.py` | `test_auto_installs_only_when_absent` (existing) | Unit | Happy — `auto` mode installs when Pi is absent | +| SM-1 (error) | `bootstrap-pyinfra/test_pi_install.py` | `test_external_never_installs` (existing) | Unit | Error — `external` mode never installs, even when absent (work overlay case) | +| SM-1 (integration) | `bootstrap-pyinfra/` (manual) | `pi_install_dry_run_external_mode_skips_with_reason` | Integration (dry-run only — real installs need brew/curl and are not run in CI) | `uv run pyinfra -y inventory.py main.py --data pi_install_mode=external --dry` prints the skip-with-reason line (matches UX Surface 4, criterion 4) | +| SM-2: same run provisions pinned extensions/packages + syncs Claude assets | `stapler-scripts/llm-sync/test_pi_config.py` | `test_renders_enabled_registries_to_native_pi_settings` (existing) | Unit | Happy — enabled registries render to native `packages`/`extensions` arrays | +| SM-2 (error) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_rejects_unpinned_untrusted_remote_package` (existing) | Unit | Error — unpinned, untrusted source rejected before render | +| SM-2 (integration) | `stapler-scripts/llm-sync/test_cli_targeting.py` | `test_pi_target_does_not_mutate_other_agents` (existing) | Integration | CLI `--target pi` end-to-end through `sync_pi_settings()`; regression anchor for existing skill/prompt sync (requirements' Baseline) | +| SM-3: tracked/local layers add/replace/disable without editing universal source | `stapler-scripts/llm-sync/test_tiered_config.py` | `test_loads_four_layers_in_precedence_order` (existing) | Unit | Happy — four-tier precedence resolves correctly | +| SM-3 (error) | `stapler-scripts/llm-sync/test_tiered_config.py` | `test_rejects_non_object_layer` (existing) | Unit | Error — malformed fragment layer rejected with layer path named | +| SM-3 (integration) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_later_layer_can_disable_a_base_registry_entry` (existing) | Integration | A later config.d/local layer disables a universal-base registry entry end-to-end through `PiConfigSource.load()` | +| SM-4: work overlay contributes packages/defaults without touching universal config | `stapler-scripts/llm-sync/test_review_gate.py` | `test_verify_pinned_sources_reviewed_exempts_trusted_scope_source_with_empty_manifest` (Task 1.2.2b) | Unit | Happy — `trustedPackageScopes`-listed work source needs no manifest entry, even with empty manifest | +| SM-4 (error) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_rejects_package_from_scope_not_listed_in_trusted_package_scopes` (existing) | Unit | Error — a scope not declared in `trustedPackageScopes` is still rejected | +| SM-4 (integration) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_work_overlay_config_d_fragment_merges_without_editing_universal_base` | Integration (tmp_path-style fixture simulating an overlay-supplied `config.d` fragment directory) | A fragment supplied from a separate overlay directory merges into the rendered config; `config.json` is asserted byte-unchanged | +| SM-5: re-running bootstrap is idempotent | `stapler-scripts/llm-sync/test_pi_settings_target.py` | `test_second_identical_save_is_idempotent` (existing) | Unit | Happy — second save of unchanged input writes nothing further | +| SM-5 (error) | `stapler-scripts/llm-sync/test_pi_settings_target.py` | `test_rejects_malformed_ownership_state` (existing) | Unit | Error — malformed `managedKeys` state file rejected rather than silently reset | +| SM-5 (integration) | `stapler-scripts/llm-sync/` (new) | `test_main_pi_target_run_twice_produces_byte_identical_settings` | Integration (subprocess: `uv run main.py --target pi` invoked twice against a tmp `--pi-dir`) | Full CLI run twice; second run's `settings.json` and package ledger are byte-identical to the first, per the Determinism/Idempotency NFRs | +| SM-6: dry-run/temp-dir validation completes before real adoption | `stapler-scripts/llm-sync/test_pi_settings_target.py` | `test_dry_run_does_not_write_files` (existing) | Unit | Happy — `--dry-run` writes nothing | +| SM-6 (error) | `stapler-scripts/llm-sync/` (new) | `test_dry_run_reports_manifest_error_without_partial_write` | Unit | Error — a malformed/unreviewed manifest still reports the same `PiConfigError` under `--dry-run`, and no partial `settings.json` write occurs | +| SM-6 (integration) | `stapler-scripts/llm-sync/` (new) | `test_cli_dry_run_pi_dir_leaves_real_pi_dir_untouched` | Integration (subprocess) | `--dry-run --pi-dir /tmp/pi-staging` prints intended changes; the real `~/.pi/agent` (mocked path) is asserted untouched — Story 3.2.2 / UX Surface 4 | +| SM-7: full Claude workflow-category parity is tracked, not assumed | `project_plans/pi-dotfiles/extensions/claude-tool-compat-matrix.md` (Task 5.3.1) | `test_compat_matrix_covers_every_discovered_claude_tool_name` | Integration (grep-based content check: `.claude/skills/**/SKILL.md` corpus vs. matrix rows) | Happy — every tool name found by the grep has a matrix row | +| SM-7 (error) | same | `test_compat_matrix_flags_tool_name_missing_a_row` | Integration | A tool name present in the skill corpus but absent from the matrix fails the check (regression guard so the matrix can't silently drift after this plan ships) | +| SM-8: no credential material written to or through dotfiles | `stapler-scripts/llm-sync/test_pi_config.py` | `test_rejects_credential_material_in_nested_settings` (existing) | Unit | Happy/enforcement — credential-shaped key anywhere in the tree is rejected before render | +| SM-8 (error) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_rejects_reserved_resource_keys_inside_settings` (existing) | Unit | Error — a `settings.*.packages` bypass of the registry mechanism is rejected | +| SM-8 (integration) | `stapler-scripts/llm-sync/test_pi_config.py` | `test_pi_config_rejects_credential_shaped_key_in_hypothetical_1password_fragment` (Task 4.1.1e) | Integration | Regression test specifically against a `jmcombs-pi-1password`-shaped fragment, confirming the credential-provider extension doesn't get a carve-out | +| SM-8 (repo-wide) | repo root (new) | `test_repo_has_no_auth_json_reference_outside_prose` | Integration (repo-wide grep, Task 4.2.1b) | Grep every file this plan adds/edits for `auth.json`; only prose-documentation matches are allowed | +| SM-8 (manual, non-CI) | `.config/pi/extensions-manifest.json` `notes` field | `redaction_dry_run_never_prints_resolved_secret_value` (Task 4.2.1c) | Manual — VERIFIED by Tyler only; requires a live 1Password session, explicitly not reproducible in CI per plan.md Story 4.2.1 | A dummy `!op read` reference resolves to the reference string in logs, never the secret value | +| SM-9: a reusable skill documents/enforces the tiered/config.d model | `.claude/skills/tiered-configuration/SKILL.md` (already exists per plan.md's Baseline) | `test_tiered_configuration_skill_documents_config_d_precedence` | Doc (content-check: required sections present) | Already satisfied — regression check only, confirming the skill isn't later deleted/hollowed out | +| C-1: third-party extensions forked, reviewed, pinned to exact commit | `stapler-scripts/llm-sync/test_review_gate.py` | `test_verify_pinned_sources_reviewed_raises_when_source_has_no_manifest_entry` (Task 1.2.1c) | Unit | Error — no manifest entry at all blocks the sync | +| C-1 (cont.) | same | `test_verify_pinned_sources_reviewed_passes_when_approved_entry_matches_exact_commit` (Task 1.2.1c) | Unit | Happy — approved entry at the exact pinned commit passes | +| C-1 (cont.) | same | `test_verify_pinned_sources_reviewed_raises_when_commit_mismatched` (Task 1.2.1c) | Unit | Error — stale approval (different commit) still blocks, naming both commits | +| C-1 (cont.) | `stapler-scripts/llm-sync/test_review_gate.py` | `test_verify_pinned_sources_reviewed_raises_when_disposition_is_hold` / `..._rejected` (Task 1.2.1e) | Unit | Error — presence without `approved` disposition still blocks, for both `hold` and `rejected` | +| C-1 (cont.) | same | `test_verify_pinned_sources_reviewed_matches_commit_case_insensitively` (Task 1.2.1e) | Unit | Happy — uppercase/lowercase SHA pairs match | +| C-1 (cont.) | same | `test_verify_pinned_sources_reviewed_treats_prefix_length_mismatch_as_mismatch` (Task 1.2.1e) | Unit | Error — a full 40-char SHA vs. its own 7-char prefix is a mismatch, not a match | +| C-1 (cont.) | `stapler-scripts/llm-sync/test_extension_manifest.py` | `test_extension_manifest_load_raises_on_duplicate_entry_id` (Task 1.2.1f) | Unit | Error — duplicate manifest id rejected at load time, before the gate runs | +| C-1 (integration) | `stapler-scripts/llm-sync/` (new) | `test_sync_pi_settings_aborts_and_writes_nothing_when_source_unreviewed` | Integration (CLI subprocess) | `sync_pi_settings()` calls `verify_pinned_sources_reviewed()` before `PiSettingsTarget.save()`; on failure, `settings.json` is confirmed unwritten (matches UX Surface 2, criterion 3) | +| C-1 (approval gate) | `stapler-scripts/llm-sync/test_fork_pin_extension.py` | `test_fork_pin_extension_argparser_has_no_flag_that_writes_approved_disposition` (Task 1.3.1c) | Unit (static inspection of the typer app) | The gate's approval field cannot be scripted — matches Approval Gate glossary entry | +| C-1 (helper, integration) | `stapler-scripts/llm-sync/test_fork_pin_extension.py` | `test_fork_pin_extension_fork_creates_candidate_manifest_entry_with_fork_commit` | Integration (`gh` subprocess mocked) | Happy — `fork` subcommand writes a `candidate` entry with the real fork commit | +| C-1 (helper, error) | `stapler-scripts/llm-sync/test_fork_pin_extension.py` | `test_fork_pin_extension_fork_fails_when_id_already_has_manifest_entry` | Integration (mocked `gh`) | Error — re-running `fork` for an existing `--id` fails, naming the id and its current disposition (UX Surface 1, criterion 4) | +| C-1 (helper, dry-run) | `stapler-scripts/llm-sync/test_fork_pin_extension.py` | `test_fork_pin_extension_dry_run_prints_plan_without_calling_gh` (Task 1.3.1d) | Integration (mocked `gh`, asserts zero subprocess calls) | Dry-run shows the planned fork + manifest diff; no `gh` call, no file write | +| C-2: work-owned/Tyler-owned packages exempt from fork requirement | `stapler-scripts/llm-sync/test_review_gate.py` | `test_verify_pinned_sources_reviewed_exempts_tstapler_scoped_source` (Task 1.2.2b) | Unit | Happy — `@tstapler`-scoped source exempted the same way as trusted work scopes | +| C-2 (error/regression) | same | `test_verify_pinned_sources_reviewed_only_scans_github_com_tstapler_fork_shaped_sources` (Task 1.2.2a) | Unit | A source that isn't fork-shaped (npm scope, local path) is never scanned by the gate at all | +| C-3: bootstrap never silently advances third-party/Tyler-owned versions | `stapler-scripts/llm-sync/test_pi_config.py` | `test_rejects_pinned_upstream_and_mutable_fork_ref` (existing) | Unit | Error — a mutable ref (branch/tag, not exact commit/version) is rejected | +| C-3 (cont.) | `stapler-scripts/llm-sync/test_review_gate.py` | (same C-1 commit-comparison tests above) | Unit | Commit-exactness is the enforcement mechanism for this constraint | +| C-4: existing skill/prompt sync must not regress | `stapler-scripts/llm-sync/test_pi_sync.py` | full existing suite (existing) | Unit + Integration | Regression anchor per requirements' Baseline — unchanged by this plan | +| C-4 (cont.) | `stapler-scripts/llm-sync/test_cli_targeting.py` | `test_pi_target_does_not_mutate_other_agents` (existing) | Integration | Confirms `--target pi` doesn't cross-contaminate Gemini/OpenCode/Antigravity outputs | +| C-5: local-path first-party extensions need no manifest entry | `stapler-scripts/llm-sync/test_review_gate.py` | `test_claude_compat_extension_path_source_exempt_from_manifest_gate` (Task 3.4.1d) | Unit | Happy — a `path:`-sourced local extension is exempt by construction (not scanned) | +| Package Ledger: sync records artifact paths | `stapler-scripts/llm-sync/test_pi_package_ledger.py` | `test_pi_package_ledger_save_records_artifact_paths_for_enabled_entries` (Task 2.2.1d) | Unit | Happy — one enabled entry produces one ledger record at the observed artifact path (blocked on Epic 2.1's spike per the story's stated precondition) | +| Package Ledger: stale detection is report-only | same | `test_pi_package_ledger_find_stale_reports_without_deleting` (Task 2.2.1d) | Unit | Happy — removed entry is reported by `find_stale()`; nothing is deleted | +| Package Ledger: prune deletes only ledger-recorded paths | same | `test_pi_package_ledger_prune_deletes_only_ledger_recorded_paths` (Task 2.2.1d) | Integration (real filesystem writes/deletes in tmp_path) | An unmanaged path outside the ledger is left untouched by `prune()` | +| Package Ledger: interrupted-write recovery | same | `test_pi_package_ledger_reconcile_recovers_entry_after_interrupted_write` (Task 2.2.1f) | Integration | `--reconcile-pi-package-ledger` rebuilds a missing ledger entry from `settings.json`/`managedKeys` state after a simulated crash between the settings write and the ledger write | +| Package Ledger (error) | same | `test_pi_package_ledger_save_rejects_malformed_ledger_state` | Unit | Error — malformed `pi-package-state.json`, mirroring `PiSettingsTarget`'s existing `test_rejects_malformed_ownership_state` pattern | +| Stale reporting surfaces in bootstrap output | `bootstrap-pyinfra/` (new) | `test_llm_sync_deploy_prints_stale_pi_package_report_line` (Task 2.2.2a doc claim, verified not just asserted) | Integration (subprocess through `llm_sync()`) | Confirms the pass-through claim in Task 2.2.2a's docstring by actually running the deploy against a fixture ledger, not just re-asserting the plan's prose | + +## UX Acceptance Tests + +25 tests, 5 per surface in `design/ux.md`. "Tool" is `manual` where the criterion requires a live `gh`/Pi/1Password session that isn't reproducible in CI (per plan.md's own explicit non-CI carve-outs), and `scripted CLI-output check` where the assertion is a subprocess-and-grep/JSON-diff test runnable in CI. + +| UX Criterion | Test File | Test Name | Tool | Steps | +|---|---|---|---|---| +| Surface 1, criterion 1 (dry-run shows plan, no `gh`/write) | `test_fork_pin_extension.py` | `test_fork_dry_run_shows_planned_target_and_manifest_diff_without_writing` | scripted CLI-output check | Run `fork --id --capability --dry-run` with `gh` mocked to fail loudly if called; assert stdout contains "Would fork:", "Would write to:", and the manifest file is byte-unchanged | +| Surface 1, criterion 2 (dry-run and real-run both show `candidate`/null approval) | `test_fork_pin_extension.py` | `test_fork_output_shows_candidate_disposition_and_null_approval_fields_always` | scripted CLI-output check | Run both with and without `--dry-run` (real run with `gh` mocked); grep stdout for `disposition: "candidate"`, `approved_by: null`, `approved_date: null` in both | +| Surface 1, criterion 3 (printed commit matches written commit) | `test_fork_pin_extension.py` | `test_fork_real_run_printed_commit_matches_manifest_written_commit` | scripted CLI-output check | Run a real (mocked-`gh`) fork; parse the printed `fork_commit` from stdout and the `fork_commit` written to the manifest file; assert equality (read-the-mutation-back) | +| Surface 1, criterion 4 (re-run on existing id names id + disposition) | `test_fork_pin_extension.py` | `test_fork_rerun_existing_id_fails_naming_id_and_current_disposition` | scripted CLI-output check | Pre-seed a manifest entry with `disposition: "approved"`; re-run `fork` with the same `--id`; assert stderr/exit-code failure message contains the id and the literal string `approved`, not a generic "already exists" | +| Surface 1, criterion 5 (dry-run closing line states next action) | `test_fork_pin_extension.py` | `test_fork_dry_run_closing_line_states_next_concrete_action` | scripted CLI-output check | Assert the last non-empty stdout line matches `Re-run without --dry-run...` | +| Surface 2, criterion 1 (error names exact unreviewed source string) | `test_review_gate.py` (new CLI-level test) | `test_blocked_sync_error_names_exact_unreviewed_source_string` | scripted CLI-output check | Run `main.py --target pi` against a fixture with an unreviewed fork source; assert the exact source string `git:github.com/tstapler/pi-permission-system@abc1234`-shaped text appears verbatim in stderr | +| Surface 2, criterion 2 (stale approval names both commits) | same | `test_blocked_sync_stale_approval_names_both_configured_and_approved_commits` | scripted CLI-output check | Fixture: config pinned at `def5678`, manifest approved at `abc1234`; assert both SHAs appear in the error text | +| Surface 2, criterion 3 (states settings.json unchanged) | same | `test_blocked_sync_error_states_settings_json_unchanged` | scripted CLI-output check | Assert stderr contains "Sync aborted; no changes were written" (or equivalent) and the target `settings.json` file (pre-seeded with known content) is byte-identical after the failed run | +| Surface 2, criterion 4 (trusted/`@tstapler` scopes never appear in this error path) | same | `test_blocked_sync_never_fires_for_trusted_or_tstapler_scoped_sources` | scripted CLI-output check | Run against a fixture with only trusted-scope/`@tstapler` sources and an empty manifest; assert exit code 0 and no `PiConfigError`/`unreviewed fork source` text anywhere in output | +| Surface 2, criterion 5 (names next action: review skill then hand-edit) | same | `test_blocked_sync_error_names_next_action_review_skill_then_hand_edit` | scripted CLI-output check | Assert stderr's last lines reference `.claude/skills/pi-extension-review/SKILL.md` and `disposition: "approved"` | +| Surface 3, criterion 1 (exact stale-report line format) | `test_pi_package_ledger.py` (new CLI-level test) | `test_stale_report_line_matches_exact_format_without_prune_flag` | scripted CLI-output check | Byte-for-byte match against `stale Pi package: old-extension (not pruned; run with --prune-stale-pi-packages)` | +| Surface 3, criterion 2 (report always runs; delete only with flag) | same | `test_stale_report_always_runs_deletion_only_with_flag` | scripted CLI-output check | Run without the flag: report line present, artifact file still exists on disk. Run with the flag: artifact file gone | +| Surface 3, criterion 3 (pruned path printed after pruning) | same | `test_prune_flag_prints_pruned_path_after_deletion` | scripted CLI-output check | With `--prune-stale-pi-packages`, assert stdout contains `Pruned: ` after the "pruning..." line | +| Surface 3, criterion 4 (silence for unmanaged paths) | same | `test_prune_output_silent_for_paths_outside_ledger` | scripted CLI-output check | Seed an unmanaged file alongside a ledger-recorded stale one; assert the unmanaged path's name never appears anywhere in stdout and the file still exists after `--prune-stale-pi-packages` | +| Surface 3, criterion 5 (bootstrap pass-through reproduces the line unmodified) | `bootstrap-pyinfra/test_pi_install.py`-adjacent (new) | `test_bootstrap_llm_sync_reproduces_exact_stale_report_line` | scripted CLI-output check | Run the `llm_sync()` deploy function (or its dry-invocation) against the same fixture; assert the identical line appears in its captured/printed output | +| Surface 4, criterion 1 (dry-run names destination path) | `stapler-scripts/llm-sync/` (new) | `test_dry_run_names_destination_path_pi_dir_or_real_path` | scripted CLI-output check | Run with `--pi-dir /tmp/pi-staging --dry-run`; assert stdout contains that exact path; run without `--pi-dir`; assert it names the real `~/.pi/agent`-derived path | +| Surface 4, criterion 2 (enumerates each changed key individually) | same | `test_dry_run_enumerates_each_changed_key_individually` | scripted CLI-output check | Fixture with 2+ changed keys; assert each dotted key path (e.g. `packages.gotgenes-pi-permission-system`) appears on its own, not just a count | +| Surface 4, criterion 3 (no credential-shaped value in dry-run output) | same | `test_dry_run_output_contains_no_credential_shaped_value` | scripted CLI-output check | Fixture containing an `apikey`/`token`/`auth`-named key (which `PiConfigSource._reject_credential_material` should already have errored on before any print); assert the run fails before any dry-run print, and — as a second check — grep whatever partial output does exist for those substrings and assert no match | +| Surface 4, criterion 4 (external mode states skip reason explicitly) | `bootstrap-pyinfra/` (new) | `test_external_install_mode_dry_run_states_skip_reason_explicitly` | scripted CLI-output check | `pyinfra -y inventory.py main.py --data pi_install_mode=external --dry`; assert output contains "Would skip install" and "externally managed" | +| Surface 4, criterion 5 (closing line states no changes + destination) | `stapler-scripts/llm-sync/` (new) | `test_dry_run_closing_line_states_no_changes_made_and_destination` | scripted CLI-output check | Assert the last stdout line matches `No changes made to (dry run).` | +| Surface 5, criterion 1 (rejects approved entry missing approved_by, names entry+field) | `test_extension_manifest.py` | `test_manifest_load_rejects_approved_entry_missing_approved_by_or_date_naming_entry_and_fields` | scripted CLI-output check (loader raises, message inspected) | Assert `ManifestError` message contains the entry id and the literal field names `approved_by`/`approved_date` | +| Surface 5, criterion 2 (notes must record specific findings, not generic sign-off) | n/a — process control, not mechanically checkable | `manual_review_notes_quality_spot_check` | manual | Tyler (or a reviewing agent) reads the `notes` field during the Epic 1.4 checklist and confirms it names the specific fail-closed/deny-default/no-inheritance findings per extension, per ux.md's explicit "process failure this doc flags but cannot mechanically block" | +| Surface 5, criterion 3 (no script writes literal `"approved"`) | `test_fork_pin_extension.py` | `test_no_script_in_repo_writes_literal_approved_disposition_string` (same as C-1's static-inspection test, Task 1.3.1c) | scripted static-inspection check | Already covered under C-1 — cited here for UX traceability, not duplicated | +| Surface 5, criterion 4 (approval diff touches only approval fields) | manual git workflow | `test_manifest_approval_diff_touches_only_approval_fields` | manual (`git diff` inspection) | Tyler runs `git diff .config/pi/extensions-manifest.json` after a real approval edit and confirms only `reviewer`/`review_date`/`notes`/`approved_by`/`approved_date`/`disposition` changed | +| Surface 5, criterion 5 (non-approved candidate has a documented next disposition) | `.claude/skills/pi-extension-review/SKILL.md` | `test_skill_documents_hold_or_rejected_disposition_for_failed_review` | Doc (content-check) | Assert the skill file's checklist names `hold`/`rejected` as the outcome for a candidate that fails review | + +## Test Stack + +- **Unit / Integration (Python)**: this repo does **not** use pytest for `stapler-scripts/llm-sync` or `bootstrap-pyinfra` — deviating from the validation template's default. Each `test_*.py` file is a plain module of `test_*` functions, self-discovered by its own `if __name__ == "__main__": ...` block (see `stapler-scripts/llm-sync/test_pi_config.py:32-40` and `bootstrap-pyinfra/test_pi_install.py`'s identical tail) and run individually via `uv run .py`. New test files (`test_extension_manifest.py`, `test_review_gate.py`, `test_fork_pin_extension.py`, `test_pi_package_ledger.py`) must follow this exact pattern to be picked up by `make llm-sync-test` / `make pyinfra-test`. +- **Integration (subprocess-heavy)**: same files, using `tempfile.TemporaryDirectory()`/`tmp_path`-style fixtures and `subprocess`-level invocation of `main.py`/`pyinfra`, with `gh`/`pi` calls mocked via monkeypatched functions (no real network/GitHub calls in CI) except where explicitly marked `manual`. +- **E2E / UX**: scripted CLI-output-assertion tests (subprocess + stdout/stderr grep, per the table above) for everything reproducible without a live GitHub/1Password/Pi session; `manual` checklist items for the rest, matching plan.md's own explicit non-CI carve-outs (Story 4.2.1's redaction dry-run, Story 3.2.1's human approval step). + +## Coverage Targets and How to Measure + +| Stack | Coverage command | Target | +|---|---|---| +| `stapler-scripts/llm-sync` | `make llm-sync-test` (runs every `test_*.py` via `uv run`); for line coverage, `cd stapler-scripts/llm-sync && for t in test_*.py; do coverage run --parallel-mode "$t"; done && coverage combine && coverage report -m` (system `coverage` binary at `/usr/bin/coverage`; not currently wired into the Makefile — add if line-coverage tracking is wanted) | ≥80% line, all new `src/sources/extension_manifest.py`, `src/sources/review_gate.py`, `src/targets/pi_package_ledger.py`, `scripts/fork_pin_extension.py` | +| `bootstrap-pyinfra` | `make pyinfra-test` | ≥80% line on `deploys/pi.py`, `deploys/llm_sync.py` | +| CI wiring | Task 1.2.1g adds `make llm-sync-test` to `.github/workflows/ci.yml`'s `test` job and adds `stapler-scripts/llm-sync/**`, `.config/pi/**` to its `paths` trigger — VERIFIED gap: no `llm-sync` match currently exists in that workflow (plan.md's own citation) | CI fails on any `verify_pinned_sources_reviewed()` or manifest-loader regression, not just local `make ready` | + +- All public methods on `ExtensionManifestSource`, `PiPackageLedger`, and `fork_pin_extension.py`'s CLI surface: happy path + error paths covered per the table above. +- All external integrations (`gh` CLI via `fork_pin_extension.py`, Pi package lifecycle via `PiPackageLedger`, 1Password via the forked `pi-1password` extension): unit-mocked plus at least one integration test, except the redaction dry-run and the live `gh repo fork`/human-approval steps, which are explicitly `manual` per plan.md. +- UX acceptance criteria: all 25 in `design/ux.md` have a corresponding test or manual step in the table above. + +## Migration Note + +`migration_should_be_reversible` in the schema-migration sense (up/down scripts against a data store) does not apply to this project. Per Step 5 of this validation task and plan.md's own "Migration Plan" section, the only migration here is a **manual, machine-specific procedure**: backing up and classifying the current macOS machine's unmanaged `~/.pi/agent/settings.json` (Stories 5.2.1 and 5.2.2) before the first managed sync can overwrite any `managedKeys`-tracked key. There is no automated up/down pair to test. + +The closest equivalent — and the row this validation plan substitutes for a schema-migration reversibility test — is Story 5.2.1's rollback verification: + +| Requirement | Test File | Test Name | Type | Scenario | +|---|---|---|---|---| +| Migration/Rollback (Story 5.2.1) | `project_plans/pi-dotfiles/implementation/rollout-runbook.md` (manual procedure) + `stapler-scripts/llm-sync/` (new automated check) | `test_rollback_restores_only_managed_keys_and_preserves_auth_and_sessions` | Migration | *Given* a backup of `~/.pi/agent/settings.json` taken before adoption and the `managedKeys` state at that time (simulated in a tmp_path fixture with dummy `auth.json`/session files alongside), *when* the documented rollback procedure restores from backup, *then* only keys present in `managedKeys` at backup time are reverted, and `auth.json`/session files are byte-identical before and after — proving rollback never touches credentials, sessions, or trust state, per requirements' Risk Control closing sentence. | + +## Summary + +- **Requirement-mapped tests**: 9 Success Metrics x 3 (happy/error/integration, with SM-7 and SM-9 adapted to doc/content-check form since they have no runtime code path) + 5 Constraints x 2-4 rows each + Package Ledger's 4 dedicated rows ≈ 51 test rows, of which 15 cite existing tests verbatim and the remainder are net-new, reconciled against plan.md's own Task-level GWT lists (1.1.1d, 1.1.2b, 1.2.1c/e/f, 1.2.2b, 1.3.1c/d, 2.2.1d/f, 3.4.1d, 4.1.1e). +- **Test type breakdown**: Unit ≈ 27, Integration ≈ 19, Manual/Doc ≈ 5. +- **Requirements coverage**: 9/9 Success Metrics mapped (100%); 5/5 testable Constraints mapped (100%). Two open items in plan.md's "Unresolved Questions" (Pi's package-artifact path/prune behavior; which existing settings keys are universal/work/machine-generated) are explicitly blocking preconditions on Story 2.2.1 and Story 5.2.2 respectively — those tests cannot be finalized until Tyler resolves them, and this is stated as a gap, not smoothed over. +- **UX acceptance tests**: 25/25 criteria in `design/ux.md` covered (5 surfaces x 5 criteria each); 21 are CI-runnable scripted CLI-output checks, 4 are `manual` (Surface 5 criteria 2 and 4, the live-`gh`-mocked-but-still-human-reviewed edges, and the redaction dry-run cited under SM-8). +- **Migration test note**: schema-migration-style reversibility does not apply; substituted with a Migration-typed row testing Story 5.2.1's rollback-preserves-credentials/sessions/trust guarantee instead, as directed by Step 5. diff --git a/stapler-scripts/llm-sync/AGENTS.md b/stapler-scripts/llm-sync/AGENTS.md index 6bb74fed..e616e7fe 100644 --- a/stapler-scripts/llm-sync/AGENTS.md +++ b/stapler-scripts/llm-sync/AGENTS.md @@ -59,6 +59,12 @@ uv run main.py --help - **No agents/sub-agents, no native MCP:** Pi's philosophy deliberately omits both (see its README) — build them via extensions if needed. `PiTarget` implements skill/prompt sync; tiered settings provision extension/package equivalents for capabilities without native support. +- **Fork-pin review gate:** a `packages`/`extensions` source pinned to a `github.com/tstapler/*` fork + commit must have a matching `approved` entry, at that exact commit, in `.config/pi/extensions-manifest.json`. + `verify_pinned_sources_reviewed()` (`stapler-scripts/llm-sync/src/sources/review_gate.py`) enforces this + during sync and raises `PiConfigError` if the entry is missing, at the wrong commit, or not yet approved; + trusted-scope npm sources, `@tstapler`-scoped packages, and local paths are exempt by construction. See + `.claude/skills/pi-extension-review/SKILL.md` for the review process that produces an approved entry. ### Antigravity - **Customizations Root:** `~/.gemini/config` (global) or `.agents` (workspace) diff --git a/stapler-scripts/llm-sync/scripts/fork_pin_extension.py b/stapler-scripts/llm-sync/scripts/fork_pin_extension.py new file mode 100644 index 00000000..d901dff5 --- /dev/null +++ b/stapler-scripts/llm-sync/scripts/fork_pin_extension.py @@ -0,0 +1,302 @@ +#!/usr/bin/env -S uv run +# /// script +# requires-python = ">=3.11" +# dependencies = [ +# "typer>=0.12", +# ] +# /// +"""Fork an upstream Pi extension repo and scaffold a review-manifest entry. + +Only ever writes `disposition: "candidate"` with `approved_by`/`approved_date` +left null. The terminal review state is reserved for a human hand-edit of +`.config/pi/extensions-manifest.json` — see +`project_plans/pi-dotfiles/extension-audit.md`'s review gate. This script has +no argument, flag, or code path that reaches that value; see +`test_fork_pin_extension_argparser_has_no_flag_that_writes_approved_disposition` +in test_fork_pin_extension.py, which scans this file's own source for the +literal string this docstring is carefully avoiding. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import typer + +_SCRIPT_DIR = Path(__file__).resolve().parent +_LLM_SYNC_DIR = _SCRIPT_DIR.parent +_REPO_ROOT = _LLM_SYNC_DIR.parent.parent +sys.path.append(str(_LLM_SYNC_DIR / "src")) + +from sources.extension_manifest import ( # noqa: E402 (path setup must precede this) + ExtensionManifestSource, + ManifestEntry, +) + +app = typer.Typer(add_completion=False, no_args_is_help=True) + + +@app.callback() +def _callback() -> None: + """Fork-and-pin scaffolding for Pi extension review manifests. + + An empty callback, kept only so Typer always requires an explicit + subcommand name (`fork ...`) instead of collapsing a single-command app + into a bare `fork_pin_extension.py ` invocation -- matches + plan.md's specified `fork_pin_extension.py fork ...` usage + and leaves room for future subcommands without changing this one's shape. + """ + +# The only namespace this script forks into. Epic 1.3's plan.md left +# org-vs-personal as an open decision (see plan.md's "Unresolved Questions"), +# but the plan's own acceptance criteria fix the fork target at +# "github.com/tstapler/", so that's what's implemented here. +FORK_OWNER = "tstapler" + +DEFAULT_MANIFEST_FILE = _REPO_ROOT / ".config" / "pi" / "extensions-manifest.json" + +# Dry-run never calls `gh`, so it cannot know the real commit yet. This +# sentinel stands in for it in the printed preview. +PENDING_COMMIT = "" + +# License isn't fetched by this script (not in scope per Task 1.3.1a/b); the +# human reviewer fills this in for real during review, before hand-editing +# disposition. +PLACEHOLDER_LICENSE = "UNKNOWN (confirm license during human review)" + + +class GhCommandError(ValueError): + """A `gh` subprocess invocation failed.""" + + +def run_gh_fork(upstream: str) -> None: + """Fork `upstream` (owner/repo) into `github.com/tstapler/` via `gh repo fork`. + + A thin, mockable wrapper — tests monkeypatch this instead of shelling + out to a real `gh repo fork`. + """ + result = subprocess.run( + ["gh", "repo", "fork", upstream, "--default-branch-only"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + if result.returncode != 0: + raise GhCommandError( + f"'gh repo fork {upstream}' failed: {result.stderr.strip() or 'unknown gh error'}" + ) + + +def run_gh_head_commit(fork_repo_slug: str) -> str: + """Return the current HEAD commit SHA of `github.com/tstapler/`. + + A thin, mockable wrapper around `gh api` — see `run_gh_fork`. + """ + endpoint = f"repos/{FORK_OWNER}/{fork_repo_slug}/commits/HEAD" + result = subprocess.run( + ["gh", "api", endpoint, "--jq", ".sha"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + if result.returncode != 0: + raise GhCommandError( + f"'gh api {endpoint}' failed: {result.stderr.strip() or 'unknown gh error'}" + ) + sha = result.stdout.strip() + if not sha: + raise GhCommandError(f"'gh api {endpoint}' returned an empty commit SHA") + return sha + + +def _repo_slug(upstream: str) -> str: + return upstream.rstrip("/").rsplit("/", 1)[-1] + + +def build_entry_dict(*, entry_id: str, capability: str, upstream: str, commit: str) -> dict: + """Build the manifest-entry dict. The one function both code paths call. + + `commit` is the only value that comes from `gh`; everything else is + derived from CLI arguments. The real run passes the SHA `gh` returned; + the dry-run preview passes `PENDING_COMMIT`. Routing both paths through + this single function is what makes + `test_fork_pin_extension_dry_run_matches_real_run_manifest_diff` a real + regression guard rather than two independently-hand-written dicts that + could silently drift apart. + """ + repo_slug = _repo_slug(upstream) + return { + "id": entry_id, + "capability": capability, + "upstream_repo": f"https://github.com/{upstream}", + "upstream_commit": commit, + "license": PLACEHOLDER_LICENSE, + "fork_repo": f"https://github.com/{FORK_OWNER}/{repo_slug}", + "fork_commit": commit, + "package_paths": None, + "disposition": "candidate", + "reviewer": None, + "review_date": None, + "notes": None, + "approved_by": None, + "approved_date": None, + } + + +def _load_raw_manifest(path: Path) -> dict: + if not path.exists(): + return {"extensions": {}} + data = json.loads(path.read_text(encoding="utf-8")) + if not isinstance(data, dict) or not isinstance(data.get("extensions"), dict): + typer.echo( + f"Error: manifest file '{path}' must be a JSON object with an 'extensions' map", + err=True, + ) + raise typer.Exit(1) + return data + + +def _print_entry(entry: dict, *, heading: str) -> None: + typer.echo(heading) + typer.echo(json.dumps(entry, indent=2, sort_keys=True)) + + +def _reject_if_id_exists(raw_manifest: dict, entry_id: str) -> None: + existing = raw_manifest["extensions"].get(entry_id) + if existing is None: + return + current_disposition = existing.get("disposition", "") + typer.echo( + f"Error: manifest entry '{entry_id}' already exists with disposition " + f"'{current_disposition}'. Pick a different --id, or if you mean to " + f"re-review it, edit the existing entry by hand instead of " + f"re-running fork.", + err=True, + ) + raise typer.Exit(1) + + +def _show_dry_run_plan( + *, entry_id: str, capability: str, upstream: str, fork_target: str, manifest_file: Path +) -> None: + """Print the fork + manifest plan. Makes zero `gh`/subprocess calls.""" + planned = build_entry_dict( + entry_id=entry_id, capability=capability, upstream=upstream, commit=PENDING_COMMIT + ) + typer.echo(f"Would fork: {upstream} -> {fork_target}") + typer.echo(f"Would write to: {manifest_file}") + _print_entry(planned, heading="Planned manifest entry:") + typer.echo("Re-run without --dry-run to create the fork and write the manifest entry.") + + +def _validate_entry(entry_dict: dict) -> None: + """Reuse `ManifestEntry`'s own constructor for its invariant checks. + + Defense in depth alongside the round-trip load in `_execute_fork`, even + though disposition is always "candidate" here. `entry_dict`'s keys match + `ManifestEntry`'s fields one-for-one (both originate from + `build_entry_dict`), so `**entry_dict` is a safe drop-in for the + field-by-field constructor call this replaced. + """ + ManifestEntry(**entry_dict) + + +def _execute_fork( + *, + entry_id: str, + capability: str, + upstream: str, + fork_target: str, + fork_repo_slug: str, + manifest_file: Path, + raw_manifest: dict, +) -> None: + """Create the fork via `gh`, then write and validate the manifest entry.""" + try: + run_gh_fork(upstream) + commit = run_gh_head_commit(fork_repo_slug) + except GhCommandError as error: + typer.echo(f"Error: {error}", err=True) + raise typer.Exit(1) + + entry_dict = build_entry_dict( + entry_id=entry_id, capability=capability, upstream=upstream, commit=commit + ) + _validate_entry(entry_dict) + + raw_manifest["extensions"][entry_id] = entry_dict + manifest_file.parent.mkdir(parents=True, exist_ok=True) + manifest_file.write_text( + json.dumps(raw_manifest, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + + # Round-trip through the real loader to prove the file just written + # parses as a valid manifest (Task 1.3.1b: "reuses ExtensionManifestSource + # for validation"). + ExtensionManifestSource.load(manifest_file) + + typer.echo(f"Forked {upstream} -> {fork_target}") + typer.echo(f"Wrote manifest entry to: {manifest_file}") + _print_entry(entry_dict, heading="Manifest entry:") + + +@app.command() +def fork( + upstream: str = typer.Argument( + ..., help="Upstream repo as owner/repo, e.g. gotgenes/pi-packages" + ), + entry_id: str = typer.Option( + ..., "--id", help="Stable manifest entry id, e.g. gotgenes-pi-packages" + ), + capability: str = typer.Option( + ..., "--capability", help="Comma-separated capability tag(s) for the entry" + ), + dry_run: bool = typer.Option( + False, "--dry-run", help="Print the plan only: no gh calls, no manifest write" + ), + manifest_file: Path = typer.Option( + DEFAULT_MANIFEST_FILE, + "--manifest-file", + help="Path to the extension review manifest JSON", + ), +) -> None: + """Fork UPSTREAM and scaffold a candidate manifest entry for it. + + Always writes disposition "candidate" with approved_by/approved_date set + to null; nothing here ever sets the terminal review state, which is a + human hand-edit of the manifest file, not a scriptable action. + """ + raw_manifest = _load_raw_manifest(manifest_file) + _reject_if_id_exists(raw_manifest, entry_id) + + fork_repo_slug = _repo_slug(upstream) + fork_target = f"github.com/{FORK_OWNER}/{fork_repo_slug}" + + if dry_run: + _show_dry_run_plan( + entry_id=entry_id, + capability=capability, + upstream=upstream, + fork_target=fork_target, + manifest_file=manifest_file, + ) + return + + _execute_fork( + entry_id=entry_id, + capability=capability, + upstream=upstream, + fork_target=fork_target, + fork_repo_slug=fork_repo_slug, + manifest_file=manifest_file, + raw_manifest=raw_manifest, + ) + + +if __name__ == "__main__": + app() diff --git a/stapler-scripts/llm-sync/src/cli.py b/stapler-scripts/llm-sync/src/cli.py index 41904fbc..2ae59793 100644 --- a/stapler-scripts/llm-sync/src/cli.py +++ b/stapler-scripts/llm-sync/src/cli.py @@ -22,14 +22,17 @@ class ChangeDetection(Enum): # Allow running from src directly or as module try: from .sources.claude import ClaudeSource + from .sources.extension_manifest import ExtensionManifestSource from .sources.mcp_config import McpConfigSource from .sources.pi_config import PiConfigSource from .sources.plugins import PluginSource, PluginSourceConfig + from .sources.review_gate import verify_pinned_sources_reviewed from .sources.tiered_config import TieredJsonConfig from .targets.gemini import GeminiTarget, AntigravityTarget from .targets.opencode import OpenCodeTarget from .targets.pi import PiTarget from .targets.pi_settings import PiSettingsTarget + from .targets.pi_package_ledger import PiPackageLedger from .targets.claude_settings import ClaudeSettingsTarget from .targets.claude_plugin_installer import ClaudePluginInstaller from .targets.antigravity_plugin_installer import AntigravityPluginInstaller @@ -40,14 +43,17 @@ class ChangeDetection(Enum): # Fallback if run as script (hacky but useful during dev) sys.path.append(str(Path(__file__).parent)) from sources.claude import ClaudeSource + from sources.extension_manifest import ExtensionManifestSource from sources.mcp_config import McpConfigSource from sources.pi_config import PiConfigSource from sources.plugins import PluginSource, PluginSourceConfig + from sources.review_gate import verify_pinned_sources_reviewed from sources.tiered_config import TieredJsonConfig from targets.gemini import GeminiTarget, AntigravityTarget from targets.opencode import OpenCodeTarget from targets.pi import PiTarget from targets.pi_settings import PiSettingsTarget + from targets.pi_package_ledger import PiPackageLedger from targets.claude_settings import ClaudeSettingsTarget from targets.claude_plugin_installer import ClaudePluginInstaller from targets.antigravity_plugin_installer import AntigravityPluginInstaller @@ -277,6 +283,31 @@ def sync_mcp(mcp_source: McpConfigSource, settings_target: ClaudeSettingsTarget, if mode is not SyncMode.PREVIEW: console.print(f"[green]Wrote {count} MCP servers to {settings_target.settings_file}[/green]") +def _enabled_package_sources(source: PiConfigSource) -> Dict[str, str]: + """Recover the stable `{entry_id: source}` mapping the rendered array drops. + + `PiConfigSource.load()` renders `packages` into Pi's native flat array of + source strings, which discards the stable registry ids `PiPackageLedger` + keys its state on (see `_render_registry`/`_render_package` in + `sources/pi_config.py`). Re-loading the raw tiered value here recovers + them. Safe to call only after a successful `source.load()`: that call + has already strictly validated the raw registry (malformed entries, + disallowed sources, credential material), so this only needs to repeat + the "is this entry enabled" check, not the full validation. + """ + raw_packages = source.config.load().value.get("packages", {}) + if not isinstance(raw_packages, dict): + return {} + enabled: Dict[str, str] = {} + for entry_id, entry in raw_packages.items(): + if not isinstance(entry, dict) or not entry.get("enabled", True): + continue + candidate_source = entry.get("source") + if isinstance(candidate_source, str) and candidate_source.strip(): + enabled[entry_id] = candidate_source + return enabled + + def sync_pi_settings(args) -> None: config_root = Path.home() / ".config" / "pi" agent_dir = args.pi_dir or Path.home() / ".pi" / "agent" @@ -298,19 +329,57 @@ def sync_pi_settings(args) -> None: for layer in loaded.layers: console.print(f"[dim]Loaded Pi configuration layer {layer}[/dim]") + manifest_path = args.pi_extensions_manifest or config_root / "extensions-manifest.json" + manifest = ExtensionManifestSource.load(manifest_path) + verify_pinned_sources_reviewed(loaded, manifest) + target = PiSettingsTarget( settings_path=args.pi_settings_file or agent_dir / "settings.json", state_path=args.pi_settings_state_file or Path.home() / ".config" / "llm-sync" / "pi-settings-state.json", ) changed = target.save(loaded, dry_run=args.dry_run) - if args.dry_run and changed: - console.print("[blue]Would update managed Pi settings[/blue]") + if args.dry_run: + if changed: + console.print(f"[blue]Would update managed Pi settings at {agent_dir}[/blue]") + console.print(f"No changes made to {agent_dir} (dry run).") elif changed: console.print("[green]Updated managed Pi settings[/green]") else: console.print("[dim]Managed Pi settings already converged.[/dim]") + enabled_packages = _enabled_package_sources(source) + ledger = PiPackageLedger( + state_path=args.pi_package_ledger_state_file + or Path.home() / ".config" / "llm-sync" / "pi-package-state.json", + agent_dir=agent_dir, + ) + + if args.reconcile_pi_package_ledger: + recovered = ledger.reconcile(enabled_packages, dry_run=args.dry_run) + for entry_id in recovered: + console.print(f"[green]Reconciled Pi package ledger entry: {entry_id}[/green]") + + # Only meaningful when settings actually changed this run -- an + # unchanged run's ledger already reflects the current enabled set, + # unless a prior run crashed between the settings write and the ledger + # write, which is exactly what --reconcile-pi-package-ledger recovers. + if changed: + ledger.save(enabled_packages, dry_run=args.dry_run) + + stale = ledger.find_stale(set(enabled_packages)) + for entry_id in stale: + print( + f"stale Pi package: {entry_id} " + "(not pruned; run with --prune-stale-pi-packages)" + ) + + if args.prune_stale_pi_packages and stale: + console.print("[dim]Pruning stale Pi packages...[/dim]") + pruned = ledger.prune(stale, dry_run=args.dry_run) + for path in pruned: + print(f"Pruned: {path}") + def should_sync_non_pi_integrations(target: str, plugins_only: bool) -> bool: """Keep a Pi-only run from mutating Claude and Antigravity state.""" @@ -339,6 +408,12 @@ def main(): parser.add_argument("--pi-local-config-dir", type=Path, help="Override machine-local Pi config.d directory") parser.add_argument("--pi-settings-file", type=Path, help="Override generated Pi settings.json path") parser.add_argument("--pi-settings-state-file", type=Path, help="Override Pi managed-key state path") + parser.add_argument("--pi-extensions-manifest", type=Path, help="Override Pi extension review manifest path") + parser.add_argument("--pi-package-ledger-state-file", type=Path, help="Override Pi package ownership ledger state path") + parser.add_argument("--prune-stale-pi-packages", action="store_true", + help="Delete on-disk artifacts for Pi packages no longer enabled in config (stale entries are always reported; this flag makes pruning actually delete them)") + parser.add_argument("--reconcile-pi-package-ledger", action="store_true", + help="Rebuild Pi package ledger entries missing from a prior interrupted sync, from the current config") parser.add_argument("--mcp-global-config", type=Path, help="Override global MCP servers JSON file") parser.add_argument("--mcp-local-config", type=Path, help="Override machine-local MCP servers JSON file") parser.add_argument("--mcp-global-config-dir", type=Path, help="Override global MCP servers config.d directory") diff --git a/stapler-scripts/llm-sync/src/sources/extension_manifest.py b/stapler-scripts/llm-sync/src/sources/extension_manifest.py new file mode 100644 index 00000000..ec4640c4 --- /dev/null +++ b/stapler-scripts/llm-sync/src/sources/extension_manifest.py @@ -0,0 +1,219 @@ +"""Load and validate the tracked Pi extension review manifest.""" + +import json +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +_REQUIRED_FIELDS = ( + "id", + "capability", + "upstream_repo", + "upstream_commit", + "license", + "fork_repo", + "fork_commit", + "disposition", +) + +# The gate checklist this evidence check is a mechanical proxy for; used only +# to phrase "which sub-area(s) look uncovered" when too few paths are cited. +_GATE_SUBAREAS = ("network", "subprocess", "filesystem", "secrets", "telemetry") +_MIN_EVIDENCE_PATHS = len(_GATE_SUBAREAS) + +# Extraction rule: a run of `/`-joined path segments (word chars, '.', '-') +# ending in a dot-extension, e.g. `src/foo/bar.py` or bare src/foo/bar.py. +# Optional surrounding backticks are matched and discarded. This is a +# syntactic heuristic over free text, not a filesystem check. +_CANDIDATE_PATH_RE = re.compile(r"`?((?:[\w.\-]+/)+[\w.\-]+\.[A-Za-z0-9]+)`?") + + +class ManifestError(ValueError): + """The extension manifest is invalid or contains an illegal review state.""" + + +@dataclass(frozen=True) +class ManifestEntry: + """One review record for a forked/reviewed Pi extension. + + Enforces the Approval Gate invariant at construction time (not only at + load time) so no caller — including a future fork-helper script — can + build an `approved` entry without an approver on record. + """ + + id: str + capability: str + upstream_repo: str + upstream_commit: str + license: str + fork_repo: str + fork_commit: str + package_paths: tuple[str, ...] | None + disposition: str + reviewer: str | None + review_date: str | None + notes: str | None + approved_by: str | None + approved_date: str | None + + def __post_init__(self) -> None: + if self.disposition == "approved" and not (self.approved_by and self.approved_date): + raise ManifestError( + f"Manifest entry '{self.id}' has disposition 'approved' but is " + "missing required field(s) 'approved_by' and 'approved_date' " + "(both are required for an approved entry)" + ) + + +@dataclass(frozen=True) +class ExtensionManifest: + """The full set of manifest entries, keyed by stable extension id.""" + + entries: dict[str, ManifestEntry] + + +class ExtensionManifestSource: + """Parse `.config/pi/extensions-manifest.json` into validated `ManifestEntry` objects.""" + + @staticmethod + def load(path: Path) -> ExtensionManifest: + if not path.exists(): + return ExtensionManifest(entries={}) + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as error: + raise ManifestError(f"Cannot read manifest file '{path}': {error}") from error + if not isinstance(raw, dict) or not isinstance(raw.get("extensions"), dict): + raise ManifestError( + f"Manifest file '{path}' must be a JSON object with an 'extensions' map" + ) + + entries: dict[str, ManifestEntry] = {} + # Map keys are unique by construction (JSON object parsing), so the + # only reachable "duplicate id" is two different map keys whose + # entries' own `id` fields collide. + seen_ids: dict[str, str] = {} + for entry_id, data in raw["extensions"].items(): + entry = ExtensionManifestSource._parse_entry(entry_id, data) + if entry.id in seen_ids: + raise ManifestError( + f"Manifest entries '{seen_ids[entry.id]}' and '{entry_id}' " + f"both resolve to duplicate id '{entry.id}'" + ) + seen_ids[entry.id] = entry_id + entries[entry_id] = entry + return ExtensionManifest(entries=entries) + + @staticmethod + def _parse_entry(entry_id: str, data: Any) -> ManifestEntry: + if not isinstance(data, dict): + raise ManifestError(f"Manifest entry '{entry_id}' must be a JSON object") + + missing = [field for field in _REQUIRED_FIELDS if not data.get(field)] + if missing: + raise ManifestError( + f"Manifest entry '{entry_id}' is missing required field(s): " + + ", ".join(missing) + ) + + package_paths = data.get("package_paths") + entry = ManifestEntry( + id=data["id"], + capability=data["capability"], + upstream_repo=data["upstream_repo"], + upstream_commit=data["upstream_commit"], + license=data["license"], + fork_repo=data["fork_repo"], + fork_commit=data["fork_commit"], + package_paths=tuple(package_paths) if package_paths else None, + disposition=data["disposition"], + reviewer=data.get("reviewer"), + review_date=data.get("review_date"), + notes=data.get("notes"), + approved_by=data.get("approved_by"), + approved_date=data.get("approved_date"), + ) + + # Requires a `gh api` call (external I/O keyed on fork_repo/fork_commit), + # so it runs here at load time rather than in ManifestEntry.__post_init__. + if entry.disposition == "approved": + ExtensionManifestSource._verify_notes_evidence(entry) + + return entry + + @staticmethod + def _verify_notes_evidence(entry: "ManifestEntry") -> None: + """Mechanically confirm an approved entry's `notes` cite real evidence. + + Requires at least `_MIN_EVIDENCE_PATHS` distinct file paths in `notes` + (see `_CANDIDATE_PATH_RE` for the extraction rule), each confirmed to + exist in the fork tree at `fork_commit` via `gh api + repos/tstapler//git/trees/?recursive=1`. + + This only proves the cited paths exist in the fork at that commit — + it cannot and does not judge whether they were meaningfully reviewed. + """ + candidates = ExtensionManifestSource._extract_candidate_paths(entry.notes or "") + if len(candidates) < _MIN_EVIDENCE_PATHS: + raise ManifestError( + f"Manifest entry '{entry.id}' has disposition 'approved' but " + f"'notes' cites only {len(candidates)} distinct file path(s); " + f"at least {_MIN_EVIDENCE_PATHS} are required (roughly one per " + f"review sub-area: {', '.join(_GATE_SUBAREAS)}). This check " + "only confirms cited paths exist — it does not judge whether " + "they were meaningfully reviewed, or which sub-area(s) they " + "actually cover." + ) + + tree_paths = ExtensionManifestSource._fetch_fork_tree_paths( + entry.fork_repo, entry.fork_commit + ) + missing_paths = [path for path in candidates if path not in tree_paths] + if missing_paths: + raise ManifestError( + f"Manifest entry '{entry.id}' notes cite path(s) not found in " + f"fork '{entry.fork_repo}' at commit '{entry.fork_commit}': " + + ", ".join(missing_paths) + ) + + @staticmethod + def _extract_candidate_paths(notes: str) -> list[str]: + """Pull distinct candidate file paths out of free-text `notes`. + + See `_CANDIDATE_PATH_RE` for the extraction rule. Returns paths in + first-seen order with duplicates removed. + """ + seen: dict[str, None] = {} + for match in _CANDIDATE_PATH_RE.finditer(notes): + seen.setdefault(match.group(1), None) + return list(seen.keys()) + + @staticmethod + def _fetch_fork_tree_paths(fork_repo: str, fork_commit: str) -> set[str]: + repo_slug = fork_repo.rstrip("/").rsplit("/", 1)[-1] + if repo_slug.endswith(".git"): + repo_slug = repo_slug[: -len(".git")] + endpoint = f"repos/tstapler/{repo_slug}/git/trees/{fork_commit}?recursive=1" + + result = subprocess.run( + ["gh", "api", endpoint], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + if result.returncode != 0: + raise ManifestError( + f"failed to fetch fork tree via 'gh api {endpoint}': " + f"{result.stderr.strip() or 'unknown gh error'}" + ) + + try: + payload = json.loads(result.stdout) + except json.JSONDecodeError as error: + raise ManifestError( + f"'gh api {endpoint}' returned malformed JSON: {error}" + ) from error + return {item["path"] for item in payload.get("tree", []) if "path" in item} diff --git a/stapler-scripts/llm-sync/src/sources/pi_config.py b/stapler-scripts/llm-sync/src/sources/pi_config.py index e1a7bc97..929ca982 100644 --- a/stapler-scripts/llm-sync/src/sources/pi_config.py +++ b/stapler-scripts/llm-sync/src/sources/pi_config.py @@ -37,6 +37,58 @@ class LoadedPiConfig: settings: dict[str, Any] managed_keys: set[str] layers: tuple[Path, ...] + trusted_scopes: tuple[str, ...] = () + + +@dataclass(frozen=True) +class SourceRef: + """A parsed `tstapler`-owned fork source: its repo and pinned commit.""" + + repo: str + commit: str + + +_FORK_SCHEME_PREFIXES = ("git:", "git@", "https://", "http://", "ssh://", "git://") + + +def normalize_fork_repo(repo: str) -> str: + """Canonicalize a fork repo URL/slug to `github.com/tstapler/` form. + + Lowercases, strips a leading scheme, converts the `git@host:owner/repo` + colon separator to a slash, and drops a trailing `.git`/`/`. + """ + normalized = repo.lower().removeprefix("git:") + for prefix in ("git://", "https://", "http://", "ssh://", "git@"): + normalized = normalized.removeprefix(prefix) + normalized = normalized.replace("github.com:tstapler/", "github.com/tstapler/") + normalized = normalized.rstrip("/") + if normalized.endswith(".git"): + normalized = normalized[: -len(".git")] + return normalized + + +def parse_fork_source(source: str) -> SourceRef | None: + """Parse a package source string as a `tstapler`-owned fork reference. + + Returns `None` when `source` isn't shaped like one at all: wrong scheme, + no trailing `@`, or not a `github.com/tstapler/` (or SSH + `github.com:tstapler/`) URL once normalized. Otherwise returns the + `(repo, commit)` pair with `repo` normalized via `normalize_fork_repo` + and `commit` exactly as given — commit case-folding, if needed, is the + caller's job (see `review_gate.py`), not this parser's. + + Does not validate that `commit` looks like an immutable SHA; that check + stays in `_is_allowed_package_source`, which layers it on top of this. + """ + if not source.startswith(_FORK_SCHEME_PREFIXES): + return None + before_ref, separator, ref = source.rpartition("@") + if not separator: + return None + normalized = normalize_fork_repo(before_ref) + if "github.com/tstapler/" not in normalized: + return None + return SourceRef(repo=normalized, commit=ref) class PiConfigSource: @@ -84,6 +136,7 @@ def load(self) -> LoadedPiConfig: settings=settings, managed_keys=set(settings), layers=loaded.layers, + trusted_scopes=self._trusted_scopes, ) @staticmethod @@ -144,6 +197,12 @@ def _render_path(resource_key: str, entry_id: str, entry: dict[str, Any]) -> str raise PiConfigError( f"Pi {resource_key} entry '{entry_id}' requires a non-empty path" ) + if not path.startswith(("/", "./", "../", "~/")): + raise PiConfigError( + f"Pi {resource_key} entry '{entry_id}' path must be a local path " + "(starting with /, ./, ../, or ~/); remote extension sources " + "belong in 'packages' and must pass the fork-pin-review gate" + ) return path def _render_package(self, entry_id: str, entry: dict[str, Any]) -> Any: @@ -184,15 +243,9 @@ def _is_allowed_package_source(self, source: str) -> bool: package_spec = source.removeprefix("npm:") package_name, separator, version = package_spec.rpartition("@") return bool(separator and package_name and version and version != "latest") - if source.startswith( - ("git:", "git@", "https://", "http://", "ssh://", "git://") - ): - before_ref, separator, ref = source.rpartition("@") - normalized = before_ref.lower().removeprefix("git:") - owned_fork = ( - "github.com/tstapler/" in normalized - or "github.com:tstapler/" in normalized - ) - immutable_commit = bool(re.fullmatch(r"[0-9a-fA-F]{7,64}", ref)) - return bool(separator and owned_fork and immutable_commit) + if source.startswith(_FORK_SCHEME_PREFIXES): + ref = parse_fork_source(source) + if ref is None: + return False + return bool(re.fullmatch(r"[0-9a-fA-F]{7,64}", ref.commit)) return False diff --git a/stapler-scripts/llm-sync/src/sources/review_gate.py b/stapler-scripts/llm-sync/src/sources/review_gate.py new file mode 100644 index 00000000..26e80c27 --- /dev/null +++ b/stapler-scripts/llm-sync/src/sources/review_gate.py @@ -0,0 +1,70 @@ +"""Cross-reference rendered Pi config sources against the extension review manifest. + +Kept separate from `extension_manifest.py` (manifest-schema parsing) and +`pi_config.py` (config rendering): this module's only job is cross-referencing +one already-loaded aggregate (`LoadedPiConfig`) against another +(`ExtensionManifest`). +""" + +from .extension_manifest import ExtensionManifest, ManifestEntry +from .pi_config import LoadedPiConfig, PiConfigError, SourceRef, normalize_fork_repo, parse_fork_source + +_SCANNED_RESOURCE_KEYS = ("packages", "extensions") + + +def verify_pinned_sources_reviewed( + loaded: LoadedPiConfig, + manifest: ExtensionManifest, +) -> None: + """Raise `PiConfigError` if any rendered fork source lacks an approved + manifest entry at the exact pinned commit. + + Only sources `parse_fork_source()` recognizes as `github.com/tstapler/` + fork-shaped are checked. Everything else — trusted-scope sources, + `@tstapler` npm sources, local paths — is exempt by construction, since + `parse_fork_source()` returns `None` for all of them. + """ + for resource_key in _SCANNED_RESOURCE_KEYS: + for entry in loaded.settings.get(resource_key, []): + source = entry["source"] if isinstance(entry, dict) else entry + if not isinstance(source, str): + continue + ref = parse_fork_source(source) + if ref is None: + continue + _check_reviewed(source, ref, manifest) + + +def _check_reviewed(source: str, ref: SourceRef, manifest: ExtensionManifest) -> None: + matching = [ + candidate + for candidate in manifest.entries.values() + if _same_repo(candidate, ref.repo) + ] + if not matching: + raise PiConfigError( + f"Pi source '{source}' is pinned to an unreviewed fork " + f"({ref.repo}@{ref.commit}); add an approved manifest entry " + "before syncing" + ) + + at_commit = [m for m in matching if m.fork_commit.lower() == ref.commit.lower()] + if not at_commit: + approved_commits = ", ".join(sorted({m.fork_commit for m in matching})) + raise PiConfigError( + f"Pi source '{source}' is pinned at commit '{ref.commit}', but " + f"the manifest's approved commit(s) for {ref.repo} are: " + f"{approved_commits}" + ) + + not_approved = [m for m in at_commit if m.disposition != "approved"] + if not_approved: + dispositions = ", ".join(sorted({m.disposition for m in not_approved})) + raise PiConfigError( + f"Pi source '{source}' has a manifest entry at the exact commit " + f"but its disposition is '{dispositions}', not 'approved'" + ) + + +def _same_repo(entry: ManifestEntry, repo: str) -> bool: + return normalize_fork_repo(entry.fork_repo) == repo diff --git a/stapler-scripts/llm-sync/src/targets/pi_package_ledger.py b/stapler-scripts/llm-sync/src/targets/pi_package_ledger.py new file mode 100644 index 00000000..513a029b --- /dev/null +++ b/stapler-scripts/llm-sync/src/targets/pi_package_ledger.py @@ -0,0 +1,191 @@ +"""Ownership-safe ledger for on-disk Pi package artifacts. + +Sibling to `PiSettingsTarget`'s `managedKeys` pattern (same atomic-write +shape), but for the files `pi install` writes under the Pi agent directory +rather than `settings.json` keys. Grounded in `.config/pi/README.md`'s +"Package lifecycle" spike: Pi does not garbage-collect installed packages +when `settings.json` is hand-edited (as `PiSettingsTarget.save()` does) -- +only its own `pi remove`/`pi uninstall` CLI prunes artifacts. This ledger +lets `llm-sync` detect and, on request, clean up artifacts orphaned by a +config-only removal. +""" + +import json +import os +import shutil +import tempfile +from pathlib import Path + + +class PiPackageLedgerError(ValueError): + """Existing Pi package ledger state cannot be read safely.""" + + +class PiPackageLedger: + def __init__(self, state_path: Path, agent_dir: Path) -> None: + self.state_path = state_path + self.agent_dir = agent_dir + + def artifact_path_for_source(self, source: str) -> Path | None: + """Return the on-disk artifact path for `source`, or `None` if unverified. + + Only the npm source shape is grounded in the Epic 2.1 spike: an + `npm:`-prefixed `packages` entry lands under + `/npm/node_modules//` + (`.config/pi/README.md`'s "Package lifecycle" section). Git-fork + (`git:...@`) and local (`path:`/`./`/`~/`) source shapes have + no confirmed artifact-path pattern -- rather than fabricate one, + this returns `None`, which callers record as an explicit + "unknown, not yet verified" sentinel instead of a guessed path. + """ + if source.startswith("npm:"): + sandbox = self.agent_dir / "npm" / "node_modules" + candidate = Path(os.path.normpath(sandbox / _npm_package_name(source))) + if candidate != sandbox and sandbox not in candidate.parents: + raise PiPackageLedgerError( + "npm package name derived an artifact path outside the " + f"node_modules sandbox: {source!r} -> {candidate}" + ) + return candidate + return None + + def save(self, enabled_packages: dict[str, str], dry_run: bool = False) -> bool: + """Upsert artifact paths for every enabled `{entry_id: source}` pair. + + Entries already in the ledger for ids *not* in `enabled_packages` + (i.e. removed from config) are left untouched here -- they remain + visible to `find_stale()` until an explicit `prune()` removes them. + """ + state = self._read_state() + desired = dict(state) + for entry_id, source in enabled_packages.items(): + path = self.artifact_path_for_source(source) + desired[entry_id] = str(path) if path is not None else None + + if desired == state: + return False + if dry_run: + return True + + self._write_state(desired) + return True + + def find_stale(self, current_ids: set[str]) -> list[str]: + """Ids present in the ledger but absent from `current_ids`. Read-only.""" + state = self._read_state() + return sorted(set(state) - current_ids) + + def prune(self, stale_ids: list[str], dry_run: bool = False) -> list[str]: + """Delete only the ledger-recorded artifact paths for `stale_ids`. + + Never touches a path that isn't recorded in the ledger for one of + `stale_ids`. Returns the paths actually deleted (or, under + `dry_run`, that would be deleted). + """ + state = self._read_state() + remaining = dict(state) + deleted: list[str] = [] + + for entry_id in stale_ids: + if entry_id not in state: + continue + remaining.pop(entry_id, None) + recorded = state[entry_id] + if recorded is None: + continue + recorded_path = Path(recorded) + if not recorded_path.exists(): + continue + deleted.append(recorded) + if not dry_run: + if recorded_path.is_dir(): + shutil.rmtree(recorded_path) + else: + recorded_path.unlink() + + if not dry_run and remaining != state: + self._write_state(remaining) + return deleted + + def reconcile( + self, enabled_packages: dict[str, str], dry_run: bool = False + ) -> list[str]: + """Backfill ledger entries missing after an interrupted sync. + + Recovery strategy: the caller re-derives the current + `{entry_id: source}` mapping the same way `sync_pi_settings()` does + (re-loading the tiered Pi config, since the rendered + `settings.json` array has already dropped stable ids) and passes it + here. Only ids absent from the ledger are added -- an id already + recorded is left exactly as-is, even if its source has since + changed (that's `save()`'s job on the next normal sync, not + reconcile's). Returns the ids actually recovered. + """ + state = self._read_state() + merged = dict(state) + recovered: list[str] = [] + + for entry_id, source in enabled_packages.items(): + if entry_id in state: + continue + path = self.artifact_path_for_source(source) + merged[entry_id] = str(path) if path is not None else None + recovered.append(entry_id) + + if recovered and not dry_run: + self._write_state(merged) + return recovered + + def _read_state(self) -> dict[str, str | None]: + if not self.state_path.exists(): + return {} + try: + value = json.loads(self.state_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as error: + raise PiPackageLedgerError( + f"Cannot read {self.state_path}: {error}" + ) from error + if not isinstance(value, dict) or not all( + isinstance(key, str) and (val is None or isinstance(val, str)) + for key, val in value.items() + ): + raise PiPackageLedgerError( + f"{self.state_path} must be a JSON object mapping id -> path-or-null" + ) + return value + + def _write_state(self, value: dict[str, str | None]) -> None: + self.state_path.parent.mkdir(parents=True, exist_ok=True) + content = json.dumps(value, indent=2, sort_keys=True) + "\n" + file_descriptor, temporary_name = tempfile.mkstemp( + dir=self.state_path.parent, prefix=f".{self.state_path.name}.", text=True + ) + temporary_path = Path(temporary_name) + try: + with os.fdopen(file_descriptor, "w", encoding="utf-8") as temporary_file: + temporary_file.write(content) + temporary_file.flush() + os.fsync(temporary_file.fileno()) + os.chmod(temporary_path, 0o600) + os.replace(temporary_path, self.state_path) + finally: + temporary_path.unlink(missing_ok=True) + + +def _npm_package_name(source: str) -> str: + """Extract the npm package name from an `npm:`-prefixed source string. + + Handles both unversioned (`npm:@scope/name`, the exact shape observed + in the Epic 2.1 spike) and versioned (`npm:@scope/name@1.2.3` or + `npm:name@1.2.3`, the shape `pi_config.py`'s `_is_allowed_package_source` + requires for `npm:@tstapler/` sources) forms. + """ + spec = source.removeprefix("npm:") + if spec.startswith("@"): + scope, separator, rest = spec.partition("/") + if not separator: + return spec + name, at, _version = rest.rpartition("@") + return f"{scope}/{name if at else rest}" + name, at, _version = spec.rpartition("@") + return name if at else spec diff --git a/stapler-scripts/llm-sync/test_extension_manifest.py b/stapler-scripts/llm-sync/test_extension_manifest.py new file mode 100644 index 00000000..fa6b44f0 --- /dev/null +++ b/stapler-scripts/llm-sync/test_extension_manifest.py @@ -0,0 +1,263 @@ +"""Regression checks for the Pi extension review manifest loader. + +Run directly: uv run test_extension_manifest.py +""" + +import json +import sys +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +sys.path.append(str(Path(__file__).parent / "src")) + +from sources.extension_manifest import ( + ExtensionManifestSource, + ManifestEntry, + ManifestError, +) + +_REPO_ROOT = Path(__file__).parent.parent.parent +_TRACKED_MANIFEST = _REPO_ROOT / ".config" / "pi" / "extensions-manifest.json" + +_BASE_FIELDS = { + "id": "gotgenes-pi-permission-system", + "capability": "permission-system", + "upstream_repo": "https://github.com/gotgenes/pi-permission-system", + "upstream_commit": "0123456789abcdef0123456789abcdef01234567", + "license": "MIT", + "fork_repo": "https://github.com/tstapler/pi-permission-system", + "fork_commit": "abcdef0123456789abcdef0123456789abcdef01", +} + + +def _write(path: Path, value: object) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value), encoding="utf-8") + + +def _write_approved_entry(path: Path, notes: str) -> None: + entry = { + **_BASE_FIELDS, + "id": "narumiruna-pi-plan-mode", + "disposition": "approved", + "approved_by": "tstapler", + "approved_date": "2026-09-01", + "notes": notes, + } + _write(path, {"extensions": {"narumiruna-pi-plan-mode": entry}}) + + +def test_tracked_manifest_file_parses_to_empty_extensions_registry(): + manifest = ExtensionManifestSource.load(_TRACKED_MANIFEST) + assert manifest.entries == {} + + +def test_load_raises_on_entry_missing_required_field(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + entry = {**_BASE_FIELDS, "disposition": "candidate"} + del entry["upstream_commit"] + _write(path, {"extensions": {"gotgenes-pi-permission-system": entry}}) + + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + assert "gotgenes-pi-permission-system" in str(error) + assert "upstream_commit" in str(error) + else: + raise AssertionError("missing required field must raise ManifestError") + + +def test_manifest_load_rejects_approved_entry_missing_approved_by_or_date_naming_entry_and_fields(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + entry = { + **_BASE_FIELDS, + "id": "narumiruna-pi-plan-mode", + "disposition": "approved", + "approved_by": None, + "approved_date": None, + } + _write(path, {"extensions": {"narumiruna-pi-plan-mode": entry}}) + + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + message = str(error) + assert "narumiruna-pi-plan-mode" in message + assert "approved_by" in message + assert "approved_date" in message + else: + raise AssertionError( + "approved entry missing approved_by/approved_date must raise ManifestError" + ) + + +def test_candidate_entry_with_null_approved_by_loads_successfully(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + entry = { + **_BASE_FIELDS, + "id": "narumiruna-pi-plan-mode", + "disposition": "candidate", + "approved_by": None, + "approved_date": None, + } + _write(path, {"extensions": {"narumiruna-pi-plan-mode": entry}}) + + manifest = ExtensionManifestSource.load(path) + + assert manifest.entries["narumiruna-pi-plan-mode"].disposition == "candidate" + + +def test_manifest_entry_construction_rejects_approved_without_approver(): + try: + ManifestEntry( + **_BASE_FIELDS, + package_paths=None, + disposition="approved", + reviewer=None, + review_date=None, + notes=None, + approved_by=None, + approved_date=None, + ) + except ManifestError as error: + message = str(error) + assert _BASE_FIELDS["id"] in message + assert "approved_by" in message + assert "approved_date" in message + else: + raise AssertionError( + "direct ManifestEntry construction must enforce the approval invariant" + ) + + +def test_approved_entry_with_fewer_than_five_notes_paths_is_rejected(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + _write_approved_entry( + path, "Reviewed `src/network.py` and `src/subprocess_runner.py` only." + ) + + with patch("sources.extension_manifest.subprocess.run") as mock_run: + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + assert "narumiruna-pi-plan-mode" in str(error) + else: + raise AssertionError( + "approved entry with <5 notes paths must raise ManifestError" + ) + + # Too few candidate paths is decidable from `notes` text alone; no + # need to spend a `gh api` call confirming existence. + mock_run.assert_not_called() + + +def test_approved_entry_with_five_verified_notes_paths_passes(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + paths = [ + "src/network.py", + "src/subprocess_runner.py", + "src/filesystem.py", + "src/secrets.py", + "src/telemetry.py", + ] + _write_approved_entry(path, "Reviewed " + ", ".join(f"`{p}`" for p in paths) + ".") + + tree_response = json.dumps({"tree": [{"path": p, "type": "blob"} for p in paths]}) + fake_result = MagicMock(returncode=0, stdout=tree_response, stderr="") + + with patch( + "sources.extension_manifest.subprocess.run", return_value=fake_result + ) as mock_run: + manifest = ExtensionManifestSource.load(path) + + assert manifest.entries["narumiruna-pi-plan-mode"].disposition == "approved" + mock_run.assert_called_once() + gh_args = mock_run.call_args.args[0] + assert gh_args[:2] == ["gh", "api"] + assert _BASE_FIELDS["fork_commit"] in gh_args[2] + + +def test_approved_entry_with_unverifiable_notes_path_is_rejected_by_name(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + paths = [ + "src/network.py", + "src/subprocess_runner.py", + "src/filesystem.py", + "src/secrets.py", + "src/telemetry_missing.py", + ] + _write_approved_entry(path, "Reviewed " + ", ".join(f"`{p}`" for p in paths) + ".") + + # Every cited path except the last is really in the fork tree. + tree_response = json.dumps({"tree": [{"path": p, "type": "blob"} for p in paths[:-1]]}) + fake_result = MagicMock(returncode=0, stdout=tree_response, stderr="") + + with patch("sources.extension_manifest.subprocess.run", return_value=fake_result): + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + assert "src/telemetry_missing.py" in str(error) + else: + raise AssertionError( + "notes path missing from fork tree must raise ManifestError" + ) + + +def test_extension_manifest_load_raises_on_duplicate_entry_id(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + first = {**_BASE_FIELDS, "id": "gotgenes-pi-packages", "disposition": "candidate"} + second = {**_BASE_FIELDS, "id": "gotgenes-pi-packages", "disposition": "candidate"} + _write( + path, + { + "extensions": { + "gotgenes-pi-packages-v1": first, + "gotgenes-pi-packages-v2": second, + } + }, + ) + + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + assert "gotgenes-pi-packages" in str(error) + else: + raise AssertionError("duplicate manifest entry id must raise ManifestError") + + +def test_load_returns_empty_registry_when_manifest_file_missing(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "does-not-exist.json" + + manifest = ExtensionManifestSource.load(path) + + assert manifest.entries == {} + + +def test_load_raises_manifest_error_on_malformed_json(): + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "extensions-manifest.json" + path.write_text("{not valid json", encoding="utf-8") + + try: + ExtensionManifestSource.load(path) + except ManifestError as error: + assert str(path) in str(error) + else: + raise AssertionError("malformed manifest JSON must raise ManifestError") + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed") diff --git a/stapler-scripts/llm-sync/test_fork_pin_extension.py b/stapler-scripts/llm-sync/test_fork_pin_extension.py new file mode 100644 index 00000000..e3457628 --- /dev/null +++ b/stapler-scripts/llm-sync/test_fork_pin_extension.py @@ -0,0 +1,376 @@ +#!/usr/bin/env -S uv run +# /// script +# requires-python = ">=3.11" +# dependencies = [ +# "typer>=0.12", +# ] +# /// +"""Regression checks for the fork-and-pin helper script. + +Run directly: uv run test_fork_pin_extension.py + +Every `gh` call is mocked -- no real network/GitHub access happens here. +""" + +import contextlib +import inspect +import io +import json +import re +import sys +import tempfile +from pathlib import Path +from unittest.mock import patch + +sys.path.append(str(Path(__file__).parent / "src")) +sys.path.append(str(Path(__file__).parent / "scripts")) + +import typer # noqa: E402 (path setup above must run first) +import typer.main # noqa: E402 + +import fork_pin_extension # noqa: E402 + +_SCRIPT_PATH = Path(__file__).parent / "scripts" / "fork_pin_extension.py" + +# Matches an exact quoted string literal `"approved"` / `'approved'` -- but +# not `"approved_by"` or `"approved_date"`, which are real, required field +# names this script legitimately references (always to set them to null). +_APPROVED_LITERAL_RE = re.compile(r"""(['"])approved\1""") + +_FIXED_SHA = "cafef00d0123456789abcdef0123456789abcdef" + + +def _write(path: Path, value: object) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value), encoding="utf-8") + + +def _run_fork(**kwargs) -> tuple[str, str, int | None]: + """Call the `fork` command function directly, capturing stdout/stderr. + + Returns (stdout, stderr, exit_code). exit_code is None when the command + returned normally instead of raising typer.Exit. + """ + out, err = io.StringIO(), io.StringIO() + exit_code = None + try: + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + fork_pin_extension.fork(**kwargs) + except typer.Exit as exit_error: + exit_code = exit_error.exit_code + return out.getvalue(), err.getvalue(), exit_code + + +def _extract_json_block(stdout: str, heading: str) -> dict: + """Pull the JSON entry printed under `heading` (see `_print_entry`). + + Uses `raw_decode` rather than `json.loads` because real (non-dry-run) + output has more lines after the JSON block; `raw_decode` stops at the + end of the first JSON value instead of erroring on the trailing text. + """ + _, _, remainder = stdout.partition(heading + "\n") + assert remainder, f"expected stdout to contain heading {heading!r}:\n{stdout}" + entry, _ = json.JSONDecoder().raw_decode(remainder) + return entry + + +def test_fork_pin_extension_argparser_has_no_flag_that_writes_approved_disposition(): + source = _SCRIPT_PATH.read_text(encoding="utf-8") + matches = _APPROVED_LITERAL_RE.findall(source) + assert not matches, ( + "fork_pin_extension.py must never contain the quoted string literal " + "'approved' -- that's the one value disposition/approved_by/" + "approved_date must never be assigned by this script. (Field " + "*names* like approved_by/approved_date are fine and expected; " + "only the literal value \"approved\" is banned.) A source-text scan " + "was chosen over pure signature introspection because the value " + "could otherwise be smuggled in via a default, a constant, or a " + "dict literal that never shows up as a CLI-visible flag." + ) + + # Belt-and-suspenders: introspect the actual click command typer builds, + # confirming no declared CLI parameter is itself approval-named or + # defaults to the literal value "approved". + click_command = typer.main.get_command(fork_pin_extension.app) + for param in click_command.params: + assert "approv" not in (param.name or "").lower(), ( + f"unexpected approval-related CLI parameter: {param.name}" + ) + assert "approved" != str(param.default).lower() + + +def test_fork_pin_extension_fork_creates_candidate_manifest_entry_with_fork_commit(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + + with ( + patch.object(fork_pin_extension, "run_gh_fork") as mock_fork, + patch.object(fork_pin_extension, "run_gh_head_commit", return_value=_FIXED_SHA), + ): + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system,subagents", + dry_run=False, + manifest_file=manifest_file, + ) + + assert exit_code is None, f"unexpected failure: {stderr}" + mock_fork.assert_called_once_with("gotgenes/pi-packages") + + written = json.loads(manifest_file.read_text(encoding="utf-8")) + entry = written["extensions"]["gotgenes-pi-packages"] + assert entry["disposition"] == "candidate" + assert entry["fork_repo"] == "https://github.com/tstapler/pi-packages" + assert entry["fork_commit"] == _FIXED_SHA + assert entry["approved_by"] is None + assert entry["approved_date"] is None + + +def test_fork_pin_extension_fork_fails_when_id_already_has_manifest_entry(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + _write( + manifest_file, + {"extensions": {"existing-id": {"disposition": "candidate"}}}, + ) + + with ( + patch.object(fork_pin_extension, "run_gh_fork") as mock_fork, + patch.object(fork_pin_extension, "run_gh_head_commit") as mock_commit, + ): + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="existing-id", + capability="permission-system", + dry_run=False, + manifest_file=manifest_file, + ) + + assert exit_code == 1 + assert "existing-id" in stderr + mock_fork.assert_not_called() + mock_commit.assert_not_called() + + +def test_fork_pin_extension_dry_run_prints_plan_without_calling_gh(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + + with patch.object(fork_pin_extension, "subprocess") as mock_subprocess: + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=True, + manifest_file=manifest_file, + ) + + assert exit_code is None, f"unexpected failure: {stderr}" + mock_subprocess.run.assert_not_called() + assert not manifest_file.exists() + + +def test_fork_dry_run_shows_planned_target_and_manifest_diff_without_writing(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + _write(manifest_file, {"extensions": {}}) + original_bytes = manifest_file.read_bytes() + + def _fail_loudly(*_args, **_kwargs): + raise AssertionError("gh must not be called during --dry-run") + + with ( + patch.object(fork_pin_extension, "run_gh_fork", side_effect=_fail_loudly), + patch.object(fork_pin_extension, "run_gh_head_commit", side_effect=_fail_loudly), + ): + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=True, + manifest_file=manifest_file, + ) + + assert exit_code is None, f"unexpected failure: {stderr}" + assert "Would fork:" in stdout + assert "Would write to:" in stdout + assert manifest_file.read_bytes() == original_bytes + + +def test_fork_output_shows_candidate_disposition_and_null_approval_fields_always(): + with tempfile.TemporaryDirectory() as tmp: + dry_manifest = Path(tmp) / "dry-manifest.json" + real_manifest = Path(tmp) / "real-manifest.json" + + dry_stdout, _, dry_exit = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=True, + manifest_file=dry_manifest, + ) + assert dry_exit is None + + with ( + patch.object(fork_pin_extension, "run_gh_fork"), + patch.object(fork_pin_extension, "run_gh_head_commit", return_value=_FIXED_SHA), + ): + real_stdout, real_stderr, real_exit = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=False, + manifest_file=real_manifest, + ) + assert real_exit is None, f"unexpected failure: {real_stderr}" + + for stdout in (dry_stdout, real_stdout): + assert '"disposition": "candidate"' in stdout + assert '"approved_by": null' in stdout + assert '"approved_date": null' in stdout + + +def test_fork_real_run_printed_commit_matches_manifest_written_commit(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + + with ( + patch.object(fork_pin_extension, "run_gh_fork"), + patch.object(fork_pin_extension, "run_gh_head_commit", return_value=_FIXED_SHA), + ): + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=False, + manifest_file=manifest_file, + ) + assert exit_code is None, f"unexpected failure: {stderr}" + + printed = _extract_json_block(stdout, "Manifest entry:") + written = json.loads(manifest_file.read_text(encoding="utf-8")) + written_entry = written["extensions"]["gotgenes-pi-packages"] + + assert printed["fork_commit"] == written_entry["fork_commit"] + assert printed["fork_commit"] == _FIXED_SHA + + +def test_fork_rerun_existing_id_fails_naming_id_and_current_disposition(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + _write( + manifest_file, + { + "extensions": { + "gotgenes-pi-packages": { + "id": "gotgenes-pi-packages", + "disposition": "approved", + "approved_by": "tstapler", + "approved_date": "2026-09-01", + } + } + }, + ) + + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=False, + manifest_file=manifest_file, + ) + + assert exit_code == 1 + assert "gotgenes-pi-packages" in stderr + assert "approved" in stderr + + +def test_fork_dry_run_closing_line_states_next_concrete_action(): + with tempfile.TemporaryDirectory() as tmp: + manifest_file = Path(tmp) / "extensions-manifest.json" + + stdout, stderr, exit_code = _run_fork( + upstream="gotgenes/pi-packages", + entry_id="gotgenes-pi-packages", + capability="permission-system", + dry_run=True, + manifest_file=manifest_file, + ) + assert exit_code is None, f"unexpected failure: {stderr}" + + non_empty_lines = [line for line in stdout.splitlines() if line.strip()] + assert non_empty_lines, "dry-run produced no output" + assert non_empty_lines[-1].startswith("Re-run without --dry-run") + + +def test_fork_pin_extension_dry_run_matches_real_run_manifest_diff(): + with tempfile.TemporaryDirectory() as tmp: + real_manifest = Path(tmp) / "real-manifest.json" + dry_manifest = Path(tmp) / "dry-manifest.json" + + fixture = { + "upstream": "gotgenes/pi-packages", + "entry_id": "gotgenes-pi-packages", + "capability": "permission-system,subagents", + } + + with ( + patch.object(fork_pin_extension, "run_gh_fork"), + patch.object(fork_pin_extension, "run_gh_head_commit", return_value=_FIXED_SHA), + ): + _, real_stderr, real_exit = _run_fork( + dry_run=False, manifest_file=real_manifest, **fixture + ) + assert real_exit is None, f"unexpected failure: {real_stderr}" + persisted = json.loads(real_manifest.read_text(encoding="utf-8"))["extensions"][ + fixture["entry_id"] + ] + + def _fail_loudly(*_args, **_kwargs): + raise AssertionError("gh must not be called during --dry-run") + + with ( + patch.object(fork_pin_extension, "run_gh_fork", side_effect=_fail_loudly), + patch.object(fork_pin_extension, "run_gh_head_commit", side_effect=_fail_loudly), + ): + dry_stdout, dry_stderr, dry_exit = _run_fork( + dry_run=True, manifest_file=dry_manifest, **fixture + ) + assert dry_exit is None, f"unexpected failure: {dry_stderr}" + planned = _extract_json_block(dry_stdout, "Planned manifest entry:") + + # Every field that doesn't depend on `gh` output must be byte-for-byte + # identical between the dry-run preview and what the real run wrote -- + # proving the two code paths cannot silently diverge in how they build + # the entry (architecture-review.md's fork_pin_extension.py concern). + commit_dependent_fields = {"fork_commit", "upstream_commit"} + for key in persisted: + if key in commit_dependent_fields: + continue + assert planned[key] == persisted[key], f"field {key!r} diverged between dry-run and real run" + + assert planned["fork_commit"] == fork_pin_extension.PENDING_COMMIT + assert planned["upstream_commit"] == fork_pin_extension.PENDING_COMMIT + assert persisted["fork_commit"] == _FIXED_SHA + assert persisted["upstream_commit"] == _FIXED_SHA + + # Tightest possible version of the same guarantee: feed the shared + # entry-builder the exact commit the real run used, and require an + # exact match against what was persisted -- proving both paths route + # through the very same function, not just similarly-shaped ones. + replayed = fork_pin_extension.build_entry_dict( + entry_id=fixture["entry_id"], + capability=fixture["capability"], + upstream=fixture["upstream"], + commit=_FIXED_SHA, + ) + assert replayed == persisted + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed") diff --git a/stapler-scripts/llm-sync/test_pi_config.py b/stapler-scripts/llm-sync/test_pi_config.py index b5c72730..be4b31d4 100644 --- a/stapler-scripts/llm-sync/test_pi_config.py +++ b/stapler-scripts/llm-sync/test_pi_config.py @@ -192,6 +192,35 @@ def test_rejects_credential_material_in_nested_settings(): raise AssertionError(f"credential-bearing key should be rejected: {key}") +def test_rejects_url_shaped_path_for_extension_registry_entry(): + """A URL-shaped `path` value isn't recognized by the fork-pin-review + gate's scan (only `packages`/`extensions` sources routed through + `parse_fork_source()` are), so it must be rejected at render time + instead of silently passing through unchecked.""" + rejected = ( + "https://github.com/someone-else/pi-extension", + "git:github.com/someone-else/pi-extension@0123456789abcdef", + "npm:@someone-else/pi-extension", + ) + for path in rejected: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write( + root / "config.json", + {"extensions": {"remote": {"path": path}}}, + ) + + try: + _source(root).load() + except PiConfigError as error: + assert "remote" in str(error) + assert "local path" in str(error) + else: + raise AssertionError( + f"URL-shaped extension path should be rejected: {path}" + ) + + def test_rejects_reserved_resource_keys_inside_settings(): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) diff --git a/stapler-scripts/llm-sync/test_pi_dry_run_ux.py b/stapler-scripts/llm-sync/test_pi_dry_run_ux.py new file mode 100644 index 00000000..f7edf749 --- /dev/null +++ b/stapler-scripts/llm-sync/test_pi_dry_run_ux.py @@ -0,0 +1,137 @@ +"""Surface 4 UX acceptance checks: `sync_pi_settings` dry-run output. + +Covers project_plans/pi-dotfiles/design/ux.md's "bootstrap/pyinfra dry-run +output" surface, criteria 1, 3, and 5 (destination path, no credential leak, +closing line). Criterion 2 (enumerate each changed key individually) is a +known, tracked gap: `PiSettingsTarget.save()` only returns a bool today, not +the set of changed keys, so there's nothing for `sync_pi_settings` to print +per-key without changing that method's return contract (and the several +existing `assert target.save(...) is True/False` call sites in +test_pi_settings_target.py that depend on it) -- out of scope for this pass. + +Run directly: uv run test_pi_dry_run_ux.py +""" + +import argparse +import contextlib +import io +import json +import tempfile +from pathlib import Path +import sys + +sys.path.append(str(Path(__file__).parent / "src")) + +from cli import sync_pi_settings +from sources.pi_config import PiConfigError + + +def _write_json(path: Path, value: dict) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value), encoding="utf-8") + + +def _base_args(root: Path, **overrides) -> argparse.Namespace: + defaults = dict( + pi_dir=root / "agent", + pi_config_file=root / "config.json", + pi_config_dir=root / "config.d", + pi_local_config=root / "config.local.json", + pi_local_config_dir=root / "config.local.d", + pi_extensions_manifest=root / "extensions-manifest.json", + pi_settings_file=root / "agent" / "settings.json", + pi_settings_state_file=root / "settings-state.json", + pi_package_ledger_state_file=root / "package-state.json", + dry_run=True, + prune_stale_pi_packages=False, + reconcile_pi_package_ledger=False, + ) + defaults.update(overrides) + return argparse.Namespace(**defaults) + + +def _run_sync(args: argparse.Namespace) -> str: + out = io.StringIO() + with contextlib.redirect_stdout(out): + sync_pi_settings(args) + return out.getvalue() + + +def test_dry_run_names_destination_path_pi_dir_or_real_path(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write_json(root / "config.d" / "10-settings.json", {"settings": {"theme": "dark"}}) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + agent_dir = root / "agent" + + output = _run_sync(_base_args(root)) + + assert str(agent_dir) in output + + +def test_dry_run_output_contains_no_credential_shaped_value(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + secret_value = "sk-super-secret-value-should-never-print" + _write_json( + root / "config.d" / "10-settings.json", + {"settings": {"apiKey": secret_value}}, + ) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + + out = io.StringIO() + raised = None + try: + with contextlib.redirect_stdout(out): + sync_pi_settings(_base_args(root)) + except PiConfigError as error: + raised = error + + # The credential-material check runs before any dry-run print, so the + # sync must abort rather than silently continue. + assert raised is not None + assert "apiKey" in str(raised) + output = out.getvalue() + assert secret_value not in output + assert secret_value not in str(raised) + for needle in ("apikey", "token", "auth"): + assert needle not in output.lower() + + +def test_dry_run_closing_line_states_no_changes_made_and_destination(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write_json(root / "config.d" / "10-settings.json", {"settings": {"theme": "dark"}}) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + agent_dir = root / "agent" + + output = _run_sync(_base_args(root)) + + assert f"No changes made to {agent_dir} (dry run)." in output.splitlines() + + +def test_dry_run_closing_line_present_even_when_already_converged(): + """A dry run against an already-converged state still states no changes + were made -- the closing line isn't conditional on there being a diff. + """ + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + _write_json(root / "config.d" / "10-settings.json", {"settings": {}}) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + agent_dir = root / "agent" + + # First, a real (non-dry) run to converge state on disk... + _run_sync(_base_args(root, dry_run=False)) + # ...then a dry run should report no changes without an "update" line. + output = _run_sync(_base_args(root)) + + assert f"No changes made to {agent_dir} (dry run)." in output.splitlines() + assert "Would update managed Pi settings" not in output + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed") diff --git a/stapler-scripts/llm-sync/test_pi_package_ledger.py b/stapler-scripts/llm-sync/test_pi_package_ledger.py new file mode 100644 index 00000000..0754d221 --- /dev/null +++ b/stapler-scripts/llm-sync/test_pi_package_ledger.py @@ -0,0 +1,357 @@ +"""Regression checks for the Pi package ownership ledger. + +Run directly: uv run test_pi_package_ledger.py +""" + +import argparse +import contextlib +import io +import json +import sys +import tempfile +from pathlib import Path + +sys.path.append(str(Path(__file__).parent / "src")) + +from cli import sync_pi_settings +from targets.pi_package_ledger import PiPackageLedger, PiPackageLedgerError + +_NPM_SOURCE = "npm:@gotgenes/pi-permission-system" +_NPM_ARTIFACT_SUFFIX = Path("npm") / "node_modules" / "@gotgenes" / "pi-permission-system" + + +# --- direct PiPackageLedger unit tests ------------------------------------- + + +def test_pi_package_ledger_save_records_artifact_paths_for_enabled_entries(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + + changed = ledger.save({"gotgenes-pi-packages": _NPM_SOURCE}) + + assert changed is True + recorded = json.loads((root / "state.json").read_text(encoding="utf-8")) + assert recorded == { + "gotgenes-pi-packages": str(root / "agent" / _NPM_ARTIFACT_SUFFIX) + } + + +def test_pi_package_ledger_find_stale_reports_without_deleting(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + artifact_dir = root / "agent" / _NPM_ARTIFACT_SUFFIX + artifact_dir.mkdir(parents=True) + (artifact_dir / "index.js").write_text("// installed", encoding="utf-8") + + state_path = root / "state.json" + state_path.write_text( + json.dumps({"gotgenes-pi-packages": str(artifact_dir)}), encoding="utf-8" + ) + ledger = PiPackageLedger(state_path=state_path, agent_dir=root / "agent") + + stale = ledger.find_stale(current_ids=set()) + + assert stale == ["gotgenes-pi-packages"] + assert artifact_dir.exists() + assert json.loads(state_path.read_text(encoding="utf-8")) == { + "gotgenes-pi-packages": str(artifact_dir) + } + + +def test_pi_package_ledger_prune_deletes_only_ledger_recorded_paths(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + managed_dir = root / "agent" / _NPM_ARTIFACT_SUFFIX + managed_dir.mkdir(parents=True) + (managed_dir / "index.js").write_text("// installed", encoding="utf-8") + + unmanaged_dir = root / "agent" / "npm" / "node_modules" / "unmanaged-package" + unmanaged_dir.mkdir(parents=True) + (unmanaged_dir / "index.js").write_text("// not tracked", encoding="utf-8") + + state_path = root / "state.json" + state_path.write_text( + json.dumps({"gotgenes-pi-packages": str(managed_dir)}), encoding="utf-8" + ) + ledger = PiPackageLedger(state_path=state_path, agent_dir=root / "agent") + + stale = ledger.find_stale(current_ids=set()) + deleted = ledger.prune(stale, dry_run=False) + + assert deleted == [str(managed_dir)] + assert not managed_dir.exists() + assert unmanaged_dir.exists() + assert json.loads(state_path.read_text(encoding="utf-8")) == {} + + +def test_pi_package_ledger_prune_dry_run_deletes_nothing(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + managed_dir = root / "agent" / _NPM_ARTIFACT_SUFFIX + managed_dir.mkdir(parents=True) + + state_path = root / "state.json" + state_path.write_text( + json.dumps({"gotgenes-pi-packages": str(managed_dir)}), encoding="utf-8" + ) + ledger = PiPackageLedger(state_path=state_path, agent_dir=root / "agent") + + deleted = ledger.prune(["gotgenes-pi-packages"], dry_run=True) + + assert deleted == [str(managed_dir)] + assert managed_dir.exists() + assert json.loads(state_path.read_text(encoding="utf-8")) == { + "gotgenes-pi-packages": str(managed_dir) + } + + +def test_pi_package_ledger_reconcile_recovers_entry_after_interrupted_write(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + # No ledger file at all: simulates PiSettingsTarget.save() having + # succeeded (settings.json already reflects the entry as active) + # but the process crashing before PiPackageLedger.save() ran. + enabled_packages = {"gotgenes-pi-packages": _NPM_SOURCE} + + recovered = ledger.reconcile(enabled_packages) + + assert recovered == ["gotgenes-pi-packages"] + assert json.loads((root / "state.json").read_text(encoding="utf-8")) == { + "gotgenes-pi-packages": str(root / "agent" / _NPM_ARTIFACT_SUFFIX) + } + assert ledger.find_stale(current_ids={"gotgenes-pi-packages"}) == [] + + +def test_pi_package_ledger_reconcile_never_overwrites_existing_entries(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + state_path = root / "state.json" + state_path.write_text( + json.dumps({"gotgenes-pi-packages": "/pinned/by/hand"}), encoding="utf-8" + ) + ledger = PiPackageLedger(state_path=state_path, agent_dir=root / "agent") + + recovered = ledger.reconcile({"gotgenes-pi-packages": _NPM_SOURCE}) + + assert recovered == [] + assert json.loads(state_path.read_text(encoding="utf-8")) == { + "gotgenes-pi-packages": "/pinned/by/hand" + } + + +def test_pi_package_ledger_save_records_null_for_unverified_source_shape(): + """Non-npm sources (git-fork, local path) have no confirmed artifact-path + pattern per the Epic 2.1 spike, so they're recorded as an explicit + unknown sentinel rather than a fabricated path.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + + ledger.save( + {"fork-package": "git:github.com/tstapler/pi-tools@abc1234"} + ) + + assert json.loads((root / "state.json").read_text(encoding="utf-8")) == { + "fork-package": None + } + + +def test_artifact_path_for_source_strips_version_for_scoped_npm_source(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + + path = ledger.artifact_path_for_source("npm:@scope/name@1.2.3") + + assert path == root / "agent" / "npm" / "node_modules" / "@scope" / "name" + + +def test_artifact_path_for_source_strips_version_for_unscoped_npm_source(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + + path = ledger.artifact_path_for_source("npm:name@1.2.3") + + assert path == root / "agent" / "npm" / "node_modules" / "name" + + +def test_artifact_path_for_source_rejects_path_traversal_in_package_name(): + """A crafted source like `npm:@tstapler/../../pwn@1.0.0` passes + `pi_config.py`'s allowlist (it matches the `npm:@tstapler/` prefix) but + must not be allowed to resolve outside the node_modules sandbox.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + ledger = PiPackageLedger(state_path=root / "state.json", agent_dir=root / "agent") + + try: + ledger.artifact_path_for_source("npm:@tstapler/../../pwn@1.0.0") + except PiPackageLedgerError as error: + assert "npm:@tstapler/../../pwn@1.0.0" in str(error) + else: + raise AssertionError("path-traversal source should raise, not resolve") + + +def test_pi_package_ledger_save_rejects_malformed_ledger_state(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + state_path = root / "state.json" + state_path.write_text("not-json", encoding="utf-8") + ledger = PiPackageLedger(state_path=state_path, agent_dir=root / "agent") + + try: + ledger.save({"gotgenes-pi-packages": _NPM_SOURCE}) + except PiPackageLedgerError as error: + assert str(state_path) in str(error) + else: + raise AssertionError("malformed ledger state should fail safely") + + +# --- CLI-level (Surface 3) tests -------------------------------------------- + + +def _write_json(path: Path, value: object) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value), encoding="utf-8") + + +def _base_args(root: Path, **overrides) -> argparse.Namespace: + defaults = dict( + pi_dir=root / "agent", + pi_config_file=root / "config.json", + pi_config_dir=root / "config.d", + pi_local_config=root / "config.local.json", + pi_local_config_dir=root / "config.local.d", + pi_extensions_manifest=root / "extensions-manifest.json", + pi_settings_file=root / "agent" / "settings.json", + pi_settings_state_file=root / "settings-state.json", + pi_package_ledger_state_file=root / "package-state.json", + dry_run=False, + prune_stale_pi_packages=False, + reconcile_pi_package_ledger=False, + ) + defaults.update(overrides) + return argparse.Namespace(**defaults) + + +def _seed_environment(root: Path) -> Path: + """Config with no packages enabled, plus a pre-existing stale ledger entry + named `old-extension` whose recorded artifact path is a real directory. + + Returns that artifact directory so tests can assert on its survival or + deletion. + """ + _write_json(root / "config.d" / "10-empty.json", {"packages": {}}) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + + artifact_dir = root / "agent" / "npm" / "node_modules" / "old-extension" + artifact_dir.mkdir(parents=True) + (artifact_dir / "index.js").write_text("// stale", encoding="utf-8") + + _write_json(root / "package-state.json", {"old-extension": str(artifact_dir)}) + return artifact_dir + + +def _run_sync(args: argparse.Namespace) -> str: + out = io.StringIO() + with contextlib.redirect_stdout(out): + sync_pi_settings(args) + return out.getvalue() + + +def test_stale_report_line_matches_exact_format_without_prune_flag(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + artifact_dir = _seed_environment(root) + + output = _run_sync(_base_args(root)) + + assert ( + "stale Pi package: old-extension " + "(not pruned; run with --prune-stale-pi-packages)" + ) in output.splitlines() + assert artifact_dir.exists() + + +def test_stale_report_always_runs_deletion_only_with_flag(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + artifact_dir = _seed_environment(root) + + without_flag = _run_sync(_base_args(root)) + assert "stale Pi package: old-extension" in without_flag + assert artifact_dir.exists() + + with_flag = _run_sync(_base_args(root, prune_stale_pi_packages=True)) + assert "stale Pi package: old-extension" in with_flag + assert not artifact_dir.exists() + + +def test_prune_flag_prints_pruned_path_after_deletion(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + artifact_dir = _seed_environment(root) + + output = _run_sync(_base_args(root, prune_stale_pi_packages=True)) + + stale_index = output.index("stale Pi package: old-extension") + pruned_index = output.index(f"Pruned: {artifact_dir}") + assert pruned_index > stale_index + assert not artifact_dir.exists() + + +def test_prune_output_silent_for_paths_outside_ledger(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + artifact_dir = _seed_environment(root) + unmanaged_dir = root / "agent" / "npm" / "node_modules" / "unmanaged-package" + unmanaged_dir.mkdir(parents=True) + (unmanaged_dir / "index.js").write_text("// keep me", encoding="utf-8") + + output = _run_sync(_base_args(root, prune_stale_pi_packages=True)) + + assert "unmanaged-package" not in output + assert unmanaged_dir.exists() + assert not artifact_dir.exists() + + +def test_reconcile_flag_recovers_missing_ledger_entry_through_cli(): + """Simulates a crash between `PiSettingsTarget.save()` (settings.json + already reflects the enabled package) and `PiPackageLedger.save()` + (never ran, so the ledger has no entry for it): pre-seed settings.json + and its state file so this run's `changed` is False -- otherwise the + normal `ledger.save()` path in `sync_pi_settings()` would mask the gap + `--reconcile-pi-package-ledger` is meant to recover from. + """ + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + entry_id = "gotgenes-pi-packages" + _write_json( + root / "config.d" / "10-packages.json", + { + "trustedPackageScopes": ["npm:@gotgenes"], + "packages": {entry_id: {"source": _NPM_SOURCE}}, + }, + ) + _write_json(root / "extensions-manifest.json", {"extensions": {}}) + _write_json(root / "agent" / "settings.json", {"packages": [_NPM_SOURCE]}) + _write_json(root / "settings-state.json", {"managedKeys": ["packages"]}) + # No package-state.json: the ledger has no entry for `entry_id`. + + output = _run_sync(_base_args(root, reconcile_pi_package_ledger=True)) + + assert f"Reconciled Pi package ledger entry: {entry_id}" in output + recorded = json.loads((root / "package-state.json").read_text(encoding="utf-8")) + assert recorded == { + entry_id: str(root / "agent" / _NPM_ARTIFACT_SUFFIX) + } + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed") diff --git a/stapler-scripts/llm-sync/test_pi_settings_target.py b/stapler-scripts/llm-sync/test_pi_settings_target.py index 40a4d248..5f4c8435 100644 --- a/stapler-scripts/llm-sync/test_pi_settings_target.py +++ b/stapler-scripts/llm-sync/test_pi_settings_target.py @@ -100,6 +100,106 @@ def test_rejects_malformed_ownership_state(): raise AssertionError("malformed ownership state should fail safely") +def test_rollback_restores_only_managed_keys_and_preserves_auth_and_sessions(): + """Rollback procedure verification for rollout-runbook.md's Task 5.2.1b. + + The runbook's documented rollback is a literal `cp` of settings.json and + the state file, not a call through PiSettingsTarget. This proves two + things empirically: PiSettingsTarget never touches auth.json/session + files (so a rollback that never names them is safe by construction), and + the restore is a whole-file copy rather than a managedKeys-scoped patch + -- a work-only key changed by something other than PiSettingsTarget + between backup and rollback is reverted too, not preserved. This + contradicts a literal reading of the runbook's "no more, no less" claim; + see the corrected wording in rollout-runbook.md's Rollback section. + """ + with tempfile.TemporaryDirectory() as tmp: + agent_dir = Path(tmp) / "agent" + agent_dir.mkdir() + settings_path = agent_dir / "settings.json" + state_path = agent_dir / "pi-settings-state.json" + auth_path = agent_dir / "auth.json" + session_path = agent_dir / "session-abc123.json" + + # Pre-adoption: theme/packages are managedKeys-tracked; workOnlyKey + # simulates a pre-existing work-specific key dotfiles never manages. + settings_path.write_text( + json.dumps( + { + "theme": "old-theme", + "packages": ["npm:@tstapler/pi-permission-system"], + "workOnlyKey": "pre-existing-work-value", + } + ), + encoding="utf-8", + ) + state_path.write_text( + json.dumps({"managedKeys": ["theme", "packages"]}), encoding="utf-8" + ) + auth_content = b'{"token": "do-not-touch-me"}' + session_content = b'{"sessionId": "abc123", "history": ["hi"]}' + auth_path.write_bytes(auth_content) + session_path.write_bytes(session_content) + + # Task 5.2.1a: backup is a literal file copy, taken before adoption. + settings_backup = settings_path.with_name(settings_path.name + ".pre-dotfiles-backup") + state_backup = state_path.with_name(state_path.name + ".pre-dotfiles-backup") + settings_backup.write_bytes(settings_path.read_bytes()) + state_backup.write_bytes(state_path.read_bytes()) + + # Adoption: a managed sync mutates only the managed keys... + changed = PiSettingsTarget(settings_path, state_path).save( + _loaded( + { + "theme": "new-theme", + "packages": [ + "npm:@tstapler/pi-permission-system", + "npm:@work-org/pi-agent", + ], + }, + {"theme", "packages"}, + ) + ) + assert changed is True + + # ...proven empirically: workOnlyKey and the auth/session files are + # untouched by PiSettingsTarget itself. + after_save = json.loads(settings_path.read_text(encoding="utf-8")) + assert after_save["workOnlyKey"] == "pre-existing-work-value" + assert auth_path.read_bytes() == auth_content + assert session_path.read_bytes() == session_content + + # ...and, separately, something other than PiSettingsTarget (Pi + # itself, a manual edit) changes the non-managed key in the same + # adopted window, to test whether rollback really scopes itself to + # managedKeys or reverts the whole file. + after_save["workOnlyKey"] = "changed-after-adoption-by-something-else" + settings_path.write_text(json.dumps(after_save), encoding="utf-8") + + # Task 5.2.1b: rollback is a literal file copy, not a + # PiSettingsTarget call. + settings_path.write_bytes(settings_backup.read_bytes()) + state_path.write_bytes(state_backup.read_bytes()) + + restored_settings = json.loads(settings_path.read_text(encoding="utf-8")) + restored_state = json.loads(state_path.read_text(encoding="utf-8")) + + assert restored_settings["theme"] == "old-theme" + assert restored_settings["packages"] == ["npm:@tstapler/pi-permission-system"] + assert restored_state == {"managedKeys": ["theme", "packages"]} + + # Discrepancy: the runbook claims this restore touches "exactly the + # keys that were present in managedKeys ... no more, no less." The + # actual mechanism is a whole-file copy, so workOnlyKey's + # out-of-band post-adoption change is reverted too, not preserved. + assert restored_settings["workOnlyKey"] == "pre-existing-work-value" + + # The one guarantee that holds unconditionally, because the copy + # commands never name these paths at all. + assert auth_path.read_bytes() == auth_content + assert session_path.read_bytes() == session_content + + if __name__ == "__main__": tests = [value for key, value in list(globals().items()) if key.startswith("test_")] for test in tests: diff --git a/stapler-scripts/llm-sync/test_review_gate.py b/stapler-scripts/llm-sync/test_review_gate.py new file mode 100644 index 00000000..275ae2dd --- /dev/null +++ b/stapler-scripts/llm-sync/test_review_gate.py @@ -0,0 +1,325 @@ +"""Regression checks for the manifest enforcement gate in the Pi settings sync. + +Run directly: uv run test_review_gate.py +""" + +import argparse +import json +import sys +import tempfile +from pathlib import Path + +sys.path.append(str(Path(__file__).parent / "src")) + +from cli import sync_pi_settings +from sources.extension_manifest import ExtensionManifest, ManifestEntry +from sources.pi_config import LoadedPiConfig, PiConfigError, parse_fork_source +from sources.review_gate import verify_pinned_sources_reviewed + +_BASE_ENTRY_FIELDS = { + "capability": "permission-system", + "upstream_repo": "https://github.com/gotgenes/pi-permission-system", + "upstream_commit": "0123456789abcdef0123456789abcdef01234567", + "license": "MIT", + "package_paths": None, + "reviewer": None, + "review_date": None, + "notes": None, + "approved_by": None, + "approved_date": None, +} + +_FORK_SOURCE = "git:github.com/tstapler/pi-permission-system@abc1234" + + +def _entry(**overrides) -> ManifestEntry: + fields = { + **_BASE_ENTRY_FIELDS, + "id": "gotgenes-pi-permission-system", + "fork_repo": "https://github.com/tstapler/pi-permission-system", + "fork_commit": "abc1234", + "disposition": "candidate", + **overrides, + } + return ManifestEntry(**fields) + + +def _approved_entry(**overrides) -> ManifestEntry: + return _entry(disposition="approved", approved_by="tstapler", approved_date="2026-09-01", **overrides) + + +def _manifest(*entries: ManifestEntry) -> ExtensionManifest: + return ExtensionManifest(entries={entry.id: entry for entry in entries}) + + +def _loaded(packages: list = None, extensions: list = None, trusted_scopes: tuple = ()) -> LoadedPiConfig: + settings = {} + if packages is not None: + settings["packages"] = packages + if extensions is not None: + settings["extensions"] = extensions + return LoadedPiConfig( + settings=settings, + managed_keys=set(settings), + layers=(), + trusted_scopes=trusted_scopes, + ) + + +def test_verify_pinned_sources_reviewed_raises_when_source_has_no_manifest_entry(): + loaded = _loaded(packages=[_FORK_SOURCE]) + try: + verify_pinned_sources_reviewed(loaded, _manifest()) + except PiConfigError as error: + assert _FORK_SOURCE in str(error) + else: + raise AssertionError("a source with no manifest entry must raise PiConfigError") + + +def test_verify_pinned_sources_reviewed_passes_when_approved_entry_matches_exact_commit(): + loaded = _loaded(packages=[_FORK_SOURCE]) + manifest = _manifest(_approved_entry(fork_commit="abc1234")) + verify_pinned_sources_reviewed(loaded, manifest) + + +def test_verify_pinned_sources_reviewed_raises_when_commit_mismatched(): + loaded = _loaded(packages=["git:github.com/tstapler/pi-permission-system@def5678"]) + manifest = _manifest(_approved_entry(fork_commit="abc1234")) + try: + verify_pinned_sources_reviewed(loaded, manifest) + except PiConfigError as error: + message = str(error) + assert "def5678" in message + assert "abc1234" in message + else: + raise AssertionError("a stale-commit approval must still raise PiConfigError") + + +def test_verify_pinned_sources_reviewed_raises_when_disposition_is_hold(): + loaded = _loaded(packages=[_FORK_SOURCE]) + manifest = _manifest(_entry(fork_commit="abc1234", disposition="hold")) + try: + verify_pinned_sources_reviewed(loaded, manifest) + except PiConfigError as error: + message = str(error) + assert _FORK_SOURCE in message + assert "hold" in message + else: + raise AssertionError("a 'hold' disposition must still raise PiConfigError") + + +def test_verify_pinned_sources_reviewed_raises_when_disposition_is_rejected(): + loaded = _loaded(packages=[_FORK_SOURCE]) + manifest = _manifest(_entry(fork_commit="abc1234", disposition="rejected")) + try: + verify_pinned_sources_reviewed(loaded, manifest) + except PiConfigError as error: + message = str(error) + assert _FORK_SOURCE in message + assert "rejected" in message + else: + raise AssertionError("a 'rejected' disposition must still raise PiConfigError") + + +def test_verify_pinned_sources_reviewed_matches_commit_case_insensitively(): + upper_manifest = _manifest(_approved_entry(fork_commit="ABC1234")) + verify_pinned_sources_reviewed(_loaded(packages=[_FORK_SOURCE]), upper_manifest) + + lower_manifest = _manifest(_approved_entry(fork_commit="abc1234")) + upper_source = "git:github.com/tstapler/pi-permission-system@ABC1234" + verify_pinned_sources_reviewed(_loaded(packages=[upper_source]), lower_manifest) + + +def test_verify_pinned_sources_reviewed_treats_prefix_length_mismatch_as_mismatch(): + full_sha = "abc1234def5678901234567890123456789012ab" + manifest = _manifest(_approved_entry(fork_commit=full_sha)) + loaded = _loaded(packages=[_FORK_SOURCE]) # pinned at the 7-char prefix "abc1234" + try: + verify_pinned_sources_reviewed(loaded, manifest) + except PiConfigError as error: + message = str(error) + assert "abc1234" in message + assert full_sha in message + else: + raise AssertionError("a short-vs-long commit pair must be treated as a mismatch") + + +def test_verify_pinned_sources_reviewed_exempts_trusted_scope_source_with_empty_manifest(): + loaded = _loaded( + packages=["npm:@work-org/pi-tool@2.3.0"], trusted_scopes=("npm:@work-org",) + ) + verify_pinned_sources_reviewed(loaded, _manifest()) + + +def test_verify_pinned_sources_reviewed_exempts_tstapler_scoped_source(): + loaded = _loaded(packages=["npm:@tstapler/pi-claude-compat@1.0.0"]) + verify_pinned_sources_reviewed(loaded, _manifest()) + + +def test_verify_pinned_sources_reviewed_only_scans_github_com_tstapler_fork_shaped_sources(): + loaded = _loaded( + packages=["npm:@other-org/pi-tool@1.0.0", "~/dotfiles/plugins/local"], + extensions=["~/dotfiles/plugins/ponytail/pi/index.ts"], + ) + # Empty manifest: if the scan touched any of these, it would raise. + verify_pinned_sources_reviewed(loaded, _manifest()) + + +def test_claude_compat_extension_path_source_exempt_from_manifest_gate(): + """Task 3.4.1d: `claude-compat` (`.config/pi/config.d/40-claude-compat.json`) + is a first-party, Tyler-owned local-path extension, not a fork. + `_render_path` in `pi_config.py` only ever renders a plain path string + for it, which `parse_fork_source()` never matches, so the gate must not + require a manifest entry — confirmed here against an empty manifest. + """ + loaded = _loaded(extensions=["~/dotfiles/plugins/pi-claude-compat/pi/index.ts"]) + verify_pinned_sources_reviewed(loaded, _manifest()) + + +_METAMORPHIC_SOURCE_CORPUS = ( + "git:github.com/tstapler/pi-tools@0123456789abcdef", + "https://github.com/tstapler/pi-tools@abc1234", + "git@github.com:tstapler/pi-tools@abc1234", + "git:github.com/someone-else/pi-tools@abc1234", # wrong owner + "git:github.com/tstapler/pi-tools@main", # mutable ref + "npm:@tstapler/pi-tool@1.2.3", # not fork-shaped at all + "npm:someone-elses-package@1.0.0", + "~/dotfiles/plugins/local", +) + + +def _source_checker(): + from sources.pi_config import PiConfigSource + from sources.tiered_config import TieredJsonConfig + + return PiConfigSource( + TieredJsonConfig( + universal_file=Path("/nonexistent/config.json"), + tracked_fragments_dir=Path("/nonexistent/config.d"), + local_file=Path("/nonexistent/config.local.json"), + local_fragments_dir=Path("/nonexistent/config.local.d"), + ) + ) + + +def _assert_agree(source: str, checker) -> None: + ref = parse_fork_source(source) + if ref is None: + # Not fork-shaped: _is_allowed_package_source may still accept it via + # the local/trusted-scope/npm-tstapler branches, but never via the + # fork branch. + return + assert ref.repo == "github.com/tstapler/pi-tools", source + if checker._is_allowed_package_source(source): + assert ref.commit.lower() in source.lower(), source + + +def test_parse_fork_source_and_is_allowed_package_source_agree(): + """Task 1.2.1h metamorphic test. + + `parse_fork_source()` is the single shape-parser both `pi_config.py`'s + `_is_allowed_package_source` and `review_gate.py`'s scan rely on: for + every fixture source, if `_is_allowed_package_source` accepts it via the + fork branch, `parse_fork_source` must have recognized the same repo and + commit (immutable-commit format is `_is_allowed_package_source`'s own + extra layer on top, not `parse_fork_source`'s job). + """ + checker = _source_checker() + for source in _METAMORPHIC_SOURCE_CORPUS: + _assert_agree(source, checker) + + +def test_sync_pi_settings_aborts_and_writes_nothing_when_source_unreviewed(): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + config_dir = root / "config.d" + config_dir.mkdir(parents=True) + (config_dir / "10-fork.json").write_text( + json.dumps({"packages": {"unreviewed": {"source": _FORK_SOURCE}}}), + encoding="utf-8", + ) + + manifest_path = root / "extensions-manifest.json" + manifest_path.write_text(json.dumps({"extensions": {}}), encoding="utf-8") + + settings_path = root / "agent" / "settings.json" + settings_path.parent.mkdir(parents=True) + original_settings = json.dumps({"theme": "dark", "lastChangelogVersion": "0.84.4"}) + settings_path.write_text(original_settings, encoding="utf-8") + + args = argparse.Namespace( + pi_dir=None, + pi_config_file=root / "config.json", + pi_config_dir=config_dir, + pi_local_config=root / "config.local.json", + pi_local_config_dir=root / "config.local.d", + pi_extensions_manifest=manifest_path, + pi_settings_file=settings_path, + pi_settings_state_file=root / "state.json", + dry_run=False, + ) + + try: + sync_pi_settings(args) + except PiConfigError as error: + assert _FORK_SOURCE in str(error) + else: + raise AssertionError("an unreviewed fork source must abort sync_pi_settings") + + assert settings_path.read_text(encoding="utf-8") == original_settings + assert not (root / "state.json").exists() + + +def test_blocked_sync_error_names_exact_unreviewed_source_string(): + loaded = _loaded(packages=[_FORK_SOURCE]) + try: + verify_pinned_sources_reviewed(loaded, _manifest()) + except PiConfigError as error: + assert str(error).count(_FORK_SOURCE) >= 1 + else: + raise AssertionError("expected PiConfigError") + + +def test_blocked_sync_stale_approval_names_both_configured_and_approved_commits(): + loaded = _loaded(packages=["git:github.com/tstapler/pi-permission-system@1111111"]) + manifest = _manifest(_approved_entry(fork_commit="2222222")) + try: + verify_pinned_sources_reviewed(loaded, manifest) + except PiConfigError as error: + message = str(error) + assert "1111111" in message + assert "2222222" in message + else: + raise AssertionError("expected PiConfigError naming both commits") + + +def test_blocked_sync_error_states_settings_json_unchanged(): + # Covered end-to-end by test_sync_pi_settings_aborts_and_writes_nothing_when_source_unreviewed; + # here we assert the gate itself never touches any settings state as a + # side effect (it is a pure check). + loaded = _loaded(packages=[_FORK_SOURCE]) + before = dict(loaded.settings) + try: + verify_pinned_sources_reviewed(loaded, _manifest()) + except PiConfigError: + pass + assert loaded.settings == before + + +def test_blocked_sync_never_fires_for_trusted_or_tstapler_scoped_sources(): + loaded = _loaded( + packages=[ + "npm:@work-org/pi-tool@2.3.0", + "npm:@tstapler/pi-claude-compat@1.0.0", + ], + trusted_scopes=("npm:@work-org",), + ) + verify_pinned_sources_reviewed(loaded, _manifest()) + + +if __name__ == "__main__": + tests = [value for key, value in list(globals().items()) if key.startswith("test_")] + for test in tests: + test() + print(f"ok {test.__name__}") + print(f"\n{len(tests)} checks passed")