From 6e07f44a84503eab8466610ef2df8e7293a15729 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 20 Oct 2026 13:23:14 +0200 Subject: [PATCH 1/2] feat: wire goal and topic approve into the CLI and update the command docs (spec 061, part 2 of 2) --- CHANGELOG.md | 5 + commands/execute-topic.md | 4 +- commands/plan-goal.md | 8 +- docs/goal-writing.md | 6 + integration/cli_test.go | 645 +++++++++++++++++++++++++++++++++++++- pkg/cli/cli.go | 126 ++++++++ 6 files changed, 787 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 915844ff..0f1a8f4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- feat: Add `GoalApproveOperation` and `TopicApproveOperation`, which move a `phase: todo` row to `phase: planning` and record `approved_by` / `approved_at` in one write; a row outside the approval inbox, or one already carrying an approval record, is refused with nothing written. +- feat: Add `vault-cli goal approve` and `vault-cli topic approve`, the CLI surface over those operations (`--by` defaults to `operator`, an empty `--by` is refused, and a refusal exits non-zero in both plain and `--output json` modes). `goal set … phase planning` and `topic set … phase planning` on a `todo` row are now refused and point at the approve verb. + ## v0.174.1 - fix: `session-close` prints its open-goal status lines from a new `session-close-goals.sh` resolver and requires the closer to reproduce them, so a close can no longer drop the line silently. Measured 2026-10-20: one real close ran the phase's own lookup, received `status=in_progress`, and emitted no goal line at all. diff --git a/commands/execute-topic.md b/commands/execute-topic.md index ca13a8ff..9af25839 100644 --- a/commands/execute-topic.md +++ b/commands/execute-topic.md @@ -51,9 +51,9 @@ vault-cli topic get "" phase --output json Refuse and STOP if any apply: -- `status: completed` OR `status: aborted` → `❌ Topic closed (status: ). Reopen: vault-cli topic set "" status in_progress; vault-cli topic set "" phase planning — there is no reopen subcommand.` +- `status: completed` OR `status: aborted` → `❌ Topic closed (status: ). Reopen: vault-cli topic set "" status in_progress; vault-cli topic set "" phase planning — there is no reopen subcommand.` (the phase set is legal here because the topic is not in the inbox) - `phase: done` → `❌ Topic phase is done. Reopen with the same two commands as above.` -- `phase: todo` OR `phase` empty AND `status: in_progress` → `❌ Topic has no plan yet (phase: todo). Set it with vault-cli topic set "" phase planning once the topic passes /verify-topic.` (planning is non-skippable) +- `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`) ### 4. Status entry contract (mutate, then continue) diff --git a/commands/plan-goal.md b/commands/plan-goal.md index d3ac5f8d..5cef0ec5 100644 --- a/commands/plan-goal.md +++ b/commands/plan-goal.md @@ -45,8 +45,12 @@ vault-cli goal get "" phase --output json The goal is to land at `status: in_progress, phase: planning` for fresh goals; respect a deliberate post-planning phase setting. -- `status` in `next`/`todo`/`backlog` → flip status AND phase together: `vault-cli goal set "" status in_progress` + `vault-cli goal set "" phase planning` (if phase is empty/`todo`/`planning`). Skip the phase flip if phase is `execution` / `done` (treat as deliberate — sharpen but don't move phase backward). -- `status` already `in_progress` and `phase` is `todo`/empty → `vault-cli goal set "" phase planning` +- `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. ### 4. Run goal-auditor diff --git a/docs/goal-writing.md b/docs/goal-writing.md index ae1f7b80..2a6eb0d8 100644 --- a/docs/goal-writing.md +++ b/docs/goal-writing.md @@ -128,6 +128,12 @@ manager_session_id: # optional — written by claud `status` valid values: `next`, `in_progress`, `backlog`, `hold`, `completed`, `aborted`. `todo` is accepted as a legacy alias for `next` on read; do not write it. +### Phase and approval + +`phase` walks `todo → planning → execution → done`, and `todo` is the operator's approval inbox — the same phase lifecycle tasks carry. `vault-cli goal approve ""` performs the `todo → planning` transition 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, and the refusal points at `goal approve`: a phase set without an approval record is not an approval. This is the same contract as `task approve`, stated in full 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, and the refusal writes nothing. + ### Dependencies (`blocked_by`) `blocked_by` declares the goals this goal cannot start before. It is a YAML list of goal names; each entry may be a plain name (`Blocker Goal`) or a wikilink (`[[Blocker Goal]]`) — both forms resolve the same way. A scalar value (`blocked_by: Blocker Goal`) is malformed and reads as an empty list, so it never blocks. `vault-cli goal lint` reports a goal whose `blocked_by` is shaped this way; `vault-cli task validate` and `vault-cli task lint` report the task-side equivalent. diff --git a/integration/cli_test.go b/integration/cli_test.go index 5a6733a4..ab2a7ef4 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -498,6 +498,7 @@ var _ = Describe("vault-cli integration tests", func() { Entry("goal set", "goal", "set"), Entry("goal clear", "goal", "clear"), Entry("goal complete", "goal", "complete"), + Entry("goal approve", "goal", "approve"), Entry("goal add", "goal", "add"), Entry("goal remove", "goal", "remove"), Entry("goal work-on", "goal", "work-on"), @@ -510,6 +511,7 @@ var _ = Describe("vault-cli integration tests", func() { Entry("topic set", "topic", "set"), Entry("topic clear", "topic", "clear"), Entry("topic complete", "topic", "complete"), + Entry("topic approve", "topic", "approve"), Entry("topic defer", "topic", "defer"), Entry("topic add", "topic", "add"), Entry("topic remove", "topic", "remove"), @@ -2808,6 +2810,112 @@ blocked_by: }) }) + Describe("goal and topic set phase guard", func() { + var vaultPath, configPath string + var cleanup func() + + AfterEach(func() { + cleanup() + }) + + runEntityCommand := func(args ...string) *gexec.Session { + fullArgs := append( + []string{"--config", configPath, "--vault", "test"}, + args..., + ) + session, err := gexec.Start(exec.Command(binPath, fullArgs...), GinkgoWriter, GinkgoWriter) + Expect(err).NotTo(HaveOccurred()) + return session + } + + readFile := func(path string) []byte { + content, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + return content + } + + sha256OfFile := func(path string) string { + sum := sha256.Sum256(readFile(path)) + return fmt.Sprintf("%x", sum) + } + + // Keys are alphabetical because the storage writer re-serializes the whole + // frontmatter in that order. + goalInbox := `--- +page_type: goal +phase: todo +priority: 1 +status: next +--- +` + + topicInbox := `--- +page_type: topic +phase: todo +priority: 1 +status: next +--- +` + + It("AC3a: goal set phase planning refuses, names the approve verb, and writes nothing", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": goalInbox, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + before := sha256OfFile(goalFile) + + session := runEntityCommand("goal", "set", "Alpha", "phase", "planning") + Eventually(session).Should(gexec.Exit(1)) + + Expect(string(session.Err.Contents())).To(ContainSubstring("vault-cli goal approve")) + Expect(sha256OfFile(goalFile)).To(Equal(before)) + }) + + It("AC3b: topic set phase planning refuses, names the approve verb, and writes nothing", func() { + vaultPath, configPath, cleanup = createTempVaultWithTopics("Topics") + Expect(os.MkdirAll(filepath.Join(vaultPath, "Topics"), 0755)).To(Succeed()) + Expect(os.WriteFile( + filepath.Join(vaultPath, "Topics", "Alpha.md"), + []byte(topicInbox), + 0600, + )).To(Succeed()) + topicFile := filepath.Join(vaultPath, "Topics", "Alpha.md") + before := sha256OfFile(topicFile) + + session := runEntityCommand("topic", "set", "Alpha", "phase", "planning") + Eventually(session).Should(gexec.Exit(1)) + + Expect(string(session.Err.Contents())).To(ContainSubstring("vault-cli topic approve")) + Expect(sha256OfFile(topicFile)).To(Equal(before)) + }) + + It("AC3c: goal set phase execution stays allowed", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": goalInbox, + }) + + Eventually(runEntityCommand("goal", "set", "Alpha", "phase", "execution")). + Should(gexec.Exit(0)) + + getSession := runEntityCommand("goal", "get", "Alpha", "phase") + Eventually(getSession).Should(gexec.Exit(0)) + Expect(string(getSession.Out.Contents())).To(ContainSubstring("execution")) + }) + + It("AC3d: goal set a non-phase key stays allowed", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": goalInbox, + }) + + Eventually(runEntityCommand("goal", "set", "Alpha", "priority", "2")). + Should(gexec.Exit(0)) + + getSession := runEntityCommand("goal", "get", "Alpha", "priority") + Eventually(getSession).Should(gexec.Exit(0)) + Expect(string(getSession.Out.Contents())).To(ContainSubstring("2")) + }) + }) + Describe("vault-cli task set flag provenance", func() { var vaultPath, configPath string var cleanup func() @@ -4022,6 +4130,537 @@ body line }) }) + Describe("goal approve", func() { + var vaultPath, configPath string + var cleanup func() + + AfterEach(func() { + cleanup() + }) + + runEntityCommand := func(args ...string) *gexec.Session { + fullArgs := append( + []string{"--config", configPath, "--vault", "test"}, + args..., + ) + session, err := gexec.Start(exec.Command(binPath, fullArgs...), GinkgoWriter, GinkgoWriter) + Expect(err).NotTo(HaveOccurred()) + return session + } + + sha256OfFile := func(path string) string { + data, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + sum := sha256.Sum256(data) + return fmt.Sprintf("%x", sum) + } + + readFile := func(path string) string { + content, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + return string(content) + } + + // valueOf returns the value of a top-level frontmatter key line, or "". + valueOf := func(content, key string) string { + for _, line := range strings.Split(content, "\n") { + if strings.HasPrefix(line, key+": ") { + return strings.TrimPrefix(line, key+": ") + } + } + return "" + } + + // withoutApprovalKeys drops the three keys the approval settles, so the + // remainder of the file can be compared as a whole. + approvalKeyPattern := regexp.MustCompile(`^(phase|approved_by|approved_at):`) + withoutApprovalKeys := func(content string) []string { + var out []string + for _, line := range strings.Split(content, "\n") { + if !approvalKeyPattern.MatchString(line) { + out = append(out, line) + } + } + return out + } + + // Every fixture's frontmatter keys are authored in alphabetical order: the + // storage writer re-serializes the whole frontmatter in that order. + + // A goal waiting in the operator's inbox. + todoFrontmatter := `--- +page_type: goal +phase: todo +priority: 1 +status: next +--- +body line +` + + // A goal carrying an unrelated scalar key alongside the inbox phase. + todoWithUnrelatedKeys := `--- +custom_key: keep +page_type: goal +phase: todo +priority: 1 +status: next +--- +body line +` + + planningFrontmatter := `--- +page_type: goal +phase: planning +priority: 1 +status: next +--- +body line +` + + executionFrontmatter := `--- +page_type: goal +phase: execution +priority: 1 +status: next +--- +body line +` + + doneFrontmatter := `--- +page_type: goal +phase: done +priority: 1 +status: next +--- +body line +` + + // The nil-phase arm: a goal with no phase key at all. + noPhaseFrontmatter := `--- +page_type: goal +priority: 1 +status: next +--- +body line +` + + // One-key-each fixtures are load-bearing: a guard that checks only one key + // passes the both-keys fixture, so a combined fixture cannot distinguish + // "refuses either key" from "refuses one key". + approvedByOnlyFrontmatter := `--- +approved_by: someone +page_type: goal +phase: todo +priority: 1 +status: next +--- +body line +` + + approvedAtOnlyFrontmatter := `--- +approved_at: 2026-01-01T00:00:00Z +page_type: goal +phase: todo +priority: 1 +status: next +--- +body line +` + + approvedBothFrontmatter := `--- +approved_at: 2026-01-01T00:00:00Z +approved_by: someone +page_type: goal +phase: todo +priority: 1 +status: next +--- +body line +` + + It("AC1: goal approve records the todo -> planning transition and its record", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoFrontmatter, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + + before := time.Now().UTC() + session := runEntityCommand("goal", "approve", "Alpha") + Eventually(session).Should(gexec.Exit(0)) + Expect(string(session.Out.Contents())).To(ContainSubstring("Alpha")) + + after := readFile(goalFile) + Expect(valueOf(after, "phase")).To(Equal("planning")) + Expect(valueOf(after, "approved_by")).To(Equal("operator")) + + // yaml.v3 renders a bare time.Time unquoted; tolerate a quoted form rather + // than pinning the encoder's choice. The raw line is parsed, because + // `goal get` renders the value through Go's %v form. + approvedAt := strings.Trim(valueOf(after, "approved_at"), `"`) + parsed, err := time.Parse(time.RFC3339, approvedAt) + Expect(err).NotTo(HaveOccurred(), "approved_at %q is not RFC3339", approvedAt) + Expect(parsed).To(BeTemporally(">=", before)) + Expect(parsed).To(BeTemporally("<=", before.Add(60*time.Second))) + + phaseSession := runEntityCommand("goal", "get", "Alpha", "phase") + Eventually(phaseSession).Should(gexec.Exit(0)) + Expect(string(phaseSession.Out.Contents())).To(ContainSubstring("planning")) + + bySession := runEntityCommand("goal", "get", "Alpha", "approved_by") + Eventually(bySession).Should(gexec.Exit(0)) + Expect(string(bySession.Out.Contents())).To(ContainSubstring("operator")) + + atSession := runEntityCommand("goal", "get", "Alpha", "approved_at") + Eventually(atSession).Should(gexec.Exit(0)) + Expect(strings.TrimSpace(string(atSession.Out.Contents()))).NotTo(BeEmpty()) + }) + + It("AC1: goal approve --by records the named approver", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoFrontmatter, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + + session := runEntityCommand("goal", "approve", "Alpha", "--by", "alice") + Eventually(session).Should(gexec.Exit(0)) + + Expect(valueOf(readFile(goalFile), "approved_by")).To(Equal("alice")) + }) + + It("AC2: an empty --by is refused and nothing is written", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoFrontmatter, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + before := sha256OfFile(goalFile) + + session := runEntityCommand("goal", "approve", "Alpha", "--by", "") + Eventually(session).Should(gexec.Exit(1)) + Expect(string(session.Err.Contents())).To(ContainSubstring("approved_by")) + Expect(sha256OfFile(goalFile)).To(Equal(before)) + }) + + DescribeTable("AC4: a refusal writes nothing and names what was found", + func(name string, fixture string, wantStderr []string) { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": fixture, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + before := sha256OfFile(goalFile) + + session := runEntityCommand("goal", "approve", name) + Eventually(session).Should(gexec.Exit(1)) + + stderr := string(session.Err.Contents()) + for _, want := range wantStderr { + Expect(stderr).To(ContainSubstring(want)) + } + Expect(sha256OfFile(goalFile)).To(Equal(before)) + }, + Entry("planning", "Alpha", planningFrontmatter, []string{"planning", "todo"}), + Entry("execution", "Alpha", executionFrontmatter, []string{"execution", "todo"}), + Entry("done", "Alpha", doneFrontmatter, []string{"done", "todo"}), + Entry("nil phase", "Alpha", noPhaseFrontmatter, []string{"todo"}), + Entry("approved_by only", "Alpha", approvedByOnlyFrontmatter, []string{"approved_by"}), + Entry("approved_at only", "Alpha", approvedAtOnlyFrontmatter, []string{"approved_at"}), + Entry("both approval keys", "Alpha", approvedBothFrontmatter, []string{"approved_by", "approved_at"}), + Entry("unknown name", "Missing", todoFrontmatter, []string{"Missing"}), + ) + + It("AC4: goal approve preserves every unrelated key", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoWithUnrelatedKeys, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + before := readFile(goalFile) + + session := runEntityCommand("goal", "approve", "Alpha") + Eventually(session).Should(gexec.Exit(0)) + + after := readFile(goalFile) + Expect(valueOf(after, "custom_key")).To(Equal("keep")) + // Whole-slice equality: a count would prove only that nothing was + // removed, not that nothing stray was added. + Expect(withoutApprovalKeys(after)).To(Equal(withoutApprovalKeys(before))) + }) + + It("JSON: --output json carries the goal name and the new phase", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoFrontmatter, + }) + + session := runEntityCommand("goal", "approve", "Alpha", "--output", "json") + Eventually(session).Should(gexec.Exit(0)) + + var parsed map[string]any + Expect(json.Unmarshal(session.Out.Contents(), &parsed)).To(Succeed()) + Expect(parsed["name"]).To(Equal("Alpha")) + Expect(parsed["phase"]).To(Equal("planning")) + }) + + It("JSON: a refusal with --output json exits non-zero", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": planningFrontmatter, + }) + + session := runEntityCommand("goal", "approve", "Alpha", "--output", "json") + Eventually(session).Should(gexec.Exit(1)) + }) + }) + + Describe("topic approve", func() { + var configPath string + var cleanup func() + + AfterEach(func() { + cleanup() + }) + + runEntityCommand := func(args ...string) *gexec.Session { + fullArgs := append( + []string{"--config", configPath, "--vault", "test"}, + args..., + ) + session, err := gexec.Start(exec.Command(binPath, fullArgs...), GinkgoWriter, GinkgoWriter) + Expect(err).NotTo(HaveOccurred()) + return session + } + + sha256OfFile := func(path string) string { + data, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + sum := sha256.Sum256(data) + return fmt.Sprintf("%x", sum) + } + + readFile := func(path string) string { + content, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + return string(content) + } + + valueOf := func(content, key string) string { + for _, line := range strings.Split(content, "\n") { + if strings.HasPrefix(line, key+": ") { + return strings.TrimPrefix(line, key+": ") + } + } + return "" + } + + approvalKeyPattern := regexp.MustCompile(`^(phase|approved_by|approved_at):`) + withoutApprovalKeys := func(content string) []string { + var out []string + for _, line := range strings.Split(content, "\n") { + if !approvalKeyPattern.MatchString(line) { + out = append(out, line) + } + } + return out + } + + // createTopicVault writes one topic page into a fresh vault and returns the + // vault path, config path and page path. + createTopicVault := func(content string) (string, string, string) { + vp, cp, cl := createTempVaultWithTopics("Topics") + configPath, cleanup = cp, cl + Expect(os.MkdirAll(filepath.Join(vp, "Topics"), 0755)).To(Succeed()) + topicFile := filepath.Join(vp, "Topics", "Alpha.md") + Expect(os.WriteFile(topicFile, []byte(content), 0600)).To(Succeed()) + return vp, cp, topicFile + } + + todoFrontmatter := `--- +page_type: topic +phase: todo +priority: 1 +status: next +--- +body line +` + + todoWithUnrelatedKeys := `--- +custom_key: keep +page_type: topic +phase: todo +priority: 1 +status: next +--- +body line +` + + planningFrontmatter := `--- +page_type: topic +phase: planning +priority: 1 +status: next +--- +body line +` + + executionFrontmatter := `--- +page_type: topic +phase: execution +priority: 1 +status: next +--- +body line +` + + doneFrontmatter := `--- +page_type: topic +phase: done +priority: 1 +status: next +--- +body line +` + + noPhaseFrontmatter := `--- +page_type: topic +priority: 1 +status: next +--- +body line +` + + approvedByOnlyFrontmatter := `--- +approved_by: someone +page_type: topic +phase: todo +priority: 1 +status: next +--- +body line +` + + approvedAtOnlyFrontmatter := `--- +approved_at: 2026-01-01T00:00:00Z +page_type: topic +phase: todo +priority: 1 +status: next +--- +body line +` + + approvedBothFrontmatter := `--- +approved_at: 2026-01-01T00:00:00Z +approved_by: someone +page_type: topic +phase: todo +priority: 1 +status: next +--- +body line +` + + It("AC1: topic approve records the todo -> planning transition and its record", func() { + _, _, topicFile := createTopicVault(todoFrontmatter) + + before := time.Now().UTC() + session := runEntityCommand("topic", "approve", "Alpha") + Eventually(session).Should(gexec.Exit(0)) + Expect(string(session.Out.Contents())).To(ContainSubstring("Alpha")) + + after := readFile(topicFile) + Expect(valueOf(after, "phase")).To(Equal("planning")) + Expect(valueOf(after, "approved_by")).To(Equal("operator")) + + approvedAt := strings.Trim(valueOf(after, "approved_at"), `"`) + parsed, err := time.Parse(time.RFC3339, approvedAt) + Expect(err).NotTo(HaveOccurred(), "approved_at %q is not RFC3339", approvedAt) + Expect(parsed).To(BeTemporally(">=", before)) + Expect(parsed).To(BeTemporally("<=", before.Add(60*time.Second))) + + phaseSession := runEntityCommand("topic", "get", "Alpha", "phase") + Eventually(phaseSession).Should(gexec.Exit(0)) + Expect(string(phaseSession.Out.Contents())).To(ContainSubstring("planning")) + + bySession := runEntityCommand("topic", "get", "Alpha", "approved_by") + Eventually(bySession).Should(gexec.Exit(0)) + Expect(string(bySession.Out.Contents())).To(ContainSubstring("operator")) + + atSession := runEntityCommand("topic", "get", "Alpha", "approved_at") + Eventually(atSession).Should(gexec.Exit(0)) + Expect(strings.TrimSpace(string(atSession.Out.Contents()))).NotTo(BeEmpty()) + }) + + It("AC1: topic approve --by records the named approver", func() { + _, _, topicFile := createTopicVault(todoFrontmatter) + + session := runEntityCommand("topic", "approve", "Alpha", "--by", "alice") + Eventually(session).Should(gexec.Exit(0)) + + Expect(valueOf(readFile(topicFile), "approved_by")).To(Equal("alice")) + }) + + It("AC2: an empty --by is refused and nothing is written", func() { + _, _, topicFile := createTopicVault(todoFrontmatter) + before := sha256OfFile(topicFile) + + session := runEntityCommand("topic", "approve", "Alpha", "--by", "") + Eventually(session).Should(gexec.Exit(1)) + Expect(string(session.Err.Contents())).To(ContainSubstring("approved_by")) + Expect(sha256OfFile(topicFile)).To(Equal(before)) + }) + + DescribeTable("AC4: a refusal writes nothing and names what was found", + func(name string, fixture string, wantStderr []string) { + _, _, topicFile := createTopicVault(fixture) + before := sha256OfFile(topicFile) + + session := runEntityCommand("topic", "approve", name) + Eventually(session).Should(gexec.Exit(1)) + + stderr := string(session.Err.Contents()) + for _, want := range wantStderr { + Expect(stderr).To(ContainSubstring(want)) + } + Expect(sha256OfFile(topicFile)).To(Equal(before)) + }, + Entry("planning", "Alpha", planningFrontmatter, []string{"planning", "todo"}), + Entry("execution", "Alpha", executionFrontmatter, []string{"execution", "todo"}), + Entry("done", "Alpha", doneFrontmatter, []string{"done", "todo"}), + Entry("nil phase", "Alpha", noPhaseFrontmatter, []string{"todo"}), + Entry("approved_by only", "Alpha", approvedByOnlyFrontmatter, []string{"approved_by"}), + Entry("approved_at only", "Alpha", approvedAtOnlyFrontmatter, []string{"approved_at"}), + Entry("both approval keys", "Alpha", approvedBothFrontmatter, []string{"approved_by", "approved_at"}), + Entry("unknown name", "Missing", todoFrontmatter, []string{"Missing"}), + ) + + It("AC4: topic approve preserves every unrelated key", func() { + _, _, topicFile := createTopicVault(todoWithUnrelatedKeys) + before := readFile(topicFile) + + session := runEntityCommand("topic", "approve", "Alpha") + Eventually(session).Should(gexec.Exit(0)) + + after := readFile(topicFile) + Expect(valueOf(after, "custom_key")).To(Equal("keep")) + Expect(withoutApprovalKeys(after)).To(Equal(withoutApprovalKeys(before))) + }) + + It("JSON: --output json carries the topic name and the new phase", func() { + createTopicVault(todoFrontmatter) + + session := runEntityCommand("topic", "approve", "Alpha", "--output", "json") + Eventually(session).Should(gexec.Exit(0)) + + var parsed map[string]any + Expect(json.Unmarshal(session.Out.Contents(), &parsed)).To(Succeed()) + Expect(parsed["name"]).To(Equal("Alpha")) + Expect(parsed["phase"]).To(Equal("planning")) + }) + + It("JSON: a refusal with --output json exits non-zero", func() { + createTopicVault(planningFrontmatter) + + session := runEntityCommand("topic", "approve", "Alpha", "--output", "json") + Eventually(session).Should(gexec.Exit(1)) + }) + }) + Describe("task answer", func() { var vaultPath, configPath string var cleanup func() @@ -4650,7 +5289,7 @@ page_type: goal }) Describe("vault-cli topic command family", func() { - It("topic --help lists exactly the twelve leaves", func() { + It("topic --help lists exactly the thirteen leaves", func() { cmd := exec.Command(binPath, "topic", "--help") session, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter) Expect(err).NotTo(HaveOccurred()) @@ -4658,9 +5297,9 @@ page_type: goal leaves := helpLeafNames(string(session.Out.Contents())) Expect(leaves).NotTo(BeEmpty()) - Expect(leaves).To(HaveLen(12)) + Expect(leaves).To(HaveLen(13)) Expect(leaves).To(ConsistOf( - "add", "clear", "complete", "defer", "get", "lint", "list", + "add", "approve", "clear", "complete", "defer", "get", "lint", "list", "remove", "search", "set", "show", "work-on", )) }) diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 48cd5ef1..cdf31199 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -1442,6 +1442,7 @@ func createGoalCommands( }, )) cmd.AddCommand(createGoalCompleteCommand(ctx, configLoader, vaultName, outputFormat)) + cmd.AddCommand(createGoalApproveCommand(ctx, configLoader, vaultName, outputFormat)) cmd.AddCommand(createGoalDeferCommand(ctx, configLoader, vaultName, outputFormat)) cmd.AddCommand(createWorkOnGoalCommand(ctx, configLoader, vaultName, outputFormat)) return cmd @@ -1510,6 +1511,68 @@ func createGoalCompleteCommand( return cmd } +// createGoalApproveCommand builds `vault-cli goal approve`, the CLI surface over +// the pkg/ops goal approval operation. It performs the recorded todo -> planning +// transition and, like `task approve`, exits non-zero when a refusal is printed +// as JSON. +// +//nolint:dupl // Mutation commands have similar structure but different operations +func createGoalApproveCommand( + ctx context.Context, + configLoader *config.Loader, + vaultName *string, + outputFormat *string, +) *cobra.Command { + var approvedBy string + + cmd := &cobra.Command{ + Use: "approve ", + Short: "Approve a goal in the operator's inbox (todo -> planning)", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + 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 { + // The JSON error object is printed and the error is then returned, so a + // refusal exits non-zero in both output modes. + if OutputFormat(*outputFormat).IsJSON() { + _ = PrintJSON(map[string]any{"success": false, "error": err.Error()}) + } + return err + } + return nil + }, + } + cmd.Flags().StringVar(&approvedBy, "by", "operator", "Who is approving; recorded as approved_by") + return cmd +} + //nolint:dupl // Structurally parallel to the topic variant; frozen entity-specific strings prevent dedup func createGoalDeferCommand( ctx context.Context, @@ -1697,6 +1760,7 @@ func createTopicCommands( }, )) cmd.AddCommand(createTopicCompleteCommand(ctx, configLoader, vaultName, outputFormat)) + cmd.AddCommand(createTopicApproveCommand(ctx, configLoader, vaultName, outputFormat)) cmd.AddCommand(createTopicDeferCommand(ctx, configLoader, vaultName, outputFormat)) cmd.AddCommand(createWorkOnTopicCommand(ctx, configLoader, vaultName, outputFormat)) return cmd @@ -1747,6 +1811,68 @@ func createTopicCompleteCommand( } } +// createTopicApproveCommand builds `vault-cli topic approve`, the CLI surface +// over the pkg/ops topic approval operation. It performs the recorded +// todo -> planning transition and, like `task approve`, exits non-zero when a +// refusal is printed as JSON. +// +//nolint:dupl // Mutation commands have similar structure but different operations +func createTopicApproveCommand( + ctx context.Context, + configLoader *config.Loader, + vaultName *string, + outputFormat *string, +) *cobra.Command { + var approvedBy string + + cmd := &cobra.Command{ + Use: "approve ", + Short: "Approve a topic in the operator's inbox (todo -> planning)", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + topicName := 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) + topicStore := storage.NewTopicStorage(storageConfig) + approveOp := ops.NewTopicApproveOperation(topicStore, currentDateTime) + result, err := approveOp.Execute(ctx, vault.Path, topicName, 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.TopicPhasePlanning), + }) + } + fmt.Printf("✅ Topic approved: %s (phase %s)\n", result.Name, domain.TopicPhasePlanning) + return nil + }) + if err != nil { + // The JSON error object is printed and the error is then returned, so a + // refusal exits non-zero in both output modes. + if OutputFormat(*outputFormat).IsJSON() { + _ = PrintJSON(map[string]any{"success": false, "error": err.Error()}) + } + return err + } + return nil + }, + } + cmd.Flags().StringVar(&approvedBy, "by", "operator", "Who is approving; recorded as approved_by") + return cmd +} + // createTopicDeferCommand defers a topic to a date. // //nolint:dupl // Structurally parallel to the goal variant; frozen entity-specific strings prevent dedup From 2faaf4cc18fcda57ff204cda8bcaba1b4852d085 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 20 Oct 2026 14:49:01 +0200 Subject: [PATCH 2/2] test: cover the --by newline injection on goal and topic approve --- integration/cli_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/integration/cli_test.go b/integration/cli_test.go index 3ade959b..3801be28 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -4406,6 +4406,20 @@ body line session := runEntityCommand("goal", "approve", "Alpha", "--output", "json") Eventually(session).Should(gexec.Exit(1)) }) + + It("security: a newline in --by cannot introduce a sibling frontmatter key", func() { + vaultPath, configPath, cleanup = createTempVaultWithGoals(nil, map[string]string{ + "Alpha": todoFrontmatter, + }) + goalFile := filepath.Join(vaultPath, "Goals", "Alpha.md") + + session := runEntityCommand("goal", "approve", "Alpha", "--by", "line1\nline2") + Eventually(session).Should(gexec.Exit(0)) + + after := readFile(goalFile) + Expect(strings.Count(after, "\napproved_by:")).To(Equal(1)) + Expect(after).NotTo(ContainSubstring("\nline2:")) + }) }) Describe("topic approve", func() { @@ -4659,6 +4673,17 @@ body line session := runEntityCommand("topic", "approve", "Alpha", "--output", "json") Eventually(session).Should(gexec.Exit(1)) }) + + It("security: a newline in --by cannot introduce a sibling frontmatter key", func() { + _, _, topicFile := createTopicVault(todoFrontmatter) + + session := runEntityCommand("topic", "approve", "Alpha", "--by", "line1\nline2") + Eventually(session).Should(gexec.Exit(0)) + + after := readFile(topicFile) + Expect(strings.Count(after, "\napproved_by:")).To(Equal(1)) + Expect(after).NotTo(ContainSubstring("\nline2:")) + }) }) Describe("task answer", func() {