Split safe outputs handler registry by domain - #55482
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
There was a problem hiding this comment.
Pull request overview
Refactors the monolithic safe-output handler registry into domain-focused files for #55406.
Changes:
- Composes six domain registries through
mergeHandlerMaps. - Extracts handler builders while preserving lookup behavior.
- Adds registry composition, builder, duplicate-key, and token tests.
Show a summary per file
| File | Description |
|---|---|
safe_outputs_handler_registry.go |
Composes domain registries. |
safe_outputs_handler_registry_issues.go |
Contains issue handlers. |
safe_outputs_handler_registry_discussions.go |
Contains discussion handlers. |
safe_outputs_handler_registry_pull_requests.go |
Contains pull-request handlers and extracted builder helpers. |
safe_outputs_handler_registry_workflow.go |
Contains workflow and artifact handlers. |
safe_outputs_handler_registry_projects.go |
Contains project and assignment handlers. |
safe_outputs_handler_registry_misc.go |
Contains miscellaneous handlers. |
safe_outputs_handler_registry_test.go |
Tests composition and helper behavior. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| @@ -0,0 +1,130 @@ | |||
| package workflow | |||
|
|
|||
| // projectHandlerRegistry contains project board and assignment handler builders. | |||
| @@ -0,0 +1,123 @@ | |||
| package workflow | |||
|
|
|||
| // miscHandlerRegistry contains comment, release, diagnostic, and no-op handler builders. | |||
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review outcome
I didn't find any changed-line correctness, security, or performance regressions in this registry split.
Notes
- The refactor preserves the existing builder logic by moving handlers verbatim into domain-scoped registries.
mergeHandlerMapsdeterministically keeps the first duplicate and emits a debug log, and the new tests cover registry composition plus duplicate-key behavior.- I discarded the requested sub-agent pass because
grumpy-coderis not available in this environment.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 19.7 AIC · ⌖ 6.86 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Review: Split safe outputs handler registry by domain
Clean structural refactor — splitting a 1000+ line monolithic registry into domain-scoped files is a good maintainability win. The mergeHandlerMaps helper and Go's dependency-driven init ordering make this functionally safe.
One minor issue: stale (see create-issue handler above) comments in two new files where create_issue is no longer co-located.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 56.6 AIC · ⌖ 9.1 AIC · ⊞ 6.2K
| AddStringSlice("labels", c.Labels). | ||
| AddStringSlice("allowed_labels", c.AllowedLabels). | ||
| AddStringSlice("allowed_repos", c.AllowedRepos). | ||
| // entity-specific env key name per shared CloseOlderConfig field (see create-issue handler above) |
There was a problem hiding this comment.
Stale cross-file comment — (see create-issue handler above) is now incorrect after the split. The create_issue handler lives in safe_outputs_handler_registry_issues.go, not in this file. Update to reference the correct file:
// entity-specific env key name per shared CloseOlderConfig.Enabled; see create_issue handler
// in safe_outputs_handler_registry_issues.go for the canonical explanation.@copilot please address this.
| AddIfTrue("recreate_ref", c.RecreateRef). | ||
| AddIfNotEmpty("patch_format", c.PatchFormat). | ||
| AddBoolPtr("signed_commits", c.SignedCommits). | ||
| // entity-specific env key name per shared CloseOlderConfig field (see create-issue handler above) |
There was a problem hiding this comment.
Stale cross-file comment — (see create-issue handler above) is now incorrect after the split. The create_issue handler lives in safe_outputs_handler_registry_issues.go, not above in this file. Update the comment to reference the correct file explicitly.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting minor changes.
📋 Key Themes & Highlights
Key Themes
- Style inconsistency carried over: several new files contain multiple chained
.Add*calls on the same line, copied verbatim from the original monolith. The refactor is a good opportunity to normalise to one-call-per-line throughout. miscregistry as an everything-else drawer:add_commentandhide_commentare cross-cutting (apply to issues, discussions, and PRs), which makes them awkward undermiscHandlerRegistry. A dedicatedcommentsHandlerRegistryor a rename would better communicate scope.- Test coverage for
nil-vs-empty-map semantics:add_labels,replace_label,set_issue_type, andset_issue_fieldall returnmake(map[string]any)(enabled, all-allowed) when built with an empty config — this behaviour is not exercised by the current test table. - Duplicate-key length assertion missing:
TestMergeHandlerMapsKeepsFirstDuplicateKeychecks the winning builder's value but not that the merged map has exactly one key.
Positive Highlights
- ✅ Excellent use of
mergeHandlerMaps— registry composition is clean and call sites are unchanged - ✅ New test file covers domain membership, builder enable/disable, token-helper paths, and duplicate-key policy — great baseline
- ✅ Stale inline comments that referenced "see create-issue handler above" now make less sense in a cross-file split — though the moved comment (
// entity-specific env key name per shared CloseOlderConfig field (see create-issue handler above)) still appears indiscussions.go; worth updating the cross-reference.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 102.3 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
| AddTemplatableInt("max", c.Max). | ||
| AddStringSlice("allowed_fields", c.AllowedFields). | ||
| AddBoolPtr("issue_intent", c.IssueIntent). | ||
| AddIfNotEmpty("target", c.Target).AddStringSlice("required_labels", c.RequiredLabels). |
There was a problem hiding this comment.
[/codebase-design] Chained Add calls on one line were carried over verbatim from the original monolith — this inconsistency makes the new file harder to scan than the handlers where each call is on its own line.
💡 Suggested fix
Split each call onto its own line:
AddIfNotEmpty("target", c.Target).
AddStringSlice("required_labels", c.RequiredLabels).
AddIfNotEmpty("required_title_prefix", c.RequiredTitlePrefix).
AddIfNotEmpty("target-repo", c.TargetRepoSlug).The same style inconsistency recurs in misc.go (~line 34), projects.go (~line 57), and pull_requests.go (~lines 68, 165).
@copilot please address this.
| @@ -0,0 +1,123 @@ | |||
| package workflow | |||
There was a problem hiding this comment.
[/codebase-design] add_comment is a cross-cutting handler that works on issues, discussions, and PRs, yet it lives in miscHandlerRegistry whose doc-comment says "comment, release, diagnostic, and no-op handlers." The misc bucket risks growing into an everything-else drawer over time, making domain navigation harder.
💡 Options
- Move
add_commentandhide_commentinto a dedicatedsafe_outputs_handler_registry_comments.gowith acommentsHandlerRegistry. - Or rename this file's registry to
crossCuttingHandlerRegistryand update the doc-comment to explicitly describe its scope.
Either approach prevents the misc bucket from silently absorbing future cross-entity handlers.
@copilot please address this.
| cfg *SafeOutputsConfig | ||
| }{ | ||
| {name: "create_issue", cfg: &SafeOutputsConfig{CreateIssues: &CreateIssuesConfig{}}}, | ||
| {name: "close_issue", cfg: &SafeOutputsConfig{CloseIssues: &CloseIssuesConfig{}}}, |
There was a problem hiding this comment.
[/tdd] TestHandlerRegistryBuilders tests the enabled path with minimal structs (e.g. &CreateIssuesConfig{}), but handlers that return make(map[string]any) for an empty config — like add_labels, replace_label, set_issue_type, and set_issue_field — are not tested for the "configured with null / no options means all allowed" semantic.
💡 Suggested additions
// Test that add_labels with explicit nil sub-fields still returns non-nil (all-allowed)
{name: "add_labels_all_allowed", cfg: &SafeOutputsConfig{AddLabels: &AddLabelsConfig{}}, wantNonNil: true},Add a wantNonEmpty bool field to the test table to distinguish between "returns nil map" (disabled), "returns non-nil empty map" (enabled, all allowed), and "returns non-nil populated map" (enabled, restricted).
@copilot please address this.
| {name: "create_report_incomplete_issue", cfg: &SafeOutputsConfig{ReportIncomplete: &ReportIncompleteConfig{CreateIssue: strPtr("true")}}}, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
[/tdd] The duplicate-key test (TestMergeHandlerMapsKeepsFirstDuplicateKey) only checks the value; it doesn't assert that the merged map has exactly one entry for the duplicate key. A bug that keeps both keys under different aliases would not be caught.
💡 Suggested addition
if len(got) != 1 {
t.Fatalf("mergeHandlerMaps: want 1 key, got %d", len(got))
}@copilot please address this.
There was a problem hiding this comment.
The main simplification I’d make here is to collapse the category-split registry back into a single table and drop the extra merge helper; the runtime behavior is unchanged and the code becomes easier to scan. net: -17 lines possible.
Generated by ✂️ Ponytail Reviewer for #55482 · codex · mai10 · 6.73 AIC · ⌖ 1.59 AIC · ⊞ 18.3K
Comment /ponytail to run again
| if len(c.AwContextWorkflows) > 0 { | ||
| builder.AddStringSlice("aw_context_workflows", c.AwContextWorkflows) | ||
| } | ||
| var handlerRegistry = mergeHandlerMaps( |
There was a problem hiding this comment.
pkg/workflow/safe_outputs_handler_registry.go:57-77: yagni: split registry into six files plus a merge helper for a fixed lookup table. Keep one map in one file.
ADR Required — Draft GeneratedStatus: Draft ADR committed to this branch · Merge blocked pending author review A draft Architecture Decision Record was automatically generated for this PR because it adds 1,347 lines of code to The draft has been committed to this branch at: What to do next
Draft ADR summaryDecision: Decompose the monolithic Key trade-offs captured:
|
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32774702076
|
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>
Follow-up pass pushed:
Registry tests pass; remaining |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/safe_outputs_handler_registry.gowas a 1K+ line monolith centered on one large registry literal. This refactor separates handler builders by safe-output domain while keeping registry lookup behavior unchanged.handlerRegistryassembly insafe_outputs_handler_registry.gomergeHandlerMaps(...)to compose focused domain registries without changing call sitesDomain-focused handler files
safe_outputs_handler_registry_issues.gosafe_outputs_handler_registry_discussions.gosafe_outputs_handler_registry_pull_requests.gosafe_outputs_handler_registry_workflow.gosafe_outputs_handler_registry_projects.gosafe_outputs_handler_registry_misc.goCoverage for the split