Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Hey
When you're ready to implement, here's a prompt you can use:
|
When gh aw add fetches a remote workflow via a fallback path (e.g. .github/workflows/my-workflow.md instead of the short-form my-workflow.md), FetchedWorkflow.SourcePath holds the actual repo-root-relative path. Use it so gh aw update can re-fetch from the correct location, preventing 404 errors. Fixes the bug where source: owner/repo/my-workflow.md@SHA was written instead of source: owner/repo/.github/workflows/my-workflow.md@SHA. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/63414f90-ba8f-47d9-a62c-e532d598b2dc Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Test Inflation NoteThe test adds 75 lines while the corresponding production file ( Test Quality HighlightsThe single new test is a strong behavioral contract test:
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24913143079
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes incorrect source: frontmatter for workflows fetched via fallback path resolution so subsequent gh aw update calls don’t 404.
Changes:
- Use the fetched workflow’s resolved
SourcePath(repo-root-relative) when building thesource:string for remote workflows in both normal add and trial install flows. - Add a unit test ensuring the written
source:field reflects the actual fetched path when fallback resolution occurred.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/add_command.go | Builds source: from the effective (resolved) workflow path when remote fallback resolution is used. |
| pkg/cli/trial_repository.go | Applies the same effective-path source: behavior in trial-mode installs. |
| pkg/cli/add_command_test.go | Adds a test asserting the source: field uses the resolved .github/workflows/... path rather than the short-form path. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| specCopy.WorkflowPath = sourceInfo.SourcePath | ||
| effectiveSpec = &specCopy | ||
| } | ||
| sourceString := buildSourceStringWithCommitSHA(effectiveSpec, commitSHA) |
There was a problem hiding this comment.
SourcePath is used to build the source: field, but when fallback resolution happens the rest of this function still uses the original workflowSpec.WorkflowPath (e.g. for fetchAllRemoteDependencies above and processIncludesWithWorkflowSpec below). Those codepaths resolve relative include/import/dispatch/resource paths based on WorkflowPath, so keeping the short-form path can cause incorrect dependency resolution (treating siblings as repo-root files instead of .github/workflows/...). Consider propagating the effective path to downstream processing as well (e.g. update workflowSpec.WorkflowPath for the remainder of the function or pass the same effectiveSpec to dependency/include processing).
| sourceString := buildSourceStringWithCommitSHA(effectiveSpec, commitSHA) | |
| // Propagate the effective path for the remainder of this function so any | |
| // downstream relative include/import resolution uses the same canonical path | |
| // that was used to build the source field. | |
| workflowSpec = effectiveSpec | |
| sourceString := buildSourceStringWithCommitSHA(workflowSpec, commitSHA) |
| // When the fetch used a fallback path (e.g. .github/workflows/my-workflow.md | ||
| // instead of the short-form my-workflow.md), SourcePath holds the actual | ||
| // repo-root-relative path. Use it so that gh aw update can later re-fetch | ||
| // from the correct location. | ||
| effectiveSpec := parsedSpec | ||
| if fetched.SourcePath != "" && fetched.SourcePath != parsedSpec.WorkflowPath { | ||
| specCopy := *parsedSpec | ||
| specCopy.WorkflowPath = fetched.SourcePath | ||
| effectiveSpec = &specCopy | ||
| } | ||
| sourceString := buildSourceStringWithCommitSHA(effectiveSpec, fetched.CommitSHA) |
There was a problem hiding this comment.
This updates the source: field to use fetched.SourcePath when fallback resolution is used, but subsequent calls (e.g. remote dependency fetching later in this function) still pass parsedSpec with the original short-form WorkflowPath. Several dependency resolvers use spec.WorkflowPath to compute the base directory, so fallback resolution can still lead to incorrect relative path resolution for includes/imports/dispatch workflows/resources. Consider updating parsedSpec.WorkflowPath (or using the same effectiveSpec) for the remainder of the trial install flow when fetched.SourcePath differs.
See below for a potential fix:
// When the fetch used a fallback path (e.g. .github/workflows/my-workflow.md
// instead of the short-form my-workflow.md), SourcePath holds the actual
// repo-root-relative path. Normalize parsedSpec so any later dependency
// resolution in this function uses the same effective workflow path.
if fetched.SourcePath != "" && fetched.SourcePath != parsedSpec.WorkflowPath {
specCopy := *parsedSpec
specCopy.WorkflowPath = fetched.SourcePath
parsedSpec = &specCopy
}
// Add source field to frontmatter for remote workflows
if !fetched.IsLocal && fetched.CommitSHA != "" {
sourceString := buildSourceStringWithCommitSHA(parsedSpec, fetched.CommitSHA)
|
@copilot review all comments, add tests |
…ter fallback resolution Per review feedback: after normalizing the spec to use SourcePath when fallback resolution was triggered, reassign workflowSpec (add_command.go) and parsedSpec (trial_repository.go) so that processIncludesWithWorkflowSpec, includeSourceDir, and fetchAllRemoteDependencies all use the canonical repo-root-relative path. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c712c321-55d8-43f3-b219-f383a9e07caa Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both reviewer suggestions in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot add more tests to cover combinations of local and remote specs within the same workflow, including a remote spec that references the current repository. |
…mote, self-referential remote Agent-Logs-Url: https://github.com/github/gh-aw/sessions/52476730-c034-41e8-a1cb-3ac9d29cfe0f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added
Also extracted a Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
gh aw addwrites a short-formsource:path when the workflow is fetched via fallback path resolution (e.g.owner/repo/my-workflow.md@SHAinstead ofowner/repo/.github/workflows/my-workflow.md@SHA), causinggh aw updateto 404.Root cause
fetchRemoteWorkflowtries the parsed spec path first, then falls back to.github/workflows/<name>.md. The actual resolved path is stored inFetchedWorkflow.SourcePath, butbuildSourceStringWithCommitSHAwas called withworkflowSpec.WorkflowPath(the original parsed path), discarding the fallback resolution. The short-form path was also being used for downstream include/import/dependency resolution, causing incorrect relative-path computation for sibling files.Changes
add_command.go: After detecting a fallback-resolved path, reassignworkflowSpecto useSourcePathso all downstream processing —buildSourceStringWithCommitSHA,processIncludesWithWorkflowSpec, andincludeSourceDir— uses the canonical repo-root-relative path.trial_repository.go: NormalizeparsedSpec.WorkflowPathtofetched.SourcePathbefore the source-field addition block, so thatbuildSourceStringWithCommitSHAand the subsequentfetchAllRemoteDependenciescall both use the correct path.add_command_test.go: AddedTestAddWorkflowWithTracking_UsesActualFetchedPath— sets up a minimal git repo, invokesaddWorkflowWithTrackingwith aResolvedWorkflowwhereSourceInfo.SourcePath(.github/workflows/my-workflow.md) differs fromSpec.WorkflowPath(my-workflow.md), and asserts the writtensource:field contains the full path.add_command_test.go: AddedTestAddWorkflowWithTracking_SourceFieldVariants— a table-driven test covering three additional combinations:IsLocal=true): verifies nosource:field is writtenSourcePath == WorkflowPath): verifies thesource:field uses the already-canonical path unchangedRepoSlugmatches the "current" repository and fallback path resolution is triggered; verifies thesource:field uses the full.github/workflows/path and not the short-form