Skip to content

fix(shell): detect the current shell from the process tree, not $SHELL - #3455

Merged
max-sixty merged 6 commits into
mainfrom
shell-restart-detection
Jul 14, 2026
Merged

fix(shell): detect the current shell from the process tree, not $SHELL#3455
max-sixty merged 6 commits into
mainfrom
shell-restart-detection

Conversation

@max-sixty

@max-sixty max-sixty commented Jul 13, 2026

Copy link
Copy Markdown
Owner

wt switch could tell a fish user "shell requires restart" because $SHELL (the login shell, say zsh) had the integration line while the shell actually in use didn't. current_shell() keyed everything on $SHELL, which names the login shell rather than the shell wt was invoked from.

Process-tree detection. shell::ancestor_shell() (src/shell/utils.rs) walks up from wt's parent to the nearest enclosing shell: Linux reads /proc/<pid>/stat; macOS parses one ps -Ao pid=,ppid=,comm= snapshot. macOS cannot use per-pid p_comm (libproc/sysctl): /bin/sh re-execs bash via /var/select/sh, so an sh script's p_comm reads "bash", while ps's comm preserves argv[0]. Verified live and pinned by test_probe_reports_invoked_name_for_sh. The walk passes through non-shell wrappers (git, sudo) and plain script interpreters (sh, dash); a known-but-unsupported interactive shell (tcsh, ksh) stops the walk so a shell further up can't claim the session. current_shell()/current_shell_name() consult the walk first and fall back to $SHELL / PSModulePath (Windows behavior unchanged; Git Bash sets $SHELL itself).

Hedged messaging. The reason string is now "shell integration installed but not active", and the hint presents restart as the likely fix with an escape hatch: "A shell restart usually activates shell integration; if it doesn't, ask an agent to debug with the docs @ https://worktrunk.dev/llms.txt" (the LLM docs index works for any agent; the /worktrunk skill exists only for plugin users). Post-install hints are unchanged (restart advice there is unconditional by construction). wt config show now prints Detected shell: <name> (process tree) next to $SHELL in the diagnostic gutter.

Also fixed along the way: shell_from_name requires a word boundary, so fishd/bashtop/numactl no longer classify as shells (numactl previously matched Nushell via the bare nu prefix); the post-install/uninstall restart hints compare Shell values instead of display strings, which never matched process-tree names like pwsh or zsh-5.9.

Testing. Integration tests spawn wt under a test harness whose real ancestry (nextest, cargo, the developer's shell) would leak in, so WORKTRUNK_TEST_PARENT_SHELL is pinned empty in STATIC_TEST_ENV_VARS (and in the two PTY helpers that mirror it by hand, which CI round 1 caught: locally the walk found the dev zsh, on CI the runner's bash) and set per-test to simulate ancestors. New integration tests cover the three detection outcomes (mismatch suppresses restart advice, configured ancestor shows the hedged hint, unsupported ancestor overrides $SHELL); unit tests cover the classifier, the walk itself (injectable process-table lookup: wrapper pass-through, transparency, cycle/depth/init terminations), and both OS probes against live processes; wt config show is snapshot-tested with an override ancestor and run once unpinned so the production walk executes end-to-end.

Known limits, accepted: the walk stops at the nearest supported shell even when it's a non-interactive script interpreter (bash -c wrapping wt reports bash), hence the hedged wording; the walk is bounded at 16 hops and any unreadable hop falls back to $SHELL.

This was written by Claude Code on behalf of max

🤖 Generated with Claude Code

max-sixty and others added 2 commits July 13, 2026 16:01
$SHELL names the login shell, so the 'shell requires restart' warning
fired for the wrong shell whenever the interactive shell differed (fish
user with zsh login shell). Walk the process tree for the nearest
enclosing shell instead, falling back to $SHELL, and hedge the warning:
'installed but not active' plus a restart-usually-fixes-it hint.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@max-sixty
max-sixty marked this pull request as ready for review July 13, 2026 23:24
max-sixty and others added 3 commits July 13, 2026 16:35
The install/uninstall PTY helpers mirror STATIC_TEST_ENV_VARS by hand, so
they missed the WORKTRUNK_TEST_PARENT_SHELL pin: locally the walk found
the dev zsh (snapshot matched), on CI it found the runner's bash and
suppressed the zsh compinit/restart output.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The detection logic reads carefully and the test matrix is thorough — mismatch, configured-ancestor, and unsupported-ancestor are all covered, and test (macos) passing confirms the ps-comm/argv[0] assumption the whole macOS branch rests on. The one thing holding this back is codecov/patch (red): two of the newly-added regions are never executed by the suite.

1. The real process-tree walk (detect_ancestor_shell, the #[cfg(unix)] loop). Every test sets WORKTRUNK_TEST_PARENT_SHELL, which returns before the walk, so parent_id()process_name_and_ppid → the bounded loop never runs end-to-end. process_name_and_ppid and ancestor_from_name are each unit-tested in isolation, but the driver that stitches them together isn't. This is somewhat inherent to the override design; the cleanest coverage is probably a serialized unit test that remove_vars WORKTRUNK_TEST_PARENT_SHELL and calls detect_ancestor_shell() directly (asserting it returns without panicking / hanging — the result is environment-dependent), or accepting the gap as a known infra path.

2. The wt config show "Detected shell: … (process tree)" line (render_shell_status). Config-show tests pin WORKTRUNK_TEST_PARENT_SHELL="", so ancestor_shell() is None and the branch never renders. This one is cleanly and deterministically coverable — set the env var to a shell name and the line fires. Sketch modeled on test_config_show_powershell_detected_via_psmodulepath (adjust helper names/ordering to match; the override must come after configure_wt_cmd, which sets it to ""):

/// `config show` surfaces the process-tree shell as "Detected shell: <name>
/// (process tree)" alongside $SHELL.
#[rstest]
fn test_config_show_detected_shell_via_process_tree(mut repo: TestRepo, temp_home: TempDir) {
    repo.setup_mock_ci_tools_unauthenticated();
    let settings = setup_snapshot_settings_with_home(&repo, &temp_home);
    settings.bind(|| {
        let mut cmd = wt_command();
        repo.configure_wt_cmd(&mut cmd);
        repo.configure_mock_commands(&mut cmd);
        cmd.arg("config").arg("show").current_dir(repo.root_path());
        set_temp_home_env(&mut cmd, temp_home.path());
        set_xdg_config_path(&mut cmd, temp_home.path());
        cmd.env("SHELL", "/bin/bash"); // login shell differs from the ancestor
        cmd.env("WORKTRUNK_TEST_PARENT_SHELL", "zsh");
        assert_cmd_snapshot!(cmd);
    });
}

Happy to push a commit with the show.rs test (and a driver test if you want one) — just say the word. Leaving this unapproved only because codecov/patch is a merge gate here.

One minor, non-blocking observation on the boundary check is inline.

Comment thread src/commands/config/show.rs
Comment thread src/shell/utils.rs Outdated
Comment thread src/shell/utils.rs
codecov/patch flagged the real walk loop and the 'Detected shell
(process tree)' branch: every test pins the walk off, so neither ran.
Extract the loop into walk_ancestors with an injectable lookup and
unit-test all terminations; snapshot config show with an override
ancestor; run one config show without the pin so the production walk
executes end-to-end.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@max-sixty
max-sixty merged commit a6972e4 into main Jul 14, 2026
39 checks passed
@max-sixty
max-sixty deleted the shell-restart-detection branch July 14, 2026 00:33
max-sixty added a commit that referenced this pull request Jul 15, 2026
…irrors (#3468)

The Documentation section of `CLAUDE.md` said "never hand-edit the
generated mirrors under `docs/content/`, `skills/worktrunk/reference/`,
or `plugins/worktrunk/skills/`", which reads as "everything under these
directories is generated." That's wrong for two of the three:

- `docs/content/` — non-command docs (faq, tips-patterns, worktrunk,
claude-code, llm-commits) are hand-edited primaries; only command pages
have generated regions.
- `skills/worktrunk/reference/` — `shell-integration.md`,
`troubleshooting.md`, and `README.md` are hand-maintained sources
(already carved out in `.gitattributes` with
`linguist-generated=false`); the rest are generated mirrors.
- `plugins/worktrunk/skills/` — wholly generated, as stated.

The over-broad wording nearly steered #3455 away from editing
`shell-integration.md`, which was the correct file to edit. The reworded
sentence names the split per directory and defers to `docs/CLAUDE.md`'s
sync taxonomy (already linked at the end of the paragraph) for the
per-file answer. No behavior or generated-file changes.

> _This was written by Claude Code on behalf of Maximilian Roos_

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@max-sixty max-sixty mentioned this pull request Jul 15, 2026
max-sixty added a commit that referenced this pull request Jul 15, 2026
Release v0.68.0. Highlights: on-demand picker preview tabs (#3439),
bare-repo project config read from the object store when the default
branch is checked out nowhere (#3462), shell detection via the process
tree (#3455), and `wt config state` flagging a stale default-branch
cache (#3478). Full details in CHANGELOG.md.

> _This was written by Claude Code on behalf of max_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants