fix(files): keep local file blobs in the platform data directory - #1233
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughEmpty local storage paths now resolve to the platform user-data ChangesLocal storage path resolution
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nemo_platform_plugin/tests/files/test_storage_config_path.py (1)
22-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
XDG_DATA_HOMEfallback.When
NMP_DATA_DIRis unset,nmp_user_data_dir()usesXDG_DATA_HOMEbefore the home-directory fallback. This test deletesXDG_DATA_HOME, so it does not define behavior on XDG-configured hosts. Add a case with onlyXDG_DATA_HOMEset and assert the intended path. If storage must remain unchanged wheneverNMP_DATA_DIRis unset, this case will expose the current mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_platform_plugin/tests/files/test_storage_config_path.py` around lines 22 - 30, Add a test alongside test_an_empty_path_without_a_data_dir_keeps_the_previous_location that leaves NMP_DATA_DIR unset, sets only XDG_DATA_HOME, and asserts LocalStorageConfig(path="").path matches the intended XDG-based location. Keep HOME and other unrelated environment variables from affecting the case so it specifically verifies the XDG_DATA_HOME fallback and exposes any mismatch with the expected unchanged storage behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nemo_platform_plugin/tests/files/test_storage_config_path.py`:
- Around line 22-30: Add a test alongside
test_an_empty_path_without_a_data_dir_keeps_the_previous_location that leaves
NMP_DATA_DIR unset, sets only XDG_DATA_HOME, and asserts
LocalStorageConfig(path="").path matches the intended XDG-based location. Keep
HOME and other unrelated environment variables from affecting the case so it
specifically verifies the XDG_DATA_HOME fallback and exposes any mismatch with
the expected unchanged storage behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0fe52866-77b4-409f-b8e7-3ecca800cf02
📒 Files selected for processing (4)
packages/nemo_platform_plugin/src/nemo_platform_plugin/files/storage_config.pypackages/nemo_platform_plugin/tests/files/test_storage_config_path.pypackages/nmp_platform/config/local.yamlpackages/nmp_platform_runner/src/nmp/platform_runner/config/local.yaml
Contributor
|
mckornfield
approved these changes
Aug 12, 2026
`NMP_DATA_DIR` relocated the entity-store database but not the Files service: both bundled local configurations pinned the literal `~/.local/share/nemo/files` for blob storage. A local run with the variable set therefore put the database in the chosen directory and the blobs in the default one. A half-isolated instance is worse than an un-isolated one. It looks isolated, so wiping the chosen directory silently leaves the blobs behind — and `SETUP.md` tells users that directory holds "the entity-store database, encryption key, files, job history, secrets", which was not true. `LocalServicesConfig.data_dir` documents the same promise, and the e2e harness already renders the files path as `<data dir>/files`; only the bundled configs disagreed. An empty `path` on `LocalStorageConfig` now means "the platform user-data directory", resolved in the existing field validator, and both local configurations use that instead of a literal path. Resolved in the validator rather than as a field default on purpose: the config-reference documentation is generated from field defaults and committed, so a default derived from the environment would bake the generating machine's home directory into the repository. This mirrors the SQLite path, which is computed in `get_database_url` for the same reason. Behaviour is unchanged for anyone who has not set `NMP_DATA_DIR` — an empty path resolves to exactly the location the configs used to name — and an explicit path still wins, which is what the container image, the Helm chart and the agentic runners all rely on. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
files-storage-data-dir/schapman
branch
from
August 12, 2026 18:53
24d906c to
9850017
Compare
SandyChapman
enabled auto-merge
August 12, 2026 19:26
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 12, 2026
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
2 participants
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.
Summary
NMP_DATA_DIRrelocated the entity-store database but not the Files service, because both bundled local configurations pinned the literal~/.local/share/nemo/filesfor blob storage. Before this change, a local run with the variable set put the database in the chosen directory and the file blobs in the default one; after it, both land under the chosen directory. Behaviour is unchanged whenNMP_DATA_DIRis unset, and an explicit path still wins.A half-isolated instance is worse than an un-isolated one: it looks isolated, so wiping the chosen directory silently leaves the blobs behind. Three places already promised otherwise —
LocalServicesConfig.data_dir("SQLite DB, encryption key, files"),SETUP.md(the wipe warning lists files), andpackages/nmp_testing/tests/unit/test_e2e_harness.py, which asserts the rendered path is<data dir>/files. Only the bundled configs disagreed.Changes
LocalStorageConfig.make_path_relative_to_program: an emptypathnow resolves to<platform user-data dir>/files. Absolute,~-prefixed, and relative paths behave exactly as before.packages/nmp_platform_runner/src/nmp/platform_runner/config/local.yamlandpackages/nmp_platform/config/local.yaml: use the empty sentinel instead of a hard-coded path.NMP_DATA_DIRlocation, explicit paths winning,~expansion, and per-construction resolution.Resolved in the field validator rather than as a field default on purpose:
docs/set-up/config-reference.mdxis generated from field defaults and committed, so a default derived from the environment would bake the generating machine's home directory into the repository. This mirrors the SQLite path, computed inget_database_urlfor the same reason.lint-config-reference-docspasses as a result.Type of Change
Quality Gates
SETUP.mdandLocalServicesConfig.data_diralready document; the generated config reference is unchanged by design.Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
uv run --frozen pytest services/core/files/tests/ packages/nemo_platform_plugin/tests/— 1737 passed, 51 skipped.uv run --frozen pytest packages/nemo_platform_plugin/tests/files/test_storage_config_path.py— 5 passed (the new tests).uv run pre-commit run -a— all substantive hooks pass:ruff,ruff format,Run ty typechecks,Check config reference doc is up to date,Helm Docs Container,Check for uv.lock drift,Fix copyright headers,Plugins must not import from nmp-common,check for merge conflicts. Two hooks could not run in my local environment, which is why the gate above is left unchecked — neither is reported as passing:Run uv lock with platform uv— requires uv 0.9.14; local uv is 0.9.30. Nopyproject.tomlis touched by this PR, the separateCheck for uv.lock drifthook passes, and CI'sCheck uv lockjob passes on this head.Run UI lint-staged—pnpmunavailable locally (untrustedmise.toml). Noweb/files are touched by this PR.tools/lint/lint-all.sh— passes exceptlint-openapi(fails locally onmapfile: command not found, macOS bash 3.2) andlint-web-sdk(same missingpnpm). Neither is affected by this change; no API surface is added.nemo services run --services entities,files:NMP_DATA_DIR=<tmp>, a new fileset resolves to<tmp>/files/filesets/default/<name>(previously~/.local/share/nemo/...).NMP_DATA_DIR→~/.local/share/nemo/files(identical to the value the configs used to hard-code), empty path with it set →<dir>/files, explicit/data/files_storage→ unchanged,~/somewhere→ expanded.Summary by CodeRabbit
New Features
NMP_DATA_DIRenvironment setting.Bug Fixes
Tests