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
|
Mutation testing (cargo-mutants,
|
7efb9db to
4ca5783
Compare
f3638e4 to
3220805
Compare
Review: enroll value-kind and query fixturesReviewed StandardsNo serious breach. No changeset or skill update is needed, because the PR changes only tests, docs and tooling. Against documented standards (minor):
Code smells (judgement calls):
SpecThe PR does what the plan step asked: "enroll the existing wider value-kind and operator fixtures under named requirements". The plan edit only moves the follow-on work into a "Next" line; it doesn't redefine the step. No assertion was removed or weakened in the Go or Rust tests. Claims stronger than the code:
Not mentioned in the PR description:
Possibly wrong:
Summary
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Source: automated review by Claude Code (/code-review medium, then company-skills:cross-check-review-findings) of head 4ca5783.
Result: no critical or blocking issues. No new findings.
Checked
fixture-profiles.jsonrequirement IDs, the documented table infixture-contract.md, and the named Rust/Go tests agree.scripts/stack-conformance.shexportsSTACK_CONFORMANCE_REQUIRED=1and rejects both update variables, including when set empty.- The Rust
fixture_updates_requested()guard runs before any fixture write in both enrolled writers. - The Go skip-to-fail change applies to both enrolled fixture clients.
- The enrolled Rust tests and the Go
-runselection match the index.
Cross-check: the concerns I could raise are already covered by the existing review comment (#issuecomment-6554303546), so I'm not repeating them:
- the untested Rust regeneration guard
- the duplicated Go skip condition
- the text-only test-existence check
- the
record_lowering.rsdirect env reads
Codex review was still running at the time of this review.
Generated by Claude Code
3220805 to
ea2be85
Compare
4ca5783 to
52d96d9
Compare
|
Addressed the review findings in 52d96d9:
Validation passed: all six profiles and fresh scalar Go-to-Rust exchange, the Rust fixture/record-lowering tests, Go missing-guest regressions, Rust clippy/formatting, Go lint, Biome, and 42 CI-wiring/script tests. I also verified the guards fail when the Rust update check is removed, an enrolled Rust test is cfg-disabled, or the index names a nonexistent Go test. Those temporary mutations were restored; frozen fixtures are unchanged. Rebased only this PR's two commits onto the updated #1462 head. It is mergeable again and remains stacked on #1462 while the earlier PRs are merged. |
Closes #1474.
The conformance command omitted the existing wider value-kind and query-operator fixtures, leaving their coverage outside the named contract. This enrolls them as
record-value-kinds-v1andrecord-query-operators-v1, using their original Rust and Go runners.An enrollment index maps stable requirement IDs to tests for typed frames, frozen-record opening, stored terms, decimal/timestamp normalization, query operands and metadata, match verification, and bounded/unbounded fetch limits. Frozen fixtures stay in their current paths with no copied expectations; typed Rust comparisons and generated Go methods remain intact.
The Go command selection is generated from the index and checked against the exact intended set. Rust checks the compiled libtest inventory for missing or ignored enrolled tests. Go's missing-guest regression checks every indexed test's actual skip/failure outcome, detecting tests omitted from compilation too. CI wiring also requires both Rust and Go workflows to include
conformance/stack-encrypt/**andpackages/stack-encrypt/**in their push and PR path filters.All conformance runners share the Go missing-guest policy: optional runs skip, required runs fail. Two full-suite query tests share that fixture client and therefore also fail in required mode; they are documented and covered by the regression without becoming shared requirements. The aggregate command and all four Rust fixture writers reject regeneration during required checks. A subprocess regression tests absent, empty and nonempty update flags without writing fixtures.
Stacked on #1462 (
feat/stack-encrypt-read-conformance), rebased onto its updated head. Merge parents first and retarget as they land. The roadmap and coverage map are updated; shared context-composition vectors are next.Validation:
mise run test:conformance, including fresh scalar Go-to-Rust exchange.This is representative fixture coverage, not a complete type/operator matrix. Shared query refusals, wider fresh artifact exchange, EQL/JSON profiles, and Python/TypeScript consumers remain follow-ups. Test/docs/tooling changes only; no production API or package behavior changes.