fix(server): expand ~ and anchor relative package locations at the config#897
fix(server): expand ~ and anchor relative package locations at the config#897mlennie wants to merge 5 commits into
Conversation
…nfig
A package `location` had two ways to surprise you.
`~/my-packages/sales` was accepted by isLocalPath but never expanded, and a
tilde path is not absolute, so it was joined onto the server root and died with
`Failed to mount local directory "<serverRoot>/~/my-packages/sales"`. Tilde is
the most natural way to write a path outside the repo and the code advertised
support for it.
A relative location anchored at the server root rather than at the config it
appears in. Those are the same directory whenever the config is found at
`<serverRoot>/publisher.config.json`, which is every config in this repo and
every Docker recipe in docs/deployment.md, so nothing there changes. They
diverge whenever `--config <path>` names a file in some other directory, a
subdirectory of the server root included: the form implied by
configuration.md ("point at a directory that holds your own
publisher.config.json + packages") did not work, because launched from
elsewhere `./sales` resolved next to the server root rather than next to the
config. Anchoring at the config's directory lets a config and its packages move
together, which is what makes one worth committing instead of carrying absolute
machine-specific paths.
This is a behaviour change for those users: a relative location now resolves
next to the config. The old rule was undocumented rather than deliberate, so
nothing promised it. The symptom if it bites is quiet, because a location that
cannot be mounted is not fatal to the process: the server still reports
`serving`, but it fails the whole environment, so every package in it is gone,
not just the one that moved. RELEASE_NOTES carries the migration note.
Two cases keep the server root as the anchor, as before: the bundled default,
which ships inside the installed package, so a relative location POSTed at
runtime under a zero-arg `npx @malloy-publisher/server` would otherwise resolve
somewhere under node_modules; and a `--config` naming a directory rather than a
file, which cannot be read as a config at all, so its parent is nobody's
intended anchor. The anchor is resolved absolute, since `--config` may itself be
relative and a relative anchor just moves the cwd dependence elsewhere.
Both call sites share one pure helper rather than repeating the expression,
which the comment at the second site previously had to describe in prose to
keep the two in sync.
Docs: configuration.md gains the rule, a config example that parses, and a
SERVER_ROOT row that no longer claims to be the config's home when `--config`
says otherwise. The `malloy-html-data-apps` skill said a relative location is
"relative to `--server_root`", which this change falsifies; it is served to
agents as an MCP prompt via skills_bundle.json, so the bundle is regenerated
too.
Verified against a live server, including a real package (`examples/storefront`,
parquet and all) copied outside the repo and served from a config beside it: it
loads, its `duckdb.table('data/*.parquet')` sources resolve, and its notebook
and `public/` page serve. A `~/` location, a `--config` bundle with relative
locations, and a relative `--config` each failed before and serve after; the
documented example serves when taken verbatim from the docs; a `--config`
naming a directory boots with no environments rather than a bogus anchor; and
the bundled examples still load from packages/server unchanged. The anchoring
has its own spec that puts the config and the server root in different
directories, since a spec that lets them coincide cannot tell the two rules
apart.
Signed-off-by: Monty Lennie <[email protected]>
`src/logger.ts` imports winston, but no package.json in the repo declares it. It resolves today only because `@malloydata/db-snowflake` and `@malloydata/db-databricks` pull it in through `snowflake-sdk` and `@databricks/sql`, and winston is not in the bundler's external list, so `bun build` inlines it from whatever the driver tree happened to hoist. The day a driver drops it, the build stops resolving a module the server imports directly. Declare the version already resolving, so this records what is in use rather than moving anything. The only resolution change is @colors/colors hoisting: winston wants ^1.6.0 while cli-table3 pins 1.5.0 exactly (an optional dependency), so 1.6.0 takes the top level and cli-table3 keeps its own nested copy. Both versions were already in the tree, and the set of resolved packages is unchanged. Signed-off-by: Monty Lennie <[email protected]>
6c9edbe to
d8dd2b6
Compare
|
Can you add info about |
Peer ReviewWell-shaped fix overall: resolution is centralized in one Important1.
Fix both by resolving home lazily inside the const home = homeDir ?? os.homedir();
if (!home) throw new Error(`Cannot expand "~" in location "${location}": home directory is not set`);Keep the eager default off the non-tilde path. Nits
Security / containmentNo new bypass. The local mount source was never confined to the server root — |
kylenesbit
left a comment
There was a problem hiding this comment.
Reviewed the full diff and verified the behaviour independently rather than from the description alone. Approving — this is exactly the fix the outside-the-repo package flow needed. Two small items below are worth folding in before merge.
What I verified
- Live smoke test with the built branch: booted the server from an unrelated cwd with
--config <elsewhere>/my-data/publisher.config.json, one package at./salesbeside the config and one at~/pkg-tildeunder an overriddenHOME. Both mounted and served, and status reported both packages — the exact shape the newdocs/configuration.mdexample promises. - Tests: the three affected spec files pass locally (157 pass, 0 fail); all 14 CI checks are green, including the Windows matrix and the Docker smoke test.
- Shape: resolution now lives in one exported helper (
resolvePackageLocation) used by both call sites that previously kept hand-synced comments pointing at each other, and the bundled-default / config-names-a-directory fallbacks are each pinned by their own test. Containment is unchanged — same conclusion as the peer review above on the security angle.
Two items before merge
- @jswir's
os.homedir()point is valid — please take it. The default parameter runsos.homedir()on every resolution, including plain./salesand absolute paths. I probed macOS: unset or emptyHOMEstill resolves via the passwd entry, so it's harmless here — but in a distroless container running as an arbitrary UID with no passwd row it throws, and Docker is a first-class deployment path in this repo. Moving the call inside the~/branch and guarding an empty result is a five-line change; the nits (negative assertion in the anchoring spec, pinning the bare-~rejection) are cheap too. - @sagarswamirao's ask is still open:
PUBLISHER_USE_BUNDLED_DEFAULTis absent from theconfiguration.mdenv-var table, and this change makes that knob more load-bearing (it's one of the cases that decides the anchor). One table row closes it.
One judgment call I'd accept, noting it for the record: the behaviour change also applies to locations POSTed at runtime (they anchor at the config dir when a --config is active), and when the change bites an existing setup the symptom is the same quiet environment-skip. The release note covers migration, the old rule was undocumented, and the affected population — --config pointing away from the server root plus relative locations — is exactly who this fix exists for.
…medir() Review feedback on the anchoring change. resolvePackageLocation took the home directory as a defaulted parameter, so os.homedir() ran on every resolution, including `./sales` and absolute locations that never contain a tilde. That has two failure modes on the common non-tilde path: os.homedir() throws where HOME is unset and the uid has no passwd entry (a distroless container with runAsUser), failing an environment that used to load; and an empty-string home turns `~/foo` into a relative "foo" that silently anchors under the config dir instead of failing. Resolve the home lazily inside the `~/` branch only, and throw a named error when it comes back empty, so a bad home can fail only the tilde locations that actually need it. Tests pin each behaviour: a `~/` location with an empty home throws; the same empty home is irrelevant to `./` and absolute locations; and only the POSIX `~/` prefix expands, with bare `~` and `~user/...` left as the ordinary non-local names they are (a note by FilesystemPath records the POSIX-only choice). The anchoring spec also gains the negative it was missing: a decoy `sales` package now sits at the server root with different contents, so a silent revert to server-root anchoring mounts the decoy and fails the content assertion, where the existence check alone would still have passed. Verified by mutation: reverting the anchor to serverRootPath fails that spec. Signed-off-by: Kyle Nesbit <[email protected]> Co-authored-by: Cursor <[email protected]>
Requested in review. The knob decides whether a config-less start falls back to the sample config bundled inside the installed package, and the anchoring change makes it more load-bearing: bundled-default mode is one of the two cases where a relative package location keeps the server root as its anchor. The row records who sets it (the server itself, on a zero-flag start), what disables it (--config or --server_root), and the anchoring consequence. Signed-off-by: Kyle Nesbit <[email protected]> Co-authored-by: Cursor <[email protected]>
|
Pushed the two review items directly onto the branch so this can merge without another round trip:
Locally green on top of the branch: full unit suite (1141 pass, 0 fail), lint, prettier. The Full review with the live smoke-test verification is in the approval above. @mlennie once checks are green this is good to merge from my side. |
The Analyze (python) job failed before analyzing anything: CodeQL's
init step could not reach GitHub ("No server is currently available"),
during a window where the Actions API was also returning 503s. The run
refuses API retries, so an empty commit retriggers the checks.
Signed-off-by: Kyle Nesbit <[email protected]>
Co-authored-by: Cursor <[email protected]>
Two small fixes so a package can live outside the repo, which is where most people want theirs.
That already worked, but only if you had read the code. Two things stood in the way.
~/was accepted and then never expanded.isLocalPathmatches~/explicitly, but a tilde path is not absolute, so it got joined onto the server root and died withFailed to mount local directory "<serverRoot>/~/my-packages/sales". Tilde is the most natural way to write a path outside the repo, and the code advertised support for it.A relative
locationanchored at the server root, not at the config it appears in. Those are the same directory whenever the config is found at<serverRoot>/publisher.config.json, which covers every config in this repo and every Docker recipe indocs/deployment.md, so nothing changes for them. They diverge under--config <path>: the formdocs/configuration.mddescribes ("point at a directory that holds your own publisher.config.json + packages") did not work, because launched from elsewhere./salesresolved next to the server root rather than next to the config. Anchoring at the config's directory lets a config and its packages move together, which is what makes one worth committing instead of carrying absolute machine-specific paths.This is a behaviour change for anyone whose
--config <path>names a file in a directory other than the server root, a subdirectory of it included, and whose packages use a relativelocation. The old rule was undocumented rather than deliberate, so nothing promised it. The symptom if it bites is quiet: a location that cannot be mounted is not fatal to the process, so the server still reportsserving, but it fails the whole environment, so every package in it is gone rather than just the one that moved.RELEASE_NOTES.mdcarries the migration note.Two cases keep the server root as the anchor: the bundled default, which ships inside the installed package, so a relative location POSTed at runtime under a zero-arg
npx @malloy-publisher/serverwould otherwise resolve somewhere undernode_modules; and a--confignaming a directory rather than a file, which cannot be read as a config at all, so its parent is nobody's intended anchor.The second commit declares
winston.packages/server/src/logger.tsimports it and no package.json in the repo declares it: it resolves only because the Snowflake and Databricks drivers pull it in transitively, and it is not in the bundler's external list, sobun buildinlines whatever the driver tree happened to hoist. The day a driver drops it, the build stops resolving a module the server imports directly. The pinned version is the one already in the lock, so the resolved package set is unchanged.Docs
docs/configuration.mdgains the rule and a config example that parses, and itsSERVER_ROOTrow no longer claims to be the config's home when--configsays otherwise. Themalloy-html-data-appsskill said a relative location is "relative to--server_root", which this change falsifies; that text is served to agents as an MCP prompt viaskills_bundle.json, so the bundle is regenerated too.Testing
Gate green: 1138 unit, 207 integration, 0 fail. Verified against a live server rather than from the test suite alone, because a load failure is quiet: the server reports
servingeither way, omitting the package if it mounted but is bad, or dropping the whole environment if the location could not be mounted at all.~/location, a--configbundle with relative locations, and a relative--configeach failed before and serve after--confignaming a directory boots with no environments rather than a bogus anchorpackages/serverunchanged, which is the no-op claim aboveexamples/storefront, parquet and all) copied outside the repo and served from a config beside it: it loads, itsduckdb.table('data/*.parquet')sources resolve to the same totals as in-repo, and its notebook andpublic/page serveThe anchoring has its own spec that puts the config and the server root in different directories, since a spec that lets them coincide cannot tell the two rules apart. Each behaviour is mutation-tested: reverting the tilde expansion, the anchor, the bundled-default guard, or the absolute-anchor resolve each fails its own test.