Problem
Six safe-output config parsers in pkg/workflow still use the old manual unmarshalConfig pattern instead of the parseConfigScaffold generic scaffold introduced to abstract this pattern. This creates inconsistency: newer handlers like assign_to_agent.go and assign_to_user.go already use the scaffold correctly.
Affected Handlers
| File |
Function |
Pattern |
pkg/workflow/create_issue.go |
parseIssuesConfig |
manual unmarshalConfig + fallback |
pkg/workflow/create_discussion.go |
parseDiscussionsConfig |
manual unmarshalConfig + fallback |
pkg/workflow/create_pull_request.go |
parsePullRequestsConfig |
manual unmarshalConfig + fallback |
pkg/workflow/add_comment.go |
parseCommentsConfig |
manual unmarshalConfig + fallback |
pkg/workflow/add_reviewer.go |
parseAddReviewerConfig |
manual unmarshalConfig + fallback |
pkg/workflow/close_entity_helpers.go |
parseCloseEntityConfig |
manual unmarshalConfig + fallback |
Current Pattern (to replace)
// create_issue.go lines 58-64
var config CreateIssuesConfig
if err := unmarshalConfig(outputMap, "create-issue", &config, createIssueLog); err != nil {
createIssueLog.Printf("Failed to unmarshal config: %v", err)
// For backward compatibility, handle nil/empty config
config = CreateIssuesConfig{}
}
Target Pattern (already used in assign_to_agent.go, assign_to_user.go)
config := parseConfigScaffold(outputMap, "create-issue", createIssueLog, func(err error) *CreateIssuesConfig {
createIssueLog.Printf("Failed to unmarshal config: %v", err)
return &CreateIssuesConfig{}
})
Migration Notes
These handlers have preprocessing steps (preprocessBoolFieldAsString, preprocessIntFieldAsString) that must run before parseConfigScaffold. Since the preprocessing mutates the config sub-map in-place, the correct migration is:
- Extract
configData, _ := outputMap["key"].(map[string]any) (nil-safe, no-op if absent)
- Run all preprocessing on
configData
- Call
parseConfigScaffold (returns nil if key absent — correct behaviour)
This is exactly the pattern in assign_to_user.go:19-47.
Recommendation
For each of the 6 handlers:
- Add the key existence + configData extraction before preprocessing
- Replace the
if err := unmarshalConfig(...) block with parseConfigScaffold(...)
- Keep all preprocessing unchanged
Severity
- Medium — consistency/maintainability, no runtime behavior change
Validation
Generated by Sergo - Serena Go Expert · ● 901.5K · ◷
Problem
Six safe-output config parsers in
pkg/workflowstill use the old manualunmarshalConfigpattern instead of theparseConfigScaffoldgeneric scaffold introduced to abstract this pattern. This creates inconsistency: newer handlers likeassign_to_agent.goandassign_to_user.goalready use the scaffold correctly.Affected Handlers
pkg/workflow/create_issue.goparseIssuesConfigunmarshalConfig+ fallbackpkg/workflow/create_discussion.goparseDiscussionsConfigunmarshalConfig+ fallbackpkg/workflow/create_pull_request.goparsePullRequestsConfigunmarshalConfig+ fallbackpkg/workflow/add_comment.goparseCommentsConfigunmarshalConfig+ fallbackpkg/workflow/add_reviewer.goparseAddReviewerConfigunmarshalConfig+ fallbackpkg/workflow/close_entity_helpers.goparseCloseEntityConfigunmarshalConfig+ fallbackCurrent Pattern (to replace)
Target Pattern (already used in assign_to_agent.go, assign_to_user.go)
Migration Notes
These handlers have preprocessing steps (
preprocessBoolFieldAsString,preprocessIntFieldAsString) that must run beforeparseConfigScaffold. Since the preprocessing mutates the config sub-map in-place, the correct migration is:configData, _ := outputMap["key"].(map[string]any)(nil-safe, no-op if absent)configDataparseConfigScaffold(returns nil if key absent — correct behaviour)This is exactly the pattern in
assign_to_user.go:19-47.Recommendation
For each of the 6 handlers:
if err := unmarshalConfig(...)block withparseConfigScaffold(...)Severity
Validation
go build ./pkg/workflow/passesgo test ./pkg/workflow/...passes