feat: capture additional git information when creating a new version/deployment#3830
feat: capture additional git information when creating a new version/deployment#3830
Conversation
mcstepp
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 42 files out of 149 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds storage, validation, normalization, and retrieval of six Git commit metadata fields (message, author name/email/username/avatar URL, timestamp ms). CreateVersion validates/limits inputs and persists them; GetVersion populates proto fields; DB schema and bulk insert updated; extensive unit tests added; many sqlc headers bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Ctrl as Ctrl Deployment Service
participant DB
participant WF as Workflow Engine
rect rgb(245,248,255)
note over Client,Ctrl: CreateVersion
Client->>Ctrl: CreateVersion(req with git metadata)
Ctrl->>Ctrl: validate/normalize git fields (trim, lower email, rune limits, timestamp ms & ≤ now+1h)
Ctrl->>DB: InsertDeployment(...git_* fields..., status=pending)
DB-->>Ctrl: deployment_id
Ctrl->>WF: StartDeploymentWorkflow(deployment_id)
alt workflow start fails
WF-->>Ctrl: error (logged, non-fatal)
else success
WF-->>Ctrl: ok
end
Ctrl-->>Client: Version{Status=pending, VersionId=deployment_id}
end
rect rgb(245,255,245)
note over Client,Ctrl: GetVersion
Client->>Ctrl: GetVersion(version_id)
Ctrl->>DB: FindDeploymentById(version_id)
DB-->>Ctrl: Deployment (incl. git_* fields, rootfs_image_id)
Ctrl->>DB: FindLatestBuildByDeploymentId(version_id)
DB-->>Ctrl: Build (optional)
Ctrl-->>Client: Version{git_* fields, RootfsImageId, BuildId?}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 53
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
go/pkg/db/key_delete_by_id.sql_generated.go (1)
1-10: Ensure all SQLC‐generated files are on v1.29.0It looks like three generated files were missed in the bump and still carry v1.27.0, which will lead to CI drift unless they’re regenerated:
- go/pkg/db/key_update_credits_decrement.sql_generated.go
- go/pkg/db/key_update_credits_increment.sql_generated.go
- go/pkg/db/key_update_credits_set.sql_generated.go
→ Regenerate these with sqlc v1.29.0 so every
*.sql_generated.gois consistent.Additionally, I spotted two
DBTXinterface definitions (one ingo/pkg/db/interface.go, one ingo/pkg/hydra/store/db.go). If the Hydra‐specific one is outdated or unneeded, please reconcile or remove it.Good news: the
DeleteKeyByID(ctx, db DBTX, id string) errorsignature hasn’t changed, and all call sites (e.g. inquerier_generated.goand your HTTP handler) will continue to work unchanged.go/pkg/db/key_role_delete_many_by_key_and_role_ids.sql_generated.go (1)
18-21: Nit: initialism casing (“IDs”) for consistencyIf you ever customize sqlc naming, consider
RoleIDsto follow Go initialism conventions. Not a blocker, and acceptable for generated code as-is.go/pkg/db/api_insert.sql_generated.go (1)
1-78: Regenerate outdated SQLC files and pin SQLC version in configThree generated files remain at sqlc v1.27.0 and must be regenerated with v1.29.0:
- go/pkg/db/key_update_credits_increment.sql_generated.go (// sqlc v1.27.0)
- go/pkg/db/key_update_credits_set.sql_generated.go (// sqlc v1.27.0)
- go/pkg/db/key_update_credits_decrement.sql_generated.go (// sqlc v1.27.0)
Your sqlc.json configs currently lack any explicit SQLC version pinning. To prevent drift, add a version constraint—either by:
- Specifying the SQLC CLI version in a lockfile or toolchain file (e.g.,
sqlc.version: "1.29.0"),- Or pinning the SQLC version in your CI/development workflow (e.g., install
sqlc@v1.29.0via npm orbrew install sqlc@1.29.0).Finally, the migrations timestamp check didn’t run because no
migrations/ordb/directories were found. Ensure your migrations path is correct (or adjust the grep patterns) so you can verify that all timestamp columns (e.g.,created_at_m,updated_at_m,git_commit_timestamp) follow the intended naming and units conventions.go/pkg/db/role_find_many_by_id_or_name_with_perms.sql_generated.go (1)
14-35: Consider ordering permissions inside JSON_ARRAYAGG for deterministic output.
The subselect feeding JSON_ARRAYAGG lacks ORDER BY, so response order can vary. If clients diff or cache by value, this can cause noise. Adjust the source SQL (not this generated file) to include an ORDER BY (e.g., by permission.name or permission.id).Example source SQL change (apply in go/pkg/db/queries/role_find_many_by_id_or_name_with_perms.sql):
- FROM (SELECT name, id, slug, description - FROM roles_permissions rp - JOIN permissions p ON p.id = rp.permission_id - WHERE rp.role_id = r.id) as permission), + FROM (SELECT name, id, slug, description + FROM roles_permissions rp + JOIN permissions p ON p.id = rp.permission_id + WHERE rp.role_id = r.id + ORDER BY p.name ASC, p.id ASC) as permission),go/pkg/db/ratelimit_override_update.sql_generated.go (1)
13-21: Consider naming bind parameters in source SQL to improve generated param names.The generated param struct uses Windowlimit/Now due to anonymous “?” placeholders and a reserved word column. To improve readability and future diffs, annotate placeholders with names in the SQL source and re-generate. Example:
-UPDATE `ratelimit_overrides` -SET - `limit` = ?, - duration = ?, - async = ?, - updated_at_m= ? -WHERE id = ? +UPDATE `ratelimit_overrides` +SET + `limit` = /*limit*/?, + duration = /*duration*/?, + async = /*async*/?, + updated_at_m = /*now*/? +WHERE id = /*id*/?This keeps the generated struct fields intuitive (Limit, Duration, Async, Now, ID) and avoids awkward names like Windowlimit.
Also applies to: 23-31
go/pkg/db/key_list_by_key_auth_id.sql_generated.go (1)
15-25: Duplicated column alias ‘identity_id’ (from k.identity_id and i.id) is confusingThe SELECT projects k.identity_id and i.id AS identity_id. While the scan order is correct, the duplicated alias reduces readability and increases maintenance risk. Consider aliasing the joined identity id distinctly in the source SQL (not the generated file), then regenerate.
Example change in go/pkg/db/queries/key_list_by_key_auth_id.sql (then re-run sqlc):
- i.id as identity_id, + i.id as joined_identity_id,Callers can then reference the generated field (e.g., JoinedIdentityID) unambiguously after regeneration.
go/pkg/db/ratelimit_delete_many_by_identity_id.sql_generated.go (1)
20-22: Ensure callers never pass a NULL identity_idThe function accepts
sql.NullString. IfValid=false, the statement becomesWHERE identity_id = NULL, which matches no rows (likely a silent no-op). If deletion-by-null is never intended, enforce a non-null string at call sites or in the source SQL signature, and consider assertingValidin the service layer.go/pkg/db/identity_find.sql_generated.go (1)
12-18: Confirm index coverage for WHERE clauseQuery filters by
(workspace_id, external_id OR id, deleted). Ensure composite indexes exist to keep this hot path fast at scale (e.g.,(workspace_id, external_id, deleted)and(workspace_id, id, deleted)) if not already present. Not a blocker for this PR, but worth verifying.go/pkg/db/permission_list_by_role_id.sql_generated.go (1)
12-18: Ordering by slug: confirm desired collationOrdering by
p.slugis deterministic. If slugs can differ only by case or locale, confirm DB collation matches desired ordering (case-insensitive vs sensitive). Non-blocking.go/pkg/db/role_find_by_name_and_workspace_id.sql_generated.go (1)
12-18: Uniqueness assumptions and LIMIT 1Using
LIMIT 1implies(workspace_id, name)should be unique. If not enforced already, consider a unique index to prevent duplicate roles and guarantee deterministic selection.go/pkg/db/key_update_credits_refill.sql_generated.go (1)
26-28: Idempotency note: safe to retry; keep contrast with decrement opsThis UPDATE sets columns to explicit values (idempotent), so wrapping with a generic retry helper is safe. Contrast with decrement-style updates flagged in prior learnings (double-charge risk). Keep this distinction documented near call sites.
go/pkg/db/ratelimit_override_insert.sql_generated.go (1)
41-50: Minor: struct tags vs column namesThe db tags use created_at/updated_at while columns are created_at_m/updated_at_m. It’s harmless for Exec order, but can confuse readers and tools that rely on tags. If desired, align via sqlc config or column aliases in SQL.
go/pkg/db/ratelimit_namespace_insert.sql_generated.go (1)
12-31: Insert defaults: explicit NULLs OK; consider server defaultsExplicit NULLs for updated_at_m and deleted_at_m are fine. If the schema supports DEFAULT NULL already, you could omit those columns for slightly cleaner SQL in the source file; otherwise, no action.
go/pkg/db/ratelimit_delete_many_by_ids.sql_generated.go (1)
23-31: Large IN lists: consider chunking to avoid driver limitsDynamic placeholder expansion is correct and safe. For very large id slices, some drivers/DBs hit parameter or packet limits. Provide a non-generated wrapper that chunks into batches (e.g., 500–1000 ids) and calls this method per chunk.
For example, add a helper outside generated code:
// in go/pkg/db/ratelimit_helpers.go (non-generated) func (q *Queries) DeleteRatelimitsByIDsChunked(ctx context.Context, db DBTX, ids []string, chunk int) error { for i := 0; i < len(ids); i += chunk { end := i + chunk if end > len(ids) { end = len(ids) } if err := q.DeleteManyRatelimitsByIDs(ctx, db, ids[i:end]); err != nil { return err } } return nil }go/apps/ctrl/services/deployment/get_version.go (1)
15-19: Differentiate NotFound from Internal errors.Returning CodeNotFound for any DB error will mask real failures. Use the db.IsNotFound helper to set the correct Connect error code.
- deployment, err := db.Query.FindDeploymentById(ctx, s.db.RO(), req.Msg.GetVersionId()) - if err != nil { - return nil, connect.NewError(connect.CodeNotFound, err) - } + deployment, err := db.Query.FindDeploymentById(ctx, s.db.RO(), req.Msg.GetVersionId()) + if err != nil { + if db.IsNotFound(err) { + return nil, connect.NewError(connect.CodeNotFound, err) + } + return nil, connect.NewError(connect.CodeInternal, err) + }
♻️ Duplicate comments (1)
go/pkg/db/keyring_insert.sql_generated.go (1)
3-3: sqlc v1.29.0 header bump confirmed. Insert SQL unchanged.Note: The hardcoded zero values for size_approx and size_last_updated_at remain intentional per prior guidance; no change required.
Run the consistency check script to ensure all generated DB files were regenerated with v1.29.0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (11)
go/gen/proto/ctrl/v1/build.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/openapi.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/routing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/ctrl/v1/version.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/assetmanagerd/v1/asset.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/billaged/v1/billing.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/deploy/builderd/v1/builder.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/metal/vmprovisioner/v1/vmprovisioner.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/object.pb.gois excluded by!**/*.pb.go,!**/gen/**go/gen/proto/vault/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (107)
go/apps/ctrl/services/deployment/create_version.go(1 hunks)go/apps/ctrl/services/deployment/create_version_simple_test.go(1 hunks)go/apps/ctrl/services/deployment/get_version.go(2 hunks)go/pkg/db/api_find_by_id.sql_generated.go(1 hunks)go/pkg/db/api_insert.sql_generated.go(1 hunks)go/pkg/db/api_soft_delete.sql_generated.go(1 hunks)go/pkg/db/api_update_delete_protection.sql_generated.go(1 hunks)go/pkg/db/audit_log_find_target_by_id.sql_generated.go(1 hunks)go/pkg/db/audit_log_insert.sql_generated.go(1 hunks)go/pkg/db/audit_log_target_insert.sql_generated.go(1 hunks)go/pkg/db/build_find_by_id.sql_generated.go(1 hunks)go/pkg/db/build_find_latest_by_deployment_id.sql_generated.go(1 hunks)go/pkg/db/build_insert.sql_generated.go(1 hunks)go/pkg/db/build_update_failed.sql_generated.go(1 hunks)go/pkg/db/build_update_status.sql_generated.go(1 hunks)go/pkg/db/build_update_succeeded.sql_generated.go(1 hunks)go/pkg/db/bulk_deployment_insert.sql.go(3 hunks)go/pkg/db/deployment_find_by_id.sql_generated.go(4 hunks)go/pkg/db/deployment_insert.sql_generated.go(6 hunks)go/pkg/db/deployment_queries_test.go(1 hunks)go/pkg/db/deployment_step_find_by_deployment_id.sql_generated.go(1 hunks)go/pkg/db/deployment_step_insert.sql_generated.go(1 hunks)go/pkg/db/deployment_update_openapi_spec.sql_generated.go(1 hunks)go/pkg/db/deployment_update_status.sql_generated.go(1 hunks)go/pkg/db/identity_delete.sql_generated.go(1 hunks)go/pkg/db/identity_delete_old_with_ratelimits.sql_generated.go(1 hunks)go/pkg/db/identity_find.sql_generated.go(1 hunks)go/pkg/db/identity_insert.sql_generated.go(1 hunks)go/pkg/db/identity_insert_ratelimit.sql_generated.go(1 hunks)go/pkg/db/identity_list.sql_generated.go(1 hunks)go/pkg/db/identity_list_ratelimits.sql_generated.go(1 hunks)go/pkg/db/identity_list_ratelimits_by_id.sql_generated.go(1 hunks)go/pkg/db/identity_list_ratelimits_by_ids.sql_generated.go(1 hunks)go/pkg/db/identity_soft_delete.sql_generated.go(1 hunks)go/pkg/db/identity_update.sql_generated.go(1 hunks)go/pkg/db/key_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/key_encryption_find_by_key_id.sql_generated.go(1 hunks)go/pkg/db/key_encryption_insert.sql_generated.go(1 hunks)go/pkg/db/key_find_by_id.sql_generated.go(1 hunks)go/pkg/db/key_find_credits.sql_generated.go(1 hunks)go/pkg/db/key_find_for_verification.sql_generated.go(1 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(1 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(1 hunks)go/pkg/db/key_insert.sql_generated.go(1 hunks)go/pkg/db/key_insert_ratelimit.sql_generated.go(1 hunks)go/pkg/db/key_list_by_key_auth_id.sql_generated.go(1 hunks)go/pkg/db/key_permission_delete_all_by_key_id.sql_generated.go(1 hunks)go/pkg/db/key_permission_delete_by_key_and_permission_id.sql_generated.go(1 hunks)go/pkg/db/key_permission_delete_many_by_key_and_permission_ids.sql_generated.go(1 hunks)go/pkg/db/key_permission_delete_many_by_permission_id.sql_generated.go(1 hunks)go/pkg/db/key_permission_insert.sql_generated.go(1 hunks)go/pkg/db/key_role_delete_all_by_key_id.sql_generated.go(1 hunks)go/pkg/db/key_role_delete_many_by_key_and_role_ids.sql_generated.go(1 hunks)go/pkg/db/key_role_delete_many_by_key_id.sql_generated.go(1 hunks)go/pkg/db/key_role_delete_many_by_role_id.sql_generated.go(1 hunks)go/pkg/db/key_role_find_by_key_and_role_id.sql_generated.go(1 hunks)go/pkg/db/key_role_insert.sql_generated.go(1 hunks)go/pkg/db/key_soft_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/key_soft_delete_many_by_key_auth_id.sql_generated.go(1 hunks)go/pkg/db/key_update.sql_generated.go(1 hunks)go/pkg/db/key_update_credits_refill.sql_generated.go(1 hunks)go/pkg/db/keyring_find_by_id.sql_generated.go(1 hunks)go/pkg/db/keyring_insert.sql_generated.go(1 hunks)go/pkg/db/keyring_update_key_encryption.sql_generated.go(1 hunks)go/pkg/db/models_generated.go(2 hunks)go/pkg/db/permission_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/permission_find_by_id.sql_generated.go(1 hunks)go/pkg/db/permission_find_by_id_or_slug.sql_generated.go(1 hunks)go/pkg/db/permission_find_by_name_and_workspace_id.sql_generated.go(1 hunks)go/pkg/db/permission_find_by_slug_and_workspace_id.sql_generated.go(1 hunks)go/pkg/db/permission_find_by_slugs.sql_generated.go(1 hunks)go/pkg/db/permission_insert.sql_generated.go(1 hunks)go/pkg/db/permission_list.sql_generated.go(1 hunks)go/pkg/db/permission_list_by_key_id.sql_generated.go(1 hunks)go/pkg/db/permission_list_by_role_id.sql_generated.go(1 hunks)go/pkg/db/permission_list_direct_by_key_id.sql_generated.go(1 hunks)go/pkg/db/project_find_by_id.sql_generated.go(1 hunks)go/pkg/db/project_find_by_workspace_slug.sql_generated.go(1 hunks)go/pkg/db/project_insert.sql_generated.go(1 hunks)go/pkg/db/querier_generated.go(4 hunks)go/pkg/db/queries/deployment_find_by_id.sql(1 hunks)go/pkg/db/queries/deployment_insert.sql(2 hunks)go/pkg/db/ratelimit_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_delete_many_by_identity_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_delete_many_by_ids.sql_generated.go(1 hunks)go/pkg/db/ratelimit_list_by_key_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_list_by_key_ids.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_find.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_find_by_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_find_by_name.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_insert.sql_generated.go(1 hunks)go/pkg/db/ratelimit_namespace_soft_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_find_by_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_find_by_identifier.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_insert.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_soft_delete.sql_generated.go(1 hunks)go/pkg/db/ratelimit_override_update.sql_generated.go(1 hunks)go/pkg/db/ratelimit_update.sql_generated.go(1 hunks)go/pkg/db/role_delete_by_id.sql_generated.go(1 hunks)go/pkg/db/role_find_by_id.sql_generated.go(1 hunks)go/pkg/db/role_find_by_id_or_name_with_perms.sql_generated.go(1 hunks)go/pkg/db/role_find_by_name_and_workspace_id.sql_generated.go(1 hunks)go/pkg/db/role_find_by_names.sql_generated.go(1 hunks)go/pkg/db/role_find_many_by_id_or_name_with_perms.sql_generated.go(1 hunks)go/pkg/db/role_find_many_by_name_with_perms.sql_generated.go(1 hunks)
⛔ Files not processed due to max files limit (18)
- go/pkg/db/role_insert.sql_generated.go
- go/pkg/db/role_list.sql_generated.go
- go/pkg/db/role_list_by_key_id.sql_generated.go
- go/pkg/db/role_permission_delete_many_by_permission_id.sql_generated.go
- go/pkg/db/role_permission_delete_many_by_role_id.sql_generated.go
- go/pkg/db/role_permission_find_by_role_and_permission_id.sql_generated.go
- go/pkg/db/role_permission_insert.sql_generated.go
- go/pkg/db/route_find_by_deployment_id.sql_generated.go
- go/pkg/db/route_insert.sql_generated.go
- go/pkg/db/schema.sql
- go/pkg/db/workspace_find_by_id.sql_generated.go
- go/pkg/db/workspace_hard_delete.sql_generated.go
- go/pkg/db/workspace_insert.sql_generated.go
- go/pkg/db/workspace_list.sql_generated.go
- go/pkg/db/workspace_soft_delete.sql_generated.go
- go/pkg/db/workspace_update_enabled.sql_generated.go
- go/pkg/db/workspace_update_plan.sql_generated.go
- go/proto/ctrl/v1/version.proto
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/db/ratelimit_list_by_key_ids.sql_generated.gogo/pkg/db/deployment_update_openapi_spec.sql_generated.gogo/pkg/db/ratelimit_update.sql_generated.gogo/pkg/db/identity_delete.sql_generated.gogo/pkg/db/key_encryption_insert.sql_generated.gogo/pkg/db/audit_log_insert.sql_generated.gogo/pkg/db/ratelimit_namespace_delete.sql_generated.gogo/pkg/db/key_soft_delete_many_by_key_auth_id.sql_generated.gogo/pkg/db/key_update.sql_generated.gogo/pkg/db/key_role_delete_many_by_key_id.sql_generated.gogo/pkg/db/audit_log_target_insert.sql_generated.gogo/pkg/db/api_update_delete_protection.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/identity_list_ratelimits.sql_generated.gogo/pkg/db/project_find_by_workspace_slug.sql_generated.gogo/pkg/db/ratelimit_override_find_by_id.sql_generated.gogo/pkg/db/key_find_by_id.sql_generated.gogo/pkg/db/identity_list_ratelimits_by_id.sql_generated.gogo/pkg/db/key_delete_by_id.sql_generated.gogo/pkg/db/key_permission_delete_many_by_key_and_permission_ids.sql_generated.gogo/pkg/db/key_find_for_verification.sql_generated.gogo/pkg/db/role_find_by_name_and_workspace_id.sql_generated.gogo/pkg/db/build_update_succeeded.sql_generated.gogo/pkg/db/permission_list_direct_by_key_id.sql_generated.gogo/pkg/db/key_insert_ratelimit.sql_generated.gogo/pkg/db/role_find_many_by_name_with_perms.sql_generated.gogo/pkg/db/role_delete_by_id.sql_generated.gogo/pkg/db/build_find_latest_by_deployment_id.sql_generated.gogo/pkg/db/identity_insert_ratelimit.sql_generated.gogo/pkg/db/audit_log_find_target_by_id.sql_generated.gogo/pkg/db/role_find_by_id.sql_generated.gogo/pkg/db/permission_find_by_id.sql_generated.gogo/pkg/db/project_find_by_id.sql_generated.gogo/pkg/db/key_encryption_find_by_key_id.sql_generated.gogo/pkg/db/keyring_find_by_id.sql_generated.gogo/pkg/db/keyring_update_key_encryption.sql_generated.gogo/pkg/db/ratelimit_delete_many_by_identity_id.sql_generated.gogo/pkg/db/deployment_step_insert.sql_generated.gogo/pkg/db/ratelimit_override_update.sql_generated.gogo/pkg/db/key_find_credits.sql_generated.gogo/pkg/db/deployment_update_status.sql_generated.gogo/pkg/db/key_soft_delete_by_id.sql_generated.gogo/pkg/db/key_role_insert.sql_generated.gogo/pkg/db/permission_find_by_slugs.sql_generated.gogo/pkg/db/ratelimit_namespace_soft_delete.sql_generated.gogo/pkg/db/deployment_step_find_by_deployment_id.sql_generated.gogo/pkg/db/key_role_delete_many_by_role_id.sql_generated.gogo/pkg/db/build_update_status.sql_generated.gogo/pkg/db/role_find_by_names.sql_generated.gogo/pkg/db/project_insert.sql_generated.gogo/pkg/db/ratelimit_namespace_find.sql_generated.gogo/pkg/db/identity_list_ratelimits_by_ids.sql_generated.gogo/pkg/db/permission_find_by_name_and_workspace_id.sql_generated.gogo/pkg/db/ratelimit_override_insert.sql_generated.gogo/pkg/db/permission_list.sql_generated.gogo/pkg/db/identity_update.sql_generated.gogo/pkg/db/build_insert.sql_generated.gogo/pkg/db/permission_list_by_role_id.sql_generated.gogo/pkg/db/ratelimit_list_by_key_id.sql_generated.gogo/pkg/db/ratelimit_delete_many_by_ids.sql_generated.gogo/pkg/db/ratelimit_namespace_insert.sql_generated.gogo/pkg/db/ratelimit_delete.sql_generated.gogo/pkg/db/ratelimit_override_soft_delete.sql_generated.gogo/pkg/db/identity_insert.sql_generated.gogo/pkg/db/key_role_delete_all_by_key_id.sql_generated.gogo/pkg/db/ratelimit_namespace_find_by_id.sql_generated.gogo/pkg/db/permission_insert.sql_generated.gogo/pkg/db/key_role_find_by_key_and_role_id.sql_generated.gogo/pkg/db/key_list_by_key_auth_id.sql_generated.gogo/pkg/db/role_find_many_by_id_or_name_with_perms.sql_generated.gogo/pkg/db/keyring_insert.sql_generated.gogo/pkg/db/key_permission_delete_all_by_key_id.sql_generated.gogo/pkg/db/identity_find.sql_generated.gogo/pkg/db/api_insert.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/pkg/db/build_update_failed.sql_generated.gogo/pkg/db/key_role_delete_many_by_key_and_role_ids.sql_generated.gogo/pkg/db/identity_delete_old_with_ratelimits.sql_generated.gogo/pkg/db/permission_list_by_key_id.sql_generated.gogo/pkg/db/key_permission_delete_many_by_permission_id.sql_generated.gogo/pkg/db/key_update_credits_refill.sql_generated.gogo/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.gogo/pkg/db/key_permission_insert.sql_generated.gogo/pkg/db/api_soft_delete.sql_generated.gogo/pkg/db/role_find_by_id_or_name_with_perms.sql_generated.gogo/pkg/db/identity_soft_delete.sql_generated.gogo/pkg/db/permission_find_by_id_or_slug.sql_generated.gogo/apps/ctrl/services/deployment/create_version.gogo/pkg/db/permission_find_by_slug_and_workspace_id.sql_generated.gogo/pkg/db/permission_delete_by_id.sql_generated.gogo/pkg/db/api_find_by_id.sql_generated.gogo/pkg/db/identity_list.sql_generated.gogo/pkg/db/ratelimit_override_find_by_identifier.sql_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/build_find_by_id.sql_generated.gogo/pkg/db/deployment_queries_test.gogo/pkg/db/ratelimit_namespace_find_by_name.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/deployment_insert.sql_generated.gogo/pkg/db/models_generated.gogo/apps/ctrl/services/deployment/create_version_simple_test.gogo/pkg/db/deployment_find_by_id.sql_generated.gogo/apps/ctrl/services/deployment/get_version.gogo/pkg/db/bulk_deployment_insert.sql.gogo/pkg/db/key_permission_delete_by_key_and_permission_id.sql_generated.go
**/*.{env,js,ts,go}
📄 CodeRabbit inference engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/db/ratelimit_list_by_key_ids.sql_generated.gogo/pkg/db/deployment_update_openapi_spec.sql_generated.gogo/pkg/db/ratelimit_update.sql_generated.gogo/pkg/db/identity_delete.sql_generated.gogo/pkg/db/key_encryption_insert.sql_generated.gogo/pkg/db/audit_log_insert.sql_generated.gogo/pkg/db/ratelimit_namespace_delete.sql_generated.gogo/pkg/db/key_soft_delete_many_by_key_auth_id.sql_generated.gogo/pkg/db/key_update.sql_generated.gogo/pkg/db/key_role_delete_many_by_key_id.sql_generated.gogo/pkg/db/audit_log_target_insert.sql_generated.gogo/pkg/db/api_update_delete_protection.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/identity_list_ratelimits.sql_generated.gogo/pkg/db/project_find_by_workspace_slug.sql_generated.gogo/pkg/db/ratelimit_override_find_by_id.sql_generated.gogo/pkg/db/key_find_by_id.sql_generated.gogo/pkg/db/identity_list_ratelimits_by_id.sql_generated.gogo/pkg/db/key_delete_by_id.sql_generated.gogo/pkg/db/key_permission_delete_many_by_key_and_permission_ids.sql_generated.gogo/pkg/db/key_find_for_verification.sql_generated.gogo/pkg/db/role_find_by_name_and_workspace_id.sql_generated.gogo/pkg/db/build_update_succeeded.sql_generated.gogo/pkg/db/permission_list_direct_by_key_id.sql_generated.gogo/pkg/db/key_insert_ratelimit.sql_generated.gogo/pkg/db/role_find_many_by_name_with_perms.sql_generated.gogo/pkg/db/role_delete_by_id.sql_generated.gogo/pkg/db/build_find_latest_by_deployment_id.sql_generated.gogo/pkg/db/identity_insert_ratelimit.sql_generated.gogo/pkg/db/audit_log_find_target_by_id.sql_generated.gogo/pkg/db/role_find_by_id.sql_generated.gogo/pkg/db/permission_find_by_id.sql_generated.gogo/pkg/db/project_find_by_id.sql_generated.gogo/pkg/db/key_encryption_find_by_key_id.sql_generated.gogo/pkg/db/keyring_find_by_id.sql_generated.gogo/pkg/db/keyring_update_key_encryption.sql_generated.gogo/pkg/db/ratelimit_delete_many_by_identity_id.sql_generated.gogo/pkg/db/deployment_step_insert.sql_generated.gogo/pkg/db/ratelimit_override_update.sql_generated.gogo/pkg/db/key_find_credits.sql_generated.gogo/pkg/db/deployment_update_status.sql_generated.gogo/pkg/db/key_soft_delete_by_id.sql_generated.gogo/pkg/db/key_role_insert.sql_generated.gogo/pkg/db/permission_find_by_slugs.sql_generated.gogo/pkg/db/ratelimit_namespace_soft_delete.sql_generated.gogo/pkg/db/deployment_step_find_by_deployment_id.sql_generated.gogo/pkg/db/key_role_delete_many_by_role_id.sql_generated.gogo/pkg/db/build_update_status.sql_generated.gogo/pkg/db/role_find_by_names.sql_generated.gogo/pkg/db/project_insert.sql_generated.gogo/pkg/db/ratelimit_namespace_find.sql_generated.gogo/pkg/db/identity_list_ratelimits_by_ids.sql_generated.gogo/pkg/db/permission_find_by_name_and_workspace_id.sql_generated.gogo/pkg/db/ratelimit_override_insert.sql_generated.gogo/pkg/db/permission_list.sql_generated.gogo/pkg/db/identity_update.sql_generated.gogo/pkg/db/build_insert.sql_generated.gogo/pkg/db/permission_list_by_role_id.sql_generated.gogo/pkg/db/ratelimit_list_by_key_id.sql_generated.gogo/pkg/db/ratelimit_delete_many_by_ids.sql_generated.gogo/pkg/db/ratelimit_namespace_insert.sql_generated.gogo/pkg/db/ratelimit_delete.sql_generated.gogo/pkg/db/ratelimit_override_soft_delete.sql_generated.gogo/pkg/db/identity_insert.sql_generated.gogo/pkg/db/key_role_delete_all_by_key_id.sql_generated.gogo/pkg/db/ratelimit_namespace_find_by_id.sql_generated.gogo/pkg/db/permission_insert.sql_generated.gogo/pkg/db/key_role_find_by_key_and_role_id.sql_generated.gogo/pkg/db/key_list_by_key_auth_id.sql_generated.gogo/pkg/db/role_find_many_by_id_or_name_with_perms.sql_generated.gogo/pkg/db/keyring_insert.sql_generated.gogo/pkg/db/key_permission_delete_all_by_key_id.sql_generated.gogo/pkg/db/identity_find.sql_generated.gogo/pkg/db/api_insert.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/pkg/db/build_update_failed.sql_generated.gogo/pkg/db/key_role_delete_many_by_key_and_role_ids.sql_generated.gogo/pkg/db/identity_delete_old_with_ratelimits.sql_generated.gogo/pkg/db/permission_list_by_key_id.sql_generated.gogo/pkg/db/key_permission_delete_many_by_permission_id.sql_generated.gogo/pkg/db/key_update_credits_refill.sql_generated.gogo/pkg/db/ratelimit_override_list_by_namespace_id.sql_generated.gogo/pkg/db/key_permission_insert.sql_generated.gogo/pkg/db/api_soft_delete.sql_generated.gogo/pkg/db/role_find_by_id_or_name_with_perms.sql_generated.gogo/pkg/db/identity_soft_delete.sql_generated.gogo/pkg/db/permission_find_by_id_or_slug.sql_generated.gogo/apps/ctrl/services/deployment/create_version.gogo/pkg/db/permission_find_by_slug_and_workspace_id.sql_generated.gogo/pkg/db/permission_delete_by_id.sql_generated.gogo/pkg/db/api_find_by_id.sql_generated.gogo/pkg/db/identity_list.sql_generated.gogo/pkg/db/ratelimit_override_find_by_identifier.sql_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/build_find_by_id.sql_generated.gogo/pkg/db/deployment_queries_test.gogo/pkg/db/ratelimit_namespace_find_by_name.sql_generated.gogo/pkg/db/querier_generated.gogo/pkg/db/deployment_insert.sql_generated.gogo/pkg/db/models_generated.gogo/apps/ctrl/services/deployment/create_version_simple_test.gogo/pkg/db/deployment_find_by_id.sql_generated.gogo/apps/ctrl/services/deployment/get_version.gogo/pkg/db/bulk_deployment_insert.sql.gogo/pkg/db/key_permission_delete_by_key_and_permission_id.sql_generated.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/pkg/db/deployment_queries_test.gogo/apps/ctrl/services/deployment/create_version_simple_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: For repo unkeyed/unkey and PR review workflows: When imeyer comments "issue" on a thread, automatically create a thorough GitHub issue (sections: Summary, Impact, Where, Repro/Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and the specific comment, and assign the issue to imeyer.
📚 Learning: 2025-08-21T15:54:45.183Z
Learnt from: chronark
PR: unkeyed/unkey#3825
File: go/internal/services/usagelimiter/limit.go:38-0
Timestamp: 2025-08-21T15:54:45.183Z
Learning: In go/internal/services/usagelimiter/limit.go, the UpdateKeyCreditsDecrement operation cannot be safely wrapped with db.WithRetry due to the lack of idempotency mechanisms in the current tech stack. Retrying this non-idempotent write operation risks double-charging users if the first attempt commits but the client sees a transient error.
Applied to files:
go/pkg/db/ratelimit_list_by_key_ids.sql_generated.gogo/pkg/db/ratelimit_list_by_key_id.sql_generated.gogo/pkg/db/key_update_credits_refill.sql_generated.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
PR: unkeyed/unkey#3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/db/key_encryption_insert.sql_generated.gogo/pkg/db/key_update.sql_generated.gogo/pkg/db/key_insert_ratelimit.sql_generated.gogo/pkg/db/key_role_delete_all_by_key_id.sql_generated.gogo/pkg/db/key_list_by_key_auth_id.sql_generated.gogo/pkg/db/keyring_insert.sql_generated.gogo/pkg/db/key_insert.sql_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.go
📚 Learning: 2025-07-21T18:05:58.236Z
Learnt from: CR
PR: unkeyed/unkey#0
File: go/deploy/CLAUDE.md:0-0
Timestamp: 2025-07-21T18:05:58.236Z
Learning: Applies to go/deploy/deploy/{assetmanagerd,billaged,builderd,metald}/**/*.go : When a service's `*.go` code changes significantly, increase the patch-level version number.
Applied to files:
go/pkg/db/deployment_find_by_id.sql_generated.gogo/apps/ctrl/services/deployment/get_version.go
🧬 Code graph analysis (5)
go/apps/ctrl/services/deployment/create_version.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/pkg/db/deployment_queries_test.go (2)
go/pkg/db/deployment_insert.sql_generated.go (1)
InsertDeploymentParams(59-79)go/pkg/db/models_generated.go (2)
DeploymentsEnvironmentPreview(199-199)DeploymentsStatusPending(240-240)
go/pkg/db/deployment_insert.sql_generated.go (1)
go/pkg/db/models_generated.go (2)
DeploymentsEnvironment(195-195)DeploymentsStatus(237-237)
go/pkg/db/models_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/ctrl/services/deployment/create_version_simple_test.go (4)
go/pkg/db/deployment_insert.sql_generated.go (1)
InsertDeploymentParams(59-79)go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/db/models_generated.go (1)
Deployment(655-675)go/gen/proto/ctrl/v1/version.pb.go (2)
SourceType_SOURCE_TYPE_GIT(91-91)VersionStatus_VERSION_STATUS_ACTIVE(32-32)
🪛 golangci-lint (2.2.2)
go/pkg/db/deployment_queries_test.go
18-18: undefined: sqlmock
(typecheck)
113-113: undefined: sqlmock
(typecheck)
116-116: not enough arguments in call to New
have ()
want (Config)
(typecheck)
127-127: undefined: sqlmock
(typecheck)
221-221: undefined: sqlmock
(typecheck)
224-224: not enough arguments in call to New
have ()
want (Config)
(typecheck)
235-235: undefined: sqlmock
(typecheck)
268-268: undefined: sqlmock
(typecheck)
269-269: undefined: sqlmock
(typecheck)
275-275: undefined: sqlmock
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
go/pkg/db/ratelimit_override_find_by_identifier.sql_generated.go
Outdated
Show resolved
Hide resolved
mcstepp
force-pushed
the
ENG-1992-rich-git-commit-data
branch
from
3c539bb to
a430d43
Compare
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
please run go fmt ./...
this PR is mostly formatting changes cause you regenerated without formatting (I fixed that in the make tooling already in another PR)
mcstepp
force-pushed
the
ENG-1992-rich-git-commit-data
branch
from
abf8414 to
b3a2aec
Compare
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (20)
go/pkg/db/permission_delete_by_id.sql_generated.go (1)
12-23: Optional: return affected row count to distinguish “not found” vs successful delete.If callers need to know whether a permission actually existed, prefer using
:execrowsin the source SQL so the generated method returns(int64, error). This avoids silent no-ops wheniddoesn’t match any row.Apply in the SQL source (not this generated file):
-- go/pkg/db/permission_delete_by_id.sql --- name: DeletePermission :exec +-- name: DeletePermission :execrows DELETE FROM permissions WHERE id = ?Expected generated signature after
sqlc generate:// func (q *Queries) DeletePermission(ctx context.Context, db DBTX, permissionID string) (int64, error)Callers can then assert
rows > 0to confirm deletion.go/pkg/db/ratelimit_delete_many_by_ids.sql_generated.go (1)
21-33: Call sites already guard empty slices; consider optional chunking for very large deletesI verified that every call to
DeleteManyRatelimitsByIDsis already wrapped in aif len(ids) > 0check, so there’s no scenario where the generated query executesIN (NULL)in production:
- go/apps/api/routes/v2_keys_update_key/handler.go: lines 324–327
- go/apps/api/routes/v2_identities_update_identity/handler.go: lines 249–253
Since the empty-slice case is already handled upstream, no need to short-circuit in generated code. If you anticipate very large ID lists (which can hit query-size or driver limits), you may still want to introduce an optional batch-chunking helper in your service layer:
// Package internal/dbutil package dbutil import ( "context" "your/module/go/pkg/db" ) const defaultChunkSize = 1000 // tune per SQL dialect // DeleteManyRatelimitsByIDsInBatches breaks a large list into chunks // to avoid oversized IN() clauses. func DeleteManyRatelimitsByIDsInBatches( ctx context.Context, q *db.Queries, tx db.DBTX, ids []string, ) error { for start := 0; start < len(ids); start += defaultChunkSize { end := start + defaultChunkSize if end > len(ids) { end = len(ids) } if err := q.DeleteManyRatelimitsByIDs(ctx, tx, ids[start:end]); err != nil { return err } } return nil }Use this helper instead of calling
DeleteManyRatelimitsByIDsdirectly when dealing with potentially large ID slices.go/pkg/db/key_role_find_by_key_and_role_id.sql_generated.go (1)
12-17: Verified: Unique Constraint on (key_id, role_id) PresentThe
keys_rolestable ingo/pkg/db/schema.sqldefines both a primary key on (role_id,key_id,workspace_id) and an explicitUNIQUE(key_id, role_id)constraint, ensuring each key–role pair is unique within a workspace. Given this invariant, it’s safe—and more idiomatic—to switch your SQLC query from:manyto:one, which will:
- Prevent unnecessary slice allocations
- Surface “not found” errors directly instead of requiring post-call length checks
To apply this refactor:
• In
go/pkg/db/key_role_find_by_key_and_role_id.sql, change the annotation from:manyto:one--- a/go/pkg/db/key_role_find_by_key_and_role_id.sql --- name: FindKeyRoleByKeyAndRoleID :many +-- name: FindKeyRoleByKeyAndRoleID :one SELECT key_id, role_id, workspace_id, created_at_m, updated_at_m FROM keys_roles WHERE key_id = ? AND role_id = ?• Regenerate your SQLC code to produce:
FindKeyRoleByKeyAndRoleID(ctx, db, params) (KeysRole, error)• Update all call sites—most notably:
apps/api/routes/v2_permissions_delete_role/200_test.go(switch from[]KeysRoletoKeysRoleand handle “not found” errors appropriately)- Any other references in
go/pkg/db/querier_generated.goand application codeThis change cleanly encodes the uniqueness invariant in your API and simplifies client code.
go/pkg/db/identity_soft_delete.sql_generated.go (1)
12-17: Split SoftDeleteIdentity into two indexable mutationsWe’ve confirmed that the current
SoftDeleteIdentityquery ingo/pkg/db/queries/identity_soft_delete.sql(and its generated counterpart ingo/pkg/db/identity_soft_delete.sql_generated.go) uses a single parameter against(id = ? OR external_id = ?). This can inadvertently soft-delete multiple rows if one row’sidand another’sexternal_idshare the same value within the sameworkspace_id, and it prevents either index (the PK onidor the unique index on(workspace_id, external_id, deleted)) from being used efficiently.Required changes:
go/pkg/db/queries/identity_soft_delete.sql
Replace the singleSoftDeleteIdentity :execstatement with two separate mutations:-- name: SoftDeleteIdentity :exec -UPDATE identities -SET deleted = 1 -WHERE workspace_id = sqlc.arg('workspace_id') - AND (id = sqlc.arg('identity') OR external_id = sqlc.arg('identity')); +-- name: SoftDeleteIdentityByID :exec +UPDATE identities +SET deleted = 1 +WHERE workspace_id = sqlc.arg('workspace_id') + AND id = sqlc.arg('identity'); + +-- name: SoftDeleteIdentityByExternalID :exec +UPDATE identities +SET deleted = 1 +WHERE workspace_id = sqlc.arg('workspace_id') + AND external_id = sqlc.arg('identity');go/pkg/db/identity_soft_delete.sql_generated.go
Regenerate so there are two functions (SoftDeleteIdentityByIDandSoftDeleteIdentityByExternalID) instead of one, each binding its own parameter.Call sites
Update all uses of the oldSoftDeleteIdentitymethod—found in:
apps/api/routes/v2_identities_delete_identity/handler.goapps/api/routes/v2_identities_get_identity/404_test.goapps/api/routes/v2_identities_list_identities/200_test.goto invoke the appropriate new function based on whether you’re deleting by internal
idor byexternal_id.Index coverage is already in place via:
- Primary key on
identities(id)- Unique index
workspace_id_external_id_deleted_idxon(workspace_id, external_id, deleted)Splitting the queries will restore single-row semantics and allow each predicate to fully leverage its respective index.
go/pkg/db/ratelimit_namespace_insert.sql_generated.go (1)
12-31: AlignInsertRatelimitNamespaceparams with thecreated_at_mcolumnThe schema for
ratelimit_namespacesdefines and usescreated_at_m(notcreated_at), yet the generated insert params struct wrongly maps the fourth field todb:"created_at". To avoid confusion and potential breakage when using named‐based utilities, update the SQL/query definitions so sqlc emits aCreatedAtM int64db:"created_at_m"field instead ofCreatedAt`:• In go/pkg/db/queries/ratelimit_namespace_insert.sql
– Change the arg mapping from
…VALUES ( ?, ?, ?, sqlc.arg(created_at), …)
to
…VALUES ( ?, ?, ?, sqlc.arg(created_at_m), …)• Regenerate the code to pick up:
CreatedAtM int64db:"created_at_m"Do not hand‐edit the generated file (go/pkg/db/ratelimit_namespace_insert.sql_generated.go). After adjusting the query, run
sqlc generateso the struct field and tag are consistent with the schema.go/pkg/db/key_permission_insert.sql_generated.go (1)
24-33: Ensure CreatedAt and UpdatedAt are populated for InsertKeyPermission
The generated SQL in key_permission_insert.sql_generated.go (lines 49–51) binds bothCreatedAt(forcreated_at_m) andUpdatedAt(forupdated_at_mon duplicate keys). When callers omit these fields, they default to zero/NULL, so neither timestamp reflects the real operation time. I’ve identified the following callers that need adjustment:
- go/pkg/testutil/seed/seed.go (line 174):
InsertKeyPermissionParamsliteral lacksCreatedAt/UpdatedAt.- go/apps/api/routes/v2_keys_remove_permissions/200_test.go: calls to
InsertKeyPermissionomit both timestamp fields.- go/apps/api/routes/v2_keys_set_permissions/200_test.go: similarly omits
CreatedAt/UpdatedAt.To fix, update each call (or introduce a shared helper) to include something like:
CreatedAt: time.Now().UnixMilli(), UpdatedAt: sql.NullInt64{Int64: time.Now().UnixMilli(), Valid: true},This will ensure
created_at_mandupdated_at_maccurately track when the record was inserted or upserted.go/pkg/db/key_update_credits_set.sql_generated.go (1)
13-22: Danger: NULL credits will set remaining_requests to NULLUpdate uses
sql.NullInt32for Credits. IfValid=false, MySQL arithmetic/assignment with NULL will setremaining_requeststo NULL. That likely breaks metering/accounting.Two safe options (edit the source SQL in go/pkg/db/queries/key_update_credits_set.sql and regenerate):
Option A (no-op on NULL):
-SET remaining_requests = ? +SET remaining_requests = COALESCE(?, remaining_requests)Option B (require non-NULL input; defensive check in code):
- Validate
Credits.Validbefore calling this query; reject otherwise.- Consider NOT NULL constraint on
keys.remaining_requests.Pick A if caller may intentionally pass NULL; otherwise pick B and add input validation. I can update the .sql and regenerate if you confirm the intended behavior.
Also applies to: 24-31
go/pkg/db/key_update_credits_increment.sql_generated.go (1)
13-22: NULL increment will NULL-out remaining_requests
remaining_requests = remaining_requests + ?with a NULL parameter yields NULL in MySQL. That’s a hard-to-detect data corruption path.Fix at the source SQL (go/pkg/db/queries/key_update_credits_increment.sql), then regenerate:
-SET remaining_requests = remaining_requests + ? +SET remaining_requests = remaining_requests + COALESCE(?, 0)Additionally, validate that
Credits.Validis true at call sites to avoid accidental NULLs. Consider a NOT NULL constraint onremaining_requestsif not already present.Also applies to: 24-31
go/pkg/db/ratelimit_override_update.sql_generated.go (1)
23-29: Naming mismatch: struct field Windowlimit vs columnlimitNot introduced by this PR, but the param name Windowlimit can be confusing given the column is
limit. If you touch the SQL next time, consider aliasing for clarity so sqlc generates a clearer param name, e.g.:UPDATE `ratelimit_overrides` SET `limit` = ? /* alias: window_limit */, ...or rename the column reference in SQLC config to yield
Limit/WindowLimitconsistently. Leave generated code as-is for now.go/pkg/db/key_find_live_by_id.sql_generated.go (1)
137-141: Optional: prefer json.RawMessage over interface{} for JSON columnsNot part of this change, but mapping JSON aggregations to interface{} loses type info and can complicate downstream unmarshalling. If/when you touch the SQL or sqlc config, consider mapping these to json.RawMessage for stricter typing without runtime cost.
go/pkg/db/key_role_delete_many_by_key_and_role_ids.sql_generated.go (1)
13-41: Avoid executing DELETE with IN(NULL) for empty role list; consider early return and chunking for large lists.Current behavior replaces an empty slice with
IN(NULL), which is a no-op but still incurs a DB round-trip. Also, very largerole_idsmay hit driver/statement limits. Prefer short-circuiting whenlen(role_ids) == 0, and consider chunking when the list is large. Note: do not hand-edit generated files—apply the change in the SQL source and regenerate.Apply this diff conceptually (in the source that generates this code), then regenerate:
func (q *Queries) DeleteManyKeyRolesByKeyAndRoleIDs(ctx context.Context, db DBTX, arg DeleteManyKeyRolesByKeyAndRoleIDsParams) error { + // Fast-path: nothing to delete + if len(arg.RoleIds) == 0 { + return nil + } query := deleteManyKeyRolesByKeyAndRoleIDs var queryParams []interface{} queryParams = append(queryParams, arg.KeyID) - if len(arg.RoleIds) > 0 { - for _, v := range arg.RoleIds { - queryParams = append(queryParams, v) - } - query = strings.Replace(query, "/*SLICE:role_ids*/?", strings.Repeat(",?", len(arg.RoleIds))[1:], 1) - } else { - query = strings.Replace(query, "/*SLICE:role_ids*/?", "NULL", 1) - } + for _, v := range arg.RoleIds { + queryParams = append(queryParams, v) + } + query = strings.Replace(query, "/*SLICE:role_ids*/?", strings.Repeat(",?", len(arg.RoleIds))[1:], 1) _, err := db.ExecContext(ctx, query, queryParams...) return err }If your inputs can exceed driver placeholder limits, introduce chunking in the caller:
- Split role_ids into batches (e.g., 1000 each).
- Execute the generated function per batch within a transaction.
go/pkg/db/key_permission_delete_many_by_key_and_permission_ids.sql_generated.go (1)
13-41: Add a unit test for empty Ids and consider batching large slices
- No existing tests reference
DeleteManyKeyPermissionByKeyAndPermissionIDswith an emptyIdsslice, so we should add one to assert that passingIds: []results in no rows deleted.- At the service layer, consider splitting very large
Idsslices into batches (e.g. chunks of 1000) to avoid exceeding database parameter limits or generating excessively longINlists.go/pkg/db/ratelimit_list_by_key_id.sql_generated.go (1)
42-44: ListRatelimitsByKeyID callers always passValid=trueI searched all usages of
ListRatelimitsByKeyIDand confirmed that every call site—both in handlers and in tests—constructs thesql.NullStringwithValid: true. For example:
- In
v2_keys_update_key/handler.go:
db.Query.ListRatelimitsByKeyID(ctx, tx, sql.NullString{String: key.ID, Valid: true})- In
v2_keys_delete_key/200_test.go:
db.Query.ListRatelimitsByKeyID(ctx, h.DB.RO(), sql.NullString{String: hardDeleteKeyID, Valid: true})- In
v2_keys_update_key/200_test.go(twice):
sql.NullString{String: keyResponse.KeyID, Valid: true}Since all callers set
Valid=true, there’s no risk of accidentally queryingWHERE key_id = NULLand getting no rows.• Optional refactor: if your database schema guarantees that
ratelimits.key_idis defined asNOT NULL, you could simplify the generated code by overriding the column’s Go type tostringin your sqlc config. That would eliminate theNullStringfootgun altogether.go/pkg/db/ratelimit_override_insert.sql_generated.go (1)
34-39: MySQL VALUES() deprecation heads-up for future-proofing.MySQL 8.0.20+ deprecates the VALUES() function in ON DUPLICATE KEY UPDATE. While this generated code is fine today, consider updating the source SQL (go/pkg/db/queries/ratelimit_override_insert.sql) to use the INSERT ... AS new ... ON DUPLICATE KEY UPDATE pattern the next time you touch it. Keep updated_at_m as a bound param if you don’t want to include it in the insert column list.
Example for the source SQL (illustrative):
INSERT INTO ratelimit_overrides ( id, workspace_id, namespace_id, identifier, `limit`, duration, async, created_at_m ) VALUES (?, ?, ?, ?, ?, ?, false, ?) AS new ON DUPLICATE KEY UPDATE `limit` = new.`limit`, duration = new.duration, async = new.async, updated_at_m = ?Please verify your target MySQL version and whether a migration off VALUES() is on your roadmap.
go/pkg/db/retry.go (2)
14-22: Add an explicit idempotency warning to prevent misuse.Per past incidents, wrapping non-idempotent writes (e.g., credit decrements) risks double-charging on partial failures. Add a prominent caution in the docblock to dissuade misuse.
Apply this doc-only diff:
// WithRetry executes a database operation with optimized retry configuration. // It retries transient errors with exponential backoff but skips non-retryable errors // like "not found" or "duplicate key" to avoid unnecessary delays. // // Configuration: // - 3 attempts maximum // - Exponential backoff: 50ms, 100ms, 200ms // - Skips retries for "not found" and "duplicate key" errors +// +// AIDEV-NOTE: Only wrap idempotent operations or operations protected by +// transactional/uniqueness guarantees. Do NOT use for non-idempotent writes +// (e.g., decrementing credits, charging, emitting external side effects), +// as retries may apply the mutation multiple times on ambiguous failures.
31-41: Optional: add jitter to backoff to reduce herd effects under contention.Small randomized jitter reduces synchronized retries after shared failures.
Apply this optional diff:
import ( "time" "github.com/unkeyed/unkey/go/pkg/retry" ) @@ retry.Backoff(func(n int) time.Duration { - // Predefined backoff delays: 50ms, 100ms, 200ms - delays := []time.Duration{ - DefaultBackoff, // 50ms for attempt 1 - DefaultBackoff * 2, // 100ms for attempt 2 - DefaultBackoff * 4, // 200ms for attempt 3 - } - if n <= 0 || n > len(delays) { - return DefaultBackoff // fallback to base delay - } - return delays[n-1] + // Predefined backoff delays: 50ms, 100ms, 200ms, with +/- 20% jitter + delays := []time.Duration{DefaultBackoff, DefaultBackoff * 2, DefaultBackoff * 4} + if n <= 0 || n > len(delays) { + return DefaultBackoff + } + base := delays[n-1] + // simple jitter without extra deps + jitterNs := time.Now().UnixNano() % int64(base/5) // 20% max + if (time.Now().UnixNano()&1) == 0 { + return base + time.Duration(jitterNs) + } + return base - time.Duration(jitterNs) }),go/pkg/db/bulk_deployment_insert.sql.go (1)
21-55: Prevent oversized statements: chunk large bulk inserts (e.g., 200–500 rows per batch).Long VALUES lists risk exceeding max packet/SQL limits. Batch the input and execute multiple INSERTs.
Apply this refactor within InsertDeployments:
@@ - // Build the bulk insert query - valueClauses := make([]string, len(args)) - for i := range args { - valueClauses[i] = "( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" - } - bulkQuery := fmt.Sprintf(bulkInsertDeployment, strings.Join(valueClauses, ", ")) - - // Collect all arguments - var allArgs []any - for _, arg := range args { - allArgs = append(allArgs, arg.ID) - allArgs = append(allArgs, arg.WorkspaceID) - allArgs = append(allArgs, arg.ProjectID) - allArgs = append(allArgs, arg.Environment) - allArgs = append(allArgs, arg.BuildID) - allArgs = append(allArgs, arg.RootfsImageID) - allArgs = append(allArgs, arg.GitCommitSha) - allArgs = append(allArgs, arg.GitBranch) - allArgs = append(allArgs, arg.GitCommitMessage) - allArgs = append(allArgs, arg.GitCommitAuthorName) - allArgs = append(allArgs, arg.GitCommitAuthorEmail) - allArgs = append(allArgs, arg.GitCommitAuthorUsername) - allArgs = append(allArgs, arg.GitCommitAuthorAvatarUrl) - allArgs = append(allArgs, arg.GitCommitTimestamp) - allArgs = append(allArgs, arg.ConfigSnapshot) - allArgs = append(allArgs, arg.OpenapiSpec) - allArgs = append(allArgs, arg.Status) - allArgs = append(allArgs, arg.CreatedAt) - allArgs = append(allArgs, arg.UpdatedAt) - } - - // Execute the bulk insert - _, err := db.ExecContext(ctx, bulkQuery, allArgs...) - return err + const chunkSize = 500 + for start := 0; start < len(args); start += chunkSize { + end := start + chunkSize + if end > len(args) { + end = len(args) + } + batch := args[start:end] + + // Build placeholders for this batch + valueClauses := make([]string, len(batch)) + for i := range batch { + valueClauses[i] = "( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" + } + bulkQuery := fmt.Sprintf(bulkInsertDeployment, strings.Join(valueClauses, ", ")) + + // Collect arguments for this batch + var allArgs []any + for _, arg := range batch { + allArgs = append(allArgs, + arg.ID, + arg.WorkspaceID, + arg.ProjectID, + arg.Environment, + arg.BuildID, + arg.RootfsImageID, + arg.GitCommitSha, + arg.GitBranch, + arg.GitCommitMessage, + arg.GitCommitAuthorName, + arg.GitCommitAuthorEmail, + arg.GitCommitAuthorUsername, + arg.GitCommitAuthorAvatarUrl, + arg.GitCommitTimestamp, + arg.ConfigSnapshot, + arg.OpenapiSpec, + arg.Status, + arg.CreatedAt, + arg.UpdatedAt, + ) + } + if _, err := db.ExecContext(ctx, bulkQuery, allArgs...); err != nil { + return err + } + } + return nilgo/apps/ctrl/services/deployment/get_version.go (1)
80-85: Differentiate not-found vs real errors when fetching latest build.Silently ignoring all errors can hide operational issues. Ignore not-found; log/warn unexpected errors.
- build, err := db.Query.FindLatestBuildByDeploymentId(ctx, s.db.RO(), deployment.ID) - if err == nil { - protoVersion.BuildId = build.ID - } + build, err := db.Query.FindLatestBuildByDeploymentId(ctx, s.db.RO(), deployment.ID) + if err == nil { + protoVersion.BuildId = build.ID + } else if !db.IsNotFound(err) { + s.logger.Warn("failed to fetch latest build for deployment", "error", err, "deployment_id", deployment.ID) + }go/apps/ctrl/services/deployment/create_version.go (2)
62-69: Use Valid flag on DefaultBranch instead of reading String unconditionally.Avoid relying on String when Valid=false (can carry stale values). Fall back only if DefaultBranch is null/empty.
gitBranch := req.Msg.GetBranch() if gitBranch == "" { - gitBranch = project.DefaultBranch.String - if gitBranch == "" { + if project.DefaultBranch.Valid && project.DefaultBranch.String != "" { + gitBranch = project.DefaultBranch.String + } else { gitBranch = "main" // fallback default - } + } }
88-91: Track TODO for environment field in proto.Happy to open a follow-up issue to add environment to CreateVersionRequest and wire it through.
♻️ Duplicate comments (48)
go/pkg/db/key_find_credits.sql_generated.go (1)
3-3: Unify sqlc header to a single repo-wide version (expected v1.29.0).This file is on sqlc v1.28.0 while prior guidance and other files target v1.29.0. Please regenerate with the pinned version to avoid churn and mismatched codegen metadata. If regeneration isn’t possible right now, minimally align the header, but prefer regeneration.
You can verify repo-wide consistency with:
#!/bin/bash set -euo pipefail echo "sqlc headers by version:" rg -nP '^//\s+sqlc v1\.\d+\.\d+' -g 'go/**/*.go' | sort | uniq -c echo -e "\nFiles not on v1.29.0:" rg -nP '^\s*//\s+sqlc v1\.(?!29\.0)\d+\.\d+' -g 'go/**/*.go' || echo "All good" # Optional: Check the sqlc version pin in tooling fd -t f -g 'Makefile|**/*.sh|**/scripts/**|**/Dockerfile|**/sqlc.*.yaml|**/sqlc.yaml' \ | xargs -I{} rg -n 'sqlc|v1\.29\.0' {}If you must hotfix the header pending regeneration, this is the minimal diff (again, prefer regenerating):
-// sqlc v1.28.0 +// sqlc v1.29.0go/pkg/db/key_encryption_find_by_key_id.sql_generated.go (1)
3-3: Unify sqlc version across generated files (this file shows v1.28.0).The PR appears to standardize sqlc output, but this header is v1.28.0. Prior review threads suggested targeting v1.29.0 repo-wide. Please confirm the intended version and regenerate to a single version to avoid churn and subtle diffs.
Run this to verify repository-wide consistency and surface outliers:
#!/bin/bash set -euo pipefail echo "Enumerating sqlc versions found in generated headers..." rg -nP --type=go -g '!**/vendor/**' -g '!**/node_modules/**' -C0 '^\s*//\s+sqlc v[0-9]+\.[0-9]+\.[0-9]+' \ | sed -E 's/^(.+):.*(sqlc v[0-9]+\.[0-9]+\.[0-9]+).*$/\2 \1/' \ | sort -u echo -e "\nChecking for multiple versions..." VERSIONS=$(rg -nP --glob '**/*_generated.go' -g '!**/vendor/**' '^\s*//\s+sqlc v[0-9]+\.[0-9]+\.[0-9]+' \ | sed -E 's/^.+sqlc (v[0-9]+\.[0-9]+\.[0-9]+).*$/\1/' | sort -u) echo "Found versions: ${VERSIONS}" if [ "$(echo "$VERSIONS" | wc -l | tr -d ' ')" -ne 1 ]; then echo "ERROR: Multiple sqlc versions detected. Please regenerate all with the single intended version." >&2 exit 1 else echo "OK: Single sqlc version detected." fi # Optional: enforce a specific target (uncomment and set desired version, e.g., v1.29.0) # TARGET=v1.29.0 # if [ "$VERSIONS" != "$TARGET" ]; then # echo "ERROR: Detected sqlc $VERSIONS, expected $TARGET." >&2 # exit 1 # figo/pkg/db/key_update.sql_generated.go (1)
3-3: Repo-wide sqlc version drift: this file is v1.28.0; project guidance is to standardize on v1.29.0Previous reviews requested pinning/regenerating all sqlc-generated files to v1.29.0 to avoid noisy diffs and mixed outputs. Please confirm target version and regenerate accordingly, then pin it in CI/devcontainer/tooling.
Apply after regenerating (illustrative diff of the header only):
// versions: -// sqlc v1.28.0 +// sqlc v1.29.0To find any stragglers:
#!/bin/bash rg -nP '^//\s+sqlc v(?!1\.29\.0)' -g '!**/vendor/**' -Sgo/pkg/db/audit_log_target_insert.sql_generated.go (1)
3-3: Please align sqlc generator header to the pinned version (v1.29.0) across the repoThis file is stamped v1.28.0. For consistency and reproducible builds, regenerate with the pinned sqlc version and commit the results; also ensure CI/devcontainers install that exact version.
Suggested header change after regeneration:
// versions: -// sqlc v1.28.0 +// sqlc v1.29.0go/pkg/db/key_update_credits_decrement.sql_generated.go (1)
3-3: Standardize sqlc to v1.29.0Header shows v1.28.0. Please regenerate with v1.29.0 and pin the tool version to avoid mixed generations.
// versions: -// sqlc v1.28.0 +// sqlc v1.29.0go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
3-3: sqlc header version is v1.28.0; align to pinned v1.29.0Please regenerate with the pinned toolchain and commit. This avoids perpetual churn in generated artifacts.
// versions: -// sqlc v1.28.0 +// sqlc v1.29.0go/pkg/db/key_permission_insert.sql_generated.go (1)
3-3: Bump sqlc header to v1.29.0 for consistencyThis file shows v1.28.0. Please regenerate with the pinned version and enforce the pin in tooling.
// versions: -// sqlc v1.28.0 +// sqlc v1.29.0go/pkg/db/role_find_by_id.sql_generated.go (1)
3-3: Unify sqlc generator version across repo (this file is v1.28.0; others appear at v1.29.0).Generated headers should be consistent to ensure reproducible builds and avoid churn in future diffs. Please regenerate using a single pinned sqlc version (recommend v1.29.0 if that’s the current standard) and ensure CI/dev tooling is pinned accordingly. Do not hand-edit generated headers.
Run this script from the repo root to audit and enforce consistency:
#!/bin/bash set -euo pipefail PINNED_VERSION="${PINNED_VERSION:-v1.29.0}" echo "All sqlc version headers found (distinct):" rg -n --glob '!**/node_modules/**' -P '^//\s+sqlc v[0-9]+\.[0-9]+\.[0-9]+' -S | sed -E 's/^.*sqlc (v[0-9]+\.[0-9]+\.[0-9]+).*$/\1/' | sort -u echo echo "Files not on ${PINNED_VERSION} (should be empty after regen):" rg -n --glob '!**/node_modules/**' -P "^//\\s+sqlc (?!${PINNED_VERSION//./\\.})(v[0-9]+\\.[0-9]+\\.[0-9]+)" -S || true echo echo "Potential generator config/files referencing sqlc:" fd -t f -a -E node_modules -E .git 'sqlc.*' || true rg -n --glob '!**/node_modules/**' -i '\bsqlc\b' || true cat <<'EOF' Next steps: 1) Pin sqlc in your tooling (e.g., devcontainer, CI workflow, or go:generate script). 2) Re-generate: - sqlc generate 3) Re-run this script and commit regenerated files. You can override the expected version: PINNED_VERSION=v1.28.0 bash scripts/check-sqlc-versions.sh EOFgo/pkg/db/permission_insert.sql_generated.go (1)
3-3: sqlc header drift: v1.28.0 here vs. v1.29.0 elsewhere — regenerate for consistency.Keep all generated files on a single sqlc version to avoid noisy diffs and mismatched outputs. Re-run generation with the pinned version and commit results. See my repository-wide audit script in go/pkg/db/role_find_by_id.sql_generated.go.
go/pkg/db/keyring_update_key_encryption.sql_generated.go (1)
3-3: Align generator version (v1.28.0) with repo-wide standard.This header indicates v1.28.0. If the repo standard is v1.29.0 (as other files suggest), please regenerate with that version and pin your CI/dev environment accordingly.
go/pkg/db/permission_find_by_id_or_slug.sql_generated.go (1)
3-3: Mixed sqlc versions detected — normalize to a single pinned version.Please re-generate this file with the pinned sqlc version used elsewhere (likely v1.29.0) and ensure tooling is pinned to prevent future drift.
go/pkg/db/ratelimit_update.sql_generated.go (1)
3-3: Please regenerate with the repo’s pinned sqlc version.This file shows v1.28.0. For deterministic builds and clean diffs, keep all generated files on the same sqlc version (e.g., v1.29.0).
go/pkg/db/project_find_by_workspace_slug.sql_generated.go (1)
3-3: sqlc v1.28.0 header bump — OKQuery text and result scanning stable; previous performance considerations are already addressed by the existing UNIQUE(workspace_id, slug) constraint/index.
go/pkg/db/permission_list_direct_by_key_id.sql_generated.go (1)
3-3: Ensure repo-wide sqlc version consistency and pin in CIThis file shows sqlc v1.28.0. To avoid churn from mixed generator versions, please verify all generated files are on the same sqlc version and pin that version in CI/dev setup.
Run this script from repo root to detect any mixed headers and get a version tally:
#!/bin/bash set -euo pipefail echo "Distinct sqlc versions in generated files:" rg -nP --hidden --glob '!**/vendor/**' $'^//\\s*sqlc v\\d+\\.\\d+\\.\\d+\\b' | sed -nE 's@^.+//\\s*(sqlc v[0-9.]+).*@\\1@p' | sort -u echo echo "Files not on the most common version (if any):" # Find the mode version mode_ver="$(rg -nP --hidden --glob '!**/vendor/**' $'^//\\s*sqlc v\\d+\\.\\d+\\.\\d+\\b' \ | sed -nE 's@^.+//\\s*(sqlc v[0-9.]+).*@\\1@p' \ | sort | uniq -c | sort -nr | awk 'NR==1{print $2}')" echo "Mode version: ${mode_ver:-unknown}" rg -nP --hidden --glob '!**/vendor/**' $'^//\\s*sqlc v\\d+\\.\\d+\\.\\d+\\b' \ | awk -v mode="$mode_ver" ' { match($0, /sqlc v[0-9.]+/); ver=substr($0, RSTART, RLENGTH); if (ver != mode) print $0 }' || truego/pkg/db/key_soft_delete_by_id.sql_generated.go (1)
3-3: Repo-wide generator version alignmentMirror the consistency check across all generated files and pin sqlc in CI to avoid future drift.
Use the same script provided in the first comment to scan the repo.
go/pkg/db/ratelimit_override_update.sql_generated.go (1)
3-3: Pin sqlc version and verify consistencyPlease run the repo-wide header scan and pin the sqlc version in CI/dev tooling.
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
3-3: Consistency check for sqlc versionRun the provided script to ensure no mixed generator versions exist.
go/pkg/db/build_update_failed.sql_generated.go (1)
3-3: Standardize sqlc version across repoPlease run the repo-wide scan and pin sqlc in CI/dev tooling.
go/pkg/db/key_role_delete_many_by_key_id.sql_generated.go (1)
3-3: sqlc v1.28.0 header — approve; also re-run the repo-wide verificationThis mirrors the prior “version drift” feedback on generated headers; let’s ensure everything now uniformly reads v1.28.0.
Run:
#!/bin/bash set -euo pipefail echo "Distinct sqlc versions found:" rg -nP '^//\s+sqlc v\d+\.\d+\.\d+' -S | awk -F'v' '{print $NF}' | sort -u echo echo "Files not on v1.28.0:" rg -nP '^//\s+sqlc v(?!1\.28\.0)\d+\.\d+\.\d+' -nS || truego/pkg/db/permission_find_by_name_and_workspace_id.sql_generated.go (1)
3-3: sqlc header bump to v1.28.0: align repo-wide and pin generator version.Past reviews flagged mixed sqlc versions (v1.27.0 vs v1.29.0). Please decide on a single target (recommend pinning the latest validated version for this repo) and regenerate to avoid churn and dialect drift. Also ensure placeholder style remains
?consistently with your driver.Run from repo root:
#!/bin/bash set -euo pipefail echo "Distinct sqlc versions in generated files:" rg -n --glob '**/*sql_generated.go' -g '!**/vendor/**' -P '^//\s+sqlc v' \ | sed -E 's/.*(sqlc v[0-9.]+).*/\1/' | sort -u echo echo "Files not at chosen version (adjust 1\.28\.0 or 1\.29\.0 as needed):" rg -n --glob '**/*sql_generated.go' -g '!**/vendor/**' -P '^//\s+sqlc v(?!1\.(28|29)\.0)' echo echo "Placeholder scan (expect '?' for MySQL/SQLite):" rg -nP '\$\d+' go/pkg/db -g '*_sql_generated.go' || echo "OK: no \$n placeholders"go/pkg/db/permission_find_by_slug_and_workspace_id.sql_generated.go (1)
3-3: Normalize sqlc generator version across the repo and pin in CI/dev.Same note as other generated files: pick a single sqlc version (prefer the repo-standard) and pin it to prevent mixed headers. Verify placeholders remain
?.Use the same script provided in the sibling file comment to confirm consistency.
go/pkg/db/permission_list.sql_generated.go (1)
3-3: Standardize sqlc generator version.Same note as other generated files: pick and pin a single sqlc version; regenerate to keep headers uniform.
Use the shared script to confirm uniformity and placeholder style.
go/pkg/db/deployment_step_find_by_deployment_id.sql_generated.go (1)
3-3: Dup: ensure repo-wide sqlc version consistencySee my verification script on permission_list_by_role_id.sql_generated.go.
go/pkg/db/ratelimit_override_find_by_id.sql_generated.go (1)
3-3: Dup: verify all generated files match v1.28.0 and pin sqlcRefer to the repo-wide verification script posted earlier.
go/pkg/db/permission_find_by_id.sql_generated.go (1)
3-3: Dup: repo-wide sqlc version checkUse the previously shared script to confirm single-version codegen across the repo and pin sqlc in CI.
go/pkg/db/audit_log_find_target_by_id.sql_generated.go (1)
3-3: Dup: ensure consistent sqlc version and toolchain pinSame verification guidance applies; see earlier comment and script.
go/pkg/db/key_list_by_key_auth_id.sql_generated.go (1)
3-3: Repo-wide sqlc version consistency check recommended.
Similar to prior reviews about mixed sqlc versions, please verify and pin one version via sqlc.yaml. Re-run generation if discrepancies remain.Use the same script shared in my other comment to enumerate versions and flag outliers. Pass your intended target (e.g., 1.28.0) as the first argument to highlight mismatches.
go/pkg/db/identity_delete.sql_generated.go (1)
3-3: Ensure a single pinned sqlc version across generated files.
Please confirm no stragglers remain on older/newer versions and pin via sqlc.yaml.Reuse the repo scan script from my earlier comment to validate.
go/pkg/db/identity_update.sql_generated.go (1)
3-3: Pin the sqlc tool version; verify consistency.
Keep generated artifacts aligned by enforcing a single sqlc version in the repo.Please run the previously provided script to confirm uniformity.
go/pkg/db/build_find_latest_by_deployment_id.sql_generated.go (1)
12-12: Non-blocking naming note (generated): prefer ID over Id in identifiers.
The symbol FindLatestBuildByDeploymentId uses “Id” instead of “ID”. Since this is generated, not actionable here; consider a thin hand-written wrapper alias if external API ergonomics matter.go/pkg/db/identity_list.sql_generated.go (1)
3-3: Ensure a single sqlc version across the repoPrevious review threads flagged mixed sqlc versions. Please verify the entire repo uses one version (ideally pinned), and regenerate any stragglers.
Run this script from repo root to confirm only one sqlc version is present in all generated DB files and to list any outliers:
#!/usr/bin/env bash set -euo pipefail echo "Unique sqlc versions found in go/pkg/db/*_generated.go:" rg -nP '^//\s+sqlc v' go/pkg/db -g '*_generated.go' | awk '{print $NF}' | sort | uniq -c echo echo "Outliers (if more than one version exists):" versions=$(rg -nP '^//\s+sqlc v' go/pkg/db -g '*_generated.go' | awk '{print $NF}' | sort | uniq | wc -l) if [ "$versions" -gt 1 ]; then rg -nP '^//\s+sqlc v' go/pkg/db -g '*_generated.go' | sort exit 1 else echo "OK: single sqlc version detected." fiOptionally pin the version to avoid drift (example):
- sqlc.yaml
version: "2" sql: - engine: "mysql" schema: "go/pkg/db/schema" queries: "go/pkg/db/queries" gen: go: package: "db" out: "go/pkg/db"
- Tooling (Makefile)
SQLC_VERSION ?= 1.28.0 sqlc-generate: docker run --rm -v $$PWD:/src -w /src sqlc/sqlc:v$(SQLC_VERSION) generatego/pkg/db/key_permission_delete_by_key_and_permission_id.sql_generated.go (1)
3-3: Follow-up: unify sqlc version across generated filesMirror of earlier request to avoid mixed generator versions across the codebase; please run the repo-wide check and pin in CI if needed.
go/pkg/db/key_insert.sql_generated.go (1)
3-3: Keep sqlc version consistent across repoSame consistency check as noted earlier to avoid multiple sqlc versions in generated files.
go/pkg/db/identity_insert_ratelimit.sql_generated.go (1)
3-3: Repo-wide sqlc version consistencyReiterating the consistency check request to ensure all generated headers match a single version.
go/pkg/db/api_soft_delete.sql_generated.go (1)
3-3: Unify sqlc version across generated filesPlease ensure there isn’t a mixture of sqlc versions across
go/pkg/dband other generated packages.go/pkg/db/permission_list_by_key_id.sql_generated.go (1)
3-3: Verify sqlc version consistency across the repoEnsure all generated files are on the same sqlc version to avoid churn in future regenerations and mixed diffs. The previous discussion in this repo flagged mixed versions; let’s re-check and standardize on v1.28.0 (or intentionally bump all to v1.29.0 if that’s the plan).
Run from repo root:
#!/bin/bash set -euo pipefail # Show distinct sqlc versions across generated files echo "Distinct sqlc versions found:" rg -nP '^//\s+sqlc v[0-9.]+' go/pkg/db | awk -F'v' '{print $NF}' | sort -u # List any files not matching v1.28.0 (adjust if targeting a different version) echo echo "Files NOT at sqlc v1.28.0 (should be empty if consistent):" rg -nP '^//\s+sqlc v(?!1\.28\.0\b).*' go/pkg/db -H || true # Optional: ensure sqlc.yaml (or generation scripts) pins the intended version fd -a sqlc.yaml | xargs -I{} sh -c 'echo "--- {};"; cat "{}"'Expected: only v1.28.0 appears; the “NOT at v1.28.0” section is empty. If not, please regenerate those files or align the configured version in CI/tooling.
go/pkg/db/key_permission_delete_many_by_permission_id.sql_generated.go (1)
3-3: LGTM: header bump; reconcile earlier thread expecting v1.29.0This file targets sqlc v1.28.0. A prior review thread in this repo referenced v1.29.0 and highlighted mixed versions. Please confirm the intended standard (v1.28.0 vs v1.29.0) and resolve that thread to avoid confusion. If v1.28.0 is the target, ensure remaining files are regenerated to match and CI pins the same.
go/pkg/db/queries/deployment_find_by_id.sql (1)
11-16: Add unit hint for git_commit_timestamp and verify migrations exist
- Nit: add the same “Unix epoch milliseconds” comment used in InsertDeployment for parity and to prevent unit confusion down the stack.
- Follow-up: ensure there is a forward SQL migration that adds these six nullable columns to the existing deployments table in production.
Apply this diff to annotate the unit:
git_branch, - git_commit_message, + git_commit_message, git_commit_author_name, git_commit_author_email, git_commit_author_username, git_commit_author_avatar_url, - git_commit_timestamp, + git_commit_timestamp, -- Unix epoch milliseconds config_snapshot,Run to verify the migration(s) are present and correct:
#!/bin/bash set -euo pipefail echo "== Find migrations that alter `deployments` ==" rg -nP -C2 '(?i)alter\s+table\s+`?deployments`?.*(add\s+column|add\s+)' -- **/*migrations*/*.sql || true echo echo "== Check each new column appears in migrations with expected types ==" rg -nP '(?i)git_commit_message\s+text|git_commit_author_name\s+varchar|git_commit_author_email\s+varchar|git_commit_author_username\s+varchar|git_commit_author_avatar_url\s+varchar|git_commit_timestamp\s+bigint' -- **/*migrations*/*.sql || true echo echo "== Sanity: matching columns in schema.sql (post-regeneration) ==" rg -n 'git_commit_(message|author_name|author_email|author_username|author_avatar_url|timestamp)' go/pkg/db/schema.sql || truego/pkg/db/deployment_find_by_id.sql_generated.go (2)
73-79: row.Scan adds six nullable git_ fields — verify Deployment uses sql.NullString/NullInt64.*To avoid empty-string bleed-through, these should be sql.NullString (message/author*) and sql.NullInt64 (timestamp). Past checks indicated that’s already true.
22-27: SELECT extended with six git_ columns — scan/order parity looks correct.*Column order matches the Scan order below. Ensure the migration added these columns with expected nullability and widths. If recent-deploy lists will sort/filter by commit time, consider a composite index including git_commit_timestamp.
go/pkg/db/bulk_deployment_insert.sql.go (1)
40-46: Args ordering matches column list — keep tests to detect drift.Please keep/extend the unit test that asserts placeholder count and arg packing order for bulk insert.
go/apps/ctrl/services/deployment/get_version.go (1)
61-63: Remove author email from public response (PII exposure).Author email isn’t required by acceptance criteria and increases privacy/compliance risk if exposed broadly. Omit for now; reintroduce later with explicit permission checks if needed.
Apply:
- if deployment.GitCommitAuthorEmail.Valid { - protoVersion.GitCommitAuthorEmail = deployment.GitCommitAuthorEmail.String - }go/apps/ctrl/services/deployment/create_version.go (3)
116-116: Prefer leaving UpdatedAt NULL on creation.Avoid conflating created vs updated. Initialize UpdatedAt as NULL; set on first real update.
- UpdatedAt: sql.NullInt64{Int64: now, Valid: true}, + UpdatedAt: sql.NullInt64{Valid: false},
17-24: Rune-safe truncation helper is good. Add an AIDEV note.Document why we bound sizes (DB width, UI). Helps future maintainers.
-// limitString truncates a string to the specified maximum number of runes +// limitString truncates a string to the specified maximum number of runes. +// AIDEV-NOTE: Defensive bound to respect DB varchar/text widths and prevent oversized rows.
71-86: Consider normalizing seconds→ms instead of hard-rejecting valid seconds.Upstreams often send seconds. Auto-normalize when clearly seconds; keep future-time guard.
- // Validate git commit timestamp if provided (must be Unix epoch milliseconds) + // Validate/normalize git commit timestamp if provided (store as Unix epoch ms) if req.Msg.GetGitCommitTimestamp() != 0 { - timestamp := req.Msg.GetGitCommitTimestamp() - // Reject timestamps that are clearly in seconds format (< 1_000_000_000_000) - // This corresponds to January 1, 2001 in milliseconds - if timestamp < 1_000_000_000_000 { - return nil, connect.NewError(connect.CodeInvalidArgument, - fmt.Errorf("git_commit_timestamp must be Unix epoch milliseconds, got %d (appears to be seconds format)", timestamp)) - } + timestamp := req.Msg.GetGitCommitTimestamp() + // Normalize seconds→ms if value looks like seconds. + if timestamp > 0 && timestamp < 1_000_000_000_000 { + timestamp = timestamp * 1000 + } // Also reject future timestamps more than 1 hour ahead (likely invalid) maxValidTimestamp := time.Now().Add(1 * time.Hour).UnixMilli() if timestamp > maxValidTimestamp { return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("git_commit_timestamp %d is too far in the future (must be Unix epoch milliseconds)", timestamp)) } + // Reassign normalized value back into the request-derived local + // for downstream InsertDeploymentParams. + // (Or keep a local variable and use it directly in params below.) + // AIDEV-NOTE: Timestamp stored in ms consistently. + // (follow-up: add a unit test for seconds input) }go/pkg/db/deployment_insert.sql_generated.go (2)
24-30: INSERT includes git_commit_timestamp with millis comment — good clarity.Comment documents epoch ms; consistent with service validation. Keep proto doc aligned.
68-79: Initialism nit: AvatarUrl → AvatarURL (optional).Generated field names use Url. If you want to adopt Go initialism conventions, map via sqlc rename rules; otherwise consistency across the codebase is acceptable.
go/pkg/db/models_generated.go (1)
655-674: Git commit metadata fields added correctly; millis semantics already documented in proto.The six new nullable fields on Deployment align with the PR objectives and types are appropriate (
sql.NullString/sql.NullInt64). Also saw the proto comments markingGitCommitTimestampas Unix epoch milliseconds, which addresses the earlier ask to document units. No code changes needed here.
mcstepp
force-pushed
the
ENG-1992-rich-git-commit-data
branch
from
5c1df60 to
35805e7
Compare
… email as lookup for github username and avatar if they have a github repo and github account
mcstepp
force-pushed
the
ENG-1992-rich-git-commit-data
branch
from
eb5225b to
69247f4
Compare
|
@perkinsjr @Flo4604 we need a 2nd approval |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/28/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
Fixes #3791
Database Schema Changes
Added the following fields to the deployments table:
Protobuf API Updates
Updated both CreateVersionRequest and Version messages in
/go/proto/ctrl/v1/version.proto to include:
SQL Query Updates
Modified SQL queries in /go/pkg/db/queries/:
Service Implementation Updates
Enhanced both services in /go/apps/ctrl/services/deployment/:
Key Benefits
details and timestamps
integration - ACTUAL FETCHING FROM GITHUB API TODO
deployment history
functionality
Type of change
How should this be tested?
new git fields populated
correctly
response
properly
stored git data
encoding correctly
Unit tests included:
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Tests
Chores