fix(pipeline): address PR review — workspace guard, anyFailed bridge, deep-equal
Resolves three review comments on PR #1886: 1. **WorkspaceGuard (P1)** — tightened to require `dependsOn`. The engine's `collectSiblingArtifacts` walks `dependsOn` transitively and nothing else; routes references and positional neighbors don't seed the BFS, so a stage that passed the guard via `hasPriorStage` or `hasRouteRefs` alone would silently receive an empty artifacts object at runtime. 2. **anyFailed → any_failed (P2)** — added a dedicated `any_failed` leaf to the DSL that checks `status === "failed"` specifically. The previous `not(all_pass)` bridge would have (incorrectly) fired for `skipped` / `outdated` stages too, since `isStagePassing` returns false for those. v1.1's semantics required `status === "failed"`; the dedicated leaf preserves it exactly. The reviewer's suggested fix had the same bug — `or(not(all_pass([s])))` is semantically identical to `not(all_pass)`. 3. **Key-order deep-equal (P2)** — replaced `JSON.stringify` comparator in config-diff.ts with a recursive deep-equal that sorts keys at every object level. A YAML reformatter reordering `executor.config` keys (`{tone,depth}` → `{depth,tone}`) would otherwise stringify to different strings and falsely classify the diff as structural, aborting an in-flight run for a no-op edit. Tests updated to reflect the tightened workspace guard; new tests for `any_failed` semantics and key-order independence. 1374/1374 passing.
This commit is contained in:
Harsh Batheja
parent
e599509adc
commit
76ced8ca4a
|
|
@ -66,6 +66,33 @@ describe("classifyConfigChange — none", () => {
|
|||
]);
|
||||
expect(classifyConfigChange(a, b).kind).toBe("none");
|
||||
});
|
||||
|
||||
it("is key-order independent for nested config (YAML reformatter safe)", () => {
|
||||
// Two pipelines with the same executor.config but keys inserted in a
|
||||
// different order — a JSON.stringify comparator would falsely call this
|
||||
// structural and abort an in-flight run.
|
||||
const a = pipeline([
|
||||
stage("review", {
|
||||
executor: {
|
||||
kind: "agent",
|
||||
plugin: "codex",
|
||||
mode: "review",
|
||||
config: { tone: "strict", depth: 2, nested: { a: 1, b: 2 } },
|
||||
},
|
||||
}),
|
||||
]);
|
||||
const b = pipeline([
|
||||
stage("review", {
|
||||
executor: {
|
||||
kind: "agent",
|
||||
plugin: "codex",
|
||||
mode: "review",
|
||||
config: { depth: 2, nested: { b: 2, a: 1 }, tone: "strict" },
|
||||
},
|
||||
}),
|
||||
]);
|
||||
expect(classifyConfigChange(a, b).kind).toBe("none");
|
||||
});
|
||||
});
|
||||
|
||||
describe("classifyConfigChange — structural", () => {
|
||||
|
|
|
|||
|
|
@ -81,6 +81,7 @@ function ctx(
|
|||
describe("PredicateSchema — parser", () => {
|
||||
it("accepts every leaf kind", () => {
|
||||
expect(PredicateSchema.safeParse({ kind: "all_pass", stages: ["a", "b"] }).success).toBe(true);
|
||||
expect(PredicateSchema.safeParse({ kind: "any_failed", stages: ["a"] }).success).toBe(true);
|
||||
expect(PredicateSchema.safeParse({ kind: "no_open_findings" }).success).toBe(true);
|
||||
expect(PredicateSchema.safeParse({ kind: "finding_count_below", n: 3 }).success).toBe(true);
|
||||
});
|
||||
|
|
@ -163,6 +164,27 @@ describe("evaluatePredicate — leaves", () => {
|
|||
expect(evaluatePredicate({ kind: "all_pass", stages: ["ghost"] }, ctx(stages))).toBe(false);
|
||||
});
|
||||
|
||||
it("any_failed fires only on status==='failed' (not skipped/outdated)", () => {
|
||||
const stages = {
|
||||
a: makeStageState("a", { status: "succeeded" }),
|
||||
b: makeStageState("b", { status: "skipped" }),
|
||||
c: makeStageState("c", { status: "outdated" }),
|
||||
d: makeStageState("d", { status: "failed" }),
|
||||
};
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: ["a"] }, ctx(stages))).toBe(false);
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: ["b"] }, ctx(stages))).toBe(false);
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: ["c"] }, ctx(stages))).toBe(false);
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: ["d"] }, ctx(stages))).toBe(true);
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: ["a", "b", "c"] }, ctx(stages))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("any_failed with empty scope is false (vacuous: no stages = no failures)", () => {
|
||||
const stages = { a: makeStageState("a", { status: "succeeded" }) };
|
||||
expect(evaluatePredicate({ kind: "any_failed", stages: [] }, ctx(stages))).toBe(false);
|
||||
});
|
||||
|
||||
it("no_open_findings counts only open finding artifacts", () => {
|
||||
const stages = { review: makeStageState("review") };
|
||||
const open = makeFinding("review", 0, { status: "open" });
|
||||
|
|
@ -303,10 +325,10 @@ describe("Legacy route predicate bridge", () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it("fromLegacyRoutePredicate maps anyFailed → not(all_pass)", () => {
|
||||
it("fromLegacyRoutePredicate maps anyFailed → any_failed", () => {
|
||||
expect(fromLegacyRoutePredicate({ kind: "anyFailed", stages: ["a"] })).toEqual({
|
||||
kind: "not",
|
||||
predicate: { kind: "all_pass", stages: ["a"] },
|
||||
kind: "any_failed",
|
||||
stages: ["a"],
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -315,7 +337,7 @@ describe("Legacy route predicate bridge", () => {
|
|||
expect(normalizeRoutePredicate(dsl)).toBe(dsl);
|
||||
});
|
||||
|
||||
it("anyFailed semantics: not(all_pass) fires when any stage didn't succeed", () => {
|
||||
it("anyFailed semantics: fires only when a stage actually status='failed'", () => {
|
||||
const stages = {
|
||||
a: makeStageState("a", { status: "succeeded" }),
|
||||
b: makeStageState("b", { status: "failed" }),
|
||||
|
|
@ -323,6 +345,22 @@ describe("Legacy route predicate bridge", () => {
|
|||
const pred = fromLegacyRoutePredicate({ kind: "anyFailed", stages: ["a", "b"] });
|
||||
expect(evaluatePredicate(pred, ctx(stages))).toBe(true);
|
||||
});
|
||||
|
||||
it("anyFailed semantics: does NOT fire for skipped/outdated (v1.1 contract)", () => {
|
||||
// The old bridge `not(all_pass)` would have incorrectly returned true
|
||||
// here because isStagePassing treats skipped/outdated as not-passing.
|
||||
// The dedicated any_failed leaf checks status === "failed" only.
|
||||
const stages = {
|
||||
a: makeStageState("a", { status: "succeeded" }),
|
||||
b: makeStageState("b", { status: "skipped" }),
|
||||
c: makeStageState("c", { status: "outdated" }),
|
||||
};
|
||||
const pred = fromLegacyRoutePredicate({
|
||||
kind: "anyFailed",
|
||||
stages: ["a", "b", "c"],
|
||||
});
|
||||
expect(evaluatePredicate(pred, ctx(stages))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("collectReferencedStages", () => {
|
||||
|
|
|
|||
|
|
@ -2,9 +2,11 @@
|
|||
* v1.3 — workspace class config validation + read-siblings prompt wiring.
|
||||
*
|
||||
* Covers WorkspaceGuard at config load: a stage that declares
|
||||
* `workspaceClass: "read-siblings"` must have an upstream stage to read
|
||||
* from. Also verifies the runtime prompt surfaces sibling artifacts when
|
||||
* the class is set.
|
||||
* `workspaceClass: "read-siblings"` must declare `dependsOn` (the only edge
|
||||
* the engine actually walks for sibling artifact collection). Positional
|
||||
* neighbors and route refs don't qualify — the engine wouldn't collect
|
||||
* their artifacts at runtime. Also verifies the prompt surfaces sibling
|
||||
* artifacts when the class is set.
|
||||
*/
|
||||
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
|
@ -30,7 +32,7 @@ function makeStageInput(name: string, overrides: Record<string, unknown> = {}):
|
|||
}
|
||||
|
||||
describe("WorkspaceGuard — read-siblings", () => {
|
||||
it("rejects a single stage declaring read-siblings (orphan)", () => {
|
||||
it("rejects a single stage declaring read-siblings (no dependsOn)", () => {
|
||||
const result = ConfiguredPipelineSchema.safeParse({
|
||||
stages: [makeStageInput("only", { workspaceClass: "read-siblings" })],
|
||||
});
|
||||
|
|
@ -39,7 +41,7 @@ describe("WorkspaceGuard — read-siblings", () => {
|
|||
const messages = result.error.issues.map((i) => i.message).join("\n");
|
||||
expect(messages).toContain('Stage "only"');
|
||||
expect(messages).toContain("read-siblings");
|
||||
expect(messages).toContain("no upstream stages");
|
||||
expect(messages).toContain("dependsOn");
|
||||
});
|
||||
|
||||
it("rejects the first-declared stage when it claims read-siblings", () => {
|
||||
|
|
@ -62,17 +64,27 @@ describe("WorkspaceGuard — read-siblings", () => {
|
|||
expect(result.success).toBe(true);
|
||||
});
|
||||
|
||||
it("accepts read-siblings when a prior stage is declared (implicit upstream)", () => {
|
||||
it("rejects read-siblings when only a prior stage is declared (no dependsOn)", () => {
|
||||
// Engine's collectSiblingArtifacts walks dependsOn only; positional
|
||||
// neighbors don't seed the BFS, so without dependsOn the prompt block
|
||||
// would silently be empty. Guard must reject.
|
||||
const result = ConfiguredPipelineSchema.safeParse({
|
||||
stages: [
|
||||
makeStageInput("review"),
|
||||
makeStageInput("fix", { workspaceClass: "read-siblings" }),
|
||||
],
|
||||
});
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.success).toBe(false);
|
||||
if (result.success) return;
|
||||
const messages = result.error.issues.map((i) => i.message).join("\n");
|
||||
expect(messages).toContain('Stage "fix"');
|
||||
expect(messages).toContain("dependsOn");
|
||||
});
|
||||
|
||||
it("accepts read-siblings when routes reference an upstream stage", () => {
|
||||
it("rejects read-siblings when only routes reference an upstream stage", () => {
|
||||
// Routes are activation gates, not data edges — they don't seed the
|
||||
// engine's artifact collection. Without dependsOn, sibling artifacts
|
||||
// would be empty at runtime.
|
||||
const result = ConfiguredPipelineSchema.safeParse({
|
||||
stages: [
|
||||
makeStageInput("review"),
|
||||
|
|
@ -82,7 +94,7 @@ describe("WorkspaceGuard — read-siblings", () => {
|
|||
}),
|
||||
],
|
||||
});
|
||||
expect(result.success).toBe(true);
|
||||
expect(result.success).toBe(false);
|
||||
});
|
||||
|
||||
it("accepts independent (default) on a single-stage pipeline", () => {
|
||||
|
|
|
|||
|
|
@ -137,13 +137,38 @@ function arraysEqual<T>(a: readonly T[], b: readonly T[]): boolean {
|
|||
}
|
||||
|
||||
/**
|
||||
* Structural deep-equal for plain-data records. Pipelines/Stages don't
|
||||
* contain Maps, Sets, Dates, or class instances — JSON.stringify is enough
|
||||
* and avoids hand-rolling a recursive comparator that would risk missing
|
||||
* an edge case. Output order isn't deterministic across JS engines for
|
||||
* objects with non-string keys, but Pipeline values are all string-keyed.
|
||||
* Structural deep-equal for plain-data records.
|
||||
*
|
||||
* `JSON.stringify` is NOT safe here: V8 preserves insertion order, so a YAML
|
||||
* reformatter that reorders `executor.config` keys (e.g. `{ tone, depth }` →
|
||||
* `{ depth, tone }`) would stringify to different strings and falsely
|
||||
* classify the diff as structural — aborting a live run for a no-op edit.
|
||||
*
|
||||
* Pipelines/Stages contain only plain values: primitives, arrays, and
|
||||
* string-keyed objects from a parsed YAML/JSON tree. No Maps, Sets, Dates,
|
||||
* class instances, or symbol-keyed properties to worry about. The recursive
|
||||
* comparator below sorts keys lexicographically at every object level so the
|
||||
* comparison is order-independent.
|
||||
*/
|
||||
function deepEqual(a: unknown, b: unknown): boolean {
|
||||
if (a === b) return true;
|
||||
return JSON.stringify(a) === JSON.stringify(b);
|
||||
if (a === null || b === null) return false;
|
||||
if (typeof a !== typeof b) return false;
|
||||
if (typeof a !== "object") return false;
|
||||
|
||||
if (Array.isArray(a)) {
|
||||
if (!Array.isArray(b) || a.length !== b.length) return false;
|
||||
for (let i = 0; i < a.length; i++) if (!deepEqual(a[i], b[i])) return false;
|
||||
return true;
|
||||
}
|
||||
if (Array.isArray(b)) return false;
|
||||
|
||||
const ao = a as Record<string, unknown>;
|
||||
const bo = b as Record<string, unknown>;
|
||||
const aKeys = Object.keys(ao).sort();
|
||||
const bKeys = Object.keys(bo).sort();
|
||||
if (aKeys.length !== bKeys.length) return false;
|
||||
for (let i = 0; i < aKeys.length; i++) if (aKeys[i] !== bKeys[i]) return false;
|
||||
for (const k of aKeys) if (!deepEqual(ao[k], bo[k])) return false;
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -215,16 +215,16 @@ export const ConfiguredPipelineSchema = z
|
|||
}
|
||||
|
||||
if (stage.workspaceClass === "read-siblings") {
|
||||
const hasDependsOn = (stage.dependsOn?.length ?? 0) > 0;
|
||||
const hasRouteRefs = stage.routes
|
||||
? collectReferencedStages(stage.routes.when as StageRoutePredicate | Predicate).length > 0
|
||||
: false;
|
||||
const hasPriorStage = i > 0;
|
||||
if (!hasDependsOn && !hasRouteRefs && !hasPriorStage) {
|
||||
// Engine's `collectSiblingArtifacts` (engine.ts) walks `dependsOn`
|
||||
// transitively and nothing else — route refs and positional
|
||||
// neighbors don't seed the BFS. Reject any read-siblings stage that
|
||||
// wouldn't actually receive artifacts at runtime, so the prompt
|
||||
// block isn't silently omitted.
|
||||
if ((stage.dependsOn?.length ?? 0) === 0) {
|
||||
ctx.addIssue({
|
||||
code: z.ZodIssueCode.custom,
|
||||
path: ["stages", i, "workspaceClass"],
|
||||
message: `Stage "${stage.name}" declares workspaceClass="read-siblings" but has no upstream stages; declare dependsOn or place the stage after the stages it should read.`,
|
||||
message: `Stage "${stage.name}" declares workspaceClass="read-siblings" but has no dependsOn; artifact collection follows only the dependsOn graph at runtime, so sibling artifacts would be empty. Add a dependsOn entry for each upstream stage whose artifacts this stage should read.`,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
@ -344,6 +344,7 @@ function cloneRouteWhen(when: StageRoutePredicate | Predicate): StageRoutePredic
|
|||
function clonePredicate(p: Predicate): Predicate {
|
||||
switch (p.kind) {
|
||||
case "all_pass":
|
||||
case "any_failed":
|
||||
case "no_open_findings":
|
||||
return p.stages !== undefined ? { kind: p.kind, stages: [...p.stages] } : { kind: p.kind };
|
||||
case "finding_count_below":
|
||||
|
|
|
|||
|
|
@ -12,6 +12,11 @@
|
|||
* `succeeded` (or `verdict === "pass"` when set).
|
||||
* Empty scope is vacuously true; the caller is
|
||||
* responsible for choosing a sensible default scope.
|
||||
* - `any_failed` — at least one referenced stage's terminal status
|
||||
* is `failed` specifically (not `skipped` or
|
||||
* `outdated`). Mirrors v1.1's `anyFailed` exactly
|
||||
* so the legacy bridge can preserve semantics for
|
||||
* existing route configs.
|
||||
* - `no_open_findings` — zero finding artifacts in scope have
|
||||
* `status === "open"`.
|
||||
* - `finding_count_below` — strictly fewer than `n` open findings in scope.
|
||||
|
|
@ -47,6 +52,7 @@ const StagesScope = z.array(z.string().min(1)).optional();
|
|||
export const PredicateSchema: z.ZodType<Predicate> = z.lazy(() =>
|
||||
z.discriminatedUnion("kind", [
|
||||
z.object({ kind: z.literal("all_pass"), stages: StagesScope }),
|
||||
z.object({ kind: z.literal("any_failed"), stages: StagesScope }),
|
||||
z.object({ kind: z.literal("no_open_findings"), stages: StagesScope }),
|
||||
z.object({
|
||||
kind: z.literal("finding_count_below"),
|
||||
|
|
@ -91,6 +97,12 @@ export function evaluatePredicate(predicate: Predicate, context: PredicateContex
|
|||
if (scope.length === 0) return true;
|
||||
return scope.every((name) => isStagePassing(context.stages[name]));
|
||||
}
|
||||
case "any_failed": {
|
||||
const scope = predicate.stages ?? context.allStageNames;
|
||||
// Empty scope is vacuously false — "no stages were failed" is the
|
||||
// only consistent answer when the set is empty.
|
||||
return scope.some((name) => context.stages[name]?.status === "failed");
|
||||
}
|
||||
case "no_open_findings": {
|
||||
return collectOpenFindings(predicate.stages, context).length === 0;
|
||||
}
|
||||
|
|
@ -141,9 +153,10 @@ function collectOpenFindings(
|
|||
*
|
||||
* - `allSucceeded` → `all_pass`
|
||||
* - `anySucceeded` → `or(all_pass([s]))`
|
||||
* - `anyFailed` → `not(all_pass)` over the same scope. v1.1's reducer
|
||||
* treats "any non-success terminal status" as failure for routing
|
||||
* purposes, which is exactly what `not all_pass` captures.
|
||||
* - `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.
|
||||
*/
|
||||
export function fromLegacyRoutePredicate(legacy: StageRoutePredicate): Predicate {
|
||||
switch (legacy.kind) {
|
||||
|
|
@ -155,7 +168,7 @@ export function fromLegacyRoutePredicate(legacy: StageRoutePredicate): Predicate
|
|||
predicates: legacy.stages.map((s) => ({ kind: "all_pass", stages: [s] })),
|
||||
};
|
||||
case "anyFailed":
|
||||
return { kind: "not", predicate: { kind: "all_pass", stages: [...legacy.stages] } };
|
||||
return { kind: "any_failed", stages: [...legacy.stages] };
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -187,6 +200,7 @@ export function collectReferencedStages(value: StageRoutePredicate | Predicate):
|
|||
function walk(p: Predicate): void {
|
||||
switch (p.kind) {
|
||||
case "all_pass":
|
||||
case "any_failed":
|
||||
case "no_open_findings":
|
||||
case "finding_count_below":
|
||||
for (const s of p.stages ?? []) refs.add(s);
|
||||
|
|
@ -222,6 +236,7 @@ export function evaluateExitPredicates(
|
|||
export function predicateDepth(predicate: Predicate): number {
|
||||
switch (predicate.kind) {
|
||||
case "all_pass":
|
||||
case "any_failed":
|
||||
case "no_open_findings":
|
||||
case "finding_count_below":
|
||||
return 0;
|
||||
|
|
|
|||
|
|
@ -127,6 +127,7 @@ export type StageRoutePredicate =
|
|||
*/
|
||||
export type Predicate =
|
||||
| { kind: "all_pass"; stages?: string[] }
|
||||
| { kind: "any_failed"; stages?: string[] }
|
||||
| { kind: "no_open_findings"; stages?: string[] }
|
||||
| { kind: "finding_count_below"; n: number; stages?: string[] }
|
||||
| { kind: "and"; predicates: Predicate[] }
|
||||
|
|
|
|||
Loading…
Reference in New Issue