Introduce typed enums for workflow fixed-value fields

[Copilot]

Several workflow fields with closed value sets were represented as bare strings, weakening compile-time type safety.

• Safe outputs and runner topology

  • Add SafeOutputsURLsPolicy and RunnerTopology.
  • Type SafeOutputsConfig.URLs and RunnerConfig.Topology.

• Reactions

  • Add ReactionType constants for all supported GitHub reactions.
  • Type WorkflowData.AIReaction and the validation map.

• MCP script parameters

  • Add MCPParamType constants for JSON-schema input types.
  • Type MCPScriptParam.Type.

• Regression coverage

  • Add field-type assertions to prevent these APIs from reverting to bare string.

type RunnerTopology string

const RunnerTopologyArcDind RunnerTopology = "arc-dind"

type RunnerConfig struct {
    Topology RunnerTopology `json:"topology,omitempty" yaml:"topology,omitempty"`
}

• Fixes [deep-report] Type 4 implicit string enums (SafeOutputsURLsPolicy, ReactionType, MCPParamType, RunnerTopology) #54532

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✂️ Ponytail Reviewer has started processing this pull request

[github-actions]

❌ Design Decision Gate 🏗️ failed during design decision gate check.

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection engine failed to produce results.

Review the workflow run logs for details.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

Comment Memory

reviewed_at: 2026-08-22T00:00:00Z
review_event: REQUEST_CHANGES
top_themes:
  - partial enum migration leaves string-literal comparisons in hot paths
  - regression coverage checks types, not end-to-end enum behavior
files_reviewed:
  - pkg/workflow/awf_config.go
  - pkg/workflow/central_slash_command_workflow.go
  - pkg/workflow/frontmatter_types.go
  - pkg/workflow/mcp_scripts_parser.go
  - pkg/workflow/reactions.go
  - pkg/workflow/runner_config.go
  - pkg/workflow/safe_outputs_config_global.go
  - pkg/workflow/safe_outputs_validation.go
  - pkg/workflow/trigger_parser.go
  - pkg/workflow/workflow_data.go
comment_count: 0

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 15.4 AIC · ⌖ 6.88 AIC · ⊞ 4.6K · ◷
_{Comment /review to run again}

[github-actions]

Request changes

This refactor mostly swaps bare strings for typed aliases, but it still leaves several stringly-typed comparison and formatting paths untouched, so the type-safety win is partial rather than end-to-end.

Blocking themes

• The new typed fields are still consumed through raw string literals in a few hot paths, which weakens the point of introducing closed enums and makes future invalid-value regressions easier to reintroduce.
• The regression coverage only asserts reflected field types, not the behavioral boundaries that matter when invalid enum values flow through parsing and compilation.

🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 15.4 AIC · ⌖ 6.88 AIC · ⊞ 4.6K
_{Comment /review to run again}

[Copilot]

Pull request overview

Introduces named string types for fixed-value workflow fields, improving type safety while preserving parsing and serialization behavior.

Changes:

• Adds typed enums for URL policies, reactions, MCP parameter types, and runner topology.
• Updates parsing and serialization boundaries for the new types.
• Adds regression assertions and updates affected tests.

Show a summary per file

File	Description
pkg/workflow/workflow_data.go	Types the AI reaction field.
pkg/workflow/trigger_parser.go	Converts validated reactions to ReactionType.
pkg/workflow/safe_outputs_validation.go	Defines typed URL-policy constants.
pkg/workflow/safe_outputs_config_types.go	Types the safe-output URL policy field.
pkg/workflow/safe_outputs_config_global.go	Converts parsed URL policies.
pkg/workflow/runner_config.go	Converts parsed runner topology.
pkg/workflow/runner_config_test.go	Updates topology assertions.
pkg/workflow/reactions.go	Defines reaction constants and typed validation keys.
pkg/workflow/notify_comment_test.go	Updates reaction fixtures.
pkg/workflow/mcp_scripts_parser.go	Defines MCP parameter types and constants.
pkg/workflow/label_command_test.go	Updates default-reaction assertion.
pkg/workflow/implicit_string_enums_test.go	Guards field types against regression.
pkg/workflow/frontmatter_types.go	Defines and applies RunnerTopology.
pkg/workflow/compiler_safe_outputs_test.go	Types expected reaction values.
pkg/workflow/compiler_reactions_numeric_test.go	Types numeric-reaction expectations.
pkg/workflow/compiler_activation_jobs_test.go	Updates reaction fixtures.
pkg/workflow/central_slash_command_workflow.go	Converts reactions at string output boundaries.
pkg/workflow/awf_config.go	Propagates typed topology internally.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 18/18 changed files
• Comments generated: 0
• Review effort level: Balanced

[github-actions]

Good refactor — consistently replacing string fields with typed string enums (ReactionType, RunnerTopology, MCPParamType, SafeOutputsURLsPolicy) improves discoverability, IDE autocomplete, and catches misuse at compile time. The new TestImplicitStringEnumFieldTypes test is a nice structural guard.

Two non-blocking issues worth addressing:

1. Parsing boundary leak (reactions.go): parseReactionValue, parseReactionConfig, and intToReactionString still return plain string. Updating these to return ReactionType would remove the ReactionType(...) cast in trigger_parser.go and complete the type-safety chain through the entire parse path.

2. No runtime validation for new enum types (runner_config.go): YAML deserialization silently accepts arbitrary strings into typed fields. ReactionType already has isValidReaction guarding it, but RunnerTopology and SafeOutputsURLsPolicy have no equivalent runtime check. Consider adding validation at the parse site.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 45.7 AIC · ⌖ 9.28 AIC · ⊞ 6.2K

Comments that could not be inline-anchored

pkg/workflow/reactions.go:56

The parseReactionValue, parseReactionConfig, and intToReactionString functions still return string rather than ReactionType. This means callers must cast back to ReactionType at every call site (e.g., trigger_parser.go line 826 does ReactionType(reactionStr)), partially defeating the purpose of the typed enum — invalid string values can still flow through unchecked until the cast.

Consider changing the return type to ReactionType (and (ReactionType, error) for the error-ret…

pkg/workflow/runner_config.go:21

The RunnerConfig.Topology field is now typed as RunnerTopology, but the struct tag yaml:"topology,omitempty" means YAML will deserialize arbitrary strings into the field without validation. An unknown topology value like "bogus-topology" will silently be accepted into a RunnerTopology-typed field.

Since RunnerTopologyArcDind is the only defined constant, consider adding validation after YAML unmarshal (or in extractRunnerConfig) to reject unknown topology values and surface a cle…

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design and /tdd — requesting changes on a few consistency and validation gaps.

📋 Key Themes & Highlights

Key Themes

• Incomplete enum propagation (reactions.go): isValidReaction, parseReactionValue, and parseReactionConfig still operate on string at their boundaries. The new ReactionType doesn't flow end-to-end — callers must scatter ReactionType(...) casts rather than getting compile-time safety at the parse boundary.
• Unchecked cast for MCPParamType (mcp_scripts_parser.go): Unlike reactions (which run through isValidReaction), any unknown YAML type string is silently accepted as a MCPParamType constant. Adding a small validation map would close this gap.
• Regression test guards types, not values (implicit_string_enums_test.go): A value-level assertion table would make the test actually catch the original bug (#54532) rather than only catching a future type regression.
• Inconsistent enum placement: MCPParamType and SafeOutputsURLsPolicy live in their respective domain files while RunnerTopology and ReactionType are in frontmatter_types.go — minor discoverability issue.

Positive Highlights

• ✅ Clean, systematic rollout: all changed files are consistently updated
• ✅ validReactions map upgraded to map[ReactionType]bool — the validation path benefits from the enum
• ✅ New implicit_string_enums_test.go is a smart regression guard
• ✅ Test files updated to use named constants rather than bare strings throughout

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 78.8 AIC · ⌖ 10.4 AIC · ⊞ 7.6K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/workflow/reactions.go:40

[/codebase-design] isValidReaction still takes a bare string even though ReactionType now exists — the type system can't protect against passing arbitrary strings here.

<details>
<summary>💡 Suggested change</summary>

Change the signature to:

func isValidReaction(reaction ReactionType) bool {
	return validReactions[reaction]
}

Update the one call-site in trigger_parser.go to cast before calling: isValidReaction(ReactionType(reactionStr)). This consolidates the string→…

pkg/workflow/reactions.go:56

[/codebase-design] parseReactionValue and parseReactionConfig still return string, so callers immediately lose the typed enum — callers must cast back to ReactionType manually (e.g. trigger_parser.go:416).

<details>
<summary>💡 Suggested change</summary>

Return ReactionType from both functions, removing the internal string(...) casts:

func parseReactionValue(value any) (ReactionType, error) { ... }
func parseReactionConfig(value any) (ReactionType, *bool, *bool, *bool…

</details>

<details><summary>pkg/workflow/mcp_scripts_parser.go:120</summary>

**[/codebase-design]** `MCPParamType(tStr)` is an unchecked cast — any YAML value (e.g. `&quot;integer&quot;`, `&quot;null&quot;`) silently becomes an apparently-valid `MCPParamType` with no validation error.

&lt;details&gt;
&lt;summary&gt;💡 Suggested change&lt;/summary&gt;

Add a validation map (mirroring `validReactions`) and check before assigning:

```go
var validMCPParamTypes = map[MCPParamType]bool{
    MCPParamTypeString: true, MCPParamTypeNumber: true,
    MCPParamTypeBoolean: true, MCPParamTypeArray: true, MCPParamTypeOb…

</details>

<details><summary>pkg/workflow/runner_config_test.go:88</summary>

**[/tdd]** `RunnerConfig{Topology: &quot;arc-dind&quot;}` uses a bare string literal — if the constant value ever changes the test will still compile and pass green while the code is broken.

&lt;details&gt;
&lt;summary&gt;💡 Suggested change&lt;/summary&gt;

```go
wd := &amp;WorkflowData{
    RunnerConfig: &amp;RunnerConfig{Topology: RunnerTopologyArcDind},
}

Using the constant makes the test specification-level: it asserts that getRunnerTopology returns the known topology constant, not a specific raw string.

</details>
…

pkg/workflow/mcp_scripts_parser.go:55

[/codebase-design] MCPParamType and SafeOutputsURLsPolicy are defined in their respective parser/validation files, while RunnerTopology and ReactionType are in frontmatter_types.go. Splitting enum declarations across files makes them harder to discover.

<details>
<summary>💡 Suggested change</summary>

Move MCPParamType (and its constants) to frontmatter_types.go alongside the other new enums, or — if the parser file is intentional because it owns that domain — at least add a…

pkg/workflow/implicit_string_enums_test.go:13

[/tdd] The regression test guards field types but not values — a constant could be renamed to an empty string or a typo and the type assertion would still pass.

<details>
<summary>💡 Suggested change</summary>

Add a second table that asserts the string values of the key constants:

func TestImplicitStringEnumValues(t *testing.T) {
    assert.Equal(t, "eyes", string(ReactionTypeEyes))
    assert.Equal(t, "+1", string(ReactionTypePlusOne))
    assert.Equal(t, "arc-dind", string(Ru…

</details>

[github-actions]

🛠️ Agentic Maintenance updated this pull request branch.

View workflow run

[github-actions]

🛠️ Agentic Maintenance updated this pull request branch.

View workflow run

[gh-aw-bot]

@copilot this PR still needs forward progress before maintainer review.

There is open blocking review feedback on this PR. Please address the remaining review items, refresh the branch if needed, rerun the relevant validations, and finish with the pr-finisher skill.

Run: https://github.com/github/gh-aw/actions/runs/32578113366

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 20.3 AIC · ⌖ 8.22 AIC · ⊞ 9.5K · ◷
_{Comment /souschef to run again}