fix(model-apps): teardown owns generative-page deletion

[Akshay Maloo (akshaymaloo)]

Teardown owns generative-page deletion

cds-maker-sdk's deleteAppCascade no longer deletes an app's generative pages. A uxagentproject is referenced by an app, not owned by one — another app's sitemap can carry the same GenPageId, and a form can embed the page via the MscrmControls.UxAgentControl PCF's RefId, a reference stored in formxml that appears in no sitemap. The SDK reports them instead and leaves the decision to the caller.

We are that caller — and we created the pages. Teardown now deletes them itself.

How

A new genpage step, ordered immediately after the app:

• Ownership from the page manifest, not the sitemap. The manifest web resource is the durable record of exactly which pages this build authored; the sitemap would also list pages someone else added. It's read before the web-resources phase deletes it.

• Safety delegated to Dataverse. Saving an app that surfaces a page creates a real solution dependency, so deleting that page fails with "cannot be deleted because it is referenced by N other components" — published or not. The dependency clears only once the subarea is removed and published, or the app and its sitemap are deleted outright (which the preceding step just did).

  So the delete is the check. Attempting it and reading the platform's answer beats a pre-flight scan: authoritative rather than our model of the truth, covers every surface the platform tracks instead of just sitemap XML, and no TOCTOU window.

• A dependency block is recorded as a skip for this kind (tolerateDependencyBlock) — another app legitimately owning the page is an expected outcome, not a broken teardown. It stays a genuine failure for every other kind.

• Files are deleted before the project. Anything we cannot prove is ours — absent/unreadable manifest, failed existence query — deletes nothing.

Tests

Seven new cases: manifest-scoped selection, delete ordering, the still-referenced skip, the dependency block remaining a failure for kinds that did not opt in, the already-gone page, and the missing manifest.

Four existing plan-shape assertions updated for the new step. deleteRecord added to SKILL_SDK_SURFACE — the surface-contract test correctly caught it.

Full suite: 1443 pass, 0 fail.

Sequencing

Merge after the SDK change and the re-vendor. Compatible with the currently vendored bundle either way: that bundle still deletes pages in the cascade, so resolve finds nothing live and the step is a no-op.

[Copilot]

Pull request overview

Updates the model-apps plugin teardown pipeline to explicitly delete generative pages (uxagentproject + uxagentprojectfile) authored by the build, compensating for the vendored SDK’s deleteAppCascade no longer cascading those deletes.

Changes:

• Add a new teardown step (genpage) immediately after the app delete, using the page manifest as the ownership authority and a cross-app sitemap scan to skip pages still referenced elsewhere.
• Extend teardown logic to delete genpage file rows before the project row via sdk.deleteRecord.
• Update unit tests and the SDK surface contract to cover and enforce the new behavior.

Reviewed changes

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

File	Description
plugins/model-apps/scripts/lib/sdk-teardown.js	Adds the genpage teardown step with manifest-scoped selection, cross-app safety scan, and ordered deletion (files then project).
plugins/model-apps/scripts/tests/sdk-teardown.test.js	Updates plan-shape assertions and adds a focused test suite covering genpage selection, ordering, safety/backoff, and failure modes.
plugins/model-apps/scripts/tests/sdk-surface-contract.test.js	Adds deleteRecord to the enforced vendored-SDK surface contract.

---

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

plugins/model-apps/scripts/lib/sdk-teardown.js:239

• genpage.del() deletes uxagentprojectfile rows in a loop, but if one file delete throws a not-found error (or other transient) the handler throws before attempting to delete the uxagentproject itself. Because deleteStep() treats any not-found as a successful delete of the whole item, this can incorrectly report the page as deleted while leaving the project row behind. Catch not-found per file delete and continue so the project delete still runs; rethrow non-not-found errors.

        if (f && f.uxagentprojectfileid) {
          await sdk.deleteRecord('uxagentprojectfile', f.uxagentprojectfileid);
        }
      }
      await sdk.deleteRecord('uxagentproject', item.id);

plugins/model-apps/scripts/tests/sdk-teardown.test.js:450

• This assertion message is now inaccurate: deleteAppCascade no longer deletes generative pages, and genpage teardown is handled by a separate step. Updating the message will avoid misleading future failures/debugging.

  assert.strictEqual(cascadeCalls[0].appModuleIdUnique, 'u-1', 'unique id passed (deleteAppCascade handles sitemap + genpage internally)');

[Copilot]

Pull request overview

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

Suppressed comments (5)

plugins/model-apps/scripts/lib/sdk-teardown.js:459

• The planTeardown comment mentions a “cross-app scan”, but no scan is performed; reference safety is enforced by Dataverse via the dependency-block error during deletion. The comment should be updated to reflect the actual mechanism.

    // Generative pages, AFTER the app. The SDK no longer deletes them — a page is
    // referenced by an app, not owned by one, so the SDK reports them and the owner decides. We are
    // the owner: the page manifest records exactly which pages this build authored. Ordered after the
    // app so the app's own sitemap reference is already gone and the cross-app scan only ever sees a
    // GENUINE other consumer. Emitted for every app-bearing spec (not gated on spec.pages) so a spec

plugins/model-apps/scripts/tests/sdk-teardown.test.js:1094

• This test configures otherApps/sitemap XML, but the current genpage handler doesn’t read appmodule/sitemap at all, so this setup doesn’t affect the assertion and is misleading. Simplify the setup so the test matches the behavior it’s actually verifying.

test('genpage deletes a page no other app references', async () => {
  const { sdk } = genpageSdk({
    manifestPages: [{ key: 'overview', name: 'Overview', pageId: PAGE_1 }],
    livePages: [PAGE_1],
    otherApps: [{ unique: 'other_app', xml: `<SiteMap><Area><Group><SubArea GenPageId="${PAGE_2}" /></Group></Area></SiteMap>` }],

plugins/model-apps/scripts/lib/sdk-teardown.js:17

• The header comment says the genpage step uses a “cross-app scan”, but the implementation delegates reference checks to Dataverse by attempting the delete and tolerating dependency-block errors. This comment is currently misleading and should be updated to match the actual behavior.

This issue also appears on line 455 of the same file.

//                      page is REFERENCED by an app, not owned by one — another app's sitemap, or a
//                      form's UxAgentControl `RefId` in formxml, can point at the same row — so the
//                      SDK reports them and the owner decides. Runs AFTER the app so the app's own
//                      sitemap reference is gone and the cross-app scan only sees genuine other
//                      consumers; a page any other app still references is SKIPPED, not deleted.

plugins/model-apps/scripts/lib/sdk-teardown.js:226

• This comment references isUndeletable, but dependency-block skips are handled by tolerateDependencyBlock + isDependencyBlocked. Keeping the reference to isUndeletable here is incorrect and makes it harder to understand the skip behavior.

    // Files first, then the project — the parent cannot be removed while children reference it.
    // A page still in use fails here with a dependency error, which `deleteStep` records as a SKIP
    // (see isUndeletable) rather than a teardown failure: another app legitimately owning the page
    // is an expected outcome, not a broken teardown.

plugins/model-apps/scripts/tests/sdk-teardown.test.js:998

• There are two duplicate “A minimal SDK stand-in…” comment blocks here, and both refer to a cross-app sitemap scan that the genpage handler does not perform. This should be deduplicated and updated to describe the helper’s actual purpose.

This issue also appears on line 1090 of the same file.

// A minimal SDK stand-in for the genpage handler: a page manifest in a web resource, the live
// uxagentproject/file rows, and the appmodule sitemaps the cross-app scan reads.
// A minimal SDK stand-in for the genpage handler: a page manifest in a web resource, the live
// uxagentproject/file rows, and the OTHER apps + sitemaps the cross-app scan walks.

[Copilot]

Pull request overview

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

Suppressed comments (2)

plugins/model-apps/scripts/tests/sdk-teardown.test.js:1001

• This comment block is duplicated and refers to a "cross-app scan"/fetchAppsForPages behavior that doesn't exist in these genpage tests, making the helper harder to understand.

// A minimal SDK stand-in for the genpage handler: a page manifest in a web resource, the live
// uxagentproject/file rows, and the appmodule sitemaps the cross-app scan reads.
// A minimal SDK stand-in for the genpage handler: a page manifest in a web resource, the live
// uxagentproject/file rows, and the OTHER apps + sitemaps the cross-app scan walks.
// `otherApps` is [{ unique, xml }] — the scan enumerates appmodules, then reads each one's sitemap

plugins/model-apps/scripts/tests/sdk-teardown.test.js:1098

• This test name claims it verifies deletion and cross-app references, but it only calls genpage.resolve and the otherApps sitemap data is never consulted. Renaming/removing the unused setup will avoid a false sense of coverage.

test('genpage deletes a page no other app references', async () => {
  const { sdk } = genpageSdk({
    manifestPages: [{ key: 'overview', name: 'Overview', pageId: PAGE_1 }],
    livePages: [PAGE_1],
    otherApps: [{ unique: 'other_app', xml: `<SiteMap><Area><Group><SubArea GenPageId="${PAGE_2}" /></Group></Area></SiteMap>` }],

[Akshay Maloo (akshaymaloo)]

Review comments addressed — one was a genuine data-loss bug

Thanks for the passes. The headline finding was correct and I'd have shipped it: the files-first delete order could gut a page the platform refuses to delete.

I verified it against a live environment instead of reasoning from the code:

Measurement	Result
RetrieveDependenciesForDelete on pages referenced by an app sitemap	1 dependent; DELETE returns 400 … referenced by 1 other components
Same call on 20 files across 5 of those referenced pages	0 dependents — every one. They would all have deleted cleanly
uxagentproject → uxagentprojectfile relationship metadata	CascadeConfiguration.Delete = "Cascade"

So the page row is protected; its content is not. Deleting files first stripped a referenced page bare moments before the platform refused to delete it.

Because the relationship cascades, the file loop was never needed. The fix is a deletion:

async del(sdk, item) {
  await sdk.deleteRecord('uxagentproject', item.id);
}

Every comment, and what changed

Finding	Status
Files deleted before the dependency-block check can fire	Fixed — one delete; a block now happens before any write
Not-found mid-loop → page reported deleted while the project row survives	Fixed — no loop, so the path is gone
Comment cites isUndeletable; skips actually use tolerateDependencyBlock	Fixed
Header + planTeardown comments describe a "cross-app scan" that doesn't exist	Fixed — both now describe platform arbitration
Duplicated "A minimal SDK stand-in…" block referencing the scan	Fixed — deduplicated, and the dead otherApps / sitemapThrows / appmodule / appmodulecomponent / sitemap scaffolding removed
genpage deletes a page no other app references — unused otherApps setup, only calls resolve	Fixed — renamed to genpage resolve ignores live pages the manifest does not claim, false setup dropped
Stale assertion message claiming deleteAppCascade handles genpages internally	Fixed

On test rigour

Both regression tests were verified red against the old ordering before being taken green. That mattered: my first skip test passed even with the bug reintroduced, because its mock returned no files. It now supplies files and asserts the page delete is the only write attempted.

1444/1444 passing.

[Copilot]

Pull request overview

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

Suppressed comments (1)

plugins/model-apps/scripts/lib/sdk-teardown.js:622

• deleteStep now treats dependency-blocked deletes as skippedIds, but downstream reporting assumes skippedIds means “undeletable system/managed artifacts”. This makes the emitted summary misleading (it will say “undeletable” for a page that is simply still referenced) and the CLI step counts will mark the step as “✓ deleted” even when 0 records were deleted. Consider returning/propagating a distinct skip reason (e.g., dependencyBlockedIds vs undeletableIds, or skipped: [{id, reason}]) so logs can accurately report “referenced — skipped” and step status/counting can reflect a skip when appropriate.

      if (handler.tolerateDependencyBlock && isDependencyBlocked(err)) {
        // The platform refused because something else still references this record. For a
        // generative page that is the CORRECT outcome, not a leftover: the page belongs to whoever
        // still points at it, and Dataverse is the authority on that (live-measured — saving an app
        // that surfaces a page creates the dependency, published or not). Record it as skipped so

[Akshay Maloo (akshaymaloo)]

Round 2 comment addressed — skip reasons are now distinct

deleteStep now treats dependency-blocked deletes as skippedIds, but downstream reporting assumes skippedIds means "undeletable system/managed artifacts"… Consider returning/propagating a distinct skip reason.

Correct, and worth fixing — the two outcomes mean opposite things to whoever reads the run:

• undeletable — a system/managed artifact that can never be removed. Nothing to act on.
• referenced — the platform refused because something else still points at it. Perfectly deletable once that consumer releases it, and for a generative page it's the expected result.

Collapsing them meant a teardown that correctly skipped a shared page printed 1 undeletable, sending the reader hunting a platform problem that doesn't exist.

deleteStep now returns skipped: [{ id, reason }] alongside the existing skippedIds union (so count-only callers are unaffected), and the step summary names each reason separately:

before   generative pages authored by this app (0 deleted, 2 undeletable)
after    generative pages authored by this app (0 deleted, 2 still referenced)

Verified by running the real deleteStep against a dependency-block error, not just by reading the code.

Added a test per reason — a dependency block reports as "still referenced", never "undeletable" and a system/managed artifact still reports as "undeletable", not "referenced" — so the two cannot silently merge again.

On the step status half of your comment: I left the step as ok. A skipped-because-referenced page is a successful teardown of everything this build owns, so failing or warning the step would be the same category error in the other direction. The count and reason are now accurate in the summary line, which is what makes it actionable.

1446/1446 passing.

[Copilot]

Pull request overview

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

Suppressed comments (2)

plugins/model-apps/scripts/tests/sdk-teardown.test.js:1128

• This test duplicates the earlier "genpage resolve returns only pages the manifest says WE authored" case (same manifest/live setup and same assertion). It would be more valuable to cover a distinct edge case (e.g., manifest entries with invalid/non-GUID pageIds are ignored).

test('genpage resolve ignores live pages the manifest does not claim', async () => {
  const { sdk } = genpageSdk({
    manifestPages: [{ key: 'overview', name: 'Overview', pageId: PAGE_1 }],
    livePages: [PAGE_1, PAGE_2],
  });

plugins/model-apps/scripts/lib/sdk-teardown.js:83

• The comment block above isDependencyBlocked currently describes “system/managed artifacts”, but the next function is actually dependency-block detection. This makes the intent harder to follow when scanning the helpers; consider splitting the comments so each helper documents its own behavior.

// Detect a system/managed artifact that Dataverse refuses to delete (e.g. the auto-generated
// "Active <Entity>" view whose name a spec view may reuse). It is not ours to remove, so a
// teardown skips it instead of failing — the same best-effort spirit as isNotFound. Deliberately
// NARROW: it must NOT match a dependency block ("...cannot be deleted because it is referenced by
// N other components"), which is a genuine leftover the teardown must surface, not swallow.

[Akshay Maloo (akshaymaloo)]

Live-tested — and it closed the last open surface, with a bad answer

Asked whether I'd live-tested this. I had measured the platform facts the fix relies on, but not the remaining unknown. Now done.

The form surface is NOT dependency-tracked

Built a form on account with a throwaway page in the MscrmControls.UxAgentControl PCF's RefId, across all three form factors, saved and published — then read the form back from the server to confirm the control and page GUID persisted rather than being silently stripped, so this isn't a vacuous negative.

Probe	Result
RetrieveDependenciesForDelete(pageId, 10372) after save	0
Same, after publish	0
RetrieveRequiredComponents(formId, 60)	1 row — type 66 (the PCF), never the page
DELETE /uxagentprojects(pageId) with the published form live	204 — deleted

Why: RefId is static="true" type="SingleLine.Text". The platform tracks the form's dependency on the control, not on the page the control points at.

So platform arbitration is authoritative for sitemaps and blind to forms. A page can be deleted out from under a published form with no objection. Not closable by asking the platform — and the cross-app sitemap scan I discarded wouldn't have closed it either, since the reference isn't in a sitemap.

Does this change this PR? Only the comment.

This step deletes only pages its own manifest authored, while tearing down the app that owns them — it never deletes on another owner's behalf. That was already the right position, and it happens to be the only one that stays correct for a surface the platform can't report.

What was wrong was the wording: the comment claimed attempting the delete "covers every surface the platform tracks". Literally true, but it invited a stronger conclusion than the facts support. Now states the gap explicitly (5666412b).

What I could NOT verify

That deleting a uxagentproject actually removes its uxagentprojectfile children. The relationship metadata is authoritative — CascadeConfiguration.Delete = "Cascade" — but I couldn't watch it happen: uxagentprojectfile is a private entity that silently refuses direct creation (POST returns 204 with an id; the row never persists), so no synthetic page with files could be built.

Risk if the metadata were somehow wrong: orphaned file rows — a cleanliness problem, not data loss, and strictly better than the files-first order it replaces. Flagging it rather than implying I proved it.

1446/1446 passing.

[Copilot]

Pull request overview

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

Suppressed comments (1)

plugins/model-apps/scripts/tests/sdk-teardown.test.js:1128

• This test duplicates the earlier "genpage resolve returns only pages the manifest says WE authored" case (same manifestPages/livePages and same assertion), which adds runtime without increasing coverage. Consider replacing it with a distinct assertion (e.g., that non-GUID manifest pageIds are ignored) or removing it.

test('genpage resolve ignores live pages the manifest does not claim', async () => {
  const { sdk } = genpageSdk({
    manifestPages: [{ key: 'overview', name: 'Overview', pageId: PAGE_1 }],
    livePages: [PAGE_1, PAGE_2],
  });

[Akshay Maloo (akshaymaloo)]

Cascade now observed end-to-end — the last assumption is gone

I said earlier I couldn't verify that deleting the project row actually removes its files, because uxagentprojectfile is a private entity that silently refuses direct creation. Solved it with pac, which builds a page the way the product does.

page: zzcascade_probe  (pac model genpage upload) — 4 real file rows
   src/pages/page.tsx
   src/pages/page.compiled
   config.json
   firstPrompt.json

DELETE /uxagentprojects(<id>)  ->  204
   src/pages/page.tsx        -> 404 (cascaded away)
   src/pages/page.compiled   -> 404 (cascaded away)
   config.json               -> 404 (cascaded away)
   firstPrompt.json          -> 404 (cascaded away)

RESULT: 4 cascaded, 0 orphaned

So CascadeConfiguration.Delete = "Cascade" holds in practice, not just in metadata. That was the one claim this fix depends on — if the cascade hadn't fired, deleting only the project row would leak file rows. It doesn't.

Every claim behind this PR is now measured, not inferred.

A methodology note worth sharing

The first run of that probe reported "0 files" — which would have looked like a clean negative result. It was actually a 401 on every request: I'd minted the Dataverse token from the corporate tenant. The probe did (res.json && res.json.value) || [], which quietly turned an auth failure into an empty result set.

Same class of mistake as the vacuous skip test earlier in this PR: a check that cannot fail tells you nothing when it passes. Probes now assert res.ok and throw with status and body before interpreting anything. Both traps are written up in the package's AGENTS.md.

Test org left at zero residue — every probe page, form and app cleaned up and verified.

1446/1446 passing.

[Copilot]

Pull request overview

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