feat(ingestion): cross-connector silver-contract parity gate (git commits)#1344
feat(ingestion): cross-connector silver-contract parity gate (git commits)#1344mitasovr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughIntroduces an enforced column-level schema contract for the ChangesSilver contract parity for git commits
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/silver-contract-parity.yml:
- Around line 32-34: The GitHub Actions in the workflow are using mutable
version tags (`@v4` and `@v5`) instead of immutable commit SHAs, which weakens
supply-chain security guarantees. Additionally, the checkout action is missing
the persist-credentials configuration. Fix this by replacing the moving major
version tags with full commit SHAs for both the actions/checkout and
actions/setup-python actions. Add a `with:` block to the actions/checkout action
that includes `persist-credentials: false` to disable credential persistence
unless authenticated git operations are explicitly required later in the job.
In `@src/ingestion/scripts/check_silver_contract_parity.py`:
- Around line 62-67: The `_ordered_columns()` function relies on dictionary
iteration order which is not guaranteed in dbt-core 1.10.x, causing the ORDER
validation check to be unreliable. Modify `_ordered_columns()` to preserve the
declared column order by accessing dbt's internal metadata (such as
`node.get("_meta")` or reconstructing order from schema files) instead of
relying on `node.get("columns", {}).items()` iteration. Once this root-cause fix
is in place, the ORDER validation logic that uses `_ordered_columns()` will
correctly detect column reordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffdaceca-a587-4de0-86be-b235eb24b601
📒 Files selected for processing (9)
.github/workflows/silver-contract-parity.ymlsrc/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__commits.sqlsrc/ingestion/connectors/git/bitbucket-cloud/dbt/schema.ymlsrc/ingestion/connectors/git/github/dbt/github__commits.sqlsrc/ingestion/connectors/git/github/dbt/schema.ymlsrc/ingestion/scripts/check_silver_contract_parity.pysrc/ingestion/scripts/tests/test_check_silver_contract_parity.pysrc/ingestion/silver/git/class_git_commits.sqlsrc/ingestion/silver/git/schema.yml
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-python@v5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Unpinned action references in workflows:"
rg -n '^\s*-\s*uses:\s*[^@]+@v[0-9]+' .github/workflows
echo
echo "checkout steps missing persist-credentials hardening:"
rg -n 'uses:\s*actions/checkout@' .github/workflows -C 3Repository: constructorfabric/insight
Length of output: 9507
Pin GitHub Actions to immutable SHAs and disable checkout credential persistence.
Using moving major tags (@v4, @v5) weakens supply-chain guarantees. Add persist-credentials: false to the checkout action unless authenticated git operations are required later in the job.
Affected lines
- uses: actions/checkout@v4
- uses: actions/setup-python@v5Replace with:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-python@v5And pin both actions to full commit SHAs instead of version tags.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 32-32: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 34-34: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/silver-contract-parity.yml around lines 32 - 34, The
GitHub Actions in the workflow are using mutable version tags (`@v4` and `@v5`)
instead of immutable commit SHAs, which weakens supply-chain security
guarantees. Additionally, the checkout action is missing the persist-credentials
configuration. Fix this by replacing the moving major version tags with full
commit SHAs for both the actions/checkout and actions/setup-python actions. Add
a `with:` block to the actions/checkout action that includes
`persist-credentials: false` to disable credential persistence unless
authenticated git operations are explicitly required later in the job.
Source: Linters/SAST tools
| def _ordered_columns(node: dict) -> list[tuple[str, str]]: | ||
| """[(name, data_type), ...] preserving the manifest's declaration order.""" | ||
| return [ | ||
| (name, col.get("data_type") or "<no data_type>") | ||
| for name, col in node.get("columns", {}).items() | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For dbt-core 1.10.x manifest.json, is model node.columns order guaranteed to match declared columns order from schema/model definitions?
💡 Result:
In dbt-core 1.10.x, the columns property within a model node in the manifest.json file is a dictionary (mapping), not an ordered list [1][2][3]. Consequently, the order of columns in the manifest is not guaranteed to match the declared order in your schema.yml or model definition files [4]. Because manifest.json structure is defined using dictionaries for column storage—where keys are column names—the serializing process (or any consumer reading the JSON) does not maintain the original YAML sequence [1][2]. If your use case requires column order (such as for DDL generation or contract enforcement), dbt internally manages that logic through its own internal representation rather than relying on the order found in the manifest artifact [5]. For any automation or tooling you are building, you should not assume that iterating over the columns dictionary will reflect the intended sequence from your project configuration.
Citations:
- 1: https://docs.getdbt.com/reference/artifacts/manifest-json
- 2: https://schemas.getdbt.com/dbt/manifest/v12/index.html
- 3: https://schemas.getdbt.com/dbt/manifest/v10/index.html
- 4: https://docs.getdbt.com/reference/resource-properties/columns.md
- 5: https://docs.getdbt.com/docs/mesh/govern/model-contracts
Column order in manifest.json is not guaranteed—ORDER check will be unsound.
The _ordered_columns() function assumes node.get("columns", {}).items() preserves declared column order, but dbt-core 1.10.x stores columns as a dictionary with no guaranteed ordering. The ORDER validation at lines 132-137 will therefore fail to reliably detect column reordering. Restore declared column order using dbt's internal metadata (e.g., node.get("_meta") or rebuild order from schema files) instead of relying on manifest dictionary iteration.
Also applies to: 132-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/scripts/check_silver_contract_parity.py` around lines 62 - 67,
The `_ordered_columns()` function relies on dictionary iteration order which is
not guaranteed in dbt-core 1.10.x, causing the ORDER validation check to be
unreliable. Modify `_ordered_columns()` to preserve the declared column order by
accessing dbt's internal metadata (such as `node.get("_meta")` or reconstructing
order from schema files) instead of relying on `node.get("columns", {}).items()`
iteration. Once this root-cause fix is in place, the ORDER validation logic that
uses `_ordered_columns()` will correctly detect column reordering.
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
| _SCRIPT_DIR = Path(__file__).resolve().parents[1] |
There was a problem hiding this comment.
Some bullshit from claude
| assert "skipped" in r.stdout | ||
|
|
||
|
|
||
| def _main() -> int: |
There was a problem hiding this comment.
Why you make tests with main? Why not to use pytest suite?
|
|
||
| failed = False | ||
| checked = skipped = 0 | ||
| for tag in sorted(connectors_by_tag): |
There was a problem hiding this comment.
Why they need to be sorted?
| continue | ||
|
|
||
| checked += 1 | ||
| problems: list[str] = [] |
There was a problem hiding this comment.
Non python way.
problems = [check_connector(anchor["columns"], name, model_index[name]) for name in connector_names]|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) |
There was a problem hiding this comment.
WTF??? Why not just main() ?
…mits) Multiple connectors feed one silver class via union_by_tag's positional `SELECT * ... UNION ALL`. When a column is added / removed / reordered / retyped in one connector's staging model but not the others, it blows up at runtime on a cluster (Code: 386 NO_COMMON_TYPE / column-count mismatch) or — when two same-typed columns swap positions — silently cross-wires data. PR CI never catches it because it never materialises every connector at once. Two layers, demonstrated on silver:class_git_commits: - Contracts (single source of truth = the silver class model). `class_git_commits` gets `contract: enforced: true` + the full typed column list; github__commits and bitbucket_cloud__commits get matching enforced contracts. Numeric columns are wrapped in toInt64(...) so the declared Int64 is deterministic regardless of bronze and the github(int) vs bitbucket(count()/UInt64) width drift can't produce Code: 386. on_schema_change=append_new_columns (required by dbt for incremental + enforced contract). - Offline parity gate: scripts/check_silver_contract_parity.py reads the dbt manifest (dbt parse, no warehouse) and, for each class whose silver model has an enforced contract, checks every connector emits exactly the silver contract's columns — same NAMES, ORDER (UNION ALL is positional, so a set-only check is insufficient) and TYPES, and that the connector itself is contracted. Opt-in: classes without an enforced silver contract are reported and skipped, so contracts roll out one class at a time. Verified against the real manifest: class_git_commits green, 33 other classes skipped. - Tests: scripts/tests/test_check_silver_contract_parity.py (dependency-free, runs under pytest or standalone) covers missing / extra / type / order / unenforced / no-columns and the end-to-end opt-in + exit codes. - CI: .github/workflows/silver-contract-parity.yml runs the unit tests + dbt parse + the gate on every PR (no paths filter, so it can be a required check), with dbt pinned. Layer 1 (dbt build validating SQL output == declared contract) runs wherever the models are built; a git e2e fixture / dedicated contract-build job is a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
3865b97 to
f4024e0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/silver-contract-parity.yml (1)
32-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin workflow actions to immutable SHAs and disable checkout credential persistence.
Line 32 and Line 34 still use mutable tags (
@v4,@v5), and checkout does not setpersist-credentials: false. This weakens supply-chain guarantees and leaves a token-persistence hardening gap.#!/bin/bash set -euo pipefail echo "Checking for unpinned GitHub Actions uses:" rg -n '^\s*-\s*uses:\s*[^@]+@v[0-9]+' .github/workflows echo echo "Checking checkout steps missing persist-credentials hardening:" rg -n 'uses:\s*actions/checkout@' .github/workflows -C 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/silver-contract-parity.yml around lines 32 - 34, Replace the mutable version tags on the GitHub Actions in the workflow file with immutable commit SHAs for both actions/checkout@v4 and actions/setup-python@v5 to strengthen supply-chain security. Additionally, add a with section to the actions/checkout step that includes persist-credentials: false to disable unnecessary token persistence and harden the workflow security posture.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/silver-contract-parity.yml:
- Around line 32-34: Replace the mutable version tags on the GitHub Actions in
the workflow file with immutable commit SHAs for both actions/checkout@v4 and
actions/setup-python@v5 to strengthen supply-chain security. Additionally, add a
with section to the actions/checkout step that includes persist-credentials:
false to disable unnecessary token persistence and harden the workflow security
posture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71831c21-865c-4aa1-a2a5-792082c7b597
📒 Files selected for processing (11)
.github/workflows/silver-contract-parity.ymlsrc/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__commits.sqlsrc/ingestion/connectors/git/bitbucket-cloud/dbt/schema.ymlsrc/ingestion/connectors/git/github/dbt/github__commits.sqlsrc/ingestion/connectors/git/github/dbt/schema.ymlsrc/ingestion/connectors/git/gitlab/dbt/gitlab__commits.sqlsrc/ingestion/connectors/git/gitlab/dbt/schema.ymlsrc/ingestion/scripts/check_silver_contract_parity.pysrc/ingestion/scripts/tests/test_check_silver_contract_parity.pysrc/ingestion/silver/git/class_git_commits.sqlsrc/ingestion/silver/git/schema.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- src/ingestion/connectors/git/github/dbt/github__commits.sql
- src/ingestion/connectors/git/github/dbt/schema.yml
- src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__commits.sql
- src/ingestion/connectors/git/bitbucket-cloud/dbt/schema.yml
- src/ingestion/silver/git/schema.yml
- src/ingestion/silver/git/class_git_commits.sql
- src/ingestion/scripts/check_silver_contract_parity.py
Problem
Several connectors feed one silver class through
union_by_tag's positionalSELECT * ... UNION ALL(e.g.github__commits+bitbucket_cloud__commits→class_git_commits). When a developer adds / removes / reorders / retypes a column in one connector's staging model and forgets the others, it surfaces only at runtime on a cluster that materialises the lagging connector — as ClickHouseCode: 386 NO_COMMON_TYPE/ a column-count mismatch, or, when two same-typed columns swap positions, as silently cross-wired data. PR CI never catches it because it never materialises every connector at once.Fix — two layers, demonstrated on
silver:class_git_commits1. Contracts, with the silver class model as single source of truth
class_git_commitsgetscontract: enforced: true+ the full typed column list;github__commitsandbitbucket_cloud__commitsget matching enforced contracts.toInt64(...)→ declaredInt64is deterministic regardless of bronze, and thegithub(int)vsbitbucket(count()/UInt64)width drift can no longer produceCode: 386.on_schema_change='append_new_columns'(dbt requires it for incremental + enforced contract).2. Offline parity gate —
src/ingestion/scripts/check_silver_contract_parity.pydbt parse, no warehouse). For each class whose silver model has an enforced contract, checks every connector emits exactly the silver contract's columns: same names, same ORDER (UNION ALL is positional — a set-only check is insufficient), same types, and that the connector itself carries an enforced contract.class_git_commitsgreen, 33 other classes skipped.3. Tests —
src/ingestion/scripts/tests/test_check_silver_contract_parity.py4. CI —
.github/workflows/silver-contract-parity.ymldbt parse+ the gate on every PR (nopaths:filter, so it can be a required check without hanging unrelated PRs on "Expected"). dbt pinned.Invariant enforced:
Scope / follow-ups
dbt buildin e2e / Argo). There is no git e2e fixture yet, so a dedicated contract-build job (empty bronze →dbt build --select +class_git_commits) or a git fixture is a follow-up — that's what verifies the declared types against real bronze.columns:block per connector +contract: enforced: trueon each silver model; the gate needs no changes.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Tests