fix(pipeline): legacy route verdict regression + route scope guard + depth guard
Resolves three issues caught in self-review of PR #1886: 1. **Legacy `allSucceeded`/`anySucceeded` consulted verdict** — `predicate.ts:fromLegacyRoutePredicate`. v1.1's evaluator checked only `status === "succeeded"` and ignored `verdict`. The new bridge mapped `allSucceeded → all_pass`, and `all_pass`'s `isStagePassing` prefers `verdict` over `status`. A stage with `status=succeeded, verdict=neutral` passed v1.1 routes but fails v1.3 routes — silent backward-compat break. Fix: attach a non-enumerable `LEGACY_TAG` symbol to bridged `all_pass` nodes; the evaluator routes through `isStageSucceededByStatus` (status only) for tagged predicates and `isStagePassing` (verdict-aware) for direct DSL callers. The tag is a Symbol-keyed prop so it doesn't leak through JSON serialization (legacy routes are re-bridged on every evaluation anyway, so the tag is regenerated rather than persisted). 2. **Unscoped DSL leaves inside `routes.when` immediately skipped the stage at trigger time** — `predicate.ts` + `config-schema.ts`. A leaf like `{ kind: "no_open_findings" }` (no `stages`) defaults to "all stages in the run." At trigger time `arePreconditionsTerminal` is trivially true (`collectReferencedStages` returns nothing to await), so the scheduler evaluates the predicate against still-pending stages — including the host stage itself — and cascade-skips it before it can run. Schema accepted it but operators got a silently-dead stage. Fix: new `validateRoutePredicateScope` walks the predicate tree and reports every DSL leaf lacking an explicit, non-empty `stages` scope. Wired into `ConfiguredPipelineSchema.superRefine` so config load rejects the shape with a clear message. Legacy shapes are exempt (the discriminated-union schema already requires `min(1)` stages on those). Unscoped leaves remain legal in `exitPredicates`, where "all stages" is a well-defined default at terminal time. 3. **`predicateDepth` returned `-Infinity` on empty combinators** — `predicate.ts:243-245`. `Math.max(...[])` is `-Infinity`, and the subsequent `depth > MAX_PREDICATE_DEPTH` check silently passes. Zod rejects `predicates: []` at parse time, but the TS type permits it, so programmatically-constructed Predicates (engine validation, tests) could hit this path. Fix: short-circuit empty combinators to depth `0`. ## Tests 11 new tests across the existing `pipeline-predicate.test.ts`: - `allSucceeded` ignores verdict (v1.1 status-only contract) - `anySucceeded` ignores verdict (v1.1 status-only contract) - `predicateDepth` returns 0 on empty combinator - `validateRoutePredicateScope` accept/reject coverage (5 tests) - `ConfiguredPipelineSchema` route-scope enforcement (4 tests) All 1386 / 1386 tests passing (previously 1374; +12 new). ## Public API - New named export: `validateRoutePredicateScope` from `pipeline/index.ts`. Pure function, no breaking change.
This commit is contained in:
Harsh Batheja
parent
76ced8ca4a
commit
cc0e034ed6
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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") {
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ export {
|
|||
isLegacyRoutePredicate,
|
||||
normalizeRoutePredicate,
|
||||
predicateDepth,
|
||||
validateRoutePredicateScope,
|
||||
type PredicateContext,
|
||||
} from "./predicate.js";
|
||||
|
||||
|
|
|
|||
|
|
@ -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<P extends Predicate>(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<symbol, unknown>)[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);
|
||||
|
|
|
|||
Loading…
Reference in New Issue