From bbc960a8ff311191445f307605fc0467e8aa5d62 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 14 Aug 2026 13:40:42 +0200 Subject: [PATCH] fix(skill): harden the fast path against the review findings Fifteen review findings on the fast-path rewrite plus one caught live, all verified against a real 1.18.1 server before landing: - sendwait picks a fresh seq (the epoch second) instead of a fixed 2, so a second prompt to the same worker is typed instead of silently swallowed as an already-applied duplicate; explicit seq remains for deliberate resends - sendwait self-heals stranded delivery: an Ink repaint occasionally eats the Enter (observed live), so a timed-out short first wait sends one bare \r and re-waits by resending the identical frame as a tagged duplicate - spawn_worker verifies the resolved casePath carries Codeman hooks (the same /api/hook-event marker the server checks), refusing names that resolve to linked or pre-existing hook-less directories instead of running the job in what may be the user's real repo - spawn_worker probes the trust dialog after a short 5s composer wait, not the full 45s, restoring the ladder staging verbs.md documents; on a readiness miss it deletes the half-spawned session and returns 1 with empty stdout, so a prompt can never be typed blind into a trust dialog - spawn_workers refuses duplicate case names and empty argument lists, and keys result files by index - section 1 is bash 3.2 compatible (indexed arrays, no declare -A), prints the full delivered/timedOut/signal tuple per worker with an explicit line for a missing result, deletes only workers whose turn really ended (a timeout means still working), cleans up spawned siblings when any spawn fails, and guards its mktemp - last_text takes the previous answer as an optional second argument for consecutive-turn reads (the transcript briefly serves the prior answer after a stop, observed live) - the stale duplicate bullets in section 1's closing list are gone - reference/verbs.md joins the mode-list drift guard's file list - README's skill inventory covers verbs.md and the new SKILL.md shape - the changeset is minor so the shipped release matches the 1.19.0 stamp Co-Authored-By: Claude Fable 5 --- .changeset/skill-fast-path.md | 19 ++- README.md | 3 +- skills/codeman/SKILL.md | 174 +++++++++++++++++++--------- skills/codeman/reference/recipes.md | 4 +- test/agent-skill-mode-lists.test.ts | 8 +- 5 files changed, 144 insertions(+), 64 deletions(-) diff --git a/.changeset/skill-fast-path.md b/.changeset/skill-fast-path.md index 6aacd296..7e933e0e 100644 --- a/.changeset/skill-fast-path.md +++ b/.changeset/skill-fast-path.md @@ -1,5 +1,5 @@ --- -'aicodeman': patch +'aicodeman': minor --- Make the `codeman` agent skill spawn workers fast instead of deliberating first. @@ -14,11 +14,18 @@ modes before the first call. `spawn_workers` (concurrent), `sendwait` and `last_text`. §1 composes them into the whole job in one Bash call, and says to stop reading there. - Dropped two ceremonies the measurements retired: the pid-poll loop (`wait-output` - already blocks on the composer) and the hooks check for cases `quick-start` creates, - which always carry hooks. That check is still required for linked cases and raw paths, - where its absence silently breaks send-and-wait. + already blocks on the composer) and the agent-driven hooks check, which is now folded + into `spawn_worker` itself as a single local grep of the resolved `casePath`, so a + name that resolves to a linked case or a hook-less pre-existing directory is refused + instead of silently running the job there. Linked cases and raw paths still require + the by-hand check, where its absence silently breaks send-and-wait. - The bootstrap's write condition now greps the version stamp, so a stale or truncated preamble file self-heals instead of failing and asking you to `rm` it by hand. +- `sendwait` picks a fresh `seq` per call (a fixed default made every second prompt to + the same worker a silently-swallowed duplicate) and self-heals stranded delivery: an + Ink repaint occasionally eats the Enter, leaving the prompt typed but unsubmitted + (observed live), so a timed-out first wait sends one bare `\r` and re-waits by + resending the identical frame as a tagged duplicate. - §5 moved to `reference/verbs.md`, leaving an index. SKILL.md is the only part paid on - every load and drops from ~16.4k to ~7.6k tokens; section numbers and anchors are - unchanged, so existing `§5.x` references still resolve. + every load and drops from ~16.4k to roughly 9k tokens (~35KB); section numbers and + anchors are unchanged, so existing `§5.x` references still resolve. diff --git a/README.md b/README.md index f99cbb01..aed55873 100644 --- a/README.md +++ b/README.md @@ -745,7 +745,8 @@ Those `DONE__` strings are the skill's **split marker** trick, and | File | Contents | | --------------------------------------------------------------------- | --------------------------------------------------------------------------------------------- | -| [`SKILL.md`](skills/codeman/SKILL.md) | Safety rules, rules of the road, and 9 single-purpose recipes. Always loaded. | +| [`SKILL.md`](skills/codeman/SKILL.md) | Safety rules, the ready-made fast path (spawn N workers, task them, collect), and the verb index. Always loaded. | +| [`reference/verbs.md`](skills/codeman/reference/verbs.md) | The 14 verbs in detail: readiness, send-and-wait, markers, interrupts, cleanup. On demand. | | [`reference/recipes.md`](skills/codeman/reference/recipes.md) | 6 worked multi-worker flows (fan-out, blocked-worker watch, messaging fan-out). On demand. | | [`reference/endpoints.md`](skills/codeman/reference/endpoints.md) | Full endpoint tables, error codes, per-mode signal table, capacity limits. On demand. | | [`reference/messaging.md`](skills/codeman/reference/messaging.md) | Talking to claude workers directly via Claude Code cross-session messaging. On demand. | diff --git a/skills/codeman/SKILL.md b/skills/codeman/SKILL.md index 515474fa..b887ddf7 100644 --- a/skills/codeman/SKILL.md +++ b/skills/codeman/SKILL.md @@ -37,9 +37,9 @@ you are not part of is not yours to drive. ⚠️ **Your shell state does not survive between tool calls.** Each Bash call starts a fresh shell, so `$API`, `$SELF`, the `CURL` array and `delete_session` are all gone by the next call, and `$$` is a different pid. **The filesystem does survive**, so write -the preamble to a file once and source it afterwards, rather than re-pasting ~30 lines -at the top of every call (a half-re-pasted preamble used to be the single most likely -way to break a run). +the preamble to a file once and source it afterwards, rather than re-pasting a +hundred-odd lines at the top of every call (a half-re-pasted preamble used to be the +single most likely way to break a run). Run this block once per Codeman session: @@ -97,57 +97,109 @@ _composer_up() { # -> "true"/"false". `shift+tab` is the one --data-urlencode "timeout=$2" | jq -r '.data.wait.matched // false' } # spawn_worker [mode] -> session id on stdout, diagnostics on stderr. -# quick-start AND readiness in one call. There is deliberately no pid poll: wait-output -# already blocks until the composer draws, and pid!=null proved startup, never readiness. +# quick-start AND readiness in one call, with a strict contract: NON-EMPTY stdout means +# a READY claude worker in a hook-carrying case. Anything less is rc 1 with EMPTY +# stdout, and the half-spawned session is deleted here rather than handed back, because +# a worker that never drew its composer would eat the task prompt with its trust +# dialog. There is deliberately no pid poll: wait-output already blocks until the +# composer draws, and pid!=null proved startup, never readiness. spawn_worker() { - local name="${1:?spawn_worker needs a case name}" mode="${2:-claude}" q sid r + local name="${1:?spawn_worker needs a case name}" mode="${2:-claude}" q sid cp r q=$("${CURL[@]}" -X POST "$API/api/v1/quick-start" -H 'Content-Type: application/json' \ -d "$(jq -nc --arg n "$name" --arg m "$mode" '{caseName:$n,mode:$m}')") sid=$(jq -r 'if .success then .data.sessionId else empty end' <<<"$q") # NOT retryable in a loop: every quick-start failure code is terminal (§5.1). [ -n "$sid" ] || { jq -c '{error,errorCode}' <<<"$q" >&2; return 1; } [ "$mode" = claude ] || { printf '%s\n' "$sid"; return 0; } # only claude draws a composer - r=$(_composer_up "$sid" 45000) - if [ "$r" != true ] && "${CURL[@]}" -G "$API/api/v1/sessions/$sid/wait-output" \ - --data-urlencode 'match=trust' --data-urlencode 'from=buffer' --data-urlencode 'timeout=2000' \ - | jq -e '.data.wait.matched' >/dev/null; then - # Codeman's own auto-accept gives up after 90 s / 3 tries; this is that bounded fallback. - "${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" -H 'Content-Type: application/json' \ - -d "$(jq -nc --arg c "$CID-$sid" '{input:"\r",useMux:true,clientId:$c,seq:1}')" >/dev/null + # quick-start RESOLVES the name before creating: a linked case or an existing dir + # wins over a fresh scratch case, so "created => hooks" is only true after this one + # local grep (the same marker the server itself checks for). No marker means sendwait + # would false-resolve on flapping idle, possibly inside the user's REAL repo: refuse + # rather than run the job there. + cp=$(jq -r '.data.casePath // empty' <<<"$q") + grep -qs '/api/hook-event' "$cp/.claude/settings.local.json" || { + echo "case '$name' resolved to '$cp', which has no Codeman hooks (linked or pre-existing?): pick an unused name, or work §5.1+§5.5 by hand" >&2 + delete_session "$sid" >/dev/null; return 1; } + # Short composer wait FIRST, then the trust-dialog probe: a case still showing the + # dialog can never pass the composer wait, so probing early keeps a cold case from + # paying the whole long wait before the fallback even runs (§5.2). A warm case + # matches in under a second and never reaches the probe. + r=$(_composer_up "$sid" 5000) + if [ "$r" != true ]; then + if "${CURL[@]}" -G "$API/api/v1/sessions/$sid/wait-output" \ + --data-urlencode 'match=trust' --data-urlencode 'from=buffer' --data-urlencode 'timeout=2000' \ + | jq -e '.data.wait.matched' >/dev/null; then + # Codeman's own auto-accept gives up after 90 s / 3 tries; this is that bounded fallback. + "${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" -H 'Content-Type: application/json' \ + -d "$(jq -nc --arg c "$CID-$sid" '{input:"\r",useMux:true,clientId:$c,seq:1}')" >/dev/null + fi r=$(_composer_up "$sid" 45000) fi - [ "$r" = true ] || echo "worker $sid never drew a composer; inspect terminal?tail=" >&2 + [ "$r" = true ] || { echo "worker $sid never drew a composer; deleted it. Retry by hand via the §5.2 ladder (its billed stage-4 probe included)" >&2 + delete_session "$sid" >/dev/null; return 1; } printf '%s\n' "$sid" } -# spawn_workers ... -> one " " line per worker, in order. -# CONCURRENT: N workers cost about what one costs. Spawning them one Bash call at a time -# is the single biggest avoidable delay in this skill. +# spawn_workers ... -> one " " line per worker, in order; +# the sessionId column is EMPTY for a spawn that failed (stderr has why). CONCURRENT: +# N workers cost about what one costs. Spawning them one Bash call at a time is the +# single biggest avoidable delay in this skill. Names must be UNIQUE: two workers in +# one case directory co-edit the same tree (§4), so a repeat is an error here, not a race. spawn_workers() { - local d n + local d n i=0 + [ "$#" -gt 0 ] || { echo "spawn_workers: no case names given" >&2; return 1; } + [ -z "$(printf '%s\n' "$@" | sort | uniq -d)" ] || { echo "spawn_workers: duplicate case names" >&2; return 1; } d=$(mktemp -d "${TMPDIR:-/tmp}/codeman-spawn.XXXXXX") || return 1 - for n in "$@"; do ( spawn_worker "$n" > "$d/$n" ) & done + for n in "$@"; do ( spawn_worker "$n" > "$d/$i" ) & i=$((i+1)); done wait - for n in "$@"; do printf '%s %s\n' "$n" "$(cat "$d/$n" 2>/dev/null)"; done + i=0; for n in "$@"; do printf '%s %s\n' "$n" "$(cat "$d/$i" 2>/dev/null)"; i=$((i+1)); done rm -rf "$d" } -# sendwait [seq] -> blocks until that worker's turn ENDS. One billed turn. -# The \r and the per-worker clientId are applied here, which is why you never hand-build -# this body. Trustworthy wherever quick-start CREATED the case (those always have hooks). +# sendwait [seq] -> blocks until that worker's turn ENDS (~10 min ceiling +# across its two waits). One billed turn. The \r and the per-worker clientId are applied +# here, which is why you never hand-build this body. seq defaults to the CURRENT EPOCH +# SECOND so that every new prompt is a new frame: the server drops any (clientId,seq) +# pair it has already applied, so a fixed default would make every later prompt to that +# worker a silent no-op that still "succeeds" and reports the previous turn's state. +# Pass seq explicitly for exactly one reason: resending a possibly-delivered frame as a +# deliberate duplicate, at the SAME number (§5.3). +# Delivery is SELF-HEALING: an Ink repaint occasionally eats the Enter, leaving the +# typed prompt stranded on the composer while a long wait runs its whole timeout +# (observed live). So the first wait is short; on its timeout a bare \r goes out (the +# missing Enter when the prompt is stranded, a no-op when the turn is genuinely +# running), then the ORIGINAL frame is resent unchanged, which the server takes as a +# tagged duplicate: it re-waits without retyping (§5.3). Trustworthy only for a claude +# worker spawn_worker handed back (hooks vetted); hook-less workspaces and other modes +# resolve on flapping idle: markers instead (§5.5). sendwait() { - local sid="${1:?}" p="${2:?}" seq="${3:-2}" - "${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" -H 'Content-Type: application/json' \ - --data-binary "$(jq -nc --arg p "$p" --arg c "$CID-$sid" --argjson s "$seq" \ - '{input:($p+"\r"),useMux:true,clientId:$c,seq:$s,wait:true,waitTimeout:600000}')" + local sid="${1:?}" p="${2:?}" seq="${3:-$(date +%s)}" body r + body=$(jq -nc --arg p "$p" --arg c "$CID-$sid" --argjson s "$seq" \ + '{input:($p+"\r"),useMux:true,clientId:$c,seq:$s,wait:true,waitTimeout:20000}') + r=$("${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" \ + -H 'Content-Type: application/json' --data-binary "$body") + if jq -e '.data.delivered and .data.wait.timedOut' <<<"$r" >/dev/null 2>&1; then + "${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" -H 'Content-Type: application/json' \ + -d "$(jq -nc --arg c "$CID-$sid" --argjson s "$(date +%s)" \ + '{input:"\r",useMux:true,clientId:$c,seq:$s}')" >/dev/null + r=$("${CURL[@]}" -X POST "$API/api/v1/sessions/$sid/input" \ + -H 'Content-Type: application/json' --data-binary "$(jq -c '.waitTimeout=580000' <<<"$body")") + fi + printf '%s\n' "$r" } -# last_text -> that worker's last assistant message. Polled, because the transcript -# write LAGS the stop signal. Non-zero exit means it really never wrote one. +# last_text [prev] -> that worker's last assistant message. Polled, because the +# transcript write LAGS the stop signal, and "some text exists" is not "THIS turn's +# text exists": right after a SECOND turn on the same worker the endpoint still serves +# the previous answer for a beat (observed live). When reading consecutive turns, pass +# the previous answer as [prev]: the poll then holds out for text that differs from it, +# falling back to whatever it last saw if the budget runs dry, so an honestly repeated +# answer still comes back. Non-zero exit means the worker really never wrote one. last_text() { - local t + local t="" prev="${2:-}" for _ in $(seq 1 15); do t=$("${CURL[@]}" "$API/api/v1/sessions/$1/last-response" | jq -r '.data.text // empty') - [ -n "$t" ] && { printf '%s\n' "$t"; return 0; } + [ -n "$t" ] && [ "$t" != "$prev" ] && { printf '%s\n' "$t"; return 0; } sleep 1 done + [ -n "$t" ] && { printf '%s\n' "$t"; return 0; } return 1 } @@ -208,17 +260,29 @@ to assemble and no per-call body to hand-build. ```bash . "${XDG_CACHE_HOME:-$HOME/.cache}/codeman-agent-$CODEMAN_SESSION_ID.sh" # §0 -declare -A W P -P[alpha]='reply with one line: the absolute path of your working directory' -P[beta]='reply with one line: your model name' +N=(alpha beta) # one FRESH case name per worker +T=('reply with one line: the absolute path of your working directory' + 'reply with one line: your model name') # tasks, same order as N -while read -r n s; do W[$n]=$s; done < <(spawn_workers "${!P[@]}") # concurrent -for n in "${!W[@]}"; do [ -n "${W[$n]}" ] || { echo "spawn failed: $n (see §5.1)"; exit 1; }; done +S=(); while read -r _ s; do S+=("$s"); done < <(spawn_workers "${N[@]}") # concurrent +for i in "${!N[@]}"; do [ -n "${S[$i]:-}" ] || FAIL=1; done +[ -z "${FAIL:-}" ] || { echo "a spawn failed (stderr says why; §5.1): deleting the siblings" + for s in "${S[@]}"; do [ -n "$s" ] && delete_session "$s" >/dev/null; done; exit 1; } -D=$(mktemp -d); for n in "${!W[@]}"; do sendwait "${W[$n]}" "${P[$n]}" > "$D/$n" & done; wait -for n in "${!W[@]}"; do jq -c --arg n "$n" '{worker:$n, signal:.data.wait.signal}' "$D/$n"; done -for n in "${!W[@]}"; do echo "== $n"; last_text "${W[$n]}" || echo "(no response written)"; done -for n in "${!W[@]}"; do delete_session "${W[$n]}" >/dev/null; done; rm -rf "$D" +D=$(mktemp -d) || { for s in "${S[@]}"; do delete_session "$s" >/dev/null; done; exit 1; } +for i in "${!N[@]}"; do sendwait "${S[$i]}" "${T[$i]}" > "$D/$i" & done; wait +for i in "${!N[@]}"; do + jq -ce --arg n "${N[$i]}" \ + '{worker:$n,delivered:.data.delivered,timedOut:.data.wait.timedOut,signal:.data.wait.signal}' \ + "$D/$i" || echo "{\"worker\":\"${N[$i]}\",\"error\":\"send produced no result\"}" + echo "== ${N[$i]}"; last_text "${S[$i]}" || echo "(no response written)" +done +for i in "${!N[@]}"; do # delete ONLY what finished; a timeout means STILL WORKING (§3 rule 5) + if jq -e '.success and .data.delivered and (.data.wait.timedOut|not)' "$D/$i" >/dev/null 2>&1 + then delete_session "${S[$i]}" >/dev/null + else echo "kept ${N[$i]} (${S[$i]}): its line above says why; re-wait or repair (§5.3), then delete_session it" + fi +done; rm -rf "$D" ``` Measured against a live 1.18.0 server: two cold workers spawned and ready in **6.3 s**, @@ -229,26 +293,26 @@ the time went into deliberation, not the API. The three things that actually cos `wait`, as above, makes N workers cost about what one costs. - **Re-deriving the happy path** from §5.1 + §5.2 + §5.3 + §5.10. That is what the preamble functions exist to end. Compose them; do not rebuild them. -- **Verifying what is already guaranteed.** Two checks specifically are not worth a call - here: `quick-start` on a case name **it creates** always writes hooks, so `stop` fires - and `sendwait` is trustworthy without reading `settings.local.json`; and the pid poll is - dead weight, because `wait-output` already blocks on the composer. +- **Verifying what is already checked for you.** Two verifications specifically are not + worth a call here, because `spawn_worker` carries them: the hooks check (it refuses a + name that resolved to a hook-less directory with one local grep, so a worker it hands + back always has a working `stop` and `sendwait` is trustworthy), and the pid poll, + which is dead weight because `wait-output` already blocks on the composer. Four things this block leans on, each one link away, no detour needed to run it: -- Those case names create **fresh scratch directories** under `~/codeman-cases/`, - not your repo. Spawning where the work actually is (a linked case, a git worktree) is - a different call with **no hooks**, and it is the mistake with the highest cost: §5.1. - That is also the one case where the hook check above is required rather than skippable. -- `sendwait` supplies the `\r`. A prompt without it is never submitted and everything - downstream times out: §3. +- Those case names must be **fresh scratch names**: they create + `~/codeman-cases/`, not your repo. A name that already means something (a + linked case, a pre-existing directory) is refused by `spawn_worker` rather than + silently reused. Spawning where the work actually is (a linked case, a git worktree) + is a different call with **no hooks**, and the costliest mistake in this skill: §5.1. +- `sendwait` supplies the `\r`, picks a fresh `seq`, and self-heals a stranded Enter. + A prompt without the `\r` is never submitted (§3), a reused `seq` is silently + swallowed as an already-applied duplicate, and an Enter eaten by an Ink repaint + strands the prompt on the composer until a bare `\r` follows: all three are reasons + to let `sendwait` build the call rather than hand-rolling it. - Each `sendwait` costs that worker one billed turn, as does every prompt you send it. - Deleting the sessions does **not** remove the case directories: §5.14. -- The prompt ends with `\r`. Without it nothing is submitted and everything downstream - times out: §3. -- The send-and-wait call costs the worker one billed turn, as does every prompt you - send it. -- Deleting the session does **not** remove the case directory it created: §5.14. ## 2. What do you want to do? diff --git a/skills/codeman/reference/recipes.md b/skills/codeman/reference/recipes.md index 74965621..a1279adc 100644 --- a/skills/codeman/reference/recipes.md +++ b/skills/codeman/reference/recipes.md @@ -285,7 +285,9 @@ other was still running). Each send costs its worker one billed turn: `sendwait [seq]` is a preamble function ([SKILL.md §0](../SKILL.md#0-guard-and-bootstrap)); it applies the `\r` and a per-worker `clientId`, -so do not redefine it here. Background one call per worker and `wait`: +and picks a fresh `seq` (the current epoch second) per call, so do not redefine it here +and pass `seq` yourself only to resend an identical frame as a deliberate duplicate. +Background one call per worker and `wait`: ```bash D=$(mktemp -d) # a function's stdout is per-worker, so collect it in files, not a var diff --git a/test/agent-skill-mode-lists.test.ts b/test/agent-skill-mode-lists.test.ts index 5092a506..821f5f0b 100644 --- a/test/agent-skill-mode-lists.test.ts +++ b/test/agent-skill-mode-lists.test.ts @@ -45,7 +45,13 @@ import type { SessionMode } from '../src/types/session.js'; const HERE = fileURLToPath(new URL('.', import.meta.url)); const SKILL_DIR = join(HERE, '../skills/codeman'); -const SKILL_FILES = ['SKILL.md', 'reference/endpoints.md', 'reference/messaging.md', 'reference/recipes.md']; +const SKILL_FILES = [ + 'SKILL.md', + 'reference/endpoints.md', + 'reference/messaging.md', + 'reference/recipes.md', + 'reference/verbs.md', +]; /** Modes the API actually accepts, read off the schema rather than restated here. */ function schemaModes(schema: typeof CreateSessionSchema | typeof QuickStartSchema): SessionMode[] {