feat(review): review → fix → re-review loop + Settings restore built-ins by radutopala · Pull Request #60 · radutopala/loop

radutopala · 2026-05-24T18:29:28Z

Summary

Headline feature is an iterative review→fix→re-review loop runnable from the Review panel; the PR also ships a Settings "Restore built-ins" affordance and a ContextMenu viewport-clamp fix that came out of the review-loop UI work.

Review loop

Adds a workflow loop node body so a loop can run an ordered sequence of children per iteration; per-iteration node_runs rows via a new iteration column + composite (run_id, node_id, iteration) unique index.
New loop review run --channel-id X [--api-url] [--wait] [--timeout] CLI subcommand drives the existing async /api/channels/{id}/review/run endpoint and emits JSON consumed by the workflow body parser. --wait timeout is bounded inside the HTTP client (not just between polls), so a hung response can't outlive the deadline.
Two seeded workflows (review-loop, review-fix-loop) shipped via fsmigrate. Both stop early when an iteration returns zero comments or the same comment-id set as the previous iteration (SameAsPrev gate). extractReviewJSON tolerates container preamble logs and requires a recognized Status to accept a candidate line as the envelope, so unrelated JSON can't masquerade as a clean review.
review-fix-loop body is chained with explicit depends_on (review → fix → verify) so WorkflowGraph draws within-iteration edges; verify uses git add -u (tracked-only) to avoid sweeping scratch files into the auto-commit.
ReviewPanel: split button (mode persisted in localStorage) + max-iter input + chip mirroring workflow events; primary button gated on loopActive AND loopRunId && !isLoopTerminal so a second concurrent run can't start mid-loop. FE re-checks the daemon's review session before dispatching the workflow (avoids a cryptic CLI failure when the in-memory session was wiped by a daemon restart).
Inline ApprovalCard rendered when no chat panel is mounted, so gate approvals during the fix step don't strand the loop.
WorkflowGraph expands loop bodies into per-iteration synthetic nodes inside a dashed group container (one column per iteration).

Settings — Restore built-ins

"Restore built-ins" bar under the Workflows and Prompt Shortcuts sections in the global Settings dialog. Re-seeds missing entries via the existing fsmigrate seeders (skip-if-name-exists, so user edits are preserved) and reports added vs. already present.
Refactors seedBuiltinCodeReviewShortcut and seedReviewLoopWorkflows to return added names, callable from both the migration runner and new RestoreBuiltinShortcuts / RestoreBuiltinWorkflows wrappers.
New POST /api/builtins/restore dispatches by kind; emits [] not null for empty added/skipped arrays so the FE doesn't crash on .length.
BDD scenarios cover already-present skip and additive re-add (plus idempotency on a second click).

Misc

ContextMenu clamps to the viewport on mount (useLayoutEffect measures + shifts left/up). Dropdowns anchored to right- or bottom-edge buttons (e.g. the Review panel's mode caret) no longer overflow the window.

Test plan

go test -race ./... green; backend touched packages (workflow, db, fsmigrate, cmd/loop, api) at 100% coverage of changed files
make coverage-check — Coverage: 100.0%
make lint — 0 issues
npm run typecheck (app) — clean
BDD: component-bdd green (Settings restore-built-ins + Global workflows panel flake fix)
Manual: run review-fix-loop end-to-end on a channel; verify chip transitions, group rect renders per iteration, ApprovalCard appears inline when chat is hidden
Manual CLI: loop review run --channel-id <id> --wait blocks until ready and prints JSON
Manual: Settings → Workflows → "Restore built-ins" re-seeds missing entries; second click is a no-op

Adds a workflow loop body with per-iteration node_runs, a `loop review run` CLI driving the existing async daemon endpoint, and two seeded workflows (review-loop, review-fix-loop) wired into a ReviewPanel split button. The loop stops when a review iteration produces zero comments or the same comment-id set as the previous iteration. WorkflowGraph expands the body into per-iteration synthetic nodes inside a dashed group container.

The review-fix-loop's bash node defaulted to `loop review run --wait` with a 5m cap. Real PR reviews routinely run 5–15 minutes, so the CLI inside the agent container timed out before the daemon finished — comments still streamed to the FE, but the loop body errored on the review node and never reached fix/verify. Bumped the CLI default to 30m. The verify step previously just printed HEAD, leaving uncommitted fixes stranded when the agent forgot to commit. It now stages and commits any leftover changes (`git diff --cached --quiet` short- circuits when the agent already committed). Added migration #7 to patch existing configs in-place, preserving any user-customized verify script.

The agent container's stdout includes a `loop-dockerproxy started` log line before `loop review run --wait` prints its JSON envelope. parseReviewOutput's strict json.Unmarshal of the full stdout failed on the preamble and fell into its error branch, which cleared CommentsJSON and set SameAsPrev = (len(prev)==0). Two visible symptoms followed: 1. The fix prompt rendered with `{{.Review.CommentsJSON}}` empty, so the agent replied "the comments section is empty". 2. On iter 0 prev is always empty, so SameAsPrev flipped to true and the outer loop's `or NoComments SameAsPrev` condition exited after a single iteration — even when max_iterations was set to 3. extractReviewJSON now scans stdout backwards through non-empty lines, returning on the first that parses as the envelope; falls back to whole-trimmed-stdout for the (hypothetical) pretty-printed case. The reversed walk lets the JSON win even if future log lines trail it.

The review-fix loop's body flips the daemon's review session.status back to "ready" between iterations (after each `review` child completes) and `busy` clears as soon as the initial POST returns. Together this made `runDisabled = busy || session?.status !== "ready"` re-enable the primary button mid-loop, letting a user start a second concurrent workflow run on the same worktree (review session + git commits would race). Track a dedicated `loopActive` state that flips true at run start and false on workflow.run_completed, and add it to runDisabled. The verify script's `git add -A` swept every untracked/modified file in the worktree into the auto-generated `fix: address review feedback` commit — scratch files, debug logs, dependency caches. Switch to `git add -u` so only tracked changes are staged; the fix prompt still asks the agent to commit new files itself. Add a follow-up migration that patches the buggy `-A` variant in configs from dev installs that already advanced past migration #7.

…ures cmd/loop/review.go: the --timeout flag only gated the inter-poll time.Now().After(deadline) check — it never bounded an in-flight client.Do call because http.DefaultClient has no Timeout set and cobra's parent ctx has no deadline. A hung GET response (network stall, daemon deadlock, partial write) blocked the polling loop inside Do indefinitely and the 30m default never fired. Wrap ctx in context.WithTimeout when --wait is set so cancellation flows into every request, and centralise the "timed out after X" message via a small wrapTimeout helper. New test TestRunReviewWaitTimeoutDuringHungResponse uses a handler that blocks until the request ctx is cancelled to prove the bound now trips inside Do, not just between polls. internal/db/migrations.go: `PRAGMA foreign_keys=OFF`/`=ON` wrapped as sqlMigration entries were silently no-ops because runSQLMigration wraps every SQL migration in its own BeginTx and SQLite ignores `PRAGMA foreign_keys` inside an open transaction. Today this is benign — no other table has an incoming FK to workflow_node_runs, so the rebuild's INSERT…SELECT/DROP/RENAME runs fine with FK enforcement on — but the dead PRAGMAs misrepresented what the migration guaranteed and the pattern would silently fail if copy-pasted to a table with incoming FKs. Replace the two PRAGMA entries with `SELECT 1` no-op placeholders (can't drop them outright without shifting later migrations' version numbers and breaking partially-applied installs) and document the actual safety property + the recipe future similar-rebuild migrations should follow (lift into funcMigration that issues PRAGMA outside any tx). internal/workflow/dag.go: when extractReviewJSON returned false on the first loop iteration, prev was nil and the parser set SameAsPrev=true. The seeded loops' stop condition `{{ or .Review.NoComments .Review.SameAsPrev }}` then terminated the loop with `completed` after a single iteration even though the review subprocess emitted unparseable stdout — the UI reported "review with no findings" when the real outcome was "the bash node succeeded but the JSON envelope was missing" (daemon/CLI bug, $API_URL misconfig that returned an empty body, future stdout pollution after the JSON line). Always set SameAsPrev=false on parse failure so the loop keeps iterating up to maxIter instead of silently treating the failure as clean. Tests updated: TestEmptyStdoutSameAsPrevWhenPrevEmpty renamed to TestEmptyStdoutDoesNotTerminateLoop and TestInvalidJSONResetsState extended to assert the new SameAsPrev=false invariant.

extractReviewJSON now requires a recognized Status ("ready" or "error") before accepting a candidate line as the review envelope, so an unrelated JSON object (e.g. a future sidecar log line emitted after the CLI's compact JSON) cannot masquerade as a clean review and prematurely terminate the seeded review-fix loop. ReviewPanel's primary-button gate now also checks `loopRunId !== null && !isLoopTerminal` in addition to `loopActive`, so the "no second concurrent run on this channel" invariant is explicit from the visible chip state rather than only the loopActive boolean.

The daemon's reviewStore is in-memory and gets wiped on restart, while the FE may still hold a stale session object in React state. Without this check the workflow starts and immediately fails on the CLI's POST /review/run with a cryptic "node loop failed: script exited with status 1". Run a GET /api/channels/{id}/review first; if absent, surface "No review session loaded — pick a PR first, then run the loop."

WorkflowGraph drew within-iteration edges only when body children had explicit deps; the seeded review-fix-loop relied on array order alone so review → fix → verify rendered as a single vertical column with no arrows, and only the cross-iteration verify[i-1] → review[i] links were visible. Declare fix depends_on review and verify depends_on fix in the seed, plus an fsmigrate patch that backfills the deps on the existing on-disk workflow (skips when a user has already set custom deps). Execution order is unchanged — executeLoopNode iterates body sequentially regardless.

Dropdowns anchored to right- or bottom-edge buttons (e.g. the Review panel's mode caret) overflowed the window because the menu rendered at the caller-supplied (x,y) unconditionally. Measure on mount via useLayoutEffect and shift left/up so the menu fits, leaving callers free to pass the natural anchor coordinates.

Adds a "Restore built-ins" bar under the Workflows and Prompt Shortcuts sections in the global Settings dialog. Re-seeds missing entries via the existing fsmigrate seeders (skip-if-name-exists, so user edits are preserved) and reports which were added vs. already present. - Refactors seedBuiltinCodeReviewShortcut and seedReviewLoopWorkflows to return the list of added names, callable from both the migration runner and the new RestoreBuiltinShortcuts / RestoreBuiltinWorkflows wrappers (internal/fsmigrate/restore.go). - New POST /api/builtins/restore endpoint dispatches by kind and reports Added/Skipped against a canonical name list. - FE: api/builtins.ts client + Settings.tsx RestoreBuiltinsBar with per-section toast feedback and config refetch on success.

Two scenarios verify the Restore-built-ins bar surfaces correctly: already-present skip for seeded workflows, and additive re-add for the cleared shortcut (plus idempotency on a second click). Adds a settings-panel data-testid so the scoped click step can disambiguate from the outer-app sidebar Workflows button.

When the runMode catch handler fires (session re-check fails, no session loaded guard trips, or startWorkflowRun rejects), the chip used to clear to empty — making the failure invisible in the header. Show "failed" instead so the chip mirrors the workflow.run_completed("failed") path and the user gets consistent visual feedback across both failure modes. Addresses the secondary recommendation from the review comment on this catch block: with the primary trap (runDisabled gating on stale loopRunId) already removed in a0d8ee3, this completes the cleanup.

Add a default branch to executeLoopBody's child-type switch so a body child with a type other than prompt/bash returns an explicit error instead of persisting as Success with empty output. validateWorkflowDef catches this at StartRun, but executeDAGFromCheckpoint resumes pinned workflow_defs without re-validating — a stored definition from before the validator landed or a manually edited DB row would otherwise appear to succeed on recovery while doing nothing. Also pulls in the in-flight test/doc tweaks for the review-loop work: direct-call coverage for the new executor default branch, a regression test for runReview cancellation during the poll interval, README entry for `loop review run`, and a small cleanup of parseJSONValue in the HJSON migrations helper.

Bug fixes (high/medium severity from /code-review sweep): - cmd/loop/review.go: classify HTTP 4xx and present=false as permanent (wrap in pollPermanentError); bound !--wait POST with reviewPostTimeout (30s default, struct-injected for tests). - internal/fsmigrate/restore.go: chain patchReviewFixVerifyScript + patchReviewFixLoopBodyDeps after seedReviewLoopWorkflows so stale on-disk workflows get patched on restore. - internal/workflow/dag.go: nrEnd now sets StartedAt to avoid NULL rows when nrStart silently failed. - internal/db/db.go: UpdateNodeHeartbeat takes iteration and scopes the UPDATE by (run_id, node_id, iteration); mock + 7 engine tests updated. - app/src/components/panels/WorkflowGraph.tsx: preserve external depends_on entries when expanding loop bodies (non-body deps fall through to the rewire pass). - app/src/components/panels/ReviewPanel.tsx: hydrate loopRunId / loopActive from sessionStorage on mount; resync via fetchWorkflowRun and clear stale handles on terminal status. Review prompt + defaults: - internal/api/review_handler.go: rewrite defaultReviewPrompt as a 5-angle recall finder (line-by-line, removed-behavior, cross-file, language-pitfalls, wrapper/proxy) + 1-vote 3-state verifier + gap sweep, capped at 15 findings. Keeps the existing <review-comment path line side> XML envelope; explicit "do not fix anything" so the agent leaves triage to the Review panel. - internal/fsmigrate/migrations.go + app ReviewPanel.tsx + docs: default max_iterations for review-loop and review-fix-loop is now 1 (was 3); the recall-mode finder makes extra passes redundant for most reviews.

Closes the last batch of unresolved threads on PR #60: bound iteration state on error, gate review-output parsing on seeded workflows, serialize restore-builtins via a mutex, harden CLI polling against transient transport errors and oversized response bodies, scope expandLoopBodies and Settings restore state per-loop/per-kind, and preserve external deps when re-keying loop body children. Tests cover the new branches; lint and coverage are clean.

Ten issue-level conversation comments on PR #60: #1 review_handler.go — runReviewAsync now takes a cancellable ctx so session-delete and server-Stop detach the long-running agent run instead of leaking the container for 5–20 min after teardown. #2/#6 dag.go — executeWithRetry's retry-attempt UpsertNodeRun now sets Iteration so a body-child retry on iter>0 no longer clobbers the iteration-0 row. #3 engine.go — recoverPausedRun/RunningRun pick the highest-iteration Success row's Output deterministically for completedOutputs, so templated body children see the correct prior-iteration value on resume instead of whatever ListNodeRuns happened to return last. #4 engine.go — both recovery paths now re-validate the pinned definition before executeDAGFromCheckpoint, so a stored def that pre-dates a validator rule fails early instead of half-executing a loop body. #5 engine.go — explicit empty-string skip on opts.Inputs so external callers sending {"max_iterations":""} no longer wipe the default. #7 WorkflowGraph.tsx — auto-center useEffect is now actually one-shot (reads hasCentered.current); new iterations no longer reset the viewport mid-investigation. #8 workflows.ts — error throws use a new describeError() helper that reads the daemon's JSON body so HTTP/2 callers stop seeing "Failed to ___: " messages with empty suffix. #9 review_handler.go — pushOneComment skips MarkPushed when ghID==0 so the 422-fallback's unparseable-response branch doesn't strand a comment as permanently undeletable. #10 ContextMenu.tsx — WAI-ARIA menu pattern: role=menu/menuitem, focus-on-mount, focus-return-on-close, roving tabindex, ArrowUp/Down/Home/End nav skipping separators. Tests: 9 new (4 review handler, 5 engine recovery/start). make lint clean, make coverage-check 100.0%.

CI electron-typecheck reported TS2345 on three setFocusIdx calls because itemIndices[i] is typed `number | undefined` under noUncheckedIndexedAccess. Hoist first/last and guard the ArrowDown/Up lookup to keep types narrow.

Loopback-gate POST /api/builtins/restore, surface patcher results as a separate Patched field, cap loop iterations at 50, scope review-output parsing to seeded workflows only, preserve IDs on parse miss (no false "no findings" exits), reject body-child IDs that collide with sibling top-level nodes, and refuse to patch a non-array config key. FE: persist the last-used review mode only after the run actually starts, iterate all gate-approval sources for inline rendering, skip the post-restore global config refetch when the user has unsaved edits, and dedupe the WorkflowGraph defs-empty fallback by node_id so 3-iteration loops no longer collapse to one node.

- Settings.tsx: per-kind restoring/restoreMsg state so Workflows/Shortcuts tabs don't leak loading state across kinds - cmd/loop/review.go: fall back to a sentinel when daemon reports status=error with an empty message, instead of surfacing a blank error - workflow/dag.go executeLoopBody: honor body-child timeout: declarations via per-iteration context.WithTimeout + explicit cancel - workflow/dag.go extractReviewJSON: forward-scan (first valid envelope wins) so debug echoes after the real envelope can't displace it - workflow/validate.go: reject body-child IDs that collide within the same loop or across two loops (both race UPSERTs on (run_id, node_id, iteration) and inflate WorkflowGraph iter counts) Tests + 100% coverage maintained.

Closes the last batch of unresolved threads on PR #60: bound iteration state on error, gate review-output parsing on seeded workflows, serialize restore-builtins via a mutex, harden CLI polling against transient transport errors and oversized response bodies, scope expandLoopBodies and Settings restore state per-loop/per-kind, and preserve external deps when re-keying loop body children. Tests cover the new branches; lint and coverage are clean.

Ten issue-level conversation comments on PR #60: #1 review_handler.go — runReviewAsync now takes a cancellable ctx so session-delete and server-Stop detach the long-running agent run instead of leaking the container for 5–20 min after teardown. #2/#6 dag.go — executeWithRetry's retry-attempt UpsertNodeRun now sets Iteration so a body-child retry on iter>0 no longer clobbers the iteration-0 row. #3 engine.go — recoverPausedRun/RunningRun pick the highest-iteration Success row's Output deterministically for completedOutputs, so templated body children see the correct prior-iteration value on resume instead of whatever ListNodeRuns happened to return last. #4 engine.go — both recovery paths now re-validate the pinned definition before executeDAGFromCheckpoint, so a stored def that pre-dates a validator rule fails early instead of half-executing a loop body. #5 engine.go — explicit empty-string skip on opts.Inputs so external callers sending {"max_iterations":""} no longer wipe the default. #7 WorkflowGraph.tsx — auto-center useEffect is now actually one-shot (reads hasCentered.current); new iterations no longer reset the viewport mid-investigation. #8 workflows.ts — error throws use a new describeError() helper that reads the daemon's JSON body so HTTP/2 callers stop seeing "Failed to ___: " messages with empty suffix. #9 review_handler.go — pushOneComment skips MarkPushed when ghID==0 so the 422-fallback's unparseable-response branch doesn't strand a comment as permanently undeletable. #10 ContextMenu.tsx — WAI-ARIA menu pattern: role=menu/menuitem, focus-on-mount, focus-return-on-close, roving tabindex, ArrowUp/Down/Home/End nav skipping separators. Tests: 9 new (4 review handler, 5 engine recovery/start). make lint clean, make coverage-check 100.0%.

radutopala added 10 commits May 22, 2026 15:41

radutopala force-pushed the feat/review-loop-v1 branch from 0b0f5a3 to 0d89386 Compare May 26, 2026 05:34

radutopala force-pushed the feat/review-loop-v1 branch from 0d89386 to f6e631c Compare May 26, 2026 05:35

radutopala changed the title ~~feat(review): iterate review → fix → re-review via seeded workflows~~ feat(review): review → fix → re-review loop + Settings restore built-ins May 26, 2026

radutopala added 5 commits May 26, 2026 10:03

fix(review): address three review-loop reliability gaps

a0d8ee3

fix: address review feedback

e8281b4

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/panels/ReviewPanel.tsx Outdated

radutopala commented May 26, 2026

View reviewed changes

Comment thread internal/api/server.go

radutopala commented May 26, 2026

View reviewed changes

Comment thread internal/workflow/dag.go

radutopala commented May 26, 2026

View reviewed changes

Comment thread cmd/loop/review.go

radutopala commented May 26, 2026

View reviewed changes

Comment thread internal/workflow/dag.go Outdated

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/panels/ReviewPanel.tsx Outdated

radutopala commented May 26, 2026

View reviewed changes

Comment thread internal/workflow/dag.go

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/panels/ReviewPanel.tsx Outdated

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/panels/ReviewPanel.tsx

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/shared/ContextMenu.tsx Outdated

radutopala commented May 26, 2026

View reviewed changes

Comment thread app/src/components/panels/ReviewPanel.tsx