From 2a3279a12acfddcb60c3936610d551ee190b9b9d Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Sun, 19 Jul 2026 22:34:49 +0200 Subject: [PATCH 1/3] Surface herdr's real error on herdr-launch.sh failure paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit herdr-launch.sh discarded herdr's stderr (2>/dev/null) on every failable call, so a launch/resume failure only ever showed the generic "did not return a pane id" guard message — as hit in a real /kickoff incident where herdr's stderr named the exact cause (agent_placement_not_found: workspace w9 not found) but nobody could see it. - Capture stderr for `agent start`, `pane move --new-tab`, resume's `tab create`, and resume's `pane run "claude -c"`. - Add a shared `herdr_diag` helper: parse herdr's {"error":{"code","message"}} JSON defensively (raw text fallback, never a traceback), print it before the existing generic/contextual message. - Append a one-line actionable hint when the error names an invalid workspace placement, pointing at a stale HERDR_WORKSPACE_ID. - Diagnostics only: stdout key=value lines, exit codes, and the moved/reused/resumed/focused semantics are unchanged. Bump work-system 1.9.0 -> 1.9.1 (patch), update CHANGELOG and the herdr-kickoff-automation knowledge entry. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017D6Ji3oU3md1ENxY2WfbcT --- .claude-plugin/marketplace.json | 2 +- .../features/herdr-kickoff-automation.md | 22 ++++- CHANGELOG.md | 3 + .../work-system/.claude-plugin/plugin.json | 2 +- plugins/work-system/scripts/herdr-launch.sh | 88 +++++++++++++++++-- 5 files changed, 104 insertions(+), 13 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index c7c33cd..9c9d125 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -18,7 +18,7 @@ "name": "work-system", "source": "./plugins/work-system", "description": "Generic task and worktree workflow system for Claude Code. Manage tasks as markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, or grok), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.9.0" + "version": "1.9.1" }, { "name": "pr-flow", diff --git a/.claude/knowledge/features/herdr-kickoff-automation.md b/.claude/knowledge/features/herdr-kickoff-automation.md index 88aec61..30dd7d4 100644 --- a/.claude/knowledge/features/herdr-kickoff-automation.md +++ b/.claude/knowledge/features/herdr-kickoff-automation.md @@ -1,10 +1,10 @@ --- title: "herdr /kickoff + /continue-reopen Automation" createdAt: 2026-06-24 -updatedAt: 2026-07-18 +updatedAt: 2026-07-19 createdFrom: "PR #17" -updatedFrom: "session: 2026-07-18 (kickoff agent selection)" -pluginVersion: 1.9.0 +updatedFrom: "session: 2026-07-19 (surface herdr launch errors)" +pluginVersion: 1.9.1 prime: false reindexedAt: 2026-07-12 --- @@ -53,6 +53,22 @@ truth; this entry captures the durable design and one non-obvious gotcha. unrelated, workspace), and both `herdr` and `python3` are on `PATH`. `--no-focus` keeps the kickoff session in front; any failure (empty `$pane`) degrades to the unchanged manual block — never block kickoff on herdr. +- **Failure diagnostics surface herdr's own error (1.9.1).** Every herdr call on a + failure path (`agent start`, `pane move --new-tab`, resume's `tab create`, resume's + `pane run "claude -c"`) now captures stderr instead of `2>/dev/null` and runs it + through a shared `herdr_diag` helper: parse herdr's `{"error":{"code","message"}}` + JSON defensively (any exception falls back to the raw stderr text, never a + traceback), print it, then the existing generic message stays as the last-resort + line. This was diagnosability-only (no fallback/self-heal logic — actively rejected, + see below) after a real incident where the launch failed with only "herdr agent + start did not return a pane id" while herdr's stderr — discarded by the old + `2>/dev/null` — had named the exact cause. When the parsed `code` is + `agent_placement_not_found` (or the message otherwise names the workspace target), + `herdr_diag` appends one hint line pointing at a stale `$HERDR_WORKSPACE_ID` — the + env var is frozen at Claude-spawn time (see the `resume`/`reused` discussion below) + and never refreshed, so a herdr server restart / `update --handoff` that reassigns + workspace ids strands it. Stdout contract (`pane=`/`tab=`/`moved=`/… key=value + lines, exit codes) is untouched — only stderr got richer. ## `resume` mode: reopen a task tab a `/exit` closed diff --git a/CHANGELOG.md b/CHANGELOG.md index e7f2646..f4d146b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,9 @@ entries are grouped per plugin, newest first. ## work-system +### 1.9.1 — 2026-07-19 +- `herdr-launch.sh` no longer swallows herdr's own error on a launch/resume failure (`2>/dev/null`). Every herdr call that can fail now captures stderr, parses herdr's `{"error":{"code","message"}}` JSON defensively (raw text as fallback, never a traceback), and prints it before the existing generic diagnostic. When the error names an invalid workspace placement (`agent_placement_not_found` or the message otherwise naming the target), a one-line actionable hint is appended pointing at a stale `HERDR_WORKSPACE_ID`. Diagnostics only — stdout key=value lines, exit codes, and `moved`/`reused`/`resumed`/`focused` semantics are unchanged. + ### 1.9.0 — 2026-07-17 - `/kickoff` no longer hardcodes Claude as the worktree worker. With no flag it launches the repo's **default** agent — a single committed per-project setting (`.claude/work-system-agent`); if none is set, `/kickoff` shows a picker and offers to save your choice as that default. No global default and no shipped fallback. Override per run with a flag: `--fable`/`--opus`, `--codex`/`--sol`, `--grok`, `--agent `, or `--pick` (force the picker). New `scripts/agent-registry.sh` is the single source of truth (registry-driven aliases, the project `default get`/`set`, and an availability probe — CLI install + auth, plus model-level for grok via `grok models` so a model the CLI no longer offers reads as unavailable instead of failing at launch); `herdr-launch.sh` execs the resolved worker argv instead of a hardcoded `claude`. Non-Claude workers degrade honestly: they get a bootstrap prompt (read `TASK.md`, drive to a PR) instead of `/continue`, `/close` teardown stays CLI-agnostic, and `/continue`'s reopen documents the per-CLI resume command. Covered by `test_agent_registry.py`. diff --git a/plugins/work-system/.claude-plugin/plugin.json b/plugins/work-system/.claude-plugin/plugin.json index c12d67b..2b5a720 100644 --- a/plugins/work-system/.claude-plugin/plugin.json +++ b/plugins/work-system/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "work-system", "description": "Generic task and worktree workflow system for Claude Code. Manage tasks as markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, or grok), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.9.0", + "version": "1.9.1", "author": { "name": "gering" }, diff --git a/plugins/work-system/scripts/herdr-launch.sh b/plugins/work-system/scripts/herdr-launch.sh index df3cbc1..46fa0c1 100755 --- a/plugins/work-system/scripts/herdr-launch.sh +++ b/plugins/work-system/scripts/herdr-launch.sh @@ -147,6 +147,50 @@ try: print(pane + "|" + tab) except Exception: print("|")' +# error diagnostics: herdr emits {"error":{"code":…,"message":…}} on stderr for +# every failed call above. Extract it defensively — any exception (not JSON, no +# "error" key, …) yields nothing, never a traceback on the user's terminal. +extract_herdr_error='import sys, json +try: + err = json.load(sys.stdin)["error"] + print((err.get("code") or "") + "|" + (err.get("message") or "")) +except Exception: + pass' + +# herdr_diag — turn a captured herdr stderr blob into +# one-or-two diagnostic lines: herdr's error.code/message when the blob parses as +# the JSON error schema, else the raw text relayed verbatim (trimmed). Prints +# nothing for an empty blob. Appends a one-line actionable hint when the error +# names an invalid placement target (the "stale $HERDR_WORKSPACE_ID after a herdr +# restart/handoff" case this repo has hit in practice) — never more than one hint +# line, so the diagnostic doesn't nag. +herdr_diag() { + raw="$1" + ws="$2" + [ -n "$raw" ] || return 0 + parsed="$(printf '%s' "$raw" | python3 -c "$extract_herdr_error" 2>/dev/null || true)" + if [ -n "$parsed" ]; then + code="${parsed%%|*}" + msg="${parsed#*|}" + line="herdr error" + [ -n "$code" ] && line="$line code=$code" + [ -n "$msg" ] && line="$line: $msg" + else + code="" + msg="$raw" + line="herdr error: $(printf '%s' "$raw" | tr '\n' ' ' | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')" + fi + case "$code" in + agent_placement_not_found) stale=1 ;; + *) case "$msg" in + *"$ws"*"not found"*|*"$ws"*placement*) stale=1 ;; + *) stale=0 ;; + esac ;; + esac + [ "$stale" = 1 ] && line="$line +HERDR_WORKSPACE_ID=$ws is not a valid workspace on this herdr server (likely a stale id after a herdr restart/handoff) — relaunch from a live session or start the worker manually." + printf '%s\n' "$line" +} case "$mode" in launch) @@ -193,21 +237,39 @@ EOF_RESOLVE [ ${#worker_argv[@]} -gt 0 ] || { echo "agent-registry resolved no argv for $selector" >&2; exit 1; } fi - # Spawn the worker as argv and read back the pane id. + # Spawn the worker as argv and read back the pane id. stderr is captured (not + # discarded) so a failure can surface herdr's actual error.code/message instead + # of only the generic "no pane id" guard below. + start_err="$(mktemp)" start_json="$(herdr agent start "$label" --workspace "$workspace" \ - --cwd "$worktree" --no-focus -- "${worker_argv[@]}" 2>/dev/null || true)" + --cwd "$worktree" --no-focus -- "${worker_argv[@]}" 2>"$start_err" || true)" pane="$(printf '%s' "$start_json" | python3 -c "$extract_agent_pane" 2>/dev/null || true)" # Empty pane id → the agent did not start (broken socket / bad response). - [ -n "$pane" ] || { echo "herdr agent start did not return a pane id" >&2; exit 1; } + # Print herdr's own error first when captured — the generic message stays as + # the last-resort fallback for a truly pane-less/malformed response. + if [ -z "$pane" ]; then + diag="$(herdr_diag "$(cat "$start_err")" "$workspace")" + rm -f "$start_err" + [ -n "$diag" ] && printf '%s\n' "$diag" >&2 + echo "herdr agent start did not return a pane id" >&2 + exit 1 + fi + rm -f "$start_err" # caller's tab). If the move fails, the worker is still running — report it in # place rather than claiming a tab that does not exist. `agent=` tells the # caller which CLI×model was launched; the tab uses the glyph-stamped label. - if move_json="$(herdr pane move "$pane" --new-tab --label "$tab_label" --no-focus 2>/dev/null)"; then + move_err="$(mktemp)" + if move_json="$(herdr pane move "$pane" --new-tab --label "$tab_label" --no-focus 2>"$move_err")"; then tab="$(printf '%s' "$move_json" | python3 -c "$extract_moved_tab" 2>/dev/null || true)" + rm -f "$move_err" printf 'pane=%s\ntab=%s\nmoved=yes\nagent=%s\n' "$pane" "$tab" "$agent_name" else + diag="$(herdr_diag "$(cat "$move_err")" "$workspace")" + rm -f "$move_err" + [ -n "$diag" ] && printf '%s\n' "$diag" >&2 + echo "herdr pane move --new-tab failed for pane $pane (worker still running; no dedicated tab)" >&2 printf 'pane=%s\ntab=\nmoved=no\nagent=%s\n' "$pane" "$agent_name" fi ;; @@ -262,8 +324,9 @@ EOF_RESOLVE # (shell) pane id and tab id in one python3 pass. Split on the FIRST `|`, so an # empty pane id (with a present tab id) stays empty and trips the guard below, # rather than the tab id being mis-read as the pane id. + create_err="$(mktemp)" create_json="$(herdr tab create --workspace "$workspace" \ - --cwd "$worktree" --label "$tab_label" 2>/dev/null || true)" + --cwd "$worktree" --label "$tab_label" 2>"$create_err" || true)" pane_tab="$(printf '%s' "$create_json" | python3 -c "$extract_root_pane_tab" 2>/dev/null || true)" pane="${pane_tab%%|*}" tab="${pane_tab#*|}" @@ -272,12 +335,18 @@ EOF_RESOLVE # socket / bad JSON / pane-less result). Cannot run claude -c without a pane. If a # tab id WAS parsed (a pane-less/partial result from schema drift), the tab is real # and would be orphaned — close it before bailing so a drifted response can't leak a - # blank tab on every resume; then the caller shows the manual block. + # blank tab on every resume; then the caller shows the manual block. herdr's own + # error is printed first when captured — the generic message stays as the + # last-resort fallback. if [ -z "$pane" ]; then [ -n "$tab" ] && herdr tab close "$tab" >/dev/null 2>&1 || true + diag="$(herdr_diag "$(cat "$create_err")" "$workspace")" + rm -f "$create_err" + [ -n "$diag" ] && printf '%s\n' "$diag" >&2 echo "herdr tab create did not return a pane id" >&2 exit 1 fi + rm -f "$create_err" # Run `claude -c` INSIDE the shell pane — the /exit hardening (a later /exit # returns to the shell, keeping the tab alive). Prefix an explicit `cd ` @@ -290,10 +359,13 @@ EOF_RESOLVE # the user must run it by hand. (Catches a failed send, not `claude -c` erroring # later on a cwd with no prior session — the caller's wording stays tentative.) resumed=yes - if ! herdr pane run "$pane" "cd $(printf '%q' "$worktree") && claude -c" >/dev/null 2>&1; then + run_err="$(mktemp)" + if ! herdr pane run "$pane" "cd $(printf '%q' "$worktree") && claude -c" >/dev/null 2>"$run_err"; then resumed=no - echo "herdr pane run could not start 'claude -c' in $pane (tab is open; run it by hand)" >&2 + diag="$(herdr_diag "$(cat "$run_err")" "$workspace")" + echo "herdr pane run could not start 'claude -c' in $pane (tab is open; run it by hand)${diag:+ — $diag}" >&2 fi + rm -f "$run_err" # Focus the reopened tab — unlike kickoff's background launch, the user is # switching to it now. Report whether the focus took, so the caller doesn't From bc0801a3b2a6dd51d871ef17b6c06e2f3c2e2a6d Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Mon, 20 Jul 2026 12:26:13 +0200 Subject: [PATCH 2/3] Address swarm review findings on herdr error diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A swarm review (codex + grok, external-only) of the herdr-launch.sh error-surfacing change found several real gaps, all fixed here: - Scope the stale-HERDR_WORKSPACE_ID hint to calls that actually send --workspace (agent start, tab create) via a new ws_relevant param on herdr_diag — pane move/pane run never take --workspace, so an agent_placement_not_found there can't be a workspace-id problem. - Anchor the message-text fallback to a bounded workspace-id token instead of a loose substring, so ws=w1 no longer false-positives on a message naming w12. - Strip control/escape bytes from herdr's stderr before it reaches the terminal (herdr's stderr is untrusted server output). - Reorder the resume pane-run failure line to print herdr's diag first, matching the other three call sites. - Add `local` to herdr_diag's working variables (they leaked into the global scope under set -eu). - Add an EXIT trap for the four stderr-capture tempfiles, matching the existing herdr-tab-glyph.sh convention (a SIGINT between mktemp and the matching rm -f would otherwise leak the file). - kickoff/continue SKILL.md now explicitly instruct relaying the helper's stderr diagnostic to the user on every failure path — the richer diagnostic is only useful if the skill layer actually shows it, which the prior wording didn't require. Update CHANGELOG (same 1.9.1 entry, not yet released) and the herdr-kickoff-automation knowledge entry. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017D6Ji3oU3md1ENxY2WfbcT --- .../features/herdr-kickoff-automation.md | 16 ++++ CHANGELOG.md | 4 +- plugins/work-system/scripts/herdr-launch.sh | 75 +++++++++++++------ plugins/work-system/skills/continue/SKILL.md | 9 ++- plugins/work-system/skills/kickoff/SKILL.md | 10 ++- 5 files changed, 86 insertions(+), 28 deletions(-) diff --git a/.claude/knowledge/features/herdr-kickoff-automation.md b/.claude/knowledge/features/herdr-kickoff-automation.md index 30dd7d4..ed6d7a2 100644 --- a/.claude/knowledge/features/herdr-kickoff-automation.md +++ b/.claude/knowledge/features/herdr-kickoff-automation.md @@ -69,6 +69,22 @@ truth; this entry captures the durable design and one non-obvious gotcha. and never refreshed, so a herdr server restart / `update --handoff` that reassigns workspace ids strands it. Stdout contract (`pane=`/`tab=`/`moved=`/… key=value lines, exit codes) is untouched — only stderr got richer. + A swarm review of this change (external-only: codex + grok) caught three + refinements before merge, all applied: (1) the stale-workspace hint is scoped by + a `ws_relevant` flag on `herdr_diag` — only `agent start`/`tab create` actually + send `--workspace`, so `pane move`/`pane run` never get an + `agent_placement_not_found` misattributed to the workspace id; (2) the message + fallback (no parseable `code`) requires the workspace id to appear as a + bounded token, not a loose substring (`ws=w1` no longer false-positives on a + message naming `w12`); (3) `herdr_diag` strips control/escape bytes from + herdr's stderr before it reaches the terminal — herdr's stderr is untrusted + server output, so an embedded ANSI/OSC sequence must never be interpreted by + the user's terminal. **The diagnostic is only as useful as the skill layer + that relays it** — kickoff/continue SKILL.md's failure branches now explicitly + instruct relaying the helper's stderr to the user (they didn't before, which + would have silently dropped this entire improvement at exactly the layer the + original incident was observed at: the model narrating only the generic + guard message, never the captured herdr error). ## `resume` mode: reopen a task tab a `/exit` closed diff --git a/CHANGELOG.md b/CHANGELOG.md index f4d146b..7ab3793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,8 +52,8 @@ entries are grouped per plugin, newest first. ## work-system -### 1.9.1 — 2026-07-19 -- `herdr-launch.sh` no longer swallows herdr's own error on a launch/resume failure (`2>/dev/null`). Every herdr call that can fail now captures stderr, parses herdr's `{"error":{"code","message"}}` JSON defensively (raw text as fallback, never a traceback), and prints it before the existing generic diagnostic. When the error names an invalid workspace placement (`agent_placement_not_found` or the message otherwise naming the target), a one-line actionable hint is appended pointing at a stale `HERDR_WORKSPACE_ID`. Diagnostics only — stdout key=value lines, exit codes, and `moved`/`reused`/`resumed`/`focused` semantics are unchanged. +### 1.9.1 — 2026-07-20 +- `herdr-launch.sh` no longer swallows herdr's own error on a launch/resume failure (`2>/dev/null`). Every herdr call that can fail now captures stderr, strips control/escape bytes (never let a possibly-compromised herdr server inject terminal escapes), parses herdr's `{"error":{"code","message"}}` JSON defensively (raw text as fallback, never a traceback), and prints it before the existing generic diagnostic — `pane run`'s failure line now follows the same before-the-generic-message order as the other three call sites. When the error names an invalid workspace placement (`agent_placement_not_found`, or the message names the workspace id as a bounded token next to "not found"/"placement"), a one-line actionable hint is appended pointing at a stale `HERDR_WORKSPACE_ID` — scoped to the two calls that actually send `--workspace` (`agent start`, `tab create`), since `pane move`/`pane run` can't have a workspace-id problem. `kickoff`/`continue` SKILL.md now explicitly instruct relaying the helper's stderr diagnostic to the user on every failure path, so the richer diagnostic actually reaches the user instead of staying in the tool output. Diagnostics only — stdout key=value lines, exit codes, and `moved`/`reused`/`resumed`/`focused` semantics are unchanged. ### 1.9.0 — 2026-07-17 - `/kickoff` no longer hardcodes Claude as the worktree worker. With no flag it launches the repo's **default** agent — a single committed per-project setting (`.claude/work-system-agent`); if none is set, `/kickoff` shows a picker and offers to save your choice as that default. No global default and no shipped fallback. Override per run with a flag: `--fable`/`--opus`, `--codex`/`--sol`, `--grok`, `--agent `, or `--pick` (force the picker). New `scripts/agent-registry.sh` is the single source of truth (registry-driven aliases, the project `default get`/`set`, and an availability probe — CLI install + auth, plus model-level for grok via `grok models` so a model the CLI no longer offers reads as unavailable instead of failing at launch); `herdr-launch.sh` execs the resolved worker argv instead of a hardcoded `claude`. Non-Claude workers degrade honestly: they get a bootstrap prompt (read `TASK.md`, drive to a PR) instead of `/continue`, `/close` teardown stays CLI-agnostic, and `/continue`'s reopen documents the per-CLI resume command. Covered by `test_agent_registry.py`. diff --git a/plugins/work-system/scripts/herdr-launch.sh b/plugins/work-system/scripts/herdr-launch.sh index 46fa0c1..ca46d4c 100755 --- a/plugins/work-system/scripts/herdr-launch.sh +++ b/plugins/work-system/scripts/herdr-launch.sh @@ -103,6 +103,13 @@ command -v python3 >/dev/null 2>&1 || { echo "python3 not on PATH" >&2; exit 1; } [ -d "$worktree" ] || { echo "worktree dir not found: $worktree" >&2; exit 1; } +# The four herdr-call stderr-capture tempfiles below are each cleaned up with an +# explicit `rm -f` on their own success/failure branches, but a SIGINT/SIGTERM +# between their `mktemp` and that `rm -f` would otherwise leak the file — matching +# the existing convention in herdr-tab-glyph.sh. `${var:-}` tolerates a variable +# that isn't assigned yet (or on a code path that never used it) under `set -u`. +trap 'rm -f "${start_err:-}" "${move_err:-}" "${create_err:-}" "${run_err:-}"' EXIT + # Stamp the task's CURRENT state glyph onto the sidebar label (○ ● ◇ ◆ ✓ — the # same mapping the [ws …] statusline renders; ws-statusline.sh is the single # source, applied via herdr-tab-glyph.sh). Best-effort: any failure keeps the @@ -157,18 +164,30 @@ try: except Exception: pass' -# herdr_diag — turn a captured herdr stderr blob into -# one-or-two diagnostic lines: herdr's error.code/message when the blob parses as -# the JSON error schema, else the raw text relayed verbatim (trimmed). Prints -# nothing for an empty blob. Appends a one-line actionable hint when the error -# names an invalid placement target (the "stale $HERDR_WORKSPACE_ID after a herdr -# restart/handoff" case this repo has hit in practice) — never more than one hint -# line, so the diagnostic doesn't nag. +# herdr_diag — turn a captured herdr +# stderr blob into one-or-two diagnostic lines: herdr's error.code/message when +# the blob parses as the JSON error schema, else the raw text relayed verbatim +# (trimmed). Prints nothing for an empty blob. Control/escape bytes are stripped +# before ANY of this text reaches the terminal — herdr's stderr is untrusted +# output from a possibly-compromised/buggy server, and an embedded ANSI/OSC +# sequence must never be interpreted by the user's terminal. +# gates the stale-$HERDR_WORKSPACE_ID hint: pass 1 only for calls +# that actually send `--workspace $ws` (agent start, tab create) — for calls that +# don't (pane move, pane run) an agent_placement_not_found can't be a workspace-id +# problem, so the hint would misattribute the cause. When relevant, the message +# fallback additionally requires $ws to appear as a bounded token next to +# "not found"/"placement" — not merely as a loose substring, which could +# false-positive on an unrelated id that happens to contain $ws (e.g. ws=w1 +# inside a message naming w12). Never more than one hint line, so the +# diagnostic doesn't nag. herdr_diag() { + local raw ws ws_relevant clean parsed code msg line stale ws_ok esc_ws raw="$1" ws="$2" + ws_relevant="${3:-1}" [ -n "$raw" ] || return 0 - parsed="$(printf '%s' "$raw" | python3 -c "$extract_herdr_error" 2>/dev/null || true)" + clean="$(printf '%s' "$raw" | tr -dc '[:print:]\t\n')" + parsed="$(printf '%s' "$clean" | python3 -c "$extract_herdr_error" 2>/dev/null || true)" if [ -n "$parsed" ]; then code="${parsed%%|*}" msg="${parsed#*|}" @@ -177,16 +196,27 @@ herdr_diag() { [ -n "$msg" ] && line="$line: $msg" else code="" - msg="$raw" - line="herdr error: $(printf '%s' "$raw" | tr '\n' ' ' | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')" + msg="$clean" + line="herdr error: $(printf '%s' "$clean" | tr '\n' ' ' | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')" + fi + stale=0 + if [ "$ws_relevant" = 1 ]; then + case "$code" in + agent_placement_not_found) stale=1 ;; + esac + if [ "$stale" != 1 ] && [ -n "$ws" ]; then + ws_ok=0 + esc_ws="$(printf '%s' "$ws" | sed 's/[.[\*^$]/\\&/g')" + if printf '%s' "$msg" | grep -qE "(^|[^[:alnum:]_:.-])${esc_ws}([^[:alnum:]_:.-]|$)" 2>/dev/null; then + ws_ok=1 + fi + if [ "$ws_ok" = 1 ]; then + case "$msg" in + *"not found"*|*placement*) stale=1 ;; + esac + fi + fi fi - case "$code" in - agent_placement_not_found) stale=1 ;; - *) case "$msg" in - *"$ws"*"not found"*|*"$ws"*placement*) stale=1 ;; - *) stale=0 ;; - esac ;; - esac [ "$stale" = 1 ] && line="$line HERDR_WORKSPACE_ID=$ws is not a valid workspace on this herdr server (likely a stale id after a herdr restart/handoff) — relaunch from a live session or start the worker manually." printf '%s\n' "$line" @@ -249,7 +279,7 @@ EOF_RESOLVE # Print herdr's own error first when captured — the generic message stays as # the last-resort fallback for a truly pane-less/malformed response. if [ -z "$pane" ]; then - diag="$(herdr_diag "$(cat "$start_err")" "$workspace")" + diag="$(herdr_diag "$(cat "$start_err")" "$workspace" 1)" rm -f "$start_err" [ -n "$diag" ] && printf '%s\n' "$diag" >&2 echo "herdr agent start did not return a pane id" >&2 @@ -266,7 +296,7 @@ EOF_RESOLVE rm -f "$move_err" printf 'pane=%s\ntab=%s\nmoved=yes\nagent=%s\n' "$pane" "$tab" "$agent_name" else - diag="$(herdr_diag "$(cat "$move_err")" "$workspace")" + diag="$(herdr_diag "$(cat "$move_err")" "$workspace" 0)" rm -f "$move_err" [ -n "$diag" ] && printf '%s\n' "$diag" >&2 echo "herdr pane move --new-tab failed for pane $pane (worker still running; no dedicated tab)" >&2 @@ -340,7 +370,7 @@ EOF_RESOLVE # last-resort fallback. if [ -z "$pane" ]; then [ -n "$tab" ] && herdr tab close "$tab" >/dev/null 2>&1 || true - diag="$(herdr_diag "$(cat "$create_err")" "$workspace")" + diag="$(herdr_diag "$(cat "$create_err")" "$workspace" 1)" rm -f "$create_err" [ -n "$diag" ] && printf '%s\n' "$diag" >&2 echo "herdr tab create did not return a pane id" >&2 @@ -362,8 +392,9 @@ EOF_RESOLVE run_err="$(mktemp)" if ! herdr pane run "$pane" "cd $(printf '%q' "$worktree") && claude -c" >/dev/null 2>"$run_err"; then resumed=no - diag="$(herdr_diag "$(cat "$run_err")" "$workspace")" - echo "herdr pane run could not start 'claude -c' in $pane (tab is open; run it by hand)${diag:+ — $diag}" >&2 + diag="$(herdr_diag "$(cat "$run_err")" "$workspace" 0)" + [ -n "$diag" ] && printf '%s\n' "$diag" >&2 + echo "herdr pane run could not start 'claude -c' in $pane (tab is open; run it by hand)" >&2 fi rm -f "$run_err" diff --git a/plugins/work-system/skills/continue/SKILL.md b/plugins/work-system/skills/continue/SKILL.md index 8308861..c991a82 100644 --- a/plugins/work-system/skills/continue/SKILL.md +++ b/plugins/work-system/skills/continue/SKILL.md @@ -115,9 +115,14 @@ the prefix-stripped task name) — comparing the raw argument instead misroutes. append: "couldn't confirm the tab focus — switch to it manually (pane ``)." - **exit 0, `reused=no resumed=no`** → the tab opened but `claude -c` could not be sent; tell the user the tab is up at the worktree and to run `claude -c` in it by - hand. + hand. The helper's stderr for this call carries herdr's own error (code/message, + and a stale-workspace hint when applicable) — **relay that stderr text**, not + just "run it by hand." - **non-zero exit** → the helper could not automate (herdr/python3 missing, broken - socket, or no pane id). Show the manual block (b). + socket, or no pane id). **Relay the helper's stderr to the user first** — on a + broken-socket/no-pane-id failure it carries herdr's own error (code/message, and + a stale-workspace hint when applicable), the actual reason this happened, not + just the generic guard text. Then show the manual block (b). Success report for the `reused=no resumed=yes` case (fill `Tab` from the `tab=` line; drop the line if `tab=` is empty): diff --git a/plugins/work-system/skills/kickoff/SKILL.md b/plugins/work-system/skills/kickoff/SKILL.md index 53e15ba..55815be 100644 --- a/plugins/work-system/skills/kickoff/SKILL.md +++ b/plugins/work-system/skills/kickoff/SKILL.md @@ -193,7 +193,10 @@ is a per-repo committed file (`.claude/work-system-agent`), set via (`tab=`) with worker `agent=`. Report success (template below). - **exit 0 with `moved=no`** → the worker started but the tab move failed, so it is running as a split in *this* session's tab — tell the user it's here, not in - a new tab. + a new tab. The helper's stderr for this call carries herdr's own error + (code/message, and a stale-workspace hint when applicable) ahead of its own + generic line — **relay that stderr text to the user**, not just "the move + failed." - **After a successful launch (either `moved=` value), if `OFFER_DEFAULT=yes`** (the picker path, user chose to save): `bash "$REG" default set ""` (the `agent=` value) to write the repo's committed default. Mention it, and @@ -204,7 +207,10 @@ is a per-repo committed file (`.claude/work-system-agent`), set via `note=`). Report it verbatim (e.g. "codex not ready — run: codex login") and re-offer the picker or another selector. Nothing was spawned. - **other non-zero exit** → the helper could not automate (herdr/python3 missing, - broken socket, or no pane id). Show the manual block (b). + broken socket, or no pane id). **Relay the helper's stderr to the user first** + — on a broken-socket/no-pane-id failure it carries herdr's own error + (code/message, and a stale-workspace hint when applicable), the actual reason + this happened, not just the generic guard text. Then show the manual block (b). Success report (fill `Tab` from the helper's `tab=` line, `Agent` from `agent=`): ``` From 7871229f97045ef902119c101daf23559e16f86d Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Mon, 20 Jul 2026 12:44:27 +0200 Subject: [PATCH 3/3] Fix residual sanitization/matching bugs from the last review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second swarm review of the round-1 fixes found the fixes themselves were still wrong in four ways: - Control bytes were stripped only from the RAW stderr blob; a JSON backslash-u-001b-style escape is still plain printable text at that point and only becomes a real ESC byte after python3's json.load decodes it. code/message now get their own strip pass post-decode too. - The "workspace id as a bounded token" check tested token-presence and keyword-presence as independent ANDs, so "workspace w1 is healthy; agent placement is unavailable" still false-triggered the stale-workspace hint. Replaced the bash sed/grep chain with a single python3 re-based check that requires both in the same clause (split on ;/./,), case-insensitively. - The ERE-escaping of $ws before embedding it in a grep -E pattern only escaped a small char class, leaving several ERE metacharacters live — folded into the same python3 rewrite (re.escape covers every metachar). - $HERDR_WORKSPACE_ID itself was never sanitized before being interpolated into the printed hint line — only herdr's own stderr was. Also: the orphaned-tab `herdr tab close` cleanup call's own stderr was discarded even though the CHANGELOG claimed every failing call surfaces stderr — now captured too. And SKILL.md wording that promised a stale-workspace hint on the pane-move/pane-run paths (where it can never fire, ws_relevant=0) is corrected. Add test_herdr_launch.py (mirrors test_agent_registry.py's stub-the- CLI-on-PATH pattern) to lock in all of the above as regression coverage, since this class of bug — a diagnostic helper's own sanitization logic — clearly needed it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017D6Ji3oU3md1ENxY2WfbcT --- .../features/herdr-kickoff-automation.md | 26 +++ CHANGELOG.md | 2 +- plugins/work-system/scripts/herdr-launch.sh | 76 ++++--- .../work-system/scripts/test_herdr_launch.py | 185 ++++++++++++++++++ plugins/work-system/skills/continue/SKILL.md | 7 +- plugins/work-system/skills/kickoff/SKILL.md | 6 +- 6 files changed, 269 insertions(+), 33 deletions(-) create mode 100644 plugins/work-system/scripts/test_herdr_launch.py diff --git a/.claude/knowledge/features/herdr-kickoff-automation.md b/.claude/knowledge/features/herdr-kickoff-automation.md index ed6d7a2..10f144b 100644 --- a/.claude/knowledge/features/herdr-kickoff-automation.md +++ b/.claude/knowledge/features/herdr-kickoff-automation.md @@ -85,6 +85,32 @@ truth; this entry captures the durable design and one non-obvious gotcha. would have silently dropped this entire improvement at exactly the layer the original incident was observed at: the model narrating only the generic guard message, never the captured herdr error). + **A second swarm-review round on that first-round fix found it was still + wrong in four ways** — sanitization/anchoring code is exactly the kind of + code whose own fix deserves an adversarial pass, not just the original + feature: (1) control bytes were stripped only from the RAW stderr blob, but a + JSON ``-style escape is still plain printable text at that point — only + after `python3`'s `json.load` decodes it does it become a real ESC byte, so + `code`/`message` need their own strip pass post-decode; (2) the "workspace id + as a bounded token" fix from round one checked token-presence and keyword- + presence as independent ANDs, so `"workspace w1 is healthy; agent placement + is unavailable"` still false-triggered — a fixed character window wasn't + enough either (a short sentence puts the keyword in range regardless), so the + check now splits the message on `;`/`.`/`,` and requires both in the SAME + clause; (3) the ERE-escaping of `$ws` before embedding it in a `grep -E` + pattern only escaped `. [ \ * ^ $`, leaving `+ ? ( ) { } |` live — a + workspace id containing one of those could match unrelated text as a regex, + not a literal; (2) and (3) together made the bash sed/grep chain fragile + enough that it was replaced with a single `python3 re`-based check + (`re.escape` + per-clause matching, case-insensitive) instead of iterating + the bash version further; (4) `$HERDR_WORKSPACE_ID` itself (interpolated into + the hint line) was never sanitized — only herdr's own stderr was. All four + fixed, plus: the orphaned-tab `herdr tab close` cleanup call's own stderr was + being discarded (`2>&1 >/dev/null || true`) even though the CHANGELOG claimed + every failing call surfaces stderr — now captured too. `test_herdr_launch.py` + (new, mirrors `test_agent_registry.py`'s stub-the-CLI-on-PATH pattern) locks + in all of this — every case above, plus the stdout contract, as regression + coverage the first round shipped without. ## `resume` mode: reopen a task tab a `/exit` closed diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ab3793..808ef4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ entries are grouped per plugin, newest first. ## work-system ### 1.9.1 — 2026-07-20 -- `herdr-launch.sh` no longer swallows herdr's own error on a launch/resume failure (`2>/dev/null`). Every herdr call that can fail now captures stderr, strips control/escape bytes (never let a possibly-compromised herdr server inject terminal escapes), parses herdr's `{"error":{"code","message"}}` JSON defensively (raw text as fallback, never a traceback), and prints it before the existing generic diagnostic — `pane run`'s failure line now follows the same before-the-generic-message order as the other three call sites. When the error names an invalid workspace placement (`agent_placement_not_found`, or the message names the workspace id as a bounded token next to "not found"/"placement"), a one-line actionable hint is appended pointing at a stale `HERDR_WORKSPACE_ID` — scoped to the two calls that actually send `--workspace` (`agent start`, `tab create`), since `pane move`/`pane run` can't have a workspace-id problem. `kickoff`/`continue` SKILL.md now explicitly instruct relaying the helper's stderr diagnostic to the user on every failure path, so the richer diagnostic actually reaches the user instead of staying in the tool output. Diagnostics only — stdout key=value lines, exit codes, and `moved`/`reused`/`resumed`/`focused` semantics are unchanged. +- `herdr-launch.sh` no longer swallows herdr's own error on a launch/resume failure (`2>/dev/null`). The `agent start`, `pane move`, `tab create`, and `pane run` calls (plus the `tab close` orphan-cleanup that can run after a malformed `tab create`) now capture stderr, strip control/escape bytes both before AND after JSON-decoding a message (a ``-style escape only becomes a real control byte once decoded), parse herdr's `{"error":{"code","message"}}` JSON defensively (raw text as fallback, never a traceback), and print it before the existing generic diagnostic — `pane run`'s failure line now follows the same before-the-generic-message order as the other call sites. When the error names an invalid workspace placement (`agent_placement_not_found`, or the message names the sanitized workspace id as a bounded token within the same clause as "not found"/"placement", case-insensitively), a one-line actionable hint is appended pointing at a stale `HERDR_WORKSPACE_ID` — scoped to the two calls that actually send `--workspace` (`agent start`, `tab create`), since `pane move`/`pane run` can't have a workspace-id problem. `kickoff`/`continue` SKILL.md now explicitly instruct relaying the helper's stderr diagnostic to the user on every failure path, so the richer diagnostic actually reaches the user instead of staying in the tool output — worded per call site, since the stale-workspace hint can only ever appear on the `agent start`/`tab create` paths. Diagnostics only — stdout key=value lines, exit codes, and `moved`/`reused`/`resumed`/`focused` semantics are unchanged. Covered by new `test_herdr_launch.py`. ### 1.9.0 — 2026-07-17 - `/kickoff` no longer hardcodes Claude as the worktree worker. With no flag it launches the repo's **default** agent — a single committed per-project setting (`.claude/work-system-agent`); if none is set, `/kickoff` shows a picker and offers to save your choice as that default. No global default and no shipped fallback. Override per run with a flag: `--fable`/`--opus`, `--codex`/`--sol`, `--grok`, `--agent `, or `--pick` (force the picker). New `scripts/agent-registry.sh` is the single source of truth (registry-driven aliases, the project `default get`/`set`, and an availability probe — CLI install + auth, plus model-level for grok via `grok models` so a model the CLI no longer offers reads as unavailable instead of failing at launch); `herdr-launch.sh` execs the resolved worker argv instead of a hardcoded `claude`. Non-Claude workers degrade honestly: they get a bootstrap prompt (read `TASK.md`, drive to a PR) instead of `/continue`, `/close` teardown stays CLI-agnostic, and `/continue`'s reopen documents the per-CLI resume command. Covered by `test_agent_registry.py`. diff --git a/plugins/work-system/scripts/herdr-launch.sh b/plugins/work-system/scripts/herdr-launch.sh index ca46d4c..38bb085 100755 --- a/plugins/work-system/scripts/herdr-launch.sh +++ b/plugins/work-system/scripts/herdr-launch.sh @@ -108,7 +108,7 @@ command -v python3 >/dev/null 2>&1 || { echo "python3 not on PATH" >&2; exit 1; # between their `mktemp` and that `rm -f` would otherwise leak the file — matching # the existing convention in herdr-tab-glyph.sh. `${var:-}` tolerates a variable # that isn't assigned yet (or on a code path that never used it) under `set -u`. -trap 'rm -f "${start_err:-}" "${move_err:-}" "${create_err:-}" "${run_err:-}"' EXIT +trap 'rm -f "${start_err:-}" "${move_err:-}" "${create_err:-}" "${run_err:-}" "${close_err:-}"' EXIT # Stamp the task's CURRENT state glyph onto the sidebar label (○ ● ◇ ◆ ✓ — the # same mapping the [ws …] statusline renders; ws-statusline.sh is the single @@ -164,24 +164,46 @@ try: except Exception: pass' +# stale_ws_check — read a message on stdin, print 1 if $ws names +# the failing placement target, else 0. Bounded-token match (not a bare +# substring — ws=w1 must not match a message naming w12) with the "not +# found"/"placement" keyword required in the SAME clause as the token (split on +# ;/./,), not merely present anywhere else in the message — a fixed character +# window is not enough: "workspace w1 is healthy; agent placement is +# unavailable" puts the keyword within any reasonable window of a short +# sentence's token even though the two halves are unrelated clauses. +# Case-insensitive ("Not Found" from herdr must still match). python3's +# re.escape covers every ERE metacharacter — a hand-rolled sed/grep escape +# class is exactly the kind of thing that quietly misses one (`+ ? ( ) { } |` +# are the ones a naive `. [ \ * ^ $` class leaves live). +stale_ws_check='import sys, re +ws = sys.argv[1] if len(sys.argv) > 1 else "" +msg = sys.stdin.read() +if not ws: + print(0); sys.exit(0) +tok_re = re.compile(r"(? — turn a captured herdr # stderr blob into one-or-two diagnostic lines: herdr's error.code/message when # the blob parses as the JSON error schema, else the raw text relayed verbatim # (trimmed). Prints nothing for an empty blob. Control/escape bytes are stripped -# before ANY of this text reaches the terminal — herdr's stderr is untrusted -# output from a possibly-compromised/buggy server, and an embedded ANSI/OSC -# sequence must never be interpreted by the user's terminal. +# TWICE — once from the raw blob, once from the code/message pulled out of it — +# because a JSON ``-style escape is still plain printable text before +# python3's json.load decodes it into a real ESC byte; stripping only the raw +# blob lets a compromised herdr's escaped control sequence survive decoding and +# reach the terminal, defeating the whole point of the filter. # gates the stale-$HERDR_WORKSPACE_ID hint: pass 1 only for calls # that actually send `--workspace $ws` (agent start, tab create) — for calls that # don't (pane move, pane run) an agent_placement_not_found can't be a workspace-id -# problem, so the hint would misattribute the cause. When relevant, the message -# fallback additionally requires $ws to appear as a bounded token next to -# "not found"/"placement" — not merely as a loose substring, which could -# false-positive on an unrelated id that happens to contain $ws (e.g. ws=w1 -# inside a message naming w12). Never more than one hint line, so the -# diagnostic doesn't nag. +# problem, so the hint would misattribute the cause. $ws itself is sanitized +# before it's ever interpolated into the printed hint (it comes from +# $HERDR_WORKSPACE_ID, an environment value this script doesn't control). +# Never more than one hint line, so the diagnostic doesn't nag. herdr_diag() { - local raw ws ws_relevant clean parsed code msg line stale ws_ok esc_ws + local raw ws ws_relevant clean parsed code msg line stale ws_hit ws_clean raw="$1" ws="$2" ws_relevant="${3:-1}" @@ -189,8 +211,8 @@ herdr_diag() { clean="$(printf '%s' "$raw" | tr -dc '[:print:]\t\n')" parsed="$(printf '%s' "$clean" | python3 -c "$extract_herdr_error" 2>/dev/null || true)" if [ -n "$parsed" ]; then - code="${parsed%%|*}" - msg="${parsed#*|}" + code="$(printf '%s' "${parsed%%|*}" | tr -dc '[:print:]\t\n')" + msg="$(printf '%s' "${parsed#*|}" | tr -dc '[:print:]\t\n')" line="herdr error" [ -n "$code" ] && line="$line code=$code" [ -n "$msg" ] && line="$line: $msg" @@ -205,20 +227,15 @@ herdr_diag() { agent_placement_not_found) stale=1 ;; esac if [ "$stale" != 1 ] && [ -n "$ws" ]; then - ws_ok=0 - esc_ws="$(printf '%s' "$ws" | sed 's/[.[\*^$]/\\&/g')" - if printf '%s' "$msg" | grep -qE "(^|[^[:alnum:]_:.-])${esc_ws}([^[:alnum:]_:.-]|$)" 2>/dev/null; then - ws_ok=1 - fi - if [ "$ws_ok" = 1 ]; then - case "$msg" in - *"not found"*|*placement*) stale=1 ;; - esac - fi + ws_hit="$(printf '%s' "$msg" | python3 -c "$stale_ws_check" "$ws" 2>/dev/null || echo 0)" + [ "$ws_hit" = 1 ] && stale=1 fi fi - [ "$stale" = 1 ] && line="$line -HERDR_WORKSPACE_ID=$ws is not a valid workspace on this herdr server (likely a stale id after a herdr restart/handoff) — relaunch from a live session or start the worker manually." + if [ "$stale" = 1 ]; then + ws_clean="$(printf '%s' "$ws" | tr -dc '[:print:]\t\n')" + line="$line +HERDR_WORKSPACE_ID=$ws_clean is not a valid workspace on this herdr server (likely a stale id after a herdr restart/handoff) — relaunch from a live session or start the worker manually." + fi printf '%s\n' "$line" } @@ -369,7 +386,14 @@ EOF_RESOLVE # error is printed first when captured — the generic message stays as the # last-resort fallback. if [ -z "$pane" ]; then - [ -n "$tab" ] && herdr tab close "$tab" >/dev/null 2>&1 || true + if [ -n "$tab" ]; then + close_err="$(mktemp)" + if ! herdr tab close "$tab" >/dev/null 2>"$close_err"; then + close_diag="$(herdr_diag "$(cat "$close_err")" "$workspace" 0)" + [ -n "$close_diag" ] && printf 'herdr tab close cleanup for orphaned tab %s also failed: %s\n' "$tab" "$close_diag" >&2 + fi + rm -f "$close_err" + fi diag="$(herdr_diag "$(cat "$create_err")" "$workspace" 1)" rm -f "$create_err" [ -n "$diag" ] && printf '%s\n' "$diag" >&2 diff --git a/plugins/work-system/scripts/test_herdr_launch.py b/plugins/work-system/scripts/test_herdr_launch.py new file mode 100644 index 0000000..0ca71d9 --- /dev/null +++ b/plugins/work-system/scripts/test_herdr_launch.py @@ -0,0 +1,185 @@ +#!/usr/bin/env python3 +"""Tests for herdr-launch.sh's error-diagnostics path (herdr_diag) — run +standalone (`python3 test_herdr_launch.py`) or via check-structure.py's +"plugin tests" check. + +A fake `herdr` binary on PATH returns canned JSON/text on stderr for each +subcommand, so these tests exercise the REAL herdr-launch.sh end to end (not +herdr_diag in isolation) against the exact integration surface skills call. +Covers: the JSON error-schema extraction, the ws_relevant-gated + clause- +bounded stale-workspace hint (the real w9 incident, the "unrelated clause" +false-positive a loose AND-of-substrings would trip, case-insensitivity, and +an ERE-metachar workspace id), the double control-byte strip (pre- AND +post-JSON-decode, plus $ws itself), the tab-close cleanup diagnostic, and that +the stdout key=value contract is untouched by any of this on the success path. +""" +import json as jsonlib +import os +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).parent +SCRIPT = HERE / "herdr-launch.sh" + +FAILS = [] + + +def check(name, cond): + if not cond: + FAILS.append(name) + + +def shquote(s): + return "'" + s.replace("'", "'\\''") + "'" + + +def herdr_stub(cases): + """Build a fake `herdr` script from {" ": (stdout, stderr, exit)}.""" + lines = ["#!/usr/bin/env bash", 'case "$1 $2" in'] + for key, (out, err, rc) in cases.items(): + lines.append(f' "{key}")') + if out: + lines.append(f" printf '%s' {shquote(out)}") + if err: + lines.append(f" printf '%s' {shquote(err)} >&2") + lines.append(f" exit {rc}") + lines.append(" ;;") + lines.append(' *) echo "unhandled herdr stub call: $*" >&2; exit 9 ;;') + lines.append("esac") + return "\n".join(lines) + + +class Env: + """A throwaway PATH with a fake `herdr` stub, plus a worktree dir for + herdr-launch.sh to target.""" + + def __init__(self, cases): + self.tmp = tempfile.TemporaryDirectory() + root = Path(self.tmp.name) + self.worktree = root / "wt" + self.worktree.mkdir() + bindir = root / "bin" + bindir.mkdir() + herdr = bindir / "herdr" + herdr.write_text(herdr_stub(cases)) + herdr.chmod(0o755) + self.env = dict(os.environ) + self.env["PATH"] = f"{bindir}:{self.env['PATH']}" + + def run(self, *args): + return subprocess.run( + ["bash", str(SCRIPT), *args], + env=self.env, capture_output=True, text=True, timeout=20, + ) + + def close(self): + self.tmp.cleanup() + + +# --- the real incident: agent_placement_not_found names the workspace ------ # +e = Env({"agent start": ("", jsonlib.dumps( + {"error": {"code": "agent_placement_not_found", + "message": "agent placement target w9 not found"}}), 1)}) +r = e.run("launch", "t", str(e.worktree), "w9") +check("incident: exit 1", r.returncode == 1) +check("incident: code/message shown", + "agent_placement_not_found" in r.stderr and "target w9 not found" in r.stderr) +check("incident: stale hint shown", + "HERDR_WORKSPACE_ID=w9 is not a valid workspace" in r.stderr) +check("incident: generic last-resort message still present", + "did not return a pane id" in r.stderr) +check("incident: no stdout on failure", r.stdout == "") +e.close() + +# --- token present + keyword present, but in UNRELATED clauses -> no hint -- # +e = Env({"agent start": ("", jsonlib.dumps( + {"error": {"code": "some_code", + "message": "workspace w1 is healthy; agent placement is unavailable"}}), 1)}) +r = e.run("launch", "t", str(e.worktree), "w1") +check("unrelated clauses: no stale hint", "is not a valid workspace" not in r.stderr) +check("unrelated clauses: code/message still shown", "some_code" in r.stderr) +e.close() + +# --- case-insensitive match ------------------------------------------------ # +e = Env({"agent start": ("", jsonlib.dumps( + {"error": {"code": "weird", "message": "Workspace w1 Not Found on server"}}), 1)}) +r = e.run("launch", "t", str(e.worktree), "w1") +check("case-insensitive: stale hint shown", "HERDR_WORKSPACE_ID=w1 is not a valid" in r.stderr) +e.close() + +# --- ERE-metachar workspace id must not false-match ------------------------ # +e = Env({"agent start": ("", jsonlib.dumps( + {"error": {"code": "other", "message": "aaab placement not found somewhere"}}), 1)}) +r = e.run("launch", "t", str(e.worktree), "a+b") +check("ere-metachar: no false stale hint", "is not a valid workspace" not in r.stderr) +e.close() + +# --- ws_relevant=0 (pane move) never gets the hint, even with the trigger code # +e = Env({ + "agent start": (jsonlib.dumps({"result": {"agent": {"pane_id": "w1:p5"}}}), "", 0), + "pane move": ("", jsonlib.dumps( + {"error": {"code": "agent_placement_not_found", "message": "pane target gone"}}), 1), +}) +r = e.run("launch", "t", str(e.worktree), "w1") +check("pane move: exit 0 (moved=no is not a hard failure)", r.returncode == 0) +check("pane move: code/message shown", "agent_placement_not_found" in r.stderr) +check("pane move: no stale hint (ws not relevant here)", "is not a valid workspace" not in r.stderr) +check("pane move: moved=no on stdout", "moved=no" in r.stdout) +check("pane move: pane= still reported", "pane=w1:p5" in r.stdout) +e.close() + +# --- control bytes stripped AFTER json-decoding too (not just the raw blob) # +esc_payload = jsonlib.dumps({"error": {"code": "x", "message": "hi \x1b[31mRED\x1b[0m end"}}) +e = Env({"agent start": ("", esc_payload, 1)}) +r = e.run("launch", "t", str(e.worktree), "w1") +check("json-escaped ESC stripped post-decode", "\x1b" not in r.stderr) +check("surrounding text preserved", "RED" in r.stderr and "end" in r.stderr) +e.close() + +# --- $HERDR_WORKSPACE_ID itself is sanitized before interpolation ---------- # +evil_ws = "w1\x1bevil" +e = Env({"agent start": ("", jsonlib.dumps( + {"error": {"code": "agent_placement_not_found", "message": "target gone"}}), 1)}) +r = e.run("launch", "t", str(e.worktree), evil_ws) +check("evil $ws: ESC stripped from the printed hint", "\x1b" not in r.stderr) +check("evil $ws: remaining text still present", "evil" in r.stderr) +e.close() + +# --- tab-close cleanup failure surfaces its own diagnostic (resume path) --- # +e = Env({ + "pane list": (jsonlib.dumps( + {"result": {"panes": [{"tab_id": "w1:t1", "cwd": "/nowhere"}]}}), "", 0), + "tab create": (jsonlib.dumps({"result": {"tab_id": "w1:t9"}}), "", 0), + "tab close": ("", jsonlib.dumps( + {"error": {"code": "tab_not_found", "message": "tab w1:t9 already gone"}}), 1), +}) +r = e.run("resume", "t", str(e.worktree), "w1") +check("tab-close cleanup: exit 1", r.returncode == 1) +check("tab-close cleanup: close diag surfaced", + "tab_not_found" in r.stderr and "also failed" in r.stderr) +check("tab-close cleanup: create diag also surfaced", + "herdr tab create did not return a pane id" in r.stderr) +e.close() + +# --- success path: stdout contract untouched by any of the above ----------- # +e = Env({ + "agent start": (jsonlib.dumps({"result": {"agent": {"pane_id": "w1:p5"}}}), "", 0), + "pane move": (jsonlib.dumps( + {"result": {"move_result": {"created_tab": {"tab_id": "w1:t9"}}}}), "", 0), +}) +r = e.run("launch", "t", str(e.worktree), "w1") +check("success: exit 0", r.returncode == 0) +check("success: stdout contract unchanged", + r.stdout == "pane=w1:p5\ntab=w1:t9\nmoved=yes\nagent=claude\n") +check("success: no stderr diagnostics", r.stderr == "") +e.close() + + +if FAILS: + print("FAIL:") + for f in FAILS: + print(" -", f) + sys.exit(1) +print("herdr-launch.sh (herdr_diag): all tests passed") diff --git a/plugins/work-system/skills/continue/SKILL.md b/plugins/work-system/skills/continue/SKILL.md index c991a82..611b1e1 100644 --- a/plugins/work-system/skills/continue/SKILL.md +++ b/plugins/work-system/skills/continue/SKILL.md @@ -115,9 +115,10 @@ the prefix-stripped task name) — comparing the raw argument instead misroutes. append: "couldn't confirm the tab focus — switch to it manually (pane ``)." - **exit 0, `reused=no resumed=no`** → the tab opened but `claude -c` could not be sent; tell the user the tab is up at the worktree and to run `claude -c` in it by - hand. The helper's stderr for this call carries herdr's own error (code/message, - and a stale-workspace hint when applicable) — **relay that stderr text**, not - just "run it by hand." + hand. The helper's stderr for this call carries herdr's own error (code/message) + — **relay that stderr text**, not just "run it by hand." (This call never sends + `--workspace`, so unlike the failure below there is no stale-workspace hint to + expect here.) - **non-zero exit** → the helper could not automate (herdr/python3 missing, broken socket, or no pane id). **Relay the helper's stderr to the user first** — on a broken-socket/no-pane-id failure it carries herdr's own error (code/message, and diff --git a/plugins/work-system/skills/kickoff/SKILL.md b/plugins/work-system/skills/kickoff/SKILL.md index 55815be..0f25eef 100644 --- a/plugins/work-system/skills/kickoff/SKILL.md +++ b/plugins/work-system/skills/kickoff/SKILL.md @@ -194,9 +194,9 @@ is a per-repo committed file (`.claude/work-system-agent`), set via - **exit 0 with `moved=no`** → the worker started but the tab move failed, so it is running as a split in *this* session's tab — tell the user it's here, not in a new tab. The helper's stderr for this call carries herdr's own error - (code/message, and a stale-workspace hint when applicable) ahead of its own - generic line — **relay that stderr text to the user**, not just "the move - failed." + (code/message) ahead of its own generic line — **relay that stderr text to the + user**, not just "the move failed." (This call never sends `--workspace`, so + unlike the failure below there is no stale-workspace hint to expect here.) - **After a successful launch (either `moved=` value), if `OFFER_DEFAULT=yes`** (the picker path, user chose to save): `bash "$REG" default set ""` (the `agent=` value) to write the repo's committed default. Mention it, and