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. 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..0b44e32 100644 --- a/internal/quest/update_test.go +++ b/internal/quest/update_test.go @@ -279,3 +279,71 @@ 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. 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) + } + }) + } + + // 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) + } +}