fix: validated turn result outranks child exit code in headless work-on

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).

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

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)
+}