Skip to content

Refit OpenAPI drift checks around shared syn analyzer#297

Merged
sdairs merged 11 commits into
issue-275-enum-value-driftfrom
codex/issue-295-openapi-drift-analyzer
Jul 15, 2026
Merged

Refit OpenAPI drift checks around shared syn analyzer#297
sdairs merged 11 commits into
issue-275-enum-value-driftfrom
codex/issue-295-openapi-drift-analyzer

Conversation

@sdairs

@sdairs sdairs commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Closes #295.

Stacked on #290 — this PR targets issue-275-enum-value-drift so its diff contains only the analyzer refit. After #290 merges, rebase this branch onto main and retarget the PR.

Follow-up model/API remediation for the 11 acknowledged scalar-backed enum constraints is tracked in #296.

Problem

The snapshot coverage test and daily Python workflow independently parsed Rust source and implemented the same code-vs-spec comparisons. That duplicated syntax handling, Serde interpretation, exemptions, and drift semantics in two fragile line/regex-based implementations.

What changed

  • Added a private, non-published clickhouse-openapi-analyzer workspace crate.
  • Parse client.rs, models.rs, and meta.rs structurally with syn.
  • Return a deterministic, versioned, serializable DriftReport with stable OpenAPI JSON pointers and Rust item locations.
  • Preserve operation, model, field, optionality, beta, deprecation, enum-value, snapshot, and stale-exemption findings.
  • Inventory enums under array items, operation parameters, and composition nodes.
  • Report numeric or scalar-backed enum constraints explicitly; acknowledge the 11 current snapshot locations while making new or stale acknowledgements actionable.
  • Reduce spec_coverage_test.rs and check-openapi-drift.py to thin consumers of the same report.
  • Keep Python responsible only for fetching, issue rendering, and GitHub orchestration.
  • Extend CI, add Rust/Python fixtures and entry-point parity coverage, and update development documentation.

Impact

There are no CLI behavior changes or public runtime API changes. syn remains isolated to the private analyzer/dev dependency path. The vendored snapshot produces zero actionable findings and 11 visible acknowledged diagnostics.

Verification

  • cargo build --workspace
  • cargo test --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • python3 -m unittest discover -s scripts/tests -p 'test_*.py'
  • Confirmed the analyzer crate is absent from both published crates' normal dependency graphs

@sdairs sdairs temporarily deployed to cloud-integration July 11, 2026 09:20 — with GitHub Actions Inactive
@sdairs sdairs marked this pull request as ready for review July 11, 2026 09:21
@sdairs sdairs requested a review from iskakaushik as a code owner July 11, 2026 09:21
Comment thread crates/clickhouse-cloud-api/tests/spec_coverage_test.rs
@sdairs sdairs temporarily deployed to cloud-integration July 11, 2026 09:46 — with GitHub Actions Inactive
@sdairs sdairs requested a review from rndD July 11, 2026 09:53
@sdairs

sdairs commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Code review findings

Reviewed the shared-analyzer refit. Verified locally: analyzer tests + entrypoint parity, spec_coverage_test (0 actionable drift), clippy -D warnings, and the Python unittest suite all pass. Three non-blocking findings:

1. MissingReferencedModelType is redundant with MissingModelType and double-counts in the issue summarycompare.rs:67-113
Loop 1 emits MissingModelType for every defined schema lacking a Rust model. Loop 2 emits MissingReferencedModelType for any referenced schema that is defined but lacks a model. Since loop 1 already iterates all defined schemas, every MissingReferencedModelType is necessarily also a MissingModelType — the referenced branch's only unique value is its MissingSchemaDefinition case (referenced-but-undefined). Result: two findings per missing type, and the Python summary inflates the count via total('missing_model_type', 'missing_referenced_model_type') (check-openapi-drift.py:190). Cosmetic on the clean snapshot, but the "Missing model types" count will be wrong during real drift. Suggest dropping the MissingReferencedModelType branch (keep only MissingSchemaDefinition in loop 2), or skipping it when the schema is also in spec.schemas.

2. Nested-object property enums are attributed to the top-level propertyopenapi.rs:321-328
enum_context sets property = path[4] for anything under .../properties/<x>/.... For a nested shape like Widget.properties.foo.properties.bar with an enum on bar, the constraint is attributed to field foo, and map_enum resolves foo's type — likely producing a spurious UnsupportedEnumConstraint ("not an enum") or a wrong mapping if foo happens to be an enum. Not exercised by the current snapshot, but since this is now the canonical checker, a comment noting the "first properties segment wins" limitation (or handling the deepest segment) would prevent a confusing future false-positive.

3. Daily openapi-drift.yml has no Rust build cache (perf only).github/workflows/openapi-drift.yml
The workflow now cold-compiles the analyzer via cargo run every day with no Swatinem/rust-cache step (contrast test-cloud-api.yml). Correctness is fine; it's just wasted CI minutes. Optional.

Only #1 has a behavioral consequence (a double-count during actual drift), and it's a small localized fix. Nice single-sourcing overall — the executable-vs-library parity test plus Python-only-consumes-JSON contract match what AGENTS.md promises.

@sdairs

sdairs commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Review findings for head aa8b14f:

  1. [P2] Mirror Serde's distinct field and variant rename rules. In rust_inventory.rs, apply_rename_rule is used for both struct fields and enum variants, but Serde applies rename_all differently to each. It also handles acronym-heavy variants differently: under snake_case, Serde serializes HTTPServer as h_t_t_p_server, while the analyzer records httpserver. This can produce false missing/extra enum findings. Please split field and variant transformations and mirror serde_derive's rules.

  2. [P2] Restrict enum discovery to schema-bearing OpenAPI nodes. In openapi.rs, the recursive walk treats every object containing an array-valued enum key as an enum constraint. That includes arbitrary example/default payloads and extensions. A valid example such as {"enum": ["sample"]} would become an unsupported or incorrectly mapped constraint and fail the drift workflow. Traversal should be limited to actual schema locations and their items/composition children.

Verification completed successfully:

  • cargo test --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • Python drift-rendering tests
  • Confirmed the analyzer is absent from clickhouse-cloud-api's normal dependency graph

The existing Cursor finding about the live test including snapshot drift appears intentional: the live consumer is expected to surface stale snapshot operation/schema findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the ClickHouse Cloud OpenAPI drift detection to rely on a single, shared Rust analyzer crate (syn-based) and makes both the snapshot test (spec_coverage_test.rs) and the daily live-spec GitHub-issue workflow (check-openapi-drift.py) consume the same versioned DriftReport.

Changes:

  • Adds a private workspace crate (clickhouse-openapi-analyzer) that inventories Rust + OpenAPI and emits a deterministic, serializable drift report.
  • Replaces the previous duplicated Rust/Python parsing/comparison logic with thin consumers (Rust test + Python workflow/renderer).
  • Updates CI/workflows and documentation to build/test the analyzer and validate issue rendering.

Reviewed changes

Copilot reviewed 22 out of 23 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 unit tests for the Python drift script’s report grouping, rendering, and error handling.
scripts/resolve-field-requirements.py Updates comments to clarify the analyzer config is the canonical policy source for partial required schemas.
scripts/regenerate-deprecated-fields.py Updates script guidance to reference analyzer-driven drift reporting.
scripts/regenerate-beta-lists.py Updates script guidance to reference analyzer-driven drift reporting.
scripts/check-openapi-drift.py Reworks the drift workflow script to fetch the live spec, invoke the Rust analyzer, and render a GitHub issue from the shared report.
crates/clickhouse-openapi-analyzer/tests/entrypoint_test.rs Ensures the analyzer binary and library produce identical reports for the vendored snapshot.
crates/clickhouse-openapi-analyzer/src/rust_inventory.rs Implements syn-based Rust source inventory (client methods, model structs/enums/aliases, serde/cfg metadata).
crates/clickhouse-openapi-analyzer/src/report.rs Defines the versioned DriftReport schema and stable sorting/deduping/rendering behavior.
crates/clickhouse-openapi-analyzer/src/openapi.rs Implements OpenAPI inventory (operations, schemas/properties, refs, beta/deprecation markers, enum constraints with stable JSON pointers).
crates/clickhouse-openapi-analyzer/src/main.rs Adds the openapi-drift-analyzer CLI entrypoint used by Python automation.
crates/clickhouse-openapi-analyzer/src/lib.rs Exposes the analyzer library API (analyze) and error types for reuse by tests/automation.
crates/clickhouse-openapi-analyzer/src/config.rs Centralizes ClickHouse-specific comparison policy/exemptions and acknowledged unsupported enum pointers.
crates/clickhouse-openapi-analyzer/src/compare.rs Implements the spec↔code drift comparisons and report generation, including stale-exemption detection.
crates/clickhouse-openapi-analyzer/Cargo.toml Declares the private analyzer crate + dependencies (syn/clap/serde/etc).
crates/clickhouse-cloud-api/tests/spec_coverage_test.rs Replaces bespoke parsing assertions with a thin analyzer invocation for snapshot and live-spec checks.
crates/clickhouse-cloud-api/src/meta.rs Updates docs to reflect drift is now reported by the shared analyzer.
crates/clickhouse-cloud-api/README.md Documents the new analyzer crate, workflow, and commands for drift checks/regeneration.
crates/clickhouse-cloud-api/Cargo.toml Adds the analyzer as a dev-dependency for snapshot/live drift tests.
Cargo.toml Adds the analyzer crate to the workspace members list.
Cargo.lock Locks analyzer dependencies (syn/quote bumps, new crate entries).
AGENTS.md Updates internal developer guidance to describe the analyzer-driven drift workflow and config/exemptions.
.github/workflows/test-cloud-api.yml Expands CI to build/test/clippy the analyzer and run Python drift-renderer tests when relevant paths change.
.github/workflows/openapi-drift.yml Installs Rust toolchain so the scheduled drift workflow can run the analyzer via Cargo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/check-openapi-drift.py Outdated
sdairs and others added 5 commits July 13, 2026 15:06
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sdairs sdairs temporarily deployed to cloud-integration July 13, 2026 14:18 — with GitHub Actions Inactive
Comment thread crates/clickhouse-openapi-analyzer/src/openapi.rs
Comment thread crates/clickhouse-openapi-analyzer/src/openapi.rs Outdated
Comment thread crates/clickhouse-openapi-analyzer/src/openapi.rs Outdated
Comment thread crates/clickhouse-openapi-analyzer/src/rust_inventory.rs Outdated
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sdairs sdairs temporarily deployed to cloud-integration July 14, 2026 12:47 — with GitHub Actions Inactive
Comment thread crates/clickhouse-openapi-analyzer/src/openapi.rs
@sdairs sdairs temporarily deployed to cloud-integration July 15, 2026 12:47 — with GitHub Actions Inactive

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ 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 7b68dc1. Configure here.

};
}
}
EnumContext::Unknown

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path item parameter enums unmapped

Medium Severity

The enum_context function doesn't correctly identify enums for path-item level parameters. Its current logic expects parameters to be nested under an HTTP method, but path-item parameters are directly under the path. This causes these enums to be classified as EnumContext::Unknown, preventing them from being properly mapped to client method arguments.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7b68dc1. Configure here.

Comment thread scripts/check-openapi-drift.py

@iskakaushik iskakaushik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sdairs sdairs merged commit be4123e into issue-275-enum-value-drift Jul 15, 2026
6 checks passed
@sdairs sdairs deleted the codex/issue-295-openapi-drift-analyzer branch July 15, 2026 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants