feat(bootstrap-db): audit staging -> silver field parity + connectors-ddl CI gate - #2080
Merged
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>
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 #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>
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>
📝 WalkthroughWalkthroughChangesThe PR adds ClickHouse bootstrap validation for committed DDL snapshots and staging-to-silver field parity. It adds the parity CLI, updates bootstrap configuration and container permissions, adds insight tables, and documents the validation process. Connector schema validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant ConnectorsDDLWorkflow
participant ClickHouse
participant BootstrapDB
participant FieldParity
PullRequest->>ConnectorsDDLWorkflow: trigger validation
ConnectorsDDLWorkflow->>ClickHouse: start disposable container
ConnectorsDDLWorkflow->>BootstrapDB: run bootstrap-db
BootstrapDB->>ClickHouse: create relations
ConnectorsDDLWorkflow->>FieldParity: run DDL and field checks
FieldParity->>ConnectorsDDLWorkflow: report drift or mismatches
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| CLICKHOUSE_PASSWORD: insight | ||
| CLICKHOUSE_DATABASE: insight | ||
| steps: | ||
| - uses: actions/checkout@v5 |
Contributor
Author
There was a problem hiding this comment.
Deliberately keeping the major-version tag: the repository convention across .github/workflows is tags (30 uses of actions/checkout@v4/v5 alone, plus setup-python@v5/v6, upload/download-artifact — only a couple of docker actions are SHA-pinned). Pinning one new workflow while every other lane floats adds no real supply-chain protection; SHA-pinning is worth doing repo-wide in one sweep instead.
| with: | ||
| persist-credentials: false # don't leave the token in .git/config | ||
|
|
||
| - uses: actions/setup-python@v6 |
|
|
||
| - name: Upload the drift diff | ||
| if: failure() | ||
| uses: actions/upload-artifact@v7 |
| headers={"X-ClickHouse-User": env("CLICKHOUSE_USER"), "X-ClickHouse-Key": env("CLICKHOUSE_PASSWORD")}, | ||
| ) | ||
| try: | ||
| with urllib.request.urlopen(request) as response: |
Contributor
Author
There was a problem hiding this comment.
Mitigated in b99fa84: CLICKHOUSE_PROTOCOL is now validated to http/https before the URL is built, which pins the urllib scheme (no file://) and fails fast on a typo'd .env. The remaining components (host/port/user/password) come from the same trusted env contract every sibling script in scripts/bootstrap-db uses — whoever sets them already executes the script.
Every connector failed in the connectors-ddl lane with PermissionError: [Errno 13] Permission denied: '/work/config.json' 26 of 26, which left bronze empty and turned the dbt run into 101 `Database bronze_* does not exist` errors. `mktemp -d` creates the workdir 0700 owned by the invoking user, and `cp` carries the config's 0600 over. The source and destination images run as their own non-root user, so on a Linux host neither the directory nor the file inside the bind-mounted /work is reachable. macOS Docker Desktop hides it — its file sharing ignores uid and mode — which is why the pipeline runs clean on a workstation and dies on a runner. chmod the workdir and the three files the containers actually read (config.json for `discover`, destination_config.json + configured_catalog.json for `write`). The directory name is random and lives for a single connector, so the wider mode is an acceptable trade for running on Linux at all. Verified locally end to end on the figma connector (discover -> destination write -> bronze_promoted, PASS=3 ERROR=0); the permission failure itself cannot be reproduced on macOS, so CI is the real check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
…warnings
Treating every type divergence as a failure buried the dangerous cases in a
pile of harmless ones: 74 findings, of which 66 were "nullable-only" — a
bucket that mixed the benign direction with its dangerous mirror image.
Now classified by direction:
* target `Nullable(T)` vs contributor `T` -> WARN. That is what ClickHouse
does when ANOTHER branch of the union is nullable. Every value from this
branch still fits, and readers already handle NULLs from the other
branches.
* contributor `Nullable(T)` vs target `T` -> FAIL (`nullable-narrowing`).
The target is supposed to be the supertype of its branches, so this means
something ALTERed it afterwards and NULLs are being coerced on insert.
* different inner type -> FAIL (`representation`), as before.
Coverage, column-set and column-order divergences stay failures. The job
fails if there is at least one failure; warnings alone exit 0.
On a full warehouse this turns 74 undifferentiated findings into 13 failures
+ 61 warnings — and the split immediately surfaced 5 real narrowing cases
that the old bucket hid: `staging.salesforce__crm_*.custom_fields` is
`Nullable(String)` while `silver.class_crm_*` publishes `String`, because
`heal_crm_table` in apply-ch-migrations.sh ALTERs the silver tables (and
hubspot's staging, but not salesforce's) to `String DEFAULT '{}'` after dbt
has built them.
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
🤖 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/connectors-ddl.yml:
- Around line 113-159: The snapshot drift check currently exits the job before
Field parity can run. Add an id to the “Bootstrap from connectors + dbt +
migrations” step, then set the Field parity step condition to
steps.bootstrap.outcome == 'success' so parity runs after drift detection but
remains skipped when bootstrap itself fails.
In `@src/ingestion/scripts/bootstrap-db/create-connector-tables.sh`:
- Around line 25-34: Update the temporary workspace handling around WORKDIR,
CONFIG_JSON, and the Docker container invocation so config files and ClickHouse
credentials remain owner-only. Remove the world-readable chmod values, and
instead run the source and destination containers with a compatible UID/GID or
apply a narrowly scoped ACL that permits only the container user to read the
bind-mounted files.
🪄 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: ecdf5a77-c174-4a84-be4e-dc8faf2d4b9c
📒 Files selected for processing (10)
.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/bootstrap-db/create-connector-tables.shsrc/ingestion/scripts/connectors-ddl/insight.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/lib/migration_applier.py
A PR run validates the PR's own merge-base, so two PRs that are each green apart can still leave main drifted — one adds a gold table, the other regenerates the snapshot, neither sees the other. Only a run on the merged tree catches that, which is exactly the shape of the drift this lane was built for (six *_metric_evidence tables merged without a regeneration). The job's fork guard has to let pushes through: `pull_request.head.repo` is empty outside a PR event, so the previous condition alone would have skipped every push. A push to main is always in-repo and always carries secrets, so it needs no guard. cancel-in-progress is now limited to pull_request. Superseding a PR push is free; superseding a main commit is not — that run is the only signal that this particular merged tree converges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/connectors-ddl.yml (3)
67-67: 🔒 Security & Privacy | 🟠 MajorPin all workflow actions to immutable commit SHAs.
All three action references use mutable major-version tags. Replace each reference with a reviewed full commit SHA. GitHub identifies full-length SHA references as the immutable form for workflow actions. (docs.github.com)
.github/workflows/connectors-ddl.yml#L67-L67: Pinactions/checkout@v5..github/workflows/connectors-ddl.yml#L71-L71: Pinactions/setup-python@v6..github/workflows/connectors-ddl.yml#L151-L151: Pinactions/upload-artifact@v7.🤖 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/connectors-ddl.yml at line 67, Pin all workflow actions to immutable reviewed full-length commit SHAs: replace actions/checkout@v5 at .github/workflows/connectors-ddl.yml lines 67-67, actions/setup-python@v6 at lines 71-71, and actions/upload-artifact@v7 at lines 151-151.Source: MCP tools
102-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the credential check match the connector configuration.
The loop always requires all four secrets. Therefore, narrowing
connectors-config.yamldoes not avoid this failure. Either derive required secrets from the configured connectors, or remove the “or narrow connectors-config.yaml” instruction from the error.🤖 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/connectors-ddl.yml around lines 102 - 107, Update the credential validation loop in the workflow so it only requires secrets for connectors enabled in connectors-config.yaml, or remove the misleading “or narrow connectors-config.yaml” guidance from the missing-secrets error while preserving the existing all-secret validation.
137-144: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompare the regenerated snapshot with
HEAD.
git add --intent-to-add --allstages removed connector SQL files in the index, sogit diff -- src/ingestion/scripts/connectors-ddlcan miss deletions. Usegit diff HEADfor both the generated diff and the stat output.Proposed fix
- git diff -- src/ingestion/scripts/connectors-ddl > "$RUNNER_TEMP/connectors-ddl.diff" + git diff HEAD -- src/ingestion/scripts/connectors-ddl > "$RUNNER_TEMP/connectors-ddl.diff" ... - git diff --stat -- src/ingestion/scripts/connectors-ddl + git diff HEAD --stat -- src/ingestion/scripts/connectors-ddl🤖 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/connectors-ddl.yml around lines 137 - 144, Update the connectors-DDL drift checks to compare against HEAD: replace both git diff commands in the snapshot validation block, including the generated diff written to connectors-ddl.diff and the subsequent stat output, with git diff HEAD while preserving the existing path scope and artifact/error handling.Source: MCP tools
♻️ Duplicate comments (1)
.github/workflows/connectors-ddl.yml (1)
123-131: 🎯 Functional Correctness | 🟠 MajorRun Field parity after snapshot drift is reported.
Line 145 exits the workflow step, so the default-success Field parity step is skipped. A run with both failures reports only snapshot drift. Add
id: bootstrapto the bootstrap step and useif: ${{ !cancelled() && steps.bootstrap.outcome == 'success' }}on Field parity. The!cancelled()status check is required because GitHub applies an implicitsuccess()condition otherwise. (docs.github.com)Proposed fix
- name: Bootstrap from connectors + dbt + migrations + id: bootstrap env: HUBSPOT_ACCESS_TOKEN: ${{ secrets.HUBSPOT_ACCESS_TOKEN }} - name: Field parity + if: ${{ !cancelled() && steps.bootstrap.outcome == 'success' }} run: |Also applies to: 157-169
🤖 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/connectors-ddl.yml around lines 123 - 131, Update the “Bootstrap from connectors + dbt + migrations” step with id “bootstrap”, then change the Field parity step’s condition to run when the workflow is not cancelled and steps.bootstrap.outcome is “success”. Apply the same condition to the additional Field parity step identified in the diff.Source: MCP 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.
Inline comments:
In `@src/ingestion/scripts/bootstrap-db/README.md`:
- Line 97: Update both descriptions of the .github/workflows/connectors-ddl.yml
audit in the bootstrap-db documentation to explicitly state that pull-request
runs are limited to same-repository PRs by fork guards. Preserve the existing
descriptions of main-branch push validation and bootstrap-db.sh rebuild
behavior.
---
Outside diff comments:
In @.github/workflows/connectors-ddl.yml:
- Line 67: Pin all workflow actions to immutable reviewed full-length commit
SHAs: replace actions/checkout@v5 at .github/workflows/connectors-ddl.yml lines
67-67, actions/setup-python@v6 at lines 71-71, and actions/upload-artifact@v7 at
lines 151-151.
- Around line 102-107: Update the credential validation loop in the workflow so
it only requires secrets for connectors enabled in connectors-config.yaml, or
remove the misleading “or narrow connectors-config.yaml” guidance from the
missing-secrets error while preserving the existing all-secret validation.
- Around line 137-144: Update the connectors-DDL drift checks to compare against
HEAD: replace both git diff commands in the snapshot validation block, including
the generated diff written to connectors-ddl.diff and the subsequent stat
output, with git diff HEAD while preserving the existing path scope and
artifact/error handling.
---
Duplicate comments:
In @.github/workflows/connectors-ddl.yml:
- Around line 123-131: Update the “Bootstrap from connectors + dbt + migrations”
step with id “bootstrap”, then change the Field parity step’s condition to run
when the workflow is not cancelled and steps.bootstrap.outcome is “success”.
Apply the same condition to the additional Field parity step identified in the
diff.
🪄 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: 5f0ae731-6af0-4ef4-9281-85ef7199dc81
📒 Files selected for processing (2)
.github/workflows/connectors-ddl.ymlsrc/ingestion/scripts/bootstrap-db/README.md
…-drift, protocol check Addresses the actionable review findings on #2080: * Connector credentials are no longer world-readable. Source images (the nocode runtime and every CDK connector) tolerate an arbitrary uid, so `discover` now runs as the invoking user (--user + HOME=/tmp) and config.json stays 0600. destination-clickhouse does NOT start under a foreign uid (its entrypoint sources /airbyte/base.sh, readable only by its baked-in user), so its two inputs stay 0644 as a documented exception — they carry no connector secrets, only the ClickHouse password of a throwaway localhost instance. Verified end to end on both image classes (figma nocode, github-copilot CDK). * Field parity runs even when the snapshot drifted: the drift gate's exit 1 used to skip it, so a PR breaking both contracts learned about them one push at a time. Parity is now conditioned on the bootstrap step's outcome instead of the previous step's. * CLICKHOUSE_PROTOCOL is validated to http/https — fails fast on a typo'd .env and pins the urllib scheme (no file://), which also resolves the code-scanning finding on the dynamic URL. * README: PR runs are same-repository only (fork PRs skip — the lane needs repo secrets), plus a single copy-paste block that runs the whole cycle from scratch: throwaway ClickHouse, fresh .env, bootstrap, snapshot re-dump, field-parity audit, cleanup. Not addressed on purpose: pinning actions to commit SHAs. The repository convention is major-version tags (30 uses of actions/checkout@v4/v5 alone); pinning one new workflow while every other lane floats is repo-wide housekeeping, not a property of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
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 `@src/ingestion/scripts/bootstrap-db/README.md`:
- Line 123: Bound the ClickHouse readiness loop by adding an overall timeout and
applying curl’s --max-time to each probe. When readiness is not reached, print
the ClickHouse container logs before exiting with failure; preserve the existing
successful readiness path.
- Around line 110-121: Update the `.env` generation block and its sourcing flow
to protect credentials: set a restrictive `umask 077` before creating the file,
and emit each interpolated credential using shell-safe quoting such as `printf
'%q'` so special characters and newlines cannot alter sourced commands or
variables. Preserve the existing configuration keys and values.
🪄 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: 3f8617cb-80b5-46f7-8960-59a7acece403
📒 Files selected for processing (4)
.github/workflows/connectors-ddl.ymlsrc/ingestion/scripts/bootstrap-db/README.mdsrc/ingestion/scripts/bootstrap-db/check-field-parity.pysrc/ingestion/scripts/bootstrap-db/create-connector-tables.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/connectors-ddl.yml
- src/ingestion/scripts/bootstrap-db/check-field-parity.py
Comment on lines
+110
to
+121
| cat > .env <<EOF | ||
| CLICKHOUSE_HOST=${CH_HOST} | ||
| CLICKHOUSE_PORT=8123 | ||
| CLICKHOUSE_PROTOCOL=http | ||
| CLICKHOUSE_USER=insight | ||
| CLICKHOUSE_PASSWORD=insight | ||
| CLICKHOUSE_DATABASE=insight | ||
| HUBSPOT_ACCESS_TOKEN=${HUBSPOT_ACCESS_TOKEN} | ||
| SALESFORCE_INSTANCE_URL=${SALESFORCE_INSTANCE_URL} | ||
| SALESFORCE_CLIENT_ID=${SALESFORCE_CLIENT_ID} | ||
| SALESFORCE_CLIENT_SECRET=${SALESFORCE_CLIENT_SECRET} | ||
| EOF |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Quote credentials before writing and sourcing .env.
The heredoc writes raw credential values, and Line 127 sources the file. A value containing shell syntax or a newline can execute commands or corrupt the environment. The file also uses the process umask, which can expose credentials to other local users. Set umask 077 and write shell-escaped values with printf '%q', or avoid sourcing the generated file.
Also applies to: 127-127
🤖 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/README.md` around lines 110 - 121, Update
the `.env` generation block and its sourcing flow to protect credentials: set a
restrictive `umask 077` before creating the file, and emit each interpolated
credential using shell-safe quoting such as `printf '%q'` so special characters
and newlines cannot alter sourced commands or variables. Preserve the existing
configuration keys and values.
| SALESFORCE_CLIENT_SECRET=${SALESFORCE_CLIENT_SECRET} | ||
| EOF | ||
|
|
||
| until curl -sf "http://localhost:8123/ping" >/dev/null; do sleep 1; done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the ClickHouse readiness loop.
If Docker exits or port 8123 is unavailable, this loop never exits. Add a timeout, use curl --max-time, and print container logs before failing.
🤖 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/README.md` at line 123, Bound the
ClickHouse readiness loop by adding an overall timeout and applying curl’s
--max-time to each probe. When readiness is not reached, print the ClickHouse
container logs before exiting with failure; preserve the existing successful
readiness path.
mitasovr
enabled auto-merge
July 31, 2026 04:57
cyberantonz
reviewed
Jul 31, 2026
cyberantonz
approved these changes
Jul 31, 2026
cyberantonz
pushed a commit
to cyberantonz/insight
that referenced
this pull request
Aug 3, 2026
…gets The staging -> silver field-parity audit (constructorfabric#2080) fails on 13 type divergences across 5 root causes. A silver class table is a positional UNION ALL of its staging contributors, so each divergence either widens the published silver type depending on which connectors are enabled, or coerces values at the insert boundary. All fixes cast the outlier branch toward the type the data-carrying branch already publishes; silver schemas on warm clusters do not change (every write stays compatible), so no full-refresh is required for correctness — only for the deployed staging tables' declared types to converge, which can happen in any convenient window: dbt run --full-refresh --select github__pull_requests_commits \ gitlab__pull_requests_commits salesforce__crm_accounts \ salesforce__crm_activities salesforce__crm_contacts \ salesforce__crm_deals salesforce__crm_users \ m365__collab_document_activity_sharepoint * commit_order (github, gitlab): the models emit a literal `0` (the APIs provide no ordering), which ClickHouse types as UInt8; bitbucket emits Int64 from real data. toInt64(0) pins the branch to the contract type. * custom_fields (5 salesforce models, 6 branches): passed through from bronze as Nullable(String) while heal_crm_table ALTERs the silver tables to `String DEFAULT '{}'` — NULLs were being coerced to the default at insert time via insert_null_as_default. coalesce makes the '{}' fallback explicit in the model, matching hubspot's literal. * visited_page_count (m365 sharepoint): bronze carries the JSON `number` as Nullable(Decimal(38, 9)) and the model passed it through; the onedrive branch emits Nullable(Int64). A page count is integral — cast to Int64. Nothing downstream reads the column (checked migrations, gold, silver), so the silver type change on fresh clusters is safe. * close_date (hubspot deals): toDate() narrowed to Date while salesforce bronze is natively Nullable(Date32). toDate32 matches the wider type. * hire_date/termination_date (ms-entra, active-directory): constant-NULL placeholders typed Nullable(Date) against the Nullable(DateTime) the data-carrying branches (bamboohr, workday) emit. Retype the NULLs; zero rows are affected by construction. The regenerated snapshot carries the visited_page_count type change and also normalizes silver.contract_version to dump-ddl.sh's statement format — that entry was appended by hand with the semicolon on the statement line, a shape the dumper never produces, so the next convergence check would have flagged it as drift. Verified on a from-scratch bootstrap (all connectors green, dbt PASS=218/ERROR=0): the field-parity audit reports 0 failures, down from 13, across 39 union targets and 273 relations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
3 tasks
3 tasks
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
3 participants
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
New Features
Bug Fixes
Documentation