diff --git a/internal/hints/engine.go b/internal/hints/engine.go index 5f7c40a..bffae87 100644 --- a/internal/hints/engine.go +++ b/internal/hints/engine.go @@ -8,6 +8,8 @@ import ( "strconv" "sync" "time" + + "github.com/mathomhaus/guild/internal/quest" ) // FollowThroughWindow is how many subsequent events the engine tracks @@ -206,7 +208,11 @@ func (e *Engine) Evaluate(ctx context.Context, ev CallEvent) Fire { if !cr.row.Enabled { continue } - if cr.row.TriggerTool != "*" && cr.row.TriggerTool != ev.Tool { + // Normalize both sides to canonical tool names before comparing so + // backward-compat aliases (e.g. quest_clear → quest_fulfill) trigger + // rules authored against the canonical name. See quest.CanonicalToolName. + if cr.row.TriggerTool != "*" && + quest.CanonicalToolName(cr.row.TriggerTool) != quest.CanonicalToolName(ev.Tool) { continue } if cr.rule.Trigger == nil || !cr.rule.Trigger(e.context, ev) { diff --git a/internal/hints/rules.go b/internal/hints/rules.go index 0bf0020..3b13b6f 100644 --- a/internal/hints/rules.go +++ b/internal/hints/rules.go @@ -79,10 +79,10 @@ func Definitions() []Rule { }, { ID: "no-brief-24h", - TriggerTool: "quest_clear", + TriggerTool: "quest_fulfill", Trigger: triggerNoBrief24h, FollowThrough: followQuestBrief, - Caveat: "Era-aware severity: 💡 hint on MCP, ℹī¸ fyi on Bash CLI (18.7pp gap in ENTRY-29 calibration).", + Caveat: "Era-aware severity: 💡 hint on MCP, ℹī¸ fyi on Bash CLI (18.7pp gap in ENTRY-29 calibration). TriggerTool is canonical quest_fulfill; quest_clear alias is handled by quest.CanonicalToolName in the engine.", }, // ℹī¸ fyi (demote) — 3 rules. @@ -95,10 +95,10 @@ func Definitions() []Rule { }, { ID: "clear-without-report-detail", - TriggerTool: "quest_clear", + TriggerTool: "quest_fulfill", Trigger: triggerClearWithoutReportDetail, FollowThrough: followQuestUpdateOrJournal, - Caveat: "Demoted to fyi; short reports are legitimate for trivial clears.", + Caveat: "Demoted to fyi; short reports are legitimate for trivial clears. TriggerTool is canonical quest_fulfill; quest_clear alias is handled by quest.CanonicalToolName in the engine.", }, { ID: "principle-too-long", diff --git a/internal/mcp/hints_e2e_test.go b/internal/mcp/hints_e2e_test.go index cfd64ed..295888f 100644 --- a/internal/mcp/hints_e2e_test.go +++ b/internal/mcp/hints_e2e_test.go @@ -176,6 +176,72 @@ func TestHints_E2E_SlugQuery_OnlyOnZeroResult(t *testing.T) { } } +// TestHints_E2E_NoBrief24h_BothVerbsFire is the QUEST-137 regression test: +// the no-brief-24h rule must fire on BOTH quest_fulfill (canonical) AND +// quest_clear (backward-compat alias). Each sub-test spins up an isolated +// server so the singleton engine's session state is fresh per run. +func TestHints_E2E_NoBrief24h_BothVerbsFire(t *testing.T) { + longReport := "commit abc123, files: x.go y.go; follow-ups: run integration tests and verify deploy pipeline stays green" + + for _, toolName := range []string{"quest_fulfill", "quest_clear"} { + toolName := toolName // capture range variable for sub-test closure + t.Run(toolName, func(t *testing.T) { + pid := isolateProject(t) + ctx := context.Background() + + db, err := openQuestDB(ctx) + if err != nil { + t.Fatalf("open quest db: %v", err) + } + defer func() { _ = db.Close() }() + + q, err := quest.Post(ctx, db, pid, quest.PostParams{Subject: "brief-regression-" + toolName}) + if err != nil { + t.Fatalf("post: %v", err) + } + + s, err := build() + if err != nil { + t.Fatalf("build: %v", err) + } + _, client, cleanup := connectInMemory(t, s) + defer cleanup() + + if _, err := client.CallTool(ctx, &sdkmcp.CallToolParams{ + Name: "guild_session_start", + Arguments: map[string]any{"project": pid}, + }); err != nil { + t.Fatalf("bootstrap: %v", err) + } + + // Use the tool under test — either quest_fulfill or quest_clear. + // A long report keeps clear-without-report-detail from winning the + // budget slot over no-brief-24h. + res, err := client.CallTool(ctx, &sdkmcp.CallToolParams{ + Name: toolName, + Arguments: map[string]any{ + "quest_id": q.ID, + "report": longReport, + "project": pid, + }, + }) + if err != nil { + t.Fatalf("%s: %v", toolName, err) + } + if res.IsError { + t.Fatalf("%s IsError=true: %s", toolName, textOf(res.Content)) + } + body := textOf(res.Content) + if !strings.Contains(body, "no quest_brief yet") { + t.Errorf("%s: expected no-brief-24h hint in body; got:\n%s", toolName, body) + } + if !strings.Contains(body, "💡") { + t.Errorf("%s: expected 💡 emoji in body; got:\n%s", toolName, body) + } + }) + } +} + // TestHints_E2E_ClearWithoutBrief fires no-brief-24h through a full MCP // round-trip — the canonical migration target of BriefHint. func TestHints_E2E_ClearWithoutBrief(t *testing.T) { diff --git a/internal/quest/aliases.go b/internal/quest/aliases.go new file mode 100644 index 0000000..e75b70b --- /dev/null +++ b/internal/quest/aliases.go @@ -0,0 +1,26 @@ +package quest + +// toolAliases maps backward-compat MCP tool names to their canonical +// successors. Used by the hint engine to normalize trigger_tool matches +// so rules authored against the canonical name fire on alias calls too. +// +// Add entries here whenever a tool is renamed and the old name is kept +// as a backward-compat alias. The map is intentionally small — only +// renames that affect live hint rules need an entry. +var toolAliases = map[string]string{ + // QUEST-106 / LORE-80: quest_clear renamed to quest_fulfill. + // quest_clear stays as a backward-compat MCP alias; hint rules use + // quest_fulfill as the canonical trigger_tool. + "quest_clear": "quest_fulfill", +} + +// CanonicalToolName returns the canonical tool name for name. If name is +// a known backward-compat alias, the canonical successor is returned. +// Otherwise name is returned unchanged. Safe for concurrent use (reads a +// package-level map that is never mutated after init). +func CanonicalToolName(name string) string { + if canonical, ok := toolAliases[name]; ok { + return canonical + } + return name +} diff --git a/internal/storage/migrations/001_init.up.sql b/internal/storage/migrations/001_init.up.sql index 0ce006c..11edb41 100644 --- a/internal/storage/migrations/001_init.up.sql +++ b/internal/storage/migrations/001_init.up.sql @@ -179,7 +179,7 @@ INSERT OR IGNORE INTO hints (rule_id, trigger_tool, severity, template, cooldown ('session-end-without-brief', '*', 'hint', 'session is long (30+ guild calls) with no quest_brief — consider quest_brief("what was done, what''s next") before compact', 10, NULL), ('slug-query', 'lore_appraise', 'hint', 'query looks slug-like — did you mean quest_list or quest_scroll?', 5, NULL), ('journal-outside-accepted', 'quest_journal', 'hint', 'journaling on a quest not accepted this session — accept it first (quest_accept) or file a fresh quest_post', 5, NULL), - ('no-brief-24h', 'quest_clear', 'hint', 'no quest_brief yet this session — consider quest_brief("what was done, what''s next") before compact', 5, '{"mcp":"hint","bash":"fyi"}'), + ('no-brief-24h', 'quest_fulfill', 'hint', 'no quest_brief yet this session — consider quest_brief("what was done, what''s next") before compact', 5, '{"mcp":"hint","bash":"fyi"}'), ('inscribe-without-appraise', 'lore_inscribe', 'fyi', 'no lore_appraise in the last 5 calls — consider appraise(query="â€Ļ", all_projects=true) before inscribing to avoid duplicates', 5, NULL), - ('clear-without-report-detail', 'quest_clear', 'fyi', 'report is short (<20 words) — a richer report (commit hash, files, follow-ups) makes cold-start handoff cleaner', 5, NULL), + ('clear-without-report-detail', 'quest_fulfill', 'fyi', 'report is short (<20 words) — a richer report (commit hash, files, follow-ups) makes cold-start handoff cleaner', 5, NULL), ('principle-too-long', 'lore_inscribe', 'fyi', 'principle exceeds the ≤60-word oath-hygiene target — consider kind=decision for long rationale and re-inscribe a shorter principle', 5, NULL);