test(data): ClickHouse data-presence / dedup / freshness audit - #1335
Closed
SharedQA wants to merge 10 commits into
Closed
test(data): ClickHouse data-presence / dedup / freshness audit#1335SharedQA wants to merge 10 commits into
SharedQA wants to merge 10 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 12 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds two independent additions: (1) a new ChangesCI Data Quality Gates
UI Render Contract Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
Contributor
Author
|
Clarifying where this runs, since it reads like a per-PR test but isn't one — pushed as docstring + workflow comments in 48e528a. It's a live-warehouse monitor. Every property here is true by construction on a freshly seeded e2e/CI database (data is always present, deduped, and "now"), so the checks only earn their keep against the real accumulated warehouse. Two distinct invocations:
One thing to flag on the current |
Checks the warehouse properties unit and PR tests cannot see: - presence: every connected bronze source has rows, every silver class_* metric is non-empty (catches wired-but-empty sources / silent blank dashboards); - dedup: silver ReplacingMergeTree count() == count() FINAL (un-merged duplicates → over-count that drifts as merges run); - freshness: newest row per populated table within --max-age-hours. Stdlib only. Two transports: kubectl exec for a local/kind cluster (no creds), or the ClickHouse HTTP interface via CH_HOST/CH_PORT/CH_USER/CH_PASSWORD for CI against a deployed environment. --check gates on duplicates by default; --fail-on-empty / --fail-on-stale opt-in; --waive-empty lists known-empty sources. Live run against kind-insight (2026-06-14): 5 empty bronze sources, 15 empty silver metric tables, 6 stale (>48h), 0 duplicated. See constructorfabric#1334. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…render-contract - data_presence_audit.py: add RESOLUTION check — every insight.* gold view must resolve (SELECT … LIMIT 0) against the live silver schema. Catches gold↔silver drift where a view selects a silver column the deployed schema lacks and the dashboard section serves a 500 rendered as a blank "No data". - connector_silver_coverage.py: fail when a connector reaches bronze but is tagged into no silver:class_* union (stranded — ingests data no dashboard can read). Currently flags task-tracking/youtrack and ai/openai. - tests/ui_render_contract: the render contract (displayed == documented transform of the API value) as a pure spec + unit tests (rounding ownership, ComingSoon, null→no-data, unit spacing), plus an env-gated Playwright e2e asserting the live DOM matches it. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…eployed)
The presence/dedup/freshness/resolution checks only earn their keep against the
real accumulated warehouse — every one is true by construction on a seeded e2e/CI
DB. Make the two invocations explicit:
- PR CI: resolution + dedup only (structural; hold on any populated schema),
and only after dbt actually builds into the service container.
- nightly vs deployed: add --fail-on-empty / --fail-on-stale (presence and
freshness only mean 'the real sync stopped' against live data).
No behaviour change — docstring + workflow comment only. Addresses reviewer
questions on constructorfabric#1335.
Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
SharedQA
force-pushed
the
claude/data-presence-audit
branch
from
June 15, 2026 15:47
48e528a to
1f75ff0
Compare
SharedQA
marked this pull request as ready for review
June 15, 2026 16:39
5 tasks
connector-silver-coverage is blocking and correctly flags openai, youtrack, and figma as stranded (bronze-only, no silver:class_* consumer). These are known, accepted gaps, so waive them via the documented --waive ratchet: the gate still blocks any NEW stranding while letting this land. Each waiver is annotated; drop a name once its silver union exists. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
…unwired The dbt-and-warehouse-gates job was red on every PR (ClickHouse 403 against an empty service container) and merely tolerated via continue-on-error. A red check we ship anyway is exactly the rot a QA gate should prevent. These gates are meaningful only against a POPULATED warehouse, so: - run them on the nightly schedule / manual dispatch, not on PRs (if: github.event_name != 'pull_request'); PRs no longer show the job; - drop continue-on-error — the job now passes or fails honestly; - add data_presence_audit.py --skip-if-unreachable: while the warehouse is unwired it exits 0 with a labelled SKIPPED line (not a fake pass, not an ignored red). Drop the flag once the dbt step builds into the container. connector-silver-coverage (the static, day-one gate) is unchanged and still blocks PRs. Signed-off-by: Kenan Salim <ks@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: 8
🧹 Nitpick comments (1)
tests/ui_render_contract/render_contract.py (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant
int()casts inround_half_up.
math.floor()/math.ceil()already return integers, so the extra casts only add lint noise.♻️ Suggested diff
def round_half_up(value: float) -> int: """Round to nearest integer, halves away from zero (4.5→5, -4.5→-5, 4.4→4).""" - return int(math.floor(value + 0.5)) if value >= 0 else int(math.ceil(value - 0.5)) + return math.floor(value + 0.5) if value >= 0 else math.ceil(value - 0.5)🤖 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 `@tests/ui_render_contract/render_contract.py` at line 36, The round_half_up function contains redundant int() casts around the return values of math.floor() and math.ceil(), which already return integers. Remove both int() casts from the return statement so that math.floor(value + 0.5) and math.ceil(value - 0.5) are returned directly without the unnecessary int() wrapper.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.
Inline comments:
In @.github/workflows/data-checks.yml:
- Around line 1-20: The workflow file lacks explicit permission declarations and
relies on default GITHUB_TOKEN scopes. Add a top-level `permissions:` block
immediately after the `on:` section in the data-checks workflow. Start with the
least-privilege approach by setting `contents: read` as the base permission,
then review what each job actually requires (such as pull request access,
workflow triggering capabilities, or other specific scopes) and add only those
additional permissions where strictly necessary rather than granting broad
default permissions.
- Around line 56-84: The dbt build step in the "dbt build + tests + contract
coverage (changed models)" step is commented out as a TODO, causing the
subsequent "Gold-view resolution + data presence" audit to run against an empty
ClickHouse schema, which produces false-passing results. To fix this, uncomment
the dbt commands (dbt deps, dbt build --select state:modified+, and the
dbt_coverage.py script) in the first step, update the profiles.yml to point the
dbt profile at the local ClickHouse service on host 127.0.0.1:8123, and then
remove the --skip-if-unreachable flag from the data_presence_audit.py command in
the "Gold-view resolution + data presence" step since the container will now be
properly populated with dbt-built views and tables to validate.
- Around line 21-24: Replace the floating version tags with full commit SHAs for
security hardening. Update actions/checkout@v4 to use its full commit SHA and
actions/setup-python@v5 to use its full commit SHA. Apply this same change
pattern to the additional occurrences of these actions mentioned at lines 52-55
to ensure all GitHub Actions references are pinned to specific commit SHAs
rather than mutable version tags.
In `@scripts/ci/data_presence_audit.py`:
- Around line 171-179: The skip condition in the exception handler for the
ClickHouse connection check only skips when a connection exception occurs, but
does not handle the case where ClickHouse is reachable but the warehouse is
unwired (empty/no data). Modify the logic to also skip when
args.skip_if_unreachable is true AND the warehouse has no data populated, not
just when a connection exception is raised. This ensures that the script returns
SKIPPED for both unreachable and unwired warehouses, preventing false PASSED
results that mask an unwired gate. Apply the same fix to the similar logic
mentioned at lines 235-253.
- Around line 87-97: The code constructs HTTP URLs while sending credentials via
the X-ClickHouse-Key header, which exposes sensitive information over cleartext.
Modify the URL scheme construction in the urllib.request.Request call to use
HTTPS instead of HTTP, either by default or conditionally when CH_PASSWORD is
non-empty. Change the protocol in the f-string that builds the request URL from
http:// to https:// based on whether credentials are being used.
In `@tests/ui_render_contract/README.md`:
- Around line 15-17: The markdown file contains fenced code blocks that are
missing language identifiers, which violates markdownlint rule MD040. Add the
bash language identifier to both code blocks: the pytest command block at lines
15-17 and the playwright/pytest command block at lines 27-34. For each code
block, change the opening fence from triple backticks to triple backticks
followed by the word bash (e.g., ```bash instead of ```).
In `@tests/ui_render_contract/test_live_render_e2e.py`:
- Around line 84-86: The KPI data extraction in the loop iterating over
body.get("results", []) is accessing items from the wrong location. Instead of
checking r.get("status") and accessing r["items"] directly, the code should
access the items through the response field according to the batch API contract
structure. Change the condition and access pattern to look for r.get("response")
and extract items from r.get("response").get("items") to correctly parse the
nested KPI response data and avoid leaving captured empty.
- Around line 113-115: The pytest.xfail() call marking the known-bug case for
constructorfabric/insight#1337 lacks proper strict configuration, meaning if the
underlying bug is fixed, the test will unexpectedly pass (XPASS) without failing
the build. Add xfail_strict = true to the pytest configuration file (typically
pytest.ini, setup.cfg, pyproject.toml, or tox.ini in the repository root) to
ensure that unexpected passes from pytest.xfail() calls are treated as test
failures and properly alert the team when the referenced bug is actually
resolved.
---
Nitpick comments:
In `@tests/ui_render_contract/render_contract.py`:
- Line 36: The round_half_up function contains redundant int() casts around the
return values of math.floor() and math.ceil(), which already return integers.
Remove both int() casts from the return statement so that math.floor(value +
0.5) and math.ceil(value - 0.5) are returned directly without the unnecessary
int() wrapper.
🪄 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: 59bcbf35-6e19-4978-8493-437b6aeaff73
📒 Files selected for processing (8)
.github/workflows/data-checks.ymlscripts/ci/connector_silver_coverage.pyscripts/ci/data_presence_audit.pytests/ui_render_contract/README.mdtests/ui_render_contract/conftest.pytests/ui_render_contract/render_contract.pytests/ui_render_contract/test_live_render_e2e.pytests/ui_render_contract/test_render_contract.py
…s green - data-checks.yml: add top-level 'permissions: contents: read' and pin actions/checkout + actions/setup-python to commit SHAs (matches the repo's other workflows; supply-chain hygiene). - data_presence_audit.py: --skip-if-unreachable now also skips when ClickHouse is REACHABLE but unpopulated (no bronze_* / silver.class_* tables). A blank warehouse would otherwise pass every resolution/dedup check vacuously — a false green worse than a red. Now it skips honestly until dbt populates it. - data_presence_audit.py: never send credentials over cleartext — default to https whenever CH_PASSWORD is set (plain http only for the password-less local/CI container); CH_SCHEME overrides. - README: add bash language to fenced blocks (MD040). Signed-off-by: Kenan Salim <ks@constructor.tech>
…g a TODO The dbt-and-warehouse-gates job never did anything real: its dbt step was an 'echo' TODO and the audit ran against an empty throwaway ClickHouse, where every resolution/dedup/presence check passes by construction. A gate that can't fail is worse than no gate. If it's useless, it shouldn't exist — so remove it. - data-checks.yml: drop the dbt-and-warehouse-gates job, the schedule trigger, and the TODO stub entirely. Keep connector-silver-coverage (a real, warehouse-free gate) and document where the warehouse audit actually belongs. - data_presence_audit.py: drop the --skip-if-unreachable flag + reachable-but-empty skip (they only existed to make the vacuous gate 'honest'). The tool runs against a real populated warehouse (e2e rig / deployed), so an unreachable warehouse is a hard failure again. Keep the https-when-credentialed hardening. Docstring now points resolution/dedup at the e2e rig (which dbt-builds), not a standalone job. - render_contract.py: drop redundant int() casts (math.floor/ceil already return int). The test_live_render_e2e.py 'use response.items' review note is a false positive: BatchQueryResult is #[serde(tag="status")] with #[serde(flatten)] response, so status/items are top-level (verified in domain/query.rs) — the test is correct. Signed-off-by: Kenan Salim <ks@constructor.tech>
…lity.yml data-checks / data-integrity / data-contracts all triggered on the same src/ingestion/** PRs — three workflows, three checkouts, three Python setups. Fold them into one data-quality.yml with parallel jobs: - connector-silver-coverage (blocking) - nullable-key-guard (blocking; absorbs constructorfabric#1348's nullable_key_audit + self-test) - dbt-coverage (report-only; absorbs constructorfabric#1320's dbt_coverage) Scripts moved into this PR; constructorfabric#1348 is closed as absorbed and constructorfabric#1320 slims to its m365 type-mismatch fix. SHA-pinned actions, persist-credentials:false throughout. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
SharedQA
added a commit
to SharedQA/insight
that referenced
this pull request
Jun 23, 2026
…constructorfabric#1335) Drop data-contracts.yml + dbt_coverage.py from this PR — they now live in the single data-quality.yml on constructorfabric#1335 (one data-gate workflow instead of three). This PR keeps only its substance: the m365 collab_document type-mismatch fix (constructorfabric#1318) + the silver collaboration contract. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
Contributor
|
#1744 - must reimplement |
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
2 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.
Adds
scripts/ci/data_presence_audit.py— a warehouse-level check for the properties unit and PR tests cannot observe: whether the fetch wrote data, whether it is deduplicated, and whether it is fresh.Checks
silver.class_*metric table is non-empty. Catches wired-but-empty sources and silver models that produced nothing (blank dashboards with no error).count()with and withoutFINAL. A mismatch means un-merged duplicates are present, which over-count for any reader withoutFINALand drift as background merges run.--max-age-hours(default 48).Transports
CH_EXEC_POD=insight-clickhouse-0 python3 scripts/ci/data_presence_audit.py(kubectl exec, no credentials).CH_HOST/CH_PORT/CH_USER/CH_PASSWORDover the ClickHouse HTTP interface.--checkexits non-zero on duplicates by default;--fail-on-emptyand--fail-on-staleare opt-in;--waive-emptylists known-empty sources.Live run against kind-insight (2026-06-14)
Documents #1334; complements #1319 (empty connectors), #1330 (dedup), #1331 (freshness/time). Intended to run nightly against the deployed environment and as a gate on the production sync DAG.
Summary by CodeRabbit
Tests
Chores