Repository navigation
Insights opt-in cross-tenant benchmarks - #1925
Conversation
Insights writes mapped to the administrator's scope at the route, so a CFO could not acknowledge an alert, save a view or start a background export, and an auditor could not sign an evidence pack (403 Missing scope). Roles that read Insights now hold insights:write and the family's writes take it; handlers still decide who may act. BI logins and the money-saved changes move to the insights_admin family, whose writes stay admin-only at the route too.
Auditors keep audit:read and insights:read only. Exports, saved views, the last visit and evidence-pack signatures change no organisation state, so they move to the insights_personal family, which needs insights:read. Acknowledging an alert stays an insights:write action.
…s one Administrators and the CFO add, change and delete budgets for the organisation, a company or a cost centre at /dashboard/insights/budgets (API /insights/budgets); auditors read them. Each shows this period's ledger spend against its limit. Every change is a signed audit row. The alert sweep raises cost.forecast_over_budget when the month's forecast, in USD, is above a tenant-wide monthly budget. The budget evaluator no longer posts slack or webhook alerts to the platform-wide URLs, and escapes the budget name in its email.
A view_pptx background export renders a landing view as a deck: title, tiles table, a native chart with its text summary for each chart and the chart's data as a table. Metric and explorer exports also download as Parquet with typed columns and the definitions and filters as metadata. Migration v6z116 widens the insights_exports kind and format checks.
The shared export menu offers Parquet, so the platform export endpoint accepts it (it renders through the same export module).
Administrators opt the organisation in; an opted-in organisation sees its run success rate, cost per run, approvals per 100 runs and average run time against the 25th, 50th and 75th percentiles of opted-in cohorts. Migration v6z117 adds the opt-in table (forced RLS), the published cohort table (no tenant ids) and SECURITY DEFINER functions that enforce the 20-tenant and 100-run floors in the database. A nightly task publishes the cohorts; the platform view shows the opted-in count.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01bf342de9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| published = ( | ||
| (await session.execute(select(InsightsBenchmark).where(InsightsBenchmark.cohort.in_(cohorts)))).scalars().all() |
There was a problem hiding this comment.
Between midnight UTC and the 04:10 publication task, this query returns yesterday's published bands while own_figures() uses today's newly shifted 30-day window. The response labels both with today's window and calculates positions across different periods. A failed publication leaves this mismatch in place indefinitely because neither window_from nor window_to is checked. Use the published window for the organisation's figures, or withhold bands that do not match the requested window.
Useful? React with 👍 / 👎.
| number = _number(row.get("value", row.get("count"))) | ||
| if number is None: | ||
| continue | ||
| labels.append(str(row.get("label") or row.get("name") or row.get("key") or row.get("t") or "")) |
There was a problem hiding this comment.
For the Finance deck, resolve_cost_by_agent() returns labels in agent_name and resolve_cost_by_model() returns them in model, but neither field is read here. Consequently, both spend breakdowns export with blank category labels; table_rows() repeats the same omission, so the accompanying tables cannot identify the agents or models either. Handle the actual metric row shapes in both helpers, matching the label selection already used by ui/src/lib/insights/chartData.ts.
Useful? React with 👍 / 👎.
| row.company_id = _uuid(changes["company_id"], "invalid_company", "company_id") | ||
| if "cost_center_id" in changes: | ||
| row.cost_center_id = _uuid(changes["cost_center_id"], "invalid_cost_center", "cost_center_id") | ||
| if "company_id" in changes or "cost_center_id" in changes: | ||
| await _check_scope(session, tenant_id, row.company_id, row.cost_center_id) |
There was a problem hiding this comment.
When a PATCH supplies a nonexistent company or cost-centre UUID, these assignments dirty the persistent budget before _check_scope() executes its ORM SELECT. Both production session factories use SQLAlchemy's default autoflush, so that SELECT first attempts the UPDATE and raises a foreign-key IntegrityError, rather than returning the intended BudgetError and 404. Validate the proposed scope before assigning it, or suppress autoflush during validation, so invalid scope changes do not become unhandled 500 responses.
Useful? React with 👍 / 👎.
The full suite left the shared app without the Insights routes in CI, so the test now reads each router and checks api/main.py mounts them.
…erage scan test_rls_tenant_coverage reads ENABLE ROW LEVEL SECURITY statements by table name; the integration test's alembic call carries a reasoned S603 note.
Insights Phase 5, release 3 of 5. Stacked on #1924 (merge that first; it is contained here).
Opt-in cross-tenant benchmarks, as the PRD decided: opt-in only, and a cohort of at least 20 organisations.
What a tenant sees
PUT /insights/benchmarks/opt-in,insights_adminfamily, audited).Privacy, enforced in the database (migration v6z117)
insights_benchmark_optins(forced RLS): one row per tenant that took part.public.insights_benchmark_cohorts(p_from, p_to, p_min_tenants, p_min_runs): SECURITY DEFINER, owned by the v6z107 platform role,search_path = pg_catalog.GREATEST).public.insights_benchmark_participation()returns the opted-in count, which the Platform view shows.insights_benchmarksstores the published figures with no tenant id; a check refuses fewer than 20 tenants.agenticorg_apponly, and no membership left with the installer.scripts/create_app_role.sqlallow-lists both functions.Verification
computepublishes without tenant ids.allwhere the plan's cohort is too small.tsc, eslint.