Refactor action pin warnings and agentdrain helpers - #51674
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR is a mechanical file split (actionpins.go and agentdrain files broken into focused files) plus consolidation of duplicated warning-dedup logic into a single emitOnce helper. No new abstractions, no speculative flexibility, no unnecessary dependencies were introduced — the diff nets roughly +29 lines total, mostly package boilerplate for new files. Nothing found to cut.
|
There was a problem hiding this comment.
Pull request overview
Refactors action pin resolution into cohesive files, centralizes warning deduplication, and colocates agentdrain helpers with related functionality.
Changes:
- Split
pkg/actionpinsby responsibility while preserving APIs. - Added centralized
PinContext.emitOncewarning deduplication. - Reorganized agentdrain event and template helpers.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/actionpins.go |
Removed monolithic implementation. |
pkg/actionpins/types.go |
Houses public types and interfaces. |
pkg/actionpins/data.go |
Handles embedded pin loading and lookup. |
pkg/actionpins/references.go |
Contains reference parsing and formatting. |
pkg/actionpins/resolve.go |
Contains pin-resolution logic. |
pkg/actionpins/mappings.go |
Handles action and container mappings. |
pkg/actionpins/warnings.go |
Centralizes deduplicated message emission. |
pkg/actionpins/actionpins_internal_test.go |
Tests warning initialization and deduplication. |
pkg/agentdrain/event.go |
Groups event formatting and sequencing helpers. |
pkg/agentdrain/mask.go |
Retains masking-specific functionality. |
pkg/agentdrain/cluster.go |
Colocates template parameter extraction. |
pkg/agentdrain/miner.go |
Removes relocated template helper. |
pkg/agentdrain/coordinator.go |
Removes relocated event sequencing helper. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 13/13 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.
Clean, correct refactoring. The monolithic actionpins.go is split into focused files (types.go, data.go, references.go, resolve.go, mappings.go, warnings.go) with all public APIs preserved. agentdrain helpers are reorganised similarly. One intentional behaviour change: invalid container-pin mapping warnings are now deduplicated via emitOnce, matching the existing deduplication for valid-mapping notifications. The new test TestApplyContainerPinMapping_DeduplicatesInvalidWarnings correctly covers this. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.1 AIC · ⌖ 9.95 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
NotesBoth tests cover deduplication invariants — verifying that a warning is emitted to stderr exactly once per unique key. This replaces the older
Minor: Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — 3 minor observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing
t.Parallel(): Two new tests omitt.Parallel(), breaking the file's consistent pattern and slowing the test suite. - Empty slice allocation:
extractParamsuses[]string{}wherevar params []stringis idiomatic and avoids a heap allocation on the zero-value path.
Positive Highlights
- ✅ Clean split of a 595-line monolith into focused, well-named files (
types.go,data.go,references.go,resolve.go,mappings.go,warnings.go) - ✅
PinContext.emitOnceis a neat encapsulation — eliminates 4 separateif !ctx.Warnings[key]/fmt.Fprintln/ctx.Warnings[key] = trueblocks - ✅ Invalid container mapping warning is now deduplicated (was emitted on every call in old code) — correct behavior improvement, well-tested
- ✅
agentdrainhelpers (FlattenEvent,Tokenize,StageSequence) move toevent.goalongside their natural home;extractParamslands correctly incluster.go - ✅ Public APIs unchanged — pure internal reorganization
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 86 AIC · ⌖ 7.64 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| } | ||
|
|
||
| func TestApplyContainerPinMapping_DeduplicatesInvalidWarnings(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Missing t.Parallel() — same pattern break as TestPinContextEmitOnce_InitializesAndDeduplicates; the new dedup test should also be marked parallel to match the file's established convention.
💡 Suggested fix
func TestApplyContainerPinMapping_DeduplicatesInvalidWarnings(t *testing.T) {
t.Parallel()
ctx := &PinContext{@copilot please address this.
| params := []string{} | ||
| for i, tok := range template { | ||
| if tok == paramToken && i < len(tokens) { | ||
| params = append(params, tokens[i]) |
There was a problem hiding this comment.
[/codebase-design] params := []string{} allocates an empty non-nil slice even when nothing matches. Idiomatic Go uses var params []string to defer allocation, which also aligns with the pattern used elsewhere in the package.
💡 Suggested fix
var params []string
for i, tok := range template {Callers using len(params) == 0 are unaffected; the only observable difference is the avoided heap allocation.
@copilot please address this.
|
|
||
| initWarnings(ctx) | ||
| func TestPinContextEmitOnce_InitializesAndDeduplicates(t *testing.T) { | ||
| ctx := &PinContext{} |
There was a problem hiding this comment.
[/tdd] Missing t.Parallel() — all sibling tests in this file call t.Parallel() at the top of the function body, but this new test omits it, slowing the suite and breaking the established convention.
💡 Suggested fix
func TestPinContextEmitOnce_InitializesAndDeduplicates(t *testing.T) {
t.Parallel()
ctx := &PinContext{}@copilot please address this.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (697 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
Great work on this refactoring! 🎉 Your PR cleanly consolidates the duplicated warn-once blocks in The changes to This PR demonstrates the kind of incremental, targeted refactoring that keeps the codebase maintainable. The ADR documentation is a nice touch for future maintainers. Ready for review! 👍
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, review the maintainer-facing feedback below, and run the Open review context (newest first):
Branch refresh was requested.
|
PR TriageCategory: refactor · Risk: medium · Priority: medium · Score: 62/100 Score breakdown
Recommended action: Automated triage via PR Triage Agent.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing feedback below, and run the Open review context (newest first):
Branch refresh was requested.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing feedback below, and run the Open review context (newest first):
Branch refresh was requested.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the maintainer-facing feedback below, and run the Open review context (newest first):
Branch refresh was requested.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in cefdd5f.
Local validation completed with |
pkg/actionpinscombined unrelated pin-data, resolution, mapping, and warning concerns in one file, while repeating warning deduplication.pkg/agentdrainhad event/template helpers separated from their related functionality.Action pin structure
actionpins.gointo focused types, data, references, resolution, mapping, and warning files.Warning deduplication
PinContext.emitOnce.event.go.cluster.go.Run: https://github.com/github/gh-aw/actions/runs/31375792022> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.8 AIC · ⌖ 5.46 AIC · ⊞ 8.5K · ◷