microsoft / microsoft/GitHub-Copilot-for-Azure

[Feature]: Convention for deterministic unit tests of skill-owned scripts (sanitizers, collectors)

Open
#3,213 1 comment 0 reactions 1 assignee Claimed by @JasonYeMSFT View on GitHub
skills
Dominant language
Python
Stars
250
Forks
204
Avg merge
1d 12h
Merged PRs (30d)
67

Description

### Problem

Some skills ship deterministic, executable pieces — `scripts/*.sh`/`*.ps1` collectors and `scripts/*.jq` filters — whose correctness is a safety property, not just a quality one. Examples from `plugins/`:

- `azure-kubernetes/azure-kubernetes-automatic-readiness/scripts/sanitize-readiness-input.jq` is an allowlist projection whose entire purpose is to keep `Secret`/`ConfigMap` data, env values, and non-allowlisted annotations out of model context.
- `aks-skills/aks-troubleshooting/scripts/{cluster-snapshot,pod-deep-dive}.sh` must refuse to read a cluster whose kube context does not match the named AKS resource, and must redact credential-shaped strings in log projections.

Today there is no sanctioned place for unit tests of these artifacts. In #3164 review (@JasonYeMSFT) we agreed `scripts/` is for repo tooling and `tests/` has stopped taking per-skill cases, so the tests I had written were removed. Vally evals are the right tool for model behavior, but they are model-backed and non-deterministic — the wrong tool for "does this jq drop secret values" or "does this script exit 2 on a context mismatch".

### Desired Solution

Design and land a convention for deterministic, model-free unit tests of skill-owned scripts, then seed it with the first few cases.

Proposed shape (open to change):

- **Location:** co-located with the skill, e.g. `plugins//skills//scripts/__tests__/`, so ownership follows `CODEOWNERS` for that skill and the tests travel with the artifact they test. Excluded from the built plugin payload by the gulp build (same mechanism that already excludes `version.json`).
- **Runner:** one repo-level vitest project (or jest, matching `tests/`) that globs `plugins/**/scripts/__tests__/**/*.test.ts`. No per-skill package.json.
- **Constraints:** mocked `az`/`kubectl`/`jq` via a temp `PATH`; no network, no cloud credentials, no live resources. A test that needs a real cluster belongs in Vally, not here.
- **CI:** a single job, PR-triggered on `plugins/**/scripts/**`. Deterministic, fast, no model tokens.

**First candidates** (previously written and passing at 3fd156cc, removed in 8154b3a6 — can be restored into whatever location is agreed):

1. `sanitize-readiness-input.jq` — 7 cases: drops every sentinel the constraint spec never evaluates; drops `Secret`/`ConfigMap` resources entirely; keeps Baseline-safeguard fields; keeps conditional (Restricted/Windows) safeguard fields; projects image policy without exposing registries; keeps only reserved label keys and AppArmor annotation validity; computes duplicate Service selectors per namespace and keeps StorageClass provisioner.
2. `cluster-snapshot.sh` / `pod-deep-dive.sh` — missing-tool preflight exits 2 with an actionable message and performs no resource/artifact activity; successful target proof; kube-context/AKS-resource mismatch is rejected; pod log projection redacts `token=` style secrets.

### Alternatives and Workarounds

- Keep these as Vally cases: rejected — model-backed and probabilistic; a redaction regression should fail deterministically.
- Put them in `scripts/src/__tests__/`: rejected in review — that project is for repo tooling.
- Put them in `tests/`: rejected in review — per-skill cases are no longer accepted there.
- Do nothing: the sanitizer and collector scripts stay untested, and a future edit can silently widen what reaches the model.

### Impact

Affects any skill that ships executable helpers (today: `azure-kubernetes-automatic-readiness`, `aks-troubleshooting`, `aks-network-capture`, `azure-diagnostics/scripts`). Once the convention exists, adding a case is one file next to the script. Prevents a class of privacy/safety regressions that no current CI job can catch.

### Additional Context

Follow-up from #3164 review thread: https://github.com/microsoft/GitHub-Copilot-for-Azure/pull/3164#discussion_r4020126437. Happy to send the seed PR once the location/runner is agreed; I'd rather not guess at the convention and get it wrong twice.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.