From baedd9157050b9a04b1355fd851dbe77244d6412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Nikolaus?= Date: Sat, 27 Jun 2026 17:22:02 +0200 Subject: [PATCH] fix(vet,debuglog): address review follow-ups on debug-capabilities Post-merge fixes from the branch-diff review of the fas vet + FAS_LOG payload-logging work. debuglog: - gc now removes only files matching the generated log-file name pattern (timestamp + 8 hex), so pointing FAS_LOG at a directory holding other *.json never deletes them (data-loss footgun). - shortID rand-failure fallback widened to 8 chars so gc still matches it. - unexport the test-only GC/LogFiles and drop the Dir() accessor; tests use the package-private forms. cmd/fas: - collectDiagErrors recurses into errors.Join children before the tree-descending errors.As, so `fas vet --format json|sarif` no longer duplicates the first diagnostic. - record the preprocessed input envelope in the debug log (the advertised `input` field was never populated). - vet rejects stray positional arguments instead of silently ignoring them. - vet routes capability errors through the format-aware renderer and emits SARIF on success under --format sarif (was plain text). - vet success summary writes to stdout instead of the errorf stderr-diagnostic helper; drop the inert --color flag from vet. - extract registerCommonFlags shared by eval/explain/vet. - document FAS_LOG payload sensitivity in the usage text. Regression tests added for gc foreign-json preservation, the join de-duplication, vet positional-arg rejection, and vet SARIF output. --- cmd/fas/main.go | 146 ++++++++++++++--------------- cmd/fas/main_test.go | 69 ++++++++++++++ internal/debuglog/debuglog.go | 24 ++--- internal/debuglog/debuglog_test.go | 39 ++++++-- 4 files changed, 172 insertions(+), 106 deletions(-) diff --git a/cmd/fas/main.go b/cmd/fas/main.go index fc0e247..ef3a357 100644 --- a/cmd/fas/main.go +++ b/cmd/fas/main.go @@ -152,7 +152,10 @@ func run(stdin io.Reader, stdout, stderr io.Writer, args []string) int { // state into the next invocation. evaluator.SetExplainEnabled(opts.explain.set) - response, matches, diags, err := evaluate(ad, raw, globalRules, projectRules, opts.failClosed) + response, matches, diags, input, err := evaluate(ad, raw, globalRules, projectRules, opts.failClosed) + if input != nil { + rec.SetInput(input) + } if err != nil { errorf(stderr, "render response: %v\n", err) return exitWithLog(rec, 1) @@ -207,30 +210,15 @@ func runExplain(stdin io.Reader, stdout, stderr io.Writer, args []string) int { fs.SetOutput(stderr) fs.Usage = func() { writeUsage(stderr) } - defaultGlobal, err := defaultGlobalConfigDir() - if err != nil { - defaultGlobal = defaultGlobalRulesSubpath - } - harness := "claude" - projectConfig := defaultProjectRulesDir - globalConfig := defaultGlobal - fs.StringVar(&harness, "harness", harness, - "vendor harness whose hook protocol to speak (e.g. claude)") - fs.StringVar(&projectConfig, "config", projectConfig, - "path to the project rules directory") - fs.StringVar(&globalConfig, "global-config", globalConfig, - "path to the user-global rules directory") - var format formatFlag + common := registerCommonFlags(fs) var color colorFlag - fs.Var(&format, "format", - "diagnostic output format: text|json|sarif (default text)") fs.Var(&color, "color", "color mode for text diagnostics: auto|always|never (default auto)") if err := fs.Parse(rest); err != nil { return 2 } - resolvedFormat, ferr := resolveFormat(format, os.Getenv("FAS_FORMAT")) + resolvedFormat, ferr := resolveFormat(common.format, os.Getenv("FAS_FORMAT")) if ferr != nil { errorln(stderr, ferr) return 2 @@ -241,19 +229,19 @@ func runExplain(stdin io.Reader, stdout, stderr io.Writer, args []string) int { return 2 } - ad, ok := selectAdapter(harness) + ad, ok := selectAdapter(common.harness) if !ok { errorf(stderr, "unknown harness %q; supported: %s\n", - harness, strings.Join(supportedHarnesses(), ", ")) + common.harness, strings.Join(supportedHarnesses(), ", ")) return 2 } - globalRules, err := loadRulesDir(globalConfig) + globalRules, err := loadRulesDir(common.globalConfig) if err != nil { errorln(stderr, err) return 2 } - projectRules, err := loadRulesDir(projectConfig) + projectRules, err := loadRulesDir(common.projectConfig) if err != nil { errorln(stderr, err) return 2 @@ -664,37 +652,48 @@ func paletteFor(mode colorMode, tty bool, noColorEnv string) diag.Palette { // parseFlags reads args into a cliOptions. The returned bool is true when the // user asked for help; callers should print usage and exit 0. -func parseFlags(args []string, stderr io.Writer) (cliOptions, bool, error) { - fs := flag.NewFlagSet("fas eval", flag.ContinueOnError) - fs.SetOutput(stderr) - fs.Usage = func() { writeUsage(stderr) } +type commonFlags struct { + harness string + projectConfig string + globalConfig string + format formatFlag +} +func registerCommonFlags(fs *flag.FlagSet) *commonFlags { defaultGlobal, err := defaultGlobalConfigDir() if err != nil { // Fall back to a bare relative path so flag parsing still works even // when HOME is unset; loadRulesDir will treat it as "does not exist". defaultGlobal = defaultGlobalRulesSubpath } - - opts := cliOptions{ + cf := &commonFlags{ harness: "claude", projectConfig: defaultProjectRulesDir, globalConfig: defaultGlobal, } - fs.StringVar(&opts.harness, "harness", opts.harness, + fs.StringVar(&cf.harness, "harness", cf.harness, "vendor harness whose hook protocol to speak (e.g. claude)") - fs.StringVar(&opts.projectConfig, "config", opts.projectConfig, + fs.StringVar(&cf.projectConfig, "config", cf.projectConfig, "path to the project rules directory") - fs.StringVar(&opts.globalConfig, "global-config", opts.globalConfig, + fs.StringVar(&cf.globalConfig, "global-config", cf.globalConfig, "path to the user-global rules directory") + fs.Var(&cf.format, "format", + "diagnostic output format: text|json|sarif (default text)") + return cf +} + +func parseFlags(args []string, stderr io.Writer) (cliOptions, bool, error) { + fs := flag.NewFlagSet("fas eval", flag.ContinueOnError) + fs.SetOutput(stderr) + fs.Usage = func() { writeUsage(stderr) } + + common := registerCommonFlags(fs) + var opts cliOptions fs.BoolVar(&opts.failClosed, "fail-closed", false, "on engine error, emit a Blocking envelope instead of Allowing") fs.Var(&opts.explain, "explain", "emit diagnostics to stderr: fired|missed|both (default missed when bare)") - var format formatFlag var color colorFlag - fs.Var(&format, "format", - "diagnostic output format: text|json|sarif (default text)") fs.Var(&color, "color", "color mode for text diagnostics: auto|always|never (default auto)") @@ -704,9 +703,12 @@ func parseFlags(args []string, stderr io.Writer) (cliOptions, bool, error) { } return opts, false, err } + opts.harness = common.harness + opts.projectConfig = common.projectConfig + opts.globalConfig = common.globalConfig opts.format = formatText opts.color = colorAuto - f, rerr := resolveFormat(format, os.Getenv("FAS_FORMAT")) + f, rerr := resolveFormat(common.format, os.Getenv("FAS_FORMAT")) if rerr != nil { errorln(stderr, rerr) return opts, false, rerr @@ -798,31 +800,31 @@ func evaluate( raw []byte, globalRules, projectRules []config.Rule, failClosed bool, -) ([]byte, []evaluator.Match, []diag.Diagnostic, error) { +) ([]byte, []evaluator.Match, []diag.Diagnostic, *envelope.Input, error) { input, hookEventName, engineErr := prepareInput(ad, raw) if engineErr != nil { out := fallbackEnvelope(engineErr, failClosed) resp, err := ad.RenderOutput(out, hookEventName) - return resp, nil, nil, err + return resp, nil, nil, nil, err } cueInput, err := encodeInput(input) if err != nil { out := fallbackEnvelope(err, failClosed) resp, rerr := ad.RenderOutput(out, hookEventName) - return resp, nil, nil, rerr + return resp, nil, nil, input, rerr } matches, diags, err := pipeline.EvaluatePhases(globalRules, projectRules, cueInput) if err != nil { out := fallbackEnvelope(err, failClosed) resp, rerr := ad.RenderOutput(out, hookEventName) - return resp, nil, nil, rerr + return resp, nil, nil, input, rerr } out := synthesis.Synthesize(matches, defaultSizeBudget) resp, err := ad.RenderOutput(out, hookEventName) - return resp, matches, diags, err + return resp, matches, diags, input, err } // prepareInput runs the adapter parse and preprocessor. It returns the @@ -1129,7 +1131,11 @@ Environment: directory path to enable; "1" or "true" uses the default (~/.local/state/fas/logs/). Each invocation writes one JSON file recording raw - input, loaded rules, matches, and rendered output. + input, preprocessed input, loaded rules, matches, + and rendered output. These payloads are written + unredacted (mode 0600) and may contain secrets, + prompts, or other sensitive data — point FAS_LOG + only at a trusted directory. FAS_LOG_TTL Max age for debug log files (default: "1h"). Files older than this are garbage-collected at the start of each invocation. Accepts Go duration @@ -1148,49 +1154,30 @@ func runVet(stdout, stderr io.Writer, args []string) int { fs.SetOutput(stderr) fs.Usage = func() { writeUsage(stderr) } - defaultGlobal, err := defaultGlobalConfigDir() - if err != nil { - defaultGlobal = defaultGlobalRulesSubpath - } - - harness := "claude" - projectConfig := defaultProjectRulesDir - globalConfig := defaultGlobal - fs.StringVar(&harness, "harness", harness, - "vendor harness whose hook protocol to speak (e.g. claude)") - fs.StringVar(&projectConfig, "config", projectConfig, - "path to the project rules directory") - fs.StringVar(&globalConfig, "global-config", globalConfig, - "path to the user-global rules directory") - var format formatFlag - var color colorFlag - fs.Var(&format, "format", - "diagnostic output format: text|json|sarif (default text)") - fs.Var(&color, "color", - "color mode for text diagnostics: auto|always|never (default auto)") + common := registerCommonFlags(fs) if err := fs.Parse(args); err != nil { return 2 } - resolvedFormat, ferr := resolveFormat(format, os.Getenv("FAS_FORMAT")) - if ferr != nil { - errorln(stderr, ferr) + if fs.NArg() != 0 { + errorf(stderr, "fas vet takes no positional arguments; got %q\n", fs.Arg(0)) return 2 } - if _, cerr := resolveColor(color, os.Getenv("FAS_COLOR"), os.Getenv("NO_COLOR")); cerr != nil { - errorln(stderr, cerr) + resolvedFormat, ferr := resolveFormat(common.format, os.Getenv("FAS_FORMAT")) + if ferr != nil { + errorln(stderr, ferr) return 2 } - ad, ok := selectAdapter(harness) + ad, ok := selectAdapter(common.harness) if !ok { errorf(stderr, "unknown harness %q; supported: %s\n", - harness, strings.Join(supportedHarnesses(), ", ")) + common.harness, strings.Join(supportedHarnesses(), ", ")) return 2 } - globalRules, globalErr := loadRulesDir(globalConfig) - projectRules, projectErr := loadRulesDir(projectConfig) + globalRules, globalErr := loadRulesDir(common.globalConfig) + projectRules, projectErr := loadRulesDir(common.projectConfig) loadErr := errors.Join(globalErr, projectErr) if loadErr != nil { @@ -1199,7 +1186,7 @@ func runVet(stdout, stderr io.Writer, args []string) int { } if err := checkAdapterCapabilities(ad, globalRules, projectRules); err != nil { - errorln(stderr, err) + renderVetErrors(stderr, err, resolvedFormat) return 1 } @@ -1210,14 +1197,16 @@ func runVet(stdout, stderr io.Writer, args []string) int { switch resolvedFormat { case formatJSON: renderVetSummaryJSON(stdout, globalIDs, projectIDs) + case formatSARIF: + _, _ = stdout.Write(diag.RenderSARIF(nil)) default: - errorf(stdout, "ok: %d rules loaded (global: %d, project: %d)\n", + _, _ = fmt.Fprintf(stdout, "ok: %d rules loaded (global: %d, project: %d)\n", total, len(globalIDs), len(projectIDs)) for _, id := range globalIDs { - errorf(stdout, " global: %s\n", id) + _, _ = fmt.Fprintf(stdout, " global: %s\n", id) } for _, id := range projectIDs { - errorf(stdout, " project: %s\n", id) + _, _ = fmt.Fprintf(stdout, " project: %s\n", id) } } return 0 @@ -1251,14 +1240,15 @@ func collectDiagErrors(err error, dst *[]diag.Diagnostic) { if err == nil { return } - var de *diag.DiagError - if errors.As(err, &de) { - *dst = append(*dst, de.D) - } if joined, ok := err.(interface{ Unwrap() []error }); ok { for _, child := range joined.Unwrap() { collectDiagErrors(child, dst) } + return + } + var de *diag.DiagError + if errors.As(err, &de) { + *dst = append(*dst, de.D) } } diff --git a/cmd/fas/main_test.go b/cmd/fas/main_test.go index 5038c04..a9ef46d 100644 --- a/cmd/fas/main_test.go +++ b/cmd/fas/main_test.go @@ -3,10 +3,13 @@ package main import ( "bytes" "encoding/json" + "errors" "os" "path/filepath" "strings" "testing" + + "github.com/srnnkls/fas/internal/diag" ) // ----------------------------------------------------------------------------- @@ -3088,6 +3091,9 @@ func TestRun_FasLog_WritesLogFile(t *testing.T) { if _, ok := entry["raw_input"]; !ok { t.Error("log entry missing raw_input") } + if _, ok := entry["input"]; !ok { + t.Error("log entry missing input (preprocessed envelope)") + } if _, ok := entry["output"]; !ok { t.Error("log entry missing output") } @@ -3157,3 +3163,66 @@ func TestRun_FasLog_NonFatalOnBadDir(t *testing.T) { t.Errorf("expected FAS_LOG warning on stderr, got %q", res.stderr) } } + +func TestCollectDiagErrors_JoinNoDuplicate(t *testing.T) { + g := &diag.DiagError{D: diag.Diagnostic{Code: "E001"}} + p := &diag.DiagError{D: diag.Diagnostic{Code: "E002"}} + + var diags []diag.Diagnostic + collectDiagErrors(errors.Join(g, p), &diags) + + if len(diags) != 2 { + t.Fatalf("got %d diagnostics, want 2 (first must not be double-counted)", len(diags)) + } + if diags[0].Code != "E001" || diags[1].Code != "E002" { + t.Errorf("codes=%q,%q want E001,E002", diags[0].Code, diags[1].Code) + } +} + +func TestRun_Vet_PositionalArg_ExitTwo(t *testing.T) { + projectDir := emptyRulesDir(t) + globalDir := emptyRulesDir(t) + + var stdout, stderr bytes.Buffer + exit := run(nil, &stdout, &stderr, + []string{"vet", "./rules", + "--config", projectDir, + "--global-config", globalDir, + }) + + if exit != 2 { + t.Fatalf("exit=%d want 2 (stray positional arg must be rejected); stderr=%s", exit, stderr.String()) + } + if !strings.Contains(stderr.String(), "positional") { + t.Errorf("expected positional-argument error; stderr=%q", stderr.String()) + } +} + +func TestRun_Vet_FormatSARIF_EmitsSARIF(t *testing.T) { + projectDir := writeRuleFiles(t, map[string]string{ + "system.cue": denySystemTargetRule, + }) + globalDir := emptyRulesDir(t) + + var stdout, stderr bytes.Buffer + exit := run(nil, &stdout, &stderr, + []string{"vet", + "--format", "sarif", + "--config", projectDir, + "--global-config", globalDir, + }) + + if exit != 0 { + t.Fatalf("exit=%d want 0; stderr=%s", exit, stderr.String()) + } + var sarif struct { + Schema string `json:"$schema"` + Runs []any `json:"runs"` + } + if err := json.Unmarshal(stdout.Bytes(), &sarif); err != nil { + t.Fatalf("vet --format sarif must emit SARIF JSON, not text; got %q (err: %v)", stdout.String(), err) + } + if len(sarif.Runs) == 0 { + t.Errorf("expected a SARIF run; stdout=%q", stdout.String()) + } +} diff --git a/internal/debuglog/debuglog.go b/internal/debuglog/debuglog.go index 0fca067..ea81084 100644 --- a/internal/debuglog/debuglog.go +++ b/internal/debuglog/debuglog.go @@ -23,12 +23,15 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "strings" "time" ) const defaultTTL = time.Hour +var logFileRE = regexp.MustCompile(`^\d{8}T\d{6}Z-[0-9a-f]{8}\.json$`) + // Entry is the full debug record for one fas invocation. type Entry struct { Timestamp string `json:"timestamp"` @@ -216,7 +219,7 @@ func gc(dir string, ttl time.Duration) { if e.IsDir() { continue } - if !strings.HasSuffix(e.Name(), ".json") { + if !logFileRE.MatchString(e.Name()) { continue } info, err := e.Info() @@ -232,27 +235,12 @@ func gc(dir string, ttl time.Duration) { func shortID() string { var b [4]byte if _, err := rand.Read(b[:]); err != nil { - return "0000" + return "00000000" } return hex.EncodeToString(b[:]) } -// Dir returns the resolved log directory, or empty if disabled. -// Useful for tests and diagnostics. -func (r *Recorder) Dir() string { - if r == nil { - return "" - } - return r.dir -} - -// GC removes files older than ttl from dir. Exported for testing. -func GC(dir string, ttl time.Duration) { - gc(dir, ttl) -} - -// LogFiles returns the .json files in dir sorted by name. Exported for testing. -func LogFiles(dir string) ([]fs.DirEntry, error) { +func logFiles(dir string) ([]fs.DirEntry, error) { entries, err := os.ReadDir(dir) if err != nil { return nil, err diff --git a/internal/debuglog/debuglog_test.go b/internal/debuglog/debuglog_test.go index e6b7868..354c2d4 100644 --- a/internal/debuglog/debuglog_test.go +++ b/internal/debuglog/debuglog_test.go @@ -34,7 +34,7 @@ func TestOpen_TruthyValues(t *testing.T) { t.Errorf("FAS_LOG=%q: expected non-nil recorder", v) continue } - if r.Dir() == "" { + if r.dir == "" { t.Errorf("FAS_LOG=%q: expected non-empty dir", v) } } @@ -46,8 +46,8 @@ func TestOpen_CustomDir(t *testing.T) { if r == nil { t.Fatal("expected non-nil recorder") } - if r.Dir() != dir { - t.Errorf("dir=%q, want %q", r.Dir(), dir) + if r.dir != dir { + t.Errorf("dir=%q, want %q", r.dir, dir) } } @@ -78,7 +78,7 @@ func TestRecorder_WritesFile(t *testing.T) { r.SetOutput([]byte(`{"result":"ok"}`)) r.Close(0) - files, err := LogFiles(dir) + files, err := logFiles(dir) if err != nil { t.Fatal(err) } @@ -111,7 +111,7 @@ func TestRecorder_WritesFile(t *testing.T) { func TestGC_RemovesOldFiles(t *testing.T) { dir := t.TempDir() - old := filepath.Join(dir, "20200101T000000Z-aaaa.json") + old := filepath.Join(dir, "20200101T000000Z-aaaaaaaa.json") if err := os.WriteFile(old, []byte(`{}`), 0o600); err != nil { t.Fatal(err) } @@ -120,21 +120,21 @@ func TestGC_RemovesOldFiles(t *testing.T) { t.Fatal(err) } - fresh := filepath.Join(dir, "20250101T000000Z-bbbb.json") + fresh := filepath.Join(dir, "20250101T000000Z-bbbbbbbb.json") if err := os.WriteFile(fresh, []byte(`{}`), 0o600); err != nil { t.Fatal(err) } - GC(dir, time.Hour) + gc(dir, time.Hour) - files, err := LogFiles(dir) + files, err := logFiles(dir) if err != nil { t.Fatal(err) } if len(files) != 1 { t.Fatalf("got %d files, want 1 (old should be removed)", len(files)) } - if files[0].Name() != "20250101T000000Z-bbbb.json" { + if files[0].Name() != "20250101T000000Z-bbbbbbbb.json" { t.Errorf("remaining file=%q, want the fresh one", files[0].Name()) } } @@ -151,13 +151,32 @@ func TestGC_SkipsNonJSON(t *testing.T) { t.Fatal(err) } - GC(dir, time.Hour) + gc(dir, time.Hour) if _, err := os.Stat(other); err != nil { t.Error("non-JSON file should not be removed by GC") } } +func TestGC_PreservesForeignJSON(t *testing.T) { + dir := t.TempDir() + + foreign := filepath.Join(dir, "package.json") + if err := os.WriteFile(foreign, []byte(`{"name":"x"}`), 0o600); err != nil { + t.Fatal(err) + } + past := time.Now().Add(-2 * time.Hour) + if err := os.Chtimes(foreign, past, past); err != nil { + t.Fatal(err) + } + + gc(dir, time.Hour) + + if _, err := os.Stat(foreign); err != nil { + t.Error("foreign .json not matching the log-file pattern must survive GC") + } +} + func TestParseTTL(t *testing.T) { tests := []struct { raw string