From 82d580128087dec33c5c8d19cf30341b4d774ccd Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 3 May 2026 23:25:22 -0700 Subject: [PATCH 1/3] docs(campaigns): use quest campaign as canonical CLI verb, quest epic as alias Closes #69. internal/quest/epic_cmd.go registers the rename verb with CLIPath: []string{"quest", "campaign"} and CLIAliases: []string{"epic"}, and the comment at lines 25-27 is explicit that 'quest campaign' is the canonical command and 'quest epic' is the alias. CAMPAIGNS.md was treating 'quest epic' as the primary verb, which steered users away from the canonical spelling. Flip the verb naming in docs/CAMPAIGNS.md to mirror CLIPath / CLIAliases: - Line 60: 'quest campaign' is the rename verb, 'quest epic' as alias. - Line 69 (bash example): 'guild quest campaign ...' with a comment noting 'quest epic' works as an alias. - Lines 72-74 (prose): 'quest campaign' canonical, 'quest epic' alias, MCP tool name 'quest_epic' unchanged for backward compat. - Line 84 (out-of-scope section): mention 'quest campaign' rather than 'quest epic'. No code or MCP tool name changes; CLI dual-naming is intentional. --- docs/CAMPAIGNS.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/CAMPAIGNS.md b/docs/CAMPAIGNS.md index 9318fd9..d84a7b5 100644 --- a/docs/CAMPAIGNS.md +++ b/docs/CAMPAIGNS.md @@ -57,8 +57,8 @@ Create a new campaign when: single new campaign would meaningfully clarify reporting. If you would have to explain the campaign in two sentences, it's probably -two campaigns. If you're not sure, reuse — `quest epic` (the rename verb) -exists and is cheap to apply later. +two campaigns. If you're not sure, reuse — `quest campaign` (the rename +verb) exists and is cheap to apply later. ## Setting and changing the campaign on a quest @@ -66,12 +66,14 @@ Set the campaign at quest creation time, or apply it to a batch of existing quests with the rename verb: ```bash -guild quest epic QUEST-1 QUEST-2 ... +guild quest campaign QUEST-1 QUEST-2 ... +# `quest epic` works as an alias for the same operation. ``` -`quest epic` is an alias for the same operation; the underlying field is -`campaign`. Use whichever spelling reads more naturally to you. The MCP -tool is `quest_epic` with `--campaign` and `--epic` both accepted as input. +`quest campaign` is the canonical CLI verb; `quest epic` is registered as +an alias. The underlying field is `campaign` and the MCP tool name stays +`quest_epic` for backward compatibility, with `--campaign` and `--epic` +both accepted as input. ## Out of scope @@ -81,4 +83,5 @@ file a quest for the merge work itself rather than treating it as a docs update. There is also no `guild campaign` verb to manage the campaign list -programmatically. Filings happen via `quest epic` on the affected quests. +programmatically. Filings happen via `quest campaign` on the affected +quests. From 3ab3726a947a374e72411a2558e4dfeed635c38d Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Tue, 5 May 2026 02:36:23 -0700 Subject: [PATCH 2/3] feat(quest): error on `--depends-on ""` instead of silent no-op Closes #47 (acceptance criterion #3 + explicit before=2/after=0 test) Note: --clear-depends-on, the MCP `clear_depends_on` parameter, and the underlying ClearDependsOn flow are already wired in v0.1.0 (see internal/quest/update_cmd.go and the existing ClearDependsOn unit tests in update_test.go). What was missing from the issue's acceptance list: - AC #3: `guild quest update --depends-on ""` was a silent no-op. The trim/skip loop in Update() dropped the empty entry and reported success without touching the dependency list, which is exactly the ambiguity the issue calls out (especially for agents). This PR adds an explicit guard: when DependsOn is non-empty but every entry trims to empty AND no Clear/Replace flag is set, Update() now returns ErrEmptyDependsOn with a message pointing the caller at --clear-depends-on. A mixed slice ([real-id, ""]) keeps the trim/skip behavior so existing callers that incidentally pass an empty entry alongside real IDs are not broken. - AC #4: explicit `before=2 -> after=0` test added as TestUpdate_ClearDependsOn_BeforeTwoAfterZero. The pre-existing TestUpdate_AutoUnblock_ClearDependsOnPath only covers before=1->after=0, so this fills the named acceptance shape. The new error path is covered by TestUpdate_EmptyDependsOn_RedirectsToClear across single-empty / single-whitespace / multiple-empty inputs, and asserts state is untouched on error. Touched files: - internal/quest/types.go: new ErrEmptyDependsOn sentinel + doc comment - internal/quest/update.go: empty-depends-on guard, ~12 lines after the existing conflict check - internal/quest/update_test.go: 2 new tests, 4 new sub-tests `go test ./...`: 1190 tests pass, 20 packages. `gofmt -l` clean. `go vet -tags testing -unsafeptr=false ./...` clean. --- internal/quest/types.go | 7 ++++ internal/quest/update.go | 21 ++++++++++++ internal/quest/update_test.go | 63 +++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) diff --git a/internal/quest/types.go b/internal/quest/types.go index 5e9398d..2fb71c1 100644 --- a/internal/quest/types.go +++ b/internal/quest/types.go @@ -80,6 +80,13 @@ var ( // ReplaceFiles). ErrConflictingUpdate = errors.New("conflicting update: cannot set both append and replace for the same field") + // ErrEmptyDependsOn is returned when an Update call passes only + // empty/whitespace strings as DependsOn entries without also setting + // ClearDependsOn or ReplaceDependsOn. The historical behavior was a + // silent no-op, which made `--depends-on ""` indistinguishable from + // "no change" for callers (especially agents). Issue #47. + ErrEmptyDependsOn = errors.New("depends_on contains only empty values: use --clear-depends-on to remove all dependencies") + // ErrAlreadyDone is returned by Forfeit when the target quest is // status='done'. Forfeit refuses to silently reopen a completed // quest — the caller should explicitly rework or reopen. diff --git a/internal/quest/update.go b/internal/quest/update.go index e325ad1..fd0fb53 100644 --- a/internal/quest/update.go +++ b/internal/quest/update.go @@ -70,6 +70,27 @@ func Update(ctx context.Context, db *sql.DB, projectID, taskID string, params Up return nil, fmt.Errorf("%w: %s", ErrConflictingUpdate, conflicts[0]) } + // Issue #47: callers (especially agents) sometimes pass `--depends-on ""` + // or `depends_on=[""]` expecting it to mean "drop all dependencies". The + // downstream loop trims/skips empty strings and the call silently no-ops, + // which is indistinguishable from "no change". Detect that shape and + // redirect to --clear-depends-on rather than failing silently. The check + // only fires when ClearDependsOn / ReplaceDependsOn are NOT also set, so + // a normal `--depends-on QUEST-1 --depends-on ""` call (an empty entry + // mixed with real ones) keeps the trim-and-skip behavior. + if len(params.DependsOn) > 0 && len(params.ReplaceDependsOn) == 0 && !params.ClearDependsOn { + allEmpty := true + for _, d := range params.DependsOn { + if strings.TrimSpace(d) != "" { + allEmpty = false + break + } + } + if allEmpty { + return nil, ErrEmptyDependsOn + } + } + agent := agentOrDefault(params.Agent) now := time.Now().UTC().Format(time.RFC3339Nano) diff --git a/internal/quest/update_test.go b/internal/quest/update_test.go index 653d028..c4b7451 100644 --- a/internal/quest/update_test.go +++ b/internal/quest/update_test.go @@ -279,3 +279,66 @@ func TestUpdate_ClearAcceptance(t *testing.T) { t.Errorf("acceptance = %v, want empty", got.Acceptance) } } + +// TestUpdate_ClearDependsOn_BeforeTwoAfterZero is the explicit acceptance-criterion +// shape for issue #47: a quest with two deps, --clear-depends-on drops both. +func TestUpdate_ClearDependsOn_BeforeTwoAfterZero(t *testing.T) { + db, pid := newTestDB(t) + ctx := context.Background() + a := mustPost(t, db, pid, PostParams{Subject: "A"}) + c := mustPost(t, db, pid, PostParams{Subject: "C"}) + b := mustPost(t, db, pid, PostParams{Subject: "B", DependsOn: []string{a.ID, c.ID}}) + + before := mustLoad(t, db, pid, b.ID) + if len(before.DependsOn) != 2 { + t.Fatalf("B before clear: deps = %v, want 2", before.DependsOn) + } + + if _, err := Update(ctx, db, pid, b.ID, UpdateParams{ClearDependsOn: true}); err != nil { + t.Fatalf("Update ClearDependsOn: %v", err) + } + + after := mustLoad(t, db, pid, b.ID) + if len(after.DependsOn) != 0 { + t.Errorf("B after clear: deps = %v, want empty", after.DependsOn) + } +} + +// TestUpdate_EmptyDependsOn_RedirectsToClear covers issue #47 acceptance criterion #3: +// `--depends-on ""` (empty/whitespace-only entries) without --clear-depends-on or +// --replace-depends-on must error pointing at --clear-depends-on rather than +// silently no-op'ing. +func TestUpdate_EmptyDependsOn_RedirectsToClear(t *testing.T) { + db, pid := newTestDB(t) + ctx := context.Background() + a := mustPost(t, db, pid, PostParams{Subject: "A"}) + b := mustPost(t, db, pid, PostParams{Subject: "B", DependsOn: []string{a.ID}}) + + cases := []struct { + name string + deps []string + }{ + {"single empty", []string{""}}, + {"single whitespace", []string{" "}}, + {"multiple empty", []string{"", " ", "\t"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := Update(ctx, db, pid, b.ID, UpdateParams{DependsOn: tc.deps}) + if !errors.Is(err, ErrEmptyDependsOn) { + t.Fatalf("Update with deps=%v: err = %v, want ErrEmptyDependsOn", tc.deps, err) + } + // State must be untouched on error. + reloaded := mustLoad(t, db, pid, b.ID) + if len(reloaded.DependsOn) != 1 || reloaded.DependsOn[0] != a.ID { + t.Errorf("B deps after refused empty update = %v, want [%s]", reloaded.DependsOn, a.ID) + } + }) + } + + // Sanity: a mixed slice (real ID + empty) is NOT redirected — the trim/skip + // path still applies for that case so existing callers aren't broken. + if _, err := Update(ctx, db, pid, b.ID, UpdateParams{DependsOn: []string{a.ID, ""}}); err != nil { + t.Fatalf("Update with mixed deps: unexpected error %v", err) + } +} From 6343671444df60ba8bd12b0210a67f567fd036af Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Tue, 5 May 2026 02:49:36 -0700 Subject: [PATCH 3/3] test(quest): use Load(ctx,...) directly in subtest to satisfy contextcheck CI lint failed on update_test.go:332 with: Function `mustLoad` should pass the context parameter (contextcheck) `mustLoad` synthesizes its own context.Background() internally, which contextcheck flags when called from inside a subtest closure that has a parent `ctx` in scope. The other mustLoad call sites in this file are at the test-function top level where contextcheck is lenient; the subtest closure scope tripped the check on the new TestUpdate_EmptyDependsOn_RedirectsToClear cases. Switching that one call to Load(ctx, db, pid, b.ID) directly threads the parent ctx through, which is what contextcheck wants. Behavior is identical (mustLoad is a 5-line wrapper around Load). --- internal/quest/update_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/quest/update_test.go b/internal/quest/update_test.go index c4b7451..0b44e32 100644 --- a/internal/quest/update_test.go +++ b/internal/quest/update_test.go @@ -328,8 +328,13 @@ func TestUpdate_EmptyDependsOn_RedirectsToClear(t *testing.T) { if !errors.Is(err, ErrEmptyDependsOn) { t.Fatalf("Update with deps=%v: err = %v, want ErrEmptyDependsOn", tc.deps, err) } - // State must be untouched on error. - reloaded := mustLoad(t, db, pid, b.ID) + // State must be untouched on error. Use Load(ctx, ...) directly so + // contextcheck sees the parent ctx flow into the subtest closure + // (the mustLoad helper synthesizes a fresh context.Background()). + reloaded, err := Load(ctx, db, pid, b.ID) + if err != nil { + t.Fatalf("Load %s: %v", b.ID, err) + } if len(reloaded.DependsOn) != 1 || reloaded.DependsOn[0] != a.ID { t.Errorf("B deps after refused empty update = %v, want [%s]", reloaded.DependsOn, a.ID) }