From 76ced8ca4a323f1915c55bcc3d97cbd3f59e9265 Mon Sep 17 00:00:00 2001 From: Harsh Batheja Date: Sat, 16 May 2026 23:09:39 +0530 Subject: [PATCH] =?UTF-8?q?fix(pipeline):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20workspace=20guard,=20anyFailed=20bridge,=20deep-equ?= =?UTF-8?q?al?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../__tests__/pipeline-config-diff.test.ts | 27 +++++++++++ .../src/__tests__/pipeline-predicate.test.ts | 46 +++++++++++++++++-- .../src/__tests__/pipeline-workspace.test.ts | 30 ++++++++---- packages/core/src/pipeline/config-diff.ts | 37 ++++++++++++--- packages/core/src/pipeline/config-schema.ts | 15 +++--- packages/core/src/pipeline/predicate.ts | 23 ++++++++-- packages/core/src/pipeline/types.ts | 1 + 7 files changed, 149 insertions(+), 30 deletions(-) diff --git a/packages/core/src/__tests__/pipeline-config-diff.test.ts b/packages/core/src/__tests__/pipeline-config-diff.test.ts index 0baeecbcc..2aa89b494 100644 --- a/packages/core/src/__tests__/pipeline-config-diff.test.ts +++ b/packages/core/src/__tests__/pipeline-config-diff.test.ts @@ -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", () => { diff --git a/packages/core/src/__tests__/pipeline-predicate.test.ts b/packages/core/src/__tests__/pipeline-predicate.test.ts index db900e348..6b50a6dc6 100644 --- a/packages/core/src/__tests__/pipeline-predicate.test.ts +++ b/packages/core/src/__tests__/pipeline-predicate.test.ts @@ -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", () => { diff --git a/packages/core/src/__tests__/pipeline-workspace.test.ts b/packages/core/src/__tests__/pipeline-workspace.test.ts index 72770c254..5df72ea72 100644 --- a/packages/core/src/__tests__/pipeline-workspace.test.ts +++ b/packages/core/src/__tests__/pipeline-workspace.test.ts @@ -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 = {}): } 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", () => { diff --git a/packages/core/src/pipeline/config-diff.ts b/packages/core/src/pipeline/config-diff.ts index 15b597606..f65ff3e37 100644 --- a/packages/core/src/pipeline/config-diff.ts +++ b/packages/core/src/pipeline/config-diff.ts @@ -137,13 +137,38 @@ function arraysEqual(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; + const bo = b as Record; + 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; } diff --git a/packages/core/src/pipeline/config-schema.ts b/packages/core/src/pipeline/config-schema.ts index f23c790b0..29bdcc48e 100644 --- a/packages/core/src/pipeline/config-schema.ts +++ b/packages/core/src/pipeline/config-schema.ts @@ -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": diff --git a/packages/core/src/pipeline/predicate.ts b/packages/core/src/pipeline/predicate.ts index 55c3e7451..eb583861d 100644 --- a/packages/core/src/pipeline/predicate.ts +++ b/packages/core/src/pipeline/predicate.ts @@ -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 = 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; diff --git a/packages/core/src/pipeline/types.ts b/packages/core/src/pipeline/types.ts index 10a05d23b..9dc2eef91 100644 --- a/packages/core/src/pipeline/types.ts +++ b/packages/core/src/pipeline/types.ts @@ -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[] }