diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cc1a34a7..f6aeca575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,99 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed — `{{#hasField}}` rendered as absent on a populated payload, in every port (npm/PyPI/NuGet/Maven) + +A prompt's conditional section — *"include the abilities block only when there ARE +abilities"* — is expressed as `{{#hasAbilities}}`, a **derived** boolean accessor over the +declared field `abilities`. The JVM has emitted `has()` onto every generated payload +record since 7.7.7 and accepts the section in its static drift check, sharing one naming +rule so the two "can never drift apart". + +**No render engine implemented the other half.** Given the same payload *data* — a map, which +is what the runtime and the conformance corpus actually pass — all five ports rendered the +section as absent: + +``` +payload {"abilities":[{"name":"Fireball"}]} +template "Abilities:{{#hasAbilities}} {{#abilities}}[{{name}}]{{/abilities}}{{/hasAbilities}}" +before "Abilities:" ← content silently dropped, no error +after "Abilities: [Fireball]" +``` + +Silent wrong output, not a failure: the prompt shipped without its block. The JVM looked +correct only because a *generated record* answers `hasFoo()` by its own method — so the same +payload rendered differently depending on whether it arrived as a record or as a map. + +`PayloadAccessors` now exists in all five ports carrying one shared rule (`"has" + +capitalize`, and presence semantics mirroring the JVM emitter exactly: string → non-blank, +collection → non-empty, reference → non-null, **number/boolean → no accessor at all**, since +`{{#hasCount}}` over an int is drift rather than a conditional). Render derives them +non-mutatingly, recursing into nested objects and collection elements so a section sees the +element it is iterating; an **authored** `hasFoo` always wins. `verify` accepts exactly what +render resolves, mirroring the JVM's deliberate permissiveness (acceptance keys off the +field existing, not its type), and still reports drift inside a has-section body. + +Found by an adopter with a JVM-authored prompt estate whose Node gate reported **157** +`ERR_VAR_NOT_ON_PAYLOAD`, all `has`-prefixed, while its JVM gate reported none. Now 0 on +both. Gated by the shared `render-derived-has-accessor` conformance case — **the corpus had +no fixture using a derived accessor at all**, which is precisely why a divergence in the +pillar that promises byte-identical rendering survived this long. + +### Fixed — a requirement could not claim a prompt template (npm) + +`@implementedBy` is documented as naming "the model nodes realising this requirement", and +it resolved through the OBJECT resolver only. So a requirement could claim an entity, a +value or a projection — and naming a `template.prompt` produced +`ERR_REQUIREMENT_DANGLING_REF` ("the model moved and the requirement is stale") for a +template sitting in the loaded tree. + +That excluded the estate with the **most** to gain from a status. A retired entity leaves a +table behind; a retired prompt leaves nothing, which is exactly the invisibility +`@status: abandoned` exists to fix. A project whose prompts are a first-class pillar could +describe every table it owns and not one of its prompts. + +**L4 now means "a declared top-level model node"** — an `object.*` or a `template.*` — and +L5 a member of one. Bare references bind package-locally and ambiguous ones bind nothing, +the same fail-closed rule objects use. Requirements themselves are excluded: hierarchy is +nesting, and a requirement claiming a requirement would be a second, contradictory parent +mechanism. Object coverage is deliberately untouched and stays entity-grain — claiming a +template must not silence the unclaimed-entity warning. + +Also verified rather than assumed, since the same report asked about them: **fields, views, +validators and identities were already claimable at L5** and needed no change. They are now +pinned by tests so that stays true. Gated by `cli/test/requirement-template-refs.test.ts`. + +### Fixed — `@verifiedBy` decided what a test file is, and was wrong about a mainstream convention (npm) + +`@verifiedBy`'s scan carried one closed list of test-file patterns for the five ported +ecosystems, with no way to extend it. **That list is a guess about someone else's repository, +and it was wrong on a mainstream case from the day it shipped:** Maven Failsafe names +integration tests `FooIT.java` / `FooIT.kt`, which matched nothing. Because the scan only fails +OPEN at *zero* test files, a JVM project with unit tests (matched) and integration tests +(unmatched) got a confident `ERR_REQUIREMENT_TEST_MISSING` — *"the claim was never true"* — for +a test sitting in the repo. An adopter hit exactly this: every repository test in the project is +an `*IT`, so `@verifiedBy` was unusable there and the honest workaround was to stop using the +attribute. + +Three changes, of which only the first is a patch to the guess: + +- **Failsafe's own defaults are now built in** (`*IT`, `*ITCase`, `IT*` for `.java`; `*IT` / + `*ITCase` for `.kt`). +- **`verify.testFiles` in `metaobjects.config.ts`** lets a project declare its own conventions + as globs, added to the built-ins. What counts as a test file is project-specific; a list + shipped by this repo cannot be authoritative about a convention it has never seen. +- **An unrecognised convention is no longer reported as a broken claim.** When a name is absent + from the corpus, `verify` now searches the unclassified source files before deciding. If the + name is there, it emits `WARN_REQUIREMENT_TEST_UNCLASSIFIED` naming the file and pointing at + `verify.testFiles`; `ERR_REQUIREMENT_TEST_MISSING` is reserved for a name that appears + **nowhere**. The second pass runs only on the miss path, so the cost is per broken claim + rather than per run. + +The reusable lesson is the failure mode, not the regex: a gate that hardcodes another +ecosystem's conventions will eventually tell a correct project that it is broken, and the +default posture when the tool cannot classify something must be to say so rather than to +convict. Gated by `cli/test/verified-by-corpus.test.ts`. + ### Fixed — `verify` gates the committed schema snapshot, which nothing checked (npm) — [#292](https://github.com/metaobjectsdev/metaobjects/issues/292) `meta migrate` diffs metadata against `.metaobjects/migrations/.schema..json` by default diff --git a/agent-context/skills/metaobjects-authoring/references/requirements.md b/agent-context/skills/metaobjects-authoring/references/requirements.md index 33c012a31..0cf90b5b1 100644 --- a/agent-context/skills/metaobjects-authoring/references/requirements.md +++ b/agent-context/skills/metaobjects-authoring/references/requirements.md @@ -47,9 +47,15 @@ line: *would this sentence have to change if the code changed but the model did is `notes`. **Hierarchy is nesting, and links live at the bottom.** L1 solution, L2 segment, L3 -service — these never reference the model. **L4** binds an object, **L5** binds a field, -view or identity. `implementedBy` above L4 is an error. Regrouping *moves* a node; it does -not edit a parent string. +service — these never reference the model. **L4** binds a declared top-level node — an +`object.*` **or a `template.*`** — and **L5** binds a member of one: a field, view, +validator, identity, or a template's child. `implementedBy` above L4 is an error. +Regrouping *moves* a node; it does not edit a parent string. + +Claim your prompts. A `template.prompt` is a model node realising a capability exactly as +an entity is, and it is the node whose retirement is hardest to see later — a removed +prompt leaves no table behind. A prompt estate with no requirement entries is the same +blind spot this whole mechanism exists to close. **L1–L3 are levels of abstraction and ownership in the problem domain** — whose need is this, and at what altitude — and are NEVER a directory, package, deployable or module. diff --git a/agent-context/skills/metaobjects-verify/references/requirements.md b/agent-context/skills/metaobjects-verify/references/requirements.md index c105389d7..4380f198d 100644 --- a/agent-context/skills/metaobjects-verify/references/requirements.md +++ b/agent-context/skills/metaobjects-verify/references/requirements.md @@ -35,9 +35,26 @@ mechanism exists to preserve. | `@implementedBy` above the L4 link floor | 1 | | live `requirement.architectural` claimed by nothing | 1 | | `@verifiedBy` naming a test that exists nowhere | 1 | +| `@verifiedBy` naming a name found only in an **unrecognised** test file | 0 (warning) | | `@verifiedBy` naming a test that is **skipped** | 0 (warning) | | an entity no requirement claims | 0 (warning) | +## What counts as a test file is YOUR project's call + +The scan ships patterns for jest/vitest/bun, JUnit, Maven Failsafe (`*IT`), xUnit/NUnit, +pytest and Kotlin. Those are a convenience, **not an authority** — a built-in list is a guess +about your repository, and a wrong guess reports a real test as a broken claim. Declare your +conventions and they are added to the built-ins: + +```ts +// metaobjects.config.ts +export default defineConfig({ verify: { testFiles: ["**/*IT.kt", "**/*.feature"] } }); +``` + +If a named test is missing from the corpus but present in some other source file, `verify` +warns and names that file rather than failing — an unrecognised convention is the tool's +ignorance, not your mistake. + ## What a green run does NOT prove It proves **referential integrity**: statuses parse, levels are in range, links sit at or diff --git a/docs/features/requirements.md b/docs/features/requirements.md index 99841082f..36b9be081 100644 --- a/docs/features/requirements.md +++ b/docs/features/requirements.md @@ -57,6 +57,21 @@ no `id` and no `parent`: regrouping moves a subtree. L4 object, L5 member. `@implementedBy` is legal at **L4 and L5 only** — L1–L3 are organisational and never reference the model. +**What L4 and L5 may name.** L4 names a declared top-level node: an `object.*` **or a +`template.*`**. A declared prompt is a model node realising a capability in the same sense +an entity is — and it is the one most in need of a status, because a retired prompt leaves +no table behind to notice. L5 names a member of one: a field, a view, a validator, an +identity, or a template's child. + +```jsonc +{ "requirement.functional": { + "name": "sceneBrief", "@level": 4, "@status": "live", + "@statement": "The game master is told what the party can currently see.", + "@violation": "A scene narrated from world state the party has no way to know.", + "@implementedBy": ["acme::play::sceneBrief"] // a template.prompt +}} +``` + **L1–L3 are levels of abstraction and ownership in the problem domain** — whose need is this, and at what altitude — and are **never** a directory, package, deployable or module. Binding to technical constructs happens only at L4 and L5, which is the allocation step. The test to @@ -149,6 +164,24 @@ entry. `@verifiedBy` names tests: `verify` checks each exists and is not skipped. It never runs them. `@trackedBy` names issues or tickets and is **not** resolved — `verify` has no network. +**What counts as a test file is your project's call.** The scan ships patterns for the +conventions this repo ports to — jest/vitest/bun, JUnit, Maven Failsafe (`*IT`), xUnit/NUnit, +pytest, Kotlin — and they are a *convenience, not an authority*: a built-in list is a guess +about someone else's repository, and a wrong guess turns a real test into a "broken claim". +Declare yours and they are added to the built-ins: + +```ts +// metaobjects.config.ts +export default defineConfig({ + verify: { testFiles: ["**/*IT.kt", "**/*.feature"] }, +}); +``` + +If a named test cannot be found in the corpus but *does* appear in some other source file, +`verify` says so (`WARN_REQUIREMENT_TEST_UNCLASSIFIED`, naming the file) instead of claiming +the requirement is broken — an unrecognised convention is the tool's ignorance, not your +mistake. `ERR_REQUIREMENT_TEST_MISSING` is reserved for a name that appears **nowhere**. + > **`@verifiedBy` is existence evidence, not proof — and the difference matters most to whoever > authored it.** The scan matches a name anywhere in the test corpus, as a whole word, in any > language; that generosity is deliberate (a "missing" verdict then means the name appears in no diff --git a/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-authoring/references/requirements.md b/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-authoring/references/requirements.md index 33c012a31..0cf90b5b1 100644 --- a/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-authoring/references/requirements.md +++ b/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-authoring/references/requirements.md @@ -47,9 +47,15 @@ line: *would this sentence have to change if the code changed but the model did is `notes`. **Hierarchy is nesting, and links live at the bottom.** L1 solution, L2 segment, L3 -service — these never reference the model. **L4** binds an object, **L5** binds a field, -view or identity. `implementedBy` above L4 is an error. Regrouping *moves* a node; it does -not edit a parent string. +service — these never reference the model. **L4** binds a declared top-level node — an +`object.*` **or a `template.*`** — and **L5** binds a member of one: a field, view, +validator, identity, or a template's child. `implementedBy` above L4 is an error. +Regrouping *moves* a node; it does not edit a parent string. + +Claim your prompts. A `template.prompt` is a model node realising a capability exactly as +an entity is, and it is the node whose retirement is hardest to see later — a removed +prompt leaves no table behind. A prompt estate with no requirement entries is the same +blind spot this whole mechanism exists to close. **L1–L3 are levels of abstraction and ownership in the problem domain** — whose need is this, and at what altitude — and are NEVER a directory, package, deployable or module. diff --git a/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-verify/references/requirements.md b/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-verify/references/requirements.md index c105389d7..4380f198d 100644 --- a/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-verify/references/requirements.md +++ b/fixtures/agent-context-conformance/ts-requirements/expected/.claude/skills/metaobjects-verify/references/requirements.md @@ -35,9 +35,26 @@ mechanism exists to preserve. | `@implementedBy` above the L4 link floor | 1 | | live `requirement.architectural` claimed by nothing | 1 | | `@verifiedBy` naming a test that exists nowhere | 1 | +| `@verifiedBy` naming a name found only in an **unrecognised** test file | 0 (warning) | | `@verifiedBy` naming a test that is **skipped** | 0 (warning) | | an entity no requirement claims | 0 (warning) | +## What counts as a test file is YOUR project's call + +The scan ships patterns for jest/vitest/bun, JUnit, Maven Failsafe (`*IT`), xUnit/NUnit, +pytest and Kotlin. Those are a convenience, **not an authority** — a built-in list is a guess +about your repository, and a wrong guess reports a real test as a broken claim. Declare your +conventions and they are added to the built-ins: + +```ts +// metaobjects.config.ts +export default defineConfig({ verify: { testFiles: ["**/*IT.kt", "**/*.feature"] } }); +``` + +If a named test is missing from the corpus but present in some other source file, `verify` +warns and names that file rather than failing — an unrecognised convention is the tool's +ignorance, not your mistake. + ## What a green run does NOT prove It proves **referential integrity**: statuses parse, levels are in range, links sit at or diff --git a/fixtures/metamodel-docs/expected/types/requirement.md b/fixtures/metamodel-docs/expected/types/requirement.md index e7168f030..71365d2e8 100644 --- a/fixtures/metamodel-docs/expected/types/requirement.md +++ b/fixtures/metamodel-docs/expected/types/requirement.md @@ -27,7 +27,7 @@ How the system is built, applied uniformly across the model. Its check is UNIVER | `@status` | string | yes | | `planned`, `live`, `partial`, `abandoned`, `superseded` | — | As on requirement.functional. A live or partial architectural requirement claimed by NOTHING is an error: a policy declared and applied to nothing. A planned one is exempt from that check — it is not applied yet by definition. | | `@supersededBy` | string | no | | | — | The requirement that replaced this one. Expected on status=superseded. | | `@trackedBy` | string[] | no | | | — | As on requirement.functional. Issue or ticket references for outstanding work; free-form, not resolved. | -| `@verifiedBy` | string[] | no | | | — | Names of the tests proving the policy holds. verify checks each exists and is not skipped; it never runs them. | +| `@verifiedBy` | string[] | no | | | — | OPTIONAL — omit unless you have opened the test and read what it asserts. Names of tests that assert the policy holds. verify checks each name EXISTS and is not skipped; it never runs them, and it cannot tell whether the named test verifies this requirement — any occurrence in the test corpus satisfies it. | | `@violation` | string | yes | | | — | What breaking it looks like — the node that would contradict it. This is what makes universality checkable. | **Allowed children** @@ -51,7 +51,7 @@ What the product does for a user, stated as one violable claim. Its check is EXI | `@status` | string | yes | | `planned`, `live`, `partial`, `abandoned`, `superseded` | — | planned intended but not built yet; live implemented and in use; partial implemented with known gaps; abandoned built then deliberately retired; superseded replaced by a different mechanism. A dangling @implementedBy is an ERROR on live/partial (the model moved, the requirement is stale) and ALLOWED on planned/abandoned/superseded — on planned the nodes do not exist YET, on the other two they are meant to be gone, and that is the entry doing its job. A planned requirement also never contributes to object coverage: planning a capability must not silence the warning that nothing implements it. | | `@supersededBy` | string | no | | | — | The requirement that replaced this one. Expected on status=superseded. | | `@trackedBy` | string[] | no | | | — | Issue or ticket references for outstanding work — a URL, an owner/repo#123 shorthand, or a tracker key. Free-form and NOT resolved by verify, which does not reach the network; unlike @verifiedBy, nothing here is checked to exist. Its job is to stop a deferred gap becoming invisible, so verify warns when a deferred requirement names no ticket. Also the right place to link the ticket that a planned requirement will be built under. | -| `@verifiedBy` | string[] | no | | | — | Names of the tests proving the behaviour. verify checks each exists and is not skipped; it never runs them. | +| `@verifiedBy` | string[] | no | | | — | OPTIONAL — omit unless you have opened the test and read what it asserts. Names of tests that assert the behaviour. verify checks each name EXISTS and is not skipped; it never runs them, and it cannot tell whether the named test verifies this requirement — any occurrence in the test corpus satisfies it. | | `@violation` | string | yes | | | — | What breaking it looks like, in one sentence. A requirement MUST be violable: 'every entity has a uuid primary key' is (point at one with a composite string key); 'things are persisted' is not, and is a description rather than a requirement. | **Allowed children** diff --git a/fixtures/render-conformance/render-derived-has-accessor/expected.txt b/fixtures/render-conformance/render-derived-has-accessor/expected.txt new file mode 100644 index 000000000..d21771c1f --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/expected.txt @@ -0,0 +1,6 @@ +title:Party +bio: (none) +sponsor: Guild +companions: (none) +abilities: Fireball[fire aoe ] Mend[untagged] +details: present \ No newline at end of file diff --git a/fixtures/render-conformance/render-derived-has-accessor/meta.json b/fixtures/render-conformance/render-derived-has-accessor/meta.json new file mode 100644 index 000000000..e3a2b7323 --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/meta.json @@ -0,0 +1 @@ +{ "format": "text", "note": "Derived has boolean accessors: present/absent/blank across scalar, collection and nested scope" } diff --git a/fixtures/render-conformance/render-derived-has-accessor/payload.json b/fixtures/render-conformance/render-derived-has-accessor/payload.json new file mode 100644 index 000000000..99d4640bc --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/payload.json @@ -0,0 +1,22 @@ +{ + "title": "Party", + "bio": " ", + "abilities": [ + { + "name": "Fireball", + "tags": [ + "fire", + "aoe" + ] + }, + { + "name": "Mend", + "tags": [] + } + ], + "companions": [], + "sponsor": { + "name": "Guild" + }, + "emptyDetails": {} +} \ No newline at end of file diff --git a/fixtures/render-conformance/render-derived-has-accessor/template.mustache b/fixtures/render-conformance/render-derived-has-accessor/template.mustache new file mode 100644 index 000000000..30c843e46 --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/template.mustache @@ -0,0 +1,6 @@ +title:{{title}} +bio:{{#hasBio}} {{bio}}{{/hasBio}}{{^hasBio}} (none){{/hasBio}} +sponsor:{{#hasSponsor}} {{sponsor.name}}{{/hasSponsor}} +companions:{{#hasCompanions}} some{{/hasCompanions}}{{^hasCompanions}} (none){{/hasCompanions}} +abilities:{{#hasAbilities}}{{#abilities}} {{name}}{{#hasTags}}[{{#tags}}{{.}} {{/tags}}]{{/hasTags}}{{^hasTags}}[untagged]{{/hasTags}}{{/abilities}}{{/hasAbilities}} +details:{{#hasEmptyDetails}} present{{/hasEmptyDetails}}{{^hasEmptyDetails}} absent{{/hasEmptyDetails}} \ No newline at end of file diff --git a/server/csharp/MetaObjects.Codegen/Generators/Fr019SharedEnum.cs b/server/csharp/MetaObjects.Codegen/Generators/Fr019SharedEnum.cs index a08987fff..f320a939c 100644 --- a/server/csharp/MetaObjects.Codegen/Generators/Fr019SharedEnum.cs +++ b/server/csharp/MetaObjects.Codegen/Generators/Fr019SharedEnum.cs @@ -67,8 +67,13 @@ public static class Fr019SharedEnum return new SharedEnum( Name: CSharpNaming.Pascal(decl.Name), Values: values, - // ADR-0039: resolving — @provided may be inherited via extends (TS reads decl.attr). - Provided: decl.Attr(FIELD_ATTR_PROVIDED) is true, + // ADR-0039 sanctioned own: @provided is a declaration-layer provenance marker + // ("THIS type is supplied by hand-written/third-party code"), like IsAbstract — + // it does not flow down an extends chain. A resolving read misfires on a chained + // declaration (root abstract B extends root abstract @provided A): B would be + // reported provided and emit a reference to a hand-written B the adopter never + // declared, instead of materializing B. Matches the JVM ports. + Provided: decl.OwnAttr(FIELD_ATTR_PROVIDED) is true, Package: PackageOf(decl)); } diff --git a/server/csharp/MetaObjects.Render/PayloadAccessors.cs b/server/csharp/MetaObjects.Render/PayloadAccessors.cs new file mode 100644 index 000000000..b1bed1132 --- /dev/null +++ b/server/csharp/MetaObjects.Render/PayloadAccessors.cs @@ -0,0 +1,130 @@ +namespace MetaObjects.Render; + +/// +/// Derived boolean accessors — {{#hasFoo}} over a payload field foo. +/// +/// +/// +/// A prompt needs conditional sections ("include the abilities block only when there ARE +/// abilities"), and the payload contract answers that with a DERIVED accessor rather than +/// an authored boolean field: the author declares abilities and hasAbilities +/// follows from it. Declaring both would let them disagree. +/// +/// +/// THE RULE IS SHARED ACROSS PORTS ON PURPOSE. The JVM has carried it since 7.7.7 +/// (com.metaobjects.render.PayloadAccessors, emitted onto every generated payload +/// record and accepted by its Verify), and its comment says emitter and verifier +/// share one rule so they "can never drift apart". C# had neither half, so the same +/// template verified clean on the JVM and reported drift here — and rendered WRONG rather +/// than failing, silently dropping the section. Gated cross-port by the +/// render-derived-has-accessor conformance case. +/// +/// +public static class PayloadAccessors +{ + /// The has prefix every derived boolean accessor carries. + public const string HasPrefix = "has"; + + /// + /// The boolean-accessor section name for a payload field: "has" + Capitalize(name) + /// (abilities → hasAbilities). Byte-identical to the JVM's + /// PayloadAccessors.hasAccessorName, including its capitalize, which leaves an + /// already-uppercase first character untouched. + /// + public static string HasAccessorName(string fieldName) => HasPrefix + Capitalize(fieldName); + + /// Capitalize the first character, leaving an already-uppercase one untouched. + public static string Capitalize(string s) + { + if (string.IsNullOrEmpty(s)) return s; + char c0 = s[0]; + if (char.IsUpper(c0)) return s; + return char.ToUpperInvariant(c0) + s.Substring(1); + } + + /// + /// True when is a derived boolean accessor over a field reachable + /// on the current context stack. Mirrors the JVM's Verify.isBooleanAccessor, + /// including its deliberate permissiveness: acceptance keys off the FIELD EXISTING, not + /// off its type. Accessors are simple (undotted) names. + /// + public static bool IsBooleanAccessor(List> stack, string name) + { + if (name.Contains('.')) return false; + if (!name.StartsWith(HasPrefix, StringComparison.Ordinal)) return false; + // Mustache outward walk (innermost → outermost) — the accessor is reachable + // exactly where its underlying field is. + for (int i = stack.Count - 1; i >= 0; i--) + foreach (var f in stack[i]) + if (name == HasAccessorName(f.Name)) return true; + return false; + } + + /// + /// Is "present" for the purposes of has<Field>? + /// Mirrors the JVM emitter's per-type bodies exactly: string → non-null and non-blank; + /// collection → non-null and non-empty; reference → non-null. Returns null for + /// numbers and booleans, which the JVM deliberately emits NO accessor for — nothing is + /// injected, so the name stays unresolved exactly as on a record with no such method. + /// + public static bool? AccessorValue(object? value) + { + switch (value) + { + case null: return false; + case string str: return !string.IsNullOrWhiteSpace(str); + case bool: return null; + case sbyte or byte or short or ushort or int or uint or long or ulong + or float or double or decimal: return null; + // A dictionary IS IEnumerable, so it must be matched FIRST — otherwise an + // empty nested object reports absent here and present in every other port. + case System.Collections.IDictionary: return true; + case System.Collections.IEnumerable seq: + { + foreach (var _ in seq) return true; + return false; + } + default: return true; + } + } + + /// + /// A view over carrying its derived has<Field> + /// accessors, recursively. NON-MUTATING — a render must not change the object it was + /// handed. An AUTHORED key always wins. Recursion follows Mustache's own scoping, so + /// every nested object and every collection ELEMENT becomes a context in its own right. + /// + public static object? WithDerivedAccessors(object? payload, int depth = 0) + { + if (depth > 32 || payload is null) return payload; // pathological graph + if (payload is string) return payload; + + if (payload is System.Collections.IDictionary dict) + { + var outMap = new Dictionary(StringComparer.Ordinal); + foreach (System.Collections.DictionaryEntry e in dict) + { + if (e.Key is not string k) continue; + outMap[k] = WithDerivedAccessors(e.Value, depth + 1); + } + foreach (System.Collections.DictionaryEntry e in dict) + { + if (e.Key is not string k) continue; + string name = HasAccessorName(k); + if (outMap.ContainsKey(name)) continue; // authored wins + bool? derived = AccessorValue(e.Value); + if (derived is not null) outMap[name] = derived; + } + return outMap; + } + + if (payload is System.Collections.IEnumerable seq) + { + var outList = new List(); + foreach (var item in seq) outList.Add(WithDerivedAccessors(item, depth + 1)); + return outList; + } + + return payload; + } +} diff --git a/server/csharp/MetaObjects.Render/Renderer.cs b/server/csharp/MetaObjects.Render/Renderer.cs index 053c1248e..37b332822 100644 --- a/server/csharp/MetaObjects.Render/Renderer.cs +++ b/server/csharp/MetaObjects.Render/Renderer.cs @@ -100,7 +100,10 @@ public static string Render(RenderRequest request) .Configure(settings => settings.SetEncodingFunction(v => escaper(v))) .Build(); - string result = stubble.Render(expanded, request.Payload); + // Derived `has` accessors are part of the payload contract, not of the + // caller's object — see PayloadAccessors. Injected here so a `{{#hasFoo}}` section + // resolves the same way it does against a generated JVM payload record. + string result = stubble.Render(expanded, PayloadAccessors.WithDerivedAccessors(request.Payload)); if (request.MaxChars is int cap && result.Length > cap) throw new RenderException( diff --git a/server/csharp/MetaObjects.Render/Verify.cs b/server/csharp/MetaObjects.Render/Verify.cs index 1ffd14162..3856c5358 100644 --- a/server/csharp/MetaObjects.Render/Verify.cs +++ b/server/csharp/MetaObjects.Render/Verify.cs @@ -163,7 +163,8 @@ void Walk(IReadOnlyList tokens, List> stack, Li case VarTok v: if (v.Value == ".") break; // implicit iterator — always valid if (atRoot) referencedAtRoot.Add(v.Value.Split('.')[0]); - if (Resolve(stack, v.Value) is null) + if (Resolve(stack, v.Value) is null + && !PayloadAccessors.IsBooleanAccessor(stack, v.Value)) errors.Add(new VerifyError(ERR_VAR_NOT_ON_PAYLOAD, v.Value)); break; @@ -173,6 +174,14 @@ void Walk(IReadOnlyList tokens, List> stack, Li var field = Resolve(stack, s.Value); if (field is null) { + // A derived `has` gate is a BOOLEAN over the current + // context: it resolves nothing and pushes nothing, so walk the + // body in the SAME scope — what `{{#hasX}}{{#x}}…` depends on. + if (PayloadAccessors.IsBooleanAccessor(stack, s.Value)) + { + Walk(s.Children, stack, seen); + break; + } // Unresolved section head is itself drift; skip the body // (its context is unknowable; walking it cascades false errors). errors.Add(new VerifyError(ERR_VAR_NOT_ON_PAYLOAD, s.Value)); diff --git a/server/java/render/src/main/java/com/metaobjects/render/PayloadAccessors.java b/server/java/render/src/main/java/com/metaobjects/render/PayloadAccessors.java index 242d5fe9a..7fe46e6a8 100644 --- a/server/java/render/src/main/java/com/metaobjects/render/PayloadAccessors.java +++ b/server/java/render/src/main/java/com/metaobjects/render/PayloadAccessors.java @@ -45,4 +45,78 @@ public static String capitalize(String s) { if (Character.isUpperCase(c0)) return s; return Character.toUpperCase(c0) + s.substring(1); } + + /** + * Is {@code value} "present" for the purposes of {@code has}? Mirrors the + * emitter's per-type bodies exactly: String → non-null and non-blank; Collection → + * non-null and non-empty; reference → non-null. + * + *

Returns {@code null} for numbers and booleans, which the emitter deliberately + * skips — they are always-present scalars, and a {@code {{#hasCount}}} over an int is + * drift rather than a conditional. Returning null (rather than false) keeps that + * distinction: nothing is injected, so the name stays unresolved exactly as it is on a + * generated record that has no such method. + */ + public static Boolean accessorValue(Object value) { + if (value == null) return Boolean.FALSE; + if (value instanceof CharSequence cs) return !cs.toString().isBlank(); + if (value instanceof Boolean || value instanceof Number) return null; + if (value instanceof java.util.Map) return Boolean.TRUE; + if (value instanceof java.util.Collection c) return !c.isEmpty(); + // getLength covers primitive arrays too; Object[] alone reported an empty + // int[] as present, where every other port reports absent. + if (value.getClass().isArray()) return java.lang.reflect.Array.getLength(value) > 0; + return Boolean.TRUE; + } + + /** + * A view over {@code payload} carrying its derived {@code has} accessors, + * recursively — for MAP-SHAPED payloads only. + * + *

A generated payload record already answers {@code hasFoo()} by its own emitted + * method and is returned untouched; this fills the gap for the map/list graphs the + * runtime and the conformance corpus actually pass. Without it, the SAME payload data + * renders differently depending on whether it arrived as a record or as a map, which + * is the divergence the shared {@code render-derived-has-accessor} fixture pins. + * + *

NON-MUTATING — a render must not change the object it was handed. An AUTHORED key + * always wins. Recursion follows Mustache's own scoping: every nested map and every + * collection ELEMENT becomes a context in its own right. + */ + public static Object withDerivedAccessors(Object payload) { + return withDerivedAccessors(payload, 0); + } + + private static Object withDerivedAccessors(Object payload, int depth) { + if (depth > 32 || payload == null) return payload; // pathological graph + if (payload instanceof java.util.Map map) { + java.util.Map out = new java.util.LinkedHashMap<>(); + for (java.util.Map.Entry e : map.entrySet()) { + if (!(e.getKey() instanceof String k)) continue; + out.put(k, withDerivedAccessors(e.getValue(), depth + 1)); + } + for (java.util.Map.Entry e : map.entrySet()) { + if (!(e.getKey() instanceof String k)) continue; + String name = hasAccessorName(k); + if (out.containsKey(name)) continue; // authored wins + Boolean derived = accessorValue(e.getValue()); + if (derived != null) out.put(name, derived); + } + return out; + } + // Every Collection, not just List — a Set's elements are contexts too, which + // this method's own contract promises. + if (payload instanceof java.util.Collection coll) { + java.util.List out = new java.util.ArrayList<>(coll.size()); + for (Object item : coll) out.add(withDerivedAccessors(item, depth + 1)); + return out; + } + if (payload.getClass().isArray() && !payload.getClass().getComponentType().isPrimitive()) { + int n = java.lang.reflect.Array.getLength(payload); + java.util.List out = new java.util.ArrayList<>(n); + for (int i = 0; i < n; i++) out.add(withDerivedAccessors(java.lang.reflect.Array.get(payload, i), depth + 1)); + return out; + } + return payload; + } } diff --git a/server/java/render/src/main/java/com/metaobjects/render/Renderer.java b/server/java/render/src/main/java/com/metaobjects/render/Renderer.java index d41c22960..53d882539 100644 --- a/server/java/render/src/main/java/com/metaobjects/render/Renderer.java +++ b/server/java/render/src/main/java/com/metaobjects/render/Renderer.java @@ -69,7 +69,11 @@ public void encode(String value, Writer writer) { }; Mustache compiled = factory.compile(new StringReader(expanded), refOrInline(req)); StringWriter writer = new StringWriter(); - compiled.execute(writer, req.payload()).flush(); + // Derived `has` accessors are part of the payload contract, not of the + // caller's object — see PayloadAccessors. A generated record already answers + // hasFoo(); this fills the same contract for a map-shaped payload, so the two + // render identically. + compiled.execute(writer, PayloadAccessors.withDerivedAccessors(req.payload())).flush(); rendered = writer.toString(); } catch (MustacheException | IOException e) { throw new RenderException("Mustache compile/execute failed", e); diff --git a/server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt b/server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt new file mode 100644 index 000000000..d21771c1f --- /dev/null +++ b/server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt @@ -0,0 +1,6 @@ +title:Party +bio: (none) +sponsor: Guild +companions: (none) +abilities: Fireball[fire aoe ] Mend[untagged] +details: present \ No newline at end of file diff --git a/server/python/src/metaobjects/codegen/generators/fr019_shared_enum.py b/server/python/src/metaobjects/codegen/generators/fr019_shared_enum.py index 4bf284bff..fb1801336 100644 --- a/server/python/src/metaobjects/codegen/generators/fr019_shared_enum.py +++ b/server/python/src/metaobjects/codegen/generators/fr019_shared_enum.py @@ -66,12 +66,21 @@ def resolve_shared_enum_decl(field: MetaField) -> MetaData | None: def is_provided(decl: MetaData) -> bool: - """Effective ``@provided`` truth of an enum declaration. - - ADR-0039 — resolves through ``extends`` (``get_meta_attr``): a concrete enum - extending an abstract ``@provided`` enum inherits the flag, so an own-only read - would misclassify it.""" - return decl.get_meta_attr(fc.FIELD_ATTR_PROVIDED) is True + """``@provided`` truth of an enum DECLARATION. + + ADR-0039 sanctioned own (Python naming inversion: ``attr()`` is the OWN read, + ``get_meta_attr()`` resolves). ``@provided`` is a declaration-layer provenance + marker — "THIS type is supplied by hand-written/third-party code", like + ``is_abstract`` — and does not flow down an ``extends`` chain. + + This is only ever called on the resolved declaration (see + ``shared_enum_for_field``), never on the consuming field, so own and resolving + agree for a plain ``field extends @provided decl``. They diverge on a CHAINED + declaration (root abstract ``B extends`` root abstract ``@provided A``): a + resolving read reports B provided and emits a reference to a hand-written ``B`` + the adopter never declared, instead of materializing B. Matches the JVM ports. + """ + return decl.attr(fc.FIELD_ATTR_PROVIDED) is True def _meta_package_of(decl: MetaData) -> str: diff --git a/server/python/src/metaobjects/render/payload_accessors.py b/server/python/src/metaobjects/render/payload_accessors.py new file mode 100644 index 000000000..4189c98c4 --- /dev/null +++ b/server/python/src/metaobjects/render/payload_accessors.py @@ -0,0 +1,134 @@ +"""Derived boolean accessors — ``{{#hasFoo}}`` over a payload field ``foo``. + +A prompt needs conditional sections ("include the abilities block only when there ARE +abilities"), and the payload contract answers that with a DERIVED accessor rather than an +authored boolean field: the author declares ``abilities`` and ``hasAbilities`` follows +from it. Declaring both would let them disagree. + +THE RULE IS SHARED ACROSS PORTS ON PURPOSE. The JVM has carried it since 7.7.7 +(``com.metaobjects.render.PayloadAccessors``, emitted onto every generated payload record +and accepted by its ``Verify``), and its comment says the emitter and the verifier share +one rule so they "can never drift apart". Python had neither half, so the same template +verified clean on the JVM and reported drift here — and rendered WRONG rather than +failing, silently dropping the section. Gated cross-port by the +``render-derived-has-accessor`` conformance case. +""" + +from __future__ import annotations + +import numbers +from collections.abc import Mapping, Sequence +from typing import Any + +__all__ = [ + "HAS_PREFIX", + "has_accessor_name", + "capitalize", + "accessor_value", + "with_derived_accessors", + "is_boolean_accessor", +] + +#: The ``has`` prefix every derived boolean accessor carries. +HAS_PREFIX = "has" + +_MAX_DEPTH = 32 + + +def capitalize(s: str) -> str: + """Capitalize the first character, leaving an already-uppercase one untouched. + + Deliberately NOT ``str.capitalize()``, which also lowercases the remainder. + """ + if not s: + return s + if s[0].isupper(): + return s + return s[0].upper() + s[1:] + + +def has_accessor_name(field_name: str) -> str: + """``"has" + capitalize(name)`` (``abilities`` → ``hasAbilities``). + + Byte-identical to the JVM's ``PayloadAccessors.hasAccessorName``. + """ + return HAS_PREFIX + capitalize(field_name) + + +def accessor_value(value: Any) -> bool | None: + """Is ``value`` "present" for the purposes of ``has``? + + Mirrors the JVM emitter's per-type bodies exactly: string → non-null and non-blank; + collection → non-null and non-empty; reference → non-null. + + Returns ``None`` for numbers and booleans, which the JVM deliberately emits NO + accessor for — they are always-present scalars, and ``{{#hasCount}}`` over an int is + drift rather than a conditional. Returning ``None`` (rather than ``False``) keeps that + distinction: nothing is injected, so the name stays unresolved exactly as it is on a + generated record with no such method. + """ + if value is None: + return False + if isinstance(value, str): + return bool(value.strip()) + # bool before int — bool IS an int in Python, and a boolean field gets no accessor. + if isinstance(value, numbers.Number): + return None + if isinstance(value, Mapping): + return True + if isinstance(value, (Sequence, set, frozenset)): + return len(value) > 0 + return True + + +def with_derived_accessors(payload: Any, depth: int = 0) -> Any: + """A view over ``payload`` carrying its derived ``has`` accessors, recursively. + + NON-MUTATING — the caller's payload is never touched, because a render must not be + able to change the object it was handed. An AUTHORED key always wins: if a payload + genuinely carries ``hasFoo``, that value is kept rather than shadowed. + + Recursion follows Mustache's own scoping: every nested mapping and every sequence + ELEMENT becomes a context in its own right, so a section over ``abilities`` sees the + accessors of the ability it is currently iterating. + """ + if depth > _MAX_DEPTH: + return payload # pathological graph; render is not a validator + if isinstance(payload, Mapping): + out: dict[str, Any] = { + k: with_derived_accessors(v, depth + 1) for k, v in payload.items() + } + for k, v in payload.items(): + if not isinstance(k, str): + continue + name = has_accessor_name(k) + if name in payload: # authored wins + continue + derived = accessor_value(v) + if derived is not None: + out[name] = derived + return out + if isinstance(payload, (str, bytes)): + return payload + if isinstance(payload, Sequence): + return [with_derived_accessors(v, depth + 1) for v in payload] + return payload + + +def is_boolean_accessor(stack: list[list[Any]], name: str) -> bool: + """True when ``name`` is a derived accessor over a field reachable on ``stack``. + + Mirrors the JVM's ``Verify.isBooleanAccessor``, including its deliberate + permissiveness: acceptance keys off the FIELD EXISTING, not off its type. Accessors + are simple (undotted) names; a dotted path is never an accessor. + """ + if "." in name: + return False + if not name.startswith(HAS_PREFIX): + return False + # Mustache outward walk (innermost → outermost). + for frame in reversed(stack): + for f in frame: + if name == has_accessor_name(f.name): + return True + return False diff --git a/server/python/src/metaobjects/render/renderer.py b/server/python/src/metaobjects/render/renderer.py index 572edf88e..02642d280 100644 --- a/server/python/src/metaobjects/render/renderer.py +++ b/server/python/src/metaobjects/render/renderer.py @@ -23,6 +23,7 @@ from typing import Any from . import escapers +from .payload_accessors import with_derived_accessors from .verify import InMemoryProvider, Provider MAX_DEPTH = 32 @@ -59,7 +60,10 @@ def render(req: RenderRequest) -> str: _validate(req) body = req.template if req.template is not None else _resolve_or_raise(req.provider, req.ref) expanded = _pre_expand_partials(body, req.provider, []) - out = _interpret(expanded, req.payload, req.format) + # Derived `has` accessors are part of the payload contract, not of the + # caller's object — see payload_accessors. Injected here so a `{{#hasFoo}}` section + # resolves the same way it does against a generated JVM payload record. + out = _interpret(expanded, with_derived_accessors(req.payload), req.format) # @maxChars is a fail-closed render budget: over-budget output RAISES (never # silently truncates). Canonical cross-port behavior — message shape matches # TS/C#/Java: "render exceeded maxChars budget: > ". diff --git a/server/python/src/metaobjects/render/verify.py b/server/python/src/metaobjects/render/verify.py index 4c5c1548d..3889f469c 100644 --- a/server/python/src/metaobjects/render/verify.py +++ b/server/python/src/metaobjects/render/verify.py @@ -23,6 +23,8 @@ from dataclasses import dataclass from typing import Protocol +from .payload_accessors import is_boolean_accessor + #: A ``{{var}}`` references a field the (contextual) payload does not declare. ERR_VAR_NOT_ON_PAYLOAD = "ERR_VAR_NOT_ON_PAYLOAD" #: A ``{{> ref}}`` partial does not resolve in the provider. @@ -215,7 +217,9 @@ def walk( continue # implicit iterator — always valid if at_root: referenced_at_root.add(tok.value.split(".")[0]) - if _resolve(stack, tok.value) is None: + if _resolve(stack, tok.value) is None and not is_boolean_accessor( + stack, tok.value + ): errors.append(VerifyError(ERR_VAR_NOT_ON_PAYLOAD, tok.value)) elif isinstance(tok, _Section): if tok.value == ".": @@ -225,6 +229,12 @@ def walk( referenced_at_root.add(tok.value.split(".")[0]) field = _resolve(stack, tok.value) if field is None: + # A derived `has` gate is a BOOLEAN over the current context: + # it resolves nothing and pushes nothing, so walk the body in the SAME + # scope — what `{{#hasAbilities}}{{#abilities}}…` depends on. + if is_boolean_accessor(stack, tok.value): + walk(tok.children, stack, seen) + continue # Unresolved section head is itself drift; skip the body (its # context is unknowable, walking it would cascade false errors). errors.append(VerifyError(ERR_VAR_NOT_ON_PAYLOAD, tok.value)) diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index 1979379b0..bf55fb5d8 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -195,7 +195,13 @@ export async function verifyCommand( function runRequirementVerify(): number { // `@verifiedBy` resolution needs the project on disk, so it is a separate // scan; its diagnostics carry the same severities and share this reporter. - const diags = [...checkRequirements(root), ...checkVerifiedBy(root, cwd)]; + // `verify.testFiles` lets a project name its own test-file conventions. What counts + // as a test is project-specific, and the built-in patterns are a convenience, not an + // authority — see the verified-by-scan header. + const diags = [ + ...checkRequirements(root), + ...checkVerifiedBy(root, cwd, forgeConfig?.verify?.testFiles), + ]; // Printed on EVERY run, clean or not — a gate that says nothing when it // passes cannot be told apart from a gate that checked nothing, and the diff --git a/server/typescript/packages/cli/src/lib/requirement-check.ts b/server/typescript/packages/cli/src/lib/requirement-check.ts index f4ae86d74..13f27e750 100644 --- a/server/typescript/packages/cli/src/lib/requirement-check.ts +++ b/server/typescript/packages/cli/src/lib/requirement-check.ts @@ -148,6 +148,47 @@ function subtreeClaimsAnything(req: MetaRequirement): boolean { return false; } +/** + * Resolve the owner segment of an `@implementedBy` reference to the node it names. + * + * OBJECTS FIRST, through the loader's own resolver, so package-local binding stays the + * ADR-0042 contract and never a parallel name scan (#228). + * + * Then ROOT-LEVEL NON-OBJECT nodes — `template.prompt` and its siblings today. The + * attribute is documented as naming "the model nodes realising this requirement", and a + * declared prompt is one: it is the durable artifact a capability like "the game master + * is told what the party can see" actually lives in. Resolving only objects meant the + * prompt estate — the thing whose retirement is hardest to see in a model, since a + * removed prompt leaves no table behind — was the one part of a model that could not + * carry a status. So L4 means "a declared top-level model node", not "an object". + * + * Requirements themselves are excluded: hierarchy is nesting, and a requirement claiming + * a requirement would be a second, contradictory parent mechanism. + */ +function resolveClaimTarget(root: MetaData, owner: string, referrerPkg: string): MetaData | undefined { + const { node } = resolveObjectRef(root, owner, referrerPkg); + if (node !== undefined) return node; + + const candidates = root + .children() + .filter((c) => c.type !== TYPE_OBJECT && c.type !== TYPE_REQUIREMENT); + + // A fully-qualified reference binds exactly, like every other FQN in the model. + if (owner.includes(PACKAGE_SEPARATOR)) { + return candidates.find((c) => c.resolutionKey() === owner); + } + // A bare reference prefers the referrer's own package, then a root-level node of that + // bare name. An ambiguous bare name binds NOTHING — same fail-closed rule objects use, + // because silently picking one of two same-named nodes is how a claim ends up pointing + // at the wrong thing without anyone noticing. + const local = referrerPkg === "" ? [] : candidates.filter((c) => c.resolutionKey() === `${referrerPkg}${PACKAGE_SEPARATOR}${owner}`); + if (local.length === 1) return local[0]; + // Root-level (unpackaged) only, matching resolveObjectRef's own bare fallback. A bare + // ref must not reach into an arbitrary package just because the name is unique there. + const bare = candidates.filter((c) => c.name === owner && c.resolutionKey() === owner); + return bare.length === 1 ? bare[0] : undefined; +} + /** Walk dotted member segments by CHILD NAME from an object node. */ function resolveMember(obj: MetaData, path: string[]): MetaData | undefined { let cur: MetaData | undefined = obj; @@ -196,7 +237,7 @@ function claimedObjectKeys(root: MetaData, reqs: MetaRequirement[]): Set const referrerPkg = req.package ?? req.fileDefaultPackage ?? ""; for (const ref of req.implementedBy()) { const { owner, path } = splitMemberRef(ref); - const { node } = resolveObjectRef(root, owner, referrerPkg); + const node = resolveClaimTarget(root, owner, referrerPkg); if (node === undefined) continue; if (path.length > 0 && resolveMember(node, path) === undefined) continue; claimed.add(node.resolutionKey()); @@ -302,7 +343,7 @@ export function checkRequirements(root: MetaData): Diagnostic[] { // binds package-locally under the ADR-0042 contract — the loader's own // resolver, never a parallel name scan (#228). const referrerPkg = req.package ?? req.fileDefaultPackage ?? ""; - const { node } = resolveObjectRef(root, owner, referrerPkg); + const node = resolveClaimTarget(root, owner, referrerPkg); const isObjectRef = path.length === 0; // GRAIN, and it stays functional-only DELIBERATELY. On a functional diff --git a/server/typescript/packages/cli/src/lib/verified-by-scan.ts b/server/typescript/packages/cli/src/lib/verified-by-scan.ts index ab8115804..9831faa08 100644 --- a/server/typescript/packages/cli/src/lib/verified-by-scan.ts +++ b/server/typescript/packages/cli/src/lib/verified-by-scan.ts @@ -21,6 +21,23 @@ // says NOTHING rather than reporting every name missing. Absence of evidence is // not evidence of absence, and a monorepo whose tests live outside `--cwd` must // not be told its requirements are unverified. +// +// WHAT COUNTS AS A TEST FILE IS THE PROJECT'S CALL, NOT OURS. The built-in patterns +// below are a convenience for the ecosystems this repo ports to, and they are a GUESS +// about someone else's repository. They were wrong on a mainstream case from the day +// they shipped: Maven Failsafe names integration tests `FooIT.java`, which matched +// nothing, so a JVM project naming a real integration test got a confident +// "the claim was never true". +// +// Two consequences, both deliberate: +// - `testFiles` (config: `verify.testFiles`) lets a project declare its own +// conventions, unioned with the built-ins. Nothing here can be authoritative +// about a convention we have never seen. +// - the fail-open above is extended from "no test files at all" to the case that +// actually bites: a name we cannot find in the corpus, which IS present in a file +// the corpus definition did not classify. That is our ignorance, not a broken +// claim, and it is reported as such (WARN_REQUIREMENT_TEST_UNCLASSIFIED) rather +// than as an error. The error is reserved for a name that appears NOWHERE. import { readdirSync, readFileSync, statSync } from "node:fs"; import { join, relative, sep } from "node:path"; @@ -33,6 +50,7 @@ import { export const ERR_REQUIREMENT_TEST_MISSING = "ERR_REQUIREMENT_TEST_MISSING"; export const WARN_REQUIREMENT_TEST_SKIPPED = "WARN_REQUIREMENT_TEST_SKIPPED"; export const WARN_REQUIREMENT_TEST_COMMENT_ONLY = "WARN_REQUIREMENT_TEST_COMMENT_ONLY"; +export const WARN_REQUIREMENT_TEST_UNCLASSIFIED = "WARN_REQUIREMENT_TEST_UNCLASSIFIED"; export interface VerifiedByDiagnostic { severity: "error" | "warn"; @@ -46,19 +64,59 @@ const IGNORE_SEGMENTS = new Set([ ".metaobjects", "generated", "target", "bin", "obj", "__pycache__", ".venv", "venv", ]); -/** Test files across the five ecosystems this project ports to. */ +/** + * Test files across the five ecosystems this project ports to — a CONVENIENCE DEFAULT, + * never an authority. A project whose conventions differ declares them via + * `verify.testFiles`; see the module header. + * + * The `IT` entries are Maven Failsafe's own defaults (`IT*`, `*IT`, `*ITCase`), which + * is how every JVM project in the wild names an integration test. Their absence is the + * bug that motivated making this list extensible in the first place. + */ const TEST_FILE = new RegExp( [ "\\.(?:test|spec)\\.[cm]?[jt]sx?$", // bun / jest / vitest / mocha "(?:^|[./_-])[Tt]est[^/]*\\.java$", // JUnit — TestFoo.java "[A-Za-z0-9]Test(?:s)?\\.java$", // JUnit — FooTest.java / FooTests.java + "[A-Za-z0-9]IT(?:Case)?\\.java$", // Failsafe — FooIT.java / FooITCase.java + "^IT[A-Za-z0-9][^/]*\\.java$", // Failsafe — ITFoo.java "[A-Za-z0-9]Tests?\\.cs$", // xUnit / NUnit "^test_[^/]*\\.py$", // pytest "[^/]*_test\\.py$", // pytest, trailing convention "[A-Za-z0-9]Test(?:s)?\\.kt$", // Kotlin + "[A-Za-z0-9]IT(?:Case)?\\.kt$", // Failsafe under Kotlin — FooIT.kt ].join("|"), ); +/** Files worth searching when a name is missing from the corpus, to tell "nowhere" from + * "somewhere I did not classify". Source-ish only; a match in a lockfile proves nothing. */ +const SOURCE_FILE = /\.(?:[cm]?[jt]sx?|java|kt|kts|cs|py|rb|go|rs|scala|groovy|feature)$/; + +/** + * A glob as permissive as the ones adopters actually write (`**​/*IT.kt`, `*.feature`), + * anchored at the project root and matched against forward-slash relative paths. + * + * Deliberately small: `**` spans separators, `*` does not, `?` is one non-separator + * character. Anything richer belongs to a glob library, and pulling one in for a config + * knob this narrow is not worth the dependency. + */ +function globToRegExp(glob: string): RegExp { + let out = ""; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]!; + if (c === "*") { + if (glob[i + 1] === "*") { + // `**/` may match zero segments, so `**/*.feature` matches a root-level file. + if (glob[i + 2] === "/") { out += "(?:.*/)?"; i += 2; } else { out += ".*"; i += 1; } + } else out += "[^/]*"; + continue; + } + if (c === "?") { out += "[^/]"; continue; } + out += c.replace(/[.+^${}()|[\]\\]/g, "\\$&"); + } + return new RegExp(`^${out}$`); +} + /** Markers that a test exists but is disabled, across the same ecosystems. */ const SKIP_MARKER = new RegExp( [ @@ -76,9 +134,18 @@ interface TestCorpus { files: number; /** rel path -> lines, kept so a skip marker can be located near the name. */ byFile: Map; + /** Source files NOT classified as tests, kept only to tell a broken claim from an + * unknown convention. Paths only — contents are read on demand, on the error path. */ + unclassified: string[]; } -function walk(dir: string, root: string, acc: TestCorpus, depth = 0): void { +function walk( + dir: string, + root: string, + acc: TestCorpus, + isTestFile: (rel: string, base: string) => boolean, + depth = 0, +): void { if (depth > 12) return; // pathological trees; the scan is advisory, not exhaustive let entries; try { @@ -89,14 +156,19 @@ function walk(dir: string, root: string, acc: TestCorpus, depth = 0): void { for (const e of entries) { if (e.isDirectory()) { if (IGNORE_SEGMENTS.has(e.name) || e.name.startsWith(".")) continue; - walk(join(dir, e.name), root, acc, depth + 1); + walk(join(dir, e.name), root, acc, isTestFile, depth + 1); continue; } - if (!e.isFile() || !TEST_FILE.test(e.name)) continue; + if (!e.isFile()) continue; const abs = join(dir, e.name); + const rel = relative(root, abs).split(sep).join("/"); + if (!isTestFile(rel, e.name)) { + if (SOURCE_FILE.test(e.name) && acc.unclassified.length < 20_000) acc.unclassified.push(rel); + continue; + } try { if (statSync(abs).size > 512 * 1024) continue; - acc.byFile.set(relative(root, abs).split(sep).join("/"), readFileSync(abs, "utf8").split("\n")); + acc.byFile.set(rel, readFileSync(abs, "utf8").split("\n")); acc.files++; } catch { /* unreadable file is not a finding */ @@ -104,6 +176,45 @@ function walk(dir: string, root: string, acc: TestCorpus, depth = 0): void { } } +/** Does this path or body look like a test the corpus definition simply did not match? + * Deliberately narrow: living under a test directory, or containing an assertion/test + * declaration. Without this, a name occurring anywhere in PRODUCTION source downgrades a + * genuinely broken claim to a warning — the exact failure the comment-only check exists + * to catch. */ +const TESTISH_PATH = /(^|\/)(tests?|spec|__tests__|src\/test)(\/|$)/i; +const TESTISH_BODY = /\b(assert\w*|expect|should|@Test|def test_|it\(|test\(|describe\()/; + +/** Test by LOCATION or by CONTENT — either is enough. Production source with a matching + * name satisfies neither, which is the case that must stay a hard error. */ +function looksLikeTest(rel: string, lines: string[]): boolean { + return TESTISH_PATH.test(rel) || lines.some((l) => TESTISH_BODY.test(l)); +} + +/** + * Where does this name live, if not in the test corpus? + * + * Only ever called on the miss path, so the cost is paid per BROKEN claim rather than + * per run. Returns the first unclassified source file containing the name, which is + * enough to tell the author which pattern they are missing. + */ +function findOutsideCorpus(name: string, root: string, files: string[]): string | undefined { + const rx = wordRx(name); + for (const rel of files) { + try { + const abs = join(root, ...rel.split("/")); + if (statSync(abs).size > 512 * 1024) continue; + const lines = readFileSync(abs, "utf8").split("\n"); + for (let i = 0; i < lines.length; i++) { + const line = lines[i] ?? ""; + if (rx.test(line) && !isCommentLine(line, rel) && looksLikeTest(rel, lines)) return rel; + } + } catch { + /* unreadable file is not a finding */ + } + } + return undefined; +} + /** Every `requirement.*` node in the tree, at any nesting depth. */ function collect(root: MetaData): MetaRequirement[] { const out: MetaRequirement[] = []; @@ -153,12 +264,23 @@ function wordRx(name: string): RegExp { * and silent on `abandoned`/`superseded`, because a retired requirement naming a * deleted test is the entry doing its job, not drift. */ -export function checkVerifiedBy(root: MetaData, cwd: string): VerifiedByDiagnostic[] { +export function checkVerifiedBy( + root: MetaData, + cwd: string, + testFiles?: string[], +): VerifiedByDiagnostic[] { const reqs = collect(root).filter((r) => r.verifiedBy().length > 0); if (reqs.length === 0) return []; // opt-in by declaration - const corpus: TestCorpus = { files: 0, byFile: new Map() }; - walk(cwd, cwd, corpus); + // Project-declared conventions ADD to the built-ins: the failure being fixed is + // under-matching, and a project that names an extra convention is telling us + // something we did not know — not asking us to forget what we did. + const declared = (testFiles ?? []).map(globToRegExp); + const isTestFile = (rel: string, base: string): boolean => + TEST_FILE.test(base) || declared.some((rx) => rx.test(rel)); + + const corpus: TestCorpus = { files: 0, byFile: new Map(), unclassified: [] }; + walk(cwd, cwd, corpus, isTestFile); if (corpus.files === 0) return []; // fail open: nothing to judge against const out: VerifiedByDiagnostic[] = []; @@ -205,15 +327,35 @@ export function checkVerifiedBy(root: MetaData, cwd: string): VerifiedByDiagnost if (foundIn === undefined) { if (req.requiresLiveNodes()) { - out.push({ - severity: "error", - code: ERR_REQUIREMENT_TEST_MISSING, - name: req.name, - message: - `'verifiedBy' names '${test}', which appears in none of the ` + - `${corpus.files} test file(s) found under this project. Either the test was ` + - `renamed or removed, or the claim was never true.`, - }); + // Before calling a claim broken, rule out the likelier explanation: that this + // project names its tests in a way the corpus definition does not know. A name + // sitting in an unclassified source file is OUR ignorance, and saying "the claim + // was never true" about it is the tool being confidently wrong. + const elsewhere = findOutsideCorpus(test, cwd, corpus.unclassified); + out.push( + elsewhere !== undefined + ? { + severity: "warn", + code: WARN_REQUIREMENT_TEST_UNCLASSIFIED, + name: req.name, + message: + `'verifiedBy' names '${test}', which is not in any of the ${corpus.files} ` + + `file(s) recognised as tests, but DOES appear in ${elsewhere}. That file is ` + + `probably a test this scan does not know how to recognise — declare the ` + + `convention in metaobjects.config.ts (verify.testFiles, e.g. ` + + `["**/*IT.kt"]) and this becomes a real check instead of a guess.`, + } + : { + severity: "error", + code: ERR_REQUIREMENT_TEST_MISSING, + name: req.name, + message: + `'verifiedBy' names '${test}', which appears in none of the ` + + `${corpus.files} test file(s) found under this project, and in no other ` + + `source file either. Either the test was renamed or removed, or the ` + + `claim was never true.`, + }, + ); } continue; } diff --git a/server/typescript/packages/cli/test/requirement-template-refs.test.ts b/server/typescript/packages/cli/test/requirement-template-refs.test.ts new file mode 100644 index 000000000..32d5d3d2e --- /dev/null +++ b/server/typescript/packages/cli/test/requirement-template-refs.test.ts @@ -0,0 +1,159 @@ +// `@implementedBy` — WHAT KIND OF NODE MAY BE CLAIMED. +// +// `implementedBy` is documented as "FQN references to the model nodes realising this +// requirement", and it resolved through the OBJECT resolver only. So a requirement could +// claim an entity, a value or a projection — and could not claim a `template.prompt`, +// even though a declared prompt is a model node realising a capability in exactly the +// same sense, and is arguably the node most in need of a status: a prompt that was +// retired, or replaced by a different one, is invisible in the model otherwise. +// +// Naming one produced ERR_REQUIREMENT_DANGLING_REF -- "the model moved and the +// requirement is stale" -- for a template sitting in the loaded tree. +// +// L4 therefore means "a declared top-level model node", not "an object". L5 still means +// a member of one. Coverage is untouched and stays entity-grain: claiming a template +// must not silence the unclaimed-entity warning. + +import { test, expect, describe } from "bun:test"; +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { loadDirectory } from "@metaobjectsdev/metadata"; +import { checkRequirements } from "../src/lib/requirement-check.js"; + +/** A project with one value object, one prompt template, and the given requirement. */ +function project(requirement: Record, subType = "functional"): string { + const dir = mkdtempSync(join(tmpdir(), "rtref-")); + mkdirSync(join(dir, "metaobjects"), { recursive: true }); + writeFileSync( + join(dir, "metaobjects", "meta.shop.json"), + JSON.stringify({ + "metadata.root": { + package: "acme::shop", + children: [ + { + "object.entity": { + name: "Order", + children: [ + { "field.uuid": { name: "id" } }, + { + "field.currency": { + name: "priceCents", + "@currency": "USD", + children: [ + { "view.currency": { name: "display", "@locale": "en-US" } }, + { "validator.length": { name: "bounded", "@min": 1, "@max": 12 } }, + ], + }, + }, + { "identity.primary": { name: "pk", "@fields": ["id"] } }, + ], + }, + }, + { "object.value": { name: "GreetPayload", children: [{ "field.string": { name: "who" } }] } }, + { + "template.prompt": { + name: "greeting", + "@payloadRef": "acme::shop::GreetPayload", + "@textRef": "greeting.md", + }, + }, + { [`requirement.${subType}`]: requirement }, + ], + }, + }), + ); + return dir; +} + +async function check(dir: string) { + const res = await loadDirectory(join(dir, "metaobjects")); + return checkRequirements(res.root); +} + +const L4 = { + name: "greets", + "@level": 4, + "@status": "live", + "@statement": "The assistant greets the user by name.", + "@violation": "A greeting addressed to nobody.", +}; + +describe("@implementedBy — templates are claimable model nodes", () => { + test("a functional L4 may claim a template.prompt by FQN", async () => { + const diags = await check(project({ ...L4, "@implementedBy": ["acme::shop::greeting"] })); + expect(diags.filter((d) => d.severity === "error")).toEqual([]); + }); + + test("a bare reference binds package-locally, as it does for objects", async () => { + const diags = await check(project({ ...L4, "@implementedBy": ["greeting"] })); + expect(diags.filter((d) => d.severity === "error")).toEqual([]); + }); + + test("an architectural requirement may claim templates too", async () => { + const diags = await check( + project( + { + name: "promptsDeclareTheirPayload", + "@status": "live", + "@statement": "Every declared prompt names the payload it renders.", + "@violation": "A prompt whose fields nobody can diff.", + "@implementedBy": ["acme::shop::greeting"], + }, + "architectural", + ), + ); + expect(diags.filter((d) => d.severity === "error")).toEqual([]); + }); + + test("a template that does NOT exist still dangles", async () => { + const diags = await check(project({ ...L4, "@implementedBy": ["acme::shop::farewell"] })); + expect(diags.filter((d) => d.severity === "error")).toHaveLength(1); + }); + + // Coverage is entity grain by design. If claiming a template counted, a project could + // clear its unclaimed-entity warning without ever claiming an entity. + test("claiming a template does not count toward entity coverage", async () => { + const diags = await check(project({ ...L4, "@implementedBy": ["acme::shop::greeting"] })); + const warns = diags.filter((d) => d.severity === "warn"); + expect(warns.some((d) => d.message.includes("Order"))).toBe(true); + }); +}); + +// L5 is documented as "a field, view or identity". A requirement about a specific +// FIELD ("money is stored in minor units"), a specific VIEW ("the grid renders this +// as currency"), or a specific VALIDATOR ("this is bounded") is the grain most claims +// about behaviour actually live at, so each is asserted here rather than assumed from +// the resolver walking child names generically. +const L5 = { + name: "priceIsMoney", + "@level": 5, + "@status": "live", + "@statement": "The order price is money and says so.", + "@violation": "A price summed with a price of another currency.", +}; + +describe("@implementedBy — L5 member grains", () => { + const cases: Array<[string, string]> = [ + ["a field", "acme::shop::Order.priceCents"], + ["a view under a field", "acme::shop::Order.priceCents.display"], + ["a validator under a field", "acme::shop::Order.priceCents.bounded"], + ["an identity", "acme::shop::Order.pk"], + ]; + for (const [label, ref] of cases) { + test(`L5 may claim ${label}`, async () => { + const diags = await check(project({ ...L5, "@implementedBy": [ref] })); + expect(diags.filter((d) => d.severity === "error")).toEqual([]); + }); + } + + test("a member that does not exist still dangles", async () => { + const diags = await check(project({ ...L5, "@implementedBy": ["acme::shop::Order.nope"] })); + expect(diags.filter((d) => d.severity === "error")).toHaveLength(1); + }); + + test("a member of a TEMPLATE resolves too", async () => { + const diags = await check(project({ ...L5, "@implementedBy": ["acme::shop::greeting.tone"] })); + expect(diags.filter((d) => d.severity === "error")).toHaveLength(1); // no such child yet + }); +}); diff --git a/server/typescript/packages/cli/test/verified-by-corpus.test.ts b/server/typescript/packages/cli/test/verified-by-corpus.test.ts new file mode 100644 index 000000000..5d1efdc0c --- /dev/null +++ b/server/typescript/packages/cli/test/verified-by-corpus.test.ts @@ -0,0 +1,176 @@ +// `@verifiedBy` — WHAT COUNTS AS A TEST FILE. +// +// The scan used to carry one closed regex list of test-file conventions for the five +// ported ecosystems, and nothing could extend it. That list is a guess about someone +// else's project, and it was wrong on a mainstream case immediately: Maven Failsafe +// names integration tests `FooIT.java` / `FooIT.kt`, which matched nothing. Because the +// scan only fails OPEN when it sees ZERO test files, a JVM project with unit tests +// (matched) plus integration tests (unmatched) got a confident +// ERR_REQUIREMENT_TEST_MISSING — "the claim was never true" — for a test sitting in the +// repo. +// +// Two things are asserted here, and they are different claims: +// 1. the built-in defaults cover the conventions we ship support for, Failsafe included; +// 2. a project can DECLARE its own convention, because test naming is project-specific +// and no built-in list can be authoritative about it. +// +// And the third, which is the real fix: when a named test cannot be found, the scan must +// distinguish "this name is nowhere" (a broken claim — error) from "this name is in a +// file I did not classify as a test" (an unknown convention — warn, and say so). Asserting +// the first when the second is true is the failure this file exists to prevent. + +import { test, expect, describe } from "bun:test"; +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { loadDirectory } from "@metaobjectsdev/metadata"; +import { + checkVerifiedBy, + ERR_REQUIREMENT_TEST_MISSING, + WARN_REQUIREMENT_TEST_UNCLASSIFIED, +} from "../src/lib/verified-by-scan.js"; + +const ENTITIES = JSON.stringify({ + "metadata.root": { + package: "acme::shop", + children: [ + { + "object.entity": { + name: "Order", + children: [ + { "field.uuid": { name: "id" } }, + { "identity.primary": { name: "pk", "@fields": ["id"] } }, + ], + }, + }, + ], + }, +}); + +const requirements = (verifiedBy: string[]) => + JSON.stringify({ + "metadata.root": { + package: "acme::shop", + children: [ + { + "requirement.functional": { + name: "orderRecord", + "@level": 4, + "@status": "live", + "@statement": "An order is a durable record.", + "@violation": "An order vanishes on restart.", + "@implementedBy": ["Order"], + "@verifiedBy": verifiedBy, + }, + }, + ], + }, + }); + +/** A project holding the given files, plus one requirement naming `verifiedBy`. */ +function project(verifiedBy: string[], files: Record): string { + const dir = mkdtempSync(join(tmpdir(), "vby-")); + mkdirSync(join(dir, "metaobjects"), { recursive: true }); + writeFileSync(join(dir, "metaobjects", "meta.shop.json"), ENTITIES); + writeFileSync(join(dir, "metaobjects", "meta.req.json"), requirements(verifiedBy)); + for (const [rel, body] of Object.entries(files)) { + const abs = join(dir, rel); + mkdirSync(join(abs, ".."), { recursive: true }); + writeFileSync(abs, body); + } + return dir; +} + +async function scan(dir: string, testFiles?: string[]) { + const res = await loadDirectory(join(dir, "metaobjects")); + return checkVerifiedBy(res.root, dir, testFiles); +} + +// A unit test that DOES match the built-in patterns, so the corpus is never empty and +// the fail-open-on-zero path is not what is being exercised. +const UNIT_TEST = "class PlacesOrderTest { void placesOrder() {} }"; + +describe("@verifiedBy — built-in conventions", () => { + test("Maven Failsafe *IT.java counts as a test file", async () => { + const dir = project(["OrderFlowIT"], { + "src/test/java/OrderTest.java": UNIT_TEST, + "src/test/java/OrderFlowIT.java": "class OrderFlowIT { void endToEnd() {} }", + }); + expect(await scan(dir)).toEqual([]); + }); + + test("Maven Failsafe *IT.kt counts as a test file", async () => { + const dir = project(["OrderFlowIT"], { + "src/test/kotlin/OrderTest.kt": UNIT_TEST, + "src/test/kotlin/OrderFlowIT.kt": "class OrderFlowIT { fun endToEnd() {} }", + }); + expect(await scan(dir)).toEqual([]); + }); + + test("Failsafe *ITCase.java counts as a test file", async () => { + const dir = project(["OrderFlowITCase"], { + "src/test/java/OrderTest.java": UNIT_TEST, + "src/test/java/OrderFlowITCase.java": "class OrderFlowITCase {}", + }); + expect(await scan(dir)).toEqual([]); + }); + + test("a name that exists NOWHERE is still an error", async () => { + const dir = project(["NoSuchTest"], { "src/test/java/OrderTest.java": UNIT_TEST }); + const diags = await scan(dir); + expect(diags).toHaveLength(1); + expect(diags[0]?.code).toBe(ERR_REQUIREMENT_TEST_MISSING); + }); +}); + +describe("@verifiedBy — project-declared conventions", () => { + test("a project can declare a convention the built-ins do not know", async () => { + const dir = project(["order_behaviour"], { + "src/test/java/OrderTest.java": UNIT_TEST, + // Nothing built-in matches this. The project says what its tests look like. + "spec/order_behaviour.feature": "Scenario: order_behaviour", + }); + expect(await scan(dir, ["**/*.feature"])).toEqual([]); + }); + + test("a declared convention ADDS to the built-ins rather than replacing them", async () => { + const dir = project(["PlacesOrderTest"], { + "src/test/java/PlacesOrderTest.java": UNIT_TEST, + "spec/x.feature": "Scenario: unrelated", + }); + expect(await scan(dir, ["**/*.feature"])).toEqual([]); + }); +}); + +describe("@verifiedBy — an unknown convention is not a broken claim", () => { + // THE POINT OF THE WHOLE FILE. The name is right there in the repo. Reporting + // "the claim was never true" is the tool being confidently wrong about a project + // whose conventions it was never told. + test("a name found in an unclassified file warns, and does not error", async () => { + const dir = project(["OrderBehaviourSuite"], { + "src/test/java/OrderTest.java": UNIT_TEST, + "src/test/java/OrderBehaviourSuite.java": "class OrderBehaviourSuite { void placesOrder() {} }", + }); + const diags = await scan(dir); + expect(diags).toHaveLength(1); + expect(diags[0]?.code).toBe(WARN_REQUIREMENT_TEST_UNCLASSIFIED); + expect(diags[0]?.severity).toBe("warn"); + }); + + test("the warning names the file it found, so the fix is obvious", async () => { + const dir = project(["OrderBehaviourSuite"], { + "src/test/java/OrderTest.java": UNIT_TEST, + "src/test/java/OrderBehaviourSuite.java": "class OrderBehaviourSuite {}", + }); + const [diag] = await scan(dir); + expect(diag?.message).toContain("src/test/java/OrderBehaviourSuite.java"); + }); + + test("declaring the convention clears the warning entirely", async () => { + const dir = project(["OrderBehaviourSuite"], { + "src/test/java/OrderTest.java": UNIT_TEST, + "src/test/java/OrderBehaviourSuite.java": "class OrderBehaviourSuite {}", + }); + expect(await scan(dir, ["**/*Suite.java"])).toEqual([]); + }); +}); diff --git a/server/typescript/packages/codegen-ts/src/enum-shared.ts b/server/typescript/packages/codegen-ts/src/enum-shared.ts index e53114ccd..54f0a46be 100644 --- a/server/typescript/packages/codegen-ts/src/enum-shared.ts +++ b/server/typescript/packages/codegen-ts/src/enum-shared.ts @@ -62,7 +62,13 @@ export function sharedEnumForField(field: MetaField): SharedEnum | undefined { return { name: toPascalCase(decl.name), values, - provided: decl.attr(FIELD_ATTR_PROVIDED) === true, + // ADR-0039 sanctioned own: @provided is a declaration-layer provenance marker + // ("THIS type is supplied by hand-written/third-party code"), like `abstract` — + // it does not flow down an extends chain. A resolving read misfires on a chained + // declaration (root abstract `B extends` root abstract `@provided A`): B would be + // reported provided and emit a reference to a hand-written `B` the adopter never + // declared, instead of materializing B. Matches the JVM ports. + provided: decl.ownAttrs().get(FIELD_ATTR_PROVIDED) === true, }; } diff --git a/server/typescript/packages/codegen-ts/src/index.ts b/server/typescript/packages/codegen-ts/src/index.ts index c31e0c649..8390318d8 100644 --- a/server/typescript/packages/codegen-ts/src/index.ts +++ b/server/typescript/packages/codegen-ts/src/index.ts @@ -37,7 +37,7 @@ export { } from "./generator-registry.js"; export type { GeneratorRegistryEntry, GeneratorTier } from "./generator-registry.js"; -export type { MetaobjectsGenConfig, NormalizedMetaobjectsGenConfig, ResolvedGenConfig, Dialect, ExtStyle, ColumnNamingStrategy, MetaDataTypeProvider, GeneratorSpec, DocsConfig, ResolvedDocsConfig, DocsSurface, ApiSurface } from "./metaobjects-config.js"; +export type { MetaobjectsGenConfig, NormalizedMetaobjectsGenConfig, ResolvedGenConfig, Dialect, ExtStyle, ColumnNamingStrategy, MetaDataTypeProvider, GeneratorSpec, DocsConfig, ResolvedDocsConfig, DocsSurface, ApiSurface, VerifyConfig } from "./metaobjects-config.js"; export { defineConfig, normalizeConfig, resolveGenerators, resolveDocsConfig } from "./metaobjects-config.js"; export { apiLabel } from "./generators/api-label.js"; diff --git a/server/typescript/packages/codegen-ts/src/metaobjects-config.ts b/server/typescript/packages/codegen-ts/src/metaobjects-config.ts index 81d61f95d..38aeddf71 100644 --- a/server/typescript/packages/codegen-ts/src/metaobjects-config.ts +++ b/server/typescript/packages/codegen-ts/src/metaobjects-config.ts @@ -154,6 +154,30 @@ export interface MetaobjectsGenConfig extends Omit { expect(t).not.toContain('from "./enums"'); }); }); + +// ── @provided is DECLARATION-LAYER, not inherited ──────────────────────────── +// +// `@provided` says "THIS type is supplied by hand-written / third-party code". Like +// `abstract`, it is a fact about the declaration, not about the values it carries, so +// it must NOT flow down an extends chain. TS, C# and Python read it RESOLVING while +// Java and Kotlin read it own-only; the JVM side was right, and the divergence is +// reachable only through a CHAINED declaration — a root-level abstract enum `B extends` +// a root-level abstract `@provided A`. +// +// Under a resolving read, B is classified provided and the ports emit an import of a +// hand-written `B` THE ADOPTER NEVER DECLARED (the marker was authored on A), instead of +// materializing B from its inherited @values. This test pins the own-only read; it did +// not exist when the fix was written, so the behaviour was unguarded in three ports. + +/** Root abstract `@provided Base`, root abstract `Derived extends Base`, entity uses Derived. */ +function chainedProvidedModel(): unknown { + return { + "metadata.root": { + package: "acme", + children: [ + { "field.enum": { name: "Base", abstract: true, "@provided": true, "@values": ["A", "B"] } }, + { "field.enum": { name: "Derived", abstract: true, extends: "Base" } }, + { + "object.entity": { + name: "Order", + children: [ + { "field.long": { name: "id" } }, + { "field.enum": { name: "kind", extends: "Derived" } }, + { "source.rdb": { "@table": "orders" } }, + { "identity.primary": { name: "id", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + ], + }, + }; +} + +describe("FR-019 @provided does not inherit (ADR-0039 own read)", () => { + test("a chained declaration MATERIALIZES rather than importing a type nobody declared", async () => { + const root = await loadRoot(chainedProvidedModel()); + const { files } = await gen(root, "~/hand-written-enums"); + + // Derived is NOT provided, so it must be emitted... + expect(files["enums.ts"]).toBeDefined(); + expect(files["enums.ts"]).toContain("Derived"); + // ...and must NOT be imported from the provided module. + const all = Object.values(files).join("\n"); + expect(all).not.toContain('Derived } from "~/hand-written-enums"'); + }); + + test("the marked declaration itself is still provided", async () => { + const root = await loadRoot(sharedModel({ provided: true })); + const { files } = await gen(root, "~/hand-written-enums"); + // Status carries @provided on its OWN declaration — nothing emitted for it. + expect(files["enums.ts"]).toBeUndefined(); + }); +}); diff --git a/server/typescript/packages/render/src/payload-accessors.ts b/server/typescript/packages/render/src/payload-accessors.ts new file mode 100644 index 000000000..ad5122658 --- /dev/null +++ b/server/typescript/packages/render/src/payload-accessors.ts @@ -0,0 +1,97 @@ +// Derived boolean accessors — `{{#hasFoo}}` over a payload field `foo`. +// +// A prompt needs conditional sections ("include the abilities block only when there +// ARE abilities"), and the payload contract answers that with a DERIVED accessor +// rather than an authored boolean field: the author declares `abilities`, and +// `hasAbilities` follows from it. Declaring both would let them disagree. +// +// THE RULE IS SHARED ON PURPOSE. The JVM has carried this since 7.7.7 +// (`com.metaobjects.render.PayloadAccessors`, emitted by `SpringPayloadGenerator` +// onto every generated payload record and accepted by `render.Verify`), and its +// comment says the emitter and the verifier share one rule so they "can never drift +// apart". TypeScript had neither half, which is why the same template verified clean +// on the JVM and reported drift here — and, worse, RENDERED WRONG rather than +// failing: `{{#hasAbilities}}` resolved to nothing on a populated payload, so the +// section silently vanished. This module is the TS half of that shared rule. + +/** The `has` prefix every derived boolean accessor carries. */ +export const HAS_PREFIX = "has"; + +/** + * The boolean-accessor section name for a payload field: `"has" + capitalize(name)` + * (`abilities` → `hasAbilities`). Byte-identical to the JVM's + * `PayloadAccessors.hasAccessorName`, including its capitalize, which leaves an + * already-uppercase first character untouched. + */ +export function hasAccessorName(fieldName: string): string { + return HAS_PREFIX + capitalize(fieldName); +} + +/** Capitalize the first character, leaving an already-uppercase one untouched. */ +export function capitalize(s: string): string { + if (s.length === 0) return s; + const c0 = s.charAt(0); + if (c0 === c0.toUpperCase() && c0 !== c0.toLowerCase()) return s; + return c0.toUpperCase() + s.slice(1); +} + +/** + * Is `value` "present" for the purposes of `has`? + * + * Mirrors the JVM emitter's per-type bodies exactly: + * string → non-null AND non-blank (`!foo.isBlank()`, so whitespace is absent) + * array → non-null AND non-empty (`!foo.isEmpty()`) + * reference → non-null (any other object) + * + * Returns `undefined` for numbers and booleans, which the JVM deliberately emits NO + * accessor for — they are always-present scalars, and a `{{#hasCount}}` over an int + * is drift rather than a conditional. Returning undefined (rather than false) keeps + * that distinction: nothing is injected, so the name stays unresolved exactly as it + * is on a generated Java record that has no such method. + */ +export function accessorValue(value: unknown): boolean | undefined { + if (value === null || value === undefined) return false; + if (typeof value === "string") return value.trim().length > 0; + if (Array.isArray(value)) return value.length > 0; + if (typeof value === "number" || typeof value === "boolean" || typeof value === "bigint") { + return undefined; + } + return typeof value === "object"; +} + +/** + * A view over `payload` carrying its derived `has` accessors, recursively. + * + * NON-MUTATING — the caller's payload is never touched, because a render must not + * be able to change the object it was handed. An AUTHORED key always wins: if a + * payload genuinely carries `hasFoo`, that value is kept rather than shadowed by a + * derived one. + * + * Recursion follows Mustache's own scoping: every nested object and every array + * ELEMENT becomes a context in its own right, so a section over `abilities` sees + * the accessors of the ability it is currently iterating. + */ +export function withDerivedAccessors(payload: T, depth = 0): T { + if (depth > 32) return payload; // pathological graph; render is not a validator + if (Array.isArray(payload)) { + return payload.map((v) => withDerivedAccessors(v, depth + 1)) as unknown as T; + } + if (payload === null || typeof payload !== "object") return payload; + // PLAIN objects only. Rebuilding from Object.entries() would flatten anything with + // its own prototype — a Date stringifies to "[object Object]" and a class instance + // loses its getters — and the other ports only rebuild map-shaped values, so + // rebuilding more here would be a divergence as well as a regression. + const proto = Object.getPrototypeOf(payload); + if (proto !== Object.prototype && proto !== null) return payload; + + const src = payload as Record; + const out: Record = {}; + for (const [k, v] of Object.entries(src)) out[k] = withDerivedAccessors(v, depth + 1); + for (const [k, v] of Object.entries(src)) { + const name = hasAccessorName(k); + if (Object.prototype.hasOwnProperty.call(src, name)) continue; // authored wins + const derived = accessorValue(v); + if (derived !== undefined) out[name] = derived; + } + return out as unknown as T; +} diff --git a/server/typescript/packages/render/src/render.ts b/server/typescript/packages/render/src/render.ts index df606760a..3290c0e02 100644 --- a/server/typescript/packages/render/src/render.ts +++ b/server/typescript/packages/render/src/render.ts @@ -2,6 +2,7 @@ import Mustache from "mustache"; import type { Provider } from "./provider.js"; import { ESCAPERS, type RenderFormat } from "./escapers.js"; import { verify, ERR_REQUIRED_SLOT_UNUSED, type PayloadField } from "./verify.js"; +import { withDerivedAccessors } from "./payload-accessors.js"; const MAX_DEPTH = 32; const PARTIAL = /\{\{>\s*([^}\s]+)\s*\}\}/g; @@ -68,7 +69,10 @@ export function render(o: RenderOptions): string { Mustache.escape = (v: unknown) => escaper(typeof v === "string" ? v : String(v)); let result: string; try { - result = Mustache.render(expanded, o.payload, {}); + // Derived `has` accessors are part of the payload contract, not of the + // caller's object — see payload-accessors.ts. Injected here so a `{{#hasFoo}}` + // section resolves the same way it does against a generated JVM payload record. + result = Mustache.render(expanded, withDerivedAccessors(o.payload), {}); } finally { Mustache.escape = prev; } diff --git a/server/typescript/packages/render/src/verify.ts b/server/typescript/packages/render/src/verify.ts index 1d2f02165..f086d809b 100644 --- a/server/typescript/packages/render/src/verify.ts +++ b/server/typescript/packages/render/src/verify.ts @@ -11,6 +11,7 @@ import Mustache from "mustache"; import type { Provider } from "./provider.js"; +import { HAS_PREFIX, hasAccessorName } from "./payload-accessors.js"; /** A `{{var}}` references a field the (contextual) payload does not declare. */ export const ERR_VAR_NOT_ON_PAYLOAD = "ERR_VAR_NOT_ON_PAYLOAD"; @@ -62,6 +63,27 @@ type Token = readonly unknown[]; */ export type ResolveStack = readonly F[][]; +/** + * True when `name` is a derived boolean accessor (`has`) over a field + * reachable on the current context stack — the same rule the payload emitter uses + * (payload-accessors.ts), so an accepted section and an emitted accessor can never + * drift apart. A `{{#hasX}}` with no field `x` on any scope is NOT an accessor and + * stays ERR_VAR_NOT_ON_PAYLOAD drift. + * + * Accessors are simple (undotted) names; a dotted path is never an accessor and is + * left to normal field resolution. Byte-identical to the JVM's + * `Verify.isBooleanAccessor`, including its deliberate permissiveness: acceptance + * keys off the FIELD EXISTING, not off its type. + */ +function isBooleanAccessor(stack: ResolveStack, name: string): boolean { + if (name.includes(".")) return false; + if (!name.startsWith(HAS_PREFIX)) return false; + for (let i = stack.length - 1; i >= 0; i--) { + for (const f of stack[i]!) if (name === hasAccessorName(f.name)) return true; + } + return false; +} + function find(fields: F[], name: string): F | undefined { return fields.find((f) => f.name === name); } @@ -162,7 +184,8 @@ export function verify( // {{{x}}} (spec); mustache.js emits "&" for it too if (value === ".") break; // implicit iterator — always valid if (atRoot) referencedAtRoot.add(value.split(".")[0]!); - if (!resolve(stack, value)) errors.push({ code: ERR_VAR_NOT_ON_PAYLOAD, path: value }); + if (!resolve(stack, value) && !isBooleanAccessor(stack, value)) + errors.push({ code: ERR_VAR_NOT_ON_PAYLOAD, path: value }); break; } case "#": // {{#x}}…{{/x}} @@ -176,6 +199,13 @@ export function verify( if (atRoot) referencedAtRoot.add(value.split(".")[0]!); const field = resolve(stack, value); if (!field) { + // A derived `has` gate is a BOOLEAN over the current context, so it + // resolves nothing and pushes nothing — walk the body in the SAME scope, + // which is what `{{#hasAbilities}}{{#abilities}}…` depends on. + if (isBooleanAccessor(stack, value)) { + walk(sub, stack, seen); + break; + } // Unresolved section head is itself drift; skip the body (its // context is unknowable, walking it would cascade false errors). errors.push({ code: ERR_VAR_NOT_ON_PAYLOAD, path: value }); diff --git a/server/typescript/packages/render/test/payload-accessors.test.ts b/server/typescript/packages/render/test/payload-accessors.test.ts new file mode 100644 index 000000000..eac86eacd --- /dev/null +++ b/server/typescript/packages/render/test/payload-accessors.test.ts @@ -0,0 +1,162 @@ +// Derived `has` accessors — the TS half of a rule the JVM has carried since 7.7.7. +// +// A prompt needs conditional sections, and the payload contract answers that with a +// DERIVED accessor: declare `abilities`, get `hasAbilities`. The JVM emits +// `has()` onto every generated payload record (SpringPayloadGenerator) and +// accepts `{{#has}}` in its static drift check (render.Verify), sharing one +// naming rule so the two can never disagree. +// +// TypeScript had NEITHER half, and the consequence was not a loud one. Verify reported +// ERR_VAR_NOT_ON_PAYLOAD for a template the JVM verified clean — and render silently +// produced the WRONG STRING: `{{#hasAbilities}}` resolved to nothing on a populated +// payload, so the section vanished and the prompt shipped without its abilities block. +// An adopter with a JVM-authored prompt estate saw 157 of these, all `has`-prefixed. +// +// The corpus is why it survived: fixtures/render-conformance/ had no case using a +// derived accessor at all, so the gate that exists to keep the ports identical never +// looked at this shape. + +import { test, expect, describe } from "bun:test"; +import { render } from "../src/render.js"; +import { verify } from "../src/verify.js"; +import { hasAccessorName, accessorValue, withDerivedAccessors } from "../src/payload-accessors.js"; +import type { PayloadField } from "../src/verify.js"; + +const provider = { resolve: () => undefined }; +const r = (template: string, payload: unknown) => render({ template, payload, provider }); + +describe("the naming rule", () => { + test("mirrors the JVM: has + capitalize", () => { + expect(hasAccessorName("abilities")).toBe("hasAbilities"); + expect(hasAccessorName("a")).toBe("hasA"); + }); + + test("an already-capitalized first character is left alone", () => { + expect(hasAccessorName("Abilities")).toBe("hasAbilities"); + }); +}); + +describe("presence semantics mirror the JVM emitter's per-type bodies", () => { + test("string → non-null and non-blank", () => { + expect(accessorValue("x")).toBe(true); + expect(accessorValue("")).toBe(false); + expect(accessorValue(" ")).toBe(false); // isBlank(), not isEmpty() + }); + + test("array → non-null and non-empty", () => { + expect(accessorValue([1])).toBe(true); + expect(accessorValue([])).toBe(false); + }); + + test("reference → non-null", () => { + expect(accessorValue({})).toBe(true); + expect(accessorValue(null)).toBe(false); + expect(accessorValue(undefined)).toBe(false); + }); + + // The JVM emits NO hasFoo for a primitive, so there is nothing to resolve there. + // Deriving `false` would be worse than deriving nothing: it would make a template + // that is drift on the JVM render quietly on TS. + test("numbers and booleans derive NOTHING", () => { + expect(accessorValue(0)).toBeUndefined(); + expect(accessorValue(42)).toBeUndefined(); + expect(accessorValue(false)).toBeUndefined(); + }); +}); + +describe("render — the bug this fixes", () => { + const template = + "Abilities:{{#hasAbilities}}{{#abilities}} [{{name}}]{{/abilities}}{{/hasAbilities}}{{^hasAbilities}} none{{/hasAbilities}}"; + + test("a populated collection renders its section (was: silently dropped)", () => { + expect(r(template, { abilities: [{ name: "Fireball" }] })).toBe("Abilities: [Fireball]"); + }); + + test("an empty collection takes the inverted branch", () => { + expect(r(template, { abilities: [] })).toBe("Abilities: none"); + }); + + test("a blank string is absent, matching isBlank()", () => { + expect(r("{{#hasBio}}{{bio}}{{/hasBio}}{{^hasBio}}-{{/hasBio}}", { bio: " " })).toBe("-"); + }); + + test("accessors are derived inside a nested scope too", () => { + const t = "{{#items}}{{#hasTags}}<{{#tags}}{{.}}{{/tags}}>{{/hasTags}}{{/items}}"; + expect(r(t, { items: [{ tags: ["a"] }, { tags: [] }] })).toBe(""); + }); + + test("an AUTHORED hasFoo wins over the derived one", () => { + expect(r("{{#hasBio}}yes{{/hasBio}}{{^hasBio}}no{{/hasBio}}", { bio: "x", hasBio: false })).toBe("no"); + }); + + test("the caller's payload is never mutated", () => { + const payload = { abilities: [{ name: "Fireball" }] }; + r(template, payload); + expect(Object.keys(payload)).toEqual(["abilities"]); + }); +}); + +describe("verify — accepts exactly what render resolves", () => { + const fields: PayloadField[] = [ + { name: "abilities", fields: [{ name: "name" }] }, + { name: "bio" }, + ]; + + test("a has-section over a declared field is not drift", () => { + expect(verify("{{#hasAbilities}}{{#abilities}}{{name}}{{/abilities}}{{/hasAbilities}}", fields)).toEqual([]); + }); + + test("an inverted has-section is not drift", () => { + expect(verify("{{^hasBio}}none{{/hasBio}}", fields)).toEqual([]); + }); + + test("a has-section over a field that does NOT exist is still drift", () => { + const errs = verify("{{#hasNope}}x{{/hasNope}}", fields); + expect(errs).toHaveLength(1); + expect(errs[0]?.path).toBe("hasNope"); + }); + + // The body of a has-section is scoped to the SAME context — the gate is a boolean, + // not a container — so a bad variable inside it must still be caught. + test("drift inside a has-section body is still reported", () => { + const errs = verify("{{#hasAbilities}}{{nope}}{{/hasAbilities}}", fields); + expect(errs).toHaveLength(1); + expect(errs[0]?.path).toBe("nope"); + }); + + test("a dotted path is never treated as an accessor", () => { + expect(verify("{{abilities.hasName}}", fields)).toHaveLength(1); + }); +}); + +// ── Regressions caught in review of the original change ────────────────────── +// +// The first draft rebuilt ANY non-array object from Object.entries(), which flattened +// everything carrying its own prototype: a Date stringified to "[object Object]" and a +// class instance lost its getters. Rendering is not allowed to reshape the payload; only +// plain map-shaped values get derived keys, which is also what the other four ports do. +describe("only PLAIN objects are rebuilt", () => { + test("a Date still renders as a Date", () => { + const out = r("{{when}}", { when: new Date("2020-01-02T03:04:05Z") }); + expect(out).not.toContain("[object Object]"); + expect(out).toContain("2020"); + }); + + test("a class instance keeps its prototype getters", () => { + class P { + constructor(public a = 1) {} + get b(): number { + return 2; + } + } + expect(r("{{b}}", new P())).toBe("2"); + }); + + test("a Date-valued field still derives its accessor", () => { + expect(r("{{#hasWhen}}Y{{/hasWhen}}", { when: new Date() })).toBe("Y"); + }); + + test("plain nested objects still get accessors", () => { + expect(r("{{#inner}}{{#hasXs}}Y{{/hasXs}}{{/inner}}", { inner: { xs: [1] } })).toBe("Y"); + }); +}); diff --git a/spec/decisions/ADR-0039-own-accessor-discipline.md b/spec/decisions/ADR-0039-own-accessor-discipline.md index 0b09c9b7d..df77ec4a4 100644 --- a/spec/decisions/ADR-0039-own-accessor-discipline.md +++ b/spec/decisions/ADR-0039-own-accessor-discipline.md @@ -26,8 +26,15 @@ Two metamodel-internal siblings use the same *"emit only the declared-here layer - **Iterating members for runtime, validation, effective serialization, schema building, or extract** → resolve (`fields()`/`children()`/`attrs()`), because you need the *effective* set including inherited members. - **"Root scans that only work because root is never extended"** (`root.OwnChildren()`) → still resolve. Working-by-accident is the fragile pattern this ADR eliminates. -### The physical exception -`@dbColumnType` is **never inherited** by explicit policy (a physical column-type override is not a logical property). It stays own-only, documented as such at the read site. This is the *only* attribute deliberately read own-only outside the emit-declared-here cases. +### The deliberately-own-only attributes +Two attributes are read own-only by explicit policy, outside the emit-declared-here cases. Each is documented as such at every read site. + +- **`@dbColumnType`** — **never inherited**: a physical column-type override is not a logical property. +- **`@provided`** (FR-019 / [ADR-0026](ADR-0026-shared-and-provided-named-types.md)) — a **declaration-layer provenance marker**, not a property of the values it carries. It asserts "*this* named type is supplied by hand-written / third-party code, so emit nothing and reference it", which is a fact about the declaration itself — like `abstract` — and does not flow down an `extends` chain. + + The distinction is only observable on a **chained declaration**: a root-level abstract enum `B extends` a root-level abstract `@provided` enum `A`. `@provided` is read on the resolved *declaration*, never on the consuming field, so for the ordinary `field extends @provided decl` shape own and resolving agree. On the chained shape a resolving read reports `B` as provided and emits a reference to a hand-written `B` **the adopter never declared** (the marker was authored on `A`), instead of materializing `B` from its inherited `@values`. Own-only matches authored intent. + + Note this is a *provenance* marker and not a value: the member set it accompanies (`@values`, and its numeric half `@intValueMap`) is a logical property and is still read **resolving**, so a declaration inheriting `@values` from its super materializes correctly. ### Naming Where a port's default-named accessor is own-only (Python `attr()` is own; TS `attr()` resolves — an inversion), the port SHOULD make the **resolving** form the default-named one and the own form explicitly `own*`, so "the obvious call" is the correct one. Any `own*()` call MUST carry a one-line comment stating which sanctioned case it is. @@ -37,4 +44,4 @@ Where a port's default-named accessor is own-only (Python `attr()` is own; TS `a - A concrete field/entity that `extends` an abstract parent now correctly inherits its properties and members through codegen, runtime, serialization-effective, schema, and validation — in all five ports. - A **conformance fixture** (abstract field with `isArray`/`maxLength`/`precision`/`default`/`objectRef`/`storage` + a concrete field that `extends` it, plus an entity-level BaseEntity case) gates the class permanently; it fails on pre-fix code. - The rule is propagated to CLAUDE.md and the agent-context authoring/codegen/audit skills; the `metaobjects-audit` skill flags own-accessor value-reads/effective-iteration in codegen/runtime as a defect. -- Each remaining `own*()` call is either the sanctioned emit-declared-here case (commented) or `@dbColumnType` (commented) — any other is a bug. +- Each remaining `own*()` call is either the sanctioned emit-declared-here case (commented) or one of the two deliberately-own-only attributes, `@dbColumnType` / `@provided` (commented) — any other is a bug.