From eed52d2967e133000ef73a75b1302c7d4bef6c9c Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 20 Oct 2026 13:21:18 +0200 Subject: [PATCH] feat: add goal and topic approve operations and the set-path guard (spec 061, part 1 of 2) --- mocks/goal-approve-operation.go | 123 +++++++++++ mocks/topic-approve-operation.go | 123 +++++++++++ pkg/ops/approval_record.go | 39 ++++ pkg/ops/frontmatter_entity.go | 78 +++++++ pkg/ops/frontmatter_entity_test.go | 167 ++++++++++++++ pkg/ops/goal_approve.go | 130 +++++++++++ pkg/ops/goal_approve_test.go | 205 ++++++++++++++++++ pkg/ops/topic_approve.go | 130 +++++++++++ pkg/ops/topic_approve_test.go | 204 +++++++++++++++++ .../completed/261-spec-061-set-path-guard.md | 168 ++++++++++++++ .../completed/262-spec-061-goal-approve.md | 175 +++++++++++++++ .../completed/263-spec-061-topic-approve.md | 160 ++++++++++++++ prompts/completed/264-spec-061-approve-cli.md | 188 ++++++++++++++++ .../completed/265-spec-061-docs-changelog.md | 125 +++++++++++ specs/completed/061-goal-topic-approve.md | 143 ++++++++++++ 15 files changed, 2158 insertions(+) create mode 100644 mocks/goal-approve-operation.go create mode 100644 mocks/topic-approve-operation.go create mode 100644 pkg/ops/approval_record.go create mode 100644 pkg/ops/goal_approve.go create mode 100644 pkg/ops/goal_approve_test.go create mode 100644 pkg/ops/topic_approve.go create mode 100644 pkg/ops/topic_approve_test.go create mode 100644 prompts/completed/261-spec-061-set-path-guard.md create mode 100644 prompts/completed/262-spec-061-goal-approve.md create mode 100644 prompts/completed/263-spec-061-topic-approve.md create mode 100644 prompts/completed/264-spec-061-approve-cli.md create mode 100644 prompts/completed/265-spec-061-docs-changelog.md create mode 100644 specs/completed/061-goal-topic-approve.md diff --git a/mocks/goal-approve-operation.go b/mocks/goal-approve-operation.go new file mode 100644 index 00000000..b707df7b --- /dev/null +++ b/mocks/goal-approve-operation.go @@ -0,0 +1,123 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package mocks + +import ( + "context" + "sync" + + "github.com/bborbe/vault-cli/pkg/ops" +) + +type GoalApproveOperation struct { + ExecuteStub func(context.Context, string, string, string, string) (ops.MutationResult, error) + executeMutex sync.RWMutex + executeArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + arg5 string + } + executeReturns struct { + result1 ops.MutationResult + result2 error + } + executeReturnsOnCall map[int]struct { + result1 ops.MutationResult + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *GoalApproveOperation) Execute(arg1 context.Context, arg2 string, arg3 string, arg4 string, arg5 string) (ops.MutationResult, 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 + arg5 string + }{arg1, arg2, arg3, arg4, arg5}) + stub := fake.ExecuteStub + fakeReturns := fake.executeReturns + fake.recordInvocation("Execute", []interface{}{arg1, arg2, arg3, arg4, arg5}) + fake.executeMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4, arg5) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *GoalApproveOperation) ExecuteCallCount() int { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + return len(fake.executeArgsForCall) +} + +func (fake *GoalApproveOperation) ExecuteCalls(stub func(context.Context, string, string, string, string) (ops.MutationResult, error)) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = stub +} + +func (fake *GoalApproveOperation) ExecuteArgsForCall(i int) (context.Context, string, string, string, string) { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + argsForCall := fake.executeArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4, argsForCall.arg5 +} + +func (fake *GoalApproveOperation) ExecuteReturns(result1 ops.MutationResult, result2 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + fake.executeReturns = struct { + result1 ops.MutationResult + result2 error + }{result1, result2} +} + +func (fake *GoalApproveOperation) ExecuteReturnsOnCall(i int, result1 ops.MutationResult, result2 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + if fake.executeReturnsOnCall == nil { + fake.executeReturnsOnCall = make(map[int]struct { + result1 ops.MutationResult + result2 error + }) + } + fake.executeReturnsOnCall[i] = struct { + result1 ops.MutationResult + result2 error + }{result1, result2} +} + +func (fake *GoalApproveOperation) 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 *GoalApproveOperation) 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.GoalApproveOperation = new(GoalApproveOperation) diff --git a/mocks/topic-approve-operation.go b/mocks/topic-approve-operation.go new file mode 100644 index 00000000..26d0d62a --- /dev/null +++ b/mocks/topic-approve-operation.go @@ -0,0 +1,123 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package mocks + +import ( + "context" + "sync" + + "github.com/bborbe/vault-cli/pkg/ops" +) + +type TopicApproveOperation struct { + ExecuteStub func(context.Context, string, string, string, string) (ops.MutationResult, error) + executeMutex sync.RWMutex + executeArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + arg4 string + arg5 string + } + executeReturns struct { + result1 ops.MutationResult + result2 error + } + executeReturnsOnCall map[int]struct { + result1 ops.MutationResult + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *TopicApproveOperation) Execute(arg1 context.Context, arg2 string, arg3 string, arg4 string, arg5 string) (ops.MutationResult, 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 + arg5 string + }{arg1, arg2, arg3, arg4, arg5}) + stub := fake.ExecuteStub + fakeReturns := fake.executeReturns + fake.recordInvocation("Execute", []interface{}{arg1, arg2, arg3, arg4, arg5}) + fake.executeMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4, arg5) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *TopicApproveOperation) ExecuteCallCount() int { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + return len(fake.executeArgsForCall) +} + +func (fake *TopicApproveOperation) ExecuteCalls(stub func(context.Context, string, string, string, string) (ops.MutationResult, error)) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = stub +} + +func (fake *TopicApproveOperation) ExecuteArgsForCall(i int) (context.Context, string, string, string, string) { + fake.executeMutex.RLock() + defer fake.executeMutex.RUnlock() + argsForCall := fake.executeArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4, argsForCall.arg5 +} + +func (fake *TopicApproveOperation) ExecuteReturns(result1 ops.MutationResult, result2 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + fake.executeReturns = struct { + result1 ops.MutationResult + result2 error + }{result1, result2} +} + +func (fake *TopicApproveOperation) ExecuteReturnsOnCall(i int, result1 ops.MutationResult, result2 error) { + fake.executeMutex.Lock() + defer fake.executeMutex.Unlock() + fake.ExecuteStub = nil + if fake.executeReturnsOnCall == nil { + fake.executeReturnsOnCall = make(map[int]struct { + result1 ops.MutationResult + result2 error + }) + } + fake.executeReturnsOnCall[i] = struct { + result1 ops.MutationResult + result2 error + }{result1, result2} +} + +func (fake *TopicApproveOperation) 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 *TopicApproveOperation) 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.TopicApproveOperation = new(TopicApproveOperation) diff --git a/pkg/ops/approval_record.go b/pkg/ops/approval_record.go new file mode 100644 index 00000000..a6178853 --- /dev/null +++ b/pkg/ops/approval_record.go @@ -0,0 +1,39 @@ +// 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" + "strings" + + "github.com/bborbe/errors" +) + +// refuseExistingEntityApprovalRecord returns the refusal error for a goal or +// topic row that already carries an approved_by or an approved_at key, and nil +// otherwise. Both keys are checked independently so a row carrying either one +// is refused, and the message names every key that is present. +func refuseExistingEntityApprovalRecord( + ctx context.Context, + kind string, + name string, + get func(string) any, +) error { + var present []string + if get("approved_by") != nil { + present = append(present, "approved_by") + } + if get("approved_at") != nil { + present = append(present, "approved_at") + } + if len(present) == 0 { + return nil + } + return errors.Errorf( + ctx, + "refusing to approve %q: %s already carries an approval record (%s); re-approving is refused", + name, kind, strings.Join(present, ", "), + ) +} diff --git a/pkg/ops/frontmatter_entity.go b/pkg/ops/frontmatter_entity.go index 7073f45e..1a517e8e 100644 --- a/pkg/ops/frontmatter_entity.go +++ b/pkg/ops/frontmatter_entity.go @@ -125,6 +125,13 @@ func (o *goalSetOperation) Execute( return err } + // Refuse a direct todo -> planning move before anything is mutated — see + // goalTodoPlanningSetRefusal. Nothing below runs on this path, so the goal + // file stays byte-identical. + if err := goalTodoPlanningSetRefusal(ctx, goal, entityName, key, value); err != nil { + return err + } + // One-step close-out: when this invocation sets a close-out goal status, // persist reason and successor first so both land in a single WriteGoal. if err := writeGoalCloseOutFieldsIfCloseOut(ctx, goal, value, reason, gateSuccessor); err != nil { @@ -274,6 +281,14 @@ func (o *topicSetOperation) Execute( if err != nil { return errors.Wrap(ctx, err, "find topic") } + + // Refuse a direct todo -> planning move before anything is mutated — see + // topicTodoPlanningSetRefusal. Nothing below runs on this path, so the topic + // file stays byte-identical. + if err := topicTodoPlanningSetRefusal(ctx, topic, entityName, key, value); err != nil { + return err + } + if err := topic.SetField(ctx, key, value); err != nil { return errors.Wrap(ctx, err, fmt.Sprintf("set field %q", key)) } @@ -288,6 +303,69 @@ func NewTopicSetOperation(topicStorage storage.TopicStorage) EntitySetOperation return &topicSetOperation{topicStorage: topicStorage} } +// goalTodoPlanningSetRefusal returns an error when a `goal set` invocation would +// move a goal out of the operator's approval inbox by setting the phase to +// planning. +// +// "todo" is the operator's approval inbox. Leaving it is the operator's approval +// and is performed by `vault-cli goal approve`, which records approved_by and +// approved_at in the same write. `goal set phase planning` writes the +// field and nothing else, leaving the row at planning with no evidence that +// anyone approved it. The refusal closes that route. +// +// The guard is deliberately one (field, value, current-phase) combination: key +// "phase", the canonical value "planning", and a current phase of "todo". A goal +// with no phase key is exempt — an absent phase does not mean todo. Every other +// target phase, every non-phase key and every unknown phase value keep their +// existing behaviour; an unknown value is still rejected by setPhaseField. +// +// Nothing is written on the refusal path: the check runs before any mutation, so +// the file on disk is byte-identical. +func goalTodoPlanningSetRefusal( + ctx context.Context, + goal *domain.Goal, + goalName, key, value string, +) error { + if key != "phase" { + return nil + } + if domain.GoalPhase(value) != domain.GoalPhasePlanning { + return nil + } + current := goal.Phase() + if current == nil || *current != domain.GoalPhaseTodo { + return nil + } + return errors.Errorf(ctx, + "refusing to set phase %q on goal %q: the goal is at phase %q, the operator's approval inbox. Leaving it is the operator's approval and is performed by `vault-cli goal approve %q`, which records approved_by and approved_at in the same write", + domain.GoalPhasePlanning, goalName, string(domain.GoalPhaseTodo), goalName, + ) +} + +// topicTodoPlanningSetRefusal is the topic arm of goalTodoPlanningSetRefusal: +// the same single (field, value, current-phase) triple, the same exemption for a +// missing phase key, and the same byte-identical refusal path. +func topicTodoPlanningSetRefusal( + ctx context.Context, + topic *domain.Topic, + topicName, key, value string, +) error { + if key != "phase" { + return nil + } + if domain.TopicPhase(value) != domain.TopicPhasePlanning { + return nil + } + current := topic.Phase() + if current == nil || *current != domain.TopicPhaseTodo { + return nil + } + return errors.Errorf(ctx, + "refusing to set phase %q on topic %q: the topic is at phase %q, the operator's approval inbox. Leaving it is the operator's approval and is performed by `vault-cli topic approve %q`, which records approved_by and approved_at in the same write", + domain.TopicPhasePlanning, topicName, string(domain.TopicPhaseTodo), topicName, + ) +} + // EntityClearOperation clears a single frontmatter field value on an entity. // //counterfeiter:generate -o ../../mocks/entity-clear-operation.go --fake-name EntityClearOperation . EntityClearOperation diff --git a/pkg/ops/frontmatter_entity_test.go b/pkg/ops/frontmatter_entity_test.go index 4a8c312a..7ad0e1de 100644 --- a/pkg/ops/frontmatter_entity_test.go +++ b/pkg/ops/frontmatter_entity_test.go @@ -328,6 +328,90 @@ var _ = Describe("NewGoalSetOperation", func() { Expect(err).To(MatchError(ContainSubstring("write goal"))) }) }) + + Context("goal at phase todo", func() { + BeforeEach(func() { + goal = domain.NewGoal( + map[string]any{"phase": "todo", "status": "next"}, + domain.FileMetadata{Name: goalName}, + domain.Content(""), + ) + mockGoalStorage.FindGoalByNameReturns(goal, nil) + }) + + Context("setting phase planning", func() { + BeforeEach(func() { + key = "phase" + value = "planning" + }) + + It("refuses and writes nothing", func() { + Expect(err).To(MatchError(ContainSubstring("goal approve"))) + Expect(mockGoalStorage.WriteGoalCallCount()).To(Equal(0)) + Expect(goal.Phase()).NotTo(BeNil()) + Expect(*goal.Phase()).To(Equal(domain.GoalPhaseTodo)) + }) + }) + + Context("setting phase execution", func() { + BeforeEach(func() { + key = "phase" + value = "execution" + }) + + It("allows the move and writes the goal", func() { + Expect(err).To(BeNil()) + Expect(mockGoalStorage.WriteGoalCallCount()).To(Equal(1)) + _, writtenGoal := mockGoalStorage.WriteGoalArgsForCall(0) + Expect(writtenGoal.Phase()).NotTo(BeNil()) + Expect(*writtenGoal.Phase()).To(Equal(domain.GoalPhaseExecution)) + }) + }) + + Context("setting a non-phase key", func() { + BeforeEach(func() { + key = "priority" + value = "2" + }) + + It("allows the write", func() { + Expect(err).To(BeNil()) + Expect(mockGoalStorage.WriteGoalCallCount()).To(Equal(1)) + _, writtenGoal := mockGoalStorage.WriteGoalArgsForCall(0) + Expect(writtenGoal.Priority()).To(Equal(domain.Priority(2))) + }) + }) + }) + + Context("goal with no phase key", func() { + BeforeEach(func() { + key = "phase" + value = "planning" + }) + + It("stays exempt from the refusal", func() { + Expect(err).To(BeNil()) + Expect(mockGoalStorage.WriteGoalCallCount()).To(Equal(1)) + }) + }) + + Context("goal at phase planning", func() { + BeforeEach(func() { + goal = domain.NewGoal( + map[string]any{"phase": "planning"}, + domain.FileMetadata{Name: goalName}, + domain.Content(""), + ) + mockGoalStorage.FindGoalByNameReturns(goal, nil) + key = "phase" + value = "planning" + }) + + It("is not guarded", func() { + Expect(err).To(BeNil()) + Expect(mockGoalStorage.WriteGoalCallCount()).To(Equal(1)) + }) + }) }) var _ = Describe("NewGoalClearOperation", func() { @@ -1273,6 +1357,89 @@ var _ = Describe("NewTopicSetOperation", func() { Expect(err).To(MatchError(ContainSubstring("write topic"))) }) }) + + Context("topic at phase todo", func() { + BeforeEach(func() { + topic = domain.NewTopic( + map[string]any{"phase": "todo", "status": "next"}, + domain.FileMetadata{Name: topicName, FilePath: "/vault/Topics/my-topic.md"}, + domain.Content(""), + ) + mockTopicStorage.FindTopicByNameReturns(topic, nil) + }) + + Context("setting phase planning", func() { + BeforeEach(func() { + key = "phase" + value = "planning" + }) + + It("refuses and writes nothing", func() { + Expect(err).To(MatchError(ContainSubstring("topic approve"))) + Expect(mockTopicStorage.WriteTopicCallCount()).To(Equal(0)) + Expect(topic.Phase()).NotTo(BeNil()) + Expect(*topic.Phase()).To(Equal(domain.TopicPhaseTodo)) + }) + }) + + Context("setting phase execution", func() { + BeforeEach(func() { + key = "phase" + value = "execution" + }) + + It("allows the move and writes the topic", func() { + Expect(err).To(BeNil()) + Expect(mockTopicStorage.WriteTopicCallCount()).To(Equal(1)) + _, written := mockTopicStorage.WriteTopicArgsForCall(0) + Expect(written.GetField("phase")).To(Equal("execution")) + }) + }) + + Context("setting a non-phase key", func() { + BeforeEach(func() { + key = "priority" + value = "2" + }) + + It("allows the write", func() { + Expect(err).To(BeNil()) + Expect(mockTopicStorage.WriteTopicCallCount()).To(Equal(1)) + _, written := mockTopicStorage.WriteTopicArgsForCall(0) + Expect(written.GetField("priority")).To(Equal("2")) + }) + }) + }) + + Context("topic with no phase key", func() { + BeforeEach(func() { + key = "phase" + value = "planning" + }) + + It("stays exempt from the refusal", func() { + Expect(err).To(BeNil()) + Expect(mockTopicStorage.WriteTopicCallCount()).To(Equal(1)) + }) + }) + + Context("topic at phase planning", func() { + BeforeEach(func() { + topic = domain.NewTopic( + map[string]any{"phase": "planning"}, + domain.FileMetadata{Name: topicName, FilePath: "/vault/Topics/my-topic.md"}, + domain.Content(""), + ) + mockTopicStorage.FindTopicByNameReturns(topic, nil) + key = "phase" + value = "planning" + }) + + It("is not guarded", func() { + Expect(err).To(BeNil()) + Expect(mockTopicStorage.WriteTopicCallCount()).To(Equal(1)) + }) + }) }) var _ = Describe("NewTopicClearOperation", func() { diff --git a/pkg/ops/goal_approve.go b/pkg/ops/goal_approve.go new file mode 100644 index 00000000..e228878c --- /dev/null +++ b/pkg/ops/goal_approve.go @@ -0,0 +1,130 @@ +// 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. + +//nolint:dupl // Structurally parallel to the sibling topic's approve; frozen field names prevent dedup +package ops + +import ( + "context" + + "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/storage" +) + +//counterfeiter:generate -o ../../mocks/goal-approve-operation.go --fake-name GoalApproveOperation . GoalApproveOperation +type GoalApproveOperation interface { + // Execute approves the named goal: it moves a goal waiting in the approval + // inbox (phase todo) to phase planning and records, in the same write, who + // approved it and the instant of the transition. A goal that is not waiting + // in the inbox, or that already carries an approval record, is refused with + // nothing written. + // + // Goals are not required to have an owner, and this verb does not assign + // one. + Execute( + ctx context.Context, + vaultPath string, + goalName string, + vaultName string, + approvedBy string, + ) (MutationResult, error) +} + +// NewGoalApproveOperation creates a new goal approve operation. +func NewGoalApproveOperation( + goalStorage storage.GoalStorage, + currentDateTime libtime.CurrentDateTime, +) GoalApproveOperation { + return &goalApproveOperation{ + goalStorage: goalStorage, + currentDateTime: currentDateTime, + } +} + +type goalApproveOperation struct { + goalStorage storage.GoalStorage + currentDateTime libtime.CurrentDateTime +} + +// Execute approves a goal waiting in the approval inbox. +func (o *goalApproveOperation) Execute( + ctx context.Context, + vaultPath string, + goalName string, + vaultName string, + approvedBy string, +) (MutationResult, error) { + // Refuse an empty approver before reading the goal: a present-but-blank + // approved_by is worse than no record at all, and nothing is written here. + if approvedBy == "" { + err := errors.Errorf( + ctx, + "approved_by must not be empty: refusing to record a blank approver on goal %q", + goalName, + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + goal, err := o.goalStorage.FindGoalByName(ctx, vaultPath, goalName) + if err != nil { + return MutationResult{ + Success: false, + Error: err.Error(), + }, errors.Wrap(ctx, err, "find goal") + } + + // Read and validate the clock before any guard: a zero instant would be + // recorded as a placeholder rather than a measurement, so it is refused. + now := o.currentDateTime.Now() + if now.Time().IsZero() { + err := errors.Errorf( + ctx, + "approved_at is not a valid instant: refusing to record a zero timestamp on goal %q", + goalName, + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + // The inbox guard: only a goal at phase todo is waiting for approval. The + // message names both the current phase and the expected one so the operator + // can see what was found and what was required. + if phase := goal.Phase(); phase == nil || *phase != domain.GoalPhaseTodo { + current := "(none)" + if phase != nil { + current = string(*phase) + } + err := errors.Errorf( + ctx, + "refusing to approve %q: goal is at phase %q, not %q; only a goal in the approval inbox (phase %q) can be approved", + goalName, current, string(domain.GoalPhaseTodo), string(domain.GoalPhaseTodo), + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + // The existing-record guard: a todo row carrying an approval record is not + // awaiting approval, and re-approving must not overwrite who approved it first. + if err := refuseExistingEntityApprovalRecord(ctx, "goal", goalName, goal.Get); err != nil { + return MutationResult{Success: false, Error: err.Error()}, err + } + + // All three keys are composed onto the existing map, then handed to storage + // in exactly one write — so a row can never end up planned without an + // approval record beside it. Every other key, known or unknown, survives + // because the map itself is preserved. + goal.SetPhase(domain.GoalPhasePlanning.Ptr()) + goal.Set("approved_by", approvedBy) + goal.Set("approved_at", now.Time()) + + if err := o.goalStorage.WriteGoal(ctx, goal); err != nil { + return MutationResult{ + Success: false, + Error: err.Error(), + }, errors.Wrap(ctx, err, "write goal") + } + + return MutationResult{Success: true, Name: goal.Name, Vault: vaultName}, nil +} diff --git a/pkg/ops/goal_approve_test.go b/pkg/ops/goal_approve_test.go new file mode 100644 index 00000000..cf91e05d --- /dev/null +++ b/pkg/ops/goal_approve_test.go @@ -0,0 +1,205 @@ +// 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" + "time" + + liberrors "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + libtimetest "github.com/bborbe/time/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "gopkg.in/yaml.v3" + + "github.com/bborbe/vault-cli/mocks" + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/ops" +) + +var _ = Describe("GoalApproveOperation", func() { + var ( + ctx context.Context + mockStorage *mocks.Storage + pinned libtime.CurrentDateTime + approveOp ops.GoalApproveOperation + ) + + // seedGoal replaces the goal the storage returns, so a spec can control the + // phase and the pre-existing frontmatter the approve guards inspect. + seedGoal := func(fields map[string]any) { + mockStorage.FindGoalByNameReturns( + domain.NewGoal(fields, domain.FileMetadata{Name: "Alpha"}, domain.Content("")), + nil, + ) + } + + BeforeEach(func() { + ctx = context.Background() + mockStorage = &mocks.Storage{} + mockStorage.WriteGoalReturns(nil) + + pinned = libtime.NewCurrentDateTime() + pinned.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) + approveOp = ops.NewGoalApproveOperation(mockStorage, pinned) + + seedGoal(map[string]any{"status": "next", "phase": "todo"}) + }) + + It("writes all three approval keys in exactly one write", func() { + result, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + Expect(result.Success).To(BeTrue()) + Expect(result.Name).To(Equal("Alpha")) + Expect(result.Vault).To(Equal("vault-a")) + + Expect(mockStorage.WriteGoalCallCount()).To(Equal(1)) + _, written := mockStorage.WriteGoalArgsForCall(0) + Expect(written.Phase()).To(Equal(domain.GoalPhasePlanning.Ptr())) + Expect(written.GetString("approved_by")).To(Equal("operator")) + Expect(written.Get("approved_at")).NotTo(BeNil()) + }) + + It("records the injected instant as a bare time.Time", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteGoalArgsForCall(0) + raw, ok := written.Get("approved_at").(time.Time) + Expect(ok).To(BeTrue()) + Expect(raw.Equal(pinned.Now().Time())).To(BeTrue()) + }) + + It("serializes approved_at unquoted and round-trips it as a time.Time", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteGoalArgsForCall(0) + out, marshalErr := yaml.Marshal(written.RawMap()) + Expect(marshalErr).NotTo(HaveOccurred()) + Expect(string(out)).To(ContainSubstring("approved_at: 2026-03-03T12:00:00Z")) + Expect(string(out)).NotTo(ContainSubstring(`approved_at: "2026-03-03T12:00:00Z"`)) + + var roundTripped map[string]any + Expect(yaml.Unmarshal(out, &roundTripped)).To(Succeed()) + rt, ok := roundTripped["approved_at"].(time.Time) + Expect(ok).To(BeTrue()) + Expect(rt.Equal(pinned.Now().Time())).To(BeTrue()) + }) + + It("writes the approver named on the call", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "Manager Layer") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteGoalArgsForCall(0) + Expect(written.GetString("approved_by")).To(Equal("Manager Layer")) + }) + + It("preserves every other frontmatter key", func() { + seedGoal(map[string]any{ + "status": "next", + "phase": "todo", + "priority": 1, + "page_type": "goal", + "theme": "Work", + "assignee": "someone", + "custom_key": "keep", + }) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteGoalArgsForCall(0) + Expect(written.Priority()).To(Equal(domain.Priority(1))) + Expect(written.PageType()).To(Equal("goal")) + Expect(written.Theme()).To(Equal("Work")) + Expect(written.Assignee()).To(Equal("someone")) + Expect(written.Get("custom_key")).To(Equal("keep")) + }) + + It("refuses an empty approver before reading the goal", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("approved_by")) + Expect(mockStorage.FindGoalByNameCallCount()).To(Equal(0)) + Expect(mockStorage.WriteGoalCallCount()).To(Equal(0)) + }) + + It("refuses a zero clock with zero writes", func() { + zero := libtime.NewCurrentDateTime() + zero.SetNow(libtime.DateTime(time.Time{})) + zeroOp := ops.NewGoalApproveOperation(mockStorage, zero) + + _, execErr := zeroOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("approved_at")) + Expect(mockStorage.WriteGoalCallCount()).To(Equal(0)) + }) + + DescribeTable("refuses a goal that is not waiting in the inbox", + func(fields map[string]any, expectedPhase string) { + seedGoal(fields) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring(expectedPhase)) + Expect(execErr.Error()).To(ContainSubstring("todo")) + Expect(mockStorage.WriteGoalCallCount()).To(Equal(0)) + }, + Entry("planning", map[string]any{"status": "next", "phase": "planning"}, "planning"), + Entry("execution", map[string]any{"status": "next", "phase": "execution"}, "execution"), + Entry("done", map[string]any{"status": "next", "phase": "done"}, "done"), + Entry("no phase key at all", map[string]any{"status": "next"}, "(none)"), + ) + + DescribeTable("refuses a todo goal that already carries an approval record", + func(fields map[string]any, expectedKeys string) { + seedGoal(fields) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring(expectedKeys)) + Expect(execErr.Error()).To(ContainSubstring("re-approving")) + Expect(mockStorage.WriteGoalCallCount()).To(Equal(0)) + }, + Entry("only approved_by", + map[string]any{"status": "next", "phase": "todo", "approved_by": "someone"}, + "approved_by", + ), + Entry("only approved_at", + map[string]any{ + "status": "next", + "phase": "todo", + "approved_at": "2026-01-01T00:00:00Z", + }, + "approved_at", + ), + Entry("both keys", + map[string]any{ + "status": "next", + "phase": "todo", + "approved_by": "someone", + "approved_at": "2026-01-01T00:00:00Z", + }, + "approved_by, approved_at", + ), + ) + + It("wraps a find failure", func() { + mockStorage.FindGoalByNameReturns(nil, liberrors.New(ctx, "boom")) + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("find goal")) + Expect(mockStorage.WriteGoalCallCount()).To(Equal(0)) + }) + + It("wraps a write failure", func() { + mockStorage.WriteGoalReturns(liberrors.New(ctx, "boom")) + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("write goal")) + }) +}) diff --git a/pkg/ops/topic_approve.go b/pkg/ops/topic_approve.go new file mode 100644 index 00000000..2cdc838e --- /dev/null +++ b/pkg/ops/topic_approve.go @@ -0,0 +1,130 @@ +// 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. + +//nolint:dupl // Structurally parallel to the sibling goal's approve; frozen field names prevent dedup +package ops + +import ( + "context" + + "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/storage" +) + +//counterfeiter:generate -o ../../mocks/topic-approve-operation.go --fake-name TopicApproveOperation . TopicApproveOperation +type TopicApproveOperation interface { + // Execute approves the named topic: it moves a topic waiting in the approval + // inbox (phase todo) to phase planning and records, in the same write, who + // approved it and the instant of the transition. A topic that is not waiting + // in the inbox, or that already carries an approval record, is refused with + // nothing written. + // + // Topics are not required to have an owner, and this verb does not assign + // one. + Execute( + ctx context.Context, + vaultPath string, + topicName string, + vaultName string, + approvedBy string, + ) (MutationResult, error) +} + +// NewTopicApproveOperation creates a new topic approve operation. +func NewTopicApproveOperation( + topicStorage storage.TopicStorage, + currentDateTime libtime.CurrentDateTime, +) TopicApproveOperation { + return &topicApproveOperation{ + topicStorage: topicStorage, + currentDateTime: currentDateTime, + } +} + +type topicApproveOperation struct { + topicStorage storage.TopicStorage + currentDateTime libtime.CurrentDateTime +} + +// Execute approves a topic waiting in the approval inbox. +func (o *topicApproveOperation) Execute( + ctx context.Context, + vaultPath string, + topicName string, + vaultName string, + approvedBy string, +) (MutationResult, error) { + // Refuse an empty approver before reading the topic: a present-but-blank + // approved_by is worse than no record at all, and nothing is written here. + if approvedBy == "" { + err := errors.Errorf( + ctx, + "approved_by must not be empty: refusing to record a blank approver on topic %q", + topicName, + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + topic, err := o.topicStorage.FindTopicByName(ctx, vaultPath, topicName) + if err != nil { + return MutationResult{ + Success: false, + Error: err.Error(), + }, errors.Wrap(ctx, err, "find topic") + } + + // Read and validate the clock before any guard: a zero instant would be + // recorded as a placeholder rather than a measurement, so it is refused. + now := o.currentDateTime.Now() + if now.Time().IsZero() { + err := errors.Errorf( + ctx, + "approved_at is not a valid instant: refusing to record a zero timestamp on topic %q", + topicName, + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + // The inbox guard: only a topic at phase todo is waiting for approval. The + // message names both the current phase and the expected one so the operator + // can see what was found and what was required. + if phase := topic.Phase(); phase == nil || *phase != domain.TopicPhaseTodo { + current := "(none)" + if phase != nil { + current = string(*phase) + } + err := errors.Errorf( + ctx, + "refusing to approve %q: topic is at phase %q, not %q; only a topic in the approval inbox (phase %q) can be approved", + topicName, current, string(domain.TopicPhaseTodo), string(domain.TopicPhaseTodo), + ) + return MutationResult{Success: false, Error: err.Error()}, err + } + + // The existing-record guard: a todo row carrying an approval record is not + // awaiting approval, and re-approving must not overwrite who approved it first. + if err := refuseExistingEntityApprovalRecord(ctx, "topic", topicName, topic.Get); err != nil { + return MutationResult{Success: false, Error: err.Error()}, err + } + + // All three keys are composed onto the existing map, then handed to storage + // in exactly one write — so a row can never end up planned without an + // approval record beside it. Every other key, known or unknown, survives + // because the map itself is preserved. + topic.SetPhase(domain.TopicPhasePlanning.Ptr()) + topic.Set("approved_by", approvedBy) + topic.Set("approved_at", now.Time()) + + if err := o.topicStorage.WriteTopic(ctx, topic); err != nil { + return MutationResult{ + Success: false, + Error: err.Error(), + }, errors.Wrap(ctx, err, "write topic") + } + + return MutationResult{Success: true, Name: topic.Name, Vault: vaultName}, nil +} diff --git a/pkg/ops/topic_approve_test.go b/pkg/ops/topic_approve_test.go new file mode 100644 index 00000000..39598782 --- /dev/null +++ b/pkg/ops/topic_approve_test.go @@ -0,0 +1,204 @@ +// 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" + "time" + + liberrors "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + libtimetest "github.com/bborbe/time/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "gopkg.in/yaml.v3" + + "github.com/bborbe/vault-cli/mocks" + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/ops" +) + +var _ = Describe("TopicApproveOperation", func() { + var ( + ctx context.Context + mockStorage *mocks.Storage + pinned libtime.CurrentDateTime + approveOp ops.TopicApproveOperation + ) + + // seedTopic replaces the topic the storage returns, so a spec can control the + // phase and the pre-existing frontmatter the approve guards inspect. + seedTopic := func(fields map[string]any) { + mockStorage.FindTopicByNameReturns( + domain.NewTopic(fields, domain.FileMetadata{Name: "Alpha"}, domain.Content("")), + nil, + ) + } + + BeforeEach(func() { + ctx = context.Background() + mockStorage = &mocks.Storage{} + mockStorage.WriteTopicReturns(nil) + + pinned = libtime.NewCurrentDateTime() + pinned.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z")) + approveOp = ops.NewTopicApproveOperation(mockStorage, pinned) + + seedTopic(map[string]any{"status": "next", "phase": "todo"}) + }) + + It("writes all three approval keys in exactly one write", func() { + result, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + Expect(result.Success).To(BeTrue()) + Expect(result.Name).To(Equal("Alpha")) + Expect(result.Vault).To(Equal("vault-a")) + + Expect(mockStorage.WriteTopicCallCount()).To(Equal(1)) + _, written := mockStorage.WriteTopicArgsForCall(0) + Expect(written.Phase()).To(Equal(domain.TopicPhasePlanning.Ptr())) + Expect(written.GetString("approved_by")).To(Equal("operator")) + Expect(written.Get("approved_at")).NotTo(BeNil()) + }) + + It("records the injected instant as a bare time.Time", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteTopicArgsForCall(0) + raw, ok := written.Get("approved_at").(time.Time) + Expect(ok).To(BeTrue()) + Expect(raw.Equal(pinned.Now().Time())).To(BeTrue()) + }) + + It("serializes approved_at unquoted and round-trips it as a time.Time", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteTopicArgsForCall(0) + out, marshalErr := yaml.Marshal(written.RawMap()) + Expect(marshalErr).NotTo(HaveOccurred()) + Expect(string(out)).To(ContainSubstring("approved_at: 2026-03-03T12:00:00Z")) + Expect(string(out)).NotTo(ContainSubstring(`approved_at: "2026-03-03T12:00:00Z"`)) + + var roundTripped map[string]any + Expect(yaml.Unmarshal(out, &roundTripped)).To(Succeed()) + rt, ok := roundTripped["approved_at"].(time.Time) + Expect(ok).To(BeTrue()) + Expect(rt.Equal(pinned.Now().Time())).To(BeTrue()) + }) + + It("writes the approver named on the call", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "Manager Layer") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteTopicArgsForCall(0) + Expect(written.GetString("approved_by")).To(Equal("Manager Layer")) + }) + + It("preserves every other frontmatter key", func() { + seedTopic(map[string]any{ + "status": "next", + "phase": "todo", + "page_type": "topic", + "tags": []string{"work", "important"}, + "defer_date": "2026-04-01", + "custom_key": "keep", + }) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(BeNil()) + + _, written := mockStorage.WriteTopicArgsForCall(0) + Expect(written.GetField("status")).To(Equal("next")) + Expect(written.GetField("page_type")).To(Equal("topic")) + Expect(written.Tags()).To(Equal([]string{"work", "important"})) + Expect(written.GetField("defer_date")).To(Equal("2026-04-01")) + Expect(written.Get("custom_key")).To(Equal("keep")) + }) + + It("refuses an empty approver before reading the topic", func() { + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("approved_by")) + Expect(mockStorage.FindTopicByNameCallCount()).To(Equal(0)) + Expect(mockStorage.WriteTopicCallCount()).To(Equal(0)) + }) + + It("refuses a zero clock with zero writes", func() { + zero := libtime.NewCurrentDateTime() + zero.SetNow(libtime.DateTime(time.Time{})) + zeroOp := ops.NewTopicApproveOperation(mockStorage, zero) + + _, execErr := zeroOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("approved_at")) + Expect(mockStorage.WriteTopicCallCount()).To(Equal(0)) + }) + + DescribeTable("refuses a topic that is not waiting in the inbox", + func(fields map[string]any, expectedPhase string) { + seedTopic(fields) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring(expectedPhase)) + Expect(execErr.Error()).To(ContainSubstring("todo")) + Expect(mockStorage.WriteTopicCallCount()).To(Equal(0)) + }, + Entry("planning", map[string]any{"status": "next", "phase": "planning"}, "planning"), + Entry("execution", map[string]any{"status": "next", "phase": "execution"}, "execution"), + Entry("done", map[string]any{"status": "next", "phase": "done"}, "done"), + Entry("no phase key at all", map[string]any{"status": "next"}, "(none)"), + ) + + DescribeTable("refuses a todo topic that already carries an approval record", + func(fields map[string]any, expectedKeys string) { + seedTopic(fields) + + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring(expectedKeys)) + Expect(execErr.Error()).To(ContainSubstring("re-approving")) + Expect(mockStorage.WriteTopicCallCount()).To(Equal(0)) + }, + Entry("only approved_by", + map[string]any{"status": "next", "phase": "todo", "approved_by": "someone"}, + "approved_by", + ), + Entry("only approved_at", + map[string]any{ + "status": "next", + "phase": "todo", + "approved_at": "2026-01-01T00:00:00Z", + }, + "approved_at", + ), + Entry("both keys", + map[string]any{ + "status": "next", + "phase": "todo", + "approved_by": "someone", + "approved_at": "2026-01-01T00:00:00Z", + }, + "approved_by, approved_at", + ), + ) + + It("wraps a find failure", func() { + mockStorage.FindTopicByNameReturns(nil, liberrors.New(ctx, "boom")) + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("find topic")) + Expect(mockStorage.WriteTopicCallCount()).To(Equal(0)) + }) + + It("wraps a write failure", func() { + mockStorage.WriteTopicReturns(liberrors.New(ctx, "boom")) + _, execErr := approveOp.Execute(ctx, "/vault", "Alpha", "vault-a", "operator") + Expect(execErr).To(HaveOccurred()) + Expect(execErr.Error()).To(ContainSubstring("write topic")) + }) +}) diff --git a/prompts/completed/261-spec-061-set-path-guard.md b/prompts/completed/261-spec-061-set-path-guard.md new file mode 100644 index 00000000..fa3b6438 --- /dev/null +++ b/prompts/completed/261-spec-061-set-path-guard.md @@ -0,0 +1,168 @@ +--- +status: completed +spec: [061-goal-topic-approve] +summary: Added goal/topic todo-to-planning set refusals with unit and binary-level coverage +execution_id: vault-cli-goal-topic-approve-exec-261-spec-061-set-path-guard +dark-factory-version: v0.198.7 +created: "2026-10-20T10:03:18Z" +queued: "2026-10-20T10:16:58Z" +started: "2026-10-20T10:17:58Z" +completed: "2026-10-20T10:20:59Z" +--- + +# Refuse the unrecorded exit from the goal and topic approval inbox (spec 061, prompt 1 of 5) + + +- A goal waiting in the approval inbox can no longer be moved to planning with the generic field-set command. +- The same refusal applies to a topic. +- The refusal names the row, the phase it is in, the phase it would need, and the command to run instead. +- A refused set leaves the file on disk byte-identical. +- Every other set keeps its current behaviour: another target phase, a non-phase key, a row already past the inbox, and a row with no phase at all. +- The task-side guard and the task approval flow are untouched. +- Unit tests pin the refusal and its narrowness for both entity types. +- Binary-level tests prove the same two things against a real vault and a real binary. + + + +Close the generic-set bypass for goals and topics so `phase: todo → planning` can only happen through the approval verb that prompt 2 adds. Mirror the task guard exactly: refuse one (key, value, current-phase) triple, refuse nothing else, and write nothing on the refusal path. + + + +Read `CLAUDE.md` for project conventions. + +Read fully, in this order: +- `docs/task-writing.md` § Phase transitions — the approval contract being mirrored. Read it rather than re-deriving the rules from code. +- `pkg/ops/frontmatter.go` — `todoPlanningSetRefusal` and its doc comment, plus where `frontmatterSetOperation.Execute` calls it. This is the exemplar: same shape, same narrowness, same pre-mutation placement. +- `pkg/ops/frontmatter_entity.go` — `goalSetOperation.Execute` and `topicSetOperation.Execute`. These are the two call sites you change. +- `pkg/domain/goal_frontmatter.go` — `Phase() *GoalPhase` (returns nil when the key is absent) and `setPhaseField`. +- `pkg/domain/topic_frontmatter.go` — `Phase() *TopicPhase` and `setPhaseField`. +- `pkg/domain/goal_phase.go` and `pkg/domain/topic_phase.go` — `GoalPhasePlanning`, `GoalPhaseTodo`, `TopicPhasePlanning`, `TopicPhaseTodo`. Goal and topic phases have no aliases and no normalizer; the canonical strings are the constants. +- `pkg/ops/frontmatter_entity_test.go` — the existing `Describe("NewGoalSetOperation", …)` and `Describe("NewTopicSetOperation", …)` blocks. Add your unit cases inside them. +- `integration/cli_test.go` — `createTempVaultWithGoals(tasks, goals)`, `createTempVaultWithTopics(topicsDir)`, and one existing topic spec that creates `/Topics/` itself and writes a page into it. Note that the storage writer re-serializes the whole frontmatter in alphabetical key order, so author every fixture's keys alphabetically. + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Errorf` / `errors.Wrap` from `github.com/bborbe/errors`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega, and `gexec` session assertions for the binary-level specs. + + + +1. **Add the two guard functions** in `pkg/ops/frontmatter_entity.go`, beside the set operations they guard: + + ```go + func goalTodoPlanningSetRefusal( + ctx context.Context, + goal *domain.Goal, + goalName, key, value string, + ) error + + func topicTodoPlanningSetRefusal( + ctx context.Context, + topic *domain.Topic, + topicName, key, value string, + ) error + ``` + + Both implement exactly this decision sequence, returning `nil` at the first non-match: + - `key != "phase"` → `nil`. + - The target value is not the canonical planning phase → `nil`. Compare with `domain.GoalPhase(value) != domain.GoalPhasePlanning` (topics: `domain.TopicPhase(value) != domain.TopicPhasePlanning`). **Do not call `Validate`** — an unknown phase value must keep its existing rejection from `setPhaseField`, which is a different error with different wording (spec Non-goal). + - `goal.Phase() == nil` → `nil`. A row with no `phase` key is exempt from the gate by operator decision; do not treat an absent phase as `todo`. + - `*goal.Phase() != domain.GoalPhaseTodo` → `nil`. + - Otherwise return a refusal from `github.com/bborbe/errors` whose message names the row, the phase found, the phase required, and the command to run instead. Use this wording as the model (the goal arm; the topic arm is identical with `topic` substituted): + + ```go + return errors.Errorf(ctx, + "refusing to set phase %q on goal %q: the goal is at phase %q, the operator's approval inbox. Leaving it is the operator's approval and is performed by `vault-cli goal approve %q`, which records approved_by and approved_at in the same write", + domain.GoalPhasePlanning, goalName, string(domain.GoalPhaseTodo), goalName, + ) + ``` + + The literal `vault-cli goal approve` / `vault-cli topic approve` must appear in the message — acceptance criterion 3 asserts it on stderr. + +2. **Call each guard before any mutation.** + - In `goalSetOperation.Execute`: immediately after `FindGoalByName` succeeds and after the existing `blockedBySetRefusal` call, **before** `writeGoalCloseOutFieldsIfCloseOut` and before `goal.SetField`. Return the error unchanged (no extra wrapping — the message must stay readable and must still contain the approve verb). + - In `topicSetOperation.Execute`: immediately after `FindTopicByName` succeeds, before `topic.SetField`. + - Nothing below the guard runs on the refusal path, so the file on disk is byte-identical. Do not add a `--force` bypass and do not consult `reason`, `gateSuccessor`, or status. + +3. **Unit tests** in `pkg/ops/frontmatter_entity_test.go`, inside the existing set-operation Describes. Each case asserts both the error and `WriteGoalCallCount()` / `WriteTopicCallCount()`. + + Goal cases: + - goal at `phase: todo`, `set phase planning` → error matching `ContainSubstring("goal approve")`, `WriteGoalCallCount() == 0`, and the in-memory goal's `Phase()` is still `todo`. + - goal at `phase: todo`, `set phase execution` → no error, one write, written goal's `Phase()` is `execution`. This is the narrowness half of acceptance criterion 3. + - goal at `phase: todo`, `set priority 2` → no error, one write, written goal's `Priority()` is `2`. + - goal with **no** `phase` key, `set phase planning` → no error. This pins the deliberate exemption (spec Non-goals: the accepted hole) so a later reader does not "fix" it into a refusal. + - goal at `phase: planning`, `set phase planning` → no error (only `todo → planning` is guarded). + + Topic cases (same five, with `topic` and `ContainSubstring("topic approve")`). A topic has no `Priority()` accessor — for the non-phase case assert the written topic's `GetField("priority")` is `"2"`. + +4. **Binary-level specs** in `integration/cli_test.go`. Add one new `Describe("goal and topic set phase guard", …)` inside the suite's outer `Describe`, beside the other mutation-verb blocks. Declare its own `runEntityCommand` / `sha256OfFile` / `readFile` helpers locally — the sibling blocks' helpers are closure-scoped and not visible. + + Fixtures: + - goal: `createTempVaultWithGoals(nil, map[string]string{"Alpha": ""})`, with `phase: todo` and `status: next`. + - topic: `createTempVaultWithTopics("Topics")`, then `os.MkdirAll(filepath.Join(vaultPath, "Topics"), 0755)` and write `Alpha.md` with the same shape. + - Author every fixture's keys alphabetically (`page_type`, `phase`, `priority`, `status`). + + Cover: + - goal at `phase: todo`: `goal set Alpha phase planning` exits non-zero, stderr contains `vault-cli goal approve`, and the file's sha256 taken immediately before and after that one invocation is unchanged. + - topic at `phase: todo`: `topic set Alpha phase planning` exits non-zero, stderr contains `vault-cli topic approve`, sha256 unchanged. + - goal at `phase: todo`: `goal set Alpha phase execution` exits 0 and `goal get Alpha phase` prints `execution`. + - goal at `phase: todo`: `goal set Alpha priority 2` exits 0 and `goal get Alpha priority` prints `2`. + + Use `Eventually(session).Should(gexec.Exit(1))` for the refusals (the neighbouring mutation specs pin exit 1) and `gexec.Exit(0)` for the allowed cases. + +5. **Self-check before finishing.** Re-run the `` block and confirm every row; walk acceptance criterion 3 of spec 061 (both halves — the refusal and the narrowness) against the change. + + + +- The approval contract being mirrored is `docs/task-writing.md` § Phase transitions — read it rather than re-deriving the rules. +- The guard mirrors `todoPlanningSetRefusal` in `pkg/ops/frontmatter.go`. Do **not** change that function, `checkPhaseRegression`, `frontmatterSetOperation`, or `pkg/ops/task_approve.go`. +- Do not change the frontmatter serializers (`serializeMapAsFrontmatter` and friends) or the `setPhaseField` validators. +- The guard is deliberately one (field, value, current-phase) triple: `phase`, `planning`, current `todo`. A missing phase, an unknown phase, every non-phase key, and every other target phase keep their existing behaviour. +- An accepted hole is carried forward from the task contract: a row with **no** `phase` key can still reach planning via `set`. Do not close it — closing it would treat an absent phase as `todo`, which contradicts the operator's decision that an absent phase means exempt. +- No override exists and none is added: `goal set` / `topic set` carry no `--force` flag, and this prompt does not add one. +- Reuse the existing storage interfaces and find-by-name methods; add no new storage method. +- No backfilling of `phase` on existing goals or topics. +- `docs/` and `CHANGELOG.md` are out of scope — prompt 5 owns them. Do not add the `## Unreleased` bullet here. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. The integration suite's `topic defer writes defer_date for a relative and an absolute date` spec is a known pre-existing failure for the hours when the local date leads the UTC date; do not "fix" it and do not let it block this prompt. + + + +EVIDENCE GREPS — run each and confirm the count. Absence assertions use `! grep -q` because `grep -c` exits 1 when the count is 0. + +``` +grep -c 'func goalTodoPlanningSetRefusal' pkg/ops/frontmatter_entity.go # 1 +grep -c 'func topicTodoPlanningSetRefusal' pkg/ops/frontmatter_entity.go # 1 +grep -c 'vault-cli goal approve' pkg/ops/frontmatter_entity.go # >= 1 +grep -c 'vault-cli topic approve' pkg/ops/frontmatter_entity.go # >= 1 +grep -c 'goal and topic set phase guard' integration/cli_test.go # 1 +grep -c 'vault-cli goal approve' integration/cli_test.go # >= 1 +grep -c 'vault-cli topic approve' integration/cli_test.go # >= 1 +grep -c 'Execute(ctx context.Context, vaultPath, entityName, key, value, reason, gateSuccessor string) error' pkg/ops/frontmatter_entity.go # >= 1 — the frozen set signature is untouched +``` + +FROZEN-NEIGHBOUR GUARD — the container has no usable `.git` (the daemon runs with `hideGit=true`), so this cannot be checked with git. The `` forbid touching these files, and `make precommit` catches a broken neighbour. If a hard guard is wanted, record `shasum -a 256` of the four files at authoring time and compare: + +``` +sha256sum pkg/ops/frontmatter.go pkg/ops/task_approve.go pkg/domain/goal_frontmatter.go pkg/domain/topic_frontmatter.go > /tmp/frozen.sha256 # BEFORE editing +sha256sum -c /tmp/frozen.sha256 # after — every row must print OK +``` + +TESTS: + +``` +go build ./... +go test ./pkg/ops/... -ginkgo.focus="SetOperation" -ginkgo.v +go test ./integration/... -ginkgo.focus="set phase guard" -ginkgo.v +make precommit +``` + +⚠️ `-ginkgo.focus` with zero matching specs exits 0 while running nothing. After each focus run, confirm the new spec names appear in the output; a log without them is a failure, not a pass. + +SYNTAX: + +``` +gofmt -e -l pkg/ops/frontmatter_entity.go pkg/ops/frontmatter_entity_test.go integration/cli_test.go +``` + +`make precommit` must exit 0 apart from the named pre-existing `topic defer` integration failure. `make precommit` runs `generate` (which re-creates `mocks/`) and then `addlicense` (which restores the copyright header on every `.go` file, including `mocks/mocks.go`). Do not use git to restore anything — `.git` is masked in the container. + diff --git a/prompts/completed/262-spec-061-goal-approve.md b/prompts/completed/262-spec-061-goal-approve.md new file mode 100644 index 00000000..7f52eb59 --- /dev/null +++ b/prompts/completed/262-spec-061-goal-approve.md @@ -0,0 +1,175 @@ +--- +status: completed +spec: [061-goal-topic-approve] +summary: Added GoalApproveOperation that moves a phase:todo goal to planning while recording approved_by and approved_at in one write, with a shared approval-record guard and Ginkgo coverage of every fact and refusal +execution_id: vault-cli-goal-topic-approve-exec-262-spec-061-goal-approve +dark-factory-version: v0.198.7 +created: "2026-10-20T10:03:18Z" +queued: "2026-10-20T10:16:58Z" +started: "2026-10-20T10:21:00Z" +completed: "2026-10-20T10:23:46Z" +--- + +# Add the goal approve operation that records phase, approver and instant (spec 061, prompt 2 of 5) + + +- A goal waiting in the approval inbox can be approved in one operation. +- Approval writes the planning phase, who approved it, and when, in a single write. +- A goal that is not in the inbox is refused, and nothing is written. +- A goal that already carries an approval record is refused, and nothing is written. +- An empty approver is refused before the goal is even read. +- A zero clock reading is refused rather than recorded as a placeholder. +- Every unrelated frontmatter key on the goal survives the write. +- The task approval flow is untouched; this is a new sibling operation. +- Unit tests pin every recorded fact and every refusal. + + + +Give goals the same recorded approval transition tasks already have: one operation that moves a `phase: todo` goal to `phase: planning` and records `approved_by` and `approved_at` in the same write, refusing every state that is not the approval inbox. This is the writer the manager-start gate reads. + + + +Read `CLAUDE.md` for project conventions. + +Read fully, in this order: +- `docs/task-writing.md` § Phase transitions — the approval contract being mirrored. Read it rather than re-deriving the rules from `pkg/ops/task_approve.go`. +- `pkg/ops/task_approve.go` — the exemplar: `TaskApproveOperation`, its constructor, and its `Execute` sequence (empty approver → find → clock → inbox guard → existing-record guard → one composed write). Note what this prompt deliberately omits: the task variant's assignee / current-user owner resolution. +- `pkg/ops/task_approve_test.go` — the test shape: `mocks.Storage`, a pinned `libtime.CurrentDateTime` via `libtimetest.ParseDateTime`, and the YAML round-trip spec for `approved_at`. +- `pkg/domain/goal.go` — `NewGoal(data map[string]any, meta FileMetadata, content Content) *Goal`. +- `pkg/domain/goal_frontmatter.go` — `Phase() *GoalPhase` (nil when absent), `SetPhase(*GoalPhase)`, `Get(key string) any`, `Set(key string, value any)`. +- `pkg/domain/goal_phase.go` — `GoalPhaseTodo`, `GoalPhasePlanning`, and `Ptr()`. +- `pkg/domain/frontmatter_map.go` — `Get` returns the raw value and `nil` for an absent key; that is what the existing-record guard checks. +- `pkg/storage/storage.go` — `GoalStorage` with `WriteGoal(ctx, *domain.Goal) error` and `FindGoalByName(ctx, vaultPath, name string) (*domain.Goal, error)`. +- `pkg/ops/complete.go` — `MutationResult` (`Success`, `Name`, `Vault`, `Error`). + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-patterns.md` — interface → constructor → private struct. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrap` / `errors.Errorf` from `github.com/bborbe/errors`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega and Counterfeiter mocks. + + + +1. **Add the shared approval-record guard** in a new file `pkg/ops/approval_record.go`: + + ```go + // refuseExistingEntityApprovalRecord returns the refusal error for a goal or + // topic row that already carries an approved_by or an approved_at key, and + // nil otherwise. + func refuseExistingEntityApprovalRecord( + ctx context.Context, + kind string, + name string, + get func(string) any, + ) error + ``` + + Behaviour: check `get("approved_by")` and `get("approved_at")` independently; collect the names of the keys that are present (`[]string`, in that order); return `nil` when none is present; otherwise return `errors.Errorf` naming the row, the kind, and every present key, e.g. `refusing to approve %q: goal already carries an approval record (approved_by); re-approving is refused`. Both keys are checked independently so a row carrying either one is refused. Do **not** call `task_approve.go`'s `refuseExistingApprovalRecord` and do **not** modify it — it is typed on `*domain.Task` and frozen. + +2. **Add the operation** in a new file `pkg/ops/goal_approve.go`, following the interface → constructor → private struct shape: + + ```go + //counterfeiter:generate -o ../../mocks/goal-approve-operation.go --fake-name GoalApproveOperation . GoalApproveOperation + type GoalApproveOperation interface { + Execute( + ctx context.Context, + vaultPath string, + goalName string, + vaultName string, + approvedBy string, + ) (MutationResult, error) + } + + func NewGoalApproveOperation( + goalStorage storage.GoalStorage, + currentDateTime libtime.CurrentDateTime, + ) GoalApproveOperation + ``` + + `libtime` is `github.com/bborbe/time`. Write a doc comment on the interface stating the contract: only a goal at `phase: todo` can be approved; the approval writes `phase: planning`, `approved_by` and `approved_at` in one write; a goal that is not in the inbox, or that already carries an approval record, is refused with nothing written; goals are not required to have an owner and this verb does not assign one. + +3. **`Execute` runs exactly this sequence**, returning `MutationResult{Success: false, Error: err.Error()}, err` at each refusal: + 1. `approvedBy == ""` → refuse **before** reading the goal: `approved_by must not be empty: refusing to record a blank approver on goal %q`. + 2. `goal, err := o.goalStorage.FindGoalByName(ctx, vaultPath, goalName)`; on error return `errors.Wrap(ctx, err, "find goal")`. + 3. `now := o.currentDateTime.Now()`; if `now.Time().IsZero()` → refuse before any guard: `approved_at is not a valid instant: refusing to record a zero timestamp on goal %q`. + 4. Inbox guard: `phase := goal.Phase()`; if `phase == nil || *phase != domain.GoalPhaseTodo` → refuse. Render an absent phase as `(none)`, mirroring `task_approve.go`, and name both the phase found and the phase required: `refusing to approve %q: goal is at phase %q, not %q; only a goal in the approval inbox (phase %q) can be approved`. + 5. `refuseExistingEntityApprovalRecord(ctx, "goal", goalName, goal.Get)`. + 6. Compose onto the existing map, then write once: + ```go + goal.SetPhase(domain.GoalPhasePlanning.Ptr()) + goal.Set("approved_by", approvedBy) + goal.Set("approved_at", now.Time()) + ``` + `now.Time()` is a bare `time.Time` so yaml.v3 renders it unquoted, exactly as `task approve` writes it. Do not format it as a string — a string is quoted by the encoder and would diverge from the task record. + 7. `o.goalStorage.WriteGoal(ctx, goal)`; on error return `errors.Wrap(ctx, err, "write goal")`. Exactly one write per successful approval. + 8. Return `MutationResult{Success: true, Name: goal.Name, Vault: vaultName}, nil`. + + Every guard runs before step 6, so a refusal leaves the file byte-identical. + +4. **Unit tests** in a new file `pkg/ops/goal_approve_test.go`, package `ops_test`, using `mocks.Storage` and a pinned clock exactly as `task_approve_test.go` does. Cover: + - one write; the written goal's `Phase()` is `domain.GoalPhasePlanning.Ptr()`, `GetString("approved_by")` is `operator`, `Get("approved_at")` is non-nil; `result.Name` and `result.Vault` are the call's values. + - the recorded instant is the injected one and is a bare `time.Time` (type assertion). + - YAML boundary: `yaml.Marshal(written.RawMap())` contains `approved_at: ` unquoted (with the exemplar's pin, `approved_at: 2026-03-03T12:00:00Z`), and unmarshalling it back yields a `time.Time` equal to the injected instant. This is the serialization boundary the manager-start gate will read. + - the approver named on the call is recorded verbatim (`Manager Layer`). + - every other key survives: seed `priority`, `page_type`, `theme`, `assignee`, `custom_key` and assert each after the write. + - the inbox guard, table-driven over `planning`, `execution`, `done`, and an absent `phase`: each refused, the error names the found and required phase, and `WriteGoalCallCount() == 0`. + - the existing-record guard, three fixtures at `phase: todo`: only `approved_by`, only `approved_at`, and both. Each refused, the error names the present key(s), `WriteGoalCallCount() == 0`. + - an empty `approvedBy` is refused with `FindGoalByNameCallCount() == 0` — the refusal happens before the read. + - a zero clock (`zero := libtime.NewCurrentDateTime(); zero.SetNow(libtime.DateTime(time.Time{}))`) is refused with zero writes. + - a `FindGoalByName` error is wrapped with `find goal`; a `WriteGoal` error is wrapped with `write goal`. + +5. **Self-check before finishing.** Re-run the `` block and confirm every row; walk acceptance criteria 1, 2 and 4 of spec 061 against the change (the CLI-level halves are prompt 4's job). + + + +- The approval contract being mirrored is `docs/task-writing.md` § Phase transitions — read it rather than re-deriving the rules. +- Reuse `storage.GoalStorage` and `FindGoalByName`; do not add a new storage method. +- Do not change `pkg/ops/task_approve.go`, the task-side guards, or the frontmatter serializers. +- A refusal writes nothing: every guard runs before the first mutation, so the file on disk is byte-identical. +- No owner/assignee handling: goals are not required to have an assignee, and this verb does not add one (spec Non-goal). +- No `--force` and no override: re-approving an already-approved goal is refused. +- No backfilling of `phase` on existing goals. +- `pkg/cli/` is out of scope — prompt 4 wires the CLI. `docs/` and `CHANGELOG.md` are prompt 5's. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +EVIDENCE GREPS — run each and confirm the count. Absence assertions use `! grep -q` because `grep -c` exits 1 when the count is 0. + +``` +grep -c 'type GoalApproveOperation interface' pkg/ops/goal_approve.go # 1 +grep -c 'func NewGoalApproveOperation' pkg/ops/goal_approve.go # 1 +grep -c 'goalStorage.WriteGoal' pkg/ops/goal_approve.go # 1 — exactly one write path +grep -c 'func refuseExistingEntityApprovalRecord' pkg/ops/approval_record.go # 1 +grep -c 'refuseExistingEntityApprovalRecord(ctx' pkg/ops/goal_approve.go # 1 — the call site +grep -c 'counterfeiter:generate' pkg/ops/goal_approve.go # 1 +grep -c 'Describe("GoalApproveOperation"' pkg/ops/goal_approve_test.go # 1 +grep -c 'custom_key' pkg/ops/goal_approve_test.go # >= 1 — the key-preservation fixture +grep -c 'yaml.Marshal' pkg/ops/goal_approve_test.go # >= 1 — the serialization boundary +``` + +FROZEN-NEIGHBOUR GUARD — the container has no usable `.git` (the daemon runs with `hideGit=true`), so this cannot be checked with git. The `` forbid touching these files, and `make precommit` catches a broken neighbour. If a hard guard is wanted, record `shasum -a 256` of the three files at authoring time and compare: + +``` +sha256sum pkg/ops/task_approve.go pkg/ops/frontmatter.go pkg/ops/frontmatter_entity.go > /tmp/frozen.sha256 # BEFORE editing +sha256sum -c /tmp/frozen.sha256 # after — every row must print OK +``` + +TESTS: + +``` +go build ./... +go test ./pkg/ops/... -ginkgo.focus="GoalApproveOperation" -ginkgo.v +make precommit +``` + +⚠️ `-ginkgo.focus` with zero matching specs exits 0 while running nothing. After the focus run, confirm the new spec names appear in the output; a log without them is a failure, not a pass. + +SYNTAX: + +``` +gofmt -e -l pkg/ops/goal_approve.go pkg/ops/approval_record.go pkg/ops/goal_approve_test.go +``` + +`make precommit` runs `generate` (which regenerates `mocks/` from the counterfeiter directives) and then `addlicense` (which restores the copyright header on every `.go` file, including `mocks/mocks.go`). Confirm `mocks/goal-approve-operation.go` was generated. Do not use git to restore anything — `.git` is masked in the container. + diff --git a/prompts/completed/263-spec-061-topic-approve.md b/prompts/completed/263-spec-061-topic-approve.md new file mode 100644 index 00000000..498dc643 --- /dev/null +++ b/prompts/completed/263-spec-061-topic-approve.md @@ -0,0 +1,160 @@ +--- +status: completed +spec: [061-goal-topic-approve] +summary: Added TopicApproveOperation recording phase planning, approved_by and approved_at in one write, reusing the shared approval-record guard, with 16 focused unit tests; make precommit exited 0. +execution_id: vault-cli-goal-topic-approve-exec-263-spec-061-topic-approve +dark-factory-version: v0.198.7 +created: "2026-10-20T10:03:18Z" +queued: "2026-10-20T10:16:58Z" +started: "2026-10-20T10:23:48Z" +completed: "2026-10-20T10:27:12Z" +--- + +# Add the topic approve operation that records phase, approver and instant (spec 061, prompt 3 of 5) + + +- A topic waiting in the approval inbox can be approved in one operation. +- Approval writes the planning phase, who approved it, and when, in a single write. +- A topic that is not in the inbox is refused, and nothing is written. +- A topic that already carries an approval record is refused, and nothing is written. +- An empty approver is refused before the topic is even read. +- A zero clock reading is refused rather than recorded as a placeholder. +- Every unrelated frontmatter key on the topic survives the write. +- The goal and task approval flows are untouched; this is a sibling operation that reuses their shared record guard. +- Unit tests pin every recorded fact and every refusal. + + + +Give topics the same recorded approval transition goals get in prompt 2: one operation that moves a `phase: todo` topic to `phase: planning` and records `approved_by` and `approved_at` in the same write, refusing every state that is not the approval inbox. + + + +Read `CLAUDE.md` for project conventions. + +Read fully, in this order: +- `docs/task-writing.md` § Phase transitions — the approval contract being mirrored. +- `pkg/ops/goal_approve.go` and `pkg/ops/approval_record.go` — the sibling shipped by prompt 2. This prompt is the topic twin: same sequence, same messages with `topic` substituted, and it **reuses** `refuseExistingEntityApprovalRecord` rather than defining a second guard. If `refuseExistingEntityApprovalRecord` is missing, stop and report that prompt 2 has not landed. +- `pkg/ops/task_approve.go` — the original exemplar for the sequence (empty approver → find → clock → inbox guard → existing-record guard → one composed write). +- `pkg/ops/topic_complete.go` — the topic-operation shape in this package (`TopicStorage`, `MutationResult`, `errors.Wrap(ctx, err, "find topic")` / `"write topic"`). +- `pkg/domain/topic.go` — `NewTopic(data map[string]any, meta FileMetadata, content Content) *Topic`. +- `pkg/domain/topic_frontmatter.go` — `Phase() *TopicPhase` (nil when absent), `SetPhase(*TopicPhase)`, `Get(key string) any`, `Set(key string, value any)`. +- `pkg/domain/topic_phase.go` — `TopicPhaseTodo`, `TopicPhasePlanning`, and `Ptr()`. +- `pkg/storage/storage.go` — `TopicStorage` with `WriteTopic(ctx, *domain.Topic) error` and `FindTopicByName(ctx, vaultPath, name string) (*domain.Topic, error)`. +- `pkg/ops/complete.go` — `MutationResult` (`Success`, `Name`, `Vault`, `Error`). + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-patterns.md` — interface → constructor → private struct. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrap` / `errors.Errorf` from `github.com/bborbe/errors`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega and Counterfeiter mocks. + + + +1. **Add the operation** in a new file `pkg/ops/topic_approve.go`, following the interface → constructor → private struct shape: + + ```go + //counterfeiter:generate -o ../../mocks/topic-approve-operation.go --fake-name TopicApproveOperation . TopicApproveOperation + type TopicApproveOperation interface { + Execute( + ctx context.Context, + vaultPath string, + topicName string, + vaultName string, + approvedBy string, + ) (MutationResult, error) + } + + func NewTopicApproveOperation( + topicStorage storage.TopicStorage, + currentDateTime libtime.CurrentDateTime, + ) TopicApproveOperation + ``` + + `libtime` is `github.com/bborbe/time`. Write a doc comment on the interface stating the contract: only a topic at `phase: todo` can be approved; the approval writes `phase: planning`, `approved_by` and `approved_at` in one write; a topic that is not in the inbox, or that already carries an approval record, is refused with nothing written; topics are not required to have an owner and this verb does not assign one. + +2. **`Execute` runs exactly this sequence**, returning `MutationResult{Success: false, Error: err.Error()}, err` at each refusal: + 1. `approvedBy == ""` → refuse **before** reading the topic: `approved_by must not be empty: refusing to record a blank approver on topic %q`. + 2. `topic, err := o.topicStorage.FindTopicByName(ctx, vaultPath, topicName)`; on error return `errors.Wrap(ctx, err, "find topic")`. + 3. `now := o.currentDateTime.Now()`; if `now.Time().IsZero()` → refuse before any guard: `approved_at is not a valid instant: refusing to record a zero timestamp on topic %q`. + 4. Inbox guard: `phase := topic.Phase()`; if `phase == nil || *phase != domain.TopicPhaseTodo` → refuse. Render an absent phase as `(none)`, mirroring `task_approve.go`, and name both the phase found and the phase required: `refusing to approve %q: topic is at phase %q, not %q; only a topic in the approval inbox (phase %q) can be approved`. + 5. `refuseExistingEntityApprovalRecord(ctx, "topic", topicName, topic.Get)` — the helper from `pkg/ops/approval_record.go` (prompt 2). Do not duplicate it. + 6. Compose onto the existing map, then write once: + ```go + topic.SetPhase(domain.TopicPhasePlanning.Ptr()) + topic.Set("approved_by", approvedBy) + topic.Set("approved_at", now.Time()) + ``` + `now.Time()` is a bare `time.Time` so yaml.v3 renders it unquoted, exactly as `task approve` writes it. Do not format it as a string — a string is quoted by the encoder and would diverge from the task record. + 7. `o.topicStorage.WriteTopic(ctx, topic)`; on error return `errors.Wrap(ctx, err, "write topic")`. Exactly one write per successful approval. + 8. Return `MutationResult{Success: true, Name: topic.Name, Vault: vaultName}, nil`. + + Every guard runs before step 6, so a refusal leaves the file byte-identical. + +3. **Unit tests** in a new file `pkg/ops/topic_approve_test.go`, package `ops_test`, using `mocks.Storage` and a pinned clock exactly as `task_approve_test.go` does. Cover: + - one write; the written topic's `Phase()` is `domain.TopicPhasePlanning.Ptr()`, `GetString("approved_by")` is `operator`, `Get("approved_at")` is non-nil; `result.Name` and `result.Vault` are the call's values. + - the recorded instant is the injected one and is a bare `time.Time` (type assertion). + - YAML boundary: `yaml.Marshal(written.RawMap())` contains `approved_at: ` unquoted (with the exemplar's pin, `approved_at: 2026-03-03T12:00:00Z`), and unmarshalling it back yields a `time.Time` equal to the injected instant. This is the serialization boundary the manager-start gate will read. + - the approver named on the call is recorded verbatim (`Manager Layer`). + - every other key survives: seed `status`, `page_type`, `tags`, `defer_date`, `custom_key` and assert each after the write. + - the inbox guard, table-driven over `planning`, `execution`, `done`, and an absent `phase`: each refused, the error names the found and required phase, and `WriteTopicCallCount() == 0`. + - the existing-record guard, three fixtures at `phase: todo`: only `approved_by`, only `approved_at`, and both. Each refused, the error names the present key(s), `WriteTopicCallCount() == 0`. + - an empty `approvedBy` is refused with `FindTopicByNameCallCount() == 0` — the refusal happens before the read. + - a zero clock (`zero := libtime.NewCurrentDateTime(); zero.SetNow(libtime.DateTime(time.Time{}))`) is refused with zero writes. + - a `FindTopicByName` error is wrapped with `find topic`; a `WriteTopic` error is wrapped with `write topic`. + +4. **Self-check before finishing.** Re-run the `` block and confirm every row; walk acceptance criteria 1, 2 and 4 of spec 061 against the change (the CLI-level halves are prompt 4's job). + + + +- The approval contract being mirrored is `docs/task-writing.md` § Phase transitions — read it rather than re-deriving the rules. +- Reuse `storage.TopicStorage` and `FindTopicByName`; do not add a new storage method. +- Reuse `refuseExistingEntityApprovalRecord` from `pkg/ops/approval_record.go`. Do not define a second copy and do not modify the goal operation shipped by prompt 2. +- Do not change `pkg/ops/task_approve.go`, the task-side guards, or the frontmatter serializers. +- A refusal writes nothing: every guard runs before the first mutation, so the file on disk is byte-identical. +- No owner/assignee handling: topics are not required to have an assignee, and this verb does not add one (spec Non-goal). +- No `--force` and no override: re-approving an already-approved topic is refused. +- No backfilling of `phase` on existing topics. +- `pkg/cli/` is out of scope — prompt 4 wires the CLI. `docs/` and `CHANGELOG.md` are prompt 5's. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +EVIDENCE GREPS — run each and confirm the count. Absence assertions use `! grep -q` because `grep -c` exits 1 when the count is 0. + +``` +grep -c 'type TopicApproveOperation interface' pkg/ops/topic_approve.go # 1 +grep -c 'func NewTopicApproveOperation' pkg/ops/topic_approve.go # 1 +grep -c 'topicStorage.WriteTopic' pkg/ops/topic_approve.go # 1 — exactly one write path +grep -c 'refuseExistingEntityApprovalRecord(ctx' pkg/ops/topic_approve.go # 1 — the reused helper's call site +! grep -q 'func refuseExistingEntityApprovalRecord' pkg/ops/topic_approve.go # the helper is not duplicated here +grep -c 'counterfeiter:generate' pkg/ops/topic_approve.go # 1 +grep -c 'Describe("TopicApproveOperation"' pkg/ops/topic_approve_test.go # 1 +grep -c 'custom_key' pkg/ops/topic_approve_test.go # >= 1 — the key-preservation fixture +grep -c 'yaml.Marshal' pkg/ops/topic_approve_test.go # >= 1 — the serialization boundary +``` + +FROZEN-NEIGHBOUR GUARD — the container has no usable `.git` (the daemon runs with `hideGit=true`), so this cannot be checked with git. The `` forbid touching these files, and `make precommit` catches a broken neighbour. If a hard guard is wanted, record `shasum -a 256` of the four files at authoring time and compare: + +``` +sha256sum pkg/ops/task_approve.go pkg/ops/frontmatter.go pkg/ops/goal_approve.go pkg/ops/approval_record.go > /tmp/frozen.sha256 # BEFORE editing +sha256sum -c /tmp/frozen.sha256 # after — every row must print OK +``` + +TESTS: + +``` +go build ./... +go test ./pkg/ops/... -ginkgo.focus="TopicApproveOperation" -ginkgo.v +make precommit +``` + +⚠️ `-ginkgo.focus` with zero matching specs exits 0 while running nothing. After the focus run, confirm the new spec names appear in the output; a log without them is a failure, not a pass. + +SYNTAX: + +``` +gofmt -e -l pkg/ops/topic_approve.go pkg/ops/topic_approve_test.go +``` + +`make precommit` runs `generate` (which regenerates `mocks/` from the counterfeiter directives) and then `addlicense` (which restores the copyright header on every `.go` file, including `mocks/mocks.go`). Confirm `mocks/topic-approve-operation.go` was generated. Do not use git to restore anything — `.git` is masked in the container. + diff --git a/prompts/completed/264-spec-061-approve-cli.md b/prompts/completed/264-spec-061-approve-cli.md new file mode 100644 index 00000000..925e1ff1 --- /dev/null +++ b/prompts/completed/264-spec-061-approve-cli.md @@ -0,0 +1,188 @@ +--- +status: completed +spec: [061-goal-topic-approve] +summary: Added `vault-cli goal approve` and `vault-cli topic approve` leaf commands with --by/JSON output and binary-level end-to-end coverage, plus changelog and command-registration updates +execution_id: vault-cli-goal-topic-approve-exec-264-spec-061-approve-cli +dark-factory-version: v0.198.7 +created: "2026-10-20T10:03:18Z" +queued: "2026-10-20T10:16:58Z" +started: "2026-10-20T10:27:13Z" +completed: "2026-10-20T10:31:14Z" +--- + +# Wire `vault-cli goal approve` and `vault-cli topic approve` and prove them end to end (spec 061, prompt 4 of 5) + + +- Operators get `vault-cli goal approve ` and `vault-cli topic approve `. +- The approver defaults to `operator` and can be named with `--by`; an explicitly empty `--by` is refused. +- Plain output names the row and the phase it landed in; `--output json` emits the name and the new phase. +- A refusal exits non-zero in both output modes, including JSON. +- The new verbs are registered in the command-registration table the project's Definition of Done requires. +- Binary-level tests prove the recorded transition, key preservation, and every refusal against a real vault and a real binary. + + + +Expose the two approval operations from prompts 2 and 3 as Cobra leaf commands, so the operator can perform the `todo → planning` transition from the command line, and prove the whole transition end to end. This closes acceptance criteria 1, 2 and 4 of spec 061 at the binary level. + + + +Read `CLAUDE.md` for project conventions. + +Read fully, in this order: +- `pkg/cli/cli.go` — `createTaskApproveCommand` (search for it). This is the exemplar: `getVaults` → `ops.NewVaultDispatcher().FirstSuccess` → build the operation per vault → plain/JSON output → the JSON error branch that prints the error object **and then returns the error**, so a refusal exits non-zero. Also read `createGoalCommands`, `createTopicCommands`, `createGoalCompleteCommand`, and `createTopicCompleteCommand` for where the new factories and registrations belong. +- `pkg/cli/output.go` — `OutputFormat.IsJSON()` and `PrintJSON`. +- `pkg/ops/vault_dispatcher.go` — `FirstSuccess` semantics: a single vault calls the callback directly and propagates its error; with several vaults only a not-found-class error lets the loop continue. A refusal stops the loop, which is the intent. +- `pkg/ops/goal_approve.go` and `pkg/ops/topic_approve.go` — the operations shipped by prompts 2 and 3. Read their `Execute` signatures before writing the call sites. +- `pkg/domain/goal_phase.go` and `pkg/domain/topic_phase.go` — `GoalPhasePlanning`, `TopicPhasePlanning`, so the output does not use bare `"planning"` literals. +- `integration/cli_test.go` — the existing `Describe("task approve", …)` block in full: its locally declared `runEntityCommand` / `sha256OfFile` / `readFile` / `valueOf` / `approvedKeys` helpers and its fixture constants. Also read `createTempVaultWithGoals` and `createTempVaultWithTopics`, and the `Describe("command registration", …)` table. +- `docs/dod.md` — "New CLI commands/subcommands have an entry in the integration test command registration table". + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-cli-guide.md` — Cobra leaf-command shape. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrap(ctx, err, "get vaults")` from `github.com/bborbe/errors`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega, `gexec`, `Eventually(session).Should(gexec.Exit(N))`. + + + +1. **Add `createGoalApproveCommand`** in `pkg/cli/cli.go`, immediately after `createGoalCompleteCommand`: + + ```go + func createGoalApproveCommand( + ctx context.Context, + configLoader *config.Loader, + vaultName *string, + outputFormat *string, + ) *cobra.Command + ``` + + - `Use: "approve "`, `Args: cobra.ExactArgs(1)`. + - One flag: `cmd.Flags().StringVar(&approvedBy, "by", "operator", "Who is approving; recorded as approved_by")`. The default **must** be `"operator"` in the flag, so an explicit `--by ""` is a changed flag carrying an empty value and reaches the operation's refusal; a `""` default would make the empty case indistinguishable from "flag not given". + - `RunE` mirrors `createTaskApproveCommand` minus the current-user lookup and the `--assignee` flag (spec Non-goal: no owner assignment): + ```go + goalName := args[0] + vaults, err := getVaults(ctx, configLoader, vaultName) + if err != nil { + return errors.Wrap(ctx, err, "get vaults") + } + currentDateTime := libtime.NewCurrentDateTime() + dispatcher := ops.NewVaultDispatcher() + err = dispatcher.FirstSuccess(ctx, vaults, func(vault *config.Vault) error { + storageConfig := storage.NewConfigFromVault(vault) + goalStore := storage.NewGoalStorage(storageConfig) + approveOp := ops.NewGoalApproveOperation(goalStore, currentDateTime) + result, err := approveOp.Execute(ctx, vault.Path, goalName, vault.Name, approvedBy) + if err != nil { + return err + } + if OutputFormat(*outputFormat).IsJSON() { + return PrintJSON(map[string]any{ + "success": true, + "name": result.Name, + "phase": string(domain.GoalPhasePlanning), + }) + } + fmt.Printf("✅ Goal approved: %s (phase %s)\n", result.Name, domain.GoalPhasePlanning) + return nil + }) + if err != nil { + if OutputFormat(*outputFormat).IsJSON() { + _ = PrintJSON(map[string]any{"success": false, "error": err.Error()}) + } + return err + } + return nil + ``` + - The plain success line must contain the goal name — acceptance criterion 1 asserts it on stdout. + +2. **Add `createTopicApproveCommand`** the same way, immediately after `createTopicCompleteCommand`: `Use: "approve "`, the same `--by` flag and default, `storage.NewTopicStorage(storageConfig)`, `ops.NewTopicApproveOperation`, `domain.TopicPhasePlanning`, and the plain line `✅ Topic approved: %s (phase %s)\n`. Add `//nolint:dupl,…` with the same linter list the neighbouring parallel commands carry if `make lint` demands it. + +3. **Register both commands** beside their siblings: + ```go + cmd.AddCommand(createGoalApproveCommand(ctx, configLoader, vaultName, outputFormat)) // in createGoalCommands + cmd.AddCommand(createTopicApproveCommand(ctx, configLoader, vaultName, outputFormat)) // in createTopicCommands + ``` + +4. **Add the binary-level specs** in `integration/cli_test.go`, immediately after the existing `Describe("task approve", …)` block: one `Describe("goal approve", …)` and one `Describe("topic approve", …)`. Declare their helpers locally (the sibling blocks' copies are closure-scoped). Author every fixture's keys alphabetically — the storage writer re-serializes the whole frontmatter in alphabetical key order, so a non-alphabetical fixture makes every byte assertion vacuous. + + Fixtures: goals via `createTempVaultWithGoals(nil, map[string]string{"Alpha": …})`; topics via `createTempVaultWithTopics("Topics")` plus `os.MkdirAll(filepath.Join(vaultPath, "Topics"), 0755)` and a written page. Name each `It` with its AC label, matching the file's style. + + ⚠️ **AC-rendering note — read before writing the `approved_at` assertion.** Acceptance criterion 1 says `vault-cli goal get approved_at` parses as RFC3339. It does not, and must not be made to: the operation writes a bare `time.Time` (spec Desired Behavior 3: "formatted the same way `task approve` writes it"), yaml.v3 renders that as an unquoted `approved_at: 2026-10-20T10:03:18Z`, and reading it back yields a `time.Time` whose `FrontmatterMap.GetString` rendering is Go's `%v` form (`2026-10-20 10:03:18 +0000 UTC`). Writing a formatted string instead would quote the line and diverge from the task record. So: run the RFC3339 parse **on the raw file line** (strip an optional surrounding pair of double quotes), exactly as the existing `task approve` spec does, and separately assert `goal get Alpha approved_at` exits 0 with a non-empty value. Do not change the write shape to satisfy the AC's literal rendering. + + Cover, for **both** entity types: + - **AC 1 — the recorded transition.** Fixture at `status: next`, `phase: todo`, no approval keys. `goal approve Alpha` exits 0. Read the file: `phase: planning`, `approved_by: operator`, and an `approved_at` line whose value parses as RFC3339. Capture `before := time.Now().UTC()` immediately before the invocation and assert `parsed.Sub(before) >= 0 && parsed.Sub(before) <= 60*time.Second`. Also assert `goal get Alpha phase` prints `planning`, `goal get Alpha approved_by` prints `operator`, and `goal get Alpha approved_at` prints a non-empty value. Assert stdout contains `Alpha`. + - **AC 1 — `--by`.** On a fresh `todo` fixture, `goal approve Alpha --by alice` exits 0 and the file's `approved_by` line reads `alice`. + - **AC 2 — empty approver.** On a fresh `todo` fixture, `goal approve Alpha --by ""` exits non-zero, stderr contains `approved_by`, and the file's sha256 is unchanged. + - **AC 4 — refusals, table-driven.** For each row, a fresh fixture, one invocation, non-zero exit, stderr contains the row name, and the sha256 taken immediately before and after that invocation is unchanged: + - `phase: planning`, `phase: execution`, `phase: done`, and no `phase` key at all — stderr names both the phase found and the phase required (`todo`); + - `phase: todo` carrying only `approved_by: someone`, only `approved_at: 2026-01-01T00:00:00Z`, and both — stderr names the present key; + - an unknown row name (`Missing`) — stderr contains `Missing`. + - **AC 4 — key preservation.** A `todo` fixture carrying `custom_key: keep`, `page_type: goal`, `priority: 1` (topics: `custom_key: keep`, `page_type: topic`). Keep every value in this fixture a scalar: the writer re-serializes the raw map with `yaml.Marshal`, which renders a flow list (`tags: [a]`) as a block list indented four spaces, so a list key would break the byte comparison for a reason unrelated to approval. After a successful approve, drop every line matching `^(phase|approved_by|approved_at):` from the pre and post content and assert the two slices are equal — whole-slice equality, not a count. Assert `custom_key` still reads `keep`. + - **JSON output.** `--output json` on a fresh `todo` fixture exits 0 and stdout parses into an object with `name == "Alpha"` and `phase == "planning"`; `--output json` on a `phase: planning` fixture exits non-zero. + +5. **Register both verbs** in the `Describe("command registration", …)` table, beside the other goal/topic entries: + ```go + Entry("goal approve", "goal", "approve"), + Entry("topic approve", "topic", "approve"), + ``` + +6. **Self-check before finishing.** Re-run the `` block and confirm every row; walk acceptance criteria 1, 2 and 4 of spec 061 against the change. + + + +- `task approve` is frozen: its command body, its flags, and its JSON-error branch are unchanged. Do not add `--assignee`, `--force`, or a current-user lookup to the new verbs (spec Non-goal: no owner assignment at approval time). +- The operations shipped by prompts 2 and 3 are called as-is. Do not add fields to `MutationResult` and do not change `pkg/ops/goal_approve.go`, `pkg/ops/topic_approve.go`, or `pkg/ops/approval_record.go`. +- No new direct `encoding/json` use for these commands — output goes through `PrintJSON`. +- `docs/` and `CHANGELOG.md` are out of scope — prompt 5 owns them. +- `commands/` docs are out of scope here; a `commands/` change needs a plugin version bump, which the releaser handles in prompt 5. Do not hand-bump the manifests. +- Fixtures must be authored in alphabetical key order (requirement 4). +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. The integration suite's `topic defer writes defer_date for a relative and an absolute date` spec is a known pre-existing failure for the hours when the local date leads the UTC date; do not "fix" it and do not let it block this prompt. + + + +EVIDENCE GREPS — run each and confirm the count. Absence assertions use `! grep -q` because `grep -c` exits 1 when the count is 0. + +``` +grep -c 'createGoalApproveCommand' pkg/cli/cli.go # >= 2 (factory definition + registration) +grep -c 'createTopicApproveCommand' pkg/cli/cli.go # >= 2 +grep -c 'NewGoalApproveOperation' pkg/cli/cli.go # 1 +grep -c 'NewTopicApproveOperation' pkg/cli/cli.go # 1 +grep -c 'approve ' pkg/cli/cli.go # 1 +grep -c 'approve ' pkg/cli/cli.go # 1 +grep -c 'StringVar(&approvedBy, "by"' pkg/cli/cli.go # 3 (task approve's existing one + one per new verb) +grep -c 'Goal approved' pkg/cli/cli.go # 1 +grep -c 'Topic approved' pkg/cli/cli.go # 1 +grep -c 'Describe("goal approve"' integration/cli_test.go # 1 +grep -c 'Describe("topic approve"' integration/cli_test.go # 1 +grep -c 'Entry("goal approve", "goal", "approve")' integration/cli_test.go # 1 +grep -c 'Entry("topic approve", "topic", "approve")' integration/cli_test.go # 1 +``` + +⚠️ Do not gate on `grep -cE 'AC[0-9]+: ' integration/cli_test.go` — the file already contains many AC labels, so that count proves nothing. The rows above are the non-vacuous ones. + +FROZEN-NEIGHBOUR GUARD — the container has no usable `.git` (the daemon runs with `hideGit=true`), so this cannot be checked with git. The `` forbid touching these files, and `make precommit` catches a broken neighbour. If a hard guard is wanted, record the `sha256sum` baseline of the three ops files at authoring time and compare; `pkg/cli/cli.go` is the fourth frozen file and is covered by the following `grep`: + +``` +sha256sum pkg/ops/goal_approve.go pkg/ops/topic_approve.go pkg/ops/approval_record.go > /tmp/frozen.sha256 # BEFORE editing +sha256sum -c /tmp/frozen.sha256 # after — every row must print OK +grep -c 'approve ' pkg/cli/cli.go # 1 — task approve's Use line is untouched +``` + +TESTS: + +``` +go build ./... +go test ./integration/... -ginkgo.focus="goal approve|topic approve" -ginkgo.v +make precommit +``` + +⚠️ `-ginkgo.focus` with zero matching specs exits 0 while running nothing. After the focus run, confirm the new spec names appear in the output; a log without them is a failure, not a pass. + +SYNTAX: + +``` +gofmt -e -l pkg/cli/cli.go integration/cli_test.go +``` + +`make precommit` must exit 0 apart from the named pre-existing `topic defer` integration failure. `make precommit` runs `ensure format generate test check addlicense`. If the known `topic defer` failure stops the chain at `test`, `addlicense` never runs — run `make addlicense` as the last action in that case. Do not use git to restore anything — `.git` is masked in the container. + diff --git a/prompts/completed/265-spec-061-docs-changelog.md b/prompts/completed/265-spec-061-docs-changelog.md new file mode 100644 index 00000000..ca9020fb --- /dev/null +++ b/prompts/completed/265-spec-061-docs-changelog.md @@ -0,0 +1,125 @@ +--- +status: completed +spec: [061-goal-topic-approve] +summary: Updated plan-goal and execute-topic to teach goal/topic approve, documented the approval transition in goal-writing.md, and added the Unreleased changelog entry +execution_id: vault-cli-goal-topic-approve-exec-265-spec-061-docs-changelog +dark-factory-version: v0.198.7 +created: "2026-10-20T10:03:18Z" +queued: "2026-10-20T10:16:58Z" +started: "2026-10-20T10:31:16Z" +completed: "2026-10-20T10:32:46Z" +--- + +# Teach the goal and topic commands the approve verb (spec 061, prompt 5 of 5) + + +- The goal planning command now approves a goal in the inbox instead of setting its phase directly. +- The topic execution command now approves a topic in the inbox instead of setting its phase directly. +- The reopen paths for closed or done rows keep their existing phase-set command, which is still legal outside the inbox. +- The goal writing guide documents the approval transition, the recorded approver and instant, and why the generic set is refused. +- The changelog carries one unreleased entry describing the new verbs and the closed bypass. +- No plugin version is hand-bumped; the release tail owns that. + + + +Update the two command docs that currently teach the generic-set bypass, and document the approval transition where goal authors read it, so an agent following the docs reaches for `goal approve` / `topic approve` instead of a command that is now refused. Closes acceptance criterion 5 of spec 061. + + + +Read `CLAUDE.md` for project conventions. + +Read fully, in this order: +- `commands/plan-goal.md` — step 3 ("Entry contract — flip if needed"), lines 44-50. The two `todo` arms currently instruct `vault-cli goal set "" phase planning`, which prompt 1 now refuses. +- `commands/execute-topic.md` — step 3 ("Refusal cases") and step 6. The `phase: todo` arm at line 56 currently instructs `vault-cli topic set "" phase planning`, which is now refused. The two `done`-reopen arms (lines 54-55) keep their `set … phase planning` command: those rows are not in the inbox, so the guard does not fire there. +- `docs/goal-writing.md` — § Frontmatter (the `status` valid-values paragraph) and § Lifecycle. There is no `phase` section yet; add one. +- `docs/task-writing.md` § Phase transitions — the contract this documentation mirrors. Reference it rather than restating the whole rule. +- `CHANGELOG.md` — the preamble and the newest section (`## v0.174.1`). There is currently no `## Unreleased` section. + +Coding-plugin docs (in-container path): +- `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — `## Unreleased` placement and the `feat:` prefix. + + + +1. **`commands/plan-goal.md` step 3.** Replace the two `todo` arms so the phase flip is the approval verb, and leave the deliberate-skip arms alone. Target wording: + + ``` + - `status` in `next`/`todo`/`backlog` → `vault-cli goal set "" status in_progress`, then the phase entry: + - `phase: todo` → `vault-cli goal approve ""` — the approval records `approved_by` and `approved_at` and flips to `planning` in one write; `vault-cli goal set "" phase planning` is refused on a `todo` row. + - `phase` empty → `vault-cli goal set "" phase todo`, then `vault-cli goal approve ""` — a goal with no phase is exempt from the gate and cannot be approved until it is in the inbox. + - `phase: planning` → no flip. + - `phase` is `execution` / `done` → skip the flip (treat as deliberate — sharpen but don't move phase backward). + - `status` already `in_progress` and `phase` is `todo` → `vault-cli goal approve ""`; `phase` empty → `vault-cli goal set "" phase todo`, then `vault-cli goal approve ""`. + - `status` already `in_progress` and `phase` is past planning → continue without flip; step 7 will skip the phase transition. + ``` + + Keep the rest of the command unchanged, including the "plan-goal never flips the phase" rule for `planning → execution`. + +2. **`commands/execute-topic.md` step 3.** Change the `phase: todo` / empty-phase arm to point at the approve verb: + + ``` + - `phase: todo` OR `phase` empty AND `status: in_progress` → `❌ Topic has no plan yet (phase: todo). Approve it with vault-cli topic approve "" once the topic passes /verify-topic.` (planning is non-skippable; a topic with no `phase` key is exempt from the gate, so set `phase: todo` first with `vault-cli topic set "" phase todo`) + ``` + + Leave the two `done`-reopen arms (lines 54-55) naming `vault-cli topic set "" phase planning` — those rows are past the inbox and the set remains legal. Add a short parenthetical to one of them: `(the phase set is legal here because the topic is not in the inbox)`. Leave the step 6 output list and the `planning → execution` gate unchanged. + +3. **`docs/goal-writing.md`.** Add a `### Phase and approval` subsection under § Frontmatter, immediately after the paragraph that lists the valid `status` values. It must state: + - `phase` walks `todo → planning → execution → done`, and `todo` is the operator's approval inbox. + - `vault-cli goal approve ""` performs `todo → planning` and records `approved_by` (default `operator`; `--by ` names another) and `approved_at` in the same write. + - `vault-cli goal set "" phase planning` is refused on a `todo` goal; the refusal points at `goal approve`. This is the same contract as `task approve` in `task-writing.md` § Phase transitions. + - A goal with no `phase` key is exempt from the gate and cannot be approved; set `phase: todo` first. + - A goal that already carries `approved_by` or `approved_at` is refused rather than re-approved. + + Do not add a new document — topics have no writing guide (spec Non-goal). + +4. **`CHANGELOG.md`.** Insert one `## Unreleased` section below the preamble and above `## v0.174.1`, with one `feat:` bullet naming both verbs, the recorded fields, and the closed bypass: + + ``` + ## Unreleased + + - feat: Add `vault-cli goal approve` and `vault-cli topic approve`, which move a row from the approval inbox (`phase: todo`) to `phase: planning` and record `approved_by` / `approved_at` in the same write; `goal set … phase planning` and `topic set … phase planning` on a `todo` row are now refused and point at the approve verb. + ``` + + Do not touch the preamble or any released section. + +5. **Self-check before finishing.** Re-run the `` block and confirm every row; walk acceptance criterion 5 of spec 061 against the change. + + + +- Only `commands/plan-goal.md`, `commands/execute-topic.md`, `docs/goal-writing.md`, and `CHANGELOG.md` change. Do not touch `pkg/`, `integration/`, or any other command doc. +- The `done`-reopen arms must keep naming `vault-cli topic set "" phase planning` — the guard fires only on `todo → planning`, so those arms are correct as written and must not be "fixed" into `topic approve` (which would refuse them). +- A `commands/` change requires a plugin version bump — do not hand-bump `.claude-plugin/plugin.json` or `.claude-plugin/marketplace.json`; the release tail owns that bump. +- The approval contract being mirrored is `docs/task-writing.md` § Phase transitions; reference it instead of restating it. +- No backfilling of `phase` on existing goals or topics; no owner assignment at approval time; no `--force`. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +EVIDENCE GREPS — run each and confirm the count. Absence assertions use `! grep -q` because `grep -c` exits 1 when the count is 0. + +``` +grep -c 'goal approve' commands/plan-goal.md # >= 1 (0 at HEAD) +grep -c 'topic approve' commands/execute-topic.md # >= 1 (0 at HEAD) +grep -c 'goal approve' docs/goal-writing.md # >= 1 (0 at HEAD) — acceptance criterion 5 +grep -c '^## Unreleased' CHANGELOG.md # 1 (0 at HEAD) +awk '/^## /{sec=$0} /goal approve/{print sec}' CHANGELOG.md # must print exactly: ## Unreleased +grep -c 'Reopen: vault-cli topic set' commands/execute-topic.md # >= 1 — the done-reopen arms survive the edit +``` + +⚠️ The `awk` row is the authoritative placement check: `grep -c 'goal approve' CHANGELOG.md` would pass with the bullet under a released section, which is the fold this repo's changelog rules guard against. + +TESTS: + +``` +make check-changelog +make precommit +``` + +`make check-changelog` must print `CHANGELOG structure OK`. `make precommit` must exit 0 apart from the named pre-existing `topic defer` integration failure; this prompt changes no Go code, so any other failure is yours to fix. + +SYNTAX: + +``` +grep -n '^## ' CHANGELOG.md | head -5 # first section is ## Unreleased, above ## v0.174.1 +``` + diff --git a/specs/completed/061-goal-topic-approve.md b/specs/completed/061-goal-topic-approve.md new file mode 100644 index 00000000..69d087a2 --- /dev/null +++ b/specs/completed/061-goal-topic-approve.md @@ -0,0 +1,143 @@ +--- +status: completed +approved: "2026-10-20T09:50:13Z" +verifying: "2026-10-20T10:32:47Z" +completed: "2026-10-20T10:40:32Z" +branch: dark-factory/goal-topic-approve +--- + +## Summary + +- Add `vault-cli goal approve ` and `vault-cli topic approve `. +- Each moves a row out of the approval inbox (`phase: todo`) into `phase: planning`, recording `approved_by` and `approved_at` in the same write. +- Close the generic-`set` bypass: `goal set … phase planning` and `topic set … phase planning` on a `todo` row are refused, exactly as `task set` already refuses them. +- Update the two command docs that currently teach the bypass (`commands/plan-goal.md`, `commands/execute-topic.md`). +- A row with no `phase` is exempt from the gate and cannot be approved; the refusal names the absent phase. + +## Problem + +A task enters planning only through `vault-cli task approve`, which records who approved it and when, and `task set … phase planning` is refused so the record cannot be skipped. Goals and topics have a `phase` field but neither the verb nor the guard. The manager-start gate in `bborbe/claude-supervisor` (`scripts/propose-managers.py`, the follow-on spec in `bborbe/claude-supervisor`) needs a recorded approval to read; without one it can only infer readiness from counts of approved tasks underneath. This spec gives that gate a single authoritative writer and closes the route that would write the phase without it. + +## Goal + +A goal or topic at `phase: todo` is approved by one command, which atomically writes `phase: planning`, `approved_by`, and `approved_at`. The generic `set` path cannot perform the same transition. A row that is not in the inbox, or that already carries an approval record, is refused with nothing written. Every other frontmatter key survives. + +## Non-goals + +- Backfilling `phase` on existing goals and topics. +- Changing the task approval flow or the task-side guards. +- Rejecting unknown phase values on the generic `set` path — `setPhaseField` already validates enum membership. +- Assigning an owner at approval time — goals and topics are not required to have an assignee, and this verb does not add one. +- A `docs/topic-writing.md` — topics have no writing guide (spec 051 declined one). +- Any change to `propose-managers.py` or `/supervisor:open` (those live in `bborbe/claude-supervisor`). +- **An accepted hole, mirrored from the task contract:** a row with no `phase` key can still reach `planning` via `goal set … phase planning`, because the guard fires only on `todo → planning`. `goal clear phase` followed by that set is therefore a two-step bypass. Closing it would require treating an absent phase as `todo`, which contradicts the operator's decision that absent `phase` means exempt. +- **No override exists.** `goal set` / `topic set` carry no `--force` flag, and this spec does not add one — adding it would change the shared entity-set command used by themes, objectives and visions. + +## Assumptions + +- Goals and topics already carry `phase`, and `todo` is the inbox phase. Verified in `pkg/domain/goal_phase.go` and `topic_phase.go`. +- `WriteGoal` / `WriteTopic` preserve unknown frontmatter keys. Verified: both serialize `RawMap()` (`pkg/storage/goal.go`, `pkg/storage/topic.go`). +- No goal or topic carries `approved_by` / `approved_at` today. Verified: only `pkg/ops/task_approve.go` writes those keys. +- `Goal` and `Topic` already embed their frontmatter and expose `Phase()`, `SetPhase()`, `Get()`, `Set()`. Verified in `pkg/domain/goal.go` and `topic.go`. +- `--by` is kept rather than replaced by a hardcoded `operator`: the record needs the field either way, and the flag already exists on `task approve`. + +## Acceptance Criteria + +- [ ] Happy path, goal and topic. `vault-cli goal approve ` on a goal at `phase: todo` exits 0 and prints one confirmation line containing the goal name; afterwards `vault-cli goal get phase` reads `planning`, `vault-cli goal get approved_by` reads `operator`, and the raw `approved_at:` line in the goal file is RFC3339 and falls within 60 seconds of `date -u` read before the invocation (`vault-cli goal get approved_at` is non-empty — the getter renders a `time.Time` as Go's `%v` form, not RFC3339). `vault-cli topic approve ` behaves identically on a topic. `--by alice` records `alice` on both. +- [ ] Table-driven refusals, goal and topic rows. Each exits non-zero, prints a message naming the row and the phase found vs required, and leaves `shasum -a 256` of the file unchanged, under **default text output**: + - approve on a row at `phase: planning`, `execution`, `done`, or with no `phase` key + - approve on a `todo` row that already carries `approved_by` or `approved_at` + - approve with `--by ""` + - approve on an unknown row name +- [ ] Set-path guard, goal and topic. `vault-cli goal set phase planning` on a goal at `phase: todo` exits non-zero, names `vault-cli goal approve` in the message, and leaves the file checksum unchanged; `vault-cli topic set phase planning` does the same. Narrowness: `vault-cli goal set phase execution` on a `todo` goal exits 0 and `vault-cli goal get phase` reads `execution`; `vault-cli goal set priority 2` on a `todo` goal exits 0 and `vault-cli goal get priority` reads `2`. +- [ ] Other keys preserved. With a fixture goal carrying an unknown key (`custom_key: keep`), the frontmatter after a successful approve differs from before only in `phase`, `approved_by`, `approved_at`. Evidence: `diff <(grep -vE '^(phase|approved_by|approved_at):' pre.md) <(grep -vE '^(phase|approved_by|approved_at):' post.md)` is empty, and `grep -c '^custom_key: keep' post.md` is 1. +- [ ] Docs updated. The `todo` arms of `commands/plan-goal.md` and `commands/execute-topic.md` name `goal approve` / `topic approve`; their `done`-reopen arms still name `set … phase planning`; `grep -c 'goal approve' docs/goal-writing.md` returns at least 1. +- [ ] `make precommit` exits 0. + +## Verification + +**Container-executable** (runs inside the YOLO container at `/workspace`): + +```bash +make precommit +make test +``` + +**Operator-executable** (host, after the binary builds — never against a live vault): + +```bash +make build +cp -r example /tmp/vault-approve-e2e +sed -i '' "s|__VAULT_PATH__|/tmp/vault-approve-e2e/vault|" /tmp/vault-approve-e2e/config.yaml +# create a scratch goal at phase: todo in /tmp/vault-approve-e2e/vault/23 Goals/ +./bin/vault-cli --config /tmp/vault-approve-e2e/config.yaml goal approve ""; echo "exit=$?" +./bin/vault-cli --config /tmp/vault-approve-e2e/config.yaml goal get "" phase +./bin/vault-cli --config /tmp/vault-approve-e2e/config.yaml goal get "" approved_by +./bin/vault-cli --config /tmp/vault-approve-e2e/config.yaml goal get "" approved_at +``` + +Repeat with `topic` and a scratch topic. Run the refusal cases against the same copy, comparing `shasum -a 256` before and after each. + +## Desired Behavior + +1. `vault-cli goal approve ` reads the goal by name, refuses unless its phase is exactly `todo`, refuses if it already carries `approved_by` or `approved_at`, then writes `phase: planning`, `approved_by`, and `approved_at` in one write. `vault-cli topic approve ` does the same for a topic. +2. `--by` sets the recorded approver and defaults to `operator`. An empty `--by` is refused before the row is read. +3. The recorded `approved_at` is the operation's clock reading, formatted the same way `task approve` writes it. A zero clock reading is refused rather than recorded — a unit-test-only path with no operator-observable trigger, so it carries a unit test rather than an acceptance criterion. +4. A refusal prints a message naming the row, the phase found, and the phase required, and exits non-zero. Nothing is written. +5. Success prints a one-line confirmation naming the row, and exits 0. +6. Unknown or extra frontmatter keys on the row survive the write untouched. +7. `goal set phase planning` and `topic set phase planning` on a row at `phase: todo` are refused before any mutation, with a message pointing at the approve verb. Only that (key, value, current-phase) triple is guarded; a missing phase, an unknown phase, every non-phase key and every other target phase keep their existing behaviour. + +## Constraints + +- The approval contract being mirrored is documented in `docs/task-writing.md` § Phase transitions — read it rather than re-deriving the rules from `pkg/ops/task_approve.go`. +- The set-path guard mirrors `todoPlanningSetRefusal` in `pkg/ops/frontmatter.go`. +- Reuse the existing storage interfaces `WriteGoal` / `WriteTopic` and the existing find-by-name methods; do not add a new storage method. +- Do not change `task approve`, the task-side guards, or the frontmatter serializers. +- A refusal writes nothing: the guard and every precondition run before the first mutation, so the file on disk is byte-identical. +- A `commands/` change requires a plugin version bump — the release tail is `autoRelease: true`, so the releaser handles it; do not hand-bump the manifests. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | +|---|---|---| +| Row not found | Non-zero exit; stderr names the row | `vault-cli goal list \| grep ` to find the real name | +| Phase is not `todo` (including absent) | Non-zero exit; stderr names found vs required | `vault-cli goal set phase todo`, then approve | +| Row already carries `approved_by` / `approved_at` | Non-zero exit; stderr names the present keys | Nothing to do — already approved; read the record | +| `--by` empty | Non-zero exit before any read | Pass `--by ` | +| Clock reading is zero | Non-zero exit, nothing written | Fix the injected clock; re-run | +| Write fails | Non-zero exit; error wrapped with context | Re-run; the file is unchanged | +| Two concurrent approves of the same row | Single-writer assumption: last writer wins, and both may record an approval. Accepted — the operator is the only writer, and the existing-record guard makes the second call refuse once the first write is visible | Re-read the row; if the record is wrong, `vault-cli goal clear approved_by` and `vault-cli goal clear approved_at`, then `vault-cli goal set phase todo`, then `vault-cli goal approve ` (the row is at `planning` after clearing, so it must be returned to the inbox first) | + +## Security / Abuse Cases + +The verbs write to a markdown file inside the configured vault. They take a row name from the operator and perform no shell interpolation. They refuse rather than guessing when the row is not in the expected state. No network access, no credential handling, and no path traversal beyond the existing name lookup the other goal/topic verbs already use. + +## Suggested Decomposition + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Set-path guard: refuse `todo → planning` on the goal and topic `set` paths, mirroring `todoPlanningSetRefusal`; tests | 7 | 3 | — | +| 2 | `pkg/ops/goal_approve.go` + Ginkgo tests, modelled on `task_approve.go` | 1, 2, 3, 4, 5, 6 | 1, 2, 4 | — | +| 3 | `pkg/ops/topic_approve.go` + Ginkgo tests | 1, 2, 3, 4, 5, 6 | 1, 2, 4 | 2 | +| 4 | CLI wiring: `goal approve` and `topic approve` subcommands with `--by`, modelled on `createTaskApproveCommand` | 1, 2, 5 | 1, 2 | 2, 3 | +| 5 | Command docs (`commands/plan-goal.md`, `commands/execute-topic.md`), `docs/goal-writing.md` approval paragraph, CHANGELOG `## Unreleased` bullet | — | 5 | 4 | + +## Do-Nothing Option + +Without this verb, no goal or topic can move from `todo` to `planning` with a recorded approval, and the generic `set` path can move it without one. The manager-start gate would keep inferring readiness from task counts, which is exactly the ambiguity this work exists to remove. The cost is one small command per repository; the alternative is that the `phase` field stays half-wired. + +## Verification Result + +**Verified:** 2026-10-20T10:40:02Z (HEAD c786b1f) +**Binary:** ./bin/vault-cli (rebuilt from HEAD via `make build`) +**Scenario:** spec `## Verification` operator walk against scratch vault `/tmp/vault-approve-e2e` with fresh goal and topic fixtures +**Evidence:** +- `goal approve "E2E Todo Goal"` -> exit 0, prints `Goal approved: E2E Todo Goal (phase planning)`; phase=planning, approved_by=operator, raw file line `approved_at: 2026-10-20T12:35:55.401684+02:00` against `date -u` = 2026-10-20T10:35:55Z +- `topic approve "E2E Todo Topic"` -> exit 0, phase=planning, approved_by=operator; `--by alice` recorded alice on both a goal and a topic +- 16 refusal cases (goal+topic x planning/execution/done/no-phase/already-approved_by/already-approved_at/`--by ""`/unknown row): all exit 1, message names the row and the found-vs-required phase or present key, `shasum -a 256` unchanged +- `goal set ... phase planning` and `topic set ... phase planning` on a todo row -> exit 1 naming `vault-cli goal approve` / `vault-cli topic approve`, checksum unchanged; `set phase execution` and `set priority 2` on a todo row -> exit 0 with the value applied +- preservation: diff excluding `phase`/`approved_by`/`approved_at` empty (order-insensitive on a natural fixture, literal on a serializer-ordered fixture); `custom_key: keep` count 1 +- docs: todo arms of `commands/plan-goal.md` and `commands/execute-topic.md` name the approve verbs; execute-topic reopen arm still names `set ... phase planning`; `grep -c "goal approve" docs/goal-writing.md` = 1 +- `make precommit` exit 0; `make test` exit 0 +**Verdict:** PASS