Replace any bool-or-expression fields with *TemplatableBool in workflow step and safe-outputs config - #51097
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
any bool-or-expression fields with *TemplatableBool in workflow step and safe-outputs config
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (65 additions detected). |
|
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. |
|
No new test functions were added or modified in this PR. This is a refactoring PR that updates existing test fixtures and assertions to work with the new TemplatableBool type signature. The PR modified 4 existing test files (compiler_safe_outputs_config_test.go, notify_comment_test.go, safe_outputs_import_test.go, step_types_test.go) but did not add any new behavioral tests. Since no new test coverage was introduced, Test Quality Sentinel skips scoring and reports no action needed. |
|
|
There was a problem hiding this comment.
Pull request overview
Unifies bool-or-expression fields under TemplatableBool, updating parsing, serialization, and conclusion-policy generation.
Changes:
- Types
ContinueOnErrorandReportFailureAsIssue. - Preserves boolean, expression, and category-filter behavior.
- Updates tests and regenerated workflow output.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/step_types.go |
Types and converts step error handling. |
pkg/workflow/step_types_test.go |
Updates step conversion tests. |
pkg/workflow/safe_outputs_import_test.go |
Updates imported-setting assertions. |
pkg/workflow/safe_outputs_config_types.go |
Types the reporting setting. |
pkg/workflow/safe_outputs_config_global.go |
Parses typed reporting values. |
pkg/workflow/notify_comment_test.go |
Tests reporting environment generation. |
pkg/workflow/notify_comment_conclusion_helpers.go |
Removes runtime type assertions. |
pkg/workflow/compiler_safe_outputs_config_test.go |
Updates configuration parsing tests. |
.github/workflows/smoke-cursor.lock.yml |
Synchronizes generated workflow output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review: Replace any with *TemplatableBool
The PR correctly unifies ContinueOnError and ReportFailureAsIssue fields under *TemplatableBool. The logic is sound:
- Nil checks before
.String()calls are properly guarded (nil check at line 339 ensures theelsebranch is only reached with a non-nil pointer) - The
buildAgentFailureReportingPolicyVarsswitch correctly initializesshouldIncludeCategoryFilters = trueand only sets it tofalsein thefalse/expression cases - Category filters are preserved correctly for the
truecase - The
compareStepValuesnil-pointer handling for*TemplatableBoolis correct
One minor observation: step_types.go now silently drops non-expression, non-bool strings for continue-on-error (the old code preserved them). This is likely intentional as part of the type narrowing.
No blocking issues found. ✅
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 56.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — overall a clean, well-scoped refactor. Requesting 4 targeted improvements before merge.
📋 Key Themes & Highlights
Key Themes
- Silent discard risk:
MapToStepsilently drops unrecognisedcontinue-on-errorstring values with no error or diagnostic - Implicit default fragility: The
"true"branch in conclusion-policy env generation relies on an implicit default rather than an explicit assignment - Test documentation gap: The meaningful behaviour change (string
"false"→ native boolfalsein compiled output) is tested but not named or documented as an invariant - Minor DRY opportunity:
strconv.FormatBool+TemplatableBool(...)construction is now scattered across two files; a sharedNewTemplatableBoolFromBoolhelper would centralise it
Positive Highlights
- ✅ Eliminates 18-line
any-type switch in conclusion-policy generation — the new.String()-based switch is dramatically more readable - ✅ Bidirectional serialisation (bool ↔ string ↔ expression) is correctly handled in both
ToMapandMapToStep - ✅ Tests updated throughout;
templatableBoolPtrhelper keeps test boilerplate low - ✅ Category-array behaviour preserved cleanly via separate fields; the updated comment on
ReportFailureAsIssueaccurately documents the new contract
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 52.2 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/step_types.go:146
[/codebase-design] Silent discard for unrecognised continue-on-error string values — if a user writes e.g. "yes" or "1", step.ContinueOnError stays nil and the field is silently dropped from compiled output with no diagnostic.
<details>
<summary>💡 Suggested fix</summary>
Add an else branch that surfaces the unrecognised value as an error:
case string:
if value == "true" || value == "false" || isExpression(value) {
templatableValue := TemplatableBool(value)
…
</details>
<details><summary>pkg/workflow/notify_comment_conclusion_helpers.go:348</summary>
**[/codebase-design]** The `"true"` case no longer sets `shouldIncludeCategoryFilters = true` explicitly — it relies on the default value declared two lines up. This is correct today but fragile: if the default changes or a future reviewer adds an early `shouldIncludeCategoryFilters = false` branch before the switch, category filters will silently stop being emitted for the `true` case.
<details>
<summary>💡 Suggested fix</summary>
Make the intent explicit:
```go
case "true":
appendRepor…
</details>
<details><summary>pkg/workflow/step_types_test.go:297</summary>
**[/tdd]** Good catch — this test now asserts `"false"` round-trips as a boolean `false` (not as the string `"false"`). But the previous test at line 290–295 keeps `ContinueOnError: templatableBoolPtr("false")` with the expected map containing `"continue-on-error": false`, while the diff shows the old expected value was `"false"` (a string). This is a meaningful behaviour change; it would benefit from a dedicated test name that documents the invariant: `TestToMap_ContinueOnErrorLiteralEmitsNati…
</details>
<details><summary>pkg/workflow/safe_outputs_config_global.go:180</summary>
**[/codebase-design]** `strconv.FormatBool` used here for a `bool` case, but `TemplatableBool` is a `string` typedef — so the three parsing branches (`bool`, `string`, and implicitly nil) each independently construct `TemplatableBool`. Consider extracting a small helper `newTemplatableBool(v bool) *TemplatableBool` (or `TemplatableBoolFromBool`) to keep construction consistent across all call sites and avoid the inline `strconv` import.
<details>
<summary>💡 Suggested helper</summary>
```go
f…
</details>|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please triage this PR, refresh the branch if possible, and run the Unresolved review threads to re-check:
Run context: https://github.com/github/gh-aw/actions/runs/31204567664
|
Triage ResultCategory: refactor · Risk: medium · Priority: medium (score 58/100 — impact 22, urgency 12, quality 24) Notes: Type-safety refactor (any → *TemplatableBool) across 10 files, +79/-56. AI reviewer approved (github-actions[bot]); some reviewer comments outstanding. CI shows all jobs skipped/cancelled — needs a fresh CI run before merge. Already labeled
|
|
🎉 This pull request is included in a new release. Release: |
This change removes two remaining
any-typed bool-or-expression fields and aligns them with the existingTemplatableBoolmodel. It also eliminates ad-hoc runtime type switching in conclusion env generation by using typed accessors.Type unification
WorkflowStep.ContinueOnErrorchanged fromany→*TemplatableBool.SafeOutputsConfig.ReportFailureAsIssuechanged fromany→*TemplatableBool.report-failure-as-issueremains represented viaReportFailureAsIssueCategories/ReportFailureAsIssueExcludedCategories.Parsing + serialization updates
continue-on-errorbool/string-expression inputs into*TemplatableBool.true/falseas booleans and expressions as strings.report-failure-as-issueas*TemplatableBoolwhile preserving include/exclude category extraction.Conclusion policy env handling cleanup
bool|string|[]anybranching withTemplatableBool-based evaluation forGH_AW_FAILURE_REPORT_AS_ISSUE.