feat(api-keys): add API key CRUD management - #53
Conversation
Full API key lifecycle management:
- Backend: CRUD endpoints, DTOs, use case wiring, auth middleware
- Frontend: API keys view, useApiKeys composable, types
- Tests: Integration tests and Playwright E2E tests
- Docs: API docs and configuration guide
- CLI: seed-admin subcommand for admin bootstrap
- Fix: axum 0.8 path syntax (':id' → '{id}')
NOTE: Changed example key prefix from 'rk_live_' to 'rook_fake_' to avoid
GitHub secret scanner false positive.
|
Warning Review limit reached
More reviews will be available in 27 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements comprehensive API key CRUD functionality for external agent authentication, adding domain types, SQLite repository with pagination, use-case business logic, Axum HTTP handlers, and a Vue dashboard UI. It includes soft revocation semantics, session-authenticated endpoints, and accompanying end-to-end and integration tests across all layers. ChangesAPI Key CRUD Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openspec/changes/archive/2026-05-31-api-key-crud/state.yaml (1)
1-202:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
state.yamlis not valid YAML and will break YAML consumers.The file content is Markdown-formatted text/tables, not YAML mappings/sequences. Convert it to valid YAML (or rename to
.mdif meant to be narrative-only).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/archive/2026-05-31-api-key-crud/state.yaml` around lines 1 - 202, The file currently named state.yaml contains Markdown (tables, headings like "Change: api-key-crud" and "## Metadata") rather than valid YAML; either convert the document into a proper YAML structure (top-level mappings for fields such as change, current_phase, completed, next, updated, metadata, stack_detected, specs, etc., replacing Markdown tables with YAML maps/lists) or rename/move the file to a .md and create a small state.yaml with the canonical YAML keys (e.g., change: api-key-crud, current_phase: verify, updated: 2026-05-31, completed: [...]) so YAML consumers can parse it (locate this in the file containing the "Change: api-key-crud" heading and the "## Metadata" table).apps/rook/dashboard/src/lib/api.ts (1)
132-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the CSRF header in the shared request helper.
The PR contract says state-changing requests use the double-submit cookie pattern, but this helper only includes cookies. Every dashboard
POST/PUT/DELETErouted throughrequest()will missX-CSRF-Tokenand be rejected once CSRF enforcement is on.Suggested fix
function createApiClient() { const baseUrl = getBaseUrl() + function getCsrfToken(): string | null { + if (typeof document === 'undefined') return null + const match = document.cookie.match(/(?:^|;\s*)csrf_token=([^;]+)/) + return match ? decodeURIComponent(match[1]) : null + } + async function request<T>( path: string, options: RequestInit = {} ): Promise<T> { const url = `${baseUrl}${path}` + const method = (options.method ?? 'GET').toUpperCase() + const csrfToken = method === 'GET' || method === 'HEAD' ? null : getCsrfToken() const response = await fetch(url, { ...options, headers: { 'Content-Type': 'application/json', + ...(csrfToken ? { 'X-CSRF-Token': csrfToken } : {}), ...options.headers, }, credentials: 'include', // Include cookies for session auth })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/rook/dashboard/src/lib/api.ts` around lines 132 - 145, The shared request helper function request currently sends cookies but omits the CSRF token header; update request to read the CSRF token from the double-submit cookie (e.g., from document.cookie or the existing cookie helper) and set the 'X-CSRF-Token' header on state-changing requests (POST, PUT, DELETE, PATCH) before calling fetch so those routes are not rejected when CSRF enforcement is enabled; keep existing Content-Type merging behavior and credentials: 'include'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/rook/dashboard/e2e/api-keys.spec.ts`:
- Around line 214-215: The tests create API keys using the fixed labels
'key-to-edit' and 'key-to-revoke', which accumulate across beforeEach runs and
cause locator ambiguity; change each createApiKeyViaApi(...) call to use a
test-scoped unique label (e.g., append a timestamp/random suffix) and store that
label in a variable used by the rest of the test so all subsequent locator
queries (the edit/revoke flows) target the unique label; update every
createApiKeyViaApi invocation and any locator references that expect
'key-to-edit'/'key-to-revoke' to use the stored unique label.
- Around line 29-30: The test is calling page.request.getCookies(...) which
fails because APIRequestContext has no getCookies; replace that call with
browser-context cookie access by using page.context().cookies(API_BASE_URL)
(e.g., const cookies = await page.context().cookies(API_BASE_URL)) or
alternatively obtain cookies from the APIRequestContext via storageState(), then
find the csrf_token cookie as before; update the references to
page.request.getCookies and csrfCookie accordingly.
In `@apps/rook/dashboard/src/components/ui/carousel/CarouselContent.vue`:
- Line 12: The template binds ref="carouselRef" but the script destructures
useCarousel() as carouselRef: _carouselRef, so the template never receives the
actual ref; change the destructuring in the script to const { carouselRef,
orientation } = useCarousel() (or update the template to use ref="_carouselRef")
so the template and script use the same ref identifier (referencing carouselRef,
_carouselRef, and useCarousel to locate the code).
In `@apps/rook/dashboard/src/components/ui/locale-switcher/LocaleSwitcher.vue`:
- Around line 20-23: The current handleChange(value: AcceptableValue) only
checks typeof string then casts to 'en'|'es' and can pass unsupported locales;
update handleChange to explicitly validate that value is one of the supported
locale strings (e.g., check value === 'en' || value === 'es' or use a small
allowedLocales set/array) before calling setLocale, and otherwise ignore/handle
invalid values (fallback to a default or no-op). Reference: function
handleChange and setLocale.
In `@apps/rook/dashboard/src/types/unovis.d.ts`:
- Around line 19-22: The default export as an object literal in the ambient
module is invalid; replace it by declaring a const typed object and exporting
that const as default. Concretely, keep the existing exported symbols
(VisCrosshair, VisTooltip, Chart) and add a declaration like `const _default: {
Chart: typeof Chart; VisCrosshair: typeof VisCrosshair; VisTooltip: typeof
VisTooltip }` then `export default _default`; update the existing `export const
VisCrosshair`, `export const VisTooltip`, and `export const Chart` declarations
to be referenced by `typeof` in the default const so the ambient typings remain
valid.
In `@apps/rook/dashboard/src/views/ApiKeysView.vue`:
- Around line 108-112: The plaintext API key must be cleared whenever the create
dialog closes (including overlay click or Escape); update every modal-close path
to call closeCreateWithKey (or at minimum set newlyCreatedKey.value = null and
showNewKey.value = false) so the secret is removed from memory; locate other
close handlers referenced around the other modal code (the alternative close
function in the 321-348 region) and either invoke closeCreateWithKey from them
or duplicate the same clearing logic to ensure the key never reappears on next
open.
- Around line 274-289: The icon-only Buttons (the ones that call
openEditModal(item) and confirmRevoke(item.id) and similar buttons at the other
locations) lack accessible names; update each <Button> that renders only an icon
(e.g., the Pencil and Trash2 buttons) to include an aria-label or add visually
hidden text that conveys the action (e.g., aria-label="Edit API key" and
aria-label="Revoke API key"), and do the same for the other instances referenced
(lines near 342-344, 353-357, 418-419) so Playwright getByLabel and
screen-readers can find them; ensure labels include contextual info when needed
(e.g., include item.id or item.name when appropriate) and keep the existing
click handlers (openEditModal, confirmRevoke) unchanged.
- Around line 114-123: openEditModal assigns editForm.scopes to the same array
instance as the row (apiKeys), causing in-modal changes to mutate the table;
modify openEditModal so that when setting editForm.scopes you clone the scopes
array (e.g., use a shallow copy like [...key.scopes] or key.scopes?.slice() with
a fallback to []), leaving editingKey, editForm and showEditModal assignment
logic intact so modal changes do not affect apiKeys until saved.
In `@apps/rook/src/main.rs`:
- Around line 67-81: The current seed-admin path builds the full RookContainer
(RookContainer::build) which pulls in unrelated startup requirements; instead
add a minimal bootstrap builder (e.g., di::build_seed_admin_container or
RookContainer::build_for_seed_admin) that only constructs the dependencies
required by the SetAdminPasswordInput/set_admin_password usecase (DB connection,
user repository, and any directly needed configs) and does not require
API_KEY_HASH_SECRET or provider encryption envs, then call that minimal builder
here and invoke set_admin_password.execute(input). Update di to expose the new
minimal builder and ensure it only initialises the specific usecase and its
direct dependencies referenced by set_admin_password so unrelated envs or
features are not required for the seed-admin command.
- Around line 24-30: The SeedAdmin CLI currently accepts a positional password
(Commands::SeedAdmin -> password: String) which leaks secrets; change the
Commands::SeedAdmin variant to not store a positional String and instead obtain
the password securely at runtime (e.g. add an optional named arg like
--password-file/--password-stdin or no arg and prompt interactively). Update the
match handling for Commands::SeedAdmin in main.rs to: if a file
path/--password-file was provided, read the secret from that file; else if a
--password-stdin flag is set, read from stdin; otherwise prompt with a
non-echoed prompt (use a crate like rpassword::read_password or
read_password_from_tty) and use that value for seeding. Ensure no plain
positional password field remains in the Commands enum or is logged.
In `@crates/application/rook-usecases/src/manage_api_keys.rs`:
- Around line 100-104: When calling self.repo.update(...) after fetching
existing with self.repo.find(...), map a repository-level
ApiKeyRepositoryError::NotFound returned by repo.update into
ManageApiKeysError::NotFound (same as the earlier ok_or_else for find) instead
of letting the ? convert it into a generic Repository error; update the error
handling around the repo.update call (and the similar case at lines 138-139) to
match ApiKeyRepositoryError::NotFound => ManageApiKeysError::NotFound and
propagate other repository errors unchanged.
- Around line 119-122: The update path currently computes expires_at from
request.expires_at or existing.expires_at but does not validate it; replicate
the same validation used in create() by checking the resolved expires_at against
the current time (e.g., Utc::now()) and return the same error when it is in the
past. Apply this check in the update() flow immediately after computing let
expires_at = ... (using request.expires_at and existing.expires_at) and before
persisting changes so callers cannot move a key to a past expiration.
In `@crates/infrastructure/auth-sqlite/src/lib.rs`:
- Around line 272-281: The revoke implementation in async fn revoke is
overwriting revoked_at on repeated calls; change the UPDATE so revoked_at
remains the original value (e.g., use revoked_at = COALESCE(revoked_at, ?1))
and/or only update rows where is_active = 1 to make the operation idempotent. In
practice, update the SQL in revoke (the execute call in lib.rs) to "UPDATE
api_keys SET is_active = 0, revoked_at = COALESCE(revoked_at, ?1) WHERE id = ?2"
or add "AND is_active = 1" to the WHERE clause so subsequent revokes do not
change the original revoked_at timestamp. Ensure parameter binding still passes
revoked_at.to_rfc3339() and id.to_string().
In `@crates/infrastructure/transport-axum/tests/api_key_routes.rs`:
- Around line 61-77: Extend the existing
update_api_key_request_deserializes_correctly test for the three expiresAt
states so the tri-state contract is covered: keep the current null case, add a
case where expiresAt is omitted (ensure UpdateApiKeyRequestDto.expires_at ==
None meaning the field was not provided) and add a case where expiresAt is a
concrete timestamp string (ensure UpdateApiKeyRequestDto.expires_at ==
Some(Some(<expected timestamp parsing result>))). Locate the test function
update_api_key_request_deserializes_correctly and the UpdateApiKeyRequestDto
deserialization assertion for dto.expires_at, then add the two additional JSON
inputs (one without the expiresAt key, one with a timestamp value) and
corresponding assertions verifying omitted vs null vs concrete timestamp
behavior.
- Around line 80-89: Update the test
create_api_key_response_serializes_correctly so the plaintext fixture is
obviously fake (e.g., use a test prefix like "rk_test_...") and add an assertion
that the serialized JSON does not expose the sensitive keyHash field; locate the
test and the DTO types CreateApiKeyResponseDto and ApiKeyRecordResponseDto,
change the plaintext_key string to a clearly fake value and add an assertion
that json["key"] does not contain "keyHash" (or that json["key"]["keyHash"] is
null/absent) while keeping the existing checks for plaintextKey and key.id.
In `@dev/e2e/run-api-keys-e2e.sh`:
- Around line 20-23: The script hardcodes machine-specific absolute paths for
TEST_CONFIG and DASHBOARD_DIR which breaks on other machines/CI; update the
runner (the top-level script where TEST_CONFIG and DASHBOARD_DIR are set) to
compute the repo root relative to the script location (e.g., resolve dirname
"$0" and traverse to the repo root) and then construct TEST_CONFIG and
DASHBOARD_DIR from that root (replace the "/Users/acosta/..." literals with path
joins from the computed repo root); apply the same change for the other
occurrences mentioned (lines setting TEST_CONFIG/DASHBOARD_DIR around 51-52).
- Around line 131-133: The script's manual mode uses `wait` which returns
immediately because there are no background jobs, so replace the `wait` call
with a blocking construct (e.g., tailing the container logs or a sleep loop) so
the process actually blocks until Ctrl+C; specifically change the non-`--test`
path to call `docker logs -f "$CONTAINER_NAME"` (or a loop like `while true; do
sleep 1; done`) and keep the existing `trap cleanup INT TERM` so `cleanup` is
invoked on interrupt.
- Line 19: The test runner sets API_PORT=8081 but never exports a Playwright
base URL, so Playwright tests (and the login/API-key helpers used in --test
mode) fall back to localhost:8080; export an environment variable pointing
Playwright to the real backend (e.g., set and export PLAYWRIGHT_API_BASE or
PLAYWRIGHT_BASE_URL to "http://localhost:${API_PORT}" before invoking npx
playwright test) so the helpers use the correct host; ensure the exported
variable name matches what the test helpers/readers expect and is present in the
script that starts Playwright.
In `@docs/api.md`:
- Line 248: The step for the CSRF flow mixes Chinese with English; update the
text for the `GET /login` step so it is fully English and consistent (e.g., "GET
/login — obtain CSRF token and `csrf_token` cookie") by editing the line
describing the `GET /login` endpoint and its mention of `csrf_token`.
- Line 281: Update the sentence in the API docs that currently reads "Requires
authenticated session" to use standard phrasing: change it to "Requires an
authenticated session." Locate the string in docs/api.md (the "List all API keys
with pagination." endpoint description) and replace the sentence exactly to
ensure consistent documentation tone.
In `@docs/configuration.md`:
- Line 101: Update the docs line that currently says "Requires `ROOK_CONFIG` and
`API_KEY_HASH_SECRET` environment variables to be set" to clarify that
`API_KEY_HASH_SECRET` is only required when API keys are enabled; specifically
state that `API_KEY_HASH_SECRET` is conditional on `auth.api_keys.enabled =
true` (per apps/rook/src/di.rs) while `ROOK_CONFIG` remains always required, so
readers know to omit `API_KEY_HASH_SECRET` unless `auth.api_keys.enabled` is
turned on.
In `@openspec/changes/archive/2026-05-31-api-key-crud/design.md`:
- Around line 149-170: The PaginatedResponse<T> DTO currently exposes the list
under the field name `data` (struct PaginatedResponse and its new(...)
constructor) while other API specs use `keys`; update this file so the response
contract matches the rest of the spec by renaming the field to `keys` everywhere
(struct definition, generic parameter uses, and the PaginatedResponse::new(...)
constructor) and ensure PaginationMeta (total, limit, offset) remains unchanged;
make the same rename in the other impacted block referenced (lines ~269-287) so
all list responses use the same `keys` property.
- Around line 81-87: Update the example route path to use Axum 0.8 path syntax
so it matches the implemented routes: change the HTTP DELETE example from the
stale `/api/api-keys/:id` form to the brace style `/api/api-keys/{id}` used by
the codebase; ensure the rest of the flow in the doc (ApiKeyId from path,
ManageApiKeys::revoke(), SqliteApiKeyRepository::revoke(), UPDATE ... WHERE id =
:id, HTTP 204) remains unchanged and still references ApiKeyId and the revoke
methods consistently.
- Around line 303-319: The table row "Revoke already-revoked" in the Error
Responses section conflicts with the implementation: the revoke handler returns
204 No Content for successful revocations (including idempotent cases). Update
that row to use HTTP 204 (No Content) as the canonical status to match the
revoke handler behavior (keep the Code column as-is or empty per convention) so
the spec and the revoke handler are consistent.
In `@openspec/changes/archive/2026-05-31-api-key-crud/proposal.md`:
- Around line 51-54: Update all route path parameter syntax from the colon form
to Axum-style braces so the proposal matches the code; specifically replace
occurrences like `/api/api-keys/:id` with `/api/api-keys/{id}` (and any other
`:param` usages noted around the second occurrence at lines referenced, e.g.,
the routes in the GET/PUT/DELETE rows and the entries mentioned at 78-79) so
every endpoint uses `{id}` consistently throughout the document.
- Around line 59-63: The spec is inconsistent about API key generation (mentions
both 32 bytes + "rook_fake_" and 24 bytes + "rk-"); pick one canonical format
and update all instances in this proposal (including the example, the "Format"
bullet, the "Key prefix" rule, and the note that storage uses HMAC-SHA256 with
API_KEY_HASH_SECRET) to match that single choice; ensure the example key, the
byte count, the visible prefix string (either "rook_fake_" or "rk-"), and any
mentions elsewhere in the doc (also at the other occurrences around the same
section) are all changed to the chosen canonical format and consistent with how
auth_sqlite computes/stores the HMAC and key_prefix.
In `@openspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-dashboard.md`:
- Around line 183-196: The API example routes use colon-style parameters; update
the PUT and DELETE examples to use brace-style parameter syntax to match the
backend routing. Replace the occurrences of "PUT /api/api-keys/:id" and "DELETE
/api/api-keys/:id" in api-key-dashboard.md with "PUT /api/api-keys/{id}" and
"DELETE /api/api-keys/{id}" respectively, and scan for any other ":id" examples
in the same file to ensure consistent use of "{id}" in the dashboard API docs.
In `@openspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-domain.md`:
- Around line 30-33: The spec's scope parse error uses the wrong variant name:
change the parse error variant listed for ApiKeyScope from `InvalidTier(String)`
to a scope-specific name such as `InvalidScope(String)` so the domain contract
matches the actual validation semantics; update the error list where
`EmptyScope` and `InvalidTier(String)` appear to instead show `EmptyScope` —
scope string is empty or whitespace-only and `InvalidScope(String)` — scope
value not in allowlist (and also search for any references to `InvalidTier` in
ApiKeyScope parsing/validation code and rename them to `InvalidScope` to keep
symbol names consistent).
In `@openspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-transport.md`:
- Around line 235-257: The error examples under the "Error Response Format"
section currently show { "error": { "code", "message" } } which conflicts with
other specs in this PR; update the examples and the descriptive text in that
section (and any other occurrences in this file) to use the canonical error
payload used across the PR (replace the JSON blocks under the "Error Response
Format" heading with the agreed shape and adjust field names/structure
accordingly so they match the rest of the docs).
- Around line 17-20: Update the OpenSpec transport routes to use Axum-compatible
path parameter syntax by replacing any `/api/api-keys/:id` occurrences with
`/api/api-keys/{id}`; specifically change the routes tied to the handlers
get_api_key, update_api_key, and revoke_api_key and any other `:id` occurrences
in this file (the other occurrences correspond to the same API-key endpoint
variations) so all path parameters use `{id}` instead of `:id`.
In `@openspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-usecases.md`:
- Around line 95-108: The spec for list_paginated is out of sync: update the
documented contract to match the implementation by changing the parameter types
from Option<usize> to concrete i64 (or the exact types used in the impl) and
replace the in-memory pagination steps with the actual behavior (call
repo.list_paginated(limit, offset) and repo.count() to return (records, total));
mention defaulting/validation rules used by the implementation (defaults or
bounds) and reference the function name list_paginated and the repository
methods repo.list_paginated and repo.count in the description so future edits
reflect the real flow.
In `@openspec/changes/archive/2026-05-31-api-key-crud/tasks.md`:
- Around line 275-277: Update the task doc that currently points contributors to
src/stores (and suggests apiKeys.js); change the implementation reference to the
actual composable name useApiKeys (useApiKeys.ts) so the task points to the
composable that contains the CRUD methods (search for the task entry mentioning
"Add API client methods to dashboard store" and replace the incorrect
src/stores/apiKeys hint with the composable reference useApiKeys.ts).
In `@openspec/changes/archive/2026-05-31-api-key-crud/verify-report.md`:
- Line 226: The report contains an incorrect artifact path string pointing to
`openspec/changes/api-key-crud/verify-report.md`; update that reference in this
file (the `verify-report.md` under
`openspec/changes/archive/2026-05-31-api-key-crud/`) to the correct archived
path `openspec/changes/archive/2026-05-31-api-key-crud/verify-report.md` so the
link/path at the reported location is accurate.
In `@openspec/specs/api-key-domain/spec.md`:
- Around line 117-119: The docs show an `rk-` example while other
transport/dashboard specs use `rook_fake_`; update this spec's example lines
(the Algorithm/Example output/Key prefix entries referencing the `rk-` prefix)
to use the canonical `rook_fake_...` example format and ensure the `key_prefix`
description (first 8 chars) matches the new example; edit the Example output and
Key prefix strings where `rk-` is mentioned so they align with the
transport/dashboard examples.
In `@openspec/specs/api-key-repository/spec.md`:
- Around line 13-39: The ApiKeyRepositoryPort trait is missing the new
pagination methods used elsewhere; update the trait ApiKeyRepositoryPort to
include async fn list_paginated(...) and async fn count(...) with the same error
type ApiKeyRepositoryError and appropriate parameter/return types matching other
use-cases (e.g. a paginated list method that accepts limit/offset or page/size
and returns Result<Vec<ApiKeyRecord>, ApiKeyRepositoryError>, and a count method
that returns Result<u64, ApiKeyRepositoryError> or Result<usize,
ApiKeyRepositoryError> to match existing conventions) so implementations will
satisfy the repository/use-case contract.
In `@openspec/specs/api-key-usecases/spec.md`:
- Around line 73-76: The spec uses the raw key example prefix `rk-<encoded>`
which conflicts with the transport/dashboard examples; update the raw key
example to match the standardized prefix used elsewhere (e.g.,
`rook_fake_<encoded>`) so examples are consistent, and ensure the surrounding
steps that reference the raw key (steps generating 24 random bytes → base64url →
raw key, the HMAC-SHA256 `key_hash` computation, `key_prefix` extraction, and
the `ApiKeyId::new(format!("key_{}", uuid::Uuid::new_v4().simple()))` line)
remain unchanged except for replacing `rk-<encoded>` with the standardized
prefix.
- Around line 95-108: The spec's list_paginated use-case is outdated: instead of
fetching all records and slicing locally as described, update the documentation
to reflect the repository-level pagination contract introduced in this PR by
calling repo.list_paginated(limit, offset) and repo.count() (or equivalent
methods) to obtain the paginated slice and total separately; modify the steps
and signature description for list_paginated to state it delegates to the
repository's list_paginated and count methods (include function names
repo.list_paginated and repo.count or the repo interface names) and remove the
local slicing algorithm and mention of repo.list() as the source of truth.
---
Outside diff comments:
In `@apps/rook/dashboard/src/lib/api.ts`:
- Around line 132-145: The shared request helper function request currently
sends cookies but omits the CSRF token header; update request to read the CSRF
token from the double-submit cookie (e.g., from document.cookie or the existing
cookie helper) and set the 'X-CSRF-Token' header on state-changing requests
(POST, PUT, DELETE, PATCH) before calling fetch so those routes are not rejected
when CSRF enforcement is enabled; keep existing Content-Type merging behavior
and credentials: 'include'.
In `@openspec/changes/archive/2026-05-31-api-key-crud/state.yaml`:
- Around line 1-202: The file currently named state.yaml contains Markdown
(tables, headings like "Change: api-key-crud" and "## Metadata") rather than
valid YAML; either convert the document into a proper YAML structure (top-level
mappings for fields such as change, current_phase, completed, next, updated,
metadata, stack_detected, specs, etc., replacing Markdown tables with YAML
maps/lists) or rename/move the file to a .md and create a small state.yaml with
the canonical YAML keys (e.g., change: api-key-crud, current_phase: verify,
updated: 2026-05-31, completed: [...]) so YAML consumers can parse it (locate
this in the file containing the "Change: api-key-crud" heading and the "##
Metadata" table).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ef8d4cb-5058-4497-8f3e-36037c98eeaf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
.atl/skill-registry.mdDockerfile.devapps/rook/Cargo.tomlapps/rook/dashboard/e2e/api-keys.spec.tsapps/rook/dashboard/src/components/NavMain.vueapps/rook/dashboard/src/components/NavSecondary.vueapps/rook/dashboard/src/components/ui/carousel/CarouselContent.vueapps/rook/dashboard/src/components/ui/locale-switcher/LocaleSwitcher.vueapps/rook/dashboard/src/composables/useApiKeys.tsapps/rook/dashboard/src/config/endpoints.tsapps/rook/dashboard/src/i18n/index.tsapps/rook/dashboard/src/lib/api.tsapps/rook/dashboard/src/types/lucide-icons.d.tsapps/rook/dashboard/src/types/unovis.d.tsapps/rook/dashboard/src/views/ApiKeysView.vueapps/rook/dashboard/src/views/EndpointsView.vueapps/rook/dashboard/src/views/ProvidersView.vueapps/rook/dashboard/src/views/sidebar/index.vueapps/rook/dashboard/tsconfig.jsonapps/rook/src/di.rsapps/rook/src/main.rscrates/application/rook-usecases/Cargo.tomlcrates/application/rook-usecases/src/authenticate_client_api.rscrates/application/rook-usecases/src/lib.rscrates/application/rook-usecases/src/manage_api_keys.rscrates/domain/rook-core/src/ports.rscrates/infrastructure/auth-sqlite/src/lib.rscrates/infrastructure/transport-axum/src/api_key_dto.rscrates/infrastructure/transport-axum/src/authz.rscrates/infrastructure/transport-axum/src/handlers/api_key.rscrates/infrastructure/transport-axum/src/handlers/mod.rscrates/infrastructure/transport-axum/src/lib.rscrates/infrastructure/transport-axum/src/provider_routes.rscrates/infrastructure/transport-axum/src/routes.rscrates/infrastructure/transport-axum/tests/api_key_routes.rsdev/e2e/run-api-keys-e2e.shdev/test-configs/rook-api-keys-test.tomldocs/api.mddocs/configuration.mdopenspec/changes/archive/2026-05-31-api-key-crud/design.mdopenspec/changes/archive/2026-05-31-api-key-crud/proposal.mdopenspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-dashboard.mdopenspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-domain.mdopenspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-repository.mdopenspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-transport.mdopenspec/changes/archive/2026-05-31-api-key-crud/specs/api-key-usecases.mdopenspec/changes/archive/2026-05-31-api-key-crud/state.yamlopenspec/changes/archive/2026-05-31-api-key-crud/tasks.mdopenspec/changes/archive/2026-05-31-api-key-crud/verify-report.mdopenspec/specs/api-key-dashboard/spec.mdopenspec/specs/api-key-domain/spec.mdopenspec/specs/api-key-repository/spec.mdopenspec/specs/api-key-transport/spec.mdopenspec/specs/api-key-usecases/spec.md
💤 Files with no reviewable changes (2)
- apps/rook/dashboard/src/views/sidebar/index.vue
- apps/rook/dashboard/src/views/EndpointsView.vue
| // Create a key to edit | ||
| await createApiKeyViaApi(page, 'key-to-edit', ['read'], 'free') |
There was a problem hiding this comment.
Use unique API-key labels per test.
Each beforeEach adds another key-to-edit / key-to-revoke row and never removes the previous one. By the second test in each suite, the downstream row locators match multiple rows and strict-mode clicks become flaky.
Also applies to: 278-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/rook/dashboard/e2e/api-keys.spec.ts` around lines 214 - 215, The tests
create API keys using the fixed labels 'key-to-edit' and 'key-to-revoke', which
accumulate across beforeEach runs and cause locator ambiguity; change each
createApiKeyViaApi(...) call to use a test-scoped unique label (e.g., append a
timestamp/random suffix) and store that label in a variable used by the rest of
the test so all subsequent locator queries (the edit/revoke flows) target the
unique label; update every createApiKeyViaApi invocation and any locator
references that expect 'key-to-edit'/'key-to-revoke' to use the stored unique
label.
| function handleChange(value: AcceptableValue) { | ||
| if (typeof value === 'string') { | ||
| setLocale(value as 'en' | 'es') | ||
| } |
There was a problem hiding this comment.
Validate locale value before casting to supported locale union.
A plain string check still allows unsupported values (e.g., 'fr') into setLocale via cast.
✅ Proposed fix
function handleChange(value: AcceptableValue) {
- if (typeof value === 'string') {
- setLocale(value as 'en' | 'es')
+ if (value === 'en' || value === 'es') {
+ setLocale(value)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function handleChange(value: AcceptableValue) { | |
| if (typeof value === 'string') { | |
| setLocale(value as 'en' | 'es') | |
| } | |
| function handleChange(value: AcceptableValue) { | |
| if (value === 'en' || value === 'es') { | |
| setLocale(value) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/rook/dashboard/src/components/ui/locale-switcher/LocaleSwitcher.vue`
around lines 20 - 23, The current handleChange(value: AcceptableValue) only
checks typeof string then casts to 'en'|'es' and can pass unsupported locales;
update handleChange to explicitly validate that value is one of the supported
locale strings (e.g., check value === 'en' || value === 'es' or use a small
allowedLocales set/array) before calling setLocale, and otherwise ignore/handle
invalid values (fallback to a default or no-op). Reference: function
handleChange and setLocale.
| export const VisCrosshair: Component<VisCrosshairProps> | ||
| export const VisTooltip: Component<VisTooltipProps> | ||
| export const Chart: Component | ||
| export default { Chart, VisCrosshair, VisTooltip } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vue 3 type definitions, is Componentfromvuegeneric (e.g.,Component), or should DefineComponentbe used for typed component declarations in.d.ts files?
💡 Result:
In Vue 3, DefineComponent is the intended type to use for typed component declarations in.d.ts files, as it provides the necessary type inference for props, emits, and internal component options [1]. While Component exists as a type in Vue 3, it is a broad, internal utility type used by the framework to represent various component definition types (such as concrete components or constructor-based components) [2]. It is not designed to be used as a simple generic like Component to define custom component types [2]. For declaring component types in.d.ts files, you should use DefineComponent [1]. This type is highly generic, allowing you to specify props, raw bindings, data, computed options, methods, and emits [1]. Common usage in a definition file: export const MyComponent: DefineComponent<{ propName: string }, {}, {}>; If you are working with Single-File Components (SFCs), the recommended approach is to let the Vue Language Tools (Volar/vue-tsc) generate the necessary.d.ts files for you [3]. When manually authoring types for components or complex generic components, DefineComponent remains the standard API for ensuring that TypeScript can properly infer the component's internal structure [1][4][5].
Citations:
- 1: https://github.com/vuejs/vue/blob/9e887079/types/v3-define-component.d.ts
- 2: https://github.com/vuejs/core/blob/main/packages%2Fruntime-core%2Fsrc%2Fcomponent.ts
- 3: https://stackoverflow.com/questions/57998121/generate-d-ts-file-for-vue-components-written-in-typescript
- 4: https://coreui.io/answers/how-to-use-definecomponent-in-vue-3/
- 5: https://vuejs.org/guide/typescript/overview
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion of the .d.ts being reviewed
sed -n '1,120p' apps/rook/dashboard/src/types/unovis.d.ts | nl -baRepository: dallay/cortex
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk '{printf "%4d: %s\n", NR, $0}' apps/rook/dashboard/src/types/unovis.d.ts | sed -n '1,120p'Repository: dallay/cortex
Length of output: 800
🌐 Web query:
Vue 3 TypeScript types: definition of Componentexported from 'vue' (is it generic? what is its signature?) vsDefineComponent``
💡 Result:
In Vue 3, 'Component' and 'DefineComponent' serve different purposes regarding type definitions [1]. Component The Component type exported from 'vue' is a union type used to represent any valid Vue component definition [1]. It acts as a broad "catch-all" for various component shapes that Vue can consume, such as objects containing component options or constructor functions [1]. Signature and Generics: Yes, it is generic. Its definition typically looks like this [1]: type Component<PropsOrInstance = any, RawBindings = any, D = any, C extends ComputedOptions = ComputedOptions, M extends MethodOptions = MethodOptions, E extends EmitsOptions = {}, S extends Record<string, any> = any> =... It is defined as a union of ConcreteComponent (which includes the generic parameters for props, data, methods, etc.) and ComponentPublicInstanceConstructor [1]. It is designed for internal use and library authors who need a type that accepts any valid component [1]. DefineComponent DefineComponent is a specific, complex type returned by the defineComponent utility function [2][1]. It is not just a type for an arbitrary component; it represents the result of the component definition process, providing rich type inference for the component's internal options (props, data, setup, etc.) [2][3][4]. Signature: DefineComponent is a highly generic type with numerous parameters that capture the internal structure of a component [2][5]: type DefineComponent<PropsOrPropOptions = {}, RawBindings = {}, D = {}, C extends ComputedOptions =..., M extends MethodOptions =...,...> = ComponentPublicInstance<...> Key Differences 1. Purpose: Component is a broad type for "anything that can be a component" [1]. DefineComponent is the specific, typed return value that enables TypeScript to know exactly what a component's props, emits, and data look like [2][3]. 2. Usage: You rarely use DefineComponent directly in application code. Instead, you use the defineComponent function to define your component, and TypeScript automatically infers the DefineComponent type for you [3][4]. You use the Component type when you are writing higher-order components or library code that needs to accept any component as an input [1].
Citations:
- 1: https://github.com/vuejs/core/blob/main/packages%2Fruntime-core%2Fsrc%2Fcomponent.ts
- 2: https://github.com/vuejs/vue/blob/main/types/v3-define-component.d.ts
- 3: https://coreui.io/answers/how-to-use-definecomponent-in-vue-3/
- 4: https://www.telerik.com/blogs/definecomponent-vue-3-pure-magic
- 5: https://github.com/vuejs/vue/blob/9e887079/types/v3-define-component.d.ts
🌐 Web query:
TypeScript declaration files (.d.ts): is export default { ... }valid syntax inside adeclare moduleblock, or must it bedeclare const _default+export default _default?
💡 Result:
In a TypeScript declare module block, using export default { ... } with an object literal directly is not valid syntax because the export default clause requires an identifier or a qualified name to reference an existing value [1]. To correctly export a default value from a declare module block, you must first declare the value (as a const, function, or class) and then export that identifier [1]. Recommended approach: declare module 'your-module' { const _default: { prop: string; method: void; }; export default _default; } While you may see examples online using export default function ... or export default class ... inside a declare module block [2][3], these are only valid because the function or class name acts as an identifier. You cannot pass an object literal directly to export default in an ambient context [1]. If you need to export an object as the default, the two-step process of declaring a variable and then exporting that variable is the required pattern [1].
Citations:
- 1: https://stackoverflow.com/questions/57131864/how-to-declare-a-module-in-typescript-with-an-object-as-default-export
- 2: https://stackoverflow.com/questions/39109027/write-a-declaration-file-for-a-default-export-module
- 3: https://oneuptime.com/blog/post/2026-01-24-typescript-declaration-files/view
Fix invalid default export in @unovis/vue ambient module typings
Component<...> is a generic type in Vue 3, but export default { Chart, VisCrosshair, VisTooltip } inside a declare module is not valid .d.ts ambient-declaration syntax and can break type-checking.
💡 Proposed fix
declare module '`@unovis/vue`' {
import type { Component } from 'vue'
interface VisCrosshairProps {
color?: string
width?: number
[key: string]: unknown
}
interface VisTooltipProps {
color?: string
[key: string]: unknown
}
export const VisCrosshair: Component<VisCrosshairProps>
export const VisTooltip: Component<VisTooltipProps>
export const Chart: Component
- export default { Chart, VisCrosshair, VisTooltip }
+ declare const _default: {
+ Chart: typeof Chart
+ VisCrosshair: typeof VisCrosshair
+ VisTooltip: typeof VisTooltip
+ }
+ export default _default
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const VisCrosshair: Component<VisCrosshairProps> | |
| export const VisTooltip: Component<VisTooltipProps> | |
| export const Chart: Component | |
| export default { Chart, VisCrosshair, VisTooltip } | |
| export const VisCrosshair: Component<VisCrosshairProps> | |
| export const VisTooltip: Component<VisTooltipProps> | |
| export const Chart: Component | |
| declare const _default: { | |
| Chart: typeof Chart | |
| VisCrosshair: typeof VisCrosshair | |
| VisTooltip: typeof VisTooltip | |
| } | |
| export default _default |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/rook/dashboard/src/types/unovis.d.ts` around lines 19 - 22, The default
export as an object literal in the ambient module is invalid; replace it by
declaring a const typed object and exporting that const as default. Concretely,
keep the existing exported symbols (VisCrosshair, VisTooltip, Chart) and add a
declaration like `const _default: { Chart: typeof Chart; VisCrosshair: typeof
VisCrosshair; VisTooltip: typeof VisTooltip }` then `export default _default`;
update the existing `export const VisCrosshair`, `export const VisTooltip`, and
`export const Chart` declarations to be referenced by `typeof` in the default
const so the ambient typings remain valid.
|
|
||
| ## Artifacts | ||
|
|
||
| - `openspec/changes/api-key-crud/verify-report.md` (this file) |
There was a problem hiding this comment.
Fix incorrect artifact path in report.
Line 226 references openspec/changes/api-key-crud/verify-report.md, but this report lives under openspec/changes/archive/2026-05-31-api-key-crud/verify-report.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/archive/2026-05-31-api-key-crud/verify-report.md` at line
226, The report contains an incorrect artifact path string pointing to
`openspec/changes/api-key-crud/verify-report.md`; update that reference in this
file (the `verify-report.md` under
`openspec/changes/archive/2026-05-31-api-key-crud/`) to the correct archived
path `openspec/changes/archive/2026-05-31-api-key-crud/verify-report.md` so the
link/path at the reported location is accurate.
| - **Algorithm**: 24 random bytes → base64url (no padding) → `rk-` prefix | ||
| - **Example output**: `rk-a3f8b2c1d0e9f2a3b4c5d6e7f8a9b0c` | ||
| - **Key prefix**: First 8 chars stored as `key_prefix` for UI display |
There was a problem hiding this comment.
Unify raw key prefix examples across specs.
Lines 117–119 document rk-..., while transport/dashboard specs in this change use rook_fake_... examples. Please standardize on one example prefix to avoid conflicting contract documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/api-key-domain/spec.md` around lines 117 - 119, The docs show
an `rk-` example while other transport/dashboard specs use `rook_fake_`; update
this spec's example lines (the Algorithm/Example output/Key prefix entries
referencing the `rk-` prefix) to use the canonical `rook_fake_...` example
format and ensure the `key_prefix` description (first 8 chars) matches the new
example; edit the Example output and Key prefix strings where `rk-` is mentioned
so they align with the transport/dashboard examples.
| pub trait ApiKeyRepositoryPort: Send + Sync { | ||
| // Auth lookup — used by middleware on every API request | ||
| async fn find_active_by_hash( | ||
| &self, | ||
| hash: &str, | ||
| ) -> Result<Option<ApiKeySubject>, ApiKeyRepositoryError>; | ||
|
|
||
| async fn record_last_used( | ||
| &self, | ||
| id: &ApiKeyId, | ||
| used_at: DateTime<Utc>, | ||
| ) -> Result<(), ApiKeyRepositoryError>; | ||
|
|
||
| // Admin CRUD | ||
| async fn list(&self) -> Result<Vec<ApiKeyRecord>, ApiKeyRepositoryError>; | ||
| async fn find(&self, id: &ApiKeyId) -> Result<Option<ApiKeyRecord>, ApiKeyRepositoryError>; | ||
| async fn create(&self, record: &ApiKeyRecord) -> Result<(), ApiKeyRepositoryError>; | ||
| async fn update(&self, record: &ApiKeyRecord) -> Result<(), ApiKeyRepositoryError>; | ||
| async fn delete(&self, id: &ApiKeyId) -> Result<(), ApiKeyRepositoryError>; | ||
|
|
||
| // Soft revoke | ||
| async fn revoke( | ||
| &self, | ||
| id: &ApiKeyId, | ||
| revoked_at: DateTime<Utc>, | ||
| ) -> Result<(), ApiKeyRepositoryError>; | ||
| } |
There was a problem hiding this comment.
Repository port contract is missing pagination methods added by this PR.
The interface here does not include list_paginated() and count(), but those methods are part of this change’s repository/use-case contract. The spec should reflect the actual required port surface to prevent incorrect future implementations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/api-key-repository/spec.md` around lines 13 - 39, The
ApiKeyRepositoryPort trait is missing the new pagination methods used elsewhere;
update the trait ApiKeyRepositoryPort to include async fn list_paginated(...)
and async fn count(...) with the same error type ApiKeyRepositoryError and
appropriate parameter/return types matching other use-cases (e.g. a paginated
list method that accepts limit/offset or page/size and returns
Result<Vec<ApiKeyRecord>, ApiKeyRepositoryError>, and a count method that
returns Result<u64, ApiKeyRepositoryError> or Result<usize,
ApiKeyRepositoryError> to match existing conventions) so implementations will
satisfy the repository/use-case contract.
| 1. Generate 24 random bytes → base64url → `rk-<encoded>` (raw key) | ||
| 2. Compute HMAC-SHA256 of raw key with `hash_secret` → `key_hash` | ||
| 3. Extract first 8 chars → `key_prefix` | ||
| 4. Generate `ApiKeyId::new(format!("key_{}", uuid::Uuid::new_v4().simple()))` |
There was a problem hiding this comment.
Standardize raw key prefix examples with the other specs.
Lines 73–76 describe rk-<encoded>, which conflicts with the rook_fake_... examples used in transport/dashboard docs for this same feature set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/api-key-usecases/spec.md` around lines 73 - 76, The spec uses
the raw key example prefix `rk-<encoded>` which conflicts with the
transport/dashboard examples; update the raw key example to match the
standardized prefix used elsewhere (e.g., `rook_fake_<encoded>`) so examples are
consistent, and ensure the surrounding steps that reference the raw key (steps
generating 24 random bytes → base64url → raw key, the HMAC-SHA256 `key_hash`
computation, `key_prefix` extraction, and the `ApiKeyId::new(format!("key_{}",
uuid::Uuid::new_v4().simple()))` line) remain unchanged except for replacing
`rk-<encoded>` with the standardized prefix.
| pub async fn list_paginated( | ||
| &self, | ||
| limit: Option<usize>, | ||
| offset: Option<usize>, | ||
| ) -> ManageApiKeysResult<(Vec<ApiKeyRecord>, usize)> | ||
| ``` | ||
|
|
||
| **Steps**: | ||
|
|
||
| 1. Call `repo.list()` to get all records | ||
| 2. Compute `total = all_records.len()` | ||
| 3. Apply `offset` (default 0) and `limit` (default 20) | ||
| 4. Return `(paginated_slice, total)` | ||
|
|
There was a problem hiding this comment.
Use-case pagination contract is out of sync with the implemented design.
This section says list_paginated calls repo.list() and slices locally, but this PR’s contract introduces repository-level list_paginated() and count() for pagination. The spec should be updated to match that source of truth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/specs/api-key-usecases/spec.md` around lines 95 - 108, The spec's
list_paginated use-case is outdated: instead of fetching all records and slicing
locally as described, update the documentation to reflect the repository-level
pagination contract introduced in this PR by calling repo.list_paginated(limit,
offset) and repo.count() (or equivalent methods) to obtain the paginated slice
and total separately; modify the steps and signature description for
list_paginated to state it delegates to the repository's list_paginated and
count methods (include function names repo.list_paginated and repo.count or the
repo interface names) and remove the local slicing algorithm and mention of
repo.list() as the source of truth.
- Remove unused getAuthCookies helper (CodeQL) - Fix CarouselContent ref destructuring (_carouselRef -> carouselRef) - Clone scopes array in openEditModal to prevent mutation - Add CSRF token header for state-changing API requests - Make revoke idempotent with COALESCE, preserve original revoked_at - Change test fixtures from rk-live to rook_test prefix - Fix E2E script: relative paths, export API_PORT, block on docker logs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Summary
Full API key lifecycle management feature:
Backend
Frontend
Tests
CLI and Dev
Documentation
Bug Fixes
Testing
All tests pass:
Notes