Conversation
|
f1cdbb8 to
887529d
Compare
28beeca to
8787b99
Compare
887529d to
ab8ca58
Compare
A policy derives each field's encryption context from names, so an ordinary rename (a proto field, a Go struct field, a table) can change a context with no error on the write path: rows already written stop decrypting and their query terms stop matching. plantest.Golden makes that a test failure. Golden builds the plan as plan.PlanFor does at startup and compares what it stores each field as with a snapshot checked in at testdata/<test name>.golden; -update rewrites it. The snapshot records only what stored rows depend on: per encrypted field its column, context, index terms and facts, keyed by column rather than field name, so a rename the policy pins with plan.Column leaves it byte-for-byte unchanged and the test passes; per field decided Plaintext, its name and facts. Output is sorted and line-ending-normalised, so it is the same on every run and platform. On a mismatch the failure sorts the changes by cost: context changes (data loss) first, target changes (a migration) next, then the rest, followed by a unified diff. For a lost context it names the pin that keeps it, and only after building the plan with that pin to confirm it restores the old column and context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U5Y8iJ3ZM71digszHzpyrd
8787b99 to
0478ea2
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Custom targets, plaintext-only policies, and Windows paths can currently produce incorrect guidance or failures.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Adds golden snapshot testing for Go encryption policies to detect unsafe context and target changes.
Changes:
- Introduces
plantest.Golden, deterministic snapshots, diffing, and migration guidance. - Adds comprehensive tests and checked-in policy snapshots.
- Documents golden policy testing.
| File | Description |
|---|---|
languages/golang/stackencrypt/README.md |
Documents golden tests. |
plan/doc.go |
Links policy documentation to plantest. |
plantest/plantest.go |
Implements the public helper and update workflow. |
plantest/snapshot.go |
Builds, renders, and parses snapshots. |
plantest/compare.go |
Classifies changes and generates guidance. |
plantest/plantest_internal_test.go |
Tests comparison and snapshot behavior. |
plantest/golden_test.go |
Exercises the public API. |
testdata/TestPolicies/individuals.golden |
Adds an encrypted-policy fixture. |
testdata/TestPolicies/audits.golden |
Adds a plaintext-policy fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The snapshot now records each column's target kind (EQL when the context is the column identity, Custom when the target supplies it), and the comparison uses it: - A table change is a context change only when the message has an EQL column; a plaintext-only or Custom-only message loses nothing. - A new column under an old one's context is guessed a database column rename only between EQL columns. Custom columns may share a context, and a column gone from a context another still writes is not a lost context. - "Restore plan.Table" is shown only after building the plan with the old table proves it brings the context back, with a pin if one is also needed. goldenPath also spells Windows device names (CON, NUL.x, COM1) and a trailing dot the same way on every platform, so a subtest name cannot fail on Windows alone or share another's snapshot there.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79ac8e77ec
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…robes
goldenPath spelled distinct subtest names alike ("a:b" and "a?b" were
both a_b.golden), so two tests read and wrote one snapshot. A name part
spelled differently from the test's, or one Windows reserves as a device,
now takes '~' and an FNV-32a hash of the original before its first dot.
No name spelled as it is contains '~', and the hash also leaves a device
stem unreserved, replacing the '_' suffix.
targetKind read one sample identity, so plan.Custom("plantest/probe")
was recorded as EQL. It now asks with two identities: a Custom context
is fixed, so it can equal one but never both.
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Nothing must change before merging. The package is well covered. It has the two acceptance cases, the 11-case change-classification table, the determinism and round-trip tests, the CRLF case, and the Windows path and collision cases. go test ./... passes in languages/golang/stackencrypt/plan/plantest on this branch.
The one finding worth taking soon is the rename guess in compare.go. An old column can lose its context. If exactly one new column carries the same facts, the failure says "likely the same field renamed". It then offers a checked plan.Column pin. It does this even when the two fields are unrelated. If the developer applies that pin, the code stores the new field's values in the old column under the old context. Nothing else in the message lists the new column, so the developer never learns that it is new. I reproduced this with a temporary test. The other findings listed here can wait.
Other findings not posted as comments
TestUpdateFlag(plantest_internal_test.go:563) sets the process-wide-updateflag.updating()readsflag.Lookup("update")onflag.CommandLine. That is one value for the whole test binary, and the externalplantest_testtests share that binary. Nothing runs in parallel today, so the test is correct now. If someone later addst.Parallel()toTestPolicies, or to one of its subtests, that test can run while this test holds the flag attrue.Goldenwould then rewritetestdata/TestPolicies/*.goldenfrom the policy and report success, so a changed context would pass in CI. A seam inplantest.gothat the test can replace (var lookupFlag = flag.Lookup) removes the shared state.parserejects four kinds of bad input, and only one of them has a test.TestTextOnlyAndUnreadableSnapshotscovers an unknown line. There is no test for an empty or truncated file ("no table line",snapshot.go:270), an unterminated quoted value (snapshot.go:289), an unknown target kind (snapshot.go:253), or a secondtableline. Whenparsefails,explaindrops the three-section summary and prints only the diff. A parse rule that rejects valid input therefore hides the context-change warning.- No test covers a read error that is not "the file does not exist" (
plantest.go:181). If that branch goes away, the code reports a snapshot it cannot read as a missing snapshot. The failure then tells the developer to run-update, which hides the real cause. - No test covers the write failures in update mode (
os.MkdirAllandos.WriteFile,plantest.go:190andplantest.go:195). If either error is dropped,-updatereports success while it writes no snapshot. - No test covers the error for a source that gives one field twice (
snapshot.go:111). The code would write both facts with the sameFieldinto the snapshot. Which one wins depends on the order the source returns them, so the snapshot stops being the same on every run.plan.PlanForrejects a repeated encrypted field first. This check is therefore reached only through a repeated field that the policy decidesPlaintext. - No test covers the branch that gives up on aligning a large snapshot (
compare.go:375). A policy over a few hundred fields produces more than 2048 lines, so this branch runs on real input. Every diff test uses about 20 lines. - No test checks the line numbers and counts in the
@@hunk header (compare.go:351). Wrong numbers send the reader to the wrong line of a checked-in snapshot. The+,-and renderwrites a column's index terms unquoted (snapshot.go:178, throughtermList), while names, contexts and facts go throughtoken. Today everyTermKind.String()value is free of spaces, so nothing breaks. Closing this needs either atokencall per term, or a comment that says the term names are a closed set with no spaces.rerun(plantest.go:163) has one test case,TestPolicy/a.b. No test checks a top-level test name with no/, and that is the common case.
I dropped one finding from a source after I checked the code. The claim was that two probe identifiers cannot prove that a plan.Target binds the column identity as its context. targetKind (snapshot.go:70) reports EQL only when the context equals both probes, and a fixed plan.Custom context can equal at most one probe. To reach a wrong answer, a target must return id.String() for both probe identifiers and a fixed context for everything else. The discussion already covers the single-probe version of this, and the author fixed it in 6e7204f2e.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5 | test-gap | 7 found, 2 posted |
| claude | claude-opus-5 | golang | 4 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | golang | 1 found, 0 posted |
Synthesis: claude-opus-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: claude-opus-5 read every comment as a new reader would. 3 comment(s) had a problem that stopped the reader acting; it rewrote 0. It also rewrote the review body.
Stack: not part of a stack.
Context loaded: the description, 0 linked issue(s) and 16 discussion entries.
| return !before && !seenCol[n.name] && !oldContexts[n.context] && len(o.facts) > 0 && slices.Equal(n.facts, o.facts) | ||
| }); ok { | ||
| seenCol[n.name] = true | ||
| msg += fmt.Sprintf(" Field %s now writes column %s under %q with the same facts, so it is likely the same field renamed. %s", |
There was a problem hiding this comment.
Fix in a follow-up: this message can name an unrelated new field as a rename, and the pin it offers then stores that field's values in the old column under the old context.
Impact: A developer removes one classified field and adds a different field with the same annotations in the same change. The old column's context is lost. Exactly one new column carries the same facts, so this branch runs and calls it "likely the same field renamed". pinAdvice checks the pin and the pin does work, so the hint looks verified. The developer applies it, and the new field's values are written into the old column under the old field's context. That column then holds two different kinds of data. Line 140 also sets seenCol[n.name] = true, which removes the new column from the "new column" list at line 164, so no other line tells the developer that this column is new.
Evidence: I added a temporary test to the package. The old field home_phone was removed and an unrelated new field work_phone was added, both annotated user.contact.phone. The failure printed exactly one line:
- column home_phone: no field writes its context "individuals/home_phone" any more. Field work_phone (WorkPhone) now writes column work_phone under "individuals/work_phone" with the same facts, so it is likely the same field renamed. Pinning the rule that decides field work_phone (WorkPhone) with plan.Column("home_phone") keeps it.
There is no second line saying work_phone is new.
Fix: Keep the guess, but state the other reading and keep listing the new column. Two changes:
}); ok {
seenCol[n.name] = true
msg += fmt.Sprintf(" Field %s now writes column %s under %q with the same facts, so it may be the same field renamed: %s"+
" If %s is instead a new field and %s was removed, do not pin it: that would store two fields in column %s under one context.",
fieldName(n.from), token(n.name), n.context, pinAdvice(m, facts, n.from, o, old.table),
token(n.name), token(o.name), token(o.name))
c.stored(o, n)and add the new column to c.other as well, so it is still listed:
c.other = append(c.other, fmt.Sprintf("new column %s, under %q, with terms [%s]. It is also named above as a possible rename of column %s.",
token(n.name), n.context, termList(n.terms), token(o.name)))Once a protobuf fact source exists, plan.Fact.Number is real evidence here, because a proto field keeps its number through a rename. Recording the number in the snapshot and requiring it to match turns the guess into a fact.
Found by 1 model: claude
| } | ||
| if err != nil { | ||
| t.Error(err) | ||
| } |
There was a problem hiding this comment.
Fix in a follow-up: no test runs the exported Golden function in the failing direction.
Impact: If someone removes this t.Error(err) call, or changes it to t.Log, every caller's test keeps passing while a changed encryption context reports nothing. The whole package then checks nothing, and no test in this repository fails.
Evidence: golden_test.go:45 and golden_test.go:46 are the only calls to Golden, and both match their checked-in snapshot, so both pass. Every failure case in plantest_internal_test.go calls the unexported check directly. Golden holds three steps check does not: goldenPath(t.Name()), the -update flag read, and this t.Error.
Fix: Add a test in plantest_internal_test.go that gives Golden a stand-in for *testing.T. Embedding *testing.T satisfies testing.TB, so this compiles.
// recorder stands in for *testing.T to read what Golden reports.
type recorder struct {
*testing.T
failed bool
msg string
}
func (r *recorder) Error(args ...any) { r.failed = true; r.msg = fmt.Sprint(args...) }
func TestGoldenReportsAMissingSnapshot(t *testing.T) {
r := &recorder{T: t}
Golden(r, individualV1(), plan.ForMessage(nil, "individuals", base))
if !r.failed {
t.Fatal("Golden passed with no snapshot checked in")
}
if !strings.Contains(r.msg, filepath.Join("testdata", t.Name()+".golden")) {
t.Errorf("message does not name the snapshot path: %s", r.msg)
}
}The test expects Golden to call Error and to name the path that goldenPath(t.Name()) returns.
Found by 1 model: claude
| token(n.name), termList(n.terms), termList(o.terms))) | ||
| } | ||
| if o.kind != n.kind { | ||
| c.other = append(c.other, fmt.Sprintf("column %s: its target is %s, was %s, under the same context.", token(n.name), n.kind, o.kind)) |
There was a problem hiding this comment.
Optional: this line says "under the same context" even when the context changed.
Impact: compare calls stored(o, n) at line 92 for every column that kept its name, including a column whose context changed and that it has just reported under CONTEXT CHANGES. The developer then reads two statements about one column that contradict each other: the first says the context moved, the second says the target changed under the same context.
Evidence: I ran check on a column that moved from plan.EQL to plan.Custom("elsewhere/v1"). The failure held both lines:
- column medicare_number: its context is "elsewhere/v1", was "individuals/medicare_number". ...
- column medicare_number: its target is Custom, was EQL, under the same context.
Fix: Say "under the same context" only when it is true.
if o.kind != n.kind {
where := ", under the same context"
if o.context != n.context {
where = ""
}
c.other = append(c.other, fmt.Sprintf("column %s: its target is %s, was %s%s.", token(n.name), n.kind, o.kind, where))
}Found by 1 model: claude
| } | ||
| for _, n := range cur.plaintext { | ||
| if _, before := oldPlain[n.field]; !before && !seenPlain[n.field] { | ||
| c.other = append(c.other, fmt.Sprintf("new plaintext field %s.", token(n.field))) |
There was a problem hiding this comment.
Optional: no test covers the "new plaintext field" message.
Impact: If a change drops this branch or puts its message in the wrong section, no test fails. A policy that starts deciding an existing field as Plaintext would then be reported wrongly, or not reported at all.
Evidence: TestChangesAreSortedByWhatTheyCost in plantest_internal_test.go:151 covers 11 kinds of change, including "plaintext now encrypted" and "plaintext field gone". It has no case for a field the policy newly decides Plaintext. I grepped the says: values in that table and none contains new plaintext field.
Fix: Add one case to that table.
"field newly decided Plaintext": {
before: plan.ForMessage(nil, "individuals", base),
after: plan.ForMessage(nil, "individuals", plan.FirstOf(
plan.When(plan.Field("id"), plan.Plaintext()),
).OrElse(base)),
section: "OTHER CHANGES",
says: []string{"new plaintext field id."},
},The test expects only OTHER CHANGES, holding new plaintext field id.
Found by 1 model: codex
| colAt, plainAt = -1, len(s.plaintext)-1 | ||
| case indented && toks[0] == "context" && len(toks) == 2 && colAt >= 0: | ||
| s.columns[colAt].context = toks[1] | ||
| case indented && toks[0] == "target" && len(toks) == 2 && colAt >= 0 && (toks[1] == kindEQL || toks[1] == kindCustom): |
There was a problem hiding this comment.
Optional: a column entry with no target line parses, and a table rename is then called harmless when it is not.
Impact: parse requires a table line but requires nothing inside a column entry. A column with no target line gets kind == "". compare at line 63 asks whether any old column has kind == kindEQL to decide whether a table rename moves a context. With every kind empty the answer is no, so the rename lands in OTHER CHANGES with the text "No context moves with it: the message has no EQL column". For a snapshot whose columns really are EQL, that is the opposite of the truth. The per-column context diff still reports the loss, so nothing is missed outright, but the failure then holds a reassuring sentence next to a data-loss one. Reaching this needs a snapshot edited by hand, which the file header tells people not to do, so the risk is small.
Evidence: I ran parse on a snapshot whose one column had context and terms but no target line. It returned no error, and the column's kind was the empty string.
Fix: Require a context and a target line for every column, so an incomplete snapshot fails to parse and the caller falls back to the plain diff. Add this after the loop:
for _, c := range s.columns {
if c.kind == "" || c.context == "" {
return snapshot{}, fmt.Errorf("column %q has no context or target line", c.name)
}
}Found by 1 model: claude
| if id, ok := strings.CutPrefix(context, string(table)+"/"); ok && id != "" { | ||
| tries = append(tries, | ||
| try{fmt.Sprintf("plan.Identity(%q)", id), []plan.RuleOption{plan.Identity(id)}}, | ||
| try{fmt.Sprintf("plan.Column(%q), plan.Identity(%q)", column, id), []plan.RuleOption{plan.Column(column), plan.Identity(id)}}, |
There was a problem hiding this comment.
Optional: no test checks the advice that names both pins, plan.Column(...), plan.Identity(...).
Impact: This is the advice for the most expensive case: a field renamed after its database column was already renamed. If this try stops working, pinAdvice falls through to the next answer. The developer then reads either a single pin that does not restore the context, or "No plan.Column or plan.Identity pin ... brings it back". They would re-encrypt data that this pair of pins could have kept.
Evidence: The CONTEXT CHANGES section title at compare.go:47 promises this advice ("and plan.Identity after a database column rename"). The table in plantest_internal_test.go:151 asserts plan.Column("...") alone and plan.Identity("...") alone, and never the pair.
Fix: Add one case to that table.
"database column renamed, then the field renamed": {
before: plan.ForMessage(nil, "individuals", plan.FirstOf(
plan.When(plan.Field("medicare_number"), gov,
plan.Column("medicare_num"), plan.Identity("medicare_id")),
).OrElse(base)),
after: plan.ForMessage(nil, "individuals", base),
src: individualV2(),
section: "CONTEXT CHANGES",
says: []string{
`column medicare_num: no field writes its context "individuals/medicare_id" any more.`,
`with plan.Column("medicare_num"), plan.Identity("medicare_id") keeps it.`,
},
},The test expects the failure to name both pins in one sentence, because only both together restore the old column and the old context.
Found by 1 model: claude
| Run it once with `go test -run '^TestIndividualsPolicy$' -update` to write | ||
| `testdata/TestIndividualsPolicy.golden`, and check the file in. It lists the | ||
| message's table and, for every field the policy decides, what it is stored | ||
| as: an encrypted field's column, context, index terms and facts, or a | ||
| plaintext field's name and facts. | ||
|
|
||
| ```text | ||
| table individuals | ||
|
|
||
| column email | ||
| context individuals/email | ||
| terms eq match | ||
| fact fides.data_categories user.contact.email | ||
|
|
||
| column medicare_number | ||
| context individuals/medicare_number | ||
| terms eq | ||
| fact fides.data_categories user.government_id | ||
| ``` |
There was a problem hiding this comment.
This example is missing the 3 header lines and the target lines that plantest.Golden writes. This suggestion adds them to the example, and adds the target to the sentence above it.
You can ignore this suggestion if you apply the plain-language rewrite of the whole section, which already includes it.
| Run it once with `go test -run '^TestIndividualsPolicy$' -update` to write | |
| `testdata/TestIndividualsPolicy.golden`, and check the file in. It lists the | |
| message's table and, for every field the policy decides, what it is stored | |
| as: an encrypted field's column, context, index terms and facts, or a | |
| plaintext field's name and facts. | |
| ```text | |
| table individuals | |
| column email | |
| context individuals/email | |
| terms eq match | |
| fact fides.data_categories user.contact.email | |
| column medicare_number | |
| context individuals/medicare_number | |
| terms eq | |
| fact fides.data_categories user.government_id | |
| ``` | |
| Run it once with `go test -run '^TestIndividualsPolicy$' -update` to write | |
| `testdata/TestIndividualsPolicy.golden`, and check the file in. It lists the | |
| message's table and, for every field the policy decides, what it is stored | |
| as: an encrypted field's column, context, target, index terms and facts, or | |
| a plaintext field's name and facts. | |
| ```text | |
| # Written by plantest.Golden: what the policy stores each field it decides as. | |
| # Regenerate it with go test -update; do not edit it by hand. | |
| # A column's context is bound into every ciphertext and query term written under it, so once a row is written it must never change. | |
| table individuals | |
| column email | |
| context individuals/email | |
| target EQL | |
| terms eq match | |
| fact fides.data_categories user.contact.email | |
| column medicare_number | |
| context individuals/medicare_number | |
| target EQL | |
| terms eq | |
| fact fides.data_categories user.government_id | |
| ``` |
| ### Checking contexts in with a golden test | ||
|
|
||
| Nothing on the write path notices a changed context: rename a proto or | ||
| struct field with no pin and new rows are simply written under a new one, | ||
| while the rows already written stop decrypting. The `plan/plantest` package | ||
| turns that into a test failure: | ||
|
|
||
| ```go | ||
| import "github.com/cipherstash/stack/languages/golang/stackencrypt/plan/plantest" | ||
|
|
||
| func TestIndividualsPolicy(t *testing.T) { | ||
| plantest.Golden(t, source, Individuals) | ||
| } | ||
| ``` | ||
|
|
||
| Run it once with `go test -run '^TestIndividualsPolicy$' -update` to write | ||
| `testdata/TestIndividualsPolicy.golden`, and check the file in. It lists the | ||
| message's table and, for every field the policy decides, what it is stored | ||
| as: an encrypted field's column, context, index terms and facts, or a | ||
| plaintext field's name and facts. | ||
|
|
||
| ```text | ||
| table individuals | ||
|
|
||
| column email | ||
| context individuals/email | ||
| terms eq match | ||
| fact fides.data_categories user.contact.email | ||
|
|
||
| column medicare_number | ||
| context individuals/medicare_number | ||
| terms eq | ||
| fact fides.data_categories user.government_id | ||
| ``` | ||
|
|
||
| From then on the test builds the plan as `MustPlanFor` does at startup and | ||
| fails when it no longer matches the file, sorting the changes by what they | ||
| cost. A changed context is data loss and is reported first, with the | ||
| `plan.Column` (or `plan.Identity`) pin that keeps it; a changed target, such | ||
| as different index terms or a plaintext field now encrypted, is a migration; | ||
| anything else, such as a new field, is reported last. Encrypted fields are | ||
| listed by column, not by field name, so a rename the policy pins leaves the | ||
| file unchanged and the test passes. When a change is intended, rerun with | ||
| `-update` and review the diff. |
There was a problem hiding this comment.
A plain-language rewrite of this section: the rule first, short sentences, numbered steps, and one sentence per line.
| ### Checking contexts in with a golden test | |
| Nothing on the write path notices a changed context: rename a proto or | |
| struct field with no pin and new rows are simply written under a new one, | |
| while the rows already written stop decrypting. The `plan/plantest` package | |
| turns that into a test failure: | |
| ```go | |
| import "github.com/cipherstash/stack/languages/golang/stackencrypt/plan/plantest" | |
| func TestIndividualsPolicy(t *testing.T) { | |
| plantest.Golden(t, source, Individuals) | |
| } | |
| ``` | |
| Run it once with `go test -run '^TestIndividualsPolicy$' -update` to write | |
| `testdata/TestIndividualsPolicy.golden`, and check the file in. It lists the | |
| message's table and, for every field the policy decides, what it is stored | |
| as: an encrypted field's column, context, index terms and facts, or a | |
| plaintext field's name and facts. | |
| ```text | |
| table individuals | |
| column email | |
| context individuals/email | |
| terms eq match | |
| fact fides.data_categories user.contact.email | |
| column medicare_number | |
| context individuals/medicare_number | |
| terms eq | |
| fact fides.data_categories user.government_id | |
| ``` | |
| From then on the test builds the plan as `MustPlanFor` does at startup and | |
| fails when it no longer matches the file, sorting the changes by what they | |
| cost. A changed context is data loss and is reported first, with the | |
| `plan.Column` (or `plan.Identity`) pin that keeps it; a changed target, such | |
| as different index terms or a plaintext field now encrypted, is a migration; | |
| anything else, such as a new field, is reported last. Encrypted fields are | |
| listed by column, not by field name, so a rename the policy pins leaves the | |
| file unchanged and the test passes. When a change is intended, rerun with | |
| `-update` and review the diff. | |
| ### Catch a changed context with a golden test | |
| A field's context must never change after you write data under it. | |
| The library binds the context into every ciphertext and index term it writes for the field. | |
| If the context changes, the rows you already wrote stop decrypting, and their index terms stop matching queries. | |
| You get no error when a context changes. | |
| If you rename a proto field or a Go struct field, its context changes too. | |
| Your code then writes new rows under the new context. | |
| To keep the old context, pin the column with `plan.Column` in the field's rule. | |
| The `plan/plantest` package makes a changed context fail a test. | |
| It gives you a golden test. | |
| A golden test compares the plan with a file you commit, called the golden file. | |
| To add the test: | |
| 1. Write a test that calls `plantest.Golden` with your source and your policy: | |
| ```go | |
| import "github.com/cipherstash/stack/languages/golang/stackencrypt/plan/plantest" | |
| func TestIndividualsPolicy(t *testing.T) { | |
| plantest.Golden(t, source, Individuals) | |
| } | |
| ``` | |
| The plantest package defines the `-update` flag. | |
| If your test package defines its own `-update` flag, remove it, and read plantest's flag instead: | |
| ```go | |
| func update() bool { | |
| f := flag.Lookup("update") | |
| return f != nil && f.Value.String() == "true" | |
| } | |
| ``` | |
| 2. Run the test once with `-update`. | |
| This writes the golden file to `testdata/TestIndividualsPolicy.golden`: | |
| ```sh | |
| go test -run '^TestIndividualsPolicy$' -update | |
| ``` | |
| 3. Read the golden file, then commit it. | |
| The golden file names the message's table. | |
| It also lists each field the policy decides: | |
| - An encrypted field shows its column, context, target, index terms and facts. | |
| - A plaintext field shows its name and facts. | |
| This is the golden file for the `Individuals` policy above: | |
| ```text | |
| # Written by plantest.Golden: what the policy stores each field it decides as. | |
| # Regenerate it with go test -update; do not edit it by hand. | |
| # A column's context is bound into every ciphertext and query term written under it, so once a row is written it must never change. | |
| table individuals | |
| column email | |
| context individuals/email | |
| target EQL | |
| terms eq match | |
| fact fides.data_categories user.contact.email | |
| column medicare_number | |
| context individuals/medicare_number | |
| target EQL | |
| terms eq | |
| fact fides.data_categories user.government_id | |
| ``` | |
| After that, each test run builds the plan the same way `MustPlanFor` does at startup. | |
| If the plan does not match the golden file, the test fails. | |
| The failure lists each change, with the most costly changes first: | |
| 1. **Context changes.** | |
| These lose data. | |
| If a rename caused the change, the failure names the `plan.Column` or `plan.Identity` pin that keeps the old context. | |
| 2. **Target changes.** | |
| These need a migration. | |
| For example, the index terms are different, or a plaintext field is now encrypted. | |
| 3. **Other changes.** | |
| These do not affect rows you already wrote. | |
| For example, the policy decides a new field. | |
| The golden file lists encrypted fields by column, not by field name. | |
| If you rename a field and your policy pins its column, the file stays the same and the test passes. | |
| Change a context only before you write rows under it, or ship a migration with the change. | |
| If you made a change on purpose, run the test again with `-update`. | |
| Then read the diff of the golden file before you commit it. |
| plantest.Golden(t, source, Individuals) | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
A test package that imports plantest and defines its own -update flag panics at startup with "flag redefined: update". This suggestion adds a warning before the step that runs -update, and shows how to read plantest's flag instead.
You can ignore this suggestion if you apply the plain-language rewrite of the whole section, which already includes it.
| The plantest package defines the `-update` flag. | |
| If your test package defines its own `-update` flag, remove it, and read plantest's flag instead: | |
| ```go | |
| func update() bool { | |
| f := flag.Lookup("update") | |
| return f != nil && f.Value.String() == "true" | |
| } | |
| ``` | |
Summary
Stacked on #1022. In the Go SDK, a policy (
plan.Policy) decides how each field of a message is encrypted, and it builds each field's encryption context from names: for an EQL column, the context is"<table>/<column>". That context is mixed into every ciphertext and every searchable index term written for the field. If it changes, rows already written stop decrypting and stop matching queries, and nothing on the write path reports an error. Renaming a proto field or a Go struct field is enough to change it.This PR adds
plantest.Golden, a test helper that saves a snapshot of every context and target a policy produces to a checked-in file. The test fails when any of them changes. A context change is reported separately from other changes, along with theplan.Columnpin that keeps the old context.Changes
stackencrypt/plan/plantest:Golden(t, src, m)builds the plan the same wayplan.PlanFordoes at startup.testdata/<test name>.golden. A subtest's snapshot goes in a folder named after the parent test.-updatewrites the file. If the file already existed, it also logs what changed.plan.Fail) fails the test with the build error.Plaintextrecords its name and facts.\r\ncheckouts compare equal. The output is the same on every run and platform.plan.Column/plan.Identitypin, and the hint appears only if that restores the old column and context. Otherwise the message gives the reason no pin can (the table changed, or the target supplies its own context).planpackage doc.Verification
TestRenameWithoutAPinIsAContextChange) and a Go struct (TestStructFieldRename):plan.Column("medicare_number").TestChangesAreSortedByWhatTheyCostcovers 11 kinds of change. For each, it checks the change lands in exactly one section, with the expected advice.\r\ncheckout passes.TestPoliciesruns the realGoldenagainst two checked-in snapshots. CI runs these on Linux, macOS, Windows and 386.gofmt,go vet ./..., andgolangci-lintv2.14.0 (the versionmise.tomlpins) report 0 issues.go test ./...passes, also underGOARCH=386.GOOS=windows go vetpasses on the new package.stackencrypttests, because the WASI guests weren't built here. Their tests skip without the guests, and this PR doesn't touch them.No changeset: the Go module has no release process yet. No skill covers the Go module.
Review notes
plan.Columnthen leaves the snapshot unchanged, which is what lets the test pass "unchanged" after the pin is added. If the source field name were recorded, every safe rename would also need-update, and an unchanged snapshot would no longer mean "storage unchanged". The current field name still appears in every failure message, so the developer knows which rule to pin.Golden(t, src, m)with an explicitplan.Source, mirroringplan.PlanFor(src, m). Once a protobuf fact source exists, it plugs in assrc.plantestregisters the conventional-updateflag on the default flag set. A test package that defines its own-updatewould panic with "flag redefined". The package doc says to read plantest's flag instead.🤖 Generated with Claude Code
https://claude.ai/code/session_01U5Y8iJ3ZM71digszHzpyrd
Generated by Claude Code