Skip to content

Commit de08e90

Browse files
Don't abort --migrate-local-actions on an unparseable workflow (#229)
* Skip unparseable files during local-action migration Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Use a local action with a remote dependency in the unparseable-workflow test Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Skip only YAML syntax errors during local-action migration Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 6272966 commit de08e90

4 files changed

Lines changed: 126 additions & 2 deletions

File tree

‎cmd/gh-actions-lock/migrate_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package main
22

33
import (
4+
"encoding/json"
45
"os"
56
"path/filepath"
67
"strings"
78
"testing"
89

10+
"github.com/github/gh-actions-lock/cmd/gh-actions-lock/format"
911
"github.com/github/gh-actions-lock/internal/ghapi/httpmock"
1012
"github.com/stretchr/testify/assert"
1113
"github.com/stretchr/testify/require"
@@ -275,3 +277,82 @@ jobs:
275277
assert.NotContains(t, got, "$/", "opt-out means no migration to $/")
276278
})
277279
}
280+
281+
func TestMigrateLocalActions_UnreadableWorkflowAborts(t *testing.T) {
282+
dir := t.TempDir()
283+
require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755))
284+
write := func(rel, content string) string {
285+
full := filepath.Join(dir, rel)
286+
require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755))
287+
require.NoError(t, os.WriteFile(full, []byte(content), 0o644))
288+
return full
289+
}
290+
write("local/action.yml", "runs:\n using: composite\n")
291+
const good = "jobs:\n build:\n steps:\n - uses: ./local\n"
292+
goodPath := write(".github/workflows/good.yml", good)
293+
lockedPath := write(".github/workflows/locked.yml", good)
294+
require.NoError(t, os.Chmod(lockedPath, 0o000))
295+
t.Cleanup(func() { _ = os.Chmod(lockedPath, 0o644) })
296+
if _, err := os.ReadFile(lockedPath); err == nil {
297+
t.Skip("file permissions not enforced for this user")
298+
}
299+
300+
total, _, err := migrateLocalActions([]string{goodPath, lockedPath})
301+
302+
require.Error(t, err)
303+
assert.ErrorIs(t, err, os.ErrPermission)
304+
assert.Zero(t, total)
305+
got, readErr := os.ReadFile(goodPath)
306+
require.NoError(t, readErr)
307+
assert.Equal(t, good, string(got))
308+
}
309+
310+
// TestMigrateLocalActions_UnparseableWorkflowIsAFinding: a workflow that
311+
// won't parse doesn't abort migration; it's reported like a default run.
312+
func TestMigrateLocalActions_UnparseableWorkflowIsAFinding(t *testing.T) {
313+
reg := &httpmock.Registry{}
314+
defer reg.Verify(t)
315+
setupGoSHA := "4a3601121dd01d1626a1e23e37211e3254c1c06c"
316+
reg.Register(
317+
httpmock.GraphQLForRepo("actions", "setup-go"),
318+
httpmock.JSONResponse(map[string]any{
319+
"data": map[string]any{
320+
"a0": testRepoResponse("actions/setup-go", setupGoSHA, nodeActionYAML),
321+
},
322+
}),
323+
)
324+
325+
dir := t.TempDir()
326+
write := func(rel, content string) {
327+
full := filepath.Join(dir, rel)
328+
require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755))
329+
require.NoError(t, os.WriteFile(full, []byte(content), 0o600))
330+
}
331+
require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755))
332+
const good = "name: CI\non: push\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n - uses: ./.github/actions/foo\n"
333+
write(".github/workflows/good.yml", good)
334+
write(".github/workflows/bad.yml", "name: [\n")
335+
write(".github/actions/foo/action.yml",
336+
"name: Foo\nruns:\n using: composite\n steps:\n - uses: actions/setup-go@v6\n")
337+
t.Chdir(dir)
338+
339+
stdout, _, err := runCommandWithHTTP(t, reg, "--migrate-local-actions", "--json=valid,findings")
340+
341+
require.ErrorIs(t, err, errSilent, "the load error blocks the write like a default run")
342+
var payload struct {
343+
Valid bool `json:"valid"`
344+
Findings []format.Finding `json:"findings"`
345+
}
346+
require.NoError(t, json.Unmarshal([]byte(stdout), &payload))
347+
assert.False(t, payload.Valid)
348+
var loadErrors []string
349+
for _, f := range payload.Findings {
350+
if strings.Contains(f.Detail, "failed to load workflow") {
351+
loadErrors = append(loadErrors, f.Workflow)
352+
}
353+
}
354+
assert.Equal(t, []string{".github/workflows/bad.yml"}, loadErrors)
355+
got, readErr := os.ReadFile(filepath.Join(dir, ".github", "workflows", "good.yml"))
356+
require.NoError(t, readErr)
357+
assert.Equal(t, good, string(got), "a blocked run rolls back the migration")
358+
}

‎cmd/gh-actions-lock/run.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -748,8 +748,14 @@ func migrateLocalActions(paths []string) (int, func(), error) {
748748
if err != nil {
749749
return 0, undo, fmt.Errorf("loading %s: %w", path, err)
750750
}
751-
wf, err := workflowfile.Load(path)
751+
// Invalid YAML can't be migrated; the scan reports it as a load
752+
// finding like any other run, so skip it. Anything else aborts.
753+
wf, err := workflowfile.Parse(path, original)
752754
if err != nil {
755+
var syntaxErr *workflowfile.SyntaxError
756+
if errors.As(err, &syntaxErr) {
757+
continue
758+
}
753759
return 0, undo, fmt.Errorf("loading %s: %w", path, err)
754760
}
755761
content, changed, err := wf.MigrateLocalActionsToSelfRepository()

‎internal/workflowfile/workflowfile.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,16 @@ import (
2121

2222
var errPathOutsideRoot = errors.New("path resolves outside repository root")
2323

24+
// SyntaxError reports content that isn't valid YAML, as opposed to a file
25+
// that couldn't be read or a document that parsed but was rejected.
26+
type SyntaxError struct {
27+
Err error
28+
}
29+
30+
func (e *SyntaxError) Error() string { return "parsing workflow YAML: " + e.Err.Error() }
31+
32+
func (e *SyntaxError) Unwrap() error { return e.Err }
33+
2434
// File is the parsed workflow YAML the CLI rewrites in-place.
2535
// It carries the original byte content alongside the parsed node tree so
2636
// RewriteActionRefs can do anchored, comment-preserving substitution.
@@ -47,7 +57,7 @@ func Parse(path string, content []byte) (*File, error) {
4757
Content: content,
4858
}
4959
if err := yaml.Unmarshal(content, &f.root); err != nil {
50-
return nil, fmt.Errorf("parsing workflow YAML: %w", err)
60+
return nil, &SyntaxError{Err: err}
5161
}
5262

5363
var invalid []string

‎internal/workflowfile/workflowfile_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package workflowfile
22

33
import (
4+
"errors"
45
"os"
56
"path/filepath"
67
"strings"
@@ -734,3 +735,29 @@ func TestDiscoverCompositeActionFiles_RejectsSymlinkEscape(t *testing.T) {
734735
assert.Contains(t, err.Error(), "outside repository root")
735736
assert.Empty(t, paths)
736737
}
738+
739+
func TestParseErrorKinds(t *testing.T) {
740+
tests := []struct {
741+
name string
742+
content string
743+
wantSyntax bool
744+
}{
745+
{name: "invalid YAML is a syntax error", content: "name: [\n", wantSyntax: true},
746+
{name: "rejected uses expression is not a syntax error", content: "jobs:\n b:\n steps:\n - uses: ${{ x }}\n"},
747+
}
748+
for _, tt := range tests {
749+
t.Run(tt.name, func(t *testing.T) {
750+
_, err := Parse("wf.yml", []byte(tt.content))
751+
require.Error(t, err)
752+
var syntaxErr *SyntaxError
753+
assert.Equal(t, tt.wantSyntax, errors.As(err, &syntaxErr))
754+
})
755+
}
756+
}
757+
758+
func TestLoadReadErrorIsNotASyntaxError(t *testing.T) {
759+
_, err := Load(filepath.Join(t.TempDir(), "missing.yml"))
760+
require.Error(t, err)
761+
var syntaxErr *SyntaxError
762+
assert.False(t, errors.As(err, &syntaxErr))
763+
}

0 commit comments

Comments
 (0)