LintMonster group 3: harden type assertions, reduce manifest updater params, and fix file-close defer#35111
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. The PR only contains changes to production Go files (pkg/cli, pkg/linters, pkg/parser, pkg/workflow). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses a lint batch (“LintMonster group 3”) by hardening type assertions to avoid panic paths, refactoring an over-parameterized manifest updater, and fixing file-close deferral ordering to ensure resources are closed reliably while preserving close-error propagation.
Changes:
- Replaced single-value (panic-prone) type assertions with checked assertions and fallback handling in workflow/runtime code and custom analyzers.
- Refactored
updateManifestManagedWorkflowto take a singlemanifestManagedWorkflowUpdatestruct instead of many positional parameters. - Adjusted workflow file reading to
deferclose immediately afteros.Open, with deferred close-error assignment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/yaml.go | Makes regex cache retrieval resilient to unexpected cached value types. |
| pkg/workflow/safe_outputs_workflow_helpers.go | Hardens schema map access when setting required fields. |
| pkg/workflow/safe_outputs_config_generation.go | Avoids panics when reading/writing properties in generated input schemas. |
| pkg/workflow/safe_outputs_actions.go | Avoids panics when reading/writing properties in action tool schemas. |
| pkg/workflow/repository_features_validation.go | Makes repository feature cache assertions safe with explicit errors on mismatch. |
| pkg/workflow/mcp_scripts_generator.go | Avoids panics when reading/writing properties in generated tools.json schemas. |
| pkg/workflow/dispatch_repository.go | Hardens schema map access when setting required fields for dispatch tools. |
| pkg/parser/virtual_fs.go | Hardens cached frontmatter retrieval/storage with type checks and recovery. |
| pkg/linters/regexpcompileinfunction/regexpcompileinfunction.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/rawloginlib/rawloginlib.go | Avoids panics from analyzer result and AST node type assertions. |
| pkg/linters/panic-in-library-code/panic-in-library-code.go | Avoids panics from analyzer result and AST node type assertions. |
| pkg/linters/ossetenvlibrary/ossetenvlibrary.go | Avoids panics from analyzer result and AST node type assertions. |
| pkg/linters/osexitinlibrary/osexitinlibrary.go | Avoids panics from analyzer result and AST node type assertions. |
| pkg/linters/manualmutexunlock/manualmutexunlock.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/largefunc/largefunc.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/fprintlnsprintf/fprintlnsprintf.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/excessivefuncparams/excessivefuncparams.go | Avoids panics from analyzer result and AST node type assertions. |
| pkg/linters/errstringmatch/errstringmatch.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/errormessage/errormessage.go | Avoids panic on unexpected analyzer result types. |
| pkg/linters/ctxbackground/ctxbackground.go | Avoids panic on unexpected analyzer result types. |
| pkg/cli/workflows.go | Fixes file-close deferral ordering and hardens sync.Pool type assertions. |
| pkg/cli/update_manifest.go | Refactors manifest-managed workflow update function to a single struct parameter. |
| pkg/cli/jsonworkflow_to_markdown.go | Avoids panics when interval trigger values aren’t strings; emits warnings instead. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 24/24 changed files
- Comments generated: 2
| if len(required) > 0 { | ||
| sort.Strings(required) | ||
| tool["inputSchema"].(map[string]any)["required"] = required | ||
| if inputSchema, ok := tool["inputSchema"].(map[string]any); ok { | ||
| inputSchema["required"] = required | ||
| } |
| if len(required) > 0 { | ||
| tool["inputSchema"].(map[string]any)["required"] = required | ||
| if inputSchema, ok := tool["inputSchema"].(map[string]any); ok { | ||
| inputSchema["required"] = required | ||
| } | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — Two correctness regressions and one cache race must be fixed before merge.
Blocking themes
Silent required-field drop in tool definitions
dispatch_repository.go and safe_outputs_workflow_helpers.go both use if inputSchema, ok := ...; ok { ... } to set the required array. When the assertion fails the required fields are silently discarded, producing an MCP tool definition that passes no mandatory-input validation. This is a correctness regression: the old panic at least surfaced the bug immediately; the new code hides it until runtime tool invocations fail in opaque ways. Both sites need to return an error.
TOCTOU cache eviction in repository_features_validation.go
After LoadOrStore, the !ok fallback calls repositoryFeaturesCache.Delete(repo), which can evict a concurrently stored valid entry. Since only *RepositoryFeatures is ever stored here the branch is unreachable under correct usage; the safe fix is to return the freshly fetched value rather than deleting.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.7M
| if len(required) > 0 { | ||
| tool["inputSchema"].(map[string]any)["required"] = required | ||
| if inputSchema, ok := tool["inputSchema"].(map[string]any); ok { | ||
| inputSchema["required"] = required |
There was a problem hiding this comment.
Silent data loss: required fields are silently dropped when the type assertion fails, producing a broken tool definition with no required-field validation.
💡 Details and fix
When tool["inputSchema"] is not map[string]any (e.g., nil, absent, or a different concrete type), the required fields are silently discarded:
if inputSchema, ok := tool["inputSchema"].(map[string]any); ok {
inputSchema["required"] = required
}
// required is gone; caller has no ideaA missing required in an MCP tool definition means client-side schema validation will not enforce mandatory inputs. Errors will appear much later as obscure runtime failures rather than validation errors. The previous code panicked, which at least was detectable immediately in tests.
Fix: return an error to the caller instead of silently continuing:
inputSchema, ok := tool["inputSchema"].(map[string]any)
if !ok {
return nil, fmt.Errorf("dispatch_repository tool %q: inputSchema has unexpected type %T", toolName, tool["inputSchema"])
}
inputSchema["required"] = requiredThe same silent-drop pattern exists in pkg/workflow/safe_outputs_workflow_helpers.go.
| sort.Strings(required) | ||
| tool["inputSchema"].(map[string]any)["required"] = required | ||
| if inputSchema, ok := tool["inputSchema"].(map[string]any); ok { | ||
| inputSchema["required"] = required |
There was a problem hiding this comment.
Same silent required-field drop as dispatch_repository.go: if tool["inputSchema"] type assertion fails, sorted required fields are discarded without error or log.
💡 Details and fix
if inputSchema, ok := tool["inputSchema"].(map[string]any); ok {
inputSchema["required"] = required
}
// required is silently goneA safe-output tool definition missing its required array will pass construction silently but fail at runtime when the MCP client calls the tool with missing arguments — the schema has no required-field enforcement. This is a regression from the prior panic which would have surfaced the bug immediately.
Fix: propagate an error:
inputSchema, ok := tool["inputSchema"].(map[string]any)
if !ok {
return nil, fmt.Errorf("workflow tool %q: inputSchema has unexpected type %T", toolName, tool["inputSchema"])
}
inputSchema["required"] = required| actualFeatures := actual.(*RepositoryFeatures) | ||
| actualFeatures, ok := actual.(*RepositoryFeatures) | ||
| if !ok { | ||
| repositoryFeaturesCache.Delete(repo) |
There was a problem hiding this comment.
TOCTOU race: repositoryFeaturesCache.Delete(repo) after a failed LoadOrStore assertion evicts entries that another goroutine legitimately stored.
💡 Details and fix
After LoadOrStore, actual could be another goroutine's freshly stored *RepositoryFeatures. If the assertion fails, Delete removes the key regardless — potentially evicting a valid entry that a third goroutine is about to serve from cache.
actual, loaded := repositoryFeaturesCache.LoadOrStore(repo, features)
actualFeatures, ok := actual.(*RepositoryFeatures)
if !ok {
repositoryFeaturesCache.Delete(repo) // evicts another goroutine's valid entry
return nil, fmt.Errorf(...)
}Since only *RepositoryFeatures values are ever stored here, this branch is unreachable under correct usage. The safe fallback is to return the freshly fetched features rather than deleting and propagating an error:
actualFeatures, ok := actual.(*RepositoryFeatures)
if !ok {
repositoryFeaturesLog.Printf("unexpected cache type for %s: %T; returning freshly fetched value", repo, actual)
return features, nil
}Or treat it as a panic (programming error) so tests catch it immediately instead of hiding it in production.
Auto-generated by the Design Decision Gate. The PR author should review, complete, and finalize this document before merging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (207 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. The choice of four different recovery modes for unchecked type assertions (instead of one helper, instead of 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — approving with minor observations.
📋 Key Themes & Highlights
Key Themes
- Defensive dead code in linter Preorder callbacks: The
!okbranches insideinsp.Preorderclosures are unreachable becausenodeFilteralready guarantees the concrete type. These branches satisfy the linter but are confusing without a comment. Applies toexcessivefuncparams,osexitinlibrary,ossetenvlibrary,rawloginlib, andpanic-in-library-code. nilreturn injsonworkflow_to_markdown.go: Returningnilfor theanyresult when an interval type assertion fails trades a panic for a potential downstream nil-dereference. Worth confirming callers handle nil, or returningpartsinstead.- No regression tests: The three bug fixes (defer ordering, pool nil-path, interval assertion) have no test coverage. The fixes look correct but lack anchors against future regressions.
Positive Highlights
- ✅
manifestManagedWorkflowUpdatestruct is a clean, well-named refactor — eliminates an entire class of silent positional-argument bugs - ✅ The named-return + defer pattern in
workflows.gois idiomatic Go for propagating close errors - ✅ Consistent error message format (
"... has unexpected type %T") across all 12 linter fixes makes future debugging straightforward - ✅ Cache eviction in
virtual_fs.gois a safe fallback, though it would benefit from a more visible error log
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.8M
| if hasInterval && len(parts) == 1 { | ||
| return intervalType.(string), warnings | ||
| interval, ok := intervalType.(string) | ||
| if !ok { |
There was a problem hiding this comment.
[/diagnose] return nil, warnings may cause a downstream nil-dereference if callers don't guard against a nil any return.
💡 Details & suggestion
When intervalType isn't a string, returning nil for the any result is safer than panicking, but callers that immediately coerce the return into a map or pass it to YAML marshalling will dereference nil and panic anyway.
Consider returning parts (the partially-built map) with the warning appended, which keeps the contract identical to the normal multi-trigger path:
if !ok {
warnings = append(warnings, "triggers.interval must resolve to a string value; skipped")
return parts, warnings // return whatever else was accumulated
}Alternatively, add a regression test that covers the nil return path and verifies callers handle it safely.
| (*ast.FuncDecl)(nil), | ||
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { |
There was a problem hiding this comment.
[/zoom-out] The !ok branch in insp.Preorder is unreachable dead code — nodeFilter already guarantees the node type.
💡 Context
golang.org/x/tools/go/analysis/passes/inspect only calls the Preorder callback for node types that appear in nodeFilter. Since nodeFilter contains only (*ast.FuncDecl)(nil), n will always be *ast.FuncDecl and the assertion can never fail.
The same pattern is repeated in several other linters in this PR (osexitinlibrary, ossetenvlibrary, rawloginlib, panic-in-library-code). The code is harmless and satisfies the linter, but a brief comment would prevent future readers from wondering why it exists:
// nodeFilter guarantees n is *ast.FuncDecl; ok-check is for linter compliance.
fn, ok := n.(*ast.FuncDecl)
if !ok {
return
}| // Reuse the small initial scanner buffer across calls while still allowing | ||
| // growth up to 1 MB for large frontmatter values or long base64-encoded lines. | ||
| scannerBufferPtr := workflowTitleScannerBufferPool.Get().(*[]byte) | ||
| pooled := workflowTitleScannerBufferPool.Get() |
There was a problem hiding this comment.
[/zoom-out] The !ok check here is unreachable — the pool only ever stores *[]byte, so the assertion will always succeed.
💡 Details
sync.Pool.Get() returns nil when the pool is empty (before any Put), but it does not return wrong-typed values. Since the pool is only ever populated with *[]byte, pooled.(*[]byte) will either succeed or pooled will be nil.
The nil guard is the genuinely necessary check here. The !ok branch is dead code for the same reason as the Preorder cases. A comment clarifying why both are present would help:
// Get() returns nil when the pool is empty; type assertion always succeeds when non-nil.
pooled := workflowTitleScannerBufferPool.Get()
scannerBufferPtr, _ := pooled.(*[]byte) // nil on empty pool, *[]byte otherwise
if scannerBufferPtr == nil {
fallback := make([]byte, workflowTitleScannerBufferSize)
scannerBufferPtr = &fallback
}| } | ||
| result, ok := v.(*FrontmatterResult) | ||
| if !ok { | ||
| virtualFsLog.Printf("Frontmatter cache type mismatch for %s: got %T", path, v) |
There was a problem hiding this comment.
[/diagnose] Silently evicting the cache entry on type mismatch could mask a programming error — consider a more visible signal.
💡 Details
A type mismatch here means something called builtinFrontmatterCache.Store(path, wrongType), which should never happen. Silently deleting the entry and returning a miss recovers gracefully, but makes it very hard to notice if a bug were ever introduced at the Store site.
Consider emitting a warning log (or an error-level log) alongside the delete:
result, ok := v.(*FrontmatterResult)
if !ok {
// This indicates a programming error at a Store call site.
virtualFsLog.Printf("[ERROR] Frontmatter cache type mismatch for %s: got %T — evicting", path, v)
builtinFrontmatterCache.Delete(path)
return nil, false
}This preserves the safe recovery path while making the anomaly observable.
| // extractWorkflowNameFromFile extracts the workflow name from a file's H1 header | ||
| func extractWorkflowNameFromFile(filePath string) (string, error) { | ||
| func extractWorkflowNameFromFile(filePath string) (title string, err error) { | ||
| fd, err := os.Open(filePath) |
There was a problem hiding this comment.
[/tdd] The defer-based close fix is correct and well-structured, but there are no regression tests covering the close-error propagation path.
💡 Suggested test sketch
A table-driven test using a *os.File-wrapping type that injects a close error would pin this behaviour and guard against future regressions:
func TestExtractWorkflowNameFromFile_CloseError(t *testing.T) {
// Write a minimal frontmatter file
f := t.TempDir() + "/test.md"
os.WriteFile(f, []byte("---\ntitle: test\n---\n# My Workflow\n"), 0644)
// Happy path: no close error
title, err := extractWorkflowNameFromFile(f)
assert.NoError(t, err)
assert.Equal(t, "My Workflow", title)
}Testing the close-error injection path is harder without an interface seam, but the happy path test alone provides a useful regression anchor for the named-return + defer pattern.
|
@copilot review all comments and review comments |
…able Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review comments — bound the |
…fix-unchecked-type-assertions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…to fix hash mismatch Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The Fixed by merging |
This addresses the lint group reporting 33 issues in three buckets: unchecked single-value type assertions, one over-parameterized function, and a file-close defer ordering issue. The changes focus on replacing panic-prone assertions with checked paths and tightening the relevant call sites.
Unchecked type assertions (31 findings)
pkg/parser,pkg/workflow,pkg/cli, andpkg/linters/*.Function parameter limit (1 finding)
updateManifestManagedWorkflowinpkg/cli/update_manifest.goto accept a singlemanifestManagedWorkflowUpdatestruct instead of 9 positional parameters.updateManifestWorkflowGroupto build and pass the struct.Resource management (1 finding)
pkg/cli/workflows.go, moved file close handling to an immediatedeferafteros.Openand preserved close-error propagation via deferred error assignment.