From 2c377dcb7ef8fb3242e67e7cb43cfe91bd4c0f1d Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Sun, 28 Jun 2026 23:24:43 +0530 Subject: [PATCH 01/13] ci(release): gate signed release on the release environment (#2249) The Desktop release fires on any desktop-v* tag push and consumes the repo-level signing secrets, so any write-access collaborator can cut a signed prod build. Reference the `release` environment from the build jobs so a designated reviewer must approve before the secrets are used. Takes effect once required reviewers are configured on that environment. Co-authored-by: Claude Opus 4.8 --- .github/workflows/frontend-release.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/frontend-release.yml b/.github/workflows/frontend-release.yml index 93b8361a8..281532985 100644 --- a/.github/workflows/frontend-release.yml +++ b/.github/workflows/frontend-release.yml @@ -41,6 +41,11 @@ jobs: matrix: os: [macos-latest, windows-latest, ubuntu-latest] runs-on: ${{ matrix.os }} + # Gates the signed release behind the `release` environment's required + # reviewers: any write-access user can push a desktop-v* tag, but only + # designated approvers can let the build that consumes the signing secrets + # proceed. Configure reviewers in repo Settings > Environments > release. + environment: release permissions: contents: write defaults: @@ -157,6 +162,7 @@ jobs: # finishes before publish-feed, but the feed is not gated on it. release-intel: runs-on: macos-13 + environment: release # same approval gate as the matrix release job permissions: contents: write defaults: From ff23e66055667e2e430616a540cbdd32151dbad2 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Sun, 28 Jun 2026 23:24:57 +0530 Subject: [PATCH 02/13] chore(release): stamp desktop app version 0.10.0 (#2248) Stable release builds read the version from frontend/package.json (the release workflow does not stamp from the git tag, unlike nightly), so the 0.10.0 prod cut needs this committed at the tagged commit. Otherwise the app ships as 0.0.0 and the stable update channel never advances. Co-authored-by: Claude Opus 4.8 --- frontend/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/package.json b/frontend/package.json index bca608324..a70a05a57 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "agent-orchestrator", "productName": "Agent Orchestrator", - "version": "0.0.0", + "version": "0.10.0", "private": true, "description": "Electron + TypeScript frontend for the agent-orchestrator rewrite", "author": "Agent Orchestrator", From 965e3887928fb6b0b2e980da9c4ef50336eb7d49 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Sun, 28 Jun 2026 23:36:06 +0530 Subject: [PATCH 03/13] fix(update): bundle app-update.yml so electron-updater can install (forge omits it) (#2244) * fix(update): generate app-update.yml at build time; drop runtime setFeedURL electron-forge does not emit app-update.yml; electron-updater requires it at process.resourcesPath to resolve the GitHub release feed. Without it every packaged app threw ENOENT on the first download attempt, making updates detected but never installed. Two-part fix: - forge.config.ts: add postPackage hook that writes app-update.yml into each platform's Resources dir (baked from AO_RELEASE_REPO so fork builds point at the fork, prod at AgentWrapper/agent-orchestrator). - auto-updater.ts: remove setFeedURL + repo() + DEFAULT_RELEASE_REPO; configureFeed now only sets channel + allowDowngrade. electron-updater auto-loads the bundled yml on the first checkForUpdates call. Co-Authored-By: Claude Opus 4.8 * chore: format with prettier [skip ci] * fix(update): generate app-update.yml before signing, not after The postPackage hook wrote app-update.yml into Contents/Resources AFTER osxSign sealed the bundle, so the added file was unsealed and macOS reported the app as "damaged" (codesign: "a sealed resource is missing or invalid"). Generate it in a prePackage hook and ship it via packagerConfig.extraResource, so it is copied into the bundle and signed as part of the seal. The generated app-update.yml is gitignored. Co-Authored-By: Claude Opus 4.8 * chore: format with prettier [skip ci] * ci(release): move Intel leg to macos-15-intel and gate the feed on it macos-13 is deprecated and has no runner capacity (multi-hour queues), so the detached release-intel leg never completed. Switch it to the supported macos-15-intel image in both the release and nightly workflows. Now that the Intel leg gets a runner and builds + signs the x64 installer reliably (verified on fork v0.10.10), re-couple it into publish-feed (needs: [release, release-intel]) so latest-mac.yml / nightly-mac.yml always carry the x64 entry and Intel macOS users receive auto-updates. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: github-actions[bot] --- .github/workflows/frontend-nightly.yml | 14 +++++++------- .github/workflows/frontend-release.yml | 17 +++++++++-------- frontend/.gitignore | 1 + frontend/forge.config.ts | 24 +++++++++++++++++++++++- frontend/src/main/auto-updater.ts | 20 +++++--------------- 5 files changed, 45 insertions(+), 31 deletions(-) create mode 100644 frontend/.gitignore diff --git a/.github/workflows/frontend-nightly.yml b/.github/workflows/frontend-nightly.yml index b58e1524c..ec3c400ab 100644 --- a/.github/workflows/frontend-nightly.yml +++ b/.github/workflows/frontend-nightly.yml @@ -93,15 +93,15 @@ jobs: # macOS signing + notarization env is exported by the "macOS signing # setup" step above via $GITHUB_ENV; the cert is in the keychain. - # Dedicated Intel (darwin-x64) nightly build leg, decoupled from the release - # matrix so the scarce macos-13 runner does not block publish-feed. Mirrors - # the release job's macOS steps exactly. publish-feed stays needs: release - # only (three fast legs); the x64 feed entry appears if this job finishes - # first, but the feed is not gated on it. + # Dedicated Intel (darwin-x64) nightly build leg. Kept separate from the + # release matrix (only a macOS host can build the x64 installer); publish-feed + # now waits on it so the x64 entry is present in nightly-mac.yml and Intel + # nightly users get auto-updates. Mirrors the release job's macOS steps exactly. + # Runner is macos-15-intel (macos-13 deprecated/no capacity; see release wf). release-intel: needs: guard if: needs.guard.outputs.should_build == 'true' - runs-on: macos-13 + runs-on: macos-15-intel permissions: contents: write defaults: @@ -161,7 +161,7 @@ jobs: # assets. The nightly version is only stamped in-memory on the build runners, # so derive the tag from the guard job's computed version, not package.json. publish-feed: - needs: [guard, release] + needs: [guard, release, release-intel] runs-on: ubuntu-latest permissions: contents: write diff --git a/.github/workflows/frontend-release.yml b/.github/workflows/frontend-release.yml index 281532985..06af8c680 100644 --- a/.github/workflows/frontend-release.yml +++ b/.github/workflows/frontend-release.yml @@ -92,7 +92,7 @@ jobs: # release the publish step just created, with --clobber (so re-runs replace). # # Arch-coverage note: each runner only builds its host arch. macos-latest - # (Apple silicon) produces darwin-arm64; macos-13 (Intel x64) produces + # (Apple silicon) produces darwin-arm64; macos-15-intel (Intel x64) produces # darwin-x64. Both macOS runners run this step (see the `if` below), so the # arm64 and x64 stable aliases are now both built (spec §8). # The Linux stable name is undecided (spec §11.3), so we build the deb/rpm @@ -155,13 +155,14 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - # Dedicated Intel (darwin-x64) build leg, decoupled from the release matrix so - # the scarce macos-13 runner does not block publish-feed. publish-feed needs - # only the three fast legs (release); this job uploads the x64 installer and - # stable alias independently. feed.mjs includes the x64 entry if this job - # finishes before publish-feed, but the feed is not gated on it. + # Dedicated Intel (darwin-x64) build leg. Kept a separate job (not in the + # release matrix) because only a macOS host can build the x64 installer, but + # publish-feed now waits on it (needs: [release, release-intel]) so the x64 + # entry is always present in latest-mac.yml and Intel users get auto-updates. + # Runner is macos-15-intel: macos-13 is deprecated/has no capacity (multi-hour + # queues), so this leg uses the supported Intel x64 image. release-intel: - runs-on: macos-13 + runs-on: macos-15-intel environment: release # same approval gate as the matrix release job permissions: contents: write @@ -216,7 +217,7 @@ jobs: # assets and upload them to the same release. Runs once, on Linux: it only # hashes already-built artifacts, so it needs no per-OS runner. publish-feed: - needs: release + needs: [release, release-intel] runs-on: ubuntu-latest permissions: contents: write diff --git a/frontend/.gitignore b/frontend/.gitignore new file mode 100644 index 000000000..8f18fb362 --- /dev/null +++ b/frontend/.gitignore @@ -0,0 +1 @@ +app-update.yml diff --git a/frontend/forge.config.ts b/frontend/forge.config.ts index 7c3cd5e42..fa7fd35aa 100644 --- a/frontend/forge.config.ts +++ b/frontend/forge.config.ts @@ -2,6 +2,7 @@ import type { ForgeConfig } from "@electron-forge/shared-types"; import { VitePlugin } from "@electron-forge/plugin-vite"; import MakerNSIS from "./makers/maker-nsis"; import MakerAppImage from "./makers/maker-appimage"; +import { writeFileSync } from "node:fs"; // Default GitHub release target (production). aoagents was the temporary rewrite // home; releases land on AgentWrapper (spec §1.1). @@ -30,7 +31,7 @@ const config: ForgeConfig = { // (.icns on macOS, .ico on Windows); Linux menu icons come from the // deb/rpm makers below, and the runtime window icon from src/main.ts. icon: "assets/icon", - extraResource: ["daemon", "assets/icon.png"], + extraResource: ["daemon", "assets/icon.png", "app-update.yml"], // Notarization. Two paths: // - CI: an App Store Connect API key. APPLE_API_KEY is a PATH to the .p8 // (the workflow decodes APPLE_API_KEY_BASE64 to a temp file), plus the @@ -53,6 +54,27 @@ const config: ForgeConfig = { } : undefined, }, + hooks: { + // electron-forge does not generate app-update.yml (electron-builder does); + // electron-updater reads it from the app's Resources dir at runtime to know + // which GitHub repo to pull from, else it throws ENOENT during download. + // Generate it in prePackage (BEFORE osxSign) and ship it via extraResource + // above, so it is copied into the bundle and SIGNED as part of the seal. + // Writing it after signing (a postPackage hook) adds an unsealed resource + // and macOS reports the app as "damaged". owner/repo are baked from + // AO_RELEASE_REPO at build time. + prePackage: async () => { + const { owner, name } = parseReleaseRepo(process.env.AO_RELEASE_REPO); + const yml = [ + "provider: github", + `owner: ${owner}`, + `repo: ${name}`, + "updaterCacheDirName: agent-orchestrator-updater", + "", + ].join("\n"); + writeFileSync("app-update.yml", yml); + }, + }, rebuildConfig: {}, makers: [ // Windows installer: NSIS via electron-builder (see makers/maker-nsis.ts). diff --git a/frontend/src/main/auto-updater.ts b/frontend/src/main/auto-updater.ts index ea6057998..bf8fceb82 100644 --- a/frontend/src/main/auto-updater.ts +++ b/frontend/src/main/auto-updater.ts @@ -10,22 +10,12 @@ import { type UpdateStatus, } from "./update-settings"; -// Default release repo, mirroring backend cli.releaseRepo. Override via env for -// fork test builds (AO_RELEASE_REPO=owner/repo). -const DEFAULT_RELEASE_REPO = "AgentWrapper/agent-orchestrator"; - -function repo(): { owner: string; name: string } { - const [owner, name] = (process.env.AO_RELEASE_REPO || DEFAULT_RELEASE_REPO).split("/"); - if (owner && name) return { owner, name }; - const [defOwner, defName] = DEFAULT_RELEASE_REPO.split("/"); - return { owner: defOwner, name: defName }; -} - -// configureFeed points electron-updater at the GitHub Releases feed for the -// chosen channel. Shared by the launch auto-check and the manual Settings flow. +// configureFeed sets the update channel on electron-updater. The repo/owner +// are loaded automatically from app-update.yml (written by forge.config.ts's +// postPackage hook into the app's Resources dir at build time). No runtime env +// or setFeedURL call is needed; electron-updater reads the bundled yml on first +// checkForUpdates. function configureFeed(channel: UpdateChannel): void { - const { owner, name } = repo(); - autoUpdater.setFeedURL({ provider: "github", owner, repo: name }); autoUpdater.channel = channel; // "latest" | "nightly" autoUpdater.allowDowngrade = true; // permits a nightly -> stable channel switch } From 6b56b1c2453304bcee58d60349dd9d9a7dc6b0f7 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Mon, 29 Jun 2026 03:37:49 +0530 Subject: [PATCH 04/13] fix(update): enable allowPrerelease on the nightly channel (#2265) Nightly builds publish as GitHub prereleases. configureFeed set channel and allowDowngrade but never allowPrerelease, which defaults to false, so electron-updater only inspected the latest NON-prerelease release (e.g. v0.10.0) and 404d looking for nightly-mac.yml there. The nightly channel therefore never resolved, regardless of whether a nightly was published. Set allowPrerelease = (channel === nightly): nightly scans prereleases and finds the nightly feed; stable stays on non-prereleases only. Co-authored-by: Claude Opus 4.8 --- frontend/src/main/auto-updater.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frontend/src/main/auto-updater.ts b/frontend/src/main/auto-updater.ts index bf8fceb82..1c8527b15 100644 --- a/frontend/src/main/auto-updater.ts +++ b/frontend/src/main/auto-updater.ts @@ -17,6 +17,11 @@ import { // checkForUpdates. function configureFeed(channel: UpdateChannel): void { autoUpdater.channel = channel; // "latest" | "nightly" + // Nightly builds ship as GitHub *prereleases*. With allowPrerelease false + // (the default) electron-updater only inspects the latest NON-prerelease + // release and looks for nightly-mac.yml there, which 404s. Enable prerelease + // scanning on the nightly channel only; stable must never pull prereleases. + autoUpdater.allowPrerelease = channel === "nightly"; autoUpdater.allowDowngrade = true; // permits a nightly -> stable channel switch } From 92a52f09929e78305b0962387638049b1c0ab9d2 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Mon, 29 Jun 2026 03:58:19 +0530 Subject: [PATCH 05/13] ci: retry transient macOS sign/notary flakes in the publish step (#2266) Releases fail intermittently on transient macOS issues that hit EITHER macOS leg, not just Intel: Apple notary -1009 connection-offline (seen on macos-latest/arm64) and osx-sign keychain races, code object is not signed at all (seen on macos-15-intel). Both pass on a manual re-run. Wrap npm run publish in a 3x retry with backoff in all four release/nightly publish steps so these self-heal in-place instead of failing the run (and, for nightly, skipping the whole feed). Keeps the Intel leg coupled so the x64 feed entry stays guaranteed. Co-authored-by: Claude Opus 4.8 --- .github/workflows/frontend-nightly.yml | 22 ++++++++++++++++++++-- .github/workflows/frontend-release.yml | 22 ++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/.github/workflows/frontend-nightly.yml b/.github/workflows/frontend-nightly.yml index ec3c400ab..b56de27d1 100644 --- a/.github/workflows/frontend-nightly.yml +++ b/.github/workflows/frontend-nightly.yml @@ -85,7 +85,16 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish - run: npm run publish + run: | + # Retry transient macOS sign/notary flakes (Apple notary -1009, + # osx-sign keychain races) so the build self-heals instead of failing. + for i in 1 2 3; do + if npm run publish; then exit 0; fi + echo "::warning::publish attempt $i/3 failed (likely a transient macOS sign/notary flake)" + [ "$i" -lt 3 ] && sleep 30 + done + echo "::error::publish failed after 3 attempts" >&2 + exit 1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AO_RELEASE_REPO: ${{ github.repository }} @@ -135,7 +144,16 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish - run: npm run publish + run: | + # Retry transient macOS sign/notary flakes (Apple notary -1009, + # osx-sign keychain races) so the build self-heals instead of failing. + for i in 1 2 3; do + if npm run publish; then exit 0; fi + echo "::warning::publish attempt $i/3 failed (likely a transient macOS sign/notary flake)" + [ "$i" -lt 3 ] && sleep 30 + done + echo "::error::publish failed after 3 attempts" >&2 + exit 1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AO_RELEASE_REPO: ${{ github.repository }} diff --git a/.github/workflows/frontend-release.yml b/.github/workflows/frontend-release.yml index 06af8c680..ccec8e3cb 100644 --- a/.github/workflows/frontend-release.yml +++ b/.github/workflows/frontend-release.yml @@ -76,7 +76,16 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish - run: npm run publish + run: | + # Retry transient macOS sign/notary flakes (Apple notary -1009, + # osx-sign keychain races) so the build self-heals instead of failing. + for i in 1 2 3; do + if npm run publish; then exit 0; fi + echo "::warning::publish attempt $i/3 failed (likely a transient macOS sign/notary flake)" + [ "$i" -lt 3 ] && sleep 30 + done + echo "::error::publish failed after 3 attempts" >&2 + exit 1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AO_RELEASE_REPO: ${{ github.repository }} @@ -191,7 +200,16 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish - run: npm run publish + run: | + # Retry transient macOS sign/notary flakes (Apple notary -1009, + # osx-sign keychain races) so the build self-heals instead of failing. + for i in 1 2 3; do + if npm run publish; then exit 0; fi + echo "::warning::publish attempt $i/3 failed (likely a transient macOS sign/notary flake)" + [ "$i" -lt 3 ] && sleep 30 + done + echo "::error::publish failed after 3 attempts" >&2 + exit 1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AO_RELEASE_REPO: ${{ github.repository }} From 2cd3b88cfac03b6b972d084cfc492f48d1969217 Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Mon, 29 Jun 2026 15:44:30 +0530 Subject: [PATCH 06/13] fix(ci): Windows publish shell + fresh-install smoke check (#2267) (#2269) * fix(ci): pin publish step to bash so the retry loop runs on Windows The 3x retry wrapper added in #2266 is bash syntax, but the release matrix Publish step inherited the runner default shell, which is PowerShell on windows-latest. That made every Windows publish fail with a ParserError. Pin shell: bash on all four publish steps. Co-Authored-By: Claude Opus 4.8 * fix(ci): point fresh-install smoke check at an empty release repo (#2267) The container check asserts `ao start` fails cleanly on a fresh box with no published asset. Now that AgentWrapper publishes a linux-x64 AppImage, an unpinned smoke binary downloads it and exits 0, tripping the assertion. Build the smoke binary against a release repo with no assets so the fetch path deterministically 404s and start exits non-zero with a clear error, preserving the test's intent without depending on what the real repo publishes. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .github/workflows/frontend-nightly.yml | 2 ++ .github/workflows/frontend-release.yml | 2 ++ test/cli/Dockerfile | 8 +++++++- test/cli/install-check.sh | 13 +++++++------ 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/.github/workflows/frontend-nightly.yml b/.github/workflows/frontend-nightly.yml index b56de27d1..c441b0bd7 100644 --- a/.github/workflows/frontend-nightly.yml +++ b/.github/workflows/frontend-nightly.yml @@ -85,6 +85,7 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish + shell: bash run: | # Retry transient macOS sign/notary flakes (Apple notary -1009, # osx-sign keychain races) so the build self-heals instead of failing. @@ -144,6 +145,7 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish + shell: bash run: | # Retry transient macOS sign/notary flakes (Apple notary -1009, # osx-sign keychain races) so the build self-heals instead of failing. diff --git a/.github/workflows/frontend-release.yml b/.github/workflows/frontend-release.yml index ccec8e3cb..e588a7f0d 100644 --- a/.github/workflows/frontend-release.yml +++ b/.github/workflows/frontend-release.yml @@ -76,6 +76,7 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish + shell: bash run: | # Retry transient macOS sign/notary flakes (Apple notary -1009, # osx-sign keychain races) so the build self-heals instead of failing. @@ -200,6 +201,7 @@ jobs: apple-api-issuer: ${{ secrets.APPLE_API_ISSUER }} apple-signing-identity: ${{ secrets.APPLE_SIGNING_IDENTITY }} - name: Publish + shell: bash run: | # Retry transient macOS sign/notary flakes (Apple notary -1009, # osx-sign keychain races) so the build self-heals instead of failing. diff --git a/test/cli/Dockerfile b/test/cli/Dockerfile index ce947d148..43cb951b9 100644 --- a/test/cli/Dockerfile +++ b/test/cli/Dockerfile @@ -18,7 +18,13 @@ RUN cd backend && go mod download COPY backend ./backend # Pure-Go SQLite (modernc) builds fine with CGO disabled -> a static binary. -RUN cd backend && CGO_ENABLED=0 go build -trimpath -o /out/ao ./cmd/ao +# Build against a release repo with NO published assets so `ao start` reaches the +# fetch path and fails cleanly (404) on a fresh box. The real AgentWrapper repo +# now publishes a linux-x64 AppImage, so an unpinned binary would download it and +# exit 0, defeating the fresh-install assertion below. See install-check.sh. +RUN cd backend && CGO_ENABLED=0 go build -trimpath \ + -ldflags "-X github.com/aoagents/agent-orchestrator/backend/internal/cli.releaseRepo=AgentWrapper/ao-fresh-install-fixture" \ + -o /out/ao ./cmd/ao # ---- stage 2: a clean machine with NO Go toolchain, just like an end user ---- FROM debian:bookworm-slim AS run diff --git a/test/cli/install-check.sh b/test/cli/install-check.sh index 3af4756c3..96c2e60a1 100755 --- a/test/cli/install-check.sh +++ b/test/cli/install-check.sh @@ -22,12 +22,13 @@ echo "ao binary : $(command -v "$AO_BIN")" # `ao start` is now the desktop-app launcher: it resolves an installed app or # fetches the release, then opens it (it no longer runs a daemon). On a fresh -# container there is no installed app, so start reaches the fetch path. With no -# published asset for this platform it must exit non-zero with a clear `ao -# start:` error (an unreachable/404 download on amd64, or an unsupported-arch -# error on arm64), never a panic or a silent success. The full launcher -# behaviour is covered by the Go e2e suite; this only proves the fresh-box path -# is sane on whatever arch the runner uses. +# container there is no installed app, so start reaches the fetch path. The smoke +# binary is built against a release repo with no published assets (see +# Dockerfile), so the fetch deterministically 404s and start must exit non-zero +# with a clear `ao start:` error (an unreachable/404 download on amd64, or an +# unsupported-arch error on arm64), never a panic or a silent success. The full +# launcher behaviour is covered by the Go e2e suite; this only proves the +# fresh-box path is sane on whatever arch the runner uses. if err="$("$AO_BIN" start 2>&1)"; then fail "start unexpectedly succeeded on a fresh machine with no installed app" fi From d87b763e34aeb5dd1d070a020c0c399f78ad5c38 Mon Sep 17 00:00:00 2001 From: PRADEEP SAHU Date: Mon, 29 Jun 2026 16:16:58 +0530 Subject: [PATCH 07/13] UI : use React.Memo position for SelectContent to prevent scroll reset (#2261) * fix(ui): use popper position for SelectContent to prevent scroll reset * fix(ui): use React.memo to prevent scroll resets without breaking popper position --- .../src/renderer/components/CreateProjectAgentSheet.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/src/renderer/components/CreateProjectAgentSheet.tsx b/frontend/src/renderer/components/CreateProjectAgentSheet.tsx index ea4a7339b..0f9a75f18 100644 --- a/frontend/src/renderer/components/CreateProjectAgentSheet.tsx +++ b/frontend/src/renderer/components/CreateProjectAgentSheet.tsx @@ -1,6 +1,6 @@ import * as Dialog from "@radix-ui/react-dialog"; import { X } from "lucide-react"; -import { useEffect, useState } from "react"; +import { memo, useEffect, useState } from "react"; import { AGENT_OPTIONS } from "../lib/agent-options"; import { Button } from "./ui/button"; import { Label } from "./ui/label"; @@ -108,7 +108,7 @@ export function CreateProjectAgentSheet({ ); } -export function RequiredAgentField({ +export const RequiredAgentField = memo(function RequiredAgentField({ id, invalid = false, label, @@ -142,4 +142,4 @@ export function RequiredAgentField({ ); -} +}); From cb456bb288744778e21377407e4eabd6a3898235 Mon Sep 17 00:00:00 2001 From: neversettle <41864816+neversettle17-101@users.noreply.github.com> Date: Mon, 29 Jun 2026 18:20:28 +0530 Subject: [PATCH 09/13] feat: show multi-PR review status (#2193) * feat: support multi-pr review runs * fix: avoid review state status stutter * feat: batch review delivery by trigger * test: update review inspector mocks * chore: fold review batch migration into 0020 * fix: submit multi-pr reviews as one batch * fix: make queued reviews autonomous * fix: clarify multi-pr review submit prompt * feat: show multi-pr review status * chore: format with prettier [skip ci] * feat: simplify multi-pr review summary * chore: format with prettier [skip ci] * fix: match multi-pr review card design * chore: format with prettier [skip ci] * fix: remove review session label --------- Co-authored-by: github-actions[bot] Co-authored-by: Vaibhaav --- backend/internal/cli/review.go | 95 +++++-- backend/internal/cli/review_test.go | 26 ++ backend/internal/domain/review.go | 14 +- backend/internal/httpd/apispec/openapi.yaml | 75 +++++- .../internal/httpd/apispec/specgen/build.go | 15 +- backend/internal/httpd/controllers/reviews.go | 81 ++++-- .../httpd/controllers/reviews_test.go | 87 ++++++- backend/internal/lifecycle/manager_test.go | 65 ++++- backend/internal/lifecycle/reactions.go | 59 ++++- backend/internal/ports/reviewer.go | 12 + backend/internal/review/launcher.go | 4 + backend/internal/review/planner.go | 95 +++++++ backend/internal/review/planner_test.go | 68 +++++ backend/internal/review/prompt.go | 43 +++- backend/internal/review/prompt_test.go | 37 +++ backend/internal/review/review.go | 238 +++++++++++------- backend/internal/review/review_test.go | 148 +++++++++-- backend/internal/service/review/review.go | 174 +++++++++++-- .../internal/service/review/review_test.go | 175 ++++++++++++- backend/internal/storage/sqlite/gen/models.go | 1 + .../internal/storage/sqlite/gen/review.sql.go | 83 +++++- .../0020_review_run_unique_pr_sha.sql | 88 +++++++ .../storage/sqlite/queries/review.sql | 20 +- .../storage/sqlite/store/review_store.go | 35 ++- .../storage/sqlite/store/review_store_test.go | 70 +++++- frontend/src/api/schema.ts | 37 ++- .../components/SessionInspector.test.tsx | 91 ++++++- .../renderer/components/SessionInspector.tsx | 210 +++++++++------- frontend/src/renderer/styles.css | 200 ++++++++++++++- 29 files changed, 1985 insertions(+), 361 deletions(-) create mode 100644 backend/internal/review/planner.go create mode 100644 backend/internal/review/planner_test.go create mode 100644 backend/internal/review/prompt_test.go create mode 100644 backend/internal/storage/sqlite/migrations/0020_review_run_unique_pr_sha.sql diff --git a/backend/internal/cli/review.go b/backend/internal/cli/review.go index fa9046f96..20207978f 100644 --- a/backend/internal/cli/review.go +++ b/backend/internal/cli/review.go @@ -1,6 +1,7 @@ package cli import ( + "encoding/json" "errors" "fmt" "io" @@ -15,29 +16,42 @@ import ( // reviewRun mirrors the daemon's domain.ReviewRun for the CLI client. type reviewRun struct { - ID string `json:"id"` - SessionID string `json:"sessionId"` - Harness string `json:"harness"` - PRURL string `json:"prUrl"` - TargetSHA string `json:"targetSha"` - Status string `json:"status"` - Verdict string `json:"verdict"` - Body string `json:"body"` - CreatedAt time.Time `json:"createdAt"` + ID string `json:"id"` + SessionID string `json:"sessionId"` + BatchID string `json:"batchId"` + Harness string `json:"harness"` + PRURL string `json:"prUrl"` + TargetSHA string `json:"targetSha"` + Status string `json:"status"` + Verdict string `json:"verdict"` + Body string `json:"body"` + GithubReviewID string `json:"githubReviewId"` + CreatedAt time.Time `json:"createdAt"` + DeliveredAt *time.Time `json:"deliveredAt,omitempty"` } // reviewRunResponse mirrors controllers.ReviewRunResponse. type reviewRunResponse struct { - Review reviewRun `json:"review"` - ReviewerHandleID string `json:"reviewerHandleId"` + Review reviewRun `json:"review"` + Reviews []reviewRun `json:"reviews"` + ReviewerHandleID string `json:"reviewerHandleId"` +} + +// submitReviewItem mirrors controllers.SubmitReviewItem. +type submitReviewItem struct { + RunID string `json:"runId"` + Verdict string `json:"verdict"` + Body string `json:"body,omitempty"` + GithubReviewID string `json:"githubReviewId,omitempty"` } // submitReviewRequest mirrors controllers.SubmitReviewInput. type submitReviewRequest struct { - RunID string `json:"runId"` - Verdict string `json:"verdict"` - Body string `json:"body"` - GithubReviewID string `json:"githubReviewId"` + RunID string `json:"runId,omitempty"` + Verdict string `json:"verdict,omitempty"` + Body string `json:"body,omitempty"` + GithubReviewID string `json:"githubReviewId,omitempty"` + Reviews []submitReviewItem `json:"reviews,omitempty"` } type reviewSubmitOptions struct { @@ -46,6 +60,7 @@ type reviewSubmitOptions struct { verdict string body string reviewID string + reviews string } func newReviewCommand(ctx *commandContext) *cobra.Command { @@ -77,6 +92,7 @@ func newReviewSubmitCommand(ctx *commandContext) *cobra.Command { cmd.Flags().StringVar(&opts.verdict, "verdict", "", "Review verdict: approved or changes_requested (required)") cmd.Flags().StringVar(&opts.body, "body", "", "Review body: a path to a Markdown file, or - to read from stdin (so nothing is written into the worktree)") cmd.Flags().StringVar(&opts.reviewID, "review-id", "", "Id of the GitHub PR review just posted (the .id from the gh api POST that created the review)") + cmd.Flags().StringVar(&opts.reviews, "reviews", "", "JSON review results array or object: a path, or - to read from stdin") return cmd } @@ -88,6 +104,9 @@ func (c *commandContext) submitReview(cmd *cobra.Command, args []string, opts re if session == "" { return usageError{errors.New("usage: worker session id is required (positional or --session)")} } + if strings.TrimSpace(opts.reviews) != "" { + return c.submitReviewBatch(cmd, session, opts) + } runID := strings.TrimSpace(opts.runID) if runID == "" { return usageError{errors.New("usage: --run is required")} @@ -121,3 +140,49 @@ func (c *commandContext) submitReview(cmd *cobra.Command, args []string, opts re _, err := fmt.Fprintf(cmd.OutOrStdout(), "recorded %s review for %s\n", res.Review.Verdict, session) return err } + +func (c *commandContext) submitReviewBatch(cmd *cobra.Command, session string, opts reviewSubmitOptions) error { + if strings.TrimSpace(opts.runID) != "" || strings.TrimSpace(opts.verdict) != "" || strings.TrimSpace(opts.body) != "" || strings.TrimSpace(opts.reviewID) != "" { + return usageError{errors.New("usage: --reviews cannot be combined with --run, --verdict, --body, or --review-id")} + } + reviews, err := readReviewItems(cmd, strings.TrimSpace(opts.reviews)) + if err != nil { + return err + } + path := "sessions/" + url.PathEscape(session) + "/reviews/submit" + var res reviewRunResponse + if err := c.postJSON(cmd.Context(), path, submitReviewRequest{Reviews: reviews}, &res); err != nil { + return err + } + count := len(res.Reviews) + if count == 0 { + count = len(reviews) + } + _, err = fmt.Fprintf(cmd.OutOrStdout(), "recorded %d review(s) for %s\n", count, session) + return err +} + +func readReviewItems(cmd *cobra.Command, path string) ([]submitReviewItem, error) { + var raw []byte + var err error + if path == "-" { + raw, err = io.ReadAll(cmd.InOrStdin()) + } else { + raw, err = os.ReadFile(path) + } + if err != nil { + return nil, usageError{fmt.Errorf("read review results: %w", err)} + } + var req submitReviewRequest + if err := json.Unmarshal(raw, &req); err == nil && len(req.Reviews) > 0 { + return req.Reviews, nil + } + var reviews []submitReviewItem + if err := json.Unmarshal(raw, &reviews); err != nil { + return nil, usageError{fmt.Errorf("decode review results JSON: %w", err)} + } + if len(reviews) == 0 { + return nil, usageError{errors.New("usage: --reviews requires at least one review result")} + } + return reviews, nil +} diff --git a/backend/internal/cli/review_test.go b/backend/internal/cli/review_test.go index b3033de12..4c3a14816 100644 --- a/backend/internal/cli/review_test.go +++ b/backend/internal/cli/review_test.go @@ -104,6 +104,32 @@ func TestReviewSubmitAcceptsUnderscoreFlags(t *testing.T) { } } +func TestReviewSubmitBatchReadsReviewsFromStdin(t *testing.T) { + cfg := setConfigEnv(t) + srv, capture := reviewServer(t, http.StatusOK, `{"reviews":[{"id":"run-1","verdict":"changes_requested"},{"id":"run-2","verdict":"approved"}]}`) + writeRunFileFor(t, cfg, srv) + + deps := aliveDeps() + deps.In = strings.NewReader(`{"reviews":[{"runId":"run-1","verdict":"changes_requested","body":"fix auth","githubReviewId":"101"},{"runId":"run-2","verdict":"approved","body":"looks good"}]}`) + out, errOut, err := executeCLI(t, deps, "review", "submit", "mer-1", "--reviews", "-") + if err != nil { + t.Fatalf("unexpected error: %v\nstderr=%s", err, errOut) + } + if !strings.Contains(out, "recorded 2 review(s) for mer-1") { + t.Fatalf("stdout = %q", out) + } + var req submitReviewRequest + if err := json.Unmarshal([]byte(capture.body), &req); err != nil { + t.Fatalf("decode body: %v", err) + } + if len(req.Reviews) != 2 || req.Reviews[0].RunID != "run-1" || req.Reviews[0].GithubReviewID != "101" || req.Reviews[1].Verdict != "approved" { + t.Fatalf("request = %+v", req) + } + if req.RunID != "" || req.Verdict != "" { + t.Fatalf("batch request should not also set legacy fields: %+v", req) + } +} + func TestReviewSubmitUsesSessionFlag(t *testing.T) { cfg := setConfigEnv(t) srv, capture := reviewServer(t, http.StatusOK, `{"review":{"verdict":"approved"}}`) diff --git a/backend/internal/domain/review.go b/backend/internal/domain/review.go index 6750a16d2..749cb6c24 100644 --- a/backend/internal/domain/review.go +++ b/backend/internal/domain/review.go @@ -29,11 +29,15 @@ type Review struct { // ReviewRun is one review pass against a worker's PR. type ReviewRun struct { - ID string `json:"id"` - ReviewID string `json:"reviewId"` - SessionID SessionID `json:"sessionId"` - Harness ReviewerHarness `json:"harness"` - PRURL string `json:"prUrl"` + ID string `json:"id"` + ReviewID string `json:"reviewId"` + SessionID SessionID `json:"sessionId"` + // BatchID groups review runs created by one trigger so worker feedback can + // be delivered once after the whole trigger batch is terminal. Empty marks + // legacy/single-run delivery. + BatchID string `json:"batchId"` + Harness ReviewerHarness `json:"harness"` + PRURL string `json:"prUrl"` // TargetSHA is the PR head commit this pass reviewed. TargetSHA string `json:"targetSha"` Status ReviewRunStatus `json:"status"` diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index 2a3de1a9b..1b76c9285 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -1292,13 +1292,13 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/ReviewRunResponse' + $ref: '#/components/schemas/TriggerReviewResponse' description: OK "201": content: application/json: schema: - $ref: '#/components/schemas/ReviewRunResponse' + $ref: '#/components/schemas/TriggerReviewResponse' description: Created "404": content: @@ -1719,7 +1719,7 @@ components: type: string reviews: items: - $ref: '#/components/schemas/ReviewRun' + $ref: '#/components/schemas/PRReviewState' type: array required: - reviewerHandleId @@ -1855,6 +1855,33 @@ components: - id - projectId type: object + PRReviewState: + properties: + latestRun: + $ref: '#/components/schemas/ReviewRun' + prNumber: + type: integer + prUrl: + type: string + status: + enum: + - needs_review + - running + - up_to_date + - changes_requested + - ineligible + type: string + targetSha: + type: string + title: + type: string + required: + - prUrl + - prNumber + - title + - targetSha + - status + type: object Project: properties: agent: @@ -2016,6 +2043,8 @@ components: type: object ReviewRun: properties: + batchId: + type: string body: type: string createdAt: @@ -2048,6 +2077,7 @@ components: - id - reviewId - sessionId + - batchId - harness - prUrl - targetSha @@ -2063,8 +2093,13 @@ components: $ref: '#/components/schemas/ReviewRun' reviewerHandleId: type: string + reviews: + items: + $ref: '#/components/schemas/ReviewRun' + type: array required: - review + - reviews - reviewerHandleId type: object RoleOverride: @@ -2467,6 +2502,26 @@ components: - projectId type: object SubmitReviewInput: + properties: + body: + description: Review body recorded by AO. Required for changes_requested. + type: string + githubReviewId: + description: Id of the GitHub PR review the reviewer posted, if any. + type: string + reviews: + description: Batched review results recorded by one reviewer CLI command. + items: + $ref: '#/components/schemas/SubmitReviewItem' + type: array + runId: + description: Review run id being completed. + type: string + verdict: + description: 'Review verdict: approved or changes_requested.' + type: string + type: object + SubmitReviewItem: properties: body: description: Review body recorded by AO. Required for changes_requested. @@ -2483,8 +2538,18 @@ components: required: - runId - verdict - - body - - githubReviewId + type: object + TriggerReviewResponse: + properties: + reviewerHandleId: + type: string + reviews: + items: + $ref: '#/components/schemas/PRReviewState' + type: array + required: + - reviewerHandleId + - reviews type: object WorkspaceRepo: properties: diff --git a/backend/internal/httpd/apispec/specgen/build.go b/backend/internal/httpd/apispec/specgen/build.go index 713e1a156..21570e358 100644 --- a/backend/internal/httpd/apispec/specgen/build.go +++ b/backend/internal/httpd/apispec/specgen/build.go @@ -183,11 +183,14 @@ var schemaNames = map[string]string{ "ControllersResolveCommentsRequest": "ResolveCommentsRequest", "ControllersResolveCommentsResponse": "ResolveCommentsResponse", // httpd/controllers — review wire envelopes - "ControllersListReviewsResponse": "ListReviewsResponse", - "ControllersReviewRunResponse": "ReviewRunResponse", - "ControllersSubmitReviewInput": "SubmitReviewInput", + "ControllersListReviewsResponse": "ListReviewsResponse", + "ControllersReviewRunResponse": "ReviewRunResponse", + "ControllersTriggerReviewResponse": "TriggerReviewResponse", + "ControllersSubmitReviewItem": "SubmitReviewItem", + "ControllersSubmitReviewInput": "SubmitReviewInput", // domain review entities - "DomainReviewRun": "ReviewRun", + "DomainReviewRun": "ReviewRun", + "ReviewPRReviewState": "PRReviewState", // httpd/controllers: import wire envelopes "ControllersImportStatusResponse": "ImportStatusResponse", "ControllersImportRunResponse": "ImportRunResponse", @@ -378,8 +381,8 @@ func reviewOperations() []operation { summary: "Trigger a code review of a worker's PR", pathParams: []any{controllers.SessionIDParam{}}, resps: []respUnit{ - {http.StatusOK, controllers.ReviewRunResponse{}}, - {http.StatusCreated, controllers.ReviewRunResponse{}}, + {http.StatusOK, controllers.TriggerReviewResponse{}}, + {http.StatusCreated, controllers.TriggerReviewResponse{}}, {http.StatusUnprocessableEntity, envelope.APIError{}}, {http.StatusNotFound, envelope.APIError{}}, {http.StatusNotImplemented, envelope.APIError{}}, diff --git a/backend/internal/httpd/controllers/reviews.go b/backend/internal/httpd/controllers/reviews.go index e629670ff..1790e3c1d 100644 --- a/backend/internal/httpd/controllers/reviews.go +++ b/backend/internal/httpd/controllers/reviews.go @@ -10,6 +10,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" + reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review" reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review" ) @@ -17,23 +18,40 @@ import ( // reviewerHandleId is the live reviewer pane's runtime handle, for the UI to // attach its terminal over /mux (empty when no reviewer has run). type ListReviewsResponse struct { - ReviewerHandleID string `json:"reviewerHandleId"` - Reviews []domain.ReviewRun `json:"reviews"` + ReviewerHandleID string `json:"reviewerHandleId"` + Reviews []reviewcore.PRReviewState `json:"reviews"` } -// ReviewRunResponse is the body of trigger (200/201) and submit (200). It -// carries the run plus the reviewer pane handle so the UI can attach a terminal. +// ReviewRunResponse is the body of submit (200). It carries the run plus the +// reviewer pane handle so the UI can attach a terminal. type ReviewRunResponse struct { - Review domain.ReviewRun `json:"review"` - ReviewerHandleID string `json:"reviewerHandleId"` + Review domain.ReviewRun `json:"review"` + Reviews []domain.ReviewRun `json:"reviews"` + ReviewerHandleID string `json:"reviewerHandleId"` +} + +// TriggerReviewResponse is the body of trigger (200/201). reviews carries the +// PR-scoped review state after the trigger. +type TriggerReviewResponse struct { + ReviewerHandleID string `json:"reviewerHandleId"` + Reviews []reviewcore.PRReviewState `json:"reviews"` +} + +// SubmitReviewItem is one review result in a batched submit request. +type SubmitReviewItem struct { + RunID string `json:"runId" description:"Review run id being completed."` + Verdict string `json:"verdict" description:"Review verdict: approved or changes_requested."` + Body string `json:"body,omitempty" description:"Review body recorded by AO. Required for changes_requested."` + GithubReviewID string `json:"githubReviewId,omitempty" description:"Id of the GitHub PR review the reviewer posted, if any."` } // SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit. type SubmitReviewInput struct { - RunID string `json:"runId" description:"Review run id being completed."` - Verdict string `json:"verdict" description:"Review verdict: approved or changes_requested."` - Body string `json:"body" description:"Review body recorded by AO. Required for changes_requested."` - GithubReviewID string `json:"githubReviewId" description:"Id of the GitHub PR review the reviewer posted, if any."` + RunID string `json:"runId,omitempty" description:"Review run id being completed."` + Verdict string `json:"verdict,omitempty" description:"Review verdict: approved or changes_requested."` + Body string `json:"body,omitempty" description:"Review body recorded by AO. Required for changes_requested."` + GithubReviewID string `json:"githubReviewId,omitempty" description:"Id of the GitHub PR review the reviewer posted, if any."` + Reviews []SubmitReviewItem `json:"reviews,omitempty" description:"Batched review results recorded by one reviewer CLI command."` } // ReviewsController owns the session-scoped /reviews routes. A nil Svc returns 501. @@ -58,11 +76,11 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) { writeReviewError(w, r, err) return } - runs := res.Runs - if runs == nil { - runs = []domain.ReviewRun{} + reviews := res.Reviews + if reviews == nil { + reviews = []reviewcore.PRReviewState{} } - envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs}) + envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: reviews}) } func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) { @@ -81,7 +99,14 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) { if res.Created { status = http.StatusCreated } - envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID}) + reviews := res.Reviews + if reviews == nil { + reviews = []reviewcore.PRReviewState{} + } + envelope.WriteJSON(w, status, TriggerReviewResponse{ + ReviewerHandleID: res.ReviewerHandleID, + Reviews: reviews, + }) } func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) { @@ -94,12 +119,34 @@ func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) { envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_BODY", "Invalid request body", nil) return } - run, err := c.Svc.Submit(r.Context(), sessionID(r), in.RunID, domain.ReviewVerdict(in.Verdict), in.Body, in.GithubReviewID) + reviews := make([]reviewsvc.SubmittedReview, 0, len(in.Reviews)) + if len(in.Reviews) > 0 { + for _, item := range in.Reviews { + reviews = append(reviews, reviewsvc.SubmittedReview{ + RunID: item.RunID, + Verdict: domain.ReviewVerdict(item.Verdict), + Body: item.Body, + GithubReviewID: item.GithubReviewID, + }) + } + } else { + reviews = append(reviews, reviewsvc.SubmittedReview{ + RunID: in.RunID, + Verdict: domain.ReviewVerdict(in.Verdict), + Body: in.Body, + GithubReviewID: in.GithubReviewID, + }) + } + runs, err := c.Svc.SubmitMany(r.Context(), sessionID(r), reviews) if err != nil { writeReviewError(w, r, err) return } - envelope.WriteJSON(w, http.StatusOK, ReviewRunResponse{Review: run}) + first := domain.ReviewRun{} + if len(runs) > 0 { + first = runs[0] + } + envelope.WriteJSON(w, http.StatusOK, ReviewRunResponse{Review: first, Reviews: runs}) } func writeReviewError(w http.ResponseWriter, r *http.Request, err error) { diff --git a/backend/internal/httpd/controllers/reviews_test.go b/backend/internal/httpd/controllers/reviews_test.go index 674105eb4..123c45420 100644 --- a/backend/internal/httpd/controllers/reviews_test.go +++ b/backend/internal/httpd/controllers/reviews_test.go @@ -20,12 +20,18 @@ import ( type fakeReviewService struct { triggerErr error + trigger reviewcore.TriggerResult + list reviewcore.SessionReviews + submitted []reviewsvc.SubmittedReview } func (f *fakeReviewService) Trigger(context.Context, domain.SessionID) (reviewcore.TriggerResult, error) { if f.triggerErr != nil { return reviewcore.TriggerResult{}, f.triggerErr } + if f.trigger.ReviewerHandleID != "" || f.trigger.Run.ID != "" || f.trigger.Reviews != nil || f.trigger.CreatedRuns != nil { + return f.trigger, nil + } return reviewcore.TriggerResult{Run: domain.ReviewRun{ID: "run-1"}, Created: true}, nil } @@ -33,8 +39,17 @@ func (f *fakeReviewService) Submit(context.Context, domain.SessionID, string, do return domain.ReviewRun{}, nil } +func (f *fakeReviewService) SubmitMany(_ context.Context, _ domain.SessionID, reviews []reviewsvc.SubmittedReview) ([]domain.ReviewRun, error) { + f.submitted = append([]reviewsvc.SubmittedReview(nil), reviews...) + runs := make([]domain.ReviewRun, 0, len(reviews)) + for _, review := range reviews { + runs = append(runs, domain.ReviewRun{ID: review.RunID, Verdict: review.Verdict, Body: review.Body, GithubReviewID: review.GithubReviewID}) + } + return runs, nil +} + func (f *fakeReviewService) List(context.Context, domain.SessionID) (reviewcore.SessionReviews, error) { - return reviewcore.SessionReviews{}, nil + return f.list, nil } func newReviewTestServer(t *testing.T, svc reviewsvc.Manager) *httptest.Server { @@ -59,3 +74,73 @@ func TestReviewsTrigger_MissingReviewerBinaryReturns422WithCause(t *testing.T) { t.Fatalf("message = %q, want reviewer binary cause", got.Message) } } + +func TestReviewsListIncludesReviewStates(t *testing.T) { + srv := newReviewTestServer(t, &fakeReviewService{list: reviewcore.SessionReviews{ + ReviewerHandleID: "review-mer-1", + Runs: []domain.ReviewRun{{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}}, + Reviews: []reviewcore.PRReviewState{{PRURL: "https://github.com/o/r/pull/1", PRNumber: 1, TargetSHA: "sha1", Status: reviewcore.ReviewStateUpToDate}}, + }}) + + body, status, headers := doRequest(t, srv, "GET", "/api/v1/sessions/mer-1/reviews", "") + assertJSON(t, headers) + if status != http.StatusOK { + t.Fatalf("status = %d body=%s", status, body) + } + if !strings.Contains(string(body), `"reviews"`) || !strings.Contains(string(body), `"up_to_date"`) || !strings.Contains(string(body), `"reviewerHandleId":"review-mer-1"`) { + t.Fatalf("body missing review states/handle: %s", body) + } + if strings.Contains(string(body), `"items"`) || strings.Contains(string(body), `"reviewItems"`) || strings.Contains(string(body), `"reviewRuns"`) { + t.Fatalf("body contains deprecated review item aliases: %s", body) + } +} + +func TestReviewsTriggerIncludesBatchFields(t *testing.T) { + run1 := domain.ReviewRun{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"} + run2 := domain.ReviewRun{ID: "run-2", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2"} + srv := newReviewTestServer(t, &fakeReviewService{trigger: reviewcore.TriggerResult{ + Run: run1, + ReviewerHandleID: "review-mer-1", + Created: true, + CreatedRuns: []domain.ReviewRun{run1, run2}, + Reviews: []reviewcore.PRReviewState{ + {PRURL: run1.PRURL, PRNumber: 1, TargetSHA: run1.TargetSHA, Status: reviewcore.ReviewStateRunning, LatestRun: &run1}, + {PRURL: run2.PRURL, PRNumber: 2, TargetSHA: run2.TargetSHA, Status: reviewcore.ReviewStateRunning, LatestRun: &run2}, + }, + }}) + + body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/trigger", "") + assertJSON(t, headers) + if status != http.StatusCreated { + t.Fatalf("status = %d body=%s", status, body) + } + for _, want := range []string{`"reviews"`, `"running"`, `"run-1"`, `"run-2"`, `"reviewerHandleId":"review-mer-1"`} { + if !strings.Contains(string(body), want) { + t.Fatalf("body missing %s: %s", want, body) + } + } + for _, unwanted := range []string{`"reviewItems"`, `"items"`, `"createdReviews"`, `"createdRuns"`, `"reviewRuns"`, `"review":`} { + if strings.Contains(string(body), unwanted) { + t.Fatalf("body contains deprecated field %s: %s", unwanted, body) + } + } +} + +func TestReviewsSubmitAcceptsBatchedReviews(t *testing.T) { + svc := &fakeReviewService{} + srv := newReviewTestServer(t, svc) + + body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/submit", `{"reviews":[{"runId":"run-1","verdict":"changes_requested","body":"fix auth","githubReviewId":"101"},{"runId":"run-2","verdict":"approved"}]}`) + assertJSON(t, headers) + if status != http.StatusOK { + t.Fatalf("status = %d body=%s", status, body) + } + if len(svc.submitted) != 2 || svc.submitted[0].RunID != "run-1" || svc.submitted[1].Verdict != domain.VerdictApproved { + t.Fatalf("submitted = %+v", svc.submitted) + } + for _, want := range []string{`"reviews"`, `"run-1"`, `"run-2"`} { + if !strings.Contains(string(body), want) { + t.Fatalf("body missing %s: %s", want, body) + } + } +} diff --git a/backend/internal/lifecycle/manager_test.go b/backend/internal/lifecycle/manager_test.go index 4ccbe21de..c8526c466 100644 --- a/backend/internal/lifecycle/manager_test.go +++ b/backend/internal/lifecycle/manager_test.go @@ -543,8 +543,10 @@ func TestApplyReviewResultSendsAndDedupsThroughPRSignature(t *testing.T) { t.Fatalf("outcome/messages = %q/%v, want sent once", outcome, msg.msgs) } got := msg.msgs[0] - if !strings.Contains(got, "[AO reviewer]") || !strings.Contains(got, "fix the bug") || !strings.Contains(got, "98[2J765") { - t.Fatalf("AO review nudge missing label/body/review id: %q", got) + for _, want := range []string{"[AO reviewer]", "PR: " + result.PRURL, "Verdict: changes_requested", "Review body:\nfix the bug", "GitHub review: 98[2J765"} { + if !strings.Contains(got, want) { + t.Fatalf("AO review nudge missing %q: %q", want, got) + } } if strings.Contains(got, "\x1b") { t.Fatalf("AO review nudge should sanitize control bytes: %q", got) @@ -572,6 +574,65 @@ func TestApplyReviewResultSendsAndDedupsThroughPRSignature(t *testing.T) { } } +func TestApplyReviewBatchSendsCombinedAndDedups(t *testing.T) { + st := newFakeStore() + st.sessions["mer-1"] = working("mer-1") + msg := &fakeMessenger{} + m := New(st, msg) + results := []ReviewResult{ + {RunID: "run-2", BatchID: "batch-1", WorkerID: "mer-1", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2", Verdict: domain.VerdictChangesRequested, Body: "fix tests", GithubReviewID: "102"}, + {RunID: "run-1", BatchID: "batch-1", WorkerID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Verdict: domain.VerdictChangesRequested, Body: "fix auth", GithubReviewID: "101"}, + } + + outcome, err := m.ApplyReviewBatch(ctx, "mer-1", "batch-1", results) + if err != nil { + t.Fatalf("ApplyReviewBatch: %v", err) + } + if outcome != ReviewDeliverySent || len(msg.msgs) != 1 { + t.Fatalf("outcome/messages = %q/%v, want sent once", outcome, msg.msgs) + } + got := msg.msgs[0] + for _, want := range []string{ + "submitted 2 review(s) requesting changes", + "PR: https://github.com/o/r/pull/1", + "GitHub review: 101", + "Review body:\nfix auth", + "PR: https://github.com/o/r/pull/2", + "GitHub review: 102", + "Review body:\nfix tests", + } { + if !strings.Contains(got, want) { + t.Fatalf("batch nudge missing %q: %q", want, got) + } + } + if st.signatures["https://github.com/o/r/pull/1"] == "" { + t.Fatal("batch nudge did not persist signature on anchor PR") + } + + outcome, err = m.ApplyReviewBatch(ctx, "mer-1", "batch-1", results) + if err != nil { + t.Fatalf("repeat ApplyReviewBatch: %v", err) + } + if outcome != ReviewDeliverySent || len(msg.msgs) != 1 { + t.Fatalf("repeat should suppress duplicate send, outcome=%q msgs=%v", outcome, msg.msgs) + } +} + +func TestApplyReviewBatchNoopsWithoutDeliverableResults(t *testing.T) { + st := newFakeStore() + st.sessions["mer-1"] = working("mer-1") + msg := &fakeMessenger{} + m := New(st, msg) + + outcome, err := m.ApplyReviewBatch(ctx, "mer-1", "batch-1", nil) + if err != nil { + t.Fatalf("ApplyReviewBatch: %v", err) + } + if outcome != ReviewDeliveryNoop || len(msg.msgs) != 0 || st.signatureWrites != 0 { + t.Fatalf("empty batch should no-op, outcome=%q msgs=%v signatureWrites=%d", outcome, msg.msgs, st.signatureWrites) + } +} + func TestApplyReviewResultNoopsWhenIrrelevant(t *testing.T) { deliveredAt := time.Unix(100, 0).UTC() tests := []struct { diff --git a/backend/internal/lifecycle/reactions.go b/backend/internal/lifecycle/reactions.go index 526119568..3b115afe2 100644 --- a/backend/internal/lifecycle/reactions.go +++ b/backend/internal/lifecycle/reactions.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "log/slog" + "sort" "strings" "sync" "time" @@ -33,6 +34,7 @@ const ( // review_run row. type ReviewResult struct { RunID string + BatchID string WorkerID domain.SessionID PRURL string TargetSHA string @@ -42,6 +44,56 @@ type ReviewResult struct { DeliveredAt *time.Time } +// ApplyReviewBatch reacts to one reviewer CLI submission after the review +// service has decided which current-head changes-requested results are +// deliverable. +func (m *Manager) ApplyReviewBatch(ctx context.Context, workerID domain.SessionID, batchID string, results []ReviewResult) (ReviewDeliveryOutcome, error) { + if batchID == "" || len(results) == 0 { + return ReviewDeliveryNoop, nil + } + rec, ok, err := m.store.GetSession(ctx, workerID) + if err != nil || !ok { + return ReviewDeliveryNoop, err + } + if rec.IsTerminated || rec.Activity.State == domain.ActivityWaitingInput { + return ReviewDeliveryNoop, nil + } + if m.messenger == nil { + return ReviewDeliveryNoop, nil + } + sort.Slice(results, func(i, j int) bool { + if results[i].PRURL != results[j].PRURL { + return results[i].PRURL < results[j].PRURL + } + return results[i].RunID < results[j].RunID + }) + var msg strings.Builder + fmt.Fprintf(&msg, "[AO reviewer] AO's internal code reviewer submitted %d review(s) requesting changes.\n", len(results)) + var sigParts []string + for i, r := range results { + fmt.Fprintf(&msg, "\nReview %d\nPR: %s\nVerdict: %s", i+1, domain.SanitizeControlChars(r.PRURL), domain.SanitizeControlChars(string(r.Verdict))) + if r.TargetSHA != "" { + fmt.Fprintf(&msg, "\nHead commit: %s", domain.SanitizeControlChars(r.TargetSHA)) + } + if r.GithubReviewID != "" { + safeReviewID := domain.SanitizeControlChars(r.GithubReviewID) + fmt.Fprintf(&msg, "\nGitHub review: %s", safeReviewID) + fmt.Fprintf(&msg, "\nOnce you have addressed it, reply on GitHub review %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID) + } + if r.Body != "" { + fmt.Fprintf(&msg, "\n\nReview body:\n%s\n", domain.SanitizeControlChars(r.Body)) + } + sigParts = append(sigParts, strings.Join([]string{r.RunID, r.PRURL, r.TargetSHA, r.GithubReviewID, r.Body}, "\x00")) + } + anchorPR := results[0].PRURL + key := "review-batch:" + anchorPR + ":" + batchID + sig := strings.Join(sigParts, "\x01") + if err := m.sendOnce(ctx, workerID, anchorPR, key, sig, msg.String(), reviewMaxNudge); err != nil { + return ReviewDeliveryNoop, err + } + return ReviewDeliverySent, nil +} + type reactionState struct { mu sync.Mutex seen map[string]string @@ -154,13 +206,14 @@ func (m *Manager) ApplyReviewResult(ctx context.Context, workerID domain.Session if m.messenger == nil { return ReviewDeliveryNoop, nil } - msg := "[AO reviewer] AO's internal code reviewer requested changes on your PR. Review the feedback below and address it." + msg := fmt.Sprintf("[AO reviewer] AO's internal code reviewer submitted a review.\n\nPR: %s\nVerdict: %s", domain.SanitizeControlChars(r.PRURL), domain.SanitizeControlChars(string(r.Verdict))) if r.GithubReviewID != "" { safeReviewID := domain.SanitizeControlChars(r.GithubReviewID) - msg += fmt.Sprintf(" This feedback is GitHub review %s. Once you have addressed it, reply on that review referencing id %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID, safeReviewID) + msg += fmt.Sprintf("\nGitHub review: %s", safeReviewID) + msg += fmt.Sprintf("\n\nOnce you have addressed it, reply on GitHub review %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID) } if r.Body != "" { - msg += "\n\n" + domain.SanitizeControlChars(r.Body) + msg += "\n\nReview body:\n" + domain.SanitizeControlChars(r.Body) } key := "review:" + r.PRURL + ":ao:" + r.RunID sig := strings.Join([]string{r.TargetSHA, r.RunID, r.GithubReviewID, r.Body}, "\x00") diff --git a/backend/internal/ports/reviewer.go b/backend/internal/ports/reviewer.go index a42df4d72..21bb4ffe1 100644 --- a/backend/internal/ports/reviewer.go +++ b/backend/internal/ports/reviewer.go @@ -37,6 +37,11 @@ type ReviewInvocation struct { PRURL string // TargetSHA is the PR head commit under review. TargetSHA string + // ReviewQueue lists all review tasks created by the same trigger so a shared + // reviewer pane can review multiple PRs and submit the results together. + ReviewQueue []ReviewTask + // ReviewIndex is this invocation's zero-based position in ReviewQueue. + ReviewIndex int // WorkspacePath is the worker's checkout the reviewer reads. WorkspacePath string // Prompt and SystemPrompt are the review instructions AO authored centrally, @@ -48,6 +53,13 @@ type ReviewInvocation struct { SystemPrompt string } +// ReviewTask is one PR/run in a multi-PR review trigger queue. +type ReviewTask struct { + RunID string + PRURL string + TargetSHA string +} + // ReviewCommandSpec is how to launch a reviewer: the argv and any extra env the // adapter needs. AO supplies the workspace and review-tracking env around it. type ReviewCommandSpec struct { diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go index e980f0b16..d9639f0bd 100644 --- a/backend/internal/review/launcher.go +++ b/backend/internal/review/launcher.go @@ -31,6 +31,8 @@ type LaunchSpec struct { WorkspacePath string PRURL string TargetSHA string + ReviewQueue []ports.ReviewTask + ReviewIndex int } // reviewerRuntime is the runtime surface the launcher needs: create a pane, @@ -73,6 +75,8 @@ func (l *agentLauncher) invocation(spec LaunchSpec) ports.ReviewInvocation { WorkerSessionID: spec.WorkerID, PRURL: spec.PRURL, TargetSHA: spec.TargetSHA, + ReviewQueue: spec.ReviewQueue, + ReviewIndex: spec.ReviewIndex, WorkspacePath: spec.WorkspacePath, Prompt: prompt, SystemPrompt: systemPrompt, diff --git a/backend/internal/review/planner.go b/backend/internal/review/planner.go new file mode 100644 index 000000000..a2cbdd6fa --- /dev/null +++ b/backend/internal/review/planner.go @@ -0,0 +1,95 @@ +package review + +import ( + "sort" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// StateStatus is the per-PR review planning state. +type StateStatus string + +const ( + // ReviewStateNeedsReview means an eligible PR has no current AO approval or running pass. + ReviewStateNeedsReview StateStatus = "needs_review" + // ReviewStateRunning means a review run is already active for the PR's current head. + ReviewStateRunning StateStatus = "running" + // ReviewStateUpToDate means AO approved the PR's current head. + ReviewStateUpToDate StateStatus = "up_to_date" + // ReviewStateChangesRequested means AO requested changes on the PR's current head. + ReviewStateChangesRequested StateStatus = "changes_requested" + // ReviewStateIneligible means the PR is draft, closed, merged, or missing required facts. + ReviewStateIneligible StateStatus = "ineligible" +) + +// PRReviewState is one PR-scoped review decision for a worker session. +type PRReviewState struct { + PRURL string `json:"prUrl"` + PRNumber int `json:"prNumber"` + Title string `json:"title"` + TargetSHA string `json:"targetSha"` + Status StateStatus `json:"status" enum:"needs_review,running,up_to_date,changes_requested,ineligible"` + LatestRun *domain.ReviewRun `json:"latestRun,omitempty"` +} + +// Plan computes per-PR review work from the currently observed PRs and existing +// review runs. It is pure so the trigger path and API list path share exactly +// the same eligibility/status rules. +func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { + latest := latestRunsByPRAndSHA(runs) + reviews := make([]PRReviewState, 0, len(prs)) + for _, pr := range prs { + review := PRReviewState{ + PRURL: pr.URL, + PRNumber: pr.Number, + Title: pr.Title, + TargetSHA: pr.HeadSHA, + Status: ReviewStateNeedsReview, + } + if pr.URL == "" || pr.HeadSHA == "" || pr.Draft || pr.Merged || pr.Closed { + review.Status = ReviewStateIneligible + if run, ok := latest[review.PRURL+"\x00"+review.TargetSHA]; ok { + review.LatestRun = &run + } + reviews = append(reviews, review) + continue + } + if run, ok := latest[review.PRURL+"\x00"+review.TargetSHA]; ok { + review.LatestRun = &run + switch { + case run.Status == domain.ReviewRunRunning: + review.Status = ReviewStateRunning + case run.Verdict == domain.VerdictApproved: + review.Status = ReviewStateUpToDate + case run.Verdict == domain.VerdictChangesRequested: + review.Status = ReviewStateChangesRequested + case run.Status == domain.ReviewRunFailed: + review.Status = ReviewStateNeedsReview + default: + review.Status = ReviewStateNeedsReview + } + } + reviews = append(reviews, review) + } + sort.SliceStable(reviews, func(i, j int) bool { + if reviews[i].PRNumber != reviews[j].PRNumber { + return reviews[i].PRNumber < reviews[j].PRNumber + } + return reviews[i].PRURL < reviews[j].PRURL + }) + return reviews +} + +func latestRunsByPRAndSHA(runs []domain.ReviewRun) map[string]domain.ReviewRun { + latest := make(map[string]domain.ReviewRun) + for _, run := range runs { + if run.PRURL == "" || run.TargetSHA == "" { + continue + } + key := run.PRURL + "\x00" + run.TargetSHA + if existing, ok := latest[key]; !ok || run.CreatedAt.After(existing.CreatedAt) { + latest[key] = run + } + } + return latest +} diff --git a/backend/internal/review/planner_test.go b/backend/internal/review/planner_test.go new file mode 100644 index 000000000..9b655f2a3 --- /dev/null +++ b/backend/internal/review/planner_test.go @@ -0,0 +1,68 @@ +package review + +import ( + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +func TestPlanStatuses(t *testing.T) { + now := time.Unix(100, 0).UTC() + tests := []struct { + name string + pr domain.PullRequest + runs []domain.ReviewRun + want StateStatus + }{ + {name: "open needs review", pr: planPR("pr1", 1, "sha1"), want: ReviewStateNeedsReview}, + {name: "draft ineligible", pr: withDraft(planPR("pr1", 1, "sha1")), want: ReviewStateIneligible}, + {name: "merged ineligible", pr: withMerged(planPR("pr1", 1, "sha1")), want: ReviewStateIneligible}, + {name: "closed ineligible", pr: withClosed(planPR("pr1", 1, "sha1")), want: ReviewStateIneligible}, + {name: "approved current sha up to date", pr: planPR("pr1", 1, "sha1"), runs: []domain.ReviewRun{ + {ID: "run-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, CreatedAt: now}, + }, want: ReviewStateUpToDate}, + {name: "changes requested current sha", pr: planPR("pr1", 1, "sha1"), runs: []domain.ReviewRun{ + {ID: "run-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictChangesRequested, CreatedAt: now}, + }, want: ReviewStateChangesRequested}, + {name: "running current sha", pr: planPR("pr1", 1, "sha1"), runs: []domain.ReviewRun{ + {ID: "run-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, CreatedAt: now}, + }, want: ReviewStateRunning}, + {name: "different sha needs review", pr: planPR("pr1", 1, "sha2"), runs: []domain.ReviewRun{ + {ID: "run-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, CreatedAt: now}, + }, want: ReviewStateNeedsReview}, + {name: "failed current sha retryable", pr: planPR("pr1", 1, "sha1"), runs: []domain.ReviewRun{ + {ID: "run-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunFailed, CreatedAt: now}, + }, want: ReviewStateNeedsReview}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Plan([]domain.PullRequest{tt.pr}, tt.runs) + if len(got) != 1 { + t.Fatalf("review states = %d, want 1", len(got)) + } + if got[0].Status != tt.want { + t.Fatalf("status = %s, want %s; item=%+v", got[0].Status, tt.want, got[0]) + } + }) + } +} + +func planPR(url string, n int, sha string) domain.PullRequest { + return domain.PullRequest{URL: url, Number: n, HeadSHA: sha} +} + +func withDraft(pr domain.PullRequest) domain.PullRequest { + pr.Draft = true + return pr +} + +func withMerged(pr domain.PullRequest) domain.PullRequest { + pr.Merged = true + return pr +} + +func withClosed(pr domain.PullRequest) domain.PullRequest { + pr.Closed = true + return pr +} diff --git a/backend/internal/review/prompt.go b/backend/internal/review/prompt.go index 09e513079..42dfca8c0 100644 --- a/backend/internal/review/prompt.go +++ b/backend/internal/review/prompt.go @@ -1,6 +1,9 @@ package review -import "fmt" +import ( + "fmt" + "strings" +) // reviewTexts returns the user-facing prompt and the system prompt to deliver to // a reviewer, authored in one place — the reviewer analogue of @@ -14,14 +17,18 @@ import "fmt" func reviewTexts(spec LaunchSpec) (prompt, systemPrompt string) { systemPrompt = `## Code reviewer role -You are an AO code reviewer. You review a single pull request's changes in the current checkout — do not start unrelated work. Inspect what the PR changed by diffing the checkout against the PR's base branch, and review for correctness bugs, missing error handling, security issues, test coverage, and clear deviations from the surrounding code's conventions. Prefer a few high-confidence findings over nitpicks. +You are an AO code reviewer. You review the requested pull request changes in the current checkout — do not start unrelated work. Inspect what each PR changed by diffing the checkout against the PR's base branch, and review for correctness bugs, missing error handling, security issues, test coverage, and clear deviations from the surrounding code's conventions. Prefer a few high-confidence findings over nitpicks. Post your review as a comment on the pull request, stating clearly whether it needs changes or is ready, with inline comments for specific findings. Do not push commits, edit files, or modify the branch — review only.` - prompt = fmt.Sprintf(`Review pull request %s (head commit %s). + queueText := reviewQueueText(spec) + prompt = fmt.Sprintf(`Review the requested pull request(s) for worker session %s. +%s + +Complete every review task in the queue autonomously. Do not ask the user whether to continue to the next PR, and do not stop after the first PR unless the provider or checkout is genuinely unusable for every queued task. Do these steps in order: -1. Post your review on the pull request and capture its id in one call. Post with `+"`gh api`"+` rather than `+"`gh pr review`"+`: it is the only way to attach inline comments, and its response carries the created review's id, so AO can tell the worker exactly which review to address. Send the review as a JSON body so the inline comments form a proper array of objects: +1. For each PR below, post a separate review on that pull request and capture its id in one call. Post with `+"`gh api`"+` rather than `+"`gh pr review`"+`: it is the only way to attach inline comments, and its response carries the created review's id, so AO can tell the worker exactly which review to address. Send the review as a JSON body so the inline comments form a proper array of objects: gh api --method POST repos/{owner}/{repo}/pulls/{number}/reviews --input - --jq '.id' <<'JSON' { "event": "COMMENT", "body": "", @@ -31,13 +38,29 @@ Do these steps in order: - Substitute the PR's owner/repo/number. Add one object to "comments" per inline finding; omit the field for a review with no inline comments. - Always use "event": "COMMENT": reviews are posted from the PR author's own account, and GitHub rejects both APPROVE and REQUEST_CHANGES on your own PR. State in the body whether you are requesting changes or approving; the machine-readable verdict goes to AO in step 2. - The printed number is the review id. If the call fails on the provider, leave the id empty. -2. Record the result with AO, passing your full review on stdin with --body - so nothing is ever written into the worktree (a file there could be committed onto the worker's branch): +2. After every PR has its own GitHub review from step 1, record AO's bookkeeping for those already-posted reviews using one command. Pass JSON on stdin so nothing is ever written into the worktree (a file there could be committed onto the worker's branch). Include one object per PR/run from the queue: - ao review submit --session %s --run %s --verdict --review-id --body - <<'MD' - - MD + ao review submit --session %s --reviews - <<'JSON' + { + "reviews": [ + { "runId": "", "verdict": "", "githubReviewId": "", "body": "" } + ] + } + JSON -Only if step 1 genuinely fails on the provider, still run step 2 (without --review-id) so the result is recorded.`, - spec.PRURL, spec.TargetSHA, spec.WorkerID, spec.RunID) +Only if step 1 genuinely fails on the provider for a PR, still include that run in step 2 with an empty githubReviewId so the result is recorded.`, + spec.WorkerID, queueText, spec.WorkerID) return prompt, systemPrompt } + +func reviewQueueText(spec LaunchSpec) string { + if len(spec.ReviewQueue) <= 1 { + return fmt.Sprintf("\nReview task queue:\n* 1. %s (head commit %s, run %s)\n", spec.PRURL, spec.TargetSHA, spec.RunID) + } + var b strings.Builder + fmt.Fprintf(&b, "\nAO created %d review tasks for this worker session. Review every queued PR, then submit all results together.\n\nReview task queue:\n", len(spec.ReviewQueue)) + for i, task := range spec.ReviewQueue { + fmt.Fprintf(&b, "* %d. %s (head commit %s, run %s)\n", i+1, task.PRURL, task.TargetSHA, task.RunID) + } + return b.String() +} diff --git a/backend/internal/review/prompt_test.go b/backend/internal/review/prompt_test.go new file mode 100644 index 000000000..c2428782e --- /dev/null +++ b/backend/internal/review/prompt_test.go @@ -0,0 +1,37 @@ +package review + +import ( + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +func TestReviewTextsIncludesMultiPRQueue(t *testing.T) { + spec := launchSpec() + spec.RunID = "run-2" + spec.PRURL = "https://github.com/o/r/pull/2" + spec.TargetSHA = "sha2" + spec.ReviewIndex = 1 + spec.ReviewQueue = []ports.ReviewTask{ + {RunID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}, + {RunID: "run-2", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2"}, + } + + prompt, _ := reviewTexts(spec) + for _, want := range []string{ + "AO created 2 review tasks", + "Review every queued PR, then submit all results together", + "Complete every review task in the queue autonomously", + "Do not ask the user whether to continue to the next PR", + "* 1. https://github.com/o/r/pull/1 (head commit sha1, run run-1)", + "* 2. https://github.com/o/r/pull/2 (head commit sha2, run run-2)", + "After every PR has its own GitHub review from step 1", + "ao review submit --session mer-1 --reviews -", + `"reviews": [`, + } { + if !strings.Contains(prompt, want) { + t.Fatalf("prompt missing %q:\n%s", want, prompt) + } + } +} diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go index 1b9816402..46d47b67c 100644 --- a/backend/internal/review/review.go +++ b/backend/internal/review/review.go @@ -18,6 +18,7 @@ import ( "github.com/google/uuid" "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) // ErrInvalid and ErrNotFound let the transport layer map failures to 422/404. @@ -34,9 +35,9 @@ type Store interface { InsertReviewRun(ctx stdctx.Context, r domain.ReviewRun) error UpdateReviewRunResult(ctx stdctx.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body, githubReviewID string) (bool, error) SupersedeReviewRun(ctx stdctx.Context, id, body string) (bool, error) - SupersedeStaleRunningReviewRuns(ctx stdctx.Context, sessionID domain.SessionID, targetSHA, body string) (int64, error) + SupersedeStaleRunningReviewRuns(ctx stdctx.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) GetReviewRun(ctx stdctx.Context, id string) (domain.ReviewRun, bool, error) - GetReviewRunBySessionAndSHA(ctx stdctx.Context, id domain.SessionID, targetSHA string) (domain.ReviewRun, bool, error) + GetReviewRunBySessionPRAndSHA(ctx stdctx.Context, id domain.SessionID, prURL, targetSHA string) (domain.ReviewRun, bool, error) ListReviewRunsBySession(ctx stdctx.Context, id domain.SessionID) ([]domain.ReviewRun, error) } @@ -134,6 +135,8 @@ type TriggerResult struct { Run domain.ReviewRun ReviewerHandleID string Created bool + Reviews []PRReviewState + CreatedRuns []domain.ReviewRun } // SessionReviews is a worker's review state: the live reviewer handle plus its @@ -141,14 +144,12 @@ type TriggerResult struct { type SessionReviews struct { ReviewerHandleID string Runs []domain.ReviewRun + Reviews []PRReviewState } -// Trigger starts (or reuses) a review of a worker's PR at its current head: -// - if a non-failed run already exists for this commit, it is returned unchanged; -// - otherwise, if a live reviewer pane exists, it is messaged to review the -// new commit; if not, a fresh reviewer is spawned; -// - the run is recorded before launch so startup failures leave a visible -// failed pass instead of an empty gap. +// Trigger starts reviews for every PR on the worker session that needs review. +// It reuses running/up-to-date runs, retries failed/current changes-requested +// heads, and uses one reviewer pane for every new run in the batch. func (e *Engine) Trigger(ctx stdctx.Context, workerID domain.SessionID) (TriggerResult, error) { if workerID == "" { return TriggerResult{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) @@ -175,39 +176,21 @@ func (e *Engine) Trigger(ctx stdctx.Context, workerID domain.SessionID) (Trigger return TriggerResult{}, fmt.Errorf("%w: worker session %q has no workspace to review", ErrInvalid, workerID) } - pr, err := e.workerPR(ctx, workerID) + prs, err := e.prs.ListPRsBySession(ctx, workerID) if err != nil { return TriggerResult{}, err } - targetSHA := pr.HeadSHA - - review, hasReview, err := e.store.GetReviewBySession(ctx, workerID) + if len(prs) == 0 { + return TriggerResult{}, fmt.Errorf("%w: worker %q has no PR to review", ErrInvalid, workerID) + } + runs, err := e.store.ListReviewRunsBySession(ctx, workerID) if err != nil { return TriggerResult{}, err } + reviews := Plan(prs, runs) - // Idempotency: a pass for this commit is reusable while it is still running - // or once it carries a verdict. The fallback branch below is defensive for - // any non-running, non-failed row that somehow lacks a verdict; normal - // Submit paths complete a run only with a valid verdict (#342). - if existing, ok, err := e.store.GetReviewRunBySessionAndSHA(ctx, workerID, targetSHA); err != nil { - return TriggerResult{}, err - } else if ok && (existing.Status == domain.ReviewRunRunning || existing.Verdict != domain.VerdictNone) { - return TriggerResult{Run: existing, ReviewerHandleID: review.ReviewerHandleID, Created: false}, nil - } else if ok && existing.Status != domain.ReviewRunFailed { - superseded, err := e.store.SupersedeReviewRun(ctx, existing.ID, "superseded by a new review trigger") - if err != nil { - return TriggerResult{}, err - } - if !superseded { - if latest, ok, err := e.store.GetReviewRun(ctx, existing.ID); err != nil { - return TriggerResult{}, err - } else if ok { - return TriggerResult{Run: latest, ReviewerHandleID: review.ReviewerHandleID, Created: false}, nil - } - } - } - if _, err := e.store.SupersedeStaleRunningReviewRuns(ctx, workerID, targetSHA, "superseded by a review trigger for a newer commit"); err != nil { + reviewRow, hasReview, err := e.store.GetReviewBySession(ctx, workerID) + if err != nil { return TriggerResult{}, err } @@ -217,78 +200,156 @@ func (e *Engine) Trigger(ctx stdctx.Context, workerID domain.SessionID) (Trigger } now := e.clock() - runID := e.newID() - spec := LaunchSpec{ - RunID: runID, - WorkerID: workerID, - Harness: harness, - WorkspacePath: worker.Metadata.WorkspacePath, - PRURL: pr.URL, - TargetSHA: targetSHA, - } - - review, err = e.upsertReview(ctx, worker, harness, pr.URL, review.ReviewerHandleID, now) + reviewRow, err = e.upsertReview(ctx, worker, harness, reviewRow.ReviewerHandleID, now) if err != nil { return TriggerResult{}, err } - run := domain.ReviewRun{ - ID: runID, - ReviewID: review.ID, - SessionID: workerID, - Harness: harness, - PRURL: pr.URL, - TargetSHA: targetSHA, - Status: domain.ReviewRunRunning, - Verdict: domain.VerdictNone, - CreatedAt: now, - } - if err := e.store.InsertReviewRun(ctx, run); err != nil { - if errors.Is(err, domain.ErrDuplicateReviewRun) { - if existing, ok, getErr := e.store.GetReviewRunBySessionAndSHA(ctx, workerID, targetSHA); getErr != nil { - return TriggerResult{}, getErr - } else if ok { - return TriggerResult{Run: existing, ReviewerHandleID: review.ReviewerHandleID, Created: false}, nil + + var created []domain.ReviewRun + batchID := "" + for _, reviewState := range reviews { + if reviewState.Status != ReviewStateNeedsReview && reviewState.Status != ReviewStateChangesRequested { + continue + } + if reviewState.LatestRun != nil && reviewState.LatestRun.Status != domain.ReviewRunFailed && reviewState.LatestRun.Status != domain.ReviewRunRunning && reviewState.LatestRun.Verdict == domain.VerdictNone { + superseded, err := e.store.SupersedeReviewRun(ctx, reviewState.LatestRun.ID, "superseded by a new review trigger") + if err != nil { + return TriggerResult{}, err + } + if !superseded { + if latest, ok, err := e.store.GetReviewRun(ctx, reviewState.LatestRun.ID); err != nil { + return TriggerResult{}, err + } else if ok { + reviews = replaceReviewLatestRun(reviews, reviewState.PRURL, reviewState.TargetSHA, latest) + continue + } } } - return TriggerResult{}, err + if _, err := e.store.SupersedeStaleRunningReviewRuns(ctx, workerID, reviewState.PRURL, reviewState.TargetSHA, "superseded by a review trigger for a newer commit"); err != nil { + return TriggerResult{}, err + } + if batchID == "" { + batchID = e.newID() + } + run := domain.ReviewRun{ + ID: e.newID(), + ReviewID: reviewRow.ID, + SessionID: workerID, + BatchID: batchID, + Harness: harness, + PRURL: reviewState.PRURL, + TargetSHA: reviewState.TargetSHA, + Status: domain.ReviewRunRunning, + Verdict: domain.VerdictNone, + CreatedAt: now, + } + if err := e.store.InsertReviewRun(ctx, run); err != nil { + if errors.Is(err, domain.ErrDuplicateReviewRun) { + if existing, ok, getErr := e.store.GetReviewRunBySessionPRAndSHA(ctx, workerID, reviewState.PRURL, reviewState.TargetSHA); getErr != nil { + return TriggerResult{}, getErr + } else if ok { + reviews = replaceReviewLatestRun(reviews, reviewState.PRURL, reviewState.TargetSHA, existing) + continue + } + } + return TriggerResult{}, err + } + created = append(created, run) + reviews = replaceReviewLatestRun(reviews, reviewState.PRURL, reviewState.TargetSHA, run) + } + if len(created) == 0 { + return TriggerResult{Run: firstReusableRun(reviews), ReviewerHandleID: reviewRow.ReviewerHandleID, Created: false, Reviews: reviews}, nil } - failRun := func(err error) error { - if _, updateErr := e.store.UpdateReviewRunResult(ctx, run.ID, domain.ReviewRunFailed, domain.VerdictNone, err.Error(), ""); updateErr != nil { - return updateErr + failRuns := func(start int, err error) error { + for _, run := range created[start:] { + if _, updateErr := e.store.UpdateReviewRunResult(ctx, run.ID, domain.ReviewRunFailed, domain.VerdictNone, err.Error(), ""); updateErr != nil { + return updateErr + } } return err } - // Reuse a live reviewer pane if there is one; otherwise spawn a fresh one. handleID := "" - if hasReview && review.ReviewerHandleID != "" { - alive, err := e.launcher.Alive(ctx, review.ReviewerHandleID) + queue := reviewQueue(created) + if hasReview && reviewRow.ReviewerHandleID != "" { + alive, err := e.launcher.Alive(ctx, reviewRow.ReviewerHandleID) if err != nil { - return TriggerResult{}, failRun(err) + return TriggerResult{}, failRuns(0, err) } if alive { - if err := e.launcher.Notify(ctx, review.ReviewerHandleID, spec); err != nil { - return TriggerResult{}, failRun(fmt.Errorf("notify reviewer: %w", err)) - } - handleID = review.ReviewerHandleID + handleID = reviewRow.ReviewerHandleID } } if handleID == "" { - h, err := e.launcher.Spawn(ctx, spec) + h, err := e.launcher.Spawn(ctx, reviewLaunchSpec(worker, harness, created[0], queue, 0)) if err != nil { - return TriggerResult{}, failRun(fmt.Errorf("launch reviewer: %w", err)) + return TriggerResult{}, failRuns(0, fmt.Errorf("launch reviewer: %w", err)) } handleID = h + } else { + if err := e.launcher.Notify(ctx, handleID, reviewLaunchSpec(worker, harness, created[0], queue, 0)); err != nil { + return TriggerResult{}, failRuns(0, fmt.Errorf("notify reviewer: %w", err)) + } } - - // The reviewer is running; now record the pass. - review, err = e.upsertReview(ctx, worker, harness, pr.URL, handleID, now) + reviewRow, err = e.upsertReview(ctx, worker, harness, handleID, now) if err != nil { return TriggerResult{}, err } - run.ReviewID = review.ID - return TriggerResult{Run: run, ReviewerHandleID: handleID, Created: true}, nil + for i := range created { + created[i].ReviewID = reviewRow.ID + } + return TriggerResult{Run: created[0], ReviewerHandleID: handleID, Created: true, Reviews: reviews, CreatedRuns: created}, nil +} + +func reviewLaunchSpec(worker domain.SessionRecord, harness domain.ReviewerHarness, run domain.ReviewRun, queue []ports.ReviewTask, index int) LaunchSpec { + return LaunchSpec{ + RunID: run.ID, + WorkerID: worker.ID, + Harness: harness, + WorkspacePath: worker.Metadata.WorkspacePath, + PRURL: run.PRURL, + TargetSHA: run.TargetSHA, + ReviewQueue: queue, + ReviewIndex: index, + } +} + +func reviewQueue(runs []domain.ReviewRun) []ports.ReviewTask { + queue := make([]ports.ReviewTask, 0, len(runs)) + for _, run := range runs { + queue = append(queue, ports.ReviewTask{ + RunID: run.ID, + PRURL: run.PRURL, + TargetSHA: run.TargetSHA, + }) + } + return queue +} + +func replaceReviewLatestRun(reviews []PRReviewState, prURL, targetSHA string, run domain.ReviewRun) []PRReviewState { + for i := range reviews { + if reviews[i].PRURL == prURL && reviews[i].TargetSHA == targetSHA { + reviews[i].LatestRun = &run + if run.Status == domain.ReviewRunRunning { + reviews[i].Status = ReviewStateRunning + } + break + } + } + return reviews +} + +func firstReusableRun(reviews []PRReviewState) domain.ReviewRun { + // Legacy compatibility only: in the multi-PR model the authoritative state + // is Reviews. When no run is created, this field is just a best-effort + // non-empty run for older clients. + for _, review := range reviews { + if review.LatestRun != nil { + return *review.LatestRun + } + } + return domain.ReviewRun{} } // List returns a worker's review state: the live reviewer handle and its passes. @@ -306,18 +367,11 @@ func (e *Engine) List(ctx stdctx.Context, workerID domain.SessionID) (SessionRev } else if ok { handle = review.ReviewerHandleID } - return SessionReviews{ReviewerHandleID: handle, Runs: runs}, nil -} - -func (e *Engine) workerPR(ctx stdctx.Context, workerID domain.SessionID) (domain.PullRequest, error) { prs, err := e.prs.ListPRsBySession(ctx, workerID) if err != nil { - return domain.PullRequest{}, err + return SessionReviews{}, err } - if len(prs) == 0 { - return domain.PullRequest{}, fmt.Errorf("%w: worker %q has no PR to review", ErrInvalid, workerID) - } - return prs[0], nil + return SessionReviews{ReviewerHandleID: handle, Runs: runs, Reviews: Plan(prs, runs)}, nil } // reviewerHarness resolves which harness reviews the worker's PR: a configured @@ -335,7 +389,7 @@ func (e *Engine) reviewerHarness(ctx stdctx.Context, worker domain.SessionRecord return cfg.ResolveReviewerHarness(worker.Harness), nil } -func (e *Engine) upsertReview(ctx stdctx.Context, worker domain.SessionRecord, harness domain.ReviewerHarness, prURL, handleID string, now time.Time) (domain.Review, error) { +func (e *Engine) upsertReview(ctx stdctx.Context, worker domain.SessionRecord, harness domain.ReviewerHarness, handleID string, now time.Time) (domain.Review, error) { existing, ok, err := e.store.GetReviewBySession(ctx, worker.ID) if err != nil { return domain.Review{}, err @@ -345,7 +399,7 @@ func (e *Engine) upsertReview(ctx stdctx.Context, worker domain.SessionRecord, h SessionID: worker.ID, ProjectID: worker.ProjectID, Harness: harness, - PRURL: prURL, + PRURL: "", ReviewerHandleID: handleID, CreatedAt: now, UpdatedAt: now, diff --git a/backend/internal/review/review_test.go b/backend/internal/review/review_test.go index 003b19b5b..455ae690d 100644 --- a/backend/internal/review/review_test.go +++ b/backend/internal/review/review_test.go @@ -43,6 +43,16 @@ func (f *fakeStore) InsertReviewRun(_ context.Context, r domain.ReviewRun) error f.runs = append(f.runs, winner) return f.insertErr } + for _, existing := range f.runs { + if existing.SessionID == r.SessionID && + existing.PRURL == r.PRURL && + existing.TargetSHA == r.TargetSHA && + existing.TargetSHA != "" && + existing.Status != domain.ReviewRunFailed && + existing.Verdict != domain.VerdictChangesRequested { + return domain.ErrDuplicateReviewRun + } + } f.runs = append(f.runs, r) return nil } @@ -74,10 +84,10 @@ func (f *fakeStore) SupersedeReviewRun(_ context.Context, id, body string) (bool } return false, nil } -func (f *fakeStore) SupersedeStaleRunningReviewRuns(_ context.Context, sessionID domain.SessionID, targetSHA, body string) (int64, error) { +func (f *fakeStore) SupersedeStaleRunningReviewRuns(_ context.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) { var n int64 for i := range f.runs { - if f.runs[i].SessionID == sessionID && f.runs[i].TargetSHA != targetSHA && f.runs[i].Status == domain.ReviewRunRunning && f.runs[i].Verdict == domain.VerdictNone { + if f.runs[i].SessionID == sessionID && f.runs[i].PRURL == prURL && f.runs[i].TargetSHA != targetSHA && f.runs[i].Status == domain.ReviewRunRunning && f.runs[i].Verdict == domain.VerdictNone { f.runs[i].Status = domain.ReviewRunFailed f.runs[i].Body = body n++ @@ -93,9 +103,9 @@ func (f *fakeStore) GetReviewRun(_ context.Context, id string) (domain.ReviewRun } return domain.ReviewRun{}, false, nil } -func (f *fakeStore) GetReviewRunBySessionAndSHA(_ context.Context, _ domain.SessionID, sha string) (domain.ReviewRun, bool, error) { +func (f *fakeStore) GetReviewRunBySessionPRAndSHA(_ context.Context, sessionID domain.SessionID, prURL, sha string) (domain.ReviewRun, bool, error) { for i := len(f.runs) - 1; i >= 0; i-- { - if f.runs[i].TargetSHA == sha { + if f.runs[i].SessionID == sessionID && f.runs[i].PRURL == prURL && f.runs[i].TargetSHA == sha { return f.runs[i], true, nil } } @@ -136,12 +146,15 @@ type fakeLauncher struct { notified bool gotSpec LaunchSpec gotHandle string + specs []LaunchSpec + handles []string } func (f *fakeLauncher) Spawn(_ context.Context, spec LaunchSpec) (string, error) { f.spawned = true f.spawnCount++ f.gotSpec = spec + f.specs = append(f.specs, spec) if f.spawnErr != nil { return "", f.spawnErr } @@ -151,6 +164,8 @@ func (f *fakeLauncher) Notify(_ context.Context, handleID string, spec LaunchSpe f.notified = true f.gotHandle = handleID f.gotSpec = spec + f.handles = append(f.handles, handleID) + f.specs = append(f.specs, spec) return f.notifyErr } func (f *fakeLauncher) Alive(_ context.Context, _ string) (bool, error) { @@ -176,7 +191,7 @@ func newEngineForTest(store Store, sessions Sessions, prs PRs, projects Projects } func prAt(sha string) fakePRs { - return fakePRs{prs: []domain.PullRequest{{URL: "https://github.com/o/r/pull/1", HeadSHA: sha}}} + return fakePRs{prs: []domain.PullRequest{{URL: "https://github.com/o/r/pull/1", Number: 1, HeadSHA: sha}}} } // --- tests --- @@ -259,7 +274,7 @@ func TestTriggerFallsBackToExistingRunOnUniqueConflict(t *testing.T) { if res.Created { t.Fatalf("expected Created=false on unique conflict: %+v", res) } - if res.Run.TargetSHA != "sha1" || res.Run.ID != "winner-id-1" { + if res.Run.TargetSHA != "sha1" || !strings.HasPrefix(res.Run.ID, "winner-") { t.Fatalf("expected the recorded winner run, got %+v", res.Run) } if launcher.spawnCount != 0 { @@ -271,7 +286,7 @@ func TestTriggerIsIdempotentForSameCommit(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, runs: []domain.ReviewRun{{ - ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1", + ID: "run-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, }}, } @@ -296,7 +311,7 @@ func TestTriggerIsIdempotentForSameCommit(t *testing.T) { func TestTriggerReusesRunningRowWithNoVerdict(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}}, + runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}}, } launcher := &fakeLauncher{alive: false, handle: "review-mer-2"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -319,7 +334,7 @@ func TestTriggerReusesRunningRowWithNoVerdict(t *testing.T) { func TestTriggerSupersedesNonRunningRowWithNoVerdict(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1", Status: domain.ReviewRunComplete}}, + runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunComplete}}, } launcher := &fakeLauncher{alive: true, handle: "review-mer-1"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -342,7 +357,7 @@ func TestTriggerSupersedesNonRunningRowWithNoVerdict(t *testing.T) { func TestTriggerNotifiesLiveReviewerOnNewCommit(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, + runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, } launcher := &fakeLauncher{alive: true} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -365,7 +380,7 @@ func TestTriggerNotifiesLiveReviewerOnNewCommit(t *testing.T) { func TestTriggerSupersedesOlderRunningRunOnNewCommit(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-old", SessionID: "mer-1", TargetSHA: "sha0", Status: domain.ReviewRunRunning}}, + runs: []domain.ReviewRun{{ID: "run-old", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha0", Status: domain.ReviewRunRunning}}, } launcher := &fakeLauncher{alive: true, handle: "review-mer-1"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -388,7 +403,7 @@ func TestTriggerSupersedesOlderRunningRunOnNewCommit(t *testing.T) { func TestTriggerSpawnsWhenReviewerDead(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, + runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, } launcher := &fakeLauncher{alive: false, handle: "review-mer-1"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -424,7 +439,7 @@ func TestTriggerLaunchFailureRecordsFailedRun(t *testing.T) { func TestTriggerRetriesAfterFailedRunForSameCommit(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-failed", ReviewID: "rev-1", SessionID: "mer-1", TargetSHA: "sha1", Status: domain.ReviewRunFailed}}, + runs: []domain.ReviewRun{{ID: "run-failed", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunFailed}}, } launcher := &fakeLauncher{handle: "review-mer-1"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) @@ -441,6 +456,111 @@ func TestTriggerRetriesAfterFailedRunForSameCommit(t *testing.T) { } } +func TestTriggerCreatesRunsForMultipleEligiblePRsWithOneReviewer(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{handle: "review-mer-1"} + prs := fakePRs{prs: []domain.PullRequest{ + {URL: "https://github.com/o/r/pull/1", Number: 1, HeadSHA: "sha1"}, + {URL: "https://github.com/o/r/pull/2", Number: 2, HeadSHA: "sha2"}, + }} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prs, fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !res.Created || len(res.CreatedRuns) != 2 || len(store.runs) != 2 { + t.Fatalf("created batch = %+v runs=%+v", res, store.runs) + } + if res.CreatedRuns[0].BatchID == "" || res.CreatedRuns[0].BatchID != res.CreatedRuns[1].BatchID { + t.Fatalf("created runs should share one batch id: %+v", res.CreatedRuns) + } + if launcher.spawnCount != 1 || len(launcher.handles) != 0 { + t.Fatalf("expected one spawn and no extra notify, launcher=%+v", launcher) + } + if len(launcher.specs) != 1 { + t.Fatalf("launch specs = %d, want 1: %+v", len(launcher.specs), launcher.specs) + } + spec := launcher.specs[0] + if spec.ReviewIndex != 0 || len(spec.ReviewQueue) != 2 { + t.Fatalf("spec queue context = index %d queue %+v", spec.ReviewIndex, spec.ReviewQueue) + } + if spec.ReviewQueue[0].PRURL != "https://github.com/o/r/pull/1" || spec.ReviewQueue[1].PRURL != "https://github.com/o/r/pull/2" { + t.Fatalf("spec queue URLs = %+v", spec.ReviewQueue) + } + if store.review == nil || store.review.ReviewerHandleID != "review-mer-1" || store.review.PRURL != "" { + t.Fatalf("review row = %+v, want shared handle and no behavioral pr_url", store.review) + } +} + +func TestTriggerAllowsTwoPRsWithSameHeadSHA(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{handle: "review-mer-1"} + prs := fakePRs{prs: []domain.PullRequest{ + {URL: "https://github.com/o/r/pull/1", Number: 1, HeadSHA: "same"}, + {URL: "https://github.com/o/r/pull/2", Number: 2, HeadSHA: "same"}, + }} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prs, fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if len(res.CreatedRuns) != 2 { + t.Fatalf("created runs = %d, want 2: %+v", len(res.CreatedRuns), res.CreatedRuns) + } + if store.runs[0].PRURL == store.runs[1].PRURL || store.runs[0].TargetSHA != store.runs[1].TargetSHA { + t.Fatalf("runs should differ by PR only: %+v", store.runs) + } +} + +func TestTriggerSkipsApprovedAndRunningCurrentHead(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{ + {ID: "approved", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, CreatedAt: time.Unix(1, 0)}, + {ID: "running", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2", Status: domain.ReviewRunRunning, CreatedAt: time.Unix(2, 0)}, + }, + } + launcher := &fakeLauncher{alive: true} + prs := fakePRs{prs: []domain.PullRequest{ + {URL: "https://github.com/o/r/pull/1", Number: 1, HeadSHA: "sha1"}, + {URL: "https://github.com/o/r/pull/2", Number: 2, HeadSHA: "sha2"}, + }} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prs, fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if res.Created || len(res.CreatedRuns) != 0 || launcher.spawned || launcher.notified { + t.Fatalf("expected no new work: res=%+v launcher=%+v", res, launcher) + } + if len(res.Reviews) != 2 || res.Reviews[0].Status != ReviewStateUpToDate || res.Reviews[1].Status != ReviewStateRunning { + t.Fatalf("review states = %+v", res.Reviews) + } +} + +func TestTriggerCreatesRunForChangesRequestedCurrentHead(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ + ID: "changes", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", + Status: domain.ReviewRunComplete, Verdict: domain.VerdictChangesRequested, CreatedAt: time.Unix(1, 0), + }}, + } + launcher := &fakeLauncher{alive: true, handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !res.Created || len(res.CreatedRuns) != 1 || !launcher.notified || launcher.spawned { + t.Fatalf("expected rerun on changes_requested current head: res=%+v launcher=%+v", res, launcher) + } +} + func TestTriggerUsesConfiguredReviewerHarness(t *testing.T) { store := &fakeStore{} projects := fakeProjects{cfg: domain.ProjectConfig{Reviewers: []domain.ReviewerConfig{{Harness: domain.ReviewerHarness("greptile")}}}} @@ -474,7 +594,7 @@ func TestTriggerRejectsBadWorkerState(t *testing.T) { func TestListReturnsHandleAndRuns(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1"}}, + runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}}, } eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, &fakeLauncher{}) got, err := eng.List(context.Background(), "mer-1") diff --git a/backend/internal/service/review/review.go b/backend/internal/service/review/review.go index be23d3a83..61f09e3b7 100644 --- a/backend/internal/service/review/review.go +++ b/backend/internal/service/review/review.go @@ -27,6 +27,7 @@ var ( type Manager interface { Trigger(ctx context.Context, workerID domain.SessionID) (reviewcore.TriggerResult, error) Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body, githubReviewID string) (domain.ReviewRun, error) + SubmitMany(ctx context.Context, workerID domain.SessionID, reviews []SubmittedReview) ([]domain.ReviewRun, error) List(ctx context.Context, workerID domain.SessionID) (reviewcore.SessionReviews, error) } @@ -45,12 +46,14 @@ type Store interface { GetReviewRun(ctx context.Context, id string) (domain.ReviewRun, bool, error) UpdateReviewRunResult(ctx context.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body, githubReviewID string) (bool, error) MarkReviewRunDelivered(ctx context.Context, id string, deliveredAt time.Time) (bool, error) + ListPRsBySession(ctx context.Context, id domain.SessionID) ([]domain.PullRequest, error) } // Reducer is the lifecycle reaction boundary used after a review result has // been persisted. type Reducer interface { ApplyReviewResult(ctx context.Context, workerID domain.SessionID, result lifecycle.ReviewResult) (lifecycle.ReviewDeliveryOutcome, error) + ApplyReviewBatch(ctx context.Context, workerID domain.SessionID, batchID string, results []lifecycle.ReviewResult) (lifecycle.ReviewDeliveryOutcome, error) } // Option customizes the review service. @@ -68,7 +71,11 @@ func WithClock(clock func() time.Time) Option { // New wraps a core review engine as the API-facing service. func New(engine *reviewcore.Engine, store Store, opts ...Option) *Service { - s := &Service{engine: engine, store: store, clock: func() time.Time { return time.Now().UTC() }} + s := &Service{ + engine: engine, + store: store, + clock: func() time.Time { return time.Now().UTC() }, + } for _, opt := range opts { opt(s) } @@ -80,11 +87,76 @@ func (s *Service) Trigger(ctx context.Context, workerID domain.SessionID) (revie return s.engine.Trigger(ctx, workerID) } +// SubmittedReview is one review result supplied by the reviewer CLI. +type SubmittedReview struct { + RunID string + Verdict domain.ReviewVerdict + Body string + GithubReviewID string +} + // Submit records a reviewer's result for a specific worker review pass. func (s *Service) Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body, githubReviewID string) (domain.ReviewRun, error) { - if workerID == "" { - return domain.ReviewRun{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) + runs, err := s.SubmitMany(ctx, workerID, []SubmittedReview{{ + RunID: runID, + Verdict: verdict, + Body: body, + GithubReviewID: githubReviewID, + }}) + if err != nil { + return domain.ReviewRun{}, err } + if len(runs) == 0 { + return domain.ReviewRun{}, fmt.Errorf("%w: no review result submitted", ErrInvalid) + } + return runs[0], nil +} + +// SubmitMany records one reviewer CLI submission containing results for one or +// more PR-scoped runs. Delivery is scoped to the runs in this submission, so a +// missing/stuck result for another PR in the same trigger cannot block feedback. +func (s *Service) SubmitMany(ctx context.Context, workerID domain.SessionID, reviews []SubmittedReview) ([]domain.ReviewRun, error) { + if workerID == "" { + return nil, fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + if len(reviews) == 0 { + return nil, fmt.Errorf("%w: at least one review result is required", ErrInvalid) + } + if s.store == nil { + return nil, fmt.Errorf("review service store is not configured") + } + runs := make([]domain.ReviewRun, 0, len(reviews)) + for _, review := range reviews { + run, err := s.submitOne(ctx, workerID, review) + if err != nil { + return nil, err + } + runs = append(runs, run) + } + if s.lifecycle == nil { + return runs, nil + } + delivered, err := s.deliverSubmitted(ctx, workerID, runs) + if err != nil { + return nil, err + } + byID := make(map[string]domain.ReviewRun, len(delivered)) + for _, run := range delivered { + byID[run.ID] = run + } + for i, run := range runs { + if deliveredRun, ok := byID[run.ID]; ok { + runs[i] = deliveredRun + } + } + return runs, nil +} + +func (s *Service) submitOne(ctx context.Context, workerID domain.SessionID, review SubmittedReview) (domain.ReviewRun, error) { + runID := review.RunID + verdict := review.Verdict + body := review.Body + githubReviewID := review.GithubReviewID if runID == "" { return domain.ReviewRun{}, fmt.Errorf("%w: review run id is required", ErrInvalid) } @@ -94,9 +166,6 @@ func (s *Service) Submit(ctx context.Context, workerID domain.SessionID, runID s if verdict == domain.VerdictChangesRequested && body == "" { return domain.ReviewRun{}, fmt.Errorf("%w: a changes_requested review requires a body", ErrInvalid) } - if s.store == nil { - return domain.ReviewRun{}, fmt.Errorf("review service store is not configured") - } run, ok, err := s.store.GetReviewRun(ctx, runID) if err != nil { return domain.ReviewRun{}, err @@ -136,35 +205,92 @@ func (s *Service) Submit(ctx context.Context, workerID domain.SessionID, runID s default: return domain.ReviewRun{}, fmt.Errorf("%w: review run %q is not running", ErrInvalid, runID) } + return run, nil +} - if s.lifecycle == nil { - return run, nil - } - outcome, err := s.lifecycle.ApplyReviewResult(ctx, workerID, lifecycle.ReviewResult{ - RunID: run.ID, - WorkerID: workerID, - PRURL: run.PRURL, - TargetSHA: run.TargetSHA, - Verdict: run.Verdict, - Body: run.Body, - GithubReviewID: run.GithubReviewID, - DeliveredAt: run.DeliveredAt, - }) +func (s *Service) deliverSubmitted(ctx context.Context, workerID domain.SessionID, runs []domain.ReviewRun) ([]domain.ReviewRun, error) { + deliverable, err := s.deliverableRuns(ctx, workerID, runs) if err != nil { - return domain.ReviewRun{}, err + return nil, err } - if outcome == lifecycle.ReviewDeliverySent { - deliveredAt := s.clock() + if len(deliverable) == 0 { + return nil, nil + } + results := reviewResults(workerID, deliverable) + var outcome lifecycle.ReviewDeliveryOutcome + if len(results) == 1 && results[0].BatchID == "" { + outcome, err = s.lifecycle.ApplyReviewResult(ctx, workerID, results[0]) + } else { + outcome, err = s.lifecycle.ApplyReviewBatch(ctx, workerID, results[0].BatchID, results) + } + if err != nil { + return nil, err + } + if outcome != lifecycle.ReviewDeliverySent { + return nil, nil + } + deliveredAt := s.clock() + delivered := make([]domain.ReviewRun, 0, len(deliverable)) + for _, run := range deliverable { updated, err := s.store.MarkReviewRunDelivered(ctx, run.ID, deliveredAt) if err != nil { - return domain.ReviewRun{}, err + return nil, err } if updated { run.Status = domain.ReviewRunDelivered run.DeliveredAt = &deliveredAt + delivered = append(delivered, run) } } - return run, nil + return delivered, nil +} + +func (s *Service) deliverableRuns(ctx context.Context, workerID domain.SessionID, runs []domain.ReviewRun) ([]domain.ReviewRun, error) { + currentHeads, err := s.currentHeadsByPR(ctx, workerID) + if err != nil { + return nil, err + } + deliverable := make([]domain.ReviewRun, 0, len(runs)) + for _, run := range runs { + if run.Status != domain.ReviewRunComplete || run.Verdict != domain.VerdictChangesRequested || run.DeliveredAt != nil { + continue + } + if run.BatchID != "" && currentHeads[run.PRURL] != run.TargetSHA { + continue + } + deliverable = append(deliverable, run) + } + return deliverable, nil +} + +func reviewResults(workerID domain.SessionID, runs []domain.ReviewRun) []lifecycle.ReviewResult { + results := make([]lifecycle.ReviewResult, 0, len(runs)) + for _, run := range runs { + results = append(results, lifecycle.ReviewResult{ + RunID: run.ID, + BatchID: run.BatchID, + WorkerID: workerID, + PRURL: run.PRURL, + TargetSHA: run.TargetSHA, + Verdict: run.Verdict, + Body: run.Body, + GithubReviewID: run.GithubReviewID, + DeliveredAt: run.DeliveredAt, + }) + } + return results +} + +func (s *Service) currentHeadsByPR(ctx context.Context, workerID domain.SessionID) (map[string]string, error) { + prs, err := s.store.ListPRsBySession(ctx, workerID) + if err != nil { + return nil, err + } + current := make(map[string]string, len(prs)) + for _, pr := range prs { + current[pr.URL] = pr.HeadSHA + } + return current, nil } // List returns a worker's review state. diff --git a/backend/internal/service/review/review_test.go b/backend/internal/service/review/review_test.go index c9e4c4553..259c47f56 100644 --- a/backend/internal/service/review/review_test.go +++ b/backend/internal/service/review/review_test.go @@ -11,18 +11,45 @@ import ( ) type fakeStore struct { - run domain.ReviewRun - ok bool + run domain.ReviewRun + ok bool + batchRuns []domain.ReviewRun + prs []domain.PullRequest updateCalls int markCalls int + markedIDs []string } -func (f *fakeStore) GetReviewRun(context.Context, string) (domain.ReviewRun, bool, error) { - return f.run, f.ok, nil +func (f *fakeStore) GetReviewRun(_ context.Context, id string) (domain.ReviewRun, bool, error) { + for _, run := range f.batchRuns { + if run.ID == id { + return run, true, nil + } + } + if f.ok && f.run.ID == id { + return f.run, true, nil + } + return domain.ReviewRun{}, false, nil } -func (f *fakeStore) UpdateReviewRunResult(_ context.Context, _ string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body, githubReviewID string) (bool, error) { +func (f *fakeStore) UpdateReviewRunResult(_ context.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body, githubReviewID string) (bool, error) { + for i := range f.batchRuns { + if f.batchRuns[i].ID == id { + if f.batchRuns[i].Status != domain.ReviewRunRunning { + return false, nil + } + f.updateCalls++ + f.batchRuns[i].Status = status + f.batchRuns[i].Verdict = verdict + f.batchRuns[i].Body = body + f.batchRuns[i].GithubReviewID = githubReviewID + if f.run.ID == id { + f.run = f.batchRuns[i] + } + return true, nil + } + } if f.run.Status != domain.ReviewRunRunning { return false, nil } @@ -34,21 +61,44 @@ func (f *fakeStore) UpdateReviewRunResult(_ context.Context, _ string, status do return true, nil } -func (f *fakeStore) MarkReviewRunDelivered(_ context.Context, _ string, deliveredAt time.Time) (bool, error) { +func (f *fakeStore) MarkReviewRunDelivered(_ context.Context, id string, deliveredAt time.Time) (bool, error) { f.markCalls++ - if f.run.Status != domain.ReviewRunComplete || f.run.DeliveredAt != nil { + f.markedIDs = append(f.markedIDs, id) + if f.run.ID == id && f.run.Status == domain.ReviewRunComplete && f.run.DeliveredAt == nil { + f.run.Status = domain.ReviewRunDelivered + f.run.DeliveredAt = &deliveredAt + } + for i := range f.batchRuns { + if f.batchRuns[i].ID == id && f.batchRuns[i].Status == domain.ReviewRunComplete && f.batchRuns[i].DeliveredAt == nil { + f.batchRuns[i].Status = domain.ReviewRunDelivered + f.batchRuns[i].DeliveredAt = &deliveredAt + return true, nil + } + } + if f.run.ID != id || f.run.Status != domain.ReviewRunDelivered { return false, nil } - f.run.Status = domain.ReviewRunDelivered - f.run.DeliveredAt = &deliveredAt return true, nil } +func (f *fakeStore) ListReviewRunsByBatch(context.Context, domain.SessionID, string) ([]domain.ReviewRun, error) { + out := append([]domain.ReviewRun(nil), f.batchRuns...) + return out, nil +} + +func (f *fakeStore) ListPRsBySession(context.Context, domain.SessionID) ([]domain.PullRequest, error) { + out := append([]domain.PullRequest(nil), f.prs...) + return out, nil +} + type fakeReducer struct { - outcome lifecycle.ReviewDeliveryOutcome - err error - calls int - got lifecycle.ReviewResult + outcome lifecycle.ReviewDeliveryOutcome + err error + calls int + batchCalls int + got lifecycle.ReviewResult + gotBatchID string + gotBatch []lifecycle.ReviewResult } func (f *fakeReducer) ApplyReviewResult(_ context.Context, _ domain.SessionID, result lifecycle.ReviewResult) (lifecycle.ReviewDeliveryOutcome, error) { @@ -57,6 +107,13 @@ func (f *fakeReducer) ApplyReviewResult(_ context.Context, _ domain.SessionID, r return f.outcome, f.err } +func (f *fakeReducer) ApplyReviewBatch(_ context.Context, _ domain.SessionID, batchID string, results []lifecycle.ReviewResult) (lifecycle.ReviewDeliveryOutcome, error) { + f.batchCalls++ + f.gotBatchID = batchID + f.gotBatch = append([]lifecycle.ReviewResult(nil), results...) + return f.outcome, f.err +} + func TestSubmitPersistsThenAppliesThenStampsDelivered(t *testing.T) { now := time.Unix(100, 0).UTC() st := &fakeStore{ok: true, run: domain.ReviewRun{ID: "run-1", SessionID: "mer-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}} @@ -78,6 +135,98 @@ func TestSubmitPersistsThenAppliesThenStampsDelivered(t *testing.T) { } } +func TestSubmitBatchRunDoesNotWaitForOtherRunningRuns(t *testing.T) { + now := time.Unix(100, 0).UTC() + st := &fakeStore{ + ok: true, + run: domain.ReviewRun{ID: "run-1", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}, + batchRuns: []domain.ReviewRun{ + {ID: "run-1", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}, + {ID: "run-2", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr2", TargetSHA: "sha2", Status: domain.ReviewRunRunning}, + }, + prs: []domain.PullRequest{{URL: "pr1", HeadSHA: "sha1"}, {URL: "pr2", HeadSHA: "sha2"}}, + } + reducer := &fakeReducer{outcome: lifecycle.ReviewDeliverySent} + svc := New(nil, st, WithLifecycleReducer(reducer), WithClock(func() time.Time { return now })) + + run, err := svc.Submit(context.Background(), "mer-1", "run-1", domain.VerdictChangesRequested, "fix pr1", "101") + if err != nil { + t.Fatalf("Submit: %v", err) + } + if run.Status != domain.ReviewRunDelivered || run.DeliveredAt == nil || !run.DeliveredAt.Equal(now) { + t.Fatalf("first submit status = %+v, want delivered", run) + } + if reducer.batchCalls != 1 || len(reducer.gotBatch) != 1 || reducer.gotBatch[0].RunID != "run-1" || st.markCalls != 1 { + t.Fatalf("submitted run should deliver independently: batchCalls=%d got=%+v markCalls=%d", reducer.batchCalls, reducer.gotBatch, st.markCalls) + } +} + +func TestSubmitManySendsCombinedChangesRequested(t *testing.T) { + now := time.Unix(100, 0).UTC() + st := &fakeStore{ + ok: true, + batchRuns: []domain.ReviewRun{ + {ID: "run-1", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}, + {ID: "run-2", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr2", TargetSHA: "sha2", Status: domain.ReviewRunRunning}, + {ID: "run-3", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr3", TargetSHA: "sha3", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved}, + {ID: "run-4", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr4", TargetSHA: "old", Status: domain.ReviewRunComplete, Verdict: domain.VerdictChangesRequested, Body: "stale"}, + {ID: "run-5", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr5", TargetSHA: "sha5", Status: domain.ReviewRunFailed}, + }, + prs: []domain.PullRequest{ + {URL: "pr1", HeadSHA: "sha1"}, + {URL: "pr2", HeadSHA: "sha2"}, + {URL: "pr3", HeadSHA: "sha3"}, + {URL: "pr4", HeadSHA: "new"}, + {URL: "pr5", HeadSHA: "sha5"}, + }, + } + reducer := &fakeReducer{outcome: lifecycle.ReviewDeliverySent} + svc := New(nil, st, WithLifecycleReducer(reducer), WithClock(func() time.Time { return now })) + + runs, err := svc.SubmitMany(context.Background(), "mer-1", []SubmittedReview{ + {RunID: "run-1", Verdict: domain.VerdictChangesRequested, Body: "fix pr1", GithubReviewID: "101"}, + {RunID: "run-2", Verdict: domain.VerdictChangesRequested, Body: "fix pr2", GithubReviewID: "102"}, + {RunID: "run-3", Verdict: domain.VerdictApproved}, + }) + if err != nil { + t.Fatalf("SubmitMany: %v", err) + } + if reducer.batchCalls != 1 || reducer.gotBatchID != "batch-1" { + t.Fatalf("batch delivery calls/id = %d/%q", reducer.batchCalls, reducer.gotBatchID) + } + if len(reducer.gotBatch) != 2 || reducer.gotBatch[0].RunID != "run-1" || reducer.gotBatch[1].RunID != "run-2" { + t.Fatalf("delivered batch = %+v, want run-1 and run-2 only", reducer.gotBatch) + } + if st.markCalls != 2 { + t.Fatalf("markCalls = %d, want 2", st.markCalls) + } + if runs[0].Status != domain.ReviewRunDelivered || runs[0].DeliveredAt == nil || !runs[0].DeliveredAt.Equal(now) || + runs[1].Status != domain.ReviewRunDelivered || runs[1].DeliveredAt == nil || !runs[1].DeliveredAt.Equal(now) { + t.Fatalf("submitted runs not stamped delivered: %+v", runs) + } +} + +func TestSubmitBatchApprovedOnlySendsNothing(t *testing.T) { + st := &fakeStore{ + ok: true, + run: domain.ReviewRun{ID: "run-2", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr2", TargetSHA: "sha2", Status: domain.ReviewRunRunning}, + batchRuns: []domain.ReviewRun{ + {ID: "run-1", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved}, + {ID: "run-2", SessionID: "mer-1", BatchID: "batch-1", PRURL: "pr2", TargetSHA: "sha2", Status: domain.ReviewRunRunning}, + }, + prs: []domain.PullRequest{{URL: "pr1", HeadSHA: "sha1"}, {URL: "pr2", HeadSHA: "sha2"}}, + } + reducer := &fakeReducer{outcome: lifecycle.ReviewDeliverySent} + svc := New(nil, st, WithLifecycleReducer(reducer)) + + if _, err := svc.Submit(context.Background(), "mer-1", "run-2", domain.VerdictApproved, "", "102"); err != nil { + t.Fatalf("Submit: %v", err) + } + if reducer.batchCalls != 0 || st.markCalls != 0 { + t.Fatalf("approved-only batch should not deliver: batchCalls=%d markCalls=%d", reducer.batchCalls, st.markCalls) + } +} + func TestSubmitDeliveryFailureLeavesCompletedUndeliveredForRetry(t *testing.T) { sendErr := errors.New("dead pane") st := &fakeStore{ok: true, run: domain.ReviewRun{ID: "run-1", SessionID: "mer-1", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}} diff --git a/backend/internal/storage/sqlite/gen/models.go b/backend/internal/storage/sqlite/gen/models.go index f3ca38655..ccc4bf74a 100644 --- a/backend/internal/storage/sqlite/gen/models.go +++ b/backend/internal/storage/sqlite/gen/models.go @@ -147,6 +147,7 @@ type ReviewRun struct { CreatedAt time.Time GithubReviewID string DeliveredAt sql.NullTime + BatchID string } type Session struct { diff --git a/backend/internal/storage/sqlite/gen/review.sql.go b/backend/internal/storage/sqlite/gen/review.sql.go index f9d08f344..16a939aee 100644 --- a/backend/internal/storage/sqlite/gen/review.sql.go +++ b/backend/internal/storage/sqlite/gen/review.sql.go @@ -35,7 +35,7 @@ func (q *Queries) GetReviewBySession(ctx context.Context, sessionID domain.Sessi } const getReviewRun = `-- name: GetReviewRun :one -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE id = ? ` @@ -55,22 +55,24 @@ func (q *Queries) GetReviewRun(ctx context.Context, id string) (ReviewRun, error &i.CreatedAt, &i.GithubReviewID, &i.DeliveredAt, + &i.BatchID, ) return i, err } -const getReviewRunBySessionAndSHA = `-- name: GetReviewRunBySessionAndSHA :one -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at -FROM review_run WHERE session_id = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1 +const getReviewRunBySessionPRAndSHA = `-- name: GetReviewRunBySessionPRAndSHA :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND pr_url = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1 ` -type GetReviewRunBySessionAndSHAParams struct { +type GetReviewRunBySessionPRAndSHAParams struct { SessionID domain.SessionID + PRURL string TargetSha string } -func (q *Queries) GetReviewRunBySessionAndSHA(ctx context.Context, arg GetReviewRunBySessionAndSHAParams) (ReviewRun, error) { - row := q.db.QueryRowContext(ctx, getReviewRunBySessionAndSHA, arg.SessionID, arg.TargetSha) +func (q *Queries) GetReviewRunBySessionPRAndSHA(ctx context.Context, arg GetReviewRunBySessionPRAndSHAParams) (ReviewRun, error) { + row := q.db.QueryRowContext(ctx, getReviewRunBySessionPRAndSHA, arg.SessionID, arg.PRURL, arg.TargetSha) var i ReviewRun err := row.Scan( &i.ID, @@ -85,19 +87,21 @@ func (q *Queries) GetReviewRunBySessionAndSHA(ctx context.Context, arg GetReview &i.CreatedAt, &i.GithubReviewID, &i.DeliveredAt, + &i.BatchID, ) return i, err } const insertReviewRun = `-- name: InsertReviewRun :exec -INSERT INTO review_run (id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, github_review_id, created_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) +INSERT INTO review_run (id, review_id, session_id, batch_id, harness, pr_url, target_sha, status, verdict, body, github_review_id, created_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ` type InsertReviewRunParams struct { ID string ReviewID string SessionID domain.SessionID + BatchID string Harness domain.ReviewerHarness PRURL string TargetSha string @@ -113,6 +117,7 @@ func (q *Queries) InsertReviewRun(ctx context.Context, arg InsertReviewRunParams arg.ID, arg.ReviewID, arg.SessionID, + arg.BatchID, arg.Harness, arg.PRURL, arg.TargetSha, @@ -125,8 +130,55 @@ func (q *Queries) InsertReviewRun(ctx context.Context, arg InsertReviewRunParams return err } +const listReviewRunsByBatch = `-- name: ListReviewRunsByBatch :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND batch_id = ? ORDER BY created_at ASC, id ASC +` + +type ListReviewRunsByBatchParams struct { + SessionID domain.SessionID + BatchID string +} + +func (q *Queries) ListReviewRunsByBatch(ctx context.Context, arg ListReviewRunsByBatchParams) ([]ReviewRun, error) { + rows, err := q.db.QueryContext(ctx, listReviewRunsByBatch, arg.SessionID, arg.BatchID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []ReviewRun{} + for rows.Next() { + var i ReviewRun + if err := rows.Scan( + &i.ID, + &i.ReviewID, + &i.SessionID, + &i.Harness, + &i.PRURL, + &i.TargetSha, + &i.Status, + &i.Verdict, + &i.Body, + &i.CreatedAt, + &i.GithubReviewID, + &i.DeliveredAt, + &i.BatchID, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listReviewRunsBySession = `-- name: ListReviewRunsBySession :many -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE session_id = ? ORDER BY created_at DESC ` @@ -152,6 +204,7 @@ func (q *Queries) ListReviewRunsBySession(ctx context.Context, sessionID domain. &i.CreatedAt, &i.GithubReviewID, &i.DeliveredAt, + &i.BatchID, ); err != nil { return nil, err } @@ -201,17 +254,23 @@ func (q *Queries) SupersedeReviewRun(ctx context.Context, arg SupersedeReviewRun } const supersedeStaleRunningReviewRuns = `-- name: SupersedeStaleRunningReviewRuns :execrows -UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND target_sha != ? AND status = 'running' AND verdict = '' +UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND pr_url = ? AND target_sha != ? AND status = 'running' AND verdict = '' ` type SupersedeStaleRunningReviewRunsParams struct { Body string SessionID domain.SessionID + PRURL string TargetSha string } func (q *Queries) SupersedeStaleRunningReviewRuns(ctx context.Context, arg SupersedeStaleRunningReviewRunsParams) (int64, error) { - result, err := q.db.ExecContext(ctx, supersedeStaleRunningReviewRuns, arg.Body, arg.SessionID, arg.TargetSha) + result, err := q.db.ExecContext(ctx, supersedeStaleRunningReviewRuns, + arg.Body, + arg.SessionID, + arg.PRURL, + arg.TargetSha, + ) if err != nil { return 0, err } diff --git a/backend/internal/storage/sqlite/migrations/0020_review_run_unique_pr_sha.sql b/backend/internal/storage/sqlite/migrations/0020_review_run_unique_pr_sha.sql new file mode 100644 index 000000000..97768c8a6 --- /dev/null +++ b/backend/internal/storage/sqlite/migrations/0020_review_run_unique_pr_sha.sql @@ -0,0 +1,88 @@ +-- Review runs are PR-scoped. Two PRs in one worker session can legitimately +-- share a head SHA, so the idempotency index must include pr_url. Completed +-- changes_requested runs are intentionally excluded so the same head can be +-- reviewed again after the worker applies feedback. Runs created by one trigger +-- also share batch_id so one reviewer CLI submission can notify the worker about +-- multiple PRs at once. + +-- +goose Up +-- +goose StatementBegin +ALTER TABLE review_run ADD COLUMN batch_id TEXT NOT NULL DEFAULT ''; +-- +goose StatementEnd + +-- +goose StatementBegin +DROP INDEX idx_review_run_session_sha; +-- +goose StatementEnd + +-- +goose StatementBegin +DELETE FROM review_run +WHERE target_sha != '' + AND pr_url != '' + AND status != 'failed' + AND verdict != 'changes_requested' + AND rowid NOT IN ( + SELECT rowid FROM ( + SELECT rowid, + ROW_NUMBER() OVER ( + PARTITION BY session_id, pr_url, target_sha + ORDER BY CASE status WHEN 'complete' THEN 0 WHEN 'delivered' THEN 0 WHEN 'running' THEN 1 ELSE 2 END, + created_at DESC, + rowid DESC + ) AS rn + FROM review_run + WHERE target_sha != '' AND pr_url != '' AND status != 'failed' AND verdict != 'changes_requested' + ) + WHERE rn = 1 + ); +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE UNIQUE INDEX idx_review_run_session_pr_sha + ON review_run (session_id, pr_url, target_sha) + WHERE target_sha != '' AND status != 'failed' AND verdict != 'changes_requested'; +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE INDEX idx_review_run_session_batch + ON review_run (session_id, batch_id, created_at) + WHERE batch_id != ''; +-- +goose StatementEnd + +-- +goose Down +-- +goose StatementBegin +DROP INDEX idx_review_run_session_batch; +-- +goose StatementEnd + +-- +goose StatementBegin +DROP INDEX idx_review_run_session_pr_sha; +-- +goose StatementEnd + +-- +goose StatementBegin +DELETE FROM review_run +WHERE target_sha != '' + AND status != 'failed' + AND rowid NOT IN ( + SELECT rowid FROM ( + SELECT rowid, + ROW_NUMBER() OVER ( + PARTITION BY session_id, target_sha + ORDER BY CASE status WHEN 'complete' THEN 0 WHEN 'delivered' THEN 0 WHEN 'running' THEN 1 ELSE 2 END, + created_at DESC, + rowid DESC + ) AS rn + FROM review_run + WHERE target_sha != '' AND status != 'failed' + ) + WHERE rn = 1 + ); +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE UNIQUE INDEX idx_review_run_session_sha + ON review_run (session_id, target_sha) + WHERE target_sha != '' AND status != 'failed'; +-- +goose StatementEnd + +-- +goose StatementBegin +ALTER TABLE review_run DROP COLUMN batch_id; +-- +goose StatementEnd diff --git a/backend/internal/storage/sqlite/queries/review.sql b/backend/internal/storage/sqlite/queries/review.sql index 2dbc3a087..335f87cdf 100644 --- a/backend/internal/storage/sqlite/queries/review.sql +++ b/backend/internal/storage/sqlite/queries/review.sql @@ -12,8 +12,8 @@ SELECT id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_ FROM review WHERE session_id = ?; -- name: InsertReviewRun :exec -INSERT INTO review_run (id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, github_review_id, created_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?); +INSERT INTO review_run (id, review_id, session_id, batch_id, harness, pr_url, target_sha, status, verdict, body, github_review_id, created_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?); -- name: UpdateReviewRunResult :execrows UPDATE review_run SET status = ?, verdict = ?, body = ?, github_review_id = ? WHERE id = ? AND status = 'running'; @@ -22,19 +22,23 @@ UPDATE review_run SET status = ?, verdict = ?, body = ?, github_review_id = ? WH UPDATE review_run SET status = 'failed', body = ? WHERE id = ? AND verdict = '' AND status != 'failed'; -- name: SupersedeStaleRunningReviewRuns :execrows -UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND target_sha != ? AND status = 'running' AND verdict = ''; +UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND pr_url = ? AND target_sha != ? AND status = 'running' AND verdict = ''; -- name: MarkReviewRunDelivered :execrows UPDATE review_run SET status = 'delivered', delivered_at = ? WHERE id = ? AND status = 'complete' AND delivered_at IS NULL; -- name: GetReviewRun :one -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE id = ?; --- name: GetReviewRunBySessionAndSHA :one -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at -FROM review_run WHERE session_id = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1; +-- name: GetReviewRunBySessionPRAndSHA :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND pr_url = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1; -- name: ListReviewRunsBySession :many -SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE session_id = ? ORDER BY created_at DESC; + +-- name: ListReviewRunsByBatch :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND batch_id = ? ORDER BY created_at ASC, id ASC; diff --git a/backend/internal/storage/sqlite/store/review_store.go b/backend/internal/storage/sqlite/store/review_store.go index cf5fd6460..9a62925ff 100644 --- a/backend/internal/storage/sqlite/store/review_store.go +++ b/backend/internal/storage/sqlite/store/review_store.go @@ -41,7 +41,7 @@ func (s *Store) GetReviewBySession(ctx context.Context, id domain.SessionID) (do } // InsertReviewRun records a new review pass. A unique-constraint hit on the -// (session_id, target_sha) index (migration 0013) is surfaced as the sentinel +// (session_id, pr_url, target_sha) index (migration 0020) is surfaced as the sentinel // domain.ErrDuplicateReviewRun so the engine can fall back to the existing run. func (s *Store) InsertReviewRun(ctx context.Context, r domain.ReviewRun) error { s.writeMu.Lock() @@ -50,6 +50,7 @@ func (s *Store) InsertReviewRun(ctx context.Context, r domain.ReviewRun) error { ID: r.ID, ReviewID: r.ReviewID, SessionID: r.SessionID, + BatchID: r.BatchID, Harness: r.Harness, PRURL: r.PRURL, TargetSha: r.TargetSHA, @@ -60,7 +61,7 @@ func (s *Store) InsertReviewRun(ctx context.Context, r domain.ReviewRun) error { CreatedAt: r.CreatedAt, }) if isSQLiteUnique(err) { - return fmt.Errorf("insert review run for session %s sha %s: %w", r.SessionID, r.TargetSHA, domain.ErrDuplicateReviewRun) + return fmt.Errorf("insert review run for session %s pr %s sha %s: %w", r.SessionID, r.PRURL, r.TargetSHA, domain.ErrDuplicateReviewRun) } return err } @@ -100,12 +101,13 @@ func (s *Store) SupersedeReviewRun(ctx context.Context, id, body string) (bool, // SupersedeStaleRunningReviewRuns marks older running unverdicted passes for a // worker failed before starting a review for a newer commit. -func (s *Store) SupersedeStaleRunningReviewRuns(ctx context.Context, sessionID domain.SessionID, targetSHA, body string) (int64, error) { +func (s *Store) SupersedeStaleRunningReviewRuns(ctx context.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) { s.writeMu.Lock() defer s.writeMu.Unlock() return s.qw.SupersedeStaleRunningReviewRuns(ctx, gen.SupersedeStaleRunningReviewRunsParams{ Body: body, SessionID: sessionID, + PRURL: prURL, TargetSha: targetSHA, }) } @@ -137,16 +139,17 @@ func (s *Store) GetReviewRun(ctx context.Context, id string) (domain.ReviewRun, return reviewRunFromRow(row), true, nil } -// GetReviewRunBySessionAndSHA returns the most recent review pass for a worker -// session at a specific commit, ok=false if none. It lets a repeat trigger for -// the same PR head short-circuit to the existing run. -func (s *Store) GetReviewRunBySessionAndSHA(ctx context.Context, id domain.SessionID, targetSHA string) (domain.ReviewRun, bool, error) { - row, err := s.qr.GetReviewRunBySessionAndSHA(ctx, gen.GetReviewRunBySessionAndSHAParams{SessionID: id, TargetSha: targetSHA}) +// GetReviewRunBySessionPRAndSHA returns the most recent review pass for a +// worker session, PR, and commit, ok=false if none. It lets a repeat trigger for +// the same PR head short-circuit to the existing run without colliding with +// another PR that happens to share the same head commit. +func (s *Store) GetReviewRunBySessionPRAndSHA(ctx context.Context, id domain.SessionID, prURL, targetSHA string) (domain.ReviewRun, bool, error) { + row, err := s.qr.GetReviewRunBySessionPRAndSHA(ctx, gen.GetReviewRunBySessionPRAndSHAParams{SessionID: id, PRURL: prURL, TargetSha: targetSHA}) if errors.Is(err, sql.ErrNoRows) { return domain.ReviewRun{}, false, nil } if err != nil { - return domain.ReviewRun{}, false, fmt.Errorf("get review run for session %s sha %s: %w", id, targetSHA, err) + return domain.ReviewRun{}, false, fmt.Errorf("get review run for session %s pr %s sha %s: %w", id, prURL, targetSHA, err) } return reviewRunFromRow(row), true, nil } @@ -164,6 +167,19 @@ func (s *Store) ListReviewRunsBySession(ctx context.Context, id domain.SessionID return out, nil } +// ListReviewRunsByBatch returns all passes in one trigger-created batch, oldest first. +func (s *Store) ListReviewRunsByBatch(ctx context.Context, id domain.SessionID, batchID string) ([]domain.ReviewRun, error) { + rows, err := s.qr.ListReviewRunsByBatch(ctx, gen.ListReviewRunsByBatchParams{SessionID: id, BatchID: batchID}) + if err != nil { + return nil, fmt.Errorf("list review runs for session %s batch %s: %w", id, batchID, err) + } + out := make([]domain.ReviewRun, 0, len(rows)) + for _, row := range rows { + out = append(out, reviewRunFromRow(row)) + } + return out, nil +} + func reviewFromRow(r gen.Review) domain.Review { return domain.Review{ ID: r.ID, @@ -187,6 +203,7 @@ func reviewRunFromRow(r gen.ReviewRun) domain.ReviewRun { ID: r.ID, ReviewID: r.ReviewID, SessionID: r.SessionID, + BatchID: r.BatchID, Harness: r.Harness, PRURL: r.PRURL, TargetSHA: r.TargetSha, diff --git a/backend/internal/storage/sqlite/store/review_store_test.go b/backend/internal/storage/sqlite/store/review_store_test.go index 399438261..e8ea62bb3 100644 --- a/backend/internal/storage/sqlite/store/review_store_test.go +++ b/backend/internal/storage/sqlite/store/review_store_test.go @@ -9,7 +9,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" ) -func TestInsertReviewRunDuplicateSHAMapsToSentinel(t *testing.T) { +func TestInsertReviewRunDuplicatePRSHAMapsToSentinel(t *testing.T) { s := newTestStore(t) ctx := context.Background() seedProject(t, s, "mer") @@ -26,21 +26,28 @@ func TestInsertReviewRunDuplicateSHAMapsToSentinel(t *testing.T) { } run := domain.ReviewRun{ ID: "run-1", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerClaudeCode, - TargetSHA: "sha1", Status: domain.ReviewRunRunning, Verdict: domain.VerdictNone, CreatedAt: now, + PRURL: "https://example/pr/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, Verdict: domain.VerdictNone, CreatedAt: now, } if err := s.InsertReviewRun(ctx, run); err != nil { t.Fatalf("first insert: %v", err) } - // A second run for the same (session_id, target_sha) hits the partial unique - // index (migration 0013) and must surface as the sentinel so the engine can - // fall back to the existing run. + // A second run for the same (session_id, pr_url, target_sha) hits the + // partial unique index (migration 0020) and must surface as the sentinel so + // the engine can fall back to the existing run. dup := run dup.ID = "run-2" if err := s.InsertReviewRun(ctx, dup); !errors.Is(err, domain.ErrDuplicateReviewRun) { t.Fatalf("duplicate insert err = %v, want ErrDuplicateReviewRun", err) } + otherPR := run + otherPR.ID = "run-other-pr" + otherPR.PRURL = "https://example/pr/2" + if err := s.InsertReviewRun(ctx, otherPR); err != nil { + t.Fatalf("same sha on different PR should insert: %v", err) + } + if ok, err := s.UpdateReviewRunResult(ctx, "run-1", domain.ReviewRunFailed, domain.VerdictNone, "claude: not found", ""); err != nil { t.Fatalf("mark failed: %v", err) } else if !ok { @@ -60,6 +67,42 @@ func TestInsertReviewRunDuplicateSHAMapsToSentinel(t *testing.T) { } } +func TestInsertReviewRunAllowsRerunAfterChangesRequested(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + seedProject(t, s, "mer") + rec, err := s.CreateSession(ctx, sampleRecord("mer")) + if err != nil { + t.Fatalf("create session: %v", err) + } + now := time.Now().UTC().Truncate(time.Second) + if err := s.UpsertReview(ctx, domain.Review{ + ID: "rev-1", SessionID: rec.ID, ProjectID: rec.ProjectID, + Harness: domain.ReviewerClaudeCode, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert review: %v", err) + } + run := domain.ReviewRun{ + ID: "run-1", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerClaudeCode, + PRURL: "https://example/pr/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, Verdict: domain.VerdictNone, CreatedAt: now, + } + if err := s.InsertReviewRun(ctx, run); err != nil { + t.Fatalf("first insert: %v", err) + } + if ok, err := s.UpdateReviewRunResult(ctx, "run-1", domain.ReviewRunComplete, domain.VerdictChangesRequested, "please fix", "rev-1"); err != nil { + t.Fatalf("mark changes requested: %v", err) + } else if !ok { + t.Fatal("mark changes requested: got ok=false") + } + + rerun := run + rerun.ID = "run-2" + rerun.CreatedAt = now.Add(time.Second) + if err := s.InsertReviewRun(ctx, rerun); err != nil { + t.Fatalf("rerun after changes_requested insert: %v", err) + } +} + func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { s := newTestStore(t) ctx := context.Background() @@ -102,7 +145,7 @@ func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { // A run inserts running and updates to complete/changes_requested. if err := s.InsertReviewRun(ctx, domain.ReviewRun{ - ID: "run-1", ReviewID: got.ID, SessionID: rec.ID, Harness: domain.ReviewerHarness("greptile"), + ID: "run-1", ReviewID: got.ID, SessionID: rec.ID, BatchID: "batch-1", Harness: domain.ReviewerHarness("greptile"), PRURL: got.PRURL, TargetSHA: "sha1", Status: domain.ReviewRunRunning, Verdict: domain.VerdictNone, CreatedAt: now, }); err != nil { @@ -118,18 +161,18 @@ func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { if err != nil || !ok { t.Fatalf("get run: ok=%v err=%v", ok, err) } - if gotRun.ID != "run-1" || gotRun.SessionID != rec.ID || gotRun.TargetSHA != "sha1" { + if gotRun.ID != "run-1" || gotRun.SessionID != rec.ID || gotRun.BatchID != "batch-1" || gotRun.TargetSHA != "sha1" { t.Fatalf("get run = %+v", gotRun) } - bySHA, ok, err := s.GetReviewRunBySessionAndSHA(ctx, rec.ID, "sha1") + bySHA, ok, err := s.GetReviewRunBySessionPRAndSHA(ctx, rec.ID, got.PRURL, "sha1") if err != nil || !ok { t.Fatalf("by sha: ok=%v err=%v", ok, err) } if bySHA.Status != domain.ReviewRunComplete || bySHA.Verdict != domain.VerdictChangesRequested || bySHA.Body != "please fix" || bySHA.GithubReviewID != "rev-987" { t.Fatalf("run result not persisted: %+v", bySHA) } - if _, ok, _ := s.GetReviewRunBySessionAndSHA(ctx, rec.ID, "other"); ok { + if _, ok, _ := s.GetReviewRunBySessionPRAndSHA(ctx, rec.ID, got.PRURL, "other"); ok { t.Fatal("unexpected run for a different sha") } @@ -140,6 +183,13 @@ func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { if len(runs) != 1 || runs[0].ID != "run-1" { t.Fatalf("list runs = %+v", runs) } + batchRuns, err := s.ListReviewRunsByBatch(ctx, rec.ID, "batch-1") + if err != nil { + t.Fatalf("list batch runs: %v", err) + } + if len(batchRuns) != 1 || batchRuns[0].ID != "run-1" || batchRuns[0].BatchID != "batch-1" { + t.Fatalf("batch runs = %+v", batchRuns) + } if ok, err := s.UpdateReviewRunResult(ctx, "run-1", domain.ReviewRunComplete, domain.VerdictApproved, "again", ""); err != nil { t.Fatalf("second update: %v", err) @@ -154,7 +204,7 @@ func TestReviewGettersMissing(t *testing.T) { if _, ok, err := s.GetReviewBySession(ctx, "mer-1"); err != nil || ok { t.Fatalf("missing review: ok=%v err=%v", ok, err) } - if _, ok, err := s.GetReviewRunBySessionAndSHA(ctx, "mer-1", "sha1"); err != nil || ok { + if _, ok, err := s.GetReviewRunBySessionPRAndSHA(ctx, "mer-1", "pr1", "sha1"); err != nil || ok { t.Fatalf("missing run: ok=%v err=%v", ok, err) } if _, ok, err := s.GetReviewRun(ctx, "run-missing"); err != nil || ok { diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index ff6d8c967..93d2a7395 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -595,7 +595,7 @@ export interface components { }; ListReviewsResponse: { reviewerHandleId: string; - reviews: components["schemas"]["ReviewRun"][]; + reviews: components["schemas"]["PRReviewState"][]; }; ListSessionPRsResponse: { prs: components["schemas"]["SessionPRSummary"][]; @@ -648,6 +648,15 @@ export interface components { projectId: string; projectName?: string; }; + PRReviewState: { + latestRun?: components["schemas"]["ReviewRun"]; + prNumber: number; + prUrl: string; + /** @enum {string} */ + status: "needs_review" | "running" | "up_to_date" | "changes_requested" | "ineligible"; + targetSha: string; + title: string; + }; Project: { agent?: string; config?: components["schemas"]["ProjectConfig"]; @@ -711,6 +720,7 @@ export interface components { sessionId: string; }; ReviewRun: { + batchId: string; body: string; /** Format: date-time */ createdAt: string; @@ -729,6 +739,7 @@ export interface components { ReviewRunResponse: { review: components["schemas"]["ReviewRun"]; reviewerHandleId: string; + reviews: components["schemas"]["ReviewRun"][]; }; RoleOverride: { agent?: string; @@ -877,14 +888,30 @@ export interface components { }; SubmitReviewInput: { /** @description Review body recorded by AO. Required for changes_requested. */ - body: string; + body?: string; /** @description Id of the GitHub PR review the reviewer posted, if any. */ - githubReviewId: string; + githubReviewId?: string; + /** @description Batched review results recorded by one reviewer CLI command. */ + reviews?: components["schemas"]["SubmitReviewItem"][]; + /** @description Review run id being completed. */ + runId?: string; + /** @description Review verdict: approved or changes_requested. */ + verdict?: string; + }; + SubmitReviewItem: { + /** @description Review body recorded by AO. Required for changes_requested. */ + body?: string; + /** @description Id of the GitHub PR review the reviewer posted, if any. */ + githubReviewId?: string; /** @description Review run id being completed. */ runId: string; /** @description Review verdict: approved or changes_requested. */ verdict: string; }; + TriggerReviewResponse: { + reviewerHandleId: string; + reviews: components["schemas"]["PRReviewState"][]; + }; WorkspaceRepo: { name: string; relativePath: string; @@ -2532,7 +2559,7 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["ReviewRunResponse"]; + "application/json": components["schemas"]["TriggerReviewResponse"]; }; }; /** @description Created */ @@ -2541,7 +2568,7 @@ export interface operations { [name: string]: unknown; }; content: { - "application/json": components["schemas"]["ReviewRunResponse"]; + "application/json": components["schemas"]["TriggerReviewResponse"]; }; }; /** @description Not Found */ diff --git a/frontend/src/renderer/components/SessionInspector.test.tsx b/frontend/src/renderer/components/SessionInspector.test.tsx index 41365e2a7..7c7eddb28 100644 --- a/frontend/src/renderer/components/SessionInspector.test.tsx +++ b/frontend/src/renderer/components/SessionInspector.test.tsx @@ -56,7 +56,7 @@ function renderWithQuery(children: ReactNode) { return render({children}); } -function mockCommonGets(reviews: unknown[] = [], reviewerHandleId = "") { +function mockCommonGets(_unusedRuns: unknown[] = [], reviewerHandleId = "", reviews: unknown[] = []) { getMock.mockImplementation(async (path: string) => { if (path === "/api/v1/sessions/{sessionId}/reviews") { return { data: { reviewerHandleId, reviews } }; @@ -94,6 +94,24 @@ const approvedReview = { createdAt: "2026-06-16T10:06:00Z", }; +const failedReview = { + ...approvedReview, + id: "run-failed", + status: "failed", + verdict: "", + body: "reviewer crashed", +}; + +const reviewState = (n: number, status: string, targetSha = `sha-${n}`) => ({ + prUrl: `https://example.com/pr/${n}`, + prNumber: n, + title: `Reviewable change ${n}`, + targetSha, + status, + latestRun: + status === "up_to_date" ? { ...approvedReview, prUrl: `https://example.com/pr/${n}`, targetSha } : undefined, +}); + beforeEach(() => { getMock.mockReset(); postMock.mockReset(); @@ -155,12 +173,13 @@ describe("SessionInspector reviews tab", () => { const openReviewsTab = async () => userEvent.click(screen.getByRole("tab", { name: /Reviews/ })); it("triggers a review and opens the returned reviewer terminal", async () => { - mockCommonGets(); + mockCommonGets([], "", [reviewState(3, "needs_review")]); + const runningReview = { ...approvedReview, status: "running", verdict: "", body: "" }; postMock.mockResolvedValue({ response: { status: 201 }, data: { reviewerHandleId: "reviewer-pane", - review: { ...approvedReview, status: "running", verdict: "", body: "" }, + reviews: [{ ...reviewState(3, "running"), latestRun: runningReview }], }, }); const onOpenReviewerTerminal = vi.fn(); @@ -180,11 +199,34 @@ describe("SessionInspector reviews tab", () => { expect(onOpenReviewerTerminal).toHaveBeenCalledWith({ handleId: "reviewer-pane", harness: "codex" }); }); - it("shows an up-to-date notice instead of opening the terminal when the backend reuses a run", async () => { - mockCommonGets([approvedReview], "reviewer-pane"); + it("shows eligible and up-to-date PR review rows", async () => { + mockCommonGets([approvedReview], "reviewer-pane", [ + reviewState(3, "needs_review", "abc123"), + reviewState(4, "up_to_date", "def456"), + ]); + + renderWithQuery(); + await openReviewsTab(); + + expect(await screen.findByText("Reviewable change 3")).toBeInTheDocument(); + expect(screen.getByText("#3")).toBeInTheDocument(); + expect(screen.getByText("Reviewable change 4")).toBeInTheDocument(); + expect(screen.getByText("#4")).toBeInTheDocument(); + expect(screen.getAllByText("Not run")).not.toHaveLength(0); + expect(screen.getAllByText("Approved")).not.toHaveLength(0); + expect(screen.getByRole("button", { name: "Re-run review" })).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Run" })).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Re-run" })).not.toBeInTheDocument(); + }); + + it("shows a no-needed-reviews notice instead of opening the terminal when the backend reuses runs", async () => { + mockCommonGets([approvedReview], "reviewer-pane", [reviewState(3, "up_to_date")]); postMock.mockResolvedValue({ response: { status: 200 }, - data: { reviewerHandleId: "reviewer-pane", review: approvedReview }, + data: { + reviewerHandleId: "reviewer-pane", + reviews: [], + }, }); const onOpenReviewerTerminal = vi.fn(); @@ -195,12 +237,15 @@ describe("SessionInspector reviews tab", () => { await userEvent.click(await screen.findByRole("button", { name: /re-run review/i })); - expect(await screen.findByText("Review is already up to date for this commit.")).toBeInTheDocument(); + expect(await screen.findByText("No needed reviews were started.")).toBeInTheDocument(); expect(onOpenReviewerTerminal).not.toHaveBeenCalled(); }); - it("shows an approved review and opens its terminal", async () => { - mockCommonGets([approvedReview], "reviewer-pane"); + it("shows one shared terminal action", async () => { + mockCommonGets([approvedReview], "reviewer-pane", [ + reviewState(3, "running", "abc123"), + reviewState(4, "up_to_date", "def456"), + ]); const onOpenReviewerTerminal = vi.fn(); renderWithQuery( @@ -208,12 +253,38 @@ describe("SessionInspector reviews tab", () => { ); await openReviewsTab(); - await waitFor(() => expect(screen.getAllByText("Approved").length).toBeGreaterThan(0)); + await waitFor(() => expect(screen.getAllByText("Open terminal")).toHaveLength(1)); + expect(screen.getAllByRole("button", { name: /review/i })).toHaveLength(1); await userEvent.click(screen.getByRole("button", { name: /open terminal/i })); expect(onOpenReviewerTerminal).toHaveBeenCalledWith({ handleId: "reviewer-pane", harness: "codex" }); }); + it("shows the reviewer identity and aggregate verdict", async () => { + mockCommonGets([approvedReview], "reviewer-pane", [reviewState(3, "changes_requested", "abc123")]); + + renderWithQuery(); + await openReviewsTab(); + + expect(await screen.findByText("codex")).toBeInTheDocument(); + expect(screen.getByText("reviewer")).toBeInTheDocument(); + expect(screen.queryByText("sess-1")).not.toBeInTheDocument(); + expect(screen.queryByText("review session")).not.toBeInTheDocument(); + expect(screen.getAllByText("Changes requested")).not.toHaveLength(0); + }); + + it("shows failed latest runs as failed and still allows rerun", async () => { + mockCommonGets([failedReview], "reviewer-pane", [ + { ...reviewState(3, "needs_review", "abc123"), latestRun: failedReview }, + ]); + + renderWithQuery(); + await openReviewsTab(); + + expect(await screen.findAllByText("Failed")).not.toHaveLength(0); + expect(screen.getByRole("button", { name: "Re-run review" })).toBeEnabled(); + }); + it("shows the no-PR empty state when the session has no PRs", async () => { mockCommonGets(); renderWithQuery(); diff --git a/frontend/src/renderer/components/SessionInspector.tsx b/frontend/src/renderer/components/SessionInspector.tsx index 83464c435..02c69f690 100644 --- a/frontend/src/renderer/components/SessionInspector.tsx +++ b/frontend/src/renderer/components/SessionInspector.tsx @@ -1,15 +1,6 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { useState, type ReactNode } from "react"; -import { - AlertCircle, - ArrowUpRight, - CheckCircle2, - CircleMinus, - GitPullRequest, - Play, - Shield, - Terminal, -} from "lucide-react"; +import { ArrowUpRight, GitPullRequest, Play, Shield, Terminal } from "lucide-react"; import type { components } from "../../api/schema"; import { apiClient, apiErrorMessage } from "../lib/api-client"; import { workspaceQueryKey } from "../hooks/useWorkspaceQuery"; @@ -26,7 +17,7 @@ import { cn } from "../lib/utils"; import { PRAttentionPanel, PRSummaryMeta } from "./PRSummaryDisplay"; type ProjectConfig = components["schemas"]["ProjectConfig"]; -type ReviewRun = components["schemas"]["ReviewRun"]; +type PRReviewState = components["schemas"]["PRReviewState"]; type ReviewsResponse = components["schemas"]["ListReviewsResponse"]; type OpenReviewerTerminal = (target: { handleId: string; harness: string }) => void; @@ -387,7 +378,8 @@ function ReviewsView({ enabled: hasPr, refetchInterval: (query) => { const data = query.state.data as ReviewsResponse | undefined; - return data?.reviews.some((review) => review.status === "running") ? 2500 : false; + const reviews = data?.reviews ?? []; + return reviews.some((review) => review.status === "running") ? 2500 : false; }, queryFn: async () => { const { data, error } = await apiClient.GET("/api/v1/sessions/{sessionId}/reviews", { @@ -422,16 +414,18 @@ function ReviewsView({ onSuccess: ({ data, reused }) => { void queryClient.invalidateQueries({ queryKey: ["session-reviews", session.id] }); void queryClient.invalidateQueries({ queryKey: workspaceQueryKey }); - if (reused) { - setReviewNotice("Review is already up to date for this commit."); + const started = data?.reviews?.find((review) => review.status === "running" && review.latestRun); + if (reused || !started?.latestRun) { + setReviewNotice("No needed reviews were started."); return; } if (data?.reviewerHandleId) { - onOpenReviewerTerminal?.({ handleId: data.reviewerHandleId, harness: data.review.harness || "reviewer" }); + const harness = started.latestRun.harness || "reviewer"; + onOpenReviewerTerminal?.({ handleId: data.reviewerHandleId, harness }); } }, }); - const reviews = reviewsQuery.data?.reviews ?? []; + const reviewStates = reviewsQuery.data?.reviews ?? []; return (
@@ -444,7 +438,7 @@ function ReviewsView({ onOpenTerminal={onOpenReviewerTerminal} onTrigger={() => triggerReview.mutate()} reviewerHandleId={reviewsQuery.data?.reviewerHandleId ?? ""} - reviews={reviews} + reviewStates={reviewStates} notice={reviewNotice} session={session} /> @@ -461,7 +455,7 @@ function projectConfig(project: components["schemas"]["ProjectOrDegraded"] | und function ReviewPanel({ session, config, - reviews, + reviewStates, reviewerHandleId, isLoading, isTriggering, @@ -472,7 +466,7 @@ function ReviewPanel({ }: { session: WorkspaceSession; config?: ProjectConfig; - reviews: ReviewRun[]; + reviewStates: PRReviewState[]; reviewerHandleId: string; isLoading: boolean; isTriggering: boolean; @@ -488,108 +482,142 @@ function ReviewPanel({ return

Loading reviews...

; } - const latest = latestReview(reviews); + const latest = reviewStates.find((review) => review.latestRun)?.latestRun; const harness = latest?.harness || config?.reviewers?.[0]?.harness || session.provider || "reviewer"; + const terminalEnabled = Boolean(reviewerHandleId && onOpenTerminal); + const aggregateVerdict = sessionReviewVerdict(reviewStates); + const runAction = reviewSessionRunAction(reviewStates, isTriggering); + const runDisabled = + isTriggering || + reviewStates.length === 0 || + reviewStates.some((reviewState) => reviewState.status === "running") || + reviewStates.every((reviewState) => reviewState.status === "ineligible"); return (
{error ?

{apiErrorMessage(error, "Review request failed")}

: null} {notice ?

{notice}

: null} - -
- ); -} - -function latestReview(reviews: ReviewRun[]): ReviewRun | undefined { - return [...reviews].sort((a, b) => Date.parse(b.createdAt) - Date.parse(a.createdAt))[0]; -} - -function ReviewerCard({ - harness, - review, - handleId, - isTriggering, - onTrigger, - onOpenTerminal, -}: { - harness: string; - review?: ReviewRun; - handleId: string; - isTriggering: boolean; - onTrigger: () => void; - onOpenTerminal?: OpenReviewerTerminal; -}) { - const status = reviewStatus(review); - const terminalEnabled = Boolean(handleId && onOpenTerminal); - const runLabel = review ? "Re-run review" : "Run review"; - - return ( -
-
-
-
- - {status.icon} - {status.label} - +
+
-
- - {review ? ( +
+
+ Pull requests + + {aggregateVerdict.label} + +
+
+ {reviewStates.length === 0 ?

No review state loaded yet.

: null} + {reviewStates.map((reviewState) => ( + + ))} +
+
+ - ) : null} +
); } -function reviewStatus(review?: ReviewRun): { +function ReviewStateRow({ reviewState }: { reviewState: PRReviewState }) { + const verdict = reviewVerdict(reviewState); + const title = reviewState.title?.trim() || `PR #${reviewState.prNumber}`; + return ( +
+
+ +
+
+
+ {verdict.label} +
+ ); +} + +function sessionReviewVerdict(reviewStates: PRReviewState[]): { label: string; tone: "neutral" | "running" | "success" | "danger"; - icon: ReactNode; } { - if (!review) return { label: "Not run", tone: "neutral", icon: null }; - if (review.status === "running") { - return { label: "Running", tone: "running", icon:
); } @@ -316,7 +327,7 @@ function BoardPRSummary({ className, pr }: { className?: string; pr: SessionPRSu {diffSummary ? {diffSummary} : null} - +
); } diff --git a/frontend/src/renderer/lib/pr-display.test.ts b/frontend/src/renderer/lib/pr-display.test.ts index fd50f591a..3a904f5ec 100644 --- a/frontend/src/renderer/lib/pr-display.test.ts +++ b/frontend/src/renderer/lib/pr-display.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import type { SessionPRSummary } from "../hooks/useSessionScmSummary"; -import { prAttentionItems, prDiffSummary, prStatusRows } from "./pr-display"; +import { prAttentionItems, prBrowserUrl, prDiffSummary, prStatusRows } from "./pr-display"; const summary = (overrides: Partial = {}): SessionPRSummary => ({ url: "https://github.com/acme/repo/pull/7", @@ -56,6 +56,19 @@ describe("prDiffSummary", () => { }); }); +describe("prBrowserUrl", () => { + it("normalizes issue-shaped GitHub PR URLs to the pull request page", () => { + expect( + prBrowserUrl( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + }), + ), + ).toBe("https://github.com/acme/repo/pull/7"); + }); +}); + describe("prAttentionItems", () => { it("returns no attention for clean open PRs", () => { expect(prAttentionItems(summary())).toEqual([]); @@ -100,6 +113,150 @@ describe("prAttentionItems", () => { }); }); + it("links failing CI checks to their provider URLs", () => { + const items = prAttentionItems( + summary({ + ci: { + state: "failing", + failingChecks: [ + { name: "unit", status: "failed", conclusion: "failure", url: "https://checks.example/unit" }, + { name: "lint", status: "failed", conclusion: "failure", url: "https://checks.example/lint" }, + { name: "build", status: "failed", conclusion: "failure", url: "https://checks.example/build" }, + { name: "types", status: "failed", conclusion: "failure", url: "https://checks.example/types" }, + ], + }, + }), + ); + + const ciItem = items.find((item) => item.kind === "ci_failing"); + expect(ciItem?.links).toEqual([ + { label: "unit", href: "https://checks.example/unit", title: "failure" }, + { label: "lint", href: "https://checks.example/lint", title: "failure" }, + { label: "build", href: "https://checks.example/build", title: "failure" }, + ]); + expect(ciItem?.overflowLabel).toBe("+1 check"); + }); + + it("prefers the submitted review summary over inline comments", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [ + { + reviewerId: "alice", + count: 2, + reviewUrl: "https://github.com/acme/repo/pull/7#pullrequestreview-1", + links: [ + { url: "https://github.com/acme/repo/pull/7#discussion_r1", file: "main.go", line: 12 }, + { url: "https://github.com/acme/repo/pull/7#discussion_r2", file: "test.go", line: 20 }, + ], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice +1", + href: "https://github.com/acme/repo/pull/7#pullrequestreview-1", + title: "Open requested-changes review from alice", + }); + }); + + it("falls back to the first inline comment when no review summary exists", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [ + { + reviewerId: "alice", + count: 2, + links: [ + { url: "https://github.com/acme/repo/pull/7#discussion_r1", file: "main.go", line: 12 }, + { url: "https://github.com/acme/repo/pull/7#discussion_r2", file: "test.go", line: 20 }, + ], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice +1", + href: "https://github.com/acme/repo/pull/7#discussion_r1", + title: "2 unresolved comments from alice", + }); + }); + + it("falls back to the PR page when review summary and inline comment URLs are missing", () => { + const items = prAttentionItems( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [{ reviewerId: "alice", count: 1, links: [] }], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice", + href: "https://github.com/acme/repo/pull/7", + title: "Open pull request for alice", + }); + }); + + it("shows bot reviewers with a bot label", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: false, + unresolvedBy: [ + { + reviewerId: "copilot", + count: 0, + reviewUrl: "https://github.com/acme/repo/pull/7#pullrequestreview-2", + isBot: true, + links: [], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "copilot bot", + href: "https://github.com/acme/repo/pull/7#pullrequestreview-2", + title: "Open requested-changes review from copilot bot", + }); + }); + + it("links merge conflicts to GitHub's conflict resolution page", () => { + const items = prAttentionItems( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + mergeability: { + state: "conflicting", + reasons: [], + prUrl: "https://github.com/acme/repo/issues/7", + }, + }), + ); + + expect(items.find((item) => item.kind === "merge_conflict")?.links[0]).toMatchObject({ + label: "conflicts", + href: "https://github.com/acme/repo/pull/7/conflicts", + }); + }); + it("suppresses attention once the PR is closed or merged", () => { expect( prAttentionItems( diff --git a/frontend/src/renderer/lib/pr-display.ts b/frontend/src/renderer/lib/pr-display.ts index b533c0d46..7ce2c3f45 100644 --- a/frontend/src/renderer/lib/pr-display.ts +++ b/frontend/src/renderer/lib/pr-display.ts @@ -46,6 +46,10 @@ export function comparePRDisplaySummaries(a: SessionPRSummary, b: SessionPRSumma return prStateRank[a.state] - prStateRank[b.state] || a.number - b.number; } +export function prBrowserUrl(pr: SessionPRSummary): string { + return prBaseUrl(pr) ?? pr.htmlUrl ?? pr.url; +} + export function sessionPRDisplaySummaries( session: WorkspaceSession, summaries: SessionPRSummary[] = [], @@ -175,11 +179,10 @@ export function prAttentionItems(pr: SessionPRSummary): PRAttentionItem[] { } if (pr.review.decision === "changes_requested" || pr.review.hasUnresolvedHumanComments) { const reviewers = pr.review.unresolvedBy.slice(0, 3); - const links = reviewers.map((reviewer) => ({ - label: reviewerLabel(reviewer), - href: reviewer.links.find((link) => link.url)?.url || undefined, - title: `${reviewer.count} unresolved ${pluralize("comment", reviewer.count)}`, - })); + const links = reviewers.map((reviewer) => reviewAttentionLink(pr, reviewer)); + if (links.length === 0 && pr.review.decision === "changes_requested") { + links.push({ label: "PR", href: prBrowserUrl(pr), title: "Open pull request" }); + } items.push({ kind: "review_changes_requested", title: "Address requested changes", @@ -331,18 +334,23 @@ function mergeAttention( fallback: string, tone: PRDisplayTone, ): PRAttentionItem { + const href = + kind === "merge_conflict" ? mergeConflictUrl(pr) : pr.mergeability.prUrl || pr.htmlUrl || pr.url || undefined; const fileLinks = (pr.mergeability.conflictFiles ?? []).slice(0, 3).map((file) => ({ label: file.path, - href: file.url || pr.mergeability.prUrl || undefined, + href: file.url || href, + title: kind === "merge_conflict" ? "Open merge conflicts" : undefined, })); const reasonLinks = - fileLinks.length > 0 + fileLinks.length > 0 || kind === "merge_conflict" ? [] : pr.mergeability.reasons.slice(0, 3).map((reason) => ({ label: mergeReasonLabel(reason), - href: pr.mergeability.prUrl || undefined, + href, })); - const links = fileLinks.length > 0 ? fileLinks : reasonLinks; + const fallbackLink = + kind === "merge_conflict" && href ? [{ label: "conflicts", href, title: "Open merge conflicts" }] : []; + const links = fileLinks.length > 0 ? fileLinks : reasonLinks.length > 0 ? reasonLinks : fallbackLink; return { kind, title, @@ -356,11 +364,78 @@ function mergeAttention( }; } -function reviewerLabel(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { - if (reviewer.count <= 1) { - return reviewer.reviewerId; +function mergeConflictUrl(pr: SessionPRSummary): string | undefined { + return prSubpageUrl(pr, "conflicts") ?? pr.mergeability.prUrl ?? prBrowserUrl(pr); +} + +function prBaseUrl(pr: SessionPRSummary): string | undefined { + return prURL(pr); +} + +function prSubpageUrl(pr: SessionPRSummary, subpage: "conflicts"): string | undefined { + const base = prURL(pr); + return base ? `${base}/${subpage}` : undefined; +} + +function prURL(pr: SessionPRSummary): string | undefined { + const raw = pr.htmlUrl || pr.mergeability.prUrl || pr.url; + if (!raw) { + return undefined; } - return `${reviewer.reviewerId} +${reviewer.count - 1}`; + try { + const url = new URL(raw); + const match = url.pathname.match(/^(\/[^/]+\/[^/]+)\/(?:pull|issues)\/(\d+)(?:\/.*)?$/); + if (!match) { + return undefined; + } + url.pathname = `${match[1]}/pull/${match[2]}`; + url.search = ""; + url.hash = ""; + return url.toString(); + } catch { + return undefined; + } +} + +function reviewerLabel(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { + const name = reviewerDisplayName(reviewer); + if (reviewer.count <= 1) { + return name; + } + return `${name} +${reviewer.count - 1}`; +} + +function reviewerDisplayName(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { + return reviewer.isBot ? `${reviewer.reviewerId} bot` : reviewer.reviewerId; +} + +function reviewAttentionLink( + pr: SessionPRSummary, + reviewer: SessionPRSummary["review"]["unresolvedBy"][number], +): PRAttentionLink { + const inlineURL = reviewer.links.find((link) => link.url)?.url; + if (reviewer.reviewUrl) { + return { + label: reviewerLabel(reviewer), + href: reviewer.reviewUrl, + title: `Open requested-changes review from ${reviewerDisplayName(reviewer)}`, + }; + } + if (inlineURL) { + return { + label: reviewerLabel(reviewer), + href: inlineURL, + title: + reviewer.count > 0 + ? `${reviewer.count} unresolved ${pluralize("comment", reviewer.count)} from ${reviewerDisplayName(reviewer)}` + : `Open review comments from ${reviewerDisplayName(reviewer)}`, + }; + } + return { + label: reviewerLabel(reviewer), + href: prBrowserUrl(pr), + title: `Open pull request for ${reviewerDisplayName(reviewer)}`, + }; } function mergeReasonLabel(reason: string): string { From 2c4bf55e9515ba05f97b9a6c23237d285b2541d2 Mon Sep 17 00:00:00 2001 From: swyam sharma <84120511+areycruzer@users.noreply.github.com> Date: Tue, 30 Jun 2026 00:14:46 +0530 Subject: [PATCH 12/13] fix(spawn): fail fast when tmux is missing (#2259) Co-authored-by: Swyam Sharma --- backend/internal/cli/doctor.go | 2 +- backend/internal/cli/doctor_test.go | 20 +++++++-- backend/internal/ports/outbound.go | 3 ++ backend/internal/service/session/service.go | 2 + .../internal/service/session/service_test.go | 1 + backend/internal/session_manager/manager.go | 26 ++++++++--- .../internal/session_manager/manager_test.go | 43 +++++++++++++++++-- 7 files changed, 82 insertions(+), 15 deletions(-) diff --git a/backend/internal/cli/doctor.go b/backend/internal/cli/doctor.go index 96492e32a..248730d5e 100644 --- a/backend/internal/cli/doctor.go +++ b/backend/internal/cli/doctor.go @@ -307,7 +307,7 @@ func (c *commandContext) checkTerminalRuntime(ctx context.Context) doctorCheck { func (c *commandContext) checkTmux(ctx context.Context) doctorCheck { path, err := c.deps.LookPath("tmux") if err != nil || path == "" { - return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "tmux", Message: "not found in PATH"} + return doctorCheck{Level: doctorWarn, Section: doctorSectionTools, Name: "tmux", Message: "not found in PATH; required on macOS/Linux to start sessions"} } reqCtx, cancel := context.WithTimeout(ctx, probeTimeout) defer cancel() diff --git a/backend/internal/cli/doctor_test.go b/backend/internal/cli/doctor_test.go index daf7c9499..7686dfb76 100644 --- a/backend/internal/cli/doctor_test.go +++ b/backend/internal/cli/doctor_test.go @@ -247,12 +247,18 @@ func TestDoctorJSONOutputIsDecodable(t *testing.T) { clearDoctorGitHubEnv(t) out, errOut, err := executeCLI(t, Deps{ LookPath: func(name string) (string, error) { - if name == "git" { + switch name { + case "git": return "/bin/git", nil + case "tmux": + return "/bin/tmux", nil } return "", errors.New("missing") }, - CommandOutput: func(context.Context, string, ...string) ([]byte, error) { + CommandOutput: func(_ context.Context, name string, _ ...string) ([]byte, error) { + if name == "/bin/tmux" { + return []byte("tmux 3.3a\n"), nil + } return []byte("git version 2.43.0\n"), nil }, ProcessAlive: func(int) bool { return false }, @@ -277,12 +283,18 @@ func TestDoctorTextOutputIsGrouped(t *testing.T) { clearDoctorGitHubEnv(t) out, errOut, err := executeCLI(t, Deps{ LookPath: func(name string) (string, error) { - if name == "git" { + switch name { + case "git": return "/bin/git", nil + case "tmux": + return "/bin/tmux", nil } return "", errors.New("missing") }, - CommandOutput: func(context.Context, string, ...string) ([]byte, error) { + CommandOutput: func(_ context.Context, name string, _ ...string) ([]byte, error) { + if name == "/bin/tmux" { + return []byte("tmux 3.3a\n"), nil + } return []byte("git version 2.43.0\n"), nil }, ProcessAlive: func(int) bool { return false }, diff --git a/backend/internal/ports/outbound.go b/backend/internal/ports/outbound.go index 359cd5f8c..f88e1b783 100644 --- a/backend/internal/ports/outbound.go +++ b/backend/internal/ports/outbound.go @@ -164,6 +164,9 @@ var ( // conflict markers for manual resolution. Adapters wrap this sentinel via // fmt.Errorf so callers can match it with errors.Is. ErrPreservedConflict = errors.New("workspace: preserved apply produced conflicts") + // ErrRuntimePrerequisite reports a missing host prerequisite for the selected + // runtime before a session can be created. + ErrRuntimePrerequisite = errors.New("runtime: prerequisite missing") ) // WorkspaceConfig is the spec for creating or restoring a session's workspace. diff --git a/backend/internal/service/session/service.go b/backend/internal/service/session/service.go index 740c9e43c..32d338897 100644 --- a/backend/internal/service/session/service.go +++ b/backend/internal/service/session/service.go @@ -481,6 +481,8 @@ func toAPIError(err error) error { return apierr.Invalid("INVALID_BRANCH", err.Error(), nil) case errors.Is(err, ports.ErrAgentBinaryNotFound): return apierr.Invalid("AGENT_BINARY_NOT_FOUND", err.Error(), nil) + case errors.Is(err, ports.ErrRuntimePrerequisite): + return apierr.Invalid("RUNTIME_PREREQUISITE_MISSING", err.Error(), nil) default: return err } diff --git a/backend/internal/service/session/service_test.go b/backend/internal/service/session/service_test.go index 405e17964..c5b1b1ca6 100644 --- a/backend/internal/service/session/service_test.go +++ b/backend/internal/service/session/service_test.go @@ -534,6 +534,7 @@ func TestToAPIErrorMapsWorkspaceBranchSentinels(t *testing.T) { {"not fetched", fmt.Errorf("spawn mer-1: workspace: %w: \"x\" has no local head", ports.ErrWorkspaceBranchNotFetched), apierr.KindInvalid, "BRANCH_NOT_FETCHED"}, {"invalid branch", fmt.Errorf("spawn mer-1: workspace: %w: \"bad!!\" (exit 1)", ports.ErrWorkspaceBranchInvalid), apierr.KindInvalid, "INVALID_BRANCH"}, {"agent binary not found", fmt.Errorf("spawn mer-1: %w", ports.ErrAgentBinaryNotFound), apierr.KindInvalid, "AGENT_BINARY_NOT_FOUND"}, + {"runtime prerequisite missing", fmt.Errorf("spawn: %w: tmux required on macOS/Linux but not in PATH", ports.ErrRuntimePrerequisite), apierr.KindInvalid, "RUNTIME_PREREQUISITE_MISSING"}, {"unknown harness", fmt.Errorf("spawn: %w: %q", sessionmanager.ErrUnknownHarness, "bogus"), apierr.KindInvalid, "UNKNOWN_HARNESS"}, {"missing harness", fmt.Errorf("spawn: %w: configure project worker.agent or pass --harness", sessionmanager.ErrMissingHarness), apierr.KindInvalid, "AGENT_REQUIRED"}, } diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 48d93fe26..a4efb7eb3 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -202,6 +202,10 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess return domain.SessionRecord{}, fmt.Errorf("spawn: %w: %q", ErrUnknownHarness, cfg.Harness) } + if err := m.validateRuntimePrerequisites(); err != nil { + return domain.SessionRecord{}, fmt.Errorf("spawn: %w", err) + } + prompt, systemPrompt, err := m.buildSpawnTexts(ctx, cfg) if err != nil { return domain.SessionRecord{}, fmt.Errorf("spawn: prompt: %w", err) @@ -237,19 +241,19 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess // post-create commands (e.g. `pnpm install`) before the agent launches. if err := m.provisionWorkspace(ctx, project, ws.Path); err != nil { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: provision: %w", id, err) } agent, ok := m.agents.Agent(cfg.Harness) if !ok { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: no agent adapter for harness %q", id, cfg.Harness) } if err := m.prepareWorkspace(ctx, agent, id, ws.Path); err != nil { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: %w", id, err) } agentConfig := effectiveAgentConfig(cfg.Kind, project.Config) @@ -264,7 +268,7 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess }) if err != nil { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: launch command: %w", id, err) } // Pre-flight: confirm argv[0] actually exists on PATH (or as an absolute @@ -273,7 +277,7 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess // unresolved binary would leak through as a "live" session that never ran. if err := m.validateAgentBinary(argv); err != nil { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: %w", id, err) } handle, err := m.runtime.Create(ctx, ports.RuntimeConfig{ @@ -284,7 +288,7 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess }) if err != nil { _ = m.workspace.Destroy(ctx, ws) - m.markSpawnFailedTerminated(ctx, id) + m.rollbackSpawnSeedRow(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: runtime: %w", id, err) } @@ -1281,6 +1285,16 @@ func (m *Manager) validateAgentBinary(argv []string) error { return nil } +func (m *Manager) validateRuntimePrerequisites() error { + if runtime.GOOS == "windows" { + return nil + } + if path, err := m.lookPath("tmux"); err != nil || path == "" { + return fmt.Errorf("%w: tmux required on macOS/Linux but not in PATH", ports.ErrRuntimePrerequisite) + } + return nil +} + func runtimeHandle(meta domain.SessionMetadata) ports.RuntimeHandle { return ports.RuntimeHandle{ID: meta.RuntimeHandleID} } diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index 82ff2c491..d12943c37 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -8,6 +8,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime" "strings" "testing" "time" @@ -465,8 +466,8 @@ func TestSpawn_RollsBackOnRuntimeFailure(t *testing.T) { if ws.destroyed != 1 { t.Fatal("workspace should roll back") } - if !st.sessions["mer-1"].IsTerminated { - t.Fatal("orphaned spawn should be terminated") + if rec, present := st.sessions["mer-1"]; present { + t.Fatalf("seed row must be deleted before a runtime handle is live, got %+v", rec) } } @@ -1098,6 +1099,9 @@ func TestSpawn_RejectsMissingAgentBinary(t *testing.T) { rt := &fakeRuntime{} ws := &fakeWorkspace{} notFound := func(name string) (string, error) { + if name == "tmux" { + return "/bin/tmux", nil + } return "", fmt.Errorf("exec: %q: not found", name) } m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: &fakeLCM{store: st}, LookPath: notFound}) @@ -1112,8 +1116,39 @@ func TestSpawn_RejectsMissingAgentBinary(t *testing.T) { if ws.destroyed != 1 { t.Fatal("workspace must be torn down when the pre-launch binary check fails") } - if !st.sessions["mer-1"].IsTerminated { - t.Fatal("the orphan row should be marked terminated after the failed spawn") + if rec, present := st.sessions["mer-1"]; present { + t.Fatalf("seed row must be deleted before a runtime handle is live, got %+v", rec) + } +} + +func TestSpawn_RejectsMissingTmuxBeforeSessionRow(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows uses ConPTY, not tmux") + } + st := newFakeStore() + st.projects["mer"] = domain.ProjectRecord{ID: "mer", Config: testRoleAgents()} + rt := &fakeRuntime{} + ws := &fakeWorkspace{} + lookPath := func(name string) (string, error) { + if name == "tmux" { + return "", fmt.Errorf("exec: %q: not found", name) + } + return "/bin/true", nil + } + m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: &fakeLCM{store: st}, LookPath: lookPath}) + + _, err := m.Spawn(ctx, ports.SpawnConfig{ProjectID: "mer", Kind: domain.KindWorker}) + if !errors.Is(err, ports.ErrRuntimePrerequisite) || !strings.Contains(err.Error(), "tmux required") { + t.Fatalf("err = %v, want missing tmux prerequisite", err) + } + if len(st.sessions) != 0 { + t.Fatalf("no session row should be created before runtime prerequisites pass, got %d", len(st.sessions)) + } + if ws.lastCfg.SessionID != "" || ws.destroyed != 0 { + t.Fatal("workspace must not be created when tmux is missing") + } + if rt.created != 0 { + t.Fatal("runtime must not be created when tmux is missing") } } From de9e90df45fa3522ab9d4e542392199cdda9f7b4 Mon Sep 17 00:00:00 2001 From: Dushyant Singh Hada Date: Tue, 30 Jun 2026 11:23:54 +0530 Subject: [PATCH 13/13] fix: resolve duplicate migration version 20 collision (#2294) Two PRs (#2193, #2200) merged ~2h apart each added a migration file numbered 0020, since each branch only saw one such file at CI time and neither was rebased against the other before merge. Renumbers 0020_pr_reviews.sql to 0021_pr_reviews.sql. TestMigrationVersionsAreUnique already existed and is correct; it didn't catch this because it only runs against each PR's branch state, not the merged result of both PRs together. --- .../migrations/{0020_pr_reviews.sql => 0021_pr_reviews.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename backend/internal/storage/sqlite/migrations/{0020_pr_reviews.sql => 0021_pr_reviews.sql} (100%) diff --git a/backend/internal/storage/sqlite/migrations/0020_pr_reviews.sql b/backend/internal/storage/sqlite/migrations/0021_pr_reviews.sql similarity index 100% rename from backend/internal/storage/sqlite/migrations/0020_pr_reviews.sql rename to backend/internal/storage/sqlite/migrations/0021_pr_reviews.sql