Conversation
📝 WalkthroughWalkthroughThis PR introduces a new CredStore gear providing tenant-scoped secret storage: SDK secret type catalog, domain service with saga-based put/delete/reap logic, SeaORM storage, tenant resolver caching, and REST APIs. It also refactors the static-credstore-plugin store, adds mini-chat OAGW deferred-upstream provisioning resilience, e2e fixtures, config rollout, and OpenAPI docs. ChangesCredStore gear core
Static credstore plugin refactor
Mini-chat OAGW resilience
Config rollout and API docs
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant PolicyEnforcer
participant Plugin
participant SecretRepo
Client->>Service: put(ctx, key, value, spec)
Service->>PolicyEnforcer: evaluate scope
Service->>SecretRepo: insert_provisioning
Service->>Plugin: put(value)
alt plugin failure
Service->>SecretRepo: rollback provisioning row
else plugin success
Service->>SecretRepo: mark_active
end
sequenceDiagram
participant MiniChatGear
participant oagw_provisioning
participant OAGW
MiniChatGear->>oagw_provisioning: register_oagw_upstreams()
oagw_provisioning->>OAGW: create_upstream()
OAGW-->>oagw_provisioning: error (deferred)
oagw_provisioning-->>MiniChatGear: deferred provider IDs
loop retry
MiniChatGear->>oagw_provisioning: reconcile_deferred_upstreams()
oagw_provisioning->>OAGW: create_or_reuse_upstream()
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
modules/credstore/credstore/src/infra/canonical_mapping.rs (1)
33-37: ⚡ Quick winPreserve
DbErras the internal cause for unclassified DB failures.This branch currently drops the original error (
cause: None), which removes key debugging context during incidents.Suggested patch
- warn!(target: "credstore.db", error = %db_err, "unclassified DB error"); - DomainError::Internal { - diagnostic: "database error".to_owned(), - cause: None, - } + warn!(target: "credstore.db", error = %db_err, "unclassified DB error"); + DomainError::Internal { + diagnostic: "database error".to_owned(), + cause: Some(Box::new(db_err)), + }🤖 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 `@modules/credstore/credstore/src/infra/canonical_mapping.rs` around lines 33 - 37, The current branch logs the DB error with warn! but discards the original DbErr by returning DomainError::Internal { cause: None }; update the return to preserve the original error as the internal cause (e.g., DomainError::Internal { diagnostic: "database error".to_owned(), cause: Some(db_err.into()) }) so the DbErr is carried into the DomainError; locate the warn! call and the subsequent DomainError::Internal construction in canonical_mapping.rs and replace the None cause with a wrapped/version-converted Some(db_err) (or Some(Box::new(db_err)) if the DomainError expects a boxed error) to retain debugging context.modules/credstore/credstore/src/infra/storage/repo_impl/writes.rs (1)
129-152: 💤 Low valueConsider distinguishing version conflict from not-found on delete.
When
expected_versionis provided and the row exists but with a different version,delete_by_idreturnsNotFound(Line 149). This conflates "row doesn't exist" with "version mismatch". The caller may want to handle these cases differently (e.g., retry on version conflict vs. fail on not-found).If this is intentional for simplicity (the REST layer maps both to 404/409 anyway), a brief comment documenting this design choice would help future maintainers.
🤖 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 `@modules/credstore/credstore/src/infra/storage/repo_impl/writes.rs` around lines 129 - 152, The delete_by_id function currently returns DomainError::NotFound for any zero rows_affected, which hides version conflicts; update delete_by_id to differentiate: if expected_version is Some and the delete returned 0, run a follow-up query (e.g., entity::secrets::Entity::find_by_id/id filter) to check whether a row with the given id exists and if it does return a version-conflict error (e.g., DomainError::Conflict or a new DomainError::VersionMismatch); otherwise keep returning DomainError::NotFound. Refer to delete_by_id, expected_version, the initial filter and the DomainError enum when making this change. Ensure the extra lookup uses the same scope_with(scope) and map_scope_err handling.
🤖 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 `@config/static-tenants.yaml`:
- Around line 22-30: The config sets modules.credstore.config.vendor to
"cyberfabric" but no credstore backend module is registered, causing plugin
resolution to fail; add a modules.static-credstore-plugin entry (or another
credstore plugin registration) that declares vendor: "cyberfabric" so
types-registry can resolve the backend, or compose a config that registers a
credstore plugin instance for that vendor to avoid the ServiceUnavailable "no
credstore plugin found" error.
In `@modules/credstore/credstore/src/config.rs`:
- Around line 63-65: The vendor field validation currently uses
self.vendor.is_empty() which allows whitespace-only values like " "; update the
validation in the config validation function (where self.vendor is checked) to
reject whitespace-only strings by using a trimmed check (e.g., if
self.vendor.trim().is_empty()) and return the same Err("vendor must be
non-empty".to_owned()) to fail fast during module init.
In `@modules/credstore/credstore/src/domain/secret/service.rs`:
- Around line 488-498: The match arm handling Ok(None) in the put flow (inside
service.rs where tenant, key and the metadata bump are handled) should not
return Ok(()) because that reports success while the metadata row vanished;
instead return a retryable/transient error so callers can retry
deterministically. Replace the Ok(None) branch (currently logging with
tracing::debug) to log the vanish and return an appropriate retryable error
variant (e.g., ServiceError::Retryable or crate::error::TransientError — or add
one if none exists), including tenant and key in the error context so callers
can distinguish this case from permanent failures.
In `@modules/credstore/plugins/static-credstore-plugin/README.md`:
- Line 114: The fenced architecture block in README.md is missing a language tag
(triggering markdownlint MD040); update the triple-backtick fence that wraps the
module list to include a language hint (e.g., change the opening ``` to ```text)
so the block becomes a fenced code block with a language tag; ensure the closing
fence remains ``` and keep the block contents unchanged.
In `@modules/mini-chat/mini-chat/src/infra/oagw_provisioning.rs`:
- Around line 33-46: create_upstream currently returns None for every failure
which hides distinct causes (like AlreadyExists) and makes the caller skip
healthy providers; change create_upstream to return a Result<Upstream,
CreateUpstreamError> (or Result<Option<Upstream>, E> with E carrying error
variants) and add an error variant for "AlreadyExists" (or map the existing
error type). Then update the caller in mini-chat/src/infra/oagw_provisioning.rs
to match the Result from create_upstream: treat Ok(upstream) as before, treat
Err(CreateUpstreamError::AlreadyExists) as a non-fatal success/path that
proceeds (eg. fetch or reuse existing upstream), and only log+continue for other
Err variants while including provider_id in the log; reference functions/idents:
create_upstream, upstream, provider_id, and the provisioning loop to locate
changes.
In `@testing/e2e/modules/mini_chat/conftest.py`:
- Around line 503-507: The current block that handles non-2xx responses (the if
resp.status_code not in (200, 201, 204): ... print(...)) should be changed to
fail fast when running in offline/deterministic test mode: instead of only
printing a warning, raise an exception (for example RuntimeError or pytest.Exit)
that includes ref, resp.status_code and resp.text[:200]. Update the conditional
logic around that block to check the offline flag used by the tests (the same
offline/deterministic flag the test suite uses) and raise on error in offline
mode while keeping the current warning behavior for non-offline runs.
In `@testing/e2e/modules/oagw/conftest.py`:
- Around line 166-176: The exception handler around the httpx.put call (which
provisions the credstore secret using httpx.put with
base_url/credstore/v1/secrets/{ref}) is too narrow — it only catches
httpx.ConnectError; broaden it to catch httpx.RequestError (the base for
connection/timeouts/transport errors) so provisioning failures remain non-fatal
and the function returns early on any transport-level request error. Update the
except clause that currently names httpx.ConnectError to except
httpx.RequestError and keep the existing early return behavior.
---
Nitpick comments:
In `@modules/credstore/credstore/src/infra/canonical_mapping.rs`:
- Around line 33-37: The current branch logs the DB error with warn! but
discards the original DbErr by returning DomainError::Internal { cause: None };
update the return to preserve the original error as the internal cause (e.g.,
DomainError::Internal { diagnostic: "database error".to_owned(), cause:
Some(db_err.into()) }) so the DbErr is carried into the DomainError; locate the
warn! call and the subsequent DomainError::Internal construction in
canonical_mapping.rs and replace the None cause with a wrapped/version-converted
Some(db_err) (or Some(Box::new(db_err)) if the DomainError expects a boxed
error) to retain debugging context.
In `@modules/credstore/credstore/src/infra/storage/repo_impl/writes.rs`:
- Around line 129-152: The delete_by_id function currently returns
DomainError::NotFound for any zero rows_affected, which hides version conflicts;
update delete_by_id to differentiate: if expected_version is Some and the delete
returned 0, run a follow-up query (e.g., entity::secrets::Entity::find_by_id/id
filter) to check whether a row with the given id exists and if it does return a
version-conflict error (e.g., DomainError::Conflict or a new
DomainError::VersionMismatch); otherwise keep returning DomainError::NotFound.
Refer to delete_by_id, expected_version, the initial filter and the DomainError
enum when making this change. Ensure the extra lookup uses the same
scope_with(scope) and map_scope_err handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffed1de0-5959-4b77-aa4a-8c715dc049f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
config/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamlmodules/credstore/credstore-sdk/README.mdmodules/credstore/credstore-sdk/src/api.rsmodules/credstore/credstore-sdk/src/error.rsmodules/credstore/credstore-sdk/src/error_tests.rsmodules/credstore/credstore-sdk/src/gts.rsmodules/credstore/credstore-sdk/src/lib.rsmodules/credstore/credstore-sdk/src/models.rsmodules/credstore/credstore-sdk/src/models_tests.rsmodules/credstore/credstore-sdk/src/plugin_api.rsmodules/credstore/credstore/Cargo.tomlmodules/credstore/credstore/README.mdmodules/credstore/credstore/src/api.rsmodules/credstore/credstore/src/api/rest.rsmodules/credstore/credstore/src/api/rest/dto.rsmodules/credstore/credstore/src/api/rest/dto_tests.rsmodules/credstore/credstore/src/api/rest/handlers.rsmodules/credstore/credstore/src/api/rest/routes.rsmodules/credstore/credstore/src/api/rest/routes_tests.rsmodules/credstore/credstore/src/client.rsmodules/credstore/credstore/src/client_tests.rsmodules/credstore/credstore/src/config.rsmodules/credstore/credstore/src/config_tests.rsmodules/credstore/credstore/src/domain.rsmodules/credstore/credstore/src/domain/authz.rsmodules/credstore/credstore/src/domain/error.rsmodules/credstore/credstore/src/domain/error_tests.rsmodules/credstore/credstore/src/domain/local_client.rsmodules/credstore/credstore/src/domain/local_client_tests.rsmodules/credstore/credstore/src/domain/mod.rsmodules/credstore/credstore/src/domain/ports.rsmodules/credstore/credstore/src/domain/ports/metrics.rsmodules/credstore/credstore/src/domain/ports/plugin.rsmodules/credstore/credstore/src/domain/resolver.rsmodules/credstore/credstore/src/domain/secret.rsmodules/credstore/credstore/src/domain/secret/model.rsmodules/credstore/credstore/src/domain/secret/repo.rsmodules/credstore/credstore/src/domain/secret/service.rsmodules/credstore/credstore/src/domain/secret/service_tests.rsmodules/credstore/credstore/src/domain/secret/test_support.rsmodules/credstore/credstore/src/domain/service.rsmodules/credstore/credstore/src/domain/service_tests.rsmodules/credstore/credstore/src/domain/test_support.rsmodules/credstore/credstore/src/infra.rsmodules/credstore/credstore/src/infra/canonical_mapping.rsmodules/credstore/credstore/src/infra/error_conv.rsmodules/credstore/credstore/src/infra/metrics.rsmodules/credstore/credstore/src/infra/metrics_tests.rsmodules/credstore/credstore/src/infra/plugin_select.rsmodules/credstore/credstore/src/infra/sdk_error_mapping.rsmodules/credstore/credstore/src/infra/storage.rsmodules/credstore/credstore/src/infra/storage/entity.rsmodules/credstore/credstore/src/infra/storage/entity/secrets.rsmodules/credstore/credstore/src/infra/storage/entity/tenant_closure.rsmodules/credstore/credstore/src/infra/storage/migrations.rsmodules/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rsmodules/credstore/credstore/src/infra/storage/repo_impl.rsmodules/credstore/credstore/src/infra/storage/repo_impl/helpers.rsmodules/credstore/credstore/src/infra/storage/repo_impl/reads.rsmodules/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsmodules/credstore/credstore/src/infra/storage/repo_impl/writes.rsmodules/credstore/credstore/src/infra/tenant_resolver.rsmodules/credstore/credstore/src/infra/tenant_resolver_tests.rsmodules/credstore/credstore/src/lib.rsmodules/credstore/credstore/src/module.rsmodules/credstore/docs/DESIGN-ADDENDUM.mdmodules/credstore/plugins/static-credstore-plugin/README.mdmodules/credstore/plugins/static-credstore-plugin/src/domain/client.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/service.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsmodules/mini-chat/mini-chat/src/infra/oagw_provisioning.rsmodules/system/oagw/oagw/src/domain/test_support.rsmodules/system/oagw/oagw/src/infra/plugin/apikey_auth.rsmodules/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsmodules/system/oagw/oagw/src/infra/proxy/service.rstesting/e2e/modules/mini_chat/config/base.yamltesting/e2e/modules/mini_chat/conftest.pytesting/e2e/modules/oagw/conftest.py
💤 Files with no reviewable changes (10)
- modules/credstore/credstore/src/domain/local_client.rs
- modules/credstore/credstore/src/domain/error_tests.rs
- modules/credstore/credstore/src/domain/mod.rs
- modules/credstore/credstore-sdk/src/error_tests.rs
- modules/credstore/credstore-sdk/src/models_tests.rs
- modules/credstore/credstore/src/domain/service_tests.rs
- modules/credstore/credstore/src/config_tests.rs
- modules/credstore/credstore/src/domain/test_support.rs
- modules/credstore/credstore/src/domain/local_client_tests.rs
- modules/credstore/credstore/src/domain/service.rs
diffora
force-pushed
the
credstore
branch
2 times, most recently
from
June 7, 2026 11:49
b658370 to
c7194cd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
modules/credstore/credstore/src/domain/error.rs (1)
36-41: 💤 Low valueConsider surfacing
diagnosticin the error message.The
Internalvariant has adiagnosticfield, but the#[error("internal error")]attribute doesn't interpolate it. This meanserror.to_string()yields only"internal error"without the diagnostic context, which may hinder debugging when errors are logged.If this is intentional to avoid leaking internal details to end users, consider using a separate method for internal logging or documenting this behavior. Otherwise, include the diagnostic:
- #[error("internal error")] + #[error("internal error: {diagnostic}")]🤖 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 `@modules/credstore/credstore/src/domain/error.rs` around lines 36 - 41, The Internal enum variant's display string omits the diagnostic field so error.to_string() only returns "internal error"; update the error attribute on the Internal variant in error.rs (the enum variant named Internal) to interpolate the diagnostic (e.g., include {diagnostic} in the format) so diagnostic details appear in the formatted error, or alternatively add a separate method (e.g., internal_diagnostic() or DisplayForLogging on the error type) that returns the diagnostic for internal logging if you want to keep the public Display unchanged.modules/credstore/credstore/src/api/rest/routes.rs (3)
61-83: ⚡ Quick winDocument the ETag and Cache-Control response headers.
The GET handler (see
handlers.rs:125-154) returnsETagandCache-Control: no-storeheaders, but these are not documented in the OpenAPI operation. Clients will not discover ETag support for conditional requests or caching directives.📝 Suggested enhancement
Add
.response_header()calls (or equivalent) to document:
ETag: Strong validator based on the version fieldCache-Control: Directive prohibiting intermediate caching of secret material🤖 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 `@modules/credstore/credstore/src/api/rest/routes.rs` around lines 61 - 83, The OpenAPI operation built with OperationBuilder::get for "credstore.get_secret" (handler handlers::get_secret) is missing documentation for the response headers that the handler actually returns; update the operation chain where GetSecretResponseDto is registered to include response header docs for "ETag" (strong validator derived from the secret's version field) and "Cache-Control" (value "no-store" to prohibit intermediate caching) by adding the appropriate .response_header(...) calls so clients can discover ETag support and the no-store caching directive.
40-59: ⚡ Quick winDocument the If-Match header for conditional updates.
The PUT handler (see
handlers.rs:94-117) parses theIf-Matchheader for optimistic locking, but this header is not documented in the OpenAPI operation. Clients will not discover this conditional update capability from the API specification.📝 Suggested enhancement
Add
.header_param()or equivalent to document the optionalIf-Matchrequest header, noting it accepts ETag values for conditional updates (409 Conflict on mismatch).🤖 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 `@modules/credstore/credstore/src/api/rest/routes.rs` around lines 40 - 59, The OpenAPI operation built with OperationBuilder::put for "credstore.put_secret" (which uses UpdateSecretRequestDto and is handled by handlers::put_secret) is missing documentation for the optional If-Match header the handler reads for optimistic locking; update the operation builder call to include a header parameter (e.g., via .header_param or equivalent) named "If-Match", describe it accepts ETag values for conditional updates, mark it optional, and note the behavior (409 Conflict returned on ETag mismatch) so clients can discover the conditional update capability.
20-38: ⚡ Quick winDocument the Location header in the OpenAPI spec.
The response description at line 32 mentions "see Location header," but the header is not formally registered in the OpenAPI operation. API consumers and generated clients will not see this header in the spec.
📝 Suggested enhancement
Consider using a method like
.response_header()(if available in OperationBuilder) to formally document the Location header, or add it to the operation description/documentation in a structured way that OpenAPI generators can parse.🤖 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 `@modules/credstore/credstore/src/api/rest/routes.rs` around lines 20 - 38, The OpenAPI operation built with OperationBuilder for the POST "/credstore/v1/secrets" (the chain that calls .json_request::<CreateSecretRequestDto>(), .handler(handlers::create_secret) and .no_content_response(StatusCode::CREATED, ...)) mentions a Location header but does not register it in the spec; update this OperationBuilder chain to document the Location response header (e.g., using .response_header("Location", "URI of created secret", HeaderType::String) or the appropriate OperationBuilder method available) so generated clients and API consumers see the Location header for the created resource.modules/credstore/credstore/src/infra/storage/entity/tenant_closure.rs (1)
1-1: ⚡ Quick winVerify the read-only claim is enforced at the application level.
The doc comment states this is a "Read-only" entity, but the default
ActiveModelBehaviorimplementation at line 24 permits writes (insert, update, delete). If this table is truly read-only from the application's perspective, consider adding a comment explaining where the enforcement happens, or implement a customActiveModelBehaviorthat panics on write operations to make the constraint explicit.Also applies to: 24-24
🤖 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 `@modules/credstore/credstore/src/infra/storage/entity/tenant_closure.rs` at line 1, The file documents tenant_closure as a read-only SeaORM entity but uses the default ActiveModelBehavior (symbol: ActiveModelBehavior) which allows writes; either add a clear doc comment pointing to where application-level enforcement occurs or implement a custom ActiveModelBehavior for the tenant_closure ActiveModel that explicitly prevents writes (e.g. panic or return an error on any insert/update/delete hooks) so the read-only constraint is enforced at compile/runtime; update tenant_closure.rs to replace the default impl ActiveModelBehavior for ActiveModel with your custom blocking implementation (or add the explanatory comment if enforcement is handled elsewhere).
🤖 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.
Nitpick comments:
In `@modules/credstore/credstore/src/api/rest/routes.rs`:
- Around line 61-83: The OpenAPI operation built with OperationBuilder::get for
"credstore.get_secret" (handler handlers::get_secret) is missing documentation
for the response headers that the handler actually returns; update the operation
chain where GetSecretResponseDto is registered to include response header docs
for "ETag" (strong validator derived from the secret's version field) and
"Cache-Control" (value "no-store" to prohibit intermediate caching) by adding
the appropriate .response_header(...) calls so clients can discover ETag support
and the no-store caching directive.
- Around line 40-59: The OpenAPI operation built with OperationBuilder::put for
"credstore.put_secret" (which uses UpdateSecretRequestDto and is handled by
handlers::put_secret) is missing documentation for the optional If-Match header
the handler reads for optimistic locking; update the operation builder call to
include a header parameter (e.g., via .header_param or equivalent) named
"If-Match", describe it accepts ETag values for conditional updates, mark it
optional, and note the behavior (409 Conflict returned on ETag mismatch) so
clients can discover the conditional update capability.
- Around line 20-38: The OpenAPI operation built with OperationBuilder for the
POST "/credstore/v1/secrets" (the chain that calls
.json_request::<CreateSecretRequestDto>(), .handler(handlers::create_secret) and
.no_content_response(StatusCode::CREATED, ...)) mentions a Location header but
does not register it in the spec; update this OperationBuilder chain to document
the Location response header (e.g., using .response_header("Location", "URI of
created secret", HeaderType::String) or the appropriate OperationBuilder method
available) so generated clients and API consumers see the Location header for
the created resource.
In `@modules/credstore/credstore/src/domain/error.rs`:
- Around line 36-41: The Internal enum variant's display string omits the
diagnostic field so error.to_string() only returns "internal error"; update the
error attribute on the Internal variant in error.rs (the enum variant named
Internal) to interpolate the diagnostic (e.g., include {diagnostic} in the
format) so diagnostic details appear in the formatted error, or alternatively
add a separate method (e.g., internal_diagnostic() or DisplayForLogging on the
error type) that returns the diagnostic for internal logging if you want to keep
the public Display unchanged.
In `@modules/credstore/credstore/src/infra/storage/entity/tenant_closure.rs`:
- Line 1: The file documents tenant_closure as a read-only SeaORM entity but
uses the default ActiveModelBehavior (symbol: ActiveModelBehavior) which allows
writes; either add a clear doc comment pointing to where application-level
enforcement occurs or implement a custom ActiveModelBehavior for the
tenant_closure ActiveModel that explicitly prevents writes (e.g. panic or return
an error on any insert/update/delete hooks) so the read-only constraint is
enforced at compile/runtime; update tenant_closure.rs to replace the default
impl ActiveModelBehavior for ActiveModel with your custom blocking
implementation (or add the explanatory comment if enforcement is handled
elsewhere).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fd95c64-f0bb-40f6-937b-1aa18a966e3c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
config/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamlmodules/credstore/credstore-sdk/README.mdmodules/credstore/credstore-sdk/src/api.rsmodules/credstore/credstore-sdk/src/error.rsmodules/credstore/credstore-sdk/src/error_tests.rsmodules/credstore/credstore-sdk/src/gts.rsmodules/credstore/credstore-sdk/src/lib.rsmodules/credstore/credstore-sdk/src/models.rsmodules/credstore/credstore-sdk/src/models_tests.rsmodules/credstore/credstore-sdk/src/plugin_api.rsmodules/credstore/credstore/Cargo.tomlmodules/credstore/credstore/README.mdmodules/credstore/credstore/src/api.rsmodules/credstore/credstore/src/api/rest.rsmodules/credstore/credstore/src/api/rest/dto.rsmodules/credstore/credstore/src/api/rest/dto_tests.rsmodules/credstore/credstore/src/api/rest/handlers.rsmodules/credstore/credstore/src/api/rest/routes.rsmodules/credstore/credstore/src/api/rest/routes_tests.rsmodules/credstore/credstore/src/client.rsmodules/credstore/credstore/src/client_tests.rsmodules/credstore/credstore/src/config.rsmodules/credstore/credstore/src/config_tests.rsmodules/credstore/credstore/src/domain.rsmodules/credstore/credstore/src/domain/authz.rsmodules/credstore/credstore/src/domain/error.rsmodules/credstore/credstore/src/domain/error_tests.rsmodules/credstore/credstore/src/domain/local_client.rsmodules/credstore/credstore/src/domain/local_client_tests.rsmodules/credstore/credstore/src/domain/mod.rsmodules/credstore/credstore/src/domain/ports.rsmodules/credstore/credstore/src/domain/ports/metrics.rsmodules/credstore/credstore/src/domain/ports/plugin.rsmodules/credstore/credstore/src/domain/resolver.rsmodules/credstore/credstore/src/domain/secret.rsmodules/credstore/credstore/src/domain/secret/model.rsmodules/credstore/credstore/src/domain/secret/repo.rsmodules/credstore/credstore/src/domain/secret/service.rsmodules/credstore/credstore/src/domain/secret/service_tests.rsmodules/credstore/credstore/src/domain/secret/test_support.rsmodules/credstore/credstore/src/domain/service.rsmodules/credstore/credstore/src/domain/service_tests.rsmodules/credstore/credstore/src/domain/test_support.rsmodules/credstore/credstore/src/infra.rsmodules/credstore/credstore/src/infra/canonical_mapping.rsmodules/credstore/credstore/src/infra/error_conv.rsmodules/credstore/credstore/src/infra/metrics.rsmodules/credstore/credstore/src/infra/metrics_tests.rsmodules/credstore/credstore/src/infra/plugin_select.rsmodules/credstore/credstore/src/infra/sdk_error_mapping.rsmodules/credstore/credstore/src/infra/storage.rsmodules/credstore/credstore/src/infra/storage/entity.rsmodules/credstore/credstore/src/infra/storage/entity/secrets.rsmodules/credstore/credstore/src/infra/storage/entity/tenant_closure.rsmodules/credstore/credstore/src/infra/storage/migrations.rsmodules/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rsmodules/credstore/credstore/src/infra/storage/repo_impl.rsmodules/credstore/credstore/src/infra/storage/repo_impl/helpers.rsmodules/credstore/credstore/src/infra/storage/repo_impl/reads.rsmodules/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsmodules/credstore/credstore/src/infra/storage/repo_impl/writes.rsmodules/credstore/credstore/src/infra/tenant_resolver.rsmodules/credstore/credstore/src/infra/tenant_resolver_tests.rsmodules/credstore/credstore/src/lib.rsmodules/credstore/credstore/src/module.rsmodules/credstore/docs/DESIGN-ADDENDUM.mdmodules/credstore/plugins/static-credstore-plugin/README.mdmodules/credstore/plugins/static-credstore-plugin/src/domain/client.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/service.rsmodules/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsmodules/mini-chat/mini-chat/src/infra/oagw_provisioning.rsmodules/system/oagw/oagw/src/domain/test_support.rsmodules/system/oagw/oagw/src/infra/plugin/apikey_auth.rsmodules/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsmodules/system/oagw/oagw/src/infra/proxy/service.rstesting/e2e/modules/mini_chat/config/base.yamltesting/e2e/modules/mini_chat/conftest.pytesting/e2e/modules/oagw/conftest.py
💤 Files with no reviewable changes (10)
- modules/credstore/credstore-sdk/src/error_tests.rs
- modules/credstore/credstore/src/domain/mod.rs
- modules/credstore/credstore/src/domain/local_client_tests.rs
- modules/credstore/credstore/src/domain/service.rs
- modules/credstore/credstore/src/config_tests.rs
- modules/credstore/credstore/src/domain/local_client.rs
- modules/credstore/credstore/src/domain/test_support.rs
- modules/credstore/credstore/src/domain/error_tests.rs
- modules/credstore/credstore/src/domain/service_tests.rs
- modules/credstore/credstore-sdk/src/models_tests.rs
✅ Files skipped from review due to trivial changes (7)
- modules/credstore/credstore/src/domain.rs
- modules/credstore/credstore/src/domain/ports.rs
- modules/credstore/credstore-sdk/README.md
- config/e2e-scope-enforcement.yaml
- modules/credstore/plugins/static-credstore-plugin/README.md
- modules/credstore/credstore/src/api/rest.rs
- modules/credstore/credstore/README.md
🚧 Files skipped from review as they are similar to previous changes (61)
- modules/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rs
- modules/credstore/credstore/src/domain/ports/plugin.rs
- modules/system/oagw/oagw/src/domain/test_support.rs
- modules/mini-chat/mini-chat/src/infra/oagw_provisioning.rs
- modules/credstore/credstore/src/infra.rs
- config/quickstart.yaml
- modules/credstore/credstore/src/api.rs
- config/static-tenants.yaml
- modules/credstore/credstore/src/domain/resolver.rs
- config/quickstart-windows.yaml
- modules/credstore/credstore/src/infra/storage/repo_impl/helpers.rs
- modules/credstore/credstore/src/infra/storage.rs
- modules/system/oagw/oagw/src/infra/plugin/apikey_auth.rs
- modules/credstore/credstore/src/infra/storage/entity.rs
- modules/credstore/credstore-sdk/src/plugin_api.rs
- modules/credstore/credstore/src/infra/storage/migrations.rs
- config/mini-chat.yaml
- testing/e2e/modules/mini_chat/config/base.yaml
- modules/credstore/credstore-sdk/src/lib.rs
- modules/system/oagw/oagw/src/infra/proxy/service.rs
- modules/credstore/credstore/src/infra/tenant_resolver_tests.rs
- modules/credstore/credstore/src/domain/secret/repo.rs
- modules/credstore/credstore/src/infra/sdk_error_mapping.rs
- modules/credstore/credstore/src/infra/storage/migrations/m0001_initial_schema.rs
- config/e2e-tr-authz.yaml
- modules/credstore/credstore/src/api/rest/dto_tests.rs
- modules/credstore/credstore/src/domain/secret/model.rs
- modules/credstore/credstore-sdk/src/api.rs
- modules/credstore/credstore/src/infra/tenant_resolver.rs
- modules/credstore/credstore-sdk/src/gts.rs
- modules/credstore/plugins/static-credstore-plugin/src/domain/client.rs
- modules/credstore/credstore/src/api/rest/handlers.rs
- testing/e2e/modules/mini_chat/conftest.py
- modules/credstore/credstore/src/infra/storage/entity/secrets.rs
- modules/credstore/credstore/src/domain/authz.rs
- modules/credstore/credstore/src/api/rest/dto.rs
- modules/credstore/credstore/src/infra/canonical_mapping.rs
- modules/credstore/credstore/src/domain/ports/metrics.rs
- modules/credstore/credstore-sdk/src/error.rs
- modules/credstore/credstore/src/infra/error_conv.rs
- modules/credstore/credstore/src/client_tests.rs
- modules/credstore/credstore/Cargo.toml
- modules/credstore/credstore/src/client.rs
- modules/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rs
- modules/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rs
- testing/e2e/modules/oagw/conftest.py
- modules/credstore/credstore/src/infra/metrics_tests.rs
- modules/credstore/credstore/src/infra/storage/repo_impl.rs
- config/e2e-local.yaml
- modules/credstore/credstore/src/lib.rs
- modules/credstore/credstore/src/domain/secret.rs
- modules/credstore/credstore/src/config.rs
- modules/credstore/credstore/src/module.rs
- modules/credstore/credstore/src/infra/storage/repo_impl/writes.rs
- modules/credstore/plugins/static-credstore-plugin/src/domain/service.rs
- modules/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rs
- modules/credstore/credstore/src/domain/secret/test_support.rs
- modules/credstore/credstore/src/infra/storage/repo_impl/reads.rs
- modules/credstore/credstore/src/domain/secret/service.rs
- modules/credstore/credstore/src/domain/secret/service_tests.rs
- modules/credstore/credstore/src/api/rest/routes_tests.rs
diffora
changed the title
diffora
pushed a commit
that referenced
this pull request
Jun 13, 2026
Resolve the second-round design review on the P1 DESIGN + companion specs: - #1 proxy/bandwidth: drop the wrong "extract stream-proxy into a byte-mover data-plane sidecar" framing; the OoP/edge escape hatch is a full stateless FileStorage replica on the shared/remote metadata DB (+ future own cache), needing no wire-contract change or trait extraction. Documented in §3.8 and cross-linked to the P3 OoP-gRPC variant. - #2 upload leak (put()→INSERT window): add a request-scoped best-effort cleanup guard (fire backend.delete on error after put before commit); keep the P2 orphan-reconciler as backstop for hard-kill; pending_uploads deferred. - #3 delete: specify metadata-row-first ordering + best-effort backend delete + idempotent re-delete (404); add a Delete sequence diagram in §3.6. - #4 metadata LWW: introduce optional If-Match-Metadata (matched on metadata_revision, 412 on mismatch); absent → last-write-wins (back-compat). - #5 PATCH overloading: require explicit ?replace_content=true for content replacement; content-without-flag and flag-without-content are 400. - #6 backend_id immutability: reword to "immutable in P1, relaxed in P2/P3"; add backend-migrator component + fr-backend-migration FR + Scope/phase entries; service-layer enforcement only (no DB constraint). - #7 multipart preview: cut the server-authoritative parts[]/concurrency shape (contradicted the client-driven model in api.md/migration.sql); keep only the P1-forward-compatible invariants, defer the contract to the P2 FEATURE. Also align acceptance criteria, conditional-headers, durability NFR, the orphan-reconciliation case list, and §3.7 field rules with the above. Signed-off-by: Roland From <rfedorov@linkentools.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
diffora
added a commit
that referenced
this pull request
Jul 6, 2026
CI fixes: - fmt: cargo fmt --all over the new credstore sources - clippy: manual_contains in credstore test_support - dylint DE0901: drop SECRET_TYPE_ID_PREFIX const (truncated GTS string); inline the literal into the starts_with assertion, which the lint skips - CFS: declare cpt-cf-credstore-seq-write-saga under §4.6 Interactions & Sequences (was under §6.2, outside the required design-tech-arch-seq section) - API contract: regenerate docs/api/api.json (make openapi) with the new credstore endpoints - cargo-deny: ignore RUSTSEC-2026-0194/0195 (quick-xml <0.41 DoS) — all vulnerable copies are transitive via kreuzberg 4.9.x (example app only), which pins quick-xml <0.41; same rationale as the existing lopdf ignore - e2e (mini-chat): provision credstore secrets under the rig's api-gateway prefix_path /cf — the fixture hit /credstore/... and got an empty 404 from the gateway, erroring all 331 tests at session setup CodeQL (github-advanced-security), 2 high alerts: - never echo the credstore provisioning response body (it could reflect the secret) and never derive log fields from the secret-value mappings; log only the ref name and HTTP status in both e2e conftests Review feedback (CodeRabbit): - credstore put: a row that vanished between the backend write and the version bump now surfaces DomainError::VersionConflict (canonical Aborted/409) instead of acknowledging success for an unreadable secret - mini-chat OAGW provisioning: differentiate create_upstream failures — AlreadyExists now recovers and reuses the existing upstream/alias (restart/idempotent re-run) instead of silently disabling a healthy provider; route re-registration treats AlreadyExists as success Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/api/api.json (2)
678-714: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
reference/refschema lacks the documented format constraints.The description states
[a-zA-Z0-9_-]+, max 255 chars, but the JSON Schema forCreateSecretRequestDto.referenceand therefpath parameter only declare"type": "string"with nopattern/maxLength. Other DTOs in this same spec (e.g.RequestOwnConversionDto.comment) do encode their prose contract viamaxLength/minLength. Generated clients validating against this schema won't catch malformed references before a round trip.💡 Suggested schema addition
"reference": { "description": "Secret reference key — `[a-zA-Z0-9_-]+`, max 255 characters.", + "pattern": "^[a-zA-Z0-9_-]+$", + "maxLength": 255, "type": "string" },Likely needs to be added at the
#[schema(...)]attribute source (not in this batch) and regenerated.Also applies to: 8194-8460
🤖 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 `@docs/api/api.json` around lines 678 - 714, The `CreateSecretRequestDto.reference` schema is missing the documented validation constraints, so add the same `pattern` and `maxLength` rules to the source `#[schema(...)]` metadata used for the `reference`/`ref` fields, then regenerate `docs/api/api.json` so the emitted JSON Schema enforces `[a-zA-Z0-9_-]+` with a 255-character limit. Make sure the update is applied consistently to both the DTO field and the `ref` path parameter definitions so generated clients validate the same contract.
8113-8460: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftETag/If-Match contract undocumented in OpenAPI.
The PR's headline feature is
ETag/If-Matchoptimistic-concurrency versioning (confirmed byroutes_tests.rscovering matching/stale/malformedIf-Matchon PUT/DELETE), andSecretMetadataDto.versioneven states it is "also returned asETag". However, none of the/credstore/v1/secrets/{ref}PUT/DELETE operations declare anIf-Matchheader parameter, and GET/POST don't declare anETagresponse header — yet 409 Conflict is documented with no visible trigger. Generated API clients won't know to send/read these headers for correct optimistic-concurrency usage.🤖 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 `@docs/api/api.json` around lines 8113 - 8460, The OpenAPI for the Credential Store secret endpoints is missing the optimistic-concurrency header contract, so update the `/credstore/v1/secrets/{ref}` operations to document it properly. Add an `If-Match` header parameter to `credstore.put_secret` and `credstore.delete_secret`, and add an `ETag` response header to `credstore.get_secret`, `credstore.create_secret`, and any response that returns `SecretMetadataDto.version`. Use the existing operation IDs and the secret DTO/schema definitions to keep the header names and version semantics consistent with the implementation.
🤖 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.
Nitpick comments:
In `@docs/api/api.json`:
- Around line 678-714: The `CreateSecretRequestDto.reference` schema is missing
the documented validation constraints, so add the same `pattern` and `maxLength`
rules to the source `#[schema(...)]` metadata used for the `reference`/`ref`
fields, then regenerate `docs/api/api.json` so the emitted JSON Schema enforces
`[a-zA-Z0-9_-]+` with a 255-character limit. Make sure the update is applied
consistently to both the DTO field and the `ref` path parameter definitions so
generated clients validate the same contract.
- Around line 8113-8460: The OpenAPI for the Credential Store secret endpoints
is missing the optimistic-concurrency header contract, so update the
`/credstore/v1/secrets/{ref}` operations to document it properly. Add an
`If-Match` header parameter to `credstore.put_secret` and
`credstore.delete_secret`, and add an `ETag` response header to
`credstore.get_secret`, `credstore.create_secret`, and any response that returns
`SecretMetadataDto.version`. Use the existing operation IDs and the secret
DTO/schema definitions to keep the header names and version semantics consistent
with the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bad8b58-3e6c-43eb-a72c-e9ca1f23d6ec
📒 Files selected for processing (32)
deny.tomldocs/api/api.jsongears/credstore/credstore-sdk/src/types.rsgears/credstore/credstore/src/api/rest/dto.rsgears/credstore/credstore/src/api/rest/routes_tests.rsgears/credstore/credstore/src/domain/error.rsgears/credstore/credstore/src/domain/secret.rsgears/credstore/credstore/src/domain/secret/service.rsgears/credstore/credstore/src/domain/secret/service_tests.rsgears/credstore/credstore/src/domain/secret/test_support.rsgears/credstore/credstore/src/domain/secret/typing.rsgears/credstore/credstore/src/domain/secret/typing_tests.rsgears/credstore/credstore/src/gear.rsgears/credstore/credstore/src/infra/canonical_mapping.rsgears/credstore/credstore/src/infra/error_conv.rsgears/credstore/credstore/src/infra/storage/entity/secrets.rsgears/credstore/credstore/src/infra/storage/entity/tenant_closure.rsgears/credstore/credstore/src/infra/storage/repo_impl.rsgears/credstore/credstore/src/infra/storage/repo_impl/helpers.rsgears/credstore/credstore/src/infra/storage/repo_impl/reads.rsgears/credstore/credstore/src/infra/storage/repo_impl/repo_tests.rsgears/credstore/credstore/src/infra/storage/repo_impl/writes.rsgears/credstore/credstore/src/infra/tenant_resolver.rsgears/credstore/credstore/src/infra/tenant_resolver_tests.rsgears/credstore/credstore/src/lib.rsgears/credstore/docs/DESIGN.mdgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsgears/mini-chat/mini-chat/src/infra/oagw_provisioning.rstesting/e2e/gears/mini_chat/conftest.pytesting/e2e/gears/oagw/conftest.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testing/e2e/gears/mini_chat/conftest.py (1)
509-510: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace
returnwithbreakhere._provision_credstore_secretsis a session-scoped generator fixture; returning before theyieldaborts setup with “did not yield a value” and fails the whole session.breakstill reaches theyieldand lets_await_oagw_upstreamsno-op whenprovisioned == 0.🤖 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 `@testing/e2e/gears/mini_chat/conftest.py` around lines 509 - 510, In _provision_credstore_secrets, replace the early return inside the httpx.ConnectError handler with a break so the session-scoped generator fixture still reaches its yield point; this prevents the “did not yield a value” failure while preserving the intended no-op behavior for _await_oagw_upstreams when provisioned remains 0.
🤖 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.
Outside diff comments:
In `@testing/e2e/gears/mini_chat/conftest.py`:
- Around line 509-510: In _provision_credstore_secrets, replace the early return
inside the httpx.ConnectError handler with a break so the session-scoped
generator fixture still reaches its yield point; this prevents the “did not
yield a value” failure while preserving the intended no-op behavior for
_await_oagw_upstreams when provisioned remains 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 435ac603-45bb-485a-a44f-5b131dc9a148
📒 Files selected for processing (6)
gears/credstore/docs/DESIGN.mdgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/mini-chat/mini-chat/src/gear.rsgears/mini-chat/mini-chat/src/infra/oagw_provisioning.rstesting/e2e/gears/mini_chat/conftest.pytesting/e2e/gears/oagw/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- testing/e2e/gears/oagw/conftest.py
- gears/mini-chat/mini-chat/src/infra/oagw_provisioning.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/service.rs
diffora
pushed a commit
that referenced
this pull request
Jul 7, 2026
`store.rs` was the highest Henry-Kafura hotspot in the gear (HK 7.35M):
a 949-line unit-of-work facade that named all nine repository types
directly (fan_out 22) while four modules routed through it (fan_in 4).
Cut the facade's own fan_out without relocating coupling onto its
dominant consumer (`service.rs`, which makes ~83 of the calls — an
ISP split by caller would have made it worse):
- Introduce a `Repos` aggregate that owns the nine repositories.
`Store` now depends on one collaborator instead of naming every
repo type; the coupling to the individual repo modules lives on
`Repos` (a node with fan_in 1), off the `Store` crossroads.
- Move the three raw `Entity::find().secure()` sweep queries out of
`store.rs` into `FileRepo::list_all_for_sweep`,
`RetentionRuleRepo::list_by_file_scope`, and
`RetentionRuleRepo::list_all`. This removes the entity / sea_orm
imports from the facade, drops ~60 lines, and fixes a layering
violation (the store was issuing SQL the repos are meant to own) —
the retention queries now reuse the repo's existing `map_model`
instead of duplicating the deserialization inline.
Result: store.rs HK 7,349,056 -> 3,641,344 (-50%), fan_out 22 -> 16,
sloc 949 -> 889. It drops from the #1 HK hotspot to #2; service.rs is
unchanged (coupling dissolved, not relocated) and the repo modules stay
cheap (retention_rule_repo HK 8,832). clippy --all-targets, fmt, and
all 157 tests clean.
Signed-off-by: Roland From <rfedorov@linkentools.com>
diffora
pushed a commit
that referenced
this pull request
Jul 7, 2026
…am type
Cuts the persistence facade's Henry-Kafura coupling by ~81% (HK 3,768,700 ->
721,968) without fragmenting its transaction logic. Two orthogonal moves:
1. fan-in (the dominant lever): introduce domain-owned capability ports
`domain::ports::{CleanupStore, MultipartStore}` — narrow slices of Store's
~40-method surface. `CleanupEngine` and `MultipartService` now hold
`Arc<dyn CleanupStore>` / `Arc<dyn MultipartStore>` instead of the concrete
`Store`, and `DataPlaneService` routes its single `get_version` call through
a new `FileService::get_version` delegation instead of holding a `Store`.
`Store` implements the ports (UFCS delegation to the inherent methods); the
composition root (`gear`) upcasts the concrete `Store` when wiring. Only
`FileService` and `gear` now name `Store` directly, so its fan-in drops 5 -> 2.
This is the ISP/DIP remedy: consumers depend on the interface they use, the
domain owns the port, infra implements it.
2. fan-out: move the `InsertRetentionRule` param struct onto the `repo/mod`
facade (like `AuditRow`/`FileEventRow`), so `store` no longer edges into the
`retention_rule_repo` submodule. fan-out 13 -> 12.
`Store` remains a single whole unit-of-work facade — the transaction boundary is
the seam, and it is kept intact; only the consumer-facing coupling is segregated.
`store.rs` drops from the #1 HK hub to #2. No behaviour change; 161 tests pass,
clippy clean.
Claude-Session: https://claude.ai/code/session_017PeAM2pn6AAGnBGM7c76Fp
Signed-off-by: Roland From <rfedorov@linkentools.com>
diffora
pushed a commit
that referenced
this pull request
Jul 7, 2026
`service.rs` was the crate's #1 Henry-Kafura hub. Two edge-dissolving moves, neither fragmenting the orchestrator: 1. fan-in 4 -> 3 (ISP/DIP): introduce a domain-owned `DataPlanePort` (`backends`, `authorize_write`, `get_version`, `finalize_upload`). `DataPlaneService` now holds `Arc<dyn DataPlanePort>` instead of the concrete `Arc<FileService>`, so it no longer names `FileService` — the data plane depends on the narrow capability it uses. `FileService` implements the port. 2. fan-out 14 -> 13: `migrate_backend` was the sole user of `infra::content::hash` in this file. Move the SHA-256 verification into `Store::verify_content_hash` (Store already imports `hash`, so no new edge for it), and `service.rs` drops its `content::hash` edge. HK 3,524,864 -> 2,019,780 (-43%). service.rs remains #1 but far closer to the pack; no sibling rose past it. No behaviour change; 165 tests pass, clippy + fmt clean. Claude-Session: https://claude.ai/code/session_017PeAM2pn6AAGnBGM7c76Fp Signed-off-by: Roland From <rfedorov@linkentools.com>
diffora
pushed a commit
that referenced
this pull request
Jul 7, 2026
…ng submodules The earlier ≤600-SLOC split fragmented `impl FileService` into ~10 tiny `service/*.rs` files; each `use super::FileService` is a fan_in edge to `service/mod.rs`, and since HK = sloc × (fan_in × fan_out)² the inflated fan_in dominated (HK ~2.19M, the #1 hub). Consolidate the over-fragmentation into the fewest cohesive files that still fit the 600-SLOC budget (write-path, read-path, backend groups), cutting the number of `use super::FileService` submodule edges. The path split is preserved (files stay ≤600 SLOC); only the surplus fragmentation is removed — no change to real coupling. service/mod.rs HK ~2,192,832 -> 726,327 (below the 1M target). 168 tests pass, clippy (-D warnings) + fmt clean. Claude-Session: https://claude.ai/code/session_017PeAM2pn6AAGnBGM7c76Fp Signed-off-by: Roland From <rfedorov@linkentools.com>
diffora
force-pushed
the
credstore
branch
11 times, most recently
from
July 8, 2026 20:44
07c1885 to
39d3b46
Compare
…iven secret types Port the credstore gear: a tenant-scoped, PDP-enforced credential gateway over pluggable value-store backends, with hierarchical resolution, a crash-safe saga lifecycle, and dynamic GTS secret typing. Gateway & domain - Stateful metadata gateway (credstore_secrets) over a trait-agnostic value-store plugin SPI; hierarchical read resolves the closest accessible secret up the barrier-respecting tenant ancestor chain (private > tenant > shared), reading the backend once for the winner. - Crash-safe write/delete sagas (provisioning / active / deprovisioning) with a background reaper that reconciles orphaned backend values, releases wedged references, and sweeps expired secrets; optimistic concurrency via If-Match / version. Authorization - Single PDP evaluation per operation on the secret's full concrete GTS type (incl. generic), gated on the caller's tenant; hierarchical visibility decided by the resolver, not the PDP. GET denial/miss is an anti-enumeration 404; PDP outage is 503. Registry-driven secret types - Secret types are GTS types derived from gts.cf.core.credstore.secret.v1~; the types-registry is the runtime source of truth (SecretTypeResolver resolves a type's id + effective traits per operation, no local cache), so custom types register a GTS schema with no credstore release. The compiled-in catalog only seeds the built-in type schemas. - Enforced traits (allow_sharing, value_schema, max_size_bytes, expirable, utf8_only, advisory rotation_period_secs) validated on write against the resolved traits; type is immutable for a secret's lifetime. - Storage: secret_type_uuid UUID (deterministic v5 of the GTS id), default pinned to the generic type. SDK/REST accept a type as catalog short name, full GTS id, or UUID (SecretTypeRef); responses carry the resolved id. - Fail-closed: unknown/non-secret type -> 400 UNKNOWN_SECRET_TYPE on create, 503 for a stored row; registry outage/timeout/malformed traits -> 503. Value-fingerprint fence & optimistic-concurrency hardening - Each row carries value_fp = HMAC-SHA256(fence_key, value), stamped in the same atomic write as the sharing label; reads verify it against the backend value and fail closed (anti-enumeration 404) on mismatch, so two crosswise concurrent last-writer-wins PUTs can never serve one writer's value under another writer's sharing label (closes the cross-tenant disclosure). The fence key is auto-generated and stored in the backend under an API-unreachable reserved entry (split knowledge; the fingerprint never leaves the gateway, in responses or logs). - Strong ETag is generation-bound "<row-id>.<version>": a validator from a deleted-and-recreated secret's earlier generation never matches even when the version counters coincide (no ABA lost update). - Out-of-band seeded rows (value_fp NULL) are served on trust and backfilled lazily on read and by the reaper sweep. See DESIGN §4.10 and ADR-0003. Integration & docs - OAGW and mini-chat consume credstore through the SDK client; e2e and integration coverage; DESIGN.md, generated OpenAPI spec, and ADRs. Signed-off-by: Diffora <ddiffora@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@config/quickstart.yaml`:
- Around line 350-353: The curl example comment uses the wrong gateway port, so
update the example to match the api-gateway configuration that binds at
127.0.0.1:8087 with the /cf prefix. Fix the commented command near the secrets
example so users hit the same endpoint as the configured api-gateway, and keep
the path consistent with cf/credstore/v1/secrets/openai-key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c405d106-5806-41d5-996c-0e637d57033e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.gitignoreconfig/e2e-local.yamlconfig/e2e-scope-enforcement.yamlconfig/e2e-tr-authz.yamlconfig/mini-chat.yamlconfig/quickstart-windows.yamlconfig/quickstart.yamlconfig/static-tenants.yamldocs/api/api.json
💤 Files with no reviewable changes (1)
- docs/api/api.json
✅ Files skipped from review due to trivial changes (3)
- config/e2e-tr-authz.yaml
- .gitignore
- config/mini-chat.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- config/quickstart-windows.yaml
- config/e2e-scope-enforcement.yaml
- config/e2e-local.yaml
- config/static-tenants.yaml
Comment on lines
+350
to
+353
| # curl -X PUT localhost:8080/cf/credstore/v1/secrets/openai-key \ | ||
| # -H 'content-type: application/json' \ | ||
| # -H 'authorization: Bearer <token>' \ | ||
| # -d '{"value":"sk-...","sharing":"tenant"}' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Incorrect port in the curl example comment.
The example curl command uses localhost:8080, but the api-gateway in this config binds to 127.0.0.1:8087 (Line 91) with prefix_path: "/cf" (Line 94). Users following this example will get a connection refused.
📝 Proposed fix for the curl example port
- # curl -X PUT localhost:8080/cf/credstore/v1/secrets/openai-key \
+ # curl -X PUT localhost:8087/cf/credstore/v1/secrets/openai-key \📝 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
| # curl -X PUT localhost:8080/cf/credstore/v1/secrets/openai-key \ | |
| # -H 'content-type: application/json' \ | |
| # -H 'authorization: Bearer <token>' \ | |
| # -d '{"value":"sk-...","sharing":"tenant"}' | |
| # curl -X PUT localhost:8087/cf/credstore/v1/secrets/openai-key \ | |
| # -H 'content-type: application/json' \ | |
| # -H 'authorization: Bearer <token>' \ | |
| # -d '{"value":"sk-...","sharing":"tenant"}' |
🤖 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 `@config/quickstart.yaml` around lines 350 - 353, The curl example comment uses
the wrong gateway port, so update the example to match the api-gateway
configuration that binds at 127.0.0.1:8087 with the /cf prefix. Fix the
commented command near the secrets example so users hit the same endpoint as the
configured api-gateway, and keep the path consistent with
cf/credstore/v1/secrets/openai-key.
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
Replaces the partial credstore gateway/SDK with the new stateful-gateway design ported from
vhp-core-credstore, adapts it to this workspace, and wires it through the run configs and e2e harness. Framework lib versions here matched the vendoredcyberfabric-coresubmodule exactly, so the source compiled unchanged — onlyCargo.tomldep keys were remapped to the workspace aliases.The gateway now owns per-secret metadata in its own DB (
credstore_secrets, sea-orm migrationm0001), enforces PDPAccessScopein SQL, resolves secrets hierarchically (TTL+LRU ancestor-chain cache, isolation barriers), adds versioning (ETag/If-Match), a crash-safe write saga + reaper, OpenTelemetry metrics, and a REST API (/credstore/v1/secrets). The backend plugin is now a value-only store.Doc:
modules/credstore/docs/DESIGN-ADDENDUM.md(placed next to the existingDESIGN.md/PRD.md).Changes
CredStoreClientV1gainsput/create/delete;GetSecretResponsegainsversion;CredStorePluginClientV1becomes a pure per-tenant value store (get/put/deletebytenant_id/key/owner_id);SecretMetadataremoved.api/rest,infra/storage,domain/secret,authz,resolver, metrics); deps rewritten (modkit-db,modkit-canonical-errors,authz-resolver-sdk,sea-orm(-migration),axum,time,opentelemetry,test-supportfeature).put/create/delete+version). No production changes.credstoremodule's owndatabase:+vendor: cyberfabricblock to the run configs that boot the example server (the stateful module callsdb_required()and selects its backend plugin by GTS vendor).oagw(apikey/oauth2) andmini_chat.Two runtime bugs caught by boot-testing (and fixed)
fix(credstore): restore thesystemcapability. This repo'soagwis asystemmodule that resolvesCredStoreClientV1during its owninit, and modkit inits all system modules before non-system ones — so the ported (non-system) credstore registered its client too late and oagw failed withclient not found. Restoredcapabilities = [system, db, rest, stateful]+ emptyimpl SystemCapability.fix(mini-chat): non-fatal per-provider OAGW provisioning. mini-chat eagerly provisions OAGW upstreams at startup; oagw validates the secret is accessible at upstream-creation time, and the stateful gateway has nothing provisioned at boot → mini-chat start aborted. Now logs a warning and skips that provider (degrades via the hostupstream_aliasfallback) instead of crashing boot.Validation
config/e2e-local.yaml; credstore migrates (credstore_secrets+ indexes), credstore initializes before oagw, and mini-chat now starts.test_websocketskipped — missingwebsocketsPython dep, unrelated.Not validated here (needs CI env)
Full
make e2e-local(release build + the whole Python suite) andmake e2e-mini-chatfor final CI-level confirmation — couldn't run a release build + full harness in this environment. All known credstore boot/integration blockers are closed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes