diff --git a/packages/core/src/__tests__/pipeline-predicate.test.ts b/packages/core/src/__tests__/pipeline-predicate.test.ts index 6b50a6dc6..f3138cdda 100644 --- a/packages/core/src/__tests__/pipeline-predicate.test.ts +++ b/packages/core/src/__tests__/pipeline-predicate.test.ts @@ -17,6 +17,7 @@ import { asRunId, asStageRunId, collectReferencedStages, + ConfiguredPipelineSchema, evaluateExitPredicates, evaluatePredicate, fromLegacyRoutePredicate, @@ -24,6 +25,7 @@ import { normalizeRoutePredicate, predicateDepth, PredicateSchema, + validateRoutePredicateScope, type Artifact, type Predicate, type PredicateContext, @@ -138,6 +140,14 @@ describe("PredicateSchema — parser", () => { }), ).toBe(2); }); + + it("predicateDepth returns 0 (not -Infinity) on a programmatically-empty combinator", () => { + // Schema rejects `predicates: []` via `.min(1)`, but the TS type permits + // it. `Math.max(...[])` is -Infinity; guarding the empty case keeps the + // depth-cap check (`depth > MAX_PREDICATE_DEPTH`) sane. + expect(predicateDepth({ kind: "and", predicates: [] })).toBe(0); + expect(predicateDepth({ kind: "or", predicates: [] })).toBe(0); + }); }); describe("evaluatePredicate — leaves", () => { @@ -361,6 +371,148 @@ describe("Legacy route predicate bridge", () => { }); expect(evaluatePredicate(pred, ctx(stages))).toBe(false); }); + + it("allSucceeded ignores verdict (v1.1 status-only contract)", () => { + // v1.1's evaluator never consulted verdict — only `status === "succeeded"`. + // The new DSL `all_pass` leaf prefers verdict over status when set. The + // legacy bridge must preserve v1.1 semantics so a stage that sets a + // non-"pass" verdict on a succeeded stage doesn't silently break existing + // route configs. + const stages = { + a: makeStageState("a", { status: "succeeded", verdict: "neutral" }), + b: makeStageState("b", { status: "succeeded", verdict: "fail" }), + }; + const legacy = fromLegacyRoutePredicate({ kind: "allSucceeded", stages: ["a", "b"] }); + expect(evaluatePredicate(legacy, ctx(stages))).toBe(true); + + // Direct (non-legacy) `all_pass` still consults verdict, matching the new + // DSL's documented semantics. + const dsl: Predicate = { kind: "all_pass", stages: ["a"] }; + expect(evaluatePredicate(dsl, ctx(stages))).toBe(false); + }); + + it("anySucceeded ignores verdict (v1.1 status-only contract)", () => { + const stages = { + a: makeStageState("a", { status: "succeeded", verdict: "fail" }), + b: makeStageState("b", { status: "failed" }), + }; + const legacy = fromLegacyRoutePredicate({ kind: "anySucceeded", stages: ["a", "b"] }); + // a is status=succeeded (with verdict=fail). v1.1's anySucceeded fires + // on status alone → true. The bridge must preserve that. + expect(evaluatePredicate(legacy, ctx(stages))).toBe(true); + }); +}); + +describe("validateRoutePredicateScope", () => { + it("accepts predicates with explicit, non-empty stages on every leaf", () => { + expect(validateRoutePredicateScope({ kind: "all_pass", stages: ["a"] })).toEqual([]); + expect( + validateRoutePredicateScope({ + kind: "and", + predicates: [ + { kind: "all_pass", stages: ["a"] }, + { kind: "no_open_findings", stages: ["b"] }, + ], + }), + ).toEqual([]); + }); + + it("rejects leaves with no stages scope", () => { + expect(validateRoutePredicateScope({ kind: "all_pass" })).toHaveLength(1); + expect(validateRoutePredicateScope({ kind: "no_open_findings" })).toHaveLength(1); + expect(validateRoutePredicateScope({ kind: "finding_count_below", n: 3 })).toHaveLength(1); + }); + + it("rejects leaves with empty stages array", () => { + expect(validateRoutePredicateScope({ kind: "all_pass", stages: [] })).toHaveLength(1); + }); + + it("walks transitively into and/or/not", () => { + const problems = validateRoutePredicateScope({ + kind: "and", + predicates: [ + { kind: "all_pass", stages: ["a"] }, + { kind: "not", predicate: { kind: "no_open_findings" } }, + ], + }); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("no_open_findings"); + }); + + it("legacy route predicates always pass (schema enforces min(1) stages)", () => { + expect(validateRoutePredicateScope({ kind: "allSucceeded", stages: ["a"] })).toEqual([]); + }); +}); + +describe("ConfiguredPipelineSchema — routes.when scope enforcement", () => { + const baseStage = { + name: "review", + trigger: { on: ["pr.opened"] }, + executor: { kind: "agent", plugin: "codex", mode: "review" }, + task: { prompt: "x" }, + }; + + it("accepts a DSL route with an explicit stages scope", () => { + const result = ConfiguredPipelineSchema.safeParse({ + stages: [ + baseStage, + { + ...baseStage, + name: "fix", + routes: { when: { kind: "all_pass", stages: ["review"] } }, + }, + ], + }); + expect(result.success).toBe(true); + }); + + it("rejects a DSL route leaf with no stages scope", () => { + // The schema accepts this shape syntactically, but the superRefine + // guard fails it — an unscoped leaf would evaluate over "all stages" + // at trigger time, find the host stage pending, and immediately skip. + const result = ConfiguredPipelineSchema.safeParse({ + stages: [ + baseStage, + { ...baseStage, name: "fix", routes: { when: { kind: "no_open_findings" } } }, + ], + }); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => i.message).join("\n")).toContain("explicit"); + }); + + it("rejects a DSL route with an empty stages scope", () => { + const result = ConfiguredPipelineSchema.safeParse({ + stages: [ + baseStage, + { + ...baseStage, + name: "fix", + routes: { when: { kind: "all_pass", stages: [] } }, + }, + ], + }); + expect(result.success).toBe(false); + }); + + it("rejects an unscoped leaf nested inside and/or/not", () => { + const result = ConfiguredPipelineSchema.safeParse({ + stages: [ + baseStage, + { + ...baseStage, + name: "fix", + routes: { + when: { + kind: "and", + predicates: [{ kind: "all_pass", stages: ["review"] }, { kind: "no_open_findings" }], + }, + }, + }, + ], + }); + expect(result.success).toBe(false); + }); }); describe("collectReferencedStages", () => { diff --git a/packages/core/src/pipeline/config-schema.ts b/packages/core/src/pipeline/config-schema.ts index 29bdcc48e..1b3e52286 100644 --- a/packages/core/src/pipeline/config-schema.ts +++ b/packages/core/src/pipeline/config-schema.ts @@ -18,6 +18,7 @@ import { MAX_PREDICATE_DEPTH, PredicateSchema, predicateDepth, + validateRoutePredicateScope, } from "./predicate.js"; import { asPipelineId, @@ -212,6 +213,20 @@ export const ConfiguredPipelineSchema = z } } } + // Routes-only constraint: every DSL leaf must declare an explicit + // `stages` scope. An unset-scope leaf would evaluate over "all + // stages" at trigger time, find the host stage still `pending`, and + // immediately cascade-skip it. Legacy shapes already require + // `stages` at the schema level. + for (const problem of validateRoutePredicateScope( + routes.when as StageRoutePredicate | Predicate, + )) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["stages", i, "routes", "when"], + message: `Stage "${stage.name}" routes.when: ${problem}.`, + }); + } } if (stage.workspaceClass === "read-siblings") { diff --git a/packages/core/src/pipeline/index.ts b/packages/core/src/pipeline/index.ts index e6e9e806b..5a7211dcc 100644 --- a/packages/core/src/pipeline/index.ts +++ b/packages/core/src/pipeline/index.ts @@ -45,6 +45,7 @@ export { isLegacyRoutePredicate, normalizeRoutePredicate, predicateDepth, + validateRoutePredicateScope, type PredicateContext, } from "./predicate.js"; diff --git a/packages/core/src/pipeline/predicate.ts b/packages/core/src/pipeline/predicate.ts index eb583861d..400eec205 100644 --- a/packages/core/src/pipeline/predicate.ts +++ b/packages/core/src/pipeline/predicate.ts @@ -95,7 +95,11 @@ export function evaluatePredicate(predicate: Predicate, context: PredicateContex case "all_pass": { const scope = predicate.stages ?? context.allStageNames; if (scope.length === 0) return true; - return scope.every((name) => isStagePassing(context.stages[name])); + // Legacy bridge: v1.1's `allSucceeded` / `anySucceeded` checked status + // only — preserve that exact semantics. New DSL callers consult the + // verdict-aware path. + const check = isLegacyAllPass(predicate) ? isStageSucceededByStatus : isStagePassing; + return scope.every((name) => check(context.stages[name])); } case "any_failed": { const scope = predicate.stages ?? context.allStageNames; @@ -124,6 +128,15 @@ function isStagePassing(stage: StageState | undefined): boolean { return stage.status === "succeeded"; } +/** + * Status-only check used by the legacy `allSucceeded` / `anySucceeded` + * bridge. v1.1's evaluator never looked at `verdict`; mirroring that here + * keeps legacy route configs evaluating identically across the upgrade. + */ +function isStageSucceededByStatus(stage: StageState | undefined): boolean { + return stage?.status === "succeeded"; +} + function collectOpenFindings( stagesScope: string[] | undefined, context: PredicateContext, @@ -151,9 +164,13 @@ function collectOpenFindings( * scheduler keeps both shapes accepted so existing pipelines don't need a * rewrite, but evaluation goes through a single code path. * - * - `allSucceeded` → `all_pass` - * - `anySucceeded` → `or(all_pass([s]))` - * - `anyFailed` → `any_failed`. v1.1's `anyFailed` matches `status === + * - `allSucceeded` / `anySucceeded` — v1.1's evaluator checked `status === + * "succeeded"` directly and ignored `verdict`. The new `all_pass` leaf + * prefers `verdict` over `status`, which would (silently) change + * semantics for any stage that sets a non-`"pass"` verdict on a succeeded + * stage. We tag the bridged predicates with an internal marker so the + * evaluator falls back to a status-only check for legacy routes. + * - `anyFailed` → `any_failed`. v1.1's `anyFailed` matches `status === * "failed"` specifically — `not(all_pass)` would (wrongly) also fire for * `skipped` / `outdated` stages, which v1.1's evaluator never treated as * failures. The dedicated `any_failed` leaf preserves the exact semantics. @@ -161,17 +178,43 @@ function collectOpenFindings( export function fromLegacyRoutePredicate(legacy: StageRoutePredicate): Predicate { switch (legacy.kind) { case "allSucceeded": - return { kind: "all_pass", stages: [...legacy.stages] }; + return tagLegacy({ kind: "all_pass", stages: [...legacy.stages] }); case "anySucceeded": return { kind: "or", - predicates: legacy.stages.map((s) => ({ kind: "all_pass", stages: [s] })), + predicates: legacy.stages.map((s) => tagLegacy({ kind: "all_pass" as const, stages: [s] })), }; case "anyFailed": return { kind: "any_failed", stages: [...legacy.stages] }; } } +/** + * Internal marker attached to `all_pass` leaves bridged from v1.1's + * `allSucceeded` / `anySucceeded`. Surfaces as a non-enumerable boolean + * property so it doesn't leak through Zod / JSON serialization (legacy + * routes are parsed via `StageRoutePredicateSchema` and re-bridged on every + * evaluation, so the tag is regenerated rather than persisted). + * + * The evaluator reads this through `isLegacyAllPass` to decide whether to + * consult `verdict` or fall back to status-only — the legacy bridge is the + * only place this gates evaluation. + */ +const LEGACY_TAG: unique symbol = Symbol.for("ao.pipeline.predicate.legacyAllPass"); + +function tagLegacy

(predicate: P): P { + Object.defineProperty(predicate, LEGACY_TAG, { + value: true, + enumerable: false, + configurable: true, + }); + return predicate; +} + +function isLegacyAllPass(predicate: Predicate): boolean { + return (predicate as unknown as Record)[LEGACY_TAG] === true; +} + const LEGACY_KINDS = new Set(["allSucceeded", "anySucceeded", "anyFailed"]); /** Type-guard: is this an old StageRoutePredicate rather than the typed DSL? */ @@ -229,9 +272,63 @@ export function evaluateExitPredicates( return predicates.every((p) => evaluatePredicate(p, context)); } +/** + * Validate that every leaf inside a route predicate has an explicit, non-empty + * `stages` scope. Routes are activation gates — a leaf with no scope defaults + * to "every stage in the run," which makes `arePreconditionsTerminal` trivially + * true at trigger time (`collectReferencedStages` returns nothing to await) and + * lets the predicate evaluate immediately against still-pending stages. The + * common failure mode is `{ kind: "all_pass" }` (no stages) on a stage that + * cascade-skips before it can run, because the stage's own `pending` status + * makes the predicate false. Exit predicates don't have this problem — they + * only fire when every stage is already terminal, so "all stages" is a + * well-defined default there. + * + * Returns a list of human-readable problems; an empty list means the predicate + * is route-safe. Callers attach `stages: [i, "routes", "when"]` etc. to the + * Zod issue context. + */ +export function validateRoutePredicateScope(value: StageRoutePredicate | Predicate): string[] { + if (isLegacyRoutePredicate(value)) { + // Legacy shapes have a required, min(1) `stages` field at the schema + // level; nothing to validate here. + return []; + } + const problems: string[] = []; + walk(value, problems); + return problems; + + function walk(p: Predicate, out: string[]): void { + switch (p.kind) { + case "all_pass": + case "any_failed": + case "no_open_findings": + case "finding_count_below": + if (!p.stages || p.stages.length === 0) { + out.push( + `${p.kind} leaf in routes.when must declare an explicit, non-empty "stages" scope (route predicates evaluate at trigger time, so the "all stages" default would consume the host stage itself and immediately skip it)`, + ); + } + return; + case "and": + case "or": + for (const child of p.predicates) walk(child, out); + return; + case "not": + walk(p.predicate, out); + return; + } + } +} + /** * Predicate-tree depth (defense-in-depth against deeply-nested config). * `0` for a leaf, `1 + max(child depths)` for combinators. + * + * Treats an empty `and` / `or` combinator as a leaf (depth `0`) instead of + * `-Infinity` from `Math.max(...[])`. The Zod schema rejects `predicates: + * []` via `.min(1)`, but a programmatically-constructed Predicate can hit + * this path (e.g. defensive `validatePipelineExitPredicates` below). */ export function predicateDepth(predicate: Predicate): number { switch (predicate.kind) { @@ -242,6 +339,7 @@ export function predicateDepth(predicate: Predicate): number { return 0; case "and": case "or": + if (predicate.predicates.length === 0) return 0; return 1 + Math.max(...predicate.predicates.map(predicateDepth)); case "not": return 1 + predicateDepth(predicate.predicate);