Files
Michael Ramos 84a0b434f9 fix(ui): smart resolution + existence-validation for code-file paths (#654)
* fix(ui): smart resolution + existence-validation for code-file paths

The bare-prose / backtick path detector linkifies anything that looks
like a code path. Two failure modes regularly produce dead links: prose
abbreviations like `editor/App.tsx` (real file is
`packages/editor/App.tsx`) and references to files the plan proposes
but hasn't created yet. Both 404 on click with no UX cue.

Resolves abbreviated paths via a case-insensitive suffix-match against
a cached project walk (`resolveCodeFile` in `packages/shared/resolve-file.ts`),
mirroring what `resolveMarkdownFile` already does for markdown. The walk
is pre-warmed when the plan/annotate server boots and on every
`/api/doc` request, with a 30s TTL so newly-created files can resolve
mid-review. Storing the walk as a Promise makes the cache race-safe —
concurrent callers piggyback rather than starting a second walk.

A new `POST /api/doc/exists` endpoint takes a batch of candidate paths
and reports `found` / `ambiguous` / `missing` / `unavailable` per path.
On the frontend, `useValidatedCodePaths` extracts candidates from the
markdown on load and POSTs once. The renderer reads the result via
`CodePathValidationContext`: `found` opens directly with the resolved
absolute path, `ambiguous` opens a `CodeFilePicker` popover listing all
matches (common in monorepos where `App.tsx` exists in several
packages), `missing` demotes the link to plain code, and `unavailable`
falls back to the optimistic linkification we have today. While
validation is in flight, every detected path renders as a link, so
first paint is unchanged.

The detection itself gets a shape filter (`isPlausibleCodeFilePath`)
that hard-rejects shell brace expansion (`{a,b}`), glob wildcards, and
whitespace, while explicitly allowing `[` / `]` so Next.js dynamic
routes (`app/[slug]/page.tsx`) still resolve. The bare-prose regex moves
out of `InlineMarkdown.tsx` into `code-file.ts` so the renderer and the
new server-side extractor use the same source of truth, and the
extractor strips fenced code blocks, HTML comments, and URL ranges
before scanning so it only emits candidates the renderer would actually
paint.

Pi extension mirrors the Bun changes (handler upgrade, pre-warm,
`/api/doc/exists` route). When the popout's `/api/doc` request 404s the
dialog now surfaces "File not found in repo: <path>" instead of
silently swallowing the error.

Tests: `code-file.test.ts` extended with shape-filter and Next.js-route
cases; new `extract-code-paths.test.ts` covers extraction, dedup,
fenced/HTML/URL exclusion, and the URL-with-parens regression; new
`resolve-file.test.ts` covers the suffix-match strategy, leading `./`
handling, ambiguous results, and ignored-dir behavior.

* fix(ui): thread doc-base through code-path validator

Out-of-tree linked docs (and annotate-mode files outside cwd) reference
files relative to themselves. The validator was resolving against cwd
only, so those paths got marked missing and the renderer demoted them
to plain text — even though clicks still resolved correctly via base.

Also tightens the suffix-match's leading-segment strip so `../foo.ts`
no longer silently misresolves to an unrelated `foo.ts` in cwd.

Cleanup: delete unused extract-code-paths import in reference-handlers,
add the export entry to packages/shared so consumers don't rely on
Bun's lenient subpath resolution. Add TODO(security) comments at both
handleDocExists sites flagging that absolute paths bypass project-root
containment.

223 tests pass (3 new resolver cases for baseDir + ../ regression).

* refactor(editor): dedupe activeDocBaseDir; expand security TODO

Self-review fallout:

1. The doc-base expression `linkedDocHook.filepath ? dirname(...) :
   imageBaseDir` lived in two places (click-time URL builder and Viewer
   prop). If they drift, validator and click resolve against different
   bases and we silently re-introduce the demote-correct-link bug.
   Extract to a single useMemo.

2. The handleDocExists security TODO mentioned absolute paths in
   `paths[]` but I just added `base` acceptance, which has the same
   shape of leak (hostile sender supplies base=/secret/dir + relative
   path). Both vectors flagged in one TODO, mirrored Bun + Pi.

223 tests pass; both builds clean.

* fix(ui): code-file popout shows real error; misc consistency

Review fallout:

- `CodeFilePopout` hardcoded "File not found in repo" regardless of
  cause. The hook already captures the server's error string, so an
  ambiguous-path 400 (which can happen if a user clicks an optimistic
  link before validation completes) was surfacing as a misleading
  not-found message. Render the actual `error` and only show the
  planned/future-file caveat when the error matches "file not found".
- `InlineMarkdown` emitted demoted bare-prose paths as raw strings
  while every other plain-text branch in `emitPlainTextWithBareUrls`
  routes through `transformPlainText`. Cosmetic-only today since
  paths rarely contain transformable content, but the divergence
  invites copy-paste rot. Routed through the same helper.
- CLAUDE.md missed the new POST /api/doc/exists endpoint in both
  Plan Server and Annotate Server tables. Added.

223 tests pass; both builds clean.

* fix(ui): demote paths the extractor excluded from validation

When the validator is ready but a candidate path has no entry in the
validated map, the extractor intentionally excluded it — e.g. inside
an HTML comment or fenced code block. The renderer was optimistically
linking these because gateCodePath returned 'link' for missing entries.

Found during manual testing: `<!-- packages/editor/App.tsx -->` inside
a paragraph (parser doesn't recognize HTML comments as block-level)
was rendered as a clickable link. The extractor correctly stripped the
comment, but the renderer's optimistic fallback overrode that.

Also adds manual test harness: tests/manual/path-detection/ with
sandbox setup + three launcher scripts (plan mode, annotate in-tree,
annotate out-of-tree) covering ~30 test cases.

223 tests pass; both builds clean.

* fix(ui): skip HTML comments in InlineMarkdown scanner

The parser doesn't recognize <!-- --> as block-level HTML, so comments
inside paragraphs fall through to InlineMarkdown. The scanner then
finds paths inside the comment text and linkifies them.

The previous gateCodePath fix (demote when not in validated map) didn't
help here because the same path appeared elsewhere in the document —
the map had an entry from the non-comment occurrence.

Fix: match <!-- ... --> at the top of the scanner loop and skip the
entire comment. HTML comments should be invisible per CommonMark spec.

* fix: handle unavailable variant in markdown resolve narrowing

The shared ResolveResult type gained an `unavailable` variant for code
files. The markdown resolver never returns it, but TS can't narrow
past it without an explicit guard. Both Bun and Pi handlers now guard
`not_found || unavailable` before accessing `result.path`.
2026-05-04 14:21:16 -07:00

366 lines
12 KiB
Bash
Executable File

#!/bin/bash
# Build a self-contained temp sandbox for testing code-file path detection.
#
# Usage:
# source ./setup.sh # sets $SANDBOX in your shell
# source ./setup.sh --keep # don't auto-clean on shell exit
#
# What it creates (everything under a single mktemp -d):
#
# $SANDBOX/
# ├── repo/ ← fake project root (git init'd)
# │ ├── packages/
# │ │ ├── editor/App.tsx ← ambiguous basename (1 of 2)
# │ │ ├── review-editor/App.tsx ← ambiguous basename (2 of 2)
# │ │ └── ui/
# │ │ ├── components/Button.tsx ← unique basename
# │ │ └── index.ts
# │ ├── src/
# │ │ ├── utils/helper.ts ← abbreviated path target
# │ │ └── config.json ← non-.ts code file
# │ ├── app/
# │ │ └── [slug]/page.tsx ← Next.js bracket-route
# │ ├── node_modules/
# │ │ └── junk/App.tsx ← should be ignored by walker
# │ └── test-plan.md ← the primary fixture plan
# │
# └── external/ ← out-of-tree (simulates ~/notes/)
# ├── notes.md ← annotate primary (out-of-tree)
# ├── script.ts ← sibling reference from notes.md
# ├── config.yaml ← sibling, different extension
# ├── design.md ← linked doc opened from notes.md
# ├── bar.ts ← sibling reference from design.md
# ├── parent.ts ← target for ../parent.ts from sub/
# └── sub/
# ├── subdoc.md ← references ../parent.ts
# └── nested.ts ← local sibling of subdoc.md
set -e
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
PROJECT_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)"
KEEP=false
for arg in "$@"; do
case $arg in
--keep) KEEP=true ;;
esac
done
SANDBOX=$(mktemp -d "${TMPDIR:-/tmp}/plannotator-pathtest-XXXXXX")
echo "Sandbox: $SANDBOX"
if [ "$KEEP" = false ]; then
trap 'echo "Cleaning up $SANDBOX"; rm -rf "$SANDBOX"' EXIT
else
echo "(--keep: won't auto-clean)"
fi
# ───────────────────────────────────────────────────
# 1. Fake repo with known file tree
# ───────────────────────────────────────────────────
REPO="$SANDBOX/repo"
mkdir -p "$REPO"
cd "$REPO"
git init -q
git config user.email "test@test.com"
git config user.name "Test"
# Ambiguous pair (both named App.tsx)
mkdir -p packages/editor
mkdir -p packages/review-editor
cat > packages/editor/App.tsx << 'TSEOF'
export default function EditorApp() { return <div>Editor</div>; }
TSEOF
cat > packages/review-editor/App.tsx << 'TSEOF'
export default function ReviewApp() { return <div>Review</div>; }
TSEOF
# Unique basenames
mkdir -p packages/ui/components
cat > packages/ui/components/Button.tsx << 'TSEOF'
export const Button = () => <button>Click</button>;
TSEOF
cat > packages/ui/index.ts << 'TSEOF'
export * from './components/Button';
TSEOF
# Abbreviated path targets (src/utils/helper.ts → match "utils/helper.ts")
mkdir -p src/utils
cat > src/utils/helper.ts << 'TSEOF'
export function helper() { return 42; }
TSEOF
# Non-.ts code file to verify resolver handles other extensions
cat > src/config.json << 'JSONEOF'
{ "key": "value" }
JSONEOF
# Next.js bracket-route (shape filter must allow [ and ])
mkdir -p "app/[slug]"
cat > "app/[slug]/page.tsx" << 'TSEOF'
export default function SlugPage() { return <div>Slug</div>; }
TSEOF
# node_modules — walker must skip this
mkdir -p node_modules/junk
cat > node_modules/junk/App.tsx << 'TSEOF'
// should never resolve
TSEOF
# Commit so git is happy
git add -A
git commit -q -m "initial"
# ───────────────────────────────────────────────────
# 2. Out-of-tree fixtures (simulates external docs)
# ───────────────────────────────────────────────────
EXT="$SANDBOX/external"
mkdir -p "$EXT/sub"
cat > "$EXT/script.ts" << 'TSEOF'
export function externalScript() { return "external"; }
TSEOF
cat > "$EXT/config.yaml" << 'YAMLEOF'
key: value
YAMLEOF
cat > "$EXT/bar.ts" << 'TSEOF'
export function bar() { return "bar"; }
TSEOF
cat > "$EXT/parent.ts" << 'TSEOF'
export function parent() { return "parent"; }
TSEOF
cat > "$EXT/sub/nested.ts" << 'TSEOF'
export function nested() { return "nested"; }
TSEOF
# ───────────────────────────────────────────────────
# 3. Test markdown: plan-fixture.md (used by run-plan.sh)
# ───────────────────────────────────────────────────
cat > "$REPO/test-plan.md" << 'MDEOF'
# Path Detection Test Plan
Use this plan to manually verify code-file path detection, smart
resolution, and existence validation. Each section tests a different
scenario. The line below each path says what should happen.
---
## §1 — Full repo paths (should all be clickable links)
A. Backtick, full path: `packages/editor/App.tsx`
→ clickable, opens packages/editor/App.tsx
B. Bare prose, full path: see packages/ui/components/Button.tsx for the button
→ clickable, opens Button.tsx
C. JSON extension: `src/config.json`
→ clickable, opens config.json
## §2 — Abbreviated paths (suffix-walk resolution)
A. Backtick abbreviated: `editor/App.tsx`
→ clickable, opens packages/editor/App.tsx
B. Bare prose abbreviated: see utils/helper.ts for the implementation
→ clickable, opens src/utils/helper.ts
C. Backtick with leading dot-slash: `./editor/App.tsx`
→ clickable, opens packages/editor/App.tsx
## §3 — Bare basename (single match → link, multiple → picker)
A. Unique basename: `Button.tsx`
→ clickable, opens packages/ui/components/Button.tsx
B. Ambiguous basename: `App.tsx`
→ clickable with superscript count badge, click opens picker
listing packages/editor/App.tsx and packages/review-editor/App.tsx
C. Unique non-component: `helper.ts`
→ clickable, opens src/utils/helper.ts
## §4 — Non-existent paths (should demote to plain text)
A. Backtick, missing file: `packages/ui/shortcuts/core.ts`
→ rendered as plain code (not clickable), no shimmer
B. Bare prose, missing file: see packages/ui/shortcuts/runtime.ts for details
→ rendered as plain text (not a link at all)
## §5 — Shape filter (should never be detected as paths)
A. Brace expansion: `packages/ui/{core,runtime}.ts`
→ rendered as plain code, NOT a link
B. Glob wildcard: `packages/ui/*.tsx`
→ rendered as plain code, NOT a link
C. Path with spaces: `some path/with spaces/file.ts`
→ rendered as plain code, NOT a link
## §6 — Bracket routes (shape filter must allow [ and ])
A. Next.js dynamic route: `app/[slug]/page.tsx`
→ clickable, opens app/[slug]/page.tsx
B. Abbreviated bracket route: `[slug]/page.tsx`
→ clickable via suffix walk
## §7 — node_modules exclusion
A. Path inside node_modules: `junk/App.tsx`
→ should be not_found (walker skips node_modules), demoted to plain text
or rendered as ambiguous with only the two real App.tsx files
## §8 — URLs (should not produce path-shaped leaks)
A. URL with .ts extension: see https://github.com/example/bar.ts in the docs
→ URL is a link, no stray "bar.ts" path link appears
B. URL on same line as real path: https://github.com/example.com and editor/App.tsx
→ URL is a URL link, editor/App.tsx is a separate code-file link
C. Wikipedia-style parens: see https://en.wikipedia.org/wiki/Foo_(bar).ts
→ entire URL is one link, no path extracted
## §9 — Fenced code blocks (should NOT detect paths inside)
```ts
import { helper } from 'src/utils/helper.ts';
const x = require('packages/editor/App.tsx');
```
→ No clickable links inside the fenced block above
## §10 — HTML comments (should not detect)
<!-- packages/editor/App.tsx is a placeholder -->
→ Comment content is invisible; no links generated
## §11 — Leading ../ paths
A. Without baseDir context (plan mode, no linked doc):
`../script.ts`
→ demoted to plain text (no baseDir to resolve against)
B. This scenario is tested by opening an out-of-tree linked doc (see §12).
## §12 — Linked doc overlay (baseDir transition)
Click this link to open the external notes. Once inside the overlay,
verify the paths listed in notes.md resolve correctly against their
own directory, not against this repo's cwd.
[Open external notes](EXTERNAL_NOTES_PLACEHOLDER)
After opening, check:
- `script.ts` in the notes should be clickable (resolves to external/script.ts)
- `../parent.ts` referenced from sub/subdoc.md should resolve
- `editor/App.tsx` in the notes should still resolve via cwd suffix-walk
MDEOF
# ───────────────────────────────────────────────────
# 4. Out-of-tree markdown fixtures
# ───────────────────────────────────────────────────
cat > "$EXT/notes.md" << 'MDEOF'
# External Notes
This file lives outside the project repo. References below should
resolve against THIS directory when opened in annotate mode or as
a linked-doc overlay.
## Sibling references (baseDir-literal should hit)
A. Backtick sibling: `script.ts`
→ clickable, opens external/script.ts
B. Bare prose sibling: see config.yaml for the config
→ clickable, opens external/config.yaml
## Relative escape (../ with baseDir)
These only work when baseDir is set (annotate or linked-doc mode):
A. From sub/subdoc.md: [Open subdoc](sub/subdoc.md)
After opening, `../parent.ts` should resolve to external/parent.ts
## Cross-context fallback (baseDir miss → cwd suffix walk)
A. Repo path from outside: `editor/App.tsx`
→ IF opened as linked doc from the repo plan: clickable, resolves
to repo's packages/editor/App.tsx via cwd suffix-walk fallback
→ IF opened standalone via annotate: may not find it (no cwd walk
if repo is not cwd)
## Linked doc from out-of-tree
A. [Open design doc](design.md)
After opening, `bar.ts` should resolve to external/bar.ts
## Missing from here
A. `packages/ui/shortcuts/core.ts`
→ demoted to plain text (doesn't exist anywhere near this file)
MDEOF
cat > "$EXT/design.md" << 'MDEOF'
# Design Doc
Opened as a linked doc from notes.md. baseDir should be external/.
## References
A. Sibling code file: `bar.ts`
→ clickable, opens external/bar.ts
B. Non-existent here: `baz.ts`
→ demoted to plain text
C. Repo file via suffix-walk: `Button.tsx`
→ If opened from repo plan → cwd walk finds packages/ui/components/Button.tsx
→ If opened standalone → demoted (no cwd context)
MDEOF
cat > "$EXT/sub/subdoc.md" << 'MDEOF'
# Sub-document
Opened from notes.md. baseDir should be external/sub/.
## Parent escape
A. `../parent.ts`
→ clickable, opens external/parent.ts (baseDir literal: external/sub/../parent.ts)
## Local sibling
A. `nested.ts`
→ clickable, opens external/sub/nested.ts
## Non-existent
A. `../missing.ts`
→ demoted to plain text (external/missing.ts doesn't exist)
MDEOF
# ───────────────────────────────────────────────────
# 5. Patch the placeholder link in plan-fixture.md
# to point to the actual external notes path
# ───────────────────────────────────────────────────
sed -i '' "s|EXTERNAL_NOTES_PLACEHOLDER|$EXT/notes.md|" "$REPO/test-plan.md"
echo ""
echo "Sandbox ready."
echo " Repo: $REPO"
echo " External: $EXT"
echo ""
echo "Export for launcher scripts:"
echo " export SANDBOX=$SANDBOX"
export SANDBOX