diff --git a/CHANGELOG.md b/CHANGELOG.md index b8c03050..bfe7d00b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ 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.1 + - fix: `task-auditor` notes a goal link its task body explicitly frames as non-advancing — the task names the goal but advances none of its criteria, and records why the link is kept — as MINOR instead of scoring it a MAJOR orphan, mirroring `goal-auditor`'s existing explicit-framing carve-out. The framing is read from the task body and never from the goal page, and the clause states that it mitigates the underlying modelling gap (a task cannot declare a topic as its parent) rather than closing it. ## v0.149.0 diff --git a/agents/task-auditor.md b/agents/task-auditor.md index f7b4f807..6213daa6 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/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/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/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/completed/236-spec-055-auditor-guard-sentence.md b/prompts/completed/236-spec-055-auditor-guard-sentence.md new file mode 100644 index 00000000..a43b70c5 --- /dev/null +++ b/prompts/completed/236-spec-055-auditor-guard-sentence.md @@ -0,0 +1,100 @@ +--- +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 +--- + +# 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/completed/237-spec-055-docs-changelog.md b/prompts/completed/237-spec-055-docs-changelog.md new file mode 100644 index 00000000..130d0b5b --- /dev/null +++ b/prompts/completed/237-spec-055-docs-changelog.md @@ -0,0 +1,98 @@ +--- +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 +--- + +# 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..d1ba8e16 100644 --- a/specs/remove-metrics-session.md +++ b/specs/in-progress/055-remove-metrics-session.md @@ -1,5 +1,10 @@ --- -status: draft +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 --- ## Summary