diff --git a/packages/e2e/README.md b/packages/e2e/README.md index 551f364a..9e3b74ac 100644 --- a/packages/e2e/README.md +++ b/packages/e2e/README.md @@ -86,6 +86,34 @@ and secret `E2E_GITHUB_TOKEN` before running it. Do not expose these credentials to untrusted pull-request code. Build the CLI in that job before running the suite so the artifact matches the checkout. +## Local auto-review loop + +`tests/local/cli-auto-review.test.ts` runs the built CLI's `--auto-review` loop +against a local bare Git remote and the deterministic fixture agent, with no +hosted PR, account, or network access. The fixture agent gains two prompt-driven +modes: `auto-review-feedback` (the bundled `review.md` prompt) prints only a +JSON verdict, and `auto-review-fix` (the bundled `fix.md` prompt) edits +`src/greeting.ts` without committing or pushing. The reviewer reports one +`medium` finding on iteration 1 and approves from iteration 2, so the loop +converges; `E2E_AUTO_REVIEW_STUCK=1` keeps every iteration unapproved to prove +`--auto-review-iterations` caps the review/fix cycles. Both modes are recorded +in `E2E_AGENT_INVOCATIONS` so the test asserts the exact +implement → review → fix → review sequence. + +The scenarios assert the pushed `feature/` branch, the exact +`src/greeting.ts` diff, the two commits ahead of base (the implementation plus +`fix: address PR review feedback`), and the +`DEVINTERN_OUTPUT_DIR//auto-review-summary.json` artifact +(`iterations`, `success`, and `history`). Per-iteration verdicts are saved under +`iteration-/feedback.json`. A non-approved loop still exits 0 and delivers +the branch; the local remote has no supported PR host, so `--create-pr` pushes +and exits without a PR URL. + +```sh +bun run --filter @getdevintern/code build +bun run --filter @devintern/e2e test +``` + ## Worker `worker add` option matrix The default local suite (`tests/local/worker-add-options.test.ts`) drives the built diff --git a/packages/e2e/docs/coverage-matrix.md b/packages/e2e/docs/coverage-matrix.md index ef030390..ca15e45e 100644 --- a/packages/e2e/docs/coverage-matrix.md +++ b/packages/e2e/docs/coverage-matrix.md @@ -68,7 +68,7 @@ independently of this repository. | GitHub: CI failure repair and retry budget | `gap` | — | Worker parity with the GitLab CI scenarios. | | GitHub: automatic merge-conflict resolution and scheduled mode | `gap` | — | Worker parity with the GitLab conflict scenario. | | GitHub: `@mention` handling | `gap` | — | — | -| GitHub: `--auto-review` loop and iteration cap | `gap` | — | Both providers; fake reviewer agent. | +| GitHub: `--auto-review` loop and iteration cap | `default` | `tests/local/cli-auto-review.test.ts` | Provider parity: real PR review feedback on GitHub/GitLab remains live/opt-in. | | GitLab Self-Managed: MR, default branch, invalid API credentials | `live` | `tests/code/gitlab.test.ts` | — | | GitLab Self-Managed: collision, rejected push, address-review | `live` | `tests/code/gitlab-lifecycle.test.ts` | Standalone `address-review` CLI options. | | GitLab Self-Managed worker: injected task, polling, CI repair/escalation, review, conflict | `live` | `tests/code/gitlab-worker.test.ts` | Automations, estimations, quiet hours, failover, orphan recovery, service, auto-update. | @@ -124,7 +124,7 @@ Run: `bun run --filter @devintern/e2e test` (build the CLI first); the sandbox s | `--force` | `gap` | — | Incomplete task rerun gate. | | `--no-git`, `--no-auto-commit` | `gap` | — | Branch/commit omissions. | | `--estimate` | `gap` | — | Per-tracker story-point write/comment. | -| `--auto-review`, `--auto-review-iterations` | `gap` | — | Review/fix loop and cap. | +| `--auto-review`, `--auto-review-iterations` | `default` | `tests/local/cli-auto-review.test.ts` (`bun run --filter @devintern/e2e test`) | Provider parity: live PR review feedback remains opt-in. | | `--env-file` | `gap` | — | Alternate credential source and precedence. | | `--max-turns`, `--hook-retries` | `partial` | helper passes `--hook-retries` | Failure/retry path. | | `--sandbox ` | `partial` | `tests/local/sandbox.test.ts` (per-provider fail-closed, removed srt/smolvm migration, native/goose rejection, unknown value) | Real run per provider remains optional. | @@ -190,7 +190,9 @@ and each task opens its own PR. 4. Trello lifecycle against a dedicated board (Layer 2), when that board and token exist. 5. GitHub worker parity: review polling, CI repair, conflict resolution, mention. -6. `--auto-review` loop and iteration cap on GitHub and GitLab. +6. ~~`--auto-review` loop and iteration cap on GitHub and GitLab~~ (the local + loop/cap is implemented by `tests/local/cli-auto-review.test.ts`; provider + parity with real PR review feedback remains live/opt-in). 7. `worker add` full option matrix (`--body-file`, idempotency, resume, wait, wait-timeout, workspace, validation and deduplication). 8. `worker scaffold`, `add-repo`, `run-now`, and `connect status` in temp homes. diff --git a/packages/e2e/helpers/fake-agent.ts b/packages/e2e/helpers/fake-agent.ts index bdcc54e0..ea65d8b4 100644 --- a/packages/e2e/helpers/fake-agent.ts +++ b/packages/e2e/helpers/fake-agent.ts @@ -5,7 +5,17 @@ import { appendFileSync, readFileSync, writeFileSync } from "node:fs"; // cannot: the same daemon runs implementation, review, CI-repair and conflict // jobs, so those tests identify the prompt by a seeded marker instead. const agentInput = process.argv.join(" "); +// The local auto-review loop sends the bundled review and fix prompts from the +// same one-shot CLI run that implemented the task. Detect those turns before +// the explicit E2E_AGENT_MODE so a single run can exercise +// implement → review → fix → review without the fixture pinning one mode. +const promptMode = agentInput.includes("You are reviewing a pull request") + ? "auto-review-feedback" + : agentInput.includes("## Feedback to Address") + ? "auto-review-fix" + : undefined; const mode = + promptMode ?? process.env.E2E_AGENT_MODE ?? (agentInput.includes("# Resolve Merge Conflicts") ? "conflict" @@ -73,6 +83,60 @@ if ( if (missingMarkers.length > 0) { throw new Error(`Seeded ${missingMarkers.join(" and ")} marker(s) did not reach the agent input`); } +// Auto-review reviewer turn: print only the JSON verdict the loop parses. The +// first iteration reports one blocking medium finding; a later iteration +// approves so the loop converges. E2E_AUTO_REVIEW_STUCK keeps every iteration +// unapproved to prove the iteration cap. +if (mode === "auto-review-feedback") { + const iteration = Number(agentInput.match(/\*\*Review Iteration\*\*:\s*(\d+)/)?.[1] ?? "1"); + const stuck = process.env.E2E_AUTO_REVIEW_STUCK === "1"; + if (!stuck && iteration > 1) { + console.log( + JSON.stringify({ + summary: "The review finding was addressed and no blocking issues remain.", + approved: true, + items: [], + }), + ); + } else { + console.log( + JSON.stringify({ + summary: "The greeting change works, but the exported value is not asserted.", + approved: false, + items: [ + { + priority: "medium", + confidence: "high", + category: "code-quality", + file: "src/greeting.ts", + line: "1", + issue: + "The fixture test only checks a Hello prefix, so it cannot catch a wrong greeting value.", + evidence: + "tests/greeting.test.ts:5 asserts only a Hello prefix, not the exact exported value", + suggestion: "Assert the exact exported greeting value in tests/greeting.test.ts.", + }, + ], + }), + ); + } + process.exit(0); +} +// Auto-review fixer turn: apply the suggested edit and leave the commit to the +// production loop. Accept an already-fixed file so a later fix turn is a no-op. +if (mode === "auto-review-fix") { + const fixedPath = "src/greeting.ts"; + const fixed = 'export const greeting = "Hello, reviewer!";\n'; + const current = readFileSync(fixedPath, "utf8"); + if (current !== fixed) { + if (current !== 'export const greeting = "Hello, world!";\n') { + throw new Error("Unexpected fixture contents; refusing to edit this workspace"); + } + writeFileSync(fixedPath, fixed); + } + console.log("Applied the review suggestion: src/greeting.ts now exports Hello, reviewer!"); + process.exit(0); +} const path = "src/greeting.ts"; if (mode === "no-change") { console.log( diff --git a/packages/e2e/helpers/fixture-workspace.ts b/packages/e2e/helpers/fixture-workspace.ts index a94f6bf1..5a97d529 100644 --- a/packages/e2e/helpers/fixture-workspace.ts +++ b/packages/e2e/helpers/fixture-workspace.ts @@ -171,6 +171,8 @@ export class FixtureWorkspace { skipComments?: boolean; /** Labels the CLI must apply to the created PR (`PR_LABELS`). */ prLabels?: string[]; + /** Additional CLI arguments appended after the fixture's defaults. */ + extraArgs?: string[]; } = {}, ) { return command( @@ -189,6 +191,7 @@ export class FixtureWorkspace { "--hook-retries", "0", "--no-update", + ...(options.extraArgs ?? []), ], this.cwd, { diff --git a/packages/e2e/tests/local/cli-auto-review.test.ts b/packages/e2e/tests/local/cli-auto-review.test.ts new file mode 100644 index 00000000..f53007d7 --- /dev/null +++ b/packages/e2e/tests/local/cli-auto-review.test.ts @@ -0,0 +1,162 @@ +import { expect, test } from "bun:test"; +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { cliPath, FixtureWorkspace } from "../../helpers/fixture-workspace"; +import { checkedCommand } from "../../helpers/process"; + +/** + * Seed a local bare Git remote so the run exercises the production git path + * without a hosted code host. `--create-pr` still pushes and exits 0 because + * no supported platform is detected. + * + * @param run - Fixture workspace whose isolated environment is reused + */ +async function prepareLocalRemote(run: FixtureWorkspace): Promise { + const remote = join(run.root, "remote.git"); + await checkedCommand(["git", "init", "--bare", remote], run.root, run.env); + await run.prepare(remote, "test-token", {}); +} + +/** + * Path of the auto-review summary the CLI writes under the output directory. + * + * @param run - Fixture workspace that ran the task + */ +function autoReviewDir(run: FixtureWorkspace): string { + return join(run.env.DEVINTERN_OUTPUT_DIR, run.taskKey.toLowerCase()); +} + +/** + * Read and parse a JSON artifact, failing with its path when it is absent. + * + * @param path - Artifact path inside the fixture's temporary output directory + */ +function readJson(path: string): Record { + if (!existsSync(path)) throw new Error(`Expected artifact was not written: ${path}`); + return JSON.parse(readFileSync(path, "utf8")) as Record; +} + +/** The exact base-to-head diff the two auto-review commits must produce. */ +const EXPECTED_GREETING_DIFF = [ + "diff --git a/src/greeting.ts b/src/greeting.ts", + "--- a/src/greeting.ts", + "+++ b/src/greeting.ts", + "@@ -1 +1 @@", + '-export const greeting = "Hello";', + '+export const greeting = "Hello, reviewer!";', +].join("\n"); + +test.skipIf(!existsSync(cliPath))( + "the local --auto-review loop fixes a finding, approves, and pushes both commits", + async () => { + const run = new FixtureWorkspace(); + try { + await prepareLocalRemote(run); + const result = await run.runTask({ + extraArgs: ["--auto-review", "--auto-review-iterations", "2"], + }); + + expect(result.exitCode).toBe(0); + // The local bare remote has no supported PR host: the documented behavior + // is still to push the branch and exit successfully, without a PR URL. + expect(await run.remoteSha(run.head)).toBe(await run.git("rev-parse", "HEAD")); + expect(await run.git("branch", "--show-current")).toBe(run.head); + + // The loop's fix commit lands on the same task branch: implementation + // plus exactly one review-fix commit. + expect(await run.git("rev-list", "--count", `${run.base}..HEAD`)).toBe("2"); + expect(await run.git("log", "-1", "--format=%s")).toBe("fix: address PR review feedback"); + expect(await run.git("diff", "--name-only", `${run.base}...HEAD`)).toBe("src/greeting.ts"); + expect(await run.git("show", "HEAD:src/greeting.ts")).toBe( + 'export const greeting = "Hello, reviewer!";', + ); + const diff = await run.git("diff", `${run.base}...HEAD`, "--", "src/greeting.ts"); + expect( + diff + .split("\n") + .filter((line) => !line.startsWith("index ")) + .join("\n"), + ).toBe(EXPECTED_GREETING_DIFF); + + const outputDir = autoReviewDir(run); + const summary = readJson(join(outputDir, "auto-review-summary.json")); + expect(summary.iterations).toBe(2); + expect(summary.success).toBe(true); + expect(summary.history).toHaveLength(2); + + const firstFeedback = readJson(join(outputDir, "iteration-1", "feedback.json")) as { + items: Array>; + }; + expect(firstFeedback.items).toHaveLength(1); + expect(firstFeedback.items[0]).toMatchObject({ + priority: "medium", + category: "code-quality", + }); + expect(firstFeedback.items[0].issue).toBeTruthy(); + expect(firstFeedback.items[0].suggestion).toBeTruthy(); + expect(firstFeedback.items[0].evidence).toBeTruthy(); + + // One implementation turn, then review → fix → review: the second review + // approves and ends the loop before the cap. + expect(run.agentInvocations().map((invocation) => invocation.mode)).toEqual([ + "implement", + "auto-review-feedback", + "auto-review-fix", + "auto-review-feedback", + ]); + + expect(result.stdout).toContain("Auto-review loop finished after 2 iteration(s)"); + // The loop stops on "no important issues to address" before its + // `approved` check, so the converged success is this line plus the + // success status, not the unreachable `PR approved` branch. + expect(result.stdout).toContain("No important issues to address. Review loop complete!"); + expect(result.stdout).toContain("Status: ✅ Success"); + expect(run.runRecords().map((record) => record.status)).toEqual(["succeeded"]); + expect(await run.git("status", "--porcelain")).toBe(""); + } finally { + run.dispose(); + } + }, + 60_000, +); + +test.skipIf(!existsSync(cliPath))( + "the --auto-review-iterations cap stops a non-converging review after one cycle", + async () => { + const run = new FixtureWorkspace(); + try { + await prepareLocalRemote(run); + run.env.E2E_AUTO_REVIEW_STUCK = "1"; + const result = await run.runTask({ + extraArgs: ["--auto-review", "--auto-review-iterations", "1"], + }); + + // A non-approved review is reported, not fatal: the branch is still + // delivered with the fix commit. + expect(result.exitCode).toBe(0); + expect(run.agentInvocations().map((invocation) => invocation.mode)).toEqual([ + "implement", + "auto-review-feedback", + "auto-review-fix", + ]); + expect(await run.remoteSha(run.head)).toBe(await run.git("rev-parse", "HEAD")); + expect(await run.git("rev-list", "--count", `${run.base}..HEAD`)).toBe("2"); + expect(await run.git("show", "HEAD:src/greeting.ts")).toBe( + 'export const greeting = "Hello, reviewer!";', + ); + + const summary = readJson(join(autoReviewDir(run), "auto-review-summary.json")); + expect(summary.iterations).toBe(1); + expect(summary.success).toBe(false); + expect(summary.history).toHaveLength(1); + + expect(result.stdout).toContain("Auto-review loop finished after 1 iteration(s)"); + expect(result.stdout).toContain("Incomplete"); + expect(run.runRecords().map((record) => record.status)).toEqual(["succeeded"]); + expect(await run.git("status", "--porcelain")).toBe(""); + } finally { + run.dispose(); + } + }, + 60_000, +);