fix(workflow): validate structured agent output - #1324
Conversation
📝 WalkthroughWalkthroughThe workflow engine now validates configured JSON Schemas and structured agent outputs. Invalid cached results are rerun. Invalid live results retry once, become dead results after repeated failure, and do not receive output-token credit. ChangesStructured output validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
788da3c to
a4e9648
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/workflow-engine/src/__tests__/hooks.test.ts (1)
500-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the revalidation warning so the test pins the intended branch.
truncatedandjournalInvalidatedare set by both journal-invalidation branches inhooks.ts: the key-divergence branch at Line 111 and the schema-revalidation branch at Line 94. The current assertions therefore do not distinguish the two. If cached-result revalidation regressed and the key diverged for any reason, this test would still pass.Capture the warn message and assert it. The message text is owned by
hooks.ts, so it is a stable assertion target.💚 Proposed test change
test('invalid legacy structured output journal hit is invalidated and rerun live', async () => { let calls = 0 const truncated: string[] = [] + const warnings: string[] = [] const params: AgentRunParams = { prompt: 'p', schema: STRUCTURED_SCHEMA, } @@ truncated, - loggerWarn: () => {}, + loggerWarn: msg => { + warnings.push(msg) + }, }) @@ expect(truncated).toEqual(['r1']) expect(ctx.journalInvalidated).toBe(true) + expect( + warnings.some(msg => + msg.includes('does not match its structured output schema'), + ), + ).toBe(true) expect(ctx.journal).toHaveLength(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflow-engine/src/__tests__/hooks.test.ts` around lines 500 - 543, Update the test’s loggerWarn setup in the invalid legacy structured output case to capture the warning message, then assert the captured message matches the schema-revalidation warning emitted by hooks.ts. Keep the existing truncated, journalInvalidated, and journal-result assertions so the test specifically distinguishes schema revalidation from key divergence.packages/workflow-engine/src/engine/hooks.ts (1)
358-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the failing property path in
detail.
validateAgainstSchemamaps each Ajv error toe.messageonly. A message such asmust be numberdoes not identify the failing property. The joineddetailis persisted in the journal and is the primary signal for diagnosing aninvalid-structured-outputdeath.Add
e.instancePathinpackages/workflow-engine/src/engine/structuredOutput.tsso each entry reads, for example,/count must be number.♻️ Proposed change in structuredOutput.ts
errors: valid ? [] - : (validate.errors ?? []).map(e => e.message ?? 'validation error'), + : (validate.errors ?? []).map(e => + `${e.instancePath || '(root)'} ${e.message ?? 'validation error'}`.trim(), + ),Note that
packages/workflow-engine/src/__tests__/types.test.tsline 40 uses the bare Ajv message form as illustrative detail text; that test does not assert against real Ajv output, so it stays valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflow-engine/src/engine/hooks.ts` around lines 358 - 361, Update validateAgainstSchema in structuredOutput.ts to map each Ajv error to its instancePath followed by its message, so the joined detail entries identify the failing property (for example, “/count must be number”). Preserve the existing fallback when no errors are present and leave the types test unchanged.packages/workflow-engine/src/__tests__/structuredOutput.test.ts (1)
45-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup the new tests in
describe()blocks across the three test files. The coding guidelines require thedescribe()+test()pattern, and all three files add top-leveltest()calls instead. The shared root cause is that the new tests follow each file's pre-existing flat layout.
packages/workflow-engine/src/__tests__/structuredOutput.test.ts#L45-L59: wrap the threeassertValidJsonSchematests indescribe('assertValidJsonSchema', ...).packages/workflow-engine/src/__tests__/hooks.test.ts#L259-L346: wrap the new structured-output tests indescribe('structured output validation', ...), and group the journal-replay tests at Lines 466-575 under a siblingdescribe.packages/workflow-engine/src/__tests__/types.test.ts#L36-L45: wrap the new round-trip test with the existingAgentRunResultround-trip tests indescribe('AgentRunResult', ...).Grouping the adjacent pre-existing tests in the same pass keeps each file consistent.
As per coding guidelines: "use
describe()+test()pattern with English descriptions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflow-engine/src/__tests__/structuredOutput.test.ts` around lines 45 - 59, Group the tests according to the describe/test convention: in packages/workflow-engine/src/__tests__/structuredOutput.test.ts lines 45-59, wrap the three assertValidJsonSchema tests in describe('assertValidJsonSchema', ...); in packages/workflow-engine/src/__tests__/hooks.test.ts lines 259-346, wrap the structured-output tests in describe('structured output validation', ...) and the journal-replay tests at lines 466-575 in a sibling describe; in packages/workflow-engine/src/__tests__/types.test.ts lines 36-45, place the new round-trip test alongside the existing AgentRunResult round-trip tests within describe('AgentRunResult', ...).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/workflow-engine/src/__tests__/hooks.test.ts`:
- Around line 500-543: Update the test’s loggerWarn setup in the invalid legacy
structured output case to capture the warning message, then assert the captured
message matches the schema-revalidation warning emitted by hooks.ts. Keep the
existing truncated, journalInvalidated, and journal-result assertions so the
test specifically distinguishes schema revalidation from key divergence.
In `@packages/workflow-engine/src/__tests__/structuredOutput.test.ts`:
- Around line 45-59: Group the tests according to the describe/test convention:
in packages/workflow-engine/src/__tests__/structuredOutput.test.ts lines 45-59,
wrap the three assertValidJsonSchema tests in describe('assertValidJsonSchema',
...); in packages/workflow-engine/src/__tests__/hooks.test.ts lines 259-346,
wrap the structured-output tests in describe('structured output validation',
...) and the journal-replay tests at lines 466-575 in a sibling describe; in
packages/workflow-engine/src/__tests__/types.test.ts lines 36-45, place the new
round-trip test alongside the existing AgentRunResult round-trip tests within
describe('AgentRunResult', ...).
In `@packages/workflow-engine/src/engine/hooks.ts`:
- Around line 358-361: Update validateAgainstSchema in structuredOutput.ts to
map each Ajv error to its instancePath followed by its message, so the joined
detail entries identify the failing property (for example, “/count must be
number”). Preserve the existing fallback when no errors are present and leave
the types test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fe83868-fe2b-47f2-8475-a9b8e47bda3f
📒 Files selected for processing (7)
packages/workflow-engine/src/__tests__/hooks.test.tspackages/workflow-engine/src/__tests__/structuredOutput.test.tspackages/workflow-engine/src/__tests__/types.test.tspackages/workflow-engine/src/engine/hooks.tspackages/workflow-engine/src/engine/structuredOutput.tspackages/workflow-engine/src/index.tspackages/workflow-engine/src/types.ts
1 participant
Fixes #1323
根因
structured output 的 schema 校验依赖具体 adapter；workflow engine 的统一结果边界没有二次校验，因此第三方 adapter、fallback runner 或旧 journal 可以把不符合 schema 的
ok结果继续向下游传播。修复
invalid-structured-outputdead reason，并接入严格的一次重试。null、不计 output token，并持久化 dead journal entry。dead -> retry throw可能触发第三次 backend 调用的问题。兼容性
合法 structured output、普通文本成功结果及新格式 dead journal 的行为保持不变。无 schema 的正常成功和一次重试语义保持不变；
dead -> retry throw现在严格止于两次 backend 调用，修复了此前意外触发第三次调用的问题。旧 journal 类型仍向后兼容。测试
git diff --checkmain@987e550基线完整 CICI 证据：
a4e964819ca348295f9a6456f98eae943baad7e2Summary by CodeRabbit
New Features
Bug Fixes