From a0601bd7160d85652b2d047e35d35e0b237f6785 Mon Sep 17 00:00:00 2001 From: Hoang Snowy Date: Thu, 22 Oct 2026 23:00:08 +0700 Subject: [PATCH] feat(review): a Reviewer agent reviews every change before it is tested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent for real on the Coding → Testing hand-off: after a proposed change's checks go green, the Reviewer agent (read-only, on the member's computer, scoped autonomy) reads the diff in context against the ticket and the repo's own rules — correctness, security / tenant isolation, tests, conventions — and grades findings must-fix / should-fix / nit. It posts the review on the pull request; must-fix findings become the change request the Coding agent gets next (the same 3 bounded rounds as failing checks and failed acceptance), then the change is reviewed again. An approved change goes on to the Testing agent and Autopilot's merge policy. - Domain: IReviewAgent, ReviewVerdict / ReviewFinding (an "approve" with a must-fix finding is not trusted). - Pipeline: ReviewAgent (prompt, JSON verdict, PR comment, change request). - Web: BoardTaskService.ReviewAsync; repo states Reviewing / Reviewed / ChangesRequested (session counts them as Done); AutopilotPlanner RunReview + ChangeReview; orchestrator maps states, recovers a review cut short by a restart; plain-language statuses on the Board. - Docs: autonomy-levels phase table. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018WAdQ7Rm24xss5QDSvXtJB --- docs/plans/autonomy-levels.md | 4 +- src/AgentOs.Domain/Sessions/IReviewAgent.cs | 30 +++ .../Agents/DependencyInjection.cs | 1 + .../Agents/ReviewAgent.cs | 177 ++++++++++++++++++ .../Repositories/SessionRepository.cs | 3 +- .../Components/Pages/BoardApp.razor | 3 + .../Services/BoardTasks/BoardTaskService.cs | 82 +++++++- .../Orchestration/AutopilotPlanner.cs | 38 +++- .../Orchestration/ProjectOrchestrator.cs | 24 ++- tests/AgentOs.Tests/Web/ReviewStepTests.cs | 109 +++++++++++ 10 files changed, 461 insertions(+), 10 deletions(-) create mode 100644 src/AgentOs.Domain/Sessions/IReviewAgent.cs create mode 100644 src/AgentOs.Modules.Pipeline/Agents/ReviewAgent.cs create mode 100644 tests/AgentOs.Tests/Web/ReviewStepTests.cs diff --git a/docs/plans/autonomy-levels.md b/docs/plans/autonomy-levels.md index ea20372..bd800e7 100644 --- a/docs/plans/autonomy-levels.md +++ b/docs/plans/autonomy-levels.md @@ -13,7 +13,7 @@ Every ticket flows through three phases (owner, 2026-10-21); the knowledge graph |---|---|---|---| | **1. Agentic Discovery** | idea → research → spec → tickets with acceptance criteria and dependencies | New idea → spec → tickets, dependencies (#240) | a research agent (market / UX / technical), reading the customer's own documents, asking back when the idea is ambiguous | | **2. Agentic Coding** | ticket → code → pull request; continues from review and CI | on a computer or the servers, scoped, Stop, live steps, Ask for changes, Fix the failing checks | autopilot over a board in dependency order | -| **3. Agentic Testing** | accepts a PR against its criteria: unit tests, Playwright on a real stack, data checks, performance → evidence → merge, or back to Coding with findings | done by hand on the lab | the acceptance agent + merge policy | +| **3. Agentic Review + Testing** | the **Reviewer agent** reads the diff against the ticket and the repo's rules (correctness, security/isolation, tests) → approve, or must-fix findings back to Coding; then the **Testing agent** accepts it against its criteria: unit tests, Playwright on a real stack, data checks → evidence → merge, or back to Coding | Reviewer (#264) + Testing (#244) + Autopilot merge policy, both posting evidence on the PR | review of a cloud worker's change; a person's review folded in | **Knowledge graph** (ADR-0008, `AgentOs.Modules.Memory`): Discovery records decisions and requirements, Coding records changed files, PRs and review lessons, Testing records failures, fixes and evidence — and each phase reads what the @@ -40,6 +40,8 @@ others learned. The board shows each ticket's phase (Discovery → Coding → Te `docs/ACCEPTANCE.md`, run the unit tests, bring the stack up and run the Playwright specs, run data checks (e.g. the CBL corpus), and post the evidence on the PR. Pass → approve; fail → *Ask for changes* with the findings. +3b. ✅ (#264) **Reviewer agent.** Between green CI and acceptance testing: a read-only agent reviews the diff in + context and posts its review on the PR; must-fix findings go back to the Coding agent (same 3 bounded rounds). 4. ✅ (#242, *Build and merge*: on green CI; acceptance agent next) **Merge policy.** Per project: merge on green + acceptance pass (squash, delete branch), or wait for a person. 5. **Environment care.** The known stack landmines (stale Keycloak/Postgres volumes, orphaned containers, ports) handled by the acceptance agent's stack step, not by a person. diff --git a/src/AgentOs.Domain/Sessions/IReviewAgent.cs b/src/AgentOs.Domain/Sessions/IReviewAgent.cs new file mode 100644 index 0000000..d34abe7 --- /dev/null +++ b/src/AgentOs.Domain/Sessions/IReviewAgent.cs @@ -0,0 +1,30 @@ +// Agentic review: before a proposed change is tested, a second agent reads its diff against the ticket and either +// approves it or asks the Coding agent for changes — the code review a person would otherwise do on the PR. + +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace AgentOs.Domain.Sessions; + +/// One review finding. is must-fix (blocks the change), should-fix or nit. +public sealed record ReviewFinding(string Severity, string File, int? Line, string Comment) +{ + /// True for a finding that blocks the change. + public bool Blocks => Severity == "must-fix"; +} + +/// The Reviewer agent's verdict. is false when it could not review at all (no computer, …). +public sealed record ReviewVerdict(bool Ran, bool Approved, IReadOnlyList Findings, string Summary, string? Error) +{ + /// The findings that block the change. + public IEnumerable Blocking => Findings.Where(f => f.Blocks); +} + +/// Reviews a proposed change on a paired computer, read-only: it never edits, commits or pushes. +public interface IReviewAgent +{ + /// Review the change on request.Repo.ExistingBranch against its ticket. + Task RunAsync(AcceptanceRequest request, CancellationToken ct = default); +} diff --git a/src/AgentOs.Modules.Pipeline/Agents/DependencyInjection.cs b/src/AgentOs.Modules.Pipeline/Agents/DependencyInjection.cs index 3dbce0f..8f02ad5 100644 --- a/src/AgentOs.Modules.Pipeline/Agents/DependencyInjection.cs +++ b/src/AgentOs.Modules.Pipeline/Agents/DependencyInjection.cs @@ -64,6 +64,7 @@ public static IServiceCollection AddAgents(this IServiceCollection services, ICo services.AddTransient(); services.AddTransient(); services.AddTransient(); + services.AddTransient(); services.AddTransient(); services.AddTransient(); services.AddTransient(); diff --git a/src/AgentOs.Modules.Pipeline/Agents/ReviewAgent.cs b/src/AgentOs.Modules.Pipeline/Agents/ReviewAgent.cs new file mode 100644 index 0000000..ac49cb5 --- /dev/null +++ b/src/AgentOs.Modules.Pipeline/Agents/ReviewAgent.cs @@ -0,0 +1,177 @@ +// Agentic review — the Reviewer agent. Between the Coding agent and the Testing agent: it reads the proposed change's +// diff against its ticket and the repository's own rules, the way a careful reviewer would, and either approves it or +// returns must-fix findings that go back to the Coding agent. It runs on the member's paired computer under scoped +// autonomy (it needs the checkout to read code in context and may build) and never edits, commits or pushes. + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using AgentOs.Domain.Llm; +using AgentOs.Domain.Sessions; +using Microsoft.Extensions.Logging; + +namespace AgentOs.Modules.Pipeline.Agents; + +/// +public sealed class ReviewAgent(ILlmClientFactory factory, ILogger logger, ISessionRunFeed? feed = null) : IReviewAgent +{ + // Reading a diff in context is quicker than testing it; a build fits too. + private static readonly TimeSpan RunTimeout = TimeSpan.FromMinutes(20); + + /// + public async Task RunAsync(AcceptanceRequest request, CancellationToken ct = default) + { + ArgumentNullException.ThrowIfNull(request); + if (string.IsNullOrWhiteSpace(request.Repo.ExistingBranch)) + { + return NotRun("There is no proposed change (branch) to review."); + } + var repoLabel = $"{request.Repo.Owner}/{request.Repo.Repo}"; + Emit(request, SessionRunEventKind.Running, $"Reviewing #{request.IssueNumber} on your computer.", repoLabel); + + ILlmClient llm; + try { llm = factory.Create("RemoteAgent"); } + catch (Exception ex) when (ex is LlmException or ArgumentException or NotSupportedException or InvalidOperationException) + { + return NotRun($"No computer to review on: {ex.Message}"); + } + + var llmRequest = new LlmRequest( + SystemPrompt: Instructions(request) + "\n\n" + UntrustedText.SystemRule, + UserPrompt: AcceptanceAgent.User(request).Replace("Test the change against these criteria and reply with the JSON verdict.", + "Review the change against this ticket and reply with the JSON verdict.", StringComparison.Ordinal), + Model: "claude-sonnet-4-6", + MaxTokens: 32768, + Timeout: RunTimeout, + Cli: request.CliProfile) + { + Autonomy = WorkspaceAutonomy.Scoped, + Progress = new InlineProgress(step => + { + if (TaskSteps.LabelFor(step) is { } label) { Emit(request, SessionRunEventKind.Step, "Reviewing — " + label, repoLabel); } + }), + }; + try + { + var response = await llm.SendAsync(llmRequest, ct).ConfigureAwait(false); + return Parse(response.Content ?? string.Empty); + } + catch (Exception ex) when (ex is LlmException or TimeoutException or InvalidOperationException or System.Net.Http.HttpRequestException) + { + logger.LogWarning(ex, "Review run failed for session {SessionId}", request.SessionId); + return NotRun(ex.Message); + } + } + + private static ReviewVerdict NotRun(string why) => new(false, false, [], why, why); + + private void Emit(AcceptanceRequest r, SessionRunEventKind kind, string message, string repo) => + feed?.Publish(new SessionRunEvent(r.TenantId, r.SessionId, kind, message, DateTimeOffset.UtcNow, repo)); + + internal static string Instructions(AcceptanceRequest r) => string.Format( + CultureInfo.InvariantCulture, + """ + You are the Reviewer Agent of AgentOS.NET. You review a proposed change before it is tested, the way a careful + senior reviewer would on its pull request, on the developer's own machine with your native shell and file tools. + You NEVER change the code: do not edit tracked files, do not commit, do not push, do not open or comment on pull + requests. + + 1. Get the change. The runner starts you in a fresh, empty directory made for this run: + git clone https://github.com/{0}/{1}.git . + git fetch origin {2} + git checkout {2} + git diff origin/{3}...HEAD + 2. Read the ticket (user message) and the repository's rules (AGENTS.md, CLAUDE.md, README, docs it points to). + 3. Review the diff in context — open the files around each change. Look for, in this order: + - correctness: does it do what the ticket asks; broken edge cases; wrong logic; regressions; + - security and data isolation: secrets in code, injection, missing authorization, cross-tenant/school reads; + - tests: is the new behaviour covered; are tests meaningful (not asserting nothing); + - the repository's own conventions and rules (language, structure, naming) where it states them. + You may build the project to confirm a suspicion. Do not run the full end-to-end stack — testing comes next. + 4. Grade each finding: + - "must-fix": a real defect, security/isolation hole, missing behaviour the ticket requires, or a broken + repository rule. Only these block the change. + - "should-fix": a clear improvement that is not a defect. + - "nit": style or taste. Keep these few. + Do not invent problems; an approval with no findings is a good outcome. + + Reply ONLY with this JSON (no other text): + {{"verdict":"approve","findings":[{{"severity":"must-fix","file":"","line":42,"comment":""}}],"summary":""}} + "verdict" is "approve" when there is no must-fix finding, otherwise "changes". + """, + r.Repo.Owner, r.Repo.Repo, r.Repo.ExistingBranch, r.Repo.DefaultBranch); + + /// Reads the agent's JSON verdict. An approval is only trusted when no finding is must-fix. + internal static ReviewVerdict Parse(string content) + { + var start = content.LastIndexOf("{\"verdict\"", StringComparison.Ordinal); + if (start < 0) { start = content.IndexOf('{', StringComparison.Ordinal); } + var end = content.LastIndexOf('}'); + if (start < 0 || end <= start) { return NotRun("The Reviewer agent did not return a verdict."); } + try + { + using var doc = JsonDocument.Parse(content[start..(end + 1)]); + var root = doc.RootElement; + var findings = root.TryGetProperty("findings", out var fs) && fs.ValueKind == JsonValueKind.Array + ? fs.EnumerateArray().Select(f => new ReviewFinding( + Severity(Str(f, "severity")), Str(f, "file"), + f.TryGetProperty("line", out var l) && l.ValueKind == JsonValueKind.Number && l.TryGetInt32(out var n) && n > 0 ? n : null, + Str(f, "comment"))).Where(f => f.Comment.Length > 0).ToList() + : []; + var claimed = string.Equals(Str(root, "verdict"), "approve", StringComparison.OrdinalIgnoreCase); + return new ReviewVerdict(true, claimed && !findings.Any(f => f.Blocks), findings, Str(root, "summary"), null); + } + catch (JsonException) + { + return NotRun("The Reviewer agent's verdict was not valid JSON."); + } + + static string Str(JsonElement e, string name) => + e.TryGetProperty(name, out var v) && v.ValueKind == JsonValueKind.String ? v.GetString() ?? "" : ""; + + static string Severity(string s) => s.Trim().ToLowerInvariant() switch + { + "must-fix" or "must" or "blocker" => "must-fix", + "should-fix" or "should" => "should-fix", + _ => "nit", + }; + } + + /// The review as a pull-request comment (Markdown). + public static string Comment(int issueNumber, ReviewVerdict verdict) + { + ArgumentNullException.ThrowIfNull(verdict); + var sb = new System.Text.StringBuilder(); + sb.Append(verdict.Approved ? "### ✅ AgentOS.NET review — approved" : "### 🔁 AgentOS.NET review — changes requested") + .Append(" (#").Append(issueNumber.ToString(CultureInfo.InvariantCulture)).Append(")\n\n"); + if (!string.IsNullOrWhiteSpace(verdict.Summary)) { sb.Append(verdict.Summary.Trim()).Append("\n\n"); } + if (verdict.Findings.Count > 0) + { + sb.Append("| Severity | Where | Finding |\n|---|---|---|\n"); + foreach (var f in verdict.Findings.OrderBy(f => f.Severity switch { "must-fix" => 0, "should-fix" => 1, _ => 2 })) + { + sb.Append("| ").Append(f.Severity).Append(" | `").Append(Cell(Where(f))).Append("` | ").Append(Cell(f.Comment)).Append(" |\n"); + } + } + sb.Append("\nRun by the AgentOS.NET Reviewer agent on a paired computer. It does not change the code; must-fix findings go back to the Coding agent.\n"); + return sb.ToString(); + + static string Cell(string s) => s.Replace("|", "\\|", StringComparison.Ordinal).Replace("\n", " ", StringComparison.Ordinal).Trim(); + } + + /// The must-fix findings, as the change request sent back to the Coding agent. + public static string ChangeRequest(ReviewVerdict verdict) + { + ArgumentNullException.ThrowIfNull(verdict); + var lines = verdict.Blocking.Select(f => $"- {Where(f)}: {f.Comment}").Prepend(verdict.Summary); + return "Code review asked for changes. Fix every must-fix finding (or explain in the pull request why it is not a defect):\n" + + string.Join("\n", lines.Where(l => !string.IsNullOrWhiteSpace(l))); + } + + private static string Where(ReviewFinding f) => + string.IsNullOrWhiteSpace(f.File) ? "(general)" : f.Line is { } line ? $"{f.File}:{line.ToString(CultureInfo.InvariantCulture)}" : f.File; +} diff --git a/src/AgentOs.Modules.Sessions/Persistence/Repositories/SessionRepository.cs b/src/AgentOs.Modules.Sessions/Persistence/Repositories/SessionRepository.cs index da0a8eb..e82c23e 100644 --- a/src/AgentOs.Modules.Sessions/Persistence/Repositories/SessionRepository.cs +++ b/src/AgentOs.Modules.Sessions/Persistence/Repositories/SessionRepository.cs @@ -230,7 +230,8 @@ public async Task RecomputeSessionStatusForTenantAsync(string tenantId, Gu : repos.Any(r => r.Status == "Blocked") ? "Blocked" // QA gate refused the PR (plan 0.5) : repos.All(r => r.Status == "Merged") ? "Merged" // every proposed change is in // Done = every repo has a proposed change, tested or not (Accepted / Rejected by the Testing agent). - : repos.All(r => r.Status is "Done" or "Merged" or "Accepted" or "Rejected") ? "Done" + // (or reviewed: Reviewed / ChangesRequested by the Reviewer agent). + : repos.All(r => r.Status is "Done" or "Merged" or "Accepted" or "Rejected" or "Reviewed" or "ChangesRequested") ? "Done" : "Running"; // Mirror the first PR + first error onto the parent for back-compat display. diff --git a/src/AgentOs.Web/Components/Pages/BoardApp.razor b/src/AgentOs.Web/Components/Pages/BoardApp.razor index c316c8c..f91f7e5 100644 --- a/src/AgentOs.Web/Components/Pages/BoardApp.razor +++ b/src/AgentOs.Web/Components/Pages/BoardApp.razor @@ -2039,6 +2039,9 @@ else if (_tab == Tab.Sessions) "Blocked" => "Needs your review", // the QA gate refused to open the change "Done" => "Ready for review", "Merged" => "Done", + "Reviewing" => "Being reviewed", + "Reviewed" => "Review passed", + "ChangesRequested" => "Review asked for changes", "Testing" => "Being tested", "Accepted" => "Passed testing", "Rejected" => "Failed testing", diff --git a/src/AgentOs.Web/Services/BoardTasks/BoardTaskService.cs b/src/AgentOs.Web/Services/BoardTasks/BoardTaskService.cs index b87fa38..c30b1e1 100644 --- a/src/AgentOs.Web/Services/BoardTasks/BoardTaskService.cs +++ b/src/AgentOs.Web/Services/BoardTasks/BoardTaskService.cs @@ -157,7 +157,7 @@ public async Task TestAsync(string tenantId, Guid sessionId, string? .GetForTenantAsync(tenantId, session.WorkspaceId, ct).ConfigureAwait(false); if (board is null) { return TaskStart.BoardNotFound; } var changes = (await sessions.ListReposForTenantAsync(tenantId, session.Id, ct).ConfigureAwait(false)) - .Where(r => r.Status is "Done" or "Accepted" or "Rejected" && !string.IsNullOrEmpty(r.PrUrl) && !string.IsNullOrEmpty(r.BranchName)) + .Where(r => r.Status is "Done" or "Reviewed" or "Accepted" or "Rejected" && !string.IsNullOrEmpty(r.PrUrl) && !string.IsNullOrEmpty(r.BranchName)) .ToList(); if (changes.Count == 0) { return TaskStart.NoCodeProject; } var pat = await scope.ServiceProvider.GetRequiredService() @@ -174,6 +174,86 @@ public async Task TestAsync(string tenantId, Guid sessionId, string? return TaskStart.Started; } + /// + /// Agentic review: the Reviewer agent reads every proposed change of a task (still unreviewed) against its ticket on + /// the member's computer, posts the review on each pull request, and marks it Reviewed or ChangesRequested (its + /// must-fix findings become the change request the Coding agent gets next). + /// + public async Task ReviewAsync(string tenantId, Guid sessionId, string? cliProfile = null, CancellationToken ct = default) + { + ArgumentException.ThrowIfNullOrWhiteSpace(tenantId); + using var scope = scopes.CreateScope(); + var sessions = scope.ServiceProvider.GetRequiredService(); + var session = (await sessions.ListForTenantAsync(tenantId, ct).ConfigureAwait(false)).FirstOrDefault(s => s.Id == sessionId); + if (session is null) { return TaskStart.TaskNotFound; } + var board = await scope.ServiceProvider.GetRequiredService() + .GetForTenantAsync(tenantId, session.WorkspaceId, ct).ConfigureAwait(false); + if (board is null) { return TaskStart.BoardNotFound; } + var changes = (await sessions.ListReposForTenantAsync(tenantId, session.Id, ct).ConfigureAwait(false)) + .Where(r => r.Status == "Done" && !string.IsNullOrEmpty(r.PrUrl) && !string.IsNullOrEmpty(r.BranchName)) + .ToList(); + if (changes.Count == 0) { return TaskStart.NoCodeProject; } + var pat = await scope.ServiceProvider.GetRequiredService() + .GetForTenantAsync(tenantId, board.CredentialRef, ct).ConfigureAwait(false); + if (string.IsNullOrEmpty(pat)) { return TaskStart.NoGitHubAccess; } + + await sessions.UpdateRunResultAsync(tenantId, session.Id, "Running", session.PrUrl, null, ct).ConfigureAwait(false); + foreach (var c in changes) + { + await sessions.UpdateRepoRunResultAsync(tenantId, c.Id, "Reviewing", c.BranchName, c.PrUrl, null, ct).ConfigureAwait(false); + } + var stopLease = stops.Begin(tenantId, session.Id); + _ = Task.Run(() => ReviewChangesAsync(tenantId, session, board, pat, changes, cliProfile, stopLease), CancellationToken.None); + return TaskStart.Started; + } + + private async Task ReviewChangesAsync(string tenantId, RemoteSessionEntity session, WorkspaceEntity board, string pat, + IReadOnlyList changes, string? cliProfile, TaskStopRegistry.Lease stopLease) + { + using var _stop = stopLease; + try + { + using var scope = scopes.CreateScope(); + using var _identity = AgentOs.SharedKernel.Identity.AmbientIdentity.Push(tenantId, session.MemberUserId, session.Id); + var sessRepo = scope.ServiceProvider.GetRequiredService(); + var reviewer = scope.ServiceProvider.GetRequiredService(); + var prs = scope.ServiceProvider.GetRequiredService(); + var body = await ReadTicketBodyAsync(scope, board, tenantId, session, pat).ConfigureAwait(false) ?? ""; + foreach (var change in changes) + { + var repoLabel = $"{change.Owner}/{change.Repo}"; + var verdict = await reviewer.RunAsync(new AcceptanceRequest(session.Id, tenantId, session.MemberUserId, + new WorkRepo(change.Id, change.Owner, change.Repo, change.DefaultBranch, change.BranchName), + session.IssueNumber ?? 0, session.Title, body, cliProfile), stopLease.Token).ConfigureAwait(false); + if (!verdict.Ran) + { + await sessRepo.UpdateRepoRunResultAsync(tenantId, change.Id, "Done", change.BranchName, change.PrUrl, null).ConfigureAwait(false); + feed.Publish(new SessionRunEvent(tenantId, session.Id, SessionRunEventKind.Step, $"{repoLabel} — couldn't review: {verdict.Error}", DateTimeOffset.UtcNow, repoLabel)); + continue; + } + if (PrNumber(change.PrUrl) is { } number) + { + await prs.CommentAsync(RepoDescriptor(board, tenantId, change.Owner, change.Repo, change.DefaultBranch, pat), number, + AgentOs.Modules.Pipeline.Agents.ReviewAgent.Comment(session.IssueNumber ?? 0, verdict)).ConfigureAwait(false); + } + await sessRepo.UpdateRepoRunResultAsync(tenantId, change.Id, verdict.Approved ? "Reviewed" : "ChangesRequested", change.BranchName, change.PrUrl, + verdict.Approved ? null : AgentOs.Modules.Pipeline.Agents.ReviewAgent.ChangeRequest(verdict)).ConfigureAwait(false); + feed.Publish(new SessionRunEvent(tenantId, session.Id, SessionRunEventKind.RepoDone, + verdict.Approved ? $"{repoLabel} — review approved" : $"{repoLabel} — review asked for changes: {verdict.Summary}", + DateTimeOffset.UtcNow, repoLabel, verdict.Approved ? "Reviewed" : "ChangesRequested", change.PrUrl)); + } + await sessRepo.RecomputeSessionStatusForTenantAsync(tenantId, session.Id).ConfigureAwait(false); + } + catch (OperationCanceledException) when (stopLease.Token.IsCancellationRequested) + { + await MarkStoppedAsync(tenantId, session.Id).ConfigureAwait(false); + } + catch (Exception ex) when (ex is System.Net.Http.HttpRequestException or InvalidOperationException or System.IO.IOException or TimeoutException) + { + await MarkFailedAsync(tenantId, session.Id, ex.Message).ConfigureAwait(false); + } + } + private async Task TestChangesAsync(string tenantId, RemoteSessionEntity session, WorkspaceEntity board, string pat, IReadOnlyList changes, string? cliProfile, TaskStopRegistry.Lease stopLease) { diff --git a/src/AgentOs.Web/Services/Orchestration/AutopilotPlanner.cs b/src/AgentOs.Web/Services/Orchestration/AutopilotPlanner.cs index beb3da1..1d7cab3 100644 --- a/src/AgentOs.Web/Services/Orchestration/AutopilotPlanner.cs +++ b/src/AgentOs.Web/Services/Orchestration/AutopilotPlanner.cs @@ -27,10 +27,13 @@ public sealed record PlanTicket(int Number, bool Open, bool AiReady, int Priorit /// Where a proposed change stands with the Testing agent. public enum ChangeTesting { Untested, Testing, Accepted, Rejected } +/// Where a proposed change stands with the Reviewer agent (reviewed before it is tested). +public enum ChangeReview { NotReviewed, Reviewing, Approved, ChangesRequested } + /// An open proposed change of a task. public sealed record PlanChange(Guid RepoId, int PrNumber, PrChecksState Checks, string? FailedReport, ChangeTesting Testing = ChangeTesting.Untested, string? Findings = null, PrMergeState Merge = PrMergeState.Clean, - string BaseBranch = "main"); + string BaseBranch = "main", ChangeReview Review = ChangeReview.Approved); /// A task as the planner sees it. public sealed record PlanTask(Guid SessionId, int IssueNumber, string Status, IReadOnlyList OpenChanges, int FixRounds); @@ -42,6 +45,7 @@ public sealed record StartTicket(int Number, string Why) : AutopilotAction(Why); public sealed record FixChecks(Guid SessionId, int IssueNumber, string Report, int Round, string Why) : AutopilotAction(Why); public sealed record UpdateBranch(Guid SessionId, int IssueNumber, Guid RepoId, int PrNumber, string Why) : AutopilotAction(Why); public sealed record RunTests(Guid SessionId, int IssueNumber, string Why) : AutopilotAction(Why); + public sealed record RunReview(Guid SessionId, int IssueNumber, string Why) : AutopilotAction(Why); public sealed record Merge(Guid SessionId, int IssueNumber, Guid RepoId, int PrNumber, string Why) : AutopilotAction(Why); public sealed record NeedsPerson(Guid SessionId, int IssueNumber, string Reason, string Why) : AutopilotAction(Why); } @@ -119,12 +123,40 @@ public static IReadOnlyList Decide(AutopilotMode mode, IReadOnl running++; continue; } - // Testing comes after green checks; nothing moves while a test runs or a check is still going. - if (task.OpenChanges.Count == 0 || task.OpenChanges.Any(c => c.Testing == ChangeTesting.Testing) + // Review, then testing, come after green checks; nothing moves while one runs or a check is still going. + if (task.OpenChanges.Count == 0 || task.OpenChanges.Any(c => c.Testing == ChangeTesting.Testing || c.Review == ChangeReview.Reviewing) || !task.OpenChanges.All(c => c.Checks == PrChecksState.Passed)) { continue; } + // The Reviewer agent asked for changes: they go back to the Coding agent (same bounded rounds as any fix). + var changesRequested = task.OpenChanges.Where(c => c.Review == ChangeReview.ChangesRequested).ToList(); + if (changesRequested.Count > 0) + { + if (task.FixRounds >= MaxFixRounds) + { + actions.Add(new AutopilotAction.NeedsPerson(task.SessionId, task.IssueNumber, + $"Code review still asks for changes after {MaxFixRounds} automatic fixes — needs a person.", + $"Stopped fixing #{task.IssueNumber}: code review still asks for changes after {MaxFixRounds} tries. It needs you.")); + continue; + } + if (running >= MaxParallel) { continue; } + var round = task.FixRounds + 1; + actions.Add(new AutopilotAction.FixChecks(task.SessionId, task.IssueNumber, + string.Join('\n', changesRequested.Select(c => c.Findings).Where(r => !string.IsNullOrWhiteSpace(r))), round, + $"Code review asked for changes on #{task.IssueNumber} — sent back to the AI (try {round} of {MaxFixRounds}).")); + running++; + continue; + } + if (task.OpenChanges.Any(c => c.Review == ChangeReview.NotReviewed)) + { + // A review only reads the code: no app, no ports — just a free slot and the member's computer. + if (!canTest || running >= MaxParallel) { continue; } + actions.Add(new AutopilotAction.RunReview(task.SessionId, task.IssueNumber, + $"Reviewing #{task.IssueNumber}'s change — its checks passed.")); + running++; + continue; + } var rejected = task.OpenChanges.Where(c => c.Testing == ChangeTesting.Rejected).ToList(); if (rejected.Count > 0) { diff --git a/src/AgentOs.Web/Services/Orchestration/ProjectOrchestrator.cs b/src/AgentOs.Web/Services/Orchestration/ProjectOrchestrator.cs index 1b7c07b..a9f2167 100644 --- a/src/AgentOs.Web/Services/Orchestration/ProjectOrchestrator.cs +++ b/src/AgentOs.Web/Services/Orchestration/ProjectOrchestrator.cs @@ -166,7 +166,8 @@ public async Task> StepAsync(WorkspaceEntity boar foreach (var s in sessions.Where(s => s.Status is "Running" or "Done")) { var open = new List(); - foreach (var r in repos[s.Id].Where(r => r.Status is "Done" or "Testing" or "Accepted" or "Rejected" && PrNumber(r.PrUrl) is not null)) + foreach (var r in repos[s.Id].Where(r => r.Status is "Done" or "Reviewing" or "Reviewed" or "ChangesRequested" or "Testing" or "Accepted" or "Rejected" + && PrNumber(r.PrUrl) is not null)) { var desc = BoardTaskService.RepoDescriptor(board, tenant, r.Owner, r.Repo, r.DefaultBranch, pat); var number = PrNumber(r.PrUrl)!.Value; @@ -185,8 +186,17 @@ public async Task> StepAsync(WorkspaceEntity boar "Rejected" => ChangeTesting.Rejected, _ => ChangeTesting.Untested, }; + // A change waits for review only while it is fresh ("Done"); once tested it was reviewed (or predates review). + var review = r.Status switch + { + "Done" => ChangeReview.NotReviewed, + "Reviewing" => ChangeReview.Reviewing, + "ChangesRequested" => ChangeReview.ChangesRequested, + _ => ChangeReview.Approved, + }; var merge = s.Status == "Done" ? await prs.GetMergeStateAsync(desc, number, ct).ConfigureAwait(false) : PrMergeState.Unknown; - open.Add(new PlanChange(r.Id, number, checks, report, testing, testing == ChangeTesting.Rejected ? r.Error : null, merge, r.DefaultBranch)); + var findings = testing == ChangeTesting.Rejected || review == ChangeReview.ChangesRequested ? r.Error : null; + open.Add(new PlanChange(r.Id, number, checks, report, testing, findings, merge, r.DefaultBranch, review)); } planTasks.Add(new PlanTask(s.Id, s.IssueNumber ?? 0, s.Status, open, _fixRounds.GetValueOrDefault(s.Id))); } @@ -278,6 +288,12 @@ private async Task ExecuteAsync(AutopilotAction action, WorkspaceEntity board, s log.Add(tenant, board.Id, error is null ? update.Why : $"Couldn't update #{update.IssueNumber}'s branch: {error}"); break; } + case AutopilotAction.RunReview review: + { + var reviewed = await tasks.ReviewAsync(tenant, review.SessionId, ct: ct).ConfigureAwait(false); + log.Add(tenant, board.Id, reviewed == TaskStart.Started ? review.Why : $"Couldn't review #{review.IssueNumber}: {reviewed}."); + break; + } case AutopilotAction.RunTests test: { var tested = await tasks.TestAsync(tenant, test.SessionId, ct: ct).ConfigureAwait(false); @@ -332,7 +348,7 @@ private async Task RecoverOrphansAsync(WorkspaceEntity board, IReadOnlyLis _orphanSeen.TryRemove(s.Id, out _); var children = repos[s.Id].ToList(); var wasCoding = children.Any(r => r.Status is "Pending" or "Running"); - foreach (var r in children.Where(r => r.Status == "Testing")) + foreach (var r in children.Where(r => r.Status is "Testing" or "Reviewing")) { await sessionsRepo.UpdateRepoRunResultAsync(tenant, r.Id, "Done", r.BranchName, r.PrUrl, null, ct).ConfigureAwait(false); } @@ -346,7 +362,7 @@ private async Task RecoverOrphansAsync(WorkspaceEntity board, IReadOnlyLis } else { - log.Add(tenant, board.Id, $"#{s.IssueNumber}'s test was cut short by a restart — it will be tested again."); + log.Add(tenant, board.Id, $"#{s.IssueNumber}'s review or test was cut short by a restart — it will run again."); } changed = true; } diff --git a/tests/AgentOs.Tests/Web/ReviewStepTests.cs b/tests/AgentOs.Tests/Web/ReviewStepTests.cs new file mode 100644 index 0000000..f5ebba4 --- /dev/null +++ b/tests/AgentOs.Tests/Web/ReviewStepTests.cs @@ -0,0 +1,109 @@ +// The Reviewer agent's step: green checks → review → (changes go back to the Coding agent, bounded) → testing → merge. +// Plus its verdict parsing and the pull-request comment. + +using AgentOs.Domain.Sessions; +using AgentOs.Domain.Workspaces; +using AgentOs.Modules.Pipeline.Agents; +using AgentOs.Web.Services.Orchestration; +using Shouldly; +using Xunit; + +namespace AgentOs.Tests.Web; + +public sealed class ReviewStepTests +{ + private static PlanTask Task(int issue, int rounds, params PlanChange[] changes) => new(Guid.NewGuid(), issue, "Done", changes, rounds); + + private static PlanChange Change(ChangeReview review, ChangeTesting testing = ChangeTesting.Untested, string? findings = null) => + new(Guid.NewGuid(), 30, PrChecksState.Passed, null, testing, findings, PrMergeState.Clean, "main", review); + + [Fact] + public void GreenUnreviewedChange_IsReviewedBeforeItIsTested() + { + var actions = AutopilotPlanner.Decide(AutopilotMode.BuildAndMerge, [], [Task(15, 0, Change(ChangeReview.NotReviewed))]); + + var review = actions.ShouldHaveSingleItem().ShouldBeOfType(); + review.Why.ShouldBe("Reviewing #15's change — its checks passed."); + } + + [Fact] + public void ReviewNeedsTheMembersComputer() => + AutopilotPlanner.Decide(AutopilotMode.Build, [], [Task(15, 0, Change(ChangeReview.NotReviewed))], canTest: false).ShouldBeEmpty(); + + [Fact] + public void WhileReviewing_NothingElseHappensToTheTask() => + AutopilotPlanner.Decide(AutopilotMode.BuildAndMerge, [], [Task(15, 0, Change(ChangeReview.Reviewing))]).ShouldBeEmpty(); + + [Fact] + public void ChangesRequested_GoBackToTheCodingAgent_WithTheFindings() + { + var actions = AutopilotPlanner.Decide(AutopilotMode.Build, [], + [Task(15, 0, Change(ChangeReview.ChangesRequested, findings: "- Viewer.tsx:42: off-by-one on the last move"))]); + + var fix = actions.ShouldHaveSingleItem().ShouldBeOfType(); + fix.Report.ShouldContain("off-by-one"); + fix.Why.ShouldBe("Code review asked for changes on #15 — sent back to the AI (try 1 of 3)."); + } + + [Fact] + public void ChangesRequested_AfterThreeRounds_NeedsAPerson() => + AutopilotPlanner.Decide(AutopilotMode.Build, [], [Task(15, 3, Change(ChangeReview.ChangesRequested, findings: "x"))]) + .ShouldHaveSingleItem().ShouldBeOfType(); + + [Fact] + public void ApprovedChange_IsTestedNext() => + AutopilotPlanner.Decide(AutopilotMode.Build, [], [Task(15, 0, Change(ChangeReview.Approved))]) + .ShouldHaveSingleItem().ShouldBeOfType(); + + [Fact] + public void ApprovedAndAccepted_IsMerged() => + AutopilotPlanner.Decide(AutopilotMode.BuildAndMerge, [], [Task(15, 0, Change(ChangeReview.Approved, ChangeTesting.Accepted))]) + .ShouldHaveSingleItem().ShouldBeOfType(); + + // ── Verdict ── + + [Fact] + public void Parse_ApproveWithOnlyNits_IsApproved() + { + var v = ReviewAgent.Parse("""noise {"verdict":"approve","findings":[{"severity":"nit","file":"a.ts","line":3,"comment":"rename x"}],"summary":"Looks right."}"""); + v.Ran.ShouldBeTrue(); + v.Approved.ShouldBeTrue(); + v.Findings.ShouldHaveSingleItem().Line.ShouldBe(3); + } + + [Fact] + public void Parse_ApproveClaimWithAMustFix_IsNotTrusted() + { + var v = ReviewAgent.Parse("""{"verdict":"approve","findings":[{"severity":"Must-Fix","file":"Api.cs","comment":"reads another school's rows"}],"summary":"ok"}"""); + v.Approved.ShouldBeFalse(); + v.Blocking.ShouldHaveSingleItem().Comment.ShouldContain("another school"); + } + + [Fact] + public void Parse_NoJson_DidNotRun() => ReviewAgent.Parse("I could not finish").Ran.ShouldBeFalse(); + + [Fact] + public void ChangeRequest_ListsOnlyMustFixFindings() + { + var v = new ReviewVerdict(true, false, + [new ReviewFinding("must-fix", "Api.cs", 12, "missing authorization"), new ReviewFinding("nit", "a.ts", null, "style")], + "One defect.", null); + + var text = ReviewAgent.ChangeRequest(v); + + text.ShouldContain("Api.cs:12: missing authorization"); + text.ShouldNotContain("style"); + } + + [Fact] + public void Comment_ShowsVerdictAndEscapesTableCells() + { + var v = new ReviewVerdict(true, false, [new ReviewFinding("must-fix", "a|b.cs", 1, "x | y")], "s", null); + + var md = ReviewAgent.Comment(15, v); + + md.ShouldStartWith("### 🔁 AgentOS.NET review — changes requested (#15)"); + md.ShouldContain(@"a\|b.cs:1"); + md.ShouldContain(@"x \| y"); + } +}