mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
main
189 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Ramos
|
782d9740c1 |
feat(annotate): carry agent-facing element context on HTML and live-app pinpoints (#1517)
A pinpointed element exported as a one-word placeholder ('[element:
Navigation]') or its flattened textContent. The bridge now captures a
bounded description at click time (tag, id, classes, ancestor path, role,
accessible name, allowlisted attributes, rendered text, an adaptive HTML
skeleton, box, landmark, heading, component hint, live route) as
elementContext; the parent re-validates and re-caps it; the export prints
a fenced skeleton plus selector/path/role/name lines under the comment;
the annotation panel gains a per-row Copy for element-bearing cards; the
feedback archive records element identity. Additive: annotations without
the field export byte-identically, share links drop it, no protocol bump.
|
||
|
a4180p
|
daa26faa2b |
fix(pi): persist approved plan checklist progress (#1496)
* fix(pi): persist approved plan checklist progress * test(pi): restore checklist progress from prior sessions * feat(pi): mark plan steps during execution * fix(pi): align mark_done instructions and harden checklist rendering Review fixes for #1496: - phase-tools-runtime: pin plannotator_mark_done in the executing-phase active tool union and its release at agent_end. - Align the plan-approved doneMsg with the executing framing: call plannotator_mark_done after each completed step, [DONE:n] markers as the fallback for interrupted executions. - Harden renderCompletedChecklist: restrict the shared checklist pattern's whitespace to [^\S\n] so a whitespace-only checkbox line can never swallow the next line (the root cause the review named — \s+ crossing newlines), replace the marker at the match's own capture position instead of first-occurrence line.replace("[ ]", ...), and mirror parseChecklist's empty-text skip so ordinals stay aligned. Regression test: blank checkbox placeholders around real steps; the real step's box flips, the placeholders are never touched (revert-verified). - Document the one-turn ordinal-desync window when an agent edits the plan's checkboxes mid-turn, bounded by upgrade-only writes plus the per-turn re-parse. --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
FND
|
e6822a256e |
fix(shared): resolve data-dir paths per call instead of capturing at import (#1490)
* fix(data-dir): resolve the data directory per call instead of at import * fix(shared): refresh agent schema files once per process, not once ever --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Graeme Folk
|
151a1e8cbf |
fix(review): stabilize JJ line-of-work bases (#1497)
* fix(review): stabilize JJ line-of-work bases * fix(review): scope switch-handler context adoption to gitbutler and jj Maintainer review follow-ups: keep plain-git sessions serving their launch-frozen context on reload (the recomputed worktree context still rides the switch response transiently), truncate frozen 40-hex commit ids in the jj-line header label, and mark the two deliberate test pins. --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Michael Ramos
|
9130d2d6a3 |
feat(review): open a review on a specific base and diff type (#1484)
Adds two session-only flags to plannotator review, parsed in the shared parser so every host inherits them together: - --base <ref> opens the session against a caller-chosen compare target (branch, origin/<branch>, tag, SHA, HEAD~N), probed with git rev-parse --verify --end-of-options before the server starts so a typo'd ref is a startup error with near-match suggestions instead of a silently mislabelled merge-base->HEAD diff. - --diff-type <id> opens the session in one of the nine flat git diff modes (REVIEW_OPEN_DIFF_TYPES, pinned against GIT_DIFF_TYPES). The flags are a seed, never a setting: nothing writes config.json or any review cookie, and the UI stays fully mutable. Validation is pure in packages/shared/review-open-state.ts (provider matrix errors on jj/GitButler/P4/workspace/PR mode, promote-with-notice when the saved default is base-irrelevant, fatal explicit contradiction). A flagged base rides explicitBase semantics: the new initialBaseExplicit server option (both runtimes) seeds baseExplicitlyChosen, suppressing the startup origin/* upgrade and canonicalization, and openStatePinned rides /api/diff so the client neither offers the first-run setup dialog (its one-time cookie is NOT consumed) nor runs the panel-pair self-heal for a pinned session. The since-base dropdown label now renders from the live active base, matching the adjacent base picker. Coverage: Bun CLI, opencode-review bridge, OpenCode embedded plugin, and the Pi extension (re-vendored; strict validation on the slash-command path only, programmatic callers unchanged). Skills, command stubs, help text, and docs updated across every host surface. |
||
|
Michael Ramos
|
ea36ea2183 |
fix(review): strict argument parsing for unknown review flags (#1483)
parseReviewArgs now reports argument-shape problems through an always-present errors[] field instead of letting unknown dash-prefixed tokens fall into the ignored positional list. All four host surfaces refuse to start a session on a parse error: the CLI and the opencode-review bridge exit 1 with the errors on stderr, and the Pi and OpenCode plugins notify through their hosts. Plain non-dashed words stay tolerated for slash-command hosts that forward raw user prose. The loop is index-based so value-taking flags can consume their value token. |
||
|
Ben Drucker
|
9c85310151 |
fix(doc): deny symlink escapes on /api/doc reads (#1437)
Gates /api/doc reads on realpath containment: a path must resolve inside the project both as written and after symlink resolution, else 403. Consolidates the duplicated doc-read logic from both server runtimes into one shared module (packages/shared/doc-resolve.ts, vendored to Pi). Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf |
||
|
FND
|
9cd6cf591b |
fix(test): isolate test-run data from contributor history (#1473)
Closes #1455. Resolves the data directory per call in shared storage and sandboxes PLANNOTATOR_DATA_DIR for every bun test via the preload, so test runs can no longer write into a contributor's real ~/.plannotator. Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf |
||
|
Michael Ramos
|
91d05511b8 |
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
|
||
|
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. |
||
|
Michael Ramos
|
18a6451dcc |
Merge remote-tracking branch 'origin/main' into feat/decision-control-pr7
# Conflicts: # packages/core/guide-viewer-manifest.ts |
||
|
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
|
||
|
Michael Ramos
|
5f938f625f |
feat(server): approval-notes capability advert + shared approved-message composer (PR5)
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 |
||
|
Leonardo Reis
|
2e15a9e4f7 |
feat(review): compare local branch with remote (#1451)
* feat(review): compare local branch with remote * chore(guides): refresh viewer manifest * fix(review): hide remote diff from first-run setup * chore(guides): pin CI viewer manifest |
||
|
Michael Ramos
|
84dbdfc98a |
feat(archive): record annotation scope in the feedback archive
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). |
||
|
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. |
||
|
Michael Ramos
|
7d6dd29c08 |
perf(ui): load the HTML viewer bridge by URL for hosts, with a protocol version and ready timeout (#1398)
Opt-in bridgeScriptUrl on HtmlViewer so multi-chunk hosts can serve the 185 KB bridge as a hashed asset instead of an inlined string; the inline bridge stays the default and Plannotator's own builds, the Pi and OpenCode copies, and the live-app proxy are unchanged apart from a protocolVersion field on the bridge's ready message. The parent checks the version (one warning naming both versions; on the URL path a dismissible banner plus onBridgeUnavailable while the old bridge keeps working), arms a ready timeout on the URL path only, and resolves the URL against the parent document before it reaches the frame so a page's own base href cannot redirect the load. A prepack-generated bridge-script.asset.js (byte-for-byte the inline string) and a bridge-script.lite.ts alias target ship in the tarball. CSP and CORP requirements for hosts are documented. AI-assisted (Claude) under maintainer direction. |
||
|
Ashish Huddar
|
4c5369ecfc |
fix(call-flow): keep big-but-valid tree lists instead of failing (#1370)
MAX_TREES=100 rejected CallDiff results at 41 changed files in
Swift-style languages (many small per-entry trees), making Call Flow
unusable on normal branch reviews. MAX_NODES was never the binding
constraint (3,138 nodes at 470 trees).
- Raise MAX_TREES to 2,000 so realistic reviews parse untouched.
- Truncate deterministically at the cap instead of throwing, and add a
warning diagnostic ("Showing the first 2,000 of N call trees") so the
omission is never silent.
- Keep hard failure for the unbounded-output guards (MAX_NODES,
MAX_TREE_DEPTH, raw length) that protect against pathological worker
output.
Fixes #1351
|
||
|
Graeme Folk
|
e206a1f5e8 |
fix(review): infer the jj line-of-work base from the fork point (#1365)
* fix(review): detect JJ mutable line-of-work base Use JJ's mutable-stack revset to find the line boundary directly instead of inferring a parent from bookmark ordering, which is ambiguous because JJ has no current bookmark. * fix(review): harden the JJ line-of-work base inference Maintainer follow-up on the line-of-work base detection. Skip the bookmarks `jj git push --change` generates. They name one change, not a line of work, and they do reach the fork point: a colleague's pushed change bookmark arrives as an untracked remote bookmark, which makes its commit immutable and therefore a candidate base, so the reviewer was told they were comparing against `push-vmopwunwxopv@origin`. The commit id is used instead. Pass a full commit id through `jjCompareTargetRevset` as a revision. It has no separators, so the commit-id fallback was being wrapped as `bookmarks(exact:"<sha>")`, which resolves to no revisions and made the whole Line of work diff fail. Fall back to `trunk()` instead of throwing. The only live caller is `getJjContext` on the review startup path, which has no handler above it, so a throw aborted `plannotator review` with a stack trace before the server was built rather than reporting anything. That also covers a `jj` too old for `fork_point`/`reachable`. Make the query explicitly single-record with `latest(..., 1)`. The parser reads one record, and bookmark preference (remote before local) is only meaningful within one commit, so the tie-break belongs in the revset rather than in a silent "first row wins" slice. Isolate the real-jj test behind its own JJ_CONFIG. It was reading the developer's real config, where `[signing] behavior = "own"` alone makes it fail with a GPG error. Live fixtures cover the generated-push-bookmark stack, the untracked remote push bookmark, and a stacked local bookmark. --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
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. |
||
|
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. |
||
|
Michael Ramos
|
271fcefded |
fix(server): live-proxy injection and config write hardening (#1364)
* fix(server): live-proxy injection and config write hardening Four confirmed pre-release QA findings, each with a test that fails on the pre-fix source. live-proxy: the HTML injector scanned for head markers with no notion of comments, so a codegen banner naming <head> before the real tag captured the bridge script into a dead comment span: never executed, annotation silently broken, no warning. The scanner now skips comments and the '>'-terminated markup-declaration / bogus-comment spans (doctype, CDATA-ish, <?...>) before matching, inside the same chunk-boundary state machine. Raw-text element contents are still not tracked; that limit and its degraded outcome are documented in the source. live-proxy: new URL(req.url) ran before Host validation, so a Host-less HTTP/1.0 request threw and served Bun's internal debug page with a stack trace. Host validation now runs first, and URL construction takes the same 403 path on failure. live-proxy: the text/html content-type test was case-sensitive, so a valid TEXT/HTML response skipped injection and the framing rewrites. config: saveConfig was an unlocked read-merge-write, so two processes sharing a data dir dropped each other's keys while both reported success. The read-merge-write now runs under an O_EXCL advisory lockfile with a bounded wait and stale takeover, degrading to the old behavior with a warning rather than ever hanging, and the write itself is temp+rename so lock-free readers cannot observe a torn file. Also consolidates the duplicated agent-terminal side predicate onto the single definition in @plannotator/core. * fix(annotate): give live app sessions their own draft slot mode "annotate-app" resolves markdown to "" by construction (the page lives behind the proxy, not in a string the server holds), and the autosave draft key was contentHash of that body. Every live session on the machine therefore collapsed to the one hash of the empty string and shared a single draft slot: two sessions against different dev servers read and overwrote each other's in-progress annotations, deterministically. A live session's identity is its target, exactly as a folder session's identity is its folder path, so the key is now derived from the target URL (normalized through the URL parser so the same dev server recovers its draft when spelled with or without a trailing slash). Classic file and folder keying is untouched. Pi has no live app mode (no annotate-app, liveApp or live-proxy outside its vendored generated/ tree), so there is nothing to mirror there. |
||
|
Leonardo Reis
|
81ecd67e75 |
feat(annotate): configurable Agent TUI placement with durable config and Hidden state (#1050)
* Allow annotate terminal to dock on either side * Allow annotate terminal to dock on either side * fix(annotate): persist Agent TUI preferences through the settings registry The Position control introduced in #1050 stored its choice in a cookie via hand-rolled helpers that bypassed the settings registry. Every annotate session runs on its own random port, so a cookie is scoped to one session: the placement silently reset on the next annotate. The sibling `plannotator-annotate-agent-terminal-default` cookie (preferred agent) had the same gap. Both now follow the `conventionalComments` precedent exactly: * `agentTerminalSide` and `agentTerminalDefaultAgent` join `SETTINGS` with serverKey/fromServer/toServer, reusing their existing cookie keys so a user who already picked a side keeps it across the upgrade. * `PlannotatorConfig` gains both as flat keys (only diffOptions, theme, reviewAnalysis and prompts deep-merge in saveConfig), emitted from `getServerConfig()` behind an `isAgentTerminalSide` guard so a hand-edited config.json cannot advertise a side that does not exist. * Both keys are added to the two /api/config allowlists: the Bun annotate server and the hand-mirrored Pi one. The side vocabulary moves to @plannotator/core/agent-terminal (widened to include the `hidden` state added next) so the registry can reach it without closing an import cycle through ConfigStore; the ui util keeps its public API by re-exporting. Regenerates the pinned guide viewer manifest, which shifts by 0.1 KB gz because the settings registry now reaches into core/agent-terminal. AI-assisted (Claude) under maintainer direction. * feat(annotate): add a Hidden Agent TUI position and extract its layout Builds on the Left/Right Position control from #1050. Hidden (third state of the Position control) Hidden is a durable preference that the Agent TUI is not part of this user's layout: nothing is docked, and choosing Hidden while the terminal is open closes it (from either surface that offers the control). It is a default, not a lock. The rail toggle, the Shift Shift shortcut and a message routed to the agent all still open the panel for the session, and none of them rewrites the preference, so explicit intent wins now without changing what happens next session. A `hidden` preference owns no dock edge, so a session open falls back to the historic left placement. Because the Position control lives inside the terminal's own popover, and Hidden closes that popover along with the terminal, the same control is now also in the Settings dialog (General tab, annotate mode). That is the way back from Hidden, and it also answers the review note that Position could not be preconfigured before the terminal was ever opened. It is gated on the terminal actually being available in the session so a remote or runtime-less annotate never offers a dead control. Both surfaces write the same `agentTerminalSide` config value and read it through ConfigStore, so they cannot drift. The existing transient hide affordances (header X, resize handle click and drag-snap, rail toggle, Shift Shift) are unchanged and stay session scoped. A running agent still stays mounted off-layout when collapsed, so hiding the panel never kills the PTY. Review fixes * Extract `getAgentTerminalLayout` from App.tsx into packages/editor/agentTerminalLayout.ts with a table test over {side including hidden} x {open} x {running} x {wideMode} x {belowBreakpoint} x {rightPanelOpen}, asserting the invariants that can actually regress: never docked on both edges, never visible below `lg` or in wide mode, a collapsed running terminal stays mounted zero-width on its own edge, and the right panel is suppressed exactly when a VISIBLE right-docked terminal holds the slot. * Fix `aiSurfaceOpen`, which still read `effectivePanelOpen && rightSidebarTab === 'ai'` after its siblings moved to `isRightPanelVisible`. A right-docked terminal visually suppresses the panel but left the Ask AI model-discovery effect firing for an invisible surface, which is exactly the eager provider work that gate exists to avoid. The layout computation is hoisted above the consumer so it can use the same fact the JSX does. * Document the right-slot invariant at both coordination sites. The asymmetry is deliberate: the panel evicts the terminal (which keeps running off-layout, so reopening resumes the same session), while the terminal only suppresses the panel visually so dismissing it restores the user's place. Symmetry would make every short terminal detour cost the reviewer their open surface. * Name the `useIsMobile(1024)` literal `AGENT_TERMINAL_LG_BREAKPOINT`, tied to the panel's own `hidden lg:flex`. * Restore `hideAgentTerminal()` in the resize hook instead of the raw setter, and point the handle at the resolved placement. AI-assisted (Claude) under maintainer direction. --------- Co-authored-by: Michael Ramos <backnotprop@gmail.com> |
||
|
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. |
||
|
Sun
|
f7068ce31e |
fix(review): GitLab upload artifact fetching via authenticated API with hardened rewrite (#1228)
* fix(pr-artifacts): read GitLab uploads through the token-readable API GitLab serves `/uploads/<secret>/<file>` from a Rails web route that only honors session cookies. A `PRIVATE-TOKEN` request is redirected to the sign-in page, so every GitLab upload attachment referenced by an MR was unreadable — and because the sign-in page is HTML served with HTTP 200, it was rendered as the artifact instead of the file. Route upload links through `GET /projects/:id/uploads/:secret/:filename`, which serves the same bytes for a personal access token. Both the bare `/uploads/...` and project-scoped `/<path>/uploads/...` forms are rewritten. Also stop following redirects into a provider sign-in path: fail with 401 and the matching `gh`/`glab auth login` hint so an auth gap is legible instead of surfacing as a corrupt artifact. The uploads API answers every file with `application/octet-stream`, and the content route serves provider types with `nosniff`, so refine an opaque type from the file extension to keep images and video rendering. Verified end-to-end against a self-hosted GitLab 18.8.0-ee instance. * fix(pr-artifacts): rewrite only real GitLab upload paths The upload rewrite matched `/uploads/<anything>/<anything>`, so a link in an MR body could name a path GitLab never minted and still be rewritten into `/api/v4/projects/:id/uploads/...` with the PRIVATE-TOKEN attached. Because the remainder allowed slashes, a crafted link could append an attacker-chosen path to that credentialed GET, leaving the safety of the request to GitLab's router rather than to this allowlist. Pin the shape GitLab actually mints: a 32 lowercase hex secret and a single filename segment. A non-conforming path is no longer rewritten, so it is fetched verbatim as the ordinary web route exactly as before this feature. Test fixtures move to a real 32-hex secret, and a new case pins that a short secret, an uppercase secret, and a multi-segment remainder are all left alone. * fix(pr-artifacts): diagnose a direct provider refusal as a missing login The sign-in guard only fired on a redirect, but GitLab's `/api/v4` routes refuse a bad or absent token directly with a 401/403 JSON body. Those fell through to `Artifact host returned HTTP 401` at status 502, which reads as a broken artifact rather than the thing the reader can fix. Hoist the sign-in messaging into `providerAuthRequiredError` and reuse it for a direct refusal, so both paths produce the same actionable error at status 401. GitHub's 403 stays a transport status on purpose: it also covers rate limiting and SSO enforcement, where a `gh auth login` hint points at the wrong problem. A GitHub 401 is unambiguous and is diagnosed. The message text moves to a colon instead of a dash to match house style. * fix(pr-artifacts): fall back to the upload web route when the API route is absent `GET /projects/:id/uploads/:secret/:filename` landed in GitLab 17.4. On an older self-hosted instance the rewrite turns an upload that used to load, on a public project where the web route reads anonymously, into a 404 surfaced as a 502. That is a regression the rewrite introduced. When the rewritten uploads API URL answers 404, retry the original web route once before failing. The retry is deliberately narrow: only for the upload rewrite, only on 404, and only when the 404 came from the rewritten URL itself, with no version probing anywhere. It reuses the loop's redirect budget, so the total number of requests stays bounded, and it is same-origin, so credentials are attached on exactly the existing `shouldSendProviderAuth` terms. A sign-in redirect from that retry still produces the actionable 401, so a private project on an old instance reports a missing login rather than a 404. * fix(pr-artifacts): drop the active content types from the extension map `html` and `text/javascript` bought nothing. An HTML artifact is read through `/api/pr-artifact-document`, which always answers `text/plain; charset=utf-8`, so nothing on that path ever consulted this map. Keeping them meant the media route could label an upload as active content, leaving safety resting on the `Content-Security-Policy: sandbox` header staying in place forever. `css` stays because `shouldRewriteCss` keys on `text/css` to rewrite provider references, and the image and video types stay because `nosniff` means an unrefined octet-stream simply does not render. `svg` stays too, on the evidence of how svg uploads actually reach the screen. `.svg` is not in the review editor's IMAGE_EXTENSIONS, so an svg upload arrives either as an authored markdown image, rendered through `<img src>`, which is a non-scripting context by spec, or as a resource referenced from an HTML artifact, which renders inside an `<iframe sandbox="">`. Both are served by the media route with `Content-Security-Policy: sandbox` and `X-Content-Type-Options: nosniff`, so even a direct navigation to the proxy URL lands in a sandboxed, opaque-origin document that cannot run script. Dropping it would give up real rendering for no reduction in reachable capability. Adds a regression test pinning that an .html or .js upload stays opaque. * test(pr-artifacts): pin that the upload rewrite does not widen token reach The rewrite sends PRIVATE-TOKEN to a different path on the provider origin, so the invariant worth guarding is that it did not also change where that token can travel. GitLab object storage answers an upload with a 302 to a signed URL on an unrelated host, which is exactly the hop a credential must not follow. Verified the test bites: forcing shouldSendProviderAuth to return true for gitlab fails it on the second request's header. The module-global auth cache has a 5 minute TTL and no reset seam, so the test resolves the same `test-token` every other gitlab case in this file resolves, which makes it correct whether the cache is cold or warm rather than dependent on test order. --------- Co-authored-by: Sun Neoh <yuensun.neoh@stashaway.com> Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
FND
|
e1ce7dabe1 |
feat(ui): Totman/Classic P favicon style switcher (#1325)
Favicon style switcher in Settings > Theme: the Totman mascot or the historical dark-navy P tile (byte-identical to the pre-Totman asset, sha256 pinned). Served server-side from first paint in both runtimes; opt-in for hosts of the published UI package. Contributed by @FNDEVVE |
||
|
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 |
||
|
Graeme Folk
|
e3091331a5 |
feat(review): jj support for Call Flow analysis (#1312)
Adds Jujutsu (jj) as a Call Flow analysis provider: jj-current/jj-last/jj-line/jj-all snapshot revsets with deterministic first-parent resolution across merge revisions, root-anchored filesets so results are cwd-independent, bounded snapshot materialization (base tree + changed-file delta) with a streamed 64MB output ceiling in both the Bun and Pi runtimes, and real-jj regression tests covering merges and subdirectory invocation. Contributed by @graemefolk, who also built the original jj integration. Review fixes pushed in-branch: merge-parent resolution, root-glob filesets, bounded materialization and buffering, plus CI gating guards for runners without jj. |
||
|
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 |
||
|
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. |
||
|
Michael Ramos
|
14e5c9ebd1 | Fix folder watcher cold-start refs scan (#1306) | ||
|
Michael Ramos
|
5f33b72b2f |
feat(remote): tailnet auto-advertise, ready QR code, and a first-class --tailscale mode (#1280)
* feat(remote): resolve urlHost auto from Tailscale for advertised URLs PLANNOTATOR_URL_HOST=auto (or config urlHost: "auto") detects this machine's tailnet host at first use in a remote session: MagicDNS name from tailscale status --json, falling back to the single tailscale ip -4 CGNAT address. Detection is cached per process, never spawns in local sessions, warns once and falls back to localhost on failure, and stays strictly display-only: binding remains governed by PLANNOTATOR_REMOTE. Pure parsers live in the new @plannotator/shared/tailscale module, vendored to the Pi extension; both runtimes mirror the resolution. * feat(remote): render a terminal QR code for remote-ready session URLs Remote sessions print their advertised URL as the lifeline; the usual next step is opening it on another device (iPad, phone, laptop off the VPS). handleServerReady now also renders a compact unicode QR of that URL via the zero-dependency uqr package, TTY-gated so piped stderr and hook transcripts keep only the plain URL line. Pi keeps URL-only parity: its ready surface is an in-chat notification, not a TTY stream, so a QR block would not render there. * feat(cli): first-class --tailscale mode for review and annotate sessions plannotator review --tailscale (also annotate and annotate-last/last) publishes the session over the user's tailnet: the server stays loopback-bound and the CLI orchestrates tailscale serve --bg --https=<port> http://127.0.0.1:<port>, then advertises the HTTPS tailnet URL with a terminal QR code. Nothing listens beyond localhost and nothing is ever public (serve, never funnel). Guarantees: preconditions fail with actionable errors (CLI missing, daemon down or logged out); a pre-existing serve mapping on the chosen port aborts instead of being stolen and other ports are never touched; every mapping the process creates is torn down on normal completion, SIGINT/SIGTERM, and errors via the exit-routed cleanup handler. When combined with PLANNOTATOR_REMOTE or SSH detection, --tailscale wins and forces local mode with a stderr notice, which also restores the random local port so simultaneous sessions get distinct serve mappings. * fix(remote): await tailscale-ready failures, harden serve teardown and conflict detection Review fixes for #1280 (external review plus internal security review). Startup failures no longer hang the session: startReviewServer and startAnnotateServer now await async ready handlers and stop the server on rejection, and the CLI's --tailscale ready path resolves publishing failures itself with an actionable stderr message and exit 1. Under the bang-prefix skill a hanging loopback server blocked the whole Claude Code prompt. Serve teardown is checked, not assumed: a failed off retries once, then warns with the exact manual command, and a port is only forgotten after a successful off. SIGHUP (terminal close) is now routed through process.exit like SIGINT/SIGTERM so exit-time cleanup runs. Docs no longer claim guaranteed cleanup: --bg mappings survive SIGKILL and reboots, and the manual removal command is documented. Conflict detection sees foreground serve sessions (Foreground.*.TCP), which Tailscale prefers over background mappings, and fails CLOSED on unrecognizable serve status output instead of assuming the port is free. The extracted serve URL must match the requested port, so a version-dependent output shape cannot advertise another mapping's URL. The annotate agent terminal is gated off by default under --tailscale behind the existing PLANNOTATOR_AGENT_TERMINAL_REMOTE opt-in: the PTY token is not an auth boundary against network peers, and tailnet reachability implies terminal reachability. Also: --tailscale is rejected with a clear error on unsupported subcommands and documented in review/annotate/annotate-last and top-level help; the remote-ready QR renders only for URLs actually reachable off-machine (never localhost); urlHost is suppressed for --tailscale runs so the local-session warning cannot mislead; the duplicated auto-host resolution moved into the shared vendored module; tailscale-serve tests restore module and process state via a reset seam. |
||
|
Michael Ramos
|
8e7b5ce300 |
feat(review): refine Call Flow navigation and annotations (#1277)
* feat(review): refine Call Flow navigation and annotations * fix(review): align viewed controls with panel navigation * feat(review): add Call Flow path search controls * fix(ui): wrap long tooltip identifiers * feat(review): annotate raw Call Flow output * feat(review): refine call flow lens context * fix(review): keep call flow lens search accessible * fix(review): scope call flow find shortcuts |
||
|
Michael Ramos
|
fc348687bf |
fix(review): contain /api/call-flow analysis throws as JSON error responses (#1272)
* fix(review): contain /api/call-flow analysis throws as JSON error responses
A hard VCS failure during patch materialization escaped the handler in
both runtimes. On Pi the unhandled rejection reached the process-level
handler and killed the user's session; on Bun it surfaced as a non-JSON
500 the client's quiet-failure UX could not parse. Both handlers now
return the standard { status: "error", reason: "analysis-failed" }
envelope.
* fix(review): cut the Call Flow consent copy down to the three facts that matter
Six sentences of disclosure read as noise. The dialog and Settings now
say: what it does, what it installs (languages + size), Node 22+, and
that other languages install as needed. Nothing consent-relevant was
removed.
* test(review): pin consent-copy facts, not prose
The presentation test now asserts the server-derived facts (languages,
size, Node floor); the dialog and Settings tests assert only that the
disclosure prop renders, via a sentinel string. Copy edits no longer
break three test files.
* docs: add Testing Rules to AGENTS.md (no prose-pinning, no round-trip prop tests)
* docs: refine copy-pinning rule — deliberate locks allowed, incidental snapshots banned
* fix(review): use the maintainer's Call flow description in the intro dialog and Settings
* fix(review): Call flow description is the maintainer's exact copy; remove the dynamic disclosure plumbing
The intro dialog and Settings now show only: 'Diffs for function call
stacks across git commits. 22 languages supported (AST-based, built
using Tree-sitter).' The callFlowEnableDescription prop, its App wiring,
and getCallFlowEnableDescription are removed; install size and Node
requirements remain visible in the Call Flow panel itself.
* fix(review): reject empty-path worktree diff types; clean up QA findings
- parseWorktreeDiffType returns null for a worktree diff type with no path.
An empty path resolved to an empty cwd, and Bun.spawn({ cwd: "" }) runs
git in the server's own directory instead of the target repo, so a
malformed 'worktree:' switch returned an unrelated checkout's diff.
Fail closed to the caller's real cwd. (Pre-existing; surfaced by QA.)
- Remove an orphaned JSDoc comment left by the callFlowEnableDescription
prop removal in Settings.tsx.
- Add useCallFlowAnalysis.test.tsx to the CI DOM_TESTS list; its two
tests were silently skipping on every run.
|
||
|
Michael Ramos
|
caf7ce1ccd | feat(review): install Call Flow automatically in the background on opt-in (#1271) | ||
|
Michael Ramos
|
9ee2e83287 |
feat(review): make the CallDiff runtime a strictly opt-in, in-UI install (#1270)
* feat(review): make the CallDiff runtime a strictly opt-in, in-UI install
The merged CallDiff integration eagerly installed a ~784MB runtime for
every user at install time, for a feature that is off by default. The
runtime is now strictly opt-in and the opt-in lives in the review UI:
toggle Call flow, click Install in the panel, watch staged progress, and
use the analysis in the same session.
Installers: the default sequence no longer installs the runtime. Opt in
with --with-call-flow (PowerShell: -WithCallFlow),
PLANNOTATOR_INSTALL_CALLDIFF=1, or { "installCallFlow": true } in
config.json (flag > env > config). PLANNOTATOR_SKIP_CALLDIFF_INSTALL is
deleted; --minimal keeps excluding the runtime; the installer prints an
honest note pointing at the in-app install. The headless CLI path
(plannotator install-runtime call-flow) is unchanged.
Server (both runtimes, contract-identical): POST /api/call-flow/install
starts installCallFlowRuntime() in the background via a single-flighted
coordinator (concurrent POSTs join the in-flight install), runs a
Node 22+ preflight before any download (distinct node-unavailable
error), and rejects cross-origin POSTs with 403. GET
/api/call-flow/install-status reports idle/running/done/error with
stage: downloading, verifying, installing-deps, building. Install
completion invalidates the 30s runtime probe cache so the next
capability advert resolves available without a server restart.
Client: the Call flow Dock's runtime-missing state is now the opt-in
funnel with an honest disclosure (about 800 MB on disk, Node 22+,
one-time), staged reduced-motion-safe progress, and error + retry with
a no-node hint. On done the advert is refreshed through
POST /api/review-analysis and the existing available-change refetch
starts the analysis for the current snapshot with no reload. The intro
dialog and Settings toggle note the separate first-use runtime.
Docs: AGENTS.md env table + Review Server API table, marketing
environment-variables / installation / ui-settings / code-review /
api-endpoints pages, and the CallDiff ADR runtime-boundary and server
contract sections.
* test(review): stop leaking PLANNOTATOR_DATA_DIR from the install endpoint tests
The call-flow install endpoint tests overrode PLANNOTATOR_DATA_DIR at
module-eval time and never restored it. bun runs CI's full suite in one
process and evaluates every test file's module before running tests,
while Pi's generated/storage.ts caches its data dir at import time; the
override therefore made storage's cached dir and later files' live
getPlannotatorDataDir() calls disagree, failing the Pi annotate-history
unwritable-dir test and both durable-submit-record tests.
An afterAll restore alone is not enough: it reproduces the same three
failures with the mismatch inverted (storage caches the leaked dir at
module eval, tests then run against the restored one). The env var is
now never touched at module-eval time at all; it changes only inside
tests and is restored to its original value in afterEach, exactly like
the PORT/PATH pattern. The config writes the advert tests persist
through the process's frozen config module are snapshotted at load and
restored in afterAll so a standalone run never flips a real
config.json setting, and the process-global scope of the mock.module
seams is documented.
Regression proof (previously failing in either mismatch direction, now
green in both orderings):
bun test packages/server/call-flow-install-endpoint.test.ts \
apps/pi-extension/server/annotate-history.test.ts \
apps/pi-extension/server/annotate-submission.test.ts
* feat(review): install CallDiff grammars selectively
* fix(review): harden CallDiff worker environment
* fix(review): close CallDiff verification gaps
|
||
|
Michael Ramos
|
3245310aa8 |
feat(review): add optional CallDiff call-flow analysis (#1268)
* feat(review): add optional CallDiff call-flow analysis * fix(review): harden CallDiff integration |
||
|
Michael Ramos
|
98113182b5 |
feat(guide): reviewer-supplied extra instructions for Guided Review (#1267)
* feat(guide): reviewer-supplied extra instructions for Guided Review (#1265) Adds a quiet, collapsed-by-default Custom instructions affordance to the guide launch page. The text is APPENDED to the built-in organizer methodology as a clearly delimited section (composeGuideMethodology) and never replaces it; absent or blank instructions produce byte-identical prompts to before. Persisted in a dedicated cookie (plannotator-guide-instructions) so a standing team preference survives sessions without bloating the plannotator.agents blob past the browser's per-cookie limit. Server side, the launch body gains an optional guide-only instructions field (both the Bun and Pi node:http agent-jobs handlers accept and thread it); prompt composition lives in the shared guide-review.ts that vendor.sh already vendors to Pi, so both runtimes compose identically. Text is capped at GUIDE_EXTRA_INSTRUCTIONS_MAX_CHARS (2000) server-side and mirrored by the textarea maxLength. Repair launches deliberately ignore instructions: a repair is a mechanical JSON fix, not a rewrite. Tests pin the regression contract (empty input keeps prior prompt bytes), appended-not-replacing composition, the length cap, repair isolation, and the cookie round-trip via the storage backend seam. * refactor(guide): store standing instructions server-side, not in a cookie Review findings on the cookie approach (silent write failure past the encoded 4KB per-cookie limit for multi-byte text) pointed at the real design problem: the instructions are consumed by the SERVER at launch time, so they belong in the data dir like review-skills.json, where no size ceiling or encoding inflation exists and the preference follows the machine instead of one browser profile. New GET/PUT /api/agents/guide-instructions in both runtimes backed by shared guide-instructions-store (vendored to Pi). Guide launches apply the stored text when the body carries none; the launch page still sends its live textarea value (explicit wins), so a just-typed preference can never race the debounced save. The sidebar surface sends nothing and inherits the stored text server-side. All cookie machinery removed. Also folds in the review fixes: marker-tag-shaped strings in instructions are defanged so first-match nonce recovery cannot be hijacked by pasted examples. |
||
|
Michael Ramos
|
e24bd8464f |
fix(annotate): persist submitted feedback before deleting the draft (#678) (#1237)
* fix(annotate): persist submitted feedback before deleting the draft (#678) * fix(annotate): scope durable submit records to single local files Adversarial verification found the durable record had no mode gate: an annotate-last or URL session, which was completely stateless before, would persist submitted feedback quoting the agent's message or the fetched page under history/, widening the documented annotateHistory contract without a docs change. The record now shares the exact eligibility gate the version history uses (mode annotate, non-URL path), so previously-stateless modes stay stateless. Also makes persistSubmittedDecision defensive about body types: /api/feedback does no validation (unlike /api/approve), and a non-string feedback previously flowed through settle() untouched with a 200; the new .trim() guard turned that into a thrown 500 after the decision had already settled. Malformed values now degrade to the exact legacy behavior (settle, delete draft, 200) instead of throwing. Both changes mirrored in the Pi server, with regression tests in both runtimes: stateless modes write no record, and a malformed feedback body returns 200 with the draft deleted and nothing persisted. |
||
|
Michael Ramos
|
ffd49080ee | fix(skills): harden skill references before first release (#1235) | ||
|
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. |
||
|
Michael Ramos
|
75e8b78cc7 |
fix(review): stop stubbing files whose worktree content the size probe cannot find (#1220)
The oversized preflight in buildBoundedTrackedDiff mapped every object the cat-file batch could not size to infinity. `missing` is routine: for tree-vs-worktree diffs git hashes the WORKING-TREE content of any path pulled into rename/copy detection and prints that hash in --raw output without ever writing the blob, and partial clones report it for unfetched blobs. Those files were excluded by pathspec and replaced with a contents-free binary stub, so a renamed-and-edited file rendered as a silently empty card and /api/file-content refused it as binary. Missing now means unknown, not oversized, and an unreadable new side is bounded by the working-tree file's stat size instead. Every rendered diff stays bounded git-side by core.bigFileThreshold (#1205), genuinely oversized files still stub via real probe sizes plus the stat door, and a blob git truly cannot read now fails loudly through assertGitSuccess instead of blanking a file. Client side, a chunk with a binary marker and no hunks now renders an explicit placeholder in both the all-files view and the single-file viewer, so an empty card can never again pass for "no changes here". AI-assisted. |
||
|
Michael Ramos
|
7ba4e3b3e4 |
fix(review): mint content-derived diff cache keys so single-file tabs render fully (#1219)
* fix(review): mint content-derived diff cache keys so single-file tabs render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree. * fix(review): explain why an oversized file's card has no diff Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown. * test(review): make the diff-swap proof machine independent, not stopwatch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away. * test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated. * test(review): stop a leaked module mock from silently unrendering the diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them |
||
|
Michael Ramos
|
2d65c65596 |
feat(ui): pair a light theme and a dark theme, switched by mode (#1217)
* feat(ui): pair a light theme and a dark theme, switched by mode
ThemeProvider stored one palette plus a mode, so picking a dark-only
palette pinned the mode and greyed out the Light/System buttons. Store a
pair instead: { mode, light, dark }, resolved as pair[preferredMode], so
System flips between the two choices as the OS scheme changes.
The Settings Theme tab now assigns one half at a time. A Light/Dark
switch decides which half the grid is filling, the grid lists only the
palettes that can render that half (from the registry's modeSupport), and
a summary line names both halves with each side clickable. Every mode
button is permanently enabled: a dark-only palette simply never occupies
the light slot, so no mode coercion is left to do.
The pair round-trips through the SETTINGS registry to the `theme` key in
~/.plannotator/config.json the way diffOptions does. A user upgrading
seeds both halves from their stored single palette, and the legacy
plannotator-color-theme key keeps tracking the active palette so a
downgrade never lands on an unstyled first frame.
Addresses part 1 of #1211.
* fix(ui): make the theme pair seed local, and keep the legacy API non-destructive
Review of #1217 found a data-loss path and three published-API regressions.
Seeding: ThemeProvider handed its resolved pair to the config store through
set(), which queues a debounced POST. configStore.init() applies the server
config but never cancelled that queued write, so a single cookie-less visit
(fresh profile, incognito, cleared cookies) flushed a default pair to
~/.plannotator/config.json AFTER the real one had arrived, and the next
session restored those defaults over the user's cookies. The provider now
uses a new configStore.seed(): memory plus cookie, never the server, and
never over a value init() already applied. init() additionally retracts
queued writes for the leaves the server just spoke for, which closes the
same race for every server-synced setting rather than this one key.
Deprecated APIs: isThemeModeAvailable() and normalizeThemeMode() are back as
one-line wrappers with @deprecated notes, since packages/ui exports utils/*.
setColorTheme: assigns exactly one half and nothing else. A both-mode palette
goes to the half on screen instead of clobbering both; a mode-restricted one
goes to its half without yanking a System user to an explicit mode (render
time already resolves that). It persists through configStore.setLocal(), so
it stays cookie-only as it was before the pair, unless a host installed its
own serverSync transport.
storageKey / colorThemeStorageKey are honored on the read path, so a host's
stored pre-pair preference is migrated rather than discarded. The two halves
have no pre-pair equivalent and stay on fixed keys, documented on the props.
Tests: a fresh-mount case that pins zero POSTs (the previous helper pre-seeded
cookies, which is why this was invisible), a case that pins a real choice
still reaching config.json, direct setColorTheme cases for all three
semantics, a host-storage-keys migration case, and configStore seed/retract
unit tests. All of them fail against the code they replace.
|
||
|
Michael Ramos
|
b1745683fd |
fix(review): fall back across GitButler JSON flag syntaxes (but 0.22.0) (#1216)
GitButler 0.22.0 removed the global --format flag in favor of --json (gitbutlerapp/gitbutler#15026), so `but --format json status` now dies with clap's unexpected-argument error and GitButler review sessions fail to start. 0.21.x accepts only --format json (gitbutlerapp/gitbutler#14061), so neither spelling works everywhere. Keep --format json as the primary invocation and, only when it fails with clap's narrow unexpected-argument rejection for the exact flag we passed, retry once with the other spelling. Real status failures never retry and keep failing loudly per the module's contract-error philosophy. The accepted spelling is remembered per runtime so 0.22.0 installs pay the failed probe once. Error strings now name the syntax actually used. Closes #1215 |
||
|
Michael Ramos
|
3d435184dc |
fix(review): keep large-diff memory bound when the object-size probe fails (#1205)
A failed `cat-file --batch-check` used to map every changed object to infinity, replacing the ENTIRE review diff with `Binary files ... differ` stubs and no visible error, and silently degrading the staleness fingerprint. The memory bound is now probe-independent: every rendered diff carries `core.bigFileThreshold=<MAX_REVIEW_FILE_CONTENT_BYTES>` injected through `GIT_CONFIG_*` environment variables (never `-c` argv flags, so argv stays byte-identical), making git itself stub oversized blobs. On probe failure blob sizes read as unknown-but-bounded and files render normally; the stat-based exclusion door for oversized working-tree files (which git's threshold does not cover) never depended on the probe and keeps working. Per-object doors (missing / unparseable size) stay conservative when the probe ran. The probe itself gains timeoutMs + interaction:"forbid" so a hung git cannot stall the review server. Three existing mocks that returned non-batch-check output (and passed only because of the all-infinity bug) now return well-formed batch-check lines. |
||
|
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. |
||
|
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
|
||
|
Raúl
|
bc5470b90d |
fix(review): bound server memory for large tracked-file diffs (#1167)
* fix(review): bound server memory for large tracked-file diffs PR #1118 renders large untracked files as binary additions, but staging one moves it into the tracked `git diff` path, which had no size guard and buffered the full multi-megabyte patch (~240 MB RSS on a 51 MB text artifact). Any large tracked text file modified in the working tree hits the same unguarded path. Add a per-invocation `git -c core.bigFileThreshold=<MAX_REVIEW_FILE_CONTENT_BYTES>` prefix to every content-producing git diff, so git renders oversized blobs as "Binary files ... differ" instead of a text patch. Their bytes never enter git's diff machinery or the server's buffered stdout, mirroring the untracked-file guard. The flag is a no-op at or below the threshold, so smaller files are byte-for-byte unaffected, and the blob hash git emits in the binary diff still changes with content, so staleness detection holds. The guard is applied in the shared cores, so the Bun and Pi runtimes inherit it identically: `review-core.ts` covers the ordinary git provider (working-tree, staged, commit, and the freshness fingerprint) and `gitbutler-core.ts` covers the GitButler object diff. The jj provider runs `jj diff`, which has no `core.bigFileThreshold` equivalent, so it is out of scope here and stays unbounded as before. * fix(review): preflight oversized tracked diffs * fix(review): batch tracked diff preflight * fix(review): restore browser-safe diff core * fix(review): preserve gitlinks and textconv * fix(review): require filesystem runtime seam Fail compilation when a runtime omits file metadata or symlink support instead of silently disabling bounded reads and expansion. |
||
|
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 |