feat(analytics): inject flat tenant filter in the compiler's shared WHERE (#1967) - #2086
Merged
cyberantonz merged 6 commits intoJul 31, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe metric-results validation path now carries a tenant UUID into the compiler. Observation and peer-cohort SQL reads apply tenant predicates with aligned bindings. Tests, fixtures, handler comments, and specifications reflect the tenant-aware behavior. ChangesTenant filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/backend/services/analytics/src/api/handlers.rs`:
- Around line 297-301: Protect execute_metric_query before tenant callers can
use the legacy query_ref path by restricting it to prevalidated tenant-safe
sources or enforcing tenant scope safely per query. In
docs/domain/presentation-layer/specs/DESIGN.md:55 and :67, limit shipment and
isolation claims to structured metric_results reads; in :290-298, document the
legacy path as outside the guarantee unless protected. In
docs/domain/presentation-layer/specs/PRD.md:210-212 and :288, keep contract-read
tenant isolation and its NFR incomplete until the legacy path is protected and
covered by isolation tests.
🪄 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: 7ff4f844-dc2e-4c1a-8914-d325d40e213a
📥 Commits
Reviewing files that changed from the base of the PR and between db65ea1 and 79db011d80bcb5d5806d36b5d82a98e38c08e6fe.
📒 Files selected for processing (7)
docs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/api/handlers.rssrc/backend/services/analytics/src/domain/metric_results/batch.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/backend/services/analytics/src/domain/metric_results/validation.rs
ktursunov
approved these changes
Jul 31, 2026
…HERE (constructorfabric#1967) Replace the single-tenant MVP no-op with a server-injected `tenant_id = <ctx>` predicate in the metric_results compiler — the one place every structured contract read compiles through. The predicate leads every observation read (`metric_where` / `shared_observation_where`) and both peer-cohort CTE reads, so a request scoped to tenant A cannot read tenant B's rows. - Carry the resolved tenant on `ValidatedMetricResultsRequest` (from the SecurityContext already passed to `validate_request`); never from client SQL. - Filter on `tenant_id`, the column the gold observation and cohort contract exposes (silver `insight_tenant_id` aliased to `tenant_id`); this surface is internally consistent, so the constructorfabric#1596 name drift does not affect it. Value is the raw tenant UUID — the representation the metric lineage stamps (no sipHash; that is identity-only, constructorfabric#1550). - Leave the legacy per-metric `query_ref` path unfiltered by design: its arbitrary FROM shapes make a flat predicate unsafe; its stale/misleading comments are corrected. - Extend the presentation PRD/DESIGN: mark the tenant-filter FR, NFR, and compiler component implemented; record the actual contract column name. Closes constructorfabric#1967 Part of constructorfabric#1803 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…ts path Address review: the compiler filter (constructorfabric#1967) covers only the structured metric_results reads, not the legacy execute_metric_query query_ref path, which runs arbitrary FROM shapes and stays unscoped. Narrow the FR/NFR completion claims accordingly — the compiler component is done, but the umbrella "every contract read" FR and the isolation NFR remain open until the legacy path is protected or restricted to tenant-safe sources. Document the legacy path as an explicit boundary outside the guarantee. Signed-off-by: Anton Zelenov <antonz@constructor.tech>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/domain/presentation-layer/specs/DESIGN.md (1)
281-281: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign tenant-filter documentation with one authoritative contract.
Line 281 and Line 361 still say the tenant-row filter is deferred to
#1967and referenceinsight_tenant_id, while the Metric Compiler contract at Line 300 usestenant_idfor the injected tenant predicate. Update these statements consistently. If saved-query execution intentionally bypasses the metric-results compiler, document that exception explicitly.🤖 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/presentation-layer/specs/DESIGN.md` at line 281, Update the tenant-filter statements in DESIGN.md, including the referenced deferred-work text, to use the authoritative tenant predicate field tenant_id instead of insight_tenant_id and align the wording with the Metric Compiler contract. If saved-query execution bypasses the metric-results compiler, explicitly document that exception in the relevant section.
🤖 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.
Outside diff comments:
In `@docs/domain/presentation-layer/specs/DESIGN.md`:
- Line 281: Update the tenant-filter statements in DESIGN.md, including the
referenced deferred-work text, to use the authoritative tenant predicate field
tenant_id instead of insight_tenant_id and align the wording with the Metric
Compiler contract. If saved-query execution bypasses the metric-results
compiler, explicitly document that exception in the relevant section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f83d768-0edb-41ca-baad-48c6cc872184
📥 Commits
Reviewing files that changed from the base of the PR and between 79db011d80bcb5d5806d36b5d82a98e38c08e6fe and 79d6307a220a9d2d37359903c8636a2894e314d6.
📒 Files selected for processing (4)
docs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/validation.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/domain/presentation-layer/specs/PRD.md
- src/backend/services/analytics/src/domain/metric_results/builder.rs
- src/backend/services/analytics/src/domain/metric_results/validation.rs
cyberantonz
force-pushed
the
pres/1967-tenant-filter
branch
from
July 31, 2026 03:39
79d6307 to
6b63360
Compare
cyberantonz
enabled auto-merge
July 31, 2026 03:41
ktursunov
approved these changes
Jul 31, 2026
The compiler now binds the request's SecurityContext tenant as a leading tenant_id predicate on every gold read, so seeded bronze rows must carry the tenant the harness's gateway JWT is scoped to. The nil-UUID stamp matched nothing and every metric smoke test read zero rows. Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Contributor
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/domain/presentation-layer/specs/DESIGN.md (1)
300-300: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winClarify that the tenant value is server-side bound.
“Literal tenant predicate” can imply SQL string interpolation. State that the compiler emits the predicate and binds the raw tenant UUID from
SecurityContextas a server-side parameter.
docs/domain/presentation-layer/specs/DESIGN.md#L300-L300: describe the predicate as server-generated with a bound tenant value.docs/domain/presentation-layer/specs/PRD.md#L212-L212: replace “literal” with wording that explicitly requires server-side binding.🤖 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/presentation-layer/specs/DESIGN.md` at line 300, The tenant predicate requirements must explicitly use a server-generated predicate with the raw tenant UUID from SecurityContext bound as a server-side parameter, never SQL interpolation. Update docs/domain/presentation-layer/specs/DESIGN.md lines 300-300 accordingly, and replace “literal” with explicit server-side binding language in docs/domain/presentation-layer/specs/PRD.md lines 212-212.
🤖 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/presentation-layer/specs/DESIGN.md`:
- Line 55: Clarify the architecture-wide tenant guarantee in the overview and
the component-boundary wording near execute_metric_query to state that it
applies only to structured metric_results reads. Keep the legacy
execute_metric_query path explicitly outside this guarantee until it is
tenant-scoped, and ensure no surrounding text claims that every read is
currently isolated.
---
Nitpick comments:
In `@docs/domain/presentation-layer/specs/DESIGN.md`:
- Line 300: The tenant predicate requirements must explicitly use a
server-generated predicate with the raw tenant UUID from SecurityContext bound
as a server-side parameter, never SQL interpolation. Update
docs/domain/presentation-layer/specs/DESIGN.md lines 300-300 accordingly, and
replace “literal” with explicit server-side binding language in
docs/domain/presentation-layer/specs/PRD.md lines 212-212.
🪄 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: c9e139a4-b094-4a32-913c-6394d698d2dd
📥 Commits
Reviewing files that changed from the base of the PR and between 79d6307a220a9d2d37359903c8636a2894e314d6 and e39f06a.
📒 Files selected for processing (23)
docs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/backend/services/analytics/src/api/handlers.rssrc/backend/services/analytics/src/domain/metric_results/batch.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/ingestion/tests/e2e/metrics/templates/bitbucket_git.yamlsrc/ingestion/tests/e2e/metrics/templates/chatgpt_team_chat.yamlsrc/ingestion/tests/e2e/metrics/templates/chatgpt_team_codex.yamlsrc/ingestion/tests/e2e/metrics/templates/claude_enterprise_users.yamlsrc/ingestion/tests/e2e/metrics/templates/claude_team_usage.yamlsrc/ingestion/tests/e2e/metrics/templates/cursor_events.yamlsrc/ingestion/tests/e2e/metrics/templates/cursor_usage.yamlsrc/ingestion/tests/e2e/metrics/templates/git_activity.yamlsrc/ingestion/tests/e2e/metrics/templates/jira_task.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_email.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_onedrive.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_teams.yamlsrc/ingestion/tests/e2e/metrics/templates/outline_wiki_comments.yamlsrc/ingestion/tests/e2e/metrics/templates/outline_wiki_pages.yamlsrc/ingestion/tests/e2e/metrics/templates/people.yamlsrc/ingestion/tests/e2e/metrics/templates/wiki_page_versions.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/backend/services/analytics/src/domain/metric_results/batch.rs
- src/backend/services/analytics/src/domain/metric_results/builder.rs
- src/backend/services/analytics/src/domain/metric_results/validation.rs
- src/backend/services/analytics/src/api/handlers.rs
- src/backend/services/analytics/src/domain/metric_results/compiler.rs
| | `cpt-presentation-fr-saved-query-crud` | The saved query (`presentation.queries` logically; the `saved_queries` table physically) is a SeaORM entity in the analytics **service database (MariaDB)**, like metric definitions; CRUD mutates that metadata, not ClickHouse. Only `/run` reaches ClickHouse — it reuses the existing read path and executes the stored SQL as `presentation_ro`, so no write grant on the contract is ever needed. Shipped (#1965) | | ||
| | `cpt-presentation-fr-query-params` | Named parameters, `tenant` always injected from context (not client SQL), `period` supported | | ||
| | `cpt-presentation-fr-tenant-filter` | Literal `insight_tenant_id = <ctx.tenant>` injected in one place — the compiler's shared `WHERE` — replacing the no-op | | ||
| | `cpt-presentation-fr-tenant-filter` | Literal leading `tenant_id = <ctx.tenant>` injected in one place — the compiler's shared `WHERE` (and the peer-cohort CTE reads) — replacing the no-op. `tenant_id` is the column the gold observation and cohort contract exposes (silver's `insight_tenant_id`, aliased to `tenant_id` in gold); filtering on it sidesteps the #1596 name drift, which affects other tables, not this read surface. Shipped for the structured `metric_results` read path (#1967). The legacy per-metric `query_ref` path (`execute_metric_query`) remains unscoped and is explicitly outside this guarantee until protected — see the component boundaries below. | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Qualify the architecture-wide tenant guarantee.
The architecture overview states that every read is tenant-scoped, but this section and Line 308 exclude execute_metric_query. State that the guarantee applies to structured metric_results reads, or update the overview. Otherwise, the design claims stronger isolation than this PR provides.
🤖 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/presentation-layer/specs/DESIGN.md` at line 55, Clarify the
architecture-wide tenant guarantee in the overview and the component-boundary
wording near execute_metric_query to state that it applies only to structured
metric_results reads. Keep the legacy execute_metric_query path explicitly
outside this guarantee until it is tenant-scoped, and ensure no surrounding text
claims that every read is currently isolated.
ktursunov
approved these changes
Jul 31, 2026
This was referenced Jul 31, 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
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.
What
Replace the single-tenant MVP no-op with a server-injected, leading
tenant_id = <ctx>predicate in themetric_resultscompiler — the one place every structured contract read compiles through. A request scoped to tenant A can no longer read tenant B's rows.ValidatedMetricResultsRequest(from theSecurityContextalready passed tovalidate_request); the value never comes from client SQL.metric_where/shared_observation_where(batch, timeseries, ranking, capped, breakdown, histogram) and both peer-cohort CTE reads. No read path bypasses it.Two settled decisions
tenant_id. Every gold observation and cohort table exposestenant_id(silver'sinsight_tenant_idaliased in gold). This read surface is internally consistent, so thetenant_idvsinsight_tenant_idname drift (Tenant column naming drift: seed/placeholders use insight_tenant_id where dbt models produce tenant_id #1596, open, affects other tables) does not block it. The presentation DESIGN/PRD wording is corrected to the real column.{tenant}param already bound by [pres] Query named params (tenant + period) #1966.Out of scope (by design, documented)
query_refpath stays unfiltered: its arbitrary FROM shapes (subqueries, bare bronze tables) make a flat predicate unsafe. Its stale/misleading comments are corrected.SELECTs use the author-driven{tenant}param ([pres] Query named params (tenant + period) #1966).Specs
Extended the presentation PRD + DESIGN: marked
cpt-presentation-fr-tenant-filter,cpt-presentation-nfr-tenant-isolation, andcpt-presentation-component-metric-compilerimplemented; recorded the actual contract column. Per-artifactcfs validatepasses with 0 errors.Tests
cargo test -p analytics— 485 pass. Addedtenant_predicate_leads_and_binds_context_tenant_on_every_contract_read(asserts the predicate and its bound value lead every read, with balanced?/param counts); updated 3 param-order tests.cargo clippy -p analytics --all-targetsclean;cargo fmtclean.Closes #1967
Part of #1803
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation