refactor: extract helpers from buildHandlerManagerStep in compiler_safe_outputs_steps.go#49971
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…fe_outputs_steps.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Testing safe outputs |
There was a problem hiding this comment.
Pull request overview
Refactors safe-output handler step generation into focused helpers while preserving behavior.
Changes:
- Extracts app-token, core-env, and token-env helpers.
- Adds direct helper tests.
- Updates an unrelated workflow skill inventory entry.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_safe_outputs_steps.go |
Decomposes handler-step generation. |
pkg/workflow/compiler_safe_outputs_steps_test.go |
Adds helper-focused tests. |
.github/skills/agentic-workflows/SKILL.md |
Adds an unrelated reference entry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| // addAppTokenMintingSteps adds per-handler and dispatch-repository GitHub App token minting | ||
| // steps that must precede the handler manager step. For each registered handler that has a | ||
| // per-handler github-app configured, a dedicated token step is minted whose permissions are | ||
| // scoped to only that handler's needs (principle of least privilege). | ||
| func (c *Compiler) addAppTokenMintingSteps(data *WorkflowData) []string { |
| // TestAddAppTokenMintingSteps tests GitHub App token minting step generation | ||
| func TestAddAppTokenMintingSteps(t *testing.T) { |
| - `.github/aw/test-coverage.md` | ||
| - `.github/aw/test-expression.md` | ||
| - `.github/aw/token-optimization-caching-budgets.md` | ||
| - `.github/aw/token-optimization-observability.md` |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor — helper extraction is correct, *[]string pointer receivers are consistent across env-var helpers, value return is appropriate for the token-minting helper, and the new unit tests give direct coverage of each extracted function. No functional or behavioral changes detected.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.1 AIC · ⌖ 7.32 AIC · ⊞ 5.4K
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 82/100 — Excellent
Analyzed 20 test cases: 18 design, 2 implementation, 0 violations.
Key observations:
- All 3 extracted helper functions are pure design tests (behavioral contracts).
- 20 table-driven test cases with comprehensive edge-case coverage (nil inputs, feature presence/absence, token fallbacks, staged filtering).
- No mocking violations; proper error handling with
require.NoError(t, err). - Build tag compliant (
//go:build !integrationon line 1). - Test inflation (2.5:1) justified by helper extraction from monolithic function.
Verdict: 90% design tests (threshold: 70%). No violations. APPROVE ✅
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 52.2 AIC · ⌖ 27.5 AIC · ⊞ 7.1K
Comment /matt to run again
| // manager step: the agent output reference, allowed-domains configuration, URL policy, | ||
| // GitHub server/API URLs, custom handler registration maps, and the delegated per-handler | ||
| // config env vars. | ||
| func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) error { |
There was a problem hiding this comment.
[/codebase-design] addSafeOutputCoreEnvVars lacks a nil guard for data.SafeOutputs — lines 202–203 always append env vars even if SafeOutputs is nil. addAppTokenMintingSteps does guard early (line 137); this function should follow the same pattern to make the precondition explicit and prevent silent misbehaviour for future callers.
💡 Suggested fix
Add a nil guard at the top, consistent with addAppTokenMintingSteps:
func (c *Compiler) addSafeOutputCoreEnvVars(steps *[]string, data *WorkflowData) error {
if data.SafeOutputs == nil {
return nil
}
// ...
}@copilot please address this.
…erManagerStep Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔍 PR TriageCategory: refactor · Risk: low · Total score: 48/100
Pure refactor extracting helpers from Recommended action:
|
buildHandlerManagerStepwas a ~250-line function mixing app-token minting, env-var assembly, and token-resolution concerns in a single body, rated 74/100 in the daily code quality check.Changes
addAppTokenMintingSteps(~60 lines) — extracts per-handler and dispatch-repository GitHub App token minting stepsaddSafeOutputCoreEnvVars(~64 lines) — extracts core env vars: agent output reference, allowed-domains, URL policy, custom handler registration maps, and delegated config helpersaddSafeOutputTokenEnvVars(~86 lines) — extracts token env vars: CI-trigger token, project URL/token, assign-to-agent, agent-session, and GITHUB_TOKEN override for cross-repo PR operationsbuildHandlerManagerStepreduced to 41 lines — now a pure orchestration function:compiler_safe_outputs_steps_test.gocovering the three extracted helpers directly (TestAddAppTokenMintingSteps,TestAddSafeOutputCoreEnvVars,TestAddSafeOutputTokenEnvVars)Behavior is identical — pure refactor.