mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
main
189 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Ramos
|
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 |
||
|
Michael Ramos
|
d3c9de1ef0 |
Fix annotate terminal startup in large folders
Avoid starving folder annotate sessions by excluding agent work directories and capping folder file-browser scans. Improve the annotate WebTUI panel startup/stop behavior. |
||
|
Michael Ramos
|
070d9a5f6d |
Make the document UI reusable as published building blocks (#957)
* docs(adr): revert failed document-ui cutover, add ADR 004 with corrected reuse plan The document-ui extraction/cutover (ADRs 002/003) was an AI-driven rewrite that broke the app; the code was reverted. Add ADR 004 as the source of truth: share @plannotator/ui as published building blocks for the Workspaces app, keep Plannotator's app unchanged, gate on human-verified parity. Banner the reverted ADRs and point AGENTS.md/CLAUDE.md at 004 so future agents don't rebuild the mess. * docs(adr): add verified document-ui extraction plan, supersede draft inventory 36-agent verification of the reuse inventory: confirmed the /api coupling but found the draft missed Viewer's transitive backend call, the cookie settings layer, 3 React contexts + identity singleton, SSE transports, and harder packaging blockers. Adds the verified per-subsystem extraction plan with a parity guardrail on every step; flags the draft inventory as superseded. * docs(adr): add document-ui extraction roadmap + parity checklist Phase 0-7 execution roadmap (safety net -> packaging -> foundation seams -> rendering -> navigation -> comments -> extras -> publish) and the reusable 'did it break?' parity checklist run after every step. Both enforce the law: move + decouple, never rewrite; Plannotator's experience cannot change. * build(ui): packaging unblock for external install (Phase 1) — no runtime change Phase 0: captured parity baseline (typecheck/test/build + shipped-bundle hashes). Phase 1 packaging fixes to packages/ui, metadata only: - add phantom dompurify ^3.3.3 dep (imported in sanitizeHtml/aiChatFormat, was undeclared) - align diff ^8.0.3 -> ^8.0.4 with root - add peerDependencies (react, react-dom, tailwindcss, tailwindcss-animate); keep as devDeps - add files allowlist (excludes tests); remove dead tsconfig @plannotator/shared alias Verified byte-identical: typecheck pass, 1620 tests pass/0 fail, all 3 builds OK, shipped plan+review bundle hashes unchanged from baseline. Remaining Phase 1 blocker (@plannotator/ai + @plannotator/shared workspace:* deps) deferred pending a publish-vs-inline decision; logged in worklog. * feat(ui): make image URL resolution host-overridable (Phase 2, seam 1) getImageSrc now delegates to a module-level resolver defaulting to the verbatim Plannotator /api/image logic; add setImageSrcResolver/resetImageSrcResolver so a host (Workspaces) can resolve images via its own backend. All 5 consumers and the signature unchanged. Verified: default URLs byte-identical, typecheck pass, 1620 tests pass/0 fail, builds OK. No Plannotator behavior change. * feat(ui): make settings storage backend host-overridable (Phase 2, seam 2) storage.ts cookie impl is now the default 'cookieBackend'; add setStorageBackend/ resetStorageBackend so a host (Workspaces) can persist settings via its own storage. getItem/setItem/removeItem delegate to the active backend; the ~24 consumers and literal plannotator-* keys are unchanged. Verified: swap works, typecheck pass, 1620 tests pass/0 fail, builds OK, theme persists across reload. * feat(ui): make MarkdownEditor theme mode host-supplyable (Phase 3) Add optional mode? prop; mode now mode ?? resolvedMode. Plannotator passes no mode (App.tsx:4261) so it keeps using ThemeProvider's resolvedMode unchanged. A host without ThemeProvider can supply mode directly. Verified: typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. * feat(ui): allow hosts to opt out of code-path validation (Phase 3) Viewer gains optional disableCodePathValidation? threaded to a new disabled? arg on useValidatedCodePaths; when set, the /api/doc/exists probe is skipped. Default undefined for Plannotator => validation stays on, /api/doc/exists fires exactly as today. Verified: typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. Also logs Phase 3 workflow outcome + remaining scroll/docfetch pieces. * feat(ui): make code-file hover preview fetch host-overridable (Phase 3) Add DocPreviewFetcher seam (default = verbatim /api/doc fetch) + setDocPreviewFetcher/resetDocPreviewFetcher; route handleMouseEnter through it, useCallback deps unchanged. No caller overrides it => Plannotator fetches /api/doc identically. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): ship ScrollViewportProvider with the library (Phase 3 scroll) Add render-transparent ScrollViewportProvider (createElement, keeps .ts) so the scroll-viewport context travels with @plannotator/ui instead of living only in App.tsx. Rewire App.tsx provider tags (3-line delta); identical tree/value/ position, sidebar TOC still reads the MAIN viewport. Fix stale OverlayScrollbars doc-comment. typecheck pass, 1620 tests/0 fail, builds OK, eyeball: TOC tracks. * fix(ui): disabled code-path validation should keep links clickable (self-review) The Phase-3 disabled branch set ready=true with an empty map, which makes gateCodePath demote every code link to plain text. Leave ready=false so the no-validation fallback renders links optimistically. No Plannotator impact (never disables). Logs Phase 3 completion + reusability note. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make file-tree backend host-overridable (Phase 4) Lift useFileBrowser's three backend wires (load-dir fetch, obsidian-vault fetch, and the SSE live-watch effect moved VERBATIM) into an injectable FileTreeBackend with default + setFileTreeBackend/resetFileTreeBackend, same pattern as the image /storage seams. useFileBrowser() stays zero-arg; default fetch/SSE URLs identical. Sidebar confirmed noop (zero backend wires, already reused by review-editor). Verified: useFileBrowser.test.tsx passes 6/0 UNMODIFIED (DOM_TESTS=1), typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched, manual eyeball (annotate adr/: tree loads, file-switch works, new file appears live via SSE). Plannotator byte-unchanged. Logs two pre-existing bugs found during testing (not regressions). * docs(adr): research + synthesis + spec for Phase 5 (comments/annotations/drafts) Five-probe code research of the comment system. Key finding: most comment UI is already portable (panel/popover/toolbar/highlighter prop-driven; review-editor already reuses the hooks). Phase 5 narrows to 3 seams — draft transport (+ the 3-party generation protocol), external-annotation transport (SSE->polling, move verbatim), and identity/authorship — plus 2 non-extraction items: renderer coupling (document as a contract) and replies/threading (defer as a new feature). * docs(adr): accept ADR 005 — make comments/annotations/drafts host-overridable (Phase 5) Three seams (identity, draft transport, external-annotation transport), each defaulting to today's behavior; renderer coupling documented as a contract; replies/threading deferred as a new feature. Locks in the recommended choices from the Phase 5 spec/synthesis. * feat(ui): make annotation identity host-overridable (Phase 5 seam 1) Add IdentityProvider + setIdentityProvider/resetIdentityProvider in identity.ts; getIdentity/isCurrentUser now delegate to a module-level provider defaulting to today's ConfigStore tater behavior. The ~9 author-stamp sites and 2 (me)-badge sites delegate with zero call-site edits. No caller overrides => Plannotator byte-unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make draft persistence transport host-overridable (Phase 5 seam 2) Add DraftTransport (load/save/remove) + getDraftTransport/setDraftTransport/ resetDraftTransport in useAnnotationDraft.ts, default = today's /api/draft fetches verbatim. useCodeAnnotationDraft reads getDraftTransport() live. The generation pre-increment, 500ms debounce, keepalive retry-gate, and pagehide/visibilitychange flush stay in the hooks; getDraftGeneration() still escapes to the host. save rejects-on-failure so the gated retry is preserved. No caller overrides => Plannotator byte-unchanged. shared/draft.test.ts 10/0, annotationDraftPersistence 13/0, typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make external-annotation transport host-overridable (Phase 5 seam 3) Add ExternalAnnotationTransport<T> (subscribe/getSnapshot/CRUD) + setters in useExternalAnnotations.ts; default = today's SSE->polling wire moved verbatim into createDefaultTransport. The reducer (applyEvent), fallback-once gate, 500ms poll, versionRef scoping, optimistic-before-await, and [enabled] gate stay in the hook. A host (Workspaces) can implement the same event contract over Durable Objects. No override caller => Plannotator byte-unchanged. external-annotations test green, typecheck pass, 1620 tests/0 fail, builds OK. Logs Phase 5 completion. * docs(adr): research + synthesis + spec for Phase 6 (versions, settings, sharing, AI) Five-probe code research. Most of the four subsystems is already portable; the real work is 5 seams (version fetchers + vscode-diff, config write-back, obsidian detect, save-to-notes, AI transport) + 1 CSS move (block/raw diff classes from the app shell into the package's theme.css). Fragile do-not-touch: the AI SSE reader loop + epoch guards, and configStore debounce/deepMerge. Five Plannotator-only pieces (OpenInApp, HooksTab, useUpdateCheck, useAgents/useAgentJobs) stay home. * docs(adr): accept ADR 006 — make extras (versions/settings/sharing/AI) host-overridable (Phase 6) Five seams + one CSS move, each defaulting to today's behavior. AI reader loop + epoch guards and configStore debounce/deepMerge stay verbatim. Five Plannotator- only pieces stay home. Locks the recommended choices from the Phase 6 spec. * feat(ui): make version fetchers + vscode-diff host-overridable; move diff CSS into package (Phase 6 versions) usePlanDiff gains optional fetchers (default /api/plan/version(s), error asymmetry kept: selectBaseVersion alerts, fetchVersions silent). PlanDiffViewer gains optional onOpenVscodeDiff (default /api/plan/vscode-diff). Relocate .annotation-highlight* + .plan-diff-* block/raw CSS from editor/index.css into ui/theme.css (next to .plan-diff-word-*) so the diff/highlights are self-styling from the package. Verified: relocated CSS gone from index.css, present in shipped bundle (33x), diff renders identical; typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. * feat(ui): make config write-back + obsidian-detect host-overridable (Phase 6 settings) configStore.setServerSync(fn) injects only the terminal POST /api/config; the 300ms debounce, deepMerge batching, singleton, and eager cookie reads stay verbatim. Settings gains optional onDetectObsidianVaults (default /api/obsidian/vaults), with the [obsidian.enabled] effect dep + auto-select-first-vault verbatim. No override caller => Plannotator unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make save-to-notes host-overridable (Phase 6 sharing) ExportModal gains optional onSaveToNotes (default = verbatim POST /api/save-notes); showNotesTab = isApiMode && !!markdown kept byte-for-byte. Sharing utils already parameterized (noop). No override caller => Plannotator unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make Ask AI transport host-overridable (Phase 6 ai) useAIChat gains a module-level AITransport (session/query/abort/permission) + setAITransport/resetAITransport, default = the five /api/ai/* fetches verbatim. The SSE reader loop, epoch/createRequest guards, and the supersede-abort position inside createSession stay untouched. Capabilities + provider-resolution stay host-owned in App.tsx. No override caller => Plannotator unchanged. ai.test.ts 97/0, typecheck pass, 1620 tests/0 fail, builds OK. * docs(adr): log Phase 6 completion (4 seams + diff CSS move) * docs(adr): research + synthesis + spec for Phase 7 (carve @plannotator/core + publish) Carve a browser-safe @plannotator/core: move the ~15 pure shared modules in, extract types from the 3-4 node-bound ones (config/storage/workspace-status) so nothing duplicates, shim @plannotator/shared so Plannotator's 99 import sites stay unchanged, re-point @plannotator/ui to depend only on core, move wideMode.ts, then publish core+ui (source-only). shared + ai stay private. Open: registry, versions, CI job. Publish is the one outward-facing step — confirm before pushing. * docs(adr): fold configurePlannotatorUI() front door + precompiled CSS into Phase 7 spec Add the single typed configure() facade over the 9 global host-override setters (zero-risk, additive) and an optional precompiled CSS bundle (smooths the Tailwind-in-shared-lib wrinkle) to the Phase 7 publish scope. Both make the published surface nicer to consume; neither touches Plannotator. * docs(adr): lock Phase 7 publish decisions + carry over review fixes Decided: ship JS as source (single internal consumer on controlled stack, no build to maintain, no dist drift); precompiled CSS now REQUIRED (the @source glob is fragile under pnpm symlinks); core CI typecheck node-free; pin ui->core exact. Recorded the interrogation's carried-over Phase-5 code fixes (useExternalAnnotations split-transport + fallbackRef reset, per-seam override tests, configStore loadFromBackend) to do before publish. * docs(adr): ADR 007 — carve @plannotator/core, complete settings provider, publish Locks Phase 7 decisions: public npm; lockstep version at repo 0.21.0 (ui->core pinned exact); JS ships as source + required precompiled CSS; core CI node-free; ai stays unpublished-to-npm. Settings provider completed (loadFromBackend, prefetch +sync) is now IN SCOPE — Workspaces uses the same UI settings stored in its own backend. CI publish job wired but artifacts validated on-branch (pack + dry-run) before merge; first publish gated. Carries the 2 override-path bug fixes + per-seam override tests as pre-publish work. * fix(ui): make external-annotation transport reads consistent + reset fallback on re-enable Two override-path bugs found by the interrogation pass (both unreachable on Plannotator's path; harden the host-override path for a real consumer): 1. Split-transport: the effect captured the transport at mount for subscribe/poll while the CRUD callbacks read the module global live, so a host swapping the transport after mount would split reads and writes across two backends. Capture once in a ref and use it in all four spots. 2. fallbackRef/receivedSnapshotRef were not reset on effect re-run, so an enabled false->true toggle inherited a stale 'already fell back' flag and silently stopped updating. Reset both at the top of the effect. Plannotator unchanged: it never overrides the transport (same default singleton captured) and enabled never toggles (reset is a no-op). typecheck clean; full test suite shows zero delta (1605 pass / 45 pre-existing env failures, identical with and without this change). * docs(adr): align Phase 7 spec with ADR 007 (version 0.21.0 lockstep, CSS required, scope completeness) * feat(core): carve @plannotator/core — move pure modules, extract node-bound types, shim shared (Phase 7 step 1) * feat(ui): depend only on @plannotator/core — re-point all shared/ai imports (Phase 7 step 2) * refactor(ui): relocate wideMode helper to @plannotator/ui/utils (Phase 7 step 3) * feat(ui): add loadFromBackend settings rehydration + configurePlannotatorUI front door (Phase 7 step 4) * build(ui): precompiled styles.css CSS build + madge circular-dep check (Phase 7 step 5) * test(ui): per-seam override tests + configure routing test (Phase 7 step 6) Add one override test per seam (setX(fake)→drive→assert→resetX()) for all 9 seams + loadFromBackend, modeled after the existing seam test pattern. Fix configure.test.ts to defer mock.module() into beforeAll and restore with captured real function references in afterAll so sibling seam test files are not poisoned by spy replacements in the shared Bun worker module registry. * fix(ui): apply Phase 7 review findings — version lockstep + seam consistency - Bump @plannotator/ui to 0.21.0 (lockstep with @plannotator/core + repo, per ADR 007) [was the 1 critical review finding] - useAnnotationDraft: route persistNow/dismissDraft save+remove through getDraftTransport() so all paths read the transport consistently (matches the load path; makes the single-global invariant explicit) - configStore.loadFromBackend: document it must be called BEFORE init() or server values get overwritten - packages/core/tsconfig: add explicit types:[] so the node-free invariant is first-class (verified: planted node:fs still fails TS2882) * docs(adr): Phase 7 implementation plan (workflow-generated, durable artifact) * fix(ui): reconcile #948 with the draft-transport seam + lockstep 0.21.1 Rebased onto origin/main (picks up #948 draft-deletion fix, the 0.21.1 bump, and the #949/#950 editor fix). The rebase auto-merged #948's code-draft logic (hasHadAnnotationsRef, empty-state tombstone, clearTimeout in restore/dismiss) with the Phase-5 transport refactor cleanly — except the empty-state tombstone delete was left as a raw fetch('/api/draft', DELETE). Route it through getDraftTransport().remove() so a host backend tombstones its own stored draft on clear (the #948 guarantee, for hosts). Plannotator unchanged (default transport hits the same endpoint). Bump @plannotator/core + @plannotator/ui 0.21.0 -> 0.21.1 to match main's version (lockstep per ADR 007). Verified: typecheck clean, madge no-cycles, plain suite 1637 pass / 0 fail, #948 draft-clear test 3/0. (The 45 DOM_TESTS failures are the known server/network integration tests that need a real OS env — same set on main, not regressions.) * fix(ui): address review nits — host-path robustness + cleanups - PlanDiffViewer: wrap onOpenVscodeDiff in try/finally so a host opener that throws can't wedge the VS Code button in a permanent loading state (default unaffected) - useExternalAnnotations: (re-)capture the transport inside the effect on enable so a host that installs a transport before enabling annotations is honored, not the stale default — keeps the split-transport fix (effect + CRUD share one ref) - configure.ts: import ServerSyncFn from configStore instead of duplicating the type - repoint the 2 remaining @plannotator/shared test imports to @plannotator/core - AGENTS.md/CLAUDE.md: document the new packages/core package All host-path only — Plannotator behavior unchanged. typecheck clean, no cycles, full suite green. Skipped (not simple/over-engineering): usePlanDiff prop->module-level (design change), Obsidian late-bind, getSnapshot guard (inert), transport <any> (variance). * docs: collapse 29 ADR process docs into one packages/ui/README.md The branch had accumulated ~6,200 lines of ADR scaffolding (6 decisions, 7 specs, 10 research spikes/synthesis, 6 worklogs/roadmaps/plans) for this one effort. Replace all of it with a single concise README that ships with the published package: what @plannotator/ui + @plannotator/core are, why they exist (commercial reuse), how the host-override seams work (configurePlannotatorUI), how a consumer installs/builds, and the one rule (don't reimplement from scratch — add a seam). Repoint the CLAUDE.md banner at the README. No code references the deleted docs; main's pre-existing adr/ docs untouched. * docs(ui): add packages/ui/AGENTS.md guardrail + CLAUDE.md symlink Directory-scoped agent guidance for anyone editing @plannotator/ui: don't rewrite from scratch, add a seam (default = today's behavior, Plannotator byte-for-byte unchanged), core stays node-free, never delete working code until human parity. Points to README.md for the architecture. CLAUDE.md -> AGENTS.md symlink mirrors the repo root convention. * build: remove madge circular-dep check (unmaintained) madge is unmaintained (~3 years stale) and the check was never wired into CI, so it was a dormant script + devDependency on a load-bearing path. Drop it: remove the check:cycles script, the madge devDependency, and .madgerc. The no-cycle invariant still holds by construction — @plannotator/core imports nothing (zero @plannotator deps in its package.json), so any accidental core->shared/ui import fails at publish-time bun pm pack (and review). No automated tripwire, but no stale unmaintained tooling either. * fix(ui): address review — TDZ guard, html-viewer export, doc corrections - useExternalAnnotations: declare unsubscribe as let (not const) + guard calls, so a host transport that fires onError synchronously during subscribe falls back to polling instead of throwing a TDZ ReferenceError (Plannotator's EventSource fires async, never hit) - package.json: add explicit ./components/html-viewer export (dir has index.ts; the ./components/* -> *.tsx wildcard can't resolve it, so external installers would fail) - README: fix configurePlannotatorUI sample keys to the real option names (storageBackend/identityProvider/imageSrcResolver/externalAnnotationTransport) - AGENTS.md: point the Ask-AI mapping at packages/core/agents.ts (shared/agents.ts is a shim now) All publish/host-path/doc only — Plannotator unchanged. (#1 CSS-build font collision deferred to publish-prep — it needs the asset pipeline + files allowlist, not a one-liner.) * build(ui): don't bundle fonts in published styles.css — app loads fonts (review #1) Industry standard for a shared UI package: ship theme + component CSS, let the consuming app load fonts. Drop the @fontsource imports from styles-entry.css (the publish CSS entry); the theme still defines --font-sans/--font-mono, and the app provides those families. Fixes the asset-name collision (every emitted .woff2 was renamed styles.css) and shrinks the published stylesheet 555kB -> 185kB. README documents the two-line @fontsource install. Plannotator unaffected: its apps (editor/review-editor index.css) load fonts via their own entry CSS — styles-entry.css is consumed ONLY by the publish CSS build. * fix(ui): build styles.css on prepack, not prepublishOnly (review #4) prepublishOnly doesn't run for npm pack / bun pm pack / git / file: installs, so the package exported ./styles.css without shipping it. prepack runs on any pack, so the stylesheet is always present. Verified: bun pm pack now emits styles.css. * chore(ui): post-rebase reconciliation — version lockstep 0.21.3, awaitable AI abort seam Rebased onto main (0.21.3). Bump @plannotator/core + @plannotator/ui to 0.21.3 to stay in lockstep with the repo version. Resolve the useAIChat conflict: main added postServerAbort (an awaitable abort that prevents session-busy races) using a raw fetch. Route it through the AITransport seam by making AITransport.abort return Promise<unknown> instead of void, so the host override is honored AND main's await-the-abort behavior is preserved. Update the abort mocks in the seam/configure tests accordingly. * fix(ui): make postServerAbort never reject regardless of AI transport The await site in ask() relies on postServerAbort resolving so a superseding query can proceed. main's original guaranteed this with its own .catch on the fetch; routing through the AITransport seam delegated that guarantee to the transport. Restore it at the call site (Promise.resolve(...).catch) so a host override that rejects — or returns void at runtime — can't throw out of ask(). * fix(ui): address review — core import, abort sync-throw, snapshot guards - useAIProviderConfig: import Origin from @plannotator/core/agents (was the only ui file still importing @plannotator/shared); drop the masking shared/* path alias from ui/tsconfig.json so a stray shared import now fails typecheck. The hook is part of the published surface — a standalone install had no @plannotator/shared to resolve. - useAIChat.postServerAbort: defer the transport call into .then so a host abort that throws *synchronously* also can't reject (the .catch only caught async). - useExternalAnnotations: default getSnapshot returns null (skip) on a malformed 200 instead of coercing to []/0, so it can't clear annotations or reset the version cursor — restoring the pre-seam behavior. * feat(ui): add upload + identity-editable seams for host backends Two override points the Workspaces app needs that had no seam: - UploadTransport (utils/upload.ts): image attachments hardcoded POST /api/upload with no override. Add a setX/resetX/getX seam (default = today's /api/upload, verbatim) and route AttachmentsButton through it. Workspaces sends bytes to its R2 asset API and returns the content-addressed URL. - IdentityProvider.isEditable() (utils/identity.ts): the Settings rename/regenerate controls wrote to the cookie store, bypassing a host identity provider — so a host with server-owned identity could split one user across two author names. Add an optional isEditable() (default true) and hide the rename controls when a host returns false. Plannotator's cookie identity stays editable — unchanged. Both wired into configurePlannotatorUI(); seam tests added; configure routing test covers uploadTransport. HANDOFF.md updated with the Workspaces seam mapping from the repo research (asset layer, identity, realtime, no-AI-infra, the Me display-name backend follow-up). README publish command corrected to bun pm pack + npm publish. * refactor(ui): capture sessionId synchronously in postServerAbort Self-review: the deferred .then read sessionIdRef.current a microtask after the guard checked it. Capture the id synchronously so the abort always targets the session current at call time and there's no double-read. * fix(ui): address review — seed host store, browser-safe timer type, harden abort - configStore.loadFromBackend: seed the host StorageBackend with resolved defaults for keys it lacks. The constructor runs at module load (before a host installs its backend), so its default-seeding writes went to the cookie backend; without this a fresh host store was never populated and generated defaults (e.g. displayName) regenerated every reload. [P1, host path] - Viewer.tsx: replace NodeJS.Timeout with ReturnType<typeof setTimeout> (2 refs) so a browser-only consumer compiling the published source doesn't need @types/node. Matches the pattern already used in configStore. [P1, published path] - useAIChat: harden the create-session supersede abort the same way as postServerAbort, so a host transport that throws can't surface an unhandled rejection. No impact on Plannotator (default self-catches). [nit] - .gitignore: correct stale 'prepublishOnly' comment to 'prepack'. [nit] Plannotator behavior unchanged (it never calls loadFromBackend; the timer/abort changes are behavior-preserving). Strengthened configStore seam test to assert first-run seeding. typecheck clean, 1773 pass / 0 fail. * refactor(ui): single-source the never-reject abort via safeAbort helper Self-review: the hardened abort pattern (defer into .then + .catch so a host transport that throws can't reject) was duplicated across postServerAbort and the create-session supersede site — the exact drift the review flagged. Extract a module-level safeAbort(sessionId) so both call sites share one hardened implementation and can't diverge again. Behavior unchanged; reads aiTransport at call time so a late override is honored. * chore(ui): post-rebase version lockstep to 0.21.4 Rebased onto main (0.21.4, adds markdown math #878 + parser hardening). Bump @plannotator/core + @plannotator/ui to 0.21.4 to stay in lockstep with the repo. katex (main's math dep) merged into ui; typecheck clean, 1810 pass / 0 fail. * docs(ui): consumer-lens handoff hardening + ADR 005 - HANDOFF.md: add supported-imports allowlist vs unsupported (hardcoded /api/*) list; document the annotation anchor schema, reattachment order, and untested stale-anchor degradation; state that the markdown editor cannot take CM6/Yjs extensions yet and the plan of record; note AI avoidability re-verified post-rebase; fix stale 0.21.3 ref. - adr/decisions/005: record the publish-as-packages decision (packages over copy/vendor, core/ui split, seam-singleton pattern + SSR revisit condition, the law, lockstep publish model). * fix(ui): make shipped source strict-TS clean for consumers + seam type barrel Consumers compile the published TS source with their own compiler options, and strict mode failed with 35 errors inside the package: - settings.ts: satisfies SettingDef<unknown> is contravariantly illegal under strictFunctionTypes (33 errors) — use SettingDef<any> - useDismissOnOutsideAndEscape: RefObject<HTMLElement> rejects React 19's useRef<T>(null) refs — widen to HTMLElement | null - globals.d.ts: declare *.png / *.webp modules, referenced from each asset-importing component so any consumer program that includes one gets the ambient declarations Also unscatter the seam contract types: configure.ts re-exports every seam type next to configurePlannotatorUI, and ServerSyncFn is now exported from config/index.ts (it was unreachable through the exports map). Verified: standalone Vite consumer importing the full supported surface passes tsc --noEmit under full strict (was 35 errors). * fix(ui): keep KaTeX fonts out of published styles.css (back to ~187KB, was 1.6MB) Main's math PR imports katex/dist/katex.min.css in theme.css; the publish build (Vite lib mode) force-inlines all 60 KaTeX math fonts as data URIs, ballooning styles.css to 1.6MB (977KB gzip) and breaking the package's consumer-owns-fonts policy. Alias the katex stylesheet to an empty stub in vite.css.config.ts only — theme.css stays untouched (no rebase surface) and Plannotator's own apps, which import theme.css directly, still bundle KaTeX as before. Hosts that render math load katex.min.css themselves (bundler import, CDN tag, or self-hosted copy per HANDOFF.md), which also gets them lazy font loading. Verified: fresh build is 186.9KB / 30.8KB gzip with zero @font-face data URIs; consumer vite build CSS drops 1.66MB -> 200KB. * docs(ui): HANDOFF corrections from adversarial consumer review - Math rendering section: KaTeX css/fonts excluded from styles.css by design; three one-time host setup options (self-hosted recommended, CDN tag, bundler import) - styles.css size claim corrected (~187KB / ~31KB gzip) + strict-TS guarantee documented (verified against a standalone consumer) - AI-avoidability claim made precise: configure.ts statically imports useAIChat for its setter; unused AI code tree-shakes to zero (bundle- verified) — the runtime claim holds, the static wording was wrong - Loud warning on the loadSettingsFromBackend ordering footgun: configuring before hydration seeds generated defaults into the host backend and nothing re-runs hydration - DraftTransport.load() tombstone-generation contract spelled out - Seam-type barrel documented on the configure row; 'everything is importable' softened (some components/*.ts don't resolve via the *.tsx wildcard); stale diff stats refreshed * docs(ui): math setup pointer in README + pnpm caveat on the katex bundler-import option * fix(ui): lazy settings resolution — zero cookies on a configured host The configStore resolved all settings eagerly in its constructor, at module import — before a host's configurePlannotatorUI() could install its StorageBackend — writing 17 plannotator-* cookies (including a generated identity) onto the host origin. Resolution now runs lazily on first settings access (get/set/init/loadFromBackend): by then the host backend is live, so the initial reads AND default-seeding writes route through it. A configured host gets zero cookies, ever. Plannotator unchanged: same resolution, same cookie seeding, same values — on first settings read (same page load) instead of at import. New configStore.lazyInit.seam.test.ts proves the contract from a fresh module graph; full suite + consumer strict tsc green. * chore(ui): post-rebase version lockstep to 0.22.0 * fix(ui): round-2 review batch — dedupe asset declarations, CI seam tests, strict consumer gate, doc corrections - components/types.d.ts: drop the *.png/*.webp declarations that globals.d.ts now owns — both shipping was a duplicate-identifier error for any consumer with skipLibCheck: false - untrack packages/ui/styles.css (generated by prepack, gitignored; got scooped into the carve commit during the rebase by git add -A before the ignore entry existed in the replay) - CI: the DOM test step now runs ALL packages/ui tests, so the seam contract tests (AI/draft/external-annotations/file-tree/inline- markdown) actually execute in CI instead of skipping - new packages/ui/tsconfig.strict-consumer.json wired into root typecheck: type-checks the supported-import surface under full strict, so the consumer strict-TS guarantee can't silently rot - HANDOFF: rot-proofed the diff stat, strict guarantee now cites the CI gate, CDN katex pinned-version wording, theme-vs-styles.css caveats (theme still imports KaTeX + needs Tailwind), Viewer required props, Yjs plan-of-record updated to the atomic-editor fork - README: @source fallback wording (build entry isn't shipped) * test(ui): make the lazy-resolution seam test deterministic The test asserted lazy resolution on the module singleton and relied on its test file getting a fresh module graph — an isolation assumption that doesn't hold under all bun test orderings (CI failed with zero observed reads because another file had already resolved the store). Test the contract on a fresh instance instead: ConfigStore is exported as @internal ConfigStoreForTest, the spy backend is installed before construction, and the test asserts construction reads nothing while the first get() resolves and seeds through the live backend. Deterministic by construction. * test(ui): poll for the debounced reconnect refetch instead of a fixed sleep The reconnect-refresh assertion waited a fixed 150ms against the SSE watcher's 120ms debounce — a 30ms margin that slower CI runners lose, flaking 'refreshes after an SSE ready event from reconnect'. The watched logic is unchanged (verified byte-identical to main's inline version — the seam only relocated it into the default watchTrees and added the onChange indirection). Poll for calls.length===2 up to 1s so the pass/fail is hardware-independent. * test(ui): poll the committed tree state, not the fetch call count Prior fix polled calls.length===2, but the fetch call is counted one tick before its result commits to React state — so the poll exited early and the next assertion (dirs[0].tree === reconnectedTree) lost the race on slow CI (toEqual failure). Poll on the committed tree itself, which is exactly what the assertion checks: now the only way to fail is a genuine no-refresh, not a timing margin. * test(ui): give the reconnect-refetch poll a 10s ceiling + 20s test timeout A CI runner was measured at 6x normal speed (1676ms for a ~275ms test), blowing through the 1.5s poll ceiling before the 120ms debounce fired — same commit passed on a faster runner. Raise the poll to ~10s and set an explicit 20s test timeout (bun's 5s default would otherwise kill the poll). Root cause is load, not logic: this timing-sensitive test only started flaking when the CI DOM step was broadened to run the whole ui suite in one process. * ci: run the file-browser DOM test isolated; scope the DOM step to DOM files Root-causes the intermittent 'refreshes after an SSE ready event from reconnect' failure. The round-2 change ran the ENTIRE ui suite under DOM_TESTS=1 to catch the seam contracts; that load intermittently starved the test's 120ms real-timer debounce so the reconnect refetch never fired (observed failing after a full 10s poll — not a margin issue). The hook logic is byte-identical to main, and main runs this test in its own process (green for months). Fix at the CI layer, not the test: run useFileBrowser.test.tsx isolated (matching main), and run the seam contracts + remaining DOM-gated tests as an explicitly-scoped light batch. The test file is reverted to main verbatim (today's timing-poll experiments dropped). Follow-up issue to file: the underlying re-subscription race the load exposed. |
||
|
egouilliard-leyton
|
73efacfa0c |
feat(annotate): per-file version diff for .md and .html (rendered HTML highlights) (#961)
* feat(annotate): version diff for annotated files Annotate mode never tracked version history, so the existing Plan Diff (highlighted diff vs a previous version) only worked in plan mode. Wire per-file version history into the annotate server so the same diff UI — badge, Version Browser, block-level comments — works when annotating a standalone .md/.txt/.html file. - key history by file path (stable across edits) rather than the plan flow's heading+date slug - save the markdown (or raw HTML source) to history on each open, expose previousPlan + versionInfo + diffCurrent on /api/plan - add /api/plan/version and /api/plan/versions to the annotate server Markdown lights up end to end; HTML needs frontend follow-ups (feed the HTML source as the diff content, surface the badge on the html surface, default to source diff mode). * feat(annotate): rendered HTML version diff with inline highlights For --render-html files, render the version diff as the real page with inline <ins>/<del> highlights instead of a markdown/source diff: - add packages/shared/html-diff.ts: a tag-aware htmlDiff() that wraps changed text in <ins>/<del> while keeping tags balanced (script/style opaque). 9 unit tests. - annotate server computes diffHtml = rewriteHtml(htmlDiff(prev, current)) and exposes it on /api/plan - HtmlViewer: inject ins/del highlight CSS, add a 'Show/Hide changes' toggle in its action bar - App: store diffHtml, swap the iframe to the diff page when toggled, and suppress the markdown block-diff path on the HTML surface Commenting still works because the diff page renders through the same HtmlViewer iframe bridge. * docs(annotate): document the annotate version diff + endpoints * feat(annotate): mirror version diff into the Pi server Parity for the Pi (node:http) runtime: per-file version history, previousPlan/versionInfo/diffCurrent + diffHtml on /api/plan, the /api/plan/version[s] endpoints, and project wiring from the Pi CLI. Vendors @plannotator/shared/html-diff into pi-extension/generated. * review fixes: pi diff dependency, attr-aware tokenizer, history opt-out, hide dead version picker on HTML - apps/pi-extension/package.json: declare the 'diff' dependency — generated/html-diff.js imports it at module load, so a standalone Pi install failed to resolve it and broke every annotate session (the monorepo masked this via root hoisting) - packages/shared/html-diff.ts: tag tokenizer now consumes quoted attribute values whole, so a '>' inside title="a > b" no longer splits the tag and corrupts the diff output; 3 regression tests - annotate history is now gated by PLANNOTATOR_ANNOTATE_HISTORY / config.annotateHistory (default on) and disclosed in AGENTS.md — it writes copies of annotated files into the data dir, which users should be able to see coming and turn off - packages/editor/App.tsx: hide the sidebar Versions tab on the HTML surface — the base-version picker has nothing to drive there (the HTML diff is fixed to current-vs-previous); the viewer's Show changes toggle is unaffected * fix(ui): document content clears the badge cluster dynamically The repo/diff badge cluster is absolutely positioned in the card's top padding, sized by guesswork (py-5..py-12). One chip row fit; the diff badge's second row overflowed into the H1, and mobile wrapping made the badge sit on top of the heading. Measure the cluster (ResizeObserver) and insert exactly the clearance it needs — zero when it fits, so existing single-row layouts don't shift. Pre-existing plan-mode bug surfaced by the annotate version diff. --------- Co-authored-by: Edouard Gouilliard <edouard.gouilliard13@gmail.com> Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Michael Ramos
|
3ebb28a835 |
fix(review): avatar lookups can no longer delay /api/commits
The gh/glab runner has no subprocess timeout, and the commits endpoint awaited avatar resolution before responding — a hanging network (proxy black-hole, dead DNS) held the already-computed commit list hostage to decoration. resolve() now races a 4s ceiling: on expiry the endpoint returns with the initials fallback, the in-flight fetch finishes in the background, and later calls pick its results out of the cache. Timed-out attempts memoize no misses. Shared module, so both runtimes get it. |
||
|
Michael Ramos
|
aa46dbb29a |
feat: guide per-file summaries + GitHub Copilot CLI agent engine (#997)
* feat(guide): per-file summary on guide diff refs Each diffs[] entry now carries a required (schema-enforced) 1-2 sentence summary of the semantic change in that file, written from the diff hunks alone. Sanitizer passes it through when it's a non-blank string and omits it otherwise -- a missing summary never drops the ref or fails the guide. Rendered as a muted inline-markdown line above each diff in the guide; marker-engine contract and demo data updated to match. * fix(guide): repair prompt must not invent missing summaries The guide schema now requires summary, so a schema-enforced repair of a payload that lacks them (marker-engine output) would force the model to fabricate captions with no diff in sight. Instruct it to fill missing required fields with an empty string instead; the sanitizer already renders nothing for blanks. * feat(agents): GitHub Copilot CLI as a marker engine for review + guide jobs Adds copilot as the fourth marker engine (no schema flag, so it uses the nonce-tagged marker-block contract like Cursor/OpenCode/Pi): - marker-review.ts: COPILOT_ENGINE — 'copilot --output-format json' JSONL stream (assistant.message carries the assembled text; deltas skipped), models discovered from 'copilot help config', live-log formatting for tool.execution_start/complete. Non-interactive posture: --no-ask-user auto-denies unallowed tools, --deny-tool=write, read-only-oriented shell allowlist (git/gh/glab/jj/wc), builtin MCPs and auto-update disabled. - Exported MarkerEngineId and replaced every 'cursor'|'opencode'|'pi' cast with it (Bun server, Pi server mirror, guide-review) so the next engine is a two-edit change. - agent-jobs (both runtimes): copilot in SERVER_BUILT_PROVIDERS; capability entry + model discovery come free from the MARKER_ENGINES loop. - UI: copilot review/guide engine with per-surface model settings (useAgentSettings), AgentsTab launch + config rows, GuideEmptyState launcher, job-detail labels, CopilotIcon (currentColor, official mark). - Tests: argv/read-only flags, help-config model parsing, stream reduction, full marker pipeline; profile-map expectations widened. Verified live: composed review prompt through the real copilot binary, marker block parsed, seeded bug found. * fix(agents): structurally deny high-consequence verbs for Copilot jobs git:*/gh:*/glab:* stay allowed for inspection ergonomics, but Copilot's deny-precedence rules now block the verbs a prompt-injected background job could abuse: git push/reset/clean/checkout/restore (local reviews run in the user's real working tree), and PR/MR/issue comment/create/merge/ close/edit/review on gh and glab. Probe-verified: git log runs, git push --dry-run is denied by the shell(git push) rule. Addresses the one confirmed finding from the PR #997 review round; the other two (untracked-file reads, git -C) were refuted by live probes. |
||
|
Michael Ramos
|
d6b98f0c0d |
feat(review): Guided Review + Pi agent-job provider (#993)
* docs(adr): Guided Review ADR/spec/research + agent-provider fit studies
ADR 006 (Guided Review as a first-class feature), four implementation
spikes, synthesis, spec (iterated through preflight and review fixes),
recap, and the Flue/Pi provider-fit research syntheses.
* feat(review): Guided Review takeover + Pi agent-job provider
Guided Review (ADR 006) — a Linear-Guides-style chaptered review:
- guide agent-job provider (packages/server/guide/guide-review.ts):
schema-constrained sections (title/overview/file refs) over the live
patch, coverage-validated server-side (every changed file exactly once,
fabricated paths dropped, fail-closed on empty output), importance-first
ordering prompt with speed discipline (diff-first, no repo exploration)
- runs on claude/codex natively and cursor/opencode/pi via the marker
contract; guide-scoped low effort defaults for quicker generation
- takeover UI (packages/review-editor/components/guide/): Guide header
badge + Mod+Shift+G, full-width screen that CSS-hides (never unmounts)
the file tree/dock, empty state with inline model pickers, skeleton
loading, generating state with collapsed activity log, two-column
section cards (sticky prose column with internally-scrolling file list,
content-height diffs, review-independent collapse, peek semantics)
- annotation parity: guide diffs mount the real DiffViewer with
file-scoped handlers into the same CodeAnnotation state and feedback
export; per-section reviewed state persisted via /api/guide/:jobId
- reviewed/tour prompt speed sections; renderMarkdownProse shared between
tour and guide (fenced-block support, muted tone variant)
Pi agent-job provider:
- third MarkerEngine (pi --mode json --no-session --no-approve) with
live model-catalog discovery, thinking-level control (--thinking),
fail-closed marker parsing (Pi exits 0 on in-run errors by design)
- available for review + guide jobs from both launch surfaces
Shared UX:
- searchable, provider-grouped model pickers (auto over 12 options)
- full Claude version catalog with latest-resolving aliases labeled
- AgentControls primitives extracted from AgentsTab; cross-instance
settings sync in useAgentSettings
- Pi extension server hand-mirrors + vendor.sh entries throughout
* fix(guide): PR-993 review fixes + failure recovery ladder
Review fixes:
- coerce marker-engine title/intent at ingest (prompt-enforced output
could store non-strings and crash the takeover)
- keep placed files out of unplacedFiles (exactly-once guarantee)
- shared REVIEW_ENGINE_LABEL (fixes 'generated by pi' header; typed
exhaustiveness prevents recurrence)
- key ActiveGuide by jobId so per-guide focus state resets
- formatModel handles marker-engine tour/guide jobs (empty model chip)
- drop dead guideCodexFast; memoize estimateDiffHeight
- store + display Pi thinking level on job cards
Failure recovery ladder (auto -> one click -> manual -> regenerate):
- mechanical JSON repair pass (fences/slice/trailing-commas/bracket
balance) before any guide parse fail-closes
- failed payloads captured per job (200KB cap, per-engine extraction)
- 'Fix output' repair job: pure text transform on a schema-capable
engine (claude/codex preferred), forced low effort, lands as a
normal guide job; repairOf threaded through launch validation
- editable output panel: GET /api/guide/:jobId/output prefills a
textarea, POST /api/guide/:jobId/submit runs the same server-side
validation and opens the fixed guide; inline errors for iteration
- Pi extension mirrors + AGENTS.md endpoint/body docs
* fix(guide): string-aware mechanical JSON repair (self-review)
- terminate a dangling string literal before appending bracket closers
(truncation mid-string is the most common shape; closers appended
inside the string never parsed)
- strip trailing commas outside string literals only (the regex could
rewrite overview content, violating the repair contract)
* fix(guide): launch-snapshot validation, codex strict schema, repair persistence
PR-993 round-2 review fixes + live-test findings:
- validate guide completion (and manual repair) against the LAUNCH-time
changed-file set, snapshotted per job — switching diff/base/PR while a
guide generates no longer destroys valid output (client already
degrades stale refs per-file)
- codex strict structured output: unplacedFiles now required in the
guide schema (OpenAI 400s schemas with optional properties under
additionalProperties:false — live-confirmed, then live-verified fixed:
5 sections / 83 files placed on a ChatGPT-account codex)
- manual repair survives reload: successful /submit flips the job to
done via completeJobExternally + status guard on the endpoint
- codex model defaults -> gpt-5.5 (5.3-codex is deprecated; ChatGPT
accounts reject it) with one-shot migration of saved picks; guide
codex default reasoning stays low
- pi extension guide branch snapshots launch state (TOCTOU hygiene)
- paragraph collector no longer swallows an unspaced code fence
- tour hook backports: out-of-order fetch guard; save outside updater
- guide empty-state copy rewritten value-first
* fix(guide): PR-993 round-3 — repair-diff fidelity, failure surfacing, pi tool hardening
- repair jobs validate against the FAILED job's recorded file set (the
payload under repair references that changeset; validating against the
diff on screen at repair time re-introduced destroy-on-switch)
- takeover follows the NEWEST running guide (progress + Cancel), and a
failure newer than the shown guide surfaces as a dismissible strip
(error + Fix output + Details) instead of being silently masked
- pi jobs run with --exclude-tools edit,write (bash stays: the agent
needs git inspection; full read-only would break generation)
- tour/guide result ingestion fails closed on unexpected throw (done-
looking jobs no longer 404 their result)
- sidebar guide mode gated on file availability like the header badge
- drop orphaned DEFAULT_GUIDE_CODEX_FAST (fast mode intentionally not
offered for guide); retry owns its fetch cancellation; final
trailing-comma pass after bracket-closing in the repair ladder;
collapsed-row checkbox/expand are sibling buttons (a11y)
* fix(guide): PR-993 round-4 — callout text, guide job detail, toggle races
- single-line callouts (> [!IMPORTANT] message) keep their message — the
form our own prompt solicits rendered an empty labeled box; type now
derives from the [!TAG] capture, not a whole-line scan that mistyped
'> [!NOTE] this is important'
- job detail panel gets a guide status card (Open guide via a new
ReviewStateContext.openGuide) instead of mislabeling 'Guide Generated'
as a red Incorrect review
- marker-engine guide jobs get readable live logs (engine fallback in
the formatter lookup; provider stays 'guide')
- repair prefers the failed job's own schema-capable engine (proven
working on this machine) before falling back by binary presence
- toggleReviewed/toggleChecked: pure functional updaters + persistence
effect with seed-skip — race-proof AND StrictMode-safe (resolves the
tradeoff previous rounds accepted)
- dedups: formatDuration/ElapsedTime shared from AgentsTab; whichCmd
exported in the pi extension; SEARCHABLE_THRESHOLD imported by
InlinePicker
- new renderMarkdownProse tests (single-line/multi-line callouts,
paragraph passthrough)
* fix(guide): PR-993 round-5 — keep engine-default model option, label Pi jobs
- GuideEmptyState's marker catalogs mirror AgentsTab's per-engine
semantics: opencode/pi prepend the engine-managed Default ('' value)
to the discovered list (dropping it left saved-default users a blank
pill with no way back); cursor replaces (its list includes 'auto')
- job detail provider pill labels pi review jobs 'Pi' instead of the
'Shell' fallback
* fix(guide): PR-993 round-6 — context scoping, stale models, repair engine, staging gate
- guide takeover + auto-open scoped to the current review context: a
guide launched against PR A no longer shows over PR B, and a guide
finishing for an away context defers its auto-open until the reviewer
returns to that context (unmarked in the dedupe set on purpose)
- guide launcher reconciles saved cursor/opencode/pi model ids against
the live catalog at read time (picker + launch share one effective
value) — no more posting dead ids after an account switch
- repair prefers the failed job's OWN engine whenever its binary is
present (provably runnable; marker binaries resolved via
MARKER_ENGINES — cursor's CLI is 'agent'); claude/codex are fallback
only, killing the broken-claude repair doom loop
- per-file staging gate (canStagePath) in guide diffs AND the
pre-existing same gap in ReviewDiffPanel — committed-only files in
since-base reviews no longer offer a no-op Git Add
* fix(review): scope tour auto-open to current context (self-review)
Same cross-context gap just fixed for guides: a tour finishing for PR A
popped its dialog over PR B. Both auto-open effects now share one
jobMatchesCurrentContext helper with the same deferred-open semantics.
* fix(guide): PR-993 round-7 — complete the context-scoping story
Round 6 scoped guides to their review context; this closes the two
surfaces that scoping left dangling, plus a focus gap:
- switching to a context that already HAS a completed guide now shows
that guide: the takeover falls back to the context's newest done
guide job when activeGuideJobId belongs elsewhere (previously landed
on the empty state with the guide sitting unreachable)
- 'Open guide' affordances are context-gated everywhere they exist:
job cards hide it for cross-context guides (opening can't switch
PRs), and the job detail panel explains where the guide belongs
instead of offering a dead button; one shared
jobMatchesReviewContext predicate now backs App, GuideScreen,
AgentsTab, and the detail panel
- file-chip navigation retargets the guide's focus arbiter (scrolling
under a stationary pointer fires no pointerenter, so the annotation
toolbar stayed bound to the previous diff)
* fix(tour): retry owns its fetch cancellation; document prUrl-only scoping
- useTourData retry converts to the nonce-driven effect re-run pattern
(the round-4 backport carried the fetch guard but not the retry fix
useGuideData got in the same batch — half a backport)
- jobMatchesReviewContext now documents WHY it matches by prUrl only
and not diffScope/diffContext: guides/tours reference files and
degrade per-file when the diff shifts; scope-strict matching would
hide useful artifacts on layer/base/mode switches. Deliberate,
do-not-tighten-without-UX-decision
* fix(guide): PR-993 round-9 — worktree-aware context, guide-scoped marker settings
- jobMatchesReviewContext now compares diffContext.worktreePath for
local jobs: prUrl + worktree define WHERE the review is (matched
strictly); mode/base/scope remain WHAT VIEW (deliberately loose) —
a guide launched against worktree A no longer opens over worktree B.
Jobs predating the snapshot coalesce to the main tree.
- Cursor/OpenCode/Pi get guide-scoped model/thinking settings
(guideCursor/guideOpencode/guidePi), mirroring the existing
guideClaude/guideCodex isolation: tuning a guide's marker model no
longer silently changes the next code review with that engine.
Defaults are each engine's natural default, not seeded from review
settings (same precedent as guideClaudeEffort='low'). AgentsTab's
reconcile effects snap both surfaces against the shared catalogs.
* fix(review): keep worktree parsing browser-safe (round-9 build fix)
parseWorktreeDiffType lives in shared/review-core, which imports
node:path at module top — pulling it into App.tsx broke the vite
browser build (typecheck and tests don't catch it; only the bundle
does). Context matching now reuses App's existing hand-parsed
activeWorktreePath memo — one parse, and it's the same one that
drives the sections/tree UI, so matching aligns with what's on
screen. Also fixes a deps-array reference to the removed memo that
bundled fine (treated as a global) but would have thrown a
ReferenceError at runtime.
* docs(review): mark worktree-subtype lockstep between server parser and App copy
* fix(review): PR-993 round-10 — guide shortcut gate, settings sync race, heading markdown
- bare a/v staging/viewed shortcuts suspend while the guide takeover is
open (the dock is only CSS-hidden, so a diff panel stayed 'active'
underneath and bare keys acted on an invisible file)
- cross-instance settings sync replaces the was-last-update-remote
boolean with value comparison (lastSyncedJsonRef): the flag conflated
'a commit happened' with 'the last change was remote', so a local
edit batched with an incoming broadcast silently skipped its own
cookie write and rebroadcast
- ### headings in guide/tour prose run through renderInlineMarkdown
like every other block (backticked symbols rendered as raw tokens);
regression test added
* fix(guide): PR-993 round-11 — reveal channel for sidebar jumps, repair exit code
- new guideRevealFile channel closes the documented round-7 gap and its
AI sibling: sidebar jumps (annotation clicks, AI line citations) made
while the guide takeover is open no longer mutate the hidden dock's
active file; instead the GuideSectionCard containing the target file
expands its collapsed (reviewed) section, focuses the diff, and
scrolls to it — so jumps into collapsed sections stop silently
no-opping. Cleared on guide close/switch so keyed remounts don't
replay the last reveal.
- completeJobExternally resets exitCode to 0 (both servers): a
successfully repaired guide kept showing the failed run's Exit 1 chip
in the job detail panel
* fix(guide): clear reveal channel in-batch at guide-switch sites (self-review)
The clear-on-change effect fires after a switched guide's keyed cards
have already mounted (child effects run before parent effects), leaving
one commit where a stale reveal from guide A could expand+scroll a
same-named file's section in guide B. All three setActiveGuideJobId
sites now clear synchronously in the same batch; the effect stays as a
backstop for close and future set sites.
* fix(guide): PR-993 round-12 — titled sections, honest large-PR validation
- sanitizeGuideSection gives every surviving section a non-empty title
('Untitled section' fallback): a diffs-only section rendered as a
blank chapter inside a 'Guide Generated' job — no parse failure, so
no recovery flow. Keeping (titled) beats dropping: placed files are
not in unplacedFiles, so dropping would orphan them silently.
- guide launches on large PRs (layerPatchIncomplete) recompute
changedFiles from the local checkout (git diff --numstat
origin/<base>...HEAD, rename/binary handling) when available — the
PR-mode prompt tells the agent to read that full local diff, but the
changed-files block and validation snapshot came from the truncated
platform patch, under-listing files to the model and then dropping
its valid refs (or failing the guide) at validation. Falls back to
the partial list on any failure. Mirrored in the Pi extension server.
Round-12's P1 (Pi marker extraction reads message_end, 'should' read
text_delta) was refuted by live probe: Pi's assistant message_end
carries the fully-assembled content blocks, which is exactly what
piExtractText reads; the delta-reading code the finding cross-
referenced is the Ask-AI STREAMING provider, a different job.
* test(guide): first direct coverage for guide-review's pure logic (self-review)
The module's repair ladder, validation, and stream parsing had zero
unit tests — exercised only end-to-end through live agent runs. Pins
the behaviors the review rounds fixed: blank-title fallback (round 12),
first-placement-wins dedup, changedFiles filtering + fail-closed empty
guides, prose-only vs lost-diffs section handling, unplacedFiles
merge/dedup/fabrication-filtering, title/intent coercion, trailing-
comma and unbalanced-bracket repair, truncated stream-line recovery.
13 tests; server suite 374 → 387.
* fix(guide): PR-993 round-13 — full-stack recompute regression, flag snapshot, unplaced reveal
- the round-12 large-PR recompute now runs ONLY in layer scope: in
full-stack scope launchPatch is already a local full recompute, and
the layer diff (origin/<base>...HEAD) was the WRONG file set — it
omits earlier stack layers' files, dropping their refs at validation
or failing the guide. Introduced by round 12; caught before any
release. Both servers.
- layerPatchIncomplete is snapshotted with the other launch locals
(launchLayerPatchIncomplete): coherence with launchPatch was
positional (same sync segment) — now structural, so a future await
inserted upstream can't silently desync them. Both servers.
- the 'Everything else' bucket handles guideRevealFile: sidebar jumps
to unplaced files now focus + scroll their diff (the section-card
effect only covers placed files) — closes the residual gap noted in
the round-11 self-review
|
||
|
Michael Ramos
|
e8df06db7c |
feat(review): Commits panel — linear history rail with per-commit diffs (#994)
* docs(adr): spec for the commit-list review view (linear history rail)
* feat(review): commit:<sha> diff mode along the shared diff-type seams
A new git-only diff family for reviewing one historical commit against
its first parent (git-show style), threaded through every seam a diff
type crosses:
- runGitDiff: git diff <sha>^ <sha>; a root commit diffs against the
empty tree. Label is 'Commit <shortsha> — <subject>'. The sha is
validated as bare hex (parseCommitDiffType) before it reaches any
argv position, on top of --end-of-options.
- getGitDiffFingerprint: anchored to the sha alone (present/gone), NOT
headSha — new commits landing mid-review don't change this diff and
must not raise the staleness banner. A vanished commit (rebase + gc)
flips present→gone and fires it honestly.
- getFileContentsForDiff: old side <sha>^ (null on root), new side <sha>.
- parseWorktreeDiffType learns worktree:<path>:commit:<sha> (the
sub-type contains a colon, so it needs its own split) — commit review
composes with worktree sessions like every other mode.
- vcs-core: the git provider owns commit:*; staging stays gated off
(canStageFiles allowlist unchanged — nothing in a historical commit
is stageable).
- Ask AI context: a commit-mode inspect string ('git show <sha>', with
an explicit 'historical commit, not the working tree' note).
Also adds listCommitHistory for the upcoming /api/commits endpoint:
one --first-parent page from HEAD (before=<sha> continues at its first
parent, +1 fetch for an honest hasMore), with isHead / isRepoUser /
isPastBase flags. isPastBase is reachability from the base — computed
as the complement of 'rev-list --first-parent HEAD ^base', which is a
prefix of the walk, so the client's single divider is exhaustive. An
unresolvable base degrades to no divider (matches since-base's posture
on such repos).
* feat(review): GET /api/commits — linear history endpoint (Bun + Pi)
One page of the branch's --first-parent history (?limit=&before=),
served by listCommitHistory against the active diff's cwd (worktree-
aware via resolveVcsCwd) and the active base (so the divider tracks
the same baseline the review compares against, including the startup
origin/<default> upgrade).
Gated to plain local git sessions — PR / workspace / jj / p4 return
400, mirroring the client's commitsCapable gate. Both runtimes; the
route-parity test covers the pair.
commit:<sha> switching needs no endpoint changes: /api/diff/switch
already dispatches through runVcsDiff (the git provider owns the new
family), the epoch guard applies as-is, the sections sidecar correctly
stays since-base-only, staging 400s via the canStageFiles allowlist,
and baseRelevantDiffType keeps the behind-GitHub banner suppressed for
commit diffs.
* feat(ui): Commits panel view — the linear history rail
Third left-panel view: 'Git status | Commits | Tree'. The Commits view
is a pure commit list (short sha, subject, relative age compacted to
'2h', author only when it isn't the repo user, HEAD dot, a divider
where the branch meets the resolved base, 'Show more' paging). It
never becomes a file list: clicking a commit switches the diff to
commit:<sha> and the existing needsInitialDiffPanel flow lands the
center dock on the all-files surface; re-clicking the active commit
just re-focuses that panel.
Persistence: reviewPanelView gains 'commits' with NO diff coupling —
the view persists, the diff opens on the user's normal default until a
commit is clicked, and a sha is never persisted (it may not survive a
rebase). The sections⟺since-base coupling is untouched, but the
classic-diff→Tree snap in Settings/ReviewSetupDialog now only fires
when the current view is 'sections', so picking a default diff no
longer stomps a Commits preference. The load-time self-heal ignores
'commits' by construction (it keys on panelView === 'sections').
Gates: commitsCapable = plain local git session (no PR / workspace /
jj / p4, matching the server's /api/commits gate); the segment is
hidden elsewhere. Staging is inert in commit diffs on both ends
(STAGEABLE_DIFF_TYPES and the server allowlist never match commit:*).
Worktree sessions compose — the client parses
worktree:<path>:commit:<sha> and handleDiffSwitch prefixes as usual;
the rail refetches on worktree/base changes but deliberately not on
commit clicks, so paging state survives selection.
Accepted edges (v1): no j/k selection in the rail (each move would run
a full diff switch); toggling Commits→Tree keeps a commit diff active,
where the tree's diff-type dropdown marks nothing — the gitRef header
still names the commit.
* fix(review): self-review cleanups on the commit-list feature
- listCommitHistory: a repo with no commits yet (unborn HEAD) returns
an empty page instead of null — the panel showed a 500-backed 'Retry'
error where 'No commits' is the truth. Every other review surface
already degrades gracefully on a commit-less repo; now this one does.
Only a genuinely unanswerable repo (not a repo at all) stays null.
- Hoist the bare-hex sha rule into one BARE_HEX_SHA_RE shared by
parseCommitDiffType and the before-cursor validation (was written
twice).
- The 'all' diff case now uses the getEmptyTreeSha helper the commit
mode extracted, instead of keeping its own inline copy.
- Drop CommitsPanel's isLoadingDiff prop — declared and passed but
never used.
* feat(ui): commit cards, labeled groups, avatars — review round 1
Four items from the live review round:
- Toggle: Commits moves to the far right (Git status | Tree | Commits)
and the segment text goes 10px → 12px (padding trimmed to keep three
segments inside the 256px header).
- The bare '── origin/main ──' divider confused more than it explained.
Replaced with two labeled groups: 'On this branch' (commits not yet
reachable from the base) and 'In <base>' (shared history), each with
a tooltip spelling out what the boundary means.
- Rows become overview cards: subject (2-line clamp) on top; a meta row
with author avatar, author name (only when it isn't the repo user),
a HEAD badge, short sha, and compact age. Active card gets the
primary border/tint.
- Author avatars, reusing the PR machinery rather than reinventing it:
parseRemoteUrl/parseRemoteHost classify the origin forge; the gh/glab
invocation shape (incl. the --hostname self-hosted convention) and
GitLab's relative-avatar absolutization rule are mirrored from
pr-github/pr-gitlab. What could NOT be reused directly: PR avatars
are keyed by platform login, which local commits don't have — so
GitHub resolves via one repos/{o}/{r}/commits?per_page=100 call per
session building an author-EMAIL → avatar map (unpushed commits by
the same author resolve through it; verified live), and GitLab uses
its per-email /avatar endpoint (capped, deduped). Everything is
memoized per session and fails closed to the initials fallback — a
repo with no remote, an opaque self-hosted host, or an unauthenticated
CLI never delays or errors the commits endpoint.
The Avatar component moves out of PRCommentsTab into its own module,
now shared by the PR timeline and the commit cards. New shared module
commit-avatars.ts is vendored to Pi (vendor.sh) and the enrichment runs
in both runtimes. CommitListEntry gains authorEmail (resolver key) and
avatarUrl (server-enriched).
* ui(review): flush commit rows, prominent base boundary — review round 2
- Cards are gone: rows stack flush (no borders, no margins), hover tint
only. Subject is strictly one line, ellipsized (no clamp/wrap).
- Author name always shown (was: only when it differed from the repo
user), with the avatar, on the compact meta line under the subject.
- The base boundary is a prominent labeled rule ('— In origin/main —',
foreground-weight lines) instead of the faint border under a group
header; the 'On this branch' header above stays quiet.
* docs(adr): evaluate local stack-parent detection — scenario matrix, no build decision
Empirical stress-test of the merge-base-reachability algorithm across 9
scenarios plus this repo itself. Findings: the stacked-ness test is graph
truth (never lied); parent attribution has one content-harmless failure
(same-merge-base sibling labels) and one dangerous, provably graph-
undetectable one (branches pointing inside B's own history win and
silently hide the user's commits). Determination: only viable as a
suggest-and-confirm affordance with per-branch persisted choice — never
silent auto-defaulting. Decision to build deliberately left open.
* fix(ui): Commits view is session-only — never the opening view
Live-review catch: the panel toggle inherited the sections/tree
'last choice becomes the default' cookie write, so clicking Commits in
one session made the NEXT review open on the history rail instead of
the diff. Ruling: a review always opens on the persisted sections/tree
default; Commits is entered via the toggle each session.
- reviewPanelView narrows back to 'sections' | 'tree'; a stale
'commits' cookie is treated as unset (self-heals to the default).
- The live view is now a session overlay in App state: selecting
Commits flips it without touching config; selecting Git status/Tree
clears it and persists as before. Single choke point in
selectPanelView, so every toggle site inherits the rule.
- Settings loses the Commits segment (it configures the OPENING view,
which Commits can no longer be). Spec updated with the revision.
* ui(review): the panel toggle is session-only — it never writes the default
Second live-review ruling on persistence: the header toggle shouldn't
persist ANY choice, not just Commits. Looking at another view mid-review
must not silently change what the next review opens on.
- selectPanelView becomes a pure session override (local state layered
over the persisted value); no configStore writes from the toggle path.
- handleSwitchToSections drops its defaultDiffType write — that write
only existed to keep the persisted view/diff pair consistent, and the
toggle no longer persists a view. Settings and the setup dialog remain
the only reviewPanelView/defaultDiffType writers and keep enforcing
the sections ⟺ since-base coupling; the load-time self-heal still
repairs conflicted pairs those writers may leave.
- Spec + settings-registry comments updated to name the new rule.
* feat(ui): commit description card + collapsed all-files for commit diffs
Clicking a commit now answers 'what and why' before 'which lines':
- New commitInfo sidecar (Bun + Pi, same mode-conditional shape as
sections): when a commit:<sha> diff is active, /api/diff and
/api/diff/switch carry the commit's full metadata — subject, multiline
body, author (+ avatar via the session resolver), sha, age — from a
single 'git show -s' (getCommitDiffInfo in review-core, body as the
trailing format field so newlines survive).
- CommitDescriptionHeader heads the all-files surface: subject, author
row, and the full body rendered as markdown via the same MarkdownBody
the PR viewer uses. Long bodies scroll inside the card (max-h) so the
diff keeps the viewport.
- Commit diffs open FOLDED: AllFilesCodeView gains defaultCollapsed —
items seed collapsed at build time, the seed is part of fileSetKey
(CodeView seeds once per instance, so a seed change must remount),
and the collapse-all toggle state initializes to match. Every other
diff mode keeps opening expanded; leaving the commit diff clears the
card and the folded default together (the sidecar is absent).
Verified live through the compiled binary: switching to a real commit
returns subject/body/avatar, switching back to since-base clears it.
* ui(review): commit description scrolls with the diff — no pinned card
Round feedback: the fixed header with its own inner scrollbar felt
embedded; the description should read as the top of the document and
scroll away with the files.
- AllFilesCodeView gains leadingContent: rendered via a portal INTO
CodeView's scroll container (absolutely positioned at content top,
so it participates in the scrollable overflow), with its measured
height fed into layout.paddingTop so the virtualized items start
below it. CodeView stays the single scroll authority — no nested
scrollers, no wrapper flex row.
- CommitDescriptionHeader shows the full body inline (no inner
OverlayScrollArea). Only genuinely huge bodies (>24 lines / >1800
chars) get a CSS max-height clamp with a fade mask and a Show
more/Show less toggle — CSS clamping keeps the markdown intact, and
the ResizeObserver feeds the expanded height back into paddingTop.
Keyed by sha so the toggle resets per commit.
* ui(review): entering the Commits view auto-opens the HEAD commit
Toggling to Commits used to leave the previous mode's all-files diff on
screen — a rail full of commits next to content that belonged to none
of them. Now entering the view selects the HEAD commit (top of the
rail) so the center immediately shows its diff + description card.
Once per entry, ref-guarded: an already-active commit diff survives a
toggle round-trip, a user click supersedes it, and a failed switch
doesn't retry-loop. When entry races the first log fetch, the effect
fires as soon as the log lands.
* perf/ui(review): fast commit navigation — instant veil + skip context recompute
Rail clicks felt laggy and jumpy: the old diff sat on screen for the
whole switch round-trip and then snapped, and the switch itself was
doing a full getVcsContext recompute (branch + worktree + recent-commit
enumeration — three git walks) that a commit click can never invalidate.
- Same-cwd commit:<sha> switches skip the context recompute in both
runtimes (the client keeps its existing context when the field is
absent — long-standing contract). Measured through the compiled
binary: commit switches now ~40-60ms server-side.
- The center dock shows an immediate 'Loading commit…' veil whenever a
commit switch is in flight (or the Commits view was just entered and
HEAD auto-select hasn't landed) — the stale previous diff never shows
and the click reads as instant. Errors surface through the normal
empty-state, never trapped under the veil.
* fix(ui): PR-994 review round — toggle/settings separation + commit-nav fixes (App)
The headline fix: the load-time settings repair guarded on the live
panelView, which now carries the session toggle override — so clicking
'Git status' once mid-review (saved prefs tree + a classic diff) hit
the healer and silently persisted defaultDiffType='since-base'. The
toggle must never be a settings writer, directly or through a repair
path. Keyed to persistedPanelView now; audited the remaining
configStore.set call sites — no toggle path can reach a write.
Also in this file, same review round:
- Commit-navigation veil gains a real predicate: it drops on a commit-log
fetch error (rail shows Retry; center was stuck under the spinner
forever) AND on a genuinely empty history (zero commits — the second
stuck-veil trigger the automated review missed), while still covering
the log-loading and pre-auto-select frames.
- handleSelectCommit composes the worktree prefix once and calls
fetchDiffSwitch directly (the equality check and the switch previously
used two different composition paths).
- commitsCapable/showCommitsPanel hoisted above the global keyboard
handler; Cmd+F is no longer intercepted in the Commits view (no search
input exists there — the browser find takes over instead of a silent
no-op).
* fix(review): PR-994 review round — commit-log staleness, loading flags, avatar cap
- useCommitLog clears its cached list when the history context changes
(worktree/base switched while the view was away): the HEAD auto-select
acted on the stale rows and opened the previous context's commit. Same
contextKey still keeps the cache (no empty flash on re-entry).
- The hook's cleanup now resets both loading flags: a generation-skipped
finally never cleared them, leaving 'Show more' stuck disabled as
'Loading…' after leaving mid-page.
- commit-avatars memoizes misses only for emails an attempt actually
covered — the GitLab per-call cap (10) was recording every email past
it as permanently unresolvable for the session. Regression test added.
- GitLab host detection tightened from a bare substring to a structured
match (gitlab.com / gitlab.* / *.gitlab.*) — 'mygitlabproxy.example.com'
no longer classifies.
- Collapse-all mirror re-derives from live item state after per-file
toggles: with commit diffs seeding all-collapsed, expanding one file
left the dock button on 'Expand all', and clicking it re-collapsed the
file the user just opened.
- The commit description card element is memoized on commitInfo identity
so the measuring ResizeObserver stops churning on every context render.
* fix(ui): entering the Commits view ends any open search session (self-review)
The Cmd+F guard from the review round only blocked NEW searches in the
Commits view; a search opened in Git status/Tree survived the toggle as
hidden-but-live state — the query kept matching against the commit
diff, marks kept rendering in the all-files body, Enter/F3 kept
stepping matches, and the input to see or edit the query didn't exist
anywhere on screen. handlePanelViewSelect (the single choke point every
toggle site routes through) now clears and closes search on entry to
Commits.
Also re-traced the rest of the round under self-review — veil terminal
states (log error/empty/loaded, switch failure, background refresh with
a commit active), the hook's cleanup-before-rerun flag ordering, the
avatar attempted-set on broken platforms, and both collapse-mirror
paths — no further findings.
* fix(ui): PR-994 round 2 — live rail freshness, non-destructive errors
- The rail now keeps itself fresh: while the Commits view is visible, a
quiet 10s poll head-compares page 1 and adopts it only when history
actually moved. The commit DIFF's sha-anchored fingerprint stays as
designed (a historical commit never goes stale, so the banner
correctly stays quiet) — but the rail no longer freezes while an
agent commits in the background. Adoption bumps the fetch generation
so an in-flight 'Show more' from the old history can't append stale
rows, and transient poll failures disturb nothing.
- A page-1 fetch resets isLoadingMore: a refresh superseding an
in-flight paging request skipped that request's generation-guarded
finally, leaving 'Show more' stuck on 'Loading…' — rare before, a
live race once the poll exists.
- Errors are non-destructive when a list is on screen: a failed page or
background refresh renders as an inline row with Retry under the
list instead of replacing the whole rail; the full-panel error state
is reserved for an empty rail.
* perf/refactor(review): parallel GitLab avatar lookups; drop dead isRepoUser
- The ≤10 per-call GitLab avatar lookups run in parallel instead of
sequentially — they sit on /api/commits' critical path, and serial
subprocess spawns added seconds to the rail's first paint on
multi-author GitLab histories (PR-994 round 2 nit).
- CommitListEntry.isRepoUser removed: v1 showed the author only when it
wasn't the repo user; the live-review pivot to always-shown names left
the field computed (one git config subprocess per page), serialized,
and tested but never read, with a doc comment describing behavior the
panel no longer has. Spec updated to match the revised row design.
* fix(ui): commit-poll adoption clears all superseded fetch state (self-review)
Adopting a new history from the background poll bumps the generation,
which strands any in-flight fetch's generation-guarded finally — the
same stale-flag class just fixed for isLoadingMore, but for isLoading
(slow first load overtaken by the poll) — and a lingering inline error
from the replaced history would otherwise sit under the fresh list.
Adoption now clears isLoading, isLoadingMore, and error together.
* fix(ui)/docs: PR-994 round 3 — worktree switches drop commit diffs; API docs
- handleWorktreeSwitch treats commit:<sha> as non-portable: every other
diff mode recomputes meaningfully against the target worktree, but a
commit diff is context-bound content (worktrees share one object
database — the reviewer's claimed git error doesn't exist, verified
empirically), so 'preserving' it just re-rendered the OLD context's
commit byte-for-byte. It now falls back to the session default (same
option-availability rule resolveInitialDiffType applies). Swept every
other activeDiffBase/diffType carrier: the whitespace toggle,
staleness refresh, and fetch-base deliberately recompute the SAME
diff (valid for commits); handleBaseSelect and the base-picker row
already exclude commit mode; job diff-context labeling and the
guarded diff-type dropdown are display-only. This was the single leak.
- AGENTS.md (CLAUDE.md symlink): /api/commits row added to the Review
Server table; /api/diff and /api/diff/switch now document the
commitInfo sidecar and the commit:<sha> diffType family; the
since-main section describes the three-segment session-only toggle
and the never-persisted Commits view.
* docs: normalize arrow glyphs in the /api/commits table row (self-review)
* fix/refactor(review): PR-994 round 4 — rebase-safe paging, locale-proof ages, shared parsing
- Pagination is rebase-safe: a 'Show more' cursor from a history that was
rewritten mid-session (rebase/force-push) still resolves in the object
store but is no longer on the branch — paging from it walked the
orphaned pre-rewrite chain for the ≤10s window before the freshness
poll adopts the new history. listCommitHistory now ancestor-checks the
cursor (merge-base --is-ancestor) and returns an empty terminal page
for orphaned AND vanished cursors alike; the poll replaces the list
moments later. Regression test rewrites history between pages.
- Ages are locale-proof: git localizes %cr via gettext ('vor 2 Stunden'),
which the English-only compactAge regex silently couldn't shorten.
CommitListEntry/CommitDiffInfo now carry committedAt (epoch ms, %ct);
the rail and description card format it with the existing
formatRelativeTime — compactAge and the third formatter variant are
gone.
- The %x1f over-split repair (fixed head/tail fields, rejoined free-text
middle) was triplicated across listRecentCommits, listCommitHistory,
and getCommitDiffInfo; one splitCommitFormatFields helper (head/tail
counts, tail=0 covers the multiline-body shape) owns the edge case.
- Avatar resets its broken-image state when src changes — latent today
(every caller keys by identity), but it's a shared component and must
be safe for callers that update src in place.
Left alone from this round: hashString/hashFingerprintPart duplication is
pre-existing, and the suggested cross-package import would drag node:path
into the browser bundle — the documented reason client mirrors exist.
* fix(review): PR-994 round 5 — merge-commit prompts, boundary-aware poll, auto-select guard
- Agent prompts for commit:<sha> diffs instruct the exact on-screen
command: git diff <sha>^ <sha> (first parent), explicitly steering
away from git show — whose combined-diff presentation for a MERGE
commit renders a different (often empty) changeset than the one under
review. Root-commit fallback noted. One site covers Ask AI and every
launched reviewer (and Pi via vendoring); prompt test pins the command.
- The rail's freshness poll now adopts on boundary/base movement, not
just a new head: an agent running git fetch advances origin/<base>
while HEAD stays put, which re-partitions isPastBase — previously the
head-only compare skipped adoption and the 'In origin/main' divider
stayed stale until the view was reopened. Boundary is compared over
the page-1 overlap window (a divider paged deeper than the probe can
see re-syncs on the next full reload — accepted micro-edge). The
reviewer's proposed trigger (the Fetch banner) is unreachable in
commit mode; the external-fetch trigger is the real one.
- The HEAD auto-select never fires while any diff switch is in flight —
hardening only: the claimed first-click overwrite isn't reachable
(isLoadingDiff was never a dependency of that effect, and auto-select
settles before a human can click), but the guard makes the invariant
explicit.
* docs(adr): spec the pre-merge commits-view structure refactor
Three behavior-preserving moves scoped for PR #994 before merge, motivated
by the audit finding that every recent review-round bug lived at the
App.tsx <-> useCommitLog seam: (R1) unify the commits-session state
machine (poll, auto-select, veil) in one hook; (R2) lift the commit-rail
block out of the 1.6k-line review-core into commit-history.ts; (R3) share
the isSameCwdCommitSwitch predicate both runtimes inlined. Ownership
tables name what deliberately stays put; execution runs smallest-first
with full gates per commit.
* refactor(review): share isSameCwdCommitSwitch across runtimes (spec R3)
The ~10-line parse-and-compare predicate both /api/diff/switch handlers
inlined moves to review-core beside its inputs (parseWorktreeDiffType /
parseCommitDiffType — the canonical home for diff-type string logic);
both call sites collapse to one line. Unit tests cover plain, worktree,
cross-worktree, and non-commit-target cases. Behavior identical.
* refactor(shared): lift the commit-rail block into commit-history.ts (spec R2)
review-core.ts (1.6k lines) was absorbing the ~230-line Commits-panel
data layer it doesn't need to own: CommitListEntry/CommitHistoryPage/
listCommitHistory and CommitDiffInfo/getCommitDiffInfo move verbatim to
a new commit-history.ts, matching the package's per-concept split
(jj-core, pr-stack, commit-avatars). The commit:<sha> DIFF plumbing
(parseCommitDiffType, the runGitDiff/fingerprint/file-contents cases)
stays in review-core with the other diff types — it participates in the
dispatch; the rail does not.
Wiring: review-core exports its three parsing primitives (COMMIT_FIELD_
SEP, splitCommitFormatFields, BARE_HEX_SHA_RE — listRecentCommits still
uses them, so they can't move); package exports + vendor.sh gain the
module (vendored siblings import relatively, same as pr-provider →
pr-github); types.ts re-exports split by source; both runtimes import
from the new module. History/metadata test suites move to
commit-history.test.ts with the package-style per-file git harness;
commit-DIFF tests stay in review-core.test.ts. Behavior identical —
verified live through the compiled binary (/api/commits + commit
switch + commitInfo sidecar).
* refactor(ui): useCommitsView owns the whole commits-session machine (spec R1)
The list cache, freshness poll, HEAD auto-select, and center-dock veil
were split between useCommitLog and App.tsx — and every sync bug found
across three review rounds (stuck flags, stale auto-select, adoption
races) lived at exactly that seam. The auto-select effect and the veil
derivation move into the hook verbatim (renamed useCommitsView, git mv),
so the machine's invariants are locally checkable in one file.
App now supplies only what it owns — visibility, the active commit,
switch state, and onOpenCommit (its handleSelectCommit: the SAME path
user clicks take, so auto- and user-selection cannot diverge by
construction) — and consumes panel props plus veilActive. Net ~45 lines
of App.tsx's most delicate logic deleted.
One equivalence note: the hook's single "enabled" (showCommitsPanel &&
origin) now gates auto-select/veil where App used bare showCommitsPanel;
the two only differ when origin is unset, which implies no gitContext
(demo mode) and therefore showCommitsPanel === false — identical in all
reachable states.
Deliberately still in App (each guards a flow App owns): capability
gating, handleSelectCommit, the Cmd+F / worktree-fallback /
staleness-refresh / search-clear guards, and render wiring. Behavior
identical; spec: adr/specs/refactor-commits-view-structure-20260703.md.
|
||
|
Michael Ramos
|
795f381ebe |
feat(review): "All changes" git-status review view (#990)
* feat(review): "Since main" git-status review view + correctness fixes
Adds a composite `since-base` diff — merge-base(origin/main, HEAD) vs the
working tree, plus untracked — as the default code-review view, rendered as a
three-section "git status" panel (Committed / Changes / Untracked) with a
Sections|Tree toggle, a first-run setup chooser, and a "baseline behind GitHub"
fetch banner. Everything normalizes to one git patch, so the diff viewer,
annotations, and feedback path are unchanged.
New diff type wired through runGitDiff / fingerprint / file-content / context /
staging / agent-context in packages/shared + both server runtimes (Bun +
Pi mirror). Default flipped from `unstaged` to `since-base`; the old
DiffTypeSetupDialog is replaced by ReviewSetupDialog (view + default-diff
chooser, screenshots, Settings access).
Includes a reviewed batch of correctness fixes:
- P0: baseBehindRemote was permanently true (rev-parse missing --verify)
- fingerprint blind to quoted/unicode untracked paths (unquote)
- sections parser: record both sides of a rename; staged-delete wins over
untracked (rm --cached collision)
- graceful degrade when merge-base can't resolve (trunk/no-remote repos)
- /api/fetch-base re-queries remote (narrow-refspec honesty)
- /api/diff/switch concurrency guard (diffSwitchEpoch) + draft rekey
- decouple remote-staleness probe from initialBase (Pi parity)
- keyboard file nav in the sections view; banner gated to base-relative modes
See adr/decisions/005-since-base-github-view-default-20260701-223706.md
* fix(review): since-base review-round hardening (11 fixes, both runtimes)
Addresses the multi-agent + PR-990 review findings:
- Sections rename parser split ` -> ` on the raw quoted porcelain token, so a
filename containing ` -> ` tore into garbage and a dirty file could render as
"Committed". Now quote-aware (splitPorcelainRename + unit tests).
- /api/diff/switch captured the epoch AFTER await req.json(), letting a
slow-body older switch overwrite a newer confirmed one. Epoch now captured
before any await; hideWhitespace committed only on win. Both runtimes.
- Unresolvable base (trunk / no origin/HEAD) no longer auto-defaults to a
degraded since-base that hides committed work — getGitContext only offers
since-base when the base ref resolves, so the default falls through to
uncommitted. Fingerprint + file-content degrade to HEAD to match the diff.
- git rm --cached no longer yields two diff entries for one path (untracked
files already in the tracked patch are dropped) — fixes wrong-file-open and
j/k nav looping in the Sections panel; also fixes latent uncommitted/unstaged.
- Settings' "Default Diff View" list now preserves the sections<->since-base
coupling (can't leave an invalid sections+classic pair).
- "Behind GitHub" banner only treats origin/* as fetchable; a bare local base
("main") is upgraded to its tracking ref at startup so Fetch can clear it.
- hashUntracked resolves untracked paths against the repo toplevel, not cwd, so
a review launched from a subdirectory isn't blind to untracked edits.
- Agent review instruction now tells agents to enumerate/inspect untracked files.
- Terminology: one "Committed changes" label; the live "Since <base>" label is
dynamic (matches the header); "Since main" kept only as product copy.
- Docs: CLAUDE.md endpoints/fields + since-base view section; ADR recap.
Verified: typecheck (all projects), bun test 1793 pass, and empirical repro of
the rm --cached dedup, trunk->uncommitted fallback, and rename parser.
* fix(review): restore "(PR view)" on the Committed-changes label (unify toward it, not away)
* fix(review): close since-base coverage gaps from PR-990 review round 2
- First-run setup no longer forces since-base on repos where it isn't available
(base ref can't resolve). getGitContext omits since-base there; the first-run
block now checks the same availability before resetting the default or showing
the chooser, so committed work isn't silently hidden on trunk/no-origin repos.
- reviewBase now includes 'since-base', so changing the base while in the
git-status view passes the selected base to /api/file-content — expandable
context is fetched from the right merge-base (was falling back to default).
- Settings now mirror ReviewSetupDialog's coupling exactly: Sections ⇒ force
since-base; Tree ⇒ leave the diff (Tree + since-base is valid and now
saveable); classic diff ⇒ Tree; since-base diff ⇒ leave the view. Removes both
coercions that previously discarded a supported preference.
* fix(review): PR-990 review round 3 — fingerprint/dedup/base-canonicalization
- Fingerprint now uses `git status --porcelain -uall`, so editing a file inside
a brand-new untracked directory changes the fingerprint and the "Diff out of
date" banner fires (was collapsed to `?? dir/` and hashed as unreadable).
- extractTrackedPatchPaths pairs `---`/`+++` and excludes only the file's KEYED
path (new side, or old side for a pure deletion) — no longer drops a recreated
rename-source file (`git mv a b && touch a`). Verified rm --cached still
dedups to one entry and the recreated file still shows.
- resolveReviewBase canonicalizes a bare local default name ("main") to its
tracking ref ("origin/main") on every call, so a client that loaded before the
startup upgrade resolved can't revert the server to the stale local base on the
next refresh/switch. Both runtimes (new Pi resolveReviewBase helper).
- Removed the dead activeDiffLabel prop chain (FileTree -> DiffTypePicker).
- ADR recap: documented the staleness-banner scope limitation (default base only).
Verified: typecheck (all projects), bun test 23/23 review-core+fingerprint pass,
empirical repro of the untracked-dir fingerprint, rm --cached dedup, and
rename-recreate cases.
* fix(review): PR-990 review round 4 — dedup content, staging gate, banner timing
- Dedup rewrite: instead of dropping the untracked side of a same-path collision
(which hid content), drop the tracked DELETION block and keep the untracked
working-tree content. `git rm --cached f` + edit now shows the new content, not
a phantom deletion — still one entry per path (no dock/nav collision). New
stripHeaderPath also strips git's trailing-tab metadata, so space-named files
dedup correctly. Verified: rm --cached+modify shows content (1 entry),
space-named (1 entry), rename+recreate still shows both.
- All-files (and the `a` shortcut on the focused file) now gate staging per-file:
committed files in since-base mode are not stageable, matching SectionsPanel /
FileTreeNode. Shared isPathStageable helper threaded via ReviewStateContext →
AllFilesCodeView. Stops the confusing `git add` no-op that still flipped local
staged/viewed state.
- /api/diff/switch now awaits recomputeBaseBehindRemote before building the
response, so the "Baseline behind GitHub" banner reflects the new base
immediately (no ~5s lag switching in, no stale banner switching away). Both
runtimes.
- ReviewSetupDialog re-applies the recommended default only on first-run dismiss,
not when reopened from the header menu (was snapping a mid-session diff back).
- SectionsPanel "N added" header now counts sidecar-staged files too, so it
matches the staged dots on rows.
Verified: typecheck (all projects), bun test 1830 pass, empirical dedup repro,
both bundles build.
* refactor(review): reuse parsePatchPathToken in removeTrackedDeletions (self-review)
Drop the duplicated stripHeaderPath helper — the shared diff-paths
parsePatchPathToken already strips a/|b/ prefix + C-quoting + git trailing-tab,
and verifies the prefix instead of blindly slicing two chars. Documents the
binary-deletion edge (no --- line, so a binary rm --cached is not deduped).
* fix(review): PR-990 review round 5 — mixed-base + staging/sort/flicker nits
- Atomic base upgrade: the startup origin/* canonicalization swapped currentBase
without rebuilding the patch, so /api/diff could advertise origin/main while
the served hunks came from local main (mixed-base review on origin/HEAD-absent
repos). Now rebuilds the diff for the new base and commits base+patch+ref+
fingerprint together (only if no user switch happened); the fingerprint change
makes the client's freshness poll pick it up. Both runtimes.
- isPathStageable: gate staging OFF when since-base is active but the sidecar
hasn't loaded (was falling through to true, allowing a git-add no-op on a
committed file).
- SectionsPanel: session-staged files now float to the top of Changes — the sort
key was `false ?? stagedFiles.has(...)` which short-circuits; now ORs them.
- /api/diff/fresh: early returns now carry baseBehindRemote, so a snapshot change
mid-probe no longer clears the "behind GitHub" banner for one poll. Both runtimes.
- Removed the now-dead activeLabelFallback prop from DiffTypePicker (FileTree
stopped forwarding it in round 3).
Skipped (agreed): the fetch-base input-validation nit (no realistic attacker)
and the diff-switch TOCTOU (concurrent switches aren't UI-reachable).
Verified: typecheck (all projects), bun test 1830 pass, both bundles build.
* refactor(review): drop redundant staged-OR in SectionsRow (self-review)
item.staged (the grouping key) already ORs in stagedFiles as of the round-5
sort fix, so the row-level `|| stagedFiles.has(...)` is dead weight.
* fix(review): surface the startup base upgrade to already-loaded clients
Review round 6 fixes:
- The startup main -> origin/main upgrade re-baselined the freshness
fingerprint, so a client that fetched /api/diff before the rebuild kept
the old patch and every /api/diff/fresh probe reported fresh (the probe
compares server state to itself, never to what the client renders). Now
the fingerprint is only re-baselined when no client has loaded the
pre-upgrade snapshot; otherwise the stale baseline trips the normal
"Diff out of date - Refresh" banner. Bun + Pi.
- preserveFile refreshes (staleness Refresh, post-Fetch) now adopt the
server's returned base — exactly the paths where the server may have
canonicalized main -> origin/main; keeping the old name sent
/api/file-content and Ask AI context against the wrong base.
- ReviewSetupDialog: clamp the fixed 800px height to the viewport so the
dialog fits on small laptop screens.
- Docs: /api/diff/fresh response also carries baseBehindRemote/agentCwd.
- Documented the committed-deletion + untracked-recreation dedupe edge as
accepted (code comment + ADR) — fixing it needs two same-path diff
entries, which the path-keyed UI cannot represent.
* fix(review): stop the Git-status default from silently reverting to tree
Users with reviewPanelView=sections could keep opening in the tree view on
a stale diff type despite their cookie saying Git status. Three causes:
- The header Sections toggle persisted the view but not defaultDiffType,
creating a conflicted pair (sections + non-since-base default) that every
UI writer is supposed to prevent. It now couples the diff default like
the setup dialog and Settings do.
- configStore's debounced POST /api/config could be lost when a session
closed within 300ms of a change, leaving cookie and config.json split.
Pending writes now flush on pagehide with a keepalive fetch.
- configStore.init() applies config.json over the cookie without the UI
coupling, so a stale server value re-corrupted the pair on every load.
The app now self-heals at load: if the view says sections but the diff
default isn't since-base, it repairs the default (cookie + config.json)
and switches the live session to since-base.
* fix(ui): keep the stage (+) button border visible on the active file row
The button's --border border has no contrast against the active row's 30%
primary tint, so it vanished on the selected row until hovered. Tint the
border with the row's primary color on active rows (both sections and tree
views); the button's own hover border still applies.
* ui(review): spell out the panel view toggle — 'Git status | Tree' text instead of icons
* fix(review): PR-990 review round 7 — export label, diff snapshot race, unicode paths
- exportFeedback describeDiff(): add the missing since-base case — every
feedback export in the new default mode read "**Diff:** since-base".
- /api/diff GET: snapshot patch/base/ref/error BEFORE the sections-sidecar
await and pass the pinned base into buildSectionsSidecar. The startup base
upgrade landing mid-await could pair a rebuilt patch with sections grouped
against the old base, with initialDiffServed still false so no refresh
banner ever came. Bun + Pi.
- SectionsPanel: a session-staged untracked file now moves to the Changes
section immediately (anticipating the server's next sidecar) instead of
sitting in Untracked with a staged dot until refresh.
- unquoteGitPath: real C-style unquoting with octal (UTF-8 byte) escape
decoding. JSON.parse rejects octal escapes, so non-ASCII names kept their
literal \303\251 form — an untracked "café.txt" was silently ABSENT from
the review (git diff --no-index could not access the quoted name) and the
deletion dedupe Set lookup could never match. getUntrackedFileDiffs now
unquotes ls-files output. Unit tests: octal decoding + end-to-end unicode
untracked file in a real repo.
- review-core.test: worktree subtype round-trip now covers since-base and
all (was 6 of 8).
Parked (deliberate): SectionsPanel/FileTree keyboard-nav dedup refactor.
* fix(shared): unquoteGitPath keeps literal unicode intact (self-review)
The byte-collector treated literal non-ASCII code units as single bytes,
which would mojibake headers synthesized by our own quoteGitPath
(JSON.stringify leaves unicode unescaped inside quotes — workspace-mode
prefixed headers round-trip through the same parser). Literal chars now
append as string code units (surrogate-safe); only octal escapes go
through the UTF-8 byte decoder.
* fix(review): PR-990 review round 8 — pre-staged toggles, explicit base, AI context race
- useGitAdd: session Set replaced with a tri-state override map folded over
the sections sidecar; stagedFiles is now the EFFECTIVE staged set (sidecar
+ session stages - session unstages). Fixes pre-staged files: the first
`a` press actually unstages (was a git-add no-op), the sidebar dot clears
after a real unstage (was stuck via sidecar OR), and the All-files header
agrees with the sidebar on load. Overrides reset whenever a fresh sidecar
arrives (switch, preserveFile refresh, PR response) so stale session
intent can't fight new porcelain truth. SectionsPanel drops its own
sidecar OR; stagedCount = effective size.
- Explicit base picks are honored verbatim: the picker sends explicitBase,
and the server permanently disables local-name -> origin/* canonicalization
once set (the local/remote groups are distinct choices). The behind-GitHub
banner also exempts an explicitly-picked local name — Fetch advances
origin/*, so the banner would be un-clearable nagging. Bun + Pi.
- buildCurrentAiReviewContext(patch, base): GET /api/diff builds Ask AI
context from the same served snapshot as the patch — the startup base
upgrade could hand Ask AI a different changeset than the screen. Bun + Pi.
- recomputeBaseBehindRemote: capture remoteDefaultInfo once — a concurrent
refresh nulling it mid-await threw. Bun + Pi.
- Revert stray "Status Update / Updated!" edit to adr/0001 (test debris
swept into the round-2 commit).
- splitPorcelainRename comment corrected (porcelain v1 does NOT quote plain
spaces; a name containing " -> " is ambiguous without -z) + ADR notes for
the index-only-changes semantics and the rename edge.
* fix(review): FileTreeNode uses the effective staged set — round 9
Round 8 made stagedFiles the effective set (sidecar + session overrides)
and removed SectionsPanel's sidecar OR, but missed the same OR in
FileTreeNode's since-base row (sectionStaged). In the Tree fallback a
pre-staged file unstaged this session kept its dot and the next toggle
re-staged it. Grep-swept: this was the last surviving sidecar-staged OR.
* refactor(review): make the staged-display invariant unrepresentable
The round-8/9 bug class (sidecar staged flag ORed over the effective set)
existed because surfaces had a second staging source to reach for. Remove
it: stagedFiles is now a REQUIRED prop on SectionsPanel/FileTree/
FileTreeNode and the optional-prop fallback branches reading
sectionEntry.staged for display are deleted — a future surface cannot
reintroduce the OR because the pattern no longer exists to copy. The
sidecar type's staged field and AGENTS.md now document the invariant at
the point of temptation. Grep for display reads of .staged now hits only
useGitAdd (owner) and the sidecar builder (producer).
* refactor(review): self-review cleanups on the staged-invariant hardening
- orderFilesBySections: document why its snapshot .staged read is safe
(only called at sidecar-fresh moments where snapshot = effective) and
that it must not be reused mid-session — the one remaining display-side
snapshot read the invariant sweep surfaced.
- FileTreeNode: drop the now-pure sectionStaged alias; use isStaged.
* fix(review): PR-990 review round 10 — header staging gate, fingerprint cap, single-writer coupling
- Single-file diff header now uses the per-path staging gate (canStagePath),
closing the last ungated staging trigger: committed-only files in
since-base offered a no-op Git Add that flipped local staged/viewed state.
Full trigger inventory swept: App `a` shortcut, all-files `a` + header,
SectionsPanel rows, FileTreeNode rows, single-file header — all six now
per-path gated or group-gated; no context-menu staging exists.
- Fingerprint circuit-breaker: the freshness poll's `git status --porcelain
-uall` degrades permanently (per cwd, per process) to collapsed -unormal
once its output exceeds 2MB — a forgotten node_modules/ no longer burns
CPU every 5s for the whole session. Costs untracked-dir edit sensitivity
only on such repos; one possibly-spurious staleness banner at the switch.
- The sections ⟺ since-base coupling now has a single writer:
setReviewPanelView/setReviewDefaultDiffType in @plannotator/ui/config.
All five call sites (setup dialog x2, Settings x2, header toggle,
first-run reset, self-heal) converted; grep for direct writes of either
setting now hits only reviewView.ts.
- /api/diff/switch responses pass snapshot args to the AI context builder
(both branches, Bun + Pi) — correct today, now robust against future
awaits between the epoch check and the response.
- Docs: explicitBase in the /api/diff/switch body.
Parked per discussion: SectionsPanel/FileTree nav+search+footer dedup
(extract when the commit-list view adds a third panel), querySelector row
measurement, fetch-base stderr passthrough (standing decision), Pi
hasAgentLocalAccess (pre-existing follow-up list).
* fix(review): PR-990 review round 11 — subdirectory launches, escape decode, settings note
- Repo-root-relative patch paths now resolve against the git toplevel
everywhere they meet a filesystem path or pathspec, via a shared
resolveRepoToplevel helper: file-content working-tree reads (hunk
expansion returned null from a subdirectory launch) and gitAddFile/
gitResetFile (stage/unstage failed with pathspec errors). Both bugs
pre-existed for uncommitted/unstaged; since-base made them the default
experience. The two existing inline toplevel resolutions (untracked
diffs, fingerprint) now use the same helper. Shared code — Pi inherits
via vendoring. Real-repo subdirectory tests for both.
- unquoteGitPath decodes \uXXXX (JSON.stringify emits it for control
chars without a short escape; our synthesized workspace headers
round-trip through this decoder). Malformed \u stays literal. Tests.
- Pi explicitBase guard matches Bun byte-for-byte on empty-string base.
- Settings Git tab notes when the CURRENT repo can't serve the Git-status
view (base ref unresolvable) instead of letting the preference look
silently broken — it's a global preference, so the options stay.
Same-class items left parked (pre-existing, untouched by this PR):
open-in root resolution and code-nav file reads from subdirectory
launches — on the follow-up list with the Pi divergences.
* fix(review): PR-990 review round 12 — per-client freshness, keyboard-operable row controls
- Freshness is now judged PER CLIENT: every patch-carrying response
(/api/diff, /api/diff/switch, pr-switch, pr-diff-scope) includes
snapshotId (the server's draftKey), and the client echoes it on
/api/diff/fresh probes. A mismatch reports stale for THAT client
regardless of the VCS fingerprint. This fixes the round-12 finding —
reloads/second tabs after the startup base upgrade got a permanently
bogus staleness banner from the shared pre-upgrade baseline — and
deletes the round-6 conditional re-baseline hack entirely (the
fingerprint recaptures unconditionally again; the old client's banner
now comes from its snapshot mismatch, not a deliberately stale
baseline). Also gives unfingerprintable modes (P4, PR layer) snapshot-
level staleness for free. Bun + Pi + client hook.
- StageControl and ViewedControl (which had the identical gap) are
keyboard-operable: tabIndex + Enter/Space activation + focus outline.
They're spans inside the row <button> (nested real buttons are invalid
HTML), so they need their own focus stop; the a/v shortcuts remain the
power path.
- Docs: snapshotId on /api/diff, ?snapshot= on /api/diff/fresh.
* fix(review): PR-990 review round 13 — re-key snapshots on scope switch, PR-tab refresh
- The PR scope switch now re-keys draftKey (= snapshotId + draft storage
key) at BOTH commit points, unconditionally — matching every other
snapshot commit site. The full-stack branch previously kept the layer
patch's key: stale layer tabs never got the banner after a cross-tab
scope switch (the exact case snapshotId exists for), and full-stack
drafts collided with layer drafts (pre-existing). The layer branch's
!layerPatchIncomplete conditional is gone — it only stayed consistent
because full-stack never re-keyed. Invariant now: every currentPatch
commit is followed by a re-key. Bun + Pi.
- Refresh works for any stale PR tab: re-selects the CURRENT scope
instead of no-opping for layer (only full-stack could go stale in the
fingerprint-only world; snapshot mismatch changed that). Accepted
residual (documented in code): after a cross-tab PR switch, refresh
updates the patch but not prMetadata — the scope endpoint doesn't
carry it, and the full-rehydrate refactor isn't worth the two-tab edge.
* fix(review): stale incomplete-layer Refresh uses the non-blocking upgrade path (self-review)
Round 13 made Refresh re-select the current scope for stale PR tabs; for
an INCOMPLETE layer patch that POST triggers the server's local recompute,
which can park for minutes behind checkout warmup — and
handlePRDiffScopeSelect renders the full-screen switch overlay the whole
time. Route that case through handleLoadFullDiff (same POST, progress
notice instead of modal), which exists for exactly this slow path.
* fix(review): round 14 — composite snapshot id, fully-pinned /api/diff, "All changes" label
- snapshotId is now content hash + diff type (+ PR scope), built by a
single currentSnapshotId() helper used at every response site and the
freshness compare. A cross-tab MODE switch with a byte-identical patch
(layer vs full-stack on a single-PR stack) now flags old tabs; the base
is deliberately excluded so a same-commit main -> origin/main
canonicalization stays banner-silent (round-12 noise-avoidance kept).
draftKey stays a pure content hash — drafts survive content-identical
round-trips. Bun + Pi.
- GET /api/diff pins ALL served fields (diffType, hideWhitespace,
prDiffScope join patch/base/ref/error/snapshotId) and the sidecar + AI
context builders take the pinned type instead of reading globals — a
concurrent tab switch during the sidecar await can no longer produce a
since-base patch labeled with another mode. Bun + Pi.
- First-user feedback: the since-base label is now plain English —
"All changes since origin/main" (dynamic, follows the picked base);
dialog/Settings short form "All changes"; feedback exports match.
uncommitted reverts to "Uncommitted" where it had borrowed "All
changes", so the two stay distinguishable side by side.
* docs(review): finish the Since-main -> All-changes terminology sweep in comments (self-review)
* fix(review): PR-990 review round 15 — base-revert race, freshness reset, unstage regroup
- resolveReviewBase gains a second, probe-independent rule: a non-explicit
echo of the bare local name of the CURRENT origin/* base stays on the
tracking ref. The existing rule keys off remoteDefaultInfo, which comes
from a second network probe that can lag the startup upgrade by seconds;
in that window a diff-type/whitespace switch echoing "main" committed
the session back onto the stale local branch and set baseEverSwitched,
permanently blocking the upgrade. Bun + Pi.
- useDiffFreshness resets stale/dismissed state on snapshotId too — since
round 14 a new snapshot can reuse identical patch text with a different
id, and the old banner state wrongly carried over until the next poll.
- SectionsPanel: unstaging a PRE-staged add moves it back to Untracked
(mirror of the round-7 stage regroup). Detection deliberately uses the
sidecar's snapshot flag to recognize "was pre-staged"; staged
modifications stay in Changes, staged renames remain a refresh-heals
edge.
* fix(review): PR-990 review round 16 — rename staged count, guarded fetch-base replay
- "N added" no longer double-counts staged renames: the sidecar truthfully
marks BOTH porcelain sides staged, but an above-threshold rename renders
as ONE file — the hidden old path inflated the count and left a phantom
effective-staged entry that unstaging the visible row couldn't clear.
The client now filters sidecar-staged paths to rendered files, which is
correct in both patch shapes (below-threshold renames render delete+add
as two rows and both sides pass the filter). Client-only; the sidecar
stays faithful to porcelain.
- The fetch-base completion only replays the diff refresh if the user's
diff type/base selection is unchanged since the click — a slow fetch no
longer yanks the review back to the view captured at click time.
Declined with reasons (in review thread): first-run persist-before-confirm
(approved forced default, second flagging), explicit-base flag ordering
(early-set preserves user intent; commit-after-win would leave picker and
server disagreeing), base-picker revert target (near-unreachable compound
race, deferred).
* fix(review): PR-990 review round 17 — find origin/main on feature-only clones
getDefaultBranch's chain (origin/HEAD -> local main -> blind "master")
skipped the fetched remote-tracking ref entirely. On checkouts with no
origin/HEAD symref and no local main/master — CI checkouts, `clone
--branch feature`, extra worktrees — it guessed "master", the base
didn't resolve, getGitContext suppressed since-base, and the flagship
"All changes" view silently disabled itself for the whole session even
though origin/main was fetched and diffable. The startup upgrade can't
rescue it either (its guard reads the bogus "master" as a deliberate
non-default base).
Chain is now: origin/HEAD (verified) -> origin/main -> local main ->
origin/master -> "master" — remote-tracking refs preferred, matching the
function's stated prefer-upstream intent. Shared core (Pi inherits);
real-repo test reproducing the exact clone shape.
|
||
|
Michael Ramos
|
e3de938914 |
feat(review): PR Overview panel + description/comment annotations + media (#981)
Combine the PR Summary/Comments/Checks tabs into one PR Overview panel, then make the description and comments annotatable and render their media. - PR Overview panel (one sidebar entry) + comment UI (avatars, filters, hide bots, live context, responsive stacking). - Annotate the PR description (select → comment) and PR comments (Annotate button), with Ask AI; notes show in the Annotations sidebar and ship to the agent. - Split/Unified diff toggle relocated into the dock tab strip. - Render images + video in descriptions and comments (raw HTML + markdown), capped to the card so nothing bleeds. - Review-flow fixes: copy-all feedback, prose-only feedback preamble, no image control on prose notes, GitHub review-body seeding; stronger review trailer. - Add Claude Sonnet 5 as the default Ask AI model. No server, endpoint, or Pi-runtime changes. |
||
|
Michael Ramos
|
cb6667e991 |
fix(ai): drive Codex Ask AI via codex app-server (#971)
* fix(ai): drive Codex Ask AI via app-server + answer-first review prompts (#971) Codex Ask AI previously ran via @openai/codex-sdk (codex exec), which forces approval_policy=never and breaks in enterprise-managed Codex environments that ban it (#971). Replace the transport with a long-lived 'codex app-server' process over JSON-RPC. - New provider packages/ai/providers/codex-app-server.ts (registered as 'codex-sdk' to preserve cookie/agents.ts/UI-gate); omits approvalPolicy so Codex resolves the user's + managed policy, pins read-only sandbox, and surfaces interactive approvals through the existing PermissionCard. - Delete codex-sdk.ts and drop the @openai/codex-sdk dependency (and its 6 prebuilt platform binaries); gate registration on 'which codex'. - SessionManager: additive, optional dispose?() hook to kill the spawned process on evict/remove — a no-op for Claude/OpenCode/Pi (they don't implement it). Also rework the Ask AI prompts (all providers, separate from the transport): - Every mode now instructs the agent to answer the user's message directly and not launch an unprompted review of the context. - Code review stops pasting the whole diff for git-reproducible diff types and instead tells the agent how to inspect it (git diff <base>..HEAD, three-dot for merge-base); non-git/PR/workspace types still paste. - Claude gains the Bash tool so it can run git (still gated by approvals). - The UI passes diffType/base (session) and what the user is viewing (per question) into the context. Verified: full typecheck, full test suite (101 ai tests), and a live end-to-end smoke against codex app-server. * fix(review): pin AI approval card above the input/model bar Render pending approval cards just above the input + provider/model bar in both the document chat (DocumentAIChatPanel) and code-review AI tab, instead of at the top of the scroll, so the user sees them where they act. * fix(ai): harden Codex abort/cancel + add Ask AI Stop button Addresses code-review findings on the codex app-server provider: - turn/interrupt was sent as a notification (no id) so Codex ignored it and abort never took effect. It's now a proper JSON-RPC request. - Filter turn events/approvals by turnId and reject an aborted turn's stragglers, so a stopped turn can no longer leak output into — or prematurely finish — the next turn (ask-stop-ask race). - Guard listeners by query generation and end the drain loop on the abort signal, so a superseded/stopped turn can't touch the live one and abort returns promptly instead of waiting for turn/completed. - Handle abort during startup: once the turn id is known, interrupt it instead of running it in the background. - Add a sendAndWait timeout so a stalled (alive-but-unresponsive) process errors instead of hanging forever. - Drain stderr (stdio 'ignore') to avoid a pipe-buffer deadlock. Also add a Stop button to both Ask AI surfaces (plan/annotate DocumentAIChatPanel and code-review AITab via ReviewSidebar). It replaces Send while streaming and calls the hook's abort -> /api/ai/abort -> session.abort(); the hook already exposed abort but nothing surfaced it. * feat(ai): drive Codex models + reasoning levels from model/list - Codex provider fetches model/list at startup (throwaway app-server, like Pi/OpenCode) and populates the real models plus each model's actual supportedReasoningEfforts + defaultReasoningEffort. Replaces the hardcoded model list and the static AI_REASONING_EFFORTS (which mislabeled xhigh as 'Max' and omitted minimal). - AIProviderBar + AIConfigBar now show the selected model's real efforts and hide the control when a model reports none. xhigh is shown verbatim. - Fix: the Stop button now also appears in the populated code-review chat state (a prior edit missed the second GeneralInput due to indentation). * fix(ai): scope Claude Ask AI Bash to read-only git; clear stale approval cards on Stop - Claude Ask AI no longer auto-allows bare Bash (which ran arbitrary shell with no Allow/Deny prompt). Replace it with scoped read-only git rules (Bash(git diff:*), show, log, status, rev-parse, merge-base, ls-files). git reads auto-run so the agent can inspect large diffs itself; any other command (write git, arbitrary, or injected compound) falls through to the permission card. Keeps the git-inspect approach (large diffs don't fit in the prompt) while closing the auto-exec hole. - useAIChat.abort() now drops still-undecided permission cards: abort cancels them server-side, so leaving them visible was a dead Allow/Deny. * feat(ai): code-review Ask AI shares the agent-review prompt machine, delivered as user messages Code-review Ask AI built its own diff description (gitInspectInstruction) in the system prompt from just diffType+base, which was wrong for full-stack, hide-whitespace, untracked files, and PR worktrees. Replace it with the same machine the review jobs use, delivered on the user's messages. - Server: buildCurrentAiReviewContext() reuses buildAgentReviewUserMessageForTarget (contextOnly) for the current view and ships it as aiReviewContext in every diff payload (/api/diff + switch/PR/scope). Mirrored in the Pi server. - Client: review-editor latches aiReviewContext onto each question via the pure buildReviewContextPreamble (packages/ui/utils/aiPrompt.ts) and buildDefaultPrompt — full block on the first message / when the view changes (incl. after a provider switch via the !sessionId fresh-session check), a short reminder otherwise (never re-pastes a large diff). - context.ts: delete the duplicate gitInspectInstruction; code-review system prompt is now role-only. Providers untouched (provider-agnostic user message). - Tests: machine scenario gaps (plain PR, full-stack default, PR-worktree origin/<base> + stale-main warning, untracked mention, jj-evolog, workspace lines); composition (first/reminder, command/pasted, preamble ordering). * fix(ai): agentic remote-PR context, UTF-8 stream decode, real Stop on supersede/disconnect Addresses review findings on the Ask AI prompt work: - Remote PR without a confirmed local checkout no longer gets URL-only. The agent is told it's in a PR worktree that's being prepared, to verify the PR files exist before relying on them, and to diff with git diff origin/<base>... HEAD (URL fallback). Inform + trust the agent rather than pasting. Shared machine, so review jobs get the same framing. - Decode Codex stdout with a streaming TextDecoder instead of per-chunk toString(), so multi-byte UTF-8 split across chunks no longer corrupts into U+FFFD (matches the Pi provider). - Stop now actually stops the server turn: ask() awaits /api/ai/abort when a new question supersedes a streaming one (awaiting avoids racing the new query into session_busy), and the /api/ai/query SSE stream gains a cancel handler so tab-close/navigation aborts the turn too. Both reuse the existing per-provider session.abort(); factored a shared postServerAbort helper. * fix(ai): per-model reasoning effort, shared PR-checkout readiness, Stop-then-ask race, type hole - Reasoning effort is now tracked per model (a map keyed by model) instead of one global value, so switching to a model that doesn't support the prior level (e.g. xhigh) no longer posts a stale/unsupported effort that Codex rejects. Each model keeps its own level; nothing leaks across. - Extract resolvePoolCwd into packages/shared/worktree-pool.ts (ready/pending/ absent) and use it from both servers' resolvePRLocalCwd so the readiness rule can't drift. Fix the Pi Ask AI helper to ready-check like Bun, so a warming PR checkout no longer claims 'checked out at PR head' and misdirects the diff. - Stop-then-ask no longer races into session_busy: the abort promise (from Stop or a superseding question) is stashed and the next ask() awaits it before sending. Stop still kills the turn instantly; a follow-up just waits the one abort round-trip. - Declare aiReviewContext on the initial /api/diff response type. * fix(ai): surface real Codex error messages, skip transient retries, auto-deny permission escalations Validated against codex-rs: - Codex's ErrorNotification nests the text at params.error.message (TurnError.message); we read params.message → always 'Unknown error'. Read the nested field (top-level fallback) so auth/usage-limit/stream failures show their real, actionable text. - Skip transient error notifications (willRetry=true): Codex retries on its own and the turn continues, so surfacing them flashed a spurious failure before the real answer. - Handle item/permissions/requestApproval: respond {permissions:{}, scope:'turn'} — byte-for-byte Codex's own cancel response (codex_delegate.rs) — instead of 'Unsupported request'. Ask AI is read-only, so denying the escalation lets the turn continue sandboxed rather than failing. Interactive grant deferred. * refactor(ai): share AI provider/model config in one hook; aggregate paginated model/list - Extract useAIProviderConfig (packages/ui/hooks): one home for provider/model/ reasoning-effort selection — initial state, auto-resolve on capabilities load, per-model effort (no leak across models), and persistence. Both the plan and code-review apps now call it and only compose the session reset (the hook can't own reset without a cycle through useAIChat). Plan editor adapted to the effect-based resolve (adds aiDefaultProvider state). This fixes the plan editor's stale-effort-on-model-switch bug by construction and stops the two apps' copies from drifting again. - fetchModels now follows model/list's nextCursor and aggregates every page into one list, so larger model catalogs aren't silently truncated (with a page guard against a misbehaving cursor). Skipped per discussion: legacy v1 approval methods (we're a v2 client). * feat(agents): per-agent review default; bound Codex model discovery so it can't stall the AI panel - The selected review profile is now tracked per review engine (claude/codex/ cursor/opencode) instead of one flat global value, so each agent keeps its own review default. Public hook API (reviewProfileId/setReviewProfileId) is unchanged — getter derives the current engine's value, setter writes it — so AgentsTab needs no changes. One-shot migration seeds every engine with any existing flat pick. Adds parseReviewProfileByEngine + tests. - fetchModels (Codex model discovery) now uses a short 6s timeout for its initialize + model/list RPCs instead of the 30s default. /api/ai/capabilities awaits model discovery, so an installed-but-unauthenticated codex could otherwise block the AI panel for ~30s; it now falls back to the static model list fast. Authed codex is unaffected (discovery completes in tens of ms). |
||
|
Michael Ramos
|
e138c824aa |
feat(review): Cursor + OpenCode review engines (unified marker-review) (#959)
Add Cursor CLI and OpenCode as background code-review engines alongside Claude/Codex/Tour, unified behind a shared marker-review module (prose marker-block parsing, since neither CLI exposes a schema flag). Includes multi-provider dynamic model discovery, review-profile support, fail-closed parse/ingest, Bun+Pi parity, and the review UI (Provider picker, brand icons). Hardening from review: per-job nonce on marker tags (prevents echoed/quoted tags from corrupting extraction), truncation fail-closed, async model discovery (never blocks the event loop), nested-slash model ids, shared finding types. |
||
|
Michael Ramos
|
affeaa07fc |
feat(review): custom reviews as Agent Skills + whole-file/general findings (#955)
Custom reviews: pick an Agent Skill and its SKILL.md body becomes the review prompt (read live from global skill folders, never copied; default review byte-identical). Agent findings can be a line, a whole file, or a general review-level comment, and are never silently dropped. Plus: agent-icon engine picker, icon-only file-header opener, semantic diff restored as its own dock panel, and a dead-code sweep. |
||
|
Michael Ramos
|
740d6fb2eb |
Add WebTUI agent panel to annotate mode (#941)
* feat(annotate): add WebTUI agent terminal * feat(annotate): wire WebTUI agent into annotate UI * docs: recap annotate agent terminal work * fix(annotate): harden agent terminal runtime * docs: add annotate agent terminal runtime ADRs * fix(annotate): polish agent terminal integration * fix(ui): preserve comment draft on Ask AI failure * fix(annotate): address terminal review findings * fix(annotate): harden agent terminal runtime fallback |
||
|
Michael Ramos
|
2a6fe5c457 |
Open files in external apps + code-review UX pass (#942)
Adds a server-side "open the current file in an external app" control to the code-review and annotate surfaces (split-button with host-detected apps; last-used becomes the default; cross-platform launch mirrored across the Bun and Pi runtimes), plus a code-review UX pass: file-header change letters and line counts, the diff-settings cog grouped with Split/Unified, an all-files collapse/expand-all toggle, and the semantic diff moved to a resizable sidebar accordion. Also fixes Cmd+click code navigation (worker token transformer). Hardened over three adversarial review rounds: shared, tested path-containment for open-in (Bun + Pi), annotate scoping aligned to /api/doc reference roots, strict PR-checkout rooting (never the launch repo), deduped app-catalog and semantic-diff fetches, macOS bundle-only availability, Windows launch fixes (reveal + terminal off cmd's parser), and PR-checkout tracking across switch and pool warmup. |
||
|
Michael Ramos
|
195328f7a6 |
Persist saved annotate file edits in drafts (#936)
* Persist saved annotate file edits in drafts * Handle stale saved file edit context * Test source edit conflict actions * Return source metadata for single-file docs * Fix saved file edit conflict races * Handle deleted source files in annotate edits * Tighten annotate missing-file recovery * Reset edit state for missing file reopen * Tighten source edit restore path handling * Harden source edit recovery paths * Harden source edit disk reconciliation * Fix live file tree startup delay * Document file tree watcher startup ordering * Preserve source save through missing files and symlinks * Harden missing source file recovery * Tighten annotate source edit boundaries |
||
|
Michael Ramos
|
1fcfad80c8 | Polish live file tree refresh | ||
|
Michael Ramos
|
40210fdfd2 |
Add live annotate file tree workspace status (#931)
* feat(annotate): add live file tree workspace status * fix(annotate): guard dirty feedback and rename stats * fix(annotate): tighten live file tree status * fix(annotate): surface deleted file browser roots * fix(annotate): normalize file tree status paths * fix(annotate): avoid optional git locks for workspace status * fix(annotate): tighten workspace status git metadata * fix(annotate): refresh file tree after reconnect * fix(pi): expose file browser stream route |
||
|
Michael Ramos
|
9c77c2f812 |
fix(server): always show remote URL + close share-html symlink gap (#929)
* fix(server): always show remote URL + close share-html symlink gap
Two pre-release fixes found by the QA sweep.
1. Remote URL stranding: handleServerReady only opened a browser (silent on
a headless box) and wrote a ready-file; the user-visible URL came solely
from writeRemoteShareLink, gated on sharingEnabled. So a remote/SSH user
with sharing disabled (PLANNOTATOR_SHARE=disabled or config.json
{share:"disabled"}, widened by #921) saw no URL and the agent hung on
waitForDecision() forever. Now handleServerReady prints the reachable
localhost URL whenever the session is remote, independent of sharing; the
share link stays an extra. Pi/OpenCode already printed the URL
unconditionally and are unaffected.
2. share-html symlink gap: the #927 symlink-containment fix hardened the
/api/html-assets asset sinks but missed packages/server/annotate.ts's
/api/share-html containment, which stayed lexical. A symlinked *.html
inside the doc directory pointing outside it leaked the target's contents
into the share payload. Now realpath-resolves both root and target like
the other sinks. Bun-only — Pi's copy was already hardened.
Adds regression tests for both (handleServerReady remote stderr; share-html
returns 403 on symlink escape).
* refactor(server): single shared isWithinDirectory for all asset/share sinks
The symlink-containment check existed as four byte-identical copies (Bun
html-assets route, share inliner, annotate /api/share-html, Pi server). That
duplication is exactly why the escape was missed in one sink before — #927
hardened three and missed the fourth, and the #929 fix added a fifth-in-waiting.
Export the single canonical isWithinDirectory from the shared (Pi-vendored)
html-assets-node module and have every sink import it; delete the three
duplicate bodies and their now-dead realpathSync/relative/isAbsolute imports.
A new sink can no longer silently diverge. Regression tests + 690 server/shared
tests pass; build:pi clean.
* fix(pi): surface the session URL in the in-turn 'opened' notice for remote
Remote Pi users never saw a review/annotate/last URL: it was emitted from
openBrowserForServer AFTER the command's turn ended (fire-and-forget), and
Pi only renders a notify during an active turn — so it silently dropped.
The 'X opened. You can keep chatting' notice fires in-turn and DOES show,
so fold the URL into it for remote sessions (single-line, matching the
notify convention). Local sessions are unchanged (browser auto-opens).
|
||
|
Michael Ramos
|
f564448650 |
fix(annotate): block symlink escape in HTML asset serving + cleanups (#927)
* fix(annotate): resolve symlinks before HTML asset containment check The /api/html-assets route and the share-payload inliner checked path containment lexically, so an in-directory symlink pointing outside the HTML's folder (e.g. evil.css -> ~/.ssh/id_rsa) passed the check and was served or base64-inlined into the share payload. In remote mode the inliner auto-fires at startup, so this could upload symlinked local files to the paste service with no user action. Resolve symlinks with realpathSync on both the asset and the root before the relative-path check, in all three runtime copies (Bun route handler, shared node inliner, Pi route handler). Non-existent assets fall back to the lexical path and 404 on read. Adds regression tests for both sinks. * test(pi): build rich git state in sandbox-pi.sh for review diff modes Expand the Pi sandbox harness to create multiple commits, a feature branch, and a rename+delete+modify commit so /plannotator-review can exercise every diff mode (uncommitted, staged, branch, merge-base). * docs: fix broken verification link in READMEs The READMEs pointed at a non-existent anchor (installation/#verifying-your-install); the verification guide is a standalone reference page. Point to /docs/reference/verifying-your-install/ (and split the hook README's link so version pinning -> installation, verification -> the reference page). |
||
|
Yoni Hornstein
|
201ca11ec4 |
feat(config): support share toggle via config.json (#921)
Allow disabling URL sharing through ~/.plannotator/config.json
({ "share": "disabled" }) in addition to the PLANNOTATOR_SHARE
env var. Adds a resolveSharingEnabled() helper (env var > config >
default enabled) and routes all sharing checks through it across the
hook server, OpenCode plugin, and Pi extension. Docs updated.
|
||
|
Michael Ramos
|
9ed3ba8937 |
feat(editor): markdown edit mode — direct document editing with diff-to-agent feedback
Adds direct markdown editing, source-backed annotate saves, folder edit buffers, and review-hardening fixes. |
||
|
Michael Ramos
|
be2d06a7c2 |
Make HTML annotations render HTML by default
* feat(annotate): render html files by default * fix(annotate): support raw html assets and sharing * fix(annotate): address html first review followups * fix(editor): avoid raw html sidebar init crash * fix(annotate): support portable html shares * fix(annotate): harden html share support * fix(share): clear attachments when loading shared payloads * fix(share): warn on remote share link failures * perf(annotate): lazy-build html share payloads * test(annotate): guard lazy html share generation * test(annotate): drop flaky html share server test |
||
|
Michael Ramos
|
6ec1a66c9b |
feat(review): large-PR pipeline, instant-open checkout, scroll perf, and worker-pool highlighting (#893)
* feat(review): large GitHub PR fallback + non-blocking PR checkout
Two PR-mode improvements:
1. Large GitHub PRs no longer fail to load. When `gh pr diff` is refused
(HTTP 406 for oversized diffs), fetchGhPR pages through the pulls files
API and stitches the per-file patches into a unified diff — mirroring
the existing GitLab raw_diffs fallback. Path quoting matches git's
exact rules (bare spaces unquoted) so downstream parsers round-trip;
truncation at the API's 3000-file cap is surfaced, never silent.
2. The --local worktree/clone no longer blocks startup. The review server
opens as soon as the platform diff arrives; the checkout warms in the
background as a seeded not-ready pool entry. Consumers that need real
files (agent jobs, full-stack diff, code-nav, semantic diff, AI
sessions) await pool.ensure(), with creations serialized so concurrent
fetches can't clobber the shared FETCH_HEAD. Cross-repo clone steps
converted from spawnSync to async spawns; warmup children are killed
on exit (plus `git worktree prune`) so aborted sessions can't leak
stale registrations; failed checkouts degrade honestly (no agent runs
in the wrong directory claiming local access) with a 30s retry
cooldown.
* fix(review): survive long PR checkout warmups + classify reconstructed renames
Stress-testing against oven-sh/bun#30412 (2,188 files) surfaced three bugs:
- Bun.serve's default 10s idleTimeout killed /api/semantic-diff while it
parked on the background checkout warmup (a clone that can take minutes).
Disable the idle timeout on all servers — AI SSE streams can also stall
>10s between bytes while a permission prompt waits.
- The file-badge hook memoized that failed fetch in a module-level cache
keyed by patch, pinning every badge to empty until a hard refresh. Never
cache failures; retry with backoff (5s/15s/30s).
- reconstructGhPatch/reconstructPatch omitted the `similarity index` line,
which Pierre's parser keys rename classification off — pure renames
rendered as blank plain changes with no old path. Emit 100% for
patch-less renames/copies (exactly accurate) and a synthetic 99% for
patched ones (consumers only branch on 100% vs not).
* feat(review): local full-diff upgrade for PRs whose API diff is truncated
On oversized PRs the platform APIs withhold per-file patch content entirely
(bun#30412: 1,066 of 2,188 files came back with status added/modified, zeroed
counts, and no patch). Those files rendered as empty stubs with no diff.
- fetchGhPR/fetchGlMR flag the result `patchIncomplete` when patch-less
non-rename entries exist or the 3000-file cap truncates the listing.
- New runPRLayerLocalDiff (pr-stack.ts) recomputes the exact layer diff in
the local checkout: platform merge-base + head SHA two-dot diff (three-dot
vs baseSha fallback), fetch-by-SHA for objects missing from shallow clones,
-l0 so rename detection doesn't silently degrade on huge PRs.
- The review UI shows a "Partial diff · Load full diff" notice in layer
scope; clicking re-requests the layer scope and the server swaps in the
recomputed full diff (waiting out the background clone if needed).
- PR scope/switch state writes are epoch-guarded: a request parked on the
checkout warmup can no longer overwrite a newer scope select or pr-switch.
- draftKey follows the upgraded patch so annotation drafts survive pr-switch
round-trips; recompute failures surface in the response error field.
- Pi server mirrors all of it, including an agentCwd fallback so the upgrade
works for PRs switched-to under a cross-repo clone pool.
* fix(review): use GitLab's too_large/collapsed flags for withheld-diff detection
External review caught a false negative: a too-large ADDED file comes back
new_file:true with an empty diff — indistinguishable from a legitimately
empty new file under the old heuristic, so the partial-diff upgrade was
never offered for exactly the files that matter most on big MRs.
The REST /diffs endpoint marks withheld content explicitly per entry
(verified against gitlab.com): too_large/collapsed are now authoritative in
both directions — withheld adds/deletes are flagged, binaries and empty
files are never misflagged. Older GitLab without the fields keeps the
empty-diff-on-modification heuristic.
* feat(prompts): unify review-denied suffix — triage first, no coding off raw feedback
The per-runtime defaults map (#627) gave OpenCode and Pi a different
review-denied suffix than every other runtime; updating one meant the
others silently kept "you must address all of them" — an instruction to
start coding immediately. Claude Code, Amp, Droid, Codex, Copilot, Gemini,
and Kiro were all still on it.
One default for every runtime now: triage the feedback, verify it against
the code, discuss before changing anything. Per-runtime customization
remains available via config (prompts.review.runtimes.<rt>.denied), which
resolves above the built-in default as before.
* fix(prompts): generalize review-denied suffix — 'from review', not 'external AI reviewers'
Review feedback isn't always from AI reviewers or agent jobs; often it's
the human reviewer's own annotations. Neutral wording covers both.
* fix(review): non-blocking 'Load full diff' + flag-handling hardenings
Self-review findings:
- The partial-diff upgrade reused the scope-switch handler, so clicking
"Load full diff" raised the full-screen PRSwitchOverlay — blocking the
entire UI, potentially for minutes behind a cold clone, with no text and
no cancel. The upgrade now has its own loading state: the notice shows a
spinner ("Loading full diff…") and the reviewer keeps working with the
partial diff while the request parks. Server-side epoch guards already
handle scope/PR changes made during the wait.
- GitLab too_large/collapsed: treat explicit null like absent (flags
inconclusive → legacy heuristic decides) instead of silently exonerating.
- Rename-limit lift uses -l100000 instead of -l0 ("0 = unlimited" only
holds on git >= 2.29; on older git it could disable detection outright).
* fix(review): stop scroll-driven sem stampede when semantic diff is failing
The badge retry change (
|
||
|
Michael Ramos
|
afac44b940 |
feat(review): migrate all-files code review to Pierre CodeView (#885)
Replaces the all-files renderer with Pierre's virtualized CodeView (one scroller, identical look, scales to huge diffs); deletes the legacy AllFilesDiffView/LazyFileDiff and their flag; single-file panels stay on FileDiff. @pierre/diffs pinned exact 1.2.8 (1.2.9's tree is broken on npm — see #880). Also ships: diff staleness detection ('Diff out of date · Refresh' toolbar notice backed by shared per-VCS fingerprints + GET /api/diff/fresh on both Bun and Pi servers), a stale-content guard so files edited mid-review degrade to raw-patch view instead of breaking virtualization, the Ask AI wrong-file fix (pre-existing), and toolbar/header UI polish (global settings cog, aligned sem badges, no all-files split dragger). Hardened through five review waves; tested with 1384 passing tests incl. real-git fingerprint and patch-consistency suites. Known minor gaps documented in the PR. |