feat(review): author suggestions by editing code in place (experimental, flag-gated)

[backnotprop]

What this is

A reviewer in the all-files code review view can now enter edit mode on ONE file at a time, fix the code in place using Pierre's experimental editor, and on finishing, the net change becomes ordinary suggestion annotations (suggestedCode/originalCode on a CodeAnnotation, the exact shape SuggestionModal produces today). The suggestions render inline, appear in the sidebar, and flow through the existing feedback export, which now also emits each suggestion's originalCode as a fenced "Replaces:" block ahead of the "Suggested code:" block so the applying agent can validate the anchor before applying (SuggestionModal-authored and edit-derived suggestions use the same format). The browser never writes files; the agent applies suggestions.

The edit-mode UI is OFF by default. It is gated behind a new cookie-only setting, editSuggestions (Settings > Display > Experimental > "Edit Code to Suggest"). With the flag off, zero new UI renders and no editor is ever constructed (in code-split hosts the editor chunk is never fetched; see Bundle numbers for the single-file build's inlining). One clarification: the flag gates only the edit-mode UI. The "Replaces:" block in the feedback export is not flag-gated; it is emitted for every suggestion that carries originalCode, including SuggestionModal suggestions authored with the flag off.

v1 boundaries and why

• One file at a time. A single session keeps the clone/restore lifecycle simple. Entering edit on a second file ends the current session (with a confirm prompt if it has unsaved changes; confirming turns them into suggestions).
• Plain all-files view only. Guided Review deliberately does not opt in: its GuideViewportManager (perf(review): virtualize Guided Review file cards #1158) evicts CodeView instances beyond ~8 mounted, and an evicted card destroys editor state. Scoping the entry point to the plain all-files panel (the only surface that passes the new props) was the simple safe option; pinning edited cards in the guide is possible later.
• No persistState. Deliberately unused (upstream bug, open PR pierre#1048; it is also file-item-only upstream, so diff items never persist state anyway). Session state lives only while the editor is mounted.
• Diff refresh while editing. Investigated: this app never auto-applies a refreshed diff. useDiffFreshness only shows a non-blocking "Diff out of date" chip and refresh is always user-initiated, so there is nothing to block. If the user refreshes, switches diff type, or changes the sort order or collapse default mid-session (all of these remount the CodeView; fileSetKey covers them), Pierre tears the editor down without a completion callback and the session cannot continue. A dirty session is never silently discarded there: the controller keeps the live document handle from the last change event, recovers the final text, and prompts to keep the edits as suggestions (the same pattern as the dirty file-switch prompt). Declining discards; a clean session drops silently.
• Selection action: skipped. v1 is whole-session capture on exit; a "make suggestion from selection" popover action would complicate the session lifecycle for little gain.

Design: the adapter wall

Every reference to @pierre/diffs/edit lives in ONE module, packages/review-editor/edit/pierreEditAdapter.ts:

• The editor chunk loads via import('@pierre/diffs/edit') on the first Edit click, never before. The session controller awaits the load BEFORE flipping any item into edit mode: the React wrapper throws if the EditProvider factory returns undefined, so the factory's undefined return is a defensive guard rather than a retry contract.
• Types (Marker, editor options, the editor instance) are derived structurally from the Editor class type because upstream exports no public paths for them.
• packages/review-editor/edit/adapterWall.test.ts enforces the wall: any new import of the edit entry outside the adapter fails CI.

Around it:

• edit/cloneDiff.ts: deep-clones FileDiffMetadata before a session (Pierre's editor mutates additionLines, hunks, editSessionDirty in place). The clone is the restore target.
• edit/deriveSuggestions.ts: our own diff (the diff package) of final contents vs pre-session content; one minimal hunk per contiguous changed region; pure inserts and deletes are expanded by one unchanged anchor line so originalCode and suggestedCode stay non-empty; CRLF normalized. Anchor expansion is collision-aware: regions are emitted left to right, backward expansion is only taken when the preceding line is not already claimed, forward expansion is used otherwise, and a region with no available anchor merges into the previous hunk as one spanning suggestion. A hard non-overlap invariant (lineStart strictly after the previous lineEnd) is enforced by a defensive fold. Fuzzed at 20,000 seeded multi-region edits (12,629 multi-hunk): zero overlapping ranges, zero originalCode anchor mismatches, zero round-trip failures under a sequential offset-tracking applier. A 5,000-case seeded fuzz with the same assertions is part of the unit suite.
• edit/useEditSession.ts: the lifecycle controller (hydration gate, clone, session end as one combined item write, suggestion creation, dirty-switch prompt, dirty remount recovery, collapse routing, remount cleanup).

The five research questions, answered by the build

1. Is the partial-diff hydration throw real? Yes, by design. DiffHunksRenderer.applyDocumentChange in 1.3.1 throws "Could not apply document change for partial diff" when isPartial is true. The session therefore hard-gates on full content: it fetches /api/file-content, verifies isContentConsistentWithPatch, reparses with processFile, and only then sets item.edit = true. If content cannot be fetched (binary, demo, changed on disk, stale snapshot) the Edit button becomes disabled with a tooltip naming the reason. The throw was never observed in any CDP run because of the gate.
2. Token-transformer re-render cost on a big file, with numbers. Measured via CDP on a live server, token transformer active (code-nav on), typing 30 keystrokes with no delay into an active session: 13 line file: about 0.4 to 1.0 ms per keystroke, 0 long tasks. 7,499 line file (fully hydrated, three collapsed-context regions): about 1.4 to 1.7 ms per keystroke, 0 long tasks (PerformanceObserver longtask, 50 ms threshold). Keystroke cost grows with file size but stays comfortably interactive.
3. Do renderAnnotation slots survive? Yes. Annotation cards created from a completed session render through the normal item-state path and survive the session-end updateItem. They even render inside a subsequent active edit session on the same file (screenshot evidence). The editor-marker projection of existing annotations (setMarkers via onAttach) now renders: single-line annotations previously produced a collapsed zero-width range that upstream's overlay renderer skips, so markers never painted. Marker ranges now end at character 0 of the line after the last annotated line, and a CDP run against a live review server confirms the severity squiggle renders across the annotated line during an active edit session, with the hover popover showing the annotation source and message.
4. Deferred UX vs immediate. Immediate: suggestions are created as ordinary annotations at session completion, not deferred to submission. This reuses the entire existing annotation machinery (rendering, sidebar, selection, drafts, export, external-annotation dedupe) and gives the reviewer an immediately visible, editable artifact. No new persistence or submission surface was needed.
5. Own diff vs Pierre's session hunks. Own diff. Pierre's session hunks are mutated in place, deliberately region-frozen during a session (editSessionDirty semantics), and the completion payload is only a FileContents; there is no stable emitted hunk artifact to trust. Diffing the final contents against the cloned pre-session content with the diff package gives deterministic minimal hunks, CRLF handling, collision-resolved anchors with a machine-checked non-overlap invariant, and is unit-tested and fuzzed in isolation.

One additional finding worth recording: ending a session as two writes (edit off first, restore later) races CodeView's own teardown re-render, and on large files the teardown render lands last, leaving edited content on screen. Upstream's documented commit pattern (ONE combined updateItem carrying edit: false plus the final fileDiff with a fresh cacheKey) is what the controller does, and the completion callback still fires from inside that write.

CDP evidence (live review server with a real git repo, plus vite dev server)

Five scripted scenarios, all green (screenshots and logs captured during the runs):

• Main flow (13/13 checks): flag-off shows zero edit affordances; flag-on shows the Edit button; enter edit on calc.ts, type a change, Suggest; the annotation appears with the correct suggestedCode, the edited text exists ONLY inside the suggestion UI (diff restored pristine), and Send Feedback produces an export containing the **Suggested code:** block with the edited line anchored at ### Line 8 (new).
• Escape hatches: typing then clicking Edit on another file prompts ("Finish editing calc.ts first? Your changes there will become suggestions."); dismissing keeps the session and edits; Discard ends the session, removes all typed text, creates no annotation, and restores the pristine rows.
• Perf and big file: a 7,499 line file hydrates, edits, completes, and restores pristine (edited text appears only inside the suggestion UI).
• Markers: with an existing new-side line annotation on the file, entering an edit session renders the severity marker squiggle across the annotated line with real width, and hovering shows the marker popover (source and message). Screenshot evidence captured.
• Network evidence (vite dev server, per-module requests): flag-off never requests the editor module; flag-on does not request it at load; @pierre_diffs_edit.js is fetched only after clicking Edit.

Bundle numbers

• Code-split builds (dev server or any multi-chunk host): the editor is a genuinely lazy chunk, 151.3 KB raw / 49.0 KB gzip, fetched on first Edit click only.
• Plannotator's production artifact is a single-file HTML with inlineDynamicImports: true (viteSingleFile), so the lazy chunk is inlined: review.html grows +167 KB raw / +54 KB gzip (18,132 KB to 18,299 KB raw; 5,429 KB to 5,483 KB gzip, about 1 percent), paid even with the flag off. This means the edit module namespace is constructed at page load in the single-file build (audited: no top-level side effects across all 21 modules); it stays functionally inert with the flag off. This is a property of the single-file build, not the adapter; serving the editor as a sidecar asset from both servers would restore true zero cost and is a reasonable follow-up if the feature graduates.
• Flag-off UI cost beyond that is just the header-button and controller code; no EditProvider, no edit props, and no editor construction path is reachable with the flag off.

Verification

Check	Result
bun run typecheck (CI set)	pass
bun test (full, 3,048 tests)	2,826 pass, 222 skip, 0 fail
DOM_TESTS=1 CI DOM step including FileHeader.edit.test.tsx and the new useEditSession.recovery.test.tsx	175 pass, 0 fail
New unit tests (derivation incl. collision cases + 5,000-case fuzz, clone invariant, adapter wall, dirty-session recovery)	31 pass
20,000-case derivation fuzz (overlaps / anchor mismatches / round-trip failures)	0 / 0 / 0
Builds in order: apps/review build, build:hook, build:opencode	pass, no dist committed
CDP main flow / escape hatches / perf / markers / network	all green

The DOM-gated tests are registered in test.yml's DOM step.

Honest remaining risks

• The entire feature sits on an experimental upstream API (@pierre/diffs/edit, the CodeView edit item flag). The adapter wall confines upstream renames to one file and the adapter-wall test keeps it that way, but upstream behavior changes (teardown ordering, completion semantics) could still surface as session bugs. The flag stays OFF by default for exactly this reason.
• Dirty-session recovery on a CodeView remount reads the torn-down editor's document through the FileContents delivered with the last change event. That read is guarded (a failure falls back to dropping the session, never a crash), but it relies on the document staying readable after upstream cleanup, which is observed behavior rather than documented contract.
• Suggestions anchored to context lines outside the visible patch render in the sidebar and export correctly, but their inline card is only visible when the surrounding context is expanded.

UI polish round (maintainer design feedback)

Two design changes after trying the feature live:

• Suggestion cards lost the green left accent bar. The colored SUGGESTION header is now the sole identity marker on the card body. Since the styling predates this PR (modal-authored suggestions share the same .suggestion-block CSS), the change intentionally applies to ALL suggestion cards for consistency: the inline (below-line) card and the compact sidebar card both lose the left edge line. Comment cards and every other annotation surface are untouched.
• The Edit entry button moved to the far right of the file header action row, after the Sem badge and adjacent to the file actions dropdown, keeping the experimental affordance out of the everyday Viewed / Git Add / Comment cluster. The isVeryTight responsive behavior is unchanged (icon-only at narrow widths, verified in a live CDP run at a 464 px header), and the FileHeader DOM test now asserts the ordering.

Make annotation from an editor selection

Selecting text during an edit session now shows Pierre's Selection Action popover with one action, "Make annotation". Clicking it opens the app's existing CommentPopover (anchored at the popover's screen position) and the submitted comment becomes a normal line-scoped comment annotation.

• Anchoring. The selection lives in the edited buffer; annotations anchor to the rendered diff's new side, which is the session's pre-edit content. edit/selectionAnchor.ts maps the selected line range through diffLines(preEdit, edited) at click time: unedited regions map exactly (the common case, including line shifts from edits above), ranges overlapping session edits anchor to the pristine lines those edits replace and are flagged approximate, and pure insertions anchor to the adjacent pristine line. Pre-edit coordinates never change during a session, so the anchor is correct after both Suggest and Discard.
• Entry UX. The comment entry deliberately lives outside the editor popover: focusing an input inside the shadow-DOM popover would blur the editor, collapse the selection, and tear the popover down mid-typing. The popover only snapshots (text, mapped range, rect) and hands off to the CommentPopover. The popover button itself is inline-styled with theme tokens (custom properties inherit through the shadow boundary), so it tracks light/dark and theme switches.
• Export. The captured selection text rides on the annotation (selectedText) and exports as a "Highlighted text" block; approximate anchors add an explicit note. A new comment also re-projects into editor markers immediately, and the editor selection collapses after submit so the popover does not reopen over the annotated lines.
• Related fix. SectionsPanel and FileTree window-level keyboard nav guards did not pierce shadow DOM, so Home/End/arrows typed inside the editor switched files mid-session; both now use the same composedPath()[0] guard AllFilesCodeView already had.
• Tests. Anchor-mapping unit tests (exact shifts, edited regions, insertions, deletions inside a selection, CRLF, clamping), export coverage for the Highlighted text block, and a DOM-gated popover-builder test registered in the CI DOM step. Verified end to end over CDP against a live review server: popover, entry, mid-session card, post-Suggest anchoring, and the exported feedback ("Lines 22-23 (new)" for a selection made below an in-session insert).

Edit session HUD

While a session is active, a slim strip renders below the file header with the session state (Editing, experimental chip, debounced change count) and the Suggest and Discard actions. The header carries only the Edit entry button when idle, so session controls have one home. Later commits also rename the review Settings Display tab to Editor with the edit toggle first, and remove the projection of annotations as editor markers (wavy underlines read as errors, which misrepresents comments; annotations render in their normal slots instead).

[backnotprop]

Adversarial review (at 53d27e6b)

Verdict: needs changes. One real correctness defect in the derivation, which is the surface that matters most since the agent applies suggestions verbatim. Everything around it is unusually well built: the adapter wall is machine-enforced by a test that walks the tree asserting zero offenders, the flag-off surface is genuinely inert (0 new elements, 0 errors, render-identity discipline held with no #1181-class churn), the Guided Review exclusion is structural (guide components untouched by the diff), the teardown-race fix is the correct single-write pattern with double-restore guarded, all five test mutation probes were killed, and the PR body is the most honest in this series.

High: deriveSuggestionHunks can emit overlapping hunks, and the export gives the applying agent no way to detect or resolve them (deriveSuggestions.ts:100-134). Fuzzing 20,000 multi-region edits through the real functions: zero originalCode anchor mismatches, zero round-trip failures under a sequential offset-tracking applier, but 3.8% produce overlapping ranges. Two failure shapes, both reproduced through the real export: a file-start insert plus an adjacent insert yields two annotations with identical range and contradictory bodies (an agent applies one and silently drops the other), and two deletes sharing an expanded anchor line yield overlapping ranges that an agent applying against original line numbers turns into duplicated code. The tests miss it because every expansion case is tested in isolation and the one multi-region test uses non-expanding modifications four lines apart; no non-overlap invariant is asserted anywhere. Fix: resolve region collisions before expansion (prefer the anchor direction that avoids the neighbor; merge into one spanning hunk when both directions collide) and add the invariant assertion plus fuzz coverage.

Medium: originalCode is derived and stored but never exported (exportFeedback.ts has zero references). The agent receives only the range and the suggested block, so it cannot validate an anchor before applying, which is exactly the safety net that would make an overlap detectable rather than silent. Emit it.

Medium: dirty sessions are discarded silently on file-set change (useEditSession.ts:195-206), and fileSetKey includes fileOrder and seedCollapsed, so changing sort order or collapse default mid-edit destroys in-progress work with no warning, inconsistent with the file-switch path which prompts. The refresh case is disclosed in the PR; the sort-order case is not.

Low: markers never render because single-line annotations produce a collapsed zero-width range (pierreEditAdapter.ts:85-86; upstream's renderer returns on width 0, so this is our bug and a one-line fix: give the marker a real span). Three code comments overstate guarantees, most notably "the editor module is never imported when off": with inlineDynamicImports in the single-file build, the edit module namespace is constructed at page load; it was audited functionally inert (no top-level side effects across all 21 modules), but the comments and the inertness framing should match the build reality, and the proposed sidecar-asset follow-up is the right graduation fix. Info: toggling the setting remounts CodeView (EditProvider wrapper changes tree shape), losing scroll state on a settings flip only.

Also verified: clone-before-edit ordering is correct with structuredClone captured before edit mode engages; rapid enter/exit is safe via per-session sequence keys; no cross-tab path can push data into a live session (staleness stays a non-blocking banner); persistState genuinely unused; bundle claims exact (+167,179 B review, +805 B plan); no dist commits; no em dashes; CDP flag-off/on element counts confirm zero leakage. One review limitation stated honestly: headless keystroke synthesis into Pierre's editor did not work, so the typed round-trip was verified at the unit level across 20k cases plus UI plumbing checks rather than one end-to-end typed pass.

[backnotprop]

Delta re-review (at 7d12eb70)

Verdict: merge as-is. The High finding is genuinely fixed and was verified with independent instruments, not the fix agent's own: the original 20,000-case fuzz re-run unmodified went from 760 overlapping hunks to zero, and a new 40,000-case adversarial fuzz targeting the rewritten logic specifically (tiny files, delete-every-other-line collision-dense patterns, EOF-without-newline, boundary inserts) passed all five invariants with zero violations. The design's load-bearing claim, that a forward anchor always lands on an unchanged line, was traced to the region-accumulation loop and holds by construction, not assertion. Both original reproduction shapes now produce disjoint, unambiguous output carrying a Replaces block the agent can verify. Mutating the collision guard breaks six tests including the committed fuzz at 614 failures, so the coverage has real teeth. The defensive fold also improves the failure direction permanently: a future regression degrades to over-merged hunks rather than overlapping ones, which is the right shape for something an agent applies verbatim.

Also verified: the shared suggestion formatter produces one consistent format for modal-authored and edit-derived suggestions with no broken test, fixture, or golden string; the dirty-session recovery's retained-getter claim was checked against upstream source (the contents getter closes over a local reference and survives both teardown paths, with a one-keystroke recovery window and fail-safe try/catch); the recovery test is genuinely registered in the DOM step and fails under mutation; the markers fix is real and was reproduced end to end via mid-line hover probes that the old collapsed range provably could not match; the corrected comments check out against build reality; full suite 2826 pass / 0 fail; merges cleanly onto current main.

Three small follow-ups on the ledger, none blocking:

1. The published blog post local-diff-review-for-coding-agents.md shows a verbatim sample export that is now stale (no Replaces block). User-facing wire-format documentation; worth updating at release time.
2. The markers commit says "so they render," but markers have no persistent visual decoration upstream; the fix restores hover discoverability (popover on mid-line hover), not squiggles. Framing corrected here for the record so nobody expects visible underlines.
3. Carried Info nits: the stale-closure marker guard keys on itemId rather than session identity, an empty marker set never clears prior markers, an annotation on the final line of a file without trailing newline re-collapses and will not hover, agent-produced external annotations never populate originalCode so the export format is non-uniform between human and agent suggestions (worth a deliberate decision), and two older adapter comments retain superseded framing. All decorative or narrow.

[backnotprop]

Review of the four commits since 7d12eb70 (head 10db59de)

TLDR: merge as-is. The keyboard fix repairs a real reproduced bug, the selection-anchor mapper survived 29,408 fuzz cases without one dishonest anchor, and HUD perf is a non-issue. Four small non-blocking items below.

Detail (AI review findings, skim as needed):

1. Shadow-DOM keyboard fix: real bug, verified end to end. CDP probes confirmed window-level handlers see DIFFS-CONTAINER as the target for keys typed inside the editor, so the old guard navigated files mid-edit. Fixed keys now pass through inside the editor while nav still fires normally outside a session. Bonus undisclosed in the commit: the new isContentEditable clause also stops j typed in CodeMirror comment boxes from navigating, a latent bug fixed along the way. Nit: the guards omit SELECT, which the pattern they cite includes.

2. Selection action: the mapper never lies. 29,408 fuzz cases, zero out-of-range anchors and zero cases where exact:true disagreed with the highlighted bytes. Unedited selections below inserts shift exactly; edited-region selections anchor to the replaced pristine lines with an explicit approximation note in the export. Adapter wall intact with the new structural context type. One asymmetry, plausibly intentional: dismissing the comment box leaves the selection, so the popover can reappear as a retry affordance.

3. HUD: correct. The dirty-count store is structurally safe (primitive snapshot, no republish path), and the debounced recount measures 0.95ms median on a 7,500-line file against a 250ms debounce. Minor: Suggest/Discard lost their narrow-viewport icon collapse in the move from the header, the debounce has no max wait so sustained typing shows a stale count until a pause, and the strip lacks the height-change callback its sibling banner has.

4. Polish commit correctly scoped: exactly the two suggestion-card accent rules removed, all suggestion cards affected as intended, Edit button at far right with narrow-width behavior intact.

Sweep: 2849 pass / 0 fail, new DOM test correctly registered, builds green, +6.97 kB review bundle across the four commits, plan bundle unchanged, no em dashes, merges cleanly onto main.

Documentation note: the HUD is intentionally absent from the PR body pending the maintainer's keep/drop verdict on the prototype commit; if kept, the body needs a section and the prototype: commit prefix should be finalized. The isContentEditable widening deserves a line as well.