docs(stack): stop advertising the WASM entry as browser-capable, and pin the reason against the core - #953
Conversation
three places on this branch saying the opposite — including the example a reader is most likely to copy. - `examples/supabase-worker/README.md` and its edge function both listed "modern browsers" among the runtimes the entry works in. The example reads `CS_CLIENT_KEY` from the environment, so it demonstrated the exact thing it claimed was browser-safe. - `packages/stack/tsup.config.ts` listed browsers among "the only runtimes that need wasm-inline", in the same file #810 cites as evidence there is no `browser` export condition. - `skills/stash-supabase` gave `pg` introspection as the reason the factory cannot run in a browser. True but incomplete, and misleading next to #805 ("add the `browser` export condition"): removing `pg` unblocks Workers, not browsers, because `clientKey` is required on every auth path regardless. Also corrects two claims in #810's own prose. `clientKey` is loaded before the core ever CALLS the strategy — it reads `opts.strategy` earlier than that — and the contract test displaces mocks in eight suites plus a stub in one, not "the stub every other wasm suite uses". (cherry picked from commit a438b0a)
…contract (#804) Review of #810 found the contract test proved less than it claimed, and that two of its stated premises were wrong. Both are fixed by testing them. `expect(calls.getToken).toBe(0)` had no positive control. Nothing in the file ever reached a state where `getToken` was called, so a count of zero was equally consistent with "key loading precedes auth" (the claim) and with "the core never calls `getToken` during `newClient` at all" — a counter that is never incremented reads as zero either way. It does call it. Structurally complete key material — derived from the core's own error messages, not from any credential: `{ p1, p2_from, p2_to, p3 }`, each a `Permutation` — clears the key provider, after which the core calls `getToken` exactly once and fails on the deliberately malformed token the stand-in returns. So auth IS reached during construction, only after the key is loaded, and the counter is live. Still offline: the token never parses, so nothing leaves the process. The claim that a rename of `opts.strategy` would silently hollow out the file turned out to be false, and is now pinned rather than assumed: the core reads the strategy by name BEFORE deserialising the credentials, so omitting it yields `opts.strategy is required`, which matches none of the other tests' regexes. They would fail loudly, not pass vacuously. Also pins the consequence the docs assert but nothing enforced: no `browser` export condition on any subpath. Adding one to quiet a bundler would ship a workspace secret to the browser and leave `src/wasm-inline.ts` silently wrong. Three corrections to comments that overstated the evidence: - "the strategy was never consulted" → never INVOKED. The core does read `opts.strategy` and typecheck its `getToken` ahead of serde; what never happens is the call. - "every other wasm test mocks `newClient`" → every wasm test that constructs a client. `wasm-inline-normalize.test.ts` relies on the alias stub, and `wasm-inline-bundle-isolation.test.ts` never loads protect-ffi at all. - "a stub that throws" → a stub whose `newClient` throws. Its `isEncrypted` returns `false`. The `e2e/wasm/roundtrip.test.ts` pointer now notes that suite covers the `accessKey` arm, so the federation arm reasoned about here has no live coverage anywhere. Fixes the stub's own docblock while adjacent: it claimed protect-ffi exports no `/wasm-inline` subpath, which 0.30.0 does, and which the contract test resolves directly. (cherry picked from commit 2f992d6)
… shape The two commits recovered from the #804 branch were written on 2026-07-28, against protect-ffi 0.30. Cherry-picking them onto main compiled but every credential assertion failed: 0.31 moved `clientId` / `clientKey` under `clientOpts` and denies unknown keys at the top level, so the calls were rejected with `unknown field 'clientId'` before reaching anything the test meant to observe. Nesting the credentials fixed three of six. The rest were asserting error strings the core no longer emits: missing field `clientKey` -> clientOpts.clientId and clientOpts.clientKey are required (one message names both; the core does not say which is absent) invalid clientKey: invalid hex -> invalid clientKey: expected a hex-encoded key opts.strategy is required -> Not authenticated The last one carried the file's ordering claim, so it was re-derived rather than re-spelled: probing all three combinations against the real core shows `Not authenticated` wins whenever `strategy` is absent, while omitting the credentials WITH a strategy gives the credential error. The strategy is still read first, which is what makes "even when an auth strategy is supplied" mean anything in the tests above. The finding the file exists to pin is unchanged and now pinned against the shipping core: federation does not remove the `clientKey` requirement. Claude-Session: https://claude.ai/code/session_01E1J2nVGJWVkqvLepDfinRf
🦋 Changeset detectedLatest commit: 5686439 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The reference doc and three TSDoc blocks all derived the default entry's
runtime from schema discovery: introspection needs Postgres, therefore the
entry cannot run on an edge runtime. That inference is false in both
directions. Declaring `schemas` removes the Postgres dependency entirely
(create.ts:303-306, :364-367) and the entry is still Node-only; and the
entry would be Node-only with no introspection code in it at all.
What actually pins it is the import: `Encryption` from `@cipherstash/stack`
pulls a module graph that statically imports `@cipherstash/auth`, whose Node
entry resolves its platform binding at module evaluation, and the emitted
bundle carries an `import("pg")` specifier a bundler resolves at build time.
Neither moves when you declare schemas.
The default entry's doc also named `@cipherstash/protect-ffi` as the binary
loaded on import. It is the one package in that graph that deliberately does
not: `packages/protect-ffi/src/index.cts` uses `import native = require(...)`
specifically so `__importStar` cannot force the neon proxy to resolve, and
`nativeLoading.test.ts` guards it.
Two smaller corrections in the same pass: bare "a Worker" is ambiguous and
false under the Node `worker_threads` reading — the native entry runs fine
there — so the edge runtimes are now named, as the table already named them;
and the browser prohibition is restored to the native entry, which the
previous revision moved onto the edge entry, leaving the native paragraph
implying the browser was fine.
Guarded by scripts/__tests__/supabase-runtime-claims.test.mjs (three
detectors, unit-tested in both directions, applied to the four prose
sources), and by three new assertions in wasm-entry-edge-safety.test.ts that
tie the corrected prose to the emitted bundles — its header comment repeated
the protect-ffi misattribution and would otherwise have contradicted them.
Not touched, to avoid conflicting with open PRs: skills/stash-supabase and
packages/stack-supabase/README.md carry defect 1 verbatim but are being
rewritten on #951 at those exact lines, and the browser-capability claims in
examples/ and packages/stack/tsup.config.ts belong to #953. The README path
is recorded in the guard's GUARDED list comment so it is added when #951
lands.
Claude-Session: https://claude.ai/code/session_01FVKXa6GjUHN5xvJq2912KA
#804) The contract test added two commits back resolves the REAL `@cipherstash/protect-ffi/wasm-inline` through Node — that is the point of it, since every other wasm suite gets the stub `vitest.shared.ts` aliases in. That entry is `dist/wasm/protect_ffi_inline.js`, wasm-pack output; only the three `.d.ts` beside it are tracked, and protect-ffi is a workspace package now, so `pnpm install` does not produce it. `run-tests` builds the binding without `wasm: 'true'` — deliberately, and its step says so — so the file could never collect there. It did not skip, it failed to COLLECT, which took the Node 24 job red on #953. A local `pnpm --filter @cipherstash/stack test` would have needed cargo and wasm-pack for the same reason. So it moves to its own config, the way the integration suites already do, and runs from `tests.yml`'s `wasm-e2e-tests` job — the one job that builds `dist/wasm/**`. Through turbo rather than a bare `pnpm --filter`, because `workflow-turbo-build-deps.test.mjs` rejects the bare form for a package with buildable workspace dependencies; the task is `cache: false` because the input this suite actually reads is a gitignored build output in ANOTHER package, which `$TURBO_DEFAULT$` cannot see. Three separate pieces now hold the suite up — an exclusion, a script, one workflow step — and deleting any of them leaves a green tree with the contract checked nowhere. `scripts/__tests__/wasm-core-contract-ci.test.mjs` is what fails instead; removing the step was mutation-checked against it. Not verified: that the suite PASSES against a real core. Building dist/wasm locally is a wasm32 cargo build of the cipherstash-client graph and there is no disk for it here. What was verified is that the CI command resolves the task, selects exactly that file, and fails on nothing but the missing artifact. Refs #804
ReviewThe finding this PR pins is right and worth pinning, and the port write-up (re-deriving the ordering claim by probing the core rather than re-spelling the old error string) is the right way to have done it. Six things below; two of them mean the guard is weaker than the PR body claims. Medium1. The contract is pinned on the deprecated strategy key. So "clientKey is required even when an auth strategy is supplied" is currently pinned for a field 2. The Net effect: someone adding Low3. 4. 5. The changeset overstates the diff. 6. Checked and not issues
|
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Comments from Claude are legit.
… the fourth piece (#804) Review found four things wrong with the reasoning around the apparatus added in 53d4b2a. None of them touches the fix itself — the contract test passed against the real core in CI on that commit — but three of the four were claims this repo would later be read as evidence for. "The only suite that loads the REAL WASM core" was false, and it was the stated justification for the whole arrangement. `packages/stack/integration/wasm/**` loads it (its config restores the genuine module over the stub alias), so do protect-ffi's own `wasm-round-trip` / `wasm-error-codes` suites and the three Deno suites in `e2e/wasm/`; three CI jobs build `dist/wasm/**`, not one. Stated in four places, corrected in all four. The true reason this file is not simply in `integration/wasm/**` is narrower and is now written down instead: it is the only one of those suites that needs the core and NOTHING else. `test-kit`'s integration `globalSetup` requires credentials AND a database unconditionally — it throws rather than skips, then runs a real `stash eql install` — and `integration-drizzle.yml` is path-filtered, fork-skipped and matrixed over two databases. A contract about the core would go unchecked on any diff those paths do not select. The guard held three of four load-bearing pieces; `turbo.json` was unasserted. The reviewer's proposed failure — delete the task and it goes quiet — turns out not to exist: turbo 2.x refuses a task the project does not declare, so the step exits 1. The silent one is the EDIT. Measured by dry run: strip `dependsOn` and flip `cache` to true and the build graph collapses from nine tasks to one while the suite becomes cacheable against a hash that cannot see the core it tests. Every other turbo guard stayed green through that mutation. Now asserted, and all four mutations (delete, cache-only, dependsOn-only, both) were confirmed to fail with the message that names the reason. `"inputs": ["$TURBO_DEFAULT$"]` is dropped, though not for the reason given. Turbo does compute an input set for an uncached task — narrowing `inputs` under `cache: false` cuts the hashed set from 170 files to 2 and changes the hash. The key is a no-op because `$TURBO_DEFAULT$` alone IS the default set, cached or not. Five other tasks carry the same no-op line; this one goes because the comment beside it reasons about what turbo can and cannot hash, which reads as though the key were load-bearing. Refs #804
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Requesting changes on two items. Full findings are in #953 (comment) — this review just marks which of them block.
The finding this PR pins is right, and re-deriving the ordering claim by probing the core rather than re-spelling the old error string was the right way to do the port. Both blockers are the guard being weaker than the PR body claims, and both are small.
Blocking 1 — the contract is pinned on the deprecated strategy key. __tests__/wasm-inline-core-credential-contract.test.ts:131 (and :157, :175, :188, :218) passes the strategy as opts.strategy. packages/protect-ffi/src/types.ts:338 marks that "Renamed to authStrategy. Still honoured … but it will be removed", and crates/protect-ffi/src/wasm.rs:469 reads authStrategy first. Production never takes that path — packages/stack/src/wasm-inline.ts:1573 passes authStrategy: strategy.
So "clientKey is required even when an auth strategy is supplied" is pinned for a field @cipherstash/stack does not send, and the day the alias is dropped four tests go red with Not authenticated, reading as a core regression rather than an alias removal. Use authStrategy, or assert both arms so the removal is legible.
Blocking 2 — the browser export-condition guard now runs almost nowhere. The assertion at :283 reads only packages/stack/package.json and needs no WASM at all, but it lives in the file vitest.config.ts:27 excludes, so it runs only in tests.yml / wasm-e2e-tests — a job with no fork guard that hard-fails at Require CipherStash secrets, so a fork PR never reaches the step.
Net: someone adds "browser" to exports to quiet a bundler — the exact accident the comment names — and it passes pnpm test and every fork PR. This repo's own AGENTS.md has the line for it: "a check nothing invokes reads exactly like a check that passes", and src/integrationSuiteCi.test.ts exists because a suite went quiet the same way. The WASM-loading tests are what needed the separate config; this one assertion belongs back in the default suite.
Worth fixing in the same pass, not blocking — the changeset overstates the diff. .changeset/wasm-inline-client-key-not-browser-safe.md:21 claims the constraint is now stated in the auth-strategy re-export, the stash-encryption entry-point table and the stash-edge skill. None of those are touched, and this text ships verbatim into @cipherstash/stack's CHANGELOG. Cheap now, permanent once released.
Findings 3, 4 and 6 in the comment are fine as follow-ups.
Suggested ordering: land #951 first and rebase this one. #951 replaces the sentence at skills/stash-supabase/SKILL.md:269 and this inserts a paragraph below it, so rebasing the insert onto the replacement is the easier direction — and this branch has edits pending anyway.
…ses (#804) The contract test supplied its auth strategy as `opts.strategy`. That is protect-ffi's deprecated alias — `NewClientOptions` marks it "Renamed to authStrategy", and `wasm.rs` reads `authStrategy` first and falls back to it — while `src/wasm-inline.ts` builds its call as `authStrategy: strategy`. So the assertion that clientKey is required even under OIDC federation was pinned on a field the shipping path never sets, and the day protect-ffi drops the alias four tests go red with `Not authenticated`, reading as a core regression rather than a rename. Both arms now run, named in their titles, so an alias removal turns exactly one of them red and `STRATEGY_KEYS` says what to do about it. Move the `browser` export-condition assertion into each package's default suite. It reads a manifest — no WASM build, no credentials — but it lived in the file `vitest.config.ts` excludes, so it ran only in `wasm-e2e-tests`: in no local `pnpm test`, and on a fork PR in nothing at all, since that job and `run-tests` both hard-fail at `require-cs-secrets` and `lint` runs only Biome. Adding a `browser` condition to quiet a bundler — the exact accident the comment names — passed everything a contributor runs. `@cipherstash/stack-supabase` gets the same guard. It carries the same "not browser-safe (#804)" note and the same `./wasm-inline` export, and nothing covered it. `vitest.wasm-core.config.ts` set no `testTimeout`, so it inherited Vitest's 5000ms while the sibling config deliberately raises it to 30000 for this package. Every case there instantiates the real inlined core and the last loads a key and calls `getToken` — a flake waiting for a cold runner, in the one job that runs it. `wasm-core-contract-ci.test.mjs` grows the two matching wiring checks. Claude-Session: https://claude.ai/code/session_01THGKcgRdyd2aPt5zBnPVLH
…overstating the diff (#804) The changeset claimed the constraint was now stated in "the auth-strategy re-export, the `stash-encryption` entry-point table, the `stash-edge` and `stash-supabase` skills". Only `stash-supabase` was touched; the re-export is unchanged, and neither of the other two was in the diff. That text ships verbatim into `@cipherstash/stack`'s CHANGELOG. Rather than only trimming it, land the piece that was worth landing. `skills/stash-edge` is what a customer's agent reads when deploying the WASM entry and it did not contain the word "browser" at all, so it now carries the constraint at the head of its Credentials section: `clientKey` is a workspace secret required on every auth path including `authStrategy`, because the core loads it as key material before it ever calls the strategy. The `stash-encryption` entry-point row gains a clause pointing there — that row calls this "the **edge** entry" and lists V8 runtimes, which is the reading that put "modern browsers" in the example README in the first place. The changeset now names only what landed, and narrows "had all described this entry as browser-capable" to the two places that actually did. `turbo.json`'s `test:wasm-core` comment did not say that `^build` cannot make the task runnable: `@cipherstash/protect-ffi#build` is `tsc` declaring `outputs: ["lib/**"]`, and the input the suite loads comes from `build:wasm`, which is a package.json script and no turbo task at all. Left as a comment rather than a dependency on purpose — declaring the wasm build would put a Rust toolchain on the path of everyone who runs the task, which is what protect-ffi's script split exists to prevent. Claude-Session: https://claude.ai/code/session_01THGKcgRdyd2aPt5zBnPVLH
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approving. I reviewed this at HEAD (9038aa1) — note for anyone reading the review timeline: Dan's changes-requested reviews predate the last two commits (01:41Z), and I verified each of his six findings against what is now on the branch rather than re-litigating the earlier state:
- Deprecated
strategykey — resolved by taking the "assert both arms" option: the contract suite is parameterized over[{key: 'authStrategy', role: 'the key production passes'}, {key: 'strategy', role: 'the deprecated alias'}], with the comment spelling out that when protect-ffi drops the alias only thestrategyarm goes red and what to do then. The shipping path is now covered. - Browser export-condition guard ran almost nowhere — the assertion moved into its own default-suite file (
packages/stack/__tests__/browser-export-condition.test.ts);WASM_CORE_SUITEnames only the contract file, so the guard runs under plainpnpm testand on fork PRs, andscripts/__tests__/wasm-core-contract-ci.test.mjspins it staying there. - Missing
testTimeout—vitest.wasm-core.config.tsnow sets 30000. ^buildcannot maketest:wasm-corerunnable — now owned explicitly in a turbo.json comment as the chosen trade (the alternative puts cargo on everyone's task path, which the protect-ffi script split exists to prevent), and the dead"inputs"line is gone. Documenting rather than "fixing" is the right call here given AGENTS.md's cargo-free-test rule.- Changeset overstated the diff — resolved by landing the rest:
skills/stash-edgeandskills/stash-encryptionare now actually touched, and the changeset text matches the diff. @cipherstash/stack-supabaseunguarded — it now has its own siblingbrowser-export-condition.test.tsasserting against its own manifest.
Beyond Dan's list, the substance holds up: the core claim (clientKey required on every auth arm, loaded as key material before the strategy is ever called) is pinned against the real WASM core with a two-stage decode assertion and call-order evidence, and the docs corrections line up with the false-cause cleanups in #951/#952 — this PR's skills/stash-supabase hunk fixes the browser-reason instance of the same inversion. CI is green at HEAD.
One rebase note already known from #951's review thread: both PRs edit adjacent text in skills/stash-supabase/SKILL.md; whichever merges second keeps both halves (this one's browser reason, that one's Worker correction).
A review argued the separate config, turbo task and CI guard are redundant, because `integration/wasm/**` already loads the real core and `integration-drizzle.yml` already builds `dist/wasm/**` with `wasm: 'true'`. Both halves of that are true. The argument still fails, on one fact: `packages/test-kit/src/integration/global-setup.ts` requires `cipherstash` AND `database` unconditionally and throws rather than skipping, then runs a real `stash eql install`. Moving the contract test there would cost it credentials, a database and a built CLI — everything it currently needs none of. That fact lived in prose in three docblocks and nothing enforced it. Making global-setup skip when unconfigured is the obvious "let people run integration tests locally" change, and it would make the redundancy argument correct while every docblock still said otherwise. One case in `wasm-core-contract-ci.test.mjs` now pins both halves — the unconditional requirement list, and that it is a throw and not a skip. Three sentences were also wrong and are corrected. The contract file said every suite that reaches the core hands it a complete real credential; protect-ffi's `wasm-error-codes` does not — it passes no `clientOpts` at all, and misses #804 for a different reason (its cases fail in config validation, and none supplies an auth strategy). The guard's docblock made the same "only one that needs nothing else" claim. The CI step comment claimed the integration job has PostgREST, which `integration-db` documents as empty for the `postgres` variant, and said "most diffs" where the condition is every diff the path filter does not select. `vitest.wasm-core.config.ts` opened by calling this "the one suite in stack's DEFAULT test run", twenty lines above explaining that it is excluded from that run. It is the one suite in the package that loads the core outside the integration harness. Claude-Session: https://claude.ai/code/session_01THGKcgRdyd2aPt5zBnPVLH
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-approved at 5686439. The post-approval commit is narration-accuracy work: it corrects the over-claim a review caught (this is not the only credential-free core-loading suite — wasm-error-codes is too, but it lives behind the path-filtered, fork-skipped integration workflow), and pins the fact the arrangement actually argues from by importing test-kit's env module into the CI guard so the unconditional cipherstash+database requirement can't silently relax. No behaviour change to anything reviewed; CI green.
3 participants
Summary
@cipherstash/stack/wasm-inlineis the build of our encryption client that carries the engine as a WebAssembly blob instead of a native module, so it can run where native modules cannot: Deno, Supabase Edge Functions, Cloudflare Workers. Several places in this repo also advertised it as working in a browser. It does not, and cannot today.The reason is
clientKey. It is a workspace-wide secret, the client requires it on every authentication path — including the OIDC federation path, where a per-user token might look like it should replace it — and the core loads it as encryption key material during construction. Putting that entry in a browser bundle ships the workspace secret to every visitor.This PR removes the browser claim from the four places that made it, and adds a test suite that pins the reason against the real WASM core rather than a mock, so the claim cannot quietly stop being true.
Changes
Documentation that said the wrong thing
examples/supabase-worker/README.mdand its edge function both listed "modern browsers" among the runtimes this entry supports. The example readsCS_CLIENT_KEYfrom the environment, so it demonstrated the exact thing it called browser-safe.packages/stack/tsup.config.tslisted browsers among "the only runtimes that need wasm-inline" — in the same file cited elsewhere as evidence that nobrowserexport condition exists.skills/stash-supabase/SKILL.mdgave Postgres introspection as the reason the factory cannot run in a browser. True but incomplete, and misleading next to Add thebrowserexport condition to @cipherstash/stack-supabase, with a live browser smoke test #805 ("add thebrowserexport condition"): dropping thepgdependency unblocks Workers, not browsers, becauseclientKeyis required either way.packages/stack/src/wasm-inline.tsnow states the constraint where callers meet it, onWasmClientConfigand itsclientKeyfield.A contract test that runs against the real core —
packages/stack/__tests__/wasm-inline-core-credential-contract.test.ts. (Now 331 lines and 11 tests — see Review round 2, which parameterised it over both strategy keys and moved the packaging assertion out.)expect(calls.getToken).toBe(0)proved nothing on its own: a counter that is never incremented reads as zero whether the claim is true or the core simply never callsgetTokenduring construction. It does call it — structurally complete key material clears the key provider, after which the core callsgetTokenexactly once. So authentication is reached during construction, only after the key is loaded, and the counter is live. Still fully offline: the token never parses, so nothing leaves the process.browserexport condition on any subpath. Adding one to quiet a bundler would ship a workspace secret to the browser. (This assertion has since moved to__tests__/browser-export-condition.test.ts, in the default suite — see Review round 2.)A drift guard, and three corrections to overstated comments — the previous version claimed a rename of
opts.strategywould silently hollow out the file. That is false, and is now pinned rather than assumed: the core reads the strategy by name before deserialising credentials, so the other tests would fail loudly rather than pass vacuously.A changeset —
@cipherstash/stackpatch andstashpatch (the skills ship in thestashtarball).CI wiring for the contract test (
53d4b2a1). The test resolves the real@cipherstash/protect-ffi/wasm-inline— that is its whole point, since every other wasm suite gets a stub. That entry is wasm-pack output whichpnpm installdoes not produce, and theRun Testsjob builds the binding without it deliberately, so the file could not collect there and took Node 24 red. It now has its own vitest config, atest:wasm-coreturbo task, and one step inRun WASM E2E Tests (Deno)— the job that buildsdist/wasm/**and needs no database.scripts/__tests__/wasm-core-contract-ci.test.mjsholds the four pieces together, since losing any one of them leaves a green tree with the contract checked nowhere.Verification
pnpm exec turbo run test:wasm-core --filter @cipherstash/stack— 7 passed at the time (11 as of Review round 2), against the real@cipherstash/protect-ffiWASM build, no mocks. (That is the command now; the suite is no longer part of stack's defaultvitest run— see below. It needsdist/wasm/**, so build it first withpnpm --filter @cipherstash/protect-ffi run build:wasm.)53d4b2a1: theTest the WASM core credential contract (stack)step in theRun WASM E2E Tests (Deno)job — 7 passed. That is the first time the contract has been checked against the real core in CI at all.pnpm --filter @cipherstash/stack exec vitest run __tests__/wasm-inline-normalize.test.ts __tests__/wasm-inline-bundle-isolation.test.ts— 8 passed. These are the two adjacent wasm suites the test file's comments now describe correctly.The test was ported, not cherry-picked clean, and that is the part worth reviewing. It was written on 2026-07-28 against
@cipherstash/protect-ffi0.30. On currentmainit compiled and every credential assertion failed: 0.31 movedclientId/clientKeyunderclientOptsand rejects unknown top-level keys, so the calls were refused withunknown field 'clientId'before reaching anything the test meant to observe. Nesting the credentials fixed three of six. The other three asserted error strings the core no longer emits:missing field `clientKey`clientOpts.clientId and clientOpts.clientKey are required(one message names both; the core does not say which is absent)invalid clientKey: invalid hexinvalid clientKey: expected a hex-encoded keyopts.strategy is requiredNot authenticatedThe last one carried the file's ordering claim, so it was re-derived by probing the real core, not re-spelled:
Not authenticatedwins wheneverstrategyis absent, while omitting the credentials with a strategy gives the credential error. The strategy is still read first, which is what makes "even when an auth strategy is supplied" mean anything in the tests above it.The finding the file exists to pin is unchanged, and is now pinned against the shipping core: federation does not remove the
clientKeyrequirement.Related
Refs #804, #805.
This supersedes #810 — do not merge that one. #810 is a 100-commit draft whose base is 2,803 commits behind
main. The two documentation commits worth keeping were recovered onto currentmainhere, plus the port commit above. #810 can be closed once this lands.Review notes
Start with the port commit (
26bafc8e) — it is the only one whose content is new rather than recovered, and the error-string table above is the whole of what changed.This branch conflicts with #951 in
skills/stash-supabase/SKILL.md. Both edit the same paragraph: this one inserts a paragraph explaining why the browser is ruled out, immediately below the sentence that PR replaces to say the Worker is not. The two are complementary; whichever merges second should rebase and keep both.e2e/wasm/roundtrip.test.tscovers theaccessKeyarm, so the federation arm reasoned about here has no live end-to-end coverage anywhere. That is stated in the test file rather than fixed, and is not something this PR sets out to close.Review round 2 (
4ab7a82e,9038aa1d,56864394)The contract was pinned on the deprecated option key. The test supplied its auth strategy as
opts.strategy. That is protect-ffi's deprecated alias —NewClientOptionsmarks it "Renamed to authStrategy", andwasm.rsreadsauthStrategyfirst withstrategyonly as a fallback — whilesrc/wasm-inline.tsbuilds its call asauthStrategy: strategy. So "clientKey is required even when an auth strategy is supplied" was pinned for a field the shipping path never sets, and the day the alias goes away four tests turn red withNot authenticated, reading as a core regression rather than a rename. Both arms now run, named in their titles, andSTRATEGY_KEYSsays what to do when one of them dies.The
browserexport-condition assertion ran almost nowhere. It reads a manifest — no WASM build, no credentials — but it lived inside the contract file, whichvitest.config.tsexcludes. So it ran only inRun WASM E2E Tests: in no localpnpm test, and on a fork PR in nothing at all, since that job andRun Testsboth hard-fail atrequire-cs-secretsandLintruns only Biome. Adding abrowsercondition to quiet a bundler — the exact accident its comment names — passed everything a contributor runs. It now lives in each package's default suite,@cipherstash/stack-supabaseincluded: that package has the same./wasm-inlineexport and the same "not browser-safe (#804)" note, and nothing covered it.vitest.wasm-core.config.tsset notestTimeout, so it inherited Vitest's 5000ms while the sibling config deliberately raises it to 30000 for this package. Every case there instantiates the real inlined core and the last loads a key and callsgetToken— a flake waiting for a cold runner, in the one job that runs it.The redundancy argument, and the fact that answers it. A reviewer argued the separate config, turbo task and CI guard were unnecessary, since
integration/wasm/**already loads the real core andintegration-drizzle.ymlalready buildsdist/wasm/**withwasm: 'true'. Both halves are true. The argument still fails on one fact:packages/test-kit/src/integration/global-setup.tsrequirescipherstashanddatabaseunconditionally and throws rather than skipping, then runs a realstash eql install. Hosting the contract there would cost it credentials, a database and a built CLI.That fact lived in prose in three docblocks and nothing enforced it — and making global-setup skip when unconfigured is the obvious "let people run integration tests locally" change, which would make the redundancy argument correct while every docblock still said otherwise. One case now pins both halves: the unconditional requirement list, and that it is a throw and not a skip.
Three sentences were wrong and are corrected. The contract file said every suite reaching the core hands it a complete real credential; protect-ffi's
wasm-error-codesdoes not — it passes noclientOptsat all and misses #804 for a different reason (its cases fail in config validation, and none supplies an auth strategy). The guard docblock made the same "only one that needs nothing else" claim. The CI step comment claimed the integration job has PostgREST, whichintegration-dbdocuments as empty for thepostgresvariant, and said "most diffs" where the condition is every diff the path filter does not select. Andvitest.wasm-core.config.tsopened by calling this "the one suite in stack's DEFAULT test run" twenty lines above explaining it is excluded from that run.Skills and the changeset.
skills/stash-edgeis what an agent reads when deploying this entry and did not contain the word "browser" at all; it now carries the constraint at the head of its Credentials section. Thestash-encryptionentry-point row gains a pointer clause — it calls this "the edge entry" and lists V8 runtimes, which is the reading that put "modern browsers" in the example README. The changeset had claimed the constraint was stated in the auth-strategy re-export, thestash-encryptiontable andstash-edge; only the first of those was false about the code, and all three were false about the diff. It now names only what landed.Verification
pnpm run test:scripts— 827 passed.pnpm --filter @cipherstash/stack-supabase test— 562 passed, 4 skipped.browserguards confirmed red by injecting a nestedbrowsercondition intoexports["./wasm-inline"]; manifests restored clean.databaseconditional;packages/test-kit/restored clean.pnpm run code:check— zero errors.The contract file itself still cannot run without cargo, the wasm32 target and wasm-pack. It typechecks and transforms, and
wasm.rsresolves both option keys into the same code path with no key name in any of the six asserted error strings, so theauthStrategyarm should produce identical messages — theTest the WASM core credential contract (stack)step is where that is confirmed, now at 11 cases rather than 7.