fix: resolve a leading ~ in every configured path, not just some - #986
Open
selfcontained wants to merge 6 commits into
Open
fix: resolve a leading ~ in every configured path, not just some#986selfcontained wants to merge 6 commits into
~ in every configured path, not just some#986selfcontained wants to merge 6 commits into
Conversation
- Derive the legacy/expanded media paths from the install's effective MEDIA_ROOT (--media-root, env, or <install>/.env) instead of hardcoding ~/.dispatch/media. A non-default tilde root such as ~/dispatch-media was previously reported as migrated while its media stayed unreadable. - Exit 0 as an explicit no-op when MEDIA_ROOT is absolute: those installs were never affected and must not incur a stop-all-agents outage. - Replace `mv` with an atomic no-clobber link(2) placement (plus a staged cross-device fallback). The preflight checks alone could not hold: the Dispatch server writes media while the migration runs, and `mv` silently overwrote a destination file that appeared in that window. - Materialize both `find` traversals and check their exit status. Streaming through a process substitution hid a failed scan, so an unreadable directory produced a zero-conflict summary and exit 0 — which the manifest treats as permission to run --apply. - Walk the ancestor-safety check over the derived destination components, and return an explicit success status from that function. - Manifest: branch on MEDIA_ROOT before stopping anything, replace the unevaluable "launched before this release" guard with stop-all-agents, and correct the rollback guidance — after --apply a plain rollback passes its health check with every migrated file unreadable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ides Round-2 review findings on the legacy media migrator. Ancestor traversal now fails closed. The previous round made the final component atomic via link(2), but a path re-resolves on every syscall, so an ancestor swapped for a symlink after the preflight still redirected the write outside the media tree (reproduced: the file landed outside and the source was deleted). Placement now `cd -P`s into the destination directory once and links under a single-component name — the shell's cwd is a kernel-held directory reference, so a later swap of the parent path cannot redirect it — and the containment check runs after the cd on the physical path actually reached, so a swap that already happened is refused rather than followed. MEDIA_ROOT is no longer classified from .env alone. dotenv does not override a value already present in the process environment, so a systemd Environment= or launchd EnvironmentVariables entry is what the service actually ran with; the script now reads those first. And a literal-tilde tree that the readable config cannot explain is treated as evidence of an override this script cannot see: it exits 1 asking for --media-root instead of silently declaring the install unaffected and stranding the files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hand-parsing the unit file missed two sources systemd merges itself: dispatch.service.d/*.conf drop-ins and EnvironmentFile=. A tilde root in either one, with an absolute .env and no legacy tree on disk yet, produced a false no-op — and a still-running agent would then recreate the tree. Ask systemd instead of reimplementing its merge: read the live process environment via MainPID when the service is running, else `systemctl show` for the merged Environment= (drop-ins included) and the EnvironmentFiles it reports. launchd needs no equivalent — its plist has no drop-in mechanism. When a systemd service exists but cannot be interrogated, exit 1 asking for --media-root rather than falling back to .env: physical evidence cannot catch a wrong verdict here, because the legacy tree does not exist yet. Two bugs in the previous parser fixed along the way: a no-match `grep` tripped pipefail and aborted the lookup, and the status global was assigned inside a command substitution so it never reached the caller. Drops the unit-file-parsing test, whose fixture no longer describes reachable behavior: a unit present without a usable systemctl is genuinely unresolvable and now fails closed. Its intent is covered by the drop-in test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…default Investigation showed this was never a bad default Dispatch shipped. The code default has been absolute since the initial commit, and install-dispatch.sh never writes MEDIA_ROOT into .env, so a stock install cannot hit the bug. It requires an operator to set MEDIA_ROOT to a tilde path explicitly. The actual vector was the operations runbook, which documented MEDIA_ROOT's default as `~/.dispatch/media` — not what the code does, and precisely the value that breaks things when copied into .env. Corrected to $HOME/... with a note that a leading tilde is expanded but an absolute path is preferred. With no known affected installs, a required assisted update that stops all agents was disproportionate, so the manifest, the assisted-update metadata, the bash migrator and its tests are removed. What remains is the actual bug fix: a leading `~` in a storage path is expanded rather than treated as a directory named "~". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
~ in shared-media paths and migrate legacy literal-tilde trees~ in shared-media paths
Expanding `~` for MEDIA_ROOT alone was a half-measure. Dispatch reads ten path-ish env vars and only TLS_CERT/TLS_KEY expanded a leading tilde, so the same `~/foo` string worked for a TLS cert and silently produced a directory *named* `~` for media — writes succeeding where nothing could find them again. Fixing one var moved that inconsistency rather than removing it. Rather than add a third tilde helper, this builds on the one already in the repo. `shared/lib/resolve-tilde.ts` gains `resolveConfiguredPath` — resolveTilde plus path.resolve — and the duplicate `resolveStoragePath` added to shared/media.ts is deleted. Every configured path now goes through it: MEDIA_ROOT, TLS_CERT/KEY, the four store paths, the release cache dir, the runtime path, the service definition path, the authoring repo dir, and DISPATCH_SERVER_DIR. Defaults were already absolute via os.homedir(), so only the configured branch changes behaviour; path.resolve on an absolute path is a no-op. Tests cover the helper directly and assert two representative stores — one resolving per call, one at module load — write to the expanded location and never create a literal `~` directory. All three fail against the unfixed stores. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
~ in shared-media paths~ in every configured path, not just some
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Dispatch reads ten path-ish values from configuration. Only
TLS_CERTandTLS_KEYexpanded a leading~, via a localexpandHome()helper inconfig.ts.MEDIA_ROOTdid not.So the same
~/foostring meant two different things depending on which variable you wrote it in. For media it became a directory literally named~beside the process working directory — writes succeeded, and nothing could ever find them again. Silent, not loud.Configuration is not read by a shell, so a
~arrives as a literal character; the only question is whether the program expands it. Dispatch had already answered yes —resolveTildeexists inshared/lib/and is used for user-supplied paths inroutes/jobs.ts,routes/templates.tsandroutes/system.ts— it just wasn't applied to configured paths consistently.Before / after, from the same live endpoint
An agent row with
media_dir = '~/dispatch-devtest-media/agt_legacytilde'and the real file present at the expanded location:GET /api/v1/agents/{id}/media/legacy-shot.pngdispatch_list_media→filePath404 {"error":"Media file not found."}~/dispatch-devtest-media/agt_legacytilde/legacy-shot.png200, correct body/Users/brad/dispatch-devtest-media/agt_legacytilde/legacy-shot.pngThe fix
One resolver, applied everywhere a path comes from configuration.
apps/server/src/shared/lib/resolve-tilde.tsgainsresolveConfiguredPath(value)—resolveTildepluspath.resolve. It builds on the helper already in the repo rather than adding a third one.MEDIA_ROOT,TLS_CERT/TLS_KEY, the four store paths (DISPATCH_APPLIED_MIGRATIONS_STORE_PATH,DISPATCH_ASSISTED_UPDATE_STORE_PATH,DISPATCH_RELEASE_CANDIDATE_STORE_PATH,DISPATCH_RELEASE_STORE_PATH),DISPATCH_RELEASE_CACHE_DIR,DISPATCH_RUNTIME_PATH,DISPATCH_SERVICE_DEFINITION_PATH,DISPATCH_RELEASE_AUTHORING_REPO_DIR, andDISPATCH_SERVER_DIR.resolveMediaDir()andlistMedia()also resolve the storedmedia_dircolumn, which inherits its value frommediaRoot.config.tsdrops its localexpandHome();process.env.HOME ?? "/tmp"becomesos.homedir().docs/10-operations-runbook.md— the runbook documentedMEDIA_ROOT's default as~/.dispatch/media, which is both wrong (the code default is absolute) and precisely the value that triggers the bug when copied into.env. Corrected.Behaviour is unchanged for existing installs: every default was already absolute via
os.homedir(), so only the configured branch is affected, andpath.resolveon an absolute path is a no-op.Why there is no migration
Earlier revisions of this branch carried a
requiredassisted update, abin/migrate-legacy-mediabash migrator and a manifest. All removed:config.tshas usedpath.join(HOME, ".dispatch", "media")since the initial commit (v0.11.12), andbin/install-dispatch.shnever writesMEDIA_ROOTinto.env. A stock install cannot reach the bug.MEDIA_ROOTis unset and the only literal-tilde directory present is empty — zero files.A required update that stops every agent to migrate nothing was the wrong trade.
Verification
pnpm run checkclean;pnpm run test— 2808 passing;pnpm run test:e2e— 181 passed, 12 skipped.dispatch_sharewrite path creating no literal~directory.apps/server/test/configured-paths.test.tscovers both resolution shapes present in the codebase — one store resolving per call, one at module load — asserting the file lands at the expanded location and that no~directory is created. All three were confirmed to fail against the unfixed stores.apps/server/test/resolve-tilde.test.ts.🤖 Generated with Claude Code