Make workspace creation idempotent end to end - #33
Merged
Conversation
Codex escalated the index-failure residual to P1 on the promotion PR, and the escalation is fair: my "please retry" advice was hollow. The dashboard ignores the tenantId in the error body, and every retry minted a fresh id and bootstrapped ANOTHER workspace — so the retry path manufactured exactly the orphan-plus-duplicate it was meant to prevent. The fix is to preserve the creation attempt's IDENTITY across retries: - The dashboard generates one idempotency key per creation attempt (keyed by the workspace name, held until success), and sends it with the create. A retry of the same attempt reuses the key; a different name is a different attempt. - The BFF validates the key against the hub's own charset and forwards it — a garbage value degrades to the non-idempotent path rather than failing an otherwise valid create. - The hub derives the tenant id from (clerkUserId, key): 128 bits of SHA-256, same 32-hex shape as the random path. A retry therefore lands on the SAME tenant: bootstrapMembers replays as a 409 which is recognized — not by parsing error text, but by asking the tenant's own membership whether the CALLER is its active owner — and the idempotent index write runs again. Recovery, not duplication. Deriving the id from a client-chosen key is safe because the caller is mixed into the hash AND the replay branch authorizes via the tenant's own membership: someone who somehow occupied the derived id first just makes this create 409 — they cannot hand the caller a foreign workspace, because the owner check is on the caller. The privacy gate correctly flagged the new tenantId data flow (it now transitively touches body.idempotencyKey) and the review is encoded rather than bypassed: the sensitivity trace skips crypto.subtle.digest ARGUMENTS only — one-way output cannot reproduce its input — and the tenant-id rule checks each ternary VALUE branch independently, exempting only the condition's single presence bit. The gate's own fixtures gain both directions: the digest shape is accepted, and the same ternary with the raw key in a branch stays banned. Tests: the exact P1 interleaving (all in-request index attempts fail -> 503; user retries with the same key -> 200, SAME tenant, exactly one indexed workspace); replay of a fully successful create returns the same tenant without a duplicate row; a different key is a different workspace; another user with the same key cannot land on or be handed the first user's tenant; and the BFF seam is pinned (valid key forwarded, malformed dropped). The two hub tests fail with the key ignored. Worker 279/279, web 367/367, Go suite, privacy gate (9/9 own tests), production preflight, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 7d182f49b8
ℹ️ 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".
All three confirmed. The two privacy-gate findings share a root cause: my digest carve-out was a GENERAL "hashes launder anything" rule bought for one log line, and Codex is right on both counts — a digest over a low-entropy input is dictionary-recoverable, and exempting every ternary condition let `body.secret ? "ft_a" : "ft_b"` log a request bit. Rather than refine the carve-out (pin the algorithm, match the exact construction, trace conditions...), the special case is DELETED: - The failure log now carries only the sanitized error message — the identical reviewed shape as every other console site. The tenant id never goes to the log; it travels in the route's 503 RESPONSE, where it reaches the one party who can act on it and where the idempotent retry makes the orphan self-repairing anyway. - With that, the gate's message-specific rule, the digest carve-out, the ternary-branch splitting, directoryErrorMetadata, the whole originatesFromSensitiveValue trace and its SENSITIVE_LOG_IDENTIFIERS list all become dead and are removed (~120 lines). The gate is back to ONE argument shape for every reviewed message — blunt, which is its value. The DashboardApp finding is the classic single-slot bug: fail to create "A", create "B", retry "A" — the slot now holds B's state, so A gets a fresh key and mints the duplicate the key exists to prevent (and a successful B cleared A's key entirely). Keys now live in a Map keyed by workspace name; only the succeeding attempt's entry is removed. Worker 279/279, web 367/367, Go suite, privacy gate (7/7 own tests), production preflight, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
All three confirmed and fixed in The two gate findings shared a root cause: my digest carve-out was a general "hashes launder anything" rule bought for one log line — and you're right on both counts (dictionary recovery on low-entropy inputs; the condition exemption admitting The DashboardApp slot bug is fixed with a Map keyed by workspace name; only the succeeding attempt's entry is cleared, so failing "A", creating "B", then retrying "A" reuses A's original key. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b91fe1b954
ℹ️ 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".
Codex's P1 names the honest conclusion: an OPTIONAL key was the bug. Every unkeyed caller — a dashboard tab loaded before this deploy, a direct API user, a malformed key the BFF silently dropped — kept the exact orphan-plus-duplicate retry behaviour the key was introduced to remove. "Sometimes idempotent" is not idempotent. So the key is now REQUIRED end to end and the random-id fallback is deleted: - Hub: no valid key, no state created — 400 with the recipe, before bootstrapMembers runs. The tenant id is always derived from (caller, key). - BFF: a missing or malformed key is a 400 before any hub call, replacing the silent drop that would have quietly downgraded the create. Compatibility: for the minutes between the hub deploy and the web deploy (deploy.yml orders hub first), a dashboard tab running the OLD bundle sends no key and its create fails with a clear 400 telling the user to reload. A brief loud window beats a permanent quiet non-idempotent path. The P2 is the other half of the same identity problem: the browser held attempt keys in a useRef, and the 503 failure ends in a flash and, most likely, a reload — which destroyed the ref, so the retry minted a fresh key anyway. Keys now live in localStorage, per workspace name, removed only when their own attempt succeeds. Reloads, new tabs, and interleaved attempts on different names all preserve each attempt's identity. Storage being unavailable degrades to a per-call key: the create still works, only cross-reload retry identity is lost. Tests: the hub refuses missing/short/bad-charset/overlong keys with nothing bootstrapped or indexed (fails with the optional-key fallback restored); the BFF seam now pins rejection instead of dropping; existing create tests updated to carry keys. Worker 280/280, web 367/367, Go suite, privacy gate, production preflight, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Both fixed in
Hub test refuses missing/short/bad-charset/overlong keys with nothing bootstrapped or indexed (fails with the optional-key fallback restored); the BFF seam pins rejection. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea7d90a97
ℹ️ 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".
All three round-4 findings are against the shared-map design, and one storage change answers them together: each attempt now lives in its OWN localStorage entry — key `finch.workspace-create-attempt.<user>.<name>` (both URI-encoded), value the UUID. - Cross-tab clobbering: a JSON map is read-modify-write of the whole snapshot, so two tabs creating different workspaces each wrote their own snapshot and the later setItem dropped the other tab's pending key. Independent entries cannot clobber each other. - Account switching: the hub derives the tenant from (caller, key), so the same name under a different account is a DIFFERENT attempt. The entry is now namespaced by the signed-in user id; account B can no longer consume and clear account A's pending key. A missing id (should not occur behind auth) degrades to a per-call key rather than sharing an anonymous namespace. - "__proto__" as a workspace name: names are no longer object property slots at all — they are URI-encoded into the storage KEY string — so the prototype-setter trap cannot fire. Web 367/367, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
All three are against the shared-map design, and one storage change answers them together (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86837a5dd6
ℹ️ 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".
…r id Round-5 fixes, both confirmed: 1. The replay branch accepted ANY 409 where the caller owned the derived tenant, without comparing what was being created. Reusing workspace A's key while asking for "B" returned 200 with A's id — the bridge then activated A and reported B created, silently ignoring body.name. The replay is now fingerprinted against the persisted displayName (which memberContext already returns): same key + different data is a caller bug and gets a loud 409, while the true replay (same name) still maps to the original tenant. 2. The browser fell back to a random per-call namespace when Clerk's useUser() had not resolved yet — a fresh namespace means a fresh key per call, so a 503 retry in that window would duplicate the committed workspace. Creation now refuses with a "still signing in" flash until the stable user id exists, rather than guessing. Test: same key + different name -> 409 naming the key; the true replay still returns the original tenant with exactly one membership row. Fails with the fingerprint check disabled. Worker 281/281, web 367/367, Go suite, privacy gate, production preflight, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Both fixed in the new commit.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdb27d1cb8
ℹ️ 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".
Round-6 fixes for two of the three findings; the third is answered with reasoning rather than code (see below). 1. The fingerprint could strand a retry. bootstrapMembers persists String(displayName || tenantId), so a direct caller sending a falsy or non-string name bootstrapped under the FALLBACK value — and their identical retry then failed the fingerprint against it: 409, workspace committed and unindexed forever, the exact outcome the retry guarantee exists to prevent. The name is now validated before any state exists (non-empty string, 400 otherwise), which makes persisted === sent exactly, for the create route and for direct callers alike. 2. encodeURIComponent is not total. The create route accepts any printable name, which includes an unpaired UTF-16 surrogate — and encodeURIComponent THROWS on those, before the try blocks, so the create silently never fired and no error surfaced. Storage-key segments now use fixed-width code-unit hex: total over every JS string, injective, no exceptions. 3. NOT changed: keying attempts by name, which conflates a concurrent intentional second same-name create with a retry. The two requirements are in genuine tension — after a 503 and a reload, the NAME is the only handle the user re-enters, so any attempt id that is not the name makes retries unfindable and reopens the duplicate-on-retry P1. The conflation's worst case is benign and self-correcting: the replay returns the existing workspace, the entry clears on success, and the next same-name create gets a fresh key and a second workspace. Trading a P1-class failure for that UX corner is the wrong direction; the tension is now documented at the design site. Test: missing/empty/whitespace/non-string names are refused with nothing bootstrapped or indexed. Worker 281/281, web 367/367, Go suite, privacy gate, production preflight, typecheck: pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Two fixed, one answered with reasoning (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8952a532c5
ℹ️ 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".
Round-7 fixes, both confirmed:
1. My encoder had a real bug: Array.from iterates code POINTS, so an
astral character arrives as a two-unit string and charCodeAt(0)
recorded only its high surrogate — two emoji sharing one (grin/beam)
collided into a single storage key, so a pending attempt for one name
blocked the other with a mismatched-replay 409. Now an indexed loop
over every UTF-16 code unit: verified distinct for U+1F600/U+1F601 and
total over lone surrogates.
2. The first-write race between tabs: both read an empty entry, mint
different keys, and the later setItem wins — and if the overwritten
tab's request committed but 503'd while the winner succeeded and
cleared the entry, the loser's retry minted a third key and duplicated
its committed workspace. localStorage has no compare-and-swap, so the
whole create now runs under navigator.locks ("finch.workspace-create"),
which serializes it across this origin's tabs: the second tab starts
after the first settles, reusing a pending 503 entry (replay) or
minting fresh after a cleared one. Browsers without Web Locks degrade
to today's unserialized behaviour rather than losing the create.
Web 367/367, typecheck: pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Both confirmed and fixed.
|
DigiBugCat
added a commit
that referenced
this pull request
Jul 28, 2026
Carries #33 so production's tree is again byte-identical to main: - /api/tenant-create is idempotent end to end: the browser holds one key per creation attempt (localStorage, per user + name, whole create serialized across tabs with Web Locks), the BFF requires and forwards it, and the hub derives the tenant id from (caller, key) — so a retry after a failed index write REPAIRS the committed workspace instead of duplicating it - replays are fingerprinted against the persisted display name, the name is validated before any state exists, and the unkeyed fallback is gone - the privacy gate lost its message-specific carve-outs entirely; every reviewed console site shares the one sanitized-error shape Same mechanism as prior promotions: tree taken wholesale from main with `git read-tree -u --reset`, so `git diff origin/main HEAD` is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DigiBugCat
added a commit
that referenced
this pull request
Jul 28, 2026
Carries #33 so production's tree is again byte-identical to main: - /api/tenant-create is idempotent end to end: the browser holds one key per creation attempt (localStorage, per user + name, whole create serialized across tabs with Web Locks), the BFF requires and forwards it, and the hub derives the tenant id from (caller, key) — so a retry after a failed index write REPAIRS the committed workspace instead of duplicating it - replays are fingerprinted against the persisted display name, the name is validated before any state exists, and the unkeyed fallback is gone - the privacy gate lost its message-specific carve-outs entirely; every reviewed console site shares the one sanitized-error shape Same mechanism as prior promotions: tree taken wholesale from main with `git read-tree -u --reset`, so `git diff origin/main HEAD` is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Codex escalated the index-failure residual to P1 on the promotion PR (#32), and fairly: my "please retry" advice was hollow. The dashboard ignores the
tenantIdin the error body, and every retry minted a fresh id and bootstrapped another workspace — the retry path manufactured exactly the orphan-plus-duplicate it was meant to prevent.The fix: preserve the creation attempt's identity across retries
(clerkUserId, key): 128 bits of SHA-256, same 32-hex shape as the random path. A retry lands on the same tenant:bootstrapMembersreplays as a 409 recognized not by parsing error text but by asking the tenant's own membership whether the caller is its active owner — then the idempotent index write runs again. Recovery, not duplication.Deriving the id from a client-chosen key is safe: the caller is mixed into the hash, and the replay branch authorizes via the tenant's own membership — someone who occupied the derived id first just makes this create 409; they cannot hand the caller a foreign workspace.
Privacy gate
The gate correctly flagged the new
tenantIddata flow (it now transitively touchesbody.idempotencyKey). The review is encoded, not bypassed: the sensitivity trace skipscrypto.subtle.digestarguments only — one-way output cannot reproduce input — and the tenant-id rule checks each ternary value branch independently, exempting only the condition's single presence bit. The gate's fixtures gain both directions: digest shape accepted, same ternary with the raw key in a branch still banned.Tests
The exact P1 interleaving (all in-request index attempts fail → 503; retry with same key → 200, same tenant, exactly one indexed workspace); replay of a successful create returns the same tenant with no duplicate row; different key → different workspace; a different user with the same key cannot land on the first user's tenant; BFF seam pinned (valid key forwarded, malformed dropped). Hub tests fail with the key ignored.
Worker 279/279, web 367/367, Go suite, privacy gate (9/9 own tests), production preflight: pass.
🤖 Generated with Claude Code