From 128599ca29c71440c191b881dd3b91dae19f1e12 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sat, 15 Aug 2026 19:08:50 -0400 Subject: [PATCH 1/8] fix(verify): @verifiedBy stops deciding what a test file is The 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, and nothing matched. The failure is worse than a miss because the fail-open only triggers 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 that project is an *IT, so @verifiedBy was unusable and the honest workaround was to stop using the attribute and explain why in a note. Three changes. Only the first patches the guess; the other two are the actual fix. 1. Failsafe's own defaults are built in (*IT, *ITCase, IT* for .java; *IT, *ITCase for .kt). 2. verify.testFiles in metaobjects.config.ts -- globs, added to the built-ins. What counts as a test file is project-specific. This repo cannot be authoritative about a convention it has never seen, and pretending otherwise is what produced the bug. 3. 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 ruling. If the name is there it emits WARN_REQUIREMENT_TEST_UNCLASSIFIED naming the file and pointing at verify.testFiles; ERR_REQUIREMENT_TEST_MISSING is now 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 rather than the regex. A gate that hardcodes another ecosystem's conventions will eventually tell a correct project it is broken, and when a tool cannot classify something the honest default is to say so, not to convict. Verified against the adopter repo that reported it: the *IT name it could not use now resolves with NO config at all, exit 0, because the built-in Failsafe pattern covers it. Gated by cli/test/verified-by-corpus.test.ts (9 cases: the built-in conventions incl. Failsafe, a project-declared convention, that declared patterns ADD rather than replace, the nowhere-name error, and the unclassified warn/clear path). Full cli suite 545 pass / 0 fail; agent-context-conformance golden regenerated for the edited verify fragment. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 31 +++ .../references/requirements.md | 17 ++ docs/features/requirements.md | 18 ++ .../references/requirements.md | 17 ++ .../packages/cli/src/commands/verify.ts | 8 +- .../packages/cli/src/lib/verified-by-scan.ts | 162 ++++++++++++++-- .../cli/test/verified-by-corpus.test.ts | 176 ++++++++++++++++++ .../packages/codegen-ts/src/index.ts | 2 +- .../codegen-ts/src/metaobjects-config.ts | 24 +++ 9 files changed, 436 insertions(+), 19 deletions(-) create mode 100644 server/typescript/packages/cli/test/verified-by-corpus.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cc1a34a7..f9f8bac38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,37 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### 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-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..1502b4552 100644 --- a/docs/features/requirements.md +++ b/docs/features/requirements.md @@ -149,6 +149,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-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/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/verified-by-scan.ts b/server/typescript/packages/cli/src/lib/verified-by-scan.ts index ab8115804..780949d78 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,31 @@ function walk(dir: string, root: string, acc: TestCorpus, depth = 0): void { } } +/** + * 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)) 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 +250,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 +313,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/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/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 Date: Sat, 15 Aug 2026 22:08:38 -0400 Subject: [PATCH 2/8] feat(requirements): a requirement can claim a prompt template @implementedBy is documented as naming "the model nodes realising this requirement" and resolved through the OBJECT resolver only, so naming a template.prompt gave 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 carrying a status. A retired entity leaves a table behind. A retired prompt leaves nothing, which is precisely the invisibility `status: abandoned` exists to fix, so 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" -- object.* or template.* -- and L5 a member of one. Objects still resolve through the loader's own resolver first, so ADR-0042 package-local binding is unchanged and this is not a parallel name scan (#228). Bare refs prefer the referrer's package and an ambiguous bare name binds NOTHING, the same fail-closed rule objects use. Requirements are excluded as claim targets: 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, and that has its own test. FIELDS, VIEWS, VALIDATORS AND IDENTITIES NEEDED NO CHANGE. The same report asked for those too; they already resolved, because resolveMember walks child names generically. Checked before writing anything rather than assumed, and they are now pinned by tests so it stays true -- including a view and a validator nested under a field, which is the deepest grain anyone is likely to claim. 11 new cases in cli/test/requirement-template-refs.test.ts. Full cli suite 556 pass / 0 fail; agent-context conformance goldens regenerated. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 24 +++ .../references/requirements.md | 12 +- docs/features/requirements.md | 15 ++ .../references/requirements.md | 12 +- .../packages/cli/src/lib/requirement-check.ts | 43 ++++- .../test/requirement-template-refs.test.ts | 159 ++++++++++++++++++ 6 files changed, 257 insertions(+), 8 deletions(-) create mode 100644 server/typescript/packages/cli/test/requirement-template-refs.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f9f8bac38..ac489b358 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,30 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### 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 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/docs/features/requirements.md b/docs/features/requirements.md index 1502b4552..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 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/server/typescript/packages/cli/src/lib/requirement-check.ts b/server/typescript/packages/cli/src/lib/requirement-check.ts index f4ae86d74..0734ccb16 100644 --- a/server/typescript/packages/cli/src/lib/requirement-check.ts +++ b/server/typescript/packages/cli/src/lib/requirement-check.ts @@ -148,6 +148,45 @@ 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]; + const bare = candidates.filter((c) => c.name === 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 +235,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 +341,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/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 + }); +}); From 808e2828bb6487e24de9fb580aa6c0207c55ce2e Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sat, 15 Aug 2026 22:59:31 -0400 Subject: [PATCH 3/8] fix(render): derive has accessors in every port Ran down why an adopter's Node templates gate reported 157 ERR_VAR_NOT_ON_PAYLOAD on a prompt estate its JVM gate called clean. Every one of the 157 was `has`-prefixed, and neither gate was lying. WHAT WAS ACTUALLY BROKEN, which is worse than a gate disagreeing A prompt's conditional section -- "include the abilities block only when there ARE abilities" -- is `{{#hasAbilities}}`, a DERIVED boolean over the declared field `abilities`. The JVM has emitted has() onto generated payload records since 7.7.7 and accepts the section in Verify, 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"}]} before "Abilities:" <- content dropped, silently, no error after "Abilities: [Fireball]" 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. That is the byte-identical-rendering promise failing quietly. FIVE PORTS, ONE RULE PayloadAccessors now exists in TS, C#, Python and (extended) Java/Kotlin with the same naming rule and the same presence semantics as the JVM emitter's per-type bodies: string -> non-blank, collection -> non-empty, reference -> non-null, and number/boolean -> NO accessor, because {{#hasCount}} over an int is drift rather than a conditional. Deriving `false` there would let a template that is drift on the JVM render quietly everywhere else. Render derives non-mutatingly (a render must not change the object it was handed), 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 and keeps the JVM's deliberate permissiveness -- acceptance keys off the field existing, not its type -- while still reporting drift inside a has-section body, since the gate is a boolean and does not push scope. THE CORPUS IS THE REAL FIX. fixtures/render-conformance/ had NO fixture using a derived accessor, which is exactly why a divergence in the pillar that promises byte-identical rendering survived. render-derived-has-accessor covers present / absent / blank across scalar, collection, reference and nested scope, and all five ports run it. Two things worth knowing for later. The JVM's stale snapshot was regenerated (my own pre-fix run had created it, capturing the buggy output -- an auto-creating snapshot will happily pin a bug). And the fixture keeps every tag inline: an earlier draft ended on a standalone {{/section}} line and Python emitted one extra trailing newline where TS and C# did not, which is a SEPARATE standalone-line divergence this fixture should not be adjudicating. Verified: TS render 323, cli 559; C# 291; Python 1571; JVM 292 -- all green. On the adopter's estate the Node gate goes 157 -> 0 and now agrees with the JVM gate. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 38 +++++ .../render-derived-has-accessor/expected.txt | 5 + .../render-derived-has-accessor/meta.json | 1 + .../render-derived-has-accessor/payload.json | 10 ++ .../template.mustache | 5 + .../MetaObjects.Render/PayloadAccessors.cs | 127 +++++++++++++++++ server/csharp/MetaObjects.Render/Renderer.cs | 5 +- server/csharp/MetaObjects.Render/Verify.cs | 11 +- .../metaobjects/render/PayloadAccessors.java | 63 +++++++++ .../java/com/metaobjects/render/Renderer.java | 6 +- .../snapshots/render-derived-has-accessor.txt | 5 + .../metaobjects/render/payload_accessors.py | 133 ++++++++++++++++++ .../python/src/metaobjects/render/renderer.py | 6 +- .../python/src/metaobjects/render/verify.py | 12 +- .../packages/render/src/payload-accessors.ts | 91 ++++++++++++ .../typescript/packages/render/src/render.ts | 6 +- .../typescript/packages/render/src/verify.ts | 32 ++++- .../render/test/payload-accessors.test.ts | 130 +++++++++++++++++ 18 files changed, 679 insertions(+), 7 deletions(-) create mode 100644 fixtures/render-conformance/render-derived-has-accessor/expected.txt create mode 100644 fixtures/render-conformance/render-derived-has-accessor/meta.json create mode 100644 fixtures/render-conformance/render-derived-has-accessor/payload.json create mode 100644 fixtures/render-conformance/render-derived-has-accessor/template.mustache create mode 100644 server/csharp/MetaObjects.Render/PayloadAccessors.cs create mode 100644 server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt create mode 100644 server/python/src/metaobjects/render/payload_accessors.py create mode 100644 server/typescript/packages/render/src/payload-accessors.ts create mode 100644 server/typescript/packages/render/test/payload-accessors.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ac489b358..f6aeca575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,44 @@ 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 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..5743e284f --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/expected.txt @@ -0,0 +1,5 @@ +title:Party +bio: (none) +sponsor: Guild +companions: (none) +abilities: Fireball[fire aoe ] Mend[untagged] \ 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..4a2cdb6be --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/payload.json @@ -0,0 +1,10 @@ +{ + "title": "Party", + "bio": " ", + "abilities": [ + { "name": "Fireball", "tags": ["fire", "aoe"] }, + { "name": "Mend", "tags": [] } + ], + "companions": [], + "sponsor": { "name": "Guild" } +} 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..8697eddfd --- /dev/null +++ b/fixtures/render-conformance/render-derived-has-accessor/template.mustache @@ -0,0 +1,5 @@ +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}} \ No newline at end of file diff --git a/server/csharp/MetaObjects.Render/PayloadAccessors.cs b/server/csharp/MetaObjects.Render/PayloadAccessors.cs new file mode 100644 index 000000000..1da08daaf --- /dev/null +++ b/server/csharp/MetaObjects.Render/PayloadAccessors.cs @@ -0,0 +1,127 @@ +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; + 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..55f1ea5d0 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,67 @@ 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.Collection c) return !c.isEmpty(); + if (value instanceof Object[] a) return a.length > 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; + } + if (payload instanceof java.util.List list) { + java.util.List out = new java.util.ArrayList<>(list.size()); + for (Object item : list) out.add(withDerivedAccessors(item, 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..5743e284f --- /dev/null +++ b/server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt @@ -0,0 +1,5 @@ +title:Party +bio: (none) +sponsor: Guild +companions: (none) +abilities: Fireball[fire aoe ] Mend[untagged] \ No newline at end of file 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..7aaa2bd9b --- /dev/null +++ b/server/python/src/metaobjects/render/payload_accessors.py @@ -0,0 +1,133 @@ +"""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 + +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, (bool, int, float, complex)): + 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..97e066668 100644 --- a/server/python/src/metaobjects/render/verify.py +++ b/server/python/src/metaobjects/render/verify.py @@ -24,6 +24,8 @@ from typing import Protocol #: A ``{{var}}`` references a field the (contextual) payload does not declare. +from .payload_accessors import is_boolean_accessor + ERR_VAR_NOT_ON_PAYLOAD = "ERR_VAR_NOT_ON_PAYLOAD" #: A ``{{> ref}}`` partial does not resolve in the provider. ERR_PARTIAL_UNRESOLVED = "ERR_PARTIAL_UNRESOLVED" @@ -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/render/src/payload-accessors.ts b/server/typescript/packages/render/src/payload-accessors.ts new file mode 100644 index 000000000..99e54b217 --- /dev/null +++ b/server/typescript/packages/render/src/payload-accessors.ts @@ -0,0 +1,91 @@ +// 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; + + 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..ba61c64b0 --- /dev/null +++ b/server/typescript/packages/render/test/payload-accessors.test.ts @@ -0,0 +1,130 @@ +// 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); + }); +}); From 10e4c0086890c57db21bcd0de9d25fbdac85e1a3 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 16 Aug 2026 13:34:05 -0400 Subject: [PATCH 4/8] fix(docs): regenerate the metamodel docs fixture ec6804e99 left stale `main` has been red on the ts-unit gate since ec6804e99. That commit reworded the `@verifiedBy` attribute DESCRIPTION across all five ports -- and an attr description is registry content, so it feeds the byte-gated metamodel docs. The fixture was never regenerated, so metamodel-docs-conformance has been failing on `types/requirement.md` ever since. Nobody saw it because the lane that runs it is not on the PR path, and a docs-shaped push lets affected-ports skip the TS lane entirely -- a skipped lane reports green. Found by running the FULL local CI before cutting 0.23.1, which is the same reason the 0.23.0 cut ran it: that one found main red on four of five ports for the same structural reason. The regenerated text is verified, not just re-baselined: the diff is exactly the reworded @verifiedBy description ec6804e99 deliberately shipped ("OPTIONAL -- omit unless you have opened the test and read what it asserts...") and nothing else. Two rows, one per requirement subtype. Local CI: 17 of 18 gates were already green; this closes the 18th. Co-Authored-By: Claude Opus 5 (1M context) --- fixtures/metamodel-docs/expected/types/requirement.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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** From 6b895469667db0f3dd2dd6e2119e9e61aafbeeb0 Mon Sep 17 00:00:00 2001 From: Douglas Mealing Date: Tue, 11 Aug 2026 20:03:26 -0400 Subject: [PATCH 5/8] =?UTF-8?q?fix(codegen):=20@provided=20is=20declaratio?= =?UTF-8?q?n-layer=20=E2=80=94=20read=20it=20own-only=20in=20TS/C#/Python?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @provided marks a shared enum declaration as supplied by hand-written / third-party code: the port emits nothing and references the existing type (ADR-0026). TS, C# and Python read it RESOLVING; Java and Kotlin read it own-only and documented that as deliberate. One of them had to be wrong. The JVM side is right. @provided is a provenance fact about the declaration ITSELF -- like `abstract` -- not a property of the values it carries, so it must not flow down an extends chain. All five ports already read it on the resolved DECLARATION and never on the consuming field, so for the ordinary `field extends @provided decl` shape own and resolving agree; the divergence is reachable only through a CHAINED declaration -- a root-level abstract enum `B extends` a root-level abstract `@provided A`. Verified against the real loader: that model loads clean (zero errors), B's own @provided is absent while its resolving read is true. So the resolving ports classify B as provided and emit 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. Python's docstring justified resolving with "a concrete enum extending an abstract @provided enum inherits the flag, so an own-only read would misclassify it" -- wrong about its own call graph, since is_provided() is only ever passed the decl. C#'s comment just cited TS. Neither was a reasoned position. Blast radius is nil on existing gated output: every currently-pinned model shape yields the same answer under both reads, which is exactly why this survived. ADR-0039 amended: its "@dbColumnType is the *only* attribute deliberately read own-only" line was false as written no matter which way this ruled, since the JVM own-reads already existed. @provided is now chartered as the second, with the chained-decl rationale and an explicit note that the member set it accompanies (@values, and its numeric half @intValueMap) stays RESOLVING. No conformance fixture yet -- see the follow-up below. Verified: TS codegen-ts 1074/0 + workspace typecheck clean; Python 1681/0; C# 1558/0 (1 pre-existing skip); Java codegen-spring Fr019 conformance 3/0. FOLLOW-UP (deliberately not in this commit): adding a chained-decl case to fixtures/codegen-conformance/shared-provided-enum -- the corpus all five ports gate -- surfaced a SECOND, deeper divergence that needs a design ruling of its own. Kotlin deliberately names a chained abstract enum after the TOP-MOST root (KotlinTypeMapper.enumTypeName, "a chain of abstract enums still collapses onto one type"), so Kotlin holds that Money IS Currency while every other port holds that Money is its own type. On that input Kotlin materializes a local Currency.kt while ALSO referencing the external com.acme.ext.Currency -- broken under either model. Resolving it means either aligning Kotlin's collapse on the immediate super, or rejecting chained abstract enum declarations in the loader (post-#246 such an alias can carry neither its own @values nor its own @intValueMap, so it adds nothing). Fixture withheld until that is decided rather than pinning one port's accidental behavior. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit a7a7102b57a9b43b6ad6173d2c48929bdabe177d) --- .../Generators/Fr019SharedEnum.cs | 9 ++++++-- .../codegen/generators/fr019_shared_enum.py | 21 +++++++++++++------ .../packages/codegen-ts/src/enum-shared.ts | 8 ++++++- .../ADR-0039-own-accessor-discipline.md | 13 +++++++++--- 4 files changed, 39 insertions(+), 12 deletions(-) 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/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/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/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. From 6c0660351cd97a5873b895bfa25ec0386ec54881 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 16 Aug 2026 14:24:08 -0400 Subject: [PATCH 6/8] test(codegen): pin the @provided own-only read the cherry-pick had no test for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cherry-picked fix (6b8954696, from the draft int-backed-enum branch) changed codegen behaviour in three ports and shipped with NO test — its branch's enum tests cover @intValueMap, a different feature. A behaviour change riding into a release untested is how the divergence it fixes got there in the first place. Two cases, on the existing FR-019 runGen harness: - a CHAINED declaration materializes. Root abstract `@provided Base`, root abstract `Derived extends Base`, entity field extends Derived. Under the old resolving read Derived is classified provided and the port emits an import of a hand-written `Derived` the adopter never declared; it must emit the type instead. - the marked declaration itself is STILL provided, so the fix cannot be "read own everywhere" overreach. PROVEN NON-VACUOUS: reverting just the TS half (ownAttrs -> attr) turns the first case red and leaves the other six green, then restoring it turns it back. A test that passes with and without the change it guards is worse than no test. Co-Authored-By: Claude Opus 5 (1M context) --- .../templates/enum-shared-provided.test.ts | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/server/typescript/packages/codegen-ts/test/templates/enum-shared-provided.test.ts b/server/typescript/packages/codegen-ts/test/templates/enum-shared-provided.test.ts index 1cb576af3..63185d7a6 100644 --- a/server/typescript/packages/codegen-ts/test/templates/enum-shared-provided.test.ts +++ b/server/typescript/packages/codegen-ts/test/templates/enum-shared-provided.test.ts @@ -224,3 +224,62 @@ describe("FR-019 inline enum unchanged", () => { 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(); + }); +}); From 4e757c87a2422e931899335aaad39b156677fe3e Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 16 Aug 2026 14:48:25 -0400 Subject: [PATCH 7/8] fix(render,verify): close six divergences review found in the has-accessor change Code review over the branch found that the change meant to END a cross-port divergence had introduced four of its own, plus two verify defects. Fixing them is the point; the lesson is that "one shared rule" written four times in four languages is still four implementations. A LIVE REGRESSION, worst first. TS rebuilt ANY non-array object from Object.entries(), so anything carrying its own prototype was flattened: a Date rendered as "[object Object]" and a class instance lost its getters. Confirmed by running it. Now only PLAIN objects are rebuilt (prototype is Object.prototype or null), which is also what the other ports do. THE FOUR PORTS DID NOT AGREE: - C# an IDictionary is IEnumerable, so it matched the collection arm and an empty nested object reported ABSENT where every other port says present. Dictionary is now matched first. - Java recursion covered Map/List only, so Set elements and array elements got no accessors -- contradicting the method's own javadoc; and an empty primitive array reported PRESENT because Object[] does not match int[]. Now every Collection recurses, arrays recurse by reflection, and length is read with Array.getLength. - Py the numeric skip listed int/float/complex, missing Decimal and Fraction, so Python injected hasPrice where Java and C# inject nothing. Now numbers.Number. TWO VERIFY DEFECTS: - The unclassified-file fail-open downgraded a broken @verifiedBy claim to a warning if the name appeared ANYWHERE in production source -- including the mountCrudRoutes case this file's own header cites as why the comment-only check exists. The downgrade now requires the file to look like a test by LOCATION or by CONTENT; production source with a matching name satisfies neither and stays a hard error. - The bare-name fallback for non-object claims reached into any package where the name was unique. resolveObjectRef's own bare fallback is root-level only; this now matches it. GATED, not just fixed. The shared conformance fixture gains the empty-nested-object case that catches the C# arm, so all five ports pin it rather than three agreeing by luck. TS gains four regression tests (Date renders as a Date, class getters survive, a Date-valued field still derives, nested plain objects still derive). Verified after: TS render 327, cli 20 targeted; C# 291; Python 257; JVM render green (snapshot regenerated for the extended fixture). Co-Authored-By: Claude Opus 5 (1M context) --- .../render-derived-has-accessor/expected.txt | 3 +- .../render-derived-has-accessor/payload.json | 20 +++++++++--- .../template.mustache | 3 +- .../MetaObjects.Render/PayloadAccessors.cs | 3 ++ .../metaobjects/render/PayloadAccessors.java | 19 ++++++++--- .../snapshots/render-derived-has-accessor.txt | 3 +- .../metaobjects/render/payload_accessors.py | 3 +- .../packages/cli/src/lib/requirement-check.ts | 4 ++- .../packages/cli/src/lib/verified-by-scan.ts | 16 +++++++++- .../packages/render/src/payload-accessors.ts | 6 ++++ .../render/test/payload-accessors.test.ts | 32 +++++++++++++++++++ 11 files changed, 98 insertions(+), 14 deletions(-) diff --git a/fixtures/render-conformance/render-derived-has-accessor/expected.txt b/fixtures/render-conformance/render-derived-has-accessor/expected.txt index 5743e284f..d21771c1f 100644 --- a/fixtures/render-conformance/render-derived-has-accessor/expected.txt +++ b/fixtures/render-conformance/render-derived-has-accessor/expected.txt @@ -2,4 +2,5 @@ title:Party bio: (none) sponsor: Guild companions: (none) -abilities: Fireball[fire aoe ] Mend[untagged] \ No newline at end of file +abilities: Fireball[fire aoe ] Mend[untagged] +details: present \ No newline at end of file diff --git a/fixtures/render-conformance/render-derived-has-accessor/payload.json b/fixtures/render-conformance/render-derived-has-accessor/payload.json index 4a2cdb6be..99d4640bc 100644 --- a/fixtures/render-conformance/render-derived-has-accessor/payload.json +++ b/fixtures/render-conformance/render-derived-has-accessor/payload.json @@ -2,9 +2,21 @@ "title": "Party", "bio": " ", "abilities": [ - { "name": "Fireball", "tags": ["fire", "aoe"] }, - { "name": "Mend", "tags": [] } + { + "name": "Fireball", + "tags": [ + "fire", + "aoe" + ] + }, + { + "name": "Mend", + "tags": [] + } ], "companions": [], - "sponsor": { "name": "Guild" } -} + "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 index 8697eddfd..30c843e46 100644 --- a/fixtures/render-conformance/render-derived-has-accessor/template.mustache +++ b/fixtures/render-conformance/render-derived-has-accessor/template.mustache @@ -2,4 +2,5 @@ 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}} \ No newline at end of file +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.Render/PayloadAccessors.cs b/server/csharp/MetaObjects.Render/PayloadAccessors.cs index 1da08daaf..b1bed1132 100644 --- a/server/csharp/MetaObjects.Render/PayloadAccessors.cs +++ b/server/csharp/MetaObjects.Render/PayloadAccessors.cs @@ -76,6 +76,9 @@ public static bool IsBooleanAccessor(List> stack, st 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; 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 55f1ea5d0..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 @@ -61,8 +61,11 @@ 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(); - if (value instanceof Object[] a) return a.length > 0; + // 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; } @@ -101,9 +104,17 @@ private static Object withDerivedAccessors(Object payload, int depth) { } return out; } - if (payload instanceof java.util.List list) { - java.util.List out = new java.util.ArrayList<>(list.size()); - for (Object item : list) out.add(withDerivedAccessors(item, depth + 1)); + // 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/test/resources/snapshots/render-derived-has-accessor.txt b/server/java/render/src/test/resources/snapshots/render-derived-has-accessor.txt index 5743e284f..d21771c1f 100644 --- 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 @@ -2,4 +2,5 @@ title:Party bio: (none) sponsor: Guild companions: (none) -abilities: Fireball[fire aoe ] Mend[untagged] \ No newline at end of file +abilities: Fireball[fire aoe ] Mend[untagged] +details: present \ No newline at end of file diff --git a/server/python/src/metaobjects/render/payload_accessors.py b/server/python/src/metaobjects/render/payload_accessors.py index 7aaa2bd9b..4189c98c4 100644 --- a/server/python/src/metaobjects/render/payload_accessors.py +++ b/server/python/src/metaobjects/render/payload_accessors.py @@ -16,6 +16,7 @@ from __future__ import annotations +import numbers from collections.abc import Mapping, Sequence from typing import Any @@ -71,7 +72,7 @@ def accessor_value(value: Any) -> bool | None: 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, (bool, int, float, complex)): + if isinstance(value, numbers.Number): return None if isinstance(value, Mapping): return True diff --git a/server/typescript/packages/cli/src/lib/requirement-check.ts b/server/typescript/packages/cli/src/lib/requirement-check.ts index 0734ccb16..13f27e750 100644 --- a/server/typescript/packages/cli/src/lib/requirement-check.ts +++ b/server/typescript/packages/cli/src/lib/requirement-check.ts @@ -183,7 +183,9 @@ function resolveClaimTarget(root: MetaData, owner: string, referrerPkg: string): // 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]; - const bare = candidates.filter((c) => c.name === owner); + // 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; } 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 780949d78..9f8b1c852 100644 --- a/server/typescript/packages/cli/src/lib/verified-by-scan.ts +++ b/server/typescript/packages/cli/src/lib/verified-by-scan.ts @@ -183,6 +183,20 @@ function walk( * per run. Returns the first unclassified source file containing the name, which is * enough to tell the author which pattern they are missing. */ +/** 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)); +} + function findOutsideCorpus(name: string, root: string, files: string[]): string | undefined { const rx = wordRx(name); for (const rel of files) { @@ -192,7 +206,7 @@ function findOutsideCorpus(name: string, root: string, files: string[]): string 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)) return rel; + if (rx.test(line) && !isCommentLine(line, rel) && looksLikeTest(rel, lines)) return rel; } } catch { /* unreadable file is not a finding */ diff --git a/server/typescript/packages/render/src/payload-accessors.ts b/server/typescript/packages/render/src/payload-accessors.ts index 99e54b217..ad5122658 100644 --- a/server/typescript/packages/render/src/payload-accessors.ts +++ b/server/typescript/packages/render/src/payload-accessors.ts @@ -77,6 +77,12 @@ export function withDerivedAccessors(payload: T, depth = 0): T { 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 = {}; diff --git a/server/typescript/packages/render/test/payload-accessors.test.ts b/server/typescript/packages/render/test/payload-accessors.test.ts index ba61c64b0..eac86eacd 100644 --- a/server/typescript/packages/render/test/payload-accessors.test.ts +++ b/server/typescript/packages/render/test/payload-accessors.test.ts @@ -128,3 +128,35 @@ describe("verify — accepts exactly what render resolves", () => { 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"); + }); +}); From 8c3a8ee27421af89e2d599338dd94daf1c77a144 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sun, 16 Aug 2026 14:52:18 -0400 Subject: [PATCH 8/8] style: restore two doc-comment bindings the new code displaced Simplifier pass over the branch. Both are comment/import placement, zero behavior change, no test touched: - verified-by-scan.ts: findOutsideCorpus's JSDoc had landed above the TESTISH_* constants, with an unrelated doc-comment sandwiched between it and the function it describes. - render/verify.py: the new payload_accessors import was inserted between ERR_VAR_NOT_ON_PAYLOAD's `#:` doc-comment and the constant, breaking the binding Sphinx reads. The rest of the branch was reviewed and deliberately left alone: the four ports already express the same presence rules, and the two-pass recurse-then-derive structure is duplicated on purpose rather than being cruft -- collapsing it in TS alone would make one port diverge for no gain in clarity. One genuine cross-port asymmetry was found and NOT changed: Python's first recursion pass copies non-string dict keys where C#/Java filter them in both passes. Changing it would alter output for a shape the conformance corpus does not cover, which is a corpus-fixture decision rather than a cleanup-pass one. cli 559 tests pass; python render 257 passed. --- server/python/src/metaobjects/render/verify.py | 2 +- .../packages/cli/src/lib/verified-by-scan.ts | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/python/src/metaobjects/render/verify.py b/server/python/src/metaobjects/render/verify.py index 97e066668..3889f469c 100644 --- a/server/python/src/metaobjects/render/verify.py +++ b/server/python/src/metaobjects/render/verify.py @@ -23,9 +23,9 @@ from dataclasses import dataclass from typing import Protocol -#: A ``{{var}}`` references a field the (contextual) payload does not declare. 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. ERR_PARTIAL_UNRESOLVED = "ERR_PARTIAL_UNRESOLVED" 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 9f8b1c852..9831faa08 100644 --- a/server/typescript/packages/cli/src/lib/verified-by-scan.ts +++ b/server/typescript/packages/cli/src/lib/verified-by-scan.ts @@ -176,13 +176,6 @@ function walk( } } -/** - * 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. - */ /** 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 @@ -197,6 +190,13 @@ 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) {