From 9a826085b2e31b027ead1efbabd7258afa5871d9 Mon Sep 17 00:00:00 2001 From: Charles Green Date: Sun, 2 Aug 2026 22:44:18 +0900 Subject: [PATCH 1/2] Fix this repository's own install, and find the caller by content v0.3.0 requires appName and changed the handle, and I updated the template without updating this repository's own install. Its config had no appName, so preflight failed, and its caller still triggered on the old prefix, so no comment could fire. The repository the agent actually works in was broken by the release. The drift check then found a flaw in itself. It looked for the caller at .github/workflows/simplycubed.yml, which is what init writes, but here that name belongs to the reusable workflow and the caller is simplycubed-caller.yml. It now finds the caller by the reusable-workflow call it makes, so a renamed caller is checked and a same-named non-caller is ignored. Only two repositories in the org have an install: this one and corp. An earlier scan reported all fifteen, which was wrong: gh api on a 404 returns a JSON body, so --jq '.sha' yielded the string "null" and every repo looked present. --- .github/simplycubed.yml | 5 ++++ .github/workflows/simplycubed-caller.yml | 2 +- cmd/simplycubed/main.go | 30 ++++++++++++++++---- cmd/simplycubed/main_test.go | 36 +++++++++++++++++++++++- 4 files changed, 66 insertions(+), 7 deletions(-) diff --git a/.github/simplycubed.yml b/.github/simplycubed.yml index 1a1b065..1b00d86 100644 --- a/.github/simplycubed.yml +++ b/.github/simplycubed.yml @@ -5,6 +5,11 @@ # decides whether a change is good. labelPrefix: sc +# The App this repository installed. Comment commands address it: +# "@simplycubed-code go" on an issue starts work. The caller workflow triggers +# on this same handle, and preflight fails if the two ever disagree. +appName: simplycubed-code + gate: make check prDescription: rich diff --git a/.github/workflows/simplycubed-caller.yml b/.github/workflows/simplycubed-caller.yml index 5f38526..52afe9f 100644 --- a/.github/workflows/simplycubed-caller.yml +++ b/.github/workflows/simplycubed-caller.yml @@ -60,7 +60,7 @@ jobs: if: >- github.event_name == 'issue_comment' && github.event.comment.user.type != 'Bot' && - startsWith(github.event.comment.body, '/simplycubed') && + startsWith(github.event.comment.body, '@simplycubed-code') && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) uses: simplycubed/code/.github/workflows/simplycubed.yml@main with: diff --git a/cmd/simplycubed/main.go b/cmd/simplycubed/main.go index 1c23a78..1a9a57b 100644 --- a/cmd/simplycubed/main.go +++ b/cmd/simplycubed/main.go @@ -459,18 +459,38 @@ func reply(argv []string, body string, stdout io.Writer) error { // way — comments simply stop working, with no error anywhere — so it is checked // on every run rather than left to be discovered. func checkMentionAgreement(repoDir, appName string) error { - path := filepath.Join(repoDir, ".github", "workflows", "simplycubed.yml") - b, err := os.ReadFile(path) + // Find the caller by what it does, not by what it is called. init writes + // .github/workflows/simplycubed.yml, but an adopter can rename it, and in + // this repository that name belongs to the reusable workflow itself. A check + // keyed on the filename would pass on the wrong file, or fail on a valid + // install, which is worse than not checking. + dir := filepath.Join(repoDir, ".github", "workflows") + entries, err := os.ReadDir(dir) if err != nil { - // No caller workflow is a valid local setup, not a misconfiguration. + // No workflows at all is a valid local setup, not a misconfiguration. return nil } want := "'" + command.MentionFor(appName) + "'" - if strings.Contains(string(b), want) { + var callers []string + for _, e := range entries { + if e.IsDir() { + continue + } + path := filepath.Join(dir, e.Name()) + b, err := os.ReadFile(path) + if err != nil || !strings.Contains(string(b), "simplycubed/code/.github/workflows/simplycubed.yml@") { + continue + } + callers = append(callers, path) + if strings.Contains(string(b), want) { + return nil + } + } + if len(callers) == 0 { return nil } return fmt.Errorf("%w: .github/simplycubed.yml sets appName %q, but %s does not trigger on %s. Comment commands will never fire. Re-run \"simplycubed init --workflow\" to rewrite the trigger from the config", - ErrConfigMissing, appName, path, want) + ErrConfigMissing, appName, strings.Join(callers, ", "), want) } func preflightCmd(argv []string, stdout io.Writer) error { diff --git a/cmd/simplycubed/main_test.go b/cmd/simplycubed/main_test.go index 3ea44ed..440a9f4 100644 --- a/cmd/simplycubed/main_test.go +++ b/cmd/simplycubed/main_test.go @@ -1290,7 +1290,11 @@ func TestPreflightCatchesHandleDrift(t *testing.T) { if err := os.MkdirAll(filepath.Dir(wf), 0o755); err != nil { t.Fatal(err) } - body := "on: [issue_comment]\njobs:\n comment:\n if: startsWith(github.event.comment.body, '" + trigger + "')\n" + // Must look like a caller: the check finds it by the reusable-workflow + // call, not by filename. + body := "on: [issue_comment]\njobs:\n comment:\n" + + " uses: simplycubed/code/.github/workflows/simplycubed.yml@v0.3.0\n" + + " if: startsWith(github.event.comment.body, '" + trigger + "')\n" if err := os.WriteFile(wf, []byte(body), 0o644); err != nil { t.Fatal(err) } @@ -1417,3 +1421,33 @@ func TestCommandCmdSurfacesConfigProblems(t *testing.T) { t.Fatal("--body with no value must be an error") } } + +// init writes the caller as simplycubed.yml, but an adopter can rename it, and +// in this repository that name belongs to the reusable workflow itself. The +// drift check has to find the caller by what it calls, or it passes on the +// wrong file and fails on a valid install. +func TestDriftCheckFindsTheCallerByContentNotFilename(t *testing.T) { + t.Setenv("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT", "https://r.openai.azure.com") + t.Setenv("SIMPLYCUBED_AZURE_OPENAI_API_KEY", "k") + + dir := repoWithConfigBody(t, "gate: make check\nappName: acme-code\n") + wfDir := filepath.Join(dir, ".github", "workflows") + if err := os.MkdirAll(wfDir, 0o755); err != nil { + t.Fatal(err) + } + // A workflow with the canonical name that is not a caller must be ignored. + if err := os.WriteFile(filepath.Join(wfDir, "simplycubed.yml"), []byte("on: workflow_call\njobs:\n run:\n runs-on: ubuntu-latest\n"), 0o644); err != nil { + t.Fatal(err) + } + // The real caller, under a different name. + caller := "on: [issue_comment]\njobs:\n comment:\n" + + " uses: simplycubed/code/.github/workflows/simplycubed.yml@v0.3.0\n" + + " if: startsWith(github.event.comment.body, '@acme-code')\n" + if err := os.WriteFile(filepath.Join(wfDir, "simplycubed-caller.yml"), []byte(caller), 0o644); err != nil { + t.Fatal(err) + } + + if err := preflightCmd([]string{"--repo-dir", dir}, io.Discard); err != nil { + t.Fatalf("a renamed caller that agrees must pass: %v", err) + } +} From 6ad4925ac1f9d77ad645c89018922bca2353263c Mon Sep 17 00:00:00 2001 From: Charles Green Date: Sun, 2 Aug 2026 22:46:58 +0900 Subject: [PATCH 2/2] Cover the paths the caller search can take Codecov flagged five lines, all branches the rewrite introduced and nothing exercised: a subdirectory in .github/workflows, an unrelated workflow encountered before the caller, and a repository with no caller at all. The middle one was hidden by filename order. The existing test wrote the caller as simplycubed-caller.yml and the non-caller as simplycubed.yml, so the caller was found first and the loop returned before it ever skipped anything. Naming them aa- and zz- makes the skip actually happen. Each of these is a check that would otherwise error on an ordinary repository layout, and a check that errors on a normal repo is one people turn off. --- cmd/simplycubed/main_test.go | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/cmd/simplycubed/main_test.go b/cmd/simplycubed/main_test.go index 440a9f4..15e7e99 100644 --- a/cmd/simplycubed/main_test.go +++ b/cmd/simplycubed/main_test.go @@ -1451,3 +1451,70 @@ func TestDriftCheckFindsTheCallerByContentNotFilename(t *testing.T) { t.Fatalf("a renamed caller that agrees must pass: %v", err) } } + +// The drift check walks .github/workflows/ looking for whatever calls the +// reusable workflow. Everything it can meet in there has to be handled, because +// a check that errors on an ordinary repository layout is a check people turn +// off. +func TestDriftCheckSkipsWhatIsNotACaller(t *testing.T) { + setAzure := func(t *testing.T) { + t.Helper() + t.Setenv("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT", "https://r.openai.azure.com") + t.Setenv("SIMPLYCUBED_AZURE_OPENAI_API_KEY", "k") + } + repoWithWorkflows := func(t *testing.T, files map[string]string, dirs ...string) string { + t.Helper() + dir := repoWithConfigBody(t, "gate: make check\nappName: acme-code\n") + wfDir := filepath.Join(dir, ".github", "workflows") + if err := os.MkdirAll(wfDir, 0o755); err != nil { + t.Fatal(err) + } + for _, d := range dirs { + if err := os.MkdirAll(filepath.Join(wfDir, d), 0o755); err != nil { + t.Fatal(err) + } + } + for name, body := range files { + if err := os.WriteFile(filepath.Join(wfDir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + return dir + } + caller := func(trigger string) string { + return "on: [issue_comment]\njobs:\n comment:\n" + + " uses: simplycubed/code/.github/workflows/simplycubed.yml@v0.3.0\n" + + " if: startsWith(github.event.comment.body, '" + trigger + "')\n" + } + plain := "on: push\njobs:\n build:\n runs-on: ubuntu-latest\n" + + t.Run("a directory in there is not a workflow", func(t *testing.T) { + setAzure(t) + dir := repoWithWorkflows(t, map[string]string{"zz-caller.yml": caller("@acme-code")}, "archive") + if err := preflightCmd([]string{"--repo-dir", dir}, io.Discard); err != nil { + t.Fatalf("a subdirectory must be skipped, not read: %v", err) + } + }) + + t.Run("unrelated workflows are skipped before the caller is found", func(t *testing.T) { + setAzure(t) + // "aa" sorts first, so the loop must skip it rather than stop there. + dir := repoWithWorkflows(t, map[string]string{ + "aa-ci.yml": plain, + "zz-caller.yml": caller("@acme-code"), + }) + if err := preflightCmd([]string{"--repo-dir", dir}, io.Discard); err != nil { + t.Fatalf("an unrelated workflow must not decide the answer: %v", err) + } + }) + + t.Run("no caller at all is a valid local setup", func(t *testing.T) { + setAzure(t) + // Running the CLI yourself needs no caller, so workflows that call + // nothing must not be reported as drift. + dir := repoWithWorkflows(t, map[string]string{"ci.yml": plain}) + if err := preflightCmd([]string{"--repo-dir", dir}, io.Discard); err != nil { + t.Fatalf("no caller is not a misconfiguration: %v", err) + } + }) +}