Add lintroller rule to prevent log level checks outside logger package

[osterman]

what

• Add new log-level-checks lintroller rule to prevent using log levels to control UI behavior
• Rule detects atmosConfig.Logs.Level accesses and comparisons outside logger package
• Exceptions for logger package itself and test files

why

• Log levels are internal implementation details and should not control UI behavior
• UI elements (spinners, progress indicators) should be controlled by explicit configuration flags
• Prevents anti-pattern of checking log levels to conditionally show/hide UI elements
• Enforces separation of concerns between logging and UI

references

• Identifies existing violations in the codebase (to be fixed in follow-up PR)
• Rule is enabled by default in .golangci.yml
• Comprehensive test coverage in rule_log_level_checks_test.go
• Documentation added to tools/lintroller/README.md

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a lint rule that flags access or comparisons of log-level values outside the logger package.

• Documentation

  • Updated linting docs to describe the new rule, its exceptions, and examples.

• Tests

  • Added unit tests and testdata covering allowed and disallowed log-level patterns.

• Chores

  • Enabled the new rule by default in the linting configuration.

[coderabbitai[bot]]

📝 Walkthrough

Walkthrough

Adds a new lintroller rule that detects direct access or comparisons of Logs.Level outside the logger package, exposes a log-level-checks settings flag (enabled by default), updates docs, and adds implementation, tests, and testdata.

Changes

Cohort / File(s)	Summary
Configuration \.golangci.yml	Updated lintroller.description to include log-level and enabled new setting settings.custom.lintroller.log-level-checks: true.
Documentation tools/lintroller/README.md	Documented the new log-level-checks rule, examples (Bad/Good), exceptions, and added rule_log_level_checks.go to architecture/files and rules list.
Plugin Setup tools/lintroller/plugin.go	Added LogLevelChecks bool to Settings, defaulted it to true when using defaults, exposed in plugin settings, and registered LogLevelChecksRule in standalone/default rule sets and run path.
Rule Implementation tools/lintroller/rule_log_level_checks.go	Added linters.LogLevelChecksRule which scans ASTs for Logs.Level selector access and binary comparisons of log level constants, skips logger package and test files, and deduplicates reports.
Rule Tests tools/lintroller/rule_log_level_checks_test.go	Added unit tests constructing in-memory ASTs to assert diagnostics for disallowed and allowed patterns.
Testdata tools/lintroller/testdata/src/a/log_levels.go, tools/lintroller/testdata/src/a/bad_test.go	Added constants, Config/LogsConfig types, example functions showing bad/good patterns, and a _test.go demonstrating allowed checks in test files.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Runner as golangci-lint / lintroller plugin
  participant Plugin as lintroller.Plugin
  participant Analyzer as analysis.Pass
  participant Rule as LogLevelChecksRule
  participant AST as ast.File
  participant Reporter as pass.Report

  Runner->>Plugin: load plugin (settings include log-level-checks)
  Plugin->>Analyzer: include LogLevelChecksRule in analyzers
  Analyzer->>AST: parse source file
  Analyzer->>Rule: Check(pass, ast.File)
  Note right of Rule #EFEFEF: skip files in logger package and *_test.go
  Rule->>AST: traverse nodes (SelectorExpr, BinaryExpr)
  alt detect Logs.Level access or level comparison
    Rule->>Reporter: emit diagnostic (pos, message)
  else
    Rule-->>Analyzer: no diagnostic
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• cloudposse/atmos#1698 — Adds new lint rules and similar plugin.go settings and rule registration changes.

Suggested labels

minor

Suggested reviewers

• aknysh

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Add lintroller rule to prevent log level checks outside logger package" directly corresponds to the main objective of the changeset: introducing a new linting rule called log-level-checks that flags log level comparisons and accesses occurring outside the logger package. The title is specific, concise, and clearly articulates what's being added and why, rather than being vague or overly broad. It encompasses the core change across all modified files—from the rule implementation and tests to the configuration and documentation updates.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment
• [ ] Commit unit tests in branch osterman/no-log-level-checks

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 90e7e25e0e1e5643f5d4d003681dd3a0b8211cad and e967c6cf9728611ebb60ed3bb721d18b12a839c3.

📒 Files selected for processing (4)

• .golangci.yml (1 hunks)
• tools/lintroller/plugin.go (6 hunks)
• tools/lintroller/rule_log_level_checks.go (1 hunks)
• tools/lintroller/testdata/src/a/log_levels.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• .golangci.yml

🧰 Additional context used

📓 Path-based instructions (3)

**/*.go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

**/*.go: All code must pass golangci-lint checks Follow Go error handling idioms and use meaningful error messages Wrap errors with context using fmt.Errorf("context: %w", err) Consider custom error types for domain-specific errors Follow standard Go coding style; run gofmt and goimports Use snake_case for environment variables Document complex logic with inline comments

**/*.go: Generate mocks with go.uber.org/mock/mockgen using //go:generate directives; never write manual mocks Use functional options pattern instead of functions with many parameters Use context.Context only for cancellation, deadlines, and narrowly-scoped request values; never for config or dependencies; context should be the first parameter All comments must end with periods (godot linter) Organize imports in three groups with blank lines (stdlib, third-party, then Atmos packages) and sort alphabetically; keep aliases: cfg, log, u, errUtils Error handling: wrap with static errors from errors/errors.go, combine with errors.Join, add context with fmt.Errorf("%w: msg", err), check with errors.Is; never compare strings or create dynamic error types Use //go:generate go run go.uber.org/mock/mockgen@latest -source= -destination=test.go for mocks Keep files small and focused (<600 lines), one command/impl per file; co-locate tests; never use //revive:disable:file-length-limit Bind environment variables with viper.BindEnv and use ATMOS prefix UI (prompts/status) must write to stderr; data outputs to stdout; logging only for system events, not UI Ensure cross-platform compatibility: prefer SDKs over binaries and use filepath.Join() instead of hardcoded separators

Files:

• tools/lintroller/plugin.go
• tools/lintroller/rule_log_level_checks.go
• tools/lintroller/testdata/src/a/log_levels.go

**/!(*_test).go

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Document all exported functions, types, and methods with Go doc comments

Files:

• tools/lintroller/plugin.go
• tools/lintroller/rule_log_level_checks.go
• tools/lintroller/testdata/src/a/log_levels.go

**/testdata/**

📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Use test fixtures for complex inputs under testdata

Files:

• tools/lintroller/testdata/src/a/log_levels.go

🧠 Learnings (1)

📚 Learning: 2024-12-25T20:28:47.526Z

Learnt from: osterman
PR: cloudposse/atmos#887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.

Applied to files:

• tools/lintroller/rule_log_level_checks.go

🧬 Code graph analysis (2)

tools/lintroller/plugin.go (1)

tools/lintroller/rule_log_level_checks.go (1)

• LogLevelChecksRule (13-13)

tools/lintroller/testdata/src/a/log_levels.go (2)

pkg/schema/schema.go (1)

• Logs (404-407)

pkg/logger/log.go (1)

• Level (171-171)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: Build (windows)
• GitHub Check: Build (macos)
• GitHub Check: Build (linux)
• GitHub Check: Analyze (go)
• GitHub Check: Lint (golangci)
• GitHub Check: Run pre-commit hooks
• GitHub Check: autofix
• GitHub Check: Review Dependency Licenses
• GitHub Check: Summary

🔇 Additional comments (11)

tools/lintroller/testdata/src/a/log_levels.go (1)

1-44: Testdata looks solid.

The test fixtures properly cover both prohibited patterns (log level comparisons/accesses) and the approved pattern (explicit flags). The "want" comments correctly specify expected diagnostics for each violation.

tools/lintroller/plugin.go (4)

15-15: Clean integration into rule set.

The rule is properly wired into both the standalone analyzer and the doc strings. Follows the established pattern.

Also applies to: 28-28, 85-85

---

50-50: Settings field properly defined.

The LogLevelChecks field follows the established naming and tagging pattern for plugin settings.

---

67-67: Default enabling works correctly.

The rule is properly enabled by default when no explicit settings are provided, matching the behavior of other rules.

Also applies to: 73-73

---

115-117: Conditional activation implemented correctly.

The rule is conditionally included based on settings, following the same pattern as existing rules.

tools/lintroller/rule_log_level_checks.go (6)

1-13: Clean setup with proper imports.

The package declaration, imports, and type definition are all correct. Cross-platform support is in place via path/filepath.

---

15-23: Interface methods properly implemented.

Both Name() and Doc() have doc comments and correct return values. Previous feedback has been addressed.

---

25-45: File filtering handles cross-platform paths correctly.

The use of filepath.ToSlash ensures the path check works on Windows. The dual check (path and package name) for the logger package, plus test file exclusion, properly implements the rule's scope.

---

47-87: AST inspection logic is solid.

The dual-case handling (comparisons vs. accesses) with duplicate prevention ensures each violation is reported exactly once. The error messages clearly explain why the pattern is prohibited.

---

89-115: Helper methods implement correct detection logic.

isLogsLevelAccess catches the .Logs.Level pattern regardless of variable name. isLogLevelComparison properly uses token constants and checks both operands. Previous feedback has been addressed.

---

117-135: Pattern detection covers the necessary cases.

referencesLogLevel correctly identifies both u.LogLevel* constants and .Logs.Level accesses. The logic ensures comparisons are caught even when one operand is a bare identifier.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 66.91%. Comparing base (3ace69f) to head (e967c6c). :warning: Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1699      +/-   ##
==========================================
+ Coverage   66.87%   66.91%   +0.03%     
==========================================
  Files         368      368              
  Lines       42960    42960              
==========================================
+ Hits        28731    28746      +15     
+ Misses      12135    12121      -14     
+ Partials     2094     2093       -1     

Flag	Coverage Δ
unittests	66.91% <ø> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more. see 3 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions[bot]]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[mergify[bot]]

💥 This pull request now has conflicts. Could you fix it @osterman? 🙏