fix(commands): name the configured alias instead of "unrecognized subcommand" outside a repository - #3982
fix(commands): name the configured alias instead of "unrecognized subcommand" outside a repository#3982yzx9 wants to merge 1 commit into
Conversation
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The diagnosis is right and the placement is good: putting the check after the PATH lookup keeps it off the wt-<name> dispatch path, and scoping it to UserConfig is correct since project config can't exist without a repo. Three things.
The hint doesn't style its commands (inline suggestion). .claude/skills/writing-user-outputs/SKILL.md — "In hints: Use <underline> for commands and data values (paths, branches)" and "Command suggestions in hints: When a hint includes a runnable command, use 'To X, run Y' pattern. End with the command for easy copying." The current hint ends with "to target one" and leaves wt and -C <path> unstyled. The suggestion routes the command through a runtime variable rather than putting -C <path> inside the cformat! literal — nowhere in src/ does an angle-bracket placeholder sit inside a cformat! format string, and the literal is what color-print's tag parser reads.
Nothing pins the rendered message. tests/integration_tests/custom.rs has no assert_cmd_snapshot! at all, while the adjacent step_alias.rs uses it 56 times, and the output skill says every command output must have snapshot tests. The contains() assertions here confirm the words but not the symbols, colors, or hint attachment — a snapshot is exactly what would have caught the styling gap above.
One path still reaches the old output. try_alias is skipped entirely when any argument isn't UTF-8 (the alias_args collect returns None), so it establishes nothing about whether the name is configured. Inside a repo, wt co $'\xff' with co configured falls through to the clap error and prints the original shape — unrecognized subcommand 'co' with tip: some similar subcommands exist: 'co', 'config'. That's pre-existing, not a regression, but it's the same bug on a sibling path, and it points at a complementary one-line fix: similar_subcommands in src/commands/mod.rs filters out help but never the input itself, so an exact match is always eligible as its own "did you mean". Filtering candidate != name there would kill the self-suggestion everywhere it can occur, independent of the repo check.
Minor, take or leave: the error's second clause states a general rule rather than the current state. "co is an alias, but there's no git repository here" reads closer to the actual/expected shape the output skill asks for.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
Everything from the last pass is addressed: the hint styles its command, the rendered output is pinned by a snapshot, and the self-suggestion filter closes the non-UTF-8 sibling path. Moving the gate into try_alias reads well and the precedence argument for it holds. Three things on the new commit.
test (windows) is red on this commit, and it's from the diff. TestRepo is imported at the top of tests/integration_tests/custom.rs, but its only use is inside custom_subcommand_alias_with_non_utf8_arg_never_self_suggests, which is #[cfg(unix)]. On Windows that leaves the import unused, and the pre-merge hook runs with RUSTFLAGS='-D warnings', so it's a build failure rather than a warning:
error: unused import: `TestRepo`
--> tests\integration_tests\custom.rs:4:5
|
4 | TestRepo,
| ^^^^^^^^
= note: `-D unused-imports` implied by `-D warnings`
error: could not compile `worktrunk` (test "integration") due to 1 previous error
Two inline suggestions move it into the #[cfg(unix)] body, beside the OsString imports that test already scopes locally.
The new message asserts a cause the code didn't establish. try_alias discards the error from Repository::current() and the message names one specific reason, but Repository::at also fails when git rev-parse --git-common-dir can't be spawned or its output can't be canonicalized. The sharpest case is a typo'd -C, where the hint then recommends exactly what the user just ran:
$ wt -C /tmp/does-not-exist-xyz co
✗ co is an alias, but there's no git repository here
↳ Run wt inside a repository, or to target one, run wt -C <path> co
Every other command surfaces the real failure for that same input — wt -C /tmp/does-not-exist-xyz list gives ✗ Failed to execute: git rev-parse --git-common-dir / No such file or directory (os error 2), and with no repo at all it gives fatal: not a git repository …. So this path is now the one place that swallows the distinction. Not a regression (the else arm already dropped the error), but the PR is specifically about making this message accurate, and it's cheap to keep it honest: capture the error and only substitute the alias message when the failure really is "not a repository" — git rev-parse exits 128 there, which is the structured signal CLAUDE.md's "Structured Output Over Error-Message Parsing" points at — otherwise propagate what git said. Your call whether that's in scope for this fix; if it isn't, it's worth a line in the docstring saying the message is a best guess at the cause.
set_git_only_path is inert in the new PATH-collision test. prepend_path rebuilds PATH from the test process's environment (std::env::vars_os()), not from what's already on cmd, so the git-only value set one line earlier is discarded; the following cmd.env("WORKTRUNK_TEST_MOCK_CONFIG_DIR", …) overwrites the other half. The test runs against the host PATH and host git, which is fine here — the alias name is unique enough that nothing collides — but the _git_only_path binding reads as isolation that's in effect, and the next person to tighten the mock-git setup will trust it. Inline suggestion drops the line.
Minor, take or leave: alias_needs_repo_error's lookup is the same three lines as the no-repo branch of alias_names_for_suggestions (suppress_warnings(), UserConfig::load(), aliases(None)), and on the miss path both run — wt <not-an-alias> outside a repo now parses the system + user config twice for one invocation. A shared user_config_alias_names() would cover both.
7faf08a to
53065a1
Compare
worktrunk-bot
left a comment
There was a problem hiding this comment.
The dispatch change reads well and the precedence argument for moving the gate into try_alias holds. I built the head and walked the paths: an alias outside a repository errors instead of falling through, a colliding wt-<name> no longer shadows it, a non-alias name still reaches its PATH binary, and the self-suggestion filter does kill the 'co' tip. One thing holds the approval.
The message asserts a cause the code never established, and the new snapshot pins that. try_alias discards the error from Repository::current(), but Repository::at fails three ways: git rev-parse --git-common-dir exiting 128 (genuinely not a repository), the spawn itself failing (a -C path that doesn't exist), and the canonicalize failing. All three now print "there's no git repository here". Built at 53065a1:
$ wt -C /tmp/does-not-exist-xyz co
✗ co is an alias, but there's no git repository here
↳ Run wt inside a repository, or to target one, run wt -C <path> co
$ wt -C /tmp/does-not-exist-xyz list # every other command
✗ Failed to execute: git rev-parse --git-common-dir
No such file or directory (os error 2)The hint there recommends exactly what the user just ran. wt step co on the same input reports the true cause as well, so top-level alias dispatch is now the one place in wt that swallows the distinction — in the PR that exists to make this message accurate.
The snapshot doesn't catch it because it isn't testing "outside a repository". set_git_only_path installs a mock git with only version configured, and mock_stub's fallback for an unmatched command is exit_code: 1 with no output — so custom_subcommand_alias_outside_repo_names_the_alias exercises "git failed", not "no repository", and would pass identically with a repository present. Same shape, run from inside a real checkout with a git that exits 1 for everything but --version:
$ git rev-parse --is-inside-work-tree # real git, real repo
true
$ PATH=/tmp/fakegit wt co
✗ co is an alias, but there's no git repository hereThe two inline suggestions gate the message on the structured signal — CLAUDE.md → Structured Output Over Error-Message Parsing, where git rev-parse exiting 128 is the "not a repository" channel — and keep the is_alias check first, so a non-alias name still falls through to its wt-<name> binary whatever git did. I compiled that patch and ran the six cases:
| invocation | before | after |
|---|---|---|
| alias, no repo | alias message | alias message |
alias, -C bad path |
alias message | Failed to execute: git rev-parse … |
| alias, broken git inside a repo | alias message | git rev-parse … failed (exit 1) |
alias, colliding wt-<name>, no repo |
alias message | alias message |
non-alias + wt-<name>, no repo |
binary runs | binary runs |
non-alias + wt-<name>, broken git |
binary runs | binary runs |
That patch turns the snapshot test red, which is the point: to keep asserting what its name says, the mock git needs to fail the way git actually fails — .command("rev-parse", MockResponse::exit(128).with_stderr("fatal: not a git repository (or any of the parent directories): .git\n")) on the MockConfig in set_git_only_path. Worth doing either way; as it stands the test passes for the wrong reason.
Happy to push this as a commit if you'd prefer that to applying the suggestions.
Verification notes
Failure modes of Repository::at, from resolve_git_common_dir in src/git/repository/mod.rs: Cmd::run().context("Failed to execute: git rev-parse --git-common-dir") (spawn), CommandError::from_failed_output (non-zero exit, carries exit_code), canonicalize(...).context("Failed to resolve git common directory"). Only the middle one with code 128 means "not a repository"; CommandError::exit_code is the field the suggestion reads.
mock_stub's default_response is CommandResponse { file: None, output: None, stderr: None, exit_code: 1, wait_for_file: None }, reached when no _default and no triple/compound/single key matches — which is the case for rev-parse --git-common-dir against MockConfig::new("git").version("git version 2.43.0").
Everything above was run against cargo build --bin wt at the PR head, and the patched variant compiled clean before the six cases were re-run.
…command" outside a repository
`wt co` outside a git repository, with `co` a user-config alias, fell
through alias dispatch and PATH lookup to the synthesized
InvalidSubcommand error — which mixes user-config alias names into its
did-you-mean candidates, so the tip suggested the very name the user
typed:
error: unrecognized subcommand 'co'
tip: some similar subcommands exist: 'co', 'config'
The alias also appears in `wt --help`'s Aliases block, so "unrecognized"
contradicted wt's own help.
try_alias now ends dispatch itself when no repository is present and the
name is a user-config alias:
✗ co is an alias, but there's no git repository here
↳ Run wt inside a repository, or to target one, run wt -C <path> co
The error states the situation; the hint ends with a styled, copyable
command (built with format! so the <path> placeholder stays out of
cformat!'s tag parser). Because the check lives in alias dispatch, the
alias owns its name outside repositories too — a colliding wt-<name>
PATH binary no longer shadows it there, matching the in-repo precedence
("user config wins over wt-<name> PATH binaries"). Inside a repository,
dispatch is unchanged.
Only a genuine "not a repository" earns the alias message: the message
gates on the structured signal — `git rev-parse` exiting 128 — while any
other discovery failure (a spawn error from a bad `-C` path, a git that
errors for its own reasons) propagates as itself, the way it does for
every other command. The `is_alias` check stays first, so a non-alias
name still falls through to its `wt-<name>` binary whatever git did.
similar_subcommands no longer suggests the input itself. Some dispatch
paths reach the error with the input in the candidate pool — non-UTF-8
args skip alias dispatch wholesale, so `wt co $'\xff'` inside a repo
still produced a tip suggesting 'co'. An exact match is filtered before
did_you_mean, killing the self-suggestion everywhere it can occur;
near-match dedupe is unaffected.
The outside-repo output is pinned by a snapshot (exit code, symbols,
styling, hint attachment); the mock git in `set_git_only_path` answers
`rev-parse` the way real git does outside a repository (exit 128 with
the fatal message), so the snapshot exercises "no repository" rather
than "git failed"; a test proves the alias wins over a colliding PATH
binary outside repos; the non-UTF-8 sibling path asserts a tip without
'co'; a unit test asserts exact matches are never suggested.
Assisted-by: Claude-Code:GLM-5.3
53065a1 to
bcf4264
Compare
worktrunk-bot
left a comment
There was a problem hiding this comment.
The dispatch change holds up. I built the head and walked it: an alias outside a repository names itself instead of falling through, a colliding wt-<name> no longer shadows it, a non-alias name still reaches its PATH binary, the self-suggestion filter does kill the 'co' tip, and set_git_only_path's mock now answers rev-parse the way real git does — so the snapshot exercises "no repository" rather than "git failed". One thing left, and it's the same accuracy question one level down.
Exit 128 isn't git's "not a repository" channel — it's its generic die(). git rev-parse exits 128 for every fatal error, so the gate admits a whole class of failures the docstring and the message say it excludes. Built at bcf4264, inside a real repository whose only problem is an unsupported repo-format version:
$ git config core.repositoryformatversion 999 # still a repository, just unreadable
$ wt co
✗ co is an alias, but there's no git repository here
↳ Run wt inside a repository, or to target one, run wt -C <path> co
$ wt list # every other command
✗ git rev-parse --git-common-dir failed (exit 128)
fatal: Expected git repo version <= 1, found 999There is a repository here, the hint's remedy won't help, and git's own actionable line is exactly what's being swallowed. The everyday instance is safe.directory: a checkout owned by another user — Docker bind mount, a sudo clone, a shared drive — dies with fatal: detected dubious ownership in repository at '…' and exit 128, which makes wt <alias> the one place in wt that doesn't tell the user to add safe.directory.
Worktrunk already writes this down about 128 elsewhere — RemoteDetection::Unavailable in src/git/repository/config.rs: "ls-remote exits 128 for all of those alike, so telling a down network from a remote that simply has no HEAD would mean reading git's error text."
Not a regression — on main the same input gives unrecognized subcommand 'co', which is worse — so this needn't hold the fix. Two ways to close it:
- Scope the claim to what the check establishes. The two inline suggestions do that and change no behavior.
- Or stop dropping git's words.
CommandError::stderris already in hand; rendering it under the error withformat_with_gutterleaves the alias line naming the alias and git's line explaining why, in every 128 case. It would change the snapshot — which reads like a gain, since the addedfatal: not a git repository …is the real git text the mock now emits.
Happy to push either as a commit if that's easier than applying the suggestions.
| /// very name the user typed as its own "did you mean". Otherwise `Ok(None)`: | ||
| /// not an alias here, the caller falls through to the `wt-<name>` PATH | ||
| /// lookup — whatever git did. Best-effort — a config that fails to load | ||
| /// reads as no aliases. | ||
| /// |
There was a problem hiding this comment.
The check establishes "git died", not "not a repository" — git rev-parse exits 128 for every fatal error, so a safe.directory violation or a bad core.repositoryformatversion reaches this message too, inside a real repository. Narrowing the claim to what the exit code actually carries:
| /// very name the user typed as its own "did you mean". Otherwise `Ok(None)`: | |
| /// not an alias here, the caller falls through to the `wt-<name>` PATH | |
| /// lookup — whatever git did. Best-effort — a config that fails to load | |
| /// reads as no aliases. | |
| /// | |
| /// A 128 exit is the closest structured signal git offers, not a dedicated | |
| /// "not a repository" code: `git rev-parse` dies with 128 for any fatal | |
| /// error, so a `safe.directory` violation or an unsupported | |
| /// `core.repositoryformatversion` reaches this message too. Failures that | |
| /// exit some other way — a spawn error from a bad `-C` path — propagate as | |
| /// themselves, the way they do for every other command. |
| .map(|uc| uc.aliases(None).contains_key(name)) | ||
| .unwrap_or(false); | ||
| if !is_alias { |
There was a problem hiding this comment.
Same claim in the inline comment. CLAUDE.md's "Structured Output Over Error-Message Parsing" is the right thing to be following here — 128 is the best structured signal git gives — but it isn't exclusive to "not a repository", and RemoteDetection::Unavailable in src/git/repository/config.rs already says so about the same exit code.
| .map(|uc| uc.aliases(None).contains_key(name)) | |
| .unwrap_or(false); | |
| if !is_alias { | |
| // `git rev-parse` exits 128 outside a repository. It is the closest | |
| // structured signal git offers, per "Structured Output Over | |
| // Error-Message Parsing" — but 128 is git's fatal-exit code generally, | |
| // not a "not a repository" code; see the docstring. |
wt co baroutside a git repository, withcoa user-config alias, fell through alias dispatch (aliases resolve the current worktree) and PATH lookup to the synthesizedInvalidSubcommanderror — which mixes user-config alias names into its did-you-mean candidates, so the tip suggested the very name the user typed:Now
try_aliasitself ends dispatch when no repository is present and the name is a user-config alias:Only a genuine "not a repository" earns that message: it gates on the structured signal (
git rev-parseexiting 128), while any other discovery failure — a spawn error from a bad-Cpath, a git that errors for its own reasons — propagates as itself, the way it does for every other command:> wt -C /tmp/does-not-exist-xyz co ✗ Failed to execute: git rev-parse --git-common-dir No such file or directory (os error 2)The
is_aliascheck stays first, so a non-alias name still falls through to itswt-<name>PATH binary whatever git did. Because the check lives in alias dispatch, the alias owns its name outside repositories too — a collidingwt-<name>PATH binary no longer shadows it there, matching the in-repo precedence ("user config wins overwt-<name>PATH binaries"). Inside a repository, dispatch is unchanged.Also fixed on a sibling path:
similar_subcommandsnever suggests the input itself anymore. Non-UTF-8 args skip alias dispatch wholesale, sowt co $'\xff'inside a repo still producedtip: … 'co'— suggesting the typed name. An exact match is now filtered beforedid_you_mean, which kills the self-suggestion everywhere it can occur; near-match dedupe is unaffected.Tests: the outside-repo output is pinned by a snapshot (exit code, symbols, styling, hint attachment), with the mock git in
set_git_only_pathansweringrev-parsethe way real git does outside a repository (exit 128 with the fatal message) so the snapshot exercises "no repository" rather than "git failed"; a test proves the alias wins over a colliding PATH binary outside repos; the non-UTF-8 sibling path asserts a tip without'co'.Assisted-by: Claude-Code:GLM-5.3