Skip to content

test(stack-encrypt): enroll value-kind and query fixtures - #1469

Open
coderdan wants to merge 2 commits into
feat/stack-encrypt-read-conformancefrom
feat/stack-encrypt-value-query-conformance
Open

coderdan wants to merge 2 commits into
feat/stack-encrypt-read-conformancefrom
feat/stack-encrypt-value-query-conformance

Conversation

@coderdan

@coderdan coderdan commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

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-v1 and record-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/** and packages/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:

  • Expanded mise run test:conformance, including fresh scalar Go-to-Rust exchange.
  • Rust fixture/update regression and record-lowering tests; expanded Go missing-guest regression.
  • Negative checks: removing the Rust update guard, cfg-disabling an enrolled Rust test, and enrolling a nonexistent Go test each produced the expected failure; all temporary mutations were restored.
  • Rust clippy/formatting, Go lint, full Biome check, and 42 conformance/crate/script CI-wiring tests pass. Frozen fixture files remain unchanged.

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.

@changeset-bot

changeset-bot Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 52d96d9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI; Enforced overrides: Organization
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f0a16b41-5f83-4734-9622-9d80dae6b020

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Mutation testing (cargo-mutants, --in-diff, stack-auth + stack-encrypt)

No mutants were generated for the changed lines.

@coderdan
coderdan force-pushed the feat/stack-encrypt-value-query-conformance branch from 7efb9db to 4ca5783 Compare November 1, 2026 06:11
@coderdan
coderdan force-pushed the feat/stack-encrypt-read-conformance branch from f3638e4 to 3220805 Compare November 1, 2026 06:11
@coderdan

coderdan commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Review: enroll value-kind and query fixtures

Reviewed 7efb9db73 against feat/stack-encrypt-read-conformance, on two axes: the repo's coding standards, and the plan step in docs/plans/2026-10-31-stack-encrypt-conformance.md plus this PR's description. The CI test passes locally (6/6). Removing the STACK_CONFORMANCE_REQUIRED guard from query_test.go makes the Go regression test fail, so that test does catch a missing guard.

Standards

No serious breach. No changeset or skill update is needed, because the PR changes only tests, docs and tooling.

Against documented standards (minor):

  1. The new Rust guard has never been seen to fail. AGENTS.md says: "Assume a guard is untested until you have watched it fail." The assert! in fixture_updates_requested() (packages/stack-encrypt/tests/common/mod.rs:24) refuses fixture regeneration when STACK_CONFORMANCE_REQUIRED=1, but no test runs it. The shell-level refusal and the Go missing-guest case both have regression tests; the Rust refusal does not.
  2. Two doc comments are out of date. query_operators.rs:365 and value_kinds.rs:447 still say only "With STACK_ENCRYPT_UPDATE_FIXTURES=1 it is written first". They don't mention that a required run now refuses the update.
  3. An issue number in a doc (borderline). The plan doc adds "stacked on PR test(stack-encrypt): add stored-read and context conformance #1462". It's a plan doc rather than a README or code comment, and the file already says "PR feat(stack-encrypt): add owned scalar ciphertext API #1404".

Code smells (judgement calls):

  • Duplicated code. This skip condition is now in query_test.go:132 and value_kinds_test.go:155:
    (errors.Is(err, ErrDeterministicGuestNotBuilt) || errors.Is(err, ErrGuestNotBuilt)) && os.Getenv("STACK_CONFORMANCE_REQUIRED") != "1"
    Variants of it appear in five more places across the Go conformance tests. A skipUnlessConformanceRequired(t, err) helper would replace all seven.
  • Two ways of checking the update flag. record_lowering.rs:431 and :746 still read var_os("STACK_ENCRYPT_UPDATE_FIXTURES") directly instead of using the new helper.
  • A name that hides a panic. fixture_updates_requested() reads like a plain yes/no check, but it can panic. A different name, or a separate check and guard, would make that visible.
  • Three edits to enroll one test. Adding one Go test means changing fixture-profiles.json, the hand-written -run regex in scripts/stack-conformance.sh, and the table in fixture-contract.md. The CI test catches them drifting apart, but the regex could be generated from the index.

Spec

The 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:

  • "Its tests exist" is checked only by text search. The CI test searches the source for #[test] fn name( and func X(t *testing.T). It never lists the compiled tests (cargo nextest list or go test -list), so a test behind a compile-time feature flag would still pass.
  • "Enrolled Rust writers … refuse fixture regeneration" is implemented correctly, but no test checks it (Standards item 1).
  • The Go regression test runs only 2 of the 7 enrolled tests. The other 5 go through the same queryFixtureClient guard, so the risk is low.

Not mentioned in the PR description:

  • A new path-filter rule. The CI test now requires packages/stack-encrypt/** in both workflows' path filters. Reasonable, but undeclared.
  • Two more Go tests now fail instead of skip. tests-golang.yml sets STACK_CONFORMANCE_REQUIRED=1 for the whole workflow. So TestAQueryTheFieldDoesNotAnswerIsRefused and TestAMatchesQueryNeverPrintsItsText now fail rather than skip when the guest is missing, although neither is enrolled. Every job checked builds or downloads the guest, so nothing breaks today.

Possibly wrong:

  • The regression test no longer notices extra tests being selected. An exact string match on the Go command became a check that each enrolled name matches the regex.

Summary

  • Standards: 3 minor issues and 4 judgement calls. The worst is the Rust regeneration guard that nothing tests.
  • Spec: 3 overstated claims, 2 undeclared changes and 1 loosened check. The worst is "its tests exist", which is checked only by text search.

@coderdan
coderdan marked this pull request as ready for review November 1, 2026 06:16
@coderdan
coderdan requested a review from a team as a code owner November 1, 2026 06:17
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T06:18:21.088403Z 4ca5783 Draft marked ready
🔒 Security Review ✅ Completed 2026-11-01T06:19:25.171687Z 4ca5783 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

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 cipherstash-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json requirement IDs, the documented table in fixture-contract.md, and the named Rust/Go tests agree.
  • scripts/stack-conformance.sh exports STACK_CONFORMANCE_REQUIRED=1 and 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 -run selection 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.rs direct env reads

Codex review was still running at the time of this review.


Generated by Claude Code

@coderdan
coderdan force-pushed the feat/stack-encrypt-value-query-conformance branch from 4ca5783 to 52d96d9 Compare November 1, 2026 06:33

coderdan commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review findings in 52d96d9:

  • Added a durable subprocess regression for the Rust update guard, renamed it fixture_updates_requested_or_panic, documented the panic, and applied it to both record-lowering writers as well as the two enrolled writers.
  • Consolidated the seven Go missing-guest checks into one helper. The regression now reads the index and exercises every enrolled fixture test, plus the two full-suite query tests sharing the helper. Their required-mode behavior is documented.
  • Generated the Go selection from the enrollment index and restored an exact-set check, including rejection of accidental prefix/suffix matches. Enrollment no longer needs a hand-edited shell regex.
  • Added compiled Rust test-inventory checks that reject missing and ignored tests. Go checks each indexed test's actual subprocess outcome, so a missing compiled test also fails.
  • Updated the writer comments, removed the transient parent-PR reference from the plan checkpoint, and clarified the CI path-filter requirements in the PR description.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants