feat(spec): a skill trigger condition's value must have the shape its operator reads (#7113) - #7212
Merged
Merged
Conversation
…ts operator reads (#7113) `SkillTriggerConditionSchema.operator` and `.value` were declared independently, so every operator accepted every shape: `{ operator: 'in', value: 'admin' }` — a membership test whose list is not a list — was spec-valid. The dormant twin of #6227 on `ViewFilterRuleSchema`; the fix mirrors that one (PR #7114) key for key. Dormant is the point: the sole consumer (`SkillRegistry.evaluateCondition`, cloud agent runtime) coerces the scalar itself, so nothing ever failed. What is closed is a second dialect — a consumer-side lenient coercion standing in for a contract the producer never declared. That coercion becomes a no-op here; removing it is a producer-first follow-up in the cloud repo. - `in` / `not_in` (SKILL_TRIGGER_LIST_VALUE_OPERATORS) require an array. - `eq` / `neq` (SKILL_TRIGGER_SCALAR_VALUE_OPERATORS) require a string — `===` on an array is reference identity, so an array comparand is a dead predicate. - `contains` is deliberately unchanged: it has two live branches (string substring, array subset), and #5685 rules against a schema stricter than its runtime. Both vocabularies are exported so producers enumerate from the contract. Authoring impact censused first across this repo and the cloud repo: no real skill authors the scalar-on-set-operator form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HCb6mPxnEjvhKnnka1RNxw
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 106 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
os-zhuang
marked this pull request as ready for review
August 10, 2026 03:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7113.
SkillTriggerConditionSchemadeclaredoperatorandvalueindependently, so every operator accepted every shape.{ field: 'userRole', operator: 'in', value: 'admin' }— a membership test whose list is not a list — was spec-valid. This is the dormant twin of #6227 onViewFilterRuleSchema, and the fix mirrors PR #7114 key for key.Phase 1 — the authorship census (the gate), in full
The PM's ruling was premise-gated: land option A only if no real (non-test) author writes the scalar-on-set-operator form; if any does, stop and report the migration size instead. Measured before writing any code.
This repo —
git grep -n "triggerConditions" -- packages examples content docs28 files, every hit classified:
spec/src/ai/skill.zod.ts,skill.form.ts,stack.zod.ts,conversions/registry.ts,migrations/registry.tsauthorable-surface{,.base}.json,liveness/skill.json,liveness/README.md,undrilled-containers.baseline.json,spec-changes.jsoncontent/docs/ai/*,content/docs/references/ai/skill.mdx,docs/adr/0040,docs/audits/*,docs/qa/platform-checklist/areas/ai.json,docs/protocol-upgrade-guide.mdspec,runtime,mcpplatform-objects/.../translations/*.metadata-forms.generated.ts— label strings, not authored valuesspec/src/ai/skill.test.tsReal authored metadata carrying
triggerConditions: zero. Noexamples/app, no seed, no fixture declares one.The
cloudrepo — cloned read-only at485cbd3Full
git grepfortriggerConditionsreturns 5 files;trigger_conditions(a persisted snake_case column) returns none.content/docs/ai/extending-with-skills.mdx:25{ field: 'objectName', operator: 'in', value: ['order','refund'] }packages/service-ai/src/__tests__/skill-registry.test.ts:94operator: 'eq', value: 'lead'packages/service-ai/src/__tests__/skill-registry.test.ts:106operator: 'in', value: ['lead','opportunity']packages/service-ai/src/skill-registry.ts,agent-runtime.ts,service-ai-studio/.../solution-design-guardrail.test.tsEvery real skill definition in the cloud repo was inspected directly —
service-ai/src/skills/schema-reader-skill.tsandservice-ai-studio/src/skills/{actions-executor,builder-handoff,data-explorer,metadata-authoring,solution-design}-skill.ts. None of the six declarestriggerConditionsat all.Gate verdict: CLEAR
No real (non-test) authorship of the scalar form exists in either repo. Migration size: zero source sites. One in-repo test handed a scalar to all five operators — it was asserting the decoupling itself, and is updated here to enumerate the shape each operator reads.
Not measurable from either repo, and flagged as such in the cloud follow-up: whether any already-persisted tenant skill metadata carries the scalar form.
Phase 2 — the change
packages/spec/src/ai/skill.zod.tsSKILL_TRIGGER_LIST_VALUE_OPERATORS/SKILL_TRIGGER_SCALAR_VALUE_OPERATORS; addedcheckSkillTriggerConditionValueShape+.superRefine(...)onSkillTriggerConditionSchema; TSDoc stating the constraint, citing #7113 and the #6227 precedent, and naming the cloud consumer's coercion as the thing that becomes a no-oppackages/spec/src/ai/skill-trigger-condition-value-shape.test.tspackages/spec/src/ai/skill.test.tsshould accept all operatorsnow enumerates fromSKILL_TRIGGER_LIST_VALUE_OPERATORSinstead of handing a scalar to all fivepackages/spec/api-surface/ai.json,packages/spec/export-origins/ai.json.changeset/skill-trigger-condition-value-shaped-by-operator.mdThe constraint and its deliberate limit:
valuemust bein/not_inlist.includes(fieldValue)— the authored value is the listeq/neq===/!==on an array is reference identity, so an array comparand is a dead predicate —eqnever fires,neqalways doescontainsNo
content/docs/releases/edit, nodocs/adr/**edit.The dispatch said
eq/neq/contains⇒z.string().containsis left accepting both shapes. Measured in the consumer rather than assumed —SkillRegistry.evaluateConditionhas two livecontainsbranches, not one:The array⊆array subset test is real code, and
SkillContextis indexed[extraField: string]: unknown, so an array-valued context field is a shape the runtime is deliberately written for. Constrainingcontainsto a string would make the schema stricter than its runtime — the #5685 error that PR #7114's own pins hold the line against ("it refuses NOTHING ELSE"), and un-declaring a working capability is an ADR-0049 retirement decision, not a rider on a shape fix.in/not_in/eq/neqhave no such branch, which is why they are constrained andcontainsis not.This is the item to overrule if the PM disagrees — it is a one-line change (add
'contains'toSKILL_TRIGGER_SCALAR_VALUE_OPERATORS) plus flipping the twocontainsaccept pins.Reverse verification — direction predicted first
Predictions were written down before the measurement. Method: detach only
.superRefine(...)from the schema (keeping the exported vocabularies, so the test file still imports on the pre-change shape) and re-run — this isolates the behavior change rather than the module surface.in+ scalar refusedinrefuses a scalarnot_inrefuses a scalareqrefuses an arrayneqrefuses an arraytriggerConditions.0.valuein/not_inaccept an arrayin/not_inaccept[](a real predicate)eq/neqaccept a stringcontainsaccepts a string and an arrayvaluereports one non-custom issue7 predicted RED, 7 measured RED — exactly the predicted set, no others. All accept pins green on both sides, which is the half that holds the
containsdecision and the empty-list allowance from regressing.One prediction was wrong and the pin was rewritten rather than the code: a missing
valueon a list operator does not produce the refinement's issue — Zod 4 skips asuperRefinewhen the object's own shape already failed, so only the required-check issue fires. The pin now records that measured behavior (expect(atValue[0].code).not.toBe('custom')) instead of the assumption.Gates
Build ran before any dist-derived regen (the stale-dist trap, #7122), and the dual-snapshot rule was honored — both
api-surface/andexport-origins/regenerated after the real build, since the two vocabularies are new public exports.pnpm --filter @objectstack/spec buildgen:api-surface→gen:export-origins→gen:skill-refs→gen:docscheck:generatedpnpm --filter @objectstack/spec testtypecheck(check:scripts-typecheck+check:test-typecheck)check-adr-0087-registrationSpecial inspection items for the PM
containsdeviation above — the one substantive call that differs from the dispatch.build-docs.tstakes the first docblock in a file as the module blurb. Adding the vocabulary docs displaced the "Skill Trigger Condition Schema" summary and rewrotecontent/docs/references/ai/skill.mdx+skills/objectstack-ai/references/_index.mdwith a truncated sentence. Fixed by keeping the module summary first (it sits above the imports — pre-existing, and now load-bearing enough to carry an explicit warning comment). Both regenerated docs are byte-identical tomainin the final tree; the fix is worth knowing about because it will bite the next person who adds a leading export to a.zod.ts.minor, notmajor. It is a tightening of an authorable surface, which is breaking in principle — justified as non-major by the census finding zero real authored sites. If the release line disagrees, that is a changeset edit, not a code change.SkillSchemais imported at the top of the new test file rather than lazily, and one pin parses a wholeSkill— that is deliberate, proving the refinement travels with thez.array(SkillTriggerConditionSchema)carrier and reports attriggerConditions.0.value.findinglabel only, no pm labels. It covers removingevaluateCondition's scalar coercion once this ships, and explicitly flags the one thing neither repo can measure: whether persisted tenant skill metadata carries the scalar form.Auto-merge deliberately not enabled — the PM lands serially.
Generated by Claude Code