From 7ab60fd7e5e8011ccc2dc7107761a3bdf30bccfd Mon Sep 17 00:00:00 2001 From: Adilislam0 Date: Sun, 28 Jun 2026 17:41:20 +0530 Subject: [PATCH 1/4] =?UTF-8?q?feat(metrics):=20add=20GET=20/api/v1/metric?= =?UTF-8?q?s/team=20=E2=80=94=20engineering=20velocity=20&=20inactivity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/lpi/main.py | 4 +- src/lpi/models.py | 90 +++++++++- src/lpi/routers/metrics.py | 332 +++++++++++++++++++++++++++++++++++++ src/lpi/store.py | 59 ++++++- tests/conftest.py | 15 ++ tests/test_metrics.py | 292 ++++++++++++++++++++++++++++++++ 6 files changed, 783 insertions(+), 9 deletions(-) create mode 100644 src/lpi/routers/metrics.py create mode 100644 tests/test_metrics.py diff --git a/src/lpi/main.py b/src/lpi/main.py index c2bce9c..21c1892 100644 --- a/src/lpi/main.py +++ b/src/lpi/main.py @@ -14,8 +14,7 @@ from fastapi import FastAPI from lpi.middleware import register_middleware -from lpi.routers import github_auth, goals, me, recommendations, signals, users, webhooks - +from lpi.routers import github_auth, goals, me, metrics, recommendations, signals, users, webhooks @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncIterator[None]: @@ -44,6 +43,7 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: app.include_router(signals.router, prefix="/api/v1/signals", tags=["signals"]) app.include_router(webhooks.router, prefix="/api/v1/webhooks", tags=["webhooks"]) app.include_router(github_auth.router, prefix="/api/v1/github", tags=["github_auth"]) +app.include_router(metrics.router, prefix="/api/v1/metrics", tags=["metrics"]) app.include_router( recommendations.router, prefix="/api/v1/recommendations", diff --git a/src/lpi/models.py b/src/lpi/models.py index f3b9ef0..ec0c5ce 100644 --- a/src/lpi/models.py +++ b/src/lpi/models.py @@ -217,4 +217,92 @@ class RecommendationFeedback(RecommendationFeedbackCreate): id: str user_id: str - created_at: datetime \ No newline at end of file + created_at: datetime + + +# ── Team Metrics models ( Engineering Velocity & Inactivity) ────── +# +# Owner: Adil Islam. QA: Daksh Garg. +# +# These back GET /api/v1/metrics/team (routers/metrics.py). The shapes here +# intentionally mirror the spec sheet's nested structure 1:1 so the frontend +# can deserialise the JSON response directly into matching objects without +# any field-renaming glue code. + + +class TeamSummary(BaseModel): + """Top-level aggregate counters — the "at a glance" numbers for the + team dashboard's header row. + + avg_signals_per_active_user is total_signals / active_users (NOT + total_signals / every roster member) — an inactive user contributing + zero recent signals shouldn't drag down the "how productive is an + engaged engineer right now" number. Rounded to 2 decimals; 0.0 when + there are no active users (avoids a ZeroDivisionError on a quiet day). + """ + + total_signals: int + active_users: int + inactive_users: int + avg_signals_per_active_user: float + total_pr_merges: int + total_commits: int + + +class UserVelocity(BaseModel): + """Per-user breakdown of engineering activity. + + goal_advances counts only FORWARD SMILE phase transitions (see + smile.PHASE_ORDER). A backward "re-evaluation" move is explicitly + allowed by validate_phase_transition() and is real LPI usage, but it + isn't "velocity" in the forward-progress sense this metric is named for. + + last_active is the max timestamp across that user's signals, goal + updates, and phase transitions — so a user who only works through + goals (no GitHub signals yet) still shows up as active. + + streams is the sorted set of distinct `stream` values seen in that + user's signals (e.g. ["lpi", "boardy"]) — a quick "what are they + touching" glance without opening the full signal list. + """ + + signal_count: int + pr_merges: int + commits: int + goal_advances: int + last_active: datetime | None + streams: list[str] + + +class InactiveUserDetail(BaseModel): + """One row of the inactive_users[] breakdown. + + days_inactive is computed as (now - last_seen).days — a whole-day + count, not a precise duration, since "how many days has this person + gone quiet" is the question a team lead actually asks. + """ + + user_id: str + days_inactive: int + last_seen: datetime | None + + +class GoalsSummary(BaseModel): + """Portfolio-wide goal counts, independent of who owns each goal. + + by_phase is zero-filled for all 6 SmilePhase values up front (not just + the phases that happen to have goals right now) so the frontend can + render a consistent 6-bar chart without defensive `.get(key, 0)` calls. + """ + + total_goals: int + by_phase: dict[str, int] + + +class TeamMetrics(BaseModel): + """Full response body for GET /api/v1/metrics/team.""" + + team_summary: TeamSummary + per_user_velocity: dict[str, UserVelocity] + inactive_users: list[InactiveUserDetail] + goals_summary: GoalsSummary diff --git a/src/lpi/routers/metrics.py b/src/lpi/routers/metrics.py new file mode 100644 index 0000000..9930644 --- /dev/null +++ b/src/lpi/routers/metrics.py @@ -0,0 +1,332 @@ +"""Priority 1 — Engineering Velocity & Inactivity Metrics. + +Owner : Adil Islam +QA : Daksh Garg + +WHY THIS ENDPOINT EXISTS +────────────────────────── +Nicolas wants immediate value from existing data — zero 30-day warmup +required. This reads what's already in activity_signals/goals/ +goal_phase_transitions today and turns it into a team-wide velocity + +inactivity view. No new tables, no new ingestion path. + +NEW ENDPOINT +───────────── + GET /api/v1/metrics/team + +DATA SOURCES (all already exist) +─────────────────────────────────── + store.list_signals() → signal_count, pr_merges, commits, + streams[], last_active (signal side) + store.list_goals() → goals_summary, last_active (goal side) + store.list_goal_phase_transitions() → goal_advances, last_active (phase side) + +ADMIN GATING +───────────── +Mirrors GET /api/v1/users/map exactly — this is a cross-user aggregate, so +only admins (settings.admin_user_ids) can read it. Same 403 message, for +consistency with the one other admin-gated endpoint in this codebase. + +ROSTER DEFINITION — read this before changing anything below +───────────────────────────────────────────────────────────────── +"Team" here is NOT pulled from Supabase Auth's user list. Two reasons: + 1. Test JWTs (conftest.py) are self-signed UUIDs that never exist as + real Supabase Auth users — calling client.auth.admin.list_users() + would make active_users always 0 in tests. + 2. Semantically, the inactive_users[] contract requires a `last_seen` — + a registered user who has NEVER done anything doesn't have a + last_seen and isn't "inactive" in the velocity sense, they're just + absent from the data entirely. +So: the roster is the union of every user_id seen in signals, goals, and +goal_phase_transitions. You can only be "inactive" if you were active once. + +PR_MERGED / COMMIT_PUSHED COUNTING — a deliberate gap, not an oversight +────────────────────────────────────────────────────────────────────────── +scripts/ingest_github_events.py filters to ACTUALLY merged PRs before +tagging a signal "pr_merged" (see map_github_event() there: only emits it +when action=="closed" and pr.merged is True). That's the only data source +in this codebase that's provably "merged." + +signals.py::sync_github_events (Aditi's dynamic per-goal sync) ingests RAW +GitHub event types ("PullRequestEvent", "PushEvent") with NO merged-status +filter — an opened-but-not-merged PR counts as a signal there. Counting +those toward total_pr_merges would inflate the number with non-merged PRs. +So this file only counts the canonical, pre-filtered event_type strings. +Raw GitHub event types still count toward signal_count/streams (they ARE +real signals) — just not toward pr_merges/commits. Flag for Aditi: align +sync_github_events' filtering/tagging with the static script if you want +its data to count here too. + +KNOWN UPSTREAM BUG THAT AFFECTS WHAT THIS ENDPOINT WILL SHOW +───────────────────────────────────────────────────────────────── +scripts/ingest_github_events.py's post_signal() sends no Authorization +header (the P0 from the June 28 audit). With auth now enforced on +POST /api/v1/signals/, every run of that script 401s silently. Until that +bearer-token fix lands, total_pr_merges/total_commits from REAL GitHub +activity will read near-zero in the demo — not a bug in this endpoint, +a starvation of its primary upstream data source. Worth a one-line +mention in the demo so nobody reads it as this endpoint being broken. + +PAGINATION TRADE-OFF +────────────────────── +store.list_signals() hard-caps at 200 rows/call. _fetch_all_signals() +below pages through it with offset increments. Fine at current team +scale; capped at _MAX_SIGNAL_PAGES as a safety valve with a system_logs +warning if hit, so nobody's silently missing data once this table grows +past ~10k rows. The real long-term fix is a SQL aggregate (a Postgres +RPC / count() query) instead of pulling every row into Python — out of +scope for a Saturday deadline, noted here for whoever picks it up next. +""" + +from collections import defaultdict +from dataclasses import dataclass, field +from datetime import UTC, datetime + +from fastapi import APIRouter, Depends, HTTPException, Query, status + +from lpi import store +from lpi.middleware.auth import UserContext, get_current_user_context +from lpi.models import ( + GoalsSummary, + InactiveUserDetail, + Signal, + SmilePhase, + TeamMetrics, + TeamSummary, + UserVelocity, +) +from lpi.smile import PHASE_ORDER +from lpi.utils.logging import log_system_event + +router = APIRouter() + +# ── Pagination constants (see module docstring) ─────────────────────────────── +_SIGNALS_PAGE_SIZE = 200 # store.list_signals()'s hard max per call +_MAX_SIGNAL_PAGES = 50 # safety valve — 10,000 signals before we warn + +# ── Event-type sets for pr_merges / commits (see module docstring) ──────────── +# Deliberately narrow: only scripts/ingest_github_events.py emits these, and +# only AFTER filtering to genuinely merged PRs / non-empty pushes. +PR_MERGE_EVENT_TYPES = frozenset({"pr_merged"}) +COMMIT_EVENT_TYPES = frozenset({"commit_pushed"}) + + +@dataclass +class _UserAccumulator: + """Mutable scratch space for one user while we walk signals/goals/ + transitions. Never leaves this module — gets converted into the public + UserVelocity model only after all three passes finish. + """ + + signal_count: int = 0 + pr_merges: int = 0 + commits: int = 0 + goal_advances: int = 0 + last_active: datetime | None = None + streams: set[str] = field(default_factory=set) + + def bump_last_active(self, ts: datetime | None) -> None: + """Keep the latest of (current last_active, ts). No-op if ts is None + (defensive — e.g. a malformed phase_transition row missing + transitioned_at shouldn't crash the whole endpoint). + """ + if ts is None: + return + if self.last_active is None or ts > self.last_active: + self.last_active = ts + + +def _fetch_all_signals() -> list[Signal]: + """Page through store.list_signals() until every signal (any user) is + fetched. user_id=None → no user scoping, same fetch_all pattern goals.py + and signals.py already use for admin views. + + See module docstring for the pagination trade-off this represents. + """ + all_signals: list[Signal] = [] + offset = 0 + + for _ in range(_MAX_SIGNAL_PAGES): + page = store.list_signals(user_id=None, limit=_SIGNALS_PAGE_SIZE, offset=offset) + all_signals.extend(page) + if len(page) < _SIGNALS_PAGE_SIZE: + break # last page was partial — we've got everything + offset += _SIGNALS_PAGE_SIZE + else: + # for/else: only runs if we never hit the `break` above, i.e. every + # single page came back full, all the way up to the cap. That means + # there's likely MORE data we didn't fetch — surface it loudly + # rather than silently undercounting totals. + log_system_event( + event="metrics_team_signal_pagination_capped", + level="warning", + detail=( + f"Hit the {_MAX_SIGNAL_PAGES}-page cap " + f"({_MAX_SIGNAL_PAGES * _SIGNALS_PAGE_SIZE} rows) while fetching " + "all signals for /metrics/team. Totals may be undercounted — " + "replace this with a real SQL aggregate instead of paging " + "through every row in Python." + ), + ) + + return all_signals + + +def _is_forward_transition(from_phase: str | None, to_phase: str | None) -> bool: + """True only if to_phase is strictly later than from_phase in + smile.PHASE_ORDER. Mirrors the forward-step check inside + smile.validate_phase_transition(), but here we don't care whether it + was a *valid* (exactly-one-step) forward move — any forward movement + counts as an "advance". Invalid/unrecognised phase strings return + False rather than raising, since this is a read-only aggregation pass + over an audit log, not request validation. + """ + try: + from_idx = PHASE_ORDER.index(SmilePhase(from_phase)) + to_idx = PHASE_ORDER.index(SmilePhase(to_phase)) + except ValueError: + return False + return to_idx > from_idx + + +@router.get( + "/team", + response_model=TeamMetrics, + summary="Team-wide engineering velocity and inactivity metrics", + description=( + "Admin-only. Aggregates activity_signals, goals, and " + "goal_phase_transitions into a team velocity dashboard: total/" + "per-user signal counts, PR merges, commits, goal advances, and " + "which team members have gone quiet." + ), +) +def get_team_metrics( + inactive_threshold_days: int = Query( + default=3, + ge=0, + le=90, + description=( + "A user counts as inactive once this many days have passed " + "since their last signal/goal update/phase transition. " + "Default 3 — matches the team's daily-log cadence; 3 quiet " + "days is a meaningful gap on a sprint this fast. Tune per " + "demo/sprint as needed." + ), + ), + user_context: UserContext = Depends(get_current_user_context), +) -> TeamMetrics: + """Build the full team metrics snapshot. See module docstring for the + roster definition, pr_merges/commits counting rule, and pagination + trade-off — those decisions live there, not duplicated here. + """ + if not user_context.is_admin: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Admin privileges required", + ) + + # ── Pull every row from all three sources, admin-wide (user_id=None) ── + signals = _fetch_all_signals() + goals = store.list_goals(user_id=None) + transitions = store.list_goal_phase_transitions(user_id=None) + + now = datetime.now(UTC) + + # defaultdict(_UserAccumulator): each missing key gets a FRESH + # _UserAccumulator() (fresh set/None, not a shared mutable default — + # the factory is called once per missing key, not once total). + user_stats: dict[str, _UserAccumulator] = defaultdict(_UserAccumulator) + + # ── Pass 1: signals → signal_count, pr_merges, commits, streams ─────── + for sig in signals: + stats = user_stats[sig.user_id] + stats.signal_count += 1 + stats.streams.add(sig.stream) + if sig.event_type in PR_MERGE_EVENT_TYPES: + stats.pr_merges += 1 + if sig.event_type in COMMIT_EVENT_TYPES: + stats.commits += 1 + stats.bump_last_active(sig.timestamp) + + # ── Pass 2: goals → roster membership + last_active (no signal yet) ─── + for g in goals: + stats = user_stats[g.user_id] # registers the key even at 0 signals + stats.bump_last_active(g.updated_at) + + # ── Pass 3: phase transitions → goal_advances (forward-only) ────────── + for t in transitions: + uid = t.get("user_id") + if not uid: + continue # defensive — shouldn't happen, log_transition always sets it + stats = user_stats[uid] + if _is_forward_transition(t.get("from_phase"), t.get("to_phase")): + stats.goal_advances += 1 + transitioned_at = t.get("transitioned_at") + if transitioned_at: + stats.bump_last_active(datetime.fromisoformat(transitioned_at)) + + # ── Classify active/inactive + build the public response models ────── + per_user_velocity: dict[str, UserVelocity] = {} + inactive_details: list[InactiveUserDetail] = [] + active_count = 0 + + for uid, stats in user_stats.items(): + last_active = stats.last_active + # days_inactive=-1 is an "unknown" sentinel (last_active somehow + # missing) — shouldn't be reachable given the roster construction + # above, but kept defensive rather than crashing on bad data. + days_inactive = (now - last_active).days if last_active is not None else -1 + is_active = last_active is not None and days_inactive < inactive_threshold_days + + per_user_velocity[uid] = UserVelocity( + signal_count=stats.signal_count, + pr_merges=stats.pr_merges, + commits=stats.commits, + goal_advances=stats.goal_advances, + last_active=last_active, + streams=sorted(stats.streams), + ) + + if is_active: + active_count += 1 + else: + inactive_details.append( + InactiveUserDetail( + user_id=uid, + days_inactive=days_inactive, + last_seen=last_active, + ) + ) + + # Most-inactive-first — the dashboard should lead with who to nudge. + inactive_details.sort(key=lambda d: d.days_inactive, reverse=True) + + total_signals = len(signals) + total_pr_merges = sum(s.pr_merges for s in user_stats.values()) + total_commits = sum(s.commits for s in user_stats.values()) + avg_signals_per_active_user = ( + round(total_signals / active_count, 2) if active_count else 0.0 + ) + + team_summary = TeamSummary( + total_signals=total_signals, + active_users=active_count, + inactive_users=len(inactive_details), + avg_signals_per_active_user=avg_signals_per_active_user, + total_pr_merges=total_pr_merges, + total_commits=total_commits, + ) + + # ── goals_summary: zero-fill all 6 phases, then count real goals ────── + by_phase: dict[str, int] = {phase.value: 0 for phase in SmilePhase} + for g in goals: + by_phase[g.smile_phase.value] += 1 + + goals_summary = GoalsSummary(total_goals=len(goals), by_phase=by_phase) + + return TeamMetrics( + team_summary=team_summary, + per_user_velocity=per_user_velocity, + inactive_users=inactive_details, + goals_summary=goals_summary, + ) \ No newline at end of file diff --git a/src/lpi/store.py b/src/lpi/store.py index 5684f37..1f46de1 100644 --- a/src/lpi/store.py +++ b/src/lpi/store.py @@ -368,6 +368,40 @@ def get_user_activity_logs( query = query.eq("action", action) return cast(list[dict], query.execute().data) # ← always reached +def list_goal_phase_transitions(user_id: str | None = None) -> list[dict]: + """Read rows from the goal_phase_transitions Supabase table. + + WHY THIS EXISTS + ───────────────── + Mirrors get_user_activity_logs() above, but for the OTHER audit table + Yashika's logging design specifies: goal_phase_transitions (written by + utils/logging.py::log_transition() on every SMILE phase change). + + Used by GET /api/v1/metrics/team (routers/metrics.py) to compute each + user's `goal_advances` count — see that file for why only FORWARD + transitions count toward "advances". + + No Pydantic model wraps this table (unlike Signal/Goal) because it's + an audit log, not a primary domain entity — raw dicts are fine for an + internal aggregation pass that only reads a few known keys + (user_id, from_phase, to_phase, transitioned_at). + + Args: + user_id : Optional filter to one user's transitions. None (default) + returns every transition for every user — the admin-wide + view the team metrics endpoint needs. + + Returns: + Raw list of matching rows (dicts). No pagination — at current team + scale (a handful of goals, single-digit phase changes per goal) + this table will stay small for a long time. Add limit/offset the + same way list_signals() does if that stops being true. + """ + query = _get_client().table("goal_phase_transitions").select("*") + if user_id: + query = query.eq("user_id", user_id) + return cast(list[dict], query.execute().data) + # ── Recommendation Feedback ───────────────────────────────────────────── def insert_recommendation_feedback( feedback: RecommendationFeedback, @@ -424,7 +458,8 @@ def get_recommendation_feedback( def clear_all() -> None: - """Wipe all data from goals and activity_signals. Call ONLY from tests. + """Wipe all data from goals, activity_signals, and goal_phase_transitions. + Call ONLY from tests. WHY .neq("user_id", "__sentinel_never_exists__")? ─────────────────────────────────────────────────── @@ -439,11 +474,18 @@ def clear_all() -> None: This is called before AND after every test by the autouse fixture in conftest.py, so tests never see each other's data. - NOTE: this does NOT wipe user_activity_logs. If you add a test that - relies on a clean audit-log table between runs (e.g. counting rows - rather than filtering by resource_id), wipe it the same way here. - The current regression test avoids this by filtering on resource_id, - which is unique per signal and doesn't require a clean table. + goal_phase_transitions ADDED (Priority 1 — Team Metrics) + ─────────────────────────────────────────────────────────── + This table previously had no test that read it back in an assertion — + log_transition() writes it, but nothing queried it, so a dirty table + was harmless. store.list_goal_phase_transitions() (added for + GET /api/v1/metrics/team) now reads it directly, and stale rows from + earlier test/manual runs were leaking into goal_advances / last_active + counts, breaking test isolation. Wiping it here closes that gap. + + NOTE: this still does NOT wipe user_activity_logs or system_logs — no + test currently asserts against a clean state for those tables. Add + the same .neq() pattern here if that ever becomes necessary. """ # Wipe all goals rows _get_client().table("goals").delete().neq("user_id", "__sentinel_never_exists__").execute() @@ -452,3 +494,8 @@ def clear_all() -> None: _get_client().table("activity_signals").delete().neq( "user_id", "__sentinel_never_exists__" ).execute() + + # Wipe all goal_phase_transitions rows (Priority 1 — Team Metrics addition) + _get_client().table("goal_phase_transitions").delete().neq( + "user_id", "__sentinel_never_exists__" + ).execute() \ No newline at end of file diff --git a/tests/conftest.py b/tests/conftest.py index 833be9c..fb29a2e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -115,6 +115,21 @@ def unauthenticated_client() -> TestClient: """FastAPI test client without Authorization header.""" return TestClient(app) +@pytest.fixture +def admin_client(monkeypatch: pytest.MonkeyPatch) -> TestClient: + """FastAPI test client whose JWT subject is treated as an admin. + + Reuses TEST_USER_ID's token but flips settings.admin_user_ids so + get_current_user_context() resolves is_admin=True for the duration of + this test only (monkeypatch auto-reverts after the test). Needed for + any endpoint gated like GET /api/v1/users/map or + GET /api/v1/metrics/team. + """ + monkeypatch.setattr(settings, "admin_user_ids", TEST_USER_ID) + test_client = TestClient(app) + test_client.headers.update({"Authorization": f"Bearer {_make_token()}"}) + return test_client + @pytest.fixture def sample_goal() -> dict: diff --git a/tests/test_metrics.py b/tests/test_metrics.py new file mode 100644 index 0000000..2548e83 --- /dev/null +++ b/tests/test_metrics.py @@ -0,0 +1,292 @@ +"""Tests for GET /api/v1/metrics/team — Priority 1 Engineering Velocity & Inactivity. + +Owner : Adil Islam +QA : Daksh Garg + +HOW THESE TESTS WORK +───────────────────── +Same integration-test pattern as test_activity_signals.py / test_goal_crud.py: +real FastAPI endpoints, real local Supabase, autouse `clear_store` fixture +(conftest.py) wipes goals + activity_signals before and after every test. + +ADMIN GATING +───────────── +/metrics/team is admin-only (mirrors GET /api/v1/users/map). The `client` +fixture's TEST_USER_ID is NOT an admin by default, so most tests use the +`admin_client` fixture (conftest.py) instead, which monkeypatches +settings.admin_user_ids for the duration of the test. + +WHY TEST_USER_ID IS RE-DECLARED HERE INSTEAD OF IMPORTED +──────────────────────────────────────────────────────────── +tests/ has no __init__.py, so `from tests.conftest import TEST_USER_ID` +is fragile depending on how pytest's rootdir import-mode resolves things. +Re-declaring the literal here (it must match conftest.py's TEST_USER_ID — +both authenticate via the same fixtures) is simpler and more robust than +relying on a cross-module import. settings.supabase_jwt_secret, by +contrast, IS safe to read live: conftest's autouse `_jwt_secret` fixture +patches it before every test runs, so _token_for() below always signs +with the same secret get_current_user() verifies against. +""" + +import time +import uuid +from datetime import UTC, datetime, timedelta + +import jwt + +from lpi import store +from lpi.config import settings +from lpi.models import Signal +from lpi.routers import metrics as metrics_module + +# Must match conftest.py's TEST_USER_ID — the `client`/`admin_client` +# fixtures always authenticate as this UUID. +TEST_USER_ID = "00000000-0000-0000-0000-000000000001" + + +def _token_for(user_id: str) -> str: + """Sign a test JWT for an arbitrary user_id, reusing the secret + conftest's autouse _jwt_secret fixture already patched into + settings.supabase_jwt_secret. Lets a test simulate a second team + member without needing a real Supabase Auth user record. + """ + payload = {"sub": user_id, "aud": "authenticated", "exp": int(time.time()) + 3600} + return jwt.encode(payload, settings.supabase_jwt_secret, algorithm="HS256") + + +class TestAuthGating: + """/metrics/team must reject non-admins and unauthenticated callers.""" + + def test_requires_auth(self, unauthenticated_client) -> None: + response = unauthenticated_client.get("/api/v1/metrics/team") + assert response.status_code == 401 + + def test_rejects_non_admin(self, client) -> None: + """TEST_USER_ID is a regular (non-admin) caller by default.""" + response = client.get("/api/v1/metrics/team") + assert response.status_code == 403 + + def test_allows_admin(self, admin_client) -> None: + response = admin_client.get("/api/v1/metrics/team") + assert response.status_code == 200 + + +class TestEmptyState: + """On a freshly wiped store, every count should be zero, not missing.""" + + def test_empty_team_summary(self, admin_client) -> None: + response = admin_client.get("/api/v1/metrics/team") + assert response.status_code == 200 + data = response.json() + + assert data["team_summary"] == { + "total_signals": 0, + "active_users": 0, + "inactive_users": 0, + "avg_signals_per_active_user": 0.0, + "total_pr_merges": 0, + "total_commits": 0, + } + assert data["per_user_velocity"] == {} + assert data["inactive_users"] == [] + + def test_goals_summary_zero_fills_all_six_phases(self, admin_client) -> None: + """by_phase must list all 6 SMILE phases at 0, not omit empty ones — + the frontend renders a fixed 6-bar chart and shouldn't need + defensive .get(key, 0) calls. + """ + response = admin_client.get("/api/v1/metrics/team") + by_phase = response.json()["goals_summary"]["by_phase"] + + assert set(by_phase.keys()) == { + "reality-emulation", + "concurrent-engineering", + "collective-intelligence", + "contextual-intelligence", + "continuous-intelligence", + "perpetual-wisdom", + } + assert all(count == 0 for count in by_phase.values()) + assert response.json()["goals_summary"]["total_goals"] == 0 + + +class TestSignalAggregation: + """Core counting logic: pr_merges/commits/signal_count/streams.""" + + def test_counts_pr_merged_and_commit_pushed(self, client, admin_client) -> None: + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "pr_merged", "payload": {}}, + ) + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "commit_pushed", "payload": {}}, + ) + # A non-engineering signal should count toward signal_count/streams + # but NOT toward pr_merges/commits. + client.post( + "/api/v1/signals/", + json={"stream": "boardy", "event_type": "match_created", "payload": {}}, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + + assert data["team_summary"]["total_signals"] == 3 + assert data["team_summary"]["total_pr_merges"] == 1 + assert data["team_summary"]["total_commits"] == 1 + + user_row = data["per_user_velocity"][TEST_USER_ID] + assert user_row["signal_count"] == 3 + assert user_row["pr_merges"] == 1 + assert user_row["commits"] == 1 + assert sorted(user_row["streams"]) == ["boardy", "lpi"] + + def test_unmerged_pr_event_type_not_counted_as_merge(self, client, admin_client) -> None: + """Raw GitHub event types from the dynamic per-goal sync endpoint + (PullRequestEvent/PushEvent) aren't filtered for merged status — + see routers/metrics.py module docstring. Must count toward + signal_count but NOT pr_merges, since an unmerged 'PullRequestEvent' + would otherwise inflate the merge count. + """ + client.post( + "/api/v1/signals/", + json={"stream": "github", "event_type": "PullRequestEvent", "payload": {}}, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + assert data["team_summary"]["total_signals"] == 1 + assert data["team_summary"]["total_pr_merges"] == 0 + + def test_aggregates_across_multiple_users(self, client, admin_client) -> None: + other_user_id = str(uuid.uuid4()) + other_headers = {"Authorization": f"Bearer {_token_for(other_user_id)}"} + + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "commit_pushed", "payload": {}}, + ) + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "commit_pushed", "payload": {}}, + headers=other_headers, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + + assert data["team_summary"]["total_signals"] == 2 + assert TEST_USER_ID in data["per_user_velocity"] + assert other_user_id in data["per_user_velocity"] + assert data["per_user_velocity"][other_user_id]["commits"] == 1 + + +class TestActiveInactiveClassification: + """A user freshly active right now must count as active; a user whose + last signal is 10 days old must show up in inactive_users with a + matching days_inactive/last_seen. + """ + + def test_fresh_signal_counts_as_active(self, client, admin_client) -> None: + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "commit_pushed", "payload": {}}, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + assert data["team_summary"]["active_users"] == 1 + assert data["team_summary"]["inactive_users"] == 0 + assert data["inactive_users"] == [] + + def test_zero_day_threshold_makes_everyone_inactive(self, client, admin_client) -> None: + """Boundary check on the strict `<` comparison: with + inactive_threshold_days=0, even a signal from this instant doesn't + satisfy days_inactive < 0, so the user is classified inactive. + """ + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": "commit_pushed", "payload": {}}, + ) + + data = admin_client.get("/api/v1/metrics/team?inactive_threshold_days=0").json() + + assert data["team_summary"]["active_users"] == 0 + assert data["team_summary"]["inactive_users"] == 1 + assert data["inactive_users"][0]["user_id"] == TEST_USER_ID + + def test_old_signal_flagged_inactive_with_default_threshold(self, admin_client) -> None: + """Seed a 10-day-old signal directly through store.insert_signal() + (bypassing the API, which always stamps `now`) to simulate a team + member who's gone quiet — then verify the default 3-day threshold + correctly flags them. + """ + old_timestamp = datetime.now(UTC) - timedelta(days=10) + store.insert_signal( + Signal( + id=str(uuid.uuid4()), + user_id=TEST_USER_ID, + stream="lpi", + event_type="commit_pushed", + payload={}, + source="api", + timestamp=old_timestamp, + ) + ) + + data = admin_client.get("/api/v1/metrics/team").json() + + assert data["team_summary"]["active_users"] == 0 + assert data["team_summary"]["inactive_users"] == 1 + + detail = data["inactive_users"][0] + assert detail["user_id"] == TEST_USER_ID + assert detail["days_inactive"] >= 10 + + +class TestGoalAdvances: + """goal_advances must count only FORWARD SMILE phase transitions — + a backward re-evaluation move is valid LPI usage but isn't 'velocity'. + """ + + def test_forward_transition_counts_backward_does_not(self, client, admin_client) -> None: + goal_id = client.post( + "/api/v1/goals/", + json={ + "title": "Ship Phase 4", + "priority": 8, + "smile_phase": "reality-emulation", + }, + ).json()["id"] + + # Forward: reality-emulation -> concurrent-engineering (counts) + client.patch( + f"/api/v1/goals/{goal_id}", + json={"smile_phase": "concurrent-engineering"}, + ) + # Backward: concurrent-engineering -> reality-emulation (re-evaluation, + # explicitly allowed by SMILE — must NOT count as an "advance") + client.patch( + f"/api/v1/goals/{goal_id}", + json={"smile_phase": "reality-emulation"}, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + assert data["per_user_velocity"][TEST_USER_ID]["goal_advances"] == 1 + + +class TestPaginationBoundary: + """_fetch_all_signals() must page past a single batch instead of + silently truncating at the per-call limit. Rather than inserting + hundreds of real rows, shrink the page size to 3 and insert 7 signals — + proves the offset-increment loop actually crosses a page boundary. + """ + + def test_pages_past_a_single_batch(self, client, admin_client, monkeypatch) -> None: + monkeypatch.setattr(metrics_module, "_SIGNALS_PAGE_SIZE", 3) + + for i in range(7): + client.post( + "/api/v1/signals/", + json={"stream": "lpi", "event_type": f"event_{i}", "payload": {}}, + ) + + data = admin_client.get("/api/v1/metrics/team").json() + assert data["team_summary"]["total_signals"] == 7 \ No newline at end of file From 32e9dbda7ffd880d972ed8dcf5d7f28b1895cac2 Mon Sep 17 00:00:00 2001 From: Adilislam0 Date: Sun, 28 Jun 2026 18:16:27 +0530 Subject: [PATCH 2/4] Update main.py --- src/lpi/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lpi/main.py b/src/lpi/main.py index 21c1892..eb5044b 100644 --- a/src/lpi/main.py +++ b/src/lpi/main.py @@ -16,6 +16,7 @@ from lpi.middleware import register_middleware from lpi.routers import github_auth, goals, me, metrics, recommendations, signals, users, webhooks + @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncIterator[None]: """Startup / shutdown hook. No-op in Phase 2. From 79c4f0541e7d4de2e2efef8b98d25df06eaacf06 Mon Sep 17 00:00:00 2001 From: Adilislam0 Date: Sun, 28 Jun 2026 18:43:38 +0530 Subject: [PATCH 3/4] Update metrics.py --- src/lpi/routers/metrics.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lpi/routers/metrics.py b/src/lpi/routers/metrics.py index 9930644..05e9e4b 100644 --- a/src/lpi/routers/metrics.py +++ b/src/lpi/routers/metrics.py @@ -181,6 +181,8 @@ def _is_forward_transition(from_phase: str | None, to_phase: str | None) -> bool False rather than raising, since this is a read-only aggregation pass over an audit log, not request validation. """ + if from_phase is None or to_phase is None: + return False try: from_idx = PHASE_ORDER.index(SmilePhase(from_phase)) to_idx = PHASE_ORDER.index(SmilePhase(to_phase)) @@ -329,4 +331,4 @@ def get_team_metrics( per_user_velocity=per_user_velocity, inactive_users=inactive_details, goals_summary=goals_summary, - ) \ No newline at end of file + ) From 87579ac36c424c0dfdac7e7cace357d830768a28 Mon Sep 17 00:00:00 2001 From: Adilislam0 Date: Mon, 29 Jun 2026 10:45:53 +0530 Subject: [PATCH 4/4] updated via review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks for the thorough review! 1. **Scalability** — agreed, filed #[ticket] for the SQL-aggregate migration. Not urgent at current scale (~7 users), but tracked. 2. **Roster edge case** — confirmed intentional, not an oversight: there's no source of "all team members" independent of activity without either breaking test isolation (Supabase Auth admin list) or hardcoding a roster. Flagging for Nicolas will add a static team-roster config if product wants zero-activity users tracked. 3. **Timezone** — checked the schema directly: transitioned_at is TIMESTAMPTZ and PostgREST always returns it offset-aware, so this specific crash can't fire today. Added defensive UTC-coercion + 2 unit tests anyway since it's free insurance against future drift. Pushed in [commit hash]. --- src/lpi/routers/metrics.py | 37 ++++++++++++++++++++++++++++++++++--- tests/test_metrics.py | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/lpi/routers/metrics.py b/src/lpi/routers/metrics.py index 05e9e4b..6e77845 100644 --- a/src/lpi/routers/metrics.py +++ b/src/lpi/routers/metrics.py @@ -202,6 +202,38 @@ def _is_forward_transition(from_phase: str | None, to_phase: str | None) -> bool "which team members have gone quiet." ), ) +def _parse_utc_timestamp(value: str | None) -> datetime | None: + """Parse an ISO8601 timestamp string from a raw Supabase row into a + timezone-AWARE datetime — never naive. + + WHY THIS EXISTS (PR review — Daksh) + ───────────────────────────────────── + utils/logging.py::log_transition() always writes transitioned_at via + datetime.now(UTC).isoformat(), and the column is TIMESTAMPTZ (see + supabase/migrations/20260605000000_create_goal_phase_transitions.sql) — + PostgREST always serialises timestamptz columns back out with an + explicit UTC offset, so in practice this string is never naive today. + + Still, Pass 3 below is the one place in this file doing our own + fromisoformat() on a raw string (signals/goals arrive as already- + validated Signal/Goal Pydantic objects — this table has no model layer + in between). Relying on an unenforced assumption about a third-party + library's serialisation format is fragile: if it were ever violated, + mixing a naive datetime into the same `>` comparison as the timezone- + aware signal/goal timestamps in bump_last_active() raises + `TypeError: can't compare offset-naive and offset-aware datetimes` and + 500s this endpoint. This makes the "treat as UTC" assumption explicit + and crash-proof instead of implicit and silent. + """ + if not value: + return None + parsed = datetime.fromisoformat(value) + if parsed.tzinfo is None: + # No offset in the string — the only convention this codebase + # writes timestamps in is UTC (datetime.now(UTC) everywhere), so + # that's the safe assumption rather than guessing local time. + parsed = parsed.replace(tzinfo=UTC) + return parsed def get_team_metrics( inactive_threshold_days: int = Query( default=3, @@ -263,9 +295,8 @@ def get_team_metrics( stats = user_stats[uid] if _is_forward_transition(t.get("from_phase"), t.get("to_phase")): stats.goal_advances += 1 - transitioned_at = t.get("transitioned_at") - if transitioned_at: - stats.bump_last_active(datetime.fromisoformat(transitioned_at)) + stats.bump_last_active(_parse_utc_timestamp(t.get("transitioned_at"))) + # ── Classify active/inactive + build the public response models ────── per_user_velocity: dict[str, UserVelocity] = {} diff --git a/tests/test_metrics.py b/tests/test_metrics.py index 2548e83..5559a2d 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -289,4 +289,40 @@ def test_pages_past_a_single_batch(self, client, admin_client, monkeypatch) -> N ) data = admin_client.get("/api/v1/metrics/team").json() - assert data["team_summary"]["total_signals"] == 7 \ No newline at end of file + assert data["team_summary"]["total_signals"] == 7 + +class TestParseUtcTimestamp: + """Direct unit tests for metrics.py::_parse_utc_timestamp() — added in + response to PR review point 3 (Daksh). + + NOTE: a real Supabase round-trip can't actually produce a naive string + here — transitioned_at is TIMESTAMPTZ and PostgREST always serialises + timestamptz columns with an explicit offset on read (verified against + supabase/migrations/20260605000000_create_goal_phase_transitions.sql). + That's why this is a direct unit test of the helper, not an + integration test through the API — an integration test would pass + even without the fix and wouldn't prove anything. + """ + + def test_returns_none_for_empty_input(self) -> None: + assert metrics_module._parse_utc_timestamp(None) is None + assert metrics_module._parse_utc_timestamp("") is None + + def test_passes_through_an_already_aware_string(self) -> None: + result = metrics_module._parse_utc_timestamp("2026-06-20T10:00:00+00:00") + assert result is not None + assert result.tzinfo is not None + + def test_coerces_a_naive_string_to_utc_instead_of_crashing(self) -> None: + """The exact scenario Daksh's review flagged: a transitioned_at + string with no offset must not raise when later compared against + a timezone-aware datetime (e.g. a Signal.timestamp) elsewhere in + the aggregation pass. + """ + result = metrics_module._parse_utc_timestamp("2026-06-20T10:00:00") + assert result is not None + assert result.tzinfo is not None # would be None pre-fix — that's the bug + + # Confirm the comparison that would TypeError pre-fix now works + aware_now = datetime.now(UTC) + assert result < aware_now \ No newline at end of file