diff --git a/README.md b/README.md index 4cf4a13..7418cf9 100644 --- a/README.md +++ b/README.md @@ -120,8 +120,8 @@ execution: models: # round-robin in / format - google/gemini-3.5-flash # 1st and 3rd task in parallel - openai/gpt-5.5 # 2nd task in parallel - autoCommit: true # commit changes after each task passes review (or when autoReview is off) - autoReview: false # review changes BEFORE commit; loop on fail, commit on pass + autoCommit: true # commit after each task (mandated when autoReview is on; standalone toggle when off) + autoReview: false # commit → review → loop on fail → merge on pass implModel: "" # model for task impl (sequential mode, empty = inherit parent) commitModel: "" # model for commit sessions (empty = inherit task model) reviewModel: "" # model for review sessions (empty = inherit task model) @@ -145,15 +145,17 @@ prompts: #### Auto-review and Auto-commit -At loop startup the review question is asked FIRST, then the commit -question. When `autoReview` is enabled, a review gate runs BEFORE the -commit: each task's uncommitted changes are reviewed against the task -description, and on a `fail` verdict the task is re-executed with the -review feedback injected into the prompt (looping until the review -passes or `maxReviewRetries` is exhausted). When `autoCommit` is also -enabled, a follow-up agent stages and commits the changes once the -review passes (or after retries exhaust); otherwise the changes are -left uncommitted for manual inspection. +At loop startup the review question is asked FIRST. When `autoReview` is +enabled, commit is **mandated** — after task execution, changes are +committed (via a commit agent session when the task agent didn't +self-commit), then the complete task diff (`baseRef..HEAD`) is reviewed +against the task description. On a `fail` verdict the task is +re-executed with the review feedback injected into the prompt (looping +until the review passes or `maxReviewRetries` is exhausted). After +re-execution, changes are committed again and the full diff is +re-reviewed with the same base ref so the reviewer sees the complete +state — original work plus fixes. On pass, the changes are already +committed and the worktree merges. When `autoReview` is disabled, `autoCommit` runs a follow-up commit agent after each task with no review. Both options can be overridden at diff --git a/index.ts b/index.ts index c8e2e7d..e5cc8d6 100644 --- a/index.ts +++ b/index.ts @@ -123,10 +123,10 @@ function buildPlanByMode( /** * Prompt the user to select auto-review and auto-commit options for this loop. - * Reviews are asked about FIRST, then commits — so the user can opt into a - * review gate (review changes, loop on fail) and decide whether a passing - * review should commit. Fields explicitly set in the config YAML are - * skipped (no prompt). + * Reviews are asked about FIRST. When autoReview is on, commit is always + * mandated (it happens before review) — so autoCommit is forced true and not + * asked about. When autoReview is off, autoCommit is asked as a stand-alone + * toggle. Fields explicitly set in the config YAML are skipped (no prompt). * Returns the selected options (or config defaults if cancelled). */ async function selectLoopOptions( @@ -136,17 +136,16 @@ async function selectLoopOptions( const explicit = config.execution.explicitKeys; // ── 1. Auto-review (asked FIRST) ── - // When enabled, a review gate runs BEFORE the commit: uncommitted changes - // are reviewed, and on a "fail" verdict the task is re-executed with the - // review feedback injected (looping until pass or maxReviewRetries is - // exhausted). Only then does the commit session run (when autoCommit is - // also enabled). + // When enabled, a commit is mandated before review (the task agent's + // changes are committed, then the complete diff is reviewed). On 'fail' + // the task is re-executed with review feedback (looping until pass or + // maxReviewRetries exhausted). On pass the worktree merges. let autoReview: boolean; if (explicit?.has("autoReview")) { autoReview = config.execution.autoReview; } else { const reviewChoice = await ctx.ui.select("Auto-review after each task?", [ - "Yes — review changes and loop on failures (re-execute until pass)", + "Yes — review the task commit and loop on failures (re-execute until pass)", "No — skip review", ]); autoReview = reviewChoice @@ -173,24 +172,15 @@ async function selectLoopOptions( } } - // ── 3. Auto-commit (asked AFTER review) ── - // When review is enabled, this gates whether the changes are committed - // after a passing review (or after retries exhaust). When review is - // disabled, this is a stand-alone "commit per task" toggle. + // ── 3. Auto-commit ── + // When autoReview is on, commit is always mandated (it happens before the + // review). autoCommit is forced true and not asked about. When review is + // disabled, autoCommit is asked as a stand-alone "commit per task" toggle. let autoCommit: boolean; - if (explicit?.has("autoCommit")) { + if (autoReview) { + autoCommit = true; // mandated by the review-gated flow + } else if (explicit?.has("autoCommit")) { autoCommit = config.execution.autoCommit; - } else if (autoReview) { - const commitChoice = await ctx.ui.select( - "Auto-commit after a passing review?", - [ - "Yes — commit changes once the review passes", - "No — leave changes uncommitted after review", - ], - ); - autoCommit = commitChoice - ? commitChoice.startsWith("Yes") - : config.execution.autoCommit; } else { const commitChoice = await ctx.ui.select("Auto-commit after each task?", [ "Yes — stage and commit changes automatically", diff --git a/src/executor.ts b/src/executor.ts index 6626417..46f32bb 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -15,7 +15,7 @@ import type { } from "@earendil-works/pi-coding-agent"; import { buildTaskPrompt, - buildReviewPromptUncommitted, + buildReviewPrompt, buildConflictResolutionPrompt, MAX_DIFF_BYTES, } from "./prompts"; @@ -42,6 +42,8 @@ import { writeFileSafe, ensureDir, captureGitCommits, + captureGitHead, + getCommitRangeDiff, hasUncommittedChanges, getGitStatusPorcelain, getGitDiff, @@ -805,6 +807,12 @@ async function executeTask( task.dependencies || [], ); + // Capture base HEAD before execution so the review-gated loop can diff + // the complete task output (baseRef..HEAD) across execution + fix attempts. + const baseRef = config.execution.autoReview + ? captureGitHead(worktreeDir) + : undefined; + // Run the task const result = await runTask( task, @@ -826,26 +834,57 @@ async function executeTask( let reviewRetries = 0; if (config.execution.autoReview) { - // ── Review-gated loop: review FIRST, re-execute on reject, commit on pass ── - // The review examines uncommitted changes. If the verdict is "fail", - // the task is re-executed with the review feedback injected into the - // prompt (up to maxReviewRetries). On pass (or after retries exhaust) - // the commit session runs when autoCommit is also enabled — otherwise - // the changes are left uncommitted for manual inspection. + // ── Review-gated loop: commit → review → re-execute on fail → merge on pass ── + // The commit is mandated — when the task agent didn't self-commit, a + // commit session handles it. Then the COMPLETE task diff (baseRef..HEAD) + // is reviewed. On 'fail' the task is re-executed with the review feedback + // injected (up to maxReviewRetries); after re-execution changes are + // committed again and the full diff is re-reviewed with the SAME baseRef + // so the reviewer sees the complete state, not just incremental fixes. + // On pass the changes are already committed — the worktree merges next. const maxRetries = config.execution.maxReviewRetries; let attempt = 0; try { - while (hasUncommittedChanges(worktreeDir)) { - const status = getGitStatusPorcelain(worktreeDir); - const reviewDiff = getGitDiff(worktreeDir); - if (!reviewDiff && !status) break; + // ── Ensure committed (commit session fallback) ── + // If the task agent didn't self-commit, a commit session handles it. + if (hasUncommittedChanges(worktreeDir)) { + const commitResult = await runCommitSession( + ctx, + config, + task, + worktreeDir, + currentModel, + roundRobin, + sendChatMessage, + ); + if (commitResult.success) { + finalCommitMessages = [ + ...finalCommitMessages, + ...commitResult.commitMessages, + ]; + finalCommitSummary = finalCommitSummary + ? `${finalCommitSummary}; ${commitResult.commitSummary}` + : commitResult.commitSummary; + } + } - const reviewPrompt = buildReviewPromptUncommitted( + // ── Review loop ── + // baseRef was captured before runTask (above). Each review iteration + // diffs the range baseRef..HEAD — the complete task output including + // all fix attempts. On re-execution the same baseRef is reused. + while (true) { + const reviewInfo = baseRef + ? getCommitRangeDiff(worktreeDir, baseRef) + : null; + if (!reviewInfo || !reviewInfo.diff) break; // nothing to review + + const reviewPrompt = buildReviewPrompt( task, project, - status, - reviewDiff, + reviewInfo.hash, + reviewInfo.subject, + reviewInfo.diff, config.prompts.projectContext, ); @@ -875,11 +914,15 @@ async function executeTask( `~ review for ${task.id} · ${task.title} — review session failed: ${reviewResult.error}`, { toolCalls: reviewToolCalls }, ); - break; // commit on autoCommit, otherwise leave uncommitted + break; // proceed with what we have (changes already committed) } const reviewText = reviewResult.text.trim(); - const review = extractReview(reviewText, task.id, "uncommitted"); + const review = extractReview( + reviewText, + task.id, + reviewInfo.hash, + ); finalReview = review ?? undefined; // Persist structured review JSON when opted in. @@ -897,7 +940,7 @@ async function executeTask( review && (review.verdict === "pass" || review.verdict === "warn") ) { - // Review passed — proceed to commit. + // Review passed — all changes are committed; merge will follow. const label = `${verdictGlyph(review.verdict)} ${verdictSummary(review)}`; const savedHint = reviewPath ? ` · saved to ${reviewPath}` : ""; sendChatMessage?.( @@ -909,7 +952,7 @@ async function executeTask( reviewResult: review, }, ); - break; // good to commit + break; // good to merge } // Review rejected (fail) or verdict not parsed. @@ -934,7 +977,7 @@ async function executeTask( } if (attempt >= maxRetries) { - // Retries exhausted. + // Retries exhausted — changes are already committed; proceed. if (config.execution.reviewBlockOnFail) { sendChatMessage?.( `✗ ${task.id} · ${task.title} — review rejected after ${maxRetries} retr${maxRetries === 1 ? "y" : "ies"} (reviewBlockOnFail)`, @@ -952,11 +995,9 @@ async function executeTask( return; } sendChatMessage?.( - config.execution.autoCommit - ? `~ review for ${task.id} · ${task.title} — max retries (${maxRetries}) exhausted, committing anyway` - : `~ review for ${task.id} · ${task.title} — max retries (${maxRetries}) exhausted, leaving changes as-is`, + `~ review for ${task.id} · ${task.title} — max retries (${maxRetries}) exhausted, proceeding with current state`, ); - break; // commit on autoCommit, otherwise leave uncommitted + break; // changes already committed — merge proceeds } attempt++; @@ -984,7 +1025,7 @@ async function executeTask( sendChatMessage?.( `~ re-execution for ${task.id} · ${task.title} failed: ${fixResult.error}`, ); - break; // commit what we have + break; // proceed with what we have } // Merge commit messages from the fix attempt. @@ -995,34 +1036,30 @@ async function executeTask( finalCommitSummary = finalCommitSummary ? `${finalCommitSummary}; ${fixResult.commitSummary ?? ""}` : (fixResult.commitSummary ?? ""); - // Loop back to review the updated changes. - } - // ── Commit (after review passes or retries exhausted) ── - // Only commit when autoCommit is enabled; review-only mode leaves - // changes uncommitted so the user can inspect them manually. - if ( - config.execution.autoCommit && - hasUncommittedChanges(worktreeDir) - ) { - const commitResult = await runCommitSession( - ctx, - config, - task, - worktreeDir, - currentModel, - roundRobin, - sendChatMessage, - ); - if (commitResult.success) { - finalCommitMessages = [ - ...finalCommitMessages, - ...commitResult.commitMessages, - ]; - finalCommitSummary = finalCommitSummary - ? `${finalCommitSummary}; ${commitResult.commitSummary}` - : commitResult.commitSummary; + // Ensure committed after re-execution (same commit fallback). + if (hasUncommittedChanges(worktreeDir)) { + const commitResult = await runCommitSession( + ctx, + config, + task, + worktreeDir, + currentModel, + roundRobin, + sendChatMessage, + ); + if (commitResult.success) { + finalCommitMessages = [ + ...finalCommitMessages, + ...commitResult.commitMessages, + ]; + finalCommitSummary = finalCommitSummary + ? `${finalCommitSummary}; ${commitResult.commitSummary}` + : commitResult.commitSummary; + } } + // Loop back to review with the same baseRef — the reviewer sees the + // complete diff (original work + fixes), not just incremental changes. } } catch (error) { sendChatMessage?.( diff --git a/src/types.ts b/src/types.ts index c7b5d93..0bed376 100644 --- a/src/types.ts +++ b/src/types.ts @@ -211,12 +211,13 @@ export interface RalpiConfig { models: string[]; /** Spawn a follow-up agent to commit changes after each task completes */ autoCommit: boolean; - /** Spawn a review agent to review uncommitted changes against the - * task description BEFORE committing. On a 'fail' verdict the task is - * re-executed with the review feedback injected (loops until pass or - * maxReviewRetries is exhausted). When autoCommit is also enabled, the - * commit runs after the review passes (or retries exhaust); otherwise - * changes are left uncommitted. */ + /** Spawn a review agent to review the task's committed changes against + * the task description. When autoReview is on, commit is mandated: + * changes are committed (via commit session fallback when the agent + * didn't self-commit), then the COMPLETE diff (baseRef..HEAD) is + * reviewed. On 'fail' the task is re-executed with feedback (loops + * until pass or maxReviewRetries). On pass the worktree merges. + * When autoReview is off, autoCommit controls standalone commit. */ autoReview: boolean; /** Persist the full review output to `.ralpi/reviews/.md`. * Only active when autoReview is true and the user opts in at loop start. */ @@ -236,13 +237,12 @@ export interface RalpiConfig { reviewTimeoutMs: number; /** Max review-fix re-execution attempts before giving up (0 = no retries; * review runs once, reject = stop). Active whenever autoReview is - * enabled. On exhaustion: if autoCommit is enabled, the changes are - * committed anyway; if reviewBlockOnFail is set, the task is marked - * failed instead of committing. */ + * enabled. On exhaustion the task proceeds with its committed changes + * (the worktree merges) unless reviewBlockOnFail is set. */ maxReviewRetries: number; /** When true, a 'fail' review verdict after exhausting maxReviewRetries - * marks the task as failed instead of leaving changes (committing when - * autoCommit, or leaving uncommitted otherwise). */ + * marks the task as failed instead of proceeding with its committed + * changes (the worktree does not merge). */ reviewBlockOnFail: boolean; /** Maximum total duration for the entire loop execution in milliseconds (0 = no limit). Checked between batches — in-progress tasks finish naturally. */ loopTimeoutMs: number; diff --git a/src/utils.ts b/src/utils.ts index ee97089..794b468 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -807,3 +807,76 @@ export function getLatestCommitDiff( return null; } } + +/** + * Capture the current HEAD commit SHA. Returns the full 40-char SHA, or + * undefined when not a git repo / git unavailable. Used to snapshot the + * worktree HEAD before a task runs so the review can diff the complete task + * output (baseRef..HEAD) — including any commits the task agent makes. + */ +export function captureGitHead(projectDir: string): string | undefined { + const { execSync } = require("node:child_process"); + try { + const sha = execSync("git rev-parse HEAD", { + cwd: projectDir, + encoding: "utf-8", + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + // Guard against injection — only accept hex SHAs. + return /^[0-9a-f]{7,40}$/i.test(sha) ? sha : undefined; + } catch { + return undefined; + } +} + +/** + * Get the diff from `baseRef` to HEAD — the complete set of committed changes + * made since the base reference. Used by the review-gated loop so the reviewer + * sees the full task diff (all commits, not just the latest) across execution + * attempts and re-execution fixes. `baseRef` must be a validated hex SHA from + * captureGitHead(). Returns the short HEAD hash, HEAD subject, and range diff, + * or null when git is unavailable / baseRef is invalid / no changes exist. + */ +export function getCommitRangeDiff( + projectDir: string, + baseRef: string, +): { hash: string; subject: string; diff: string } | null { + const { execSync } = require("node:child_process"); + + // Only pass validated hex SHAs to the shell. + if (!/^[0-9a-f]{7,40}$/i.test(baseRef)) return null; + + try { + execSync("git rev-parse --git-dir", { + cwd: projectDir, + stdio: "pipe", + }); + } catch { + return null; + } + + try { + const hash = execSync("git rev-parse --short HEAD", { + cwd: projectDir, + encoding: "utf-8", + }).trim(); + + const subject = execSync("git log -1 --format=%s", { + cwd: projectDir, + encoding: "utf-8", + }).trim(); + + // Diff from baseRef to HEAD — shows all committed changes made since + // the snapshot. Includes stat overview + full patch. + const diff = execSync(`git diff ${baseRef} HEAD --stat --patch`, { + cwd: projectDir, + encoding: "utf-8", + maxBuffer: 1024 * 1024, + }).trim(); + + if (!diff) return null; // no changes since baseRef + return { hash, subject, diff }; + } catch { + return null; + } +}