Make the root dir the invocation dir, not the config dir - #438
Merged
Conversation
Contributor
Reviewer's GuideThis PR decouples the config file directory from the command execution directory by introducing explicit RootDir and ConfigDir concepts, making commands consistently resolve all file paths against the invocation/root directory while keeping mixin resolution relative to the config location, updating runtime/env behavior, Sequence diagram for resolving RootDir/ConfigDir and running a commandsequenceDiagram
actor User
participant lets
participant FindConfig
participant loadConfigFromFile
participant Config
participant shellRunner
participant Command
participant Envs
participant EnvFiles
User->>lets: invoke lets (configName, configDirFlag)
lets->>FindConfig: FindConfig(configName, configDirFlag)
FindConfig->>FindConfig: getSearchDir(configName, configDirFlag)
FindConfig->>FindConfig: path.GetFullConfigPath / GetFullConfigPathRecursive
FindConfig-->>lets: PathInfo{AbsPath, ConfigDir, RootDir, DotLetsDir}
lets->>loadConfigFromFile: loadConfigFromFile(AbsPath, RootDir, DotLetsDir, displayName, remoteSource, version)
loadConfigFromFile->>Config: NewConfig(RootDir, AbsPath, DotLetsDir)
loadConfigFromFile->>Config: RemoteSource = remoteSource
loadConfigFromFile-->>lets: *Config
User->>lets: run command
lets->>shellRunner: run(command, cmdScript)
shellRunner->>Config: CommandWorkDir(command)
Config-->>shellRunner: workDir
shellRunner->>Command: GetEnv(*Config, workDir, defaultEnv)
Command->>Envs: Execute(shell, workDir, baseEnv)
Envs->>EnvFiles: Load(workDir, filenameEnv)
EnvFiles-->>Command: env from files
Command-->>shellRunner: resolved env
shellRunner->>shellRunner: osCmd.Dir = workDir
shellRunner-->>User: command runs in RootDir or command.WorkDir
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/config/workdir.go" line_range="10-13" />
<code_context>
- workDir, err := os.Getwd()
+// getSearchDir is where lets starts looking for the config file: the process
+// cwd, or rootDir when the user pinned one via --config-dir / LETS_CONFIG_DIR.
+func getSearchDir(filename string, rootDir string) (string, error) {
+ searchDir, err := os.Getwd()
if err != nil {
return "", fmt.Errorf("failed to get workdir for config %s: %w", filename, err)
}
</code_context>
<issue_to_address>
**suggestion:** Error message still refers to "workdir" while the helper is now getSearchDir, which could be confusing.
Please update the error message to refer to the "search dir" (or whatever term you standardize on) instead of "workdir" so it aligns with getSearchDir’s semantics and avoids confusion when debugging config discovery issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+10
to
13
| func getSearchDir(filename string, rootDir string) (string, error) { | ||
| searchDir, err := os.Getwd() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get workdir for config %s: %w", filename, err) |
Contributor
There was a problem hiding this comment.
suggestion: Error message still refers to "workdir" while the helper is now getSearchDir, which could be confusing.
Please update the error message to refer to the "search dir" (or whatever term you standardize on) instead of "workdir" so it aligns with getSearchDir’s semantics and avoids confusion when debugging config discovery issues.
kindermax
force-pushed
the
fix/root-dir-semantics
branch
from
August 10, 2026 11:16
03e7ba6 to
92bc86a
Compare
Config.WorkDir meant two things at once: the directory holding the config file, and the directory commands run in. Every path-resolving directive read the same field, so there was no way to tell which meaning a call site wanted. Split it. ConfigDir is the config file's directory and is read only by mixin resolution; RootDir is where commands run and is read by everything else. Mixin configs now carry the absolute path of the file that declares them, so a nested mixin resolves against its own directory (removes an old TODO). No behavior change: RootDir is still initialised to the config dir.
The root dir is now the directory lets was invoked from, whatever config it loaded and wherever that config lives. A config describes commands; it does not relocate them. Everything a command reads or runs resolves against one directory - that command's working dir, which is the root unless the command sets work_dir. That covers cmd, checksum globs, env_file paths and env.sh. Previously these disagreed: checksum and env_file resolved against the config dir while cmd ran in the config dir and env.sh ran in the cwd, so the same filename in one command definition could mean two directories. Mixins are the exception and stay relative to the config file that declares them - they are an include, and must resolve the same way wherever lets runs. Also: - .lets follows the root, so persisted checksums stay paired with the files they were computed from - remote configs stop being a special case; their root was already the cwd - a remote config declaring a local mixin now errors instead of silently resolving it against the cwd, since its config dir only holds the cached yaml - LETS_CONFIG_DIR is the config's real dir for remote configs too (the cache dir); the project root is now just $PWD
Add a 'Where commands run' section stating the rule and the six ways of pointing lets at a config, and rewrite the statements the previous commit invalidated: work_dir's base, env_file resolution at both scopes, and the remote config note. Changelog records the behaviour changes as breaking - the 0.0.63 entry described this only as a checksum fix.
The previous commits changed several behaviours that had no test: - checksum, env_file and env.sh following work_dir (env_file did the opposite before, and env.sh had no defined base at all) - the same three resolving against the root when no work_dir is set - mixin paths still resolving against the config dir when the root is elsewhere - the root dir of a remote config being the cwd, not the cache dir - a remote config declaring a local mixin being rejected Writing the last of these found a real bug: LoadRemote assigned RemoteSource after decoding, so the rejection never fired during mixin resolution. Set it before decode instead. Drops the changelog claim about nested mixins resolving against the declaring file - recursive mixins are disallowed, so it is not observable.
The previous test only checked that ConfigDir differed from RootDir, which would pass for any wrong-but-different value and never touched the env var a command receives. Pin both to the cache dir, and pin LETS_CONFIG to the URL.
git does not track empty directories, so tests/root_dir/deep/nested vanished on a fresh clone and the recursive-discovery test could not cd into it.
The rules were only reachable from a section buried in the config reference, which is the wrong place for the first question a confused user asks. Give them their own page in the sidebar: the short answer up front, the three directories lets distinguishes, what resolves against what, every way of pointing lets at a config, recipes, and the reasoning behind each choice. The config reference keeps a short summary and links out. Record the decision and the rejected alternatives as ADR-0004, and add Root dir and Config dir to the domain vocabulary - conflating them in one field is what caused this.
kindermax
force-pushed
the
fix/root-dir-semantics
branch
from
August 10, 2026 15:57
bd35cbc to
5ee086d
Compare
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
1 participant
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.
Why
0.0.63changed where commands run, as a side effect of a checksum fix (9859486). Before it, every command silently gotCommand.WorkDir = filepath.Abs("")— the process cwd — which shadowedConfig.WorkDiratrunner.go:48. Adding anif cmd.WorkDir != ""guard un-shadowed a field that had been dead since 2020, and commands started running in the config file's directory:Neither version was coherent. Each resolved paths against two different directories within a single command definition:
0.0.620.0.63cmd:cwdenv.shcwdchecksum:work_direnv_file:work_dir:baseSo on
0.0.62,checksum: [data.txt]andcmd: cat data.txtin the same command could mean two different files. On0.0.63,cmd:andenv.shdisagreed instead.What
One rule, two halves:
mixins:is the only config-assembly directive, and the only thing still relative to the config file — it is an include, so it has to resolve the same way whereverletsruns.cmd:,checksum:,env_file:andenv.shall now share one directory.The root dir is where you ran
lets, whatever config was loaded and wherever it lives:cd proj && lets xprojcd proj && lets -c sub/lets.yaml xprojcd proj/deep && lets x(config found up the tree)proj/deepcd proj/deep && lets -c ../lets.yaml xproj/deepcd proj && lets -c https://…/lets.yaml xproj--config-dir/LETS_CONFIG_DIRstill only steer which config is found. To act on the project rather than your current directory, use$LETS_CONFIG_DIR.Breaking changes
0.0.62and earlier for the common case).checksum:andenv_file:resolve against the command's working dir.env_filepreviously resolved against the config dir at both scopes, and did not followwork_dir..lets/is created in the root dir, so persisted checksums stay paired with the files they were computed from.LETS_CONFIG_DIRfor a remote config is now the local cache dir rather than the cwd.mixinspath is now an error instead of silently resolving against the cwd — its config dir only ever holds the downloaded yaml.Structure
576e8easplitConfig.WorkDirintoConfigDir/RootDir— pure refactor, verified behaviour-identical before anything else moved. The conflation of these two meanings in one field is why the bug existed and went unnoticed for six years.7401d82the semantics.65d219cdocs — new Where commands run section, plus the statements commit 2 invalidated.1974130tests for the changed rules.329c4f9tighten the remoteLETS_CONFIG_DIRassertion.3e4bb42track an empty fixture dir.Testing
163 bats tests pass from a clean clone, unit tests pass,
lets lintclean.New coverage for behaviours that had none:
checksum/env_file/env.shfollowingwork_dirand defaulting to the root; mixins resolving against the config dir when the root is elsewhere; a remote config rooting at the cwd; a remote config rejecting a local mixin.tests/root_dir.batspins each way of pointingletsat a config.Two existing tests asserted the old behaviour and were rewritten rather than deleted —
.letslocation, and checksum-from-a-subdir (that one now has real fixture files with different contents insubdir/, so it proves the resolution rather than observing an empty hash).Writing the remote-mixin test found a real bug:
LoadRemotesetRemoteSourceafter decoding, but mixins resolve during decode, so the new rejection never fired. Fixed in1974130.Notes for review
.lets/moving to the root dir is the judgement call I am least certain of. The argument for it is checksum integrity — a persisted checksum stored next to a config but computed from a different directory would flip-flop. The cost is stray.lets/dirs when running from subdirectories. One line infind.goto flip back.work_diris now relative to where you stand, sowork_dir: docsworks from the project root and fails from a subdirectory. That is inherent to the model. It also does not expand env vars, sowork_dir: ${LETS_CONFIG_DIR}/docsdoes not work; docs recommendcmd: cd "${LETS_CONFIG_DIR}/docs" && …instead. Adding expansion would be a separate feature.config.go:342), so a mixin's ownmixins:block is never read. Commit 2 passes the mixin's absolute path toNewMixinConfig(clearing an oldTODO(maybe bug)), but that has no user-visible effect today.Summary by Sourcery
Align command execution semantics so that commands run from the directory where
letsis invoked, with a clear separation between the config file’s directory and the root working directory.Bug Fixes:
work_dir, checksum, env_file, and env.sh all resolve consistently against the command’s working directory instead of mixing config and invocation directories.Enhancements:
.lets/in the root directory so persisted checksums stay with the files they were computed from.Documentation:
Tests: