Skip to content

REVIEW.md

Latest commit

 

History

History
83 lines (66 loc) · 3.38 KB

REVIEW.md

File metadata and controls

83 lines (66 loc) · 3.38 KB
Raw

CIRCL Code Review Checklist

What CI enforces, plus what a human reviewer should add. See AGENTS.md for rationale.

CI must be green

  • go build ./... and go test -count=1 ./... pass on amd64 for Go 1.25 and 1.26.
  • golangci-lint run is clean (config: .golangci.yaml).
  • go vet ./... and go vet -vettool=shadow ./... are clean.
  • go generate -v ./... leaves the working tree unchanged.
  • arm64, WASM (GOOS=js GOARCH=wasm), macOS, and Windows builds pass.
  • make circl_static and make circl_plugin still work if package layout changed.
  • CodeQL and Semgrep workflows have no new findings.

Security

  • No new secret-dependent branches, memory accesses, table indices, or loop bounds. crypto/subtle used for comparisons/selection on secrets.
  • No new unsafe — or written justification in the commit message and confinement to internal/.
  • crypto/rand (or an injected io.Reader) for randomness. No math/rand in crypto paths.
  • Parsers reject malformed encodings; identity elements, zero scalars, zero challenges are handled explicitly (precedents: sign/bls, ecc/bls12381, zk/qndleq, pki).
  • No new require entries unless already permitted by depguard; new ones include a depguard update.
  • gosec findings are addressed or explicitly justified (G115 is pre-excluded).

Correctness

  • Implementation cites its spec — RFC, FIPS, IETF draft, or paper (ia.cr/...) — in code comments or commit message.
  • Edge cases exercised: zero / one / identity / max-value inputs, empty slices, oversized inputs, malformed encodings.
  • KATs still pass. Regenerated golden values include a written reason and an upstream reference (see commit 91088f2).
  • Errors are returned, not swallowed. No panic on attacker-controlled input.

Testing

  • Every bug fix has a regression test that fails without the fix.
  • New crypto code has unit tests covering the documented contract.

API and compatibility

  • Exported names have godoc.
  • Additive changes are preferred. Breaking changes are called out in the PR description and paired with // Deprecated: where feasible (see dh/sidh).
  • No silent wire-format changes. If a serialized representation changes, KATs are regenerated and the change is documented.

Code generation and assembly

  • Generated packages: the template was edited (not the generated file), and make generate was run.
  • Changed .s files were produced by rerunning their generator under */internal/asm/; the diff is reproducible.
  • Any new assembly path has a pure-Go fallback (make NOASM=1 test passes).

Documentation

  • Package-level godoc explains what primitive is implemented and from which spec.
  • Non-obvious parameter choices (security level, hash, domain separation tag) are documented.
  • New top-level packages are listed in README.md under the relevant "List of Algorithms" subsection.

The README disclaims API stability — "changes in the code, repository, and API occur in the future." The bar is "correct, justified, and reproducible," not "frozen." When a change looks risky on security or correctness grounds, request a second review from someone with cryptographic background before merging.