Conversation
|
📝 WalkthroughWalkthroughAdds a new POST /v2/keys.rerollKey endpoint (OpenAPI, route, handler), DB/SQL expansions and KeyData aggregation for richer key context, Vault-backed recoverable key support in seeding, audit log event "key.reroll", tests for success and error paths, and defensive JSON decoding for key relations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Auth
participant DB
participant KeysSvc
participant Vault
participant Auditlogs
Client->>API: POST /v2/keys.rerollKey { keyId, expiration }
API->>Auth: validate root key & permissions
Auth-->>API: OK
API->>DB: FindLiveKeyByID(keyId)
DB-->>API: key + roles/permissions/ratelimits + key_auth/workspace/identity
API->>KeysSvc: generate new key material (prefix, bytes)
KeysSvc-->>API: new key
alt encryption needed
API->>Vault: Encrypt(new key, workspace keyring)
Vault-->>API: ciphertext, encryptionKeyID
end
API->>DB: Tx insert new key (+encryption, relations), update old key Expires
DB-->>API: Tx commit
API->>Auditlogs: Insert key.reroll event
Auditlogs-->>API: logged
API-->>Client: 200 { data: { keyId, key }, meta }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Flo4604
requested review from
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
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: 51
🔭 Outside diff range comments (8)
apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts (1)
61-65: Fix stray brace in error log template string.There’s an extra closing brace in the template literal which will print a malformed log line.
Apply this diff:
- )}\n Output ${(error as Error).message}}`, + )}\n Output ${(error as Error).message}`,internal/schema/src/auditlog.ts (1)
59-99: Tighten event validation to use the enumCurrently
auditLogSchemaV1.eventisz.string(). Consider referencingunkeyAuditLogEventshere to reject invalid event names at the schema boundary.Suggested change (outside the shown diff):
// Replace event: z.string(), // With event: unkeyAuditLogEvents,go/apps/api/routes/register.go (1)
56-58: Use GoDoc-style comment for exported RegisterMinor doc nit: exported functions should have comments that start with the function name for proper GoDoc.
Apply:
-// here we register all of the routes. -// this function runs during startup. +// Register registers all API routes and runs during server startup.go/internal/services/keys/get.go (2)
121-152: Good defensive nil handling; consider DRYing and accepting more JSON field typesThe three unmarshal blocks are nearly identical and assume []byte. Some drivers may yield json.RawMessage or string. Suggest extracting a small helper to coerce to []byte and reuse for roles, permissions, and ratelimits.
Apply within this range:
- // Safely handle roles field - rolesBytes, ok := key.Roles.([]byte) - if !ok || rolesBytes == nil { - roles = []string{} // Default to empty array if nil or wrong type - } else { - err = json.Unmarshal(rolesBytes, &roles) - if err != nil { - return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal roles")) - } - } + // Safely handle roles field + if rolesBytes, ok := jsonBytesOf(key.Roles); !ok || len(rolesBytes) == 0 { + roles = []string{} + } else if err = json.Unmarshal(rolesBytes, &roles); err != nil { + return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal roles")) + } - // Safely handle permissions field - permissionsBytes, ok := key.Permissions.([]byte) - if !ok || permissionsBytes == nil { - permissions = []string{} // Default to empty array if nil or wrong type - } else { - err = json.Unmarshal(permissionsBytes, &permissions) - if err != nil { - return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal permissions")) - } - } + // Safely handle permissions field + if permissionsBytes, ok := jsonBytesOf(key.Permissions); !ok || len(permissionsBytes) == 0 { + permissions = []string{} + } else if err = json.Unmarshal(permissionsBytes, &permissions); err != nil { + return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal permissions")) + } - // Safely handle ratelimits field - ratelimitsBytes, ok := key.Ratelimits.([]byte) - if !ok || ratelimitsBytes == nil { - ratelimitArr = []db.KeyFindForVerificationRatelimit{} // Default to empty array if nil or wrong type - } else { - err = json.Unmarshal(ratelimitsBytes, &ratelimitArr) - if err != nil { - return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal ratelimits")) - } - } + // Safely handle ratelimits field + if ratelimitsBytes, ok := jsonBytesOf(key.Ratelimits); !ok || len(ratelimitsBytes) == 0 { + ratelimitArr = []db.KeyFindForVerificationRatelimit{} + } else if err = json.Unmarshal(ratelimitsBytes, &ratelimitArr); err != nil { + return nil, emptyLog, fault.Wrap(err, fault.Internal("failed to unmarshal ratelimits")) + }Add this helper in the same package (outside the selected lines):
// jsonBytesOf normalizes various JSON field representations into a byte slice. func jsonBytesOf(v any) ([]byte, bool) { switch x := v.(type) { case []byte: return x, true case json.RawMessage: return []byte(x), true case string: return []byte(x), true default: return nil, false } }
156-161: Micro-optimization: pre-size the mapSmall perf win for large ratelimit arrays.
Apply:
- ratelimitConfigs := make(map[string]db.KeyFindForVerificationRatelimit) + ratelimitConfigs := make(map[string]db.KeyFindForVerificationRatelimit, len(ratelimitArr))go/pkg/testutil/http.go (3)
294-297: Fix JSON unmarshal bug: passing a double pointer prevents populating response.
json.Unmarshalcurrently receives&bodywherebodyis already a pointer. This results in a**Bodyand the target never gets populated.Apply this diff:
func UnmarshalBody[Body any](t *testing.T, r *httptest.ResponseRecorder, body *Body) { - err := json.Unmarshal(r.Body.Bytes(), &body) + err := json.Unmarshal(r.Body.Bytes(), body) require.NoError(t, err) }
64-65: Use Go naming conventions for initialisms: redisURL.Prefer
URLoverUrlto match Go conventions and improve consistency with struct fieldRedisURL.Apply this diff:
-redisUrl := containers.Redis(t) +redisURL := containers.Redis(t)ctr, err := counter.NewRedis(counter.RedisConfig{ - RedisURL: redisUrl, + RedisURL: redisURL, Logger: logger, })Also applies to: 101-105
31-50: Add doc comments to exported types and functions.Per repo Go documentation guidelines, exported identifiers require comments. Please add brief comments for
Harness,NewHarness,CallRaw, andCallRoute.You can add comments like:
// Harness provides an in-process HTTP test server and supporting services // (DB, caches, rate limiting, ClickHouse, Vault, seeding utilities) for route tests. type Harness struct { ... } // NewHarness constructs a fully-initialized test Harness with seeded data. func NewHarness(t *testing.T) *Harness { ... } // CallRaw executes the given HTTP request against the Harness server and returns a raw response wrapper. func CallRaw[Res any](h *Harness, req *http.Request) testutil.TestResponse[Res] { ... } // CallRoute encodes the request as JSON, invokes the route, and decodes the JSON response into Res. func CallRoute[Req any, Res any](h *Harness, route zen.Route, headers http.Header, req Req) testutil.TestResponse[Res] { ... }
♻️ Duplicate comments (1)
go/pkg/db/querier_generated.go (1)
479-494: Duplicate:auto_applynumeric vs boolean in FindLiveKeyByID.Same fix as above required here.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (29)
apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts(1 hunks)go/apps/api/integration/harness.go(1 hunks)go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/openapi-split.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml(1 hunks)go/apps/api/routes/register.go(2 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(1 hunks)go/internal/services/keys/get.go(4 hunks)go/internal/services/keys/get_test.go(1 hunks)go/pkg/auditlog/events.go(1 hunks)go/pkg/db/custom_types.go(1 hunks)go/pkg/db/key_data.go(1 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_find_live_by_hash.sql(1 hunks)go/pkg/db/queries/key_find_live_by_id.sql(1 hunks)go/pkg/testutil/http.go(1 hunks)go/pkg/testutil/seed/seed.go(4 hunks)internal/schema/src/auditlog.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/internal/services/keys/get_test.gogo/pkg/auditlog/events.gogo/pkg/db/custom_types.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_keys_reroll_key/403_test.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/pkg/db/key_data.gogo/pkg/testutil/http.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/internal/services/keys/get.gogo/apps/api/openapi/gen.gogo/pkg/testutil/seed/seed.gogo/apps/api/integration/harness.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/querier_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/internal/services/keys/get_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_reroll_key/403_test.gogo/apps/api/routes/v2_keys_reroll_key/200_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/internal/services/keys/get_test.gogo/pkg/auditlog/events.gogo/pkg/db/custom_types.gointernal/schema/src/auditlog.tsgo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/register.gogo/apps/api/routes/v2_keys_reroll_key/403_test.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/pkg/db/key_data.goapps/dashboard/lib/trpc/routers/audit/llm-search/utils.tsgo/pkg/testutil/http.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/internal/services/keys/get.gogo/apps/api/openapi/gen.gogo/pkg/testutil/seed/seed.gogo/apps/api/integration/harness.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.gogo/pkg/db/querier_generated.go
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
internal/schema/src/auditlog.tsapps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
internal/schema/src/auditlog.tsapps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts
🧬 Code Graph Analysis (15)
go/pkg/db/custom_types.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/apps/api/routes/v2_keys_reroll_key/401_test.go (4)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/pkg/testutil/seed/seed.go (3)
CreateApiRequest(83-91)CreateKeyRequest(187-206)New(39-46)go/apps/api/openapi/gen.go (1)
UnauthorizedErrorResponse(450-456)
go/apps/api/routes/v2_keys_reroll_key/400_test.go (5)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/pkg/testutil/seed/seed.go (2)
Resources(23-28)New(39-46)go/apps/api/openapi/gen.go (1)
BadRequestErrorResponse(59-65)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)
go/apps/api/routes/v2_keys_reroll_key/404_test.go (7)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/apps/api/routes/register.go (1)
Register(58-554)go/pkg/testutil/seed/seed.go (2)
Resources(23-28)New(39-46)go/apps/api/integration/harness.go (1)
New(53-111)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)go/apps/api/openapi/gen.go (1)
NotFoundErrorResponse(270-276)
go/apps/api/routes/register.go (1)
go/apps/api/routes/v2_keys_reroll_key/handler.go (1)
Handler(32-38)
go/apps/api/routes/v2_keys_reroll_key/403_test.go (6)
go/pkg/rbac/query.go (1)
T(84-90)go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/pkg/testutil/seed/seed.go (3)
CreateApiRequest(83-91)Resources(23-28)CreateKeyRequest(187-206)go/pkg/rbac/permissions.go (1)
CreateKey(49-49)go/apps/api/openapi/gen.go (2)
ForbiddenErrorResponse(132-138)Meta(259-262)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (7)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (3)
Handler(32-38)Request(28-28)Response(29-29)go/apps/api/routes/register.go (1)
Register(58-554)go/pkg/testutil/seed/seed.go (7)
Resources(23-28)CreateApiRequest(83-91)CreateIdentityRequest(349-354)CreateRatelimitRequest(305-313)CreateKeyRequest(187-206)CreatePermissionRequest(421-426)CreateRoleRequest(384-390)go/apps/api/openapi/gen.go (1)
Meta(259-262)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (1)
ToKeyData(33-35)
go/pkg/db/key_data.go (3)
go/pkg/db/custom_types.go (3)
RoleInfo(10-14)PermissionInfo(16-21)RatelimitInfo(23-31)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(103-139)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(104-140)
go/pkg/testutil/http.go (2)
go/apps/api/integration/harness.go (1)
New(53-111)go/pkg/testutil/seed/seed.go (1)
New(39-46)
go/apps/api/openapi/gen.go (2)
go/pkg/codes/unkey_data.go (1)
Data(93-136)go/pkg/db/models_generated.go (1)
Key(725-749)
go/pkg/testutil/seed/seed.go (4)
apps/agent/services/vault/service.go (2)
Service(20-30)New(39-74)go/pkg/db/database.go (1)
New(63-102)go/pkg/db/interface.go (1)
Database(10-20)go/pkg/db/key_encryption_insert.sql_generated.go (1)
InsertKeyEncryptionParams(18-24)
go/apps/api/integration/harness.go (1)
go/pkg/testutil/seed/seed.go (1)
New(39-46)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/pkg/db/key_find_live_by_id.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(103-139)
🪛 Gitleaks (8.27.2)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseBody.yaml
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 YAMLlint (1.37.1)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyRequestBody.yaml
[error] 27-27: trailing spaces
(trailing-spaces)
🪛 golangci-lint (2.2.2)
go/pkg/db/key_data.go
55-55: db.KeyData is missing fields Identity, Roles, Permissions, RolePermissions, Ratelimits
(exhaustruct)
72-72: db.Identity is missing fields Environment, Deleted, CreatedAt, UpdatedAt
(exhaustruct)
⏰ 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). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (35)
apps/dashboard/lib/trpc/routers/audit/llm-search/utils.ts (1)
218-218: LGTM: Added key.reroll mapping to the system prompt.Good catch including key.reroll in the generic-term mapping list; this keeps the LLM aligned with new audit events.
go/pkg/db/custom_types.go (1)
10-31: Verified — NullString has JSON methods; Ratelimit.Limit int32 matches DB (no change required)
- go/pkg/db/types/null_string.go — implements MarshalJSON / UnmarshalJSON.
- go/pkg/db/custom_types.go — RatelimitInfo.Limit is int32.
- go/pkg/db/models_generated.go and sqlc-generated structs — Limit is int32 in generated models/params.
- go/pkg/db/schema.sql —
limitis declared asint NOT NULL(matches int32).No code changes required for the two concerns above.
go/pkg/db/queries/key_find_live_by_hash.sql (2)
3-9: Embedding KeyAuth/Workspace and identity metadata looks goodThe additional embeds and identity fields expand the row shape consistently with the generated Go struct and downstream needs.
79-83: Confirmed: apis.key_auth_id is UNIQUE — no action requiredrg output shows a UNIQUE constraint on apis.key_auth_id, so the JOIN enforces 1:1 and :one semantics are safe.
- go/pkg/db/schema.sql —
CONSTRAINT apis_key_auth_id_unique UNIQUE(key_auth_id)(around line 16)- internal/db/drizzle/0000_broken_blockbuster.sql —
CONSTRAINT apis_key_auth_id_unique UNIQUE(key_auth_id)(around line 13)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
267-302: Scan order and struct embeds align with the expanded SELECTThe scan sequence correctly matches the SELECT list, including KeyAuth and Workspace embeds and the new identity fields and JSON aggregates.
go/internal/services/keys/get_test.go (1)
28-28: LGTM — non-functional whitespace changeNo behavioral impact. Tests remain clear and organized.
internal/schema/src/auditlog.ts (1)
15-15: Add of "key.reroll" event is consistent with the new endpoint and back-end event namesAligned with go/pkg/auditlog/events.go.
go/apps/api/routes/register.go (2)
45-45: Route import wiring looks correctThe import for v2_keys_reroll_key is consistent with existing route imports and matches the constructed handler type.
389-399: Reroll route registered with correct dependenciesThe handler is wired with Logger, DB, Keys, Auditlogs, and Vault under default middlewares, consistent with create/get routes. Placement next to createKey is sensible.
One thing to double-check: if the reroll operation writes audit logs with the new KeyRerollEvent, ensure the handler actually emits that event (the registration provides Auditlogs correctly).
Would you confirm the handler uses auditlog.KeyRerollEvent on success?
go/pkg/auditlog/events.go (1)
21-21: New audit event added appropriatelykey.reroll fits the existing event naming scheme and grouping under Key events. No issues.
go/apps/api/routes/v2_keys_reroll_key/404_test.go (1)
29-34: Verify permission used matches rerollKey requirementsThe test authenticates with "api..create_key". If reroll requires a distinct permission (e.g., "api..reroll_key" or "api.*.update_key"), this could mask 403 vs 404 behavior.
If rerollKey checks a different permission, update the harness to issue the appropriate root key permission.
go/pkg/testutil/http.go (1)
145-145: Seeder now receives Vault service (good alignment with new signature).Passing
vintoseed.Newaligns with the updated constructor and enables seeding recoverable/encrypted keys in tests.go/apps/api/routes/v2_keys_reroll_key/400_test.go (1)
46-54: LGTM: empty keyId should trigger validation failure.This case is appropriate for 400 validation of
keyId.go/apps/api/integration/harness.go (1)
97-99: Order of operations is fine.Seeding occurs before the API nodes are started. This avoids boot-time data races and flakiness.
go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
33-44: Good: Encrypted keyring path seeded for realistic reroll.Seeding with
EncryptedKeys: trueand aRecoverablekey is a solid e2e coverage for the Vault-backed path.go/pkg/testutil/seed/seed.go (5)
12-12: Good: Adding Vault proto import enables recoverable-key seeding.Import is correct and scoped.
34-35: Injecting Vault into the seeder improves test realism.Carrying
Vault *vault.Servicein the seeder is a solid design for opt-in encryption paths.
39-45: Constructor signature change: ensure all call sites are updated.
New(t, database, vault)is a breaking change for tests constructing the seeder. Make sure all usages (harness, helpers) pass a Vault (or nil).I can run a repo-wide search to flag any stale invocations if needed.
198-199: Recoverable flag placement looks right.Adding
Recoverableto the CreateKeyRequest gives precise control during seeding.
254-271: Keep workspace ID as the Vault keyring — do not switch to KeyAuthIDVault keyring is used as the workspace identifier across the codebase (Encrypt/Decrypt calls pass workspace IDs), so using req.WorkspaceID is correct.
- go/pkg/testutil/seed/seed.go — CreateKey: Vault.Encrypt uses Keyring: req.WorkspaceID (around line 255)
- go/apps/api/routes/v2_keys_create_key/handler.go — Vault.Encrypt uses Keyring: s.AuthorizedWorkspaceID() (around line 167)
- go/apps/api/routes/v2_keys_reroll_key/handler.go — Vault.Encrypt uses Keyring: s.AuthorizedWorkspaceID() (around line 161)
- go/apps/api/routes/v2_keys_get_key/handler.go — Vault.Decrypt uses Keyring: key.WorkspaceID (around line 161)
- Tests also call Vault.Encrypt with workspace IDs (e.g. v2_keys_whoami/200_test.go, v2_keys_get_key/200_test.go)
Remove the suggested diff — leave Keyring: req.WorkspaceID as-is.
Likely an incorrect or invalid review comment.
go/pkg/db/key_find_live_by_id.sql_generated.go (4)
13-24: Joins and projections look correct and completeThe expanded SELECT adds KeyAuth/Workspace joins and identity/encryption columns with proper nullability. Filtering out deleted rows for keys/apis/key_auth is correct.
104-140: Struct shape matches the SELECT orderFindLiveKeyByIDRow’s new fields (KeyAuth, Workspace, identity*, encrypted*, and JSON collections) align with the query. IdentityMeta as []byte is appropriate for JSON blobs.
233-304: Scan order aligns; keep an eye on JSON field typesThe Scan sequence corresponds to the SELECT order. Again, confirm ToKeyData expects []byte for JSON fields.
Same verification as above applies here. No code change needed.
25-91: OK — DB JSON aggregates are handled: ToKeyData unmarshals []byte into typed fieldsVerified: the generated query rows use interface{} for roles/permissions/role_permissions/ratelimits and ToKeyData performs []byte type assertions and json.Unmarshal into the KeyData fields (errors ignored), so the MySQL driver returning []byte is handled.
Files inspected:
- go/pkg/db/key_data.go — buildKeyData: checks r.Roles/.Permissions/.RolePermissions/.Ratelimits as []byte and calls json.Unmarshal into kd.Roles, kd.Permissions, kd.RolePermissions, kd.Ratelimits.
- go/internal/services/keys/get.go — also type-asserts []byte and json.Unmarshal for roles/permissions/ratelimits.
- Generated rows (interface{} fields): go/pkg/db/key_find_live_by_id.sql_generated.go, go/pkg/db/key_find_live_by_hash.sql_generated.go, go/pkg/db/key_find_for_verification.sql_generated.go.
Conclusion: no change required here — handling is correct.
go/apps/api/openapi/openapi-generated.yaml (2)
929-969: Request schema: clarify parameter naming in docs (no code change here)The field name remaining communicates “grace period ms,” but the linked issue used “expiration” as a relative TTL. The description here is clear, so this is mostly a consistency note: ensure external docs and handler use the same terminology to avoid confusion.
If “remaining” is the final API name, please update any user-facing docs or issue acceptance criteria that still reference “expiration”.
4411-4416: Path naming consistency with acceptance criteriaThe issue text references /v2/keys/rollKey while the API uses /v2/keys.rerollKey (dot-style). Assuming the dot-style is canonical for v2, just confirm downstream clients and docs reflect this path to avoid integration friction.
go/apps/api/routes/v2_keys_reroll_key/handler.go (7)
66-80: Not-found and DB error mapping is solidClear 404 mapping when key doesn’t exist and service-unavailable wrapping on DB errors. Good.
84-115: Require update_key permission in addition to create_keyReroll creates a key and updates the old key’s expiry. Only checking create_key risks allowing rotations that shorten old key lifetime without explicit update permission.
[Suggest essential refactor]- checks := rbac.Or( - rbac.T(rbac.Tuple{ - ResourceType: rbac.Api, - ResourceID: key.Api.ID, - Action: rbac.CreateKey, - }), - rbac.T(rbac.Tuple{ - ResourceType: rbac.Api, - ResourceID: "*", - Action: rbac.CreateKey, - }), - ) + createChecks := rbac.Or( + rbac.T(rbac.Tuple{ResourceType: rbac.Api, ResourceID: key.Api.ID, Action: rbac.CreateKey}), + rbac.T(rbac.Tuple{ResourceType: rbac.Api, ResourceID: "*", Action: rbac.CreateKey}), + ) + updateChecks := rbac.Or( + rbac.T(rbac.Tuple{ResourceType: rbac.Api, ResourceID: key.Api.ID, Action: rbac.UpdateKey}), + rbac.T(rbac.Tuple{ResourceType: rbac.Api, ResourceID: "*", Action: rbac.UpdateKey}), + ) + checks := rbac.And(createChecks, updateChecks)
120-135: Prefix/length derivation is reasonableExtracting prefix from Start and falling back to KeyAuth defaults makes sense. No issues spotted.
218-248: Ratelimit copy logic is correct (skips identity-level limits)Good call to only duplicate key-level rate limits. Bulk insert usage is appropriate.
250-289: Role and permission cloning is correctDirect roles/permissions are preserved via bulk inserts. Looks good.
309-343: Audit log coverage is thoroughCaptures actor, new/old key resources, and API resource. Display message is clear. Nice.
356-364: Response payload is correctReturns new keyId + key with requestId meta. Matches OpenAPI and requirements.
go/pkg/db/queries/key_find_live_by_id.sql (2)
3-12: Embeds and identity/encryption fields are correctsqlc.embed for apis/key_auth/workspaces and explicit identity/encryption projections look good. Nullability is handled appropriately.
80-90: Join filters are appropriateFiltering out deleted entries for keys/apis/key_auth reduces stale data surface. If roles/permissions can be soft-deleted, consider adding similar filters in subqueries.
Do roles and permissions have soft-delete flags? If so, we should exclude deleted entries in the subqueries.
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyRequestBody.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
go/pkg/db/custom_types.go (3)
10-14: Add GoDoc for exported type RoleInfo (required by guidelines).Per repository guidelines, all exported types must have documentation comments.
Apply this diff:
+// RoleInfo represents a role row decoded from JSON produced by DB queries. type RoleInfo struct { ID string `json:"id"` Name string `json:"name"` Description dbtype.NullString `json:"description"` }
16-21: Add GoDoc for exported type PermissionInfo (required by guidelines).Per repository guidelines, all exported types must have documentation comments.
Apply this diff:
+// PermissionInfo represents a permission row decoded from JSON produced by DB queries. type PermissionInfo struct { ID string `json:"id"` Name string `json:"name"` Slug string `json:"slug"` Description dbtype.NullString `json:"description"` }
23-31: Add GoDoc for exported type RatelimitInfo (required by guidelines).Per repository guidelines, all exported types must have documentation comments.
Apply this diff:
+// RatelimitInfo represents a ratelimit row decoded from JSON produced by DB queries. type RatelimitInfo struct { ID string `json:"id"` Name string `json:"name"` KeyID dbtype.NullString `json:"key_id"` IdentityID dbtype.NullString `json:"identity_id"` Limit int32 `json:"limit"` Duration int64 `json:"duration"` AutoApply bool `json:"auto_apply"` }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/pkg/db/custom_types.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/custom_types.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/custom_types.go
🧬 Code Graph Analysis (1)
go/pkg/db/custom_types.go (2)
go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/rbac/permissions.go (1)
Limit(74-74)
⏰ 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). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
go/pkg/db/custom_types.go (2)
7-8: File-level comment polished — nice cleanup.The comment is now clear, concise, and accurately describes the purpose of these types.
3-5: dbtype.NullString implements JSON (un)marshaling — no action required.Confirmed both methods are defined in go/pkg/db/types/null_string.go:
- type NullString sql.NullString (go/pkg/db/types/null_string.go:10)
- func (ns *NullString) MarshalJSON() ([]byte, error) (go/pkg/db/types/null_string.go:13)
- func (ns *NullString) UnmarshalJSON(data []byte) error (go/pkg/db/types/null_string.go:22)
imeyer
left a comment
imeyer
left a comment
There was a problem hiding this comment.
One major issue in key_data.go, other than that looks good.
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
go/pkg/testutil/seed/seed.go (2)
428-432: Validation checks use wrong fields; fix Name and Slug assertions.Currently checks WorkspaceID three times. Ensure Name and Slug are validated.
- require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission WorkspaceID must be set")) - require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission Name must be set")) - require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission Slug must be set")) + require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission WorkspaceID must be set")) + require.NoError(s.t, assert.NotEmpty(req.Name, "Permission Name must be set")) + require.NoError(s.t, assert.NotEmpty(req.Slug, "Permission Slug must be set"))
396-398: Use the role prefix for role IDs — replace PermissionPrefix with RolePrefixRole IDs should use the role-specific prefix ("role"). go/pkg/uid/uid.go defines RolePrefix = "role", so update the seed generation to avoid breaking consumers.
- File: go/pkg/testutil/seed/seed.go (around lines 396–398) — replace uid.PermissionPrefix with uid.RolePrefix
- roleID := uid.New(uid.PermissionPrefix) + roleID := uid.New(uid.RolePrefix)
♻️ Duplicate comments (29)
go/apps/api/routes/v2_keys_reroll_key/404_test.go (1)
35-45: Optional: convert to a table-driven 404 test for extensibilityPreps the suite for adding more 404 cases (e.g., unknown but valid format, or variants by workspace) and aligns with the Go testing guideline to use table-driven tests.
Apply:
- t.Run("nonexistent key", func(t *testing.T) { - nonexistentKeyID := uid.New(uid.KeyPrefix) - req := handler.Request{ - KeyId: nonexistentKeyID, - } - - res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse](h, route, headers, req) - require.Equal(t, 404, res.Status) - require.NotNil(t, res.Body) - require.Contains(t, res.Body.Error.Detail, "The specified key was not found") - }) + tests := []struct { + name string + req handler.Request + }{ + { + name: "nonexistent key", + req: handler.Request{KeyId: uid.New(uid.KeyPrefix)}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse](h, route, headers, tc.req) + require.Equal(t, 404, res.Status) + require.NotNil(t, res.Body) + require.Contains(t, res.Body.Error.Detail, "The specified key was not found") + }) + }Note: You previously asked to track table-driven conversions in an issue; happy to draft it and include this file in scope.
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseBody.yaml (1)
18-19: Replace example secrets to avoid secret-scanner false positivesGitleaks flags the example as a generic API key. Use obviously non-secret placeholders.
Apply:
data: - keyId: key_2cGKbMxRyIzhCxo1Idjz8q - key: prod_2cGKbMxRjIzhCxo1IdjH3arELti7Sdyc8w6XYbvtcyuBowPT + keyId: key_example_2cGKbMxRyIzhCxo1Idjz8q + key: sk_example_000000000000000000000000000000000000000000go/apps/api/routes/v2_keys_reroll_key/400_test.go (3)
15-76: Table-drive the 400 cases and assert validation details
- Use a table to consolidate invalid-input cases in line with project guidelines.
- Strengthen assertions by ensuring the response includes validation errors referring to the offending field.
Apply:
func TestRerollKeyBadRequest(t *testing.T) { h := testutil.NewHarness(t) @@ headers := http.Header{ "Content-Type": {"application/json"}, "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, } - t.Run("missing keyId", func(t *testing.T) { - req := handler.Request{ - // Missing keyId - } - - res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) - require.Equal(t, 400, res.Status) - require.NotNil(t, res.Body) - }) - - t.Run("empty keyId", func(t *testing.T) { - req := handler.Request{ - KeyId: "", - } - - res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) - require.Equal(t, 400, res.Status) - require.NotNil(t, res.Body) - }) - - t.Run("keyId too short", func(t *testing.T) { - req := handler.Request{ - KeyId: "ab", - } - - res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) - require.Equal(t, 400, res.Status) - require.NotNil(t, res.Body) - }) - - t.Run("negative expiration", func(t *testing.T) { - req := handler.Request{ - KeyId: uid.New(uid.KeyPrefix), - Expiration: -1, - } - - res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, req) - require.Equal(t, 400, res.Status) - require.NotNil(t, res.Body) - }) + tests := []struct { + name string + req handler.Request + wantFieldSubst string + }{ + { + name: "missing keyId", + req: handler.Request{}, // missing keyId + wantFieldSubst: "keyId", + }, + { + name: "empty keyId", + req: handler.Request{KeyId: ""}, + wantFieldSubst: "keyId", + }, + { + name: "keyId too short (prefix preserved)", + req: handler.Request{KeyId: "key_ab"}, + wantFieldSubst: "keyId", + }, + { + name: "negative expiration (milliseconds)", + req: handler.Request{KeyId: uid.New(uid.KeyPrefix), Expiration: -1}, + wantFieldSubst: "expiration", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + res := testutil.CallRoute[handler.Request, openapi.BadRequestErrorResponse](h, route, headers, tc.req) + require.Equal(t, 400, res.Status) + require.NotNil(t, res.Body) + // Validation details should point at the offending field. + require.Greater(t, len(res.Body.Error.ValidationErrors), 0) + require.Contains(t, res.Body.Error.ValidationErrors[0].Field, tc.wantFieldSubst) + }) + } }Outside the diff, ensure the openapi.BadRequestErrorResponse exposes Error.ValidationErrors[].Field as used above; if it's different, adapt the assertions accordingly.
56-61: Prefer prefix-preserving invalid KeyId to isolate validationUsing "key_ab" keeps the expected key prefix while remaining invalid, improving the specificity of validation.
Apply:
- req := handler.Request{ - KeyId: "ab", - } + req := handler.Request{ + KeyId: "key_ab", + }
66-75: Nit: clarify units in subtest nameIf you keep the current subtest style, make the units explicit for readability.
Apply:
- t.Run("negative expiration", func(t *testing.T) { + t.Run("negative expiration (milliseconds)", func(t *testing.T) {go/apps/api/routes/v2_keys_reroll_key/403_test.go (1)
68-71: Strengthen 403 assertions: verify error shape (title and requestId).As previously noted, assert structured fields to catch schema regressions.
Apply these diffs in each 403 case:
@@ - res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) - require.Equal(t, 403, res.Status) - require.NotNil(t, res.Body) + res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) + require.Equal(t, 403, res.Status) + require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Error.Title) + require.NotEmpty(t, res.Body.Meta.RequestId)@@ - res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) - require.Equal(t, 403, res.Status) - require.NotNil(t, res.Body) + res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) + require.Equal(t, 403, res.Status) + require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Error.Title) + require.NotEmpty(t, res.Body.Meta.RequestId)@@ - res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) - require.Equal(t, 403, res.Status) - require.NotNil(t, res.Body) + res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) + require.Equal(t, 403, res.Status) + require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Error.Title) + require.NotEmpty(t, res.Body.Meta.RequestId)@@ - res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) - require.Equal(t, 403, res.Status) - require.NotNil(t, res.Body) + res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) + require.Equal(t, 403, res.Status) + require.NotNil(t, res.Body) + require.NotEmpty(t, res.Body.Error.Title) + require.NotEmpty(t, res.Body.Meta.RequestId)Also applies to: 82-85, 96-99, 133-135
go/apps/api/routes/v2_keys_reroll_key/401_test.go (1)
16-85: Consider table-driven tests to reduce duplication.The three 401 subtests share setup and assertions; convert to a table per repo guidelines and prior discussion.
I can provide a compact table-driven refactor if you want it in this PR. Also noting prior learning from this repo to track this globally; happy to open/append to the existing issue.
go/pkg/db/querier_generated.go (2)
391-399: Emit real JSON booleans forauto_apply(not 0/1).
'auto_apply', rl.auto_apply = 1serializes as a number. The consumer unmarshals intobool(RatelimitInfo.AutoApply) and will fail, silently dropping ratelimits.Do not edit this generated file. Update the source SQL to emit booleans and regenerate.
Suggested change in:
- go/pkg/db/queries/key_find_live_by_hash.sql
- go/pkg/db/queries/key_find_live_by_id.sql
JSON_OBJECT( 'id', rl.id, 'name', rl.name, 'key_id', rl.key_id, 'identity_id', rl.identity_id, 'limit', rl.`limit`, 'duration', rl.duration, 'auto_apply', IF(rl.auto_apply, TRUE, FALSE) )Also applies to: 482-490
339-403: Stabilize JSON aggregate scan types across drivers (CAST to JSON).The JSON aggregates (
roles,permissions,role_permissions,ratelimits) are scanned intointerface{}and may arrive as[]byteorstringdepending on the driver. Standardize by casting the aggregates to JSON in SQL so the driver consistently returns JSON.Do not edit this generated file. Update the SQL to wrap each aggregate with
CAST(... AS JSON)and regenerate.Example:
COALESCE( CAST( (SELECT JSON_ARRAYAGG(JSON_OBJECT(...)) FROM ...) AS JSON ), JSON_ARRAY() ) AS rolesAlternatively (or additionally), ensure the Go decoder accepts both
[]byteandstring(see my comment on key_data.go).Also applies to: 430-496
go/pkg/db/key_data.go (3)
84-88: Initialize slices to empty to avoid nils and satisfy linters.Pre-initialize relation slices. This avoids nil surprises and keeps linters quiet without relying on nolint elsewhere.
- Roles: nil, - Permissions: nil, - RolePermissions: nil, - Ratelimits: nil, + Roles: make([]RoleInfo, 0), + Permissions: make([]PermissionInfo, 0), + RolePermissions: make([]PermissionInfo, 0), + Ratelimits: make([]RatelimitInfo, 0),
99-114: Decode JSON robustly from either []byte or string and remove duplication.Some drivers return JSON aggregates as
string. Replace the repeated blocks with a small helper to handle both[]byteandstring, defaulting to empty slices.- // It's fine to fail here - if roleBytes, ok := r.Roles.([]byte); ok && roleBytes != nil { - _ = json.Unmarshal(roleBytes, &kd.Roles) // Ignore error, default to empty array - } - - if permissionsBytes, ok := r.Permissions.([]byte); ok && permissionsBytes != nil { - _ = json.Unmarshal(permissionsBytes, &kd.Permissions) // Ignore error, default to empty array - } - - if rolePermissionsBytes, ok := r.RolePermissions.([]byte); ok && rolePermissionsBytes != nil { - _ = json.Unmarshal(rolePermissionsBytes, &kd.RolePermissions) // Ignore error, default to empty array - } - - if ratelimitsBytes, ok := r.Ratelimits.([]byte); ok && ratelimitsBytes != nil { - _ = json.Unmarshal(ratelimitsBytes, &kd.Ratelimits) // Ignore error, default to empty array - } + decodeJSON(r.Roles, &kd.Roles) + decodeJSON(r.Permissions, &kd.Permissions) + decodeJSON(r.RolePermissions, &kd.RolePermissions) + decodeJSON(r.Ratelimits, &kd.Ratelimits)Add this helper to the same file:
func decodeJSON[T any](src interface{}, out *[]T) { switch v := src.(type) { case []byte: if len(v) > 0 { _ = json.Unmarshal(v, out) } case string: if v != "" { _ = json.Unmarshal([]byte(v), out) } } if *out == nil { *out = make([]T, 0) } }Note: This complements the SQL-side CAST suggestion; either approach alone works, both together are most future-proof.
90-97: Optional: clarify intentionally omitted Identity fields.Static analysis flags missing fields (Environment, Deleted, CreatedAt, UpdatedAt). If not selected upstream, add a brief comment to set expectations.
if r.IdentityTableID.Valid { kd.Identity = &Identity{ ID: r.IdentityTableID.String, ExternalID: r.IdentityExternalID.String, WorkspaceID: r.WorkspaceID, Meta: r.IdentityMeta, } + // Note: Additional Identity fields (Environment, Deleted, CreatedAt, UpdatedAt) + // are not populated here because the query does not select them. }go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml (1)
31-31: Document the emitted audit event (“key.reroll”).Call out the audit event to help users correlate API calls with logs.
- **Important:** Analytics and usage metrics are tracked at both the key level AND identity level. If the original key has an identity, the new key will inherit it, allowing you to track usage across both individual keys and the overall identity. + **Important:** Analytics and usage metrics are tracked at both the key level AND identity level. If the original key has an identity, the new key will inherit it, allowing you to track usage across both individual keys and the overall identity. + This action is audited as "key.reroll".go/pkg/db/key_find_live_by_id.sql_generated.go (2)
73-84: Emit real JSON booleans forauto_apply(not 0/1).Same issue as the by-hash query. Update the source SQL to use
IF(rl.auto_apply, TRUE, FALSE)and regenerate; otherwise unmarshalling intoboolfails and ratelimits may be dropped by consumers.Suggested change in: go/pkg/db/queries/key_find_live_by_id.sql
'auto_apply', IF(rl.auto_apply, TRUE, FALSE)
25-91: Standardize JSON aggregate types across drivers (CAST to JSON).Wrap each JSON aggregate (
roles,permissions,role_permissions,ratelimits) withCAST(... AS JSON)to avoid driver-dependent scan types.Apply in the underlying SQL and regenerate.
Example:
COALESCE( CAST((SELECT JSON_ARRAYAGG(JSON_OBJECT(...)) FROM ...) AS JSON), JSON_ARRAY() ) AS permissionsgo/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml (1)
14-15: Cross-reference createKey response semantics.Add an explicit note that this response mirrors createKey’s
keyId/keysemantics to avoid ambiguity.Apply this diff:
Note: This is a new ID - the original key retains its own ID. + This field and the `key` below follow the same response format as the createKey endpoint.go/pkg/db/queries/key_find_live_by_hash.sql (4)
13-26: Stabilize roles array ordering for deterministic results.Add ORDER BY inside JSON_ARRAYAGG to avoid nondeterministic ordering across executions.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', r.id, 'name', r.name, 'description', r.description - ) - ) + ) ORDER BY r.name, r.id + )
28-42: Stabilize direct permissions array ordering.Same rationale as roles — add ORDER BY for deterministic output.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', p.id, 'name', p.name, 'slug', p.slug, 'description', p.description - ) - ) + ) ORDER BY p.slug, p.id + )
44-59: Order role_permissions and consider deduplication.At minimum, order the array; optionally dedupe if multiple roles share the same permission.
Apply this diff (ordering only):
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', p.id, 'name', p.name, 'slug', p.slug, 'description', p.description - ) - ) + ) ORDER BY p.slug, p.id + )
61-77: Stabilize ratelimits ordering and guard identity comparison.
- Add ORDER BY for deterministic array order.
- Guard identity comparison to avoid relying on NULL semantics.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', rl.id, 'name', rl.name, 'key_id', rl.key_id, 'identity_id', rl.identity_id, 'limit', rl.`limit`, 'duration', rl.duration, 'auto_apply', rl.auto_apply = 1 - ) - ) + ) ORDER BY rl.name, rl.id + ) FROM ratelimits rl - WHERE rl.key_id = k.id OR rl.identity_id = i.id), + WHERE rl.key_id = k.id OR (i.id IS NOT NULL AND rl.identity_id = i.id)),go/pkg/db/queries/key_find_live_by_id.sql (4)
13-26: Stabilize roles array ordering.Add ORDER BY inside JSON_ARRAYAGG to avoid nondeterministic ordering in responses and tests.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', r.id, 'name', r.name, 'description', r.description - ) - ) + ) ORDER BY r.name, r.id + )
28-42: Stabilize direct permissions array ordering.Same rationale — add ORDER BY to the aggregation.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', p.id, 'name', p.name, 'slug', p.slug, 'description', p.description - ) - ) + ) ORDER BY p.slug, p.id + )
44-59: Order role_permissions and consider deduplication.At minimum, order the array; optionally dedupe if roles overlap on the same permission.
Apply this diff (ordering only):
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', p.id, 'name', p.name, 'slug', p.slug, 'description', p.description - ) - ) + ) ORDER BY p.slug, p.id + )
61-78: Stabilize ratelimits ordering and guard identity comparison.Add ORDER BY and NULL-safe identity guard.
Apply this diff:
- (SELECT JSON_ARRAYAGG( - JSON_OBJECT( + (SELECT JSON_ARRAYAGG( + JSON_OBJECT( 'id', rl.id, 'name', rl.name, 'key_id', rl.key_id, 'identity_id', rl.identity_id, 'limit', rl.`limit`, 'duration', rl.duration, 'auto_apply', rl.auto_apply = 1 - ) - ) + ) ORDER BY rl.name, rl.id + ) FROM ratelimits rl - WHERE rl.key_id = k.id - OR rl.identity_id = i.id), + WHERE rl.key_id = k.id + OR (i.id IS NOT NULL AND rl.identity_id = i.id)),go/apps/api/routes/v2_keys_reroll_key/handler.go (3)
161-164: LGTM: Encrypt using the key’s workspace keyring.Using key.WorkspaceID for Vault.Encrypt aligns the keyring with the data owner.
176-193: Use the original key’s workspace for the new key (don’t overwrite with auth workspace).This risks cross-workspace inconsistencies; preserve the source key’s WorkspaceID.
Apply this diff:
err = db.Query.InsertKey(ctx, tx, db.InsertKeyParams{ ID: keyID, KeyringID: key.KeyAuthID, Hash: keyResult.Hash, Start: keyResult.Start, - WorkspaceID: auth.AuthorizedWorkspaceID, + WorkspaceID: key.WorkspaceID, ForWorkspaceID: key.ForWorkspaceID, CreatedAtM: now, Enabled: key.Enabled,
320-354: Consider logging the event under the key’s workspace.If auth workspace can differ from the key’s, the audit entry should use key.WorkspaceID.
Apply this diff:
- auditLogs = append(auditLogs, auditlog.AuditLog{ - WorkspaceID: auth.AuthorizedWorkspaceID, + auditLogs = append(auditLogs, auditlog.AuditLog{ + WorkspaceID: key.WorkspaceID,go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
136-139: Type JSON fields explicitly (avoid interface{}).
Roles,Permissions,RolePermissions, andRatelimitstyped as interface{} scan as []byte and require downstream assertions. Prefer json.RawMessage (or concrete types) via sqlc overrides.You can add overrides like:
# sqlc.yaml version: "2" overrides: - db: mysql column: "FindLiveKeyByHash.roles" go_type: "encoding/json.RawMessage" - db: mysql column: "FindLiveKeyByHash.permissions" go_type: "encoding/json.RawMessage" - db: mysql column: "FindLiveKeyByHash.role_permissions" go_type: "encoding/json.RawMessage" - db: mysql column: "FindLiveKeyByHash.ratelimits" go_type: "encoding/json.RawMessage"Then re-generate to update the struct field types.
go/apps/api/openapi/openapi-generated.yaml (1)
2530-2567: Good: key example now resembles a real secret token (not a keyId)Thanks for fixing the example to use a realistic, prefixed token string. This avoids confusing the secret “key” with the management “keyId.”
📜 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 settings 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 (21)
go/apps/api/openapi/gen.go(2 hunks)go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyRequestBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseBody.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml(1 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml(1 hunks)go/apps/api/routes/v2_keys_create_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/400_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/404_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_update_key/401_test.go(1 hunks)go/pkg/db/key_data.go(1 hunks)go/pkg/db/key_find_live_by_hash.sql_generated.go(2 hunks)go/pkg/db/key_find_live_by_id.sql_generated.go(2 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/key_find_live_by_hash.sql(1 hunks)go/pkg/db/queries/key_find_live_by_id.sql(1 hunks)go/pkg/testutil/seed/seed.go(5 hunks)
🧰 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/apps/api/routes/v2_keys_update_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_create_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/pkg/testutil/seed/seed.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/pkg/db/key_data.gogo/apps/api/openapi/gen.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/apps/api/routes/v2_keys_reroll_key/403_test.gogo/pkg/db/querier_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_live_by_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/apps/api/routes/v2_keys_update_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_create_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/apps/api/routes/v2_keys_reroll_key/403_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_keys_update_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_create_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/pkg/testutil/seed/seed.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/pkg/db/key_data.gogo/apps/api/openapi/gen.gogo/apps/api/routes/v2_keys_reroll_key/handler.gogo/apps/api/routes/v2_keys_reroll_key/403_test.gogo/pkg/db/querier_generated.gogo/pkg/db/key_find_live_by_hash.sql_generated.gogo/pkg/db/key_find_live_by_id.sql_generated.go
🧠 Learnings (11)
📚 Learning: 2025-08-14T16:25:48.123Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.123Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_reroll_key/400_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.gogo/apps/api/routes/v2_keys_reroll_key/200_test.gogo/apps/api/routes/v2_keys_reroll_key/403_test.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/404_test.gogo/apps/api/routes/v2_keys_reroll_key/401_test.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go tests by HTTP status codes
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/401_test.go
📚 Learning: 2025-08-08T16:10:00.224Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/401_test.go
📚 Learning: 2025-08-08T15:10:46.436Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/401_test.go
📚 Learning: 2025-08-08T14:59:52.283Z
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/401_test.go
📚 Learning: 2025-08-14T16:41:57.336Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:32-52
Timestamp: 2025-08-14T16:41:57.336Z
Learning: Using unsafe.Pointer to cast between different struct types (even with identical fields) in Go can cause memory corruption because the runtime may arrange identical field layouts differently in memory due to alignment, padding, or compiler optimizations. Always use explicit type handling or safe value copies instead.
Applied to files:
go/pkg/db/key_data.go
📚 Learning: 2025-08-14T16:26:55.244Z
Learnt from: imeyer
PR: unkeyed/unkey#3785
File: go/pkg/db/key_data.go:54-69
Timestamp: 2025-08-14T16:26:55.244Z
Learning: In the Unkey codebase, readability always trumps code optimization for line count. Struct literals with many fields should be formatted with one field per line for better readability, maintainability, and git diff clarity.
Applied to files:
go/pkg/db/key_data.go
📚 Learning: 2025-02-21T11:15:16.185Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2896
File: internal/clickhouse/src/ratelimits.ts:468-592
Timestamp: 2025-02-21T11:15:16.185Z
Learning: The cursor logic in getRatelimitOverviewLogs query uses (time, request_id) < (cursorTime, cursorRequestId) comparison which works well with descending order but may need adjustment for ascending sorts based on real usage patterns.
Applied to files:
go/pkg/db/queries/key_find_live_by_id.sql
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/pkg/db/queries/key_find_live_by_hash.sqlgo/apps/api/routes/v2_keys_reroll_key/handler.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/handler.go
🧬 Code Graph Analysis (13)
go/apps/api/routes/v2_keys_update_key/401_test.go (1)
go/pkg/uid/uid.go (1)
KeyPrefix(16-16)
go/apps/api/routes/v2_keys_reroll_key/404_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/pkg/testutil/seed/seed.go (2)
Resources(23-28)New(39-46)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)go/apps/api/openapi/gen.go (1)
NotFoundErrorResponse(270-276)
go/apps/api/routes/v2_keys_create_key/401_test.go (1)
go/pkg/uid/uid.go (1)
KeyPrefix(16-16)
go/apps/api/routes/v2_keys_reroll_key/400_test.go (5)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/pkg/testutil/seed/seed.go (2)
Resources(23-28)New(39-46)go/apps/api/openapi/gen.go (1)
BadRequestErrorResponse(59-65)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)
go/apps/api/routes/v2_keys_reroll_key/401_test.go (6)
go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/apps/api/routes/register.go (1)
Register(58-554)go/pkg/testutil/seed/seed.go (2)
CreateApiRequest(83-91)New(39-46)go/apps/api/openapi/gen.go (1)
UnauthorizedErrorResponse(450-456)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)
go/pkg/testutil/seed/seed.go (5)
apps/agent/services/vault/service.go (2)
Service(20-30)New(39-74)go/pkg/ptr/deref.go (1)
SafeDeref(35-44)go/gen/proto/vault/v1/service.pb.go (3)
EncryptRequest(104-110)EncryptRequest(123-123)EncryptRequest(138-140)go/pkg/db/key_encryption_insert.sql_generated.go (1)
InsertKeyEncryptionParams(18-24)go/gen/proto/vault/v1/object.pb.go (3)
Encrypted(252-264)Encrypted(277-277)Encrypted(292-294)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (6)
go/pkg/testutil/http.go (2)
NewHarness(52-175)CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (3)
Handler(32-38)Request(28-28)Response(29-29)go/pkg/testutil/seed/seed.go (7)
Resources(23-28)CreateApiRequest(83-91)CreateIdentityRequest(349-354)CreateRatelimitRequest(305-313)CreateKeyRequest(187-206)CreatePermissionRequest(421-426)CreateRoleRequest(384-390)go/apps/api/openapi/gen.go (1)
Meta(259-262)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (1)
ToKeyData(32-45)
go/pkg/db/key_data.go (5)
go/pkg/db/types/null_string.go (1)
NullString(10-10)go/pkg/db/custom_types.go (3)
RoleInfo(10-14)PermissionInfo(16-21)RatelimitInfo(23-31)apps/dashboard/lib/trpc/routers/api/keys/query-api-keys/schema.ts (1)
Ratelimits(33-33)go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)go/pkg/db/key_find_live_by_id.sql_generated.go (1)
FindLiveKeyByIDRow(105-141)
go/apps/api/openapi/gen.go (2)
go/pkg/codes/unkey_data.go (1)
Data(93-136)go/pkg/db/models_generated.go (1)
Key(725-749)
go/apps/api/routes/v2_keys_reroll_key/handler.go (16)
go/apps/api/openapi/gen.go (4)
V2KeysRerollKeyRequestBody(1043-1068)V2KeysRerollKeyResponseBody(1071-1076)Meta(259-262)V2KeysRerollKeyResponseData(1079-1106)go/pkg/zen/request_util.go (1)
BindBody(10-22)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/fault/wrap.go (3)
Public(97-111)Wrap(25-67)Internal(75-89)go/pkg/db/key_data.go (1)
ToKeyData(32-45)go/pkg/rbac/query.go (3)
Or(64-70)T(84-90)And(46-52)go/pkg/rbac/permissions.go (3)
Tuple(175-184)Limit(74-74)UpdateKey(52-52)go/internal/services/keys/options.go (1)
WithPermissions(47-52)go/pkg/uid/uid.go (1)
KeyPrefix(16-16)go/gen/proto/vault/v1/service.pb.go (6)
EncryptResponse(156-162)EncryptResponse(175-175)EncryptResponse(190-192)EncryptRequest(104-110)EncryptRequest(123-123)EncryptRequest(138-140)go/pkg/db/tx.go (1)
Tx(196-201)go/pkg/db/key_insert.sql_generated.go (1)
InsertKeyParams(51-67)go/gen/proto/vault/v1/object.pb.go (3)
Encrypted(252-264)Encrypted(277-277)Encrypted(292-294)go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
InsertKeyRatelimitParams(39-49)go/pkg/db/key_update.sql_generated.go (1)
UpdateKeyParams(51-70)go/pkg/auditlog/events.go (1)
KeyRerollEvent(21-21)
go/apps/api/routes/v2_keys_reroll_key/403_test.go (7)
go/pkg/rbac/query.go (1)
T(84-90)go/pkg/testutil/http.go (1)
NewHarness(52-175)go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
Handler(32-38)Request(28-28)go/apps/api/routes/register.go (1)
Register(58-554)go/pkg/testutil/seed/seed.go (2)
CreateApiRequest(83-91)Resources(23-28)go/pkg/rbac/permissions.go (1)
CreateKey(49-49)go/apps/api/openapi/gen.go (2)
ForbiddenErrorResponse(132-138)Meta(259-262)
go/pkg/db/querier_generated.go (1)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
FindLiveKeyByHashRow(104-140)
go/pkg/db/key_find_live_by_hash.sql_generated.go (1)
go/pkg/db/types/null_string.go (1)
NullString(10-10)
🪛 golangci-lint (2.2.2)
go/pkg/db/key_data.go
53-53: db.KeyData is missing field Identity
(exhaustruct)
91-91: db.Identity is missing fields Environment, Deleted, CreatedAt, UpdatedAt
(exhaustruct)
🪛 Gitleaks (8.27.2)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseBody.yaml
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 YAMLlint (1.37.1)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml
[error] 31-31: trailing spaces
(trailing-spaces)
⏰ 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). (5)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
go/apps/api/routes/v2_keys_update_key/401_test.go (1)
58-63: LGTM: Use uid.KeyPrefix for synthetic bearer keysSwitching to uid.New(uid.KeyPrefix) makes the "nonexistent key" token well-formed and consistent with other tests.
go/apps/api/routes/v2_keys_create_key/401_test.go (1)
45-50: LGTM: Consistent construction of nonexistent bearer tokenUsing uid.New(uid.KeyPrefix) aligns with how keys are generated across tests and avoids brittle string literals.
go/apps/api/routes/v2_keys_reroll_key/404_test.go (1)
15-46: Good 404 coverage and clear assertion messageTest is concise, uses a validly-shaped unknown KeyId, and asserts a helpful error detail. This pairs well with the 200/400/401/403 suites.
go/apps/api/routes/v2_keys_reroll_key/403_test.go (1)
15-27: Good rename and route wiring.Test name now matches the reroll endpoint and the route is correctly registered with the harness.
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyRequestBody.yaml (1)
1-41: Spec looks good: int64 for expiration and clear semantics.
- Uses int64 for expiration with reasonable bounds.
- Description clarifies relative TTL and overlap behavior.
go/pkg/testutil/seed/seed.go (1)
254-270: No action needed — vault.Encrypt returns a string and matches the DB schema.EncryptResponse.Encrypted is a string and EncryptResponse.GetEncrypted() returns string, while InsertKeyEncryptionParams.Encrypted is declared as string — the current code is type-compatible.
- go/gen/proto/vault/v1/service.pb.go: EncryptResponse.Encrypted is string; GetEncrypted() returns string.
- go/pkg/db/key_encryption_insert.sql_generated.go: InsertKeyEncryptionParams.Encrypted is string and used in the INSERT.
go/apps/api/openapi/gen.go (3)
1042-1068: Generated request type aligns with spec (int64 expiration).V2KeysRerollKeyRequestBody correctly uses int64 for Expiration and documents semantics. No action on generated code.
1070-1106: Generated response types look consistent and secure messaging is clear.Key vs KeyId distinction and one-time key visibility notes are solid.
2021-2023: Alias added for JSON request body is correct.RerollKeyJSONRequestBody alias matches the new request type and follows existing patterns.
go/apps/api/routes/v2_keys_reroll_key/200_test.go (1)
160-214: LGTM: Strong, set-based equality checks for related collections.Good move replacing length comparisons with set equality for permissions (direct+role-derived), roles, and ratelimits (including values). This makes the test robust against reordering and subtle mismatches.
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml (1)
1-86: Confirmed: repository consistently usesrerollKey(norollKeyreferences).Repo search shows only
rerollKeyoccurrences — OpenAPI splits/generated files, route handler, registration, and tests — so no rename is required in this PR.Files of interest:
- go/apps/api/openapi/openapi-split.yaml (path ref -> spec/paths/v2/keys/rerollKey)
- go/apps/api/openapi/openapi-generated.yaml (path /v2/keys.rerollKey, operationId: rerollKey)
- go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml
- go/apps/api/routes/v2_keys_reroll_key/handler.go (route, path string)
- go/apps/api/routes/register.go (route registration)
- go/apps/api/routes/v2_keys_reroll_key/*_test.go (tests)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml (2)
16-33: LGTM: Clear security guidance and realistic key token example.The
keyfield’s description adequately warns about one-time visibility and sensitivity, and the example now looks like a secret token rather than an ID.
34-36: LGTM: Required fields set correctly.Both
keyIdandkeyare required, matching the API contract.go/pkg/db/queries/key_find_live_by_hash.sql (1)
86-89: LGTM: Excluding soft-deleted workspaces.Adding
ws.deleted_at_m IS NULLaligns with other soft-delete guards.go/pkg/db/queries/key_find_live_by_id.sql (1)
87-90: LGTM: Excluding soft-deleted workspaces.
ws.deleted_at_m IS NULLis present here as well — consistent with other filters.go/apps/api/routes/v2_keys_reroll_key/handler.go (2)
54-58: Ensure emit() is safe to defer on error paths.If GetRootKey returns an error, confirm emit is non-nil. If not guaranteed, wrap defer with a nil-check.
Would you verify whether emit is always non-nil even when err != nil? If not, change:
defer func() { if emit != nil { emit() } }()
84-113: RBAC checks look correct and granular.CreateKey is required; EncryptKey is additionally enforced for recoverable/encrypted keys.
go/apps/api/openapi/openapi-generated.yaml (2)
929-970: Request body shape and semantics look solid
- Fields match the PR objective (keyId + relative expiration in ms).
- Description clearly states “Duration in milliseconds until the ORIGINAL key is revoked, starting from now.” Good.
No functional issues from the schema side.
971-979: Response envelope aligns with existing patternsV2KeysRerollKeyResponseBody mirrors other endpoints’ meta/data envelope. Consistent and correct.
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/V2KeysRerollKeyResponseData.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml (2)
31-38: Call out the emitted audit event in the operation description.Document the audit event so users can correlate with logs.
**Important:** Analytics and usage metrics are tracked at both the key level AND identity level. If the original key has an identity, the new key will inherit it, allowing you to track usage across both individual keys and the overall identity. + + This action is audited as "key.reroll".
21-25: Clarify “expiration” semantics as relative TTL (ms).Minor doc tweak to reduce ambiguity for clients.
- **Original Key Handling:** - - The original key will be revoked after the duration specified in `expiration` + **Original Key Handling:** + - The original key will be revoked after the duration specified in `expiration` (relative TTL in milliseconds, starting from now)go/apps/api/routes/v2_keys_reroll_key/handler.go (4)
201-208: Align key encryption row’s workspace with the key’s workspace.Avoid cross-workspace inconsistencies.
if encryption != nil { err = db.Query.InsertKeyEncryption(ctx, tx, db.InsertKeyEncryptionParams{ - WorkspaceID: auth.AuthorizedWorkspaceID, + WorkspaceID: key.WorkspaceID, KeyID: keyID, CreatedAt: now, Encrypted: encryption.GetEncrypted(), EncryptionKeyID: encryption.GetKeyId(), })
32-38: Add GoDoc for exported Handler type.Public types must be documented per guidelines.
-type Handler struct { +// Handler processes POST /v2/keys.rerollKey requests. +// It creates a new key preserving settings and schedules the original key's expiry. +type Handler struct {
60-66: Validate that expiration is non-negative; return 400 for invalid input.Prevents surprising behavior for negative TTLs.
req, err := zen.BindBody[Request](s) if err != nil { return err } + if req.Expiration < 0 { + return fault.New("invalid expiration", + fault.Code(codes.App.Validation.InvalidInput.URN()), + fault.Public("expiration must be a non-negative number of milliseconds."), + ) + }
291-300: Avoid earlier-than-requested expiry and don’t extend an existing earlier expiry.
- Ceil to next minute to avoid expiring early.
- Never extend beyond original expiry if it’s sooner.
- // Calculate the desired expiry time (rounded up to next minute) - expiration := time.Now().Add(time.Millisecond * time.Duration(req.Expiration)) - // Round up to next minute (ceil) - if expiration.Truncate(time.Minute) != expiration { - expiration = expiration.Truncate(time.Minute).Add(time.Minute) - } - - if req.Expiration == 0 { - expiration = time.Now() - } + // Calculate requested expiry. 0 = immediate revocation. + nowTime := time.Now() + var desiredExpiry time.Time + if req.Expiration == 0 { + desiredExpiry = nowTime + } else { + // Round up to next minute (ceil) to avoid expiring earlier than requested. + d := nowTime.Add(time.Millisecond * time.Duration(req.Expiration)) + if d.Truncate(time.Minute) != d { + d = d.Truncate(time.Minute).Add(time.Minute) + } + desiredExpiry = d + } + // Never extend beyond an existing earlier expiry on the original key. + expiration := desiredExpiry + if key.Expires.Valid && key.Expires.Time.Before(desiredExpiry) { + expiration = key.Expires.Time + }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
go/apps/api/openapi/openapi-generated.yaml(3 hunks)go/apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml(1 hunks)go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/handler.go(1 hunks)
🧰 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/apps/api/routes/v2_keys_reroll_key/200_test.gogo/apps/api/routes/v2_keys_reroll_key/handler.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/apps/api/routes/v2_keys_reroll_key/200_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_keys_reroll_key/200_test.gogo/apps/api/routes/v2_keys_reroll_key/handler.go
🧠 Learnings (3)
📚 Learning: 2025-08-14T16:25:48.123Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.123Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/200_test.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/handler.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: When querying or updating namespaces in the Unkey dashboard, always scope the operations to the current workspace using `eq(table.workspaceId, ctx.workspace.id)` to prevent cross-workspace access.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/handler.go
🧬 Code Graph Analysis (2)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (6)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (3)
Handler(32-38)Request(28-28)Response(29-29)go/pkg/testutil/seed/seed.go (7)
Resources(23-28)CreateApiRequest(83-91)CreateIdentityRequest(349-354)CreateRatelimitRequest(305-313)CreateKeyRequest(187-206)CreatePermissionRequest(421-426)CreateRoleRequest(384-390)go/apps/api/openapi/gen.go (1)
Meta(259-262)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (1)
ToKeyData(32-45)
go/apps/api/routes/v2_keys_reroll_key/handler.go (14)
go/apps/api/openapi/gen.go (4)
V2KeysRerollKeyRequestBody(1043-1068)V2KeysRerollKeyResponseBody(1071-1076)Meta(259-262)V2KeysRerollKeyResponseData(1079-1106)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/fault/wrap.go (2)
Public(97-111)Wrap(25-67)go/pkg/rbac/query.go (3)
Or(64-70)T(84-90)And(46-52)go/pkg/rbac/permissions.go (3)
Tuple(175-184)Limit(74-74)UpdateKey(52-52)go/internal/services/keys/options.go (1)
WithPermissions(47-52)go/pkg/uid/uid.go (2)
KeyPrefix(16-16)RatelimitPrefix(29-29)go/gen/proto/vault/v1/service.pb.go (6)
EncryptResponse(156-162)EncryptResponse(175-175)EncryptResponse(190-192)EncryptRequest(104-110)EncryptRequest(123-123)EncryptRequest(138-140)go/pkg/db/tx.go (1)
Tx(196-201)go/pkg/db/key_insert.sql_generated.go (1)
InsertKeyParams(51-67)go/gen/proto/vault/v1/object.pb.go (3)
Encrypted(252-264)Encrypted(277-277)Encrypted(292-294)go/pkg/db/key_insert_ratelimit.sql_generated.go (1)
InsertKeyRatelimitParams(39-49)go/pkg/db/key_update.sql_generated.go (1)
UpdateKeyParams(51-70)go/pkg/auditlog/events.go (1)
KeyRerollEvent(21-21)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_keys_reroll_key/200_test.go
17-17: TestRerollKeySuccess's subtests should call t.Parallel
(tparallel)
245-245: unnecessary conversion
(unconvert)
247-247: unnecessary conversion
(unconvert)
go/apps/api/routes/v2_keys_reroll_key/handler.go
302-302: db.UpdateKeyParams is missing fields NameSpecified, Name, IdentityIDSpecified, IdentityID, EnabledSpecified, Enabled, MetaSpecified, Meta, RemainingRequestsSpecified, RemainingRequests, RefillAmountSpecified, RefillAmount, RefillDaySpecified, RefillDay, Now
(exhaustruct)
⏰ 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). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
go/apps/api/openapi/openapi-generated.yaml (1)
4414-4498: LGTM: Generated models and path for rerollKey look consistent and accurate.
- Request uses expiration (relative ms) and keyId as required fields.
- Response returns new keyId + key; examples look realistic.
- Path, summary, and security match the handler and tests.
Also applies to: 929-970, 2530-2566
imeyer
left a comment
imeyer
left a comment
There was a problem hiding this comment.
One more pass and this should be good!
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (2)
17-25: Consider converting to a table-driven suite (tracked as follow-up).Per our guideline for *_test.go, this file can be table-driven. Since you opted to track this as a follow-up in a previous discussion, no blocker here.
248-250: Remove unnecessary conversion (golangci-lint: unconvert).The type is already int64; the extra int64(...) casts are redundant.
- require.True(t, expMs >= now && expMs <= now+int64(ttlMs)+60000, - "original key expiration should be between now and now+TTL+1min for rounding (got %d, expected between %d and %d)", - expMs, now, now+int64(ttlMs)+60000) + require.True(t, expMs >= now && expMs <= now+ttlMs+60000, + "original key expiration should be between now and now+TTL+1min for rounding (got %d, expected between %d and %d)", + expMs, now, now+ttlMs+60000)
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
go/apps/api/routes/v2_keys_reroll_key/200_test.go(1 hunks)
🧰 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/apps/api/routes/v2_keys_reroll_key/200_test.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/apps/api/routes/v2_keys_reroll_key/200_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_keys_reroll_key/200_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/handler.go:176-193
Timestamp: 2025-08-14T17:45:14.420Z
Learning: When rerolling keys in the v2 reroll endpoint, expiration dates should be copied from the original key to preserve business constraints like trial periods or temporary access policies. Users expect rerolled keys to maintain the same expiration limitations as the original.
📚 Learning: 2025-08-14T16:25:48.123Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.123Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/200_test.go
📚 Learning: 2025-08-14T17:45:14.420Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/handler.go:176-193
Timestamp: 2025-08-14T17:45:14.420Z
Learning: When rerolling keys in the v2 reroll endpoint, expiration dates should be copied from the original key to preserve business constraints like trial periods or temporary access policies. Users expect rerolled keys to maintain the same expiration limitations as the original.
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/200_test.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/apps/api/routes/v2_keys_reroll_key/200_test.go
🧬 Code Graph Analysis (1)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (6)
go/pkg/testutil/http.go (1)
CallRoute(257-291)go/apps/api/routes/v2_keys_reroll_key/handler.go (3)
Handler(32-38)Request(28-28)Response(29-29)go/pkg/testutil/seed/seed.go (4)
Resources(23-28)CreateApiRequest(83-91)CreateIdentityRequest(349-354)CreateRatelimitRequest(305-313)go/apps/api/openapi/gen.go (1)
Meta(259-262)go/pkg/ptr/pointer.go (1)
P(49-51)go/pkg/db/key_data.go (1)
ToKeyData(32-45)
🪛 golangci-lint (2.2.2)
go/apps/api/routes/v2_keys_reroll_key/200_test.go
248-248: unnecessary conversion
(unconvert)
250-250: unnecessary conversion
(unconvert)
🔇 Additional comments (4)
go/apps/api/routes/v2_keys_reroll_key/200_test.go (4)
17-19: Good rename and top-level parallelization.The test name now clearly reflects reroll semantics and runs in parallel at the root, matching our test-org guideline for 200-status suites.
120-123: Confirmed — Expiration=0 = immediate revocation (documented).The generated OpenAPI type V2KeysRerollKeyRequestBody explicitly documents: "Set to
0to revoke the original key immediately."
- Location: go/apps/api/openapi/gen.go — type V2KeysRerollKeyRequestBody (≈ line 1043), field
Expiration int64 json:"expiration"
149-156: Confirmed — expiration=0 intentionally revokes the old key (immediate expiry) and is documentedThe handler explicitly treats req.Expiration == 0 as immediate expiry, and the OpenAPI spec documents "Set
expirationto 0 to revoke immediately", so the test is valid.Relevant locations:
- go/apps/api/routes/v2_keys_reroll_key/handler.go — sets expiration = time.Now() when req.Expiration == 0 (around the req.Expiration handling, ~lines 292–306).
- apps/api/openapi/openapi-generated.yaml and apps/api/openapi/spec/paths/v2/keys/rerollKey/index.yaml — include "Set
expirationto 0 to revoke immediately".- go/apps/api/routes/v2_keys_reroll_key/200_test.go — the assertions at lines 149–156 rely on this contract; consider adding a one-line inline comment referencing the contract for future readers (optional).
217-256: Add test asserting rerolled key inherits the original expirationPer product expectation (and retrieved learning that rerolled keys should preserve original expirations), the test should verify the new key copies the original key's Expires. The current test asserts the rolled key has no expiration — update/add a test to assert inheritance.
Files to change:
- go/apps/api/routes/v2_keys_reroll_key/200_test.go — update the t.Run at ~lines 217–256 or add a new subtest that:
- seeds an original key with a non-nil expiration (e.g., now+24h),
- rerolls without providing TTL,
- asserts rolledKeyRow.Expires.Valid == true and rolledKeyRow.Expires.Time ≈ original expiration (allow small tolerance for minute-alignment),
- retains/validates createdKeyRow.Expires is set per TTL policy (existing checks).
- go/apps/api/routes/v2_keys_reroll_key/handler.go (around lines 176–193) — verify/fix the handler to copy Expires from the original key to the new key when rerolling if it does not already.
I can draft the concrete test snippet — please confirm seed.CreateKeyRequest.Expires type (time.Time vs ms).
⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3785 File: go/apps/api/routes/v2_keys_reroll_key/handler.go:176-193 Timestamp: 2025-08-14T17:45:14.420Z Learning: When rerolling keys in the v2 reroll endpoint, expiration dates should be copied from the original key to preserve business constraints like trial periods or temporary access policies. Users expect rerolled keys to maintain the same expiration limitations as the original.Learnt from: Flo4604 PR: unkeyed/unkey#3785 File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61 Timestamp: 2025-08-14T16:25:48.123Z Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
|
Ill fix this one issue later, since we can use json array or just - [ ]byte in other places as well instead of an interface We can't use types directly since sqlc does not know how to handle that, maybe its possible to write a plugin that deals with that not too sure. The other thing was a non-issue |
|
should be able to make this work with overrides |
What is "this" ? |
|
this being sqlc and not using interfaces |
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
If Ian is happy I am happy.
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/14/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
Fixes #3767
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Unit tests and possible spinning up the v2 api locally and running e2e tests.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Documentation
Tests
Chores