ReviewOS

stacks/uplink

fix(app): a malformed settings.json restart-loops the app, which is the loop #9 removed for the database

#44
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Part of #5.

#9 removed a launchd restart loop for the database: a file Uplink cannot open no longer takes the app with it. The same loop is still standing on settings.json, which is read three lines earlier, so the fix does not hold where it is reached first.

What is there today

  • readSettings merges the parsed file raw, { ...DEFAULT_SETTINGS, ...parsed } (app/Uplink/settings.ts#L55), and validates exactly one field: engine, which falls back with a comment anticipating "a hand-edited or downgraded file" (app/Uplink/settings.ts#L56-L59).
  • Nothing checks that allowed is an array, workdir a string, model a string or null, or openAtLogin and paused booleans. The try/catch around it covers JSON.parse only (app/Uplink/settings.ts#L62-L65).
  • settingsEnv then calls settings.allowed.join(',') (app/Uplink/settings.ts#L79).
  • That runs at app/Uplink/desktop-agent.ts#L160, before the SqliteStore construction that #9 wrapped in a try (app/Uplink/desktop-agent.ts#L169).
  • app/Desktop/launcher.ts has no top-level handler, and startDesktopAgent is awaited at module scope (app/Desktop/launcher.ts#L54).
  • The LaunchAgent sets KeepAlive with SuccessfulExit false (app/Uplink/desktop-agent.ts#L145), so a non-zero exit is restarted.

Reproduced against the real module: a settings.json holding "allowed": null returns unchanged from readSettings, and settingsEnv throws TypeError: null is not an object (evaluating 'settings.allowed.join'). The throw escapes startDesktopAgent, the process exits non-zero, launchd restarts it, and it happens again. No menubar ever appears, so there is nothing to say why.

A null allowed reaches the file the same ways the engine comment already anticipates: a hand edit, a downgrade to a build with a different shape, or a partial write from a crash mid-save.

Proposal

  1. Coerce every field in readSettings the way engine already is, right after the merge at app/Uplink/settings.ts#L55: allowed to an array, workdir and model to their types, openAtLogin and paused to booleans, each falling back to the default rather than refusing to start.
  2. The /api/settings handler already validates every field on the write path (app/Uplink/desktop-agent.ts#L622-L632). Share one validator between the two rather than writing the checks twice.

Requirements

  • Fall back, never throw. This file is read before the menubar exists, so a refusal is invisible by construction.
  • Every field, not the ones that look risky. engine was guarded because someone thought of it. The point is to stop depending on that.
  • The write path and the read path agree. A settings file the app itself wrote must always read back.

Acceptance

  • A test writes a settings.json with a wrong type in each field, one at a time, and asserts readSettings returns usable settings and settingsEnv does not throw.
  • A test asserts a file written by writeSettings reads back unchanged.
  • Deleting settings.json and starting the app still works.

Sign in to comment on this issue.