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
18 changes: 18 additions & 0 deletions packages/loopover-miner/lib/ams-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ export type ResolvedAmsPolicy = {
warnings: string[];
};

/** JSON fields for a resolved policy, omitted entirely when there is nothing to surface (#8853). */
export function amsPolicyWarningJsonFields(
resolved: { source: string; warnings: string[] },
): { amsPolicySource: string; amsPolicyWarnings: string[] } | Record<string, never> {
if (resolved.warnings.length === 0) return {};
return { amsPolicySource: resolved.source, amsPolicyWarnings: [...resolved.warnings] };
}

/** Human-readable lines matching discover-cli's `ai-policy warnings` / note phrasing (#8853). */
export function renderAmsPolicyWarnings(resolved: { source: string; warnings: string[] }): string[] {
if (resolved.warnings.length === 0) return [];
return [
`ams-policy warnings: ${resolved.warnings.length}`,
...resolved.warnings.map((warning) => ` ${warning}`),
`ams-policy source: ${resolved.source}`,
];
}

export type AmsPolicyOptions = {
/** Accepted for forward/API compatibility with callers that pass a fetch override; unused today since this
* resolver never fetches (see the module doc comment above). */
Expand Down
34 changes: 22 additions & 12 deletions packages/loopover-miner/lib/attempt-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import { fetchSelfReviewContext } from "./self-review-context.js";
import type { SelfReviewContextFetch, fetchSelfReviewContext as FetchSelfReviewContextFn } from "./self-review-context.js";
import { buildCodingTaskSpec } from "./coding-task-spec.js";
import type { buildCodingTaskSpec as BuildCodingTaskSpecFn } from "./coding-task-spec.js";
import { resolveAmsPolicy } from "./ams-policy.js";
import { amsPolicyWarningJsonFields, renderAmsPolicyWarnings, resolveAmsPolicy } from "./ams-policy.js";
import type { resolveAmsPolicy as ResolveAmsPolicyFn } from "./ams-policy.js";
import {
buildAmsAttemptFailedPayload,
Expand Down Expand Up @@ -203,42 +203,42 @@ export function parseAttemptArgs(args: string[]): ParsedAttemptArgs {
const positional: string[] = [];

for (let index = 0; index < args.length; index += 1) {
const token = args[index]!;
if (token === "--json") {
const arg = args[index]!;
if (arg === "--json") {
options.json = true;
continue;
}
// Opt-in only: resolveCodingAgentModeFromConfig's own default (no agentDryRun override) is "live", not
// "dry_run" -- so #5132's "dry-run is default" acceptance criteria (#2342) has to be enforced HERE, by
// requiring an explicit --live flag before this command will ever request live mode.
if (token === "--live") {
if (arg === "--live") {
options.live = true;
continue;
}
// #4847: distinct from --live's absence above -- --live only ever gated the coding-agent DRIVER's mode,
// but a non---live run still opened every store and made real worktree/claim/ledger writes. --dry-run
// short-circuits BEFORE any of that infrastructure is even opened, guaranteeing zero writes rather than
// merely skipping the driver.
if (token === "--dry-run") {
if (arg === "--dry-run") {
options.dryRun = true;
continue;
}
if (token === "--miner-login") {
if (arg === "--miner-login") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: ATTEMPT_USAGE };
options.minerLogin = value;
index += 1;
continue;
}
if (token === "--base") {
if (arg === "--base") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: ATTEMPT_USAGE };
options.base = value;
index += 1;
continue;
}
if (token.startsWith("-")) return { error: `Unknown option: ${token}` };
positional.push(token);
if (arg.startsWith("-")) return { error: `Unknown option: ${arg}` };
positional.push(arg);
}

if (positional.length !== 2) return { error: ATTEMPT_USAGE };
Expand Down Expand Up @@ -822,11 +822,15 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}
mode,
attemptId,
};
const blockedPayload = { ...blockedResult, ...amsPolicyWarningJsonFields(amsPolicy) };
if (parsed.json) {
console.log(JSON.stringify(blockedResult, null, 2));
console.log(JSON.stringify(blockedPayload, null, 2));
} else {
console.error(
`Attempt for ${parsed.repoFullName}#${parsed.issueNumber} is blocked: this repo's maxConcurrentClaims cap (${minerGoalSpec.spec.maxConcurrentClaims}) is already met (${claimResult.activeClaimCount} active claim(s)).`,
[
...renderAmsPolicyWarnings(amsPolicy),
`Attempt for ${parsed.repoFullName}#${parsed.issueNumber} is blocked: this repo's maxConcurrentClaims cap (${minerGoalSpec.spec.maxConcurrentClaims}) is already met (${claimResult.activeClaimCount} active claim(s)).`,
].join("\n"),
);
}
// blocked_max_concurrent_claims is a real runtime outcome omitted from AttemptCliResult (.d.ts drift).
Expand Down Expand Up @@ -992,6 +996,7 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}
base: parsed.base,
mode,
attemptId,
...amsPolicyWarningJsonFields(amsPolicy),
submissionMode: amsPolicy.spec.submissionMode,
// Every runMinerAttempt outcome carries a real loopResult (#5135's loop needs its genuine turn-usage and
// cost to save real GovernorCapUsage via governor-state.js's saveCapUsage -- nothing else in the codebase
Expand Down Expand Up @@ -1048,7 +1053,12 @@ export async function runAttempt(args: string[], options: RunAttemptOptions = {}
if (parsed.json) {
console.log(JSON.stringify(finalResult, null, 2));
} else {
console.log(`Attempt for ${parsed.repoFullName}#${parsed.issueNumber} finished with outcome: ${result.outcome}.`);
console.log(
[
...renderAmsPolicyWarnings(amsPolicy),
`Attempt for ${parsed.repoFullName}#${parsed.issueNumber} finished with outcome: ${result.outcome}.`,
].join("\n"),
);
}
options.onResult?.(finalResult as AttemptCliResult);

Expand Down
47 changes: 32 additions & 15 deletions packages/loopover-miner/lib/loop-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import type { RunStateStore } from "./run-state.js";
import { runDiscover } from "./discover-cli.js";
import { runAttempt } from "./attempt-cli.js";
import type { AttemptCliResult } from "./attempt-cli.js";
import { resolveAmsPolicy } from "./ams-policy.js";
import { amsPolicyWarningJsonFields, renderAmsPolicyWarnings, resolveAmsPolicy } from "./ams-policy.js";
import { pollPrDisposition, classifyPrDisposition } from "./pr-disposition-poller.js";
import type { PollPrDispositionOptions } from "./pr-disposition-poller.js";
import { pollCheckRuns } from "./ci-poller.js";
Expand Down Expand Up @@ -79,6 +79,8 @@ export type LoopCycleSummary = {
ciConclusion?: CheckRunConclusion | null;
reentered?: boolean;
reasons?: string[];
amsPolicySource?: string;
amsPolicyWarnings?: string[];
};

export type RunLoopOptions = {
Expand Down Expand Up @@ -150,43 +152,43 @@ export function parseLoopArgs(args: string[]): ParsedLoopArgs {
const targets: string[] = [];

for (let index = 0; index < args.length; index += 1) {
const token = args[index]!;
if (token === "--json") {
const arg = args[index]!;
if (arg === "--json") {
options.json = true;
continue;
}
if (token === "--live") {
if (arg === "--live") {
options.live = true;
continue;
}
// #4847: see attempt-cli.js's own --dry-run comment -- distinct from --live's absence, this short-circuits
// BEFORE governor state or any other store is opened, guaranteeing zero discovery/queue/ledger writes.
if (token === "--dry-run") {
if (arg === "--dry-run") {
options.dryRun = true;
continue;
}
if (token === "--search") {
if (arg === "--search") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: LOOP_USAGE };
options.search = value;
index += 1;
continue;
}
if (token === "--miner-login") {
if (arg === "--miner-login") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: LOOP_USAGE };
options.minerLogin = value;
index += 1;
continue;
}
if (token === "--base") {
if (arg === "--base") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: LOOP_USAGE };
options.base = value;
index += 1;
continue;
}
if (token === "--max-cycles") {
if (arg === "--max-cycles") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: LOOP_USAGE };
try {
Expand All @@ -197,7 +199,7 @@ export function parseLoopArgs(args: string[]): ParsedLoopArgs {
index += 1;
continue;
}
if (token === "--cycle-delay-ms") {
if (arg === "--cycle-delay-ms") {
const value = args[index + 1];
if (!value || value.startsWith("-")) return { error: LOOP_USAGE };
try {
Expand All @@ -208,9 +210,9 @@ export function parseLoopArgs(args: string[]): ParsedLoopArgs {
index += 1;
continue;
}
if (token.startsWith("-")) return { error: `Unknown option: ${token}` };
const target = parseRepoTarget(token);
if (!target) return { error: `Repository must be in owner/repo form: ${token}` };
if (arg.startsWith("-")) return { error: `Unknown option: ${arg}` };
const target = parseRepoTarget(arg);
if (!target) return { error: `Repository must be in owner/repo form: ${arg}` };
targets.push(target);
}

Expand Down Expand Up @@ -337,6 +339,7 @@ export async function runLoop(args: string[], options: RunLoopOptions = {}): Pro
const cycles: LoopCycleSummary[] = [];
let sinceSeq = eventLedger.readEvents({}).at(-1)?.seq ?? 0;
let haltReason: string | null = null;
let amsPolicyWithWarnings: { source: string; warnings: string[] } | null = null;

try {
// Checked BEFORE any work at all -- including the very first discovery call -- so an already-active kill
Expand Down Expand Up @@ -420,6 +423,9 @@ export async function runLoop(args: string[], options: RunLoopOptions = {}): Pro
const claimedEntry = claimed;

const amsPolicy = await resolveAmsPolicyFn(claimedEntry.repoFullName, { env });
if (amsPolicy.warnings.length > 0) {
amsPolicyWithWarnings = { source: amsPolicy.source, warnings: amsPolicy.warnings };
}
// Real, SQLite-persisted per-item convergence history (#5677): the dequeueNext claim above already recorded
// this attempt and the markDone/markFailed calls below record the outcome, so reading it back here shares one
// source of truth with attempt-cli.js (#5654) and survives a loop-daemon restart instead of resetting.
Expand Down Expand Up @@ -613,6 +619,7 @@ export async function runLoop(args: string[], options: RunLoopOptions = {}): Pro
ciConclusion,
reentered: reentry.decision.reenter,
reasons: reentry.decision.reasons,
...amsPolicyWarningJsonFields(amsPolicy),
});

if (!reentry.decision.reenter) {
Expand Down Expand Up @@ -644,11 +651,21 @@ export async function runLoop(args: string[], options: RunLoopOptions = {}): Pro
}

// After the max-cycles release block above, haltReason is always set on a clean exit.
const summary = { haltReason, cyclesRun: cycles.length, cycles };
const summary = {
haltReason,
cyclesRun: cycles.length,
cycles,
...(amsPolicyWithWarnings ? amsPolicyWarningJsonFields(amsPolicyWithWarnings) : {}),
};
if (parsed.json) {
console.log(JSON.stringify(summary, null, 2));
} else {
console.log(`Loop finished after ${cycles.length} cycle(s): ${haltReason}.`);
console.log(
[
...(amsPolicyWithWarnings ? renderAmsPolicyWarnings(amsPolicyWithWarnings) : []),
`Loop finished after ${cycles.length} cycle(s): ${haltReason}.`,
].join("\n"),
);
}
return 0;
} catch (error) {
Expand Down
25 changes: 24 additions & 1 deletion test/unit/miner-ams-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ vi.mock("@loopover/engine", async () => {
});

import { DEFAULT_AMS_POLICY_SPEC } from "../../packages/loopover-engine/src/index";
import { resolveAmsPolicy, resolveAmsPolicyConfigPath } from "../../packages/loopover-miner/lib/ams-policy.js";
import { resolveAmsPolicy, resolveAmsPolicyConfigPath, amsPolicyWarningJsonFields, renderAmsPolicyWarnings } from "../../packages/loopover-miner/lib/ams-policy.js";

const roots: string[] = [];

Expand All @@ -29,6 +29,29 @@ describe("resolveAmsPolicyConfigPath (#5132)", () => {
});
});

describe("amsPolicy warning surfacing helpers (#8853)", () => {
it("omits JSON fields and human lines when warnings are empty", () => {
expect(amsPolicyWarningJsonFields({ source: "default", warnings: [] })).toEqual({});
expect(renderAmsPolicyWarnings({ source: "default", warnings: [] })).toEqual([]);
});

it("surfaces source and warnings with discover-cli phrasing when warnings are non-empty", () => {
const resolved = {
source: "local" as const,
warnings: ['AmsPolicySpec field "capLimits" must be a mapping; falling back to defaults.'],
};
expect(amsPolicyWarningJsonFields(resolved)).toEqual({
amsPolicySource: "local",
amsPolicyWarnings: resolved.warnings,
});
expect(renderAmsPolicyWarnings(resolved)).toEqual([
"ams-policy warnings: 1",
' AmsPolicySpec field "capLimits" must be a mapping; falling back to defaults.',
"ams-policy source: local",
]);
});
});

describe("resolveAmsPolicy (#5132)", () => {
it("returns the engine's safe defaults when no local operator policy exists", async () => {
const root = tempRoot();
Expand Down
Loading