Skip to content

feat(coolify): set up a Coolify server over SSH - #4326

Open
RyanGroch wants to merge 41 commits into
dyad-sh:mainfrom
RyanGroch:coolify-server-setup
Open

feat(coolify): set up a Coolify server over SSH#4326
RyanGroch wants to merge 41 commits into
dyad-sh:mainfrom
RyanGroch:coolify-server-setup

Conversation

@RyanGroch

@RyanGroch RyanGroch commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Dyad can already deploy to an existing Coolify instance. This adds the step before it: pointing Dyad at a bare Linux server and getting a working, signed-in Coolify onto it.

The user provides an address, an email, and optionally a domain they own. Dyad shows a public key to install on the server, then connects, checks the machine, runs Coolify's installer, waits for the dashboard, ensures an admin account exists, tries to put the instance on HTTPS, and mints an API token for the existing deploy flow. A failure reports what the server said rather than an exit code.

Without a domain, HTTPS goes through sslip.io. With one, Dyad checks it resolves to the server before applying it, since Coolify will not issue a certificate for a name that does not point at it. An address that cannot have a certificate at all — loopback, private, or IPv6 — finishes on plain HTTP and says so. A Coolify too old to mint a token finishes too, handing over the sign-in details instead.

Several setup steps drive Coolify's internals rather than a supported interface, because no supported interface exists. Coolify has no way to enable API access, mint a token, create or find the first user, set the instance domain, or state its version before its API is reachable — so each of those runs a short PHP script through php artisan tinker in the Coolify container. This is the least durable part of the PR: it depends on model and config names that Coolify is free to change. Every one of these call sites is marked WORKAROUND with a TODO naming what an official API would replace, and the hope is to delete them as Coolify grows real support.

The setup runs as a state machine in the main process, per rules/state-machines.md, so an install survives leaving the panel. Covered by unit tests, integration tests driving the real flow against a real ssh2 server, and two Playwright tests.

This PR adds ssh2 (^1.17.0) as a runtime dependency of the desktop app, along with @types/ssh2 as a dev dependency. It is the only new runtime dependency, and it holds the private key and sees the admin password, so it is worth a deliberate look.

Why a library rather than shelling out to ssh:

  • No assumption that an ssh binary exists, is on PATH, and behaves the same on Windows, macOS and Linux.
  • The private key stays in memory. Shelling out means writing it to a temp file with the right permissions and removing it on every failure path.
  • Failures arrive as values. Telling an auth rejection from an unreachable host by parsing stderr breaks the first time the wording changes.
  • Host key verification happens in process, before any credential is sent.
  • Commands stream output, end with an exit status, and can be aborted, with no PTY to scrape.
  • Scripts go over stdin, so there is no shell quoting layer to get wrong.

On supply chain:

  • ssh2 is long established, pure JavaScript at its core, with two small runtime dependencies (asn1, bcrypt-pbkdf). Its native pieces (cpu-features, nan) are optional and installs proceed without them.
  • package-lock.json pins 1.17.0 with a sha512 integrity hash, and CI installs from the lockfile. The caret matters only on a deliberate update.
  • Releases are infrequent — 1.15.0 in December 2023, 1.16.0 in September 2024, 1.17.0 in August 2025 — so there is little pressure to move off the pin.

That is not a guarantee. If the dependency ever has to go, every SSH call goes through src/ipc/utils/ssh_client.ts behind connectSsh, run and end, so reimplementing it over the system ssh binary would not touch the flow, the state machine, or the UI.

Not included: IPv6 addresses install but get no certificate; registering further servers from inside Dyad; setting a wildcard domain on the server, so deployed apps get names under it instead of sslip.io addresses — Dyad already reads one when Coolify has it configured.

Review in cubic

@socket-security

socket-security Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​ssh2@​1.15.51001007680100
Addedssh2@​1.17.09310010080100

View full report

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c12a10a27

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/shared/coolify_admin_email.ts Outdated
Comment thread src/coolify_setup/install.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Reviewed all 61 changed files. The combined diff blob in the context is marked truncated, but every individual file patch is complete (patchTruncated: false throughout), so the review is based on the full per-file diffs.

This is a large, carefully-built feature. The main/renderer boundary is respected (only the public half of the SSH key ever crosses, the private key and generated password stay in main), IPC inputs go through zod contracts, new channels are added to both VALID_INVOKE_CHANNELS and VALID_RECEIVE_CHANNELS with a test that enforces it, main-process errors consistently use DyadError with sensible DyadErrorKinds, renderer data fetching uses TanStack Query, telemetry is correctly extended to suppress self-hosted exceptions, and there are no schema changes so no migration is owed. Shell-injection surfaces (buildInstallCommand, envArgs, applyInstanceDomain) reject unsafe characters rather than escaping them, which is the right call. The state machine follows rules/state-machines.md and correlates every event by invocation ref.

The findings below are all MEDIUM — none block merge.

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/coolify_setup_handlers.ts:59 Install accepts any host key and never pins the inspected fingerprint
🟡 MEDIUM src/coolify_setup/controller.ts:140 Commands from a stay transition are silently dropped
🟡 MEDIUM src/ipc/handlers/coolify_setup_handlers.ts:222 customDomain is not validated before a multi-minute install starts
🟢 Low Priority Notes (8 items)
  • Unhandled clipboard rejection - navigator.clipboard.writeText is awaited inside an async onClick with no catch. If the write is refused, the button does nothing, no error is surfaced, and the promise rejection is unhandled. The 2s setTimeout that resets copied is also never cleared on unmount. (src/components/CoolifyServerSetup.tsx, src/components/CoolifyCredentials.tsx)
  • Connector does not subscribe to setup events - CoolifyConnector reads queryKeys.coolify.setup to decide whether to show the installer, but only CoolifyServerSetup registers ipc.events.coolifySetup.onChanged. In a window where the setup component is not mounted (second window, or the paste-a-token screen), a run that starts or finishes elsewhere is not reflected until the query happens to refetch. (src/components/CoolifyConnector.tsx)
  • Empty form flashes before the snapshot resolves - setup falls back to {type:"idle"} while snapshot is pending, so reopening the panel during a running install briefly shows the blank form with Install enabled; pressing it produces a "server is already being set up" error instead of the running view. A pending state would read better. (src/components/CoolifyServerSetup.tsx)
  • Credentials are fetched eagerly - The revealCredentials contract comment says secrets cross to the renderer "only when someone asks to see them", but CoolifyCredentials mounts unconditionally inside coolifySection and the installer screen, so the admin password and API token are pulled into the renderer on every panel render (masked, but present). The component also renders null on query error, so a failure to read them is invisible. (src/components/CoolifyCredentials.tsx)
  • Cancel during HTTPS leaves the fqdn set - tryEnableHttps rethrows UserCancelled from the certificate poll before reaching the applyInstanceDomain(session, null) rollback, so a cancel at that moment leaves Coolify pointed at a domain with no certificate. Impact is limited because port 8000 keeps serving, but the revert is skipped on exactly the path that documents it. (src/coolify_setup/https_setup.ts)
  • Poll sleeps ignore the abort signal - waitForDashboard, waitForAdminSeeded and tryEnableHttps all sleep with a bare setTimeout and only check signal.aborted at the top of the loop, so Cancel can take up to one full interval (5-6s) to be noticed even though the signal is already set. (src/coolify_setup/install.ts, src/coolify_setup/https_setup.ts)
  • Unit test imports from the Playwright tree - setup_flow.integration.test.ts imports startFakeSshServer from ../../e2e-tests/helpers/fake_ssh_server, coupling the vitest suite to the e2e directory. Moving the fake under testing/ would keep the two trees independent. (src/coolify_setup/setup_flow.integration.test.ts)
  • tinker transcript parsing is verified only against Dyad's own fake - extractOutput anchors on > __DYAD_OUT_START__, and the only thing producing that shape in CI is e2e-tests/helpers/fake_ssh_server.ts, which was written from the same assumption. A psysh version that prompts differently would break every setup step at once with a generic "Coolify did not answer as expected". The PR already flags this class of risk; a note in the WORKAROUND comment naming the psysh version this was observed against would help whoever debugs it. (src/coolify_setup/tinker.ts)

Generated by Dyadbot persona-based code review

@github-actions github-actions Bot added the needs-human:review-issue ai agent flagged an issue that requires human review label Aug 20, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 61 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/coolify_setup/install.ts Outdated
Comment thread src/coolify_setup/install.ts
Comment thread src/ipc/utils/ssh_client.ts
Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/components/CoolifyConnector.test.tsx Outdated
Comment thread src/coolify_setup/setup_flow.test.ts Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.test.ts
Comment thread src/coolify_setup/controller.ts Outdated
Comment thread src/shared/coolify_admin_email.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f40dcecbe0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/shared/coolify_admin_email.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07fc8d9c4f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/coolify_setup/setup_flow.ts
Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/coolify_setup/transition.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 23 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/coolify_setup/https_setup.ts Outdated

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/coolify_setup/setup_flow.ts Outdated
Comment thread src/coolify_setup/https_setup.ts
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

A large, unusually disciplined PR: 63 files, a pure state machine (state.ts/transition.ts) that respects the machine-boundary rules, an injectable controller, a new SSH client behind a three-function seam, and unit + integration + Playwright coverage for nearly every branch I checked. The Electron boundary holds up: every new channel goes through defineContract with zod input validation, is registered in VALID_INVOKE_CHANNELS/VALID_RECEIVE_CHANNELS, the SSH private key never crosses to the renderer, expected failures use DyadError with sensible DyadErrorKinds, and telemetry.ts was extended so coolify-setup: failures are not reported as product exceptions. Renderer data fetching uses TanStack Query with the shared queryKeys.coolify.* namespace, UI primitives are the existing wrappers (no Radix added), and there is no schema change, so no Drizzle migration is required.

Values that reach a root shell are rejected rather than escaped (isShellSafe, envArgs, assertSafeContainer, isPlausibleInstanceDomain), secrets go over stdin/docker exec -e instead of the command line, and the new settings fields ride the existing encrypted SecretSchema path with a drop-on-undecryptable read. I could not find a command-injection or credential-exposure defect.

The two issues below are robustness/UX, not correctness blockers. Note also that the context's combined diff is marked truncated, but every one of the 63 per-file patches is complete, so this review is based on the full change.

Issues Summary

Severity File Issue
🟡 MEDIUM src/coolify_setup/setup_flow.ts:190 Dashboard timeout blames Coolify for what is usually a blocked port
🟡 MEDIUM src/coolify_setup/https_setup.ts:70 Hostname resolving to a private address still pays the certificate wait
🟢 Low Priority Notes (7 items)
  • The fingerprint pin is re-armed on every check - inspect always uses trustOnFirstUse, so a second "Check server" silently overwrites the stored fingerprint. The pin therefore only protects the gap between one check and the install that follows it, not against a server that changes identity between sessions. Worth saying so in the panel, or persisting the first-seen key. (src/ipc/handlers/coolify_setup_handlers.ts)
  • inspectedFingerprints is never pruned - one entry per address checked, held for the life of the main process. Harmless in practice, but it is unbounded state in a long-lived process. (src/ipc/handlers/coolify_setup_handlers.ts)
  • A failed snapshot query dead-ends the form - snapshot.isError disables Install with no message and no retry, while the adjacent serverKey.isError gets both "Could not read the key" and a "Try again" button. Low likelihood (the snapshot is an in-memory read), but the disabled control has no explanation. (src/components/CoolifyServerSetup.tsx)
  • Cancel can take a while to land during the HTTPS step - the finally in tryEnableHttps unsets the domain without the abort signal, bounded only by APPLY_DOMAIN_TIMEOUT_MS (60s). On a healthy server this is seconds; on a wedged one the panel sits on "Stopping…" for up to a minute, or two if the apply also timed out. (src/coolify_setup/https_setup.ts)
  • A running or finished setup takes over every publish panel - CoolifyConnector returns the setup screen for running/done before any app-specific branch, so every app in every window shows the installer until Continue or Cancel is pressed. Deliberate, and both exits are on screen, but it is a wide blast radius for a multi-minute operation. (src/components/CoolifyConnector.tsx)
  • The setup path does not clear previousAccessToken - coolify:set-token explicitly sets it to undefined when a new token is stored ("a token from two connections ago"), but the setup handler's success write does not. Nothing leaks today only because revealCredentials pairs a token with instanceUrl; the asymmetry is still a trap for the next edit. (src/ipc/handlers/coolify_setup_handlers.ts)
  • Only the subscriber keeps the snapshot live - CoolifyServerSetup is the only component that subscribes to coolifySetup:onChanged; CoolifyConnector reads the same query key without one. When the setup screen is not rendered, a run started in another window is invisible until something invalidates the coolify family. Narrow, since the installer screen is what a second window would be on. (src/components/CoolifyConnector.tsx)

The php artisan tinker driving and the > MARKER transcript parsing are, as the PR itself says, the least durable part. That is a reasonable trade given no supported interface exists, every call site carries a WORKAROUND/TODO, and the failure mode is a clean degrade to the existing paste-a-token path rather than a broken install — so I am not raising it as a finding.


Generated by Dyadbot persona-based code review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fbd8cab38c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/coolify_setup_handlers.ts
Comment thread src/components/CoolifyConnector.tsx Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/ipc/handlers/coolify_setup_handlers.ts
Comment thread src/coolify_setup/https_setup.ts Outdated
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

A large, unusually well-tested feature: a main-process state machine for installing Coolify over SSH, a new ssh_client wrapper around ssh2, and a rewritten CoolifyConnector landing experience. The IPC boundary looks sound — every new channel is contract-defined with Zod input/output, registered in VALID_INVOKE_CHANNELS/VALID_RECEIVE_CHANNELS (with a test asserting it), the private half of the server key never crosses to the renderer, expected failures use DyadError with sensible DyadErrorKinds, renderer data fetching goes through TanStack Query with the shared queryKeys, UI uses the existing @/components/ui wrappers, and coolify-setup: is added to the telemetry redaction prefixes. Values that reach a root shell or a PHP script on the user's machine are validated against an allowlist and refused rather than escaped (isShellSafe, envArgs, assertSafeContainer, isPlausibleInstanceDomain), scripts travel on stdin rather than a command line, and there is no schema change so no missing migration. The diff is complete (nothing truncated).

Two medium issues below. Neither blocks merge. I reviewed the diff statically; I did not run the suite, and the php artisan tinker transcript parsing is only verifiable against a real Coolify — the PR is explicit that those call sites are workarounds.

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/coolify_setup_handlers.ts:81 Install trusts any SSH host key when the server was not inspected first
🟡 MEDIUM src/coolify_setup/https_setup.ts:63 A custom domain skips the check for an address that can never get a certificate
🟢 Low Priority Notes (7 items)
  • Aborted command leaves its timeout timer pending - onAbort in run() sets cancelled, closes the stream and rejects, but never calls clearTimeout(timer); only the close handler and stopListening do. If the channel never opens, the timer survives until it fires. Harmless, but it is the one path that escapes the otherwise careful cleanup. (src/ipc/utils/ssh_client.ts)
  • domainPointsAtServer is advisory in a direction that can still store the wrong host - the comment names the risk precisely: a custom domain still pointing at the user's old site would answer HTTPS with its own valid certificate and be stored as Coolify's address. The guard only objects on a definite points-elsewhere, so a bundled-resolver failure (which fetch, using the system resolver, may not share) lets that through. Narrow, but it is the one path where the root-capable token ends up addressed to another machine. (src/coolify_setup/https_setup.ts)
  • Signing out now keeps the token on disk indefinitely - previousAccessToken is retained (encrypted) and shown in the panel. The success toast says so, and the reasoning is sound, but it is worth confirming this is the intended meaning of "Sign out of Coolify" for a user whose motive is revocation rather than rotation. (src/ipc/handlers/coolify_handlers.ts)
  • Inconsistent strictness against the same transcript parser - applyInstanceDomain matches with includes because "the transcript carries its own noise", while enableApi requires exact equality with "enabled" and mintApiToken uses an anchored regex. If the noise the first comment anticipates is real, the other two fail where the first succeeds. (src/coolify_setup/api_token.ts)
  • Fake SSH server reaches into ssh2 internals - inner._client._protocol.exitStatus(...) and outgoing.id are private; an ssh2 upgrade breaks the late-exit-status test in a way that will read as a product bug. Worth a comment pinning the version it was written against, next to the cast. (e2e-tests/helpers/fake_ssh_server.ts)
  • Global /api/v1 rewrite on the shared fake server - the new app.use claims that prefix for every spec that uses the fake LLM server, not just the Coolify ones. Nothing collides today, but scoping it (or registering it inside the Coolify mount) would keep it from quietly capturing a future fake's routes. (testing/fake-llm-server/coolify.ts)
  • An install failure produces both a toast and the in-panel failure block - the onError comment says only a refusal to start belongs to the caller, but every non-cancel rejection reaches showError, so a failed install is reported twice. (src/components/CoolifyServerSetup.tsx)

Generated by Dyadbot persona-based code review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 12 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/components/CoolifyServerSetup.test.tsx Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3290fd95f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 12 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.test.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 432149885b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/coolify_setup/setup_flow.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 3 inline finding(s).

Comment thread src/coolify_setup/https_setup.ts Outdated
Comment thread src/ipc/handlers/coolify_setup_handlers.ts
Comment thread src/ipc/utils/ssh_client.ts
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Large, well-structured feature: a main-process state machine for installing Coolify over SSH, a new ssh2-backed client, IPC contracts, settings storage for the generated admin account, and three new renderer panels. I reviewed all 69 changed files (every per-file patch in the context is complete; only the combined diff blob is truncated, which does not affect confidence).

What holds up well:

  • IPC boundary. New channels are contract-defined, added to both VALID_INVOKE_CHANNELS and VALID_RECEIVE_CHANNELS, and covered by a test that asserts it. Only the public half of the server key crosses to the renderer; the private key never leaves main. Inputs are zod-validated and re-validated in the handler (isPlausibleAdminEmail, isPlausibleInstanceDomain).
  • Command construction. Values sent to the server are single-quoted and rejected rather than escaped (isShellSafe, envArgs, assertSafeContainer, the tokenName regex). Secrets go over stdin or docker exec -e, never on a command line visible to ps. The busy= shell precedence (A || B && C || D) parses correctly.
  • Error kinds. Expected refusals use DyadError with Precondition / Validation / Auth / UserCancelled, so they are filtered from telemetry; telemetry.ts was extended so coolify-setup: messages are stripped like coolify:.
  • Host key handling. Trust-on-first-use only during inspect, then pinned by fingerprint for the install, with readyHosts enforced server-side (the renderer cannot skip the check). The fingerprint and the "ready" verdict are written together after the probe finishes.
  • State machine. Pure transition + controller split follows rules/state-machines.md; invocation refs correlate answers, stay vs change avoids no-op broadcasts, and the renderer reads the snapshot through TanStack Query with a push subscription and an overtake guard rather than caching it.
  • Settings. forgottenCoolify() correctly names every key as undefined so reconcilePreservedSecrets treats it as a deliberate clear instead of re-injecting ciphertext; the admin password encrypt/decrypt hooks mirror the existing token path, and dropping only password (not admin) on a failed decrypt preserves the ciphertext container.
  • UI. Base UI wrappers (alert-dialog, checkbox), and loading/empty/error/disabled states are handled on every new surface, including the awkward ones (a refetch that fails over credentials already in hand keeps them on screen).

The three items below are informational and none blocks merge.

Issues Summary

Severity File Issue
🟡 MEDIUM src/coolify_setup/https_setup.ts:259 Domain ownership guard is skipped when the server hostname does not resolve
🟡 MEDIUM src/ipc/handlers/coolify_setup_handlers.ts:207 API token stored for an unencrypted instance without the insecure acknowledgement
🟡 MEDIUM src/ipc/utils/ssh_client.ts:145 Unreachable and timeout SSH failures are classified External and report as exceptions

Domain ownership guard. domainPointsAtServer returns "different-families" for an unknown verdict only when expectedIps.length > 0; with an empty expectedIps it falls through to "points-here". expectedIps is empty whenever the host is a name that dns.Resolver cannot answer for but connectSsh can still reach. A custom domain is then applied and its certificate accepted as proof, and https://<domain> becomes the stored instance URL the API token travels to — the precise outcome the "no-answer" branch three lines above exists to prevent. Narrow to reach, but the fix is small.

Insecure token store. The setup path writes instanceUrl + accessToken directly, bypassing the acknowledgedInsecure gate coolify:save-token enforces. When the install lands on plain HTTP the root-capable token is persisted and travels unencrypted on every deploy without explicit consent. The code comment explains why asking beforehand is not honest here; worth a deliberate sign-off rather than silent inheritance.

Telemetry classification. SshError for unreachable / timeout / unknown uses DyadErrorKind.External, which is not filtered, so a mistyped address or a firewalled port 22 fires a $exception on every attempt. The repo already suppresses the analogous Coolify HTTP transport error by name in shouldFilterTelemetryException; SshError was not added there, and the new coolify-setup: prefix only strips the message.

🟢 Low Priority Notes (6 items)
  • Sign-out now also forgets the instance address - clearToken writes forgottenCoolify(), so a user rotating a hand-pasted token must retype their instance URL; previously it was remembered. Mitigated by the sign-out dialog showing it with a copy button, but it is a behaviour change for users who never touch the installer. (src/ipc/handlers/coolify_handlers.ts)
  • Duplicate test ids while the dialog is open - CoolifySignOutDialog renders its own CoolifyCredentials while coolifySection still renders one behind it, so coolify-credentials and coolify-field-* appear twice in the DOM. The component tests render the dialog in isolation and do not catch it; a future e2e selector would be ambiguous. (src/components/CoolifySignOutDialog.tsx)
  • Doc comment does not match the field - SetupResultSchema.tokenStored is a boolean but its comment reads "Null when Coolify was installed but its API could not be opened." (src/ipc/types/coolify_setup.ts)
  • Unbounded in-memory maps - inspectedFingerprints and readyHosts grow for the life of the process with no eviction. Harmless in practice, but there is no upper bound. (src/ipc/handlers/coolify_setup_handlers.ts)
  • Scroll effect re-runs every render - useEffect(..., [setup]) depends on snapshot.data ?? { type: "idle" }, a fresh object on every render while the snapshot is undefined, so the effect fires each render. No visible impact since logRef is null in that state. (src/components/CoolifyServerSetup.tsx)
  • Vitest test imports an e2e helper - setup_flow.integration.test.ts reaches into e2e-tests/helpers/fake_ssh_server.ts. It works (the integration project includes src/**/*.integration.test.ts), but it couples the unit suite to the Playwright tree; a src/testing/ home would be more conventional. (src/coolify_setup/setup_flow.integration.test.ts)

Two things I could not verify from the diff and am not asserting either way: whether the tinker transcript shape the parser depends on matches real Psy Shell output on Coolify versions other than 4.3.2 (the PR flags this as the least durable part and every call site carries a WORKAROUND/TODO), and whether ssh2's optional native pieces are genuinely skippable on all packaged targets — forge.config.ts ships only the pure-JS dependencies, which is right if the optional native path is truly optional.


Generated by Dyadbot persona-based code review

RyanGroch and others added 4 commits August 24, 2026 17:15
…a fault

classify maps unreachable and timeout to External, which telemetry does not
filter, so a mistyped address or a firewalled port 22 sent a PostHog
$exception on every check — with whatever the user typed in the message.
The same call over HTTP is already treated as the user's own network rather
than a fault here; SSH was never wired into it.

Only the two that say what went wrong. "unknown" is the bucket for a
failure nothing here recognised, which is the kind worth hearing about.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dyad invents the admin password rather than discovering it, so it is known
a moment before the installer writes it into Coolify's own .env. Nothing
was stored until the server reported the account back, minutes later — and
anything that ended the process in between took the only copy with it,
leaving a running Coolify nobody has the password for and a preflight that
refuses to install over the container.

It goes down before the run now. A run that ends without ever seeding an
account puts back whatever stood there before, so a server that never got
one leaves no password behind for it — and an account belonging to a server
set up earlier, still the only copy of its own password, is not taken along
with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A server reached by a name plain DNS cannot see — an /etc/hosts entry, a
search-domain name — left nothing on our side to hold the domain against,
and that fell through as though the domain had been checked. It had not
been checked at all, which is the whole of what the function is for, and
the certificate poll after it settles for any address answering with a
certificate it trusts.

The earlier reasoning was that refusing blocks a setup over a private
name. It does, and that is the smaller loss: such a setup keeps a working
plain-HTTP address and can have the domain set in Coolify by hand, where
the alternative is the API token going to whatever the domain points at.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Saving a token by hand refuses an address that is not encrypted unless the
user says otherwise. Installing a server did not: the token is written when
the run ends, before the finished screen has said anything, so the warning
there described something already decided.

The screen asks now, unticked, and Continue takes the token and the address
back off unless it was agreed to. What is left is the state a run whose
token could not be minted already produces — the account, the address on
screen, and the token form a paste away — so nothing new had to be invented
for the answer to mean something.

Only those two fields go. The admin account is the way into a server that
is running either way, and holding it sends nothing anywhere; the token is
what would have crossed the network in the clear on every deploy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RyanGroch and others added 3 commits August 24, 2026 17:52
…led run

Putting the password down before the installer meant putting something
back if no account ever appeared. What went back was a snapshot taken when
the record went down — and the record goes down after connecting and after
the preflight, so a run that ended before either wrote `undefined` over
whatever stood there. Cancelling, a server not answering, and retrying an
address that already has Coolify all end that way, and all of them threw
away the password of a server set up earlier: the loss this was written to
prevent, on the ordinary paths.

Only a run that put a record down takes one back, and only if its own is
still the one there. Minutes of installing sit in between, and signing out
in another window is a newer answer than a snapshot from before the run —
which rules/electron-ipc.md says in as many words about reading settings
across an await.

Also splits the refusal for a server whose own name does not resolve. It
was borrowing the sentence for a domain that could not be looked up, and
naming the domain, when the domain may be perfectly correct and the server
is what nothing could be found for. And a decline that fails now keeps the
finished screen up rather than dismissing over a token still stored.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dable

The way out told the record apart by its password, and a password that
will not decrypt is dropped on the way out of readSettings with the
account kept. So a keychain relocking during the install — minutes, on a
screen that says leaving it does not stop the run — made the record
unrecognisable: the restore was skipped, an earlier server's account went
with it, and what stood in its place was a password for an account that
was never created.

Identified by what still says whose it is. The password when there is one,
and the email and address either way, which is what the other window
finishing its own install differs by.

Also puts the comment about families back over the branch about families.
It had been left heading the one about a server that does not resolve,
where its first clause said the opposite of the condition beneath it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An account whose password will not decrypt comes back from readSettings
with the key gone, not with it undefined. Writing it back that way reads
to writeSettings as a key some consumer dropped, so the ciphertext on disk
is handed back — and by then that ciphertext is this run's provisional
password, filed under the earlier server's name and address. A credential
for a machine it does not open, which is the harm the record's own
description warns about.

Named now, so an absent password is the clear it was meant to be. Nothing
is lost that was not already: the earlier ciphertext was overwritten when
the provisional record went down.

Also covers the account's email in the check that tells this run's record
from someone else's — it was right and nothing held it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 11 files (changes from recent commits).

Confidence score: 2/5

  • src/coolify_setup/https_setup.ts lets a custom domain with no DNS records fall through from no-records to points-here, so tryEnableHttps may accept an unverified domain; keep the no-records result from being treated as pointing here.
  • src/components/CoolifyServerSetup.tsx leaves an already-persisted token in place when setup is abandoned, allowing a relaunch to enable deployment over a plain-HTTP address; remove the token when the screen is closed without Continue.
  • src/ipc/handlers/coolify_setup_handlers.ts can clear a newer Coolify connection and token created by another window because cleanup is not tied to this setup run; pass and validate the run-specific token or instance identity before clearing.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/ipc/handlers/coolify_setup_handlers.ts">

<violation number="1" location="src/ipc/handlers/coolify_setup_handlers.ts:138">
P2: When another window changes the Coolify connection before this setup screen is dismissed, this handler clears that newer instance and token because it has no way to identify the token belonging to this run. Pass the setup instance or token as an expected value and clear only when the current settings still match it.</violation>
</file>

<file name="src/coolify_setup/https_setup.ts">

<violation number="1" location="src/coolify_setup/https_setup.ts:265">
P1: When the SSH host name has no DNS addresses and the custom domain currently has no records, `domainCheckVerdict` returns `no-records`, so this guard falls through to `points-here`. `tryEnableHttps` can then accept a trusted certificate from an old server if DNS changes during polling; classify `no-records` as `server-unresolved` when `expectedIps` is empty.</violation>
</file>

<file name="src/components/CoolifyServerSetup.tsx">

<violation number="1" location="src/components/CoolifyServerSetup.tsx:261">
P1: When the user closes or abandons this screen before clicking Continue, the unchecked box never removes the already-persisted token. Relaunching then sees `hasToken` and can enable deployment over the plain-HTTP address without explicit acceptance; keep the token out of persistent settings until acceptance, or revoke it on every abandonment path.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/components/CoolifyServerSetup.tsx
Comment thread src/coolify_setup/https_setup.ts
Comment thread src/ipc/handlers/coolify_setup_handlers.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d40878a61

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/utils/ssh_client.ts
Comment thread src/ipc/handlers/coolify_setup_handlers.ts Outdated

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/ipc/types/coolify_setup.ts Outdated
Comment thread src/ipc/types/coolify_setup.ts
@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Large, unusually well-documented feature: a main-process state machine that SSHes into a bare server, runs Coolify's installer, seeds an admin account, tries for HTTPS, and mints an API token, plus the panel that drives it. The IPC surface is registered through the existing typed-contract layer, both new channels are in VALID_INVOKE_CHANNELS/VALID_RECEIVE_CHANNELS (with a test asserting it), the machine follows rules/state-machines.md (pure transition + controller + invocation refs), renderer reads go through TanStack Query, expected failures use DyadError with sensible DyadErrorKinds, the new dialog uses the Base UI alert-dialog wrapper, and the SSH/shell boundaries reject rather than escape (isShellSafe, envArgs, assertSafeContainer, the token-name and instance-domain regexes). The private key never leaves the main process; only the public half crosses IPC. ssh2 is externalized in vite.main.config.mts and its runtime deps are unignored in forge.config.ts, with the optional native pieces correctly left out.

Two things are worth a second look before this ships, both about how the freshly minted credentials are handled rather than about the install itself. Neither is a blocker.

Note on the review context: the aggregate diff field is truncated, but every one of the 69 per-file patches is complete (patchTruncated: false), so this review is based on the full changes.

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/types/coolify_setup.ts:252 Insecure-address token is stored before the user consents
🟡 MEDIUM src/ipc/types/coolify_setup.ts:127 Coolify API token is now readable by the renderer
🟢 Low Priority Notes (6 items)
  • Signing out lands on the token form, not the installer - onConfirm resets token and acknowledgedInsecure but not isEnteringToken, so after a sign-out the panel shows the paste-a-token screen rather than the installer the PR deliberately makes the landing page. Recoverable in one click via the "Set one up" link (which does render, since status.serverUrl is cleared by then), so it is cosmetic — but setIsEnteringToken(false) beside the other two resets would match the stated design. (src/components/CoolifyConnector.tsx:376)
  • Stale doc on tokenStored - The comment reads "Null when Coolify was installed but its API could not be opened" over tokenStored: z.boolean(), which is never null. Likely left over from when the field carried the token. (src/ipc/types/coolify_setup.ts:102)
  • Orphaned doc comments - Three doc blocks are detached from what they describe: "Where the dashboard answers" in install.ts documents plainUrlFor, which now lives in https_setup.ts; the "Tries to put the instance on HTTPS" block sits immediately above resolvesPublicly rather than tryEnableHttps; and "Returns Dyad's server key, creating it the first time" sits above storedMatching rather than ensureServerKey. (src/coolify_setup/install.ts, src/coolify_setup/https_setup.ts, src/coolify_setup/server_key.ts)
  • No unit test for isPlausibleInstanceDomain - Every other new module in this PR has a sibling test, including the closely analogous coolify_admin_email.ts. This one guards a value that ends up in a script running as root on the user's server, so it is the one most worth pinning. (src/shared/coolify_domain.ts)
  • No pending state for the credentials panel inside the connector - CoolifyCredentials returns null while its query is in flight. CoolifySignOutDialog compensates with its own "Looking up what Dyad has stored…" line, but the connector panel just shows a heading over empty space for a moment. (src/components/CoolifyCredentials.tsx)
  • Duplicate test ids when the sign-out dialog is open - CoolifySignOutDialog renders a second CoolifyCredentials while the panel's copy is still mounted, so coolify-field-address and friends resolve to two elements. idPrefix only disambiguates within a single instance. Test-brittleness rather than a user-facing problem. (src/components/CoolifySignOutDialog.tsx)

Generated by Dyadbot persona-based code review

@github-actions

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 295 1 1 12

Summary: 295 passed, 1 failed, 1 flaky, 12 skipped

Failed Tests

🍎 macOS

  • coolify_setup.spec.ts > installs Coolify onto a server and connects to it
    • Error: expect(locator).toBeVisible() failed

📋 Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/coolify_setup.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • test_recording.spec.ts > records interactions in the preview and reviews them without writing a file (passed after 1 retry)

📊 View full report

RyanGroch and others added 2 commits August 26, 2026 08:59
…oing it

The gate asked after the token was already on disk, so it only worked for
someone who pressed Continue. Closing the panel, switching away, quitting,
or a crash while the finished screen was up left a token stored for an
address nobody had agreed to — and the next launch simply read as
connected. Two reviewers called it, and it was the known cost of undoing
rather than withholding.

Withheld now. A run that ends on an address without a certificate keeps its
token in the process, and only agreeing puts it on disk. Anything else
loses it, which is the safe direction and lands on the screen a run whose
token could not be minted already produces. The decline handler goes with
it, and with it an unconditional settings write that would have cleared
another window's connection.

Also stops a domain being taken on trust when neither side resolved.
domainCheckVerdict answers about the domain first, so a name with no
records yet came back before our own address was ever looked at, and read
as agreement having compared nothing.

And hands ssh2 an address it recognises: a v6 literal typed the way
documentation writes it, in brackets, was looked up as a hostname, so a
reachable server reported as unreachable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The installer's end-to-end journey installs onto a loopback address, which
can never be given a certificate — so it ends on the branch that now holds
the token rather than storing it, and went on asserting the sentence the
secure branch says and pressing Continue without a word.

It ticks the box now. That click is the whole of the difference between the
server picker and the token form, which makes this the one place the
agreement is proved end to end rather than in parts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30d77033f5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

*/
export function isPlausibleAdminEmail(email: string): boolean {
const trimmed = email.trim();
if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(trimmed)) return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject invalid mailbox syntax before installing

For an address such as a..b@gmail.com, this permissive regex returns true even though consecutive dots make the mailbox invalid. The run handler therefore starts the installation, but Coolify's RootUserSeeder later rejects the address after the Coolify container exists; subsequent setup attempts are then blocked by preflight as already installed. Validate the complete mailbox syntax before beginning the non-idempotent install, independently of whether the domain resolves.

Useful? React with 👍 / 👎.

Comment on lines +400 to +402
export function resetCoolifySetupStateForTests(): void {
inspectedFingerprints.clear();
readyHosts.clear();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear held insecure tokens during test reset

When a handler test finishes an insecure setup without accepting or dismissing it—as the first test in the insecure-address describe block currently does—heldInsecureToken survives this reset, so a later test can call accept-insecure-token and persist a credential created by the previous case. This is fresh evidence beyond the earlier controller-reset issue because the token holder was introduced later and is another process-global owner omitted here; include it in teardown or construct the handler state per test. rules/state-machines.mdL610-L611

Useful? React with 👍 / 👎.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 8 files (changes from recent commits).

Confidence score: 5/5

  • In src/ipc/handlers/coolify_setup_handlers.ts, resetCoolifySetupStateForTests omits clearing heldInsecureToken, which can make tests order-dependent and leak a token across cases — clear that field alongside the other process-global state to keep test isolation reliable.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/ipc/handlers/coolify_setup_handlers.ts">

<violation number="1" location="src/ipc/handlers/coolify_setup_handlers.ts:83">
P3: `resetCoolifySetupStateForTests` does not clear the new held token, so tests can leak an insecure token across cases and become order-dependent. Clear `heldInsecureToken` alongside the other process-global setup state.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

*
* One at a time, like the machine itself.
*/
let heldInsecureToken: { instanceUrl: string; token: string } | null = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: resetCoolifySetupStateForTests does not clear the new held token, so tests can leak an insecure token across cases and become order-dependent. Clear heldInsecureToken alongside the other process-global setup state.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/coolify_setup_handlers.ts, line 83:

<comment>`resetCoolifySetupStateForTests` does not clear the new held token, so tests can leak an insecure token across cases and become order-dependent. Clear `heldInsecureToken` alongside the other process-global setup state.</comment>

<file context>
@@ -68,6 +68,20 @@ const inspectedFingerprints = new Map<string, string>();
+ *
+ * One at a time, like the machine itself.
+ */
+let heldInsecureToken: { instanceUrl: string; token: string } | null = null;
+
 function broadcastState(state: SetupSnapshot) {
</file context>

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 3 inline finding(s).

// without reaching the code below is exactly how the only copy of it
// gets lost. Put back on the way out if no account ever appeared, so a
// server that never got one does not leave a password behind for it.
onCredentialsBuilt: ({ credentials, dashboardUrl }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM

Second install overwrites the stored admin password for the first server

onCredentialsBuilt writes the new run's admin account into settings.coolify.admin before the installer runs, and settings hold exactly one admin record. The restore path below only fires when this run never seeded an account (provisional && !accountConfirmed), so a run that does seed one silently discards whatever account was stored for a different machine. This is reachable: if an install onto server A fails after the account was seeded (for example the dashboard poll fails because port 8000 is blocked), the state is failed with cancelled=false, which makes isReportingFailure true in CoolifyConnector, which skips the 'Dyad already set up a server' refusal card and hands the installer form back with an empty host field. Typing server B and pressing Install then destroys the only copy of server A's password, on a machine that has a running Coolify with an account nobody else knows the credentials for. Signing out asks for an explicit acknowledgement before forgetting the same record; this path asks for nothing.

💡 Suggestion: Before writing over an existing coolify.admin whose instanceUrl does not match the host being installed, either refuse or make the installer screen say plainly that continuing forgets the stored account for that other server, the way CoolifySignOutDialog does.

<p className="text-sm text-muted-foreground">
{result.secure
? "Dyad created its own API token, so you can pick a server and project next."
: "Dyad created an API token for this server. It is not kept unless you say so above, because this address is not encrypted."}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM

Declining an insecure token leaves no way to obtain one

When a run ends on a plain-HTTP address with a token minted (result.tokenStored true, result.secure false), leaving the checkbox unticked and pressing Continue calls leaveResult with acceptToken false, so dismiss() drops heldInsecureToken and the minted token is gone. onUseExisting then puts the user on the token form, pinned read-only to that same unencrypted address, where saving requires ticking 'Connect anyway' - the consent they just declined. The coolify-setup-manual-token block, which is the only place that explains how to enable the API and create a token by hand in Coolify, is rendered only when tokenStored is false, so it is hidden in exactly this case. The user is left on a form asking for a token they were never shown and are given no instructions for producing.

💡 Suggestion: Show the manual-token guidance (or a pointer to adding a domain so HTTPS can be retried) when the user continues without accepting the insecure token, rather than only when no token could be minted.

* On the ordinary path it is stored instead, and read back through
* revealCredentials — a password shown once is a password nobody can use.
*/
adminPassword: z.string(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM

Generated admin password rides in the broadcast setup snapshot

SetupResultSchema.adminPassword is part of the done snapshot, which broadcastState pushes to every BrowserWindow over coolify-setup:changed and which any renderer can pull with coolify-setup:snapshot. The plaintext password therefore sits in the React Query cache of every window, including ones not showing the panel, for as long as the machine rests in done. The run and revealCredentials contracts and handlers both carry a 'DO NOT LOG this handler' marker because they carry this secret; snapshot and changed carry the identical payload and carry no such marker, so a maintainer switching snapshot to createLoggedTypedHandler would log the password without any warning in the file.

💡 Suggestion: Mark the snapshot contract and the changed event with the same DO NOT LOG note as run and revealCredentials, and consider having the done screen read the password through revealCredentials rather than shipping it in a state that is pushed to every window.

@dyad-assistant

Copy link
Copy Markdown
Contributor

🔍 Dyadbot Code Review Summary

Verdict: 🤔 NOT SURE - Potential issues
Recommendation: ready

Large, unusually well-documented feature: a state machine in the main process, a new ssh_client wrapper around ssh2, host-key pinning between the preflight check and the install, DyadError/DyadErrorKind used consistently for user, validation and precondition failures, telemetry message-stripping extended to the new coolify-setup: channel prefix, all new IPC channels added to the preload allowlists (with a test that enforces it), Base UI primitives rather than Radix, and TanStack Query for every renderer read. Shell and PHP interpolation is refused rather than escaped in admin_credentials.ts, tinker.ts, coolify_domain.ts and coolify_admin_email.ts, and secrets travel to the container via docker exec -e and to the installer via stdin rather than a command line. No database schema changes, so no migration is required.

Nothing I found blocks merge. The three items below are worth a look; two are about what happens to a password only Dyad holds, and one is a dead-end in the unencrypted-address flow.

Note on confidence: the context's aggregate diff field is truncated, but every one of the 69 per-file patches is complete (patchTruncated: false), so this review is based on the full change. I could not exercise the flow against a real Coolify, so claims about Coolify's own internals (tinker transcripts, seeder behaviour, proxy rebuild) are taken as the PR describes them.

Issues Summary

Severity File Issue
🟡 MEDIUM src/ipc/handlers/coolify_setup_handlers.ts:150 Second install overwrites the stored admin password for the first server
🟡 MEDIUM src/components/CoolifyServerSetup.tsx:279 Declining an insecure token leaves no way to obtain one
🟡 MEDIUM src/ipc/types/coolify_setup.ts:101 Generated admin password rides in the broadcast setup snapshot
🟢 Low Priority Notes (8 items)
  • Orphan doc comments - Three doc blocks describe something that is not the next declaration: the "Where the dashboard answers" block sits above waitForDashboard's own block but describes plainUrlFor, which now lives in https_setup.ts; "What the account write could not store" sits above accountConfirmed but describes unsavedAccount; and "Returns Dyad's server key, creating it the first time" sits above storedMatching. (src/coolify_setup/install.ts, src/ipc/handlers/coolify_setup_handlers.ts, src/coolify_setup/server_key.ts)
  • resetCoolifySetupStateForTests does not clear heldInsecureToken - It clears the fingerprint pin, the ready set and the controller, so a token held by one test can still be written by acceptInsecureToken in the next one. (src/ipc/handlers/coolify_setup_handlers.ts)
  • SetupStep is defined three times - state.ts, setup_flow.ts and ipc/types/coolify_setup.ts each spell the same eight steps out. The state/wire pair is held together by _snapshotMatchesState; the setup_flow.ts copy is only caught indirectly, by the hooks.onProgress(step, output) call site in the handler. (src/coolify_setup/setup_flow.ts)
  • Dashboard-failure message hard-codes port 8000 - The URL in the same message comes from dashboardPort(), so the two disagree under a test build. No production impact, since the installer fixes the port. (src/coolify_setup/setup_flow.ts)
  • An unreadable tinker transcript is reported as an unsupported version - readCoolifyVersion returns null both for a version it cannot parse and for a runTinker that threw because the markers were missing (a container still starting), and the caller turns null into "This version of Coolify could not be set up automatically." The SSH-failure branch just above draws exactly this distinction. (src/coolify_setup/api_token.ts)
  • VERIFIED_AGAINST is exported but unused by product code - The comment says it is deliberately not a maximum; nothing reads it outside tests, so it documents rather than does anything. (src/coolify_setup/api_token.ts)
  • Preflight reports the package lock before an existing Coolify - A server that is both mid-first-boot and already running Coolify is described as "still finishing its own first-boot setup", so the user waits and re-checks to learn the real reason. Both refuse the install, so only the message is affected. (src/coolify_setup/install.ts)
  • The finished screen shows the admin password unmasked - CoolifyCredentials puts the same password behind a reveal toggle; the done screen prints it in a <code> element. Defensible ("this is the moment they are needed"), but it is the one screen most likely to be up while the user is sharing a window. (src/components/CoolifyServerSetup.tsx)

Generated by Dyadbot persona-based code review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants