From 28b5b28d7b5027588142d1d87393780c7c2f777e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 24 May 2026 05:33:26 +0000 Subject: [PATCH 1/2] Initial plan From dd3266d757fedc109accba6f8c2f7b4ea6080fce Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 24 May 2026 05:41:50 +0000 Subject: [PATCH 2/2] Enforce panicinlibrarycode in CI Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/workflows/cgo.yml | 7 +- .../panic-in-library-code.go | 136 +++++++++++++++++- .../panicinlibrarycode/panicinlibrarycode.go | 29 +++- pkg/workflow/agentic_engine.go | 3 +- pkg/workflow/claude_tools.go | 1 + pkg/workflow/model_aliases.go | 2 + pkg/workflow/strings.go | 1 + 7 files changed, 169 insertions(+), 10 deletions(-) diff --git a/.github/workflows/cgo.yml b/.github/workflows/cgo.yml index ea7fc7cc565..9118ca46b17 100644 --- a/.github/workflows/cgo.yml +++ b/.github/workflows/cgo.yml @@ -1034,10 +1034,11 @@ jobs: - name: Lint error messages run: make lint-errors - # Enforce the production errstringmatch analyzer without blocking on unrelated - # legacy custom analyzer findings in tests or other analyzer families. + # Enforce the production errstringmatch and panicinlibrarycode analyzers without + # blocking on unrelated legacy custom analyzer findings in tests or other analyzer + # families. - name: Run custom linters - run: make golint-custom LINTER_FLAGS="-errstringmatch -test=false" + run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -test=false" actions-build: runs-on: ubuntu-latest diff --git a/pkg/linters/panic-in-library-code/panic-in-library-code.go b/pkg/linters/panic-in-library-code/panic-in-library-code.go index 11e83bb740d..89fbfd0769e 100644 --- a/pkg/linters/panic-in-library-code/panic-in-library-code.go +++ b/pkg/linters/panic-in-library-code/panic-in-library-code.go @@ -4,6 +4,8 @@ package panicinlibrarycode import ( "go/ast" + "go/constant" + "go/token" "go/types" "strings" @@ -36,32 +38,156 @@ func run(pass *analysis.Pass) (any, error) { (*ast.CallExpr)(nil), } - insp.Preorder(nodeFilter, func(n ast.Node) { + insp.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return true + } + call := n.(*ast.CallExpr) // Skip test files if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { - return + return true } // Check if this is a call to the builtin panic function ident, ok := call.Fun.(*ast.Ident) if !ok { - return + return true } if ident.Name != "panic" { - return + return true } // Verify it's the builtin panic, not a user-defined function if obj := pass.TypesInfo.Uses[ident]; obj != nil { if _, ok := obj.(*types.Builtin); !ok { - return // Not the builtin panic + return true // Not the builtin panic } } + if shouldSkipPanic(pass, call, stack) { + return true + } + pass.ReportRangef(call, "avoid panic in library code; return an error instead") + return true }) return nil, nil } + +func shouldSkipPanic(pass *analysis.Pass, call *ast.CallExpr, stack []ast.Node) bool { + return isInSyncOnceDoFuncLit(pass, stack) || + panicMessageStartsWithBUG(pass, call) || + isInInitFunction(stack) || + hasDocumentedPanicContract(stack) +} + +func isInSyncOnceDoFuncLit(pass *analysis.Pass, stack []ast.Node) bool { + for i := len(stack) - 1; i >= 0; i-- { + funcLit, ok := stack[i].(*ast.FuncLit) + if !ok || i == 0 { + continue + } + + call, ok := stack[i-1].(*ast.CallExpr) + if !ok || !containsExpr(call.Args, funcLit) { + continue + } + + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Do" { + continue + } + + if isSyncOnceType(pass.TypesInfo.TypeOf(sel.X)) { + return true + } + } + + return false +} + +func containsExpr(args []ast.Expr, target ast.Expr) bool { + for _, arg := range args { + if arg == target { + return true + } + } + return false +} + +func isSyncOnceType(t types.Type) bool { + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + + named, ok := t.(*types.Named) + if !ok || named.Obj() == nil || named.Obj().Pkg() == nil { + return false + } + + return named.Obj().Pkg().Path() == "sync" && named.Obj().Name() == "Once" +} + +func panicMessageStartsWithBUG(pass *analysis.Pass, call *ast.CallExpr) bool { + if len(call.Args) == 0 { + return false + } + + prefix, ok := stringPrefix(pass, call.Args[0]) + if !ok { + return false + } + + return strings.HasPrefix(strings.ToUpper(strings.TrimSpace(prefix)), "BUG:") +} + +func stringPrefix(pass *analysis.Pass, expr ast.Expr) (string, bool) { + if tv, ok := pass.TypesInfo.Types[expr]; ok && tv.Value != nil && tv.Value.Kind() == constant.String { + return constant.StringVal(tv.Value), true + } + + switch e := expr.(type) { + case *ast.BinaryExpr: + if e.Op != token.ADD { + return "", false + } + return stringPrefix(pass, e.X) + case *ast.CallExpr: + if len(e.Args) == 0 { + return "", false + } + return stringPrefix(pass, e.Args[0]) + default: + return "", false + } +} + +func isInInitFunction(stack []ast.Node) bool { + decl := enclosingFuncDecl(stack) + return decl != nil && decl.Name != nil && decl.Name.Name == "init" +} + +func hasDocumentedPanicContract(stack []ast.Node) bool { + decl := enclosingFuncDecl(stack) + if decl == nil || decl.Doc == nil { + return false + } + + doc := strings.ToLower(decl.Doc.Text()) + return strings.Contains(doc, "panics on") || + strings.Contains(doc, "panics if") || + strings.Contains(doc, "panic on") || + strings.Contains(doc, "panic if") +} + +func enclosingFuncDecl(stack []ast.Node) *ast.FuncDecl { + for i := len(stack) - 1; i >= 0; i-- { + if decl, ok := stack[i].(*ast.FuncDecl); ok { + return decl + } + } + return nil +} diff --git a/pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go b/pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go index 2902992ef10..6c7828ffa5d 100644 --- a/pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go +++ b/pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go @@ -1,6 +1,10 @@ package panicinlibrarycode -import "errors" +import ( + "errors" + "fmt" + "sync" +) // bad: panic in a pkg/ package. func riskyFunction() { @@ -28,3 +32,26 @@ func callCustomPanic() { m := myType{} m.panic("this is ok") // Should not be flagged } + +var once sync.Once + +func allowedSyncOncePanic() { + once.Do(func() { + panic("lazy init failure") + }) +} + +func allowedBUGPanic() { + panic(fmt.Sprintf("BUG: unreachable: %v", errors.New("boom"))) +} + +func init() { + panic("startup registration failure") +} + +// documentedPreconditionPanics panics if the caller passes an invalid mode. +func documentedPreconditionPanics(mode string) { + if mode == "" { + panic("invalid mode") + } +} diff --git a/pkg/workflow/agentic_engine.go b/pkg/workflow/agentic_engine.go index 5bf4fe1f2c8..e516119bde2 100644 --- a/pkg/workflow/agentic_engine.go +++ b/pkg/workflow/agentic_engine.go @@ -440,7 +440,8 @@ var ( registryInitOnce sync.Once ) -// NewEngineRegistry creates a new engine registry with built-in engines +// NewEngineRegistry creates a new engine registry with built-in engines. +// Panics on invalid built-in engine registration. func NewEngineRegistry() *EngineRegistry { agenticEngineLog.Print("Creating new engine registry") diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index 435ea77b739..a0738865679 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -165,6 +165,7 @@ func (e *ClaudeEngine) expandNeutralToolsToClaudeTools(tools map[string]any) map // user-visible tools map but must be explicitly added to --allowed-tools when // --permission-mode acceptEdits is in use, because acceptEdits actually enforces the // allowlist (unlike bypassPermissions which silently ignores it). +// Panics if callers pass a Claude-specific tools section instead of neutral tools. func (e *ClaudeEngine) computeAllowedClaudeToolsString(tools map[string]any, safeOutputs *SafeOutputsConfig, cacheMemoryConfig *CacheMemoryConfig, mcpScripts *MCPScriptsConfig, sandboxConfig *SandboxConfig) string { claudeToolsLog.Print("Computing allowed Claude tools string") diff --git a/pkg/workflow/model_aliases.go b/pkg/workflow/model_aliases.go index 3d196a099c7..2f37f17f206 100644 --- a/pkg/workflow/model_aliases.go +++ b/pkg/workflow/model_aliases.go @@ -78,6 +78,8 @@ type builtinModelAliasesFile struct { // - "claude" → agent, sonnet-6x, haiku, any // - "codex" → agent, gpt-5-codex, gpt-5, any // - "gemini" → agent, gemini-pro, gemini-flash, any +// +// Panics on invalid embedded model_aliases.json data. func BuiltinModelAliases() map[string][]string { var data builtinModelAliasesFile if err := json.Unmarshal(builtinModelAliasesJSON, &data); err != nil { diff --git a/pkg/workflow/strings.go b/pkg/workflow/strings.go index f1718d73188..a6994e93a3a 100644 --- a/pkg/workflow/strings.go +++ b/pkg/workflow/strings.go @@ -163,6 +163,7 @@ func ShortenCommand(command string) string { // // When seed is empty, the function falls back to crypto/rand — the same behaviour as // GenerateHeredocDelimiter — so callers that lack a hash continue to work correctly. +// Panics on crypto/rand failure when no seed is provided. func GenerateHeredocDelimiterFromSeed(name string, seed string) string { upperName := strings.ToUpper(name) var tag string