mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
fix/opencode-serve-leak
1099 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Ramos
|
5f33b72b2f |
feat(remote): tailnet auto-advertise, ready QR code, and a first-class --tailscale mode (#1280)
* feat(remote): resolve urlHost auto from Tailscale for advertised URLs PLANNOTATOR_URL_HOST=auto (or config urlHost: "auto") detects this machine's tailnet host at first use in a remote session: MagicDNS name from tailscale status --json, falling back to the single tailscale ip -4 CGNAT address. Detection is cached per process, never spawns in local sessions, warns once and falls back to localhost on failure, and stays strictly display-only: binding remains governed by PLANNOTATOR_REMOTE. Pure parsers live in the new @plannotator/shared/tailscale module, vendored to the Pi extension; both runtimes mirror the resolution. * feat(remote): render a terminal QR code for remote-ready session URLs Remote sessions print their advertised URL as the lifeline; the usual next step is opening it on another device (iPad, phone, laptop off the VPS). handleServerReady now also renders a compact unicode QR of that URL via the zero-dependency uqr package, TTY-gated so piped stderr and hook transcripts keep only the plain URL line. Pi keeps URL-only parity: its ready surface is an in-chat notification, not a TTY stream, so a QR block would not render there. * feat(cli): first-class --tailscale mode for review and annotate sessions plannotator review --tailscale (also annotate and annotate-last/last) publishes the session over the user's tailnet: the server stays loopback-bound and the CLI orchestrates tailscale serve --bg --https=<port> http://127.0.0.1:<port>, then advertises the HTTPS tailnet URL with a terminal QR code. Nothing listens beyond localhost and nothing is ever public (serve, never funnel). Guarantees: preconditions fail with actionable errors (CLI missing, daemon down or logged out); a pre-existing serve mapping on the chosen port aborts instead of being stolen and other ports are never touched; every mapping the process creates is torn down on normal completion, SIGINT/SIGTERM, and errors via the exit-routed cleanup handler. When combined with PLANNOTATOR_REMOTE or SSH detection, --tailscale wins and forces local mode with a stderr notice, which also restores the random local port so simultaneous sessions get distinct serve mappings. * fix(remote): await tailscale-ready failures, harden serve teardown and conflict detection Review fixes for #1280 (external review plus internal security review). Startup failures no longer hang the session: startReviewServer and startAnnotateServer now await async ready handlers and stop the server on rejection, and the CLI's --tailscale ready path resolves publishing failures itself with an actionable stderr message and exit 1. Under the bang-prefix skill a hanging loopback server blocked the whole Claude Code prompt. Serve teardown is checked, not assumed: a failed off retries once, then warns with the exact manual command, and a port is only forgotten after a successful off. SIGHUP (terminal close) is now routed through process.exit like SIGINT/SIGTERM so exit-time cleanup runs. Docs no longer claim guaranteed cleanup: --bg mappings survive SIGKILL and reboots, and the manual removal command is documented. Conflict detection sees foreground serve sessions (Foreground.*.TCP), which Tailscale prefers over background mappings, and fails CLOSED on unrecognizable serve status output instead of assuming the port is free. The extracted serve URL must match the requested port, so a version-dependent output shape cannot advertise another mapping's URL. The annotate agent terminal is gated off by default under --tailscale behind the existing PLANNOTATOR_AGENT_TERMINAL_REMOTE opt-in: the PTY token is not an auth boundary against network peers, and tailnet reachability implies terminal reachability. Also: --tailscale is rejected with a clear error on unsupported subcommands and documented in review/annotate/annotate-last and top-level help; the remote-ready QR renders only for URLs actually reachable off-machine (never localhost); urlHost is suppressed for --tailscale runs so the local-session warning cannot mislead; the duplicated auto-host resolution moved into the shared vendored module; tailscale-serve tests restore module and process state via a reset seam. |
||
|
Michael Ramos
|
1bf90a2357 |
feat(editor): focus-mode shortcut to toggle both sidebars (#1279)
* feat(editor): focus-mode shortcut to toggle both sidebars (#1276) Keyboard-first reviewers had to reach for the mouse to collapse the Contents sidebar and the right-hand panel every time they wanted to read a document. Bind that to one key. Mod+. now toggles the existing `focus` view mode from the keyboard, on both the plan review and annotate surfaces. The first press remembers whatever was open and closes both panels; the next press restores that exact arrangement, so a session with only one panel open comes back the same way. Any manual open (a sidebar tab, the annotation panel, the agent terminal) already exits the view mode and clears the memory, so the shortcut cannot leave the layout stuck. The binding was picked after auditing every plan-review and annotate binding: Mod+B / Mod+Shift+B are the sidebar toggles, Mod+S, Mod+P, Mod+Enter, Mod+C and Mod+Z are taken, and code review already uses Mod+. for its own "collapse the chrome" toggle. It is unshifted on every keyboard layout and claimed by no browser or OS default. The dispatcher gate is shared with the annotate sidebar shortcuts, so the key is inert while a dialog, an overlay, a submission, or a text field owns the keystroke. HTML surfaces are excluded because they own their own persisted chrome and never render the Focus control. * fix(editor): keep focus-mode exit reachable on HTML surfaces; note the HTML caveat in help |
||
|
Michael Ramos
|
4d2dc5772e |
fix(review): restore the prior diff when leaving the Commits view (#1278)
* fix(review): restore the prior diff when leaving the Commits view Clicking a commit in the Commits panel (or its HEAD auto-select) switches the single session-global diff to commit:<sha>, and switching the panel back to Tree never switched it back, leaving the review stuck on a historical commit (shipped as an accepted v1 edge in #994). A page reload did not escape either, since GET /api/diff serves the current session diff. Client-only fix: - preCommitDiffRef captures the pre-commit diff (full diff type + base) on the first non-commit to commit switch in handleSelectCommit. - Leaving Commits for Tree restores it via fetchDiffSwitch; without a memo (post-reload) it falls back to the session default with the same resolution handleWorktreeSwitch uses. Exiting while the commit switch is still in flight also restores, superseding it server-side. - Any applied non-commit switch clears the memo in fetchDiffSwitch. - Reload un-trap: a load that is served a commit diff while the opening panel view is not Commits snaps back to the session default once. Git status keeps its existing since-base reset; the server keeps its single-active-diff model unchanged. * test(review): cover the offered-but-not-first preferred default; document the Commits detour contract |
||
|
Michael Ramos
|
d4ce3dcb57 |
ci: harden releases and add security scanning (#1274)
* ci: harden release and add security scanning * Harden release and deploy recovery paths * Fix npm artifact pack destinations |
||
|
Michael Ramos
|
8e7b5ce300 |
feat(review): refine Call Flow navigation and annotations (#1277)
* feat(review): refine Call Flow navigation and annotations * fix(review): align viewed controls with panel navigation * feat(review): add Call Flow path search controls * fix(ui): wrap long tooltip identifiers * feat(review): annotate raw Call Flow output * feat(review): refine call flow lens context * fix(review): keep call flow lens search accessible * fix(review): scope call flow find shortcuts |
||
|
Michael Ramos
|
eb34960da9 |
fix(review): remember the last-used panel view; full-width toggle and cleaner panel chrome (#1273)
* fix(review): remember the last-used panel view across sessions
- Add cookie-only reviewPanelViewLastUsed config (sections | tree, never
commits); the header toggle records it and new sessions resolve
session state, then last-used, then the persisted reviewPanelView.
Explicit Settings/setup-dialog writes through the reviewView setters
sync last-used so a stale memo cannot shadow them.
- Wire the FileTree fallback PanelViewToggle to the real panel-view
selection and route Tree clicks through the shared select funnel
instead of rendering a hardcoded, no-op tree segment.
- Mark the review-setup-seen cookie when the first-run reset runs, not
only on the Got it click, so closing the tab without dismissing no
longer re-stomps preferences every session.
* fix(review): full-width panel toggle, relocated tree controls, click-to-copy diff count
- Give the Git status | Tree | Commits toggle the entire top row in all
three left panels; segments stretch to fill the row width.
- Move the controls that shared that row (staged count, search,
collapse-all, hide-viewed, viewed counter) into a shared
PanelControlsRow rendered below the All files entry, directly above
the file list; the Commits panel's loaded-commit count moves to its
own sub-row.
- Replace the footer Copy button with the diff count itself as the copy
trigger (shared CopyDiffFooter): a keyboard-reachable button with the
"click to copy the entire diff set" tooltip and transient
Copied/Failed feedback in the old button's slot.
- Add a tooltip to the file-row stage button using the same Tooltip
mechanism as the viewed control ("Stage file (git add)").
* fix(review): put Tree first in the panel view toggle
* fix(review): resolve effective panel view for the toggle; keep self-heal off the last-used memo
- Add resolvePanelView() and pass the EFFECTIVE view to FileTree so a
latent sections/commits selection the session cannot offer highlights
Tree instead of a hidden or wrong segment; allFilesOrder reuses it.
Unit test covers the no-segment and wrong-segment cases.
- setReviewPanelView() gains { recordLastUsed?: boolean } (default
true); the App self-heal passes false so repairing a conflicted
persisted pair no longer overwrites the user's last-used memo.
- Update the AGENTS.md since-base section to the new toggle semantics
(last-used memo, resolution order, setter sync, one-time first-run).
- Hardening: fold the diff counts into the copy button's aria-label,
skip the tooltip while the copy button is disabled (hover never fires
on a disabled native button in several browsers), let toggle segments
shrink (min-w-0 + truncate), and rework the config test onto a fresh
ConfigStoreForTest instead of resolving the singleton against a
throwaway backend.
|
||
|
Michael Ramos
|
fc348687bf |
fix(review): contain /api/call-flow analysis throws as JSON error responses (#1272)
* fix(review): contain /api/call-flow analysis throws as JSON error responses
A hard VCS failure during patch materialization escaped the handler in
both runtimes. On Pi the unhandled rejection reached the process-level
handler and killed the user's session; on Bun it surfaced as a non-JSON
500 the client's quiet-failure UX could not parse. Both handlers now
return the standard { status: "error", reason: "analysis-failed" }
envelope.
* fix(review): cut the Call Flow consent copy down to the three facts that matter
Six sentences of disclosure read as noise. The dialog and Settings now
say: what it does, what it installs (languages + size), Node 22+, and
that other languages install as needed. Nothing consent-relevant was
removed.
* test(review): pin consent-copy facts, not prose
The presentation test now asserts the server-derived facts (languages,
size, Node floor); the dialog and Settings tests assert only that the
disclosure prop renders, via a sentinel string. Copy edits no longer
break three test files.
* docs: add Testing Rules to AGENTS.md (no prose-pinning, no round-trip prop tests)
* docs: refine copy-pinning rule — deliberate locks allowed, incidental snapshots banned
* fix(review): use the maintainer's Call flow description in the intro dialog and Settings
* fix(review): Call flow description is the maintainer's exact copy; remove the dynamic disclosure plumbing
The intro dialog and Settings now show only: 'Diffs for function call
stacks across git commits. 22 languages supported (AST-based, built
using Tree-sitter).' The callFlowEnableDescription prop, its App wiring,
and getCallFlowEnableDescription are removed; install size and Node
requirements remain visible in the Call Flow panel itself.
* fix(review): reject empty-path worktree diff types; clean up QA findings
- parseWorktreeDiffType returns null for a worktree diff type with no path.
An empty path resolved to an empty cwd, and Bun.spawn({ cwd: "" }) runs
git in the server's own directory instead of the target repo, so a
malformed 'worktree:' switch returned an unrelated checkout's diff.
Fail closed to the caller's real cwd. (Pre-existing; surfaced by QA.)
- Remove an orphaned JSDoc comment left by the callFlowEnableDescription
prop removal in Settings.tsx.
- Add useCallFlowAnalysis.test.tsx to the CI DOM_TESTS list; its two
tests were silently skipping on every run.
|
||
|
Michael Ramos
|
caf7ce1ccd | feat(review): install Call Flow automatically in the background on opt-in (#1271) | ||
|
Michael Ramos
|
9ee2e83287 |
feat(review): make the CallDiff runtime a strictly opt-in, in-UI install (#1270)
* feat(review): make the CallDiff runtime a strictly opt-in, in-UI install
The merged CallDiff integration eagerly installed a ~784MB runtime for
every user at install time, for a feature that is off by default. The
runtime is now strictly opt-in and the opt-in lives in the review UI:
toggle Call flow, click Install in the panel, watch staged progress, and
use the analysis in the same session.
Installers: the default sequence no longer installs the runtime. Opt in
with --with-call-flow (PowerShell: -WithCallFlow),
PLANNOTATOR_INSTALL_CALLDIFF=1, or { "installCallFlow": true } in
config.json (flag > env > config). PLANNOTATOR_SKIP_CALLDIFF_INSTALL is
deleted; --minimal keeps excluding the runtime; the installer prints an
honest note pointing at the in-app install. The headless CLI path
(plannotator install-runtime call-flow) is unchanged.
Server (both runtimes, contract-identical): POST /api/call-flow/install
starts installCallFlowRuntime() in the background via a single-flighted
coordinator (concurrent POSTs join the in-flight install), runs a
Node 22+ preflight before any download (distinct node-unavailable
error), and rejects cross-origin POSTs with 403. GET
/api/call-flow/install-status reports idle/running/done/error with
stage: downloading, verifying, installing-deps, building. Install
completion invalidates the 30s runtime probe cache so the next
capability advert resolves available without a server restart.
Client: the Call flow Dock's runtime-missing state is now the opt-in
funnel with an honest disclosure (about 800 MB on disk, Node 22+,
one-time), staged reduced-motion-safe progress, and error + retry with
a no-node hint. On done the advert is refreshed through
POST /api/review-analysis and the existing available-change refetch
starts the analysis for the current snapshot with no reload. The intro
dialog and Settings toggle note the separate first-use runtime.
Docs: AGENTS.md env table + Review Server API table, marketing
environment-variables / installation / ui-settings / code-review /
api-endpoints pages, and the CallDiff ADR runtime-boundary and server
contract sections.
* test(review): stop leaking PLANNOTATOR_DATA_DIR from the install endpoint tests
The call-flow install endpoint tests overrode PLANNOTATOR_DATA_DIR at
module-eval time and never restored it. bun runs CI's full suite in one
process and evaluates every test file's module before running tests,
while Pi's generated/storage.ts caches its data dir at import time; the
override therefore made storage's cached dir and later files' live
getPlannotatorDataDir() calls disagree, failing the Pi annotate-history
unwritable-dir test and both durable-submit-record tests.
An afterAll restore alone is not enough: it reproduces the same three
failures with the mismatch inverted (storage caches the leaked dir at
module eval, tests then run against the restored one). The env var is
now never touched at module-eval time at all; it changes only inside
tests and is restored to its original value in afterEach, exactly like
the PORT/PATH pattern. The config writes the advert tests persist
through the process's frozen config module are snapshotted at load and
restored in afterAll so a standalone run never flips a real
config.json setting, and the process-global scope of the mock.module
seams is documented.
Regression proof (previously failing in either mismatch direction, now
green in both orderings):
bun test packages/server/call-flow-install-endpoint.test.ts \
apps/pi-extension/server/annotate-history.test.ts \
apps/pi-extension/server/annotate-submission.test.ts
* feat(review): install CallDiff grammars selectively
* fix(review): harden CallDiff worker environment
* fix(review): close CallDiff verification gaps
|
||
|
Michael Ramos
|
3245310aa8 |
feat(review): add optional CallDiff call-flow analysis (#1268)
* feat(review): add optional CallDiff call-flow analysis * fix(review): harden CallDiff integration |
||
|
Michael Ramos
|
a5f9937f9c |
fix(pi): never touch Pi's system prompt; phase framing as conversation messages (#922) (#1269)
* fix(pi): never touch Pi's system prompt; deliver phase framing as messages (#922) The extension replaced Pi's system prompt with its phase framing during planning and execution, dropping AGENTS.md context, the skills catalog, tools guidance, and user append text, and re-templating the todo list into the system prompt busted the provider's prompt-cache prefix on every checklist update. Plannotator now never returns or modifies systemPrompt (approach suggested by Karrq on the PR). Phase framing is delivered exactly once per phase entry as a hidden plannotator-framing conversation message, execution progress rides in small per-turn plannotator-context todo messages, and a phase-aware context filter keeps only the newest framing for the current phase (idle still clears everything). Cache-busting reduces to conversation-suffix appends plus one history adjustment per phase transition. BREAKING CHANGE: the Pi plan-mode toggle command is renamed from /plannotator to /plannotator-plan-mode with no alias, and the phases.*.systemPrompt config key is retired in favor of phases.*.instructions (a phase-entry message template); old systemPrompt keys are ignored with a session-start warning. * docs: root README uses the renamed plannotator-plan-mode command * fix(pi): make the framing latch survive compaction and tree navigation (#922) Review follow-ups on the message-based framing design: - session_compact reopens the framing latch: compaction can summarize away the delivered framing message, so the next prompt re-delivers it (the context filter keeps only the newest copy if the old one survived in the kept tail). - session_tree re-derives phase, latch, and checklist state from the new active path via the restore logic session_start uses (now shared as resyncPhaseFromSession and reading getBranch(), the active path, instead of the whole append-only entry file). A path with no plannotator state means idle. - The per-turn todo message restates the [DONE:n] convention in one line so the protocol survives between a compaction and re-delivery. - Warn at session start when the bundled plannotator.json is missing, so a packaging regression cannot silently produce a rule-less planning phase. - Tests pin the persistState payload on both sides of the latch, cover compaction re-delivery (exactly once) and tree-switch resync, and the harnesses expose getBranch. |
||
|
Michael Ramos
|
98113182b5 |
feat(guide): reviewer-supplied extra instructions for Guided Review (#1267)
* feat(guide): reviewer-supplied extra instructions for Guided Review (#1265) Adds a quiet, collapsed-by-default Custom instructions affordance to the guide launch page. The text is APPENDED to the built-in organizer methodology as a clearly delimited section (composeGuideMethodology) and never replaces it; absent or blank instructions produce byte-identical prompts to before. Persisted in a dedicated cookie (plannotator-guide-instructions) so a standing team preference survives sessions without bloating the plannotator.agents blob past the browser's per-cookie limit. Server side, the launch body gains an optional guide-only instructions field (both the Bun and Pi node:http agent-jobs handlers accept and thread it); prompt composition lives in the shared guide-review.ts that vendor.sh already vendors to Pi, so both runtimes compose identically. Text is capped at GUIDE_EXTRA_INSTRUCTIONS_MAX_CHARS (2000) server-side and mirrored by the textarea maxLength. Repair launches deliberately ignore instructions: a repair is a mechanical JSON fix, not a rewrite. Tests pin the regression contract (empty input keeps prior prompt bytes), appended-not-replacing composition, the length cap, repair isolation, and the cookie round-trip via the storage backend seam. * refactor(guide): store standing instructions server-side, not in a cookie Review findings on the cookie approach (silent write failure past the encoded 4KB per-cookie limit for multi-byte text) pointed at the real design problem: the instructions are consumed by the SERVER at launch time, so they belong in the data dir like review-skills.json, where no size ceiling or encoding inflation exists and the preference follows the machine instead of one browser profile. New GET/PUT /api/agents/guide-instructions in both runtimes backed by shared guide-instructions-store (vendored to Pi). Guide launches apply the stored text when the body carries none; the launch page still sends its live textarea value (explicit wins), so a just-typed preference can never race the debounced save. The sidebar surface sends nothing and inherits the stored text server-side. All cookie machinery removed. Also folds in the review fixes: marker-tag-shaped strings in instructions are defanged so first-match nonce recovery cannot be hijacked by pasted examples. |
||
|
Michael Ramos
|
ab8d2581eb |
feat(ui): onUnanchoredChange report + readOnly keeps the host footer slot (0.30.0) (#1263)
The bridge now names WHICH annotations have no live representation on the page (every target dead, or the restore never resolved), reported on change through a new validated message and the HtmlViewer onUnanchoredChange prop, delivered in readOnly mode too. Fail-closed anchors previously hid markers silently. AnnotationPanel readOnly no longer suppresses the renderCardFooter slot: its contents are host-owned and may be read affordances (replies, links), so the host gates what belongs in it. Built-in delete/edit and direct-edit discard stay hidden. |
||
|
Michael Ramos
|
96ad52cd90 |
fix(ui): null-guard vim focus reassert + publish/migration doc corrections (#1262)
* fix(ui): null-guard vim focus reassert + publish/migration doc corrections The vim focus-reassert guard compared a nullable iframe ref to the nullable document.activeElement: with both null the branch runs and iframe.contentWindow throws. Also flagged by the first strict-TS consumer of @plannotator/ui 0.29.0. HANDOFF gains the CI-only provenance note, the highlight.js bundler-alias removal warning, and the known @pierre/theming peer-range warning. * chore(ui): bump to 0.29.1 for the null-guard fix |
||
|
Michael Ramos
|
acb4979c59 |
chore(ui): prep @plannotator/ui 0.29.0 + core 0.23.0 publish (#1222) (#1261)
Bless components/html-viewer as supported host surface, document the .hljs to pn-code migration, pin the readOnly view-only contract with tests, and bump both published packages for the next manual publish. Core must publish first: ui 0.29.0 imports @plannotator/core/annotatable, which is absent from published core 0.22.0. Closes #1222 |
||
|
Michael Ramos
|
2fff8756d9 | chore: bump version to 0.26.8 v0.26.8 | ||
|
Michael Ramos
|
7d7654fcda |
feat(annotate): minimal-by-default HTML sessions with stale-preference decay (#1260)
* feat(annotate): minimal-by-default HTML sessions with stale-preference decay Raw-HTML annotate sessions now open minimal by default: Pinpoint input, tools hidden, left sidebar closed, and the right annotations drawer closed (the drawer joins the persisted HTML chrome state, previously it always opened on desktop). Explicit user choices still persist between HTML sessions, but the records now carry a timestamp and expire after 7 days without a refresh, so users who have not changed anything or annotated HTML in a while come back to the product defaults. Explicit changes and annotation activity both re-stamp the records, so active users keep their setup. Legacy untimestamped cookies are treated as expired (a one-time reset to the new defaults). The markdown surface keeps its own preference with no TTL, unchanged. Mutation-verified: disabling the TTL fails 4 tests. * test(annotate): stamp seeded chrome cookies and compare semantic fields The App-level chrome suite seeded legacy untimestamped cookies, which the stale-preference decay now treats as expired by design, and one assertion compared cookie bytes that re-stamping legitimately changes. Seeds carry a fresh savedAt and the write-integrity assertion compares the chrome fields. |
||
|
Michael Ramos
|
c4fc79f0aa |
fix(annotate): neutralize document-authored CSP meta tags so the bridge can run (#1259)
A document carrying its own <meta http-equiv=Content-Security-Policy> (including Plannotator's own portable guided-review exports, which embed default-src 'none') blocked the injected inline bridge script, disabling annotation entirely for that file. The iframe sandbox attribute is the annotate surface's security boundary; the page's CSP was authored for its standalone context, so injectIntoHead now strips CSP meta tags (order/quote/case tolerant) before splicing the bridge. Pre-existing bug, surfaced during v0.26.8 manual QA. Mutation-verified test. |
||
|
Michael Ramos
|
121082430e |
fix: QA-gate hardening for the v0.26.8 feature set (overlay perf, numbering, OpenCode 2 parity) (#1258)
* fix(opencode): consolidate V2 system parts into one composed prompt (#1114) The OpenCode 2 adapter still shipped the pre-#1114 multi-part system injection: replacePlanningSystemParts kept one part per source and the generic reminder pushed a separate part, so Qwen3.x Jinja template corruption persisted for OpenCode 2 users. Mirror the V1 entry exactly: compose the stripped existing text plus additions into a single system part via composeSystemPrompt, and compose the generic reminder into the existing text instead of appending a second part. Also adds the regression tests for the bug class flagged in #1114's review: both helpers must read/compose the existing system text BEFORE truncating the array (a reorder to 'system.length = 0' first drops the host prompt and goes red here). * perf(annotate): harden the raw-HTML overlay reconcile (dead-target backoff, cull, batching) Bridge-script hardening for mutation-heavy pages and large annotation sets, plus the lost click-to-select hover affordance: - A: dead-target re-search now carries a wall-clock backoff (300ms doubling to a 5s cap, reset on success) ON TOP of the generation gate, plus a 2-searches-per-reconcile-pass budget with a scheduled follow-up pass for budget-skipped eligible targets. A page that mutates every frame advances domGeneration every frame, so the generation gate alone re-ran the whole-document TreeWalker sweep (and anchor re-resolution) per frame forever for permanently unresolvable targets. - B1: early viewport cull (64px margin) for element and range targets: wholly offscreen targets skip targetStyleHidden / getComputedStyle / clipBoundsFor / client-rect collection entirely and just omit their markers, which is what the visible pipeline produced anyway. - B2: read/write batching in renderAnnotationOverlay: highlight rects are queued during the read phase and flushed as one write phase, so the pass no longer forces a synchronous layout per record. - B3: restoreAnnotation defers its render through the existing rAF-coalesced reconcile scheduler; restoring N annotations now renders once instead of N full passes (searches stay synchronous for the mark-applied reply). DOM tests flush the frame via the suite's standard macrotask flush. - B4: zero-work observer gate: page mutations with no records, no pending draft, and pinpoint inactive still bump domGeneration but no longer schedule a reconcile frame. - D: hover affordance for click-to-select: the rAF-throttled mousemove hit-tests the pointer against the CACHED rendered committed rects and toggles a brightness class on that annotation's rect divs inside the shadow root. No page-DOM writes, rects stay pointer-transparent, and shadow-root writes are unobserved so there is no reconcile loop. - G: while a text drag is in progress in drag mode, placed markers yield pointer input (data-pn-hittest) so the 25px bubble cannot capture a selection drag; armed only by a >4px primary-button move from a non-overlay mousedown, so marker clicks and click-to-select paths are untouched. withMarkersYielded now restores (not clears) the attribute. New regression tests for A, B1, B3, B4, D; A/B1/B3 mutation-verified (fix reverted, test observed failing, fix restored). * fix(annotate): make on-page marker numbers match exportAnnotations numbering The HtmlViewer sync excluded GLOBAL_COMMENT annotations before numbering while exportAnnotations numbers '## N.' sections across the FULL list including globals — so an on-page 'Comment 2' could be '## 3.' in the feedback the agent reads. The sync now derives each marker's number from its position in the full createdA-sorted list (globals occupy a number but ship no entry, leaving the correct gaps on-page). Export format is unchanged. New buildSyncNumbering helper + tests asserting a mixed list yields identical numbers between the sync payload and exportAnnotations output (mutation-verified against the pre-fix ordering). * chore: sync stale workspace versions in bun.lock (0.26.1 -> 0.26.7) * docs: document raw-HTML overlay model, multi-target types, and known limitations - Data Types: add htmlAdditionalTargets to the Annotation listing plus the HtmlElementAnchor (including the optional normalized point used by placed markers) and HtmlAnnotationTarget shapes. - Annotation System: describe the post-#1257 raw-HTML surface (placed comment markers + overlay-projected highlights, no inline mark mutation; durable anchors persisted, disposable markers projected) and the print-parity limitation. - URL Sharing: note that share links intentionally drop HTML element anchors and additional targets (restore is text-search based, per sharing.multiTarget.test.ts). * test: fix Range.getClientRects stub typing in the B1 cull test * fix(annotate): hover-race teardown and unbounded one-shot dead-search passes Polish round on the overlay hardening: - Hover race (1): switching into pinpoint mode (or opening a draft) now tears hover down fully via clearHoverHighlight() — cancels the pending rAF hit test and clears the tracked position and id — and the rAF callback itself refuses to paint outside drag mode / with an open draft. Previously the pending callback re-applied the class after the mode switch and every flushQueuedHighlights re-painted it from the stale hoverHighlightId, leaving a permanent phantom hover. - One-shot budgets (3): beginDeadSearchPass takes a per-pass budget. Reconcile passes keep 2 (they repeat, skipped targets get follow-up frames); print and scroll-to are user-initiated one-shots with no follow-up and now run unbounded (backoff and generation gates still apply), so printing with 3+ dead-but-recoverable targets no longer silently prints fewer highlights. Both changes carry new regression tests, mutation-verified (fix reverted, test observed failing, fix restored). * fix(annotate): number markers by array position and cap entries after dropping globals The createdA sort made the export-match invariant false with external annotations: exportAnnotations' sort keys tie for every raw-HTML annotation (blockId '', startOffset 0), so its stable sort numbers the combined [...local, ...external] list in ARRAY order — and external annotations arrive appended with server-stamped createdA values that can interleave with local timestamps. buildSyncNumbering now numbers by array position of the input (verified to be the same combined list both consumers receive from packages/editor/App.tsx allAnnotations; the viewerAnnotations diffContext filter is order-preserving and vacuous on the raw-HTML surface). Also reorders the cap: number the full list, drop globals, THEN slice 512 entries — globals no longer waste sync capacity and a non-global the export numbers past position 512 still syncs while slots remain. Numbers may now exceed 512 (array positions); the bridge's own bound (100000) accepts them and its 512-entry cap still agrees with the sender. Tests updated: interleaved-external agreement with exportAnnotations (mutation-verified against the createdA sort) and slice-after-filter capacity. * docs(opencode): note the accepted cache-hint flattening trade-off in V2 consolidation |
||
|
Yohai Wiener
|
50092d7634 | feat(review): add file header hover state (#1256) | ||
|
Michael Ramos
|
be0b1f185d |
feat(annotate): placed comment markers for raw-HTML annotation (#1257)
* feat(annotate): overlay-projected placed comment markers for raw HTML
Annotation state is no longer written into the visited page's DOM. A
fixed, pointer-transparent, shadow-rooted overlay host (appended to the
root element, outside page layout) now owns every committed annotation
visual:
- numbered placed-marker buttons (product-owned SVG speech bubble,
accent-token colors, accessible 'Comment N' labels) projected at the
user's selected relative point, re-resolved from durable anchors and
reprojected on scroll/resize/mutation/animation via rAF-coalesced
invalidation (never polled)
- persistent highlight rectangles for text-range annotations (built
from Range.getClientRects), replacing inline <mark> wrapping
- the focused (blue) treatment as overlay rects covering EVERY rect of
EVERY target, replacing the .focused class that querySelector applied
to only the first mark of a multi-paragraph selection
- the draft selection highlight as overlay rects from the live pending
range
Markers omit rather than guess: unresolved anchors, zero-size targets,
viewport/clip-scrolled-away targets, and clamped points no longer
visibly associated with their target all hide the marker. Viewport-edge
clamping keeps the full marker reachable; coincident markers spread
horizontally and deterministically. Numbering is parent-authoritative:
HtmlViewer syncs the ordered saved-annotation list (panel order,
index+1) to the bridge; grouped multi-select targets all carry their
one annotation's number. The anchor DTO additively gains the normalized
selected point (validated and clamped at the trust boundary), captured
from pinpoint/shift/data-annotate clicks.
Fixes the partial/auto blue highlight (focus-mark and scroll-to touched
only the first mark) and page layout breakage (surroundContents plus
padding/negative-margin marks mutated author content).
* test(annotate): migrate bridge DOM suite to the overlay marker contract
Mark/badge assertions become overlay assertions: markers are queried
through the shadow overlay host, restoration binding is proven via
scroll-to targets instead of inline mark containment, and page-DOM
purity is asserted byte-for-byte across restores.
* test(annotate): regression coverage for the placed-marker overlay contract
Bridge-side (srcdoc.test.ts): layout neutrality (byte-identical page DOM
across restore/sync/focus/scroll, host outside body, pointer-transparent),
relative-point capture and reprojection with fresh geometry, unresolved-
anchor and scrolled/clipped omission, viewport edge clamping with
visually-detached omission, deterministic coincident spreading, parent-
synced numbering override and renumbering, malformed sync rejection,
full-coverage focus rects (partial-blue regression), overlay draft
highlight, and hit-test yielding beneath markers.
Parent-side (htmlPinpointProtocol.test.tsx): anchor-point validation
(clamping, hostile-point dropping without losing the anchor), point
propagation onto committed annotations, multi-target anchor points, and
the ordered saved-annotation number sync (createdA order, index+1,
globals excluded, never before bridge-ready).
* test(annotate): fix strict typings in new overlay tests
* fix(annotate): clip-test overlay highlights, gate dead-target re-search, honest edge clamping
- M1: every painted highlight rect (committed comment/deletion, focus flash,
draft selection) is now intersected with the target's clip-ancestor chain
via shared clipBoundsFor()/clipRect(); rects with no visible remainder are
dropped, so inner-scroll-container content scrolled out of its box no
longer paints stripes over unrelated content.
- M2: computed-style visibility gate (visibility:hidden/collapse,
display:none, opacity:0) treats targets as unresolved-for-display, so
markers/focus rects stop rendering over visible content stacked in the
same box (e.g. visibility-toggled carousel slides).
- M3: dead-target re-search (whole-document findTextRange + anchor
re-resolution) is generation-gated: the generation advances only on
text-capable signals (page mutations, settle events, frame loads), never
on scroll/resize, and each target caches its last failed generation.
- m3: clipBoundsFor skips plain static overflow clippers for position:fixed
targets until a fixed containing block (transform/perspective/filter/
backdrop-filter/will-change) is reached.
- m4: marker association is tested against the UNCLAMPED point; the 29px
viewport inset is rendering-only, so fully visible edge-flush elements
keep their markers (dead band removed; sliver test updated — it conflated
viewport-edge clamping with clip-container omission).
- m5: reconcile on document.fonts.ready and capture-phase subresource load
(geometry-only: they never unlock dead-target re-search).
- m7: Range.getClientRects containment filter drops border boxes that
duplicate their own line rects (redline double-paint).
- m9: settle events from viewer overlay nodes are identity-filtered out.
- m10: one marker per resolved element per record during refresh.
- m12: painting caps at 48 rects but the marker anchors to the TRUE last
client rect.
* fix(annotate): restore print parity for committed highlights (M4)
The branch's '@media print { .pn-layer { display:none } }' hid ALL
annotation visuals in print, but pre-overlay the inline highlight marks
stayed visible in print on purpose (only pin badges were print-hidden).
On beforeprint (plus a matchMedia('print') mirror for Safari), committed
comment/deletion rects are re-projected into a temporary absolute-positioned
light-DOM layer in document coordinates so it paginates with content, and
torn down on afterprint. Markers remain print-hidden (parity: highlights
print, markers don't). A screen media guard keeps the layer from ever
flashing on screen, and any build error fails safe into printing without
visuals.
* fix(annotate): restore highlight click-to-select; marker-consistent pinpoint hover (M5, m6, m8, m11)
- M5: clicking anywhere on a committed range highlight posts mark-click
again (pre-overlay parity). Rects stay pointer-transparent — the document
bubble click handler hit-tests the point against the painted committed
rects (smallest wins on overlap, ties to the topmost/later annotation).
Coexistence matches the old '.annotation-highlight' handler: capture-phase
pinpoint annotate clicks stopPropagation() first, marker buttons stop
propagation, shift-clicks and drag-selection tails are skipped, and
[data-annotate] elements defer to a highlight under the click point.
- m6: when the raw (pre-yield) pinpoint hit is a placed marker, the hover
advertises the MARKER's identity (label 'Comment N', no annotate box)
instead of labeling the element beneath — click and hover now agree.
Annotating beneath still works by moving off the 25px bubble.
- m8: the pinpoint hover label is kept AFTER the overlay host on the root
element (re-appended when not last), so marker bubbles can never occlude
it at equal z-index.
- m11: clear-marks also clears the parent-synced number map so stale
numbers cannot leak onto future records reusing an id.
* fix(annotate): parent-side bounds — cap mark-click ids, truncate the sync feed at 512 (m1, m2)
- m1: parseBridgeMessage rejects mark-click ids longer than 256 chars — the
one page-controlled string in the changed path that lacked a length cap
(the bridge's own sync validation already caps ids at 256).
- m2: the HtmlViewer sync-annotations effect slices the ordered collection
to 512 entries AFTER the stable sort, mirroring the bridge-side
MAX_SYNC_ANNOTATIONS bound so both sides agree on the first 512 numbers.
* test(annotate): committed-range extent assertions + fix-round regression coverage
- M6: restores the migration-dropped EXTENT assertions via a test-only
bridge introspection hook (committedRanges): the vim Visual commit binds
exactly 'Alpha ', the visual-block commit exactly 'Whole block target',
and the pin-restore scoped range binds inside the anchored element
covering exactly 'Anchor target text' (the scroll proxy only checked the
element target).
- Regression tests for every behavioral fix: clip-tested highlight rects +
focus flash (M1), style-hidden visibility gate (M2), generation-gated
dead-target re-search (M3), print-parity layer lifecycle (M4),
highlight click-to-select with smallest-wins overlap (M5), fixed-position
clip exemption (m3) plus preserved clip-container omission, containment
filter (m7), refresh dedup (m10), clear-marks numbering reset (m11),
true-last-rect marker anchoring past the paint cap (m12), and
marker-consistent pinpoint hover with label paint order (m6/m8).
- Parent-side: sync feed truncation at 512 after the stable sort (m2) and
the 256-char mark-click id cap (m1).
- Test honesty: the hit-test-yield test now string-asserts the exact
':host([data-pn-hittest]) .pn-marker' pointer-events rule, since the
elementFromPoint mock implements the yield itself.
- Tests advance the re-search generation via the settle-event signal:
happy-dom stops delivering body MutationObserver callbacks once the
overlay host holds an SVG marker button (environment bug; isolated repro
without any bridge code — real browsers are unaffected).
* fix(annotate): bound range-rect collection at 48 with a by-index true-last read
The m12 fix had removed the collection cap: rangeClientRects materialized
EVERY client rect and the O(n^2) containment filter ran over the full list,
per range target per rAF reconcile and synchronously per click hit-test. A
large drag-selection or redline (the Range extent is uncapped — only the
selection text is capped at 10k chars) yields thousands of rects, i.e. tens
of millions of iterations per scroll frame.
Collection now breaks at MAX_HIGHLIGHT_RECTS again, so the zero-size and
containment (m7) filters operate on at most 48 entries, and the m12
requirement is met by reading the DOMRectList's final entry directly by
index (with the marker-association union extended to that tail rect, and a
zero-size tail falling back to the last paintable rect). Regression test:
60 mocked rects with a containing border box paint 47 (cap + containment)
while the marker anchors at the true 60th rect; mutation-verified against
both an uncapped collection and a capped-prefix last-rect read.
* fix(annotate): drop the opacity:0 display gate; contain-aware fixed clipping; dedup among placed markers
- The M2 gate's opacity:0 leg hid markers for the legitimate
invisible-hit-target pattern (transparent input stretched over a styled
control — the pinpoint hit resolves the input and pre-overlay the marker
rendered exactly over the visible control), and failed its own carousel
motivation anyway: computed opacity does not inherit, so a container
faded to 0 leaves descendants at computed 1. visibility:hidden/collapse
and display:none remain the gate.
- establishesFixedContainingBlock also treats contain layout/paint/strict/
content and container-type size/inline-size as establishing a fixed
containing block, so such clippers correctly apply to fixed targets.
- The per-record element dedup now runs among PLACED (visible) markers
only: a target whose stored point is clipped away no longer consumes the
element's slot and suppresses a sibling target whose point is visible.
Tests updated/added and mutation-verified: opacity keeps the marker,
contain:layout re-applies the clipper, and the visible sibling survives
the dedup.
* fix(annotate): watch documentElement for page mutations; print layer on the root element
- The M3 generation gate could lock out re-search forever when a page swaps
the <body> element itself: the observer watched document.body, so
documentElement.replaceChild(newBody, oldBody) produced no record, no
generation bump, and a dead target whose one free retry ran against the
interim skeleton never retried again. The observer now watches
document.documentElement (same config), so body swaps and the new body's
content are in-subtree.
- Consequence handled: childList mutations on the root/body whose
added/removed nodes are ALL overlay-registered (host append, hover-label
re-append, print-layer insert/remove) are filtered out via
isOverlayOnlyMutation, so overlay writes neither bump the generation nor
schedule the reconcile frame that caused them. The print layer stays
overlay-registered through its async removal record (retired-layer
deregistration is deferred to the next lifecycle step).
- The print layer is appended to documentElement instead of body: a page
styling body { position: relative } made body's padding box the containing
block and shifted every stripe by body's document offset. On <html> the
containing block is the ICB, matching the viewport+scroll coordinates; a
positioned documentElement is accepted as out of scope (commented).
Tests: the bridge's observer is captured at load (happy-dom stops
delivering records once the overlay host holds an SVG marker — environment
bug, so scope tests assert the observed target and drive the callback with
synthetic records): body-swap unlock, overlay-only no-bump, and the
print-layer parent are all covered and mutation-verified.
|
||
|
Michael Ramos
|
62ffbd47b8 |
Merge pull request #1255 from backnotprop/seo/self-hosting-docs-redirect
fix(marketing): redirect self-hosting guide to canonical docs |
||
|
Michael Ramos
|
3fd6d33906 | fix(marketing): redirect self-hosting guide to canonical docs | ||
|
Michael Ramos
|
f8951cd3c6 |
feat(annotate): shift-click multi-element selection for raw-HTML pinpoint (#1254)
* feat(annotate): shift-click multi-element selection for raw-HTML pinpoint drafts One comment covering multiple targets: shift-clicking elements while a pinpoint draft composer is open toggles them in/out of the SAME draft. - bridge: pendingMultiTargets registry with per-target pinned outline boxes, DOM-identity + anchor-equality toggle dedup, primary promotion on removal, draft cancel on last removal, shift-hover preview, rAF pointer relay for the composer yield, remove-target/flash-target parent messages, capped at 16 additional targets at the source - pins: registerPin now allows several elements per annotation id (deduped by (id, element)/(id, anchor)); badges number by first-seen id so all targets of one annotation share one number; find-and-mark restores additionalAnchors as same-numbered pins (anchor-only, fail-closed) - parent hook: multi-target-added/removed/pointer messages validated and capped at the trust boundary (key<=64, label<=64 truncated, text via the 10k surrogate-safe cap, anchors via parseHtmlElementAnchor, array cap 16); draftTargets state, chip removal with deterministic promotion mirrored on both sides, composerFocusToken for focus return - types: additive Annotation.htmlAdditionalTargets (label/text/anchor per extra target; anchor optional so fail-closed targets still export) - CommentPopover (all seams optional, default off): horizontally scrollable target chips with remove buttons and hover-to-flash, refocusToken, captureStrayKeys first-keystroke guard, yieldState fade/click-through with 180ms transition and prefers-reduced-motion fallback - HtmlViewer: composer-yield state machine (composerYield.ts) fed by parent mousemoves plus bridge-relayed pointer positions, with 48px/96px hysteresis - export: multi-target comments append an 'Also applies to N more elements' block (label + excerpt per target); single-target output byte-identical - share URLs: additional targets are dropped exactly like htmlAnchor (the compact tuple format never carried anchors); drafts carry them verbatim * test(annotate): cover shift-click multi-select across bridge, DTO, composer, export, drafts, sharing - srcdoc.test.ts (bridge DOM): shift-click add + toggle-off with echoed removals and per-target pinned boxes; create-mark commits all targets under one id with one badge number (second annotation numbers 2); primary promotion and last-removal cancel; parent remove-target mirrors without echo; flash-target; 16-target cap at the source; find-and-mark restores additionalAnchors as same-numbered pins with stale anchors failing closed - htmlPinpointProtocol.test.tsx: multi-target-added/removed/pointer DTO validation (key/label/text/anchor caps, hostile payloads), selection targetKey/targetLabel validation; mounted-composer flows — primary chip, shift-adds into ONE submitted comment carrying htmlAnchor + 2 additional targets, single-target submit shape unchanged, promotion onto the comment, last-removal closes the composer, chip removal, 16-cap at the trust boundary, drag selections never arm multi-select - CommentPopover.multiTarget.test.tsx: chips render primary-first with remove/hover handlers, refocusToken focus return, captureStrayKeys stray keydown routing (and non-interference when focused), yieldState classes + reduced-motion-aware 180ms style; default composer renders none of it - composerYield.test.ts: distance + hysteresis state machine (48px over-exit, 80/96px near enter/exit) - parser.test.ts: multi-target export block (labels, excerpt clipping, fail-closed targets) and byte-identical single-target output - useAnnotationDraft.seam.test.tsx: multi-target annotations round-trip the draft transport verbatim (save body + restoreDraft) - sharing.multiTarget.test.ts: share tuples drop anchors AND additional targets while the comment itself still shares * fix(annotate): keep pinpoint drafts alive when the pinned element scrolls out of view Found by the real-browser signoff harness: reaching a second element to shift-click often scrolls the pinned primary out of the viewport BEFORE any additional target exists, and the scroll-out teardown then cleared the bridge's pin state mid-compose — the shift-click landed on a dead draft and started a new one instead of adding to it (and even a single-target pinpoint draft silently lost its visual pin on commit after scrolling). Pinpoint drafts (pendingPinViaPinpoint) now survive scroll-out; drag selections keep the existing close-on-scroll-out behavior unchanged. * fix(annotate): address adversarial review of multi-select (arm handshake, label sanitization, iframe shift relay, removal resync) D1 (blocker): the bridge accepted shift-toggles for ANY pinpoint draft while the parent only mirrors targets when the comment composer owns it — in quickLabel mode the user could pin elements the saved annotation would never carry. Multi-select is now ARMED EXPLICITLY: the parent posts arm-multi-select (keyed to the primary, so a stale arm can never arm a new draft) only from the composer flow, and the bridge refuses the toggle — shift-click behaves as a plain click — until armed. D2: target labels derive from page-controlled attributes (aria-label), so newlines could smuggle real markdown structure (fake headings) into agent-read feedback. parseTargetLabel now collapses all whitespace at the trust boundary, and the exporter collapses again (defense in depth for persisted pre-fix drafts). D3: the composer yield armed Shift only from parent-window keydown/mousemove, but window blur (focus entering the iframe) cleared it and modifier keydowns don't reach the parent from the sandbox — from the second shift-click on, the composer never yielded. The bridge pointer relay now carries the observed shiftKey (validated strict boolean) and arms/disarms yield directly. D4: a forged multi-target-removed desynced parent (promotes) from bridge (keeps original). applyTargetRemoval now ALWAYS echoes remove-target — idempotent for legit bridge-side removals, forcing convergence after forgery. D5: the 'Also applies to N more elements' block gains a leading blank line so markdown lazy continuation cannot fold it into the preceding blockquote. D6: the stray-key guard registers in capture phase (a bubbling global shortcut can no longer both fire and have its character appended), inserts at the textarea's remembered caret instead of end-of-text, and refocusToken now preserves the caret rather than jumping to the end. D7: the expanded dialog no longer carries the dead yield class/style — its data-comment-popover wrapper spans the viewport, making proximity meaningless; the dialog deliberately does not yield. Also reverts the incidental bun.lock version-catch-up churn. Tests: unarmed/stale-arm refusal, quickLabel non-arming and non-mirroring, newline-label collapse at both layers, bridge-shift-driven yield, forged removal echo + bridge-side idempotent resync, caret-preserving stray keys, blockquote separation. Signoff harness re-run green (14/14) on rules-ui-signoff.html including the arm handshake. * fix(annotate): reset multi-select arm on every new pinpoint draft (D1-R) The re-review caught that multiSelectArmed was never cleared when annotateElement started a fresh draft — only clearPendingPin reset it. So a comment-mode draft (armed) followed by a mode switch the parent doesn't mirror (quick label posts no arm) and a new pinpoint click left the stale arm live: the bridge accepted shift-clicks and pinned elements the saved annotation would never carry. Reset the flag at the top of annotateElement alongside clearMultiTargets. Regression test reproduces the exact sequence (armed draft -> new unarmed draft -> shift-click must not add a target); mutation-verified that removing only this reset fails it. * docs(annotate): correct the first-keystroke guard comment reasoning The comment claimed capture phase prevents a global shortcut from also firing; preventDefault does not stop the dispatcher (it ignores defaultPrevented by design). Restate the actual invariant: the guard is safe only because no bare printable single-key binding exists on this surface, and flag that as a constraint for future bindings. Comment only. |
||
|
Andrew
|
bb6a65ac76 |
Fix OpenCode plugin Jinja template corruption with Qwen3.6 (#1114)
* fix(plugin): consolidate system prompt injections into single array element The plugin previously pushes planning prompts and improvement contexts as separate elements in the output.system array. This change appends them to output.system[0] with newline separators instead. This keeps all system instructions within a single message block to prevent potential parsing or formatting issues when the agent processes the context. * refactor(opencode-plugin): extract composeSystemPrompt helper to centralize system prompt assembly and add unit tests * style(opencode-plugin): remove extra newline before plan submission reminder heading * fix(opencode-plugin): store composed prompt result before clearing system array to prevent data loss Previously, `output.system` was cleared with `length = 0` before being passed into `composeSystemPrompt`, causing the function to compose from an empty array instead of the original system content. The fix stores the composition result in a variable first, then pushes it after clearing. Additionally, add `.trim()` in `stripConflictingPlanModeRules` to normalize whitespace before filtering empty entries, and include a test case for empty string collapse behavior. * refactor(plan-mode.ts): move string trimming from stripConflictingPlanModeRules to composeSystemPrompt for centralized whitespace handling * test(plan-mode): add test case for trimming trailing newlines in composeSystemPrompt |
||
|
Mehdi
|
696bf1f24e | fix(vim): preserve plan position on refocus (#1252) | ||
|
Michael Ramos
|
62c1eab119 | chore: bump version to 0.26.7 v0.26.7 | ||
|
Michael Ramos
|
608a8003aa |
feat(annotate): hit-test pinpoint targeting for raw-HTML sessions (#1251)
* feat(annotate): hit-test pinpoint hover instead of the semantic whitelist Pinpoint hover in raw-HTML sessions now resolves the real element under the cursor via deep elementFromPoint (piercing open shadow roots) instead of the SEMANTIC_* tag-whitelist graph, so styled div/span prototypes (chips, icon buttons, cards) are individually targetable. Scope control stays mouse-only and geometric, matching the markdown surface: the deepest element under the pointer wins; pointing at a container's padding or any area not covered by a child selects the container. - Exclusions are identity-based: html/body, script/style, and an identity Set of viewer overlay nodes (pin badges, pinpoint box/label, vim UI) so page markup cannot spoof its way out of targeting. Annotation marks are transparent (resolve to their parent). - Tiny-element promotion: a leaf under 16px on both axes climbs to the nearest ancestor at least 16px on one axis (MIN_CAPTURE_SIZE floor, not a whitelist). SVG shape primitives still promote to their <g>. - Hover no longer builds the semantic target graph per pointer frame (graphForPointerFrame removed); it is per-event hit-testing with a 2px/16ms last-position cache that the scroll reconcile invalidates. The graph survives untouched as the vim-navigation vocabulary. - Hover labels for generic containers use a cascade: aria-label -> role -> 1-2 meaningful class tokens (hash-ish tokens stripped) -> own text when <= 40 chars -> 'container'. Known semantic tags keep their names. - Click annotates exactly what the hover box shows; text-less elements post '[element: <label>]' instead of being untargetable. - Anchors stay fail-closed: data-test-id/data-cy/data-qa join the author-controlled identity attrs, and text-less elements store a '[[pn-shape]]' signature (tag + sorted classes + child count + 16px size buckets) verified whole-string on restore. * test(annotate): cover the pinpoint hit-testing contract New DOM regression tests modeled on the signoff prototype pages: chip/button targeting on styled div/span markup, container selection via uncovered-area pointing, tiny-element promotion (mocked 8x8 dot promotes to its 200x100 card), the generic-container label cascade, the text-less shape-signature anchor round trip (verify + fail-closed rejection on shape drift), data-cy identity-attr trust, and an instrumented assertion that hover hit-testing performs zero document-wide querySelectorAll calls across 30 mousemoves. Also reattaches the cached hover label element when the body was replaced (parity with the hover box), and accepts the shape-prefixed snapshot through the parent-side DTO test. * fix(annotate): drop shape-signature anchors; fail closed on text-less weak selectors Review findings on the pinpoint hit-testing branch: - D1 (blocker): structure-derived shape signatures are identical across identical siblings by construction, so a positional selector whose sibling was removed resolved the WRONG element while the signature still verified — the exact wrong-binding failure the anchor system exists to prevent. The mechanism is removed entirely: text-less elements now anchor ONLY when the element itself carries a stable-identity rung (#id or the data-* identity attrs); otherwise no anchor ships and the pin simply does not restore (same as before the branch, fail closed). A wrong-binding anchor is worse than no anchor. D2-D4 (in-band prefix discriminator, uncapped class-list component, viewport-dependent size bucket) die with it. - D5: the pinpoint click handler gated pin-badge ownership on a [data-plannotator-pin-badge] selector match, contradicting the identity-only overlay rule. It now gates on the overlay identity set: a page element spoofing the attribute hovers and annotates like any other element, while real badges keep owning their clicks. - D6: class-token hover labels now route through the 40-char label cap (two long tokens could reach 73 chars), and the '[element: ...]' posted text goes through capSelectionText for consistency. - D7: the pointer-hit cache invalidation moved to the top of the rAF reconcile, ahead of its guards, so every reconcile pass (scroll, resize, and body ResizeObserver — i.e. layout-changing mutations) clears it; the 2px/16ms TTL bounds any remaining staleness to one frame. Tests updated to the new contract, with regressions for D1 (identical text-less siblings ship no anchor; crafted positional anchors with empty snapshots resolve nothing after sibling removal), D5 (spoofed badge is annotatable, real badge posts mark-click), and the D6 label cap. |
||
|
Michael Ramos
|
d579ff8db2 | chore: bump version to 0.26.6 v0.26.6 | ||
|
Michael Ramos
|
e53a933106 |
fix(ci): bump Bun build pin to 1.3.14 for sandbox env loading (#1249) (#1250)
Bun <= 1.3.11 loads an empty process.env when a cwd ancestor directory is unreadable, the normal state inside OS sandboxes (Seatbelt/Landlock): every released binary silently ignored all PLANNOTATOR_* env vars there (oven-sh/bun#27802, fixed in 1.3.13). The 1.3.11 pin existed for the Bun 1.3.12 cross-compile signing regression (#541, binaries SIGKILLed on macOS); verified gone on 1.3.14: cross-compiled darwin-arm64 output carries the same linker-signed CodeDirectory as the known-good shipped binaries and executes cleanly on macOS 26.3, and the unreadable-ancestor env repro passes on a 1.3.14 build. Guardrails so neither regression class can ship silently again: the release smoke matrix gains a macOS arm64 leg (executing the binary is the signing assertion), and a new smoke step runs the annotate server from a cwd with an unreadable ancestor, where responding on the fixed PLANNOTATOR_PORT proves env vars were read. |
||
|
Michael Ramos
|
9c40ffadcc | chore: bump version to 0.26.5 v0.26.5 | ||
|
Michael Ramos
|
2d61b76c44 |
fix: address the three pre-release sweep findings (#1246)
Three real, new-in-range bugs from the free-hunt regression sweep: - The pinpoint anchor builder ran a document-wide uniqueness query per ancestor against a growing selector with no depth cap, so one click on a deeply wrapped document froze the tab synchronously (measured 58s at depth 800). The walk now abandons the anchor past 40 ancestors (fail closed: text-search restoration takes over), bounding the work to interactive time. Regression test uses two identical depth-60 chains so uniqueness cannot short-circuit before the cap. - The 10k selection-text cap sliced UTF-16 code units and could split a surrogate pair at the boundary, silently corrupting the annotation tail into U+FFFD downstream. Both sides of the bridge now back the cut off one unit when it would land mid-pair. - The old-git sparse fallback matched git's "unknown option" error literally, which localized git builds translate, so non-English users on old git hit the exact hard failure #1239 was written to fix. All three installers now pin LC_ALL=C around the probe clone (saved and restored; kept single-line in install.ps1 for the scoped-call scanner, whose expectation is updated to the new prefix). |
||
|
Michael Ramos
|
4d9147200d |
ci: wire 8 orphaned DOM-gated test files into the DOM test step
The DOM step runs an explicit file list, and eight describe.if(hasDom) suites were never added to it, so they silently reported 0 pass / all skip on every CI run: the three new #1243 suites (htmlPinpointProtocol, htmlChrome, inputMethod) plus five older vim/highlight suites (vimHud, Viewer.vimMode.integration, useVimSelection, codeHighlight, vimNavigation). Found by the pre-release sweep. All 69 tests pass together locally under the exact CI invocation. |
||
|
Michael Ramos
|
9b14b19a7b |
chore: fold in pre-release QA findings (print overlays, doc alignment) (#1245)
* chore: fold in pre-release QA findings (print overlays, doc alignment) From the 25-item QA sweep over the v0.26.4..main range (all items passed; these were the three real minors worth folding into the release): - The raw-HTML iframe's injected CSS had no print rules, so pin badges (and, in a narrow window, the pinpoint outline box) printed into hard copies of annotated HTML pages. The overlay elements now carry an explicit @media print hide inside the iframe document, where the outer print.css cannot reach. Inline annotation marks stay printable on purpose, matching markdown documents. - AGENTS.md/CLAUDE.md now state that PLANNOTATOR_ANNOTATE_HISTORY also gates the durable submitted-feedback records from #1237, and the Annotation interface listing includes the htmlAnchor field from #1243. - apps/codex/README.md aligns with the #1241 top-level wording (Windows Codex hooks are experimental with printed manual steps, not disabled). - Marketing docs: annotate page documents the pinpoint-first default and minimal-first chrome for raw-HTML sessions; installation page notes the old-git plain-clone fallback from #1239. * chore: drop deprecated marketing-docs edits (canonical docs are Mintlify) |
||
|
Michael Ramos
|
f2ac2adb59 | docs: align README with installer on Windows Codex hook status (#1241) | ||
|
Michael Ramos
|
005c32c7c8 |
ci: remove the install-script auto-sync job (manual sync only)
The deploy-install-scripts job added in #1214 assumed the CI deploy role could write to the plannotator-install-scripts bucket. It cannot, by design, and the job's first-ever real execution (the #1239 installer change) failed on AccessDenied. Install-script syncs are manual with local credentials; the workflow now carries a comment pointing at the release runbook's exact commands instead of a job that cannot succeed. |
||
|
Michael Ramos
|
9ab5392bad |
feat(annotate): pinpoint-first raw-HTML sessions with element anchors and a minimal-first render (#1243)
* feat(annotate): rebuild HTML pinpoint mode with element anchors, pin badges, and a minimal-first render Raw-HTML annotate sessions now open as just the page: first-ever run hides all annotation chrome (toolstrip, tongue tabs, action cluster, sidebar) and the user's last chrome state is restored on every later session via the plannotator-html-chrome cookie. Pinpoint becomes the default input method on the HTML surface, persisted separately from the markdown preference. Pinpoint mechanics are rebuilt in the sandbox bridge, modeled on app-notes-extension and agentation: a fixed-position outline box + label replaces class writes on author elements, hover is identity-gated and re-hit-tested on scroll/resize via a rAF-coalesced reconcile pass, clicks pin the element (crosshair cursor, pinned outline while composing) and go straight to the comment popover. Each pin serializes a verified-unique CSS selector anchor (id > identity attrs > meaningful classes > nth-of-type path) with a text-snapshot fingerprint; restoration is anchor-first with fail-closed validation and falls back to document-wide text search. Element pins that cannot take an inline mark (SVG etc.) get numbered pin badges that track their element and re-acquire it after re-renders. All DOM inspection stays inside the bridge; only validated, size-capped DTOs cross postMessage. Annotation model is extended additively (htmlAnchor), so exported feedback, drafts, and share links keep working. * fix(annotate): harden HTML element anchors per adversarial review Four fixes from the pre-merge adversarial review of the pinpoint round: - Stable identity is now only the element's own #id or data-* rung, re-derived from the resolved element and compared whole instead of parsed out of the selector string. Behavioral attributes (role, href, aria-label, name, alt) no longer exempt an anchor from the text check, so a regenerated page can no longer bind an annotation to the wrong element. A weak anchor with a missing or empty snapshot is rejected, not exempted. - Restoration order: the document-wide text search now runs before the pin-badge fallback, so text that moved elsewhere in a regenerated page is followed rather than badging the stale container. Resolved SVG anchors still pin directly. - Selection text is capped at 10k chars on both sides of the bridge (truncated, not rejected); one pinpoint click on a huge pre/table no longer ships an unbounded page-controlled string into drafts, feedback, or share URLs. - buildAnchorSelector returns null instead of a terminal fallback path already proven non-unique; no anchor beats a known-ambiguous one. Also moves the HTML chrome restore out of the one-shot mount effect into a surface-transition effect, so a linked .html doc opened from a markdown session gets the minimal-first chrome and persists its state, and a markdown surface's sidebar use never leaks into the HTML cookie. * fix(annotate): skip the chrome save in the restore commit itself The reviewer's residual finding: htmlChromeRestoredRef flips synchronously inside the restore effect, but the restored state lands a commit later, so the save effect's run in the restore commit wrote pre-restore values over the remembered state (self-corrected next flush, but a page ending between the two writes would freeze the inverted value). The restore now arms a skip-one flag the writer consumes, so the stale write never happens. A follow-up save fires from the changed deps when the restore changed anything; when it changed nothing the cookie already holds those values. Regression test instruments every chrome cookie write on a returning-user mount and asserts none ever differs from the remembered state (mutation tested: removing the skip fails it). |
||
|
Raúl
|
183cae2c44 |
fix(vim): keep j/k cursor clear of HUD bands when scrolling (#1154)
Document Vim navigation moved the cursor with
`Element.scrollIntoView({ block: 'nearest' })`, which parks the target
flush against the nearest viewport edge — exactly where the sticky action
bar (top) and the key HUD / status pill (bottom) float. Motion to the top
or bottom of a document then hid the caret behind an overlay, while a
mouse wheel (which the browser lets overshoot) kept the same line nearer
centre.
Add vimScroll.ts: a pure computeVimScrollDelta returning the signed
scrollTop delta needed to clear a HUD band at each edge (0 when the
target is already inside the safe band; a target taller than the band
aligns to its top edge so reading order wins), resolveVimScrollMargin
sizing the fallback band as clamp(20% of viewport height, 24px..160px),
and a scrollVimTargetIntoView wrapper.
Both bands are measured from live geometry instead of guessed constants:
the top band widens past the ratio margin to clear the sticky action
bar, and the bottom band derives from the portaled key HUD / mode pill
rects ([data-vim-key-hud] / [data-vim-mode-badge]). The opt-in key HUD
(fixed bottom: 150, height: 88 — a ~238px band, past the 160px clamp) is
actually cleared, while the default pill reserves ~49px instead of
over-reserving 160px. An expanded key HUD is a deliberate modal state
that can outgrow the viewport, so it keeps the ratio margin.
The scrolling element is the native-scroll host fed through
ScrollViewportContext, so the wrapper takes that element from the caller
(useScrollViewport() in Viewer, the same node the reticle measures
against) and falls back to the historical scrollIntoView when it is
absent, so behaviour never regresses.
Route every cursor/target move in useVimSelection through it, and add
vimScroll.test.ts to the DOM allowlist in test.yml so its integration
tests run in CI.
|
||
|
Michael Ramos
|
e24bd8464f |
fix(annotate): persist submitted feedback before deleting the draft (#678) (#1237)
* fix(annotate): persist submitted feedback before deleting the draft (#678) * fix(annotate): scope durable submit records to single local files Adversarial verification found the durable record had no mode gate: an annotate-last or URL session, which was completely stateless before, would persist submitted feedback quoting the agent's message or the fetched page under history/, widening the documented annotateHistory contract without a docs change. The record now shares the exact eligibility gate the version history uses (mode annotate, non-URL path), so previously-stateless modes stay stateless. Also makes persistSubmittedDecision defensive about body types: /api/feedback does no validation (unlike /api/approve), and a non-string feedback previously flowed through settle() untouched with a 200; the new .trim() guard turned that into a thrown 500 after the decision had already settled. Malformed values now degrade to the exact legacy behavior (settle, delete draft, 200) instead of throwing. Both changes mirrored in the Pi server, with regression tests in both runtimes: stateless modes write no record, and a malformed feedback body returns 200 with the draft deleted and nothing persisted. |
||
|
Michael Ramos
|
65b0b739a9 | fix(install): fall back to a plain shallow clone when git lacks --sparse (#1238) (#1239) | ||
|
Will Hampson
|
033368ede4 | Fix feedback delivery after Pi reload (#1240) | ||
|
Michael Ramos
|
d5ae439f7a | chore: bump version to 0.26.4 v0.26.4 | ||
|
Michael Ramos
|
3dd80f37f0 | fix(skills): remove the human-only menu treatment and its hover jitter (#1236) | ||
|
Michael Ramos
|
c760fc522b | chore: bump version to 0.26.3 v0.26.3 | ||
|
Michael Ramos
|
ffd49080ee | fix(skills): harden skill references before first release (#1235) | ||
|
Michael Ramos
|
2edad89dfe | fix(editor): stop the skill-content priming effect from re-render looping (#1234) | ||
|
Michael Ramos
|
710ce1a065 | docs: correct archive sharing posture (#1176) | ||
|
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, `.`,
`..`.
|
||
|
Michael Ramos
|
57b0358265 |
test(review): actually restore @pierre/diffs after the lifecycle mocks (#1230)
The afterAll restore in AllFilesCodeView.lifecycle.test.tsx captured the module namespace object with `await import(...)`. A namespace is a live view of the module record and bun's `mock.module` rewrites that record in place, so the captured handle became the stub the moment the mocks were installed and handing it back re-installed the stubs as themselves. The stubs therefore stayed live for the rest of the process. Whenever bun's file walk put this file before discardRestoreRender.test.tsx, that test's `processFile(...)` returned null and it failed on its first assertion. Spread-snapshot the exports at capture time, the idiom configure.test.ts already documents, and add an in-file guard so a future revert to the bare import fails here instead of somewhere downstream. Also correct the claim that the relative-path sibling mocks cannot escape this file: `mock.module` resolves them to absolute module paths and they leak like any other. |
||
|
Michael Ramos
|
9dc816959c |
test(install): give the PowerShell scanner tests room for pwsh cold start (#1227)
The first pwsh spawn in the process pays assembly-load and JIT cost that exceeds bun's 5s default on a loaded CI runner, so whichever scanner test ran first failed with a timeout. It blocked two unrelated PRs today. Warm spawns finish in ~300ms, so the six tests now carry an explicit 60s budget instead of racing the default. |