From c46b37dafb20d020a4836bb389d8602b8f0a5330 Mon Sep 17 00:00:00 2001 From: Operator Bot Date: Sat, 11 Jul 2026 11:44:36 +0100 Subject: [PATCH 1/3] Completed task: T20260705-7E556EBC --- .operator/data/tasks/T20260705-7E556EBC.md | 4 +- .../_shared/supervisor-after-agent.ts | 240 +++++++++++++ .../_shared/supervisor-bot-messages.test.ts | 56 +++ .../_shared/supervisor-bot-messages.ts | 29 ++ .../composers/_shared/supervisor-scratch.ts | 28 ++ .../composers/_shared/supervisor-task.test.ts | 25 ++ .../composers/_shared/supervisor-task.ts | 85 +++++ .../pr-feedback-supervisor-stage.test.ts | 24 -- .../composers/pr-feedback-supervisor-stage.ts | 335 +----------------- 9 files changed, 477 insertions(+), 349 deletions(-) create mode 100644 engine/pipeline/composers/_shared/supervisor-after-agent.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-bot-messages.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-bot-messages.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-scratch.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-task.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-task.ts diff --git a/.operator/data/tasks/T20260705-7E556EBC.md b/.operator/data/tasks/T20260705-7E556EBC.md index cf33142..eed87d7 100644 --- a/.operator/data/tasks/T20260705-7E556EBC.md +++ b/.operator/data/tasks/T20260705-7E556EBC.md @@ -2,12 +2,12 @@ id: T20260705-7E556EBC kind: task title: Split pr-feedback-supervisor-stage.ts under the 200 code-line cap by extracting the supervisor prompt builder -status: pending +status: completed priority: 4 created_at: '2026-07-11T08:48:02Z' +completed_at: "2026-07-11T10:44:36Z" parent_id: F20260705-CC7FF1B9 --- - # Split pr-feedback-supervisor-stage.ts under the pipeline line cap ## Problem diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent.ts b/engine/pipeline/composers/_shared/supervisor-after-agent.ts new file mode 100644 index 0000000..78ee54e --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-after-agent.ts @@ -0,0 +1,240 @@ +import type { + OperationContext, KindRegistry, WorkItemSource, AgentEventStream, +} from "@operator/core"; +import type { PRManager } from "../../../delivery/pr-manager.js"; +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { Logger } from "../../../logging/logger.js"; +import { formatDebugRunLinkSuffix } from "../../../delivery/vcs-helpers.js"; +import type { BotAttribution } from "../../../delivery/bot-footer.js"; +import type { StageDef, AgentResult, Verdict } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import { applyAgentEvents } from "../../primitives/aop-applier.js"; +import { applyThreadDispositions } from "./thread-dispositions.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; +import { + formatAppliedReviewFeedbackMessage, + formatNoCodeChangesMessage, + formatReviewLimitReachedMessage, + formatStaleCiFixMessage, + formatSupervisorTerminalMessage, +} from "./supervisor-bot-messages.js"; + +export interface SupervisorAfterAgentDeps { + readonly prManager: PRManager; + readonly git: WorkspaceGit; + readonly kindRegistry: KindRegistry; + readonly workItemSource: WorkItemSource; + readonly agentEventStream: AgentEventStream; + readonly log?: Logger; + readonly debug?: boolean; + readonly debugRunUrl?: string; +} + +function inferKindFromBranch(branch: string, registry: KindRegistry): { kind: string; id: string } | null { + for (const kindDef of registry.all) { + const prefix = kindDef.branchPrefix.endsWith("/") ? kindDef.branchPrefix : `${kindDef.branchPrefix}/`; + if (branch.startsWith(prefix)) { + const id = branch.slice(prefix.length); + if (id) return { kind: kindDef.name, id }; + } + } + return null; +} + +export async function processSupervisorAfterAgent( + deps: SupervisorAfterAgentDeps, + stage: StageDef, + payload: PrFeedbackPayload, + scratch: PrFeedbackSupervisorScratch, + agentResult: AgentResult, + ctx: OperationContext, +): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> { + const suffix = formatDebugRunLinkSuffix(deps.debug, deps.debugRunUrl); + const ciFailing = payload.checks.value === "failing"; + const nextAttribution: BotAttribution = { + responded: new Set(payload.respondedIds), + ciHead: payload.checks.headSha, + ciAttempt: ciFailing + ? { current: payload.ciAttempts + 1, max: payload.maxCiRetryAttempts } + : undefined, + }; + + if (scratch.limitReached) { + const msg = formatReviewLimitReachedMessage(scratch.reviewAttempts, scratch.maxAttempts, suffix); + await deps.prManager.postBotComment(payload.prId, msg, nextAttribution); + deps.log?.info(`${stage.name}: PR #${payload.prId} limit reached — verdict override failed`, { + stage: stage.name, prNumber: payload.prId, + reviewAttempts: scratch.reviewAttempts, maxAttempts: scratch.maxAttempts, + }); + return { + verdictOverride: "failed", + summaryOverride: `review cycle limit reached (${scratch.reviewAttempts}/${scratch.maxAttempts})`, + }; + } + + // 2026-05-13: removed defense-in-depth "approved + ciFailing → + // override to failed" check. The verifier (inside the agent chain + // when stage has reviewEnabled: true) is the authority on whether + // the supervisor's fix addresses CI. Defense-in-depth duplicated + // verifier and second-guessed it from a stale CI observation — + // CI was observed at cycle start (BEFORE supervisor committed via + // Bash) so it always looked "failing" even when the fix had just + // been pushed and CI re-run hadn't completed yet. The canonical + // case: supervisor correctly fixed all 47 backend test failures + // and 14 Copilot comments and + // committed/pushed, but the post-verifier check flipped to failed + // because checks.headSha was the pre-commit SHA. Per user guidance: + // "verify process should be able to detect commits and verify them + // even if committed — if OK act as usual; if wrong comment back to + // redo/fix. Committed work has no difference except technical to + // detect changes." Trust verifier — if its judgment is wrong, the + // next pr-feedback cycle picks the PR up with fresh CI data. + + const activeItem = inferKindFromBranch(payload.branch, deps.kindRegistry); + const applied = await applyAgentEvents( + agentResult.output, + { + stream: deps.agentEventStream, + source: deps.workItemSource, + registry: deps.kindRegistry, + log: deps.log, + }, + { + workItem: activeItem ? { id: activeItem.id, kind: activeItem.kind } : undefined, + }, + ctx, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} applied ${applied.applied.childItems.length} child-item(s), ${applied.applied.statusUpdates.length} status-update(s); applier verdict=${applied.verdict}`, { + stage: stage.name, prNumber: payload.prId, + applierVerdict: applied.verdict, + childItems: applied.applied.childItems.length, + statusUpdates: applied.applied.statusUpdates.length, + bodyUpdates: applied.applied.bodyUpdates.length, + applyErrors: applied.applyErrors.length, + }); + + // Answer + resolve inline review threads the supervisor disposed of + // this cycle. Runs on every agent path (fix-in-place, cancel, escalate, + // …) so no reviewer comment is left without a note. Bot threads (Copilot) + // are resolved; human threads get the note but stay open for the human. + if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { + await applyThreadDispositions({ + prId: payload.prId, + stage: stage.name, + commentReplies: applied.commentReplies, + reviewThreads: payload.reviewThreads, + freshReviewCommentIds: payload.freshReviewCommentIds, + prManager: deps.prManager, + log: deps.log, + }); + } + + // "Changes applied" detection — true when EITHER the workspace + // has uncommitted edits OR the agent advanced HEAD by committing + // directly via Bash. Pre-2026-05-20 the check only inspected the + // dirty flag; a post-commit clean tree was reported as "No code + // changes" even though the agent had committed (PR-887). The + // headSha comparison catches that path — falls back to the dirty + // check when the pre-agent SHA was not captured. Hoisted above the + // verdict branches so the stale-CI guard below sees it too. + const workspaceDirty = !(await deps.git.isClean()); + let postAgentHeadSha = ""; + try { + postAgentHeadSha = (await deps.git.headSha()).trim(); + } catch (err) { + deps.log?.warn(`${stage.name}: failed to capture post-agent HEAD SHA (falling back to dirty-only check)`, { + stage: stage.name, prNumber: payload.prId, + error: err instanceof Error ? err.message : String(err), + }); + } + const headAdvanced = scratch.preAgentHeadSha.length > 0 + && postAgentHeadSha.length > 0 + && scratch.preAgentHeadSha !== postAgentHeadSha; + const changesApplied = workspaceDirty || headAdvanced; + + // Stale-CI guard — completes the 2026-05-13 stale-CI fix (PR-1186). + // `payload.checks` is observed at cycle start, BEFORE the supervisor + // edits/commits, so a `failed` verdict resting on it — the verifier + // hard-rule "approved while CI failing", or the supervisor declining to + // approve over red CI — is judging a run the just-pushed fix already + // supersedes. When the supervisor DID push a fix in response to that + // failing CI, never latch terminal `ai:failed` on the stale observation: + // leave the PR in-review so fresh CI on the new commit decides and the + // next pr-feedback cycle re-evaluates (the recovery the 2026-05-13 note + // promised but the selector's `ai:failed` exclusion otherwise blocks). + // Bounded by the maxReviewAttempts cap in beforeAgent, so a genuinely + // unfixed failure still reaches `ai:failed` once the review budget is + // spent. Excludes `cancelled` (a human /cancel is a real terminal) and + // apply/parse errors (real contract violations, not CI staleness). + const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) + ? applied.verdict + : agentResult.verdict; + if (effectiveVerdict === "failed" && applied.applyErrors.length === 0 && changesApplied && ciFailing) { + await deps.prManager.postBotComment( + payload.prId, + formatStaleCiFixMessage(payload.checks.headSha, suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} failed verdict rested on stale pre-fix CI but the supervisor pushed a fix — downgrading to in-review for fresh-CI re-evaluation`, { + stage: stage.name, prNumber: payload.prId, + effectiveVerdict, ciHead: payload.checks.headSha, + changesApplied, workspaceDirty, headAdvanced, + }); + return { verdictOverride: "approved", summaryOverride: applied.summary || agentResult.summary }; + } + + if (applied.verdict !== "approved" || applied.applyErrors.length > 0) { + const reason = applied.summary || agentResult.summary || "supervisor decision"; + const detail = applied.applyErrors.length > 0 + ? `\n\nApply errors: ${applied.applyErrors.map((e) => `${e.code}: ${e.message}`).join("; ")}` + : ""; + await deps.prManager.postBotComment( + payload.prId, + formatSupervisorTerminalMessage(reason, detail, suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} verdict=${applied.verdict} — posted terminal comment`, { + stage: stage.name, prNumber: payload.prId, + verdict: applied.verdict, reason, + }); + return { + verdictOverride: applied.verdict, + summaryOverride: applied.summary, + }; + } + + const effectiveSummary = applied.summary || agentResult.summary; + + if (changesApplied) { + await deps.prManager.postBotComment( + payload.prId, + formatAppliedReviewFeedbackMessage(suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place with changes — posted applied comment`, { + stage: stage.name, prNumber: payload.prId, + changesApplied: true, workspaceDirty, headAdvanced, + preAgentHeadSha: scratch.preAgentHeadSha.slice(0, 12), + postAgentHeadSha: postAgentHeadSha.slice(0, 12), + }); + } else { + // The engine asserts ONLY the fact it can observe (clean tree + + // unchanged HEAD ⇒ no code changes this cycle). It must NOT + // editorialize the REASON: a verdict=approved + no-changes run can + // mean "feedback genuinely already addressed" OR "supervisor chose + // escalate" (supervisor.md maps escalate → approved + no changes). + // The old hard-coded "considered the feedback already addressed" + // contradicted the agent's own reasoning on escalate cycles + // (PR #892, 2026-05-21). The WHY lives in the agent's + // reasoning block below — never in a fixed engine sentence. + await deps.prManager.postBotComment( + payload.prId, + formatNoCodeChangesMessage(effectiveSummary ?? "", suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place no changes — posted no-changes comment, leaving at ai:in-review`, { + stage: stage.name, prNumber: payload.prId, changesApplied: false, + }); + } + return { summaryOverride: effectiveSummary }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-bot-messages.test.ts b/engine/pipeline/composers/_shared/supervisor-bot-messages.test.ts new file mode 100644 index 0000000..82cc970 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-bot-messages.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect } from "vitest"; +import { + formatAppliedReviewFeedbackMessage, + formatNoCodeChangesMessage, + formatReviewLimitReachedMessage, + formatStaleCiFixMessage, + formatSupervisorTerminalMessage, +} from "./supervisor-bot-messages.js"; + +describe("supervisor bot message formatters", () => { + it("formats review limit reached message", () => { + const msg = formatReviewLimitReachedMessage(5, 20, " [debug]"); + expect(msg).toContain("Review cycle limit reached"); + expect(msg).toContain("5 review-fix cycles"); + expect(msg).toContain("limit: 20"); + expect(msg).toContain("[debug]"); + }); + + it("formats stale CI fix message with truncated head SHA", () => { + const msg = formatStaleCiFixMessage("abc123def456789", ""); + expect(msg).toContain("abc123def456"); + expect(msg).toContain("pushed fix supersedes"); + }); + + it("formats stale CI fix message with unknown SHA fallback", () => { + const msg = formatStaleCiFixMessage(undefined, ""); + expect(msg).toContain("unknown"); + }); + + it("formats terminal supervisor decision message", () => { + const msg = formatSupervisorTerminalMessage("cancelled by user", "", " [link]"); + expect(msg).toBe("Supervisor decision: cancelled by user. [link]"); + }); + + it("formats terminal message with apply error detail", () => { + const msg = formatSupervisorTerminalMessage("bad emit", "\n\nApply errors: PARSE: invalid", ""); + expect(msg).toContain("Apply errors: PARSE: invalid"); + }); + + it("formats applied review feedback message", () => { + expect(formatAppliedReviewFeedbackMessage(" [dbg]")).toBe("Applied review feedback. [dbg]"); + }); + + it("formats no-code-changes message with reasoning block", () => { + const msg = formatNoCodeChangesMessage("Escalating to owner.", ""); + expect(msg).toContain("No code changes in this cycle."); + expect(msg).toContain("Escalating to owner."); + expect(msg).toContain("Reply on this PR if you disagree"); + }); + + it("truncates long reasoning in no-code-changes message", () => { + const long = "x".repeat(2000); + const msg = formatNoCodeChangesMessage(long, ""); + expect(msg.length).toBeLessThan(long.length + 100); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-bot-messages.ts b/engine/pipeline/composers/_shared/supervisor-bot-messages.ts new file mode 100644 index 0000000..200dca6 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-bot-messages.ts @@ -0,0 +1,29 @@ +export function formatReviewLimitReachedMessage( + reviewAttempts: number, + maxAttempts: number, + suffix: string, +): string { + return `⚠️ **Review cycle limit reached** — This PR has gone through ${reviewAttempts} review-fix cycles (limit: ${maxAttempts}). The supervisor was unable to resolve all feedback within the allowed iterations. Marking as failed — manual intervention required.${suffix}`; +} + +export function formatStaleCiFixMessage(ciHeadSha: string | undefined, suffix: string): string { + return `Applied review feedback. The failing check(s) were observed on the pre-fix commit (${ciHeadSha?.slice(0, 12) ?? "unknown"}); the pushed fix supersedes that run — leaving the PR in review so fresh CI on the new commit decides.${suffix}`; +} + +export function formatSupervisorTerminalMessage( + reason: string, + applyErrorDetail: string, + suffix: string, +): string { + return `Supervisor decision: ${reason}.${applyErrorDetail}${suffix}`; +} + +export function formatAppliedReviewFeedbackMessage(suffix: string): string { + return `Applied review feedback.${suffix}`; +} + +export function formatNoCodeChangesMessage(effectiveSummary: string, suffix: string): string { + const reasoning = effectiveSummary.trim().slice(0, 1500); + const reasoningBlock = reasoning ? `\n\n${reasoning}` : ""; + return `No code changes in this cycle.${reasoningBlock}\n\nReply on this PR if you disagree and I'll re-evaluate.${suffix}`; +} diff --git a/engine/pipeline/composers/_shared/supervisor-scratch.ts b/engine/pipeline/composers/_shared/supervisor-scratch.ts new file mode 100644 index 0000000..6f23a51 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-scratch.ts @@ -0,0 +1,28 @@ +import { createScratchStore } from "./scratch.js"; + +export interface PrFeedbackSupervisorScratch { + readonly prId: number; + readonly branch: string; + readonly prType: string; + readonly reviewAttempts: number; + readonly maxAttempts: number; + readonly limitReached: boolean; + readonly threadFile: string; + readonly newFeedback: string; + checksContextFile: string; + /** + * HEAD SHA captured at beforeAgent (post workspace checkout). Used in + * afterAgent to detect whether the supervisor agent committed during + * the run. `git.isClean()` alone returned `true` after a successful + * commit, leading the engine to misreport "No code changes" when the + * agent had in fact committed and pushed — the 2026-05-20 PR-887 + * regression. Empty string when capture failed (best-effort; the + * afterAgent comparison treats empty as "unknown, fall back to dirty + * check"). + */ + preAgentHeadSha: string; +} + +export const prFeedbackSupervisorScratch = createScratchStore(); + +export const prFeedbackSupervisorScratchKey = (prId: number): string => String(prId); diff --git a/engine/pipeline/composers/_shared/supervisor-task.test.ts b/engine/pipeline/composers/_shared/supervisor-task.test.ts new file mode 100644 index 0000000..455af8c --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-task.test.ts @@ -0,0 +1,25 @@ +import { describe, it, expect } from "vitest"; +import { buildSupervisorTask } from "./supervisor-task.js"; + +describe("buildSupervisorTask", () => { + it("includes PR coordinates and feedback", () => { + const task = buildSupervisorTask("task", "ai/tasks/T-0001", "user comment here", ""); + expect(task).toContain("ai/tasks/T-0001"); + expect(task).toContain("user comment here"); + expect(task).toContain("fix-in-place"); + expect(task).toContain("cancel"); + expect(task).toContain("retry-as-new"); + }); + + it("includes CI context file path when supplied", () => { + const task = buildSupervisorTask("task", "ai/tasks/T-0001", "fb", "", "/tmp/ci.md"); + expect(task).toContain("/tmp/ci.md"); + expect(task).toContain("CI Pipeline Context"); + }); + + it("includes thread file path when supplied", () => { + const task = buildSupervisorTask("task", "ai/tasks/T-0001", "fb", "/tmp/thread.md"); + expect(task).toContain("/tmp/thread.md"); + expect(task).toContain("Discussion History"); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-task.ts b/engine/pipeline/composers/_shared/supervisor-task.ts new file mode 100644 index 0000000..807bf6d --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-task.ts @@ -0,0 +1,85 @@ +export function buildSupervisorTask( + prType: string, + branch: string, + newFeedback: string, + threadFile: string, + checksContextFile?: string, +): string { + const sections = [ + "Review the PR event and decide the right action via AOP EMIT records.", + "", + "## PR Context", + "", + `- **PR Type**: ${prType}`, + `- **Branch**: ${branch}`, + "", + "## Decision Vocabulary", + "", + "Choose ONE outcome and emit the matching EMIT records (see supervisor.md for full spec):", + "", + "- **fix-in-place** — actionable feedback, you edit files in the working tree (do NOT run git add/commit/push — the orchestrator does that), end with `EMIT verdict value: approved`", + "- **cancel** — user said /cancel or scope dead → `EMIT status-update target: self status: cancelled` + `EMIT verdict value: cancelled`", + "- **duplicate** — user said /duplicate → `EMIT status-update target: self status: duplicate` + `EMIT verdict value: rejected`", + "- **retry-as-new** — user clarified new scope → `EMIT child-item kind: task parent: self` + `EMIT status-update target: self status: rejected` + `EMIT verdict value: rejected`", + "- **escalate** — ambiguous/contradictory comments → `EMIT verdict value: approved` (no code changes, no status update)", + "", + "## Answering inline review comments (REQUIRED)", + "", + "Every `[Review # …]` entry below is an inline review thread you MUST answer with exactly one `EMIT comment-reply`, whichever outcome you pick:", + "", + "- Code change addresses it → `disposition: fixed`", + "- Comment is wrong / out of scope / already satisfied → `disposition: not-applicable`", + "", + "```", + "=== EMIT comment-reply ===", + "thread: 123456789 # copy the # from the [Review # …] line", + "disposition: fixed", + "note: Added the missing null guard in login().", + "=== END EMIT ===", + "```", + "", + "The `#` is the review comment id — copy it verbatim. Leave NO inline comment without a comment-reply; the note is what the reviewer reads. (Top-level PR comments and CI failures are answered by your verdict + summary, not by comment-reply.)", + "", + "## Rules", + "", + "- Emit one `EMIT comment-reply` for EVERY `[Review # …]` entry — fixed or not-applicable, always with a note", + "- NEVER write `---\\nstatus:` frontmatter directly — emit EMIT status-update instead", + "- NEVER run git add/commit/push — git is owned by the orchestrator; make edits and stop. A commit you make but do not push is discarded, and claiming a commit you didn't push does not make the change land.", + "- For cancel/duplicate/retry-as-new/escalate, make NO code edits", + "- NEVER return verdict: approved if CI is failing without fixing the failure (edit the code; the orchestrator commits it)", + "- For research PRs, only update findings/tasks files; for retrospective PRs, only `.operator/data/retrospectives/`", + ]; + if (checksContextFile) { + sections.push( + "", + "## CI Pipeline Context", + "", + `Detailed CI status (failing checks, annotations, log URLs) is in \`${checksContextFile}\`.`, + "Read this file with the Read tool BEFORE deciding on a fix when CI failures are referenced below.", + "Do NOT declare \"no changes needed\" on a failing PR without inspecting the failure details.", + ); + } + if (threadFile) { + sections.push( + "", + "## Discussion History", + "", + `Full PR conversation thread is available in \`${threadFile}\`.`, + "Read it if a new comment references earlier discussion or you need context.", + "Do NOT re-address old comments that were already handled (those marked responded in the bot footer).", + ); + } + sections.push( + "", + "## NEW Comments to Address", + "", + "These are the comments you MUST classify in this cycle:", + "", + newFeedback, + "", + "## Output", + "", + "End your output with AOP EMIT blocks. The orchestrator parses them. Anything outside EMIT blocks is captured as freeform analysis for the execution log.", + ); + return sections.join("\n"); +} diff --git a/engine/pipeline/composers/pr-feedback-supervisor-stage.test.ts b/engine/pipeline/composers/pr-feedback-supervisor-stage.test.ts index 6a325cb..41b2655 100644 --- a/engine/pipeline/composers/pr-feedback-supervisor-stage.test.ts +++ b/engine/pipeline/composers/pr-feedback-supervisor-stage.test.ts @@ -12,7 +12,6 @@ import { buildPrFeedbackSupervisorBuildPR, buildPrFeedbackSupervisorAfterAgent, buildPrFeedbackSupervisorSynthesizeAgentResult, - buildSupervisorTask, type PrFeedbackSupervisorHookDeps, } from "./pr-feedback-supervisor-stage.js"; @@ -160,29 +159,6 @@ function makeDeps(overrides: Partial = {}): PrFeed } describe("supervisor stage-logic", () => { - describe("buildSupervisorTask", () => { - it("includes PR coordinates and feedback", () => { - const task = buildSupervisorTask("task", "ai/tasks/T-0001", "user comment here", ""); - expect(task).toContain("ai/tasks/T-0001"); - expect(task).toContain("user comment here"); - expect(task).toContain("fix-in-place"); - expect(task).toContain("cancel"); - expect(task).toContain("retry-as-new"); - }); - - it("includes CI context file path when supplied", () => { - const task = buildSupervisorTask("task", "ai/tasks/T-0001", "fb", "", "/tmp/ci.md"); - expect(task).toContain("/tmp/ci.md"); - expect(task).toContain("CI Pipeline Context"); - }); - - it("includes thread file path when supplied", () => { - const task = buildSupervisorTask("task", "ai/tasks/T-0001", "fb", "/tmp/thread.md"); - expect(task).toContain("/tmp/thread.md"); - expect(task).toContain("Discussion History"); - }); - }); - describe("buildPrFeedbackSupervisorBeforeAgent", () => { it("transitions label to processing and returns processingPRs", async () => { const deps = makeDeps(); diff --git a/engine/pipeline/composers/pr-feedback-supervisor-stage.ts b/engine/pipeline/composers/pr-feedback-supervisor-stage.ts index 0484032..e90bc60 100644 --- a/engine/pipeline/composers/pr-feedback-supervisor-stage.ts +++ b/engine/pipeline/composers/pr-feedback-supervisor-stage.ts @@ -12,16 +12,17 @@ import type { PRManager } from "../../delivery/pr-manager.js"; import type { WorkspaceGit } from "../../infra/git.js"; import type { Logger } from "../../logging/logger.js"; import { resolveRole, buildRunInput } from "../../agents/roles.js"; -import { formatDebugRunLinkSuffix } from "../../delivery/vcs-helpers.js"; import type { StateContextVars } from "../../work-items/work-items.js"; import type { StageDef, StageInput, AgentResult, Verdict } from "../types.js"; import type { WorkspaceHandle } from "../primitives/workspace-scope.js"; import type { PrFeedbackPayload } from "../primitives/pr-feedback-selector.js"; import { writeChecksContextFile } from "../primitives/checks-context.js"; -import type { BotAttribution } from "../../delivery/bot-footer.js"; -import { applyAgentEvents } from "../primitives/aop-applier.js"; -import { applyThreadDispositions } from "./_shared/thread-dispositions.js"; -import { createScratchStore } from "./_shared/scratch.js"; +import { buildSupervisorTask } from "./_shared/supervisor-task.js"; +import { processSupervisorAfterAgent } from "./_shared/supervisor-after-agent.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./_shared/supervisor-scratch.js"; import { StageLogicError } from "./errors.js"; /** @@ -53,32 +54,6 @@ import { StageLogicError } from "./errors.js"; * canonical example. */ -interface PrFeedbackSupervisorScratch { - readonly prId: number; - readonly branch: string; - readonly prType: string; - readonly reviewAttempts: number; - readonly maxAttempts: number; - readonly limitReached: boolean; - readonly threadFile: string; - readonly newFeedback: string; - checksContextFile: string; - /** - * HEAD SHA captured at beforeAgent (post workspace checkout). Used in - * afterAgent to detect whether the supervisor agent committed during - * the run. `git.isClean()` alone returned `true` after a successful - * commit, leading the engine to misreport "No code changes" when the - * agent had in fact committed and pushed — the 2026-05-20 PR-887 - * regression. Empty string when capture failed (best-effort; the - * afterAgent comparison treats empty as "unknown, fall back to dirty - * check"). - */ - preAgentHeadSha: string; -} - -const prFeedbackSupervisorScratch = createScratchStore(); -const prKey = (prId: number): string => String(prId); - export interface PrFeedbackSupervisorHookDeps { readonly prManager: PRManager; readonly git: WorkspaceGit; @@ -133,17 +108,6 @@ async function computeReviewAttempts( } } -function inferKindFromBranch(branch: string, registry: KindRegistry): { kind: string; id: string } | null { - for (const kindDef of registry.all) { - const prefix = kindDef.branchPrefix.endsWith("/") ? kindDef.branchPrefix : `${kindDef.branchPrefix}/`; - if (branch.startsWith(prefix)) { - const id = branch.slice(prefix.length); - if (id) return { kind: kindDef.name, id }; - } - } - return null; -} - export function buildPrFeedbackSupervisorBeforeAgent(deps: PrFeedbackSupervisorHookDeps) { return async ( stage: StageDef, @@ -192,7 +156,7 @@ export function buildPrFeedbackSupervisorBeforeAgent(deps: PrFeedbackSupervisorH }); } - prFeedbackSupervisorScratch.set(ctx, prKey(payload.prId), { + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(payload.prId), { prId: payload.prId, branch: payload.branch, prType: payload.prType, reviewAttempts, maxAttempts, limitReached, threadFile, newFeedback: payload.newFeedback, @@ -227,7 +191,7 @@ export function buildPrFeedbackSupervisorSynthesizeAgentResult(deps: PrFeedbackS ctx: OperationContext, ): Promise => { const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prKey(payload.prId)); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); if (!scratch?.limitReached) return null; deps.log?.info(`${stage.name}: PR #${payload.prId} review cycle cap reached (${scratch.reviewAttempts}/${scratch.maxAttempts}) — skipping supervisor agent`, { stage: stage.name, prNumber: payload.prId, @@ -249,7 +213,7 @@ export function buildPrFeedbackSupervisorBuildRunInput(deps: PrFeedbackSuperviso ctx: OperationContext, ): Promise => { const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prKey(payload.prId)); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); if (!scratch) { throw new StageLogicError( "STAGE_SCRATCH_MISSING", @@ -303,7 +267,7 @@ export function buildPrFeedbackSupervisorBuildPR(_deps: PrFeedbackSupervisorHook ctx: OperationContext, ): Promise<{ title: string; body: string; commitMessage: string; onSuccess?: "in-review" | "ready-to-merge" | "none" }> => { const payload = payloadOf(stage.name, input); - prFeedbackSupervisorScratch.clear(ctx, prKey(payload.prId)); + prFeedbackSupervisorScratch.clear(ctx, prFeedbackSupervisorScratchKey(payload.prId)); return { title: `PR #${payload.prId} supervisor decision`, body: `Applied supervisor decision on PR #${payload.prId}.`, @@ -322,7 +286,7 @@ export function buildPrFeedbackSupervisorAfterAgent(deps: PrFeedbackSupervisorHo ctx: OperationContext, ): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> => { const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prKey(payload.prId)); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); if (!scratch) { throw new StageLogicError( "STAGE_SCRATCH_MISSING", @@ -330,196 +294,7 @@ export function buildPrFeedbackSupervisorAfterAgent(deps: PrFeedbackSupervisorHo ); } try { - const suffix = formatDebugRunLinkSuffix(deps.debug, deps.debugRunUrl); - const ciFailing = payload.checks.value === "failing"; - const nextAttribution: BotAttribution = { - responded: new Set(payload.respondedIds), - ciHead: payload.checks.headSha, - ciAttempt: ciFailing - ? { current: payload.ciAttempts + 1, max: payload.maxCiRetryAttempts } - : undefined, - }; - - if (scratch.limitReached) { - const msg = `⚠️ **Review cycle limit reached** — This PR has gone through ${scratch.reviewAttempts} review-fix cycles (limit: ${scratch.maxAttempts}). The supervisor was unable to resolve all feedback within the allowed iterations. Marking as failed — manual intervention required.${suffix}`; - await deps.prManager.postBotComment(payload.prId, msg, nextAttribution); - deps.log?.info(`${stage.name}: PR #${payload.prId} limit reached — verdict override failed`, { - stage: stage.name, prNumber: payload.prId, - reviewAttempts: scratch.reviewAttempts, maxAttempts: scratch.maxAttempts, - }); - return { - verdictOverride: "failed", - summaryOverride: `review cycle limit reached (${scratch.reviewAttempts}/${scratch.maxAttempts})`, - }; - } - - // 2026-05-13: removed defense-in-depth "approved + ciFailing → - // override to failed" check. The verifier (inside the agent chain - // when stage has reviewEnabled: true) is the authority on whether - // the supervisor's fix addresses CI. Defense-in-depth duplicated - // verifier and second-guessed it from a stale CI observation — - // CI was observed at cycle start (BEFORE supervisor committed via - // Bash) so it always looked "failing" even when the fix had just - // been pushed and CI re-run hadn't completed yet. The canonical - // case: supervisor correctly fixed all 47 backend test failures - // and 14 Copilot comments and - // committed/pushed, but the post-verifier check flipped to failed - // because checks.headSha was the pre-commit SHA. Per user guidance: - // "verify process should be able to detect commits and verify them - // even if committed — if OK act as usual; if wrong comment back to - // redo/fix. Committed work has no difference except technical to - // detect changes." Trust verifier — if its judgment is wrong, the - // next pr-feedback cycle picks the PR up with fresh CI data. - - const activeItem = inferKindFromBranch(payload.branch, deps.kindRegistry); - const applied = await applyAgentEvents( - agentResult.output, - { - stream: deps.agentEventStream, - source: deps.workItemSource, - registry: deps.kindRegistry, - log: deps.log, - }, - { - workItem: activeItem ? { id: activeItem.id, kind: activeItem.kind } : undefined, - }, - ctx, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} applied ${applied.applied.childItems.length} child-item(s), ${applied.applied.statusUpdates.length} status-update(s); applier verdict=${applied.verdict}`, { - stage: stage.name, prNumber: payload.prId, - applierVerdict: applied.verdict, - childItems: applied.applied.childItems.length, - statusUpdates: applied.applied.statusUpdates.length, - bodyUpdates: applied.applied.bodyUpdates.length, - applyErrors: applied.applyErrors.length, - }); - - // Answer + resolve inline review threads the supervisor disposed of - // this cycle. Runs on every agent path (fix-in-place, cancel, escalate, - // …) so no reviewer comment is left without a note. Bot threads (Copilot) - // are resolved; human threads get the note but stay open for the human. - if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { - await applyThreadDispositions({ - prId: payload.prId, - stage: stage.name, - commentReplies: applied.commentReplies, - reviewThreads: payload.reviewThreads, - freshReviewCommentIds: payload.freshReviewCommentIds, - prManager: deps.prManager, - log: deps.log, - }); - } - - // "Changes applied" detection — true when EITHER the workspace - // has uncommitted edits OR the agent advanced HEAD by committing - // directly via Bash. Pre-2026-05-20 the check only inspected the - // dirty flag; a post-commit clean tree was reported as "No code - // changes" even though the agent had committed (PR-887). The - // headSha comparison catches that path — falls back to the dirty - // check when the pre-agent SHA was not captured. Hoisted above the - // verdict branches so the stale-CI guard below sees it too. - const workspaceDirty = !(await deps.git.isClean()); - let postAgentHeadSha = ""; - try { - postAgentHeadSha = (await deps.git.headSha()).trim(); - } catch (err) { - deps.log?.warn(`${stage.name}: failed to capture post-agent HEAD SHA (falling back to dirty-only check)`, { - stage: stage.name, prNumber: payload.prId, - error: err instanceof Error ? err.message : String(err), - }); - } - const headAdvanced = scratch.preAgentHeadSha.length > 0 - && postAgentHeadSha.length > 0 - && scratch.preAgentHeadSha !== postAgentHeadSha; - const changesApplied = workspaceDirty || headAdvanced; - - // Stale-CI guard — completes the 2026-05-13 stale-CI fix (PR-1186). - // `payload.checks` is observed at cycle start, BEFORE the supervisor - // edits/commits, so a `failed` verdict resting on it — the verifier - // hard-rule "approved while CI failing", or the supervisor declining to - // approve over red CI — is judging a run the just-pushed fix already - // supersedes. When the supervisor DID push a fix in response to that - // failing CI, never latch terminal `ai:failed` on the stale observation: - // leave the PR in-review so fresh CI on the new commit decides and the - // next pr-feedback cycle re-evaluates (the recovery the 2026-05-13 note - // promised but the selector's `ai:failed` exclusion otherwise blocks). - // Bounded by the maxReviewAttempts cap in beforeAgent, so a genuinely - // unfixed failure still reaches `ai:failed` once the review budget is - // spent. Excludes `cancelled` (a human /cancel is a real terminal) and - // apply/parse errors (real contract violations, not CI staleness). - const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) - ? applied.verdict - : agentResult.verdict; - if (effectiveVerdict === "failed" && applied.applyErrors.length === 0 && changesApplied && ciFailing) { - await deps.prManager.postBotComment( - payload.prId, - `Applied review feedback. The failing check(s) were observed on the pre-fix commit (${payload.checks.headSha?.slice(0, 12) ?? "unknown"}); the pushed fix supersedes that run — leaving the PR in review so fresh CI on the new commit decides.${suffix}`, - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} failed verdict rested on stale pre-fix CI but the supervisor pushed a fix — downgrading to in-review for fresh-CI re-evaluation`, { - stage: stage.name, prNumber: payload.prId, - effectiveVerdict, ciHead: payload.checks.headSha, - changesApplied, workspaceDirty, headAdvanced, - }); - return { verdictOverride: "approved", summaryOverride: applied.summary || agentResult.summary }; - } - - if (applied.verdict !== "approved" || applied.applyErrors.length > 0) { - const reason = applied.summary || agentResult.summary || "supervisor decision"; - const detail = applied.applyErrors.length > 0 - ? `\n\nApply errors: ${applied.applyErrors.map((e) => `${e.code}: ${e.message}`).join("; ")}` - : ""; - await deps.prManager.postBotComment( - payload.prId, - `Supervisor decision: ${reason}.${detail}${suffix}`, - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} verdict=${applied.verdict} — posted terminal comment`, { - stage: stage.name, prNumber: payload.prId, - verdict: applied.verdict, reason, - }); - return { - verdictOverride: applied.verdict, - summaryOverride: applied.summary, - }; - } - - const effectiveSummary = applied.summary || agentResult.summary; - - if (changesApplied) { - await deps.prManager.postBotComment( - payload.prId, - `Applied review feedback.${suffix}`, - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place with changes — posted applied comment`, { - stage: stage.name, prNumber: payload.prId, - changesApplied: true, workspaceDirty, headAdvanced, - preAgentHeadSha: scratch.preAgentHeadSha.slice(0, 12), - postAgentHeadSha: postAgentHeadSha.slice(0, 12), - }); - } else { - // The engine asserts ONLY the fact it can observe (clean tree + - // unchanged HEAD ⇒ no code changes this cycle). It must NOT - // editorialize the REASON: a verdict=approved + no-changes run can - // mean "feedback genuinely already addressed" OR "supervisor chose - // escalate" (supervisor.md maps escalate → approved + no changes). - // The old hard-coded "considered the feedback already addressed" - // contradicted the agent's own reasoning on escalate cycles - // (PR #892, 2026-05-21). The WHY lives in the agent's - // reasoning block below — never in a fixed engine sentence. - const reasoning = (effectiveSummary ?? "").trim().slice(0, 1500); - const reasoningBlock = reasoning ? `\n\n${reasoning}` : ""; - await deps.prManager.postBotComment( - payload.prId, - `No code changes in this cycle.${reasoningBlock}\n\nReply on this PR if you disagree and I'll re-evaluate.${suffix}`, - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place no changes — posted no-changes comment, leaving at ai:in-review`, { - stage: stage.name, prNumber: payload.prId, changesApplied: false, - }); - } - return { summaryOverride: effectiveSummary }; + return await processSupervisorAfterAgent(deps, stage, payload, scratch, agentResult, ctx); } catch (err) { deps.log?.error(`${stage.name}: afterAgent failed for PR #${payload.prId}`, { stage: stage.name, prNumber: payload.prId, error: errorMessage(err), @@ -532,89 +307,3 @@ export function buildPrFeedbackSupervisorAfterAgent(deps: PrFeedbackSupervisorHo } }; } - -export function buildSupervisorTask( - prType: string, - branch: string, - newFeedback: string, - threadFile: string, - checksContextFile?: string, -): string { - const sections = [ - "Review the PR event and decide the right action via AOP EMIT records.", - "", - "## PR Context", - "", - `- **PR Type**: ${prType}`, - `- **Branch**: ${branch}`, - "", - "## Decision Vocabulary", - "", - "Choose ONE outcome and emit the matching EMIT records (see supervisor.md for full spec):", - "", - "- **fix-in-place** — actionable feedback, you edit files in the working tree (do NOT run git add/commit/push — the orchestrator does that), end with `EMIT verdict value: approved`", - "- **cancel** — user said /cancel or scope dead → `EMIT status-update target: self status: cancelled` + `EMIT verdict value: cancelled`", - "- **duplicate** — user said /duplicate → `EMIT status-update target: self status: duplicate` + `EMIT verdict value: rejected`", - "- **retry-as-new** — user clarified new scope → `EMIT child-item kind: task parent: self` + `EMIT status-update target: self status: rejected` + `EMIT verdict value: rejected`", - "- **escalate** — ambiguous/contradictory comments → `EMIT verdict value: approved` (no code changes, no status update)", - "", - "## Answering inline review comments (REQUIRED)", - "", - "Every `[Review # …]` entry below is an inline review thread you MUST answer with exactly one `EMIT comment-reply`, whichever outcome you pick:", - "", - "- Code change addresses it → `disposition: fixed`", - "- Comment is wrong / out of scope / already satisfied → `disposition: not-applicable`", - "", - "```", - "=== EMIT comment-reply ===", - "thread: 123456789 # copy the # from the [Review # …] line", - "disposition: fixed", - "note: Added the missing null guard in login().", - "=== END EMIT ===", - "```", - "", - "The `#` is the review comment id — copy it verbatim. Leave NO inline comment without a comment-reply; the note is what the reviewer reads. (Top-level PR comments and CI failures are answered by your verdict + summary, not by comment-reply.)", - "", - "## Rules", - "", - "- Emit one `EMIT comment-reply` for EVERY `[Review # …]` entry — fixed or not-applicable, always with a note", - "- NEVER write `---\\nstatus:` frontmatter directly — emit EMIT status-update instead", - "- NEVER run git add/commit/push — git is owned by the orchestrator; make edits and stop. A commit you make but do not push is discarded, and claiming a commit you didn't push does not make the change land.", - "- For cancel/duplicate/retry-as-new/escalate, make NO code edits", - "- NEVER return verdict: approved if CI is failing without fixing the failure (edit the code; the orchestrator commits it)", - "- For research PRs, only update findings/tasks files; for retrospective PRs, only `.operator/data/retrospectives/`", - ]; - if (checksContextFile) { - sections.push( - "", - "## CI Pipeline Context", - "", - `Detailed CI status (failing checks, annotations, log URLs) is in \`${checksContextFile}\`.`, - "Read this file with the Read tool BEFORE deciding on a fix when CI failures are referenced below.", - "Do NOT declare \"no changes needed\" on a failing PR without inspecting the failure details.", - ); - } - if (threadFile) { - sections.push( - "", - "## Discussion History", - "", - `Full PR conversation thread is available in \`${threadFile}\`.`, - "Read it if a new comment references earlier discussion or you need context.", - "Do NOT re-address old comments that were already handled (those marked responded in the bot footer).", - ); - } - sections.push( - "", - "## NEW Comments to Address", - "", - "These are the comments you MUST classify in this cycle:", - "", - newFeedback, - "", - "## Output", - "", - "End your output with AOP EMIT blocks. The orchestrator parses them. Anything outside EMIT blocks is captured as freeform analysis for the execution log.", - ); - return sections.join("\n"); -} From 89e182f28a90ea86e10f2e2c64b0a496bc115956 Mon Sep 17 00:00:00 2001 From: Operator Bot Date: Sat, 11 Jul 2026 12:01:05 +0100 Subject: [PATCH 2/3] Applied supervisor decision on PR #38 --- .../_shared/supervisor-after-agent-deps.ts | 15 + .../_shared/supervisor-after-agent-hook.ts | 44 +++ .../_shared/supervisor-after-agent.ts | 213 +------------ .../composers/_shared/supervisor-aop-apply.ts | 54 ++++ .../_shared/supervisor-before-agent.ts | 92 ++++++ .../_shared/supervisor-branch-item.ts | 12 + .../composers/_shared/supervisor-build-pr.ts | 25 ++ .../_shared/supervisor-build-run-input.ts | 67 ++++ .../_shared/supervisor-change-detection.ts | 40 +++ .../composers/_shared/supervisor-payload.ts | 14 + .../_shared/supervisor-stage-deps.ts | 28 ++ .../_shared/supervisor-synthesize-agent.ts | 32 ++ .../_shared/supervisor-verdict-routing.ts | 93 ++++++ .../composers/pr-feedback-supervisor-stage.ts | 292 +----------------- 14 files changed, 537 insertions(+), 484 deletions(-) create mode 100644 engine/pipeline/composers/_shared/supervisor-after-agent-deps.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-after-agent-hook.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-aop-apply.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-before-agent.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-branch-item.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-build-pr.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-build-run-input.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-change-detection.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-payload.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-stage-deps.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-verdict-routing.ts diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent-deps.ts b/engine/pipeline/composers/_shared/supervisor-after-agent-deps.ts new file mode 100644 index 0000000..9771944 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-after-agent-deps.ts @@ -0,0 +1,15 @@ +import type { KindRegistry, WorkItemSource, AgentEventStream } from "@operator/core"; +import type { PRManager } from "../../../delivery/pr-manager.js"; +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { Logger } from "../../../logging/logger.js"; + +export interface SupervisorAfterAgentDeps { + readonly prManager: PRManager; + readonly git: WorkspaceGit; + readonly kindRegistry: KindRegistry; + readonly workItemSource: WorkItemSource; + readonly agentEventStream: AgentEventStream; + readonly log?: Logger; + readonly debug?: boolean; + readonly debugRunUrl?: string; +} diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent-hook.ts b/engine/pipeline/composers/_shared/supervisor-after-agent-hook.ts new file mode 100644 index 0000000..3aa8cf8 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-after-agent-hook.ts @@ -0,0 +1,44 @@ +import { unlink } from "node:fs/promises"; +import type { OperationContext } from "@operator/core"; +import { errorMessage } from "@operator/core"; +import type { StageDef, StageInput, AgentResult, Verdict } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; +import { processSupervisorAfterAgent } from "./supervisor-after-agent.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { payloadOf } from "./supervisor-payload.js"; +import { StageLogicError } from "../errors.js"; + +export function buildPrFeedbackSupervisorAfterAgent(deps: PrFeedbackSupervisorHookDeps) { + return async ( + stage: StageDef, + input: StageInput, + agentResult: AgentResult, + _workspace: WorkspaceHandle, + ctx: OperationContext, + ): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> => { + const payload = payloadOf(stage.name, input); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); + if (!scratch) { + throw new StageLogicError( + "STAGE_SCRATCH_MISSING", + `${stage.name} afterAgent: missing scratch for PR #${payload.prId} — beforeAgent not run`, + ); + } + try { + return await processSupervisorAfterAgent(deps, stage, payload, scratch, agentResult, ctx); + } catch (err) { + deps.log?.error(`${stage.name}: afterAgent failed for PR #${payload.prId}`, { + stage: stage.name, prNumber: payload.prId, error: errorMessage(err), + }); + throw err; + } finally { + if (scratch.threadFile) { + await unlink(scratch.threadFile).catch(() => { /* best-effort cleanup */ }); + } + } + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent.ts b/engine/pipeline/composers/_shared/supervisor-after-agent.ts index 78ee54e..b32882f 100644 --- a/engine/pipeline/composers/_shared/supervisor-after-agent.ts +++ b/engine/pipeline/composers/_shared/supervisor-after-agent.ts @@ -1,45 +1,16 @@ -import type { - OperationContext, KindRegistry, WorkItemSource, AgentEventStream, -} from "@operator/core"; -import type { PRManager } from "../../../delivery/pr-manager.js"; -import type { WorkspaceGit } from "../../../infra/git.js"; -import type { Logger } from "../../../logging/logger.js"; +import type { OperationContext } from "@operator/core"; import { formatDebugRunLinkSuffix } from "../../../delivery/vcs-helpers.js"; import type { BotAttribution } from "../../../delivery/bot-footer.js"; import type { StageDef, AgentResult, Verdict } from "../../types.js"; import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; -import { applyAgentEvents } from "../../primitives/aop-applier.js"; -import { applyThreadDispositions } from "./thread-dispositions.js"; +import { formatReviewLimitReachedMessage } from "./supervisor-bot-messages.js"; import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; -import { - formatAppliedReviewFeedbackMessage, - formatNoCodeChangesMessage, - formatReviewLimitReachedMessage, - formatStaleCiFixMessage, - formatSupervisorTerminalMessage, -} from "./supervisor-bot-messages.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; +import { applySupervisorAgentEvents } from "./supervisor-aop-apply.js"; +import { detectSupervisorChanges } from "./supervisor-change-detection.js"; +import { routeSupervisorVerdict } from "./supervisor-verdict-routing.js"; -export interface SupervisorAfterAgentDeps { - readonly prManager: PRManager; - readonly git: WorkspaceGit; - readonly kindRegistry: KindRegistry; - readonly workItemSource: WorkItemSource; - readonly agentEventStream: AgentEventStream; - readonly log?: Logger; - readonly debug?: boolean; - readonly debugRunUrl?: string; -} - -function inferKindFromBranch(branch: string, registry: KindRegistry): { kind: string; id: string } | null { - for (const kindDef of registry.all) { - const prefix = kindDef.branchPrefix.endsWith("/") ? kindDef.branchPrefix : `${kindDef.branchPrefix}/`; - if (branch.startsWith(prefix)) { - const id = branch.slice(prefix.length); - if (id) return { kind: kindDef.name, id }; - } - } - return null; -} +export type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; export async function processSupervisorAfterAgent( deps: SupervisorAfterAgentDeps, @@ -72,169 +43,11 @@ export async function processSupervisorAfterAgent( }; } - // 2026-05-13: removed defense-in-depth "approved + ciFailing → - // override to failed" check. The verifier (inside the agent chain - // when stage has reviewEnabled: true) is the authority on whether - // the supervisor's fix addresses CI. Defense-in-depth duplicated - // verifier and second-guessed it from a stale CI observation — - // CI was observed at cycle start (BEFORE supervisor committed via - // Bash) so it always looked "failing" even when the fix had just - // been pushed and CI re-run hadn't completed yet. The canonical - // case: supervisor correctly fixed all 47 backend test failures - // and 14 Copilot comments and - // committed/pushed, but the post-verifier check flipped to failed - // because checks.headSha was the pre-commit SHA. Per user guidance: - // "verify process should be able to detect commits and verify them - // even if committed — if OK act as usual; if wrong comment back to - // redo/fix. Committed work has no difference except technical to - // detect changes." Trust verifier — if its judgment is wrong, the - // next pr-feedback cycle picks the PR up with fresh CI data. - - const activeItem = inferKindFromBranch(payload.branch, deps.kindRegistry); - const applied = await applyAgentEvents( - agentResult.output, - { - stream: deps.agentEventStream, - source: deps.workItemSource, - registry: deps.kindRegistry, - log: deps.log, - }, - { - workItem: activeItem ? { id: activeItem.id, kind: activeItem.kind } : undefined, - }, - ctx, + const applied = await applySupervisorAgentEvents( + deps, stage, payload, agentResult.output, ctx, deps.log, + ); + const changes = await detectSupervisorChanges(deps.git, scratch, stage, payload, deps.log); + return routeSupervisorVerdict( + deps, stage, payload, scratch, agentResult, applied, changes, nextAttribution, ciFailing, ); - deps.log?.info(`${stage.name}: PR #${payload.prId} applied ${applied.applied.childItems.length} child-item(s), ${applied.applied.statusUpdates.length} status-update(s); applier verdict=${applied.verdict}`, { - stage: stage.name, prNumber: payload.prId, - applierVerdict: applied.verdict, - childItems: applied.applied.childItems.length, - statusUpdates: applied.applied.statusUpdates.length, - bodyUpdates: applied.applied.bodyUpdates.length, - applyErrors: applied.applyErrors.length, - }); - - // Answer + resolve inline review threads the supervisor disposed of - // this cycle. Runs on every agent path (fix-in-place, cancel, escalate, - // …) so no reviewer comment is left without a note. Bot threads (Copilot) - // are resolved; human threads get the note but stay open for the human. - if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { - await applyThreadDispositions({ - prId: payload.prId, - stage: stage.name, - commentReplies: applied.commentReplies, - reviewThreads: payload.reviewThreads, - freshReviewCommentIds: payload.freshReviewCommentIds, - prManager: deps.prManager, - log: deps.log, - }); - } - - // "Changes applied" detection — true when EITHER the workspace - // has uncommitted edits OR the agent advanced HEAD by committing - // directly via Bash. Pre-2026-05-20 the check only inspected the - // dirty flag; a post-commit clean tree was reported as "No code - // changes" even though the agent had committed (PR-887). The - // headSha comparison catches that path — falls back to the dirty - // check when the pre-agent SHA was not captured. Hoisted above the - // verdict branches so the stale-CI guard below sees it too. - const workspaceDirty = !(await deps.git.isClean()); - let postAgentHeadSha = ""; - try { - postAgentHeadSha = (await deps.git.headSha()).trim(); - } catch (err) { - deps.log?.warn(`${stage.name}: failed to capture post-agent HEAD SHA (falling back to dirty-only check)`, { - stage: stage.name, prNumber: payload.prId, - error: err instanceof Error ? err.message : String(err), - }); - } - const headAdvanced = scratch.preAgentHeadSha.length > 0 - && postAgentHeadSha.length > 0 - && scratch.preAgentHeadSha !== postAgentHeadSha; - const changesApplied = workspaceDirty || headAdvanced; - - // Stale-CI guard — completes the 2026-05-13 stale-CI fix (PR-1186). - // `payload.checks` is observed at cycle start, BEFORE the supervisor - // edits/commits, so a `failed` verdict resting on it — the verifier - // hard-rule "approved while CI failing", or the supervisor declining to - // approve over red CI — is judging a run the just-pushed fix already - // supersedes. When the supervisor DID push a fix in response to that - // failing CI, never latch terminal `ai:failed` on the stale observation: - // leave the PR in-review so fresh CI on the new commit decides and the - // next pr-feedback cycle re-evaluates (the recovery the 2026-05-13 note - // promised but the selector's `ai:failed` exclusion otherwise blocks). - // Bounded by the maxReviewAttempts cap in beforeAgent, so a genuinely - // unfixed failure still reaches `ai:failed` once the review budget is - // spent. Excludes `cancelled` (a human /cancel is a real terminal) and - // apply/parse errors (real contract violations, not CI staleness). - const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) - ? applied.verdict - : agentResult.verdict; - if (effectiveVerdict === "failed" && applied.applyErrors.length === 0 && changesApplied && ciFailing) { - await deps.prManager.postBotComment( - payload.prId, - formatStaleCiFixMessage(payload.checks.headSha, suffix), - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} failed verdict rested on stale pre-fix CI but the supervisor pushed a fix — downgrading to in-review for fresh-CI re-evaluation`, { - stage: stage.name, prNumber: payload.prId, - effectiveVerdict, ciHead: payload.checks.headSha, - changesApplied, workspaceDirty, headAdvanced, - }); - return { verdictOverride: "approved", summaryOverride: applied.summary || agentResult.summary }; - } - - if (applied.verdict !== "approved" || applied.applyErrors.length > 0) { - const reason = applied.summary || agentResult.summary || "supervisor decision"; - const detail = applied.applyErrors.length > 0 - ? `\n\nApply errors: ${applied.applyErrors.map((e) => `${e.code}: ${e.message}`).join("; ")}` - : ""; - await deps.prManager.postBotComment( - payload.prId, - formatSupervisorTerminalMessage(reason, detail, suffix), - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} verdict=${applied.verdict} — posted terminal comment`, { - stage: stage.name, prNumber: payload.prId, - verdict: applied.verdict, reason, - }); - return { - verdictOverride: applied.verdict, - summaryOverride: applied.summary, - }; - } - - const effectiveSummary = applied.summary || agentResult.summary; - - if (changesApplied) { - await deps.prManager.postBotComment( - payload.prId, - formatAppliedReviewFeedbackMessage(suffix), - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place with changes — posted applied comment`, { - stage: stage.name, prNumber: payload.prId, - changesApplied: true, workspaceDirty, headAdvanced, - preAgentHeadSha: scratch.preAgentHeadSha.slice(0, 12), - postAgentHeadSha: postAgentHeadSha.slice(0, 12), - }); - } else { - // The engine asserts ONLY the fact it can observe (clean tree + - // unchanged HEAD ⇒ no code changes this cycle). It must NOT - // editorialize the REASON: a verdict=approved + no-changes run can - // mean "feedback genuinely already addressed" OR "supervisor chose - // escalate" (supervisor.md maps escalate → approved + no changes). - // The old hard-coded "considered the feedback already addressed" - // contradicted the agent's own reasoning on escalate cycles - // (PR #892, 2026-05-21). The WHY lives in the agent's - // reasoning block below — never in a fixed engine sentence. - await deps.prManager.postBotComment( - payload.prId, - formatNoCodeChangesMessage(effectiveSummary ?? "", suffix), - nextAttribution, - ); - deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place no changes — posted no-changes comment, leaving at ai:in-review`, { - stage: stage.name, prNumber: payload.prId, changesApplied: false, - }); - } - return { summaryOverride: effectiveSummary }; } diff --git a/engine/pipeline/composers/_shared/supervisor-aop-apply.ts b/engine/pipeline/composers/_shared/supervisor-aop-apply.ts new file mode 100644 index 0000000..ec812c5 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-aop-apply.ts @@ -0,0 +1,54 @@ +import type { OperationContext } from "@operator/core"; +import type { Logger } from "../../../logging/logger.js"; +import type { StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import { applyAgentEvents, type AopApplyResult } from "../../primitives/aop-applier.js"; +import { applyThreadDispositions } from "./thread-dispositions.js"; +import { inferKindFromBranch } from "./supervisor-branch-item.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; + +export async function applySupervisorAgentEvents( + deps: SupervisorAfterAgentDeps, + stage: StageDef, + payload: PrFeedbackPayload, + agentOutput: string, + ctx: OperationContext, + log?: Logger, +): Promise { + const activeItem = inferKindFromBranch(payload.branch, deps.kindRegistry); + const applied = await applyAgentEvents( + agentOutput, + { + stream: deps.agentEventStream, + source: deps.workItemSource, + registry: deps.kindRegistry, + log: deps.log, + }, + { + workItem: activeItem ? { id: activeItem.id, kind: activeItem.kind } : undefined, + }, + ctx, + ); + log?.info(`${stage.name}: PR #${payload.prId} applied ${applied.applied.childItems.length} child-item(s), ${applied.applied.statusUpdates.length} status-update(s); applier verdict=${applied.verdict}`, { + stage: stage.name, prNumber: payload.prId, + applierVerdict: applied.verdict, + childItems: applied.applied.childItems.length, + statusUpdates: applied.applied.statusUpdates.length, + bodyUpdates: applied.applied.bodyUpdates.length, + applyErrors: applied.applyErrors.length, + }); + + if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { + await applyThreadDispositions({ + prId: payload.prId, + stage: stage.name, + commentReplies: applied.commentReplies, + reviewThreads: payload.reviewThreads, + freshReviewCommentIds: payload.freshReviewCommentIds, + prManager: deps.prManager, + log: deps.log, + }); + } + + return applied; +} diff --git a/engine/pipeline/composers/_shared/supervisor-before-agent.ts b/engine/pipeline/composers/_shared/supervisor-before-agent.ts new file mode 100644 index 0000000..20cc776 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-before-agent.ts @@ -0,0 +1,92 @@ +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import type { OperationContext } from "@operator/core"; +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { Logger } from "../../../logging/logger.js"; +import type { StageDef, StageInput } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { payloadOf } from "./supervisor-payload.js"; + +async function computeReviewAttempts( + git: WorkspaceGit, + baseBranch: string, + prType: string, + stageName: string, + log: Logger | undefined, +): Promise { + try { + const total = await git.commitCount(baseBranch); + const initial = prType === "task" ? 2 : 1; + return Math.max(0, total - initial); + } catch (err) { + log?.warn(`${stageName}: commitCount failed (defaulting attempts to 0)`, { + stage: stageName, baseBranch, prType, + error: err instanceof Error ? err.message : String(err), + }); + return 0; + } +} + +export function buildPrFeedbackSupervisorBeforeAgent(deps: PrFeedbackSupervisorHookDeps) { + return async ( + stage: StageDef, + input: StageInput, + workspace: WorkspaceHandle, + ctx: OperationContext, + ): Promise<{ processingPRs?: readonly number[] } | void> => { + const payload = payloadOf(stage.name, input); + const reviewAttempts = await computeReviewAttempts( + deps.git, workspace.baseBranch, payload.prType, stage.name, deps.log, + ); + const maxAttempts = deps.defaults.limits.maxReviewAttempts; + const limitReached = reviewAttempts >= maxAttempts; + + deps.log?.info(`${stage.name}: PR #${payload.prId} (${payload.prType}) attempts ${reviewAttempts}/${maxAttempts}`, { + stage: stage.name, prNumber: payload.prId, prType: payload.prType, + reviewAttempts, maxAttempts, limitReached, + }); + + let threadFile = ""; + if (!limitReached) { + await deps.prManager.markProcessing(payload.prId); + deps.log?.info(`${stage.name}: PR #${payload.prId} label ai:pending → ai:processing`, { + stage: stage.name, prNumber: payload.prId, + }); + + if (payload.fullThread) { + threadFile = join( + tmpdir(), + `operator-pr-thread-${payload.prId}-${Date.now()}-${Math.random().toString(36).slice(2)}.md`, + ); + await writeFile(threadFile, `# PR #${payload.prId} Discussion Thread\n\n${payload.fullThread}\n`, "utf-8"); + } + } + + let preAgentHeadSha = ""; + try { + preAgentHeadSha = (await deps.git.headSha()).trim(); + } catch (err) { + deps.log?.warn(`${stage.name}: failed to capture pre-agent HEAD SHA (non-fatal)`, { + stage: stage.name, prNumber: payload.prId, + error: err instanceof Error ? err.message : String(err), + }); + } + + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(payload.prId), { + prId: payload.prId, branch: payload.branch, prType: payload.prType, + reviewAttempts, maxAttempts, limitReached, threadFile, + newFeedback: payload.newFeedback, + checksContextFile: "", + preAgentHeadSha, + }); + + if (limitReached) return; + return { processingPRs: [payload.prId] }; + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-branch-item.ts b/engine/pipeline/composers/_shared/supervisor-branch-item.ts new file mode 100644 index 0000000..0df0caa --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-branch-item.ts @@ -0,0 +1,12 @@ +import type { KindRegistry } from "@operator/core"; + +export function inferKindFromBranch(branch: string, registry: KindRegistry): { kind: string; id: string } | null { + for (const kindDef of registry.all) { + const prefix = kindDef.branchPrefix.endsWith("/") ? kindDef.branchPrefix : `${kindDef.branchPrefix}/`; + if (branch.startsWith(prefix)) { + const id = branch.slice(prefix.length); + if (id) return { kind: kindDef.name, id }; + } + } + return null; +} diff --git a/engine/pipeline/composers/_shared/supervisor-build-pr.ts b/engine/pipeline/composers/_shared/supervisor-build-pr.ts new file mode 100644 index 0000000..1f916a1 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-build-pr.ts @@ -0,0 +1,25 @@ +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput } from "../../types.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { payloadOf } from "./supervisor-payload.js"; + +export function buildPrFeedbackSupervisorBuildPR(_deps: PrFeedbackSupervisorHookDeps) { + return async ( + stage: StageDef, + input: StageInput, + ctx: OperationContext, + ): Promise<{ title: string; body: string; commitMessage: string; onSuccess?: "in-review" | "ready-to-merge" | "none" }> => { + const payload = payloadOf(stage.name, input); + prFeedbackSupervisorScratch.clear(ctx, prFeedbackSupervisorScratchKey(payload.prId)); + return { + title: `PR #${payload.prId} supervisor decision`, + body: `Applied supervisor decision on PR #${payload.prId}.`, + commitMessage: `Applied supervisor decision on PR #${payload.prId}`, + onSuccess: "in-review", + }; + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-build-run-input.ts b/engine/pipeline/composers/_shared/supervisor-build-run-input.ts new file mode 100644 index 0000000..dc920a3 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-build-run-input.ts @@ -0,0 +1,67 @@ +import type { OperationContext } from "@operator/core"; +import type { AgentRunInput } from "../../../agents/runtime.js"; +import { resolveRole, buildRunInput } from "../../../agents/roles.js"; +import type { StageDef, StageInput } from "../../types.js"; +import { writeChecksContextFile } from "../../primitives/checks-context.js"; +import { buildSupervisorTask } from "./supervisor-task.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { payloadOf } from "./supervisor-payload.js"; +import { StageLogicError } from "../errors.js"; + +export function buildPrFeedbackSupervisorBuildRunInput(deps: PrFeedbackSupervisorHookDeps) { + return async ( + stage: StageDef, + input: StageInput, + ctx: OperationContext, + ): Promise => { + const payload = payloadOf(stage.name, input); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); + if (!scratch) { + throw new StageLogicError( + "STAGE_SCRATCH_MISSING", + `${stage.name} buildRunInput: missing scratch for PR #${payload.prId} — beforeAgent not run`, + ); + } + const role = resolveRole(deps.agentsConfig, deps.agentRole); + const reviewCriteria = role.review + ? await deps.promptSource.loadChain(`verifier/${deps.verifierTopic}`) + : undefined; + + if (payload.checks.value === "failing" || payload.checks.value === "pending") { + try { + scratch.checksContextFile = await writeChecksContextFile({ + observation: payload.checks, + prNumber: payload.prId, + branch: payload.branch, + }); + } catch (err) { + deps.log?.warn(`${stage.name}: writeChecksContextFile failed (non-fatal)`, { + stage: stage.name, prNumber: payload.prId, + error: err instanceof Error ? err.message : String(err), + }); + } + } + const taskContent = buildSupervisorTask( + payload.prType, payload.branch, scratch.newFeedback, + scratch.threadFile, scratch.checksContextFile || undefined, + ); + return buildRunInput( + role, + { + promptSource: deps.promptSource, + automationDir: deps.automationDir, + vars: { PR_NUMBER: String(payload.prId), PR_TYPE: payload.prType, ...deps.stateVars }, + }, + { + taskContent, + cwd: deps.workspacePath, + maxRetries: 1, + reviewCriteria, + }, + ); + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-change-detection.ts b/engine/pipeline/composers/_shared/supervisor-change-detection.ts new file mode 100644 index 0000000..eaad3ed --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-change-detection.ts @@ -0,0 +1,40 @@ +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { Logger } from "../../../logging/logger.js"; +import type { StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; + +export interface SupervisorChanges { + readonly workspaceDirty: boolean; + readonly headAdvanced: boolean; + readonly changesApplied: boolean; + readonly postAgentHeadSha: string; +} + +export async function detectSupervisorChanges( + git: WorkspaceGit, + scratch: PrFeedbackSupervisorScratch, + stage: StageDef, + payload: PrFeedbackPayload, + log?: Logger, +): Promise { + const workspaceDirty = !(await git.isClean()); + let postAgentHeadSha = ""; + try { + postAgentHeadSha = (await git.headSha()).trim(); + } catch (err) { + log?.warn(`${stage.name}: failed to capture post-agent HEAD SHA (falling back to dirty-only check)`, { + stage: stage.name, prNumber: payload.prId, + error: err instanceof Error ? err.message : String(err), + }); + } + const headAdvanced = scratch.preAgentHeadSha.length > 0 + && postAgentHeadSha.length > 0 + && scratch.preAgentHeadSha !== postAgentHeadSha; + return { + workspaceDirty, + headAdvanced, + changesApplied: workspaceDirty || headAdvanced, + postAgentHeadSha, + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-payload.ts b/engine/pipeline/composers/_shared/supervisor-payload.ts new file mode 100644 index 0000000..f709e74 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-payload.ts @@ -0,0 +1,14 @@ +import type { StageInput } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import { StageLogicError } from "../errors.js"; + +export function payloadOf(stageName: string, input: StageInput): PrFeedbackPayload { + const data = input.data as PrFeedbackPayload | undefined; + if (!data || typeof data.prId !== "number") { + throw new StageLogicError( + "INVALID_STAGE_INPUT", + `${stageName} hook: stage input missing PrFeedbackPayload (scopeKey: ${input.scopeKey})`, + ); + } + return data; +} diff --git a/engine/pipeline/composers/_shared/supervisor-stage-deps.ts b/engine/pipeline/composers/_shared/supervisor-stage-deps.ts new file mode 100644 index 0000000..c76a8c9 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-stage-deps.ts @@ -0,0 +1,28 @@ +import type { + DefaultsConfig, PromptSource, KindRegistry, WorkItemSource, AgentEventStream, AgentRoleName, +} from "@operator/core"; +import type { AgentsFile } from "../../../config/schemas.js"; +import type { PRManager } from "../../../delivery/pr-manager.js"; +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { Logger } from "../../../logging/logger.js"; +import type { StateContextVars } from "../../../work-items/work-items.js"; + +/** Dependencies + stage-shape parameters for the PR-feedback supervisor composer. */ +export interface PrFeedbackSupervisorHookDeps { + readonly prManager: PRManager; + readonly git: WorkspaceGit; + readonly agentsConfig: AgentsFile; + readonly promptSource: PromptSource; + readonly defaults: DefaultsConfig; + readonly automationDir: string; + readonly workspacePath: string; + readonly kindRegistry: KindRegistry; + readonly workItemSource: WorkItemSource; + readonly agentEventStream: AgentEventStream; + readonly stateVars?: StateContextVars; + readonly log?: Logger; + readonly debug?: boolean; + readonly debugRunUrl?: string; + readonly agentRole: AgentRoleName; + readonly verifierTopic: string; +} diff --git a/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts new file mode 100644 index 0000000..53b5c0d --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts @@ -0,0 +1,32 @@ +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput, AgentResult } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { payloadOf } from "./supervisor-payload.js"; + +export function buildPrFeedbackSupervisorSynthesizeAgentResult(deps: PrFeedbackSupervisorHookDeps) { + return async ( + stage: StageDef, + input: StageInput, + _workspace: WorkspaceHandle, + ctx: OperationContext, + ): Promise => { + const payload = payloadOf(stage.name, input); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); + if (!scratch?.limitReached) return null; + deps.log?.info(`${stage.name}: PR #${payload.prId} review cycle cap reached (${scratch.reviewAttempts}/${scratch.maxAttempts}) — skipping supervisor agent`, { + stage: stage.name, prNumber: payload.prId, + reviewAttempts: scratch.reviewAttempts, maxAttempts: scratch.maxAttempts, + }); + return { + verdict: "failed", + output: "", + attempts: 0, + summary: `review cycle limit reached (${scratch.reviewAttempts}/${scratch.maxAttempts})`, + }; + }; +} diff --git a/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts b/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts new file mode 100644 index 0000000..e8f2267 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts @@ -0,0 +1,93 @@ +import type { AgentResult, StageDef, Verdict } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { AopApplyResult } from "../../primitives/aop-applier.js"; +import { formatDebugRunLinkSuffix } from "../../../delivery/vcs-helpers.js"; +import type { BotAttribution } from "../../../delivery/bot-footer.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; +import type { SupervisorChanges } from "./supervisor-change-detection.js"; +import { + formatAppliedReviewFeedbackMessage, + formatNoCodeChangesMessage, + formatStaleCiFixMessage, + formatSupervisorTerminalMessage, +} from "./supervisor-bot-messages.js"; + +export async function routeSupervisorVerdict( + deps: SupervisorAfterAgentDeps, + stage: StageDef, + payload: PrFeedbackPayload, + scratch: PrFeedbackSupervisorScratch, + agentResult: AgentResult, + applied: AopApplyResult, + changes: SupervisorChanges, + nextAttribution: BotAttribution, + ciFailing: boolean, +): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> { + const suffix = formatDebugRunLinkSuffix(deps.debug, deps.debugRunUrl); + const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) + ? applied.verdict + : agentResult.verdict; + + if (effectiveVerdict === "failed" && applied.applyErrors.length === 0 && changes.changesApplied && ciFailing) { + await deps.prManager.postBotComment( + payload.prId, + formatStaleCiFixMessage(payload.checks.headSha, suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} failed verdict rested on stale pre-fix CI but the supervisor pushed a fix — downgrading to in-review for fresh-CI re-evaluation`, { + stage: stage.name, prNumber: payload.prId, + effectiveVerdict, ciHead: payload.checks.headSha, + changesApplied: changes.changesApplied, + workspaceDirty: changes.workspaceDirty, headAdvanced: changes.headAdvanced, + }); + return { verdictOverride: "approved", summaryOverride: applied.summary || agentResult.summary }; + } + + if (applied.verdict !== "approved" || applied.applyErrors.length > 0) { + const reason = applied.summary || agentResult.summary || "supervisor decision"; + const detail = applied.applyErrors.length > 0 + ? `\n\nApply errors: ${applied.applyErrors.map((e) => `${e.code}: ${e.message}`).join("; ")}` + : ""; + await deps.prManager.postBotComment( + payload.prId, + formatSupervisorTerminalMessage(reason, detail, suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} verdict=${applied.verdict} — posted terminal comment`, { + stage: stage.name, prNumber: payload.prId, + verdict: applied.verdict, reason, + }); + return { + verdictOverride: applied.verdict, + summaryOverride: applied.summary, + }; + } + + const effectiveSummary = applied.summary || agentResult.summary; + + if (changes.changesApplied) { + await deps.prManager.postBotComment( + payload.prId, + formatAppliedReviewFeedbackMessage(suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place with changes — posted applied comment`, { + stage: stage.name, prNumber: payload.prId, + changesApplied: true, + workspaceDirty: changes.workspaceDirty, headAdvanced: changes.headAdvanced, + preAgentHeadSha: scratch.preAgentHeadSha.slice(0, 12), + postAgentHeadSha: changes.postAgentHeadSha.slice(0, 12), + }); + } else { + await deps.prManager.postBotComment( + payload.prId, + formatNoCodeChangesMessage(effectiveSummary ?? "", suffix), + nextAttribution, + ); + deps.log?.info(`${stage.name}: PR #${payload.prId} fix-in-place no changes — posted no-changes comment, leaving at ai:in-review`, { + stage: stage.name, prNumber: payload.prId, changesApplied: false, + }); + } + return { summaryOverride: effectiveSummary }; +} diff --git a/engine/pipeline/composers/pr-feedback-supervisor-stage.ts b/engine/pipeline/composers/pr-feedback-supervisor-stage.ts index e90bc60..9b7f6b8 100644 --- a/engine/pipeline/composers/pr-feedback-supervisor-stage.ts +++ b/engine/pipeline/composers/pr-feedback-supervisor-stage.ts @@ -1,30 +1,3 @@ -import { writeFile, unlink } from "node:fs/promises"; -import { join } from "node:path"; -import { tmpdir } from "node:os"; -import type { - OperationContext, DefaultsConfig, PromptSource, - KindRegistry, WorkItemSource, AgentEventStream, AgentRoleName, -} from "@operator/core"; -import { errorMessage } from "@operator/core"; -import type { AgentRunInput } from "../../agents/runtime.js"; -import type { AgentsFile } from "../../config/schemas.js"; -import type { PRManager } from "../../delivery/pr-manager.js"; -import type { WorkspaceGit } from "../../infra/git.js"; -import type { Logger } from "../../logging/logger.js"; -import { resolveRole, buildRunInput } from "../../agents/roles.js"; -import type { StateContextVars } from "../../work-items/work-items.js"; -import type { StageDef, StageInput, AgentResult, Verdict } from "../types.js"; -import type { WorkspaceHandle } from "../primitives/workspace-scope.js"; -import type { PrFeedbackPayload } from "../primitives/pr-feedback-selector.js"; -import { writeChecksContextFile } from "../primitives/checks-context.js"; -import { buildSupervisorTask } from "./_shared/supervisor-task.js"; -import { processSupervisorAfterAgent } from "./_shared/supervisor-after-agent.js"; -import { - prFeedbackSupervisorScratch, - prFeedbackSupervisorScratchKey, -} from "./_shared/supervisor-scratch.js"; -import { StageLogicError } from "./errors.js"; - /** * Generic stage composer for the "PR feedback supervisor" pattern. * @@ -48,262 +21,13 @@ import { StageLogicError } from "./errors.js"; * `.operator/data/*.md` frontmatter — and it goes through * `FileBackedWorkItemSource.updateStatus`. * - * The composer is consumed by any stage whose pattern is "supervisor - * LLM router over PR events with AOP-driven decisions". A - * `pr-review` stage handling feedback on AI-authored PRs is the - * canonical example. + * Hook implementations live under `./_shared/`; this module re-exports + * the public composer surface consumed by `stage-handlers.ts`. */ -export interface PrFeedbackSupervisorHookDeps { - readonly prManager: PRManager; - readonly git: WorkspaceGit; - readonly agentsConfig: AgentsFile; - readonly promptSource: PromptSource; - readonly defaults: DefaultsConfig; - readonly automationDir: string; - readonly workspacePath: string; - readonly kindRegistry: KindRegistry; - readonly workItemSource: WorkItemSource; - readonly agentEventStream: AgentEventStream; - readonly stateVars?: StateContextVars; - readonly log?: Logger; - readonly debug?: boolean; - readonly debugRunUrl?: string; - - // ── Stage-shape parameters ──────────────────────────────────────── - /** Supervisor agent role (e.g. `"supervisor"`). */ - readonly agentRole: AgentRoleName; - /** Verifier chain topic suffix, used as `verifier/{verifierTopic}`. */ - readonly verifierTopic: string; -} - -function payloadOf(stageName: string, input: StageInput): PrFeedbackPayload { - const data = input.data as PrFeedbackPayload | undefined; - if (!data || typeof data.prId !== "number") { - throw new StageLogicError( - "INVALID_STAGE_INPUT", - `${stageName} hook: stage input missing PrFeedbackPayload (scopeKey: ${input.scopeKey})`, - ); - } - return data; -} - -async function computeReviewAttempts( - git: WorkspaceGit, - baseBranch: string, - prType: string, - stageName: string, - log: Logger | undefined, -): Promise { - try { - const total = await git.commitCount(baseBranch); - const initial = prType === "task" ? 2 : 1; - return Math.max(0, total - initial); - } catch (err) { - log?.warn(`${stageName}: commitCount failed (defaulting attempts to 0)`, { - stage: stageName, baseBranch, prType, - error: err instanceof Error ? err.message : String(err), - }); - return 0; - } -} - -export function buildPrFeedbackSupervisorBeforeAgent(deps: PrFeedbackSupervisorHookDeps) { - return async ( - stage: StageDef, - input: StageInput, - workspace: WorkspaceHandle, - ctx: OperationContext, - ): Promise<{ processingPRs?: readonly number[] } | void> => { - const payload = payloadOf(stage.name, input); - const reviewAttempts = await computeReviewAttempts(deps.git, workspace.baseBranch, payload.prType, stage.name, deps.log); - const maxAttempts = deps.defaults.limits.maxReviewAttempts; - const limitReached = reviewAttempts >= maxAttempts; - - deps.log?.info(`${stage.name}: PR #${payload.prId} (${payload.prType}) attempts ${reviewAttempts}/${maxAttempts}`, { - stage: stage.name, prNumber: payload.prId, prType: payload.prType, - reviewAttempts, maxAttempts, limitReached, - }); - - let threadFile = ""; - if (!limitReached) { - await deps.prManager.markProcessing(payload.prId); - deps.log?.info(`${stage.name}: PR #${payload.prId} label ai:pending → ai:processing`, { - stage: stage.name, prNumber: payload.prId, - }); - - if (payload.fullThread) { - threadFile = join( - tmpdir(), - `operator-pr-thread-${payload.prId}-${Date.now()}-${Math.random().toString(36).slice(2)}.md`, - ); - await writeFile(threadFile, `# PR #${payload.prId} Discussion Thread\n\n${payload.fullThread}\n`, "utf-8"); - } - } - - // Capture HEAD SHA AFTER the workspace handle has resolved (branch - // checked out, base PR head pulled) so afterAgent can detect commits - // the agent itself made via Bash. Without this anchor a clean post- - // commit workspace looked identical to a no-op run, triggering the - // wrong "No code changes" comment. - let preAgentHeadSha = ""; - try { - preAgentHeadSha = (await deps.git.headSha()).trim(); - } catch (err) { - deps.log?.warn(`${stage.name}: failed to capture pre-agent HEAD SHA (non-fatal)`, { - stage: stage.name, prNumber: payload.prId, - error: err instanceof Error ? err.message : String(err), - }); - } - - prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(payload.prId), { - prId: payload.prId, branch: payload.branch, prType: payload.prType, - reviewAttempts, maxAttempts, limitReached, threadFile, - newFeedback: payload.newFeedback, - checksContextFile: "", - preAgentHeadSha, - }); - - if (limitReached) return; - return { processingPRs: [payload.prId] }; - }; -} - -/** - * Short-circuit the supervisor agent when the review-cycle cap has already - * been reached. `beforeAgent` computes `limitReached` (and, when true, skips - * the ai:processing transition + thread-file write); this hook then bypasses - * the agent invocation entirely so the engine never spends a full supervisor - * run — a ~10-minute Opus call — only for `afterAgent` to discard the result - * (PR #898, 2026-06-04, burnt 631s of Opus before the verdict was - * overridden to failed). `afterAgent` still posts the limit-reached comment - * and overrides the verdict to `failed`; this just feeds it a placeholder - * result instead of one the agent was paid to produce. - * - * Returns `null` on the normal path (cap not reached) so `runStage` falls - * through to `buildRunInput` + the real agent invocation. - */ -export function buildPrFeedbackSupervisorSynthesizeAgentResult(deps: PrFeedbackSupervisorHookDeps) { - return async ( - stage: StageDef, - input: StageInput, - _workspace: WorkspaceHandle, - ctx: OperationContext, - ): Promise => { - const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); - if (!scratch?.limitReached) return null; - deps.log?.info(`${stage.name}: PR #${payload.prId} review cycle cap reached (${scratch.reviewAttempts}/${scratch.maxAttempts}) — skipping supervisor agent`, { - stage: stage.name, prNumber: payload.prId, - reviewAttempts: scratch.reviewAttempts, maxAttempts: scratch.maxAttempts, - }); - return { - verdict: "failed", - output: "", - attempts: 0, - summary: `review cycle limit reached (${scratch.reviewAttempts}/${scratch.maxAttempts})`, - }; - }; -} - -export function buildPrFeedbackSupervisorBuildRunInput(deps: PrFeedbackSupervisorHookDeps) { - return async ( - stage: StageDef, - input: StageInput, - ctx: OperationContext, - ): Promise => { - const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); - if (!scratch) { - throw new StageLogicError( - "STAGE_SCRATCH_MISSING", - `${stage.name} buildRunInput: missing scratch for PR #${payload.prId} — beforeAgent not run`, - ); - } - const role = resolveRole(deps.agentsConfig, deps.agentRole); - const reviewCriteria = role.review - ? await deps.promptSource.loadChain(`verifier/${deps.verifierTopic}`) - : undefined; - - if (payload.checks.value === "failing" || payload.checks.value === "pending") { - try { - scratch.checksContextFile = await writeChecksContextFile({ - observation: payload.checks, - prNumber: payload.prId, - branch: payload.branch, - }); - } catch (err) { - deps.log?.warn(`${stage.name}: writeChecksContextFile failed (non-fatal)`, { - stage: stage.name, prNumber: payload.prId, - error: err instanceof Error ? err.message : String(err), - }); - } - } - const taskContent = buildSupervisorTask( - payload.prType, payload.branch, scratch.newFeedback, - scratch.threadFile, scratch.checksContextFile || undefined, - ); - return buildRunInput( - role, - { - promptSource: deps.promptSource, - automationDir: deps.automationDir, - vars: { PR_NUMBER: String(payload.prId), PR_TYPE: payload.prType, ...deps.stateVars }, - }, - { - taskContent, - cwd: deps.workspacePath, - maxRetries: 1, - reviewCriteria, - }, - ); - }; -} - -export function buildPrFeedbackSupervisorBuildPR(_deps: PrFeedbackSupervisorHookDeps) { - return async ( - stage: StageDef, - input: StageInput, - ctx: OperationContext, - ): Promise<{ title: string; body: string; commitMessage: string; onSuccess?: "in-review" | "ready-to-merge" | "none" }> => { - const payload = payloadOf(stage.name, input); - prFeedbackSupervisorScratch.clear(ctx, prFeedbackSupervisorScratchKey(payload.prId)); - return { - title: `PR #${payload.prId} supervisor decision`, - body: `Applied supervisor decision on PR #${payload.prId}.`, - commitMessage: `Applied supervisor decision on PR #${payload.prId}`, - onSuccess: "in-review", - }; - }; -} - -export function buildPrFeedbackSupervisorAfterAgent(deps: PrFeedbackSupervisorHookDeps) { - return async ( - stage: StageDef, - input: StageInput, - agentResult: AgentResult, - _workspace: WorkspaceHandle, - ctx: OperationContext, - ): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> => { - const payload = payloadOf(stage.name, input); - const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(payload.prId)); - if (!scratch) { - throw new StageLogicError( - "STAGE_SCRATCH_MISSING", - `${stage.name} afterAgent: missing scratch for PR #${payload.prId} — beforeAgent not run`, - ); - } - try { - return await processSupervisorAfterAgent(deps, stage, payload, scratch, agentResult, ctx); - } catch (err) { - deps.log?.error(`${stage.name}: afterAgent failed for PR #${payload.prId}`, { - stage: stage.name, prNumber: payload.prId, error: errorMessage(err), - }); - throw err; - } finally { - if (scratch.threadFile) { - await unlink(scratch.threadFile).catch(() => { /* best-effort cleanup */ }); - } - } - }; -} +export type { PrFeedbackSupervisorHookDeps } from "./_shared/supervisor-stage-deps.js"; +export { buildPrFeedbackSupervisorBeforeAgent } from "./_shared/supervisor-before-agent.js"; +export { buildPrFeedbackSupervisorSynthesizeAgentResult } from "./_shared/supervisor-synthesize-agent.js"; +export { buildPrFeedbackSupervisorBuildRunInput } from "./_shared/supervisor-build-run-input.js"; +export { buildPrFeedbackSupervisorBuildPR } from "./_shared/supervisor-build-pr.js"; +export { buildPrFeedbackSupervisorAfterAgent } from "./_shared/supervisor-after-agent-hook.js"; From 8868997be0f4ea101133bd836a09d39a901d8640 Mon Sep 17 00:00:00 2001 From: Operator Bot Date: Thu, 6 Aug 2026 23:35:55 +0100 Subject: [PATCH 3/3] Applied supervisor decision on PR #38 --- .../supervisor-after-agent-hook.test.ts | 72 +++++++++++ .../_shared/supervisor-after-agent.test.ts | 91 ++++++++++++++ .../_shared/supervisor-after-agent.ts | 2 - .../_shared/supervisor-aop-apply.test.ts | 74 ++++++++++++ .../composers/_shared/supervisor-aop-apply.ts | 4 + .../_shared/supervisor-before-agent.test.ts | 95 +++++++++++++++ .../_shared/supervisor-before-agent.ts | 5 + .../_shared/supervisor-bot-messages.ts | 11 ++ .../_shared/supervisor-branch-item.test.ts | 48 ++++++++ .../_shared/supervisor-build-pr.test.ts | 49 ++++++++ .../supervisor-build-run-input.test.ts | 89 ++++++++++++++ .../supervisor-change-detection.test.ts | 78 ++++++++++++ .../_shared/supervisor-payload.test.ts | 32 +++++ .../_shared/supervisor-scratch.test.ts | 45 +++++++ .../_shared/supervisor-stage-deps.ts | 3 + .../supervisor-synthesize-agent.test.ts | 71 +++++++++++ .../_shared/supervisor-synthesize-agent.ts | 14 +++ .../supervisor-verdict-routing.test.ts | 112 ++++++++++++++++++ .../_shared/supervisor-verdict-routing.ts | 32 +++++ 19 files changed, 925 insertions(+), 2 deletions(-) create mode 100644 engine/pipeline/composers/_shared/supervisor-after-agent-hook.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-after-agent.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-aop-apply.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-before-agent.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-branch-item.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-build-pr.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-build-run-input.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-change-detection.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-payload.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-scratch.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-synthesize-agent.test.ts create mode 100644 engine/pipeline/composers/_shared/supervisor-verdict-routing.test.ts diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent-hook.test.ts b/engine/pipeline/composers/_shared/supervisor-after-agent-hook.test.ts new file mode 100644 index 0000000..c335897 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-after-agent-hook.test.ts @@ -0,0 +1,72 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { AgentResult, StageDef, StageInput } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { StageLogicError } from "../errors.js"; +import { buildPrFeedbackSupervisorAfterAgent } from "./supervisor-after-agent-hook.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makeInput(): StageInput { + return { + scopeKey: "8", + data: { + prId: 8, branch: "ai/tasks/T-8", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing" as const, observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }, + }; +} + +function makeDeps(): PrFeedbackSupervisorHookDeps { + return { + prManager: { postBotComment: vi.fn().mockResolvedValue(undefined) } as never, + git: { + isClean: vi.fn().mockResolvedValue(true), + headSha: vi.fn().mockResolvedValue("sha"), + } as never, + kindRegistry: { all: [] } as never, + workItemSource: {} as never, + agentEventStream: { + parse: vi.fn().mockReturnValue({ + events: [{ type: "verdict", value: "approved", summary: "ok" }], + diagnostics: [], + }), + } as never, + log: { info: vi.fn(), error: vi.fn() } as never, + agentsConfig: {} as never, + promptSource: {} as never, + defaults: {} as never, + automationDir: "/tmp", + workspacePath: "/tmp/ws", + agentRole: "supervisor", + verifierTopic: "pr-feedback", + }; +} + +const stage = { name: "pr-review" } as StageDef; +const workspace = {} as WorkspaceHandle; +const agentResult = { verdict: "approved", summary: "ok", output: "", attempts: 1 } as AgentResult; + +describe("buildPrFeedbackSupervisorAfterAgent", () => { + it("throws STAGE_SCRATCH_MISSING when scratch is absent", async () => { + const hook = buildPrFeedbackSupervisorAfterAgent(makeDeps()); + await expect( + hook(stage, makeInput(), agentResult, workspace, makeCtx()), + ).rejects.toBeInstanceOf(StageLogicError); + await expect( + hook(stage, makeInput(), agentResult, workspace, makeCtx()), + ).rejects.toMatchObject({ code: "STAGE_SCRATCH_MISSING" }); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent.test.ts b/engine/pipeline/composers/_shared/supervisor-after-agent.test.ts new file mode 100644 index 0000000..2406699 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-after-agent.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { AgentResult, StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; +import { processSupervisorAfterAgent } from "./supervisor-after-agent.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makePayload(): PrFeedbackPayload { + return { + prId: 7, branch: "ai/tasks/T-7", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing", observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }; +} + +function makeScratch(overrides: Partial = {}): PrFeedbackSupervisorScratch { + return { + prId: 7, branch: "ai/tasks/T-7", prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", preAgentHeadSha: "sha-pre", + ...overrides, + }; +} + +function makeAgentResult(verdict: AgentResult["verdict"]): AgentResult { + return { verdict, summary: "ok", output: "", attempts: 1 } as AgentResult; +} + +function makeDeps(): SupervisorAfterAgentDeps { + return { + prManager: { postBotComment: vi.fn().mockResolvedValue(undefined) } as never, + git: { + isClean: vi.fn().mockResolvedValue(true), + headSha: vi.fn().mockResolvedValue("sha-pre"), + } as never, + kindRegistry: { all: [] } as never, + workItemSource: {} as never, + agentEventStream: { + parse: vi.fn().mockReturnValue({ + events: [{ type: "verdict", value: "approved", summary: "ok" }], + diagnostics: [], + }), + } as never, + log: { info: vi.fn(), warn: vi.fn() } as never, + }; +} + +const stage = { name: "pr-review" } as StageDef; + +describe("processSupervisorAfterAgent", () => { + it("overrides verdict to failed when review limit was reached", async () => { + const deps = makeDeps(); + const result = await processSupervisorAfterAgent( + deps, stage, makePayload(), + makeScratch({ limitReached: true, reviewAttempts: 20, maxAttempts: 20 }), + makeAgentResult("approved"), makeCtx(), + ); + expect(result).toMatchObject({ + verdictOverride: "failed", + summaryOverride: "review cycle limit reached (20/20)", + }); + expect(deps.prManager.postBotComment).toHaveBeenCalledWith( + 7, expect.stringContaining("Review cycle limit reached"), expect.any(Object), + ); + }); + + it("posts applied-feedback comment when workspace has changes", async () => { + const deps = makeDeps(); + deps.git.isClean = vi.fn().mockResolvedValue(false); + await processSupervisorAfterAgent( + deps, stage, makePayload(), makeScratch(), + makeAgentResult("approved"), makeCtx(), + ); + expect(deps.prManager.postBotComment).toHaveBeenCalledWith( + 7, expect.stringContaining("Applied review feedback"), expect.any(Object), + ); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-after-agent.ts b/engine/pipeline/composers/_shared/supervisor-after-agent.ts index b32882f..96f8949 100644 --- a/engine/pipeline/composers/_shared/supervisor-after-agent.ts +++ b/engine/pipeline/composers/_shared/supervisor-after-agent.ts @@ -10,8 +10,6 @@ import { applySupervisorAgentEvents } from "./supervisor-aop-apply.js"; import { detectSupervisorChanges } from "./supervisor-change-detection.js"; import { routeSupervisorVerdict } from "./supervisor-verdict-routing.js"; -export type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; - export async function processSupervisorAfterAgent( deps: SupervisorAfterAgentDeps, stage: StageDef, diff --git a/engine/pipeline/composers/_shared/supervisor-aop-apply.test.ts b/engine/pipeline/composers/_shared/supervisor-aop-apply.test.ts new file mode 100644 index 0000000..6d7878c --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-aop-apply.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; +import { applySupervisorAgentEvents } from "./supervisor-aop-apply.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makePayload(): PrFeedbackPayload { + return { + prId: 10, branch: "ai/tasks/T-10", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing", observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [{ + threadId: "THREAD_1", isResolved: false, authorType: "Bot", commentIds: ["c1"], + }], + freshReviewCommentIds: ["c1"], + }; +} + +function makeDeps(events: unknown[]): SupervisorAfterAgentDeps { + return { + prManager: { + postThreadReply: vi.fn().mockResolvedValue(undefined), + resolveThread: vi.fn().mockResolvedValue(undefined), + } as never, + git: {} as never, + kindRegistry: { + all: [{ + name: "task", idPrefix: "T", dataDir: ".operator/data/tasks", + branchPrefix: "ai/tasks", terminalStatuses: [], parentKinds: [], + }], + } as never, + workItemSource: {} as never, + agentEventStream: { + parse: vi.fn().mockReturnValue({ events, diagnostics: [] }), + } as never, + log: { info: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn() } as never, + }; +} + +const stage = { name: "pr-review" } as StageDef; + +describe("applySupervisorAgentEvents", () => { + it("applies agent output and disposes review threads when replies are present", async () => { + const deps = makeDeps([ + { type: "comment-reply", thread: "c1", disposition: "fixed", note: "done" }, + { type: "verdict", value: "approved", summary: "fixed" }, + ]); + const result = await applySupervisorAgentEvents( + deps, stage, makePayload(), "agent stdout", makeCtx(), + ); + expect(result.verdict).toBe("approved"); + expect(deps.prManager.postThreadReply).toHaveBeenCalled(); + expect(deps.prManager.resolveThread).toHaveBeenCalledWith("THREAD_1"); + }); + + it("skips thread disposition when there are no threads and no replies", async () => { + const deps = makeDeps([{ type: "verdict", value: "approved", summary: "ok" }]); + const payload = { ...makePayload(), reviewThreads: [], freshReviewCommentIds: [] }; + await applySupervisorAgentEvents(deps, stage, payload, "out", makeCtx()); + expect(deps.prManager.postThreadReply).not.toHaveBeenCalled(); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-aop-apply.ts b/engine/pipeline/composers/_shared/supervisor-aop-apply.ts index ec812c5..3f04d9e 100644 --- a/engine/pipeline/composers/_shared/supervisor-aop-apply.ts +++ b/engine/pipeline/composers/_shared/supervisor-aop-apply.ts @@ -38,6 +38,10 @@ export async function applySupervisorAgentEvents( applyErrors: applied.applyErrors.length, }); + // Answer + resolve inline review threads the supervisor disposed of + // this cycle. Runs on every agent path (fix-in-place, cancel, escalate, + // …) so no reviewer comment is left without a note. Bot threads (Copilot) + // are resolved; human threads get the note but stay open for the human. if (payload.reviewThreads.length > 0 || applied.commentReplies.length > 0) { await applyThreadDispositions({ prId: payload.prId, diff --git a/engine/pipeline/composers/_shared/supervisor-before-agent.test.ts b/engine/pipeline/composers/_shared/supervisor-before-agent.test.ts new file mode 100644 index 0000000..5099b8d --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-before-agent.test.ts @@ -0,0 +1,95 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { buildPrFeedbackSupervisorBeforeAgent } from "./supervisor-before-agent.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makeStage(): StageDef { + return { + name: "pr-review", agent: "supervisor", selector: "pr-feedback", + merge: "gated", branchScope: "pr", schedule: "* * * * *", + enabled: true, baseBranch: "develop", + }; +} + +function makeInput(prType: string) { + const data = { + prId: 99, branch: "ai/tasks/T-99", baseBranch: "develop", prType, + newFeedback: "fix it", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing" as const, observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }; + return { scopeKey: "99", data } satisfies StageInput; +} + +function makeDeps(commitCount: number): PrFeedbackSupervisorHookDeps { + return { + prManager: { markProcessing: vi.fn().mockResolvedValue(undefined) } as never, + git: { + commitCount: vi.fn().mockResolvedValue(commitCount), + headSha: vi.fn().mockResolvedValue("sha-pre"), + } as never, + agentsConfig: {} as never, + promptSource: {} as never, + defaults: { limits: { maxReviewAttempts: 20, maxCiRetryAttempts: 3 } } as never, + automationDir: "/tmp", workspacePath: "/tmp/ws", + kindRegistry: {} as never, workItemSource: {} as never, agentEventStream: {} as never, + agentRole: "supervisor", verifierTopic: "pr-feedback", + log: { info: vi.fn(), warn: vi.fn() } as never, + }; +} + +const workspace = { branch: "ai/tasks/T-99", baseBranch: "develop", existedRemote: true } as WorkspaceHandle; + +describe("buildPrFeedbackSupervisorBeforeAgent", () => { + it("computes task reviewAttempts with initial offset 2", async () => { + const deps = makeDeps(5); + const ctx = makeCtx(); + await buildPrFeedbackSupervisorBeforeAgent(deps)(makeStage(), makeInput("task"), workspace, ctx); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(99)); + expect(scratch?.reviewAttempts).toBe(3); // 5 - 2 + expect(deps.prManager.markProcessing).toHaveBeenCalledWith(99); + }); + + it("computes non-task reviewAttempts with initial offset 1", async () => { + const deps = makeDeps(5); + const ctx = makeCtx(); + await buildPrFeedbackSupervisorBeforeAgent(deps)(makeStage(), makeInput("finding"), workspace, ctx); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(99)); + expect(scratch?.reviewAttempts).toBe(4); // 5 - 1 + }); + + it("defaults reviewAttempts to 0 when commitCount fails", async () => { + const deps = makeDeps(0); + deps.git.commitCount = vi.fn().mockRejectedValue(new Error("git down")); + const ctx = makeCtx(); + await buildPrFeedbackSupervisorBeforeAgent(deps)(makeStage(), makeInput("task"), workspace, ctx); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(99)); + expect(scratch?.reviewAttempts).toBe(0); + expect(deps.log?.warn).toHaveBeenCalled(); + }); + + it("captures preAgentHeadSha for afterAgent change detection", async () => { + const deps = makeDeps(2); + const ctx = makeCtx(); + await buildPrFeedbackSupervisorBeforeAgent(deps)(makeStage(), makeInput("task"), workspace, ctx); + const scratch = prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(99)); + expect(scratch?.preAgentHeadSha).toBe("sha-pre"); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-before-agent.ts b/engine/pipeline/composers/_shared/supervisor-before-agent.ts index 20cc776..aa825b0 100644 --- a/engine/pipeline/composers/_shared/supervisor-before-agent.ts +++ b/engine/pipeline/composers/_shared/supervisor-before-agent.ts @@ -68,6 +68,11 @@ export function buildPrFeedbackSupervisorBeforeAgent(deps: PrFeedbackSupervisorH } } + // Capture HEAD SHA AFTER the workspace handle has resolved (branch + // checked out, base PR head pulled) so afterAgent can detect commits + // the agent itself made via Bash. Without this anchor a clean post- + // commit workspace looked identical to a no-op run, triggering the + // wrong "No code changes" comment. let preAgentHeadSha = ""; try { preAgentHeadSha = (await deps.git.headSha()).trim(); diff --git a/engine/pipeline/composers/_shared/supervisor-bot-messages.ts b/engine/pipeline/composers/_shared/supervisor-bot-messages.ts index 200dca6..49263a4 100644 --- a/engine/pipeline/composers/_shared/supervisor-bot-messages.ts +++ b/engine/pipeline/composers/_shared/supervisor-bot-messages.ts @@ -22,6 +22,17 @@ export function formatAppliedReviewFeedbackMessage(suffix: string): string { return `Applied review feedback.${suffix}`; } +/** + * Format the no-changes bot comment for a supervisor cycle. + * + * The engine asserts ONLY the fact it can observe (clean tree + unchanged HEAD + * ⇒ no code changes this cycle). It must NOT editorialize the REASON: a + * verdict=approved + no-changes run can mean "feedback genuinely already + * addressed" OR "supervisor chose escalate" (supervisor.md maps escalate → + * approved + no changes). A fixed engine sentence contradicted the agent's + * own reasoning on escalate cycles (PR #892, 2026-05-21). The WHY lives in + * the agent's reasoning block below — never in a fixed engine sentence. + */ export function formatNoCodeChangesMessage(effectiveSummary: string, suffix: string): string { const reasoning = effectiveSummary.trim().slice(0, 1500); const reasoningBlock = reasoning ? `\n\n${reasoning}` : ""; diff --git a/engine/pipeline/composers/_shared/supervisor-branch-item.test.ts b/engine/pipeline/composers/_shared/supervisor-branch-item.test.ts new file mode 100644 index 0000000..2ade391 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-branch-item.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from "vitest"; +import type { KindRegistry } from "@operator/core"; +import { inferKindFromBranch } from "./supervisor-branch-item.js"; + +function makeRegistry(prefixes: Array<{ name: string; branchPrefix: string }>): KindRegistry { + return { + all: prefixes.map((p) => ({ + name: p.name, + idPrefix: p.name[0]!.toUpperCase(), + dataDir: `.operator/data/${p.name}s`, + branchPrefix: p.branchPrefix, + terminalStatuses: ["rejected"], + parentKinds: [], + })), + } as unknown as KindRegistry; +} + +describe("inferKindFromBranch", () => { + it("normalises branchPrefix without trailing slash", () => { + const registry = makeRegistry([ + { name: "task", branchPrefix: "ai/tasks" }, + ]); + expect(inferKindFromBranch("ai/tasks/T20260511-0001", registry)).toEqual({ + kind: "task", + id: "T20260511-0001", + }); + }); + + it("accepts branchPrefix that already ends with slash", () => { + const registry = makeRegistry([ + { name: "finding", branchPrefix: "ai/findings/" }, + ]); + expect(inferKindFromBranch("ai/findings/F20260511-0001", registry)).toEqual({ + kind: "finding", + id: "F20260511-0001", + }); + }); + + it("returns null when branch matches no kind prefix", () => { + const registry = makeRegistry([{ name: "task", branchPrefix: "ai/tasks" }]); + expect(inferKindFromBranch("feature/unrelated", registry)).toBeNull(); + }); + + it("returns null when id segment is empty after prefix", () => { + const registry = makeRegistry([{ name: "task", branchPrefix: "ai/tasks" }]); + expect(inferKindFromBranch("ai/tasks/", registry)).toBeNull(); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-build-pr.test.ts b/engine/pipeline/composers/_shared/supervisor-build-pr.test.ts new file mode 100644 index 0000000..6b98ce3 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-build-pr.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput } from "../../types.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { buildPrFeedbackSupervisorBuildPR } from "./supervisor-build-pr.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +const deps = {} as PrFeedbackSupervisorHookDeps; +const stage = { name: "pr-review" } as StageDef; + +describe("buildPrFeedbackSupervisorBuildPR", () => { + it("clears scratch and returns in-review PR metadata", async () => { + const ctx = makeCtx(); + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(55), { + prId: 55, branch: "ai/tasks/T-55", prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", preAgentHeadSha: "", + }); + const input: StageInput = { + scopeKey: "55", + data: { + prId: 55, branch: "ai/tasks/T-55", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing", observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }, + }; + const result = await buildPrFeedbackSupervisorBuildPR(deps)(stage, input, ctx); + expect(result).toMatchObject({ + title: "PR #55 supervisor decision", + onSuccess: "in-review", + }); + expect(prFeedbackSupervisorScratch.get(ctx, prFeedbackSupervisorScratchKey(55))).toBeUndefined(); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-build-run-input.test.ts b/engine/pipeline/composers/_shared/supervisor-build-run-input.test.ts new file mode 100644 index 0000000..2f7273a --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-build-run-input.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput } from "../../types.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { StageLogicError } from "../errors.js"; +import { buildPrFeedbackSupervisorBuildRunInput } from "./supervisor-build-run-input.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makeInput(): StageInput { + return { + scopeKey: "3", + data: { + prId: 3, branch: "ai/tasks/T-3", baseBranch: "develop", prType: "task", + newFeedback: "please fix", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing" as const, observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }, + }; +} + +function makeDeps(): PrFeedbackSupervisorHookDeps { + return { + prManager: {} as never, + git: {} as never, + agentsConfig: { + defaultProvider: "claude", + providers: { claude: { command: "claude" } }, + agents: { + supervisor: { + provider: "claude", instructions: "agents/supervisor.md", + timeout: 3600, model: "opus", review: true, + tools: "Read", maxBudget: 1, context: ["base"], + }, + }, + } as never, + promptSource: { loadChain: vi.fn().mockResolvedValue("criteria") } as never, + defaults: {} as never, + automationDir: "/tmp/.operator", + workspacePath: "/tmp/ws", + kindRegistry: {} as never, + workItemSource: {} as never, + agentEventStream: {} as never, + agentRole: "supervisor", + verifierTopic: "pr-feedback", + log: { warn: vi.fn() } as never, + }; +} + +const stage = { name: "pr-review" } as StageDef; + +describe("buildPrFeedbackSupervisorBuildRunInput", () => { + it("throws STAGE_SCRATCH_MISSING when beforeAgent did not run", async () => { + const ctx = makeCtx(); + await expect( + buildPrFeedbackSupervisorBuildRunInput(makeDeps())(stage, makeInput(), ctx), + ).rejects.toBeInstanceOf(StageLogicError); + await expect( + buildPrFeedbackSupervisorBuildRunInput(makeDeps())(stage, makeInput(), ctx), + ).rejects.toMatchObject({ code: "STAGE_SCRATCH_MISSING" }); + }); + + it("loads verifier chain from verifier/{verifierTopic}", async () => { + const deps = makeDeps(); + const ctx = makeCtx(); + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(3), { + prId: 3, branch: "ai/tasks/T-3", prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "please fix", checksContextFile: "", preAgentHeadSha: "", + }); + const runInput = await buildPrFeedbackSupervisorBuildRunInput(deps)(stage, makeInput(), ctx); + expect(deps.promptSource.loadChain).toHaveBeenCalledWith("verifier/pr-feedback"); + expect(runInput.taskContent).toContain("please fix"); + expect(runInput.cwd).toBe("/tmp/ws"); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-change-detection.test.ts b/engine/pipeline/composers/_shared/supervisor-change-detection.test.ts new file mode 100644 index 0000000..9a2500e --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-change-detection.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect, vi } from "vitest"; +import type { WorkspaceGit } from "../../../infra/git.js"; +import type { StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; +import { detectSupervisorChanges } from "./supervisor-change-detection.js"; + +function makeScratch(preAgentHeadSha: string): PrFeedbackSupervisorScratch { + return { + prId: 1, branch: "ai/tasks/T-1", prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", + preAgentHeadSha, + }; +} + +function makePayload(): PrFeedbackPayload { + return { + prId: 1, branch: "ai/tasks/T-1", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing", observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }; +} + +const stage = { name: "pr-review" } as StageDef; + +describe("detectSupervisorChanges", () => { + it("reports changesApplied when workspace is dirty", async () => { + const git = { + isClean: vi.fn().mockResolvedValue(false), + headSha: vi.fn().mockResolvedValue("sha-same"), + } as unknown as WorkspaceGit; + const result = await detectSupervisorChanges(git, makeScratch("sha-same"), stage, makePayload()); + expect(result).toMatchObject({ + workspaceDirty: true, + headAdvanced: false, + changesApplied: true, + postAgentHeadSha: "sha-same", + }); + }); + + it("reports headAdvanced when post-agent SHA differs from pre-agent anchor", async () => { + const git = { + isClean: vi.fn().mockResolvedValue(true), + headSha: vi.fn().mockResolvedValue("sha-post"), + } as unknown as WorkspaceGit; + const result = await detectSupervisorChanges(git, makeScratch("sha-pre"), stage, makePayload()); + expect(result).toMatchObject({ + workspaceDirty: false, + headAdvanced: true, + changesApplied: true, + postAgentHeadSha: "sha-post", + }); + }); + + it("falls back to dirty-only check when headSha() throws", async () => { + const log = { warn: vi.fn() }; + const git = { + isClean: vi.fn().mockResolvedValue(true), + headSha: vi.fn().mockRejectedValue(new Error("git unavailable")), + } as unknown as WorkspaceGit; + const result = await detectSupervisorChanges( + git, makeScratch("sha-pre"), stage, makePayload(), log as never, + ); + expect(result).toMatchObject({ + workspaceDirty: false, + headAdvanced: false, + changesApplied: false, + postAgentHeadSha: "", + }); + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining("falling back to dirty-only check"), + expect.any(Object), + ); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-payload.test.ts b/engine/pipeline/composers/_shared/supervisor-payload.test.ts new file mode 100644 index 0000000..034f2bc --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-payload.test.ts @@ -0,0 +1,32 @@ +import { describe, it, expect } from "vitest"; +import type { StageInput } from "../../types.js"; +import { StageLogicError } from "../errors.js"; +import { payloadOf } from "./supervisor-payload.js"; + +describe("payloadOf", () => { + it("returns PrFeedbackPayload when data is valid", () => { + const payload = { prId: 42, branch: "ai/tasks/T-1" }; + const input: StageInput = { scopeKey: "42", data: payload }; + expect(payloadOf("pr-review", input)).toBe(payload); + }); + + it("throws INVALID_STAGE_INPUT when data is missing", () => { + const input: StageInput = { scopeKey: "42", data: undefined }; + expect(() => payloadOf("pr-review", input)).toThrow(StageLogicError); + try { + payloadOf("pr-review", input); + } catch (err) { + expect((err as StageLogicError).code).toBe("INVALID_STAGE_INPUT"); + expect((err as Error).message).toContain("scopeKey: 42"); + } + }); + + it("throws INVALID_STAGE_INPUT when prId is not a number", () => { + const input: StageInput = { scopeKey: "x", data: { prId: "42" } }; + try { + payloadOf("pr-review", input); + } catch (err) { + expect((err as StageLogicError).code).toBe("INVALID_STAGE_INPUT"); + } + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-scratch.test.ts b/engine/pipeline/composers/_shared/supervisor-scratch.test.ts new file mode 100644 index 0000000..7644987 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-scratch.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from "vitest"; +import type { OperationContext } from "@operator/core"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, + type PrFeedbackSupervisorScratch, +} from "./supervisor-scratch.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makeEntry(prId: number): PrFeedbackSupervisorScratch { + return { + prId, branch: `ai/tasks/T-${prId}`, prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", + preAgentHeadSha: "", + }; +} + +describe("prFeedbackSupervisorScratch", () => { + it("scopes entries by traceId and prId key", () => { + const ctxA = makeCtx(); + const ctxB = makeCtx(); + const key = prFeedbackSupervisorScratchKey(42); + prFeedbackSupervisorScratch.set(ctxA, key, makeEntry(42)); + expect(prFeedbackSupervisorScratch.get(ctxA, key)?.prId).toBe(42); + expect(prFeedbackSupervisorScratch.get(ctxB, key)).toBeUndefined(); + }); + + it("clear removes the entry for the current trace", () => { + const ctx = makeCtx(); + const key = prFeedbackSupervisorScratchKey(7); + prFeedbackSupervisorScratch.set(ctx, key, makeEntry(7)); + prFeedbackSupervisorScratch.clear(ctx, key); + expect(prFeedbackSupervisorScratch.get(ctx, key)).toBeUndefined(); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-stage-deps.ts b/engine/pipeline/composers/_shared/supervisor-stage-deps.ts index c76a8c9..ba2c009 100644 --- a/engine/pipeline/composers/_shared/supervisor-stage-deps.ts +++ b/engine/pipeline/composers/_shared/supervisor-stage-deps.ts @@ -23,6 +23,9 @@ export interface PrFeedbackSupervisorHookDeps { readonly log?: Logger; readonly debug?: boolean; readonly debugRunUrl?: string; + // ── Stage-shape parameters ──────────────────────────────────────── + /** Supervisor agent role (e.g. `"supervisor"`). */ readonly agentRole: AgentRoleName; + /** Verifier chain topic suffix, used as `verifier/{verifierTopic}`. */ readonly verifierTopic: string; } diff --git a/engine/pipeline/composers/_shared/supervisor-synthesize-agent.test.ts b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.test.ts new file mode 100644 index 0000000..0117ec1 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.test.ts @@ -0,0 +1,71 @@ +import { describe, it, expect, vi } from "vitest"; +import type { OperationContext } from "@operator/core"; +import type { StageDef, StageInput } from "../../types.js"; +import type { WorkspaceHandle } from "../../primitives/workspace-scope.js"; +import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; +import { buildPrFeedbackSupervisorSynthesizeAgentResult } from "./supervisor-synthesize-agent.js"; +import { + prFeedbackSupervisorScratch, + prFeedbackSupervisorScratchKey, +} from "./supervisor-scratch.js"; + +function makeCtx(): OperationContext { + return { + traceId: `trace-${Math.random()}`, + repoId: "sample", + action: "test", + budget: { limitUsd: undefined, spentUsd: 0, add: () => {}, isExceeded: () => false }, + signal: AbortSignal.timeout(10_000), + }; +} + +function makeInput() { + return { + scopeKey: "1", + data: { + prId: 1, branch: "ai/tasks/T-1", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks: { value: "passing" as const, observedAt: "", checks: [] }, + respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }, + } satisfies StageInput; +} + +const stage = { name: "pr-review" } as StageDef; +const workspace = {} as WorkspaceHandle; + +describe("buildPrFeedbackSupervisorSynthesizeAgentResult", () => { + it("returns null when review cap is not reached (normal path)", async () => { + const deps = { log: { info: vi.fn() } } as unknown as PrFeedbackSupervisorHookDeps; + const ctx = makeCtx(); + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(1), { + prId: 1, branch: "ai/tasks/T-1", prType: "task", + reviewAttempts: 1, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", preAgentHeadSha: "", + }); + const result = await buildPrFeedbackSupervisorSynthesizeAgentResult(deps)( + stage, makeInput(), workspace, ctx, + ); + expect(result).toBeNull(); + }); + + it("returns failed placeholder when review cap is reached", async () => { + const deps = { log: { info: vi.fn() } } as unknown as PrFeedbackSupervisorHookDeps; + const ctx = makeCtx(); + prFeedbackSupervisorScratch.set(ctx, prFeedbackSupervisorScratchKey(1), { + prId: 1, branch: "ai/tasks/T-1", prType: "task", + reviewAttempts: 20, maxAttempts: 20, limitReached: true, + threadFile: "", newFeedback: "", checksContextFile: "", preAgentHeadSha: "", + }); + const result = await buildPrFeedbackSupervisorSynthesizeAgentResult(deps)( + stage, makeInput(), workspace, ctx, + ); + expect(result).toMatchObject({ + verdict: "failed", + output: "", + attempts: 0, + summary: "review cycle limit reached (20/20)", + }); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts index 53b5c0d..96f752c 100644 --- a/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts +++ b/engine/pipeline/composers/_shared/supervisor-synthesize-agent.ts @@ -8,6 +8,20 @@ import { import type { PrFeedbackSupervisorHookDeps } from "./supervisor-stage-deps.js"; import { payloadOf } from "./supervisor-payload.js"; +/** + * Short-circuit the supervisor agent when the review-cycle cap has already + * been reached. `beforeAgent` computes `limitReached` (and, when true, skips + * the ai:processing transition + thread-file write); this hook then bypasses + * the agent invocation entirely so the engine never spends a full supervisor + * run — a ~10-minute Opus call — only for `afterAgent` to discard the result + * (PR #898, 2026-06-04, burnt 631s of Opus before the verdict was + * overridden to failed). `afterAgent` still posts the limit-reached comment + * and overrides the verdict to `failed`; this just feeds it a placeholder + * result instead of one the agent was paid to produce. + * + * Returns `null` on the normal path (cap not reached) so `runStage` falls + * through to `buildRunInput` + the real agent invocation. + */ export function buildPrFeedbackSupervisorSynthesizeAgentResult(deps: PrFeedbackSupervisorHookDeps) { return async ( stage: StageDef, diff --git a/engine/pipeline/composers/_shared/supervisor-verdict-routing.test.ts b/engine/pipeline/composers/_shared/supervisor-verdict-routing.test.ts new file mode 100644 index 0000000..b3173a9 --- /dev/null +++ b/engine/pipeline/composers/_shared/supervisor-verdict-routing.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, vi } from "vitest"; +import type { AgentResult, StageDef } from "../../types.js"; +import type { PrFeedbackPayload } from "../../primitives/pr-feedback-selector.js"; +import type { AopApplyResult } from "../../primitives/aop-applier.js"; +import type { BotAttribution } from "../../../delivery/bot-footer.js"; +import type { PrFeedbackSupervisorScratch } from "./supervisor-scratch.js"; +import type { SupervisorAfterAgentDeps } from "./supervisor-after-agent-deps.js"; +import type { SupervisorChanges } from "./supervisor-change-detection.js"; +import { routeSupervisorVerdict } from "./supervisor-verdict-routing.js"; + +function makePayload(checks: PrFeedbackPayload["checks"]): PrFeedbackPayload { + return { + prId: 100, branch: "ai/tasks/T-100", baseBranch: "develop", prType: "task", + newFeedback: "", fullThread: "", botAttempts: 0, oldestFreshAt: "", + checks, respondedIds: [], ciAttempts: 0, maxCiRetryAttempts: 3, + reviewThreads: [], freshReviewCommentIds: [], + }; +} + +function makeScratch(): PrFeedbackSupervisorScratch { + return { + prId: 100, branch: "ai/tasks/T-100", prType: "task", + reviewAttempts: 0, maxAttempts: 20, limitReached: false, + threadFile: "", newFeedback: "", checksContextFile: "", preAgentHeadSha: "sha-pre", + }; +} + +function makeApplied(verdict: AopApplyResult["verdict"]): AopApplyResult { + return { + verdict, + summary: "applier summary", + applyErrors: [], + applied: { childItems: [], statusUpdates: [], bodyUpdates: [] }, + commentReplies: [], + }; +} + +function makeChanges(changesApplied: boolean): SupervisorChanges { + return { + workspaceDirty: changesApplied, + headAdvanced: false, + changesApplied, + postAgentHeadSha: "sha-post", + }; +} + +function makeAgentResult(verdict: AgentResult["verdict"]): AgentResult { + return { verdict, summary: "agent summary", output: "", attempts: 1 } as AgentResult; +} + +function makeDeps(): SupervisorAfterAgentDeps { + return { + prManager: { postBotComment: vi.fn().mockResolvedValue(undefined) } as never, + git: {} as never, + kindRegistry: { all: [] } as never, + workItemSource: {} as never, + agentEventStream: {} as never, + log: { info: vi.fn() } as never, + }; +} + +const stage = { name: "pr-review" } as StageDef; +const attribution = { responded: new Set() } as BotAttribution; + +describe("routeSupervisorVerdict", () => { + it("downgrades failed verdict to approved when fix was pushed over stale failing CI", async () => { + const deps = makeDeps(); + const payload = makePayload({ + value: "failing", observedAt: "2026-06-26T00:00:00Z", + headSha: "abc123def456", checks: [], + }); + const result = await routeSupervisorVerdict( + deps, stage, payload, makeScratch(), + makeAgentResult("failed"), makeApplied("approved"), + makeChanges(true), attribution, true, + ); + expect(result).toMatchObject({ verdictOverride: "approved" }); + expect(deps.prManager.postBotComment).toHaveBeenCalledWith( + 100, expect.stringContaining("pushed fix supersedes"), attribution, + ); + }); + + it("does not override approved verdict when CI is failing but no changes were pushed", async () => { + const deps = makeDeps(); + const payload = makePayload({ + value: "failing", observedAt: "2026-06-26T00:00:00Z", + headSha: "abc123", checks: [], + }); + const result = await routeSupervisorVerdict( + deps, stage, payload, makeScratch(), + makeAgentResult("approved"), makeApplied("approved"), + makeChanges(false), attribution, true, + ); + expect(result?.verdictOverride).toBeUndefined(); + expect(deps.prManager.postBotComment).toHaveBeenCalledWith( + 100, expect.stringContaining("No code changes"), attribution, + ); + }); + + it("posts terminal comment for non-approved applier verdict", async () => { + const deps = makeDeps(); + const result = await routeSupervisorVerdict( + deps, stage, makePayload({ value: "passing", observedAt: "", checks: [] }), + makeScratch(), makeAgentResult("approved"), makeApplied("cancelled"), + makeChanges(false), attribution, false, + ); + expect(result).toMatchObject({ verdictOverride: "cancelled" }); + expect(deps.prManager.postBotComment).toHaveBeenCalledWith( + 100, expect.stringContaining("Supervisor decision"), attribution, + ); + }); +}); diff --git a/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts b/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts index e8f2267..e395b4f 100644 --- a/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts +++ b/engine/pipeline/composers/_shared/supervisor-verdict-routing.ts @@ -25,10 +25,42 @@ export async function routeSupervisorVerdict( ciFailing: boolean, ): Promise<{ verdictOverride?: Verdict; summaryOverride?: string } | void> { const suffix = formatDebugRunLinkSuffix(deps.debug, deps.debugRunUrl); + + // 2026-05-13: removed defense-in-depth "approved + ciFailing → + // override to failed" check. The verifier (inside the agent chain + // when stage has reviewEnabled: true) is the authority on whether + // the supervisor's fix addresses CI. Defense-in-depth duplicated + // verifier and second-guessed it from a stale CI observation — + // CI was observed at cycle start (BEFORE supervisor committed via + // Bash) so it always looked "failing" even when the fix had just + // been pushed and CI re-run hadn't completed yet. The canonical + // case: supervisor correctly fixed all 47 backend test failures + // and 14 Copilot comments and + // committed/pushed, but the post-verifier check flipped to failed + // because checks.headSha was the pre-commit SHA. Per user guidance: + // "verify process should be able to detect commits and verify them + // even if committed — if OK act as usual; if wrong comment back to + // redo/fix. Committed work has no difference except technical to + // detect changes." Trust verifier — if its judgment is wrong, the + // next pr-feedback cycle picks the PR up with fresh CI data. const effectiveVerdict = (applied.verdict !== "approved" || applied.applyErrors.length > 0) ? applied.verdict : agentResult.verdict; + // Stale-CI guard — completes the 2026-05-13 stale-CI fix (PR-1186). + // `payload.checks` is observed at cycle start, BEFORE the supervisor + // edits/commits, so a `failed` verdict resting on it — the verifier + // hard-rule "approved while CI failing", or the supervisor declining to + // approve over red CI — is judging a run the just-pushed fix already + // supersedes. When the supervisor DID push a fix in response to that + // failing CI, never latch terminal `ai:failed` on the stale observation: + // leave the PR in-review so fresh CI on the new commit decides and the + // next pr-feedback cycle re-evaluates (the recovery the 2026-05-13 note + // promised but the selector's `ai:failed` exclusion otherwise blocks). + // Bounded by the maxReviewAttempts cap in beforeAgent, so a genuinely + // unfixed failure still reaches `ai:failed` once the review budget is + // spent. Excludes `cancelled` (a human /cancel is a real terminal) and + // apply/parse errors (real contract violations, not CI staleness). if (effectiveVerdict === "failed" && applied.applyErrors.length === 0 && changes.changesApplied && ciFailing) { await deps.prManager.postBotComment( payload.prId,