Add code-linked feedback and configurable review question agents

AGENTS.md

@@ -0,0 +1,48 @@
+# Repository agent instructions
+
+Follow the repository conventions in `CLAUDE.md`. Read `DESIGN.md` before making visual or interaction changes.
+
+## Async jobs and polling
+
+For features with background jobs, polling, retries, cancellation, or shutdown:
+
+- Define the lifecycle states and ownership before implementation: pending, running, completed, failed, cancelled, stale, and closing.
+- Treat persisted state, in-memory jobs, subprocesses, HTTP requests, and rendered UI as separate state holders. Define how each transitions and settles.
+- Never apply a background response without proving it is still current. Use a generation, attempt ID, version, or guarded merge so older polling responses cannot overwrite newer actions.
+- Do not release a concurrency slot when cancellation is requested. Keep the job tracked until its underlying invocation or subprocess has terminated.
+- Do not let a retry replace a locally active job, even when its persisted lease has expired or wall-clock time changes.
+- Validate retry context against the current snapshot, plan revision, assignment, and referenced code. If any context is stale, disable retry and require a new request.
+- Preserve the original timeout, cancellation, and shutdown reason through every layer. Do not replace actionable errors with generic cancellation text.
+- Begin shutdown by rejecting new work at the outer admission boundary. Drain already-admitted HTTP requests, then cancel and await jobs, then close storage.
+- Polling endpoints should read only the state they need. Do not rebuild Git history or the full review merely to retrieve background-job status.
+
+## Async review UI
+
+- A background response must not erase text, selections, attachments, navigation changes, or other input made after the request started.
+- Clear a submitted draft only if its current value and attachment still match what was submitted. Treat this as compare-and-swap behavior.
+- Preserve completed historical results, but visibly mark them stale when their snapshot, plan revision, assignment, or referenced code no longer matches.
+- When polling updates one part of the screen, update only that state. Preserve scroll position unless the user was already following the bottom.
+
+## Required race regressions
+
+Before opening or updating a PR for asynchronous behavior, test every applicable interleaving with controllable promises, clocks, and partial requests:
+
+- Poll starts, then a user action completes, then the old poll returns.
+- A job lease expires, then retry is attempted while the original job still runs.
+- Timeout fires, then the provider remains unsettled temporarily, then retry is attempted.
+- Shutdown starts, then a new request arrives.
+- A request is partially received, then shutdown starts, then the request completes.
+- An abort error fires, then subprocess close arrives later.
+- Submit starts, then the user edits the composer or switches items, then the response returns.
+- Referenced code is reassigned or the snapshot changes, then retry or rendering occurs.
+
+Every reproduced race requires a failing-before and passing-after regression. Assert both the visible result and the durable state when they can diverge.
+
+## Review readiness
+
+- Run final validation against the exact pushed head after the last change.
+- Report current test counts separately from historical milestone counts.
+- Before requesting automated review, report the current head, CI state, mergeability, unresolved threads, and deferred follow-up issues.
+- Reproduce summary-only review concerns or turn them into a concrete follow-up issue. Do not repeatedly patch vague wording without a failure case.
+- For each review round, record what changed, what was declined and why, and the regression evidence. Re-request review until a round returns no new findings.
+- Before closing or merging a PR, extract the highest-value, broadly reusable lessons from its review and add concise rules to this file. Omit one-off implementation details and rules already covered here.

README.md

@@ -2,7 +2,7 @@
 
 Review agent-made Git changes one plan item at a time. The approved plan lists each item's files and acceptance checks; the review engine shows which item produced each change and flags foreign or overlapping work.
 
-**Status:** the plan/linking library, SQLite store, and local read-only review screen are implemented. Run `npm run demo` and open its private local URL. There is no agent execution or merge command. The human go/no-go experiment is still pending; see [the local review guide](docs/implementation/read-only-review.md).
+**Status:** the plan/linking library, SQLite store, and local read-only review screen are implemented. Run `npm run demo` and open its private local URL. Ask can invoke Claude Code or Codex for read-only answers; choose the provider in Settings. Code-writing agents and merge commands are not implemented. The human go/no-go experiment is still pending; see [the local review guide](docs/implementation/read-only-review.md).
 
 ## Development
 
@@ -52,6 +52,6 @@ Inputs such as `planText` and the ledger must come from the trusted runner. `run
 - Ownership uses line diffs, not semantic inference. Within one replacement block, new lines inherit all affected owners conservatively. Function context comes from Git hunk headers, not an AST.
 - The importer requires accurate typed base entries, stable plan identity, a selected issue, and a trusted checkout path-identity function. It rejects path traversal, Git metadata paths, and traversal through a listed file/symlink/submodule. Runtime symlink and write-scope enforcement belong to the future container/runner; plan validation alone is not a sandbox.
 - Allowed commands restrict accidents, not hostile programs or changed scripts. Parsing returns argv and never executes it. An unlisted valid command is a warning and must not run until allowed.
-- No code here claims container isolation, vendor-only network access, credential protection, or safe dependency installation. Those controls must be implemented before running agents.
+- No code here claims container isolation, vendor-only network access, credential protection, or safe dependency installation. Those controls must be implemented before running code-writing agents. Question answering uses bounded supplied context in a separate temporary working directory, with command tools disabled.
 
 See [implementation decisions and evidence](docs/implementation/build-step-1.md) and the [plan format](docs/plan-format.md).

docs/implementation/read-only-review.md

@@ -50,3 +50,63 @@ Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in
 Review round 6 reproduced destination-inside-source mutation; planting now resolves the existing destination ancestor and rejects canonical source descendants before creating files, including `.git` and symlink aliases. The Git-environment finding was declined: `gitRaw` already passes `{cwd, env, ...}` to `execFileSync`. A new end-to-end test with inherited `GIT_DIR`/`GIT_WORK_TREE` passed before any production change. The summary's snapshot-race claim supplied no concrete interleaving; the service already checks revision/snapshot/review version before publishing a view and writes use atomic store CAS, covered by concurrent-write tests.
 
 Review round 7 reproduced oversized-dimension raster previews and dangling SQLite sidecar symlinks. Preview metadata is now parsed from the bounded buffer with image-size: at most 8,192 pixels per side, 4 million pixels per image and 16 million pixels across unique preview blobs. Unknown dimensions omit the preview; existing compressed/response byte limits remain. Sidecars use lstat so dangling links are rejected. Both new regressions failed before the fixes. The summary mentioned rename-scope/literal-path concerns without specific findings; no additional behavior was inferred from that shorthand.
+
+## Feedback on selected code
+
+Click a changed line number, Shift-click to extend a range, or highlight code within one changed block. The selection toolbar offers Ask and Request change; each attaches the full selected lines to that item's composer. Question/change-request drafts retain separate attachments. Remove snippet returns to item-level feedback.
+
+The server validates the segment and range against the current review token and constructs the saved reference itself: path, old/new side, exact text, line range, base and head commits. Limits are 200 lines and 16,000 characters. References on unassigned changes require assigning them to a plan item first. Existing item-level notes remain compatible without a database migration.
+
+Saved references navigate to the selected lines while the reviewed commits and assignment still match. Otherwise they are labeled Outdated and open the original captured snippet, never silently pointing at new code. This deliberately marks references outdated on any base/head change, even if the snippet is unchanged. Refresh invalidates transient selections; outdated draft attachments must be removed and reselected before submission. No agent or GitHub comment is invoked.
+
+Snippet-feedback validation: 176 unit/integration tests and 15 browser tests pass (baseline 174 and 13), plus typecheck and diff checks. Coverage includes clicked lines, Shift-click ranges, DOM text highlighting, independent question/change drafts, removal, persistence, navigation, removed-side references, forged/invalid references and outdated snapshots. The actual PR #597 walkthrough was also opened in Chromium and the selected-line/composer layout visually inspected without saving test notes to its database.
+
+## Agent answers and Settings
+
+Scope update requested during the PR #597 walkthrough: Ask now invokes a read-only question agent before the broader implementation/queue go/no-go gate. This supersedes the earlier “no agent answers” limitation only for questions. Request change still saves feedback without running edits, and answering never changes a plan revision or approval.
+
+Open Settings and choose Claude Code or Codex. The choice persists in this review database (schema v3 adds app_settings); existing notes and snippet references survive migration. No provider is selected automatically. The installed CLI must already be signed in. The question, selected snippet, plan item, changed code, checks and bounded prior conversation are sent to the selected provider. Truncation and absent evidence are part of the context; the agent must not claim unrun tests passed.
+
+The question is saved before launch. Conversation displays Answering, then a persisted answer or an error with Retry answer. Retries reuse the question and have attempt IDs to reject late results from older attempts. At most two requests run per server; each has a two-minute deadline. Graceful shutdown cancels running answers; after a crash, pending attempts become retryable after their lease expires. A question from an older snapshot must be asked again against current code. Answers retain their provider and original question snapshot. Polling updates only notes, preserving the current draft and code selection.
+
+The CLI adapter runs without a shell in a fresh temporary directory. Claude uses safe mode with no tools and no session persistence. Codex uses an ephemeral, read-only session with user config/rules ignored, shell/apps/plugins/hooks/memory/delegation disabled, and web search disabled. These are restricted question adapters, not the future containerized code-running agent environment. Stdout and answer sizes are bounded; raw process logs and credentials are not returned to the browser.
+
+Codex options were checked against the installed CLI help and the official [non-interactive documentation](https://learn.chatgpt.com/docs/non-interactive-mode) and [configuration reference](https://learn.chatgpt.com/docs/config-file/config-reference). Both installed providers passed live connection checks. A separate copy of PR #597's review database passed a real Settings → Ask → saved Claude answer browser test; the user's review state and source checkout were unchanged. Native Node startup is covered by enabling TypeScript's erasableSyntaxOnly check after the live test caught an unsupported parameter-property declaration.
+
+Validation: 181 unit/integration tests, 19 browser tests, and typecheck pass. A delayed-initial-review regression reproduced an unresponsive Settings button; binding controls before awaiting the first load fixes it. The restarted PR #597 instance also passed an immediate Settings-open check with both provider options visible.
+
+Single-click submission follow-up: a held `/api/action` browser request reproduced the lack of immediate feedback (the submit button stayed enabled with no saving state). Question/change submission now shows Saving immediately, disables submission while a review action or refresh is in progress, and scrolls the saved note into view. Failures explicitly retain the draft for refresh/retry. The delayed-request regression checks one click produces exactly one stored question and one agent invocation; a lost first click was not independently reproduced.
+
+Answer scrolling: when an asynchronous answer or failure arrives, Conversation follows the bottom if the reader was already within 32 pixels of it. Scrolling up keeps the current position through polling updates. Only the conversation list scrolls; the code pane and composer stay in place. Browser regressions cover both arrival at the bottom and arrival while reading earlier messages; the bottom-follow test failed before the fix with a 1,302-pixel gap.
+
+Conversation width: drag the divider on its left edge to resize between 280 and 480 pixels. Focus the divider and use Left/Right for 20-pixel steps or Home/End for the limits. The width survives collapsing/reopening the pane within the page; reloading uses the layout default. This replaces the hard-to-discover native corner resize grip. The browser regression covers pointer dragging, keyboard limits, code-pane space, and collapse/reopen.
+
+Selection actions now appear in a bordered floating toolbar beside the visible selected lines, with Ask emphasized. The toolbar stays within the code viewport, leaves line numbers available for Shift-click extension, and hides when the selection scrolls out of view. Clear selection removes it. The long-diff browser regression checks proximity, viewport bounds, attachment, scrolling away/back, and clearing; existing range/highlight tests remain required.
+
+PR #11 review round 1 reproduced a stale-poll race: a held question-list response hid a question submitted after the poll began. Review actions and refreshes now advance a generation counter; earlier poll successes and errors are ignored. The browser regression failed with one visible note instead of two before the fix.
+
+PR #11 review round 2 confirmed the poll fix and raised an outdated-retry issue in its summary. A browser regression reproduced the retry button on an older snapshot; such unanswered questions now instruct the reviewer to ask again against current code. Completed historical answers retain their outdated label. The summary's timeout-reporting concern supplied no concrete failure; timeout and interrupted-lease regressions remain in the validation suite.
+
+PR #11 review round 3 reproduced retrying a locally active job after its persisted lease expires (for example after a clock jump). The manager now rejects a retry while that note is in its running map, before touching the persisted attempt. The regression advances wall time without advancing the deadline timer and checks the original attempt, single invocation, and shutdown cancellation remain intact.
+
+PR #11 review round 4 reproduced new question work entering during shutdown. Questions now marks itself closing synchronously before cancelling/draining jobs; start rejects before any store write or agent invocation, during and after shutdown. The regression failed before the guard and checks that the unstarted note has no attempt recorded.
+
+PR #11 round 5 confirmed the shutdown fix and made the timeout-summary concern concrete: the CLI adapter replaced the abort reason with generic cancellation. Direct adapter regressions reproduced this for timeout and shutdown reasons. The adapter now preserves Error-valued abort reasons; provider launch errors remain sanitized.
+
+PR #11 round 6 reproduced releasing a concurrency slot while a cancelled invocation was still unsettled. Cancellation now persists the visible failure promptly but retains the job until the invocation settles; retries remain blocked and shutdown awaits it. The CLI adapter defers rejection and temporary-directory cleanup until child close, including abort and output-limit failures. Regressions cover late settlement and abort-error-before-close ordering. Injected agents must settle after cancellation; production adapters terminate with SIGKILL and await closure.
+
+The review's polling-efficiency observation is tracked separately in issue #12: answer polling currently rebuilds the full review, and the follow-up will measure and remove that work while preserving snapshot metadata and stale-response protection.
+
+PR #11 round 7 confirmed cancellation tracking and described outdated snippet retries in its summary. Direct UI reassignment of owned code is already rejected; a persisted assignment change reproduced the stale-reference case, now guarded in both manager and retry UI. The summary also mentioned in-flight composer state: browser tests reproduced lost typing during submission. Action responses now capture current drafts before rendering and clear only the unchanged submitted draft/attachment, preserving edits and drafts on other items.
+
+PR #11 round 8 returned no inline findings and identified one related summary gap: completed answers only warned for an older snapshot or plan revision, not a snippet invalidated by reassignment. Completed answers now show the same historical-context warning for either condition. A browser regression preserves the historical answer while checking the warning after a persisted assignment change.
+
+PR #11 round 9 returned no inline findings and identified shutdown ordering in its summary. Server shutdown now stops accepting connections and drains in-flight HTTP requests before closing the question manager and database. A partial-body request regression proves a question already admitted during shutdown starts its agent and is persisted as interrupted rather than left unanswered.
+
+Current PR validation: 188 unit/integration tests, 32 browser tests, typecheck, and diff checks. Earlier counts above identify the stage when each behavior was added.
+
+The documentation-only review after adding `AGENTS.md` exposed one more concrete lifecycle race: an expired persisted attempt could show Retry while its cancelled provider was still settling. Question polling now exposes whether the local invocation remains active, displays Finishing cancellation, and keeps polling without exposing Retry until settlement. A browser regression advances the persisted clock while leaving the invocation unresolved, then confirms Retry appears only after settlement.
+
+The follow-up review exposed the same active-job marker missing from the initial review response. Reloading during provider settlement could therefore expose Retry and stop polling. Initial loads and action responses now include the marker used by question polling; the browser regression reloads during settlement and confirms Retry remains hidden until the invocation finishes.
+
+The next review made assignment drift concrete for item-level answers without snippet references. Each answer now records a hash of the changed segments supplied for its item. Moving code into or out of that item preserves the answer but labels it as earlier review context and rejects retrying the old question; a regression assigns new code without changing the snapshot or plan revision, checks the historical marker, and proves no second agent invocation starts.