Skip to content

Commit a9d5cb9

Browse files
committed
fix(config): derive the ignore glob from output.dir instead of hard-coding .enola
`.enola/**` was a literal in the default ignore list, sitting between `.next/**` and `dist/**` as though it were another build-artifact glob, and agreeing with cfg.Output.Dir only by coincidence. Point output.dir anywhere else and each snapshot walked the previous one's artifacts — facts.jsonl, insights.json, llm_context.md, plus the previous/ rotation from run 2 on — so an unchanged tree produced a different snapshot every run. Reproducibility is the property the baseline diff rests on, and this broke it for a reason having nothing to do with determinism, on a setting users are invited to change, with a symptom pointing nowhere near the cause. Comparability checking cannot catch it either: the config is identical on both sides. config.Normalize() now fills in the defaults, validates them, and derives `<output.dir>/**`. It runs from config.Load AND engine.New, because a config assembled in code never passes through Load and has the same problem; it is idempotent. The literal .enola/** stays in the defaults so a repository that used the default before changing it does not start indexing its own history. output.dir must name a subdirectory of the repository. It is joined to the repo path in half a dozen places, so an absolute value silently produced /repo/private/tmp/.../out rather than the location asked for, and no derived glob could describe it. Absolute paths, `..` escapes and `.` are now rejected by name, and the value is cleaned so the glob and the joined directory describe the same place. Rejecting a bad output.dir exposed a worse behaviour above it: a config that EXISTED but could not be used was a warning, replaced by the built-in defaults — whose `repo: "."` is the working directory. A typo therefore made enola analyse whichever repo you were standing in and present it as an answer about the one the config named. ResolveConfig now errors for a config that is present and wrong; a missing config still falls back, which is the intended leniency. Same rule the CLI already applies to an explicitly-named path that does not exist. Migration: the ignore list feeds ignore_glob_hash and config_hash, so a custom output.dir yields one incomparable diff and needs a re-pin. The default location is unaffected — the derivation dedupes against the literal already there. TestSnapshot_CustomOutputDirIsNotIndexedAsSource takes three snapshots of an unchanged tree, writing artifacts each round, and asserts files_seen is constant. It fails on the code it replaces, reporting [2 7 11].
1 parent 4945d8b commit a9d5cb9

7 files changed

Lines changed: 397 additions & 45 deletions

File tree

ARCHITECTURE.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,6 +831,12 @@ The bundled [`mcp-arch.yaml`](mcp-arch.yaml) ships a much fuller `ignore` list (
831831

832832
That line exists because the failure it prevents is silent. A config decides which extractors run and which paths are ignored, so loading the wrong one does not error — it analyses something other than what was asked for. Before the restriction and the announcement, a config sitting beside a `go build` output governed every repository that binary was ever pointed at, from any directory without one of its own; an eleven-extractor list written before the Rust extractor landed turned a 780-file Rust repository into `0 facts`, with no error and no mention of Rust anywhere in the log.
833833

834+
A config that is **missing** falls back to the built-in defaults. A config that is **present and unusable** — unparseable, or naming an `output.dir` that cannot be honoured — is a fatal error instead, because the fallback's `repo: "."` is the working directory: a typo would otherwise make enola analyse whichever repository you were standing in and present it as an answer about the one the config named. Same rule the CLI already applies to an explicitly-named path that does not exist.
835+
836+
**The output directory ignores itself, wherever it is.** `config.Normalize` — run by both `config.Load` and `engine.New`, so a config assembled in code is treated identically — appends `<output.dir>/**` to the ignore list. `.enola/**` remains in the defaults as a literal as well, so a repository that used the default before changing it does not start indexing its own history.
837+
838+
That derivation is load-bearing rather than tidy. The literal used to be the *only* entry, sitting between `.next/**` and `dist/**` as though it were another build-artifact glob, agreeing with `Output.Dir` only by coincidence. Set `output.dir` to anything else and each snapshot walked the previous one's artifacts — `facts.jsonl`, `insights.json`, `llm_context.md`, plus the `previous/` rotation from run 2 onward — so an unchanged tree produced a different snapshot every run. Reproducibility is the property the baseline diff rests on, and comparability checking cannot catch this: the config is identical on both sides.
839+
834840
**A list-valued key REPLACES its default; it does not merge.** `yaml.Unmarshal` overwrites the slice, so `extractors:` names the complete set — a config written before an extractor existed disables it permanently, and a disabled extractor is never tried and so never appears in the log. Two things make that visible: a bundled config that names no plugin lists at all, and a warning naming any *excluded* extractor that would have detected the repository (also recorded as `shadowed_extractors` in the snapshot receipt). The semantics are unchanged on purpose — an explicit list is the only way to disable an extractor.
835841

836842
| Field | Description | Default |
@@ -841,7 +847,7 @@ That line exists because the failure it prevents is silent. A config decides whi
841847
| `extractors` | Enabled extractors | `["cpp", "go", "grpc", "java", "kotlin", "openapi", "php", "python", "typescript", "swift", "ruby", "rust"]` |
842848
| `explainers` | Enabled explainers | `["cycles", "layers", "crossrepo", "coverage", "unused-routes", "god-class", "hotspots", "dependency-depth", "exported-surface", "complexity-outliers"]` |
843849
| `renderers` | Enabled renderers | `["llm_context"]` |
844-
| `output.dir` | Output directory for artifacts | `".enola"` |
850+
| `output.dir` | Output directory for artifacts. Must name a **subdirectory of the repository** — it is joined to the repository path, so an absolute value would nest that whole path inside the repo rather than write where it says. An ignore glob is derived from it automatically (see below) | `".enola"` |
845851
| `output.max_context_tokens` | Token budget for `llm_context.md` | `16000` |
846852
| `dashboard.port` | The fixed **shared URL** port every server competes for, in addition to its own ephemeral one. A negative value serves only the ephemeral port. `ENOLA_DASHBOARD_PORT` overrides it (`off` disables). | `7171` |
847853
| `incremental` | Reuse each extractor's cached facts across snapshots when its files are unchanged; set `false` to force full re-extraction every run | `true` |

internal/config/config.go

Lines changed: 69 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,12 @@ func Default() *Config {
149149
"**/test_*.py",
150150
"**/tests/**/*.py",
151151
"**/test/**/*.py",
152-
".enola/**",
152+
// enola's own output. This is the DEFAULT location only; the glob for the
153+
// configured one is derived in Normalize, which is what makes a custom
154+
// output.dir safe. The literal stays because a repository that used the
155+
// default before changing it still has artifacts here, and indexing its own
156+
// history is exactly the failure the derived glob exists to prevent.
157+
defaultOutputDir + "/**",
153158
// Build / cache artifacts. These are generated output (often transpiled
154159
// JS, e.g. Next.js .next/) and must never be indexed as source — doing so
155160
// pollutes query_facts with thousands of spurious facts. The **/<dir>/**
@@ -220,7 +225,7 @@ func Default() *Config {
220225
Explainers: []string{"cycles", "layers", "crossrepo", "coverage", "unused-routes", "god-class", "hotspots", "dependency-depth", "exported-surface", "complexity-outliers"},
221226
Renderers: []string{"llm_context"},
222227
Output: OutputConfig{
223-
Dir: ".enola",
228+
Dir: defaultOutputDir,
224229
MaxContextTokens: 16000,
225230
},
226231
}
@@ -257,15 +262,71 @@ func Load(path string) (*Config, error) {
257262
cfg.SourcePath = path
258263
}
259264

260-
// Ensure required defaults
261-
if cfg.Output.Dir == "" {
262-
cfg.Output.Dir = ".enola"
265+
if err := cfg.Normalize(); err != nil {
266+
return nil, fmt.Errorf("in config %s: %w", path, err)
263267
}
264-
if cfg.Output.MaxContextTokens == 0 {
265-
cfg.Output.MaxContextTokens = 16000
268+
return cfg, nil
269+
}
270+
271+
// defaultOutputDir is where artifacts go unless output.dir says otherwise. It is
272+
// also present as a literal in Default().Ignore, and deliberately so — see Normalize.
273+
const defaultOutputDir = ".enola"
274+
275+
// Normalize fills in required defaults, validates them, and derives the settings
276+
// that follow from other settings. Idempotent, and called both by Load and by the
277+
// engine, so a config built in code gets the same treatment as one read from a file.
278+
//
279+
// Its real job is the output directory. `.enola/**` used to be a hard-coded literal
280+
// in the default ignore list, sitting between `.next/**` and `dist/**` as though it
281+
// were another build-artifact glob, and agreeing with Output.Dir only by coincidence.
282+
// Point output.dir anywhere else and the next snapshot walked the previous one's
283+
// artifacts — facts.jsonl, insights.json, llm_context.md, plus the previous/ rotation
284+
// from run 2 onward — so an unchanged tree produced a different snapshot every run.
285+
// Reproducibility is the property the baseline diff rests on, and this broke it for a
286+
// reason that has nothing to do with enola's determinism, on a setting users are
287+
// invited to change, with a symptom that points nowhere near the cause.
288+
//
289+
// The literal `.enola/**` stays in Default().Ignore as well: a repository that once
290+
// used the default and later changed it must not start indexing its own history.
291+
func (c *Config) Normalize() error {
292+
if c.Output.Dir == "" {
293+
c.Output.Dir = defaultOutputDir
294+
}
295+
if c.Output.MaxContextTokens == 0 {
296+
c.Output.MaxContextTokens = 16000
266297
}
267298

268-
return cfg, nil
299+
dir, err := cleanOutputDir(c.Output.Dir)
300+
if err != nil {
301+
return err
302+
}
303+
c.Output.Dir = dir
304+
305+
glob := dir + "/**"
306+
if !contains(c.Ignore, glob) {
307+
c.Ignore = append(c.Ignore, glob)
308+
}
309+
return nil
310+
}
311+
312+
// cleanOutputDir validates output.dir and returns it in slash form.
313+
//
314+
// It must be a subdirectory of the repository, because that is the only thing the
315+
// rest of the code can express: Output.Dir is JOINED to the repository path in half a
316+
// dozen places, so an absolute path silently produced a directory nested inside the
317+
// repo (/repo/private/tmp/.../out) rather than the location asked for, and an ignore
318+
// glob derived from it could not describe the artifacts either. An error naming the
319+
// constraint beats a path that looks accepted and means something else.
320+
func cleanOutputDir(dir string) (string, error) {
321+
if filepath.IsAbs(dir) {
322+
return "", fmt.Errorf("output.dir %q must be a path inside the repository, not an absolute one "+
323+
"(it is joined to the repository path, so an absolute value would nest the whole path inside the repo)", dir)
324+
}
325+
clean := filepath.ToSlash(filepath.Clean(dir))
326+
if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") {
327+
return "", fmt.Errorf("output.dir %q must name a subdirectory of the repository", dir)
328+
}
329+
return clean, nil
269330
}
270331

271332
// RepoPaths returns the absolute repository paths this run covers, in the order

internal/config/output_dir_test.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
package config
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
func loadYAML(t *testing.T, yaml string) (*Config, error) {
11+
t.Helper()
12+
p := filepath.Join(t.TempDir(), "mcp-arch.yaml")
13+
if err := os.WriteFile(p, []byte(yaml), 0o644); err != nil {
14+
t.Fatal(err)
15+
}
16+
return Load(p)
17+
}
18+
19+
// The ignore glob for enola's own output must be DERIVED from output.dir, not
20+
// assumed to be `.enola/**`. The two agreed only by coincidence, and the coincidence
21+
// ends the moment a user takes up the invitation to configure the directory.
22+
func TestLoad_DerivesIgnoreGlobFromOutputDir(t *testing.T) {
23+
cfg, err := loadYAML(t, "repo: \".\"\noutput:\n dir: \".enola-bench\"\n")
24+
if err != nil {
25+
t.Fatal(err)
26+
}
27+
if !contains(cfg.Ignore, ".enola-bench/**") {
28+
t.Errorf("no ignore glob derived for output.dir; the next snapshot would walk this "+
29+
"one's artifacts. ignore = %v", cfg.Ignore)
30+
}
31+
if !contains(cfg.Ignore, ".enola/**") {
32+
t.Error("the literal .enola/** was dropped; a repo that used the default before " +
33+
"switching would start indexing its own history")
34+
}
35+
}
36+
37+
// Nested directories are ordinary: `x/y/**` matches `x/y` and everything under it.
38+
func TestLoad_DerivesIgnoreGlobForANestedOutputDir(t *testing.T) {
39+
cfg, err := loadYAML(t, "repo: \".\"\noutput:\n dir: \"build/enola\"\n")
40+
if err != nil {
41+
t.Fatal(err)
42+
}
43+
if !contains(cfg.Ignore, "build/enola/**") {
44+
t.Errorf("ignore = %v, want a derived build/enola/** entry", cfg.Ignore)
45+
}
46+
}
47+
48+
// The default location must not gain a second, identical entry. A duplicate would
49+
// change the ignore-glob hash for every existing user and decline every diff against
50+
// a baseline pinned before the upgrade — a migration cost for no behaviour change.
51+
func TestNormalize_DoesNotDuplicateTheDefaultGlob(t *testing.T) {
52+
cfg := Default()
53+
before := len(cfg.Ignore)
54+
for range 3 {
55+
if err := cfg.Normalize(); err != nil {
56+
t.Fatal(err)
57+
}
58+
}
59+
if len(cfg.Ignore) != before {
60+
t.Errorf("ignore grew from %d to %d entries on a default config", before, len(cfg.Ignore))
61+
}
62+
}
63+
64+
// Normalize runs from both config.Load and engine.New, so it has to be safe to
65+
// repeat on a config that already went through it.
66+
func TestNormalize_IsIdempotentForACustomDir(t *testing.T) {
67+
cfg := Default()
68+
cfg.Output.Dir = "out/enola"
69+
for range 3 {
70+
if err := cfg.Normalize(); err != nil {
71+
t.Fatal(err)
72+
}
73+
}
74+
n := 0
75+
for _, g := range cfg.Ignore {
76+
if g == "out/enola/**" {
77+
n++
78+
}
79+
}
80+
if n != 1 {
81+
t.Errorf("derived glob appears %d times after three Normalize calls, want 1", n)
82+
}
83+
}
84+
85+
// output.dir is joined to the repository path in half a dozen places, so an absolute
86+
// value silently produced a directory nested INSIDE the repo (/repo/private/tmp/…/out)
87+
// rather than the location asked for. Erroring names the constraint; accepting it
88+
// writes the artifacts somewhere the user did not choose and cannot easily find.
89+
func TestLoad_RejectsAnOutputDirItCannotHonour(t *testing.T) {
90+
abs := filepath.Join(t.TempDir(), "out")
91+
for _, tc := range []struct{ name, dir string }{
92+
{"absolute", abs},
93+
{"parent", ".."},
94+
{"escaping", "../elsewhere"},
95+
{"repo root", "."},
96+
} {
97+
t.Run(tc.name, func(t *testing.T) {
98+
_, err := loadYAML(t, "repo: \".\"\noutput:\n dir: \""+tc.dir+"\"\n")
99+
if err == nil {
100+
t.Fatalf("output.dir %q was accepted", tc.dir)
101+
}
102+
if !strings.Contains(err.Error(), "output.dir") {
103+
t.Errorf("error does not name the setting at fault: %v", err)
104+
}
105+
})
106+
}
107+
}
108+
109+
// A path written with redundant segments must normalize, so the glob and the joined
110+
// directory describe the same place.
111+
func TestLoad_CleansTheOutputDir(t *testing.T) {
112+
cfg, err := loadYAML(t, "repo: \".\"\noutput:\n dir: \"./out/./enola\"\n")
113+
if err != nil {
114+
t.Fatal(err)
115+
}
116+
if cfg.Output.Dir != "out/enola" {
117+
t.Errorf("Output.Dir = %q, want the cleaned form", cfg.Output.Dir)
118+
}
119+
if !contains(cfg.Ignore, "out/enola/**") {
120+
t.Errorf("ignore = %v, want the glob to match the cleaned directory", cfg.Ignore)
121+
}
122+
}

internal/engine/engine.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,14 @@ type Engine struct {
6868
// New creates a new Engine with the given config.
6969
// Extractors, explainers, and renderers must be registered after creation.
7070
func New(cfg *config.Config) (*Engine, error) {
71+
// Normalize here as well as in config.Load: a config assembled in code — by a
72+
// test, by a wrapper, by anything that did not read a file — must get the same
73+
// derived ignore glob for its output directory, or it indexes its own artifacts.
74+
// Idempotent, so the file path pays nothing for it.
75+
if err := cfg.Normalize(); err != nil {
76+
return nil, err
77+
}
78+
7179
// The build-scratch store and the initial published store are the same empty
7280
// store, so AutoLoadSnapshot (which mutates Store() in place before serving)
7381
// and the first generate both start from a consistent, non-nil bundle.

internal/engine/output_dir_test.go

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
package engine
2+
3+
import (
4+
"context"
5+
"os"
6+
"path/filepath"
7+
"testing"
8+
9+
"github.com/enola-labs/enola/internal/config"
10+
)
11+
12+
// A snapshot of an unchanged tree must be the same snapshot. That is the property
13+
// the whole baseline diff rests on — a clean diff means something only if a rerun
14+
// over identical inputs is identical.
15+
//
16+
// It used to fail for anyone who set output.dir, for a reason having nothing to do
17+
// with determinism: `.enola/**` was a hard-coded literal in the default ignore list,
18+
// agreeing with Output.Dir only by coincidence. Point output.dir elsewhere and each
19+
// run walked the previous run's artifacts — facts.jsonl, insights.json,
20+
// llm_context.md, plus the previous/ rotation from run 2 onward — so files_seen grew
21+
// every time. Comparability checking cannot catch it either: the config is identical
22+
// on both sides.
23+
func TestSnapshot_CustomOutputDirIsNotIndexedAsSource(t *testing.T) {
24+
repo := t.TempDir()
25+
writeRepoFile(t, repo, "go.mod", "module example.com/out\n\ngo 1.25\n")
26+
writeRepoFile(t, repo, "pkg/x.go", "package pkg\n\nfunc X() string { return \"x\" }\n")
27+
28+
cfg := config.Default()
29+
cfg.Repo = repo
30+
cfg.Output.Dir = ".enola-bench"
31+
cfg.Explainers = nil
32+
33+
e, err := New(cfg)
34+
if err != nil {
35+
t.Fatal(err)
36+
}
37+
38+
var seen []int
39+
for range 3 {
40+
snap, err := e.GenerateSnapshot(context.Background(), repo, false)
41+
if err != nil {
42+
t.Fatalf("GenerateSnapshot: %v", err)
43+
}
44+
// Artifacts have to be WRITTEN for the defect to appear at all: it is the
45+
// previous run's output on disk that the next walk picks up.
46+
if err := e.WriteArtifacts(repo); err != nil {
47+
t.Fatalf("WriteArtifacts: %v", err)
48+
}
49+
seen = append(seen, snap.Meta.FilesSeen)
50+
}
51+
52+
for i, n := range seen {
53+
if n != seen[0] {
54+
t.Errorf("files_seen across three runs of an unchanged tree: %v — run %d walked "+
55+
"%d files instead of %d, which is the previous run's artifacts being indexed "+
56+
"as source", seen, i+1, n, seen[0])
57+
break
58+
}
59+
}
60+
}
61+
62+
// The engine must derive the glob even for a config nobody loaded from a file — a
63+
// wrapper, a test, anything assembled in code. Deriving it only in config.Load would
64+
// leave exactly those callers indexing their own output.
65+
func TestNew_DerivesTheOutputDirIgnoreGlob(t *testing.T) {
66+
cfg := config.Default()
67+
cfg.Output.Dir = "artifacts/enola"
68+
if _, err := New(cfg); err != nil {
69+
t.Fatal(err)
70+
}
71+
72+
if !contains(cfg.Ignore, "artifacts/enola/**") {
73+
t.Errorf("engine.New did not derive an ignore glob for output.dir; ignore = %v", cfg.Ignore)
74+
}
75+
// The default location stays ignored too: a repository that used .enola before
76+
// switching still has artifacts there, and indexing its own history is the same
77+
// defect wearing a different path.
78+
if !contains(cfg.Ignore, ".enola/**") {
79+
t.Errorf("the default .enola/** glob was dropped; ignore = %v", cfg.Ignore)
80+
}
81+
}
82+
83+
// An absolute output.dir was silently joined to the repository path, producing
84+
// /repo/private/tmp/…/out — a directory nested inside the repo rather than the
85+
// location asked for, and one no derived glob could describe.
86+
func TestNew_RejectsAnUnusableOutputDir(t *testing.T) {
87+
for _, dir := range []string{filepath.Join(t.TempDir(), "out"), "..", "../elsewhere", "."} {
88+
cfg := config.Default()
89+
cfg.Output.Dir = dir
90+
if _, err := New(cfg); err == nil {
91+
t.Errorf("output.dir %q was accepted; it cannot be honoured as written", dir)
92+
}
93+
}
94+
}
95+
96+
func contains(ss []string, s string) bool {
97+
for _, v := range ss {
98+
if v == s {
99+
return true
100+
}
101+
}
102+
return false
103+
}
104+
105+
func writeRepoFile(t *testing.T, repo, rel, content string) {
106+
t.Helper()
107+
p := filepath.Join(repo, rel)
108+
if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
109+
t.Fatal(err)
110+
}
111+
if err := os.WriteFile(p, []byte(content), 0o644); err != nil {
112+
t.Fatal(err)
113+
}
114+
}

0 commit comments

Comments
 (0)