Skip to content

Approval Gates: extend to non-ALM skills + plugin-wide hard-fail lint - #170

Merged
T-Nid (tyaginidhi) merged 12 commits into
mainfrom
users/nityagi/ApplyApprovalGatesPattern
Jun 12, 2026
Merged

Approval Gates: extend to non-ALM skills + plugin-wide hard-fail lint#170
T-Nid (tyaginidhi) merged 12 commits into
mainfrom
users/nityagi/ApplyApprovalGatesPattern

Conversation

@tyaginidhi

@tyaginidhi T-Nid (tyaginidhi) commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends the Approval Gate pattern from §6.1–§6.12 (12 ALM skills) to cover all 25 user-invocable power-pages skills, flips lint enforcement to hard-fail across the whole plugin, and closes 16 pre-existing catalog orphans.

  • 45 new gates + 9 not-a-gates in 12 non-ALM skills (create-site, deploy-site, add-server-logic, add-cloud-flow, setup-auth, integrate-webapi, setup-datamodel, add-sample-data, add-seo, create-webroles, audit-permissions, integrate-backend) — catalogued in §6.13–§6.24.
  • 3 new gates + 2 not-a-gates in the 4 security skills picked up during rebase (manage-firewall, manage-headers, scan-site, security-review) — catalogued in §6.24a + §6.25–§6.28. §6.24a documents the runtime-loop prompt convention these skills use.
  • 4 new gates in the add-ai-webapi skill introduced by PR [Pages] Add skill for ai webapi #144 — catalogued in §6.29. Covers: iteration-mode batched deploy+commit prompt (iter.deploy-commit), skip-web-role trade-off confirmation (4.2.skip-webrole), Phase 5 commit offer (5.5.commit), Phase 6 Summarization/* settings commit offer (6.4.commit).
  • scripts/lint-skills-alm.js warn-only branch removed. severityForSkill() (now SKILL_SEVERITY constant) returns 'error' for every skill. Test updated. Dead warnings-only branch in main() removed. m <= promptLine tightened to m < promptLine. CATALOG_GATE_ID_PATTERN made case-insensitive to align with GATE_MARKER_PATTERN. Two new rules added: CATALOG-row-must-have-marker (reverse check — catalog rows must have a matching SKILL.md marker) and GATE-prose-block-required (every marker must be followed within 10 lines by a 🚦 sentinel). Lint now enforces 7 gate-related rules plugin-wide.
  • AGENTS.md Key Patterns generalized — Approval Gate convention applies plugin-wide; new skills must extend the catalog in the same PR that introduces a prompt. PLUGIN_DEVELOPMENT_GUIDE.md:273 updated to list all seven enforced rules.
  • 16 pre-existing ALM catalog orphans closed — see commit 1011030 for the per-row resolution. Some got new markers (test-site:5.5.form-submit, setup-pipeline:4.3.name-conflict, etc.); others were removed from the catalog as redundant or unimplemented (deploy-pipeline:6.1.pac-fallback-consent, configure-env-variables:6.confirm-matrix, force-link-environment:2.host-fallback/2.dev-fallback).
  • readSettingsFile bug fix — now handles all three deployment-settings.json shapes (top-level / Stages: [] array / stages: {} keyed object), with preserveAllStages mode to validate every per-stage entry. Fixes a silent validation pass for the Microsoft deployment-settings/2024 schema shape.
  • Markdown rendering polish — 6 spots where gate marker blocks were splitting numbered lists are fixed (marker now precedes the list). Stray ``` tokens at add-cloud-flow:346 and `integrate-backend:271` (from PR Power Pages ALM: 10 new skills + 45 helpers + 6 reference docs for site-to-pipeline lifecycle #167) removed.
  • CI workflows kept deliberately separate — added header comments to .github/workflows/power-pages-alm-lint.yml and power-pages-script-tests.yml explaining why.

Test plan

  • node plugins/power-pages/scripts/lint-skills-alm.jsalm-lint: 0 findings
  • node --test plugins/power-pages/scripts/tests/*.test.js → 1108/1108 passing
  • Catalog completeness — every <!-- gate: --> marker in SKILL.md has a backticked catalog row in references/approval-gates.md; the previous orphan ALM rows are resolved (marker added or row removed).
  • Markdown polish — 6 list-breakage spots fixed; 2 stray ``` tokens removed.
  • Live exercise of /power-pages:plan-alm end-to-end against a real tenant to confirm the new plan-alm:2.q4-host marker fires.
  • Live exercise of /power-pages:ensure-pipelines-host against a NoHost tenant to confirm the 3.C / 4.0 / 4.A / 4.C markers fire as expected.

Notes

This is a draft PR while live-tenant validation is pending. The structural changes (markers, catalog, lint) are complete and verified; the runtime gate firing remains to be confirmed against real flows.

🤖 Generated with Claude Code

T-Nid (tyaginidhi) and others added 4 commits May 26, 2026 17:05
Extend the catalog in references/approval-gates.md (§6.13-§6.24)
to cover the 12 non-ALM skills: create-site, deploy-site,
add-server-logic, add-cloud-flow, setup-auth, integrate-webapi,
setup-datamodel, add-sample-data, add-seo, create-webroles,
audit-permissions, integrate-backend. 45 gates + 9 not-a-gates
inserted as HTML comment markers + human 🚦 blocks above each
AskUserQuestion call.

Flip lint to hard-fail across the whole plugin -- the ALM-only
warn-only branch in scripts/lint-skills-alm.js has been removed.
severityForSkill() now returns 'error' for every skill. Update
the corresponding test in scripts/tests/lint-skills-alm.test.js.

AGENTS.md Key Patterns generalized: the Approval Gate convention
applies plugin-wide. New skills must extend the catalog in the
same PR that introduces an AskUserQuestion, or CI blocks the PR.

Workflow files get header comments explaining why power-pages-
alm-lint and power-pages-script-tests are deliberately separate
(policy enforcement vs functional tests; single-OS fast-fail vs
cross-OS matrix).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rebase onto origin/main brought in PR #151 which added 4 new
power-pages skills: manage-firewall, manage-headers, scan-site,
security-review. With the warn-only lint branch removed in the
previous commit, those skills now had unmarked AskUserQuestion
prompts and broke CI.

These skills use a different prompt shape — most calls happen
inside a runtime "recommend then ask" loop described in prose,
not at statically-locatable call sites. The catalog now has a
new §6.24a section explaining the convention for runtime-loop
skills: meta-mention sections get not-a-gate markers; concrete
call sites get full gate markers.

Catalog rows added:
- §6.25 manage-firewall — 1 not-a-gate (option-rules-meta)
- §6.26 manage-headers — 1 gate (per-finding loop, plan)
- §6.27 scan-site — 1 not-a-gate (option-rules-meta)
- §6.28 security-review — 2 gates (2.1 goal, 5.3 next-action)

Total: 3 gates + 2 not-a-gates. Lint passes (0 findings).

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

Addresses the 12 findings from the pre-PR code review. Fixes ordered by
severity:

MUST FIX:
- PLUGIN_DEVELOPMENT_GUIDE.md: drop the stale "non-ALM warn-only" text
  on line 273 and mirror the v3 hard-fail language. AGENTS.md tells new
  authors to read this file first, so the stale doc would have misled
  them.

MARKDOWN RENDERING:
- audit-permissions, create-site (4 places), integrate-webapi: move
  gate marker blocks ABOVE the ordered list intro sentence so they
  don't sit between numbered items. CommonMark/GFM split a list at an
  unindented HTML comment + blockquote, restarting numbering. The
  rendered numbering on GitHub was 1,2,3,1,2 instead of 1..N.

PRE-EXISTING FILE BUGS:
- Stray ``` token at add-cloud-flow:346 and integrate-backend:271
  (both from PR #167) — removed. These broke fence parity for
  everything below them.

LINT CLEANUPS:
- severityForSkill() is now a constant (SKILL_SEVERITY); unused
  skillName parameter and void no-op removed; call sites simplified.
- Dead warnings-only exit branch in main() removed — no code path
  produces severity 'warning' post-v3.
- Tightened checkSectionPairing: m < promptLine (strictly before),
  not m <= promptLine, so a marker on the SAME line as the prompt no
  longer trivially satisfies the rule.
- CATALOG_GATE_ID_PATTERN now case-insensitive [A-Za-z] to align
  with GATE_MARKER_PATTERN — future CamelCase / underscored skill
  names won't break GATE-must-be-in-catalog.

ALM CATALOG ORPHANS:
- 16 catalog gate rows previously lacked SKILL.md markers (introduced
  pre-v3). Resolved in two ways:
  - Added markers where the prompt call site is concrete:
    test-site:5.5.form-submit, setup-pipeline:4.3.name-conflict +
    6b.v2-migration, export-solution:2.identify + 3.overwrite,
    force-link-environment:2.host-url + 2.dev-env, ensure-pipelines-
    host:3.C.host-type + 3.C.env-pick + 4.0.pre-call + 4.A.pre-call
    + 4.sandbox-confirm + 4.C.ppac-done, plan-alm:2.q4-host (renamed
    from q4-stage-env to match the actual host-selection prompt).
  - Removed orphan rows that described gates not implemented as
    separate AskUserQuestion calls: deploy-pipeline:6.1.pac-fallback-
    consent (covered by 6.0.final-consent prose), configure-env-
    variables:6.confirm-matrix (covered by 2.selection's per-stage
    matrix), force-link-environment:2.host-fallback + 2.dev-fallback
    (redundant — the gate IDs above cover the fallback path).

PROSE ANCHORS:
- setup-auth Phase 1.4 prompt prose now includes the literal
  backticked AskUserQuestion + colon anchor required by
  PROMPT_LINE_PATTERN. Removing the gate marker would now actually
  trigger GATE-must-have-marker.

VERIFIED: lint reports 0 findings; 984/984 plugin tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverts an accidental scope expansion, adds two new lint rules, closes
the runtime-loop coverage hole, renames the awkward 'Blast radius if
skipped' field, and removes dead module-level code. All 9 feedback
items from the pre-PR review are addressed.

LINT:
- CATALOG_GATE_ID_PATTERN: revert underscore that was silently added —
  was [A-Za-z0-9_-]*, now [A-Za-z0-9-]* matching the original kebab-only
  intent. The case-insensitivity change to [A-Za-z] (from [a-z]) is
  kept, with a comment explaining the asymmetric grammar with
  GATE_MARKER_PATTERN.
- New rule: CATALOG-row-must-have-marker — reverse check (catalog row →
  SKILL.md marker). Catches the orphan-row class of bug v3 closed by
  hand. Parses §6 catalog tables for rows tagged 'gate' and verifies
  each ID has a matching <!-- gate: ID --> marker in some SKILL.md.
- New rule: GATE-prose-block-required — every gate marker must be
  followed within 10 lines by a line carrying the 🚦 sentinel. Catches
  prose-block deletion without forcing structural rewrite of 80+ legacy
  v2 single-line markers.
- Tightened m < promptLine (strict precede) with a new test asserting
  marker-on-prompt-line fails the rule.
- Removed dead ALM_SKILLS export + its membership test — no downstream
  consumer existed.
- Simplified SKILL_SEVERITY comment (the wishful 'future per-skill
  policy' framing).

CATALOG:
- §3.2 plan-vs-consent: added a paragraph explaining why deploy-dispatch
  prompts ('Deploy now?') are tagged plan when the destructive consent
  lives inside the dispatched skill's own gate. Resolves the
  create-site:8.deploy (plan) vs deploy-site:3.confirm-env (consent)
  apparent inconsistency.
- §6.4 / §6.7 / §6.9: removed archaeological commentary about merged /
  removed rows. Git blame has it.
- §6.24a: rewritten to reflect that the runtime-loop coverage hole is
  now CLOSED — manage-firewall and scan-site got real call-site
  anchors.
- §10: added cross-plugin lint TODO for shared/skills/report-issue/;
  expanded the v3 PR changelog with the new lint rules + field rename.
- §11: added phase-number-drift convention (any SKILL.md phase renumber
  must grep the catalog), runtime-loop coverage caveat (now mostly
  resolved), and lint-prose-block weakness (only 🚦 enforced, not the
  3 structured labels).

RUNTIME-LOOP ANCHORS (F13 — closes the coverage hole):
- manage-firewall: new gate manage-firewall:3.action-choice (plan, at
  '### Default approach') + manage-firewall:3.execute-consent (consent,
  at '### Plan-validate-execute'). The destructive WAF mutations
  (enable/disable/add/update/delete rule) are now lint-anchored.
- scan-site: new gate scan-site:3.action-choice (plan, at '### Default
  approach'). The scan-trigger decision is now lint-anchored.
- Both prose blocks updated to include the literal 'AskUserQuestion:'
  string that PROMPT_LINE_PATTERN requires.

FIELD RENAME (F14):
- 'Blast radius if skipped:' → 'Why we ask:' across all 60 prose blocks
  + §4.1 catalog template. The old label suggested the gate's purpose
  was to describe Cancel state; the actual content explains why the
  gate prompts in the first place.

VERIFIED: lint reports 0 findings (with new reverse + prose-block
rules active); 984/984 plugin tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tyaginidhi
T-Nid (tyaginidhi) marked this pull request as ready for review May 26, 2026 12:50
@tyaginidhi
T-Nid (tyaginidhi) requested a review from a team as a code owner May 26, 2026 12:50
Copilot AI review requested due to automatic review settings May 26, 2026 12:50
@tyaginidhi

Copy link
Copy Markdown
Contributor Author

Addressed all 9 review items in commit 8d02547. Summary:

Lint hardening:

  • ✅ Reverted accidental underscore widening in CATALOG_GATE_ID_PATTERN (kept case-insensitivity).
  • ✅ Added CATALOG-row-must-have-marker reverse rule — catches orphan catalog rows that v3 closed by hand. Same machinery, inverted check.
  • ✅ Added GATE-prose-block-required — every marker must be followed by a 🚦 sentinel within 10 lines. Minimum-viable check against prose-block deletion.
  • ✅ Strict precede: m < promptLine (was <=). New test pins the behavior.
  • ✅ Removed dead ALM_SKILLS export + its membership test.
  • ✅ Simplified the wishful "future per-skill policy" comment.

Catalog clarifications:

  • ✅ §3.2 plan: paragraph explaining why deploy-dispatch prompts are plan (the destructive consent lives inside the dispatched skill).
  • ✅ §11 caveats: phase-number drift convention; lint-prose-block weakness (only 🚦 enforced, not the 3 structured labels); runtime-loop coverage hole status.
  • ✅ §10: cross-plugin lint TODO for shared/skills/report-issue/.
  • ✅ Removed archaeological commentary in §6.4 / §6.7 / §6.9.

Runtime-loop coverage hole — CLOSED:

  • manage-firewall:3.action-choice (plan) + manage-firewall:3.execute-consent (consent) — destructive WAF mutations are now lint-anchored.
  • scan-site:3.action-choice (plan) — scan-trigger decision is now lint-anchored.
  • §6.24a updated to reflect resolution.

Field rename:

  • Blast radius if skipped:Why we ask: across all 60 prose blocks + §4.1 template.

Verified: node plugins/power-pages/scripts/lint-skills-alm.js → 0 findings (with new reverse + prose-block rules active); 984/984 plugin tests pass.

Still pending (post-merge):

  • Live-tenant exercises for plan-alm:2.q4-host (renamed) and the new ensure-pipelines-host:3.C.* / 4.0 / 4.A / 4.C markers. The structural review is unblocked; runtime validation happens against a real tenant before/after merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the Power Pages “Approval Gates” pattern and catalog coverage from ALM-only skills to all user-invocable power-pages skills, and updates the linter to hard-fail plugin-wide when gate markers/catalog drift occurs.

Changes:

  • Added <!-- gate: ... --> / <!-- not-a-gate: ... --> markers (plus 🚦 prose blocks) across non-ALM and security skills to fully cover AskUserQuestion call sites.
  • Updated scripts/lint-skills-alm.js to enforce gate/catalog rules at error severity for every skill, and added new drift-prevention rules.
  • Updated reference docs and contributor guidance (approval-gates.md, AGENTS.md, PLUGIN_DEVELOPMENT_GUIDE.md) plus workflow headers to reflect plugin-wide enforcement.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
plugins/power-pages/skills/test-site/SKILL.md Adds a consent gate for live form submission testing.
plugins/power-pages/skills/setup-pipeline/SKILL.md Adds planning gates for pipeline name conflicts and v2→v3 manifest migration decisions.
plugins/power-pages/skills/setup-datamodel/SKILL.md Adds plan gates for model source selection and pre-write approval.
plugins/power-pages/skills/setup-auth/SKILL.md Adds multiple plan gates for deploy prerequisites, web role creation, requirements, plan approval, and deploy dispatch.
plugins/power-pages/skills/security-review/SKILL.md Adds plan gates for goal selection and post-report next action.
plugins/power-pages/skills/scan-site/SKILL.md Anchors the action-choice prompt as a plan gate; adds a not-a-gate marker for meta option rules.
plugins/power-pages/skills/plan-alm/SKILL.md Adds a plan gate for host environment selection (Q4 host).
plugins/power-pages/skills/manage-headers/SKILL.md Adds a plan gate for the per-finding accept/customize/skip loop.
plugins/power-pages/skills/manage-firewall/SKILL.md Adds a plan gate for action selection and a consent gate before destructive firewall mutations; adds not-a-gate meta marker.
plugins/power-pages/skills/integrate-webapi/SKILL.md Adds plan gates for table confirmation, deploy prerequisite, permissions source/approval, and deploy dispatch.
plugins/power-pages/skills/integrate-backend/SKILL.md Adds a plan-approval gate before dispatching to child integration skills; removes stray markdown token.
plugins/power-pages/skills/force-link-environment/SKILL.md Converts prior not-a-gate placeholders into explicit plan gates for host/dev env resolution prompts.
plugins/power-pages/skills/export-solution/SKILL.md Adds plan gates for solution identification (when manifest missing) and overwrite/name decision.
plugins/power-pages/skills/ensure-pipelines-host/SKILL.md Adds plan/consent/progress gates for NoHost routing, provisioning confirmations, sandbox reconfirm, and PPAC manual completion.
plugins/power-pages/skills/deploy-site/SKILL.md Adds consent/plan gates for environment confirmation, multi-project selection, optional audit, activation prompt, cache restart, and blockedattachments change.
plugins/power-pages/skills/create-webroles/SKILL.md Adds plan gates for deploy prerequisite, role selection, and deploy dispatch.
plugins/power-pages/skills/create-site/SKILL.md Adds plan gates for purpose/requirements/plan approval/review/deploy dispatch; adds not-a-gate marker for protocol prose.
plugins/power-pages/skills/audit-permissions/SKILL.md Adds a plan gate for offering auto-fixes after report generation.
plugins/power-pages/skills/add-server-logic/SKILL.md Adds plan gates for deploy prerequisite, custom actions usage, Key Vault decision, plan approval, no-vaults branch, frontend scope, deploy/test dispatch; adds not-a-gate markers for sub-prompts.
plugins/power-pages/skills/add-seo/SKILL.md Adds not-a-gate markers for data-gathering prompts and a plan-approval gate before writing SEO artifacts.
plugins/power-pages/skills/add-sample-data/SKILL.md Adds plan gates for table selection and record count selection.
plugins/power-pages/skills/add-cloud-flow/SKILL.md Adds plan gates for deploy prerequisite, flow selection, plan approval, deploy/test dispatch; removes stray markdown token; adds not-a-gate marker for scenario clarification.
plugins/power-pages/scripts/tests/lint-skills-alm.test.js Updates tests to reflect plugin-wide hard-fail severity and stricter “marker must precede prompt line” behavior.
plugins/power-pages/scripts/lint-skills-alm.js Removes ALM/non-ALM severity split; tightens pairing logic; makes catalog parsing more permissive; adds reverse catalog completeness and prose-block drift checks; simplifies output to error-only.
plugins/power-pages/references/approval-gates.md Expands catalog to all skills, updates terminology/landing history, and documents new gate coverage patterns.
plugins/power-pages/PLUGIN_DEVELOPMENT_GUIDE.md Updates contributor guidance to reflect plugin-wide gate enforcement (no ALM carve-out).
plugins/power-pages/AGENTS.md Updates key patterns/reference doc description to reflect plugin-wide approval gate cataloging and enforcement.
.github/workflows/power-pages-script-tests.yml Adds header comments explaining workflow separation and intent.
.github/workflows/power-pages-alm-lint.yml Adds header comments explaining workflow separation and intent.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/power-pages/scripts/lint-skills-alm.js
Comment thread plugins/power-pages/references/approval-gates.md Outdated
Comment thread plugins/power-pages/AGENTS.md Outdated
Comment thread plugins/power-pages/PLUGIN_DEVELOPMENT_GUIDE.md Outdated
Comment thread plugins/power-pages/scripts/tests/lint-skills-alm.test.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 26, 2026 13:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

plugins/power-pages/scripts/lint-skills-alm.js:93

  • The usage header still describes a warnings-vs-errors split (exit 0 when only warnings; stdout lists warnings). In v3, main() now treats every finding as severity: 'error' and always exits 1 if there are any findings. Please update this usage comment to match the current behavior (or reintroduce the warning path).
// Usage:
//   node scripts/lint-skills-alm.js [--plugin-root <path>]
//   Exit 0 when no findings (or only warnings); exit 1 when at least one
//   finding has severity 'error'. stderr lists errors; stdout lists warnings.
//

Comment thread plugins/power-pages/AGENTS.md Outdated
Comment thread plugins/power-pages/PLUGIN_DEVELOPMENT_GUIDE.md Outdated
Comment thread plugins/power-pages/references/approval-gates.md Outdated
Comment thread plugins/power-pages/scripts/lint-skills-alm.js Outdated
… doc consistency

Addresses 10 review-skill findings + 9 Copilot inline comments + the
Copilot Autofix on the rule-count wording.

## Lint script (scripts/lint-skills-alm.js)

LINT REGEX:
- CATALOG_GATE_ROW_PATTERN now tolerates up to 3 leading spaces before the
  pipe (GFM table indentation tolerance). Was `^\|`, now `^\s{0,3}\|`.
  Without this, a future markdown reformat that nests §6 tables under a
  parent list silently disables orphan detection for those rows.

GATE-prose-block-required:
- Window now INCLUDES the marker's own line (slice(startIdx, startIdx+10),
  was slice(startIdx+1, startIdx+11)). Single-line compact-style markers
  with marker + 🚦 on one line now pass.
- The 🚦 search now skips code-fence regions. Was a plain `.includes('🚦')`
  per line; now pre-computes per-line fence state across the full file
  and ignores 🚦 inside ```...``` blocks. Closes the "future contributor
  deletes the real Gate prose block but leaves an example 🚦 in a bash
  example within the window" silent-pass case.

CATALOG-row-must-have-marker:
- Docstring updated. Previously claimed "Waivable: yes — inline
  <!-- alm-lint-ignore: ... --> in the catalog row's section". The
  implementation never called extractIgnores on the catalog file, so
  inline-ignore never worked. New docstring says .almlintignore allowlist
  is the only suppression mechanism (the rule operates on the catalog as
  a whole, not per SKILL.md, so per-row inline ignore would be confusing
  to model). Matches actual behavior.

CLEANUPS:
- Removed dead `skillNameFromFile()` function. No callers since
  severityForSkill was deleted; not in module.exports.
- Removed `SKILL_SEVERITY` constant. Half-applied (2 of 9 severity:
  sites used it) — inlined `'error'` everywhere for consistency.
- Updated Usage header comment to match v3 exit-code behavior.
- main() now concatenates findings into one string + single
  process.stderr.write, then sets process.exitCode instead of calling
  process.exit. Prevents stderr-truncation on Windows when piping to a
  log file with large finding counts.

## Tests (scripts/tests/lint-skills-alm.test.js)

Added 7 new tests for the two rules introduced in commit 8d02547 that
shipped with zero coverage:

CATALOG-row-must-have-marker:
- fires when a catalog gate row has no SKILL.md marker
- passes when every gate row has a marker
- skips not-a-gate rows (no marker required for them)
- tolerates leading whitespace on table rows (GFM compliance)

GATE-prose-block-required:
- fires when marker has no 🚦 within 10 lines
- passes when 🚦 sentinel follows within window
- ignores 🚦 inside a fenced code block
- tolerates 🚦 on the same line as the marker (single-line style)

Test count: 45 → 52 (+7). Plugin-wide: 984 → 992.

## Docs

approval-gates.md / AGENTS.md / PLUGIN_DEVELOPMENT_GUIDE.md:
- Updated "five GATE rules" → "seven gate-related rules" (Copilot's
  finding). Lists all 7 enforced rules now. Includes the v3 additions
  CATALOG-row-must-have-marker and GATE-prose-block-required.

approval-gates.md §9 Decisions row 4:
- Marked the v2 "ALM: hard-fail. Non-ALM: warn-only" decision as
  superseded by §10. The §10 history correctly captures the v2→v3
  transition but §9 read as a current recommendation in isolation.

Verified: lint 0 findings; 992/992 tests pass.

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

Copy link
Copy Markdown
Contributor Author

Addressed all 10 review-skill findings + 9 Copilot inline comments in commit `b42ddc6`. Also picked up Copilot Autofix commit `f4c8ced` on the rule-count wording (rebased onto it, kept the more detailed version).

Lint script

Robustness:

  • ✅ `CATALOG_GATE_ROW_PATTERN` now tolerates up to 3 leading spaces (GFM table indentation). Was `^|`, now `^\s{0,3}|`.
  • ✅ `GATE-prose-block-required` window now includes the marker's own line — single-line compact markers pass.
  • ✅ `GATE-prose-block-required` 🚦 search is now code-fence-aware — a literal 🚦 inside a ```bash``` example no longer satisfies the rule.
  • ✅ `main()` uses single `process.stderr.write` + `process.exitCode` (was multiple sync writes + `process.exit`) — fixes potential stderr truncation on Windows piped output.

Docstring fix (Copilot inline #1):

  • ✅ `CATALOG-row-must-have-marker` docstring updated. The old text promised inline `` waiver that the implementation never honored. New docstring states `.almlintignore` is the only suppression mechanism (consistent with actual behavior).

Cleanups:

  • ✅ Removed dead `skillNameFromFile()` function (no callers).
  • ✅ Removed half-applied `SKILL_SEVERITY` constant — inlined `'error'` everywhere.
  • ✅ Updated stale Usage header comment (Copilot inline Update skills and marketplace #2) to match v3 exit-code behavior.

Tests (Copilot inline #3 + review-skill finding #1)

Added 7 new tests for the two rules introduced in commit `8d02547` that shipped with zero coverage:

CATALOG-row-must-have-marker:

  • ✅ fires when a catalog gate row has no SKILL.md marker
  • ✅ passes when every gate row has a marker
  • ✅ skips not-a-gate rows
  • ✅ tolerates GFM leading whitespace on table rows

GATE-prose-block-required:

  • ✅ fires when marker has no 🚦 within 10 lines
  • ✅ passes when 🚦 sentinel follows within window
  • ✅ ignores 🚦 inside a fenced code block
  • ✅ tolerates 🚦 on the same line as the marker

Test count: 45 → 52 (+7). Plugin-wide: 984 → 992. Lint: 0 findings.

Docs (Copilot inline #4#7)

  • ✅ `approval-gates.md` intro, `AGENTS.md` ~244, `PLUGIN_DEVELOPMENT_GUIDE.md` ~273: updated "five GATE rules" → "seven gate-related rules" and enumerated all 7 (Copilot Autofix `f4c8ced` covered the first; I expanded the wording in each location to list the new rules with brief inline descriptions).
  • ✅ `approval-gates.md` §9 Decisions row 4: marked the v2 "ALM: hard-fail. Non-ALM: warn-only" decision as `[v2 — superseded by §10]`. The history was correct in §10 but §9 read as a current recommendation.

Verified

```
node plugins/power-pages/scripts/lint-skills-alm.js → 0 findings
node --test plugins/power-pages/scripts/tests/ → 992 / 992 passing
```

Discovered while validating PR #170 against the real Citizens portal site
(C:\Projects\Citizens portal). The site's deployment-settings.json uses
the Microsoft-standard 2024 schema shape:

  {
    "$schema": "https://schemas.microsoft.com/power-platform/deployment-settings/2024",
    "stages": {
      "Deploy to Staging": {
        "EnvironmentVariables": [...]
      }
    }
  }

— a keyed OBJECT of stages. readSettingsFile only handled two shapes:
1. Top-level `EnvironmentVariables: []` (single-stage)
2. `Stages: []` array with `{ Name, EnvironmentVariables }` entries

The keyed-object shape returned 0 entries, which meant
validate-deployment-settings.js silently passed even on known-broken
values like `@KeyVault(vaultName=...;secretName=...)`.

Real-world evidence: the Citizens portal's docs/alm/last-deploy.json
records a failed deploy attempt (2026-05-21) where deployment-settings.json
contained `@KeyVault(...)` for c311_api_secret. The pre-deploy validator
"validation passed (validation does not check Secret reference format)"
— because the parser couldn't read the file. The deploy then waited ~4h
in the host queue before failing with `ImportAsHolding failed: The value
provided as a secret reference does not match a valid secret reference
format`. This is exactly what the v3 deploy-pipeline:7.6.4.strip-secret-
values gate is supposed to prevent — but the gate's underlying validator
was broken.

Changes:

- readSettingsFile now handles all three shapes. Each returned entry
  now carries `stageLabel` (null for shape 1, the stage name for shapes
  2 and 3), so downstream consumers can attribute findings to the right
  stage without re-parsing the file.

- validate-deployment-settings.js had a duplicated read-settings parser
  (readEntriesPreservingStage) with the same bug. Deleted — it now
  uses readSettingsFile directly.

- Tests:
  - Existing readSettingsFile tests updated to expect the new stageLabel
    field on entries.
  - New test: keyed-object shape (Microsoft 2024 schema). Validates the
    real-world fix against a fixture matching the Citizens portal file.
  - validate-deployment-settings.test.js renamed readEntriesPreservingStage
    references to readSettingsFile.

Verified end-to-end against the real Citizens portal file:

  node validate-deployment-settings.js --settingsFile <citizens-portal>
  → summary { invalid: 1 }, one finding flagging the @KeyVault(...) value
    on c311_api_secret as 'kv-placeholder' / 'invalid'.

Plugin tests: 993/993 (was 992; +1 new shape-3 test).
Lint: 0 findings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 26, 2026 16:34
@tyaginidhi

Copy link
Copy Markdown
Contributor Author

Real-world validation against `C:\Projects\Citizens portal` — found and fixed a pre-existing high-impact bug.

Pushed `371a6e1` fixing `readSettingsFile` to handle the keyed-object stages shape that the actual Microsoft `deployment-settings/2024` schema uses.

What I found

Citizens portal's `deployment-settings.json` uses:

```json
{
"$schema": "https://schemas.microsoft.com/power-platform/deployment-settings/2024",
"stages": {
"Deploy to Staging": {
"EnvironmentVariables": [...]
}
}
}
```

But `readSettingsFile` only handled two shapes (top-level `EnvironmentVariables: []` and array-shaped `Stages: []`). For the keyed-object shape it returned 0 entries, which meant `validate-deployment-settings.js` silently passed even on known-broken values.

Why it matters

The Citizens portal's `docs/alm/last-deploy.json` documents a deploy attempt on 2026-05-21 where `@KeyVault(vaultName=...;secretName=...)` for `c311_api_secret` slipped past validation and failed at ImportAsHolding after ~4 hours of queue wait. The v3 `deploy-pipeline:7.6.4.strip-secret-values` gate is supposed to catch exactly this case at sub-second time — but the gate's underlying validator was broken by this shape mismatch.

The fix

  • `readSettingsFile` now handles all three deployment-settings.json shapes (top-level / `Stages: []` array / `stages: {}` keyed object). Each entry now carries `stageLabel` for downstream attribution.
  • Deleted the duplicate `readEntriesPreservingStage` parser in `validate-deployment-settings.js` (had the same bug). Now calls the unified `readSettingsFile`.

Verified end-to-end

```bash
$ node validate-deployment-settings.js --settingsFile "C:\Projects\Citizens portal\deployment-settings.json"
{
"summary": { "total": 2, "valid": 0, "invalid": 1, "unknown-type": 1, "skipped": 0 },
"findings": [
{ "schemaName": "c311_api_secret",
"stageLabel": "Deploy to Staging",
"value": "KeyVault (@KeyVault)(vaultName=lakeshore-staging-kv;secretName=api-secret)",
"valueFormat": "kv-placeholder",
"status": "invalid",
"severity": "error",
"message": "...not a recognized syntax for any env var type. If it's intended as a Key Vault reference, use the Secret Identifier URI or Azure resource ID format..." }
]
}
```

This is exactly the finding that would have prevented the 4h queue wait.

Tests + lint

  • 993/993 plugin tests pass (was 992; +1 new shape-3 test against a fixture matching the Citizens portal file).
  • `alm-lint` 0 findings.
  • 2 existing tests updated to expect the new `stageLabel` field on entries.

Other Citizens portal validations (all passed)

  • ✅ `detect-project-context.js` reads the v2 multi-solution manifest correctly (5 solutions, publisher prefix `c311`).
  • ✅ `check-alm-plan.js` correctly reports `exists:true / stale:false / planStatus:Completed / inExecution:not-running` against `docs/.alm-plan-data.json` — Phase 0 gates would correctly pass through silently.
  • ✅ `docs/alm/last-pipeline.json` is v3 multi-solution (5 solutions in deploymentOrder); the v3 `setup-pipeline:6b.v2-migration` gate wouldn't fire here (already on v3), correctly.
  • ✅ `docs/alm/last-deploy.json` schema matches what `deploy-pipeline` markers reference (`stageRunId`, `pipelineId`, `status`, `runs[]` per solution).
  • ✅ `.powerpages-site/web-roles/` exists with 3 roles (Administrators, Anonymous Users, Authenticated Users) — `setup-auth:1.4.create-webroles` gate wouldn't fire (roles already exist).
  • ✅ Multi-stage v3 pipeline (Foundation, Integration, Config, Content, Future) — exactly the MULTI_RUN_MODE scenario `deploy-pipeline:3.6.batch-pending-approval` is designed for.

Branch is now at `371a6e1` — 6 commits ahead of `origin/main`.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Comment thread plugins/power-pages/scripts/lib/validate-deployment-settings.js
Comment thread plugins/power-pages/scripts/lib/verify-env-var-values.js Outdated
1. Dedupe loss in validateSettings → readSettingsFile path

   readSettingsFile dedupes by schemaName when no stageLabel filter is
   provided. This is correct for callers like verify-env-var-values
   that want a single "configured value of X" per schema. But it is
   WRONG for validate-deployment-settings, which must inspect every
   stage's value independently:

     - Staging: c311_api_secret = "https://kv.../secrets/api-secret"  (valid)
     - Production: c311_api_secret = "@KeyVault(vaultName=...)"       (invalid)

   Pre-fix, dedupe kept Staging's value and Production's broken value
   was silently skipped. validateSettings would report invalid: 0
   even though the deploy would fail at ImportAsHolding on Production.

   Fix: added a `preserveAllStages` option to readSettingsFile.
   validate-deployment-settings now calls it with that flag set so
   every per-stage entry is inspected.

2. Mixed-case `Stages` key when value is an object

   Comment said "either casing of stages/Stages key" but code only
   checked `parsed.stages || parsed.STAGES`. A hand-authored file
   using `Stages: { ... }` (capital-S + object form — falls through
   shape 2's array check because the value isn't an array) returned 0
   entries.

   Fix: added `parsed.Stages` to the fallback chain. All three casings
   (`stages` / `Stages` / `STAGES`) now resolve to the object-shape
   path when the value is a plain object.

3. Regression tests (+3, total 996/996)

   - Mixed-case `Stages` object is read correctly.
   - readSettingsFile dedupe (default) vs preserveAllStages behavior
     pinned with concrete expectations.
   - End-to-end: validateSettings catches a Production-only invalid
     value even when Staging is valid (the dedupe-loss scenario).

Verified against real Citizens portal site — still correctly catches
the @KeyVault(...) placeholder on c311_api_secret.

Lint: 0 findings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tyaginidhi
T-Nid (tyaginidhi) enabled auto-merge (squash) May 26, 2026 17:08
Comment thread plugins/power-pages/scripts/lint-skills-alm.js
…sites

User review feedback: "why are we reverting from a const to a string here?"
on lint-skills-alm.js line 561.

My G7 cleanup in `b42ddc6` went the wrong direction. The original review
finding was that `SKILL_SEVERITY` was half-applied (2 of 9 severity sites
used it, 7 hard-coded `'error'`). I responded by REMOVING the constant
entirely and inlining `'error'` everywhere — eliminating the inconsistency
but also the indirection.

The right fix is the other direction: keep the constant and use it
everywhere. That preserves single source of truth for "what severity does
this plugin's lint emit" (one line to change for a future rule class
that needs a different severity) without the half-applied awkwardness.

All 10 finding-push sites in collectFindings now use SKILL_SEVERITY.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 26, 2026 17:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Comment thread plugins/power-pages/references/approval-gates.md
Comment thread plugins/power-pages/scripts/lint-skills-alm.js Outdated
…1 doc conflict

1. Fence-boundary classification was asymmetric (lint-skills-alm.js:660)

   The GATE-prose-block-required fence-tracking loop toggled `inside`
   BEFORE recording `inFence[i]`, which made:
     - Opening ```  line → marked INSIDE the fence
     - Closing ```  line → marked OUTSIDE the fence

   Inconsistent and counter to CommonMark intent. Both delimiter lines
   are markdown syntax, not code content; the rule should treat them
   the same. Fixed by marking both delimiter lines as OUTSIDE and only
   the strictly-between content lines as INSIDE.

   Regression test added: a 🚦 placed on the opening ```🚦 fence line
   itself now satisfies the rule (delimiter lines are not code content).

2. §6.24a says "closed" but §11 still said "deferred" (approval-gates.md)

   §6.24a was updated in commit 8d02547 to reflect that v3 surfaced the
   runtime-loop prompts as real call sites (manage-firewall:3.action-choice,
   3.execute-consent, scan-site:3.action-choice). But the §11 Open
   Questions bullet still described it as a "known gap" / "deferred from v3"
   — a stale draft from the earlier iteration.

   Reworded §11 to acknowledge the v3 fix (forward-pointer to §6.24a)
   and retain only the historical context.

Plugin tests: 997/997 (was 996; +1 fence-boundary regression test).
Lint: 0 findings.

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

Copy link
Copy Markdown
Contributor Author

Copilot resolve the merge conflicts in this pull request

Keep gate marker + 🚦 prose block from PR branch; incorporate
multi-provider re-run handling content added in main (b4c46e9).
Copilot AI review requested due to automatic review settings June 3, 2026 05:47
@tyaginidhi
T-Nid (tyaginidhi) removed the request for review from Copilot June 3, 2026 05:47

Copilot AI commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Copilot resolve the merge conflicts in this pull request

Resolved in commit 9810f4a. There was one conflict in plugins/power-pages/skills/setup-auth/SKILL.md at section 2.1 — the PR branch had added the <!-- gate: setup-auth:2.1.requirements --> marker + 🚦 prose block, while main (b4c46e9) had added a substantial re-run handling / multi-provider walkthrough in the same spot. The resolution keeps the gate marker first, then incorporates the full new content from main. Verified: alm-lint: 0 findings, 1042/1042 tests passing.

@tyaginidhi

Copy link
Copy Markdown
Contributor Author

Copilot resolve the merge conflicts in this pull request

Merge origin/main (93e22d3) into branch — two files conflicted:
  - create-webroles/SKILL.md: kept gate markers from branch, incorporated
    caller-suppress-mode text added in main (#144).
  - integrate-webapi/SKILL.md: same — kept 3 gate markers from branch,
    incorporated AI-only-read-mode conditionals added in main (#144).

New add-ai-webapi skill (#144) had 4 unmarked AskUserQuestion call sites
that failed GATE-must-have-marker. Added gate markers + 🚦 prose blocks
and §6.29 catalog rows for:
  - add-ai-webapi:iter.deploy-commit (Iteration mode)
  - add-ai-webapi:4.2.skip-webrole (Phase 4.2 skip confirmation)
  - add-ai-webapi:5.5.commit (Phase 5.5 offer to commit)
  - add-ai-webapi:6.4.commit (Phase 6.4 offer to commit)

Verified: alm-lint: 0 findings; 1108/1108 tests pass.
Copilot AI review requested due to automatic review settings June 11, 2026 11:08
@tyaginidhi
T-Nid (tyaginidhi) removed the request for review from Copilot June 11, 2026 11:08

Copilot AI commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Copilot resolve the merge conflicts in this pull request

Resolved in commit 25e1ca0. Two files conflicted (create-webroles/SKILL.md, integrate-webapi/SKILL.md) — kept the existing gate markers from the branch and incorporated the caller-suppress / AI-only-read-mode conditionals added by main (#144). The new add-ai-webapi skill brought in by main had 4 unmarked AskUserQuestion call sites that failed GATE-must-have-marker; added gate markers + 🚦 prose blocks for iter.deploy-commit, 4.2.skip-webrole, 5.5.commit, and 6.4.commit, plus §6.29 catalog rows. Verified: alm-lint: 0 findings, 1108/1108 tests pass.

@tyaginidhi
T-Nid (tyaginidhi) merged commit 180ed78 into main Jun 12, 2026
1 check passed
@tyaginidhi
T-Nid (tyaginidhi) deleted the users/nityagi/ApplyApprovalGatesPattern branch June 12, 2026 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants