docs: correct REVIEW.md inaccuracies flagged by automated review

- Scope manifest-version support correctly: raw schema types span
  v1-v12, but insight-check wrapper support (DBTFactory.get_manifest_wrapper)
  is v10-v12 only
- Stop telling reviewers to suppress `assert` findings — `S101` is ignored
  in ruff config, so CI does not actually catch assert in non-test code
- Clarify Pydantic v2 semantics: `Optional[str]` without a default is
  still a required field, not automatically optional
- Fix Python floor from stale README claim (3.7) to the actually enforced
  3.8 (setup.py python_requires, pyproject/tox target-version)
- Correct pre-commit hook invariant: it always loads the full
  manifest/catalog; process_changed_files only narrows selected models,
  it does not generate partial artifacts
- Trim some unrelated focus-area prose to keep the file under 10,000 chars

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: anandgupta42

REVIEW.md

@@ -1,8 +1,10 @@
 # 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
+artifacts (vendored `dbt-artifacts-parser` schema types span manifest v1–v12) and runs
 data-quality "insight" checks over dbt projects, usable standalone or as a pre-commit hook.
+Insight-check processing itself is narrower — `DBTFactory.get_manifest_wrapper` only wraps
+v10/v11/v12, raising `AltimateNotSupportedError` for older versions.
 
 ## Goal: catch what CI cannot
 
@@ -17,7 +19,9 @@ This repo's `pre-commit` + `tox` pipeline (ruff, black) already enforces, so nev
 - 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`).
+- `eval`/`exec` usage (`flake8-bandit` `S102`/`S307`, still enforced). Exception: `S101` (assert)
+  is ignored in `pyproject.toml`, so CI does NOT catch `assert` in non-test code — flag it (it also
+  vanishes under `python -O`).
 - 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
@@ -53,9 +57,10 @@ call site actually surfaces failures to the user before claiming error handling
 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.
+   manifest versions. Note: with `pydantic>=2.0`, `Optional[str]` without a default (`= None`) is
+   still required — don't treat `Optional`-typed as automatically safe. Flag any new required 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
@@ -71,39 +76,36 @@ call site actually surfaces failures to the user before claiming error handling
    `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.
+6. **Mutable default arguments** (flagged in review — 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.
+7. **Hardcoded materialization/dialect assumptions in insight checks** (some dialects support
+   multiple materializations for a given config; use the shared `VIEW`-style constant). 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.
+9. **API-client failures not surfaced to the CLI user**. 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.
+10. **Hardcoded environment-specific defaults**. 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.
+11. **Enum/resource-type completeness across manifest versions**.
+    `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.
+    `tag-release.yml`). Flag changes to release-workflow `on:` triggers; verify a routine merge to
+    main can't unintentionally fire a tag/release.
 
 ## Repo invariants & landmines
 
@@ -113,11 +115,14 @@ call site actually surfaces failures to the user before claiming error handling
   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.
+- Two run modes, both load the *full* manifest/catalog from disk (`load_manifest_file`/
+  `load_catalog_file`, default `target/manifest.json`/`target/catalog.json`): a full CLI run
+  (`datapilot dbt project-health`) vs. the pre-commit hook. The hook's `process_changed_files` only
+  narrows which models get insight-checked — it does not generate a partial manifest/catalog.
+  Manifest-loading changes need to work under both.
+- Minimum supported Python is 3.8 (`setup.py` `python_requires=">=3.8"`, `pyproject.toml`/`tox.ini`
+  `target-version = "py38"`) — README's "3.7 or higher" is stale; avoid syntax/stdlib features
+  unavailable in 3.8 unless the 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.