Skip to content

Fix false-positive heredoc detection on bare (( )) arithmetic in generatedyamlheredoc linter - #53991

Open
pelikhan with Copilot wants to merge 7 commits into
mainfrom
copilot/generatedyamlheredoc-fix-arithmetic-reporting
Open

Fix false-positive heredoc detection on bare (( )) arithmetic in generatedyamlheredoc linter#53991
pelikhan with Copilot wants to merge 7 commits into
mainfrom
copilot/generatedyamlheredoc-fix-arithmetic-reporting

Conversation

Copilot AI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

lineContainsShellHeredoc only 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 )) and if (( a << 2 )); then to lock in the fix, alongside the existing $((1 << 2)) case.
// Previously flagged as a heredoc (false positive), now correctly ignored:
"(( count << 1 ))\n"
"if (( a << 2 )); then\n"

run: https://github.com/github/gh-aw/actions/runs/32276767055> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 26.4 AIC · ⌖ 9.42 AIC · ⊞ 9.3K ·

Comment /souschef to run again

Copilot AI and others added 2 commits August 19, 2026 12:24
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix misreporting of bare arithmetic with heredocs Fix false-positive heredoc detection on bare (( )) arithmetic in generatedyamlheredoc linter Aug 19, 2026
Copilot AI requested a review from pelikhan August 19, 2026 12:26
@github-actions

Copy link
Copy Markdown
Contributor

PR Triage

  • Category: bug
  • Risk: low
  • Priority score: 10/100 (impact 5 + urgency 3 + quality 2)
  • Recommended action: defer

WIP placeholder PR (0 diff) for bare arithmetic heredoc misreporting fix; no substantive changes yet.

Generated by 🔧 PR Triage Agent · auto · 44.3 AIC · ⌖ 2.78 AIC · ⊞ 8.3K ·

@pelikhan
pelikhan marked this pull request as ready for review August 19, 2026 12:26
Copilot AI balanced review requested due to automatic review settings August 19, 2026 12:27
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #53991

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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).

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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.

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )) and if (( 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

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

reviewed_at: 2026-08-19T00:00:00Z
review_event: COMMENT
top_themes:
  - no actionable issues in changed lines
  - arithmetic false-positive guard covered by added tests
files_reviewed:
  - pkg/linters/generatedyamlheredoc/generatedyamlheredoc.go
  - pkg/linters/generatedyamlheredoc/testdata/src/generatedyamlheredoc/generatedyamlheredoc.go
comment_count: 0

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 2.62 AIC · ⌖ 6.73 AIC · ⊞ 7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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], "))") {
@github-actions github-actions Bot mentioned this pull request Aug 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for this fix! 🎯 The PR correctly addresses the bug where bare (( )) arithmetic syntax was being flagged as shell heredocs. The change to search for (( instead of $(( is elegant — since $(( always contains (( as a suffix, this single check covers both cases without redundancy.

This PR looks ready for review:

  • Focused change: one logical fix to the arithmetic-expansion guard
  • Tests included: new test cases for (( count << 1 )) and if (( a << 2 )); then lock in the fix
  • Clear description: well-documented rationale and changes
  • Low risk: minimal diff affecting only linter logic and tests for the same component

Great work shipping this through the agentic workflow!

Generated by ✅ Contribution Check · auto · 50.1 AIC · ⌖ 10.5 AIC · ⊞ 9.2K ·

Copilot AI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot This PR looks close, but it still needs a maintainer-facing finish pass.

  • Please refresh the branch if needed....

Finish pass complete. The branch already matches the PR base, so no refresh was needed. Validation: make fmt, make test-unit, make test, and make recompile passed; make lint is blocked only by three pre-existing testifylint findings in pkg/intent/governance_formal_test.go. No failed CI job is present (one cache job was cancelled; Copilot remains in progress). Remaining risks: the unresolved inline review thread still needs maintainer disposition, and the current CI check must complete before merge.

Copilot AI requested a review from gh-aw-bot August 19, 2026 13:23
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot This PR looks close, but it still needs a maintainer-facing finish pass.

  • Please refresh the branch if GitHub offers one.
  • There is still an unresolved inline review thread to resolve or answer.
  • Then run the pr-finisher skill and leave a concise maintainer-facing handoff comment with validation and any remaining risks.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 26.4 AIC · ⌖ 9.42 AIC · ⊞ 9.3K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot This PR looks close, but it still needs a maintainer-facing finish pass.

  • Please refresh the branch if GitHub offers one....

Addressed in 80a38f4: quoted (( no longer suppresses a following heredoc, with the requested printf … '((' && cat <<EOF regression fixture. The branch was already refreshed with main. Validation passed: make agent-report-progress, make test, and make recompile; CodeQL found no alerts. The inline review is addressed by this commit, but this environment does not expose a thread-resolution operation. Remaining risk: CI on the new head has not been rerun by this agent.

@github-actions

Copy link
Copy Markdown
Contributor

PR Triage

Category: bug · Risk: low · Score: 40/100 (impact 15 + urgency 10 + quality 15)
Recommended action: fast_track
Batch: B2-linter-fixes

Linter false-positive fix, 2x APPROVED reviews; ready pending CI green.

Automated triage — see run report for full details.

Generated by 🔧 PR Triage Agent · auto · 90.2 AIC · ⌖ 2.82 AIC · ⊞ 8.3K ·

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot PR is close, but there is still maintainer-visible follow-up needed.

Please complete one final pass:

  • Resolve or answer the remaining unresolved inline review thread directly.
  • Refresh the branch if GitHub offers one.
  • Then run the pr-finisher skill and leave a concise maintainer-facing handoff comment with validation and remaining risks.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 31.4 AIC · ⌖ 8.18 AIC · ⊞ 9.3K ·
Comment /souschef to run again

Copilot AI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot PR is close, but there is still maintainer-visible follow-up needed.

Please complete one final pass:...

Final pass complete. main is already an ancestor of 80a38f4; no refresh is offered. The outstanding inline thread is outdated and its quoted-literal concern is addressed by 80a38f4, but this environment exposes no thread-resolution operation. Local validation: make fmt, make test-unit, make test, and make recompile passed; make lint remains blocked only by the same three pre-existing testifylint findings in pkg/intent/governance_formal_test.go on main. The two prior CI runs have no jobs or failed-job logs; CI for the agent head remains unverified and must be re-triggered by a maintainer before merge.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot PR is close, but there is still maintainer-visible follow-up needed.

Please complete one final pass:

  • Resolve or answer the remaining unresolved inline review thread directly.
  • Refresh the branch if GitHub offers one.
  • Then run the pr-finisher skill and leave a concise maintainer-facing handoff comment with validation and remaining risks.

Open review threads (newest first):

Checks are no longer running on the current head, so this is ready for that final maintainer-facing sweep.

Generated by 👨🍳 PR Sous Chef

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 19.9 AIC · ⌖ 8.28 AIC · ⊞ 9.3K ·
Comment /souschef to run again

…heredoc-fix-arithmetic-reporting

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

@copilot PR is close, but there is still maintainer-visible follow-up needed.

Please complete one final pass:...

Refreshed in a117313996 by merging current main with no conflicts. Confirmed the current-head 80a38f4 fix: quoted (( is ignored by the arithmetic detector, and the requested printf '%s\n' '((' && cat <<EOF regression fixture detects the real heredoc. Local validation after refresh passed via make agent-report-progress (format, changed-file lint, impacted tests, and workflow lock sync); make test and make recompile also passed. The review thread is outdated; this environment exposes no direct thread-reply or resolve operation. CI was inspected: the current dynamic run has no failed-job logs, but CI on the refreshed agent head must be re-triggered by a maintainer before merge. Remaining risk: the known three pre-existing testifylint findings in pkg/intent/governance_formal_test.go still block full make lint.

@github-actions

Copy link
Copy Markdown
Contributor

PR Triage

  • Category: bug
  • Risk: low
  • Priority score: 61 / 100 (impact 25 + urgency 20 + quality 16)
  • Recommended action: fast_track

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.

Generated by 🔧 PR Triage Agent · auto · 57.1 AIC · ⌖ 2.88 AIC · ⊞ 8.3K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generatedyamlheredoc: bare (( ... )) arithmetic with << is misreported as a heredoc

4 participants