lenstringzero: report the actual expression in diagnostic messages

[Copilot]

lenstringzero hardcoded the placeholder text s and len(s) in its diagnostic format string, so every violation named a variable that doesn't exist in the source. The alias path (n := len(s); if n == 0) was doubly wrong: it reported len(s) where the source reads n, and no s appears at the flagged line at all.

Before / after for if len(username) == 0:

- use s == "" to check for empty string instead of len(s) == 0
+ use username == "" to check for empty string instead of len(username) == 0

Changes

• matchLenLiteralExpr now also returns lenNode, the expression actually being compared — the len(...) call for the direct case, the alias identifier for the alias case. Yoda-order branches return the corresponding operand.
• Message construction interpolates astutil.NodeText for both the string argument and the compared node, via a small exprTextOr helper that falls back to the previous placeholders if a node can't be printed.
• Fixtures: alias expectations in the analysistest source and golden files updated to n; added namedVariableEmpty / namedVariableAliasEmpty cases asserting exact message text for a non-trivial variable name (username / usernameLen).

SuggestedFixes construction is untouched — the autofix was already correct.

Out of scope

The issue notes lenstringzero is missing from LINTER_FLAGS in .github/workflows/cgo.yml, which is why this went unnoticed. Enabling it repo-wide is a separate change with broader blast radius and isn't part of the issue's validation checklist.

• Fixes lenstringzero: diagnostic message hardcodes literal "s"/"len(s)" text, misreporting every violation's actual expression #54717

[github-actions]

Hey @Copilot 👋 — thanks for working on fixing the hardcoded diagnostic messages in lenstringzero! The bug fix itself looks solid and is well-tested.

⚠️ However, this PR doesn't follow the contribution process for non-core team members in this repository.

Per CONTRIBUTING.md:

🚫 Traditional Pull Requests Are Not Enabled for non-Core team members: If you are not part of the core team, please do not create pull requests directly. Instead, you create detailed agentic plans in issues, discuss with the team, and a core team member will create and implement the PR for you using agents.

Next steps:

1. Open an issue with a detailed agentic plan for this fix (including the scope limitation you mention about LINTER_FLAGS in .github/workflows/cgo.yml).
2. Await core team feedback and approval in that issue.
3. A core team member will create the actual PR and assign it to the Copilot agent for implementation.

This ensures coordination and maintains the project's agentic development workflow.

The code quality is excellent — once you follow the process, this fix should move through quickly. 🚀

Generated by ✅ Contribution Check · auto · 59.1 AIC · ⌖ 5.99 AIC · ⊞ 9.2K · ◷

[github-actions]

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #54721

[github-actions]

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

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

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch PR file list

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #54721 does not have the 'implementation' label and has only 58 new lines of code in business logic directories (threshold: 100).

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

Reviewed PR #54721 and found no actionable changed-line issues to comment on; submitting no-op safe output to satisfy workflow requirement.

🔎 Code quality review by PR Code Quality Reviewer

[Copilot]

Pull request overview

Updates lenstringzero diagnostics to report the actual source expressions.

Changes:

• Propagates matched len or alias nodes into diagnostic construction.
• Renders accurate argument and comparison text with fallbacks.
• Adds exact-message regression fixtures for named variables and aliases.

Show a summary per file

File	Description
pkg/linters/lenstringzero/lenstringzero.go	Builds diagnostics from matched AST expressions.
pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go	Adds and updates diagnostic expectations.
pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden	Updates expected suggested-fix output.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Balanced

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs — the fix correctly addresses root cause, not just symptoms. Approving.

📋 Key Themes & Highlights

Key Themes

• Root cause fixed: hardcoded s/len(s) placeholders replaced with actual source text at the diagnostic site
• Fallback is safe: exprTextOr gracefully degrades to the old placeholders when NodeText can't render
• Regression tests added: namedVariableEmpty and namedVariableAliasEmpty pin exact message text for non-trivial variable names

Positive Highlights

• ✅ All 8 return paths in matchLenLiteralExpr correctly thread the new lenNode value
• ✅ Golden file correctly leaves alias cases unchanged (consistent with existing behavior)
• ✅ exprTextOr is a clean, reusable helper with a clear contract
• ✅ PR scope is tight: only the diagnostic message is touched; SuggestedFixes construction is correctly left alone

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.8 AIC · ⌖ 9.76 AIC · ⊞ 7.6K
_{Comment /matt to run again}

[github-actions]

The changes are correct and well-implemented. The linter now reports actual source expressions in diagnostic messages (e.g. use n == "" instead of the hardcoded use s == ""), which makes messages significantly more actionable for developers.

The implementation is clean:

• lenNode is threaded through the return signature with clear documentation
• exprTextOr has a proper nil guard and meaningful fallback
• All test expectations and golden files are updated consistently

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.5 AIC · ⌖ 8.98 AIC · ⊞ 6.2K

[github-actions]

Comment Memory

reviewed_at: 2026-08-22T00:00:00Z
review_event: COMMENT
top_themes:
  - no actionable issues found in changed lines
files_reviewed:
  - pkg/linters/lenstringzero/lenstringzero.go
  - pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go
  - pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go.golden
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 · 3.15 AIC · ⌖ 6.79 AIC · ⊞ 7K · ◷
_{Comment /review to run again}

[github-actions]

PR Triage

• Category: bug
• Risk: low
• Priority score: 60/100 (impact 30, urgency 10, quality 20)
• Recommended action: auto_merge
• CI status: passing

Generated by 🔧 PR Triage Agent · auto · 82.9 AIC · ⌖ 6.65 AIC · ⊞ 8.3K · ◷

[github-actions]

PR Triage

Category: bug | Risk: low | Priority: medium | Score: 55/100
Recommended action: auto_merge

Non-draft, CI green (29 success), 2x APPROVED.

_{Automated triage — run 32572524009}

Generated by 🔧 PR Triage Agent · auto · 65.4 AIC · ⌖ 6.04 AIC · ⊞ 8.3K · ◷

[gh-aw-bot]

@copilot this PR still needs forward progress before maintainer review.

• Please address the remaining issues, refresh the branch if possible, and run the pr-finisher skill before handoff.
• Failed checks:
  • impacted-go-tests: https://github.com/github/gh-aw/actions/runs/32553488661/job/96983946865
  • lint-go-custom: https://github.com/github/gh-aw/actions/runs/32553488661/job/96983947105
• Leave a concise maintainer handoff with exact commands and outcomes.

Run: https://github.com/github/gh-aw/actions/runs/32573431024

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 9.09 AIC · ⌖ 8.39 AIC · ⊞ 9.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot this PR still needs forward progress before maintainer review.

• Please address the remaining issues, refresh the branch if possible, and run the pr-finisher skill before handoff....

Branch refreshed by merging origin/main (was behind; now includes 603fde6). Both failures traced to the CI checkout cache not having origin/main resolvable, not to this PR's code:

• impacted-go-tests — Error: unable to determine merge-base from BASE_REF=origin/main. No test ran at all.
• lint-go-custom — cgo.yml computes errormessage_flags from git diff --name-only "$BASE_REF...HEAD"; with origin/main missing it hits the Base ref not found, falling back to all Go files branch and lints the whole tree, surfacing pre-existing violations in files this PR never touches (pkg/cli/workflows.go, pkg/cli/yamllint.go, pkg/cli/zizmor.go, pkg/linters/errorfwrapv/errorfwrapv.go, ...).

Verified locally after the merge:

Command	Outcome
make test-impacted-go BASE_REF=origin/main	pass — ok github.com/github/gh-aw/pkg/linters/lenstringzero
make golint-custom LINTER_FLAGS="-test=false -errormessage -errormessage.changed-files=pkg/linters/lenstringzero/lenstringzero.go,pkg/linters/lenstringzero/testdata/src/lenstringzero/lenstringzero.go"	clean, no findings
make fmt	no diff
make agent-report-progress	pass

Diff vs main is still confined to 3 files (lenstringzero.go + its two testdata fixtures). No source changes were needed for either failure.