Open
Conversation
Add implementation plan for migrating proxy to use CanonicalEncryptionConfig from the cipherstash-config crate.
…CanonicalEncryptionConfig Migrate from the proxy-local encryption config types to the canonical types provided by the cipherstash-config crate. This removes ~200 lines of duplicated type definitions (CastAs, Column, Indexes, etc.) and consolidates config parsing into the shared crate. - Bump cipherstash-client/cts-common to 0.34.0-alpha.5 - Add cipherstash-config workspace dependency - Add InvalidEncryptionConfig error variant for config crate errors - Update manager to use CanonicalEncryptionConfig with Identifier conversion - Rename Utf8Str -> Text and JsonB -> Json to match canonical types - Add [patch.crates-io] for local development against cipherstash-suite NOTE: The [patch.crates-io] section and version bumps to 0.34.0-alpha.5 must be updated when the canonical types are published to crates.io.
…g migration Covers proxy-side integration tests for JSON -> CanonicalEncryptionConfig -> EncryptConfig pipeline including Identifier bridging, ColumnType mapping, error propagation, and a realistic schema fixture matching the integration test database.
…igration Add 9 unit tests covering ColumnType-to-Postgres type mapping and CanonicalEncryptionConfig parsing to establish a safety net before further refactoring: - column.rs: 4 tests for column_type_to_postgres_type (text, json, json accessor, exhaustive variant coverage) - config.rs: 5 tests for config map construction (identifier bridging, multi-table configs, invalid index validation, realistic EQL schema fixture, malformed JSON error handling)
📝 WalkthroughWalkthroughThe PR migrates cipherstash-proxy's encryption configuration system from proxy-local types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs (1)
352-355: Assert the error shape, not onlyis_err().These tests currently pass for any error. Tighten assertions to validate the intended failure mode (validation vs. deserialization-shape), so regressions don’t slip through.
Suggested tightening
let config: CanonicalEncryptionConfig = serde_json::from_value(json).unwrap(); -let result = config.into_config_map(); -assert!(result.is_err(), "ste_vec on text column should fail validation"); +let err = config.into_config_map().unwrap_err(); +assert!( + err.to_string().contains("ste_vec"), + "expected ste_vec validation failure, got: {err}" +); let result = serde_json::from_value::<CanonicalEncryptionConfig>(json); -assert!(result.is_err()); +let err = result.unwrap_err(); +assert!( + err.to_string().contains("tables"), + "expected tables shape error, got: {err}" +);Also applies to: 440-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs` around lines 352 - 355, The test currently only checks that CanonicalEncryptionConfig::into_config_map() returns Err, which is too loose; change the assertion to inspect the error shape and message so it specifically asserts the validation failure (not a serde/deserialization error). After calling config.into_config_map(), call unwrap_err() and assert it is the expected validation variant (e.g., matches EncryptionConfigError::Validation(_) or contains the expected validation message like "ste_vec on text column"), and apply the same tighter assertion pattern to the other test occurrence that calls CanonicalEncryptionConfig::into_config_map() later in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 60-64: The shared Cargo manifest currently contains a
[patch.crates-io] section referencing local paths for cipherstash-client,
cipherstash-config, cts-common, and zerokms-protocol which break CI; remove that
entire [patch.crates-io] block from Cargo.toml (the entries for
cipherstash-client, cipherstash-config, cts-common, zerokms-protocol) before
merging, and instead move those local path overrides into a non-shared local
config (e.g., Cargo.local or a developer-only manifest) so CI uses the published
crates.
In `@docs/plans/2026-04-01-migrate-to-canonical-encryption-config.md`:
- Line 17: The heading "Task 1: Add `cipherstash-config` dependency" is
currently H3 causing a level jump from the H1 at the top; change this heading
from "### Task 1: Add `cipherstash-config` dependency" to an H2 ("## Task 1: Add
`cipherstash-config` dependency") or insert an appropriate intermediate H2 above
it so the document has a proper H1 -> H2 -> H3 hierarchy and satisfies
markdownlint MD001.
- Line 13: The plan file references a local filesystem path that only works on
the author's machine; update the pointer in
docs/plans/2026-04-01-migrate-to-canonical-encryption-config.md so the Design
doc link points to a repo-relative path or a stable permalink within the
cipherstash-suite repository (e.g., reference
docs/plans/2026-04-01-canonical-encryption-config-design.md or the repo URL/PR
permalink) instead of `~/cipherstash/...` so all readers can resolve the design
doc.
In `@docs/plans/2026-04-01-proxy-backwards-compat-tests.md`:
- Line 11: The task headings in the document (e.g., "Task 1: Test Identifier
bridging in EncryptConfig") use level-3 headings (###) which violates
markdownlint MD001; change each task heading to level-2 (##) so tasks use
consistent heading level; update the occurrences listed (including the headings
at the shown fragment and the other task headings referenced on lines 68, 134,
178, and 266) by replacing the leading "###" with "##" for those task titles.
In `@packages/cipherstash-proxy/src/error.rs`:
- Around line 188-189: The InvalidEncryptionConfig error variant currently lacks
a remediation/documentation link; update the #[error("Invalid encryption
configuration: {0}")] message for InvalidEncryptionConfig(#[from]
cipherstash_config::errors::ConfigError) to include the same friendly
remediation/docs URL used by the other ConfigError variants (or call the same
helper/formatter they use) so operators receive a pointer to the fix path when
this canonical config validation error occurs.
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs`:
- Around line 306-307: The test currently calls
config.get(&ident).expect("column exists") which uses a generic expect message;
replace this with config.get(&ident).unwrap() (or, alternatively, provide an
identifier-specific message like expect(&format!("column {:?} exists", ident)))
so the test follows the guideline to use unwrap() in tests; update the use site
where the local variable column is assigned (variable name: column, lookup:
config.get(&ident)) accordingly.
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs`:
- Around line 215-218: The serde_json deserialization of json_value into
CanonicalEncryptionConfig is currently using the bare `?` which routes serde
errors into the general Error::Encrypt; instead map deserialization failures
into the config domain by calling serde_json::from_value(json_value).map_err(|e|
crate::error::ConfigError::Parse(e).into()) (or equivalent) before unwrapping,
so that CanonicalEncryptionConfig::from_value errors are converted to
ConfigError::Parse; keep the subsequent call to
canonical.into_config_map().map_err(crate::error::ConfigError::from)? as-is.
---
Nitpick comments:
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs`:
- Around line 352-355: The test currently only checks that
CanonicalEncryptionConfig::into_config_map() returns Err, which is too loose;
change the assertion to inspect the error shape and message so it specifically
asserts the validation failure (not a serde/deserialization error). After
calling config.into_config_map(), call unwrap_err() and assert it is the
expected validation variant (e.g., matches EncryptionConfigError::Validation(_)
or contains the expected validation message like "ste_vec on text column"), and
apply the same tighter assertion pattern to the other test occurrence that calls
CanonicalEncryptionConfig::into_config_map() later in the file.
🪄 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
Run ID: 723dbdb6-f3ff-4278-8bff-343dd233160e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomldocs/plans/2026-04-01-migrate-to-canonical-encryption-config.mddocs/plans/2026-04-01-proxy-backwards-compat-tests.mdpackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/postgresql/context/column.rspackages/cipherstash-proxy/src/postgresql/data/from_sql.rspackages/cipherstash-proxy/src/postgresql/data/to_sql.rspackages/cipherstash-proxy/src/proxy/encrypt_config/config.rspackages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
Comment on lines
+60
to
+64
| [patch.crates-io] | ||
| cipherstash-client = { path = "../cipherstash-suite/packages/cipherstash-client" } | ||
| cipherstash-config = { path = "../cipherstash-suite/packages/cipherstash-config" } | ||
| cts-common = { path = "../cipherstash-suite/packages/cts-common" } | ||
| zerokms-protocol = { path = "../cipherstash-suite/packages/zerokms-protocol" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '44,64p' Cargo.toml
printf '\n---\n'
for p in \
"../cipherstash-suite/packages/cipherstash-client" \
"../cipherstash-suite/packages/cipherstash-config" \
"../cipherstash-suite/packages/cts-common" \
"../cipherstash-suite/packages/zerokms-protocol"
do
if [ -e "$p" ]; then
echo "$p exists"
else
echo "$p MISSING"
fi
doneRepository: cipherstash/proxy
Length of output: 1784
Remove the [patch.crates-io] entries before merge.
These patches reference ../cipherstash-suite/packages/*, which do not exist in the CI environment and are causing the current GitHub Actions failure. Move these patches to a local Cargo.local configuration or remove them from the shared manifest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 60 - 64, The shared Cargo manifest currently
contains a [patch.crates-io] section referencing local paths for
cipherstash-client, cipherstash-config, cts-common, and zerokms-protocol which
break CI; remove that entire [patch.crates-io] block from Cargo.toml (the
entries for cipherstash-client, cipherstash-config, cts-common,
zerokms-protocol) before merging, and instead move those local path overrides
into a non-shared local config (e.g., Cargo.local or a developer-only manifest)
so CI uses the published crates.
|
|
||
| **Prerequisite:** The canonical config work in `cipherstash-suite` (CIP-2871) must be completed first — specifically, `CanonicalEncryptionConfig`, `PlaintextType`, `Identifier`, and `into_config_map()` must exist in `cipherstash-config`. | ||
|
|
||
| **Design doc:** `~/cipherstash/cipherstash-suite/docs/plans/2026-04-01-canonical-encryption-config-design.md` |
There was a problem hiding this comment.
Use a shareable design-doc reference here.
~/cipherstash/... only resolves on the author's machine, so this pointer is broken for everyone else reading the plan. Prefer a repo-relative reference or a stable permalink into cipherstash-suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-01-migrate-to-canonical-encryption-config.md` at line 13,
The plan file references a local filesystem path that only works on the author's
machine; update the pointer in
docs/plans/2026-04-01-migrate-to-canonical-encryption-config.md so the Design
doc link points to a repo-relative path or a stable permalink within the
cipherstash-suite repository (e.g., reference
docs/plans/2026-04-01-canonical-encryption-config-design.md or the repo URL/PR
permalink) instead of `~/cipherstash/...` so all readers can resolve the design
doc.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Add `cipherstash-config` dependency |
There was a problem hiding this comment.
Fix the heading level jump.
The document goes from an H1 on Line 1 straight to an H3 here, which is why markdownlint is raising MD001. Make the task sections ## or add an intermediate H2.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 17-17: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-01-migrate-to-canonical-encryption-config.md` at line 17,
The heading "Task 1: Add `cipherstash-config` dependency" is currently H3
causing a level jump from the H1 at the top; change this heading from "### Task
1: Add `cipherstash-config` dependency" to an H2 ("## Task 1: Add
`cipherstash-config` dependency") or insert an appropriate intermediate H2 above
it so the document has a proper H1 -> H2 -> H3 hierarchy and satisfies
markdownlint MD001.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Test Identifier bridging in EncryptConfig |
There was a problem hiding this comment.
Fix heading level increments to satisfy markdownlint (MD001).
These task headings jump from # to ###. Use ## for each task heading to avoid lint failures.
Suggested patch
-### Task 1: Test Identifier bridging in EncryptConfig
+## Task 1: Test Identifier bridging in EncryptConfig
...
-### Task 2: Test ColumnType mapping through column_type_to_postgres_type
+## Task 2: Test ColumnType mapping through column_type_to_postgres_type
...
-### Task 3: Test error propagation for invalid configs
+## Task 3: Test error propagation for invalid configs
...
-### Task 4: Test real integration schema config shape
+## Task 4: Test real integration schema config shape
...
-### Task 5: Full verification
+## Task 5: Full verificationAlso applies to: 68-68, 134-134, 178-178, 266-266
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 11-11: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-01-proxy-backwards-compat-tests.md` at line 11, The task
headings in the document (e.g., "Task 1: Test Identifier bridging in
EncryptConfig") use level-3 headings (###) which violates markdownlint MD001;
change each task heading to level-2 (##) so tasks use consistent heading level;
update the occurrences listed (including the headings at the shown fragment and
the other task headings referenced on lines 68, 134, 178, and 266) by replacing
the leading "###" with "##" for those task titles.
Comment on lines
+188
to
+189
| #[error("Invalid encryption configuration: {0}")] | ||
| InvalidEncryptionConfig(#[from] cipherstash_config::errors::ConfigError), |
There was a problem hiding this comment.
Add the remediation link to this new config error.
This is now the main variant for canonical config validation failures, but unlike the surrounding ConfigError cases it gives operators no pointer to the fix path.
📎 Suggested tweak
- #[error("Invalid encryption configuration: {0}")]
+ #[error(
+ "Invalid encryption configuration: {0}. For help visit {}",
+ ERROR_DOC_CONFIG_URL
+ )]
InvalidEncryptionConfig(#[from] cipherstash_config::errors::ConfigError),As per coding guidelines "Customer-facing errors should include friendly messages and documentation links".
📝 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
| #[error("Invalid encryption configuration: {0}")] | |
| InvalidEncryptionConfig(#[from] cipherstash_config::errors::ConfigError), | |
| #[error( | |
| "Invalid encryption configuration: {0}. For help visit {}", | |
| ERROR_DOC_CONFIG_URL | |
| )] | |
| InvalidEncryptionConfig(#[from] cipherstash_config::errors::ConfigError), |
🤖 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 188 - 189, The
InvalidEncryptionConfig error variant currently lacks a
remediation/documentation link; update the #[error("Invalid encryption
configuration: {0}")] message for InvalidEncryptionConfig(#[from]
cipherstash_config::errors::ConfigError) to include the same friendly
remediation/docs URL used by the other ConfigError variants (or call the same
helper/formatter they use) so operators receive a pointer to the fix path when
this canonical config validation error occurs.
Comment on lines
+306
to
+307
| let column = config.get(&ident).expect("column exists"); | ||
| assert_eq!(column.name, "email_address"); |
There was a problem hiding this comment.
Prefer unwrap() over a generic expect() in this test lookup.
expect("column exists") adds little context; use unwrap() here (or make the message identifier-specific).
As per coding guidelines: "**/*.rs: Use unwrap() instead of expect() unless providing meaningful context in test code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs` around lines
306 - 307, The test currently calls config.get(&ident).expect("column exists")
which uses a generic expect message; replace this with
config.get(&ident).unwrap() (or, alternatively, provide an identifier-specific
message like expect(&format!("column {:?} exists", ident))) so the test follows
the guideline to use unwrap() in tests; update the use site where the local
variable column is assigned (variable name: column, lookup: config.get(&ident))
accordingly.
Comment on lines
+215
to
+218
| let canonical: CanonicalEncryptionConfig = serde_json::from_value(json_value)?; | ||
| let config_map = canonical | ||
| .into_config_map() | ||
| .map_err(crate::error::ConfigError::from)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '202,223p' packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
printf '\n---\n'
sed -n '115,190p' packages/cipherstash-proxy/src/error.rs
printf '\n---\n'
sed -n '478,482p' packages/cipherstash-proxy/src/error.rsRepository: cipherstash/proxy
Length of output: 3528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check imports at top of manager.rs
head -30 packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs
printf '\n--- ConfigError conversions ---\n'
# Search for impl From<ConfigError> for Error
rg -A 3 'impl From<ConfigError> for Error' packages/cipherstash-proxy/src/error.rs
printf '\n--- Check if ConfigError is directly imported or via crate::error ---\n'
# Check how ConfigError is used elsewhere in manager.rs
rg 'ConfigError' packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs | head -20Repository: cipherstash/proxy
Length of output: 921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full error.rs structure to see if ConfigError can convert to Error
rg -B 2 -A 5 'ConfigError' packages/cipherstash-proxy/src/error.rs | head -80
printf '\n--- Check Error enum structure ---\n'
# Find the Error enum definition
sed -n '1,50p' packages/cipherstash-proxy/src/error.rs
printf '\n--- Check if Error has ConfigError variant ---\n'
rg 'enum Error' -A 20 packages/cipherstash-proxy/src/error.rs | head -50Repository: cipherstash/proxy
Length of output: 3392
Route JSON deserialization failures through ConfigError::Parse.
Line 215's bare ? on serde_json::from_value(json_value)? uses the direct impl From<serde_json::Error> for Error, placing deserialization failures in Error::Encrypt instead of the config domain. This violates the error grouping guideline: errors must be grouped by problem domain and routed through packages/cipherstash-proxy/src/error.rs.
Add explicit error mapping to route through ConfigError::Parse:
Fix
- let canonical: CanonicalEncryptionConfig = serde_json::from_value(json_value)?;
+ let canonical: CanonicalEncryptionConfig =
+ serde_json::from_value(json_value).map_err(ConfigError::from)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs` around lines
215 - 218, The serde_json deserialization of json_value into
CanonicalEncryptionConfig is currently using the bare `?` which routes serde
errors into the general Error::Encrypt; instead map deserialization failures
into the config domain by calling serde_json::from_value(json_value).map_err(|e|
crate::error::ConfigError::Parse(e).into()) (or equivalent) before unwrapping,
so that CanonicalEncryptionConfig::from_value errors are converted to
ConfigError::Parse; keep the subsequent call to
canonical.into_config_map().map_err(crate::error::ConfigError::from)? as-is.
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
1 participant
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.
refactor(proxy): replace local ColumnEncryptionConfig with canonical CanonicalEncryptionConfig
Migrate from the proxy-local encryption config types to the canonical types provided by the cipherstash-config crate. This removes ~200 lines of duplicated type definitions (CastAs, Column, Indexes, etc.) and consolidates config parsing into the shared crate.
NOTE: The [patch.crates-io] section and version bumps to 0.34.0-alpha.5 must be updated when the canonical types are published to crates.io.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores