diff --git a/.github/workflows/smoke-call-workflow.lock.yml b/.github/workflows/smoke-call-workflow.lock.yml index 8e6f0b9211f..17814deb639 100644 --- a/.github/workflows/smoke-call-workflow.lock.yml +++ b/.github/workflows/smoke-call-workflow.lock.yml @@ -619,6 +619,11 @@ jobs: "inputSchema": { "additionalProperties": false, "properties": { + "aw_context": { + "default": "", + "description": "Agent caller context (used internally by Agentic Workflows).", + "type": "string" + }, "payload": { "description": "Input parameter 'payload' for workflow smoke-workflow-call", "type": "string" @@ -1102,15 +1107,25 @@ jobs: needs: safe_outputs if: needs.safe_outputs.outputs.call_workflow_name == 'smoke-workflow-call' # Imported from called workflow "smoke-workflow-call" because GitHub requires the caller job to grant permissions requested by reusable workflow jobs. - # Review the called workflow's frontmatter permissions in ./.github/workflows/smoke-workflow-call.md. + # Review the called workflow's job-level permissions in ./.github/workflows/smoke-workflow-call.lock.yml. permissions: + actions: read contents: read - pull-requests: read + issues: write + pull-requests: write uses: ./.github/workflows/smoke-workflow-call.lock.yml with: + aw_context: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).aw_context }} payload: ${{ needs.safe_outputs.outputs.call_workflow_payload }} task-description: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload)['task-description'] }} - secrets: inherit + secrets: + COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }} + GH_AW_GITHUB_MCP_SERVER_TOKEN: ${{ secrets.GH_AW_GITHUB_MCP_SERVER_TOKEN }} + GH_AW_GITHUB_TOKEN: ${{ secrets.GH_AW_GITHUB_TOKEN }} + GH_AW_OTEL_GRAFANA_AUTHORIZATION: ${{ secrets.GH_AW_OTEL_GRAFANA_AUTHORIZATION }} + GH_AW_OTEL_GRAFANA_ENDPOINT: ${{ secrets.GH_AW_OTEL_GRAFANA_ENDPOINT }} + GH_AW_OTEL_SENTRY_AUTHORIZATION: ${{ secrets.GH_AW_OTEL_SENTRY_AUTHORIZATION }} + GH_AW_OTEL_SENTRY_ENDPOINT: ${{ secrets.GH_AW_OTEL_SENTRY_ENDPOINT }} conclusion: needs: diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 6f52c70b49c..5489a31c795 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -17,6 +17,7 @@ import ( "golang.org/x/tools/go/analysis/multichecker" "github.com/github/gh-aw/pkg/linters/appendbytestring" + "github.com/github/gh-aw/pkg/linters/bytesbufferstring" "github.com/github/gh-aw/pkg/linters/bytescomparestring" "github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred" "github.com/github/gh-aw/pkg/linters/ctxbackground" @@ -68,6 +69,7 @@ import ( func main() { multichecker.Main( appendbytestring.Analyzer, + bytesbufferstring.Analyzer, bytescomparestring.Analyzer, contextcancelnotdeferred.Analyzer, ctxbackground.Analyzer, diff --git a/docs/adr/45301-bytesbufferstring-linter-flag-string-of-buf-bytes.md b/docs/adr/45301-bytesbufferstring-linter-flag-string-of-buf-bytes.md new file mode 100644 index 00000000000..b48301dc06e --- /dev/null +++ b/docs/adr/45301-bytesbufferstring-linter-flag-string-of-buf-bytes.md @@ -0,0 +1,50 @@ +# ADR-45301: Add `bytesbufferstring` Linter — Flag `string(buf.Bytes())` in Favour of `buf.String()` + +**Date**: 2026-07-13 +**Status**: Draft +**Deciders**: Unknown (automated PR by linter-miner) + +--- + +### Context + +The `gh-aw` repository maintains a custom suite of Go static-analysis linters under `pkg/linters/`. Each linter detects a specific sub-optimal code pattern and provides an auto-applicable suggested fix. The pattern `string(buf.Bytes())` — where `buf` is `*bytes.Buffer` — creates an unnecessary intermediate `[]byte` allocation before the final `string` conversion. `bytes.Buffer.String()` already returns the buffer's contents as a string directly, making the intermediate conversion redundant and wasteful. The linter-miner agent identified this pattern by scanning non-test Go source files in `pkg/` and `cmd/`. + +### Decision + +We will add a new Go analysis pass, `bytesbufferstring`, to the custom linter suite. The analyzer flags any `string(x.Bytes())` call where `x` is statically typed as `*bytes.Buffer` or `bytes.Buffer`, and offers a single suggested fix that rewrites the expression to `x.String()`. The analyzer skips test files and respects `//nolint:bytesbufferstring` suppressions. It is registered in `cmd/linters/main.go` alongside all other custom analyzers. + +### Alternatives Considered + +#### Alternative 1: Rely on Human Code Review + +Leave detection of this pattern entirely to human reviewers during pull request review. No tooling change needed, and no new linter to maintain. + +This was rejected because human review is inconsistent and does not scale: the same sub-optimal pattern can be introduced repeatedly across a large codebase. An automated linter provides reliable, zero-effort enforcement. + +#### Alternative 2: Use an Existing Third-Party Linter (`staticcheck` / `golangci-lint`) + +Configure an external linter tool that already covers this pattern, rather than writing a custom one. + +This was not viable in this context: the existing tooling infrastructure is built around custom `golang.org/x/tools/go/analysis` passes in the `pkg/linters/` package. Introducing a new external tool would require changes to the CI pipeline, build configuration, and version-pinning strategy, which exceeds the scope of adding a single focused rule. No existing linter in the project's current set covers this pattern. + +### Consequences + +#### Positive +- Eliminates unnecessary intermediate `[]byte` allocations wherever the pattern is found, producing modest runtime improvements. +- The suggested fix is auto-applicable (zero manual effort for the author once the linter runs). +- Zero false-positive risk: the match is purely structural — any `string(x.Bytes())` where `x` is `*bytes.Buffer` is unambiguously replaceable. +- Follows the established linter conventions in the repository (`largefunc` layout, `nolint` suppression, test fixture with golden file). + +#### Negative +- Adds one more analyzer to the custom linter suite, increasing maintenance surface. +- Each additional linter marginally increases the time for a full lint pass. +- Any existing occurrences of `string(buf.Bytes())` in the codebase that are not suppressed will become lint failures, requiring authors to update code or add `//nolint:bytesbufferstring`. + +#### Neutral +- The pattern is detected at analysis time only; no runtime behaviour or API contracts change. +- Suppression via `//nolint:bytesbufferstring` is available for cases where the conversion is intentional (e.g., operating on a copy, not the original buffer). + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/linters b/linters index 04a7c5188f1..bc942cc6754 100755 Binary files a/linters and b/linters differ diff --git a/pkg/linters/bytesbufferstring/bytesbufferstring.go b/pkg/linters/bytesbufferstring/bytesbufferstring.go new file mode 100644 index 00000000000..ee723e2602c --- /dev/null +++ b/pkg/linters/bytesbufferstring/bytesbufferstring.go @@ -0,0 +1,124 @@ +// Package bytesbufferstring implements a Go analysis linter that flags +// string(buf.Bytes()) calls where buf is a bytes.Buffer value receiver, +// suggesting buf.String() instead. +package bytesbufferstring + +import ( + "fmt" + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// Analyzer is the bytes-buffer-string analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "bytesbufferstring", + Doc: "reports string(buf.Bytes()) calls where buf is a bytes.Buffer value and suggests buf.String() instead", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/bytesbufferstring", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "bytesbufferstring") + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + // Match string(...) type conversion. + typeInfo, ok := pass.TypesInfo.Types[call.Fun] + if !ok || !typeInfo.IsType() { + return + } + basic, ok := typeInfo.Type.(*types.Basic) + if !ok || basic.Kind() != types.String { + return + } + + if len(call.Args) != 1 { + return + } + + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + return + } + if nolint.HasDirective(pos, noLintLinesByFile) { + return + } + + // The argument must be buf.Bytes() where buf is a bytes.Buffer value. + inner, ok := call.Args[0].(*ast.CallExpr) + if !ok { + return + } + sel, ok := inner.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Bytes" { + return + } + if len(inner.Args) != 0 { + return + } + receiverType := pass.TypesInfo.TypeOf(sel.X) + if receiverType == nil { + return + } + // Only flag value receivers (bytes.Buffer), not pointer receivers (*bytes.Buffer). + // The rewrite string(buf.Bytes()) → buf.String() is not semantics-preserving when + // buf is a nil *bytes.Buffer: string(buf.Bytes()) panics, while buf.String() returns + // "". Restricting to value receivers avoids this semantic difference entirely. + if !isBytesBufferValue(receiverType) { + return + } + + receiverText := astutil.NodeText(pass.Fset, sel.X) + if receiverText == "" { + return + } + + pass.Report(analysis.Diagnostic{ + Pos: call.Pos(), + End: call.End(), + Message: fmt.Sprintf("string(%s.Bytes()) can be simplified to %s.String()", receiverText, receiverText), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: fmt.Sprintf("Replace string(%s.Bytes()) with %s.String()", receiverText, receiverText), + TextEdits: []analysis.TextEdit{{ + Pos: call.Pos(), + End: call.End(), + NewText: []byte(receiverText + ".String()"), + }}, + }}, + }) + }) + + return nil, nil +} + +// isBytesBufferValue reports whether t is exactly bytes.Buffer (value receiver, not pointer). +// We intentionally exclude *bytes.Buffer: the rewrite string(buf.Bytes()) → buf.String() is not +// semantics-preserving when buf is nil — the former panics while the latter returns "". +func isBytesBufferValue(t types.Type) bool { + named, ok := t.(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj.Pkg() != nil && obj.Pkg().Path() == "bytes" && obj.Name() == "Buffer" +} diff --git a/pkg/linters/bytesbufferstring/bytesbufferstring_test.go b/pkg/linters/bytesbufferstring/bytesbufferstring_test.go new file mode 100644 index 00000000000..fd5c806cdb3 --- /dev/null +++ b/pkg/linters/bytesbufferstring/bytesbufferstring_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package bytesbufferstring_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/bytesbufferstring" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, bytesbufferstring.Analyzer, "bytesbufferstring") +} diff --git a/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go b/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go new file mode 100644 index 00000000000..9c5c1e4acbe --- /dev/null +++ b/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go @@ -0,0 +1,59 @@ +package bytesbufferstring + +import ( + "bytes" +) + +func BadStringOfBytesCall() string { + var buf bytes.Buffer + buf.WriteString("hello") + return string(buf.Bytes()) // want `string\(buf\.Bytes\(\)\) can be simplified to buf\.String\(\)` +} + +func BadStringOfBytesCallPtr() string { + buf := &bytes.Buffer{} + buf.WriteString("world") + // pointer receiver: not flagged — rewrite would change nil-pointer semantics + return string(buf.Bytes()) +} + +func NilPointerBuf() string { + var buf *bytes.Buffer + // nil *bytes.Buffer: string(buf.Bytes()) panics; buf.String() returns "" — not flagged + return string(buf.Bytes()) +} + +// wrappedBuffer embeds bytes.Buffer but is a distinct named type. +type wrappedBuffer struct { + bytes.Buffer +} + +func GoodStringConversionWrappedType() string { + var buf wrappedBuffer + // wrappedBuffer is not bytes.Buffer — receiver type check excludes it; no diagnostic + return string(buf.Bytes()) +} + +func getBufferPtr() *bytes.Buffer { return &bytes.Buffer{} } + +func GoodStringOfBytesCallIndirect() string { + // getBufferPtr() returns *bytes.Buffer (pointer receiver) — not flagged + return string(getBufferPtr().Bytes()) +} + +func SuppressedStringOfBytes() string { + var buf bytes.Buffer + buf.WriteString("hello") + return string(buf.Bytes()) //nolint:bytesbufferstring +} + +func GoodStringCall() string { + var buf bytes.Buffer + buf.WriteString("hello") + return buf.String() // correct pattern — no diagnostic expected +} + +func GoodStringConversionOther() string { + b := []byte("hello") + return string(b) // not a buf.Bytes() call — no diagnostic +} diff --git a/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go.golden b/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go.golden new file mode 100644 index 00000000000..1937e8f9e68 --- /dev/null +++ b/pkg/linters/bytesbufferstring/testdata/src/bytesbufferstring/bytesbufferstring.go.golden @@ -0,0 +1,59 @@ +package bytesbufferstring + +import ( + "bytes" +) + +func BadStringOfBytesCall() string { + var buf bytes.Buffer + buf.WriteString("hello") + return buf.String() // want `string\(buf\.Bytes\(\)\) can be simplified to buf\.String\(\)` +} + +func BadStringOfBytesCallPtr() string { + buf := &bytes.Buffer{} + buf.WriteString("world") + // pointer receiver: not flagged — rewrite would change nil-pointer semantics + return string(buf.Bytes()) +} + +func NilPointerBuf() string { + var buf *bytes.Buffer + // nil *bytes.Buffer: string(buf.Bytes()) panics; buf.String() returns "" — not flagged + return string(buf.Bytes()) +} + +// wrappedBuffer embeds bytes.Buffer but is a distinct named type. +type wrappedBuffer struct { + bytes.Buffer +} + +func GoodStringConversionWrappedType() string { + var buf wrappedBuffer + // wrappedBuffer is not bytes.Buffer — receiver type check excludes it; no diagnostic + return string(buf.Bytes()) +} + +func getBufferPtr() *bytes.Buffer { return &bytes.Buffer{} } + +func GoodStringOfBytesCallIndirect() string { + // getBufferPtr() returns *bytes.Buffer (pointer receiver) — not flagged + return string(getBufferPtr().Bytes()) +} + +func SuppressedStringOfBytes() string { + var buf bytes.Buffer + buf.WriteString("hello") + return string(buf.Bytes()) //nolint:bytesbufferstring +} + +func GoodStringCall() string { + var buf bytes.Buffer + buf.WriteString("hello") + return buf.String() // correct pattern — no diagnostic expected +} + +func GoodStringConversionOther() string { + b := []byte("hello") + return string(b) // not a buf.Bytes() call — no diagnostic +}