M1 Phase 6: capture the PII workflow as a skill + the /tinkerdown:save capability

[adnaan]

The last M1 phase — the "keep it, the need recurs" path of the ephemeral-UI model. Two skills + the convention-13 doc.

skills/pii-access-approval/ — a pointer skill

A captured, re-runnable workflow: the PII / data-export approval console stands up in seconds with no LLM generation. It points at examples/pii-access-approval/ (triggers, when-to-use, stand-up steps) rather than copying it.

The plan said "bundle the manifest + golden app.md + fixtures." That would copy a committed source of truth and then need a byte-match guard to keep the two in sync — the duplicate-the-source anti-pattern this milestone kept killing (Phase 5's corpus rot). A skill that ships in-repo to re-run an in-repo app should reference it. Zero copied assets, zero drift.

/tinkerdown:save — the capture meta-skill, blind-verified

A skill that writes skills: given a session where the user generated + served an ephemeral UI they now want to keep, it distils it into a durable, re-runnable skill. It defines what to extract, how to produce skills/<name>/, and the bundle-vs-point rule (bundle ephemeral scratch artifacts — the skill is their only home; point at committed sources — don't duplicate), plus a required verify step.

Verified by a blind agent — given only the instructions + a synthetic "Team Standup Board" scratch session (never the golden PII skill), to avoid grading my own output. It produced a valid, validate-clean, correctly-bundled skill — and found 4 real gaps, all fixed:

1. The seed command was unspecified, and the natural sqlite3 … < seed.sql fails where sqlite3 isn't installed (proven in CI's environment — it fell back to python3). Added a python3 fallback everywhere the command appears.
2. tinkerdown serve never applies seed.sql — seeding is a mandatory-but-implicit step.
3. No criterion for --operator/--allow-exec.
4. Fresh-vs-persistent DB was unresolved and contradicts the recurring use case ("we run standup every day"): seed.sql did CREATE TABLE + sample inserts, so a re-run errors or wipes real data → now idempotent seed + seed-only-if-absent.

This is the value of the blind test over self-dogfooding: the mechanical capture (bundle-vs-point, what to extract, validate) was well-specified; the operational half wasn't, and only an independent run surfaced it.

docs/guides/ai-generation.md

The convention-13 model — ephemeral by default; persist on request as a re-runnable skill — plus a corrected stale note that the generate skill was "in development" (M1 Phase 3 shipped it).

Tests / scope

• captured_skills_test.go — structural (frontmatter present + every referenced repo path exists); also the guard that makes the pointer safe (a moved example fails it).
• No re-run-from-skill chromedp e2e — struck as redundant: the pointer skill serves the same artifacts Phase 4's four-channel e2e already drives end to end.

Verification

GOWORK=off go test -count=1 -timeout 60m ./... green (17 ok packages, 0 failures).

🤖 Generated with Claude Code

https://claude.ai/code/session_018M9pJSPmG6i1D8s6rpEV4h

[claude]

Review: M1 Phase 6 — capture skills + /tinkerdown:save

Overall this is well-structured, and the "point at committed sources, bundle only ephemeral ones" design decision (and the self-correction away from a byte-match-guard duplicate) is the right call. The blind-verification process for /tinkerdown:save is a nice touch — genuinely more convincing than self-grading. A couple of concrete issues below.

1. Heading hierarchy bug in docs/guides/ai-generation.md

The new "Ephemeral by default; persist on request" H2 section is inserted between the "Using Claude Code Skills" H2 and the "Example: Creating an App with Natural Language" H3. Since the latter is an H3, it now nests under "Ephemeral by default" instead of "Using Claude Code Skills" — but that example (a generic "create a task app" walkthrough) has nothing to do with the ephemeral/persist topic. This misattributes the section in the document outline / any generated TOC. Worth either promoting "Ephemeral by default…" to fit after the example, demoting it to H3 under "Using Claude Code Skills", or moving the example above it.

2. The captured skill's "Run it" steps aren't actually safe to re-run — which undercuts the whole premise

skills/pii-access-approval/SKILL.md (and examples/pii-access-approval/README.md) tell the user to run: mkdir -p data && sqlite3 data/access.db < seed.sql && tinkerdown serve . --operator you@corp.example

seed.sql uses plain CREATE TABLE (no IF NOT EXISTS) and non-idempotent INSERTs. If someone re-runs these exact steps against an existing data/access.db — e.g. running the "captured, re-runnable" skill again the next day, which is the whole point per the SKILL.md's own framing ("the need recurs") — the sqlite3 CLI doesn't abort on the CREATE TABLE error by default; it prints the error and keeps going, so the INSERTs re-run and silently duplicate every request/PII/audit row.

That's exactly gap #4 from this PR's own retro (fresh-vs-persistent DB, "a re-run errors or wipes real data"), and the plan doc's Learn section claims it was fixed "everywhere the seed command appears" — but that fix only landed as guidance in skills/tinkerdown-save/SKILL.md for future captures, not in the concrete pii-access-approval artifact itself. TestCapturedSkillsWellFormed only checks frontmatter + referenced-path existence, so nothing catches this. Worth adding a "delete data/ before re-seeding" note, or making seed.sql idempotent, so the worked example practices what the meta-skill preaches.

3. Minor: duplicated "Run it" shell block

skills/pii-access-approval/SKILL.md's "Run it" section repeats the exact command block from examples/pii-access-approval/README.md verbatim instead of linking to it (the "How it works" section right below it does link out). Given this PR's stated principle of pointing-not-copying to avoid drift, having the same shell commands live in two files is a small instance of the pattern it otherwise avoids — low risk since it's just a command block, but if the stand-up steps ever change they now need updating in two places.

Nit

captured_skills_test.go's frontmatter check (strings.Contains(cs, "triggers:")) is a substring check, not real YAML parsing — fine for this purpose, just noting it wouldn't catch "triggers:" appearing only in prose.

Nothing here blocks merging — #1 and #2 are the ones I'd actually fix; #3 and the nit are optional polish.

[adnaan]

All three fixed.

2 (re-run safety — the important one). You're right, and it's a sharp catch of my own inconsistency: I added the fresh-vs-persistent guidance to /tinkerdown:save for future captures but left the concrete seed.sql on plain CREATE TABLE + inserts, so re-running the "re-runnable" skill would silently duplicate every request/PII/audit row. Fixed by making each table DROP … IF EXISTS then CREATE, so re-seeding resets to a clean demo state — verified re-runnable (seeded twice, stable row counts: access_requests 3→3, orders_pii 8→8, no error, no duplicates). The plan's "fixed everywhere" claim was about the sqlite3-portability, not idempotency; corrected in the Learn.

1 (heading hierarchy). Reordered ai-generation.md so the "Example: Creating an App" H3 stays under "Using Claude Code Skills"; the new "Ephemeral by default" H2 now follows it.

3 (duplicated Run-it block). Good point given this PR's own pointing-not-copying principle. The pii skill's "Run it" now points at the README's Run-it steps (single source) rather than repeating the command block — the one place they live.

The frontmatter substring-check nit I've left as-is (sufficient for presence).

Full suite green (17 ok packages).

[claude]

Review

Reviewed the diff, the plan-doc history, and cross-checked every referenced path and link. This is well executed, and the two review rounds already visible in the commit history (idempotent seed, heading fix, point-not-copy) already resolved the issues I would otherwise flag. A few notes below.

Design

• The point-vs-bundle rule (skills/pii-access-approval/ references examples/pii-access-approval/ instead of copying it) is the right call: it avoids the byte-match-guard anti-pattern the PR description calls out, and captured_skills_test.go is exactly the right guard to keep the pointer honest if the example ever moves.
• The blind-agent verification of /tinkerdown:save (never shown the golden PII skill) is a good practice here: the 4 gaps it surfaced (portable seed command, implicit seeding step, --operator/--allow-exec criteria, fresh-vs-persistent DB) are all real operational traps, not markup nits.
• seed.sql, DROP TABLE IF EXISTS then CREATE, makes the demo genuinely re-runnable (stable row counts across reseeds). Note this also wipes audit_log on every reseed, which is fine for a demo that resets to a clean state each run (documented in the file), but if this pattern is ever copied into a real recurring-workflow capture, the tinkerdown-save guidance (idempotent CREATE TABLE IF NOT EXISTS + seed-only-if-absent) is the one that should apply there instead. Not a blocker, just flagging for future captures.

Verified

• All repo-relative paths referenced by both new SKILL.md files and by captured_skills_test.go exist: examples/pii-access-approval/tinkerdown.yaml, app.md, seed.sql, README.md, and skills/pii-access-approval.
• Markdown anchor links resolve correctly: docs/reference/config.md#the-generation-block and examples/pii-access-approval/README.md#run-it both match real headings in those files.
• The ai-generation.md heading-hierarchy fix correctly leaves "Example: Creating an App with Natural Language" (H3) nested under "Using Claude Code Skills" (H2), no longer orphaned under the new "Ephemeral by default" H2.
• --operator and --allow-exec flags referenced in the new skill docs match real CLI behavior (internal/source/source.go, internal/runtime/actions.go).
• Seed data is all synthetic (*.test TLD, fictional names) with no real PII and no injected secrets.

Minor / non-blocking

• captured_skills_test.go frontmatter check is a plain strings.Contains for name:/description:/triggers: anywhere in the file, not scoped to the frontmatter block, so a false positive could slip through if one of those words appeared in body prose. Already called out in the PR description as an accepted nit from the prior review round, so no action needed unless it gets tightened later (e.g. scanning only between the first two --- lines).
• No new chromedp e2e for the re-run-from-skill path. Reasonable, since the pointer skill serves the exact same examples/pii-access-approval/ artifacts that Phase 4 e2e already exercises end to end, so an additional run would be redundant coverage.

Overall nice work: the design rationale is unusually well documented in both the PR body and the plan doc Learn section, which made this an easy review.

[adnaan]

Thanks — treating this as converged. On the audit_log-wipe-on-reseed note: agreed, and it's the intended split. This example is a demo (fresh, self-contained state each run, documented in seed.sql), so DROP-and-recreate is correct here; a persistent captured workflow is exactly the case /tinkerdown:save steers to CREATE TABLE IF NOT EXISTS + seed-only-if-absent instead. Keeping the two models distinct on purpose. Appreciate the thorough path/anchor/flag verification.