diff --git a/docs/image-automation.md b/docs/image-automation.md index f8ed64c..c4328de 100644 --- a/docs/image-automation.md +++ b/docs/image-automation.md @@ -71,6 +71,34 @@ Enable it on the daemon with `ROLLOPS_IMAGE_AUTOMATION=1`. Private registries: `ROLLOPS_REGISTRY_USER` / `ROLLOPS_REGISTRY_TOKEN`. Writeback needs a token with push access on the config repo (see `docs/deploy-kubernetes.md`). +## Writeback to a protected branch + +By default the bump is **committed and pushed directly** to the tracked branch. +That fails on a **protected branch** — one requiring pull requests or status +checks — because rollopsd's direct push is rejected (`GH006: Changes must be +made through a pull request`). The symptom is quiet: the config shows `=error` +in the reconcile summary and the target simply never updates. + +Set `imagePolicy.writeback: pull-request` for those repos: + +```yaml +imagePolicy: + mode: digest + allowMutableTags: true + writeback: pull-request # default: push +``` + +In this mode rollopsd never writes the tracked branch. It pushes the bump to a +deterministic branch (`rollops/image/`) and opens a PR into the +tracked branch, enabling GitHub **auto-merge** so the change lands the moment +its required checks pass. The deploy happens through the ordinary reconcile once +the PR merges and the tracked branch advances — the cluster never leads Git. + +The token needs `pull-requests: write` (and `contents: write` for the branch +push). If the repository has auto-merge disabled, the PR is still opened and +waits for a human to merge it — writeback is unblocked either way. GitHub is the +only forge supported for pull-request writeback today. + ## Commit-SHA and mutable tags Not every pipeline publishes semver. Common schemes and the mode to use: diff --git a/docs/keel-migration.md b/docs/keel-migration.md index 0ece7fd..4879a39 100644 --- a/docs/keel-migration.md +++ b/docs/keel-migration.md @@ -41,6 +41,13 @@ Enable image automation: `ROLLOPS_IMAGE_AUTOMATION=1` plus `ROLLOPS_REGISTRY_USER` / `ROLLOPS_REGISTRY_TOKEN` for private registries. The git token needs Contents **read+write** (automation pushes bumps). +> **Protected `main`?** If the config repo protects its branch (PRs / status +> checks required), keel's direct push has no equivalent — rollopsd's bump push +> is rejected (`GH006`) and the target silently stops updating. Set +> `imagePolicy.writeback: pull-request` on those configs so rollopsd opens an +> auto-merging PR instead of pushing; give the token `pull-requests: write` too. +> See `docs/image-automation.md` → *Writeback to a protected branch*. + ```sh kubectl -n rollops-system create secret generic rollopsd-git --from-literal=token= kubectl -n rollops-system create secret generic rollopsd-registry --from-literal=user= --from-literal=token= diff --git a/internal/config/config.go b/internal/config/config.go index 66e8893..e221c11 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -136,6 +136,31 @@ type ImagePolicy struct { // the list — a supply-chain guard so a compromised or mistyped image ref can't // pull automation toward an unexpected registry. AllowedRegistries []string `yaml:"allowedRegistries,omitempty" json:"allowedRegistries,omitempty"` + // Writeback selects how a bumped image reaches the tracked branch: + // - "push" (default) commits the bump directly onto the branch. Fast, but + // it needs the rollops identity to be allowed to push, which a protected + // branch forbids — the push is rejected and the deploy never happens. + // - "pull-request" pushes the bump to a fresh branch and opens a PR into + // the tracked branch (enabling GitHub auto-merge when the repo allows + // it). rollops never writes the branch directly, so branch protection is + // honoured; the deploy happens later, when the PR merges and the tracked + // branch advances through the normal reconcile. + Writeback string `yaml:"writeback,omitempty" json:"writeback,omitempty"` // push (default) | pull-request +} + +// Image-automation writeback modes. +const ( + WritebackPush = "push" // commit the bump directly onto the tracked branch (default) + WritebackPullRequest = "pull-request" // open a PR instead — for protected branches +) + +// WritebackMode returns the effective writeback mode, defaulting to +// WritebackPush when unset so existing configs are unchanged. +func (p *ImagePolicy) WritebackMode() string { + if p != nil && p.Writeback == WritebackPullRequest { + return WritebackPullRequest + } + return WritebackPush } // Target selects the deployment target plugin and its criticality weight. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 880cb21..c160c30 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -249,3 +249,19 @@ func TestSchemaJSON_Published(t *testing.T) { t.Errorf("embedded schema should reference the schema version %q", SchemaVersion) } } + +func TestImagePolicy_WritebackMode(t *testing.T) { + var nilPol *ImagePolicy + if nilPol.WritebackMode() != WritebackPush { + t.Error("nil policy must default to push") + } + if (&ImagePolicy{}).WritebackMode() != WritebackPush { + t.Error("unset writeback must default to push") + } + if (&ImagePolicy{Writeback: "pull-request"}).WritebackMode() != WritebackPullRequest { + t.Error("explicit pull-request not honoured") + } + if (&ImagePolicy{Writeback: "bogus"}).WritebackMode() != WritebackPush { + t.Error("an unrecognised value must fall back to push, not silently disable writeback") + } +} diff --git a/internal/config/schema/rollops.v1.schema.json b/internal/config/schema/rollops.v1.schema.json index c485ccf..8fe1ae7 100644 --- a/internal/config/schema/rollops.v1.schema.json +++ b/internal/config/schema/rollops.v1.schema.json @@ -285,7 +285,8 @@ "mode": { "type": "string", "enum": ["none", "major", "minor", "patch", "any", "digest"], "description": "Which version increments to auto-adopt. 'none' disables automation (the Git-committed image is authoritative)." }, "pattern": { "type": "string", "description": "Optional regexp filter on candidate tags." }, "allowMutableTags": { "type": "boolean", "description": "Permit latest/main/master." }, - "allowedRegistries": { "type": "array", "items": { "type": "string", "minLength": 1 }, "description": "Restrict image automation to these registry hosts (e.g. ghcr.io, docker.io). Empty means no restriction." } + "allowedRegistries": { "type": "array", "items": { "type": "string", "minLength": 1 }, "description": "Restrict image automation to these registry hosts (e.g. ghcr.io, docker.io). Empty means no restriction." }, + "writeback": { "type": "string", "enum": ["push", "pull-request"], "description": "How a bumped image reaches the tracked branch. 'push' (default) commits directly — requires push rights the branch may forbid. 'pull-request' pushes the bump to a branch and opens a PR (enabling auto-merge when the repo allows it), so a protected branch is respected: the deploy happens when the PR merges." } } }, "trafficRouting": { diff --git a/internal/git/pullrequest.go b/internal/git/pullrequest.go new file mode 100644 index 0000000..e131a9d --- /dev/null +++ b/internal/git/pullrequest.go @@ -0,0 +1,184 @@ +package git + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" +) + +// pull-request writeback. +// +// When image automation cannot push to the tracked branch — a protected branch +// requiring reviews or status checks — it proposes the bump as a pull request +// instead. rollops opens (or refreshes) the PR and, when the repository permits +// it, enables GitHub's native auto-merge so the change lands the moment its +// required checks pass. The deploy still happens through the ordinary reconcile +// once the PR merges and the tracked branch advances; nothing here deploys. + +// OpenPullRequest ensures a PR proposes headBranch be merged into base, with the +// given title and body. It is idempotent: an existing open PR for headBranch is +// reused rather than duplicated. When the repository allows auto-merge, it is +// enabled so the PR lands automatically once required checks pass. +// +// Returns the PR's URL and whether auto-merge was enabled. Auto-merge failing to +// enable is not an error — the PR is open and can be merged by a human — so a +// repo without auto-merge still gets unblocked, just not hands-free. +func (s *Source) OpenPullRequest(ctx context.Context, headBranch, base, title, body string) (prURL string, autoMerge bool, err error) { + owner, repo, err := ownerRepoFromURL(s.url) + if err != nil { + return "", false, err + } + nodeID, url, err := s.createOrFindPR(ctx, owner, repo, headBranch, base, title, body) + if err != nil { + return "", false, err + } + if merr := s.enableAutoMerge(ctx, nodeID); merr == nil { + autoMerge = true + } + return url, autoMerge, nil +} + +// createOrFindPR opens a PR and returns its node id and URL. A 422 whose body +// names an existing PR is treated as success — GitHub returns it when a PR for +// the same head already exists — and the open PR is looked up and reused. +func (s *Source) createOrFindPR(ctx context.Context, owner, repo, head, base, title, body string) (nodeID, url string, err error) { + payload, _ := json.Marshal(map[string]string{"title": title, "head": head, "base": base, "body": body}) + status, respBody, err := s.githubAPI(ctx, http.MethodPost, + fmt.Sprintf("/repos/%s/%s/pulls", owner, repo), payload) + if err != nil { + return "", "", err + } + if status == http.StatusCreated { + var pr struct { + NodeID string `json:"node_id"` + HTMLURL string `json:"html_url"` + } + if uerr := json.Unmarshal(respBody, &pr); uerr != nil { + return "", "", fmt.Errorf("git: parse created PR: %w", uerr) + } + return pr.NodeID, pr.HTMLURL, nil + } + // Already exists → find and reuse it. GitHub returns 422 for a duplicate head. + if status == http.StatusUnprocessableEntity && bytes.Contains(bytes.ToLower(respBody), []byte("already exist")) { + return s.findOpenPR(ctx, owner, repo, head) + } + return "", "", fmt.Errorf("git: open PR %s/%s (%s→%s): status %d: %s", + owner, repo, head, base, status, bytes.TrimSpace(respBody)) +} + +// findOpenPR resolves the open PR whose head is headBranch. +func (s *Source) findOpenPR(ctx context.Context, owner, repo, head string) (nodeID, url string, err error) { + status, respBody, err := s.githubAPI(ctx, http.MethodGet, + fmt.Sprintf("/repos/%s/%s/pulls?state=open&head=%s:%s", owner, repo, owner, head), nil) + if err != nil { + return "", "", err + } + if status != http.StatusOK { + return "", "", fmt.Errorf("git: list PRs for %s: status %d: %s", head, status, bytes.TrimSpace(respBody)) + } + var prs []struct { + NodeID string `json:"node_id"` + HTMLURL string `json:"html_url"` + } + if uerr := json.Unmarshal(respBody, &prs); uerr != nil { + return "", "", fmt.Errorf("git: parse PR list: %w", uerr) + } + if len(prs) == 0 { + return "", "", fmt.Errorf("git: PR for head %s reported existing but none found open", head) + } + return prs[0].NodeID, prs[0].HTMLURL, nil +} + +// enableAutoMerge turns on GitHub's native auto-merge for the PR via GraphQL, so +// it merges itself once required checks pass. It fails (returns an error the +// caller tolerates) when the repository has auto-merge disabled or the token +// lacks permission — in which case the PR simply waits for a manual merge. +func (s *Source) enableAutoMerge(ctx context.Context, prNodeID string) error { + if prNodeID == "" { + return fmt.Errorf("git: auto-merge: empty PR node id") + } + query := map[string]any{ + "query": `mutation($id:ID!){enablePullRequestAutoMerge(input:{pullRequestId:$id,mergeMethod:SQUASH}){clientMutationId}}`, + "variables": map[string]string{"id": prNodeID}, + } + payload, _ := json.Marshal(query) + status, respBody, err := s.githubAPI(ctx, http.MethodPost, "/graphql", payload) + if err != nil { + return err + } + // GraphQL returns 200 even for logical errors, reported in an "errors" array. + if status != http.StatusOK || bytes.Contains(respBody, []byte(`"errors"`)) { + return fmt.Errorf("git: enable auto-merge: status %d: %s", status, bytes.TrimSpace(respBody)) + } + return nil +} + +// githubAPI performs one authenticated GitHub API call and returns the status +// and body. The path is either REST (/repos/...) or "/graphql"; both hang off +// the same host. The installation/PAT token is resolved per call so a rotating +// credential is honoured, and is never written to disk or logged. +func (s *Source) githubAPI(ctx context.Context, method, path string, body []byte) (int, []byte, error) { + token, err := s.auth.token(ctx) + if err != nil { + return 0, nil, fmt.Errorf("git: resolve token: %w", err) + } + base := s.apiBase + if base == "" { + base = "https://api.github.com" + } + var rdr io.Reader + if body != nil { + rdr = bytes.NewReader(body) + } + req, err := http.NewRequestWithContext(ctx, method, base+path, rdr) + if err != nil { + return 0, nil, err + } + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return 0, nil, fmt.Errorf("git: github api %s %s: %w", method, path, err) + } + defer func() { _ = resp.Body.Close() }() + out, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + return resp.StatusCode, out, nil +} + +// ownerRepoFromURL parses "owner" and "repo" out of a GitHub remote URL in +// https, ssh, or git form. It is deliberately strict: pull-request writeback +// only targets GitHub, and anything else should fail loudly rather than post to +// the wrong place. +func ownerRepoFromURL(url string) (owner, repo string, err error) { + u := strings.TrimSpace(url) + u = strings.TrimSuffix(u, ".git") + switch { + case strings.HasPrefix(u, "git@"): // git@github.com:owner/repo + if i := strings.Index(u, ":"); i >= 0 { + u = u[i+1:] + } + case strings.Contains(u, "://"): // https://github.com/owner/repo, ssh://git@github.com/owner/repo + if i := strings.Index(u, "://"); i >= 0 { + u = u[i+3:] + } + if i := strings.Index(u, "/"); i >= 0 { + u = u[i+1:] // drop host + } + u = strings.TrimPrefix(u, "git@") + } + parts := strings.Split(strings.Trim(u, "/"), "/") + if len(parts) < 2 || parts[len(parts)-2] == "" || parts[len(parts)-1] == "" { + return "", "", fmt.Errorf("git: cannot parse owner/repo from URL %q", url) + } + return parts[len(parts)-2], parts[len(parts)-1], nil +} diff --git a/internal/git/pullrequest_test.go b/internal/git/pullrequest_test.go new file mode 100644 index 0000000..a51158d --- /dev/null +++ b/internal/git/pullrequest_test.go @@ -0,0 +1,155 @@ +package git + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestOwnerRepoFromURL(t *testing.T) { + for _, tc := range []struct { + url, owner, repo string + wantErr bool + }{ + {"https://github.com/klarlabs-studio/klarlabs", "klarlabs-studio", "klarlabs", false}, + {"https://github.com/klarlabs-studio/klarlabs.git", "klarlabs-studio", "klarlabs", false}, + {"git@github.com:klarlabs-studio/klarlabs.git", "klarlabs-studio", "klarlabs", false}, + {"ssh://git@github.com/acme/web", "acme", "web", false}, + {"https://github.com/acme/web/", "acme", "web", false}, + {"https://github.com/", "", "", true}, + {"not-a-url", "", "", true}, + } { + owner, repo, err := ownerRepoFromURL(tc.url) + if (err != nil) != tc.wantErr { + t.Errorf("%q: err=%v wantErr=%v", tc.url, err, tc.wantErr) + continue + } + if !tc.wantErr && (owner != tc.owner || repo != tc.repo) { + t.Errorf("%q: got %s/%s want %s/%s", tc.url, owner, repo, tc.owner, tc.repo) + } + } +} + +// sourceFor builds a Source pointed at a stub GitHub API with a token set. +func sourceFor(t *testing.T, apiBase string) *Source { + t.Helper() + return Open(t.TempDir(), "main", Auth{Token: "test-token"}). + WithURL("https://github.com/acme/web"). + WithAPIBase(apiBase) +} + +func TestOpenPullRequest_CreatesAndEnablesAutoMerge(t *testing.T) { + var createBody map[string]string + var sawGraphQL bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer test-token" { + t.Errorf("missing/wrong auth header: %q", r.Header.Get("Authorization")) + } + switch { + case r.Method == http.MethodPost && r.URL.Path == "/repos/acme/web/pulls": + b, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(b, &createBody) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"node_id":"PR_node_1","html_url":"https://github.com/acme/web/pull/7"}`)) + case r.Method == http.MethodPost && r.URL.Path == "/graphql": + sawGraphQL = true + _, _ = w.Write([]byte(`{"data":{"enablePullRequestAutoMerge":{"clientMutationId":null}}}`)) + default: + t.Errorf("unexpected call: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + url, autoMerge, err := sourceFor(t, srv.URL).OpenPullRequest( + context.Background(), "rollops/image/web", "main", "chore(image): bump", "body") + if err != nil { + t.Fatalf("OpenPullRequest: %v", err) + } + if url != "https://github.com/acme/web/pull/7" { + t.Fatalf("PR url = %q", url) + } + if !autoMerge { + t.Fatal("auto-merge should be enabled when the mutation succeeds") + } + if !sawGraphQL { + t.Fatal("auto-merge mutation was never attempted") + } + if createBody["head"] != "rollops/image/web" || createBody["base"] != "main" { + t.Fatalf("wrong PR head/base: %+v", createBody) + } +} + +// A repo without auto-merge must still get its PR opened — the feature degrades +// to "open, wait for a human", never to an error that leaves the deploy stuck. +func TestOpenPullRequest_AutoMergeDisabledStillOpens(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/graphql" { + // GraphQL reports logical failure with 200 + an errors array. + _, _ = w.Write([]byte(`{"errors":[{"message":"Auto-merge is not allowed for this repository"}]}`)) + return + } + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"node_id":"n","html_url":"https://github.com/acme/web/pull/8"}`)) + })) + defer srv.Close() + + url, autoMerge, err := sourceFor(t, srv.URL).OpenPullRequest( + context.Background(), "rollops/image/web", "main", "t", "b") + if err != nil { + t.Fatalf("a disabled auto-merge must not be an error: %v", err) + } + if autoMerge { + t.Fatal("auto-merge should report false when the repo forbids it") + } + if url == "" { + t.Fatal("PR should still be opened") + } +} + +// An existing PR for the same head (GitHub answers create with 422) is reused, +// not duplicated — so re-running each poll cycle is idempotent. +func TestOpenPullRequest_ReusesExisting(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/repos/acme/web/pulls": + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message":"Validation Failed","errors":[{"message":"A pull request already exists for acme:rollops/image/web."}]}`)) + case r.Method == http.MethodGet && r.URL.Path == "/repos/acme/web/pulls": + _, _ = w.Write([]byte(`[{"node_id":"existing","html_url":"https://github.com/acme/web/pull/3"}]`)) + case r.URL.Path == "/graphql": + _, _ = w.Write([]byte(`{"data":{}}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + url, _, err := sourceFor(t, srv.URL).OpenPullRequest( + context.Background(), "rollops/image/web", "main", "t", "b") + if err != nil { + t.Fatalf("reuse path errored: %v", err) + } + if !strings.HasSuffix(url, "/pull/3") { + t.Fatalf("should reuse the existing PR, got %q", url) + } +} + +// A non-2xx that is not the known "already exists" 422 must surface as an error, +// not be swallowed — otherwise a real failure looks like a successful open. +func TestOpenPullRequest_HardFailureIsAnError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Resource not accessible by integration"}`)) + })) + defer srv.Close() + + if _, _, err := sourceFor(t, srv.URL).OpenPullRequest( + context.Background(), "h", "main", "t", "b"); err == nil { + t.Fatal("a 403 must be an error") + } +} diff --git a/internal/git/repo.go b/internal/git/repo.go index cee82a7..a00b4c0 100644 --- a/internal/git/repo.go +++ b/internal/git/repo.go @@ -43,12 +43,16 @@ type Source struct { dir string branch string auth Auth + url string // remote URL, retained for the pull-request API (owner/repo) + // apiBase is the GitHub REST/GraphQL host, overridable in tests. Empty means + // the public default (https://api.github.com). + apiBase string } // Clone checks out url@branch into dir. Each watched repo gets its own Source — // isolation is a property of Git structure. func Clone(ctx context.Context, url, branch, dir string, auth Auth) (*Source, error) { - s := &Source{dir: dir, branch: branch, auth: auth} + s := &Source{dir: dir, branch: branch, auth: auth, url: url} args := []string{"clone", "--depth", "1", "--branch", branch, url, dir} if _, err := s.git(ctx, "", args...); err != nil { return nil, fmt.Errorf("git: clone %s@%s: %w", url, branch, err) @@ -61,6 +65,16 @@ func Open(dir, branch string, auth Auth) *Source { return &Source{dir: dir, branch: branch, auth: auth} } +// WithURL sets the remote URL on a Source built by Open (Clone sets it already). +// It is needed for pull-request writeback, which derives owner/repo from the URL. +func (s *Source) WithURL(url string) *Source { s.url = url; return s } + +// WithAPIBase overrides the GitHub API host (tests point it at a stub server). +func (s *Source) WithAPIBase(base string) *Source { s.apiBase = base; return s } + +// Branch is the tracked branch this Source follows. +func (s *Source) Branch() string { return s.branch } + // Dir is the working-tree path where the config is read from. func (s *Source) Dir() string { return s.dir } @@ -169,6 +183,43 @@ func (s *Source) Push(ctx context.Context) error { return err } +// CommitFileOnBranch creates (or resets) headBranch off the tracked branch, +// writes one file, and commits it there — the pull-request writeback path, +// which must never touch the tracked branch itself. Returns whether a commit +// was made (false when content matches what the branch already has). +// +// The branch is force-created from the tracked branch each call (checkout -B), +// so a stale head from a previous cycle is refreshed to the current base rather +// than accumulating; the head is deterministic per bump, so re-running updates +// the same proposed change instead of spawning new ones. +func (s *Source) CommitFileOnBranch(ctx context.Context, headBranch, relPath string, content []byte, message string) (bool, error) { + if headBranch == "" || headBranch == s.branch { + return false, fmt.Errorf("git: refusing to commit on the tracked branch %q via the PR path", s.branch) + } + if _, err := s.git(ctx, s.dir, "checkout", "-B", headBranch, s.branch); err != nil { + return false, fmt.Errorf("git: checkout -B %s: %w", headBranch, err) + } + committed, _, err := s.CommitFile(ctx, relPath, content, message) + if err != nil { + // Always return to the tracked branch, even on failure, so the working + // tree is not left on a half-built head for the next reconcile. + _, _ = s.git(ctx, s.dir, "checkout", s.branch) + return false, err + } + if _, err := s.git(ctx, s.dir, "checkout", s.branch); err != nil { + return committed, fmt.Errorf("git: return to %s: %w", s.branch, err) + } + return committed, nil +} + +// PushBranch force-pushes headBranch to origin. Force is safe and intended +// here: the branch is rollops-owned and deterministically named per bump, so +// pushing refreshes an existing proposal rather than clobbering shared work. +func (s *Source) PushBranch(ctx context.Context, headBranch string) error { + _, err := s.git(ctx, s.dir, "push", "--force", "origin", headBranch+":"+headBranch) + return err +} + // git runs a git command, threading per-repo auth via env (GIT_SSH_COMMAND for // deploy keys; token in the URL is handled by the caller for https). func (s *Source) git(ctx context.Context, workdir string, args ...string) (string, error) { diff --git a/internal/git/repo_test.go b/internal/git/repo_test.go index 7c76534..587cb61 100644 --- a/internal/git/repo_test.go +++ b/internal/git/repo_test.go @@ -148,3 +148,13 @@ func TestRedactArgs_MasksToken(t *testing.T) { t.Errorf("redaction malformed: %q", got) } } + +func TestCommitFileOnBranch_RefusesTrackedBranch(t *testing.T) { + s := Open(t.TempDir(), "main", Auth{}) + if _, err := s.CommitFileOnBranch(context.Background(), "main", "x.yaml", []byte("y"), "m"); err == nil { + t.Fatal("committing the PR path onto the tracked branch must be refused") + } + if _, err := s.CommitFileOnBranch(context.Background(), "", "x.yaml", []byte("y"), "m"); err == nil { + t.Fatal("empty head branch must be refused") + } +} diff --git a/internal/reconcile/imageauto.go b/internal/reconcile/imageauto.go index d346f79..7775734 100644 --- a/internal/reconcile/imageauto.go +++ b/internal/reconcile/imageauto.go @@ -65,6 +65,13 @@ func (ia ImageAuto) Process(ctx context.Context, src *git.Source, nc config.Name return nc.Config, "", nil } msg := fmt.Sprintf("chore(image): %s %s -> %s (rollops)", nc.Config.Metadata.Name, from, to) + + if pol.WritebackMode() == config.WritebackPullRequest { + return ia.proposeViaPR(ctx, src, nc, patched, msg, from, to) + } + + // Push writeback: commit the bump directly on the tracked branch and deploy + // it this cycle, since Git and the cluster now agree. if _, _, err := src.CommitFile(ctx, nc.Path, patched, msg); err != nil { return nc.Config, "", fmt.Errorf("imageauto: commit: %w", err) } @@ -78,6 +85,54 @@ func (ia ImageAuto) Process(ctx context.Context, src *git.Source, nc config.Name return bumped, newRef, nil } +// proposeViaPR is the pull-request writeback path: it commits the bump on a +// rollops-owned branch, pushes it, and opens (or refreshes) a PR into the +// tracked branch — never writing that branch directly, so branch protection is +// honoured. +// +// Crucially it returns ref="" (no deploy). The bump lives only on the PR; the +// cluster must not lead Git. The deploy happens later, through the ordinary +// reconcile, once the PR merges and the tracked branch advances to carry it. +// This is what keeps a protected branch and the running target consistent. +func (ia ImageAuto) proposeViaPR(ctx context.Context, src *git.Source, nc config.NamedConfig, patched []byte, msg, from, to string) (*config.Config, string, error) { + head := prBranchName(nc.Config.Metadata.Name) + committed, err := src.CommitFileOnBranch(ctx, head, nc.Path, patched, msg) + if err != nil { + return nc.Config, "", fmt.Errorf("imageauto: pr commit: %w", err) + } + if !committed { + // The proposal branch already carries exactly this bump; the PR (if any) + // stands. Nothing to push or reopen. + return nc.Config, "", nil + } + if err := src.PushBranch(ctx, head); err != nil { + return nc.Config, "", fmt.Errorf("imageauto: pr push: %w", err) + } + body := fmt.Sprintf("Automated image bump by rollops.\n\n- config: `%s`\n- %s → %s\n\nMerging this deploys the new image through the normal reconcile. Opened as a PR because the tracked branch does not accept direct pushes.", nc.Path, from, to) + if _, _, err := src.OpenPullRequest(ctx, head, src.Branch(), msg, body); err != nil { + return nc.Config, "", fmt.Errorf("imageauto: open pr: %w", err) + } + return nc.Config, "", nil +} + +// prBranchName is the deterministic head branch for a config's image proposals. +// Deterministic so re-running updates the same PR rather than spawning a new one +// each poll; sanitised so an arbitrary config name is always a valid git ref. +func prBranchName(configName string) string { + safe := strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_': + return r + default: + return '-' + } + }, configName) + if safe == "" { + safe = "config" + } + return "rollops/image/" + safe +} + // resolve computes the new image reference for the current image and policy, or // returns newRef="" when nothing changed. from/to are the human-readable old/new // versions for the commit message. diff --git a/internal/reconcile/imageauto_test.go b/internal/reconcile/imageauto_test.go index 7eb6539..d94ab12 100644 --- a/internal/reconcile/imageauto_test.go +++ b/internal/reconcile/imageauto_test.go @@ -2,6 +2,10 @@ package reconcile import ( "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" "os" "os/exec" "path/filepath" @@ -336,3 +340,105 @@ func TestImageAuto_AlreadyCurrent(t *testing.T) { t.Fatalf("already current must be no-op, got ref=%q err=%v", ref, err) } } + +// TestImageAuto_PullRequestWriteback proves the protected-branch path: a bump in +// pull-request mode opens a PR and does NOT deploy. The cluster must never lead +// Git — the deploy waits for the merge — so Process returns ref="" and the +// tracked branch is left untouched, while a PR is opened via the API. +func TestImageAuto_PullRequestWriteback(t *testing.T) { + // Stub GitHub: record the create-PR call, succeed on auto-merge. + var opened bool + var head, base string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/pulls"): + opened = true + var b map[string]string + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &b) + head, base = b["head"], b["base"] + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"node_id":"n","html_url":"https://github.com/acme/web/pull/1"}`)) + case r.URL.Path == "/graphql": + _, _ = w.Write([]byte(`{"data":{}}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + cfgYAML := strings.Replace(imgConfigYAML, "image: ghcr.io/acme/web:v1.0.0", "image: ghcr.io/acme/web:latest", 1) + cfgYAML = strings.Replace(cfgYAML, "mode: minor", "mode: digest\n allowMutableTags: true\n writeback: pull-request", 1) + + src := newGitRepo(t, cfgYAML).WithURL("https://github.com/acme/web").WithAPIBase(srv.URL) + cfg, err := config.Load([]byte(cfgYAML)) + if err != nil { + t.Fatal(err) + } + bumped, ref, err := ImageAuto{Scanner: fakeDigest("sha256:brandnew")}. + Process(context.Background(), src, config.NamedConfig{Path: "apps/web.yaml", Config: cfg}) + if err != nil { + t.Fatalf("Process: %v", err) + } + + // No deploy this cycle: ref empty, config returned unchanged. + if ref != "" { + t.Fatalf("PR mode must not deploy; got ref=%q", ref) + } + if got, _ := bumped.Spec.Target.Spec["image"].(string); got != "ghcr.io/acme/web:latest" { + t.Errorf("returned config must be the original (un-bumped), got %q", got) + } + // A PR was opened into the tracked branch. + if !opened { + t.Fatal("no create-PR call reached the API") + } + if head != "rollops/image/web" || base != "main" { + t.Fatalf("wrong PR head/base: %q -> %q", head, base) + } + // The tracked branch on disk was NOT modified — the bump lives only on the + // PR branch. (checkout -B head, commit there, checkout back to main.) + data, _ := os.ReadFile(filepath.Join(src.Dir(), "apps/web.yaml")) + if strings.Contains(string(data), "sha256:brandnew") { + t.Errorf("tracked branch must be untouched in PR mode:\n%s", data) + } + // The PR branch was pushed to origin with the bump. + if out := gitOut(t, src.Dir(), "ls-remote", "origin", "rollops/image/web"); !strings.Contains(out, "rollops/image/web") { + t.Fatalf("PR branch was not pushed to origin: %q", out) + } + head2 := gitOut(t, src.Dir(), "show", "origin/rollops/image/web:apps/web.yaml") + if !strings.Contains(head2, "sha256:brandnew") { + t.Errorf("PR branch does not carry the bump:\n%s", head2) + } +} + +// TestImageAuto_PushWritebackUnchanged pins that the default (push) mode still +// commits to the tracked branch and deploys, so this feature is additive. +func TestImageAuto_PushWritebackUnchanged(t *testing.T) { + cfgYAML := strings.Replace(imgConfigYAML, "image: ghcr.io/acme/web:v1.0.0", "image: ghcr.io/acme/web:latest", 1) + cfgYAML = strings.Replace(cfgYAML, "mode: minor", "mode: digest\n allowMutableTags: true", 1) // no writeback → default push + src := newGitRepo(t, cfgYAML) + cfg, _ := config.Load([]byte(cfgYAML)) + _, ref, err := ImageAuto{Scanner: fakeDigest("sha256:pushed")}. + Process(context.Background(), src, config.NamedConfig{Path: "apps/web.yaml", Config: cfg}) + if err != nil { + t.Fatalf("Process: %v", err) + } + if ref == "" { + t.Fatal("push mode must deploy this cycle (non-empty ref)") + } + data, _ := os.ReadFile(filepath.Join(src.Dir(), "apps/web.yaml")) + if !strings.Contains(string(data), "sha256:pushed") { + t.Errorf("push mode must bump the tracked branch:\n%s", data) + } +} + +func gitOut(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v: %s", strings.Join(args, " "), err, out) + } + return string(out) +} diff --git a/memory/decisions.md b/memory/decisions.md index 4a42b3d..2a0db24 100644 --- a/memory/decisions.md +++ b/memory/decisions.md @@ -64,3 +64,45 @@ Append-only. Superseded entries get `→ superseded [date]`, never deleted. independent instances with independent lifecycles, not one service configured twice. Going forward: any new mnemos-using product deploys+pins its own instance; do not introduce a shared mnemos endpoint. + +- **`go.sum` stays excluded from nox scanning; the fix belongs in nox** (decided + 2026-07-19). A note in open-threads claimed excluding `go.sum` "drops dependency + enumeration to 0" and implied 28 repos needed fixing. Measured across all 28: + false. Enumeration continues from `go.mod`, and `go.sum` findings are ~99% + false-positive because it hashes the entire module graph, not the build — 148 of + 148 Go findings on mnemos named versions MVS never selected. The exclusions were + correct workarounds. Removing them would have injected ~5,263 false positives + fleet-wide. **Rule going forward: verify a dependency finding against + `go list -deps` (packages actually imported) before calling it real — not + `go list -m all`, which reports the module graph and over-reports.** + +- **nox is the single dependency scanner; govulncheck is ruled out** (operator + decision, 2026-07-19). When the `go.sum` blind spot surfaced, the obvious fix was + restoring govulncheck (version- and reachability-aware) in the shared workflow. + Operator declined: "we use nox and don't want govulncheck." That scoped the work + to making nox correct rather than adding a second scanner, and produced + Nox-HQ/nox#248. + +- **Unreachable dependency findings are demoted, never dropped** (decided + 2026-07-19, implemented in Nox-HQ/nox#248). Reachability rests on a `go list` + call that can be wrong, and a silently vanished finding is indistinguishable from + a scanner that missed it. Unreachable → `info`, with `reachable=false` and + `affected_imports` recorded and the reason in the message. Conclusions are drawn + only from positive evidence: an advisory with no import metadata, or a build that + cannot be enumerated, leaves the finding exactly as it was. + +- **The shared `go-ci.yml` pin was bumped to nox 1.10.0 knowing agent-go breaks** + (decided 2026-07-19). A canary measured the blast radius first: agent-go 56 + gate-failing findings (42 critical), senat-os 3, mnemos 2, seven other repos 0. + Merged anyway — those vulnerabilities were always present and passing as + `medium`; a red build is a fair representation of the actual state, and a stale + pin is precisely how they stayed invisible. Consequence to expect: agent-go CI + red until its criticals are triaged. + +- **RETRACTION: rollops does NOT have a live `GO-2026-5932`** (2026-07-19). An + earlier entry in open-threads recorded it as a confirmed true positive in + `x/crypto@v0.53.0`. It is a false positive: the advisory has `introduced: 0`, no + fixed version, and is scoped by OSV to `x/crypto/openpgp` — which rollops does not + import (it links `chacha20`/`cryptobyte`). The original claim came from checking + module@version against the build and stopping there. → supersedes the + "rollops has a live dependency vuln" note of 2026-07-19. diff --git a/memory/open-threads.md b/memory/open-threads.md index ed2c8b0..283b031 100644 --- a/memory/open-threads.md +++ b/memory/open-threads.md @@ -1,9 +1,110 @@ # Open threads — Rollops -## Blocking next work -- None blocking. v0.1.0 is cut, published to github.com/klarlabs-studio/rollops - with a GitHub release, and importable as go.klarlabs.de/rollops. Next work is - P3/studio scope or the notification channel decision. +> **Reading order:** this section is the authoritative current state. Everything +> below it is dated history — later sections supersede earlier ones, and the +> `[OPEN]`/`[WAITING]` markers in the 2026-06-14 and 2026-07-17 blocks are +> historical, NOT live. Trust this list. + +## ACTUALLY OPEN (as of 2026-07-19) + +0. **[OPEN — HIGHEST VALUE] agent-go has 42 CRITICAL dependency vulnerabilities** + (+14 high). Found 2026-07-19 by the canary run before bumping the shared nox + pin. Not new — they were reported as `medium` and passing the gate for as long + as nox's severity bug existed. Everything else that session was plumbing to make + these visible. Triage them. +1. **[OPEN] Drop `go.sum` excludes across the 28 repos** — unblocked by nox v1.10.0 + + pin bump. ~28 PRs. agent-go LAST. Do not touch any repo pinned below 1.10.0. +2. **[OPEN] relicta is broken in the nox repo** — `relicta notes` exits 1 silently + (`ai.enabled: true`, `model: gpt-4` in `.relicta.yaml`, error swallowed) and + `relicta approve` renders `0.0.0` / `0 commits` on its confirmation screen while + the state file holds the correct version. v1.10.0's changelog was hand-written + as a result. Fix before the next cut. +3. **[OPEN] nox #248's commit message still mischaracterises `GO-2026-5932`** as a + confirmed true positive. The PR body carries an explicit retraction; the commit + message would need an amend + force-push. Decide whether that is worth it. + +1. **Six merged PRs are undeployed.** #65, #66, #72, #74, #75, #76 are on main; + the cluster still runs **v0.26.0**, which predates all of them. Deploy impact + was audited (see "NEXT-DEPLOY IMPACT" below) and is small — #74's + health-on-promote is the only real behaviour change. +2. **CHANGELOG is unwritten for those same six PRs.** Batch at the next release cut. +3. **`k3s-ghcr-pull` expires 2026-08-29** — one shared credential behind ten + `/ghcr-pull` secrets and 39 pods. Rotate deliberately before end of Aug 2026. + This is the only dated time bomb left. +4. **vorhut's first tagged release is still the real proof** of ghcr push on + `GITHUB_TOKEN`. PR runs proved login only; push fires on tags. +5. **Fleet has no working Go dependency-vuln detection — root cause is a nox bug** + (found 2026-07-19, detail below). nox resolves Go deps from `go.sum` and never + parses `go.mod`, so it reports module versions the build never selected; the 28 + repos excluding `go.sum` are therefore *correct*, and the exclusion leaves a + blind spot. Fix in nox (`core/analyzers/deps`), not in the fleet configs or the + shared workflow. govulncheck is ruled out by operator decision. + **Status: MERGED to nox `main` 2026-07-19 as `2c3888c` ([#248](https://github.com/Nox-HQ/nox/pull/248)); + #247 closed as superseded.** Three bugs, each hiding the others: + - **Versions from `go.sum`** (the whole module graph, not the build). + Now `go.mod`, using `go.sum` only for pruned transitives and only for + source-hashed entries. vorhut 376→19, mnemos 148→0, no in-build module lost. + - **Severity/remediation never populated.** All 5,263 findings were `medium` + with empty summaries — *not one high or critical* — so a critical dep CVE + **could never trip the high/critical gate**. Two causes: OSV `querybatch` + returns only `{id, modified}`, and OSV ships CVSS as *vector strings* that + `cvssToSeverity` couldn't parse. Also explains why the shipped fix-version + remediation field (`c8d8e3b`) had **never emitted anything** — it reads + `Affected`, which querybatch never returned. Now: 1 critical / 24 high / + 57 medium / 3 low on vorhut, with real summaries and CVE aliases. + - **Module-granular advisory matching.** Now scoped to OSV's + `ecosystem_specific.imports` ∩ `go list -deps`. Unreachable findings are + demoted to Info with `reachable=false`, never dropped. + + **✅ SHIPPED 2026-07-19.** nox **v1.10.0** released (release/sign/docker all + green; sha256 `4cc1f33e…` verified against the tarball, not just + `checksums.txt`), and the shared `go-ci.yml` pinned **1.7.1 → 1.10.0** + (klarlabs-studio/.github#38, merged). The fleet picks it up on next CI run. + + **⚠ BLAST RADIUS — MEASURED BEFORE MERGING THE PIN.** Canary across a 10-repo + sample, counting findings that would newly fail the gate: + **agent-go 56 (42 critical!), senat-os 3, mnemos 2, seven others 0.** + Those vulns were always present, passing as `medium`. Expect agent-go red. + Caveat: the canary ran with `go.sum` excludes REMOVED (the end state), so + failures may arrive in two waves — some now from better severity data, the + rest when the excludes drop. + + **NOW UNBLOCKED: drop the `go.sum` excludes across the 28 repos.** They were + correct workarounds, not bugs. **Do NOT remove them on any repo still pinned + below 1.10.0.** Suggest agent-go LAST so its criticals surface deliberately + rather than inside a bulk sweep. + + **Known ceiling:** of vorhut's 19 Go findings, 6 reachable, 2 provably not, + **11 undetermined** — most advisories carry no import metadata, so + reachability decides less than the rollops case suggests. +6. ~~**rollops itself has a live dependency vuln:** `GO-2026-5932` in + `x/crypto@v0.53.0`.~~ **RETRACTED 2026-07-19 — it is a false positive, and + nothing needs bumping.** `GO-2026-5932` has `introduced: 0` and no fixed + version: it is the advisory that `x/crypto/openpgp` is unmaintained, scoped + by OSV to specific import paths. rollops does not import `openpgp` — it links + `chacha20`/`cryptobyte`. The finding is real at *module* granularity and false + at *package* granularity. I originally called it a confirmed true positive + because I checked only module@version against the build; that check was too + coarse. **Lesson: for Go advisories, always check `ecosystem_specific.imports` + against `go list -deps` before calling a dependency finding real.** +7. **Optional/future:** flip a marketing site to semver once its CI publishes a + `site-v*`/`marketing-v*` image; add a monitor that alerts when no new image + version appears fleet-wide for N days (the billing outage went invisible for + 6 days precisely because nothing watched this). + +Not blocking: v0.1.0+ is published, importable as go.klarlabs.de/rollops, and the +fleet reconciles cleanly on GitHub App auth. + +## Closed since the markers below were written +- **git-auth migration (roady #34)** — EXECUTED 2026-07-18, fleet fully on App auth. +- **Leaked PAT `ghp_76ied…`** — REVOKED by operator 2026-07-17. +- **`tokenFile` stripping + `.pem` cleanup** — DONE 2026-07-18 (keys removed from + `~/Downloads`; `rollopsd-git` holds only the two App keys). +- **PAT decommission** — CLOSED by scope reduction instead of revocation: `RollOps` + was reduced in place to `read:packages` (value preserved, no downtime), so the + "mint a dedicated token then revoke" plan is moot. It stays as registry auth. +- **MCP tokens before next deploy** — NOT a blocker for this cluster; + `ROLLOPS_MCP_ADDR` is unset, so MCP is not served at all. ## Resolved - **Stack module paths** (2026-06-08): all 7 published + pinned in go.mod, full build resolves. @@ -41,11 +142,11 @@ - **Plugin marketplace** — search/info/install/list/update + 10 providers across 3 capabilities; flagconformance suite. - **In-cluster rollopsd** — containerized, deployed, running the fleet on v0.16.0. -### [WAITING] operator -- Revoke leaked PAT `ghp_76ied…`; recreate `rollopsd-git`/`rollopsd-registry` privately (`read -rs`). Cluster works on the (leaked) token until then. +### [WAITING] operator — ✅ CLOSED 2026-07-17 +- ~~Revoke leaked PAT `ghp_76ied…`~~; recreate `rollopsd-git`/`rollopsd-registry` privately (`read -rs`). **Operator revoked it 2026-07-17.** -### [OPEN] -- **roady #34** — per-repo deploy keys / GitHub App for least-privilege multi-org git auth (replaces the broad classic PAT). +### [OPEN] — ✅ ALL CLOSED (historical marker, see top of file) +- ~~**roady #34**~~ — per-repo deploy keys / GitHub App for least-privilege multi-org git auth. **EXECUTED 2026-07-18**; fleet fully on App auth. - Marketing sites (armada/brotwerk/kraftsport-coach/klarlabs) digest→semver: their CI now supports `site-v*`/`marketing-v*` tags; flip `.rollops` imagePolicy to minor after first semver release. Or stay continuous-deploy (digest). - hermes/mnemos config lives in klarlabs-studio/mnemos repo (done); shared-service config ownership pattern may need revisiting. @@ -65,7 +166,10 @@ - **MCP per-caller transport auth (#66):** bearer-token→identity reusing `api.Authenticator`; fail-closed via mcp-go `WithAuthorize` (403) + `WithRequestContextFn`; `Tools` derive the caller per-request (defense-in-depth handler-level fail-close). **BREAKING on deploy — see WAITING.** - **roady #34 git-auth — DESIGNED, ready to execute (#68).** Finding: GitHub App auth (auto-rotating installation tokens) *and* deploy keys already exist in the code (`git.Auth` + `internal/git/githubapp.go`) — #34 was never a build, only an operational migration. Landed `docs/git-auth-migration.md` + `deploy/kubernetes/rollopsd-git.example.yaml` + updated `rollopsd-watch.example.yaml`. Decisions: **separate App per org** (blast-radius isolation), cutover = `rollopsd-watch` ConfigMap edit + restart, uniform `contents: write` default. Execution is operator work → WAITING. -### [WAITING] operator +### [WAITING] operator — ✅ BOTH CLOSED (historical marker, see top of file) +- ~~Before the next MCP-serving deploy: set `ROLLOPS_MCP_TOKENS`~~ — **superseded + 2026-07-18**: MCP is not served on this cluster (`ROLLOPS_MCP_ADDR` unset), and + #76 replaced the env var with `ROLLOPS_MCP_TOKENS_FILE`. Original text below. - **Before the next MCP-serving deploy: set `ROLLOPS_MCP_TOKENS`** (JSON `{"":""}`) and give every MCP caller an `Authorization: Bearer `. #66 made the MCP surface **fail-closed** (no fallback agent) — it rejects all calls until tokens are configured. Merged to main but NOT deployed (cluster still on v0.26.0, which predates #66). - **Execute the git-auth migration (roady #34)** per `docs/git-auth-migration.md`: create the two GitHub Apps (one per org) + keys, populate the `rollopsd-git` Secret, cut the `rollopsd-watch` ConfigMap over to App auth repo-by-repo, then delete the PAT. Optionally flag any watch-only repos to split read-only vs write scope (else uniform `contents: write`). Design + templates are merged; this is GitHub/cluster operator work. @@ -84,7 +188,11 @@ optional-off, mnemos-repo config = hermes's instance). No config change needed; recorded in decisions.md. Not a shared pool. -### [OPEN] — remaining +### Reference — git-auth topology and hard-won gotchas (NOT an open list) + +The entries below were written as `[OPEN]` mid-migration. The migration is done; +what survives here is durable reference material — App IDs, the scope-vs-watch-list +trap, and the ordering constraint. Keep it, don't action it. - Future/optional: flip a marketing-site to semver *after* its CI publishes a `site-v*`/`marketing-v*` tagged image (see verified note above). - **git-auth migration (roady #34) — EXECUTED 2026-07-18, fleet fully on GitHub @@ -97,9 +205,10 @@ - **felixgeelhaar** (a USER account, not an org — `/orgs/...` 404s, use `/settings/installations/`): app_id `4330615` (`rollopsd-felixgeelhaar`), install `147373546`, key `github-app-fg.pem` — 2 repos (website, glossa). - - Both keys live in the `rollopsd-git` Secret alongside the legacy `token`; - mounted at `/etc/rollops/git`. The original `.pem`s are in `~/Downloads` - (chmod 600'd) — delete once satisfied. + - Both keys live in the `rollopsd-git` Secret, mounted at `/etc/rollops/git`. + The legacy `token` key was removed 2026-07-18, so the Secret now holds only + the two App keys. The original `.pem`s were deleted from `~/Downloads` the + same day. - **GOTCHA THAT NEARLY BROKE THE FLEET (both Apps!):** the install scope did NOT match the watch list. klarlabs-studio was missing **armada, klarlabs, mnemos**; felixgeelhaar was missing **felixgeelhaar.com**. Cutting over without checking @@ -205,8 +314,53 @@ coverctl, mnemos, scout, warden and nomi do. Migrating to the thin caller `.github/workflows/cf-pages.yml` (`secrets.*` refs). Added the same exclude set scout/mnemos ship → 0 high/critical. Two traps kept as comments in the file: **root-level `README.md` needs its own entry** (`**/*.md` does NOT - match it), and **`go.sum` must stay in dependency scanning** (nox reads it - as the module manifest for OSV; excluding it drops dep enumeration to 0). + match it), and **`go.sum` must stay in dependency scanning**. + + **CORRECTION — the note above is WRONG, and so was the first attempt to + correct it (measured across all 28 repos, 2026-07-19, nox 1.6.0):** + + 1. **"Drops dep enumeration to 0" is false.** Enumeration continues from + `go.mod`. Measured: scout 137 → 169 deps, mnemos 6 → 158, vorhut 1279 → 1527. + 2. **The entropy/secret rationale in the exclude comments is also false.** + Removing the exclusion adds **zero** SEC findings in every repo tested. The + entire delta is `VULN-001` (OSV dependency lookups). `go.sum` is not being + flagged as secrets by current nox at all. + 3. **But excluding it is still mostly RIGHT, for a reason nobody wrote down:** + `go.sum` is not the build manifest — it records hashes for the whole module + graph, including versions MVS never selects. Checked mnemos' 148 go-ecosystem + findings against `go list -m all`: **0 matched a version actually built; 148 + were stale entries** (e.g. `x/net` flagged at a 2019 pseudo-version while the + build uses v0.56.0). Scanning `go.sum` is ~99% false-positive. + 4. **The catch:** real vulns surface *only* through `go.sum` too. rollops (which + does not exclude) reports `GO-2026-5932` on `x/crypto@v0.53.0` — and that one + **does** match the built version. So the exclusion trades 99% noise for a + blind spot on true positives. + + **Net: 5,263 suppressed `VULN-001` findings across 22 repos, all severity + medium, overwhelmingly stale — but hiding an unknown number of real ones.** + + **The real problem is a nox bug, not a `.nox.yaml` setting** (operator decision + 2026-07-19: stay on nox, no govulncheck). `core/analyzers/deps/deps.go` registers + **`go.sum` and never parses `go.mod`** — but `go.sum` is not a lockfile. It + records hashes for the entire module graph, so `parseGoSum` emits every version + Go ever considered, not the ones MVS selected. Confirmed unchanged on **nox + 1.9.2** (148/148 still stale), so this is not a version lag. + + **Fix belongs in nox: resolve Go deps from `go.mod`, not `go.sum`.** Validated + against the fleet: rollops' true positive (`GO-2026-5932`, `x/crypto@v0.53.0`) + **is** in `go.mod` and survives; mnemos' stale hits (`x/net`@2019, + `x/crypto`@2019, `yaml.v2`@2.2.2) are **not** in `go.mod` and vanish. mnemos: + go.mod 76 modules vs go.sum 152 pairs — the exact 2× inflation. + + **Known trade-off:** Go 1.17+ pruning means `go.mod` lists modules providing + *imported* packages, so a vuln in a deeper transitive module can be missed + (mnemos builds `x/crypto@v0.53.0` but doesn't name it in `go.mod`). `go.mod` + gives ~100% precision with some recall loss; `go.sum` gives full recall at ~1% + precision. Full fidelity needs `go list -m all`, which requires the toolchain + and breaks nox's offline-graceful file-parser model. + + **Until nox is fixed: keep `go.sum` excluded fleet-wide** — the 28 repos are + correct as-is and must NOT be "fixed". - **A real lint finding**: the shared bar enables `noctx`. Fixed with `httptest.NewRequestWithContext(t.Context(), …)`. diff --git a/memory/sessions/2026-07-19.md b/memory/sessions/2026-07-19.md new file mode 100644 index 0000000..a477d1f --- /dev/null +++ b/memory/sessions/2026-07-19.md @@ -0,0 +1,124 @@ +--- +date: 2026-07-19 +--- + +## Decisions Made + +- **The 28 repos excluding `go.sum` were RIGHT — do not "fix" them.** The session + began intending to strip that exclusion fleet-wide, on the strength of a note in + `open-threads.md` claiming it "drops dep enumeration to 0". Measured instead of + trusted: enumeration continues from `go.mod`, and 148 of 148 Go findings on + mnemos named versions the build never selects (`x/net` at a 2019 pseudo-version + while the build uses v0.56.0). Scanning `go.sum` is ~99% false-positive because + it hashes the whole module graph, not the build. Opening those 28 PRs would have + injected ~5,263 false positives into baselines fleet-wide. + +- **Fix belongs in nox, not in fleet config or the shared workflow.** Once the + exclusions proved correct, the blind spot was nox's. Operator ruled out + govulncheck explicitly ("we use nox and don't want govulncheck"), which made the + scope "make nox do this properly" rather than "add a second scanner". + +- **Resolve Go deps from `go.mod`, consulting `go.sum` only for pruned + transitives, and only for source-hashed entries.** Neither file suffices alone: + `go.mod` omits deep transitives under Go 1.17+ pruning; `go.sum` names versions + that were never built. A `/go.mod`-only entry means Go fetched metadata but never + downloaded the code — that entry cannot be in the build. That third rule is what + took mnemos from 53 residual false positives to 0. + +- **Demote unreachable findings, never drop them.** Reachability rests on a + toolchain call that can be wrong, and a silently vanished finding is + indistinguishable from a scanner that missed it. Unreachable → `info` with + `reachable=false` and `affected_imports` recorded. Conclusions drawn only from + positive evidence: no import metadata or no enumerable build leaves the finding + untouched. + +- **Ship as one PR with three commits, not a stack.** Stacked PRs get no CI here — + `ci.yml` only triggers on PRs targeting `main`, so #248 sat at `mergeable: CLEAN` + with literally nothing having run. Retargeted to `main` to get real coverage. + +- **Released v1.10.0 (minor, not patch)** — 3 features since v1.9.2 alongside the + fixes. Changelog hand-written because `relicta notes` is broken. + +- **Merged the pin bump despite knowing agent-go breaks.** A red build is a fair + representation of the actual state; a stale pin is how 42 criticals stayed + invisible. + +## What Went Wrong + +- **I trusted a memory note instead of measuring, and nearly acted on it.** The + `go.sum` claim in `open-threads.md` was wrong in *both* directions (not "0 + enumeration"; and the "module digests look like API keys" rationale produced zero + secret findings on current nox). One A/B scan would have caught it — I ran one + only because the first sweep result looked strange. + +- **I asserted a false positive as a "confirmed true positive" and used it as + validation evidence.** Claimed `GO-2026-5932` (`x/crypto@v0.53.0`) was a real + live vuln in rollops, in the commit message, PR body, and this memory file. It + has `introduced: 0`, no fixed version, and is scoped to `x/crypto/openpgp` — which + rollops does not import. I had checked module@version against the build and + stopped there. Corrected in the PR with an explicit retraction section rather + than a quiet edit; the commit message still carries it (would need force-push). + +- **Used the wrong ground truth twice.** First `go list -m all` (reports the module + *graph*, over-reports) which made the fix look like it lost 7 true positives; + then `go list -deps` (packages actually imported) showed 0 lost. Then a third + correction: module-level "true positives" are still overcounted, because OSV + scopes Go advisories to import paths. + +- **Wrote a real concurrency bug.** Flattened the OSV results map, hydrated, and + rebuilt it — map iteration order is randomised, so advisories were reattributed + to the wrong packages. `TestQueryOSV_BatchQuery` caught it (express's and + lodash's swapped). Fixed by hydrating in place. + +- **Wrote a second bug in the same direction as the danger.** `go list -e` outside + a module echoes `./...` and exits 0; accepting it as a package would have turned + "unknown" into a false "not linked" — silently hiding real vulns. Own test caught + it. + +- **Two of my own CVSS test expectations were wrong** (1.6/6.1 vs correct 1.8/5.4). + Adjusting tests to match implementation is a smell; defensible only because the + canonical 9.8 and 10.0 vectors validate the formula independently. + +- **Reported "direct push to main isn't protected" based on the wrong check.** I + queried `required_pull_request_reviews` (false) and concluded unprotected. Main + requires 3 status checks; the operator's push bypassed them via admin rights. + +- **`relicta publish` blocked 4 times; declined to route around it.** Refused + `git tag && git push` when asked to "ignore relicta and publish" — same artifact + through a door the block exists to close. Operator ran it, which is the correct + resolution. + +- **Operator typo nearly re-released an old version.** `git tag -a v1.8.0` on + today's code; failed only because v1.8.0 already existed. `release.yml` fires on + any `v*`, so it would have published a conflicting v1.8.0. + +## Open Questions + +- **agent-go's 42 critical dependency vulnerabilities** — real backlog, untouched. +- Do the remaining 27 repos drop their `go.sum` excludes in bulk, and does agent-go + go last so its criticals surface deliberately? +- Should the nox commit message for #248 be amended (force-push) to remove the + `GO-2026-5932` mischaracterisation, or is the PR-body retraction sufficient? +- `relicta notes` (silent exit 1, `model: gpt-4`) and `relicta approve` (renders + `0.0.0` / `0 commits`) — fix before the next release cut? +- 11 of vorhut's 19 Go findings are `undetermined` because those advisories carry + no import metadata. Is there a better source for import scoping? + +## Tasks + +Done: +- Cleaned `open-threads.md` drift (stale `[OPEN]`/`[WAITING]` markers from + 2026-06-14 and 2026-07-17 that later sections had already resolved). +- Fleet-wide A/B measurement across all 28 `go.sum`-excluding repos. +- Nox-HQ/nox#248 merged (`2c3888c`): go.mod resolution, OSV detail hydration + + CVSS v3.1 base scores, import-path reachability. 13+ new tests, 38 packages green. +- nox v1.10.0 released, signed, docker published; sha256 verified against the + tarball rather than `checksums.txt` alone. +- klarlabs-studio/.github#38 merged — shared workflow pinned 1.7.1 → 1.10.0. +- Canary measured before merging the pin: agent-go 56 gate-failing (42 critical), + senat-os 3, mnemos 2, seven others 0. + +Remaining: +- Drop `go.sum` excludes across the 28 repos (now unblocked; ~28 PRs). +- Triage agent-go's 42 criticals. +- Fix relicta's `notes`/`approve` in the nox repo. diff --git a/memory/status.md b/memory/status.md index e5aae51..72df5ca 100644 --- a/memory/status.md +++ b/memory/status.md @@ -1,6 +1,6 @@ # Status — Rollops -*Updated: 2026-07-18* +*Updated: 2026-07-19* ## Current State @@ -66,7 +66,40 @@ load-bearing. Headline risk is **`k3s-ghcr-pull`, expiring 2026-08-29** — one credential shared by all ten `ghcr-pull` Secrets, 39 pods, fleet-wide `ImagePullBackOff` when it lapses. Detail + verification recipes in open-threads. -**Next Session Should:** Most open threads closed this session — housekeeping +**Last Session Summary (2026-07-19):** Started as rollops housekeeping and ended in +nox. Two tasks: clean the drift in `open-threads.md`, and fix the 28 repos whose +`.nox.yaml` excludes `go.sum`. **The second task was based on a wrong note in this +very memory and would have made things worse.** Measured instead of trusting: +excluding `go.sum` is CORRECT — it hashes the whole module graph, so ~99% of its +findings name versions the build never selects (148/148 stale on mnemos). Opening +those 28 PRs would have injected ~5,263 false positives fleet-wide. The real bug +was in nox, and pulling it surfaced two more: **every dependency finding ever +produced was `medium` with an empty summary** (5,263/5,263), so a critical +dependency CVE could never trip the high/critical gate — and the shipped +fix-version remediation field had never emitted anything, because both read data +OSV's `querybatch` does not return. Third: advisories matched per module, not per +affected import path. All three fixed in **Nox-HQ/nox#248** (merged `2c3888c`), +released as **nox v1.10.0**, and the shared `go-ci.yml` pinned **1.7.1 → 1.10.0** +(klarlabs-studio/.github#38). Canary before merging the pin: **agent-go 56 +gate-failing findings (42 critical)**, senat-os 3, mnemos 2, seven others 0 — those +vulnerabilities were always there, mislabelled medium. Also retracted a claim made +earlier in this file: rollops does NOT have a live `GO-2026-5932`; it is scoped to +`x/crypto/openpgp`, which rollops does not import. Full detail in +sessions/2026-07-19.md. + +**Next Session Should:** **Triage agent-go's 42 critical dependency +vulnerabilities** — that is the real finding of 2026-07-19 and the only genuinely +urgent item; everything else that session was plumbing to make it visible. Then +drop the `go.sum` excludes across the remaining 27 repos (now unblocked by +v1.10.0 — they were correct workarounds, not bugs, and must NOT be removed on any +repo still pinned below 1.10.0), leaving agent-go last so its criticals surface +deliberately rather than inside a bulk sweep. Lower priority: relicta's `notes` +(silent exit 1, `model: gpt-4` in `.relicta.yaml`) and `approve` (renders `0.0.0` / +`0 commits`) are both broken in the nox repo — the v1.10.0 changelog had to be +hand-written; fix before the next cut. + +**Superseded (2026-07-18 plan, kept for context):** Most open threads closed — +housekeeping (flat-key decision, docs-PR merge gotcha #63, PAT revocation) plus two features: metric analysis on manual `Verify`/`Promote` (#65) and **MCP per-caller bearer auth (#66, fail-closed, BREAKING)**. FIRST: **before the next MCP-serving deploy, set