From cf62edb7b47f906f7f4ebf72b66aecb7687d8a83 Mon Sep 17 00:00:00 2001 From: lex Date: Thu, 30 Jul 2026 17:16:48 +0800 Subject: [PATCH] fix(dag): close verified self-review findings with shared settlement gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes verified out of the DAG self-review (workflow dag_04df6bfa6ffe): - capture: enforce JSON Schema type arrays (H1 — typeof guard skipped validation entirely for ["string","null"]) plus scalar constraints (min/max, length, pattern, item counts, additionalProperties:false); warn at create/replan on keywords the subset validator ignores - recovery: enforce the review-result contract on crash-recovered diff reviews (B1 — recovery bypassed spawn's completion gate, letting an unvalidated verdict/stale fingerprint through the deep-mode gate) - eval: numeric comparisons fail loudly on non-finite operands instead of silently evaluating false and cascading condition_false skips (B3) - spawn: pre-admission failures settle once and return an empty fiber instead of Effect.fail, removing the duplicate guard-rejected NodeFailed from loop's catchCause (B5) - settlement converged: spawn and recovery both decide structured-output completion through capture.ts settleCapturedOutput, so the review contract cannot drift between the two paths again Root cause of B1: validateReviewResult had exactly one call site (spawn) while recovery re-settled captured output independently — two settlement points, one contract. --- packages/opencode/src/dag/dag.ts | 19 + packages/opencode/src/dag/review-lifecycle.ts | 2 +- packages/opencode/src/dag/runtime/capture.ts | 184 ++++++-- packages/opencode/src/dag/runtime/eval.ts | 45 +- packages/opencode/src/dag/runtime/recovery.ts | 54 ++- packages/opencode/src/dag/runtime/spawn.ts | 77 ++-- .../dag/dag-review-audit-regressions.test.ts | 399 ++++++++++++++++++ .../test/dag/dag-structured-output.test.ts | 9 +- 8 files changed, 689 insertions(+), 100 deletions(-) create mode 100644 packages/opencode/test/dag/dag-review-audit-regressions.test.ts diff --git a/packages/opencode/src/dag/dag.ts b/packages/opencode/src/dag/dag.ts index 12bf5b4559..531e947a80 100644 --- a/packages/opencode/src/dag/dag.ts +++ b/packages/opencode/src/dag/dag.ts @@ -30,6 +30,7 @@ import { } from "./admission" import { unresolvedReviewOutcomes, validateReviewLifecycle } from "./review-lifecycle" import { conditionReference } from "./runtime/eval" +import { unsupportedSchemaKeywords } from "./runtime/capture" // Re-export domain types export const ID = DagEvent.DagID @@ -204,6 +205,22 @@ function conditionReferenceErrors(nodes: readonly NodeConfig[]): string[] { }) } +// The runtime validator enforces a JSON Schema subset; anything outside it is +// inert. Warn (not reject) at create/replan so authors learn their constraint +// won't fire before a payload silently sails past it. +function warnUnsupportedSchemaKeywords(nodes: readonly NodeConfig[]) { + return Effect.forEach( + nodes.flatMap((node) => { + if (!node.output_schema) return [] + const keywords = unsupportedSchemaKeywords(node.output_schema) + return keywords.length > 0 ? [{ nodeID: node.id, keywords }] : [] + }), + (hit) => + Effect.logWarning("output_schema uses keywords the subset validator does not enforce — they will be ignored at runtime", hit), + { discard: true }, + ) +} + export interface Interface { readonly create: (input: { projectID: string @@ -304,6 +321,7 @@ export const layer = Layer.effect( if (conditionErrors.length > 0) { return yield* Effect.fail(new Error(`Invalid workflow config: ${conditionErrors.join("; ")}`)) } + yield* warnUnsupportedSchemaKeywords(config.nodes) // Enforce the total node ceiling at creation, not only on replan — the // ceiling is a lifetime cap and the initial graph counts toward it. const maxTotalNodes = config.max_total_nodes ?? DEFAULT_WORKFLOW_CONFIG.maxTotalNodes @@ -518,6 +536,7 @@ export const layer = Layer.effect( if (conditionErrors.length > 0) { return yield* Effect.fail(new Error(`Replan rejected: ${conditionErrors.join("; ")}`)) } + yield* warnUnsupportedSchemaKeywords(normalizedFragment.nodes) const maxReplanAttempts = wfConfig?.max_node_replan_attempts ?? DEFAULT_WORKFLOW_CONFIG.maxNodeReplanAttempts const maxTotalNodes = wfConfig?.max_total_nodes ?? DEFAULT_WORKFLOW_CONFIG.maxTotalNodes diff --git a/packages/opencode/src/dag/review-lifecycle.ts b/packages/opencode/src/dag/review-lifecycle.ts index 0fe416a9f3..b6848dd818 100644 --- a/packages/opencode/src/dag/review-lifecycle.ts +++ b/packages/opencode/src/dag/review-lifecycle.ts @@ -73,7 +73,7 @@ export function validateReviewExecutionInput( } export function reviewImplementationFingerprint( - node: NodeConfig, + node: Pick, resolvedMapping: Record, ) { if (node.review?.phase !== "diff") return undefined diff --git a/packages/opencode/src/dag/runtime/capture.ts b/packages/opencode/src/dag/runtime/capture.ts index 1192101698..ab15b09f87 100644 --- a/packages/opencode/src/dag/runtime/capture.ts +++ b/packages/opencode/src/dag/runtime/capture.ts @@ -8,6 +8,8 @@ * via NodeStarted, so each attempt starts with a clean slate). */ +import { validateReviewResult } from "../review-lifecycle" + const schemas = new Map>() export function registerCaptureSlot(sessionID: string, schema: Record): void { @@ -33,21 +35,12 @@ export function validatePayload(sessionID: string, payload: unknown): { ok: true } export function validateAgainstSchema(value: unknown, schema: Record): { ok: true } | { ok: false; error: string } { + // JSON Schema allows `type` to be a single name or an array of names + // (nullable/union types like ["string","null"]) — the value must match one. const type = schema["type"] - if (typeof type === "string") { - if (type === "object" && (typeof value !== "object" || value === null || Array.isArray(value))) - return { ok: false, error: `expected type "object", got ${Array.isArray(value) ? "array" : typeof value}` } - if (type === "array" && !Array.isArray(value)) - return { ok: false, error: `expected type "array", got ${typeof value}` } - if (type === "string" && typeof value !== "string") - return { ok: false, error: `expected type "string", got ${typeof value}` } - if (type === "number" && typeof value !== "number") - return { ok: false, error: `expected type "number", got ${typeof value}` } - if (type === "integer" && (typeof value !== "number" || !Number.isInteger(value))) - return { ok: false, error: `expected type "integer", got ${typeof value === "number" && !Number.isInteger(value) ? "non-integer number" : typeof value}` } - if (type === "boolean" && typeof value !== "boolean") - return { ok: false, error: `expected type "boolean", got ${typeof value}` } - } + const declared = typeof type === "string" ? [type] : Array.isArray(type) ? type.filter((t): t is string => typeof t === "string") : [] + if (declared.length > 0 && !declared.some((t) => matchesScalarType(value, t))) + return { ok: false, error: `expected type ${declared.length === 1 ? `"${declared[0]}"` : JSON.stringify(declared)}, got ${describeType(value)}` } if ("const" in schema && !deepEqual(value, schema["const"])) return { ok: false, error: `expected const ${truncate(JSON.stringify(schema["const"]))}, got ${truncate(JSON.stringify(value))}` } @@ -56,31 +49,74 @@ export function validateAgainstSchema(value: unknown, schema: Record deepEqual(value, v))) return { ok: false, error: `expected one of ${truncate(JSON.stringify(enumVals))}, got ${truncate(JSON.stringify(value))}` } + if (typeof value === "number") { + const minimum = schema["minimum"] + if (typeof minimum === "number" && value < minimum) + return { ok: false, error: `expected minimum ${minimum}, got ${value}` } + const maximum = schema["maximum"] + if (typeof maximum === "number" && value > maximum) + return { ok: false, error: `expected maximum ${maximum}, got ${value}` } + const exclusiveMinimum = schema["exclusiveMinimum"] + if (typeof exclusiveMinimum === "number" && value <= exclusiveMinimum) + return { ok: false, error: `expected exclusiveMinimum ${exclusiveMinimum}, got ${value}` } + const exclusiveMaximum = schema["exclusiveMaximum"] + if (typeof exclusiveMaximum === "number" && value >= exclusiveMaximum) + return { ok: false, error: `expected exclusiveMaximum ${exclusiveMaximum}, got ${value}` } + } + + if (typeof value === "string") { + const minLength = schema["minLength"] + if (typeof minLength === "number" && value.length < minLength) + return { ok: false, error: `expected minLength ${minLength}, got length ${value.length}` } + const maxLength = schema["maxLength"] + if (typeof maxLength === "number" && value.length > maxLength) + return { ok: false, error: `expected maxLength ${maxLength}, got length ${value.length}` } + const pattern = schema["pattern"] + if (typeof pattern === "string" && !safeRegexTest(pattern, value)) + return { ok: false, error: `expected value to match pattern ${pattern}` } + } + + if (Array.isArray(value)) { + const minItems = schema["minItems"] + if (typeof minItems === "number" && value.length < minItems) + return { ok: false, error: `expected minItems ${minItems}, got ${value.length}` } + const maxItems = schema["maxItems"] + if (typeof maxItems === "number" && value.length > maxItems) + return { ok: false, error: `expected maxItems ${maxItems}, got ${value.length}` } + if (schema["uniqueItems"] === true) { + const duplicate = value.findIndex((item, index) => value.slice(0, index).some((prev) => deepEqual(prev, item))) + if (duplicate !== -1) + return { ok: false, error: `expected uniqueItems, found duplicate at index ${duplicate}` } + } + } + const required = schema["required"] - if (Array.isArray(required) && typeof value === "object" && value !== null && !Array.isArray(value)) { - const obj = value as Record + if (Array.isArray(required) && isSchemaObject(value)) { for (const field of required) { - if (typeof field === "string" && !(field in obj)) + if (typeof field === "string" && !(field in value)) return { ok: false, error: `missing required field: "${field}"` } } } const properties = schema["properties"] - if (typeof properties === "object" && properties !== null && typeof value === "object" && value !== null && !Array.isArray(value)) { - const obj = value as Record - const props = properties as Record - for (const [key, propSchema] of Object.entries(props)) { - if (key in obj && typeof propSchema === "object" && propSchema !== null) { - const result = validateAgainstSchema(obj[key], propSchema as Record) + if (isSchemaObject(properties) && isSchemaObject(value)) { + for (const [key, propSchema] of Object.entries(properties)) { + if (key in value && isSchemaObject(propSchema)) { + const result = validateAgainstSchema(value[key], propSchema) if (!result.ok) return { ok: false, error: `field "${key}": ${result.error}` } } } + if (schema["additionalProperties"] === false) { + const extra = Object.keys(value).find((key) => !(key in properties)) + if (extra !== undefined) + return { ok: false, error: `unexpected additional property: "${extra}"` } + } } const items = schema["items"] - if (Array.isArray(value) && typeof items === "object" && items !== null) { + if (Array.isArray(value) && isSchemaObject(items)) { for (let i = 0; i < value.length; i++) { - const result = validateAgainstSchema(value[i], items as Record) + const result = validateAgainstSchema(value[i], items) if (!result.ok) return { ok: false, error: `item[${i}]: ${result.error}` } } } @@ -88,6 +124,104 @@ export function validateAgainstSchema(value: unknown, schema: Record): string[] { + const found = new Set() + const visit = (node: Record) => { + for (const key of Object.keys(node)) { + if (!SUPPORTED_KEYWORDS.has(key)) found.add(key) + } + // Only the boolean `false` form is enforced; the schema form is inert. + if (isSchemaObject(node["additionalProperties"])) found.add("additionalProperties (schema form)") + const properties = node["properties"] + if (isSchemaObject(properties)) { + for (const child of Object.values(properties)) { + if (isSchemaObject(child)) visit(child) + } + } + const items = node["items"] + if (isSchemaObject(items)) visit(items) + // Tuple form items:[...] is not enforced by the validator either — flag it + // and still descend so nested unsupported keywords surface. + if (Array.isArray(items)) { + found.add("items (tuple form)") + for (const child of items) { + if (isSchemaObject(child)) visit(child) + } + } + } + visit(schema) + return [...found].sort() +} + +function isSchemaObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value) +} + +function matchesScalarType(value: unknown, type: string): boolean { + if (type === "object") return typeof value === "object" && value !== null && !Array.isArray(value) + if (type === "array") return Array.isArray(value) + if (type === "string") return typeof value === "string" + if (type === "number") return typeof value === "number" + if (type === "integer") return typeof value === "number" && Number.isInteger(value) + if (type === "boolean") return typeof value === "boolean" + if (type === "null") return value === null + // Unknown type name: permissive, consistent with subset semantics. + return true +} + +function describeType(value: unknown): string { + if (value === null) return "null" + if (Array.isArray(value)) return "array" + if (typeof value === "number" && !Number.isInteger(value)) return "non-integer number" + return typeof value +} + +// Schema patterns come from workflow config; a malformed regex must not crash +// validation, it just fails the constraint. +function safeRegexTest(pattern: string, value: string): boolean { + try { + return new RegExp(pattern).test(value) + } catch { + return false + } +} + function truncate(text: string | undefined): string { if (text === undefined) return "undefined" if (text.length <= 200) return text diff --git a/packages/opencode/src/dag/runtime/eval.ts b/packages/opencode/src/dag/runtime/eval.ts index b63baf80d7..2f9298f888 100644 --- a/packages/opencode/src/dag/runtime/eval.ts +++ b/packages/opencode/src/dag/runtime/eval.ts @@ -20,14 +20,16 @@ const CONDITION_RE = /^(.+?)\s*(==|!=|>=|<=|>|<)\s*(.+)$/ * Supported syntax: `nodeID.output.field == value` or `nodeID.output.field > N`. * * Returns `{ ok: true, value }` — `value` is true (run the node) or false (skip). - * Returns `{ ok: false, error }` when the expression cannot be parsed — the - * caller MUST fail the node rather than running it on an unevaluable condition. + * Returns `{ ok: false, error }` when the expression cannot be parsed, or when + * a numeric comparison's operand is not a number (missing field path, plain + * text output) — the caller MUST fail the node rather than running or silently + * skipping it on an unevaluable condition. * * @example * ```ts * evaluateCondition( - * "explore-src.output.findings.size > 0", - * { "explore-src": { output: { findings: [1,2,3] } } } + * "explore-src.output.findings_count > 0", + * { "explore-src": { output: { findings_count: 3 } } } * ) // → { ok: true, value: true } * ``` */ @@ -44,15 +46,34 @@ export function evaluateCondition( const lhs = resolvePath(lhsRaw.trim(), outputs) const rhs = parseValue(rhsRaw.trim()) - switch (op) { - case "==": return { ok: true, value: lhs === rhs } - case "!=": return { ok: true, value: lhs !== rhs } - case ">": return { ok: true, value: (lhs as number) > (rhs as number) } - case "<": return { ok: true, value: (lhs as number) < (rhs as number) } - case ">=": return { ok: true, value: (lhs as number) >= (rhs as number) } - case "<=": return { ok: true, value: (lhs as number) <= (rhs as number) } - default: return { ok: true, value: true } + // Numeric comparisons on non-numeric or non-finite operands (missing field + // path, plain-text output, NaN, "Infinity" parsed by parseValue) must fail + // the node loudly — the alternative is a silent condition_false skip that + // cascades through required downstream nodes, the same failure mode + // conditionReference guards against at create time. + if (op === ">" || op === "<" || op === ">=" || op === "<=") { + if (typeof lhs !== "number" || !Number.isFinite(lhs)) + return { ok: false, error: `condition "${condition}": left operand resolved to ${describeOperand(lhs)}, expected a finite number` } + if (typeof rhs !== "number" || !Number.isFinite(rhs)) + return { ok: false, error: `condition "${condition}": right operand ${describeOperand(rhs)} is not a finite number` } + if (op === ">") return { ok: true, value: lhs > rhs } + if (op === "<") return { ok: true, value: lhs < rhs } + if (op === ">=") return { ok: true, value: lhs >= rhs } + return { ok: true, value: lhs <= rhs } } + + // CONDITION_RE only produces the six operators; after the numeric block + // only equality remains. + if (op === "==") return { ok: true, value: lhs === rhs } + return { ok: true, value: lhs !== rhs } +} + +function describeOperand(value: unknown): string { + if (value === undefined) return "undefined (field path not found)" + if (value === null) return "null" + if (typeof value === "number") return String(value) + if (typeof value === "string") return `string "${value.length > 50 ? value.slice(0, 50) + "\u2026" : value}"` + return typeof value } /** diff --git a/packages/opencode/src/dag/runtime/recovery.ts b/packages/opencode/src/dag/runtime/recovery.ts index 8d86ba1424..c6ad8d5584 100644 --- a/packages/opencode/src/dag/runtime/recovery.ts +++ b/packages/opencode/src/dag/runtime/recovery.ts @@ -19,16 +19,21 @@ import { Effect, Clock } from "effect" import { Dag } from "../dag" +import type { NodeConfig } from "../dag" import { Session } from "@/session/session" import { SessionID } from "@/session/schema" import type { DagStore } from "@opencode-ai/core/dag/store" import { isTransitionRejection } from "@opencode-ai/core/dag/core/types" +import { reviewImplementationFingerprint } from "../review-lifecycle" +import { resolveInputMapping } from "./eval" +import { settleCapturedOutput } from "./capture" +import type { CapturedSettlement } from "./capture" export function reconcileWorkflow( dagID: string, checkSessionStatus: (childSessionID: string) => Effect.Effect<"active" | "completed" | "failed" | "unknown", Error>, cancelSession?: (sessionID: string) => Effect.Effect, - workflowConfig?: { nodes: { id: string; output_schema?: Record }[] } | undefined, + workflowConfig?: { nodes: Pick[] } | undefined, ): Effect.Effect<{ reconciled: number; ownershipLost: number }, Error, Dag.Service> { return Effect.gen(function* () { const dag = yield* Dag.Service @@ -83,19 +88,15 @@ export function reconcileWorkflow( if (sessionStatus === "completed") { const nodeConfig = workflowConfig?.nodes.find((n) => n.id === node.id) if (nodeConfig?.output_schema) { - if (node.capturedOutput !== undefined && node.capturedOutput !== null) { - yield* settle(node.id, dag.nodeCompleted(dagID, node.id, node.capturedOutput)) - } else { - yield* settle( - node.id, - dag.nodeFailed( - dagID, - node.id, - "output_schema declared but submit_result was never successfully called (recovered)", - "verdict_fail", - ), - ) - } + // Same settlement decision as spawn's completion gate — recovery + // must not become a bypass of the review-result contract again (B1). + const settlement = recoveredSettlement(nodeConfig, nodes, node.capturedOutput) + yield* settle( + node.id, + settlement.kind === "complete" + ? dag.nodeCompleted(dagID, node.id, settlement.output) + : dag.nodeFailed(dagID, node.id, settlement.reason, "verdict_fail"), + ) } else { yield* settle(node.id, dag.nodeCompleted(dagID, node.id, undefined)) } @@ -148,6 +149,31 @@ export function reconcileWorkflow( }) } +/** + * Recovery-side wrapper around the shared settlement decision + * (capture.ts settleCapturedOutput). Resolves the implementation fingerprint + * from durable sibling rows — the same source input_mapping reads from — then + * delegates. An unresolvable fingerprint fails conservatively: re-running the + * review is always safe; completing an unvalidated one is not. + * + * Known asymmetry vs the spawn path: loop.ts passes the fingerprint through + * sanitizeInput before spawning, this path reads the raw durable value. A + * fingerprint the sanitizer would rewrite can therefore only produce a + * spurious mismatch → forced re-run, never a false accept; typical hashes are + * untouched by the sanitizer. + */ +function recoveredSettlement( + nodeConfig: Pick, + rows: readonly DagStore.NodeRow[], + captured: unknown, +): CapturedSettlement { + if (nodeConfig.review?.phase !== "diff") return settleCapturedOutput(captured, undefined, " (recovered)") + const resolved = resolveInputMapping(nodeConfig.input_mapping, (nodeID) => rows.find((row) => row.id === nodeID)?.output) + const fingerprint = reviewImplementationFingerprint(nodeConfig, resolved) + if (!fingerprint) return { kind: "fail", reason: "review implementation fingerprint could not be resolved from durable state (recovered)" } + return settleCapturedOutput(captured, fingerprint, " (recovered)") +} + export function makeSessionStatusChecker( sessions: Session.Interface, ): (childSessionID: string) => Effect.Effect<"active" | "completed" | "failed" | "unknown", Error> { diff --git a/packages/opencode/src/dag/runtime/spawn.ts b/packages/opencode/src/dag/runtime/spawn.ts index bd76bf4255..633bb53fbd 100644 --- a/packages/opencode/src/dag/runtime/spawn.ts +++ b/packages/opencode/src/dag/runtime/spawn.ts @@ -28,12 +28,11 @@ import { deriveSubagentSessionPermission } from "@/agent/subagent-permissions" import { SessionPrompt } from "@/session/prompt" import { Dag } from "../dag" import { DagModel } from "../model" -import { validateReviewResult } from "../review-lifecycle" import { isTransitionRejection } from "@opencode-ai/core/dag/core/types" import type { DagStore } from "@opencode-ai/core/dag/store" import { ModelV2 } from "@opencode-ai/core/model" import { ProviderV2 } from "@opencode-ai/core/provider" -import { registerCaptureSlot, clearCaptureSlot } from "./capture" +import { registerCaptureSlot, clearCaptureSlot, settleCapturedOutput } from "./capture" type PromptParts = SessionPrompt.PromptInput["parts"] @@ -68,12 +67,26 @@ export function spawnNode( const promptSvc = yield* SessionPrompt.Service const scope = yield* Scope.Scope + // Pre-admission failures settle here and return an empty fiber (same + // shape as the !admitted path below) instead of Effect.fail — failing + // would make the caller's catchCause publish a second, guard-rejected + // NodeFailed (noise). + const failWithoutFiber = (reason: string, label: string) => + Effect.gen(function* () { + yield* dag.nodeFailed(input.dagID, input.nodeID, reason, "exec_failed").pipe( + Effect.catchIf( + isTransitionRejection, + () => Effect.logWarning(`nodeFailed (${label}) guard rejected — node already terminal`), + ), + ) + return { fiber: yield* Effect.forkIn(scope)(Effect.void) } + }) + const agent = yield* agentService.get(input.node.workerType).pipe( Effect.catchCause(() => Effect.succeed(undefined)), ) if (!agent) { - yield* dag.nodeFailed(input.dagID, input.nodeID, `unknown worker_type: ${input.node.workerType}`, "exec_failed") - return yield* Effect.fail(new Error(`Unknown worker_type: ${input.node.workerType}`)) + return yield* failWithoutFiber(`unknown worker_type: ${input.node.workerType}`, "unknown worker_type") } const parent = yield* sessions.get(SessionID.make(input.parentSessionID)) @@ -97,8 +110,7 @@ export function spawnNode( parent: parent.model ? { modelID: parent.model.id, providerID: parent.model.providerID } : undefined, }) if (!resolvedModel) { - yield* dag.nodeFailed(input.dagID, input.nodeID, `no model configured for agent: ${agent.name}`, "exec_failed") - return yield* Effect.fail(new Error(`No model configured for agent: ${agent.name}`)) + return yield* failWithoutFiber(`no model configured for agent: ${agent.name}`, "no model") } const model = { modelID: ModelV2.ID.make(resolvedModel.modelID), @@ -227,46 +239,19 @@ export function spawnNode( if (input.outputSchema) { clearCaptureSlot(childSession.id) const updatedNode = yield* dag.store.getNode(input.dagID, input.nodeID).pipe(Effect.orDie) - const captured = updatedNode?.capturedOutput - if (captured !== undefined && captured !== null) { - if (input.reviewImplementationFingerprint) { - const reviewResult = validateReviewResult( - captured, - input.reviewImplementationFingerprint, - ) - if (!reviewResult.valid) { - yield* dag.nodeFailed( - input.dagID, - input.nodeID, - `Review result contract failed: ${reviewResult.errors.join("; ")}`, - "verdict_fail", - ).pipe( - Effect.catchIf( - isTransitionRejection, - () => Effect.logWarning("nodeFailed (review result contract) guard rejected — node already terminal"), - ), - ) - return - } - } - yield* dag.nodeCompleted(input.dagID, input.nodeID, captured).pipe( - Effect.catchIf( - isTransitionRejection, - () => Effect.logWarning("nodeCompleted guard rejected — node already terminal"), - ), - ) - } else { - yield* dag.nodeFailed( - input.dagID, input.nodeID, - "output_schema declared but submit_result was never successfully called", - "verdict_fail", - ).pipe( - Effect.catchIf( - isTransitionRejection, - () => Effect.logWarning("nodeFailed (verdict_fail) guard rejected — node already terminal"), - ), - ) - } + // Single settlement decision shared with crash recovery + // (capture.ts settleCapturedOutput) — the review-result contract + // must never be enforced in one path and not the other. + const settlement = settleCapturedOutput(updatedNode?.capturedOutput, input.reviewImplementationFingerprint) + yield* (settlement.kind === "complete" + ? dag.nodeCompleted(input.dagID, input.nodeID, settlement.output) + : dag.nodeFailed(input.dagID, input.nodeID, settlement.reason, "verdict_fail") + ).pipe( + Effect.catchIf( + isTransitionRejection, + () => Effect.logWarning(`${settlement.kind === "complete" ? "nodeCompleted" : "nodeFailed (verdict_fail)"} guard rejected — node already terminal`), + ), + ) } else { const rawText = resultOpt.value.parts.findLast((p) => p.type === "text")?.text ?? "" if (rawText.trim() === "") { diff --git a/packages/opencode/test/dag/dag-review-audit-regressions.test.ts b/packages/opencode/test/dag/dag-review-audit-regressions.test.ts new file mode 100644 index 0000000000..e0a98b9a8e --- /dev/null +++ b/packages/opencode/test/dag/dag-review-audit-regressions.test.ts @@ -0,0 +1,399 @@ +/** + * Regression suite for the findings verified out of the DAG self-review + * (workflow dag_04df6bfa6ffe3EO429nwGkG1VT) and fixed afterwards: + * + * - H1: validateAgainstSchema skipped ALL type validation when `type` was a + * JSON Schema type array (["string","null"]). + * - B1: reconcileWorkflow completed recovered diff-review nodes from + * capturedOutput without validateReviewResult — the crash-recovery bypass + * of spawn's review completion gate. Includes pins proving the completion + * gate (reviewAccepted) never re-checks fingerprints, which is why the fix + * lives in recovery. + * - B2 (no bug): pins proving loop's validateReviewExecutionInput gate rejects + * diff reviews with empty/missing fingerprints BEFORE spawn, so spawn's + * falsy-fingerprint guard is not a reachable bypass. + * - B3: evaluateCondition returned a silent `false` for numeric comparisons on + * non-numeric operands (missing field path, plain-text output) → silent + * condition_false skip. Now fails loudly. + * - B4: the schema subset validator silently ignored min/max, length, pattern, + * item-count, and additionalProperties constraints. + */ +import { describe, expect, it } from "bun:test" +import { Effect, Layer, Semaphore, Fiber } from "effect" +import { reconcileWorkflow } from "@/dag/runtime/recovery" +import { validateAgainstSchema, unsupportedSchemaKeywords } from "@/dag/runtime/capture" +import { evaluateCondition } from "@/dag/runtime/eval" +import { spawnNode } from "@/dag/runtime/spawn" +import { Dag } from "@/dag/dag" +import type { NodeConfig, WorkflowConfig } from "@/dag/dag" +import { Agent } from "@/agent/agent" +import { Session } from "@/session/session" +import { SessionPrompt } from "@/session/prompt" +import { validateReviewExecutionInput, unresolvedReviewOutcomes } from "@/dag/review-lifecycle" +import type { DagStore } from "@opencode-ai/core/dag/store" +import { makeNodeRow } from "./fixtures" + +type TrackedEvent = { type: string; nodeID: string; output?: unknown; reason?: string; trigger?: string } + +function makeDagLayer(nodes: DagStore.NodeRow[], trackedEvents: TrackedEvent[]) { + return Layer.mock(Dag.Service, { + store: { + getNodes: () => Effect.succeed(nodes), + getNode: (id: string) => Effect.succeed(nodes.find((n) => n.id === id)), + } as unknown as DagStore.Interface, + nodeCompleted: Effect.fn("stub.nodeCompleted")((dagID: string, nodeID: string, output: unknown) => + Effect.sync(() => trackedEvents.push({ type: "nodeCompleted", nodeID, output })), + ), + nodeFailed: Effect.fn("stub.nodeFailed")((dagID: string, nodeID: string, reason: string, trigger: string) => + Effect.sync(() => trackedEvents.push({ type: "nodeFailed", nodeID, reason, trigger })), + ), + }) +} + +// ============================================================================ +// H1 — capture.ts:37 type-array bypass +// ============================================================================ +describe("H1: validateAgainstSchema with JSON Schema type arrays", () => { + it("rejects a number when schema declares type ['string','null']", () => { + const result = validateAgainstSchema(42, { type: ["string", "null"] }) + expect(result.ok).toBe(false) + }) + + it("rejects an object when schema declares type ['number','integer']", () => { + const result = validateAgainstSchema({ sneaky: true }, { type: ["number", "integer"] }) + expect(result.ok).toBe(false) + }) + + it("accepts null when schema declares type ['string','null']", () => { + expect(validateAgainstSchema(null, { type: ["string", "null"] }).ok).toBe(true) + }) + + it("accepts a string when schema declares type ['string','null']", () => { + expect(validateAgainstSchema("ok", { type: ["string", "null"] }).ok).toBe(true) + }) + + it("single-string type behavior unchanged", () => { + expect(validateAgainstSchema("ok", { type: "string" }).ok).toBe(true) + expect(validateAgainstSchema(42, { type: "string" }).ok).toBe(false) + }) + + it("single-string type 'null' is now enforced (was inert before the type-array fix)", () => { + // Deliberate behavior change beyond H1's letter: the old validator had no + // null branch, so { type: "null" } accepted anything. + expect(validateAgainstSchema(null, { type: "null" }).ok).toBe(true) + expect(validateAgainstSchema("x", { type: "null" }).ok).toBe(false) + }) +}) + +// ============================================================================ +// B1 — recovery path completes review nodes without validateReviewResult +// ============================================================================ +describe("B1: reconcileWorkflow review-contract gap", () => { + // A diff review exactly as the normal path would run it: review block + + // fingerprint input_mapping, so spawn.ts:233 WOULD validate this node. + // Recovery must enforce the same contract. + const reviewNodeConfig = { + id: "review-1", + output_schema: { type: "object", required: ["verdict", "implementation_fingerprint"] }, + review: { phase: "diff" as const, implementation_node_id: "implement", verification_node_id: "verify" }, + input_mapping: { + diff: "implement.output.diff", + fingerprint: "implement.output.fingerprint", + verification: "verify.output", + }, + } + const rows = (capturedOutput: unknown) => [ + makeNodeRow({ + id: "implement", + status: "completed", + output: { fingerprint: "current-fp", diff: "diff --git a b" }, + }), + makeNodeRow({ id: "verify", status: "completed", output: { verdict: "PASS" } }), + makeNodeRow({ + id: "review-1", + workerType: "review", + status: "running", + childSessionId: "ses_r", + capturedOutput, + }), + ] + const config = { nodes: [{ id: "implement" }, { id: "verify" }, reviewNodeConfig] } + + it("recovered diff review with invalid verdict must not be completed", async () => { + const events: TrackedEvent[] = [] + const nodes = rows({ verdict: "MAYBE", implementation_fingerprint: "current-fp" }) + await Effect.runPromise( + reconcileWorkflow("wf-1", () => Effect.succeed("completed" as const), undefined, config).pipe( + Effect.provide(makeDagLayer(nodes, events)), + ), + ) + // Recovery enforces the same review contract (verdict ∈ ACCEPT|REJECT) + // as spawn's completion gate — both call settleCapturedOutput. + expect(events).not.toContainEqual(expect.objectContaining({ type: "nodeCompleted", nodeID: "review-1" })) + expect(events).toContainEqual(expect.objectContaining({ type: "nodeFailed", nodeID: "review-1", trigger: "verdict_fail" })) + }) + + it("recovered diff review with stale fingerprint must not be completed", async () => { + const events: TrackedEvent[] = [] + const nodes = rows({ verdict: "ACCEPT", implementation_fingerprint: "stale-fp" }) + await Effect.runPromise( + reconcileWorkflow("wf-1", () => Effect.succeed("completed" as const), undefined, config).pipe( + Effect.provide(makeDagLayer(nodes, events)), + ), + ) + expect(events).not.toContainEqual(expect.objectContaining({ type: "nodeCompleted", nodeID: "review-1" })) + }) + + it("valid recovered diff review still completes", async () => { + const events: TrackedEvent[] = [] + const nodes = rows({ verdict: "ACCEPT", implementation_fingerprint: "current-fp" }) + await Effect.runPromise( + reconcileWorkflow("wf-1", () => Effect.succeed("completed" as const), undefined, config).pipe( + Effect.provide(makeDagLayer(nodes, events)), + ), + ) + expect(events).toContainEqual(expect.objectContaining({ type: "nodeCompleted", nodeID: "review-1" })) + }) + + it("deep-mode completion gate accepts ACCEPT with a stale fingerprint (last line of defense has no fingerprint check)", () => { + const config: WorkflowConfig = { + name: "wf", + mode: "deep", + nodes: [ + node("implement", { output_schema: { type: "object", required: ["fingerprint", "diff"] } }), + node("verify", { depends_on: ["implement"] }), + node("review-1", { + worker_type: "review", + depends_on: ["verify"], + review: { phase: "diff", implementation_node_id: "implement", verification_node_id: "verify" }, + input_mapping: { + diff: "implement.output.diff", + fingerprint: "implement.output.fingerprint", + verification: "verify.output", + }, + condition: "verify.output.verdict == PASS", + output_schema: { type: "object", required: ["verdict", "implementation_fingerprint"] }, + }), + node("finalize", { + depends_on: ["review-1"], + input_mapping: { review: "review-1.output" }, + condition: "review-1.output.verdict == ACCEPT", + }), + ], + } + const rows = [ + { id: "implement", status: "completed", output: { fingerprint: "current-fp", diff: "d" } }, + { id: "verify", status: "completed", output: { verdict: "PASS" } }, + { id: "review-1", status: "completed", output: { verdict: "ACCEPT", implementation_fingerprint: "stale-fp" } }, + { id: "finalize", status: "completed", output: "done" }, + ] + // Documents the residual gap: reviewAccepted checks verdict === ACCEPT and + // a completed final gate, but never re-checks the fingerprint. The chosen + // fix layer is recovery (entry point of unvalidated data); this pin makes + // the read-side behavior explicit. + expect(unresolvedReviewOutcomes(config, rows)).toEqual([]) + }) +}) + +// ============================================================================ +// B2 — is spawn.ts:232's falsy-fingerprint guard reachable for a diff review? +// ============================================================================ +describe("B2: loop-level gate coverage for empty fingerprints", () => { + it("diff review with empty fingerprint evidence is rejected BEFORE spawn (loop.ts:146 gate)", () => { + const review = node("review-1", { + worker_type: "review", + review: { phase: "diff", implementation_node_id: "implement", verification_node_id: "verify" }, + input_mapping: { + diff: "implement.output.diff", + fingerprint: "implement.output.fingerprint", + verification: "verify.output", + }, + }) + const resolved = { diff: "diff --git a b", fingerprint: "", verification: { verdict: "PASS" } } + const result = validateReviewExecutionInput(review, resolved) + expect(result.valid).toBe(false) + expect(result.errors.join(" ")).toContain("fingerprint") + }) + + it("diff review with missing fingerprint mapping is rejected BEFORE spawn", () => { + const review = node("review-1", { + worker_type: "review", + review: { phase: "diff", implementation_node_id: "implement", verification_node_id: "verify" }, + input_mapping: { diff: "implement.output.diff", verification: "verify.output" }, + }) + const resolved = { diff: "diff --git a b", verification: { verdict: "PASS" } } + expect(validateReviewExecutionInput(review, resolved).valid).toBe(false) + }) + + it("review worker without a review block has no contract to enforce (standard mode)", () => { + const review = node("review-1", { worker_type: "review" }) + expect(validateReviewExecutionInput(review, {}).valid).toBe(true) + }) +}) + +// ============================================================================ +// B3 — eval.ts numeric comparison silently false on non-numeric operands +// ============================================================================ +describe("B3: evaluateCondition numeric comparisons", () => { + it("unresolvable field path in a numeric comparison must fail loudly, not skip silently", () => { + // nodeA has plain-text output, so nodeA.output.count resolves to undefined. + // A silent { ok:true, value:false } here would cascade nodeSkipped through + // required downstream nodes; ok:false makes the loop fail the node loudly. + const result = evaluateCondition("a.output.count > 0", { a: { output: "plain text output" } }) + expect(result.ok).toBe(false) + }) + + it("non-numeric operand in a numeric comparison must fail loudly", () => { + const result = evaluateCondition("a.output.label > 5", { a: { output: { label: "not-a-number" } } }) + expect(result.ok).toBe(false) + }) + + it("non-finite operands fail loudly too (parseValue turns 'Infinity' into a number)", () => { + expect(evaluateCondition("a.output.count > Infinity", { a: { output: { count: 1 } } }).ok).toBe(false) + expect(evaluateCondition("a.output.count > 0", { a: { output: { count: Number.POSITIVE_INFINITY } } }).ok).toBe(false) + }) + + it("numeric comparison on real numbers still works", () => { + expect(evaluateCondition("a.output.count > 0", { a: { output: { count: 3 } } })).toEqual({ ok: true, value: true }) + expect(evaluateCondition("a.output.count < 2", { a: { output: { count: 3 } } })).toEqual({ ok: true, value: false }) + }) + + it("equality comparisons on strings unaffected", () => { + expect(evaluateCondition("a.output.verdict == ACCEPT", { a: { output: { verdict: "ACCEPT" } } })).toEqual({ + ok: true, + value: true, + }) + }) +}) + +// ============================================================================ +// B4 — schema validator silently ignores unsupported constraint keywords +// ============================================================================ +describe("B4: validateAgainstSchema unsupported keywords", () => { + it("minimum is enforced", () => { + expect(validateAgainstSchema(5, { type: "number", minimum: 10 }).ok).toBe(false) + expect(validateAgainstSchema(15, { type: "number", minimum: 10 }).ok).toBe(true) + }) + + it("maxLength is enforced", () => { + expect(validateAgainstSchema("toolong", { type: "string", maxLength: 3 }).ok).toBe(false) + }) + + it("additionalProperties:false rejects extra properties", () => { + const schema = { type: "object", properties: { a: { type: "string" } }, additionalProperties: false } + expect(validateAgainstSchema({ a: "x", extra: 1 }, schema).ok).toBe(false) + expect(validateAgainstSchema({ a: "x" }, schema).ok).toBe(true) + }) +}) + +describe("B4: unsupportedSchemaKeywords", () => { + it("reports combinators and $ref the subset validator ignores, including nested ones", () => { + const schema = { + type: "object", + properties: { + a: { oneOf: [{ type: "string" }] }, + b: { type: "array", items: { $ref: "#/defs/x" } }, + }, + allOf: [{ required: ["a"] }], + } + expect(unsupportedSchemaKeywords(schema)).toEqual(["$ref", "allOf", "oneOf"]) + }) + + it("stays silent for fully supported schemas and bare annotations", () => { + const schema = { + type: "object", + title: "result", + description: "…", + required: ["verdict"], + properties: { verdict: { enum: ["ACCEPT", "REJECT"] }, count: { type: "number", minimum: 0 } }, + additionalProperties: false, + } + expect(unsupportedSchemaKeywords(schema)).toEqual([]) + }) + + it("flags inert forms the validator cannot enforce: tuple items and schema-form additionalProperties", () => { + expect(unsupportedSchemaKeywords({ type: "array", items: [{ type: "string" }, { oneOf: [] }] })) + .toEqual(["items (tuple form)", "oneOf"]) + expect(unsupportedSchemaKeywords({ type: "object", additionalProperties: { type: "string" } })) + .toEqual(["additionalProperties (schema form)"]) + }) +}) + +// ============================================================================ +// B5 — spawn early-exit failures settle once, without failing the caller +// ============================================================================ +describe("B5: spawn pre-admission failures", () => { + function makeSpawnHarness() { + const events: { type: string; nodeID: string; reason?: string }[] = [] + const dagLayer = Layer.mock(Dag.Service, { + store: {} as DagStore.Interface, + nodeFailed: Effect.fn("stub.nodeFailed")((dagID: string, nodeID: string, reason: string) => + Effect.sync(() => events.push({ type: "nodeFailed", nodeID, reason })), + ), + }) + const sessionLayer = Layer.mock(Session.Service, { + get: () => Effect.succeed({ id: "ses_parent" as never, permission: [], agent: "build" } as never), + create: () => Effect.succeed({ id: "ses_child" as never } as never), + }) + const promptLayer = Layer.mock(SessionPrompt.Service, {}) + return { events, dagLayer, sessionLayer, promptLayer } + } + + async function runSpawnToSettled(agentLayer: Layer.Layer, harness: ReturnType) { + await Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const result = yield* spawnNode(Semaphore.makeUnsafe(1), { + dagID: "wf-1", + nodeID: "node-1", + node: makeNodeRow(), + parentSessionID: "ses_parent", + promptParts: [{ type: "text", text: "run" }] as never, + }) + yield* Fiber.await(result.fiber) + }), + ).pipe(Effect.provide(Layer.mergeAll(harness.dagLayer, agentLayer, harness.sessionLayer, harness.promptLayer))) as Effect.Effect, + ) + } + + it("unknown worker_type publishes exactly one nodeFailed and resolves (no Effect.fail to the caller)", async () => { + const harness = makeSpawnHarness() + const agentLayer = Layer.mock(Agent.Service, { + get: () => Effect.die(new Error("no such agent")), + }) + // The await itself is the assertion that spawnNode no longer fails — + // before B5 this promise rejected and loop's catchCause published a + // second, guard-rejected NodeFailed. + await runSpawnToSettled(agentLayer, harness) + expect(harness.events).toEqual([ + { type: "nodeFailed", nodeID: "node-1", reason: "unknown worker_type: build" }, + ]) + }) + + it("missing model publishes exactly one nodeFailed and resolves", async () => { + const harness = makeSpawnHarness() + const agentLayer = Layer.mock(Agent.Service, { + get: () => + Effect.succeed({ + name: "general", mode: "all", permission: [], options: {}, description: "", prompt: "", + tools: {}, hooks: {}, + }), + }) + await runSpawnToSettled(agentLayer, harness) + expect(harness.events).toEqual([ + { type: "nodeFailed", nodeID: "node-1", reason: "no model configured for agent: general" }, + ]) + }) +}) + +function node(id: string, overrides: Partial = {}): NodeConfig { + return { + id, + name: id, + worker_type: "build", + depends_on: [], + required: true, + prompt_template: { inline: `Run ${id}` }, + ...overrides, + } +} diff --git a/packages/opencode/test/dag/dag-structured-output.test.ts b/packages/opencode/test/dag/dag-structured-output.test.ts index 5500a9b7cc..5b1e1b8c5f 100644 --- a/packages/opencode/test/dag/dag-structured-output.test.ts +++ b/packages/opencode/test/dag/dag-structured-output.test.ts @@ -140,8 +140,13 @@ describe("evaluateCondition", () => { expect(evaluateCondition('check.output.status == "fail"', outputs)).toEqual({ ok: true, value: false }) }) - it("returns ok:true value:false when path is missing (comparison with undefined)", () => { - expect(evaluateCondition("missing.output.field > 0", {})).toEqual({ ok: true, value: false }) + it("fails loudly when a numeric comparison's path is missing (silent skip was the worst failure mode)", () => { + const result = evaluateCondition("missing.output.field > 0", {}) + expect(result.ok).toBe(false) + }) + + it("equality with a missing path still evaluates (undefined never equals a literal)", () => { + expect(evaluateCondition("missing.output.field == done", {})).toEqual({ ok: true, value: false }) }) })