feat(bootstrap-db): audit staging -> silver field parity + connectors-ddl CI gate - #2077
Closed
mitasovr wants to merge 5 commits into
Closed
feat(bootstrap-db): audit staging -> silver field parity + connectors-ddl CI gate#2077mitasovr wants to merge 5 commits into
mitasovr wants to merge 5 commits into
Conversation
A silver `class_*` model is a UNION ALL of every staging model tagged `silver:<target>` (the `union_by_tag` macro). ClickHouse matches UNION branches BY POSITION and takes the column names from the first branch, so a contributor that renames, reorders or retypes a column does not fail the build — it silently misaligns data or widens the published silver type depending on which connectors happen to be enabled. Neither drift is visible in the dbt DAG or in the connectors-ddl snapshot (which carries a single staging table by design). `check-field-parity.py` reads `system.columns` from a bootstrap warehouse — the only place where every model is materialised at once — and the staging -> silver mapping from the dbt manifest tags, then fails on any divergence: column set, positional order, exact type. There is no baseline file and no warning tier, so a `Nullable(T)` vs `T` split fails like a renamed column. The run also fails when a model present in the manifest has no relation in the warehouse: a connector whose `discover` failed would otherwise shrink the comparison silently and the audit would pass for the wrong reason. Ephemeral contributors are covered when they are plain pass-throughs over a single `source()`/`ref()` — the shape of `jira__task_field_history`, whose physical table is written by the `jira-enrich` Rust binary and whose DDL lives in the `create_task_field_history_staging` macro (ADR-003), so it has no dbt DAG edge to its silver target. The audit follows the `source()` dependency to that table, which means a future YouTrack twin of the enrich table gets the same guard for free. Like `bootstrap-db.sh`, the script sources the `.env` next to it and lets those values win over the inherited environment; `--no-env-file` points the audit at another cluster instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
`generate-connectors-config.sh` emits exactly the fields each connector spec marks required, and two of those moved: zoom's `start_date` is no longer required, salesforce's now is. The hubspot and salesforce credential fields keep their `env:` indirection so no secret lands in the committed file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Contributor
|
📝 WalkthroughWalkthroughAdds a PR workflow that validates the committed connectors DDL snapshot through placeholder replay, full bootstrap drift detection, and staging-to-silver field parity checks. It also adds the parity CLI, ClickHouse startup helper, new insight tables, connector configuration updates, and documentation. ChangesConnectors DDL validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant Workflow
participant ClickHouse
participant Bootstrap
participant ParityAudit
PullRequest->>Workflow: trigger connectors-ddl validation
Workflow->>ClickHouse: start phase 1
Workflow->>ParityAudit: check committed snapshot parity
Workflow->>ClickHouse: start fresh phase 2
Workflow->>Bootstrap: run full bootstrap
Bootstrap->>ClickHouse: build warehouse
Workflow->>Workflow: compare regenerated DDL
Workflow->>ParityAudit: check rebuilt warehouse parity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
The committed connectors-ddl snapshot and the staging -> silver field contract are the two things no existing lane can see: both are only observable in a real ClickHouse where every model is materialised at once. Until now the only guard was a sticky reminder comment, and an ignored reminder reintroduces the constructorfabric#1744 drift — discovered at the next fresh deploy. Two phases, each on its OWN throwaway ClickHouse pinned to the production version from pins.env: 1. Apply the committed snapshot to an empty cluster exactly as a fresh deploy does (create-bronze-placeholders.sh), then audit field parity with --allow-missing-relations: the snapshot carries only the gold-referenced staging tables by design, so a strict coverage demand would fail every run for the wrong reason. 2. A second, virgin cluster: bootstrap-db.sh (real connector `discover`, the real destination-clickhouse connector, real dbt, real migrations), re-dump, fail on any snapshot diff, then audit field parity strictly. Phase 2 must not inherit phase 1's cluster: the applicator uses CREATE ... IF NOT EXISTS, so a relation the committed snapshot still carries but the current code no longer produces would survive into the fresh dump, hide its own deletion from the diff, and break convergence — the same reason bootstrap-db.sh sets BOOTSTRAP_SKIP_SNAPSHOT=1. `pull_request` (never `pull_request_target`) plus a head-repo guard: the job runs PR code and needs the HubSpot / Salesforce credentials whose CDK `discover` calls a live API, and secrets never reach fork PRs. Those secrets are checked up front so a missing one fails in seconds instead of 20 minutes into the bootstrap. Drift fails with the regeneration command, the first 200 diff lines inline and the full diff as an artifact; the job never commits. Also corrects three references that described the snapshot as "CI-generated". CI regenerates it to compare, but the committed file is produced and committed by a human. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
…e tables `feat(gold): add metric evidence serving tables` (6e2c56b) added six gold relations without regenerating the committed snapshot, so a fresh cluster would not pre-create them: insight.ai_metric_evidence insight.task_metric_evidence insight.collab_metric_evidence insight.task_worklog_flow insight.git_metric_evidence insight.wiki_metric_evidence Regenerated with the full bootstrap-db pipeline (real connector `discover`, destination-clickhouse, dbt, migrations) on the pinned ClickHouse, then `dump-ddl.sh`; only insight.sql changed, which is what a purely additive set of migration-owned gold tables should produce. This is exactly the drift the new connectors-ddl lane fails on — without this commit the lane is red on arrival. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
mitasovr
changed the title
The first phase applied the committed snapshot to an empty cluster and audited field parity over it. Measured against the real snapshot, that compared exactly ONE of 141 staging contributors: dump-ddl.sh keeps only the gold-referenced staging tables, so 153 relations were absent and the audit had almost nothing to look at. The remaining signal — "the snapshot applies to an empty cluster" — is already implied by every fresh deploy and by the e2e rig, which applies the same snapshot on every run. So the lane now creates one empty ClickHouse and goes straight to bootstrap-db.sh. Gone with the phase: the second container, the `--select __no_such_model__` manifest warm-up (the dbt run inside bootstrap-db writes manifest.json itself) and check-field-parity.py's `--allow-missing-relations`, which existed only to keep that phase from failing for the wrong reason. The header still records why the snapshot must NOT be applied before generation: `CREATE ... IF NOT EXISTS` would let a relation the snapshot still carries but the code no longer produces survive into the fresh dump and hide its own deletion from the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ingestion/scripts/bootstrap-db/connectors-config.yaml (1)
99-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd
zoom_start_dateto the new Zoom bootstrap config.
docs/components/connectors/collaboration/zoom/zoom.mdstill documentsstart_dateas a Zoom connector config, andconnector.yaml/secrets/connectors/zoom.yaml.examplestill define it. Theconnectors-config.yamlentry now lacks any Zoom start-date value; addzoom_start_datefor the same2020-01-01/connector-default pattern so regenerated bootstrap settings don’t omit an expected declared input.🤖 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/bootstrap-db/connectors-config.yaml` around lines 99 - 112, Add zoom_start_date to the zoom entry in connectors-config.yaml, using the existing connector-default pattern and the documented default value 2020-01-01. Keep the other Zoom configuration fields unchanged so regenerated bootstrap settings include the declared start-date input.
🧹 Nitpick comments (2)
.github/workflows/scripts/start-clickhouse.sh (1)
25-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider binding ClickHouse's port to loopback only.
-p 8123:8123exposes ClickHouse on all interfaces with a fixed weak credential pair (insight/insight) and admin-management enabled. Low risk on isolated GitHub-hosted runners, but-p 127.0.0.1:8123:8123would be a cheap hardening step if this script is ever reused on a shared/self-hosted runner.🔒 Proposed change
-docker run -d --name "${NAME}" -p 8123:8123 \ +docker run -d --name "${NAME}" -p 127.0.0.1:8123:8123 \🤖 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/scripts/start-clickhouse.sh around lines 25 - 30, Update the Docker port mapping in the ClickHouse startup command to bind port 8123 to loopback only, preserving the existing container port and other environment settings.src/ingestion/scripts/bootstrap-db/check-field-parity.py (1)
1-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding unit tests for the core comparison logic.
passthrough_relation,nullable_only, and the columns/order/types comparison logic (Lines 276-297) are non-trivial and are explicitly framed as "a data-correctness gate, not style" in the workflow.pyproject.tomlalready listspytestas a dev dependency, so a small unit-test module exercising these pure functions with synthetic manifest fragments would be low-risk to add and would guard this gate against future regressions.🤖 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/bootstrap-db/check-field-parity.py` around lines 1 - 354, Add a pytest module covering the pure helpers passthrough_relation and nullable_only, plus synthetic-manifest tests for the contributor-versus-target comparison in main, including missing/extra columns, order mismatches, differing types, and nullable-only differences. Keep tests isolated from ClickHouse and environment access by using synthetic structures or refactoring the comparison into a testable helper if needed.
🤖 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 `@docs/domain/ingestion/specs/ADR/0007-fresh-cluster-placeholders.md`:
- Around line 16-17: Clarify the ADR statement about
`.github/workflows/connectors-ddl.yml` by limiting the rerun behavior to
eligible same-repository PRs. Explicitly exclude fork PRs, which are skipped due
to the repository-owned branch requirement for secret access.
In `@src/ingestion/scripts/bootstrap-db/check-field-parity.py`:
- Around line 235-245: Update the union-target audit around target_node,
relation_of, and structure.get so ephemeral targets are explicitly listed or
flagged instead of being silently skipped. Preserve the existing handling for
missing manifest nodes and non-ephemeral targets, and follow the script’s
documented ephemeral behavior when target_columns is unavailable.
---
Outside diff comments:
In `@src/ingestion/scripts/bootstrap-db/connectors-config.yaml`:
- Around line 99-112: Add zoom_start_date to the zoom entry in
connectors-config.yaml, using the existing connector-default pattern and the
documented default value 2020-01-01. Keep the other Zoom configuration fields
unchanged so regenerated bootstrap settings include the declared start-date
input.
---
Nitpick comments:
In @.github/workflows/scripts/start-clickhouse.sh:
- Around line 25-30: Update the Docker port mapping in the ClickHouse startup
command to bind port 8123 to loopback only, preserving the existing container
port and other environment settings.
In `@src/ingestion/scripts/bootstrap-db/check-field-parity.py`:
- Around line 1-354: Add a pytest module covering the pure helpers
passthrough_relation and nullable_only, plus synthetic-manifest tests for the
contributor-versus-target comparison in main, including missing/extra columns,
order mismatches, differing types, and nullable-only differences. Keep tests
isolated from ClickHouse and environment access by using synthetic structures or
refactoring the comparison into a testable helper if needed.
🪄 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 Plus
Run ID: 590e868c-e9c4-44dd-9f83-35544b772178
📒 Files selected for processing (9)
.github/workflows/connectors-ddl.yml.github/workflows/scripts/start-clickhouse.shdocs/domain/ingestion/specs/ADR/0007-fresh-cluster-placeholders.mdsrc/ingestion/scripts/bootstrap-db/README.mdsrc/ingestion/scripts/bootstrap-db/check-field-parity.pysrc/ingestion/scripts/bootstrap-db/connectors-config.yamlsrc/ingestion/scripts/connectors-ddl/insight.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/lib/migration_applier.py
Comment on lines
+16
to
+17
| > change, and `.github/workflows/connectors-ddl.yml` re-runs the pipeline on | ||
| > every PR and fails when the committed snapshot no longer matches. The |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that fork PRs are excluded.
The workflow does not rerun on every PR: fork PRs are skipped because the job requires repository-owned branches for secret access. Please qualify this as “eligible same-repository PRs” to keep the ADR accurate.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: The official name of this software platform is spelled with a capital “H”.
Context: ...ver connector/dbt sources > change, and .github/workflows/connectors-ddl.yml re-runs t...
(GITHUB)
🤖 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 `@docs/domain/ingestion/specs/ADR/0007-fresh-cluster-placeholders.md` around
lines 16 - 17, Clarify the ADR statement about
`.github/workflows/connectors-ddl.yml` by limiting the rerun behavior to
eligible same-repository PRs. Explicitly exclude fork PRs, which are skipped due
to the repository-owned branch requirement for secret access.
Contributor
Author
|
Superseded by #2080 — same commits, but the branch now lives in this repository instead of a fork, so the new |
This was referenced Jul 30, 2026
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two contracts are invisible to every existing lane, because both are only observable in a real ClickHouse where every model is materialised at once:
connectors-ddlsnapshot. A gold/staging/bronze relation added without regenerating it simply will not exist on the next fresh cluster (New environment: Git + Task Delivery KPIs (Commits, PRs Merged, Tasks Completed, Cycle Time, …) never load until silver tables are manually reset #1744). Today the only guard is a sticky reminder comment, and an ignored reminder reintroduces exactly that drift.class_*model is aUNION ALLof every staging model taggedsilver:<target>(union_by_tag). ClickHouse matches UNION branches by position and takes the column names from the first branch, so a contributor that renames, reorders or retypes a column does not fail the build — it silently misaligns data, or widens the published silver type depending on which connectors happen to be enabled.This PR adds the audit for (2) and a CI lane that gates both.
What
src/ingestion/scripts/bootstrap-db/check-field-parity.py— readssystem.columnsfor the structure anddbt/target/manifest.jsonfor the staging -> silver mapping (that mapping lives only in the dbt tags, not in the database), then fails on any divergence:discoverfailed would shrink the comparison silently and the audit would pass for the wrong reason.system.columns.type;Nullable(T)vsTcounts, since that split decides the published silver type.No baseline file, no warning tier: everything is a hard failure. Same
CLICKHOUSE_*env contract as the sibling scripts, and it sources the local.envthe waybootstrap-db.shdoes (--no-env-fileto audit another cluster).Ephemeral contributors are covered too.
jira__task_field_historyis an ephemeral pass-through overstaging.jira__task_field_history, a table written by thejira-enrichRust binary whose DDL lives in thecreate_task_field_history_stagingmacro (ADR-003) — so it has no dbt DAG edge to its silver target. The audit follows thesource()dependency to that table, which means a future YouTrack twin of the enrich table gets the same guard for free, as long as it keeps the shape (ephemeral pass-through + thesilver:<target>tag)..github/workflows/connectors-ddl.yml— one empty ClickHouse pinned to the production version frompins.env, then straight intobootstrap-db.sh(real connectordiscover, the real destination-clickhouse connector, real dbt, real migrations) -> re-dump -> fail on any snapshot diff -> audit field parity.The committed snapshot is deliberately not applied first.
create-bronze-placeholders.shissuesCREATE ... IF NOT EXISTS, so a relation the snapshot still carries but the current code no longer produces would survive into the fresh dump, hide its own deletion from the diff and break convergence — the same reasonbootstrap-db.shsetsBOOTSTRAP_SKIP_SNAPSHOT=1during generation.pull_request(neverpull_request_target) plus a head-repo guard, so the lane runs only for branches in this repository: the job executes PR code and needs the HubSpot / Salesforce credentials whose CDKdiscovercalls a live API, and secrets never reach fork PRs. Those secrets are checked up front, so a missing one fails in seconds instead of 20 minutes into the bootstrap. Drift fails with the regeneration command, the first 200 diff lines inline and the full diff as an artifact; the job never commits.Findings this already surfaces
The snapshot was stale.
feat(gold): add metric evidence serving tables(6e2c56b) added six gold relations without regenerating it, so a fresh cluster would not pre-create them. Regenerated here in its own commit — without it the new lane is red on arrival.Field parity, over a full bootstrap warehouse (270 relations, 39 union targets): 74 findings — 66 nullable-only, 8 representation, 0 coverage gaps, 0 unchecked. Column sets and column order are clean everywhere. The 8 representation-level ones:
commit_orderUInt8vsInt64— overflows at the 256th commit in a PRclass_git_pull_requests_commitshire_date,termination_dateNullable(Date)vsNullable(DateTime)class_peoplevisited_page_countNullable(Int64)vsNullable(Decimal(38, 9))class_collab_document_activityclose_dateNullable(Date)vsNullable(Date32)—Datecannot hold pre-1970class_crm_dealsFixing those is deliberately not in this PR. Note the consequence: the lane is red until they are fixed — either land the type alignments first, or merge this and treat the lane as advisory until it is made a required check.
Verification
Every check was proven to trip, by mutating an empty table in a throwaway warehouse and restoring it (
SHOW CREATEidentical before/after):ADD COLUMN probe_extra StringFAIL columns … extraDROP COLUMN report_periodFAIL columns … missingMODIFY COLUMN calls_count Int32FAIL types … [representation]MODIFY COLUMN user_name … AFTER calls_countFAIL order … positional UNION mismatch+ both position listsFAIL coverage … read by the ephemeral contributor … but absentFAIL … (via ephemeral pass-through) …The lane's own steps were rehearsed against a real warehouse:
dump-ddl.sh+ the diff gate is what caught the stale snapshot above, and the strict audit is the run reported in "Findings". An earlier draft also applied the committed snapshot to an empty cluster and audited that; measured, it compared 1 of 141 contributors (153 relations absent, since the snapshot keeps only the gold-referenced staging tables), so it was dropped.The pass-through detector was unit-checked against
SELECT *(with a-- depends_onhint and a trailing;) plus the shapes it must reject: explicit column list,SELECT *, 1 AS extra,WHERE,JOIN, CTE — those resolve to UNCHECKED rather than being compared against the wrong relation.Needs doing before this lane can pass
HUBSPOT_ACCESS_TOKEN,SALESFORCE_CLIENT_ID,SALESFORCE_CLIENT_SECRET,SALESFORCE_INSTANCE_URL. Their CDKdiscovercalls a live API, so fake values fail. The workflow checks them up front and fails in seconds rather than 20 minutes into the bootstrap. The documented alternative — seeding those two connectors' bronze from the snapshot instead — is not wired up here, because it would freeze exactly the schemas the gate is meant to watch.e2e-bronze-to-api.yml's convention (PR-only, all paths). If the ~40 minutes on every PR is too much, add apaths:filter onsrc/ingestion/**.connectors-ddl-reminder.ymlis now redundant with a real gate; retiring it is a separate call, so it is left in place.Commits
Five, each droppable on its own: the audit script, the
connectors-config.yamlregeneration (two spec-required fields moved), the CI lane (plus three doc references that described the snapshot as "CI-generated" — CI regenerates it to compare, but the committed file is produced by a human), the snapshot regeneration for the metric-evidence tables, and the follow-up that drops the snapshot-apply phase.Summary by CodeRabbit