microsoft / microsoft/GitHub-Copilot-for-Azure
[Feature]: Convention for deterministic unit tests of skill-owned scripts (sanitizers, collectors)
- 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
Assessment
This issue has not been assessed yet.