feat(usage): add task filtering and attribution - #3019
Conversation
![helixml-bot[bot]](https://avatars.githubusercontent.com/u/149581110?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(usage): add task filtering and attribution
Well-structured feature adding task-level filtering to Usage and persisting proxy session/app/runtime attribution. The design doc is clear and the SQL expression hierarchy (snapshot → interaction → spec-task fallback) is sound. The test coverage is thorough, particularly the Postgres integration tests for task and runtime attribution.
Issues
| # | Severity | File | Description |
|---|---|---|---|
| 1 | Medium | proxy_usage_attribution_test.go:33 |
Test assertion bug — mock session has no CodeAgentRuntime set but test asserts CodeAgentRuntimeOpenCode |
| 2 | Medium | proxy_usage_attribution.go:26-28 |
Hard failure on session lookup blocks the entire proxy request for an attribution concern |
| 3 | Low | store_usage_metrics.go |
orgUsageBaseQuery now unconditionally joins spec_tasks and sessions — overhead for queries that don't need them |
| 4 | Low | OrgUsage.tsx:1106 |
Grid changed from 6→4 columns at xl breakpoint — verify layout doesn't regress for wide screens |
Positive observations
- The snapshot-first, fallback-second approach in
usageSessionIDExpr/usageAppIDExprcorrectly handles both new rows (with snapshots) and legacy rows (without) — good incremental migration. resolveProxyUsageAttributioncross-checks org, task, and app between the user token and session, preventing attribution drift.- Compute task filtering through
computeTaskIDExpr(spec-task or trigger session) correctly unifies both task types. - The deduplicated
usageSessionsJoineliminates the previous ad-hoc session join ingetOrgUsageAgentRuntimeTimeSeries.
Verdict: COMMENT (no blocking issues)
The test assertion bug (#1) will cause TestResolveProxyUsageAttributionUsesCurrentSessionApp to fail — the mock session metadata needs CodeAgentRuntime: types.CodeAgentRuntimeOpenCode to match the assertion. The hard-failure concern (#2) is a design question worth discussing but not blocking. Good work overall.
| require.NoError(t, err) | ||
| require.Equal(t, "ses_123", attribution.SessionID) | ||
| require.Equal(t, "app_current", attribution.AppID) | ||
| require.Equal(t, types.CodeAgentRuntimeOpenCode, attribution.CodeAgentRuntime) |
There was a problem hiding this comment.
Bug: the mock session at line 24 has Metadata: types.SessionMetadata{SpecTaskID: "spt_123"} with no CodeAgentRuntime set (zero value = empty string), but this assertion expects CodeAgentRuntimeOpenCode. This test will fail.
Fix: add CodeAgentRuntime: types.CodeAgentRuntimeOpenCode to the mock session's Metadata, or change this assertion to expect the zero value.
| session, err := s.Store.GetSession(ctx, user.SessionID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load API key session %q: %w", user.SessionID, err) | ||
| } |
There was a problem hiding this comment.
If the session doesn't exist (e.g. deleted, stale key), this returns an error that becomes a 500 in both anthropicAPIProxyHandler and createChatCompletion. For an attribution-only concern, consider logging and falling back to defaultSessionID so a missing session doesn't block inference. The cross-validation below (org/task/app mismatch) should still hard-fail since those indicate a security issue.
| Model(&types.UsageMetric{}). | ||
| Joins(usageInteractionsJoin). | ||
| Joins(usageSpecTasksJoin). | ||
| Joins(usageSessionsJoin). |
There was a problem hiding this comment.
Unlike GetAggregatedUsageMetrics (which conditionally adds joins only when session/app/task filters are active), orgUsageBaseQuery now unconditionally joins spec_tasks and sessions on every call. This adds two LEFT JOINs to all org usage queries regardless of filter state. Consider making these conditional, matching the pattern in GetAggregatedUsageMetrics.
There was a problem hiding this comment.
Incremental review (head 125e9670)
This commit fixes the test assertion bug flagged in the previous review: the mock session now sets CodeAgentRuntime: types.CodeAgentRuntimeOpenCode, matching what the assertion expects. Fix is correct and minimal.
The two lower-severity observations from the prior review (hard-failure on session lookup in the proxy path; unconditional JOINs in orgUsageBaseQuery) remain open but are not blocking — they are design-level considerations for a draft PR.
Verdict: Approve — the blocking issue is resolved.
There was a problem hiding this comment.
Incremental review of new commit 18860ef — clean follow-up.
SQL ordering — taskCreatedExpr uses COALESCE(trigger_configurations.created, usage_spec_tasks.created_at, MAX(usage_metrics.created)) DESC to sort newest-first. GROUP BY correctly includes the non-aggregated timestamp columns. The MAX() fallback for rows with no task/trigger metadata is sound.
initialUsageParam route-params-first — Good solution for the internal navigation timing race. When orgNavigate("usage", {}, query) lands on the Usage page, router params are available immediately while URL search params may lag behind the state synchronizer. Preferring route params eliminates the window where the task filter could be lost.
Utility extraction — usageDateRange.ts extracts toDateInput/rangeFrom from OrgUsage.tsx into a shared module. Tests cover the inclusive range arithmetic and the param-priority logic.
No correctness, security, or performance issues.
1 participant
Summary
Verification
go build ./pkg/server ./pkg/store ./pkg/typesyarn test OrgUsage.test.tsxyarn buildspt_01kzx26k7nnwwrpjpxjvh4a2fbnarrows to Agent OpenCode, harness opencode, one session, and one sandbox; Clear restores the org report