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
Client half of spec §6.4, extending PR3's tripwire exactly as its comment
instructed:
- REVIEW_APPROVAL_NOTES_SUPPORTED is deleted. The spec input is now the
server advert, read off /api/diff (and re-read from any diff payload that
carries it) through readApprovalNotesAdvert — absent reads false, so an old
server renders no approve-carrying items and a new server against an old
client changes nothing.
- The approve-with-notes route stops being a marked refusal: it forks on
withAnnotations ('Approve with notes' ships the live annotations plus their
export; 'Approve with a note…' ships the composer note alone) and lands on
handleApprove via the new pure buildReviewApprovalBody.
- handleApprove drops the 'LGTM - no changes requested.' placeholder: a bare
approval sends feedback '' — consumers now print approve-time feedback, and
the empty body is what makes the archive's lgtm decision reachable and
stops bare approvals writing sidecars (spec §6.2 fact 1).
- reviewDecision.test.ts: the PR3 tripwire becomes the delivery assertion
(under a true advert every approve-carrying item's payload carries the
content, never the placeholder) plus the absent-advert-is-false pin.
- App.decisionControl.test.tsx: bare-approval assertions move to the empty
body, the advert-off test pins the old-server payload shape, and a new
advert-on case proves 'Approve with notes' posts the live annotations.
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
The four waitForDecision consumers that threw result.feedback away on the
approved branch (spec §6.3) now emit composeReviewApprovedMessage — the
approved prompt, then the note when one rides the decision:
1. Claude Code CLI `plannotator review` (apps/hook/server/index.ts); the
amp/droid plugins relay its stdout and inherit the delivery.
2. OpenCode native (apps/opencode-plugin/commands.ts) — also fixes the
delivery gate: it rode on the LGTM placeholder making feedback truthy, so
with the placeholder gone a bare approval would have been silently dropped;
the gate is now `feedback || approved`.
3. OpenCode CLI bridge (buildReviewPromptFromBridgeOutcome) — the CLI's JSON
record always carried the feedback; the bridge stops discarding it.
4. Pi (apps/pi-extension/index.ts) via the vendored prompts module.
Each consumer's startReviewServer call now passes the matching advert:
supportsReviewApprovalNotes(origin) for the hook CLI (new seam in
apps/hook/server/review-output.ts — every origin shares the one stdout
relay today), Boolean(sessionId) for OpenCode native (no session, no
delivery — the annotate precedent), unconditional true for Pi and the
standalone dev server (which already emitted feedback on approve).
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
Both runtimes gain an `approvalNotesSupported` option on the review server
(default false) echoed on every diff payload — /api/diff, /api/diff/switch,
/api/pr-diff-scope, /api/pr-switch — so the advert survives a diff switch
(spec §6.4). An old caller that never passes the option advertises false and
the client keeps rendering no approve-carrying items.
packages/shared/prompts.ts gains composeReviewApprovedMessage (vendored to Pi
with the rest of prompts.ts): the one composer every review decision consumer
emits approvals through — the approved prompt, then the approve-time feedback
when the decision carries any. It filters the legacy
'LGTM - no changes requested.' placeholder so a new consumer reading an old
built client's approval never appends filler the reviewer did not write.
Dual-runtime tests: advert default false / true-and-survives-switch on both
servers; approve-time feedback reaches waitForDecision byte-identical and
archives as approved-with-notes; a bare approval (post-placeholder shape)
archives as lgtm with no sidecar — the decision the placeholder made
unreachable (spec §6.2 fact 1).
Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9
- 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
The portable viewer's Tailwind scan covers packages/review-editor, so the
sidebar composer's new utility classes changed the built viewer CSS hash;
check:manifest fails on a stale pin.
Spec §7 PR4 hunt list + §8 test 17:
- ReviewSidebar.generalComment.test.tsx (DOM, registered in test.yml's DOM
lane so it cannot silently skip): reachable at totalCount === 0; the
General section header offers the button with only line comments; a commit
delivers the trimmed text exactly once (button and Mod+Enter); an empty
note never commits; Escape dismisses but keeps the draft.
- App.decisionControl.test.tsx: end-to-end flip proof — a sidebar general
comment flips the header primary from Approve to Send Feedback · 1 and
rides the posted /api/feedback body with the sentinel shape and no prUrl.
Guards the wiring a component test cannot see (prop dropped from the
sidebar mount, handler stamping PR context).
- reviewDecision.test.ts (pure lane): createGeneralReviewComment shape —
trimmed scope:'general' with sentinels, randomUUID ids that cannot collide
in one millisecond (spec §9), whitespace-only returns null, and the
PR-switch survival pin: no prUrl/diffScope, passes annotationMatchesPrScope
for any PR + diff scope.
Spec §3.3: scope:'general' annotations already render, badge, and export;
the sole producer was Call Flow. This adds the human one — the community's
"integrated global comment field" ask.
- ReviewSidebar gains optional onAddGeneralComment. The SAME button renders
in the General section header (which now renders whenever the callback is
present, even with zero general comments) AND in the all-empty state — the
state the affordance is most useful in. The composer is the shared
DecisionNoteField in a small anchored popover (the third consumer the
branches' extraction tripwire named); the draft survives a dismissal, an
empty commit refocuses the field (the decision-composer contract).
- Both human producers now share one shape factory,
createGeneralReviewComment in reviewDecision.ts: scope:'general',
sentinel filePath ''/0/0, review-note-${randomUUID()} id, and NO PR
context — an unstamped annotation passes every PR scope predicate
(utils/annotationScope.ts), so the comment survives an in-place PR switch.
- Unlike the header composer's one-submit note, the sidebar comment goes
through addCodeAnnotationsWithHistory: undoable, draft-persisted, and
deletable via the sidebar's existing delete. Creating one raises
totalAnnotationCount, which flips the header control to Send Feedback · n
— the control is state-driven by construction.
No server change (spec §6.1): the comment rides the existing /api/feedback
annotations array and the export's ## General section on both runtimes.
Maintainer ruling on spec open question 2 (rides PR4): normalizeAnnotation's
copied-field allowlist gains scope?: string, so a review-level general
comment archives distinguishably from a line comment in index.jsonl.
Additive only — the JSONL line shape is a cross-tool contract (fields are
added, never repurposed) and readers must keep ignoring unknown fields; an
absent scope stays absent (the pre-scope line default), never defaulted-in.
apps/pi-extension/generated/ is gitignored and rebuilt by vendor.sh, so the
shared edit reaches the Pi runtime at build time; the dual-runtime
review-note-payload archive test now asserts the archived general-note
record carries scope on BOTH runtimes, and the pure shared-lane test pins
the normalizer directly (cannot silently skip).
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.
- reviewDecision.test.ts (pure lane): every id the spec can emit resolves
to a route, routes fork only on approved, compact row ids stay unique -
the runtime stand-in for the missing app typecheck (spec 8C).
- review-note-payload.test.ts, ported as-is from the held branch: the
scope:'general' note survives waitForDecision unmodified on BOTH runtimes
and archives as a feedback decision counting the note.
- exportFeedback.reviewNote.test.ts, ported as-is: the note renders under
'## General', co-exists with placed annotations, and is real feedback.
- App.decisionControl.test.tsx (DOM lane): mounts the real review App
(workerPool's Vite-only ?worker&inline import stubbed per the
AllFilesCodeView.lifecycle idiom) and asserts through the posted body:
empty-state Approve posts the legacy LGTM shape, Send Feedback posts the
live annotations, Request changes... delivers the sentinel general note in
the array and the export, discard posts empty arrays only after its
confirm, Mod+Enter equals the visible primary, approve-carrying items are
absent while the advert is off, and compact touch offers a positive row at
zero that posts (E16-review). Registered in the CI DOM lane so it cannot
silently skip.
- shortcuts.test.ts now asserts the decision-control scope is registered in
BOTH adopting registries (the PR2-era compose-cleanly probe became real).
Agent-mode review replaces AgentReviewActions (Close/SendFeedback flip +
dimmed Approve + hover tooltip) with the ghost-X Close + DecisionControl
driven by buildDecisionSpec: Approve at zero, Send Feedback . n otherwise,
with Request changes... / Send with a note... and the explicit
'Approve, discard n annotations...' confirm behind the caret. One
submitPrimaryDecision() serves the header primary, the global Mod+Enter
handler, and the compact primary row; compact rows are generated from the
same spec so a visible positive decision exists in every state, with
DecisionNoteDialog / ConfirmDialog resolving from the LIVE spec by item id.
Transport routing is pure in reviewDecision.ts and single-endpoint: every
decision POSTs /api/feedback with approved as the only fork. A note commits
a scope:'general' CodeAnnotation (filePath ''/0/0 sentinels, crypto-random
id, no PR-context stamp, not history-recorded) and submits one render later;
a failed POST keeps the armed note decision for a primary retry (cleared
only on success). Approve-carrying items stay capability-gated on
reviewApprovalNotesSupported, hardcoded false until PR5 ships the server
advert + consumer delivery (spec 6.4) - no server field is invented here.
Deletions per spec 5.1/5.2: AgentReviewActions.tsx outright, the
'Annotations Won't Be Sent' approve confirm + its suppression references,
and handleSendFeedback's send-at-zero guard (unreachable once no send is
offered at zero, and it would swallow a request-changes submission). The
exit warning and the copy-path 'No Annotations' dialog survive. The
decision-control shortcut scope joins the review settings registry and the
submit entry now describes the adaptive primary.
M1 (ruling: honest label, keep transport): additive feedbackDelivered on
DecisionSpecInput; the agent-terminal delivered empty state keeps the
Done primary and its full-payload /api/feedback transport but stops
claiming 'no feedback' in the tooltip and note-item subtitle (free
prose, not frozen). App passes isCurrentFeedbackDeliveredToAgent;
decisionSpec fact-guard pins the delivered-state copy.
L1: dead gate prop removed from AppHeader (decl, destructure, App call
site, both test prop objects).
L2: confirm/composer surfaces resolve from the LIVE spec — DecisionControl
stores only the confirm item id and steps back to the menu when the item
leaves the spec; the App's compact composer/confirm dialogs do the same.
F6 DOM test extended with a confirm-open spec-flip case.
L3: a failed submit keeps the captured note decision armed —
handleAnnotateFeedback/Approve report success, pendingDecisionSubmit is
cleared only on success (one auto-dispatch per arming, in-flight ref
guards double submit), and the next primary invocation replays the
captured route/framing. Pinned: note-commit -> POST 500 -> primary retry
posts approvalFraming intact on the captured route. Trivial guard: a
pending whose note id left annotations state disarms.
L4 (ruling: delete): the F6 empty-items close branch and its test are
gone — buildDecisionSpec guarantees at least one item.
L5: discard confirm copy neutralized (counts can include other tools'
findings; the non-gate message notes direct edits still ride).
Nit: dropped the review-registry absence assertion in shortcuts.test.ts.
- decisionHandlers.test.ts (pure, spec §8C): every id buildDecisionSpec
can emit resolves to a transport route; note/discard routes follow the
gate matrix; compact row ids stay collision-free.
- App.decisionControl.test.tsx (DOM, spec §8D/E16, adapted from the held
App.submitNote.test.tsx harness): Done posts the byte-identical legacy
/api/feedback body, Mod+Enter equals the visible primary, the two note
items do not collapse into one payload, gate approves on /api/approve,
the discard confirm drops annotations, composer Escape keeps the note,
compact touch has a working positive row at zero, and the closed
popover never consumes the HTML Esc ladder's Escape.
- Registered the DOM file in the CI seam-contract step so it cannot
silently skip.
The annotate header's Exit/Send Feedback/Approve cluster becomes ghost-X
Close + DecisionControl driven by buildDecisionSpec. One
submitPrimaryDecision() serves the header primary, the global Mod+Enter
handler (via a render-assigned ref), and the compact primary row. The
zero-state keyboard-only silent submit becomes the visible Done primary
and keeps posting the byte-identical legacy /api/feedback body; the gate
empty primary stays Approve on /api/approve.
Note transports reimplement the held #1436 mechanism: the note becomes
a GLOBAL_COMMENT at submit time with a one-render deferred submit whose
route is captured at menu-choice time. Non-gated "Done with a note…"
carries the approval-framing sentence (buildCompleteAnnotateFeedback
approvalFraming); discard flows post the legacy zero payload with empty
annotation arrays on their flow's endpoint.
Deletions per spec §5.3-§5.5: getAnnotateApprovalPolicy + its confirm
dialog and suppression refs, exitWarningAction's approve half,
requestAnnotateApprove, handleHeaderAnnotateApprove/Feedback, the
annotate half of the shared Approve cluster and its header props.
Compact/touch rows are generated from the same spec (visible positive
action in every state); composer items open DecisionNoteDialog, the
discard item the shared ConfirmDialog. Header predicate consolidation:
the flip is hasFeedbackToSend, so agent-terminal-delivered feedback no
longer leaves a stale Send Feedback primary. decision-control scope
registered in the annotate settings shortcut registry; the
submitAnnotations entry now describes the adaptive primary.
F2 (maintainer default, option A, pending final confirmation): the
count:0 + hasFeedback:true cell now offers approve-with-notes on capable
approval flows with zero-form copy (no annotation-count language);
discard-and-finish stays omitted (nothing to discard).
F6: when the live spec changes while the composer is open and the active
item id leaves the spec, the control morphs back to the menu (draft
kept); an emptied menu closes. Implemented in the control so PR2/PR3
cannot diverge.
Review F1: the hook's document-level Escape dismissed the popover but let
the event travel on to the apps' window-level Escape ladders — from PR2
one Esc would both close the menu AND drop an HTML-annotate surface to
Interact. The hook now skips defaultPrevented events and, when it acts,
calls preventDefault + stopPropagation (document bubbles before window).
An outside-focus Escape while the composer is open closes the whole
popover — an outside-dismissal gesture like an outside click; the draft
is kept — commented at the dismiss call site. New DOM test pins all
three: open+outside-focus consumes, composer case closes-all keeping the
draft, closed control consumes nothing (verified failing without the
hook fix).
Review F3: pin the frozen 'Request changes…' label (it is in the spec's
frozen set but was never asserted) and downgrade the unfrozen sentence
pins (composer action labels, approve-note labels, discard sentences) to
data assertions — flow verb, live count, singular form, composer
distinctness — per the Testing Rules.
Internal (non-host-surface) decision-control primitives only:
DecisionControl, decisionSpec, useDismissablePopover join the
unsupported list — no supported import is added. Core pin stays exact
at 0.25.1; smoke:package verified the 0.38.0 tarball.
One scope at the shortcuts root (both apps mount the identical control)
documenting only the two chords the control implements: Mod+Enter
submits the note, Escape steps back keeping it. Enter is deliberately
not documented as submit — it is a newline. Registry test guards that
the scope composes into both adopting settings registries without id
or token collisions ahead of the PR2/PR3 registration.
The joined split control: incumbent primary segment (never opens the
popover, never fades while it is open — the popover holds only
alternates, so the primary keeps its meaning), caret popover with
role=menu rows and roving arrow-key focus, in-place composer morph
(Esc ladder consumes on exactly two rungs, drafts kept; Mod+Enter
submits trimmed; plain Enter is a newline; empty note refocuses instead
of graying), and the single discard confirm through ConfirmDialog.
useDismissablePopover is the shared pointerdown-outside + Escape effect
with the framed-surface strategy: window blur to an iframe dismisses,
since iframe clicks never reach the parent document. ActionMenuItem
gains additive role/className props and ExitButton an additive
appearance='ghost' form; defaults are byte-identical for existing
consumers. DOM tests registered in the CI seam-contract step so they
cannot silently skip.
buildDecisionSpec maps { app, gate, count, hasFeedback,
approvalNotesSupported } onto one primary plus ordered menu items —
labels, subtitles and confirm strings verbatim from the approved
prototype (DESIGN_final-proposal.html). approvalNotesSupported gates
every approve-carrying item; the discard item is count-gated so it can
never offer to discard zero annotations; frozen copy is marked inline.
Pure (no React, no DOM) so the full state matrix runs in the plain
bun test lane.
Independent quality review of the merged Viewer.annotationHeader work
(#1450) verdicted publish-after-renumbering: a new public API is a
semver minor, and 0.36.1 never reached npm so the number is free.
- packages/ui to 0.37.0; HANDOFF/README version prose updated, with the
never-published 0.36.1 noted in the install matrix.
- StickyHeaderLane's visibility=always / sticky=false pair is
soft-deprecated in HANDOFF: shipped in 0.36.0, its one intended
consumer moved to Viewer.annotationHeader before adopting it, and it
has no known consumers. Retained and tested; no new integrations.
- README's annotationHeader section now mentions actionsLabelMode (the
review's one doc gap): omitting it costs earlier stacking, never
breakage.
* feat(ui): allow hiding the quick label tool
* docs(ui): review follow-ups for the hideQuickLabel seam
Independent review verdict was merge-with-minor-changes; this closes the
three findings:
- StickyHeaderLane now forwards hideQuickLabel to its compact toolstrip,
so a host's opt-out survives the pinned scroll header instead of the
Label button reappearing on scroll.
- HANDOFF.md gains the AnnotationToolstrip row on the supported-imports
table (the strict-consumer gate's convention): the showHelpLink warning
(the help modal embeds Plannotator's own videos), the hideQuickLabel /
hideInputMethodSwitch opt-outs, and the caveat that hiding the button
does not clamp 'quickLabel' mode - host mode state must stay out of it,
including preferences restored through utils/editorMode.
- README.md gains the matching 'Toolstrip host props (0.35.0)' section.
* 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.
* 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.
The e2e case from #1444 spawns the real CLI, whose module load imports
../dist/index.html and ../dist/review.html. Dev machines always have
those built, but CI's test job does not build the apps, so the spawn
died at import time with 'Cannot find module' before the guard ran and
the assertion on the unknown-command message failed, breaking main.
The test now drops empty placeholders for any missing artifact before
spawning and removes exactly what it created afterwards; a real dist is
never touched (verified with sentinel content). The guard under test
exits before either page is served, so placeholder content is
irrelevant.
A typo'd subcommand fell through the dispatcher to the plan-hook path,
which reads the hook payload from stdin. From a terminal that stdin never
closes, so `plannotator annotatte README.md` hung with no output instead
of reporting the typo.
Reject an unrecognized first token before the dispatcher runs: print the
offending word, the nearest documented command, and a pointer to --help,
then exit 1. Flags and the no-argument hook invocation are left alone so
the real hook path is untouched.
* fix(opencode): show the session URL on OpenCode 2's native command path
On OpenCode 2 a remote session's URL was invisible. runNativeCommand builds
its bridge client with createV2BridgeClient, which deliberately has no tui
domain, so toastPlannotatorUrl optional-chained to a no-op; both URL delivery
paths (the CLI stderr forwarder and the ready-file poller) route through it.
The V2 client's app.log is console.error, and OpenCode discards a server
plugin's stderr under both default launch modes (packages/cli/src/services/
standalone.ts uses stderr: "ignore" unless OPENCODE_PRINT_LOGS=1). Remote mode
also suppresses the browser, so /plannotator-review showed the user nothing at
all and presented as a hang.
Deliver the URL as a visible transcript notice instead. createSessionUrlNotifier
duck-types ctx.session.synthetic and exposes it to cli-bridge as notifyUrl, a
seam toastPlannotatorUrl prefers over the toast when present; OpenCode 1 clients
carry no notifyUrl and keep their real toast unchanged. The notice is posted
with resume: false, which upstream skips the wake for, so nothing starts a model
turn, and it carries the URL in both text and description because the TUI drops
a synthetic row whose description is empty and renders the description rather
than the text. Everything is guarded: a host without session.synthetic, or a
call with no session, gets no notifier and falls back to today's log-only
behavior, and a rejecting synthetic is caught and leaves the URL retryable by
the other delivery path.
The README's remedy line claimed remote sessions should read the URL from the
OpenCode log, which was never true; it now describes the transcript notice and
names OPENCODE_PRINT_LOGS=1 for older hosts.
Also fixes two bugs in the OpenCode 2 native-command smoke:
- scripts/opencode2-native-commands-smoke.sh looked for a node_modules/.bin/
opencode binary. @opencode-ai/cli publishes opencode2 on every dist-tag, so
the script failed before it started a server. It now tries both names and
reports which it looked for.
- The command-ownership check read /api/command once, immediately after
activation, racing the reclaim schedule whose last tick lands about 15.5s
later. Under PLANNOTATOR_SMOKE_EXPECT_NATIVE=1 that reported a shadowing bug
the reclaim had simply not reached yet. It now polls to a 30s deadline
(PLANNOTATOR_SMOKE_COMMAND_TIMEOUT_MS), still only after /api/plugin reports
the plugin loaded.
AI-assisted (Claude) under maintainer direction.
* fix(opencode): deliver the session URL on OpenCode 2's plan review path too
The first commit fixed only the native command path. The plan path builds its
own client (createV2Client, typed as { app: { agents, log } } with no notifier),
so a remote OpenCode 2 user who reached a review through submit_plan still never
saw the URL: no browser is opened for them and the plugin's console output is
discarded by the host.
The plan path now builds the same bridge client the command path uses, with
toolContext.sessionID, so it carries notifyUrl whenever the host exposes
session.synthetic. That covers both runtimes: the CLI runtime already prefers
notifyUrl inside toastPlannotatorUrl, and the embedded runtime's previously
empty logReady hook is now createPlanReadyNotifier.
That hook still does not log. app.log is console.error, the same stderr
handleServerReady already printed the URL to, so logging there would duplicate
the line in remote mode and add a stray one locally, which is why the hook was
empty. The transcript notice is a different surface, and it is the only one a
remote reviewer can see. Without session.synthetic the hook stays silent exactly
as before.
createV2Client is gone: it duplicated the bridge client's URL-deduped app.log
verbatim, and nothing else used it.
Three tests on the plan path (delivers the notice; stays silent and does not
re-log without synthetic; catches a rejecting notice) plus one that pins the two
wiring seams at source level, since the notifier tests all pass while the plan
path is wired to nothing, which is the shape the bug had.
Also from review: console.error is stubbed across the V2 URL delivery block, so
those tests no longer print URL lines into the suite output. The README bullet
now says the notice covers every way a session opens rather than slash commands
alone.
AI-assisted (Claude) under maintainer direction.
* feat(opencode): restore the slash commands on OpenCode 2
OpenCode's V2 plugin API gained native command execution upstream
(anomalyco/opencode issue #2185, PR #44765): ctx.command.transform lets a
plugin add a command whose execute callback fully owns the invocation. That
shape currently ships on the beta and dev dist-tags of @opencode-ai/plugin
while next and latest still carry the older context, so the capability is
duck-typed at runtime and never imported. On a host that exposes it the V2
adapter registers /plannotator-review, /plannotator-annotate and
/plannotator-last and runs the same handleCliCommand machinery OpenCode 1
uses, passing the raw argument tail straight through to the CLI. On a host
without it nothing new is registered and behavior is byte-identical to before.
Also wires ctx.session.switchAgent (same API generation, same probe) so an
agent switch chosen in the review UI is applied instead of only warned about,
and accepts both agent.list() response shapes: the HTTP client types it as a
{ location, data } envelope while the in-process plugin domain answers with a
bare array, where reading .data threw and silently emptied the agent list.
The shared command stubs get model-mediated fallback bodies for OpenCode 2
hosts on the stale channels. They carry no shell interpolation on purpose:
OpenCode 1 evaluates a template's !`...` before the V1 plugin's
command.execute.before hook can clear the parts, so a bang template there
would launch a second Plannotator session on every OC1 invocation. A source
level test pins that.
AI-assisted (Claude) under maintainer direction.
* fix(opencode): probe the command draft and reclaim the names from the stubs
Review found the capability probe was wrong in the direction that matters.
ctx.command.transform exists on pre-#44765 hosts too: our own pinned
@opencode-ai/plugin@0.0.0-next-16775 declares CommandDraft as
{ list, get, update, remove } with no add. The probe therefore returned true on
next and latest, draft.add was undefined, and because transforms are stored and
replayed the TypeError landed in the batched reload flush and aborted it before
commit, plausibly taking every command registration on the host down with it.
Capability is now read from the draft handed to the callback, which is the only
witness, and the registration call is wrapped so no transform rejection can fail
plugin setup.
The stubs also shadowed the native definitions on new hosts. Command definitions
land in a name-keyed map where add is Map.set, transforms replay in registration
order, and OpenCode's own ConfigCommandPlugin activates in the post group after
package plugins while scanning the exact directory the installer writes the
three stubs to. A setup-time registration is therefore always overwritten on a
normal install. The plugin now re-registers the same transform once activation
settles, so its definitions are last in the replay order, and calls
ctx.command.reload() explicitly because a late registration only adds its reload
to the already-flushed boot batch. Ownership is read back from
ctx.command.list() by description, which is why the native descriptions and the
stub frontmatter are deliberately distinct. If the reclaim cannot run the stubs
keep the names and the commands still work through their fallback bodies.
Also: a failing switchAgent no longer costs the reviewer their feedback on the
command path, feedback is delivered as "queue" rather than replaying the
invocation's admission mode minutes later when a steer would land mid-turn, and
the agent-list comment no longer asserts a bare-array response that could not be
reproduced upstream (accepting both shapes is still right, since reading .data
blindly throws into a catch that degrades silently).
Tests: the real old-host draft shape registers nothing and throws nothing, the
shadowing contest is modelled against upstream's replay semantics, the OpenCode 1
parts-clearing invariant is pinned for all three commands in both plan-agent and
manual mode now that the stubs carry real instructions, and the V2 smoke asserts
the plugin did not activate as failed and that all three commands resolve. The
smoke now also installs the stubs into its sandbox config dir so the contest
actually happens there. scripts/opencode2-native-commands-smoke.sh runs the same
smoke against a dev-channel build with native commands required; CI cannot,
because it pins a next build.
AI-assisted (Claude) under maintainer direction.
* fix(opencode): keep the reclaim ticking and stop an unbuilt checkout failing setup
The reclaim ended the loop when the draft-probe flag read false, but that flag
only flips when the transform replays, which under boot batching is the flush
after every plugin has loaded. Plannotator loads before the post-group config
plugins, so the first tick legitimately reads false and the loop exited for
good: the reclaim was inert in exactly the shape production has. The tick is
skipped now instead, with a test that flips the flag between ticks.
The V1 entry called resolveBundledHtmlPath synchronously during plugin
construction, outside the .catch that was there to absorb a missing asset, so an
unbuilt checkout threw out of construction before any code path that needs the
HTML. The Test workflow runs bun test with no build step, so the new OpenCode 1
interception tests failed there. Both preloads are guarded; the lazy getters
still raise a clear error if something actually needs the file.
The smoke's failed-plugin guard read entry.state.status, but Plugin.Info carries
status and error at the top level, so a failed activation slipped through.
Reads the top level first and keeps the nested one as a fallback.
Comment corrections: State.batch clears its active flag before flushing, so a
late transform registration materializes on its own; the explicit reload() is
redundant-but-defensive rather than required. The reclaim schedule is a list of
deltas the loop awaits in turn, so the ticks land near 0.3s, 1.5s, 5.5s and
15.5s, not at the raw numbers.
AI-assisted (Claude) under maintainer direction.
* feat(review): mark files viewed as you scroll past them
Reviewers reading the all-files diff top to bottom had to check every file
off by hand. Now a file marks itself viewed when the reviewer MOVES ON from
it, after its content was actually on screen long enough to have been read.
Arriving at a file never marks it; leaving it downward does.
- All-files surface: a file marks when the reader scrolls past it (its
successor has reached the viewport top, so it genuinely scrolled out above)
and has accumulated at least 1000ms as the reported reading file. Dwell is
cumulative per diff snapshot, so bouncing between two files still accrues,
while a momentum flick to the bottom marks nothing. The last file, which can
never scroll out above, marks on reaching the end of the diff.
- Single-file panel: opening a file never marks it; navigating away after the
same dwell floor does. Keyboard file navigation drives the same panel
switches, so keyboard-only parity is automatic.
- Collapsed cards never mark. Generated files seed collapsed, so nobody
reviews a lockfile by scrolling past its folded header.
- Un-viewing a file suppresses auto-view for it until it is marked viewed by
hand again. That set rides the review draft as an additive optional field.
- Inert inside the Guided Review takeover and on a commit detour, where the
files on screen are not the change under review.
- A viewed file whose patch changes under a refresh loses its checkmark, but
only while auto-view is on, so the off state stays byte-identical to today.
- PR sessions batch the marks into one /api/pr-viewed request rather than one
per file.
The setting is reviewAutoViewed, cookie-only and on by default, with two off
switches: Settings > Git and a row in the file-list gear popover. The first
time auto-view actually fires, a toast says so and offers Turn off; using
either switch consumes that one-time notice.
The decision core is pure and clock-injected (utils/autoViewed.ts), the
binding is a hook (hooks/useAutoViewed.ts), and AllFilesCodeView only gains
one optional emission callback on the rAF path it already runs. No server
changes in either runtime.
AI-assisted (Claude) under maintainer direction.
* fix(review): scope auto-mark-viewed to the transitions it was meant for
Four review findings on the auto-mark-viewed branch.
Rule 5 fired on EVERY applied diff switch, not just the staleness refresh.
The review app funnels every transition through one apply path, so entering
the Commits detour (the rail auto-opens HEAD), switching base branch, and
toggling hide-whitespace all un-viewed files whose per-path patch text
legitimately differs, which contradicts both Rule 4's "a commit detour is
inert" and Rule 5's own rationale. The apply path now goes through
resolveDiffSwitchUnviews, which requires the caller to opt in
(`contentRefresh`) and re-checks the identity of the diff on top of that:
same selection, same base, and never a commit-family type on either side.
Only the staleness refresh and the post-fetch base refresh opt in. The pure
delta resolver is unchanged. A source-level test pins which call sites may
opt in, since that is where the guarantee actually lives.
The at-bottom branch fired on the mount tick. A diff shorter than the
viewport is at-bottom from the very first report, and that report is the
mount seed, so the file on screen marked itself about a second later with
zero interaction and fired the first-time toast at a motionless page. It now
requires a real scroll event on the current file set.
Staging a file marked it viewed without clearing auto-view suppression,
unlike v, the header button and the tree row, so a file the reviewer
un-viewed and later staged stayed permanently off-limits to auto-view.
Dwell accrued while the setting was off, so enabling mid-read could mark the
current file instantly on time the reviewer spent with the feature
deliberately disabled. Disabled is now fully inert: the clock does not
accrue, and enabling starts a fresh one rather than replaying the gap.
AI-assisted (Claude) under maintainer direction.
* chore: refresh pinned guide viewer manifest after merging main
Michael Ramos