feat(quest): error on --depends-on "" instead of silent no-op - #73
Merged
kunallanjewar merged 6 commits intoAug 18, 2026
Merged
Conversation
… as alias Closes mathomhaus#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.
Closes mathomhaus#47 (acceptance criterion mathomhaus#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 mathomhaus#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 mathomhaus#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.
github-actions
Bot
added
area: quest
…check 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).
The error told the caller to use --clear-depends-on, which is the CLI spelling of a field the registry calls clear_depends_on. Most callers hit this over MCP, where the flag form does not exist, so the recovery hint pointed at something they cannot set. Name the field first and keep the CLI spelling as the parenthetical.
kunallanjewar
marked this pull request as ready for review
August 18, 2026 16:59
Contributor
|
Picking this up so it can land. What I did on the branch:
The design is right. A silent no-op on Marked ready for review. Thanks for the writeup in the description, the state-of-play section made this quick to verify. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #47
State of play before this PR
The
--clear-depends-onflag and its MCP equivalent (clear_depends_on=true) are already shipped in v0.1.0 (commitad72f16).internal/quest/update_cmd.goregisters the cobra flag and threads it through toUpdateParams.ClearDependsOn, andinternal/quest/update.godoes the right thing when it's set. Acceptance criteria #1, #2, and #5 from the issue are met by that existing code.What's not yet handled is acceptance criterion #3 — the empty-string ambiguity:
The trim/skip loop at the top of
Update()was dropping""entries and reporting success without touching the dependency list. From an agent's view, "I tried to clear deps and the call succeeded" was indistinguishable from "I made no change."What this PR does
1. Explicit error for
--depends-on ""(internal/quest/update.go)Added a guard right after the existing append-vs-replace conflict check: when
DependsOnis non-empty but every entry trims to empty AND noClear*/Replace*flag is set,Update()now returns the newErrEmptyDependsOn:A mixed slice (
["QUEST-1", ""]) deliberately keeps the trim/skip behavior so existing callers that incidentally pass an empty entry alongside a real ID are not broken. Only the all-empty shape gets redirected.2. Explicit before=2 / after=0 test for
--clear-depends-on(AC #4)The existing
TestUpdate_AutoUnblock_ClearDependsOnPathcovers the auto-unblock side-effect forbefore=1 -> after=0. This PR addsTestUpdate_ClearDependsOn_BeforeTwoAfterZeroas the literal shape called out in the acceptance criteria — a quest with two deps,--clear-depends-ondrops both.3. Tests for the new error path
TestUpdate_EmptyDependsOn_RedirectsToClearruns the new guard against three inputs (single empty, single whitespace, multiple empty), asserts the call returnsErrEmptyDependsOnviaerrors.Is, and verifies the dependency list is untouched on error. A trailing assertion confirms the mixed-slice case still goes through.Files
internal/quest/types.go— newErrEmptyDependsOnsentinel with doc comment citing [feature] quest update: support clearing all dependencies via --clear-depends-on flag #47internal/quest/update.go— ~12-line guard after the existing conflict check (new logic only; existing flow unchanged)internal/quest/update_test.go— two newTest*functions, four sub-tests totalVerification
go test ./...: 1190 tests pass across 20 packagesgofmt -l ./internal/quest/...: cleango vet -tags testing -unsafeptr=false ./internal/quest/...: cleango build ./...: cleanOut of scope
Not changing the
--depends-on/--replace-depends-oninteraction beyond the empty-string redirect. The existing conflict logic at line 63 already errors when both append and replace forms are set with non-empty content; that path is unchanged.