From 58f7c6ba0aeefd3f0c5cf2f5c144dd798b78f538 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 27 Sep 2026 12:12:33 +0200 Subject: [PATCH 1/5] Add the `task remove-metrics-session` verb (spec 055, prompt 1 of 3) --- CHANGELOG.md | 4 + integration/cli_test.go | 244 ++++++++++++++++++ mocks/remove-metrics-session-operation.go | 116 +++++++++ pkg/cli/cli.go | 51 ++++ pkg/domain/task_frontmatter_metrics.go | 26 ++ pkg/domain/task_frontmatter_metrics_test.go | 75 ++++++ pkg/ops/metrics_session_remove.go | 74 ++++++ pkg/ops/metrics_session_remove_test.go | 136 ++++++++++ prompts/1-spec-041-session-turn-block.md | 20 +- prompts/2-spec-041-post-exit-persist.md | 93 ++++--- prompts/3-spec-041-docs-changelog.md | 57 ++-- ...35-spec-055-remove-metrics-session-verb.md | 191 ++++++++++++++ .../236-spec-055-auditor-guard-sentence.md | 95 +++++++ .../237-spec-055-docs-changelog.md | 93 +++++++ .../055-remove-metrics-session.md} | 6 +- 15 files changed, 1208 insertions(+), 73 deletions(-) create mode 100644 mocks/remove-metrics-session-operation.go create mode 100644 pkg/ops/metrics_session_remove.go create mode 100644 pkg/ops/metrics_session_remove_test.go create mode 100644 prompts/completed/235-spec-055-remove-metrics-session-verb.md create mode 100644 prompts/in-progress/236-spec-055-auditor-guard-sentence.md create mode 100644 prompts/in-progress/237-spec-055-docs-changelog.md rename specs/{remove-metrics-session.md => in-progress/055-remove-metrics-session.md} (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ecb04fd..a4abeac7 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 + +- feat: `vault-cli task remove-metrics-session ` removes every `metrics_sessions` entry carrying the supplied session id from one task, preserving every other entry and every other frontmatter key, and deletes the key outright when the last entry goes. A call whose id matches nothing fails loudly and writes nothing, and an empty, non-UUID or path-bearing id is refused before the task is read. `task set`, `task add` and `task remove` keep refusing `metrics_sessions` unchanged. + ## v0.149.0 - docs: document the rollup baseline file (`docs/baseline-file.md`) — its location and config rules, the frozen five-key frontmatter contract, the mapping of each stored figure to the rollup figure it has an analogue in, the recorded definitional mismatches, and the frozen report layout. diff --git a/integration/cli_test.go b/integration/cli_test.go index 6e574761..83eb5ca6 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -473,6 +473,7 @@ var _ = Describe("vault-cli integration tests", func() { Entry("task add", "task", "add"), Entry("task remove", "task", "remove"), Entry("task append-metrics-session", "task", "append-metrics-session"), + Entry("task remove-metrics-session", "task", "remove-metrics-session"), // Goal subcommands Entry("goal list", "goal", "list"), Entry("goal lint", "goal", "lint"), @@ -2870,6 +2871,249 @@ body line }) }) + Describe("task remove-metrics-session", func() { + const ( + s1 = "11111111-1111-4111-8111-111111111111" + s2 = "22222222-2222-4222-8222-222222222222" + s3 = "33333333-3333-4333-8333-333333333333" + ) + + var vaultPath, configPath string + var cleanup func() + + AfterEach(func() { + if cleanup != nil { + cleanup() + } + }) + + runEntityCommand := func(args ...string) *gexec.Session { + fullArgs := append( + []string{"--config", configPath, "--vault", "test"}, + args..., + ) + session, err := gexec.Start(exec.Command(binPath, fullArgs...), GinkgoWriter, GinkgoWriter) + Expect(err).NotTo(HaveOccurred()) + return session + } + + sha256OfFile := func(path string) string { + data, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + sum := sha256.Sum256(data) + return fmt.Sprintf("%x", sum) + } + + readFile := func(path string) string { + content, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + return string(content) + } + + // countMetricsEntries counts the entries under the `metrics_sessions:` + // frontmatter key as written on disk: every block-sequence item opens with a + // `- session_id:` line. + countMetricsEntries := func(content string) int { + count := 0 + for _, line := range strings.Split(content, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "- session_id:") { + count++ + } + } + return count + } + + // entryLineCount counts the entry-line prefix for one session id. The prefix + // is load-bearing: the fixture also carries the id as `task_identifier`, so a + // bare substring count would match the surviving base key and prove nothing. + entryLineCount := func(content, sessionID string) int { + return strings.Count(content, "- session_id: "+sessionID) + } + + // Every fixture's frontmatter keys are authored in alphabetical order, and + // its metrics_sessions block in the writer's own shape. The storage writer + // re-serializes the whole frontmatter in alphabetical key order on every + // write, so a hand-authored non-alphabetical fixture would be reordered and + // every byte comparison below would be meaningless. + baseFrontmatter := `--- +page_type: task +priority: 1 +status: in_progress +task_identifier: 11111111-1111-4111-8111-111111111111 +--- +body line +` + + twoEntryFrontmatter := `--- +metrics_sessions: + - session_id: 11111111-1111-4111-8111-111111111111 + started_at: "2026-09-01T08:00:00Z" + - session_id: 22222222-2222-4222-8222-222222222222 + started_at: "2026-09-02T08:00:00Z" +page_type: task +priority: 1 +status: in_progress +task_identifier: 11111111-1111-4111-8111-111111111111 +--- +body line +` + + threeEntryDuplicateFrontmatter := `--- +metrics_sessions: + - session_id: 11111111-1111-4111-8111-111111111111 + started_at: "2026-09-01T08:00:00Z" + - session_id: 11111111-1111-4111-8111-111111111111 + started_at: "2026-09-02T08:00:00Z" + - session_id: 33333333-3333-4333-8333-333333333333 + started_at: "2026-09-03T08:00:00Z" +page_type: task +priority: 1 +status: in_progress +task_identifier: 11111111-1111-4111-8111-111111111111 +--- +body line +` + + It("AC1: removes exactly one entry and preserves the survivor's own id and started_at", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + + Eventually(runEntityCommand("task", "remove-metrics-session", "Alpha", s1)). + Should(gexec.Exit(0)) + + after := readFile(taskFile) + Expect(countMetricsEntries(after)).To(Equal(1)) + Expect(entryLineCount(after, s1)).To(Equal(0)) + Expect(entryLineCount(after, s2)).To(Equal(1)) + Expect(after).To(ContainSubstring("2026-09-02T08:00:00Z")) + Expect(after).NotTo(ContainSubstring("2026-09-01T08:00:00Z")) + }) + + It("AC2: removing the last entry deletes the key and leaves every base key and the body unchanged", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + + Eventually(runEntityCommand("task", "remove-metrics-session", "Alpha", s1)). + Should(gexec.Exit(0)) + Eventually(runEntityCommand("task", "remove-metrics-session", "Alpha", s2)). + Should(gexec.Exit(0)) + + after := readFile(taskFile) + Expect(after).NotTo(ContainSubstring("metrics_sessions")) + Expect(after).To(ContainSubstring("page_type: task")) + Expect(after).To(ContainSubstring("priority: 1")) + Expect(after).To(ContainSubstring("status: in_progress")) + Expect(after).To(ContainSubstring("task_identifier: " + s1)) + Expect(after).To(ContainSubstring("body line")) + }) + + It("AC3: a duplicate id is removed in full, leaving the other entry", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": threeEntryDuplicateFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + + Eventually(runEntityCommand("task", "remove-metrics-session", "Alpha", s1)). + Should(gexec.Exit(0)) + + after := readFile(taskFile) + Expect(countMetricsEntries(after)).To(Equal(1)) + Expect(entryLineCount(after, s1)).To(Equal(0)) + Expect(entryLineCount(after, s3)).To(Equal(1)) + }) + + It("AC4: a call matching nothing fails loudly and writes nothing", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + before := sha256OfFile(taskFile) + + session := runEntityCommand("task", "remove-metrics-session", "Alpha", s3) + Eventually(session).Should(gexec.Exit(1)) + stderr := string(session.Err.Contents()) + Expect(stderr).To(ContainSubstring(s3)) + Expect(stderr).To(ContainSubstring("nothing removed")) + + Expect(sha256OfFile(taskFile)).To(Equal(before)) + }) + + It("AC5: an empty, non-UUID or path-bearing session id is refused with nothing written", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + before := sha256OfFile(taskFile) + + for _, sessionID := range []string{"", "not-a-uuid", "../escape"} { + session := runEntityCommand("task", "remove-metrics-session", "Alpha", sessionID) + Eventually(session).Should(gexec.Exit(1)) + Expect(string(session.Err.Contents())). + To(ContainSubstring("expected a well-formed UUID")) + } + + Expect(sha256OfFile(taskFile)).To(Equal(before)) + }) + + It("AC6: task set, add and remove still refuse metrics_sessions and leave the file byte-identical", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + before := sha256OfFile(taskFile) + + for _, args := range [][]string{ + {"task", "set", "Alpha", "metrics_sessions", "x"}, + {"task", "add", "Alpha", "metrics_sessions", "x"}, + {"task", "remove", "Alpha", "metrics_sessions", "x"}, + } { + session := runEntityCommand(args...) + Eventually(session).Should(gexec.Exit(1)) + stderr := string(session.Err.Contents()) + Expect(stderr).To(ContainSubstring("metrics_sessions")) + Expect(stderr).To(ContainSubstring("append-metrics-session")) + } + + Expect(sha256OfFile(taskFile)).To(Equal(before)) + }) + + It("AC7: the JSON output contract holds for a matching and a non-matching id", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": twoEntryFrontmatter, + }) + taskFile := filepath.Join(vaultPath, "Tasks", "Alpha.md") + + matching := runEntityCommand( + "task", "remove-metrics-session", "Alpha", s1, "--output", "json", + ) + Eventually(matching).Should(gexec.Exit(0)) + Expect(string(matching.Out.Contents())).To(ContainSubstring(`"success": true`)) + + nonMatching := runEntityCommand( + "task", "remove-metrics-session", "Alpha", s3, "--output", "json", + ) + Eventually(nonMatching).Should(gexec.Exit(0)) + Expect(string(nonMatching.Out.Contents())).To(ContainSubstring(`"success": false`)) + + Expect(countMetricsEntries(readFile(taskFile))).To(Equal(1)) + }) + + It("registers the verb and prints its own help", func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": baseFrontmatter, + }) + Expect(vaultPath).NotTo(BeEmpty()) + + session := runEntityCommand("task", "remove-metrics-session", "--help") + Eventually(session).Should(gexec.Exit(0)) + Expect(string(session.Out.Contents())).To(ContainSubstring("remove-metrics-session")) + }) + }) + Describe("vault-cli defer", func() { var vaultPath, configPath string var cleanup func() diff --git a/mocks/remove-metrics-session-operation.go b/mocks/remove-metrics-session-operation.go new file mode 100644 index 00000000..8e4eddbf --- /dev/null +++ b/mocks/remove-metrics-session-operation.go @@ -0,0 +1,116 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package mocks + +import ( + "context" + "sync" + + "github.com/bborbe/vault-cli/pkg/ops" +) + +type RemoveMetricsSessionOperation struct { + ExecuteStub func(context.Context, string, string, string) error + executeMutex sync.RWMutex + executeArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + } + executeReturns struct { + result1 error + } + executeReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *RemoveMetricsSessionOperation) Execute(arg1 context.Context, arg2 string, arg3 string, arg4 string) error { + fake.executeMutex.Lock() + ret, specificReturn := fake.executeReturnsOnCall[len(fake.executeArgsForCall)] + fake.executeArgsForCall = append(fake.executeArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + }{arg1, arg2, arg3, arg4}) + stub := fake.ExecuteStub + fakeReturns := fake.executeReturns + fake.recordInvocation("Execute", []interface{}{arg1, arg2, arg3, arg4}) + fake.executeMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *RemoveMetricsSessionOperation) ExecuteCallCount() int { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + return len(fake.executeArgsForCall) +} + +func (fake *RemoveMetricsSessionOperation) ExecuteCalls(stub func(context.Context, string, string, string) error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = stub +} + +func (fake *RemoveMetricsSessionOperation) ExecuteArgsForCall(i int) (context.Context, string, string, string) { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + argsForCall := fake.executeArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *RemoveMetricsSessionOperation) ExecuteReturns(result1 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + fake.executeReturns = struct { + result1 error + }{result1} +} + +func (fake *RemoveMetricsSessionOperation) ExecuteReturnsOnCall(i int, result1 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + if fake.executeReturnsOnCall == nil { + fake.executeReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.executeReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *RemoveMetricsSessionOperation) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *RemoveMetricsSessionOperation) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ ops.RemoveMetricsSessionOperation = new(RemoveMetricsSessionOperation) diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index ee9cb2d0..83b1ecbb 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -1342,6 +1342,7 @@ func createTaskCommands( }, )) cmd.AddCommand(createTaskAppendMetricsSessionCommand(ctx, configLoader, vaultName, outputFormat)) + cmd.AddCommand(createTaskRemoveMetricsSessionCommand(ctx, configLoader, vaultName, outputFormat)) cmd.AddCommand(createTaskWatchCommand(ctx, configLoader, vaultName)) cmd.AddCommand( createGenericSearchCommand( @@ -2457,6 +2458,56 @@ func createTaskAppendMetricsSessionCommand( } } +//nolint:dupl,gocognit,nestif // Mutation commands have similar structure but different operations +func createTaskRemoveMetricsSessionCommand( + ctx context.Context, + configLoader *config.Loader, + vaultName *string, + outputFormat *string, +) *cobra.Command { + return &cobra.Command{ + Use: "remove-metrics-session ", + Short: "Remove every metrics_sessions entry carrying one session id from a task", + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + taskName := args[0] + sessionID := args[1] + + vaults, err := getVaults(ctx, configLoader, vaultName) + if err != nil { + return errors.Wrap(ctx, err, "get vaults") + } + + dispatcher := ops.NewVaultDispatcher() + err = dispatcher.FirstSuccess(ctx, vaults, func(vault *config.Vault) error { + storageConfig := storage.NewConfigFromVault(vault) + removeOp := ops.NewRemoveMetricsSessionOperation( + storage.NewTaskStorage(storageConfig), + ) + return removeOp.Execute(ctx, vault.Path, taskName, sessionID) + }) + if err != nil { + if OutputFormat(*outputFormat).IsJSON() { + return PrintJSON(map[string]any{ + "success": false, + "error": err.Error(), + }) + } + return err + } + if OutputFormat(*outputFormat).IsJSON() { + return PrintJSON(map[string]any{ + "success": true, + "name": taskName, + "session_id": sessionID, + }) + } + fmt.Printf("✅ Removed metrics session %s from: %s\n", sessionID, taskName) + return nil + }, + } +} + func createTaskSetCommand( ctx context.Context, configLoader *config.Loader, diff --git a/pkg/domain/task_frontmatter_metrics.go b/pkg/domain/task_frontmatter_metrics.go index 46ee43b5..78593713 100644 --- a/pkg/domain/task_frontmatter_metrics.go +++ b/pkg/domain/task_frontmatter_metrics.go @@ -51,6 +51,32 @@ func (f *TaskFrontmatter) AppendMetricsSession(entry MetricsSession) { f.Set("metrics_sessions", append(f.MetricsSessions(), entry)) } +// RemoveMetricsSession removes every "metrics_sessions" entry whose SessionID +// equals sessionID, preserving every other entry in order. When the last entry +// goes, the key is deleted rather than left as an empty list. Returns the number +// of entries removed; 0 means nothing matched and nothing was written. +func (f *TaskFrontmatter) RemoveMetricsSession(sessionID string) int { + sessions := f.MetricsSessions() + kept := make([]MetricsSession, 0, len(sessions)) + removed := 0 + for _, entry := range sessions { + if entry.SessionID == sessionID { + removed++ + continue + } + kept = append(kept, entry) + } + if removed == 0 { + return 0 + } + if len(kept) == 0 { + f.Delete("metrics_sessions") + return removed + } + f.Set("metrics_sessions", kept) + return removed +} + // ClearMetricsSessions removes the "metrics_sessions" key entirely. func (f *TaskFrontmatter) ClearMetricsSessions() { f.Delete("metrics_sessions") } diff --git a/pkg/domain/task_frontmatter_metrics_test.go b/pkg/domain/task_frontmatter_metrics_test.go index 9d140302..ecd03752 100644 --- a/pkg/domain/task_frontmatter_metrics_test.go +++ b/pkg/domain/task_frontmatter_metrics_test.go @@ -293,6 +293,81 @@ var _ = Describe("TaskFrontmatter metrics", func() { }) }) + Describe("RemoveMetricsSession", func() { + start1 := libtime.DateOrDateTime(time.Date(2026, 9, 1, 8, 0, 0, 0, time.UTC)) + start2 := libtime.DateOrDateTime(time.Date(2026, 9, 2, 8, 0, 0, 0, time.UTC)) + start3 := libtime.DateOrDateTime(time.Date(2026, 9, 3, 8, 0, 0, 0, time.UTC)) + + It("removes the named entry and preserves the survivor's own id and started_at", func() { + fm = domain.NewTaskFrontmatter(map[string]any{ + "metrics_sessions": []domain.MetricsSession{ + {SessionID: "s1", StartedAt: start1}, + {SessionID: "s2", StartedAt: start2}, + }, + }) + + Expect(fm.RemoveMetricsSession("s1")).To(Equal(1)) + + sessions := fm.MetricsSessions() + Expect(sessions).To(HaveLen(1)) + Expect(sessions[0].SessionID).To(Equal("s2")) + Expect(sessions[0].StartedAt).To(Equal(start2)) + }) + + It("removes all entries carrying a duplicated id", func() { + fm = domain.NewTaskFrontmatter(map[string]any{ + "metrics_sessions": []domain.MetricsSession{ + {SessionID: "s1", StartedAt: start1}, + {SessionID: "s1", StartedAt: start2}, + {SessionID: "s3", StartedAt: start3}, + }, + }) + + Expect(fm.RemoveMetricsSession("s1")).To(Equal(2)) + + sessions := fm.MetricsSessions() + Expect(sessions).To(HaveLen(1)) + Expect(sessions[0].SessionID).To(Equal("s3")) + Expect(sessions[0].StartedAt).To(Equal(start3)) + }) + + It("deletes the key when the last entry goes", func() { + fm = domain.NewTaskFrontmatter(map[string]any{ + "metrics_sessions": []domain.MetricsSession{ + {SessionID: "s1", StartedAt: start1}, + }, + }) + + Expect(fm.RemoveMetricsSession("s1")).To(Equal(1)) + Expect(fm.Get("metrics_sessions")).To(BeNil()) + Expect(fm.MetricsSessions()).To(BeNil()) + }) + + It("returns 0 and leaves the field untouched when nothing matches", func() { + fm = domain.NewTaskFrontmatter(map[string]any{ + "metrics_sessions": []domain.MetricsSession{ + {SessionID: "s1", StartedAt: start1}, + }, + }) + before := fm.Get("metrics_sessions") + + Expect(fm.RemoveMetricsSession("absent")).To(Equal(0)) + Expect(fm.Get("metrics_sessions")).To(Equal(before)) + Expect(fm.MetricsSessions()).To(HaveLen(1)) + }) + + It("returns 0 on an absent metrics_sessions key", func() { + Expect(fm.RemoveMetricsSession("s1")).To(Equal(0)) + Expect(fm.Get("metrics_sessions")).To(BeNil()) + }) + + It("returns 0 on a non-list metrics_sessions value", func() { + fm = domain.NewTaskFrontmatter(map[string]any{"metrics_sessions": "not-a-list"}) + Expect(fm.RemoveMetricsSession("s1")).To(Equal(0)) + Expect(fm.Get("metrics_sessions")).To(Equal("not-a-list")) + }) + }) + Describe("Clear metrics", func() { It("clearers delete the keys entirely", func() { d := libtime.DateOrDateTime(time.Date(2026, 8, 24, 9, 0, 0, 0, time.UTC)) diff --git a/pkg/ops/metrics_session_remove.go b/pkg/ops/metrics_session_remove.go new file mode 100644 index 00000000..ed1f2e68 --- /dev/null +++ b/pkg/ops/metrics_session_remove.go @@ -0,0 +1,74 @@ +// Copyright (c) 2026 Benjamin Borbe All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package ops + +import ( + "context" + + "github.com/bborbe/errors" + "github.com/bborbe/validation" + "github.com/google/uuid" + + "github.com/bborbe/vault-cli/pkg/storage" +) + +// RemoveMetricsSessionOperation removes every metrics entry carrying one session +// id from a task. +// +//counterfeiter:generate -o ../../mocks/remove-metrics-session-operation.go --fake-name RemoveMetricsSessionOperation . RemoveMetricsSessionOperation +type RemoveMetricsSessionOperation interface { + Execute(ctx context.Context, vaultPath, taskName, sessionID string) error +} + +// NewRemoveMetricsSessionOperation creates a new remove-metrics-session operation. +func NewRemoveMetricsSessionOperation( + taskStorage storage.TaskStorage, +) RemoveMetricsSessionOperation { + return &removeMetricsSessionOperation{taskStorage: taskStorage} +} + +type removeMetricsSessionOperation struct { + taskStorage storage.TaskStorage +} + +// Execute validates the session id, then removes every entry carrying it from the +// task's metrics_sessions, preserving every other entry in order. When the last +// entry goes, the key is deleted rather than left as an empty list. +// +// The id is validated before the task is read, so an id this verb cannot honour is +// refused with nothing read and nothing written. +// +// A call whose id matches no entry fails loudly and writes nothing at all: WriteTask +// is never called, so the file is byte-identical by construction rather than by a +// rewrite that happens to be stable. The removal goes through the domain +// (domain.TaskFrontmatter.RemoveMetricsSession) — the same reader every other +// consumer uses — so no string-level rewrite of the frontmatter block is possible. +// +// The verb stamps nothing, takes no clock, never writes claude_session_id, and adds +// no --force / --all / --started-at flag: removal is by session id only. +func (o *removeMetricsSessionOperation) Execute( + ctx context.Context, + vaultPath, taskName, sessionID string, +) error { + if err := uuid.Validate(sessionID); err != nil { + return errors.Wrapf(ctx, validation.Error, + "invalid session id %q: expected a well-formed UUID", sessionID) + } + task, err := o.taskStorage.FindTaskByName(ctx, vaultPath, taskName) + if err != nil { + return errors.Wrap(ctx, err, "find task") + } + removed := task.RemoveMetricsSession(sessionID) + if removed == 0 { + return errors.Errorf(ctx, + "no metrics_sessions entry with session id %q on task %q: nothing removed", + sessionID, taskName, + ) + } + if err := o.taskStorage.WriteTask(ctx, task); err != nil { + return errors.Wrap(ctx, err, "write task") + } + return nil +} diff --git a/pkg/ops/metrics_session_remove_test.go b/pkg/ops/metrics_session_remove_test.go new file mode 100644 index 00000000..e8436246 --- /dev/null +++ b/pkg/ops/metrics_session_remove_test.go @@ -0,0 +1,136 @@ +// Copyright (c) 2026 Benjamin Borbe All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package ops_test + +import ( + "context" + + "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + libtimetest "github.com/bborbe/time/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/bborbe/vault-cli/mocks" + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/ops" +) + +var _ = Describe("RemoveMetricsSessionOperation", func() { + const ( + sessionA = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" + sessionB = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" + ) + + var ( + ctx context.Context + err error + mockTaskStorage *mocks.TaskStorage + removeOp ops.RemoveMetricsSessionOperation + ) + + BeforeEach(func() { + ctx = context.Background() + mockTaskStorage = &mocks.TaskStorage{} + mockTaskStorage.FindTaskByNameReturns( + domain.NewTask( + map[string]any{"status": "in_progress"}, + domain.FileMetadata{Name: "Alpha"}, + domain.Content(""), + ), + nil, + ) + mockTaskStorage.WriteTaskReturns(nil) + + removeOp = ops.NewRemoveMetricsSessionOperation(mockTaskStorage) + }) + + // seedTask replaces the task the storage returns, so a spec can control the + // metrics_sessions list the removal filters. + seedTask := func(fields map[string]any) { + mockTaskStorage.FindTaskByNameReturns( + domain.NewTask(fields, domain.FileMetadata{Name: "Alpha"}, domain.Content("")), + nil, + ) + } + + startA := libtime.DateOrDateTime(libtimetest.ParseDateTime("2026-09-01T08:00:00Z").Time()) + startB := libtime.DateOrDateTime(libtimetest.ParseDateTime("2026-09-02T08:00:00Z").Time()) + + It("writes the survivors once when the id matches", func() { + seedTask(map[string]any{ + "status": "in_progress", + "metrics_sessions": []domain.MetricsSession{ + {SessionID: sessionA, StartedAt: startA}, + {SessionID: sessionB, StartedAt: startB}, + }, + }) + + err = removeOp.Execute(ctx, "/vault", "Alpha", sessionA) + Expect(err).To(BeNil()) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(1)) + + _, written := mockTaskStorage.WriteTaskArgsForCall(0) + Expect(written.MetricsSessions()).To(HaveLen(1)) + Expect(written.MetricsSessions()[0].SessionID).To(Equal(sessionB)) + Expect(written.MetricsSessions()[0].StartedAt).To(Equal(startB)) + }) + + It("does not write when the id matches nothing", func() { + seedTask(map[string]any{ + "status": "in_progress", + "metrics_sessions": []domain.MetricsSession{ + {SessionID: sessionA, StartedAt: startA}, + }, + }) + + err = removeOp.Execute(ctx, "/vault", "Alpha", sessionB) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(sessionB)) + Expect(err.Error()).To(ContainSubstring("nothing removed")) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(0)) + }) + + assertRefused := func(sessionID string) { + err = removeOp.Execute(ctx, "/vault", "Alpha", sessionID) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("expected a well-formed UUID")) + Expect(mockTaskStorage.FindTaskByNameCallCount()).To(Equal(0)) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(0)) + } + + It("refuses an empty session id before reading the task", func() { + assertRefused("") + }) + + It("refuses a session id that is not a UUID", func() { + assertRefused("not-a-uuid") + }) + + It("refuses a session id carrying a path separator", func() { + assertRefused("../escape") + }) + + It("wraps a find failure", func() { + mockTaskStorage.FindTaskByNameReturns(nil, errors.New(ctx, "boom")) + err = removeOp.Execute(ctx, "/vault", "Alpha", sessionA) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("find task")) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(0)) + }) + + It("wraps a write failure", func() { + seedTask(map[string]any{ + "status": "in_progress", + "metrics_sessions": []domain.MetricsSession{ + {SessionID: sessionA, StartedAt: startA}, + }, + }) + mockTaskStorage.WriteTaskReturns(errors.New(ctx, "boom")) + err = removeOp.Execute(ctx, "/vault", "Alpha", sessionA) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("write task")) + }) +}) diff --git a/prompts/1-spec-041-session-turn-block.md b/prompts/1-spec-041-session-turn-block.md index 4621a8cb..ce0de2c4 100644 --- a/prompts/1-spec-041-session-turn-block.md +++ b/prompts/1-spec-041-session-turn-block.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-17T15:58:28Z" +created: "2026-09-27T08:54:45Z" --- # Confirm the non-interactive session start blocks until the headless turn exits (spec 041, prompt 1 of 3) @@ -13,7 +13,7 @@ created: "2026-09-17T15:58:28Z" - Confirms the interactive terminal branch, its 5-minute cap, and the resume scenario are unchanged. - Backfill 1: renames one test-local variable so the spec's pinned evidence grep for the turn bound matches. The assertion already exists under the old name, so this is a rename with no behaviour change. - Backfill 2: adds the one genuinely missing test — that the turn's temporary output file is deleted after a clean exit. -- Flags spec evidence greps that can never match the real source: three of them carry a literal double quote the source does not contain, and one is stale. This prompt verifies the correct unquoted forms instead, and forbids editing error strings to force a broken grep to pass. +- Flags spec evidence greps that can never match the real source: three of them carry a literal double quote the source does not contain. This prompt verifies the correct unquoted forms instead, and forbids editing error strings to force a broken grep to pass. - Flags that the spec's error-string list for the shared validation helper is pre-spec-045 and must NOT be applied — the shipped strings are the current contract, and "restoring" the spec's forms would revert a later fix. - Makes no production-code change: verification plus two test-only backfills. @@ -25,7 +25,7 @@ Prove — and backfill the two gaps in — the already-shipped half of spec 041: Read `CLAUDE.md` for project conventions. -**Read this first — the spec's Design section is stale on two points.** `specs/in-progress/041-bug-resume-races-live-headless-turn.md` was written against v0.116.4. The `claude_session.go` half of it shipped as commit `247a789` and was then refined by spec 042 (per-session flock locker) and spec 045 (a validated turn result outranks a non-zero child exit). The spec's Design still quotes the pre-045 shapes — in particular its `validateSessionTurn` error strings and its bare `case exitErr := <-done:` handler. Those are superseded; see requirements 4 and 5. Do NOT "restore" them. +**Read this first — the spec's Design section is stale on two points.** `specs/in-progress/041-bug-resume-races-live-headless-turn.md` was written against v0.116.4. The `claude_session.go` half of it shipped as commit `247a789` and was then refined by spec 042 (per-session flock locker) and spec 045 (a validated turn result outranks a non-zero child exit), plus a later SC5 fix (`5c2fc57`, the child's reason leads the error). The spec's Design still quotes the pre-045 shapes — in particular its `validateSessionTurn` error strings and its bare `case exitErr := <-done:` handler. Those are superseded; see requirements 4 and 6. Do NOT "restore" them. Read fully (in this order): - `pkg/ops/claude_session.go` — the whole file (364 lines). This is the file under test. @@ -67,7 +67,7 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read - A waiter goroutine whose only send is `waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout)`, on a `waitCh` buffered with capacity 1. - A `select` over `done` and `waitCh`. The read of the output file MUST live inside the `done` branch and must 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. There is a unit test locking this ("fails on timeout even when a valid blob is already on disk"); keep it. -4. **Confirm the `done` branch's current (post-spec-045) shape — do NOT replace it with the spec's simpler form.** The spec's Design says a non-nil `exitErr` should immediately return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. That is NOT the shipped contract; spec 045 refined it so a validated turn result outranks a non-zero child exit. The shipped order is: +4. **Confirm the `done` branch's current (post-spec-045, post-`5c2fc57`) shape — do NOT replace it with the spec's simpler form.** The spec's Design says a non-nil `exitErr` should immediately return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. That is NOT the shipped contract; spec 045 refined it so a validated turn result outranks a non-zero child exit. The shipped order is: - Read the file (`errors.Wrap(ctx, readErr, "read claude output")` on failure), then call `validateSessionTurn(ctx, output)`. - If validation returns nil → return nil. If `exitErr` is non-nil, log it (`slog.Warn("validated turn result overrides non-zero child exit", ...)`) rather than swallowing it silently. - If validation failed AND `exitErr != nil` AND `errors.Is(validateErr, errClaudeOutputUnparseable)` → return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. There is no usable result, so the child's exit status is the only reason that can be named. @@ -94,10 +94,10 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read ```go const SessionTurnTimeout = sessionTurnTimeout ``` - with a comment noting it is a test-only alias: asserting against it locks the WIRING (StartSession hands the constant, not a stray literal) but NOT the value, because a retune moves both sides — so tests must also assert the literal `30 * libtime.Minute`. The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — that is expected; leave it untouched. + with a comment noting it is a test-only alias: asserting against it locks the WIRING (StartSession hands the constant, not a stray literal) but NOT the value, because a retune moves both sides — so tests must also assert the literal `30 * libtime.Minute`. The file also carries exports that belong to other specs — `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042) and `var RollupFamilyName` / `var RollupMedian` (spec 049). All of them are expected; leave them untouched. This prompt touches nothing in the file. -9. **Confirm the existing test matrix in `pkg/ops/claude_session_test.go`.** In `Context("non-interactive branch", ...)` confirm these specs exist and match: - - "blocks until the detached child exits" — a blocking waiter, `Consistently(returned, "100ms").ShouldNot(Receive())` before `doneCh <- nil`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound through `windowCh` and it is asserted equal to BOTH `ops.SessionTurnTimeout` and `30 * libtime.Minute`. +9. **Confirm the existing test matrix in `pkg/ops/claude_session_test.go`.** In `Context("non-interactive branch", ...)` (opens at line 256) confirm these specs exist and match: + - "blocks until the detached child exits" (line ~327) — a blocking waiter, `Consistently(returned, "100ms").ShouldNot(Receive())` before `doneCh <- nil`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound through `windowCh` and it is asserted equal to BOTH `ops.SessionTurnTimeout` and `30 * libtime.Minute`. - "passes the session id and name to the detached runner" — clean exit with valid JSON returns nil, and argv carries `--session-id`, `--print`, `-n`, the name, and the cwd. - "validates the turn and rejects a zero-turn result", "validates the turn and rejects an is_error result", "rejects an unparseable turn result". - "treats a child exit error as an error" — the error contains `"exit status 1"` AND `"exited with error"`. No assertion anywhere may still use `"exited during startup"`. @@ -159,9 +159,9 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read Expect(statErr).To(HaveOccurred()) }) ``` - Notes: capture `bw := blockWaiter` spec-locally 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. The fake writes valid JSON and then feeds `done`, so the child-exit branch wins and the waiter goroutine stays parked until `DeferCleanup` closes `blockWaiter`. The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the `os.Stat` after the call must fail. No new import is needed — `os` is already imported. + Notes: capture `bw := blockWaiter` spec-locally 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. The fake writes valid JSON and then feeds `done`, so the child-exit branch wins and the waiter goroutine stays parked until `DeferCleanup` closes `blockWaiter`. The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the `os.Stat` after the call must fail. No new import is needed — `os` is already imported. `libtime.WaiterDurationFunc` is the real type from `github.com/bborbe/time` (`time_waiter-duration.go`); the closure signature `func(context.Context, libtime.Duration) error` must match it exactly. -12. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a spec ("child outlives a cancelled parent wait") that writes a real shell 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. It constructs the starter with the two-argument form `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the locker is spec 042's; keep it). If the file or spec is missing, report `"status":"failed"` — do not re-implement from the spec, whose snippet uses a 12s script and a 1s cancel, neither of which matters to the invariant. +12. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a spec ("child outlives a cancelled parent wait") that writes a real shell 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. It constructs the starter with the two-argument form `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the locker is spec 042's; keep it). If the file or spec is missing, report `"status":"failed"` — do not re-implement from the spec, whose snippet uses a 12s script and a 1s cancel, neither of which matters to the invariant. 13. **Confirm the AC10 guards by reading, then by grep.** `defaultCommandRunner` is defined once and referenced by both constructors — the grep count in `pkg/ops/claude_session.go` must be exactly 3. `context.WithTimeout` must appear exactly once, on the interactive branch. `scenarios/005-work-on-resume-auto-invokes-subtask.md` must be byte-identical to `HEAD` (see ``). `mocks/claude-session-starter.go` must be untouched: `ClaudeSessionStarter.StartSession`'s signature is `StartSession(context.Context, string, string, string, string, bool) error` — six parameters, unchanged. @@ -179,7 +179,7 @@ Failure-mode coverage carried by this prompt (spec's Failure Modes table): row 1 - 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 constant — do NOT add a config field (spec Non-goals and Open Question 1 both forbid it). - Do NOT alter error strings to satisfy a grep pattern. Three of the spec's evidence greps carry a literal double quote the source does not contain (see ``); the source strings are correct as written. -- Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, `pkg/ops/workon_test.go`, `pkg/ops/goal_workon_test.go`, `pkg/ops/workon_session_writeback_test.go`, or `docs/work-on-session-lifecycle.md` in this prompt — the caller-side reorder is prompt 2 and the doc reword is prompt 3. +- Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, `pkg/ops/workon_test.go`, `pkg/ops/goal_workon_test.go`, `pkg/ops/workon_session_writeback_test.go`, `agents/work-on-task-assistant.md`, or `docs/work-on-session-lifecycle.md` in this prompt — the caller-side reorder is prompt 2 and the doc reword is prompt 3. - Do NOT add a double-Start guard and do NOT add any config knob — both are spec Non-goals. - Existing tests must still pass. diff --git a/prompts/2-spec-041-post-exit-persist.md b/prompts/2-spec-041-post-exit-persist.md index ceabc8e2..de18e027 100644 --- a/prompts/2-spec-041-post-exit-persist.md +++ b/prompts/2-spec-041-post-exit-persist.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-17T15:58:28Z" +created: "2026-09-27T08:54:45Z" --- # Persist the task session id only after the headless turn exits (spec 041, prompt 2 of 3) @@ -13,7 +13,7 @@ created: "2026-09-17T15:58:28Z" - Rewords the stale pre-spawn and liveness-window comments in the write-back test file to the post-exit, no-clear semantics. Every on-disk assertion in that file stays byte-identical — the ids never land either way, so only the prose and titles change. - Confirms the goal path and its tests are already in the target state and leaves them untouched. - Runs the spec's AC7-9 evidence gate plus the full repository gate. -- IMPORTANT FLAG FOR THE HUMAN REVIEWER: the task-side half of this spec was deliberately REVERTED in the tree after the spec was approved, because persisting only after the turn left the field empty while the child ran and the child's own session-connect then bound the task to a live, unrelated session. This prompt implements the spec as approved and re-applies the reversion's opposite; the reviewer must adjudicate at audit time (details and the two options are in the comment at the top of ``). The docs prompt is coupled to that decision. +- IMPORTANT FLAG FOR THE HUMAN REVIEWER: the task-side half of this spec was deliberately REVERTED in the tree after the spec was approved, because persisting only after the turn left the field empty while the child ran and the child's own session-connect then bound the task to a live, unrelated session. This prompt implements the spec as approved and re-applies the reversion's opposite; the reviewer must adjudicate at audit time. The comment at the top of `` carries the full current evidence — including that the specific mechanism the reversion cited (an mtime transcript scan) was itself removed from the agent definition, while that definition still documents the pre-spawn pre-set as load-bearing. The docs prompt is coupled to this decision. @@ -24,11 +24,12 @@ Make the task path of `work-on` persist `claude_session_id` only after the detac Read `CLAUDE.md` for project conventions. Read fully (in this order): -- `pkg/ops/workon.go` — the whole file (404 lines). Focus on `handleClaudeSession`, `persistSessionAndMetrics`, `clearSessionAndMetrics`, and `sessionFailureResult`. +- `pkg/ops/workon.go` — the whole file (404 lines). Focus on `handleClaudeSession` (line ~281), `persistSessionAndMetrics` (line ~221), `clearSessionAndMetrics` (line ~250), and `sessionFailureResult` (line ~165). - `pkg/ops/goal_workon.go` — the whole file (240 lines). This is the structural TEMPLATE the reordered task path must match: its `handleClaudeSession` starts the turn first and persists only after it returns cleanly, on both branches, with no compensating clear. -- `pkg/ops/workon_test.go` — the whole file (1096 lines). The contexts this prompt reworks are `"success"`, `"when the session id write precedes the spawn"`, `"when the pre-spawn persist re-read fails"`, `"when persisting the session id before spawning"`, and `"when the spawn fails"`. -- `pkg/ops/goal_workon_test.go` — read `Context("when persisting the goal session id after the child exits", ...)` (~line 356) and `Context("goal work-on persists nothing for a failed turn", ...)` (~line 644) to confirm the goal path is already in the target state. Do not modify this file. +- `pkg/ops/workon_test.go` — the whole file (1096 lines). The contexts this prompt reworks are `"success"` (line 93), `"when the session id write precedes the spawn"` (line 160), `"when the pre-spawn persist re-read fails"` (line 877), `"when persisting the session id before spawning"` (line 906), and `"when the spawn fails"` (line 982). +- `pkg/ops/goal_workon_test.go` — read `Context("when persisting the goal session id after the child exits", ...)` (line 356) and `Context("goal work-on persists nothing for a failed turn", ...)` (line 644) to confirm the goal path is already in the target state. Do not modify this file. - `pkg/ops/workon_session_writeback_test.go` — the whole file (524 lines). Note the shared `newStarter(detachRun)` helper, the `pinnedSessionID` constant, and that every success-path fake already writes a valid turn-JSON line to the caller-owned `stdout *os.File` and then feeds `done <- nil` with a blocking waiter. +- `agents/work-on-task-assistant.md` — read § Session connect (lines ~127-161) and the status-flip line at line 124. This is the agent definition whose contract this reorder interacts with; the reviewer comment in `` quotes it. Do NOT edit it. - `docs/work-on-session-lifecycle.md` — the design record. Its task-path sections currently describe the pre-spawn ordering; prompt 3 rewords them. Do NOT edit the doc here. Coding-plugin docs (in-container paths): @@ -41,23 +42,32 @@ NOTE: git IS available in this container (`.dark-factory.yaml` is `workflow: dir +For the executing agent: implement the requirements below as written. The approved 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, and do NOT add a compensating clear or any other new mechanism that the spec does not name. Do NOT edit agents/work-on-task-assistant.md. --> 1. **Fail-loud gate — prompt 1 must have shipped.** Before changing anything, confirm prompt 1's deliverables exist: ``` @@ -67,7 +77,7 @@ For the executing agent: implement the requirements below as written. The approv ``` 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 and do NOT re-implement prompt 1's work. -2. **Reorder the fresh-start path of `workon.go`'s `handleClaudeSession` to start-then-persist.** Replace the block that begins at the `// Persist id + metrics BEFORE the child exists, on both branches, ...` comment and ends at the function's closing `return sessionID, nil`, with exactly: +2. **Reorder the fresh-start path of `workon.go`'s `handleClaudeSession` to start-then-persist.** Replace the block that begins at the `// Persist id + metrics BEFORE the child exists, on both branches, ...` comment (line ~303) and ends at the function's closing `return sessionID, nil` (line ~321), with exactly: ```go // Captured BEFORE the spawn — the turn's true start, not the write time. startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) @@ -85,14 +95,14 @@ For the executing agent: implement the requirements below as written. The approv The cached-session path at the top of `handleClaudeSession` (the `if existing := task.ClaudeSessionID(); existing != ""` branch) must be UNCHANGED. Note that `persistSessionAndMetrics` is called from two places in this file — the cached path (keep) and the fresh path (this reorder). It has no other callers anywhere in the repo. -3. **Delete the dead `clearSessionAndMetrics` method from `workon.go`.** Remove the whole method — its doc comment and body. Its only call site was the block deleted in requirement 2. Do not add a replacement, and do not add any other clearing mechanism (the spec's Non-goals forbid it). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return nothing. `ClearClaudeSessionID()` on the domain type stays — `pkg/ops/complete.go` still uses it. +3. **Delete the dead `clearSessionAndMetrics` method from `workon.go`.** Remove the whole method — its doc comment and body (lines ~246-272). Its only call site was the block deleted in requirement 2. Do not add a replacement, and do not add any other clearing mechanism (the spec's Non-goals forbid it). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return nothing. `ClearClaudeSessionID()` on the domain type stays — `pkg/ops/complete.go` still uses it. 4. **Reword the `workon.go` doc comments that describe the pre-spawn design.** - `persistSessionAndMetrics`'s comment: replace `Used pre-spawn on the fresh-start path (the session id is new and must be on disk before the child exists) and on the cached-session path (the id already exists and is preserved).` with `Used post-exit on the fresh-start path (the session id is new and is persisted only after the headless turn completes cleanly) and on the cached-session path (the id already exists and is preserved).` Leave the surrounding sentences about the load-bearing re-read and the empty-id rule as they are. - `handleClaudeSession`'s comment: replace it with the wording `goal_workon.go` already uses for the same method — on both branches the session id is persisted only AFTER the headless turn has finished cleanly, so an id on disk means the session is resumable rather than merely that one was started; nothing is written on any failure path, so there is no compensating clear, and frontmatter the child wrote before failing stays untouched so the Vault UI correctly keeps offering Start. Keep the cached-session sentence. - - `sessionFailureResult`'s comment says the warnings are `the accumulated warnings (including any compensating-clear warning)`. Reword that clause — there is no clear, so the warnings are only the assignee and daily-note ones. Do not change the function's behaviour. + - `sessionFailureResult`'s comment (line ~163) says the warnings are `the accumulated warnings (including any compensating-clear warning)`. Reword that clause — there is no clear, so the warnings are only the assignee and daily-note ones. Do not change the function's behaviour. -5. **Confirm `goal_workon.go` is already in the target state — do NOT change it.** Its `handleClaudeSession` must already start the turn first and persist after on both branches, return `errors.Wrap(ctx, err, "start claude session")` with no compensating clear, and keep the cached path as `return existing, nil`. `persistGoalSessionID` must already return an empty id on failure. `clearGoalSession` must not exist (`grep -c 'clearGoalSession' pkg/ops/goal_workon.go` == 0). If it all matches, leave the file untouched. +5. **Confirm `goal_workon.go` is already in the target state — do NOT change it.** Its `handleClaudeSession` (line ~200) must already start the turn first and persist after on both branches, return `errors.Wrap(ctx, err, "start claude session")` with no compensating clear, and keep the cached path as `return existing, nil`. `persistGoalSessionID` must already return an empty id on failure. `clearGoalSession` must not exist (`grep -c 'clearGoalSession' pkg/ops/goal_workon.go` == 0). If it all matches, leave the file untouched. 6. **Rework `"when persisting the session id before spawning"` in `workon_test.go` to post-exit (AC7).** Rename the context to `"when persisting the session id after the child exits"`. In the variable block rename `spawnAt time.Time` to `childExitAt time.Time`, and in the fake `detachRun` rename the `spawnAt = time.Now()` assignment to `childExitAt = time.Now()` — keep it in the same position (immediately before the buffered `done` channel is fed, which is the child's exit point). The `BeforeEach` already captures `spawnedSessionID` from the `--session-id` argv, writes valid turn JSON to the `stdout *os.File`, and uses a blocking waiter; keep all of that unchanged. Rename the `It` to `"writes the session id to storage only after the child exits"` and replace its body with exactly: ```go @@ -108,11 +118,11 @@ For the executing agent: implement the requirements below as written. The approv ``` The old assertion `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())` must be gone. This produces AC7's `After(childExitAt)` evidence. -7. **Invert `"when the session id write precedes the spawn"` in `workon_test.go`.** 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 flip the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. Leave the `WriteTaskStub` / `StartSessionStub` sequencing setup exactly as it is. The ordering is still deterministic: `Execute` writes first (empty id, so `writeSeq` is not set), then `StartSession` runs (`startSeq`), then the post-exit persist writes the id (`writeSeq`). +7. **Invert `"when the session id write precedes the spawn"` in `workon_test.go` (line 160).** 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 flip the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. Leave the `WriteTaskStub` / `StartSessionStub` sequencing setup exactly as it is. The ordering is still deterministic: `Execute` writes first (empty id, so `writeSeq` is not set), then `StartSession` runs (`startSeq`), then the post-exit persist writes the id (`writeSeq`). -8. **Replace the clear-based failure tests in `"when the spawn fails"` with a persists-nothing assertion (AC9).** In that context: +8. **Replace the clear-based failure tests in `"when the spawn fails"` with a persists-nothing assertion (AC9).** In that context (line 982): - Keep `"returns the wrapped spawn error"` and `"returns Success=false"` unchanged. - - DELETE the spec `"clears the pre-persisted session id and the metrics entry for the failed run"` and the whole nested `Context("when the compensating clear itself fails", ...)`. + - DELETE the spec `"clears the pre-persisted session id and the metrics entry for the failed run"` (line 1014) and the whole nested `Context("when the compensating clear itself fails", ...)` (line 1025). - ADD one spec: ```go It("persists no session id when the spawn fails", func() { @@ -123,39 +133,39 @@ For the executing agent: implement the requirements below as written. The approv Expect(writtenMetricsSessions[0]).To(BeEmpty()) }) ``` - - Reword the `BeforeEach` comment that explains the compensating-clear pointer mutation so it reads as a snapshot of what each write lands: with no pre-persist and no clear there is exactly one write, and it carries an empty id because the id is minted inside `handleClaudeSession` after `Execute` has already written the task. Keep the stub itself — it is what makes `writtenIDs` observable. + - Reword the `BeforeEach` comment (lines ~989-990) that explains the compensating-clear pointer mutation so it reads as a snapshot of what each write lands: with no pre-persist and no clear there is exactly one write, and it carries an empty id because the id is minted inside `handleClaudeSession` after `Execute` has already written the task. Keep the stub itself — it is what makes `writtenIDs` observable. -9. **Rework `"when the pre-spawn persist re-read fails"` in `workon_test.go` to post-exit.** Rename the context to `"when the post-exit persist re-read fails"` and the `It` `"does not append a metrics entry when the pre-spawn re-read fails"` to `"does not append a metrics entry when the post-exit re-read fails"`. Reword the `BeforeEach` comment from `The failing call is persistSessionAndMetrics' PRE-SPAWN re-read, which runs before StartSession is ever called.` to `The failing call is persistSessionAndMetrics' POST-EXIT re-read, which runs after StartSession returns (the mock starter returns nil immediately).`, and the comment on the write-count `It` from `pre-spawn persist failed before writing` to `post-exit persist failed before writing`. The mock call indexes do NOT change: call 0 loads the task, `mockStarter.StartSessionReturns(nil)` makes no storage call, call 1 is the post-exit re-read that fails. All four `It` bodies stay byte-identical. +9. **Rework `"when the pre-spawn persist re-read fails"` in `workon_test.go` to post-exit (line 877).** Rename the context to `"when the post-exit persist re-read fails"` and the `It` `"does not append a metrics entry when the pre-spawn re-read fails"` (line 900) to `"does not append a metrics entry when the post-exit re-read fails"`. Reword the `BeforeEach` comment (line 880) from `The failing call is persistSessionAndMetrics' PRE-SPAWN re-read, which runs before StartSession is ever called.` to `The failing call is persistSessionAndMetrics' POST-EXIT re-read, which runs after StartSession returns (the mock starter returns nil immediately).`, and the comment on the write-count `It` (line 896) from `pre-spawn persist failed before writing` to `post-exit persist failed before writing`. The mock call indexes do NOT change: call 0 loads the task, `mockStarter.StartSessionReturns(nil)` makes no storage call, call 1 is the post-exit re-read that fails. All four `It` bodies stay byte-identical. 10. **Reword the remaining pre-spawn wording in `workon_test.go`'s `"success"` context.** Assertions and counts are unchanged; only comments and two titles change: - - `"calls FindTaskByName"` comment: `Twice: once to load the task, once to re-read it before the child is spawned so the fresh session id lands on disk before the child exists.` → `Twice: once to load the task, once to re-read it after the child exits so the post-exit persist lands the fresh session id without reverting the child's frontmatter writes.` - - `"re-reads the task from the vault path before spawning the session"`: rename to `"re-reads the task from the vault path after the child exits"` and reword its comment to `The second FindTaskByName is persistSessionAndMetrics' post-exit re-read: the session id is written to disk only after the child has exited.` - - `"Fresh run records one entry"` comment: `The metrics entry lands in the pre-spawn persist write: write 0 is Execute's status/assignee/phase write, write 1 is the pre-spawn persist.` → `The metrics entry lands in the post-exit persist write: write 0 is Execute's status/assignee/phase write, write 1 is the post-exit persist.` + - `"calls FindTaskByName"` comment (line 112): `Twice: once to load the task, once to re-read it before the child is spawned so the fresh session id lands on disk before the child exists.` → `Twice: once to load the task, once to re-read it after the child exits so the post-exit persist lands the fresh session id without reverting the child's frontmatter writes.` + - `"re-reads the task from the vault path before spawning the session"` (line 110): rename to `"re-reads the task from the vault path after the child exits"` and reword its comment to `The second FindTaskByName is persistSessionAndMetrics' post-exit re-read: the session id is written to disk only after the child has exited.` + - `"Fresh run records one entry"` comment (lines 149-150): `The metrics entry lands in the pre-spawn persist write: write 0 is Execute's status/assignee/phase write, write 1 is the pre-spawn persist.` → `The metrics entry lands in the post-exit persist write: write 0 is Execute's status/assignee/phase write, write 1 is the post-exit persist.` The `Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(2))` assertion stays as it is — Execute's write plus the post-exit persist is still two. 11. **Reword `pkg/ops/workon_session_writeback_test.go` to post-exit semantics — comments and titles ONLY (AC8).** The fakes already write a valid turn-JSON line to the `stdout *os.File` and exit cleanly via `done <- nil` with a blocking waiter; confirm that and do NOT change it. Every on-disk assertion in this file holds unchanged under the new ordering — the ids simply never land — so DO NOT touch any assertion. The full list of prose to change (this is every stale occurrence in the file; the drift-guard grep in `` requires all of them): - - Line ~127-132, the task context's `BeforeEach` comment (work-on persists BEFORE spawning, then the child writes its own frontmatter on top, and nothing writes after) → `Simulate the real headless turn: work-on spawns the child first, then the Claude session runs plan-task -> execute-task and writes its own frontmatter on top of that file inside the detached child (the detachRun fake). Only after the child exits does work-on re-read and persist the session id, so the child's frontmatter survives.` - - Line ~187-189, the task `It`'s comment `The metrics entry lands in the pre-spawn persist write (real storage round-trip) and survives because nothing writes to the file after the child's own write.` → `The metrics entry lands in the post-exit persist write (real storage round-trip), which re-reads the file the child already wrote.` - - Line ~216, the goal context's `BeforeEach` comment clause `while the parent has already returned within the liveness window` → `while the parent blocks waiting for the detached turn`. - - Line ~270, rename `Context("when the child exits non-zero inside the liveness window", ...)` → `Context("when the child exits non-zero within the turn wait", ...)`. - - Line ~286-290, reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window. A nil-returning waiter would race the select against the child's buffered exit; ...` → `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins. A nil-returning waiter would race the select against the child's buffered exit; ...` (keep the rest of the sentence about the blocking waiter). - - Line ~318, rename the `It` `"clears the pre-persisted session id and preserves the child's frontmatter write when the child exited non-zero inside the window"` → `"persists no session id and preserves the child's frontmatter write when the child exited non-zero within the turn wait"`. - - Line ~342-347, reword the body comment (the one describing the pre-spawn persist + compensating clear re-read) so it describes the post-exit, no-clear ordering: the turn failed, so nothing was ever written for this id, and the child's own `phase: planning` write is what survives. - - Line ~353-359, reword the `On-disk shape:` comment (which currently says the pre-spawn persist wrote the id and the compensating clear removed it) to say no id was ever persisted on this path, so its absence from the raw file proves the invariant directly. Keep the explanatory note about the pinned accessor-call counts and the raw-file assertion — that reasoning is unchanged. - - Line ~369-371, reword the comment above `Context("when the child exits non-zero after writing a valid turn result", ...)`: the pre-persisted id is no longer the mechanism — the post-exit persist runs because the validated result outranks the non-zero exit. - - Line ~412, rename the `It` `"retains the pre-persisted session id when the turn result validated despite the non-zero exit"` → `"persists the session id when the turn result validated despite the non-zero exit"`. - - Line ~424-427, reword that `It`'s comment so the retain is attributed to the post-exit persist (the validated result makes `StartSession` return nil, so the persist runs) rather than to a pre-spawn write surviving a clear. - - Line ~463-465, reword the `BeforeEach` comment clause `but the blob reports the turn's own failure, so the compensating clear still fires` → `but the blob reports the turn's own failure, so the turn is rejected and nothing is persisted`. - - Line ~487, rename the `It` `"clears the pre-persisted session id when the turn result reports its own failure"` → `"persists no session id when the turn result reports its own failure"`. - - Line ~514-515, reword the trailing comment (the one about the compensating clear removing the id) to say the turn failed so nothing was ever persisted, while the child's `phase: planning` write survives. + - Line 128, the task context's `BeforeEach` comment (work-on persists BEFORE spawning, then the child writes its own frontmatter on top, and nothing writes after) → `Simulate the real headless turn: work-on spawns the child first, then the Claude session runs plan-task -> execute-task and writes its own frontmatter on top of that file inside the detached child (the detachRun fake). Only after the child exits does work-on re-read and persist the session id, so the child's frontmatter survives.` + - Line 187, the task `It`'s comment `The metrics entry lands in the pre-spawn persist write (real storage round-trip) and survives because nothing writes to the file after the child's own write.` → `The metrics entry lands in the post-exit persist write (real storage round-trip), which re-reads the file the child already wrote.` + - Line 216, the goal context's `BeforeEach` comment clause `while the parent has already returned within the liveness window` → `while the parent blocks waiting for the detached turn`. + - Line 270, rename `Context("when the child exits non-zero inside the liveness window", ...)` → `Context("when the child exits non-zero within the turn wait", ...)`. + - Line 286, reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window. A nil-returning waiter would race the select against the child's buffered exit; ...` → `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins. A nil-returning waiter would race the select against the child's buffered exit; ...` (keep the rest of the sentence about the blocking waiter). + - Line 318, rename the `It` `"clears the pre-persisted session id and preserves the child's frontmatter write when the child exited non-zero inside the window"` → `"persists no session id and preserves the child's frontmatter write when the child exited non-zero within the turn wait"`. + - Lines 342-344, reword the body comment (the one describing the pre-spawn persist + compensating clear re-read) so it describes the post-exit, no-clear ordering: the turn failed, so nothing was ever written for this id, and the child's own `phase: planning` write is what survives. + - Lines 353-354, reword the `On-disk shape:` comment (which currently says the pre-spawn persist wrote the id and the compensating clear removed it) to say no id was ever persisted on this path, so its absence from the raw file proves the invariant directly. Keep the explanatory note about the pinned accessor-call counts and the raw-file assertion — that reasoning is unchanged. + - Line 370, reword the comment above `Context("when the child exits non-zero after writing a valid turn result", ...)`: the pre-persisted id is no longer the mechanism — the post-exit persist runs because the validated result outranks the non-zero exit. + - Line 412, rename the `It` `"retains the pre-persisted session id when the turn result validated despite the non-zero exit"` → `"persists the session id when the turn result validated despite the non-zero exit"`. + - Lines 424-425, reword that `It`'s comment so the retain is attributed to the post-exit persist (the validated result makes `StartSession` return nil, so the persist runs) rather than to a pre-spawn write surviving a clear. + - Line 465, reword the `BeforeEach` comment clause `but the blob reports the turn's own failure, so the compensating clear still fires` → `but the blob reports the turn's own failure, so the turn is rejected and nothing is persisted`. + - Line 487, rename the `It` `"clears the pre-persisted session id when the turn result reports its own failure"` → `"persists no session id when the turn result reports its own failure"`. + - Line 514, reword the trailing comment (the one about the compensating clear removing the id) to say the turn failed so nothing was ever persisted, while the child's `phase: planning` write survives. - Do NOT touch the pinned-count strings anywhere in this file: `TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`. The AC8 greps must keep returning their exact counts, so your reword must not add or remove any occurrence of those tokens. -12. **Confirm the goal AC7 test and the goal assertions are already correct — do not touch them.** `goal_workon_test.go`'s `"when persisting the goal session id after the child exits"` already asserts `writeGoalAt.After(childExitAt)` with both non-zero, and its `"goal work-on persists nothing for a failed turn"` context already proves the no-persist half. Leave the file unmodified. +12. **Confirm the goal AC7 test and the goal assertions are already correct — do not touch them.** `goal_workon_test.go`'s `"when persisting the goal session id after the child exits"` (line 356) already asserts `writeGoalAt.After(childExitAt)` with both non-zero (lines 405-406), and its `"goal work-on persists nothing for a failed turn"` context (line 644) already proves the no-persist half. Leave the file unmodified. -13. **Confirm the spec's Failure Modes rows that this prompt owns.** +13. **Confirm the spec's Failure Modes rows that this prompt owns.** Row numbering follows the spec's table order: - Row 6 (claude binary missing): the `ErrStarterUnavailable` soft path is unchanged — `handleClaudeSession` returns `"", ErrStarterUnavailable` when `w.starter == nil` and `Execute` downgrades it to a warning. Confirm both `workon.go` and `goal_workon.go` still reference it. - Row 7 (post-exit persist fails): covered by requirement 9 — `persistSessionAndMetrics` returns an error AND an empty id when the re-read or the write fails, so the task keeps whatever the turn wrote and no id lands. - - Row 9 (two Start clicks): confirm NO double-start guard was added. It is a documented residual risk and a spec Non-goal. The spec 042 per-session lock already exists and is out of this prompt's scope. + - Row 9 (two Start clicks): confirm NO double-start guard was added. It is a documented residual risk and a spec Non-goal. (The spec's Non-goals section cites this as "Failure Modes row 7" — that citation is off by two; two Start clicks is row 9.) The spec 042 per-session lock already exists and is out of this prompt's scope. 14. **Self-check before finishing.** Re-read every changed hunk and walk spec 041 ACs 7, 8 and 9 against them, naming which artifact satisfies each. Run every command in `` and confirm each holds, including the two flips: `clearSessionAndMetrics` in `workon.go` 3 → 0, and `After(childExitAt)` in `workon_test.go` 0 → ≥ 1. @@ -169,6 +179,7 @@ For the executing agent: implement the requirements below as written. The approv - `ClaudeSessionStarter.StartSession`'s signature is UNCHANGED — six parameters, `mocks/claude-session-starter.go` untouched. `handleClaudeSession`'s `(string, error)` signature is UNCHANGED; the spec's three-value `return "", nil, errors.Wrap(...)` snippet is a typo and must NOT be used. - The pinned-count tokens in `workon_session_writeback_test.go` (`TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`) must remain byte-identical in count — requirement 11's reword is prose-only and must not touch an assertion. - `goal_workon.go` and `goal_workon_test.go` are already in the target state — do not modify them except to read and confirm. +- Do NOT edit `agents/work-on-task-assistant.md`. Its § Session connect text (lines 124, 158) documents the pre-spawn pre-set this reorder removes; updating it is a reviewer-owned follow-up under option (A) of the comment above, and is outside spec 041's stated scope. - Do NOT touch `pkg/ops/claude_session.go`, `pkg/ops/claude_session_test.go`, `pkg/ops/claude_session_detach_test.go`, or `pkg/ops/export_test.go` (prompt 1 owns them), and do NOT touch `docs/work-on-session-lifecycle.md`, `scenarios/002-task-lifecycle.md`, or `CHANGELOG.md` (prompt 3 owns them). - Existing tests must still pass. diff --git a/prompts/3-spec-041-docs-changelog.md b/prompts/3-spec-041-docs-changelog.md index 67c1e761..2d8f5150 100644 --- a/prompts/3-spec-041-docs-changelog.md +++ b/prompts/3-spec-041-docs-changelog.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-17T15:58:28Z" +created: "2026-09-27T08:54:45Z" --- # Reword the work-on lifecycle doc, confirm the scenario, and record the change (spec 041, prompt 3 of 3) @@ -13,6 +13,7 @@ created: "2026-09-17T15:58:28Z" - Does not touch the plugin version strings or any tag — this repository's release agent owns version bumps and tagging after merge, so a feature branch only adds an unreleased bullet. - Runs the repository's full gate as the batch's final validation, and reports the exact exit code. - Coupled to the task-persistence prompt: the document reword and the changelog bullet both describe the ordering that prompt re-applies. If that prompt is rejected at audit, this one must be rejected too, because it would then document behaviour the code does not have. A reviewer comment inside requirement 4 records the tension with the release note that documented the earlier reversion. +- Flags one thing this prompt does NOT fix: the work-on-task agent definition still describes the headless start path as pre-setting the session id before the turn. That text becomes wrong if the reorder ships; it is outside this spec's stated scope and is recorded as a reviewer-owned follow-up. - Confirms the two open questions from the spec are already resolved and need no work: the turn bound stays a constant rather than a config field, and the Vault UI modal copy lives in a different repository and is out of scope. @@ -24,11 +25,12 @@ Bring the durable documentation and the changelog in line with the re-applied po Read `CLAUDE.md` for project conventions. Read fully (in this order): -- `docs/work-on-session-lifecycle.md` — the whole file (205 lines). This is the file under test. +- `docs/work-on-session-lifecycle.md` — the whole file (242 lines). This is the file under test. - `scenarios/002-task-lifecycle.md` — the whole file (67 lines). -- `CHANGELOG.md` — read the top ~40 lines only (the frozen `# Changelog` preamble and the newest versioned sections). That is where the new section and bullet land; the rest of the file is not needed. +- `CHANGELOG.md` — read the top ~20 lines only (the frozen `# Changelog` preamble and the newest versioned section, `## v0.149.0`). That is where the new section and bullet land; the rest of the file is not needed. - `pkg/ops/workon.go` and `pkg/ops/goal_workon.go` — read only to confirm the ordering the documentation must describe. Do not modify either. - `prompts/2-spec-041-post-exit-persist.md` — read the reviewer comment at the top of its `` block; it records the conflict this prompt is coupled to. +- `docs/dod.md` — the repository's Definition of Done; its Documentation section states the changelog placement rule this prompt must satisfy. Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — the unreleased-section placement rule, the frozen-preamble rule, the required conventional prefixes, and the rule that in an auto-release repository a feature branch adds bullets under the unreleased section and does NOT bump version strings. @@ -48,8 +50,8 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th ``` If ANY check fails, STOP and report `"status":"failed"` with message `"spec-041 prompt 3 precondition missing: prompt 2 not yet deployed"`. Do NOT proceed, and do NOT reword the documentation to describe an ordering the code does not implement — if prompt 2 was rejected at audit, this prompt is wrong and should be rejected too. -2. **Reword the stale bodies in `docs/work-on-session-lifecycle.md` to the post-exit, no-clear ordering.** The introduction (the paragraph mentioning "spec 040, revised by spec 041", "An id on disk now means the session is resumable") is already correct — keep it. Fix these three bodies, and nothing else: - - **`## Post-exit write ordering`** — the heading is correct; the body is stale. Replace the whole body (the two paragraphs starting `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**...` and `On the task path the pre-spawn re-read before writing is load-bearing: ...`) with: +2. **Reword the stale bodies in `docs/work-on-session-lifecycle.md` to the post-exit, no-clear ordering.** The introduction (the paragraph mentioning "spec 040, revised by spec 041", "An id on disk now means the session is resumable") is already correct — keep it. Fix these four bodies, and nothing else: + - **`## Post-exit write ordering`** (line 32) — the heading is correct; the body is stale. Replace the whole body (the paragraph starting `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**...`, the sentence `The **goal path** (pkg/ops/goal_workon.go) keeps its post-exit ordering unchanged: persistGoalSessionID runs only after StartSession returns cleanly.`, and the paragraph starting `On the task path the pre-spawn re-read before writing is load-bearing: ...`) with: ``` On both paths — task (`pkg/ops/workon.go`) and goal (`pkg/ops/goal_workon.go`) — the fresh id and its `metrics_sessions` entry are persisted **only after the detached @@ -65,9 +67,9 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th bound expiry, or cancellation — so there is no clear to run, and frontmatter the child wrote before failing stays untouched. ``` - The sentence `The **goal path** (pkg/ops/goal_workon.go) keeps its post-exit ordering unchanged: persistGoalSessionID runs only after StartSession returns cleanly.` is subsumed by the replacement — delete it rather than leaving it as a dangling contrast. - - **`## What the turn timeout does and does not cover`** — one sentence in the last-but-one paragraph is stale: `The compensating clear is unchanged: it still fires on every error the detached turn returns. Only the definition of "failed" moved.` Replace it with `No compensating clear runs on any error the detached turn returns — nothing was written before the turn, so there is nothing to undo. Only the definition of "failed" moved.` Leave the rest of that section, including the "read is never hoisted above the select" paragraph, exactly as it is. - - **`## Failure path`** — replace the body paragraph starting `On the **task path** the id is pre-persisted, so a failed spawn runs a compensating clear: ...` with: + The goal-path sentence is subsumed by the replacement — delete it rather than leaving it as a dangling contrast. + - **`## What the turn timeout does and does not cover`** (line 131) — one sentence in the last-but-one paragraph is stale: `The compensating clear is unchanged: it still fires on every error the detached turn returns. Only the definition of "failed" moved.` Replace it with `No compensating clear runs on any error the detached turn returns — nothing was written before the turn, so there is nothing to undo. Only the definition of "failed" moved.` Leave the rest of that section, including the "read is never hoisted above the select" paragraph, exactly as it is. + - **`## Failure path`** (line 172) — replace the body paragraph starting `On the **task path** the id is pre-persisted, so a failed spawn runs a compensating clear: ...` (lines 174-180) with: ``` On both paths the id is never written before the turn finishes, so there is no clear to run. A failed turn — child exit error, invalid turn result, bound expiry, or @@ -75,8 +77,8 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th child wrote before failing (for example `phase: planning`) survives, and no `claude_session_id` lands. The UI correctly keeps offering **Start**. ``` - Keep the following paragraph about the empty-id rule verbatim — it is already correct. - - **`## The per-session lock`** — the `**The detached-child safety property.**` paragraph is stale. Replace it with: + Keep the following paragraph about the empty-id rule (starting `The persist step itself can also fail ...`) verbatim — it is already correct. + - **`## The per-session lock`** (line 187) — the `**The detached-child safety property.**` paragraph (lines 234-242) is stale. Replace it with: ``` **The detached-child safety property.** On the spawn path, when the parent stops waiting — a failed turn, ctx cancel, or the 30m bound — the detached child keeps @@ -87,12 +89,12 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th running unlocked is not targetable. On any failure nothing was persisted, so the id cannot stay resumable-looking. ``` - - Do NOT touch `## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, or the rest of `## The per-session lock`. In particular, the phrase "liveness gating" in the lock's "Lock scope" paragraph is a spec-042 Vault UI follow-on concept, NOT the removed liveness-window concept — leave it. `scenarios/005-work-on-resume-auto-invokes-subtask.md` is never edited. - - After the reword the whole file must contain none of the reverted vocabulary — see the drift-guard grep in ``. + - Do NOT touch `## Session id ownership`, `## The second writer: the session-connect append` (line 50), `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, or the rest of `## The per-session lock`. In particular, the phrase "liveness gating" in the lock's "Lock scope" paragraph is a spec-042 Vault UI follow-on concept, NOT the removed liveness-window concept — leave it. `scenarios/005-work-on-resume-auto-invokes-subtask.md` is never edited. + - After the reword the whole file must contain none of the reverted vocabulary — see the drift-guard greps in ``. 3. **Confirm `scenarios/002-task-lifecycle.md` — no edit expected (AC11).** Its work-on action note must already state that the headless turn blocks until completion (it does: `**Both branches block until the turn completes**`, `bounded by a 30m turn timeout`, and `A fast return is a FAIL, not a pass`). Confirm `grep -c '~10s' scenarios/002-task-lifecycle.md` is 0. If it is non-zero, replace the fast-return wording with the blocking wording. Make no other change to this file. -4. **Create the unreleased section in `CHANGELOG.md` and record the change under it (AC12).** Today `CHANGELOG.md` has NO unreleased section — the newest section is `## v0.133.0`. Insert, immediately after the frozen preamble (the `* MAJOR version...` bullet block) and immediately above `## v0.133.0`: +4. **Create the unreleased section in `CHANGELOG.md` and record the change under it (AC12).** Today `CHANGELOG.md` has NO unreleased section — the newest section is `## v0.149.0` (line 11). Insert, immediately after the frozen preamble (the `* MAJOR version...` / `* MINOR version...` / `* PATCH version...` bullet block ending at line 9) and immediately above `## v0.149.0`: ``` ## Unreleased @@ -101,17 +103,19 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th Rules for this edit: - If an unreleased section already exists when you run (a concurrent change may have created it), do NOT create a second one — append this bullet as the last bullet inside the existing section. - Never move, delete, or edit the frozen preamble (`# Changelog`, the "All notable changes…" line, the SemVer link, the MAJOR/MINOR/PATCH bullets). `scripts/check-changelog.sh` fails the build if any `## ` section precedes the preamble line. - - Do NOT delete, reorder, or reword any existing bullet or any `## vX.Y.Z` section. - - Do NOT bump the plugin version strings in `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`, and do NOT create a tag. This repository's release agent owns version bumps and tagging after merge (see `CLAUDE.md` § Plugin Release Checklist and the changelog guide's "Version Alignment Is Release-Time"). Write `## Unreleased`, never `## vX.Y.Z`. + - Do NOT delete, reorder, or reword any existing bullet or any `## vX.Y.Z` section. In particular, do NOT touch the `## v0.117.1` section (which recorded the original spec-041 change) or the `## v0.118.3` section (which recorded the reversion this prompt reverses) — the changelog is append-only history and both entries stay as they are. + - Do NOT bump the plugin version strings in `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`, and do NOT create a tag. This repository's release agent owns version bumps and tagging after merge (`.maintainer.yaml` sets `release.autoRelease: true`; see `CLAUDE.md` § Plugin Release Checklist and the changelog guide's "Version Alignment Is Release-Time"). Write `## Unreleased`, never `## vX.Y.Z`. - The bullet must be within the first 15 lines after the `## Unreleased` heading, and must contain the substring "Resume" (case-insensitive), because AC12's evidence grep reads exactly those 15 lines. -5. **Confirm the spec's two open questions need no code change.** Open Question 1: the turn bound stays an unexported tunable constant with no config field — do not add one (spec Non-goals). Open Question 2: the Vault UI modal copy is a different repository — no change here. +5. **Note, do not fix, the agent-definition drift.** `agents/work-on-task-assistant.md` § Session connect (line 158) still states `A miss is safe **only** on the headless Start path, which pre-sets this field via vault-cli before the turn`, and line 124 describes the status-flip routing that depends on the same pre-set. If prompt 2 ships, that text describes a pre-spawn pre-set the code no longer performs. Updating `agents/work-on-task-assistant.md` is OUTSIDE this spec's stated file scope (the spec's Design and Suggested Decomposition name only `pkg/ops` and `docs/scenarios`), so make NO edit to it in this prompt. Record it in the completion report's `## Improvements` section as a reviewer-owned follow-up (category: PROMPT) so it is not lost. -6. **Full gate (AC13).** Run `make precommit` at the repo root. It must exit 0. If it fails on something this prompt introduced (most likely `check-changelog`), fix it and re-run only the failing target (`make check-changelog`, `make lint`, ...), then `make precommit` once more. Note that `make precommit` does not run `check-versions`; the four version strings are already aligned at the last released version and must stay that way. +6. **Confirm the spec's two open questions need no code change.** Open Question 1: the turn bound stays an unexported tunable constant with no config field — do not add one (spec Non-goals). Open Question 2: the Vault UI modal copy is a different repository — no change here. -7. **Self-check before finishing.** Re-read the changed doc, scenario, and changelog hunks and walk spec 041 ACs 11, 12 and 13 against them. Run every command in `` and confirm each holds — including the content-level drift guard, which is what actually catches the stale body wording; the spec's own AC11 greps already pass in the current tree and are therefore NOT evidence that the reword was done. +7. **Full gate (AC13).** Run `make precommit` at the repo root. It must exit 0. If it fails on something this prompt introduced (most likely `check-changelog`), fix it and re-run only the failing target (`make check-changelog`, `make lint`, ...), then `make precommit` once more. Note that `make precommit` runs `check` (which includes `check-changelog`) but does NOT run `check-versions`; the four version strings are already aligned at the last released version and must stay that way. + +8. **Self-check before finishing.** Re-read the changed doc, scenario, and changelog hunks and walk spec 041 ACs 11, 12 and 13 against them. Run every command in `` and confirm each holds — including the content-level drift guard, which is what actually catches the stale body wording; the spec's own AC11 greps already pass in the current tree and are therefore NOT evidence that the reword was done. @@ -119,8 +123,9 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th - `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it. - The interactive TTY branch is unchanged; do not reword any documentation text into claiming otherwise. - Do NOT bump the plugin manifests and do NOT create a tag — only the unreleased bullet is in scope. Write `## Unreleased`, never `## vX.Y.Z`. -- Do NOT delete or reword existing changelog bullets or any `## vX.Y.Z` section — the new bullet is appended inside the unreleased section (or the section is created if absent). +- Do NOT delete or reword existing changelog bullets or any `## vX.Y.Z` section — the new bullet is appended inside the unreleased section (or the section is created if absent). The `## v0.117.1` and `## v0.118.3` sections in particular stay byte-identical. - 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 edit `agents/work-on-task-assistant.md` — its pre-spawn contract text is a reviewer-owned follow-up (requirement 5), not this prompt's work. - Do NOT touch `pkg/ops/*.go` or any `_test.go` file in this prompt — code changes belong to prompts 1 and 2. - Do NOT add a config field for the turn bound (spec Non-goals / Open Question 1). - Existing tests must still pass. @@ -132,15 +137,25 @@ Evidence greps — run each, record the count, and confirm it against the expect ``` grep -c '^## Unreleased' CHANGELOG.md # >= 1 (AC12) — must flip 0 -> >= 1 grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 (AC12) — must flip 0 -> >= 1 -grep -c 'wait for the detached headless turn' CHANGELOG.md # >= 1 if you used the suggested wording +grep -c 'only after the detached headless turn exits' CHANGELOG.md # >= 1 if you used the suggested wording ! grep -q 'livenessWindow' docs/work-on-session-lifecycle.md # AC11: absent ! grep -ci 'liveness window' docs/work-on-session-lifecycle.md # AC11: absent (prose form too) ! grep -q '~10s' scenarios/002-task-lifecycle.md # AC11: absent -! grep -qE 'pre-spawn|pre-persisted|pre-persist|before the child is spawned|compensating clear' docs/work-on-session-lifecycle.md # drift guard — the real check for req 2 grep -n -m1 '^All notable changes to this project' CHANGELOG.md # preamble present grep -n -m1 '^## ' CHANGELOG.md # must be the Unreleased heading, AFTER the preamble line above ``` +CONTENT DRIFT GUARD — the real check for requirement 2. The reverted vocabulary must be gone, and the new framing present. The reworded text legitimately contains the words "compensating clear" in a negated sentence ("No compensating clear runs on any error…") and the lock section legitimately keeps "no compensating clears" — so the guard targets the STALE CLAIMS by exact phrase, not the bare two words: + +``` +! grep -qiE 'pre-spawn|pre-persisted|pre-persist|before the child is spawned' docs/work-on-session-lifecycle.md +! grep -q 'runs a compensating clear' docs/work-on-session-lifecycle.md +! grep -q 'The compensating clear is' docs/work-on-session-lifecycle.md +! grep -q 'the compensating clear removes' docs/work-on-session-lifecycle.md +grep -c 'only after the detached headless turn has finished cleanly' docs/work-on-session-lifecycle.md # >= 1 (req 2, Post-exit write ordering) +grep -c 'the id is never written before the turn finishes' docs/work-on-session-lifecycle.md # >= 1 (req 2, Failure path) +``` + FULL GATE — `make precommit` at the repo root must exit 0, and the completion report must carry its actual exit code. If it fails, fix the cause and re-run only the failing target, then `make precommit` once more. The spec's AC10 guard (`git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md`) was verified in prompt 1 and is not repeated here. diff --git a/prompts/completed/235-spec-055-remove-metrics-session-verb.md b/prompts/completed/235-spec-055-remove-metrics-session-verb.md new file mode 100644 index 00000000..4668cef4 --- /dev/null +++ b/prompts/completed/235-spec-055-remove-metrics-session-verb.md @@ -0,0 +1,191 @@ +--- +status: completed +spec: [055-remove-metrics-session] +summary: Added the task remove-metrics-session verb end to end (domain method, ops operation, generated mock, CLI command and registration, plus domain/ops/integration specs) while leaving metricsSessionsWriteRefusal, knownTaskListFields and the AC4 refusal spec byte-identical +execution_id: vault-cli-exec-235-spec-055-remove-metrics-session-verb +dark-factory-version: v0.196.0 +created: "2026-09-27T09:26:40Z" +queued: "2026-09-27T10:04:31Z" +started: "2026-09-27T10:04:33Z" +completed: "2026-09-27T10:12:32Z" +branch: dark-factory/remove-metrics-session +--- + +# Add the `task remove-metrics-session` verb (spec 055, prompt 1 of 3) + + +- Adds one new vault-cli verb, `task remove-metrics-session `, that removes every `metrics_sessions` entry carrying the supplied session id from one task and leaves every other entry — and every other frontmatter key, known or unknown — exactly as it was. +- This is the missing half of the session-connect pair: entries could already be appended but never removed, so the shared-session rule's clearing step had no programmatic path and required hand-editing a field whose own contract forbids it. +- When the removal takes the last entry, the `metrics_sessions` key is deleted outright rather than left as an empty list, so a task that has had its last session removed is indistinguishable from one that never had an entry. +- A call whose id matches nothing fails loudly and writes nothing — the file stays byte-identical — because a silent success here would reproduce the exact defect class this verb exists to close. +- A session id that is empty, not a well-formed UUID, or carries a path separator is refused before the task is even read, so a caller-supplied string can never reach the frontmatter block. +- Adds the domain method, the operation, the counterfeiter mock, the CLI command and its registration, plus domain, ops and integration specs. +- Leaves the existing refusal completely alone: `task set`, `task add` and `task remove` still reject `metrics_sessions` before any mutation, with no `--force` bypass, and the integration suite's existing AC4 spec is not modified. +- Does not touch the append path, does not write `claude_session_id`, and takes no clock — this verb stamps nothing. +- Covers spec 055 ACs 1-7. + + + +Ship the `task remove-metrics-session` verb end to end — domain, ops, CLI, mock and tests — so an operator can remove exactly one session's `metrics_sessions` entries from a task without touching frontmatter by hand, while `metricsSessionsWriteRefusal` and `knownTaskListFields` remain byte-identical. This prompt covers spec 055 ACs 1-7. It is the precondition for prompt 3, which documents the verb, and is independent of prompt 2. + + + +Read `CLAUDE.md` for project conventions. + +**The verb is a mirror of `task append-metrics-session`.** That command, its operation and its mock shipped in v0.147.0 and are the shape to copy. Read them before writing anything; the new code should be recognisably the same code with the append replaced by a filtered removal and the clock dropped. + +Read fully (in this order): +- `pkg/ops/metrics_session_append.go` — the whole file. The operation interface, the `//counterfeiter:generate` annotation, the constructor, and `Execute`'s validate-then-read-then-write order are all copied from here. Note that it stamps `started_at` from an injected `libtime.CurrentDateTime`; the new verb takes **no** clock, so its constructor takes only `TaskStorage`. +- `pkg/ops/metrics_session_write.go` — the whole file. `metricsSessionsWriteRefusal`. This file must NOT change. +- `pkg/domain/task_frontmatter_metrics.go` — the whole file. `MetricsSessions()`, `AppendMetricsSession`, `ClearMetricsSessions` and `coerceMetricsSession` live here; the new `RemoveMetricsSession` joins them. +- `pkg/domain/metrics.go` — the `MetricsSession` struct. +- `pkg/ops/metrics_session_append_test.go` — the whole file. The ops unit-spec idiom: `mocks.TaskStorage`, `seedTask`, `libtimetest.ParseDateTime`. The new verb needs no pinned clock, so drop the `libtime`/`libtimetest` bits. +- `pkg/domain/task_frontmatter_metrics_test.go` — the whole file. Note `Describe("Clear metrics", ...)` at the end; the new domain specs go beside it. +- `mocks/append-metrics-session-operation.go` — the whole file. This is what `make generate` will emit for the new interface; do not hand-write the mock. +- `pkg/cli/cli.go` — `createTaskAppendMetricsSessionCommand` (definition at line 2410) and its registration at line 1344. Also read `createTaskSetCommand` immediately below it for the vault-dispatch shape. +- `integration/cli_test.go` — the `Describe("task append-metrics-session", ...)` block starting at line 2588, in full. Its fixture helpers (`runEntityCommand`, `sha256OfFile`, `readFile`, `countMetricsEntries`, `frontmatterOf`, `sortedKeys`), its `baseFrontmatter` / `twoEntryFrontmatter` constants, and its `It("AC4: task set, add and remove each refuse metrics_sessions and leave the file byte-identical", ...)` spec at line 2809 are the template to copy from — ⚠️ they are declared **inside that Describe's own closure**, so they are *not* visible to a sibling Describe. Copy them into the new one; do not assume you can call them. The new Describe goes after this block (which ends before `Describe("vault-cli defer", ...)` at line 2873) and must NOT modify it. +- `pkg/ops/frontmatter_entity.go` — lines 735-775 only. `metricsSessionsWriteRefusal` is called at line 743, the `knownTaskListFields` check at line 750, and the dispatch switch reading only `Goals()` / `Tags()` / `BlockedBy()` at lines 760-768. Read this to understand why the allowlist route cannot work — but do not change a byte of it. + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-patterns.md` — interface → constructor → private struct, counterfeiter annotation, `errors.Wrap` idiom. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrapf(ctx, ...)` / `errors.Wrap(ctx, ...)` / `errors.Errorf(ctx, ...)` from `github.com/bborbe/errors`; never `fmt.Errorf`, never a bare `return err`, never `context.Background()` inside `pkg/`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega conventions, external `_test` packages, counterfeiter mocks. + +NOTE: git IS available in this container (`.dark-factory.yaml` is `workflow: direct`, no `hideGit`), so `git diff --exit-code` guards run here. + + + +1. **Add the domain method.** In `pkg/domain/task_frontmatter_metrics.go`, beside `AppendMetricsSession`: + ```go + // RemoveMetricsSession removes every "metrics_sessions" entry whose SessionID + // equals sessionID, preserving every other entry in order. When the last entry + // goes, the key is deleted rather than left as an empty list. Returns the number + // of entries removed; 0 means nothing matched and nothing was written. + func (f *TaskFrontmatter) RemoveMetricsSession(sessionID string) int + ``` + Semantics, all four of which are asserted: + - Filter `f.MetricsSessions()` on `entry.SessionID == sessionID`, keeping the survivors in their original order. + - `removed == 0` → return 0 and leave the field untouched. Do NOT call `Set` — a no-op write would still change the in-memory map. + - `removed > 0 && len(kept) == 0` → `f.Delete("metrics_sessions")`, return `removed`. + - `removed > 0 && len(kept) > 0` → `f.Set("metrics_sessions", kept)`, return `removed`. + A duplicate id is removed in full, not once — the append accumulator deliberately never suppresses a repeat, so N > 1 is reachable and must be handled. + +2. **Add the operation.** New file `pkg/ops/metrics_session_remove.go`, mirroring `metrics_session_append.go`: + ```go + //counterfeiter:generate -o ../../mocks/remove-metrics-session-operation.go --fake-name RemoveMetricsSessionOperation . RemoveMetricsSessionOperation + type RemoveMetricsSessionOperation interface { + Execute(ctx context.Context, vaultPath, taskName, sessionID string) error + } + + func NewRemoveMetricsSessionOperation(taskStorage storage.TaskStorage) RemoveMetricsSessionOperation + ``` + `Execute` does exactly this, in this order: + - `if err := uuid.Validate(sessionID); err != nil` → `errors.Wrapf(ctx, validation.Error, "invalid session id %q: expected a well-formed UUID", sessionID)`. Copy the append's guard verbatim. This runs **before** the task is read, which is what makes AC5's "nothing written" true by construction. + - `o.taskStorage.FindTaskByName(ctx, vaultPath, taskName)`; on error `errors.Wrap(ctx, err, "find task")`. + - `removed := task.RemoveMetricsSession(sessionID)`. + - `removed == 0` → return `errors.Errorf(ctx, "no metrics_sessions entry with session id %q on task %q: nothing removed", sessionID, taskName)`. The message must name the id and say nothing was removed. + - Only now `o.taskStorage.WriteTask(ctx, task)`; on error `errors.Wrap(ctx, err, "write task")`. When nothing matched, `WriteTask` is **never called** — the byte-identical guarantee is structural, not a rewrite that happens to be stable. + - Return nil. + The operation never touches `claude_session_id`, takes no clock, and adds no `--force` / `--all` / `--started-at` flag. Removal is by session id only. + +3. **Generate the mock.** Add the counterfeiter annotation from requirement 2 and run `make generate`. ⚠️ `make generate` rewrites `mocks/mocks.go` as a bare `package mocks` line and **strips its four-line copyright header** — a pre-existing generator defect, not yours to fix. Restore `mocks/mocks.go` to its committed content (`git checkout -- mocks/mocks.go`) and do not carry the deletion. The new `mocks/remove-metrics-session-operation.go` is the only mock file that should appear as a change. + +4. **Add the CLI command.** In `pkg/cli/cli.go`, immediately after `createTaskAppendMetricsSessionCommand`, add `createTaskRemoveMetricsSessionCommand(ctx context.Context, configLoader *config.Loader, vaultName *string, outputFormat *string) *cobra.Command` and register it next to the append at line 1344: + ```go + cmd.AddCommand(createTaskRemoveMetricsSessionCommand(ctx, configLoader, vaultName, outputFormat)) + ``` + The command mirrors the append exactly, minus the clock: + - `Use: "remove-metrics-session "`, `Args: cobra.ExactArgs(2)`, and a `Short:` describing per-session entry removal. + - `getVaults(ctx, configLoader, vaultName)`, then `ops.NewVaultDispatcher().FirstSuccess(ctx, vaults, func(vault *config.Vault) error { ... })` building `ops.NewRemoveMetricsSessionOperation(storage.NewTaskStorage(storage.NewConfigFromVault(vault)))` and calling `Execute(ctx, vault.Path, taskName, sessionID)`. This is what makes the verb behave identically under `--vault` and under all-vaults, exactly as `task set` does. + - Error path: `if OutputFormat(*outputFormat).IsJSON() { return PrintJSON(map[string]any{"success": false, "error": err.Error()}) }` then `return err`. Mirror the append precisely — the JSON branch returns `PrintJSON`'s result (normally nil, so exit 0 with a `success:false` object) and only the plain branch propagates the error and exits non-zero. This asymmetry is the established contract and AC7 depends on it; do NOT "fix" it to return the error in both branches. + - Success path: JSON → `PrintJSON(map[string]any{"success": true, "name": taskName, "session_id": sessionID})`; plain → a confirmation line naming the id and the task, in the same style as the append's `✅ Appended metrics session %s to: %s`. + - **No `encoding/json` import in `pkg/cli/`.** Output goes through `PrintJSON`. + - Add the same `//nolint:dupl,...` directive the neighbouring mutation commands carry if the linter demands it. + +5. **Add the domain specs.** In `pkg/domain/task_frontmatter_metrics_test.go`, beside `Describe("Clear metrics", ...)`: + - removes the named entry and preserves the survivor's own id **and** its own `started_at` (a filter that keeps the wrong entry, or rewrites the survivor's timestamp, fails here); + - removes all entries carrying a duplicated id, leaving exactly one; + - deletes the key when the last entry goes — assert the key is absent, not merely empty; + - returns 0 and leaves the field untouched when nothing matches; + - returns 0 on an absent `metrics_sessions` key and on a non-list value. + +6. **Add the ops specs.** New file `pkg/ops/metrics_session_remove_test.go`, mirroring `metrics_session_append_test.go`'s structure (`mocks.TaskStorage`, a local `seedTask`, external `ops_test` package). Cover: + - a matching id calls `WriteTask` exactly once and leaves the task's `metrics_sessions` with only the survivors; + - a non-matching id returns an error whose message contains the id, and `WriteTask` is **never called** (`Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(0))`) — this is the assertion that pins "writes only when something changed"; + - an empty id, `"not-a-uuid"` and `"../escape"` each return an error and never call `FindTaskByName` (`Expect(mockTaskStorage.FindTaskByNameCallCount()).To(Equal(0))`) — the validation runs before the read; + - a `FindTaskByName` error is wrapped and propagated; + - a `WriteTask` error is wrapped and propagated. + +7. **Add the integration specs.** In `integration/cli_test.go`, add a new `Describe("task remove-metrics-session", ...)` after the append block. The append block's fixture helpers (`runEntityCommand`, `sha256OfFile`, `readFile`, `countMetricsEntries`, `frontmatterOf`, `sortedKeys`) and its `baseFrontmatter` / `twoEntryFrontmatter` constants are declared **inside that Describe's closure** (starting line 2588) and are therefore not visible to a sibling Describe — declare your own copies inside the new Describe, as the file already does (`runEntityCommand := func(...)` is declared locally in three separate Describes). Do NOT modify the append Describe or the AC4 refusal spec at line 2809. Note that in `twoEntryFrontmatter` the fixture's `task_identifier` **is** `11111111-1111-4111-8111-111111111111`, which is also the first entry's session id: every absence assertion must match the entry-line prefix (`- session_id: `), never a bare id substring, or it will match the surviving `task_identifier` line and read a false positive. Cover ACs 1-7: + - **AC1** — from the two-entry fixture, removing the first id exits 0 and leaves exactly one entry whose `session_id` and `started_at` are the survivor's own; `countMetricsEntries` returns 1 and the entry-line prefix for the removed id returns 0. + - **AC2** — removing both ids in turn leaves no `metrics_sessions` occurrence at all, while `page_type`, `priority`, `status`, `task_identifier` and the body line are unchanged. + - **AC3** — a three-entry fixture with the first id duplicated leaves exactly one entry, the one carrying the third id. + - **AC4** — removing an absent id exits non-zero, stderr contains the id and says nothing was removed, and the file's `sha256` is unchanged before and after. + - **AC5** — `""`, `"not-a-uuid"` and `"../escape"` each exit non-zero with a message naming the required shape, and the file hash is unchanged after all three. + - **AC6** — `task set`, `task add` and `task remove` on `metrics_sessions` still each exit non-zero, and the file hash is unchanged. The pre-existing AC4 spec must remain **unmodified** and still pass. + - **AC7** — `go test ./integration/... -ginkgo.focus="remove-metrics-session"` exits 0; `--output json` emits `{"success": true, ...}` for a matching id and `{"success": false, ...}` for a non-matching one. + Also add the registration entry the project's DoD requires (`docs/dod.md`: "New CLI commands/subcommands have an entry in the integration test command registration table"): in the existing `Describe("command registration", ...)` `DescribeTable` (`integration/cli_test.go`, ~line 447, beside `Entry("task append-metrics-session", "task", "append-metrics-session")`) add `Entry("task remove-metrics-session", "task", "remove-metrics-session")`. That DescribeTable is a different Describe from the append block, so this does not violate the "do not modify the append Describe" constraint. Additionally, inside the new Describe, assert `--help task remove-metrics-session` exits 0 and its output names the verb. + +8. **Do not touch the refusal.** `pkg/ops/metrics_session_write.go` and the `knownTaskListFields` map in `pkg/ops/frontmatter_entity.go` must be byte-identical to their committed state, and the integration AC4 spec must be unmodified. Verify with `git diff --exit-code` on those two files at the end. + + + +- Do NOT commit — dark-factory handles git. `git diff` / `git diff --exit-code` only read; never stage or commit. +- **The refusal is a frozen invariant.** `metricsSessionsWriteRefusal` and `knownTaskListFields` are not modified, and the integration AC4 spec at `integration/cli_test.go:2809` is not modified. Any diff touching them fails review. +- **Do NOT add `metrics_sessions` to `knownTaskListFields`.** The entry is inert — the refusal fires first at line 743, and the dispatch switch at lines 760-768 reads only `[]string` fields, so a map-valued field falls through setting nothing — and it would additionally open `task add metrics_sessions …`. +- **Do NOT exempt `remove` from the refusal.** The dedicated verb is the only shape that leaves the refusal intact. +- **The append path is untouched.** `task append-metrics-session`, `AppendMetricsSessionOperation` and `TaskFrontmatter.AppendMetricsSession` keep their behaviour and their accumulate-never-replace semantics. +- **The write goes through the domain and `TaskStorage.WriteTask`.** A string-level rewrite of the `metrics_sessions:` block that bypasses the domain would satisfy every AC while violating this constraint — the domain unit specs in requirement 5 are what rule it out. +- **`ClearMetricsSessions` is not the tool.** It runs on task completion and clears the whole field; this verb removes per entry and adds no flag that clears the field. +- **No new flags.** No `--force`, no `--all`, no `--started-at`, and no removal by timestamp — removal is by session id, the key the shared-session rule reads. +- **No `encoding/json` import in `pkg/cli/`** — output goes through `PrintJSON`. +- **The verb writes only when something changed**, never writes `claude_session_id`, and takes no clock. Its constructor takes only `TaskStorage`. +- **`mocks/mocks.go` must be restored** after `make generate` (see requirement 3). Do not carry the header deletion. +- ⚠️ **Known pre-existing failure, not this change's to fix.** `integration/cli_test.go`'s `topic defer writes defer_date for a relative and an absolute date` is red for the ~2 h each day when the local date leads the UTC date: it computes its expectation from `time.Now().UTC().AddDate(0,0,7)` while the CLI writes the local date. CI runs UTC and never sees it. Do NOT "fix" it opportunistically, and do not let it block the prompt — `make test` passing apart from exactly that named failure is a pass. +- Existing tests must still pass; the `InteractionCounter` specs and spec 038's dedup specs are unmodified. + + + +PRIMARY GATE — evidence greps. Run each, record the count, and confirm it against the expectation. Rows expecting 0 are written as `! grep -q` because `grep -c` exits 1 when it prints 0: + +``` +grep -c 'createTaskRemoveMetricsSessionCommand' pkg/cli/cli.go # >= 2 (the AddCommand registration and the factory definition) +grep -c 'RemoveMetricsSession' pkg/domain/task_frontmatter_metrics.go # >= 1 +grep -c 'RemoveMetricsSessionOperation' pkg/ops/metrics_session_remove.go # >= 2 (interface + constructor) +grep -c 'invalid session id' pkg/ops/metrics_session_remove.go # >= 1 (AC5 guard) +grep -n 'metrics_sessions' pkg/cli/cli.go # only Short:/Use: string literals expected (the append's Short at line 2418 plus the new command's own); any other hit is a raw field-name write — see below +grep -n 'remove-metrics-session' pkg/cli/cli.go # >= 1 (the Use: field) +grep -c 'WriteTaskCallCount' pkg/ops/metrics_session_remove_test.go # >= 1 (the "writes only when changed" pin) +``` + +⚠️ **AC7's grep trap — do NOT assert `>= 2` on the hyphenated verb literal.** The spec's AC7 asks for `grep -n 'createTaskRemoveMetricsSessionCommand' pkg/cli/cli.go` returning ≥ 2 lines, which is what the row above checks. The hyphenated literal `remove-metrics-session` appears in that file **exactly once** (the `Use:` field) because the constructor is camelCase — the twin `append-metrics-session` behaves identically. A `>= 2` threshold on the hyphenated form is unachievable by a correct implementation; do not edit the source to force it. The row `grep -n 'metrics_sessions' pkg/cli/cli.go` guards the other direction: every hit must be inside a `Use:`/`Short:` string literal (the append's at line 2418 and the new command's own). A hit anywhere else means the new command is naming the raw field outside its help text — the write must go through the domain and `TaskStorage.WriteTask`. + +FROZEN-NEIGHBOUR GUARD — these must exit 0 with empty output: +``` +git diff --exit-code -- pkg/ops/metrics_session_write.go +git diff -U0 -- integration/cli_test.go | grep -c 'AC4: task set, add and remove' # must print 0 — the AC4 spec body is unmodified (the file itself gains the new Describe by design, so a whole-file --exit-code check would fail spuriously) +``` + +TESTS: +``` +go test ./pkg/domain/... # domain specs pass +go test ./pkg/ops/... # ops specs pass +go test ./integration/... -ginkgo.focus="remove-metrics-session" # exits 0 +make test # exits 0 apart from the named `topic defer` failure +``` + +⚠️ **Guard the focus run against a false pass.** `-ginkgo.focus` with zero matching specs can exit 0 while running nothing. After the focus run, assert the new spec names actually appear in the output (run with `-v -ginkgo.v` and grep for a distinctive spec name), and treat a run whose log does not contain them as a failure. + +SYNTAX: +``` +gofmt -e -l pkg/domain/task_frontmatter_metrics.go pkg/ops/metrics_session_remove.go pkg/cli/cli.go integration/cli_test.go # must list NO files +``` + +FULL GATE — `make precommit` at the repo root must exit 0. If it fails on something this prompt introduced, fix it and re-run only the failing target (`make lint`, `make vet`, `make vulncheck`, `make check-changelog`, `make generate`, ...), then `make precommit` once more. ⚠️ `make precommit` does **not** run `check-versions` — that check is release-time only (`make release-check`, see `docs/releasing-vault-cli.md` § Version alignment). The four version strings must still be untouched: this prompt adds no release, and a `## Unreleased` CHANGELOG section is prompt 3's job, not this one's. + +⚠️ **`make precommit` ends in `generate`, which re-runs the generator and re-strips the `mocks/mocks.go` copyright header.** So requirement 3's restoration must be the **last** action, after the final `make precommit` — restoring it earlier just gets stripped again: +``` +git checkout -- mocks/mocks.go +git diff --exit-code -- mocks/mocks.go # the generator's header strip is not carried +``` + diff --git a/prompts/in-progress/236-spec-055-auditor-guard-sentence.md b/prompts/in-progress/236-spec-055-auditor-guard-sentence.md new file mode 100644 index 00000000..b5bfc790 --- /dev/null +++ b/prompts/in-progress/236-spec-055-auditor-guard-sentence.md @@ -0,0 +1,95 @@ +--- +status: approved +spec: [055-remove-metrics-session] +created: "2026-09-27T09:26:40Z" +queued: "2026-09-27T10:04:31Z" +branch: dark-factory/remove-metrics-session +--- + +# Stop the auditor recommending a link it will then score as an orphan (spec 055, prompt 2 of 3) + + +- Adds one guard sentence to the task auditor's goal-alignment section so the rule that judges a goal link and the advice that recommends one use the same predicate. +- Today the section flags as MAJOR any goal link that advances none of the goal's criteria, while the surrounding guidance can suggest linking a goal on family resemblance — so a link the auditor itself recommends can come back as a MAJOR orphan on the next run. +- That contradiction cost two extra audit cycles plus an operator ruling on 2026-09-15 to unblock a single task: the recommendation was taken, and the two following runs scored the very same link as an orphan. +- The new sentence sits between the orphan bullet and the implementation-level bullet, so it is read at the moment the auditor decides whether to recommend a link. +- The rule it states: if the goal can be marked complete without this task, do not recommend the link — recommend theme-only linkage and say so. +- Markdown-only change to an agent definition. No Go code, no tests, no version bump. +- Covers spec 055 AC 8. AC 9 is the post-deploy behavioural half and is not checkable before release. +- Independent of prompt 1 — this can land or be reverted without the Go change. + + + +Remove the self-contradiction in `task-auditor`: a goal link the auditor recommends must never be one its own alignment check would then score as a MAJOR orphan. Covers spec 055 AC 8. Prompt 1 (the removal verb) and prompt 3 (the docs) are unaffected by this change. + + + +Read `CLAUDE.md` for project conventions. + +Read fully: +- `agents/task-auditor.md` § `## Task-Goal Alignment (per-goal-link check)` — the section starting at line 237. Read to at least the `### 9. Scope Appropriateness` heading at line 248. The two bullets that bracket the insertion point are: + - line 243: `3. **Flag orphans as MAJOR** — task has a goal link but advances none of its criteria.` + - line 244: `4. **Flag implementation-level tasks** — if title reads like a low-level code change ("Add field X to struct Y"), check whether a dark-factory spec or prompt is the right artifact instead.` + Also read the note at line 246 (`**When \`goals:\` is absent but \`themes:\` is populated**`), which already carries the "theme linkage is acceptable" half of this rule, and line 133, which states the same threshold for the structure check. The new sentence must not contradict either. +- The `## Task-Goal Alignment` report template at line 501 — the table whose `ORPHAN — MAJOR` verdict is the check the new sentence has to agree with. + +This is a Direct-layer markdown edit to an agent definition. No Go code is involved. + + + +1. **Insert exactly one guard sentence** into § Task-Goal Alignment, as a sub-bullet of item 3, on its own line, positioned **after** the `**Flag orphans as MAJOR**` bullet (line 243) and **before** the `**Flag implementation-level tasks**` bullet (line 244). Leave the two bracketing bullets' text unchanged and do not renumber them. + + The inserted line MUST contain this exact substring, verbatim and in lowercase, because AC 8 greps for it: + ``` + never recommend linking a goal the alignment check will then score as an orphan + ``` + A capitalised `Never` at the start of the sentence will NOT satisfy the grep. Put the phrase mid-sentence, for example: + ```markdown + 3. **Flag orphans as MAJOR** — task has a goal link but advances none of its criteria. + - When the goal can be marked complete without this task, never recommend linking a goal the alignment check will then score as an orphan — recommend theme-only linkage instead and say so explicitly. + 4. **Flag implementation-level tasks** — ... + ``` + Keep the wording close to that — the requirement is that the sentence states the rule (do not recommend a link the check would then flag) and the fallback (recommend theme-only linkage and say so). + +2. **Make the two halves agree.** The sentence must be consistent with the existing note at line 246 and with the `ORPHAN — MAJOR` verdict in the report template at line 501. Do not weaken the orphan rule itself, and do not change the threshold at which a missing `goals:` link is flagged — this prompt adds a constraint on the *advice*, not on the *check*. + +3. **Change nothing else.** No other section of `agents/task-auditor.md` is edited, no bullet is reordered, no heading is renamed, and the file's frontmatter is untouched. This is a one-sentence change. + + + +- Do NOT commit — dark-factory handles git. `git diff` only reads; never stage or commit. +- **Exactly one sentence is added.** A diff that restructures § Task-Goal Alignment, renumbers the list, or edits the line-246 note fails review. +- **Do NOT weaken the orphan check.** The `**Flag orphans as MAJOR**` bullet's threshold is unchanged; this prompt constrains what the auditor *recommends*, not what it *flags*. +- **Do NOT touch the plugin cache** at `~/.claude/plugins/cache/vault-cli/vault-cli/*` — that is an install artifact, clobbered by the next `claude plugin update`. Edit only `agents/task-auditor.md` in the repo. +- **No version bump.** This is an unreleased source change; the release is handled by the repo's `autoRelease` releaser after merge. Do not edit `CHANGELOG.md`, `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json` — prompt 3 owns the CHANGELOG bullet. +- This change ships in the plugin, so it reaches the installed `task-auditor` only after `claude plugin update vault-cli@vault-cli`. That is AC 9's post-deploy rung and is explicitly out of scope here. + + + +PRIMARY GATE — AC 8. The sentence must exist AND sit strictly between the two bracketing bullets, checked section-scoped rather than file-wide. Run and record: + +``` +grep -n 'never recommend linking a goal the alignment check will then score as an orphan' agents/task-auditor.md +``` +Must return ≥ 1 line. Let ``, `` and `` be the line numbers of the guard sentence, `**Flag orphans as MAJOR**`, and `**Flag implementation-level tasks**` respectively — all three inside § Task-Goal Alignment (the section beginning at `## Task-Goal Alignment (per-goal-link check)`): + +``` +grep -n 'Flag orphans as MAJOR' agents/task-auditor.md # the orphan bullet — line +grep -n 'Flag implementation-level tasks' agents/task-auditor.md # the impl bullet — line +``` +Assert ` < < `. A guard line equal to `` or `` fails — it must be strictly between. A match elsewhere in the file (the report template at line 501 also discusses orphans) does NOT satisfy this; confirm the guard's line number falls inside the section's line range. + +NEGATIVE EVIDENCE — the two bracketing bullets' text is unchanged: +``` +git diff -- agents/task-auditor.md # must show exactly one added line, no other hunks +``` +Confirm the diff is a single insertion with no deletions other than the `4.` line's re-emission if your editor rewrote it; a hunk touching any other section fails. + +SCOPE — ⚠️ this checkout is shared and is routinely dirty with other sessions' and the daemon's in-flight work, so scope the check to the file rather than asserting a clean tree: +``` +git diff --name-only -- agents/task-auditor.md # must list agents/task-auditor.md +``` +Do NOT assert that `git diff --stat` lists only this file — unrelated paths from sibling work will appear and the assertion fails spuriously, which could push an implementer to "fix" files outside this prompt's scope. The evidence that nothing else was touched is the single-insertion diff above plus the unchanged bracketing bullets. + +FULL GATE — `make precommit` at the repo root must exit 0. This change adds no Go code, so the gate should be unaffected by it; a failure here means either a pre-existing issue or an accidental edit outside `agents/task-auditor.md`. If it fails on something this prompt introduced, fix it and re-run only the failing target, then `make precommit` once more. + diff --git a/prompts/in-progress/237-spec-055-docs-changelog.md b/prompts/in-progress/237-spec-055-docs-changelog.md new file mode 100644 index 00000000..1ac98b46 --- /dev/null +++ b/prompts/in-progress/237-spec-055-docs-changelog.md @@ -0,0 +1,93 @@ +--- +status: approved +spec: [055-remove-metrics-session] +created: "2026-09-27T09:26:40Z" +queued: "2026-09-27T10:04:31Z" +branch: dark-factory/remove-metrics-session +--- + +# Document the removal verb and record it in the changelog (spec 055, prompt 3 of 3) + + +- Records the new `task remove-metrics-session` verb where the repo keeps its behaviour prose, so the next reader finds the removal path next to the refusal that protects the field. +- States why the generic verbs cannot express the removal: `set` stores a scalar and `add` / `remove` comma-split into a list of scalars, while a `metrics_sessions` entry is a map — which is the reason the dedicated verb exists rather than an allowlist entry. +- Adds a `## Unreleased` CHANGELOG section carrying one `feat:` bullet for the verb and one `fix:` bullet for the auditor guard sentence. +- Documentation only — no Go code, no tests, no version bump. The release is cut by the repo's `autoRelease` releaser after merge, so the plugin manifests stay untouched. +- Depends on prompt 1 only for the verb's name, which is frozen. +- Covers spec 055 AC 10. + + + +Record the removal verb and the auditor change where the repo keeps its prose: `docs/work-on-session-lifecycle.md` § the write-verb section, and a `## Unreleased` CHANGELOG section with a `feat:` bullet for the verb and a `fix:` bullet for the auditor. Covers spec 055 AC 10. + + + +Read `CLAUDE.md` for project conventions, then read fully: +- `docs/work-on-session-lifecycle.md` § `## The second writer: the session-connect append` (starting at line 50). The two paragraphs the new text belongs beside are **The generic write verbs refuse the field.** (line 76) and **The concurrent-append race is accepted.** (line 82). The section already documents the append verb, its non-fatal contract, the refusal, and the accepted race; the removal verb is the missing counterpart to all four. +- `CHANGELOG.md` — the preamble block and the newest versioned section (`## v0.149.0`). Note the entry style: a `- : ` bullet, one bullet per logical change, specific enough to name the command and the field. The `## v0.147.0` entry for `task append-metrics-session` is the closest precedent — mirror its level of detail and its sentence structure. +- `pkg/cli/cli.go` — `createTaskRemoveMetricsSessionCommand`, to confirm the exact verb name and argument shape you are documenting. Do NOT edit this file. +- `pkg/ops/metrics_session_write.go` — the refusal's message text, so the doc paragraph and the error agree. + +This is a documentation-only change. No Go code is edited. + + + +**Fail-loud gate.** If `grep -c 'remove-metrics-session' pkg/cli/cli.go` is 0, the verb from prompt 1 has not landed. STOP and report `"status":"failed"` with message `"remove-metrics-session verb not yet deployed (prompt 1)"`. Do NOT document a verb that does not exist, and do NOT implement it here. + +1. **Document the removal verb** in `docs/work-on-session-lifecycle.md` § `## The second writer: the session-connect append`, immediately after the **The generic write verbs refuse the field.** paragraph. Add one or two paragraphs stating: + - the verb's name and shape — `vault-cli task remove-metrics-session ` — and that it removes **every** entry carrying that id while preserving every other entry and every other frontmatter key; + - **why the generic verbs cannot do this**: a `metrics_sessions` entry is a map (`session_id` + `started_at`), while `set` stores a scalar and `add` / `remove` comma-split into a list of scalars, so every reader discards those shapes — which is exactly why the refusal is absolute and why the removal is its own verb rather than an entry in `knownTaskListFields`; + - that when the last entry goes the `metrics_sessions` key is deleted rather than left as an empty list; + - that a call matching nothing exits non-zero and writes nothing, so the failure is visible rather than a silent no-op; + - that the verb writes only the task file, takes no clock, and never writes `claude_session_id` — the id write stays `task set`. + Note that the shared-session rule is the reason per-entry removal exists: the id set is read from `claude_session_id` **and** every `metrics_sessions` id, so clearing the whole field would destroy legitimate runs' entries while a surviving entry would keep the collision alive. `ClearMetricsSessions` (which fires on task completion) is therefore not the tool. ⚠️ The rule itself lives in the Personal vault's `Manager Session` runbook § Step 4, **not** in this document — do not cite a section of this file as its home, and do not spend budget looking for one. + Match the section's existing voice: bold lead-in phrase, then the explanation, in the same register as the neighbouring paragraphs. Do not restructure the section or reorder the existing paragraphs. + +2. **Add a `## Unreleased` section** to `CHANGELOG.md`, immediately below the preamble block and **above** the newest `## vX.Y.Z` section (`## v0.149.0`). It carries exactly two bullets: + - a `feat:` bullet for the removal verb — name the command, the field, the per-entry semantics, the key deletion on the last entry, the loud failure on a non-matching id, the UUID validation before the task is read, and that the generic verbs' refusal is unchanged; + - a `fix:` bullet for the auditor — state that `agents/task-auditor.md` § Task-Goal Alignment no longer recommends a goal link its own alignment check would then score as an orphan, and say what it recommends instead (theme-only linkage). + Follow the `changelog-guide` style: `- : [context]`, one bullet per logical change, no file paths as the subject, no mention of what you verified. Do NOT copy any command or comment out of this prompt's `` section. + +3. **Do NOT bump any version string.** `.claude-plugin/plugin.json`, both `version` fields in `.claude-plugin/marketplace.json`, and the CHANGELOG's newest `## vX.Y.Z` heading are all untouched. This repo is `autoRelease: true` — the `github-releaser` converts `## Unreleased` into a versioned section and bumps the three JSON fields after merge. Hand-bumping here races the releaser and is caught by `make release-check` (`check-versions`), which `make precommit` does not run. + + + +- Do NOT commit — dark-factory handles git. `git diff` only reads; never stage or commit. +- **Documentation only.** No Go file, no test, no agent definition, and no plugin manifest is edited by this prompt. If you find yourself changing `pkg/`, stop — that is prompt 1's scope. +- **Do NOT bump versions** (requirement 3). ⚠️ `make precommit` does **not** run `check-versions` — the alignment check is release-time only (`make release-check`; see `docs/releasing-vault-cli.md` § Version alignment). The four version strings must still agree with each other after your change, and the VERSION-ALIGNMENT GUARD below is what proves they were not touched. A `## Unreleased` section does not participate in that check. +- **Do NOT edit the plugin cache** at `~/.claude/plugins/cache/vault-cli/vault-cli/*` — install artifact. +- **Do NOT touch** `agents/task-auditor.md` (prompt 2) or any file under `pkg/` (prompt 1). This prompt records what they did; it does not redo it. +- Match the existing prose voice. No bullet lists where the section uses paragraphs, no new headings inside the section. + + + +PRIMARY GATE — AC 10: +``` +grep -n 'remove-metrics-session' docs/work-on-session-lifecycle.md # >= 1 line +grep -n 'remove-metrics-session' CHANGELOG.md # >= 1 line +``` + +The `## Unreleased` section exists, sits above the newest `## vX.Y.Z`, and carries both bullets: +``` +grep -n '^## Unreleased' CHANGELOG.md # exactly 1 line +grep -n '^## v' CHANGELOG.md | head -1 # its line number must be GREATER than the Unreleased line +``` +Then confirm, section-scoped to the `## Unreleased` block only, that it holds one `feat:` bullet naming the verb and one `fix:` bullet naming the auditor. A `feat:`/`fix:` match outside that block (in an older versioned section) does NOT satisfy this. Run each and confirm it prints `1`: +``` +awk '/^## Unreleased/{f=1;next} f&&/^## /{exit} f' CHANGELOG.md | grep -c '^- feat:.*remove-metrics-session' +awk '/^## Unreleased/{f=1;next} f&&/^## /{exit} f' CHANGELOG.md | grep -c '^- fix:.*task-auditor' +``` + +VERSION-ALIGNMENT GUARD — the four version strings still agree and were not bumped: +``` +grep -n '^## v' CHANGELOG.md | head -1 # still the pre-existing newest version, unchanged +git diff -- .claude-plugin/plugin.json .claude-plugin/marketplace.json # must be empty +``` + +SCOPE: +``` +git diff --stat # only docs/work-on-session-lifecycle.md and CHANGELOG.md may appear +``` + +FULL GATE — `make precommit` at the repo root must exit 0. This is the batch's authoritative full-gate check and prompt 3 runs last, so it is the one that proves prompts 1 and 2 left the tree green. If it fails on something an earlier prompt introduced, fix that issue here and re-run only the failing target (`make lint`, `make vet`, `make vulncheck`, `make check-changelog`, `make generate`, ...), then `make precommit` once more. + diff --git a/specs/remove-metrics-session.md b/specs/in-progress/055-remove-metrics-session.md similarity index 99% rename from specs/remove-metrics-session.md rename to specs/in-progress/055-remove-metrics-session.md index a310c76f..a1d3158e 100644 --- a/specs/remove-metrics-session.md +++ b/specs/in-progress/055-remove-metrics-session.md @@ -1,5 +1,9 @@ --- -status: draft +status: prompted +approved: "2026-09-27T08:27:23Z" +generating: "2026-09-27T09:06:39Z" +prompted: "2026-09-27T09:39:41Z" +branch: dark-factory/remove-metrics-session --- ## Summary From 0e5a85e4880529480828fc908f8688168fbf1a01 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 27 Sep 2026 12:15:10 +0200 Subject: [PATCH 2/5] Stop the auditor recommending a link it will then score as an orphan (spec 055, prompt 2 of 3) --- agents/task-auditor.md | 1 + .../236-spec-055-auditor-guard-sentence.md | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) rename prompts/{in-progress => completed}/236-spec-055-auditor-guard-sentence.md (95%) diff --git a/agents/task-auditor.md b/agents/task-auditor.md index fa5e2986..f83c4573 100644 --- a/agents/task-auditor.md +++ b/agents/task-auditor.md @@ -241,6 +241,7 @@ For each `[[Goal Name]]` listed in the task's `goals:` frontmatter: 1. Resolve the goal page 2. Match this task to ≥ 1 of the goal's Success Criteria — does the task's Impact / SC reference any of the goal's outcomes? 3. **Flag orphans as MAJOR** — task has a goal link but advances none of its criteria. + - When the goal can be marked complete without this task, never recommend linking a goal the alignment check will then score as an orphan — recommend theme-only linkage instead and say so explicitly. 4. **Flag implementation-level tasks** — if title reads like a low-level code change ("Add field X to struct Y"), check whether a dark-factory spec or prompt is the right artifact instead. **When `goals:` is absent but `themes:` is populated**: do NOT flag as MAJOR orphan. Theme linkage is acceptable for operational, infrastructure, and follow-up tasks where forcing a synthetic parent goal would create goal-creep. Note as MINOR with: "No `goals:` link; theme link covers strategic context. Consider linking a goal if/when one emerges that this task directly advances." diff --git a/prompts/in-progress/236-spec-055-auditor-guard-sentence.md b/prompts/completed/236-spec-055-auditor-guard-sentence.md similarity index 95% rename from prompts/in-progress/236-spec-055-auditor-guard-sentence.md rename to prompts/completed/236-spec-055-auditor-guard-sentence.md index b5bfc790..a43b70c5 100644 --- a/prompts/in-progress/236-spec-055-auditor-guard-sentence.md +++ b/prompts/completed/236-spec-055-auditor-guard-sentence.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [055-remove-metrics-session] +summary: Added one guard sentence to task-auditor.md § Task-Goal Alignment forbidding the auditor from recommending a goal link its own alignment check would then score as a MAJOR orphan, falling back to theme-only linkage instead +execution_id: vault-cli-exec-236-spec-055-auditor-guard-sentence +dark-factory-version: v0.196.0 created: "2026-09-27T09:26:40Z" queued: "2026-09-27T10:04:31Z" +started: "2026-09-27T10:12:34Z" +completed: "2026-09-27T10:15:10Z" branch: dark-factory/remove-metrics-session --- From fa58be0475f3137c9f1c3ce9c2c22b998ef2c398 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 27 Sep 2026 12:17:56 +0200 Subject: [PATCH 3/5] Document the removal verb and record it in the changelog (spec 055, prompt 3 of 3) --- CHANGELOG.md | 1 + docs/work-on-session-lifecycle.md | 19 +++++++++++++++++++ .../237-spec-055-docs-changelog.md | 7 ++++++- 3 files changed, 26 insertions(+), 1 deletion(-) rename prompts/{in-progress => completed}/237-spec-055-docs-changelog.md (96%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4abeac7..04ddbc8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## Unreleased - feat: `vault-cli task remove-metrics-session ` removes every `metrics_sessions` entry carrying the supplied session id from one task, preserving every other entry and every other frontmatter key, and deletes the key outright when the last entry goes. A call whose id matches nothing fails loudly and writes nothing, and an empty, non-UUID or path-bearing id is refused before the task is read. `task set`, `task add` and `task remove` keep refusing `metrics_sessions` unchanged. +- fix: **the `task-auditor` agent's Task-Goal Alignment check no longer recommends a goal link its own alignment check would then score as an orphan.** When the goal can be marked complete without the task, the check now recommends theme-only linkage instead and says so explicitly, so the recommendation and the orphan verdict that follows it agree. ## v0.149.0 diff --git a/docs/work-on-session-lifecycle.md b/docs/work-on-session-lifecycle.md index e35e6423..7bb054f9 100644 --- a/docs/work-on-session-lifecycle.md +++ b/docs/work-on-session-lifecycle.md @@ -79,6 +79,25 @@ stores a scalar and `add` / `remove` comma-split into a list of scalars — both the dedicated list reader discards. That refusal lives in `pkg/ops/metrics_session_write.go`. +**The removal is its own verb.** `vault-cli task remove-metrics-session +` removes **every** entry carrying that id while preserving every other +entry and every other frontmatter key. It cannot be folded into the generic verbs for +the reason the refusal above states: an entry is a map (`session_id` + `started_at`), +while `set` stores a scalar and `add` / `remove` comma-split into a list of scalars, so +every reader discards those shapes. That is why the refusal is absolute and why removal +is a dedicated verb rather than an entry in `knownTaskListFields` — a list field would +admit a shape nothing reads. When the last entry goes the `metrics_sessions` key is +deleted rather than left as an empty list, and a call whose id matches no entry exits +non-zero and writes nothing, so a mistaken id is a visible failure rather than a silent +no-op. The verb writes only the task file, takes no clock, and never touches +`claude_session_id`; the id write stays `task set`. + +**Per-entry removal is what the shared-session rule requires.** The set of session ids +that collide on a task is read from `claude_session_id` **and** every `metrics_sessions` +id, so clearing the whole field would destroy legitimate runs' entries while a surviving +entry would keep the collision alive. `ClearMetricsSessions`, which fires on task +completion, is therefore not the tool. + **The concurrent-append race is accepted.** Two processes appending to the same task file are last-write-wins, the same read-modify-write race the existing `work-on` path has; a lost row is re-appended by re-running the verb. No lock, no re-read and no dedup diff --git a/prompts/in-progress/237-spec-055-docs-changelog.md b/prompts/completed/237-spec-055-docs-changelog.md similarity index 96% rename from prompts/in-progress/237-spec-055-docs-changelog.md rename to prompts/completed/237-spec-055-docs-changelog.md index 1ac98b46..130d0b5b 100644 --- a/prompts/in-progress/237-spec-055-docs-changelog.md +++ b/prompts/completed/237-spec-055-docs-changelog.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [055-remove-metrics-session] +summary: 'Documented the task remove-metrics-session verb in docs/work-on-session-lifecycle.md and added the Unreleased feat: and fix: changelog bullets without bumping versions' +execution_id: vault-cli-exec-237-spec-055-docs-changelog +dark-factory-version: v0.196.0 created: "2026-09-27T09:26:40Z" queued: "2026-09-27T10:04:31Z" +started: "2026-09-27T10:15:12Z" +completed: "2026-09-27T10:17:55Z" branch: dark-factory/remove-metrics-session --- From b68eff9372e7f9b6437f9bf421d4316b153cd0e6 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 27 Sep 2026 12:19:21 +0200 Subject: [PATCH 4/5] docs: restore spec 041's prompts, regenerated out of scope by the daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting the dark-factory daemon to generate spec 055's prompts also re-triggered generation for spec 041, which is approved but whose generation keeps failing ("generation produced no prompt files"). The daemon rewrote all three of 041's prompt files with new created timestamps and edited prose. Those files belong to another spec, are not part of spec 055, and were not reviewed here — so this commit restores them to their state on origin/master. Reverting is the conservative choice: it returns the repo to where it stood before this run touched anything outside its scope. Spec 041's own regeneration failure is recorded on the driving task and is not fixed here. --- prompts/1-spec-041-session-turn-block.md | 20 ++--- prompts/2-spec-041-post-exit-persist.md | 93 +++++++++++------------- prompts/3-spec-041-docs-changelog.md | 57 ++++++--------- 3 files changed, 72 insertions(+), 98 deletions(-) diff --git a/prompts/1-spec-041-session-turn-block.md b/prompts/1-spec-041-session-turn-block.md index ce0de2c4..4621a8cb 100644 --- a/prompts/1-spec-041-session-turn-block.md +++ b/prompts/1-spec-041-session-turn-block.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-27T08:54:45Z" +created: "2026-09-17T15:58:28Z" --- # Confirm the non-interactive session start blocks until the headless turn exits (spec 041, prompt 1 of 3) @@ -13,7 +13,7 @@ created: "2026-09-27T08:54:45Z" - Confirms the interactive terminal branch, its 5-minute cap, and the resume scenario are unchanged. - Backfill 1: renames one test-local variable so the spec's pinned evidence grep for the turn bound matches. The assertion already exists under the old name, so this is a rename with no behaviour change. - Backfill 2: adds the one genuinely missing test — that the turn's temporary output file is deleted after a clean exit. -- Flags spec evidence greps that can never match the real source: three of them carry a literal double quote the source does not contain. This prompt verifies the correct unquoted forms instead, and forbids editing error strings to force a broken grep to pass. +- Flags spec evidence greps that can never match the real source: three of them carry a literal double quote the source does not contain, and one is stale. This prompt verifies the correct unquoted forms instead, and forbids editing error strings to force a broken grep to pass. - Flags that the spec's error-string list for the shared validation helper is pre-spec-045 and must NOT be applied — the shipped strings are the current contract, and "restoring" the spec's forms would revert a later fix. - Makes no production-code change: verification plus two test-only backfills. @@ -25,7 +25,7 @@ Prove — and backfill the two gaps in — the already-shipped half of spec 041: Read `CLAUDE.md` for project conventions. -**Read this first — the spec's Design section is stale on two points.** `specs/in-progress/041-bug-resume-races-live-headless-turn.md` was written against v0.116.4. The `claude_session.go` half of it shipped as commit `247a789` and was then refined by spec 042 (per-session flock locker) and spec 045 (a validated turn result outranks a non-zero child exit), plus a later SC5 fix (`5c2fc57`, the child's reason leads the error). The spec's Design still quotes the pre-045 shapes — in particular its `validateSessionTurn` error strings and its bare `case exitErr := <-done:` handler. Those are superseded; see requirements 4 and 6. Do NOT "restore" them. +**Read this first — the spec's Design section is stale on two points.** `specs/in-progress/041-bug-resume-races-live-headless-turn.md` was written against v0.116.4. The `claude_session.go` half of it shipped as commit `247a789` and was then refined by spec 042 (per-session flock locker) and spec 045 (a validated turn result outranks a non-zero child exit). The spec's Design still quotes the pre-045 shapes — in particular its `validateSessionTurn` error strings and its bare `case exitErr := <-done:` handler. Those are superseded; see requirements 4 and 5. Do NOT "restore" them. Read fully (in this order): - `pkg/ops/claude_session.go` — the whole file (364 lines). This is the file under test. @@ -67,7 +67,7 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read - A waiter goroutine whose only send is `waitCh <- c.waiter.Wait(ctx, c.sessionTurnTimeout)`, on a `waitCh` buffered with capacity 1. - A `select` over `done` and `waitCh`. The read of the output file MUST live inside the `done` branch and must 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. There is a unit test locking this ("fails on timeout even when a valid blob is already on disk"); keep it. -4. **Confirm the `done` branch's current (post-spec-045, post-`5c2fc57`) shape — do NOT replace it with the spec's simpler form.** The spec's Design says a non-nil `exitErr` should immediately return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. That is NOT the shipped contract; spec 045 refined it so a validated turn result outranks a non-zero child exit. The shipped order is: +4. **Confirm the `done` branch's current (post-spec-045) shape — do NOT replace it with the spec's simpler form.** The spec's Design says a non-nil `exitErr` should immediately return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. That is NOT the shipped contract; spec 045 refined it so a validated turn result outranks a non-zero child exit. The shipped order is: - Read the file (`errors.Wrap(ctx, readErr, "read claude output")` on failure), then call `validateSessionTurn(ctx, output)`. - If validation returns nil → return nil. If `exitErr` is non-nil, log it (`slog.Warn("validated turn result overrides non-zero child exit", ...)`) rather than swallowing it silently. - If validation failed AND `exitErr != nil` AND `errors.Is(validateErr, errClaudeOutputUnparseable)` → return `errors.Errorf(ctx, "claude session exited with error: %v", exitErr)`. There is no usable result, so the child's exit status is the only reason that can be named. @@ -94,10 +94,10 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read ```go const SessionTurnTimeout = sessionTurnTimeout ``` - with a comment noting it is a test-only alias: asserting against it locks the WIRING (StartSession hands the constant, not a stray literal) but NOT the value, because a retune moves both sides — so tests must also assert the literal `30 * libtime.Minute`. The file also carries exports that belong to other specs — `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042) and `var RollupFamilyName` / `var RollupMedian` (spec 049). All of them are expected; leave them untouched. This prompt touches nothing in the file. + with a comment noting it is a test-only alias: asserting against it locks the WIRING (StartSession hands the constant, not a stray literal) but NOT the value, because a retune moves both sides — so tests must also assert the literal `30 * libtime.Minute`. The file must also carry `var DefaultSessionLockDir = defaultSessionLockDir` (spec 042's export) — that is expected; leave it untouched. -9. **Confirm the existing test matrix in `pkg/ops/claude_session_test.go`.** In `Context("non-interactive branch", ...)` (opens at line 256) confirm these specs exist and match: - - "blocks until the detached child exits" (line ~327) — a blocking waiter, `Consistently(returned, "100ms").ShouldNot(Receive())` before `doneCh <- nil`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound through `windowCh` and it is asserted equal to BOTH `ops.SessionTurnTimeout` and `30 * libtime.Minute`. +9. **Confirm the existing test matrix in `pkg/ops/claude_session_test.go`.** In `Context("non-interactive branch", ...)` confirm these specs exist and match: + - "blocks until the detached child exits" — a blocking waiter, `Consistently(returned, "100ms").ShouldNot(Receive())` before `doneCh <- nil`, then `Eventually(returned).Should(Receive(BeNil()))`; the waiter receives the bound through `windowCh` and it is asserted equal to BOTH `ops.SessionTurnTimeout` and `30 * libtime.Minute`. - "passes the session id and name to the detached runner" — clean exit with valid JSON returns nil, and argv carries `--session-id`, `--print`, `-n`, the name, and the cwd. - "validates the turn and rejects a zero-turn result", "validates the turn and rejects an is_error result", "rejects an unparseable turn result". - "treats a child exit error as an error" — the error contains `"exit status 1"` AND `"exited with error"`. No assertion anywhere may still use `"exited during startup"`. @@ -159,9 +159,9 @@ The target state for this prompt ALREADY EXISTS in the tree. Your job is to read Expect(statErr).To(HaveOccurred()) }) ``` - Notes: capture `bw := blockWaiter` spec-locally 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. The fake writes valid JSON and then feeds `done`, so the child-exit branch wins and the waiter goroutine stays parked until `DeferCleanup` closes `blockWaiter`. The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the `os.Stat` after the call must fail. No new import is needed — `os` is already imported. `libtime.WaiterDurationFunc` is the real type from `github.com/bborbe/time` (`time_waiter-duration.go`); the closure signature `func(context.Context, libtime.Duration) error` must match it exactly. + Notes: capture `bw := blockWaiter` spec-locally 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. The fake writes valid JSON and then feeds `done`, so the child-exit branch wins and the waiter goroutine stays parked until `DeferCleanup` closes `blockWaiter`. The eager unlink in `runDetachedTurn` runs before `StartSession` returns, so the `os.Stat` after the call must fail. No new import is needed — `os` is already imported. -12. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a spec ("child outlives a cancelled parent wait") that writes a real shell 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. It constructs the starter with the two-argument form `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the locker is spec 042's; keep it). If the file or spec is missing, report `"status":"failed"` — do not re-implement from the spec, whose snippet uses a 12s script and a 1s cancel, neither of which matters to the invariant. +12. **Confirm the detachment integration test.** `pkg/ops/claude_session_detach_test.go` must contain a spec ("child outlives a cancelled parent wait") that writes a real shell 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. It constructs the starter with the two-argument form `ops.NewClaudeSessionStarter(script, ops.NewSessionLockerWithDir(lockDir))` (the locker is spec 042's; keep it). If the file or spec is missing, report `"status":"failed"` — do not re-implement from the spec, whose snippet uses a 12s script and a 1s cancel, neither of which matters to the invariant. 13. **Confirm the AC10 guards by reading, then by grep.** `defaultCommandRunner` is defined once and referenced by both constructors — the grep count in `pkg/ops/claude_session.go` must be exactly 3. `context.WithTimeout` must appear exactly once, on the interactive branch. `scenarios/005-work-on-resume-auto-invokes-subtask.md` must be byte-identical to `HEAD` (see ``). `mocks/claude-session-starter.go` must be untouched: `ClaudeSessionStarter.StartSession`'s signature is `StartSession(context.Context, string, string, string, string, bool) error` — six parameters, unchanged. @@ -179,7 +179,7 @@ Failure-mode coverage carried by this prompt (spec's Failure Modes table): row 1 - 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 constant — do NOT add a config field (spec Non-goals and Open Question 1 both forbid it). - Do NOT alter error strings to satisfy a grep pattern. Three of the spec's evidence greps carry a literal double quote the source does not contain (see ``); the source strings are correct as written. -- Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, `pkg/ops/workon_test.go`, `pkg/ops/goal_workon_test.go`, `pkg/ops/workon_session_writeback_test.go`, `agents/work-on-task-assistant.md`, or `docs/work-on-session-lifecycle.md` in this prompt — the caller-side reorder is prompt 2 and the doc reword is prompt 3. +- Do NOT touch `pkg/ops/workon.go`, `pkg/ops/goal_workon.go`, `pkg/ops/workon_test.go`, `pkg/ops/goal_workon_test.go`, `pkg/ops/workon_session_writeback_test.go`, or `docs/work-on-session-lifecycle.md` in this prompt — the caller-side reorder is prompt 2 and the doc reword is prompt 3. - Do NOT add a double-Start guard and do NOT add any config knob — both are spec Non-goals. - Existing tests must still pass. diff --git a/prompts/2-spec-041-post-exit-persist.md b/prompts/2-spec-041-post-exit-persist.md index de18e027..ceabc8e2 100644 --- a/prompts/2-spec-041-post-exit-persist.md +++ b/prompts/2-spec-041-post-exit-persist.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-27T08:54:45Z" +created: "2026-09-17T15:58:28Z" --- # Persist the task session id only after the headless turn exits (spec 041, prompt 2 of 3) @@ -13,7 +13,7 @@ created: "2026-09-27T08:54:45Z" - Rewords the stale pre-spawn and liveness-window comments in the write-back test file to the post-exit, no-clear semantics. Every on-disk assertion in that file stays byte-identical — the ids never land either way, so only the prose and titles change. - Confirms the goal path and its tests are already in the target state and leaves them untouched. - Runs the spec's AC7-9 evidence gate plus the full repository gate. -- IMPORTANT FLAG FOR THE HUMAN REVIEWER: the task-side half of this spec was deliberately REVERTED in the tree after the spec was approved, because persisting only after the turn left the field empty while the child ran and the child's own session-connect then bound the task to a live, unrelated session. This prompt implements the spec as approved and re-applies the reversion's opposite; the reviewer must adjudicate at audit time. The comment at the top of `` carries the full current evidence — including that the specific mechanism the reversion cited (an mtime transcript scan) was itself removed from the agent definition, while that definition still documents the pre-spawn pre-set as load-bearing. The docs prompt is coupled to this decision. +- IMPORTANT FLAG FOR THE HUMAN REVIEWER: the task-side half of this spec was deliberately REVERTED in the tree after the spec was approved, because persisting only after the turn left the field empty while the child ran and the child's own session-connect then bound the task to a live, unrelated session. This prompt implements the spec as approved and re-applies the reversion's opposite; the reviewer must adjudicate at audit time (details and the two options are in the comment at the top of ``). The docs prompt is coupled to that decision. @@ -24,12 +24,11 @@ Make the task path of `work-on` persist `claude_session_id` only after the detac Read `CLAUDE.md` for project conventions. Read fully (in this order): -- `pkg/ops/workon.go` — the whole file (404 lines). Focus on `handleClaudeSession` (line ~281), `persistSessionAndMetrics` (line ~221), `clearSessionAndMetrics` (line ~250), and `sessionFailureResult` (line ~165). +- `pkg/ops/workon.go` — the whole file (404 lines). Focus on `handleClaudeSession`, `persistSessionAndMetrics`, `clearSessionAndMetrics`, and `sessionFailureResult`. - `pkg/ops/goal_workon.go` — the whole file (240 lines). This is the structural TEMPLATE the reordered task path must match: its `handleClaudeSession` starts the turn first and persists only after it returns cleanly, on both branches, with no compensating clear. -- `pkg/ops/workon_test.go` — the whole file (1096 lines). The contexts this prompt reworks are `"success"` (line 93), `"when the session id write precedes the spawn"` (line 160), `"when the pre-spawn persist re-read fails"` (line 877), `"when persisting the session id before spawning"` (line 906), and `"when the spawn fails"` (line 982). -- `pkg/ops/goal_workon_test.go` — read `Context("when persisting the goal session id after the child exits", ...)` (line 356) and `Context("goal work-on persists nothing for a failed turn", ...)` (line 644) to confirm the goal path is already in the target state. Do not modify this file. +- `pkg/ops/workon_test.go` — the whole file (1096 lines). The contexts this prompt reworks are `"success"`, `"when the session id write precedes the spawn"`, `"when the pre-spawn persist re-read fails"`, `"when persisting the session id before spawning"`, and `"when the spawn fails"`. +- `pkg/ops/goal_workon_test.go` — read `Context("when persisting the goal session id after the child exits", ...)` (~line 356) and `Context("goal work-on persists nothing for a failed turn", ...)` (~line 644) to confirm the goal path is already in the target state. Do not modify this file. - `pkg/ops/workon_session_writeback_test.go` — the whole file (524 lines). Note the shared `newStarter(detachRun)` helper, the `pinnedSessionID` constant, and that every success-path fake already writes a valid turn-JSON line to the caller-owned `stdout *os.File` and then feeds `done <- nil` with a blocking waiter. -- `agents/work-on-task-assistant.md` — read § Session connect (lines ~127-161) and the status-flip line at line 124. This is the agent definition whose contract this reorder interacts with; the reviewer comment in `` quotes it. Do NOT edit it. - `docs/work-on-session-lifecycle.md` — the design record. Its task-path sections currently describe the pre-spawn ordering; prompt 3 rewords them. Do NOT edit the doc here. Coding-plugin docs (in-container paths): @@ -42,32 +41,23 @@ NOTE: git IS available in this container (`.dark-factory.yaml` is `workflow: dir +For the executing agent: implement the requirements below as written. The approved 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, and do NOT add a compensating clear or any other new mechanism that the spec does not name. --> 1. **Fail-loud gate — prompt 1 must have shipped.** Before changing anything, confirm prompt 1's deliverables exist: ``` @@ -77,7 +67,7 @@ For the executing agent: implement the requirements below as written. The approv ``` 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 and do NOT re-implement prompt 1's work. -2. **Reorder the fresh-start path of `workon.go`'s `handleClaudeSession` to start-then-persist.** Replace the block that begins at the `// Persist id + metrics BEFORE the child exists, on both branches, ...` comment (line ~303) and ends at the function's closing `return sessionID, nil` (line ~321), with exactly: +2. **Reorder the fresh-start path of `workon.go`'s `handleClaudeSession` to start-then-persist.** Replace the block that begins at the `// Persist id + metrics BEFORE the child exists, on both branches, ...` comment and ends at the function's closing `return sessionID, nil`, with exactly: ```go // Captured BEFORE the spawn — the turn's true start, not the write time. startedAt := libtime.DateOrDateTime(w.currentDateTime.Now().Time()) @@ -95,14 +85,14 @@ For the executing agent: implement the requirements below as written. The approv The cached-session path at the top of `handleClaudeSession` (the `if existing := task.ClaudeSessionID(); existing != ""` branch) must be UNCHANGED. Note that `persistSessionAndMetrics` is called from two places in this file — the cached path (keep) and the fresh path (this reorder). It has no other callers anywhere in the repo. -3. **Delete the dead `clearSessionAndMetrics` method from `workon.go`.** Remove the whole method — its doc comment and body (lines ~246-272). Its only call site was the block deleted in requirement 2. Do not add a replacement, and do not add any other clearing mechanism (the spec's Non-goals forbid it). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return nothing. `ClearClaudeSessionID()` on the domain type stays — `pkg/ops/complete.go` still uses it. +3. **Delete the dead `clearSessionAndMetrics` method from `workon.go`.** Remove the whole method — its doc comment and body. Its only call site was the block deleted in requirement 2. Do not add a replacement, and do not add any other clearing mechanism (the spec's Non-goals forbid it). After this, `grep -rn 'clearSessionAndMetrics' pkg/` must return nothing. `ClearClaudeSessionID()` on the domain type stays — `pkg/ops/complete.go` still uses it. 4. **Reword the `workon.go` doc comments that describe the pre-spawn design.** - `persistSessionAndMetrics`'s comment: replace `Used pre-spawn on the fresh-start path (the session id is new and must be on disk before the child exists) and on the cached-session path (the id already exists and is preserved).` with `Used post-exit on the fresh-start path (the session id is new and is persisted only after the headless turn completes cleanly) and on the cached-session path (the id already exists and is preserved).` Leave the surrounding sentences about the load-bearing re-read and the empty-id rule as they are. - `handleClaudeSession`'s comment: replace it with the wording `goal_workon.go` already uses for the same method — on both branches the session id is persisted only AFTER the headless turn has finished cleanly, so an id on disk means the session is resumable rather than merely that one was started; nothing is written on any failure path, so there is no compensating clear, and frontmatter the child wrote before failing stays untouched so the Vault UI correctly keeps offering Start. Keep the cached-session sentence. - - `sessionFailureResult`'s comment (line ~163) says the warnings are `the accumulated warnings (including any compensating-clear warning)`. Reword that clause — there is no clear, so the warnings are only the assignee and daily-note ones. Do not change the function's behaviour. + - `sessionFailureResult`'s comment says the warnings are `the accumulated warnings (including any compensating-clear warning)`. Reword that clause — there is no clear, so the warnings are only the assignee and daily-note ones. Do not change the function's behaviour. -5. **Confirm `goal_workon.go` is already in the target state — do NOT change it.** Its `handleClaudeSession` (line ~200) must already start the turn first and persist after on both branches, return `errors.Wrap(ctx, err, "start claude session")` with no compensating clear, and keep the cached path as `return existing, nil`. `persistGoalSessionID` must already return an empty id on failure. `clearGoalSession` must not exist (`grep -c 'clearGoalSession' pkg/ops/goal_workon.go` == 0). If it all matches, leave the file untouched. +5. **Confirm `goal_workon.go` is already in the target state — do NOT change it.** Its `handleClaudeSession` must already start the turn first and persist after on both branches, return `errors.Wrap(ctx, err, "start claude session")` with no compensating clear, and keep the cached path as `return existing, nil`. `persistGoalSessionID` must already return an empty id on failure. `clearGoalSession` must not exist (`grep -c 'clearGoalSession' pkg/ops/goal_workon.go` == 0). If it all matches, leave the file untouched. 6. **Rework `"when persisting the session id before spawning"` in `workon_test.go` to post-exit (AC7).** Rename the context to `"when persisting the session id after the child exits"`. In the variable block rename `spawnAt time.Time` to `childExitAt time.Time`, and in the fake `detachRun` rename the `spawnAt = time.Now()` assignment to `childExitAt = time.Now()` — keep it in the same position (immediately before the buffered `done` channel is fed, which is the child's exit point). The `BeforeEach` already captures `spawnedSessionID` from the `--session-id` argv, writes valid turn JSON to the `stdout *os.File`, and uses a blocking waiter; keep all of that unchanged. Rename the `It` to `"writes the session id to storage only after the child exits"` and replace its body with exactly: ```go @@ -118,11 +108,11 @@ For the executing agent: implement the requirements below as written. The approv ``` The old assertion `Expect(writeTaskAt.Before(spawnAt)).To(BeTrue())` must be gone. This produces AC7's `After(childExitAt)` evidence. -7. **Invert `"when the session id write precedes the spawn"` in `workon_test.go` (line 160).** 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 flip the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. Leave the `WriteTaskStub` / `StartSessionStub` sequencing setup exactly as it is. The ordering is still deterministic: `Execute` writes first (empty id, so `writeSeq` is not set), then `StartSession` runs (`startSeq`), then the post-exit persist writes the id (`writeSeq`). +7. **Invert `"when the session id write precedes the spawn"` in `workon_test.go`.** 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 flip the final assertion from `Expect(writeSeq).To(BeNumerically("<", startSeq))` to `Expect(writeSeq).To(BeNumerically(">", startSeq))`. Leave the `WriteTaskStub` / `StartSessionStub` sequencing setup exactly as it is. The ordering is still deterministic: `Execute` writes first (empty id, so `writeSeq` is not set), then `StartSession` runs (`startSeq`), then the post-exit persist writes the id (`writeSeq`). -8. **Replace the clear-based failure tests in `"when the spawn fails"` with a persists-nothing assertion (AC9).** In that context (line 982): +8. **Replace the clear-based failure tests in `"when the spawn fails"` with a persists-nothing assertion (AC9).** In that context: - Keep `"returns the wrapped spawn error"` and `"returns Success=false"` unchanged. - - DELETE the spec `"clears the pre-persisted session id and the metrics entry for the failed run"` (line 1014) and the whole nested `Context("when the compensating clear itself fails", ...)` (line 1025). + - DELETE the spec `"clears the pre-persisted session id and the metrics entry for the failed run"` and the whole nested `Context("when the compensating clear itself fails", ...)`. - ADD one spec: ```go It("persists no session id when the spawn fails", func() { @@ -133,39 +123,39 @@ For the executing agent: implement the requirements below as written. The approv Expect(writtenMetricsSessions[0]).To(BeEmpty()) }) ``` - - Reword the `BeforeEach` comment (lines ~989-990) that explains the compensating-clear pointer mutation so it reads as a snapshot of what each write lands: with no pre-persist and no clear there is exactly one write, and it carries an empty id because the id is minted inside `handleClaudeSession` after `Execute` has already written the task. Keep the stub itself — it is what makes `writtenIDs` observable. + - Reword the `BeforeEach` comment that explains the compensating-clear pointer mutation so it reads as a snapshot of what each write lands: with no pre-persist and no clear there is exactly one write, and it carries an empty id because the id is minted inside `handleClaudeSession` after `Execute` has already written the task. Keep the stub itself — it is what makes `writtenIDs` observable. -9. **Rework `"when the pre-spawn persist re-read fails"` in `workon_test.go` to post-exit (line 877).** Rename the context to `"when the post-exit persist re-read fails"` and the `It` `"does not append a metrics entry when the pre-spawn re-read fails"` (line 900) to `"does not append a metrics entry when the post-exit re-read fails"`. Reword the `BeforeEach` comment (line 880) from `The failing call is persistSessionAndMetrics' PRE-SPAWN re-read, which runs before StartSession is ever called.` to `The failing call is persistSessionAndMetrics' POST-EXIT re-read, which runs after StartSession returns (the mock starter returns nil immediately).`, and the comment on the write-count `It` (line 896) from `pre-spawn persist failed before writing` to `post-exit persist failed before writing`. The mock call indexes do NOT change: call 0 loads the task, `mockStarter.StartSessionReturns(nil)` makes no storage call, call 1 is the post-exit re-read that fails. All four `It` bodies stay byte-identical. +9. **Rework `"when the pre-spawn persist re-read fails"` in `workon_test.go` to post-exit.** Rename the context to `"when the post-exit persist re-read fails"` and the `It` `"does not append a metrics entry when the pre-spawn re-read fails"` to `"does not append a metrics entry when the post-exit re-read fails"`. Reword the `BeforeEach` comment from `The failing call is persistSessionAndMetrics' PRE-SPAWN re-read, which runs before StartSession is ever called.` to `The failing call is persistSessionAndMetrics' POST-EXIT re-read, which runs after StartSession returns (the mock starter returns nil immediately).`, and the comment on the write-count `It` from `pre-spawn persist failed before writing` to `post-exit persist failed before writing`. The mock call indexes do NOT change: call 0 loads the task, `mockStarter.StartSessionReturns(nil)` makes no storage call, call 1 is the post-exit re-read that fails. All four `It` bodies stay byte-identical. 10. **Reword the remaining pre-spawn wording in `workon_test.go`'s `"success"` context.** Assertions and counts are unchanged; only comments and two titles change: - - `"calls FindTaskByName"` comment (line 112): `Twice: once to load the task, once to re-read it before the child is spawned so the fresh session id lands on disk before the child exists.` → `Twice: once to load the task, once to re-read it after the child exits so the post-exit persist lands the fresh session id without reverting the child's frontmatter writes.` - - `"re-reads the task from the vault path before spawning the session"` (line 110): rename to `"re-reads the task from the vault path after the child exits"` and reword its comment to `The second FindTaskByName is persistSessionAndMetrics' post-exit re-read: the session id is written to disk only after the child has exited.` - - `"Fresh run records one entry"` comment (lines 149-150): `The metrics entry lands in the pre-spawn persist write: write 0 is Execute's status/assignee/phase write, write 1 is the pre-spawn persist.` → `The metrics entry lands in the post-exit persist write: write 0 is Execute's status/assignee/phase write, write 1 is the post-exit persist.` + - `"calls FindTaskByName"` comment: `Twice: once to load the task, once to re-read it before the child is spawned so the fresh session id lands on disk before the child exists.` → `Twice: once to load the task, once to re-read it after the child exits so the post-exit persist lands the fresh session id without reverting the child's frontmatter writes.` + - `"re-reads the task from the vault path before spawning the session"`: rename to `"re-reads the task from the vault path after the child exits"` and reword its comment to `The second FindTaskByName is persistSessionAndMetrics' post-exit re-read: the session id is written to disk only after the child has exited.` + - `"Fresh run records one entry"` comment: `The metrics entry lands in the pre-spawn persist write: write 0 is Execute's status/assignee/phase write, write 1 is the pre-spawn persist.` → `The metrics entry lands in the post-exit persist write: write 0 is Execute's status/assignee/phase write, write 1 is the post-exit persist.` The `Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(2))` assertion stays as it is — Execute's write plus the post-exit persist is still two. 11. **Reword `pkg/ops/workon_session_writeback_test.go` to post-exit semantics — comments and titles ONLY (AC8).** The fakes already write a valid turn-JSON line to the `stdout *os.File` and exit cleanly via `done <- nil` with a blocking waiter; confirm that and do NOT change it. Every on-disk assertion in this file holds unchanged under the new ordering — the ids simply never land — so DO NOT touch any assertion. The full list of prose to change (this is every stale occurrence in the file; the drift-guard grep in `` requires all of them): - - Line 128, the task context's `BeforeEach` comment (work-on persists BEFORE spawning, then the child writes its own frontmatter on top, and nothing writes after) → `Simulate the real headless turn: work-on spawns the child first, then the Claude session runs plan-task -> execute-task and writes its own frontmatter on top of that file inside the detached child (the detachRun fake). Only after the child exits does work-on re-read and persist the session id, so the child's frontmatter survives.` - - Line 187, the task `It`'s comment `The metrics entry lands in the pre-spawn persist write (real storage round-trip) and survives because nothing writes to the file after the child's own write.` → `The metrics entry lands in the post-exit persist write (real storage round-trip), which re-reads the file the child already wrote.` - - Line 216, the goal context's `BeforeEach` comment clause `while the parent has already returned within the liveness window` → `while the parent blocks waiting for the detached turn`. - - Line 270, rename `Context("when the child exits non-zero inside the liveness window", ...)` → `Context("when the child exits non-zero within the turn wait", ...)`. - - Line 286, reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window. A nil-returning waiter would race the select against the child's buffered exit; ...` → `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins. A nil-returning waiter would race the select against the child's buffered exit; ...` (keep the rest of the sentence about the blocking waiter). - - Line 318, rename the `It` `"clears the pre-persisted session id and preserves the child's frontmatter write when the child exited non-zero inside the window"` → `"persists no session id and preserves the child's frontmatter write when the child exited non-zero within the turn wait"`. - - Lines 342-344, reword the body comment (the one describing the pre-spawn persist + compensating clear re-read) so it describes the post-exit, no-clear ordering: the turn failed, so nothing was ever written for this id, and the child's own `phase: planning` write is what survives. - - Lines 353-354, reword the `On-disk shape:` comment (which currently says the pre-spawn persist wrote the id and the compensating clear removed it) to say no id was ever persisted on this path, so its absence from the raw file proves the invariant directly. Keep the explanatory note about the pinned accessor-call counts and the raw-file assertion — that reasoning is unchanged. - - Line 370, reword the comment above `Context("when the child exits non-zero after writing a valid turn result", ...)`: the pre-persisted id is no longer the mechanism — the post-exit persist runs because the validated result outranks the non-zero exit. - - Line 412, rename the `It` `"retains the pre-persisted session id when the turn result validated despite the non-zero exit"` → `"persists the session id when the turn result validated despite the non-zero exit"`. - - Lines 424-425, reword that `It`'s comment so the retain is attributed to the post-exit persist (the validated result makes `StartSession` return nil, so the persist runs) rather than to a pre-spawn write surviving a clear. - - Line 465, reword the `BeforeEach` comment clause `but the blob reports the turn's own failure, so the compensating clear still fires` → `but the blob reports the turn's own failure, so the turn is rejected and nothing is persisted`. - - Line 487, rename the `It` `"clears the pre-persisted session id when the turn result reports its own failure"` → `"persists no session id when the turn result reports its own failure"`. - - Line 514, reword the trailing comment (the one about the compensating clear removing the id) to say the turn failed so nothing was ever persisted, while the child's `phase: planning` write survives. + - Line ~127-132, the task context's `BeforeEach` comment (work-on persists BEFORE spawning, then the child writes its own frontmatter on top, and nothing writes after) → `Simulate the real headless turn: work-on spawns the child first, then the Claude session runs plan-task -> execute-task and writes its own frontmatter on top of that file inside the detached child (the detachRun fake). Only after the child exits does work-on re-read and persist the session id, so the child's frontmatter survives.` + - Line ~187-189, the task `It`'s comment `The metrics entry lands in the pre-spawn persist write (real storage round-trip) and survives because nothing writes to the file after the child's own write.` → `The metrics entry lands in the post-exit persist write (real storage round-trip), which re-reads the file the child already wrote.` + - Line ~216, the goal context's `BeforeEach` comment clause `while the parent has already returned within the liveness window` → `while the parent blocks waiting for the detached turn`. + - Line ~270, rename `Context("when the child exits non-zero inside the liveness window", ...)` → `Context("when the child exits non-zero within the turn wait", ...)`. + - Line ~286-290, reword the comment `The liveness window has NOT elapsed when the child exits, so the starter must treat the exit as inside-the-window. A nil-returning waiter would race the select against the child's buffered exit; ...` → `The turn wait has NOT elapsed when the child exits, so the child-exit branch of the select wins. A nil-returning waiter would race the select against the child's buffered exit; ...` (keep the rest of the sentence about the blocking waiter). + - Line ~318, rename the `It` `"clears the pre-persisted session id and preserves the child's frontmatter write when the child exited non-zero inside the window"` → `"persists no session id and preserves the child's frontmatter write when the child exited non-zero within the turn wait"`. + - Line ~342-347, reword the body comment (the one describing the pre-spawn persist + compensating clear re-read) so it describes the post-exit, no-clear ordering: the turn failed, so nothing was ever written for this id, and the child's own `phase: planning` write is what survives. + - Line ~353-359, reword the `On-disk shape:` comment (which currently says the pre-spawn persist wrote the id and the compensating clear removed it) to say no id was ever persisted on this path, so its absence from the raw file proves the invariant directly. Keep the explanatory note about the pinned accessor-call counts and the raw-file assertion — that reasoning is unchanged. + - Line ~369-371, reword the comment above `Context("when the child exits non-zero after writing a valid turn result", ...)`: the pre-persisted id is no longer the mechanism — the post-exit persist runs because the validated result outranks the non-zero exit. + - Line ~412, rename the `It` `"retains the pre-persisted session id when the turn result validated despite the non-zero exit"` → `"persists the session id when the turn result validated despite the non-zero exit"`. + - Line ~424-427, reword that `It`'s comment so the retain is attributed to the post-exit persist (the validated result makes `StartSession` return nil, so the persist runs) rather than to a pre-spawn write surviving a clear. + - Line ~463-465, reword the `BeforeEach` comment clause `but the blob reports the turn's own failure, so the compensating clear still fires` → `but the blob reports the turn's own failure, so the turn is rejected and nothing is persisted`. + - Line ~487, rename the `It` `"clears the pre-persisted session id when the turn result reports its own failure"` → `"persists no session id when the turn result reports its own failure"`. + - Line ~514-515, reword the trailing comment (the one about the compensating clear removing the id) to say the turn failed so nothing was ever persisted, while the child's `phase: planning` write survives. - Do NOT touch the pinned-count strings anywhere in this file: `TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`. The AC8 greps must keep returning their exact counts, so your reword must not add or remove any occurrence of those tokens. -12. **Confirm the goal AC7 test and the goal assertions are already correct — do not touch them.** `goal_workon_test.go`'s `"when persisting the goal session id after the child exits"` (line 356) already asserts `writeGoalAt.After(childExitAt)` with both non-zero (lines 405-406), and its `"goal work-on persists nothing for a failed turn"` context (line 644) already proves the no-persist half. Leave the file unmodified. +12. **Confirm the goal AC7 test and the goal assertions are already correct — do not touch them.** `goal_workon_test.go`'s `"when persisting the goal session id after the child exits"` already asserts `writeGoalAt.After(childExitAt)` with both non-zero, and its `"goal work-on persists nothing for a failed turn"` context already proves the no-persist half. Leave the file unmodified. -13. **Confirm the spec's Failure Modes rows that this prompt owns.** Row numbering follows the spec's table order: +13. **Confirm the spec's Failure Modes rows that this prompt owns.** - Row 6 (claude binary missing): the `ErrStarterUnavailable` soft path is unchanged — `handleClaudeSession` returns `"", ErrStarterUnavailable` when `w.starter == nil` and `Execute` downgrades it to a warning. Confirm both `workon.go` and `goal_workon.go` still reference it. - Row 7 (post-exit persist fails): covered by requirement 9 — `persistSessionAndMetrics` returns an error AND an empty id when the re-read or the write fails, so the task keeps whatever the turn wrote and no id lands. - - Row 9 (two Start clicks): confirm NO double-start guard was added. It is a documented residual risk and a spec Non-goal. (The spec's Non-goals section cites this as "Failure Modes row 7" — that citation is off by two; two Start clicks is row 9.) The spec 042 per-session lock already exists and is out of this prompt's scope. + - Row 9 (two Start clicks): confirm NO double-start guard was added. It is a documented residual risk and a spec Non-goal. The spec 042 per-session lock already exists and is out of this prompt's scope. 14. **Self-check before finishing.** Re-read every changed hunk and walk spec 041 ACs 7, 8 and 9 against them, naming which artifact satisfies each. Run every command in `` and confirm each holds, including the two flips: `clearSessionAndMetrics` in `workon.go` 3 → 0, and `After(childExitAt)` in `workon_test.go` 0 → ≥ 1. @@ -179,7 +169,6 @@ For the executing agent: implement the requirements below as written. The approv - `ClaudeSessionStarter.StartSession`'s signature is UNCHANGED — six parameters, `mocks/claude-session-starter.go` untouched. `handleClaudeSession`'s `(string, error)` signature is UNCHANGED; the spec's three-value `return "", nil, errors.Wrap(...)` snippet is a typo and must NOT be used. - The pinned-count tokens in `workon_session_writeback_test.go` (`TaskPhaseExecution`, `GoalPhaseExecution`, `session_note`, `MetricsSessions()`, `ClaudeSessionID()`) must remain byte-identical in count — requirement 11's reword is prose-only and must not touch an assertion. - `goal_workon.go` and `goal_workon_test.go` are already in the target state — do not modify them except to read and confirm. -- Do NOT edit `agents/work-on-task-assistant.md`. Its § Session connect text (lines 124, 158) documents the pre-spawn pre-set this reorder removes; updating it is a reviewer-owned follow-up under option (A) of the comment above, and is outside spec 041's stated scope. - Do NOT touch `pkg/ops/claude_session.go`, `pkg/ops/claude_session_test.go`, `pkg/ops/claude_session_detach_test.go`, or `pkg/ops/export_test.go` (prompt 1 owns them), and do NOT touch `docs/work-on-session-lifecycle.md`, `scenarios/002-task-lifecycle.md`, or `CHANGELOG.md` (prompt 3 owns them). - Existing tests must still pass. diff --git a/prompts/3-spec-041-docs-changelog.md b/prompts/3-spec-041-docs-changelog.md index 2d8f5150..67c1e761 100644 --- a/prompts/3-spec-041-docs-changelog.md +++ b/prompts/3-spec-041-docs-changelog.md @@ -1,7 +1,7 @@ --- spec: ["041-bug-resume-races-live-headless-turn"] status: draft -created: "2026-09-27T08:54:45Z" +created: "2026-09-17T15:58:28Z" --- # Reword the work-on lifecycle doc, confirm the scenario, and record the change (spec 041, prompt 3 of 3) @@ -13,7 +13,6 @@ created: "2026-09-27T08:54:45Z" - Does not touch the plugin version strings or any tag — this repository's release agent owns version bumps and tagging after merge, so a feature branch only adds an unreleased bullet. - Runs the repository's full gate as the batch's final validation, and reports the exact exit code. - Coupled to the task-persistence prompt: the document reword and the changelog bullet both describe the ordering that prompt re-applies. If that prompt is rejected at audit, this one must be rejected too, because it would then document behaviour the code does not have. A reviewer comment inside requirement 4 records the tension with the release note that documented the earlier reversion. -- Flags one thing this prompt does NOT fix: the work-on-task agent definition still describes the headless start path as pre-setting the session id before the turn. That text becomes wrong if the reorder ships; it is outside this spec's stated scope and is recorded as a reviewer-owned follow-up. - Confirms the two open questions from the spec are already resolved and need no work: the turn bound stays a constant rather than a config field, and the Vault UI modal copy lives in a different repository and is out of scope. @@ -25,12 +24,11 @@ Bring the durable documentation and the changelog in line with the re-applied po Read `CLAUDE.md` for project conventions. Read fully (in this order): -- `docs/work-on-session-lifecycle.md` — the whole file (242 lines). This is the file under test. +- `docs/work-on-session-lifecycle.md` — the whole file (205 lines). This is the file under test. - `scenarios/002-task-lifecycle.md` — the whole file (67 lines). -- `CHANGELOG.md` — read the top ~20 lines only (the frozen `# Changelog` preamble and the newest versioned section, `## v0.149.0`). That is where the new section and bullet land; the rest of the file is not needed. +- `CHANGELOG.md` — read the top ~40 lines only (the frozen `# Changelog` preamble and the newest versioned sections). That is where the new section and bullet land; the rest of the file is not needed. - `pkg/ops/workon.go` and `pkg/ops/goal_workon.go` — read only to confirm the ordering the documentation must describe. Do not modify either. - `prompts/2-spec-041-post-exit-persist.md` — read the reviewer comment at the top of its `` block; it records the conflict this prompt is coupled to. -- `docs/dod.md` — the repository's Definition of Done; its Documentation section states the changelog placement rule this prompt must satisfy. Coding-plugin docs (in-container paths): - `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — the unreleased-section placement rule, the frozen-preamble rule, the required conventional prefixes, and the rule that in an auto-release repository a feature branch adds bullets under the unreleased section and does NOT bump version strings. @@ -50,8 +48,8 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th ``` If ANY check fails, STOP and report `"status":"failed"` with message `"spec-041 prompt 3 precondition missing: prompt 2 not yet deployed"`. Do NOT proceed, and do NOT reword the documentation to describe an ordering the code does not implement — if prompt 2 was rejected at audit, this prompt is wrong and should be rejected too. -2. **Reword the stale bodies in `docs/work-on-session-lifecycle.md` to the post-exit, no-clear ordering.** The introduction (the paragraph mentioning "spec 040, revised by spec 041", "An id on disk now means the session is resumable") is already correct — keep it. Fix these four bodies, and nothing else: - - **`## Post-exit write ordering`** (line 32) — the heading is correct; the body is stale. Replace the whole body (the paragraph starting `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**...`, the sentence `The **goal path** (pkg/ops/goal_workon.go) keeps its post-exit ordering unchanged: persistGoalSessionID runs only after StartSession returns cleanly.`, and the paragraph starting `On the task path the pre-spawn re-read before writing is load-bearing: ...`) with: +2. **Reword the stale bodies in `docs/work-on-session-lifecycle.md` to the post-exit, no-clear ordering.** The introduction (the paragraph mentioning "spec 040, revised by spec 041", "An id on disk now means the session is resumable") is already correct — keep it. Fix these three bodies, and nothing else: + - **`## Post-exit write ordering`** — the heading is correct; the body is stale. Replace the whole body (the two paragraphs starting `On the **task path** the fresh id and its metrics_sessions entry are now persisted **before the child is spawned**...` and `On the task path the pre-spawn re-read before writing is load-bearing: ...`) with: ``` On both paths — task (`pkg/ops/workon.go`) and goal (`pkg/ops/goal_workon.go`) — the fresh id and its `metrics_sessions` entry are persisted **only after the detached @@ -67,9 +65,9 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th bound expiry, or cancellation — so there is no clear to run, and frontmatter the child wrote before failing stays untouched. ``` - The goal-path sentence is subsumed by the replacement — delete it rather than leaving it as a dangling contrast. - - **`## What the turn timeout does and does not cover`** (line 131) — one sentence in the last-but-one paragraph is stale: `The compensating clear is unchanged: it still fires on every error the detached turn returns. Only the definition of "failed" moved.` Replace it with `No compensating clear runs on any error the detached turn returns — nothing was written before the turn, so there is nothing to undo. Only the definition of "failed" moved.` Leave the rest of that section, including the "read is never hoisted above the select" paragraph, exactly as it is. - - **`## Failure path`** (line 172) — replace the body paragraph starting `On the **task path** the id is pre-persisted, so a failed spawn runs a compensating clear: ...` (lines 174-180) with: + The sentence `The **goal path** (pkg/ops/goal_workon.go) keeps its post-exit ordering unchanged: persistGoalSessionID runs only after StartSession returns cleanly.` is subsumed by the replacement — delete it rather than leaving it as a dangling contrast. + - **`## What the turn timeout does and does not cover`** — one sentence in the last-but-one paragraph is stale: `The compensating clear is unchanged: it still fires on every error the detached turn returns. Only the definition of "failed" moved.` Replace it with `No compensating clear runs on any error the detached turn returns — nothing was written before the turn, so there is nothing to undo. Only the definition of "failed" moved.` Leave the rest of that section, including the "read is never hoisted above the select" paragraph, exactly as it is. + - **`## Failure path`** — replace the body paragraph starting `On the **task path** the id is pre-persisted, so a failed spawn runs a compensating clear: ...` with: ``` On both paths the id is never written before the turn finishes, so there is no clear to run. A failed turn — child exit error, invalid turn result, bound expiry, or @@ -77,8 +75,8 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th child wrote before failing (for example `phase: planning`) survives, and no `claude_session_id` lands. The UI correctly keeps offering **Start**. ``` - Keep the following paragraph about the empty-id rule (starting `The persist step itself can also fail ...`) verbatim — it is already correct. - - **`## The per-session lock`** (line 187) — the `**The detached-child safety property.**` paragraph (lines 234-242) is stale. Replace it with: + Keep the following paragraph about the empty-id rule verbatim — it is already correct. + - **`## The per-session lock`** — the `**The detached-child safety property.**` paragraph is stale. Replace it with: ``` **The detached-child safety property.** On the spawn path, when the parent stops waiting — a failed turn, ctx cancel, or the 30m bound — the detached child keeps @@ -89,12 +87,12 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th running unlocked is not targetable. On any failure nothing was persisted, so the id cannot stay resumable-looking. ``` - - Do NOT touch `## Session id ownership`, `## The second writer: the session-connect append` (line 50), `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, or the rest of `## The per-session lock`. In particular, the phrase "liveness gating" in the lock's "Lock scope" paragraph is a spec-042 Vault UI follow-on concept, NOT the removed liveness-window concept — leave it. `scenarios/005-work-on-resume-auto-invokes-subtask.md` is never edited. - - After the reword the whole file must contain none of the reverted vocabulary — see the drift-guard greps in ``. + - Do NOT touch `## Session id ownership`, `## Why stream-json was rejected`, `## Why the TTY branch is untouched`, `## The fate of --output-format json`, or the rest of `## The per-session lock`. In particular, the phrase "liveness gating" in the lock's "Lock scope" paragraph is a spec-042 Vault UI follow-on concept, NOT the removed liveness-window concept — leave it. `scenarios/005-work-on-resume-auto-invokes-subtask.md` is never edited. + - After the reword the whole file must contain none of the reverted vocabulary — see the drift-guard grep in ``. 3. **Confirm `scenarios/002-task-lifecycle.md` — no edit expected (AC11).** Its work-on action note must already state that the headless turn blocks until completion (it does: `**Both branches block until the turn completes**`, `bounded by a 30m turn timeout`, and `A fast return is a FAIL, not a pass`). Confirm `grep -c '~10s' scenarios/002-task-lifecycle.md` is 0. If it is non-zero, replace the fast-return wording with the blocking wording. Make no other change to this file. -4. **Create the unreleased section in `CHANGELOG.md` and record the change under it (AC12).** Today `CHANGELOG.md` has NO unreleased section — the newest section is `## v0.149.0` (line 11). Insert, immediately after the frozen preamble (the `* MAJOR version...` / `* MINOR version...` / `* PATCH version...` bullet block ending at line 9) and immediately above `## v0.149.0`: +4. **Create the unreleased section in `CHANGELOG.md` and record the change under it (AC12).** Today `CHANGELOG.md` has NO unreleased section — the newest section is `## v0.133.0`. Insert, immediately after the frozen preamble (the `* MAJOR version...` bullet block) and immediately above `## v0.133.0`: ``` ## Unreleased @@ -103,19 +101,17 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th Rules for this edit: - If an unreleased section already exists when you run (a concurrent change may have created it), do NOT create a second one — append this bullet as the last bullet inside the existing section. - Never move, delete, or edit the frozen preamble (`# Changelog`, the "All notable changes…" line, the SemVer link, the MAJOR/MINOR/PATCH bullets). `scripts/check-changelog.sh` fails the build if any `## ` section precedes the preamble line. - - Do NOT delete, reorder, or reword any existing bullet or any `## vX.Y.Z` section. In particular, do NOT touch the `## v0.117.1` section (which recorded the original spec-041 change) or the `## v0.118.3` section (which recorded the reversion this prompt reverses) — the changelog is append-only history and both entries stay as they are. - - Do NOT bump the plugin version strings in `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`, and do NOT create a tag. This repository's release agent owns version bumps and tagging after merge (`.maintainer.yaml` sets `release.autoRelease: true`; see `CLAUDE.md` § Plugin Release Checklist and the changelog guide's "Version Alignment Is Release-Time"). Write `## Unreleased`, never `## vX.Y.Z`. + - Do NOT delete, reorder, or reword any existing bullet or any `## vX.Y.Z` section. + - Do NOT bump the plugin version strings in `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`, and do NOT create a tag. This repository's release agent owns version bumps and tagging after merge (see `CLAUDE.md` § Plugin Release Checklist and the changelog guide's "Version Alignment Is Release-Time"). Write `## Unreleased`, never `## vX.Y.Z`. - The bullet must be within the first 15 lines after the `## Unreleased` heading, and must contain the substring "Resume" (case-insensitive), because AC12's evidence grep reads exactly those 15 lines. -5. **Note, do not fix, the agent-definition drift.** `agents/work-on-task-assistant.md` § Session connect (line 158) still states `A miss is safe **only** on the headless Start path, which pre-sets this field via vault-cli before the turn`, and line 124 describes the status-flip routing that depends on the same pre-set. If prompt 2 ships, that text describes a pre-spawn pre-set the code no longer performs. Updating `agents/work-on-task-assistant.md` is OUTSIDE this spec's stated file scope (the spec's Design and Suggested Decomposition name only `pkg/ops` and `docs/scenarios`), so make NO edit to it in this prompt. Record it in the completion report's `## Improvements` section as a reviewer-owned follow-up (category: PROMPT) so it is not lost. +5. **Confirm the spec's two open questions need no code change.** Open Question 1: the turn bound stays an unexported tunable constant with no config field — do not add one (spec Non-goals). Open Question 2: the Vault UI modal copy is a different repository — no change here. -6. **Confirm the spec's two open questions need no code change.** Open Question 1: the turn bound stays an unexported tunable constant with no config field — do not add one (spec Non-goals). Open Question 2: the Vault UI modal copy is a different repository — no change here. +6. **Full gate (AC13).** Run `make precommit` at the repo root. It must exit 0. If it fails on something this prompt introduced (most likely `check-changelog`), fix it and re-run only the failing target (`make check-changelog`, `make lint`, ...), then `make precommit` once more. Note that `make precommit` does not run `check-versions`; the four version strings are already aligned at the last released version and must stay that way. -7. **Full gate (AC13).** Run `make precommit` at the repo root. It must exit 0. If it fails on something this prompt introduced (most likely `check-changelog`), fix it and re-run only the failing target (`make check-changelog`, `make lint`, ...), then `make precommit` once more. Note that `make precommit` runs `check` (which includes `check-changelog`) but does NOT run `check-versions`; the four version strings are already aligned at the last released version and must stay that way. - -8. **Self-check before finishing.** Re-read the changed doc, scenario, and changelog hunks and walk spec 041 ACs 11, 12 and 13 against them. Run every command in `` and confirm each holds — including the content-level drift guard, which is what actually catches the stale body wording; the spec's own AC11 greps already pass in the current tree and are therefore NOT evidence that the reword was done. +7. **Self-check before finishing.** Re-read the changed doc, scenario, and changelog hunks and walk spec 041 ACs 11, 12 and 13 against them. Run every command in `` and confirm each holds — including the content-level drift guard, which is what actually catches the stale body wording; the spec's own AC11 greps already pass in the current tree and are therefore NOT evidence that the reword was done. @@ -123,9 +119,8 @@ The documentation is in a DRIFTED state. A later change rewrote the BODIES of th - `scenarios/005-work-on-resume-auto-invokes-subtask.md` is untouched — do not edit it. - The interactive TTY branch is unchanged; do not reword any documentation text into claiming otherwise. - Do NOT bump the plugin manifests and do NOT create a tag — only the unreleased bullet is in scope. Write `## Unreleased`, never `## vX.Y.Z`. -- Do NOT delete or reword existing changelog bullets or any `## vX.Y.Z` section — the new bullet is appended inside the unreleased section (or the section is created if absent). The `## v0.117.1` and `## v0.118.3` sections in particular stay byte-identical. +- Do NOT delete or reword existing changelog bullets or any `## vX.Y.Z` section — the new bullet is appended inside the unreleased section (or the section is created if absent). - 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 edit `agents/work-on-task-assistant.md` — its pre-spawn contract text is a reviewer-owned follow-up (requirement 5), not this prompt's work. - Do NOT touch `pkg/ops/*.go` or any `_test.go` file in this prompt — code changes belong to prompts 1 and 2. - Do NOT add a config field for the turn bound (spec Non-goals / Open Question 1). - Existing tests must still pass. @@ -137,25 +132,15 @@ Evidence greps — run each, record the count, and confirm it against the expect ``` grep -c '^## Unreleased' CHANGELOG.md # >= 1 (AC12) — must flip 0 -> >= 1 grep -A15 '^## Unreleased' CHANGELOG.md | grep -ci 'resume' # >= 1 (AC12) — must flip 0 -> >= 1 -grep -c 'only after the detached headless turn exits' CHANGELOG.md # >= 1 if you used the suggested wording +grep -c 'wait for the detached headless turn' CHANGELOG.md # >= 1 if you used the suggested wording ! grep -q 'livenessWindow' docs/work-on-session-lifecycle.md # AC11: absent ! grep -ci 'liveness window' docs/work-on-session-lifecycle.md # AC11: absent (prose form too) ! grep -q '~10s' scenarios/002-task-lifecycle.md # AC11: absent +! grep -qE 'pre-spawn|pre-persisted|pre-persist|before the child is spawned|compensating clear' docs/work-on-session-lifecycle.md # drift guard — the real check for req 2 grep -n -m1 '^All notable changes to this project' CHANGELOG.md # preamble present grep -n -m1 '^## ' CHANGELOG.md # must be the Unreleased heading, AFTER the preamble line above ``` -CONTENT DRIFT GUARD — the real check for requirement 2. The reverted vocabulary must be gone, and the new framing present. The reworded text legitimately contains the words "compensating clear" in a negated sentence ("No compensating clear runs on any error…") and the lock section legitimately keeps "no compensating clears" — so the guard targets the STALE CLAIMS by exact phrase, not the bare two words: - -``` -! grep -qiE 'pre-spawn|pre-persisted|pre-persist|before the child is spawned' docs/work-on-session-lifecycle.md -! grep -q 'runs a compensating clear' docs/work-on-session-lifecycle.md -! grep -q 'The compensating clear is' docs/work-on-session-lifecycle.md -! grep -q 'the compensating clear removes' docs/work-on-session-lifecycle.md -grep -c 'only after the detached headless turn has finished cleanly' docs/work-on-session-lifecycle.md # >= 1 (req 2, Post-exit write ordering) -grep -c 'the id is never written before the turn finishes' docs/work-on-session-lifecycle.md # >= 1 (req 2, Failure path) -``` - FULL GATE — `make precommit` at the repo root must exit 0, and the completion report must carry its actual exit code. If it fails, fix the cause and re-run only the failing target, then `make precommit` once more. The spec's AC10 guard (`git diff --exit-code HEAD -- scenarios/005-work-on-resume-auto-invokes-subtask.md`) was verified in prompt 1 and is not repeated here. From c290801d3b7f705660e138c6accd850e257e4ff2 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 27 Sep 2026 12:19:36 +0200 Subject: [PATCH 5/5] docs: mark spec 055 verifying after all three prompts completed The daemon flipped the spec to verifying when the last of its three prompts landed. Bookkeeping for this spec, carried onto the branch so the PR records the state. --- specs/in-progress/055-remove-metrics-session.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specs/in-progress/055-remove-metrics-session.md b/specs/in-progress/055-remove-metrics-session.md index a1d3158e..d1ba8e16 100644 --- a/specs/in-progress/055-remove-metrics-session.md +++ b/specs/in-progress/055-remove-metrics-session.md @@ -1,8 +1,9 @@ --- -status: prompted +status: verifying approved: "2026-09-27T08:27:23Z" generating: "2026-09-27T09:06:39Z" prompted: "2026-09-27T09:39:41Z" +verifying: "2026-09-27T10:17:56Z" branch: dark-factory/remove-metrics-session ---