test(dev): pin that the engine SHIPS KMS_MODE blank, and drop the IAM name - #862
Merged
Conversation
… name I merged #861 while the review still said BLOCK, having assumed its verdict was the previous round's. It was not: it had re-run and raised a different, correct finding, and I did not read it before merging. Both fixes are here. 1) The shipped DEFAULT was not pinned, only the enforcement. My tests covered that the four AWS fields are `parityRequired` and that only `KMS_MODE=local` relaxes them. None of that notices the manifest going back to `{ scope: "local", value: "local" }` — `pnpm dev:secrets` would then WRITE `KMS_MODE=local` into every .dev.vars, preflight would relax all four, and the engine would use the hermetic KEK again with all three tests still green. The regression lives in what we SHIP, not in what we enforce, and I had only guarded the second. Now pinned on both: the spec's scope and value, and the rendered example line (`KMS_MODE=`), because the rendered artifact is what actually reaches a developer's machine.⚠️ My first mutation check "passed" and I nearly took that as proof the test was weak. It was the CHECK that was wrong: `s.index('name: "KMS_MODE",')` matched a `relaxedBy: { name: "KMS_MODE", ... }` reference seven lines earlier, so it mutated a different field's scope. Verifying the mutation actually mutated — printing the resulting spec — is what caught it. A mutation that does not mutate proves nothing in either direction. 2) The IAM principal name was in a public doc. `no-secrets` covers account identifiers, not only secrets, and this repo is public. The doc now says what the principal is scoped to without naming it; the name lives in the credential store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I merged #861 while the review still said BLOCK, assuming the verdict was the previous round's. It wasn't — it had re-run and raised a different, correct finding. Both fixes are here.
1. The shipped default wasn't pinned, only the enforcement
My tests covered that the four AWS fields are
parityRequiredand that onlyKMS_MODE=localrelaxes them. None of that notices the manifest reverting to{ scope: "local", value: "local" }—pnpm dev:secretswould then writeKMS_MODE=localeverywhere, preflight would relax all four, and the engine would silently use the hermetic KEK again with all three tests still green.Now pinned on the spec and the rendered example line, since the rendered artifact is what reaches a developer's machine.
s.index('name: "KMS_MODE",')matched arelaxedByreference seven lines earlier and mutated a different field. Printing the resulting spec caught it. A mutation that doesn't mutate proves nothing.2. IAM principal name in a public doc
no-secretscovers account identifiers, not just secrets. The doc now states the scope without the name.Test plan
pnpm lint— 997 pass, 0 faillocal "local") → fails; restored → passes🤖 Generated with Claude Code