Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Registry-based refactor with a coverage guard test. Part of the safe-output parsing refactor cluster — batch review recommended.
|
|
Triage: category= Part of the B1-safeoutput-refactor batch (with #53841, #53840). Draft, no CI yet, no reviews. Undraft once ready and review together with the batch.
|
There was a problem hiding this comment.
Pull request overview
Refactors safe-output repository targeting into a registry, reducing duplicated tool dispatch logic.
Changes:
- Adds tool-specific repository-target accessors.
- Preserves centralized repo parameter generation.
- Adds registry coverage validation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_tools_repo_params.go |
Replaces the switch with an accessor registry. |
pkg/workflow/safe_outputs_tools_generation_test.go |
Verifies the registry’s supported tool set. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
![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 — one suggestion on test depth, otherwise approving.
📋 Key Themes & Highlights
Key Themes
- Test depth: The new coverage guard confirms the registry has the right keys but does not verify each accessor reads the correct config field. A copy-paste error at the accessor body level would go undetected. See the inline comment for a concrete suggestion.
Positive Highlights
- ✅ The registry-based approach is a textbook deep-module improvement: the interface (one map lookup) is simpler than the old O(n) switch, and the behaviour is identical.
- ✅ Consistent use of
*repoTargetConfigas a nil-able return value cleanly replaces the dualhasAllowedRepos/targetRepoSlugvariables, reducing the chance of divergence. - ✅ The coverage guard test is a good pattern; it just needs one more layer of assertion.
- ✅ The old nested
switch-within-switchforadd_labelsthroughset_issue_fieldis gone — a real readability win.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.3 AIC · ⌖ 9.98 AIC · ⊞ 7.8K
Comment /matt to run again
|
|
||
| func TestParseUpdateIssuesConfigWithWildcardTargetRepo(t *testing.T) { | ||
| compiler := &Compiler{} | ||
| outputMap := map[string]any{ |
There was a problem hiding this comment.
[/tdd] The coverage guard verifies that registry keys exist but not that each key maps to the correct SafeOutputsConfig field — a copy-paste error like "close_issue" → config.CloseDiscussions would silently pass.
💡 Suggested approach
Consider calling each accessor with a fully-populated SafeOutputsConfig and asserting the returned repoTargetConfig is non-nil and contains expected values. Spot-checking a few distinct config types (issue, discussion, PR, label) would catch misrouted accessors:
func TestRepoTargetAccessorsReturnCorrectField(t *testing.T) {
cfg := &SafeOutputsConfig{
CloseIssues: &SafeOutputTargetConfig{AllowedRepos: []string{"org/repo"}, TargetRepoSlug: "org/repo"},
CloseDiscussions: &SafeOutputTargetConfig{AllowedRepos: []string{"org/other"}, TargetRepoSlug: "org/other"},
}
got := repoTargetAccessors["close_issue"](cfg)
require.NotNil(t, got)
assert.Equal(t, []string{"org/repo"}, got.allowedRepos, "close_issue must read CloseIssues, not another field")
}@copilot please address this.
There was a problem hiding this comment.
Addressed in ec1120a. The metadata-driven test now creates a distinct configuration for every eligible handler and verifies each accessor returns that handler’s own AllowedRepos and TargetRepoSlug values.
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.
|
There was a problem hiding this comment.
Request changes
The refactor is moving in the right direction, but it still leaves repo-target support wired through a second handwritten registry. That creates an easy-to-miss failure mode where future repo-aware safe-output tools compile without exposing the repo schema parameter the runtime expects.
Blocking theme
repoTargetAccessorsis now another source of truth that must stay in sync with the real safe-output tool/handler set.- The added test only compares one hard-coded list to another, so it will not catch the more important regression: adding a new repo-target-capable tool/config path without registering it here.
That is exactly the sort of drift this refactor was supposed to remove, so I would not merge it until the accessor mapping is derived from existing metadata or at least validated against the real handler registry instead of a duplicated literal.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 19.9 AIC · ⌖ 6.93 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| type repoTargetAccessor func(*SafeOutputsConfig) *repoTargetConfig | ||
|
|
||
| var repoTargetAccessors = map[string]repoTargetAccessor{ |
There was a problem hiding this comment.
This registry refactor still leaves repo-target support dangerously manual: adding a new repo-aware safe-output tool now requires updating both the handler registry and this separate repoTargetAccessors map, and the new test only checks a hard-coded list rather than deriving it from the actual tool definitions. That means a newly added cross-repo tool can silently ship without a repo schema parameter even though the runtime supports target-repo/allowed-repos, which breaks agent behavior in exactly the kind of incremental feature addition this refactor is supposed to simplify.
💡 Why this matters and how to fix it
Right now the source of truth is split:
var repoTargetAccessors = map[string]repoTargetAccessor{ ... }and separately the enabled/tool registries live elsewhere. The test only proves the handwritten list matches another handwritten list today; it does not fail when someone adds a new repo-target-capable config type but forgets to register it here.
A safer design is to co-locate the accessor with the tool/handler metadata, or derive the accessor map from the same registry used to generate safe-output handlers/tools. Short of that, add a failing test that compares against the real repo-target-capable handler set instead of a duplicated slice literal.
There was a problem hiding this comment.
Addressed in ec1120a. Accessors are now derived from safeOutputHandlers and their config types; no independent tool-name list remains.
There was a problem hiding this comment.
Current head 94b6b00 derives accessors by iterating canonical safeOutputHandlers; eligibility is inferred from each handler’s config type and fields, so no separate repo-tool list remains. TestRepoTargetAccessorsMatchHandlerMetadata exercises every eligible handler’s routed AllowedRepos and TargetRepoSlug values.
There was a problem hiding this comment.
Ponytail review: over-engineering only.
net: -40 lines possible.
Generated by ✂️ Ponytail Reviewer for #53838 · auto · 35.6 AIC · ⌖ 4.38 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/safe_outputs_tools_repo_params.go:44
L44-213: yagni: 28 near-identical closures (if output := config.X; output != nil { return &repoTargetConfig{...} }) replacing a 149-line switch with a 189-line map — net +40 lines for the same duplication, just per-entry instead of per-case.
pkg/workflow/safe_outputs_tools_generation_test.go:10
L10-19: delete: hardcoded 28-tool slice duplicates the keys already listed in repoTargetAccessors. Assert len(repoTargetAccessors) > 0 and a couple of spot checks instead of re-listing all 28 tool names.
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in ec1120a. Repository-target accessors now derive from the canonical safe-output handler metadata, and the test verifies accessor routing for every eligible handler. |
|
@copilot Please address the latest maintainer-visible blockers on this PR, refresh the branch if needed, and then run the
|
…-target-extraction Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Verified the accessor mapping remains derived from |
|
@copilot Please address the remaining maintainer-visible blockers on this PR in one pass.
|
Current head |
|
@copilot The branch has author follow-up, but the blocking review thread is still unresolved.
|
PR Triage
Registry-based refactor for safe-output repo target accessors. Reduces duplication in schema switch.
|
Safe-output schema generation repeated
AllowedReposandTargetRepoSlugextraction across a large tool-name switch, making new tool support easy to implement inconsistently.Registry-based targeting
Coverage guard
Generated by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/32249206937> Generated by 👨🍳 PR Sous Chef · gpt54 · 23.7 AIC · ⌖ 8.26 AIC · ⊞ 9.3K · ◷