Extract shared SafeOutputAllowBlockConfig across safe-output handlers#42322
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (99 additions, threshold is 100). |
There was a problem hiding this comment.
Pull request overview
This PR refactors safe-output handler configuration structs to eliminate duplication of the allowed / blocked YAML fields by introducing a shared embedded config type, then updates existing tests to construct the new embedded config explicitly.
Changes:
- Added
SafeOutputAllowBlockConfigto centralize the canonical YAML definition forallowedandblocked. - Replaced duplicated per-handler
Allowed/Blockedfields with inline embedding across several safe-output handler config structs (andListJobConfig). - Updated workflow/compiler/CLI tests to initialize the embedded config in composite literals.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_parser.go | Introduces SafeOutputAllowBlockConfig and embeds it into ListJobConfig. |
| pkg/workflow/add_labels.go | Embeds the shared allow/block config into AddLabelsConfig. |
| pkg/workflow/remove_labels.go | Embeds the shared allow/block config into RemoveLabelsConfig. |
| pkg/workflow/assign_to_user.go | Embeds the shared allow/block config into AssignToUserConfig. |
| pkg/workflow/unassign_from_user.go | Embeds the shared allow/block config into UnassignFromUserConfig. |
| pkg/workflow/set_issue_type.go | Embeds the shared allow/block config into SetIssueTypeConfig. |
| pkg/workflow/assign_milestone.go | Embeds the shared allow/block config into AssignMilestoneConfig. |
| pkg/workflow/safe_outputs_tools_test.go | Updates tool-description enhancement test fixtures to use the embedded config. |
| pkg/workflow/safe_outputs_config_generation_test.go | Updates safe-outputs config generation fixtures to use the embedded config. |
| pkg/workflow/compiler_safe_outputs_job_test.go | Updates compiler job-related fixtures to use the embedded config. |
| pkg/workflow/compiler_safe_outputs_env_test.go | Updates env var compilation fixtures to use the embedded config. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Updates handler config/env var fixtures to use the embedded config. |
| pkg/cli/compile_workflow_processor_test.go | Updates CLI compilation processor fixtures to use the embedded config. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Low
| type SetIssueTypeConfig struct { | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed issue type names. If omitted, any type is allowed (including clearing with ""). | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Fixed in the latest commit — SetIssueTypeConfig now uses a direct Allowed []string field again instead of embedding SafeOutputAllowBlockConfig, so no blocked: key is exposed in YAML for this handler.
| type AssignMilestoneConfig struct { | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed milestone titles or IDs | ||
| AutoCreate bool `yaml:"auto_create,omitempty"` // If true, auto-create missing milestones found in the allowed list | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` | ||
| AutoCreate bool `yaml:"auto_create,omitempty"` // If true, auto-create missing milestones found in the allowed list |
There was a problem hiding this comment.
Fixed in the latest commit — AssignMilestoneConfig now uses a direct Allowed []string field again instead of embedding SafeOutputAllowBlockConfig, so no blocked: key is exposed in YAML for this handler.
| type AssignToUserConfig struct { | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed usernames. If omitted, any users are allowed. | ||
| Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked usernames or patterns (e.g., "copilot", "*[bot]") | ||
| UnassignFirst *string `yaml:"unassign-first,omitempty"` // If true, unassign all current assignees before assigning new ones | ||
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` | ||
| UnassignFirst *string `yaml:"unassign-first,omitempty"` // If true, unassign all current assignees before assigning new ones | ||
| } |
There was a problem hiding this comment.
Fixed in the latest commit — safe_outputs_integration_test.go now initializes the embedded struct explicitly: SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{Allowed: []string{"user1"}}. go vet -tags=integration passes cleanly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Extract shared SafeOutputAllowBlockConfig
The structural refactor is clean and the test updates are correct. However, there are two correctness bugs introduced by applying the shared config too broadly.
Blocking: Silent blocked config loss for assign_milestone and set_issue_type
Both AssignMilestoneConfig and SetIssueTypeConfig previously had only Allowed []string — neither ever supported Blocked. By embedding SafeOutputAllowBlockConfig, both structs now gain a Blocked field and will happily parse it from YAML. But the handler entries in safe_outputs_handler_registry.go were not updated: neither assign_milestone nor set_issue_type calls AddStringSlice("blocked", c.Blocked). Any blocked: list a user adds to these handlers will be silently dropped at runtime — no error, no warning.
The handlers that already supported blocked before this PR (add_labels, remove_labels, assign_to_user, unassign_from_user) are fine — their registry entries were already correct.
Fix options:
- Add
AddStringSlice("blocked", c.Blocked)to both handler registry entries (and add tests), or - Introduce a narrower
SafeOutputAllowOnlyConfigstruct with onlyAllowed, and embed that inAssignMilestoneConfigandSetIssueTypeConfiginstead.
Minor: Doc comment specificity lost for SetIssueTypeConfig
The original Allowed comment carried the note: "If omitted, any type is allowed (including clearing with "")." This is now replaced by the generic comment on SafeOutputAllowBlockConfig. Consider restoring this via a struct-level doc comment on SetIssueTypeConfig.
References:
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 43.5 AIC · ⌖ 7.02 AIC · ⊞ 4.9K
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Silent config loss: Blocked field added but not emitted by handler registry.
Embedding SafeOutputAllowBlockConfig silently introduces a Blocked []string field into AssignMilestoneConfig. However, the assign_milestone entry in safe_outputs_handler_registry.go only calls AddStringSlice("allowed", c.Allowed) — it never calls AddStringSlice("blocked", c.Blocked).
assign_milestone previously had only Allowed (no Blocked), so blocked-list support was never wired up. Any blocked: list a user writes in their YAML will be parsed and accepted by the struct, then silently discarded at runtime — no error, no warning.
Fix by either:
- Adding
AddStringSlice("blocked", c.Blocked)to theassign_milestonehandler entry in the registry, or - Introducing a narrower shared config (e.g.,
SafeOutputAllowOnlyConfig) that contains onlyAllowed, and embed that here instead.
@copilot please address this.
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Silent config loss: Blocked field added but not emitted by handler registry.
Same issue as AssignMilestoneConfig. Embedding SafeOutputAllowBlockConfig introduces a Blocked []string field, but the set_issue_type handler entry in safe_outputs_handler_registry.go only emits allowed, never blocked. A user who writes:
set-issue-type:
blocked: ["Epic", "Task"]will have the blocked list silently ignored at runtime.
Additionally, the original doc comment on Allowed read: "If omitted, any type is allowed (including clearing with "")." The note about clearing with empty string is important user-facing documentation and should be preserved (e.g., add a Blocked-specific doc comment to SafeOutputAllowBlockConfig that carries this context, or restore it via a struct-level comment here).
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 84/100 — Excellent
📊 Metrics & Test Classification (17 tests analyzed)
Go: 17 ( Score breakdown: behavioral 40/40 · edge-cases 14/30 · duplication 20/20 · inflation 10/10 = 84 Verdict
References: §28397842942
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — requesting changes on two correctness gaps.
📋 Key Themes & Highlights
Key Themes
-
Silent feature expansion:
AssignMilestoneConfigandSetIssueTypeConfigoriginally had noBlockedfield. By embeddingSafeOutputAllowBlockConfig, this PR adds ablocked:YAML key that the parser will happily accept — but the handler registry for bothassign_milestoneandset_issue_typedoes not forwardc.Blockedwhen building the runtime config map. The net effect is a silently ignored config field, which is worse than a parse error. -
Doc comment regression: The per-field comments in original structs carried handler-specific context (e.g. *including
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.6 AIC · ⌖ 7.58 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/assign_milestone.go:14
[/zoom-out] AssignMilestoneConfig never had a Blocked field before this PR — and the handler registry (safe_outputs_handler_registry.go lines 219-233) still does not forward c.Blocked. Embedding SafeOutputAllowBlockConfig silently adds a blocked: YAML key that will be accepted by the parser but dropped at runtime, giving users false confidence that blocking works.
<details>
<summary>💡 Options to resolve</summary>
Option A — Wire it up in the handler registry (add one line…
pkg/workflow/set_issue_type.go:14
[/zoom-out] Same issue as AssignMilestoneConfig: SetIssueTypeConfig previously had only Allowed, but now gains Blocked through the embed. The handler registry function for set_issue_type (lines 928-948 of safe_outputs_handler_registry.go) does not call AddStringSlice("blocked", c.Blocked), so a user who writes blocked: in their set-issue-type config will have it silently ignored.
This is in tension with the PR's stated goal of "no functional behavior changes" — it does ch…
pkg/workflow/safe_outputs_parser.go:32
[/improve-codebase-architecture] The shared doc comment // Optional list of allowed values loses context that each handler's original field comment carried. The previous comments were meaningfully specific — e.g., SetIssueTypeConfig.Allowed had // If omitted, any type is allowed (including clearing with "") and AssignMilestoneConfig.Allowed had // Optional list of allowed milestone titles or IDs.
<details>
<summary>💡 Suggestion</summary>
Consider keeping the handler-level conte…
pkg/workflow/assign_milestone.go:27
[/improve-codebase-architecture] The log message tracks allowed_count but not blocked_count — inconsistent with add_labels.go and remove_labels.go which log both after embedding the same struct. If Blocked is going to be part of this config, the log should reflect it.
assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d, blocked_count=%d",
config.Target, len(config.Allowed), len(config.Blocked))(Same applies to set_issue_type.go.)
@co…
There was a problem hiding this comment.
REQUEST_CHANGES — two high-severity silent-bypass bugs must be fixed before merge.
### Blocking issues (2)
1. assign_milestone and set_issue_type silently gained a blocked field that is never forwarded to the runtime.
Both AssignMilestoneConfig and SetIssueTypeConfig previously had no Blocked field. Embedding SafeOutputAllowBlockConfig introduces one that parses valid YAML without error, but the handler registry entries for assign_milestone (line 226) and set_issue_type (line 939) only call AddStringSlice("allowed", c.Allowed) — there is no corresponding AddStringSlice("blocked", c.Blocked) call for either. Any blocked: constraint an operator configures is silently dropped at compile time, giving a false sense of security.
Fix: either add the missing AddStringSlice("blocked", c.Blocked) calls in the registry (if blocking is intentionally supported for those handlers), or avoid embedding the shared struct on configs where only Allowed was previously supported.
### Non-blocking observations (2)
- Docstring generalization erases per-handler semantics —
add_labels's auto-label-creation side-effect andset_issue_type's empty-string clearing behavior are no longer documented at the struct site. - Generic
Allowedcomment — "Optional list of allowed values" conveys nothing about glob-pattern support, permissive-by-default posture, or dual title/ID matching.
These are maintainability debts introduced by this PR and should be addressed alongside the blocking fixes.
🔎 Code quality review by PR Code Quality Reviewer · 124.7 AIC · ⌖ 8.72 AIC · ⊞ 5.2K
Comment /review to run again
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Blocked field silently added and silently dropped at runtime: AssignMilestoneConfig never had a Blocked field before; this embedding introduces one, but the handler registry never forwards it — so any blocked: the user writes in YAML will parse successfully and then be completely ignored.
💡 Details and fix
safe_outputs_handler_registry.go (around line 226) builds the assign_milestone config map with:
return newHandlerConfigBuilder().
AddTemplatableInt("max", c.Max).
AddStringSlice("allowed", c.Allowed).
// ... but no AddStringSlice("blocked", c.Blocked)Before this PR AssignMilestoneConfig had no Blocked field, so this was never a problem. Now that SafeOutputAllowBlockConfig is embedded, the field exists in the struct and is YAML-parseable — a user writing:
assign-milestone:
blocked: ["v99"]...will see no error but the blocked constraint will never reach the runtime handler. This is a silent security policy bypass.
Fix options:
- Add
AddStringSlice("blocked", c.Blocked)to theassign_milestoneregistry entry if blocking milestones is a supported feature. - Or keep a standalone
Allowed []stringonAssignMilestoneConfiginstead of embedding the shared struct, untilblockedsupport is verified end-to-end.
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Blocked field silently added and silently dropped at runtime: SetIssueTypeConfig never had a Blocked field before; embedding SafeOutputAllowBlockConfig adds one, but the set_issue_type handler registry entry only emits allowed — blocked is parsed and then discarded.
💡 Details and fix
In safe_outputs_handler_registry.go the set_issue_type entry (around line 939) is:
config := newHandlerConfigBuilder().
AddTemplatableInt("max", c.Max).
AddStringSlice("allowed", c.Allowed).
// no AddStringSlice("blocked", c.Blocked)A user writing:
set-issue-type:
blocked: ["spam"]...will get no YAML parse error, but the blocked constraint is never forwarded to the runtime. The user believes they are blocking certain issue types; they are not.
Fix options:
- Add
AddStringSlice("blocked", c.Blocked)to theset_issue_typeregistry entry if blocking is intended. - Or use a standalone
Allowed []stringfield instead of the shared struct to keep the struct's public surface exactly as it was before.
| // SafeOutputAllowBlockConfig contains common allow/block lists for safe output configurations. | ||
| // Embed this in safe output config structs that support optional allowed/blocked value filters. | ||
| type SafeOutputAllowBlockConfig struct { | ||
| Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed values |
There was a problem hiding this comment.
Semantic docstrings lost in generalization: the shared Allowed comment "Optional list of allowed values" replaces precise per-handler documentation that carried important semantic distinctions.
💡 What was lost
| Handler | Original comment |
|---|---|
AddLabelsConfig |
"Labels will be created if they don't already exist in the repository. If omitted, any labels are allowed (including creating new ones)." |
RemoveLabelsConfig |
"Optional list of allowed label patterns to remove ... If omitted, any labels can be removed." |
SetIssueTypeConfig |
"Optional list of allowed issue type names. If omitted, any type is allowed (including clearing with "")." |
AssignToUserConfig |
"If omitted, any users are allowed." |
The generic comment doesn't convey: when the list is omitted vs. present, what the wildcard behavior is, whether labels are auto-created, or that the empty-string sentinel for clearing a type is allowed. This context should be preserved either by adding handler-specific doc on the embedding site or by enriching the shared struct comments.
| BaseSafeOutputConfig `yaml:",inline"` | ||
| SafeOutputTargetConfig `yaml:",inline"` | ||
| SafeOutputFilterConfig `yaml:",inline"` | ||
| SafeOutputAllowBlockConfig `yaml:",inline"` |
There was a problem hiding this comment.
Operator-critical side-effect stripped from documentation: the original Allowed comment noted that "Labels will be created if they don't already exist in the repository." — this is a write side-effect, not just a filter. The generic replacement says only "Optional list of allowed values" and conceals this completely.
💡 Why this matters
For add_labels, Allowed serves a dual role:
- Filter: only labels on this list may be applied.
- Creation whitelist: the handler will create labels in the repository if they don't already exist, trusting that any entry in
Allowedwas pre-approved.
A user adding a label pattern like team-backend to Allowed is implicitly authorizing the agent to create that label in the target repository. Without documentation at the struct definition site, this side-effect is invisible to anyone reading just the config struct. At minimum, the embedding struct or a comment at the field declaration site should note this behavior.
|
@copilot please run the Unresolved review threads (newest first):
Branch refresh: This branch is 4 commits behind
|
- SetIssueTypeConfig: revert to direct Allowed field; handler registry
only serializes allowed, so embedding SafeOutputAllowBlockConfig would
silently accept (and ignore) a blocked: YAML key
- AssignMilestoneConfig: same fix — keep direct Allowed field
- safe_outputs_integration_test.go: update AssignToUserConfig composite
literal to use SafeOutputAllowBlockConfig{} embedded struct syntax,
fixing the go vet / integration build error
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pelikhan
deleted the
copilot/deep-report-quick-win-extract-safeoutputallowblock
branch
Several safe-output handler configs were duplicating the same
allowed/blockedfields despite already following an embedded shared-config pattern for other common fields. This change extracts that pair into a single shared config and reuses it across the affected handlers without changing the external config shape.Shared allow/block config
SafeOutputAllowBlockConfiginpkg/workflow/safe_outputs_parser.goallowed []stringblocked []stringEmbedded in affected safe-output configs
AddLabelsConfigRemoveLabelsConfigAssignToUserConfigUnassignFromUserConfigSetIssueTypeConfigAssignMilestoneConfigListJobConfigto avoid leaving a second local copy behindTest fixture updates
Example