Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ Codex sandbox runs go through the Agent Client Protocol: `lib/harness/acp/` hold

### Decision layer

`lib/decisions/` asks an evaluation model closed questions about program state (command risk, hidden tool failures, unsupported claims, loops, memory promotion, and, observing only, which skills and injected memories a request needed) and records every answer in `decision_events`. Read [docs/decisions.md](./docs/decisions.md) before touching it. The binding rules: `decide()` fails open and never throws; a decision may only add caution (an approval, a note) and never relaxes `policy.ts` or the shell guard, except that `review_format` may reword a published PR review after `review_rewrite_faithful` confirms it kept every claim; nothing in it may end or shorten a run; question wording and thresholds live only in `definitions.ts` and `definitions-selection.ts` with a `version` bump on every change; every call site puts `teamId` in its scope so the account's Run checks switch applies; customer-facing text never names the evaluation provider. The one authored decision is the automation Classify node (`classify()` in `lib/decisions/classify.ts`): its question lives in the customer's flow graph, and it does not fail open, because the author chose that node to pick a branch. An outage fails the node so the flow's `error` handle can route it. Tests inject evaluators through `deps`; the real evaluator is inert under Vitest.
`lib/decisions/` asks an evaluation model closed questions about program state (command risk, hidden tool failures, unsupported claims, loops, memory promotion, and, observing only, which skills and injected memories a request needed) and records every answer in `decision_events`. Read [docs/decisions.md](./docs/decisions.md) before touching it. The binding rules: `decide()` fails open and never throws; a decision may only add caution (an approval, a note) and never relaxes `policy.ts` or the shell guard, except that `review_format` sends a flagged native review draft back to the reviewer that wrote it (the revision must keep the verdict and every finding) and may reword a published PR review after `review_rewrite_faithful` confirms it kept every claim; nothing in it may end or shorten a run; question wording and thresholds live only in `definitions.ts` and `definitions-selection.ts` with a `version` bump on every change; every call site puts `teamId` in its scope so the account's Run checks switch applies; customer-facing text never names the evaluation provider. The one authored decision is the automation Classify node (`classify()` in `lib/decisions/classify.ts`): its question lives in the customer's flow graph, and it does not fail open, because the author chose that node to pick a branch. An outage fails the node so the flow's `error` handle can route it. Tests inject evaluators through `deps`; the real evaluator is inert under Vitest.

## Core Commands

Expand Down
22 changes: 14 additions & 8 deletions docs/decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Code acts.
| `memory_promotion_gate` | Before memory promotion's extraction call | `shadow` | In `enforce`, skips extraction when the record holds nothing durable |
| `skill_selection` | When a run or turn has skills in play, meaning a roster agent's linked skills, the user's skill catalog, or both: the sandbox harness route (`harness`), the native runner (`agent_run`), Control (`control`), and workspace chat (`chat`) | `shadow` | Records only. Linked skills are still all loaded and the catalog is still offered as an index; the row says which skills the task needed. `metadata.sources` tells a linked skill from a catalog one, `metadata.invoked` lists what the user named, and `baseline.loaded` against `baseline.offered` shows how much was delivered in full |
| `memory_relevance` | Every Control turn that injects memories, beside the turn | `shadow` | Records only. The prompt is already built; the row says which injected memories bear on the request |
| `review_format` | Every structured PR review, before the check run, native review, and timeline comment are published (`pr_review`) | `enforce` | A dense paragraph, tool or report-field chatter, a pointer to details that are not stated, or unformatted code sends the text to a platform model for a formatting rewrite. The rewrite is published only if `review_rewrite_faithful` accepts it |
| `review_format` | A native review's draft inside the run, and at publish time any structured PR review not already passed there, before the check run, native review, and timeline comment are published (`pr_review`) | `enforce` | A dense paragraph, tool or report-field chatter, a pointer to details that are not stated, or unformatted code. In a native review the reviewer that wrote the draft is told what is wrong and files its report again in the same conversation; its revision replaces the draft if it keeps the verdict and every finding. A review still flagged at publish time (a harness review, or a revision that did not fix it) goes to a platform model for a formatting rewrite, published only if `review_rewrite_faithful` accepts it |
| `review_rewrite_faithful` | After a `review_format` rewrite | `enforce` | Accepts the rewrite. Anything else, including an outage, publishes the reviewer's original text |
| `flow_classify` | Every automation Classify node | always acts | The run takes the answered branch. The question is authored in the flow graph |

Expand Down Expand Up @@ -105,13 +105,19 @@ Settings, Account (`PATCH /api/settings/decision-checks`).
turned its checks off, makes it fail, not pass.
- **Only add caution.** A decision can add an approval or a note. It can never
relax `policy.ts`, a protected-branch rule, or the shell guard. The one
decision that changes output is `review_format`: it may replace the wording
of a published review, never its verdict, check conclusion, or a finding's
title, severity, path, or line, and only after `review_rewrite_faithful`
confirms the rewrite keeps every statement about the code.
- **Free to the account.** Every call, including the `review_format` rewrite
(`anthropic/claude-sonnet-5`, `PR_REVIEW_REWRITE_MODEL` overrides it), runs
on the platform credential and is never debited from the account.
decision that changes output is `review_format`. It sends a flagged native
draft back to its reviewer, whose revision is accepted only when it keeps
the verdict and every finding; it can add findings the draft referred to
without stating, which no rewrite could supply. At publish time it may
replace the wording of a review, never its verdict, check conclusion, or a
finding's title, severity, path, or line, and only after
`review_rewrite_faithful` confirms the rewrite keeps every statement about
the code.
- **Free to the account.** Every decision call, including the `review_format`
rewrite (`anthropic/claude-sonnet-5`, `PR_REVIEW_REWRITE_MODEL` overrides
it), runs on the platform credential and is never debited from the account.
A reviewer's revision is a turn of the review itself, on the review's model,
and is billed like the rest of the review.
- **Never end a run.** The agent execution policy forbids iteration budgets.
`loop_check` records what it sees and nothing more.
- **Second opinion on the uncertain band.** When a definition sets `escalate`
Expand Down
12 changes: 7 additions & 5 deletions lib/decisions/definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,10 @@ const memoryPromotionGate: DecisionDefinition = {

/**
* Decisions that guard what Mogplex publishes on a pull request. The review
* model writes the text; these check its shape before it reaches GitHub and
* that a platform rewrite kept every claim. Both run on the platform's
* credential and are never billed to the account.
* model writes the text; these check its shape before it reaches GitHub, so
* a native reviewer can fix its own draft, and that a platform rewrite kept
* every claim. Both run on the platform's credential and are never billed to
* the account.
*
* Thresholds come from the 2026-09-28 replay of seven published reviews: a
* well-formatted control scored under 0.3 on every question, and each real
Expand Down Expand Up @@ -277,8 +278,9 @@ export function reviewFormatProblems(
const reviewFormat: DecisionDefinition = {
id: "review_format",
version: "2026-09-28.1",
// Acting means a platform-paid rewrite that a second check must accept,
// and the original is published whenever anything fails.
// Acting sends a native draft back to its reviewer, or at publish time
// means a platform-paid rewrite that a second check must accept; the
// original is published whenever anything fails.
defaultMode: "enforce",
timeoutMs: 4000,
escalate: false,
Expand Down
19 changes: 19 additions & 0 deletions lib/workflows/automation-job-agent-runners-shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import {
type AutomationLanguageModel,
type JobContext,
} from "@/lib/workflows/automation-job-types";
import type { ReviewFormatProblem } from "@/lib/decisions/definitions";
import { resolveAutomationSkills } from "@/lib/workflows/automation-job-skills";
import {
findReviewFormatProblems,
prReviewDecisionScope,
} from "@/lib/workflows/pr-review-format-check";
import type { PrReviewHarnessResult } from "@/lib/workflows/pr-review-harness-types";
import { normalizeAutomationAssignmentType } from "@/lib/workflows/automation-job-utils";
import { getAutomationGenerateTimeoutMs } from "@/lib/workflows/automation-model-defaults";

Expand All @@ -38,6 +44,11 @@ export type AutomationAgentDeps = {
loadApprovalSpentWaitMs: typeof loadToolApprovalSpentWaitMs;
/** The node's skills and its agent's attachments; never rejects. */
resolveSkills: typeof resolveAutomationSkills;
/** The review_format check on a reviewer's draft; null when it did not run. */
judgeReviewFormat: (
draft: PrReviewHarnessResult,
context: JobContext
) => Promise<readonly ReviewFormatProblem[] | null>;
};

export const defaultAutomationAgentDeps: AutomationAgentDeps = {
Expand All @@ -46,6 +57,14 @@ export const defaultAutomationAgentDeps: AutomationAgentDeps = {
waitStore: supabaseWaitStore,
loadApprovalSpentWaitMs: loadToolApprovalSpentWaitMs,
resolveSkills: resolveAutomationSkills,
// A flow run carries its job run id in metadata; a standalone review's is
// not in its context, and its rows are found by repo, PR, and time.
judgeReviewFormat: (draft, context) =>
findReviewFormatProblems(draft, prReviewDecisionScope(context, null), {
job_run_id: context.metadata.flow_job_run_id ?? null,
pr_number: context.metadata.pr_number ?? null,
stage: "reviewer_draft",
}),
};

// Applies the tool-approval gate when the flow agent node opted in via
Expand Down
10 changes: 4 additions & 6 deletions lib/workflows/automation-job-agent-runners.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,8 @@ import {
splitRepoFullName,
} from "@/lib/workflows/automation-job-utils";
import { normalizeAutomationAgentResult } from "@/lib/workflows/automation-job-metadata";
import {
fileMissingReviewReport,
type ReportRepairRequest,
} from "@/lib/workflows/pr-review-report-repair";
import type { ReportRepairRequest } from "@/lib/workflows/pr-review-report-repair";
import { finishPrReview } from "@/lib/workflows/pr-review-self-revision";
import {
appendToRunSpec,
buildJobRunSpec,
Expand Down Expand Up @@ -314,13 +312,13 @@ export function createAutomationAgentRunner(
});
if (assignmentType !== "pr_review") return review.normalized;

// A reviewer that ends without its report leaves no verdict; ask for it.
return fileMissingReviewReport({
return finishPrReview({
result: review.normalized,
responseMessages: review.responseMessages,
prompt: runSpec.prompt,
tools,
generate: async (request) => (await generate(request)).normalized,
judge: (draft) => deps.judgeReviewFormat(draft, context),
});
};
}
Expand Down
17 changes: 5 additions & 12 deletions lib/workflows/automation-job-pr-review-finalize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import type {
import { isPrReviewVerdictMissing } from "./pr-review-harness-extraction";
import type { persistJobReviewFindings } from "./automation-job-persistence";
import type { PrReviewReporterState } from "./automation-job-pr-review-reporter";
import { metadataTeamId } from "./automation-job-classify";
import { polishPrReviewForPublish } from "./pr-review-format-check";
import {
polishPrReviewForPublish,
prReviewDecisionScope,
} from "./pr-review-format-check";

export type FinalizePrReviewSuccessResult =
| {
Expand Down Expand Up @@ -167,16 +169,7 @@ export async function finalizePrReviewSuccess(
deps.polishPrReviewForPublish ?? polishPrReviewForPublish
)(
input.reviewHarnessResult,
{
surface: "pr_review",
userId: context.repo.user_id,
teamId:
metadataTeamId(context.metadata) ??
context.repo.product_team_id ??
null,
repoId: context.repo.id,
aiCallId: input.result.aiCallId ?? null,
},
prReviewDecisionScope(context, input.result.aiCallId ?? null),
{ job_run_id: input.jobRunId, pr_number: reviewPrNumber }
);
const publishedOutcome =
Expand Down
2 changes: 2 additions & 0 deletions lib/workflows/automation-job-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ export type AutomationAgentResult = {
usage: AutomationAgentUsage | null;
execution?: AutomationModelExecutionMetadata | null;
aiCallId?: string | null;
/** The review's report was judged and passed the format check inside the run, so publishing does not check it again. */
reviewFormatPassed?: boolean;
};

// Carries the model id that actually executed. It can differ from the pinned
Expand Down
88 changes: 85 additions & 3 deletions lib/workflows/pr-review-format-check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import type { PrReviewReporterState } from "./automation-job-pr-review-reporter";
import type { JobContext } from "./automation-job-types";
import {
findReviewFormatProblems,
polishPrReviewForPublish,
type PrReviewFormatDeps,
type PrReviewRewrite,
Expand Down Expand Up @@ -57,11 +58,20 @@ const withFinding: PrReviewHarnessResult = {
},
};

function handle(id: DecisionId, act: boolean, answers = {}): DecisionHandle {
const ENFORCED: Pick<DecisionHandle, "mode" | "status"> = {
mode: "enforce",
status: "ok",
};

function handle(
id: DecisionId,
act: boolean,
answers = {},
judged = ENFORCED
): DecisionHandle {
return {
id,
mode: "enforce",
status: "ok",
...judged,
verdict: act ? "act" : "pass",
act,
escalated: false,
Expand Down Expand Up @@ -258,6 +268,78 @@ describe("polishPrReviewForPublish", () => {
});
});

describe("findReviewFormatProblems", () => {
const judge = (
judged: Pick<DecisionHandle, "mode" | "status">,
act = false,
available = true
) =>
findReviewFormatProblems(
denseClean,
scope,
{},
{
available: () => available,
decide: async (id) => handle(id, act, flagged, judged),
}
);

it("should name the problems of text it judged and flagged", async () => {
await expect(
judge({ mode: "enforce", status: "ok" }, true)
).resolves.toEqual(["denseParagraph", "danglingReference"]);
});

it("should pass text it judged and did not flag", async () => {
await expect(judge({ mode: "enforce", status: "ok" })).resolves.toEqual([]);
});

it("should never report a pass for text it did not judge", async () => {
await expect(
judge({ mode: "enforce", status: "unavailable" })
).resolves.toBeNull();
await expect(judge({ mode: "off", status: "off" })).resolves.toBeNull();
await expect(judge({ mode: "shadow", status: "ok" })).resolves.toBeNull();
await expect(
judge({ mode: "enforce", status: "ok" }, false, false)
).resolves.toBeNull();
});
});

describe("polishPrReviewForPublish and a review passed inside the run", () => {
it("should publish it without checking it again", async () => {
const { deps, calls } = makeDeps({ formatActs: true });
const passed = { ...denseClean, formatPassed: true };

await expect(
polishPrReviewForPublish(passed, scope, {}, deps)
).resolves.toBe(passed);
expect(calls).toEqual([]);
});

it("should read the pass from the reviewer's result", () => {
const reported = {
text: "Done.",
steps: [
{
toolCalls: [
{
toolName: "reportReview",
input: { hasIssues: false, summary: "Fine." },
},
],
},
],
};

expect(
extractPrReviewHarnessResult({ ...reported, reviewFormatPassed: true })
.formatPassed
).toBe(true);
expect(extractPrReviewHarnessResult(reported).formatPassed).toBeUndefined();
});
});

describe("what a clean structured review publishes", () => {
it("should publish the report's summary and commentBody, never the closing chat message", () => {
const harnessResult = extractPrReviewHarnessResult({
Expand Down
Loading
Loading