trimleftright: widen heuristic to catch distinct-character prefix/suffix bugs#46536
Conversation
…fix/suffix bugs
Replace the repeated-rune proxy with a smarter heuristic:
- Remove the repeated-rune requirement that was missing common bugs
like TrimLeft(s, "http"), TrimLeft(s, "abc"), TrimRight(s, "txt")
- Add three narrow exceptions for intentional character-class trimming:
1. Pure decimal-digit sets (e.g. "0123456789")
2. Pure ASCII-vowel sets (e.g. "aeiou")
3. Complete hex-letter alphabet (all 6 of a-f present in any case,
optionally with digits, e.g. "abcdef", "0123456789abcdef")
- Partial hex subsets like "abc" are correctly flagged
Update testdata with new bad cases (http, abc, txt, v1) and good cases
(digits, vowels, hex alphabets). Expand unit tests for looksSuspiciousCutset.
Update Analyzer.Doc string and ADR to reflect the revised decision.
Closes #46526
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ent empty-slice edge cases Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Broadens trimleftright detection while exempting recognized character-class cutsets.
Changes:
- Flags distinct-character prefix and suffix mistakes.
- Adds digit, vowel, and hexadecimal exceptions.
- Expands tests and updates the ADR.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/trimleftright/trimleftright.go |
Implements the broader heuristic and exceptions. |
pkg/linters/trimleftright/trimleftright_internal_test.go |
Expands heuristic unit cases. |
pkg/linters/trimleftright/testdata/src/trimleftright/trimleftright.go |
Adds analyzer fixtures. |
docs/adr/46465-add-trimleftright-linter.md |
Documents the revised decision. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
|
|
||
| // Suspicious: partial hex-letter subset (not the complete alphabet). | ||
| // "abc" is tested above via "abc prefix"; here we confirm the | ||
| // 5-letter subset also escapes the hex exception. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The heuristic replacement looks correct. The three helpers (isAllASCIIDigits, isAllASCIIVowels, isCompleteHexAlphabet) are straightforward and well-tested. Test coverage expands from 6 to 23 cases covering all exception paths.
The pre-existing inline comment about the inaccurate "5-letter subset" remark for abce (4 letters, not 5) is already flagged and should be fixed before merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.5 AIC · ⌖ 6.71 AIC · ⊞ 5K
Design Decision Gate — ADR VerifiedADR reviewed: ADR-46465: Add trimleftright Linter to Detect TrimLeft/TrimRight Cutset Misuse — implementation aligns with the stated decision. Great work! Verification SummaryThe PR updates the existing ADR to reflect the widened heuristic and the code changes faithfully implement the updated decision:
No divergences found.
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (21 tests)
Test Classification
Verdict✅ APPROVED. All 21 subtests are design tests (100%). 0% implementation tests (threshold: 30%). Zero violations. Highlights:
The test suite directly validates that the new heuristic catches real bugs that the old implementation missed. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🤖 PR Triage
Automated triage — run §29677420780
|
There was a problem hiding this comment.
One correctness regression must be fixed before merge.
The logic is sound and the test coverage is good. One issue needs to be addressed:
isCompleteHexAlphabetregresses repeated-rune detection — cutsets like"aabbccddeeff"pass the hex exception silently because the map deduplicates; the old repeated-rune check would have flagged these. See inline comment for a fix.
🔎 Code quality review by PR Code Quality Reviewer · 34.9 AIC · ⌖ 4.69 AIC · ⊞ 5.6K
Comment /review to run again
| return false | ||
| } | ||
| } | ||
| return len(seen) == 6 |
There was a problem hiding this comment.
Regression: isCompleteHexAlphabet silently accepts suspicious repeated-rune hex cutsets — e.g. "aabbccddeeff" now passes the hex exception when it would have been flagged by the old repeated-rune check.
💡 Details and fix
The hex exception is intended to recognise intentional hex-class trimming like "abcdef". But the current implementation only checks that all 6 hex letters appear in seen; it never rejects duplicate runes. A cutset like "aabbccddeeff" populates seen with all 6 letters and returns false (not suspicious), silently un-flagging what is almost certainly a bug.
The old code caught this class via the repeated-rune check; widening the heuristic regressed it.
Simple fix — add a uniqueness check:
func isCompleteHexAlphabet(runes []rune) bool {
if len(runes) > 16 { // 6 hex letters + 10 digits maximum
return false
}
seen := make(map[rune]struct{})
for _, r := range runes {
lower := unicode.ToLower(r)
switch {
case lower >= 'a' && lower <= 'f':
if _, dup := seen[lower]; dup {
return false // repeated hex letter → suspicious
}
seen[lower] = struct{}{}
case r >= '0' && r <= '9':
default:
return false
}
}
return len(seen) == 6
}Add a test case: {name: "repeated hex letters", cutset: "aabbccddeeff", want: true}.
There was a problem hiding this comment.
Fixed in commit 322ddb3: isCompleteHexAlphabet now has a len(runes) > 16 guard and a per-letter uniqueness check, so "aabbccddeeff" (and any other repeated-hex-letter cutset) is correctly returned as suspicious. The test case {name: "repeated hex letters", cutset: "aabbccddeeff", want: true} was also added.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is solid and well-tested; leaving comments on minor coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Inverted heuristic is correct: flagging all multi-rune alphanumeric except known safe idioms is a much better model than the previous repeated-rune proxy.
- Minor test coverage gaps: the boundary of
isCompleteHexAlphabet(duplicate hex letters, all-six-plus-one) is untested and undocumented. - ADR update is thorough: the negative-consequence note now accurately reflects the trade-off.
Positive Highlights
- ✅ Three narrowly-scoped helper functions — each with a single, clearly-stated responsibility
- ✅ Test count grew from 6 → 23 with grouped, readable categories
- ✅ Existing regression cases ("foo", "barr") retained alongside new ones
- ✅ ADR updated to reflect the changed heuristic rationale
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38.9 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
| // Suspicious: partial hex-letter subset (not the complete alphabet). | ||
| // "abc" is tested above via "abc prefix"; here we confirm the | ||
| // 5-letter subset also escapes the hex exception. | ||
| {name: "partial hex abce", cutset: "abce", want: true}, |
There was a problem hiding this comment.
[/tdd] Missing boundary test: "abcdefg" should be flagged (suspicious) but there is no test to confirm it escapes the hex exception.
💡 Suggested test case
Add to the suspicious section:
{name: "almost hex abcdefg", cutset: "abcdefg", want: true},This documents that adding any non-hex-letter to all-six-hex-letters still triggers the linter, locking down the boundary of isCompleteHexAlphabet.
@copilot please address this.
There was a problem hiding this comment.
Added {name: "almost hex abcdefg", cutset: "abcdefg", want: true} in commit trimleftright: fix regression in isCompleteHexAlphabet, add boundary tests.
| return false | ||
| } | ||
| } | ||
| return len(seen) == 6 |
There was a problem hiding this comment.
[/diagnosing-bugs] isCompleteHexAlphabet silently deduplicates: a cutset like "aabcdef" (duplicate 'a', all six distinct hex letters present) returns false (safe). This is probably correct, but is untested and undocumented — a future reader may wonder whether duplicates were forgotten or intentional.
💡 Suggested test + doc note
Add to the safe-hex block in TestLooksSuspiciousCutset:
{name: "hex letters with duplicate", cutset: "aabcdef", want: false},Add a comment inside isCompleteHexAlphabet to make intent explicit:
// Duplicate hex letters are allowed; only distinct coverage of all six matters.@copilot please address this.
There was a problem hiding this comment.
The regression fix in thread r3610090990 now explicitly rejects duplicate hex letters, so "aabcdef" is flagged as suspicious (want: true) rather than safe. I added the test as {name: "hex with duplicate letter", cutset: "aabcdef", want: true} and updated the function comment to document that repeated hex letters are rejected. Both changes are in commit trimleftright: fix regression in isCompleteHexAlphabet, add boundary tests.
|
@copilot run pr-finisher skill |
|
@copilot please run the Unresolved review feedback:
The latest maintainer comment asked for
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed the inaccurate comment — |
…tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All three
Local |
|
@copilot please run the Unresolved review feedback:
The thread set appears stale relative to recent author follow-up, so please confirm whether each thread can now be resolved and hand back the next concrete maintainer action.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
All four threads are addressed in the current HEAD (
Local |
looksSuspiciousCutsetonly fired when the cutset contained a repeated rune, silently missing the most common form of the bug — distinct-character literals like"http","bar","abc","v1"that are almost certainly intended as prefixes/suffixes rather than character sets.Changes
trimleftright.go— Replaces the repeated-rune proxy with an inverted approach: flag all multi-rune all-alphanumeric cutsets except three recognized character-class idioms:"012","0123456789")"aeiou","aei")a–fpresent (any case, optionally with digits):"abcdef","ABCDEF","0123456789abcdef"Partial hex subsets like
"abc"(only 3 of 6 letters) are correctly flagged. Adds three focused helpers:isAllASCIIDigits,isAllASCIIVowels,isCompleteHexAlphabet.testdata/…/trimleftright.go— Adds bad cases for"http","abc","txt","v1"; adds good cases for"0123456789","abcdef","0123456789abcdef"; removes the now-incorrectgoodNoRepeatedAlnumfixture.trimleftright_internal_test.go— ExpandsTestLooksSuspiciousCutsetfrom 6 to 23 cases covering all exception paths and the newly flagged patterns.docs/adr/46465-add-trimleftright-linter.md— Updates the decision rationale and negative consequences to reflect the broader heuristic.