lore: Use correct caller working directory for absolute path resolution in the service process#133
Closed
mjansson wants to merge 6 commits into
Closed
lore: Use correct caller working directory for absolute path resolution in the service process#133mjansson wants to merge 6 commits into
mjansson wants to merge 6 commits into
Conversation
mjansson
force-pushed
the
service-working-directory
branch
from
July 20, 2026 14:06
357e20e to
5ec2657
Compare
# Summary A command sent to the service resolved its relative paths in the service process, against whatever working directory the service happened to inherit from whoever started it. `lore clone <url>` from the home directory put the repository under the directory the service was started in instead, because the CLI passes the name derived from the URL as a relative repository path and `make_absolute` joined it to the service's own working directory. This affects every relative path a command carries, not just clone's destination: `make_absolute` is the single point where relative paths are resolved, and each of its callers runs in the service when the call is routed there. ## Changes - `LoreGlobalArgs` gains `working_directory`, which already crosses the wire with the rest of the globals. The CLI fills it with the directory the command was run in. - `make_absolute` resolves against that directory when the call in progress named one, and against this process's own otherwise, so a call executed locally behaves exactly as before. Resolution is per call rather than process state, which matters because the service handles connections concurrently and a shared working directory would let two commands corrupt each other. - `make_absolute_from` takes the base directory explicitly, for the call wrappers that resolve paths before the execution context exists and so cannot look it up. `prepare_repository_call` is one: it absolutizes the repository path before `setup_execution`, and would otherwise still have used the service's directory. - The service now parks its working directory at the filesystem root, rather than keeping whatever it inherited. Callers send the directory their relative paths belong to, so it needs none of its own, and not holding one also keeps it from pinning a filesystem it has no interest in. # Test Plan With the service started from one directory and commands run from another, verified on macOS that both resolution paths now use the caller's directory: - Through the explicit base: `--repository relsub status` over IPC reports the same absolute path as the identical call executed locally. - Through the context lookup, which is the path clone takes: `repository create` with a relative repository path created the repository under the client's directory, and not under the service's. - The service's working directory is the filesystem root, confirmed with `lsof -d cwd`. - `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`, `cargo test -p lore --lib` (90), `cargo test -p lore-revision --lib` (159): all clean. Not verified on Windows, including the `SystemRoot` working directory. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
# Summary An audit of working-directory use across the workspace found two more places in the library that resolve a relative path against the process working directory, which is the service's rather than the caller's when the call is routed there. Both open-code the same join that `make_absolute` performs, so they were not covered by teaching `make_absolute` to use the directory the call carries. ## Changes - `lore-revision/src/file/write.rs`: resolve the output path with `make_absolute` instead of joining `std::env::current_dir` directly, in both places that did so. Writing to a relative output path through the service now writes where the caller asked, rather than under the service's directory. - `clippy.toml`: ban `std::env::current_dir` so the library cannot reacquire this class of bug. See the caveat in the test plan: the file is not currently read, so the entry does not yet take effect. - `make_absolute_from` takes the base directory as a `&Path` rather than an owned `PathBuf`, and `make_absolute` borrows it from the execution context it already holds for the duration of the call. Resolving a relative path no longer allocates a copy of the directory to do so. The remaining uses are deliberate and stay: - `make_absolute` falls back to the process directory when the call does not carry one, which is what a caller executing in its own process wants. - The CLI reads the process directory to find the repository root, to render paths relative for display, and to fill in the directory it sends with each call. Those all run in the process the user invoked, which is the directory they mean. - The service sets its own working directory once at start-up, deliberately. - `lore-server` reads it when resolving its config path, in its own process. # Test Plan - `cargo build -p lore-revision` and `cargo test -p lore-revision --lib` (159 passed): clean. Caveat on the clippy ban: `clippy.toml` at the workspace root is not read for workspace members, which was confirmed by putting invalid TOML in it and seeing clippy report nothing. The existing bans in that file have therefore never been enforced either. Making them take effect needs the config pointed at explicitly, which will surface pre-existing violations of the other bans and is left as a separate change. Known remaining issue, not addressed here: `lore-revision/src/projfs/serve.rs` sets the process working directory while serving a repository. That is process global state in a process that serves several callers at once, so it would disturb concurrent calls. It predates this work and warrants its own change. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
# Summary Nothing covered relative paths crossing to the service. The test harness passes `--repository` as an absolute path and runs commands from the repository root, so no existing test sends a relative path, and the service's own working directory never mattered. The bug this covers was therefore invisible to the suite. ## Changes - `scripts/test/conftest.py`: `lore_service_in_directory` starts the service in a chosen directory, which `background_lore_service` cannot do because it inherits the directory pytest runs in. Both are needed: a test that wants the service's directory to differ from the caller's has to place it deliberately. - `scripts/test/test_service.py`: start the service in one directory, clone to a relative path from another, then stage a relative path from inside the clone. Asserts the clone lands under the caller's directory and, just as importantly, that nothing appears under the service's, so the test fails rather than passes by accident if resolution silently changes again. # Test Plan Not run: the clone step needs a server, which is not available here. Checked statically that the module parses, that every fixture it names is defined, and that the CLI invocations match the argument shapes the existing wrappers build (`clone <url> <path>` positionally, `stage <path>`). The resolution the test asserts was verified by hand on macOS while making the change, with the service started in one directory and commands run from another, through both the explicit and the context resolution paths. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
# Summary The smoke test added alongside this found a second way a relative path reached the wrong directory. `RelativePath::new_from_user_path`, which validates every user supplied path against the repository, resolved with `std::path::absolute`. That reads the process working directory just as `current_dir` does, so a path argument sent to the service was resolved against the service's directory. Staging a relative path through the service therefore staged nothing, and reported success while doing so, because the resolved path fell outside the repository and matched no file. The earlier audit missed this: it searched for `current_dir` and this is a different function reaching the same process state. ## Changes - `lore-revision/src/util/path.rs`: resolve both the user path and the repository path through `make_absolute`, which uses the directory the call carries. # Test Plan - `scripts/test/test_service.py`: 2 passed, 1 skipped, against a running server. The new test fails before this change at the staging step and passes after, which is how the bug was found. - `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`, `cargo test -p lore-revision --lib` (159), `cargo test -p lore --lib` (90): all clean. `std::path::absolute` has no other uses in the library. The two remaining ones are in the CLI, which runs in the directory the user is actually in. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
mjansson
force-pushed
the
service-working-directory
branch
from
July 21, 2026 19:42
30ecf2f to
babfe63
Compare
matt-hoffman-epic
approved these changes
Jul 21, 2026
| pub fn lore_globals_from_args(cli: &LoreCli) -> LoreGlobalArgs { | ||
| let mut args = LoreGlobalArgs { | ||
| repository_path: get_repository_path(cli.repository.clone()), | ||
| working_directory: std::env::current_dir() |
Collaborator
There was a problem hiding this comment.
Rather than having the CLI set this, maybe it should be set in lore/src/remote/call.rs::service_call_impl or lore/src/call_delegation.rs::dispatch_call so that any C API callers using the service pick this up as well.
It can also only overwrite the LoreGlobalArgs::working_directory if its "", so that an installed tool running from C:\Program Files or whatever can set it and have relative paths work as expected as well.
| environment = os.environ.copy() | ||
| environment.update(LORE_SERVICE_ENVIRONMENT) | ||
|
|
||
| def run_in(directory, args): |
Collaborator
There was a problem hiding this comment.
I think this is supported by Lore's .run method already. If you pass a cwd kwarg to any of its functions they'll get passed to .run and used to set the run_cwd variable.
You'd also need to add back in the environment_vars=LORE_SERVICE_ENVIRONMENT.copy() to the new_lore_repo() command.
# Summary The working directory a service call resolves relative paths against was filled in by the CLI. A C API caller that routes through the service did not set it, so its relative paths resolved against the service's directory rather than its own. Move the defaulting into the library, at the point a call is sent to the service, so every caller gets it. A caller that sets the field keeps its value, so an installed tool running from a fixed directory can still direct relative paths elsewhere. ## Changes - `lore/src/remote/call.rs`: `service_call` fills `working_directory` with the process directory when the caller left it unset, before the globals are sent over IPC. - `lore-client/src/cli/cli.rs`: drop the CLI's own assignment, now redundant. A command that runs locally still resolves against the process directory through `make_absolute`'s fallback, so nothing changes for the non-service path. # Test Plan - `uv run pytest scripts/test/test_service.py`: 2 passed, 1 skipped. The relative-path test drives the real binary through the service with the CLI assignment removed, so its passing shows the library now supplies the directory. - `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`, `cargo test -p lore --lib` (92): all clean. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
# Summary The working directory test built its own subprocess calls with a hand-rolled environment. `Lore.run` already takes a `cwd` argument and merges the repository's `environment_vars`, and it also isolates the global config and auth token store per test, which the hand-rolled calls skipped. Route the test through those helpers instead. ## Changes - `scripts/test/test_service.py`: create the source repository with `environment_vars=LORE_SERVICE_ENVIRONMENT`, so every command routes through the service, and use `cwd=` on the clone and the clone's own `stage`/`status` to place each command's directory. Drops the `run_in` closure and its manual environment. - `scripts/test/conftest.py`: `lore_service_in_directory` now starts the service with the test's `LORE_GLOBAL_PATH`, so a shared store the service creates during the clone lands where the client looks for it. # Test Plan - `uv run pytest scripts/test/test_service.py`: 2 passed, 1 skipped, three times, stable. The relative-path test still fails at the staging step when the library resolves against the service's directory and passes once it resolves against the caller's. - `uv run ruff check`: clean. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
epic-lore-bot
Bot
added
imported
|
Imported as Lore CR-234. |
epic-lore-bot Bot
pushed a commit
that referenced
this pull request
Jul 22, 2026
…on in the service process # Summary When executing a command in the service process it must use the working directory of the calling process for resolving relative paths to absolute paths, NOT the service process current working directory. Ensure that all relative -> absolute translation goes through the helper `make_absolute` function and store the "working directory" to use when doing this resolution in the global args, which makes the service process do the correct resolution. Also changes the working directory of the service process to the standard root path. Adds a smoke test that verifies the relative path handling in a service process. # Testing Ran `service start` from one path, changed to a different path and did a `clone <relative-path>`. Verified that the cloned instance was in the correct relative path of the new working directory, not in a path relative to the directory where the service process was started (prior to this change it would end up in the original path where the service process was started from). ``` Imported-PR: #133 Imported-From: ae21456 Imported-Base: 437e727 Imported-Merge: df90bc4 Imported-Author: Mattias Jansson (mjansson) Signed-off-by: Mattias Jansson <mjansson@gmail.com> GH-URL: #133 ``` Lore-RevId: 335 Lore-Signature: ac86e5ef4c260f1c453d91ad10a92f0d9da3f550536e8beea422c6bf8ef6a07e
|
Closed by mirrored commit 9179c6d. |
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
When executing a command in the service process it must use the working directory of the calling process for resolving relative paths to absolute paths, NOT the service process current working directory.
Ensure that all relative -> absolute translation goes through the helper
make_absolutefunction and store the "working directory" to use when doing this resolution in the global args, which makes the service process do the correct resolution.Also changes the working directory of the service process to the standard root path.
Adds a smoke test that verifies the relative path handling in a service process.
Testing
Ran
service startfrom one path, changed to a different path and did aclone <relative-path>. Verified that the cloned instance was in the correct relative path of the new working directory, not in a path relative to the directory where the service process was started (prior to this change it would end up in the original path where the service process was started from).