5 Commits

Author	SHA1	Message	Date
Michael Ramos	7ad4d39ed9	feat(comments): reference agent skills with / or $ in plan review and annotate comments (#1229) ... * feat(comments): reference agent skills with / or $ in plan and annotate comments Typing / or $ at the start of a word in the document-UI comment composer opens a picker of the user's global agent skills (~/.claude/skills, ~/.codex/skills, ~/.agents skills roots), served by a new GET /api/skills on the plan and annotate servers in both runtimes (Bun + Pi mirror). Multiple references per comment are supported; references live in the comment text itself and are appended to exported feedback as a 'Skills referenced' block so the acting agent knows which skills to apply. Human-invocation-only skills (disable-model-invocation: true frontmatter) stay listed and selectable but render dimmed with a badge, warn in the menu and composer, and are marked in the export so the agent is never asked to invoke something it cannot. Discovery reuses the review-skill loader (same roots, precedence, and skip-and-log discipline), reads only an 8KB head per SKILL.md, caps the catalog at 500 skills, takes no client input, and is never persisted; any failure degrades to plain typing. * fix(comments): harden skill references per review (trigger, IME, seam, fail-closed frontmatter) Blockers: - B1: a trigger now requires at least one query character. A bare / or $ no longer opens the catalog, so Enter stays a newline and Tab still leaves the field ("This costs $" + Enter, "cd /" + Tab, bullets). - B2: the menu ignores keys mid-IME-composition (nativeEvent.isComposing), matching the 16 existing guards; Enter committing a Pinyin/Telex/Korean candidate can no longer insert a skill. - B3: the catalog request is a host seam (skillCatalogTransport via configurePlannotatorUI), defaulting to the existing GET /api/skills. - B4: resetSkillCatalogCache() invalidates outstanding requests (generation counter), and a late-resolving stale request can no longer overwrite a newer cached value or the export registry. The catalog tests reset in beforeEach, so they hold in any file order. Also: - F1: skillReferences={false} is fully inert — the human-only notice memo and the cache seed are gated on the prop. - F4: frontmatter flag parsing no longer fails open: trailing YAML comments are stripped, on/1 (and TRUE/yes etc.) read as true, the head read is 64KB, and truncated unterminated frontmatter fails CLOSED on disable-model-invocation. - F5: extraction ignores markdown link destinations ([x](/name)), shell redirects (cat /x > out), and /-triggered FHS root names (/run, /tmp); menu insertion switches / to $ for those names so inserted references always survive extraction. - F6: the 500-skill cap slices after sorting, so which skills survive no longer depends on readdir order. - F3: /api/skills wiring guards for the Bun and Pi plan + annotate servers (skills-endpoint.test.ts). - Keyboard state machine tests against the real CommentPopover in happy-dom (bare trigger, insertion, composition, Escape, highlight bounding, opt-out inertness), added to the CI DOM step. - The insertion path dismisses the trigger start so the menu close is ordering-safe against React's select-plugin re-reading a stale caret. * feat(comments): redesign the skill reference menu (bare triggers, no preselection, highlighted tokens) Per maintainer direction, reversing the earlier bare-trigger opt-out deliberately: typing a bare / or $ at the start of a word now opens the full skill catalog immediately, and the safety story moves from the trigger to the menu itself. No preselection (the load-bearing rule): the menu opens with NO row active, and while nothing is active every key behaves exactly as if the menu were closed. "This costs $" + Enter is a newline; "cd /" + Tab leaves the field (the proven regression that must never return). A row activates only via ArrowDown/ArrowUp (Down from none lands on the first row, Up on the last); only then do Enter/Tab insert. Pointer hover never activates a row, because the menu floats exactly where the mouse rests over the composer; a click inserts directly and never arms Enter. Continuing to type re-filters and disarms any active row. Escape clears the active row and dismisses when the user engaged (query typed or row active); an unengaged bare-trigger menu passes Escape through so closing the composer still costs one press. Menu redesign to the reference look: icon, bold name, dimmed inline description with ellipsis, right-aligned source column (Agents / Claude / Codex from the discovery roots), rounded generously padded rows, and a subtle active-row background; human-only rows stay dimmed with their badge and the warning now shows while such a row is ACTIVE. Inserted references render highlighted in the composer via a mirrored aria-hidden overlay behind a transparent-text textarea (identical font, padding and wrapping metrics; scroll synced; tokens change color and background only, drawn from the --primary theme token so every palette works in light and dark). The caret keeps --foreground, selection uses a translucent primary wash, and IME composition temporarily restores native textarea text so composition underlines render normally. skillReferences={false} still renders the plain pre-feature textarea. Also, per review: - extraction: dropped the over-broad shell-redirect exclusion (false negatives on prose like "use /animate <- this one"; the motivating case stays covered by the reserved-path rule) - frontmatter: an unterminated frontmatter block now fails CLOSED on disable-model-invocation even in complete (untruncated) files - the reserved-path / to $ insertion switch stays: extraction still reads /run as a path, and the new token highlight makes the switch self-explanatory (an unhighlighted insert would look broken) The composition guard, transport seam, catalog generation counter, enabled gating, and export rules are unchanged and re-covered by the rewritten DOM test matrix. * fix(comments): give the skill reference menu adaptive, viewport-clamped placement The menu rendered bottom-full with a fixed max-h-64: always upward, up to 256px, with no viewport awareness. With the comment popover near the top of the viewport (annotating near the top of a document), typing a trigger ran the menu off the top of the screen with its upper rows unreachable. Placement now mirrors the popover's own computePosition idiom: measure the space above and below the composer wrapper against window.innerHeight, prefer above (the shipped direction; keeps the action row and human-only notice visible), flip below when the list fits below but not above, and when neither side fits pick the roomier side. The list's max height is clamped to the available space (still capped at the former 256px), so the menu never extends past a viewport edge. Recomputes on every commit (drag moves, popover flips, filtering changing the item count, warning-footer toggles) plus capture-phase scroll and resize listeners, matching the popover's tracking. Visual design of the menu and rows is unchanged. * feat(comments): inject human-only skill instructions into exported feedback A human-only skill (disable-model-invocation: true) referenced in a review comment used to export as a dead name the agent could do nothing with. A human referencing a human-only skill IS the human invocation, so the export now injects the skill's SKILL.md body verbatim (frontmatter stripped) inside clearly delimited BEGIN/END SKILL INSTRUCTIONS markers, with the absolute skill directory and the resolve-relative-paths pointer so references/, scripts/, and assets/ stay actionable. Model-invocable skills keep exporting as names the agent can invoke itself. Transport is lazy: a new GET /api/skills/content?name= endpoint (Bun and Pi) serves one discovered skill's body, capped at 20k chars with an explicit truncation notice pointing at the file; the client fetches contents only for the human-only skills actually referenced, keyed off comment state, and the catalog now carries each skill's absolute dir so every failure path (deleted skill, unreadable file, race with submit) degrades to naming the skill plus its directory. Names are matched against discovery only and never used as paths, so traversal cannot escape the skill roots. A per-export dedupe injects each skill once even when several comments reference it, and GLOBAL_COMMENT annotations run through the same block. The referenced-skills header now says the reviewer is asking for the invocation, and the human-only menu footer and composer notice explain that the skill's instructions will be included with the feedback instead of warning that the reference will not work. * polish(comments): quiet, progressive human-only skill treatment The human-only surfaces shipped with too much emphasis: a dimmed row plus a bordered uppercase badge, an amber warning footer, and a persistent amber notice in the composer after insertion. Human-only is a property of a skill, not an error state, so the treatment is now quiet and progressively disclosed: - Menu rows render at full strength with a small muted 'human-only' pill (bg-muted / muted-foreground tokens; no border, no dimming). - The plain-language explanation (a model cannot invoke it, so its instructions will be included with your feedback) appears as a muted footer only while a human-only row is active (keyboard) or hovered (pointer). Hover disclosure is purely visual state local to the menu; it never touches activeIndex, so the no-preselection invariant and the hover-never-arms-Enter rule are unchanged and re-asserted by a new test. - When not disclosed, the same sentence stays in the DOM sr-only and human-only rows point at it with aria-describedby, so the state reaches assistive tech as text rather than as a purely visual badge (this does not attempt the #1233 combobox semantics, and does not worsen them). - After insertion, the highlighted token itself carries the quiet inline marker (a dotted primary underline; text-decoration cannot move glyphs, so overlay alignment is untouched) and the standing amber notice is replaced by a native <details> disclosure: a single muted 'Includes skill instructions' summary line that expands to the full accurate sentence, operable by pointer, keyboard, and AT alike. No amber remains; every color is a theme token (muted, muted-foreground, border, primary, ring), so the treatment follows every palette in light and dark. Copy is unchanged where it was accurate. Behavior is unchanged: human-only skills stay selectable and injection still happens. * fix(comments): harden human-only skill injection per adversarial review Three findings on the injection path, each with tests that fail pre-fix: 1. Marker forgery: an injected SKILL.md body containing our own `--- BEGIN/END SKILL INSTRUCTIONS ---` markers (or an `[Instructions truncated:` notice) could close the block early — making everything after it read as the reviewer's own words — forge a block for a skill nobody referenced, or forge a truncation notice pointing at an attacker-chosen path. Body lines matching the structural marker forms (leading-whitespace and case variants included) are now visibly neutralized before injection: kept verbatim but prefixed, never silently deleted (neutralizeSkillMarkerLines). 2. Forged human invocation: POST /api/external-annotations is unauthenticated on localhost, so any local process could submit a comment referencing a human-only skill and cause its instructions to be injected "at the reviewer's request". Annotations carrying a `source` now still LIST their skill references but never cause verbatim injection — human-only references fall back to naming the skill plus its directory, with an honest reason. The content-prime effect skips external texts for the same reason. A human referencing a human-only skill IS the human invocation; a tool is not. 3. Unbounded read: readReferenceSkillContent read the whole SKILL.md before slicing to the 20k cap, so an unauthenticated no-cors fetch loop could balloon RSS by file size per request (measured +64.4MB for a 64MB file). It now uses the same bounded readFileHead as the catalog, reading only frontmatter allowance + 4 bytes per capped char + slack; truncation detection is unchanged for any file whose frontmatter fits the catalog bound, and frontmatter that overflows the read falls back to null rather than serving raw YAML. Measured: 12 reads of a 64MB SKILL.md now cost +5.1MB total. Also: the fast-fail guard no longer rejects legitimately discovered names — `name.includes("..")` 404'd a real `v1..2` skill dir forever (and `\` is legal in POSIX names) while defending nothing, since the name is only ever matched against discovery output and never joined into a path. It now rejects exactly the names that can never be a readdir entry: empty, `.`, `..`.	2026-08-07 09:50:42 -07:00
Michael Ramos	463f6ed57c	fix(ui): fall back to legacy copy in insecure browser contexts (#1174) ... * fix(ui): fall back to legacy copy in insecure browser contexts navigator.clipboard only exists in secure contexts. Remote mode serves plain HTTP on a non-localhost host, so every bare navigator.clipboard.writeText call threw TypeError and copy buttons silently broke. Add copyTextToClipboard(text): Promise<boolean> to packages/ui/utils/clipboard.ts: it tries the async Clipboard API (guarded against synchronous throws), falls back to the existing copy-event plus execCommand path, reports success as a boolean, and never throws. copyTextWithFallback now returns whether the copy happened and accepts an optional focusOwner; copyTextPreservingFocus keeps its exported signature and behavior unchanged. Route all bare call sites through the helper, preserving each site's UX: Copied states only flip on success, error toasts and console errors remain for the failure case, fire-and-forget sites stay fire-and-forget. GoalSetupSurface gains the fallback and keeps its error surface for the all-strategies-failed case. Add DOM-gated unit tests for the helper and register them in CI. Closes #1173 * fix(ui): address clipboard fallback review findings Review follow-ups for the insecure-context clipboard fallback: 1. Tag the fallback textarea with data-clipboard-fallback and whitelist it in PopoutDialog's ANNOTATION_SELECTORS so the transient focus shift during a fallback copy no longer closes popout dialogs (TablePopout copy buttons, CodeFilePopout copy contents). 2. Widen AnnotationPanel's onQuickCopy prop to Promise<void | boolean>. A false resolution now suppresses the Copied flash; void resolution stays success so existing hosts keep today's behavior. The editor quick-copy site returns the helper's boolean. 3. In copyTextWithFallback, only flag the copy-event path as success when clipboardData was present and setData actually ran, and only when execCommand also reported success. A null clipboardData no longer calls preventDefault, so the textarea retry still runs. 4. Add tests pinning that the fallback runs synchronously when navigator.clipboard is absent (execCommand fires before the call returns, keeping it inside the user-gesture window), that a copy event without clipboardData is not treated as success, and that the fallback textarea carries the PopoutDialog focus-out marker and is removed after the copy resolves. 5. GoalSetupSurface surfaces the real writeText rejection message when the Clipboard API exists but fails and the fallback also fails; the generic unavailable message is reserved for the API-absent case.	2026-08-03 10:14:34 -07:00
Michael Ramos	36948e8572	chore: bump version to 0.19.11 ... For provenance purposes, this commit was AI assisted.	2026-05-07 23:24:00 -04:00
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
Michael Ramos	a11bf802bd	feat(ui): code file viewer with syntax highlighting and annotations (#634) ... * feat(ui): extract reusable PopoutDialog, fix backdrop blur The table popout lost its backdrop blur when we switched to modal={false} to keep annotation toolbars interactive. Radix ignores Dialog.Overlay in non-modal mode, so replace it with a plain div backdrop that works regardless. Extract the dialog shell (backdrop, close button, portal, annotation-aware dismiss) into a reusable PopoutDialog component for upcoming use cases. Add a demo table to the dev plan content. * Add read-only code file popout * Add code file annotation support * Fix code selection popover position * fix(editor): restore global-attachment-only drafts The save condition was broadened to persist drafts with only global attachments, but the restore handler still skipped applying when both annotation arrays were empty — silently dropping the attachments. * fix(ui): import SelectedLineRange from @pierre/diffs base package SelectedLineRange is not re-exported from @pierre/diffs/react — import it from the base @pierre/diffs entry point instead. * chore: add TODO for bot callback + code annotation limitation	2026-04-30 10:42:36 -07:00