my-pi/pi-permission-system/docs/retro/0288-extract-shared-test-fixtures.md

issue, issue_title

issue	issue_title
288	Extract shared test fixtures to cut permission-system test duplication

Retro: #288 — Extract shared test fixtures to cut permission-system test duplication

Stage: Planning (2026-05-31T00:00:00Z)

Session summary

Produced a numbered migration plan for extracting duplicated test setup in pi-permission-system/test/ into focused test/helpers/ modules. Grounded the clone families in a live fallow dupes run (122 groups, 9.1%) and confirmed the divergent makeCheckResult defaults across gates/runner, gates/bash-path, and tool-call copies. Plan is a pure test refactor (no src/ changes); next step is /build-plan since cycles are migrate → full-suite-green → commit, not red→green.

Observations

• Three user-confirmed design decisions via ask_user: no co-located helper tests (transitive coverage), focused files by concern (mirror pi-subagents/test/helpers/), and a single neutral-default makeCheckResult with explicit per-call overrides.
• The divergent makeCheckResult defaults are the main correctness risk — bash-path uses toolName: "path"/source: "special"/origin: "global"; runner adds matchedPattern: "*". Migration must pass each site's original fields as explicit overrides.
• Watch the testing-skill trap: do not annotate mock-bag factories (makeHandler, makeRunnerDeps) with the production interface, or .mockReturnValue access is erased.
• Keep the regression-guard import in external-directory-integration.test.ts — it intentionally fails the load if a message helper is removed.
• permission-system.test.ts is 2839 lines; only the targeted intra-file createManager/config clones are in scope — leave withIsolatedSubagentEnv and env handling untouched.
• Step 5 (lifecycle setup) and the ext-dir block's final home are flagged as open questions to settle during implementation.
• Initial Write hit an external-directory denial from a wrong absolute path (/Users/chris/development/pi/pi-permission-system/...); the repo root is pi-packages. Use repo-relative paths.

Stage: Implementation — TDD (2026-05-31T14:45:00Z)

Session summary

Completed all 6 migration steps from the plan: handler fixtures (Step 1), external-directory family (Step 2), gate fixtures (Step 3), manager harness (Step 4), lifecycle setup (Step 5), and docs refresh (Step 6). Test count held steady at 71 files / 1628 tests throughout — pure refactor, no assertions changed. Pre-completion reviewer returned WARN (resolved before shipping).

Observations

• Step 1 makeCheckResult signature change required converting positional-state calls in tool-call-events.test.ts to override-bag form with explicit matchedPattern: "*" where the original factory had it as a default. All other files used the neutral default safely.
• Step 2 makeToolCallEvent in external-directory-integration.test.ts used input as a direct second argument (not wrapped); migrated all call sites to { input: {...} } wrapper convention to align with the shared factory. No test failures.
• Step 3 makeCheckResult defaults diverged across runner vs bash-path/path files; gate-fixtures introduces makeGateCheckResult (path defaults) alongside the neutral makeCheckResult from handler-fixtures to avoid verbose per-call overrides in the 20+ bash-path call sites.
• Step 4 CreateManagerOptions was still used in createManagerWithProject in permission-system.test.ts after removing the local definition — needed an explicit import from the harness. The TS2345 error at line 1170 (pre-existing latent type issue) was resolved as a side effect once CreateManagerOptions was properly imported.
• Step 5 makeSession in before-agent-start.test.ts and lifecycle.test.ts have different method sets (different lifecycle phases), so only makeCtx was extracted. The 39-line fallow clone was primarily the makeCtx body.
• WARN 1 resolved: stale PermissionGateHandler import in tool-call.test.ts removed (biome lint warning, exit 0).
• WARN 2 resolved: package-pi-permission-system SKILL.md Testing section updated with test/helpers/ layout and the divergent-default makeCheckResult override pattern.
• Pre-completion reviewer verdict: WARN (2 findings, both resolved before shipping).

Stage: Final Retrospective (2026-05-31T18:56:41Z)

Session summary

Single-session execution of the full lifecycle (plan → TDD → ship → retro) for the test-fixture extraction. All 6 migration steps landed green, duplication dropped 9.1% → 7.1% (clone groups 122 → 113), and pi-permission-system-v8.2.1 released cleanly. The dominant friction was a recurring import-reconciliation slip when removing local factory definitions during migration — three instances, two caught by tsc, one that escaped both green-gates to the pre-completion reviewer.

Observations

What went well

• The upfront ask_user in planning (three design decisions: no co-located helper tests, focused files by concern, single neutral-default makeCheckResult) paid off — zero design churn during implementation across all 6 steps.
• Incremental verification: pnpm run check + vitest run ran after every step (msgs 58, 65, 76, 79, 89, 96, 101), so each commit left the suite green; no broken-baseline commits.
• The makeGateCheckResult decision (Step 3) — introducing a path-surface factory alongside the neutral makeCheckResult rather than forcing four explicit overrides at 20+ bash-path call sites — was a sound mid-implementation judgment that stayed within the plan's intent.
• The pre-completion reviewer was the only safety net that caught the stale PermissionGateHandler import; the deterministic green-gates (check, lint) both passed it.

What caused friction (agent side)

• missing-context — import reconciliation after removing local factory definitions (recurring, 3 instances). In bash-path.test.ts the removed PermissionCheckResult import was still used by the CheckPermissionFn type alias (tsc caught it, msg 76-78); in permission-system.test.ts the removed local CreateManagerOptions was still referenced by createManagerWithProject (tsc caught two errors, msg 89-95); in tool-call.test.ts the now-unused PermissionGateHandler value import was left behind and escaped to the reviewer. Impact: 2 extra edit+recheck cycles (~6 tool calls) plus 1 post-reviewer cleanup commit (67259f66).
• other (tooling) — the multi-block Edit on tool-call.test.ts failed on whitespace matching (msg 41); fell back to full-file Write. A cat -A diagnostic also failed (macOS cat lacks -A). Impact: ~2 extra tool calls; no rework.
• instruction-violation (self-identified) — the first plan-file Write targeted /Users/chris/development/pi/pi-permission-system/..., dropping the pi-packages repo segment, and hit an external-directory denial (msg 18-20). Self-corrected in one retry with a repo-relative path. Impact: 1 wasted Write + 1 diagnostic bash.

What caused friction (user side)

• None substantive. The three Continue. nudges (msgs 64, 70, 74, 121, 127) were mechanical pacing prompts, not redirections — the work was on-track at each.

Diagnostic details

• Model-performance correlation — no mismatches. Planning + retro ran on claude-opus-4-8 (judgment-heavy synthesis), TDD on claude-sonnet-4-6 (mechanical migration), the pre-completion reviewer subagent on anthropic/claude-sonnet-4-6 (judgment-heavy review), and shipping on opencode-go/deepseek-v4-flash (deterministic checklist). Each model was well-matched to task complexity; the cheap flash model on the mechanical ship checklist is appropriate cost optimization, not a mismatch.
• Feedback-loop gap analysis — check and vitest ran per-step, but pnpm run lint ran only at the end (msg 109). The decisive gap: pnpm run lint exits 0 on biome warnings, and an unused value import is a warning — so the stale PermissionGateHandler import passed both pnpm run check (tsc does not flag unused imports without noUnusedLocals) and the pnpm run lint exit code. Only the reviewer's reading of the biome warning output caught it.
• Unused-tool detection — a grep for each removed symbol before deleting its import would have pre-empted all three missing-context instances; grep was available and used elsewhere but not systematically before import removal.

Follow-up proposal

A testing-skill bullet capturing the import-reconciliation step and the biome-warning gotcha was proposed but declined by the user — the observation lives here in the retro only. If the import-reconciliation slip recurs in a future session, revisit promoting it to the testing skill.

Changes made

1. Appended this Final Retrospective stage entry to packages/pi-permission-system/docs/retro/0288-extract-shared-test-fixtures.md. No prompt or AGENTS.md changes — the user chose to record the import-reconciliation observation in the retro only.