From be4f83c7667b135487ca16fef6d66ebe51886673 Mon Sep 17 00:00:00 2001 From: Smaine Kahlouch Date: Wed, 23 Sep 2026 22:13:39 +0200 Subject: [PATCH] feat(serve): name the investigation tools the loop runs without, once at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lore investigate` tells the user which evidence backends are off — "note: running without metrics (query_metrics), logs (query_logs), ..." — but `lore serve` never did. serve announces at Info everything it turned ON (the ledger, the rate limit, the coalescer, the debounce, the watcher, the poller), so an in-cluster install missing its evidence backends looked identical in the logs to one that has them all. The `minimal` Helm profile wires neither metrics.url nor logs.url, and was indistinguishable from a misconfigured full one. The notice reuses disabledTools, the CLI's pure function over config, so the two commands cannot disagree about what counts as off. Structured rather than the CLI's prose, to match the surrounding startup lines: INFO running without tools=["metrics (query_metrics)","logs (query_logs)"] The issue's open questions, decided here: - Info, not Warn. It fires on every boot of a deliberately-minimal install, and a warning that fires on purpose is tuned out within a day. - It fires even when the under-configuration is intentional. The log is where someone looks when investigations seem thin, and an Info line costs nothing. - Scope stays what disabledTools covers — metrics, logs, the catalog — so the CLI note and this one name the same set. Widening it to flows, cloud and the forge is a change to disabledTools, and then both commands get it. Pinned the way the recall-decay warning is: RunServe must call disabledTools directly in its own body (not in a closure the incident path runs), exactly once, bind the result, and pass it to .Info in the same statement. The shared assertion matched callees by their *Warning suffix, which this notice deliberately lacks, so it now matches through a name-agnostic callName and warningCallName is that plus the suffix filter. Existing pins are unchanged. Fixes #467 --- internal/app/serve.go | 11 ++++ internal/app/serve_disabled_tools_test.go | 73 +++++++++++++++++++++++ internal/app/serve_guard_test.go | 11 ++-- internal/app/warnings_wired_test.go | 19 ++++-- 4 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 internal/app/serve_disabled_tools_test.go diff --git a/internal/app/serve.go b/internal/app/serve.go index af9bd600..2261999a 100644 --- a/internal/app/serve.go +++ b/internal/app/serve.go @@ -354,6 +354,17 @@ func RunServe(version string, args []string) error { if msg := RecallDecayWarning(cfg, resolveCapable); msg != "" { log.Warn(msg, "resolve_capable_source", resolveCapable) } + // What the loop will run WITHOUT — named once, here, for the same reason the + // warning above is raised once: the condition is pure config. serve announces at + // Info everything it turned on, so an install missing its evidence backends (the + // `minimal` profile wires neither metrics.url nor logs.url) looked identical in + // the logs to one that has them all. Info, not Warn: a deliberately-minimal + // install boots this way every time, and a warning that fires on purpose is + // tuned out. The same helper as `lore investigate`'s stderr note, so the two + // commands cannot disagree about what counts as off (#467). + if off := disabledTools(cfg); len(off) > 0 { + log.Info("running without", "tools", off) + } // The queue (not alertEnq, which may be the coalescer) is wired as the // pipeline's Canceller; the pipeline only calls it when // triggers.incidents.cancel_queued_on_resolve is on. diff --git a/internal/app/serve_disabled_tools_test.go b/internal/app/serve_disabled_tools_test.go new file mode 100644 index 00000000..a65e6a51 --- /dev/null +++ b/internal/app/serve_disabled_tools_test.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) + +// TestDisabledToolsNoticeIsEmittedAtServeStartup pins #467: `lore serve` names the +// investigation tools it runs WITHOUT, once, on the startup path — exactly as +// `lore investigate` already does on stderr. serve announces at Info everything it +// turned ON (the ledger, the coalescer, the debounce, the watcher, ...), so an +// install missing its evidence backends looked identical in the logs to one that has +// them all, and the `minimal` Helm profile — no metrics.url, no logs.url — was +// indistinguishable from a misconfigured full one. +// +// The notice reuses disabledTools, the CLI's pure function over config, so the two +// commands can never disagree about what counts as "off". It is pinned the way the +// recall-decay warning is (TestRecallDecayWarningIsEmittedOnceAtStartup): RunServe +// must call it directly in its own body — not inside a closure that runs per alert — +// exactly once, bind the result to a real variable, and pass that variable to a +// .Info(...) in the same statement. Info, not Warn: it fires on every boot of a +// deliberately-minimal install, and a warning that fires on purpose is tuned out. +// +// Unlike the recall-decay pin, other callers are NOT an error: the CLI is one. +func TestDisabledToolsNoticeIsEmittedAtServeStartup(t *testing.T) { + const guarded, caller, raiser, file = "disabledTools", "RunServe", "Info", "serve.go" + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, file, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", file, err) + } + calls := 0 + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil || fn.Name.Name != caller { + continue + } + // stack tracks the ancestry of the visited node, so a call site is judged by + // WHERE it sits — a FuncLit inside RunServe is a closure handed to the incident + // path, not startup. + var stack []ast.Node + ast.Inspect(fn.Body, func(n ast.Node) bool { + if n == nil { + stack = stack[:len(stack)-1] + return false + } + stack = append(stack, n) + call, ok := n.(*ast.CallExpr) + if !ok || callName(call) != guarded { + return true + } + calls++ + for _, anc := range stack[:len(stack)-1] { + if _, isLit := anc.(*ast.FuncLit); isLit { + t.Errorf("%s: %s calls %s inside a function literal — that runs whenever the "+ + "closure runs, not once at startup", file, caller, guarded) + return true + } + } + assertWarningIsRaised(t, file, outermostStmt(stack), guarded, raiser) + return true + }) + } + if calls != 1 { + t.Fatalf("%s must call %s exactly once (got %d) — otherwise the startup notice is "+ + "either absent or duplicated", caller, guarded, calls) + } +} diff --git a/internal/app/serve_guard_test.go b/internal/app/serve_guard_test.go index 0bbd907f..d0ba82a8 100644 --- a/internal/app/serve_guard_test.go +++ b/internal/app/serve_guard_test.go @@ -201,10 +201,13 @@ func outermostStmt(stack []ast.Node) ast.Stmt { // so `_ = f()` and a bare `f()` are the only ways to discard the result, and both are // caught here. // -// The call is matched through warningCallName, so BOTH spellings count: a -// same-package identifier (WebhookAuthWarning) and a qualified selector +// The call is matched through callName, so BOTH spellings count: a same-package +// identifier (WebhookAuthWarning) and a qualified selector // (config.ChatWithoutCaptureWarning). Matching only the first is what let the -// package-app guard this replaced miss every cross-package warning. +// package-app guard this replaced miss every cross-package warning. callName rather +// than warningCallName because `guarded` is compared by full name anyway, and the +// disabledTools startup notice — an Info line, so no *Warning suffix — is pinned +// through this same assertion. func assertWarningIsRaised(t *testing.T, file string, stmt ast.Stmt, guarded, raiser string) { t.Helper() if stmt == nil { @@ -218,7 +221,7 @@ func assertWarningIsRaised(t *testing.T, file string, stmt ast.Stmt, guarded, ra return true } for i, rhs := range as.Rhs { - if warningCallName(rhs) != guarded { + if callName(rhs) != guarded { continue } if i < len(as.Lhs) { diff --git a/internal/app/warnings_wired_test.go b/internal/app/warnings_wired_test.go index c44c9ba5..d385e587 100644 --- a/internal/app/warnings_wired_test.go +++ b/internal/app/warnings_wired_test.go @@ -372,19 +372,26 @@ func warningCallsIn(fn *ast.FuncDecl) map[string][]ast.Stmt { // (WebhookAuthWarning in serve.go) and a qualified selector // (config.ChatWithoutCaptureWarning). func warningCallName(n ast.Node) string { + if name := callName(n); strings.HasSuffix(name, warningSuffix) { + return name + } + return "" +} + +// callName reports the function n calls — a bare identifier's name or a qualified +// selector's — or "" if n is not a call. It is the name-agnostic half of +// warningCallName, shared with pins on startup notices that are deliberately NOT +// warnings (disabledTools, an Info line) and so carry no suffix to match on. +func callName(n ast.Node) string { call, ok := n.(*ast.CallExpr) if !ok { return "" } switch fn := call.Fun.(type) { case *ast.Ident: - if strings.HasSuffix(fn.Name, warningSuffix) { - return fn.Name - } + return fn.Name case *ast.SelectorExpr: - if strings.HasSuffix(fn.Sel.Name, warningSuffix) { - return fn.Sel.Name - } + return fn.Sel.Name } return "" }