Refactor parser config-field extraction into composable helpers to satisfy largefunc limits#36664
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors pkg/parser import frontmatter config extraction by decomposing a previously large extractConfigFields function into smaller, composable helpers to satisfy function-length linting limits, while aiming to preserve first-wins and accumulation semantics across imports.
Changes:
- Split
extractConfigFieldsinto targeted helpers for first-wins scalar extraction and append/merge field accumulation. - Added a regression test to lock in “first import wins” for scalar max-* fields and accumulation for several builder/slice fields.
- Renamed the “Install GitHub Copilot SDK” workflow step label across many locked workflows to clarify it’s the Node.js SDK.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/import_field_extractor.go | Refactors config-field extraction into helper methods while keeping existing extraction strategies. |
| pkg/parser/import_field_extractor_test.go | Adds a regression test for first-wins scalar fields and multi-import accumulation. |
| .github/workflows/smoke-copilot-sdk.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/q.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/python-data-charts.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/pr-triage-agent.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/pr-nitpick-reviewer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/plan.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/pdf-summary.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/org-health-report.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/metrics-collector.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/mergefest.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/mcp-inspector.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/linter-miner.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/layout-spec-maintainer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/jsweep.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/firewall.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/firewall-escape.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/draft-pr-cleanup.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/docs-noob-tester.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/discussion-task-miner.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/dictation-prompt.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/dev-hawk.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/deployment-incident-monitor.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/delight.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/dead-code-remover.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-workflow-updater.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-syntax-error-quality.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-spdd-spec-planner.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-skill-optimizer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-sentrux-report.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-security-observability.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-secrets-analysis.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-safe-output-integrator.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-repo-chronicle.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-performance-summary.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-model-inventory.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-mcp-concurrency-analysis.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-malicious-code-scan.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-issues-report.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-geo-optimizer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-experiment-report.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-compiler-threat-spec-optimizer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-compiler-quality.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-cli-performance.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-assign-issue-to-user.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-architecture-diagram.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/daily-agent-of-the-day-blog-writer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/craft.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/copilot-pr-merged-report.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/copilot-opt.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/copilot-cli-deep-research.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/code-scanning-fixer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/cli-consistency-checker.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/ci-coach.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/breaking-change-checker.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/brave.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/artifacts-summary.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/architecture-guardian.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
| .github/workflows/agent-performance-analyzer.lock.yml | Clarifies Copilot SDK install step name (Node.js). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 64/64 changed files
- Comments generated: 3
| "runtimes": map[string]any{"node": map[string]any{"version": "20"}}, | ||
| "network": map[string]any{"allow": []any{"github.com"}}, | ||
| "permissions": map[string]any{"contents": "read"}, | ||
| "secret-masking": map[string]any{"enabled": true}, | ||
| } |
| "runtimes": map[string]any{"python": map[string]any{"version": "3.12"}}, | ||
| "network": map[string]any{"allow": []any{"api.github.com"}}, | ||
| "permissions": map[string]any{"issues": "write"}, | ||
| "secret-masking": map[string]any{"log-mask": true}, | ||
| } |
| assert.Contains(t, acc.runtimesBuilder.String(), "node") | ||
| assert.Contains(t, acc.runtimesBuilder.String(), "python") | ||
| assert.Contains(t, acc.networkBuilder.String(), "github.com") | ||
| assert.Contains(t, acc.networkBuilder.String(), "api.github.com") | ||
| assert.Contains(t, acc.permissionsBuilder.String(), "contents") |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36664 does not have the 'implementation' label (has_implementation_label=false) and has 93 new lines of code in business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). The single new test (TestExtractConfigFields_FirstWinsAndAccumulates) is a behavioral design test with descriptive assertion messages and no guideline violations. Consider adding edge-case coverage (nil/empty inputs, zero-value numeric fields) to raise the score above 80.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — one minor coverage gap to address, otherwise solid work.
📋 Key Themes & Highlights
Key Themes
- Coverage gap:
appendYAMLBuilderField(thestepsandservicespaths) is the only new helper not exercised byTestExtractConfigFields_FirstWinsAndAccumulates. All three JSON-flavored helpers are well covered. - Refactor fidelity: The extraction of first-wins logic (
extractFirstWinsJSONField) correctly replicates the original*target != ""guard and the"null"check. No semantic drift detected.
Positive Highlights
- ✅ Clean decomposition — each helper has a single, obvious responsibility
- ✅
extractConfigFieldsis now a readable index of all nine fields in the accumulator - ✅ Test uses concrete, differentiated values for first vs. second import, making failures easy to diagnose
- ✅ Net line reduction confirms no dead code was introduced
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 942.9K
Comments that could not be inline-anchored
pkg/parser/import_field_extractor_test.go:927
[/tdd] steps and services (both appendYAMLBuilderField call sites) are absent from the test fixtures and assertions, leaving the only new helper that calls extractYAMLFieldFromMap uncovered by this test.
<details>
<summary>💡 Suggested addition</summary>
Add YAML-valued fields to both first and second, then assert accumulation:
first := map[string]any{
// ... existing fields ...
"steps": []any{map[string]any{"name": "step-a", "run": "echo first"}},
"servi…
</details>
The lint-monster sweep identified widespread function-length violations; in this PR, we take a focused slice in
pkg/parserby refactoring a 71-line config extractor into smaller units without changing behavior. This keeps first-wins import semantics and merged-field accumulation intact while bringing the target function under the 60-line threshold.Config extraction decomposition (
pkg/parser/import_field_extractor.go)extractConfigFieldsinto focused helpers:extractFirstWinsJSONFieldappendJSONBuilderFieldappendJSONSliceFieldappendYAMLBuilderFieldmax-turns,max-runs,max-effective-tokens,max-daily-effective-tokens)mcp-servers,safe-outputs,mcp-scripts,steps,runtimes,services,network,permissions,secret-masking)Behavior lock-in for refactor (
pkg/parser/import_field_extractor_test.go)TestExtractConfigFields_FirstWinsAndAccumulatesto assert: