WIP fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382
WIP fix(deps): bump cipherstash-client from 0.33.2 to 0.34.0-alpha.2#382
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughReplaces config-driven ZeroKMS initialization with a builder/AutoStrategy-based flow using client-id/key from env; updates workspace and proxy package dependencies; adds a ZeroKMSBuilder error variant; adjusts error matching and updates related tests and docker env passthrough. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (CS_CLIENT_ID / CS_CLIENT_KEY)
participant Proxy as Proxy init
participant Strategy as AutoStrategy
participant Builder as ZeroKMSBuilder
participant ZeroKMS as ZeroKMS Client
Env->>Proxy: provide CS_CLIENT_ID & CS_CLIENT_KEY
Proxy->>Strategy: AutoStrategy::detect() / select strategy
Strategy-->>Proxy: AutoStrategy
Proxy->>Builder: ZeroKMSBuilder::auto().with_client_key(client_key)
Builder->>ZeroKMS: build()
ZeroKMS-->>Proxy: ZerokmsClient (ready)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
8cb9a16 to
c524252
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy-integration/src/generate.rs`:
- Around line 130-142: The test currently reads local credentials via std::env
(CS_CLIENT_ID/CS_CLIENT_KEY) and calls ZeroKMSBuilder::auto(), making it
host-dependent; move credential acquisition and ZeroKMS instantiation into the
Docker-backed integration test harness and consume them from the harness instead
of environment and auto discovery: replace direct uses of std::env::var and
ClientKey::from_hex_v1(client_id, &client_key_hex) and the
ZeroKMSBuilder::auto().unwrap() chain with injected fixtures or helper functions
provided by the Docker harness (e.g., have the harness supply a ClientKey
instance or a ZeroKMS client via a helper like harness.zero_kms_client() or
ZeroKMSBuilder::from_client(key)), and update the test to use these
harness-provided values so no local env reads or auto discovery occur in
generate.rs.
In `@packages/cipherstash-proxy/src/error.rs`:
- Around line 57-58: Remove the top-level Error::ZeroKMSBuilder variant and
instead surface builder failures under the ZeroKMSError domain: add a Builder
variant (or reuse System) to the ZeroKMSError enum, move the #[from] conversion
for cipherstash_client::zerokms::ZeroKMSBuilderError into that ZeroKMSError
variant, and update Error to hold ZeroKMSError (e.g.,
Error::ZeroKMSError(#[from] ZeroKMSError)); this means deleting the
ZeroKMSBuilder entry from the Error enum and wiring the
From<cipherstash_client::zerokms::ZeroKMSBuilderError> conversion to produce
ZeroKMSError::Builder (or map to ZeroKMSError::System) so all ZeroKMS-related
errors are grouped under ZeroKMSError.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs`:
- Around line 26-32: The code currently panics on malformed operator config by
calling expect() when parsing client_id and client_key and when handling
zerokms_host; replace these panics with proper error propagation: validate and
parse the UUID (client_id) and the hex client key (ClientKey::from_hex_v1) and
return a typed error (or map the parsing errors into your function's error type)
instead of calling expect(), and likewise validate/parse zerokms_host and return
an error on failure; alternatively move these validations into the config
loading path so the values are guaranteed valid before reaching the code paths
that use client_id, ClientKey::from_hex_v1, and zerokms_host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 288b8a24-26ba-43f8-bf17-1c654823250b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy/src/proxy/mod.rs
| let client_id: uuid::Uuid = std::env::var("CS_CLIENT_ID") | ||
| .expect("CS_CLIENT_ID must be set") | ||
| .parse() | ||
| .expect("CS_CLIENT_ID must be a valid UUID"); | ||
| let client_key_hex = | ||
| std::env::var("CS_CLIENT_KEY").expect("CS_CLIENT_KEY must be set"); | ||
| let client_key = ClientKey::from_hex_v1(client_id, &client_key_hex) | ||
| .expect("CS_CLIENT_KEY must be valid hex"); | ||
| let zerokms_client = ZeroKMSBuilder::auto() | ||
| .unwrap() | ||
| .with_client_key(client_key) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This integration test is still host-dependent.
The new setup still reads local credentials and performs a live ZeroKMS auth flow, so the test only runs on a preconfigured machine. Please move this behind the Docker-backed integration harness instead of wiring std::env + ZeroKMSBuilder::auto() directly in the test body.
As per coding guidelines, "Integration tests should use Docker containers for reproducible environments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy-integration/src/generate.rs` around lines 130 -
142, The test currently reads local credentials via std::env
(CS_CLIENT_ID/CS_CLIENT_KEY) and calls ZeroKMSBuilder::auto(), making it
host-dependent; move credential acquisition and ZeroKMS instantiation into the
Docker-backed integration test harness and consume them from the harness instead
of environment and auto discovery: replace direct uses of std::env::var and
ClientKey::from_hex_v1(client_id, &client_key_hex) and the
ZeroKMSBuilder::auto().unwrap() chain with injected fixtures or helper functions
provided by the Docker harness (e.g., have the harness supply a ClientKey
instance or a ZeroKMS client via a helper like harness.zero_kms_client() or
ZeroKMSBuilder::from_client(key)), and update the test to use these
harness-provided values so no local env reads or auto discovery occur in
generate.rs.
| #[error(transparent)] | ||
| ZeroKMSBuilder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/cipherstash-proxy/src/error.rsRepository: cipherstash/proxy
Length of output: 22399
🏁 Script executed:
rg -i "zerokmsbuilder" --type rustRepository: cipherstash/proxy
Length of output: 728
🏁 Script executed:
cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rsRepository: cipherstash/proxy
Length of output: 1879
🏁 Script executed:
rg "init_zerokms_client" --type rust -A 2 -B 2Repository: cipherstash/proxy
Length of output: 1391
🏁 Script executed:
rg "ZeroKMSBuilderError" --type rust -B 5 -A 5 | head -50Repository: cipherstash/proxy
Length of output: 2502
Move ZeroKMSBuilder errors into the ZeroKMSError domain enum.
ZeroKMSBuilder exposes an implementation detail (the builder pattern) in the public error taxonomy instead of grouping by problem domain. Builder failures are initialization-phase ZeroKMS errors—the same domain as runtime ZeroKMS failures. Consolidate this into ZeroKMSError (e.g., add a Builder variant or map to the existing System variant), then remove the top-level ZeroKMSBuilder variant from the Error enum. This aligns with the guideline requiring errors to be "grouped by problem domain (not module structure)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy/src/error.rs` around lines 57 - 58, Remove the
top-level Error::ZeroKMSBuilder variant and instead surface builder failures
under the ZeroKMSError domain: add a Builder variant (or reuse System) to the
ZeroKMSError enum, move the #[from] conversion for
cipherstash_client::zerokms::ZeroKMSBuilderError into that ZeroKMSError variant,
and update Error to hold ZeroKMSError (e.g., Error::ZeroKMSError(#[from]
ZeroKMSError)); this means deleting the ZeroKMSBuilder entry from the Error enum
and wiring the From<cipherstash_client::zerokms::ZeroKMSBuilderError> conversion
to produce ZeroKMSError::Builder (or map to ZeroKMSError::System) so all
ZeroKMS-related errors are grouped under ZeroKMSError.
| let client_id: uuid::Uuid = config | ||
| .encrypt | ||
| .client_id | ||
| .parse() | ||
| .expect("client_id must be a valid UUID"); | ||
| let client_key = ClientKey::from_hex_v1(client_id, &config.encrypt.client_key) | ||
| .expect("client_key must be valid hex"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in zerokms/mod.rs
cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rs | head -50Repository: cipherstash/proxy
Length of output: 1879
🏁 Script executed:
# Check if error.rs exists and what error types are defined
cat -n packages/cipherstash-proxy/src/error.rs | head -100Repository: cipherstash/proxy
Length of output: 3676
🏁 Script executed:
# Search for other parsing operations in the codebase to see how they handle errors
rg -A 3 "\.parse\(\)" packages/cipherstash-proxy/src/ --max-count 10Repository: cipherstash/proxy
Length of output: 2558
🏁 Script executed:
# Look for config validation or EncryptConfig usage
rg -B 5 -A 5 "EncryptConfig\|client_id\|client_key" packages/cipherstash-proxy/src/ -t rs | head -60Repository: cipherstash/proxy
Length of output: 87
🏁 Script executed:
# Search for EncryptConfig definition
rg "struct EncryptConfig\|pub struct EncryptConfig" packages/cipherstash-proxy/src/ -A 5Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Look for where init_zerokms_client is called and how config is loaded
rg "init_zerokms_client" packages/cipherstash-proxy/src/ -B 3 -A 3Repository: cipherstash/proxy
Length of output: 1960
🏁 Script executed:
# Look for config loading and validation logic
find packages/cipherstash-proxy/src/config -name "*.rs" -type fRepository: cipherstash/proxy
Length of output: 392
🏁 Script executed:
# Check the main config module to see how EncryptConfig is defined
grep -n "EncryptConfig" packages/cipherstash-proxy/src/config/mod.rs -A 10 -B 2Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Look at tandem.rs for EncryptConfig definition
grep -n "EncryptConfig\|struct.*Encrypt\|pub.*encrypt" packages/cipherstash-proxy/src/config/tandem.rs -A 5 -B 2Repository: cipherstash/proxy
Length of output: 899
🏁 Script executed:
# Check if there's validation in the config loading
cat -n packages/cipherstash-proxy/src/config/tandem.rs | head -100Repository: cipherstash/proxy
Length of output: 3499
🏁 Script executed:
# Continue reading tandem.rs to see config loading and validation logic
cat -n packages/cipherstash-proxy/src/config/tandem.rs | sed -n '95,200p'Repository: cipherstash/proxy
Length of output: 4813
🏁 Script executed:
# Check the ZeroKMS.init function to understand the error flow
cat -n packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs | head -80Repository: cipherstash/proxy
Length of output: 3350
🏁 Script executed:
# Search for ConfigError definition to understand how config parsing errors are handled
rg "enum ConfigError\|struct ConfigError" packages/cipherstash-proxy/src/ -A 10Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Search for ConfigError in error.rs
cat -n packages/cipherstash-proxy/src/error.rs | grep -A 20 "enum ConfigError\|struct ConfigError"Repository: cipherstash/proxy
Length of output: 834
🏁 Script executed:
# Let's see the full error.rs file structure
wc -l packages/cipherstash-proxy/src/error.rsRepository: cipherstash/proxy
Length of output: 104
🏁 Script executed:
# Get the second part of error.rs to find ConfigError
cat -n packages/cipherstash-proxy/src/error.rs | sed -n '100,250p'Repository: cipherstash/proxy
Length of output: 6270
🏁 Script executed:
# Check the complete config building to see if validation happens before init_zerokms_client is called
cat -n packages/cipherstash-proxy/src/config/tandem.rs | sed -n '199,220p'Repository: cipherstash/proxy
Length of output: 1651
🏁 Script executed:
# Look at the main.rs or lib.rs to understand when ZeroKms::init is called
find packages/cipherstash-proxy/src -name "main.rs" -o -name "lib.rs" | head -5Repository: cipherstash/proxy
Length of output: 137
🏁 Script executed:
# Check when ZeroKms::init is called
rg "ZeroKms::init\|ZeroKms::new" packages/cipherstash-proxy/src/ -B 3 -A 3Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Look at what ZeroKMSBuilderError contains
rg "pub.*struct ZeroKMSBuilderError\|pub.*enum ZeroKMSBuilderError" packages/cipherstash-proxy/src/ -A 10Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Check main.rs to see how ZeroKms is initialized
cat -n packages/cipherstash-proxy/src/main.rs | head -100Repository: cipherstash/proxy
Length of output: 4063
🏁 Script executed:
# Search for ZeroKms initialization anywhere in the codebase
rg "ZeroKms" packages/cipherstash-proxy/src/main.rs packages/cipherstash-proxy/src/lib.rs -B 2 -A 2Repository: cipherstash/proxy
Length of output: 43
🏁 Script executed:
# Check if there's validation of client_id as UUID format before init_zerokms_client
rg "client_id" packages/cipherstash-proxy/src/config/ -B 2 -A 2Repository: cipherstash/proxy
Length of output: 2537
🏁 Script executed:
# Check how init_zerokms_client is called from ZeroKms::init
cat -n packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs | sed -n '44,46p'Repository: cipherstash/proxy
Length of output: 205
🏁 Script executed:
# Check the exact return type and how errors should be handled
cat -n packages/cipherstash-proxy/src/proxy/zerokms/mod.rs | sed -n '16,19p'Repository: cipherstash/proxy
Length of output: 248
🏁 Script executed:
# Let's verify what happens if we look at how other builders handle parse errors in the codebase
rg "\.parse\(\)" packages/cipherstash-proxy/src/ -B 2 -A 5 | grep -A 10 "Result\|map_err\|expect"Repository: cipherstash/proxy
Length of output: 2469
Map config parse failures to proper errors instead of panicking.
Lines 26-32 and 39-42 use expect() on values from operator config (client_id, client_key, zerokms_host). These are not validated during config loading, so malformed values will abort startup instead of returning a proper typed error. Either validate these values during config load (preferred), or convert parse failures into appropriate error types returned from this function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs` around lines 26 - 32,
The code currently panics on malformed operator config by calling expect() when
parsing client_id and client_key and when handling zerokms_host; replace these
panics with proper error propagation: validate and parse the UUID (client_id)
and the hex client key (ClientKey::from_hex_v1) and return a typed error (or map
the parsing errors into your function's error type) instead of calling expect(),
and likewise validate/parse zerokms_host and return an error on failure;
alternatively move these validations into the config loading path so the values
are guaranteed valid before reaching the code paths that use client_id,
ClientKey::from_hex_v1, and zerokms_host.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/cipherstash-proxy/src/error.rs (1)
55-58:⚠️ Potential issue | 🟠 MajorKeep builder failures inside the ZeroKMS domain.
Error::ZeroKMSBuildersplits one problem domain across two top-level variants and leaks the construction mechanism into the public error taxonomy. Foldcipherstash_client::zerokms::ZeroKMSBuilderErrorintoZeroKMSErrorinstead, then keepErrorexposing onlyError::ZeroKMS.Suggested shape
pub enum Error { #[error(transparent)] ZeroKMS(#[from] ZeroKMSError), - - #[error(transparent)] - ZeroKMSBuilder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError), } pub enum ZeroKMSError { #[error("ZeroKMS authentication failed. Check the configured credentials. For help visit {}#zerokms-authentication-failed", ERROR_DOC_BASE_URL)] AuthenticationFailed, + + #[error(transparent)] + Builder(#[from] cipherstash_client::zerokms::ZeroKMSBuilderError), #[error(transparent)] System(#[from] cipherstash_client::zerokms::Error), }As per coding guidelines: "All errors must be defined in
packages/cipherstash-proxy/src/error.rsand grouped by problem domain (not module structure). Use descriptive variant names without 'Error' suffix."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/error.rs` around lines 55 - 58, Replace the top-level Error::ZeroKMSBuilder variant by folding cipherstash_client::zerokms::ZeroKMSBuilderError into the ZeroKMSError domain: add a From<ZeroKMSBuilderError> (or #[from]) conversion inside the ZeroKMSError definition and remove the Error::ZeroKMSBuilder variant so the public Error enum only exposes Error::ZeroKMS (which remains #[from] ZeroKMSError); update any code that constructed or matched Error::ZeroKMSBuilder to build or match ZeroKMSError instead so builder failures are contained within the ZeroKMSError type.packages/cipherstash-proxy/src/proxy/zerokms/mod.rs (1)
18-41:⚠️ Potential issue | 🟠 MajorDon't panic on malformed ZeroKMS config.
Line 18 narrows this path to
ZeroKMSBuilderError, and Line 30, Line 32, and Line 41 compensate by callingexpect().packages/cipherstash-proxy/src/config/tandem.rsstill storesclient_idandclient_keyas rawStrings, so bad operator config will abort startup instead of returning a typed config/ZeroKMS error. Please validate these fields during config loading or broaden this function's error type and replace theexpect()calls withmap_err(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs` around lines 18 - 41, The startup currently panics on malformed ZeroKMS config because client_id parse and ClientKey::from_hex_v1 and URL parse use expect(); change these to return proper errors instead: either validate and convert config.encrypt.client_id and config.encrypt.client_key when loading config (so they are typed UUID/ClientKey ahead of time) or broaden the error type of the builder function and replace the expect() calls on config.encrypt.client_id.parse(), ClientKey::from_hex_v1(...), and zerokms_host.parse() with map_err(...) that converts parsing/hex/URL errors into the function's Result (e.g. ZeroKMSBuilderError or a new variant), keeping ZeroKMSBuilder::new and AutoStrategy::builder usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cipherstash-proxy/src/error.rs`:
- Around line 55-58: Replace the top-level Error::ZeroKMSBuilder variant by
folding cipherstash_client::zerokms::ZeroKMSBuilderError into the ZeroKMSError
domain: add a From<ZeroKMSBuilderError> (or #[from]) conversion inside the
ZeroKMSError definition and remove the Error::ZeroKMSBuilder variant so the
public Error enum only exposes Error::ZeroKMS (which remains #[from]
ZeroKMSError); update any code that constructed or matched Error::ZeroKMSBuilder
to build or match ZeroKMSError instead so builder failures are contained within
the ZeroKMSError type.
In `@packages/cipherstash-proxy/src/proxy/zerokms/mod.rs`:
- Around line 18-41: The startup currently panics on malformed ZeroKMS config
because client_id parse and ClientKey::from_hex_v1 and URL parse use expect();
change these to return proper errors instead: either validate and convert
config.encrypt.client_id and config.encrypt.client_key when loading config (so
they are typed UUID/ClientKey ahead of time) or broaden the error type of the
builder function and replace the expect() calls on
config.encrypt.client_id.parse(), ClientKey::from_hex_v1(...), and
zerokms_host.parse() with map_err(...) that converts parsing/hex/URL errors into
the function's Result (e.g. ZeroKMSBuilderError or a new variant), keeping
ZeroKMSBuilder::new and AutoStrategy::builder usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a62f3288-e41d-4ebe-86c8-c64f3f74b724
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlpackages/cipherstash-proxy-integration/src/generate.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/proxy/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/mod.rspackages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
💤 Files with no reviewable changes (1)
- packages/cipherstash-proxy/src/proxy/mod.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cipherstash-proxy/src/proxy/zerokms/zerokms.rs
- packages/cipherstash-proxy/Cargo.toml
- packages/cipherstash-proxy-integration/src/generate.rs
32126c1 to
f9db5f1
Compare
Migrate ZeroKMS initialization from config-builder chain (ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new ZeroKMSBuilder + AutoStrategy API. - Replace AutoRefresh<ServiceCredentials> with AutoStrategy - Use ClientKey::from_hex_v1 for client key construction - Update error variant: zerokms::Error::Credentials -> Error::Auth - Add ZeroKMSBuilderError variant to Error enum - Remove build_zerokms_config (replaced by init_zerokms_client) - Update cts-common to 0.34.0-alpha.3 - Pin vitaminc to 0.1.0-pre4.1 (pre4.2 broke backward compat)
f9db5f1 to
a20d56a
Compare
Migrate ZeroKMS initialization from config-builder chain (ConsoleConfig/CtsConfig/ZeroKMSConfig/EnvSource) to the new ZeroKMSBuilder + AutoStrategy API.
Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.
Summary by CodeRabbit
Chores
Bug Fixes
Tests