Conversation
Add migration 011 and CoreGenesisRepository fields for SVG logo text and profile image blob plus MIME. Genesis complete accepts optional root_organization_logo_svg and bootstrap_operator_profile_image_*; validates sizes and formats. Expose GET /genesis/branding for read-back. Frontend: ShellOrgLogo and useGenesisBranding, wizard sends assets on complete, Settings prefers Core profile when available. Tests and Ruff clean. Made-with: Cursor
![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: c64eccb359
ℹ️ 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".
| if (!file.type.startsWith("image/")) { | ||
| setOperatorAvatarError("Choose an image file."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
onOperatorProfileImageChange accepts any image/*, but POST /genesis/complete only accepts image/png, image/jpeg, image/webp, and image/gif (validated server-side in genesis completion). This creates a user-visible dead end: selecting SVG/BMP/TIFF passes step 3 and enables completion, then fails at step 5 with an INVALID_REQUEST from Core. Validate against the same MIME allowlist here (or narrow accept) so unsupported files are rejected before challenge/complete flow.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** Core rejects ``.`` in root org name (single segment only). */ | ||
| const rootOrgNameHasDot = networkLabel.includes("."); | ||
| const step3BothFilled = | ||
| operatorPubkeyHex.trim().length > 0 && operatorDisplayName.trim().length > 0; | ||
| operatorPubkeyHex.trim().length > 0 && | ||
| operatorDisplayName.trim().length > 0 && | ||
| Boolean(settings.profileAvatarDataUrl?.trim()); | ||
|
|
||
| function goBack() { | ||
| setCurrentStep((s) => Math.max(1, s - 1)); |
There was a problem hiding this comment.
🔴 Genesis step 3 reads and writes the shared settings.profileAvatarDataUrl (localStorage) instead of its own local state, causing two cross-component side effects: (1) if the user already has a SettingsPanel profile image, step3BothFilled evaluates to true immediately and silently submits that pre-existing image to Core as the bootstrap operator profile without the user making an explicit genesis-context choice; (2) clicking Remove picture in step 3 calls clearOperatorProfileImage() which wipes settings.profileAvatarDataUrl, permanently deleting the user's SettingsPanel profile picture with no warning. The wizard should maintain its own useState for the operator profile image rather than reading from or mutating global app settings.
Extended reasoning...
What the bug is: GenesisNoticeModal step 3 reads and writes settings.profileAvatarDataUrl — the same localStorage-persisted value that SettingsPanel uses for the local profile picture. Two distinct defects flow from this shared state.
Side effect 1 — silent pre-fill on genesis: step3BothFilled (which gates the Next button on step 3) is computed as:
const step3BothFilled =
operatorPubkeyHex.trim().length > 0 &&
operatorDisplayName.trim().length > 0 &&
Boolean(settings.profileAvatarDataUrl?.trim());If the user previously uploaded a profile picture in SettingsPanel, settings.profileAvatarDataUrl is already non-empty when the genesis wizard opens. The avatar circle in step 3 renders that old image, and the Next gate is satisfied immediately. When the user clicks Complete on step 5, handleGenesisComplete reads settings.profileAvatarDataUrl and posts it to Core as bootstrap_operator_profile_image_base64. The casual settings avatar is silently submitted as the bootstrap operator profile — an asset that persists in Core and is served via GET /genesis/branding.
Step-by-step proof:
- User visits SettingsPanel, uploads selfie.jpg → settings.profileAvatarDataUrl is set in localStorage.
- User opens the genesis wizard (Core not yet initialized).
- On step 3, the profile circle shows selfie.jpg with no explanation that it came from Settings.
- The Next button is immediately enabled (step3BothFilled is true as long as pubkey + username are also filled).
- User completes genesis without touching the avatar.
- handleGenesisComplete calls dataUrlToBase64Payload(settings.profileAvatarDataUrl) and posts the selfie to POST /genesis/complete.
- Core stores selfie.jpg as the bootstrap operator profile image permanently.
Side effect 2 — silent data destruction: clearOperatorProfileImage calls:
setSettings((s) => ({ ...s, profileAvatarDataUrl: '' }));This writes the empty string to localStorage via the app settings persistence layer. Concrete scenario: user with an existing SettingsPanel profile picture opens genesis, reaches step 3, clicks Remove picture (perhaps to swap to a different image for genesis), and their SettingsPanel profile picture is permanently erased — with no warning, no undo, and no indication that this action affects Settings.
Why existing code does not prevent it: Both components share the same settings object from useAppUi(). There is no isolation layer. onOperatorProfileImageChange also calls setSettings((s) => ({ ...s, profileAvatarDataUrl: data })), so uploading a genesis avatar also overwrites whatever the user had in SettingsPanel.
Impact: Any user with a pre-existing SettingsPanel avatar who runs genesis will silently submit that image to Core without explicit consent. Anyone who uses Remove picture in step 3 loses their SettingsPanel avatar permanently. The Remove action in a first-boot wizard having undisclosed scope over a separate, persistent settings panel is a genuine UX defect.
Fix: Introduce a local useState<string | null> (e.g. genesisOperatorAvatarDataUrl) inside GenesisNoticeModal. Wire onOperatorProfileImageChange and clearOperatorProfileImage to that local state. Update step3BothFilled and handleGenesisComplete to use the local state. The global settings.profileAvatarDataUrl should not be read or written by the genesis wizard.
| bootstrap_operator_profile_image_mime: profileParts?.mime, | ||
| }); | ||
| onGenesisCompleteSuccess?.(); | ||
| } catch (e: unknown) { | ||
| setGenesisCompleteError(formatClientError(e)); | ||
| } finally { | ||
| setGenesisCompleteLoading(false); | ||
| } | ||
| } | ||
|
|
||
| const backDisabled = currentStep <= 1 || genesisCompleteLoading; | ||
| const primaryFooterDisabled = | ||
| genesisCompleteLoading || | ||
| (currentStep === 2 && rootOrgNameHasDot) || | ||
| (currentStep === 3 && !step3BothFilled) || | ||
| (currentStep === 4 && !genesisChallengeVerified); | ||
| (currentStep === 4 && !genesisChallengeVerified) || | ||
| (currentStep === 5 && | ||
| (genesisOpsBlocked || | ||
| !genesisChallengeId || | ||
| !isEd25519PubkeyHex(operatorPubkeyHex) || | ||
| !isEd25519PubkeyHex(orgSigningPubkeyHex) || | ||
| !networkLabel.trim() || | ||
| rootOrgNameHasDot)); | ||
|
|
||
| return ( | ||
| <div |
There was a problem hiding this comment.
🔴 The Complete button in the genesis wizard re-enables for the duration of the GET /version round-trip after a successful POST /genesis/complete. A user who clicks it again receives a confusing 'genesis already complete' error even though genesis did succeed — fix by tracking a permanent genesisCompleteSuccess flag that keeps the button disabled after the call returns.
Extended reasoning...
What the bug is: In handleGenesisComplete(), the finally block unconditionally calls setGenesisCompleteLoading(false). This runs immediately after postGenesisComplete() resolves — regardless of success or failure. On success, onGenesisCompleteSuccess?.() is called first, which invokes refetchCoreVersion(). That function only synchronously increments the refreshTick state counter; the actual GET /version HTTP request fires later inside a useEffect.
The code path: React 18 batches the setRefreshTick(n+1) and setGenesisCompleteLoading(false) updates together into one render. On that render, genesisCompleteLoading is false and refreshTick is incremented. The useEffect for useCoreVersion fires after the paint, so coreVersion still has kind:'ok' with genesisComplete:false during this render. Because showGenesisNotice depends on coreVersion.genesisComplete being false, the modal remains open. Meanwhile primaryFooterDisabled evaluates to false: genesisCompleteLoading is false, all step-5 prerequisites (genesisChallengeId set, both pubkeys valid, networkLabel set, genesisOpsBlocked false) are still satisfied. The Complete button is re-enabled.
Why existing code does not prevent it: The only guard on the Complete button is genesisCompleteLoading, which is reset to false in the finally block. There is no persistent success state that keeps the button disabled after success. The modal closure depends entirely on an async HTTP response updating coreVersion.
Impact: The window lasts from the re-render until GET /version completes. On a local dev server this might be 10-100ms, but on a slower network or under load it could be several seconds. A user who notices the button become active and clicks it triggers another POST /genesis/complete, which returns a 'genesis already complete' error displayed in the UI. This is confusing because genesis did succeed; the error makes it appear something went wrong. This occurs in a critical first-boot flow where operator confidence matters.
Concrete proof: (1) User completes all 5 wizard steps and clicks Complete. (2) postGenesisComplete() succeeds. (3) React renders with genesisCompleteLoading=false, refreshTick incremented, but coreVersion still has genesisComplete:false. (4) Modal is still visible; Complete button is re-enabled since all step-5 conditions still pass. (5) User notices the re-enabled button and clicks it. (6) A second POST /genesis/complete fires; Core returns genesis already complete. (7) setGenesisCompleteError() is called and the error is shown in the UI. (8) GET /version eventually resolves, modal closes. The user is left confused about whether genesis actually succeeded.
Fix: Add a genesisCompleteSuccess state boolean (default false). Set it to true in the success branch before the finally block resets genesisCompleteLoading. Include it in primaryFooterDisabled so the button stays permanently disabled: genesisCompleteSuccess || genesisCompleteLoading || (rest of conditions). Optionally show a brief inline success message such as 'Genesis complete! Closing...' while waiting for the modal to disappear.
| return URL.createObjectURL(file); | ||
| }); | ||
| setNetworkLogoFileName(file.name); | ||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| const t = reader.result; | ||
| if (typeof t === "string") { | ||
| setNetworkLogoSvgText(t); | ||
| } | ||
| }; | ||
| reader.readAsText(file); | ||
| } | ||
|
|
||
| function clearNetworkLogo() { | ||
| setNetworkLogoObjectUrl((prev) => { | ||
| if (prev) URL.revokeObjectURL(prev); | ||
| return null; | ||
| }); | ||
| setNetworkLogoSvgText(null); | ||
| setNetworkLogoFileName(null); | ||
| if (logoFileInputRef.current) logoFileInputRef.current.value = ""; | ||
| } |
There was a problem hiding this comment.
🟡 onNetworkLogoFileChange() is missing e.target.value = "" after accepting a valid SVG, so re-selecting the same filename never fires onChange and a modified SVG is silently ignored. The async FileReader also lacks a stale-result guard: if two files are selected in rapid succession the readers can complete out of order, causing networkLogoSvgText to contain file A's markup while networkLogoObjectUrl shows file B's preview, submitting mismatched data to POST /genesis/complete. Both sibling handlers added in this PR (onOperatorProfileImageChange and SettingsPanel.onProfileImageChange) reset e.target.value unconditionally on their second line — this handler is missing that pattern.
Extended reasoning...
Issue 1 — Missing input reset after valid SVG accepted
onNetworkLogoFileChange() calls e.target.value = '' only in the rejection branch (when the file type is not SVG). After successfully accepting a file it never resets the input. This means the browser retains the selected path in the input element. If a user selects logo.svg, edits it on disk, and tries to select it again, the browser sees the same path and filename and does not fire the onChange event — the stale version remains in networkLogoSvgText and networkLogoObjectUrl without any indication to the user.
Compare the two sibling handlers introduced in this same PR: onOperatorProfileImageChange (GenesisNoticeModal.tsx line 285: e.target.value = '') and SettingsPanel.onProfileImageChange (SettingsPanel.tsx line 71: e.target.value = '') both reset unconditionally on their second line, before any other logic. onNetworkLogoFileChange should follow the same pattern — add e.target.value = '' immediately after extracting the file. Note that clearNetworkLogo() correctly calls logoFileInputRef.current.value = '', but this only helps if the user explicitly clicks 'Remove logo' first.
Issue 2 — Unguarded async FileReader (race condition)
Each call to onNetworkLogoFileChange() creates a new FileReader and starts readAsText(file) without cancelling or guarding against an earlier in-flight reader. networkLogoObjectUrl and networkLogoFileName are set synchronously (reflecting the latest file), but networkLogoSvgText is populated asynchronously when the reader's onload fires. If the user selects file A and then selects file B before reader A's onload fires, both readers are in flight simultaneously. Reader A could fire after reader B and overwrite networkLogoSvgText with A's content, while networkLogoObjectUrl and networkLogoFileName already point to B. handleGenesisComplete() would then send B's visual preview but A's SVG markup to POST /genesis/complete.
Addressing the refutation
One verifier argues the race is implausible because FileReader.readAsText on a local file completes in sub-millisecond time, and the OS file picker is a blocking modal that naturally serializes interactions. This is largely correct in practice — the window requires two separate file picker interactions to complete before the first reader fires, which is effectively impossible in normal browser usage. The refutation does not dispute that the code pattern is incorrect, only that the timing constraint is very narrow. The missing e.target.value reset (issue 1) is fully independent of the race and is a concrete usability defect regardless.
Step-by-step proof of issue 1
- User is on wizard step 2 and opens the logo file picker.
- User selects ~/logos/brand.svg — onChange fires, SVG is read, preview appears correctly.
- User opens an editor, modifies ~/logos/brand.svg, and saves it.
- User clicks the file input again and selects the same ~/logos/brand.svg.
- The browser sees the input value is already 'brand.svg' — onChange does NOT fire.
- networkLogoSvgText still holds the original pre-edit SVG content.
- User clicks Complete — the stale SVG is submitted to POST /genesis/complete without any error or warning.
Fix
Add e.target.value = '' immediately after const file = e.target.files?.[0]; (before the type check), matching the pattern in onOperatorProfileImageChange. For the race condition, capture the file reference in a local variable and check inside onload whether it is still the current file, or use a cancelled flag to discard stale reader results.
| ### Design notes (`docs/`) | ||
|
|
||
| | Document | Topic | | ||
| | -------- | ----- | | ||
| | [`docs/organizations_and_envelope_encryption.md`](docs/organizations_and_envelope_encryption.md) | Organizations, DEKs, envelope encryption vs Modulr.Storage | | ||
| | [`docs/genesis_and_session_roadmap.md`](docs/genesis_and_session_roadmap.md) | Finishing genesis (`/genesis/complete`), Keymaster-style shell sign-in, session tiers | | ||
| | [`docs/partner_methods.md`](docs/partner_methods.md) | Partner-visible methods between orgs (idea; not implemented) | | ||
| | [`docs/implementation_plan_profile_and_org_images.md`](docs/implementation_plan_profile_and_org_images.md) | Genesis complete, Keymaster sign-in, local vs server avatars | | ||
|
|
There was a problem hiding this comment.
🟡 The PR adds a 'Design notes' table to README.md with four links pointing to files under docs/ (e.g. docs/organizations_and_envelope_encryption.md), but the docs/ directory does not exist in the repository. All four links are broken 404s from GitHub or any markdown viewer.
Extended reasoning...
The PR inserts a new 'Design notes' section in README.md (lines 188-196) containing a markdown table with four document links, all under a docs/ subdirectory. However, the docs/ directory does not exist anywhere in this repository. Only README.md, LICENSE.md, frontend/README.md, and keymaster/README.md are present at the root or nested levels — there is no docs/ folder.
The four broken links are:
- docs/organizations_and_envelope_encryption.md
- docs/genesis_and_session_roadmap.md
- docs/partner_methods.md
- docs/implementation_plan_profile_and_org_images.md
When any reader clicks these links from GitHub's rendered README view, they will receive a 404 file-not-found error. The same applies in any local markdown viewer that resolves relative paths.
The existing code does nothing to prevent this: the README is a static markdown file and there is no tooling or CI check that validates internal markdown links in this repository, so the broken links pass through undetected.
The impact is purely documentation-level — no runtime behavior is affected. However, these links are intended to explain the design decisions behind the very features added in this PR (genesis complete, branding storage, etc.), so readers who want to understand the design rationale will hit dead ends immediately.
To fix: either create the four referenced .md files under a new docs/ directory before merging, or remove the table from the README until the documents exist. A third option is to inline the descriptions as plain text without links.
The README design table linked to docs/ but the directory was listed in .gitignore, so nothing under docs/ was committed. Remove the ignore rule and add design notes: README-linked orgs/envelope, genesis roadmap, partner methods, profile/org images, plus other local spec drafts now included. Made-with: Cursor
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
1 participant
Add migration 011 and CoreGenesisRepository fields for SVG logo text and profile image blob plus MIME. Genesis complete accepts optional root_organization_logo_svg and bootstrap_operator_profile_image_*; validates sizes and formats. Expose GET /genesis/branding for read-back. Frontend: ShellOrgLogo and useGenesisBranding, wizard sends assets on complete, Settings prefers Core profile when available. Tests and Ruff clean.