From 65d01387ccd8fa4fe5128447f7451a8e0561d58e Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Jul 2026 20:48:54 -0700 Subject: [PATCH] docs: add REVIEW.md for KiloCode automated review guidance Built by mining 113 commits (11 fix commits over 18mo) and ~90 human PR review comments for this repo's real bug classes: pydantic extra="forbid" crashes on new dbt manifest fields, required fields breaking on disabled/ unit-test nodes, version-specific field access on vendored dbt-artifacts- parser types, checksum shape assumptions, mutable default arguments, and API-client failures not surfaced to the CLI user. Co-Authored-By: Claude Opus 4.8 --- REVIEW.md | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 REVIEW.md diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000..4439dae --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,150 @@ +# REVIEW.md — datapilot-cli + +Python CLI (`altimate-datapilot-cli` on PyPI) that parses dbt `manifest.json`/`catalog.json` +artifacts across dbt manifest schema versions (vendored `dbt-artifacts-parser`, v1–v12) and runs +data-quality "insight" checks over dbt projects, usable standalone or as a pre-commit hook. + +## Goal: catch what CI cannot + +CI runs formatting/linting/tests. You exist for correctness bugs in dbt-manifest parsing across +schema versions, node-shape assumptions, and API/CLI behavior that no linter can see. If a diff is +clean and doesn't touch any of the areas below, say `lgtm` — don't invent nits. + +## Don't duplicate CI — never flag + +This repo's `pre-commit` + `tox` pipeline (ruff, black) already enforces, so never re-flag: +- Formatting (black) or import ordering (`ruff` rule `I`/isort). +- Unused imports/variables, bare `except`, raise-from style (`RUF`, `RSE`), bugbear patterns (`B`), + comprehension style (`C4`), pathlib usage (`PTH`), quote style (`Q`), pytest style (`PT`). +- Naive `datetime` construction — ruff's `DTZ` rule already catches this. +- Basic `flake8-bandit` (`S`) security smells (e.g. `assert` in non-test code, `eval`/`exec`). +- Line length (140 cols, ruff-enforced). +- Whether tests exist/pass — `tox` runs the suite; that's CI's job, not yours. +Pre-existing issues in code the diff doesn't touch are out of scope — only review changed lines +and their direct call sites. + +## Evidence standard — no speculation + +Before flagging a manifest/catalog-parsing issue, trace which manifest version(s) the changed code +actually runs against and check the vendored schema in +`src/vendor/dbt_artifacts_parser/parsers/manifest/manifest_vN.py` for that version — don't assume +a field exists just because it's on `ManifestV12`. For CLI/API-client changes, trace whether the +call site actually surfaces failures to the user before claiming error handling is missing. + +## Severity calibration + +- **Critical**: will raise/crash on a real dbt manifest version this tool claims to support (e.g. + missing field, wrong required-ness, wrong checksum shape), or silently drops/misreports an + insight-check result. +- **Warning**: works today but is version-fragile, dialect-fragile, or duplicates logic that will + drift (see focus areas below); or an API failure that's swallowed instead of surfaced to the CLI + user. +- **Nit**: naming, magic strings that have an existing constant, minor duplication. + +## Focus areas — bug classes this repo actually ships + +1. **Pydantic `extra="forbid"` on manifest/catalog wrapper models** (root cause of #92, #48, and + related fixes). New dbt manifest versions add fields the `Altimate*` wrapper models don't know + about yet; a wrapper `BaseModel` with `extra="forbid"` raises `ValidationError` and crashes + parsing the moment dbt ships a minor version bump. Flag any new/edited model in + `schemas/manifest.py`-style files whose `model_config` sets `extra="forbid"` (or omits it while + inheriting a forbidding base) instead of `extra="allow"`. + +2. **Fields marked required on node wrapper models** (#98, #95, #96 — `identifier`, + `resource_type` made optional after crashing on `Disabled*`/unit-test nodes). A field required + on the "normal" node case often isn't populated on disabled nodes, unit-test nodes, or older + manifest versions. Flag any new required (non-`Optional`) field added to an `Altimate*` node + model; verify it's actually present across disabled/unit-test/seed/source variants, not just + the common model case. + +3. **Version-specific field access without a version guard** (e.g. `deferred` not present on v12 + `RPCNode`; behavior tied to a specific dbt/artifact-parser version). Flag direct attribute + access on a vendored `manifest_vN` type inside code paths meant to be version-agnostic; verify + against the specific `manifest_vN.py` the code targets. + +4. **Checksum/hash shape assumptions** (#94 — `checksum` is a dict on some node types, a + string/object on others). Flag `.checksum.name` / `.checksum.checksum`-style access without + confirming that node type has that structure in that manifest version. + +5. **Catalog/manifest lookups assumed non-`None`** ("catalog nodes can be `None`", catalog can + have fewer columns than the manifest). `--catalog-path` is optional per the CLI, and + `dbt docs generate` output can be stale relative to the manifest. Flag catalog/manifest + `.nodes[x]` / `.columns[y]` access without a `None`/`KeyError` guard in insight-check code. + +6. **Mutable default arguments** (explicitly flagged in review — "list is mutable, weird things can + happen" — not reliably caught by this repo's ruff config). Flag `def f(x=[])`, `def f(x={})`, or + a list/dict class attribute mutated in place across calls instead of copied per-instance. + +7. **Hardcoded materialization/dialect assumptions in insight checks** ("dialects may have + multiple materializations and it might flag something we don't want to flag", "use the VIEW + constant, we have it somewhere"). Flag magic materialization strings (`"view"`, `"table"`, + `"incremental"`) instead of the shared constant, and any check that assumes one dialect's + materialization vocabulary applies to all configured warehouses. + +8. **Duplicated logic across check/insight classes** ("put this in the base `DBTCheck` class so + it's not repeated", "make it a reusable util"). Flag near-identical blocks copy-pasted across + files under the insights/checks package instead of factored onto a shared base class or util. + +9. **API-client failures not surfaced to the CLI user** ("should handle exceptions and not-200s + nicely", "get the response code and log it", "return the status so we inform the user what + happened"). The API client intentionally doesn't raise on non-2xx (see Known-intentional) — the + bug class is the CLI layer silently swallowing that failure instead of reporting it to the + user. Flag call sites that ignore or only debug-log a non-2xx/error API response. + +10. **Hardcoded environment-specific defaults** ("needs to be injectable from the CLI/environment, + not hardcoded"). Flag new hardcoded endpoint/host/instance defaults added to config or client + code that aren't also exposed as a CLI flag or env var override. + +11. **Enum/resource-type completeness across manifest versions** ("how is it mapping between the + enums", "not sure all types have labels"). `AltimateResourceType`/`AltimateAccess`-style enums + must track new dbt node/resource types introduced by an artifact-parser version bump. Flag a + vendored-parser bump or new node type handled without a corresponding Altimate enum member. + +12. **Release/version-bump workflow trigger conditions** (`.bumpversion.cfg`, `bump-version.yml`, + `tag-release.yml` — "won't this always release a tag? should be manual"). Flag changes to + release-workflow `on:` triggers; verify a routine merge to main can't unintentionally fire a + tag/release. + +## Repo invariants & landmines + +- `Manifest = Union[ManifestV1, ..., ManifestV12]` (`schemas/manifest.py`) — code that pattern- + matches or accesses fields without checking manifest version breaks on the boundary versions. +- `src/vendor/dbt_artifacts_parser` is a deliberately vendored/forked copy (see fix history: "use + vendored dbt-artifacts-parser", "use forked dbt-artifacts-parser") — it exists because upstream + didn't support something needed here; don't "clean up" divergence from upstream as if it were + drift. +- Two distinct run modes with different data completeness: a full CLI run (`datapilot dbt + project-health`) vs. the pre-commit hook, which partially generates manifest/catalog from only + changed files. Manifest-loading changes need to work under both. +- Minimum supported Python is 3.7 per README — avoid relying on syntax/stdlib features unavailable + there unless the actual tox/CI matrix has been checked. +- `--catalog-path` is optional; code paths must degrade gracefully (skip catalog-dependent + insights) rather than assume catalog data is present. + +## Known-intentional — don't flag + +- `APIClient.get()` not raising on non-2xx responses — deliberate design ("safer as it passes + things through," per review). Only flag if the *caller* drops the failure silently (focus area + 9), not the client's non-raising behavior itself. +- `print()` used for CLI-facing output — confirmed intentional ("this is the intended output, not + really logging"). Don't suggest converting these to `logger` calls. +- The manifest wrapper not modeling unit-test nodes in certain paths — confirmed as not a real gap + where raised ("there's no unit_test/UnitTest reference in the wrapper code" — deliberate scope). + +## High-blast-radius — never auto-edit + +- `src/vendor/dbt_artifacts_parser/**` — vendored upstream parser; changes must be deliberate forks + with a stated reason, not incidental "fixes." +- `.bumpversion.cfg`, `bump-version.yml`, `tag-release.yml`, `release.sh`, `setup.py` — PyPI release + plumbing for `altimate-datapilot-cli`. +- `pyproject.toml`, `tox.ini`, `.pre-commit-config.yaml` — changing these changes what's enforced + repo-wide, not just this PR. +- `CHANGELOG.rst`, `AUTHORS.rst` — maintained by release tooling, not hand-edited in feature PRs. + +## Comment style + +- Point at exact lines; one finding per comment; most-severe first. +- Phrase as suggestions the author can accept or reject, not mandates — link a `diff` snippet if a + concrete fix is short. +- Skip deep review of trivial diffs (README/CHANGELOG-only, formatting-only, dependency bumps with + no vendored-parser impact).