Skip to content

analytics-api: run as an idiomatic gears-rust gear#1546

Merged
cyberantonz merged 15 commits into
constructorfabric:mainfrom
cyberantonz:analytics-api-gear
Jul 2, 2026
Merged

analytics-api: run as an idiomatic gears-rust gear#1546
cyberantonz merged 15 commits into
constructorfabric:mainfrom
cyberantonz:analytics-api-gear

Conversation

@cyberantonz

@cyberantonz cyberantonz commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Migrate analytics-api from a self-hosted axum::serve service to a gears-rust host on toolkit::bootstrap::run_server:

  • main.rs: toolkit bootstrap (AppConfig::load_or_default → run_server / gear::run_migrate); links the api-gateway REST host + auth/authz/tenant system gears via inventory.
  • gear.rs (new): AnalyticsApiGear #[toolkit::gear(capabilities=[rest])]. Gear::init reconstructs the full former run_server startup (self-managed MariaDB pool + 45 migrations + CHECK/product-default probes, redis catalog cache, ClickHouse + Identity clients, schema validator, admin-CRUD service), stores AppState in a OnceLock, and tokio::spawns the one-shot validate_all() scan. RestApiCapability::register_rest wires the OperationBuilder routes into the host router + Extension(AppState).
  • Auth: host runs auth_disabled (the platform api-gateway is the sole authenticator); it injects a SecurityContext with DEFAULT_TENANT_ID (== the single-tenant default the seeded data uses). The former hand-rolled tenant_middleware becomes a thin, stateless layer that reads toolkit_security::SecurityContext and overrides the tenant from X-Insight-Tenant-Id when present. The local SecurityContext struct is removed; handlers + domain services now use toolkit_security only.
  • DB stays self-managed (no toolkit db capability; ClickHouse is not a toolkit-db backend anyway).
  • config.rs: figment loader removed; GearConfig is deserialized from gears.analytics-api.config via ctx.config(). New config/insight.yaml in the gears layout (auth_disabled: true), modeled on api-gateway's no-auth.yaml.

Build, clippy (pedantic), and tests (325 pass) all green. Helm/compose manifests still reference the old ANALYTICS__* env model and are updated in a follow-up.

Summary by CodeRabbit

  • New Features
    • Added a gears-based runtime for the analytics API, including migrate and check commands, with improved startup/config loading.
    • Introduced default Insight analytics configuration and host-led REST/docs/OpenAPI setup.
  • Bug Fixes
    • Improved tenant scoping consistency across metrics, thresholds, admin actions, and queries using the request’s effective tenant.
    • Refined X-Insight-Tenant-Id override behavior; updated routing so health works with host auth-disabled behavior.
  • Tests
    • Updated tenant-resolution integration tests to focus on host-injected security context override.
    • Added new end-to-end ignored-by-default HTTP live-route tests.
  • Chores
    • Updated Helm/Docker/GitOps/secrets to use the APP__gears__analytics_api__config__* env-var contract.
    • Added missing source and source_id placeholder columns for seeded schema.

@cyberantonz
cyberantonz requested a review from a team as a code owner July 1, 2026 11:17
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The analytics-api service is converted into a ToolKit gear. It adds bootstrap-based startup and config loading, stateless tenant override middleware, route registration through a shared OpenAPI registry, and updates handlers and domain logic to use subject_tenant_id() and Extension state injection.

Changes

Analytics-api ToolKit gear migration

Layer / File(s) Summary
Dependencies and host config
src/backend/services/analytics-api/Cargo.toml, src/backend/services/analytics-api/src/config.rs, src/backend/services/analytics-api/config/insight.yaml, src/backend/services/analytics-api/Dockerfile, docker-compose.yml, deploy/gitops/scripts/compose-app-secrets.sh, deploy/gitops/environments/local/values.yaml.template, charts/insight/templates/secrets.yaml, charts/insight/values.yaml, src/backend/services/analytics-api/helm/templates/configmap.yaml, src/backend/services/analytics-api/helm/templates/deployment.yaml, src/backend/services/analytics-api/helm/values.yaml, .github/workflows/ci.yml, scripts/ci/changed.py, scripts/ci/components.py
Adds bootstrap/security dependencies, replaces AppConfig with GearConfig, adds the host YAML config, and updates container, Secret, Helm, and CI wiring to use the new config file and APP__gears__analytics_api__config__* environment contract.
Gear implementation and startup
src/backend/services/analytics-api/src/gear.rs, src/backend/services/analytics-api/src/main.rs
Adds AnalyticsApiGear, route registration, migration support, config extraction, bootstrap server startup, new CLI flags, and the Check subcommand.
Tenant override middleware and tests
src/backend/services/analytics-api/src/auth.rs, src/backend/services/analytics-api/src/api/tenant_resolution_tests.rs
Rewrites tenant middleware to inject or override SecurityContext from the host context and request header, and rewrites tests around the injected-context behavior.
Route registration via register_routes
src/backend/services/analytics-api/src/api/mod.rs
Replaces router() with register_routes(router, openapi, state), retargets AppState.config, and rewires route registration and middleware layering.
Handlers use Extension and subject tenant
src/backend/services/analytics-api/src/api/admin/handlers.rs, src/backend/services/analytics-api/src/api/catalog.rs, src/backend/services/analytics-api/src/api/handlers.rs, src/backend/services/analytics-api/src/api/error.rs
Switches handler state extraction to Extension and changes tenant scoping from ctx.insight_tenant_id to ctx.subject_tenant_id().
Domain auth and admin-threshold tenant plumbing
src/backend/services/analytics-api/src/domain/auth.rs, src/backend/services/analytics-api/src/domain/admin_threshold/service.rs, src/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rs, src/backend/services/analytics-api/src/domain/admin_threshold/live_tests.rs
Switches SecurityContext imports to toolkit_security, updates test helpers to use SecurityContext::builder(), and routes authz, ownership, audit, and cache calls through ctx.subject_tenant_id().
Live DB tests and CI orchestration
src/backend/services/analytics-api/src/api/http_live_tests.rs, src/backend/services/analytics-api/src/domain/catalog/live_tests.rs, src/backend/services/analytics-api/src/domain/schema_validator/live_tests.rs, src/backend/services/analytics-api/src/migration/live_tests.rs, src/backend/services/analytics-api/src/migration/mod.rs
Adds live-route coverage, removes shared DB reset helpers, updates schema/catalog live tests to run against shared seeded data, and drops the old migration live-test module.

Estimated code review effort: 4 (Complex) | ~75 minutes

Possibly related PRs

Suggested reviewers: ktursunov

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: migrating analytics-api to an idiomatic gears-rust gear.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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-api/config/insight.yaml`:
- Line 78: The runnable analytics config currently commits a real MySQL
credential in the database_url field, which should not be present in the
checked-in insight.yaml. Update the config entry to use an empty or
placeholder-only value and rely on deployment-time secret injection instead,
keeping the change localized to the database_url setting in insight.yaml.
- Around line 6-10: The no-auth listener in analytics-api is still reachable
from arbitrary in-cluster workloads, so tighten the trust boundary around the
listener setup and tenant override path. Update the server binding/auth
configuration in insight.yaml and the related crate::auth handling so only
gateway traffic can reach the REST host, either by adding a
NetworkPolicy/mTLS/trusted-proxy validation or by binding the listener to a
gateway-only interface. Make sure the X-Insight-Tenant-Id override is accepted
only after verifying the request came through the expected gateway path.

In `@src/backend/services/analytics-api/src/auth.rs`:
- Around line 44-55: tenant_middleware currently falls back to
SecurityContext::anonymous when the host-injected context is missing, which
should be treated as a deny/fail-closed case instead. Update tenant_middleware
to require a SecurityContext in the request extensions and return an
unauthorized/forbidden Response when it is absent, rather than continuing with
anonymous. Keep read_session_tenant and rebuild_with_tenant for valid contexts
only, and ensure any tenant override logic cannot run without the
gateway-provided SecurityContext.

In `@src/backend/services/analytics-api/src/gear.rs`:
- Around line 83-92: The Redis connection logs in the connection success/error
path leak sensitive credentials by printing cfg.redis_url. Update the logging in
the Redis backend setup in gear.rs to use a redacted representation or only safe
metadata (such as host, port, or db) in both the tracing::info! and
tracing::warn! calls, keeping the error context but never emitting the full
redis:// URL.

In `@src/backend/services/analytics-api/src/main.rs`:
- Around line 95-96: The `--print-config` path in `main` currently prints the
full `config.to_yaml()` output, which can expose secrets. Update the
`cli.print_config` branch to serialize a redacted config instead, masking
sensitive fields like `database_url`, `clickhouse_password`, and `redis_url`
before calling `println!`. Keep the change localized to the
configuration-printing logic so the normal runtime config handling remains
unchanged.
- Around line 100-103: The `Commands::Check` branch in `main` only succeeds
after loading `AppConfig`, so it never verifies the analytics gear config is
present or valid. Update the `check` path to call `extract_gear_config` and then
run a `GearConfig::validate()` helper (or a `gear::validate_config` wrapper) so
`gears.analytics-api.config` and required URLs / partial ClickHouse credentials
are actually validated. Keep the change centered around the `Commands::Check`
match arm and the gear config extraction/validation symbols.
🪄 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: 28a136b5-45a3-4533-8209-ca8eaa09d6fd

📥 Commits

Reviewing files that changed from the base of the PR and between 3ad1ff3 and 4cf20c2.

⛔ Files ignored due to path filters (1)
  • src/backend/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (16)
  • src/backend/services/analytics-api/Cargo.toml
  • src/backend/services/analytics-api/config/insight.yaml
  • src/backend/services/analytics-api/src/api/admin/handlers.rs
  • src/backend/services/analytics-api/src/api/catalog.rs
  • src/backend/services/analytics-api/src/api/error.rs
  • src/backend/services/analytics-api/src/api/handlers.rs
  • src/backend/services/analytics-api/src/api/mod.rs
  • src/backend/services/analytics-api/src/api/tenant_resolution_tests.rs
  • src/backend/services/analytics-api/src/auth.rs
  • src/backend/services/analytics-api/src/config.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/live_tests.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/service.rs
  • src/backend/services/analytics-api/src/domain/auth.rs
  • src/backend/services/analytics-api/src/gear.rs
  • src/backend/services/analytics-api/src/main.rs

Comment on lines +6 to +10
# The `api-gateway` system gear is the REST host with auth DISABLED — the
# platform api-gateway is the sole authenticator and proxies to us, so this
# host injects a single-tenant SecurityContext (DEFAULT_TENANT_ID). The
# `analytics-api` gear then optionally overrides the tenant from the
# `X-Insight-Tenant-Id` header (see crate::auth). NO oidc plugin is registered.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for deployment-level isolation around analytics-api:8081.
fd -i 'networkpolicy|service|deployment|ingress|values|kustomization|helm' . \
  | xargs -r rg -n -C3 'analytics-api|8081|NetworkPolicy|auth_disabled|X-Insight-Tenant-Id'

Repository: constructorfabric/insight

Length of output: 50384


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== analytics-api helm files =="
for f in \
  src/backend/services/analytics-api/helm/values.yaml \
  src/backend/services/analytics-api/helm/templates/service.yaml \
  src/backend/services/analytics-api/helm/templates/deployment.yaml \
  src/backend/services/analytics-api/helm/Chart.yaml \
  charts/insight/values.yaml
do
  echo
  echo "---- $f ----"
  nl -ba "$f" | sed -n '1,220p'
done

echo
echo "== network policy / exposure search =="
rg -n -C2 'kind: NetworkPolicy|api-gateway|analytics-api|8081|auth_disabled|X-Insight-Tenant-Id|ingress:' \
  src/backend/services charts/insight deploy/gitops environments docs \
  --glob '!**/target/**' --glob '!**/node_modules/**' || true

Repository: constructorfabric/insight

Length of output: 298


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== analytics-api helm values (selected) =="
sed -n '1,120p' src/backend/services/analytics-api/helm/values.yaml | cat -n

echo
echo "== analytics-api service template =="
sed -n '1,120p' src/backend/services/analytics-api/helm/templates/service.yaml | cat -n

echo
echo "== analytics-api deployment template =="
sed -n '1,140p' src/backend/services/analytics-api/helm/templates/deployment.yaml | cat -n

echo
echo "== NetworkPolicy search =="
rg -n -C3 'kind: NetworkPolicy|apiVersion: networking.k8s.io/v1|podSelector:|namespaceSelector:|policyTypes:|analytics-api|8081' \
  src/backend/services charts/insight deploy/gitops docs \
  --glob '!**/target/**' --glob '!**/node_modules/**' || true

Repository: constructorfabric/insight

Length of output: 50384


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== exact NetworkPolicy hits =="
rg -n 'kind: NetworkPolicy|apiVersion: networking.k8s.io/v1' \
  src/backend/services charts/insight deploy/gitops environments docs \
  --glob '!**/target/**' --glob '!**/node_modules/**' || true

echo
echo "== api-gateway route and auth context =="
sed -n '340,380p' charts/insight/values.yaml | cat -n
echo
sed -n '1,120p' src/backend/services/api-gateway/src/proxy.rs | cat -n

Repository: constructorfabric/insight

Length of output: 6763


Restrict the no-auth listener to gateway traffic. The service is ClusterIP, but it still binds 0.0.0.0:8081 and there’s no NetworkPolicy here, so any in-cluster workload can hit analytics-api directly, bypass the gateway’s JWT checks, and set X-Insight-Tenant-Id. Add a policy/mTLS/trusted-proxy check, or bind the host to a gateway-only interface.

🤖 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/backend/services/analytics-api/config/insight.yaml` around lines 6 - 10,
The no-auth listener in analytics-api is still reachable from arbitrary
in-cluster workloads, so tighten the trust boundary around the listener setup
and tenant override path. Update the server binding/auth configuration in
insight.yaml and the related crate::auth handling so only gateway traffic can
reach the REST host, either by adding a NetworkPolicy/mTLS/trusted-proxy
validation or by binding the listener to a gateway-only interface. Make sure the
X-Insight-Tenant-Id override is accepted only after verifying the request came
through the expected gateway path.

Comment thread src/backend/services/analytics-api/config/insight.yaml Outdated
Comment on lines +44 to +55
pub async fn tenant_middleware(mut req: Request, next: Next) -> Response {
// Host-injected context (fallback to anonymous if, for some reason, the
// host didn't inject one — e.g. a direct pod hit on a non-public route).
let base = req
.extensions()
.get::<SecurityContext>()
.cloned()
.unwrap_or_else(SecurityContext::anonymous);

let Some(tenant_id) = tenant_auth.resolve_tenant(session_tenant) else {
return tenant_unresolved_response();
};

let ctx = SecurityContext {
// TODO: populate from JWT `sub` claim when JWT validation lands.
subject_id: Uuid::nil(),
insight_tenant_id: tenant_id,
let ctx = match read_session_tenant(&req) {
Some(tenant_id) => rebuild_with_tenant(&base, tenant_id),
None => base,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Fail closed when the host SecurityContext is missing.

Line 51 allows tenant-scoped routes to continue with SecurityContext::anonymous, and Line 54 can still honor a tenant override header. Since downstream writes and filters use ctx.subject_tenant_id(), a direct/internal hit without the gateway-injected context can bind requests to an anonymous or header-selected tenant.

🛡️ Proposed fail-closed guard
 use axum::extract::Request;
+use axum::http::StatusCode;
 use axum::middleware::Next;
-use axum::response::Response;
+use axum::response::{IntoResponse, Response};
@@
-    let base = req
-        .extensions()
-        .get::<SecurityContext>()
-        .cloned()
-        .unwrap_or_else(SecurityContext::anonymous);
+    let Some(base) = req.extensions().get::<SecurityContext>().cloned() else {
+        tracing::warn!("missing host-injected SecurityContext on tenant-scoped route");
+        return StatusCode::UNAUTHORIZED.into_response();
+    };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub async fn tenant_middleware(mut req: Request, next: Next) -> Response {
// Host-injected context (fallback to anonymous if, for some reason, the
// host didn't inject one — e.g. a direct pod hit on a non-public route).
let base = req
.extensions()
.get::<SecurityContext>()
.cloned()
.unwrap_or_else(SecurityContext::anonymous);
let Some(tenant_id) = tenant_auth.resolve_tenant(session_tenant) else {
return tenant_unresolved_response();
};
let ctx = SecurityContext {
// TODO: populate from JWT `sub` claim when JWT validation lands.
subject_id: Uuid::nil(),
insight_tenant_id: tenant_id,
let ctx = match read_session_tenant(&req) {
Some(tenant_id) => rebuild_with_tenant(&base, tenant_id),
None => base,
pub async fn tenant_middleware(mut req: Request, next: Next) -> Response {
// Host-injected context (fallback to anonymous if, for some reason, the
// host didn't inject one — e.g. a direct pod hit on a non-public route).
let Some(base) = req.extensions().get::<SecurityContext>().cloned() else {
tracing::warn!("missing host-injected SecurityContext on tenant-scoped route");
return StatusCode::UNAUTHORIZED.into_response();
};
let ctx = match read_session_tenant(&req) {
Some(tenant_id) => rebuild_with_tenant(&base, tenant_id),
None => base,
🤖 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/backend/services/analytics-api/src/auth.rs` around lines 44 - 55,
tenant_middleware currently falls back to SecurityContext::anonymous when the
host-injected context is missing, which should be treated as a deny/fail-closed
case instead. Update tenant_middleware to require a SecurityContext in the
request extensions and return an unauthorized/forbidden Response when it is
absent, rather than continuing with anonymous. Keep read_session_tenant and
rebuild_with_tenant for valid contexts only, and ensure any tenant override
logic cannot run without the gateway-provided SecurityContext.

Comment thread src/backend/services/analytics-api/src/gear.rs Outdated
Comment on lines +95 to +96
if cli.print_config {
println!("Effective configuration:\n{}", config.to_yaml()?);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Redact secrets in --print-config.

config.to_yaml() can include database_url, clickhouse_password, and redis_url. Printing the full effective config risks leaking credentials into terminals, CI logs, or support bundles.

🤖 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/backend/services/analytics-api/src/main.rs` around lines 95 - 96, The
`--print-config` path in `main` currently prints the full `config.to_yaml()`
output, which can expose secrets. Update the `cli.print_config` branch to
serialize a redacted config instead, masking sensitive fields like
`database_url`, `clickhouse_password`, and `redis_url` before calling
`println!`. Keep the change localized to the configuration-printing logic so the
normal runtime config handling remains unchanged.

Comment thread src/backend/services/analytics-api/src/main.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 `@charts/insight/templates/secrets.yaml`:
- Around line 156-162: The analytics-api MariaDB DSN in the secrets template is
building `APP__gears__analytics-api__config__database_url` with raw credentials,
unlike the existing `IDENTITY__mariadb__url` handling. Update the DSN
construction in the `charts/insight/templates/secrets.yaml` template to
percent-encode the username and password before formatting the MySQL URL, so
credentials containing special characters are parsed correctly by analytics-api.

In `@deploy/gitops/scripts/compose-app-secrets.sh`:
- Around line 132-138: The DSN composition in compose-app-secrets.sh is writing
raw secret values into APP__gears__analytics-api__config__database_url and
APP__gears__analytics-api__config__redis_url, which can break when credentials
contain reserved characters. Update the secret-generation logic to URL-encode
the user/password parts before building these URLs, and make the behavior match
the existing Helm template handling for the same DSNs. Use the analytics-api
config keys in this script as the place to apply the fix.

In `@src/backend/services/analytics-api/helm/templates/deployment.yaml`:
- Around line 17-18: The deployment rollout trigger only covers the ConfigMap,
so `insight-analytics-api` pods can keep stale values from `envFrom` secrets
such as `tenant_default_id` or rotated credentials. Update the `deployment.yaml`
template to add a checksum annotation for the relevant secret/config source used
by `envFrom`, or wire in a reloader, alongside the existing `checksum/config`
annotation so pods restart when secret-backed config changes.
🪄 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: 98589891-2583-414d-9855-b9105921ea50

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf20c2 and 4f157c4.

📒 Files selected for processing (9)
  • charts/insight/templates/secrets.yaml
  • charts/insight/values.yaml
  • deploy/gitops/environments/local/values.yaml.template
  • deploy/gitops/scripts/compose-app-secrets.sh
  • docker-compose.yml
  • src/backend/services/analytics-api/Dockerfile
  • src/backend/services/analytics-api/helm/templates/configmap.yaml
  • src/backend/services/analytics-api/helm/templates/deployment.yaml
  • src/backend/services/analytics-api/helm/values.yaml
✅ Files skipped from review due to trivial changes (3)
  • src/backend/services/analytics-api/helm/values.yaml
  • deploy/gitops/environments/local/values.yaml.template
  • charts/insight/values.yaml

Comment thread charts/insight/templates/secrets.yaml Outdated
Comment thread deploy/gitops/scripts/compose-app-secrets.sh Outdated
Comment on lines +132 to +138
APP__gears__analytics-api__config__database_url: "mysql://${MDB_USER}:${MDB_PW}@${MDB_HOST}:${MDB_PORT}/${MDB_DB}"
APP__gears__analytics-api__config__clickhouse_url: "http://${CH_HOST}:${CH_PORT}"
APP__gears__analytics-api__config__clickhouse_database: "${CH_DB}"
APP__gears__analytics-api__config__clickhouse_user: "${CH_USER}"
APP__gears__analytics-api__config__clickhouse_password: "${CH_PW}"
APP__gears__analytics-api__config__identity_url: "http://${RELEASE}-identity:8082"
APP__gears__analytics-api__config__redis_url: "${REDIS_URL}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

URL-encode credentials before writing DSNs.

These values come from Secrets, so BYO passwords containing @, :, /, %, or quotes can produce an invalid MySQL/Redis URL in the generated Secret. Encode credentials before composing the URL strings, matching the Helm template behavior.

🤖 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 `@deploy/gitops/scripts/compose-app-secrets.sh` around lines 132 - 138, The DSN
composition in compose-app-secrets.sh is writing raw secret values into
APP__gears__analytics-api__config__database_url and
APP__gears__analytics-api__config__redis_url, which can break when credentials
contain reserved characters. Update the secret-generation logic to URL-encode
the user/password parts before building these URLs, and make the behavior match
the existing Helm template handling for the same DSNs. Use the analytics-api
config keys in this script as the place to apply the fix.

Comment on lines +17 to +18
# Roll pods when the gears config changes.
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check whether analytics-api has a Secret checksum/reloader contract elsewhere.
rg -n 'checksum/(secret|config)|reloader|insight-analytics-api-config|existingSecret' \
  src/backend/services/analytics-api/helm charts/insight deploy/gitops

Repository: constructorfabric/insight

Length of output: 5202


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the deployment and related config/secret templates.
for f in \
  src/backend/services/analytics-api/helm/templates/deployment.yaml \
  src/backend/services/analytics-api/helm/templates/configmap.yaml \
  src/backend/services/analytics-api/helm/values.yaml \
  charts/insight/templates/secrets.yaml \
  deploy/gitops/scripts/compose-app-secrets.sh
do
  echo "===== $f ====="
  sed -n '1,220p' "$f"
  echo
done

Repository: constructorfabric/insight

Length of output: 27004


Add a rollout trigger for insight-analytics-api-config envFrom secrets are only read at pod start, and this deployment only hashes the ConfigMap. Secret rotations or tenant_default_id changes can leave running pods on stale credentials/config unless a secret checksum or reloader is wired in.

🤖 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/backend/services/analytics-api/helm/templates/deployment.yaml` around
lines 17 - 18, The deployment rollout trigger only covers the ConfigMap, so
`insight-analytics-api` pods can keep stale values from `envFrom` secrets such
as `tenant_default_id` or rotated credentials. Update the `deployment.yaml`
template to add a checksum annotation for the relevant secret/config source used
by `envFrom`, or wire in a reloader, alongside the existing `checksum/config`
annotation so pods restart when secret-backed config changes.

@ktursunov
ktursunov self-requested a review July 1, 2026 15:23
@cyberantonz
cyberantonz marked this pull request as draft July 1, 2026 15:41
@cyberantonz
cyberantonz marked this pull request as ready for review July 1, 2026 17:23
cyberantonz added a commit to cyberantonz/insight that referenced this pull request Jul 2, 2026
- config/insight.yaml: drop the committed `mysql://insight:insight@…`
  default; database_url/clickhouse_url are empty and injected per-deploy
  via APP__* env overrides (no credential in the checked-in runnable config).
- gear.rs: redact userinfo from redis_url before logging (redis:// URLs
  can embed credentials); add `check_config` that validates the gear
  config section + required URLs without touching any backend.
- main.rs: `check` subcommand now runs `gear::check_config` instead of a
  no-op success.
- charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password
  in the analytics DSN, matching the identity DSN, so operator credentials
  with :/@/%/ chars produce a valid connection string.

Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed
(the deliberate header-trust posture — infra follow-up), --print-config
redaction (operator debug flag, parity with api-gateway), bash DSN
url-encoding (pre-existing, also affects identity — needs shared helper),
Secret-change reload (Secret is externally managed in gitops).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
@cyberantonz

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai — addressed in c08de15. Per-finding:

Fixed

  • insight.yaml:78 — committed DB credentials. database_url/clickhouse_url are now empty in the checked-in config; connection strings are injected per-deploy via APP__gears__analytics_api__config__* (umbrella Secret / compose). No credential in the runnable config.
  • gear.rs:96 — redact redis_url. Added redact_url() (strips user:pass@); both boot log sites now log the redacted form. (The migrate-path warn never logged the URL.)
  • main.rs:104check was a no-op. It now runs gear::check_config() which proves gears.analytics-api.config exists, deserializes, and has non-empty database_url/clickhouse_url — no backend connection.
  • secrets.yaml:162 — DSN not percent-encoded. The analytics database_url now urlquery-encodes the MariaDB user/password, matching the identity DSN.

Already fixed earlier in this PR

  • compose-app-secrets.sh:131 — backticks in unquoted heredoc. Removed in 40a1285.

Deferred, with reason

  • insight.yaml:10 + auth.rs:55 — no-auth listener reachable in-cluster / fail-closed. This is the intentional header-trust posture: the platform api-gateway is the sole authenticator and proxies to this host (auth_disabled), which injects a single-tenant SecurityContext; the X-Insight-Tenant-Id override is the documented bridge. Under auth_disabled the host always injects a context, so the anonymous() branch is a defensive fallback, not the live path. The correct mitigation is a network trust boundary (NetworkPolicy restricting ingress to the gateway pod / mTLS), which is a deploy-infra change — tracked as follow-up, happy to add a NetworkPolicy template if wanted.
  • main.rs:97 — redact --print-config. It's an explicit operator/debug flag with the same behavior as api-gateway's; the checked-in file now has no secrets (see above). Not adding a bespoke redactor here.
  • compose-app-secrets.sh:138 — url-encode bash DSNs. Pre-existing and applies equally to the identity DSN in the same script; the right fix is a shared urlencode helper across both, out of scope for this PR.
  • deployment.yaml:18 — Secret-change reload. In gitops the Secret is externally managed (compose-app-secrets.sh, resource-policy: keep), so a helm checksum/secret can't cover it; the ConfigMap already has checksum/config.

Also flagged as follow-up (not CodeRabbit): the gear's ConfigMap renders as insight-analytics-api-config, same name as the Secret (different kinds) — will give it a distinct name separately.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@cyberantonz Thanks for the detailed rundown and for pushing the fixes in c08de15 (and the backtick fix earlier in 40a1285). Confirming the fixed items:

  • insight.yaml credentials removed / injected at deploy time — good.
  • redact_url() for the redis_url logging in gear.rs — reasonable approach.
  • gear::check_config() giving check real teeth (existence + deserialization + non-empty URLs, no live connection) — matches the intent of my original comment.
  • Percent-encoding the MariaDB DSN in secrets.yaml to match the identity DSN — good consistency fix.

On the deferred items, your reasoning holds up:

  • The auth_disabled header-trust model with the gateway as sole authenticator is a legitimate architecture choice as long as the network boundary (NetworkPolicy/mTLS) is tracked as a real follow-up — happy to see a NetworkPolicy template in a later PR if you go that route.
  • Not redacting --print-config is fine now that the checked-in config carries no secrets.
  • Deferring the bash DSN url-encode helper as a cross-cutting fix (shared with identity) rather than a one-off patch here makes sense.
  • The externally-managed Secret / resource-policy: keep explanation for why checksum/secret can't be added is a fair constraint of the gitops model.

Sounds like a solid state to land this in. Let me know when the NetworkPolicy follow-up or the ConfigMap/Secret naming collision fix are up and I'll take a look.

ᕦ(ò_óˇ)ᕤ


✏️ Learnings added
Learnt from: cyberantonz
URL: https://github.com/constructorfabric/insight/pull/1546

Timestamp: 2026-07-02T09:00:20.237Z
Learning: In this repository, `src/backend/services/analytics-api` runs behind an `auth_disabled` gears-rust host (api-gateway). The platform api-gateway is the sole authenticator and injects a `toolkit_security::SecurityContext` into requests; `tenant_middleware` in `src/backend/services/analytics-api/src/auth.rs` only optionally overrides the tenant via `X-Insight-Tenant-Id`, and its `SecurityContext::anonymous()` fallback is a defensive-only path, not reachable in the live auth_disabled flow. The intended mitigation for unauthenticated in-cluster reachability is a network trust boundary (NetworkPolicy/mTLS restricting ingress to the gateway pod), tracked as deploy-infra follow-up rather than an application-layer fix.

Learnt from: cyberantonz
URL: https://github.com/constructorfabric/insight/pull/1546

Timestamp: 2026-07-02T09:00:20.237Z
Learning: In `charts/insight` gitops deployments, the `insight-analytics-api-config` Secret (and similar secrets) is externally managed by `deploy/gitops/scripts/compose-app-secrets.sh` with Helm `resource-policy: keep`, so Helm cannot compute a `checksum/secret` annotation for it to trigger pod restarts on change. Only the ConfigMap (rendered via Helm templates, e.g. `configmap.yaml`) supports a `checksum/config` annotation for restart-on-change; Secret rotation must be handled separately (e.g., manual rollout or external tooling).

Learnt from: cyberantonz
URL: https://github.com/constructorfabric/insight/pull/1546

Timestamp: 2026-07-02T09:00:20.237Z
Learning: `deploy/gitops/scripts/compose-app-secrets.sh` does not URL-encode DSN user/password segments for bash-composed connection strings (e.g., analytics-api's database_url); this is a pre-existing gap shared with the identity service's DSN in the same script. A fix should introduce a shared urlencode helper used by both services rather than a one-off patch.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

cyberantonz added a commit to cyberantonz/insight that referenced this pull request Jul 2, 2026
- config/insight.yaml: drop the committed `mysql://insight:insight@…`
  default; database_url/clickhouse_url are empty and injected per-deploy
  via APP__* env overrides (no credential in the checked-in runnable config).
- gear.rs: redact userinfo from redis_url before logging (redis:// URLs
  can embed credentials); add `check_config` that validates the gear
  config section + required URLs without touching any backend.
- main.rs: `check` subcommand now runs `gear::check_config` instead of a
  no-op success.
- charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password
  in the analytics DSN, matching the identity DSN, so operator credentials
  with :/@/%/ chars produce a valid connection string.

Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed
(the deliberate header-trust posture — infra follow-up), --print-config
redaction (operator debug flag, parity with api-gateway), bash DSN
url-encoding (pre-existing, also affects identity — needs shared helper),
Secret-change reload (Secret is externally managed in gitops).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
@cyberantonz
cyberantonz force-pushed the analytics-api-gear branch from 1176808 to 93a80d5 Compare July 2, 2026 09:53
cyberantonz and others added 11 commits July 2, 2026 14:09
Migrate analytics-api from a self-hosted axum::serve service to a
gears-rust host on toolkit::bootstrap::run_server:

- main.rs: toolkit bootstrap (AppConfig::load_or_default → run_server /
  gear::run_migrate); links the api-gateway REST host + auth/authz/tenant
  system gears via inventory.
- gear.rs (new): AnalyticsApiGear #[toolkit::gear(capabilities=[rest])].
  Gear::init reconstructs the full former run_server startup (self-managed
  MariaDB pool + 45 migrations + CHECK/product-default probes, redis
  catalog cache, ClickHouse + Identity clients, schema validator,
  admin-CRUD service), stores AppState in a OnceLock, and tokio::spawns the
  one-shot validate_all() scan. RestApiCapability::register_rest wires the
  OperationBuilder routes into the host router + Extension(AppState).
- Auth: host runs auth_disabled (the platform api-gateway is the sole
  authenticator); it injects a SecurityContext with DEFAULT_TENANT_ID
  (== the single-tenant default the seeded data uses). The former
  hand-rolled tenant_middleware becomes a thin, stateless layer that reads
  toolkit_security::SecurityContext and overrides the tenant from
  X-Insight-Tenant-Id when present. The local SecurityContext struct is
  removed; handlers + domain services now use toolkit_security only.
- DB stays self-managed (no toolkit `db` capability; ClickHouse is not a
  toolkit-db backend anyway).
- config.rs: figment loader removed; GearConfig is deserialized from
  gears.analytics-api.config via ctx.config(). New config/insight.yaml in
  the gears layout (auth_disabled: true), modeled on api-gateway's
  no-auth.yaml.

Build, clippy (pedantic), and tests (325 pass) all green. Helm/compose
manifests still reference the old ANALYTICS__* env model and are updated
in a follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The gear migration switched analytics-api from figment (`ANALYTICS__*`
env, no config file) to toolkit AppConfig (a gears-layout YAML at `-c`
plus `APP__*` env overrides). Update both deploy paths to match:

- Dockerfile: bake config/ into the image; launch
  `-c /app/config/insight.yaml run`.
- docker-compose.yml: bind-mount the config dir, add `-c … run`, and
  rename every `ANALYTICS__<x>` env var to
  `APP__gears__analytics-api__config__<x>`.
- Helm (services/analytics-api): new configmap.yaml (the gears structure
  with the auth-disabled api-gateway host); deployment mounts it at
  /app/config/analytics.yaml, launches `-c … run`, fixes the migrate
  initContainer likewise, and rolls on config change via checksum/config.
- Umbrella secret (charts/insight) + compose-app-secrets.sh: rename the
  generated `ANALYTICS__*` keys to `APP__gears__analytics-api__config__*`
  (hyphenated names delivered via envFrom, same as api-gateway's
  `APP__gears__oidc-authn-plugin__*`). Structural config now lives in the
  mounted ConfigMap; the Secret carries only the leaf overrides
  (DB/clickhouse/redis/identity URLs + tenant_default_id).
- Drop the now-host-owned `bind_addr` env var; refresh stale comments
  (the /health-400 CrashLoop rationale no longer applies — the
  auth-disabled host injects DEFAULT_TENANT_ID and /health is off the
  auth path).

Validated: `docker compose config -q`, `helm template` of the chart, and
the gears config YAML all parse cleanly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Apply rustfmt to the files touched by the gears-rust migration — the
migration left several formatting violations (fmt is not covered by
clippy, so build/clippy/test were green while `cargo fmt --check` was
not). No behavioral change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Runtime fix: with the gears-rust host, config overrides arrive as
env vars under `gears.<name>.config.*`. The gear name is
`analytics-api` (kebab, macro-enforced), so the natural env key was
`APP__gears__analytics-api__config__*` — but hyphenated env-var names
are silently dropped by the compose `sh`/dash entrypoint (dash discards
env names that aren't valid identifiers) and skipped by Kubernetes
`envFrom`. The result: every override (DB URL, tenant, clickhouse,
redis, identity) was stranded at its YAML default, so the gear tried to
reach `localhost:3306` and failed init with "pool timed out". (The old
service worked because `ANALYTICS__*` had no hyphen.)

Fix, keeping the kebab gear name:
- Rename every override key to the identifier-safe alias
  `APP__gears__analytics_api__config__*` (compose env, umbrella Secret,
  gitops compose-app-secrets.sh) — survives dash and k8s envFrom.
- The toolkit does no kebab/snake normalization, so bridge it in
  main.rs: `fold_gear_env_alias` deep-merges the `analytics_api` gears
  alias into `analytics-api` right after config load (both `run` and
  `migrate` paths). Verified via `--print-config`: the underscore env
  keys now land in `gears.analytics-api.config`.

Build, clippy (-D warnings), fmt, and tests (325) all green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
With the toolkit host, the api-gateway system gear's `rest_prepare`
already registers `/health` and `/healthz`. Our gear also registered
`/health`, so route composition panicked at boot with "Overlapping
method route. Handler for `GET /health` already exists".

Drop the gear's `/health` route (and the now-unused `handlers::health`).
Also build the gear's endpoints on a fresh sub-router so the
tenant-override middleware + `AppState` extension scope to analytics-api's
own routes and don't wrap the host's `/health`/`/healthz`/`/openapi.json`.

DB now connects (the earlier env-key fix landed); this was the next boot
panic. Build, clippy (-D warnings), fmt, tests (325) green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The analytics-api/identity config Secrets are emitted via an unquoted
`cat <<EOF` heredoc (unquoted so ${MDB_USER} etc. expand). Backticks in
the heredoc *body* comments were therefore run as commands, printing
`keep: command not found`, `APP__gears__analytics_api__config__*: command
not found`, `ANALYTICS__*: command not found` during `make deploy`. The
`keep` one was pre-existing; the APP__/ANALYTICS__ ones came in with the
env-key rename.

Remove the backticks from the heredoc comments (shell comments elsewhere
in the file are unaffected — bash doesn't substitute in `#` lines). Only
comment text was corrupted, not the Secret data, but the noise is gone
and the script is correct.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Migration 20260623_ai_personal_gold_views (from constructorfabric#1514 ai-personal-gold)
selects and GROUP BYs c.source / c.source_id from silver.class_ai_dev_usage,
but the seed's placeholder DDL was never updated to add those columns.
On a fresh ClickHouse the view creation fails with UNKNOWN_IDENTIFIER
("Identifier 'c.source' cannot be resolved"), breaking `seed silver`.

Add source/source_id (String) to the class_ai_dev_usage placeholder,
matching the real silver model — same placeholder-mirror gap the file
already documents for the post-20260601 columns. Not gear-related;
surfaced while validating the analytics-api gear on a clean k8s cluster.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
- config/insight.yaml: drop the committed `mysql://insight:insight@…`
  default; database_url/clickhouse_url are empty and injected per-deploy
  via APP__* env overrides (no credential in the checked-in runnable config).
- gear.rs: redact userinfo from redis_url before logging (redis:// URLs
  can embed credentials); add `check_config` that validates the gear
  config section + required URLs without touching any backend.
- main.rs: `check` subcommand now runs `gear::check_config` instead of a
  no-op success.
- charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password
  in the analytics DSN, matching the identity DSN, so operator credentials
  with :/@/%/ chars produce a valid connection string.

Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed
(the deliberate header-trust posture — infra follow-up), --print-config
redaction (operator debug flag, parity with api-gateway), bash DSN
url-encoding (pre-existing, also affects identity — needs shared helper),
Secret-change reload (Secret is externally managed in gitops).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The gear migration changed how analytics-api boots — it's now a
toolkit::bootstrap host that needs `-c <gears-config> run` and reads
`APP__gears__analytics_api__config__*` (not the old `ANALYTICS__*`). The
e2e harness still launched the bare binary with `ANALYTICS__*` env, so the
new binary came up with empty config and the suite failed.

Update lib/analytics_api.py to spawn the way the deployment does:
- pass `-c <repo>/services/analytics-api/config/insight.yaml run`
- inject leaf overrides via APP__* (direct Popen execve preserves the
  hyphenated gear-name segments, unlike the compose sh entrypoint)
- override the REST-host bind on the api-gateway gear to loopback:port
- give grpc-hub a per-spawn UDS so parallel workers/re-runs don't collide
- widen the health-wait to 60s (host inits all system gears + migrations)
- /health is now host-served (public); refresh the stale tenant comments

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Adds tests for the pure/new logic introduced by the gear migration and
refactors the route table to be testable without a DB:

- api/mod.rs: split the OperationBuilder chains into a stateless
  `build_operations(router, openapi)`; `register_routes` now just layers
  state/middleware over it and merges into the host router. A test drives
  `build_operations` so the full 20-endpoint route table + OpenAPI
  registration is exercised with no AppState/DB (0% → ~95%).
- main.rs: tests for `fold_gear_env_alias` + `deep_merge` (alias→canonical
  fold, object merge, leaf overwrite).
- gear.rs: tests for `redact_url` (strips userinfo) and `check_config`
  (missing section / empty required URLs).
- config.rs reaches 100% via the above.

Patch coverage for the crate rises ~40% → 60%. The remainder is
MariaDB/ClickHouse-bound boot + handler code (gear init, infra::db
connect, AdminThresholdService, REST handler bodies) that needs a live
DB — exercised by the e2e suite + the #[ignore]d live_tests, neither of
which runs in the unit-coverage job.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…torfabric#1564)

Wire the existing `rust:` CI job to exercise analytics-api's `#[ignore]`d
live_tests against a real MariaDB — no new job, no ClickHouse (CH-gated
tests skip themselves).

CI (`.github/workflows/ci.yml`, `scripts/ci/*`):
- Add a `live_db` component flag (components.py) threaded into the matrix
  entry (changed.py); default false, true only for analytics-api.
- For live_db entries the rust job starts a MariaDB 11.4 container, applies
  migrations once up front via `analytics-api migrate` (a failed migration
  fails the job), then runs the live_tests in parallel with
  INTEGRATION_TESTS_MARIADB_URL set (`--include-ignored`). Both the migrate
  and the tests run under llvm-cov, so the DB paths now count toward coverage
  (patch coverage 60% -> 73%).

Parallel-safe live_tests (no DB reset):
- Migrations run once before the suite; tests never migrate or reset the DB.
- Isolation is per-tenant / per-metric-key, so the suite is parallel-safe:
  admin_threshold + catalog + schema_validator each own unique keys/tenants.
- Drop the per-test `reset_catalog` + `Migrator::up` blocks and the helper.
- Fix an admin_threshold key collision: use the full UUID (`metric_id.simple()`)
  instead of the v7 timestamp prefix `[..8]`, which collided in same-ms runs.
- Delete migration/live_tests.rs: obsolete now that the CI migrate step (plus
  the startup CHECK probe) covers "migrations apply on an empty DB".

Identity resolution already uses Testcontainers MariaDB (MariaDbFixture) in
the dotnet job — no CI change needed there.

Verified: 8/8 fresh-DB parallel runs green (362 tests, ~3s); no-DB run skips
all 24 live_tests cleanly; fmt + clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
@cyberantonz
cyberantonz force-pushed the analytics-api-gear branch from 294f333 to 99d1a2e Compare July 2, 2026 12:10
…age gap (constructorfabric#1564)

The service-layer live_tests exercise the domain logic directly but never
drive the axum handlers, leaving the extract -> delegate -> Result->Response
glue at 0% (new-code patch coverage stuck at 73%, under the 80% gate).

Add `api/http_live_tests.rs`: builds a full AppState against the live DB and
mounts the real route table via `register_routes`, then drives it with
`tower::oneshot`. ClickHouse/Identity clients point at an unreachable address
on purpose — DB-backed handlers return real 2xx; the CH/Identity ones exercise
their entry + error-mapping path. Covers the admin-threshold CRUD round-trip
(201/200/204 status mapping), the metric/column/catalog reads, and the
unknown-field rejection envelope. Same `#[ignore]` + `INTEGRATION_TESTS_MARIADB_URL`
gating as the domain live_tests; a fresh tenant per write keeps it parallel-safe.

Result: `admin/handlers.rs` and `catalog.rs` 100%, `api/mod.rs` (register_routes)
100%, patch coverage 73% -> 84% (clears the gate).

Also fix a latent bug the reset-removal exposed:
`response_link_map_omits_metrics_dropped_by_walk_all` DELETEs the GLOBAL
`product-default` seed threshold for `ic_kpis.tasks_closed` and never restored
it — `reset_catalog` used to rebuild the seed each run and hid this. Left as-is
it corrupts the shared seed (every subsequent run/reader sees the metric
permanently dropped) and fails on re-run. Fix: capture the row (as CHAR via CAST
— sqlx-mysql can't decode DECIMAL->f64), delete, resolve, then RESTORE it before
any assertion can unwind. The `fetch_links` surfaced-ids filter already keeps
metrics + links consistent during the delete window, so concurrent readers
never observe a phantom. Verified: full suite 3x on one DB, no reset, seed
intact (good/warn preserved), 372 passed each; no-DB run skips all 34
live_tests; fmt + clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/backend/services/analytics-api/src/domain/catalog/live_tests.rs (1)

388-460: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore-before-return ordering bug: resolve() error skips seed restoration.

resolver.resolve(...).await? at line 428 propagates immediately on error, bypassing the restore loop below it (lines 430-459). Since these tests intentionally never reset/migrate the DB anymore, any transient failure here (not just an assertion failure) permanently deletes the product-default threshold row for target_metric_key from the shared seed — corrupting state for this test's own pre-conditions (lines 376-386) and any other test/resolver call depending on that row, for the rest of the CI run.

Capture the resolve outcome without short-circuiting, so restoration always runs before any error is propagated:

🛡️ Proposed fix
-    let after = resolver.resolve(Uuid::now_v7(), "", "").await?;
+    let after_result = resolver.resolve(Uuid::now_v7(), "", "").await;
 
-    // Restore the deleted seed row(s) before any assertion can unwind.
+    // Restore the deleted seed row(s) unconditionally — even if `resolve()`
+    // itself failed — so a transient error can never permanently corrupt
+    // the shared, non-reset seed data.
     for row in &saved {
         let id_hex: String = row.try_get("", "id_hex")?;
         let role_slug: String = row.try_get("", "role_slug")?;
         let team_id: String = row.try_get("", "team_id")?;
         let good_s: String = row.try_get("", "good_s")?;
         let warn_s: String = row.try_get("", "warn_s")?;
         let is_locked_s: String = row.try_get("", "is_locked_s")?;
         let lock_reason: Option<String> = row.try_get("", "lock_reason")?;
         db.execute(Statement::from_sql_and_values(
             db.get_database_backend(),
             "INSERT INTO metric_threshold \
                 (id, tenant_id, metric_key, scope, role_slug, team_id, good, warn, is_locked, lock_reason) \
              VALUES (UNHEX(?), NULL, ?, 'product-default', ?, ?, ?, ?, ?, ?)",
             [
                 Value::from(id_hex),
                 Value::from(target_metric_key),
                 Value::from(role_slug),
                 Value::from(team_id),
                 Value::from(good_s),
                 Value::from(warn_s),
                 Value::from(is_locked_s),
                 match lock_reason {
                     Some(r) => Value::from(r),
                     None => Value::String(None),
                 },
             ],
         ))
         .await?;
     }
+
+    let after = after_result?;
🤖 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/backend/services/analytics-api/src/domain/catalog/live_tests.rs` around
lines 388 - 460, The resolve call in the live test can short-circuit before the
deleted seed row is restored, leaving shared `metric_threshold` state corrupted.
Update the `resolver.resolve(...)` flow so restoration in the `saved` loop
always runs even when resolve fails, then propagate the resolve error afterward;
use the surrounding test block and the `saved`/restore logic as the anchor
points.
🤖 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 `@src/backend/services/analytics-api/src/domain/catalog/live_tests.rs`:
- Around line 388-460: The resolve call in the live test can short-circuit
before the deleted seed row is restored, leaving shared `metric_threshold` state
corrupted. Update the `resolver.resolve(...)` flow so restoration in the `saved`
loop always runs even when resolve fails, then propagate the resolve error
afterward; use the surrounding test block and the `saved`/restore logic as the
anchor points.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3aec5df1-88a3-4c41-a94c-56c4ba636c9d

📥 Commits

Reviewing files that changed from the base of the PR and between c08de15 and 5530465.

⛔ Files ignored due to path filters (1)
  • src/backend/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (35)
  • .github/workflows/ci.yml
  • charts/insight/templates/secrets.yaml
  • charts/insight/values.yaml
  • deploy/gitops/environments/local/values.yaml.template
  • deploy/gitops/scripts/compose-app-secrets.sh
  • deploy/seed/sql/placeholders.sql
  • docker-compose.yml
  • scripts/ci/changed.py
  • scripts/ci/components.py
  • src/backend/services/analytics-api/Cargo.toml
  • src/backend/services/analytics-api/Dockerfile
  • src/backend/services/analytics-api/config/insight.yaml
  • src/backend/services/analytics-api/helm/templates/configmap.yaml
  • src/backend/services/analytics-api/helm/templates/deployment.yaml
  • src/backend/services/analytics-api/helm/values.yaml
  • src/backend/services/analytics-api/src/api/admin/handlers.rs
  • src/backend/services/analytics-api/src/api/catalog.rs
  • src/backend/services/analytics-api/src/api/error.rs
  • src/backend/services/analytics-api/src/api/handlers.rs
  • src/backend/services/analytics-api/src/api/http_live_tests.rs
  • src/backend/services/analytics-api/src/api/mod.rs
  • src/backend/services/analytics-api/src/api/tenant_resolution_tests.rs
  • src/backend/services/analytics-api/src/auth.rs
  • src/backend/services/analytics-api/src/config.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/live_tests.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/service.rs
  • src/backend/services/analytics-api/src/domain/auth.rs
  • src/backend/services/analytics-api/src/domain/catalog/live_tests.rs
  • src/backend/services/analytics-api/src/domain/schema_validator/live_tests.rs
  • src/backend/services/analytics-api/src/gear.rs
  • src/backend/services/analytics-api/src/main.rs
  • src/backend/services/analytics-api/src/migration/live_tests.rs
  • src/backend/services/analytics-api/src/migration/mod.rs
  • src/ingestion/tests/e2e/lib/analytics_api.py
💤 Files with no reviewable changes (2)
  • src/backend/services/analytics-api/src/migration/live_tests.rs
  • src/backend/services/analytics-api/src/migration/mod.rs
✅ Files skipped from review due to trivial changes (4)
  • src/backend/services/analytics-api/src/api/error.rs
  • scripts/ci/components.py
  • src/backend/services/analytics-api/helm/values.yaml
  • charts/insight/values.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
  • deploy/gitops/environments/local/values.yaml.template
  • src/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rs
  • docker-compose.yml
  • src/backend/services/analytics-api/helm/templates/deployment.yaml
  • src/backend/services/analytics-api/Cargo.toml
  • deploy/seed/sql/placeholders.sql
  • src/backend/services/analytics-api/src/api/admin/handlers.rs
  • src/backend/services/analytics-api/Dockerfile
  • src/backend/services/analytics-api/src/api/tenant_resolution_tests.rs
  • src/backend/services/analytics-api/src/api/catalog.rs
  • src/backend/services/analytics-api/config/insight.yaml
  • src/backend/services/analytics-api/src/main.rs
  • src/backend/services/analytics-api/src/domain/admin_threshold/service.rs
  • src/backend/services/analytics-api/src/gear.rs
  • deploy/gitops/scripts/compose-app-secrets.sh
  • src/backend/services/analytics-api/src/api/mod.rs
  • src/backend/services/analytics-api/src/auth.rs
  • src/backend/services/analytics-api/src/api/handlers.rs
  • src/backend/services/analytics-api/src/config.rs

…ot shared mutation

Replace the capture->delete->restore approach (which mutated the GLOBAL
`product-default` seed row for `ic_kpis.tasks_closed`) with a fully isolated,
purely-additive fixture that proves the same ADR-003 `surfaced_ids` invariant
through tenant scoping alone — the model's own rule: a DEFAULT metric
(`tenant_id IS NULL`) is visible to everyone, and per-tenant threshold data
never intersects across tenants.

The test now inserts a GLOBAL catalog row with a UNIQUE key (catalog is
global-only in v1), links it into `metric_query_catalog`, and gives it a
threshold for ONE tenant. Resolving as that tenant surfaces it + its link;
resolving as any other (never-seen) tenant has no candidate, so `walk_all`
drops it and the surfaced-ids filter drops its link. No shared row is read or
mutated, nothing is deleted or restored.

Verified: shared seed untouched (tasks_closed product-default stays intact),
full suite 3x on one DB with no reset (372 passed each), no-DB run skips all
34 live_tests, fmt + clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz and others added 2 commits July 2, 2026 17:04
…cy spillover)

`cargo llvm-cov --package X` reports every instrumented file, including
path-dependency crates compiled into X's test binary. So the analytics-api
report also carried `libs/insight-clickhouse` at ~15% (only `Client::new` is
hit from analytics-api) — and the gate buckets files into components via
`component_for`, so those lines land on the insight-clickhouse component.

Today this is masked: the gate merges all reports with max-hits, and this
branch also changes insight-clickhouse (its own job reports ~93%), so the union
wins. But on a PR that touches analytics-api and NOT insight-clickhouse, the
latter's job never runs — the only insight-clickhouse data is analytics-api's
15% spillover, which would fail the component's OVERALL_MIN (80%) gate. No
amount of insight-clickhouse tests fixes that; the spillover must not be
counted here in the first place.

Add a per-component `cover_ignore_regex` (threaded through changed.py) wired to
`cargo llvm-cov report --ignore-filename-regex`. Set `src/backend/libs/` on
analytics-api (the only rust service that links a shared lib). Each library
crate keeps self-reporting in its own job. Verified: the analytics-api report
no longer lists any insight-clickhouse file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
… (no ClickHouse)

Raise the crate's own coverage without a live ClickHouse by testing the pure,
connection-free surface that was untested: `Client::new` auth branches,
`query()` with/without a timeout, the `inner()`/`config()` accessors, and
`tenant_query` validation (lib.rs 46% -> 100% line); the password-redacting
`Config` Debug impl (config.rs -> 100%); and `Error` Display (error.rs ->
100%). Crate line coverage 88% -> 94%.

The remaining gap is `QueryBuilder::fetch_all` — query EXECUTION, which needs a
live ClickHouse and is out of scope here (cf/insight#1564 skips CH).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
@cyberantonz
cyberantonz merged commit dbe992a into constructorfabric:main Jul 2, 2026
30 checks passed
@cyberantonz
cyberantonz deleted the analytics-api-gear branch July 3, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants