fix(desktop): repair update-mutex path quoting and publish an integer lockfile pid

[SZWzz]

Follow-up to #96084 (merged as beb212d). That salvage fixed the reservation-site double-quoting and the dash ${var//} bashism; two defects in the same spawn machinery remain on main.

1. withRemoteUpdateMutex still double-quotes the mutex path

remote-lifecycle.ts (main, line 916):

return `python3 -c ${shq(script)} ${shq(mutexPath)} ${shq(command)}`

Both call sites pass expandRemotePath(...) output, so mutexPath is already a complete shell word and the extra shq() stores the quote characters literally — the same defect class #96084 removed at the reservation sites. As @deepanaishtaweera demonstrated on #96084 by shim-capturing argv from the real buildSpawnCommand output, the remote python then makedirs/flocks a bogus $HOME/' tree instead of the real sidecar (reproduces the #96212 stray tree): a mutex that silently serializes nothing, with no boot symptom. Verified again here after the fix: argv[3] is now the single quote-free word $HOME/.hermes/.hermes-update-in-progress.mutex.

2. The sed placeholder swap publishes a quoted-string pid

#96084's POSIX swap replaces the bare __PID__ inside the JSON-quoted "pid":"__PID__" template, so the initial lockfile carries "pid":"4242" (string). Downstream:

• readLockfile() requires Number.isInteger(parsed.pid) — a client crash between the payload write and readiness leaves a malformed-pid record that fails closed (remote-lockfile-skew, no reap/respawn) on the next connect;
• the in-payload reuse regex "pid":\([0-9][0-9]*\) only matches an unquoted number.

This PR replaces the quoted placeholder (sed "s/\"__PID__\"/${child}/"), publishing a real JSON integer.

Regression coverage

• New argv-level test: the mutex path must appear as exactly one shell word (assert.ok(cmd.includes(...)) + assert.doesNotMatch(cmd, /'"\$HOME"'/)) — the previous substring assertion cmd.includes('.hermes-update-in-progress.mutex') matches both the broken and fixed forms.
• The POSIX-sh test from fix(desktop): fresh managed-SSH spawns no longer wedge on dash or double-quoted paths (salvage #96061) #96084 now pins the quoted-placeholder sed form.

Test plan

• All 93 remote-lifecycle unit tests pass (npx vitest run electron/remote-lifecycle.test.ts)
• Rendered payload passes dash -n; mutex argv verified quote-free by shlex-splitting the real buildSpawnCommand output
• Prior end-to-end round-trip of the same changes on a live fnOS (dash) remote: serve --isolated alive, backend.lock.json published, Remote Hermes backend is ready (reported on fix(desktop): fresh managed-SSH spawns no longer wedge on dash or double-quoted paths (salvage #96061) #96084)

Refs #96188, #96212. Previously surfaced in the discussion on #96084 and in the (closed duplicate) #96189.

[SZWzz]

A note on CI for reviewers: the ci.yaml run on this PR failed with zero jobs executed (startup-level failure), and the same workflow is currently failing on main as well (plus a startup_failure on a recent main run) — so this looks like a repo-wide workflow issue, not something introduced by these two files. The Docker/Nix runs show action_required (first-time-contributor approval gate). Happy to rebase/retrigger once the workflow is healthy again. Local verification in the meantime: all 93 remote-lifecycle unit tests pass, rendered payload passes dash -n, and the mutex argv verifies quote-free on a live fnOS remote.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

This PR fixes two defects in the Desktop remote-lifecycle spawn machinery: it drops the redundant shq() around the already-quoted mutex path (so the remote python no longer makedirs/flocks a bogus $HOME/' tree — a mutex that silently serializes nothing), and it changes the sed substitution to target the JSON-quoted "__PID__" placeholder so the initial lockfile publishes a real integer pid instead of a quoted string (which readLockfile rejects as malformed-pid skew, failing closed on reconnect). The argv-level regression tests are a meaningful improvement over the previous substring assertions. A couple of observations:

1. Correctness now rests on a caller invariant the signature does not express. With shq(mutexPath) removed (apps/desktop/electron/remote-lifecycle.ts:920), the function depends on every caller passing a path that is already a complete shell word (the expandRemotePath(...) output), as documented in the comment at :916. That is true for the known call sites today, but nothing in the parameter's name or type enforces it — a future caller passing a raw path would reintroduce word-splitting/globbing silently (same no-boot-symptom failure class this PR removes). Renaming the parameter to make the contract explicit (e.g. mutexPathWord) or adding a defensive check would make the invariant self-documenting.

2. The dash-safety property isn't re-pinned by a committed syntax check (minor). The new test asserts includes('sed "s/\\"__PID__\\"/${child}/"') and doesNotMatch(/\$\{lock_json\//), and the PR body reports a manual dash -n pass — but the committed suite doesn't render the payload and run it through a POSIX sh -n/dash -n check. Since the entire fix(desktop): fresh managed-SSH spawns no longer wedge on dash or double-quoted paths (salvage #96061) #96084 lineage exists because a payload that looked fine aborted under dash after spawning, a committed test that builds the payload and syntax-checks it with a real POSIX sh would lock that property against future edits to the same template. Minor hardening, not a defect.

The sed swap itself is safe: ${child} is a pid (digits), so the replacement string contains no sed-special characters, and without the g flag only the first "__PID__" on the single-line JSON is replaced. The fix is correct and well-scoped.

[SZWzz]

Thanks for the detailed review. I addressed both hardening suggestions in commit 9691ba2:

1. Renamed the mutex argument to mutexPathWord to make the caller invariant explicit: it must be a complete, already shell-quoted word. I also separated the inner payload generation from the outer mutex wrapper so the boundary is clearer.

2. Added buildSpawnPayload() and a regression test that renders the actual ownership/lock-metadata payload to a temporary file and runs dash -n against it. This keeps the POSIX shell-safety property covered, including the lockfile PID rewrite.

Local verification passes:

• npm run test --workspace apps/desktop -- electron/remote-lifecycle.test.ts
• npm run check --workspace apps/desktop

[SZWzz]

text Updated the branch after rebasing onto the latest main (105b865).

Refreshed head: c3893f5
Preserved the mutex-path/PID hardening and dash -n regression coverage
Local npm run check --workspace apps/desktop passes, including UI/Electron tests and Linux packaging
The PR is ready for another look. GitHub checks are pending on the new head.

[SZWzz]

@teknium1 Friendly ping — this PR has been sitting at action_required for CI since the rebase on Aug 30 (head c3893f5c6). As a first-time contributor I need a maintainer to approve the workflow runs. The repo-level CI outage from earlier this week appears resolved. Could you approve the runs / take a look when you get a chance? The earlier AI review feedback has been addressed, and all 93 remote-lifecycle tests pass locally.

[girishmithran]

Confirming this reproduces on a macOS Desktop client → Ubuntu remote over SSH.

Symptom matches #96212 exactly: a literal single-quote directory tree gets created under $HOME on every fresh spawn —

$HOME/'/home//.hermes/.hermes-update-in-progress.mutex'

The backend connection itself works (the #96084 fix resolved the boot failure), so this is the residual mutex-path double-quoting, not the reservation-site issue. Deleting the tree doesn't help — it regenerates on the next spawn.

Both fixes in this PR look correct against what I see live: the shq(mutexPath) removal produces a quote-free argv (previously python3 received '/home//.hermes/.hermes-update-in-progress.mutex' with literal quotes and makedirs/flock hit the bogus path), and the quoted-PID → integer pid swap addresses the malformed-pid fail-closed I was watching for.

👍 Would be great to get this merged

[SZWzz]

@girishmithran Thanks for the independent confirmation — that is exactly the residual this PR fixes.

Your analysis matches the defect precisely: after #96084 resolved the boot hang, withRemoteUpdateMutex was the one remaining site still wrapping an already-quoted expandRemotePath() word in shq(), so the remote python opens/flocks the literal-quote path ($HOME'/home/<user>/...mutex') instead of the real sidecar. Since nothing else uses that path, the update mutex silently serializes nothing — which is why the connection works but the stray tree keeps regenerating on every spawn.

With this PR applied, the mutex word is passed raw (${mutexPath} → interpolated unquoted), and argv-level regression tests now pin the exact argv the remote python receives, so the site cannot regress silently again. Local verification on a Linux remote: the real ~/.hermes/.hermes-update-in-progress.mutex sidecar is the one that gets flocked, and no quoted tree is created.

[SZWzz]

Update: branch rebased onto current main (63279301b) with a new hardening commit on top — refreshed head: 8a7c60b98.

fix(desktop): publish a scalar remote spawn pid closes a residual correctness gap in the detached spawn chain:

1. Single-PID guarantee — the inner setsid/nohup shell no longer backgrounds Hermes with its own & echo $!. Previously both the inner and outer shells echoed a PID, so the command substitution captured two lines and $child held a two-line value, which made the lockfile sed substitution invalid under POSIX sh. Now Hermes stays in the foreground of the detached shell and only the outer shell emits a PID.
2. Numeric guard — right after spawn, a case "$child" check fails closed (exit 76) if the captured value is not a pure integer, instead of publishing a corrupt lockfile.
3. Precise sed target + error path — the substitution now matches the exact JSON field ("pid":"__PID__" → "pid":<n>) rather than any quoted __PID__ occurrence, and a sed failure kills the child and exits 76 instead of writing a partial record.

Test coverage grew with it (argv-level assertions pin the single-PID contract and the guard). Local npx vitest run electron/remote-lifecycle.test.ts electron/connection-registry.test.ts: 185 passed, 1 skipped. The PR remains blocked only on the first-time-contributor CI approval gate — happy to address anything the runs surface once approved.

[kshitijk4poor]

@SZWzz — #104206 carries the withRemoteUpdateMutex fix (from #96187, the earliest PR on that site, with your overlap notes on the sibling PRs as context) and is armed for auto-merge, so this PR will conflict on the remote-lifecycle.ts:916 hunk and the test file once it lands.

Your second concern is real and is NOT covered by #104206: I verified on main that the sh-published record is {"schemaVersion":3,"pid":"4242"} (string), so the in-payload reuse regex "pid":[0-9]+ never matches it, and readLockfile would classify it malformed-pid if the client's own writeLockfile() (which runs immediately after spawn) didn't overwrite it first. That's a narrow window but a genuine defect — worth landing on its own. Suggest rebasing this PR down to the pid-integer + scalar-pid commits (c3893f5c69, 8a7c60b98f minus the mutex hunk) once #104206 merges; happy to review that slice. Also noting for the record: your mutexPathWord rename was superseded by keeping the parameter name and documenting the contract in the function's doc comment (per the review on #104206), so no naming conflict remains.

@girishmithran thanks for the independent repro — that's the residual #104206 closes.

[SZWzz]

@kshitijk4poor Rebased onto main after #104206 and removed the overlapping mutex changes. The PR now keeps the PID hardening (integer lockfile PID and scalar detached-spawn PID) plus POSIX dash runtime coverage. Verified with the full Electron test suite (2123 passed, 6 skipped), typecheck, and lint (0 errors).

[alt-glitch]

This was generated by AI during triage.

Partially superseded: the mutex-path double-quoting half landed on main via #104206. The quoted-"pid" lockfile half is still present on main (pid: '__PID__' + sed "s/__PID__/..."), so this PR remains relevant for that fix after a rebase.