112 Commits

Author	SHA1	Message	Date
Michael Ramos	94d4360242	feat(review): hover card trigger settings and first-run introduction (#1462) ... * docs(review): design the token hover card trigger settings and announcement * feat(review): token hover card trigger settings and first-run announcement Hover-for-everyone is a good default and a bad only-option. Two cookie-only settings now cover the three complaints it produces, and a one-time dialog puts the choice in front of the reviewer once. - @plannotator/core/token-hover (shimmed to shared): TokenHoverTrigger (hover | modifier | off), TokenHoverDelay (200 | 350 | 700), and the migration that reads the old tokenHoverCards boolean once. - settings.ts: tokenHoverTrigger REPLACES tokenHoverCards rather than sitting beside it, so there is no unreachable enabled-but-off state. A legacy `false` resolves to `off`; the legacy cookie is read, never written, so a downgrade cannot silently re-enable cards for someone who turned them off. - useTokenHover: an options argument, { mode, delayMs }. In modifier mode the Alt gate sits ahead of the dwell timer, the cache read and every piece of state, so a hover with the key up costs one boolean read. Pressing Alt while already parked on a token opens the card (a key press fires no pointer event, so the last enter is remembered); releasing it starts the same leave grace, except while the pointer is inside the card; window blur clears a stale held flag; and typing owns the key, since Alt+Backspace and Alt+arrow are word-editing chords. The three listeners exist only in modifier mode, so hover mode is byte-for-byte what shipped. - The diff views are untouched: their prop signatures are unchanged, which matters because both are compiled into the portable guide viewer. `off` is still enforced in App by withholding the handler props entirely. - TokenHoverAnnouncementDialog: last in the first-run chain, compact, with a radio group that applies the trigger immediately so Done and Escape both mean "accept what is selected". It skips a session where cards cannot run without consuming its cookie, and never shows to a reviewer who already has a non-default trigger. - Settings > Review Display: the toggle becomes a trigger segment plus a delay segment, the delay inert while the trigger is off. Click behavior is deliberately untouched. Cmd/Ctrl+click and Alt+click both still open the References panel; every gesture on a token is already spoken for, and the modifier mode delivers what a click mode would be wanted for. Alt rather than Cmd because Cmd/Ctrl+hover already paints the navigable-target affordance, and Alt is spelled the same on every platform. Reasoning and the rejected controls are in DESIGN_hover-settings.md. The guides.show viewer manifest moves with this: the settings registry is in the portable viewer's graph. * fix(review): hover card review findings, M1/M2 and minors M1 stale pointerInCardRef. onCardEnter set "the pointer is inside the card" and only onCardLeave cleared it, but a scroll-close or a window blur unmounts the card UNDER the pointer, so no leave ever arrives. The flag then stayed true for the rest of the session and every later Alt release was ignored as "they are reading the card", leaving cards stuck open. close() now resets it, which covers the blur path too since onBlur closes. M2 Alt+click double surface. handleCodeNavRequest closes the hover first, so EVERY route into the References panel is covered by one line: Cmd+click, Ctrl+click, the Alt+click alias, and the card's own location links. It sits ahead of the no-checkout early return, because a click that cannot resolve must still dismiss the card it was made on top of. This also repairs the overlap #1461 shipped with, where a click on a token whose card was open or mid-dwell left both surfaces on screen; close() cancels the pending dwell and aborts an in-flight request, so a click during the dwell can no longer resolve into a card behind the panel. Alt+click is where this is routine rather than incidental: in modifier mode the alias and the trigger share the key. Minors: - The announcement's featureAvailable is LATCHED at the first post-load render, like guideIntroEligibleRef. canUseLiveWorkspaceActions changes on mid-session diff switches, so an unlatched read could pop the dialog over work in progress or yank an open one away mid-read. - resolveTokenHoverAnnouncementPending is pure again, like its editMode sibling. The cookie write that retires the announcement for an already-decided reviewer moved to shouldConsumeTokenHoverAnnouncement, called from an App effect: a state initializer can run more than once, and a getter that writes storage goes unnoticed until StrictMode double-invokes it. - The migration is documented as what it is. fromCookie RE-READS the legacy key on every load until the user touches the setting, because a migrating read returns a value and so never triggers the registry's default-seeding write. Resolution is pure and identical every time, so the repeated read costs one lookup and cannot drift; seeding inside a getter was rejected as more code for no behavioral gain. DESIGN §7, the settings comment and AGENTS.md all say this now instead of claiming a one-time seed. - SegmentedControl builds its classNames from a filtered array (no double or trailing spaces) and only puts `disabled` on the button when true. Nits: the Cmd-only copy in the dialog note and the Settings row is now platform-aware through a new modKeyWord export ('Cmd' / 'Ctrl'), which is the prose spelling modKey's glyph cannot serve; and the dialog's radio group follows the WAI-ARIA roving-tabindex pattern, so Tab enters and leaves it as one stop while arrows move the selection and wrap. Tests: M1's exact reviewer sequence (hold Alt, open, pointer into card, scroll-close, hover another token, release, assert it closes after the grace) fails without the reset; the References-handoff mechanism (open card gone, pending dwell cancelled, in-flight request aborted) is behavioral in the hook suite, and codeNavHoverHandoff.test.ts pins the App wiring at source level the way iframeIsolation does, failing without the closeTokenHover call. Also: blur clears the held state, arrows move the radio selection, and the announcement latch no longer writes. The dialog suite additionally seeds its trigger cookie: configStore is a process-global singleton whose loadFromBackend keeps the in-memory value when the new backend is silent, so an unseeded map inherited another file's last write. The new roving-focus test is what surfaced that. * fix(review): dialog footer names the real Settings tab (Editor, not Review Display) * feat(review): jumbo hover card announcement, and rename the feature to Hover cards The announcement was too plain for a first-use dialog. It now uses the repo's big-format shell (max-w-5xl, badge/title/description header, a 1.1fr/1fr body that collapses to one column under 820px, footer with the Settings pointer and one action), matching EditModeAnnouncementDialog and LookAndFeelAnnouncementDialog. The left column, where the Edit Mode dialog plays its recording, is a worked example: a three-line strip of diff with `charge` shown under the pointer, and below it a hover card reproducing the real card's anatomy in the order TokenHoverCard renders it (name plus kind badge, approximate signature with its matched-line marker, doc line, Defined at, a reference sample ending in the overflow line). It is JSX over the existing theme tokens rather than a bitmap, so it is crisp at any DPI, follows the active palette in both light and dark, and cannot go stale against a card whose anatomy changes. It is decorative: the whole block is aria-hidden, carries nothing focusable, and every fact it shows is also stated in the prose beside it. The right column is unchanged in behavior: the same three-option radio group, the same immediate apply, the same roving-tabindex keyboard model, and the click-behavior note (now also carrying the ripgrep requirement, which the Settings row already stated). The footer keeps the Editor-tab destination d04cfce5 corrected. Rename: the user-facing name is "Hover cards" everywhere a reader meets it, in the dialog title and the Settings heading. "Token" described the implementation's unit rather than the thing the reviewer points at, and in an app full of annotations "token hover card" invites reading it as a kind of comment card. Every internal identifier is untouched: tokenHoverTrigger, tokenHoverDelay, the cookie keys, the hook and component names, and the data-token-hover-* test ids all keep their spelling, so there is no migration. Tests: the behavioral assertions are unchanged (once-ness, radio applies, dismiss and Escape keep the choice, arrows move selection), plus one that the example block is aria-hidden and holds nothing focusable, because read aloud it is a wall of invented identifiers that says nothing about the choice on offer. The guides.show CSS bundle hash moves with this: Tailwind scans source for class names, and the example introduces utilities the shared stylesheet did not carry. The JS bundle is unchanged, as expected for an app-side dialog. * feat(review): make the announcement example a live try-it on the real card The example was a hand-drawn impression of a hover card. A fork of that markup drifts from the shipped card the first time either changes, which is exactly what an example is supposed to prevent. It is now the real thing, and it is interactive: the `withRetry` token in the strip is genuinely hoverable, and resting on it opens the REAL TokenHoverCard through the REAL useTokenHover, so the reviewer feels the actual dwell, leave grace and Alt gate before committing to a setting. Nothing is reimplemented. Three thin seams carry it: - useTokenHover gains `resolve`, defaulting to the POST /api/code-nav/hover it always did. The try-it returns a hardcoded CodeNavHoverResponse instead. This is the ONLY fixture: a demo must not search the reviewer's repository for a symbol they never asked about, and a second copy of the dwell/supersession/ grace logic would drift. The resolver is read through a ref so an inline literal cannot re-arm the hook's callbacks every render. - TokenHoverCard gains `layerClassName` (default `fixed z-[60]`, unchanged). The card portals to <body> like every instance, so the one thing its host has to be able to say is which stacking context it belongs in; the try-it puts it at z-[110], above the z-[100] modal it is demonstrated inside. - TokenHoverCard gains `inert`, which drops the location buttons out of the tab order. The try-it has no References panel behind it, so they lead nowhere. The card body was extracted to TokenHoverCardContent so the positioned wrapper and any other host share one copy; every class, section and separator is the same markup the live card renders, not a duplicate. The hovered token wears the diff pane's own underline treatment, now defined once in tokenHoverStyles.ts: usePierreTheme serializes it into Pierre's shadow stylesheet as `.pn-token-hover`, the dialog applies the same declarations as a style object. Two consumers, one definition. The try-it reads the LIVE setting rather than a prop, so flipping the radio to "While holding Alt" makes the demo behave that way immediately and "Off" makes it do nothing and closes any open card. That is the honest preview of each choice, which is the point of putting the control next to the demo. Because it is interactive it is labeled rather than hidden: the region carries role="group" and an aria-label, the visible prompt line ("Try it: rest your pointer on withRetry") carries the meaning, and only the mock code lines stay aria-hidden, since read aloud they are a wall of invented identifiers. Tests: hovering the token opens the real card (asserted through the component's own data-token-hover-card marker) with the fixture's symbol and definition path after the dwell and not before; the try-it obeys the live trigger, so under "Hold Alt" a rest opens nothing until Alt goes down, and choosing Off takes an open card away; and the region is labeled while its mock code is hidden. React synthesizes onPointerEnter from pointerover, which is what the tests dispatch. The guides.show viewer hashes move on both halves: usePierreTheme is in the viewer's graph and now imports the shared style module (JS), and the try-it introduces Tailwind utilities the shared stylesheet did not carry (CSS). Budgets are unaffected (395.7 KB gz of 400). * fix(review): the try-it fixture names the token the prompt asks you to hover Two defects the browser capture caught, both invisible in the DOM tests. The prompt said "rest your pointer on withRetry" and the card that opened was titled `charge`: the fixture still described the symbol from the earlier static example. It now describes withRetry, and takes its name from EXAMPLE_SYMBOL so the prompt and the card cannot disagree again. The hovered token sat mid-line, and the card anchors at the token's left edge and is 400px wide, so it opened across the radio column the reviewer is meant to be reading. The token now leads the line, which keeps the card inside the example's own column. * feat(review): the hover card modifier gate is Cmd (Ctrl), not Alt Maintainer ruling. Two reasons, both recorded in DESIGN_hover-settings.md §4.1 in place of the old Alt argument: - Alt is very commonly bound to push-to-talk dictation, so an Alt-held gate would open cards every time the user starts speaking, with the pointer wherever they left it. That is the exact failure this setting exists to prevent. Alt-hold is also already taken inside Plannotator, by the plan editor's temporary input-method switch. - Cmd+hover is already VS Code's "tell me about this symbol" gesture: the navigable-target underline plus a definition peek under one held key. Gating on Cmd rides that muscle memory instead of competing with it. The old anti-Cmd argument is moot. The underline and the card appearing together under a held Cmd is not a collision, it IS that composite gesture; and handleCodeNavRequest already dismisses the hover surface on every References invocation, so Cmd+click cleanly supersedes an open card or a pending dwell rather than stacking with it. The key is platform-aware through two new helpers beside the existing ones in packages/ui/utils/platform.ts: `isModKeyHeld` (metaKey on macOS, ctrlKey elsewhere) and `modEventKey` ('Meta' / 'Control'). Labels come from the existing modKeyWord, so the key the code checks and the key the copy names cannot disagree. Labels updated in the dialog radio and its description, the Settings segmented control, the design doc and AGENTS.md. Only the modifier ALONE arms. Any other key going down while it is held (Cmd+C, Cmd+V, Cmd+S) disarms and takes an open card with it, so a copy performed with the pointer parked over the diff cannot pop a card mid-copy. This was cheap: the keydown handler already had to compare event.key. The typing-target guard stays as belt and braces. Blur handling stays and matters more now: on macOS the app switcher is the same key this gate arms on, so Cmd+Tab is the common way to leave with it held. Internal ids are untouched. The stored trigger value is still `modifier`, because the setting names the shape of the gate and not which key fills it, so nothing migrates. The #1461 Alt+click References alias is unrelated and is left alone. The try-it feels the new gate for free, since it drives the same hook: choosing the hold option and holding Cmd over the demo token opens the demo card. Tests: the gate suites now build their events from the platform helpers, so they exercise Cmd on macOS and Ctrl on the Linux runner rather than pinning one platform. All behavioral assertions are unchanged. Two new ones cover the chord rule, and both fail without it: a copy chord with the pointer parked on a token opens nothing, and a chord while a card is open closes it. * fix(review): pointer cursor wins over Pierre's I-beam on hovered and Cmd-held tokens * fix(review): the intro dialog's demo token gets the pointer cursor too * feat(review): hover delay default is 300ms matching VS Code; tiers respaced 150/300/700 * test(review): restore the 700-delay test's summed advances The 350 -> 300 sweep in b5538350 rewrote both advances in this test, but they SUM to the configured dwell rather than each naming it: 300+300 is 600, so a 700ms delay never elapsed and the suite went red at the tip. Back to 350+350 with the pairing spelled out, so the guard stays meaningful in both directions: the first advance is past the 300 default (a hook that ignored delayMs would have fired already) and the pair reaches exactly 700. Verified fail-closed by pinning the timeout back to DEFAULT_TOKEN_HOVER_DELAY_MS, which fails this test and nothing else. The dialog test's two advances become DEFAULT_TOKEN_HOVER_DELAY_MS itself: its dwell IS the registry default (the seeded backend says nothing about tokenHoverDelay), so naming the constant is both correct and immune to the next respacing. * fix(review): the hover gate's typing guard pierces the shadow root Two defects in one branch of the modifier gate's keydown handler. The guard read event.target, which a window-level listener receives RETARGETED to the shadow host. Pierre's edit-session editor is a contenteditable inside that shadow root, so a bare Cmd pressed while editing code in place read as a plain <div>, armed the gate, and popped a card over the text being edited: verbatim the failure "typing owns the key" exists to prevent. The three other window-level typing guards in this package (AllFilesCodeView, FileTree, SectionsPanel) already read composedPath()[0] for exactly this reason and say so in comments; this one is no longer the exception. The guard also sat ahead of the chord branch, so it suppressed DISARMING as well as arming. Focus reaches a composer between the two routinely (a card is open, the reviewer clicks into the comment box and pastes), and that left the card standing over the diff with the gate still armed behind it. The chord branch now runs first and typing only gates the arm path. keyup deliberately keeps no typing guard: if the gate never armed the branch is already a no-op, and if it did arm the release has to be honored wherever focus has since travelled. Tests: the old "modifier chords in a comment box" case exercised the chord branch, not the guard, so it passed with isTypingTarget deleted. Replaced by a bare-modifier press from a textarea and one from a shadow-root editor, plus a chord-while-typing case for the asymmetry; the two pre-existing chord-disarm tests stay, so that coverage is unchanged. The shadow event is synthesized rather than dispatched from a real shadow root because happy-dom does not retarget, which would have made the test vacuous. Each of the three fails closed against its own defect: reverting composedPath fails only the shadow case, deleting the guard fails both typing cases, and moving the guard back above the chord branch fails only the asymmetry case. * feat(review): the modifier gate paints the navigable-target affordance The ruling that settled Cmd over Alt was that the underline and the card appearing together under one held key IS the composite gesture rather than a collision. The code did not deliver it: the diff views paint pn-token-nav from the pointer ENTER event, and neither half of the gesture this mode exists for fires one. Parking on a symbol and THEN holding the key opened a card on a token wearing no affordance, and releasing closed the card while leaving the affordance painted until the pointer eventually left. useTokenHover now reports the arm and disarm transitions through an optional onModifierGate(armed, tokenElement), always carrying the token the pointer is parked on RIGHT NOW so a disarm also unpaints a neighbour the pointer drifted onto while the key was down. Every disarm route is covered: chord, release, blur, and the effect cleanup when the reviewer switches trigger with the key still held. The release notifies ahead of the reading-the-card early return, because the key being up ends the affordance whether or not the card is being read. App owns the painting, not the hook: the diff views are compiled into the portable guides.show viewer and their prop signatures must not move, and the class is a review-app concern. Enter-time painting is untouched, so plain Cmd+hover in the default hover mode behaves exactly as before. Also gates pn-token-hover on a hover handler actually being wired, and paints it in AllFilesCodeView. That class carries a cursor: pointer that became !important in eebab519, so painting it unconditionally started promising clickability on every token in a session with hover cards switched off; and leaving it out of the all-files view meant the affordance the announcement's try-it demonstrates was missing from the default review surface. A read-only or portable host passes no hover handler and still gets neither class. Fail-closed: dropping notifyGate(true) fails both new hook tests, dropping the App wiring fails the App pin, and making the DiffViewer paint unconditional fails the gating pin. * fix(review): hover card cleanups, and the design's delay tiers Comments first, because several of them had become false and this repo treats them as contract. The dialog cited a StaticTokenHoverCard that no longer exists and a card that "lives inside an aria-hidden decorative block" (it does not; the try-it region is a labelled role=group), the try-it still named "the Alt gate", and two hook comments plus four test comments still described the trigger key as Alt or claimed the Alt+click alias and the trigger share a key, which stopped being true when the gate became Cmd. The focus trap's rationale claimed the example card's buttons were in its query; the card portals to <body>, so they never were. tokenHoverStyles now single-sources what it says it does. Only thickness and offset were actually shared; the decoration line and the cursor were written twice and could drift. Both forms read one object, and the two genuinely per-form bits (the resolved color, and the !important the shadow form needs to beat Pierre's I-beam) are named as such. The display-only card stops advertising clicks it swallows: its own footer says "Click a location to jump", so under `inert` the locations lose the hand cursor and hover underline as well as the tab order, and the card gets aria-hidden — it portals outside the aria-modal dialog it belongs to, where a screen reader would meet it as a stray tooltip. The announcement's availability latch moves from the render body into an effect: a ref written during render is a side effect React may discard. The gate independently requires !isLoading, so the ordering is unchanged. Both announcement test files now re-hydrate configStore in afterEach. They re-resolve a process-global singleton from a two-entry fake map, and the pure lane runs in one process, so they were handing their own defaults to whatever file ran next. The dialog test gains the two assertions that were missing: the demo card sits at z-[110] (the layerClassName seam's entire purpose is not being buried under the modal it is demonstrated inside) and its locations are non-interactive. Both fail closed against dropping the respective prop. Design doc: §4.2 still listed 200/350/700 with a 350 default, which the maintainer ruling in §6 had already replaced; §3's 350 now reads as the history it is; §6 records how the composite gesture is delivered and §9 how the hovered-token treatment is gated. * chore(core): re-pin the guide viewer build The manifest was synced before the last three commits of this branch and went stale: usePierreTheme (which serializes the token-hover CSS into Pierre's stylesheet), the token-hover default constant, and now AllFilesCodeView are all inside the viewer bundle, so the JS hash moved while the CSS hash did not — the .pn-token-hover rule is a JS template string, not a stylesheet entry, which is exactly why the drift was easy to miss by eye. check:manifest was failing at the tip. Rebuilt and synced last, so this pins the branch's final source. Budgets after: js 395.8 KB gz against a 400 KB budget, css 50.9 against 64, worker still import-free.	2026-09-02 23:59:18 -07:00
Michael Ramos	91ad7c95fc	feat(review): token hover cards, Tier 0 (#1461) ... * feat(review): add POST /api/code-nav/hover in both runtimes Tier 0 of the token hover card: the hover pipeline is the ripgrep search /resolve already runs, plus three cheap enrichments read off the same machinery. - packages/shared/code-nav.ts: definition patterns carry the kind they prove (alternations split one-per-kind, so definition-vs-reference classification is byte-identical and classifyMatch becomes a thin wrapper over classifyMatchDetailed); scanDocComment, buildSignature and resolveCodeNavHover; an additive timeoutMs option on resolveCodeNav so hover can ask for 3s while /resolve keeps its 5s; an optional readFile member on CodeNavRuntime so /resolve callers stay unchanged. - Both review servers gain /api/code-nav/hover behind the same guard stack as /resolve. /resolve itself is untouched. The doc scan is conservative by construction: per-language, blank-line separated, capped, and null for an unknown language. Returning nothing always beats returning garbage. * feat(review): token hover cards in the code-review diff Resting the pointer on a symbol opens a card with where it is defined, an approximate signature, its doc comment if the scan found a real one, and a sample of its references. Every location on the card routes into the same References panel Cmd+click opens. - utils/stitchTokenIdentifier: rebuilds one identifier from the token spans Shiki fragmented it into, using each span's data-char column to prove adjacency. It stops at dots (rg searches with --word-regexp, where a dotted path matches nothing) and refuses keywords and one-character names, which is what keeps most hovers off the wire entirely. - hooks/useTokenHover: 350ms dwell before any request exists, one in-flight request aborted by its successor, a 30-entry LRU flushed whenever the diff snapshot changes, a 250ms leave grace so the card's own links are reachable, and a scroll/wheel cancel because the anchor rect is stale the moment the pane moves. An unavailable backend, a failure, a timeout and a thin answer all render nothing, silently: a hover is an idle gesture and must never nag. - components/TokenHoverCard: portaled to body so it escapes the Dockview overflow and stacking context; anchored below the token, flipped above when the viewport would clip it. It shows what the search found and nothing it did not, so uncertainty is a second location line rather than a description of the ranking. - Wiring: two optional props beside onCodeNavRequest in both diff views, passed only when the existing live-workspace gate AND the new cookie-only "Token hover cards" setting are on. Off means no listeners, no requests and no card in the tree. Alt+click joins Cmd+click as an unadvertised alias into the References panel; the meta/ctrl branch is unchanged. The guides.show viewer manifest moves with this: AllFilesCodeView is in the portable viewer's graph, so its new optional props shift the bundle hash. * fix(review): correct token hover supersession, scroll and doc-scan defects Review findings, each with the regression test that fails without the fix. - An open card could be rewritten by a NEIGHBOUR's answer: drifting onto an adjacent token launched its request, and returning to the open card took the same-key early return without reclaiming the active key, so the neighbour's answer still passed the landing check. The early return now reclaims the key, kills the pending dwell, and aborts a foreign request. - Re-entering a token inside the leave grace re-armed the dwell while that token's own request was still in flight, spawning a second ripgrep for an answer already on its way. The dwell now joins the in-flight request, and the answer anchors to the span the pointer is on now. - Scrolling INSIDE the card closed it, which made the signature block's horizontal scroller unreadable by the gesture meant to read it. The cancel now ignores events originating in the card's own subtree; a pane scroll still closes it. - A below-threshold answer for a different token left the previous token's card standing over a symbol the reviewer had already left. - The doc scan rendered tooling directives as documentation. Directives are dropped from BOTH ENDS of the comment run — eslint-disable, @ts-*, prettier-ignore, biome-ignore, istanbul ignore, noqa, type: ignore and triple-slash references — because the commonest real position is the line immediately above the definition, which is the trailing end of the run as collected. Never from the middle: a directive surrounded by prose sits inside documentation we would have to interpret to cut safely. A run that is nothing but directives returns null, and prose that merely mentions a directive is untouched. Also: an answer whose token has been recycled out of the DOM opens no card (a detached rect is 0,0 and would pin it to the viewport corner); a flipped card is clamped to the top edge; the card is a tooltip, not a dialog; a location click describes the CLICKED location rather than forwarding the hover's charStart and language into another file; definition.preview stays declared but unpopulated until a consumer exists; the overflow line regains its leading ellipsis and now renders under the banned-vocabulary sweep. Portable viewer: the hover prop is inverted to (props, filePath) so the two diff views import nothing new and stitching lives in App. The read-only guide viewer bundle no longer carries the stitcher or the request builder.	2026-09-02 19:00:39 -07:00
Michael Ramos	50d5320a8e	fix(ui): collapse the non-gate empty menu to one Send a note composer; purge em dashes from decision copy ... Maintainer rulings: the non-gated annotate empty state offered 'Done with a note…' and 'Request changes…' whose only difference was the approval-framing sentence on the same /api/feedback transport - they collapse into a single 'Send a note…' item (id 'request-changes', plain feedback, no framing). The approvalFraming machinery stays for the non-gated discard path and gate mode; the non-gate 'note-with-approval' route is now dead code pinned as unframed feedback so a stray dispatch can never fabricate approval. And no user-facing decision-control string carries an em dash any more: titles, subtitles, composer action labels, confirm copy, and the two Mod+Enter shortcut descriptions are rewritten with plain punctuation (agent-facing prompt constants and frozen labels untouched).	2026-09-02 14:06:25 -07:00
Michael Ramos	36d59d3bbd	fix(ui): vim/popover Escape ownership, ladder defaultPrevented, doc-flow accuracy (PR7 review) ... F1: with vim enabled, Escape on an open ActionMenu/ApproveDropdown/ DecisionControl was consumed by useVimDocumentFocus (registered first on document, preventDefaults while reclaiming focus; the popover hook then skips the defaultPrevented event) and the menu stayed open. Open popovers now stamp data-pn-dismissable-popover and the vim hook's BLOCKING_OVERLAY_SELECTOR treats them as blocking overlays, so vim defers. Pinned in ActionMenu.test.tsx (verified failing without the selector entry). F2: the plan editor's diff-exit Escape listener gains the defaultPrevented early-return so a popover-consumed Escape no longer also exits the diff view. Audit of the remaining document-level Escape listeners found only component-internal ones (Settings theme preview, which already consumes; dialog focus traps handle Tab, not Escape) — left alone. F7: onDismiss callbacks memoized in ActionMenu/ApproveDropdown so useDismissablePopover stops re-registering listeners every render while open. F8: ActionMenu.test.tsx, PlanHeaderMenu.mobile.test.tsx, and ReviewHeaderMenu.mobile.test.tsx were DOM-gated but never registered in the CI DOM lane; added to the test.yml file list so they actually run (the F1 pin lives in a lane-registered file). F3-F5 (docs): test-opencode-review.sh stdout describes the decision control (Approve at zero) instead of the removed always-on Send Feedback button; UI-TESTING.md's platform-mode flow uses the real positional PR URL; the AGENTS.md annotate flow diagram ends on the adaptive primary like the review diagram.	2026-09-02 13:10:47 -07:00
Michael Ramos	dd68e339fb	docs: decision-control manual QA checklist, stack docs coherence, ui 0.38.0 handoff accuracy (PR7) ... tests/UI-TESTING.md gains the Decision Control Manual Checklist (annotate file/gate/folder/last, HTML/live-app iframe dismissal, review agent + platform modes, compact/touch, sidebar general comment) and fixes the test-opencode-review.sh flow that asserted a Send Feedback button at zero annotations. AGENTS.md: the code-review flow diagram stops claiming Approve sends the literal LGTM (gone since the approve-with-notes delivery), and the shortcuts-root scope list names decisionControl.shortcuts.ts beside history.shortcuts.ts. packages/ui/HANDOFF.md's 0.38.0 line now covers the whole stack accurately: internal primitives stay off the host-supported surface, decisionControlShortcuts rides the blessed shortcuts barrel, and the ToolbarButtons muted-prop removal is internal. The stack ships as one unpublished 0.38.0 — no further version bump (spec §2.1/§9: batch the bump into PR1 and PR7; nothing was published in between).	2026-09-02 12:47:39 -07:00
Michael Ramos	4cbdbb269d	fix(review): stage-review fixes — doc truth, dead muted prop, tooltip a11y, item mute guard ... - AGENTS.md: the platform-mode sentence now describes the shipped PR6 state (ghost-X + DecisionControl, no composer, everything through the existing ReviewSubmissionDialog, self-approval mute preserved) instead of the pre-PR6 three-button row. - ToolbarButtons: delete the now-dead `muted` prop from FeedbackButton and ApproveButton — PR6 removed its last consumer. `dimmed` stays (live plan- mode consumer in packages/editor AppHeader); ToolbarButtons is not on the host-supported README surface, so the removal is internal. - DecisionControl: the muted primary's reason now renders through the shared Tooltip (Base UI trigger wires floating-ui useFocus, so it shows on hover AND keyboard focus-visible) plus a persistent aria-describedby description via a hidden span; the native title is dropped for the muted case only so the tooltip is not doubled. Frozen sentence unchanged. - runPlatformDecisionAction: structural mute guard for the approve ITEM ids too — a muted item's handler is inert from any surface, not only where the DOM disables the row. - DOM test: the empty+self menu pins initial focus on the first non-disabled row (the roving-focus :not(:disabled) skip now has a regression pin), and the muted-primary assertions follow the reason to aria-describedby. Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9	2026-09-02 12:28:42 -07:00
Michael Ramos	2f9b831617	fix(review): stage-review fixes — with-notes framing, bridge handshake, PR-payload advert, note fold ... Applies the PR5 stage-review rulings: M0: approve-time feedback is no longer appended raw after the approved prompt ("no changes requested" beside a change-request-shaped export read as a contradiction). composeReviewApprovedMessage now resolves the new DEFAULT_REVIEW_APPROVED_WITH_NOTES_PROMPT (configurable as prompts.review.approvedWithNotes; field added to the config review section), which frames the notes as non-blocking guidance and says not to revise or reopen. Signature is now (runtime, feedback, config) so one function fixes all four consumers; re-vendored to Pi; prompts.test.ts pins the framing, the config override, the byte-identical bare approval, and the legacy placeholder filter; cli-bridge.test.ts asserts the bridge routes through the composer. M1: fail-closed approval-notes handshake for the OpenCode CLI bridge. The plugin declares supportsApprovalNotes: true on the opencode-review stdin JSON; the binary adverts approvalNotesSupported for opencode ONLY when the declaration is present, so a new binary + old plugin (advert in the binary, delivery in the independently-versioned plugin) renders no approve-carrying items instead of silently dropping the reviewer's note. supportsReviewApprovalNotes stays the seam; documented at both ends beside the existing version-skew reasoning. Pinned end to end by apps/hook/server/opencode-review-advert.test.ts, which spawns the real entrypoint and reads /api/diff: stdin without the declaration serves false, with it true. m1: applyPRResponse re-applies the advert from the PR-family payloads (pr-diff-scope, pr-switch, load-full-diff), so the client stays in lockstep with whatever diff payload it last applied — the "whole diff family" comment is now literally true. m2: buildReviewApprovalBody folds a note in ahead of the export when annotations also ride, so a future combined item cannot lose data; pinned with a pure assertion. i1: AGENTS.md corrected — the standalone dev server emits raw decision JSON with unfiltered feedback and does not route through the composer; the consumer list, framing, and handshake are now described accurately. Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9	2026-09-02 11:50:45 -07:00
Michael Ramos	673ffb3227	docs: describe the shipped approval-notes advert and approve-with-notes delivery ... Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9	2026-09-02 11:19:37 -07:00
Michael Ramos	dc96419012	fix(review): stage-review fixes — panel-clamped composer, lifted draft, in-flight guard ... - M1: clamp the general-comment popover to the sidebar panel width (inline maxWidth = panelWidth - 32) so it never clips inside OverlayScrollArea's overflow-x hidden at the 200px end of the persisted 200-600px range; the viewport-relative class remains only as the overlay-presentation guard. - L2: lift the composer's open + draft state into ReviewSidebar, shared by both placements, so an SSE placement flip (empty state -> General header) or a tab switch keeps a half-typed draft; sidebar collapse discards it. - L1: guard handleAddGeneralComment against submitted/in-flight decisions so a commit during a decision POST cannot vanish from wire and archive. - I1: AGENTS.md — platform-mode presence is deliberate (rides the pr-action body via scope:'general'), and honest draft-lifetime wording. - NIT: the empty-commit test also asserts the refocus contract; new flip test drives the draft survival through the parent. Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9	2026-09-02 10:50:57 -07:00
Michael Ramos	7f9ff75bcf	docs: describe the sidebar general-comment producer and the archived scope field	2026-09-02 10:08:41 -07:00
Michael Ramos	9bdabf5f8a	fix(review): stage-review fixes — confirm-dialog Mod+Enter guard, armed-note failure coverage, loud PR5-gated refusal ... HIGH-1: one Mod+Enter over the open discard confirm posted TWO contradictory decisions — the app's window-level Mod+Enter effect fired submitPrimaryDecision (approved:false send) while ConfirmDialog's own window-level handler fired onConfirm -> handleApprove (approved:true LGTM) from the same event; stopPropagation cannot stop same-target listeners. Fixed with PR2's idiom: the review Mod+Enter effect bails on the data-plannotator-confirm-dialog sentinel, which covers both the DecisionControl-internal confirm and the compact confirm. Pinned by a DOM repro test asserting exactly ONE POST (the confirm's LGTM body); revert-verified — with the guard neutralized the test fails with two recorded POSTs. MEDIUM-1: added the armed-note failure-path DOM test (500-then-200 double): a failed note POST keeps the scope:'general' note in state with the primary flipped to Send Feedback, the next primary click retries the same note-carrying body, and success clears the armed decision so a subsequent Mod+Enter dispatches nothing. MEDIUM-2: the approve-with-notes refusal now console.errors naming PR5, and the route object carries implemented:false. The advert constant moved to reviewDecision.ts (REVIEW_APPROVAL_NOTES_SUPPORTED) so the new pure contract test can pin 'the advert never emits an id whose route is an unimplemented refusal' — flipping the advert in PR5 without wiring delivery fails the test, and deleting the constant without updating it breaks at import. Docs: the review-header section now records F8 (Approve primary icon-only below lg where ApproveButton showed OK; title carries the accessible name), the two-click discard replacing the dimmed one-click Approve, the Mod+Enter confirm-ownership rule, and the accepted compact-composer draft asymmetry. Also trimmed the pure routing test's per-id echo assertions to the load-bearing equalities.	2026-09-02 09:49:22 -07:00
Michael Ramos	97ef5a4eae	docs: describe the unified review header decision control	2026-09-02 09:27:34 -07:00
Michael Ramos	579ca6ec88	docs: describe the unified annotate header decision control	2026-09-02 08:35:06 -07:00
Michael Ramos	990f3e8905	feat(server): durable feedback archive for every submitted review (#1438) ... * feat(server): archive every submitted review to a durable local feedback store Submitted feedback was only as durable as the agent session that asked for it. Code review persisted nothing at all: /api/feedback deleted the draft, settled the decision promise, and if the invoking agent had already timed out the review existed nowhere (the failure #678 fixed for annotate). Plan decisions only reached plans/ while the client-side planSave setting was on, and repeat decisions on one plan overwrote each other. Annotate kept the #678 record for single local files only. Every submission now appends one record to ${PLANNOTATOR_DATA_DIR}/feedback/{project}/index.jsonl, plus a records/{stamp}-{surface}-{decision}.md sidecar when it carries content, written at decision settlement time inside the servers so all nine agent frontends are covered by two implementations. Surfaces wired in both runtimes: plan approve and deny, code review /api/feedback (Send Feedback, Approve, LGTM) and /api/exit, annotate submit, approve and exit. Bare approvals, LGTMs and dismissals are decision-only JSONL lines with no sidecar. Records are cheap by design. Code review carries diff identity (vcsType, diffType, base, gitRef, snapshotId, cwd, PR metadata, changed-file count, patch byte count) and never the patch bytes; plan records carry the decision text plus a reference to the history/{project}/{slug}/NNN.md version the decision was made on rather than a second copy of the plan. Annotation provenance (source, author) is preserved, so external, review-agent and WebMCP findings stay tagged and source == null selects the reviewer's own comments. The shared module never throws: an archive failure is logged, degrades silently for the user, and keeps the annotation draft as the recovery copy. The append happens before deleteDraft, generalizing the #678 ordering. Controlled by PLANNOTATOR_FEEDBACK_HISTORY / feedbackHistory (default on). PLANNOTATOR_ANNOTATE_HISTORY=0 additionally suppresses records for every annotate surface, so the documented stateless-annotate promise still holds. "feedback" is added to PURGE_OWNED_TOP_LEVEL so uninstall purge removes it. AI-assisted (Claude) under maintainer direction. * fix(server): stop the feedback archive from writing into the real data dir in tests Review findings on the durable feedback archive. 1. The archive is default-on, and most server tests boot a real plan, review, or annotate server without redirecting PLANNOTATOR_DATA_DIR, so `bun test` deposited records in the contributor's own ~/.plannotator/feedback (24 files across 12 buckets from two test files alone) on CI and every machine. A new bunfig test preload, tests/setup/feedback-archive-off.ts, turns the archive off for the suite; the archive's own tests opt back in inside their test bodies, which is also how they exercise the opt-out. Those tests now use distinctive project names and remove the annotate history they leave in the real data dir, since storage.ts fixes its data directory at import time. 2. PR reviews bucketed under feedback/pr-<n>/. PR mode never sets gitContext and --local points agentCwd at a pool/pr-<n> checkout, so deriving the project from the review cwd was wrong. ReviewServerOptions now takes a `project` option, mirroring the annotate server, preferred over the cwd derivation on both runtimes; the Claude Code, OpenCode, and Pi entry points pass their already-computed detectProjectName() result. 3. changedFiles overcounted renames: extractChangedFiles unions the a/ and b/ sides so a reader can resolve either path. The record now counts b-side paths through countChangedFiles, so a rename is one file. 4. Docs: the feedback archive is added to the privacy page and PLANNOTATOR_FEEDBACK_HISTORY (plus PLANNOTATOR_ANNOTATE_HISTORY) to the environment variables reference. The overclaim that every submitted review is archived is corrected: a review posted straight to GitHub or GitLab through /api/pr-action is not archived locally yet. Three behaviors are now written down: O_APPEND is not atomic on NFS or SMB and a genuine interleave damages both records that raced, folder-session records carry the folder path rather than the open document, and URL-session records store the full URL including its query string. 5. Pi parity: the Node mirror now has the failed-archive-write test (the one invariant its handler copies by hand) and the PR-mode bucketing test. Comments only, no behavior change: the pool checkout recorded in target.review.cwd can be cleaned up before anyone reads the record, and getPlanVersionPath resolves the data directory storage.ts captured at import while the archive resolves it per call. AI-assisted (Claude) under maintainer direction. * docs(server): make the feedback index an explicit multi-client contract plannotator-tui will append to the same feedback/{project}/index.jsonl with client "plannotator-tui", so the module's stance of "a client tool may emit this shape under its own clients/ namespace" is out of date. The index is one shared source of records, labeled by client. 1. The module docstring and the FEEDBACK_RECORD_CLIENT comment now describe the shared index: several tools append to the same file, separated by `client`; plannotator-tui is a known second writer, herdr-annotate is reserved, and `client` is an open set rather than an enum to validate against. 2. Two optional fields are declared so v1 reserves their names across clients: target.agent ({ host, session, transcript }) for surfaces whose subject is an agent session rather than a file or a diff, and top-level clientVersion. Neither is populated here. clientVersion stays unset deliberately: there is no runtime-agnostic version constant in packages/shared, and reading package.json from a vendored module would be a new filesystem dependency for cosmetic data. 3. Sidecar naming is documented at the naming site and in AGENTS.md: other clients suffix their id ({stamp}-{surface}-{decision}-plannotator-tui.md), so recordFile values carrying such suffixes are valid and nothing may parse a sidecar name. Nothing in this repo did: every consumer treats recordFile as an opaque handle and no test pins a filename pattern. A new test appends a foreign line (unknown client, unknown fields, suffixed recordFile) and pins that the reader keeps it. 4. Honesty fix to the atomicity comments, in code and in AGENTS.md: appendFileSync loops internally, so "one write syscall" was wrong even on a local filesystem. The real model is that a line-sized buffer handed to a single append-mode write completes without interleaving in practice locally, with the reader's skip-unparsable tolerance as the backstop and the NFS/SMB caveat unchanged. 5. Exhausting the sidecar collision counter now throws a named error instead of re-throwing a bare EEXIST, so the server log says what actually happened: 100 taken names in one millisecond means a stopped clock or a runaway writer, not a transient disk problem. 6. AGENTS.md and the parseFeedbackIndex doc state the reader contract: lines are gated on a numeric `v` and unparsable ones are skipped, so analyzers that depend on v1 semantics should filter v <= 1 themselves. Fields are added, never repurposed, so a v2 would mean a real shape change. AI-assisted (Claude) under maintainer direction.	2026-09-01 10:56:39 -07:00
Michael Ramos	a79e6b0efe	fix(ai): stop leaking opencode serve processes (#1445) ... * fix(ai): stop leaking opencode serve processes Every server boot with the opencode CLI on PATH eagerly called the provider's fetchModels() to fill the Ask AI dropdown, which spawned (or attached to) an 'opencode serve' on the shared default port 4096. Dispose only ran on the clean decision path, so Ctrl-C orphaned the child, and every later session attached to the orphan and piled unevictable per-directory instances into it (multi-GB over a day of normal use). Three changes: - Lazy start: opencode model discovery moves onto the same deferred provider initializer Codex uses. Nothing spawns until the user activates opencode in Ask AI (?activate= from the model picker, or the first opencode session). The picker still lists the provider with an empty model list pre-activation, exactly like Codex. - Own server per process: spawn with port 0 (OS-assigned; the SDK reads the real URL from the child's listening line) and never attach to a server we did not spawn. An explicitly configured port is honored. - Exit cleanup: a process 'exit' handler closes the spawned server (SIGINT/SIGTERM are routed through process.exit by the CLI), removed again on dispose. No SIGHUP listener, preserving nohup. Both runtimes; regression tests mock the SDK so no real server spawns. * fix(ai): close review findings on the opencode lifecycle Independent review of the leak fix found two holes, both now closed and regression-tested against the mocked SDK: - A failure after the spawn (client construction) left the child running and its exit handler registered, and because the handler read this.server late instead of capturing its own server, a retry's second spawn made the first unreachable by any cleanup. doStart now captures the server in its handler closure and reaps child + handler on any post-spawn failure. - dispose() during an in-flight spawn was a no-op the completing spawn then undid, resurrecting a disposed provider with a live child and a fresh exit handler. dispose() now bumps a start epoch; a spawn that completes past its epoch reaps its own server and rejects, and the provider remains restartable afterwards. Also documents the OpenCode transport (per-process server, deferred discovery) beside the Codex note in AGENTS.md.	2026-09-01 10:55:17 -07:00
Michael Ramos	4465950f0c	feat: add bounded annotation undo and redo (#1426) ... * feat: add bounded undo and redo history * fix: address undo redo review feedback * chore: refresh guide viewer manifest * fix undo history review regressions * chore: refresh guide viewer manifest	2026-08-31 09:12:13 -07:00
Michael Ramos	c2950e709f	fix: pre-release QA findings for 0.27.9 (#1405) ... Fixes from the 0.27.9 pre-release review. Servers: an unreadable rendered-HTML root falls back to the startup snapshot on both runtimes with a once-per-process warning instead of hanging (Pi) or answering 500 (Bun); the version diff is recomputed against current bytes on reload and carried through the in-app Refresh instead of being dropped, with no history write on a GET. Client: a Refresh action on the compact touch shell; HtmlSurfaceControls renders Refresh independently of the eye; the dead HtmlSurfaceActions removed. Threading: one linear, cycle-safe reply resolution shared by the annotations panel, its sort, and the export (5,000-chain tests), PATCH ingest on both runtimes rejects self-references and cycles, nothing is ever dropped from feedback. WebMCP and viewer hygiene: bounded tombstone and request memories, per-instance minted ids, nudge id caps, waiter cleanup on unmount, a shared retry epoch for diagram blocks. Docs: HTML Refresh documented, the WebMCP design pointer fixed, marketing pages updated. AI-assisted (Claude) under maintainer direction.	2026-08-27 15:23:28 -07:00
Michael Ramos	6903d7a3dd	feat(webmcp): expose plan review and annotate as WebMCP tools for browser agents (#1393) ... Phase 1 of WebMCP support: a zero-dependency, feature-detected engine in packages/ui/webmcp plus a read-and-comment tool catalog for plan review and annotate (read_document, add_comments, update_comment, remove_comments, reveal, nudge_user, list_documents). No decision tools; the human approves. Zero footprint in browsers without document.modelContext (DOM, network, console, timers, and cookies identical to main), idle until called where the API exists, and never registered inside the annotate iframes. Adds an optional inReplyTo field on annotations for threaded replies. Client-only; no server changes. AI-assisted (Claude) under maintainer direction.	2026-08-26 14:39:37 -07:00
Michael Ramos	db86d38ca4	feat(skills): top-level plannotator knowledge skill, per-host install, and plannotator.ai/llms.txt (#1377) ... * feat(skills): add the plannotator knowledge-layer skill with a CLI freshness guard A new model-invocable core skill (apps/skills/core/plannotator) that teaches an agent the whole CLI surface: decision guide, per-command reference with flags and exit codes, env vars, the external-annotations API, and a do-not list. The existing plannotator-* core skills stay lightweight action stubs. A freshness test (apps/hook/server/plannotator-skill-reference.test.ts) parses the skill's documented subcommands and flags and diffs them against cli.ts usage text plus the CLI arg-parsing sources, in both directions, so the reference cannot drift from the real CLI without failing the suite. Installers copy the single-sourced core body into ~/.claude/skills and ~/.agents/skills on all three platforms; uninstall removes it from both scopes. The skill ships model-invocable as a documented exception to the locked-by-default rule, asserted both ways in install.test.ts. * feat(marketing): serve the plannotator knowledge skill as /llms.txt Single-sourced at build time from apps/skills/core/plannotator/SKILL.md per the llmstxt.org spec (H1, blockquote, detail sections, Docs link list), so the CLI freshness guard transitively keeps llms.txt current. * fix(skills): reach every install path with the plannotator knowledge skill The knowledge skill reached Claude Code and ~/.agents but was missing from three install paths. Six fixes from the install-reach review of #1377. Kiro: the installer's Kiro leg copied only the two action skills, so Kiro users got launchers and no CLI reference. One copy line per installer, and "plannotator" joins uninstall.ts's KIRO_SKILLS. OpenCode npm: @plannotator/opencode's postinstall copied only commands/*.md. The package now ships the skill (copied at build time like the HTML assets, gitignored so the shipped copy cannot drift) and postinstall places it under ${XDG_CONFIG_HOME:-$HOME/.config}/opencode/skills/plannotator/, which is a path OpenCode really scans ({skill,skills}/**/SKILL.md under xdgConfig/ opencode). Uninstall sweeps it, skills only, so a user's own opencode/commands/plannotator.md stays out of scope. Pi npm: vendor.sh copies the skill to apps/pi-extension/skills/plannotator/ and package.json declares it under pi.skills, which Pi resolves relative to the package root. Neither vendored copy carries the // @generated header the .ts files use: a SKILL.md must open with its frontmatter on line 1. llms.txt: the endpoint resolved the skill through process.cwd(), which breaks under any invocation but --cwd apps/marketing. new URL(import.meta.url) does not fix it either, because Vite rewrites import.meta.url to the emitted SSR chunk's location. Inlined with Vite's ?raw, resolved by the bundler relative to the source file. Also drops the summary paragraph the required blockquote already carries; SKILL.md itself is unchanged. Uninstall: KNOWLEDGE_SKILLS is a separate list from CORE_SKILLS precisely so the bare name "plannotator" cannot leak into LEGACY_COMMAND_NAMES or STALE_CODEX_SKILLS and delete a user's own files. Nothing tested that; now a test proves the five installed scopes are removed and commands/plannotator.md (Claude and OpenCode) plus ~/.codex/skills/plannotator survive. Also cleanupStaleSkillLayout now knows KNOWLEDGE_SKILLS. Origins: oh-my-pi (#1373) was missing from SKILL.md's PLANNOTATOR_ORIGIN row. The guard now imports AGENT_CONFIG and asserts the row names every key and invents none, and its header comment is narrowed to what it actually proves: bidirectional for subcommands and origins, one-directional for flags. AI-assisted (Claude) under maintainer direction.	2026-08-22 12:07:42 -07:00
FND	8a8d0544c6	feat: detect the oh-my-pi harness as its own agent origin (#1373) ... * feat: detect the oh-my-pi harness as its own agent origin - omp injects OMPCODE=1 (+ a CLAUDECODE=1 compat shim) into every Bash-tool child; the env chain now maps it to a dedicated oh-my-pi origin labeled "Oh My Pi". - Distinct origin rather than aliasing claude-code, because the claude-code-only gates (permission-mode setup, permissionMode in approve) presuppose a PermissionRequest hook that a bash-invoked plannotator never has; omp has no approve support and no planning integration yet. - Fallback deliberately left "claude-code"; wording unchanged. - No dedicated Ask AI provider for oh-my-pi. * chore: sync lockfile workspace versions * fix: check OMPCODE last so runtimes inside an OMP session keep their label OMP exports OMPCODE into every shell it spawns. With the check at the top of the chain, opencode/codex/... launched from an OMP session inherited OMPCODE and were mislabeled "Oh My Pi". Moving it just above the terminal fallback lets specific runtime env vars win; OMPCODE still beats the claude-code fallback for bare shells. Reported by backnotprop in #1373.	2026-08-21 18:06:23 -07:00
Michael Ramos	89f0b6628e	feat(pi): live local app annotation through a shared proxy core and Node transport (#1366) ... Phase 2 of live app annotation: full parity on Pi over one shared implementation instead of drifting copies. - Extract every proxy decision into packages/shared/live-proxy-core.ts (HTML injector state machine, loopback/Host/Origin predicates, CSP/X-Frame-Options policy, redirect rewrite, WS origin gate, bridge assembly, liveAppDraftIdentity) and the CLI probe + live-mode messages into packages/shared/live-probe.ts. packages/server/live-proxy.ts is now a thin Bun transport over the core; its test suite passes unmodified. - Add packages/shared/live-proxy-node.ts, the node:http transport the Pi extension runs: streaming request/response piping through the shared injector, and WebSocket (HMR) passthrough that replays the client's handshake upstream over raw TCP and pipes the sockets byte-for-byte. Transport tests run the proxy in a real node child process, because Bun's node:http shim drops writes to an upgrade event's socket. - Wire Pi: /plannotator-annotate probes loopback URLs live-first with the shared probe (same 3s timeout, same <500 gate, same messages), recognizes --app/--static via parseAnnotateArgs's liveFlags opt-in (OpenCode deliberately does not opt in), and serves mode annotate-app from serverAnnotate.ts with the shared per-target draft identity, live sessions excluded from history/submissions, the remote hard-off throw, and guarded live-proxy shutdown. - Vendor live-proxy-core/live-probe/live-proxy-node plus the dependency-free bridge-script constants to generated/. - Docs: AGENTS.md phase-gate passages, marketing annotate page, Pi README.	2026-08-21 10:29:29 -07:00
Michael Ramos	f4756493cf	docs: align AGENTS.md and public docs with v0.27.5 behavior (#1361) ... Corrects staleness that landed with the live local app annotation work (#1352) and the configurable Agent TUI placement (#1050). - AGENTS.md "Session shape" no longer claims drag selection is disabled in live mode. Drag-select commenting is always live on HTML and live surfaces, in both the armed and Interact states. - Adds the shared HTML/live interaction model to the canonical "## Annotation System" section: pinpoint armed by default, the Esc ladder, pen and Mod+Shift+A re-arm, comment-only clamping at the postMessage trust boundary, no toolstrip, and the header eye toggle. - Documents the agentTerminalSide and agentTerminalDefaultAgent config-only settings, including registry precedence. - Corrects the plan-review shortcut scope list to its actual 12 files. - Marketing docs: documents live local app annotation on the annotate command page and the remote-mode refusal in the env var reference. - Fixes a stale code comment in App.tsx that contradicted the code restoring the toolsHidden flag 17 lines below it.	2026-08-21 09:05:28 -07:00
Michael Ramos	2ca55c8332	feat(annotate): live local app annotation through a loopback reverse proxy (#1352) ... * feat(bridge): additive live-mode gate + LIVE_BRIDGE_BOOTSTRAP Adds the config-gated live branch to BRIDGE_SCRIPT: frame gate, pinned parent origin, token-stamped postToParent, origin+token checks on both inbound handlers, pinpoint-only clamp, vim and resize off, pageUrl on ready, and coalesced page-change reporting for SPA history navigation. With no config present (srcdoc) every branch is inert and behavior is unchanged; the existing html-viewer suites pass unmodified as the regression proof. LIVE_BRIDGE_BOOTSTRAP installs the annotation CSS from the JSON config prelude before the IIFE runs. New package export exposes the string constants without the React barrel. * feat(ui): live-session parent side for proxied app annotation useHtmlAnnotation gains a live option (origin + token validated before parseBridgeMessage; token + concrete targetOrigin on every outbound post) and a validated page-change message with onPageChange. HtmlViewer gains src/liveSession/currentPageUrl/onPageChange: src-mode iframe with no sandbox and no srcdoc, ready pageUrl handling, per-page restore filtering with explicit clear-marks + re-sync on navigation, and one postToBridge choke point for its direct posts. Annotation.pageUrl is additive; exportAnnotations groups by page (with global numbering kept) only when a pageUrl is present, byte-identical otherwise. AnnotationPanel shows the page label; AnnotationToolstrip can hide the input switch. The editor app wires mode annotate-app: full-viewport live surface, forced pinpoint, vim off, diff/share hidden, pageUrl stamping. * feat(server): loopback reverse proxy for live app annotation Whole-origin mirror of a local dev server on a dedicated 127.0.0.1 port: streaming bridge injection (after the head open tag, before a bare </head>, or appended; exactly one per document; 8-byte holdback plus a state machine for tags split across chunks), header hygiene (upstream Host rewrite, X-Forwarded-*, identity Accept-Encoding on document intent only, hop-by-hop strip), CSP drop-and-replace with frame-ancestors listing the editor origins, X-Frame-Options removal, target-origin Location rewrite, byte-identical passthrough for assets and encoded HTML (no injection, once-per-session diagnostic), SSE streaming, and WebSocket passthrough with a bounded pending queue for HMR. Host header validation runs before any upstream contact; the bind is the literal loopback constant and the advertised-URL override is never applied. Tests boot a fake dev server and cover injection, hygiene, fidelity, WS echo, and the security posture. * feat(annotate): annotate-app server mode + CLI live probe with remote hard-off startAnnotateServer gains mode annotate-app and a liveApp option: it throws under PLANNOTATOR_REMOTE, generates the per-session token, composes the proxy-served bridge body (JSON config prelude with both editor origin forms, localhost first, plus bootstrap and bridge supplied by the caller so packages/server never imports @plannotator/ui), starts the loopback proxy after the annotate port is known, serves the live /api/plan payload (no rawHtml, no version fields, sharing off), and stops the proxy with the server. Version history and durable submission records stay excluded via the explicit mode gate. The CLI resolution probes loopback http URLs (3s, accept text/html) and defaults them to live mode when the probe returns HTML; --static forces conversion, --app forces live and fails loudly on non-loopback, https, unreachable, or non-HTML targets; both flags are mutually exclusive transport-shape flags never echoed in the tolerant handoff. A live resolution under PLANNOTATOR_REMOTE is a startup failure suggesting --static. OpenCode and Pi parsers are untouched this phase. * test(live-annotate): protocol, server, and probe suites + smoke script + docs htmlLiveProtocol.test.tsx covers the parent trust boundary (origin and token rejection before parseBridgeMessage, token + targetOrigin on every outbound post, validated page-change and ready pageUrl, per-page restore filtering with full-list numbering) and the bridge live gate, executed as the composed config + bootstrap + bridge body inside a dedicated harness iframe so the srcdoc suites keep running the same script uncontaminated in this process. annotate.test.ts gains annotate-app cases (live payload shape, composed bridge served by the proxy, no-history version endpoints, proxy stopped with the server, remote rejection); annotate-live-resolution.test.ts covers the probe matrix. The two post helpers now drop unmatched-targetOrigin posts silently, matching browser semantics where some DOM environments throw. Adds the manual Vite/Next smoke script and the AGENTS.md live app annotation section (phase gate, security posture, limitations). * test(annotate-cli): cover the CLI layer of the live app remote hard-off Spawns the real CLI entry (async, so the in-process fake app can answer the live probe) with PLANNOTATOR_REMOTE=1 against a loopback HTML server and asserts the startup-failure exit with the --static hint. Completes per-layer coverage of the three-layer hard-off (CLI exit, server throw, unconditional loopback proxy bind). * fix(live-annotate): harden the loopback trust boundary end to end - isLoopbackHostname (now canonical in live-proxy.ts, re-exported by the CLI resolution) requires localhost, ::1, or a LITERAL 127/8 IPv4 address: DNS names like 127.0.0.1.evil.example no longer classify as loopback, so neither the default probe nor --app can start a live proxy against an off-box origin. - The live-eligibility probe judges the FINAL response URL: a target that redirects off its loopback origin falls back to the static pipeline (or fails loudly under --app) instead of opening a live session whose iframe immediately leaves the proxy. - WS upgrades with a browser Origin not naming the proxy itself are refused, so a hostile page's cross-site connect is never laundered into the origin-less shape dev servers trust as a non-browser client (Vite CVE-2025-24010 class). - /__plannotator__/bridge.js refuses cross-site/same-site Sec-Fetch-Site fetches: the per-session token is no longer readable via an off-origin script include on modern browsers. - X-Frame-Options is stripped only on HTML responses (where frame-ancestors replaces it); non-HTML responses keep the app's own framing protection. - Redirect Locations are re-anchored by loopback-host + port equivalence instead of a string prefix: alternate loopback spellings are now caught and lookalike ports (5173 vs 51730) pass through untouched. - --app on a non-URL target fails loudly instead of being silently swallowed. * fix(live-annotate): session correctness for SPA restores, origins, and pathful targets - A live find-and-mark that resolves nothing keeps its record, seeded with unresolved placeholder targets from the durable anchor/text params, so the mutation-driven reconcile re-acquires the pin once a lazy route or data-dependent tree renders (SPA navigation no longer permanently drops pins). Srcdoc restores keep the fail-closed drop. - The bridge posts every outbound message once per listed editor origin; the browser delivers only the one matching the parent document, so an editor opened at 127.0.0.1 instead of localhost no longer silently loses ready and every subsequent message. - The advertised appUrl is the proxy under its localhost spelling with the target URL's own path and query: the framed app stays same-site with the editor, shares the dev app's host-only localhost cookies and storage, and a pathful target opens its page instead of the app root. The proxy still binds the 127.0.0.1 literal. * ci(live-annotate): run the live protocol DOM suite; document the hardened posture htmlLiveProtocol.test.tsx is DOM-gated and was absent from the workflow's DOM_TESTS file list, so none of its trust-boundary assertions ran in CI. Add it, and update the live-app section of the project docs: literal-loopback gate, probe redirect rule, WS Origin check, bridge.js delivery gate, localhost appUrl advertisement, live restore resilience, and the remote-mode behavior change (loopback URL annotate under PLANNOTATOR_REMOTE now exits asking for --static instead of silently converting). * fix(live-annotate): absorb the v0.27 mainline into the live session surface Post-rebase seam work after replaying the branch onto main (v0.27.4 era): - Route the bridge's unanchored-transparency report through postToParent so live sessions deliver it token-stamped to the listed editor origins; the raw '*' post main introduced for srcdoc would be dropped by the live parent's message authentication exactly where restores fail most. New live-harness test pins the contract. - Extend the live remote hard-off to --tailscale sessions (flag postdates the branch): CLI startup failure + startAnnotateServer throw keyed on tailnetPublished, matching how the annotate agent terminal treats tailnet publication. Covered in annotate.test.ts and documented in AGENTS.md. - Keep main's compact-touch input controls and effective mode/input values on the HTML surface while preserving the live pinpoint-only clamps. - Regenerate the pinned guide-viewer manifest (CSS hash moved with the new UI classes; JS unchanged). * feat(live-annotate): Interact/Annotate mode toggle for live app and raw HTML sessions A live app session used to be unusable: the pinpoint capture-phase click handler owned every click, so buttons, checkboxes, inputs, and links never fired. One boolean mode now governs the HTML/live viewer surface: - Interact: the bridge is fully passive. Pinpoint capture, hover outline, drag-selection toolbar, [data-annotate] clicks, and committed-highlight click interception are all gated behind annotateModeActive, so clicks, forms, text selection, and SPA navigation reach the page natively. Committed markers and highlights stay VISIBLE, and marker buttons keep their clicks (a marker click still opens its comment). - Annotate: classic behavior, unchanged. Live sessions annotate exclusively via pinpoint while armed. Control: a single bubble icon button in the editor header (icon never changes; armed = accent + visible border, idle = transparent border of the same width, so the box is pixel-identical in both states), plus a subtle inset accent ring floated over the viewer while armed (pointer-transparent, no layout shift). Keyboard: Mod+Shift+A through the shortcut registry (html-annotate scope; the bridge mirrors the chord inside the iframe and forwards it over the authenticated postToParent path). Esc gains a final ladder rung: draft closes first, then the hover outline clears, then Esc exits Annotate back to Interact (bridge posts annotate-exit; a parent-side listener covers Esc with editor focus). The parent owns the mode and pushes it with the same re-post-on-ready pattern as set-input-method, so it survives live page changes, HMR reloads, and bridge re-injection without ever reloading the iframe. Defaults: live app sessions START in Interact; static/raw HTML sessions START in Annotate (today's behavior preserved, and the srcdoc bridge default keeps behavior byte-identical when no set-annotate-mode ever arrives). Session-only state, no persistence. Vim navigation is available only while Annotate is armed. Covered by new bridge-harness and parent-side DOM tests in htmlLiveProtocol.test.tsx and htmlPinpointProtocol.test.tsx: Interact pass-through, armed capture, the Esc ladder order, mode survival across re-injection, marker clicks in Interact, and both defaults. * feat(live-annotate): pinpoint-armed default, always-on drag comments, comment-only HTML surfaces Simplifies the Interact/Annotate design after live review. The new contract replaces the previous one where they conflict: - BOTH surfaces (raw HTML and live app) now START ARMED with pinpoint; the live-session Interact default is gone. Esc keeps the ladder (close draft, clear hover, then exit to Interact) and the header toggle re-arms. The bridge also paints the pinpoint cursor at init instead of waiting for the parent's first round trip. - The header toggle is a PEN icon: the old bubble sat next to the annotations-panel bubble and the two were indistinguishable. Same box geometry (armed = accent + visible border, idle = transparent border of identical width), aria-pressed, Mod+Shift+A, and the armed ring over the viewer are all unchanged. - Text drag-selection commenting is ALWAYS live on HTML/live surfaces, in BOTH states: the selection pass is ungated from annotateModeActive and from the pinpoint input method. In armed pinpoint, click = pin an element and drag = select text, simultaneously; the >4px drag arming decides which one a gesture was, a completed drag's trailing click never re-pins (one-shot dragEndedClick), and a plain click is never swallowed (the pass only acts on a real selection and never preventDefaults). Esc in Interact still closes an open drag draft before yielding to the page. - HTML/live surfaces are COMMENT-ONLY: useHtmlAnnotation clamps redline/quickLabel (both the host mode and a bridge-posted modeOverride, so a hostile page cannot force a DELETION), the selection toolbar drops Delete and quick labels behind a new commentOnly seam on AnnotationToolbar, and the quick-label picker portal is gone from HtmlViewer. Markdown surfaces keep the full toolbar, and persisted DELETION annotations still restore. - The "Show tools"/"Hide tools" header button is removed. It hid the floating toolstrip (now gone from HTML surfaces entirely: with comment-only plus both input paths live there is nothing left to switch), the collapsed sidebar tab flags, and the viewer's floating action cluster (attachments + global comment + version-diff toggle), all of which are now always visible. htmlChrome persistence keeps only the sidebar/panel state; an old cookie's toolsHidden flag is read tolerantly and ignored, so a stale record cannot strand a user with hidden chrome and no way back. - HTML surfaces pin the viewer input method to pinpoint (the drag/ pinpoint switch is meaningless when both are live); the Alt input switch no-ops there. Vim stays armed-only, as built. No server, proxy, or protocol-security changes; the armed flag stays session-only. Tests: the live-bridge harness is reworked around the armed default (forged-DISARM posture, drag-selection passes in armed and Interact, the trailing-click guard), the pinpoint suite covers the comment-only toolbar and the redline/quickLabel clamp at the trust boundary, a new AnnotationToolbar.commentOnly seam test guards both surfaces' toolbars, App.htmlChrome.test.tsx replaces App.htmlHideTools.test.tsx (no tools button, stale-cookie tolerance, pen armed default), and the htmlChrome tests cover the narrowed persisted shape. * feat(live-annotate): collapsible floating controls cluster The simplification removed the Hide tools toggle, which left the floating comment/attachments cluster permanently over the page. Restore a hide affordance on the cluster itself: a collapse chevron shrinks it to a small expand pill in the same corner, so the page is never obstructed without a way back. Collapsed state persists with the rest of the HTML chrome cookie (sidebar/panel), tolerantly read. Hosts that do not wire the toggle (readOnly viewers, review-editor panels) are unchanged. * feat(live-annotate): header Show/Hide tools replaces the collapse pill The collapse pill was a half measure: it left its own artifact over the page and the sidebar tongue tabs stayed. Revert it and restore the real thing as a header control: an eye toggle immediately left of the pen that removes ALL floating chrome over the page from the DOM (sidebar tongue tabs + the comment/attachments cluster), leaving nothing behind. The toggle lives in the header, so a hidden state always has a way back, which also makes honoring a persisted (or pre-existing) toolsHidden cookie safe again.	2026-08-19 10:44:21 -07:00
Michael Ramos	15f8d4fe4c	feat(review): collapse generated files by default in the all-files view (#1346) ... * feat(review): collapse linguist-generated files by default (#1317) Code review now respects linguist-generated (and linguist-generated=true) from .gitattributes, collapsing those diffs by default the way GitHub does. Server (Bun + Pi mirror): a generatedFiles sidecar rides /api/diff and /api/diff/switch, resolved through git's own attribute machinery — one batched 'git check-attr --stdin -z' over the served patch's paths at the review cwd, so stacked and negated rules land exactly as git resolves them. Plain local git sessions only; PR worktrees, workspace multi-repo, jj, GitButler, and P4 omit the sidecar (degrade to no-collapse). Shared logic in packages/shared/generated-files.ts, vendored to Pi. Client: generated files SEED their CodeView item collapsed (the existing Pierre collapse state — same mechanism as commit-diff folding), render the one-line FileHeader bar with a 'generated' tag next to the +/- counts, and expand per file on click. Expansion is session-local App state so it survives remounts and diff switches. Presentation-only: the diff data, annotations, search, and Edit Mode are untouched; the file tree and single-file tabs list generated files normally (tag, no auto-collapse). Guide viewer manifest pin regenerated (AllFilesCodeView/FileHeader are bundled into the guides.show viewer) from a clean frozen-lockfile install. * feat(review): built-in generated defaults, visible collapsed strip, review-round fixes (#1317) Round 2 on PR #1346, per maintainer review. Built-in generated defaults (industry-standard two-layer detection): packages/shared/generated-files.ts (vendored to Pi) now carries DEFAULT_GENERATED_PATTERNS — lockfiles (package-lock.json, yarn.lock, bun.lock, Cargo.lock, go.sum, ...) plus *.min.js / *.min.css / *.map — matched against the path's last segment only. Explicit .gitattributes wins in BOTH directions: linguist-generated (set/true) marks any file, -linguist-generated / =false un-marks even a built-in name, unspecified falls through to the defaults. In plain local git sessions check-attr refines the defaults; the non-git degrade modes (piped patches, PR worktrees, workspace, jj, GitButler, P4) now emit the sidecar from the name-based defaults alone instead of omitting it. Visible collapsed state: a collapsed generated card no longer renders as a bare header — a GeneratedFileNotice strip ('Generated file collapsed', +N/-N, 'Click to view') styled like the other below-header notices sits in the card, and clicking it expands through the SAME reportFileCollapsed funnel as the chevron. Review findings: - F1: search-match and sidebar-comment navigation expanded items without reporting through the funnel, so those expansions died on diff switch. Both now call syncAllCollapsedMirror + reportFileCollapsed; the funnel invariant comment lists the navigation-driven sites. - F2: the check-attr call gets the same 5000ms timeout as review-core's stdin git callers, and Pi's vcs.ts stdin write gets the one-line EPIPE guard (call-flow.ts shape) a timeout kill makes reachable. - F3: removed the dead prevGeneratedRef + collectSetDelta leg — a changed generated set always remounts via fileSetKey, so the delta path was unreachable. Tests: default-list matching (glob + directory-named-bun.lock), both- direction precedence, non-git sidecar from defaults (dual-runtime), the placeholder strip through the funnel, and search expansion surviving a re-seed round-trip. AGENTS.md payload docs updated. Guide viewer manifest pin regenerated from this clean frozen-lockfile worktree.	2026-08-17 23:32:29 -07:00
Michael Ramos	64062af9a1	feat: Portable Guided Reviews — export, share links, agent-authored guides, guides.show (#1324) ... A Guided Review can now leave Plannotator: as a single self-contained HTML file that renders exactly like the in-app guide, as an encrypted-by-default share link on guides.show, or authored by any agent through the new guide CLI. Highlights: packages/guide-viewer extracted from review-editor at the injection seam (read-only host, no third renderer); guides.show Worker with R2-backed share storage, per-IP rate limiting on creation, delete tokens hashed at rest, and 128-bit ids; portable exports pin the viewer by SRI hash with budget and manifest gates in PR CI and at deploy; two-runtime parity across Bun and Pi verified; v0.27.x saved guides load unchanged. Retention is indefinite by explicit decision, to revisit with the lean sharing refactor. Decision record: adr/decisions/007-portable-guided-reviews-20260815.md	2026-08-16 12:17:13 -07:00
Michael Ramos	192b026073	fix(annotate): stop the folder watcher freezing the server (#1314) ... * fix(annotate): stop the folder watcher freezing the server (#1313) The file-browser content watcher built a chokidar scan over the whole workspace synchronously on the request path. Under Bun that scan monopolizes the event loop (a 780-directory nested tree measured 79 seconds), and because teardown was immediate on the last unsubscribe, every EventSource reconnect paid the scan again: the reconnect the freeze itself provoked made the hang self-sustaining. The watcher engine now lives once in packages/shared/file-browser-watch-core and both runtimes keep only their transport: - construction is deferred off the request path, so the SSE ready event and concurrent API requests are served before any scan starts - teardown gets a 30s reconnect grace; a reload reuses the warm watcher - on macOS and Windows the content watcher is the platform's native recursive fs.watch (measured ~0ms for the same tree); chokidar stays the Linux backend and the runtime fallback, with a forced catch-up refresh on the swap so no events are lost - server stop tears every watcher down immediately in both runtimes The responsiveness regression test reproduces the reported freeze on the pre-fix implementation (79s, fails) and passes in under a second on the fix. * docs: folder annotate sessions do write per-file version history The PLANNOTATOR_ANNOTATE_HISTORY row claimed URL, folder, and annotate-last sessions never write to the data dir. The folder /api/doc path deliberately runs the per-file version-history pipeline (lazily, memoized per resolved path, gated on the same flag) to power the per-file version diff, and has since it shipped. The code is the intended behavior; the sentence was stale. URL and annotate-last sessions remain fully stateless, and submit records remain single-file only. * fix(annotate): review follow-ups for the watcher engine Applied from the independent review of #1314: - contentWatchBackend gains a forced 'native' mode and the fallback tests use it, so the native-to-chokidar paths (creation failure and runtime error) genuinely execute on Linux CI; the runtime-error test is no longer macOS-only - a platform-agnostic responsiveness test pins that SSE ready is served before the scan starts on the chokidar backend, via the runtime test hooks; the tight full-scan bound stays macOS-only - watcher construction failures and the native-to-chokidar swap now log one console.error each instead of stranding subscribers silently; the swap also increments the diagnostics start counter honestly - closeEntry guards both watcher close() calls; the Bun annotate stop chain got the same try/finally shape as the plan server; all four stop chains close watchers ahead of throwable disposals so a failing dispose cannot strand a watcher keeping embedded hosts alive - a broadcast that empties the subscriber map by deleting dead subscribers now schedules the teardown grace instead of leaving the entry live until closeAll - bun.lock drift reverted: only the chokidar edge and the workspace version corrections remain (27 unrelated esbuild resolution entries dropped; frozen-lockfile install verified) - stale never-write comments in both annotate servers corrected to match the folder per-file history reality documented in AGENTS.md; the engine header now states plainly that chokidar is a correctness fallback, not a performance one	2026-08-13 16:02:41 -07:00
Michael Ramos	d2d2dba7fa	feat(annotate): configurable extra markdown extensions (#1309) ... * feat(annotate): configurable extra markdown extensions (#1307) Adds a config-only `markdownExtensions` key to ~/.plannotator/config.json, e.g. { "markdownExtensions": [".livemd"] } for Livebook notebooks. A listed extension is accepted everywhere .md is on the annotate path: CLI target resolution, folder discovery and the file browser, /api/doc plus relative and wiki-link navigation between sibling docs, the 2MB size cap, and per-file version history. Listed extensions render as markdown with frontmatter stripped, never as raw HTML, and they only widen the accepted set. Design: - packages/core/annotatable.ts stays browser-safe and zero-dep. Its regexes and predicates now take an optional, defaulted-empty list of extra extensions, plus a normalizer and regex builders. - packages/shared/markdown-extensions.ts is the node-side seam: it reads config.json once per process through the existing loadConfig() and threads the normalized list into those pure functions. resolve-file re-exports the config-aware predicates so both runtimes pick them up; the Bun server, the Pi mirror, the OpenCode plugin and the CLI all go through them. - The annotate /api/plan payload ships the resolved list so the renderer can linkify links to sibling documents (module-level UI registry, empty by default, so nothing changes without config). Validation: entries must be dot-led, lowercase-normalized, and free of path separators, globs and whitespace. Invalid entries are dropped silently, built-ins are deduplicated, and `.env` is denylisted so config can never register it (annotate copies file contents into the data dir). Deliberately unchanged: the Pi plan-write allowlist (ALLOWED_PLAN_EXTENSIONS in tool-scope.ts) and Edit Mode source save (SOURCE_SAVE_FILE_REGEX), which keep their own narrower allowlists. * fix(annotate): deny the dotenv family and sandbox config-aware tests Review follow-ups on #1309: - deny the whole dotenv family (.prod.env, .env.local, ...) in normalizeMarkdownExtensions, not just the exact .env name - resolve config.json path per call instead of at module scope so PLANNOTATOR_DATA_DIR sandboxing works in single-process test runs - stop resolve-file.test.ts reading the real user config: pure predicate imports plus pinned empty extras on every resolve call - add the config.json -> memo -> predicate integration test using resetMarkdownExtensionsCache under a temp data dir * test(call-flow): make the stale-read advert test self-sufficient The read-only GET only probes the node runtime while Call flow is enabled. The stale-read test relied on earlier tests' settings POSTs leaking callFlow=true through the process-frozen config path; with lazy config resolution each sandbox is genuinely isolated, so the test now enables Call flow in its own data dir. Locally the dependency was masked by an fnm-shimmed sem sidecar spawning node coincidentally.	2026-08-13 09:47:18 -07:00
Michael Ramos	d3633c9c52	Mobile foundation and quieter first run (#1295) ... * feat: establish mobile foundation and simplify onboarding * fix(ui): finish mobile foundation cleanup	2026-08-13 08:45:00 -07:00
Michael Ramos	28d6d55927	Default new Code Review users to Tree (#1292) ... * fix(review): default new reviewers to Tree * test(review): prove Tree seed preserves preferences	2026-08-12 20:30:36 -07:00
Michael Ramos	ed6f44bf2e	fix(release): tailscale gate exit codes and lease gating, conditional SIGHUP, informative guide validation error (#1286) ... - annotate --tailscale publish failures now exit through annotateStartupFailureExitCode: exit 2 under a strict gate (--require-approval / --result-file), where exit 1 is reserved for "the reviewer did not approve, decision record published". Non-strict annotate and review keep the documented exit 1. - the annotate client lease (auto-dismiss on abandonment) is forced off while tailnetPublished is set: --tailscale reads as local to the CLI predicate, but clients connect through the serve proxy, and a proxy disconnect longer than the grace would dismiss a live review. Same rationale as remote/shared sessions; decided at the single point both the /api/plan advert and the SSE endpoint read. - the SIGHUP-to-process.exit route moved from an unconditional CLI-entry listener into enableTailscaleServe's success path, installed only once a serve mapping exists. Any SIGHUP listener overrides the ignored disposition nohup depends on, so plain sessions now keep zero listeners and "nohup plannotator review &" survives terminal close again; --tailscale sessions still tear their mapping down on HUP (exit 129). - validateGuideOutput explains a fully-invalidated guide whose refs named files outside the changeset (count plus up to 3 example paths, with a pointer to the Commits panel) instead of the bare generic message; the generic message stays for genuinely structural emptiness. The informative error now flows through onJobComplete to the job failure card; Pi picks the change up via the vendored guide-review copy.	2026-08-12 14:04:59 -07:00
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.	2026-08-12 12:07:30 -07:00
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	2026-08-12 11:54:27 -07:00
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.	2026-08-11 23:54:22 -07:00
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.	2026-08-11 22:55:18 -07:00
Michael Ramos	caf7ce1ccd	feat(review): install Call Flow automatically in the background on opt-in (#1271)	2026-08-11 17:48:18 -07:00
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	2026-08-11 16:28:08 -07:00
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	2026-08-11 13:18:35 -07:00
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.	2026-08-11 10:24:39 -07:00
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	2026-08-10 15:27:04 -07:00
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)	2026-08-09 17:19:50 -07:00
Michael Ramos	710ce1a065	docs: correct archive sharing posture (#1176)	2026-08-07 12:29:51 -07:00
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	b69742c3bf	feat: add PLANNOTATOR_URL_HOST display-only override for advertised URLs (#1225) ... * feat: add PLANNOTATOR_URL_HOST display-only override for advertised URLs Remote mode binds 0.0.0.0 but every advertised URL hardcoded http://localhost:<port>, so a session opened from another device (e.g. a phone on the same tailnet) got an unopenable link (#657). - resolveUrlHost() in packages/shared/config.ts: PLANNOTATOR_URL_HOST env var over config.json urlHost, validated host-only (bare hostname, IPv4, bracketed IPv6); invalid values warn once and fall back to localhost. - buildAdvertisedUrl(port) in packages/server/remote.ts and its Pi mirror in apps/pi-extension/server/network.ts; all 7 construction sites use it. - Strictly display-only: binding stays governed by PLANNOTATOR_REMOTE, and agent-review jobs get a pinned http://127.0.0.1:<port> API URL. - Remote-ready copy says "open on your device" when the host is overridden; local sessions with an override warn it is unreachable. - Tests for validation, precedence, and URL composition in both runtimes; docs in CLAUDE.md and the marketing site. * fix(review): ignore urlHost in local sessions, harden warning output Review follow-ups on #1225: - Local (loopback-bound) sessions no longer honor the advertised-host override: honoring it auto-opened http://<host>:<port> against a server nothing was listening on, openBrowser still reported success, and the agent blocked on waitForDecision. Local sessions now advertise and open localhost, warning once that PLANNOTATOR_REMOTE=1 is required. - The invalid-host warning JSON-encodes the echoed value so an embedded newline cannot forge extra stderr lines (hosts surface session-ready lines as clickable links); warn-once is now per value. - Docs: local-session behavior reworded, the empty-env-suppresses-config semantic documented, secure-context note generalized.	2026-08-06 18:29:11 -07:00
Michael Ramos	c08b188812	perf(ui): single Shiki highlighter, palette-matched code blocks, drop highlight.js (#1218) ... * perf(build): stub out the dead Oniguruma WASM in every bundle @pierre/diffs picks its Shiki engine with a runtime ternary: engine: preferredHighlighter === "shiki-wasm" ? createOnigurumaEngine(import("shiki/wasm")) : createJavaScriptRegexEngine() Plannotator pins `preferredHighlighter: 'shiki-js'` (and Pierre's own default is 'shiki-js'), so the Oniguruma branch never executes. Because the choice is a runtime ternary, bundlers keep the `import("shiki/wasm")` edge anyway and inline `@shikijs/engine-oniguruma/wasm-inlined`, a ~622 KB base64 blob, into the single-file HTML builds. The review app paid for it twice: once on the main thread (via `highlighter/shared_highlighter.js`) and once inside the `?worker&inline` Pierre worker. Alias `shiki/wasm` to a stub that throws if it is ever reached. Wired via `resolve.alias` rather than a plugin because `resolve.alias` is shared with Vite's worker build and `plugins` are not. Highlighting output is unchanged: the JS regex engine was already the one doing the work. Opting back into 'shiki-wasm' now fails loudly instead of silently costing every user a megabyte of dead bytes. apps/review/dist/index.html 19,424,646 -> 18,180,545 (-1,244,101 raw / -463,348 gzip) apps/hook/dist/index.html 23,032,467 -> 22,410,416 (-622,051 raw / -233,485 gzip) * perf(ui): consolidate code highlighting onto Shiki, drop highlight.js The app shipped two highlighters. Shiki already tokenised the code-review diff pane (via @pierre/diffs, JavaScript regex engine); highlight.js separately coloured markdown fences and review suggestion snippets at ~982 KB minified for a full build of ~190 grammars. That second highlighter is now gone. Every call site moves onto `packages/ui/utils/codeHighlight.ts`, a thin wrapper over Pierre's SHARED Shiki instance: CodeBlock, Viewer, PlanCleanDiffView markdown fences InlineMarkdown code-file hover preview HighlightedCode review suggestion snippets Reusing Pierre's instance rather than standing up a second fine-grained one is deliberate. Pierre imports Shiki's full bundle, so every grammar and theme is ALREADY inlined in the single-file builds: a separate highlighter with a curated language list would have duplicated a subset of bytes that are already there. Sharing costs nothing, gives every language Shiki bundles instead of a shortlist, and — the point of the change — guarantees fences resolve the exact same theme the diff pane resolves. Theming. `SHIKI_THEME_MAP` / `resolveSyntaxTheme` move from `packages/review-editor/hooks/usePierreTheme.ts` to `packages/ui/utils/syntaxTheme.ts`; usePierreTheme re-exports them, so the review editor's imports are unchanged. `useFenceTheme()` feeds the components and re-highlights on palette or mode change. Code blocks now follow the active palette across all ~52 themes in both light and dark, instead of always rendering github-dark and relying on hand-written `.hljs-*` override stacks to stay legible. Those stacks are deleted: `packages/editor/index.css`'s light-mode token palette, and `colorblind.css`'s hand-tuned tokens which existed to APPROXIMATE @pierre/theme's protanopia-deuteranopia themes that are now simply used. Behaviour held fixed: - Language-less fences stay plain text (#1212). No auto-detection anywhere, including the hover preview, which previously called `hljs.highlightAuto`. `HighlightedCode` derives its language from the caller's file path; an unknown extension renders plain. - `applyHighlight(el, ...)` keeps the imperative `hljs.highlightElement` DOM contract the annotation layer reaches into, and writes plain text at final size first so async highlighting causes no layout shift. Already-attached grammars highlight synchronously — no flicker on cached highlights. - It also verifies the rendered text is byte-identical to the source and falls back to plain otherwise, because annotations address code blocks by text offset. - `@plannotator/ui`'s public API is unchanged: the highlighter is a module-level default like the package's other seams, no new props. The `hljs` class on fenced `<code>` becomes `pn-code` (it is a structural hook for blockTargeting, vim navigation and print.css, and it named a library we no longer ship). `language-*` stays. apps/review/dist/index.html 18,180,545 -> 17,270,889 (-909,656 raw / -291,921 gzip) apps/hook/dist/index.html 22,410,416 -> 21,704,434 (-705,982 raw / -238,096 gzip) Verified the diff pane is untouched: the rendered Pierre shadow-DOM markup is byte-for-byte identical between an origin/main build and this one (SHA-256 aa1ee88a…). * fix(ui): strip stray NUL bytes from the code-highlight source Two U+0000 bytes slipped into comments in the previous commit, which made git treat the file as binary. Replaced with spaces; no behaviour change. * fix(ui): keep code-block annotation marks across highlight swaps Fenced code is annotated by hand: one `<mark data-bind-id>` inside the `<code>` element, which `applyHighlight` also owns. Every highlight swap (palette change, dark/light toggle, or the first async grammar attach after load) replaces that element's children, so the mark was silently wiped and nothing put it back. Annotation state, the sidebar panel and exports were unaffected; the loss was purely visual, and deterministic. `applyHighlight` now publishes every write through `onCodeHighlightSwap`, synchronously, immediately after it. `Viewer` subscribes and re-paints the fence's mark, so a swapped block ends up with BOTH the new theme's tokens and its annotation. The shared painter (`paintCodeBlockMark`) moves the token spans into the mark instead of flattening them to text, so creating an annotation no longer costs a block its colours either. Being driven by the swap also fixes the cousin race by ordering rather than timing: share/draft restore runs on a timer after load, and on a slow machine the first async swap could land after it and wipe the restored marks per block. A restore that painted before the swap is now re-established in the same task the swap ran in, and one that runs after finds the mark already there. Removal tombstones the id before re-highlighting, because the host drops the annotation from state a tick later — without it the swap listener would paint the just-removed annotation back in, and a fence carrying a second annotation would end up bare. Also closes the named gap in the WASM coverage: entry-assets only grepped source, so a future @pierre/diffs bump could reintroduce the inlined blob through a different import specifier unnoticed. It now greps the built `apps/{review,hook}/dist/index.html` for the base64 WASM magic, skipping on an unbuilt checkout and running for real in the CI job that builds the bundles.	2026-08-05 21:54:40 -07:00
Michael Ramos	10a5104888	fix(install): repair the skills checkout guard, add --skip-skills (#1201) ... * fix(install): make a failed skills checkout stop reporting success The skills/commands checkout runs in a subshell written as `( set -e; ... ) || checkout_failed=1`. POSIX ignores `set -e` for every command of an AND-OR list except the last, and bash 3.2.57 (what `curl | bash` gets on macOS), bash 5.3, dash, zsh and ksh all carry that suppression into the subshell. The `set -e` was inert. A failed clone therefore ran the whole block anyway, the subshell exited with the status of its trailing `if` (0), `checkout_failed` stayed 0, and the installer printed "YOU'RE ALL SET!" with no skills installed. It also blamed the wrong thing, printing "Tag vX.Y.Z predates the per-agent skill layout" when the real cause was a failed clone. Drop the inert `set -e` and guard the four fetch steps with explicit `|| exit 1`. Everything after the checkout stays best-effort, matching install.cmd, which only checks git clone and lets every xcopy run unchecked. A local cp/mkdir/rm failure must not surface as the "network or git error" message. Verified on bash 3.2.57 in a sandboxed HOME: the failure case now exits 1 with the fetch error and no success banner, and original vs patched success runs produce byte-identical trees (74 paths, 35 files) and identical logs. * feat(install): add --skip-skills opt-out The skills and slash commands come from a sparse `git clone` of the release tag. There was no way to decline that fetch short of --minimal, which also drops the sem sidecar, the agent-terminal runtime, the hooks, and every per-agent config. Anything that installs a tag github.com cannot serve had no option at all. Add --skip-skills to all three installers, following the existing --skip-codex / --skip-gemini / --skip-kiro / --skip-opencode family: CLI flag (-SkipSkills in PowerShell), PLANNOTATOR_SKIP_SKILLS_INSTALL env var, skipInstall.skills config key, resolved flag > env > config. It is not a per-agent switch; it covers every scope the checkout writes (Claude, ~/.agents, OpenCode, Gemini, Kiro), the extras, and the skill-scope cleanup sweeps. Skip means do-not-write: nothing already installed is replaced or removed, and git stops being a hard requirement. The run reports "Skills: skipped (<source>)" and the closing banner no longer claims the /plannotator-* commands are ready, which is the same false-success the checkout guard exists to prevent. Use it in the install-script-smoke job. That job installs a synthetic v9.9.9: the fake curl serves the freshly built binary for any URL, but the skills clone goes to real github.com, where the tag does not and cannot exist. That clone has always failed; it only went unnoticed while the broken guard let the installer exit 0 anyway. The job asserts Codex hook config, not skills, so it opts out rather than ignoring a real error. Both run_installer call sites go through the one function definition. Verified in an env -i sandbox on bash 3.2.57 (what `curl | bash` gets on macOS) with a fake curl and a local stand-in remote. Flag, env var, and config each skip and name their own source; flag beats env=0; env=0 beats config true; an explicit "skills": false stays a veto. Without the flag, pre-change and post-change runs produce byte-identical trees (39 entries) and identical logs. A bad clone URL without the flag still exits 1 with the fetch error and no success banner. A --skip-skills re-run over an existing install leaves all 12 skill and command files byte-identical. The CI step was reproduced locally: both run_installer calls exit 0 and every Codex assertion still passes. * test(install): cover --skip-skills and repoint the pinned source strings scripts/install.test.ts asserts against exact install-script source text, so six assertions broke when --skip-skills landed. Each is repointed at the new string with its intent preserved, not weakened: - The "hook/config writing happens before the git hard-fail" ordering test keeps proving the ordering; it just matches the gate's new conditional form. git being a hard requirement is now a narrower invariant (it applies only when the checkout actually runs), so that is asserted separately rather than dropped. - The skipInstall walk assertions follow codex/gemini/kiro/opencode gaining a skills entry, in install.sh's `for _agent` loop and install.cmd's PowerShell key list. - The three "never remove" sweep assertions follow the Codex stale-skill cleanup gaining its skills-opt-out arm, in all three installers. Add nine tests covering --skip-skills itself in the same style as the per-agent family: flag/switch parsing, PLANNOTATOR_SKIP_SKILLS_INSTALL, skipInstall.skills, and flag > env > config precedence by textual layering, for each installer. Each installer also gets a test that the opt-out bails before the clone and leaves the checkout guard intact (#1201's fix must keep failing a real fetch error), and one that the run reports honestly, never prints the "commands are ready" banner over an empty skills dir, and suspends the extras, the model-invocation rewrite, and the stale-stub sweeps rather than applying them partially. bun test scripts/: 116 pass, 6 skip, 0 fail. Full bun test: 2884 pass, 234 skip, 0 fail. bun run typecheck clean.	2026-08-04 21:49:49 -07:00
Sergiy Dybskiy	050dfcda9a	feat(opencode): add OpenCode 2 plan review adapter (#1194) ... * feat(opencode): add OpenCode 2 plan review adapter * fix(opencode): harden V2 review lifecycle * fix(opencode): address V2 review feedback * fix(opencode): update V2 target and isolate tests * test(opencode): assert prompt composition invariants	2026-08-04 17:56:33 -07:00
Michael Ramos	7682628db7	feat(install): Codex opt-out and credential-free attestation verification (#1197) ... * feat(install): Codex opt-out and credential-free attestation verification Implements both asks from #1178 (reported and designed by @astradevkin): - Per-agent installer opt-outs: --skip-codex / --skip-gemini / --skip-kiro flags, PLANNOTATOR_SKIP_{CODEX,GEMINI,KIRO}_INSTALL env vars, and config.json skipInstall.{codex,gemini,kiro} keys, with flag > env > config precedence mirroring verifyAttestation. Detected-but-skipped is reported as its own honest state, never conflated with not-detected, and a skipped agent's home is neither written nor cleaned up. - Credential-free provenance verification: when --verify-attestation is active, the Sigstore bundle is fetched from GitHub's public attestations endpoint (single unauthenticated attempt, no retry) and verified via gh attestation verify --bundle with the same --repo/--source-ref/--signer-workflow constraints; gh's authenticated fetch remains the fallback. TUF trust-root failures are reported as connectivity, distinct from real provenance failures; every path stays fail-closed. Zero behavior change for users who do not opt in: the default install path is unchanged (verified by sandbox-HOME parity runs against main). * fix(install): address #1197 review round (H1 retry, M2-M7, lows) - H1: a failed gh --bundle invocation now retries once through the exact authenticated path before any classification, so an older gh (unknown flag) or a corrupt bundle never reports as a provenance failure. Pinned by a functional stub-gh test; a real failure still fails again on the retry and aborts. - M2: the sh config layer extracts the skipInstall object (awk, character indexed) before matching per-agent keys and honors explicit false as a veto; cmd now parses the real JSON via PowerShell like ps1. Functional tests cover the foreign-key collision and explicit-true cases. - M3: sh names the real cause when the bundle path cannot run (no JSON extractor vs fetch vs extraction failure) and gains python3 and jq fallback extractors; docs state the dependency. - M4: ps1/cmd gate the existing-integration note on plannotator content in hooks.json and word the skip state as what those platforms actually do (manual instructions suppressed). - M5: sh bundle lives inside a private mktemp -d, one rm -rf on every exit, and a mktemp failure degrades to the fallback instead of aborting. - M6: README, verifying-your-install, environment-variables, and installation docs updated for the credential-free path and skip flags. - M7: ps1/cmd extraction replaced with a byte-exact string scanner (no ConvertFrom/ConvertTo round trip, immune to DateTime coercion), with PowerShell-driven unit tests over the captured real attestations response plus synthetic DateTime and brace-in-string controls. - Lows: fail-closed abort pinned by a functional test; TUF beats auth in cmd classification (matches sh/ps1); skipped-state output mentions the shared ~/.agents/skills; Gemini summary is skip-aware and gains an honest not-detected state; --skip-opencode do-not-write switch added (flag, env var, config key) across all three scripts. * fix(install): review round three (EncodedCommand fetcher, mutation-proof fail-closed test, lows) - R1: install.cmd's attestation fetcher no longer touches disk. The %RANDOM%-named %TEMP% .ps1 (predictable-path code execution, the M5 class escalated) is replaced by powershell -NoProfile -EncodedCommand with a base64(UTF-16LE) payload defined next to its full REM PS: plaintext; a test decodes the blob and asserts byte equality with the documented lines plus the security-relevant shape (env-var inputs, ordinal scan, no JSON round trip, distinct exit codes). Inputs still travel via env vars. Verified end to end under pwsh: the decoded blob fetched the real attestations response, wrote 2 bundles, gh verified the real v0.25.1 binary credential-free (exit 0) and rejected a wrong binary (exit 1). - R2: the fail-closed test now asserts the output ENDS with 'Refusing to install.' - mutation-verified: with the verify-failure exit 1 deleted the mutant still exits 1 via an incidental mv failure, but the trailing mv error breaks the endsWith and the test fails; restored, it passes. - Lows: CI guard test fails loudly when process.env.CI is set and no pwsh/powershell is on PATH (scanner coverage cannot silently vanish); scanner IndexOf calls are ordinal in ps1 and the encoded cmd variant; the awk skipInstall extraction requires optional-whitespace-then-colon- then-brace after the key (string values can no longer anchor it, with non-token occurrences skipped, unit-checked against escaped-embedded payloads); install.cmd comments warn that the fallback-reason literals inside parenthesized blocks must stay parenthesis-free.	2026-08-04 13:17:27 -07:00
Michael Ramos	46f1e8d5b2	fix(annotate): recognize wrapped URLs in token probe and port #1185 coverage (#1187) ... Ports five small items from the closed parallel PR #1185 into the tolerant annotate argument resolution that landed in #1183 (#1182): - Bug fix: the token probe tested the raw token against the URL regex, but the pipeline strips the @ reference marker and wrapping quotes first, so a multi-token 'annotate @https://example.com/page and summarize it' probed to nothing and emitted the handoff instead of opening the URL. The probe now unwraps with stripAtPrefix before the regex and returns the unwrapped form (the pipeline re-strips harmlessly). Tests cover @-prefixed and quote-wrapped URLs as multi-token candidates. - Test ports: absolute-path candidate, the wider plain-text set (.txt, .yaml) guarding ANNOTATABLE_DOC_REGEX breadth, the scoped-package literal-@ fallback against a real @scope/ directory, and the whole-un-split-string preference over its own tokens ('Meeting Notes.md' wins over a resolving 'Notes.md' token) covering annotateInputNamesExistingTarget. - Defensive scan: the strict-mode source-scan test now asserts the annotate startup block gates tolerance on !strictAnnotate via isStrictAnnotateInvocation, since an inverted gate cannot be spawn-tested without starting a server. - DRY: the strict predicate was defined twice (strict-annotate-result exit-code helper and the index.ts tolerance bypass). Extracted isStrictAnnotateInvocation with a StrictAnnotateFlags type; both sites use it so the exit-code path and the tolerance bypass can never drift. Behavior byte-identical; existing subprocess tests unchanged. - Docs: the tolerant-resolution section now cites #872 (commit aac5aacb) for why the bang prefix is deliberate and states that argument-shape issues belong in the CLI's resolution, not the skill templates. Refs #1185, #1182 Co-authored-by: Josh Nichols <josh.nichols+agent@gusto.com>	2026-08-03 13:25:41 -07:00
Michael Ramos	747b5ea7e6	fix(annotate): resolve natural-language arguments or hand off to the agent (#1183) ... * fix(annotate): resolve natural-language arguments or hand off to the agent Claude Code skills run the CLI through a bash-substitution prefix that executes before the model sees anything, so any trailing natural language in /plannotator-annotate died with 'File not found: the'. Worse, a non-zero exit from that prefix aborts the whole prompt before the model runs (verified empirically), so the error was never even visible to the agent. Three-tier resolution in the binary's annotate argument handling, shared by every host via packages/shared/annotate-target.ts: 1. Fast path: probe each whitespace-delimited token; exactly one naming an existing file, URL, or folder proceeds with it directly. 2. Ambiguity: two or more tokens resolve; error naming every candidate, never guess. 3. Handoff: nothing resolves; emit an agent-addressed message echoing the words tried and asking the reading agent to interpret the request and re-run with a concrete target, preserving flags. In plain mode it lands on stdout with exit 0, the only combination that reaches the model through the bang prefix; in --json/--hook mode it goes to stderr with exit 1 so machine stdout stays clean. Single-token invocations run the unchanged pipeline first, so bare correct invocations are byte-identical. Strict gates (--require-approval or --result-file) bypass the tolerance entirely: a typo'd path stays a startup failure with exit 2 and no agent-facing prose. The CLI resolution pipeline moves to apps/hook/server/annotate-resolution.ts (returns typed outcomes instead of exiting) so the token fallback can run it once with a selected candidate; OpenCode and Pi wire the same shared selection into their own not-found paths. Skill bodies gain one line telling the agent to re-run with a concrete target when the command reports unresolvable arguments. Closes #1182 Reported-by: @technicalpickles * fix(annotate): harden tolerant resolution per review Review fixes for the three-tier annotate argument handling: - A single unresolvable token now falls through to the legacy pipeline verbatim: 'annotate nope.md' is exit 1 with 'File not found: nope.md' again in every non-strict mode, instead of an exit-0 handoff that fail-opened scripts gating on the exit code. The handoff fires only when two or more words resolve to nothing. - Unrecognized dash-prefixed tokens disable tolerance instead of being skipped, so a typo'd flag ('--no-jna') errors the way it did on base rather than silently fetching via Jina. Known flags are stripped before selection as before. - Token selection now receives the original argv tokens, so a quoted missing path ('my notes.md') is probed as one token and can never be re-split into a silently resolving 'notes.md'. - Bare directory names only count as fast-path candidates when they are the sole argument; a stray word matching a directory (or '.') hands off instead of opening folder mode. Explicit paths like 'src/' keep resolving, and the bare-existence probe fallback is file-only. - The handoff re-run suggestion echoes content flags only (--markdown, --no-jina, --render-html), never transport flags (--gate, --json, --hook). - New subprocess suite (annotate-cli.test.ts) spawns the real CLI entry and pins the contract: single-token typo exit 1, strict invocations (--require-approval and --result-file) exit 2 with empty stdout and no handoff prose, unknown-flag error, quoted-token preservation, and the directory-hijack case. Placeholder dist files are created when a build is absent so the suite runs in CI. - The copilot and gemini annotate command bodies gain the same handoff instruction as the Claude, core, and kiro skills. - AGENTS.md documents the three tiers under Annotate Flow and corrects the strict-section sentences that claimed non-strict behavior was fully unchanged; the marketing annotate doc mentions the tolerant arguments. Refs #1182	2026-08-03 10:14:37 -07:00
Michael Ramos	4c89418bad	fix(pi): contain the pi-todos mirror inside the project root ... The mirror resolved <cwd>/.pi/todos with a plain path.join, so a repo that ships .pi/todos as a symlink pointing elsewhere had its plan checklist written to the symlink target on the first plan approval. The mirror is on by default, so checking out a hostile repo and approving a plan was enough. Write-only and non-overwriting, but not something to ship knowingly. The repo-implied path must now realpath inside cwd or the provider reads as absent, which is the documented no-mirror no-op. An explicit PI_TODO_PATH stays trusted verbatim: that is the user's own choice at the same trust level as config.json, and pi-todos honors it the same way. Containment reuses the shared isWithinDirectory sink, resolved through the deepest existing ancestor so a not-yet-created todos dir under a symlinked .pi cannot be created outside either. Also corrects the detection docstring: pi-todos calls ensureTodosDir on session_start, so an installed-but-unused provider does not read as absent. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS	2026-07-30 03:03:13 -07:00