feat(model-apps): SDK uptake, silent-failure fixes, genpage input contract, 2.5.0

[Akshay Maloo (akshaymaloo)]

Takes up the current maker SDK and fixes the two silent-failure classes it revealed, resolves the genpage sitemap-placement vs pageInput policy conflict, closes the actionable #456 follow-ups, and releases 2.5.0.

Closes #457. Addresses #456 (items 2–5; item 1 remains, with reasoning on the issue). Supersedes #459, folded in here so the whole language-code surface is reviewed and live-tested as one unit.

The SDK uptake found two silent failures, not stale tests

Re-vendored from the SDK's master (572 KB → 641 KB; the delta is 92 files / +8085/−656). The SDK moved both pushArtifact and publishArtifact from throwing to reporting by value. We were checking neither.

1. The 412 guard had been silently disarmed. PushResult.success was renamed to saved — the SDK's own type comment says the rename exists to "force every existing call site to be looked at once". requireSuccessfulPush checked success === false, which against the new shape reads undefined === false and simply stops firing. A concurrent Maker edit would be overwritten with no error and no log.

2. Every publish failure was silent — peer review caught this one, and it is worse. All nine publishArtifact call sites discarded the result. Measured: a /PublishXml 500 returns { publish: { kind: 'failed' } } on this bundle and threw on the previous one, and a build where every publish failed still reported ok: true with zero error events. It is not gated on --publish (reconciling an existing form or view publishes on every --apply), the trigger is routine (429 / 503 / customization-lock), and because those no longer throw, the transient-retry loop had become unreachable for them.

Also drains PushResult.warnings at all eleven push sites — that field exists so an app whose components could not all be pinned, or whose system-administrator role assignment failed (which yields an app nobody can open), is not read as a clean success. It is also the concrete saved vs shipped gap: an app create publishes inside the SDK, so a failed publish arrives as saved: true with no top-level error — a shape the push check alone waves straight through.

And closes the gap that let this land. publishArtifact was in the method-presence guard but had no behavioural contract test, so a throw→return change was invisible to CI. This PR adds one, plus the integration test that the engine is actually wired to read the result — which no unit test can stand in for, since the original bug was a wiring break.

Language code

• languageCode in an App Spec, and --language-code on both CLIs, with a conflicting pair of spellings now rejected rather than silently resolved to the kebab form.
• The flag now warns when a stage selector skips data-model, where it is a no-op.
• A hand-pinned languageCode survives a download (was fix(model-apps): keep a hand-pinned languageCode across a download (#456) #459). Download still never reads the LCID from Dataverse — copying the source org's language into a portable spec is how it starts failing in an org that lacks that language — but a value the author wrote is carried over from the previous app-spec.json. Losing it was quiet: the next build resolved the org default, so new columns got one language while the pinned ones kept another.

Genpage: sitemap-placement vs pageInput

Two rules contradicted each other with no way for an author to satisfy both. Every page must be sitemap-placed — the sitemap is download's only membership oracle, so a page reached only by navigatesTo is invisible to download and gets re-created as a duplicate. But that means a detail page is reachable from the nav with no input, and nothing made the author account for it, so the generated page read undefined context on a path a user reaches by clicking.

Resolved by keeping the membership invariant and adding directEntry (selector | emptyState), which page-plan feeds to the generator so the .tsx actually implements that path. Plus the input-contract trace: every pageInput.data key must be produced by an incoming navigatesTo[].data edge. Allowing navigation-only pages was considered and rejected — the duplicate-page bug is worse than an extra nav entry.

Verification

1540 plugin tests + 159 eval tests pass; 7 validators green. Every guard is red-green verified — restoring success === false turns two red; making the publish reporter a no-op turns four red; unwiring all nine publish sites turns the integration test red; dropping the download restore turns the pin test red.

Live, against a real environment, all artifacts torn down (0 failures each):

run	result
full build + --verify	17/17 created, verify PASS 11/11 — data-model incl. global choice, status reason, alternate key, relationship; views + default-view enrichment; charts; forms; app-shell
idempotent rebuild	exercises the new etag/concurrency path by re-pushing view, form and app against existing rows
build with --publish	8/8, verify 6/6, publish phase clean
teardown	11 deleted incl. deleteAppCascade

Corrections recorded rather than quietly fixed

• I twice reported this uptake's blocker wrongly — first "three contract tests", then "one" — both measured against a local SDK checkout 154 commits behind master. The real answer was four.
• My comment claiming "a 412 is the only failure pushArtifact signals by return value" was false; ARTIFACT_ALREADY_EXISTS is a second, and the guard was reporting it with the opposite remedy.
• parseManifest's comment claimed it rejected corrupt directEntry values when it only checked object-ness.
• The first red-green of the publish-wiring test looked ineffective. It was the mutation that was incomplete — the mapLimit site ends in )) and slipped the regex, and it was the very site the test exercises. Re-verified with a paren-matching unwire.

[Copilot]

Pull request overview

Updates the model-apps plugin to a newer vendored Maker SDK, hardens the build pipeline against SDK by-value failure reporting (push/publish), and resolves the genpage sitemap-membership vs pageInput contract conflict by introducing directEntry. This culminates in a 2.5.0 release bump with corresponding documentation and changelog updates.

Changes:

• Uptake of the newer Maker SDK surface by updating push/publish result handling and refreshing affected test mocks/contracts.
• Adds and enforces the directEntry + pageInput input contract for genpages (including manifest/download round-trip support).
• Introduces stricter CLI flag alias handling (--language-code vs --languageCode) and bumps plugin version/docs to 2.5.0.

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
plugins/model-apps/scripts/tests/vendor-sdk-smoke.test.js	Updates SDK contract tests (PushResult rename handling; deleteAppCascade now asserts retained[]).
plugins/model-apps/scripts/tests/sdk-build.test.js	Updates SDK build mock push result shape (needs publish mock alignment per review).
plugins/model-apps/scripts/tests/sdk-build-pages-order.test.js	Updates page build harness mocks to new push/publish result shapes.
plugins/model-apps/scripts/tests/sdk-build-pages-migrate.test.js	Updates page migration harness mock push result shape (needs publish mock alignment per review).
plugins/model-apps/scripts/tests/sdk-build-pages-deploy.test.js	Updates page deploy harness mocks to new push/publish result shapes.
plugins/model-apps/scripts/tests/helpers/mock-sdk.js	Refreshes shared mock SDK to return publish-by-value results.
plugins/model-apps/scripts/tests/hardening2-real-bundle.test.js	Pins push conflict contract across success/saved rename.
plugins/model-apps/scripts/tests/entity-provision.test.js	Adds tests for renamed push contract, publish-by-value reporting, and warnings propagation.
plugins/model-apps/scripts/tests/default-view-createdon.test.js	Makes the view fixture realistic by adding required @odata.etag.
plugins/model-apps/scripts/tests/build-model-app.test.js	Adds tests for rejecting conflicting aliased language flags.
plugins/model-apps/scripts/tests/app-spec-keys.test.js	Adds validation tests for directEntry requirements and page input traceability.
plugins/model-apps/scripts/provision-entities.js	Switches language flag parsing to readAliasedFlag to reject conflicting spellings.
plugins/model-apps/scripts/lib/sdk-build.js	Starts consuming publish results via reportPartialPush and threads warning sink into push guard.
plugins/model-apps/scripts/lib/page-plan.js	Includes directEntry behavior in generated page plan output.
plugins/model-apps/scripts/lib/page-manifest.js	Persists and parses directEntry in the durable page manifest.
plugins/model-apps/scripts/lib/hydrate-spec.js	Hydrates directEntry from manifest/spec into the in-memory spec shape.
plugins/model-apps/scripts/lib/entity-provision.js	Hardens push guard to handle saved vs success, adds reportPartialPush, distinguishes already-exists vs 412.
plugins/model-apps/scripts/lib/dataverse-auth.js	Adds readAliasedFlag helper to reject conflicting alias pairs.
plugins/model-apps/scripts/lib/app-spec.js	Adds directEntry enum + validation rules tying pageInput keys to incoming navigation edges.
plugins/model-apps/scripts/build-model-app.js	Uses readAliasedFlag for language-code parsing and warns when LCID is no-op for selected phases.
plugins/model-apps/references/app-spec-schema.md	Documents the new directEntry + pageInput input contract and rationale.
plugins/model-apps/CHANGELOG.md	Updates release notes for 2.5.0, including SDK behavioral changes and contract fixes.
plugins/model-apps/AGENTS.md	Documents the membership invariant corollary and the new directEntry contract.
plugins/model-apps/.plugin/plugin.json	Bumps plugin version to 2.5.0.
plugins/model-apps/.claude-plugin/plugin.json	Mirrors plugin version bump to 2.5.0 for legacy manifest compatibility.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Akshay Maloo (akshaymaloo)]

Peer review found the half I missed — and it was the worse half

An adversarial review of this branch caught something I had not: I migrated the push half of the SDK's throw-to-return refactor and missed the publish half.

All nine publishArtifact call sites in sdk-build.js were bare await provision.publishArtifact(...) with the result discarded. Measured against both bundles:

NEW bundle: publishArtifact RETURNED  { publish: { kind: 'failed', error: HTTP 500 ... } }
OLD bundle: publishArtifact THREW     HTTP 500 from /PublishXml

End to end, a build in which every publish failed still reported ok: true, zero error events, zero warnings.

Three things make it worse than the push bug I did catch:

• Not gated on --publish — reconciling an existing form or view publishes on every --apply.
• Routine trigger — 429 / 503 / customization-lock are the everyday PublishXml failures in a real tenant.
• It disabled existing machinery. Those codes are exactly what isTransientHalt enumerates. On the old bundle they threw and hit the retry loop; here they were swallowed, so the transient-retry path had become unreachable for publish failures.

Fixed with reportPartialPush, wired into all nine sites. It never halts — the write committed — but silence turns a partial success into a reported clean one.

Also from the review

PushResult.warnings was dropped at all eleven push sites. That field exists so an app whose components could not all be pinned, or whose system-administrator role assignment failed — which yields an app nobody can open — is not read as a clean build. This is also the concrete saved vs shipped gap I had asked about: an app create publishes inside the SDK, so a failed publish arrives as saved: true with no top-level error — a shape the push guard alone waves straight through. Now drained.

My own comment was false. I wrote "a 412 is the only failure pushArtifact signals by return value". ARTIFACT_ALREADY_EXISTS is a second one, and the guard was reporting it as a version conflict — telling the operator to re-download when nothing had changed under them. Now distinguished: adopt the existing row, versus re-download.

Six test mocks still returned the old { success: true } / void-publish shapes, so the entire sdk-build integration surface was exercising the legacy branch of the new guard and was blind to the shape it actually ships against. That is why my red-green only turned two tests red — those two were the new unit tests. Migrated.

Where the review corrected me

• I called the push-guard migration "the highest-stakes change on the branch". It was the half I caught; the unguarded publish half was higher-stakes.
• I said the live run "covered the artifact surface". It covered the happy path — 17/17 created and verify 11/11 means every publish succeeded, so the run never induced the failure mode at all.
• My Array.isArray(phases) guard can never be false (stagePhasesOrResolve returns an array or throws). Left in, but I added the phases.length check it actually needed — an empty selector rendered the warning as …for the selected phases ().

Verification

1533 plugin tests + 159 eval tests pass; 10/10 CI green. Both guards red-green verified — restoring success === false turns two red; making the publish reporter a no-op turns four red.

CI also caught one thing I missed procedurally: the 2-orders-multipage eval fixture is itself an instance of the genpage conflict this PR resolves, and now fails validation correctly. I had looked for an eval runner under plugins/model-apps/ and concluded there was none — the harness lives at the repo root under evals/model-apps/, in a separate job. Both suites have to be run; the commit message records the commands.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

plugins/model-apps/scripts/lib/app-spec.js:860

• pageInput is only validated as an object, but the new input-contract logic treats pageInput.data as a plain object and calls Object.keys(...) on it. If pageInput.data is missing or not an object/array (e.g., string/number), validation can produce misleading errors (or treat string indices as keys) instead of a clear schema error. Add an explicit validation that pageInput.data is a non-null, non-array object whenever pageInput is present (or at least before computing inputKeys).

    for (const p of spec.pages || []) {
      const key = p.key || p.name;
      const inputKeys = Object.keys((p.pageInput && p.pageInput.data) || {});
      if (!inputKeys.length) continue;

plugins/model-apps/scripts/lib/page-manifest.js:154

• parseManifest validates that pageInput is an object, but it doesn't validate the pageInput.data sub-shape even though the rest of the codebase treats it as { data: object } (and validates navigatesTo[].data similarly). A corrupted/hand-edited manifest could round-trip a non-object pageInput.data and then fail later in spec validation/build. Consider validating pageInput.data is a non-null, non-array object when present (fail-closed here, like navigatesTo[].data).

    if (p.pageInput !== undefined) {
      if (!p.pageInput || typeof p.pageInput !== 'object' || Array.isArray(p.pageInput)) return null;
    }

    // directEntry — same shape rule. Validated here too so a corrupt manifest is rejected whole

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

plugins/model-apps/scripts/lib/page-plan.js:266

• buildPagePlan prints a “Direct entry (no input)” plan whenever pageInput is present, but App Spec validation only requires directEntry when pageInput.data declares at least one key. For pages with pageInput: { data: {} } (or other empty/non-object shapes), this output is misleading and defaults the behavior to emptyState without the author opting in. Consider gating this block on Object.keys(p.pageInput?.data ?? {}).length > 0 (or p.directEntry being present) so the plan output matches the input-contract rules and doesn’t invent a default behavior.

    if (p.pageInput !== undefined) {
      out.push('- **Page Input:** reads `pageInput` (caller-supplied context)');
      // The page is sitemap-placed, so it is ALSO reachable straight from the app navigation with
      // no input at all. The generator has to be told what that renders, or it writes a page that
      // reads `undefined` context on a path a user can reach by clicking the nav entry.

plugins/model-apps/scripts/lib/app-spec.js:860

• validateAppSpec uses Object.keys((p.pageInput && p.pageInput.data) || {}) to drive the new directEntry/input-trace rules, but there’s no validation that pageInput.data itself is a plain object. Non-object values (e.g. null, arrays, numbers, strings) can make inputKeys empty (silently skipping the contract) or produce unexpected keys. Add an explicit check that pageInput.data is an object (and not null/array) whenever pageInput is present, so malformed specs fail fast with a clear error.

      const key = p.key || p.name;
      const inputKeys = Object.keys((p.pageInput && p.pageInput.data) || {});
      if (!inputKeys.length) continue;

plugins/model-apps/scripts/lib/sdk-build.js:1080

• The new publish-result warnings use labels like form ${formId} / view ${viewId}. These GUID-only identifiers make the warning hard to act on (especially in large builds) even though def.name is available in this scope. Consider using a human-friendly label (e.g. form ${def.name} (${formId})) so reportPartialPush warnings clearly identify the artifact.

    requireSuccessfulPush(await provision.pushArtifact('form', formId), `form ${def.name}`, opts.warn);
    reportPartialPush(await provision.publishArtifact('form', formId), `form ${formId}`, opts.warn);
    await provision.addSolutionComponent({ componentId: formId, componentType: COMPONENT_TYPE.form, solutionUniqueName: sol.uniqueName });

[Copilot]

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

Previously missed (2) — in code that hasn't changed since the last review.

plugins/model-apps/scripts/lib/page-plan.js:266

• directEntry.note is appended into the plan without passing through mdText, even though this file otherwise hardens all spec-sourced strings against markdown structure injection (newlines/headings/tables). Since the plan is executed as instructions by a write-capable worker, de.note should be sanitized the same way (and ideally only rendered when directEntry is actually present/valid, rather than defaulting missing/unknown behavior to the emptyState text).

    if (p.pageInput !== undefined) {
      out.push('- **Page Input:** reads `pageInput` (caller-supplied context)');
      // The page is sitemap-placed, so it is ALSO reachable straight from the app navigation with
      // no input at all. The generator has to be told what that renders, or it writes a page that
      // reads `undefined` context on a path a user can reach by clicking the nav entry.
      const de = p.directEntry || {};
      const behavior = de.behavior === 'selector'
        ? 'render a picker/list so the user can choose the record, then show it'
        : 'render an explanatory empty state (do NOT render a broken/blank detail)';
      out.push(`- **Direct entry (no input):** the page is in the sitemap, so it can be opened from the nav with no \`pageInput\` — ${behavior}.${de.note ? ` ${de.note}` : ''}`);

plugins/model-apps/scripts/lib/sdk-build.js:1080

• The publish warning label uses the GUID (form ${formId}), even though the human-friendly name is available as def.name and is already used for the push label. If a publish fails, warning output like publish form <guid> FAILED is much harder to act on than publish form <name> FAILED; consider using the same label string for both push and publish here.

This issue also appears in the following locations of the same file:

• line 1099
• line 1299
• line 1339

    requireSuccessfulPush(await provision.pushArtifact('form', formId), `form ${def.name}`, opts.warn);
    reportPartialPush(await provision.publishArtifact('form', formId), `form ${formId}`, opts.warn);
    await provision.addSolutionComponent({ componentId: formId, componentType: COMPONENT_TYPE.form, solutionUniqueName: sol.uniqueName });

plugins/model-apps/scripts/lib/sdk-build.js:1101

• Same as the form path above: the publish warning label uses view ${viewId} (GUID) even though def.name is available and used for the push label. Using the name for publish warnings would make failures much more actionable.

    requireSuccessfulPush(await provision.pushArtifact('view', viewId), `view ${def.name}`, opts.warn);
    reportPartialPush(await provision.publishArtifact('view', viewId), `view ${viewId}`, opts.warn);
    await provision.addSolutionComponent({ componentId: viewId, componentType: COMPONENT_TYPE.view, solutionUniqueName: sol.uniqueName });

plugins/model-apps/scripts/lib/sdk-build.js:1301

• When wiring form events, the publish warning label is form ${id} (GUID) while the surrounding step labels already use d.f.name || d.f.entity. For consistency and debuggability, consider passing the same human-readable label into reportPartialPush (e.g. form ${d.f.name || d.f.entity} events) so a failure points at the form/step the operator recognizes, not just the GUID.

            requireSuccessfulPush(await provision.pushArtifact('form', id), `form ${d.f.name || d.f.entity} events`, opts.warn);
            reportPartialPush(await provision.publishArtifact('form', id), `form ${id}`, opts.warn);
          }

plugins/model-apps/scripts/lib/sdk-build.js:1341

• In the quick-view placement path, the publish warning label is form ${hostId} (GUID) while the push label uses form ${f.name || f.entity} quick-views. Consider using the same human-readable label for reportPartialPush so publish failures don’t require correlating GUIDs back to a form.

          requireSuccessfulPush(await provision.pushArtifact('form', hostId), `form ${f.name || f.entity} quick-views`, opts.warn);
          reportPartialPush(await provision.publishArtifact('form', hostId), `form ${hostId}`, opts.warn);
        }

[Akshay Maloo (akshaymaloo)]

#459 folded in, plus the contract test that would have caught this class

Combined per request — one PR is easier to live-test and peer-review as a unit, and both halves touch the same language-code surface. #459 is closed; its commit is bcae5a2f, unchanged.

Also added the two things that were missing rather than broken:

• A behavioural contract test for publishArtifact against the real bundle. It was in the method-presence guard but had no behavioural one, which is precisely why the throw→return change was invisible to CI. Which way the bundle behaves is load-bearing in both directions: a throw silently re-arms the transient-retry path, a by-value failure silently disarms it.
• An integration test that the engine is wired to read the result. The unit tests pin reportPartialPush; they say nothing about whether anything calls it — and a wiring break is exactly what the original bug was.

One verification note worth recording

The first red-green of that integration test looked ineffective — I unwired the publish sites and it stayed green. That was the mutation being incomplete, not the test: the mapLimit site ends in )) rather than ); and slipped my regex, and it was the very site the test exercises. Re-run with a paren-matching unwire that reaches all nine, it turns red as it should. A test that passes for the wrong reason is worse than no test, so I chased it rather than accepting the green.

What is and is not live-verified

Live, artifacts torn down, 0 failures: full build 17/17 + verify 11/11, idempotent rebuild (the new etag/concurrency path), a --publish build 8/8 + verify 6/6, and teardown 11 deleted incl. deleteAppCascade.

Not live-verified: the download path (the languageCode preserve). I tried, but the test environment began returning 401 on WhoAmI partway through — access lapsed after my earlier runs today, which is environmental rather than a code problem. That change is a pure function with injected fs dependencies, covered by 4 red-green-verified tests including corrupt-JSON, invalid-value and read-throws cases, and it is best-effort by construction (any failure leaves the field absent, which is the pre-existing behaviour). I would still rather say it is untested live than imply otherwise.

1540 plugin + 159 eval tests pass; 7 validators green; 10/10 CI.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

plugins/model-apps/scripts/lib/sdk-build.js:1080

• reportPartialPush warnings will currently identify artifacts by GUID (form ${formId} / view ${viewId}), even though the human-readable def.name is available at these call sites. That makes publish-failure warnings hard to act on (operators typically know the form/view name, not the id). Consider using a label like form ${def.name} (${formId}) / view ${def.name} (${viewId}) for clearer output, consistent with the push labels on the previous line.

    requireSuccessfulPush(await provision.pushArtifact('form', formId), `form ${def.name}`, opts.warn);
    reportPartialPush(await provision.publishArtifact('form', formId), `form ${formId}`, opts.warn);
    await provision.addSolutionComponent({ componentId: formId, componentType: COMPONENT_TYPE.form, solutionUniqueName: sol.uniqueName });