Split pkg/parser/import_field_extractor.go into focused files by concern#54378
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR TriageCategory: Score: 38/100 (impact 20/50 + urgency 8/30 + quality 10/20) Recommended action: Draft, large mechanical file split (682/641), no CI yet. Low urgency. Automated triage — run 32432526976
|
There was a problem hiding this comment.
Pull request overview
Splits the remaining oversized parser extractor into focused modules without changing behavior, addressing #54064.
Changes:
- Separates engine, activation, step/job, and model extraction concerns.
- Retains the core accumulator and extraction pipeline in the original file.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/import_field_extractor.go |
Retains core accumulation and pipeline logic. |
pkg/parser/import_field_extractor_engine.go |
Houses engine and configuration extraction. |
pkg/parser/import_field_extractor_activation.go |
Houses activation and authentication extraction. |
pkg/parser/import_field_extractor_steps.go |
Houses step, job, environment, and feature extraction. |
pkg/parser/import_field_extractor_models.go |
Houses model normalization and policy extraction. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot Quick triage nudge for PR #54378.
Run: https://github.com/github/gh-aw/actions/runs/32434686968
|
Finish pass complete at
Actions taken: completed full triage + local validation; no code changes were needed. Still needed (human): let required checks finish/re-trigger as needed, then merge once checks are green. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Pure mechanical split of a 1048-line file into 5 files along existing logical boundaries; functions moved verbatim with only import lists trimmed. No new abstractions, no reinvented stdlib, no added dependencies, no logic changes.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, pure refactoring PR that splits import_field_extractor.go into four focused files by concern:
import_field_extractor_activation.go— bots, skip-roles, skip-bots, ambient-folders, GitHub token/app, checkoutimport_field_extractor_engine.go— engine config and MCP settingsimport_field_extractor_models.go— model policy, providers, costsimport_field_extractor_steps.go— steps, jobs, env, labels, cache, features, observability
No behavioral changes detected. The split follows existing file-naming conventions in the package, and the package-level doc comment pattern is consistent with other files.
mergeJSONStringListField (defined in the activation file, reused in steps) and parseStringSliceField (defined in models, referenced by engine) are both correct cross-file usages within the same package.
LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.7 AIC · ⌖ 8.84 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a pure refactor with no behaviour changes. The split is clean and well-motivated; two minor helper-placement issues worth fixing.
📋 Key Themes & Highlights
Key Themes
- Helper function homing:
mergeJSONStringListField(activation file) andparseStringSliceField(models file) are both called across file boundaries. Utilities shared by multiple themed files belong in the core file or a dedicated helpers file, not in whichever themed file happened to define them first.
Positive Highlights
- ✅ The five-way split follows the existing logical groupings already expressed in the original function names — minimal cognitive overhead.
- ✅ Each file has a focused file-level doc comment that accurately describes its scope.
- ✅ No logic changes — a pure mechanical split is the right approach for a file this large.
- ✅ Per-file import lists are trimmed correctly to only what each file needs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 43.8 AIC · ⌖ 10 AIC · ⊞ 7.8K
Comment /matt to run again
| }) | ||
| } | ||
|
|
||
| func mergeJSONStringListField( |
There was a problem hiding this comment.
[/codebase-design] mergeJSONStringListField is a general-purpose helper but lives in the activation-themed file, while _steps.go also calls it — the file boundary and the function's home are mismatched.
💡 Suggestion
Move mergeJSONStringListField (and, for symmetry, parseStringSliceField from _models.go) into import_field_extractor.go or a small new import_field_extractor_helpers.go. Keeping shared utilities in a themed file is a navigation trap: future contributors hunting for mergeJSONStringListField will have no obvious reason to look in the activation file.
@copilot please address this.
There was a problem hiding this comment.
Addressed in dbf93fb: moved mergeJSONStringListField into new shared pkg/parser/import_field_extractor_helpers.go so it is no longer housed in the activation-focused file.
| return sanitizedProviders, true | ||
| } | ||
|
|
||
| func parseStringSliceField(value any, keepEmpty bool) []string { |
There was a problem hiding this comment.
[/codebase-design] parseStringSliceField is a generic slice-parsing utility that lives in _models.go but is also called from _engine.go — same cross-file coupling issue as mergeJSONStringListField.
💡 Suggestion
Co-locate both generic helpers (mergeJSONStringListField, parseStringSliceField) in the core import_field_extractor.go or a new import_field_extractor_helpers.go. This gives callers a single predictable home for shared utilities and keeps themed files purely concern-specific.
@copilot please address this.
There was a problem hiding this comment.
Addressed in dbf93fb: moved parseStringSliceField into new shared pkg/parser/import_field_extractor_helpers.go alongside the other cross-file helper.
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.
Verdict
No blocking issues found in the changed lines.
Review notes
This is a mechanical file split of the parser import-field extractor. I checked the moved helpers against the remaining orchestrator, verified the package still builds/tests, and did not find a behavioral delta in the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.5 AIC · ⌖ 6.83 AIC · ⊞ 7K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (682 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 Matter
ADRs 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
|
PR Triage
|
|
@copilot Quick triage nudge for PR #54378.
Run: https://github.com/github/gh-aw/actions/runs/32471825475
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
Validation run locally on this branch: Maintainer handoff:
|
A DeepReport monolithic-file analysis flagged
pkg/parser/import_field_extractor.go(1048 LOC, 51 functions) as a high-value refactor target, along withpkg/cli/add_package_manifest.go. The latter had already been split down to 130 LOC in an earlier change, so this PR addresses the remaining file.Changes
import_field_extractor.gointo 5 files along existing logical boundaries (already reflected in function names/doc comments), with no behavior change:import_field_extractor.go— coreimportAccumulatorstruct, constructor, and main extraction pipeline (extractAllImportFields,prepareFrontmatter, result building)import_field_extractor_engine.go— engine config and scalar/builder-based fields (max-turns, mcp-servers, safe-outputs, sandbox mounts, etc.)import_field_extractor_activation.go— activation/auth fields (bots, skip-roles/bots, github-token, github-app, checkout)import_field_extractor_steps.go— step/job/env/labels/cache/features/run-install-scripts/observability extractionimport_field_extractor_models.go—modelsfield normalization (aliases, allow/block policies, cost overlays)All functions were moved verbatim with per-file import lists trimmed to only what each file uses; no logic was altered.