fix: failed work-on turn surfaces the child's reason as the first line

[bborbe]

Completes spec 045 SC5: a failed headless work-on turn must surface the child's own reason as the banner's first line.

The gap

PR #140 made the child's reason present. It did not make it lead. Verified live in Vault UI with a seeded failing turn: the session id cleared correctly and the reason was there — as the last line, under a slog WARN and two full Go stack traces.

vault-ui builds its red banner from vault-cli's stderr, so anything vault-cli logs lands in the banner ahead of the error itself.

Cause

• sessionFailureResult (and goal_workon.go's equivalent) logged sessionErr at Warn — an error they already return to the caller, which the CLI then prints. The duplicate carried a full bborbe/errors stack, which was the bulk of the noise.
• The turn rejected by predicate signal added in fix: validated turn result outranks child exit code in headless work-on #140 was at Warn, so it reached stderr first and became the banner's opening line.

Change

• Drop both redundant workon session error logs; the error is returned and printed. Comments left at each site so it is not "fixed" back.
• Demote the predicate signal to Debug — it is secondary information about an error already being surfaced.

Measured

Seeded failing turn (is_error: true, num_turns: 0, child exits 1), stderr:

	before	after
lines	~45	2
first line	level=WARN msg="turn rejected by predicate…"	Error: start work-on session: start claude session: seeded failure text (claude returned num_turns: 0)

Success path re-checked and unchanged: valid blob + non-zero exit still exits 0 and retains claude_session_id.

make precommit green.

[ben-s-pull-request-reviewer]

The diff is small and surgical. I've now read all the relevant changed code. Let me assess the four concerns:

Concern	Verdict
claude_session.go: removed Warn log for sessionErr — error still returned?	Not an issue. Line 297 returns validateErr unwrapped. The demotion to Debug is intentional (secondary signal, already in returned error).
goal_workon.go: similar log removal — error return path complete?	Not an issue. Line 119-120 returns the MutationResult with Error: sessionErr.Error() and the wrapped error.
workon.go: similar log removal — error propagation unaffected?	Not an issue. sessionFailureResult (line 176) returns the MutationResult with Error: sessionErr.Error(). The caller (Execute) wraps it with errors.Wrap.
workon.go / claude_session.go: predicate signal demoted to Debug — correct?	Not an issue. The primary error is returned (line 297: return validateErr). The slog.Debug at line 294 is explicitly a secondary signal about a co-occurring non-zero exit. The PR comment justifies the demotion correctly.

The changes are correct: the errors are returned (not dropped), and the redundant stderr copies are removed so vault-ui's banner leads with the child's own reason instead of ~40 lines of stack trace.

No new issues found. The mechanical-funnel findings about //counterfeiter:generate and go-composition are pre-existing in these files and outside this PR's scope.

{
  "verdict": "approve",
  "summary": "Bugfix is correct and minimal. Three duplicate slog.Warn calls that sent bborbe/errors stacks to stderr are removed; the errors remain fully returned to callers. The predicate-rejection signal in claude_session.go is demoted from Warn to Debug as a secondary signal. No error path is broken.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: removed Warn log in claude_session.go — verify error is still fully returned to caller and printed by CLI",
      "disposition": "not-an-issue",
      "detail": "claude_session.go:297 returns validateErr unwrapped; the Debug demotion is only for the secondary non-zero-exit co-signal, not the primary error."
    },
    {
      "concern": "correctness: removed Warn log in goal_workon.go — ensure error return path is complete without the duplicate log",
      "disposition": "not-an-issue",
      "detail": "goal_workon.go:119-120 returns MutationResult with Error:sessionErr.Error() and errors.Wrap(ctx, sessionErr, 'start work-on session') — error path is intact."
    },
    {
      "concern": "correctness: removed Warn log in workon.go — verify error propagation is unaffected",
      "disposition": "not-an-issue",
      "detail": "workon.go sessionFailureResult (line 176) returns MutationResult with Error:sessionErr.Error(); caller wraps with errors.Wrap — propagation is intact."
    },
    {
      "concern": "correctness: predicate signal demoted to Debug — confirm this secondary signal does not need to be visible at Warn in production",
      "disposition": "not-an-issue",
      "detail": "claude_session.go:294 slog.Debug replaces the former slog.Warn; the primary error (validateErr) is still returned at line 297; the demotion is justified by the PR comment (spec 045 SC5) and matches the workon.go sessionFailureResult change."
    }
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

I've reviewed the diff thoroughly. The changes are minimal and focused. Let me verify the specific concern about the slog.Warn → slog.Debug demotion in claude_session.go.

The diff shows:

• claude_session.go:291-294: slog.Warn demoted to slog.Debug with clear comment explaining the rationale
• workon.go:172-175: Removed redundant slog.Warn with explanatory comment
• goal_workon.go:116-118: Removed redundant slog.Warn with explanatory comment
• CHANGELOG.md: Documents the fix referencing spec 045 SC5

These are all correct, targeted changes addressing the banner issue. The pre-existing architectural findings (counterfeiter directives, raw goroutines) are not introduced by this PR.

Step 0d: Cleanup — no worktree was created, nothing to remove.

Step 5: Consolidated Report

---

Must Fix (Critical): None.

Should Fix (Important): None.

Nice to Have (Optional): None.

---

The fix is correct and minimal:

• pkg/ops/claude_session.go:294 — slog.Warn → slog.Debug is correct: the "turn rejected" signal is secondary to the already-returned error; at Warn level it was appearing ahead of the child's reason in vault-ui's banner.
• pkg/ops/workon.go:172 and pkg/ops/goal_workon.go:116-118 — removing the redundant slog.Warn is correct: the error is already returned to the caller and printed by the CLI; a second copy with a bborbe/errors stack on stderr was burying the child's reason under ~40 lines of trace.

No test suite changes, but this is a bugfix touching only error-display plumbing — the change is verifiable by inspection.

Verdict: approve

{
  "verdict": "approve",
  "summary": "Bugfix correctly removes duplicate error logging that was causing vault-ui to display ~40 lines of error stack before the child's own reason in the banner. Changes are minimal, targeted, and each has a clear comment explaining the rationale. No new issues introduced.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: slog.Warn demoted to slog.Debug for 'turn rejected by predicate' — verify Debug-level logging is sufficient for this secondary signal in production observability",
      "disposition": "addressed",
      "detail": "claude_session.go:294 — the log is secondary signal; the primary error is already returned and printed. Demoting to Debug is correct and documented with the spec 045 SC5 rationale."
    },
    {
      "concern": "correctness: redundant error log removed from sessionFailureResult — verify CLI still prints the returned error correctly and the call chain is intact",
      "disposition": "addressed",
      "detail": "workon.go:172 — sessionErr is returned to caller and printed by the CLI. Verified the return value is unchanged; the MutationResult.Error field carries sessionErr.Error() to the caller."
    },
    {
      "concern": "correctness: redundant error log removed — verify MutationResult + error return path is unchanged and error message leads in output",
      "disposition": "addressed",
      "detail": "goal_workon.go:116-118 — same pattern as workon.go. MutationResult returned with Error field set; errors.Wrap returned as second return value. No change to the return path itself."
    }
  ]
}