Deduplicate declarative constraint builders in tool_description_enhancer.go - #52157
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed the refactor of tool_description_enhancer.go: this is genuine deduplication, not over-engineering. buildConstraints[T] and the three field-gate helpers (appendTargetConstraint, appendTargetRepoSlugConstraint, appendRequiredTitlePrefixConstraint) each collapse 5-13 duplicated call sites, net -155 lines, no new speculative abstractions or unused flexibility introduced. Nothing to cut.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
There was a problem hiding this comment.
Pull request overview
Refactors tool-description constraint builders to remove duplicated control flow while preserving behavior.
Changes:
- Adds a generic constraint-building helper.
- Extracts common target, repository, and title-prefix constraints.
- Migrates all per-tool builders to shared helpers.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tool_description_enhancer.go |
Deduplicates constraint-builder boilerplate and common field gates. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
![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 /codebase-design and /tdd — commenting with two design refinements and one test coverage gap.
📋 Key Themes & Highlights
Key Themes
- Duplicate helpers:
appendTargetRepoSlugConstraintandappendRequiredTitlePrefixConstraintare byte-for-byte identical; they should be oneappendStringConstraint. - Incomplete helper adoption:
linkSubIssueConstraintsstill uses raw*constraints = append(...)forParentTitlePrefix/SubTitlePrefixinstead of the new helper. - Missing regression test: the
addCommentConstraintsnil-config special case (trailing constraint always appended) has no test coverage.
Positive Highlights
- ✅
buildConstraints[T]is a clean, generic nil-guard that eliminates ~33 boilerplate blocks — the core abstraction is well-designed. - ✅ PR description is clear and the before/after example makes the pattern immediately legible.
- ✅ All declarative differences are preserved exactly; the refactor is structurally sound.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.6 AIC · ⌖ 7.22 AIC · ⊞ 7K
Comment /matt to run again
| if prefix != "" { | ||
| *constraints = append(*constraints, fmt.Sprintf(format, prefix)) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/codebase-design] appendTargetRepoSlugConstraint and appendRequiredTitlePrefixConstraint have identical implementations — both guard on a non-empty string and call fmt.Sprintf(format, value). The semantic distinction lives in the caller's format string, not the helper itself, so two functions add naming complexity without adding behaviour.
💡 Suggested consolidation
Collapse both (and potentially appendTargetConstraint) into one helper:
// appendStringConstraint appends a formatted constraint when value is non-empty.
func appendStringConstraint(constraints *[]string, value, format string) {
if value != "" {
*constraints = append(*constraints, fmt.Sprintf(format, value))
}
}All call sites stay self-descriptive via their format argument. appendTargetConstraint can then become a thin wrapper or be inlined.
@copilot please address this.
There was a problem hiding this comment.
Consolidated in 1e5bd53. appendTargetRepoSlugConstraint and appendRequiredTitlePrefixConstraint are replaced by a single appendStringConstraint(constraints *[]string, value, format string); appendTargetConstraint is now a thin wrapper over it for the common "Target: %s." message. Every non-empty-string field gate in the file (18 sites) now routes through it, so the file is down to 550 lines.
| *constraints = append(*constraints, fmt.Sprintf("The parent issue title must start with %q.", config.ParentTitlePrefix)) | ||
| } | ||
| if config.SubTitlePrefix != "" { | ||
| *constraints = append(*constraints, fmt.Sprintf("The sub-issue title must start with %q.", config.SubTitlePrefix)) |
There was a problem hiding this comment.
[/codebase-design] linkSubIssueConstraints uses raw *constraints = append(...) for ParentTitlePrefix and SubTitlePrefix instead of the new appendRequiredTitlePrefixConstraint helper. This inconsistency means a future reader has to track two styles for the same pattern.
💡 Suggested fix
appendRequiredTitlePrefixConstraint(constraints, config.ParentTitlePrefix, "The parent issue title must start with %q.")
appendRequiredTitlePrefixConstraint(constraints, config.SubTitlePrefix, "The sub-issue title must start with %q.")@copilot please address this.
There was a problem hiding this comment.
Fixed in 1e5bd53. linkSubIssueConstraints now uses the shared helper (appendStringConstraint) for both ParentTitlePrefix and SubTitlePrefix, and the same conversion was applied to every other remaining hand-rolled non-empty-string gate in the file so there is a single style throughout.
| if config.TargetRepoSlug != "" { | ||
| constraints = append(constraints, fmt.Sprintf("Comments will be added in repository %q.", config.TargetRepoSlug)) | ||
| } | ||
| constraints := buildConstraints(config, func(config *AddCommentsConfig, constraints *[]string) { |
There was a problem hiding this comment.
[/tdd] addCommentConstraints is the only builder that has a non-nil-guard code path (it always appends a trailing constraint even when config == nil). The refactor preserves that behaviour, but there are no tests asserting it — a future cleanup could accidentally drop the trailing "Supports reply_to_id for discussion threading." line.
💡 Suggested test
func TestAddCommentConstraintsNilConfig(t *testing.T) {
got := addCommentConstraints(nil)
require.Equal(t, []string{"Supports reply_to_id for discussion threading."}, got)
}This makes the special-case contract explicit and guards against regression.
@copilot please address this.
There was a problem hiding this comment.
Added TestAddCommentConstraintsNilConfig in 1e5bd53 (pkg/workflow/tool_description_enhancer_test.go), asserting addCommentConstraints(nil) returns exactly ["Supports reply_to_id for discussion threading."]. ADR-52157 also notes the test pins this contract.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.2 AIC · ⌖ 6.21 AIC · ⊞ 5.4K
| return constraints | ||
| } | ||
|
|
||
| // appendTargetConstraint appends the common "Target: <value>." constraint when target is set. |
There was a problem hiding this comment.
Minor: appendTargetRepoSlugConstraint and appendRequiredTitlePrefixConstraint are functionally identical. Both guard on a non-empty string and call fmt.Sprintf(format, value). Consider consolidating into a single appendStringConstraint(constraints *[]string, value, format string) helper to avoid duplication of the same one-liner. The distinct names are readable, but if more single-string-guarded helpers are added in the future they would also each need their own function. This is a minor suggestion, not a blocker.
There was a problem hiding this comment.
Done in 1e5bd53 — both were collapsed into a single appendStringConstraint(constraints *[]string, value, format string) helper, with appendTargetConstraint kept as a thin wrapper. All other single-string-guarded appends in the file now use it too, so future additions won't need new per-field helpers.
|
@copilot This PR looks ready for a finishing pass. Please refresh the branch context, address the latest reviewer feedback, update the branch state if needed, and run the Open items:
Run context: https://github.com/github/gh-aw/actions/runs/31545290203
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 Triage: PR #52157
|
tool_description_enhancer.gohad ~33 per-tool constraint builder functions, each repeating the same skeleton: nil guard, slice setup, a handful of field gates, and a return. The control flow was duplicated across the whole file, with only strings and field selectors differing between builders.Shared helpers
buildConstraints[T any](config *T, build func(*T, *[]string)) []stringthat centralizes the nil-check + slice-setup boilerplate previously repeated in every builder.appendTargetConstraint,appendTargetRepoSlugConstraint, andappendRequiredTitlePrefixConstraintfor the three most common field-gate patterns (appeared 12, 13, and 5 times respectively).Builder refactor
*Constraintsfunction (createIssueConstraints,closeDiscussionConstraints,updatePullRequestConstraints, etc.) to use the new helpers instead of hand-rolled nil checks and repeatedfmt.Sprintfcalls for common fields.addCommentConstraintsalways appending a trailing constraint) exactly as before — no observable behavior change.Example of the pattern change:
Net effect: the file shrinks from 740 to 585 lines while the per-tool declarative logic remains fully readable and easy to extend for new tools.