The Trilogy Times

## Summary

Two config fixes for pipelines that failed on the P1 prod release (PR #31):

- school-master-data-sync: first invocation raised ValueError: Master sheet URL not configured. Set EDU_SCHOOLS_DATA_SHEET_URL environment variable or pass master_sheet_url in params. The env var was never added to pipeline.json. Set it to the authoritative Alpha School Master Data sheet (documented in Klair's data-lineage-v2/layer-1-sor/016-l1-school-master-data-gsheet.md).

- edu-expense-report-sender: all 4 scheduled sends failed with SES MessageRejected - Email address is not verified: noreply@klairvoyant.ai. The handler caught each per-schedule error and returned 200 to Lambda (silent fail). Swap SES_SOURCE_EMAIL to noreply@klair.ai, which is already verified in us-east-1 and in use by orphan-classes-pipeline.

## Verification

Pulled CloudWatch from /klair/pipelines/prod/school-master-data-sync and /klair/pipelines/prod/edu-expense-report-sender to confirm root causes.

The sheet URL in this PR matches the one in Klair's data-lineage docs — no secrets exposed (the sheet relies on a Google service account, not a public link).

## Out of scope

- ps-pipeline failure (missing surtr/netsuite-ps-credentials secret) — tracked separately; requires the NetSuite OAuth2 private key from the old Klair deployment.

- quickbooks-expense-analysis Zoom classification warning — cosmetic, pipeline completed successfully (20 cost opportunities, $14,124/mo est. savings).

## Test plan

- [ ] Merge to main, CI builds and deploys the two Lambdas

- [ ] Manually invoke pipeline-school-master-data-sync-prod — confirm handler reaches Google Sheets auth / sync steps (no ValueError at line 77)

- [ ] Manually invoke pipeline-edu-expense-report-sender-prod — confirm Execution completed in ...: N successful, 0 failed in CloudWatch

- [ ] Check recipient inbox for the expected weekly report (recipients are internal @klair.ai addresses, safe to test)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

## Screenshots

<img width="1585" height="652" alt="Screenshot 2026-04-21 141135" src="https://github.com/user-attachments/assets/e64df1dc-ce3f-4405-9474-46dfba88f231" />

<img width="1615" height="687" alt="Screenshot 2026-04-21 141214" src="https://github.com/user-attachments/assets/e4713e62-1a8c-4bb7-a92e-639c51c85a79" />

<img width="936" height="885" alt="Screenshot 2026-04-21 141329" src="https://github.com/user-attachments/assets/7739571d-7747-42c9-99c2-901c44fc6dcf" />

<img width="1592" height="898" alt="Screenshot 2026-04-21 141410" src="https://github.com/user-attachments/assets/127dff8d-f05d-4fcc-a421-218d72262399" />

<img width="1601" height="867" alt="Screenshot 2026-04-21 141435" src="https://github.com/user-attachments/assets/303ebad4-34c2-43fb-8f0c-f5ba980a6d81" />

---

## Summary

Hardens the analytics-worker projection layer in three categories, each driven by a specific finding from this week's [Convex projections audit](../blob/main/context_cache/audit_aerie_convex_projections.md):

1. Tier 1 — reliability: kill the "Many documents read" warnings the worker logs every cycle on joeChartsBudgetActuals and similar tables (~29k/32k docs read per upsert today).

2. Tier 2 — stop wasting work: drop projections with no consumers; refresh static reference tables on a daily/weekly cadence instead of hourly.

3. Tier 3 — observability: add live freshness indicators to dashboard headers so leadership can see "data as of X" at a glance — counters the recurring "but is this even current?" question that drives most "let's just hit Redshift" momentum.

Goal is to make the hourly worker boring: no warnings, no failed batches, no wasted writes.

---

## Prod impact (please prioritise this PR's review)

Dev Convex went from 18 distinct critical/warning insights to 5 after the original PR #118 commits landed. Verified on the Aerie dev Convex Health -> Insights dashboard.

The 13 that disappeared map directly to PR #118 work:

- 5 joeCharts:upsert* "Nearing documents read limit" criticals/warnings -> resolved by Tier 1 composite-index refactor

- 4 replaceEnrollmentCohortStudents / marketing:upsertMarketingEventContacts / enrollment:clearPipelineStudents / marketing:insertWeeklyDeposits write-conflict criticals -> resolved by .unique() migration + dead-projection drops reducing write contention

- 4 retried-write-conflict warnings on the same tables -> same root cause

Prod is still on main and is therefore still hitting all 18 — including the criticals. Until this PR merges and deploys, the prod worker is one growth nudge from upsertBudgetActuals failing the 32k document read limit (it was at ~29k as of the audit).

The two commits added since the original review (below) cover 2 of the remaining 5 dev warnings — the most dangerous one (a separate read-limit risk on _checkPipeline at 31k/32k) and a silent data-loss bug on expenseTransactions that was under-counting prod by every multi-line QuickBooks expense. After these merge, dev should be at 3 warnings, all auto-retried tolerable noise.

### Follow-up commits added since initial review

| Commit | Issue | Fix |

|---|---|---|

| [a8b2b7e](#) | expenseTransactions was keying upserts on transactionId alone. QuickBooks expenses split across multiple schools share one transaction_id (e.g. one Dell invoice → $379 Alpha Miami + $1,137 Austin K-8). Each subsequent line silently overwrote the previous via .unique() patch. Local Aerie was under-counting prod by 102 vs 101 in the 4/13–4/19 sample — exactly the multi-line transactions in the window. | Project the source PK (staging_education.quickbooks_expense_transactions.id BIGINT IDENTITY) as sourceRowId, key the Convex upsert on it. Matches Klair's quickbooks_expense_analysis_service.py which surfaces id directly. Logs when a patch changes line amount materially (audit trail). Adds pnpm backfill-expense-transactions for fast local re-population. Verified: 102 rows / $37,111.41 in window, exact parity with Klair. |

| [d2ce81c](#) | dataConsistency:_checkPipeline was at 31k/32k document reads — 3% from the same total-failure mode the joeCharts upserts had. pipelineFunnel accumulates history (every snapshot/program/stage row forever), and the function .collect()'d the whole table just to find the latest snapshot per program. | Two indexed reads per program (latest-date probe + same-date scan) using existing by_programCode_snapshotDate_stageId index. Reads drop from O(historyDepth × programs × stages) → O(programs × (1 + stages)) — about 310 reads vs 30k+ for a 39-program / 7-stage funnel. Verified locally against prod-imported data: runConsistencyCheck returns 663 stages across 39 programs, no truncation, no warnings. |

| [b4ad842](#) | Joe Charts → Financial → Model vs Actuals showed "Actual $/student" 2–10× inflated vs Klair across every category. Two coordinated bugs: (a) queryPlRecords did not ::text-cast its date columns, so the Postgres driver returned JS Date objects which z.coerce.string() then mangled into "Thu Jan 01 2026 00:00:00 GMT-0600 ..." strings that broke every period filter; (b) getModelVsActuals filtered >= periodStart with no upper bound while Klair's _compute_period_dates returns (start, today) and uses >= start AND <= today, so forward-looking fct_pl entries inflated YTD totals. | Added ::text cast to queryPlRecords (sync side) and execPeriodEnd() helper applied at the one Joe Charts site that aggregates from plRecords (chat side). Deliberately left the 5 other Joe Charts queries that aggregate from joeChartsBudgetActuals (getKPIs, getBudgetVarianceByClass, getExpenseBreakdown, getAlerts) on >= start only — Klair's equivalent queries against consolidated_budgets_and_actuals use the same start-only pattern (joe_charts_edu_service.py:233, :720), and adding an upper clamp would zero out current-quarter KPIs whenever month-end snapshot dates fall after today. Also adds pnpm backfill-pl-records for fast local re-population. Verified: Model vs Actuals (Holdings, Portfolio, YTD, 3,200 students) now matches Klair row-for-row across all 9 categories. |

### Known follow-up

| Dashboard | Issue | Plan |

|---|---|---|

| Joe Charts → CAPEX & AP | Small data drift vs Klair: Total Open card shows 166 invoices / $16.70M period AP vs Klair's 196 / $16.50M; Current Days $-7K and 91+ Days $-10K despite matching invoice counts. Per-bucket counts and amounts for 1-30 / 31-60 / 61-90 buckets match exactly. | Three projection-layer bugs: (1) r.lineAmount ?? r.totalAmount fallback in refresh.ts:1095 inflates totals when source rows have null line_amount (Klair uses 0); (2) double pro-rate of balance — once at projection write time, again at dashboard read time; (3) total_transactions returns exploded line-item count rather than source-row count. Cleanest fix flattens joeChartsQBAP to per-line rows matching Klair's source schema and removes the JSON re-explode round-trip in the dashboard. Out of scope here — will be a follow-up PR (~2-3h refactor + verification). This is the last apparent Joe Charts parity issue. |

### Intentionally deferred with rationale

| Issue | Rationale |

|---|---|

| Medium #9 — Redshift queries still run every cycle | Cadence only skips Convex writes. Convex cost dominated the hot path; Redshift queries on static reference tables are milliseconds. Documented; not a safety issue. |

| Medium #10 — Optional fields in composite keys | Duplicate check on imported prod data showed 0 collisions across all 7 tables. Fixing in-mutation creates duplicates against existing data; needs a one-time migration PR. File issue. |

| Medium #11 — NetSuite AP status in composite key | Needs finance input on whether table semantically wants "current snapshot" (drop status from key) or "history" (keep). File issue. |

| Medium #12 — UTC date in tooltip can be off-by-one | Minor UX polish. Swap to toLocaleDateString(...) in a follow-up. |

| Low #14 — Redundant Tooltip.Provider on badge | Nested providers are a no-op per Radix docs; keeping the self-wrap lets the component work in unit tests without additional setup. |

| Low #15 — new args object per useQuery render | Convex compares serialized args; useMemo is micro-optimization that trades clarity for ~nothing. |

| Low #16 — rolling vs calendar interval semantics | Intentional and documented in module comment. Fine for an internal tool. |

| Nit #17 — queryFunnelActivity location in PR description | Corrected in this body update. |

---

## Tier 1 — Reliability

Seven joeCharts upserts shared the same anti-pattern: a partial withIndex() narrowing followed by .filter() over the trailing fields of the unique key. For popular dimensions (e.g. Holdings BU on upsertBudgetActuals) this scanned thousands of docs per record and tripped Convex's "Many documents read" warning at ~29k/32k.

Fix per table: add a composite by_uniqueKey index covering the FULL unique key, then refactor the mutation to use .unique() against that index. Lookup goes from O(n-per-key) to O(log n).

| Mutation | Old: index + filter | New: composite index |

|---|---|---|

| upsertBudgetActuals | (BU, dataSource) + filter (period, className) | (BU, dataSource, period, className) |

| upsertStudentEnrollment | (campusName) + filter (status, quarter) | (campusName, status, quarter) |

| upsertMapGrowth | (campus) + filter (termName) | (campus, termName) |

| upsertMapGrowthBySubject | (campus, subject) + filter (grade) | (campus, subject, grade) |

| upsertFinancialModels | (schoolName, modelName) + filter (metricName) | (schoolName, modelName, metricName) |

| upsertCapex | (periodType) + filter (className) | (className, periodType) |

| upsertNetSuiteAP | (periodType) + filter on 4 fields | (vendorName, periodType, subsidiaryName, className, status) |

Also bonus correctness: .unique() enforces the invariant the schema implies (one row per composite key) where the old .filter().first() would silently pick whichever row the index returned first if duplicates ever existed.

---

## Tier 2 — Stop wasting work

### 2a. Stop refreshing dead projections

analyticsDeals and contactActivity have zero runtime consumers — no dashboard, AI chat tool, or cron query reads them; they were only self-merged by the writer (audit Finding 3). The cron stops calling upsertDeals and upsertContactActivity; queryFunnelActivity is also dropped since nothing else needs it. queryDeals is kept because contact enrichment uses deals to derive enrollment periods even though we don't persist them anymore.

Schema entries and the upsert mutations themselves are preserved with @deprecated JSDoc — a follow-up PR will drop the tables once we're sure existing rows are safe to remove for a release cycle. Two-step on purpose so any forgotten consumer surfaces in this PR's bake-in window rather than as a runtime error.

### 2b. Per-table refresh cadence

New module sync/src/analytics/refresh-cadence.ts lets domains opt into weekly or daily refresh instead of the default hourly. Applied to the seven non-chat-only static tables flagged in the audit:

| Cadence | Domains |

|---|---|

| weekly | schoolMappings, crossSystemMappings, joeChartsCapacityMap, joeChartsSchoolMetadata |

| daily | programs, joeChartsUnitEconomics, joeChartsFinancialModels |

Implementation is opt-in: anything not in REFRESH_TIERS defaults to hourly so behaviour for time-series tables (snapshots, projections, funnel) is unchanged. In-memory tracker on the worker process — restart forces a refresh on next cycle, which is fine.

Per-table chat-only projections are not touched in this PR.

---

## Tier 3 — Freshness indicators on dashboards

Counters the "but how do I know if this data is even current?" anxiety that drives most "let's just hit Redshift directly" momentum. Reactive Convex makes the live badge nearly free — the moment a fresh projection lands, the badge re-renders without a refresh button.

- chat/convex/analytics/freshness.ts — getDomainFreshness + getSingleDomainFreshness queries for five domains (admissions, joeCharts, wiki, pmo, camps). Uses Convex's always-available _creationTime system index (.order("desc").first()) so it works against any representative table regardless of whether it has a snapshotDate index.

- New <DashboardFreshnessBadge> component — color-tiered (fresh <2h green / aging <24h yellow / stale red / unknown slate). Self-wraps with Tooltip.Provider so it works in any rendering context including unit tests.

- Wired into FunnelView (admissions) and CampsView (camps) as a PoC. Other dashboards can adopt the same one-liner: <DashboardFreshnessBadge domain="..." />.

---

## Tests

| Suite | Result |

|---|---|

| sync (full suite, 46 files, 654 tests) | passing |

| chat/convex/freshness.test.ts (4 new) | passing |

| chat/convex/buildoutDetails.test.ts (12) | passing |

| chat/convex/dashboards.test.ts (41) | passing |

| chat/convex/eventProcessor.test.ts (42) | passing |

| chat/components/dashboards/admissions/camps/__tests__/camps-view.test.tsx (8, modified due to badge integration) | passing |

New test coverage:

- 6 unit tests on refresh-cadence covering tier defaults, independent domain tracking, hourly fallback for unlisted domains, and a REFRESH_TIERS allow-list test that catches accidentally adding chat-only tables here.

- 4 query tests on getDomainFreshness — empty-table case, non-empty selection, single-domain variant, unknown-domain fallthrough.

- Updated 2 refreshEntityRecords tests to reflect the new "deals not upserted" semantics; failure-isolation property still under test.

Pre-existing failures NOT fixed by this PR: lib/__tests__/agent.test.ts has 2 getDataDir tests that fail on main today (verified). Unrelated to this work; tracked separately.

---

## Deploy sequence (one-time)

The expense sourceRowId migration (commit a8b2b7e) leaves legacy rows in

prod that the new upsert can't see -- without the purge below, every sync

cycle would INSERT a fresh row alongside each legacy row and the Expense

Analysis dashboard would double-count. Run these steps in order when

deploying this PR to any environment with pre-existing expenseTransactions

data:

1. Merge & deploy (Convex schema + functions land first via the existing

pipeline).

2. Pause the analytics worker (or wait until it's idle between cycles)

so it can't re-insert ghost rows mid-purge.

3. Run the purge until hasMore returns false -- it pages 4k rows per

call to stay under the Convex 32k document read limit:

   # repeat until { ..., "hasMore": false }
npx convex run analytics/expenses:_clearLegacyExpenseTransactions

4. Repopulate with the corrected schema. Either resume the worker (next

cycle picks up where the watermark left off) or run the focused backfill

for faster validation:

   pnpm --filter @bran/sync backfill-expense-transactions --dry-run  # preview
pnpm --filter @bran/sync backfill-expense-transactions             # for real
# The script prompts for confirmation when CONVEX_URL points at a non-dev
# deployment; set BACKFILL_CONFIRM=yes to bypass for non-interactive runs.

5. Resume the analytics worker.

6. Spot-check the Expense Analysis dashboard -- counts and totals should

match Klair's QuickBooks Expense Analysis view for the same date window

(we verified 102 rows / $37,111.41 in 4/13-4/19 against prod Klair).

P&L records (commit b4ad842) follow the same pattern but are simpler --

the worker will overwrite the bad date strings on its next cycle, or you

can force-refresh with pnpm --filter @bran/sync backfill-pl-records. No

purge needed because the upsert keys on plId (which existed pre-migration).

Follow-up PR after a bake-in window: tighten sourceRowId: v.optional(v.string())

to v.string() in analyticsSchema.ts so this class of ghost-row bug cannot

recur.

## Test Plan for Reviewer

CI runs tests, typecheck, and lint automatically.

### 1. Read the diff

- [ ] Confirm the 7 by_uniqueKey composite indexes in chat/convex/analyticsSchema.ts cover the FULL unique key for each table (matches the table in the Tier 1 section above)

- [ ] Confirm each refactored mutation in chat/convex/analytics/joeCharts.ts uses the new index AND switches .first() → .unique()

- [ ] Confirm REFRESH_TIERS in sync/src/analytics/refresh-cadence.ts only contains the 7 non-chat-only static tables

- [ ] Confirm the @deprecated mutations in chat/convex/analytics/entities.ts are still present (preserved for ad-hoc backfills) but no longer called by the cron

### 2. Local worker run (recommended — concrete signal that the reliability fix works)

# from repo root, with local Convex pointing at prod-like data
# (run pnpm sync:pull-prod-data first if your local is stale)
cd sync
$env:DATA_DIR = "../data"           # PowerShell — bash: export DATA_DIR=../data
npx tsx run-analytics-worker.ts     # or pnpm worker:analytics

Wait through one cycle (~5-10 min) and confirm:

- [ ] Each joeCharts push logs ✓ {table}: N records — zero "Many documents read" warnings, zero "X/N records failed" lines

- [ ] [analytics] Cross-system mappings pushed to Convex: N schools appears on first cycle (cadence-gated path fires when no prior run is recorded)

- [ ] No log lines for analyticsDeals or contactActivity upserts (those are now dropped)

- [ ] Joe Charts financial refresh completes in ~60s (previously slow + warning-prone)

After cycle 2 (let the worker keep running):

- [ ] [analytics] ⏭ {domain}: skipped (within {tier} cadence) lines appear for each tiered domain (cycle 2 within the daily/weekly window)

### 3. Local UI validation (optional)

docker compose build chat && docker compose up -d chat

Then open the browser:

- [ ] Open Admissions → Funnel → confirm freshness badge appears in the top filter row

- [ ] Open Admissions → Camps → confirm freshness badge appears (right-aligned)

- [ ] Hover the badge → tooltip shows "Data as of YYYY-MM-DD (Xh ago)"

- [ ] Confirm badge colour matches expected tier (green <2h, yellow <24h, red older, slate when no snapshot)

### 4. Direct query validation (optional, fast)

cd chat
npx convex run analytics/freshness:getDomainFreshness

Should return all 5 domains with snapshotDate and ageMs populated (or null/null if your local Convex is empty). Local validation result above shows what to expect when prod-like data is imported.

### 5. Post-deploy spot-check

After this PR merges:

- [ ] Tail prod analytics-worker logs through one full cycle — confirm zero "Many documents read" warnings (was firing on upsertBudgetActuals previously)

- [ ] Confirm the cadence-skip lines appear on the second cycle for the tiered domains

- [ ] Confirm dashboards still render correctly (no missing data from the removed analyticsDeals / contactActivity upserts)

---

## Risks & Mitigations

| Risk | Mitigation |

|---|---|

| Adding a 4th index per table increases write cost | Each index is small (3-5 fields, no large strings). Net win because the upsert .unique() path drops the dominant cost (32k+ doc reads). Convex pricing model favours fewer reads over fewer indexes. |

| Cadence in-memory state lost on worker restart | First cycle after restart refreshes all tiered domains; acceptable cost. Persisting cadence to Convex would add complexity for marginal benefit (worker restarts are rare). |

| @deprecated dead-projection mutations still callable | Intentional — preserved for ad-hoc backfills if a consumer ever materialises. Real cleanup (drop tables, drop mutations) is a separate PR after a bake-in window. |

| Freshness badge picks the wrong representative table | Mapping is hand-coded and greppable; trivial to swap a table if a domain owner disagrees. The badge degrades gracefully to "Unknown" when its representative table is empty. |

| Existing rows have duplicates on the new composite key (would break .unique()) | Validated locally against prod-imported data: zero collisions across all 7 tables. If prod somehow has duplicates we don't have, the worker logs a clear errors[label] from the catch — visible in the next cycle's log. |

---

## What's NOT in this PR

- AI Chat-only projection demotion (audit Finding 2) — needs usage data on the chat agent before deciding. Held back per current direction.

- JSON-string → sub-table migrations (audit Finding 5) — structural debt, deserves its own PR per table with backfill code and rollout plan. File issue.

- Incremental sync for huge tables (audit Recommendation 7) — architectural change to runRefreshCycle, deserves its own discussion.

- Drop the analyticsDeals / contactActivity schema entries — wait one release cycle to confirm no hidden consumers, then remove tables in a follow-up.

## Demo

### Azure Cost Reports

<img width="2624" height="1636" alt="image" src="https://github.com/user-attachments/assets/eefae772-1679-416b-aa21-3000ca6622dd" />

### Azure Token Usage

<img width="2624" height="1636" alt="image" src="https://github.com/user-attachments/assets/aa89d42c-e61e-41d4-8979-643f58d706bf" />

## Summary

- Adds a super-admin-only Raw Data Reports detail view inside AI Spend & Adoption (/ai-adoption) for inspecting raw source tables behind the rollups. First entry: Azure — Cost Reports (core_finance.ai_spend_azure_cost_reports).

- Backend: GET /api/ai-costs/raw/azure-cost-reports guarded by require_super_admin, returns { data, total_rows } preserving full 6-decimal cost_usd precision.

- Frontend: new AICostsShell wraps the existing AIAdoptionV2 screen, adds a header-strip "Raw Data Reports" button (super-admin only), a registry-backed picker page, and an AzureCostReportsView rendering all rows in UnifiedTable with client-side sort/search/column-selector, a pre-computed total row, and a custom full-precision CSV export button.

## Why

Implements [spec 01 — Azure Cost Reports Raw View](features/ai-spend-and-adoption/ai-spend-raw-data/specs/01-azure-cost-reports-raw-view/spec.md) for Linear ticket KLAIR-2579. Establishes the shell refactor, super-admin gating, breadcrumb pattern, picker registry, and first raw-table surface. Future specs register additional raw tables (Azure token usage, etc.) without structural changes.

## Notable deviations from spec

All documented in the spec's new "Implementation Deviations" section:

1. Shell wraps AIAdoptionV2, not AICosts. The /ai-adoption route already pointed to AIAdoptionV2; legacy screens/AICosts/ is unreferenced and left untouched. Files placed under screens/AIAdoptionV2/.

2. Custom Export CSV button replaces UnifiedTable's built-in export feature, which hardcodes .toFixed(2) on numeric cells and would drop precision. Custom button builds CSV from raw rows, preserving all 6 decimals of cost_usd.

3. Shell <h1> title visible across both modes — keeps the page title always present per FR2, without needing AIAdoptionV2 changes.

## Test plan

- [ ] As a super-admin, visit /ai-adoption → "Raw Data Reports" button appears in the header strip.

- [ ] As a non-super-admin, the button is hidden; endpoint returns 403.

- [ ] Click "Raw Data Reports" → picker lists "Azure — Cost Reports" with source-table label.

- [ ] Click the entry → view shows 3-level breadcrumb + header strip (source table, row count, min–max date range) + UnifiedTable with 5 default-visible columns; column selector reveals 4 more.

- [ ] Table sorts default by report_date DESC, search works, total row shows $<sum> under cost_usd.

- [ ] Click "Export CSV" → downloads azure-cost-reports-<yyyy-MM-dd>.csv with full 6-decimal precision.

- [ ] Breadcrumb levels return to picker / rollup correctly.

- [ ] pnpm tsc --noEmit passes in klair-client/.

- [ ] ESLint passes with --max-warnings 0 on changed files.

- [ ] uv run ruff format and uv run ruff check pass on changed backend files.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

## Screenshots

<img width="1694" height="936" alt="image" src="https://github.com/user-attachments/assets/b0de902a-d3f1-42b5-b930-a73e15c0435e" />

<img width="1660" height="915" alt="image" src="https://github.com/user-attachments/assets/34b2767e-3e94-4e1a-a44b-a952a54c53d7" />

---

## Summary

Demo Sprint DS4-DS7: ships the first two Review Agent checks, the /review endpoint, and the editor-side scorecard panel that renders the findings.

### Backend (DS4-DS6)

- CanonicalBudgetPlan.targets — new PlanTargets wrapper around the parsed BUDGET_BOT_TARGETS sheet, so checks read margin/ARR/retention targets from the canonical layer instead of poking DataPackage directly. has_targets joins the completeness flags.

- budget_bot/board_doc/review_checks/ — new package:

- _helpers.py: shared P&L row lookup + safe percent math (now guards denominator <= 0)

- margin_target.py (C2.1): planned vs approved EBITDA margin (pass / warning ≤5pp / critical >5pp)

- margin_trajectory.py (C2.6): Q-over-Q gross margin direction (pass if flat/improving ≥-0.5pp, warning ≤-2pp, critical >-2pp)

- __init__.py: CheckSpec registry + run_all_checks(plan, spec) with per-check exception isolation so a regression in one check can't take the whole scorecard down

- POST /board-doc/wizard/{id}/review — builds the canonical plan from session state, runs all registered checks, returns a fully-typed ReviewResponse { findings, skipped_checks, errored_checks, completeness, data_fetch_status, ran_at }. 409 if no spec yet, 502 only on orchestrator-level failure (per-source failures surface in data_fetch_status.failed_sources).

- Per-session asyncio.Lock so two parallel /review calls (multi-tab, double-click) serialise rather than racing on session.data_package.

- DataPackage negative cache — failed fetches go to a new failed_keys set rather than being silently stored as None. Subsequent /review clicks short-circuit known-failed sources by default; callers that want to retry pass retry_failed=True. Avoids burning 20-40s per click while upstream is down.

- scripts/run_review_checks.py — CLI that runs the full path against real BU data without booting the server. Same path the endpoint uses.

### Frontend (DS7)

- boardDocApi.ts — new types (ReviewFinding, ReviewResponse, ReviewSeverity, ReviewFindingStatus, ReviewCompleteness, ReviewDataFetchStatus), a runReview() API call, and a stable compareReviewFindings comparator (sort key: severity → check_area → check_id → finding_id).

- hooks/useReviewAgent.ts — owns the lifecycle state machine (idle → running → ready | error) with cross-session-safe concurrency:

- In-flight ref keyed by sessionId so a Run review click on session B mid-flight starts a fresh run instead of receiving session A's promise.

- Monotonic runIdRef ensures a stale resolve (slow A response landing after the user switched to B) is dropped on the floor instead of overwriting B's panel.

- Companion useReviewAgentResetOnSession hook for the page-level reset on sessionId change.

- Error branch wipes stale findings/completeness so the error state can never render mixed content.

- components/FindingCard.tsx — severity-tinted left border, expandable body showing why / preferred action / options / a humanised supporting-data table. Refactored to a single non-button container with sibling chevron + section buttons — no more invalid nested-button HTML, predictable a11y / keyboard tab order. Multi-line agent prose renders with white-space: pre-line.

- components/ReviewPanel.tsx — 360px right rail when open, 36px ribbon when collapsed (count badge now includes info findings, tinted by worst severity present so an info-only run still surfaces a signal). Header has Run review / Re-run + collapse. Body renders four states (idle / running / error / ready); ready state shows a summary header, findings grouped by check_area, an expandable "N passed" chip, and a static skipped-checks chip that surfaces missing_sources from the completeness payload.

- DocumentEditorPage.tsx — adds a Review toggle button in the top header (mirrors the panel's collapse), wraps editor + panel in a flex row so they share horizontal space without the editor losing its scroll behaviour. Wires useReviewAgentResetOnSession.

### Bonus bug fix (commits 2 + 3)

Live verification against Skyvera Q2 2026 surfaced that the "Data for Budget Bot" sheet has inverted column names for the EBITDA fields:

- *_ebitda_target → actually contains the margin %

- *_ebitda_margin_target → actually contains the dollar amount

Before the fix, C2.1 reported nonsense: *"planned EBITDA margin (60.3%) is 7991771.7pp below the approved target (7991832.0%)"*. Fixed in both canonical_plan._TARGET_FIELD_MAP and section_generators.get_ground_truth_metrics (the latter was also feeding the LLM swapped labels for every doc generation — this PR corrects that too). Both spots carry cross-referencing comments so when the GSheet column headers eventually get renamed, the inversion can be reverted cleanly. Captured as a tech-debt item (C1.7) in the backlog.

### Review feedback addressed

This PR went through internal review and addresses every flagged item except one stylistic disagreement (commits 6-10):

| Severity | Items addressed |

|---|---|

| Critical | C1: sessionId-keyed in-flight ref + stale-resolve guard. C2: reset on sessionId change via dedicated companion hook. |

| High | H3: structured data_fetch_status field distinguishes "data legitimately missing" from "upstream broke". H4: DataPackage.failed_keys negative cache. H5: compareReviewFindings finding_id tie-break. H6: FindingCard refactored to fix invalid nested <button> HTML. H7: per-check exception isolation in run_all_checks + new errored_checks field. |

| Medium | M8: per-session asyncio.Lock on /review. M9: documented as design choice (LLM prompt path wants string "60%", deterministic path wants float 60.0 — different consumers, different shapes; clarified with extensive docstring + regression test). M10: useReviewAgent.spec.ts covering C1/C2/sort. M11: stable-IDs caveat documented in hook docstring (blocks shipping a dismiss UI until backend produces stable IDs). M12: safe_pct guards denominator <= 0. |

| Low | L13: typed Pydantic ReviewResponse (no more list[Any] / dict[str, Any]). L14: cross-user 403/404 test. L15: error branch clears stale findings. L16: collapsed-panel badge now counts info findings, tints by worst severity. L17: white-space: pre-line on agent-emitted prose. L18: filed as backlog item C1.8 (pre-existing planner-module mislabel). |

| Nit | N19: hook docstring rewritten around cross-session guarantees. N20: implemented as a one-directional alias-subset invariant TEST rather than a "must stay in sync" comment (catches regression in CI; comments don't). N21: --year sanity-bounded (2020-2030). N22: SkippedChip docstring captures the design intent so a future refactor doesn't strip the missing_sources line. |

One disagreement: reviewer's framing of H4 suggested every click would burn 20-40s while upstream is down; in practice the orchestrator's failure path short-circuits faster, but the negative-cache fix still lands as recommended just for clearer semantics.

### Live verification

Running scripts/run_review_checks.py --bu skyvera -q 2 -y 2026 against real data produces:

[C2.1] Margin target hit/miss          PASS
Q2'26 planned EBITDA margin (60.3%) meets the approved target (60.0%).
[C2.6] Q-over-Q gross margin trajectory  WARNING
Gross margin degrading 2.0pp Q-over-Q (Q1'26: 80.0% → Q2'26: 78.0%).

One pass + one warning — exactly the kind of mixed scorecard that makes the demo feel real.

### Backlog impact

- DS4, DS5, DS6, DS7 → done

- C2.1, C2.6 → done (Phase C check suite, 2 of 17)

- C4.1, C4.6 → done (scorecard panel + re-run button); C4.2 partial (FindingCard ships in DS7, dismiss/addressed flow pending stable finding IDs per M11)

- C0.1, C0.4 → flipped to done (already shipped in #2634; backlog status was stale)

- C1.8 → new tech-debt item filed for the budget_translator.py mislabel surfaced during this review (pre-existing, separate consumer)

## Test plan

### Backend

- [x] New unit tests pass: 38 check + 8 endpoint + 7 target-extraction + 1 regression with real Skyvera payload, plus new tests for per-check isolation, negative cache, retry_failed, data_fetch_status, cross-user 403, safe_pct negative-denominator, and the alias subset invariant.

- [x] Live CLI verification against Skyvera Q2 2026 produces sensible findings.

- [x] Full tests/board_doc/ suite shows no new regressions (5 pre-existing LLM-mock failures unchanged).

- [ ] Reviewer spot-check: column-name inversion comments in both canonical_plan._TARGET_FIELD_MAP and section_generators.get_ground_truth_metrics agree.

- [ ] Reviewer spot-check: run_all_checks contract — empty list from a check means "skipped", at least one severity="pass" finding means "ran and passed", any exception lands in errored_checks and the request still returns 200.

### Frontend

- [x] pnpm lint --max-warnings 0 clean on all touched files.

- [x] pnpm tsc --noEmit clean.

- [x] useReviewAgent unit tests pass (7/7) — covers same-session de-dupe, different-session start-fresh, stale-resolve drop, stale-error drop, error clears findings, reset cancels in-flight, useReviewAgentResetOnSession wiring.

- [x] Open a Skyvera Q2 session in the editor → "Review" button visible in header, panel open by default on the right.

- [x] Click "Run review" → spinner appears, panel transitions to ready state in 20-40s.

- [x] Findings render grouped by check_area; severity-coded left borders; sort order critical → warning → info → pass; same-area same-severity ties broken stably by check_id then finding_id.

- [x] Click a finding → expands to show why / suggested action / options / "Numbers" table; click again to collapse. Chevron toggle button is a11y-correct (no nested buttons; section pill is independently focusable).

- [x] "1 passed" chip is visible and expands to show the C2.1 pass finding.

- [x] Click panel header chevron (or "Review" header button) → collapses to 36px ribbon with severity count badge; click again restores.

- [x] Force an error path → panel shows error state with "Try again" button; stale findings are wiped (no mixed "old findings + new error" content).

- [x] Re-run button on the panel header → triggers a second run, replacing previous findings.

- [x] Window narrowing: panel keeps fixed width, editor narrows but stays usable down to ~600px viewport.

## Supply-chain defense: add 7-day release cooldown on Python deps

### Why a minimum release age matters

Most malicious packages in supply-chain attacks are discovered and removed within hours to a few days of publication. Recent examples: the September 2025 chalk / debug / ansi-styles npm hijack (yanked within ~2 hours), the shai-hulud self-replicating worm, event-stream, ua-parser-js, coa, rc. The standard attacker playbook is to compromise a maintainer account (phishing, token leak), publish a patch-level version that looks legitimate, and collect credentials / wallets from CI machines before anyone notices.

A rolling age gate blocks that window: if builds only resolve to versions at least 7 days old, the vast majority of malicious publishes have already been yanked from the registry before we install them. The tradeoff is a one-week delay on legitimate security patches — acceptable because (a) real CVEs typically take longer than 7 days to see in-the-wild exploitation, and (b) the age gate can be overridden per-package for an urgent fix.

Klair's attack surface matters here: CI and the EC2 host run uv sync / pnpm install with AWS credentials, NPM tokens, and Secrets Manager access. A compromised transitive dependency executing postinstall on any of those hosts would have broad blast radius.

### What this PR does (uv / Python)

Adds a 7-day rolling supply-chain cooldown to three uv projects using uv's relative-duration syntax (exclude-newer = "7 days"):

| Project | Location |

| --- | --- |

| klair-api/ | new uv.toml |

| klair-lambdas/hubspot_sync_v2/ | new uv.toml |

| klair-misc/redshift-mcp-old/ | new uv.toml |

The rolling window auto-advances — no manual timestamp bumping. The setting does not affect already-pinned versions in uv.lock; it only gates what uv is allowed to resolve to on a fresh uv sync --upgrade or uv add. Existing lockfiles keep working.

Requires uv 0.9.17+ (the release that added relative-duration support — see the [uv settings reference](https://docs.astral.sh/uv/reference/settings/)). CI installs latest uv via the install script, so CI is fine. The EC2 deploy hosts run uv sync against whatever uv is already installed on the box (.github/workflows/ci-cd-backend.yml lines 63 and 113) — those hosts need to be on 0.9.17+ or uv sync will error on parse. Verify / upgrade uv on the dev and prod hosts as part of this rollout.

### What's still needed — JavaScript side (follow-up)

Not included in this PR because the story is more complicated:

#### klair-client/ (pnpm)

pnpm *does* support a rolling minimum release age via the .npmrc key minimum-release-age (value in minutes; 7 days = 10080). But:

- The repo pins pnpm@9.15.9 in the root package.json (line 12). The minimum-release-age setting shipped in pnpm 10.16+, so it will be silently ignored on 9.x. Enabling it requires bumping packageManager to pnpm@10.x first, which brings its own install-behavior and packageExtensions changes worth testing separately.

- Deploy uses pnpm install --no-frozen-lockfile (.github/workflows/ci_cd-frontend.yml line 77) — the exact call that would resolve new versions and needs the age gate most.

- Plan: (1) upgrade pnpm to 10.x in its own PR, verify CI + Amplify deploy; (2) add klair-client/.npmrc with minimum-release-age=10080 and optionally minimum-release-age-exclude[]=@trilogy-group so internal packages aren't blocked.

#### The 6 npm projects

klair-convex/, klair-e2e-playwright/, klair-misc/klair-mcp-ts/, klair-misc/klair-mcp-edu/, klair-udm/, klair-udm/netsuite-reports-poc/ — npm itself does not support any age-gate feature. An .npmrc with minimum-release-age is silently ignored by the npm client. Options:

- Migrate to pnpm (recommended for the two MCP servers — klair-mcp-ts and klair-mcp-edu — since they deploy to prod with secrets). Low-risk per-project change: install pnpm, pnpm import to convert the lockfile, update the CI workflow's npm ci to pnpm install --frozen-lockfile.

- Registry proxy (Artifactory, Verdaccio, Socket.dev firewall) — enforces the delay at the registry layer regardless of client. Best long-term solution but requires infra.

- Accept risk for dev-only projects (klair-convex/, klair-e2e-playwright/) that don't ship to prod hosts with secrets.

#### Dependabot

The repo has .github/workflows/dependabot-auto-merge.yml that auto-approves and auto-merges semver-patch PRs immediately, with no embargo. Even with age gates in place at resolve-time, Dependabot files PRs against the newly-published version. Two follow-ups worth considering:

1. Add a cooldown: block to a new .github/dependabot.yml so Dependabot itself waits 7 days before filing a PR.

2. Or add a "wait N days since release" check inside the auto-merge workflow before approving.

### Verification

- uv sync in each of the three projects continues to work from existing lockfiles (lockfile resolution is unaffected by exclude-newer when versions are already pinned).

- On uv 0.9.17+, uv sync --upgrade / uv add <pkg> will refuse packages published in the last 7 days.

- On uv <0.9.17, uv sync fails with a parse error on the "7 days" string — treat that as the signal to upgrade uv on that host.