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 "" }