nic-code-review

# NIC Code Review This skill defines **how** to review a NIC PR: the workflow, guardrails, dimension coverage, and output format. It intentionally does **not** restate the codebase-specific rules that already live in the domain skills -- load the referenced skill for depth on any topic. If you find yourself wanting to add a paragraph of file paths or function names here, add it to the relevant domain skill instead. ## When this skill applies - Local review inside VS Code / IDE (Copilot Chat, Claude, or any agent) - `.github/prompts/pr-review.prompt.md` invocation - GitHub Copilot Code Review bot (reads `.github/copilot-instructions.md`, which references this skill) - Any request phrased as "review this PR", "review the diff", "review my branch" ## Review guardrails - Comment only at **>80% confidence**. If unsure, skip. - Be **concise, actionable, file+line specific**. Point at the fix, not the theory. - Prefer **one strong comment** over many weak ones. - Do **not** rewrite the diff for the author, instead suggest the change and let them apply it. - Do **not** compliment, restate the diff, or narrate what the PR does. - Never post secrets, tokens, license keys, or any credential value in a review comment. - Do not fabricate file paths, symbol names, or line numbers. Always verify before citing. ## Verify before flagging Before writing any comment, you must confirm the claim against actual code or config. Speculation is not review. If you cannot verify it, do not write it. | If the comment claims... | You must first... | | --- | --- | | "X is not tracked / covered / handled by tool Y" | Read Y's config (`renovate.json`, `.golangci.yml`, `Makefile`, workflow file). Default managers cover more than you think. | | "This library / action does Z on failure / edge case" | Read the library docs or source, or find an existing call site in the repo that proves the behaviour. | | "This shell / expression / YAML will evaluate as W" | Trace it end-to-end. GitHub Actions expression semantics, bash quoting, and YAML type coercion all have non-obvious rules. | | "This is a security issue because untrusted input reaches sink S" | Identify the actual trust boundary. Inputs from repo-controlled workflows, composite action callers inside the same repo, and matrix values are not "untrusted" in the OWASP sense. | | "The generated file / snapshot is wrong" | Re-run the generator (`make update-codegen`, `make update-crds`, `make telemetry-schema`, `make test-update-snaps`) and diff. Comment on the source, not the artifact. | | "This will break at runtime" | Grep for at least one caller. Read the surrounding function. A missing nil check may already be guarded upstream. | | "This NGINX directive does/does not do X" | Look it up on <https://nginx.org/en/docs/> (or the NGINX Plus docs for Plus-only directives) **before** commenting. Quote the directive's context, default, and version. | | "This directive is allowed in this context" | Check the directive's `Context:` line in the nginx docs. `http`, `server`, `location`, `stream`, `upstream` are not interchangeable, and a wrong-context directive fails `nginx -t` at reload, not at build time. | | "This is not how NIC exposes this feature" | Check <https://docs.nginx.com/nginx-ingress-controller/> and the existing annotation / CRD field for the same capability before claiming a new API is redundant or misnamed. | If verification is impractical (e.g. it requires running the CI), drop the finding. Do not file it with a hedge. ## Verify against upstream NGINX before reviewing config behaviour NIC generates NGINX configuration. A review that reasons about NGINX semantics from memory is unreliable -- directive contexts, defaults, and Plus-vs-OSS availability change between versions. Consult the authoritative source, then comment. | What you need to know | Authoritative source | | --- | --- | | Does this directive exist? What is its context, syntax and default? | <https://nginx.org/en/docs/dirindex.html> | | What do these variables resolve to? | <https://nginx.org/en/docs/varindex.html> | | Is this module available in the OSS build we ship? | <https://nginx.org/en/docs/> module page + `build/Dockerfile` package list | | Is this directive / module Plus-only? | <https://docs.nginx.com/nginx/admin-guide/> and the `nginx-plus` template variant | | Exact upstream behaviour or edge case not covered by the docs | <https://github.com/nginx/nginx> source, or `njs` docs at <https://nginx.org/en/docs/njs/> | | How does NIC already expose this? | <https://docs.nginx.com/nginx-ingress-controller/> plus `internal/configs/annotations.go` and `pkg/apis/configuration/v1/types.go` | | NGINX App Protect WAF / DoS behaviour | <https://docs.nginx.com/nginx-app-protect-waf/> and <https://docs.nginx.com/nginx-app-protect-dos/> | Rules for using these sources: - **Check before you flag, and check before you approve.** A generated directive that is syntactically valid but in the wrong context still breaks the reload -- that is a Blocking finding, and it is only findable by reading the docs. - When a finding rests on upstream behaviour, **cite the source** in the bullet so the author can verify it in one click. - If the docs and the diff disagree, prefer the docs -- unless the PR description explains a deliberate deviation, in which case drop it. - Do not cite a doc page you did not read. Fabricated citations are worse than no citation. - Plus-only directives must appear only in `nginx-plus.*.tmpl`. If one leaks into the OSS template, NGINX OSS fails to start -- always Blocking. ## Confidence downgrades Downgrade or drop a finding when any of these apply: - The bug depends on a code path you have not read end-to-end -> drop. - The behaviour depends on external tool internals (BuildKit cache, Docker registry retry, Kubernetes API server ordering) -> drop, unless you can cite the docs. - The "vulnerability" requires an attacker who already controls the repo / workflow file -> Non-blocking hygiene note at most. - The finding is "this could be better" without a concrete failure mode -> drop. ## Review workflow 1. **Read the PR title, description, and linked issue.** Understand intent before reading the diff. 2. **Get the diff.** Locally: `git diff origin/main...HEAD` or `gh pr diff <n>`. In agent context, use the `get_changed_files` tool. 3. **Classify the change** using the table below to pick the right sub-skills. 4. **Read the surrounding code**, not just the diff hunks, context often lives in the same file just outside the hunk. 5. **Verify NGINX / NIC semantics upstream** for any change that reaches a `.tmpl` file, an annotation, or a CRD field -- see the source table above. 6. **Walk the review dimensions** in order (Security -> Correctness -> Architecture -> Tests -> Build/chart/CI -> Docs and Examples), loading the referenced skills for depth. 7. **Verify claims before commenting.** Grep for the symbol, read the referenced file, run `make lint`/`make test` if in doubt. 8. **Run the completeness gate** below before writing anything. 9. **Produce the review** in the Output Format below. 10. **Clean up.** Leave no trace of the review -- see below. ### Local checkout and cleanup This section applies only to reviews run on a developer's machine, where a review can leave worktrees, scratch files and pulled images behind. The GitHub Copilot Code Review bot can skip it. - Never write to the user's checkout. Run anything that can write files -- `make test` (go-snaps writes missing snapshots outside CI), `make test-update-snaps`, `make update-codegen`, `make update-crds`, `make format` -- in a detached throwaway worktree, so the review creates no branch: `git fetch origin pull/<n>/head && git worktree add --detach /tmp/pr-<n> FETCH_HEAD` for a PR, `git worktree add --detach /tmp/pr-head HEAD` for the current branch. - Pass environment variables per command (`GH_PAGER=cat gh pr view <n>`); never `export` them in the shared shell. - Delete only what this review created, and ask before removing older leftovers. - Do not wipe shared caches as cleanup: no `go clean -cache`, `docker system prune` or `git gc --prune=now`. When the review is done, remove what it created, then confirm `git status --short` matches what it showed before the review: ```sh git worktree remove /tmp/pr-<n> # /tmp/pr-head for a current-branch review rm -f /tmp/<scratch files> docker rmi <image> # only if it was not present before the review git worktree list && git status --short ``` ## Severity ladder Two severities, nothing else. No "nit", "minor", "praise", "FYI", "possibly blocking". No overall verdict such as "approve" or "request changes" -- the findings are the review. | Severity | Test it must pass | | --- | --- | | **Blocking** | Verified, and you can name the trigger, the failure, and the blast radius in one sentence. Coverage gaps are blocking too. | | **Non-blocking** | Verified, but the worst case is confusing code or future maintenance | Unverified -> do not write it. There is no third bucket for hunches. **Verified** means one of: a call site you read, command output (`make test`, `nginx -t`, `git diff`), or a doc page you opened. Reading the diff is not verification. Blocking means something breaks for someone: NGINX fails to reload, a credential lands in an image layer, a generated artifact ships stale, a sanitisation guard is missing, or no test proves the new behaviour works. This applies equally to Go code, templates, the chart and workflow files -- a mutable action tag on a job holding `id-token: write` is as blocking as a Plus-only directive in an OSS template. Non-blocking means the code works today but will cost someone time later: an error that drops its cause, a workflow condition that re-derives a value already exported as a job output, a missing negative test on a non-security path. ### Fixed verdicts These recur in NIC. The verdict is settled -- do not re-litigate it per PR. | Situation | Verdict | | --- | --- | | Plus-only directive reachable from an OSS template | Blocking -- NGINX OSS refuses to start | | `.tmpl` edited, `__snapshots__` unchanged | Blocking -- no fixture exercises the new branch | | `.tmpl` edited, snapshots regenerated, but no fixture field added | Blocking -- same defect, hidden by a reformat-only diff | | Shared directive added to only one of the OSS/Plus template pair | Blocking -- edition drift | | Plus-only directive added to the Plus template only, OSS snapshot unchanged | **Not a finding** -- this is correct | | `types.go` changed without regenerated `pkg/**` or `config/crd/bases` | Blocking -- cite it even though `verify-codegen` also fails; you save a CI round-trip | | `types.go` changed without regenerated `deploy/crds*.yaml` or `docs/crd/` | Blocking -- **CI never diffs these**, so stale bundles ship silently | | Telemetry `Data`/`NICResourceCounts` changed without `make telemetry-schema` | Blocking -- `verify-codegen` fails | | New pytest marker missing from `pyproject.toml` | Blocking -- `--strict-markers` fails the entire suite, not just the new test | | `values.yaml` value's **type or shape changed** without updating `values.schema.json` | Blocking -- schema validation rejects the render and `helm install` fails | | **New** `values.yaml` key absent from `values.schema.json` | Non-blocking -- the root schema has no `additionalProperties: false`, so it installs but gets no validation. Blocking only under `hostPort`/`containerPort`, which do set it | | Plus credentials via `COPY` instead of `--secret` | Blocking -- credential persists in the image layer | | GitHub Action pinned to a tag or branch instead of a SHA | Blocking -- supply chain | | New workflow job missing its `github.repository` gate | Blocking -- `validate-workflow-gating.sh` fails and the job would run on forks | | `docker build` step added to a publish-stage workflow | Blocking -- violates the internal/public repo split | | User-controlled string reaching NGINX config with no `containsDangerousChars()`/`ValidateEscapedString()` guard | Blocking -- injection | | Security or validation path changed with no negative test | Blocking | | Error not wrapped with `%w` | Non-blocking -- **unless** a caller uses `errors.Is`/`errors.As` on it, then Blocking | | `//nolint:gosec` without a same-line justification | Non-blocking | | Missing negative test on a non-security path | Non-blocking | | Naming or duplication | Non-blocking, and only with a named drift scenario. Otherwise drop | | Formatting, import order, `golangci-lint`-enforced style | Drop -- tooling owns it | | Contents of a generated file look wrong | Drop -- comment on the source that generated it | | "This could be better" with no failure mode | Drop | | Behaviour you could not trace to a call site | Drop | | Deviation that looks deliberate but is explained nowhere | Drop | ### Tie-breaks - Two rows disagree -> the **higher** severity wins. - One defect is one bullet, even if it spans four lines. - Never soften because the PR is large, urgent, or authored by a maintainer. - More than five findings -> say so in one Summary line and list only the Blocking ones. ## Completeness gate Before producing output, confirm you have checked each row that the diff touches. A silently missing artifact is the most common real defect in this repo and the easiest to miss by only reading the diff. | If the diff touches... | Confirm the PR also contains... | | --- | --- | | Any `*.tmpl` | Regenerated `__snapshots__` **and** a new/extended fixture that renders the new directive. An unchanged snapshot after a template edit means the branch is untested -- Blocking | | A template struct (`version1/config.go`, `version2/http.go`, `version2/stream.go`) | Snapshot diff showing the field rendered | | One of `nginx.*.tmpl` / `nginx-plus.*.tmpl` | The sibling template updated, unless the directive is Plus-only -- then confirm it appears in the Plus template only | | `pkg/apis/**/types.go` | Regenerated `pkg/**` (`make update-codegen`) and `config/crd/bases` (`make update-crds`). `deploy/crds*.yaml` and `docs/crd/` are regenerated by the same target but are **not** diffed by CI -- check them by hand | | Telemetry `Data` / `NICResourceCounts` | Regenerated `internal/telemetry/*_generated.go` and `data.avdl` (`make telemetry-schema`) | | `charts/nginx-ingress/values.yaml` | Matching `values.schema.json` entry, testdata file, helmunit case, `charts/tests/__snapshots__` diff. A **changed type/shape** without a schema update is Blocking; a **new key** absent from the schema is Non-blocking | | Chart workload templates | All three of deployment / daemonset / statefulset, where the helper is shared | | New `@pytest.mark.<name>` | Marker registered in `pyproject.toml` (`--strict-markers` is on) | | Imports / dependencies | `go.mod` and `go.sum` tidy | | `.github/workflows/**` | Correct `github.repository` gate for the stage (internal repo builds, public repo publishes), pinned action SHAs, matrix JSON in sync | | A new user-controlled string reaching NGINX config | A `containsDangerousChars()` / `ValidateEscapedString()` guard **and** a negative test | An unmet row is Blocking unless the Fixed verdicts table above assigns it a lower severity. Cite the missing artifact by path. ## Change type classification Use this table to pick which domain skills to load; the referenced skill owns the up-to-date rules for that area. | Change touches | Focus for the review | Cross-reference skill | | --- | --- | --- | | CRD types (`pkg/apis/**/types.go`) | CRD field, codegen, validation | `nic-add-feature`, `nic-add-policy` | | Validation (`pkg/apis/**/validation/**`) | Validation, security (input sanitisation) | `nic-add-feature` | | Controller (`internal/k8s/**`) | Sync flow, concurrency, secret handling | `nic-structure` | | Config generation (`internal/configs/**` non-template) | Config assembly, layer boundary | `nic-structure` | | Ingress templates (`internal/configs/version1/*.tmpl`) | Template parity (OSS vs Plus), snapshot fixture + regenerated golden files, directive context per nginx.org | `nic-add-feature`, `nic-testing` | | VS/TS templates (`internal/configs/version2/*.tmpl`) | Template parity, snapshot fixture + regenerated golden files, v1-parity check, directive context per nginx.org | `nic-add-feature`, `nic-testing` | | NGINX process (`internal/nginx/**`) | Reload safety, process lifecycle | `nic-structure` | | Telemetry (`internal/telemetry/**`) | Regenerated schema, no PII in exported attributes | `nic-structure` | | Helm chart (`charts/nginx-ingress/**`) | Values <-> schema, workload template consistency, helmunit snapshot | `nic-add-feature` | | Docker (`build/Dockerfile`, `build/scripts/**`) | Layers, credential handling, base images | `nic-docker-images` | | CI (`.github/workflows/**`) | Repo gate (internal vs public), pinned SHAs, matrix JSON, secret sourcing | `nic-ci-pipelines` | | Integration tests (`tests/suite/**`) | Fixtures, markers, wait patterns | `nic-testing` |