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
 |
The spec in #1341 depends on two facts that had not been checked. The first is that the extension can read the engine's decrypted record frame with the public stack-encrypt API. The second is the sync contract: an interrupt waits for a GIL-released call, and a dropped Pending wipes its plaintext. Both facts hold. A Sink written outside the engine reads the frame. A probe function behind an off-by-default cargo feature is sent SIGINT during the call. The interrupt arrives after the call returns. The wipe is from source reading only. The issue's wording of the first fact is corrected: a sealed value's kind is inside its encryption, so the kind is read after decrypting. The findings, how to run each probe again, and what was not tested are in languages/python/docs/. Refs #1384 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) The codes follow the amendment on #1341. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eeab4bd to
909bd36
Compare
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 909bd36 · posted from Claude Code · claude-sonnet-5-5 · effort medium
Sources: code-review · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
Findings (3): 0 critical, 0 major, 3 minor — 3 inline, 0 below.
Verified against the PR diff and the head file content only; the engine source was not available in this session, so two further candidate findings (frame passthrough round trip, maturin feature merge) were not confirmed and are not posted.
Source: code-review · claude-sonnet-5-5
Generated by Claude Code
| probe = getattr(_engine, "_probe_detached_sleep", None) | ||
|
|
||
| pytestmark = [ | ||
| pytest.mark.skipif(probe is None, reason="built without the gate-probe feature"), |
There was a problem hiding this comment.
minor Both tests skip unless the wheel is built with the non-default gate-probe feature, so a normal mise run build plus pytest reports green without running them. A later change to the detach or signal behaviour is never caught, and the gate fact rests on one manual macOS run. Build with the feature in a CI job, or state that this is a one-off check.
Source: code-review · claude-sonnet-5-5
Generated by Claude Code
| # The signal was sent at 0.2 s. If the call had been cut short, the | ||
| # interrupt would arrive then. It must arrive after the call's full second. | ||
| assert raised_after_s >= CALL_MS / 1000 | ||
|
|
There was a problem hiding this comment.
minor This assertion cannot fail on the engine's behaviour. CPython runs Python signal handlers only on the main thread, between bytecodes, so a KeyboardInterrupt cannot be raised while the main thread is inside probe() whether or not the Rust side releases the GIL. raised_after_s >= 1.0 is guaranteed by the interpreter. Say so in the doc, or add a case that would distinguish (for example a call that checks signals itself), and note the real engine path adds network I/O.
Source: code-review · claude-sonnet-5-5
Generated by Claude Code
| deadline = time.monotonic() + 5 | ||
| while time.monotonic() < deadline: | ||
| pass |
There was a problem hiding this comment.
minor The while ...: pass loop pegs a core for up to 5 seconds, and with a 0.2s timer against a 1s call there is little margin on a loaded runner. If SIGINT is blocked or the timer thread is starved, the test waits the full 5s and fails with "DID NOT RAISE". Use time.sleep(0.01) inside the loop; it still gives Python its signal check.
Source: code-review · claude-sonnet-5-5
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
Change before merging: the frame reader probe must cover a typed null and a sequence, and the SIGINT timer in the Python test must be cancelled in a finally block. The other points can wait.
Other findings not posted as comments
languages/python/tests/test_gate_keyboard_interrupt.py:30: the tests assume SIGINT raisesKeyboardInterrupt. If the launcher ignores SIGINT, both tests fail withDID NOT RAISE. Installsignal.default_int_handlerin a fixture and restore it afterwards.languages/python/tests/test_gate_keyboard_interrupt.py:21: no CI job builds withgate-probe, so the tests always skip in CI. Add a CI step that builds with the feature and fails when the tests skip.languages/python/docs/gate-facts/keyboard-interrupt.md:12: part 2 of fact 2 (a droppedPendingwipes its plaintext) has no executable test. The doc already says so. Add a heap-hygiene test besidepackages/stack-encrypt/tests/heap_hygiene.rs.languages/python/crates/engine/src/lib.rs:32: the probe builds a new tokio runtime on each call and panics if the build fails. Optional: keep one runtime in aOnceLockand return aPyResult.languages/python/crates/core/tests/gate_frame_reader.rs:201: the malformed-frame test checks only truncation. Optional: add a case for trailing bytes and an empty buffer.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 1 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| claude | claude-opus-5-5 | python | 2 found, 0 posted |
| claude | claude-opus-5-5 | infracode | 0 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | python | 1 found, 1 posted |
| codex | gpt-5.6-terra | infracode | 0 found, 0 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 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 3 of 15 (#1405, #1406, *️⃣ #1414, #1415, #1433, #1437, #1453, #1499, #1509, #1510, #1511, #1513, #1516, #1517, #1582). *️⃣ marks this pull request.
Context loaded: the description, 2 linked issue(s) and 6 discussion entries.
| Ok(()) | ||
| } | ||
|
|
||
| fn typed_null(self, _: FixedType) -> Result<(), ReadError> { |
There was a problem hiding this comment.
Change before merge: typed_null drops the FixedType, and no test walks a typed null, a sequence or a nested map.
Impact: Fact 1 says the extension can find each field's type. A typed null field is recorded as logical: None, physical: "null", so its type is lost. The decrypt kind-mismatch check needs that type. The test record has only flat, non-null scalars, so no test fails if null or sequence frames read wrongly.
Evidence: User::visit writes only scalar and one scalar passthrough. The typed_null and seq methods of Reader run in no test.
Fix: Keep the type in typed_null, and add a record with a typed null and a sequence.
fn typed_null(self, ty: FixedType) -> Result<(), ReadError> {
self.scalar(ty.logical(), Physical::Null)
}Then assert that a null Date field reads back with logical type Date, and that each sequence element is read.
Found by 1 model: claude
| //! frame and recover each field's value kind, with only the public API of | ||
| //! `stack-encrypt`'s `dynamic` module. | ||
| //! | ||
| //! The record is built with `ValueFrame::of`, the same type and writer |
There was a problem hiding this comment.
Fix in a follow-up: the comment says ValueFrame::of uses the same writer as dynamic::decrypt, but decrypt writes its record with a plain FrameWriter.
Impact: Fact 1 is about the record that decrypt returns. This probe reads a frame written through the Modeled writer, so it does not test the bytes of a real decrypt result. It also does not test the batch shape, which is a sequence of maps.
Evidence: packages/stack-encrypt/src/dynamic/value_frame.rs:98 uses Modeled::new(writer). packages/stack-encrypt/src/dynamic/record.rs writes the decrypted row with FrameWriter::new and calls ValueFrame::from_written.
Fix: Correct the comment to say "the same type". Also build the test frame the way decrypt does, a map of per-field ValueFrame values and a sequence of such maps, and assert the reader gives the same result.
Found by 1 model: claude
|
|
||
|
|
||
| def test_interrupt_waits_for_the_detached_call_to_return(): | ||
| timer = send_sigint_later() |
There was a problem hiding this comment.
Change before merge: if the probe or an assertion raises an unexpected error, the SIGINT timer stays active.
Impact: The test skips timer.join(). The timer then sends SIGINT to the whole pytest process, which can interrupt a later, unrelated test.
Evidence: Both tests start the timer before code that can fail. They join it only after the expected KeyboardInterrupt.
Fix: Cancel and join the timer in finally in both tests.
timer = send_sigint_later()
try:
with pytest.raises(KeyboardInterrupt):
call_then_wait_for_the_interrupt()
finally:
timer.cancel()
timer.join()Found by 1 model: codex
Summary
The Python SDK spec (#1341) depends on two facts about the engine that nobody had checked. This PR checks both, with a probe for each, and records the results in
languages/python/docs/gate-facts/. Both facts hold. No SDK code changes, and the normal wheel build is unchanged.The engine is the Rust crate
stack-encrypt. The engine encrypts and decrypts. The extension is the Rust code compiled into the Python package. Python calls the extension, and the extension calls the engine.KeyboardInterrupt) during an engine call is raised only after the call finishes. Also, a request object (Pending) that is dropped early wipes the plaintext it contains.Changes
crates/core/tests/gate_frame_reader.rs: the probe for fact 1. A reader written outside the engine walks a record and records each field's type.crates/engine/src/lib.rsandcrates/engine/Cargo.toml: the probe for fact 2. A function that blocks with Python's global interpreter lock (GIL) released, behind a new cargo feature,gate-probe. The feature is off by default, so the function is not in a normal wheel.tests/test_gate_keyboard_interrupt.py: sends Ctrl-C during that call. The tests skip themselves when the feature is not built.docs/gate-facts/frame-reader.mdanddocs/gate-facts/keyboard-interrupt.md: the findings, how to run each probe again, and what was not tested.Cargo.lock: one line, fortokioas an optional dependency of the probe feature.Verification
All of this was run on macOS 14 (arm64) with Python 3.14.7:
cargo test --locked -p stackencrypt-core --test gate_frame_reader: 2 passed.--features gate-probe: 2 passed. Ctrl-C was sent 0.2 seconds into a 1-second call. The interrupt was raised after 1.01 seconds. As a control, the same signal interrupted a plain Pythontime.sleep(1.0)after 0.21 seconds.mise run test: 4 passed and 2 skipped. The 2 skipped tests are the probe tests, in a normal build.mise run test:cargoandcargo fmt --check: both pass.cargo clippy -D warnings, with and without the probe feature: passes.pnpm run test:scriptson Node 22: 83 of 83 test files pass.Not tested:
Related
Closes #1384. Refs #1341 (the spec). Stacked on #1406.
Review notes
Start with the two files in
docs/gate-facts/. Each one says what was checked, how, and what was not.🤖 Generated with Claude Code