fix(rook): resolve block_on deadlock, fix dashboard route, add Docker e2e infra - #36
Merged
Conversation
- Make RookContainer::build() async and use tokio::join!() to run
registry refresh and admin user init concurrently, avoiding
Handle::current().block_on() deadlock in multi-thread runtime
- Fix dashboard wildcard route: /*path -> {*path} for Axum 0.8.x
- Add OAuth credentials test (8.9) for ManageConnections::create
- Dockerfile.dev: multi-stage build with Node.js 22 + pnpm for ARM64 (linux/arm64/v8) - .dockerignore: exclude node_modules, target/, dist/, .git/ - dev/: Dockerfiles for Ubuntu/Alpine, docker-compose, e2e-test script - Dockerfile.dev.alpine: Alpine-based variant - dashboard/package-lock.json: npm lockfile for dashboard deps
…rchitecture - Archive change dynamic-provider-registry with full verify-report, tasks, design - Add Runtime Provider Registry section to ARCHITECTURE.md documenting the SQLite-backed dynamic registry pattern (FallbackRouter + ManageConnections) - Sync delta specs to openspec/specs/dynamic-provider-registry/
|
Thank you for contributing to this project with this PR, welcome to the community and the amazing world of open source! |
Contributor
|
Warning Review limit reached
More reviews will be available in 37 minutes and 16 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 delivers three major features plus extensive specification documentation: (1) comprehensive shadcn-vue developer skills covering CLI, component composition rules, theming, and MCP; (2) dynamic provider registry refactored from TOML to SQLite with async DI initialization, concurrent startup, and registry refresh on CRUD; (3) Docker/e2e infrastructure with multi-stage builds, Compose services, automated testing; plus PWA enhancements and specification archives for completed security authz and provider connections features. ChangesFrontend Platform shadcn-vue Skills Documentation
Dynamic Provider Registry and DI Refactoring
Docker and Development Infrastructure
Dashboard PWA Enhancements
Specification and Change Documentation
🎯 2 (Simple) | ⏱️ ~15 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- The commit SHA 9ab91c80e06747c8d9324b3b2d8aa442c6fb317c no longer exists (404) - Use @master which always resolves to latest stable - Add if: secrets.SONAR_TOKEN != '' to skip gracefully when token not configured
![semgrep-code-dallay[bot]](https://avatars.githubusercontent.com/u/47160086?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/application/rook-usecases/src/manage_connections.rs (1)
95-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report CRUD failure after the SQLite write already committed.
create(),update(), anddelete()now return therefresh_registry()error after the repository mutation succeeds. That makes the HTTP layer surface a failed write even though the row has already been created/updated/deleted, which is a retry hazard for these non-idempotent endpoints.Either treat the refresh failure as post-commit recovery work and still return success, or introduce an explicit partial-success/compensation path. Returning a plain error here misrepresents the persisted state.
Also applies to: 147-151, 154-160
🤖 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 `@crates/application/rook-usecases/src/manage_connections.rs` around lines 95 - 97, The repository mutation (self.repo.create / update / delete) already commits before calling self.refresh_registry(), so do not propagate refresh_registry() failure as the overall error; instead treat refresh_registry() as post-commit recovery: catch or map its error, log or report a warning, and still return success (e.g., Ok(conn) or Ok(())) so the HTTP layer reflects the persisted change. Apply this change to the create/update/delete call sites in manage_connections.rs where you currently call self.repo.*.await? followed by self.refresh_registry().await? (including the other blocks referenced around lines 147-151 and 154-160). Keep the repository result path unchanged and only handle refresh_registry() errors non-fatally.
♻️ Duplicate comments (1)
openspec/changes/dynamic-provider-registry/spec.md (1)
79-79:⚠️ Potential issue | 🟠 MajorDuplicate: Specification contradicts implementation (R8 deviation).
Same issue as in
openspec/specs/dynamic-provider-registry/spec.md: Lines 79 and 612 state the registry is "always active" independent ofprovider_crud.enabled, but the implementation and verification report document that it IS gated behind this flag. The spec needs to be updated to match reality.Also applies to: 612-612
🤖 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/dynamic-provider-registry/spec.md` at line 79, The spec currently states the dynamic registry is "always active" independent of provider_crud.enabled but implementation and verification gate it behind provider_crud.enabled; update the specification text in both places that claim "always active" so it matches reality by stating the dynamic registry is enabled only when provider_crud.enabled is true (i.e., the dynamic registry is gated by the provider_crud feature flag), and ensure any related wording for provider_crud and the dynamic registry (references to provider_crud.enabled and provider_crud) is consistent across the document.
🤖 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 @.dockerignore:
- Around line 5-8: .dockerignore currently excludes target/, but dev/Dockerfile
expects target/debug/rook to be present (it runs COPY target/debug/rook
/usr/local/bin/rook), so remove target/ from .dockerignore (or specifically
allow target/debug/rook) so that the build context includes that binary; update
.dockerignore to stop ignoring target/ or add an explicit negation for
target/debug/rook so the COPY in dev/Dockerfile can succeed.
In `@apps/rook/src/di.rs`:
- Around line 120-121: The code currently treats refresh_result Err as a warning
and continues startup; instead, when refresh_registry() fails (refresh_result is
Err), log the error with tracing::error! and propagate the failure so the
process fails fast—e.g., return Err(e) (or use anyhow::bail!(e)) from the
enclosing function rather than tracing::warn!, ensuring startup aborts and
existing connections are not hidden; update the block that checks refresh_result
(and any caller expecting a Result) to propagate the error.
In `@crates/application/rook-usecases/src/manage_connections.rs`:
- Around line 1244-1300: The test update_preserves_credentials_when_not_provided
only checks the Credentials::ApiKey variant and should assert the actual
encrypted blob value to catch regressions in ManageConnections::update; change
the assertion to unwrap the Credentials::ApiKey payload and compare the api_key
string to "enc:v1:sk-original" (and in the related test that replaces keys
assert "enc:v1:sk-new-replacement"), locating the checks around the test
function update_preserves_credentials_when_not_provided and the similar test at
the other block, and ensure you validate the inner EncryptedBlob value rather
than using a wildcard match.
- Around line 2207-2224: The tests currently use MockRegistryWithProvider (and
EmptyRegistry) so refresh_registry() assertions don't depend on replace_all();
change the tests to use MockRegistry (not MockRegistryWithProvider or
EmptyRegistry), construct two connection entries where one builder/manager
deliberately fails and the other succeeds (e.g., keep SuccessProviderBuilder for
"openai-primary" and inject a failing builder or a FailDecryptKeyManager only
for the other connection), call
ManageConnections::new(...).refresh_registry().await.expect(...), then check
registry.get(...) to assert that only the successful provider was inserted by
replace_all(); also update the other two test blocks (the blocks around the
other ranges) the same way so partial-failure behavior is actually exercised.
In `@dev/Dockerfile.runtime`:
- Around line 18-20: The COPY uses a non-existent build stage name "builder"
(COPY --from=builder /app/target/debug/rook ...) so the image build will fail;
either add a multi-stage build stage named "builder" that compiles and outputs
/app/target/debug/rook (e.g., a FROM ... AS builder that builds the binary), or
change the --from value to the actual stage or image that contains the compiled
binary; update the Dockerfile.runtime accordingly so the referenced stage name
matches the one that produces /app/target/debug/rook.
In `@dev/Dockerfile.ubuntu`:
- Around line 21-39: The Dockerfile builds the binary in debug mode (RUN cargo
build --bin rook) but later tries to copy the release binary (COPY
--from=builder /app/target/release/rook ...) and uses an invalid “fallback” COPY
with shell redirection; fix by making the build produce a release binary and
removing the fallback: change RUN cargo build --bin rook to RUN cargo build
--bin rook --release, remove the erroneous fallback COPY line, and keep a single
COPY --from=builder /app/target/release/rook /usr/local/bin/rook (also ensure
the earlier build stage is named builder to match COPY --from=builder).
- Around line 15-22: The builder stage fails to include the workspace member and
uses invalid Dockerfile COPY fallback; update the build stage to copy the apps
directory (at least apps/rook and its Cargo.toml) into the build context so
cargo can find apps/rook, build the binary with cargo build --release (or run
cargo install --locked --path apps/rook if preferred), and in the runtime stage
remove the invalid COPY "fallback" logic and instead unambiguously COPY the
built release binary target/release/rook into the runtime image (or adjust to
copy the release path you produced), ensuring the runtime stage references the
same release artifact produced by the build stage (e.g., target/release/rook)
and not a debug-only build or shell-redirection hack.
In `@dev/e2e-test.sh`:
- Around line 85-87: `cmd_up()` is swallowing health-check failures in the
distro loop, so a container that never becomes healthy still lets `up`/`test`
continue. Update the `wait_healthy` call inside the `for distro in
"${DISTROS[@]}"` loop to propagate failures instead of forcing success, and
ensure the failure path aborts before printing the “Containers started…”
message. Use the existing `wait_healthy` and `cmd_up` flow to locate the change.
In `@Dockerfile.dev.alpine`:
- Around line 15-26: The Dockerfile currently copies Cargo.lock*, Cargo.toml,
rust-toolchain.toml and crates/ but omits the apps/ sources needed to build the
rook binary; before the RUN cargo build --bin rook step, add a COPY for the
application sources (either COPY apps/ ./apps/ or at minimum COPY apps/rook/
./apps/rook/) so that the rook crate (apps/rook/Cargo.toml and its src/) is
present when cargo builds the rook binary.
In `@openspec/ARCHITECTURE.md`:
- Line 135: Add an explicit "Implementation Note" or "Known Deviation" entry to
the dynamic-provider-registry specification that documents the R8 deviation
described in ARCHITECTURE.md: state that the registry is gated by the
configuration key provider_crud.enabled (which makes manage_connections None and
yields an empty registry when disabled) even though R8 expects the registry to
be "always active", and include the pragmatic justification already in
ARCHITECTURE.md so readers know this is an intentional, documented deviation.
In `@openspec/changes/archive/2026-05-31-dynamic-provider-registry/state.yaml`:
- Line 29: Update the inconsistent test count in the state record: replace the
current "Tests: 56 passed in rook-usecases, 5 passed in rook-core" entry with
the correct value that matches the other archive documents (change 56 to 73) so
the line reads "Tests: 73 passed in rook-usecases, 5 passed in rook-core";
locate the string in state.yaml (the "Tests:" entry for 2026-05-31) and make the
numeric correction.
In `@openspec/changes/archive/2026-05-31-dynamic-provider-registry/tasks.md`:
- Around line 73-79: The Phase 5 task checklist is inconsistent: tasks 5.6–5.12
are still unchecked but the verification and archive reports show those tests as
implemented and passing; update the checklist entries `5.6
refresh_registry_skips_inactive_connections`, `5.7
refresh_registry_decrypts_and_builds_provider`, `5.8
refresh_registry_partial_failure_keeps_valid_providers`, `5.9
refresh_registry_all_failures_results_in_empty_registry`, `5.10
create_calls_refresh_after_write`, `5.11 update_calls_refresh_after_write`, and
`5.12 delete_calls_refresh_after_write` in tasks.md to checked (change `[ ]` to
`[x]`) so the tasks list matches `verify-report.md` and `archive-report.md`;
after editing run a quick grep to confirm those test names appear as ✅ in the
verification files to ensure consistency.
In `@openspec/changes/dynamic-provider-registry/verify-report.md`:
- Line 6: The Verification Result currently reads "PASS WITH WARNINGS" while
WARNING `#1` flags that rule R8 (registry "always active") conflicts with the
implementation which gates activation behind provider_crud.enabled; update the
spec and verification state to eliminate this mismatch: either (A) update R8 in
the spec.md files to reflect the gating behavior (search for R8 and the phrase
"always active" and change wording to match provider_crud.enabled) then re-run
verification and mark as full "PASS", or (B) if you accept the divergence,
change the verification verdict string from "PASS WITH WARNINGS" to "CONDITIONAL
PASS - pending spec update" and add a clear note referencing R8 and
provider_crud.enabled documenting the approved divergence before archiving.
In `@openspec/specs/dynamic-provider-registry/spec.md`:
- Line 79: The spec claims the dynamic registry "is always active" independent
of provider_crud.enabled, but implementation (di.rs: manage_connections gated at
di.rs:54-76) and docs (ARCHITECTURE.md, verify-report.md R8) show it is
feature-gated; update spec.md to reflect reality by either adding an
"Implementation Notes" or "Deviations" section that documents the pragmatic
choice to gate the registry behind provider_crud.enabled (mention R8 and di.rs
manage_connections), or change R8's wording to state "The dynamic registry is
constructed when provider_crud.enabled = true" so the spec matches
ARCHITECTURE.md and the implementation.
---
Outside diff comments:
In `@crates/application/rook-usecases/src/manage_connections.rs`:
- Around line 95-97: The repository mutation (self.repo.create / update /
delete) already commits before calling self.refresh_registry(), so do not
propagate refresh_registry() failure as the overall error; instead treat
refresh_registry() as post-commit recovery: catch or map its error, log or
report a warning, and still return success (e.g., Ok(conn) or Ok(())) so the
HTTP layer reflects the persisted change. Apply this change to the
create/update/delete call sites in manage_connections.rs where you currently
call self.repo.*.await? followed by self.refresh_registry().await? (including
the other blocks referenced around lines 147-151 and 154-160). Keep the
repository result path unchanged and only handle refresh_registry() errors
non-fatally.
---
Duplicate comments:
In `@openspec/changes/dynamic-provider-registry/spec.md`:
- Line 79: The spec currently states the dynamic registry is "always active"
independent of provider_crud.enabled but implementation and verification gate it
behind provider_crud.enabled; update the specification text in both places that
claim "always active" so it matches reality by stating the dynamic registry is
enabled only when provider_crud.enabled is true (i.e., the dynamic registry is
gated by the provider_crud feature flag), and ensure any related wording for
provider_crud and the dynamic registry (references to provider_crud.enabled and
provider_crud) is consistent across the document.
🪄 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: 193b3575-bc26-4ae9-a1c8-cb038c48031e
⛔ Files ignored due to path filters (2)
apps/rook/dashboard/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.agents/skills/frontend-platform/shadcn-vue/SKILL.md.agents/skills/frontend-platform/shadcn-vue/cli.md.agents/skills/frontend-platform/shadcn-vue/customization.md.agents/skills/frontend-platform/shadcn-vue/mcp.md.agents/skills/frontend-platform/shadcn-vue/rules/composition.md.agents/skills/frontend-platform/shadcn-vue/rules/forms.md.agents/skills/frontend-platform/shadcn-vue/rules/icons.md.agents/skills/frontend-platform/shadcn-vue/rules/styling.md.dockerignoreDockerfile.devDockerfile.dev.alpineapps/rook/src/dashboard.rsapps/rook/src/di.rsapps/rook/src/main.rscrates/application/rook-usecases/src/manage_connections.rsdev/Dockerfiledev/Dockerfile.alpinedev/Dockerfile.runtimedev/Dockerfile.ubuntudev/README.mddev/docker-compose.ymldev/e2e-test.shdev/rook.tomldev/test-configs/rook-minimal.tomlopenspec/ARCHITECTURE.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/archive-report.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/state.yamlopenspec/changes/archive/2026-05-31-dynamic-provider-registry/tasks.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/verify-report.mdopenspec/changes/dynamic-provider-registry/design.mdopenspec/changes/dynamic-provider-registry/spec.mdopenspec/changes/dynamic-provider-registry/state.yamlopenspec/changes/dynamic-provider-registry/tasks.mdopenspec/changes/dynamic-provider-registry/verify-report.mdopenspec/specs/dynamic-provider-registry/design.mdopenspec/specs/dynamic-provider-registry/spec.md
…eports Add proposal, design, tasks, verification report, state, and archive report for the provider-connections change. Documents the full SDD lifecycle, including domain model, repository port, encryption, API design, config, and test coverage. Marks all phases as complete and verified.
Dockerfile fixes: - .dockerignore: allow !target/debug/rook for dev/Dockerfile workflow - Dockerfile.dev.alpine: add missing COPY apps/ ./apps/, use release build - dev/Dockerfile.ubuntu: fix missing apps COPY, debug→release build, remove invalid fallback COPY syntax - dev/Dockerfile.runtime: rewrite as proper multi-stage build with builder stage (was referencing non-existent 'builder' stage) Security hardening (Semgrep): - Add USER non-root to all runtime Dockerfiles: Dockerfile.dev, dev/Dockerfile, dev/Dockerfile.ubuntu, dev/Dockerfile.runtime, Dockerfile.dev.alpine Also fix copy-paste comment typo in Dockerfile.dev (debug→release)
…y-authz-architecture-notes changes - Update `state.yaml` to archive `provider-connections` (phase: sdd-archive, archived: true, archive_date: 2026-05-31) - Add archive state for `security-authz-architecture-notes` (archived: true, archived_at: 2025-05-31) - Add `apply-progress.md` and `verification_summary.md` for security-authz-architecture-notes (phase 8 complete, all specs verified) - Update `archive-report.md` for provider-connections with artifact and verification tables - Normalize and align markdown tables in `design.md`, `tasks.md`, `proposal.md`, `ARCHITECTURE.md`, `DOMAIN.md`, and `spec.md` for consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/rook/dashboard/public/icons/README.txt (1)
1-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign README icon paths with actual implementation paths.
The examples here still point to root-level assets (
/favicon.ico,/icon-192.png, etc.), but this PR serves icons under/icons/.... Copy/pasting this README will produce broken references.Suggested doc patch
- <link rel="icon" href="/favicon.ico" sizes="any"> - <link rel="apple-touch-icon" href="/apple-touch-icon.png"> + <link rel="icon" href="/icons/favicon.ico" sizes="any"> + <link rel="apple-touch-icon" href="/icons/ios/AppIcon~ios-marketing.png"> @@ - { "src": "/favicon.ico", "type": "image/x-icon", "sizes": "16x16 32x32" }, - { "src": "/icon-192.png", "type": "image/png", "sizes": "192x192" }, - { "src": "/icon-512.png", "type": "image/png", "sizes": "512x512" }, - { "src": "/icon-192-maskable.png", "type": "image/png", "sizes": "192x192", "purpose": "maskable" }, - { "src": "/icon-512-maskable.png", "type": "image/png", "sizes": "512x512", "purpose": "maskable" } + { "src": "/icons/favicon.ico", "type": "image/x-icon", "sizes": "16x16 32x32" }, + { "src": "/icons/icon-192.png", "type": "image/png", "sizes": "192x192" }, + { "src": "/icons/icon-512.png", "type": "image/png", "sizes": "512x512" }, + { "src": "/icons/icon-192-maskable.png", "type": "image/png", "sizes": "192x192", "purpose": "maskable" }, + { "src": "/icons/icon-512-maskable.png", "type": "image/png", "sizes": "512x512", "purpose": "maskable" }🤖 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/public/icons/README.txt` around lines 1 - 19, Update the README examples so the icon paths match the app's served location (/icons/...), not root-level assets; replace occurrences of "/favicon.ico", "/apple-touch-icon.png", "/icon-192.png", "/icon-512.png", "/icon-192-maskable.png", and "/icon-512-maskable.png" in the HTML <head> snippet and the manifest "icons" array with their corresponding "/icons/..." counterparts used by the implementation so copy/pasted snippets point to valid assets.openspec/specs/provider-connections/spec.md (1)
476-484:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActive spec config contract does not match implementation.
The spec requires
provider_crud.db_path, but verification notes in this PR indicate runtime usesdatabase.db_pathandProviderCrudConfiglacksdb_path. This should be corrected in the active spec (or explicitly marked as deferred) to prevent incorrect deployments.🤖 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/provider-connections/spec.md` around lines 476 - 484, The spec declares provider_crud.db_path but the implementation reads database.db_path and ProviderCrudConfig does not expose db_path; reconcile them by either updating the spec to document database.db_path as the source of truth or changing the code to add db_path to ProviderCrudConfig and make the runtime read provider_crud.db_path (and validate it when provider_crud.enabled is true alongside ENCRYPTION_PASSPHRASE/ENCRYPTION_SALT); update any config validation/verification logic and docs so the config key, the ProviderCrudConfig type, and the runtime lookup all consistently reference the same symbol (provider_crud.db_path or database.db_path).openspec/specs/provider-connections-transport/spec.md (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCross-spec reference is ambiguous/stale.
This points to
provider-connections.md, but the provided canonical spec isopenspec/specs/provider-connections/spec.md. Use the explicit path to avoid broken navigation.🤖 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/provider-connections-transport/spec.md` at line 13, Update the ambiguous cross-spec reference in the sentence that points to `provider-connections.md` so it uses the explicit canonical path `openspec/specs/provider-connections/spec.md`; locate the line containing "provider-connections.md" in `spec.md` and replace it with the full path `openspec/specs/provider-connections/spec.md` to ensure correct navigation and avoid stale/ambiguous links.openspec/changes/archive/2025-05-31-security-authz-architecture-notes/proposal.md (1)
124-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCSRF flow cannot work as written with
HttpOnlytoken cookie.The flow requires client-side echo of cookie value into
X-CSRF-Token, butHttpOnlyblocks client access. Update the proposal to use a readable CSRF cookie (non-HttpOnly) or a different CSRF token transport model.🤖 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/2025-05-31-security-authz-architecture-notes/proposal.md` around lines 124 - 127, The proposal's CSRF double-submit flow is inconsistent because the step "Set-Cookie: csrf_token (HttpOnly, Secure)" prevents the client from reading the cookie to echo it in the "X-CSRF-Token" header; update the proposal to either set the csrf_token cookie as readable (remove HttpOnly) so the client can read and echo it in the POST flow ("Client → POST /api/... (cookie + X-CSRF-Token header)" and "Validate X-CSRF-Token == csrf_token cookie"), or replace the double-submit model with a synchronizer token (issue token in GET /login response body and validate against server-side session) or another transport (e.g., store token in a JavaScript-accessible storage and use SameSite/secure cookie only for session) and explicitly call out the chosen change in the documented flow.Dockerfile.dev (1)
48-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCreate the runtime account before switching users.
USER non-rootpoints to a username that this image never creates. Docker uses theUSERvalue for the runtimeENTRYPOINT/CMD, and when a username is supplied it must exist in the container, so this image will not start as written. (docs.docker.com)Proposed fix
# Copy binary from builder (release build) COPY --from=builder /app/target/release/rook /usr/local/bin/rook # Copy test config COPY dev/test-configs/rook-minimal.toml /app/rook.toml + +RUN groupadd --system rook \ + && useradd --system --gid rook --home-dir /app --no-create-home rook \ + && chown -R rook:rook /app # Expose default port EXPOSE 8080 -USER non-root +USER rook🤖 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 `@Dockerfile.dev` around lines 48 - 57, The Dockerfile sets USER non-root but never creates that account, so container startup will fail; modify the Dockerfile to create the runtime user and group before the USER non-root instruction (e.g. add commands that create a group and user like addgroup/useradd or use groupadd/useradd with a stable UID/GID), ensure ownership of /usr/local/bin/rook and /app/rook.toml is adjusted (chown) so the non-root user can access them, then keep the existing USER non-root line; reference the existing USER non-root and the copied files (/usr/local/bin/rook and /app/rook.toml) when adding the creation and chown steps.
♻️ Duplicate comments (2)
openspec/specs/dynamic-provider-registry/spec.md (1)
79-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve R8 contradiction with implemented feature-gate behavior.
The spec still says the registry is “always active,” but your architecture/archive verification docs explicitly state it is gated by
provider_crud.enabled. Please align R8 and section 3.2 with the documented implemented behavior (or add a formal deviation note in this spec section).Also applies to: 611-614
🤖 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/dynamic-provider-registry/spec.md` around lines 79 - 80, Update the spec to resolve the contradiction: change the R8/section 3.2 language that claims the dynamic registry is "always active" to state that the dynamic registry's CRUD HTTP routes are gated by the feature flag provider_crud.enabled (matching your architecture/archive docs), or alternatively add an explicit "Deviation" note in R8 and 3.2 that documents the implemented behavior and rationale; reference R8, section 3.2, and the provider_crud.enabled flag in the updated text so readers can reconcile the behavior with the implementation.openspec/changes/archive/2026-05-31-dynamic-provider-registry/tasks.md (1)
73-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync Phase 5 checklist with verification artifacts.
Tasks 5.6–5.12 are still unchecked here, but the verification docs in this PR report those tests as implemented and passing. Update these checklist items to
[x]so tasks.md matches the archived verification state.🤖 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-dynamic-provider-registry/tasks.md` around lines 73 - 79, Update the Sync Phase 5 checklist by marking items 5.6 through 5.12 as completed (change their list markers from [ ] to [x]) so the tasks.md checklist matches the archived verification state reported by the PR; specifically edit the lines for "5.6 Add `refresh_registry_skips_inactive_connections`", "5.7 Add `refresh_registry_decrypts_and_builds_provider`", "5.8 Add `refresh_registry_partial_failure_keeps_valid_providers`", "5.9 Add `refresh_registry_all_failures_results_in_empty_registry`", "5.10 Add `create_calls_refresh_after_write`", "5.11 Add `update_calls_refresh_after_write`", and "5.12 Add `delete_calls_refresh_after_write`" to be checked.
🤖 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 @.github/workflows/ci.yml:
- Line 186: Replace the invalid job-level condition that references secrets by
exposing the secret as an env var and checking that env var (set workflow or job
env: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}) and change jobs.sonar.if to use
if: env.SONAR_TOKEN != '' so the job-level condition uses env instead of secrets
(refer to jobs.sonar.if and env.SONAR_TOKEN); also pin the
sonarsource/sonarcloud-action@master reference to a specific commit SHA (replace
sonarsource/sonarcloud-action@master with
sonarsource/sonarcloud-action@<commit-sha>) to avoid using an unpinned action.
In `@dev/Dockerfile`:
- Line 33: The Dockerfile currently switches to a non-existent user via the
instruction "USER non-root"; create the runtime account before switching by
adding steps to create the user and group (e.g., adduser/useradd or groupadd),
create its home directory, set ownership of any app directories (chown), and
then set USER non-root so the container runs as that account; locate the
Dockerfile section around the "USER non-root" token and insert the user creation
and chown commands immediately before that USER instruction and ensure the
created username matches "non-root".
In `@dev/Dockerfile.runtime`:
- Around line 41-46: The runtime Dockerfile sets USER non-root but never creates
that account; add steps before the USER non-root line to create the non-root
account and group (e.g., using adduser/useradd or addgroup + adduser), create
its home directory, chown any runtime-owned files/directories (like /app and
/usr/local/bin/rook) to that user, and set any needed HOME/UID/GID environment
vars so USER non-root references a real user; update the runtime stage around
the COPY lines and before USER non-root to perform these actions.
In `@dev/Dockerfile.ubuntu`:
- Around line 37-46: The Dockerfile ends with "USER non-root" but never creates
that account, causing container startup failures; modify the Dockerfile to
create a non-root runtime user and group before the "USER non-root" line (e.g.,
use groupadd/useradd or addgroup/adduser), chown any runtime-owned paths (like
/usr/local/bin/rook and /app or /app/rook.toml created by the COPY lines), and
then switch to "USER non-root"; ensure the user creation command and chown occur
after the COPY --from=builder /app/target/release/rook ... and COPY
dev/test-configs/rook-minimal.toml ... steps and before the final USER non-root
instruction so the named user actually exists at runtime.
In `@Dockerfile.dev.alpine`:
- Around line 38-46: The Dockerfile switches to USER non-root but never creates
that account, causing the container to fail to start; before the line that sets
"USER non-root" create the runtime account/group (e.g., add a non-root user and
group) and chown the installed binary (/usr/local/bin/rook) and config
(/app/rook.toml) to that user so runtime permissions are correct; ensure the
created username matches the USER directive ("non-root") and use the same
UID/GID/paths referenced in the COPY lines and EXPOSE 8080 so the switch to USER
non-root succeeds.
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/apply-progress.md`:
- Around line 51-53: Update the absolute completion claims to acknowledge the
recorded open deviations: replace the sentence "All 71 specs (SPEC-001 through
SPEC-071) verified as ✅ IMPLEMENTED in `verification_summary.md`." with a
qualified statement such as "All 71 specs (SPEC-001 through SPEC-071) verified
with known deviations — see verification_summary.md for details," and change the
heading "## Phase 8 Status: ✅ COMPLETE" to a qualified status like "## Phase 8
Status: PASS WITH KNOWN DEVIATIONS" so the document aligns with the unresolved
gaps documented in the lines referencing deviations (lines 43–47).
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/design.md`:
- Around line 318-320: The design currently requires a double-submit CSRF token
but marks the cookie `csrf_token` as HttpOnly, which prevents client-side JS
from reading and sending it in the `X-CSRF-Token` header; fix by choosing one of
two options and updating the design and table: either make `csrf_token`
non-HttpOnly (set HttpOnly=false in the table and document that dashboard/client
JS reads the cookie and sends `X-CSRF-Token`) or switch to a
server-issued/header-token pattern (remove double-submit, describe a
server-generated token delivered in a non-cookie header or via an authenticated
endpoint and validated on each request), and update the `csrf_token` handling in
the design text (references to double-submit, `csrf_token`, `HttpOnly`, and
`X-CSRF-Token`) to match the chosen approach.
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/spec.md`:
- Around line 429-435: The spec is inconsistent: SPEC-060 mandates an HttpOnly
cookie named csrf_token while SPEC-061 requires browser clients to read and copy
that token into the X-CSRF-Token header; choose one fix: either update SPEC-060
to make csrf_token non-HttpOnly (allowing JS to read it) and note
SameSite/Secure constraints, or update SPEC-061 to accept the cookie being sent
automatically (double-submit-cookie alternative) or require an alternative
non-HttpOnly token carrier (e.g., a separate public cookie or hidden form field)
and remove the requirement that browsers copy csrf_token into X-CSRF-Token;
apply the same change consistently for the other occurrence referenced as well.
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/tasks.md`:
- Line 436: Fix the typo in the task list item that reads "Test expired session
returns401" by adding a space so it reads "Test expired session returns 401";
locate the list under the tasks section (the numbered item "4.") in the document
and update the text for clarity.
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/verification_summary.md`:
- Line 27: The verification summary claims Argon2id was implemented with "1
parallelism" but SPEC-010 requires parallelism ≥4; update the actual
implementation in encryption-inmemory/src/password.rs by changing the
Argon2idHasher parameters to use parallelism >=4 (keep OWASP memory and
iterations), or if you cannot change the implementation now, update
verification_summary.md to mark SPEC-010 as not fully implemented and correct
the parameter detail; reference the Argon2idHasher symbol when making the change
so the evidence and code stay consistent.
- Around line 75-76: The verification summary incorrectly marks SPEC-070 and
SPEC-071 as fully implemented despite per-key enforcement being deferred; update
the SPEC-070 and SPEC-071 rows to reflect partial compliance (e.g., "PARTIALLY
IMPLEMENTED" or "DEFERRED - SERVER-WIDE ONLY") and amend the top-line summary
"All 71 specs verified" to the correct partial count and wording; also add a
brief note next to the SPEC-070/SPEC-071 entries (or in the notes section)
stating that the implementation in
transport-axum/src/middleware/api_key_rate_limiter.rs currently provides per-key
buckets configuration but per-key enforcement is not active and therefore
rate-limiting is partially implemented.
- Line 61: Update the spacing typos in verification_summary.md by replacing any
run-together tokens: change "Returns429" to "Returns 429" for the SPEC-051 row
(and any other rows like the one at line ~130) and change "Phase8 Status" to
"Phase 8 Status" where present so the table reads "Returns 429" and "Phase 8
Status" consistently (search for SPEC-051 and the string "Phase8" to locate
occurrences).
In
`@openspec/changes/archive/2026-05-31-dynamic-provider-registry/verify-report.md`:
- Around line 153-154: Update the verdict summary bullets in the verify report
to accurately reflect the R8 deviation: change any statements that claim full
compliance for R1–R8 to "implemented, with documented R8 deviation" (or similar
wording) and call out the specific deviation about gating the dynamic registry
behind provider_crud.enabled which causes manage_connections to be None when
disabled; ensure the same rewording is applied to the duplicate section around
lines 170–174 so the report consistently states "implemented with documented R8
deviation" rather than full compliance.
In `@openspec/changes/archive/2026-05-31-provider-connections/design.md`:
- Around line 352-364: The design doc currently lists ProviderCrudConfig.db_path
but the implementation uses DatabaseConfig.db_path and ProviderCrudConfig only
contains enabled; update the archived text to match the verified implementation
by removing db_path from ProviderCrudConfig and moving the db_path setting into
the DatabaseConfig section (and update the default TOML accordingly), or
alternatively add a short “planned vs implemented” note that explicitly states
ProviderCrudConfig only has enabled while DatabaseConfig.db_path is used in
production; reference ProviderCrudConfig.enabled, ProviderCrudConfig.db_path,
and DatabaseConfig.db_path when making the change.
- Line 7: The line in
openspec/changes/archive/2026-05-31-provider-connections/design.md currently
points to the stale string "openspec/changes/provider-connections/spec.md";
locate the actual canonical spec file for provider-connections in the repository
(the true spec path) and replace that stale reference with the correct
path/filename and a working markdown link, ensuring the referenced file exists
and the link is not broken.
In `@openspec/changes/archive/2026-05-31-provider-connections/verify-report.md`:
- Around line 146-147: The archived verify-report references the wrong artifact
path; update the artifact link in
openspec/changes/archive/2026-05-31-provider-connections/verify-report.md so it
points to the archived location instead of the non-archive one (replace
occurrences of "openspec/changes/provider-connections/verify-report.md" with
"openspec/changes/archive/2026-05-31-provider-connections/verify-report.md" to
restore correct traceability).
---
Outside diff comments:
In `@apps/rook/dashboard/public/icons/README.txt`:
- Around line 1-19: Update the README examples so the icon paths match the app's
served location (/icons/...), not root-level assets; replace occurrences of
"/favicon.ico", "/apple-touch-icon.png", "/icon-192.png", "/icon-512.png",
"/icon-192-maskable.png", and "/icon-512-maskable.png" in the HTML <head>
snippet and the manifest "icons" array with their corresponding "/icons/..."
counterparts used by the implementation so copy/pasted snippets point to valid
assets.
In `@Dockerfile.dev`:
- Around line 48-57: The Dockerfile sets USER non-root but never creates that
account, so container startup will fail; modify the Dockerfile to create the
runtime user and group before the USER non-root instruction (e.g. add commands
that create a group and user like addgroup/useradd or use groupadd/useradd with
a stable UID/GID), ensure ownership of /usr/local/bin/rook and /app/rook.toml is
adjusted (chown) so the non-root user can access them, then keep the existing
USER non-root line; reference the existing USER non-root and the copied files
(/usr/local/bin/rook and /app/rook.toml) when adding the creation and chown
steps.
In
`@openspec/changes/archive/2025-05-31-security-authz-architecture-notes/proposal.md`:
- Around line 124-127: The proposal's CSRF double-submit flow is inconsistent
because the step "Set-Cookie: csrf_token (HttpOnly, Secure)" prevents the client
from reading the cookie to echo it in the "X-CSRF-Token" header; update the
proposal to either set the csrf_token cookie as readable (remove HttpOnly) so
the client can read and echo it in the POST flow ("Client → POST /api/...
(cookie + X-CSRF-Token header)" and "Validate X-CSRF-Token == csrf_token
cookie"), or replace the double-submit model with a synchronizer token (issue
token in GET /login response body and validate against server-side session) or
another transport (e.g., store token in a JavaScript-accessible storage and use
SameSite/secure cookie only for session) and explicitly call out the chosen
change in the documented flow.
In `@openspec/specs/provider-connections-transport/spec.md`:
- Line 13: Update the ambiguous cross-spec reference in the sentence that points
to `provider-connections.md` so it uses the explicit canonical path
`openspec/specs/provider-connections/spec.md`; locate the line containing
"provider-connections.md" in `spec.md` and replace it with the full path
`openspec/specs/provider-connections/spec.md` to ensure correct navigation and
avoid stale/ambiguous links.
In `@openspec/specs/provider-connections/spec.md`:
- Around line 476-484: The spec declares provider_crud.db_path but the
implementation reads database.db_path and ProviderCrudConfig does not expose
db_path; reconcile them by either updating the spec to document database.db_path
as the source of truth or changing the code to add db_path to ProviderCrudConfig
and make the runtime read provider_crud.db_path (and validate it when
provider_crud.enabled is true alongside ENCRYPTION_PASSPHRASE/ENCRYPTION_SALT);
update any config validation/verification logic and docs so the config key, the
ProviderCrudConfig type, and the runtime lookup all consistently reference the
same symbol (provider_crud.db_path or database.db_path).
---
Duplicate comments:
In `@openspec/changes/archive/2026-05-31-dynamic-provider-registry/tasks.md`:
- Around line 73-79: Update the Sync Phase 5 checklist by marking items 5.6
through 5.12 as completed (change their list markers from [ ] to [x]) so the
tasks.md checklist matches the archived verification state reported by the PR;
specifically edit the lines for "5.6 Add
`refresh_registry_skips_inactive_connections`", "5.7 Add
`refresh_registry_decrypts_and_builds_provider`", "5.8 Add
`refresh_registry_partial_failure_keeps_valid_providers`", "5.9 Add
`refresh_registry_all_failures_results_in_empty_registry`", "5.10 Add
`create_calls_refresh_after_write`", "5.11 Add
`update_calls_refresh_after_write`", and "5.12 Add
`delete_calls_refresh_after_write`" to be checked.
In `@openspec/specs/dynamic-provider-registry/spec.md`:
- Around line 79-80: Update the spec to resolve the contradiction: change the
R8/section 3.2 language that claims the dynamic registry is "always active" to
state that the dynamic registry's CRUD HTTP routes are gated by the feature flag
provider_crud.enabled (matching your architecture/archive docs), or
alternatively add an explicit "Deviation" note in R8 and 3.2 that documents the
implemented behavior and rationale; reference R8, section 3.2, and the
provider_crud.enabled flag in the updated text so readers can reconcile the
behavior with the implementation.
🪄 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: 2a0c1cb3-eaeb-41b7-a034-a846a4463086
⛔ Files ignored due to path filters (49)
apps/rook/dashboard/public/icons/android/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-hdpi/ic_launcher_background.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-hdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-mdpi/ic_launcher_background.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-mdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xhdpi/ic_launcher_background.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxhdpi/ic_launcher_background.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxxhdpi/ic_launcher_background.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/android/mipmap-xxxhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/apple-touch-icon.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/favicon.icois excluded by!**/*.icoapps/rook/dashboard/public/icons/icon-192-maskable.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/icon-192.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/icon-512-maskable.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/icon-512.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-20@2x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-20@2x~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-20@3x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-20~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-29.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-29@2x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-29@2x~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-29@3x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-29~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-40@2x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-40@2x~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-40@3x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-40~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-60@2x~car.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-60@3x~car.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon-83.5@2x~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon@2x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon@2x~ipad.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon@3x.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon~ios-marketing.pngis excluded by!**/*.pngapps/rook/dashboard/public/icons/ios/AppIcon~ipad.pngis excluded by!**/*.pngdocs/assets/rook.pngis excluded by!**/*.pngdocs/assets/rook.svgis excluded by!**/*.svg
📒 Files selected for processing (43)
.dockerignore.github/workflows/ci.ymlDockerfile.devDockerfile.dev.alpineapps/rook/dashboard/index.htmlapps/rook/dashboard/public/icons/README.txtapps/rook/dashboard/public/icons/android/mipmap-anydpi-v26/ic_launcher.xmlapps/rook/dashboard/public/icons/ios/Contents.jsonapps/rook/dashboard/public/icons/manifest.jsondev/Dockerfiledev/Dockerfile.runtimedev/Dockerfile.ubuntuopenspec/ARCHITECTURE.mdopenspec/DOMAIN.mdopenspec/archive/security-authz-architecture-notes/apply-progress.mdopenspec/archive/security-authz-architecture-notes/tasks.mdopenspec/archive/security-authz-architecture-notes/verification_summary.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/apply-progress.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/design.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/proposal.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/spec.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/state.yamlopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/tasks.mdopenspec/changes/archive/2025-05-31-security-authz-architecture-notes/verification_summary.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/archive-report.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/design.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/spec.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/tasks.mdopenspec/changes/archive/2026-05-31-dynamic-provider-registry/verify-report.mdopenspec/changes/archive/2026-05-31-provider-connections/archive-report.mdopenspec/changes/archive/2026-05-31-provider-connections/design.mdopenspec/changes/archive/2026-05-31-provider-connections/proposal.mdopenspec/changes/archive/2026-05-31-provider-connections/spec.mdopenspec/changes/archive/2026-05-31-provider-connections/state.yamlopenspec/changes/archive/2026-05-31-provider-connections/tasks.mdopenspec/changes/archive/2026-05-31-provider-connections/verify-report.mdopenspec/changes/dynamic-provider-registry/verify-report.mdopenspec/changes/provider-connections/state.yamlopenspec/specs/dynamic-provider-registry/design.mdopenspec/specs/dynamic-provider-registry/spec.mdopenspec/specs/provider-connections-transport/spec.mdopenspec/specs/provider-connections/design.mdopenspec/specs/provider-connections/spec.md
💤 Files with no reviewable changes (7)
- openspec/archive/security-authz-architecture-notes/tasks.md
- openspec/changes/provider-connections/state.yaml
- openspec/archive/security-authz-architecture-notes/verification_summary.md
- openspec/archive/security-authz-architecture-notes/apply-progress.md
- openspec/changes/dynamic-provider-registry/verify-report.md
- openspec/changes/archive/2026-05-31-dynamic-provider-registry/spec.md
- openspec/changes/archive/2026-05-31-dynamic-provider-registry/design.md
| name: SonarCloud | ||
| runs-on: ubuntu-latest | ||
| needs: [test] | ||
| if: secrets.SONAR_TOKEN != '' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant section around the referenced line(s)
sed -n '150,220p' .github/workflows/ci.yml | nl -ba | sed -n '1,120p'Repository: dallay/cortex
Length of output: 100
🌐 Web query:
GitHub Actions jobs.<job_id>.if secrets context not allowed
💡 Result:
In GitHub Actions, you cannot directly use the secrets context in a jobs.<job_id>.if conditional expression [1][2][3]. This is a documented limitation implemented for security reasons [3]. If you attempt to reference secrets in a job-level if statement, the workflow will fail with an "Unrecognized named-value" error [1][3]. To conditionally run a job based on the presence or value of a secret, use the following recommended workarounds: 1. Use job-level environment variables: Instead of checking the secret in the job's if condition, define the secret as an environment variable within the job or step, and then perform the conditional check at the step level [2][3]. yaml jobs: my_job: runs-on: ubuntu-latest steps: - name: Run only if secret is set if: env.MY_SECRET != '' env: MY_SECRET: ${{ secrets.MY_SECRET }} run: echo "Secret is available" 2. Perform pre-check jobs: If you need to skip an entire job based on a secret's status, use a preceding job to check the secret and output a result. You can then use the needs context in the subsequent job's if conditional to trigger or skip it based on that output [3]. yaml jobs: check_secret: runs-on: ubuntu-latest outputs: has_secret: ${{ steps.check.outputs.defined }} steps: - id: check run: echo "defined=${{ secrets.MY_SECRET != '' }}" >> $GITHUB_OUTPUT dependent_job: needs: [check_secret] if: needs.check_secret.outputs.has_secret == 'true' runs-on: ubuntu-latest steps: - run: echo "Secret exists, running job." For further context on available contexts, refer to the official GitHub Actions context availability documentation, which explicitly excludes secrets from being valid in jobs.<job_id>.if expressions [4][5].
Citations:
- 1: Secrets cannot be used to condition job runs actions/runner#520
- 2: https://docs.github.com/actions/security-guides/using-secrets-in-github-actions
- 3: fix: GitHub Actions workflow fails due to invalid secret reference in job-level conditional atxtechbro/dotfiles#1347
- 4: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/contexts.md
- 5: https://stackoverflow.com/questions/76471787/why-is-env-context-not-available-in-github-action-job-level-if-statement
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=170 && NR<=210 {print NR ":" $0}' .github/workflows/ci.ymlRepository: dallay/cortex
Length of output: 1679
Fix invalid jobs.sonar.if expression referencing secrets
.github/workflows/ci.ymlcurrently usesif: secrets.SONAR_TOKEN != ''at job level; GitHub Actions doesn’t allowsecretsinjobs.<job_id>.if, which breaks workflow validation.- Pin
sonarsource/sonarcloud-action@masterto a specific commit/SHA (currently unpinned).
💡 Minimal fix
sonar:
name: SonarCloud
runs-on: ubuntu-latest
needs: [test]
- if: secrets.SONAR_TOKEN != ''
+ env:
+ SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
steps:
- uses: actions/checkout@v6.0.2
with:
persist-credentials: false
- - uses: sonarsource/sonarcloud-action@master
+ - if: env.SONAR_TOKEN != ''
+ uses: sonarsource/sonarcloud-action@master
env:
- SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
+ SONAR_TOKEN: ${{ env.SONAR_TOKEN }}📝 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.
Suggested change
| if: secrets.SONAR_TOKEN != '' | |
| sonar: | |
| name: SonarCloud | |
| runs-on: ubuntu-latest | |
| needs: [test] | |
| env: | |
| SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} | |
| steps: | |
| - uses: actions/checkout@v6.0.2 | |
| with: | |
| persist-credentials: false | |
| - if: env.SONAR_TOKEN != '' | |
| uses: sonarsource/sonarcloud-action@master | |
| env: | |
| SONAR_TOKEN: ${{ env.SONAR_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 186-186: context "secrets" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 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 @.github/workflows/ci.yml at line 186, Replace the invalid job-level
condition that references secrets by exposing the secret as an env var and
checking that env var (set workflow or job env: SONAR_TOKEN: ${{
secrets.SONAR_TOKEN }}) and change jobs.sonar.if to use if: env.SONAR_TOKEN !=
'' so the job-level condition uses env instead of secrets (refer to
jobs.sonar.if and env.SONAR_TOKEN); also pin the
sonarsource/sonarcloud-action@master reference to a specific commit SHA (replace
sonarsource/sonarcloud-action@master with
sonarsource/sonarcloud-action@<commit-sha>) to avoid using an unpinned action.
Comment on lines
+75
to
+76
| | SPEC-070 | Per-Key Token Bucket Rate Limiting for CLIENT_API | ✅ IMPLEMENTED | `transport-axum/src/middleware/api_key_rate_limiter.rs` — per-key buckets with configurable capacity/refill | | ||
| | SPEC-071 | Rate Limited CLIENT_API Returns 429 | ✅ IMPLEMENTED | Returns 429 with `Retry-After` header and `RATE_LIMITED` code | |
Contributor
There was a problem hiding this comment.
Final status conflicts with the stated API key rate-limit deviation.
The summary marks SPEC-070/071 as implemented and “All 71 specs verified,” while Line 116-117 says per-key enforcement is deferred/not active. This should be reported as partial compliance, not full implementation.
Also applies to: 116-117, 126-126
🤖 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/2025-05-31-security-authz-architecture-notes/verification_summary.md`
around lines 75 - 76, The verification summary incorrectly marks SPEC-070 and
SPEC-071 as fully implemented despite per-key enforcement being deferred; update
the SPEC-070 and SPEC-071 rows to reflect partial compliance (e.g., "PARTIALLY
IMPLEMENTED" or "DEFERRED - SERVER-WIDE ONLY") and amend the top-line summary
"All 71 specs verified" to the correct partial count and wording; also add a
brief note next to the SPEC-070/SPEC-071 entries (or in the notes section)
stating that the implementation in
transport-axum/src/middleware/api_key_rate_limiter.rs currently provides per-key
buckets configuration but per-key enforcement is not active and therefore
rate-limiting is partially implemented.
Comment on lines
+153
to
+154
| 1. **R8 Deviation**: The dynamic registry is gated behind `provider_crud.enabled` (di.rs:54). When `provider_crud.enabled = false`, `manage_connections` is `None` and the registry starts empty (never populated). The spec R8 says "the dynamic registry is always active." The implementation is pragmatically correct — when the feature is disabled, there are no connections to load anyway. However, the spec language is not met. | ||
|
|
Contributor
There was a problem hiding this comment.
Make the verdict summary consistent with the R8 deviation.
This report flags R8 as a deviation, but later states all R1–R8 are implemented. Reword the verdict bullets so they reflect “implemented with documented R8 deviation” instead of full compliance.
Also applies to: 170-174
🤖 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-dynamic-provider-registry/verify-report.md`
around lines 153 - 154, Update the verdict summary bullets in the verify report
to accurately reflect the R8 deviation: change any statements that claim full
compliance for R1–R8 to "implemented, with documented R8 deviation" (or similar
wording) and call out the specific deviation about gating the dynamic registry
behind provider_crud.enabled which causes manage_connections to be None when
disabled; ensure the same rewording is applied to the duplicate section around
lines 170–174 so the report consistently states "implemented with documented R8
deviation" rather than full compliance.
|
|
||
| This design implements dynamic `ProviderConnection` management as a persisted administrative API. v1 stores connection metadata and encrypted credentials, exposes CRUD/test endpoints, and keeps existing TOML-configured providers as the only request-routing source. Runtime hot registration from SQLite into routing is explicitly future work. | ||
|
|
||
| The canonical behavioral contract is `openspec/changes/provider-connections/spec.md`. |
Contributor
There was a problem hiding this comment.
Fix stale canonical spec path reference.
openspec/changes/provider-connections/spec.md appears stale/non-existent in this PR layout. Point this to the actual canonical spec location to avoid dead links.
Suggested doc fix
-The canonical behavioral contract is `openspec/changes/provider-connections/spec.md`.
+The canonical behavioral contract is `openspec/specs/provider-connections/spec.md`.📝 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.
Suggested change
| The canonical behavioral contract is `openspec/changes/provider-connections/spec.md`. | |
| The canonical behavioral contract is `openspec/specs/provider-connections/spec.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-provider-connections/design.md` at line
7, The line in
openspec/changes/archive/2026-05-31-provider-connections/design.md currently
points to the stale string "openspec/changes/provider-connections/spec.md";
locate the actual canonical spec file for provider-connections in the repository
(the true spec path) and replace that stale reference with the correct
path/filename and a working markdown link, ensuring the referenced file exists
and the link is not broken.
Comment on lines
+352
to
+364
| pub struct ProviderCrudConfig { | ||
| pub enabled: bool, | ||
| pub db_path: String, | ||
| } | ||
| ``` | ||
|
|
||
| Default TOML: | ||
|
|
||
| ```toml | ||
| [provider_crud] | ||
| enabled = false | ||
| db_path = "~/.local/share/cortex/rook/providers.db" | ||
| ``` |
Contributor
There was a problem hiding this comment.
Config contract conflicts with verified implementation.
This section specifies ProviderCrudConfig.db_path, but your own verification artifacts state implementation uses DatabaseConfig.db_path and ProviderCrudConfig only has enabled. Please align this archived design text or add an explicit “planned vs implemented” note to prevent operator misconfiguration.
Also applies to: 366-371
🤖 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-provider-connections/design.md` around
lines 352 - 364, The design doc currently lists ProviderCrudConfig.db_path but
the implementation uses DatabaseConfig.db_path and ProviderCrudConfig only
contains enabled; update the archived text to match the verified implementation
by removing db_path from ProviderCrudConfig and moving the db_path setting into
the DatabaseConfig section (and update the default TOML accordingly), or
alternatively add a short “planned vs implemented” note that explicitly states
ProviderCrudConfig only has enabled while DatabaseConfig.db_path is used in
production; reference ProviderCrudConfig.enabled, ProviderCrudConfig.db_path,
and DatabaseConfig.db_path when making the change.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yuniel Acosta Pérez <33158051+yacosta738@users.noreply.github.com>
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
4 commits resolving critical bugs and adding Docker e2e infrastructure:
Bug Fixes
fix(rook): resolve block_on deadlock(apps/rook/src/di.rs,apps/rook/src/main.rs)Handle::current().block_on()inside#[tokio::main]multi-thread runtime caused deadlockRookContainer::build()async and usetokio::join!()for concurrent init tasksManageConnections::createfix(rook): dashboard wildcard route(apps/rook/src/dashboard.rs)/*pathis Axum 0.7 syntax; Axum 0.8.x panics at runtime with "Path segments must not start with `*`"{*path}(Axum 0.8 wildcard syntax)Infrastructure
feat(dev): Docker e2e infrastructureDockerfile.dev: Multi-stage ARM64 build (linux/arm64/v8) with Node.js 22 + pnpm.dockerignore: Excludes node_modules, target/, dist/, .git/dev/: Dockerfiles for Ubuntu/Alpine, docker-compose, e2e-test scriptDockerfile.dev.alpine: Alpine variant--network hostwhen running on OrbStack (macOS)docs(openspec): archive dynamic-provider-registryopenspec/specs/dynamic-provider-registry/feat(skills): shadcn-vue skillVerification
cargo test --workspace --exclude dashboard: 189 tests passingcargo clippy: cleancargo fmt --check: clean/healthand/(Vue dashboard)rook:e2eruns successfully on OrbStackRunning E2E Tests