[Pages][ALM] plan-alm: deterministic Draft/Approved status writer + EDM-site reliability fixes + siteType→declarative rename - #202
Merged
Conversation
…maintain the plan) plan-alm previously executed the whole deployment from a single "approve and execute" answer, which under autopilot ran an entire unattended deployment with a silently git-captured approver. Since a skill cannot reliably detect it is running unattended, the only robust fix is structural: make NOT executing the default. - plan-alm is now a 4-phase planner (Detect → Gather → Generate → Approve & save). Phases 5–8 (the execute orchestration) are removed; Phase 4 offers Save-approved / Save-draft / Change — there is no "execute" path. Adds PLAN_MODE, PLAN_QUALITY (degraded on auth/discovery failure), decisionsLog, an always-interactive approver capture, and a completeness gate. - Execution skills are self-sufficient: each ALM skill's Phase 0 recommends creating the plan via plan-alm if missing, and its final phase refreshes the plan + prints the next recommended step (refresh-alm-plan-data.js now emits `nextStep`). No auto-chaining — sequencing is user-driven. - Reverts an autopilot-defaults policy + an uncatalogued q2-manual-confirm gate that a prior session added (a skill can't detect autopilot; it also broke the gate lint); keeps the "(Recommended)" Q2 labels + inline recommendation. - Gate catalog, render comment, README, and AGENTS.md updated to match. PR 1 of 4 (stacked): plan-only → EDM → table-discovery → refresh-enforcement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- refresh-alm-plan-data.js reconcile(): per-phase refresh failures are no longer
swallowed by an empty catch. Each failure is captured into result.failed =
[{ phase, error }] and written to stderr, so a marker-schema break that makes a
refresh throw is diagnosable instead of silently skipped. `reconciled` now lists
only the phases that actually healed; `failed` is present (array) on every return
path. Contract-guard test added.
- setup-solution SKILL.md: the next-step guidance documented `nextStep: { name, skill }`
but `skill` can be null for an internal step (e.g. Finalize) — the prose would tell
the user to "run null". Contract corrected to `skill: string | null` and the guidance
now branches: print the command only when skill is non-null, else name the step alone.
- plan-alm SKILL.md: the finalize commit step always showed the Approved message with a
vague "(use the (draft) suffix for option 2)" aside. Now shows two explicit commit
commands — Approved vs Draft — so the Draft path can't be followed incorrectly.
1183 tests pass, alm-lint 0 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From a full review pass of the plan-only conversion:
- refreshDeployPipeline now completes the "Activate site in {stage}" step when
the deploy marker evidences activation (activationStatus set, deploy not
failed). The PP deploy flow activates the site internally, so leaving the step
pending made computeNextStep redundantly nudge the user to run /activate-site
for work the deploy already did. Testing stays a separate step (test-site).
When the marker has no activationStatus, the Activate step is left pending.
- Next-steps guidance corrected: the PP path does activation inside the deploy
flow but NOT testing — testing is the separate /power-pages:test-site step the
plan already lists. Guidance now ends "... → /power-pages:test-site".
- steps[] template: documented the MANUAL-path shape (Setup solution → Export
solution → per-target Import/Activate/Test) alongside the PP-path example, so a
manual-strategy plan emits step names that step-sync + computeNextStep match —
previously only the PP shape was shown.
- Error Handling: the "No powerpages.config.json: stop" rule contradicted Phase 1,
which resolves data-model/EDM sites from .powerpages-site/website.yml (no
config.json). Now stops only when BOTH markers are absent.
1185 tests pass, alm-lint 0 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- refreshDeployPipeline: gate the Activate-step auto-completion on the explicit
"Activated" outcome, not mere truthiness. deploy-pipeline writes
activationStatus: "Pending" when the user DEFERS activation — that's truthy but
means /power-pages:activate-site is still required, so the previous check would
have wrongly completed the Activate step and dropped it from nextStep. Now any
non-"Activated" value (Pending, null, a failure note) leaves the step pending.
Regression test extended to cover undefined/null/"Pending"/"Failed".
- nextStep skill:null guidance: the same "run {nextStep.skill}" guidance lived in
7 other execution skills (activate-site, configure-env-variables, deploy-pipeline,
export-solution, import-solution, setup-pipeline, test-site) — the prior fix only
updated setup-solution. All 7 now document `skill: string | null` and branch so
an internal step (e.g. Finalize) names the step without ever printing `run null`.
1185 tests pass, alm-lint 0 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… label priyanshu92 review on #191: a finished PP deploy never advanced its plan step. setup-pipeline names pipeline stages "Deploy to {targetLabel}", and deploy-pipeline writes that verbatim as last-deploy.json's stageName ("Deploy to Staging"). The plan step is "Deploy via pipeline to Staging", so setStepStatus's substring match of "deploy to staging" against the step name failed and the step (and the Activate step my earlier change keys off the same stage) stayed pending forever — defeating the self-maintaining-plan goal. Every existing fixture used the bare "Staging", so the suite gave false confidence. setStepStatus now strips a leading "Deploy to " from the stage filter to recover the bare label before matching; callers that already pass the label (e.g. test-site --stageName "Staging") are unaffected (no-op strip). Added regression tests using the REAL "Deploy to Staging" marker shape (step + activate flip; Production untouched) plus a direct setStepStatus normalization test. Also removed a duplicate inline comment on the 'ensure-pipelines-host' PHASES entry (per review — the explanation already lives at the refreshEnsurePipelinesHost def). 1187 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
reconcile() documents a `nextStep` field but three return paths omitted it, forcing callers to special-case a missing property: - .alm-deferred early return -> now nextStep: null (nothing to guide toward) - no-plan early return -> now nextStep: null (no plan to compute from) - nothing-pending return -> the plan is current with all markers but may still have unfinished checklist steps, so load the plan and return the computed nextStep instead of dropping it. The plan is now parsed once before the pending-size check and reused by both the no-op and heal paths. Contract-guard test extended to assert failed[] AND nextStep are present on every return path (null for deferred/no-plan; the next unfinished step for nothing-pending). 1187 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the step-sync fix. Factored the "strip leading 'Deploy to '" logic
into a shared normalizeStageLabel() helper and routed BOTH setStepStatus AND the
per-stage object-key paths (validationRuns / manualImports / activations) through
it. Previously only setStepStatus stripped the prefix; the key paths used the
resolved stage verbatim, so if a marker ever carried "Deploy to {label}" the
renderer (which keys by the bare label) would silently miss the run. Markers there
currently emit the bare label so this is defensive — it closes the whole class of
mismatch in one place rather than one code path. Added a test that a "Deploy to
Staging" stage keys validationRuns["Staging"].
1188 tests pass, alm-lint 0 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- refresh-alm-plan-data.js header comment: document the nextStep stdout shape as
`{ name, skill: string | null } | null` (skill is null for internal/unmapped
steps) — the SKILL.md guidance + the JSDoc already branch on this; the file
header was the last spot still showing the old non-nullable shape.
- test fixtures: align STRATEGY to the canonical schema value `pp-pipelines`
(SKILL.md planData + render-alm-plan.js both use it; `pipeline` was non-schema).
All 5 fixtures updated. refresh-alm-plan-data.js doesn't branch on STRATEGY, so
this is correctness/representativeness only — no behavior change.
- plan-alm next-steps guidance: the PP-path line implied deploy always activates;
clarified that the deploy flow activates the site but if you DEFER activation
(activationStatus "Pending"), run /power-pages:activate-site.
1188 tests pass, alm-lint 0 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…scovery
Enhanced data-model sites are downloaded with `pac pages download` (not
download-code-site): they have NO `powerpages.config.json` and no SPA build output —
just `.powerpages-site/` with a config tree + `website.yml`. The shared discovery
helpers hard-required `powerpages.config.json`, so every ALM skill broke on EDM sites.
- `findProjectRoot` (validation-helpers.js): treats a `.powerpages-site/` directory as
a project-root marker, not just `powerpages.config.json`.
- `detect-project-context.js`: falls back to `.powerpages-site/website.yml`
(`id`→websiteRecordId, `name`→siteName) when no `powerpages.config.json`; returns a
new `siteType` ("code" | "data-model"); exits 1 only when neither marker exists.
- `check-activation-status.js`: same fallback (verified live against an EDM site →
resolves identity + activation status instead of erroring).
Backward-compatible: `powerpages.config.json` stays the primary signal for code sites.
PR 2 of 4 (stacked, on plan-alm-plan-only).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces publisher-prefix table discovery (which over-counts catastrophically with a shared/default publisher — a 6-table site matched 22 unrelated tables — and misses real tables from a different prefix) with site-referenced scoping: the custom tables the site's `.powerpages-site/table-permissions/` (+ datamodel manifest) actually reference, intersected with the env's custom-unmanaged tables. SME-confirmed: table permissions are the complete signal. Replaces the old one-solution-per-table-name-stem split heuristic (which produced ~one solution per table — e.g. a 21-solution split) with a dependency-aware, capacity-bounded packer: union-find connected-component clusters over table relationships (lookups + N:N), then first-fit-decreasing bin-packing of whole clusters into the fewest solutions under maxTableCount/maxSchemaAttrs, capped at maxSchemaSplitSolutions (8). The split trigger + thresholds are unchanged — only the packing. New shared libs: - resolve-site-tables.js — site-referenced table scoping (single source of truth) - query-metadata.js — consolidated custom-unmanaged-table query - query-table-relationships.js — relationship edges (lib; audit-permissions CLI is now a thin wrapper) - validation-helpers.js — odataGet/odataGetAll shared paginator estimate-solution-size.js now emits tableCountScope + tableRelationships[]; compute-split-plan.js consumes the edges. setup-solution Phase 5.2.D uses the shared discovery helper. 1209 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
deriveDomainsByCapacity seeded the packer's bin count from a LOWER bound (max(ceil(tables/maxTableCount), ceil(attrs/maxSchemaAttrs))), so when independent (no-edge) clusters fragment, FFD ran out of bins and dropped the non-fitting cluster into the least-loaded bucket — overflowing it past maxSchemaAttrs with no warning (the oversized-cluster guard only checks per- cluster table COUNT, not attrs). Verified repro: 4 independent 8000-attr tables, maxSchemaAttrs 15000 -> seed n=3 -> one bucket holds 16000 attrs. Seed the packer with the maximum permitted bins instead (one per cluster, capped at maxSchemaSplitSolutions). FFD still consolidates — clusters that fit together share a bin and empty bins are dropped, so the solution count stays minimal — but a cluster that fits nowhere opens a NEW bin rather than overflowing. The existing 16000-attr/2-solution test is unchanged (FFD still consolidates); added a regression test for the 4-independent-table overflow case. 1211 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The site-referenced table discovery (this PR) made discover-site-components return customTables (and thus missing.customTables) ONLY when --projectRoot (or --datamodelManifest) is passed — without a local signal it returns [] rather than the old publisher-prefix dump. setup-solution Step D was updated to pass --projectRoot, but three other consumers that read missing.customTables were missed, so their "custom tables missing from the solution" completeness check silently reported 0 for every site: - export-solution Phase 2.5 (pre-export completeness) - plan-alm Phase 1 (pre-plan completeness) - deploy-pipeline Phase 3.5 (pre-sync completeness) Added --projectRoot "." to all three so the check is restored with the correct site-scoped count. (setup-solution Phase 5.4b/5.4c calls consume missing.envVars / missing.powerpagecomponents / siteLanguages, not customTables, so they're correct as-is with --publisherPrefix/--solutionId.) 1211 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The refresh-alm-plan-data.js calls in each ALM SKILL.md are advisory markdown — silently dropped on session fragmentation, manual execution, or oversight. Three observed gaps where the rendered plan never reflected real run state: ensure-pipelines-host (no refresh call at all), setup-pipeline (refresh lives in Phase 7; phases 5-6 run manually after a resume skip it), activate-site (Phase 5.2b refresh missed). Fix is auto-heal, not fail-validation (per feedback_skill_validation_hooks: never hard-block). After ANY ALM plan skill completes, the centralized PostToolUse hook spawnSyncs `refresh-alm-plan-data.js --reconcile --render`, which ingests any marker (docs/alm/last-*.json) newer than the plan. Because it fires on any ALM skill (not just the marker's writer), a skip in skill A is healed when the next ALM skill B completes. Best-effort and non-blocking: never changes the hook's exit code, honors .alm-deferred, idempotent. - powerpages-hook-utils.js: ALM_PLAN_SKILLS set + isAlmPlanSkill(value) - run-skill-posttool-validation.js: reconcile backstop after the validator - ensure-pipelines-host SKILL.md: explicit Phase 6 self-refresh (direct gap-1 fix; hook reconcile remains the backstop) The reconcile mode + ensure-pipelines-host phase in refresh-alm-plan-data.js ship in the plan-only PR earlier in this stack. 1212 tests pass, alm-lint 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follows the Copilot review fix in #191 (reconcile no longer swallows per-phase errors): the PostToolUse hook now reads result.failed alongside reconciled and prints a one-line, non-blocking notice naming the phases that could not heal, so a marker-schema break is diagnosable at the hook level (details still go to the lib's stderr). AGENTS.md reconcile bullet updated to document the failed[] field. 1213 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression pass finding: the refresh-alm-plan-data.js doc entry still said it was "Used by plan-alm Phases 6 / 7 / 8", but those phases were deleted when plan-alm was converted to a plan-only 4-phase planner. The helper is now driven by the execution skills' final-phase refresh + the PostToolUse --reconcile backstop, not by plan-alm. Corrected the clause (CLAUDE.md is a symlink to AGENTS.md, so both are fixed in one edit). 1218 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eted
The plan had an elaborate heartbeat/active-chain lifecycle (check-alm-plan.js:
Draft|Approved|In Execution|Completed + 60-min LAST_INVOCATION_AT heartbeat,
stale-heartbeat reclassification) that was entirely DEAD: nothing ever set
"In Execution", and refreshFinalize ("Completed") was never called. So every
plan was stuck at "Approved" and the heartbeat machinery never engaged.
Gap 1 — Approved -> In Execution (check-alm-plan.js): plan-alm is plan-only and
leaves the plan "Approved"; the FIRST execution skill's Phase 0 call now promotes
it to "In Execution" and writes the first heartbeat (one atomic write). Gated on
writeHeartbeat, so read-only callers don't mutate the plan — and plan-alm's own
deferral check now passes --no-heartbeat (a planner re-run isn't execution and
must not promote).
Gap 5 — In Execution -> Completed (refresh-alm-plan-data.js): a completion
evaluator runs after every phase's step-sync (refresh + reconcile) and flips
PLAN_STATUS to "Completed" + stamps COMPLETED_AT once every non-skip step is
completed and none failed. The last execution skill terminates the plan
automatically — no skill needs to call --phase finalize. A failed step blocks
completion so a failed deploy can't look "done".
Gaps 2/3/4 (ensure-pipelines-host / setup-pipeline / activate-site refresh calls)
were already in place + covered by the reconcile backstop — no change needed.
+8 tests (3 promotion in check-alm-plan, 5 completion in refresh); 1227 pass,
alm-lint 0 findings. AGENTS.md documents both transitions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the lifecycle reaches "Completed", the plan footer now shows a "Completed: <timestamp>" line beneath the approver/approval-date, rendered from COMPLETED_AT. Conditional — the `__COMPLETED_LINE__` placeholder maps to an empty string until the plan completes, so there's no orphan token and the line only appears once execution is done. (The template already styled the .completed and .in-execution status badges; this adds the timestamp detail.) +2 renderer tests. 1229 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces publisher-prefix table discovery (which over-counts catastrophically with a shared/default publisher — a 6-table site matched 22 unrelated tables — and misses real tables from a different prefix) with site-referenced scoping: the custom tables the site's `.powerpages-site/table-permissions/` (+ datamodel manifest) actually reference, intersected with the env's custom-unmanaged tables. SME-confirmed: table permissions are the complete signal. Replaces the old one-solution-per-table-name-stem split heuristic (which produced ~one solution per table — e.g. a 21-solution split) with a dependency-aware, capacity-bounded packer: union-find connected-component clusters over table relationships (lookups + N:N), then first-fit-decreasing bin-packing of whole clusters into the fewest solutions under maxTableCount/maxSchemaAttrs, capped at maxSchemaSplitSolutions (8). The split trigger + thresholds are unchanged — only the packing. New shared libs: - resolve-site-tables.js — site-referenced table scoping (single source of truth) - query-metadata.js — consolidated custom-unmanaged-table query - query-table-relationships.js — relationship edges (lib; audit-permissions CLI is now a thin wrapper) - validation-helpers.js — odataGet/odataGetAll shared paginator estimate-solution-size.js now emits tableCountScope + tableRelationships[]; compute-split-plan.js consumes the edges. setup-solution Phase 5.2.D uses the shared discovery helper. 1209 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
deriveDomainsByCapacity seeded the packer's bin count from a LOWER bound (max(ceil(tables/maxTableCount), ceil(attrs/maxSchemaAttrs))), so when independent (no-edge) clusters fragment, FFD ran out of bins and dropped the non-fitting cluster into the least-loaded bucket — overflowing it past maxSchemaAttrs with no warning (the oversized-cluster guard only checks per- cluster table COUNT, not attrs). Verified repro: 4 independent 8000-attr tables, maxSchemaAttrs 15000 -> seed n=3 -> one bucket holds 16000 attrs. Seed the packer with the maximum permitted bins instead (one per cluster, capped at maxSchemaSplitSolutions). FFD still consolidates — clusters that fit together share a bin and empty bins are dropped, so the solution count stays minimal — but a cluster that fits nowhere opens a NEW bin rather than overflowing. The existing 16000-attr/2-solution test is unchanged (FFD still consolidates); added a regression test for the 4-independent-table overflow case. 1211 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The site-referenced table discovery (this PR) made discover-site-components return customTables (and thus missing.customTables) ONLY when --projectRoot (or --datamodelManifest) is passed — without a local signal it returns [] rather than the old publisher-prefix dump. setup-solution Step D was updated to pass --projectRoot, but three other consumers that read missing.customTables were missed, so their "custom tables missing from the solution" completeness check silently reported 0 for every site: - export-solution Phase 2.5 (pre-export completeness) - plan-alm Phase 1 (pre-plan completeness) - deploy-pipeline Phase 3.5 (pre-sync completeness) Added --projectRoot "." to all three so the check is restored with the correct site-scoped count. (setup-solution Phase 5.4b/5.4c calls consume missing.envVars / missing.powerpagecomponents / siteLanguages, not customTables, so they're correct as-is with --publisherPrefix/--solutionId.) 1211 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + 4 more Substantive (both reviews flagged): - **maxSchemaAttrs overflow now warned at the ceiling boundary** (compute-split-plan.js). Commit 2 fixed the under-ceiling case (seed n=min(clusters,ceiling)); at the maxSchemaSplitSolutions ceiling (>8 independent attr-heavy clusters) the FFD least-loaded fallback can still co-locate clusters and bust maxSchemaAttrs. The oversized guard only checked table COUNT — added a companion attr-cap warning (summed attributeCount > maxSchemaAttrs). Regression test with 9×14000-attr clusters. - **Table-domain componentCount is now a schema-component proxy** (sum(attributeCount) + 1/table), not the table count. Counting 1/table severely undercounts solution components and could let an over-cap Table solution slip past validateSplits (and distorted the Site solution's subtracted count). Test asserts 500+300+2 = 802. Copilot inline: - **odataGetAll FAILS CLOSED**: throws if it hits maxPages with @odata.nextLink still present, instead of silently returning a truncated set (wrong ALM counts). Test added. - **resolve-site-tables: sources.tablePermissions counts permission FILES**, not parsed records — a malformed file no longer makes a real site look manifest-only/unavailable (which drives tableCountScope). Test with a malformed file. - **Renamed local odataGet → odataGetPath** in estimate-solution-size.js to avoid the name collision with validation-helpers' shared odataGet (different arg order). Minor: - **discoverTableRelationships now uses bounded concurrency (5)** instead of fully serial (~2 calls/table; a 34-table site was 68 sequential round-trips). Edge assembly stays sequential for deterministic dedup. - AGENTS.md: corrected the packing claim (both cap-exceeding cases now warn, not "always under the caps"). Deferred (tracked as follow-up): renderer doesn't surface tableCountScope:'unavailable' distinctly — edge-case (plan-alm always passes --projectRoot) + touches the merged #191 renderer; bundling with the renderer/terminology follow-up. 1228 tests pass, alm-lint 0 findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Merge main into branch" resolution (525405a) left three problems that broke validate-repository-metadata + the env-var convention: - Restored the `.claude-plugin/plugin.json` legacy symlink (the merge deleted it → validator "missing legacy plugin manifest"). - marketplace.json power-pages version 2.4.0 -> 2.5.0 (it was left out of sync with .plugin/plugin.json's 2.5.0 → validator version-mismatch). - ensure-pipelines-host SKILL.md:906 (this PR's Phase 6 refresh call) ${CLAUDE_PLUGIN_ROOT} -> ${PLUGIN_ROOT} (post Open Plugins migration convention). validate-legacy-compatibility: metadata in sync. 1238 tests pass, alm-lint 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e tests Three minor findings from the /review 194 pass: 1. Hardcoded plan path — `docs/.alm-plan-data.json` / `docs/alm-plan.html` were inlined at four call-sites (the PostToolUse hook, check-alm-plan.js, and refresh-alm-plan-data.js reconcile + refresh). Centralize them in alm-paths.js as `planDataPath()` / `planHtmlPath()` (these artifacts live at the docs/ ROOT, not docs/alm/) and route all four callers through the helper so the path can't drift. + tests guarding the docs-root invariant. 2. Coverage gap — no automated test for the hook's reconcile spawn. Add run-skill-posttool-validation.test.js: the backstop heals a skipped refresh after an ALM skill, is skipped for non-ALM skills, and is exit-code-neutral (a blocking validator's status is unchanged whether or not the plan exists). 3. Completion edge — pin evaluatePlanCompletion's status gate: the defensive Approved → Completed fallback completes, a Draft plan never auto-completes, and a failed step blocks completion so a failed deploy can't look "done". Full suite: 1246 pass. legacy-compat + alm-lint: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1. hook: forward reconcile failure detail to stderr. The empty JSON.parse catch swallowed spawn errors / timeouts / non-zero exits, and rec.stderr was never forwarded — so the "See stderr for details" pointer was empty. Now track spawnFailed (rec.error / non-zero status / signal) and a parsed flag, report on STDERR only on actual failure (clean runs stay quiet — the hook fires on every Skill use), and forward the child's stderr verbatim where refresh-alm-plan-data.js already writes its per-phase error detail. Stays non-blocking (validator's exit code unchanged). + test: malformed plan → broken reconcile is surfaced AND exit code is unchanged; + quiet-on-success assertion on the heal test. 2. render-alm-plan.js: COMPLETED_AT is optional, not required. Move it out of the required-keys list into its own "optional lifecycle key" note — it's present only once the plan reaches "Completed"; the renderer omits the footer line otherwise. 3. powerpages-hook-utils.js: isAlmPlanSkill JSDoc said @PARAM {string} but the function (and its tests) accept non-strings (null/undefined → false). Widen to {*} and note the contract. Full suite: 1247 pass. legacy-compat + alm-lint: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Draft/Approved/In Execution/Completed status badge sat top-right of the topbar (justify-content:space-between). Move it inline next to the "Generated <timestamp>" sub-line via a new .topbar-sub-row flexbox, so the status reads alongside the plan metadata rather than floating in the corner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rd + in-place approve The Draft/Approved tag was the only PLAN_STATUS transition with no helper behind it — plan-alm Phase 4 set it via hand-authored Edits to the HTML spans AND the JSON. Since the badge + approver are re-derived from docs/.alm-plan-data.json on every render, the manual HTML Edit was non-durable, and a partial write left the plan "approver recorded but PLAN_STATUS=Draft" — stuck forever (check-alm-plan only promotes from Approved; evaluatePlanCompletion ignores Draft). - NEW scripts/lib/set-plan-status.js: single deterministic owner of the creation-time Draft/Approved write. Writes PLAN_STATUS + PLAN_MODE + APPROVED_BY + APPROVAL_DATE together (atomic temp+rename) and optionally re-renders (reuses refresh-alm-plan-data.js findRendererPath/invokeRenderer, now exported). Invariants: only Draft/Approved settable here; Approved requires a non-empty approver; Draft clears the approver; a live (In Execution/Completed) plan is not re-drafted without --force. + 10 unit tests. - validate-plan-alm.js: consistency guard blocks the two half-written states (Draft+approver, Approved+no-approver) for plans created the old way or hand-edited. + 5 tests. - plan-alm Phase 4: both save options now call set-plan-status.js instead of hand-editing the HTML spans + JSON. - plan-alm Phase 1 step 0b: in-place Draft -> Approved fast-path (new gate plan-alm:1.approve-draft) — approve an existing draft without a full re-plan. - approval-gates.md: catalog the new gate. AGENTS.md: document the helper + lifecycle ownership. Version -> 2.6.0 (2.5.0 held by #194). Full suite: 1262 pass. alm-lint: 0 findings. legacy-compat: in sync. End-to-end: stuck state blocks (2) -> set-plan-status -> badge Approved -> validator approves (0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…emediation cmds
Three Copilot findings on validate-plan-alm.js:
1. APPROVED_BY was assumed to be a string — a hand-edited plan-data with a truthy
non-string (number/object) made `.trim()` throw, which escaped runValidation and
SILENTLY APPROVED, bypassing the consistency guard. Coerce with String() first.
Added a regression test (Draft + numeric APPROVED_BY must still block, exit 2).
2-3. The two block-message remediation hints showed a bare
`scripts/lib/set-plan-status.js --status ...` with no `node` and no --projectRoot,
so following them literally failed with "--projectRoot is required". Replace with
a copy/pasteable command matching the skill docs:
`node "${PLUGIN_ROOT}/scripts/lib/set-plan-status.js" --projectRoot "<root>" --status Approved --approver "..." --render`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All five confirmed against source (and PAC 2.8.1) during a data-model/EDM site run of plan-alm: 1. getEnvironmentUrl() grepped "Environment URL:" but `pac env who` on PAC 2.8.x prints the URL under "Org URL:" → returned null, so every caller relying on the pac-env-who fallback (verify-alm-prerequisites without --envUrl, the datamodel / solution / permissions validators, and the #204 declarative-site path) silently failed. Match either label. Extracted parseEnvironmentUrl() (pure) + tests. 2. `pac env list --output json` is INVALID on PAC 2.8.1 (env list accepts only --filter), so ENV_LIST pre-fill never worked. New list-environments.js parses the plain `pac env list` table into JSON {displayName, environmentId, environmentUrl, uniqueName, active}; verified against live PAC (237 rows). Updated all call sites (plan-alm, setup-pipeline, ensure-pipelines-host, cicd-pipeline-patterns.md) + AGENTS.md. Pure parseEnvList() + tests. 3. estimate-solution-size.js hardcoded siteType:'code-site', mislabeling every EDM/data-model site. Now resolves via new --siteType arg (plan-alm passes SITE_TYPE from Phase 1) with a local marker fallback, emitting canonical 'code' | 'data-model' | 'unknown'. resolveSiteType() + tests. 4. plan-alm risk rule #9 emitted a spurious "pipeline host resolution did not run" warning whenever a pipeline already exists (rawDiscovery.hostResolution is legitimately null then, per the Phase 1 Step 12 skip rule). Added a PIPELINE_DONE carve-out so the warning only fires for fresh-pipeline projects. 5. NEW env-match guard (plan-alm Phase 1 step 6b, gate plan-alm:1.env-match, warn+prompt): cross-checks `pac env who` against the project's recorded env URL (powerpages.config.json / .solution-manifest.json) and a websiteRecordId existence probe, so discovery can't silently run against the wrong environment. Catalogued in approval-gates.md §6.1; alm-lint 0 findings. 1279 tests pass (+10). Riding the existing 2.6.0 bump on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename (canonical, build-axis label only — nothing branches on the value, and it
is NOT rendered in the plan HTML; it lives in docs/.alm-plan-data.json + the
estimator's diagnostic field + agent prose):
- detect-project-context.js now emits siteType 'declarative' (was 'data-model');
'data-model' documented as the legacy alias (older plan-data stays equivalent).
- estimate-solution-size.js resolveSiteType returns 'declarative' and NORMALIZES a
legacy 'data-model' arg → 'declarative'; non-canonical values (e.g. an
unsubstituted "{SITE_TYPE}" literal) are ignored in favor of the marker probe.
- plan-alm SKILL.md SITE_TYPE prose + planData comment; AGENTS.md contract;
validation-helpers / check-activation-status comments. Left the unrelated
"Dataverse data model / EDM" prose in pipeline skills untouched. Tests updated.
- Corrected the SKILL.md claim that siteType is "surfaced in the plan" — it isn't.
Final-review fixes (from the 8-angle review pass):
- plan-alm Step 4: stop telling the agent to grep "Environment URL" from
`pac env who` — PAC 2.8.x prints "Org URL:"; read either label (matches the
getEnvironmentUrl fix), and fall back to verify-alm-prerequisites' resolved URL.
- Removed mojibake ("À-côté" → "or wrong") in the Step 6b env-match prose.
- approval-gates.md §6.1 header count 16 → 17 (matches the 17 catalog rows).
- list-environments.js docstring: corrected the `pac admin list` rationale
(admin-scoped + different shape, not "tenant-wide admin-only").
1281 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>
Addresses #202 review: previously the new plan-data was renamed into place BEFORE the renderer ran, so a renderer failure left docs/.alm-plan-data.json updated but docs/alm-plan.html stale while the CLI exited non-zero — a caller committing docs/ would ship a new JSON beside a stale HTML. Now stage plan-data to a temp file, render FROM the temp into a temp HTML, and only swap both into place after a clean render. A render failure discards both staged files, leaving plan-data AND the HTML byte-for-byte unchanged so the caller can retry cleanly. Added a regression test (failing renderer → both files untouched, no leftover temps). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
T-Nid (tyaginidhi)
changed the title
…s normalized, not echoed) Addresses #202 review: the comment said a caller passing the legacy 'data-model' value is "echoed unchanged", but the code normalizes 'data-model' -> 'declarative'. Corrected the docstring to say it is normalized so the output is always canonical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
T-Nid (tyaginidhi)
enabled auto-merge (squash)
June 22, 2026 13:29
…fter activation Two gaps surfaced in EDM-site deploy testing (both SKILL.md-only — the helpers already support these paths): Fix #5 (Phase 1 Step 1): verify-alm-prerequisites.js defaulted to PAC's org context (`pac env who`), which isn't guaranteed to match the project — and with a stale/ambiguous context (duplicate active pac auth profiles) it failed with a misleading "PAC CLI is not authenticated". Now resolve the project's recorded env URL first (.solution-manifest.json top-level `environmentUrl`, else powerpages.config.json `environmentUrl`) and pass it as --envUrl so the gate is deterministic; fall back to the pac-context default only when neither file records one. (verify-alm-prerequisites already skips getEnvironmentUrl() when --envUrl is supplied — no code change.) Progress-tracking row updated to match. Fix #7 (Phase 7.7): the Phase 7.5b plan refresh runs BEFORE activation is resolved, so the plan's "Activate site in {stage}" step stayed pending even after last-deploy.json recorded activationStatus. Added a second refresh-alm-plan-data.js --phase deploy-pipeline call right after the marker is patched; its existing auto-complete logic flips the activate step to completed when activationStatus === "Activated" (a deferred "Pending" correctly leaves it pending). Soft no-op when no plan exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Priyanshu Agrawal (priyanshu92)
approved these changes
Jun 22, 2026
…nv-var verify match Four gaps from a live code-site Dev→Staging run (CitizenServices portal): Gap C (priority) — env drift: ALM skills trusted the ambient PAC env when --envUrl was omitted, with no cross-check against the project's env. The Org-URL parser fix (cb68f86) made this worse — a drifted PAC context now resolves and proceeds silently instead of failing loudly. Added an opt-in `--expectedEnvUrl` to verify-alm-prerequisites.js that compares the resolved env (origin-only) and HARD-STOPS on mismatch with a "run pac env select" message. Wired deploy-pipeline Phase 1 to assert against the project's configured env URL (.solution-manifest.json / powerpages.config.json) — chosen over pinning --envUrl because the assertion also protects later PAC-CLI ops (pac pipeline deploy), not just the Dataverse calls. Documented as the recommended guard in AGENTS.md. Tests added. Gap B (doc) — deploy-pipeline Phase 3 only documented the 404 fallback for RetrieveDeploymentPipelineInfo; a 400 (observed live) was wrongly routing to the PAC-CLI path even though ValidatePackageAsync still works. Added an inline branch: non-404 errors use the marker's sourceDeploymentEnvironmentId and CONTINUE the normal ValidatePackageAsync flow (do NOT set VALIDATE_PACKAGE_UNAVAILABLE). Gap D (enhancement) — plan-alm Phase 2 Q3 now emits a soft Risks warning when a chosen target stage has no matching stage on an existing pipeline (docs/alm/last-pipeline.json), e.g. plan says "Dev→Production" but the live pipeline only has "Deploy to Staging". Informational only; no new prompt. Gap E — verify-env-var-values.js returned total:0 for a String override: NOT a type filter (it counts all EnvironmentVariables[]), but a stage-label mismatch — deploy-pipeline passes "Deploy to Staging" while the settings file is keyed "Staging", and the match was exact. Now normalized via the shared normalizeStageLabel (exported from refresh-alm-plan-data.js — the documented single source for this), so the missed code path matches like every other consumer. Test added. 1286 tests pass (+4). alm-lint 0, legacy-compat in sync, version-check pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- set-plan-status.js: a crash between the HTML-then-JSON renames leaves "HTML ahead of JSON" (new HTML, old JSON), not "JSON behind HTML". Reworded to the correct direction (it's the inverse of the original pre-atomic bug; benign + self-healing). - estimate-solution-size.js: the marker probe was said to be "documented in CLAUDE.md"; point at the canonical source (detect-project-context.js, also in AGENTS.md) instead. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Contributor
Author
Final pre-approval review (4-angle pass)Ran a 4-angle review (correctness, cross-file/circular-deps, tests+conventions, SKILL.md coherence) over the full net diff. Outcome: Fixed in df4265b:
Verified clean (no change needed): no circular Out of scope (pre-existing code this PR doesn't touch — filed mentally for follow-up): Status: 1288 tests pass, alm-lint 0 findings, legacy-compat in sync, version-check passes. Ready for review/approval. |
Priyanshu Agrawal (priyanshu92)
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR started as the deterministic plan Draft/Approved status fix and grew to fold in reliability fixes surfaced by a real EDM-site
plan-alm/deploy-pipelinerun, plus asiteTyperename. All onplan-alm/deploy-pipeline/ their shared ALM libs. v2.6.0 (2.5.0 landed in #194).1. Deterministic plan status (Draft / Approved)
The plan-status tag was the only
PLAN_STATUStransition with no deterministic helper — plan-alm Phase 4 set it via hand-authoredEdits to the HTML spans and the JSON. Because the badge +approved-byspans are re-derived fromdocs/.alm-plan-data.jsonon every render, the manual HTML edit was non-durable, and a partial write (approver in plan-data butPLAN_STATUSleftDraft) left the plan shown-as-approved but stuck on Draft forever (check-alm-plan.jsonly promotes fromApproved). Reported symptom: "I'm marked as approver but the plan never moved to Approved."scripts/lib/set-plan-status.js— single deterministic owner of the creation-timeDraft/Approvedwrite. WritesPLAN_STATUS+PLAN_MODE+APPROVED_BY+APPROVAL_DATEtogether, and--renderis atomic across plan-data + HTML (renders from a temp, swaps both only on success; a render failure leaves both files untouched). Invariants: onlyDraft/Approvedsettable here;Approvedrequires a non-empty--approver;Draftclears the approver; a live plan isn't re-drafted without--force.APPROVED_BYis string-coerced so a hand-edited non-string can't throw past the guard.validate-plan-alm.jsconsistency guard — blocks the two half-written states (Draft+approver,Approved+no-approver), with copy/pasteable remediation commands.Draft → Approvedfast-path (gateplan-alm:1.approve-draft) — approve an existing draft with no re-plan.Lifecycle ownership
Draft/Approvedset-plan-status.js(this PR)Approved→In Executioncheck-alm-plan.jsIn Execution/Approved→Completedrefresh-alm-plan-data.js2. ALM reliability fixes (from live EDM + code-site Dev→Staging runs)
getEnvironmentUrl()matched onlyEnvironment URL:but PAC 2.8.x printsOrg URL:→ returnednull, breaking thepac env whofallback for 5 callers. Now matches either label (extracted, testedparseEnvironmentUrl).pac env list --output jsonis invalid on PAC 2.8.1 (only--filter) → ENV_LIST pre-fill never worked. NEWscripts/lib/list-environments.jsparses the plainpac env listtable into JSON; all call sites updated.siteType: 'code-site', mislabeling EDM sites → resolved via a--siteTypearg + local marker fallback.PIPELINE_DONEcarve-out.plan-alm:1.env-match, warn+prompt): cross-checkspac env whoagainst the project's recorded env URL + awebsiteRecordIdexistence probe.verify-alm-prerequisites.js --expectedEnvUrlenv-drift HARD-STOP. Since theOrg URL:parser fix (Update README.md for clarity and structure #1) made a drifted PAC context resolve silently (the old parse-miss had been an accidental safety net), an ALM op could target the wrong env (e.g. PROD). The opt-in flag asserts the resolved env (origin-only) matches the project's and hard-stops on mismatch.deploy-pipelinePhase 1 now asserts against.solution-manifest.json/powerpages.config.json— chosen over pinning--envUrlbecause the assertion also protects later PAC-CLI ops (pac pipeline deploy).deploy-pipelinePhase 7.7 re-refreshes the ALM plan after activation resolves, so the "Activate site" step auto-completes (the 7.5b refresh ran beforeactivationStatuswas set).deploy-pipelinePhase 3 distinguishes a 400 from a 404 onRetrieveDeploymentPipelineInfo: a non-404 error continues the normalValidatePackageAsyncflow instead of wrongly routing to the PAC-CLI path.docs/alm/last-pipeline.json).verify-env-var-values.jsstage-label match now uses the sharednormalizeStageLabel, so--stageLabel "Deploy to Staging"matches a"Staging"-keyed settings file (was returningtotal:0, a silent no-op).3.
siteTyperename:data-model→declarativeThe build-axis label is now
declarative(was the compat-nameddata-model) indetect-project-context.js,estimate-solution-size.js(resolveSiteTypealso normalizes the legacydata-modelalias and rejects non-canonical input), plan-alm SKILL.md, AGENTS.md, and tests. Nothing branches on the value and it is not rendered in the plan HTML (it lives indocs/.alm-plan-data.json+ the estimator's diagnostic field), so the rename is safe and older plan-data carryingdata-modelstays equivalent. The unrelated Dataverse data model / EDM prose is left untouched.Verification
lint-skills-alm.js0 findings;validate-legacy-compatibility.jsin sync;ensure-skill-version-check.jspasses.🤖 Generated with Claude Code