fix(writebytestring): emit string() cast for named string types; add RunWithSuggestedFixes#44208
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… add RunWithSuggestedFixes test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in the writebytestring analyzer where the suggested io.WriteString fix could fail to compile when the original argument was a named string type (e.g. type MyStr string), and strengthens the test harness to compile-check suggested fixes.
Changes:
- Update
writebytestringto emitstring(<expr>)casts when the argument is a named string type soio.WriteStringfixes compile. - Switch the analyzer test to
analysistest.RunWithSuggestedFixesand add/update golden + want expectations to validate compiled fix output. - Includes regenerated workflow
.lock.ymlupdates (currently not described in the PR description).
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/writebytestring/writebytestring.go | Adjusts fix/message generation to wrap named string types with string(...); adds isExactString. |
| pkg/linters/writebytestring/writebytestring_test.go | Ensures suggested fixes are applied/compiled via RunWithSuggestedFixes. |
| pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go | Updates // want expectation for named string case. |
| pkg/linters/writebytestring/testdata/src/writebytestring/writebytestring.go.golden | Adds golden output reflecting the compiled suggested fix (including string(s) for named string). |
| .github/workflows/lint-monster.lock.yml | Regenerated lock workflow content (Safe Outputs config heredoc/config JSON changes). |
| .github/workflows/eslint-monster.lock.yml | Regenerated lock workflow content (Safe Outputs config heredoc/config JSON changes). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Low
| // isExactString reports whether t is the predeclared string type, not a named | ||
| // type whose underlying type is string. io.WriteString(w Writer, s string) | ||
| // requires a predeclared string; named string types need an explicit string(...) | ||
| // conversion to satisfy the parameter type. | ||
| func isExactString(t types.Type) bool { | ||
| b, ok := t.(*types.Basic) | ||
| return ok && b.Kind() == types.String | ||
| } |
| mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs" | ||
| mkdir -p /tmp/gh-aw/safeoutputs | ||
| mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_0caeba2ce3ff905f_EOF' | ||
| {"assign_to_agent":{"allowed":["copilot"],"max":3,"target":"*"},"close_issue":{"max":10,"required_title_prefix":"[lint-monster] ","state_reason":"duplicate"},"create_discussion":{"category":"audits","close_older_discussions":true,"expires":48,"fallback_to_issue":true,"max":1,"title_prefix":"[lint-monster] "},"create_issue":{"expires":168,"labels":["automation","lint","cookie"],"max":3,"title_prefix":"[lint-monster] "},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{},"update_issue":{"allow_body":true,"max":10,"required_title_prefix":"[lint-monster] ","title_prefix":"[lint-monster] "}} | ||
| GH_AW_SAFE_OUTPUTS_CONFIG_0caeba2ce3ff905f_EOF | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_77a7106ac1879fb2_EOF' | ||
| {"assign_to_agent":{"allowed":["copilot"],"max":3,"target":"*"},"close_issue":{"max":10,"required_title_prefix":"[lint-monster] ","state_reason":"duplicate"},"create_discussion":{"category":"audits","close_older_discussions":true,"expires":48,"fallback_to_issue":true,"max":1,"title_prefix":"[lint-monster] "},"create_issue":{"expires":168,"labels":["automation","lint","cookie"],"max":3,"title_prefix":"[lint-monster] "},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{},"update_issue":{"allow_body":true,"max":10,"title_prefix":"[lint-monster] "}} | ||
| GH_AW_SAFE_OUTPUTS_CONFIG_77a7106ac1879fb2_EOF |
| - name: Generate Safe Outputs Config | ||
| run: | | ||
| mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs" | ||
| mkdir -p /tmp/gh-aw/safeoutputs | ||
| mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_eaee126711029267_EOF' | ||
| {"assign_to_agent":{"allowed":["copilot"],"max":3,"target":"*"},"close_issue":{"max":10,"required_title_prefix":"[eslint-monster] ","state_reason":"duplicate"},"create_discussion":{"category":"audits","close_older_discussions":true,"expires":48,"fallback_to_issue":true,"max":1,"title_prefix":"[eslint-monster] "},"create_issue":{"expires":168,"labels":["automation","eslint","cookie"],"max":3,"title_prefix":"[eslint-monster] "},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{},"update_issue":{"allow_body":true,"max":10,"required_title_prefix":"[eslint-monster] ","title_prefix":"[eslint-monster] "}} | ||
| GH_AW_SAFE_OUTPUTS_CONFIG_eaee126711029267_EOF | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_a70707033a3aa7b2_EOF' | ||
| {"assign_to_agent":{"allowed":["copilot"],"max":3,"target":"*"},"close_issue":{"max":10,"required_title_prefix":"[eslint-monster] ","state_reason":"duplicate"},"create_discussion":{"category":"audits","close_older_discussions":true,"expires":48,"fallback_to_issue":true,"max":1,"title_prefix":"[eslint-monster] "},"create_issue":{"expires":168,"labels":["automation","eslint","cookie"],"max":3,"title_prefix":"[eslint-monster] "},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{},"update_issue":{"allow_body":true,"max":10,"title_prefix":"[eslint-monster] "}} | ||
| GH_AW_SAFE_OUTPUTS_CONFIG_a70707033a3aa7b2_EOF |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (95 additions across 6 files, all in .github/skills/). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Verdict
References: §28921605289 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: writebytestring — named string type cast fix ✅
The fix is correct and complete.
isExactString uses a direct t.(*types.Basic) assertion (not .Underlying()) — the precise tool for the job. It correctly distinguishes the predeclared string from a named type like type myString string, while type aliases (type MyStr = string) pass through without an unnecessary cast.
isStringType continues to use .Underlying() as the entry gate, which is right — we want to flag both plain string and named-string types.
sExpr guard (st != nil && !isExactString(st)) is safe: since isStringType already succeeded, TypeOf should never return nil here, but the nil guard is a good defensive touch.
Golden file — the preserved // want trailing comments in the fixed output are expected; RunWithSuggestedFixes replaces only the call.Pos()→call.End() span, leaving trailing comments intact.
Lock file changes are auto-generated and unrelated to the linter fix.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 59.9 AIC · ⌖ 6.25 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — approving with two minor observations.
📋 Key Themes & Highlights
Key Themes
- Root cause well-addressed: The fix correctly distinguishes
*types.Basic(predeclaredstring) from named string types by using a direct type assertion rather than.Underlying()— exactly the right approach. - Test harness upgrade: Switching to
RunWithSuggestedFixesis the correct move and aligns with all sibling linters; it would have caught this bug immediately. - Two edge cases to consider: (1) type aliases (
type S = string) are handled correctly by the*types.Basicassertion but are untested; (2) the golden file retains// wantcomments — verify this is correct for theRunWithSuggestedFixesharness.
Positive Highlights
- ✅
isExactStringis a clean, minimal helper with an accurate doc comment explaining theio.WriteStringsignature constraint - ✅ Both the diagnostic message and
buildFixare updated consistently to usesExpr - ✅ The PR description is thorough and includes a clear before/after example
- ✅ The golden file covers all existing test cases, preserving regression coverage
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 93.9 AIC · ⌖ 5.64 AIC · ⊞ 6.6K
Comment /matt to run again
| // conversion to satisfy the parameter type. | ||
| func isExactString(t types.Type) bool { | ||
| b, ok := t.(*types.Basic) | ||
| return ok && b.Kind() == types.String |
There was a problem hiding this comment.
[/tdd] isExactString correctly handles type aliases (type MyStr = string) — aliases share the same *types.Basic node as the predeclared string so no string(...) wrap is emitted. This edge case is untested; adding a badAliasString testdata case would pin this behaviour as a regression guard.
💡 Suggested testdata addition
type stringAlias = string
func badAliasString() {
var buf bytes.Buffer
s := stringAlias("hello")
buf.Write([]byte(s)) // want `...io\.WriteString\(&buf, s\)...`
// alias IS the predeclared string — no string() wrap
}This documents the alias boundary and prevents a regression if isExactString is ever changed to use .Underlying().
@copilot please address this.
| func bad() { | ||
| var buf bytes.Buffer | ||
| s := "hello" | ||
| io.WriteString(&buf, s) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(&buf, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` |
There was a problem hiding this comment.
[/diagnosing-bugs] The golden file retains // want regex comments in the fixed output. The golden file represents the rewritten source after fixes are applied — these // want annotations are left over from the original and will cause the test harness to re-run diagnostics against the already-fixed code, producing unexpected second-pass diagnostics.
Check how sibling linters handle this: appendbytestring.go.golden also retains // want comments, so this may be intentional harness behaviour. If RunWithSuggestedFixes applies fixes and re-runs the analyser on the result, these comments would need to match the new diagnostics (on io.WriteString(...) calls which the linter does not flag). Consider verifying this is intentional and add a brief code comment explaining the pattern.
@copilot please address this.
🤖 PR Triage
Rationale: Fixes silent linter false-fix: Run §28924016278
|
|
🎉 This pull request is included in a new release. Release: |
writebytestringaccepted named string types (e.g.type MyStr string) viaUnderlying()but emittedio.WriteString(w, s)verbatim — non-compiling, sinceio.WriteStringrequires a predeclaredstring. The bug was invisible because the test usedanalysistest.Run(diagnostic-only) instead ofRunWithSuggestedFixes, which would have compiled the fix output.Changes
writebytestring.go— addisExactString(types.Type) boolusing a direct*types.Basicassertion (not.Underlying()) to distinguish the predeclaredstringfrom named string types. ComputesExprinrun: pass through for exactstring, wrap withstring(...)for named types. Both the diagnostic message andbuildFixusesExpr.writebytestring_test.go— switch toanalysistest.RunWithSuggestedFixes, matching all sibling linters (appendbytestring,stringsindexcontains,sprintfint).testdata
writebytestring.go— updatebadNamedString// wantregex to expectio.WriteString(&buf, string(s)).writebytestring.go.golden(new) — golden file compiled by the harness;badNamedStringreflects thestring(s)wrap, all other cases unchanged.Before / After