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) + } +}