From 7f6469df9c306c02cd11ab07cb0177aae362b627 Mon Sep 17 00:00:00 2001 From: Adam Tal Date: Wed, 16 Sep 2026 20:28:45 +0900 Subject: [PATCH] telemetry: error reports send only the error's kind The TUI's error view reported every classified error to PostHog with the raw error string (raw_message) and the headline. The raw string is err.Error(), which for a file error is the full path, home directory and repo name included, and for a URL error the URL. The headline is raw text on three rules as well. The error_seen board only ever reads the kind, so both fields carried the riskiest content in the project for no use. Report now sends the kind, the severity and the auto-repair flag, and nothing from the error itself. The raw string and headline stay in the local errors.log, where they help. The telemetry call is a parameter of an unexported report(), so the test drives it with a path in the error and checks that none of it reaches the call while the local errors.log still gets it, with no package state swapped. Also pins the command wiring from #112 with a test that parses the package: every telemetry.NewEvent call in a non-test file, however it is written, must be the assignment of a string-literal event in a top-level function's body, followed on the next statement by `defer func() { finishCommand(, err) }()`, in a function whose one result is the named `err error` that call reads. The classifier tests could not see a command put back on `defer te.Finish()`, which would report ok for every failure again. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5.1 --- cmd/kai/telemetry_wiring_test.go | 179 +++++++++++++++++++++++++++++ internal/tui/errors/report.go | 25 +++- internal/tui/errors/report_test.go | 77 +++++++++++++ 3 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 cmd/kai/telemetry_wiring_test.go create mode 100644 internal/tui/errors/report_test.go diff --git a/cmd/kai/telemetry_wiring_test.go b/cmd/kai/telemetry_wiring_test.go new file mode 100644 index 0000000..6ed224d --- /dev/null +++ b/cmd/kai/telemetry_wiring_test.go @@ -0,0 +1,179 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "strconv" + "strings" + "testing" +) + +// Every command that opens a telemetry event closes it through +// finishCommand, so the event reports the error the command returned. +// The classifier has its own tests; this one pins the wiring, which +// they cannot see: a command put back on `defer te.Finish()` would +// report ok for every failure again and nothing else would notice. +// +// The package is parsed, not pattern-matched: every telemetry.NewEvent +// call in this package's non-test files, however it is written, must be +// the assignment +// of a string-literal event in a top-level function's body, followed by +// `defer func() { finishCommand(, err) }()`, in a function +// whose one result is the named `err error` that deferred call reads. +// +// The defer has to be the very next statement, on purpose: anything in +// between could return early, and an event opened but never finished is +// a command that ran and was never counted. +// +// The rule is for commands, which return one error. The TUI's own events +// (gate review, the negativity signal, in internal/tui/views) are opened +// inside a session, set their result at each branch and are not covered. +func TestEveryCommandEventIsFinishedWithItsResult(t *testing.T) { + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, ".", func(fi os.FileInfo) bool { + return !strings.HasSuffix(fi.Name(), "_test.go") + }, 0) + if err != nil { + t.Fatal(err) + } + var seen []string + for _, pkg := range pkgs { + for _, f := range pkg.Files { + // The package is found by its import path, so an alias + // (`tm "…/telemetry"`) is seen too; a file that does not + // import it cannot open an event. + telemetryName := importName(f, telemetryImportPath) + if telemetryName == "" { + continue + } + isNewEvent := func(call *ast.CallExpr) bool { return isCallOn(call, telemetryName, "NewEvent") } + accepted := map[*ast.CallExpr]bool{} + for _, d := range f.Decls { + fd, ok := d.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + stmts := fd.Body.List + for i, st := range stmts { + as, ok := st.(*ast.AssignStmt) + if !ok || len(as.Lhs) != 1 || len(as.Rhs) != 1 { + continue + } + call, ok := as.Rhs[0].(*ast.CallExpr) + if !ok || !isNewEvent(call) { + continue + } + accepted[call] = true + at := fset.Position(as.Pos()) + event, ok := eventLiteral(call) + switch { + case len(call.Args) != 1: + t.Errorf("%s: NewEvent takes the event name and nothing else here, got %d arguments", at, len(call.Args)) + case !ok: + t.Errorf("%s: the event name must be a string literal", at) + } + seen = append(seen, event) + v, ok := as.Lhs[0].(*ast.Ident) + if !ok { + t.Errorf("%s: the event must be assigned to a plain variable", at) + continue + } + if !namedErrResult(fd) { + t.Errorf("%s: %s must return a named `err error`, the value finishCommand reads", at, fd.Name.Name) + } + if i+1 >= len(stmts) || !isFinishDefer(stmts[i+1], v.Name) { + t.Errorf("%s: the %q event is not closed by `defer func() { finishCommand(%s, err) }()` as the very next statement (nothing may come between: it could return early)", at, event, v.Name) + } + } + } + ast.Inspect(f, func(n ast.Node) bool { + if call, ok := n.(*ast.CallExpr); ok && isNewEvent(call) && !accepted[call] { + t.Errorf("%s: a telemetry event must be opened as `v := telemetry.NewEvent(...)` as a top-level statement of the command's body — not inside a block, a closure or a larger expression, where an early return could leave it unfinished", fset.Position(call.Pos())) + } + return true + }) + } + } + // A sanity floor on the scan itself: the ten commands are there. + if len(seen) < 10 { + t.Fatalf("expected at least the ten command events, found %d: %v", len(seen), seen) + } +} + +const telemetryImportPath = "github.com/kaicontext/kai-engine/telemetry" + +// importName is the name path is imported under in f: its alias, else +// the last element of the path; "" when f does not import it. +func importName(f *ast.File, path string) string { + for _, imp := range f.Imports { + p, err := strconv.Unquote(imp.Path.Value) + if err != nil || p != path { + continue + } + if imp.Name != nil { + return imp.Name.Name + } + return p[strings.LastIndex(p, "/")+1:] + } + return "" +} + +// isCallOn is `pkg.fn(...)`. +func isCallOn(call *ast.CallExpr, pkg, fn string) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != fn { + return false + } + x, ok := sel.X.(*ast.Ident) + return ok && x.Name == pkg +} + +func eventLiteral(call *ast.CallExpr) (string, bool) { + if len(call.Args) != 1 { + return "", false + } + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return "", false + } + s, err := strconv.Unquote(lit.Value) + return s, err == nil +} + +func namedErrResult(fd *ast.FuncDecl) bool { + rs := fd.Type.Results + if rs == nil || len(rs.List) != 1 || len(rs.List[0].Names) != 1 || rs.List[0].Names[0].Name != "err" { + return false + } + id, ok := rs.List[0].Type.(*ast.Ident) + return ok && id.Name == "error" +} + +// isFinishDefer is `defer func() { finishCommand(v, err) }()`. +func isFinishDefer(st ast.Stmt, v string) bool { + ds, ok := st.(*ast.DeferStmt) + if !ok || len(ds.Call.Args) != 0 { + return false + } + lit, ok := ds.Call.Fun.(*ast.FuncLit) + if !ok || len(lit.Body.List) != 1 { + return false + } + es, ok := lit.Body.List[0].(*ast.ExprStmt) + if !ok { + return false + } + call, ok := es.X.(*ast.CallExpr) + if !ok || len(call.Args) != 2 { + return false + } + fn, ok := call.Fun.(*ast.Ident) + if !ok || fn.Name != "finishCommand" { + return false + } + a, ok1 := call.Args[0].(*ast.Ident) + b, ok2 := call.Args[1].(*ast.Ident) + return ok1 && ok2 && a.Name == v && b.Name == "err" +} diff --git a/internal/tui/errors/report.go b/internal/tui/errors/report.go index 4adb86c..c31071d 100644 --- a/internal/tui/errors/report.go +++ b/internal/tui/errors/report.go @@ -23,17 +23,34 @@ import ( // per package telemetry's IsEnabled()). Local log always runs. // Both are best-effort — Report never returns errors and never // blocks the caller. +// +// What goes to PostHog is the kind, the severity and whether the +// auto-repair worked — nothing else. LogContext is err.Error(), +// which for a file error is the full path and for a URL error the +// URL; and Headline is raw text on three rules (a provider's cap +// message, the build-regression lede, the multiroot first line). +// Both stay in the local log, where they are useful, and are never +// sent. The error_seen board groups on kind alone. func Report(workspace string, ue UserError, autoRepaired bool) { + report(workspace, ue, autoRepaired, telemetry.ReportError) +} + +// report is Report with the telemetry call passed in, so a test can +// see what would be sent without swapping package state. +func report(workspace string, ue UserError, autoRepaired bool, send func(kind, headline, raw string, autoRepaired bool, severity string, ctx map[string]any)) { if ue.Kind == "" || ue.Kind == "none" { return } LogLocal(workspace, ue, autoRepaired) - telemetry.ReportError( + // Context is not sent either. Nothing populates it today; if a + // whitelisted context is ever added, it has to be admitted here on + // purpose, not forwarded by default. + send( ue.Kind, - ue.Headline, - ue.LogContext, + "", // headline: may carry raw text; not sent + "", // raw message: err.Error(); not sent autoRepaired, severityName(ue.Severity), - ue.Context, + nil, ) } diff --git a/internal/tui/errors/report_test.go b/internal/tui/errors/report_test.go new file mode 100644 index 0000000..0c79307 --- /dev/null +++ b/internal/tui/errors/report_test.go @@ -0,0 +1,77 @@ +package errors + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// Report sends the kind, the severity and the auto-repair flag, and +// nothing from the error itself: LogContext is err.Error() and Headline +// can be raw text, and either can hold a path, a URL or a message. The +// local errors.log is the other half of Report and still gets the raw +// text, which is where it is useful. +func TestReportSendsOnlyTheKind(t *testing.T) { + type sent struct { + kind, headline, raw, severity string + repaired bool + ctx map[string]any + } + var got []sent + record := func(kind, headline, raw string, repaired bool, severity string, ctx map[string]any) { + got = append(got, sent{kind, headline, raw, severity, repaired, ctx}) + } + + // A workspace with a .kai dir, so the local log has somewhere to go. + ws := t.TempDir() + if err := os.Mkdir(filepath.Join(ws, ".kai"), 0o755); err != nil { + t.Fatal(err) + } + secret := "/Users/someone/acme-payroll/src/salaries.go" + // Two classes: the fallback, whose headline is fixed text, and a + // gate rule whose headline IS the error's first line — the kind of + // class the raw text used to reach PostHog through. + fallback := Classify(errors.New("open " + secret + ": permission denied")) + fallback.Headline = "Couldn't read " + secret + gate := Classify(errors.New("the change broke the build: " + secret + "\n" + secret + ":12:3: undefined: salary")) + if gate.Kind != "gate.build_regression" || !strings.Contains(gate.Headline, secret) { + t.Fatalf("the build-gate rule must put the first line in the headline, got %+v", gate) + } + for _, ue := range []UserError{fallback, gate} { + got = nil + report(ws, ue, false, record) + if len(got) != 1 { + t.Fatalf("%s: want one telemetry call, got %d", ue.Kind, len(got)) + } + s := got[0] + if s.kind != ue.Kind || s.severity != severityName(ue.Severity) || s.repaired { + t.Errorf("kind/severity/repaired = %q/%q/%v, want %q/%q/false", s.kind, s.severity, s.repaired, ue.Kind, severityName(ue.Severity)) + } + if s.headline != "" || s.raw != "" || s.ctx != nil { + t.Errorf("%s: headline/raw/ctx must be empty, got %q/%q/%v", ue.Kind, s.headline, s.raw, s.ctx) + } + for _, v := range []string{s.kind, s.headline, s.raw, s.severity} { + if strings.Contains(v, "acme") || strings.Contains(v, "salaries") { + t.Fatalf("%s: the error's text reached telemetry: %+v", ue.Kind, s) + } + } + } + + local, err := os.ReadFile(filepath.Join(ws, ".kai", "errors.log")) + if err != nil { + t.Fatalf("the local errors.log must still be written: %v", err) + } + if strings.Count(string(local), secret) < 2 || !strings.Contains(string(local), "gate.build_regression") || !strings.Contains(string(local), fallback.Kind) { + t.Fatalf("the local log keeps the raw text and the kind of both errors, got: %s", local) + } + + // Nothing to report for a nil or "none" classification. + got = nil + report("", UserError{Kind: "none"}, false, record) + report("", UserError{}, false, record) + if len(got) != 0 { + t.Fatalf("a non-error must not be reported: %+v", got) + } +}