my-pi/pi-permission-system/docs/retro/0345-canonicalize-path-containment.md

---
issue: 345
issue_title: "external_directory gate uses lexical path normalization (no symlink resolution) — in-cwd symlink escapes the cwd boundary"
---

# Retro: #345 — Canonicalize paths before the external-directory containment check

## Stage: Planning (2026-06-08T21:59:34Z)

### Session summary

Planned a fix for the lexical-containment flaw in the `external_directory` gate: containment is decided on lexically-normalized paths with no symlink resolution, so an in-cwd symlink escapes cwd (symptom 1) and a symlinked cwd flags its own paths as external (symptom 2).
The plan introduces a best-effort `canonicalizePath` helper (`src/canonicalize-path.ts`) and routes both containment computations — `isPathOutsideWorkingDirectory` (tool-call surface) and `BashProgram.externalPaths` (bash surface) — through it.
Filed at `packages/pi-permission-system/docs/plans/0345-canonicalize-path-containment.md`.

### Observations

- Both reported repros (`cat ./link/hosts`, `/tmp/...`) actually run through `bash` → `BashProgram.externalPaths`, not the tool-call gate; the tool-call gate (`isPathOutsideWorkingDirectory`, used by `read`/`write`/`edit`/`find`/`grep`/`ls`) carries the identical flaw.
  User confirmed fixing both surfaces.
- Issue [#350] already shipped `$HOME` expansion in `normalizePathForComparison`, so the "secondary gap" the issue mentions is already closed — the plan only addresses symlink canonicalization.
- IO approach decided via `ask_user`: direct `fs.realpathSync` in a small isolated module, tested with `vi.mock("node:fs")` (mirroring the existing `node:os` mock in `path-utils.test.ts`), rather than threading a `realpath` dependency through the pipeline.
  User pushed back on DI threading as overkill and was right that vitest can mock the builtin.
- Key safety property: the best-effort walk-up returns the lexical input unchanged when no ancestor exists, so the integration tests that use synthetic non-existent paths (`/test/project`) keep current behavior with no mock and need no edits.
- Kept `normalizePathForComparison` lexical (skill-read / skill-prompt matching is not a security boundary); canonicalization is surgical to the two containment paths.
- Deferred (Non-Goals): the optional path-pattern deny-evasion surface (symlink alias vs `*.env`) and skill-read canonicalization.
- TOCTOU is inherent and accepted — the fix narrows the gap, does not close it.

## Stage: Implementation — TDD (2026-06-08T22:46:22Z)

### Session summary

Completed all 4 TDD cycles: added `src/canonicalize-path.ts` + 8-test suite; switched `isPathOutsideWorkingDirectory` and `describeExternalDirectoryGate` to canonical comparison; canonicalized `BashProgram.externalPaths`; updated architecture docs.
Test count rose from 1858 to 1873 (+15) across 91 test files.
Pre-completion reviewer returned PASS.

### Observations

- **Loop form deviation:** The plan used `while (true)` but `@typescript-eslint/no-unnecessary-condition` rejected it.
  During the user review pause, refactored from a `for (;;)` walk-up to a split-based `for (let i = parts.length; i >= 0; i--)` loop — explicit bound, no `toReversed()`, no root-detection heuristic.
  Cleaner and correct.
- **Critical bash classifier discovery:** The plan's bash symlink-escape test used `cat ./link/hosts`.
  `classifyTokenAsPathCandidate` only accepts absolute, `~/`-relative, and `..`-traversal tokens — it rejects `./relative` paths entirely, so the bash external-directory gate never processes them.
  The correct test surface is the absolute form `cat /projects/my-app/link/hosts`.
  Noted in commit body.
  This means `cat ./link/hosts` is not fixed by canonicalization; it is a separate classifier-scope gap.
- **macOS platform hazard:** `test/bash-external-directory.test.ts` (top-level integration suite) uses real paths like `/etc/hosts`.
  On macOS, `/etc -> /private/etc`, so `realpathSync("/etc/hosts")` returns `/private/etc/hosts`, breaking all expected-value literals.
  Added an identity `node:fs` mock to the file — not anticipated in the plan.
  Any test file importing `bash-program.ts` transitively needs this mock after canonicalization was added.
- **WARN from reviewer:** `canonicalNormalizePathForComparison` reads `process.platform` directly (consistent with pre-existing `normalizePathForComparison` pattern); not a blocker.
- Pre-completion reviewer verdict: PASS.

## Stage: Final Retrospective (2026-06-08T23:03:19Z)

### Session summary

A single continuous session carried issue #345 through plan → TDD → ship for `pi-permission-system`, releasing `v10.6.0`.
The change adds a best-effort `canonicalizePath` helper and routes both containment checks (`isPathOutsideWorkingDirectory`, `BashProgram.externalPaths`) through symlink resolution.
Execution was clean overall (pre-completion PASS, CI green, release merged); the two friction points were a planning assumption invalidated at TDD time and one careless edit.

### Observations

#### What went well

- **User mid-TDD redirect produced a better design.**
  The user's question “Was that infinite `for` loop safe?
  Was there something better?”
  converted a `for (;;)` walk-up into a split-based `for (let i = parts.length; i >= 0; i--)` loop with an explicit bound and no `toReversed()`.
  A strategic-judgment nudge at exactly the right moment, not mechanical oversight.
- **`ask_user` at planning settled the IO approach cheaply.**
  The DI-threading-vs-direct-`fs.realpathSync` question surfaced the simpler answer (direct call + `vi.mock("node:fs")`) before any code was written.
- **Verification cadence caught the macOS hazard at the right step.**
  Running the full suite after TDD step 3 (not deferred to the end) surfaced the 45 `test/bash-external-directory.test.ts` failures from `/etc -> /private/etc` immediately, while the bash change was still fresh.

#### What caused friction (agent side)

- `missing-context` — the plan built the bash test around the issue's headline repro `cat ./link/hosts` without tracing that token through `classifyTokenAsPathCandidate`, which rejects `./`-relative paths so they never reach the `external_directory` gate.
  Surfaced only at TDD time as `externalPaths()` returning `[]`.
  Impact: ~4 tool calls debugging (a failed `node --input-type` TS-param-property attempt, a failed `jiti` module-resolution attempt) before reading the classifier source; a test rewrite to the absolute form `cat /cwd/link/hosts`; and the realization that the issue's literal repro is **not** fixed by this change (separate classifier-scope gap).
- `other` (careless edit) — the first `Edit` adding the identity `node:fs` mock to `test/bash-external-directory.test.ts` left a duplicate `vi.mock("node:os")` sentinel block in `newText`.
  Impact: one extra fix-up edit; caught immediately by re-reading the file.
  No rework beyond the follow-up edit.
- `instruction-violation` (self-unidentified) — the `/plan-issue` template asked to load the `colgrep` and `design-review` skills before/while exploring; neither was loaded.
  Impact: none observable — exploration via `grep`/`read` was sufficient for a localized bug fix.
  Noted for completeness, no rule change warranted.

#### What caused friction (user side)

- None.
  The one user intervention (the `for`-loop question) was a net positive and is recorded under wins.
  Opportunity, not criticism: the classifier-scope gap could have been surfaced at planning if the plan had traced the repro input through the tokenizer — a process fix, not a user-context gap.

### Diagnostic details

- **Model-performance correlation** — one subagent dispatched (`pre-completion-reviewer`, agent-default model, 410s / 26 tool uses).
  Judgment-heavy review work on a review-tuned agent; appropriate, no mismatch.
- **Escalation-delay tracking** — the `./link/hosts` debug ran ~4 consecutive tool calls before reading the classifier source: under the 5-call threshold, but the two doomed out-of-band execution attempts (`node`, `jiti`) would have been skipped by reading the source first.
- **Feedback-loop gap analysis** — no gap.
  `pnpm run check` ran after the shared-function change (step 2); the full suite ran after step 3 and caught the macOS failures; lint/fallow ran at ship.
  This is the prescribed incremental cadence.

### Changes made

1. `.pi/skills/package-pi-permission-system/SKILL.md` — added a “Notes for Agents” note documenting the bash path-candidate classifier scope (`classifyTokenAsPathCandidate` vs `classifyTokenAsRuleCandidate`) and the instruction to trace a bash repro token through the classifier before building a plan/test around it.
2. `.pi/skills/code-design/SKILL.md` — added an “Unbounded loops” structural-design heuristic: a `no-unnecessary-condition` flag on `while (true)` signals an unbounded loop to bound over a known sequence, not to dodge with `for (;;)`.
   (Reframed from the original “linter conflict → `for (;;)`” proposal after the user noted the infinite loop was the real smell.)
3. `packages/pi-permission-system/docs/retro/0345-canonicalize-path-containment.md` — this Final Retrospective stage entry.