fix(config): marker set/clear no-op outside a git repository

[mahirhir]

Problem

marker set/marker clear are called unconditionally by the Claude Code, Codex, and Gemini plugin hooks on nearly every turn (UserPromptSubmit, Notification, Stop, SessionEnd, ...), regardless of whether the session's directory is inside a git repository. Outside a repo, Repository::current() fails and handle_state_set/handle_state_clear print

✗ git rev-parse --git-common-dir failed (exit 128)
  fatal: not a git repository (or any of the parent directories): .git

to stderr and exit 1. The hooks' own || true already discards this, but it's still a spurious failure on every marker call in a session started outside a repo.

This is the third consequence @worktrunk-bot's triage called out on #3921, and flagged as correct "under every option" independent of how the other two consequences (a shell cd retargeting the marker mid-session) get resolved:

the non-repo exit 1 swallowed by || true is wrong under every option, since a hook that can't find a repo should be a no-op rather than a suppressed failure.

Fix

Scoped to the marker state key only: when Repository::current() fails specifically because the discovery path isn't inside a git repository (checked via CommandError's exit code + stderr, not any other resolution failure like a permission error or a corrupt repo), handle_state_set/handle_state_clear now return Ok(()) instead of propagating the error — matching the no-op the hooks already effectively get, minus the stderr noise and the exit 1.

Every other state key (default-branch, previous-branch, ci-status, logs) is untouched and keeps failing outside a repo the way every other wt command does — test_state_set_default_branch_outside_repo_still_fails pins that so the fallback doesn't silently widen later.

I did not touch hooks.json/.codex-plugin/plugin.json/the Gemini hooks — Claude Code's marker hooks already got -C "$CLAUDE_PROJECT_DIR" pinning in #3956 for the other two consequences (the cd-retargeting ones), and that commit's own message notes Codex and Gemini stay cwd-based "until their hook harnesses expose a stable session project directory." This PR is orthogonal to that: it fixes the swallowed-failure case regardless of whether a given plugin is pinned yet, so it helps Codex/Gemini today and keeps helping them once they get a pin.

Verification

• New regression tests: test_state_set_marker_outside_repo_is_noop, test_state_clear_marker_outside_repo_is_noop (exit 0, empty stdout/stderr, run from a tempdir with no -C), test_state_set_default_branch_outside_repo_still_fails (guards the scoping).
• cargo test --lib: 1517 passed, 0 failed.
• cargo test --test integration: 2034 passed, 0 failed, 1 ignored (pre-existing, unrelated nightly-only test).
• cargo clippy --lib --bins -- -D warnings and cargo clippy --test integration -- -D warnings: clean.
• cargo fmt --check: clean.
• Manually verified the pre-fix behavior first (git rev-parse --git-common-dir failed (exit 128), exit 1) against current main, then the post-fix behavior (exit 0, silent) with the built binary run from a fresh tempdir outside any repo.

[worktrunk-bot]

The scoping reads right — marker-only, with test_state_set_default_branch_outside_repo_still_fails pinning it so it can't widen quietly — and the description makes the case well. My pushback is on the classification mechanism rather than the goal.

The one that matters: matching git's stderr text means the fix never reaches a non-English user. Reproduced against this PR's own build, from a directory outside any repo:

$ LANG=en_US.UTF-8 LANGUAGE=de wt config state marker set TEST
✗ git rev-parse --git-common-dir failed (exit 128)
  Schwerwiegend: Kein Git-Repository (oder irgendeines der Elternverzeichnisse): .git
$ echo $?
1

They keep the exact per-turn exit 1 the PR removes, and LC_ALL=C in the test env means the suite can only ever exercise the English path. Details and two possible remedies inline, plus a second inline note that the is_missing_repository_error docstring claims to exclude permission errors and corrupt repos when in practice both match (also reproduced).

One doc line goes stale, outside the diff. docs/src/content/docs/claude-code.md — primary for that page per the sync taxonomy in docs/CLAUDE.md — says under Activity tracking is not plugin-specific:

• Don't let a failed marker call fail the session. Both set and clear exit non-zero outside a repository, and hosts differ on what a non-zero hook does. Append || true (or the host's equivalent) to every call unless you want that surfaced.

After this change they exit zero outside a repository, so the stated reason no longer holds. The || true advice itself still does — an invalid --branch, a failed git config write, and (per the inline comments) any non-English locale can still make these exit non-zero — so it's the first sentence that needs rewording rather than the bullet. cargo test --test integration test_docs_are_in_sync regenerates the two mirrors under skills/ and plugins/ from it. Happy to push that commit if you'd rather not chase it.

[mahirhir]

Pushed 525c908. Took the first of your two options on the stderr match.

Locale. is_missing_repository_error is now exit_code == Some(128) and nothing else. Keeping the substring plus a fragility note would have documented the German user's broken case rather than fixed it, and the suite pins LC_ALL=C so nothing would have gone red the day it drifted.

Docstring. Rewritten to say what the predicate actually does, including that an unreadable or corrupt .git matches too. That is also the honest cost of dropping the substring: any exit 128 out of git rev-parse --git-common-dir now takes the no-op arm for marker, not just the ones whose stderr says not a git repository. For a marker write that still looks like the right call to me, and the key scoping is what keeps it off everything else, but it is a real widening and worth you disagreeing with if you do.

Tests. The tempdir and the current_dir override are gone from all four new tests, per wt_command()'s docstring. cargo test --test integration -- outside_repo passes 6 of 6 here, the two pre-existing ones included.

Doc line. Reworded the first sentence of the Don't let a failed marker call fail the session bullet, and also the clause one bullet above it, which said a hook outside a repository fails. The || true advice stays for the reasons you listed minus the locale one, which this change removes. skills/ and plugins/ mirrors carry the same two sentences.

What I could not run: test_docs_are_in_sync is #![cfg(not(windows))] and this machine is Windows, so I edited the two mirrors by hand instead of letting the sync write them. They are byte-identical to each other and differ from the Starlight source only in the absolute link rewriting that was already there, and neither edited sentence contains a link. If CI disagrees, that is the file to look at.

Also ran: cargo fmt --check clean, cargo clippy --all-targets with no findings, cargo test --test integration config_state 142 passed 0 failed.

[worktrunk-bot]

All three earlier points are addressed, and I verified the fix end to end against this PR's build: outside a repository marker set and marker clear both exit 0 with nothing on stderr, while default-branch set still prints git rev-parse --git-common-dir failed (exit 128) and exits 1. Dropping the substring is the right call — Repository::at() runs exactly one subprocess (resolve_git_common_dir), and its only other failure mode, the canonicalize after it, is an io::Error that carries no CommandError, so the exit code is a clean discriminator here rather than a lossy proxy for one. test_state_clear_default_branch_outside_repo_still_fails is a good addition: it pins the second Repository::current() arm the first guard test never reached. check-docs is green, so the hand-edited mirrors are in sync — they differ from the primary only in the absolute-link rewriting that was already there.

One sibling case is still open: a detached worktree. It produces the same per-turn exit 1 the PR removes for the no-repo case, from the same unconditional hook calls. Reproduced against this PR's build:

$ git init -q r && cd r && git commit -q --allow-empty -m init && git checkout -q --detach HEAD
$ wt config state marker set "🤖"
✗ Cannot set marker for current branch: not on a branch (detached HEAD)
↳ To switch to a branch, run git switch <branch>
$ echo $?
1

handle_state_set's marker arm reaches repo.require_current_branch("set marker for current branch"), which returns GitError::DetachedHead when current_worktree().branch() is None; handle_state_clear does the same. Every UserPromptSubmit/Stop/SessionEnd in an agent session running in a detached worktree hits it, so it's the same swallowed-by-|| true failure on every turn that #3921 describes — just reached through a different resolution step.

Whether to extend the no-op there is your call, not mine: skills/worktrunk/SKILL.md documents the rejection as deliberate ("a detached worktree (which marker still rejects, since it keys state by branch name)"), and a marker genuinely has nowhere to go without a branch, so silence may be worse than the error for a human typing the command. I'm not asking for it in this PR. But the bullet this PR rewrites is advice for hook authors about exactly which calls still exit non-zero, and it's the one case in that list an agent hook actually hits repeatedly — inline suggestion for the minimum version.

Holding the merge call for a human. src/commands/config/state.rs sits on worktrunk's deletion surface: handle_state_clear's logs arm calls clear_logs, which removes per-branch log trees with std::fs::remove_dir_all and shared log files with std::fs::remove_file, and the same file's clear_trash sweeps .git/wt/trash/. This diff can't reach either — the new arm is guarded on key == "marker", and the non-marker path is behaviourally identical to the ? it replaced — but this repo's review guidance holds a source change near that path for a human even when it reads acceptable (.claude/skills/running-tend/references/review-pr.md, Data-Loss Surface: Hold for Human Review). So no approval from me; requesting @max-sixty.

[worktrunk-bot]

A detached worktree is the third case, and the one a hook is most likely to hit repeatedly: marker set/clear resolve the branch through require_current_branch, which returns GitError::DetachedHead, so every turn of an agent session in a detached worktree exits 1 with Cannot set marker for current branch: not on a branch (detached HEAD). Worth naming here since the bullet exists to tell hook authors which calls still need the || true.

Suggested change

	- **Don't let a failed marker call fail the session.** Outside a repository both `set` and `clear` do nothing and exit 0, but an invalid `--branch` or a failed git config write still exits non-zero, and hosts differ on what a non-zero hook does. Append `|| true` (or the host's equivalent) to every call unless you want that surfaced.
	- **Don't let a failed marker call fail the session.** Outside a repository both `set` and `clear` do nothing and exit 0, but a detached worktree, an invalid `--branch`, or a failed git config write still exits non-zero, and hosts differ on what a non-zero hook does. Append `|| true` (or the host's equivalent) to every call unless you want that surfaced.

The two mirrors under skills/ and plugins/ carry the same sentence — cargo test --test integration test_docs_are_in_sync regenerates them from this file, so applying the suggestion alone will leave check-docs red until that runs.

[worktrunk-bot]

Two things from your comment that my review just now didn't cover.

The widening. No disagreement — and I'd put it slightly more strongly than you did. Branching on an exit code rather than a message is what CLAUDE.md already asks for under Structured Output Over Error-Message Parsing ("git merge-base exit codes encode meaning … branch on status.code(), not message text"), so 525c908 moves this to the side of the convention rather than away from it.

I did reproduce the widening you flagged, since it's the part you asked to be argued with. Against this PR's build, a directory whose .git is a gitdir file pointing nowhere:

$ echo "gitdir: /nope" > .git
$ wt config state marker set "🤖"; echo $?
0
$ wt list
✗ git rev-parse --git-common-dir failed (exit 128)
  fatal: not a git repository: (null)
$ wt config state default-branch set main
✗ git rev-parse --git-common-dir failed (exit 128)
  fatal: not a git repository: (null)

So it's real: a broken-but-present repository now takes the no-op arm too. What bounds it is that marker is the only key that goes quiet — every other wt command in that same directory still reports the breakage, and the hook path was discarding this error through || true before the change anyway. The user who needs to know their repository is broken still finds out from the next command they run.

The mirrors. test_docs_are_in_sync isn't in check-docs — that job builds the Starlight site. The test runs in the integration suite, so test (linux), test (macos) and test (windows) are the gate, and all three are green on 525c908. Your hand edits match what the sync would have written.

[mahirhir]

Both of your bounding claims check out independently, so I'll put the evidence here rather than just agree.

"marker is the only key that goes quiet." Holds on the diff as it stands. The early return is guarded by key == "marker" in handle_state_set and again in handle_state_clear, and handle_state_get is untouched, so default-branch and every other key still propagate the exit 128 from Repository::current(). The guard is on the key rather than on the call site, which is what makes the bound hold for a user typing the command directly and not only for the hooks.

"The hook path was discarding this through || true anyway." I counted rather than spot-checked, against plugins/worktrunk/hooks/hooks.json on main:

$ node count.mjs
hook entries: 8, marker calls: 6, ending in || true: 6
  UserPromptSubmit   guarded
  Notification       guarded
  PreToolUse         guarded
  PermissionRequest  guarded
  Stop               guarded
  SessionEnd         guarded
control (a bare command must read BARE): ok

Six of six, no exceptions — so nobody on the shipped plugin was seeing that stderr before this change, and the behaviour they lose is a message they never got. The control line is there because "all six guarded" is the kind of result a broken matcher also returns; a bare command still reads as unguarded.

That does leave the docs bullet doing real work, since || true is a recommendation there and not something the CLI enforces. A user who wrote their own hook without it, outside a repository, went from exit 1 to exit 0 — which is the fix — and a user who wrote their own hook without it in a corrupt repository went from a message to silence, which is the widening. Same bound you drew; I just wanted the second half of it written down next to the first.

On the mirrors: agreed, and thanks for tracing which job actually gates it. I had assumed check-docs from the name.