Subprocess e2e tests for the five Claude Code hook handlers - #28
Closed
lazypower wants to merge 2 commits into
Closed
Subprocess e2e tests for the five Claude Code hook handlers#28lazypower wants to merge 2 commits into
lazypower wants to merge 2 commits into
Conversation
PR #27's retract_e2e_test.go inlined ~260 lines of subprocess-test plumbing — binary build, free-port allocation, server process management, CLI invocation, exit-code/stdout/stderr assertions. With a second consumer landing in this stack (hooks subprocess tests), keeping the harness duplicated would be the kind of structural debt that hurts in six months. Move the harness into a new non-test package, internal/testharness: - BuildContinuityBinary(t) — go build -tags noembed into a temp path - FreeTCPPort(t) — net.Listen(":0"); kernel-allocated, race-acceptable - StartServeProcess(t, bin, env) + ServerProcess.Stop()/Stderr() - WaitForReady(t, url) — poll /api/health - WaitForCondition(t, timeout, msg, check) — generic poll-until-true, needed for async server-side state assertions (extraction goroutines, DB writes after fire-and-forget hook POSTs) - CLIResult + RunCLI + RunCLIWithStdin (stdin is what hook tests need) - ExpectExit / ExpectStdoutContains / ExpectStderrContains / ExpectStdoutAbsent / ExpectStderrAbsent — chainable assertions - HermeticEnv(t, workDir, dbPath, port) — canonical env set used by every subprocess test: CONTINUITY_DB / PORT / BIND / EMBEDDER=tfidf, HOME redirected to a tempdir, CONTINUITY_URL pointed at the chosen port. Returns (serverURL, env) so test setup is a one-liner. testharness is a non-test package because Go forbids importing _test files across package boundaries. The cost (importing "testing" from a non-test package) is paid intentionally — it's only ever consumed by test code. Refactor PR #27's retract_e2e_test.go to use the new package. Test logic is unchanged; the file shrinks from 471 lines to 217. The retract-specific seedTFIDFCorpus helper stays inline since it's explicitly about the retract scenario's TFIDF vocabulary needs, not a general harness concern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hooks are the primary integration point with Claude Code (continuity hook start | submit | tool | stop | end) and their contract is exactly what subprocess testing is built for: agent-visible exit codes, strict stdout/stderr channel separation, exact text Claude parses on stdout for SessionStart context injection. PR #4 fixed a silent-extraction lock-out bug at this layer; that bug was caught by manual review, not by tests. This change closes that visibility gap. Eleven tests in internal/hooks/hooks_e2e_test.go, all using the shared testharness package (renamed setup helpers; one new hook-specific helper for piping JSON to the binary's stdin): SessionStart: - WritesContextJSON — stdout MUST be valid SessionStartOutput JSON with hookEventName="SessionStart" (Claude parses this to inject the additionalContext payload). Exit 0. - ServerDownDegradesGracefully — server killed before the hook fires; stdout MUST still be valid JSON (empty context). Exit 0. Crashing or returning malformed JSON here would break every Claude session that fires while the server is restarting. UserPromptSubmit: - CreatesSession — POST /api/sessions/init lands; DB sessions row present after the subprocess returns. - InternalSentinelSkipsInit — prompts starting with "[continuity-internal]" MUST NOT create a session. The anti- recursion guard prevents the server's own claude -p extraction calls from spawning sessions that fire more extractions. - SignalTriggerReachesServer — "remember this" + friends trigger a fire-and-forget POST /api/sessions/<id>/signal. In CI there is no LLM, so the server's ExtractSignal call fails — the failure log is our proof the route was reached. PostToolUse: - RecordsObservation — happy path; observation count >= 1. - SkipsMetaTools — TodoRead, TodoWrite, Thinking, TaskList, TaskCreate, TaskGet, TaskUpdate MUST NOT produce observations. These are meta-noise that would crowd out real tool signals during extraction. Stop: - LowMessageSkipsExtractCall — the client-side gate in stop.go MUST skip the /extract POST entirely when the transcript has <3 user messages, so the server never sees a per-turn round-trip on early turns. Asserted via absence of any "extraction:" log line and extracted_at remaining NULL. SessionEnd (the load-bearing PR #4 regression): - PR4Invariant_LowContentDoesNotMark — End ALWAYS POSTs /extract (belt-and-suspenders). With a low-content transcript the server- side gate must short-circuit BEFORE MarkExtracted. If the gate accidentally ran after the mark (the original bug), extracted_at would get set despite no extraction happening, and a later End with real content would idempotency-skip silently. Test pins: - Server stderr shows "extraction: skipping <id> ... (not marking)" - No "extraction failed for <id>" (we never reached the LLM path) - DB extracted_at remains NULL - PastThresholdReachesExtractor — counterpart positive assertion: >=3 user messages AND >=100 chars condensed MUST let extraction reach the LLM call. Without this, a regression flipping the gate to always-skip would pass the PR-4 invariant test silently. Lifecycle: - FullSession — Start → 3x Submit → 2x Tool → Stop → End walked against one hermetic DB. Cumulative state pinned: session present, observations >=2, extraction was attempted. Sanity-checked during development: inverted the PR-4 extracted_at assertion (set sess.ExtractedAt == nil as the failure trigger), saw the test catch it with the right error, restored. The contract assertion is real. Runtime: ~12s for all 11 tests on a warm cache (each builds the binary on first call via t.TempDir; the build is shared across tests of the same setup helper but each test spins its own setupHookE2E and thus its own binary — straightforward to share later if needed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 13, 2026
Copilot stopped reviewing on behalf of
lazypower due to an error
June 13, 2026 05:39
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a shared subprocess E2E test harness and expands end-to-end coverage for hooks, while refactoring existing CLI E2E tests to reuse the shared harness.
Changes:
- Introduce
internal/testharnessfor building the binary, starting/stoppingcontinuity serve, and running CLI subprocesses. - Add comprehensive hooks subprocess E2E tests covering start/submit/tool/stop/end lifecycle contracts.
- Refactor retract subprocess E2E test to use the shared harness utilities.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| internal/testharness/harness.go | New reusable helpers for subprocess E2E tests (build/run server, env wiring, CLI runner). |
| internal/hooks/hooks_e2e_test.go | New hooks-focused subprocess E2E suite validating key hook/server contracts. |
| internal/cli/retract_e2e_test.go | Replaces local helper implementations with internal/testharness usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+108
to
+116
| cmd := exec.Command(bin, "serve") | ||
| cmd.Env = env | ||
|
|
||
| stderr := &bytes.Buffer{} | ||
| cmd.Stderr = stderr | ||
| cmd.Stdout = io.Discard | ||
|
|
||
| cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | ||
|
|
Comment on lines
+129
to
+146
| func (s *ServerProcess) Stop() { | ||
| if s.stopped { | ||
| return | ||
| } | ||
| s.stopped = true | ||
|
|
||
| if s.cmd.Process != nil { | ||
| _ = syscall.Kill(-s.cmd.Process.Pid, syscall.SIGTERM) | ||
| } | ||
|
|
||
| done := make(chan error, 1) | ||
| go func() { done <- s.cmd.Wait() }() | ||
| select { | ||
| case <-done: | ||
| case <-time.After(5 * time.Second): | ||
| if s.cmd.Process != nil { | ||
| _ = syscall.Kill(-s.cmd.Process.Pid, syscall.SIGKILL) | ||
| } |
Comment on lines
+94
to
+114
| // ServerProcess wraps a running `continuity serve` child process. Stderr is | ||
| // captured to a buffer so tests can inspect server-side log lines (the server | ||
| // uses Go's log package which writes to stderr). | ||
| type ServerProcess struct { | ||
| cmd *exec.Cmd | ||
| stderrBuf *bytes.Buffer | ||
| stopped bool | ||
| } | ||
|
|
||
| // StartServeProcess spawns `continuity serve` with the given env. Callers | ||
| // MUST call WaitForReady before issuing CLI commands and SHOULD register | ||
| // Stop with t.Cleanup. | ||
| func StartServeProcess(t *testing.T, bin string, env []string) *ServerProcess { | ||
| t.Helper() | ||
| cmd := exec.Command(bin, "serve") | ||
| cmd.Env = env | ||
|
|
||
| stderr := &bytes.Buffer{} | ||
| cmd.Stderr = stderr | ||
| cmd.Stdout = io.Discard | ||
|
|
Comment on lines
+123
to
+125
| // Stderr returns the server's accumulated stderr. Safe to call repeatedly; | ||
| // reflects whatever the server has logged up to the moment of the call. | ||
| func (s *ServerProcess) Stderr() string { return s.stderrBuf.String() } |
Comment on lines
+209
to
+213
| cmd := exec.CommandContext(ctx, bin, args...) | ||
| cmd.Env = env | ||
| if stdin != "" { | ||
| cmd.Stdin = strings.NewReader(stdin) | ||
| } |
Comment on lines
+95
to
+108
| line, _ := json.Marshal(userMsg) | ||
| b.Write(line) | ||
| b.WriteString("\n") | ||
|
|
||
| asstMsg := map[string]any{ | ||
| "type": "assistant", | ||
| "message": map[string]any{ | ||
| "role": "assistant", | ||
| "content": fmt.Sprintf("%s (turn %d)", assistantText, i+1), | ||
| }, | ||
| } | ||
| line, _ = json.Marshal(asstMsg) | ||
| b.Write(line) | ||
| b.WriteString("\n") |
Comment on lines
+224
to
+233
| // Wait long enough that an init would have landed if it were going to, | ||
| // then assert it didn't. | ||
| time.Sleep(200 * time.Millisecond) | ||
| sess, err := h.db.GetSession(sessionID) | ||
| if err != nil { | ||
| t.Fatalf("GetSession: %v", err) | ||
| } | ||
| if sess != nil { | ||
| t.Errorf("internal-sentinel prompt must NOT create session %q (recursion guard); got %+v", sessionID, sess) | ||
| } |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on top of #27 — follow-up to #21. Closes the highest-leverage gap from #27's planning doc: subprocess tests for the five Claude Code hook handlers (
continuity hook start | submit | tool | stop | end), plus a PR #4 regression invariant that would have caught the silent-extraction-lock-out bug at the test layer instead of via manual review.This is item ① in the testing-hardening roadmap. Picked by the rubric:
Ships
Commit 1 —
internal/testharness(harness extraction)PR #27 inlined the subprocess plumbing in
retract_e2e_test.go. With this PR's second consumer, that inlining becomes structural debt. Extracted to a non-test package so any test can import:BuildContinuityBinary(t)go build -tags noembedinto a tempdirFreeTCPPort(t)StartServeProcess(t, bin, env)+ServerProcess.Stop()/.Stderr()WaitForReady(t, url)/api/healthWaitForCondition(t, timeout, msg, check)CLIResult+RunCLI/RunCLIWithStdinExpectExit/ExpectStdoutContains/ExpectStderrContains/ExpectStdoutAbsent/ExpectStderrAbsentHermeticEnv(t, workDir, dbPath, port)CONTINUITY_DB/PORT/BIND/EMBEDDER=tfidf,HOMEto tempdir,CONTINUITY_URLwiredtestharnessis intentionally a non-test package — Go forbids importing_test.gofiles across boundaries. The"testing"import in a non-test package is paid intentionally; it's only ever consumed by test code.PR #27's retract test refactors to use the new package: 471 lines → 217 lines, logic unchanged. The retract-specific
seedTFIDFCorpusstays inline since it's about the retract scenario's TFIDF vocabulary needs, not a general harness concern.Commit 2 — 11 hook subprocess tests
TestHookStart_…_WritesContextJSONSessionStartOutputJSON, exit 0TestHookStart_…_ServerDownDegradesGracefullyTestHookSubmit_…_CreatesSession/api/sessions/initPOST landed; DB row presentTestHookSubmit_…_InternalSentinelSkipsInit[continuity-internal]prompts MUST NOT create a session (anti-recursion)TestHookSubmit_…_SignalTriggerReachesServerremember thisetc. trigger fire-and-forget/signal; server log proves the call landedTestHookTool_…_RecordsObservationTestHookTool_…_SkipsMetaToolsTestHookStop_…_LowMessageSkipsExtractCall/extractentirely on <3-msg transcripts (avoid per-turn round-trip)TestHookEnd_…_PR4Invariant_LowContentDoesNotMark/extract; server-side gate MUST short-circuit beforeMarkExtractedTestHookEnd_…_PastThresholdReachesExtractorTestHookLifecycle_…_FullSessionWhy TFIDF / clean-room CI / hermetic env
Same story as #27: every test uses
CONTINUITY_EMBEDDER=tfidfso the Ollama probe is bypassed entirely. Per-testHOMEand DB land in a tempdir; per-test port is kernel-allocated. Build tag!windowsbecause the SIGTERM-process-group shutdown pattern is Unix-specific.Sanity check
During development I inverted the load-bearing assertion in
TestHookEnd_…_PR4Invariant_LowContentDoesNotMark(changedif sess.ExtractedAt != niltoif sess.ExtractedAt == nil), confirmed the test fails with the right error message ("must NOT mark extracted_at; got NULL"), then restored. The PR-4 contract assertion is real.CI
This PR inherits the
e2ejob that PR #27 added to.github/workflows/ci.yml. The job runs:Both
TestRetract_SubprocessE2E_TFIDF(#27) and all 11TestHook*_SubprocessE2E_*tests in this PR match the filter; no CI changes needed in this PR's diff.Test plan
go test -tags noembed -count=1 ./internal/...clean — all 11 hook tests + retract test + existing in-process tests greengo vet -tags noembed ./...cleango test -tags noembed -run 'E2E|Subprocess' ./internal/...— CI filter dry-run; both packages report tests; the rest "no tests to run"Out of scope / next opportunities
🤖 Generated with Claude Code