From 41bb3f6798e1a9ca9836c3d696f1813ae53961be Mon Sep 17 00:00:00 2001 From: Charles Green Date: Sun, 2 Aug 2026 17:26:09 +0900 Subject: [PATCH] preflight checks the four adopter-set values and names their section Nothing checked these until a real run failed, and the one thing that did check was deleted at the end of install: the self-test names the section on a miss, then init tells the adopter to delete it. Two of the four live under Variables and two under Secrets, and a value filed on the wrong tab reads back empty rather than erroring. That is not hypothetical; it happened during a real install and was found by reading the caller workflow. A shared requireSet gives every message the same shape and names the tab. Whitespace counts as empty, since that is what a wrong-tab value looks like from here. The App pair is only checked with --actions, which the reusable workflow now passes. A local run authenticates as the operator and never uses them, so requiring them there would fail a working setup. A configuration miss exits 3. It is something to go and set, not a bug to report, and a caller should not have to match on message text to tell. Closes #115 --- .github/workflows/simplycubed.yml | 8 +++- cmd/simplycubed/main.go | 61 +++++++++++++++++++++++++--- cmd/simplycubed/main_test.go | 66 +++++++++++++++++++++++++++++++ docs/setup.md | 15 +++++++ 4 files changed, 143 insertions(+), 7 deletions(-) diff --git a/.github/workflows/simplycubed.yml b/.github/workflows/simplycubed.yml index 73fb430..8224f24 100644 --- a/.github/workflows/simplycubed.yml +++ b/.github/workflows/simplycubed.yml @@ -100,7 +100,9 @@ jobs: env: SIMPLYCUBED_AZURE_OPENAI_ENDPOINT: ${{ inputs['azure-openai-endpoint'] }} SIMPLYCUBED_AZURE_OPENAI_API_KEY: ${{ secrets['azure-openai-api-key'] }} - run: simplycubed preflight --repo-dir . + SIMPLYCUBED_GH_APP_CLIENT_ID: ${{ inputs['github-app-client-id'] }} + SIMPLYCUBED_GH_APP_PRIVATE_KEY: ${{ secrets['github-app-private-key'] }} + run: simplycubed preflight --repo-dir . --actions - name: Let the engine sandbox start # The engine sandboxes itself with a vendored bubblewrap, and bubblewrap @@ -188,7 +190,9 @@ jobs: env: SIMPLYCUBED_AZURE_OPENAI_ENDPOINT: ${{ inputs['azure-openai-endpoint'] }} SIMPLYCUBED_AZURE_OPENAI_API_KEY: ${{ secrets['azure-openai-api-key'] }} - run: simplycubed preflight --repo-dir . + SIMPLYCUBED_GH_APP_CLIENT_ID: ${{ inputs['github-app-client-id'] }} + SIMPLYCUBED_GH_APP_PRIVATE_KEY: ${{ secrets['github-app-private-key'] }} + run: simplycubed preflight --repo-dir . --actions - name: Let the engine sandbox start # The engine sandboxes itself with a vendored bubblewrap, and bubblewrap diff --git a/cmd/simplycubed/main.go b/cmd/simplycubed/main.go index 26c84cc..c2e4ed4 100644 --- a/cmd/simplycubed/main.go +++ b/cmd/simplycubed/main.go @@ -11,6 +11,7 @@ import ( "context" _ "embed" "encoding/json" + "errors" "flag" "fmt" "io" @@ -52,6 +53,12 @@ func dispatch(args []string, stdout, stderr io.Writer) int { } fail := func(err error) int { fmt.Fprintln(stderr, "error:", err) + // A missing adopter-set value is something to go and set, not a bug to + // report. Its own exit code lets a caller tell those apart without + // matching on message text. + if errors.Is(err, ErrConfigMissing) { + return 3 + } return 1 } switch args[0] { @@ -149,10 +156,10 @@ func engineEnv(cfg *config.Config) (string, error) { if cfg != nil && cfg.Engine == "claude" { return "", nil } - endpoint := strings.TrimRight(os.Getenv("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT"), "/") - if endpoint == "" { - return "", fmt.Errorf("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT is not set. It is a repository variable on your own repository; a reusable workflow never inherits variables from SimplyCubed") + if err := requireSet("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT", sectionVariable); err != nil { + return "", err } + endpoint := strings.TrimRight(os.Getenv("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT"), "/") u, err := url.Parse(endpoint) if err != nil { return "", fmt.Errorf("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT is not a valid URL: %w", err) @@ -160,8 +167,8 @@ func engineEnv(cfg *config.Config) (string, error) { if u.Scheme != "https" || u.Host == "" { return "", fmt.Errorf("SIMPLYCUBED_AZURE_OPENAI_ENDPOINT must be an https URL like https://.openai.azure.com, got %q", endpoint) } - if os.Getenv("SIMPLYCUBED_AZURE_OPENAI_API_KEY") == "" { - return "", fmt.Errorf("SIMPLYCUBED_AZURE_OPENAI_API_KEY is not set. It is a repository secret on your own repository; a reusable workflow never inherits secrets from SimplyCubed") + if err := requireSet("SIMPLYCUBED_AZURE_OPENAI_API_KEY", sectionSecret); err != nil { + return "", err } return endpoint, nil } @@ -171,6 +178,33 @@ func engineEnv(cfg *config.Config) (string, error) { // identity unknown" after the change is already made and the gate has passed. // The identity is the credential's own login, so commits are attributable to // whoever the run authenticated as. +// ErrConfigMissing marks a missing adopter-set value, as opposed to an internal +// failure. The two want different responses: one is something to go and set, +// the other is a bug. dispatch turns this into its own exit code so a caller +// can tell them apart without parsing text. +var ErrConfigMissing = errors.New("configuration missing") + +// section is where a value lives in the adopter's repository settings. GitHub +// puts Variables and Secrets on different tabs, and a value filed under the +// wrong one reads back as empty rather than failing, so every message about a +// missing value has to say which tab it belongs on. +type section string + +const ( + sectionVariable section = "variable" + sectionSecret section = "secret" +) + +// requireSet returns an error naming the value and its section when unset or +// empty. Empty matters as much as unset: an empty string is exactly what a +// value filed under the wrong tab looks like from here. +func requireSet(name string, where section) error { + if strings.TrimSpace(os.Getenv(name)) != "" { + return nil + } + return fmt.Errorf("%w: %s is not set. It is a repository %s on your own repository, under Settings > Secrets and variables > Actions; a reusable workflow never inherits %ss from SimplyCubed", ErrConfigMissing, name, where, where) +} + func newVCS(self string) *vcsgit.Git { if self == "" { return &vcsgit.Git{} @@ -376,6 +410,7 @@ func reply(argv []string, body string, stdout io.Writer) error { func preflightCmd(argv []string, stdout io.Writer) error { fs := flag.NewFlagSet("preflight", flag.ContinueOnError) repoDir := fs.String("repo-dir", ".", "path to the target repo checkout") + actions := fs.Bool("actions", false, "also check the values only a caller workflow supplies") if _, err := parseInterleaved(fs, argv); err != nil { return err } @@ -386,6 +421,22 @@ func preflightCmd(argv []string, stdout io.Writer) error { if _, err := engineEnv(cfg); err != nil { return err } + // The App credentials are only reachable when the caller workflow exports + // them. A local run authenticates as the operator and never uses them, so + // checking them there would fail a working setup. + if *actions { + for _, v := range []struct { + name string + where section + }{ + {"SIMPLYCUBED_GH_APP_CLIENT_ID", sectionVariable}, + {"SIMPLYCUBED_GH_APP_PRIVATE_KEY", sectionSecret}, + } { + if err := requireSet(v.name, v.where); err != nil { + return err + } + } + } fmt.Fprintln(stdout, "preflight ok: config and engine settings are present") return nil } diff --git a/cmd/simplycubed/main_test.go b/cmd/simplycubed/main_test.go index ab100e4..6fae05a 100644 --- a/cmd/simplycubed/main_test.go +++ b/cmd/simplycubed/main_test.go @@ -1194,3 +1194,69 @@ func TestWorkflowRestrictedPushCoversAnyBotIdentity(t *testing.T) { } }) } + +// Two of the four adopter-set values live under Variables and two under +// Secrets. A value filed on the wrong tab reads back empty rather than +// erroring, so preflight has to catch it and say which tab it belongs on. +func TestPreflightChecksTheAdopterSetValues(t *testing.T) { + setAll := 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") + t.Setenv("SIMPLYCUBED_GH_APP_CLIENT_ID", "Iv23example") + t.Setenv("SIMPLYCUBED_GH_APP_PRIVATE_KEY", "-----BEGIN...") + } + + for _, tc := range []struct { + name, unset, wantSection string + }{ + {"endpoint", "SIMPLYCUBED_AZURE_OPENAI_ENDPOINT", "repository variable"}, + {"api key", "SIMPLYCUBED_AZURE_OPENAI_API_KEY", "repository secret"}, + {"client id", "SIMPLYCUBED_GH_APP_CLIENT_ID", "repository variable"}, + {"private key", "SIMPLYCUBED_GH_APP_PRIVATE_KEY", "repository secret"}, + } { + t.Run(tc.name+" missing names its section", func(t *testing.T) { + setAll(t) + t.Setenv(tc.unset, "") + err := preflightCmd([]string{"--repo-dir", repoWithConfig(t), "--actions"}, io.Discard) + if err == nil { + t.Fatalf("%s unset must fail preflight", tc.unset) + } + if !errors.Is(err, ErrConfigMissing) { + t.Fatalf("err = %v, want it to classify as a configuration miss", err) + } + for _, want := range []string{tc.unset, tc.wantSection} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("err = %q, expected it to mention %q", err, want) + } + } + }) + } + + t.Run("a value filed on the wrong tab looks empty and fails the same way", func(t *testing.T) { + setAll(t) + t.Setenv("SIMPLYCUBED_GH_APP_CLIENT_ID", " ") + err := preflightCmd([]string{"--repo-dir", repoWithConfig(t), "--actions"}, io.Discard) + if !errors.Is(err, ErrConfigMissing) { + t.Fatalf("err = %v, want a whitespace-only value treated as unset", err) + } + }) + + t.Run("a local run does not require the App pair", func(t *testing.T) { + setAll(t) + t.Setenv("SIMPLYCUBED_GH_APP_CLIENT_ID", "") + t.Setenv("SIMPLYCUBED_GH_APP_PRIVATE_KEY", "") + if err := preflightCmd([]string{"--repo-dir", repoWithConfig(t)}, io.Discard); err != nil { + t.Fatalf("a local run authenticates as the operator and never uses the App: %v", err) + } + }) + + t.Run("a configuration miss exits 3, not 1", func(t *testing.T) { + setAll(t) + t.Setenv("SIMPLYCUBED_AZURE_OPENAI_API_KEY", "") + var out, errOut bytes.Buffer + if code := dispatch([]string{"preflight", "--repo-dir", repoWithConfig(t)}, &out, &errOut); code != 3 { + t.Fatalf("exit = %d, want 3 so a caller can tell a missing value from a bug", code) + } + }) +} diff --git a/docs/setup.md b/docs/setup.md index 1c44885..4fba31f 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -258,6 +258,21 @@ That validates the repository config and the engine settings and exits. It is what the workflow runs before installing the rest of the toolchain, so a misconfigured repository finds out in seconds. +It names the section a missing value belongs in, because Variables and Secrets +are different tabs and a value filed under the wrong one reads back as empty +rather than failing: + +```text +error: configuration missing: SIMPLYCUBED_AZURE_OPENAI_ENDPOINT is not set. It is a +repository variable on your own repository, under Settings > Secrets and variables > +Actions; a reusable workflow never inherits variables from SimplyCubed +``` + +A missing value exits **3**, so a caller can tell "go and set this" apart from a +bug without matching on message text. In Actions the workflow passes `--actions`, +which also checks the App credentials; a local run authenticates as you and never +uses them. + ```sh simplycubed run owner/repo#N --dry-run ```