Refactor safe-outputs config parsing into focused workflow modules#44313
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Status: DRAFT — needs undraft + CI before review. Splits Run §28966928999
|
🤖 PR Triage
Score breakdown: Impact 18 + Urgency 12 + Quality 10 Rationale: DRAFT. Refactors safe-outputs config parsing into focused modules — no behaviour change expected. Medium risk due to size (1282 add / 1254 del). Defer until undraft + CI. Batch: similar refactor/chore drafts Run §28986426320
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Refactors the safe-outputs frontmatter/config extraction code by splitting the former monolithic safe_outputs_config.go into smaller, responsibility-focused modules, while also consolidating repeated bounded-integer parsing logic into shared helpers (with direct unit test coverage).
Changes:
- Split safe-outputs config parsing/types/runtime serialization into dedicated files to reduce mixed responsibilities.
- Introduced
parseBoundedIntField/parseBoundedIntFieldOrDefaultand reused them for global bounded integer fields. - Added a focused unit test covering truncation/clamping/invalid handling for the bounded-int helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_config.go | Deleted the former monolithic safe-outputs config implementation. |
| pkg/workflow/safe_outputs_config_types.go | Introduces shared safe-outputs config types and the module logger. |
| pkg/workflow/safe_outputs_config_extraction.go | Hosts the top-level extractSafeOutputsConfig flow and delegates global field parsing. |
| pkg/workflow/safe_outputs_config_global.go | Implements global safe-outputs field parsing and new bounded-int helpers. |
| pkg/workflow/safe_outputs_config_base.go | Centralizes shared per-handler “base” parsing helpers (max/token/app/staged/samples). |
| pkg/workflow/safe_outputs_config_runtime.go | Contains runtime handler-manager config assembly + serialization helpers. |
| pkg/workflow/safe_outputs_config_global_test.go | Adds unit coverage for the new bounded-int parsing helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Low
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| configMap := map[string]any{} | ||
| if tt.name != "missing field" { | ||
| configMap["field"] = tt.input | ||
| } | ||
|
|
||
| got, ok := parseBoundedIntField(configMap, "field", safeOutputsConfigLog) | ||
| if ok != tt.wantOK { | ||
| t.Fatalf("parseBoundedIntField() ok = %v, want %v", ok, tt.wantOK) | ||
| } | ||
| if got != tt.want { | ||
| t.Fatalf("parseBoundedIntField() = %d, want %d", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
| // Handle jobs (safe-jobs must be under safe-outputs) | ||
| if jobs, exists := outputMap["jobs"]; exists { | ||
| if jobsMap, ok := jobs.(map[string]any); ok { | ||
| c := NewCompiler() // Create a temporary compiler instance for parsing | ||
| config.Jobs = c.parseSafeJobsConfig(jobsMap) | ||
| } | ||
| } |
🧪 Test Quality Sentinel Report❌ Test Quality Score: 100/100 — Excellent (fails due to guideline violation)
📊 Metrics (1 test)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 100/100 (numeric) but FAILS. Hard violation: safe_outputs_config_global_test.go is missing the mandatory //go:build !integration tag on line 1. All other unit test files in pkg/workflow/ carry this tag. Review flagged tests in the comment above.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,282 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
|
There was a problem hiding this comment.
Review: Refactor safe-outputs config parsing into focused workflow modules
This is a clean, well-executed refactor. The split by responsibility is logical, behavior is preserved (and improved in a few edge cases), and the new parseBoundedIntField helper eliminates real duplication.
Correctness improvements detected (net positive)
Old max-patch-size float handler had no NaN/Inf guard
The deleted code for max-patch-size (unlike the max-patch-files code that came later) did not guard against float64(NaN) or float64(Inf) before calling int(v). In Go, int(NaN) and int(Inf) are implementation-defined and typically produce 0, which then silently fell through to the default of 4096. The new parseBoundedIntField explicitly rejects these via math.IsNaN / math.IsInf, making the behavior deterministic and logged.
Old max-patch-size int64/uint64 paths lacked overflow clamping
The old int64 and uint64 branches for max-patch-size did int(v) directly without an overflow check — unsafe on 32-bit platforms. The new parseBoundedIntField clamps to math.MaxInt, matching the more defensive handling that was already in the max-patch-files and timeout-minutes branches.
One non-blocking suggestion
See the inline comment on safe_outputs_config_global_test.go line 33: the tt.name != "missing field" test guard is a fragile name-coupling smell. Not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 119.4 AIC · ⌖ 6.12 AIC · ⊞ 4.8K
|
|
||
| configMap := map[string]any{} | ||
| if tt.name != "missing field" { | ||
| configMap["field"] = tt.input |
There was a problem hiding this comment.
The test gate tt.name != "missing field" couples setup logic to a magic string — if the case name ever changes, the field is silently inserted with a nil value instead of being absent, giving a false pass for the wrong reason.
Prefer an explicit absent bool field in the test struct:
tests := []struct {
name string
absent bool // when true, don't insert field into configMap
input any
want int
wantOK bool
}{
{name: "missing field", absent: true, want: 0, wantOK: false},
// ...
}
// ...
if !tt.absent {
configMap["field"] = tt.input
}This makes the "key absent" contract explicit and survives future renames.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on test coverage gaps and one structural concern.
Key Themes
Issues (4 actionable)
int64branch never tested —parseBoundedIntFieldhas 4 switch cases; onlyint,uint64, andfloat64are exercised. Theint64clamp path is untested (comments 1).- Fragile
missing fieldsentinel —if tt.name != "missing field"couples test logic to the row name; a simple boolean field is safer (comment 2). NewCompiler()shadows the receiver —extractGlobalConfigFieldsis a*Compilermethod, but internally creates a fresh compiler forparseSafeJobsConfig, silently discardingforceStaged,useSamples, and the engine registry. Even if intentional (and it was in the original), the refactor makes this invisible to readers (comment 3).- No
defaultbranch / no log for unsupported types — a quoted YAML integer (max-patch-size: "1024") silently falls back to 4096 with no diagnostic (comment 4).
Minor (non-blocking)
parseBoundedIntFieldOrDefaulthas no direct test (comment 5).- Schema Generation Architecture block is misplaced in the extraction file (comment 6).
Positive Highlights
- The split boundaries are clean and well-reasoned: types / extraction / global / base / runtime.
- Deduplication of
parseBoundedIntFieldinto a single helper is the right call. - The test covers NaN, Inf, truncation, and clamping — good baseline.
- Module boundary doc in
safe_outputs_config_types.gois a nice touch for navigability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 137.7 AIC · ⌖ 7.35 AIC · ⊞ 6.6K
Comment /matt to run again
| {name: "float truncate", input: 12.75, want: 12, wantOK: true}, | ||
| {name: "float nan", input: math.NaN(), want: 0, wantOK: false}, | ||
| {name: "float inf", input: math.Inf(1), want: 0, wantOK: false}, | ||
| } |
There was a problem hiding this comment.
[/tdd] The int64 switch case in parseBoundedIntField has no test coverage — all three branches (normal, clamp, non-positive) are untested.
Add at least three rows to the table:
{name: "int64 normal", input: int64(42), want: 42, wantOK: true},
{name: "int64 clamp", input: int64(math.MaxInt64), want: math.MaxInt, wantOK: true},
{name: "int64 non-positive", input: int64(-1), want: 0, wantOK: false},The int64 path has its own bounds check (v > int64(math.MaxInt)) that mirrors uint64 but is never exercised, leaving a silent regression risk.
@copilot please address this.
| t.Parallel() | ||
|
|
||
| configMap := map[string]any{} | ||
| if tt.name != "missing field" { |
There was a problem hiding this comment.
[/tdd] Controlling the "missing field" case via tt.name is fragile — if the test name changes, the field-injection guard silently breaks.
Replace the name-based sentinel with a dedicated boolean field on the test struct:
tests := []struct {
name string
input any
omitFromMap bool // true => do not set the key in configMap
want int
wantOK bool
}{
{name: "missing field", omitFromMap: true, want: 0, wantOK: false},
...
}
// in the loop:
if !tt.omitFromMap {
configMap["field"] = tt.input
}This makes the intent explicit and decouples the test logic from the name string.
@copilot please address this.
| // Handle jobs (safe-jobs must be under safe-outputs) | ||
| if jobs, exists := outputMap["jobs"]; exists { | ||
| if jobsMap, ok := jobs.(map[string]any); ok { | ||
| c := NewCompiler() // Create a temporary compiler instance for parsing |
There was a problem hiding this comment.
[/codebase-design] c := NewCompiler() inside extractGlobalConfigFields (which is a method on *Compiler) shadows the receiver and discards all caller-configured state (engine registry, forceStaged, useSamples, etc.).
This pattern was copied from the original file, but moving it into extractGlobalConfigFields makes the hazard invisible — callers see a *Compiler receiver and reasonably expect it is used throughout. Either call c.parseSafeJobsConfig(jobsMap) directly with the receiver, or document explicitly why a fresh compiler is intentional here.
@copilot please address this.
| // parseBoundedIntField parses a positive integer field from a heterogeneous YAML map. | ||
| // It accepts int, int64, uint64, and float64 values, clamps integer overflow to math.MaxInt, | ||
| // logs float truncation, and rejects non-positive, NaN, infinite, or otherwise invalid values. | ||
| func parseBoundedIntField(configMap map[string]any, key string, debugLog *logger.Logger) (int, bool) { |
There was a problem hiding this comment.
[/tdd] parseBoundedIntField silently ignores unrecognised types (e.g. string, bool) — the switch falls through to return 0, false with no log. When a workflow author writes max-patch-size: "1024" (a quoted string, which YAML happily passes), they get the 4 096 default with no diagnostic, making the mistake hard to debug.
Add a default branch that logs the unexpected type:
default:
debugLog.Printf("%s: unsupported type %T, ignoring", key, raw)Also add a test case for this path:
{name: "string input", input: "1024", want: 0, wantOK: false},@copilot please address this.
| return 0, false | ||
| } | ||
|
|
||
| func parseBoundedIntFieldOrDefault(configMap map[string]any, key string, defaultValue int, debugLog *logger.Logger) int { |
There was a problem hiding this comment.
[/tdd] parseBoundedIntFieldOrDefault is not tested directly. Its behaviour when given a value of 0 (non-positive → falls through to default) and when given a valid value that overrides the default is implicit. Add a small focused test, or extend TestParseBoundedIntField with a wrapper:
func TestParseBoundedIntFieldOrDefault(t *testing.T) {
m := map[string]any{"k": 10}
if got := parseBoundedIntFieldOrDefault(m, "k", 99, safeOutputsConfigLog); got != 10 {
t.Fatalf("want 10, got %d", got)
}
m2 := map[string]any{}
if got := parseBoundedIntFieldOrDefault(m2, "k", 99, safeOutputsConfigLog); got != 99 {
t.Fatalf("want default 99, got %d", got)
}
}@copilot please address this.
| // Safe Output Configuration Extraction | ||
| // ======================================== | ||
| // | ||
| // ## Schema Generation Architecture |
There was a problem hiding this comment.
[/grill-with-docs] The 37-line Schema Generation Architecture comment block lives at the top of the extraction module but describes schema generation and validation — work that belongs to safe_outputs_config_generation.go and the test files, not to the extraction flow. This creates a navigation mismatch: a reader opening safe_outputs_config_extraction.go to understand frontmatter extraction sees a lengthy detour about JSON schemas.
Consider moving this documentation into a doc.go comment or into pkg/workflow/README.md (which already exists and covers the workflow package), keeping the file header focused on extraction.
@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
The refactor is structurally sound and the parseBoundedIntField consolidation is a net improvement over three divergent ad-hoc switch blocks. The extraction flow, runtime config, and types split all read cleanly.
🔎 Findings
Medium — parseBoundedIntFieldOrDefault has no test coverage
The new parseBoundedIntFieldOrDefault wrapper is the critical integration point for max-patch-size (default 4096) and max-patch-files (default 100). Its contract — return defaultVal when the field is absent or the raw value is invalid — is untested. The timeout-minutes caller intentionally passes defaultVal=0 to mean "no default"; if that subtle contract is ever broken, compiled workflows silently get a wrong timeout. See inline comment on line 143 of safe_outputs_config_global.go.
Medium — int64 branch untested in TestParseBoundedIntField
The test suite skips the int64 case entirely; the at-min, below-min, and overflow-clamp sub-branches for that type are unverified. See inline comment on the test file.
Low — DispatchRepository yaml struct tag changed from underscore to hyphen
The struct tag on SafeOutputsConfig.DispatchRepository changed from dispatch_repository to dispatch-repository. This is harmless if SafeOutputsConfig is never marshaled to YAML (it's only read via parseDispatchRepositoryConfig), but that assumption is worth an explicit confirmation grep before merge. See inline comment on safe_outputs_config_types.go line 81.
🔎 Code quality review by PR Code Quality Reviewer · 238.4 AIC · ⌖ 5.89 AIC · ⊞ 5.4K
Comment /review to run again
| // Handle group-reports flag | ||
| if groupReports, exists := outputMap["group-reports"]; exists { | ||
| if groupReportsBool, ok := groupReports.(bool); ok { | ||
| config.GroupReports = groupReportsBool |
There was a problem hiding this comment.
parseBoundedIntFieldOrDefault is untested and its zero-default behavior is invisible: No test exercises parseBoundedIntFieldOrDefault directly — only parseBoundedIntField is tested. The function's contract (return defaultValue when field is absent or invalid) is load-bearing for max-patch-size and max-patch-files, and the timeout-minutes case deliberately passes defaultValue=0 to express "no default" — a subtlety that could silently regress.
💡 Suggested fix
Add at minimum:
func TestParseBoundedIntFieldOrDefault(t *testing.T) {
t.Parallel()
// absent → returns default
got := parseBoundedIntFieldOrDefault(map[string]any{}, "k", 4096, safeOutputsConfigLog)
if got != 4096 { t.Fatalf("want 4096, got %d", got) }
// present valid → overrides default
got = parseBoundedIntFieldOrDefault(map[string]any{"k": 10}, "k", 4096, safeOutputsConfigLog)
if got != 10 { t.Fatalf("want 10, got %d", got) }
// invalid (zero) → returns default
got = parseBoundedIntFieldOrDefault(map[string]any{"k": 0}, "k", 4096, safeOutputsConfigLog)
if got != 4096 { t.Fatalf("want 4096, got %d", got) }
// zero-default pattern (timeout-minutes semantics)
got = parseBoundedIntFieldOrDefault(map[string]any{}, "k", 0, safeOutputsConfigLog)
if got != 0 { t.Fatalf("want 0, got %d", got) }
}| SetIssueType *SetIssueTypeConfig `yaml:"set-issue-type,omitempty"` // Set the type of an issue (empty string clears the type) | ||
| SetIssueField *SetIssueFieldConfig `yaml:"set-issue-field,omitempty"` // Set a single issue field value by name/value | ||
| DispatchWorkflow *DispatchWorkflowConfig `yaml:"dispatch-workflow,omitempty"` // Dispatch workflow_dispatch events to other workflows | ||
| DispatchRepository *DispatchRepositoryConfig `yaml:"dispatch-repository,omitempty"` // Dispatch repository_dispatch events to external repositories; the underscore alias remains supported via parseDispatchRepositoryConfig. |
There was a problem hiding this comment.
DispatchRepository yaml struct tag changed from dispatch_repository to dispatch-repository: The old struct tag used an underscore (yaml:"dispatch_repository,omitempty"); the new one uses a hyphen (yaml:"dispatch-repository,omitempty"). Any code path that round-trips SafeOutputsConfig through YAML marshal/unmarshal (e.g., serialization in tests, config snapshots, or any external tool consuming compiled output) will silently change the emitted key name and fail to re-read data written by the old code.
💡 Detail
The comment on line 81 says "the underscore alias remains supported via parseDispatchRepositoryConfig" — which handles reading the YAML key — but the struct tag governs what is emitted when marshaling SafeOutputsConfig to YAML. Any consumer of marshaled SafeOutputsConfig data (snapshots, cached configs, test fixtures) that expects dispatch_repository will break silently after this change.
If SafeOutputsConfig is never marshaled to YAML (only parsed from user input), this is safe. If it is marshaled anywhere, the tag must stay dispatch_repository or migration must be done explicitly.
Verify with: grep -rn "dispatch_repository" pkg/ --include="*.go" to confirm no marshal path exists before merging.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test cases for int64 type are missing: The test covers int, uint64, and float64 but skips int64 entirely. The int64 branch has distinct logic (comparison against int64(1), overflow clamping to math.MaxInt) that could regress independently.
💡 Suggested additions
{name: "int64 above min", input: int64(100), want: 100, wantOK: true},
{name: "int64 at min", input: int64(1), want: 1, wantOK: true},
{name: "int64 below min", input: int64(0), want: 0, wantOK: false},
{name: "int64 clamp", input: int64(math.MaxInt64), want: math.MaxInt, wantOK: true},Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher flow and pushed follow-up fixes in |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/safe_outputs_config.gohad grown into a 1,254-line mixed-responsibility file centered around a ~760-line extraction function. This change breaks that logic into smaller modules, keeps behavior intact, and removes duplicated bounded-integer parsing for global safe-outputs settings.File split by responsibility
safe_outputs_config_types.go: shared config types, logger, module boundary docssafe_outputs_config_extraction.go: top-level frontmatter extraction flowsafe_outputs_config_global.go: global safe-outputs field parsingsafe_outputs_config_base.go: shared per-handler base parsing helperssafe_outputs_config_runtime.go: runtime handler-manager config assembly and serializationsafe_outputs_config.goGlobal field parsing cleanup
parseBoundedIntField/parseBoundedIntFieldOrDefaultmax-patch-sizemax-patch-filestimeout-minutesBehavior-preserving extraction refactor
extractSafeOutputsConfigas the single entrypointFocused direct coverage
Example of the new shared parsing path: