From 90d8339d98e3664f44e5e415f13b5fad987a3ddc Mon Sep 17 00:00:00 2001 From: REPPL Date: Wed, 12 Aug 2026 11:50:05 +0200 Subject: [PATCH 1/2] fix: one SessionStart entry, and a successful install reads as success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hooks/hooks.json listed the bootstrap and the two binary-backed commands as three sibling SessionStart entries and relied on list order. The harness runs every hook matching an event in parallel, so both gated entries raced the ~10.7 MB download, lost, printed "the plugin binary is not installed", and genuinely did not run — on every fresh install and every plugin update, since an update lands in a fresh cache directory with no binary. The three entries collapse into ONE command that runs the bootstrap and then both binary calls in a single shell, so the sequencing is owned by the manifest rather than assumed of the harness. The bootstrap's own message is emitted first, which is what the transcript renders: on a fresh install the visible line is the checksum-verified success rather than one of two missing-binary complaints (iss-208), and the two complaints collapse into one. A refusal keeps its message and its exit code, a genuinely absent binary is still said out loud, and the binary calls' stdout still reaches the model untouched. spc-21 carried the false warrant this rested on — "ordering within one event's hook list is preserved by the harness" — as a load-bearing claim. It is corrected in place with a note not to restore it, along with the brief's two descriptions of the manifest. Parallel hook execution and the plugin cache are not present in CI, so the end-to-end proof is the manual install gate. What CI holds is new: the manifest's shape (one entry, one command, bootstrap before the binary calls, valid POSIX sh) and the chained command's behaviour against fixtures — success leads the stderr, both calls run against the just-installed binary, the missing binary is reported exactly once, a refusal's first line and exit code survive, and an unset CLAUDE_PLUGIN_ROOT does nothing. Resolves iss-204, iss-208. Assisted-by: Claude:claude-opus-5[1m] --- .../development/brief/04-surfaces/01-ahoy.md | 6 +- .../brief/05-internals/03-configuration.md | 3 +- ...all-just-works-the-hook-binary-survives.md | 32 +- ...-bootstrap-sh-and-the-two-binary-backed.md | 2 + ...ap-install-is-rendered-to-the-user-as-a.md | 2 + CHANGELOG.md | 23 ++ hooks/hooks.json | 4 +- .../surface/cli/hooks_sessionstart_test.go | 300 ++++++++++++++++++ 8 files changed, 358 insertions(+), 14 deletions(-) rename .abcd/work/issues/{open => resolved}/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md (84%) rename .abcd/work/issues/{open => resolved}/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md (81%) create mode 100644 internal/surface/cli/hooks_sessionstart_test.go diff --git a/.abcd/development/brief/04-surfaces/01-ahoy.md b/.abcd/development/brief/04-surfaces/01-ahoy.md index 289587f5..60265c4d 100644 --- a/.abcd/development/brief/04-surfaces/01-ahoy.md +++ b/.abcd/development/brief/04-surfaces/01-ahoy.md @@ -164,8 +164,10 @@ Steps, run in parallel where independent: `hooks/hooks.json` is present in the plugin install AND contains the three required event entries (`UserPromptSubmit`, `SessionStart`, `PreCompact`) each referencing the expected prompt-router hook commands. The shipped - manifest also wires `abcd hook session-start` (a second `SessionStart` - command), `abcd hook session-end` (a `SessionEnd` event), and `abcd guard + manifest also wires `abcd hook session-start` (chained after the bootstrap + and `prompt-router-reset` inside the ONE `SessionStart` command — the + harness runs sibling hooks in parallel, so the event carries a single + entry), `abcd hook session-end` (a `SessionEnd` event), and `abcd guard hook` (a `PreToolUse` event, matcher `Bash`, that checks a shell command against the hazard registry before it runs) — five event types in all; verification covers only the three prompt-router commands above. A missing or diff --git a/.abcd/development/brief/05-internals/03-configuration.md b/.abcd/development/brief/05-internals/03-configuration.md index b55e47ea..e378495b 100644 --- a/.abcd/development/brief/05-internals/03-configuration.md +++ b/.abcd/development/brief/05-internals/03-configuration.md @@ -356,7 +356,8 @@ abcd/ │ ├── release-changelog-composer.md / ruthless-reviewer.md / security-reviewer.md │ └── sota-researcher.md # plus per-agent fixtures/ dirs, README.md, CHANGELOG.md └── hooks/ # Claude Code event hooks — each command shells directly to the binary - └── hooks.json # UserPromptSubmit → hook prompt-router; SessionStart → prompt-router-reset + session-start; + └── hooks.json # UserPromptSubmit → hook prompt-router; SessionStart → ONE chained command: + # bootstrap.sh, then prompt-router-reset + session-start (siblings would run in parallel); # PreToolUse (matcher Bash) → guard hook; PreCompact → prompt-router-reset; SessionEnd → session-end ``` diff --git a/.abcd/development/specs/closed/spc-21-a-fresh-plugin-install-just-works-the-hook-binary-survives.md b/.abcd/development/specs/closed/spc-21-a-fresh-plugin-install-just-works-the-hook-binary-survives.md index 0dfa9ecc..e7fd18ee 100644 --- a/.abcd/development/specs/closed/spc-21-a-fresh-plugin-install-just-works-the-hook-binary-survives.md +++ b/.abcd/development/specs/closed/spc-21-a-fresh-plugin-install-just-works-the-hook-binary-survives.md @@ -18,9 +18,10 @@ plus wiring plus reporting: no `internal/core` behaviour changes. - **`hooks/bootstrap.sh`** (new, committed): the self-provisioning script. Needs no abcd binary to run — the binary is exactly what is missing. -- **`hooks/hooks.json`**: the bootstrap becomes the first `SessionStart` - entry, ahead of the binary-backed `prompt-router-reset` / `session-start` - commands. +- **`hooks/hooks.json`**: the `SessionStart` event is ONE entry whose command + runs the bootstrap and then the binary-backed `prompt-router-reset` / + `session-start` calls in the same shell (see Hook wiring, and the correction + recorded there). - **`$CLAUDE_PLUGIN_ROOT/.binary-meta`** (new, written by the bootstrap): one small key=value file recording `release_tag`, `release_sha` (when resolvable), `fetched_at`, and `plugin_sha` — the input for skew reporting. @@ -85,15 +86,30 @@ Behaviour, in order: ### Hook wiring -`hooks.json` gains, as the first `SessionStart` hook: +`hooks.json` declares ONE `SessionStart` entry, whose command runs the +bootstrap and then the two binary-backed calls in a single shell: ```json -{"type": "command", "command": "\"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\""} +{"type": "command", "timeout": 240, "command": "… bootstrap.sh …; abcd hook prompt-router-reset; abcd hook session-start"} ``` -Ordering within one event's hook list is preserved by the harness, so the -binary-backed session hooks run after the bootstrap in the same event. The -`UserPromptSubmit`/`PreToolUse`/`PreCompact`/`SessionEnd` commands are +**Correction (2026-08-12, iss-204 / iss-208).** This section wired the +bootstrap as the first of THREE sibling `SessionStart` entries on the warrant +that *"ordering within one event's hook list is preserved by the harness, so +the binary-backed session hooks run after the bootstrap in the same event"*. +That warrant is **false**, and it was load-bearing: the harness runs every hook +matching an event **in parallel** (the hooks reference says so verbatim). Both +gated entries raced the ~10.7 MB download, lost, printed "the plugin binary is +not installed", and genuinely did not run — on every fresh install and every +plugin update, since an update lands in a fresh commit-stamped cache directory +with no binary. The three entries are collapsed into the one command above, so +the sequencing is OWNED by the manifest rather than assumed of the harness; the +two gated messages become one; and the bootstrap's own message is emitted +first, so the single line the transcript renders is the success rather than a +missing-binary complaint (iss-208). Nothing else in this spec rests on hook +ordering. Do not restore the ordering warrant. + +The `UserPromptSubmit`/`PreToolUse`/`PreCompact`/`SessionEnd` commands are unchanged — on the first-ever event before any `SessionStart` completed they fail as today (fail-open guard with UNGUARDED warning, per the intent's guard-window decision). diff --git a/.abcd/work/issues/open/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md b/.abcd/work/issues/resolved/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md similarity index 84% rename from .abcd/work/issues/open/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md rename to .abcd/work/issues/resolved/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md index ee375598..35a7a47a 100644 --- a/.abcd/work/issues/open/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md +++ b/.abcd/work/issues/resolved/iss-204-hooks-hooks-json-puts-bootstrap-sh-and-the-two-binary-backed.md @@ -7,6 +7,8 @@ category: "bug" source: "user-observation" found_during: "first manual plugin install test (2026-08-10)" found_at: "hooks/hooks.json" +resolution: "Collapsed the three SessionStart entries into one chained command (bootstrap, then prompt-router-reset and session-start in the same shell), so sequencing is owned by hooks.json rather than assumed of a harness that runs sibling hooks in parallel. spc-21's false ordering warrant corrected in place." +impact: fix --- hooks/hooks.json puts bootstrap.sh and the two binary-backed SessionStart hooks in ONE event group and relies on list order, but the harness runs all matching hooks in PARALLEL — the Claude Code hooks reference states verbatim 'All matching hooks run in parallel.' spc-21 asserts the opposite: 'Ordering within one event's hook list is preserved by the harness, so the binary-backed session hooks run after the bootstrap in the same event.' That warrant is false, and it is load-bearing. On a fresh install the two gated hooks evaluate [ -f "$CLAUDE_PLUGIN_ROOT/abcd" ] while bootstrap.sh is still downloading the ~10.7MB release binary (timeout 240), lose the race, and both print 'the plugin binary is not installed'. Observed on the first manual marketplace install (2026-08-10): both errors printed BEFORE bootstrap's own success notice, which is direct evidence of parallel execution. Consequence is not only noise — 'hook prompt-router-reset' and 'hook session-start' genuinely do not run for that session, and itd-105 AC#1 ('every hook executes successfully — no No such file or directory, no unguarded-shell warning') fails on every fresh install AND every plugin update, since each update lands in a fresh commit-stamped cache directory with no binary. Fix direction: collapse the three entries into ONE SessionStart command that runs bootstrap.sh and then the two binary calls in a single shell, so the sequencing is owned here rather than assumed of the harness. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md b/.abcd/work/issues/resolved/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md similarity index 81% rename from .abcd/work/issues/open/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md rename to .abcd/work/issues/resolved/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md index 4ed46928..44e9c520 100644 --- a/.abcd/work/issues/open/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md +++ b/.abcd/work/issues/resolved/iss-208-a-successful-bootstrap-install-is-rendered-to-the-user-as-a.md @@ -8,6 +8,8 @@ source: "user-observation" found_during: "first manual plugin install test (2026-08-10)" found_at: "hooks/bootstrap.sh" blocked_by: [iss-204] +resolution: "The chained SessionStart command emits the bootstrap's own message first, so the single line the transcript renders on a fresh install is the checksum-verified success rather than a missing-binary complaint. The non-zero exit stays: it is the only channel that puts stderr in front of the human." +impact: fix --- A successful bootstrap install is rendered to the user as a third 'SessionStart:startup hook error', indistinguishable from the two genuine failures above it. hooks/bootstrap.sh's notice() exits 2 deliberately, and its comment gives the sound reason: a SessionStart hook's stdout becomes model context, while only a non-zero exit puts stderr in front of the human. But the harness renders ANY non-zero SessionStart exit as a ' hook error' notice — the docs confirm exit 2 'renders in the transcript as a hook error notice, the same way a non-blocking error does'. So there is no 'notice' channel distinct from 'error', and the checksum-verified happy path is labelled a fault. Observed on the first manual install (2026-08-10): three consecutive hook-error lines, of which the third was the install SUCCEEDING. Fix direction: fold this into the single chained SessionStart entry proposed in iss-204 and lead the visible first line with the success, since the transcript shows only the first line of stderr; the honest-failure posture for the genuinely-no-binary window is unaffected. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e0e17a4..4ed75fd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,29 @@ called out in a **Breaking** section. ### Fixed +- **The session-start hooks run after the bootstrap that provisions their binary, + and a successful install reads as success** (iss-204, iss-208). The hook + manifest listed the bootstrap and the two binary-backed commands as three + sibling `SessionStart` entries and relied on list order; the harness runs every + hook matching an event in parallel, so both gated entries raced a ~10.7 MB + download, lost, printed "the plugin binary is not installed", and genuinely did + not run — on every fresh install and every plugin update, since an update lands + in a fresh cache directory with no binary. The three entries are now ONE + command that runs the bootstrap and then both binary calls in a single shell, + so the sequencing is owned by the manifest rather than assumed of the harness. + The bootstrap's own message is emitted first, which is what the transcript + renders: on a fresh install the visible line is the checksum-verified success + rather than one of two missing-binary complaints, and the two complaints + collapse into one. The honest-failure posture is unchanged — a refusal keeps + its message and its exit code, a binary that is genuinely absent is still said + out loud, and the binary calls' stdout still reaches the model untouched. The + spec that shipped the bootstrap carried the false warrant ("ordering within one + event's hook list is preserved by the harness") as a load-bearing claim; it is + corrected in place, with the brief's two descriptions of the manifest. Parallel + hook execution and the plugin cache are not present in CI, so the end-to-end + proof is the manual install gate; what CI holds is the manifest's shape and the + chained command's behaviour against fixtures. + - **The build plumbing's own comments describe the gate suite that runs** (iss-182). The `Makefile` preflight comment claimed the target ran "the same steps CI's check job runs" and named only the reviews-charter gate, though the diff --git a/hooks/hooks.json b/hooks/hooks.json index 59e1a531..2756ead6 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -5,9 +5,7 @@ ], "SessionStart": [ {"hooks": [ - {"type": "command", "timeout": 240, "command": "\"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\""}, - {"type": "command", "command": "if [ -f \"$CLAUDE_PLUGIN_ROOT/abcd\" ] && [ -x \"$CLAUDE_PLUGIN_ROOT/abcd\" ]; then \"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset; else echo \"abcd: the plugin binary is not installed, so the rule loader did not reset for this session — hooks/bootstrap.sh provisions it at session start; start a session with network access, or install per https://github.com/REPPL/abcd-cli#install\" >&2; exit 2; fi"}, - {"type": "command", "command": "if [ -f \"$CLAUDE_PLUGIN_ROOT/abcd\" ] && [ -x \"$CLAUDE_PLUGIN_ROOT/abcd\" ]; then \"$CLAUDE_PLUGIN_ROOT/abcd\" hook session-start; else echo \"abcd: the plugin binary is not installed, so the session-start checks did not run — hooks/bootstrap.sh provisions it at session start; start a session with network access, or install per https://github.com/REPPL/abcd-cli#install\" >&2; exit 2; fi"} + {"type": "command", "timeout": 240, "command": "[ -n \"${CLAUDE_PLUGIN_ROOT:-}\" ] || exit 0; b=''; s=0; if [ -x \"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\" ]; then b=$(\"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\" 2>&1 >/dev/null); s=$?; fi; [ -z \"$b\" ] || printf '%s\\n' \"$b\" >&2; if [ -f \"$CLAUDE_PLUGIN_ROOT/abcd\" ] && [ -x \"$CLAUDE_PLUGIN_ROOT/abcd\" ]; then \"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset; p=$?; \"$CLAUDE_PLUGIN_ROOT/abcd\" hook session-start; e=$?; [ \"$s\" -eq 0 ] || exit \"$s\"; [ \"$e\" -eq 0 ] || exit \"$e\"; exit \"$p\"; fi; [ -n \"$b\" ] || printf '%s\\n' \"abcd: the plugin binary is not installed, so the rule loader did not reset and the session-start checks did not run — hooks/bootstrap.sh provisions it at session start; start a session with network access, or install per https://github.com/REPPL/abcd-cli#install\" >&2; [ \"$s\" -eq 0 ] || exit \"$s\"; exit 2"} ]} ], "PreToolUse": [ diff --git a/internal/surface/cli/hooks_sessionstart_test.go b/internal/surface/cli/hooks_sessionstart_test.go new file mode 100644 index 00000000..a9c82ffb --- /dev/null +++ b/internal/surface/cli/hooks_sessionstart_test.go @@ -0,0 +1,300 @@ +package cli + +import ( + "bytes" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// The SessionStart wiring these tests pin exists because the harness runs every +// hook matching an event IN PARALLEL (iss-204). The previous manifest listed the +// bootstrap and the two binary-backed commands as three sibling entries of one +// event and relied on list order; spc-21 asserted that ordering was preserved, +// which is false. Both gated entries lost the race against a ~10.7 MB download, +// printed "the plugin binary is not installed", and genuinely did not run — on +// every fresh install and every plugin update, since an update lands in a fresh +// cache directory with no binary. +// +// So sequencing is owned by ONE command string: bootstrap first, then the two +// binary calls in the same shell. These tests assert that shape and the +// behaviour that only the collapsed form can have. + +// sessionStartHooks is the decoded shape of the events these tests read. Only +// what is asserted is modelled; anything else in the manifest is ignored. +type sessionStartHooks struct { + Hooks map[string][]struct { + Hooks []struct { + Type string `json:"type"` + Timeout int `json:"timeout"` + Command string `json:"command"` + } `json:"hooks"` + } `json:"hooks"` +} + +// hooksManifest locates the committed hooks/hooks.json from this test file's own +// on-disk position, the same way bootstrapScript locates the shipped script — so +// the assertions below derive from the manifest that actually ships. +func hooksManifest(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed to locate the test source file") + } + return filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..", "..", "hooks", "hooks.json")) +} + +// sessionStartCommand returns the single SessionStart command string, failing if +// the event carries anything other than exactly one entry holding exactly one +// command. The count IS the fix: a second sibling entry is a hook the harness +// would run in parallel with the bootstrap again. +func sessionStartCommand(t *testing.T) string { + t.Helper() + data, err := os.ReadFile(hooksManifest(t)) + if err != nil { + t.Fatalf("reading the committed hooks manifest: %v", err) + } + var doc sessionStartHooks + if err := json.Unmarshal(data, &doc); err != nil { + t.Fatalf("hooks/hooks.json does not parse: %v", err) + } + entries := doc.Hooks["SessionStart"] + if len(entries) != 1 { + t.Fatalf("SessionStart must declare exactly one entry group (the harness runs matching hooks in parallel), found %d", len(entries)) + } + if len(entries[0].Hooks) != 1 { + t.Fatalf("the SessionStart entry group must hold exactly one command (siblings race each other), found %d", len(entries[0].Hooks)) + } + return entries[0].Hooks[0].Command +} + +// sessionStartRun executes the shipped SessionStart command under `sh -c` with a +// constructed environment, returning stdout, stderr and the exit code SEPARATELY +// — the split is the point. A SessionStart hook's stdout becomes model context +// while only a non-zero exit puts its stderr in front of the human, and the +// transcript shows only the FIRST line of that stderr (iss-208). +func sessionStartRun(t *testing.T, root string, extraEnv ...string) (string, string, int) { + t.Helper() + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh unavailable") + } + cmd := exec.Command("sh", "-c", sessionStartCommand(t)) + cmd.Env = append([]string{ + "PATH=" + os.Getenv("PATH"), + "HOME=" + t.TempDir(), + "CLAUDE_PLUGIN_ROOT=" + root, + }, extraEnv...) + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + code := 0 + if err := cmd.Run(); err != nil { + e, ok := err.(*exec.ExitError) + if !ok { + t.Fatalf("running the SessionStart command: %v (stderr %s)", err, stderr.String()) + } + code = e.ExitCode() + } + return stdout.String(), stderr.String(), code +} + +// sessionStartRoot builds a plugin root holding a stub hooks/bootstrap.sh, and +// (when body is non-empty) a stub abcd that appends every invocation to a call +// log. The stubs stand in for the real script and binary so the WIRING is what +// is under test; hooks/bootstrap.sh's own behaviour is covered by bootstrap_test. +func sessionStartRoot(t *testing.T, bootstrap, binary string) (root, calls string) { + t.Helper() + root = t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "hooks"), 0o755); err != nil { + t.Fatal(err) + } + calls = filepath.Join(root, "calls.log") + if err := os.WriteFile(filepath.Join(root, "hooks", "bootstrap.sh"), []byte(bootstrap), 0o755); err != nil { + t.Fatal(err) + } + if binary != "" { + if err := os.WriteFile(filepath.Join(root, "abcd"), []byte(binary), 0o755); err != nil { + t.Fatal(err) + } + } + return root, calls +} + +// stubBinary records each invocation, writes to stdout (the model-context +// channel) and returns 2 from `hook session-start`, which is what the real +// binary does when it has a notice to report. +const stubBinary = `#!/bin/sh +printf '%s %s\n' "$1" "$2" >> "$ABCD_CALLS" +printf 'stdout from %s\n' "$2" +if [ "$2" = "session-start" ]; then + printf 'abcd: a session-start notice\n' >&2 + exit 2 +fi +exit 0 +` + +// callLog returns the recorded invocations, or "" when nothing ran. +func callLog(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return "" + } + t.Fatal(err) + } + return string(data) +} + +// firstLine is what the transcript renders of a hook's stderr. +func firstLine(s string) string { + s = strings.TrimLeft(s, "\n") + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +} + +// TestSessionStartIsOneChainedCommand pins the collapse itself: one entry, one +// command, naming the bootstrap and both binary calls, and parsing as POSIX sh. +func TestSessionStartIsOneChainedCommand(t *testing.T) { + command := sessionStartCommand(t) + for _, want := range []string{"hooks/bootstrap.sh", "hook prompt-router-reset", "hook session-start"} { + if !strings.Contains(command, want) { + t.Errorf("the single SessionStart command must run %q; command = %q", want, command) + } + } + if strings.Index(command, "hooks/bootstrap.sh") > strings.Index(command, "hook prompt-router-reset") { + t.Error("the bootstrap must be invoked before the binary-backed calls in the chained command") + } + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh unavailable") + } + script := filepath.Join(t.TempDir(), "session-start.sh") + if err := os.WriteFile(script, []byte(command+"\n"), 0o644); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("sh", "-n", script).CombinedOutput(); err != nil { + t.Fatalf("the SessionStart command is not valid POSIX sh: %v (%s)", err, out) + } +} + +// TestSessionStartLeadsWithTheBootstrapSuccess is iss-208's fix and iss-204's +// together: on a fresh install the bootstrap's success is the FIRST stderr line +// (the only one the transcript renders), and both binary-backed calls still run +// — against the binary the bootstrap has just installed. +func TestSessionStartLeadsWithTheBootstrapSuccess(t *testing.T) { + root, calls := sessionStartRoot(t, `#!/bin/sh +cp "$CLAUDE_PLUGIN_ROOT/staged-abcd" "$CLAUDE_PLUGIN_ROOT/abcd" +chmod 0755 "$CLAUDE_PLUGIN_ROOT/abcd" +printf 'abcd bootstrap: installed the checksum-verified abcd binary\n' >&2 +exit 2 +`, "") + // Staged beside the root rather than written by the stub: the binary the + // bootstrap installs is what the chained calls must then find. + if err := os.WriteFile(filepath.Join(root, "staged-abcd"), []byte(stubBinary), 0o755); err != nil { + t.Fatal(err) + } + stdout, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + + if got := firstLine(stderr); !strings.HasPrefix(got, "abcd bootstrap: installed") { + t.Errorf("the first stderr line must be the bootstrap's success (it is the only line the transcript shows); got %q\nfull stderr:\n%s", got, stderr) + } + if code == 0 { + t.Error("the exit must stay non-zero: only a non-zero SessionStart exit puts stderr in front of the human") + } + if strings.Contains(stderr, "the plugin binary is not installed") { + t.Errorf("a successful bootstrap must not be followed by a missing-binary complaint; stderr:\n%s", stderr) + } + log := callLog(t, calls) + for _, want := range []string{"hook prompt-router-reset", "hook session-start"} { + if !strings.Contains(log, want) { + t.Errorf("%q must run after the bootstrap in the same shell; call log = %q", want, log) + } + } + if !strings.Contains(stdout, "stdout from prompt-router-reset") || !strings.Contains(stdout, "stdout from session-start") { + t.Errorf("the binary calls' stdout must pass through untouched (it is the model-context channel); stdout = %q", stdout) + } +} + +// TestSessionStartSteadyStateRunsBothCalls is the boring session: the binary is +// already there, the bootstrap takes its fast path and says nothing, and both +// calls run with the binary's own notice leading. +func TestSessionStartSteadyStateRunsBothCalls(t *testing.T) { + root, calls := sessionStartRoot(t, "#!/bin/sh\nexit 0\n", stubBinary) + _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + + log := callLog(t, calls) + if !strings.Contains(log, "hook prompt-router-reset") || !strings.Contains(log, "hook session-start") { + t.Errorf("both binary calls must run in the steady state; call log = %q", log) + } + if got := firstLine(stderr); got != "abcd: a session-start notice" { + t.Errorf("the binary's own notice must reach the human unchanged; first line = %q", got) + } + if code != 2 { + t.Errorf("the binary's notice exit must propagate; code = %d", code) + } +} + +// TestSessionStartReportsAMissingBinaryOnce is the genuinely-no-binary window: +// the honest failure is unchanged, but it is now said ONCE rather than by two +// sibling hooks that raced the download. +func TestSessionStartReportsAMissingBinaryOnce(t *testing.T) { + root, calls := sessionStartRoot(t, "#!/bin/sh\nexit 0\n", "") + _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + + if n := strings.Count(stderr, "the plugin binary is not installed"); n != 1 { + t.Errorf("the missing binary must be reported exactly once, got %d; stderr:\n%s", n, stderr) + } + if code == 0 { + t.Error("a missing binary must exit non-zero so the message reaches the human") + } + if log := callLog(t, calls); log != "" { + t.Errorf("no binary call can have run with no binary; call log = %q", log) + } +} + +// TestSessionStartKeepsTheBootstrapRefusalFirst: when the bootstrap refuses it +// has already said what is missing and the three ways out. That message leads, +// its exit code propagates, and the wrapper adds no second complaint on top of +// it — the first line of a refusal is the one the transcript shows. +func TestSessionStartKeepsTheBootstrapRefusalFirst(t *testing.T) { + root, calls := sessionStartRoot(t, `#!/bin/sh +printf 'abcd bootstrap: the latest release tag could not be resolved\n\nThe abcd binary is not installed in the plugin root.\n' >&2 +exit 1 +`, "") + _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + + if got := firstLine(stderr); got != "abcd bootstrap: the latest release tag could not be resolved" { + t.Errorf("the bootstrap's refusal must lead; first line = %q", got) + } + if code != 1 { + t.Errorf("the bootstrap's refusal exit must propagate; code = %d", code) + } + if strings.Contains(stderr, "the plugin binary is not installed, so") { + t.Errorf("a refusal already names the missing binary; the wrapper must not repeat it. stderr:\n%s", stderr) + } +} + +// TestSessionStartWithoutAPluginRootDoesNothing: the manifest is only ever run +// by the harness, but a command that dereferences an unset CLAUDE_PLUGIN_ROOT +// would reach for /hooks/bootstrap.sh and report a raw shell error. +func TestSessionStartWithoutAPluginRootDoesNothing(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh unavailable") + } + cmd := exec.Command("sh", "-c", sessionStartCommand(t)) + cmd.Env = []string{"PATH=" + os.Getenv("PATH"), "HOME=" + t.TempDir()} + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + t.Errorf("an unset CLAUDE_PLUGIN_ROOT must exit 0 and change nothing: %v (stderr %s)", err, stderr.String()) + } + if stderr.Len() != 0 { + t.Errorf("an unset CLAUDE_PLUGIN_ROOT must say nothing; stderr = %q", stderr.String()) + } +} From ad50cd15a096915a97046fb1596a6b68a3665c4a Mon Sep 17 00:00:00 2001 From: REPPL Date: Wed, 12 Aug 2026 12:56:23 +0200 Subject: [PATCH 2/2] fix: give each chained hook call its own payload, and let session-start lead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the collapsed SessionStart command, both found by adversarial review and both reproduced here with the built binary before being fixed. Shared stdin (blocker). Every hook verb takes its input with io.ReadAll over the whole of stdin (readHookInput, cli.go:792), so the two chained calls sharing one stdin left the second reading EOF: json.Unmarshal failed and `hook session-start` took its silent return-nil path (cli.go:979), permanently disabling both of its notices — the iss-95 "transcripts will not be captured" warning and the itd-105 skew notice — in every session, fresh install and steady state alike. Collapsing the entries is what created this; three sibling hooks each got their own stdin. The wrapper now reads the payload once (`i=$(cat)`) and pipes a copy to each call. The bootstrap is additionally given `&1 >/dev/null); s=$?; fi; [ -z \"$b\" ] || printf '%s\\n' \"$b\" >&2; if [ -f \"$CLAUDE_PLUGIN_ROOT/abcd\" ] && [ -x \"$CLAUDE_PLUGIN_ROOT/abcd\" ]; then \"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset; p=$?; \"$CLAUDE_PLUGIN_ROOT/abcd\" hook session-start; e=$?; [ \"$s\" -eq 0 ] || exit \"$s\"; [ \"$e\" -eq 0 ] || exit \"$e\"; exit \"$p\"; fi; [ -n \"$b\" ] || printf '%s\\n' \"abcd: the plugin binary is not installed, so the rule loader did not reset and the session-start checks did not run — hooks/bootstrap.sh provisions it at session start; start a session with network access, or install per https://github.com/REPPL/abcd-cli#install\" >&2; [ \"$s\" -eq 0 ] || exit \"$s\"; exit 2"} + {"type": "command", "timeout": 240, "command": "[ -n \"${CLAUDE_PLUGIN_ROOT:-}\" ] || exit 0; b=''; s=0; if [ -x \"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\" ]; then b=$(\"$CLAUDE_PLUGIN_ROOT/hooks/bootstrap.sh\" 2>&1 >/dev/null &2; if [ -f \"$CLAUDE_PLUGIN_ROOT/abcd\" ] && [ -x \"$CLAUDE_PLUGIN_ROOT/abcd\" ]; then i=$(cat); printf '%s' \"$i\" | \"$CLAUDE_PLUGIN_ROOT/abcd\" hook session-start; e=$?; printf '%s' \"$i\" | \"$CLAUDE_PLUGIN_ROOT/abcd\" hook prompt-router-reset; p=$?; [ \"$s\" -eq 0 ] || exit \"$s\"; [ \"$e\" -eq 0 ] || exit \"$e\"; exit \"$p\"; fi; [ -n \"$b\" ] || printf '%s\\n' \"abcd: the plugin binary is not installed, so the rule loader did not reset and the session-start checks did not run — hooks/bootstrap.sh provisions it at session start; start a session with network access, or install per https://github.com/REPPL/abcd-cli#install\" >&2; [ \"$s\" -eq 0 ] || exit \"$s\"; exit 2"} ]} ], "PreToolUse": [ diff --git a/internal/surface/cli/hooks_sessionstart_test.go b/internal/surface/cli/hooks_sessionstart_test.go index a9c82ffb..9ba19fa7 100644 --- a/internal/surface/cli/hooks_sessionstart_test.go +++ b/internal/surface/cli/hooks_sessionstart_test.go @@ -73,16 +73,20 @@ func sessionStartCommand(t *testing.T) string { } // sessionStartRun executes the shipped SessionStart command under `sh -c` with a -// constructed environment, returning stdout, stderr and the exit code SEPARATELY -// — the split is the point. A SessionStart hook's stdout becomes model context -// while only a non-zero exit puts its stderr in front of the human, and the -// transcript shows only the FIRST line of that stderr (iss-208). -func sessionStartRun(t *testing.T, root string, extraEnv ...string) (string, string, int) { +// constructed environment and stdin, returning stdout, stderr and the exit code +// SEPARATELY — the split is the point. A SessionStart hook's stdout becomes model +// context while only a non-zero exit puts its stderr in front of the human, and +// the transcript shows only the FIRST line of that stderr (iss-208). +// +// stdin is never left nil: the harness delivers the hook payload there, and a +// command that reads it must be exercised with it. +func sessionStartRun(t *testing.T, root, stdin string, extraEnv ...string) (string, string, int) { t.Helper() if _, err := exec.LookPath("sh"); err != nil { t.Skip("sh unavailable") } cmd := exec.Command("sh", "-c", sessionStartCommand(t)) + cmd.Stdin = strings.NewReader(stdin) cmd.Env = append([]string{ "PATH=" + os.Getenv("PATH"), "HOME=" + t.TempDir(), @@ -124,19 +128,31 @@ func sessionStartRoot(t *testing.T, bootstrap, binary string) (root, calls strin return root, calls } -// stubBinary records each invocation, writes to stdout (the model-context -// channel) and returns 2 from `hook session-start`, which is what the real -// binary does when it has a notice to report. +// stubBinary stands in for the real binary, and it models the three things +// about it the chained command has to respect: +// +// - every hook verb CONSUMES stdin whole (readHookInput is io.ReadAll over a +// LimitReader), so two calls sharing one stdin leave the second reading EOF; +// - `hook prompt-router-reset` writes an UNCONDITIONAL success diagnostic to +// stderr, so whichever call runs first owns the line the transcript renders; +// - `hook session-start` exits 2 when it has a notice, which is the only way +// its warning reaches the human. const stubBinary = `#!/bin/sh -printf '%s %s\n' "$1" "$2" >> "$ABCD_CALLS" +in=$(cat) +printf '%s %s stdin=[%s]\n' "$1" "$2" "$in" >> "$ABCD_CALLS" printf 'stdout from %s\n' "$2" -if [ "$2" = "session-start" ]; then - printf 'abcd: a session-start notice\n' >&2 - exit 2 +if [ "$2" = "prompt-router-reset" ]; then + printf 'abcd rules: reset session ("SessionStart")\n' >&2 + exit 0 fi -exit 0 +printf 'abcd: a session-start notice\n' >&2 +exit 2 ` +// sessionStartPayload is a harness SessionStart payload of the shape +// readHookInput unmarshals. Every chained call must receive it in full. +const sessionStartPayload = `{"session_id":"s1","hook_event_name":"SessionStart","source":"startup","cwd":"/tmp"}` + // callLog returns the recorded invocations, or "" when nothing ran. func callLog(t *testing.T, path string) string { t.Helper() @@ -199,7 +215,7 @@ exit 2 if err := os.WriteFile(filepath.Join(root, "staged-abcd"), []byte(stubBinary), 0o755); err != nil { t.Fatal(err) } - stdout, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + stdout, stderr, code := sessionStartRun(t, root, sessionStartPayload, "ABCD_CALLS="+calls) if got := firstLine(stderr); !strings.HasPrefix(got, "abcd bootstrap: installed") { t.Errorf("the first stderr line must be the bootstrap's success (it is the only line the transcript shows); got %q\nfull stderr:\n%s", got, stderr) @@ -223,29 +239,56 @@ exit 2 // TestSessionStartSteadyStateRunsBothCalls is the boring session: the binary is // already there, the bootstrap takes its fast path and says nothing, and both -// calls run with the binary's own notice leading. +// calls run. +// +// The first-line assertion is the one with teeth. `hook prompt-router-reset` +// ends by writing an UNCONDITIONAL "abcd rules: reset session" diagnostic to +// stderr (cli.go), so if it runs first it owns the only line the transcript +// renders and `hook session-start`'s actionable notice — "transcripts will not +// be captured", the version-skew line — is never seen. The chain therefore runs +// session-start FIRST; the exit precedence is computed from the saved codes and +// does not depend on the order. func TestSessionStartSteadyStateRunsBothCalls(t *testing.T) { root, calls := sessionStartRoot(t, "#!/bin/sh\nexit 0\n", stubBinary) - _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + _, stderr, code := sessionStartRun(t, root, sessionStartPayload, "ABCD_CALLS="+calls) log := callLog(t, calls) if !strings.Contains(log, "hook prompt-router-reset") || !strings.Contains(log, "hook session-start") { t.Errorf("both binary calls must run in the steady state; call log = %q", log) } if got := firstLine(stderr); got != "abcd: a session-start notice" { - t.Errorf("the binary's own notice must reach the human unchanged; first line = %q", got) + t.Errorf("session-start's notice must be the line the transcript renders, not the reset's success diagnostic; first line = %q\nfull stderr:\n%s", got, stderr) } if code != 2 { t.Errorf("the binary's notice exit must propagate; code = %d", code) } } +// TestSessionStartFeedsThePayloadToEveryCall is the defect a shared stdin +// creates. Every hook verb reads its payload with io.ReadAll over the whole of +// stdin (readHookInput), so two calls chained on ONE stdin leave the second +// reading EOF: json.Unmarshal fails and `hook session-start` takes its silent +// return-nil path, permanently disabling both of its notices in every session. +// The wrapper therefore reads the payload once and feeds each call a copy. +func TestSessionStartFeedsThePayloadToEveryCall(t *testing.T) { + root, calls := sessionStartRoot(t, "#!/bin/sh\nexit 0\n", stubBinary) + sessionStartRun(t, root, sessionStartPayload, "ABCD_CALLS="+calls) + + log := callLog(t, calls) + for _, verb := range []string{"prompt-router-reset", "session-start"} { + want := "hook " + verb + " stdin=[" + sessionStartPayload + "]" + if !strings.Contains(log, want) { + t.Errorf("`hook %s` must receive the whole payload, not what a previous call left; call log = %q", verb, log) + } + } +} + // TestSessionStartReportsAMissingBinaryOnce is the genuinely-no-binary window: // the honest failure is unchanged, but it is now said ONCE rather than by two // sibling hooks that raced the download. func TestSessionStartReportsAMissingBinaryOnce(t *testing.T) { root, calls := sessionStartRoot(t, "#!/bin/sh\nexit 0\n", "") - _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + _, stderr, code := sessionStartRun(t, root, sessionStartPayload, "ABCD_CALLS="+calls) if n := strings.Count(stderr, "the plugin binary is not installed"); n != 1 { t.Errorf("the missing binary must be reported exactly once, got %d; stderr:\n%s", n, stderr) @@ -267,7 +310,7 @@ func TestSessionStartKeepsTheBootstrapRefusalFirst(t *testing.T) { printf 'abcd bootstrap: the latest release tag could not be resolved\n\nThe abcd binary is not installed in the plugin root.\n' >&2 exit 1 `, "") - _, stderr, code := sessionStartRun(t, root, "ABCD_CALLS="+calls) + _, stderr, code := sessionStartRun(t, root, sessionStartPayload, "ABCD_CALLS="+calls) if got := firstLine(stderr); got != "abcd bootstrap: the latest release tag could not be resolved" { t.Errorf("the bootstrap's refusal must lead; first line = %q", got) @@ -289,6 +332,7 @@ func TestSessionStartWithoutAPluginRootDoesNothing(t *testing.T) { } cmd := exec.Command("sh", "-c", sessionStartCommand(t)) cmd.Env = []string{"PATH=" + os.Getenv("PATH"), "HOME=" + t.TempDir()} + cmd.Stdin = strings.NewReader(sessionStartPayload) var stderr bytes.Buffer cmd.Stderr = &stderr if err := cmd.Run(); err != nil {