Unify create-* close-older fields via shared CloseOlderConfig embed#54656
CloseOlderConfig embed#54656Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
CloseOlderConfig embed
There was a problem hiding this comment.
Pull request overview
Consolidates close-older settings across create-issue, discussion, and pull-request configurations while preserving existing YAML keys.
Changes:
- Adds shared
CloseOlderConfigwith parse-time aliases. - Updates handlers, permission checks, and tests to use the shared config.
- Refreshes the generated network-audit workflow lock file.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/create_entity_helpers.go |
Defines shared config and alias helper. |
pkg/workflow/create_issue.go |
Embeds and parses shared issue settings. |
pkg/workflow/create_discussion.go |
Embeds and parses shared discussion settings. |
pkg/workflow/create_pull_request.go |
Embeds shared PR settings and updates permission logic. |
pkg/workflow/safe_outputs_handler_registry.go |
Reads unified close-older fields. |
pkg/workflow/create_close_older_config_test.go |
Tests legacy-key aliasing for all entities. |
pkg/workflow/compiler_safe_outputs_config_test.go |
Updates discussion test fixtures. |
pkg/workflow/compiler_safe_outputs_config_handlers_test.go |
Updates handler configuration tests. |
.github/workflows/weekly-network-domains-audit.lock.yml |
Refreshes generated installation and timeout steps. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #54656; no actionable blocking issues found in changed lines, so no review comments were needed beyond submitting the overall review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: PR #54656 diff reviewed for over-engineering only. The CloseOlderConfig consolidation is a legitimate DRY refactor removing duplicated Enabled/Key fields across three config structs; the setCloseOlderEnabledAlias helper is a minimal 4-line single-purpose function used 3x for backward-compat aliasing, not speculative. No dead code, unneeded deps, or speculative abstractions found. Lean already. Ship.
|
PR TriageCategory: refactor · Risk: medium · Score: 55/100
Recommended action: Consolidates duplicated close-older config fields into a shared embed with backward-compatible aliasing. Reasonable maintainability improvement but touches 3 safe-output handlers (create-issue/discussion/pull-request) — worth a careful human review before merge. CI mostly green with 2 failures; no reviews yet.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking issues in the changed lines.
Scope reviewed
I checked the shared close-older config refactor across parsing, handler serialization, and the new parser coverage. The aliasing keeps the external keys stable, and I did not find a changed-line regression that would justify blocking this PR.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 34.4 AIC · ⌖ 6.84 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Clean, well-scoped refactor. The CloseOlderConfig embed with setCloseOlderEnabledAlias correctly handles backward-compatible aliasing from entity-specific keys (close-older-issues, close-older-discussions, close-older-pull-requests) to the canonical close-older-enabled field before YAML unmarshaling. Tests cover all three entity parsers. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20 AIC · ⌖ 8.97 AIC · ⊞ 6.2K
ADR Required — Draft CreatedStatus: Draft ADR committed to this branch at This PR adds 114 lines of new business logic in What to do next
Why ADRs matterArchitecture Decision Records create a permanent, searchable record of why a decision was made — not just what was done. Future contributors reading ADR sections required (Michael Nygard template)
Important This PR should not merge until the ADR status is changed from
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with minor improvements requested. The refactor is clean and well-structured.
📋 Key Themes & Highlights
Key Themes
- Leaky internal key:
close-older-enabledis tagged as "internal canonical key" in comments but is a real public YAML key due to the inline embed — users can write it directly. Either document it or prevent it. - Orphaned source key in BoolFields: both the original entity-specific key (e.g.
close-older-issues) and the new alias (close-older-enabled) are listed inBoolFields, causing redundant coercion. The source key should either be cleaned up after aliasing or the duplication explained. - Handler-local env key naming:
Enabledis unified at the struct level but mapped to three different downstream names (close_older_issues,close_older_discussions,close_older_pull_requests). Worth a clarifying comment. - Missing edge-case tests: the new tests cover the happy path but not
Enabled == nil(absent) orEnabled == "false"(explicitly disabled).
Positive Highlights
- ✅ Excellent backward-compatibility design via parse-time aliasing — existing YAML is unaffected
- ✅
setCloseOlderEnabledAliasis a clean, single-purpose helper — easy to test and reason about - ✅ Three focused parser tests with clear Arrange/Act/Assert structure
- ✅
isCloseOlderPullRequestsEnabledupdated consistently with the consolidated field
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.1 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
| HandleExpires bool | ||
| } | ||
|
|
||
| // CloseOlderConfig holds shared close-older settings across create entity handlers. |
There was a problem hiding this comment.
[/codebase-design] close-older-enabled is described as "Internal canonical key" but is a valid public YAML key via the inline embed — any user who writes close-older-enabled: true in their workflow YAML will have it accepted silently.
💡 Options to address the leaky seam
The yaml:"close-older-enabled,omitempty" tag on an inline-embedded field means the key is always accepted in YAML, regardless of intent. Options:
- Document it as a supported alias — drop "Internal" from the comment, add it to schema docs.
- Validate and reject it — in the preprocess hook, detect
close-older-enabledand emit a validation error pointing users to the entity-specific key. - Use the
yaml:"-"tag and populate programmatically — keep the field unexported to YAML, set it only through the alias function.
Option 3 is the cleanest for a truly internal field, but requires a small struct-tag change.
@copilot please address this.
| Enabled *string `yaml:"close-older-enabled,omitempty"` // Internal canonical key; populated from entity-specific close-older-* keys before unmarshaling. | ||
| Key string `yaml:"close-older-key,omitempty"` // Optional explicit deduplication key for close-older matching. When set, uses gh-aw-close-key marker instead of workflow-id markers. | ||
| } | ||
|
|
There was a problem hiding this comment.
[/codebase-design] setCloseOlderEnabledAlias silently skips when configData is nil, but never removes the source key after copying — if downstream YAML unmarshaling checks for close-older-issues as a bool field and close-older-enabled as a bool field, the same boolean coercion runs twice on the same value.
💡 Why this matters
BoolFields in CreateParseOptions lists both close-older-issues and close-older-enabled. The preprocess step coerces both to string form. Since setCloseOlderEnabledAlias copies the value without deleting the original, the coercion over close-older-issues is redundant — the field no longer exists on the struct, so it is harmless today, but a future handler that re-adds a field named close-older-issues would have it populated unexpectedly.
Consider either:
- Deleting the source key after aliasing:
delete(configData, sourceKey) - Or adding a comment explaining why the source key stays (no struct field → harmless orphan).
@copilot please address this.
| @@ -72,8 +72,8 @@ var handlerRegistry = map[string]handlerBuilder{ | |||
| AddStringSlice("assignees", c.Assignees). | |||
| AddIfNotEmpty("target-repo", c.TargetRepoSlug). | |||
| AddTemplatableBool("group", c.Group). | |||
There was a problem hiding this comment.
[/codebase-design] In safe_outputs_handler_registry.go, the discussion handler passes c.CloseOlderConfig.Enabled as the close_older_discussions env key, the issue handler as close_older_issues, and the PR handler as close_older_pull_requests — three different downstream keys from one shared Enabled field.
💡 Semantic coupling concern
The unified CloseOlderConfig.Enabled field carries a single boolean intent ("close older entities"), but it is mapped to three different downstream environment variable names by the callers. This means the shared field's semantic meaning is actually split across call sites — a subtlety that could confuse future contributors who see Enabled and assume the downstream key is also unified.
A short comment at each mapping site (e.g. // entity-specific env key name per AWF handler contract) would make this explicit, or alternatively document it on the CloseOlderConfig type itself.
@copilot please address this.
|
|
||
| func TestParseCreateIssuesConfigMapsCloseOlderConfig(t *testing.T) { | ||
| compiler := NewCompiler(WithFailFast(true)) | ||
| config := compiler.parseCreateIssuesConfig(map[string]any{ |
There was a problem hiding this comment.
[/tdd] The new tests in create_close_older_config_test.go cover the happy path (alias maps to Enabled/Key) but do not test the absence case: when neither close-older-issues nor close-older-enabled is set, config.CloseOlderConfig.Enabled should be nil.
💡 Suggested additional test cases
func TestParseCreateIssuesConfigNoCloseOlderWhenAbsent(t *testing.T) {
compiler := NewCompiler(WithFailFast(true))
config := compiler.parseCreateIssuesConfig(map[string]any{
"create-issue": map[string]any{},
})
require.NotNil(t, config)
assert.Nil(t, config.CloseOlderConfig.Enabled, "Enabled should be nil when not set")
assert.Empty(t, config.CloseOlderConfig.Key)
}Also worth testing: close-older-issues: false maps to Enabled = strPtr("false") (not nil), which is what isCloseOlderPullRequestsEnabled depends on to correctly return false.
@copilot please address this.
|
@copilot Please address the current forward-progress items on this branch, then run the Open review feedback:
Open failing checks:
Please review the latest feedback, make any needed follow-up changes, rerun the exact validations needed after the fix, and summarize any remaining human follow-up. Run: https://github.com/github/gh-aw/actions/runs/32543132845
|
… add absence/false-case tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…e review Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review follow-ups in commits b1525ba and 5e9aab4:
Validation: Remaining human follow-up: none identified for this PR's code changes. The ADR status is still |
PR Triage
|
|
@-
|
|
@copilot this PR looks close, but it still needs a maintainer-ready finish.
Run: https://github.com/github/gh-aw/actions/runs/32569036201
|
Confirmed HEAD (8989476) is already up to date with
The 4 open review threads from Maintainer handoff:
No code changes were needed this pass since the substantive fixes already landed. Nothing further actionable from this sandbox. |
|
@copilot sous-chef triage: please review this PR, refresh the branch if needed, address any unresolved review feedback, and use the pr-finisher skill before handing back for maintainer review.
|
No changes since my last pass on this PR: HEAD (8989476) is still up to date with |
PR TriageCategory: Unifies close-older config fields across create-* safe outputs. CI green (2 success), APPROVED. Low urgency. Automated triage — run 32572524009
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
create-issue,create-discussion, andcreate-pull-requesteach re-declared close-older enable/key fields independently, while downstream handler config consumed them in parallel paths. This change consolidates those fields into a single shared config embed and keeps existing YAML keys backward-compatible through parse-time aliasing.Config model consolidation
CloseOlderConfiginpkg/workflow/create_entity_helpers.go:Enabled *stringKey stringCloseOlderConfiginto:CreateIssuesConfigCreateDiscussionsConfigCreatePullRequestsConfigBackward-compatible parsing
close-older-issues→ sharedEnabledclose-older-discussions→ sharedEnabledclose-older-pull-requests→ sharedEnabledclose-older-keybehavior via sharedKey.Consumer unification
c.CloseOlderConfig.Enabledc.CloseOlderConfig.KeyTest updates
Run: https://github.com/github/gh-aw/actions/runs/32558278813> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.36 AIC · ⌖ 8.18 AIC · ⊞ 9.5K · ◷
Branch refresh requested by PR Sous Chef.
Run: https://github.com/github/gh-aw/actions/runs/32569036201> Generated by 👨🍳 PR Sous Chef · gpt54 · 28.6 AIC · ⌖ 8.36 AIC · ⊞ 9.5K · ◷