Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ce841919-e57f-4c67-b519-9fada0a7a89b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ce841919-e57f-4c67-b519-9fada0a7a89b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix asymmetric .md/.lock.yml issue causing ERR_CONFIG at runtime
Harden Apr 20, 2026
gh aw add SHA pinning and enforce .md/.lock.yml frontmatter hash parity
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens gh aw add <path>@ref by enforcing ref→SHA pinning (with transient retries) and adds regression coverage to ensure .md frontmatter hashes and .lock.yml metadata hashes stay in parity based on the same effective source bytes.
Changes:
- Add retry + explicit failure behavior for ref→SHA resolution (no silent fallback).
- Ensure install-time processing reads back the written
.mdbefore downstream steps to align hash inputs. - Add unit/integration tests for SHA resolution retries, failure behavior, and frontmatter-hash parity; update stale-lock guidance docs.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/fetch.go | Adds ref→SHA retry logic and transient error detection; removes silent SHA fallback. |
| pkg/cli/remote_workflow_test.go | Adds unit tests covering transient success, permanent failure, and retry exhaustion for SHA resolution. |
| pkg/cli/add_command.go | Reads back the written workflow file to ensure downstream processing uses on-disk bytes. |
| pkg/cli/add_integration_test.go | Adds integration assertions for SHA-resolution failure behavior and lock frontmatter-hash parity. |
| pkg/workflow/stale_check_test.go | Adds regression test asserting lock frontmatter-hash parity for pinned and ref-based source:. |
| actions/setup/md/stale_lock_file_failed.md | Updates operator guidance for fresh-install mismatch due to SHA resolution failures. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
Comment on lines
+159
to
+166
| if !isTransientSHAResolutionError(err) { | ||
| retryCommand := fmt.Sprintf("gh aw add %s/%s/%s@<40-char-sha>", owner, repo, workflowPath) | ||
| message := fmt.Sprintf( | ||
| "failed to resolve '%s' to commit SHA for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Try: %s.", | ||
| ref, owner, repo, err, retryCommand, | ||
| ) | ||
| fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) | ||
| return "", fmt.Errorf("%s: %w", message, err) |
There was a problem hiding this comment.
resolveCommitSHAWithRetries prints a formatted error to stderr and also returns an error. Since main() already prints returned errors, this will likely result in duplicated user-facing error output. Consider returning a pre-formatted error (e.g., starting with the same formatted prefix) and letting the top-level handler print it once, or remove the direct Fprintln calls here and rely on the returned error only.
Comment on lines
+161
to
+167
| message := fmt.Sprintf( | ||
| "failed to resolve '%s' to commit SHA for '%s/%s': %v. Expected the GitHub API to return a commit SHA for the ref. Try: %s.", | ||
| ref, owner, repo, err, retryCommand, | ||
| ) | ||
| fmt.Fprintln(os.Stderr, console.FormatErrorMessage(message)) | ||
| return "", fmt.Errorf("%s: %w", message, err) | ||
| } |
There was a problem hiding this comment.
The returned error is constructed as fmt.Errorf("%s: %w", message, err) but message already embeds the error via %v, so the final error string will repeat the underlying error text (once in message, once from %w). Consider removing %v from message or avoid wrapping with %w when the error is already fully included in the message.
Comment on lines
+169
to
+177
| if attempt < attempts { | ||
| delay := shaResolutionRetryDelays[attempt-1] | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage( | ||
| fmt.Sprintf("Transient SHA resolution failure for '%s' (attempt %d/%d). Retrying in %s...", ref, attempt, attempts, delay), | ||
| )) | ||
| } | ||
| sleepBeforeSHAResolutionRetry(delay) | ||
| } |
There was a problem hiding this comment.
The retry loop sleeps via time.Sleep but has no way to respect cancellation (Ctrl-C) because no context.Context is plumbed through. This can make gh aw add feel unresponsive during the 1s/3s/9s backoff. Consider threading a context into FetchWorkflowFromSource/fetchRemoteWorkflow/resolveCommitSHAWithRetries and using a context-aware sleep (select on ctx.Done() vs time.After(delay)).
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 95/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (6 tests)
ObservationsFunction Variable Injection in
|
| Component | Score | Notes |
|---|---|---|
| Behavioral coverage (40 pts) | 40/40 | 6/6 tests verify behavioral contracts |
| Error/edge case coverage (30 pts) | 25/30 | 5/6 tests include error/failure paths; hash parity addition is happy-path only |
| Low duplication (20 pts) | 20/20 | No copy-paste clusters detected |
| Proportional growth (10 pts) | 10/10 | remote_workflow_test.go (101 lines) vs fetch.go (90 lines) = 1.12:1; stale_check_test.go (52 lines) covers a new invariant with minimal production code |
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 6 tests — 1 integration (//go:build integration), 5 unit (//go:build !integration) - 🟨 JavaScript (
*.test.cjs,*.test.js): 0 tests
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All 6 new/modified tests enforce observable behavioral contracts: SHA resolution retry semantics, clean failure on resolution error (no partial artifacts), and frontmatter hash parity between source markdown and compiled lock file.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §24690039566
🧪 Test quality analysis by Test Quality Sentinel · ● 540.6K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 6 new/modified tests enforce behavioral contracts covering SHA resolution retry semantics, clean failure modes, and frontmatter hash parity.
Contributor
|
Hey This looks ready for maintainer review. ✅
|
Collaborator
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Reviewed and addressed the actionable review comments in commits 9438981 and 895e58f: removed duplicate stderr emission, fixed duplicated wrapped error text, and added context-cancelable retry backoff by plumbing context through add/resolve/fetch paths with updated tests. 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:
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pelikhan
reviewed
Apr 20, 2026
Comment on lines
+175
to
+177
| func AddWorkflows(workflows []string, opts AddOptions) (*AddWorkflowsResult, error) { | ||
| return AddWorkflowsWithContext(context.Background(), workflows, opts) | ||
| } |
Collaborator
There was a problem hiding this comment.
@copilot context is always required, avoid context.Background
Contributor
Author
There was a problem hiding this comment.
Updated in 4d28e3f: removed the context.Background() default wrapper and made AddWorkflows require caller context explicitly.
Comment on lines
+50
to
+53
| func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, error) { | ||
| return ResolveWorkflowsWithContext(context.Background(), workflows, verbose) | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
@copilot always try to get the context from the caller, avoid context.Background
Contributor
Author
There was a problem hiding this comment.
Updated in 4d28e3f: removed the context.Background() default wrapper and made ResolveWorkflows require caller-provided context.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8ddf9ab4-df4f-4bfa-b6b3-634452eb614e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Collaborator
|
@copilot the default compiler mode should be to error when an action ref cannot be resolved and pinned. Add a looser mode (--allow-action-refs) that lowers this error as a warning. Update commands that do action pins resolution with this argument/mode. |
Collaborator
|
@copilot when a ref resolution fails, record error type and store in lock file manifest for auditing. |
…erride Agent-Logs-Url: https://github.com/github/gh-aw/sessions/45b2aa41-2063-45e0-838b-d6afb0860d52 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in 4d28e3f: unresolved action refs now fail by default in compiler-driven pinning, and added looser 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
AI
changed the title
Harden
Harden Apr 20, 2026
gh aw add SHA pinning and enforce .md/.lock.yml frontmatter hash paritygh aw add SHA pinning, enforce .md/.lock.yml frontmatter hash parity, and require pinned action refs by default
This was referenced Apr 20, 2026
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e68b7938-ba90-4e65-91a4-f434dd9579e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in f2ebd1e (core behavior introduced in e49dddc): unresolved action-ref pinning now records 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
AI
changed the title
Harden
Harden Apr 20, 2026
gh aw add SHA pinning, enforce .md/.lock.yml frontmatter hash parity, and require pinned action refs by defaultgh aw add SHA pinning, enforce .md/.lock.yml frontmatter hash parity, require pinned action refs by default, and audit pin-resolution failures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gh aw add <path>@refcould silently fall back when ref→SHA resolution failed, producing unpinnedsourcemetadata and, in edge cases, stale-lock failures immediately after install. This change removes silent fallback behavior and tightens install/compile parity so lock hashes are derived from the same effective source bytes as runtime checks.SHA resolution: retry + explicit failure (no silent fallback)
1s,3s,9s) on rate-limit/timeouts/5xx-style failures.addfails with a user-visible error and a concrete retry command using an exact SHA placeholder.Install pipeline parity guard
sourceinjection/include processing), the final.mdis written and read back from disk before downstream processing that depends on content fidelity.Regression coverage for stale-lock/hash invariants
.md/.lock.ymlartifacts are produced on failure.lock.ymlfrontmatter hash matches recomputed hash from written.md.source: ...@<sha>source: ...@<ref>.Operator-facing docs
Review follow-up hardening
AddWorkflows/ResolveWorkflows(caller-provided context required)FetchWorkflowFromSourceWithContextfetchRemoteWorkflow/resolveCommitSHAWithRetriescontext.Background()defaults.Action pinning policy update (compiler/validate)
--allow-action-refs, forcompileandvalidateto downgrade unresolved action-ref pinning failures to warnings.Manifest audit trail for unresolved action-ref pinning
repo,ref,error_type) during compilation and stored them in lock metadata.gh-aw-manifestwith aresolution_failuressection containing normalized, deduplicated, and sorted records for auditing.