From 98713ae7308ab31e009b858639aff712da124f02 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 13 Jun 2026 14:47:32 -0400 Subject: [PATCH] fix: make e2e localizer uses-rewrite idempotent The harness reusable-workflow localizer prefixed any `uses:` value matching `[^/]...\.yaml` with `./`. Once the generator began emitting fully-qualified local callback paths (`uses: ./.github/workflows/build.yaml`), the leading `.` matched `[^/]` and the rewrite produced `uses: ././...`, which act cannot resolve. Tighten the capture group to a bare filename (no leading `.`, `/`, or `@` and no embedded `/`) so already-qualified local paths and cross-repo `owner/repo/...@ref` values are left unchanged while a bare `build.yaml` still gains its `./` prefix. Extract the two sed expressions to named constants and add an idempotency test that runs the real expression and asserts no `././` is ever produced. Signed-off-by: Joshua Temple --- e2e/harness/harness.go | 29 ++++++++++--- e2e/harness/localize_test.go | 84 ++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 e2e/harness/localize_test.go diff --git a/e2e/harness/harness.go b/e2e/harness/harness.go index 3631be27..2b053384 100644 --- a/e2e/harness/harness.go +++ b/e2e/harness/harness.go @@ -717,10 +717,14 @@ func (h *Harness) ensureCLIBinary(ctx context.Context) (string, error) { // localizeWorkflows rewrites generated workflows so external action refs // (`stablekernel/cascade/.github/actions/X@ref`) point at the local -// mock actions, and reusable-workflow refs (`uses: build.yaml`) gain the -// `./` prefix act needs for local resolution. Each pass is checked for -// success and verified by grepping for any remaining `stablekernel/...` ref; -// the operation retries on transient failure. +// mock actions. A second pass adds the `./` prefix act needs for any bare +// local reusable-workflow ref (`uses: build.yaml` -> `uses: ./build.yaml`). +// That pass is idempotent: it skips values that are already `./`-qualified, +// already a `.github/...` path, or a cross-repo `owner/repo/...@ref`, so an +// already-normalized `uses: ./.github/workflows/build.yaml` is left unchanged +// rather than gaining a second `./` prefix. Each pass is checked for success +// and verified by grepping for any remaining `stablekernel/...` ref; the +// operation retries on transient failure. // // The `.github/workflows/*.yaml` glob is intentionally broad: it covers every // generated workflow, including cascade-hotfix.yaml (whose plan/apply/finalize @@ -729,6 +733,19 @@ func (h *Harness) ensureCLIBinary(ctx context.Context) (string, error) { // GenerateWorkflows only appends a per-scenario suffix to the workflow-level // `name:` line, so workflow_files assertions over hotfix job/trigger content // remain stable as long as they do not assert on the top-level name line. +// actionLocalizeSedExpr rewrites external composite-action refs +// (`stablekernel/cascade/.github/actions/X@ref`) to the local mock path +// (`./.github/actions/X`). +const actionLocalizeSedExpr = `s|stablekernel/cascade/\.github/actions/\([^@]*\)@[^[:space:]]*|./.github/actions/\1|g` + +// usesLocalizeSedExpr prefixes a bare local reusable-workflow ref +// (`uses: build.yaml`) with `./` so act can resolve it. It is idempotent: the +// capture group requires a bare filename (no leading `.`, `/`, or `@`, and no +// embedded `/`), so an already-qualified `uses: ./.github/workflows/build.yaml` +// or a cross-repo `uses: owner/repo/...@ref` is left unchanged rather than +// gaining a second `./` prefix. +const usesLocalizeSedExpr = `s|uses: \([^./@][^/@]*\.yaml\)|uses: ./\1|g` + func (h *Harness) localizeWorkflows(ctx context.Context) error { const maxAttempts = 3 const retryDelay = 200 * time.Millisecond @@ -737,8 +754,8 @@ func (h *Harness) localizeWorkflows(ctx context.Context) error { "bash", "-c", "set -e; cd /tmp/repo && " + "for f in .github/workflows/*.yaml; do " + - " sed -i 's|stablekernel/cascade/\\.github/actions/\\([^@]*\\)@[^[:space:]]*|./.github/actions/\\1|g' \"$f\"; " + - " sed -i 's|uses: \\([^/][^@]*\\.yaml\\)|uses: ./\\1|g' \"$f\"; " + + " sed -i '" + actionLocalizeSedExpr + "' \"$f\"; " + + " sed -i '" + usesLocalizeSedExpr + "' \"$f\"; " + "done", } // grep -l exits 0 on match (= un-localized ref still present, which we diff --git a/e2e/harness/localize_test.go b/e2e/harness/localize_test.go new file mode 100644 index 00000000..a558cbc1 --- /dev/null +++ b/e2e/harness/localize_test.go @@ -0,0 +1,84 @@ +package harness + +import ( + "os/exec" + "strings" + "testing" +) + +// runSed pipes in through `sed ` and returns the transformed output. It +// exercises the exact expression the localizer runs inside the act container. +func runSed(t *testing.T, expr, in string) string { + t.Helper() + sed, err := exec.LookPath("sed") + if err != nil { + t.Skipf("sed not available: %v", err) + } + cmd := exec.Command(sed, expr) + cmd.Stdin = strings.NewReader(in) + out, err := cmd.Output() + if err != nil { + t.Fatalf("sed %q failed: %v", expr, err) + } + return string(out) +} + +// TestUsesLocalizeSedExpr_Idempotent_LeavesQualifiedPathsUnchanged verifies the +// reusable-workflow localizer prefixes a bare ref with `./` but never adds a +// second `./` to an already-qualified path or mangles a cross-repo `@ref`. +// +// Regression guard: before the generator emitted fully-qualified local callback +// paths, the localizer turned a bare `uses: build.yaml` into `uses: ./build.yaml`. +// Once the generator started emitting `uses: ./.github/workflows/build.yaml`, the +// old expression matched the leading `.` and produced `uses: ././...`, which act +// could not resolve. +func TestUsesLocalizeSedExpr_Idempotent_LeavesQualifiedPathsUnchanged(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + { + name: "already qualified local path is unchanged", + in: " uses: ./.github/workflows/build.yaml\n", + want: " uses: ./.github/workflows/build.yaml\n", + }, + { + name: "qualified reusable deploy path is unchanged", + in: " uses: ./.github/workflows/deploy-infra.yaml\n", + want: " uses: ./.github/workflows/deploy-infra.yaml\n", + }, + { + name: "bare filename gains a single ./ prefix", + in: " uses: build.yaml\n", + want: " uses: ./build.yaml\n", + }, + { + name: "cross-repo ref with @ is left unchanged", + in: " uses: owner/repo/.github/workflows/y.yaml@main\n", + want: " uses: owner/repo/.github/workflows/y.yaml@main\n", + }, + { + name: "stablekernel cross-repo ref is left unchanged", + in: " uses: stablekernel/cascade/.github/workflows/x.yaml@v1\n", + want: " uses: stablekernel/cascade/.github/workflows/x.yaml@v1\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := runSed(t, usesLocalizeSedExpr, tt.in) + if got != tt.want { + t.Errorf("single pass:\n in: %q\n got: %q\n want: %q", tt.in, got, tt.want) + } + // Idempotency: a second pass over the output must be a no-op. + again := runSed(t, usesLocalizeSedExpr, got) + if again != got { + t.Errorf("second pass changed output (not idempotent):\n first: %q\n second: %q", got, again) + } + if strings.Contains(again, "././") { + t.Errorf("produced a double ./ prefix: %q", again) + } + }) + } +}