diff --git a/internal/core/audit/audit.go b/internal/core/audit/audit.go index c0f103ae..e116e525 100644 --- a/internal/core/audit/audit.go +++ b/internal/core/audit/audit.go @@ -81,7 +81,7 @@ type Finding struct { // than silently omitting them. type Result struct { Findings []Finding `json:"findings"` - Skipped []string `json:"skipped,omitempty"` + Skipped []string `json:"skipped"` Blockers int `json:"-"` Warnings int `json:"-"` ExitCode int `json:"-"` diff --git a/internal/core/audit/audit_test.go b/internal/core/audit/audit_test.go index 6ab1cb89..fb678912 100644 --- a/internal/core/audit/audit_test.go +++ b/internal/core/audit/audit_test.go @@ -200,6 +200,27 @@ func TestJSONSerializerCleanRepo(t *testing.T) { } } +// The JSON always carries a present "skipped" array, even when nothing was +// skipped — the command doc promises { findings, skipped }, so neither key may +// vanish. +func TestJSONSerializerSkippedAlwaysPresent(t *testing.T) { + out, err := audit.JSONSerializer{}.Serialize(audit.Result{}) + if err != nil { + t.Fatal(err) + } + var decoded map[string]json.RawMessage + if err := json.Unmarshal(out, &decoded); err != nil { + t.Fatal(err) + } + raw, ok := decoded["skipped"] + if !ok { + t.Fatalf(`clean result must still emit a "skipped" key: %s`, out) + } + if string(raw) != "[]" { + t.Errorf(`empty "skipped" must be [], got %s`, raw) + } +} + func TestJSONSerializerCarriesRuleID(t *testing.T) { res := audit.Result{Findings: []audit.Finding{ {RuleID: "three-tier-layout", Severity: audit.SeverityError, File: ".abcd", Line: 0, Message: "missing work/"}, diff --git a/internal/core/audit/rule_docs.go b/internal/core/audit/rule_docs.go index a1630bdc..6fdca7a6 100644 --- a/internal/core/audit/rule_docs.go +++ b/internal/core/audit/rule_docs.go @@ -39,13 +39,18 @@ func (docsCurrency) Where(ctx Context) bool { func (docsCurrency) Eval(ctx Context) ([]Finding, error) { cfgPath := filepath.Join(ctx.RepoRoot, ".abcd", "docs-lint.json") // The engine reuse needs the repo's docs-lint config. A repo with docs/ but - // no committed config cannot be linted; degrade to no findings rather than - // fail — the config is what defines the checks, and its absence is a - // prepare-this-repo gap, not a docs-drift violation. + // no committed config cannot be linted — but the rule must not then read as a + // silent pass. Surface a warn that the check could not run (a + // prepare-this-repo gap), not an absence of drift. if present, err := fsutil.Exists(cfgPath); err != nil { return nil, err } else if !present { - return nil, nil + return []Finding{{ + RuleID: "docs-currency", + Severity: SeverityWarn, + File: ".abcd/docs-lint.json", + Message: "docs/ is present but .abcd/docs-lint.json is missing — docs currency cannot be checked; run prepare-this-repo", + }}, nil } cfg, err := lint.LoadConfig(cfgPath) diff --git a/internal/core/audit/rule_layout.go b/internal/core/audit/rule_layout.go index 9890faa4..1c4e1229 100644 --- a/internal/core/audit/rule_layout.go +++ b/internal/core/audit/rule_layout.go @@ -37,16 +37,20 @@ func (threeTierLayout) Eval(ctx Context) ([]Finding, error) { {".abcd/development", "durable-record tier .abcd/development/"}, {".abcd/work", "shared-working tier .abcd/work/"}, } { - present, err := fsutil.Exists(filepath.Join(ctx.RepoRoot, filepath.FromSlash(tier.rel))) + // A tier is a directory: a regular file at the tier path does not satisfy + // the convention, so check the type, not mere presence. (IsDir follows a + // symlink, so a symlink-to-directory does satisfy it — acceptable for a + // layout check, which is not the owned-store trust boundary.) + isDir, err := fsutil.IsDir(filepath.Join(ctx.RepoRoot, filepath.FromSlash(tier.rel))) if err != nil { return nil, err } - if !present { + if !isDir { out = append(out, Finding{ RuleID: "three-tier-layout", Severity: SeverityError, File: tier.rel, - Message: "missing the " + tier.label, + Message: "missing the " + tier.label + " (must be a directory)", }) } } diff --git a/internal/core/audit/rule_router.go b/internal/core/audit/rule_router.go index b037eb43..9977a67b 100644 --- a/internal/core/audit/rule_router.go +++ b/internal/core/audit/rule_router.go @@ -24,17 +24,25 @@ func (conventionsRouter) Meta() RuleMeta { func (conventionsRouter) Where(Context) bool { return true } func (conventionsRouter) Eval(ctx Context) ([]Finding, error) { - present, err := fsutil.Exists(filepath.Join(ctx.RepoRoot, "AGENTS.md")) + // The router is a file: a directory named AGENTS.md does not satisfy it. + path := filepath.Join(ctx.RepoRoot, "AGENTS.md") + present, err := fsutil.Exists(path) if err != nil { return nil, err } if present { - return nil, nil + isDir, err := fsutil.IsDir(path) + if err != nil { + return nil, err + } + if !isDir { + return nil, nil // a real router file + } } return []Finding{{ RuleID: "conventions-router", Severity: SeverityError, File: "AGENTS.md", - Message: "no AGENTS.md conventions router at the repo root", + Message: "no AGENTS.md conventions router file at the repo root", }}, nil } diff --git a/internal/core/audit/rules_test.go b/internal/core/audit/rules_test.go index 8959de3e..49225297 100644 --- a/internal/core/audit/rules_test.go +++ b/internal/core/audit/rules_test.go @@ -205,6 +205,91 @@ func TestRule_WorkLocalNotGitignored(t *testing.T) { } } +// A tier path that exists but is a regular FILE (not a directory) does not +// satisfy three-tier-layout — the tiers are directories. +func TestRule_TierPresentButNotADirectory(t *testing.T) { + b := newFixtureRepo(t). + file(".gitignore", ".abcd/.work.local/\n"). + file(".abcd/development", "I am a file, not the durable-record tier\n"). // a file at a tier path + file(".abcd/work/DECISIONS.md", "x\n"). + file(".abcd/.work.local/NEXT.md", "x\n"). + file("AGENTS.md", "x\n"). + commit() + res := b.run() + + f := findingFor(res, "three-tier-layout") + if f == nil { + t.Fatal("no three-tier-layout finding when .abcd/development is a file, not a directory") + } + if f.Severity != audit.SeverityError { + t.Errorf("severity = %q, want error", f.Severity) + } +} + +// The work tier specifically: when .abcd/work is a regular file, decision-durability +// stats .abcd/work/DECISIONS.md (ENOTDIR). That must read as "not present", not +// abort the audit — so the three-tier "must be a directory" finding still surfaces. +func TestRule_WorkTierIsAFileDoesNotAbort(t *testing.T) { + b := newFixtureRepo(t). + file(".gitignore", ".abcd/.work.local/\n"). + file(".abcd/development/README.md", "x\n"). + file(".abcd/work", "I am a file, not the shared-working tier\n"). + file(".abcd/.work.local/NEXT.md", "x\n"). + file("AGENTS.md", "x\n"). + commit() + res := b.run() // must not error/abort + + f := findingFor(res, "three-tier-layout") + if f == nil { + t.Fatal("no three-tier-layout finding when .abcd/work is a file (audit aborted instead of reporting)") + } + if f.Severity != audit.SeverityError { + t.Errorf("severity = %q, want error", f.Severity) + } +} + +// A directory named AGENTS.md does not satisfy conventions-router — the router is +// a file. +func TestRule_ConventionsRouterIsADirectory(t *testing.T) { + b := newFixtureRepo(t). + file(".gitignore", ".abcd/.work.local/\n"). + file(".abcd/development/README.md", "x\n"). + file(".abcd/work/DECISIONS.md", "x\n"). + file(".abcd/.work.local/NEXT.md", "x\n"). + file("AGENTS.md/keep.txt", "AGENTS.md is a directory here\n"). // dir, not a router file + commit() + res := b.run() + + f := findingFor(res, "conventions-router") + if f == nil { + t.Fatal("no conventions-router finding when AGENTS.md is a directory") + } + if f.Severity != audit.SeverityError { + t.Errorf("severity = %q, want error", f.Severity) + } +} + +// docs-currency: docs/ exists but no docs-lint config → a warn that the check +// could not run, never a silent pass. +func TestRule_DocsCurrencyNoConfigWarns(t *testing.T) { + b := newFixtureRepo(t).conforming(). + file("docs/how-to/thing.md", "clean docs\n"). // docs/ present, but no .abcd/docs-lint.json + commit() + res := b.run() + + f := findingFor(res, "docs-currency") + if f == nil { + t.Fatal("docs-currency silently passed when docs/ exists but the config is missing") + } + if f.Severity != audit.SeverityWarn { + t.Errorf("severity = %q, want warn", f.Severity) + } + // It must be a warn, not an error: exit 1, not 2. + if res.ExitCode != 1 { + t.Errorf("exit = %d, want 1", res.ExitCode) + } +} + // AC3: a committed file with an absolute local path → privacy-hygiene error // citing file:line — unless a waiver escape is on that line. func TestAC_PrivacyAbsolutePath(t *testing.T) { @@ -231,8 +316,10 @@ func TestAC_PrivacyAbsolutePath(t *testing.T) { func TestAC_PrivacyWaiverSuppresses(t *testing.T) { const waived = "example path /Users/alice/x is illustrative abcd-audit:allow\n" + // Kept out of docs/ so docs-currency stays skipped and does not add a warn — + // this test isolates the privacy waiver's effect on the exit code. b := newFixtureRepo(t).conforming(). - file("docs/reference/paths.md", waived). + file("reference/paths.md", waived). commit() res := b.run() diff --git a/internal/core/audit/serialize.go b/internal/core/audit/serialize.go index 4cd54040..43079b35 100644 --- a/internal/core/audit/serialize.go +++ b/internal/core/audit/serialize.go @@ -16,9 +16,13 @@ type Serializer interface { type JSONSerializer struct{} func (JSONSerializer) Serialize(res Result) ([]byte, error) { - // Guarantee a present empty array rather than JSON null for a clean repo. + // Guarantee present empty arrays rather than JSON null: the command doc + // promises { findings, skipped }, so neither key may be null or absent. if res.Findings == nil { res.Findings = []Finding{} } + if res.Skipped == nil { + res.Skipped = []string{} + } return json.MarshalIndent(res, "", " ") } diff --git a/internal/fsutil/paths.go b/internal/fsutil/paths.go index f44c13b3..3a36af0f 100644 --- a/internal/fsutil/paths.go +++ b/internal/fsutil/paths.go @@ -4,8 +4,18 @@ import ( "errors" "io" "os" + "syscall" ) +// notPresent reports whether a stat/open error means the path cannot exist: it +// is absent (ErrNotExist), or a component of its prefix is not a directory +// (ENOTDIR, e.g. asking about a/b where a is a regular file). Both are "not +// present", not a filesystem fault, so a fail-closed caller must not abort on +// them. +func notPresent(err error) bool { + return errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) +} + // Exists reports whether path exists, following symlinks — so a link to a real // file exists and a dangling link does not. A stat error other than not-exist is // returned rather than swallowed, so a caller checking a convention fails closed @@ -15,7 +25,7 @@ func Exists(path string) (bool, error) { if err == nil { return true, nil } - if errors.Is(err, os.ErrNotExist) { + if notPresent(err) { return false, nil } return false, err @@ -31,7 +41,7 @@ func IsDir(path string) (bool, error) { if err == nil { return fi.IsDir(), nil } - if errors.Is(err, os.ErrNotExist) { + if notPresent(err) { return false, nil } return false, err @@ -47,7 +57,7 @@ func IsDir(path string) (bool, error) { func DirHasEntries(path string) (bool, error) { f, err := os.Open(path) if err != nil { - if errors.Is(err, os.ErrNotExist) { + if notPresent(err) { return false, nil } return false, err diff --git a/internal/fsutil/paths_test.go b/internal/fsutil/paths_test.go index bb95f064..c3977bca 100644 --- a/internal/fsutil/paths_test.go +++ b/internal/fsutil/paths_test.go @@ -61,6 +61,29 @@ func TestExistsSymlink(t *testing.T) { } } +// A path whose parent component is a regular file cannot exist; stat returns +// ENOTDIR, and Exists/IsDir/DirHasEntries must read that as "not present" +// (false, nil), not propagate it as a hard error — otherwise a caller that +// fails closed on errors aborts on an obviously-absent path. +func TestPathUnderAFileIsNotPresent(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "afile") + if err := os.WriteFile(file, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + under := filepath.Join(file, "child") // afile is not a directory + + if got, err := fsutil.Exists(under); err != nil || got { + t.Errorf("Exists(under-a-file) = %v, %v; want false, nil", got, err) + } + if got, err := fsutil.IsDir(under); err != nil || got { + t.Errorf("IsDir(under-a-file) = %v, %v; want false, nil", got, err) + } + if got, err := fsutil.DirHasEntries(under); err != nil || got { + t.Errorf("DirHasEntries(under-a-file) = %v, %v; want false, nil", got, err) + } +} + func TestIsDir(t *testing.T) { dir := t.TempDir() file := filepath.Join(dir, "f.txt")