From 7b49fcd668df0faac81416dad5422604aca10337 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 6 Sep 2026 16:00:37 +0200 Subject: [PATCH 1/5] Result outranks exit code in the detached turn (spec 045, prompt 1 of 3) --- CHANGELOG.md | 4 + pkg/ops/claude_session.go | 75 +++++-- pkg/ops/claude_session_test.go | 161 +++++++++++++- .../2-spec-045-workon-writeback-precedence.md | 147 +++++++++++++ prompts/3-spec-045-docs-and-changelog.md | 131 +++++++++++ .../208-spec-045-result-outranks-exit-code.md | 206 ++++++++++++++++++ ...041-bug-resume-races-live-headless-turn.md | 2 +- ...5-bug-exit-code-outranks-validated-turn.md | 178 +++++++++++++++ 8 files changed, 881 insertions(+), 23 deletions(-) create mode 100644 prompts/2-spec-045-workon-writeback-precedence.md create mode 100644 prompts/3-spec-045-docs-and-changelog.md create mode 100644 prompts/completed/208-spec-045-result-outranks-exit-code.md create mode 100644 specs/in-progress/045-bug-exit-code-outranks-validated-turn.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 855ac33..e857781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ 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. + ## v0.125.0 - feat: `verify-goal` status-consistency check gains the inverse direction — a subtask may not outrank its goal. If a goal is not `in_progress` (`next`/`backlog`/`hold`/`aborted`) no subtask may be `in_progress`; if a goal is `backlog` no subtask may be `next` or `in_progress`. Report-only, matching the goal-necessity check; never modifies goal or task files. diff --git a/pkg/ops/claude_session.go b/pkg/ops/claude_session.go index 502bf77..eb4ce22 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,32 @@ 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. Return it unwrapped so the + // child's own result text leads the message (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 +299,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 +320,32 @@ 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") + 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/prompts/2-spec-045-workon-writeback-precedence.md b/prompts/2-spec-045-workon-writeback-precedence.md new file mode 100644 index 0000000..6b13272 --- /dev/null +++ b/prompts/2-spec-045-workon-writeback-precedence.md @@ -0,0 +1,147 @@ +--- +spec: ["045-bug-exit-code-outranks-validated-turn"] +status: draft +created: "2026-09-06T13:20:00Z" +--- + +# 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/3-spec-045-docs-and-changelog.md b/prompts/3-spec-045-docs-and-changelog.md new file mode 100644 index 0000000..e141263 --- /dev/null +++ b/prompts/3-spec-045-docs-and-changelog.md @@ -0,0 +1,131 @@ +--- +spec: ["045-bug-exit-code-outranks-validated-turn"] +status: draft +created: "2026-09-06T13:20:00Z" +--- + +# 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/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/specs/in-progress/041-bug-resume-races-live-headless-turn.md b/specs/in-progress/041-bug-resume-races-live-headless-turn.md index 2f25f38..6789c74 100644 --- a/specs/in-progress/041-bug-resume-races-live-headless-turn.md +++ b/specs/in-progress/041-bug-resume-races-live-headless-turn.md @@ -1,5 +1,5 @@ --- -status: approved +status: generating approved: "2026-08-28T08:35:16Z" generating: "2026-08-30T17:05:06Z" branch: dark-factory/bug-resume-races-live-headless-turn 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..f1490b5 --- /dev/null +++ b/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md @@ -0,0 +1,178 @@ +--- +status: prompted +approved: "2026-09-06T13:07:28Z" +generating: "2026-09-06T13:43:00Z" +prompted: "2026-09-06T13:43:00Z" +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. From 1d0b1a123699e4bb50b50fb083df81bb57956438 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 6 Sep 2026 16:14:38 +0200 Subject: [PATCH 2/5] Task and goal write-back under the new precedence (spec 045, prompt 2 of 3) --- CHANGELOG.md | 1 + pkg/ops/goal_workon_test.go | 178 ++++++++++++++++++ pkg/ops/workon_session_writeback_test.go | 156 +++++++++++++++ prompts/1-spec-041-session-turn-block.md | 101 +++++----- prompts/2-spec-041-post-exit-persist.md | 51 +++-- prompts/3-spec-041-docs-changelog.md | 47 ++--- ...9-spec-045-workon-writeback-precedence.md} | 10 +- .../210-spec-045-docs-and-changelog.md} | 5 +- ...041-bug-resume-races-live-headless-turn.md | 2 +- ...5-bug-exit-code-outranks-validated-turn.md | 3 +- 10 files changed, 443 insertions(+), 111 deletions(-) rename prompts/{2-spec-045-workon-writeback-precedence.md => completed/209-spec-045-workon-writeback-precedence.md} (95%) rename prompts/{3-spec-045-docs-and-changelog.md => in-progress/210-spec-045-docs-and-changelog.md} (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index e857781..3e577cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## 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. ## v0.125.0 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/1-spec-041-session-turn-block.md b/prompts/1-spec-041-session-turn-block.md index 5921a18..2d6f4d2 100644 --- a/prompts/1-spec-041-session-turn-block.md +++ b/prompts/1-spec-041-session-turn-block.md @@ -1,18 +1,17 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-03T10:00:00Z" +created: "2026-09-06T16:20:00Z" --- -- Confirms the non-interactive branch of `StartSession` already blocks until the detached headless turn exits, instead of returning after ~10s while the child keeps writing — the spec-041 design is present in the tree and NOT reverted. The task-side reversion (v0.118.3, commit dae6563) only touched `workon.go` and the docs; this file, `export_test.go`, and the session tests were never reverted. -- Confirms the turn's `--output-format json` blob is validated on both branches through the shared `validateSessionTurn` helper, so a zero-turn, errored, or unparseable result is an error and persists nothing. -- Confirms child exit error, 30-minute bound expiry, and context cancellation all return an error so the caller persists no session id, and that the detached child survives parent timeout and cancellation (a wait bound, never a kill). -- Confirms the interactive TTY branch, `defaultCommandRunner`, and the 5-minute cap are unchanged (AC10 guards), and that `mocks/claude-session-starter.go` is untouched. -- Backfill 1: renames the turn-bound test variable `window` to `capturedWindow` in `pkg/ops/claude_session_test.go` so the spec's AC1 evidence grep (`Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`) matches; the assertion already exists under the other name, so this is a pure rename with no behavior change. -- Backfill 2: adds the one genuinely missing test — the temp output file is removed after a clean exit (the AC2 "temp file is removed" half). -- Flags a spec artifact for the reviewer: the spec's AC4/AC5 evidence greps carry a literal trailing double-quote (`'"claude session exited with error"'`, `'"did not complete within"'`) that can NEVER match the real source strings (`"claude session exited with error: %v"`, `"claude session turn did not complete within %v"`). This prompt verifies the unquoted forms instead and forbids editing error strings to force the quoted greps. -- Runs `make test` and the AC10 `git diff --exit-code HEAD` guard for `scenarios/005` (git is available in this container — `.dark-factory.yaml` is `workflow: direct`, no hideGit). +- Confirms the non-interactive branch of `StartSession` already blocks until the detached headless turn exits (a wait-channel select bounded by `sessionTurnTimeout`, never a kill) and validates the turn's JSON on BOTH branches through the shared `validateSessionTurn` helper — the spec-041 session-side is shipped in the tree and NOT reverted. Do not rewrite what is already correct. +- Notes that spec 045's precedence inversion already landed in the same file: a validated turn result outranks a non-zero child exit, `validateSessionTurn`'s rejection messages were reworded (they now lead with the child's `result` text via the `rejectTurn` helper, plus the `errClaudeOutputUnparseable` sentinel), and the detached branch reads + validates the blob before consulting the exit status. This prompt confirms the 041 invariants that SURVIVE that shape and does NOT re-touch the exit-status-vs-result routing (spec 045 owns it). +- Confirms the 30-minute `sessionTurnTimeout` constant, the caller-owned temp-file capture with eager unlink, the `defaultDetachedRunner` signature, `export_test.go`'s `SessionTurnTimeout` accessor, and the AC10 interactive-branch guards (`defaultCommandRunner` == 3, `context.WithTimeout` == 1). +- Backfill 1: renames the turn-bound test variable `window` to `capturedWindow` in `pkg/ops/claude_session_test.go` so the spec's AC1 evidence grep (`Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`) matches. The assertion already exists under the other name — this is a pure rename, no behavior change. +- Backfill 2: adds the one genuinely missing test — the temp output file is removed after a clean exit (the AC2 "temp file is removed" half). No test currently asserts it. +- Flags a spec artifact: the spec's AC3 evidence grep `'"0 turns"'` cannot match the post-045 validator string (`"claude returned num_turns: 0"`). This prompt verifies the post-045 strings and forbids editing the validator to force the stale grep. +- Runs `make test` + the grep gate. `make precommit` is the batch's full-gate check (AC13) in prompt 3. @@ -23,22 +22,23 @@ Confirm — and backfill where anything is missing — that the non-interactive Read CLAUDE.md for project conventions. Read fully (in this order): -- `pkg/ops/claude_session.go` — the whole file. This is the file under test. +- `pkg/ops/claude_session.go` — the whole file. This is the file under test. It already contains BOTH the spec-041 session-side (block-until-exit + temp-file capture + shared validation) AND spec 045's precedence inversion (the read+validate happens before the exit-status check inside the child-exit select branch). Do not undo either. - `pkg/ops/export_test.go` — exposes the unexported constant. -- `pkg/ops/claude_session_test.go` — the whole file; the "non-interactive branch" context starts at line 256. +- `pkg/ops/claude_session_test.go` — the whole file. The "non-interactive branch" context starts at line 256. The "session lock lifecycle" context (spec 042) starts at line 625 and must stay untouched. - `pkg/ops/claude_session_detach_test.go` — the detachment integration test. -- `docs/work-on-session-lifecycle.md` — the durable design record this implementation realizes (note: its task-path sections were reverted in v0.118.3 — that is prompt 3's job to fix; do NOT edit the doc in this prompt). +- `docs/work-on-session-lifecycle.md` — the durable design record this implementation realizes. Note its task-path sections still describe the reverted pre-spawn design — that is prompt 3's job to fix; do NOT edit the doc in this prompt. +- `pkg/ops/session_lock.go` — `SessionLocker` / `NewSessionLockerWithDir` / `ErrSessionBusy` / `SessionLock.Release()` (spec 042 wiring already in the file). Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrapf(ctx, ...)` / `errors.Wrap(ctx, ...)` / `errors.Errorf(ctx, ...)` idiom from `github.com/bborbe/errors`; never `fmt.Errorf`, never bare `return err`, never `context.Background()` in `pkg/`. - `/home/node/.claude/plugins/marketplaces/coding/docs/go-concurrency-patterns.md` — why the raw `go func`s in this file are deliberate (documented inline). - `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2/Gomega conventions. -NOTE: git IS available in this container (`.dark-factory.yaml` is `workflow: direct`, no hideGit). The AC10 `git diff --exit-code HEAD -- scenarios/005-...` guard runs here — it is NOT operator-side. +IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked, a character device). Do NOT run any `git` command — not to diff, not to check scenario 005. The AC10 `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` guard is an OPERATOR-side check (spec Verification ladder); the container-side proxies are the `defaultCommandRunner` / `context.WithTimeout` greps below. -The target state for this prompt already exists in the tree (shipped as commit 247a789; the v0.118.3 task-side reversion did NOT touch `claude_session.go`, `export_test.go`, or the session tests). Your job is to CONFIRM each piece matches the spec-041 Design below, and BACKFILL the two specific gaps named in requirements 8 and 9. Do not rewrite what is already correct — "confirm" means read the actual source and verify it matches; correct only genuine mismatches, which are not expected. +The target state for this prompt already exists in the tree (the spec-041 session-side shipped; the v0.118.3 task-side reversion did NOT touch `claude_session.go`, `export_test.go`, or the session tests, and spec 045 has since refined the detached branch). Your job is to CONFIRM each piece matches the spec-041 invariants below, and BACKFILL the two specific gaps named in requirements 8 and 9. Do not rewrite what is already correct — "confirm" means read the actual source and verify it matches; correct only genuine mismatches, which are not expected. 1. **Confirm the constant.** In `pkg/ops/claude_session.go` the unexported constant must be: ```go @@ -50,50 +50,40 @@ The target state for this prompt already exists in the tree (shipped as commit 2 ```go func defaultDetachedRunner(args []string, dir string, stdout *os.File) (<-chan error, error) ``` - It must use `exec.Command` (NOT `exec.CommandContext`), set `cmd.Stdout = stdout` (the caller-owned temp file — the function must NOT close it), set `cmd.Stderr` to an `os.OpenFile(os.DevNull, os.O_WRONLY, 0)` handle (closed only after the child exits, inside the reaper goroutine), set `cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}`, log the spawn audit line (`slog.Info("claude detached spawn started", ...)` with pid), and return a buffered `done` channel (capacity 1) that receives `cmd.Wait()`'s error. `exec.CommandContext` must not appear in this function. If any piece differs, correct it to match; never close the caller-owned stdout file. + It must use `exec.Command` (NOT `exec.CommandContext`), set `cmd.Stdout = stdout` (the caller-owned temp file — the function must NOT close it), set `cmd.Stderr` to an `os.OpenFile(os.DevNull, os.O_WRONLY, 0)` handle (closed only after the child exits, inside the reaper goroutine), set `cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}`, log the spawn audit line (`slog.Info("claude detached spawn started", ...)` with pid), and return a buffered `done` channel (capacity 1) that receives `cmd.Wait()`'s error. If any piece differs, correct it to match; never close the caller-owned stdout file. -3. **Confirm the non-interactive branch.** `StartSession`'s non-interactive path (the `if !isInteractive` branch) must delegate to `runDetachedTurn(ctx, args, cwd)` — a method `func (c *claudeSessionStarter) runDetachedTurn(ctx context.Context, args []string, cwd string) error` — that does, in order: +3. **Confirm `runDetachedTurn`'s spec-041 invariants (do NOT re-touch spec-045's precedence routing).** The method `func (c *claudeSessionStarter) runDetachedTurn(ctx context.Context, args []string, cwd string) error` must have ALL of these, in order: - `outFile, err := os.CreateTemp("", "vault-claude-session-*.json")`; on error wrap with `"create claude output file"`. - Eager unlink + close via `defer` (`_ = os.Remove(outFile.Name())`, `_ = outFile.Close()`) so no temp file survives any return path, including cancel/timeout while the child still holds the fd. - `done, err := c.detachRun(args, cwd, outFile)`; on error wrap with `"start detached claude session"`. - A waiter goroutine: `waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout)`. - - A `select` with exactly these outcomes, ALL of which except a clean exit are errors: - - `case exitErr := <-done:` — non-nil → `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`; nil → fall through to read + validate. - - `case err := <-waitCh:` — `err != nil` (ctx cancelled) → `errors.Wrap(ctx, err, "claude session wait cancelled")`; `err == nil` (bound expired) → `errors.Errorf(ctx, "claude session turn did not complete within %v", c.sessionTurnTimeout)`. The child is detached and survives either way. - - After a clean exit: `os.ReadFile(outFile.Name())` (wrap with `"read claude output"`) then `validateSessionTurn(ctx, output)`. - - The old strings `"claude session start timed out"` and `"exited during startup"` must not exist anywhere in the repo. - If the branch differs, rewrite it to the contract above. Do NOT use `exec.CommandContext` here — the child must survive the parent. - -4. **Confirm `validateSessionTurn` extraction.** A helper - ```go - func validateSessionTurn(ctx context.Context, output []byte) error - ``` - must exist and be called from BOTH branches (the interactive branch via `c.runCmd` output, the non-interactive branch from the read temp file). Its checks and error strings must be byte-identical to these: - - `json.Unmarshal` failure → `errors.Wrap(ctx, err, "parse claude output")` - - empty `session_id` → `errors.Errorf(ctx, "claude returned empty session_id")` - - `num_turns == 0` → `errors.Errorf(ctx, "claude returned 0 turns: %s", result.Result)` - - `is_error == true` → `errors.Errorf(ctx, "claude reported error: %s", result.Result)` - - otherwise nil. It must not return nil on a dead session: a `session_id` alone proves nothing. - The interactive branch must otherwise be byte-identical to today: `defaultCommandRunner` unchanged, the 5m `context.WithTimeout(ctx, 5*time.Minute)` cap, `"claude bootstrap turn timed out after 5m"`, and `"run claude"` wrap. + - A `select` with these outcomes: + - `case exitErr := <-done:` — the child has exited. It MUST read `outFile` and run `validateSessionTurn` before deciding success. The exit-status-vs-result precedence inside this branch is spec 045's — do NOT change it (the current shape reads + validates, treats a validated result as success even on a non-zero exit, falls back to the exit status only for unparseable output, and logs a `slog.Warn("validated turn result overrides non-zero child exit", ...)` when a validated result wins over a non-zero exit). + - `case err := <-waitCh:` — both outcomes are errors so the caller persists no session id. `err != nil` (ctx cancelled) → `errors.Wrap(ctx, err, "claude session wait cancelled")`; `err == nil` (bound expired) → `errors.Errorf(ctx, "claude session turn did not complete within %v", c.sessionTurnTimeout)`. The child is detached and survives either way. + - The old string `"claude session start timed out"` must not exist anywhere in the repo. `"exited during startup"` must not exist either. + The 041-invariant properties this confirms: StartSession never returns early with a still-running child, every failure path returns an error (never nil on ctx-cancel or bound expiry), and the bound is a wait, never a kill (`exec.Command`, not `CommandContext`). + +4. **Confirm `validateSessionTurn` extraction and both-branch use.** A helper `func validateSessionTurn(ctx context.Context, output []byte) error` must exist and be called from BOTH branches (the interactive branch via `c.runCmd` output at the end of `StartSession`, the non-interactive branch from the read temp file in `runDetachedTurn`). Its checks must be `num_turns > 0` AND `is_error == false` AND `session_id` non-empty. NOTE: spec 045 already reworded its error strings (the current implementation uses `errors.Wrapf(ctx, errClaudeOutputUnparseable, "parse claude output: %v", err)` for unparseable output and a `rejectTurn(ctx, resultText, reason)` helper that produces ` ()` messages like `claude returned num_turns: 0`, `claude reported is_error: true`, `claude returned empty session_id`). These post-045 strings are CORRECT — do NOT reword them back to the pre-045 strings the spec text quotes, and do NOT force the spec's stale `"0 turns"` evidence grep (see ``). The interactive branch must otherwise be byte-identical to today: `defaultCommandRunner` unchanged, the 5m `context.WithTimeout(ctx, 5*time.Minute)` cap, `"claude bootstrap turn timed out after 5m"`, and `"run claude"` wrap. 5. **Confirm `export_test.go`.** It must contain ```go const SessionTurnTimeout = sessionTurnTimeout ``` - with a comment noting it is a test-only alias (locks wiring, not value — tests must also assert the literal `30 * libtime.Minute`). The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — that is expected and must be left untouched. + with a comment noting it is a test-only alias (locks wiring, not value — tests must also assert the literal `30 * libtime.Minute`). The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — leave it untouched. + +6. **Confirm the AC10 guards.** `grep -c 'defaultCommandRunner' pkg/ops/claude_session.go` must be 3 (the two constructors assign `runCmd: defaultCommandRunner` and the func is defined once — pinned so a rework cannot silently drop it) and `grep -c 'context.WithTimeout' pkg/ops/claude_session.go` must be 1 (on the interactive branch only). `mocks/claude-session-starter.go` must be untouched (the `StartSession` signature is unchanged). -6. **Confirm the test matrix exists.** In `pkg/ops/claude_session_test.go` the "non-interactive branch" context (starts at line 256) must contain specs that cover: - - AC1 — "blocks until the detached child exits" (line 327): blocking waiter, `doneCh` only fires after a `Consistently(returned, "100ms").ShouldNot(Receive())`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound via `windowCh` and it is asserted to equal `ops.SessionTurnTimeout` AND `30 * libtime.Minute`. +7. **Confirm the test matrix covers AC1-6 and the detachment integration test.** In `pkg/ops/claude_session_test.go` the "non-interactive branch" context (starts at line 256) must contain specs that cover: + - AC1 — "blocks until the detached child exits": blocking waiter, `doneCh` only fires after a `Consistently(returned, "100ms").ShouldNot(Receive())`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound via `windowCh` and it is asserted to equal `ops.SessionTurnTimeout` AND `30 * libtime.Minute`. - AC2 — a clean exit (`doneCh <- nil` with valid JSON written to stdout) returns nil ("passes the session id and name to the detached runner"). - AC3 — "validates the turn and rejects a zero-turn result", "validates the turn and rejects an is_error result", "rejects an unparseable turn result". - - AC4 — "treats a child exit error as an error": error containing `"exit status 1"` AND `"exited with error"`; no assertion anywhere still uses the old `"exited during startup"` string. + - AC4 — "treats a child exit error as an error": error containing `"exit status 1"` AND `"exited with error"`; no assertion anywhere still uses `"exited during startup"`. - AC5 — "treats the turn timeout as an error so no id is persisted": error containing `"did not complete within"`. - AC6 — "treats context cancellation as an error so no id is persisted": error containing `"wait cancelled"` (NOT nil); plus "wraps a spawn failure" for `"start detached claude session"`. - The existing interactive-branch tests (lines 54-254) must be UNCHANGED — they lock the byte-identical validation strings. The "session lock lifecycle" context (spec 042, line 490) must also be left untouched. + The spec-045 test specs already present in the same context (valid blob + non-zero exit → nil; parsed-but-rejected blob leads with the child's reason; unparseable output names the exit status; timeout-with-valid-blob-on-disk still errors) are spec 045's — confirm they exist, do NOT touch them. The interactive-branch tests (lines 54-254) must be UNCHANGED by you (spec 045 already updated the two that lock its reworded validator strings). The "session lock lifecycle" context (line 625) must also be left untouched. + `pkg/ops/claude_session_detach_test.go` must contain "child outlives a cancelled parent wait": spawns a real script (`#!/bin/sh\nsleep 6\ntouch `), cancels the context after ~500ms, asserts `StartSession` returns an error, asserts the sentinel does NOT exist yet, then `Eventually(..., "20s", "200ms")` asserts the sentinel appears — proving the detached child survived the parent's cancelled wait. It constructs the starter via `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the two-arg form is spec 042's; keep it). If any of these is missing, implement it. -7. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a test ("child outlives a cancelled parent wait", line 24) that spawns a real script (`#!/bin/sh\nsleep 6\ntouch `), cancels the context after ~500ms, asserts `StartSession` returns an error, asserts the sentinel does NOT exist yet, and then `Eventually(..., "20s", "200ms")` asserts the sentinel appears — proving the detached child survived the parent's cancelled wait. The file constructs the starter via `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the two-arg form is spec 042's; keep it). If the file or test is missing, implement it. - -8. **BACKFILL — rename the test variable to satisfy AC1's evidence grep.** In `pkg/ops/claude_session_test.go`, inside the "blocks until the detached child exits" spec (lines 340-347), the local variable is currently named `window`. Rename it to `capturedWindow` so the assertion line reads exactly `Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`. The block changes old → new as follows (keep `windowCh` — the channel — as-is; only the bare `window` variable is renamed): +8. **BACKFILL — rename the test variable to satisfy AC1's evidence grep.** In `pkg/ops/claude_session_test.go`, inside the "blocks until the detached child exits" spec (starts at line 327), the local variable is currently named `window`. Rename it to `capturedWindow` so the assertion line reads exactly `Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`. The block changes old → new as follows (keep `windowCh` — the channel — as-is; only the bare `window` variable is renamed): ```go // OLD var window libtime.Duration @@ -118,7 +108,7 @@ The target state for this prompt already exists in the tree (shipped as commit 2 ``` Do not rename the `windowCh` channel or any other identifier. This is a pure rename; no behavior changes. -9. **BACKFILL — assert the temp output file is removed after a clean exit.** No existing test asserts AC2's "the temp file is removed" half. Add ONE dedicated spec in the "non-interactive branch" context of `pkg/ops/claude_session_test.go` (after "wraps a spawn failure", the last spec in that context, which ends at line 487). `validTurnJSON`, `blockWaiter`, `starter`, `ctx`, and `locker` are all in scope there. Add this spec verbatim: +9. **BACKFILL — assert the temp output file is removed after a clean exit.** No existing test asserts AC2's "the temp file is removed" half. Add ONE dedicated spec in the "non-interactive branch" context of `pkg/ops/claude_session_test.go` (after "wraps a spawn failure", the last spec in that context, which ends around line 622). `validTurnJSON`, `blockWaiter`, `starter`, `ctx`, and `locker` are all in scope there. This requires adding `path/filepath` to the file's imports (the current import block is: `context`, `errors`, `os`, `time`, `libtime`, `uuid`, ginkgo, gomega, `ops`). Add this spec verbatim: ```go It("removes the temp output file after a clean exit", func() { matches := func() []string { @@ -148,24 +138,25 @@ The target state for this prompt already exists in the tree (shipped as commit 2 Expect(matches()).To(Equal(before)) }) ``` - This requires adding `path/filepath` to the file's imports (the current import block is: `context`, `errors`, `os`, `time`, `libtime`, `uuid`, ginkgo, gomega, `ops`). The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the before/after glob counts must be equal. Note the spec overrides `starter` with its own fake (writing `validTurnJSON` then `done <- nil`) so the child-exit branch wins and the blocking waiter goroutine stays parked on `<-bw` until `DeferCleanup` closes `blockWaiter` — the established pattern in this context. + The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the before/after glob counts must be equal. Note the spec overrides `starter` with its own fake (writing `validTurnJSON` then `done <- nil`) so the child-exit branch wins and the blocking waiter goroutine stays parked on `<-bw` until `DeferCleanup` closes `blockWaiter` — the established pattern in this context. -10. **Self-check against AC1-6 and AC10.** Before finishing, re-read the changed hunks and walk each AC: the constant, the wait-select, the validation helper, the rename, and the new cleanup test. Run the `` block and confirm every grep that is expected to pass does pass; the two greps flagged in `` as spec-quoting artifacts must NOT be "fixed" by editing error strings. +10. **Self-check against AC1-6 and AC10.** Before finishing, re-read the changed hunks and walk each AC: the constant, the wait-select, the validation helper, the rename, and the new cleanup test. Run the `` block and confirm every grep that is expected to pass does pass; the two greps flagged in `` as spec artifacts must NOT be "fixed" by editing error strings. -Failure-mode coverage from the spec's table: bound expiry (row 1, AC5 test), ctx cancel mid-wait with child survival (row 2, AC6 unit + detach integration), child exits non-zero (row 3, AC4 test), turn JSON is_error / 0 turns (row 4, AC3 tests), temp file unreadable/empty (row 5, AC3 unparseable test), UI request timeout < turn (row 8, AC6 cancel path). Each is covered by the corresponding test in this prompt. +Failure-mode coverage from the spec's table: bound expiry (row 1, AC5 test), ctx cancel mid-wait with child survival (row 2, AC6 unit + detach integration), child exits non-zero (row 3, AC4 test — with the post-045 refinement, the unparseable-output case is what this assertion covers), turn JSON is_error / 0 turns (row 4, AC3 tests), temp file unreadable/empty (row 5, AC3 unparseable test), UI request timeout < turn (row 8, AC6 cancel path). Each is covered by the corresponding test in this prompt. -- Do NOT commit — dark-factory handles git. `git diff --exit-code HEAD` reads only; do not stage or commit anything. -- Interactive branch behavior unchanged. `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only permitted interactive-branch edit is the already-extracted `validateSessionTurn` call — behavior-preserving, same checks, byte-identical error strings. Do NOT re-extract or change it. +- Do NOT commit — dark-factory handles git. +- Interactive branch behavior unchanged. `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only permitted interactive-branch edit is none — the shared `validateSessionTurn` is already extracted and called from both branches; do NOT re-extract or reword it. - Detachment preserved: `exec.Command` (NOT `CommandContext`), `Setpgid`, stdout/stderr handling that lets the child survive the parent. NEVER SIGKILL the child on timeout; `--max-turns` is inert (`-1`), so the 30-min bound is a wait-channel select, not a context kill. Do NOT resurrect `"claude session start timed out"`. - Never offer a broken Resume: on any failure (exit error, `is_error`, 0 turns, bound expiry, ctx cancel) `StartSession` returns an error. Returning nil on ctx-cancel is wrong — it would persist an id for a still-running child. - JSON validation: `num_turns > 0` AND `is_error == false`. Lowercase UUIDs; keep `-n ""` at mint so resume inherits the title. - Error idiom: `errors.Wrapf(ctx, err, ...)` / `errors.Wrap(ctx, err, ...)` / `errors.Errorf(ctx, ...)` from `github.com/bborbe/errors`; no `fmt.Errorf`; no bare `return err`; no `context.Background()` in `pkg/`. - `sessionTurnTimeout` stays a tunable const — do NOT add a config field (spec Open Question 1 recommends const; no second caller exists). -- Do NOT alter the error strings to satisfy a grep pattern. The AC4/AC5 evidence greps in the spec's Verification carry a trailing-quote artifact (see ``); the source strings are correct as written. +- Do NOT alter the spec-045 error strings or the exit-status-vs-result precedence to satisfy a grep pattern. The spec's AC3/AC4/AC5 evidence greps are stale against the post-045 validator (see ``); the source strings are correct as written. - `ClaudeSessionStarter.StartSession` signature is UNCHANGED (6 args: ctx, sessionID, prompt, cwd, name, isInteractive) — `mocks/claude-session-starter.go` is untouched. The `SessionLocker` constructor parameter (spec 042) is already wired in and must stay. - Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, or `docs/work-on-session-lifecycle.md` in this prompt — the workon reorder is prompt 2, the doc reword is prompt 3. +- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). The scenario-005-untouched check is operator-side. - Existing tests must still pass. @@ -178,8 +169,7 @@ grep -c '30 \* libtime.Minute' pkg/ops/claude_session_test.go # grep -c 'validateSessionTurn' pkg/ops/claude_session.go # >= 2 (both branches call it) grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go # >= 1 — THIS IS BACKFILL REQ 8; must flip 0 -> 1 grep -c 'removes the temp output file after a clean exit' pkg/ops/claude_session_test.go # >= 1 — THIS IS BACKFILL REQ 9; must flip 0 -> 1 -grep -c '"0 turns"' pkg/ops/claude_session_test.go # >= 1 (AC3) -grep -c 'claude session exited with error' pkg/ops/claude_session.go # >= 1 (AC4, real check — unquoted form) +grep -c 'claude session exited with error' pkg/ops/claude_session.go # >= 1 (AC4, real check — unquoted form; the string survives on the unparseable-output path) grep -c 'exited during startup' pkg/ops/claude_session.go # == 0 (AC4) grep -c 'did not complete within' pkg/ops/claude_session.go # >= 1 (AC5, real check — unquoted form) grep -c 'livenessWindow' -r pkg/ # == 0 @@ -187,12 +177,11 @@ grep -c 'defaultCommandRunner' pkg/ops/claude_session.go # grep -c 'context.WithTimeout' pkg/ops/claude_session.go # == 1 (AC10, interactive branch) ``` -Note on spec-quoting artifacts: the spec's AC4/AC5 evidence greps use the literal `'"claude session exited with error"'` and `'"did not complete within"'` (trailing double-quote inside the pattern). Those two forms CANNOT match the real source strings (`"claude session exited with error: %v"` / `"claude session turn did not complete within %v"`), so they read 0 against CORRECT code. Do NOT force them to 1 by editing error strings — the unquoted forms above are the real checks and must pass. +Note on spec artifacts you must NOT "fix": +- The spec's AC3 evidence `grep -c '"0 turns"' pkg/ops/claude_session_test.go >= 1` CANNOT match the post-045 validator (`claude returned num_turns: 0`); the tests assert `num_turns` / `claude reported is_error` instead. Do NOT reword the validator to force "0 turns". +- The spec's AC4/AC5 evidence greps carry a literal trailing double-quote (`'"claude session exited with error"'`, `'"did not complete within"'`) that can never match the real source strings (`"claude session exited with error: %v"` / `"claude session turn did not complete within %v"`). The unquoted forms above are the real checks. -SECONDARY — AC10 git guard (git IS available — workflow `direct`, no hideGit): -``` -git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md # must exit 0 with empty output -``` +SECONDARY — AC10 scenario-005 guard is OPERATOR-SIDE (git is masked in this container). The container-side proxies are the `defaultCommandRunner` == 3 and `context.WithTimeout` == 1 greps above. SYNTAX + TESTS: ``` diff --git a/prompts/2-spec-041-post-exit-persist.md b/prompts/2-spec-041-post-exit-persist.md index ab1b6b1..801aba8 100644 --- a/prompts/2-spec-041-post-exit-persist.md +++ b/prompts/2-spec-041-post-exit-persist.md @@ -1,17 +1,17 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-03T10:05:00Z" +created: "2026-09-06T16:25:00Z" --- -- Re-applies the spec-041 start→persist reorder to `workon.go`'s fresh-start path: the headless turn is started FIRST and `claude_session_id` + the metrics entry are persisted only AFTER it exits cleanly. -- Deletes the now-dead `clearSessionAndMetrics` compensating-clear function from `workon.go` and its doc references (AC9) — the current tree still carries the pre-spawn persist + compensating clear that spec-041's AC9 requires removing. +- Re-applies the spec-041 start→persist reorder to `workon.go`'s fresh-start path: the headless turn is started FIRST and `claude_session_id` + the metrics entry are persisted only AFTER it exits cleanly, structurally matching `goal_workon.go`, which already ships this. +- Deletes the now-dead `clearSessionAndMetrics` compensating-clear function from `workon.go` and its doc references (AC9) — the current tree still carries the pre-spawn persist + compensating clear that AC9 requires removing. - Reworks `workon_test.go`: the "persisting the session id before spawning" test becomes "after the child exits" asserting `writeTaskAt.After(childExitAt)` (AC7), the "write precedes the spawn" sequencing test inverts, the clear-based failure tests are deleted and replaced with a persists-nothing assertion, and the "pre-spawn persist re-read fails" context becomes "post-exit". - Rewords the stale pre-spawn / liveness-window comments and the clear-based child-exit context in `workon_session_writeback_test.go` to the post-exit no-clear semantics; the writeback fakes already write valid JSON to the stdout file and exit cleanly via `done <- nil` with a blocking waiter, so the AC8 invariant assertions are confirmed unchanged. -- Confirms `goal_workon.go` and its tests are already in the spec-041 target state (they were never reverted) and are left untouched. -- ⚠️ IMPORTANT TREE CONFLICT FLAGGED FOR THE HUMAN REVIEWER: the task-side half of this spec was REVERTED in the tree after approval (commit dae6563, released v0.118.3) because persist-after-exit left `claude_session_id` empty during the turn and the child's own session-connect scanned the transcript dir by mtime and bound the task to a live unrelated session (reproduced live 2026-09-01). This prompt implements the spec AS APPROVED — re-applying start→persist — and the reviewer must adjudicate the conflict at audit time (details in requirement 2's comment): (A) approve, spec-041 wins and the session-connect regression is owned as a follow-up; or (B) reject and re-scope the spec to treat the reversion as the target. Prompt 3 is coupled to this decision. -- Runs `make test` and the spec-041 AC7-9 grep gate. +- Confirms `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state (the goal AC7 half — `writeGoalAt.After(childExitAt)` — already passes) and are left untouched. +- Coordinates with the in-flight spec-045 batch: prompt 2 of spec 045 adds retain/clear contexts to the SAME two test files this prompt touches. Those added contexts remain valid under the reorder (a successful turn still leaves the id on disk; a failed turn still leaves none) and must be left green and untouched. This prompt must run AFTER spec-045 prompt 2 has landed (see the dependency comment in requirements). +- Runs `make test` + the AC7-9 grep gate. No git in this container (masked); nothing here needs it. @@ -26,39 +26,32 @@ Read fully (in this order): - `pkg/ops/goal_workon.go` — the whole file; this is the structural TEMPLATE the reordered task path must match (`handleClaudeSession` at line 198, `persistGoalSessionID` at line 174). - `pkg/ops/workon_test.go` — the whole file; the AC7 contexts are "when persisting the session id before spawning" (line 906), "when the session id write precedes the spawn" (line 160), "when the spawn fails" (line 982), "when the pre-spawn persist re-read fails" (line 877), and the comment-era specs at lines 98-157. - `pkg/ops/goal_workon_test.go` — the whole file; the AC7 context "when persisting the goal session id after the child exits" starts at line 355 (already spec-041 — confirm, do not touch). -- `pkg/ops/workon_session_writeback_test.go` — the whole file. -- `docs/work-on-session-lifecycle.md` — the design record for the post-exit ordering (its task-path sections were reverted in v0.118.3; prompt 3 fixes the doc — do NOT edit it here). +- `pkg/ops/workon_session_writeback_test.go` — the whole file. NOTE: if spec-045 prompt 2 has already landed, this file also contains its two added contexts (a task retain spec, a task `is_error` clear spec, and the mirrored goal pair). Those must remain green and untouched. +- `pkg/ops/claude_session.go` — `runDetachedTurn` and `validateSessionTurn` as they now stand (spec-041 + spec-045), so the seeded blobs and expected errors match the real implementation. +- `docs/work-on-session-lifecycle.md` — the design record for the post-exit ordering (its task-path sections still describe the reverted pre-spawn design; prompt 3 fixes the doc — do NOT edit it here). Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrapf(ctx, ...)` / `errors.Wrap(ctx, ...)` / `errors.Errorf(ctx, ...)` idiom from `github.com/bborbe/errors`. - `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2/Gomega conventions. -NOTE: git IS available in this container (`workflow: direct`, no hideGit) — but this prompt has no git commands; AC10's `scenarios/005` guard is verified in prompt 1. +IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked). Do NOT run any `git` command. Nothing in this prompt needs it. - +Also note: the task-side half of spec-041 was REVERTED in the tree after approval (commit dae6563, released v0.118.3) — it re-adopted spec-040's persist-before-spawn + compensating clear on the task path ONLY. claude_session.go (prompt 1), goal_workon.go, and the session tests were never reverted. This prompt implements the spec AS APPROVED: re-applies start→persist and deletes the clear. That re-introduces the session-connect concern that v0.118.3 fixed (the child's own /vault-cli:work-on-task session-connect reads claude_session_id during the turn). The spec-042 per-session lock and the vault-ui resolver gate already mitigate the double-writer/resume window. The reviewer should adjudicate this at audit time — the spec is the source of truth for this batch. --> 1. **Guard — prompt 1 must have shipped.** Before doing anything, confirm prompt 1's deliverables exist: `grep -c 'validateSessionTurn' pkg/ops/claude_session.go` >= 2, `grep -c 'sessionTurnTimeout' pkg/ops/claude_session.go` >= 1, and `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` >= 1. If ANY of these is 0, STOP and report `"status":"failed"` with message `"spec-041 prompt 2 precondition missing: prompt 1 not yet deployed"` — do not proceed. -2. **Reorder the fresh-start path in `workon.go`'s `handleClaudeSession` to start→persist.** The current code (the fresh-start block starting at the `prompt := fmt.Sprintf(...)` line and ending at `return sessionID, nil`) persists BEFORE the spawn and compensates on failure. Replace the block from `prompt := fmt.Sprintf(...)` through `return sessionID, nil` with this exact code (structurally identical to `goal_workon.go`'s non-interactive branch, and to the interactive branch — same checks, same error strings): +2. **Reorder the fresh-start path in `workon.go`'s `handleClaudeSession` to start→persist.** The current code (the fresh-start block starting at the `prompt := fmt.Sprintf(...)` line and ending at `return sessionID, nil`) persists BEFORE the spawn and compensates on failure. Replace the block from `prompt := fmt.Sprintf(...)` through `return sessionID, nil` with this exact code (structurally identical to `goal_workon.go`'s non-interactive branch): ```go prompt := fmt.Sprintf(`%s "%s" --non-interactive`, vault.GetWorkOnCommand(), task.FilePath) sessionID := w.uuidGenerator() @@ -72,7 +65,7 @@ For the executing agent: implement the requirements below as written — the spe sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) return sessionID, err ``` - The old code being replaced (delete these lines entirely): the `// Persist id + metrics BEFORE the child exists...` comment block, the `if _, err := persistSessionAndMetrics(...); err != nil { return "", errors.Wrap(ctx, err, "persist claude session before spawn") }` call, and the `if err := w.starter.StartSession(...); err != nil { ... clearSessionAndMetrics ... }` compensating-clear block. The cached-session path (the `if existing := task.ClaudeSessionID(); existing != ""` branch, which re-reads and re-persists via `persistSessionAndMetrics`) must be UNCHANGED. Note the function returns `(string, error)` — the new `return "", errors.Wrap(...)` is 2 values; `sessionID, err := ...` compiles because `sessionID` is already declared above and `err` is newly introduced in that scope (this is exactly `goal_workon.go` line 224's pattern). Do NOT copy the spec Design's `return "", nil, errors.Wrap(...)` snippet — that 3-value form is a spec typo and does not compile against the real signature. + The old code being replaced (delete these lines entirely): the `// Persist id + metrics BEFORE the child exists...` comment block, the `if _, err := persistSessionAndMetrics(...); err != nil { return "", errors.Wrap(ctx, err, "persist claude session before spawn") }` call, and the `if err := w.starter.StartSession(...); err != nil { ... clearSessionAndMetrics ... }` compensating-clear block. The cached-session path (the `if existing := task.ClaudeSessionID(); existing != ""` branch, which re-reads and re-persists via `persistSessionAndMetrics`) must be UNCHANGED. Note the function returns `(string, error)` — the new `return "", errors.Wrap(...)` is 2 values; `sessionID, err := ...` compiles because `sessionID` is already declared above and `err` is newly introduced in that scope (this is exactly `goal_workon.go` line 236's pattern). Do NOT copy the spec Design's `return "", nil, errors.Wrap(...)` snippet — that 3-value form is a spec typo and does not compile against the real signature. 3. **Delete the dead `clearSessionAndMetrics` function from `workon.go`.** Remove the entire function (its doc comment plus body, currently at lines 243-269). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return NOTHING (AC9). Its only call site was the compensating-clear block deleted in requirement 2. Do not add any replacement. @@ -88,7 +81,7 @@ For the executing agent: implement the requirements below as written — the spe Expect(writeTaskAt.After(childExitAt)).To(BeTrue()) Expect(writtenSessionID).To(Equal(spawnedSessionID)) ``` - Keep the existing comment about AC5's "id equals the value in task frontmatter". This yields the AC7 evidence `After(childExitAt)` (currently absent — the old assertion is `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())`). + Keep the existing comment about AC5's "id equals the value in task frontmatter". This yields the AC7 evidence `After(childExitAt)` in `workon_test.go` (currently absent — the old assertion is `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())`). 7. **Invert "when the session id write precedes the spawn" in `workon_test.go` (lines 160-185).** Rename the context to `"when the session id write follows the spawn"`, rename the `It` to `"writes the session id to storage after StartSession returns"`, and change the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. The `WriteTaskStub`/`StartSessionStub` sequencing setup stays as-is. @@ -121,6 +114,7 @@ For the executing agent: implement the requirements below as written — the spe - Reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window.` to `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins.` - Reword the mechanism comments that describe the pre-spawn persist + compensating clear so they describe the post-exit no-clear ordering. The on-disk assertions they annotate (phase survives, raw file has no `claude_session_id:`, no `pinnedSessionID`) are byte-identical under the new ordering — a failed turn simply never persisted anything — so DO NOT touch the assertions. - Do NOT touch the pinned-count strings anywhere in this file: `TaskPhaseExecution` (==2), `GoalPhaseExecution` (==2), `session_note` (==4), `MetricsSessions()` (==2), `ClaudeSessionID()` (==2). The AC8 greps must stay byte-identical. + - If spec-045 prompt 2 has already landed, its added retain/clear contexts are present in this file — do NOT edit, rename, or reword them, and confirm they still pass after your reorder (they should, per the sequencing comment at the top of requirements). 12. **Confirm the goal AC7 test and AC8 assertions are already correct — do not touch them.** `goal_workon_test.go` "when persisting the goal session id after the child exits" already asserts `writeGoalAt.After(childExitAt)` with both non-zero. `workon_session_writeback_test.go`'s task and goal `It`s already assert the child's phase + `session_note` survive, `ClaudeSessionID() == pinnedSessionID`, and `MetricsSessions()` length 1. Confirm and leave unchanged. @@ -141,8 +135,9 @@ For the executing agent: implement the requirements below as written — the spe - The `ClaudeSessionStarter` interface signature is UNCHANGED — `mocks/claude-session-starter.go` is untouched. `handleClaudeSession`'s `(string, error)` signature is UNCHANGED — the spec Design's `return "", nil, errors.Wrap(...)` snippet is a typo and must NOT be used (it does not compile). - Do NOT add a double-Start guard and do NOT add any config knob (both are spec Non-goals / Open Question 1). - The AC8 pinned-count strings (`TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`) in `workon_session_writeback_test.go` must remain byte-identical — requirement 11's comment reword must not touch any assertion. -- `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state — do not modify them except to confirm. +- `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state — do not modify them except to confirm. If spec-045 prompt 2's added contexts are present in `goal_workon_test.go`, leave them untouched. - Do NOT touch `docs/work-on-session-lifecycle.md` in this prompt (prompt 3 rewords it) or `pkg/ops/claude_session.go` (prompt 1 owns it). +- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). - Existing tests must still pass. diff --git a/prompts/3-spec-041-docs-changelog.md b/prompts/3-spec-041-docs-changelog.md index d76c021..0e9ca30 100644 --- a/prompts/3-spec-041-docs-changelog.md +++ b/prompts/3-spec-041-docs-changelog.md @@ -1,15 +1,15 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-03T10:10:00Z" +created: "2026-09-06T16:30:00Z" --- -- Rewords the task-path sections of `docs/work-on-session-lifecycle.md` back to the spec-041 post-exit ordering: "Post-exit write ordering", "Failure path", and the per-session lock's detached-child safety paragraph describe BOTH paths persisting only after the turn, with no compensating clear — the v0.118.3 reversion rewrote the bodies to pre-spawn + compensating clear while leaving the spec-041 headings, and this prompt removes that stale content. -- Confirms `scenarios/002-task-lifecycle.md` already says the headless turn blocks until completion (no "~10s" return claim) — AC11. -- Appends the spec-041 bullet under `## Unreleased` in `CHANGELOG.md` (AC12). The section exists today (carrying the concurrent spec-044 resolve bullet) but holds no spec-041 bullet, and `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads 0. The requirement is written to create the section if a concurrent change consumes it between now and execution. +- Rewords the task-path sections of `docs/work-on-session-lifecycle.md` to the spec-041 post-exit no-clear ordering: the "Post-exit write ordering" and "Failure path" bodies, and the per-session lock's "detached-child safety property" paragraph. The v0.118.3 task-side reversion rewrote those bodies to pre-spawn persist + compensating clear while leaving the spec-041 headings and intro — this prompt removes that stale content. +- Coordinates with the in-flight spec-045 batch: spec-045 prompt 3 owns the doc's "What the turn timeout does and does not cover" section (the "a non-zero child exit all return an error" sentence becomes "validated result outranks the exit code"). This prompt leaves that section alone and does NOT reword the 045 sentence. Run AFTER spec-045 prompt 3. +- Confirms `scenarios/002-task-lifecycle.md` already says the headless turn blocks until completion (no "~10s" return claim) — AC11. No edit needed. +- Appends the spec-041 bullet under `## Unreleased` in `CHANGELOG.md` (AC12). Today `## Unreleased` exists and carries spec-045's precedence bullet; it holds NO spec-041 bullet, and the `grep -A15 '^## Unreleased' | grep -ci 'resume'` hit that reads 1 today comes from v0.124.0's bullet downstream, not from a 041 bullet. The 041 bullet makes the grep match itself. - Runs the `make precommit` full gate (AC13) as the batch's final validation. -- Coupled to prompt 2: the doc reword describes the post-exit behavior prompt 2 re-applies to `workon.go`. The appended changelog bullet also contradicts the v0.118.3 release note that documented the task-side reversion — a reviewer comment inside requirement 4 flags this tension. - Open questions surfaced for the reviewer: (1) spec Open Question 1 (configurable `sessionTurnTimeout`) is resolved as a tunable const — no config field; (2) spec Open Question 2 (the vault-ui "Creating session… up to 2 minutes" modal copy) is a separate repo and out of scope here — no vault-ui change is made by this prompt. @@ -21,23 +21,27 @@ Confirm — and correct where the tree drifted — that the docs and scenario de Read CLAUDE.md for project conventions. Read fully (in this order): -- `docs/work-on-session-lifecycle.md` — the whole file. This is the file under test. -- `scenarios/002-task-lifecycle.md` — the whole file. -- `CHANGELOG.md` — read the top ~50 lines fully (the `# Changelog` header block, `## Unreleased` if present, and the newest version sections `## v0.121.1`, `## v0.121.0`, `## v0.120.0`). That is where the AC12 bullet lands; the rest of the file is not needed. +- `docs/work-on-session-lifecycle.md` — the whole file. This is the file under test. Its intro (lines 3-13) and section headings are already spec-041; the BODIES of "Post-exit write ordering" (lines 32-48), "Failure path" (lines 106-114), and the "detached-child safety property" paragraph (lines 168-176) still describe the reverted pre-spawn + compensating-clear design. If spec-045 prompt 3 has already landed, the "What the turn timeout does and does not cover" section carries its "validated result outranks the exit code" sentence — leave it alone. +- `scenarios/002-task-lifecycle.md` — the whole file. Already spec-041 (the work-on note says the turn blocks until completion). Confirm only. +- `CHANGELOG.md` — read the top ~30 lines fully (the `# Changelog` preamble, `## Unreleased`, and the newest version sections). That is where the AC12 bullet lands; the rest of the file is not needed. - `pkg/ops/goal_workon.go` — only to confirm the post-exit wording the doc must match (lines 192-238). Do not modify it. Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — `## Unreleased` placement and style rules, prefix requirement (`feat:` / `fix:` / ...), one bullet per logical change. Write `## Unreleased` bullets only, never a version number and never a manifest/tag bump (this repo's release model: the github-releaser owns version bumps/tags post-merge; `make precommit` runs `check-versions` which requires the four version strings aligned, but this prompt does not hand-bump them). -NOTE: git IS available in this container (`workflow: direct`, no hideGit) — but AC10's `scenarios/005` guard is verified in prompt 1; this prompt has no git commands. +IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked). Do NOT run any `git` command. Nothing in this prompt needs it. -The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversion rewrote the BODIES of the "Post-exit write ordering" and "Failure path" sections (and the per-session lock's detached-child safety paragraph) to describe pre-spawn persist + compensating clear on the task path, while the section HEADINGS and the intro still carry the spec-041 post-exit framing. This prompt rewrites those bodies back to the spec-041 target. The scenario is already correct; the one genuinely missing item is the spec-041 bullet under CHANGELOG `## Unreleased` (AC12). +The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversion rewrote the BODIES of the "Post-exit write ordering" and "Failure path" sections (and the per-session lock's "detached-child safety property" paragraph) to describe pre-spawn persist + compensating clear on the task path, while the section HEADINGS and the intro still carry the spec-041 post-exit framing. This prompt rewrites those bodies back to the spec-041 target. The scenario is already correct; the one genuinely missing item is the spec-041 bullet under CHANGELOG `## Unreleased` (AC12). -1. **Guard — prompts 1 and 2 must have shipped.** Before doing anything, confirm prompt 2's deliverables exist: `grep -c 'After(childExitAt)' pkg/ops/workon_test.go` >= 1 AND `grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go` >= 1 AND `grep -c 'clearSessionAndMetrics' pkg/ops/workon.go` == 0. If ANY is absent, STOP and report `"status":"failed"` with message `"spec-041 prompt 3 precondition missing: prompt 2 not yet deployed"` — do not proceed. + + +1. **Guard — prompts 1 and 2 must have shipped.** Before doing anything, confirm: `grep -c 'After(childExitAt)' pkg/ops/workon_test.go` >= 1 AND `grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go` >= 1 AND `grep -c 'clearSessionAndMetrics' pkg/ops/workon.go` == 0 AND `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` >= 1. If ANY is absent, STOP and report `"status":"failed"` with message `"spec-041 prompt 3 precondition missing: prompt 1/2 not yet deployed"` — do not proceed. + +2. **Reword `docs/work-on-session-lifecycle.md`'s task-path sections to the spec-041 post-exit, no-clear ordering.** The intro (lines 3-13, "spec 040, revised by spec 041 ... An id on disk now means the session is resumable") is already correct — keep it. Fix these stale bodies: - **"## Post-exit write ordering" (heading is correct; BODY is stale).** The paragraph currently says `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**: persistSessionAndMetrics runs first, then StartSession... A spawn failure triggers a re-read-based compensating clear...` and `On the task path the pre-spawn re-read before writing is load-bearing`. Reword the body to: ``` On both paths — task (`pkg/ops/workon.go`) and goal (`pkg/ops/goal_workon.go`) — the @@ -74,18 +78,17 @@ The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversio running unlocked is not targetable. On any failure nothing was persisted, so the id cannot stay resumable-looking. ``` - - Do NOT touch the other sections (`## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, `## What the turn timeout does and does not cover`, and the rest of `## The per-session lock`) — they are already spec-041. In particular the "liveness gating" phrase in the lock's "Lock scope" paragraph is the spec-042 vault-ui follow-on concept, NOT the removed liveness-window concept — leave it. + - **Small term cleanup in "## The per-session lock" → "No stale lock."** The sentence `There are no cleanup sweeps, no compensating clears, and no lock TTL;` contains the reverted-vocabulary term "compensating clears" (it describes the LOCK, but the drift guard in `` pins the term to 0). Reword `no compensating clears` → `no explicit clears`, keeping the rest of the sentence. + - Do NOT touch the other sections (`## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, `## What the turn timeout does and does not cover` — the latter is spec-045 prompt 3's scope, leave it as it stands — and the rest of `## The per-session lock`). In particular the "liveness gating" phrase in the lock's "Lock scope" paragraph is the spec-042 vault-ui follow-on concept, NOT the removed liveness-window concept — leave it. - After the reword, the whole file must contain NO occurrence of the reverted vocabulary: `grep -c 'pre-spawn\|pre-persisted\|pre-persist\|before the child is spawned\|compensating clear' docs/work-on-session-lifecycle.md` must be 0. 3. **Confirm `scenarios/002-task-lifecycle.md` (AC11).** The work-on action note must say the headless turn blocks until completion — it already does (`**Both branches block until the turn completes** ... bounded by a 30m turn timeout` and `A fast return is a FAIL, not a pass`). Verify `grep -c '~10s' scenarios/002-task-lifecycle.md` == 0. If it returns non-zero, replace the "~10s" wording. No other edit to this file. -4. **ADD the spec-041 bullet under the `## Unreleased` section of `CHANGELOG.md` (AC12 — the one genuinely missing item).** Today `## Unreleased` exists (it carries the concurrent spec-044 `resolve` bullet) — do NOT create a second `## Unreleased` section and do NOT touch any existing bullet. Append ONE new bullet directly after the existing bullet(s) within the same section, so `## Unreleased` reads (existing bullet(s) then) the spec-041 bullet: +4. **ADD the spec-041 bullet under the `## Unreleased` section of `CHANGELOG.md` (AC12 — the one genuinely missing item).** Today `## Unreleased` exists and carries spec-045's precedence bullet — do NOT create a second `## Unreleased` section and do NOT touch any existing bullet (045's included). Append ONE new bullet directly after the existing bullet(s) within the same section, so the spec-041 bullet is within the first 15 lines of the section: ``` - fix: non-interactive `task work-on` / `goal work-on` now wait for the detached headless turn to exit before persisting `claude_session_id`, bounded by a 30m turn timeout (a wait bound, never a kill), so the Vault UI offers Resume only against a complete, single-writer transcript; a failed or zero-turn session persists no id. The interactive TTY branch is unchanged. ``` - If `## Unreleased` does NOT exist when you run (a concurrent change may have consumed it into a `## vX.Y.Z` section), CREATE it: insert `## Unreleased` immediately after the `# Changelog` preamble and before the newest `## vX.Y.Z` section, with the spec-041 bullet as its only bullet. Either way, the spec-041 bullet must appear within the first 15 lines of the `## Unreleased` section so `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads >= 1 from THIS bullet. The bullet must contain the substrings "Resume" (case-insensitive) and "wait for the detached headless turn". Today `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads 0; after this edit it must read >= 1. - - + If `## Unreleased` does NOT exist when you run (a concurrent change may have consumed it into a `## vX.Y.Z` section), CREATE it: insert `## Unreleased` immediately after the `# Changelog` preamble and before the newest `## vX.Y.Z` section, with the spec-041 bullet as its only bullet. Either way, after this edit `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` must read >= 1 from THIS bullet. (Today it reads 1 from v0.124.0's downstream bullet — that does NOT satisfy the intent.) The bullet must contain the substrings "Resume" (case-insensitive) and "wait for the detached headless turn". 5. **Full gate (AC13).** Run `make precommit` at the repo root. This is the batch's final validation. If it fails on any spec-041-related lint/test, fix it (re-run only the failing target — `make lint`, `make gosec`, `make errcheck`, etc. — then `make precommit` once more). Note the repo's version-alignment rule: `make precommit` runs `check-versions`; do NOT hand-bump the plugin manifests or `git tag` — the github-releaser owns version bumps post-merge. If `check-versions` fails because `## Unreleased` now sits above an un-released `## vX.Y.Z`, that is a pre-existing release-sequencing condition, not a spec-041 defect — report it in `## Improvements` and continue. @@ -94,12 +97,14 @@ The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversio - Do NOT commit — dark-factory handles git. -- `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it (verified in prompt 1). +- `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it (verified operator-side via git; this container masks `.git`). - Interactive TTY branch unchanged; do not reword any doc text into claiming otherwise. - This prompt does NOT hand-bump the plugin manifests or `git tag` — only the `## Unreleased` bullet is in scope. Write `## Unreleased`, never `## vX.Y.Z`. -- Do NOT delete or reword the existing `## Unreleased` bullets (e.g. the concurrent spec-044 `resolve` bullet) or any `## vX.Y.Z` section — the spec-041 bullet is APPENDED (or, only if `## Unreleased` is absent, creates the section). +- Do NOT delete or reword the existing `## Unreleased` bullets (spec-045's precedence bullet, any concurrent bullet) or any `## vX.Y.Z` section — the spec-041 bullet is APPENDED (or, only if `## Unreleased` is absent, creates the section). +- Do NOT touch the "What the turn timeout does and does not cover" section of the doc — it is spec-045 prompt 3's scope; leave its sentence as it stands. - The "liveness gating" phrase in the per-session lock section is spec-042's vault-ui follow-on concept — leave it; it is not the removed liveness-window concept. - Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, or any `_test.go` file in this prompt — code changes belong to prompts 1 and 2. +- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). - Existing tests must still pass. @@ -115,12 +120,12 @@ grep -c '~10s' scenarios/002-task-lifecycle.md # == 0 grep -c 'pre-spawn\|pre-persisted\|pre-persist\|before the child is spawned\|compensating clear' docs/work-on-session-lifecycle.md # == 0 # AC12 CHANGELOG — Unreleased exists AND carries the spec-041 bullet: grep -c '^## Unreleased' CHANGELOG.md # >= 1 -grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 — must now match the spec-041 bullet itself (today 0) +grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 — must now match the spec-041 bullet itself (today the hit is v0.124.0's downstream bullet) grep -c 'wait for the detached headless turn' CHANGELOG.md # >= 1 — the spec-041 bullet is present ``` FULL GATE — `make precommit` (AC13): Run `make precommit` at the repo root. It must exit 0. If it fails on a spec-041-related lint/test, fix it (re-run only the failing target, then `make precommit` once more). If `check-versions` fails on a release-sequencing condition unrelated to spec-041, report it in `## Improvements` and re-run after confirming the four version strings are aligned. -AC10's `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` was verified in prompt 1 (this container has git; the guard passed there). +AC10's `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` is an OPERATOR-side check (this container masks `.git`); the container-side AC10 proxies are the `defaultCommandRunner` == 3 / `context.WithTimeout` == 1 greps verified in prompt 1. diff --git a/prompts/2-spec-045-workon-writeback-precedence.md b/prompts/completed/209-spec-045-workon-writeback-precedence.md similarity index 95% rename from prompts/2-spec-045-workon-writeback-precedence.md rename to prompts/completed/209-spec-045-workon-writeback-precedence.md index 6b13272..128ff96 100644 --- a/prompts/2-spec-045-workon-writeback-precedence.md +++ b/prompts/completed/209-spec-045-workon-writeback-precedence.md @@ -1,7 +1,13 @@ --- -spec: ["045-bug-exit-code-outranks-validated-turn"] -status: draft +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) diff --git a/prompts/3-spec-045-docs-and-changelog.md b/prompts/in-progress/210-spec-045-docs-and-changelog.md similarity index 99% rename from prompts/3-spec-045-docs-and-changelog.md rename to prompts/in-progress/210-spec-045-docs-and-changelog.md index e141263..6afce29 100644 --- a/prompts/3-spec-045-docs-and-changelog.md +++ b/prompts/in-progress/210-spec-045-docs-and-changelog.md @@ -1,7 +1,8 @@ --- -spec: ["045-bug-exit-code-outranks-validated-turn"] -status: draft +status: approved +spec: [045-bug-exit-code-outranks-validated-turn] created: "2026-09-06T13:20:00Z" +queued: "2026-09-06T14:07:44Z" --- # Lifecycle doc contract rewrite and CHANGELOG bullet (spec 045, prompt 3 of 3) diff --git a/specs/in-progress/041-bug-resume-races-live-headless-turn.md b/specs/in-progress/041-bug-resume-races-live-headless-turn.md index 6789c74..2f25f38 100644 --- a/specs/in-progress/041-bug-resume-races-live-headless-turn.md +++ b/specs/in-progress/041-bug-resume-races-live-headless-turn.md @@ -1,5 +1,5 @@ --- -status: generating +status: approved approved: "2026-08-28T08:35:16Z" generating: "2026-08-30T17:05:06Z" branch: dark-factory/bug-resume-races-live-headless-turn 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 index f1490b5..5df6e39 100644 --- a/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md +++ b/specs/in-progress/045-bug-exit-code-outranks-validated-turn.md @@ -1,8 +1,9 @@ --- -status: prompted +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 --- From ee525b5f4da5e669a49f4ef9ce0148a2f611e924 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 6 Sep 2026 16:16:58 +0200 Subject: [PATCH 3/5] Lifecycle doc contract rewrite and CHANGELOG bullet (spec 045, prompt 3 of 3) --- CHANGELOG.md | 1 + docs/work-on-session-lifecycle.md | 49 +++++++++++++++---- .../210-spec-045-docs-and-changelog.md | 7 ++- 3 files changed, 46 insertions(+), 11 deletions(-) rename prompts/{in-progress => completed}/210-spec-045-docs-and-changelog.md (96%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e577cc..4e73cf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). - 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.0 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/prompts/in-progress/210-spec-045-docs-and-changelog.md b/prompts/completed/210-spec-045-docs-and-changelog.md similarity index 96% rename from prompts/in-progress/210-spec-045-docs-and-changelog.md rename to prompts/completed/210-spec-045-docs-and-changelog.md index 6afce29..9e51f91 100644 --- a/prompts/in-progress/210-spec-045-docs-and-changelog.md +++ b/prompts/completed/210-spec-045-docs-and-changelog.md @@ -1,8 +1,13 @@ --- -status: approved +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) From 3e695877c86863ea3db1d1b3090e603e12d26f46 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 6 Sep 2026 16:25:56 +0200 Subject: [PATCH 4/5] restore spec-041 prompts clobbered by prompt generation --- prompts/1-spec-041-session-turn-block.md | 101 +++++++++++++---------- prompts/2-spec-041-post-exit-persist.md | 51 ++++++------ prompts/3-spec-041-docs-changelog.md | 47 +++++------ 3 files changed, 105 insertions(+), 94 deletions(-) diff --git a/prompts/1-spec-041-session-turn-block.md b/prompts/1-spec-041-session-turn-block.md index 2d6f4d2..5921a18 100644 --- a/prompts/1-spec-041-session-turn-block.md +++ b/prompts/1-spec-041-session-turn-block.md @@ -1,17 +1,18 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-06T16:20:00Z" +created: "2026-09-03T10:00:00Z" --- -- Confirms the non-interactive branch of `StartSession` already blocks until the detached headless turn exits (a wait-channel select bounded by `sessionTurnTimeout`, never a kill) and validates the turn's JSON on BOTH branches through the shared `validateSessionTurn` helper — the spec-041 session-side is shipped in the tree and NOT reverted. Do not rewrite what is already correct. -- Notes that spec 045's precedence inversion already landed in the same file: a validated turn result outranks a non-zero child exit, `validateSessionTurn`'s rejection messages were reworded (they now lead with the child's `result` text via the `rejectTurn` helper, plus the `errClaudeOutputUnparseable` sentinel), and the detached branch reads + validates the blob before consulting the exit status. This prompt confirms the 041 invariants that SURVIVE that shape and does NOT re-touch the exit-status-vs-result routing (spec 045 owns it). -- Confirms the 30-minute `sessionTurnTimeout` constant, the caller-owned temp-file capture with eager unlink, the `defaultDetachedRunner` signature, `export_test.go`'s `SessionTurnTimeout` accessor, and the AC10 interactive-branch guards (`defaultCommandRunner` == 3, `context.WithTimeout` == 1). -- Backfill 1: renames the turn-bound test variable `window` to `capturedWindow` in `pkg/ops/claude_session_test.go` so the spec's AC1 evidence grep (`Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`) matches. The assertion already exists under the other name — this is a pure rename, no behavior change. -- Backfill 2: adds the one genuinely missing test — the temp output file is removed after a clean exit (the AC2 "temp file is removed" half). No test currently asserts it. -- Flags a spec artifact: the spec's AC3 evidence grep `'"0 turns"'` cannot match the post-045 validator string (`"claude returned num_turns: 0"`). This prompt verifies the post-045 strings and forbids editing the validator to force the stale grep. -- Runs `make test` + the grep gate. `make precommit` is the batch's full-gate check (AC13) in prompt 3. +- Confirms the non-interactive branch of `StartSession` already blocks until the detached headless turn exits, instead of returning after ~10s while the child keeps writing — the spec-041 design is present in the tree and NOT reverted. The task-side reversion (v0.118.3, commit dae6563) only touched `workon.go` and the docs; this file, `export_test.go`, and the session tests were never reverted. +- Confirms the turn's `--output-format json` blob is validated on both branches through the shared `validateSessionTurn` helper, so a zero-turn, errored, or unparseable result is an error and persists nothing. +- Confirms child exit error, 30-minute bound expiry, and context cancellation all return an error so the caller persists no session id, and that the detached child survives parent timeout and cancellation (a wait bound, never a kill). +- Confirms the interactive TTY branch, `defaultCommandRunner`, and the 5-minute cap are unchanged (AC10 guards), and that `mocks/claude-session-starter.go` is untouched. +- Backfill 1: renames the turn-bound test variable `window` to `capturedWindow` in `pkg/ops/claude_session_test.go` so the spec's AC1 evidence grep (`Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`) matches; the assertion already exists under the other name, so this is a pure rename with no behavior change. +- Backfill 2: adds the one genuinely missing test — the temp output file is removed after a clean exit (the AC2 "temp file is removed" half). +- Flags a spec artifact for the reviewer: the spec's AC4/AC5 evidence greps carry a literal trailing double-quote (`'"claude session exited with error"'`, `'"did not complete within"'`) that can NEVER match the real source strings (`"claude session exited with error: %v"`, `"claude session turn did not complete within %v"`). This prompt verifies the unquoted forms instead and forbids editing error strings to force the quoted greps. +- Runs `make test` and the AC10 `git diff --exit-code HEAD` guard for `scenarios/005` (git is available in this container — `.dark-factory.yaml` is `workflow: direct`, no hideGit). @@ -22,23 +23,22 @@ Confirm — and backfill where anything is missing — that the non-interactive Read CLAUDE.md for project conventions. Read fully (in this order): -- `pkg/ops/claude_session.go` — the whole file. This is the file under test. It already contains BOTH the spec-041 session-side (block-until-exit + temp-file capture + shared validation) AND spec 045's precedence inversion (the read+validate happens before the exit-status check inside the child-exit select branch). Do not undo either. +- `pkg/ops/claude_session.go` — the whole file. This is the file under test. - `pkg/ops/export_test.go` — exposes the unexported constant. -- `pkg/ops/claude_session_test.go` — the whole file. The "non-interactive branch" context starts at line 256. The "session lock lifecycle" context (spec 042) starts at line 625 and must stay untouched. +- `pkg/ops/claude_session_test.go` — the whole file; the "non-interactive branch" context starts at line 256. - `pkg/ops/claude_session_detach_test.go` — the detachment integration test. -- `docs/work-on-session-lifecycle.md` — the durable design record this implementation realizes. Note its task-path sections still describe the reverted pre-spawn design — that is prompt 3's job to fix; do NOT edit the doc in this prompt. -- `pkg/ops/session_lock.go` — `SessionLocker` / `NewSessionLockerWithDir` / `ErrSessionBusy` / `SessionLock.Release()` (spec 042 wiring already in the file). +- `docs/work-on-session-lifecycle.md` — the durable design record this implementation realizes (note: its task-path sections were reverted in v0.118.3 — that is prompt 3's job to fix; do NOT edit the doc in this prompt). Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrapf(ctx, ...)` / `errors.Wrap(ctx, ...)` / `errors.Errorf(ctx, ...)` idiom from `github.com/bborbe/errors`; never `fmt.Errorf`, never bare `return err`, never `context.Background()` in `pkg/`. - `/home/node/.claude/plugins/marketplaces/coding/docs/go-concurrency-patterns.md` — why the raw `go func`s in this file are deliberate (documented inline). - `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2/Gomega conventions. -IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked, a character device). Do NOT run any `git` command — not to diff, not to check scenario 005. The AC10 `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` guard is an OPERATOR-side check (spec Verification ladder); the container-side proxies are the `defaultCommandRunner` / `context.WithTimeout` greps below. +NOTE: git IS available in this container (`.dark-factory.yaml` is `workflow: direct`, no hideGit). The AC10 `git diff --exit-code HEAD -- scenarios/005-...` guard runs here — it is NOT operator-side. -The target state for this prompt already exists in the tree (the spec-041 session-side shipped; the v0.118.3 task-side reversion did NOT touch `claude_session.go`, `export_test.go`, or the session tests, and spec 045 has since refined the detached branch). Your job is to CONFIRM each piece matches the spec-041 invariants below, and BACKFILL the two specific gaps named in requirements 8 and 9. Do not rewrite what is already correct — "confirm" means read the actual source and verify it matches; correct only genuine mismatches, which are not expected. +The target state for this prompt already exists in the tree (shipped as commit 247a789; the v0.118.3 task-side reversion did NOT touch `claude_session.go`, `export_test.go`, or the session tests). Your job is to CONFIRM each piece matches the spec-041 Design below, and BACKFILL the two specific gaps named in requirements 8 and 9. Do not rewrite what is already correct — "confirm" means read the actual source and verify it matches; correct only genuine mismatches, which are not expected. 1. **Confirm the constant.** In `pkg/ops/claude_session.go` the unexported constant must be: ```go @@ -50,40 +50,50 @@ The target state for this prompt already exists in the tree (the spec-041 sessio ```go func defaultDetachedRunner(args []string, dir string, stdout *os.File) (<-chan error, error) ``` - It must use `exec.Command` (NOT `exec.CommandContext`), set `cmd.Stdout = stdout` (the caller-owned temp file — the function must NOT close it), set `cmd.Stderr` to an `os.OpenFile(os.DevNull, os.O_WRONLY, 0)` handle (closed only after the child exits, inside the reaper goroutine), set `cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}`, log the spawn audit line (`slog.Info("claude detached spawn started", ...)` with pid), and return a buffered `done` channel (capacity 1) that receives `cmd.Wait()`'s error. If any piece differs, correct it to match; never close the caller-owned stdout file. + It must use `exec.Command` (NOT `exec.CommandContext`), set `cmd.Stdout = stdout` (the caller-owned temp file — the function must NOT close it), set `cmd.Stderr` to an `os.OpenFile(os.DevNull, os.O_WRONLY, 0)` handle (closed only after the child exits, inside the reaper goroutine), set `cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}`, log the spawn audit line (`slog.Info("claude detached spawn started", ...)` with pid), and return a buffered `done` channel (capacity 1) that receives `cmd.Wait()`'s error. `exec.CommandContext` must not appear in this function. If any piece differs, correct it to match; never close the caller-owned stdout file. -3. **Confirm `runDetachedTurn`'s spec-041 invariants (do NOT re-touch spec-045's precedence routing).** The method `func (c *claudeSessionStarter) runDetachedTurn(ctx context.Context, args []string, cwd string) error` must have ALL of these, in order: +3. **Confirm the non-interactive branch.** `StartSession`'s non-interactive path (the `if !isInteractive` branch) must delegate to `runDetachedTurn(ctx, args, cwd)` — a method `func (c *claudeSessionStarter) runDetachedTurn(ctx context.Context, args []string, cwd string) error` — that does, in order: - `outFile, err := os.CreateTemp("", "vault-claude-session-*.json")`; on error wrap with `"create claude output file"`. - Eager unlink + close via `defer` (`_ = os.Remove(outFile.Name())`, `_ = outFile.Close()`) so no temp file survives any return path, including cancel/timeout while the child still holds the fd. - `done, err := c.detachRun(args, cwd, outFile)`; on error wrap with `"start detached claude session"`. - A waiter goroutine: `waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout)`. - - A `select` with these outcomes: - - `case exitErr := <-done:` — the child has exited. It MUST read `outFile` and run `validateSessionTurn` before deciding success. The exit-status-vs-result precedence inside this branch is spec 045's — do NOT change it (the current shape reads + validates, treats a validated result as success even on a non-zero exit, falls back to the exit status only for unparseable output, and logs a `slog.Warn("validated turn result overrides non-zero child exit", ...)` when a validated result wins over a non-zero exit). - - `case err := <-waitCh:` — both outcomes are errors so the caller persists no session id. `err != nil` (ctx cancelled) → `errors.Wrap(ctx, err, "claude session wait cancelled")`; `err == nil` (bound expired) → `errors.Errorf(ctx, "claude session turn did not complete within %v", c.sessionTurnTimeout)`. The child is detached and survives either way. - - The old string `"claude session start timed out"` must not exist anywhere in the repo. `"exited during startup"` must not exist either. - The 041-invariant properties this confirms: StartSession never returns early with a still-running child, every failure path returns an error (never nil on ctx-cancel or bound expiry), and the bound is a wait, never a kill (`exec.Command`, not `CommandContext`). - -4. **Confirm `validateSessionTurn` extraction and both-branch use.** A helper `func validateSessionTurn(ctx context.Context, output []byte) error` must exist and be called from BOTH branches (the interactive branch via `c.runCmd` output at the end of `StartSession`, the non-interactive branch from the read temp file in `runDetachedTurn`). Its checks must be `num_turns > 0` AND `is_error == false` AND `session_id` non-empty. NOTE: spec 045 already reworded its error strings (the current implementation uses `errors.Wrapf(ctx, errClaudeOutputUnparseable, "parse claude output: %v", err)` for unparseable output and a `rejectTurn(ctx, resultText, reason)` helper that produces ` ()` messages like `claude returned num_turns: 0`, `claude reported is_error: true`, `claude returned empty session_id`). These post-045 strings are CORRECT — do NOT reword them back to the pre-045 strings the spec text quotes, and do NOT force the spec's stale `"0 turns"` evidence grep (see ``). The interactive branch must otherwise be byte-identical to today: `defaultCommandRunner` unchanged, the 5m `context.WithTimeout(ctx, 5*time.Minute)` cap, `"claude bootstrap turn timed out after 5m"`, and `"run claude"` wrap. + - A `select` with exactly these outcomes, ALL of which except a clean exit are errors: + - `case exitErr := <-done:` — non-nil → `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`; nil → fall through to read + validate. + - `case err := <-waitCh:` — `err != nil` (ctx cancelled) → `errors.Wrap(ctx, err, "claude session wait cancelled")`; `err == nil` (bound expired) → `errors.Errorf(ctx, "claude session turn did not complete within %v", c.sessionTurnTimeout)`. The child is detached and survives either way. + - After a clean exit: `os.ReadFile(outFile.Name())` (wrap with `"read claude output"`) then `validateSessionTurn(ctx, output)`. + - The old strings `"claude session start timed out"` and `"exited during startup"` must not exist anywhere in the repo. + If the branch differs, rewrite it to the contract above. Do NOT use `exec.CommandContext` here — the child must survive the parent. + +4. **Confirm `validateSessionTurn` extraction.** A helper + ```go + func validateSessionTurn(ctx context.Context, output []byte) error + ``` + must exist and be called from BOTH branches (the interactive branch via `c.runCmd` output, the non-interactive branch from the read temp file). Its checks and error strings must be byte-identical to these: + - `json.Unmarshal` failure → `errors.Wrap(ctx, err, "parse claude output")` + - empty `session_id` → `errors.Errorf(ctx, "claude returned empty session_id")` + - `num_turns == 0` → `errors.Errorf(ctx, "claude returned 0 turns: %s", result.Result)` + - `is_error == true` → `errors.Errorf(ctx, "claude reported error: %s", result.Result)` + - otherwise nil. It must not return nil on a dead session: a `session_id` alone proves nothing. + The interactive branch must otherwise be byte-identical to today: `defaultCommandRunner` unchanged, the 5m `context.WithTimeout(ctx, 5*time.Minute)` cap, `"claude bootstrap turn timed out after 5m"`, and `"run claude"` wrap. 5. **Confirm `export_test.go`.** It must contain ```go const SessionTurnTimeout = sessionTurnTimeout ``` - with a comment noting it is a test-only alias (locks wiring, not value — tests must also assert the literal `30 * libtime.Minute`). The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — leave it untouched. - -6. **Confirm the AC10 guards.** `grep -c 'defaultCommandRunner' pkg/ops/claude_session.go` must be 3 (the two constructors assign `runCmd: defaultCommandRunner` and the func is defined once — pinned so a rework cannot silently drop it) and `grep -c 'context.WithTimeout' pkg/ops/claude_session.go` must be 1 (on the interactive branch only). `mocks/claude-session-starter.go` must be untouched (the `StartSession` signature is unchanged). + with a comment noting it is a test-only alias (locks wiring, not value — tests must also assert the literal `30 * libtime.Minute`). The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — that is expected and must be left untouched. -7. **Confirm the test matrix covers AC1-6 and the detachment integration test.** In `pkg/ops/claude_session_test.go` the "non-interactive branch" context (starts at line 256) must contain specs that cover: - - AC1 — "blocks until the detached child exits": blocking waiter, `doneCh` only fires after a `Consistently(returned, "100ms").ShouldNot(Receive())`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound via `windowCh` and it is asserted to equal `ops.SessionTurnTimeout` AND `30 * libtime.Minute`. +6. **Confirm the test matrix exists.** In `pkg/ops/claude_session_test.go` the "non-interactive branch" context (starts at line 256) must contain specs that cover: + - AC1 — "blocks until the detached child exits" (line 327): blocking waiter, `doneCh` only fires after a `Consistently(returned, "100ms").ShouldNot(Receive())`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound via `windowCh` and it is asserted to equal `ops.SessionTurnTimeout` AND `30 * libtime.Minute`. - AC2 — a clean exit (`doneCh <- nil` with valid JSON written to stdout) returns nil ("passes the session id and name to the detached runner"). - AC3 — "validates the turn and rejects a zero-turn result", "validates the turn and rejects an is_error result", "rejects an unparseable turn result". - - AC4 — "treats a child exit error as an error": error containing `"exit status 1"` AND `"exited with error"`; no assertion anywhere still uses `"exited during startup"`. + - AC4 — "treats a child exit error as an error": error containing `"exit status 1"` AND `"exited with error"`; no assertion anywhere still uses the old `"exited during startup"` string. - AC5 — "treats the turn timeout as an error so no id is persisted": error containing `"did not complete within"`. - AC6 — "treats context cancellation as an error so no id is persisted": error containing `"wait cancelled"` (NOT nil); plus "wraps a spawn failure" for `"start detached claude session"`. - The spec-045 test specs already present in the same context (valid blob + non-zero exit → nil; parsed-but-rejected blob leads with the child's reason; unparseable output names the exit status; timeout-with-valid-blob-on-disk still errors) are spec 045's — confirm they exist, do NOT touch them. The interactive-branch tests (lines 54-254) must be UNCHANGED by you (spec 045 already updated the two that lock its reworded validator strings). The "session lock lifecycle" context (line 625) must also be left untouched. - `pkg/ops/claude_session_detach_test.go` must contain "child outlives a cancelled parent wait": spawns a real script (`#!/bin/sh\nsleep 6\ntouch `), cancels the context after ~500ms, asserts `StartSession` returns an error, asserts the sentinel does NOT exist yet, then `Eventually(..., "20s", "200ms")` asserts the sentinel appears — proving the detached child survived the parent's cancelled wait. It constructs the starter via `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the two-arg form is spec 042's; keep it). If any of these is missing, implement it. + The existing interactive-branch tests (lines 54-254) must be UNCHANGED — they lock the byte-identical validation strings. The "session lock lifecycle" context (spec 042, line 490) must also be left untouched. -8. **BACKFILL — rename the test variable to satisfy AC1's evidence grep.** In `pkg/ops/claude_session_test.go`, inside the "blocks until the detached child exits" spec (starts at line 327), the local variable is currently named `window`. Rename it to `capturedWindow` so the assertion line reads exactly `Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`. The block changes old → new as follows (keep `windowCh` — the channel — as-is; only the bare `window` variable is renamed): +7. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a test ("child outlives a cancelled parent wait", line 24) that spawns a real script (`#!/bin/sh\nsleep 6\ntouch `), cancels the context after ~500ms, asserts `StartSession` returns an error, asserts the sentinel does NOT exist yet, and then `Eventually(..., "20s", "200ms")` asserts the sentinel appears — proving the detached child survived the parent's cancelled wait. The file constructs the starter via `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the two-arg form is spec 042's; keep it). If the file or test is missing, implement it. + +8. **BACKFILL — rename the test variable to satisfy AC1's evidence grep.** In `pkg/ops/claude_session_test.go`, inside the "blocks until the detached child exits" spec (lines 340-347), the local variable is currently named `window`. Rename it to `capturedWindow` so the assertion line reads exactly `Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))`. The block changes old → new as follows (keep `windowCh` — the channel — as-is; only the bare `window` variable is renamed): ```go // OLD var window libtime.Duration @@ -108,7 +118,7 @@ The target state for this prompt already exists in the tree (the spec-041 sessio ``` Do not rename the `windowCh` channel or any other identifier. This is a pure rename; no behavior changes. -9. **BACKFILL — assert the temp output file is removed after a clean exit.** No existing test asserts AC2's "the temp file is removed" half. Add ONE dedicated spec in the "non-interactive branch" context of `pkg/ops/claude_session_test.go` (after "wraps a spawn failure", the last spec in that context, which ends around line 622). `validTurnJSON`, `blockWaiter`, `starter`, `ctx`, and `locker` are all in scope there. This requires adding `path/filepath` to the file's imports (the current import block is: `context`, `errors`, `os`, `time`, `libtime`, `uuid`, ginkgo, gomega, `ops`). Add this spec verbatim: +9. **BACKFILL — assert the temp output file is removed after a clean exit.** No existing test asserts AC2's "the temp file is removed" half. Add ONE dedicated spec in the "non-interactive branch" context of `pkg/ops/claude_session_test.go` (after "wraps a spawn failure", the last spec in that context, which ends at line 487). `validTurnJSON`, `blockWaiter`, `starter`, `ctx`, and `locker` are all in scope there. Add this spec verbatim: ```go It("removes the temp output file after a clean exit", func() { matches := func() []string { @@ -138,25 +148,24 @@ The target state for this prompt already exists in the tree (the spec-041 sessio Expect(matches()).To(Equal(before)) }) ``` - The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the before/after glob counts must be equal. Note the spec overrides `starter` with its own fake (writing `validTurnJSON` then `done <- nil`) so the child-exit branch wins and the blocking waiter goroutine stays parked on `<-bw` until `DeferCleanup` closes `blockWaiter` — the established pattern in this context. + This requires adding `path/filepath` to the file's imports (the current import block is: `context`, `errors`, `os`, `time`, `libtime`, `uuid`, ginkgo, gomega, `ops`). The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the before/after glob counts must be equal. Note the spec overrides `starter` with its own fake (writing `validTurnJSON` then `done <- nil`) so the child-exit branch wins and the blocking waiter goroutine stays parked on `<-bw` until `DeferCleanup` closes `blockWaiter` — the established pattern in this context. -10. **Self-check against AC1-6 and AC10.** Before finishing, re-read the changed hunks and walk each AC: the constant, the wait-select, the validation helper, the rename, and the new cleanup test. Run the `` block and confirm every grep that is expected to pass does pass; the two greps flagged in `` as spec artifacts must NOT be "fixed" by editing error strings. +10. **Self-check against AC1-6 and AC10.** Before finishing, re-read the changed hunks and walk each AC: the constant, the wait-select, the validation helper, the rename, and the new cleanup test. Run the `` block and confirm every grep that is expected to pass does pass; the two greps flagged in `` as spec-quoting artifacts must NOT be "fixed" by editing error strings. -Failure-mode coverage from the spec's table: bound expiry (row 1, AC5 test), ctx cancel mid-wait with child survival (row 2, AC6 unit + detach integration), child exits non-zero (row 3, AC4 test — with the post-045 refinement, the unparseable-output case is what this assertion covers), turn JSON is_error / 0 turns (row 4, AC3 tests), temp file unreadable/empty (row 5, AC3 unparseable test), UI request timeout < turn (row 8, AC6 cancel path). Each is covered by the corresponding test in this prompt. +Failure-mode coverage from the spec's table: bound expiry (row 1, AC5 test), ctx cancel mid-wait with child survival (row 2, AC6 unit + detach integration), child exits non-zero (row 3, AC4 test), turn JSON is_error / 0 turns (row 4, AC3 tests), temp file unreadable/empty (row 5, AC3 unparseable test), UI request timeout < turn (row 8, AC6 cancel path). Each is covered by the corresponding test in this prompt. -- Do NOT commit — dark-factory handles git. -- Interactive branch behavior unchanged. `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only permitted interactive-branch edit is none — the shared `validateSessionTurn` is already extracted and called from both branches; do NOT re-extract or reword it. +- Do NOT commit — dark-factory handles git. `git diff --exit-code HEAD` reads only; do not stage or commit anything. +- Interactive branch behavior unchanged. `defaultCommandRunner`, the 5m TTY cap, and `scenarios/005-work-on-resume-auto-invokes-subtask.md` are untouched. The only permitted interactive-branch edit is the already-extracted `validateSessionTurn` call — behavior-preserving, same checks, byte-identical error strings. Do NOT re-extract or change it. - Detachment preserved: `exec.Command` (NOT `CommandContext`), `Setpgid`, stdout/stderr handling that lets the child survive the parent. NEVER SIGKILL the child on timeout; `--max-turns` is inert (`-1`), so the 30-min bound is a wait-channel select, not a context kill. Do NOT resurrect `"claude session start timed out"`. - Never offer a broken Resume: on any failure (exit error, `is_error`, 0 turns, bound expiry, ctx cancel) `StartSession` returns an error. Returning nil on ctx-cancel is wrong — it would persist an id for a still-running child. - JSON validation: `num_turns > 0` AND `is_error == false`. Lowercase UUIDs; keep `-n ""` at mint so resume inherits the title. - Error idiom: `errors.Wrapf(ctx, err, ...)` / `errors.Wrap(ctx, err, ...)` / `errors.Errorf(ctx, ...)` from `github.com/bborbe/errors`; no `fmt.Errorf`; no bare `return err`; no `context.Background()` in `pkg/`. - `sessionTurnTimeout` stays a tunable const — do NOT add a config field (spec Open Question 1 recommends const; no second caller exists). -- Do NOT alter the spec-045 error strings or the exit-status-vs-result precedence to satisfy a grep pattern. The spec's AC3/AC4/AC5 evidence greps are stale against the post-045 validator (see ``); the source strings are correct as written. +- Do NOT alter the error strings to satisfy a grep pattern. The AC4/AC5 evidence greps in the spec's Verification carry a trailing-quote artifact (see ``); the source strings are correct as written. - `ClaudeSessionStarter.StartSession` signature is UNCHANGED (6 args: ctx, sessionID, prompt, cwd, name, isInteractive) — `mocks/claude-session-starter.go` is untouched. The `SessionLocker` constructor parameter (spec 042) is already wired in and must stay. - Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, or `docs/work-on-session-lifecycle.md` in this prompt — the workon reorder is prompt 2, the doc reword is prompt 3. -- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). The scenario-005-untouched check is operator-side. - Existing tests must still pass. @@ -169,7 +178,8 @@ grep -c '30 \* libtime.Minute' pkg/ops/claude_session_test.go # grep -c 'validateSessionTurn' pkg/ops/claude_session.go # >= 2 (both branches call it) grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go # >= 1 — THIS IS BACKFILL REQ 8; must flip 0 -> 1 grep -c 'removes the temp output file after a clean exit' pkg/ops/claude_session_test.go # >= 1 — THIS IS BACKFILL REQ 9; must flip 0 -> 1 -grep -c 'claude session exited with error' pkg/ops/claude_session.go # >= 1 (AC4, real check — unquoted form; the string survives on the unparseable-output path) +grep -c '"0 turns"' pkg/ops/claude_session_test.go # >= 1 (AC3) +grep -c 'claude session exited with error' pkg/ops/claude_session.go # >= 1 (AC4, real check — unquoted form) grep -c 'exited during startup' pkg/ops/claude_session.go # == 0 (AC4) grep -c 'did not complete within' pkg/ops/claude_session.go # >= 1 (AC5, real check — unquoted form) grep -c 'livenessWindow' -r pkg/ # == 0 @@ -177,11 +187,12 @@ grep -c 'defaultCommandRunner' pkg/ops/claude_session.go # grep -c 'context.WithTimeout' pkg/ops/claude_session.go # == 1 (AC10, interactive branch) ``` -Note on spec artifacts you must NOT "fix": -- The spec's AC3 evidence `grep -c '"0 turns"' pkg/ops/claude_session_test.go >= 1` CANNOT match the post-045 validator (`claude returned num_turns: 0`); the tests assert `num_turns` / `claude reported is_error` instead. Do NOT reword the validator to force "0 turns". -- The spec's AC4/AC5 evidence greps carry a literal trailing double-quote (`'"claude session exited with error"'`, `'"did not complete within"'`) that can never match the real source strings (`"claude session exited with error: %v"` / `"claude session turn did not complete within %v"`). The unquoted forms above are the real checks. +Note on spec-quoting artifacts: the spec's AC4/AC5 evidence greps use the literal `'"claude session exited with error"'` and `'"did not complete within"'` (trailing double-quote inside the pattern). Those two forms CANNOT match the real source strings (`"claude session exited with error: %v"` / `"claude session turn did not complete within %v"`), so they read 0 against CORRECT code. Do NOT force them to 1 by editing error strings — the unquoted forms above are the real checks and must pass. -SECONDARY — AC10 scenario-005 guard is OPERATOR-SIDE (git is masked in this container). The container-side proxies are the `defaultCommandRunner` == 3 and `context.WithTimeout` == 1 greps above. +SECONDARY — AC10 git guard (git IS available — workflow `direct`, no hideGit): +``` +git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md # must exit 0 with empty output +``` SYNTAX + TESTS: ``` diff --git a/prompts/2-spec-041-post-exit-persist.md b/prompts/2-spec-041-post-exit-persist.md index 801aba8..ab1b6b1 100644 --- a/prompts/2-spec-041-post-exit-persist.md +++ b/prompts/2-spec-041-post-exit-persist.md @@ -1,17 +1,17 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-06T16:25:00Z" +created: "2026-09-03T10:05:00Z" --- -- Re-applies the spec-041 start→persist reorder to `workon.go`'s fresh-start path: the headless turn is started FIRST and `claude_session_id` + the metrics entry are persisted only AFTER it exits cleanly, structurally matching `goal_workon.go`, which already ships this. -- Deletes the now-dead `clearSessionAndMetrics` compensating-clear function from `workon.go` and its doc references (AC9) — the current tree still carries the pre-spawn persist + compensating clear that AC9 requires removing. +- Re-applies the spec-041 start→persist reorder to `workon.go`'s fresh-start path: the headless turn is started FIRST and `claude_session_id` + the metrics entry are persisted only AFTER it exits cleanly. +- Deletes the now-dead `clearSessionAndMetrics` compensating-clear function from `workon.go` and its doc references (AC9) — the current tree still carries the pre-spawn persist + compensating clear that spec-041's AC9 requires removing. - Reworks `workon_test.go`: the "persisting the session id before spawning" test becomes "after the child exits" asserting `writeTaskAt.After(childExitAt)` (AC7), the "write precedes the spawn" sequencing test inverts, the clear-based failure tests are deleted and replaced with a persists-nothing assertion, and the "pre-spawn persist re-read fails" context becomes "post-exit". - Rewords the stale pre-spawn / liveness-window comments and the clear-based child-exit context in `workon_session_writeback_test.go` to the post-exit no-clear semantics; the writeback fakes already write valid JSON to the stdout file and exit cleanly via `done <- nil` with a blocking waiter, so the AC8 invariant assertions are confirmed unchanged. -- Confirms `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state (the goal AC7 half — `writeGoalAt.After(childExitAt)` — already passes) and are left untouched. -- Coordinates with the in-flight spec-045 batch: prompt 2 of spec 045 adds retain/clear contexts to the SAME two test files this prompt touches. Those added contexts remain valid under the reorder (a successful turn still leaves the id on disk; a failed turn still leaves none) and must be left green and untouched. This prompt must run AFTER spec-045 prompt 2 has landed (see the dependency comment in requirements). -- Runs `make test` + the AC7-9 grep gate. No git in this container (masked); nothing here needs it. +- Confirms `goal_workon.go` and its tests are already in the spec-041 target state (they were never reverted) and are left untouched. +- ⚠️ IMPORTANT TREE CONFLICT FLAGGED FOR THE HUMAN REVIEWER: the task-side half of this spec was REVERTED in the tree after approval (commit dae6563, released v0.118.3) because persist-after-exit left `claude_session_id` empty during the turn and the child's own session-connect scanned the transcript dir by mtime and bound the task to a live unrelated session (reproduced live 2026-09-01). This prompt implements the spec AS APPROVED — re-applying start→persist — and the reviewer must adjudicate the conflict at audit time (details in requirement 2's comment): (A) approve, spec-041 wins and the session-connect regression is owned as a follow-up; or (B) reject and re-scope the spec to treat the reversion as the target. Prompt 3 is coupled to this decision. +- Runs `make test` and the spec-041 AC7-9 grep gate. @@ -26,32 +26,39 @@ Read fully (in this order): - `pkg/ops/goal_workon.go` — the whole file; this is the structural TEMPLATE the reordered task path must match (`handleClaudeSession` at line 198, `persistGoalSessionID` at line 174). - `pkg/ops/workon_test.go` — the whole file; the AC7 contexts are "when persisting the session id before spawning" (line 906), "when the session id write precedes the spawn" (line 160), "when the spawn fails" (line 982), "when the pre-spawn persist re-read fails" (line 877), and the comment-era specs at lines 98-157. - `pkg/ops/goal_workon_test.go` — the whole file; the AC7 context "when persisting the goal session id after the child exits" starts at line 355 (already spec-041 — confirm, do not touch). -- `pkg/ops/workon_session_writeback_test.go` — the whole file. NOTE: if spec-045 prompt 2 has already landed, this file also contains its two added contexts (a task retain spec, a task `is_error` clear spec, and the mirrored goal pair). Those must remain green and untouched. -- `pkg/ops/claude_session.go` — `runDetachedTurn` and `validateSessionTurn` as they now stand (spec-041 + spec-045), so the seeded blobs and expected errors match the real implementation. -- `docs/work-on-session-lifecycle.md` — the design record for the post-exit ordering (its task-path sections still describe the reverted pre-spawn design; prompt 3 fixes the doc — do NOT edit it here). +- `pkg/ops/workon_session_writeback_test.go` — the whole file. +- `docs/work-on-session-lifecycle.md` — the design record for the post-exit ordering (its task-path sections were reverted in v0.118.3; prompt 3 fixes the doc — do NOT edit it here). Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrapf(ctx, ...)` / `errors.Wrap(ctx, ...)` / `errors.Errorf(ctx, ...)` idiom from `github.com/bborbe/errors`. - `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2/Gomega conventions. -IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked). Do NOT run any `git` command. Nothing in this prompt needs it. +NOTE: git IS available in this container (`workflow: direct`, no hideGit) — but this prompt has no git commands; AC10's `scenarios/005` guard is verified in prompt 1. - +The current tree therefore FAILS spec-041 AC7 (grep 'After(childExitAt)' pkg/ops/workon_test.go = 0) and AC9 (grep 'clearSessionAndMetrics' pkg/ops/workon.go = 3). + +This prompt implements the spec AS APPROVED: it re-applies the start→persist reorder to workon.go and deletes clearSessionAndMetrics. That re-introduces the session-connect bug v0.118.3 fixed UNLESS a separate mechanism ships in the same batch (e.g. making the child's session-connect read the id from a non-frontmatter source, or vault-ui writing the id earlier) — none exists in this spec. + +The reviewer must decide at audit time between: + (A) Approve this prompt — spec-041 wins; the reorder is re-applied and the session-connect regression is accepted/owned as a follow-up. + (B) Reject and re-scope spec-041 — treat dae6563 as the new target; AC7/AC9 would need rewording to assert pre-spawn + compensating-clear behavior (which the current tree already satisfies). +If you approve, also approve prompt 3 — it rewrites the doc back to post-exit and is coupled to this decision. + +For the executing agent: implement the requirements below as written — the spec is the source of truth for this batch; the reviewer adjudicates the conflict at audit time. Do not "fix" the requirements to preserve the reversion. --> 1. **Guard — prompt 1 must have shipped.** Before doing anything, confirm prompt 1's deliverables exist: `grep -c 'validateSessionTurn' pkg/ops/claude_session.go` >= 2, `grep -c 'sessionTurnTimeout' pkg/ops/claude_session.go` >= 1, and `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` >= 1. If ANY of these is 0, STOP and report `"status":"failed"` with message `"spec-041 prompt 2 precondition missing: prompt 1 not yet deployed"` — do not proceed. -2. **Reorder the fresh-start path in `workon.go`'s `handleClaudeSession` to start→persist.** The current code (the fresh-start block starting at the `prompt := fmt.Sprintf(...)` line and ending at `return sessionID, nil`) persists BEFORE the spawn and compensates on failure. Replace the block from `prompt := fmt.Sprintf(...)` through `return sessionID, nil` with this exact code (structurally identical to `goal_workon.go`'s non-interactive branch): +2. **Reorder the fresh-start path in `workon.go`'s `handleClaudeSession` to start→persist.** The current code (the fresh-start block starting at the `prompt := fmt.Sprintf(...)` line and ending at `return sessionID, nil`) persists BEFORE the spawn and compensates on failure. Replace the block from `prompt := fmt.Sprintf(...)` through `return sessionID, nil` with this exact code (structurally identical to `goal_workon.go`'s non-interactive branch, and to the interactive branch — same checks, same error strings): ```go prompt := fmt.Sprintf(`%s "%s" --non-interactive`, vault.GetWorkOnCommand(), task.FilePath) sessionID := w.uuidGenerator() @@ -65,7 +72,7 @@ Also note: the task-side half of spec-041 was REVERTED in the tree after approva sessionID, err := persistSessionAndMetrics(ctx, vaultPath, task.Name, sessionID, startedAt, w.taskStorage) return sessionID, err ``` - The old code being replaced (delete these lines entirely): the `// Persist id + metrics BEFORE the child exists...` comment block, the `if _, err := persistSessionAndMetrics(...); err != nil { return "", errors.Wrap(ctx, err, "persist claude session before spawn") }` call, and the `if err := w.starter.StartSession(...); err != nil { ... clearSessionAndMetrics ... }` compensating-clear block. The cached-session path (the `if existing := task.ClaudeSessionID(); existing != ""` branch, which re-reads and re-persists via `persistSessionAndMetrics`) must be UNCHANGED. Note the function returns `(string, error)` — the new `return "", errors.Wrap(...)` is 2 values; `sessionID, err := ...` compiles because `sessionID` is already declared above and `err` is newly introduced in that scope (this is exactly `goal_workon.go` line 236's pattern). Do NOT copy the spec Design's `return "", nil, errors.Wrap(...)` snippet — that 3-value form is a spec typo and does not compile against the real signature. + The old code being replaced (delete these lines entirely): the `// Persist id + metrics BEFORE the child exists...` comment block, the `if _, err := persistSessionAndMetrics(...); err != nil { return "", errors.Wrap(ctx, err, "persist claude session before spawn") }` call, and the `if err := w.starter.StartSession(...); err != nil { ... clearSessionAndMetrics ... }` compensating-clear block. The cached-session path (the `if existing := task.ClaudeSessionID(); existing != ""` branch, which re-reads and re-persists via `persistSessionAndMetrics`) must be UNCHANGED. Note the function returns `(string, error)` — the new `return "", errors.Wrap(...)` is 2 values; `sessionID, err := ...` compiles because `sessionID` is already declared above and `err` is newly introduced in that scope (this is exactly `goal_workon.go` line 224's pattern). Do NOT copy the spec Design's `return "", nil, errors.Wrap(...)` snippet — that 3-value form is a spec typo and does not compile against the real signature. 3. **Delete the dead `clearSessionAndMetrics` function from `workon.go`.** Remove the entire function (its doc comment plus body, currently at lines 243-269). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return NOTHING (AC9). Its only call site was the compensating-clear block deleted in requirement 2. Do not add any replacement. @@ -81,7 +88,7 @@ Also note: the task-side half of spec-041 was REVERTED in the tree after approva Expect(writeTaskAt.After(childExitAt)).To(BeTrue()) Expect(writtenSessionID).To(Equal(spawnedSessionID)) ``` - Keep the existing comment about AC5's "id equals the value in task frontmatter". This yields the AC7 evidence `After(childExitAt)` in `workon_test.go` (currently absent — the old assertion is `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())`). + Keep the existing comment about AC5's "id equals the value in task frontmatter". This yields the AC7 evidence `After(childExitAt)` (currently absent — the old assertion is `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())`). 7. **Invert "when the session id write precedes the spawn" in `workon_test.go` (lines 160-185).** Rename the context to `"when the session id write follows the spawn"`, rename the `It` to `"writes the session id to storage after StartSession returns"`, and change the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. The `WriteTaskStub`/`StartSessionStub` sequencing setup stays as-is. @@ -114,7 +121,6 @@ Also note: the task-side half of spec-041 was REVERTED in the tree after approva - Reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window.` to `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins.` - Reword the mechanism comments that describe the pre-spawn persist + compensating clear so they describe the post-exit no-clear ordering. The on-disk assertions they annotate (phase survives, raw file has no `claude_session_id:`, no `pinnedSessionID`) are byte-identical under the new ordering — a failed turn simply never persisted anything — so DO NOT touch the assertions. - Do NOT touch the pinned-count strings anywhere in this file: `TaskPhaseExecution` (==2), `GoalPhaseExecution` (==2), `session_note` (==4), `MetricsSessions()` (==2), `ClaudeSessionID()` (==2). The AC8 greps must stay byte-identical. - - If spec-045 prompt 2 has already landed, its added retain/clear contexts are present in this file — do NOT edit, rename, or reword them, and confirm they still pass after your reorder (they should, per the sequencing comment at the top of requirements). 12. **Confirm the goal AC7 test and AC8 assertions are already correct — do not touch them.** `goal_workon_test.go` "when persisting the goal session id after the child exits" already asserts `writeGoalAt.After(childExitAt)` with both non-zero. `workon_session_writeback_test.go`'s task and goal `It`s already assert the child's phase + `session_note` survive, `ClaudeSessionID() == pinnedSessionID`, and `MetricsSessions()` length 1. Confirm and leave unchanged. @@ -135,9 +141,8 @@ Also note: the task-side half of spec-041 was REVERTED in the tree after approva - The `ClaudeSessionStarter` interface signature is UNCHANGED — `mocks/claude-session-starter.go` is untouched. `handleClaudeSession`'s `(string, error)` signature is UNCHANGED — the spec Design's `return "", nil, errors.Wrap(...)` snippet is a typo and must NOT be used (it does not compile). - Do NOT add a double-Start guard and do NOT add any config knob (both are spec Non-goals / Open Question 1). - The AC8 pinned-count strings (`TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`) in `workon_session_writeback_test.go` must remain byte-identical — requirement 11's comment reword must not touch any assertion. -- `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state — do not modify them except to confirm. If spec-045 prompt 2's added contexts are present in `goal_workon_test.go`, leave them untouched. +- `goal_workon.go` and `goal_workon_test.go` are already in the spec-041 target state — do not modify them except to confirm. - Do NOT touch `docs/work-on-session-lifecycle.md` in this prompt (prompt 3 rewords it) or `pkg/ops/claude_session.go` (prompt 1 owns it). -- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). - Existing tests must still pass. diff --git a/prompts/3-spec-041-docs-changelog.md b/prompts/3-spec-041-docs-changelog.md index 0e9ca30..d76c021 100644 --- a/prompts/3-spec-041-docs-changelog.md +++ b/prompts/3-spec-041-docs-changelog.md @@ -1,15 +1,15 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-06T16:30:00Z" +created: "2026-09-03T10:10:00Z" --- -- Rewords the task-path sections of `docs/work-on-session-lifecycle.md` to the spec-041 post-exit no-clear ordering: the "Post-exit write ordering" and "Failure path" bodies, and the per-session lock's "detached-child safety property" paragraph. The v0.118.3 task-side reversion rewrote those bodies to pre-spawn persist + compensating clear while leaving the spec-041 headings and intro — this prompt removes that stale content. -- Coordinates with the in-flight spec-045 batch: spec-045 prompt 3 owns the doc's "What the turn timeout does and does not cover" section (the "a non-zero child exit all return an error" sentence becomes "validated result outranks the exit code"). This prompt leaves that section alone and does NOT reword the 045 sentence. Run AFTER spec-045 prompt 3. -- Confirms `scenarios/002-task-lifecycle.md` already says the headless turn blocks until completion (no "~10s" return claim) — AC11. No edit needed. -- Appends the spec-041 bullet under `## Unreleased` in `CHANGELOG.md` (AC12). Today `## Unreleased` exists and carries spec-045's precedence bullet; it holds NO spec-041 bullet, and the `grep -A15 '^## Unreleased' | grep -ci 'resume'` hit that reads 1 today comes from v0.124.0's bullet downstream, not from a 041 bullet. The 041 bullet makes the grep match itself. +- Rewords the task-path sections of `docs/work-on-session-lifecycle.md` back to the spec-041 post-exit ordering: "Post-exit write ordering", "Failure path", and the per-session lock's detached-child safety paragraph describe BOTH paths persisting only after the turn, with no compensating clear — the v0.118.3 reversion rewrote the bodies to pre-spawn + compensating clear while leaving the spec-041 headings, and this prompt removes that stale content. +- Confirms `scenarios/002-task-lifecycle.md` already says the headless turn blocks until completion (no "~10s" return claim) — AC11. +- Appends the spec-041 bullet under `## Unreleased` in `CHANGELOG.md` (AC12). The section exists today (carrying the concurrent spec-044 resolve bullet) but holds no spec-041 bullet, and `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads 0. The requirement is written to create the section if a concurrent change consumes it between now and execution. - Runs the `make precommit` full gate (AC13) as the batch's final validation. +- Coupled to prompt 2: the doc reword describes the post-exit behavior prompt 2 re-applies to `workon.go`. The appended changelog bullet also contradicts the v0.118.3 release note that documented the task-side reversion — a reviewer comment inside requirement 4 flags this tension. - Open questions surfaced for the reviewer: (1) spec Open Question 1 (configurable `sessionTurnTimeout`) is resolved as a tunable const — no config field; (2) spec Open Question 2 (the vault-ui "Creating session… up to 2 minutes" modal copy) is a separate repo and out of scope here — no vault-ui change is made by this prompt. @@ -21,27 +21,23 @@ Confirm — and correct where the tree drifted — that the docs and scenario de Read CLAUDE.md for project conventions. Read fully (in this order): -- `docs/work-on-session-lifecycle.md` — the whole file. This is the file under test. Its intro (lines 3-13) and section headings are already spec-041; the BODIES of "Post-exit write ordering" (lines 32-48), "Failure path" (lines 106-114), and the "detached-child safety property" paragraph (lines 168-176) still describe the reverted pre-spawn + compensating-clear design. If spec-045 prompt 3 has already landed, the "What the turn timeout does and does not cover" section carries its "validated result outranks the exit code" sentence — leave it alone. -- `scenarios/002-task-lifecycle.md` — the whole file. Already spec-041 (the work-on note says the turn blocks until completion). Confirm only. -- `CHANGELOG.md` — read the top ~30 lines fully (the `# Changelog` preamble, `## Unreleased`, and the newest version sections). That is where the AC12 bullet lands; the rest of the file is not needed. +- `docs/work-on-session-lifecycle.md` — the whole file. This is the file under test. +- `scenarios/002-task-lifecycle.md` — the whole file. +- `CHANGELOG.md` — read the top ~50 lines fully (the `# Changelog` header block, `## Unreleased` if present, and the newest version sections `## v0.121.1`, `## v0.121.0`, `## v0.120.0`). That is where the AC12 bullet lands; the rest of the file is not needed. - `pkg/ops/goal_workon.go` — only to confirm the post-exit wording the doc must match (lines 192-238). Do not modify it. Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — `## Unreleased` placement and style rules, prefix requirement (`feat:` / `fix:` / ...), one bullet per logical change. Write `## Unreleased` bullets only, never a version number and never a manifest/tag bump (this repo's release model: the github-releaser owns version bumps/tags post-merge; `make precommit` runs `check-versions` which requires the four version strings aligned, but this prompt does not hand-bump them). -IMPORTANT — git is NOT usable in this container: the daemon runs with `hideGit=true` (`.git` is masked). Do NOT run any `git` command. Nothing in this prompt needs it. +NOTE: git IS available in this container (`workflow: direct`, no hideGit) — but AC10's `scenarios/005` guard is verified in prompt 1; this prompt has no git commands. -The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversion rewrote the BODIES of the "Post-exit write ordering" and "Failure path" sections (and the per-session lock's "detached-child safety property" paragraph) to describe pre-spawn persist + compensating clear on the task path, while the section HEADINGS and the intro still carry the spec-041 post-exit framing. This prompt rewrites those bodies back to the spec-041 target. The scenario is already correct; the one genuinely missing item is the spec-041 bullet under CHANGELOG `## Unreleased` (AC12). +The docs for this prompt are in a DRIFTED state: the v0.118.3 task-side reversion rewrote the BODIES of the "Post-exit write ordering" and "Failure path" sections (and the per-session lock's detached-child safety paragraph) to describe pre-spawn persist + compensating clear on the task path, while the section HEADINGS and the intro still carry the spec-041 post-exit framing. This prompt rewrites those bodies back to the spec-041 target. The scenario is already correct; the one genuinely missing item is the spec-041 bullet under CHANGELOG `## Unreleased` (AC12). - - -1. **Guard — prompts 1 and 2 must have shipped.** Before doing anything, confirm: `grep -c 'After(childExitAt)' pkg/ops/workon_test.go` >= 1 AND `grep -c 'After(childExitAt)' pkg/ops/goal_workon_test.go` >= 1 AND `grep -c 'clearSessionAndMetrics' pkg/ops/workon.go` == 0 AND `grep -c 'Expect(capturedWindow).To(Equal(ops.SessionTurnTimeout))' pkg/ops/claude_session_test.go` >= 1. If ANY is absent, STOP and report `"status":"failed"` with message `"spec-041 prompt 3 precondition missing: prompt 1/2 not yet deployed"` — do not proceed. - -2. **Reword `docs/work-on-session-lifecycle.md`'s task-path sections to the spec-041 post-exit, no-clear ordering.** The intro (lines 3-13, "spec 040, revised by spec 041 ... An id on disk now means the session is resumable") is already correct — keep it. Fix these stale bodies: +2. **Reword `docs/work-on-session-lifecycle.md`'s task-path sections to the spec-041 post-exit, no-clear ordering.** The intro (lines 3-13, "spec 040, revised by spec 041 ... An id on disk now means the session is resumable") is already correct — keep it. Fix these three stale bodies: - **"## Post-exit write ordering" (heading is correct; BODY is stale).** The paragraph currently says `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**: persistSessionAndMetrics runs first, then StartSession... A spawn failure triggers a re-read-based compensating clear...` and `On the task path the pre-spawn re-read before writing is load-bearing`. Reword the body to: ``` On both paths — task (`pkg/ops/workon.go`) and goal (`pkg/ops/goal_workon.go`) — the @@ -78,17 +74,18 @@ Spec-045 prompt 3 rewrites the SAME doc's "What the turn timeout does and does n running unlocked is not targetable. On any failure nothing was persisted, so the id cannot stay resumable-looking. ``` - - **Small term cleanup in "## The per-session lock" → "No stale lock."** The sentence `There are no cleanup sweeps, no compensating clears, and no lock TTL;` contains the reverted-vocabulary term "compensating clears" (it describes the LOCK, but the drift guard in `` pins the term to 0). Reword `no compensating clears` → `no explicit clears`, keeping the rest of the sentence. - - Do NOT touch the other sections (`## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, `## What the turn timeout does and does not cover` — the latter is spec-045 prompt 3's scope, leave it as it stands — and the rest of `## The per-session lock`). In particular the "liveness gating" phrase in the lock's "Lock scope" paragraph is the spec-042 vault-ui follow-on concept, NOT the removed liveness-window concept — leave it. + - Do NOT touch the other sections (`## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, `## What the turn timeout does and does not cover`, and the rest of `## The per-session lock`) — they are already spec-041. In particular the "liveness gating" phrase in the lock's "Lock scope" paragraph is the spec-042 vault-ui follow-on concept, NOT the removed liveness-window concept — leave it. - After the reword, the whole file must contain NO occurrence of the reverted vocabulary: `grep -c 'pre-spawn\|pre-persisted\|pre-persist\|before the child is spawned\|compensating clear' docs/work-on-session-lifecycle.md` must be 0. 3. **Confirm `scenarios/002-task-lifecycle.md` (AC11).** The work-on action note must say the headless turn blocks until completion — it already does (`**Both branches block until the turn completes** ... bounded by a 30m turn timeout` and `A fast return is a FAIL, not a pass`). Verify `grep -c '~10s' scenarios/002-task-lifecycle.md` == 0. If it returns non-zero, replace the "~10s" wording. No other edit to this file. -4. **ADD the spec-041 bullet under the `## Unreleased` section of `CHANGELOG.md` (AC12 — the one genuinely missing item).** Today `## Unreleased` exists and carries spec-045's precedence bullet — do NOT create a second `## Unreleased` section and do NOT touch any existing bullet (045's included). Append ONE new bullet directly after the existing bullet(s) within the same section, so the spec-041 bullet is within the first 15 lines of the section: +4. **ADD the spec-041 bullet under the `## Unreleased` section of `CHANGELOG.md` (AC12 — the one genuinely missing item).** Today `## Unreleased` exists (it carries the concurrent spec-044 `resolve` bullet) — do NOT create a second `## Unreleased` section and do NOT touch any existing bullet. Append ONE new bullet directly after the existing bullet(s) within the same section, so `## Unreleased` reads (existing bullet(s) then) the spec-041 bullet: ``` - fix: non-interactive `task work-on` / `goal work-on` now wait for the detached headless turn to exit before persisting `claude_session_id`, bounded by a 30m turn timeout (a wait bound, never a kill), so the Vault UI offers Resume only against a complete, single-writer transcript; a failed or zero-turn session persists no id. The interactive TTY branch is unchanged. ``` - If `## Unreleased` does NOT exist when you run (a concurrent change may have consumed it into a `## vX.Y.Z` section), CREATE it: insert `## Unreleased` immediately after the `# Changelog` preamble and before the newest `## vX.Y.Z` section, with the spec-041 bullet as its only bullet. Either way, after this edit `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` must read >= 1 from THIS bullet. (Today it reads 1 from v0.124.0's downstream bullet — that does NOT satisfy the intent.) The bullet must contain the substrings "Resume" (case-insensitive) and "wait for the detached headless turn". + If `## Unreleased` does NOT exist when you run (a concurrent change may have consumed it into a `## vX.Y.Z` section), CREATE it: insert `## Unreleased` immediately after the `# Changelog` preamble and before the newest `## vX.Y.Z` section, with the spec-041 bullet as its only bullet. Either way, the spec-041 bullet must appear within the first 15 lines of the `## Unreleased` section so `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads >= 1 from THIS bullet. The bullet must contain the substrings "Resume" (case-insensitive) and "wait for the detached headless turn". Today `grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume'` reads 0; after this edit it must read >= 1. + + 5. **Full gate (AC13).** Run `make precommit` at the repo root. This is the batch's final validation. If it fails on any spec-041-related lint/test, fix it (re-run only the failing target — `make lint`, `make gosec`, `make errcheck`, etc. — then `make precommit` once more). Note the repo's version-alignment rule: `make precommit` runs `check-versions`; do NOT hand-bump the plugin manifests or `git tag` — the github-releaser owns version bumps post-merge. If `check-versions` fails because `## Unreleased` now sits above an un-released `## vX.Y.Z`, that is a pre-existing release-sequencing condition, not a spec-041 defect — report it in `## Improvements` and continue. @@ -97,14 +94,12 @@ Spec-045 prompt 3 rewrites the SAME doc's "What the turn timeout does and does n - Do NOT commit — dark-factory handles git. -- `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it (verified operator-side via git; this container masks `.git`). +- `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it (verified in prompt 1). - Interactive TTY branch unchanged; do not reword any doc text into claiming otherwise. - This prompt does NOT hand-bump the plugin manifests or `git tag` — only the `## Unreleased` bullet is in scope. Write `## Unreleased`, never `## vX.Y.Z`. -- Do NOT delete or reword the existing `## Unreleased` bullets (spec-045's precedence bullet, any concurrent bullet) or any `## vX.Y.Z` section — the spec-041 bullet is APPENDED (or, only if `## Unreleased` is absent, creates the section). -- Do NOT touch the "What the turn timeout does and does not cover" section of the doc — it is spec-045 prompt 3's scope; leave its sentence as it stands. +- Do NOT delete or reword the existing `## Unreleased` bullets (e.g. the concurrent spec-044 `resolve` bullet) or any `## vX.Y.Z` section — the spec-041 bullet is APPENDED (or, only if `## Unreleased` is absent, creates the section). - The "liveness gating" phrase in the per-session lock section is spec-042's vault-ui follow-on concept — leave it; it is not the removed liveness-window concept. - Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, or any `_test.go` file in this prompt — code changes belong to prompts 1 and 2. -- Do NOT run `git` — `.git` is masked in this container (`hideGit=true`). - Existing tests must still pass. @@ -120,12 +115,12 @@ grep -c '~10s' scenarios/002-task-lifecycle.md # == 0 grep -c 'pre-spawn\|pre-persisted\|pre-persist\|before the child is spawned\|compensating clear' docs/work-on-session-lifecycle.md # == 0 # AC12 CHANGELOG — Unreleased exists AND carries the spec-041 bullet: grep -c '^## Unreleased' CHANGELOG.md # >= 1 -grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 — must now match the spec-041 bullet itself (today the hit is v0.124.0's downstream bullet) +grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 — must now match the spec-041 bullet itself (today 0) grep -c 'wait for the detached headless turn' CHANGELOG.md # >= 1 — the spec-041 bullet is present ``` FULL GATE — `make precommit` (AC13): Run `make precommit` at the repo root. It must exit 0. If it fails on a spec-041-related lint/test, fix it (re-run only the failing target, then `make precommit` once more). If `check-versions` fails on a release-sequencing condition unrelated to spec-041, report it in `## Improvements` and re-run after confirming the four version strings are aligned. -AC10's `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` is an OPERATOR-side check (this container masks `.git`); the container-side AC10 proxies are the `defaultCommandRunner` == 3 / `context.WithTimeout` == 1 greps verified in prompt 1. +AC10's `git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md` was verified in prompt 1 (this container has git; the guard passed there). From a0d94493ccafa7e5f45043e4a0295babe4ec9a03 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 6 Sep 2026 17:58:44 +0200 Subject: [PATCH 5/5] fix: split CHANGELOG entry out of released section; log ignored exit code on predicate rejection The merge of origin/master folded this branch's Unreleased bullets into v0.125.1, which is already tagged and released, leaving no Unreleased section for the release watcher to cut. Split them back out; everything from v0.125.1 down is byte-identical to master. Also address two code-review findings in runDetachedTurn: - a non-zero child exit is now logged when the output parsed but failed a predicate, mirroring the existing override log rather than dropping the signal silently - document why the json.Unmarshal error is flattened into the message instead of chained, so the sentinel stays errors.Is-matchable --- CHANGELOG.md | 9 ++++++--- pkg/ops/claude_session.go | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3b6c7f..0a58819 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,13 +8,16 @@ 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. -## 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). +## 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). + - fix: `verify-goal` forward status-consistency rule for `in_progress` goals relaxed — subtasks at `backlog`/`next`/`in_progress`/`completed` are all aligned under an active goal (queued work is normal), matching the documented matrix (goal `in_progress` → task `backlog`/`next`/`in_progress`/`completed`). Previously every subtask had to be `in_progress`/`completed`, false-positiving active goals with queued subtasks. The `completed`-goal → all `completed` rule and the inverse rules are unchanged. ## v0.125.0 diff --git a/pkg/ops/claude_session.go b/pkg/ops/claude_session.go index eb4ce22..691ffb4 100644 --- a/pkg/ops/claude_session.go +++ b/pkg/ops/claude_session.go @@ -284,8 +284,13 @@ func (c *claudeSessionStarter) runDetachedTurn( // 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. Return it unwrapped so the - // child's own result text leads the message (see rejectTurn). + // 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 @@ -320,6 +325,11 @@ func validateSessionTurn(ctx context.Context, output []byte) error { Result string `json:"result"` } if err := json.Unmarshal(output, &result); err != nil { + // 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) }