Deduplicate safe-output parser wrapper boilerplate with parseConfigScaffoldWithPostProcess - #52158
Conversation
…arser wrappers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #52158 for over-engineering. This is a pure deduplication refactor: parseConfigScaffoldWithPostProcess has 13 real call sites, each replacing a previously duplicated 5-8 line nil-check/default/log wrapper. No speculative abstraction, no unused flexibility, no reinvented stdlib. Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped refactor. The new parseConfigScaffoldWithPostProcess helper correctly eliminates repeated nil-check wrapper boilerplate across all safe-output parsers. The nil-guard on postProcess and the unchanged behavior for onError-returning-nil are both handled correctly. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.4 AIC · ⌖ 6.14 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Centralizes safe-output parser post-processing to reduce repeated nil checks, defaulting, and logging across workflow handlers.
Changes:
- Adds a generic post-processing parser scaffold.
- Migrates 13 safe-output configuration parsers.
- Preserves handler-specific defaults and compatibility logic.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/config_helpers.go |
Adds the shared post-processing helper. |
pkg/workflow/add_comment.go |
Migrates comment configuration parsing. |
pkg/workflow/add_labels.go |
Migrates add-label parsing. |
pkg/workflow/add_reviewer.go |
Moves defaults and legacy-field handling into the callback. |
pkg/workflow/assign_milestone.go |
Migrates milestone parsing and logging. |
pkg/workflow/assign_to_agent.go |
Moves default maximum handling into the callback. |
pkg/workflow/assign_to_user.go |
Moves default maximum handling into the callback. |
pkg/workflow/close_entity_helpers.go |
Moves close-entity defaults and compatibility mapping. |
pkg/workflow/mark_pull_request_as_ready_for_review.go |
Moves target and filter extraction into the callback. |
pkg/workflow/remove_labels.go |
Migrates remove-label parsing. |
pkg/workflow/replace_label.go |
Migrates replace-label parsing. |
pkg/workflow/set_issue_field.go |
Migrates issue-field parsing. |
pkg/workflow/set_issue_type.go |
Migrates issue-type parsing. |
pkg/workflow/unassign_from_user.go |
Moves default maximum handling into the callback. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
| // parseConfigScaffoldWithPostProcess wraps parseConfigScaffold and additionally invokes the | ||
| // optional postProcess callback when parsing succeeds (i.e. the returned config is non-nil). |
| // assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d", | ||
| // config.Target, len(config.Allowed)) | ||
| // }) | ||
| func parseConfigScaffoldWithPostProcess[T any]( |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two minor issues found, no blocking concerns.
📋 Key Themes & Highlights
Issues
- Missing test:
parseConfigScaffoldWithPostProcessis the new central abstraction but has no unit test. Given thatparseConfigScaffolditself likely lacks a test, this is a good opportunity to add one. - Style inconsistency: Seven callers use
config := ...; return configinstead of the simplerreturn ...pattern used by the other seven callers.
Positive Highlights
- ✅ Clean, minimal implementation (12 lines) that does exactly one thing
- ✅ Nil-safe
postProcessguard is correct and well-documented - ✅ PR description is clear, with before/after examples
- ✅
create_entity_helpers.gocorrectly left untouched — good judgment on scope - ✅ No behavioral changes; purely mechanical deduplication of 186 lines
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.7 AIC · ⌖ 6.8 AIC · ⊞ 7K
Comment /matt to run again
| key string, | ||
| debugLog *logger.Logger, | ||
| onError func(err error) *T, | ||
| postProcess func(config *T), |
There was a problem hiding this comment.
[/tdd] No unit test covers parseConfigScaffoldWithPostProcess — the new generic helper is the central abstraction of this PR, but config_helpers_test.go does not exist.
💡 Suggested test skeleton
func TestParseConfigScaffoldWithPostProcess_CallsPostProcess(t *testing.T) {
called := false
got := parseConfigScaffoldWithPostProcess(map[string]any{"key": map[string]any{}}, "key", someLog,
func(err error) *SomeConfig { return &SomeConfig{} },
func(cfg *SomeConfig) { called = true },
)
if got == nil || !called {
t.Fatalf("expected non-nil config and postProcess called")
}
}
func TestParseConfigScaffoldWithPostProcess_NilPostProcessIsNoop(t *testing.T) {
got := parseConfigScaffoldWithPostProcess(map[string]any{"key": map[string]any{}}, "key", someLog,
func(err error) *SomeConfig { return &SomeConfig{} },
nil,
)
if got == nil {
t.Fatal("expected non-nil config")
}
}Edge cases worth covering: nil postProcess, parse failure via onError, missing key in outputMap.
@copilot please address this.
| } | ||
| }) | ||
|
|
||
| return config |
There was a problem hiding this comment.
[/codebase-design] Minor style inconsistency: some callers assign the result to a local config variable then immediately return config, while others (e.g. add_labels.go, assign_milestone.go) use return parseConfigScaffoldWithPostProcess(...) directly. The assign-then-return form adds a line of noise with no benefit.
💡 Preferred pattern
// Before (this file and add_reviewer.go, assign_to_agent.go, assign_to_user.go, etc.)
config := parseConfigScaffoldWithPostProcess(...)
return config
// After — consistent with add_labels.go, assign_milestone.go
return parseConfigScaffoldWithPostProcess(...)Affected files: add_comment.go, add_reviewer.go, assign_to_agent.go, assign_to_user.go, close_entity_helpers.go, mark_pull_request_as_ready_for_review.go, unassign_from_user.go.
@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (213 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. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot This PR needs a finishing pass before maintainers look again. Please refresh the branch context, address outstanding feedback, and run the Open items:
Run context: https://github.com/github/gh-aw/actions/runs/31545290203
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Finishing pass done in edc89ef:
|
🤖 Triage: PR #52158
|
Many safe-output handlers already used the shared
parseConfigScaffoldhelper, but each caller repeated the same outer wrapper: a nil-check, default-value assignment, and post-parse debug logging. This pattern was duplicated across 13+ non-test files inpkg/workflow.Changes
parseConfigScaffoldWithPostProcess[T any]inpkg/workflow/config_helpers.go. It wrapsparseConfigScaffoldand invokes an optionalpostProcess(config *T)callback only when parsing succeeds, replacing the repeatedif config == nil { return nil } ... return configwrapper.postProcessclosure:add_comment.go,add_labels.go,add_reviewer.go,assign_milestone.go,assign_to_agent.go,assign_to_user.go,close_entity_helpers.go,mark_pull_request_as_ready_for_review.go,remove_labels.go,replace_label.go,set_issue_field.go,set_issue_type.go,unassign_from_user.go.create_entity_helpers.gountouched — it already has its own generic scaffold (parseCreateEntityConfig) with pre/post hooks tailored to the create-* handler family.No behavioral changes; existing default values, log messages, and error handling are preserved exactly.
Before:
After: