Skip to content

Commit 3063352

Browse files
os-zhuangclaude
andauthored
test(spec): pin DecisionOutputDef.required at the schema level (#4525) (#4561)
* test(spec): pin `DecisionOutputDef.required` at the schema level (#4525) #4525 asked the spec to model `required` on `DecisionOutputDef` so the contract declares what `ApprovalService.decide()` already enforces. The modelling itself landed three days before the issue was filed — cd6b9f2 (`feat(approvals): decisionOutputs may be declared required`, objectui#2955) added the Zod key, its TSDoc, the `strictUnknownKeyError` known-key entry, the `normalizeDecisionOutputs` pass-through, the authorable-surface baseline row and the generated docs table. The issue was recorded from objectui's ledger burn-down, whose derived-type comment ("the spec does not model it yet") was written against an older spec and was already stale. What was genuinely missing is the pin. `DecisionOutputDefSchema` is `.strict()`, so dropping the key would turn every author's `required: true` into an unknown-key rejection — and the only thing standing in the way was `authorable-surface.json`, a REGENERATED baseline that a `gen:schema` run rewrites without comment. Verified by mutation: commenting the key out left all 40 pre-existing cases green, including the `normalizeDecisionOutputs` ones, because that normalizer is hand-written and never consults the schema. Three cases close it — the key parses and round-trips, it stays optional (absent must not become `required: false`; the parsed shape is what ships to every decision UI on `decision_output_defs`), and a non-boolean is rejected rather than coerced. That last one is the AI-authoring case: the runtime compares `d.required === true`, so a coerced `'true'` would declare a constraint the server then never enforces — the same declared-vs-enforced gap #4525 exists to close, one layer down. No behaviour change; no changeset (the feature shipped with its own in cd6b9f2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 * chore: empty changeset — this PR releases nothing (#4525) The "Check Changeset" gate counts changesets ADDED by the PR relative to base, so pointing at cd6b9f2's `.changeset/required-decision-outputs.md` (which shipped the feature itself) does not satisfy it. An empty-frontmatter changeset is the sanctioned form for a PR that bumps no package: the diff is one test file pinning a spec key that already exists, so there is nothing for a consumer to read in a CHANGELOG. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012C2cd7tL8QDoZ2QKN3djJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 24915d2 commit 3063352

2 files changed

Lines changed: 36 additions & 0 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
---
3+
4+
Test-only pin for `DecisionOutputDef.required` (#4525) — releases nothing.

packages/spec/src/automation/approval.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,38 @@ describe('unknown keys are rejected, not stripped (#4001)', () => {
396396
expect(unknownKeyIssue(DecisionOutputDefSchema, { key: 'k', widget: 'user' })!.message)
397397
.toContain('`widget` → `type`');
398398
});
399+
400+
/**
401+
* #4525: `required` is the one decision-output key the RUNTIME enforces —
402+
* `ApprovalService.decide()` refuses an approve whose required outputs are
403+
* blank. The spec has declared it since objectui#2955, but nothing at the
404+
* schema level pinned it: the schema is `.strict()`, so dropping the key
405+
* would turn every author's `required: true` into an unknown-key rejection,
406+
* and the only guard against that was `authorable-surface.json` — a
407+
* REGENERATED baseline, which a `gen:schema` run silently rewrites. These
408+
* cases fail loudly instead, which is what "declared = enforced" needs on
409+
* the declaring side.
410+
*/
411+
it('accepts `required` — the key the runtime enforces (#4525)', () => {
412+
expect(DecisionOutputDefSchema.parse({ key: 'next_reviewers', required: true }))
413+
.toEqual({ key: 'next_reviewers', required: true });
414+
expect(DecisionOutputDefSchema.parse({ key: 'note', required: false }))
415+
.toEqual({ key: 'note', required: false });
416+
});
417+
418+
it('leaves `required` optional — an unflagged output stays unflagged', () => {
419+
// Absent must stay absent rather than defaulting to `false`: the parsed
420+
// shape is what `normalizeDecisionOutputs` ships to every decision UI.
421+
expect(DecisionOutputDefSchema.parse({ key: 'note' })).toEqual({ key: 'note' });
422+
});
423+
424+
it('rejects a non-boolean `required` instead of coercing it', () => {
425+
// A truthy string is exactly how an AI-authored flow would spell it; the
426+
// runtime compares `d.required === true`, so a coerced 'true' would
427+
// declare a constraint the server then never enforces.
428+
expect(() => DecisionOutputDefSchema.parse({ key: 'note', required: 'true' })).toThrow();
429+
expect(() => DecisionOutputDefSchema.parse({ key: 'note', required: 1 })).toThrow();
430+
});
399431
});
400432

401433
it('publishes additionalProperties:false through the JSON schema (Studio + registerFlow)', () => {

0 commit comments

Comments
 (0)