Repository navigation
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Add the fourth Cargo workspace at languages/python with two crates: stackencrypt-core (no pyo3, depends on stack-encrypt by path) and stackencrypt-engine (the `_engine` cdylib, pyo3 0.29, abi3-py39). The pure-Python package `stackencrypt` exposes `__version__` (0.1.0, from the engine crate's manifest through maturin) and `__engine__` (the checkout's commit, `-dirty` when packages/stack-encrypt has local edits). `extension-module` is set only by maturin, so `cargo test --locked` links libpython. Tasks live in languages/python/mise.toml: build, test, test:cargo. The Cargo.lock is seeded from the root one because stack-encrypt requires a yanked cllw-ore pre-release a fresh resolve cannot select. Exclude the workspace from the root Cargo.toml; add it to the Dependabot cargo entry and the publish opt-out and lock freshness guards; update AGENTS.md and SECURITY.md. Mark ADR-0001 accepted, add the Python row to ADR-0002's host-language table, and start the Python section of docs/sdk-design-principles.md, the doc edits the spec says ride with the first PR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… comments `-dirty` only watched packages/stack-encrypt, but the engine also takes stack-kms and stack-encrypt-derive by path, so an edit there shipped under a clean SHA. The check now covers every packages/stack-* crate. The comments listing core's future responsibilities now point at the spec instead, so they cannot drift into a second one. Refs #1382 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ADR-0001 is recorded as accepted, with its open items listed and its kind rule amended to the spec's. ADR-0002 cites the spec, not the ADR, for the value rules. The glossary records the `_engine` naming. The Dependabot guard now reads [workspace.dependencies], which hid the Python edge. The dirty check skips fuzz corpora. The unasked release profile and rlib are gone. Refs #1382 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
30e4646 to
a0ae3fc
Compare
Mutation testing (cargo-mutants,
|
The lockfile added with the walking skeleton pinned vitaminc 0.6.1, but the stack-encrypt it builds by path already required 0.8 (8f9c9fb), so any `cargo --locked` in this workspace refused to run. Regenerated with the smallest change cargo allows: the vitaminc family moves to 0.8.1, ore-rs to 0.10, and clww-ore and base85-simd are added, matching the root workspace's move. Refs #1382 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 98be072 · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review (inline read of the diff) · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
No findings.
Scope note: the diff was read in sections. The generated languages/python/Cargo.lock (lines ~228–3828 of the diff), the ADR/docs changes and the SECURITY.md/AGENTS.md edits were not read in full. The build.rs -dirty pathspec (absolute paths containing .., plus :(exclude)) was checked with git status against an existing directory and behaves correctly. Existing PR feedback consisted only of bot comments (changeset-bot, CodeRabbit skipped, cargo-mutants); nothing to cross-check.
Generated by Claude Code
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Before merging, add a test for the -dirty and engine-commit logic in build.rs (finding below). The other findings can wait for a follow-up.
Other findings not posted as comments
- Optional (
languages/python/tests/test_smoke.py:17): the test compares__version__to the literal"0.1.0", so the first version bump fails it. Check the version form instead. - Optional (
languages/python/crates/core/build.rs:77):rerun-if-changedwatches each wholepackages/stack-*directory, includingfuzz/output. Fuzz runs trigger rebuilds that cannot change the-dirtyresult. Watchsrc/andCargo.tomlinstead.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| claude | claude-opus-5-5 | python | 1 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | python | 0 found, 0 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: the reader test did not run; only the word-and-shape check was applied.
Stack: position 1 of 15 (:asterisk: #1405, #1406, #1414, #1415, #1433, #1437, #1453, #1499, #1509, #1510, #1511, #1513, #1516, #1517, #1582). *️⃣ marks this pull request.
Context loaded: the description, 3 linked issue(s) and 4 discussion entries.
| ); | ||
| let mut status = vec!["status", "--porcelain", "--"]; | ||
| status.extend(paths.iter().map(String::as_str)); | ||
| let dirty = git(&status).is_some_and(|s| !s.is_empty()); |
There was a problem hiding this comment.
Fix in a follow-up: when git status fails, the build records a clean commit with no error or log message.
Impact: The git closure returns None for a non-zero exit. is_some_and turns None into "not dirty". git rev-parse HEAD can succeed while git status fails, for example with a locked or corrupt index. The wheel then reports a clean commit for a tree that may have edits. The module comment says an engine commit nobody can act on must fail the build.
Evidence: let dirty = git(&status).is_some_and(|s| !s.is_empty()); at this line. The rev-parse call at line 81 does panic on failure.
Fix: Panic when git status fails, as rev-parse does:
let porcelain = git(&status)
.unwrap_or_else(|| panic!("`git status` failed: cannot tell if the engine tree is clean"));
let dirty = !porcelain.is_empty();Found by 1 model: claude
| ); | ||
| let mut status = vec!["status", "--porcelain", "--"]; | ||
| status.extend(paths.iter().map(String::as_str)); | ||
| let dirty = git(&status).is_some_and(|s| !s.is_empty()); |
There was a problem hiding this comment.
Change before merge: no test checks the -dirty branch or that __engine__ equals the checkout commit.
Impact: The Rust test and the pytest test check only the format (40 hex characters, optional -dirty). If the git status pathspec or the :(exclude) form stops matching, the wheel reports -dirty too often or never. No test fails, and a bug report then names the wrong engine source.
Evidence: languages/python/tests/test_smoke.py uses the pattern [0-9a-f]{40}(-dirty)?. build.rs does not change in later pull requests of the stack.
Fix: Add a test in the core crate that asks git the same question and compares. Skip it when STACKENCRYPT_ENGINE_COMMIT is set. Expect the SHA to equal git rev-parse HEAD, and expect -dirty exactly when git status --porcelain for the non-fuzz packages/stack-* paths is not empty.
Found by 2 models: claude, codex
| fn main() { | ||
| println!("cargo:rerun-if-env-changed={ENV_OVERRIDE}"); | ||
| let commit = match env::var(ENV_OVERRIDE) { | ||
| Ok(value) if !value.is_empty() => value.to_ascii_lowercase(), |
There was a problem hiding this comment.
Optional: the STACKENCRYPT_ENGINE_COMMIT override path has no test.
Impact: The override has three rules: an empty value falls back to git, the value is changed to lower case, and a value that is not a 40-character hex SHA stops the build. CI wheel builds in a later pull request use only this path. A broken rule could ship a wheel with a wrong value, and no test would fail.
Fix: Move the rules into a small pure function in its own file. Include it from build.rs with mod, and from a test with #[path]. Test lower-casing, -dirty acceptance, and refusal of short or non-hex values.
Found by 1 model: claude
| //! was not built from. The scope is every stack-* crate, not only | ||
| //! `stack-encrypt`: it depends by path on `stack-kms` and | ||
| //! `stack-encrypt-derive`, and a wider scope can only add a `-dirty` that is | ||
| //! true. Edits outside `packages/stack-*` (this workspace, the Python |
There was a problem hiding this comment.
Fix in a follow-up: -dirty does not cover the root Cargo.toml or languages/python/Cargo.lock, which choose the engine's dependency versions.
Impact: stack-encrypt takes clww-ore, ore-rs and vitaminc-* with workspace = true, so versions come from the root Cargo.toml and the exact ones from languages/python/Cargo.lock. An uncommitted edit to either file changes the engine. The wheel still reports a clean commit. A bug report then points at the wrong dependency graph.
Evidence: The pathspec built at lines 91-102 names only the packages/stack-* directories. The module comment says only the stack-* crates count as engine source.
Fix: Add the root Cargo.toml and languages/python/Cargo.lock to the git status pathspec and to the rerun-if-changed list. Update the module comment.
Found by 1 model: claude
Summary
The first piece of the Python SDK,
stackencrypt. It installs into avirtualenv and exposes two facts: its version (0.1.0) and the commit of
the Rust encryption engine it was built from. It encrypts nothing yet.
The point is the build path: a Rust extension built by maturin, over
the
stack-encryptcrate in this repository, importable from Python.Changes
languages/python: a fourth Cargo workspace with two crates.corehas no Python dependency, so its tests run without an interpreter.
engineis the pyo3 extension module. A pure-Python package wraps it.languages/python/mise.toml:build,test,test:cargo.Cargo.toml, Dependabot, and two repo guards know the new workspace.The Dependabot guard now also reads path dependencies declared at a
workspace root, which it missed before.
in ADR-0002, and a first Python section in the SDK design principles.
Verification
Run locally on macOS, Python 3.14:
mise run test(four pytest tests) andmise run test:cargo(two Rust tests) inlanguages/python.cargo metadata --lockedpasses in all four workspaces. The repo guardsuite and the supply-chain e2e test pass.
Not verified: Python 3.9, and any CI run. Both are #1383.
Related
Closes #1382. Refs #1341 (the spec). Stacked on #1284; retarget to
nextafter that merges.
Review notes
No Python lockfile yet, and no CI workflow: both are deferred to #1383.
The engine commit ends in
-dirtywhen apackages/stack-*crate hasuncommitted changes.
🤖 Generated with Claude Code