Final-review fixes: harden env-match assertion + SKILL.md coherence

From a 4-angle final review of #202:

- verify-alm-prerequisites.js: sameEnvOrigin/envOrigin hardened so a bare host
  (no scheme) matches a scheme-prefixed URL, and an empty / unsubstituted
  "{CONFIGURED_ENV_URL}" / junk value returns null (indeterminate) instead of a
  bogus origin. The --expectedEnvUrl assertion now hard-stops ONLY on a definite
  mismatch (=== false), so a misrendered or schemeless value can no longer FALSE-
  hard-stop a legitimate deploy. Tests added (bare host, placeholder, null).
- deploy-pipeline Phase 1: show both invocation forms (with / without
  --expectedEnvUrl) so the agent omits the flag when no env URL is recorded
  instead of passing an empty/placeholder value.
- deploy-pipeline Phase 3: the 400-branch now documents the nav-property fallback
  for the rare missing-sourceDeploymentEnvironmentId case (still without setting
  VALIDATE_PACKAGE_UNAVAILABLE).
- plan-alm env-match gate: replace the two confusing "Cancel"-prefixed options with
  two distinct ones ("Switch PAC env & re-run" / "Continue anyway"), both documented.
- plan-alm step 6b: fix the self-referential "environmentUrl/environmentUrl-
  equivalent" typo → top-level `environmentUrl`.
- set-plan-status.js: unlink the staged temp if the final JSON rename fails (no
  orphaned .alm-plan-data.json.tmp).
- compute-split-plan.test.js: stale siteType fixture 'code-site' → 'code'.

Pre-existing issues NOT in this PR's scope (noted for follow-up): tableCountScope
'manifest-only' mislabel when an empty table-permissions dir + no manifest;
verifyOne String() value-mismatch vs missing-value categorization; parseEnvList
forward-compat edges on best-effort pre-fill.

1288 tests pass (+2). alm-lint 0, legacy-compat in sync, version-check pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tyaginidhi

plugins/power-pages/scripts/lib/set-plan-status.js

@@ -170,7 +170,15 @@ function setPlanStatus(opts) {
     fs.renameSync(htmlTmp, htmlPath);
     rendered = true;
   }
-  fs.renameSync(dataTmp, dataPath);
+  // Commit the JSON. If this final rename ever fails (e.g. a transient lock on the
+  // plan file), unlink the staged temp so we don't leave an orphaned
+  // `.alm-plan-data.json.tmp` behind, then rethrow so the caller sees the failure.
+  try {
+    fs.renameSync(dataTmp, dataPath);
+  } catch (e) {
+    try { fs.unlinkSync(dataTmp); } catch {}
+    throw e;
+  }
 
   return {
     ok: true,

plugins/power-pages/scripts/lib/verify-alm-prerequisites.js

@@ -42,15 +42,40 @@ function parseArgs(argv) {
   return { envUrl, requireManifest, expectedEnvUrl };
 }
 
-// Compare two Dataverse env URLs by origin only (scheme+host), case-insensitive,
-// path/query/trailing-slash ignored — so `https://Org.crm.dynamics.com/` and
-// `https://org.crm.dynamics.com/api/data/v9.2` count as the same environment.
+// Normalize a Dataverse env reference to its origin (scheme+host), lowercased.
+// Tolerates a missing scheme (`org.crm.dynamics.com` → `https://org.crm.dynamics.com`)
+// since a hand-authored manifest may omit it. Returns null when the value is empty
+// or not a parseable host — so the caller can treat "can't compare" distinctly from
+// "definitely different" and avoid a false mismatch on garbage input.
+function envOrigin(u) {
+  const s = String(u || '').trim();
+  if (!s) return null;
+  // Try as-is, then with an https:// prefix (covers a bare host like
+  // `org.crm.dynamics.com`). The WHATWG URL parser is lenient and will happily
+  // accept `https://{CONFIGURED_ENV_URL}` as a "host", so after parsing we ALSO
+  // require a plausible DNS hostname (dot-separated alnum/hyphen labels). That
+  // rejects an unsubstituted `{PLACEHOLDER}`, `null`, or other junk (→ null) so it
+  // can never be compared as if it were a real environment.
+  for (const candidate of [s, 'https://' + s.replace(/^\/+/, '')]) {
+    let parsed;
+    try { parsed = new URL(candidate); } catch { continue; }
+    const host = parsed.hostname.toLowerCase();
+    if (!/^[a-z0-9-]+(\.[a-z0-9-]+)+$/.test(host)) continue;
+    return parsed.origin.toLowerCase();
+  }
+  return null;
+}
+
+// Compare two env references by origin only — path/query/trailing-slash/case ignored.
+// Returns true (same), false (definitely different), or null (indeterminate: one side
+// isn't a parseable env URL). Callers must hard-stop ONLY on an explicit `false`, never
+// on null, so a missing/placeholder/garbage value disables the assertion instead of
+// blocking a legitimate run.
 function sameEnvOrigin(a, b) {
-  const origin = (u) => {
-    try { return new URL(u).origin.toLowerCase(); }
-    catch { return String(u || '').replace(/\/+$/, '').toLowerCase(); }
-  };
-  return origin(a) === origin(b);
+  const oa = envOrigin(a);
+  const ob = envOrigin(b);
+  if (oa === null || ob === null) return null;
+  return oa === ob;
 }
 
 async function verifyAlmPrerequisites({ envUrl, requireManifest, expectedEnvUrl } = {}) {
@@ -75,7 +100,12 @@ async function verifyAlmPrerequisites({ envUrl, requireManifest, expectedEnvUrl
   // env (from .solution-manifest.json / powerpages.config.json / the approved plan)
   // passes it here; a mismatch HARD-STOPS before any token acquisition or write, so
   // an ALM operation can never silently target the wrong environment (e.g. PROD).
-  if (expectedEnvUrl && !sameEnvOrigin(resolvedEnvUrl, expectedEnvUrl)) {
+  // HARD-STOP only on a DEFINITE mismatch (sameEnvOrigin === false). A null result
+  // means expectedEnvUrl wasn't a parseable env URL (empty, an unsubstituted
+  // `{PLACEHOLDER}`, junk) — in that case skip the assertion rather than block a
+  // legitimate run on bad input; the SKILL.md guidance is to omit the flag entirely
+  // when no env URL is recorded.
+  if (expectedEnvUrl && sameEnvOrigin(resolvedEnvUrl, expectedEnvUrl) === false) {
     throw new Error(
       `Environment mismatch: PAC CLI is connected to ${resolvedEnvUrl} but this project targets ` +
       `${expectedEnvUrl.replace(/\/+$/, '')}. Run \`pac env select --environment ${expectedEnvUrl.replace(/\/+$/, '')}\` ` +
@@ -157,4 +187,4 @@ if (require.main === module) {
     });
 }
 
-module.exports = { verifyAlmPrerequisites, parseArgs, sameEnvOrigin };
+module.exports = { verifyAlmPrerequisites, parseArgs, sameEnvOrigin, envOrigin };

plugins/power-pages/scripts/tests/compute-split-plan.test.js

@@ -39,7 +39,7 @@ function baseEstimate(overrides = {}) {
     botCount: 0,
     envVarCount: 5,
     mediaRatio: 0.3,
-    siteType: 'code-site',
+    siteType: 'code',
     tables: [],
     ...overrides,
   };

plugins/power-pages/scripts/tests/verify-alm-prerequisites.test.js

@@ -121,3 +121,36 @@ test('parseArgs captures --expectedEnvUrl', () => {
   assert.equal(a.requireManifest, true);
   assert.equal(parseArgs(['node', 'x']).expectedEnvUrl, null);
 });
+
+// --- sameEnvOrigin / envOrigin robustness (no false hard-stop on garbage/bare host) ---
+
+test('sameEnvOrigin: true/false for parseable URLs; null (indeterminate) for unparseable', () => {
+  const { sameEnvOrigin } = require('../lib/verify-alm-prerequisites');
+  assert.equal(sameEnvOrigin('https://a.crm.dynamics.com', 'https://A.crm.dynamics.com/api/data/v9.2'), true);
+  assert.equal(sameEnvOrigin('https://a.crm.dynamics.com', 'https://b.crm.dynamics.com'), false);
+  // Bare host (no scheme) on the expected side must still match a scheme-prefixed resolved URL.
+  assert.equal(sameEnvOrigin('https://dev.crm.dynamics.com', 'dev.crm.dynamics.com'), true);
+  // Unparseable / empty / unsubstituted placeholder → null (caller must NOT hard-stop).
+  assert.equal(sameEnvOrigin('https://dev.crm.dynamics.com', ''), null);
+  assert.equal(sameEnvOrigin('https://dev.crm.dynamics.com', '{CONFIGURED_ENV_URL}'), null);
+  assert.equal(sameEnvOrigin('https://dev.crm.dynamics.com', null), null);
+});
+
+test('verifyAlmPrerequisites does NOT hard-stop when expectedEnvUrl is an unsubstituted placeholder', async (t) => {
+  const helpers = require('../lib/validation-helpers');
+  const origEnv = helpers.getEnvironmentUrl;
+  const origToken = helpers.getAuthToken;
+  const origReq = helpers.makeRequest;
+  helpers.getEnvironmentUrl = () => 'https://dev.crm.dynamics.com';
+  helpers.getAuthToken = () => 'tok';
+  helpers.makeRequest = async () => ({ statusCode: 200, body: JSON.stringify({ UserId: 'u', OrganizationId: 'o' }) });
+  t.after(() => {
+    helpers.getEnvironmentUrl = origEnv;
+    helpers.getAuthToken = origToken;
+    helpers.makeRequest = origReq;
+  });
+  // A SKILL that fails to resolve {CONFIGURED_ENV_URL} would pass the literal — must
+  // NOT block the run (the assertion is skipped on an unparseable expected value).
+  const res = await verifyAlmPrerequisites({ expectedEnvUrl: '{CONFIGURED_ENV_URL}' });
+  assert.equal(res.envUrl, 'https://dev.crm.dynamics.com');
+});

plugins/power-pages/skills/deploy-pipeline/SKILL.md

@@ -125,11 +125,17 @@ Steps:
 
    Read the project's recorded env URL (first match wins): `.solution-manifest.json` → top-level `environmentUrl`, else `powerpages.config.json` → `environmentUrl`. Store as `CONFIGURED_ENV_URL`. (Both fields are top-level `environmentUrl` strings; declarative/EDM sites have no `powerpages.config.json`, so the manifest is the source there.)
 
+   **Pass `--expectedEnvUrl` only when `CONFIGURED_ENV_URL` actually resolved to a URL.** Use the first form when a recorded env URL exists, the second when neither file records one — do **not** pass an empty or unresolved `--expectedEnvUrl "{CONFIGURED_ENV_URL}"`:
+
    ```bash
+   # CONFIGURED_ENV_URL resolved (recorded in manifest/config) — assert PAC is on it:
    node "${PLUGIN_ROOT}/scripts/lib/verify-alm-prerequisites.js" --require-manifest --expectedEnvUrl "{CONFIGURED_ENV_URL}"
+
+   # Neither file records an env URL — omit the flag, fall back to the pac-context default:
+   node "${PLUGIN_ROOT}/scripts/lib/verify-alm-prerequisites.js" --require-manifest
    ```
 
-   `--expectedEnvUrl` makes the helper compare PAC's resolved env (origin-only) against `CONFIGURED_ENV_URL` and exit non-zero with an *"Environment mismatch: PAC CLI is connected to {X} but this project targets {Y} — run `pac env select …`"* error on mismatch. (If neither file records an env URL, omit `--expectedEnvUrl`; the helper falls back to the pac-context default with no assertion.) Capture output as JSON; extract `.envUrl` (store as `devEnvUrl`) and `.token` (store as `DEV_TOKEN`). If the script exits non-zero, stop and surface the error — it indicates an env mismatch, or that `az login` / `pac auth` / WhoAmI failed.
+   `--expectedEnvUrl` makes the helper compare PAC's resolved env (origin-only) against `CONFIGURED_ENV_URL` and exit non-zero with an *"Environment mismatch: PAC CLI is connected to {X} but this project targets {Y} — run `pac env select …`"* error on mismatch. (As a safety net the helper skips the assertion if the value isn't a parseable env URL — an empty string or an unsubstituted placeholder won't hard-stop — but prefer omitting the flag outright when there's no recorded URL.) Capture output as JSON; extract `.envUrl` (store as `devEnvUrl`) and `.token` (store as `DEV_TOKEN`). If the script exits non-zero, stop and surface the error — it indicates an env mismatch, or that `az login` / `pac auth` / WhoAmI failed.
 
 2. Run `detect-project-context.js` to read project config and solution manifest:
    ```bash
@@ -293,7 +299,7 @@ Use `solutionId` from `.solution-manifest.json` as `ARTIFACT_SOLUTION_ID` and `u
 > ```
 > Filter for `environmenttype = 200000000` to get the source record. Use `deploymentenvironmentid` as the `sourceDeploymentEnvironmentId`. For the artifact/solution list, use `sourceDeploymentEnvironmentId` from `docs/alm/last-pipeline.json` and `solutionName` from `.solution-manifest.json` as fallbacks. Set a flag `VALIDATE_PACKAGE_UNAVAILABLE = true` to skip Phase 4.2–4.3 and use the PAC CLI path in Phase 6.
 >
-> **If `RetrieveDeploymentPipelineInfo` returns a NON-404 error (e.g. 400/4xx/5xx)** — observed: some Pipelines packages return **400** for this call even though `ValidatePackageAsync` works fine — do **NOT** set `VALIDATE_PACKAGE_UNAVAILABLE`. The 404 branch above is specifically for older packages that lack the OData validation API; a 400 is just this metadata call failing, not the validation API being absent. Instead, fall back to `sourceDeploymentEnvironmentId` from `docs/alm/last-pipeline.json` (and `solutionName` from `.solution-manifest.json`) and **continue the normal `ValidatePackageAsync` flow** (Phase 4 onward). Only a genuine 404 — or a later `ValidatePackageAsync` 404 (Phase 4.2) — routes to the PAC-CLI path.
+> **If `RetrieveDeploymentPipelineInfo` returns a NON-404 error (e.g. 400/4xx/5xx)** — observed: some Pipelines packages return **400** for this call even though `ValidatePackageAsync` works fine — do **NOT** set `VALIDATE_PACKAGE_UNAVAILABLE`. The 404 branch above is specifically for older packages that lack the OData validation API; a 400 is just this metadata call failing, not the validation API being absent. Instead, fall back to `sourceDeploymentEnvironmentId` from `docs/alm/last-pipeline.json` (and `solutionName` from `.solution-manifest.json`) and **continue the normal `ValidatePackageAsync` flow** (Phase 4 onward). If `docs/alm/last-pipeline.json` is somehow missing `sourceDeploymentEnvironmentId`, use the same `deploymentpipeline_deploymentenvironment` navigation-property GET shown in the 404 branch above to recover it (still **without** setting `VALIDATE_PACKAGE_UNAVAILABLE`). Only a genuine 404 — or a later `ValidatePackageAsync` 404 (Phase 4.2) — routes to the PAC-CLI path.
 
 ### Phase 3.5 — Pre-deploy Completeness Check
 

plugins/power-pages/skills/plan-alm/SKILL.md

@@ -138,7 +138,7 @@ Steps:
 
 6b. **Environment-match guard** — confirm `pac env who` points at the project's environment *before* running discovery. `DEV_ENV_URL` comes from whatever environment PAC happens to be connected to, which is **not** guaranteed to be the project's. If it isn't, every query in Steps 7–12 runs against the wrong environment and silently produces a degraded plan (zero or wrong site settings, wrong size, wrong host) that *looks* valid. Cross-check both signals available:
 
-    1. **Recorded-URL comparison** (no token needed): collect any environment URL the project already records — `powerpages.config.json` → `environmentUrl` (code/SPA sites; absent for declarative/EDM sites) and `.solution-manifest.json` → its `environmentUrl`/`environmentUrl`-equivalent field if present. Normalize by **origin** (lowercase host, drop trailing slash + path/query). If any recorded URL exists and its origin **differs** from `DEV_ENV_URL`'s origin → **mismatch**.
+    1. **Recorded-URL comparison** (no token needed): collect any environment URL the project already records — `powerpages.config.json` → top-level `environmentUrl` (code/SPA sites; absent for declarative/EDM sites) and `.solution-manifest.json` → top-level `environmentUrl` if present. Normalize by **origin** (lowercase host, drop trailing slash + path/query). If any recorded URL exists and its origin **differs** from `DEV_ENV_URL`'s origin → **mismatch**.
     2. **Site-existence probe** (covers declarative/EDM sites that record no URL; only when `DEV_TOKEN` is available): verify the site's `websiteRecordId` actually exists in the connected env:
        ```
        GET {DEV_ENV_URL}/api/data/v9.2/powerpagesites({websiteRecordId})?$select=powerpagesiteid
@@ -155,10 +155,11 @@ Steps:
 
     | Question | Header | Options |
     |---|---|---|
-    | PAC CLI is connected to **{DEV_ENV_NAME}** (`{DEV_ENV_URL}`), which does not match this project's configured environment ({recorded URL, or "this site was not found there"}). Discovery will run against the connected environment. How do you want to proceed? | Env Mismatch | Cancel — switch PAC env, then re-run (Recommended), Continue against {DEV_ENV_NAME} anyway, Cancel |
+    | PAC CLI is connected to **{DEV_ENV_NAME}** (`{DEV_ENV_URL}`), which does not match this project's configured environment ({recorded URL, or "this site was not found there"}). Discovery will run against the connected environment. How do you want to proceed? | Env Mismatch | Switch PAC env & re-run (Recommended), Continue against {DEV_ENV_NAME} anyway |
 
-    - **Cancel — switch PAC env (Recommended)**: stop the skill. Tell the user to point PAC at the right environment (`pac auth select --name <profile>` or `pac org select --environment <url>`) and re-run `/power-pages:plan-alm`. Nothing has been written.
-    - **Continue anyway**: proceed to Step 7 against `DEV_ENV_URL`, but set `PLAN_QUALITY = "degraded"` and record the cause (*"discovery ran against {DEV_ENV_NAME}, which may not be the project's environment — verify the plan's site settings / size / host before executing"*) so Phase 3 surfaces it as a prominent risk.
+    Exactly two outcomes (both halt-or-proceed; no separate "cancel" — "Switch & re-run" already stops the skill):
+    - **Switch PAC env & re-run (Recommended)**: stop the skill. Tell the user to point PAC at the right environment (`pac auth select --name <profile>` or `pac org select --environment <url>`) and re-run `/power-pages:plan-alm`. Nothing has been written.
+    - **Continue against {DEV_ENV_NAME} anyway**: proceed to Step 7 against `DEV_ENV_URL`, but set `PLAN_QUALITY = "degraded"` and record the cause (*"discovery ran against {DEV_ENV_NAME}, which may not be the project's environment — verify the plan's site settings / size / host before executing"*) so Phase 3 surfaces it as a prominent risk.
 
     > **Why this exists**: a real EDM-site run produced a valid-looking plan after PAC had silently stayed connected to a different env than the project targeted. The site-existence probe + recorded-URL comparison catch that at the earliest gate, before any discovery runs.