Skip to content

api: attribute activity rows to api_key.created_by for per-user WAU#54

Merged
meAmitPatil merged 2 commits into
mainfrom
amit/activity-actor-attribution
May 12, 2026
Merged

api: attribute activity rows to api_key.created_by for per-user WAU#54
meAmitPatil merged 2 commits into
mainfrom
amit/activity-actor-attribution

Conversation

@meAmitPatil
Copy link
Copy Markdown
Contributor

@meAmitPatil meAmitPatil commented May 12, 2026

activity.actor_id was always NULL. Middleware now reads api_key.created_by and threads it into both activity log helpers so each row gets the profile id of the api-key creator. System events (reaper, build_completed) stay NULL by design.


Open in Stage

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@stage-review
Copy link
Copy Markdown

stage-review Bot commented May 12, 2026

Ready to review this PR? Stage has broken it down into 4 individual chapters for you:

Title
1 Extract actor ID from API keys in middleware
2 Update activity logging helpers to support actor attribution
3 Wire actor ID through sandbox and template handlers
4 Verify actor attribution with integration tests
Open in Stage

Chapters generated by Stage for commit ee68d68 on May 12, 2026 9:24pm UTC.

@pavitrabhalla
Copy link
Copy Markdown
Contributor

Review

What's Good

  • Correct NULL design for system events. The reaper's timeout_paused call explicitly passes nil, and build_supervisor.go's logBuildCompleted has no ActorID field — both intentional and matching the PR description.
  • actorUUID helper cleanly centralizes the *uuid.UUID → pgtype.UUID conversion and handles nil safely.
  • actorIDFromContext is defensive — double type-assertion guards against a bad cast without panicking.
  • The middleware change is minimal — one extra column in an existing query, stored only when Valid.

Issues & Suggestions

1. Missing DB migration confirmation

The query SELECT id, team_id, created_by FROM api_key requires the column to exist. created_by is defined in 20260403000003_api_key.sql, so existing deployments should be fine — but worth confirming there's no environment where that migration hasn't run yet (e.g. a local dev schema).

2. pgtype import in middleware.go

Minor: pgtype is now a dependency of middleware just for the Scan target. An alternative is to scan into a nullable *string or *[16]byte and convert only if non-nil — keeps middleware decoupled from pgtype. Not a blocker.

3. &id in actorIDFromContext looks like a dangling-pointer trap

id, ok := raw.(uuid.UUID)
// ...
return &id

This is correct Go — id escapes to heap — but a future reader may flag it as suspicious. A short comment would help.

4. No test coverage for NULL created_by

If api_key.created_by IS NULL, createdBy.Valid is false, actor_id is not set in the gin context, and actorIDFromContext returns nil. This is the right behavior, but there are no tests to codify it. Worth adding.


Overall this is a solid, focused change. Main things before merging: confirm the migration has run everywhere, add a note on &id, and consider a test for the NULL path.

@meAmitPatil meAmitPatil merged commit ce1fdd1 into main May 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants