Repository navigation
fix(opencode): read an empty OPENCODE_CONFIG_DIR as unset, not as a relative path - #4084
Conversation
There was a problem hiding this comment.
Reviewed 24e8878, which has since been force-pushed away. Both findings below are resolved at the current head — see the note at the end.
Two checks are red on this head, and both are the PR's own.
affected tests (windows, advisory) is a real failure, not an advisory flake. empty_override_falls_through_to_xdg panics on must not resolve against the CWD: on Windows PathBuf::from("/xdg").join("opencode") is \xdg\opencode, which has a root but no drive prefix, so Path::is_absolute returns false. The assertion is testing the fixture rather than the resolver — the assert_eq! on the line above already pins the whole returned value, which is what proves the empty override didn't fall through to a relative path. Required test (windows) is still pending on this head and will fail the same way.
codecov/patch is at 93.75%, and the single missed line is closable. The cobertura report for this head has exactly one uncovered line among the 16 counted non-test additions: the bare )?; closing the multi-line call in opencode_plugins_dir, whose only region is the error propagation from home_dir(). Binding the two env reads first puts the ? on a line that also carries the executed call, so the line reports hit — no test contortion, and the call reads more plainly besides. That takes the patch to 16/16.
Superseded. The branch was force-pushed to 9e42190 while this review was being written. That commit inlines the empty-value filter back into opencode_plugins_dir and drops the extracted opencode_config_dir helper along with its three unit tests, replacing them with an integration test that runs the real install from a scratch directory and asserts nothing lands under the invocation path. Both lines above are gone as a result — the Windows is_absolute assertion and the lone )?; no longer exist — so I am not pushing the fixes I said I would. The new head carries neither finding and gets its own review.
…elative path
An exported-but-empty OPENCODE_CONFIG_DIR resolved to PathBuf::from(""), collapsing the install target to a relative plugins/worktrunk.ts under the process CWD. The adjacent XDG_CONFIG_HOME arm already filtered empty values, as do CLAUDE_CONFIG_DIR in config::show and PI_CONFIG_DIR in config::pi.
24e8878 to
9e42190
Compare
worktrunk-bot
left a comment
There was a problem hiding this comment.
The fix and its guard check out: reverting the filter(|s| !s.is_empty()) locally makes the new test fail on the XDG assertion, and restoring it makes it pass. Three things before this merges.
The same empty-value gap is still live where wt writes the user's zsh rc file. line_based_config_paths in src/shell/paths.rs resolves zsh's config as std::env::var("ZDOTDIR").map(PathBuf::from).unwrap_or_else(|_| home.to_path_buf()), so an exported-but-empty ZDOTDIR yields the relative .zshrc — the same shape as the bug fixed here, on a file wt config shell install appends to and uninstall rewrites whole. The consequences are worse than a stray plugin file: ~/.zshrc drops out of the candidate list entirely, so install skips zsh for want of a config location; and where the cwd does hold a .zshrc — a dotfiles repo being the obvious case — the integration line lands in that file instead. scan_for_detection_details in src/shell/detection.rs reads ZDOTDIR the same way, which is what wt config show's shell section reports from, and legacy_nushell_autoload_dirs in src/shell/paths.rs does the same with XDG_CONFIG_HOME (PathBuf::from(xdg_config).join("nushell")), handing uninstall's stranded-file search a relative path. nushell_data_dir_fallback, immediately above it in the same file, already guards its XDG_DATA_HOME read with path.is_absolute(), so the guard is that file's own convention rather than a new idea. Those sites write and rewrite user rc files, so they want their own change rather than being folded in here — but they shouldn't be left standing either.
code-coverage is red on 9e42190 for a reason unrelated to the diff, so codecov/patch never posts for this head. All 4859 tests passed and the job then died merging profiles: invalid instrumentation profile data (file header is corrupt), then error: no profile can be merged. That is a corrupt .profraw — transient infrastructure, not a coverage gap. The changelog push below re-runs it, and the patch number stays unverified until that lands.
No changelog entry. Every comparable fix(…) on main carries one under ## Unreleased → ### Fixed — #4067, #4073, #4061, #4058 — and the symptom here (install reports success while writing into whatever directory wt ran from, and OpenCode never sees the plugin) is what that section is for. Pushing one.
…o-end
The rule was unit-tested in `path.rs`, but the target where a relative
`$XDG_CONFIG_HOME` actually writes a file — `wt config plugins opencode
install` — was not. The integration tests spawn `wt` as a child process,
so `cmd.env("XDG_CONFIG_HOME", …)` sets the child's environment and races
nothing in the binary; this is a sibling of
`test_opencode_install_treats_empty_config_dir_as_unset`, which #4084
added for the same shape one rung up the precedence.
Fails against the pre-fix resolution, passes with the shared guard.
…ectory read (#4104) `wt`'s Gitea support reads `tea`'s config file directly to answer "is tea logged in to this host?" — `is_authed_for` for the `wt switch pr:<n>` forge dispatcher, `has_any_login` for the CI-status backend. `read_tea_config` took `$XDG_CONFIG_HOME` at face value, so an exported-but-empty value turned the lookup into the relative `tea/config.yml`, resolved against whatever directory `wt` was invoked from. The user's real `~/.config/tea/config.yml` was never read, and where the repository happened to contain a `tea/config.yml`, that file was consulted instead. With no logins found, both callers report no Gitea login, and the effect is silent: - `wt list --full` in a Gitea repository drops the CI column (`ci_status/mod.rs` gates the backend on `has_any_login()`). - `wt switch pr:<n>` against a self-hosted host that `tea` is logged in to falls through to the GitHub CLI (`switch.rs`, the forge dispatcher). The [XDG base directory spec](https://specifications.freedesktop.org/basedir-spec/latest/) makes both exclusions: an unset *or empty* value means the `$HOME`-relative default, and a relative path is invalid and must be ignored. ## Why this isn't gitea-specific It isn't, and the first revision was wrong to scope it there. worktrunk read an `XDG_*_HOME` variable at four sites, each joining a subpath onto it, and each had written the rule at a different strictness — so `XDG_CONFIG_HOME=cfg` picked the target by cwd at three of them. The three that mean "the XDG config directory" now delegate to `etcetera::base_strategy::Xdg::config_dir()` — the env read, the spec's absolute-only filter, and the `~/.config` fallback in one call, from the crate worktrunk already resolves its own config with. Each caller's hand-written `~/.config` fallback goes with it, so the fix is net −39 lines. | Site | Variable | Before | After | |---|---|---|---| | `read_tea_config` (`git::remote_ref::gitea`) | `XDG_CONFIG_HOME` | face value | `Xdg::config_dir()` | | `opencode_plugins_dir` (`commands::config::opencode`) | `XDG_CONFIG_HOME` | empty filtered, relative not | `Xdg::config_dir()` | | `legacy_nushell_autoload_dirs` (`shell::paths`) | `XDG_CONFIG_HOME` | face value | `Xdg::config_dir()` | | `nushell_data_dir_fallback` (`shell::paths`) | `XDG_DATA_HOME` | absolute-only, written inline | unchanged | `Xdg` rather than `choose_base_strategy()` at the first three: both tools honor `$XDG_CONFIG_HOME` on every platform, which `Xdg` keeps and the native strategies don't. Their *defaults* differ — OpenCode reads `~/.config` everywhere (`~/Library/Application Support/opencode/` is its managed-settings path, which is why `opencode_plugins_dir` already avoided `dirs::config_dir()`), while tea's default comes from `github.com/adrg/xdg`: `~/Library/Application Support` on macOS, `%LOCALAPPDATA%` on Windows. So `Xdg` matches tea's default on Linux only; `choose_base_strategy()` would give `~/Library/Preferences` and `%APPDATA%`, matching it on neither. worktrunk's own config keeps `choose_base_strategy()`, which is the right answer for a native CLI config dir. That leaves a pre-existing gap this PR doesn't close: on macOS with `$XDG_CONFIG_HOME` unset, `wt` looks in `~/.config/tea/config.yml` while tea's logins are in `~/Library/Application Support/tea/config.yml` — the same silent symptom described above. It is unchanged from `main`, and whether `read_tea_config` should also try tea's native macOS and Windows defaults is a behavior change past this PR's scope. Beyond tea, this fixes two real targets: `wt config plugins opencode install` with a relative `$XDG_CONFIG_HOME` wrote the plugin under the invocation directory (the empty case was fixed in #4084; the relative case was not), and the Nushell uninstall's search for wrappers stranded by older versions searched a relative `nushell/vendor/autoload`. ## The home directory is a precondition `etcetera`'s `Xdg` captures the home directory at construction, so `Xdg::new()` fails when none can be resolved even where `$XDG_CONFIG_HOME` is set and absolute — a container run under a uid with no passwd entry and no `$HOME`. Per review, that is accepted rather than worked around: a machine with no resolvable home has larger problems than worktrunk's XDG reads, and preserving the case means reading the variable by hand ahead of etcetera at every site, which is the duplication this PR deletes. Taking it as a precondition simplifies one more site than the delegation itself did. `read_tea_config` resolved a home directory twice — inside `Xdg::new()` for `~/.config/tea/config.yml`, then again through `path::home_dir()` for the legacy `~/.tea/tea.yml`. Both now come off the one strategy (`xdg.home_dir()`), and the two `if let` chains collapse into one candidate list. The no-home case read neither file before, so nothing changes behaviorally. ## The fourth site stays spelled out `nushell_data_dir_fallback` is the one read that doesn't follow the XDG spec — it mirrors Nushell's own `resolve_xdg_base` (`XDG_DATA_HOME` when absolute, otherwise `dirs::data_dir()`), because etcetera's `Xdg::data_dir()` falls back to `~/.local/share` everywhere, which is the wrong directory on macOS and Windows, while the `Apple`/`Windows` strategies that get those right ignore `XDG_DATA_HOME`. It already applied the absolute-only rule, so it is unchanged here apart from moving to `var_os` for parity with the other sites. An intermediate revision delegated it to [`nu-config`](https://crates.io/crates/nu-config), Nushell's own path-resolution crate, and that was dropped in `a1fe07cb8` after measuring it. The delegation bought drift-proofing, not coverage: `resolve_paths(...).vendor_autoload_dirs.last()` resolves to the same directory the ~15 hand-written lines compute on all three platforms, because nu-config appends the `$XDG_DATA_DIRS` and macOS `/Library/Application Support` entries *before* the data-home one and only the last entry is the write target. Against that it cost eight new `Cargo.lock` entries and ~18.7k lines of source — over half of it the `darling` chain, pulled in for one `#[derive(Setters)]` on nu-config's always-compiled `TestEnv` — and a version lock to a Nushell release, so it would answer for the pinned version rather than the installed one in exactly the case where there is no installed `nu` to check against. Worktrunk already delegates the reachable half: `nu_dirs()` asks a running `nu` for `$nu.vendor-autoload-dirs | last`, and that is what the install writes to whenever `nu` is on PATH. The reinstated lines answer only the case shelling out can't reach — the uninstall scan looking for a wrapper an older worktrunk stranded on a machine where Nushell has since been removed. ## Testing The OpenCode install target — the one site where a bad value writes a file rather than failing to read one — is pinned end-to-end by `test_opencode_install_ignores_relative_xdg_config_home` in `tests/integration_tests/config_show.rs`. Those tests spawn `wt` as a child process, so `cmd.env("XDG_CONFIG_HOME", …)` sets the child's environment and races nothing; it is a sibling of `test_opencode_install_treats_empty_config_dir_as_unset`, which #4084 added for the same shape one rung up the precedence. It fails against the pre-fix resolution and passes now. `test_uninstall_nushell_finds_wrapper_under_xdg_data_home` covers the `$XDG_DATA_HOME` candidate end-to-end. The XDG *config* rule itself is not unit-tested here: etcetera's XDG strategy carries doctests asserting exactly the empty-and-relative behavior, so a local copy would be restating a dependency's contract. ## Overlap with #4085 That PR adds an absolute-only filter inline to `legacy_nushell_autoload_dirs`. This supersedes that hunk — its `ZDOTDIR` change is independent and unaffected, so on rebase #4085 should drop the `shell/paths.rs` hunk and the CHANGELOG sentence about it. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Co-authored-by: worktrunk-bot <worktrunk-bot@users.noreply.github.com>
worktrunk 0.78.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **Hook scripts read renamed context keys**: the JSON piped to every hook drops `worktree`, `repo_root`, `main_worktree`, and `main_worktree_path` in favour of `worktree_path`, `repo_path`, `repo`, and `primary_worktree_path`. Config templates migrate on load, so hooks written with the old names keep working; scripts reading the JSON, `--execute` templates, and `--var` overrides need updating. (Breaking.) ([#4080](max-sixty/worktrunk#4080)) - **`wt config plugins pi` installs for Pi; oh-my-pi moves to `wt config plugins omp`**: `pi install` wrote an oh-my-pi hook, which Pi never loads; Pi users re-run it to get a Pi extension. Existing oh-my-pi hooks keep working, listed as outdated until `wt config plugins omp install`. (Breaking: scripts that run `wt config plugins pi` for oh-my-pi need `omp`.) [Docs](https://worktrunk.dev/claude-code/#oh-my-pi) ([#4135](max-sixty/worktrunk#4135), thanks @ashebanow) - **Interactive prompts no longer open with a blank line**: `wt config shell install`, `wt config plugins claude install`, and the commit-message setup offer in `wt merge` began with one. ([#4059](max-sixty/worktrunk#4059)) - **`wt config show` gives a single `wt config shell install` hint**: an outdated wrapper, a fish wrapper at the deprecated `conf.d` path, and missing fish completions each printed their own hint. The zsh `compinit` snippet is now syntax-highlighted. ([#4059](max-sixty/worktrunk#4059)) - **`wt config update --output <path>` confirms the write**: it now prints `✓ Wrote user config migration @ ~/migrated.toml` where it printed nothing. `--output=-` stays silent. ([#4053](max-sixty/worktrunk#4053)) ### Fixed - **Removal no longer discards untracked files hidden by `status.showUntrackedFiles = no`**: under that setting, a worktree holding only untracked files looked clean, so `wt remove`, `wt merge`, and `wt step prune` removed it without `--force`. Safety checks now always count untracked files, and auto-staging warnings list every file they stage. ([#4111](max-sixty/worktrunk#4111), thanks @Duang777) - **`wt remove` and `wt merge` check the right worktree under an inherited `GIT_DIR`**: run from a `!wt` git alias or a git hook, which pass `GIT_DIR` down, `wt remove` could delete a dirty worktree without `--force`, and `wt merge` could refuse a clean merge. Both now check the worktree they act on. ([#4082](max-sixty/worktrunk#4082), thanks @Duang777) - **`wt merge` no longer deletes a worktree locked with `git worktree lock`**: the merge completes and prints `Worktree preserved (locked)`. Every Worktrunk removal refuses a locked worktree, even with `--force`; run `git worktree unlock <path>` first. ([#4073](max-sixty/worktrunk#4073), thanks @Duang777) - **`wt config update` no longer overwrites a config edited while its prompt is open**: it wrote the pre-prompt snapshot over the newer file and reported success. It now fails with the path and leaves the newer file; re-running previews the current contents. ([#4127](max-sixty/worktrunk#4127), thanks @zach-hammad-vs for reporting) - **`wt step prune` no longer deletes a branch created minutes ago from an older commit**: `--min-age` aged a branch without a worktree by its commit's date, so a new branch off a day-old commit was pruned on the next run. Age now comes from the branch's oldest reflog entry. ([#4077](max-sixty/worktrunk#4077)) - **A comment or blank line above an inline config section no longer breaks the config**: a save or migration that rewrote `commit = { … }` as `[commit]` put that line inside the brackets. `wt switch`, `wt merge`, `wt remove`, and several `wt step` commands then failed with `Failed to load config`; `wt list` warned and ran without the config. Saves also dropped unrecognized keys from inline sections. ([#4120](max-sixty/worktrunk#4120), thanks @zach-hammad-vs for reporting) - **Config saves keep the trailing comment on a value they rewrite**: a save such as answering the commit-generation offer dropped the comment after each value it rewrote, including one after an inline section's closing brace. ([#4138](max-sixty/worktrunk#4138)) - **Ignored project-config keys warn from every worktree**: an unknown key in `.config/wt.toml`, or one that belongs in user config such as `[merge]`, warned only in the main worktree, so `wt merge` from a linked worktree ignored it silently. ([#4145](max-sixty/worktrunk#4145), thanks @ee-prog for reporting) - **`wt merge` skips `post-commit` when it removes that hook's worktree**: before, the hook ran in the primary worktree when the removed one sat inside the repository, and otherwise didn't run. The merge now prints `▲ Skipped post-commit`; move work that must finish there to `pre-remove`. ([#4049](max-sixty/worktrunk#4049)) - **`wt switch` recovers from a removed worktree in a bare repository**: `wt switch` and the picker surfaced git's raw `fatal: Unable to read current working directory` instead, and the removed-directory message lacked `wt switch ^`. ([#4067](max-sixty/worktrunk#4067)) - **`wt config shell install fish` puts the wrapper where fish reads it under a custom `$XDG_CONFIG_HOME`**: the wrapper always went to `~/.config/fish/functions/`, so install reported success while `wt switch` changed no directory. On Windows, the fish completion moves from `%APPDATA%\fish\completions` to the wrapper's directory. ([#4107](max-sixty/worktrunk#4107)) - **`wt config shell install zsh` honours `$ZDOTDIR` only when it is absolute**: an empty or relative value resolved against the current directory, so install appended to a `.zshrc` under the current directory and reported success while the file zsh reads went untouched. A non-absolute value now falls back to `$HOME`. ([#4085](max-sixty/worktrunk#4085)) - **`wt config shell install` for Nushell no longer deletes a `wt.nu` under the current directory**: an empty or relative `$XDG_CONFIG_HOME` resolved against it, so install removed a `nushell/vendor/autoload/wt.nu` beneath it unread, as a stale wrapper. It now falls back to `~/.config`, which also fixes unread `tea` logins that let `wt switch pr:<n>` treat a Gitea host as GitHub. ([#4104](max-sixty/worktrunk#4104)) - **`wt switch --create` names the branch a failed worktree add leaves behind**: `git worktree add -b` writes the branch ref before populating the worktree, so a failure in between left the branch with no worktree, and the next attempt reported a name collision. The error now says how to delete or reuse it. ([#4109](max-sixty/worktrunk#4109), thanks @technicalpickles for reporting) - **`wt config shell install` no longer tells an already-wrapped shell to restart**: reinstalling from a shell with the wrapper loaded, such as after a version bump, printed `↳ Restart shell to activate shell integration`. ([#4059](max-sixty/worktrunk#4059)) - **`wt config shell install` migrates a deprecated `conf.d` fish wrapper when `~/.config/fish/functions` is missing**: before, only `wt config shell install fish` migrated it. `wt config show` now reports that wrapper even when fish isn't on `PATH`. ([#4059](max-sixty/worktrunk#4059)) - **Plugin status and uninstall checks ask the agent CLIs**: `wt config show` could list an installed Claude Code plugin or Gemini CLI extension as not installed, and `wt config plugins claude|codex uninstall` could report success over a marketplace still configured. A repeat Claude uninstall now runs both removals instead of stopping at `Plugin not installed`. ([#4048](max-sixty/worktrunk#4048), [#4054](max-sixty/worktrunk#4054)) - **`wt config plugins opencode install` no longer writes the plugin into the current directory**: with `OPENCODE_CONFIG_DIR` set but empty, it wrote `plugins/worktrunk.ts` there, typically inside a repository, reporting success though OpenCode never saw it. An empty value now counts as unset. ([#4084](max-sixty/worktrunk#4084)) - **The OpenCode plugin clears its activity marker when OpenCode 1 disposes it**: only explicit session deletion cleared it, so a finished session kept its `💬` and `wt list` showed it as active until `wt config state marker clear` was run by hand. ([#4129](max-sixty/worktrunk#4129), thanks @drewdas for reporting) - **`wt config update` migrates `[ci] platform` into a `[forge]` section that only sets `hostname`**: that config, common with GitHub Enterprise and self-hosted GitLab, kept the deprecated key with no deprecation warning. ([#4061](max-sixty/worktrunk#4061)) - **Deprecated keys in an inline `[projects]` entry now migrate**: `"example.com/org/repo" = { merge = { no-ff = true } }` skipped the migration the section form got, so the per-project setting was ignored. ([#4122](max-sixty/worktrunk#4122), thanks @zach-hammad-vs for reporting) - **A deprecated config section the migration can't rewrite now warns**: `select = "not a table"`, or a `[select]` whose `[switch.picker]` destination is already set, was ignored without a message. It now reports `unknown field select (will be ignored)`. ([#4121](max-sixty/worktrunk#4121), thanks @zach-hammad-vs for reporting) - **`wt config update` no longer changes what a template renders**: it could rename `{{ repo_root }}` inside a quoted s See merge request: Harmonybrew/homebrew-core!20435
wt config plugins opencode installwrites the plugin to a relativeplugins/worktrunk.ts— i.e. into whatever directorywtwas run from, typically the user's repo — whenOPENCODE_CONFIG_DIRis exported with an empty value.is_plugin_installed()andplugin_file_exists()then read that same relative path back, so the install reports success while OpenCode never sees the plugin.The cause is a one-branch gap in
opencode_plugins_dir: theXDG_CONFIG_HOMEarm filtered empty values, theOPENCODE_CONFIG_DIRarm above it did not, soPathBuf::from("")passed through as a valid config directory. Every other agent-config override in the tree already reads an empty value as unset —CLAUDE_CONFIG_DIRincommands::config::show,PI_CODING_AGENT_DIRandPI_CONFIG_DIRincommands::config::pi— so this arm was the odd one out rather than a deliberate choice.The documented precedence is unchanged:
$OPENCODE_CONFIG_DIR>$XDG_CONFIG_HOME/opencode>~/.config/opencode. Only the reading of an empty value moves, from "a directory named""" to "unset".The regression test runs the real install from a scratch directory with
OPENCODE_CONFIG_DIR="", then asserts the plugin landed under$XDG_CONFIG_HOME/opencode/plugins/and that nothing was written relative to the invocation directory. It fails against the previous code on the first assertion.