feat: add substitution-key rule - #64
Open
shreyaGupta1202 wants to merge 3 commits into
Open
shreyaGupta1202 wants to merge 3 commits into
shreyaGupta1202 wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds the deterministic ChangesSubstitution-key lint rule
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant lintCLI
participant rulesAll
participant substitutionKeyRule
lintCLI->>rulesAll: Load registered lint rules
rulesAll-->>lintCLI: Return substitutionKeyRule
lintCLI->>substitutionKeyRule: Apply rule selection and lint values
substitutionKeyRule-->>lintCLI: Return malformed-substitution findings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/lint/rules/deterministic/substitution_key.go`:
- Around line 145-153: Ignore unsupported braced expansions in the
substitution-key rule: update the validation around validKeyName so only $KEY
and ${KEY} forms are evaluated, while forms such as ${KEY:-fallback} return no
finding. In internal/lint/rules/deterministic/substitution_key_test.go lines
43-66, replace the invalid-key expectation with cases confirming unsupported
expansions are ignored. In docs/lint/rules/deterministic/substitution-key.md
lines 18-21, remove the claim that non-portable braced names are reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3033e85b-20a3-4e9a-8d7a-964ead6d57db
📒 Files selected for processing (8)
docs/lint/rules/README.mddocs/lint/rules/deterministic/substitution-key.mdinternal/cli/lint_test.gointernal/lint/rules/all.gointernal/lint/rules/compat.gointernal/lint/rules/deterministic/substitution_key.gointernal/lint/rules/deterministic/substitution_key_test.gointernal/lint/rules/fixability_test.go
Collaborator
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 33 minutes. |
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new deterministic lint rule (substitution-key) to Vaar’s dotenv linter to detect malformed $KEY / ${KEY} substitution syntax inside assignment values, addressing #46 and reducing parser-to-parser ambiguity before deployment/runtime.
Changes:
- Implemented
substitution-keyrule to report missing closing}, unmatched extra}, and empty${}(while skipping single-quoted and unbalanced-quote values). - Registered the rule in the canonical rule set and compatibility constructors; added fixtures and CLI tests for
--only/--skip. - Documented the new rule in the lint rule index and added a dedicated rule page with examples and sample output.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/lint/rules/fixability_test.go | Adds a fixture for substitution-key to ensure fixability behavior stays pinned. |
| internal/lint/rules/deterministic/substitution_key.go | Implements the deterministic substitution-key rule and scanning helpers. |
| internal/lint/rules/deterministic/substitution_key_test.go | Adds unit tests covering valid/invalid substitutions, quoting, comments, and multiple findings. |
| internal/lint/rules/compat.go | Exposes the new rule via the compat constructor. |
| internal/lint/rules/all.go | Registers substitution-key in the canonical rule list. |
| internal/cli/lint_test.go | Adds end-to-end CLI tests for --only=substitution-key and --skip=substitution-key. |
| docs/lint/rules/README.md | Adds substitution-key to the rule catalog table. |
| docs/lint/rules/deterministic/substitution-key.md | Provides full rule documentation with examples and expected output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/lint/rules/deterministic/substitution_key.go`:
- Around line 121-130: Update the missing-closing-brace branch in the
substitution key rule to detect and ignore non-empty unsupported braced
expansion candidates such as `${KEY:-fallback` before creating the lint finding,
while preserving errors for genuinely malformed supported substitutions. Add a
test covering this ignored expansion behavior in the existing substitution-key
ignored-expansion test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9908c5f8-bfdb-4623-964e-f4949c05455a
📒 Files selected for processing (8)
docs/lint/rules/README.mddocs/lint/rules/deterministic/substitution-key.mdinternal/cli/lint_test.gointernal/lint/rules/all.gointernal/lint/rules/compat.gointernal/lint/rules/deterministic/substitution_key.gointernal/lint/rules/deterministic/substitution_key_test.gointernal/lint/rules/fixability_test.go
Comment on lines
+121
to
+130
| closeOffset := strings.IndexByte(value[start+2:], '}') | ||
| if closeOffset < 0 { | ||
| finding := finding( | ||
| substitutionKeyRule{}.ID(), | ||
| lint.SeverityError, | ||
| path, | ||
| line, | ||
| fmt.Sprintf(`substitution %q is missing a closing "}"`, value[start:]), | ||
| ) | ||
| return &finding, len(value) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignore unclosed unsupported braced expansions.
${KEY:-fallback reaches this missing-brace branch and is reported, despite unsupported expansion syntax being out of scope. Classify a non-empty invalid candidate before emitting the finding, and add this case to the ignored-expansion tests.
Proposed fix
if closeOffset < 0 {
+ candidate := value[start+2:]
+ if candidate != "" && !validKeyName(candidate) {
+ return nil, len(value)
+ }
finding := finding(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/lint/rules/deterministic/substitution_key.go` around lines 121 -
130, Update the missing-closing-brace branch in the substitution key rule to
detect and ignore non-empty unsupported braced expansion candidates such as
`${KEY:-fallback` before creating the lint finding, while preserving errors for
genuinely malformed supported substitutions. Add a test covering this ignored
expansion behavior in the existing substitution-key ignored-expansion test
suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #46. Add the deterministic
substitution-keylint rule for malformed$KEYand${KEY}syntax in dotenv assignment values.Why
Malformed substitution syntax can be interpreted differently across dotenv parsers, shells and deployment tools. Vaar should report incomplete, empty or mismatched substitution syntax before the file reaches an application or deployment environment.
Type
Breaking change
Changes
substitution-keydeterministic lint rule and registered it in the canonical rule set.--only,--skipand fixability behavior.User-visible changes
Before:
These malformed substitution values were not reported by a dedicated rule.
After:
Validation
Commands run:
gofmt -w <touched Go files>make lintgo test ./...go run ./cmd/vaar lint ...Additional commands:
Tests
Documentation
Release notes
Add the
substitution-keylint rule to report malformed$KEYand${KEY}substitution syntax in dotenv assignment values.Reviewer notes
Please pay special attention to substitution parsing scope: the rule scans unquoted and double-quoted assignment values, ignores recognized inline comments, treats single-quoted values as literal text, and does not expand values or check whether referenced keys exist.
Checklist
Summary by CodeRabbit
substitution-keylint rule to detect malformed$KEY/${KEY}syntax in dotenv assignment values.${}, and unmatched extra}with precise, line-based errors.--only/--skipand skips scanning inside single-quoted values.