docs: add REVIEW.md for KiloCode automated review guidance#104
docs: add REVIEW.md for KiloCode automated review guidance#104anandgupta42 wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by glm-5.2 · Input: 11.5K · Output: 1K · Cached: 83.3K |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d01387cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - 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`). |
There was a problem hiding this comment.
This tells Kilo not to flag assert in non-test code because the Bandit S rules supposedly catch it, but pyproject.toml explicitly ignores S101 (flake8-bandit assert). In diffs that add runtime assertions, CI will not report them, and this guidance would also suppress the reviewer from catching code paths that can disappear under python -O; please either remove assert from the never-flag examples or enable the rule.
Useful? React with 👍 / 👎.
| 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 |
There was a problem hiding this comment.
This equates required fields with non-Optional, but the package uses Pydantic 2 (setup.py:66 requires pydantic >=2.0,<3.0), where identifier: Optional[str] is still required unless it also has a default such as = None. A node wrapper can therefore introduce the same disabled/unit-test missing-field crash while satisfying this guidance, so the reviewer should check for Pydantic-required fields rather than just non-Optional annotations.
Useful? React with 👍 / 👎.
| - 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 was a problem hiding this comment.
This sets Kilo's compatibility floor to Python 3.7 based on README text, but the install metadata is python_requires=">=3.8" in setup.py:63 and the tooling targets py38+ (pyproject.toml:40/:52). With this guidance, the reviewer will flag valid Python 3.8-only code as incompatible with a version users cannot install, creating false positives instead of catching real compatibility regressions.
Useful? React with 👍 / 👎.
| # 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 |
There was a problem hiding this comment.
This describes the CLI as handling manifest schemas v1-v12, but DBTFactory.get_manifest_wrapper only creates wrappers for ManifestV10, ManifestV11, and ManifestV12 and raises AltimateNotSupportedError for older manifests. Because later guidance treats crashes on any claimed-supported version as Critical, Kilo will review diffs against v1-v9 shapes that project-health cannot run; please separate parser coverage from insight-wrapper support or narrow this to v10-v12.
Useful? React with 👍 / 👎.
| project-health`) vs. the pre-commit hook, which partially generates manifest/catalog from only | ||
| changed files. Manifest-loading changes need to work under both. |
There was a problem hiding this comment.
The current pre-commit path loads the configured manifest/catalog files in executor_hook.py before calling process_changed_files, and that helper only returns the changed file names; it does not generate partial manifest/catalog objects from those files. This invariant will make Kilo require manifest-loading changes to handle a data shape the active hook no longer produces, while the real requirement is full manifest plus optional/missing catalog.
Useful? React with 👍 / 👎.
1 participant
What this is
REVIEW.mdis repo-specific guidance for the KiloCode automated code reviewer. Kilo reads it fromthe PR base branch, so it activates once this merges to
main(enable Use REVIEW.md in Kilo'srepo settings).
How it was built
Mined this repo's history in one bounded pass: 113 commits, 11 fix commits over the last 18
months, and ~90 human PR review comments. The focus areas are grounded in that evidence, not
generic advice.
Top focus areas
extra="forbid"on manifest/catalog wrapper models — new dbt manifest versions add fieldsthe wrapper doesn't know about yet; forbidding extras crashes parsing on a routine dbt bump.
nodes, and older manifest versions routinely miss fields the "normal" node case has.
deferreddon't exist on everymanifest version's node types (e.g. v12
RPCNode).checksumis a dict on some node types, a string/object onothers.
None—--catalog-pathis optional and can be stalerelative to the manifest.
called out explicitly in past human review.
Also documents known-intentional patterns (e.g. the API client deliberately not raising on non-2xx,
print()used for CLI output) so the reviewer doesn't re-litigate settled decisions, and marks thevendored
dbt_artifacts_parsercopy and release/version-bump plumbing as high-blast-radius.🤖 Generated with Claude Code
Note
Low Risk
Documentation-only change with no runtime, release, or dependency impact.
Overview
Adds a new
REVIEW.mdat the repo root so KiloCode (with Use REVIEW.md enabled) can review PRs against datapilot-cli–specific rules instead of generic lint-style feedback.The doc tells the reviewer to focus on manifest/catalog correctness (Pydantic
extra="forbid", optional vs required node fields, version-guarded field access, checksum shapes, optional catalog paths), insight-check pitfalls (mutable defaults, duplicated check logic, dialect/materialization magic strings), and CLI/API surfacing (non-2xx not swallowed). It also lists repo invariants (multi-versionManifestunion, vendored parser, pre-commit vs full CLI runs, Python 3.7), known-intentional patterns (API client not raising,print()for output), and high-blast-radius paths the bot should not auto-edit.CI overlap is explicitly out of scope (ruff/black/tox already cover formatting and style), with severity calibration and comment-style guidance so reviews stay evidence-based and scoped to changed lines.
Reviewed by Cursor Bugbot for commit 65d0138. Bugbot is set up for automated code reviews on this repo. Configure here.