mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
codex-mobile-touch-selection
163 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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 |
||
|
Michael Ramos
|
d53cbfb373 |
fix(annotate): enforce archive read-only surfaces (#1171)
* fix(annotate): enforce archive read-only surfaces * fix(archive): close remaining read-only leaks |
||
|
Michael Ramos
|
8f84852f97 |
fix(review): surface partial GitLab comment submissions (#1164)
Surface partial GitLab submission outcomes and preserve narrowed, duplicate-safe retries across dialog reopen and same-tab refresh. Follow-up for an explicit blocked-recovery escape: #1166. |
||
|
Raúl
|
c750427ab8 |
feat(annotate): dismiss abandoned gate sessions (#1143)
A direct local `plannotator annotate --gate --json` waits for one authoritative decision. If every review surface disappears without approving, sending feedback, or exiting, the caller blocks forever: the server has no notion of whether a client ever connected, whether another tab is still open, or whether a disconnect is a reload. Page lifecycle events cannot answer that. `pagehide` and `beforeunload` also fire on reload and navigation, so dismissing from them ends reviews the user expects to resume. Use connection presence instead, which is exactly what the transport can observe. Local direct structured gates advertise a client lease in /api/plan and serve /api/annotate/client-lease as SSE. One open stream is one connected review surface. The server heartbeats every 5s and, only after at least one client has connected, starts a 30s reconnect grace when the last one disconnects. A reconnect inside the grace continues the same review; expiry resolves the gate through the same path as explicit Close, so it produces an ordinary `dismissed` decision and inherits the strict-result contract unchanged. Approve, feedback, explicit exit, and server stop all cancel a pending expiry. Presence lives in two runtime-independent pieces so Bun and Pi cannot drift. createAnnotateClientLeaseTracker owns first-client, active-count, reconnect, cancellation, and one-shot expiry. createAnnotateClientLease- StreamSession owns one connected client: acquire the slot, write the ready comment, heartbeat, release exactly once. Each server passes only its own write primitive (a ReadableStream controller for Bun, res.write for Pi). A write that fails closes the session, because a stream that can no longer be written to is a client that is no longer present; holding the slot there would make the gate un-dismissable for the rest of the run, which is reachable only through a half-open connection and so is covered by unit tests rather than an integration test. Scope is deliberately narrow. The capability stays off for remote and shared sessions, where tunnel disconnects would read as abandonment, and off for hook transport, legacy plaintext, archive, plan, review, and folder-picker sessions. A session that never receives its first client never auto-dismisses, so browser-launch failures still need a caller-side timeout. Decision settlement is explicit for the same reason: a connected surface and the lease can both try to settle the session, and the awaited promise ignoring the second resolve was not enough. The loser still deleted the reviewer's draft and answered ok, so a tab reported success for a decision the caller never received. createAnnotateDecisionSettler makes the winner explicit; a loser changes nothing and answers 409. Expiry deliberately keeps the saved draft, unlike explicit Close, so an abandoned review stays recoverable. Stopping the server closes live lease streams instead of only releasing their slots, so a long-lived host process does not retain a heartbeat timer and an open response for every finished session. |
||
|
jms830
|
a54b46bbee |
feat(pi): mirror plan checklist to pi-todos (#1139)
* feat(pi): mirror plan checklist into an editable todo provider Closes the one-way half of #484: on plan approval, feature-detect an editable todo provider and mirror the approved checklist into it, then reflect [DONE:n] markers onto todo status as execution proceeds. pi-todos is the first provider. It exposes no API to other extensions, so the integration surface is its on-disk format (.pi/todos/<id>.md with JSON front-matter, <id>.lock taken with O_EXCL). The provider writes through that contract, reconciles by tag so repeated syncs are idempotent, closes steps dropped from an edited plan, and skips any todo another session holds a lock on. Provider state resets on return to idle, so a second plan re-detects rather than inheriting the first plan's decision. The mirror is additive: the existing progress widget is untouched. pi-todos has no live surface of its own -- its list renders on demand in /todos -- so suppressing the widget would trade a visible tracker for files behind a keystroke. Sync is one-way, so provider-side edits can never desync plan execution. oh-my-pi's native todo panel is the obvious second provider and slots in behind the same interface, but is not implementable from an extension today: the panel repaint is gated to the built-in `todo` tool, and the session handed to an extension-registered tool is a read-only projection without the todo accessors. Details in todo-providers/index.ts. Gated by PLANNOTATOR_TODO_PROVIDER / config.todoProvider, and inert when no provider is detected. * fix(pi): harden todo provider sync --------- Co-authored-by: jms830 <jms830@noreply.github.com> |
||
|
Raúl
|
9a450a69e7 |
feat(annotate): preserve notes on structured approval (#1092)
* feat(annotate): add strict atomic result output * feat(annotate): exit 2 for strict-gate usage and publication errors Adopt the grep convention for the strict annotate gate's exit codes: 0 = approved, 1 = negative human outcome (annotated/dismissed under --require-approval), 2 = the gate itself was misconfigured or could not start/deliver a decision. Previously all usage/startup/validation failures shared exit 1 with "reviewer did not approve", so callers could not tell a denied review from a broken gate. - parseStrictAnnotateOptions failures (bad flag combos, strict flags outside annotate --gate --json) now exit 2 - --result-file preflight failures (missing parent, pre-existing or dangling-symlink destination) now exit 2 - post-decision publication failures (destination raced into existence, hard links unavailable, stdout write failure) now exit 2: they deliver no decision record at all, so the code's own fail-closed handling presents them as environment errors, never as a reviewer outcome -- and never approval, since only 0 means approved - decision outcomes keep 0/1 exactly as before; signal deaths keep 128+n - document the contract in AGENTS.md and the annotate-gates guide Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk * feat(annotate): preserve notes on structured approval * test(pi): use exact annotate outcome import * fix(annotate): exit 2 for strict-gate startup failures The six startup-failure sites in the annotate path (missing path, unreachable URL, empty folder, ambiguous name, missing/unsupported file, oversized file) run after flag parsing and exited 1. Under --require-approval / --result-file, 1 is the "reviewer requested changes" signal, so a typo'd path made automation misclassify a configuration error as a legitimate rejection. Route those sites through exitAnnotateStartupFailure(), which picks its code from the already-parsed strict options via the new pure helper annotateStartupFailureExitCode(). Non-strict invocations still exit 1 with byte-identical stderr; strict invocations exit STRICT_GATE_ERROR_EXIT_CODE (2). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): emit the strict decision on stdout before publishing it writeResultFile ran before the decision JSON reached stdout. On a filesystem without hard links (exFAT, FAT32, most SMB/NFS, some container bind mounts) publication fails deterministically, the catch exited 2 with nothing written anywhere — and the reviewer's autosaved draft had already been deleted by the feedback flow, so their completed decision was lost. Emit the stdout record first, then publish the result file. Exit semantics are unchanged: a publication failure still exits 2, but the decision has reached stdout by then. Only a stdout write failure now leaves no record at all. Correct the docs and comments that claimed exit 2 delivers no decision record: it means the result *file* was not published. Also document the two publication caveats: the 0600 mode is a no-op on Windows, and the atomic link/rename is not followed by a parent-directory fsync, so publication is atomic but not crash-durable. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): parse linked docs with the render-side frontmatter rule on export buildCompleteAnnotateFeedback re-parsed each linked document with parseMarkdownToBlocks(entry.markdown) — no options, so frontmatter stripping defaulted on. The render side parses with { frontmatter: shouldStripFrontmatter(path) }. For plain-text linked docs (.yaml/.json/.toml/…) a leading `---` is real content, not frontmatter: a multi-document YAML opens with it. Stripping it on the export side shifted every block id, so ordinary Send Feedback and deny emitted wrong `(line N)` labels — or dropped them entirely when the annotation's block no longer existed. Pass the same shouldStripFrontmatter(filepath) option at the export call site so both sides agree. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * fix(annotate): carry the message scope through approve-with-notes /api/feedback forwards selectedMessageId and feedbackScope; /api/approve dropped them. Pi resolves the anchor message from those fields, so notes delivered on the approve path anchored to the last message instead of the one the reviewer picked in a multi-message annotate-last session — while Send Feedback in the same session anchored correctly. Forward both fields on the approve path in the Bun and Pi servers, and have the client build the approval body with the same scope resolution Send Feedback uses (extracted as getFeedbackMessageScope so the two can no longer drift). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * docs(annotate): tell agents an approval may carry notes The skill and slash-command files still described `"decision": "approved"` as "acknowledge and stop", with no mention of the feedback field the gate can now attach — so an agent reading them would silently drop the reviewer's approval notes. Update the Claude core/claude skills, the Copilot commands, the Gemini annotate command, and the annotate command reference so the approved branch names the optional feedback field and says what to do with it: carry it into subsequent work, do not treat it as a change request. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS * docs(annotate): document the real approvedWithNotes default The default annotate.approvedWithNotes template is `{{contextBlock}}{{feedback}}`, not `{{context}}` on its own line, and {{contextBlock}} was missing from the variable table entirely. Show the actual default, add {{contextBlock}} to the variable table, and explain why the default prefers it: it collapses to nothing for message annotations instead of leaving a stray blank line. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Ben Newman
|
6b542da8b9 |
feat(annotate): extend per-file version diff to folder sessions (#1105)
* refactor(annotate): extract per-file version history into a shared helper Move the single-file annotate-history pipeline (slug derivation, saveToHistory, previous-version lookup, degrade-on-error) out of the Bun-specific annotate server and into packages/shared/annotate-history.ts, built on node:fs/node:path/node:crypto only so other runtimes can vendor it unmodified. annotate.ts now calls computeAnnotateHistory() instead of inlining the pipeline; behavior for single-file sessions is unchanged. * feat(annotate): extend per-file version history to folder annotate sessions Eligible folder files served through /api/doc now get snapshotted into the same version history the single-file flow uses, and their doc responses carry the same previousPlan/versionInfo/diffCurrent fields /api/plan already returns for single-file sessions. The pipeline runs lazily on first open and is memoized per resolved absolute path for the life of the server, so reopening a file never re-snapshots it. Eligibility mirrors the single-file source-save gates: a local file under the session's folder root, markdown-branch documents only (.md/.txt, not HTML, not a Turndown-converted doc), under the existing 2MB annotatable-file cap, and gated by the same annotateHistory config toggle. Storage failures degrade to a plain render (never a gate on the request) via the same try/catch computeAnnotateHistory already wraps. /api/plan/version and /api/plan/versions gain an optional path (+ base) query param so folder sessions can ask for a specific file's history; the slug is always derived server-side from the resolved, containment-checked path — never accepted from the client, since it gets joined unsanitized into a filesystem path. Omitting path keeps today's single-session-binding behavior unchanged. * test(annotate): cover folder annotate version history Adds a new describe block exercising the folder-mode history pipeline added in the previous commit: first-open snapshot + same-session memoization, storage-level dedupe, cross-mode slug continuity with the single-file flow, first-ever-open field shape, the config toggle, an ineligible (HTML) file type, degrade-on-unwritable-history-dir, and the path-parameterized version endpoints (including containment rejection and the no-path fallback). * feat(ui): add a docKey seam to usePlanDiff for per-document resets usePlanDiff's diff-base state (diffBasePlan, diffBaseVersion, versions, ...) was seeded once from its constructor args and only ever synced later via a "still falsy" guard - fine for a single root document, but switching to a different document (a different previousPlan/versionInfo) would silently keep the previous document's diff base around instead of adopting the new one's. Add an optional docKey param identifying which document the current previousPlan/versionInfo belong to. When it changes between renders, reset diffBasePlan/diffBaseVersion/versions (and in-flight loading/selecting flags) to the newly-provided values. Omitting docKey (or keeping it stable) preserves exactly today's one-time-hydration behavior, so the root document's call site is unaffected until it opts in. No caller passes docKey yet - this is purely additive. * feat(ui): carry a per-document version-diff baseline through useLinkedDoc /api/doc now returns previousPlan/versionInfo/diffCurrent for eligible folder files (same shape /api/plan already returns for single-file sessions). Extend LinkedDocLoadData with those fields and carry them through the same activate/cache/back lifecycle annotations and markdown already use, so a document's diff baseline: - is captured once when the document is first opened - persists in the per-filepath cache across back()/re-open, instead of being lost or needing a re-fetch - resolves cache-first via the new resolveDiffBaseline helper, gated on whether a baseline was ever captured (versionInfo presence) rather than truthiness of previousPlan - a document at its first-ever version legitimately caches previousPlan: null, which is a resolved fact, not a cache miss The hook exposes the active document's baseline as diffPreviousPlan/ diffVersionInfo, both null when no document is active or the active one has no eligible history (every non-folder linked doc, since /api/doc never populates these fields for those). Not yet consumed by App.tsx - purely additive. * feat(editor): render folder-doc version diffs via the active document Folder annotate's version-diff UI (inline PlanDiffViewer blocks, the +N/-M badge, and the Version Browser) was root-document-coupled: usePlanDiff was fed only the root's previousPlan/versionInfo, and every render site keyed off linkedDocHook.isActive to blank out the badge/version tab whenever any linked or folder document was open. Wire the two new per-document seams together instead: - Feed usePlanDiff the active document's own previousPlan/versionInfo/ filepath (falling back to the root document's when none is active), using the document's filepath as usePlanDiff's new docKey so switching documents resets the diff base instead of inheriting the previous one's. - Add per-document fetchers (fetchVersion/fetchVersions with &path=<filepath>) so selecting a base version or listing versions targets the active document's own history, not the session-bound bare endpoints. - Replace the root-only versionInfo/showVersionsTab reads with the active document's, so the Version Browser now reflects whichever document is on screen (previously it kept showing the root document's versions while a linked doc was open). - Drop the blanket "linkedDocHook.isActive ? null/false : ..." suppression at the Viewer callsite and in DocBadges - planDiffStats/hasPreviousVersion already resolve to the active document's own (possibly absent) diff data, so the badge now shows for folder docs with history and stays hidden for every other document exactly as it did before. Root-document behavior (single-file, plan, review, HTML surfaces) is unaffected: none of those ever set a docKey or have an eligible document history, so they fall through to the same defaults as before. * feat(pi): extend per-file version history to folder annotate sessions Mirrors the Bun runtime's folder annotate history support (packages/server/annotate.ts + reference-handlers.ts) in the Pi Node server: - Vendor the shared annotate-history helper (deriveAnnotateHistorySlug, computeAnnotateHistory) from packages/shared into generated/ via vendor.sh, and delegate the single-file version-history pipeline in serverAnnotate.ts to it instead of the hand-duplicated inline block. Behavior for single-file sessions is unchanged. - Eligible folder files served through /api/doc now get snapshotted into the same version history the single-file flow uses, and their doc responses carry the same previousPlan/versionInfo/diffCurrent fields /api/plan already returns. The pipeline runs lazily on first open and is memoized per resolved absolute path for the life of the server, so reopening a file never re-snapshots it. - /api/plan/version and /api/plan/versions gain an optional path (+ base) query param so folder sessions can ask for a specific file's history; the slug is always derived server-side from the resolved, containment-checked path (resolveAllowedDocPath in reference.ts) — never accepted from the client. * test(pi): cover folder annotate version history Adds apps/pi-extension/server/annotate-history.test.ts, the Node mirror of packages/server/annotate.test.ts's folder-history describe block: first-open snapshot + same-session memoization, cross-mode slug continuity with the single-file flow, the config toggle, an ineligible (HTML) file type, degrade-on-unwritable-history-dir, and the path-parameterized version endpoints (including containment rejection and the no-path fallback). History writes land in the real ~/.plannotator data dir rather than a per-test PLANNOTATOR_DATA_DIR override: generated/storage.js caches its data directory in a module-level constant at first import, so a per-test env var override taken after that point silently no-ops. Each test uses its own unique project namespace instead, same approach as the Bun-side suite. * ci: run docKey/linked-doc DOM tests in CI usePlanDiff.test.tsx and useLinkedDoc.test.tsx use the test.skipIf(!hasDom) pattern but were never added to the DOM_TESTS step, so they silently skipped under CI's plain `bun test` and never actually ran. * refactor(annotate): drop diffCurrent from the folder /api/doc path diffCurrent equals the document's own markdown and the client never reads it off /api/doc — it only exists on /api/plan for legacy single-file shape parity, which is untouched. Stop merging it into folder /api/doc responses and stop retaining it in the per-launch folder history memo (Bun and Pi), and drop the now-unused field from LinkedDocLoadData. - packages/server/reference-handlers.ts: new FolderAnnotateHistory type (AnnotateHistoryResult minus diffCurrent); applyDocOptions no longer copies diffCurrent onto the response - packages/server/annotate.ts: the folder memo now stores/returns only slug/previousPlan/versionInfo - apps/pi-extension/server/reference.ts + serverAnnotate.ts: mirrored changes for the Pi runtime - packages/ui/hooks/useLinkedDoc.ts: removed the unused diffCurrent field from LinkedDocLoadData * test(annotate): stop leaking history dirs; update diffCurrent expectations The folder annotate history tests (Bun and Pi) minted a fresh project namespace per test but never cleaned up, leaving hundreds of directories under the real ~/.plannotator/history over repeated runs. Track every minted project and remove its history directory in afterAll — this also covers the stray non-directory artifact the "unwritable data dir" test deliberately plants inside its own project's history dir, since removing the project dir recursively takes it with it. Also update the two assertions that expected diffCurrent on the folder /api/doc response: that field is no longer propagated on the folder path (see the preceding diffCurrent-removal commit), so both now assert its absence instead. * fix(ui): remember per-document diff-base selection across navigation usePlanDiff reset diffBasePlan/diffBaseVersion to the newly-provided document's defaults on every docKey change. That discarded a manually selected base version when navigating away from a document and back (e.g. root -> linked doc -> root), regressing behavior upstream relied on keeping (nothing reset the selection before this seam existed). Track each docKey's selection in a ref-held Map (keyed by docKey, including null for the root document) and restore it on return instead of re-seeding defaults; a key visited for the first time still seeds from its own initialPreviousPlan/versionInfo exactly as before, and selections never leak between distinct keys. Adds two DOM-gated tests: restoring a manual selection after a detour to another document, and confirming distinct docKeys don't leak into each other. * fix(annotate): match folder history eligibility to the single-file plain-text set The folder /api/doc history gate was a hardcoded /\.(md|txt)$/i in both runtimes, so any other annotatable plain-text file (.mdx, .yaml, .json, .toml, ...) opened via a folder session silently skipped snapshotting — breaking the cross-mode continuity this feature advertises (a .yaml with an existing single-file version thread showed no diff when opened via its folder). Reuse the canonical predicate instead: isAnnotatableTextPath (ANNOTATABLE_TEXT_REGEX in @plannotator/core/annotatable), the exact set the single-file pipeline snapshots. HTML stays deferred and .env stays excluded, both by that same definition. Tests extended in both runtimes: .mdx mints on first open, .yaml single-file history serves as the folder baseline, .env mints nothing, .html unchanged. * feat(ui): label the folder diff badge with its baseline The in-file version-diff badge in annotate/folder sessions shows +N/-M against the file's last-reviewed snapshot, while the git badges in the file tree count uncommitted-vs-HEAD — same numbers, different baselines. Give the badge an optional baseline suffix and tooltip override (PlanDiffBadge baselineLabel/baselineTooltip, threaded through DocBadges, Viewer, and StickyHeaderLane) and have annotate mode pass 'since last review' / 'Changes since you last reviewed this file'. Plan review passes nothing and renders byte-identically to before. DOM tests cover both the labeled and the unchanged default rendering. * fix(editor): exit diff view when the active document loses its baseline Follow-up to the per-document diff baselines: with diff view active on file A, opening a history-less file B left isPlanDiffActive latched on — the diff viewer could not render for B, but the stale flag hid the annotation toolstrip and sticky header until the user pressed Escape. Auto-exit the diff view whenever the active (non-HTML) document has no baseline. The --render-html surface is explicitly gated out: its diff view is driven by htmlDiffHtml with usePlanDiff fed nulls, so hasPreviousVersion is always false there and auto-exiting would kill the HTML diff toggle. Plan review is unaffected — the root document's baseline never goes false mid-session. DOM tests cover the exit, the keep-active document switch, the HTML gate, and the no-baseline activation snap-back. --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Kevin
|
0eda139cbe |
Add an option to disable AI features (#1129)
* feat: allow disabling Plannotator AI * fix: enforce disabled AI review endpoints * fix: fully disable AI review surfaces --------- Co-authored-by: Kevin <kcosrdev@gmail.com> |
||
|
Kevin
|
193b07e22c |
fix(review): bound memory for large untracked files (#1118)
Co-authored-by: Kevin <kcosrdev@gmail.com> |
||
|
Ben Newman
|
3455d8285f |
fix(review): treat cross-drive relative() results as repo escapes (#1117)
- path.relative returns the target's absolute path when base and target are on different Windows drives; normalizeAgentPath's escape guard only checked ".." and "/" prefixes, so such results slipped through and were glued onto the repo label (e.g. "api/L:/checkout/src/file.ts") - extract the guard into isRepoRelative and reject drive-letter-prefixed results at all three call sites - fixes the annotation-path assertions in review-workspace.test.ts, which failed on Windows whenever the checkout and %TEMP% live on different drives; add a platform-independent unit test for the new guard |
||
|
Michael Ramos
|
99d11dca04 |
Persist Guided Reviews across sessions (#1115)
* feat(guide): add durable guide store with repo-scoped keys and opt-out Runtime-agnostic guide persistence for #1112: packages/shared/guide-store.ts writes validated guides to ${PLANNOTATOR_DATA_DIR}/guides/{repo-key}/{id}.json with atomic tmp+rename writes and graceful corrupt-file handling. The repo key is a sanitized host__owner__repo from the origin remote (or the PR url), with a dir-name+hash8 fallback when no remote parses, so PR and branch sessions of one repository share a shelf and same-named branches in different repos never collide. Includes the session glue (repo-key/headSha/label resolution plus the jobId-to-savedId map) shared by both server runtimes, the guideHistory config key with resolveGuideHistory (PLANNOTATOR_GUIDE_HISTORY, coerced booleans), and the browser-safe SavedGuideListEntry/CodeGuideData extensions. * feat(guide): autosave guides and serve saved: ids in both server runtimes Both packages/server/review.ts and the Pi mirror serverReview.ts now: autosave a guide the moment it passes the existing validateGuideOutput gate (including manual-repair submits); write reviewed-state changes on a live job id through to that job's saved file; serve persisted guides through the existing guide endpoints as saved:{id} pseudo job ids (GET guide + PUT reviewed); and expose GET /api/guides (repo-scoped list with progress and a moved flag comparing the stored head sha to the current head) and DELETE /api/guides/:id. guide-store joins vendor.sh's flat copy list; cross-runtime endpoint wiring is covered by packages/server/guide-persistence.test.ts against both servers, including reviewed-state persistence across a server restart and traversal-id rejection. * feat(guide): previous-guides list, Saved chip, and outdated Regenerate hint GuideEmptyState grows a Previous guides section under the Generate controls: rows show the target label chip, title, age, reviewed progress, a quiet diff changed flag when the stored head no longer matches, and a per-row delete; clicking a row loads the guide via its saved:{id} pseudo job id (the existing useGuideData/GuideScreen id plumbing already treats ids as opaque). GuideView shows a small Saved chip once the active guide is persisted and, for an outdated saved guide, one muted hint line whose Regenerate action launches a fresh guide with the persisted defaults. The engine/model resolution and launch-param shapes move into the shared useGuideLaunch hook so the empty state and the hint stay in lockstep. DOM tests cover the new GuideView states. * docs: document guide persistence endpoints and PLANNOTATOR_GUIDE_HISTORY * fix(guide): label saved envelopes with launch-time context, not completion-time state Review finding on #1115: saveForJob read the live session getters when the job COMPLETED, but guide jobs run for minutes while the session supports mid-generation PR switching (/api/pr-switch) and diff switches. Launch on PR A, switch to B, complete: the envelope permanently carried A's content labeled with B's PR label/url/headSha (and could even land on B's repo shelf). The review-target context (pr url/label/head, branch label, head sha) is now snapshotted at job LAUNCH via guideStore.captureLaunchContext() in the guide buildCommand branch of both runtimes and carried on the job itself as AgentJobInfo.guideContext, the same discipline as changedFilesSnapshot, so it is garbage-collected with the job and needs no separate cleanup. saveForJob prefers the snapshot (falling back to the live getters only for jobs launched without one), derives the shelf from the launch-time PR url, and records the shelf alongside the saved id so reviewed write-through follows the file wherever it landed. Repair jobs reuse the FAILED job's own snapshot. Covered by new session tests that mutate the injected getters between launch capture and completion. * fix(guide): close a saved guide when the review context switches Review finding on #1115: a saved:{id} guide has no AgentJobInfo, so GuideScreen's context match passes trivially (unknown ids are tolerated for the demo path). Switching PRs or worktrees while a saved guide was open left it mounted over the new context's diff with a stale moved flag. App.tsx now clears activeGuideJobId on any prMetadata.url / activeWorktreePath change when it points at a saved: id; the user reopens it from the Previous guides list. Live job ids are untouched, GuideScreen's own matching handles those. |
||
|
Michael Ramos
|
3f3b3514a0 |
fix(annotate): use annotate feedback template for clipboard copy, not plan-deny (#1109)
Clipboard Copy paths (ExportModal Copy and the annotation panel quick copy) unconditionally wrapped exported annotations with the plan-deny template, so annotate sessions copied text starting with "YOUR PLAN WAS NOT APPROVED." while Send Feedback used the annotate template (including custom prompts.annotate.* config). The annotate servers (Bun and Pi) now ship the resolved, unsubstituted copy-wrapper templates in the /api/plan payload (feedbackTemplates), and the plan editor wraps copied annotations mode-aware: server template when present, browser-safe default annotate template otherwise, plan-deny only in actual plan review. Send Feedback behavior is unchanged. Closes #1107 |
||
|
Michael Ramos
|
5ef89a0a2e |
fix(config): coerce quoted boolean strings in config.json resolvers (#1102)
~/.plannotator/config.json is hand-edited, so boolean settings often
arrive as quoted strings ("cursorSandbox": "false"). The resolvers
passed the raw JSON value through, so a quoted "false" reached strict
=== false checks downstream as a string and silently kept the default
behavior. Concretely, the Cursor sandbox opt-out added for NixOS users
in #1095 did nothing when set via config.json with quotes, even though
the equivalent env var accepts the string form.
Add a coerceConfigBoolean helper and apply it to the config-file branch
of resolveUseGlimpse, resolveAnnotateHistory, resolveUseJina, and
resolveCursorSandbox. Real booleans pass through, "true"/"false"/
"1"/"0" strings (any case) coerce, and anything else falls back to
the default. Env-var branches and their precedence are unchanged.
resolveSharingEnabled is left alone: it is keyed on the string sentinel
"disabled" and already fails safe.
Pi picks the fix up automatically via apps/pi-extension/vendor.sh.
|
||
|
Michael Ramos
|
f9a6c1e39d |
feat: annotate accepts YAML, JSON, TOML and other plain-text files (#1099)
* feat(annotate): accept common plain-text config formats (.yaml, .json, .toml, …) Annotate previously rejected every file that wasn't .md/.mdx/.txt (or .html/.htm), even though the pipeline reads files as UTF-8 text and renders anything. Widen the accepted set to unambiguously plain-text config/data formats: .yaml .yml .json .jsonc .json5 .toml .ini .cfg .conf .properties .csv .tsv .log .xml .env.example. They render exactly the way .txt renders today. - New single source of truth: packages/core/annotatable.ts (ANNOTATABLE_TEXT_REGEX / ANNOTATABLE_DOC_REGEX + predicates), re-exported through @plannotator/shared/resolve-file and vendored into the Pi extension. - .env stays excluded (commonly holds secrets; annotate history copies file contents into the data dir). Source-code extensions stay with code review. - Single-file accept + bare-filename fuzzy search widen in resolveMarkdownFile; folder discovery and the file-browser listing widen in all three runtimes (hook CLI, OpenCode, Pi). - /api/doc gains a `doc=1` param set by the file browser so extensions that overlap CODE_FILE_REGEX (.yaml/.json/.toml/.ini/.xml) render as annotatable documents there while code-file links inside documents keep the syntax-highlighted popout. - Error messages now list the wider set; docs updated (AGENTS.md, marketing annotate page). Closes #1029 Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk * fix(annotate): frontmatter, size caps, edit-guard, and skill docs from review Review fixes for #1099: - Frontmatter: `--- … ---` stripping is a markdown convention; for non-markdown annotatable sources (multi-document YAML, .txt starting with ---) the delimiters are real content. parseMarkdownToBlocks gains a { frontmatter } option and the editor keys it off the active document's path via shouldStripFrontmatter() (strip for .md/.mdx and pathless/converted sources; keep raw for other annotatable text). - Size caps: new shared MAX_ANNOTATABLE_FILE_BYTES (2MB — same limit the code-file popout always had) now guards the annotate CLI single-file read in all three runtimes and the /api/doc document branches in both servers. Also applies to .md/.txt (behavior change for pathological inputs; previously unbounded). - Editing guard: mid-edit file opens gate on isSourceSaveFilePath (.md/.mdx/.txt) instead of the wider annotatable set — config files are view-only, so switching to one mid-edit no longer silently downgrades "Done editing" to feedback-only edits. - Skill docs: plannotator-annotate SKILL.md (core + Kiro) now mention the plain-text config formats. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk |
||
|
Michael Ramos
|
9ddb87abb4 |
feat(data-dir): fall back to $XDG_DATA_HOME/plannotator when ~/.plannotator does not exist (#1093)
New resolution order for the Plannotator data directory:
1. PLANNOTATOR_DATA_DIR (unchanged, top priority, ~ expansion)
2. ~/.plannotator when it already exists (legacy default — existing
installs never move)
3. $XDG_DATA_HOME/plannotator when XDG_DATA_HOME is set to a
non-empty absolute path
4. ~/.plannotator (default for everyone else)
This is git's legacy-first pattern: the XDG branch only fires for fresh
installs whose user has explicitly set XDG_DATA_HOME. The XDG spec's
implicit ~/.local/share default is deliberately NOT applied, and the
directory stays monolithic (no config/data/cache split).
Mirrored in every private copy of the resolver: the Amp plugin,
scripts/install.sh, scripts/install.ps1, and scripts/install.cmd. The
Pi runtime picks the change up automatically via vendor.sh. Docs updated
in README.md, AGENTS.md/CLAUDE.md, and the marketing env-var reference.
Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk
|
||
|
Michael Ramos
|
c645999761 |
fix(review): allow disabling Cursor sandbox via PLANNOTATOR_CURSOR_SANDBOX for systems where it cannot start (#1095)
The Cursor review engine hardcodes `--sandbox enabled` when launching the
`agent` CLI. On systems where Cursor's sandbox cannot start (NixOS,
AppArmor-restricted Linux) that hard-fails every job with "Sandbox mode is
enabled but not available on this system", and the flag overrides the
user's own `agent sandbox disable` configuration.
Default is unchanged: review jobs still pass `--sandbox enabled` as part
of their read-only posture. Setting PLANNOTATOR_CURSOR_SANDBOX=0 (or
`{ "cursorSandbox": false }` in ~/.plannotator/config.json; the env var
wins) omits the flag pair entirely — never `--sandbox disabled` — so the
user's own Cursor Agent sandbox configuration governs. Resolution follows
the established env-plus-config pattern via resolveCursorSandbox() in
packages/shared/config.ts, applied at the buildMarkerCommand call sites in
both the Bun server (review.ts, guide-review.ts) and the Pi server
(serverReview.ts; guide-review is vendored, with the new ../config import
rewritten in vendor.sh).
Closes #1094
Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk
|
||
|
Michael Ramos
|
8a6288b9fe |
fix(annotate): resolve explicit ../ paths that escape the project root
plannotator annotate ../docs/plan.md failed with a misleading 'File type not supported: .md' because resolveMarkdownFile rejected any relative path resolving outside the project root, even when the file existed and was a supported type. The CLI then found the file on disk and mislabeled the resolver miss as a type error. An explicit path the user types (one containing a separator, including ../) is now honored when it exists, matching the trust already given to absolute paths. Bare filenames stay restricted to the in-root fuzzy search, so a stray notes.md cannot resolve into a parent directory. Closes #1085. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk |
||
|
Michael Ramos
|
e27771eb71 |
fix: bound workspace repo discovery so symlink escapes can't stall startup
Workspace discovery follows symlinks since #1060, which means the walk can leave the workspace root by design. A link into a huge unrelated tree was enumerated synchronously before the server bound its port, hanging startup with no output. The walk now shares the PLANNOTATOR_FILE_BROWSER_MAX_FILES budget: it visits at most that many directories and returns what it found. Symlinked repos are still discovered under the default budget. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk |
||
|
Michael Ramos
|
fa294b12e4 |
feat(review): add PR and MR artifact gallery (#1055)
Adds a first-class artifact gallery and annotation workflow for GitHub pull requests and GitLab merge requests, including images, GIFs, video timestamps, rendered HTML, Markdown, provenance-aware feedback, authenticated provider resources, and reliable CDN/media handling. |
||
|
Michael Ramos
|
56df64c751 |
Add modern GitButler review support (#1067)
Adds current-architecture GitButler workspace, stack, and branch review support across Bun and Pi while preserving the existing Git, JJ, and P4 paths. Co-authored-by: Dan Susman <56033661+dansusman@users.noreply.github.com> |
||
|
Michael Ramos
|
467c11b29f | fix: discover symlinked workspace repositories (#1060) | ||
|
Michael Ramos
|
d0665571c7 | Fix OpenCode plan review cancellation cleanup (#1064) | ||
|
Michael Ramos
|
8e9a359a8f | fix(review): make remote discovery noninteractive (#1062) | ||
|
Michael Ramos
|
60b5e8d31a | Narrow review feedback validation to submitted findings (#1065) | ||
|
iury souza
|
13309e322b |
feat(server): support bounded port ranges (#1042)
* feat(server): support bounded port ranges * fix(server): harden bounded port retries * fix(server): preserve non-range port behavior --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Michael Ramos
|
7f0c36028b |
fix(server): bound startup file discovery (#1036)
* fix(server): bound startup file discovery (#978) * test: observe the server's own warm cache key so the ordering tests actually pin the fix Review finding: both bind-before-warm ordering tests raced against observeWarmState(projectRoot), but on macOS mkdtempSync returns /var/... while the chdir'd server warms under the realpath /private/var/... — a different warmFileListCache key. The tests therefore raced a FRESH warm (always pending at observation time) and passed on the OLD broken code too. Observing process.cwd() inside onReady uses the server's real key. Verified: with old resolve-file/server code checked out, all four ordering tests now fail; on the fix they pass (17/17 across both files). Claude-Session: https://claude.ai/code/session_01SFy9fY27SA8g5BtotWPi1G |
||
|
Michael Ramos
|
1047539801 |
Render arbitrary HTML in HtmlViewer without altering it (#1023)
HtmlViewer injected host-app state into rendered documents: ~26 bare theme tokens (--muted, --background, ...) written into :root and re-applied inline on the author's documentElement on every host theme flip, a `light` class toggled on the author's root, an asymmetric color-scheme:light injection, and unconditional ins/del diff styles. Documents defining the same token names rendered with wrong colors in both Plannotator and @plannotator/ui consumers. Arbitrary documents now render exactly as in a standalone tab: - Host tokens are pushed only under the viewer-owned --pn-* prefix; annotation CSS and the bridge read only var(--pn-*, fallback). The bridge refuses non---pn- writes and never touches the root class list unless the document opts in. - Host theme-following is opt-in per document via <meta name="plannotator-theme" content="host">, which restores the bare-token push, the light class, and a symmetric color-scheme sync. The visual-explainer skill now emits the tag in generated artifacts. - Diff CSS is injected only while the version-diff view is active and scoped to ins/del.plannotator-diff, which htmlDiff now emits on its generated wrappers; author <ins>/<del> markup is never restyled. The srcdoc injection logic moves to a pure module (srcdoc.ts) with tests pinning the neutrality contract, including a DOM test that runs the actual bridge script and asserts a theme flip lands nothing on the author's root except --pn-* properties. Claude-Session: https://claude.ai/code/session_01MDqD8jdgTbdjiXcVVigzV2 |