Repository navigation
Conversation
Require named Rust, Go and PostgreSQL evidence, share document and predicate refusal cases, and report engine and SQL bundle identities. Closes #1476
|
|
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
 |
coderdan
added this to the
Stable Stack Encrypt Rust release milestone
ChatGPT Codex Connector
Mutation testing (cargo-mutants,
|
Review: enroll EQL and JSON conformanceReviewed StandardsNo hard violations. All three copies of the EQL path filter gained the same A defect tooling won't catch:
Judgement calls against documented rules:
Code smells (judgement calls):
SpecVerified:
(a) Missing or partial:
(b) Scope creep:
(c) Implemented but worth fixing:
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
cipherstash-agent
left a comment
There was a problem hiding this comment.
Automated review attestation: 7f36ec0
Source: Claude Code scheduled routine (/code-review medium plus cross-check-review-findings). It read the full diff against base feat/stack-encrypt-context-conformance and checked each finding against the head commit.
Critical / blocking: none.
New findings: none. Every defect I confirmed at head is already in the existing review, and I have nothing material to add to those items. Confirmed at head:
- The doc comment for
is_refused_with_its_reason_and_offsetnow sits onshared_predicate_refusals(packages/stack-encrypt/tests/json_operators.rs:2186-2190). - The report's
'schema', 4is hard-coded, not read from the database (scripts/stack-eql-conformance.mjs:214). - The
eql-postgresjob runs nextest throughmise x --env testwithoutMISE_ENV: test(.github/workflows/tests-golang.yml).
Checked, no problem found:
- Both deterministic guests are built before the new fast step (
tests-golang.ymllines 202-215, step at 266). checkGoResultsfails on skip or fail and requires every selected test to report a pass, so an empty selection cannot pass.checkRustInventorychecks ignored status per lane and--run-ignoredis set per lane.- Fixture-update variables are refused, and the report upload uses
if-no-files-found: error.
Not run: the PostgreSQL lane, and the Codex code and security reviews were still running when this was written.
Generated by Claude Code
|
Addressed the standards/spec review and second-agent confirmation in e6ce5d0.
I retained the small independent Rust corpus loaders and per-test Go reads: the consumers belong to different Cargo workspaces, and sharing their test-only JSON parsing would add coupling without consolidating any semantic expectations. The corpus itself remains shared; no cache or new dependency is needed for this small fixture. Validation: required native/Go fast lane; unified PostgreSQL task (32 native JSON tests, four Go JSON tests and three remaining Go database suites); Rust clippy, Go lint, formatting, Biome and CI guards. The database report records actual schema |
This comment has been minimized.
This comment has been minimized.
|
Follow-up CI fixes are pushed in 64f5163. The EQL mise setup now installs its tools without exporting its local DATABASE_URL into the hermetic Go job. The EQL cache guard also accepts a multi-workspace cache entry that includes the nested EQL workspace. All 78 relevant runner/CI regression tests and formatting checks pass locally; CI is rerunning. Both fixes have been carried into the stacked lifecycle PR #1493. |
|
Fixed the shared CI bootstrap failure in 7a3dec9 (also merged into #1493). Every failed job stopped before tests with Verified with CI's exact mise 2026.4.0 in isolated tool/Cargo directories: the original configuration reproduces the failure; adding the pin installs and runs cargo-nextest 0.9.146 successfully. All 47 relevant CI/tooling regression tests and formatting checks pass. Fresh CI runs have been triggered on both PRs. |
|
Fixed in 65d4937 and carried into the stacked PR #1493. The CRAP failures were executable compatibility failures after coverage tests passed: cargo-crap 0.6.1's downloaded Linux binary requires GLIBC 2.39. Both CRAP jobs now run on Ubuntu 24.04 with matching mise cache keys, and check Also fixed the PostgreSQL job's cached Rust toolchain failure: it now reconciles both configured Rust toolchains and verifies cargo before restoring the build cache. Verified in GitHub Actions on both PRs: CRAP (stack-auth), CRAP (stack-encrypt), the full WASI/Go job, and Go rows/queries plus JSON conformance in PostgreSQL all passed. Locally, all 116 relevant CI regression tests and the 10 child conformance-runner tests passed. |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approved. The shared corpus is written by hand and not generated from the code it tests, and no case was dropped. CI runs both lanes: the reports show 41 Rust and 21 Go tests in the fast lane, and 18 Rust and 4 Go tests in the PostgreSQL lane. If one corpus entry is changed, shared_predicate_refusals fails as it must.
The inline comments carry suggestions that you can commit with one click. The two items below are outside the diff, so I cannot attach suggestions to them.
1. Should-fix: the PR description
The description says the PR is test-only, but it does not mention three CI and toolchain changes. Please add a line for each:
mise.test.tomlpinscargo-binstall1.26.2 in everyMISE_ENV=testjob. It is a new tool binary in every test job, so it is a supply-chain decision.crap-crates.ymlmoves both CRAP jobs from ubuntu-2204 to ubuntu-2404, because cargo-crap 0.6.1 needs GLIBC 2.39.tests-golang.ymladds amise install --force rustrepair step.
Also: CI ran PostgreSQL 17.11, not 17.10. The "95 native / 32 PG native" counts are totals for the whole binary; the corpus names 41 and 18 of those tests.
2. Should-fix: the docs for go:test:postgres
go:test:postgres now starts with test:conformance:eql:postgres. That task runs mise -C packages/eql run clean and build, installs the bundle with psql, runs the Rust --run-ignored only nextest lane, needs both deterministic guests, and refuses STACK_CONFORMANCE_UPDATE, STACK_ENCRYPT_UPDATE_FIXTURES and STACK_ENCRYPT_UPDATE_JSON_TREE_VECTORS. A contributor who has only Go and follows CONTRIBUTING.md now gets a cargo or nextest failure with no explanation. Proposed text:
CONTRIBUTING.md (the paragraph after the mise run go:test block):
For EQL v4 PostgreSQL coverage, set `EQL_TEST_DATABASE_URL` to a disposable
database and run `mise run go:test:postgres`. The task builds the EQL v4 bundle
from this tree and installs it into that database, so it replaces the EQL there.
It needs `psql`, the Rust test tools (`mise install --env test`) and both
deterministic guests. It refuses to run while a fixture-update variable, such as
`STACK_ENCRYPT_UPDATE_FIXTURES`, is set. Go examples that connect to ZeroKMS need
CipherStash credentials. The [Go module guide](languages/golang/README.md)
links to the SDK, generator, credentials, examples and release notes. Commit
generated `*_stash.go` files when changing Go model tags; CI checks for drift.languages/golang/README.md (the paragraph after the build block):
The `.wasm` files are build outputs and are not committed on development
branches. `mise run go:test:postgres` also runs the PostgreSQL suite against
the EQL v4 bundle built from this tree, and the shared EQL/JSON conformance
lane before it. It needs the disposable database named by
`EQL_TEST_DATABASE_URL`, `psql`, and the Rust test tools
(`mise install --env test`), because the conformance lane runs Rust tests too.
See `mise.toml` and [AGENTS.md](../../AGENTS.md) for the build and release
details.AGENTS.md (the "CI:" bullet under "Working on the Rust crates"), change the go:test:postgres clause to:
module on Linux, macOS and Windows, its PostgreSQL tests against the EQL v4
bundle built from the tree — `mise run go:test:postgres` locally, which also
runs the Rust and Go EQL/JSON conformance lane and so needs `psql` and the
`MISE_ENV=test` cargo tools — and the live tests), `crap-crates.yml`,| assert_eq!(payload["at"], case["at"], "{}", case["id"]); | ||
| assert_eq!(payload["construct"], case["construct"], "{}", case["id"]); | ||
| assert_eq!(error.field(), Some("profile")); | ||
| assert!(!format!("{error} {payload:?}").contains("secret")); |
There was a problem hiding this comment.
Nit: Go (json_match_test.go, TestMatchRefusalsNameTheConstructAndItsOffset) also checks that the error does not hold the predicate text. The contract says "no caller text in diagnostics", so this side must check it too. Without the check, a change that copies $.weight + 1 == 2 into the error still passes.
I ran this with cargo nextest run -p stack-encrypt --all-features --test json_operators shared_predicate_refusals. It passes, and cargo fmt --check is clean.
| assert!(!format!("{error} {payload:?}").contains("secret")); | |
| let rendered = format!("{error} {payload:?}"); | |
| assert!( | |
| !rendered.contains("secret") && !rendered.contains(predicate), | |
| "{}: the error holds the caller's text: {rendered}", | |
| case["id"] | |
| ); |
|
|
||
| ## EQL/JSON operation checkpoint | ||
|
|
||
| Implemented the six requirements in |
There was a problem hiding this comment.
Nit: eql-json-contract.md and eql-json-profiles.json both have nine requirements.
| Implemented the six requirements in | |
| Implemented the nine requirements in |
| # its prebuilt installer here too: an unselected shim is still found on PATH | ||
| # by the cargo backend, then fails before it can install any test tools. | ||
| "cargo-binstall" = "1.26.2" | ||
| "cargo:cargo-nextest" = "0.9.146" |
There was a problem hiding this comment.
This adds a pinned tool binary to every MISE_ENV=test job. The comment explains why. Please also say it in the PR description, because the description calls the PR test-only (see the review summary).
EQL/JSON tests already cover the operations, but lack shared requirements and a required, versioned conformance report. This enrolls resolver/catalog capabilities, document writes, queries, request-bound extraction/subtree reads, element-match and predicate-match in the existing Rust, Go and PostgreSQL suites.
Shares 11 valid documents, four invalid documents and 15 predicate refusals across native Rust and Go while retaining typed/generated API assertions and independent PostgreSQL plaintext oracles. The contract documents the supported grammar, null/missing behavior, authentication limits and intentional SQL differences.
Adds required fast and PostgreSQL CI steps. Compiled-test inventory checks and Go result checks reject missing or skipped evidence; reports record engine/binding versions, guest and lockfile hashes, installed PostgreSQL/EQL versions and the exact installed SQL bundle hash. Existing fixture paths remain intact and required runs refuse regeneration.
Validation:
Stacked on #1491; base is
feat/stack-encrypt-context-conformance. No production encryption behavior changes. Python/TypeScript runners, fresh JSON cross-language artifact exchange and release compatibility matrices remain separate work.Closes #1476