From 9debc3e4fa49b231f2c03742108bc1fea4e078bf Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 11:51:07 +0000 Subject: [PATCH] feat(spec): cover all three fold-and-drop aliases, not just `execute` (#3743 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3838 introduced `lintDeprecatedAliases` — the pre-parse pass that reports an alias the parse itself is about to consume — with one rule, for `action.execute`. #3743 predicted the pass would earn its keep beyond that rule. It does: `execute` was never special. The spec has exactly THREE transforms that fold an alias into its canonical key and then drop it from the parsed output, and all three share one failure mode — declare both slots with different values and one is discarded with no signal, invisible to every downstream check because the parse already erased the evidence. Two more rules, same shape, same advisory severity, same two surfaces: field-requiredwhen-conditionalrequired-conflict — `FieldSchema` folds `conditionalRequired` into `requiredWhen` (#3754/#3764); the discarded predicate never gates the field. Covers fields on objects AND on object extensions, and compares the predicate TEXT so a bare string and the `{ dialect, source }` envelope it lowers into read as the same predicate. agent-knowledge-sources-topics-conflict — `AIKnowledgeSchema` folds `knowledge.topics` into `knowledge.sources` (#1878/#1891); the discarded list names RAG sources the agent never recruits from. Compares by SET, so the same sources in a different order stay quiet. The three findings now share one builder, so they phrase the common half identically (which slot wins, that the alias is dropped, delete it) while each still names what its own discarded value would have done. Also corrects `content/docs/ai/agents.mdx`, which documented `knowledge` as `{ topics, indexes }` and used `topics` in all three examples — teaching the deprecated alias as the canonical key, and contradicting `skills/objectstack-ai/SKILL.md`, which already had it right. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JqC9ckaLbmhmytt65Gxi96 --- .changeset/deprecated-alias-conflict-rules.md | 37 +++ content/docs/ai/agents.mdx | 10 +- content/docs/data-modeling/fields.mdx | 8 +- packages/spec/api-surface.json | 2 + packages/spec/src/index.ts | 8 +- .../src/shared/deprecated-aliases.test.ts | 160 +++++++++++- .../spec/src/shared/deprecated-aliases.ts | 227 +++++++++++++++--- packages/spec/src/stack.test.ts | 31 +++ 8 files changed, 439 insertions(+), 44 deletions(-) create mode 100644 .changeset/deprecated-alias-conflict-rules.md diff --git a/.changeset/deprecated-alias-conflict-rules.md b/.changeset/deprecated-alias-conflict-rules.md new file mode 100644 index 0000000000..fac2a0e28a --- /dev/null +++ b/.changeset/deprecated-alias-conflict-rules.md @@ -0,0 +1,37 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): the deprecated-alias warning now covers all three fold-and-drop aliases (#3743 follow-up) + +#3838 introduced `lintDeprecatedAliases` — the pre-parse pass that reports an +alias the parse is about to consume — with one rule, for `action.execute`. The +issue that asked for it predicted the pass would earn its keep beyond that rule, +and it does: `execute` was never special. The spec has exactly **three** +transforms that fold an alias into its canonical key and then drop it from the +parsed output, and all three share the same failure mode — declare both slots +with different values and one of them is discarded with no signal, invisible to +every downstream check because the parse already erased it. + +Two more rules, same shape, same advisory severity, same two surfaces +(`defineStack` at authoring time; `os build` / `os validate` for stacks that skip +strict `defineStack`): + +- **`field-requiredwhen-conditionalrequired-conflict`** — `FieldSchema` folds + `conditionalRequired` into `requiredWhen` (#3754). The discarded predicate + never gates the field. Covers fields on objects *and* on object extensions. + Compares the predicate **text**, so a bare string and the + `{ dialect, source }` envelope it lowers into are recognised as the same + predicate and stay quiet. +- **`agent-knowledge-sources-topics-conflict`** — `AIKnowledgeSchema` folds + `knowledge.topics` into `knowledge.sources` (#1891). The discarded list names + RAG sources the agent never recruits from. Compares by **set**, so the same + sources in a different order stay quiet. + +Neither fails the build; both name the two values and give the one-line fix. + +Also corrects `content/docs/ai/agents.mdx`, which documented `knowledge` as +`{ topics, indexes }` and used `topics` in all three examples — teaching the +deprecated alias as if it were the canonical key, and disagreeing with +`skills/objectstack-ai/SKILL.md`, which already had it right. The examples now +use `sources`. diff --git a/content/docs/ai/agents.mdx b/content/docs/ai/agents.mdx index a776328921..e1ce193058 100644 --- a/content/docs/ai/agents.mdx +++ b/content/docs/ai/agents.mdx @@ -137,7 +137,7 @@ own `ask` / `build` records use exactly these fields): | `model` | Provider + model config (`provider`: `openai` \| `azure_openai` \| `anthropic` \| `local`) | | `skills` | Skill names to attach (the primary Agent → Skill → Tool capability model) | | `tools` | Direct tool **references** `{ type, name, description }` — `type` is `action` \| `flow` \| `query` \| `vector_search`; `name` points at an existing Action/Flow/query | -| `knowledge` | RAG access: `{ topics: string[], indexes: string[] }` | +| `knowledge` | RAG access: `{ sources: string[], indexes: string[] }`. `sources` is the canonical key the renderer reads; `topics` is a deprecated alias folded into it at parse time | There is no `type` field and no fixed agent "type" taxonomy — behaviour comes from persona, instructions, skills, and tools. There are no `triggers` / `schedule` @@ -199,9 +199,9 @@ Always be professional and data-driven.`, { type: 'action', name: 'generate_email', description: 'Generate a personalized email template' }, ], - // RAG access: topics to recruit knowledge from + vector store indexes. + // RAG access: sources to recruit knowledge from + vector store indexes. knowledge: { - topics: ['sales-playbook', 'leads', 'opportunities'], + sources: ['sales-playbook', 'leads', 'opportunities'], indexes: ['sales_docs'], }, }); @@ -239,7 +239,7 @@ Always be empathetic and solution-focused.`, ], knowledge: { - topics: ['support-kb', 'cases'], + sources: ['support-kb', 'cases'], indexes: ['support_docs'], }, }); @@ -313,7 +313,7 @@ Use the data tools to query records and aggregate metrics.`, ], knowledge: { - topics: ['opportunities', 'pipeline'], + sources: ['opportunities', 'pipeline'], indexes: ['sales_docs'], }, }); diff --git a/content/docs/data-modeling/fields.mdx b/content/docs/data-modeling/fields.mdx index 14f82d20e4..5616f66052 100644 --- a/content/docs/data-modeling/fields.mdx +++ b/content/docs/data-modeling/fields.mdx @@ -347,7 +347,13 @@ only guard. Use `requiredWhen` for new metadata; keep `conditionalRequired` only for older packages that already emitted it. The alias is still accepted on input, but it is lowered into `requiredWhen` at parse time and then removed from the parsed metadata, so every consumer reads one canonical slot — if a field -declares both, `requiredWhen` wins and the alias is discarded. +declares both, `requiredWhen` wins and the alias is discarded. Declaring both +with *different* predicates means the `conditionalRequired` one never gates the +field, so it raises an advisory `field-requiredwhen-conditionalrequired-conflict` +warning naming both — from `defineStack` when it parses your config, and from +`os build` / `os validate` for stacks that skip strict `defineStack`. It never +fails the build; the fix is to delete `conditionalRequired`. The same predicate +in both slots is harmless duplication and stays quiet. ## Naming Conventions diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index e0ee9dc4f5..92dc7bd5b5 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2,6 +2,7 @@ ".": [ "ACTION_TARGET_EXECUTE_CONFLICT (const)", "ADMIN_FULL_ACCESS (const)", + "AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT (const)", "ALL_CONVERSIONS (const)", "AUDIENCE_ANCHOR_POSITIONS (const)", "Agent (type)", @@ -51,6 +52,7 @@ "ExpressionMetaSchema (const)", "ExpressionSchema (const)", "F (const)", + "FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT (const)", "GUEST_POSITION (const)", "MAP_SUPPORTED_FIELDS (const)", "MEMBERSHIP_ROLE_ADMIN (const)", diff --git a/packages/spec/src/index.ts b/packages/spec/src/index.ts index b3ae702eb1..b42014a1c1 100644 --- a/packages/spec/src/index.ts +++ b/packages/spec/src/index.ts @@ -110,7 +110,13 @@ export type { MetadataCollectionInput, MapSupportedField, NormalizeStackInputOpt // Pre-parse authoring lint (#3743) — the one window where a deprecated alias is // still visible, since the parse itself resolves and drops it. `defineStack` // warns from here; the CLI runs the same rules over stacks that skip it. -export { lintDeprecatedAliases, formatDeprecatedAliasFinding, ACTION_TARGET_EXECUTE_CONFLICT } from './shared/deprecated-aliases'; +export { + lintDeprecatedAliases, + formatDeprecatedAliasFinding, + ACTION_TARGET_EXECUTE_CONFLICT, + FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT, + AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT, +} from './shared/deprecated-aliases'; export type { DeprecatedAliasFinding } from './shared/deprecated-aliases'; // Metadata conversion layer (ADR-0087 D2) — old-shape → canonical-shape transforms applied at load. diff --git a/packages/spec/src/shared/deprecated-aliases.test.ts b/packages/spec/src/shared/deprecated-aliases.test.ts index 4f1d19deb9..2174cd411c 100644 --- a/packages/spec/src/shared/deprecated-aliases.test.ts +++ b/packages/spec/src/shared/deprecated-aliases.test.ts @@ -1,7 +1,12 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { lintDeprecatedAliases, ACTION_TARGET_EXECUTE_CONFLICT } from './deprecated-aliases'; +import { + lintDeprecatedAliases, + ACTION_TARGET_EXECUTE_CONFLICT, + FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT, + AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT, +} from './deprecated-aliases'; // ── #3743: the discarded `execute` alias now has an author-facing warning ── // @@ -156,3 +161,156 @@ describe('lintDeprecatedAliases — action-target-execute-conflict', () => { ]); }); }); + +// ── The two sibling fold-and-drop aliases (#3743 follow-up) ────────────────── +// +// `execute` was never special: `FieldSchema` folds `conditionalRequired` into +// `requiredWhen` and `AIKnowledgeSchema` folds `topics` into `sources`, both +// dropping the alias from the parsed output exactly the same way. That makes +// all three invisible to every post-parse check, so all three belong to this +// pre-parse pass — which is the reason the pass exists as a rule SET rather +// than one hard-coded rule. + +describe('lintDeprecatedAliases — field-requiredwhen-conditionalrequired-conflict', () => { + const objectWith = (field: Record) => ({ + objects: [{ + name: 'crm_task', + fields: { due_date: { type: 'date', label: 'Due', ...field } }, + }], + }); + + it('flags a field declaring both predicates with different sources', () => { + const findings = lintDeprecatedAliases(objectWith({ + requiredWhen: 'record.stage == "closed"', + conditionalRequired: 'record.amount > 0', + })); + + expect(findings).toHaveLength(1); + const [f] = findings; + expect(f.rule).toBe(FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT); + expect(f.severity).toBe('warning'); + expect(f.where).toBe(`field 'due_date' on object 'crm_task'`); + expect(f.message).toContain('record.stage == "closed"'); + expect(f.message).toContain('record.amount > 0'); + expect(f.message).toContain(`'requiredWhen' wins`); + expect(f.message).toContain('never gates the field'); + expect(f.hint).toContain(`Delete 'conditionalRequired'`); + }); + + it('stays quiet when only one predicate slot is declared', () => { + expect(lintDeprecatedAliases(objectWith({ requiredWhen: 'record.paid' }))).toEqual([]); + expect(lintDeprecatedAliases(objectWith({ conditionalRequired: 'record.paid' }))).toEqual([]); + }); + + it('compares the predicate TEXT, not the wrapper', () => { + // A bare string and the `{ dialect, source }` envelope it lowers into are + // the same predicate written two ways — nothing is lost either way. + expect(lintDeprecatedAliases(objectWith({ + requiredWhen: 'record.paid', + conditionalRequired: { dialect: 'cel', source: 'record.paid' }, + }))).toEqual([]); + }); + + it('reads the envelope form on both slots', () => { + const [f] = lintDeprecatedAliases(objectWith({ + requiredWhen: { dialect: 'cel', source: 'record.a' }, + conditionalRequired: { dialect: 'cel', source: 'record.b' }, + })); + expect(f.rule).toBe(FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT); + expect(f.message).toContain('record.a'); + expect(f.message).toContain('record.b'); + }); + + it('covers fields added by an object extension', () => { + // An extension adds/overrides fields on another package's object through + // the same `FieldSchema`, so it carries the same silent discard. + const [f] = lintDeprecatedAliases({ + objectExtensions: [{ + objectName: 'sys_user', + fields: { nickname: { type: 'text', requiredWhen: 'record.a', conditionalRequired: 'record.b' } }, + }], + }); + expect(f.where).toBe(`field 'nickname' on object extension 'sys_user'`); + }); + + it('tolerates an object with no fields', () => { + expect(lintDeprecatedAliases({ objects: [{ name: 'crm_task' }] })).toEqual([]); + }); +}); + +describe('lintDeprecatedAliases — agent-knowledge-sources-topics-conflict', () => { + const agentWith = (knowledge: Record) => ({ + agents: [{ name: 'support_bot', label: 'Support', knowledge }], + }); + + it('flags an agent declaring both source lists with different members', () => { + const findings = lintDeprecatedAliases(agentWith({ + sources: ['faq', 'policies'], + topics: ['legacy_kb'], + })); + + expect(findings).toHaveLength(1); + const [f] = findings; + expect(f.rule).toBe(AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT); + expect(f.severity).toBe('warning'); + expect(f.where).toBe(`agent 'support_bot' knowledge`); + expect(f.message).toContain(`'faq', 'policies'`); + expect(f.message).toContain(`'legacy_kb'`); + expect(f.message).toContain(`'sources' wins`); + expect(f.message).toContain('never recruited into RAG context'); + expect(f.hint).toContain(`Delete 'topics'`); + }); + + it('stays quiet when only one list is declared', () => { + expect(lintDeprecatedAliases(agentWith({ sources: ['faq'] }))).toEqual([]); + expect(lintDeprecatedAliases(agentWith({ topics: ['faq'] }))).toEqual([]); + }); + + it('stays quiet when the two lists have the same members in any order', () => { + // Order and repetition are not meaningful for either slot, so the same set + // written twice loses nothing. + expect(lintDeprecatedAliases(agentWith({ + sources: ['faq', 'policies'], + topics: ['policies', 'faq', 'faq'], + }))).toEqual([]); + }); + + it('treats an empty list as undeclared', () => { + expect(lintDeprecatedAliases(agentWith({ sources: [], topics: ['faq'] }))).toEqual([]); + }); + + it('tolerates an agent with no knowledge block', () => { + expect(lintDeprecatedAliases({ agents: [{ name: 'support_bot' }] })).toEqual([]); + }); +}); + +describe('lintDeprecatedAliases — all three rules together', () => { + it('reports each rule independently in one pass', () => { + const stack = { + objects: [{ + name: 'crm_task', + fields: { due_date: { type: 'date', requiredWhen: 'record.a', conditionalRequired: 'record.b' } }, + actions: [{ name: 'convert', type: 'script', target: 'preferred', execute: 'legacy' }], + }], + agents: [{ name: 'support_bot', knowledge: { sources: ['faq'], topics: ['legacy_kb'] } }], + }; + + expect(lintDeprecatedAliases(stack).map((f) => f.rule).sort()).toEqual([ + ACTION_TARGET_EXECUTE_CONFLICT, + AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT, + FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT, + ].sort()); + }); + + it('returns nothing for a stack that uses only canonical keys', () => { + const stack = { + objects: [{ + name: 'crm_task', + fields: { due_date: { type: 'date', requiredWhen: 'record.a' } }, + actions: [{ name: 'convert', type: 'script', target: 'preferred' }], + }], + agents: [{ name: 'support_bot', knowledge: { sources: ['faq'] } }], + }; + expect(lintDeprecatedAliases(stack)).toEqual([]); + }); +}); diff --git a/packages/spec/src/shared/deprecated-aliases.ts b/packages/spec/src/shared/deprecated-aliases.ts index 2ad9d85211..b5384dcf61 100644 --- a/packages/spec/src/shared/deprecated-aliases.ts +++ b/packages/spec/src/shared/deprecated-aliases.ts @@ -8,7 +8,7 @@ * every check that runs on parsed metadata — the CLI's `lintFlowPatterns`, * `lintViewRefs`, `@objectstack/lint`'s validators, a renderer, the runtime — is * structurally blind to the one thing worth reporting here: that the author - * declared the alias at all, let alone that they put a *different* handler in it. + * declared the alias at all, let alone that they put a *different* value in it. * * That leaves exactly one window: **after `normalizeStackInput`, before the * parse.** This module is the rule set for that window, kept in `spec` rather @@ -29,20 +29,32 @@ * Each layer warns only for the discards IT performs, so an authored conflict * produces exactly one warning no matter which path the stack takes. * - * Rules: + * ── Scope: every fold-and-drop alias in the spec ────────────────────────────── + * + * A rule belongs here when the transform makes the alias *unrepresentable* on + * the parsed output — that is what puts it out of reach of every downstream + * check. The spec has exactly three such transforms, and all three share one + * failure mode: declare both slots with different values and the alias is + * discarded without a word. * * action-target-execute-conflict — WARNING - * An action declares BOTH the canonical `target` and its deprecated alias - * `execute`, with different values. `target` wins everywhere (#3742) and - * `execute` is discarded — silently, until now: the author wrote two - * handlers and one of them is thrown away with no signal (Prime Directive - * #12). Advisory rather than fatal, because the resulting stack is - * well-defined and shippable; the cost is a handler that never runs, not a - * broken build. Identical values in both slots are harmless duplication and - * stay quiet. + * `ActionSchema` (#3713 / #3742). The discarded `execute` names a handler + * that never runs. + * + * field-requiredwhen-conditionalrequired-conflict — WARNING + * `FieldSchema` (#3754 / #3764). The discarded `conditionalRequired` is a + * predicate that never gates the field. + * + * agent-knowledge-sources-topics-conflict — WARNING + * `AIKnowledgeSchema` (#1878 / #1891). The discarded `topics` names RAG + * sources the agent never recruits from. * - * Advisory-only today, but `severity` is modelled and honoured by every call - * site, so a future rule here can gate the build without rewiring any of them. + * All three are advisory rather than fatal, because the resulting stack is + * well-defined and shippable; the cost is a value that never takes effect, not + * a broken build. Identical values in both slots are harmless duplication — + * nothing the author wrote is lost — and stay quiet. `severity` is modelled and + * honoured by every call site, so promoting a rule gates both CLI surfaces at + * once (#3782) with no rewiring. */ export interface DeprecatedAliasFinding { @@ -64,6 +76,10 @@ export interface DeprecatedAliasFinding { type AnyRec = Record; +export const ACTION_TARGET_EXECUTE_CONFLICT = 'action-target-execute-conflict'; +export const FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT = 'field-requiredwhen-conditionalrequired-conflict'; +export const AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT = 'agent-knowledge-sources-topics-conflict'; + /** Normalise a record-or-map metadata slot into an array, injecting `name` from * the map key (mirrors the helper in the CLI's sibling authoring lints). */ function asArray(v: unknown): AnyRec[] { @@ -72,8 +88,6 @@ function asArray(v: unknown): AnyRec[] { return []; } -export const ACTION_TARGET_EXECUTE_CONFLICT = 'action-target-execute-conflict'; - /** A handler slot counts as declared when it holds a non-empty string ref or an * inline callable. Both forms are authorable and both resolve by the same * `target`-wins precedence. */ @@ -87,6 +101,75 @@ function describeHandler(v: unknown): string { return typeof v === 'function' ? 'an inline function' : `'${String(v)}'`; } +/** Pull the predicate text out of an `ExpressionInput` — a bare string, or the + * `{ dialect, source }` envelope it is lowered into. Anything else is not an + * authored predicate. */ +function expressionSource(v: unknown): string | undefined { + if (typeof v === 'string') return v; + if (v && typeof v === 'object' && typeof (v as AnyRec).source === 'string') return (v as AnyRec).source as string; + return undefined; +} + +function isDeclaredExpression(v: unknown): boolean { + const src = expressionSource(v); + return typeof src === 'string' && src.length > 0; +} + +function describeExpression(v: unknown): string { + return `\`${expressionSource(v)}\``; +} + +/** A knowledge slot counts as declared when it holds a non-empty string array. */ +function isDeclaredList(v: unknown): boolean { + return Array.isArray(v) && v.length > 0; +} + +function describeList(v: unknown): string { + return `[${(v as unknown[]).map((s) => `'${String(s)}'`).join(', ')}]`; +} + +/** Set-equality: `sources: ['a','b']` beside `topics: ['b','a']` recruits the + * same context, so nothing the author wrote is lost and the rule stays quiet. + * Order and repetition are not meaningful for either slot. */ +function sameMembers(a: unknown, b: unknown): boolean { + const norm = (v: unknown) => [...new Set((v as unknown[]).map((x) => String(x)))].sort().join('\u0000'); + return norm(a) === norm(b); +} + +/** The one shape every rule here reports. Sharing the skeleton keeps the three + * messages phrased identically where they say the same thing (which slot wins, + * that the alias is dropped, delete it) while each still names what its own + * discarded value would have done. */ +interface AliasConflict { + /** Sentence subject, e.g. `Action`, `Field`, `Agent knowledge`. */ + subject: string; + where: string; + canonicalKey: string; + aliasKey: string; + /** Rendered values, already quoted/bracketed for display. */ + canonical: string; + alias: string; + /** What the discarded value would have done, e.g. "never runs". */ + lost: string; + /** Rule-specific half of the hint. */ + fix: string; + rule: string; +} + +function aliasFinding(c: AliasConflict): DeprecatedAliasFinding { + return { + where: c.where, + message: + `${c.subject} declares both '${c.canonicalKey}' (${c.canonical}) and the deprecated alias ` + + `'${c.aliasKey}' (${c.alias}). '${c.canonicalKey}' wins: '${c.aliasKey}' is dropped while the stack ` + + `is compiled and never reaches the runtime or a renderer, so ${c.alias} ${c.lost}.`, + hint: + `Delete '${c.aliasKey}' — it is a deprecated alias of '${c.canonicalKey}', not a second value. ${c.fix}`, + rule: c.rule, + severity: 'warning', + }; +} + /** * Collect deprecated-alias findings from a NORMALIZED (not yet parsed) stack. * @@ -98,10 +181,21 @@ export function lintDeprecatedAliases(stack: AnyRec): DeprecatedAliasFinding[] { const findings: DeprecatedAliasFinding[] = []; // An action commonly appears BOTH top-level and nested under its object (the - // loader auto-populates `objects[*].actions` from `actions[*].objectName`), so - // dedupe by identity + both slot values: one authored mistake, one finding. + // loader auto-populates `objects[*].actions` from `actions[*].objectName`), + // and a field can be declared on an object and again on an extension of it. + // Dedupe by rule + identity + both slot values: one authored mistake, one + // finding. `\u0000` is written as an escape, never as a raw byte — a literal + // NUL makes ripgrep treat the whole file as binary and silently drop it from + // every grep-based lint (`scripts/check-nul-bytes.mjs` enforces this). const seen = new Set(); + const firstTime = (rule: string, ...parts: string[]) => { + const key = [rule, ...parts].join('\u0000'); + if (seen.has(key)) return false; + seen.add(key); + return true; + }; + // ── action.target vs action.execute (#3713 / #3742) ──────────────────────── const checkAction = (action: AnyRec, ownerObject?: string) => { if (!action) return; const { target, execute } = action; @@ -111,34 +205,95 @@ export function lintDeprecatedAliases(stack: AnyRec): DeprecatedAliasFinding[] { // lost. Staying quiet keeps the rule's signal-to-noise at 1. if (target === execute) return; - const actionName = typeof action.name === 'string' && action.name ? action.name : '(unnamed)'; - // `\u0000` written as an escape, never as a raw byte: a literal NUL makes - // ripgrep treat the whole file as binary and silently drop it from every - // grep-based lint (`scripts/check-nul-bytes.mjs` enforces this). - const dedupeKey = `${actionName}\u0000${describeHandler(target)}\u0000${describeHandler(execute)}`; - if (seen.has(dedupeKey)) return; - seen.add(dedupeKey); - - findings.push({ - where: ownerObject ? `action '${actionName}' on object '${ownerObject}'` : `action '${actionName}'`, - message: - `Action declares both 'target' (${describeHandler(target)}) and the deprecated alias 'execute' ` + - `(${describeHandler(execute)}). 'target' wins: 'execute' is dropped while the stack is compiled and ` + - `never reaches the runtime or a renderer, so ${describeHandler(execute)} never runs.`, - hint: - `Delete 'execute' — it is a deprecated alias of 'target', not a second handler. If ` + - `${describeHandler(execute)} is the handler you meant to bind, put it in 'target' instead.`, + const name = typeof action.name === 'string' && action.name ? action.name : '(unnamed)'; + const canonical = describeHandler(target); + const alias = describeHandler(execute); + if (!firstTime(ACTION_TARGET_EXECUTE_CONFLICT, name, canonical, alias)) return; + + findings.push(aliasFinding({ + subject: 'Action', + where: ownerObject ? `action '${name}' on object '${ownerObject}'` : `action '${name}'`, + canonicalKey: 'target', + aliasKey: 'execute', + canonical, + alias, + lost: 'never runs', + fix: `If ${alias} is the handler you meant to bind, put it in 'target' instead.`, rule: ACTION_TARGET_EXECUTE_CONFLICT, - severity: 'warning', - }); + })); }; - // Object-nested first so the retained (deduped) finding keeps object context. + // ── field.requiredWhen vs field.conditionalRequired (#3754 / #3764) ──────── + const checkField = (field: AnyRec, ownerLabel: string) => { + if (!field) return; + const { requiredWhen, conditionalRequired } = field; + if (!isDeclaredExpression(requiredWhen) || !isDeclaredExpression(conditionalRequired)) return; + // Compare the predicate TEXT, not the wrapper: `'record.paid'` and + // `{ dialect: 'cel', source: 'record.paid' }` are the same predicate written + // two ways, and neither loses anything. + if (expressionSource(requiredWhen) === expressionSource(conditionalRequired)) return; + + const name = typeof field.name === 'string' && field.name ? field.name : '(unnamed)'; + const canonical = describeExpression(requiredWhen); + const alias = describeExpression(conditionalRequired); + if (!firstTime(FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT, name, canonical, alias)) return; + + findings.push(aliasFinding({ + subject: 'Field', + where: `field '${name}' on ${ownerLabel}`, + canonicalKey: 'requiredWhen', + aliasKey: 'conditionalRequired', + canonical, + alias, + lost: 'never gates the field', + fix: `If ${alias} is the predicate you meant, put it in 'requiredWhen' instead.`, + rule: FIELD_REQUIREDWHEN_CONDITIONALREQUIRED_CONFLICT, + })); + }; + + // ── agent.knowledge.sources vs .topics (#1878 / #1891) ───────────────────── + const checkAgent = (agent: AnyRec) => { + const knowledge = agent?.knowledge; + if (!knowledge || typeof knowledge !== 'object') return; + const { sources, topics } = knowledge as AnyRec; + if (!isDeclaredList(sources) || !isDeclaredList(topics)) return; + if (sameMembers(sources, topics)) return; + + const name = typeof agent.name === 'string' && agent.name ? agent.name : '(unnamed)'; + const canonical = describeList(sources); + const alias = describeList(topics); + if (!firstTime(AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT, name, canonical, alias)) return; + + findings.push(aliasFinding({ + subject: 'Agent knowledge', + where: `agent '${name}' knowledge`, + canonicalKey: 'sources', + aliasKey: 'topics', + canonical, + alias, + lost: 'is never recruited into RAG context', + fix: `If ${alias} names the sources you meant, put them in 'sources' instead.`, + rule: AGENT_KNOWLEDGE_SOURCES_TOPICS_CONFLICT, + })); + }; + + // Object-nested actions first so the retained (deduped) finding keeps object + // context; the same object walk covers its fields. for (const obj of asArray(stack.objects)) { const object = typeof obj.name === 'string' ? obj.name : undefined; for (const action of asArray(obj.actions)) checkAction(action, object); + for (const field of asArray(obj.fields)) checkField(field, `object '${object ?? '(unnamed)'}'`); } for (const action of asArray(stack.actions)) checkAction(action); + // An extension adds/overrides fields on an object owned by another package — + // same `FieldSchema`, same transform, same silent discard. + for (const ext of asArray(stack.objectExtensions)) { + const target = typeof ext.objectName === 'string' ? ext.objectName + : typeof ext.name === 'string' ? ext.name + : '(unnamed)'; + for (const field of asArray(ext.fields)) checkField(field, `object extension '${target}'`); + } + for (const agent of asArray(stack.agents)) checkAgent(agent); return findings; } diff --git a/packages/spec/src/stack.test.ts b/packages/spec/src/stack.test.ts index 41abc58cb8..145cc10831 100644 --- a/packages/spec/src/stack.test.ts +++ b/packages/spec/src/stack.test.ts @@ -1436,4 +1436,35 @@ describe('defineStack — deprecated alias warnings (#3743)', () => { const action = (stack.actions as Array>)[0]; expect(action.execute).toBe('legacy_f'); }); + + it('warns for every rule in the pass, not just the action one', () => { + // The wiring is rule-agnostic — it loops over whatever `lintDeprecatedAliases` + // returns — so a field alias must surface through `defineStack` on the same + // terms as an action alias. `FieldSchema` folds `conditionalRequired` into + // `requiredWhen` and drops it, which is invisible one line later. + const stack = defineStack({ + manifest, + objects: [{ + name: 'demo_task', + label: 'Task', + fields: { + title: { type: 'text' as const }, + due_date: { + type: 'date' as const, + requiredWhen: 'record.stage == "closed"', + conditionalRequired: 'record.amount > 0', + }, + }, + }], + }); + + expect(warn).toHaveBeenCalledTimes(1); + const msg = warn.mock.calls[0][0] as string; + expect(msg).toContain('field-requiredwhen-conditionalrequired-conflict'); + expect(msg).toContain(`field 'due_date' on object 'demo_task'`); + + // …and the parse still drops the alias, exactly as before the warning. + const field = (stack.objects as Array<{ fields: Record> }>)[0].fields.due_date; + expect('conditionalRequired' in field).toBe(false); + }); });