Uh oh!
There was an error while loading. Please reload this page.
Split add_package_manifest.go into focused files - #54030
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: refactor · Risk: medium · Score: 40/100 (impact 20 + urgency 10 + quality 10) Large draft refactor (2.5K line diff); needs undraft, CI, and careful review before merge. Automated triage — see run report for full details.
|
✅ 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.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR is a pure mechanical file split: pkg/cli/add_package_manifest.go was split verbatim into 5 new files, each carrying only a module-boundary doc comment describing its single responsibility. No new abstractions, reinvented stdlib, or speculative flexibility were introduced, and no logic changed. Nothing to cut per ponytail-review scope.
|
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
This looks like a straight file split of add_package_manifest.go into smaller units. I did not find an actionable correctness, performance, or maintainability regression in the changed lines.
Review notes
- The moved functions appear byte-for-byte equivalent in behavior.
- Imports and helper ownership still line up with their callers.
- I did not see any changed control flow, validation rules, or error handling semantics in the extracted files.
- The background
grumpy-codersub-agent did not return usable output before adjudication, so this review is based on independent analysis only.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.32 AIC · ⌖ 7.91 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Splits repository-package manifest support into focused Go files while preserving behavior and existing test coverage.
Changes:
- Separates resolution, parsing, includes, skills, and remote operations.
- Retains shared types and test indirections centrally.
- Adds file-level responsibility comments.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/add_package_manifest.go | Retains shared types, constants, and stubs. |
pkg/cli/add_package_manifest_resolve.go | Handles package-resolution orchestration. |
pkg/cli/add_package_manifest_parse.go | Handles manifest parsing and validation. |
pkg/cli/add_package_manifest_includes.go | Handles include extraction and normalization. |
pkg/cli/add_package_manifest_skills.go | Handles skill, agent, and file discovery. |
pkg/cli/add_package_manifest_remote.go | Handles repository parsing and remote operations. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
PR Triage
Draft PR, pure code-organization split. CI unstable. No urgency; revisit once undrafted. Automated triage by PR Triage Agent.
|
There was a problem hiding this comment.
Clean, pure-refactoring split. The extracted code in add_package_manifest_includes.go is byte-for-byte identical to what was removed from add_package_manifest.go — no behavioural changes. The file-header comment is accurate and import lists are correct. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.1 AIC · ⌖ 8.77 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — commenting on two minor module-boundary issues. No logic was changed; the split is clean overall.
📋 Key Themes & Highlights
Key Themes
- Utility placement:
stringValue(a general helper) lives in_remote.gobut is called from_includes.goand_parse.go. Moving it to the shared-types file (add_package_manifest.go) clarifies its package-wide scope. - Domain boundary:
extractManifestSkillDirs/extractManifestAgentFilesand their supporting predicates are in_includes.gobut belong to the skills/agents domain. They could move to_skills.goto sharpen each file's single responsibility.
Positive Highlights
- ✅ Excellent module-boundary comments atop each file — they make the split immediately navigable.
- ✅ Shared error sentinels and test-stub indirection vars correctly concentrated in
add_package_manifest.go. - ✅ No logic changes — verbatim move confirmed by the diff; existing test suite continues to exercise all paths.
- ✅ The
_resolve.goorchestration layer is noticeably cleaner to read after the extraction. - ✅
_remote.gothin-wrapper pattern is well executed; theFor/ForHostvariants show consistent naming.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 58.1 AIC · ⌖ 10.1 AIC · ⊞ 7.8K
Comment /matt to run again
| return filepath.ToSlash(filepath.Join(packagePath, relativePath)) | ||
| } | ||
| func stringValue(value any) (string, bool) { |
There was a problem hiding this comment.
[/codebase-design]stringValue is a generic type-assertion helper, not a remote-fetch concern — placing it in _remote.go creates a hidden cross-file dependency for _includes.go and _parse.go which call it widely.
💡 Suggestion
Move stringValue to add_package_manifest.go (the shared-types file), whose module comment explicitly describes it as home for "shared types ... used by the other files below."
// stringValue extracts a string from an untyped any value.funcstringValue(valueany) (string, bool) {
s, ok:=value.(string)
returns, ok
}This makes its package-wide scope immediately apparent to any reader of _includes.go or _parse.go.
@copilot please address this.
| // extractManifestSkillDirs parses the skills array from an aw.yml manifest, validating | ||
| // and normalizing each entry. Each entry must be a path under skills/ that represents | ||
| // the directory for a skill (e.g. "skills/my-skill"). | ||
| func extractManifestSkillDirs(value any, manifestPath string) ([]string, []string) { |
There was a problem hiding this comment.
[/codebase-design]extractManifestSkillDirs and extractManifestAgentFiles live in _includes.go but belong to the skill/agent domain. They parse skills: and agents: manifest keys — and are called from _parse.go — not the includes: field. Placing them here blurs the file's single-responsibility boundary.
💡 Suggestion
Move extractManifestSkillDirs and extractManifestAgentFiles (and the supporting predicates isSupportedSkillDirPath, isSupportedAgentFilePath, etc.) into add_package_manifest_skills.go, which already owns skill/agent discovery. This matches the file's stated responsibility and removes the cross-domain coupling to _includes.go.
@copilot please address this.
Documents the architectural decision to decompose the 1330-line pkg/cli/add_package_manifest.go into six single-responsibility files, capturing context, decision rationale, alternatives, and consequences. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1295 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
|
gh-aw-bot
commented
Aug 20, 2026
@copilot This PR still needs maintainer-facing follow-up. Current blockers:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the follow-up in |
🎉 This pull request is included in a new release. Release: |
pkg/cli/add_package_manifest.gohad grown to 1330 lines, mixing manifest resolution, parsing/validation, include-path extraction, skill/agent-file resolution, and remote-fetch helpers in one file — the largest non-test Go source file in the repo.Changes
add_package_manifest.go(123 lines) — shared types, error sentinels, test-stub indirection varsadd_package_manifest_resolve.go(188 lines) — top-level orchestration of resolving a remote packageadd_package_manifest_parse.go(227 lines) — manifest YAML parsing & validationadd_package_manifest_includes.go(464 lines) —includes/filesfield parsing & normalizationadd_package_manifest_skills.go(225 lines) — skill/agent file discoveryadd_package_manifest_remote.go(177 lines) — GitHub remote-fetch wrappers & repo-spec parsingadd_package_manifest_test.go,add_package_manifest_mapping_test.go) remain unchanged in place, since they're already organized by scenario rather than by source file and continue to exercise the same package.