Skip to content

fix(desktop): store saved SSH hosts in the versioned JSON document - #454

Merged
iamdin merged 12 commits into
mainfrom
fix/ssh-environments-store
Oct 11, 2026
Merged

iamdin merged 12 commits into
mainfrom
fix/ssh-environments-store

Conversation

@oxwen11

@oxwen11 oxwen11 commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Requirement

Saved SSH hosts must move onto the versioned JSON document without losing hosts, and without blocking Desktop when the file cannot be read.

Developer decisions:

  1. Adopt a pre-envelope { version, activeId, environments } file into version 1 only when every entry validates. Drop activeId. If any entry fails validation, abort adoption and leave the file byte-for-byte unchanged.
  2. A corrupt file, a version-1 file that fails the schema, a newer version, or an aborted adoption is left untouched. Desktop still starts. SSH hosts are unavailable and the connections menu shows a fixed error.
  3. Do not create the file at startup. Write it on the first connect or remove.

Expected behavior

  • Missing file: empty host list, no file created until a real change.
  • Fully valid pre-envelope file: rewritten as { version: 1, data: { environments } } at 0600.
  • Pre-envelope file with any invalid entry: file bytes unchanged. SSH hosts unavailable with the same fixed error as a corrupt file.
  • Unreadable file: Desktop window opens. Connections shows "Saved SSH hosts could not be read. The file was left unchanged."
  • A saved host list update encodes without hostsError: undefined.

Changes and risks

makeJsonDocument can skip seeding and pin a mode on each atomic write. Desktop uses both. An unreadable or partially invalid legacy store no longer fails startup and never drops entries to force a migration.

UI: the connections menu shows the fixed hosts error and hides Add SSH host while the file cannot be read.

Verification

apps/desktop vitest: desktop-ssh.test.ts and desktop-application.test.ts passed, including adoption mode 0600, aborted adoption leaving the original bytes, and encoding a snapshot after a list update.

Desktop screenshots and video: #454 (comment)

Corrupt, invalid, and pre-envelope files fail and stay untouched. Version stays 1.
@pkg-pr-new

pkg-pr-new Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
npx https://pkg.pr.new/oxwen11/pie/@getpie/cli@454

commit: bef82a2

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit bef82a2.

@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Blocked — head 2fe0e4dd, base bd1af63a (now behind main 047a49cf).

  1. Required CI fails. Check run 37438177270: Electron e2e daemon-attach.spec.ts and import-project.spec.ts time out on electronApplication.firstWindow (30 s, all 3 attempts). The window never opens. This PR is the only one changing Desktop startup, so treat it as a regression until shown otherwise, not as a flake.

  2. Host-write decisions need Developer confirmation (persistence.md gate) before review continues:

    • New startup write. makeJsonDocument seeds storage/ssh-environments.json (then chmod 0600) on every Desktop start, even for users who never save an SSH host. Before, the file was written only on save.
    • Legacy adoption dropped. The pre-envelope {version:1, activeId, environments} file used to be read. It now fails Desktop startup.
    • Partially malformed files. A version-1 file with one bad entry used to load the valid entries. It now prevents Desktop from starting at all.

    ADR 0007 forbids silently resetting the file, but it does not require refusing to start the whole app over an optional feature's store. Options: adopt the legacy shape through the library's legacy step, and/or keep Desktop running with only SSH hosts unavailable (file left untouched).

Needs: a green Check on a head updated to current main, plus a Developer decision on the three points above. No independent verification was run; it is blocked behind CI.

iamdin added 2 commits October 6, 2026 13:24
electron-vite externalizes production deps. Leaving the store external made Main import its TypeScript source, so the window never opened.
@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

auto-merge: no

Reviewed head 75be3876c392 (open, non-draft, author oxwen11, base main; every check green); rules recovered from trusted main copy 451a89407c6b (auto-merge-pr.md + exclusions; .agents/rules/auto-merge-pr.md is absent on current main).

  • Not mergeable: GitHub reports CONFLICTING / DIRTY against main b1888a3ff654, so the gate fails.
  • Functional desktop persistence change (excluded): desktop-ssh.ts rewrites saved SSH host storage into the versioned JSON document (+71/−158), startup-failure.ts changes startup handling, and config/paths.ts changes a path.
  • New package dependency: apps/desktop/package.json adds @getpie/effect-json-store (+ lockfile, electron-vite externals).

Needs a rebase and a manual review/merge decision.

@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Developer decisions on the host-write questions:

  1. Legacy format. Adopt the existing saved-SSH-hosts data through a migration into the versioned JSON document, without losing any hosts.
  2. Corrupt or newer file. Desktop still starts; only the SSH hosts feature is unavailable, and it shows a safe error. Do not overwrite or discard the file.
  3. No seeding. Do not create the file at startup; write it only on the first real change.

Please also resolve the current conflicts with main and update docs/host-persistence.md in the same PR. Re-review starts from CI on the next push.

iamdin and others added 3 commits October 7, 2026 09:03
A missing file is not created until the first change. A pre-envelope file is migrated. A corrupt or newer file is left untouched and only SSH hosts are unavailable.
@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

CI on bd160539: required Check failed only on the known unrelated packages/ui browser flake (reasoning.test.tsx click timeout; see #444). That flake is not an infrastructure failure, so I did not re-run the job. main has since moved to 8e5cef14, so I updated the branch (merge of main only). Review starts once required CI is green on the new head.

@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Evidence for the review record.

  • Legacy file at base 8e5cef14: Desktop starts normally.
  • Legacy file at head 5c7e2a79: "Pie could not start".
  • Current v1 file with one host at head: the same crash.
  • Corrupt file at head: Desktop starts and the SSH hosts error is shown.

before-b-legacy-before-startup2

after-b-legacy-after-startup

after-e-current-v1-after-startup

after-c-corrupt-after-menu

@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Review and verification of head 5c7e2a79: changes needed

The newer head 48c78155 only merges main (#411) into the branch and does not touch these files, so the findings below apply to it unchanged.

  1. Required CI fails. Check run 37438177270: Electron e2e daemon-attach.spec.ts and import-project.spec.ts time out on electronApplication.firstWindow (30 s, all 3 attempts). The window never opens. This PR is the only one changing Desktop startup, so treat it as a regression until shown otherwise, not as a flake.

  2. Host-write decisions need Developer confirmation (persistence.md gate) before review continues:

    • New startup write. makeJsonDocument seeds storage/ssh-environments.json (then chmod 0600) on every Desktop start, even for users who never save an SSH host. Before, the file was written only on save.
    • Legacy adoption dropped. The pre-envelope {version:1, activeId, environments} file used to be read. It now fails Desktop startup.
    • Partially malformed files. A version-1 file with one bad entry used to load the valid entries. It now prevents Desktop from starting at all.

    ADR 0007 forbids silently resetting the file, but it does not require refusing to start the whole app over an optional feature's store. Options: adopt the legacy shape through the library's legacy step, and/or keep Desktop running with only SSH hosts unavailable (file left untouched).

Needs: a green Check on a head updated to current main, plus a Developer decision on the three points above. No independent verification was run; it is blocked behind CI.

An explicit undefined hostsError failed RPC output validation after a list update. Every hosts-file write now passes 0600 to the atomic writer.
@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Developer decision on invalid legacy entries

If any entry in a legacy, pre-envelope file fails validation, abort the migration:

  • leave the file byte-for-byte untouched;
  • make the SSH hosts feature unavailable with the same safe error shown for a corrupt file.

No entry is ever discarded. Please add a test for this case.

@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Desktop evidence after the hostsError encode fix.

  • after-saved-host.png: Desktop opens with a saved hosts file present. No "Pie could not start" dialog.
  • after-hosts-error.png: a corrupt hosts file leaves Desktop up. Connections shows "Saved SSH hosts could not be read. The file was left unchanged."
  • ssh-hosts.webm: those two states.

The crash dialog from the previous head was not re-recorded.

after-saved-host

after-hosts-error

ssh-hosts.webm

A pre-envelope file with one bad entry stays byte-for-byte unchanged. SSH hosts stay unavailable instead of dropping that entry.
@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

CI blocker on a0eecf71. The required Check fails at the apps/desktop typecheck (run 37632284792):

src/main/ssh/desktop-ssh.ts(178,23): error TS2345: Argument of type '{ id: string; alias: string; hostname: string; username: string | null; port: number | null; }' is not assignable to parameter of type 'never'.

Review and verification have not started. Please fix the typecheck and push. Running pnpm check locally first will catch this.

An untyped empty array was never[], so pushing a host failed typecheck.
@oxwen11

oxwen11 commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

CI on ca0ccfb0:

  • The typecheck is fixed.
  • The required Check failed again, only on the known flaky packages/ui browser test: reasoning.test.tsx "uses the Tool-style control…" hits a locator.click timeout (CI: required Check fails on unrelated test timeouts (browser + node suites) #444, run 37632895552). This PR does not touch packages/ui.
  • Under the current rules a flaky test is not an infrastructure failure, so I did not re-run CI.
  • Review and verification are waiting for a green Check on this head.

# Conflicts:
#	apps/desktop/src/main/application/desktop-application.test.ts
@iamdin

iamdin commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

Review record — verified, merging

  • Head: bef82a2c321f3a01f50dece97dd0eb1c0012ceae
  • Base: main @ 3bd796b47c9a411c4761ad4d1bc958bcbcd34365
  • Trusted rules: that main commit
  • Conclusion: pass. Squash merge of this head follows.

CI

On this head: Check, react-doctor, and both Publish @getpie/cli preview runs succeeded. Merge state was CLEAN against current main.

Review

Saved SSH hosts move to $PIE_HOME/storage/ssh-environments.json as one versioned JSON document, mode 0600. A legacy list is adopted only when every entry decodes; one bad entry leaves the file unchanged. Missing file is not created until the first successful connect. Removing a saved host drops it from the document and closes a live session for that id.

bef82a2c merges main after #484. The only conflict was the desktop application test imports: both EnvironmentSnapshotSchema and disabledDesktopCli stay. Local result on that head: desktop-application.test.ts 16 passed, desktop-ssh.test.ts 12 passed, and @getpie/effect-json-store 42 passed.

@iamdin
iamdin merged commit 4a6bcee into main Oct 11, 2026
6 checks passed
@iamdin
iamdin deleted the fix/ssh-environments-store branch October 11, 2026 15:53
@iamdin

iamdin commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

Merged as 4a6bcee2a90ab773b35378e12d845e59cdf8047a.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants