Fix false-positive heredoc detection on bare (( )) arithmetic in generatedyamlheredoc linter#53991
(( )) arithmetic in generatedyamlheredoc linter#53991Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
(( )) arithmetic in generatedyamlheredoc linter
PR Triage
WIP placeholder PR (0 diff) for bare arithmetic heredoc misreporting fix; no substantive changes yet.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53991 does not have the 'implementation' label and has only 4 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. This PR modified production code in the generatedyamlheredoc linter and added test data inputs (testdata/), but no behavioral test functions (*_test.go) were added or changed.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review with no GitHub write because no actionable review comments were warranted; the changed linter logic and tests look correct, and the only candidate concern was speculative.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving; one minor comment on the guard's precision.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: the guard is broadened from
$((to((, which is the correct minimal change. - Regression tests added: both new arithmetic patterns (
(( count << 1 ))andif (( a << 2 )); then) are covered by testdata.
Positive Highlights
- ✅ Minimal, surgical diff — only the lines that needed to change were touched.
- ✅ Test cases correctly validate the absence of false positives.
- ✅ PR description is clear and self-contained.
Minor observation
The inline comment says "$((" is also matched here, since it contains "((" as a suffix — technically (( is a substring (at offset 1), not a suffix. Consider: // "$((" is also covered since it contains "((" as a substring.
Also worth noting: broadening to (( will now skip any shell string that contains a bare (( before a <<. This is the correct trade-off for a false-positive fix in this domain, but a short code comment documenting the known limitation would help future maintainers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.2 AIC · ⌖ 9.87 AIC · ⊞ 7.8K
Comment /matt to run again
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
The fix is correct and minimal. Changing "$((" to "((" properly handles bare (( )) arithmetic expressions where << is a left-shift operator, not a heredoc. The inline comment clarifies that "$((" is a special case of "((", and the two new test cases cover the regression.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21 AIC · ⌖ 8.79 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Pull request overview
Broadens heredoc detection to recognize bare Bash arithmetic commands using (( )).
Changes:
- Matches
((as an arithmetic context. - Adds bare arithmetic shift fixtures.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/generatedyamlheredoc/generatedyamlheredoc.go |
Expands arithmetic detection. |
pkg/linters/generatedyamlheredoc/testdata/src/generatedyamlheredoc/generatedyamlheredoc.go |
Adds regression fixtures. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| } | ||
| if strings.LastIndex(line[:operatorIndex], "$((") > strings.LastIndex(line[:operatorIndex], "))") { | ||
| // "$((" is also matched here, since it contains "((" as a suffix. | ||
| if strings.LastIndex(line[:operatorIndex], "((") > strings.LastIndex(line[:operatorIndex], "))") { |
Final pass complete. |
|
@copilot PR is close, but there is still maintainer-visible follow-up needed. Please complete one final pass:
Open review threads (newest first):
Checks are no longer running on the current head, so this is ready for that final maintainer-facing sweep.
|
…heredoc-fix-arithmetic-reporting Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed in |
PR Triage
Batch: B2-linter-fixes Small, well-scoped linter fix (add:53/del:1) for a false-positive heredoc detection bug. CI mostly green. Automated triage by PR Triage Agent.
|
|
@copilot This PR still needs a forward-progress pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…heredoc-fix-arithmetic-reporting Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Investigated the failing CGO run/job logs: |
|
@copilot Sous-chef triage: please investigate this PR and move it toward merge readiness. Refresh the branch if needed, review any unresolved feedback, and run the
|
PR finisher pass complete. |
|
@copilot quick triage for this PR: Unresolved review threads:
Please refresh the branch if needed, address the items above, and then run the
|
PR finisher triage complete. |
PR TriageCategory: bug | Risk: low | Priority: medium (score 43/100)
|
lineContainsShellHeredoconly recognized$((as an arithmetic-expansion context guard for<<, so bare(( expr << n ))arithmetic commands/conditionals were misreported as shell heredocs.Changes
pkg/linters/generatedyamlheredoc/generatedyamlheredoc.go: Broadened the arithmetic guard to search for((instead of$((. Since$((always contains((as a suffix, a single((search covers both spellings without redundant logic.pkg/linters/generatedyamlheredoc/testdata/src/generatedyamlheredoc/generatedyamlheredoc.go: Added test cases for(( count << 1 ))andif (( a << 2 )); thento lock in the fix, alongside the existing$((1 << 2))case.(( ... ))arithmetic with<<is misreported as a heredoc #53901run: https://github.com/github/gh-aw/actions/runs/32276767055> Generated by 👨🍳 PR Sous Chef · gpt54 · 26.4 AIC · ⌖ 9.42 AIC · ⊞ 9.3K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32326521694> Generated by 👨🍳 PR Sous Chef · gpt54 · 11.9 AIC · ⌖ 8.15 AIC · ⊞ 9.3K · ◷