Teach the OpenAPI analyzer to inventory Rust module trees - #390
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ClickHouse OpenAPI drift analyzer to inventory entire Rust module trees (inline + out-of-line), and switches all callers (Rust tests + Python drift tooling) from passing individual files (client.rs/models.rs/meta.rs) to passing a single --source-root directory.
Changes:
- Add Rust module-tree loading to the analyzer (module traversal,
#[cfg]/#[cfg_attr]evaluation, and#[path]handling) so public API declared behind private facades is still inventoried. - Update analyzer entrypoints/callers (Rust library API, analyzer binary CLI, Python drift script) to use
rust_source_root/--source-root. - Add fixtures + tests for nested modules, cfg gating, and path resolution; update spec/policy tests to use analyzer inventory instead of substring scanning.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/test_check_openapi_drift.py | Adds coverage ensuring the Python drift script invokes the analyzer with --source-root (and no longer passes --client/--models/--meta). |
| scripts/check-openapi-drift.py | Switches drift-script analyzer invocation to pass --source-root pointing at the Cloud API src/ tree. |
| crates/clickhouse-openapi-analyzer/src/rust_inventory.rs | Implements module-tree loading/traversal and cfg/path evaluation; expands inventory to operate across the full tree. |
| crates/clickhouse-openapi-analyzer/src/main.rs | Replaces per-file CLI args with a single --source-root directory argument. |
| crates/clickhouse-openapi-analyzer/src/lib.rs | Replaces AnalysisInput per-file strings with rust_source_root: &Path; updates public helper APIs accordingly. |
| crates/clickhouse-openapi-analyzer/tests/entrypoint_test.rs | Updates executable/library parity test to use the source-tree boundary and validates report schema version. |
| crates/clickhouse-cloud-api/tests/spec_coverage_test.rs | Updates spec-coverage/policy tests to use response_tree(Path) + analyzer-backed model inventory instead of raw source scanning. |
| crates/clickhouse-cloud-api/tests/models_test.rs | Updates the “manual Default impl coverage” test to use the model module tree input. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/client.rs | Fixture: minimal client root for #[path] resolution tests. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/lib.rs | Fixture: basic crate root (not compiled) supporting module-tree tests. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/meta.rs | Fixture: metadata constants root. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/models.rs | Fixture: exercises #[path] in root + inline modules and relocated module directories. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/model_files/direct.rs | Fixture: direct #[path = "..."] model module. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/models/inline/renamed.rs | Fixture: nested #[path] inside inline module context. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/path_context/relocated/leaf.rs | Fixture: nested inline module under a relocated #[path] directory. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/nested/client.rs | Fixture: nested client module layout with private facade + re-exports. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/nested/client/operations/widgets.rs | Fixture: operation method in a nested module to verify impl Client inventory works across modules. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/nested/models.rs | Fixture: nested models facade + re-export. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/nested/models/domain.rs | Fixture: nested structs/enums/aliases to validate structural and serde inventory across modules. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/nested/meta.rs | Fixture: nested metadata module with re-exported constants. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/flat/client.rs | Fixture: flat baseline equivalent of nested client layout. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/flat/models.rs | Fixture: flat baseline equivalent of nested model layout. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/flat/meta.rs | Fixture: flat baseline equivalent of nested meta layout. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/client.rs | Fixture: cfg-gated client modules to validate cfg evaluation and test-module exclusion. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/models.rs | Fixture: cfg/cfg_attr/path/feature/custom-cfg scenarios for module loading + inventory behavior. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/meta.rs | Fixture: cfg-gated meta modules to validate exclusion of test-only modules. |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/production.rs | Fixture: selected module via cfg_attr(..., path=...). |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/platform_unix.rs | Fixture: platform-selected module via cfg_attr(target_family=...). |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/platform_windows.rs | Fixture: platform-selected module via cfg_attr(target_family=...). |
| crates/clickhouse-openapi-analyzer/tests/fixtures/module_tree/cfg_modules/platform_other.rs | Fixture: fallback platform-selected module via cfg_attr(not(any(...))). |
| AGENTS.md | Updates repo documentation to describe module-tree inventory behavior. |
Suppressed comments (1)
crates/clickhouse-openapi-analyzer/src/rust_inventory.rs:381
current_target_vendor()hardcodes only a few vendor strings, but Rust targets have many validtarget_vendorvalues. When the vendor is unrecognized,cfg(target_vendor = "...")becomesUnknown(treated as active) and can incorrectly load/inventory modules. Preferoption_env!("CARGO_CFG_TARGET_VENDOR")to reflect the actual compilation target.
fn current_target_vendor() -> Option<&'static str> {
if cfg!(target_vendor = "apple") {
Some("apple")
} else if cfg!(target_vendor = "pc") {
Some("pc")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9d3d9ae to
604e83a
Compare
|
|
||
| fn collect_metadata(&mut self, file: &syn::File) { | ||
| for item in &file.items { | ||
| fn collect_metadata(&mut self, items: &[Item]) { |
There was a problem hiding this comment.
🟡 Medium src/rust_inventory.rs:709
collect_metadata lets a nested or private module's BETA_OPERATIONS or DEPRECATED_FIELDS overwrite the intended metadata values, producing incorrect drift results. Because ModuleTree supplies flattened items and this code matches only unqualified names, restrict collection to the intended exports or preserve module boundaries.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhouse-openapi-analyzer/src/rust_inventory.rs around line 709:
`collect_metadata` lets a nested or private module's `BETA_OPERATIONS` or `DEPRECATED_FIELDS` overwrite the intended metadata values, producing incorrect drift results. Because `ModuleTree` supplies flattened items and this code matches only unqualified names, restrict collection to the intended exports or preserve module boundaries.
| fn collect_models(&mut self, items: &[Item]) -> syn::Result<()> { | ||
| for item in items { | ||
| match item { | ||
| Item::Struct(item_struct) if matches!(item_struct.vis, Visibility::Public(_)) => { |
There was a problem hiding this comment.
🟡 Medium src/rust_inventory.rs:585
Private helpers such as pub struct Helper inside a non-public, non-re-exported mod internal are added to model_types and checked as public API models. Because collect_models receives the flattened module items without filtering module visibility or re-exports, policy and drift checks report false findings for types users cannot access. Preserve module/re-export visibility when building the inventory, or filter these items before collect_models.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhouse-openapi-analyzer/src/rust_inventory.rs around line 585:
Private helpers such as `pub struct Helper` inside a non-public, non-re-exported `mod internal` are added to `model_types` and checked as public API models. Because `collect_models` receives the flattened module items without filtering module visibility or re-exports, policy and drift checks report false findings for types users cannot access. Preserve module/re-export visibility when building the inventory, or filter these items before `collect_models`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 604e83a. Configure here.
| } else { | ||
| path.with_extension("") | ||
| } | ||
| } |
There was a problem hiding this comment.
Path-loaded modules resolve children wrong
Medium Severity
load_file always derives the child-module directory with child_module_dir, which treats any non-mod.rs file as non-mod-rs. In rustc, a file loaded through #[path = "..."] is treated as mod-rs, so nested mod child; declarations resolve as siblings of that file, not under a same-named subdirectory. Path-loaded modules that declare further out-of-line children will miss those files or load the wrong ones, so inventory can diverge from the real crate.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 604e83a. Configure here.


Closes #380
Summary
Tests
cargo fmt --all --checkcargo test -p clickhouse-cloud-api -p clickhouse-openapi-analyzercargo clippy -p clickhouse-cloud-api -p clickhouse-openapi-analyzer --all-targets -- -D warningspython3 -m unittest discover -s scripts/tests -p 'test_*.py'