Normalize safe-output issue/PR constraints to required-title-prefix, add migration codemod, add required-labels#33350
Normalize safe-output issue/PR constraints to required-title-prefix, add migration codemod, add required-labels#33350Copilot wants to merge 16 commits into
required-title-prefix, add migration codemod, add required-labels#33350Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
require-title-prefix, add migration codemod, and enforce label gating
| */ | ||
| interface PushToPullRequestBranchConfig extends SafeOutputConfig { | ||
| target?: string; | ||
| "require-title-prefix"?: string; |
There was a problem hiding this comment.
@copilot Sorry this should be required-title-prefix not require-title-prefix, also everywhere else
Also close-pull-request and close-issue should suppor both required-title-prefix and required-labels
For push-to-pull-request-branch, labels should be renamed required-labels with codemod added.
Review all other labels: in safe outputs
There was a problem hiding this comment.
Addressed in 451fe72.
- Canonicalized to
required-title-prefix(keptrequire-title-prefixandtitle-prefixas deprecated compatibility aliases where applicable). close-issueandclose-pull-requestcontinue to supportrequired-title-prefix+required-labels(with tests covering required-label gating for close PR as well).- For
push-to-pull-request-branch, renamed canonicallabelstorequired-labels, added deprecatedlabelsalias support, updated handler emission/runtime consumption, and updated validation/error text. - Extended codemod to migrate:
title-prefix->required-title-prefix(for targeted handlers)push-to-pull-request-branch.labels->required-labels
- Updated schema/types/docs/tests accordingly and reviewed other safe-output
labels:usages to keep non-push label fields unchanged.
There was a problem hiding this comment.
@copilot There's no need to include require-title-prefix in the deprecation or codemod because it's literally being added in this PR
There was a problem hiding this comment.
Fixed in ad8d096.
I removed require-title-prefix from deprecated/compat handling and migration paths:
- removed deprecated
require-title-prefixkey from schema/type definitions - removed codemod special-casing for
require-title-prefix - removed parser fallback to
require-title-prefix(canonicalrequired-title-prefix, withtitle-prefixdeprecated alias retained)
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
…bels Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
dsyme
changed the title
require-title-prefix, add migration codemod, and enforce label gatingrequired-title-prefix, add migration codemod, add required-labels
dsyme
changed the title
required-title-prefix, add migration codemod, add required-labelsrequired-title-prefix, add migration codemod, add required-labels
There was a problem hiding this comment.
Pull request overview
This PR tightens safe-output constraint handling by normalizing title/label gating fields, enforcing label gating when title-prefix constraints are used, and adding migration support for deprecated field names.
Changes:
- Adds parsing/validation support for title-prefix aliases and required label gating.
- Updates push-to-PR-branch handler config generation/runtime consumption to use
required_labels. - Adds a codemod, tests, schema/docs updates, and regenerated workflow locks.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_validation.go |
Adds validation requiring labels with title-prefix constraints. |
pkg/workflow/safe_outputs_required_labels_validation_test.go |
Tests new title-prefix/label validation. |
pkg/workflow/safe_outputs_parser.go |
Adds deprecated title-prefix alias parsing. |
pkg/workflow/safe_outputs_deprecated_title_prefix_test.go |
Tests deprecated alias compatibility. |
pkg/workflow/push_to_pull_request_branch.go |
Parses canonical/deprecated push title and label constraint keys. |
pkg/workflow/push_to_pull_request_branch_warning_test.go |
Updates warning tests for required label/title fields. |
pkg/workflow/push_to_pull_request_branch_validation.go |
Updates unconstrained push warning logic/messages. |
pkg/workflow/push_to_pull_request_branch_test.go |
Updates push handler config tests for required labels. |
pkg/workflow/config_parsing_helpers_test.go |
Updates expression parsing expectations for required labels. |
pkg/workflow/compiler_validators.go |
Wires new label/title validation into compilation. |
pkg/workflow/compiler_safe_outputs_handlers.go |
Emits required_labels in handler config. |
pkg/workflow/close_entity_helpers.go |
Maps deprecated close-entity title-prefix to required title prefix. |
pkg/parser/schemas/main_workflow_schema.json |
Updates schema fields and deprecation metadata. |
pkg/cli/fix_codemods.go |
Registers the new codemod. |
pkg/cli/fix_codemods_test.go |
Updates codemod registry/order tests. |
pkg/cli/codemod_safe_output_require_title_prefix.go |
Adds migration logic for deprecated safe-output constraint keys. |
pkg/cli/codemod_safe_output_require_title_prefix_test.go |
Tests the new codemod. |
docs/src/content/docs/reference/safe-outputs.md |
Updates push-to-PR-branch examples. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Updates push-to-PR-branch reference docs. |
actions/setup/js/types/safe-outputs-config.d.ts |
Adds new push-to-PR-branch config fields. |
actions/setup/js/push_to_pull_request_branch.cjs |
Reads required_labels with deprecated fallback. |
.github/workflows/smoke-otel-backends.lock.yml |
Updates generated Datadog env reference. |
.github/workflows/semantic-function-refactor.md |
Adds label gating to close-issue safe output. |
.github/workflows/semantic-function-refactor.lock.yml |
Regenerates lock file with label gating. |
.github/workflows/dependabot-go-checker.md |
Adds label gating to close-issue safe output. |
.github/workflows/dependabot-go-checker.lock.yml |
Regenerates lock file with label gating. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 25/26 changed files
- Comments generated: 3
| push-to-pull-request-branch: | ||
| target-repo: "org/target-repo" | ||
| title-prefix: "[bot] " | ||
| required-title-prefix: "[bot] " |
There was a problem hiding this comment.
@copilot We have this wrong. We shouldn't require required-labels if required-title-prefix is present - we should instead simply support both required-title-prefix and required-labels as two separate, orthogonal hard constraints.
There was a problem hiding this comment.
Implemented in e1965c6. I removed the compile-time validation that required required-labels when required-title-prefix is present, so these are now orthogonal constraints for safe-output handlers.
| Description: "Renames deprecated constraint fields to required-title-prefix/required-labels for applicable safe-outputs handlers.", | ||
| IntroducedIn: "1.0.0", | ||
| Apply: func(content string, frontmatter map[string]any) (string, bool, error) { | ||
| handlersToRename := safeOutputsHandlersNeedingTitlePrefixMigration(frontmatter) |
| if inSafeOutputs && isDescendant(indent, safeOutputsIndent) && strings.HasSuffix(trimmed, ":") && !strings.HasPrefix(trimmed, "#") { | ||
| key := strings.TrimSuffix(trimmed, ":") | ||
| if handlersToRename[key] { | ||
| activeHandler = key | ||
| activeHandlerIndent = indent | ||
| } else { | ||
| activeHandler = "" | ||
| activeHandlerIndent = "" | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /improve-codebase-architecture based on the constraint normalization + codemod migration in this PR.
Key Themes
✅ Strong Points
- Excellent codemod test coverage: The codemod tests comprehensively cover positive/negative cases and edge cases (rename vs. no-rename scenarios)
- Thoughtful backward compatibility: The multi-stage fallback chain (
required-title-prefix→require-title-prefix→title-prefix) prevents breaking changes while guiding users to the canonical naming - Hard constraint enforcement: The new validation requiring labels when title-prefix is set prevents misconfiguration at compile time—excellent defensive design
- Seamless migration path: The automated codemod makes upgrades effortless for existing workflows
🔍 Areas for Improvement
1. Missing edge case tests — /tdd
- Validation logic lacks tests for
nilconfig pointers (comment on line 190) - Parser fallback chain not covered by dedicated tests (comment on line 80)
- JavaScript handler fallback (
config.required_labels ?? config.labels) not tested (comment on line 112) - Three-stage parsing fallback in
push_to_pull_request_branch.golacks test coverage (comment on line 133)
Rationale: While the implementation is correct, fallback logic and edge cases are common sources of regressions. Explicit tests provide confidence during future refactoring.
2. Structural inconsistency — /improve-codebase-architecture
PushToPullRequestBranchConfigdefinesRequiredLabels+Labelsas separate fields- Other handlers use
SafeOutputFilterConfigwhich already has these fields - This creates two different mental models for the same "title-prefix + labels constraint" pattern
Suggestion: Consider whether PushToPullRequestBranchConfig should embed SafeOutputFilterConfig to unify constraint handling across all safe-output handlers. This would reduce duplication and make the domain language more consistent.
Overall Verdict
The core implementation is solid and well-designed. The migration strategy is exemplary. The requested changes are minor improvements to test coverage and architectural consistency—not blockers, but they would strengthen the codebase.
Recommended next steps:
- Add the missing edge case tests (should take ~15-20 minutes)
- Consider the structural refactor as a follow-up (optional, but worthwhile)
Skills Applied
/tdd— Identified missing test coverage for fallback chains and edge cases/improve-codebase-architecture— Highlighted structural inconsistency in constraint field definitions
Great work on this PR! The naming normalization and validation enforcement are important improvements to the safe-outputs system. 🎯
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 7.5M
| // validateSafeOutputsRequiredLabelsWithTitlePrefix validates that title-prefix | ||
| // constraints are always paired with label constraints. | ||
| func validateSafeOutputsRequiredLabelsWithTitlePrefix(config *SafeOutputsConfig) error { | ||
| if config == nil { |
There was a problem hiding this comment.
[/tdd] The new validation function validateSafeOutputsRequiredLabelsWithTitlePrefix lacks tests for nil config pointers in the validation test file.
The validation logic correctly handles nil config, but there is no explicit test case verifying this behavior. Consider adding:
t.Run("nil config passes validation", func(t *testing.T) {
err := validateSafeOutputsRequiredLabelsWithTitlePrefix(nil)
require.NoError(t, err)
})This ensures the guard clause at the start works as expected and prevents future regressions if the logic changes.
| // Parse required-title-prefix (preferred) with fallback to deprecated title-prefix | ||
| config.RequiredTitlePrefix = extractStringFromMap(configMap, "required-title-prefix", safeOutputParserLog) | ||
| if config.RequiredTitlePrefix == "" { | ||
| config.RequiredTitlePrefix = extractStringFromMap(configMap, "title-prefix", safeOutputParserLog) |
There was a problem hiding this comment.
[/tdd] The parser fallback chain (required-title-prefix → title-prefix) lacks dedicated test coverage.
While the codemod tests verify the migration, there are no parser-level tests confirming:
required-title-prefixtakes precedence when both are presenttitle-prefixis used as fallback whenrequired-title-prefixis missing
Consider adding to safe_outputs_deprecated_title_prefix_test.go:
t.Run("parser prefers required-title-prefix over title-prefix", func(t *testing.T) {
config := ParseFilterConfig(map[string]any{
"required-title-prefix": "[new] ",
"title-prefix": "[old] ",
})
assert.Equal(t, "[new] ", config.RequiredTitlePrefix)
})| const target = config.target || "triggering"; | ||
| const titlePrefix = config.title_prefix || ""; | ||
| const envLabels = config.labels ? (Array.isArray(config.labels) ? config.labels : config.labels.split(",")).map(label => String(label).trim()).filter(label => label) : []; | ||
| const rawRequiredLabels = config.required_labels ?? config.labels; |
There was a problem hiding this comment.
[/tdd] The JavaScript handler fallback logic (config.required_labels ?? config.labels) lacks test coverage.
The .cjs file correctly implements the fallback, but there are no tests verifying:
required_labelstakes precedence when both are presentlabelsis used as fallback whenrequired_labelsis undefined
While this logic mirrors the Go implementation, JavaScript has different null/undefined semantics that warrant explicit testing. Consider adding tests to a new push_to_pull_request_branch.test.cjs file if one doesn't already exist.
| @@ -14,7 +14,8 @@ type PushToPullRequestBranchConfig struct { | |||
| BaseSafeOutputConfig `yaml:",inline"` | |||
| Target string `yaml:"target,omitempty"` // Target for push-to-pull-request-branch: like add-comment but for pull requests | |||
| TitlePrefix string `yaml:"title-prefix,omitempty"` // Required title prefix for pull request validation | |||
There was a problem hiding this comment.
[/improve-codebase-architecture] Inconsistent naming creates two mental models for the same concept.
The struct has both:
RequiredLabels(canonical, line 16)Labels(deprecated alias, line 17)
But other safe-output handlers use SafeOutputFilterConfig which already defines both fields. This PR introduces push-to-pull-request-branch.required-labels as the canonical name, but the struct doesn't embed SafeOutputFilterConfig.
Suggestion: Consider embedding SafeOutputFilterConfig here or extracting a shared constraint struct. This would unify the "title-prefix + labels constraint" pattern across all handlers and eliminate duplicate field definitions.
| if pushToBranchConfig.TitlePrefix == "" { | ||
| pushToBranchConfig.TitlePrefix = extractStringFromMap(configMap, "require-title-prefix", pushToPullRequestBranchLog) | ||
| } | ||
| if pushToBranchConfig.TitlePrefix == "" { |
There was a problem hiding this comment.
[/tdd] The three-stage fallback chain here (required-title-prefix → require-title-prefix → title-prefix) is not covered by tests.
The parsing logic correctly implements the precedence order, but tests only verify the canonical required-title-prefix case. Add test cases for:
t.Run("falls back to require-title-prefix when required-title-prefix missing", func(t *testing.T) {
config := parsePushToPullRequestBranchConfig(map[string]any{
"push-to-pull-request-branch": map[string]any{
"require-title-prefix": "[bot] ",
},
})
assert.Equal(t, "[bot] ", config.TitlePrefix)
})Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 72/100
Test Classification Details
Flagged Tests — Requires Review
|
| Test File | Production File | Test Lines | Prod Lines | Ratio | Status |
|---|---|---|---|---|---|
codemod_safe_output_require_title_prefix_test.go |
codemod_safe_output_require_title_prefix.go |
89 | 146 | 0.61:1 | ✅ Pass |
safe_outputs_deprecated_title_prefix_test.go |
safe_outputs_parser.go |
36 | 4 | 9.0:1 | ❌ Exceeds 2:1 |
safe_outputs_required_labels_validation_test.go |
safe_outputs_validation.go |
103 | 44 | 2.34:1 | ❌ Exceeds 2:1 |
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected.
Score breakdown:
- Behavioral Coverage (40 pts): 40/40 — All tests verify behavioral contracts ✅
- Edge Case Coverage (30 pts): 12/30 — 40% of tests include error/edge cases
⚠️ - Low Duplication (20 pts): 20/20 — No duplicate test patterns ✅
- Proportional Growth (10 pts): 0/10 — 2 files exceed 2:1 inflation ratio ❌
Strengths:
- 🎯 100% behavioral coverage — all tests verify what the system does, not how
- ✅ Proper build tags on all new Go test files
- ✅ No forbidden mock libraries (gomock/testify mock)
- ✅ Good use of table-driven patterns (2 out of 5 tests)
- ✅ Tests cover critical features: codemods, validation, backward compatibility
Recommendations for improvement:
- Add descriptive assertion messages to the 4 flagged assertions for better failure diagnostics
- Increase edge case coverage — 3 of 5 tests only cover happy paths; consider adding error scenarios
- Review test inflation — 2 files exceed the 2:1 ratio (though both tests verify important behavioral contracts)
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
Test Inflation: When test files grow disproportionately faster than production code (>2:1 ratio), it may indicate verbose test setup, redundant test cases, or opportunities for helper functions.
References:
🧪 Test quality analysis by Test Quality Sentinel · ● 8.6M · ◷
There was a problem hiding this comment.
✅ Test Quality Sentinel: 72/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected.
All 5 new/modified tests verify behavioral contracts. See detailed analysis in comment above.
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
@copilot apply codemod to current repo (gh aw fix --write). |
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>
Done in I ran |
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in I investigated the linked job logs, reproduced the failure locally (
|
|
@copilot merge main, recompile, fix conflicts |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in Merged
|
|
@copilot the codemod is too aggressive at changing the workflows. Revert the changes in agentic workflows and review the codemod to be more selective |
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>
Fixed in I reverted the agentic workflow markdown/lock changes back to
|
|
/review |
|
|
This change standardizes safe-output constraint syntax on
require-title-prefix(withtitle-prefixkept as deprecated input for compatibility) and ensures title-prefix-gated operations also require label gating. It also adds an upgrade codemod so existing workflows migrate automatically.Constraint key normalization + compatibility
require-title-prefixas the canonical key for constraint-style title filtering.title-prefixas a deprecated alias where it previously meant a constraint, and normalize it at parse time.title-prefix(e.g. create issue/PR title decoration) is unchanged.Upgrade/codemod migration
title-prefixtorequire-title-prefixfor relevant handlers:close-issueclose-discussionclose-pull-requestmark-pull-request-as-ready-for-reviewpush-to-pull-request-branchHard-constraint enforcement
required-title-prefixrequiresrequired-labelson close/mark handlers.require-title-prefixrequireslabelsonpush-to-pull-request-branch.Schema/docs/types updates
require-title-prefixfor constraint usage on push-to-PR-branch.Representative migration
pr-sous-chef: request to update branch (run id: 26122348307)