chore: second-pass review feedback across validator, skills, and workflows (#25)

* chore: second-pass review feedback across validator, skills, and workflows

Validator (scripts/validate_skill.py):
- Required-section emptiness check now uses H2-only positions so an
  '## Workflow' with '### Step 1' subheadings is no longer flagged empty.
- New _strip_code_fences masks fenced regions to whitespace of equal
  length, preserving byte offsets, so '## something' inside a 'bash'
  fence cannot break section detection.
- validate_file and _load_skill_entry open files as utf-8-sig to tolerate
  a BOM that some Windows editors insert.
- changed_modules warns to stderr and returns [] when both 'git diff'
  invocations fail, instead of raising.
- main() reconfigures stdout/stderr to UTF-8 (errors=replace) so the
  check/cross glyphs no longer crash Python 3.14 on a cp1252 Windows
  console.
- Tests: four new cases cover fence stripping, subheadings inside
  required sections, headings inside fences, and BOM-prefixed input.
  58 pass.

github-actions skill:
- OIDC debug snippet decodes the JWT instead of dumping the request
  wrapper (jq .value | cut -d. -f2 | base64 -d | jq .sub).
- REFERENCE.md 'packages: read|write' rows distinguish GitHub Packages
  npm from GHCR; example dependabot.yml and dependabot-auto-merge.yml
  now match the chassis this repo actually ships (deny-all workflow
  scope, per-job opt-ins, 'set -euo pipefail' in inline shell).

openshift-deployment skill:
- Replaced the invalid 'oc get pods --field-selector=status.phase=
  Pending,status.phase=Terminating' (duplicate key, and Terminating is
  not a Pod phase) with the metadata.deletionTimestamp jq filter.
- Probe table now says '(150 s budget)' to match the YAML math
  (periodSeconds: 5 * failureThreshold: 30).
- Patroni image bumped from patroni-postgres:12.4-2.0 (PG 12 EOL Nov
  2024) to patroni-postgres:16.4-3.0.

azure-networking skill:
- Rewrote the privateEndpointNetworkPolicies = 'Disabled' rationale -
  the previous wording had the semantics backwards. Documented all four
  values (Disabled, NetworkSecurityGroupEnabled, RouteTableEnabled,
  Enabled) and the consumer-side threat-model justification for the
  BC Gov default.
- App Gateway NSG GatewayManager row labelled 'v2 SKU; v1 uses
  65503-65534'.
- Tagged the untagged Terraform pseudocode fence as 'text'.

Workflows:
- pages.yml: workflow-scope 'permissions: {}' with per-job opt-ins. The
  build job only reads the repo; pages:write + id-token:write are scoped
  to the deploy job.
- pr.yml: added concurrency 'pr-${{ github.head_ref || github.ref }}'
  with cancel-in-progress: true so a force-push supersedes stale runs.

Housekeeping:
- .gitignore: removed the duplicate Python block - every entry was
  already covered earlier in the file.
- Internal meta-skills (skill-author, skill-validator) gained
  owner: bcgov and tags in frontmatter to model best-practice frontmatter
  that the spec recommends.

* style: ruff format new tests (blank-line + double-quoted string)

Author: mishraomp

.github/skills/skill-author/SKILL.md

@@ -1,6 +1,8 @@
 ---
 name: skill-author
 description: Scaffolds a new skill profile in this repo and fills in the required SKILL.md structure when a contributor wants to add a skill.
+owner: bcgov
+tags: [meta, skills]
 metadata:
   internal: true
 ---

.github/skills/skill-validator/SKILL.md

@@ -1,6 +1,8 @@
 ---
 name: skill-validator
 description: Validates skill profiles against the spec and explains how to fix failures before a PR is opened or merged.
+owner: bcgov
+tags: [meta, validation]
 metadata:
   internal: true
 ---

.github/workflows/pages.yml

@@ -18,12 +18,11 @@ on:
     # for a while. Sundays 07:00 UTC is off-peak for BC Gov maintainers.
     - cron: "0 7 * * 0"
 
-# Pages deploys need pages:write + id-token:write (for the OIDC-signed
-# environment URL). Both jobs only read repo contents.
-permissions:
-  contents: read
-  pages: write
-  id-token: write
+# Deny-all baseline at the workflow scope; jobs opt into the minimum scopes they
+# need. Build only reads the repo; deploy is the only job that needs the elevated
+# Pages permissions (pages:write to publish, id-token:write for the OIDC-signed
+# environment URL on actions/deploy-pages@v5).
+permissions: {}
 
 # Allow only one Pages deployment at a time. Don't cancel in-flight runs —
 # letting them finish keeps the deployment history honest.
@@ -34,6 +33,8 @@ concurrency:
 jobs:
   build:
     runs-on: ubuntu-24.04
+    permissions:
+      contents: read
     steps:
       - name: Checkout
         uses: actions/checkout@v6
@@ -61,6 +62,10 @@ jobs:
   deploy:
     needs: build
     runs-on: ubuntu-24.04
+    permissions:
+      contents: read
+      pages: write
+      id-token: write
     environment:
       name: github-pages
       url: ${{ steps.deployment.outputs.page_url }}

.github/workflows/pr.yml

@@ -28,6 +28,15 @@ on:
 permissions:
   contents: read
 
+# Cancel superseded runs on the same PR (a force-push or new commit makes the
+# in-flight run's results stale). The group key is keyed on the PR's head
+# ref so concurrent PRs don't cancel each other. Pushes to a branch outside a
+# PR (this workflow doesn't trigger on `push`, but the guard is defensive)
+# would fall back to ${{ github.ref }}.
+concurrency:
+  group: pr-${{ github.head_ref || github.ref }}
+  cancel-in-progress: true
+
 jobs:
   # 0. Hard gate: fail loudly on PRs opened from a fork. This produces a real
   #    failed status check (not a neutral "skipped") so branch protection can

.gitignore

@@ -216,12 +216,6 @@ __marimo__/
 
 # Streamlit
 .streamlit/secrets.toml
-# Python
-__pycache__/
-*.py[cod]
-.venv/
-.ruff_cache/
-.pytest_cache/
 
 # Node / npm (skill packaging)
 node_modules/

scripts/validate_skill.py

@@ -193,17 +193,54 @@ def _heading_positions(body: str):
   return [m.start() for m in re.finditer(r"^#{1,6}\s+.+$", body, re.MULTILINE)]
 
 
+# Fenced code blocks open with three or more backticks (or tildes) as the
+# first non-space content on a line. A `## ` inside such a fence is sample
+# text, not a real markdown heading — masking the fence content before any
+# regex scan keeps the structural checks from being fooled by it.
+_FENCE_RE = re.compile(r"^\s*(`{3,}|~{3,})")
+
+
+def _strip_code_fences(body: str) -> str:
+  """Return ``body`` with fenced-code-block lines blanked to spaces.
+
+  Byte offsets are preserved: every blanked line keeps its original length so
+  that ``re.finditer(...).start()`` matches the same character index in the
+  returned string and in the input body. The opening and closing fence lines
+  are also blanked so an interior ``## comment`` cannot be matched as a real
+  markdown heading by downstream regexes.
+
+  Args:
+    body: The markdown body of a SKILL.md file.
+
+  Returns:
+    A string of identical length and line count to ``body``, with every
+    character inside (and on) a fenced code block replaced by a space.
+  """
+  lines = body.split("\n")
+  in_fence = False
+  for i, line in enumerate(lines):
+    if _FENCE_RE.match(line):
+      in_fence = not in_fence
+      lines[i] = " " * len(line)
+    elif in_fence:
+      lines[i] = " " * len(line)
+  return "\n".join(lines)
+
+
 def _content_after(body: str, start: int, heads) -> str:
-  """Return the text between a heading and the next heading.
+  """Return the text between a heading and the next section boundary.
 
   Args:
     body: The markdown body being inspected.
     start: The character offset of the heading whose content is wanted.
-    heads: All heading offsets in ``body`` (from :func:`_heading_positions`).
+    heads: Section-boundary heading offsets in ``body``. Pass only the ``##``
+      positions when checking whether a required section is empty, so that a
+      sub-heading (``###`` and deeper) inside the section is treated as part
+      of the section's content rather than as the section's end.
 
   Returns:
-    The stripped text that follows the heading line up to the next heading, or
-    an empty string when the section has no content.
+    The stripped text that follows the heading line up to the next boundary,
+    or an empty string when the section has no content.
   """
   nxt = min([h for h in heads if h > start], default=len(body))
   line_end = body.find("\n", start)
@@ -233,17 +270,25 @@ def validate_body(body: str) -> list:
   if not body.strip():
     return ["body is empty — it must contain the required sections"]
 
-  heads = _heading_positions(body)
+  # Strip fenced-code-block content before scanning for headings so that a
+  # ``## comment`` inside a YAML / Bash / Terraform sample is not mistaken for
+  # a markdown heading. Byte offsets are preserved so `m.start()` lines up
+  # with the original `body` for content extraction below.
+  scan_body = _strip_code_fences(body)
 
   # H1 title
-  if not re.search(r"^#\s+(.+?)\s*$", body, re.MULTILINE):
+  if not re.search(r"^#\s+(.+?)\s*$", scan_body, re.MULTILINE):
     errors.append("missing H1 title line '# <Skill Name>'")
 
   # required sections present + non-empty
   sections = [
     (m.start(), m.group(1).strip())
-    for m in re.finditer(r"^##\s+(.+?)\s*$", body, re.MULTILINE)
+    for m in re.finditer(r"^##\s+(.+?)\s*$", scan_body, re.MULTILINE)
   ]
+  # Only the ``##`` positions count as section boundaries. A required section
+  # that opens with a ``###`` sub-heading (e.g. ``## Workflow`` followed by
+  # ``### Step 1``) is still non-empty — its content runs until the next H2.
+  section_starts = [s for s, _ in sections]
   # Normalise smart-quote apostrophe to ASCII before comparison so headings
   # like "## Don\u2019t Use When" still match the spec's "## Don't Use When".
   present = {title.replace("\u2019", "'").lower(): start for start, title in sections}
@@ -255,7 +300,7 @@ def validate_body(body: str) -> list:
 
   for start, title in sections:
     if title.replace("\u2019", "'").lower() in required_lower and not _content_after(
-      body, start, heads
+      body, start, section_starts
     ):
       errors.append(f"section '## {title}' is empty — add at least one line of content")
 
@@ -352,7 +397,9 @@ def validate_file(path: str) -> list:
   """
   if not os.path.isfile(path):
     return [f"file not found: {path}"]
-  with open(path, encoding="utf-8") as f:
+  # ``utf-8-sig`` transparently strips a leading BOM so editors that save
+  # SKILL.md with one don't trip the ``startswith('---')`` frontmatter check.
+  with open(path, encoding="utf-8-sig") as f:
     text = f.read()
   skill_dir = os.path.dirname(path)
   data, body, errors = parse_frontmatter(text)
@@ -430,11 +477,20 @@ def changed_modules(base: str) -> list:
       text=True,
     )
     if res.returncode != 0:
+      # Fall back to a plain two-dot diff when the three-dot form fails (e.g.
+      # because the merge-base cannot be found in a shallow clone).
       res = subprocess.run(
         ["git", "diff", "--name-only", base],
         capture_output=True,
         text=True,
       )
+      if res.returncode != 0:
+        print(
+          f"git diff vs '{base}' failed (exit {res.returncode}); "
+          f"validating no changed skills. stderr: {res.stderr.strip()}",
+          file=sys.stderr,
+        )
+        return []
   except FileNotFoundError:
     print("git not found on PATH; cannot compute changed skills.", file=sys.stderr)
     return []
@@ -486,7 +542,7 @@ def _load_skill_entry(path: str):
     ``body_norm`` fingerprints, or ``None`` if the file cannot be read.
   """
   try:
-    with open(path, encoding="utf-8") as f:
+    with open(path, encoding="utf-8-sig") as f:
       text = f.read()
   except OSError:
     return None
@@ -615,6 +671,21 @@ def main(argv=None) -> int:
   Returns:
     ``0`` if every targeted skill passed, ``1`` if any skill failed validation.
   """
+  # The pass/fail report uses U+2713 / U+2717. On Windows the default console
+  # code page is cp1252, which can't encode those — Python raises
+  # UnicodeEncodeError mid-print and the whole run aborts. Reconfigure both
+  # stdio streams to UTF-8 with replacement so the script is portable.
+  for stream in (sys.stdout, sys.stderr):
+    reconfigure = getattr(stream, "reconfigure", None)
+    if callable(reconfigure):
+      try:
+        reconfigure(encoding="utf-8", errors="replace")
+      except (OSError, ValueError):
+        # Stream is detached, redirected to a non-TextIOWrapper, or otherwise
+        # not reconfigurable — fall through; the print() calls below will
+        # still work for ASCII output and only the check/cross glyphs may be
+        # mojibake'd. That's a strictly better failure mode than aborting.
+        pass
   p = argparse.ArgumentParser(description="Validate SKILL.md files against the spec.")
   p.add_argument("paths", nargs="*", help="specific SKILL.md files to validate")
   p.add_argument("--all", action="store_true", help="validate every skill in the repo")

skills/azure-networking/SKILL.md

@@ -38,7 +38,7 @@ tags: [azure, networking, terraform, bicep, subnets, private-endpoints, nsg]
 5. **Create the subnet with its NSG attached in the same API call** — declare `networkSecurityGroup` on the subnet body itself, not as a separate association. Tool specifics: Terraform — use `azapi_resource` with `networkSecurityGroup.id` inside `body.properties` (the `azurerm_subnet` + `azurerm_subnet_network_security_group_association` pair is two PUTs and leaves an unprotected gap); Bicep/ARM — set the `networkSecurityGroup` property on the subnet resource; az CLI — pass `--network-security-group` to `az network vnet subnet create`.
 6. **Serialize sibling-subnet writes** — ARM serializes writes against the same VNet; parallel writes fail with `AnotherOperationInProgress`. Terraform — list every preceding sibling subnet in `depends_on`. Bicep — rely on the implicit resource graph, or add an explicit `dependsOn` array when siblings live in separate modules. az CLI / scripts — issue creates sequentially.
 7. **(Terraform only) Lock the VNet** — add `locks = [data.azurerm_virtual_network.target.id]` to every subnet's `azapi_resource` so Terraform's own parallelism doesn't race ARM in the gap between dependent operations. Bicep/ARM deployments don't need this — the deployment engine doesn't parallelize within one scope.
-8. **For PE subnets** — set `privateEndpointNetworkPolicies = "Disabled"` and no delegation. Size the CIDR for the worst-case service that will land on it (some services consume more than one IP per PE — see references).
+8. **For PE subnets** — set `privateEndpointNetworkPolicies = "Disabled"` (the long-standing default, which means NSGs and UDRs on the subnet are **not** evaluated for the PE NIC) and no delegation. If you do need to override the PE's `/32` default route through a firewall, set `"RouteTableEnabled"` (or `"Enabled"` for both NSG and UDR); for defence-in-depth NSG enforcement on a PE NIC itself, use `"NetworkSecurityGroupEnabled"`. Size the CIDR for the worst-case service that will land on it (some services consume more than one IP per PE — see references).
 9. **For App Gateway subnets** — no delegation. If asymmetric routing is observed and your platform allows custom Route Tables, attach one with `0.0.0.0/0 → Internet` (plus any internal corp ranges that must not egress via ExpressRoute / VWAN hub). **BC Gov Landing Zone**: Route Tables are platform-protected (see Step 1) — raise a Public Cloud Service Request for any required UDR.
 10. **Expose subnet ID and CIDR as outputs** (Terraform `output`, Bicep `output`, deployment script return value) so downstream stacks can read them via `terraform_remote_state`, Bicep `existing` references, or another state-sharing mechanism.
 11. **Validate before apply** — run the tool's validate + dry-run path: Terraform `fmt -recursive`, `validate`, `plan`; Bicep `az bicep build` then `az deployment ... what-if`; ARM `az deployment ... what-if`. Catch CIDR overlap and delegation errors before apply.
@@ -49,7 +49,7 @@ tags: [azure, networking, terraform, bicep, subnets, private-endpoints, nsg]
 - Never share a delegated subnet between services. (Why: each subnet allows exactly one `serviceDelegations` entry; the second service errors at associate time.)
 - Never set a delegation on a Private Endpoint or App Gateway subnet. (Why: Azure rejects PE creation in delegated subnets, and App Gateway refuses to launch in one.)
 - Never move an existing Private Endpoint to a different subnet without a maintenance window. (Why: moving a PE destroys and recreates it, breaking the customer-facing DNS record until propagation completes.)
-- Always set `privateEndpointNetworkPolicies = "Disabled"` on PE subnets. (Why: NSG enforcement on PE NICs requires the subnet flag to be off; otherwise PE traffic is silently dropped.)
+- Always set `privateEndpointNetworkPolicies` explicitly on PE subnets — typically `"Disabled"`, which is the long-standing default and is what most BC Gov PE designs use. (Why: this property controls whether the subnet's NSG and UDR are evaluated for PE NIC traffic; `"Disabled"` skips both, `"NetworkSecurityGroupEnabled"` applies NSGs only, `"RouteTableEnabled"` applies UDRs only, `"Enabled"` applies both. Most threat models for private endpoints only need to govern the *consumer* side of the connection — the workload subnet talking to the PE — because the PE itself only exposes the upstream Azure service and has no arbitrary egress to control. Pin the value in IaC so behaviour is reproducible across regions and subscriptions where the platform default may have drifted. Switch to `"RouteTableEnabled"` (or `"Enabled"`) when you need a UDR to override the PE's `/32` default route through a firewall or NVA.)
 - Always size PE subnets for the **worst-case** service that will land on them. (Why: Cosmos DB PE consumes 2 IPs — one global + one regional endpoint; Azure AI Services "AIServices" kind exposes up to 3 sub-resources = 3 IPs per PE. A `/27` fills surprisingly fast.)
 - Always expose subnet IDs as outputs (Terraform `output`, Bicep `output`) even if no downstream stack reads them yet. (Why: adding outputs later is cheap, but a downstream stack that needs the ID can't read state it isn't exposed to.)
 - Never modify VNet address space or create a Private DNS Zone — both are managed centrally by the platform team (see Step 1's platform-protected list; raise a [Public Cloud Service Request](https://citz-do.atlassian.net/servicedesk/customer/portal/3) instead).
@@ -58,7 +58,7 @@ tags: [azure, networking, terraform, bicep, subnets, private-endpoints, nsg]
 - "Add a Container Apps Environment subnet" → declare the subnet with a `Microsoft.App/environments` delegation (Terraform `azapi_resource` body, Bicep `delegations` array on the subnet, or `az network vnet subnet create --delegations Microsoft.App/environments`); attach an NSG that allows outbound 443 to AzureContainerRegistry, AzureMonitor, AzureActiveDirectory, and VirtualNetwork; serialize against every other subnet in the VNet.
 - "Deployment keeps failing intermittently with `AnotherOperationInProgress`" → sibling subnets are being written in parallel against the same VNet. Terraform — add `depends_on = [azapi_resource.first_subnet]` to the second, and `[azapi_resource.first_subnet, azapi_resource.second_subnet]` to a third. Bicep — add `dependsOn: [first, second]` when siblings span separate modules. CLI — stop backgrounding the `az network vnet subnet create` calls.
 - "App Gateway deployment fails with `SubnetMustNotHaveDelegation`" → remove the delegation from the AppGW subnet (Terraform: drop the `delegations` block from the `azapi_resource` body; Bicep: remove the `delegations` array from the subnet; CLI: omit `--delegations`).
-- "PE creation fails with `PrivateEndpointCannotBeCreatedInSubnet`" → subnet has a delegation, or `privateEndpointNetworkPolicies` is still `"Enabled"`. Fix both.
+- "PE creation fails with `PrivateEndpointCannotBeCreatedInSubnet`" → subnet has a delegation incompatible with PEs, or `privateEndpointNetworkPolicies` is set to a value the PE provider's automation refuses. Set the property to `"Disabled"` and remove any delegation.
 
 ## Edge Cases
 - **VNet owned by a separate IaC stack** — reference it as an existing resource in the current root (Terraform `data.azurerm_virtual_network`, Bicep `resource ... existing`); pin to its ID for the parent and (Terraform only) for `locks`. Do **not** try to import the VNet.

skills/azure-networking/references/REFERENCE.md

@@ -147,7 +147,7 @@ ARM serializes writes against the same VNet — concurrent subnet PUTs fail with
 
 Each subnet must depend on **all** preceding siblings:
 
-```
+```text
 azapi_resource.pe_subnet                  # no deps (first)
 azapi_resource.apim_subnet                # depends_on: [pe_subnet]
 azapi_resource.appgw_subnet               # depends_on: [pe_subnet, apim_subnet]
@@ -223,7 +223,7 @@ Reserve a priority band (e.g. 400–499) for dynamic peer entries so adding or r
 | Priority | Direction | Source / Destination               | Port        | Purpose                       |
 |----------|-----------|------------------------------------|-------------|-------------------------------|
 | 100      | Inbound   | `Internet`                         | 443         | HTTPS ingress                 |
-| 120      | Inbound   | `GatewayManager` service tag       | 65200–65535 | AGW infrastructure control    |
+| 120      | Inbound   | `GatewayManager` service tag       | 65200–65535 (v2 SKU; v1 uses 65503–65534) | AGW infrastructure control    |
 | 130      | Inbound   | `AzureLoadBalancer` service tag    | *           | LB health probes              |
 
 Port 80 is intentionally not allowed — let App Gateway terminate HTTPS and skip an HTTP-to-HTTPS hop at the NSG layer.
@@ -273,4 +273,4 @@ App Gateway is one of the few subnets that often needs an explicit route table 
 | `NetworkSecurityGroupNotAssociated`                | Subnet created without an NSG in its body under an NSG-at-creation policy           | Attach the NSG inline at creation (Terraform `azapi_resource` with `networkSecurityGroup.id` in `body.properties`; Bicep `networkSecurityGroup` property on the subnet; CLI `--network-security-group`) |
 | `SubnetConflictWithOtherSubnet`                    | CIDR overlap within the same address space                                         | Re-check the allocation map; pick non-overlapping CIDRs                            |
 | `AnotherOperationInProgress`                       | Concurrent subnet PUTs to the same VNet                                            | Serialize subnet writes (Terraform `depends_on` chain + `locks`; Bicep `dependsOn` across modules; CLI — sequential calls) |
-| `PrivateEndpointCannotBeCreatedInSubnet`           | Subnet has a delegation, or `privateEndpointNetworkPolicies` is not `"Disabled"`   | Fix both on the subnet                                                             |
+| `PrivateEndpointCannotBeCreatedInSubnet`           | Subnet has a delegation incompatible with PEs, or `privateEndpointNetworkPolicies` is set to a value the PE provider's automation refuses | Set the property to `"Disabled"` and remove any delegation                          |