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)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates workspace dependencies ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
tobyhede
approved these changes
Apr 7, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Cargo.toml (2)
46-48:⚠️ Potential issue | 🔴 CriticalBlocker: shared manifest is pinned to local sibling paths (CI/non-local builds will fail).
Line 46-Line 48 and Line 60-Line 64 require
../cipherstash-suite/..., which is not portable and has already caused CI breakage. Keep published versions in the sharedCargo.toml, and move local overrides to developer-local Cargo config.Suggested manifest direction
[workspace.dependencies] sqltk = { version = "0.10.0" } -cipherstash-client = { path = "../cipherstash-suite/packages/cipherstash-client" } -cipherstash-config = { path = "../cipherstash-suite/packages/cipherstash-config" } -cts-common = { path = "../cipherstash-suite/packages/cts-common" } +cipherstash-client = "0.34.0-alpha.5" +cipherstash-config = "0.2.3" +cts-common = "0.34.0-alpha.5" -[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" }#!/bin/bash set -euo pipefail echo "=== Cargo.toml dependency section ===" sed -n '44,70p' Cargo.toml echo echo "=== Check whether local sibling paths exist in this checkout ===" 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 "FOUND: $p" else echo "MISSING: $p" fi doneAlso applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 46 - 48, The shared Cargo.toml currently pins local sibling paths for crates cipherstash-client, cipherstash-config, cts-common (and similarly zerokms-protocol), which breaks CI/non-local builds; change the dependency entries in Cargo.toml to reference the published crate versions (remove the path = "../..." overrides) and move any developer-only local path overrides into a developer-local cargo config (e.g., .cargo/config.toml) using [patch.crates-io] or [replace] so local development can still override those crates without contaminating the shared manifest.
60-63:⚠️ Potential issue | 🟠 MajorGlobal
[patch.crates-io]masks real release compatibility testing.The
cipherstash-configoverride on line 62 redirects allcipherstash-configdependencies to the local path, including the explicitcipherstash-config = "0.2.3"declaration inpackages/cipherstash-proxy-integration/Cargo.toml. This prevents validating that the integration crate actually works with the published 0.2.3 version and can hide compatibility breakage before release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 60 - 63, The patch entry is overriding cipherstash-config globally which prevents testing against the published 0.2.3 release; remove the cipherstash-config = { path = "../cipherstash-suite/packages/cipherstash-config" } line from the [patch.crates-io] section in Cargo.toml so the integration crate's explicit dependency (cipherstash-config = "0.2.3" in packages/cipherstash-proxy-integration/Cargo.toml) will resolve to the published crate; if you still need a local override for development, limit the patch to only the specific workspace crates that must use local sources (e.g., keep cipherstash-client and cts-common) or use a temporary, targeted override mechanism instead of globally patching cipherstash-config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Cargo.toml`:
- Around line 46-48: The shared Cargo.toml currently pins local sibling paths
for crates cipherstash-client, cipherstash-config, cts-common (and similarly
zerokms-protocol), which breaks CI/non-local builds; change the dependency
entries in Cargo.toml to reference the published crate versions (remove the path
= "../..." overrides) and move any developer-only local path overrides into a
developer-local cargo config (e.g., .cargo/config.toml) using [patch.crates-io]
or [replace] so local development can still override those crates without
contaminating the shared manifest.
- Around line 60-63: The patch entry is overriding cipherstash-config globally
which prevents testing against the published 0.2.3 release; remove the
cipherstash-config = { path = "../cipherstash-suite/packages/cipherstash-config"
} line from the [patch.crates-io] section in Cargo.toml so the integration
crate's explicit dependency (cipherstash-config = "0.2.3" in
packages/cipherstash-proxy-integration/Cargo.toml) will resolve to the published
crate; if you still need a local override for development, limit the patch to
only the specific workspace crates that must use local sources (e.g., keep
cipherstash-client and cts-common) or use a temporary, targeted override
mechanism instead of globally patching cipherstash-config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f014b88-8d9c-4de2-98f6-ae8fc35650b5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoreCargo.tomlmise.local.example.tomlmise.tomlpackages/cipherstash-proxy/src/proxy/encrypt_config/config.rs
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- mise.local.example.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cipherstash-proxy/src/proxy/encrypt_config/config.rs
tobyhede
approved these changes
Apr 15, 2026
freshtonic
force-pushed
the
refactor/canonical-config
branch
from
a088022 to
c21471e
Compare
Move the test-only config.rs module into manager.rs alongside the code under test, extract a shared canonical_to_map helper used by both load_encrypt_config and the tests, and add coverage for the `real` cast_as alias that maps to ColumnType::Float. Addresses PR #386 review feedback.
freshtonic
force-pushed
the
refactor/canonical-config
branch
from
d42c5d2 to
63b9aba
Compare
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.
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.
Summary by CodeRabbit
Documentation
Chores
cipherstash-clientandcts-commonto 0.34.1-alpha.3.cipherstash-configas a workspace dependency.Refactor