Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/src/content/docs/claude-code.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ Activity tracking is not plugin-specific. The plugins above only call `wt` on th

Three things to get right:

- **Run the command inside the worktree.** Each one resolves the branch from its working directory, so a hook that runs elsewhere marks the wrong branch, and one that runs outside a repository fails. Where the host pins the working directory elsewhere, pass the global `-C <worktree>`, which moves both the repository lookup and the branch resolution. `--branch <branch>` names the branch on its own, but the repository lookup still comes from the working directory — that, not a missing worktree argument, is why a caller pinned outside the repository needs `-C`. Elsewhere, a command that names a branch ([`wt switch`](/switch/), [`wt remove`](/remove/), `wt step diff --branch`) already names the worktree it acts on, and `-C` is for reaching a different repository rather than a different worktree.
- **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.
- **Run the command inside the worktree.** Each one resolves the branch from its working directory, so a hook that runs elsewhere marks the wrong branch, and one that runs outside a repository silently does nothing. Where the host pins the working directory elsewhere, pass the global `-C <worktree>`, which moves both the repository lookup and the branch resolution. `--branch <branch>` names the branch on its own, but the repository lookup still comes from the working directory — that, not a missing worktree argument, is why a caller pinned outside the repository needs `-C`. Elsewhere, a command that names a branch ([`wt switch`](/switch/), [`wt remove`](/remove/), `wt step diff --branch`) already names the worktree it acts on, and `-C` is for reaching a different repository rather than a different worktree.
- **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.

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.

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.

- **Clear on exit.** A marker set on session start persists until something clears it, so pair every set with a clear on the host's session-end event — and expect the same stale marker as above if the process is killed first.

## Worktree isolation (Claude Code only)
Expand Down
4 changes: 2 additions & 2 deletions plugins/worktrunk/skills/worktrunk/reference/claude-code.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ Activity tracking is not plugin-specific. The plugins above only call `wt` on th

Three things to get right:

- **Run the command inside the worktree.** Each one resolves the branch from its working directory, so a hook that runs elsewhere marks the wrong branch, and one that runs outside a repository fails. Where the host pins the working directory elsewhere, pass the global `-C <worktree>`, which moves both the repository lookup and the branch resolution. `--branch <branch>` names the branch on its own, but the repository lookup still comes from the working directory — that, not a missing worktree argument, is why a caller pinned outside the repository needs `-C`. Elsewhere, a command that names a branch ([`wt switch`](https://worktrunk.dev/switch/), [`wt remove`](https://worktrunk.dev/remove/), `wt step diff --branch`) already names the worktree it acts on, and `-C` is for reaching a different repository rather than a different worktree.
- **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.
- **Run the command inside the worktree.** Each one resolves the branch from its working directory, so a hook that runs elsewhere marks the wrong branch, and one that runs outside a repository silently does nothing. Where the host pins the working directory elsewhere, pass the global `-C <worktree>`, which moves both the repository lookup and the branch resolution. `--branch <branch>` names the branch on its own, but the repository lookup still comes from the working directory — that, not a missing worktree argument, is why a caller pinned outside the repository needs `-C`. Elsewhere, a command that names a branch ([`wt switch`](https://worktrunk.dev/switch/), [`wt remove`](https://worktrunk.dev/remove/), `wt step diff --branch`) already names the worktree it acts on, and `-C` is for reaching a different repository rather than a different worktree.
- **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.
- **Clear on exit.** A marker set on session start persists until something clears it, so pair every set with a clear on the host's session-end event — and expect the same stale marker as above if the process is killed first.

## Worktree isolation (Claude Code only)
Expand Down
4 changes: 2 additions & 2 deletions skills/worktrunk/reference/claude-code.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 41 additions & 3 deletions src/commands/config/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ use crate::commands::picker::preview_cache;
use anyhow::Context;
use color_print::cformat;
use path_slash::PathExt as _;
use worktrunk::git::{BranchRef, Repository, resolve_input_path, sha_cache};
use worktrunk::git::{BranchRef, CommandError, Repository, resolve_input_path, sha_cache};
use worktrunk::path::format_path_for_display;
use worktrunk::styling::{
eprintln, format_heading, format_with_gutter, hint_message, info_message, println,
Expand Down Expand Up @@ -834,9 +834,39 @@ pub fn handle_state_get(
Ok(())
}

/// True when `err` is `Repository::current()` failing with the exit 128 that
/// `git rev-parse --git-common-dir` returns when the discovery path has no
/// usable git repository. That covers the missing-repo case the marker hooks
/// hit, and equally an unreadable or corrupt `.git`, which git reports the
/// same way and which no-ops just as harmlessly for a marker. Any other exit
/// code still propagates, so the #3921 fallback stays off unrelated failures.
///
/// The exit code is the whole check on purpose. git translates
/// `not a git repository`, and the translation ships in the distro package,
/// so matching that text would leave every non-English user on the old
/// behavior with nothing in the suite able to catch it.
fn is_missing_repository_error(err: &anyhow::Error) -> bool {
CommandError::find_in(err).is_some_and(|cmd_err| cmd_err.exit_code == Some(128))
}

/// Handle the state set command
pub fn handle_state_set(key: &str, value: String, branch: Option<String>) -> anyhow::Result<()> {
let repo = Repository::current()?;
let repo = match Repository::current() {
Ok(repo) => repo,
Err(err) if key == "marker" && is_missing_repository_error(&err) => {
// The Claude Code / Codex / Gemini marker hooks call `marker set`
// unconditionally on every turn (`UserPromptSubmit`, `Stop`, …),
// regardless of whether the session's directory is inside a git
// repository. Before this, a session started outside a repo made
// every one of those calls print `git rev-parse --git-common-dir
// failed (exit 128)` to stderr and exit 1 — silently discarded by
// the hook's own `|| true`, but still a spurious failure on every
// turn. Treat "no repository here" as the no-op it already
// behaves like from the hook's perspective (#3921).
return Ok(());
}
Err(err) => return Err(err),
};

match key {
"default-branch" => {
Expand Down Expand Up @@ -893,7 +923,15 @@ pub fn handle_state_set(key: &str, value: String, branch: Option<String>) -> any

/// Handle the state clear command
pub fn handle_state_clear(key: &str, branch: Option<String>, all: bool) -> anyhow::Result<()> {
let repo = Repository::current()?;
let repo = match Repository::current() {
Ok(repo) => repo,
Err(err) if key == "marker" && is_missing_repository_error(&err) => {
// `SessionEnd` fires the same unconditional `marker clear` call —
// see the matching comment in `handle_state_set`.
return Ok(());
}
Err(err) => return Err(err),
};

match key {
"default-branch" => {
Expand Down
72 changes: 72 additions & 0 deletions tests/integration_tests/config_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,78 @@ fn test_state_clear_marker_all_empty(repo: TestRepo) {
assert_snapshot!(String::from_utf8_lossy(&output.stderr), @"○ No markers to clear");
}

/// `marker set`/`marker clear` run unconditionally from the Claude Code,
/// Codex, and Gemini plugin hooks (`UserPromptSubmit`, `Stop`, `SessionEnd`,
/// …), whether or not the session's directory is inside a git repository.
/// Outside a repository, both must no-op silently — exit 0, nothing on
/// stdout or stderr — rather than the pre-fix behavior of printing
/// `git rev-parse --git-common-dir failed (exit 128)` and exiting 1, a
/// failure the hooks' own `|| true` already discarded (#3921).
#[rstest]
fn test_state_set_marker_outside_repo_is_noop() {
let mut cmd = wt_command();
cmd.args(["config", "state", "marker", "set", "🤖"]);
let output = cmd.output().unwrap();

assert!(
output.status.success(),
"stderr: {}",
String::from_utf8_lossy(&output.stderr)
);
assert_eq!(String::from_utf8_lossy(&output.stdout), "");
assert_eq!(String::from_utf8_lossy(&output.stderr), "");
}

#[rstest]
fn test_state_clear_marker_outside_repo_is_noop() {
let mut cmd = wt_command();
cmd.args(["config", "state", "marker", "clear"]);
let output = cmd.output().unwrap();

assert!(
output.status.success(),
"stderr: {}",
String::from_utf8_lossy(&output.stderr)
);
assert_eq!(String::from_utf8_lossy(&output.stdout), "");
assert_eq!(String::from_utf8_lossy(&output.stderr), "");
}

/// The no-op fallback is scoped to `marker` — a state key without a git
/// hook depending on it, run outside a repository, must keep failing the
/// way every `wt` command does. Guards against widening the `#3921` fix
/// beyond the one command the hooks actually call unconditionally.
#[rstest]
fn test_state_set_default_branch_outside_repo_still_fails() {
let mut cmd = wt_command();
cmd.args(["config", "state", "default-branch", "set", "main"]);
let output = cmd.output().unwrap();

assert!(!output.status.success());
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("git rev-parse --git-common-dir failed"),
"stderr: {stderr}"
);
}

/// Same guard as `test_state_set_default_branch_outside_repo_still_fails`,
/// for the `clear` side — `handle_state_clear` has its own `Repository::current()`
/// match with its own fallback arm.
#[rstest]
fn test_state_clear_default_branch_outside_repo_still_fails() {
let mut cmd = wt_command();
cmd.args(["config", "state", "default-branch", "clear"]);
let output = cmd.output().unwrap();

assert!(!output.status.success());
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(
stderr.contains("git rev-parse --git-common-dir failed"),
"stderr: {stderr}"
);
}

// ============================================================================
// logs
// ============================================================================
Expand Down
Loading