From 8091275b64817543c2b1f84d3705bdd2ba290592 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 22:52:26 +0000 Subject: [PATCH 1/7] fix: make lint results independent of how the target is named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directory arguments were passed through almost verbatim, so whether a target was named relatively or absolutely leaked past the reported paths into rule verdicts and internal path resolution. - id-dir-mismatch derived the containing directory's name from the reported path, which has no directory component when the target is named "." — the default. Linting a Feature or Template from inside it therefore always reported the id as mismatched. The name now comes from the directory's own location, handed to rules as linter.Dir. - Compose resolution assumed the lint root was absolute, which only held when the target was named that way; a Dockerfile outside the configuration directory could otherwise be displayed as if inside it. - The github format emitted absolute paths, which GitHub cannot place on a diff. Paths inside the working directory are now reported relative to it, with "/" separators. - Arguments naming the same directory twice are linted once. - Errors name the path once, cleaned, and say what to pass when given a file rather than a directory. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KsXG8rvSVXzUGYMe4AR5mi --- README.md | 6 + cmd/decolint/format.go | 6 +- cmd/decolint/format_test.go | 9 +- cmd/decolint/main.go | 84 ++++++++----- cmd/decolint/main_test.go | 118 +++++++++++++++++- cmd/decolint/opts.go | 23 +++- cmd/decolint/opts_test.go | 37 ++++++ feature/compose.go | 9 +- feature/compose_test.go | 37 ++++++ format/github.go | 25 +++- format/github_test.go | 49 ++++++++ linter/linter.go | 11 +- linter/linter_test.go | 25 ++-- linter/rule.go | 22 +++- rules/category_test.go | 2 +- .../feature_install_script_not_executable.go | 4 +- ...ture_install_script_not_executable_test.go | 2 +- rules/helpers_test.go | 16 +-- rules/id_dir_mismatch.go | 9 +- rules/id_dir_mismatch_test.go | 38 ++++-- rules/missing_feature_install_script.go | 4 +- rules/missing_feature_install_script_test.go | 4 +- rules/rules_test.go | 8 +- rules/undefined_template_option.go | 4 +- rules/undefined_template_option_test.go | 4 +- rules/unused_template_option.go | 4 +- rules/unused_template_option_test.go | 8 +- 27 files changed, 452 insertions(+), 116 deletions(-) diff --git a/README.md b/README.md index 5fcbcc7..9c52606 100644 --- a/README.md +++ b/README.md @@ -188,6 +188,12 @@ Select a different output format to change this: ::warning file=.devcontainer/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version ``` +Findings are reported under the directory as you named it on the +command line, so naming it relatively keeps the paths relative. The +`github` format instead reports every path relative to the directory +`decolint` runs in, which is where GitHub looks for the file to +annotate. + ### Exit codes - `0` — no `error`-severity findings (there may still be `warn` diff --git a/cmd/decolint/format.go b/cmd/decolint/format.go index 00b3810..ad3c603 100644 --- a/cmd/decolint/format.go +++ b/cmd/decolint/format.go @@ -3,6 +3,7 @@ package main import ( "fmt" "io" + "os" "strings" "github.com/bare-devcontainer/decolint/format" @@ -24,7 +25,10 @@ func parseFormat(name string) (Format, error) { case "json": return format.JSONFormat{}, nil case "github": - return format.GitHubFormat{}, nil + // A workflow runs decolint from the checkout, so the working directory is what GitHub + // resolves annotation paths against. An unobtainable one just leaves absolute paths alone. + wd, _ := os.Getwd() + return format.GitHubFormat{BaseDir: wd}, nil default: return nil, fmt.Errorf("unknown format %q (want one of: text, json, github)", name) } diff --git a/cmd/decolint/format_test.go b/cmd/decolint/format_test.go index df7ad9c..0088cc5 100644 --- a/cmd/decolint/format_test.go +++ b/cmd/decolint/format_test.go @@ -1,6 +1,7 @@ package main import ( + "os" "testing" "github.com/bare-devcontainer/decolint/format" @@ -9,6 +10,12 @@ import ( func TestParseFormat(t *testing.T) { t.Parallel() + // The github format is handed the working directory it reports absolute issue paths relative to. + wd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + tests := []struct { name string in string @@ -18,7 +25,7 @@ func TestParseFormat(t *testing.T) { {"empty", "", format.TextFormat{}, false}, {"text", "text", format.TextFormat{}, false}, {"json", "json", format.JSONFormat{}, false}, - {"github", "github", format.GitHubFormat{}, false}, + {"github", "github", format.GitHubFormat{BaseDir: wd}, false}, {"unknown", "bogus", nil, true}, } for _, tt := range tests { diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 2654438..74d87b2 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -302,45 +302,52 @@ type substituteFn func(workspaceFolder string, doc *linter.Document) func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, path string) ([]linter.Issue, error) { root, err := os.OpenRoot(path) if err != nil { - return nil, fmt.Errorf("resolve directory %s: %w", path, err) + // Pointing decolint straight at a devcontainer.json is a natural mistake; the open error + // alone ("not a directory") does not say what to pass instead. + if info, statErr := os.Stat(path); statErr == nil && !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", filepath.Clean(path)) + } + // The wrapped error names the path itself, so repeating it here would only make the message + // longer. + return nil, fmt.Errorf("lint directory: %w", err) } // The root is only read from, so a close error is inconsequential. defer func() { _ = root.Close() }() - issues, err := lintDir(ctx, l, subst, merge, root) - if err != nil { - return nil, fmt.Errorf("lint directory %s: %w", path, err) - } - return issues, nil + return lintDir(ctx, l, subst, merge, root) +} + +// lintRoot is the devcontainer directory a lint runs on, in the two forms the lint needs it in. +type lintRoot struct { + // name is the directory as it was named on the command line. Every reported path is a file's + // location joined onto it, so findings read back the way the user asked for them. + name string + // abs is where the directory actually is. Values that must mean the same thing however the + // directory was named are resolved from it rather than from name: the workspace folder variables + // substitute to, and the directory names rules compare against. + abs string } // lintDir lints every configuration file in the devcontainer directory root is opened on. It is an -// error if the directory contains no configuration. Issue paths are the files' locations joined -// onto root's name. +// error if the directory contains no configuration. func lintDir(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, root *os.Root) ([]linter.Issue, error) { - dir := root.Name() + dir := filepath.Clean(root.Name()) if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) } - // The linted directory is the workspace folder the real tooling would mount, even for a - // configuration discovered in a sub-root like .devcontainer. It is only needed for variable - // substitution, so it is resolved only when that runs. - var workspaceFolder string - if subst != nil { - abs, err := filepath.Abs(dir) - if err != nil { - return nil, fmt.Errorf("resolve workspace folder %s: %w", dir, err) - } - workspaceFolder = abs + abs, err := filepath.Abs(dir) + if err != nil { + return nil, fmt.Errorf("resolve directory %s: %w", dir, err) } + lr := lintRoot{name: dir, abs: abs} var issues []linter.Issue var errs []error found := false - err := discovery.VisitConfigs(root, func(f discovery.ConfigFile) error { + err = discovery.VisitConfigs(root, func(f discovery.ConfigFile) error { found = true if err := ctx.Err(); err != nil { return fmt.Errorf("aborted %s: %w", filepath.Join(f.Root.Name(), f.Path), err) } - fileIssues, err := lintFile(ctx, l, subst, merge, f, workspaceFolder) + fileIssues, err := lintFile(ctx, l, subst, merge, f, lr) if err != nil { // A broken file must not stop the remaining files from being linted, so record the // error and keep visiting. @@ -359,13 +366,13 @@ func lintDir(ctx context.Context, l *linter.Linter, subst substituteFn, merge me return issues, errors.Join(errs...) } -// lintFile reads and lints the single configuration file f, reporting issues under -// filepath.Join(f.Root.Name(), f.Path). The file is read through f.Root, so its resolution cannot -// escape that boundary. subst, if non-nil, resolves variables in dev container configurations -// before merge and rules run, so both see resolved values. merge, if non-nil, runs on dev container -// configurations before rules and is skipped when no rule applies to f's type, so a file with no -// active rules does no Feature fetches. -func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, f discovery.ConfigFile, workspaceFolder string) ([]linter.Issue, error) { +// lintFile reads and lints the single configuration file f, found under the lint root root and +// reported under filepath.Join(f.Root.Name(), f.Path). The file is read through f.Root, so its +// resolution cannot escape that boundary. subst, if non-nil, resolves variables in dev container +// configurations before merge and rules run, so both see resolved values. merge, if non-nil, runs on +// dev container configurations before rules and is skipped when no rule applies to f's type, so a +// file with no active rules does no Feature fetches. +func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, f discovery.ConfigFile, root lintRoot) ([]linter.Issue, error) { display := filepath.Join(f.Root.Name(), f.Path) src, err := f.Root.ReadFile(f.Path) if err != nil { @@ -376,7 +383,9 @@ func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge m return nil, fmt.Errorf("%s: %w", display, err) } if subst != nil && f.Type == linter.Devcontainer { - subst(workspaceFolder, doc) + // The lint root is the workspace folder the real tooling would mount, even for a + // configuration discovered in a sub-root like .devcontainer. + subst(root.abs, doc) } if merge != nil && f.Type == linter.Devcontainer && l.HasRules(f.Type) { if err := merge(ctx, f, doc); err != nil { @@ -384,10 +393,21 @@ func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge m } } // Give rules read access to the config file's directory, confined to the discovery root so - // resolution cannot escape it (see Context.Dir). - dir, err := fs.Sub(f.Root.FS(), path.Dir(filepath.ToSlash(f.Path))) + // resolution cannot escape it (see linter.Dir). + sub, err := fs.Sub(f.Root.FS(), path.Dir(filepath.ToSlash(f.Path))) if err != nil { return nil, fmt.Errorf("open config dir %s: %w", display, err) } - return l.LintDocument(display, f.Type, doc, dir), nil + return l.LintDocument(display, f.Type, doc, linter.Dir{FS: sub, Name: configDirName(root, display)}), nil +} + +// configDirName returns the name of the directory holding the configuration file reported at +// display. The name is taken from the lint root's absolute location, because display carries none of +// its own when the file sits directly in a root named "." — the default lint target. +func configDirName(root lintRoot, display string) string { + rel, err := filepath.Rel(root.name, filepath.Dir(display)) + if err != nil { + return "" + } + return filepath.Base(filepath.Join(root.abs, rel)) } diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 33e438d..075dd26 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -267,6 +267,52 @@ func TestRun(t *testing.T) { } } +// TestRun_IDDirMismatchNamesTheDirectory checks that id-dir-mismatch judges a Feature by the +// directory it sits in rather than by how that directory was named on the command line. Naming it +// from inside, which is what the default lint target does, leaves the reported path with no +// directory component, and the rule must still name the directory the Feature is in. +func TestRun_IDDirMismatchNamesTheDirectory(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + // The fixture's id deliberately does not match its directory, so the rule fires in every run and + // the message it fires with is what distinguishes the runs. + const want = `id "wrong-id" does not match containing directory "feature"` + abs, err := filepath.Abs("testdata/e2e/feature") + if err != nil { + t.Fatalf("Abs: %v", err) + } + + tests := []struct { + name string + args []string + }{ + {"named from outside", []string{abs}}, + {"named as the working directory", []string{"."}}, + {"not named at all", nil}, + } + t.Chdir(abs) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + run(t.Context(), append([]string{"-format=json"}, tt.args...), &stdout, &stderr) + + var issues []linter.Issue + if err := json.Unmarshal(stdout.Bytes(), &issues); err != nil { + t.Fatalf("output is not a JSON issue array: %v\noutput: %s", err, stdout.String()) + } + var got []string + for _, issue := range issues { + if issue.RuleID == "id-dir-mismatch" { + got = append(got, issue.Message) + } + } + if diff := cmp.Diff([]string{want}, got); diff != "" { + t.Errorf("id-dir-mismatch messages mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestRun_Flags(t *testing.T) { t.Parallel() @@ -1323,6 +1369,15 @@ func TestLintPath(t *testing.T) { } }) + t.Run("a configuration file is not a directory", func(t *testing.T) { + t.Parallel() + file := filepath.Join(writeDevcontainer(t, body), ".devcontainer", "devcontainer.json") + _, err := lintPath(t.Context(), newLinter(), noSubst, nil, file) + if err == nil || !strings.Contains(err.Error(), "is not a directory; pass the directory") { + t.Errorf("err = %v, want the error to say a directory is expected", err) + } + }) + t.Run("merge error aborts the file", func(t *testing.T) { t.Parallel() dir := writeDevcontainer(t, body) @@ -1382,11 +1437,64 @@ func TestLintPath(t *testing.T) { }) } -func TestLintDir_WorkspaceFolderError(t *testing.T) { +func TestConfigDirName(t *testing.T) { + t.Parallel() + + abs := filepath.Join(string(filepath.Separator), "work", "my-feature") + tests := []struct { + name string + root lintRoot + display string + want string + }{ + { + "lint root named as the working directory", + lintRoot{name: ".", abs: abs}, + "devcontainer-feature.json", + "my-feature", + }, + { + "lint root named relatively", + lintRoot{name: "my-feature", abs: abs}, + filepath.Join("my-feature", "devcontainer-feature.json"), + "my-feature", + }, + { + "lint root named absolutely", + lintRoot{name: abs, abs: abs}, + filepath.Join(abs, "devcontainer-feature.json"), + "my-feature", + }, + { + "configuration in a sub-directory of the lint root", + lintRoot{name: ".", abs: abs}, + filepath.Join(".devcontainer", "devcontainer.json"), + ".devcontainer", + }, + { + // The lint root and the reported path are always named the same way, so this cannot arise + // from a real lint; naming nothing beats naming the wrong directory. + "path unrelatable to the lint root", + lintRoot{name: "my-feature", abs: abs}, + filepath.Join(abs, "devcontainer-feature.json"), + "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := configDirName(tt.root, tt.display); got != tt.want { + t.Errorf("configDirName(%+v, %q) = %q, want %q", tt.root, tt.display, got, tt.want) + } + }) + } +} + +func TestLintDir_LocationError(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. - // A relative root name resolves against the working directory; deleting it makes the - // workspace folder unresolvable, which must surface as an error. + // A relative root name resolves against the working directory; deleting it leaves the linted + // directory with no location to resolve names from, which must surface as an error. dir := t.TempDir() t.Chdir(dir) root, err := os.OpenRoot(".") @@ -1400,8 +1508,8 @@ func TestLintDir_WorkspaceFolderError(t *testing.T) { subst := func(string, *linter.Document) {} if _, err := lintDir(t.Context(), linter.New(), subst, nil, root); err == nil || - !strings.Contains(err.Error(), "resolve workspace folder") { - t.Errorf("err = %v, want a workspace folder resolution error", err) + !strings.Contains(err.Error(), "resolve directory") { + t.Errorf("err = %v, want a directory resolution error", err) } } diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 2486333..c7e889a 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -4,6 +4,7 @@ import ( "flag" "fmt" "io" + "path/filepath" "strings" "github.com/bare-devcontainer/decolint/linter" @@ -88,7 +89,7 @@ func parseOptions(args []string, output io.Writer) (Options, error) { // config file's "format" member is merged in, so both sources go through one validation path. opts.Format = formatFlag - opts.Paths = fs.Args() + opts.Paths = dedupePaths(fs.Args()) if len(opts.Paths) == 0 { opts.Paths = []string{"."} } @@ -96,6 +97,26 @@ func parseOptions(args []string, output io.Writer) (Options, error) { return opts, nil } +// dedupePaths drops arguments naming a directory an earlier argument already names, so that passing +// e.g. both "." and its absolute path lints it once rather than reporting every finding twice. The +// spelling kept is the first one given, since that is the one the findings are reported under. An +// argument whose location cannot be resolved is kept as given, leaving it for the lint to reject. +func dedupePaths(paths []string) []string { + seen := make(map[string]bool, len(paths)) + kept := make([]string, 0, len(paths)) + for _, p := range paths { + abs, err := filepath.Abs(p) + if err == nil { + if seen[abs] { + continue + } + seen[abs] = true + } + kept = append(kept, p) + } + return kept +} + // parsePlatforms parses a comma-separated list of platform names into a slice of linter.Platform. // An empty string yields a nil slice. func parsePlatforms(s string) ([]linter.Platform, error) { diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index f3f6c53..878020a 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -3,6 +3,7 @@ package main import ( "fmt" "io" + "path/filepath" "testing" "github.com/bare-devcontainer/decolint/linter" @@ -232,3 +233,39 @@ func TestParseOptions_Paths(t *testing.T) { t.Errorf("Paths mismatch (-want +got):\n%s", diff) } } + +func TestParseOptions_PathsDeduped(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + dir := t.TempDir() + t.Chdir(dir) + // t.TempDir can hand back a path through a symlink (/var on macOS), which resolves to the same + // directory as "." but not to the same absolute path; ask for the one deduplication compares. + abs, err := filepath.Abs(".") + if err != nil { + t.Fatalf("Abs: %v", err) + } + + tests := []struct { + name string + args []string + want []string + }{ + {"the same directory spelled two ways", []string{".", abs}, []string{"."}}, + {"the first spelling is the one kept", []string{abs, "."}, []string{abs}}, + {"repeated argument", []string{abs, abs}, []string{abs}}, + {"trailing separator", []string{".", "." + string(filepath.Separator)}, []string{"."}}, + {"distinct directories are all kept", []string{".", filepath.Join(abs, "sub")}, []string{".", filepath.Join(abs, "sub")}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts, err := parseOptions(tt.args, io.Discard) + if err != nil { + t.Fatalf("parseOptions: %v", err) + } + if diff := cmp.Diff(tt.want, opts.Paths); diff != "" { + t.Errorf("Paths mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/feature/compose.go b/feature/compose.go index 5802f54..1939103 100644 --- a/feature/compose.go +++ b/feature/compose.go @@ -43,8 +43,13 @@ func composeContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, confi return nil, true, nil } // The Compose files resolve relative to the referencing devcontainer.json's directory on the real - // filesystem, as "docker compose" itself would. - baseDir := filepath.Join(fsRoot.Name(), configDir) + // filesystem, as "docker compose" itself would. The directory is resolved to an absolute path so + // that compose-go resolves the build context to one too (see composeBuildMetadata), which the + // display of a Dockerfile reached through it depends on (see composeDisplayRef). + baseDir, err := filepath.Abs(filepath.Join(fsRoot.Name(), configDir)) + if err != nil { + return nil, true, fmt.Errorf("resolve %s: %w", configDir, err) + } svc, err := loadComposeService(ctx, baseDir, paths, service, localEnv) if err != nil { return nil, true, err diff --git a/feature/compose_test.go b/feature/compose_test.go index d742794..a9e757c 100644 --- a/feature/compose_test.go +++ b/feature/compose_test.go @@ -334,6 +334,43 @@ func TestMerge_ComposeFileOutsideConfigDir(t *testing.T) { }) } +// TestMerge_ComposeDockerfileRefIsRootIndependent checks that a Dockerfile reached through a compose +// build is named the same way however the lint root was named. The reference appears in the error a +// broken Dockerfile produces, and reaches the merged configuration as a contributor's display id. +func TestMerge_ComposeDockerfileRefIsRootIndependent(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + repo := t.TempDir() + writeFiles(t, repo, map[string]string{ + ".devcontainer/devcontainer.json": `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, + // The build context sits outside the .devcontainer directory, so the reference cannot be a + // name relative to it and must name the Dockerfile's own location instead. + ".devcontainer/docker-compose.yml": "services:\n app:\n build:\n context: ..\n", + "Dockerfile": "FROM scratch\nRUN --bogus=1 x\n", + }) + configDir := filepath.Join(repo, ".devcontainer") + want := filepath.Join(repo, "Dockerfile") + + // The lint root is named relative to the working directory in one run and absolutely in the + // other; both name the same directory. + t.Chdir(repo) + for _, dir := range []string{".devcontainer", configDir} { + t.Run(dir, func(t *testing.T) { + root, err := hujson.Parse([]byte(`{"dockerComposeFile": "docker-compose.yml", "service": "app"}`)) + if err != nil { + t.Fatalf("parse devcontainer.json: %v", err) + } + err = Merge(t.Context(), NewFetcher(), openRoot(t, dir), ".", nil, &root) + if err == nil { + t.Fatal("Merge: got nil error, want the Dockerfile's parse error") + } + if !strings.Contains(err.Error(), want) { + t.Errorf("Merge error = %v, want it to name the Dockerfile as %q", err, want) + } + }) + } +} + // mergeOutsideDir parses src as the .devcontainer/devcontainer.json of repo and merges it with the // root confined to that .devcontainer directory, the confinement discovery applies. func mergeOutsideDir(t *testing.T, repo, src string) *hujson.Value { diff --git a/format/github.go b/format/github.go index 07ceb78..ba225a4 100644 --- a/format/github.go +++ b/format/github.go @@ -3,16 +3,21 @@ package format import ( "fmt" "io" + "path/filepath" "strings" "github.com/bare-devcontainer/decolint/linter" ) // GitHubFormat prints one GitHub Actions workflow command (::error/::warning) per issue. -type GitHubFormat struct{} +type GitHubFormat struct { + // BaseDir is the directory absolute issue paths are made relative to; see [annotationPath]. It + // may be empty, leaving such paths as they are. + BaseDir string +} // WriteIssues writes issues to w as GitHub Actions workflow commands. -func (GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { +func (f GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { for _, issue := range issues { command := "error" if issue.Severity == linter.SeverityWarn { @@ -22,7 +27,7 @@ func (GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { w, "::%s file=%s,line=%d,col=%d,title=%s::%s\n", command, - escapeGitHubProperty(issue.Path), + escapeGitHubProperty(annotationPath(issue.Path, f.BaseDir)), issue.Line, issue.Col, escapeGitHubProperty(issue.RuleID), @@ -35,6 +40,20 @@ func (GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { return nil } +// annotationPath renders path for a workflow command's "file" property. GitHub places an annotation +// by resolving that property against the repository checkout, so an absolute path lands nowhere: one +// inside baseDir is rewritten relative to it, and separators are normalized to "/", which is what +// GitHub matches on every runner. A path outside baseDir is written as is, since there is nothing +// better to say about it. +func annotationPath(path, baseDir string) string { + if baseDir != "" && filepath.IsAbs(path) { + if rel, err := filepath.Rel(baseDir, path); err == nil && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + path = rel + } + } + return filepath.ToSlash(path) +} + // githubDataReplacer escapes the data (message) portion of a GitHub Actions workflow command, per // https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#escaping-properties. // strings.Replacer is safe for concurrent use, so a single package-level instance is reused across calls. diff --git a/format/github_test.go b/format/github_test.go index 8ff67d3..3743dad 100644 --- a/format/github_test.go +++ b/format/github_test.go @@ -2,6 +2,7 @@ package format import ( "errors" + "path/filepath" "strings" "testing" ) @@ -22,6 +23,54 @@ func TestGitHubWriteIssues(t *testing.T) { } } +func TestAnnotationPath(t *testing.T) { + t.Parallel() + + base := filepath.Join(string(filepath.Separator), "work", "repo") + tests := []struct { + name string + path string + baseDir string + want string + }{ + {"relative path is left alone", ".devcontainer/devcontainer.json", base, ".devcontainer/devcontainer.json"}, + {"absolute path inside the base directory", filepath.Join(base, ".devcontainer", "devcontainer.json"), base, ".devcontainer/devcontainer.json"}, + {"the base directory itself", filepath.Join(base, "devcontainer.json"), base, "devcontainer.json"}, + {"absolute path outside the base directory", filepath.Join(string(filepath.Separator), "elsewhere", "devcontainer.json"), base, "/elsewhere/devcontainer.json"}, + {"no base directory", filepath.Join(base, "devcontainer.json"), "", filepath.ToSlash(filepath.Join(base, "devcontainer.json"))}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := annotationPath(tt.path, tt.baseDir); got != tt.want { + t.Errorf("annotationPath(%q, %q) = %q, want %q", tt.path, tt.baseDir, got, tt.want) + } + }) + } +} + +func TestGitHubWriteIssues_AbsolutePath(t *testing.T) { + t.Parallel() + + base := filepath.Join(string(filepath.Separator), "work", "repo") + issues := testIssues() + for i := range issues { + issues[i].Path = filepath.Join(base, filepath.FromSlash(issues[i].Path)) + } + + var sb strings.Builder + if err := (GitHubFormat{BaseDir: base}).WriteIssues(&sb, issues); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + want := `::warning file=.devcontainer/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version +::error file=.devcontainer/devcontainer.json,line=8,col=3,title=some-error-rule::something is broken +` + if sb.String() != want { + t.Errorf("WriteIssues github = %q, want %q", sb.String(), want) + } +} + func TestGitHubWriteIssues_WriteError(t *testing.T) { t.Parallel() diff --git a/linter/linter.go b/linter/linter.go index 4040869..0b89e98 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -6,7 +6,6 @@ package linter import ( "fmt" - "io/fs" "sort" ) @@ -55,11 +54,11 @@ func (l *Linter) HasRules(t FileType) bool { } // LintDocument applies the linter's rules to doc, a configuration file of the given type, and -// returns the findings sorted by position. path is used only for reporting. dir, if non-nil, gives -// rules read access to the directory containing the file (see Context.Dir); it may be nil for an -// in-memory document. It reads the document as given; any mutation of its tree (see Document.Tree) -// must happen before calling it. -func (l *Linter) LintDocument(path string, fileType FileType, doc *Document, dir fs.FS) []Issue { +// returns the findings sorted by position. path is used only for reporting, and dir describes the +// directory containing the file; both are handed to the rules as [Context.Path] and [Context.Dir], +// and dir may be left zero for an in-memory document. It reads the document as given; any mutation +// of its tree (see Document.Tree) must happen before calling it. +func (l *Linter) LintDocument(path string, fileType FileType, doc *Document, dir Dir) []Issue { patterns := l.patterns[fileType] if len(patterns) == 0 { return nil diff --git a/linter/linter_test.go b/linter/linter_test.go index 9d1941b..7647a57 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -2,7 +2,6 @@ package linter import ( "fmt" - "io/fs" "slices" "strings" "testing" @@ -46,7 +45,7 @@ func lintSource(t *testing.T, l *Linter, path string, fileType FileType, src str if err != nil { t.Fatalf("ParseDocument: %v", err) } - return l.LintDocument(path, fileType, doc, nil) + return l.LintDocument(path, fileType, doc, Dir{}) } func TestLintDocument_Position(t *testing.T) { @@ -109,7 +108,7 @@ func TestLintDocument_TreeMutation(t *testing.T) { l := New() l.RegisterRule(flagRule, SeverityWarn) - issues := l.LintDocument("devcontainer.json", Devcontainer, doc, nil) + issues := l.LintDocument("devcontainer.json", Devcontainer, doc, Dir{}) if len(issues) != 1 { t.Fatalf("got %d issues %v, want 1", len(issues), issues) } @@ -122,8 +121,8 @@ func TestLintDocument_ContextDir(t *testing.T) { t.Parallel() // A stub rule records the Dir it is handed so we can assert LintDocument passes it through - // unchanged, including the nil case for an in-memory document. - var got fs.FS + // unchanged, including the zero case for an in-memory document. + var got Dir dirRule := &Rule{ ID: "dir-rule", FileTypes: []FileType{Devcontainer}, @@ -134,13 +133,13 @@ func TestLintDocument_ContextDir(t *testing.T) { }, } - want := fstest.MapFS{"install.sh": {Data: []byte("#!/bin/sh\n")}} + want := Dir{FS: fstest.MapFS{"install.sh": {Data: []byte("#!/bin/sh\n")}}, Name: "my-feature"} for _, tt := range []struct { name string - dir fs.FS + dir Dir }{ {"with directory", want}, - {"nil directory", nil}, + {"zero directory", Dir{}}, } { t.Run(tt.name, func(t *testing.T) { doc, err := ParseDocument([]byte(`{"name": "test"}`)) @@ -149,13 +148,13 @@ func TestLintDocument_ContextDir(t *testing.T) { } l := New() l.RegisterRule(dirRule, SeverityWarn) - got = nil + got = Dir{} l.LintDocument("devcontainer.json", Devcontainer, doc, tt.dir) - if got == nil && tt.dir != nil { - t.Fatalf("ctx.Dir = nil, want the passed filesystem") + if got.Name != tt.dir.Name { + t.Errorf("ctx.Dir.Name = %q, want %q", got.Name, tt.dir.Name) } - if tt.dir == nil && got != nil { - t.Fatalf("ctx.Dir = %v, want nil", got) + if (got.FS == nil) != (tt.dir.FS == nil) { + t.Errorf("ctx.Dir.FS = %v, want %v", got.FS, tt.dir.FS) } }) } diff --git a/linter/rule.go b/linter/rule.go index 4747c29..60cc656 100644 --- a/linter/rule.go +++ b/linter/rule.go @@ -138,17 +138,29 @@ func ParseCategory(name string) (Category, error) { // Context carries everything a rule needs to inspect a single configuration file. type Context struct { - // Path is the path of the file being linted. + // Path is the path of the file being linted. It is meant for reporting: its spelling follows + // whatever the caller named the file, so a rule must not read anything into its shape (see + // [Dir] for the containing directory). Path string // Type is the kind of configuration file being linted. Type FileType // Root is the HuJSON syntax tree of the file. It preserves comments and byte offsets into the // original source. Root *hujson.Value - // Dir gives read access to the directory containing the file being linted, confined to the lint - // root. It is nil when the caller has no backing filesystem (e.g. an in-memory document); a rule - // that inspects sibling files must return no findings when it is nil. - Dir fs.FS + // Dir describes the directory containing the file being linted. + Dir Dir +} + +// Dir is the directory a configuration file is linted in. Each field is independently optional, and +// a rule that needs one must return no findings when it is unset. +type Dir struct { + // FS gives read access to the directory's contents, confined to the lint root. It is nil when + // the caller has no backing filesystem, e.g. an in-memory document. + FS fs.FS + // Name is the directory's own name, resolved from where the directory actually is. It is not + // derived from [Context.Path], which carries no name of its own when the caller names the file + // relative to the directory itself. It is empty when the caller has no directory to name. + Name string } // Severity indicates how a finding should be treated: whether it's reported as an error or a diff --git a/rules/category_test.go b/rules/category_test.go index 12c2fbe..18bd5f3 100644 --- a/rules/category_test.go +++ b/rules/category_test.go @@ -121,7 +121,7 @@ func TestRegisterRules_CategoryOverride(t *testing.T) { if err := rules.RegisterRules(l, nil, tt.overrides); err != nil { t.Fatalf("RegisterRules: %v", err) } - issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, nil) + issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, linter.Dir{}) if fired := ruleFired(issues, "no-seccomp-override"); fired != tt.wantFired { t.Errorf("no-seccomp-override fired = %v, want %v (issues: %v)", fired, tt.wantFired, issues) } diff --git a/rules/feature_install_script_not_executable.go b/rules/feature_install_script_not_executable.go index 2055e89..9ba8c4b 100644 --- a/rules/feature_install_script_not_executable.go +++ b/rules/feature_install_script_not_executable.go @@ -23,10 +23,10 @@ var FeatureInstallScriptNotExecutable = &linter.Rule{ func checkFeatureInstallScriptNotExecutable(ctx *linter.Context, node *linter.Node) []linter.Finding { // Windows working trees carry no executable bits (git does not set them there), so the check // would report every install.sh as non-executable. Skip it rather than emit false positives. - if ctx.Dir == nil || runtime.GOOS == "windows" { + if ctx.Dir.FS == nil || runtime.GOOS == "windows" { return nil } - info, err := fs.Stat(ctx.Dir, installScriptName) + info, err := fs.Stat(ctx.Dir.FS, installScriptName) if err != nil || info.IsDir() { return nil } diff --git a/rules/feature_install_script_not_executable_test.go b/rules/feature_install_script_not_executable_test.go index 2aa844a..6f4367c 100644 --- a/rules/feature_install_script_not_executable_test.go +++ b/rules/feature_install_script_not_executable_test.go @@ -35,7 +35,7 @@ func TestFeatureInstallScriptNotExecutable(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.FeatureInstallScriptNotExecutable, linter.SeverityError, path, linter.Feature, src, tt.dir, tt.want) + assertIssuesInDir(t, rules.FeatureInstallScriptNotExecutable, linter.SeverityError, path, linter.Feature, src, linter.Dir{FS: tt.dir}, tt.want) }) } diff --git a/rules/helpers_test.go b/rules/helpers_test.go index abb71bc..5ee1e97 100644 --- a/rules/helpers_test.go +++ b/rules/helpers_test.go @@ -20,9 +20,8 @@ var errUnreadable = errors.New("unreadable filesystem") // lintSource parses src and applies l's registered rules to it as a file at the given path and of // the given type, failing the test on any error. dir is the directory the file is linted in (see -// [linter.Context.Dir]); it is nil for the in-memory documents most rule tests use in place of a -// real directory. -func lintSource(t *testing.T, l *linter.Linter, path string, fileType linter.FileType, src string, dir fs.FS) []linter.Issue { +// [linter.Dir]). +func lintSource(t *testing.T, l *linter.Linter, path string, fileType linter.FileType, src string, dir linter.Dir) []linter.Issue { t.Helper() doc, err := linter.ParseDocument([]byte(src)) if err != nil { @@ -42,16 +41,17 @@ func assertIssues(t *testing.T, r *linter.Rule, severity linter.Severity, src st // assertIssuesAt is like assertIssues but lets the caller control the path and file type src is // linted as, e.g. to exercise rules that apply only to Features or Templates. The linted file has -// no backing directory; use assertIssuesInDir for rules that inspect sibling files. +// no backing directory; use assertIssuesInDir for rules that inspect the directory they are in. func assertIssuesAt(t *testing.T, r *linter.Rule, severity linter.Severity, path string, fileType linter.FileType, src string, want []linter.Issue) { t.Helper() - assertIssuesInDir(t, r, severity, path, fileType, src, nil, want) + assertIssuesInDir(t, r, severity, path, fileType, src, linter.Dir{}, want) } // assertIssuesInDir is like assertIssuesAt but lets the caller supply the directory the file is -// linted in (see [linter.Context.Dir]), so rules that inspect sibling files can be exercised -// against a fake filesystem (e.g. a [testing/fstest.MapFS]). -func assertIssuesInDir(t *testing.T, r *linter.Rule, severity linter.Severity, path string, fileType linter.FileType, src string, dir fs.FS, want []linter.Issue) { +// linted in (see [linter.Dir]), so rules that inspect sibling files can be exercised against a fake +// filesystem (e.g. a [testing/fstest.MapFS]) and rules that read the directory's name against a +// name of the caller's choosing. +func assertIssuesInDir(t *testing.T, r *linter.Rule, severity linter.Severity, path string, fileType linter.FileType, src string, dir linter.Dir, want []linter.Issue) { t.Helper() l := linter.New() diff --git a/rules/id_dir_mismatch.go b/rules/id_dir_mismatch.go index bce428a..9256d12 100644 --- a/rules/id_dir_mismatch.go +++ b/rules/id_dir_mismatch.go @@ -2,7 +2,6 @@ package rules import ( "fmt" - "path/filepath" "github.com/bare-devcontainer/decolint/linter" "github.com/tailscale/hujson" @@ -24,13 +23,15 @@ func checkIDDirMismatch(ctx *linter.Context, node *linter.Node) []linter.Finding if !ok || lit.Kind() != '"' { return nil } + if ctx.Dir.Name == "" { + return nil + } id := lit.String() - dir := filepath.Base(filepath.Dir(ctx.Path)) - if id == dir { + if id == ctx.Dir.Name { return nil } return []linter.Finding{{ - Message: fmt.Sprintf("id %q does not match containing directory %q", id, dir), + Message: fmt.Sprintf("id %q does not match containing directory %q", id, ctx.Dir.Name), Offset: node.Value.StartOffset, }} } diff --git a/rules/id_dir_mismatch_test.go b/rules/id_dir_mismatch_test.go index c5fc2ac..0e5eb57 100644 --- a/rules/id_dir_mismatch_test.go +++ b/rules/id_dir_mismatch_test.go @@ -10,38 +10,50 @@ import ( func TestIDDirMismatch(t *testing.T) { t.Parallel() + // The reported path and the containing directory's name are supplied separately, as the linter + // supplies them: the path is only how the file is named in output, and carries no directory + // component of its own when the lint target is the directory the file sits in. + const featurePath = "my-feature/devcontainer-feature.json" + const templatePath = "my-template/devcontainer-template.json" + tests := []struct { name string path string + dir string fileType linter.FileType src string want []linter.Issue }{ - {"no id property", "my-feature/devcontainer-feature.json", linter.Feature, `{"name": "test"}`, nil}, - {"feature matching id", "my-feature/devcontainer-feature.json", linter.Feature, `{"id": "my-feature"}`, nil}, - {"feature mismatched id", "my-feature/devcontainer-feature.json", linter.Feature, `{"id": "other"}`, []linter.Issue{ - {Path: "my-feature/devcontainer-feature.json", Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-feature"`}, + {"no id property", featurePath, "my-feature", linter.Feature, `{"name": "test"}`, nil}, + {"feature matching id", featurePath, "my-feature", linter.Feature, `{"id": "my-feature"}`, nil}, + {"feature mismatched id", featurePath, "my-feature", linter.Feature, `{"id": "other"}`, []linter.Issue{ + {Path: featurePath, Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-feature"`}, + }}, + {"feature reported with no directory component", "devcontainer-feature.json", "my-feature", linter.Feature, `{"id": "my-feature"}`, nil}, + {"feature reported with no directory component, mismatched id", "devcontainer-feature.json", "my-feature", linter.Feature, `{"id": "other"}`, []linter.Issue{ + {Path: "devcontainer-feature.json", Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-feature"`}, }}, - {"feature nested under a parent directory", "src/my-feature/devcontainer-feature.json", linter.Feature, `{"id": "my-feature"}`, nil}, - {"feature nested, id matches parent instead of own directory", "src/my-feature/devcontainer-feature.json", linter.Feature, `{"id": "src"}`, []linter.Issue{ + {"feature nested under a parent directory", "src/my-feature/devcontainer-feature.json", "my-feature", linter.Feature, `{"id": "my-feature"}`, nil}, + {"feature nested, id matches parent instead of own directory", "src/my-feature/devcontainer-feature.json", "my-feature", linter.Feature, `{"id": "src"}`, []linter.Issue{ {Path: "src/my-feature/devcontainer-feature.json", Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "src" does not match containing directory "my-feature"`}, }}, - {"template matching id", "my-template/devcontainer-template.json", linter.Template, `{"id": "my-template"}`, nil}, - {"template mismatched id", "my-template/devcontainer-template.json", linter.Template, `{"id": "other"}`, []linter.Issue{ - {Path: "my-template/devcontainer-template.json", Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-template"`}, + {"template matching id", templatePath, "my-template", linter.Template, `{"id": "my-template"}`, nil}, + {"template mismatched id", templatePath, "my-template", linter.Template, `{"id": "other"}`, []linter.Issue{ + {Path: templatePath, Line: 1, Col: 8, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-template"`}, }}, - {"non-string id", "my-feature/devcontainer-feature.json", linter.Feature, `{"id": 42}`, nil}, - {"position points at the value, not the key", "my-feature/devcontainer-feature.json", linter.Feature, `{ + {"unnamed directory", featurePath, "", linter.Feature, `{"id": "other"}`, nil}, + {"non-string id", featurePath, "my-feature", linter.Feature, `{"id": 42}`, nil}, + {"position points at the value, not the key", featurePath, "my-feature", linter.Feature, `{ // the feature's id "id": "other" }`, []linter.Issue{ - {Path: "my-feature/devcontainer-feature.json", Line: 3, Col: 9, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-feature"`}, + {Path: featurePath, Line: 3, Col: 9, RuleID: "id-dir-mismatch", Message: `id "other" does not match containing directory "my-feature"`}, }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assertIssuesAt(t, rules.IDDirMismatch, linter.SeverityError, tt.path, tt.fileType, tt.src, tt.want) + assertIssuesInDir(t, rules.IDDirMismatch, linter.SeverityError, tt.path, tt.fileType, tt.src, linter.Dir{Name: tt.dir}, tt.want) }) } } diff --git a/rules/missing_feature_install_script.go b/rules/missing_feature_install_script.go index a0e10d4..0b16cd3 100644 --- a/rules/missing_feature_install_script.go +++ b/rules/missing_feature_install_script.go @@ -23,10 +23,10 @@ var MissingFeatureInstallScript = &linter.Rule{ } func checkMissingFeatureInstallScript(ctx *linter.Context, node *linter.Node) []linter.Finding { - if ctx.Dir == nil { + if ctx.Dir.FS == nil { return nil } - info, err := fs.Stat(ctx.Dir, installScriptName) + info, err := fs.Stat(ctx.Dir.FS, installScriptName) switch { case err == nil && !info.IsDir(): return nil diff --git a/rules/missing_feature_install_script_test.go b/rules/missing_feature_install_script_test.go index 02dbe42..d4ff677 100644 --- a/rules/missing_feature_install_script_test.go +++ b/rules/missing_feature_install_script_test.go @@ -30,7 +30,7 @@ func TestMissingFeatureInstallScript(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.MissingFeatureInstallScript, linter.SeverityError, path, linter.Feature, src, tt.dir, tt.want) + assertIssuesInDir(t, rules.MissingFeatureInstallScript, linter.SeverityError, path, linter.Feature, src, linter.Dir{FS: tt.dir}, tt.want) }) } @@ -41,6 +41,6 @@ func TestMissingFeatureInstallScript(t *testing.T) { t.Run("unreadable directory reports nothing", func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.MissingFeatureInstallScript, linter.SeverityError, path, linter.Feature, src, errFS{}, nil) + assertIssuesInDir(t, rules.MissingFeatureInstallScript, linter.SeverityError, path, linter.Feature, src, linter.Dir{FS: errFS{}}, nil) }) } diff --git a/rules/rules_test.go b/rules/rules_test.go index 1402f33..c0ad72f 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -83,7 +83,7 @@ func TestRegisterRules_PlatformFilter(t *testing.T) { if err := rules.RegisterRules(l, tt.platforms, rules.Overrides{}); err != nil { t.Fatalf("RegisterRules: %v", err) } - issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, nil) + issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, linter.Dir{}) fired := ruleFired(issues, "no-bind-mount") if fired != tt.wantFired { t.Errorf("no-bind-mount fired = %v, want %v (issues: %v)", fired, tt.wantFired, issues) @@ -103,7 +103,7 @@ func TestRegisterRules_DefaultOffRule(t *testing.T) { if err := rules.RegisterRules(l, nil, rules.Overrides{}); err != nil { t.Fatalf("RegisterRules: %v", err) } - issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, nil) + issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, linter.Dir{}) if ruleFired(issues, "no-seccomp-override") { t.Errorf("no-seccomp-override fired despite being off by default: %v", issues) } @@ -119,7 +119,7 @@ func TestRegisterRules_OverrideEnablesOffDefaultRule(t *testing.T) { if err := rules.RegisterRules(l, nil, overrides); err != nil { t.Fatalf("RegisterRules: %v", err) } - issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, nil) + issues := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, linter.Dir{}) if !ruleFired(issues, "no-seccomp-override") { t.Errorf("no-seccomp-override did not fire despite being overridden to error: %v", issues) } @@ -173,7 +173,7 @@ func TestRegisterRules_SeverityOverride(t *testing.T) { if err := rules.RegisterRules(l, nil, rules.Overrides{Rules: tt.overrides}); err != nil { t.Fatalf("RegisterRules: %v", err) } - got := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, nil) + got := lintSource(t, l, "devcontainer.json", linter.Devcontainer, src, linter.Dir{}) if diff := cmp.Diff(tt.want, got); diff != "" { t.Errorf("issues mismatch (-want +got):\n%s", diff) } diff --git a/rules/undefined_template_option.go b/rules/undefined_template_option.go index f96a8a4..2b76fc1 100644 --- a/rules/undefined_template_option.go +++ b/rules/undefined_template_option.go @@ -23,14 +23,14 @@ var UndefinedTemplateOption = &linter.Rule{ } func checkUndefinedTemplateOption(ctx *linter.Context, node *linter.Node) []linter.Finding { - if ctx.Dir == nil { + if ctx.Dir.FS == nil { return nil } obj, ok := node.Value.Value.(*hujson.Object) if !ok { return nil } - refs, err := templateOptionRefs(ctx.Dir) + refs, err := templateOptionRefs(ctx.Dir.FS) if err != nil { return nil } diff --git a/rules/undefined_template_option_test.go b/rules/undefined_template_option_test.go index 9c5dfcd..511eba3 100644 --- a/rules/undefined_template_option_test.go +++ b/rules/undefined_template_option_test.go @@ -76,13 +76,13 @@ func TestUndefinedTemplateOption(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.UndefinedTemplateOption, linter.SeverityError, path, linter.Template, tt.src, tt.dir, tt.want) + assertIssuesInDir(t, rules.UndefinedTemplateOption, linter.SeverityError, path, linter.Template, tt.src, linter.Dir{FS: tt.dir}, tt.want) }) } t.Run("unreadable directory reports nothing", func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.UndefinedTemplateOption, linter.SeverityError, path, linter.Template, withOptions, errFS{}, nil) + assertIssuesInDir(t, rules.UndefinedTemplateOption, linter.SeverityError, path, linter.Template, withOptions, linter.Dir{FS: errFS{}}, nil) }) t.Run("nil directory", func(t *testing.T) { diff --git a/rules/unused_template_option.go b/rules/unused_template_option.go index 8a260cf..73c0418 100644 --- a/rules/unused_template_option.go +++ b/rules/unused_template_option.go @@ -20,14 +20,14 @@ var UnusedTemplateOption = &linter.Rule{ } func checkUnusedTemplateOption(ctx *linter.Context, node *linter.Node) []linter.Finding { - if ctx.Dir == nil { + if ctx.Dir.FS == nil { return nil } obj, ok := node.Value.Value.(*hujson.Object) if !ok { return nil } - refs, err := templateOptionRefs(ctx.Dir) + refs, err := templateOptionRefs(ctx.Dir.FS) if err != nil { return nil } diff --git a/rules/unused_template_option_test.go b/rules/unused_template_option_test.go index 9b58878..c56c2f1 100644 --- a/rules/unused_template_option_test.go +++ b/rules/unused_template_option_test.go @@ -59,25 +59,25 @@ func TestUnusedTemplateOption(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, src, tt.dir, tt.want) + assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, src, linter.Dir{FS: tt.dir}, tt.want) }) } t.Run("no options member", func(t *testing.T) { t.Parallel() dir := fstest.MapFS{".devcontainer/devcontainer.json": {Data: []byte("${templateOption:used}")}} - assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, `{"id": "my-template"}`, dir, nil) + assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, `{"id": "my-template"}`, linter.Dir{FS: dir}, nil) }) t.Run("options is not an object", func(t *testing.T) { t.Parallel() dir := fstest.MapFS{".devcontainer/devcontainer.json": {Data: []byte("${templateOption:used}")}} - assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, `{"options": "nope"}`, dir, nil) + assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, `{"options": "nope"}`, linter.Dir{FS: dir}, nil) }) t.Run("unreadable directory reports nothing", func(t *testing.T) { t.Parallel() - assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, src, errFS{}, nil) + assertIssuesInDir(t, rules.UnusedTemplateOption, linter.SeverityError, path, linter.Template, src, linter.Dir{FS: errFS{}}, nil) }) t.Run("nil directory", func(t *testing.T) { From eae8e1962377cb1b6b88348bda9a70fcfc1b6db4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 23:45:40 +0000 Subject: [PATCH 2/7] fix: report every path relative to the working directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Paths were still kept in three representations — the spelling given on the command line, the absolute location, and the github format's own base directory — so each function had to know which space its path was in, and conversions like configDirName existed only to move between them. Opening the lint root on its absolute location makes every path derived from it absolute for free: os.Root.OpenRoot names a sub-root by joining onto its parent's name, so the .devcontainer sub-root and the paths built from it are absolute too. Only the rendering for the reader is left, which absPath.String does — relative to the working directory when the path is inside it, absolute otherwise. Being a Stringer, it renders in error messages without any call site having to ask for it. This drops lintRoot, configDirName, lintDir's filepath.Abs, and the github format's BaseDir. Findings now name a file the same way however the lint target was named, so an absolute target under the working directory is reported relative to it. The Dockerfile reference a Compose build resolves keeps its own base, the configuration directory: it identifies a contributor inside the merged configuration, so it must not depend on where decolint was run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KsXG8rvSVXzUGYMe4AR5mi --- README.md | 8 +-- cmd/decolint/format.go | 6 +- cmd/decolint/format_test.go | 9 +-- cmd/decolint/main.go | 108 +++++++++++++++------------- cmd/decolint/main_test.go | 140 +++++++++++++++++++++++------------- cmd/decolint/opts.go | 6 +- feature/compose.go | 4 ++ format/github.go | 27 ++----- format/github_test.go | 42 ++--------- 9 files changed, 172 insertions(+), 178 deletions(-) diff --git a/README.md b/README.md index 9c52606..0d980d3 100644 --- a/README.md +++ b/README.md @@ -188,11 +188,9 @@ Select a different output format to change this: ::warning file=.devcontainer/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version ``` -Findings are reported under the directory as you named it on the -command line, so naming it relatively keeps the paths relative. The -`github` format instead reports every path relative to the directory -`decolint` runs in, which is where GitHub looks for the file to -annotate. +Every format reports paths relative to the directory `decolint` runs in, +whichever way the linted directory was named on the command line. A file +outside that directory is reported with its absolute path. ### Exit codes diff --git a/cmd/decolint/format.go b/cmd/decolint/format.go index ad3c603..00b3810 100644 --- a/cmd/decolint/format.go +++ b/cmd/decolint/format.go @@ -3,7 +3,6 @@ package main import ( "fmt" "io" - "os" "strings" "github.com/bare-devcontainer/decolint/format" @@ -25,10 +24,7 @@ func parseFormat(name string) (Format, error) { case "json": return format.JSONFormat{}, nil case "github": - // A workflow runs decolint from the checkout, so the working directory is what GitHub - // resolves annotation paths against. An unobtainable one just leaves absolute paths alone. - wd, _ := os.Getwd() - return format.GitHubFormat{BaseDir: wd}, nil + return format.GitHubFormat{}, nil default: return nil, fmt.Errorf("unknown format %q (want one of: text, json, github)", name) } diff --git a/cmd/decolint/format_test.go b/cmd/decolint/format_test.go index 0088cc5..df7ad9c 100644 --- a/cmd/decolint/format_test.go +++ b/cmd/decolint/format_test.go @@ -1,7 +1,6 @@ package main import ( - "os" "testing" "github.com/bare-devcontainer/decolint/format" @@ -10,12 +9,6 @@ import ( func TestParseFormat(t *testing.T) { t.Parallel() - // The github format is handed the working directory it reports absolute issue paths relative to. - wd, err := os.Getwd() - if err != nil { - t.Fatalf("Getwd: %v", err) - } - tests := []struct { name string in string @@ -25,7 +18,7 @@ func TestParseFormat(t *testing.T) { {"empty", "", format.TextFormat{}, false}, {"text", "text", format.TextFormat{}, false}, {"json", "json", format.JSONFormat{}, false}, - {"github", "github", format.GitHubFormat{BaseDir: wd}, false}, + {"github", "github", format.GitHubFormat{}, false}, {"unknown", "bogus", nil, true}, } for _, tt := range tests { diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 74d87b2..54d5185 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -297,15 +297,40 @@ type mergeFn func(ctx context.Context, f discovery.ConfigFile, doc *linter.Docum // lintFile. type substituteFn func(workspaceFolder string, doc *linter.Document) -// lintPath lints the devcontainer directory at path. The directory is opened as an os.Root, so -// every file the lint reads is confined to it. It is an error if path is not a directory. +// absPath is the location of a file or directory, always absolute so that it means the same thing +// however the lint target was named on the command line. Printing it renders it for the reader; see +// [absPath.String]. +type absPath string + +// String renders p the way findings and error messages name it: relative to the working directory +// when it is inside it, absolute otherwise. A path outside the working directory has no relative +// form worth reading, and neither has one when the working directory cannot be determined. +func (p absPath) String() string { + wd, err := os.Getwd() + if err != nil { + return string(p) + } + rel, err := filepath.Rel(wd, string(p)) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return string(p) + } + return rel +} + +// lintPath lints the devcontainer directory at path. The directory is opened as an os.Root on its +// absolute location, so every file the lint reads is confined to it and every path derived from it +// is absolute. It is an error if path is not a directory. func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, path string) ([]linter.Issue, error) { - root, err := os.OpenRoot(path) + abs, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("resolve directory %s: %w", path, err) + } + root, err := os.OpenRoot(abs) if err != nil { // Pointing decolint straight at a devcontainer.json is a natural mistake; the open error // alone ("not a directory") does not say what to pass instead. - if info, statErr := os.Stat(path); statErr == nil && !info.IsDir() { - return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", filepath.Clean(path)) + if info, statErr := os.Stat(abs); statErr == nil && !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", absPath(abs)) } // The wrapped error names the path itself, so repeating it here would only make the message // longer. @@ -316,38 +341,22 @@ func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge m return lintDir(ctx, l, subst, merge, root) } -// lintRoot is the devcontainer directory a lint runs on, in the two forms the lint needs it in. -type lintRoot struct { - // name is the directory as it was named on the command line. Every reported path is a file's - // location joined onto it, so findings read back the way the user asked for them. - name string - // abs is where the directory actually is. Values that must mean the same thing however the - // directory was named are resolved from it rather than from name: the workspace folder variables - // substitute to, and the directory names rules compare against. - abs string -} - // lintDir lints every configuration file in the devcontainer directory root is opened on. It is an // error if the directory contains no configuration. func lintDir(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, root *os.Root) ([]linter.Issue, error) { - dir := filepath.Clean(root.Name()) + dir := absPath(root.Name()) if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) } - abs, err := filepath.Abs(dir) - if err != nil { - return nil, fmt.Errorf("resolve directory %s: %w", dir, err) - } - lr := lintRoot{name: dir, abs: abs} var issues []linter.Issue var errs []error found := false - err = discovery.VisitConfigs(root, func(f discovery.ConfigFile) error { + err := discovery.VisitConfigs(root, func(f discovery.ConfigFile) error { found = true if err := ctx.Err(); err != nil { - return fmt.Errorf("aborted %s: %w", filepath.Join(f.Root.Name(), f.Path), err) + return fmt.Errorf("aborted %s: %w", configPath(f), err) } - fileIssues, err := lintFile(ctx, l, subst, merge, f, lr) + fileIssues, err := lintFile(ctx, l, subst, merge, f, dir) if err != nil { // A broken file must not stop the remaining files from being linted, so record the // error and keep visiting. @@ -366,48 +375,45 @@ func lintDir(ctx context.Context, l *linter.Linter, subst substituteFn, merge me return issues, errors.Join(errs...) } -// lintFile reads and lints the single configuration file f, found under the lint root root and -// reported under filepath.Join(f.Root.Name(), f.Path). The file is read through f.Root, so its -// resolution cannot escape that boundary. subst, if non-nil, resolves variables in dev container -// configurations before merge and rules run, so both see resolved values. merge, if non-nil, runs on -// dev container configurations before rules and is skipped when no rule applies to f's type, so a -// file with no active rules does no Feature fetches. -func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, f discovery.ConfigFile, root lintRoot) ([]linter.Issue, error) { - display := filepath.Join(f.Root.Name(), f.Path) +// configPath returns the location of the configuration file f. It is absolute because f.Root is the +// lint root or a sub-root of it, both opened on an absolute path (see lintPath). +func configPath(f discovery.ConfigFile) absPath { + return absPath(filepath.Join(f.Root.Name(), f.Path)) +} + +// lintFile reads and lints the single configuration file f, found in the devcontainer directory dir. +// The file is read through f.Root, so its resolution cannot escape that boundary. subst, if non-nil, +// resolves variables in dev container configurations before merge and rules run, so both see +// resolved values. merge, if non-nil, runs on dev container configurations before rules and is +// skipped when no rule applies to f's type, so a file with no active rules does no Feature fetches. +func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, f discovery.ConfigFile, dir absPath) ([]linter.Issue, error) { + location := configPath(f) src, err := f.Root.ReadFile(f.Path) if err != nil { - return nil, fmt.Errorf("read config %s: %w", display, err) + return nil, fmt.Errorf("read config %s: %w", location, err) } doc, err := linter.ParseDocument(src) if err != nil { - return nil, fmt.Errorf("%s: %w", display, err) + return nil, fmt.Errorf("%s: %w", location, err) } if subst != nil && f.Type == linter.Devcontainer { - // The lint root is the workspace folder the real tooling would mount, even for a + // The linted directory is the workspace folder the real tooling would mount, even for a // configuration discovered in a sub-root like .devcontainer. - subst(root.abs, doc) + subst(string(dir), doc) } if merge != nil && f.Type == linter.Devcontainer && l.HasRules(f.Type) { if err := merge(ctx, f, doc); err != nil { - return nil, fmt.Errorf("merge features %s: %w", display, err) + return nil, fmt.Errorf("merge features %s: %w", location, err) } } // Give rules read access to the config file's directory, confined to the discovery root so // resolution cannot escape it (see linter.Dir). sub, err := fs.Sub(f.Root.FS(), path.Dir(filepath.ToSlash(f.Path))) if err != nil { - return nil, fmt.Errorf("open config dir %s: %w", display, err) - } - return l.LintDocument(display, f.Type, doc, linter.Dir{FS: sub, Name: configDirName(root, display)}), nil -} - -// configDirName returns the name of the directory holding the configuration file reported at -// display. The name is taken from the lint root's absolute location, because display carries none of -// its own when the file sits directly in a root named "." — the default lint target. -func configDirName(root lintRoot, display string) string { - rel, err := filepath.Rel(root.name, filepath.Dir(display)) - if err != nil { - return "" + return nil, fmt.Errorf("open config dir %s: %w", location, err) } - return filepath.Base(filepath.Join(root.abs, rel)) + // The directory's name is taken from the file's own location, which names it whatever the lint + // target was called; the reported path does not, being relative to the working directory. + name := filepath.Base(filepath.Dir(string(location))) + return l.LintDocument(location.String(), f.Type, doc, linter.Dir{FS: sub, Name: name}), nil } diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 075dd26..421bd8a 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -313,6 +313,58 @@ func TestRun_IDDirMismatchNamesTheDirectory(t *testing.T) { } } +// TestRun_ReportedPathsAreWorkingDirectoryRelative checks that findings name files the same way +// however the lint target was named, and that a target outside the working directory — which has no +// relative form to name it by — is named absolutely. +func TestRun_ReportedPathsAreWorkingDirectoryRelative(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + const fixture = "testdata/e2e/violations" + abs, err := filepath.Abs(fixture) + if err != nil { + t.Fatalf("Abs: %v", err) + } + reportedPaths := func(t *testing.T, target string) []string { + t.Helper() + var stdout, stderr bytes.Buffer + run(t.Context(), []string{"-format=json", "-platform=codespaces", target}, &stdout, &stderr) + var issues []linter.Issue + if err := json.Unmarshal(stdout.Bytes(), &issues); err != nil { + t.Fatalf("output is not a JSON issue array: %v\noutput: %s", err, stdout.String()) + } + if len(issues) == 0 { + t.Fatalf("no findings for %s; the fixture is expected to trip codespaces rules", target) + } + var paths []string + for _, issue := range issues { + paths = append(paths, issue.Path) + } + return paths + } + + t.Run("a target inside the working directory is named relative to it", func(t *testing.T) { + want := reportedPaths(t, fixture) + if diff := cmp.Diff(want, reportedPaths(t, abs)); diff != "" { + t.Errorf("paths from the absolute target differ from the relative one (-relative +absolute):\n%s", diff) + } + for _, p := range want { + if !strings.HasPrefix(p, fixture) { + t.Errorf("path = %q, want it under %q", p, fixture) + } + } + }) + + t.Run("a target outside the working directory is named absolutely", func(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + t.Chdir(t.TempDir()) + for _, p := range reportedPaths(t, abs) { + if !filepath.IsAbs(p) { + t.Errorf("path = %q, want an absolute path", p) + } + } + }) +} + func TestRun_Flags(t *testing.T) { t.Parallel() @@ -1437,77 +1489,67 @@ func TestLintPath(t *testing.T) { }) } -func TestConfigDirName(t *testing.T) { - t.Parallel() +func TestAbsPathString(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + wd := t.TempDir() + // t.TempDir can hand back a path through a symlink (/var on macOS), which is not the path the + // process reports as its working directory; ask for the one absPath renders against. + t.Chdir(wd) + wd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } - abs := filepath.Join(string(filepath.Separator), "work", "my-feature") tests := []struct { - name string - root lintRoot - display string - want string + name string + path absPath + want string }{ - { - "lint root named as the working directory", - lintRoot{name: ".", abs: abs}, - "devcontainer-feature.json", - "my-feature", - }, - { - "lint root named relatively", - lintRoot{name: "my-feature", abs: abs}, - filepath.Join("my-feature", "devcontainer-feature.json"), - "my-feature", - }, - { - "lint root named absolutely", - lintRoot{name: abs, abs: abs}, - filepath.Join(abs, "devcontainer-feature.json"), - "my-feature", - }, - { - "configuration in a sub-directory of the lint root", - lintRoot{name: ".", abs: abs}, - filepath.Join(".devcontainer", "devcontainer.json"), - ".devcontainer", - }, - { - // The lint root and the reported path are always named the same way, so this cannot arise - // from a real lint; naming nothing beats naming the wrong directory. - "path unrelatable to the lint root", - lintRoot{name: "my-feature", abs: abs}, - filepath.Join(abs, "devcontainer-feature.json"), - "", - }, + {"inside the working directory", absPath(filepath.Join(wd, ".devcontainer", "devcontainer.json")), filepath.Join(".devcontainer", "devcontainer.json")}, + {"the working directory itself", absPath(wd), "."}, + {"the parent of the working directory", absPath(filepath.Dir(wd)), filepath.Dir(wd)}, + {"a sibling of the working directory", absPath(filepath.Join(filepath.Dir(wd), "elsewhere")), filepath.Join(filepath.Dir(wd), "elsewhere")}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if got := configDirName(tt.root, tt.display); got != tt.want { - t.Errorf("configDirName(%+v, %q) = %q, want %q", tt.root, tt.display, got, tt.want) + if got := tt.path.String(); got != tt.want { + t.Errorf("absPath(%q).String() = %q, want %q", string(tt.path), got, tt.want) } }) } } -func TestLintDir_LocationError(t *testing.T) { +func TestAbsPathString_NoWorkingDirectory(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. - // A relative root name resolves against the working directory; deleting it leaves the linted - // directory with no location to resolve names from, which must surface as an error. + // With the working directory gone there is nothing to render against, so the path stays absolute + // rather than being dropped from the message it appears in. dir := t.TempDir() t.Chdir(dir) - root, err := os.OpenRoot(".") - if err != nil { + if err := os.Remove(dir); err != nil { t.Fatal(err) } - defer func() { _ = root.Close() }() + + p := absPath(filepath.Join(dir, "devcontainer.json")) + if got := p.String(); got != string(p) { + t.Errorf("absPath(%q).String() = %q, want it unchanged", string(p), got) + } +} + +func TestLintPath_LocationError(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + // A relative target resolves against the working directory; deleting it leaves the target with + // no location to resolve, which must surface as an error. + dir := t.TempDir() + t.Chdir(dir) if err := os.Remove(dir); err != nil { t.Fatal(err) } subst := func(string, *linter.Document) {} - if _, err := lintDir(t.Context(), linter.New(), subst, nil, root); err == nil || + if _, err := lintPath(t.Context(), linter.New(), subst, nil, "."); err == nil || !strings.Contains(err.Error(), "resolve directory") { t.Errorf("err = %v, want a directory resolution error", err) } diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index c7e889a..760f8e2 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -102,15 +102,15 @@ func parseOptions(args []string, output io.Writer) (Options, error) { // spelling kept is the first one given, since that is the one the findings are reported under. An // argument whose location cannot be resolved is kept as given, leaving it for the lint to reject. func dedupePaths(paths []string) []string { - seen := make(map[string]bool, len(paths)) + seen := make(map[string]struct{}, len(paths)) kept := make([]string, 0, len(paths)) for _, p := range paths { abs, err := filepath.Abs(p) if err == nil { - if seen[abs] { + if _, ok := seen[abs]; ok { continue } - seen[abs] = true + seen[abs] = struct{}{} } kept = append(kept, p) } diff --git a/feature/compose.go b/feature/compose.go index 1939103..1d997d7 100644 --- a/feature/compose.go +++ b/feature/compose.go @@ -230,6 +230,10 @@ func composeBuildMetadata(ctx context.Context, f *Fetcher, baseDir, firstCompose // composeDisplayRef renders a Dockerfile path for display: relative to the configuration directory // when it lies within it, and absolute otherwise (a build context reached through "extends" or a // "../" Compose file may sit outside it). +// +// The configuration directory, rather than the working directory decolint reports findings against, +// is what this is relative to: the reference identifies a contributor within the merged +// configuration (see [contributor.displayID]), so it must not depend on where decolint was run from. func composeDisplayRef(baseDir, path string) string { if rel, err := filepath.Rel(baseDir, path); err == nil && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return rel diff --git a/format/github.go b/format/github.go index ba225a4..f919b46 100644 --- a/format/github.go +++ b/format/github.go @@ -10,14 +10,11 @@ import ( ) // GitHubFormat prints one GitHub Actions workflow command (::error/::warning) per issue. -type GitHubFormat struct { - // BaseDir is the directory absolute issue paths are made relative to; see [annotationPath]. It - // may be empty, leaving such paths as they are. - BaseDir string -} +type GitHubFormat struct{} -// WriteIssues writes issues to w as GitHub Actions workflow commands. -func (f GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { +// WriteIssues writes issues to w as GitHub Actions workflow commands. Paths are written with "/" +// separators, which is what GitHub matches an annotation against on every runner. +func (GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { for _, issue := range issues { command := "error" if issue.Severity == linter.SeverityWarn { @@ -27,7 +24,7 @@ func (f GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { w, "::%s file=%s,line=%d,col=%d,title=%s::%s\n", command, - escapeGitHubProperty(annotationPath(issue.Path, f.BaseDir)), + escapeGitHubProperty(filepath.ToSlash(issue.Path)), issue.Line, issue.Col, escapeGitHubProperty(issue.RuleID), @@ -40,20 +37,6 @@ func (f GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { return nil } -// annotationPath renders path for a workflow command's "file" property. GitHub places an annotation -// by resolving that property against the repository checkout, so an absolute path lands nowhere: one -// inside baseDir is rewritten relative to it, and separators are normalized to "/", which is what -// GitHub matches on every runner. A path outside baseDir is written as is, since there is nothing -// better to say about it. -func annotationPath(path, baseDir string) string { - if baseDir != "" && filepath.IsAbs(path) { - if rel, err := filepath.Rel(baseDir, path); err == nil && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - path = rel - } - } - return filepath.ToSlash(path) -} - // githubDataReplacer escapes the data (message) portion of a GitHub Actions workflow command, per // https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#escaping-properties. // strings.Replacer is safe for concurrent use, so a single package-level instance is reused across calls. diff --git a/format/github_test.go b/format/github_test.go index 3743dad..b33586c 100644 --- a/format/github_test.go +++ b/format/github_test.go @@ -23,48 +23,20 @@ func TestGitHubWriteIssues(t *testing.T) { } } -func TestAnnotationPath(t *testing.T) { +// TestGitHubWriteIssues_PathSeparators checks that a path built with the host's separators is +// written with "/" ones, which is what GitHub matches an annotation against. +func TestGitHubWriteIssues_PathSeparators(t *testing.T) { t.Parallel() - base := filepath.Join(string(filepath.Separator), "work", "repo") - tests := []struct { - name string - path string - baseDir string - want string - }{ - {"relative path is left alone", ".devcontainer/devcontainer.json", base, ".devcontainer/devcontainer.json"}, - {"absolute path inside the base directory", filepath.Join(base, ".devcontainer", "devcontainer.json"), base, ".devcontainer/devcontainer.json"}, - {"the base directory itself", filepath.Join(base, "devcontainer.json"), base, "devcontainer.json"}, - {"absolute path outside the base directory", filepath.Join(string(filepath.Separator), "elsewhere", "devcontainer.json"), base, "/elsewhere/devcontainer.json"}, - {"no base directory", filepath.Join(base, "devcontainer.json"), "", filepath.ToSlash(filepath.Join(base, "devcontainer.json"))}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if got := annotationPath(tt.path, tt.baseDir); got != tt.want { - t.Errorf("annotationPath(%q, %q) = %q, want %q", tt.path, tt.baseDir, got, tt.want) - } - }) - } -} - -func TestGitHubWriteIssues_AbsolutePath(t *testing.T) { - t.Parallel() - - base := filepath.Join(string(filepath.Separator), "work", "repo") - issues := testIssues() - for i := range issues { - issues[i].Path = filepath.Join(base, filepath.FromSlash(issues[i].Path)) - } + issues := testIssues()[:1] + issues[0].Path = filepath.Join(".devcontainer", "go", "devcontainer.json") var sb strings.Builder - if err := (GitHubFormat{BaseDir: base}).WriteIssues(&sb, issues); err != nil { + if err := (GitHubFormat{}).WriteIssues(&sb, issues); err != nil { t.Fatalf("WriteIssues: %v", err) } - want := `::warning file=.devcontainer/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version -::error file=.devcontainer/devcontainer.json,line=8,col=3,title=some-error-rule::something is broken + want := `::warning file=.devcontainer/go/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version ` if sb.String() != want { t.Errorf("WriteIssues github = %q, want %q", sb.String(), want) From 6bc58423d8f6fd3aba5c6b175be8c25c366590bf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 00:02:12 +0000 Subject: [PATCH 3/7] fix: resolve each lint target once dedupePaths resolved every argument to decide whether it duplicated an earlier one, then threw the result away and returned the original spelling, leaving lintPath to resolve the same argument again. Options.Paths now holds the resolved directories, so lintPath takes them as they are. Deduplication compares the resolved values themselves, and the rule about which spelling to keep goes away with them: rendering no longer depends on how a directory was named, so there is nothing to choose between. Resolving in parseOptions means an argument that cannot be resolved fails there rather than mid-lint, which also makes -version and -rules fail when the working directory has been deleted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KsXG8rvSVXzUGYMe4AR5mi --- cmd/decolint/main.go | 22 +++++++---------- cmd/decolint/main_test.go | 42 ++++++++++---------------------- cmd/decolint/opts.go | 49 ++++++++++++++++++++++---------------- cmd/decolint/opts_test.go | 50 +++++++++++++++++++++++---------------- 4 files changed, 79 insertions(+), 84 deletions(-) diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 54d5185..674b497 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -266,8 +266,8 @@ func runLint(ctx context.Context, stdout, stderr io.Writer, opts Options, cfg Co var allIssues []linter.Issue var worstSeverity linter.Severity var lintErr error - for _, path := range opts.Paths { - issues, err := lintPath(ctx, l, subst, merge, path) + for _, dir := range opts.Paths { + issues, err := lintPath(ctx, l, subst, merge, dir) for _, issue := range issues { if issue.Severity > worstSeverity { worstSeverity = issue.Severity @@ -317,20 +317,16 @@ func (p absPath) String() string { return rel } -// lintPath lints the devcontainer directory at path. The directory is opened as an os.Root on its -// absolute location, so every file the lint reads is confined to it and every path derived from it -// is absolute. It is an error if path is not a directory. -func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, path string) ([]linter.Issue, error) { - abs, err := filepath.Abs(path) - if err != nil { - return nil, fmt.Errorf("resolve directory %s: %w", path, err) - } - root, err := os.OpenRoot(abs) +// lintPath lints the devcontainer directory dir. It is opened as an os.Root, so every file the lint +// reads is confined to it, and because dir is absolute so is every path derived from it. It is an +// error if dir is not a directory. +func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, dir absPath) ([]linter.Issue, error) { + root, err := os.OpenRoot(string(dir)) if err != nil { // Pointing decolint straight at a devcontainer.json is a natural mistake; the open error // alone ("not a directory") does not say what to pass instead. - if info, statErr := os.Stat(abs); statErr == nil && !info.IsDir() { - return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", absPath(abs)) + if info, statErr := os.Stat(string(dir)); statErr == nil && !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", dir) } // The wrapped error names the path itself, so repeating it here would only make the message // longer. diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 421bd8a..687ee97 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -810,7 +810,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{dir}}, Config{}) + hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []absPath{absPath(dir)}}, Config{}) if runErr != nil || hasIssue { t.Errorf("hasIssue = %v, err = %v, want false, nil; stdout: %s", hasIssue, runErr, stdout.String()) } @@ -821,7 +821,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - opts := Options{Paths: []string{dir}} + opts := Options{Paths: []absPath{absPath(dir)}} cfg := Config{Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || !hasIssue { @@ -834,7 +834,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{}`) // triggers missing-container-def (error by default) var stdout bytes.Buffer - opts := Options{Paths: []string{dir}} + opts := Options{Paths: []absPath{absPath(dir)}} cfg := Config{Rules: map[string]linter.Severity{"missing-container-def": linter.SeverityOff}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || hasIssue { @@ -862,7 +862,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) // no-image-latest is in reproducibility var stdout bytes.Buffer - opts := Options{Paths: []string{dir}} + opts := Options{Paths: []absPath{absPath(dir)}} cfg := Config{Categories: map[string]linter.Severity{"reproducibility": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || !hasIssue { @@ -891,7 +891,7 @@ func TestRunLint(t *testing.T) { file := filepath.Join(dir, ".devcontainer", "devcontainer.json") var stdout bytes.Buffer - hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{file}}, Config{}) + hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []absPath{absPath(file)}}, Config{}) if runErr == nil || hasIssue { t.Errorf("hasIssue = %v, err = %v, want false, 'not a directory'", hasIssue, runErr) } @@ -902,7 +902,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - opts := Options{Paths: []string{dir}} + opts := Options{Paths: []absPath{absPath(dir)}} cfg := Config{Rules: map[string]linter.Severity{"no-bind-mount": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || hasIssue { @@ -1368,7 +1368,7 @@ func TestLintPath(t *testing.T) { t.Fatal(err) } - issues, err := lintPath(t.Context(), newLinter(), noSubst, nil, dir) + issues, err := lintPath(t.Context(), newLinter(), noSubst, nil, absPath(dir)) if err != nil { t.Fatalf("lintPath: %v", err) } @@ -1415,7 +1415,7 @@ func TestLintPath(t *testing.T) { t.Run("directory without config", func(t *testing.T) { t.Parallel() - _, err := lintPath(t.Context(), newLinter(), noSubst, nil, t.TempDir()) + _, err := lintPath(t.Context(), newLinter(), noSubst, nil, absPath(t.TempDir())) if err == nil || !strings.Contains(err.Error(), "no devcontainer configuration found") { t.Errorf("err = %v, want 'no devcontainer configuration found'", err) } @@ -1424,7 +1424,7 @@ func TestLintPath(t *testing.T) { t.Run("a configuration file is not a directory", func(t *testing.T) { t.Parallel() file := filepath.Join(writeDevcontainer(t, body), ".devcontainer", "devcontainer.json") - _, err := lintPath(t.Context(), newLinter(), noSubst, nil, file) + _, err := lintPath(t.Context(), newLinter(), noSubst, nil, absPath(file)) if err == nil || !strings.Contains(err.Error(), "is not a directory; pass the directory") { t.Errorf("err = %v, want the error to say a directory is expected", err) } @@ -1436,7 +1436,7 @@ func TestLintPath(t *testing.T) { wantErr := errors.New("fetch failed") merge := func(context.Context, discovery.ConfigFile, *linter.Document) error { return wantErr } - if _, err := lintPath(t.Context(), newLinter(), noSubst, merge, dir); !errors.Is(err, wantErr) { + if _, err := lintPath(t.Context(), newLinter(), noSubst, merge, absPath(dir)); !errors.Is(err, wantErr) { t.Errorf("lintPath error = %v, want %v", err, wantErr) } }) @@ -1450,7 +1450,7 @@ func TestLintPath(t *testing.T) { return nil } - if _, err := lintPath(t.Context(), linter.New(), noSubst, merge, dir); err != nil { + if _, err := lintPath(t.Context(), linter.New(), noSubst, merge, absPath(dir)); err != nil { t.Fatalf("lintPath: %v", err) } if called { @@ -1480,7 +1480,7 @@ func TestLintPath(t *testing.T) { } subst := func(string, *linter.Document) {} - if _, err := lintPath(t.Context(), l, subst, merge, dir); err != nil { + if _, err := lintPath(t.Context(), l, subst, merge, absPath(dir)); err != nil { t.Fatalf("lintPath: %v", err) } if called { @@ -1537,24 +1537,6 @@ func TestAbsPathString_NoWorkingDirectory(t *testing.T) { } } -func TestLintPath_LocationError(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - // A relative target resolves against the working directory; deleting it leaves the target with - // no location to resolve, which must surface as an error. - dir := t.TempDir() - t.Chdir(dir) - if err := os.Remove(dir); err != nil { - t.Fatal(err) - } - - subst := func(string, *linter.Document) {} - if _, err := lintPath(t.Context(), linter.New(), subst, nil, "."); err == nil || - !strings.Contains(err.Error(), "resolve directory") { - t.Errorf("err = %v, want a directory resolution error", err) - } -} - // TestRun_Substitution checks that ${...} variables resolve only under -merge, since substitution // and merging together compute the effective configuration: with merging on, no-image-latest // reports the resolved image reference; with merging off, it reports the raw ${localEnv:...} text. diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 760f8e2..633d8cb 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -10,11 +10,13 @@ import ( "github.com/bare-devcontainer/decolint/linter" ) -// Options holds the parsed command-line arguments. It is purely the CLI's view of the world; see -// Config for the on-disk config file's shape, and mergeConfig for how the two are reconciled. +// Options holds the parsed command-line arguments. Apart from the directories to lint, which are +// resolved to the locations they name, it is purely the CLI's view of the world; see Config for the +// on-disk config file's shape, and mergeConfig for how the two are reconciled. type Options struct { - // Paths are the directories to lint. - Paths []string + // Paths are the directories to lint, resolved so that each names the same directory however it + // was spelled on the command line (see absPath). Defaults to the working directory. + Paths []absPath // DenyWarnings mirrors [Config.DenyWarnings]. When -deny-warnings is explicitly given it takes // precedence over the config file's "denyWarnings" member, in either direction (see // denyWarningsSet and mergeConfig). @@ -89,32 +91,37 @@ func parseOptions(args []string, output io.Writer) (Options, error) { // config file's "format" member is merged in, so both sources go through one validation path. opts.Format = formatFlag - opts.Paths = dedupePaths(fs.Args()) - if len(opts.Paths) == 0 { - opts.Paths = []string{"."} + targets := fs.Args() + if len(targets) == 0 { + targets = []string{"."} + } + opts.Paths, err = resolvePaths(targets) + if err != nil { + return Options{}, err } return opts, nil } -// dedupePaths drops arguments naming a directory an earlier argument already names, so that passing -// e.g. both "." and its absolute path lints it once rather than reporting every finding twice. The -// spelling kept is the first one given, since that is the one the findings are reported under. An -// argument whose location cannot be resolved is kept as given, leaving it for the lint to reject. -func dedupePaths(paths []string) []string { - seen := make(map[string]struct{}, len(paths)) - kept := make([]string, 0, len(paths)) +// resolvePaths resolves each argument to the directory it names, dropping one an earlier argument +// already names so that passing e.g. both "." and its absolute path lints it once rather than +// reporting every finding twice. +func resolvePaths(paths []string) ([]absPath, error) { + seen := make(map[absPath]struct{}, len(paths)) + resolved := make([]absPath, 0, len(paths)) for _, p := range paths { abs, err := filepath.Abs(p) - if err == nil { - if _, ok := seen[abs]; ok { - continue - } - seen[abs] = struct{}{} + if err != nil { + return nil, fmt.Errorf("resolve directory %s: %w", p, err) + } + dir := absPath(abs) + if _, ok := seen[dir]; ok { + continue } - kept = append(kept, p) + seen[dir] = struct{}{} + resolved = append(resolved, dir) } - return kept + return resolved, nil } // parsePlatforms parses a comma-separated list of platform names into a slice of linter.Platform. diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index 878020a..c6e8b7f 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -3,7 +3,9 @@ package main import ( "fmt" "io" + "os" "path/filepath" + "strings" "testing" "github.com/bare-devcontainer/decolint/linter" @@ -223,39 +225,30 @@ func TestParseOptions_Config(t *testing.T) { } func TestParseOptions_Paths(t *testing.T) { - t.Parallel() - - opts, err := parseOptions(nil, io.Discard) - if err != nil { - t.Fatalf("parseOptions: %v", err) - } - if diff := cmp.Diff([]string{"."}, opts.Paths); diff != "" { - t.Errorf("Paths mismatch (-want +got):\n%s", diff) - } -} - -func TestParseOptions_PathsDeduped(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. dir := t.TempDir() t.Chdir(dir) - // t.TempDir can hand back a path through a symlink (/var on macOS), which resolves to the same - // directory as "." but not to the same absolute path; ask for the one deduplication compares. + // t.TempDir can hand back a path through a symlink (/var on macOS), which names the same + // directory as "." but does not resolve to the same absolute path; ask for the one arguments + // resolve to. abs, err := filepath.Abs(".") if err != nil { t.Fatalf("Abs: %v", err) } + sub := filepath.Join(abs, "sub") tests := []struct { name string args []string - want []string + want []absPath }{ - {"the same directory spelled two ways", []string{".", abs}, []string{"."}}, - {"the first spelling is the one kept", []string{abs, "."}, []string{abs}}, - {"repeated argument", []string{abs, abs}, []string{abs}}, - {"trailing separator", []string{".", "." + string(filepath.Separator)}, []string{"."}}, - {"distinct directories are all kept", []string{".", filepath.Join(abs, "sub")}, []string{".", filepath.Join(abs, "sub")}}, + {"no arguments lint the working directory", nil, []absPath{absPath(abs)}}, + {"a relative argument", []string{"."}, []absPath{absPath(abs)}}, + {"the same directory spelled two ways", []string{".", abs}, []absPath{absPath(abs)}}, + {"a repeated argument", []string{abs, abs}, []absPath{absPath(abs)}}, + {"a trailing separator", []string{".", "." + string(filepath.Separator)}, []absPath{absPath(abs)}}, + {"distinct directories are all kept", []string{".", sub}, []absPath{absPath(abs), absPath(sub)}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -269,3 +262,20 @@ func TestParseOptions_PathsDeduped(t *testing.T) { }) } } + +func TestParseOptions_PathsLocationError(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + // A relative argument resolves against the working directory; deleting it leaves the argument + // with no location to resolve, which must surface as an error. + dir := t.TempDir() + t.Chdir(dir) + if err := os.Remove(dir); err != nil { + t.Fatal(err) + } + + if _, err := parseOptions([]string{"."}, io.Discard); err == nil || + !strings.Contains(err.Error(), "resolve directory") { + t.Errorf("err = %v, want a directory resolution error", err) + } +} From 36c4650813ea1e93bfa464a39854d57659a65d48 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 00:30:45 +0000 Subject: [PATCH 4/7] fix: resolve lint targets where they are linted, not where they are parsed Holding the resolved directories in Options gave parseOptions a dependency on the working directory and an error that has nothing to do with parsing a command line, which is all Options is meant to describe. It also let a deleted working directory fail -version and -rules, which name no directory at all. Options.Paths goes back to the arguments as named, and runLint resolves and deduplicates them before linting. Each target is still resolved once: lintPath keeps taking an absPath and does not resolve anything itself. resolvePaths moves next to absPath, so the whole of the CLI's path resolution now sits in one file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KsXG8rvSVXzUGYMe4AR5mi --- cmd/decolint/main.go | 28 +++++++++++- cmd/decolint/main_test.go | 90 ++++++++++++++++++++++++++++++++++++--- cmd/decolint/opts.go | 43 ++++--------------- cmd/decolint/opts_test.go | 46 +++----------------- 4 files changed, 126 insertions(+), 81 deletions(-) diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 674b497..96f3485 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -263,10 +263,15 @@ func runLint(ctx context.Context, stdout, stderr io.Writer, opts Options, cfg Co } } + targets, err := resolvePaths(opts.Paths) + if err != nil { + return false, err + } + var allIssues []linter.Issue var worstSeverity linter.Severity var lintErr error - for _, dir := range opts.Paths { + for _, dir := range targets { issues, err := lintPath(ctx, l, subst, merge, dir) for _, issue := range issues { if issue.Severity > worstSeverity { @@ -317,6 +322,27 @@ func (p absPath) String() string { return rel } +// resolvePaths resolves each of the directories named on the command line to the location it names, +// dropping one an earlier argument already names so that passing e.g. both "." and its absolute path +// lints it once rather than reporting every finding twice. +func resolvePaths(paths []string) ([]absPath, error) { + seen := make(map[absPath]struct{}, len(paths)) + resolved := make([]absPath, 0, len(paths)) + for _, p := range paths { + abs, err := filepath.Abs(p) + if err != nil { + return nil, fmt.Errorf("resolve directory %s: %w", p, err) + } + dir := absPath(abs) + if _, ok := seen[dir]; ok { + continue + } + seen[dir] = struct{}{} + resolved = append(resolved, dir) + } + return resolved, nil +} + // lintPath lints the devcontainer directory dir. It is opened as an os.Root, so every file the lint // reads is confined to it, and because dir is absolute so is every path derived from it. It is an // error if dir is not a directory. diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 687ee97..ce700a2 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -802,6 +802,30 @@ func TestRun_Init(t *testing.T) { }) } +func TestRunLint_UnresolvableTarget(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + // A target that cannot be resolved fails the run before any file is read, so no findings are + // written either. + dir := t.TempDir() + t.Chdir(dir) + if err := os.Remove(dir); err != nil { + t.Fatal(err) + } + + var stdout bytes.Buffer + hasIssue, err := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{"."}}, Config{}) + if err == nil || !strings.Contains(err.Error(), "resolve directory") { + t.Errorf("err = %v, want a directory resolution error", err) + } + if hasIssue { + t.Error("hasIssue = true, want false") + } + if stdout.Len() != 0 { + t.Errorf("stdout = %q, want empty", stdout.String()) + } +} + func TestRunLint(t *testing.T) { t.Parallel() @@ -810,7 +834,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []absPath{absPath(dir)}}, Config{}) + hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{dir}}, Config{}) if runErr != nil || hasIssue { t.Errorf("hasIssue = %v, err = %v, want false, nil; stdout: %s", hasIssue, runErr, stdout.String()) } @@ -821,7 +845,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - opts := Options{Paths: []absPath{absPath(dir)}} + opts := Options{Paths: []string{dir}} cfg := Config{Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || !hasIssue { @@ -834,7 +858,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{}`) // triggers missing-container-def (error by default) var stdout bytes.Buffer - opts := Options{Paths: []absPath{absPath(dir)}} + opts := Options{Paths: []string{dir}} cfg := Config{Rules: map[string]linter.Severity{"missing-container-def": linter.SeverityOff}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || hasIssue { @@ -862,7 +886,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) // no-image-latest is in reproducibility var stdout bytes.Buffer - opts := Options{Paths: []absPath{absPath(dir)}} + opts := Options{Paths: []string{dir}} cfg := Config{Categories: map[string]linter.Severity{"reproducibility": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || !hasIssue { @@ -891,7 +915,7 @@ func TestRunLint(t *testing.T) { file := filepath.Join(dir, ".devcontainer", "devcontainer.json") var stdout bytes.Buffer - hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []absPath{absPath(file)}}, Config{}) + hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{file}}, Config{}) if runErr == nil || hasIssue { t.Errorf("hasIssue = %v, err = %v, want false, 'not a directory'", hasIssue, runErr) } @@ -902,7 +926,7 @@ func TestRunLint(t *testing.T) { dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) var stdout bytes.Buffer - opts := Options{Paths: []absPath{absPath(dir)}} + opts := Options{Paths: []string{dir}} cfg := Config{Rules: map[string]linter.Severity{"no-bind-mount": linter.SeverityError}} hasIssue, runErr := runLint(t.Context(), &stdout, io.Discard, opts, cfg) if runErr != nil || hasIssue { @@ -1489,6 +1513,60 @@ func TestLintPath(t *testing.T) { }) } +func TestResolvePaths(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + t.Chdir(t.TempDir()) + // t.TempDir can hand back a path through a symlink (/var on macOS), which names the same + // directory as "." but does not resolve to the same absolute path; ask for the one arguments + // resolve to. + wd, err := filepath.Abs(".") + if err != nil { + t.Fatalf("Abs: %v", err) + } + sub := filepath.Join(wd, "sub") + + tests := []struct { + name string + paths []string + want []absPath + }{ + {"a relative argument", []string{"."}, []absPath{absPath(wd)}}, + {"the same directory spelled two ways", []string{".", wd}, []absPath{absPath(wd)}}, + {"a repeated argument", []string{wd, wd}, []absPath{absPath(wd)}}, + {"a trailing separator", []string{".", "." + string(filepath.Separator)}, []absPath{absPath(wd)}}, + {"distinct directories are all kept", []string{".", sub}, []absPath{absPath(wd), absPath(sub)}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolvePaths(tt.paths) + if err != nil { + t.Fatalf("resolvePaths: %v", err) + } + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("resolvePaths(%q) mismatch (-want +got):\n%s", tt.paths, diff) + } + }) + } +} + +func TestResolvePaths_LocationError(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + // A relative argument resolves against the working directory; deleting it leaves the argument + // with no location to resolve, which must surface as an error. + dir := t.TempDir() + t.Chdir(dir) + if err := os.Remove(dir); err != nil { + t.Fatal(err) + } + + if _, err := resolvePaths([]string{"."}); err == nil || + !strings.Contains(err.Error(), "resolve directory") { + t.Errorf("err = %v, want a directory resolution error", err) + } +} + func TestAbsPathString(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 633d8cb..7df7c3f 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -4,19 +4,17 @@ import ( "flag" "fmt" "io" - "path/filepath" "strings" "github.com/bare-devcontainer/decolint/linter" ) -// Options holds the parsed command-line arguments. Apart from the directories to lint, which are -// resolved to the locations they name, it is purely the CLI's view of the world; see Config for the -// on-disk config file's shape, and mergeConfig for how the two are reconciled. +// Options holds the parsed command-line arguments. It is purely the CLI's view of the world; see +// Config for the on-disk config file's shape, and mergeConfig for how the two are reconciled. type Options struct { - // Paths are the directories to lint, resolved so that each names the same directory however it - // was spelled on the command line (see absPath). Defaults to the working directory. - Paths []absPath + // Paths are the directories to lint, as named on the command line; runLint resolves them (see + // resolvePaths). + Paths []string // DenyWarnings mirrors [Config.DenyWarnings]. When -deny-warnings is explicitly given it takes // precedence over the config file's "denyWarnings" member, in either direction (see // denyWarningsSet and mergeConfig). @@ -91,39 +89,14 @@ func parseOptions(args []string, output io.Writer) (Options, error) { // config file's "format" member is merged in, so both sources go through one validation path. opts.Format = formatFlag - targets := fs.Args() - if len(targets) == 0 { - targets = []string{"."} - } - opts.Paths, err = resolvePaths(targets) - if err != nil { - return Options{}, err + opts.Paths = fs.Args() + if len(opts.Paths) == 0 { + opts.Paths = []string{"."} } return opts, nil } -// resolvePaths resolves each argument to the directory it names, dropping one an earlier argument -// already names so that passing e.g. both "." and its absolute path lints it once rather than -// reporting every finding twice. -func resolvePaths(paths []string) ([]absPath, error) { - seen := make(map[absPath]struct{}, len(paths)) - resolved := make([]absPath, 0, len(paths)) - for _, p := range paths { - abs, err := filepath.Abs(p) - if err != nil { - return nil, fmt.Errorf("resolve directory %s: %w", p, err) - } - dir := absPath(abs) - if _, ok := seen[dir]; ok { - continue - } - seen[dir] = struct{}{} - resolved = append(resolved, dir) - } - return resolved, nil -} - // parsePlatforms parses a comma-separated list of platform names into a slice of linter.Platform. // An empty string yields a nil slice. func parsePlatforms(s string) ([]linter.Platform, error) { diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index c6e8b7f..ec71d16 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -3,9 +3,6 @@ package main import ( "fmt" "io" - "os" - "path/filepath" - "strings" "testing" "github.com/bare-devcontainer/decolint/linter" @@ -225,33 +222,21 @@ func TestParseOptions_Config(t *testing.T) { } func TestParseOptions_Paths(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - dir := t.TempDir() - t.Chdir(dir) - // t.TempDir can hand back a path through a symlink (/var on macOS), which names the same - // directory as "." but does not resolve to the same absolute path; ask for the one arguments - // resolve to. - abs, err := filepath.Abs(".") - if err != nil { - t.Fatalf("Abs: %v", err) - } - sub := filepath.Join(abs, "sub") + t.Parallel() + // Arguments are taken as given; resolving and deduplicating them is runLint's job (see + // TestResolvePaths). tests := []struct { name string args []string - want []absPath + want []string }{ - {"no arguments lint the working directory", nil, []absPath{absPath(abs)}}, - {"a relative argument", []string{"."}, []absPath{absPath(abs)}}, - {"the same directory spelled two ways", []string{".", abs}, []absPath{absPath(abs)}}, - {"a repeated argument", []string{abs, abs}, []absPath{absPath(abs)}}, - {"a trailing separator", []string{".", "." + string(filepath.Separator)}, []absPath{absPath(abs)}}, - {"distinct directories are all kept", []string{".", sub}, []absPath{absPath(abs), absPath(sub)}}, + {"no arguments lint the working directory", nil, []string{"."}}, + {"arguments are kept as named", []string{".", "/work/repo", "."}, []string{".", "/work/repo", "."}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() opts, err := parseOptions(tt.args, io.Discard) if err != nil { t.Fatalf("parseOptions: %v", err) @@ -262,20 +247,3 @@ func TestParseOptions_Paths(t *testing.T) { }) } } - -func TestParseOptions_PathsLocationError(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - // A relative argument resolves against the working directory; deleting it leaves the argument - // with no location to resolve, which must surface as an error. - dir := t.TempDir() - t.Chdir(dir) - if err := os.Remove(dir); err != nil { - t.Fatal(err) - } - - if _, err := parseOptions([]string{"."}, io.Discard); err == nil || - !strings.Contains(err.Error(), "resolve directory") { - t.Errorf("err = %v, want a directory resolution error", err) - } -} From e385049579b2b8753a1009c1ede21455d3d08691 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 00:45:47 +0000 Subject: [PATCH 5/7] fix: lint each target in one pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolvePaths walked the arguments to build a slice that runLint then walked again, so resolving, deduplicating and linting now happen in the one loop over the arguments. Folding them together also settles how a target that cannot be resolved is treated. It used to abort the run before anything was written, while every other per-target failure is recorded and the remaining targets are linted — the posture lintDir already takes for a broken file. Resolution failures now join the same way, so a bad target no longer discards the findings of the good ones. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KsXG8rvSVXzUGYMe4AR5mi --- cmd/decolint/main.go | 41 ++++++----------- cmd/decolint/main_test.go | 96 +++++++++++++++------------------------ cmd/decolint/opts.go | 3 +- 3 files changed, 52 insertions(+), 88 deletions(-) diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 96f3485..42ceaf0 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -263,15 +263,23 @@ func runLint(ctx context.Context, stdout, stderr io.Writer, opts Options, cfg Co } } - targets, err := resolvePaths(opts.Paths) - if err != nil { - return false, err - } - var allIssues []linter.Issue var worstSeverity linter.Severity var lintErr error - for _, dir := range targets { + // A directory named more than once is linted once, so its findings are not reported twice. + seen := make(map[absPath]struct{}, len(opts.Paths)) + for _, target := range opts.Paths { + abs, err := filepath.Abs(target) + if err != nil { + lintErr = errors.Join(lintErr, fmt.Errorf("resolve directory %s: %w", target, err)) + continue + } + dir := absPath(abs) + if _, ok := seen[dir]; ok { + continue + } + seen[dir] = struct{}{} + issues, err := lintPath(ctx, l, subst, merge, dir) for _, issue := range issues { if issue.Severity > worstSeverity { @@ -322,27 +330,6 @@ func (p absPath) String() string { return rel } -// resolvePaths resolves each of the directories named on the command line to the location it names, -// dropping one an earlier argument already names so that passing e.g. both "." and its absolute path -// lints it once rather than reporting every finding twice. -func resolvePaths(paths []string) ([]absPath, error) { - seen := make(map[absPath]struct{}, len(paths)) - resolved := make([]absPath, 0, len(paths)) - for _, p := range paths { - abs, err := filepath.Abs(p) - if err != nil { - return nil, fmt.Errorf("resolve directory %s: %w", p, err) - } - dir := absPath(abs) - if _, ok := seen[dir]; ok { - continue - } - seen[dir] = struct{}{} - resolved = append(resolved, dir) - } - return resolved, nil -} - // lintPath lints the devcontainer directory dir. It is opened as an os.Root, so every file the lint // reads is confined to it, and because dir is absolute so is every path derived from it. It is an // error if dir is not a directory. diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index ce700a2..3135fe7 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -802,11 +802,43 @@ func TestRun_Init(t *testing.T) { }) } +func TestRunLint_DeduplicatesTargets(t *testing.T) { + t.Parallel() + + // The same directory named twice must be linted once, so its findings are reported once. + dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) + cfg := Config{ + Format: "json", + Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityError}, + } + + lint := func(t *testing.T, paths []string) int { + t.Helper() + var stdout bytes.Buffer + if _, err := runLint(t.Context(), &stdout, io.Discard, Options{Paths: paths}, cfg); err != nil { + t.Fatalf("runLint: %v", err) + } + var issues []linter.Issue + if err := json.Unmarshal(stdout.Bytes(), &issues); err != nil { + t.Fatalf("output is not a JSON issue array: %v\noutput: %s", err, stdout.String()) + } + return len(issues) + } + + want := lint(t, []string{dir}) + if want == 0 { + t.Fatal("no findings for the fixture; the test cannot tell deduplication from a silent lint") + } + if got := lint(t, []string{dir, dir + string(filepath.Separator)}); got != want { + t.Errorf("findings for the directory named twice = %d, want %d", got, want) + } +} + func TestRunLint_UnresolvableTarget(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. - // A target that cannot be resolved fails the run before any file is read, so no findings are - // written either. + // A target whose location cannot be resolved is recorded like any other per-target failure, so + // the findings collected so far are still written. dir := t.TempDir() t.Chdir(dir) if err := os.Remove(dir); err != nil { @@ -814,15 +846,15 @@ func TestRunLint_UnresolvableTarget(t *testing.T) { } var stdout bytes.Buffer - hasIssue, err := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{"."}}, Config{}) + hasIssue, err := runLint(t.Context(), &stdout, io.Discard, Options{Paths: []string{"."}}, Config{Format: "json"}) if err == nil || !strings.Contains(err.Error(), "resolve directory") { t.Errorf("err = %v, want a directory resolution error", err) } if hasIssue { t.Error("hasIssue = true, want false") } - if stdout.Len() != 0 { - t.Errorf("stdout = %q, want empty", stdout.String()) + if got := strings.TrimSpace(stdout.String()); got != "[]" { + t.Errorf("stdout = %q, want an empty JSON issue array", got) } } @@ -1513,60 +1545,6 @@ func TestLintPath(t *testing.T) { }) } -func TestResolvePaths(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - t.Chdir(t.TempDir()) - // t.TempDir can hand back a path through a symlink (/var on macOS), which names the same - // directory as "." but does not resolve to the same absolute path; ask for the one arguments - // resolve to. - wd, err := filepath.Abs(".") - if err != nil { - t.Fatalf("Abs: %v", err) - } - sub := filepath.Join(wd, "sub") - - tests := []struct { - name string - paths []string - want []absPath - }{ - {"a relative argument", []string{"."}, []absPath{absPath(wd)}}, - {"the same directory spelled two ways", []string{".", wd}, []absPath{absPath(wd)}}, - {"a repeated argument", []string{wd, wd}, []absPath{absPath(wd)}}, - {"a trailing separator", []string{".", "." + string(filepath.Separator)}, []absPath{absPath(wd)}}, - {"distinct directories are all kept", []string{".", sub}, []absPath{absPath(wd), absPath(sub)}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := resolvePaths(tt.paths) - if err != nil { - t.Fatalf("resolvePaths: %v", err) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("resolvePaths(%q) mismatch (-want +got):\n%s", tt.paths, diff) - } - }) - } -} - -func TestResolvePaths_LocationError(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - // A relative argument resolves against the working directory; deleting it leaves the argument - // with no location to resolve, which must surface as an error. - dir := t.TempDir() - t.Chdir(dir) - if err := os.Remove(dir); err != nil { - t.Fatal(err) - } - - if _, err := resolvePaths([]string{"."}); err == nil || - !strings.Contains(err.Error(), "resolve directory") { - t.Errorf("err = %v, want a directory resolution error", err) - } -} - func TestAbsPathString(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 7df7c3f..d6e75d3 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -12,8 +12,7 @@ import ( // Options holds the parsed command-line arguments. It is purely the CLI's view of the world; see // Config for the on-disk config file's shape, and mergeConfig for how the two are reconciled. type Options struct { - // Paths are the directories to lint, as named on the command line; runLint resolves them (see - // resolvePaths). + // Paths are the directories to lint, as named on the command line; runLint resolves them. Paths []string // DenyWarnings mirrors [Config.DenyWarnings]. When -deny-warnings is explicitly given it takes // precedence over the config file's "denyWarnings" member, in either direction (see From 554b85e0f1d02e92be754f3d48e9322e6ee63e37 Mon Sep 17 00:00:00 2001 From: nozaq Date: Sat, 25 Jul 2026 01:18:58 +0000 Subject: [PATCH 6/7] tidy up comments --- .devcontainer/devcontainer.json | 2 +- cmd/decolint/main.go | 23 +++++++++++------------ cmd/decolint/main_test.go | 13 +++++-------- cmd/decolint/opts_test.go | 2 +- feature/compose.go | 11 +++++------ feature/compose_test.go | 7 +++---- format/github.go | 2 +- linter/rule.go | 11 +++++------ rules/helpers_test.go | 5 ++--- rules/id_dir_mismatch_test.go | 6 +++--- 10 files changed, 37 insertions(+), 45 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 88e621a..c3d5858 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -29,7 +29,7 @@ "vscode": { "extensions": [ // renovate: datasource=github-tags depName=anthropics/claude-code - "anthropic.claude-code@2.1.206", + "anthropic.claude-code@2.1.219", // renovate: datasource=github-tags depName=editorconfig/editorconfig-vscode "EditorConfig.EditorConfig@0.18.2", // renovate: datasource=github-tags depName=golang/vscode-go diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 42ceaf0..4b352ea 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -311,13 +311,13 @@ type mergeFn func(ctx context.Context, f discovery.ConfigFile, doc *linter.Docum type substituteFn func(workspaceFolder string, doc *linter.Document) // absPath is the location of a file or directory, always absolute so that it means the same thing -// however the lint target was named on the command line. Printing it renders it for the reader; see -// [absPath.String]. +// however the lint target was named on the command line. Printing it does not yield that absolute +// form; see [absPath.String]. type absPath string // String renders p the way findings and error messages name it: relative to the working directory -// when it is inside it, absolute otherwise. A path outside the working directory has no relative -// form worth reading, and neither has one when the working directory cannot be determined. +// when it is inside it, absolute otherwise — a path reached by climbing out of the working directory +// reads no better than its own location. func (p absPath) String() string { wd, err := os.Getwd() if err != nil { @@ -331,18 +331,17 @@ func (p absPath) String() string { } // lintPath lints the devcontainer directory dir. It is opened as an os.Root, so every file the lint -// reads is confined to it, and because dir is absolute so is every path derived from it. It is an -// error if dir is not a directory. +// reads is confined to it; because dir is absolute, so is every path derived from it. It is an error +// if dir is not a directory. func lintPath(ctx context.Context, l *linter.Linter, subst substituteFn, merge mergeFn, dir absPath) ([]linter.Issue, error) { root, err := os.OpenRoot(string(dir)) if err != nil { - // Pointing decolint straight at a devcontainer.json is a natural mistake; the open error - // alone ("not a directory") does not say what to pass instead. + // Pointing decolint straight at a devcontainer.json is a natural mistake, and "not a + // directory" alone does not say what to pass instead. if info, statErr := os.Stat(string(dir)); statErr == nil && !info.IsDir() { return nil, fmt.Errorf("%s is not a directory; pass the directory that contains the devcontainer configuration", dir) } - // The wrapped error names the path itself, so repeating it here would only make the message - // longer. + // os.OpenRoot's error already names the path. return nil, fmt.Errorf("lint directory: %w", err) } // The root is only read from, so a close error is inconsequential. @@ -421,8 +420,8 @@ func lintFile(ctx context.Context, l *linter.Linter, subst substituteFn, merge m if err != nil { return nil, fmt.Errorf("open config dir %s: %w", location, err) } - // The directory's name is taken from the file's own location, which names it whatever the lint - // target was called; the reported path does not, being relative to the working directory. + // The reported path can be relative to the containing directory itself, naming no directory at + // all, so the name comes from the absolute location. name := filepath.Base(filepath.Dir(string(location))) return l.LintDocument(location.String(), f.Type, doc, linter.Dir{FS: sub, Name: name}), nil } diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 3135fe7..4753bba 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -268,9 +268,8 @@ func TestRun(t *testing.T) { } // TestRun_IDDirMismatchNamesTheDirectory checks that id-dir-mismatch judges a Feature by the -// directory it sits in rather than by how that directory was named on the command line. Naming it -// from inside, which is what the default lint target does, leaves the reported path with no -// directory component, and the rule must still name the directory the Feature is in. +// directory it sits in rather than by how that directory was named on the command line. The default +// lint target names it from inside, leaving the reported path with no directory component. func TestRun_IDDirMismatchNamesTheDirectory(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. @@ -314,8 +313,8 @@ func TestRun_IDDirMismatchNamesTheDirectory(t *testing.T) { } // TestRun_ReportedPathsAreWorkingDirectoryRelative checks that findings name files the same way -// however the lint target was named, and that a target outside the working directory — which has no -// relative form to name it by — is named absolutely. +// however the lint target was named, and that a target outside the working directory is named +// absolutely. func TestRun_ReportedPathsAreWorkingDirectoryRelative(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. @@ -805,7 +804,6 @@ func TestRun_Init(t *testing.T) { func TestRunLint_DeduplicatesTargets(t *testing.T) { t.Parallel() - // The same directory named twice must be linted once, so its findings are reported once. dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) cfg := Config{ Format: "json", @@ -1579,8 +1577,7 @@ func TestAbsPathString(t *testing.T) { func TestAbsPathString_NoWorkingDirectory(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel. - // With the working directory gone there is nothing to render against, so the path stays absolute - // rather than being dropped from the message it appears in. + // With the working directory gone there is nothing to render against, so the path stays absolute. dir := t.TempDir() t.Chdir(dir) if err := os.Remove(dir); err != nil { diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index ec71d16..d9c48d8 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -225,7 +225,7 @@ func TestParseOptions_Paths(t *testing.T) { t.Parallel() // Arguments are taken as given; resolving and deduplicating them is runLint's job (see - // TestResolvePaths). + // TestRunLint_DeduplicatesTargets). tests := []struct { name string args []string diff --git a/feature/compose.go b/feature/compose.go index 1d997d7..6a237a7 100644 --- a/feature/compose.go +++ b/feature/compose.go @@ -43,9 +43,8 @@ func composeContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, confi return nil, true, nil } // The Compose files resolve relative to the referencing devcontainer.json's directory on the real - // filesystem, as "docker compose" itself would. The directory is resolved to an absolute path so - // that compose-go resolves the build context to one too (see composeBuildMetadata), which the - // display of a Dockerfile reached through it depends on (see composeDisplayRef). + // filesystem, as "docker compose" itself would. The directory is made absolute so that compose-go + // resolves the build context to an absolute path too, which composeDisplayRef relies on. baseDir, err := filepath.Abs(filepath.Join(fsRoot.Name(), configDir)) if err != nil { return nil, true, fmt.Errorf("resolve %s: %w", configDir, err) @@ -231,9 +230,9 @@ func composeBuildMetadata(ctx context.Context, f *Fetcher, baseDir, firstCompose // when it lies within it, and absolute otherwise (a build context reached through "extends" or a // "../" Compose file may sit outside it). // -// The configuration directory, rather than the working directory decolint reports findings against, -// is what this is relative to: the reference identifies a contributor within the merged -// configuration (see [contributor.displayID]), so it must not depend on where decolint was run from. +// It is deliberately not relative to the working directory: the reference identifies a contributor +// within the merged configuration (see [contributor.displayID]), so it must not depend on where +// decolint ran. func composeDisplayRef(baseDir, path string) string { if rel, err := filepath.Rel(baseDir, path); err == nil && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return rel diff --git a/feature/compose_test.go b/feature/compose_test.go index a9e757c..eea8654 100644 --- a/feature/compose_test.go +++ b/feature/compose_test.go @@ -343,16 +343,15 @@ func TestMerge_ComposeDockerfileRefIsRootIndependent(t *testing.T) { repo := t.TempDir() writeFiles(t, repo, map[string]string{ ".devcontainer/devcontainer.json": `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, - // The build context sits outside the .devcontainer directory, so the reference cannot be a - // name relative to it and must name the Dockerfile's own location instead. + // The build context sits outside the .devcontainer directory, so the reference must name the + // Dockerfile's own location rather than a name relative to the configuration directory. ".devcontainer/docker-compose.yml": "services:\n app:\n build:\n context: ..\n", "Dockerfile": "FROM scratch\nRUN --bogus=1 x\n", }) configDir := filepath.Join(repo, ".devcontainer") want := filepath.Join(repo, "Dockerfile") - // The lint root is named relative to the working directory in one run and absolutely in the - // other; both name the same directory. + // The lint root is named relative to the working directory in one run and absolutely in the other. t.Chdir(repo) for _, dir := range []string{".devcontainer", configDir} { t.Run(dir, func(t *testing.T) { diff --git a/format/github.go b/format/github.go index f919b46..910b2c7 100644 --- a/format/github.go +++ b/format/github.go @@ -13,7 +13,7 @@ import ( type GitHubFormat struct{} // WriteIssues writes issues to w as GitHub Actions workflow commands. Paths are written with "/" -// separators, which is what GitHub matches an annotation against on every runner. +// separators, which is what GitHub matches an annotation against. func (GitHubFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { for _, issue := range issues { command := "error" diff --git a/linter/rule.go b/linter/rule.go index 60cc656..a076d1f 100644 --- a/linter/rule.go +++ b/linter/rule.go @@ -138,9 +138,9 @@ func ParseCategory(name string) (Category, error) { // Context carries everything a rule needs to inspect a single configuration file. type Context struct { - // Path is the path of the file being linted. It is meant for reporting: its spelling follows - // whatever the caller named the file, so a rule must not read anything into its shape (see - // [Dir] for the containing directory). + // Path is the path of the file being linted, for reporting only: it is spelled however the + // caller named the file, so a rule must not read anything into its shape. Use [Dir] for the + // containing directory. Path string // Type is the kind of configuration file being linted. Type FileType @@ -157,9 +157,8 @@ type Dir struct { // FS gives read access to the directory's contents, confined to the lint root. It is nil when // the caller has no backing filesystem, e.g. an in-memory document. FS fs.FS - // Name is the directory's own name, resolved from where the directory actually is. It is not - // derived from [Context.Path], which carries no name of its own when the caller names the file - // relative to the directory itself. It is empty when the caller has no directory to name. + // Name is the directory's own name, taken from where the directory actually is rather than from + // [Context.Path]. It is empty when the caller has no directory to name. Name string } diff --git a/rules/helpers_test.go b/rules/helpers_test.go index 5ee1e97..3c0f6f8 100644 --- a/rules/helpers_test.go +++ b/rules/helpers_test.go @@ -48,9 +48,8 @@ func assertIssuesAt(t *testing.T, r *linter.Rule, severity linter.Severity, path } // assertIssuesInDir is like assertIssuesAt but lets the caller supply the directory the file is -// linted in (see [linter.Dir]), so rules that inspect sibling files can be exercised against a fake -// filesystem (e.g. a [testing/fstest.MapFS]) and rules that read the directory's name against a -// name of the caller's choosing. +// linted in (see [linter.Dir]): a fake filesystem (e.g. a [testing/fstest.MapFS]) for rules that +// inspect sibling files, and a directory name for rules that read it. func assertIssuesInDir(t *testing.T, r *linter.Rule, severity linter.Severity, path string, fileType linter.FileType, src string, dir linter.Dir, want []linter.Issue) { t.Helper() diff --git a/rules/id_dir_mismatch_test.go b/rules/id_dir_mismatch_test.go index 0e5eb57..b2d53cd 100644 --- a/rules/id_dir_mismatch_test.go +++ b/rules/id_dir_mismatch_test.go @@ -10,9 +10,9 @@ import ( func TestIDDirMismatch(t *testing.T) { t.Parallel() - // The reported path and the containing directory's name are supplied separately, as the linter - // supplies them: the path is only how the file is named in output, and carries no directory - // component of its own when the lint target is the directory the file sits in. + // The reported path and the directory's name are supplied separately, as the linter supplies + // them: the path is only how the file is named in output (see [linter.Context.Path]), so the + // cases below vary the two independently. const featurePath = "my-feature/devcontainer-feature.json" const templatePath = "my-template/devcontainer-template.json" From e727239f4fc5841f528c9b6d72862295683acaf2 Mon Sep 17 00:00:00 2001 From: nozaq Date: Sat, 25 Jul 2026 01:33:25 +0000 Subject: [PATCH 7/7] test: assert the directory name rules see, not one rule's message The directory name a rule is handed comes from the file's location on disk rather than the path findings report it at. That was checked through id-dir-mismatch's message and the JSON output, tying the test to the only rule that reads the name. Observe it with a stub rule at lintPath instead. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/decolint/main_test.go | 115 +++++++++++++++++++++++--------------- 1 file changed, 70 insertions(+), 45 deletions(-) diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 4753bba..706aa1e 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -267,51 +267,6 @@ func TestRun(t *testing.T) { } } -// TestRun_IDDirMismatchNamesTheDirectory checks that id-dir-mismatch judges a Feature by the -// directory it sits in rather than by how that directory was named on the command line. The default -// lint target names it from inside, leaving the reported path with no directory component. -func TestRun_IDDirMismatchNamesTheDirectory(t *testing.T) { - // Uses t.Chdir, which cannot be combined with t.Parallel. - - // The fixture's id deliberately does not match its directory, so the rule fires in every run and - // the message it fires with is what distinguishes the runs. - const want = `id "wrong-id" does not match containing directory "feature"` - abs, err := filepath.Abs("testdata/e2e/feature") - if err != nil { - t.Fatalf("Abs: %v", err) - } - - tests := []struct { - name string - args []string - }{ - {"named from outside", []string{abs}}, - {"named as the working directory", []string{"."}}, - {"not named at all", nil}, - } - t.Chdir(abs) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var stdout, stderr bytes.Buffer - run(t.Context(), append([]string{"-format=json"}, tt.args...), &stdout, &stderr) - - var issues []linter.Issue - if err := json.Unmarshal(stdout.Bytes(), &issues); err != nil { - t.Fatalf("output is not a JSON issue array: %v\noutput: %s", err, stdout.String()) - } - var got []string - for _, issue := range issues { - if issue.RuleID == "id-dir-mismatch" { - got = append(got, issue.Message) - } - } - if diff := cmp.Diff([]string{want}, got); diff != "" { - t.Errorf("id-dir-mismatch messages mismatch (-want +got):\n%s", diff) - } - }) - } -} - // TestRun_ReportedPathsAreWorkingDirectoryRelative checks that findings name files the same way // however the lint target was named, and that a target outside the working directory is named // absolutely. @@ -1543,6 +1498,76 @@ func TestLintPath(t *testing.T) { }) } +// dirNameRule is a stub rule that reports the name of the directory the linted file sits in, used to +// observe the [linter.Dir] a lint hands to rules without depending on a rule that reads it. +var dirNameRule = &linter.Rule{ + ID: "test-dir-name", + Description: "reports the containing directory's name", + FileTypes: []linter.FileType{linter.Feature}, + Paths: []string{"/id"}, + Check: func(ctx *linter.Context, node *linter.Node) []linter.Finding { + return []linter.Finding{{Message: ctx.Dir.Name, Offset: node.Value.StartOffset}} + }, +} + +// TestLintPath_DirName checks that rules are handed the name of the directory the configuration file +// sits in wherever the lint runs from, while the path the finding is reported at follows the working +// directory. Linting a directory from inside it leaves that path with no directory component, so the +// name cannot be taken from there. +func TestLintPath_DirName(t *testing.T) { + // Uses t.Chdir, which cannot be combined with t.Parallel. + + parent := t.TempDir() + // t.TempDir can hand back a path through a symlink (/var on macOS), which is not the path the + // process reports as its working directory; ask for the one findings are reported against. + t.Chdir(parent) + parent, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + target := filepath.Join(parent, "my-feature") + sibling := filepath.Join(parent, "elsewhere") + for _, dir := range []string{target, sibling} { + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(target, "devcontainer-feature.json"), []byte(`{"id": "my-feature"}`), 0o644); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + wd string + wantPath string + }{ + {"from the directory itself", target, "devcontainer-feature.json"}, + {"from its parent", parent, filepath.Join("my-feature", "devcontainer-feature.json")}, + {"from outside", sibling, filepath.Join(target, "devcontainer-feature.json")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Chdir(tt.wd) + l := linter.New() + l.RegisterRule(dirNameRule, linter.SeverityWarn) + + issues, err := lintPath(t.Context(), l, nil, nil, absPath(target)) + if err != nil { + t.Fatalf("lintPath: %v", err) + } + if len(issues) != 1 { + t.Fatalf("got %d issues %v, want 1", len(issues), issues) + } + if issues[0].Message != "my-feature" { + t.Errorf("ctx.Dir.Name = %q, want %q", issues[0].Message, "my-feature") + } + if issues[0].Path != tt.wantPath { + t.Errorf("Path = %q, want %q", issues[0].Path, tt.wantPath) + } + }) + } +} + func TestAbsPathString(t *testing.T) { // Uses t.Chdir, which cannot be combined with t.Parallel.