fix: gh aw add resolves uses: references transitively in imported files#44763
Conversation
- Add downloadFn to frontmatterImportsOpts for test injection - Fix sibling path resolution: bare filenames resolve to importing file's dir - Fix recursion skip: recurse into existing files' imports on force=false - Add unit tests for both bug fixes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw add to resolve uses: references in imported files
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Fixes gh aw add so it correctly fetches transitive frontmatter imports: dependencies and resolves certain relative import paths the same way the compiler does, preventing silently-missing shared files during installation.
Changes:
- Adjusts frontmatter import path resolution so bare filenames resolve relative to the importing file’s directory, while paths containing
/resolve relative to the original workflow base directory. - Ensures recursion into imports still happens when an imported file already exists locally (and
force=false), so transitive dependencies are discovered and fetched. - Adds unit tests to validate sibling (bare-filename) resolution and recursion into pre-existing imported files, using an injectable download function for testability.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/includes.go | Updates recursive frontmatter import fetching: path resolution, recurse-when-existing behavior, and injectable download function. |
| pkg/cli/remote_workflow_test.go | Adds targeted tests covering sibling resolution and recursion into already-present imported files. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| // Fallback: if the selected baseDir is empty (e.g. workflow at repo root | ||
| // with no originalBaseDir set), use currentBaseDir as a last resort. | ||
| if baseDir == "" { | ||
| baseDir = currentBaseDir | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and the approach is sound. Two bugs fixed: (1) bare-filename resolution now uses currentBaseDir, matching the compiler logic; (2) pre-existing files are now recursed into for transitive deps. Cycle safety is intact via the shared seen map. Test coverage is solid with mock downloadFn injection.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 21.9 AIC · ⌖ 4.28 AIC · ⊞ 4.8K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Acceptable
📊 Metrics (2 tests)
Test DetailsTestFetchFrontmatterImportsRecursive_SiblingPathResolution (lines 689-739)
TestFetchFrontmatterImportsRecursive_RecurseIntoExistingFile (lines 741-807)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — COMMENT with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Silent coupling to compiler logic: The
strings.Contains(filePath, "/")heuristic mirrorsdetermineNestedBaseDirbut is not linked to it by comment. Drift between the two will be invisible until it breaks. - Recursion base-dir clarity: The
importedBaseDir := path.Dir(remoteFilePath)choice in the pre-existing-file branch is correct but subtly relies onremoteFilePathbeing the canonical remote path — worth a comment. - Test completeness: Tests verify file existence and download paths, but not the written file content.
Positive Highlights
- ✅ Both bugs are properly diagnosed, root-caused, and fixed — not just papered over
- ✅
downloadFninjection follows the established dispatch-workflow pattern, keeping tests network-free - ✅ Test names read as specifications; Arrange/Act/Assert structure is clear
- ✅ The
seenmap prevents infinite cycles through already-visited imports - ✅ Detailed PR description makes the root-cause analysis easy to follow
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38.4 AIC · ⌖ 4.64 AIC · ⊞ 6.6K
Comment /matt to run again
| // Example: "shared/reporting.md" from ".github/workflows/shared/daily-audit-base.md" | ||
| // → ".github/workflows/shared/reporting.md" | ||
| var baseDir string | ||
| if !strings.Contains(filePath, "/") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The strings.Contains(filePath, "/") heuristic mirrors determineNestedBaseDir in the compiler, but no code comment links to that function. If the compiler logic diverges, this silent coupling will be hard to spot.
💡 Suggestion
Add a reference comment above the if block:
// Resolution mirrors pkg/parser.determineNestedBaseDir:
// bare filenames (no "/") → sibling of the importing file (currentBaseDir)
// slash-paths → repo-root-relative (originalBaseDir)
if !strings.Contains(filePath, "/") {This makes it easy to grep for the coupling and keep both sides in sync.
@copilot please address this.
| // any missing transitive dependencies. | ||
| if existingContent, readErr := os.ReadFile(targetPath); readErr == nil { | ||
| importedBaseDir := path.Dir(remoteFilePath) | ||
| fetchFrontmatterImportsRecursive(ctx, string(existingContent), importedBaseDir, opts) |
There was a problem hiding this comment.
[/diagnosing-bugs] When recursing into an existing file, importedBaseDir is set to path.Dir(remoteFilePath), but remoteFilePath may equal originalBaseDir + "/" + localRelPath — not the local disk path. This is correct for remote-path recursion, but if the existing file's local content references paths that were written relative to a different layout, the recursion will use the wrong base dir.
Consider adding an assertion or comment that remoteFilePath is always the canonical remote path at this point, so future readers know this is intentional and not a bug.
@copilot please address this.
| } | ||
|
|
||
| // Process shared/control.md; currentBaseDir mirrors its location in the source repo. | ||
| fetchFrontmatterImportsRecursive(t.Context(), controlContent, ".github/workflows/shared", opts) |
There was a problem hiding this comment.
[/tdd] The test asserts require.Len(t, downloadedPaths, 1) but does not verify the content or location of the written file for the stub ([]byte("# stub\n")). The file-existence check later catches gross path errors, but a test that also asserts the written bytes would make the regression test more complete and protect against silent overwrites.
💡 Example addition
content, err := os.ReadFile(filepath.Join(tmpDir, "shared", "control-aux.md"))
require.NoError(t, err)
assert.Equal(t, "# stub\n", string(content))@copilot please address this.
There was a problem hiding this comment.
Review: non-blocking observations
The two fixes (sibling-path resolution and recursion into existing files) are correct and well-tested. Two issues worth addressing before this hardens further:
Issues found
1. Silent transitive-dep failures (medium) — fetchFrontmatterImportsRecursive is void and swallows all internal errors. A failed transitive download or write produces an incomplete local install with no signal to the user beyond a log line. Addressed in the inline comment at line 384.
2. Windows path-separator heuristic (low) — strings.Contains(filePath, "/") uses a hard-coded POSIX separator. Addressed in the inline comment at line 297.
🔎 Code quality review by PR Code Quality Reviewer · 48.4 AIC · ⌖ 4.53 AIC · ⊞ 5.4K
Comment /review to run again
| // any missing transitive dependencies. | ||
| if existingContent, readErr := os.ReadFile(targetPath); readErr == nil { | ||
| importedBaseDir := path.Dir(remoteFilePath) | ||
| fetchFrontmatterImportsRecursive(ctx, string(existingContent), importedBaseDir, opts) |
There was a problem hiding this comment.
Silent failure on transitive dependency recursion: errors inside the recursive call are fully swallowed — if any transitive download or write fails, the caller has no indication that the local copy is incomplete.
💡 Detail
At line 384, fetchFrontmatterImportsRecursive is called for an existing file's imports. The function has no return value, so download failures, disk-write failures, or permission errors inside recursion are silently logged at best. A broken transitive dep leaves an incomplete install with no observable error surfaced to the user.
The same void-return pattern exists at line 438 (newly downloaded files), so partial-failure state cannot propagate at any recursion depth.
Consider returning an error or accumulating failures (e.g., a []error slice) so the top-level caller can report incomplete installs clearly.
| // Example: "shared/reporting.md" from ".github/workflows/shared/daily-audit-base.md" | ||
| // → ".github/workflows/shared/reporting.md" | ||
| var baseDir string | ||
| if !strings.Contains(filePath, "/") { |
There was a problem hiding this comment.
Forward-slash heuristic breaks on Windows paths: strings.Contains(filePath, "/") only detects POSIX separators; on Windows, import paths using backslashes will be misclassified as bare filenames and resolved against currentBaseDir instead of originalBaseDir.
💡 Suggested fix
Normalise filePath to forward-slashes before the check, consistent with the filepath.FromSlash usage elsewhere in this function:
normalisedFilePath := filepath.ToSlash(filePath)
if !strings.Contains(normalisedFilePath, "/") {
baseDir = currentBaseDir
} else {
baseDir = opts.originalBaseDir
}Or use path.Base(filePath) == filePath as a cross-platform equivalent (true when there is no directory component).
…tion Documents the path-structure heuristic and recursive-into-existing-file decisions made in pkg/cli/includes.go as part of PR #44763. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (163 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
|
|
@copilot please run the
|
When originalBaseDir is empty (workflow at repo root) and a shared file imports a path with "/" (e.g. "shared/helper.md"), the previous fallback incorrectly prefixed the path with currentBaseDir, producing "shared/shared/helper.md" instead of "shared/helper.md". Remove the fallback entirely; the existing `if baseDir != ""` block already handles the empty case by using filePath as-is. Add TestFetchFrontmatterImportsRecursive_RepoRootSlashPath to cover this scenario explicitly. Also update wasm golden files for the oauth_token_check_failed field added in the recent main merge. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review feedback in the latest commit. The |
|
🎉 This pull request is included in a new release. Release: |
gh aw addcorrectly fetched directimports:dependencies but silently dropped transitive ones — files referenced by those shared files were never installed.Root Causes
Two bugs in
fetchFrontmatterImportsRecursive(pkg/cli/includes.go):1. Wrong base directory for bare filenames
Simple filenames with no
/(e.g.,control-aux.md) were resolved againstoriginalBaseDir(the workflow root) instead ofcurrentBaseDir(the importing file's directory). This mismatched the compiler'sdetermineNestedBaseDirlogic:2. Recursion skipped for pre-existing files
When
force=falseand a shared file already existed on disk, the code skipped both the download and recursion into that file's imports. Any transitive deps below an already-present file were never fetched.Changes
pkg/cli/includes.go/) now resolve usingcurrentBaseDir; paths containing/continue to useoriginalBaseDir— matching compiler behaviorcontinuedownloadFnfield tofrontmatterImportsOpts(nil →parser.DownloadFileFromGitHub) to enable unit-testable mock injection, consistent with the dispatch-workflow patternpkg/cli/remote_workflow_test.goTestFetchFrontmatterImportsRecursive_SiblingPathResolution: asserts bare-filename import resolves to the importing file's directory, not the workflow rootTestFetchFrontmatterImportsRecursive_RecurseIntoExistingFile: asserts transitive deps are fetched even when the parent shared file already exists on disk