fix(desktop): avoid double-quoting the SSH update mutex path on remote hosts - #99189
fix(desktop): avoid double-quoting the SSH update mutex path on remote hosts#99189twotnguyen wants to merge 1 commit into
Conversation
|
Thanks! |
…e hosts withRemoteUpdateMutex wrapped mutexPath in shq() even though mutexPath is already the output of expandRemotePath() (a shell-quoted string or $HOME fragment). On remote Linux hosts, this passed literal single-quote characters in sys.argv[1], causing Python's os.path.dirname() to treat the path as relative and create a literal ' directory under $HOME. Embed mutexPath directly into the command string so $HOME expands and quote characters are not duplicated, matching the pattern used by other expandRemotePath call sites. Fixes NousResearch#99133
98e59ea to
43e431b
Compare
|
Heads-up on overlap: this fixes the same Whichever one lands fixes the quoting fault, so no argument from me either way — but for whoever reviews: #96260 covers this same one-line mutex change plus two adjacent correctness defects in the same spawn chain that the one-line fix alone leaves behind:
It also pins the contract with argv-level regression tests (asserting the exact argv the remote python receives, under dash), which is the gap that let this site regress silently in the first place — the existing Happy to rebase/adjust if a maintainer prefers to land a minimal one first. |
|
Thanks @twotnguyen — this is the right fix for the right line (#99133's root cause), and it was verified live: the composed spawn command run through a real Closing in favour of #104206, which carries the same one-line change from #96187 (@koltyj, filed 27 Aug — earliest of the three PRs on this site) with cherry-pick authorship, plus its test that parses the command with a real |
`apps/desktop/'` is not a real path. It is a literal single-quote directory
holding a full absolute path as nested subdirectories:
apps/desktop/'/var/folders/5h/.../hermes-update-mutex-LMF9y5/home/.hermes-update-in-progress.mutex'
Both files are 0 bytes, nothing in the tree references them, and the leading
and trailing `'` are part of the filenames. They are the fingerprint of the
double-quoted mutex path in `withRemoteUpdateMutex()`: the path reaches Python
with its shell quotes still attached, so it is treated as CWD-relative and
`os.makedirs()` materialises the whole absolute path under whatever directory
the process happened to be in. Running
`apps/desktop/electron/remote-lifecycle.test.ts` from `apps/desktop`
reproduces it on the spot.
They were swept in by a `git add .` in 3662057, an unrelated menu-label
commit.
This only removes the committed artifact. The generator is a separate concern
already covered by open PRs (#99189, #96187, #96260) and issues (#99133,
#96212, #96188); those repair the quoting but none of them deletes these two
files, so the litter would survive whichever one lands.
Summary
When connecting to a remote host over SSH,
withRemoteUpdateMutexwrappedmutexPathinshq()even thoughmutexPathwas already the output ofexpandRemotePath()(which already produces a shell-quoted string or"$HOME"'/…'fragment).On remote Linux hosts, the extra
shq()passed literal single-quote characters insidesys.argv[1]to the Python mutex helper. When Python evaluatedos.path.dirname(sys.argv[1])on a path like'/home/hermes/.hermes/…', it treated the path as relative because it began with'instead of/. Callingos.makedirs(parent, exist_ok=True)then created a literal directory named'under the user's home directory (e.g./home/hermes/').Changes
apps/desktop/electron/remote-lifecycle.ts, embedmutexPathdirectly intowithRemoteUpdateMutex's command string instead of wrapping it inshq(mutexPath).apps/desktop/electron/remote-lifecycle.test.ts, add regression testbuildSpawnCommand does not double-quote the update mutex pathasserting that both absolute and tildehermesHomepaths produce cleanpython3 -c ... <path>arguments without double-quoting or literal single quotes.Fixes #99133