diff --git a/CLAUDE.md b/CLAUDE.md index c6d8028..7577612 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,7 +52,7 @@ Mirrors the checker plugin pattern exactly: each fixable rule ID registers a `Re Not every checker has a paired remediator — only rules where the fix is a deterministic file edit (see the "Auto-Remediation" section in `README.md` for the current list and the excluded-as-too-risky rationale). Detection logic that needs to be shared between a checker and its remediator (so they can never drift) is exported from the checker's file — e.g. `checks.FindUnpinnedActions`, `checks.FindNodeInstallViolations`, `checks.FindJobsMissingHardenRunner` — rather than re-derived from the checker's free-text `Result.Message`. -Git write operations live in `internal/github/write.go` (`EnsureBranch`, `CommitFiles`, `CreateOrUpdatePullRequest`) — separate from the read-only helpers in `repos.go`/`cache.go`. `CommitFiles` batches all of a `Fix`'s file changes into one commit via the Git Data API (blob → tree → commit → ref update) and is a no-op if the resulting tree is unchanged. `CreateOrUpdatePullRequest` upserts against an existing open PR with the same head branch rather than creating duplicates on repeat scans — if the title/body of that PR differ from the current fix (e.g. a remediator's PR title format changed), it edits them in place rather than leaving the existing PR untouched. Remediator `Fix.PRTitle` values follow Conventional Commits (`fix(): `), matching the repo's commit message convention. +Git write operations live in `internal/github/write.go` (`EnsureBranch`, `CommitFiles`, `CreateOrUpdatePullRequest`) — separate from the read-only helpers in `repos.go`/`cache.go`. `CommitFiles` batches all of a `Fix`'s file changes into one commit via the Git Data API (blob → tree → commit → ref update) and is a no-op if the resulting tree is unchanged. `CreateOrUpdatePullRequest` upserts against an existing open PR with the same head branch rather than creating duplicates on repeat scans — if the title/body of that PR differ from the current fix (e.g. a remediator's PR title format changed), it edits them in place rather than leaving the existing PR untouched. `remediateOne` (`internal/remediation/engine.go`) always calls `CreateOrUpdatePullRequest` after `CommitFiles`, even when the commit was a no-op — otherwise a branch whose content already matches the fix (from an earlier run) would never get its stale PR title/body synced. Remediator `Fix.PRTitle` values follow Conventional Commits (`fix(): `), matching the repo's commit message convention. Remediation credentials (`--remediate-*` / `GIT_CASCADE_REMEDIATE_*`) have **no fallback** to scan or notify credentials — `resolveRemediateCredentials` errors out if `remediation.enabled` is true and none are set, since this is the one credential set that writes directly to scanned repositories. diff --git a/internal/remediation/engine.go b/internal/remediation/engine.go index c878c0d..98e60ae 100644 --- a/internal/remediation/engine.go +++ b/internal/remediation/engine.go @@ -101,13 +101,9 @@ func remediateOne(ctx context.Context, client *github.Client, cfg config.Remedia for i, f := range fix.Files { files[i] = gh.FileWrite{Path: f.Path, Content: f.Content} } - newSHA, err := gh.CommitFiles(ctx, client, repo.Owner, repo.Name, branch, headSHA, fix.CommitMessage, author, files) - if err != nil { + if _, err := gh.CommitFiles(ctx, client, repo.Owner, repo.Name, branch, headSHA, fix.CommitMessage, author, files); err != nil { return "", fmt.Errorf("committing fix: %w", err) } - if newSHA == "" { - return "", nil - } labels := cfg.PRLabels if labels == nil { diff --git a/internal/remediation/engine_full_test.go b/internal/remediation/engine_full_test.go index cfb2409..6e73804 100644 --- a/internal/remediation/engine_full_test.go +++ b/internal/remediation/engine_full_test.go @@ -131,6 +131,71 @@ func TestRun_OpensPRForRealFix(t *testing.T) { } } +// TestRun_SyncsPRTitleWhenCommitIsNoOp verifies that when the fix branch +// already has the right content (CreateTree yields the same tree SHA, so +// CommitFiles is a no-op), the PR title/body still gets synced against a +// stale existing PR instead of being left untouched. +func TestRun_SyncsPRTitleWhenCommitIsNoOp(t *testing.T) { + const branch = "git-cascade/fix/test-rule" + var gotEditTitle string + + mux := http.NewServeMux() + mux.HandleFunc("/api/v3/repos/o/r/git/ref/heads/main", func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, &github.Reference{Ref: github.Ptr("refs/heads/main"), Object: &github.GitObject{SHA: github.Ptr("base1")}}) + }) + mux.HandleFunc("/api/v3/repos/o/r/git/ref/heads/"+branch, func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, &github.Reference{Ref: github.Ptr("refs/heads/" + branch), Object: &github.GitObject{SHA: github.Ptr("head1")}}) + }) + mux.HandleFunc("/api/v3/repos/o/r/git/commits/head1", func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, &github.Commit{SHA: github.Ptr("head1"), Tree: &github.Tree{SHA: github.Ptr("tree1")}}) + }) + mux.HandleFunc("/api/v3/repos/o/r/git/trees", func(w http.ResponseWriter, r *http.Request) { + // Same tree SHA as the existing head commit's tree: CommitFiles treats + // this as a no-op (branch content already matches the fix). + writeJSON(w, &github.Tree{SHA: github.Ptr("tree1")}) + }) + mux.HandleFunc("/api/v3/repos/o/r/pulls", func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, []*github.PullRequest{{ + Number: github.Ptr(307), + HTMLURL: github.Ptr("https://github.com/o/r/pull/307"), + Title: github.Ptr("git-cascade: old title"), + Body: github.Ptr("old body"), + }}) + }) + mux.HandleFunc("/api/v3/repos/o/r/pulls/307", func(w http.ResponseWriter, r *http.Request) { + var body github.PullRequest + json.NewDecoder(r.Body).Decode(&body) + gotEditTitle = body.GetTitle() + writeJSON(w, &github.PullRequest{Number: github.Ptr(307), HTMLURL: github.Ptr("https://github.com/o/r/pull/307")}) + }) + client := newTestClient(t, mux) + + r := &fakeRemediator{id: "test-rule", fix: &Fix{ + Files: []FileChange{{Path: "a.txt", Content: []byte("hi")}}, + CommitMessage: "fix it", + PRTitle: "fix(test-rule): new title", + PRBody: "new body", + }} + Register(r) + defer delete(registry, "test-rule") + + rules := map[string]config.Rule{"test-rule": {ID: "test-rule", AutoRemediation: boolPtr(true)}} + results := []compliance.Result{{RuleID: "test-rule", Repo: "o/r", Status: compliance.StatusFail}} + repos := map[string]gh.Repository{"o/r": {Owner: "o", Name: "r", FullName: "o/r", DefaultBranch: "main"}} + cfg := config.RemediationConfig{Enabled: true} + + outcomes := Run(context.Background(), client, cfg, results, rules, repos, slog.Default()) + if len(outcomes) != 1 || outcomes[0].Err != nil { + t.Fatalf("expected 1 clean outcome, got %+v", outcomes) + } + if outcomes[0].PRURL != "https://github.com/o/r/pull/307" { + t.Errorf("got PRURL=%q", outcomes[0].PRURL) + } + if gotEditTitle != "fix(test-rule): new title" { + t.Errorf("expected PR title to be synced to new title, got %q", gotEditTitle) + } +} + // TestRun_UsesCustomBranchPrefix verifies RemediationConfig.BranchPrefix // overrides the "git-cascade/fix" default in the created branch name. func TestRun_UsesCustomBranchPrefix(t *testing.T) {