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); + }); });