--- title: "Codebase audit" description: "Checks a whole repository: CI gates, dependencies, setup, and conventions." canonical: "https://goodcodereview.com/codebase" updated: "2026-10-25" --- # codebase ## code-adr severity: discuss concerns: maintainability, process applies_when: Auditing whether the reasons behind significant design choices are recorded. **Rule.** Keep a short written record for each significant design choice (datastore, framework, deviation from a pattern) that states the context, the decision, and its consequences. **Why.** Without a record, the next reader reverses a decision blindly or cargo-cults it. Also list the options you ruled out and why: a record that weighs only the chosen design, its strengths or its flaws, is not a trade-off analysis. **FAIL** ```text Repo uses Kafka for one 10-msg/day job. No record why; nobody dares remove it. ``` **PASS** ```text docs/adr/0007-queue.md: context, decision, consequences, status, and the 2 options ruled out. ``` **Counterpoint.** ADRs rot and become paperwork. Keep each to a page, write them only for hard-to-reverse choices, and mark superseded ones. ## code-build-hermetic severity: should-fix concerns: process applies_when: The repository has a build, a packaging, or a release step. **Rule.** Build from declared inputs only, so that the same source revision gives the same artifact. **Why.** A build that reads whatever is installed on the machine gives different output on different machines, so a bug cannot be reproduced and a release cannot be checked against its source. Tools are inputs too: pin the compiler and toolchain versions, and do not call host binaries. Undeclared shared files also break builds at scale, for example two tasks that clean and use the same `/tmp` directory race each other. **FAIL** ```yaml steps: - run: make # uses whatever gcc, node and protoc the runner has - run: date > BUILD_ID # the clock is an undeclared input ``` **PASS** ```yaml container: image: ghcr.io/acme/builder@sha256:4b1e... # toolchain pinned by digest steps: - run: SOURCE_DATE_EPOCH=$(git log -1 --format=%ct) make # timestamps come from the commit ``` **Counterpoint.** "Bit-identical builds are overkill." Full reproducibility is the ceiling. Declared inputs, pinned tools, and a recorded revision are the floor, and they cost little. [code-dependency-pinning](codebase.md#code-dependency-pinning) covers dependency pins. ## code-ci-gates severity: should-fix concerns: process applies_when: Auditing which checks must pass before a change merges. **Rule.** Require review and passing build, tests, lint, and type checks on the default branch before merge, and leave nothing mechanical to reviewer vigilance. **Why.** Any rule that depends on someone remembering will be skipped. Components where changes merge unreviewed or without discussion tend to ship more post-release defects. **FAIL** ```text Default branch: no protection. Force-push allowed. CI optional. ``` **PASS** ```text Protected main: 1 approval + required checks (build, test, lint, types). ``` **Counterpoint.** Gates slow small teams, and flaky tests train people to override. Keep gates few and reliable, fix flakes instead of overriding them, and let the gates own the mechanical checks so review time goes to design. ## code-consistency-drift severity: discuss concerns: correctness, maintainability applies_when: Auditing whether one concern is solved in several incompatible ways across the repo. **Rule.** For each recurring concern (errors, config, HTTP clients, logging), make one pattern the standard instead of leaving several as equal peers. **Why.** Each extra variant is another thing to learn, test, and secure, and drift accumulates through changes that each looked locally fine. Two limiters built two ways enforce the same quota differently. List the older variants and count them, so the migration shows progress and a new one shows up in review. **FAIL** ```text Three HTTP clients (requests, urllib3, httpx) with three timeout conventions; no document says which is current. ``` **PASS** ```text docs: "use httpx via net/client.py". Lint bans direct requests imports; legacy sites listed in MIGRATION.md. ``` **Counterpoint.** "Forcing one way can block a better tool." Allow change through a proposal that updates the standard and the migration list, not through a quiet second pattern. Consistency breaks ties. It does not outrank a measured reason to switch. ## code-dead-code severity: should-fix concerns: maintainability applies_when: Auditing for code, flags and files that nothing uses. **Rule.** Remove unreferenced functions, commented-out code, expired flags, and orphaned files; history keeps them. **Why.** Dead code costs reading time, misleads searches, and gets "fixed" and tested for no benefit. **FAIL** ```text # def old_export(): ... (commented out since 2022) if FLAGS["legacy_ui"]: # flag permanently false since release 4 ``` **PASS** ```text Code deleted; git log -S shows where it went. Detector reports 0 unused symbols or each is allowlisted with a reason. ``` **Counterpoint.** Code may be called by reflection, plugins, or external users. Check with the detector's config and a grep of consumers. For public API, deprecate first (see `chg-expand-contract`). ## code-dependency-licence-sbom severity: should-fix concerns: security applies_when: Auditing what third-party code the repo ships and under which terms. **Rule.** Generate an SBOM, and scan dependencies for known vulnerabilities on every change or on a schedule. **Why.** Without an inventory, a team cannot answer a disclosed vulnerability quickly. CI is the usual place for both steps: the SBOM then matches what was built, and the scan runs without anyone remembering it. Team policy: also fail the build on a dependency whose licence is not on the approved list. **FAIL** ```text No SBOM anywhere, no scanner in CI, and dependency alerts turned off. ``` **PASS** ```text CI: generate SBOM, run vulnerability scanner, fail on high severity; release artifacts attach the SBOM. ``` **Counterpoint.** SBOMs and scanners create noise that nobody triages. Tune severity thresholds and assign an owner, since an ignored report is worse than none. The licence list is team policy, not a universal rule. Drop it if your team has no licence constraints. ## code-dependency-pinning severity: block concerns: security applies_when: Auditing how the repo resolves third-party packages. **Rule.** Commit a lockfile with hashes and install from it in CI. **Why.** Unpinned versions make builds non-reproducible and let a compromised release flow straight into production. Updates should arrive as reviewed, staged changes, not silently. [code-workflow-supply-chain](codebase.md#code-workflow-supply-chain) covers CI action pins. **FAIL** ```text requirements.txt: requests>=2 (no lockfile; CI runs pip install -U) ``` **PASS** ```text requirements.lock with hashes; CI: pip install --require-hashes -r requirements.lock Update bot opens PRs; changes are rolled out in stages. ``` **Counterpoint.** Pinning delays security patches. A pin plus an update bot with a defined cadence beats floating versions, because the lock makes the update visible and revertable. ## code-perf-budget-in-ci severity: should-fix concerns: performance, process applies_when: The repo has a performance target (bundle size, page metrics, latency under load, or a benchmark) that matters to users. **Rule.** Write the performance budget down as a CI check that fails the build when it is exceeded. **Why.** Performance regresses a few percent at a time, and no reviewer sees a 3% slowdown in a diff. A budget that only warns becomes a log line nobody reads. **FAIL** ```text lighthouserc.json: "assertions": { "categories:performance": ["warn", { "minScore": 0.9 }] } k6 script with no thresholds block ``` **PASS** ```text size-limit: [{ "path": "dist/app.js", "limit": "180 kB" }] # npm run size in CI k6: thresholds: { http_req_duration: ["p(95)<300"] } # non-zero exit fails the job ``` **Counterpoint.** "Benchmarks in CI are noisy and flake." They are when a gate trusts one run. Gate on a stable budget, compare repeated runs (benchstat wants 10 to 20), and keep the threshold above the noise. On a site with much room to improve, a tight budget only turns every build red, so set it at today's level and lower it later. ## code-rule-has-enforcer severity: should-fix concerns: maintainability applies_when: Auditing a repo's instruction files, review history or comments that state a rule. **Rule.** Back each rule that a type, lint or CI check could enforce with that check, not with a line in an instruction file or a repeated review comment. **Why.** A rule that relies on memory gets broken, and reviewers then repeat the same correction. An instruction file is advisory: a person or an agent can skip it. A lint message that names the fix teaches it at the moment of the mistake. A "do not remove" code comment has the same weakness as an instruction file line. **FAIL** ```text CONTRIBUTING.md: "never call requests.get without a timeout" (no lint rule) Review history: the same comment on 6 PRs ``` **PASS** ```toml # pyproject.toml, checked by ruff in CI [tool.ruff.lint] extend-select = ["S113"] # request-without-timeout ``` **Counterpoint.** "Not every rule is checkable." Right, and judgement rules stay as prose. The FAIL is a checkable rule with no check. See [code-ci-gates](codebase.md#code-ci-gates) for where checks run, and [chg-review-signal](change.md#chg-review-signal) for what reviewers stop commenting on once a tool owns it. ## code-secret-scan severity: block concerns: security, process applies_when: Any repository, including its history, issues and pull requests. **Rule.** Scan for committed credentials, and treat any hit as leaked: revoke and replace it first. **Why.** A secret in history is readable by everyone with clone access, and forks and caches keep it after a rewrite. Rewriting history is slow and often unnecessary once the secret is dead. **FAIL** ```text git log -p | rg 'AKIA' # a live cloud key in an old commit; "removed it in the next commit" ``` **PASS** ```text scanner runs on every push and covers history key revoked and reissued the same day; new value comes from the secret store ``` **Counterpoint.** "Scanners give false positives." Tune with an allowlist of reviewed, non-secret strings. A missed live key costs far more than a triaged alert. [sys-secrets-and-errors](system.md#sys-secrets-and-errors) covers where secrets live at runtime. ## code-workflow-supply-chain severity: block concerns: security, process applies_when: Auditing CI workflow files and release pipelines. **Rule.** Pin every CI action to a full commit SHA. **Why.** The pipeline holds secrets and publish rights, so a compromised action or injected input owns the artifact. Also give workflows least-privilege tokens, keep untrusted event data out of `run:` and out of `pull_request_target` checkouts, and attach build provenance to releases. [code-dependency-pinning](codebase.md#code-dependency-pinning) covers dependency pins, and [code-dependency-licence-sbom](codebase.md#code-dependency-licence-sbom) covers the inventory. **FAIL** ```yaml steps: - uses: actions/checkout@v4 # tag, can be moved to other code - uses: some/action@main # branch, changes on every push ``` **PASS** ```yaml permissions: contents: read steps: - uses: actions/checkout@<40-char-sha> # v4.2.2 - uses: some/action@<40-char-sha> # v3.1.0 ``` **Counterpoint.** "SHA pinning blocks automatic fixes and adds update churn." Let an update bot propose SHA bumps, so fixes still arrive as reviewed changes. Where the platform offers a lockfile for workflow dependencies, that lockfile is stronger than SHA pins and meets this check.