Phase 3 fixes (time-range filter, audit logging) + Phase 4 Wave 1: recommendations endpoint - #31
Merged
Merged
Conversation
…commendations endpoint
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the remaining Phase 3 gate gap and ships Phase 4 Wave 1 — a real,
auth-protected
GET /recommendations/{user_id}endpoint backed by mockdata, so Jahanvi can build frontend cards against a stable contract
before Jaivardhan's reasoning core lands.
What's in this PR
Phase 3 — closing the gate gap
GET /signals/time-range filter — addedstart/endquery params,wired through
store.list_signals()via.gte()/.lte()ontimestamp.This was the single largest gap between the gate sheet ("Timeline
queryable by user and time range") and the running code.
log_user_activity()was failing silently onsignal_ingestedbecause theuser_activity_logsCHECK constraint onlyallowed
goal_created/goal_updated/goal_deleted. Migration20260615000000_signals_rls_and_log_action.sqladdssignal_ingestedtothe constraint — run
supabase db pushbefore testing this branch.logging.py:print()→logger.exception()forlog_user_activityfailures, so a future constraint mismatch surfaces in real log output
instead of silently disappearing into stdout.
store.get_user_activity_logs()added — queries Supabase directly sotests can assert the audit row actually exists in the DB, not just in
the in-memory mirror (which appends unconditionally and would pass even
if the real insert failed).
try/exceptaroundlog_user_activity()insignals.py— that function already never raises internally.Phase 4 Wave 1 — recommendations endpoint (mine)
GET /api/v1/recommendations/{user_id}now requires a valid SupabaseJWT (previously had no auth dependency at all).
limit(default 3, max 10) mockRecommendationobjects,sorted by priority descending, on the same 0.80–7.00 priority scale
goals use — so the frontend reuses one priority-badge component.
_build_mock_recommendations()is a clean swap point: Jaivardhanreplaces this one call in Wave 2 with real goal/signal reasoning; the
response contract (and every test in
test_recommendations_endpoint.py)stays unchanged.
user_idis a path param, not derived from the JWT, by design — oneauthenticated demo session needs to pull up all 3 seeded intern profiles
for the demo. Documented in the module docstring; flagged as the thing
to revisit if this becomes real multi-tenant.
Tests
tests/test_recommendations_endpoint.py(new) — covers auth, responseshape, sort order, and
limitvalidation against Wave 1's mock data.Deliberately does not assert on which actions come back — that's
Jaivardhan's correctness tests in
test_recommendations.py.Also included (test scaffolding, not for review depth)
scripts/ingest_bpi2013.py,scripts/test_endpoints.py— local-onlyingestion/smoke scripts used to manually validate the signals endpoint
against BPI 2013 and synthetic data. Not part of the app; happy to move
these to a
scripts/local-only/folder or drop them from this PR ifpreferred.
lpi_testing_guide.md— local testing reference doc, same call as above.How to test
Manually via Swagger (
/docs):GET /api/v1/signals/?start=2026-06-13T00:00:00Z&end=2026-06-20T00:00:00ZGET /api/v1/recommendations/intern-a-demo-profile→ 3 mock cards, sorted by priorityGET /api/v1/recommendations/intern-a-demo-profile?limit=1→ top-priority card only/api/v1/recommendations/{user_id}with noAuthorizationheader → 401Known follow-ups (not in scope here)
log_transition()andlog_system_event()inlogging.pystill useprint()— same fix aslog_user_activity(), left for a separate pass.ingest_github_events.py) still missing itsAuthorizationheader — will 401 on any auth-enforced deployment.Reviewers
@jaivardhan — recommendations contract, swap-in point for Wave 2
@Daksh —
_build_mock_recommendations()as orchestration fallback@yashika —
user_activity_logsschema/migrationType
Checklist
make lintexits 0make testexits 0 (no new failures)reports/.envsecrets committedstaging(nevermaindirectly)