diff --git a/CHANGELOG.md b/CHANGELOG.md index b0a94eb..0a58819 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- fix: `pkg/ops` `runDetachedTurn` precedence — a validated turn result now overrides a non-zero child exit, so a clean headless-turn blob is a success even when the child exits non-zero. Parsed-but-rejected output leads with the child's own `result` text (predicate named in parentheses), and the child's exit status is reported only when the output is unparseable or missing. +- test: `pkg/ops` write-back specs prove the spec 045 clear-vs-retain consequence end-to-end through `Execute` on real vault files — the task path retains the pre-persisted session id (and the goal path persists it) when a non-zero-exit turn's result validated, and both paths still leave no session id when the turn reported its own failure, with the child's reason leading the surfaced error. +- fix: `docs/work-on-session-lifecycle.md` contract — the validated result outranks the exit code, so a headless `work-on` turn that completed successfully is no longer discarded because its child process exited non-zero: the session id persists and the Vault UI offers Resume. A genuinely failed turn still clears the id and now reports the child's own reason instead of `exit status 1`. + ## v0.125.1 - docs: `commands/verify-goal.md` rule matrix updated to match the relaxed forward `in_progress` rule (goal `in_progress` → subtask `backlog`/`next`/`in_progress`/`completed` all aligned). diff --git a/docs/work-on-session-lifecycle.md b/docs/work-on-session-lifecycle.md index a6d5274..5b70fc1 100644 --- a/docs/work-on-session-lifecycle.md +++ b/docs/work-on-session-lifecycle.md @@ -78,15 +78,18 @@ file and validates the same blob after the child exits, through the shared Validation is not optional. `claude` reports a `session_id` even for a turn that did no work or failed outright, so an unvalidated id would be handed to the operator as -resumable when it is not — the same class of lie this fix exists to remove. A turn -whose result is `num_turns: 0`, `is_error: true`, or unparseable is an error, and no -id is persisted. +resumable when it is not — the same class of lie this fix exists to remove. The +validation verdict, not the process exit status, is what decides: a turn whose result +is `num_turns: 0`, `is_error: true`, or unparseable is an error, and no id is +persisted. The same shared `validateSessionTurn` still serves both branches. A temp **file** rather than a pipe is deliberate: the child writes to an inherited fd with no reader, so there is no pipe-buffer deadlock and no EPIPE if the parent goes away, and the file is complete once `cmd.Wait()` returns. It is unlinked eagerly, so no path — including cancel and timeout, where the child still holds the fd — leaves -anything behind. Stderr still goes to `os.DevNull`; a crash surfaces via exit code. +anything behind. Stderr still goes to `os.DevNull` — which is precisely why the +exit code carries no diagnostic content and is now the fallback signal rather than +the primary one. ## What the turn timeout does and does not cover @@ -97,11 +100,37 @@ the parent only stops waiting. `--max-turns` is inert (`maxTurns` is -1), so a legitimate agentic chain can run for minutes; 30m is roughly 6-10x the observed turn length, chosen to bound a pathological hang without cutting off normal work. -Expiry, ctx cancellation, and a non-zero child exit all return an error, so the -caller persists nothing and the UI keeps showing **Start** rather than offering a -Resume that cannot work. This is deliberately **not** an inactivity watchdog — a -session that hangs after starting is left to the Vault UI's existing -`claude_session_started` cleanup sweep, which is out of scope here. +Expiry and ctx cancellation still return an error and the caller persists nothing — +those two are unchanged. A non-zero child exit is no longer a failure by itself. +Once the child has exited, the captured result is read and validated: the +**validated result outranks the exit code**: when the blob validates, the turn is +a success and the id persists. + +Why the exit status is the weaker signal by construction: the child's stderr goes to +`os.DevNull`, so a non-zero exit arrives with no accompanying explanation, while the +result blob is a structured document the code already knows how to validate. Trusting +the opaque signal over the structured one was the inversion. The mirror-image lie +matters too: discarding a session that *can* be resumed costs the whole turn, while +the false positive it was guarding against costs one failed `claude --resume`. + +The exit status remains the reason in exactly one case: the output is missing, +unreadable, or not valid turn JSON, so there is no `result` text to surface. When the +blob parses but fails a predicate, the error leads with the child's own `result` text +and names the failed predicate — `claude reported is_error: true`, `claude returned +num_turns: 0`, `claude returned empty session_id`. The compensating clear is +unchanged: it still fires on every error the detached turn returns. Only the +definition of "failed" moved. + +**The read is never hoisted above the `select`.** The read lives inside the +child-exited branch. On the timeout and cancellation paths the child is still running, +so any bytes in the output file are partial by definition and must never be validated +as success. This is a regression lock with a unit test behind it, not a stylistic +preference — a future reader who "simplifies" the read out of its branch reintroduces +the bug in a worse form. + +This is deliberately **not** an inactivity watchdog — a session that hangs after +starting is left to the Vault UI's existing `claude_session_started` cleanup sweep, +which is out of scope here. ## Failure path @@ -166,7 +195,7 @@ non-interactive re-persist path spawns no writer and takes no lock; liveness gat there belongs to the vault-ui follow-on, not to the locker. **The detached-child safety property.** On the spawn path, when the parent stops -waiting — child exit error, ctx cancel, or the 30m bound — the detached child keeps +waiting — a failed turn, ctx cancel, or the 30m bound — the detached child keeps running *without* the parent's lock. On the task path the id is pre-persisted, and the safety argument is layered: during the running window Resume is not offered for a live turn (the Vault UI resolver fix, shipped separately) and the per-session lock (spec diff --git a/pkg/ops/claude_session.go b/pkg/ops/claude_session.go index 502bf77..691ffb4 100644 --- a/pkg/ops/claude_session.go +++ b/pkg/ops/claude_session.go @@ -7,6 +7,7 @@ package ops import ( "context" "encoding/json" + stderrors "errors" "fmt" "log/slog" "os" @@ -31,10 +32,11 @@ type ClaudeSessionStarter interface { // result; they differ in how. The interactive branch runs the child under the // request context (bounded by a 5m timeout). The non-interactive branch spawns // the child detached from the request context and waits for its exit, bounded by - // sessionTurnTimeout — a wait bound, not a kill. Any outcome other than a clean, - // validated turn returns an error, so the caller persists no session id and the - // UI never offers Resume against a live or failed transcript. See - // docs/work-on-session-lifecycle.md. + // sessionTurnTimeout — a wait bound, not a kill. The turn is judged by its + // validated result: a non-zero child exit is not itself a failure when the result + // validates, and the caller persists no session id only when the turn genuinely + // failed. That way the UI never offers Resume against a live or failed transcript. + // See docs/work-on-session-lifecycle.md. StartSession(ctx context.Context, sessionID string, prompt string, cwd string, name string, isInteractive bool) error } @@ -224,8 +226,10 @@ func (c *claudeSessionStarter) StartSession( // runDetachedTurn spawns the child detached and blocks until its headless turn // finishes. Returning early would hand the caller a session id whose transcript is // still being written — the Vault UI would offer Resume against a live, -// single-writer-assumed jsonl and `claude --resume` would fail. So every exit path -// except a clean, validated turn returns an error, and the caller persists nothing. +// single-writer-assumed jsonl and `claude --resume` would fail. The turn is judged +// by its validated result: a non-zero child exit is not itself a failure when the +// result validates, and the caller persists nothing only when the turn genuinely +// failed. func (c *claudeSessionStarter) runDetachedTurn( ctx context.Context, args []string, @@ -257,9 +261,37 @@ func (c *claudeSessionStarter) runDetachedTurn( }() select { case exitErr := <-done: - if exitErr != nil { + // The child has exited, so its fd is closed and the file is complete. + // The read may not be hoisted above this select: on the timeout and + // cancellation paths the child is still running, so any bytes present are + // partial by definition and must never be validated as success. + output, readErr := os.ReadFile(outFile.Name()) + if readErr != nil { + return errors.Wrap(ctx, readErr, "read claude output") + } + validateErr := validateSessionTurn(ctx, output) + if validateErr == nil { + // The validated result is authoritative. A non-zero exit is the only + // signal we are deliberately ignoring here, so log it rather than + // swallow it silently. + if exitErr != nil { + slog.Warn("validated turn result overrides non-zero child exit", "err", exitErr) + } + return nil + } + if exitErr != nil && errors.Is(validateErr, errClaudeOutputUnparseable) { + // No usable result exists: the output did not even parse, so the + // child's exit status is the only reason we can name. return errors.Errorf(ctx, "claude session exited with error: %v", exitErr) } + // The output parsed but failed a predicate. The child's own reason leads the + // message, but a non-zero exit alongside it is still a distinct signal, so log + // it rather than drop it — mirrors the override log above. + if exitErr != nil { + slog.Warn("turn rejected by predicate; child also exited non-zero", "err", exitErr) + } + // Return it unwrapped so the child's own result text leads (see rejectTurn). + return validateErr case err := <-waitCh: // Both outcomes are errors so the caller persists no session id. The child // is detached and keeps running in either case; we only stop waiting on it. @@ -272,15 +304,15 @@ func (c *claudeSessionStarter) runDetachedTurn( c.sessionTurnTimeout, ) } - - // The child has exited, so its fd is closed and the file is complete. - output, err := os.ReadFile(outFile.Name()) - if err != nil { - return errors.Wrap(ctx, err, "read claude output") - } - return validateSessionTurn(ctx, output) } +// errClaudeOutputUnparseable marks a turn result that could not be parsed at all, +// as opposed to one that parsed and then failed a predicate. The distinction is +// load-bearing: a parsed blob carries the child's own `result` text and can explain +// itself, while an unparseable one cannot — so only the unparseable case falls back +// to the child's exit status as the reason. +var errClaudeOutputUnparseable = stderrors.New("claude output is not valid turn JSON") + // validateSessionTurn checks the --output-format json blob a finished headless turn // emits. Shared by both branches: a session id alone proves nothing, because claude // reports one even for a turn that did no work or failed, so an unvalidated id would @@ -293,20 +325,37 @@ func validateSessionTurn(ctx context.Context, output []byte) error { Result string `json:"result"` } if err := json.Unmarshal(output, &result); err != nil { - return errors.Wrap(ctx, err, "parse claude output") + // The sentinel is the wrapped cause on purpose so errors.Is can match it; the + // underlying unmarshal error is flattened into the text rather than chained. + // Do not "fix" this back to errors.Wrap(ctx, err, ...) — that breaks the + // errors.Is check in runDetachedTurn and silently restores the old behaviour + // of reporting a bare exit status for output that actually parsed. + return errors.Wrapf(ctx, errClaudeOutputUnparseable, "parse claude output: %v", err) } if result.SessionID == "" { - return errors.Errorf(ctx, "claude returned empty session_id") + return rejectTurn(ctx, result.Result, "claude returned empty session_id") } if result.NumTurns == 0 { - return errors.Errorf(ctx, "claude returned 0 turns: %s", result.Result) + return rejectTurn(ctx, result.Result, "claude returned num_turns: 0") } if result.IsError { - return errors.Errorf(ctx, "claude reported error: %s", result.Result) + return rejectTurn(ctx, result.Result, "claude reported is_error: true") } return nil } + +// rejectTurn builds the error for a turn result that parsed but failed a predicate. +// The child's own `result` text leads, because it is the only part of the message an +// operator can act on; the predicate name follows in parentheses. When the child +// reported no result text at all there is nothing to lead with, so the predicate +// stands alone. +func rejectTurn(ctx context.Context, resultText string, reason string) error { + if resultText == "" { + return errors.New(ctx, reason) + } + return errors.Errorf(ctx, "%s (%s)", resultText, reason) +} diff --git a/pkg/ops/claude_session_test.go b/pkg/ops/claude_session_test.go index aaa345a..b719788 100644 --- a/pkg/ops/claude_session_test.go +++ b/pkg/ops/claude_session_test.go @@ -132,7 +132,7 @@ var _ = Describe("ClaudeSessionStarter", func() { It("returns error containing 0 turns and result", func() { err := starter.StartSession(ctx, "session-abc", "prompt", "/vault", "", true) Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(ContainSubstring("0 turns")) + Expect(err.Error()).To(ContainSubstring("num_turns")) Expect(err.Error()).To(ContainSubstring("Unknown command: /x")) }) }) @@ -366,7 +366,7 @@ var _ = Describe("ClaudeSessionStarter", func() { ) err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("0 turns")) + Expect(err.Error()).To(ContainSubstring("num_turns")) }) It("validates the turn and rejects an is_error result", func() { @@ -388,7 +388,7 @@ var _ = Describe("ClaudeSessionStarter", func() { ) err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("claude reported error")) + Expect(err.Error()).To(ContainSubstring("claude reported is_error")) }) It("rejects an unparseable turn result", func() { @@ -434,6 +434,141 @@ var _ = Describe("ClaudeSessionStarter", func() { Expect(err.Error()).To(ContainSubstring("exited with error")) }) + It("returns nil when the child writes a valid blob and exits non-zero", func() { + bw := blockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(validTurnJSON) + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-bw + return nil + }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(BeNil()) + }) + + It("leads with the child's reason when a parsed blob reports is_error", func() { + bw := blockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`{"session_id":"session-abc","num_turns":2,"is_error":true,"result":"seeded failure text"}`) + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-bw + return nil + }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(HavePrefix("seeded failure text")) + Expect(err.Error()).To(ContainSubstring("is_error")) + Expect(err.Error()).NotTo(HavePrefix("exit status")) + }) + + It("leads with the child's reason when a parsed blob has zero turns", func() { + bw := blockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`{"session_id":"session-abc","num_turns":0,"is_error":false,"result":"seeded failure text"}`) + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-bw + return nil + }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(HavePrefix("seeded failure text")) + Expect(err.Error()).To(ContainSubstring("num_turns")) + Expect(err.Error()).NotTo(HavePrefix("exit status")) + }) + + It("leads with the child's reason when a parsed blob has an empty session_id", func() { + bw := blockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`{"session_id":"","num_turns":2,"is_error":false,"result":"seeded failure text"}`) + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-bw + return nil + }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(HavePrefix("seeded failure text")) + Expect(err.Error()).To(ContainSubstring("session_id")) + Expect(err.Error()).NotTo(HavePrefix("exit status")) + }) + + It("names the exit status when the output is non-empty but unparseable", func() { + bw := blockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(`not valid json at all`) + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-bw + return nil + }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("exit status")) + }) + + It("fails on timeout even when a valid blob is already on disk", func() { + // Regression lock against hoisting the read above the select: the child + // is still running (its channel never fires), so the bytes already on + // disk are partial by definition and must never be validated as success. + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + _, _ = stdout.WriteString(validTurnJSON) + // Child never exits within the bound. + return make(chan error), nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { return nil }), + locker, + ) + err := starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("did not complete within")) + }) + It("treats the turn timeout as an error so no id is persisted", func() { starter = ops.NewClaudeSessionStarterWithRunner( "/usr/local/bin/claude", @@ -555,7 +690,25 @@ var _ = Describe("ClaudeSessionStarter", func() { }) It("releases the lock when the child exits with an error", func() { - lockDoneCh <- ErrTest + // The child must leave the output file empty for the exit status to be + // authoritative under the result-over-exit-code precedence: a valid blob + // on disk would validate as a success even with a non-zero exit. + waiter := lockWaiter + starter = ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, _ *os.File) (<-chan error, error) { + spawnCount++ + done := make(chan error, 1) + done <- ErrTest + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-waiter + return nil + }), + lockLocker, + ) err := starter.StartSession(ctx, "session-abc", "prompt", "/vault", "", false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("exited with error")) diff --git a/pkg/ops/goal_workon_test.go b/pkg/ops/goal_workon_test.go index d365f4a..5e876fd 100644 --- a/pkg/ops/goal_workon_test.go +++ b/pkg/ops/goal_workon_test.go @@ -9,6 +9,7 @@ import ( stderrors "errors" "os" "path/filepath" + "strings" "time" "github.com/bborbe/errors" @@ -548,6 +549,183 @@ body }) }) + Context("goal work-on retains the session id after a non-zero exit with a valid turn result", func() { + var realVaultPath string + var realGoalStore storage.GoalStorage + + BeforeEach(func() { + var mkErr error + realVaultPath, mkErr = os.MkdirTemp("", "vault-goal-retain-*") + Expect(mkErr).To(BeNil()) + lockDir, lockErr := os.MkdirTemp("", "vault-goal-retain-lock-*") + Expect(lockErr).To(BeNil()) + DeferCleanup(func() { _ = os.RemoveAll(lockDir) }) + lockDirLocker := ops.NewSessionLockerWithDir(lockDir) + for _, dir := range []string{"24 Tasks", "23 Goals"} { + Expect(os.MkdirAll(filepath.Join(realVaultPath, dir), 0755)).To(Succeed()) + } + realGoalStore = storage.NewGoalStorage(&storage.Config{TasksDir: "24 Tasks", GoalsDir: "23 Goals"}) + + const retainFixture = `--- +phase: execution +status: in_progress +--- +body +` + Expect(os.WriteFile( + filepath.Join(realVaultPath, "23 Goals", "Rollback Goal.md"), + []byte(retainFixture), 0600, + )).To(Succeed()) + + // A validated turn result is authoritative over the child's non-zero exit, so + // the persist-on-success path stays alive and the id lands on top of the + // child's own frontmatter write. The phase change is deliberately different + // from the seeded `execution` so the "child's write survived" assertion is + // non-vacuous. + block := make(chan struct{}) + realStarter := ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + fresh, ferr := realGoalStore.FindGoalByName(ctx, realVaultPath, "Rollback Goal") + if ferr != nil { + return nil, ferr + } + fresh.SetPhase(domain.GoalPhasePlanning.Ptr()) + if ferr := realGoalStore.WriteGoal(ctx, fresh); ferr != nil { + return nil, ferr + } + if _, werr := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ); werr != nil { + return nil, werr + } + done := make(chan error, 1) + done <- stderrors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-block + return nil + }), + lockDirLocker, + ) + DeferCleanup(func() { close(block) }) + DeferCleanup(func() { _ = os.RemoveAll(realVaultPath) }) + + goalWorkOnOp = ops.NewGoalWorkOnOperation( + realGoalStore, + func() string { return pinnedSessionID }, + realStarter, + nil, + ) + vaultPath = realVaultPath + goalName = "Rollback Goal" + }) + + It("goal work-on retains the session id after a non-zero exit when the turn result validated", func() { + Expect(err).To(BeNil()) + Expect(result.Success).To(BeTrue()) + Expect(result.SessionID).To(Equal(pinnedSessionID)) + + written, ferr := realGoalStore.FindGoalByName(ctx, realVaultPath, "Rollback Goal") + Expect(ferr).To(BeNil()) + Expect(written.ClaudeSessionID()).To(Equal(pinnedSessionID)) + // The child's own write survived the persist-on-success re-read. + Expect(written.Phase()).NotTo(BeNil()) + Expect(*written.Phase()).To(Equal(domain.GoalPhasePlanning)) + + raw, rerr := os.ReadFile(filepath.Join(realVaultPath, "23 Goals", "Rollback Goal.md")) + Expect(rerr).To(BeNil()) + Expect(strings.Count(string(raw), "claude_session_id:")).To(Equal(1)) + }) + }) + + Context("goal work-on persists nothing for a failed turn", func() { + var realVaultPath string + var realGoalStore storage.GoalStorage + + BeforeEach(func() { + var mkErr error + realVaultPath, mkErr = os.MkdirTemp("", "vault-goal-clear-*") + Expect(mkErr).To(BeNil()) + lockDir, lockErr := os.MkdirTemp("", "vault-goal-clear-lock-*") + Expect(lockErr).To(BeNil()) + DeferCleanup(func() { _ = os.RemoveAll(lockDir) }) + lockDirLocker := ops.NewSessionLockerWithDir(lockDir) + for _, dir := range []string{"24 Tasks", "23 Goals"} { + Expect(os.MkdirAll(filepath.Join(realVaultPath, dir), 0755)).To(Succeed()) + } + realGoalStore = storage.NewGoalStorage(&storage.Config{TasksDir: "24 Tasks", GoalsDir: "23 Goals"}) + + const clearFixture = `--- +phase: execution +status: in_progress +--- +body +` + Expect(os.WriteFile( + filepath.Join(realVaultPath, "23 Goals", "Rollback Goal.md"), + []byte(clearFixture), 0600, + )).To(Succeed()) + + block := make(chan struct{}) + realStarter := ops.NewClaudeSessionStarterWithRunner( + "/usr/local/bin/claude", + nil, + func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + // The blob reports the turn's own failure; `pkg/ops/goal_workon.go` has + // its own handleClaudeSession that persists the id only after a clean, + // validated turn, so nothing was written for this id on any failure path. + if _, werr := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":0,"is_error":true,"result":"seeded failure text"}`, + ); werr != nil { + return nil, werr + } + done := make(chan error, 1) + done <- stderrors.New("exit status 1") + return done, nil + }, + libtime.WaiterDurationFunc(func(_ context.Context, _ libtime.Duration) error { + <-block + return nil + }), + lockDirLocker, + ) + DeferCleanup(func() { close(block) }) + DeferCleanup(func() { _ = os.RemoveAll(realVaultPath) }) + + goalWorkOnOp = ops.NewGoalWorkOnOperation( + realGoalStore, + func() string { return pinnedSessionID }, + realStarter, + nil, + ) + vaultPath = realVaultPath + goalName = "Rollback Goal" + }) + + It("goal work-on persists no session id for a failed turn", func() { + Expect(err).To(HaveOccurred()) + Expect(result.Success).To(BeFalse()) + + // The child's own reason leads the message; the spec's AC2 permits an + // exit-status mention only as a trailing clause, never before it. + msg := err.Error() + Expect(msg).To(ContainSubstring("seeded failure text")) + if idx := strings.Index(msg, "exit status"); idx >= 0 { + Expect(strings.Index(msg, "seeded failure text")).To(BeNumerically("<", idx)) + } + + // The goal path needs no compensating clear: nothing is persisted for a failed + // turn (the non-interactive branch persists only after a clean, validated + // turn), so the raw file proves the invariant rather than a clear. + raw, rerr := os.ReadFile(filepath.Join(realVaultPath, "23 Goals", "Rollback Goal.md")) + Expect(rerr).To(BeNil()) + Expect(strings.Count(string(raw), "claude_session_id:")).To(Equal(0)) + }) + }) + Context("when the session lock is already held", func() { var ( lockDirLocker ops.SessionLocker diff --git a/pkg/ops/workon_session_writeback_test.go b/pkg/ops/workon_session_writeback_test.go index 126b651..5e102e2 100644 --- a/pkg/ops/workon_session_writeback_test.go +++ b/pkg/ops/workon_session_writeback_test.go @@ -365,4 +365,160 @@ body }) }) + Context("when the child exits non-zero after writing a valid turn result", func() { + // AC6 retain half: under the spec 045 precedence a validated turn result is + // authoritative over the child's non-zero exit, so the pre-persisted session id + // survives what used to be the failed-spawn path and the Vault UI can offer Resume. + const rollbackFixture = `--- +phase: execution +status: in_progress +--- +body +` + var taskStore storage.TaskStorage + + BeforeEach(func() { + taskStore = storage.NewTaskStorage(storageConfig) + Expect(os.WriteFile( + filepath.Join(vaultPath, "24 Tasks", "Repro Task.md"), + []byte(rollbackFixture), 0600, + )).To(Succeed()) + + // Same re-read/set-phase/write-back shape as the rollback Context, but with a + // valid JSON blob written to stdout before the child exits non-zero. The phase + // change is deliberately different from the seeded `execution`, so the + // "child's write survived" assertion is non-vacuous. + detachRun := func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + fresh, err := taskStore.FindTaskByName(ctx, vaultPath, "Repro Task") + if err != nil { + return nil, err + } + fresh.SetPhase(domain.TaskPhasePlanning.Ptr()) + if err := taskStore.WriteTask(ctx, fresh); err != nil { + return nil, err + } + if _, err := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":3,"is_error":false,"result":"done"}`, + ); err != nil { + return nil, err + } + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + } + starter = newStarter(detachRun) + }) + + It("retains the pre-persisted session id when the turn result validated despite the non-zero exit", func() { + currentDateTime := libtime.NewCurrentDateTime() + currentDateTime.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) + testVault := config.Vault{ + Path: vaultPath, + Name: "test-vault", + WorkOnCommand: "/vault-cli:work-on-task", + } + workOnOp := ops.NewWorkOnOperation( + taskStore, mockDailyNote, currentDateTime, func() string { return pinnedSessionID }, starter, nil, + ) + + // The retain is caller-side (handleClaudeSession/Execute): the pre-spawn + // persist wrote the id, the validated result stops the compensating clear from + // firing, so only Execute observes the id surviving — a direct StartSession + // call never sees it. + result, err := workOnOp.Execute( + ctx, vaultPath, "Repro Task", "user@example.com", "test-vault", + false, sessionDir, &testVault, + ) + Expect(err).To(BeNil()) + Expect(result.Success).To(BeTrue()) + Expect(result.SessionID).To(Equal(pinnedSessionID)) + + // In-test form of the spec's `grep -c '^claude_session_id:' ` + // evidence: exactly one id line survives the validated-but-non-zero-exit turn. + raw, err := os.ReadFile(filepath.Join(vaultPath, "24 Tasks", "Repro Task.md")) + Expect(err).To(BeNil()) + Expect(strings.Count(string(raw), "claude_session_id:")).To(Equal(1)) + Expect(strings.Contains(string(raw), pinnedSessionID)).To(BeTrue()) + // Seeded with phase: execution, so this only passes if the child's write landed. + Expect(strings.Contains(string(raw), "phase: planning")).To(BeTrue()) + }) + }) + + Context("when the child reports its own failure", func() { + const rollbackFixture = `--- +phase: execution +status: in_progress +--- +body +` + var taskStore storage.TaskStorage + + BeforeEach(func() { + taskStore = storage.NewTaskStorage(storageConfig) + Expect(os.WriteFile( + filepath.Join(vaultPath, "24 Tasks", "Repro Task.md"), + []byte(rollbackFixture), 0600, + )).To(Succeed()) + + // Same re-read/set-phase/write-back shape as the rollback Context (the child's + // frontmatter write must survive the clear), but the blob reports the turn's + // own failure, so the compensating clear still fires. + detachRun := func(_ []string, _ string, stdout *os.File) (<-chan error, error) { + fresh, err := taskStore.FindTaskByName(ctx, vaultPath, "Repro Task") + if err != nil { + return nil, err + } + fresh.SetPhase(domain.TaskPhasePlanning.Ptr()) + if err := taskStore.WriteTask(ctx, fresh); err != nil { + return nil, err + } + if _, err := stdout.WriteString( + `{"session_id":"` + pinnedSessionID + `","num_turns":0,"is_error":true,"result":"seeded failure text"}`, + ); err != nil { + return nil, err + } + done := make(chan error, 1) + done <- errors.New("exit status 1") + return done, nil + } + starter = newStarter(detachRun) + }) + + It("clears the pre-persisted session id when the turn result reports its own failure", func() { + currentDateTime := libtime.NewCurrentDateTime() + currentDateTime.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) + testVault := config.Vault{ + Path: vaultPath, + Name: "test-vault", + WorkOnCommand: "/vault-cli:work-on-task", + } + workOnOp := ops.NewWorkOnOperation( + taskStore, mockDailyNote, currentDateTime, func() string { return pinnedSessionID }, starter, nil, + ) + + result, err := workOnOp.Execute( + ctx, vaultPath, "Repro Task", "user@example.com", "test-vault", + false, sessionDir, &testVault, + ) + Expect(err).To(HaveOccurred()) + Expect(result.Success).To(BeFalse()) + + // The child's own reason leads the message; the spec's AC2 permits an + // exit-status mention only as a trailing clause, never before it. + msg := err.Error() + Expect(msg).To(ContainSubstring("seeded failure text")) + if idx := strings.Index(msg, "exit status"); idx >= 0 { + Expect(strings.Index(msg, "seeded failure text")).To(BeNumerically("<", idx)) + } + + // The compensating clear removed the id and this run's metrics entry, but the + // child's `phase: planning` write survived (the fixture seeded `execution`). + raw, err := os.ReadFile(filepath.Join(vaultPath, "24 Tasks", "Repro Task.md")) + Expect(err).To(BeNil()) + Expect(strings.Count(string(raw), "claude_session_id:")).To(Equal(0)) + Expect(strings.Contains(string(raw), pinnedSessionID)).To(BeFalse()) + Expect(strings.Contains(string(raw), "phase: planning")).To(BeTrue()) + }) + }) + }) diff --git a/prompts/completed/208-spec-045-result-outranks-exit-code.md b/prompts/completed/208-spec-045-result-outranks-exit-code.md new file mode 100644 index 0000000..12c5757 --- /dev/null +++ b/prompts/completed/208-spec-045-result-outranks-exit-code.md @@ -0,0 +1,206 @@ +--- +status: completed +spec: [045-bug-exit-code-outranks-validated-turn] +summary: Inverted runDetachedTurn precedence in pkg/ops/claude_session.go so a validated turn result decides success and the child's exit code only decides when no usable result exists; added errClaudeOutputUnparseable sentinel, rejectTurn helper, doc-comment updates, and AC1-AC4 Ginkgo specs +execution_id: vault-cli-exit-code-exec-208-spec-045-result-outranks-exit-code +dark-factory-version: dev +created: "2026-09-06T13:20:00Z" +queued: "2026-09-06T13:42:38Z" +started: "2026-09-06T13:57:06Z" +completed: "2026-09-06T14:00:37Z" +--- + +# Result outranks exit code in the detached turn (spec 045, prompt 1 of 3) + + +- A headless session whose child process produced a complete, valid turn result is no longer thrown away just because that process exited non-zero. +- The captured turn result becomes the authority on success; the exit code is consulted only when there is no usable result to judge. +- When the child's result is present but rejected, the reported error now leads with the child's own explanation instead of an opaque process status, so an operator reads the real reason first. +- When the result is missing, unreadable, or not valid turn JSON, the reported error still names the process exit status — that is the only case where the exit code decides. +- A turn that runs past its wait bound, or whose wait is cancelled, keeps failing exactly as before, and deliberately never inspects the partially-written result. +- The existing rule that a genuinely failed turn produces an error (so the caller can undo its bookkeeping) is unchanged — only the definition of "failed" moves. +- Unit tests cover all four result/exit combinations plus the timeout-with-a-complete-result case, which locks out the tempting refactor that would let a still-running child's partial output count as success. +- One pre-existing lock-lifecycle test that accidentally depended on the old precedence is corrected so it still tests what it claims to test. + + + +Invert the precedence inside `runDetachedTurn` so a validated turn result decides success and the child's exit code only decides when no usable result exists. Covers spec 045 Desired Behaviors 1-5 and Acceptance Criteria 1-4. + + + +Read `CLAUDE.md` and `docs/dod.md` for project conventions first. + +Read in full before changing anything: + +- `pkg/ops/claude_session.go` — the whole file. The change is confined to `runDetachedTurn` and `validateSessionTurn`. Note the existing shape: `runDetachedTurn` creates the temp file, defers `os.Remove` + `Close`, calls `c.detachRun(args, cwd, outFile)`, then `select`s on the child-exit channel `done` against `waitCh` (fed by `c.waiter.Wait`). Today the `case exitErr := <-done:` branch returns immediately when `exitErr != nil`, before the `os.ReadFile(outFile.Name())` + `validateSessionTurn(ctx, output)` pair that sits below the `select`. +- `pkg/ops/errors.go` — the package's sentinel-error pattern (`stderrors "errors"` import alias, `stderrors.New(...)`, doc comment stating what the sentinel means). The new sentinel this prompt adds follows this exact pattern. +- `pkg/ops/claude_session_test.go` — the whole file. Ginkgo v2 + Gomega, `ops_test` package. The `Context("non-interactive branch")` block is where the new specs go; note its `validTurnJSON` constant, its blocking-waiter discipline (`bw := blockWaiter` captured spec-locally, never read from the outer variable inside a waiter closure), and how each spec that needs a bespoke child rebuilds `starter` via `ops.NewClaudeSessionStarterWithRunner`. +- `pkg/ops/workon_session_writeback_test.go` and `pkg/ops/goal_workon_test.go` — read only the specs that assert `ContainSubstring("exit status 1")` (search for that string). They must keep passing byte-identical; their `detachRun` fakes take `_ *os.File` and write nothing, so they exercise the zero-length-output path. + +Read these coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `github.com/bborbe/errors` wrapping with a real `ctx`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo/Gomega conventions in this codebase. + +Library facts already verified against the module — do not re-derive them: +- `github.com/bborbe/errors` exposes `Wrap(ctx, err, msg)`, `Wrapf(ctx, err, format, args...)`, `Errorf(ctx, format, args...)`, `New(ctx, msg)` and `Is(err, target)`. `Wrap`/`Wrapf` build on `github.com/pkg/errors`, so `Error()` reads `": "` and the chain supports `errors.Is`. +- `Errorf(ctx, ...)`'s `Error()` is exactly the formatted string — no prefix is added. That is what makes a result-text-first message possible. +- The file already imports `"encoding/json"`, `"log/slog"`, `"os"` and `"github.com/bborbe/errors"` (aliased as `errors`). Adding a sentinel needs a `stderrors "errors"` import, matching `pkg/ops/errors.go`. + + + + +1. **Add an unparseable-output sentinel** to `pkg/ops/claude_session.go`, declared immediately above `validateSessionTurn`, following the `pkg/ops/errors.go` pattern (add the `stderrors "errors"` import to this file): + + ```go + // errClaudeOutputUnparseable marks a turn result that could not be parsed at all, + // as opposed to one that parsed and then failed a predicate. The distinction is + // load-bearing: a parsed blob carries the child's own `result` text and can explain + // itself, while an unparseable one cannot — so only the unparseable case falls back + // to the child's exit status as the reason. + var errClaudeOutputUnparseable = stderrors.New("claude output is not valid turn JSON") + ``` + +2. **Rewrite `validateSessionTurn`'s three rejection messages so the child's own `result` text leads.** Keep the function as the single shared validator for both branches, keep the struct, keep the predicate order (`session_id` → `num_turns` → `is_error`), and keep the `ctx`-carrying `github.com/bborbe/errors` constructors. + + - Parse failure: return `errors.Wrapf(ctx, errClaudeOutputUnparseable, "parse claude output: %v", jsonErr)`. The message must still contain the literal substring `parse claude output` (the existing interactive-branch spec asserts it) and `errors.Is(err, errClaudeOutputUnparseable)` must be true. + - Add a small unexported helper beside `validateSessionTurn` that composes a rejection so the three predicate branches do not duplicate the empty-result fallback: + + ```go + // rejectTurn builds the error for a turn result that parsed but failed a predicate. + // The child's own `result` text leads, because it is the only part of the message an + // operator can act on; the predicate name follows in parentheses. When the child + // reported no result text at all there is nothing to lead with, so the predicate + // stands alone. + func rejectTurn(ctx context.Context, resultText string, reason string) error { + if resultText == "" { + return errors.New(ctx, reason) + } + return errors.Errorf(ctx, "%s (%s)", resultText, reason) + } + ``` + + - Empty `session_id` → `rejectTurn(ctx, result.Result, "claude returned empty session_id")` + - `num_turns == 0` → `rejectTurn(ctx, result.Result, "claude returned num_turns: 0")` + - `is_error == true` → `rejectTurn(ctx, result.Result, "claude reported is_error: true")` + + Each reason string deliberately contains the JSON field name (`session_id`, `num_turns`, `is_error`) — spec AC2 asserts on those names. + +3. **Invert the precedence inside `runDetachedTurn`.** Move the read + validate pair from below the `select` into the `case exitErr := <-done:` branch. The waiter branch is untouched and must NOT read the file. The resulting branch, in order: + + 1. `output, readErr := os.ReadFile(outFile.Name())` — on error `return errors.Wrap(ctx, readErr, "read claude output")`. This is the fd/permission failure mode; the read error is the reason, not the exit status. + 2. `validateErr := validateSessionTurn(ctx, output)`. + 3. `validateErr == nil` → the turn succeeded. If `exitErr != nil`, emit exactly one line, verbatim: + + ```go + slog.Warn("validated turn result overrides non-zero child exit", "err", exitErr) + ``` + + This is the only trace of a signal that is now deliberately ignored, so the message string is pinned by a `` check rather than left to wording. Then `return nil`. + 4. `exitErr != nil && errors.Is(validateErr, errClaudeOutputUnparseable)` → `return errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. A zero-length file lands here naturally, because unmarshalling empty bytes is a parse failure. This is the sole surviving path where the exit code is the reason, and this string must appear exactly once in the file. + 5. Otherwise `return validateErr` **unwrapped**. Do not wrap it — `errors.Wrap` would prepend text and destroy the result-text-leads property AC2 asserts. + + Keep the existing `// The child has exited, so its fd is closed and the file is complete.` reasoning as a comment inside the branch, and add a comment stating explicitly why the read may not be hoisted above the `select`: on the timeout and cancellation paths the child is still running, so any bytes present are partial by definition and must never be validated as success. + +4. **Update the stale doc comments in `pkg/ops/claude_session.go`** so the file's prose matches the new contract: + - `ClaudeSessionStarter.StartSession`'s comment currently says "Any outcome other than a clean, validated turn returns an error". Restate it as: the turn is judged by its validated result; a non-zero child exit is not itself a failure when the result validates. + - `runDetachedTurn`'s comment currently says "every exit path except a clean, validated turn returns an error". Restate the same way, and keep the existing rationale about not returning before the child exits. + - Do not touch the `sessionTurnTimeout` comment, `defaultDetachedRunner`, `defaultCommandRunner`, the `maxTurns` field comment, or the interactive branch. + +5. **Update the existing assertions that pin the old predicate wording** in `pkg/ops/claude_session_test.go`. These are the only pre-existing assertions this prompt may change, and each changes for a stated reason: + - Interactive spec `"returns error containing 0 turns and result"`: `ContainSubstring("0 turns")` → `ContainSubstring("num_turns")`. Keep the `ContainSubstring("Unknown command: /x")` assertion. + - Non-interactive spec `"validates the turn and rejects a zero-turn result"`: `ContainSubstring("0 turns")` → `ContainSubstring("num_turns")`. + - Non-interactive spec `"validates the turn and rejects an is_error result"`: `ContainSubstring("claude reported error")` → `ContainSubstring("claude reported is_error")`. + - The `empty session_id` / `missing session_id field` / `rejects an unparseable turn result` specs need no change — verify that and leave them alone. + +6. **Fix the one pre-existing spec that silently depended on the old precedence.** In the `Context("session lock lifecycle")` block, the spec `"releases the lock when the child exits with an error"` sends `lockDoneCh <- ErrTest` while that Context's shared `JustBeforeEach` `detachRun` writes `validTurnJSON` to the stdout file. Under the new precedence that combination is a **success**, so the spec's `Expect(err).To(HaveOccurred())` would fail and the lock-release property it exists to prove would go untested. + + Rebuild `starter` inside that spec (the same way sibling specs in that Context already do) with a `detachRun` that takes `_ *os.File`, writes nothing, increments `spawnCount`, and returns a channel carrying `ErrTest`. Keep both assertions (`HaveOccurred`, `ContainSubstring("exited with error")`) and the re-acquire/release assertions unchanged. Add a comment explaining that the child must leave the output file empty for the exit status to be authoritative under the result-over-exit-code precedence. Do not change the sibling lock specs. + +7. **Add new specs to `Context("non-interactive branch")` in `pkg/ops/claude_session_test.go`.** Each rebuilds `starter` locally with `ops.NewClaudeSessionStarterWithRunner`, captures the blocking waiter spec-locally (`bw := blockWaiter`) exactly as neighbouring specs do, and drives `starter.StartSession(ctx, "session-abc", "prompt", "/my/vault", "", false)`: + + - **AC1 — valid blob, non-zero exit → success.** `detachRun` writes `{"session_id":"session-abc","num_turns":3,"is_error":false,"result":"done"}` to `stdout` and returns a channel carrying `errors.New("exit status 1")` (this test file imports stdlib `errors` unaliased — the same call the neighbouring `"treats a child exit error as an error"` spec already makes). Assert `Expect(err).To(BeNil())`. + - **AC2 — parsed-but-rejected blob leads with the child's reason.** Three specs, each with a non-zero exit on the channel and a seeded distinctive `result` string (e.g. `"seeded failure text"`): + - `{"session_id":"session-abc","num_turns":2,"is_error":true,"result":"seeded failure text"}` → `Expect(err.Error()).To(HavePrefix("seeded failure text"))` and `ContainSubstring("is_error")`. + - `{"session_id":"session-abc","num_turns":0,"is_error":false,"result":"seeded failure text"}` → `HavePrefix("seeded failure text")` and `ContainSubstring("num_turns")`. + - `{"session_id":"","num_turns":2,"is_error":false,"result":"seeded failure text"}` → `HavePrefix("seeded failure text")` and `ContainSubstring("session_id")`. + Each of the three must additionally assert `Expect(err.Error()).NotTo(HavePrefix("exit status"))` — an exit-status mention may trail, but must never precede the child's own reason. + - **AC3 — non-empty but unparseable output with a non-zero exit names the exit status.** `detachRun` writes `not valid json at all` to `stdout` and returns a channel carrying `errors.New("exit status 1")`. Assert the message contains `exit status`. (The zero-length half of AC3 is already covered by the untouched spec `"treats a child exit error as an error"` — do not modify it.) + - **AC4 — timeout with a valid blob already on disk still fails.** `detachRun` writes the valid blob to `stdout` and returns a channel that never fires (`make(chan error)`); the waiter returns `nil` immediately so the timeout branch wins the select. Assert `err != nil` and `ContainSubstring("did not complete within")`. Add a comment naming this as the regression lock against hoisting the read above the `select`: the child is still running, so the bytes present are partial by definition. + - **Read error is surfaced as a read error, not an exit status** is not separately testable without an injected filesystem — do not fake it, and do not add a filesystem seam for it. The wrap in requirement 3.1 is the implementation; leave it untested here. + +8. **Do NOT touch** `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, `pkg/ops/workon_session_writeback_test.go`, `pkg/ops/goal_workon_test.go`, `pkg/ops/workon_test.go`, `docs/work-on-session-lifecycle.md`, `CHANGELOG.md`, or anything under `scenarios/`. Prompts 2 and 3 of this spec own those. + +9. **Self-check before finishing.** Re-run every command in `` and confirm each passes. Then walk spec 045 Acceptance Criteria 1-4 one by one against the actual diff and state which spec or code line satisfies each. + + + + +- `validateSessionTurn` stays the single shared validator for both branches — do not fork its logic into the detached path. +- **The read must stay inside the child-exited branch.** Hoisting `os.ReadFile` above the `select` is the specific refactor this spec forbids: a timed-out child is still writing, so its partial blob could validate as success. The `awk` check in `` fails if the read moves. +- The temp file must still be unlinked on every return path, including cancel and timeout where the child holds the fd — do not touch the existing `defer` that does `os.Remove` + `Close`. +- Stderr still goes to `os.DevNull` — `defaultDetachedRunner` is unchanged. +- The interactive branch's `cmd.Output()` + `validateSessionTurn` call sequence must not change. +- The compensating clear is not weakened: `runDetachedTurn` must still return an error for a genuinely failed turn. Only the definition of "failed" moves. +- Tests use Ginkgo v2 + Gomega with Counterfeiter mocks — no stdlib `t.Run` table tests. +- Errors wrap via `github.com/bborbe/errors` with a real `ctx` — no `fmt.Errorf`, no bare `return err`. +- The three existing `exit status 1` assertions must keep passing **unchanged**: the `"treats a child exit error as an error"` spec in `pkg/ops/claude_session_test.go`, the `"goal work-on early exit rollback..."` spec in `pkg/ops/goal_workon_test.go`, and the `"clears the pre-persisted session id..."` spec in `pkg/ops/workon_session_writeback_test.go`. Their `detachRun` stubs ignore the `*os.File` parameter, so they exercise the zero-length-output path and encode the AC3 contract. If a change makes one of them fail, the empty-file case has been misrouted to the predicate branch — fix the routing, never the assertion. +- Do NOT add config fields, flags, thresholds, or metrics. The spec asks for a precedence inversion and one log line; nothing else. +- Do NOT root-cause why a clean-`end_turn` child exits 1 — explicit spec Non-goal. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +Run from the repo root: + +``` +make precommit +``` + +Must exit 0. + +Then every one of these must exit 0. They are written as self-failing assertions on +purpose: a bare `grep -c` exits 0 for any non-zero count, and a piped `go test` takes +the exit status of the last stage, so both forms report success on unchanged code. + +``` +test "$(grep -c 'claude session exited with error' pkg/ops/claude_session.go)" = "1" +test "$(grep -c 'errClaudeOutputUnparseable' pkg/ops/claude_session.go)" -ge 3 +test "$(grep -c 'did not complete within' pkg/ops/claude_session_test.go)" -ge 3 +test "$(grep -c 'HavePrefix' pkg/ops/claude_session_test.go)" -ge 3 +test "$(grep -c 'exited with error' pkg/ops/claude_session_test.go)" -ge 2 +test "$(grep -c 'validated turn result overrides non-zero child exit' pkg/ops/claude_session.go)" = "1" +``` + +The read must sit inside the child-exited case branch, not merely below the `select` +keyword — the unchanged file already satisfies "read line > select line", so that +weaker form proves nothing: + +``` +awk '/case exitErr := <-done:/{a=NR} /case err := <-waitCh:/{b=NR} /os\.ReadFile\(outFile\.Name\(\)\)/{r=NR} END{exit !(a>0 && b>a && r>a && r diff --git a/prompts/completed/209-spec-045-workon-writeback-precedence.md b/prompts/completed/209-spec-045-workon-writeback-precedence.md new file mode 100644 index 0000000..128ff96 --- /dev/null +++ b/prompts/completed/209-spec-045-workon-writeback-precedence.md @@ -0,0 +1,153 @@ +--- +status: completed +spec: [045-bug-exit-code-outranks-validated-turn] +summary: 'Extended workon_session_writeback_test.go and goal_workon_test.go with four Execute-driven specs proving spec 045''s clear-vs-retain precedence end-to-end on real vault files (task and goal paths), with AC2''s child-reason-leads message ordering asserted; CHANGELOG updated with a test: bullet.' +execution_id: vault-cli-exit-code-exec-209-spec-045-workon-writeback-precedence +dark-factory-version: dev +created: "2026-09-06T13:20:00Z" +queued: "2026-09-06T14:07:44Z" +started: "2026-09-06T14:07:49Z" +completed: "2026-09-06T14:14:38Z" +--- + +# Task and goal write-back under the new precedence (spec 045, prompt 2 of 3) + + +- Proves on real files that a task whose headless turn produced a valid result keeps its session id, even when the underlying process exited non-zero — the case the Vault UI needs in order to offer Resume. +- Proves the same for a goal, which persists its id on a different code path that must not be left behind. +- Proves the undo still fires for a genuinely failed turn: a child that reports its own failure leaves no session id on the task file. +- Proves the same undo on the goal path, where nothing is persisted for a failed turn in the first place. +- Confirms that the error an operator sees for a genuinely failed turn leads with the child's own explanation rather than a process exit status. +- Tests only — no behavior change in this prompt. + + + +Extend the two caller-side write-back test files so the clear-vs-retain consequence of the new precedence is proven end-to-end through `Execute` on real vault files, for both the task path and the goal path. Covers spec 045 Desired Behavior 6 and Acceptance Criteria 5 and 6. + + + +This prompt depends on prompt 1 of spec 045 being already applied — `runDetachedTurn` now treats a validated turn result as authoritative over a non-zero child exit, and `validateSessionTurn`'s rejection messages lead with the child's own `result` text followed by the failed predicate in parentheses (`claude reported is_error: true`, `claude returned num_turns: 0`, `claude returned empty session_id`). + +Read `CLAUDE.md` and `docs/dod.md` first. Then read in full: + +- `pkg/ops/workon_session_writeback_test.go` — the whole file. Note: the shared `newStarter(detachRun)` helper, the spec-local `bw := blockWaiter` capture inside the waiter closure (a waiter that returns immediately races the select and makes the outcome nondeterministic), the `pinnedSessionID` constant, and the existing three Contexts: the happy-path `Context("task work-on")` (child writes frontmatter + a valid blob, `done <- nil`) and `Context("when the child exits non-zero inside the liveness window")` (child writes frontmatter, writes **nothing** to stdout, `done <- errors.New("exit status 1")`, asserts the id was cleared). +- `pkg/ops/goal_workon_test.go` — the whole file. Note `Context("goal work-on early exit rollback")`, which builds a real goal store on a temp vault, seeds `23 Goals/Rollback Goal.md`, and drives `Execute`; and note the file imports stdlib `errors` as `stderrors` while `github.com/bborbe/errors` is imported as `errors`. +- `pkg/ops/workon.go` — `handleClaudeSession`. The task path pre-persists the id plus a `metrics_sessions` entry BEFORE spawning, and runs a re-read-based compensating clear when `StartSession` returns an error. +- `pkg/ops/goal_workon.go` — `handleClaudeSession` (a separate method from the task one, on `goalWorkOnOperation`). The non-interactive goal path persists the id only AFTER a successful turn via `persistGoalSessionID`, and has no compensating clear because nothing was written. +- `pkg/ops/claude_session.go` — `runDetachedTurn` and `validateSessionTurn` as they now stand, so the seeded blobs and asserted messages match the real implementation. + +Read this coding-plugin doc (in-container path): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo/Gomega conventions in this codebase. + + + + +1. **Add a retain spec to `pkg/ops/workon_session_writeback_test.go`** — new `Context`, modelled on the existing `Context("when the child exits non-zero inside the liveness window")` but with the opposite outcome. Name it so the intent is legible, e.g. `Context("when the child exits non-zero after writing a valid turn result")`. + + Seed the fixture with `phase: execution` (same as the sibling Context's `rollbackFixture`). The `detachRun` fake must, in this order: re-read the task through `taskStore.FindTaskByName`, set `domain.TaskPhasePlanning` — deliberately DIFFERENT from the seeded value, or the "child's write survived" assertion is vacuous — write it back (proving the child's frontmatter write survives), write the valid blob + `{"session_id":"","num_turns":3,"is_error":false,"result":"done"}` + to the `stdout *os.File`, then send `errors.New("exit status 1")` on a buffered `done` channel. The waiter must block, exactly as the neighbouring Contexts do. + + Drive it through `workOnOp.Execute(...)` (never a direct `StartSession` call — the persist/clear is caller-side and only `Execute` observes it) and assert: + - `Expect(err).To(BeNil())` and `Expect(result.Success).To(BeTrue())` and `Expect(result.SessionID).To(Equal(pinnedSessionID))` + - On the raw file bytes (`os.ReadFile` of `24 Tasks/Repro Task.md`), `strings.Count(string(raw), "claude_session_id:")` equals `1` — this is the in-test form of the spec's `grep -c '^claude_session_id:' ` evidence. + - `strings.Contains(string(raw), pinnedSessionID)` is true, and `strings.Contains(string(raw), "phase: planning")` is true — the fixture seeded `execution`, so this only passes if the child's write actually landed. + + Add a comment stating what this spec is for: a non-zero exit no longer discards a turn whose result validated, so the pre-persisted id survives and the Vault UI can offer Resume. + +2. **Add an `is_error` clear spec to `pkg/ops/workon_session_writeback_test.go`** — new `Context`, e.g. `Context("when the child reports its own failure")`. The `detachRun` fake writes the child's frontmatter change (same re-read/set-phase/write-back shape as the sibling Contexts, so the "preserve the child's write" property is asserted here too), then writes + `{"session_id":"","num_turns":0,"is_error":true,"result":"seeded failure text"}` + to `stdout`, then sends `errors.New("exit status 1")` on the `done` channel. + + Assert through `Execute`: + - `Expect(err).To(HaveOccurred())` and `Expect(result.Success).To(BeFalse())` + - `err.Error()` contains `seeded failure text`, and the child's reason precedes any exit-status mention. `Execute` wraps with `start work-on session`, so `HavePrefix` is unavailable at this level; assert ordering explicitly: + + ```go + msg := err.Error() + Expect(msg).To(ContainSubstring("seeded failure text")) + if idx := strings.Index(msg, "exit status"); idx >= 0 { + Expect(strings.Index(msg, "seeded failure text")).To(BeNumerically("<", idx)) + } + ``` + + Spec AC2 permits an exit-status mention as a trailing clause; it must never precede the child's own reason. Do NOT assert full-string absence of `exit status` — that would narrow prompt 1's contract below what the spec allows. + - On the raw file bytes, `strings.Count(string(raw), "claude_session_id:")` equals `0` and `strings.Contains(string(raw), pinnedSessionID)` is false — the compensating clear removed both the id and this run's metrics entry. + - The child's frontmatter write survived the clear: `strings.Contains(string(raw), "phase: planning")` is true, given the fixture seeded `phase: execution`. Do not seed and write the same phase — that assertion would pass even if the child wrote nothing. + +3. **Add the mirrored pair to `pkg/ops/goal_workon_test.go`**, alongside `Context("goal work-on early exit rollback")` and using the same real-goal-store-on-a-temp-vault setup that Context already establishes (temp vault dir, `23 Goals` + `24 Tasks` dirs, a seeded goal fixture, `ops.NewSessionLockerWithDir` on a temp lock dir, a blocking waiter with a `DeferCleanup` close). Critical detail: this file drives `Execute` from a top-level `JustBeforeEach` (~line 75) over the outer `vaultPath`, `goalName`, and `goalWorkOnOp` vars — each new Context's `BeforeEach` MUST reassign all three exactly as the rollback Context does (~lines 524-531), or the spec silently drives the mock store and passes meaninglessly. Do not add a Context-local `Execute` call. The file does not currently import `strings` — add it: + + - **Retain**: `detachRun` writes `{"session_id":"","num_turns":3,"is_error":false,"result":"done"}` to `stdout` and sends `stderrors.New("exit status 1")`. Assert `err` is nil, `result.Success` is true, and the goal re-read through the real store has `ClaudeSessionID() == pinnedSessionID`. Also assert on the raw goal file bytes that `claude_session_id:` appears exactly once. Add a comment: `pkg/ops/goal_workon.go` has its own `handleClaudeSession` that persists only after a successful turn, so the goal path must be proven separately from the task path — it is the one most easily left behind. + - **Clear**: `detachRun` writes `{"session_id":"","num_turns":0,"is_error":true,"result":"seeded failure text"}` to `stdout` and sends `stderrors.New("exit status 1")`. Assert `err` occurred, `result.Success` is false, `err.Error()` contains `seeded failure text` with any `exit status` mention appearing strictly after it (same ordering assertion as requirement 2 — never a full-string absence assertion), and the raw goal file (`23 Goals/Rollback Goal.md`) contains zero occurrences of `claude_session_id:`. Note in a comment that the goal path needs no compensating clear because nothing was persisted for a failed turn — the assertion proves the invariant, not a clear. + +4. **Do not modify the two protected specs in the files this prompt may touch** (a third, in `pkg/ops/claude_session_test.go`, belongs to prompt 1 and is out of scope here). The `"goal work-on early exit rollback preserves the child's frontmatter write"` spec and the `"clears the pre-persisted session id and preserves the child's frontmatter write when the child exited non-zero inside the window"` spec must stay byte-identical, along with their `detachRun` fakes that ignore the `*os.File` parameter. They encode the zero-length-output contract: exit status is authoritative only when there is no usable blob. + +5. **Do not change production code in this prompt.** No edits under `pkg/ops/*.go` except the two `_test.go` files named above. If a new spec fails, the fix belongs in the test's seeded blob or fake, not in `runDetachedTurn` — unless the failure reveals prompt 1 shipped the wrong routing, in which case fix `pkg/ops/claude_session.go` and say so explicitly in the completion summary. + +6. **Do not touch** `docs/work-on-session-lifecycle.md`, `CHANGELOG.md`, or anything under `scenarios/` — prompt 3 owns those. + +7. **Self-check before finishing.** Re-run every command in `` and confirm it passes. Then walk spec 045 Acceptance Criteria 5 and 6 against the four new specs and state which spec satisfies which half. + + + + +- Tests use Ginkgo v2 + Gomega with Counterfeiter mocks — no stdlib `t.Run` table tests. +- The waiter in every new spec must BLOCK (closed by `DeferCleanup`). A waiter that returns immediately makes both select branches ready and flips the outcome nondeterministically between success and a spurious turn-timeout error. +- Capture the block channel into a spec-local variable before the waiter closure reads it — `StartSession` can return via the child-exit branch while the waiter goroutine is still parked, so that goroutine outlives the spec and must not read a variable the next spec reassigns. +- Every new success-path fake must write a valid JSON blob to the `stdout *os.File`, or validation fails with `parse claude output`. +- The compensating clear is not weakened by this spec — a genuinely failed turn must still clear the id. These tests are the proof, not a relaxation. +- Errors wrap via `github.com/bborbe/errors` with a real `ctx` — no `fmt.Errorf`, no bare `return err` — if any helper code is added. +- Note for the agent, not a task: an older spec (041) recorded evidence greps pinning the count of `ClaudeSessionID()` / `MetricsSessions()` accessor calls in `workon_session_writeback_test.go` at 2. Those counts are historical evidence for a completed spec, are not enforced by any script or by `make precommit`, and adding specs here legitimately changes them. Prefer raw-file `strings.Count` assertions (as the existing rollback spec does) so the new specs assert the spec-045 evidence shape directly. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +Run from the repo root: + +``` +make precommit +``` + +Must exit 0. + +Then: + +``` +go test ./pkg/ops/... 2>&1 | tail -5 +``` +must report no failures. + +``` +grep -c 'seeded failure text' pkg/ops/workon_session_writeback_test.go +``` +must print `1` or more. + +``` +grep -c 'seeded failure text' pkg/ops/goal_workon_test.go +``` +must print `1` or more. + +``` +grep -c 'claude_session_id:' pkg/ops/workon_session_writeback_test.go +``` +must print `3` or more (the pre-existing rollback assertion plus the two new ones). + +``` +grep -c 'claude_session_id:' pkg/ops/goal_workon_test.go +``` +must print `2` or more (the two new raw-file assertions). + +``` +grep -c 'exit status 1' pkg/ops/workon_session_writeback_test.go +``` +must print `4` or more. The baseline is 2 — the protected spec's `done <- errors.New(...)` line and its `ContainSubstring` assertion — plus one line from each of the two new specs. A threshold of 3 is satisfied by adding only one of the two. + +``` +test "$(grep -c 'is_error\":false' pkg/ops/workon_session_writeback_test.go)" -ge 3 +test "$(grep -c 'is_error\":false' pkg/ops/goal_workon_test.go)" -ge 3 +``` +Both must exit 0. Baseline in each file is 2 (the existing happy-path blobs), so this +gates the AC6 *retain* half — the actual bug being fixed. Without it, every other check +here is satisfiable by adding only the two *clear* specs. + diff --git a/prompts/completed/210-spec-045-docs-and-changelog.md b/prompts/completed/210-spec-045-docs-and-changelog.md new file mode 100644 index 0000000..9e51f91 --- /dev/null +++ b/prompts/completed/210-spec-045-docs-and-changelog.md @@ -0,0 +1,137 @@ +--- +status: completed +spec: [045-bug-exit-code-outranks-validated-turn] +summary: 'Rewrote docs/work-on-session-lifecycle.md contract to the new result-over-exit-code precedence (spec 045 AC7) and appended the corresponding CHANGELOG fix bullet under ## Unreleased (AC8), all greps and make precommit passing' +execution_id: vault-cli-exit-code-exec-210-spec-045-docs-and-changelog +dark-factory-version: dev +created: "2026-09-06T13:20:00Z" +queued: "2026-09-06T14:07:44Z" +started: "2026-09-06T14:14:39Z" +completed: "2026-09-06T14:16:58Z" +--- + +# Lifecycle doc contract rewrite and CHANGELOG bullet (spec 045, prompt 3 of 3) + + +- The design document currently states, as intended behavior, the exact rule this spec removes: that a non-zero child exit always fails the turn. That sentence is deleted. +- It is replaced by a statement of the new precedence — the validated turn result decides, and the process exit status only decides when there is no usable result to judge. +- The document also records why the exit status is the weaker signal: it arrives with no explanation attached, while the result is a structured document the code already knows how to validate. +- The section explaining why the wait bound never inspects a still-running child's output is made explicit, so a future reader does not "simplify" the read out of its branch. +- The changelog records the precedence change for the next release. +- Documentation and changelog only — no code, no tests, no scenarios. + + + +Bring `docs/work-on-session-lifecycle.md` in line with the behavior spec 045 shipped, replacing the now-wrong documented contract, and record the change in the changelog. Covers spec 045 Acceptance Criteria 7 and 8. + + + +This prompt depends on prompts 1 and 2 of spec 045 already being applied — the doc must describe what actually shipped on this branch, not an intention. + +Read `CLAUDE.md` and `docs/dod.md` first. The changelog placement rule from `docs/dod.md` is load-bearing: `## Unreleased` goes **below** the preamble block (the `All notable changes…` line and the `* MAJOR / MINOR / PATCH` lines) and **above** the newest `## vX.Y.Z` section — never between the `# Changelog` title and the preamble. + +Then read in full: + +- `docs/work-on-session-lifecycle.md` — the whole file. The sections that matter here are `## The fate of --output-format json` and `## What the turn timeout does and does not cover`. The second of those contains the sentence this prompt must remove: *"Expiry, ctx cancellation, and a non-zero child exit all return an error, so the caller persists nothing and the UI keeps showing **Start** rather than offering a Resume that cannot work."* +- `CHANGELOG.md` — read the top ~20 lines before editing. At authoring time it had **no** `## Unreleased` section and the newest was `## v0.124.1`, but `.maintainer.yaml` sets `autoRelease: true`, so verify the current shape rather than trusting that. +- `pkg/ops/claude_session.go` as it now stands — `runDetachedTurn`'s child-exited branch and `validateSessionTurn`'s rejection messages. The doc must describe the real implementation, including the real predicate wording. +- `specs/in-progress/045-bug-exit-code-outranks-validated-turn.md` — the "Why this is a bug" section carries the argument the replacement prose should compress into the doc. + +Read this coding-plugin doc (in-container path): +- `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — bullet format and placement. + + + + +1. **Delete the stale contract sentence** from `## What the turn timeout does and does not cover` in `docs/work-on-session-lifecycle.md`. The literal string `a non-zero child exit all return an error` must not survive anywhere in the file. + +2. **Replace it with prose stating the new precedence.** The replacement must contain the literal single-line phrase `validated result outranks the exit code` (spec AC7 greps for it verbatim, so do not hyphenate it, split it across a line wrap, or reword it). The replacement must state, in real prose: + - Expiry and ctx cancellation still return an error and the caller persists nothing — those two are unchanged. + - A non-zero child exit is no longer a failure by itself. Once the child has exited, the captured result is read and validated, and the **validated result outranks the exit code**: when the blob validates the turn is a success and the id persists. + - Why the exit status is the weaker signal by construction: stderr goes to `os.DevNull`, so a non-zero exit arrives with no accompanying explanation, while the result blob is a structured document the code already knows how to validate. Trusting the opaque signal over the structured one was the inversion. + - Why the mirror-image lie matters: discarding a session that *can* be resumed costs the whole turn, while the false positive it was guarding against costs one failed `claude --resume`. + - The exit status remains the reason in exactly one case: the output is missing, unreadable, or not valid turn JSON, so there is no `result` text to surface. + - When the blob parses but fails a predicate, the error leads with the child's own `result` text and names the failed predicate (`claude reported is_error: true`, `claude returned num_turns: 0`, `claude returned empty session_id`) — quote the real strings from `pkg/ops/claude_session.go`, do not paraphrase them. + - The compensating clear is unchanged: it still fires on every error the detached turn returns. Only the definition of "failed" moved. + +3. **Make the no-read-on-timeout rule explicit** in the same section, as its own short paragraph: the read lives inside the child-exited branch and is never hoisted above the `select`. On the timeout and cancellation paths the child is still running, so any bytes in the output file are partial by definition and must never be validated as success. Say plainly that this is a regression lock, with a unit test behind it, not a stylistic preference — a future reader who "simplifies" the read out of its branch reintroduces the bug in a worse form. + +4. **Reconcile `## The fate of --output-format json`** with the new contract. Its closing line currently reads *"Stderr still goes to `os.DevNull`; a crash surfaces via exit code."* Keep the fact and correct the implication: stderr still goes to `os.DevNull`, which is precisely why the exit code carries no diagnostic content and is now the fallback signal rather than the primary one. Also update the sentence *"A turn whose result is `num_turns: 0`, `is_error: true`, or unparseable is an error, and no id is persisted."* so it says the validation verdict — not the process exit status — is what decides, and that the same shared `validateSessionTurn` still serves both branches. + +5. **Do not rewrite other sections.** `## Session id ownership`, `## Why the TTY branch is untouched`, `## Failure path`, `## Post-exit write ordering`, `## Why stream-json was rejected`, and `## The per-session lock` stay as they are, apart from any sentence that directly contradicts the new precedence. **One is known**: in `## The per-session lock`, the *detached-child safety property* paragraph (~lines 168-176) lists "child exit error" alongside ctx cancel and the 30m bound as cases where the parent stops waiting and the clear removes the id. Under the new precedence a non-zero child exit with a validating blob is a success, not a failure — correct that enumeration to name only the paths that still fail, and leave the rest of the paragraph (the layered safety argument, the lock, the goal-path ordering) untouched. If you find any other contradicting sentence, correct only that sentence and say which in the completion summary. + +6. **Add the `## Unreleased` section to `CHANGELOG.md`**, below the preamble block and above the newest `## vX.Y.Z` section (whatever it is when you run — do not assume a version number). If `## Unreleased` already exists, APPEND the bullet to it and do NOT create a second one. Never rename an existing released section. Under it, one `- fix:` bullet describing the precedence change. The bullet must contain the literal phrase `validated result outranks the exit code` (spec AC8 greps the topmost `## ` section for it). It should also name the user-visible payoff: a headless `work-on` turn that completed successfully is no longer discarded because its child process exited non-zero, so the session id persists and the Vault UI offers Resume; a genuinely failed turn still clears the id and now reports the child's own reason instead of `exit status 1`. + +7. **Do NOT bump version fields** in `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`, do NOT tag, and do NOT rename `## Unreleased` to a version. `.maintainer.yaml` sets `autoRelease: true` — the release bot owns version bumps and tags, and hand-bumping races it. + +8. **Do NOT modify** any file under `pkg/`, any `_test.go` file, or anything under `scenarios/`. This prompt is documentation and changelog only. + +9. **Self-check before finishing.** Re-run every command in `` and confirm it passes. Then quote the deleted sentence and its replacement side by side, and state which edit satisfies spec 045 AC7 and which satisfies AC8. + + + + +- Documentation and changelog only in this prompt: no behavior, signature, or test changes. +- The doc describes behavior that exists on this branch after prompts 1 and 2 — write it in the present tense as shipped fact, and quote the real error strings from `pkg/ops/claude_session.go` rather than inventing wording. +- `CHANGELOG.md` structure must satisfy `scripts/check-changelog.sh`, which `make precommit` runs. The final order is always `# Changelog` → preamble → `## Unreleased` → newest `## vX.Y.Z` (newest first). +- The phrase `validated result outranks the exit code` must appear on a single line in both `docs/work-on-session-lifecycle.md` and the topmost `## ` section of `CHANGELOG.md`. A line wrap in the middle of it defeats the AC greps even though the prose reads fine. +- Do NOT weaken or remove the documented compensating clear — spec Non-goal. A genuinely failed turn must still clear the id, and the doc must keep saying so. +- Do NOT document Vault UI banner styling — spec Non-goal. Only the message content changed. +- Do NOT speculate in the doc about why a clean-`end_turn` child exits 1 — spec Non-goal, and the cause is unreproduced. State only that vault-cli must not depend on the exit code being trustworthy. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +Run from the repo root: + +``` +make precommit +``` + +Must exit 0 (this runs `scripts/check-changelog.sh` and `scripts/check-versions.sh`). + +Then, each of these must hold: + +``` +! grep -q 'a non-zero child exit all return an error' docs/work-on-session-lifecycle.md +``` +must succeed — the stale contract sentence is gone. (Written as `! grep -q` on purpose: `grep -c` prints `0` but exits `1`, which would report a passing absence check as a failure.) + +``` +grep -c 'validated result outranks the exit code' docs/work-on-session-lifecycle.md +``` +must print `1` or more. + +``` +awk '/^## /{n++} n==1' CHANGELOG.md | grep -c 'validated result outranks the exit code' +``` +must print `1` or more — the phrase is inside the topmost `## ` section. Heading-independent on purpose: the release bot may rename `## Unreleased` to `## vX.Y.Z` between prompts. + +``` +test "$(grep -n -m1 '^## ' CHANGELOG.md | cut -d: -f1)" -gt "$(grep -n -m1 '^All notable changes to this project' CHANGELOG.md | cut -d: -f1)" +``` +must exit 0 — the first `## ` heading sits below the preamble, never above it. Assertive on +purpose: a bare `head -12 CHANGELOG.md | grep -n '^## '` exits 0 even when the heading is +misplaced above the preamble, so it could never fail the case it targets. + +``` +! grep -rq 'a non-zero child exit all return an error' docs/ +``` +must succeed. + +Nothing above distinguishes "the full rationale landed" from "one sentence containing the +magic phrase landed". The rewritten section must carry the argument, not just the string: + +``` +awk '/^## What the turn timeout does and does not cover$/,/^## Failure path$/' docs/work-on-session-lifecycle.md > /tmp/turn-section.txt +grep -q 'validated result outranks the exit code' /tmp/turn-section.txt +grep -q 'os.DevNull' /tmp/turn-section.txt +grep -qi 'hoist' /tmp/turn-section.txt +test "$(wc -l < /tmp/turn-section.txt)" -ge 30 +``` + +All must exit 0: the precedence statement, the why-the-exit-code-is-the-weaker-signal +argument (`os.DevNull`), and the no-hoist regression lock all live in this section, which +was 16 lines before the rewrite. + diff --git a/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md b/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md new file mode 100644 index 0000000..5df6e39 --- /dev/null +++ b/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md @@ -0,0 +1,179 @@ +--- +status: verifying +approved: "2026-09-06T13:07:28Z" +generating: "2026-09-06T13:43:00Z" +prompted: "2026-09-06T13:43:00Z" +verifying: "2026-09-06T14:00:37Z" +branch: dark-factory/bug-exit-code-outranks-validated-turn +--- + +## Summary + +- A headless `work-on` turn that completes successfully is discarded when its child process exits non-zero. +- `runDetachedTurn` already captures and validates the child's result JSON — but only on the clean-exit path; a non-zero exit returns before the read. +- The operator sees a ~40-line Go stack whose only content is `exit status 1`, and the task loses its `claude_session_id`. +- The completed work survives on disk as a transcript the Vault UI can no longer reach; recovery requires knowing the UUID by hand. +- The fix inverts the precedence: the validated turn result decides success, and the exit code is only consulted when there is no usable result. + +## Problem + +`vault-cli work-on` spawns a detached `claude --print` child, redirects its stdout to a temp file, and validates that blob through `validateSessionTurn` — a session id alone proves nothing, because `claude` reports one even for a turn that did no work. That validation is the thing that makes the persisted id trustworthy. But the exit-status branch in `runDetachedTurn` returns before the file is ever read, so a turn that produced a complete, valid result is thrown away on the strength of a number the child sets for unrelated reasons. The operator loses both the session and any explanation of why. + +## Reproduction + +Observed 2026-09-06 on `vault-cli v0.122.3`; the code is unchanged at `v0.124.1` (`5d2b1ed`). + +1. In Vault UI, click Start/Resume on a task with no `claude_session_id` (observed: `Diagnose Early Exit on Overnight US100 Demo Trade`, Personal vault). +2. `vault-cli work-on` mints a UUID, pre-persists it, and spawns the detached child. The argv it builds (`pkg/ops/claude_session.go:193-202`) is: + + ``` + --print -n "" \ + -p '/vault-cli:work-on-task "" --non-interactive' \ + --output-format json --session-id + ``` + + with stdout redirected to a temp file and stderr to `os.DevNull`. `--max-turns` is omitted because `maxTurns` is `-1`. +3. The child runs a full turn — roughly 2 minutes — and ends cleanly. +4. The child process exits 1. + +Observed evidence, verbatim from `~/Library/Logs/vault-ui.log`: + +``` +2026-09-06 12:59:23 INFO [vault_ui.api.tasks:1095] Starting vault-cli session for task Diagnose Early Exit on Overnight US100 Demo Trade +2026-09-06 13:01:15 ERROR [vault_ui.api.tasks:1154] Error creating session: vault-cli work-on failed: … error="claude session exited with error: exit status 1 +``` + +The child's own transcript, `~/.claude/projects/-Users-bborbe-Documents-Obsidian-Personal/5a1b9c38-076e-42e9-9222-adf03f5c08e4.jsonl` (87 lines): + +- `"stop_reason":"end_turn"` on the final assistant message +- zero records with `"is_error":true` +- `"permission_denials":[]` +- final assistant message written in full, followed by the `last-prompt` record + +Task frontmatter after the run: no `claude_session_id` key; `metrics_sessions` still lists only the prior day's session. + +Ruled out during triage: claude-code-router reachable; `claude` 2.1.260 works standalone; `-n` is a valid flag; `--max-turns` inert (`maxTurns: -1`, `claude_session.go:59`); a baseline `claude --print -n … --session-id … --output-format json` from the same cwd exits 0; a session-id collision produces a different, instant failure (`Error: Session ID … is already in use.`). + +### Deterministic repro without waiting for the wild failure + +The cause of the non-zero exit is a Non-goal and is unreproduced, so verification must not depend on it recurring. The repo already exposes the seam: `claude_script` (`pkg/config/config.go:35`, read via `GetClaudeScript()` at `pkg/cli/cli.go:386`, resolved through `exec.LookPath` at `pkg/ops/claude_session.go:53`) lets a vault point at any executable. + +Two stubs make both outcomes deterministic: + +- `stub-valid-exit1` — prints a valid blob (`session_id` set, `num_turns: 3`, `is_error: false`, `result: "done"`) to stdout, then `exit 1`. This is the bug. +- `stub-error-exit1` — prints `{"session_id":"…","num_turns":0,"is_error":true,"result":"seeded failure text"}`, then `exit 1`. This is a genuine failure and must still clear the id. + +Point a scratch vault's `claude_script` at each in turn and click Start. + +## Expected vs Actual + +**Expected** — the turn is validated by its result blob, the same contract `validateSessionTurn` enforces on the interactive branch and on the clean-exit path of the detached branch. `docs/work-on-session-lifecycle.md:80` states the reason validation exists: *"an unvalidated id would be handed to the operator as resumable when it is not."* + +**Actual** — `runDetachedTurn` (`pkg/ops/claude_session.go:258-260`) returns on `exitErr != nil` before reaching the `os.ReadFile` + `validateSessionTurn` pair at lines 276-280. A valid blob sitting in the temp file is never read; the id is cleared and the operator gets `exit status 1`. + +## Why this is a bug + +`docs/work-on-session-lifecycle.md:100` documents the current behavior as intended: *"Expiry, ctx cancellation, and a non-zero child exit all return an error, so the caller persists nothing."* So this is a contract defect, not code drifting from its spec — the documented rule is itself wrong, and the doc must change with the code. + +The doc's own justification is what condemns it. It defends validation on the grounds that offering a Resume that cannot work is a lie to the operator. Discarding a session that *can* be resumed is the mirror image of that lie, and it is the more expensive one: the false-positive costs a failed `claude --resume`, while this costs the entire turn. + +The exit code is also the weaker signal by construction. `claude`'s stderr goes to `os.DevNull`, so the exit status arrives with no accompanying explanation, while the result blob is a structured document the code already knows how to validate. Trusting the opaque signal over the structured one inverts the precedence. + +## Workaround + +Until the fix lands, a stranded session is recoverable by hand: + +```bash +ls -t ~/.claude/projects//*.jsonl | head -1 # newest transcript = the stranded session +vault-cli task set "" claude_session_id # restore the id the clear removed +``` + +The `` is the vault path with `/` replaced by `-`, e.g. `-Users-bborbe-Documents-Obsidian-Personal`. + +## Goal + +A headless turn is judged by what it produced, not by how its process happened to exit. When the captured result JSON validates, the session id persists and the Vault UI offers Resume. When it does not, the error names the child's own reason instead of an exit status, and the compensating clear runs exactly as it does today. + +## Non-goals + +- Root-causing why a clean-`end_turn` child exits 1 at all. The leading suspect is a non-async `Stop` hook (`afplay`) in a detached process with no audio session, but it is untested and irrelevant to this fix: vault-cli must not depend on the exit code being trustworthy. +- Any change to the interactive TTY branch's **control flow** — `cmd.Output()`, the synchronous validation, the 5m cap, the blocking wait. Its error message *text* does change, because AC 2 requires the child's reason to lead and Constraints forbid forking the shared `validateSessionTurn` (called at `claude_session.go:221` interactive and `:281` detached). Rewording the shared validator is the only implementation reachable under both rules; two interactive-branch assertions are updated accordingly. +- Removing or weakening the compensating clear. This spec narrows *when* a turn counts as failed; a genuinely failed turn must still clear the id. +- Vault UI banner styling. Only the message content changes. + +## Acceptance Criteria + +- [ ] A child that writes a valid result blob (`session_id` non-empty, `num_turns > 0`, `is_error: false`) and exits non-zero causes `runDetachedTurn` to return `nil` — evidence: unit test asserts `err == nil` for that input pair; `make precommit` exits 0. +- [ ] A child that writes a **non-empty blob that parses but fails a predicate** (`is_error: true`, `num_turns: 0`, or empty `session_id`) returns an error that leads with the blob's `result` text and names the failed predicate — evidence: unit test asserts `err.Error()` has the seeded `result` string as a prefix AND contains one of `is_error` / `num_turns` / `session_id`. An exit-status mention may follow as a trailing clause, but must not precede the child's own reason. +- [ ] A child that exits non-zero AND leaves a **zero-length, unreadable, or non-empty-but-unparseable** output file returns an error naming the exit status — evidence: unit test asserts the message contains `exit status`. This is the only path where the exit code is authoritative; unparseable bytes land here rather than under the predicate case, because there is no `result` text to surface. +- [ ] A turn that hits `sessionTurnTimeout` while a valid blob is already present in the output file still returns an error — evidence: unit test seeds a valid blob, fires the timeout path, asserts `err != nil` and the message contains `did not complete within`. This locks DB 5 against the hoist-the-read refactor. +- [ ] Negative — the compensating clear still fires for a genuinely failed turn, on **both** the task path and the goal path: after `Execute` against a child seeded with `is_error: true`, `grep -c '^claude_session_id:' ` returns 0 — evidence: `workon_session_writeback_test.go` and `goal_workon_test.go` each extended, asserting zero matches. +- [ ] Negative — a task or goal whose turn validates retains its id: after `Execute` against a valid-blob/non-zero-exit child, `grep -c '^claude_session_id:' ` returns 1 — evidence: same two test files, opposite assertion. `goal_workon.go:198` has its own `handleClaudeSession` and must not be left behind. +- [ ] The stale contract sentence is gone from the lifecycle doc AND replaced by one stating result-over-exit-code precedence — evidence: `grep -c 'a non-zero child exit all return an error' docs/work-on-session-lifecycle.md` returns 0, AND `grep -c 'validated result outranks the exit code' docs/work-on-session-lifecycle.md` returns ≥1. +- [ ] `CHANGELOG.md`'s topmost section carries a bullet naming the precedence change — evidence: `awk '/^## /{n++} n==1' CHANGELOG.md | grep -c 'validated result outranks the exit code'` returns ≥1. Heading-independent on purpose: `.maintainer.yaml` sets `autoRelease: true` and `.dark-factory.yaml` sets `pr: false`, so the release bot can rename `## Unreleased` to `## vX.Y.Z` between prompts and a heading-pinned grep would fail a correct implementation. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +- `make precommit` — lint, format, generate, test, checks; exits 0 +- `grep -c 'a non-zero child exit all return an error' docs/work-on-session-lifecycle.md` — returns 0 (stale contract sentence deleted) +- `grep -c 'validated result outranks the exit code' docs/work-on-session-lifecycle.md` — returns ≥1 (replacement sentence present) +- `grep -n 'claude session exited with error' pkg/ops/claude_session.go` — returns exactly one line +- `awk '/^## /{n++} n==1' CHANGELOG.md | grep -c 'validated result outranks the exit code'` — returns ≥1 + +### Operator-executable (runs on the host after PR merge) + +These two bullets are deliberately **not** Acceptance Criteria: they observe the Vault UI, which no container-executable check can reach, and the AC set is intentionally all container-executable so `spec-verifier` gates on evidence the pipeline can produce. They remain the operator's proof of the user-visible payoff. Both use the `claude_script` stubs from the Reproduction section, so neither waits on the wild failure: + +- Scratch vault pointed at `stub-valid-exit1`, Vault UI Start: card reaches `▶ Resume`, no red banner, and `grep -c '^claude_session_id:' ""` returns 1 +- Scratch vault pointed at `stub-error-exit1`, Vault UI Start: banner's first line contains `seeded failure text` and not `exit status`, and `grep -c '^claude_session_id:' ""` returns 0 + +## Desired Behavior + +1. `runDetachedTurn` reads the captured output file on every path where the child has actually exited, before the file is unlinked. +2. When the blob validates, the turn is a success — the non-zero exit is not surfaced as an error. +3. When the blob is present but invalid, the returned error carries the blob's `result` text and names the failed predicate, so the operator learns the child's own reason. +4. When the blob is absent or unreadable and the child exited non-zero, the error names the exit status — the sole remaining case where the exit code decides. +5. The timeout and ctx-cancellation paths are unchanged and must NOT read the blob: the child is still running, so any bytes present are partial by definition and must never be validated as success. +6. `handleClaudeSession`'s compensating clear continues to fire on every error `runDetachedTurn` returns — the behavior change lives entirely in what counts as an error. + +## Constraints + +- `validateSessionTurn` stays the single shared validator for both branches; do not fork its logic. +- The read must stay inside the child-exited branch. Hoisting it above the `select` is the specific refactor DB 5 and its AC forbid. +- The temp file must still be unlinked on every return path, including cancel and timeout where the child holds the fd. +- Stderr still goes to `os.DevNull`. +- The interactive branch's `cmd.Output()` + `validateSessionTurn` sequence must not change. +- Tests use Ginkgo v2 + Gomega with Counterfeiter mocks; no stdlib `t.Run` table tests. +- Errors wrap via `github.com/bborbe/errors` with a real `ctx` — no `fmt.Errorf`, no bare `return err`. +- The existing `exit status 1` assertions must keep passing unchanged: `claude_session_test.go:433`, `goal_workon_test.go:537`, `workon_session_writeback_test.go:339`. Their `detachRun` stubs ignore the `*os.File` parameter, so they exercise the zero-length-output path and encode the AC-3 contract. If a change makes one of them fail, the empty-file case has been misrouted to the predicate branch. + +## Failure Modes + +| Trigger | Expected behavior | Detection | Recovery | +|---|---|---|---| +| Child exits non-zero, blob valid | Success; id persists | Vault UI shows `▶ Resume` | None needed | +| Child exits non-zero, blob non-empty and parses but fails a predicate | Error leading with the blob's `result` and failed predicate; id cleared | Banner first line is the child's reason | Operator fixes the named cause, re-runs Start, confirms `▶ Resume` | +| Child exits non-zero, output file zero-length, unreadable, or unparseable | Error naming the exit status; id cleared | Banner says `exit status N` | Run the Workaround commands: newest transcript under `~/.claude/projects//`, then `vault-cli task set … claude_session_id `; confirm `grep -c '^claude_session_id:'` returns 1 | +| Output file unreadable (fd/permission failure) after child exit | Error wrapping the read failure; id cleared | Banner names the read error, not the exit status | Re-run Start; if it recurs, `ls -ld $TMPDIR` and confirm the temp dir is writable | +| `claude` changes its `--output-format json` shape | `validateSessionTurn` rejects every turn; no id ever persists | Every Start banner reports a parse or predicate failure with the same text | Pin the parser to the fields it needs and add the new shape; under the new precedence this validator is the only remaining gate, so a drift here fails closed, not silently | +| Turn timeout (30m) expires with child still running | Unchanged — error, id cleared, blob NOT read | Banner names the timeout | Unchanged; the detached child keeps running and its transcript is recoverable via the Workaround | +| ctx cancelled mid-turn | Unchanged — error, id cleared, blob NOT read | Banner names the cancellation | Unchanged | +| Two Start clicks race the same task | Unchanged — the per-session lock refuses the second before any child spawns | Second click errors immediately | Wait for the first turn to finish | + +## Suggested Decomposition + +Prompts should be generated in this order — each row is a single prompt with a clear scope. + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | `runDetachedTurn` precedence inversion + unit tests for the four exit/blob combinations | 1, 2, 3, 4, 5 | 1, 2, 3, 4 | — | +| 2 | `workon` / `goal_workon` writeback tests for clear-vs-retain | 6 | 5, 6 | prompt 1 | +| 3 | Lifecycle doc contract rewrite + CHANGELOG bullet | — | 7, 8 | prompt 1 | + +Rationale: prompt 1 carries the whole behavior change and its direct tests, including the timeout guard that prevents the plausible regression; prompt 2 proves the caller-side consequence on real task files; prompt 3 is text-only and depends on prompt 1 only so the doc describes what actually shipped. + +## Do-Nothing Option + +Every Vault UI Start whose child trips this exit-code path keeps costing a complete agentic turn plus the operator time to work out that nothing is actually wrong. The error surface stays a Go stack with no diagnostic content, so each occurrence is re-investigated from scratch — this one cost roughly forty minutes. The stranded transcripts are recoverable only by an operator who knows the Workaround exists, which means in practice they are lost. Doing nothing also leaves the exit code trusted over a validated result, so the next unrelated cause of a non-zero exit produces the same silent loss.