From a10d51acc0e025cf2ac551f2ac18cd56a15fc5b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Mon, 28 Sep 2026 19:29:21 +0200 Subject: [PATCH 1/4] Add pruefbyte: GitLab MR review bot on top of OpenCodeReview pruefbyte runs as a GitLab CI job, reviews the merge request with the OpenCodeReview CLI and posts findings as inline discussions from a bot account (PAT). Findings are anchored to diff lines with suggestion blocks, deduplicated via hidden fingerprints, and the bot's own threads are resolved once the flagged code changed. Unplaceable or over-limit findings go to a single summary note that is updated in place. Configuration is layered (defaults < global file < .pruefbyte.yml at the MR base commit < PRUEFBYTE_* env); the repo file cannot change the LLM endpoint, GitLab settings or executed binaries. Includes a Dockerfile bundling a pinned, checksum-verified ocr binary and a reusable CI template. Co-Authored-By: Claude Opus 5.5 (1M context) --- .dockerignore | 4 + Dockerfile | 28 +++ README.md | 109 ++++++++ cmd/pruefbyte/main.go | 271 ++++++++++++++++++++ cmd/pruefbyte/main_test.go | 192 ++++++++++++++ go.mod | 19 ++ go.sum | 45 ++++ internal/ci/ci.go | 56 +++++ internal/config/config.go | 258 +++++++++++++++++++ internal/config/config_test.go | 146 +++++++++++ internal/config/env.go | 87 +++++++ internal/gitlab/client.go | 233 +++++++++++++++++ internal/gitlab/client_test.go | 67 +++++ internal/gitlab/dryrun.go | 37 +++ internal/gitutil/git.go | 67 +++++ internal/gitutil/git_test.go | 56 +++++ internal/ocr/ocr_test.go | 143 +++++++++++ internal/ocr/result.go | 74 ++++++ internal/ocr/rules.go | 14 ++ internal/ocr/runner.go | 256 +++++++++++++++++++ internal/review/diff.go | 126 ++++++++++ internal/review/diff_test.go | 89 +++++++ internal/review/filter.go | 88 +++++++ internal/review/format.go | 175 +++++++++++++ internal/review/review.go | 367 +++++++++++++++++++++++++++ internal/review/review_test.go | 403 ++++++++++++++++++++++++++++++ pruefbyte.example.yml | 49 ++++ templates/pruefbyte.gitlab-ci.yml | 38 +++ 28 files changed, 3497 insertions(+) create mode 100644 .dockerignore create mode 100644 Dockerfile create mode 100644 README.md create mode 100644 cmd/pruefbyte/main.go create mode 100644 cmd/pruefbyte/main_test.go create mode 100644 go.mod create mode 100644 go.sum create mode 100644 internal/ci/ci.go create mode 100644 internal/config/config.go create mode 100644 internal/config/config_test.go create mode 100644 internal/config/env.go create mode 100644 internal/gitlab/client.go create mode 100644 internal/gitlab/client_test.go create mode 100644 internal/gitlab/dryrun.go create mode 100644 internal/gitutil/git.go create mode 100644 internal/gitutil/git_test.go create mode 100644 internal/ocr/ocr_test.go create mode 100644 internal/ocr/result.go create mode 100644 internal/ocr/rules.go create mode 100644 internal/ocr/runner.go create mode 100644 internal/review/diff.go create mode 100644 internal/review/diff_test.go create mode 100644 internal/review/filter.go create mode 100644 internal/review/format.go create mode 100644 internal/review/review.go create mode 100644 internal/review/review_test.go create mode 100644 pruefbyte.example.yml create mode 100644 templates/pruefbyte.gitlab-ci.yml diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..f9980df --- /dev/null +++ b/.dockerignore @@ -0,0 +1,4 @@ +.git +.serena +.pruefbyte +*.exe diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 0000000..d271b35 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,28 @@ +# syntax=docker/dockerfile:1 +ARG GO_VERSION=1.25 + +FROM golang:${GO_VERSION}-alpine AS build +WORKDIR /src +COPY go.mod go.sum ./ +RUN go mod download +COPY . . +ARG VERSION=dev +RUN CGO_ENABLED=0 go build -trimpath -ldflags "-s -w -X main.version=${VERSION}" -o /out/pruefbyte ./cmd/pruefbyte + +FROM alpine:3 AS ocr +ARG OCR_VERSION=v1.12.10 +ARG TARGETARCH=amd64 +RUN apk add --no-cache curl \ + && cd /tmp \ + && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/opencodereview-linux-${TARGETARCH}" \ + && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/sha256sum.txt" \ + && grep " opencodereview-linux-${TARGETARCH}\$" sha256sum.txt | sha256sum -c - \ + && install -m 0755 "opencodereview-linux-${TARGETARCH}" /out-ocr + +FROM alpine:3 +# OCR needs git >= 2.41; alpine:3 ships a current git. +RUN apk add --no-cache git ca-certificates +COPY --from=ocr /out-ocr /usr/local/bin/ocr +COPY --from=build /out/pruefbyte /usr/local/bin/pruefbyte +ENTRYPOINT [] +CMD ["pruefbyte", "review"] diff --git a/README.md b/README.md new file mode 100644 index 0000000..518cc2d --- /dev/null +++ b/README.md @@ -0,0 +1,109 @@ +# pruefbyte + +AI code review for GitLab merge requests, in the spirit of +[pr-agent](https://github.com/The-PR-Agent/pr-agent). The review itself is done by +[OpenCodeReview](https://github.com/alibaba/open-code-review) (`ocr`). pruefbyte +runs it in a merge request pipeline and posts the findings as inline discussions +from a dedicated bot account. + +- Findings are anchored to the right diff line, with GitLab "apply suggestion" blocks where possible. +- Re-running the pipeline never duplicates a comment. +- The bot resolves its own threads once the flagged code has changed and the finding is gone. +- Findings that can't be placed on the diff, or that go over the comment limit, go into a single summary note, which is updated in place. +- Works with every LLM provider OCR supports (Anthropic, OpenAI, Bedrock, DashScope, OpenRouter, …) and with any OpenAI- or Anthropic-compatible endpoint. + +## Setup + +1. **Bot user.** Create a GitLab user (e.g. `pruefbyte-bot`) and add it to your group or projects as **Developer**. Create a personal access token for it with scope `api`. +2. **CI/CD variables** (group level, masked): + | Variable | Value | + |---|---| + | `PRUEFBYTE_GITLAB_TOKEN` | the bot's PAT | + | `PRUEFBYTE_LLM_API_KEY` | the LLM API key | + | `PRUEFBYTE_LLM_PROVIDER` | e.g. `anthropic` | + | `PRUEFBYTE_LLM_MODEL` | e.g. `claude-sonnet-5` | +3. **Image.** Build and push it: + ```sh + docker build -t registry.example.com/tools/pruefbyte:latest --build-arg OCR_VERSION=v1.12.10 . + docker push registry.example.com/tools/pruefbyte:latest + ``` +4. **Pipeline.** Include the template in each project (or in a shared CI config): + ```yaml + include: + - project: tools/pruefbyte + file: templates/pruefbyte.gitlab-ci.yml + variables: + PRUEFBYTE_IMAGE: registry.example.com/tools/pruefbyte:latest + ``` + +## Configuration + +Settings are layered; later layers win: + +1. Built-in defaults. +2. The global file, from `--config` or `PRUEFBYTE_CONFIG`. See [`pruefbyte.example.yml`](pruefbyte.example.yml) for every key. +3. `.pruefbyte.yml` in the repository, **read from the merge request's base commit**. A merge request can't change its own review settings: config changes take effect once they are merged. +4. Environment variables `PRUEFBYTE_
_`, e.g. `PRUEFBYTE_REVIEW_MIN_SEVERITY=medium`. Lists are comma-separated. + +The repository file may only set `llm.model`, `ocr.*` (except `binary` and `extra_args`), `review.*` and `skip.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there. + +Secrets are only ever read from the env vars named by `gitlab.token_env` and `llm.api_key_env`. `ocr` runs with a private, temporary `HOME`, so its config file and session logs never touch the runner. + +Example `.pruefbyte.yml`: + +```yaml +ocr: + effort: high + exclude: ["**/generated/**"] + rules: + - path: "**/*.go" + rule: "Errors must be wrapped with %w; flag bare returns of err from external calls." +review: + min_severity: medium + max_comments: 15 +skip: + authors: [renovate-bot] +``` + +## Behaviour + +| Situation | What happens | +|---|---| +| Draft MR, skip label/author/branch/title | Job exits 0 without reviewing. | +| MR has new commits since the pipeline started | Skipped; the newer pipeline reviews it. | +| Finding on a line in the diff | Inline discussion. Suggestion block if OCR proposed replacement code. | +| Finding outside the diff, or GitLab rejects the position | Listed in the summary note. | +| Same finding as an earlier run (open or resolved) | Not posted again. Matched by a hidden fingerprint of file + text. | +| Earlier bot thread not reported again **and** its code changed | Reply and resolve, but only after a complete review. | +| OCR fails | A failure note with the redacted error; job exits 1. | +| `review.fail_on_severity` reached | Comments are posted; job exits 3. | + +## Local use + +```sh +export PRUEFBYTE_GITLAB_TOKEN=glpat-... PRUEFBYTE_LLM_API_KEY=sk-... +pruefbyte review --config pruefbyte.yml --gitlab-url https://gitlab.example.com \ + --project group/project --mr 42 --repo . --dry-run +``` + +`--dry-run` prints the discussions instead of posting them. `pruefbyte config print` shows the effective configuration. + +## Development + +```sh +mise install # Go toolchain +go test ./... +``` + +Layout: + +| Path | Purpose | +|---|---| +| `cmd/pruefbyte` | CLI (cobra) and wiring | +| `internal/config` | layered config, repo-file allow-list, env overrides | +| `internal/ocr` | drives the `ocr` CLI and parses its JSON | +| `internal/gitlab` | client-go wrapper bound to one MR; dry-run decorator | +| `internal/review` | orchestration: filter, place, dedupe, publish, resolve | +| `internal/gitutil` | reads the repo config at the base commit; fetches missing commits | + +OCR is used as a subprocess. Its Go packages all live under `internal/`, so they can't be imported from another module; its JSON output is the stable interface. diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go new file mode 100644 index 0000000..a96c4d1 --- /dev/null +++ b/cmd/pruefbyte/main.go @@ -0,0 +1,271 @@ +// Command pruefbyte reviews GitLab merge requests with OpenCodeReview and posts +// the findings as inline discussions from a bot account. +package main + +import ( + "context" + "errors" + "fmt" + "os" + "os/signal" + "path/filepath" + "strings" + "syscall" + + "github.com/spf13/cobra" + + "pruefbyte/internal/ci" + "pruefbyte/internal/config" + "pruefbyte/internal/gitlab" + "pruefbyte/internal/gitutil" + "pruefbyte/internal/ocr" + "pruefbyte/internal/review" +) + +var version = "dev" + +// Exit codes. +const ( + exitError = 1 + exitGate = 3 +) + +type exitCodeError struct { + code int + err error +} + +func (e *exitCodeError) Error() string { return e.err.Error() } + +func main() { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + err := rootCmd().ExecuteContext(ctx) + if err == nil { + return + } + fmt.Fprintln(os.Stderr, "[pruefbyte] error:", err) + var ec *exitCodeError + if errors.As(err, &ec) { + os.Exit(ec.code) + } + os.Exit(exitError) +} + +type globalFlags struct { + configPath string + repoConfig bool +} + +func rootCmd() *cobra.Command { + g := &globalFlags{} + root := &cobra.Command{ + Use: "pruefbyte", + Short: "GitLab merge request reviews powered by OpenCodeReview", + SilenceUsage: true, + SilenceErrors: true, + } + root.PersistentFlags().StringVarP(&g.configPath, "config", "c", os.Getenv("PRUEFBYTE_CONFIG"), "global config file (env PRUEFBYTE_CONFIG)") + root.PersistentFlags().BoolVar(&g.repoConfig, "repo-config", true, "read "+config.RepoConfigFile+" from the merge request's base commit") + root.AddCommand(reviewCmd(g), configCmd(g), &cobra.Command{ + Use: "version", + Short: "Print the version", + Run: func(cmd *cobra.Command, _ []string) { fmt.Fprintln(cmd.OutOrStdout(), "pruefbyte", version) }, + }) + return root +} + +// baseConfig is defaults < global file < env. The repo file is layered in later, +// below env, once the base commit is known. +func baseConfig(g *globalFlags) (config.Config, error) { + cfg := config.Default() + if err := cfg.LoadGlobalFile(g.configPath); err != nil { + return cfg, err + } + return cfg, cfg.ApplyEnv(os.LookupEnv) +} + +func configCmd(g *globalFlags) *cobra.Command { + cmd := &cobra.Command{Use: "config", Short: "Inspect configuration"} + var repoFile string + printCmd := &cobra.Command{ + Use: "print", + Short: "Print the effective configuration (secrets are only referenced by env var name)", + RunE: func(cmd *cobra.Command, _ []string) error { + cfg := config.Default() + if err := cfg.LoadGlobalFile(g.configPath); err != nil { + return err + } + if repoFile != "" { + data, err := os.ReadFile(repoFile) + if err != nil { + return err + } + if err := cfg.ApplyRepo(data); err != nil { + return err + } + } + if err := cfg.ApplyEnv(os.LookupEnv); err != nil { + return err + } + fmt.Fprint(cmd.OutOrStdout(), cfg.YAML()) + return cfg.Validate() + }, + } + printCmd.Flags().StringVar(&repoFile, "repo-file", "", "also apply this repository config file") + cmd.AddCommand(printCmd) + return cmd +} + +type reviewFlags struct { + gitlabURL string + project string + mr int64 + repoDir string + dryRun bool + artifactDir string +} + +func reviewCmd(g *globalFlags) *cobra.Command { + f := &reviewFlags{} + cmd := &cobra.Command{ + Use: "review", + Short: "Review the current merge request and post inline comments", + Long: `Review a merge request with OpenCodeReview and post the findings as inline +discussions. Inside a GitLab merge request pipeline everything is read from the +CI variables; outside CI pass --project and --mr and run from a clone of the repo.`, + RunE: func(cmd *cobra.Command, _ []string) error { return runReview(cmd.Context(), g, f) }, + } + cmd.Flags().StringVar(&f.gitlabURL, "gitlab-url", "", "GitLab URL (default: gitlab.url or CI_SERVER_URL)") + cmd.Flags().StringVar(&f.project, "project", "", "project ID or path (default: CI_MERGE_REQUEST_PROJECT_ID)") + cmd.Flags().Int64Var(&f.mr, "mr", 0, "merge request IID (default: CI_MERGE_REQUEST_IID)") + cmd.Flags().StringVar(&f.repoDir, "repo", "", "path of the git clone (default: CI_PROJECT_DIR or cwd)") + cmd.Flags().BoolVar(&f.dryRun, "dry-run", false, "print the comments instead of posting them") + cmd.Flags().StringVar(&f.artifactDir, "artifact-dir", os.Getenv("PRUEFBYTE_ARTIFACT_DIR"), "directory to keep ocr-result.json in (env PRUEFBYTE_ARTIFACT_DIR)") + return cmd +} + +func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { + base, err := baseConfig(g) + if err != nil { + return err + } + mrc := ci.FromEnv(os.Getenv) + if base.GitLab.URL != "" { + mrc.ServerURL = base.GitLab.URL + } + if f.gitlabURL != "" { + mrc.ServerURL = f.gitlabURL + } + if f.project != "" { + mrc.Project = f.project + mrc.HeadSHA = "" // not the pipeline's MR + } + if f.mr != 0 { + mrc.MRIID = f.mr + mrc.HeadSHA = "" + } + if f.repoDir != "" { + mrc.RepoDir = f.repoDir + } + if mrc.RepoDir, err = filepath.Abs(mrc.RepoDir); err != nil { + return err + } + if err := mrc.Validate(); err != nil { + return err + } + + token := os.Getenv(base.GitLab.TokenEnv) + if token == "" { + return fmt.Errorf("GitLab token missing: set %s to the bot account's personal access token", base.GitLab.TokenEnv) + } + apiKey := os.Getenv(base.LLM.APIKeyEnv) + + client, err := gitlab.New(strings.TrimRight(mrc.ServerURL, "/")+"/api/v4", token, mrc.Project, mrc.MRIID) + if err != nil { + return err + } + var api gitlab.API = client + if f.dryRun { + api = gitlab.DryRun{API: client, Out: os.Stdout} + } + + runner, err := ocr.NewRunner(base.OCR.Binary, os.Stderr) + if err != nil { + return err + } + defer runner.Close() + v, err := runner.Version(ctx) + if err != nil { + return err + } + fmt.Fprintf(os.Stderr, "[pruefbyte] %s, %s\n", version, strings.SplitN(v, "\n", 2)[0]) + + repo := gitutil.Repo{Dir: mrc.RepoDir} + deps := review.Deps{ + GitLab: api, + OCR: runner, + Log: os.Stderr, + LoadConfig: func(ctx context.Context, baseSHA string) (config.Config, error) { + cfg := config.Default() + if err := cfg.LoadGlobalFile(g.configPath); err != nil { + return cfg, err + } + if g.repoConfig { + data, found, err := repo.ShowFile(ctx, baseSHA, config.RepoConfigFile) + if err != nil { + return cfg, err + } + if found { + fmt.Fprintf(os.Stderr, "[pruefbyte] using %s from %.8s\n", config.RepoConfigFile, baseSHA) + if err := cfg.ApplyRepo(data); err != nil { + return cfg, err + } + } + } + if err := cfg.ApplyEnv(os.LookupEnv); err != nil { + return cfg, err + } + if err := cfg.Validate(); err != nil { + return cfg, err + } + if apiKey == "" && cfg.LLM.Provider != "bedrock" { + fmt.Fprintf(os.Stderr, "[pruefbyte] warning: %s is empty; ocr falls back to the provider's own env var\n", cfg.LLM.APIKeyEnv) + } + return cfg, runner.Configure(ctx, cfg.LLM, apiKey, cfg.OCR.Language) + }, + EnsureCommits: func(ctx context.Context, shas ...string) error { + return repo.EnsureCommits(ctx, []string{fmt.Sprintf("refs/merge-requests/%d/head", mrc.MRIID)}, shas...) + }, + Redact: func(s string) string { + for _, secret := range []string{apiKey, token} { + if len(secret) >= 4 { + s = strings.ReplaceAll(s, secret, "***") + } + } + return s + }, + } + if f.artifactDir != "" { + deps.SaveResult = func(raw []byte) error { + if err := os.MkdirAll(f.artifactDir, 0o755); err != nil { + return err + } + return os.WriteFile(filepath.Join(f.artifactDir, "ocr-result.json"), raw, 0o644) + } + } + + out, err := review.Run(ctx, deps, review.Options{RepoDir: mrc.RepoDir, ExpectedHeadSHA: mrc.HeadSHA}) + if out != nil && out.Skipped == "" { + s := out.Stats + fmt.Fprintf(os.Stderr, "[pruefbyte] findings=%d posted=%d duplicates=%d outside_diff=%d over_limit=%d failed=%d resolved=%d\n", + s.Findings, s.Posted, s.Duplicates, s.Unplaced, s.Overflow, s.Failed, s.Resolved) + } + if err != nil { + return err + } + if out.GateFailed { + return &exitCodeError{code: exitGate, err: errors.New(out.GateReason)} + } + return nil +} diff --git a/cmd/pruefbyte/main_test.go b/cmd/pruefbyte/main_test.go new file mode 100644 index 0000000..005feac --- /dev/null +++ b/cmd/pruefbyte/main_test.go @@ -0,0 +1,192 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" +) + +// When FAKE_OCR is set, the test binary behaves like the ocr CLI. +func TestMain(m *testing.M) { + if os.Getenv("FAKE_OCR") != "" { + os.Exit(fakeOCR(os.Args[1:])) + } + os.Exit(m.Run()) +} + +func fakeOCR(args []string) int { + if len(args) == 0 { + return 2 + } + switch args[0] { + case "version": + fmt.Println("open-code-review vFAKE") + return 0 + case "config": + // Record settings so the test can check them. + f, _ := os.OpenFile(filepath.Join(os.Getenv("HOME"), "settings.txt"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + defer f.Close() + fmt.Fprintf(f, "%s=%s\n", args[2], args[3]) + return 0 + case "review": + var out string + for i, a := range args { + if a == "--output" { + out = args[i+1] + } + } + _ = os.WriteFile(os.Getenv("FAKE_OCR_ARGS"), []byte(strings.Join(args, "\n")), 0o600) + _ = os.WriteFile(out, []byte(os.Getenv("FAKE_OCR_RESULT")), 0o600) + return 0 + } + return 2 +} + +type fakeServer struct { + mu sync.Mutex + base string + head string + posted []map[string]any + notes []string + headers []string +} + +func (s *fakeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + s.headers = append(s.headers, r.Header.Get("Private-Token")) + w.Header().Set("Content-Type", "application/json") + p := strings.TrimPrefix(r.URL.EscapedPath(), "/api/v4") + mrPath := "/projects/grp%2Fproj/merge_requests/1" + switch { + case p == "/user": + fmt.Fprint(w, `{"id": 99, "username": "pruefbyte-bot"}`) + case p == mrPath: + fmt.Fprintf(w, `{"iid": 1, "title": "Feature", "description": "Adds a map", "source_branch": "feat", "target_branch": "main", + "author": {"username": "alice"}, "draft": false, "labels": [], + "diff_refs": {"base_sha": %q, "start_sha": %q, "head_sha": %q}}`, s.base, s.base, s.head) + case p == mrPath+"/diffs": + fmt.Fprint(w, `[{"old_path": "main.go", "new_path": "main.go", "diff": "@@ -1,3 +1,4 @@\n package main\n \n+var m map[string]int\n func main() {}\n"}]`) + case p == mrPath+"/discussions" && r.Method == http.MethodGet: + fmt.Fprint(w, `[]`) + case p == mrPath+"/discussions" && r.Method == http.MethodPost: + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + s.posted = append(s.posted, body) + fmt.Fprint(w, `{"id": "d1", "notes": []}`) + case p == mrPath+"/notes" && r.Method == http.MethodPost: + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + s.notes = append(s.notes, fmt.Sprint(body["body"])) + fmt.Fprint(w, `{"id": 5}`) + default: + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"message": "404 Not Found"}`) + } +} + +func gitCmd(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-c", "user.name=t", "-c", "user.email=t@t", "-c", "core.autocrlf=false"}, args...)...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + +func TestReviewEndToEnd(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + repo := t.TempDir() + gitCmd(t, repo, "init", "-q") + os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\n\nfunc main() {}\n"), 0o644) + // Repo config at the base commit: raises min_severity so the low finding is dropped. + os.WriteFile(filepath.Join(repo, ".pruefbyte.yml"), []byte("review:\n min_severity: medium\nocr:\n effort: high\n"), 0o644) + gitCmd(t, repo, "add", ".") + gitCmd(t, repo, "commit", "-qm", "base") + base := gitCmd(t, repo, "rev-parse", "HEAD") + os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\n\nvar m map[string]int\nfunc main() {}\n"), 0o644) + // Changing the config in the MR itself must have no effect. + os.WriteFile(filepath.Join(repo, ".pruefbyte.yml"), []byte("review:\n min_severity: low\n"), 0o644) + gitCmd(t, repo, "commit", "-qam", "head") + head := gitCmd(t, repo, "rev-parse", "HEAD") + + srv := &fakeServer{base: base, head: head} + ts := httptest.NewServer(srv) + defer ts.Close() + + dir := t.TempDir() + global := filepath.Join(dir, "pruefbyte.yml") + os.WriteFile(global, []byte(fmt.Sprintf(` +llm: + provider: anthropic + model: claude-sonnet-5 +ocr: + binary: %q +review: + fail_on_severity: critical +`, os.Args[0])), 0o644) + + result := `{"status": "success", "llm": {"provider": "anthropic", "model": "claude-sonnet-5"}, "comments": [ + {"path": "main.go", "content": "Writing to a nil map panics.", "start_line": 3, "end_line": 3, + "existing_code": "var m map[string]int", "suggestion_code": "var m = map[string]int{}", "severity": "critical", "category": "bug"}, + {"path": "main.go", "content": "Nitpick.", "start_line": 3, "end_line": 3, "severity": "low"}]}` + argsFile := filepath.Join(dir, "args.txt") + for k, v := range map[string]string{ + "FAKE_OCR": "1", "FAKE_OCR_RESULT": result, "FAKE_OCR_ARGS": argsFile, + "PRUEFBYTE_GITLAB_TOKEN": "glpat-bot", "PRUEFBYTE_LLM_API_KEY": "sk-ant-test", + "CI_SERVER_URL": ts.URL, "CI_MERGE_REQUEST_PROJECT_ID": "grp/proj", "CI_MERGE_REQUEST_IID": "1", + "CI_COMMIT_SHA": head, "CI_PROJECT_DIR": repo, "PRUEFBYTE_ARTIFACT_DIR": filepath.Join(dir, "artifacts"), + "CI_MERGE_REQUEST_SOURCE_BRANCH_SHA": "", + } { + t.Setenv(k, v) + } + + cmd := rootCmd() + cmd.SetArgs([]string{"review", "--config", global}) + cmd.SetOut(io.Discard) + err := cmd.ExecuteContext(context.Background()) + + var ec *exitCodeError + if !errors.As(err, &ec) || ec.code != exitGate { + t.Fatalf("want severity gate exit, got %v", err) + } + if len(srv.posted) != 1 { + t.Fatalf("posted %d discussions, want 1 (low finding filtered by repo config at base)", len(srv.posted)) + } + pos := srv.posted[0]["position"].(map[string]any) + if pos["new_line"] != float64(3) || pos["head_sha"] != head || pos["base_sha"] != base { + t.Errorf("position %v", pos) + } + body := srv.posted[0]["body"].(string) + if !strings.Contains(body, "suggestion:-0+0") || !strings.Contains(body, "Critical · bug") { + t.Errorf("body:\n%s", body) + } + for _, h := range srv.headers { + if h != "glpat-bot" { + t.Fatalf("request without bot token: %q", h) + } + } + args, _ := os.ReadFile(argsFile) + for _, want := range []string{base, head, "--effort\nhigh", "--provider\nanthropic", "--background-file"} { + if !strings.Contains(string(args), want) { + t.Errorf("ocr args missing %q:\n%s", want, args) + } + } + if _, err := os.Stat(filepath.Join(dir, "artifacts", "ocr-result.json")); err != nil { + t.Errorf("artifact not written: %v", err) + } +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..8461b24 --- /dev/null +++ b/go.mod @@ -0,0 +1,19 @@ +module pruefbyte + +go 1.25.14 + +require ( + github.com/spf13/cobra v1.10.2 + gitlab.com/gitlab-org/api/client-go v1.46.0 + gopkg.in/yaml.v3 v3.0.1 +) + +require ( + github.com/google/go-querystring v1.2.0 // indirect + github.com/hashicorp/go-cleanhttp v0.5.2 // indirect + github.com/hashicorp/go-retryablehttp v0.7.8 // indirect + github.com/inconshreveable/mousetrap v1.1.0 // indirect + github.com/spf13/pflag v1.0.9 // indirect + golang.org/x/oauth2 v0.34.0 // indirect + golang.org/x/time v0.14.0 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..9ad3009 --- /dev/null +++ b/go.sum @@ -0,0 +1,45 @@ +github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= +github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0= +github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= +github.com/graph-gophers/graphql-go v1.9.0 h1:yu0ucKHLc5qGpRwLYKIWtr9bOoxovkWasuBrPQwlHls= +github.com/graph-gophers/graphql-go v1.9.0/go.mod h1:23olKZ7duEvHlF/2ELEoSZaY1aNPfShjP782SOoNTyM= +github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= +github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48= +github.com/hashicorp/go-hclog v1.6.3 h1:Qr2kF+eVWjTiYmU7Y31tYlP1h0q/X3Nl3tPGdaB11/k= +github.com/hashicorp/go-hclog v1.6.3/go.mod h1:W4Qnvbt70Wk/zYJryRzDRU/4r0kIg0PVHBcfoyhpF5M= +github.com/hashicorp/go-retryablehttp v0.7.8 h1:ylXZWnqa7Lhqpk0L1P1LzDtGcCR0rPVUrx/c8Unxc48= +github.com/hashicorp/go-retryablehttp v0.7.8/go.mod h1:rjiScheydd+CxvumBsIrFKlx3iS0jrZ7LvzFGFmuKbw= +github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= +github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= +github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= +github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= +github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= +github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= +github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= +github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +gitlab.com/gitlab-org/api/client-go v1.46.0 h1:YxBWFZIFYKcGESCb9fpkwzouo+apyB9pr/XTWzNoL24= +gitlab.com/gitlab-org/api/client-go v1.46.0/go.mod h1:FtgyU6g2HS5+fMhw6nLK96GBEEBx5MzntOiJWfIaiN8= +go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +golang.org/x/oauth2 v0.34.0 h1:hqK/t4AKgbqWkdkcAeI8XLmbK+4m4G5YeQRrmiotGlw= +golang.org/x/oauth2 v0.34.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= +golang.org/x/sys v0.39.0 h1:CvCKL8MeisomCi6qNZ+wbb0DN9E5AATixKsvNtMoMFk= +golang.org/x/sys v0.39.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/time v0.14.0 h1:MRx4UaLrDotUKUdCIqzPC48t1Y9hANFKIRpNx+Te8PI= +golang.org/x/time v0.14.0/go.mod h1:eL/Oa2bBBK0TkX57Fyni+NgnyQQN4LitPmob2Hjnqw4= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/ci/ci.go b/internal/ci/ci.go new file mode 100644 index 0000000..9417b01 --- /dev/null +++ b/internal/ci/ci.go @@ -0,0 +1,56 @@ +// Package ci reads the GitLab CI predefined variables that identify the merge request. +package ci + +import ( + "errors" + "os" + "strconv" +) + +type MRContext struct { + ServerURL string + // Project is a numeric ID or a namespace/path. + Project string + MRIID int64 + // HeadSHA is the MR source commit the pipeline runs for; empty outside CI. + HeadSHA string + RepoDir string +} + +// FromEnv reads CI_* variables. Missing values are left empty so flags can fill them. +func FromEnv(getenv func(string) string) MRContext { + c := MRContext{ + ServerURL: getenv("CI_SERVER_URL"), + Project: getenv("CI_MERGE_REQUEST_PROJECT_ID"), + // In merged-results pipelines CI_COMMIT_SHA is a merge commit; the MR head is this one. + HeadSHA: getenv("CI_MERGE_REQUEST_SOURCE_BRANCH_SHA"), + RepoDir: getenv("CI_PROJECT_DIR"), + } + if c.Project == "" { + c.Project = getenv("CI_PROJECT_ID") + } + if c.HeadSHA == "" { + c.HeadSHA = getenv("CI_COMMIT_SHA") + } + if n, err := strconv.ParseInt(getenv("CI_MERGE_REQUEST_IID"), 10, 64); err == nil { + c.MRIID = n + } + if c.RepoDir == "" { + c.RepoDir, _ = os.Getwd() + } + return c +} + +func (c MRContext) Validate() error { + var errs []error + if c.ServerURL == "" { + errs = append(errs, errors.New("GitLab URL unknown: set gitlab.url, --gitlab-url or run inside GitLab CI")) + } + if c.Project == "" { + errs = append(errs, errors.New("project unknown: pass --project or run in a merge request pipeline")) + } + if c.MRIID == 0 { + errs = append(errs, errors.New("merge request unknown: pass --mr or run in a merge request pipeline (CI_MERGE_REQUEST_IID)")) + } + return errors.Join(errs...) +} diff --git a/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..3afcb08 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,258 @@ +// Package config loads the layered pruefbyte configuration: +// built-in defaults < global file < repository file < PRUEFBYTE_* env vars. +package config + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "regexp" + "strings" + "time" + + "gopkg.in/yaml.v3" +) + +// RepoConfigFile is the file name looked up at the MR's base commit. +const RepoConfigFile = ".pruefbyte.yml" + +type Config struct { + GitLab GitLab `yaml:"gitlab"` + LLM LLM `yaml:"llm"` + OCR OCR `yaml:"ocr"` + Review Review `yaml:"review"` + Skip Skip `yaml:"skip"` +} + +type GitLab struct { + // URL of the GitLab instance. Defaults to CI_SERVER_URL. + URL string `yaml:"url"` + // TokenEnv names the env var holding the bot account's PAT. + TokenEnv string `yaml:"token_env"` +} + +type LLM struct { + // Provider is an OCR built-in provider name or any other name for a custom provider. + Provider string `yaml:"provider"` + Model string `yaml:"model"` + // APIKeyEnv names the env var holding the LLM API key. + APIKeyEnv string `yaml:"api_key_env"` + // URL overrides the built-in provider's base URL; required for custom providers. + URL string `yaml:"url"` + // Protocol is required for custom providers: anthropic|openai|openai-responses|anthropic-bedrock. + Protocol string `yaml:"protocol"` + TimeoutSec int `yaml:"timeout_sec"` + ExtraBody map[string]any `yaml:"extra_body"` + ExtraHeaders map[string]string `yaml:"extra_headers"` + AWSRegion string `yaml:"aws_region"` + AWSProfile string `yaml:"aws_profile"` +} + +type OCR struct { + Binary string `yaml:"binary"` + Effort string `yaml:"effort"` + Language string `yaml:"language"` + Timeout time.Duration `yaml:"timeout"` + Concurrency int `yaml:"concurrency"` + MaxTokensBudget int `yaml:"max_tokens_budget"` + Exclude []string `yaml:"exclude"` + Rules []Rule `yaml:"rules"` + RuleFile string `yaml:"rule_file"` + UseMRDescriptionAsBackground bool `yaml:"use_mr_description_as_background"` + ExtraArgs []string `yaml:"extra_args"` +} + +type Rule struct { + Path string `yaml:"path" json:"path"` + Rule string `yaml:"rule" json:"rule"` +} + +type Review struct { + MinSeverity string `yaml:"min_severity"` + Categories []string `yaml:"categories"` + MaxComments int `yaml:"max_comments"` + Suggestions bool `yaml:"suggestions"` + ResolveOutdated bool `yaml:"resolve_outdated"` + FailOnSeverity string `yaml:"fail_on_severity"` + PostFailures bool `yaml:"post_failures"` +} + +type Skip struct { + Drafts bool `yaml:"drafts"` + Labels []string `yaml:"labels"` + Authors []string `yaml:"authors"` + TitleRegex []string `yaml:"title_regex"` + SourceBranches []string `yaml:"source_branches"` + TargetBranches []string `yaml:"target_branches"` +} + +// repoConfig lists the only keys a repository file may set. Anything that decides +// where secrets are sent or what gets executed (gitlab.*, llm endpoint/provider, +// ocr.binary, ocr.extra_args) stays under the operator's control. +type repoConfig struct { + LLM struct { + Model string `yaml:"model"` + } `yaml:"llm"` + OCR struct { + Effort string `yaml:"effort"` + Language string `yaml:"language"` + Timeout time.Duration `yaml:"timeout"` + Concurrency int `yaml:"concurrency"` + MaxTokensBudget int `yaml:"max_tokens_budget"` + Exclude []string `yaml:"exclude"` + Rules []Rule `yaml:"rules"` + RuleFile string `yaml:"rule_file"` + UseMRDescriptionAsBackground bool `yaml:"use_mr_description_as_background"` + } `yaml:"ocr"` + Review Review `yaml:"review"` + Skip Skip `yaml:"skip"` +} + +func Default() Config { + return Config{ + GitLab: GitLab{TokenEnv: "PRUEFBYTE_GITLAB_TOKEN"}, + LLM: LLM{APIKeyEnv: "PRUEFBYTE_LLM_API_KEY"}, + OCR: OCR{ + Binary: "ocr", + Effort: "medium", + Timeout: 30 * time.Minute, + Concurrency: 8, + Exclude: []string{}, + UseMRDescriptionAsBackground: true, + }, + Review: Review{ + MinSeverity: "low", + MaxComments: 30, + Suggestions: true, + ResolveOutdated: true, + PostFailures: true, + }, + Skip: Skip{ + Drafts: true, + Labels: []string{"no-review"}, + TitleRegex: []string{`^\[WIP\]`}, + }, + } +} + +// ApplyGlobal overlays an operator-controlled YAML document. All keys are allowed. +func (c *Config) ApplyGlobal(data []byte) error { + return decodeStrict(data, c) +} + +// ApplyRepo overlays a repository YAML document. Only keys in repoConfig are allowed; +// anything else is rejected so a merge request cannot redirect credentials. +func (c *Config) ApplyRepo(data []byte) error { + var probe repoConfig + if err := decodeStrict(data, &probe); err != nil { + return fmt.Errorf("%s: %w (only llm.model, ocr.*, review.* and skip.* are allowed here, except ocr.binary and ocr.extra_args)", RepoConfigFile, err) + } + return decodeStrict(data, c) +} + +func decodeStrict(data []byte, out any) error { + if len(bytes.TrimSpace(data)) == 0 { + return nil + } + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + if err := dec.Decode(out); err != nil && !errors.Is(err, io.EOF) { + return err + } + return nil +} + +// LoadGlobalFile reads the global config file if path is non-empty. +func (c *Config) LoadGlobalFile(path string) error { + if path == "" { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("reading config %s: %w", path, err) + } + if err := c.ApplyGlobal(data); err != nil { + return fmt.Errorf("parsing config %s: %w", path, err) + } + return nil +} + +var severities = map[string]int{"low": 1, "medium": 2, "high": 3, "critical": 4} + +// SeverityRank returns 1 (low) .. 4 (critical), or 0 for unknown values. +func SeverityRank(s string) int { + return severities[strings.ToLower(strings.TrimSpace(s))] +} + +var protocols = map[string]bool{"anthropic": true, "openai": true, "openai-responses": true, "anthropic-bedrock": true} + +func (c Config) Validate() error { + var errs []error + if c.LLM.Provider == "" { + errs = append(errs, errors.New("llm.provider is required")) + } + if c.LLM.Model == "" { + errs = append(errs, errors.New("llm.model is required")) + } + if !IsBuiltinProvider(c.LLM.Provider) && c.LLM.Provider != "" { + if !protocols[c.LLM.Protocol] { + errs = append(errs, fmt.Errorf("llm.provider %q is not an OCR built-in provider, so llm.protocol must be one of anthropic, openai, openai-responses, anthropic-bedrock", c.LLM.Provider)) + } + if c.LLM.URL == "" && c.LLM.Protocol != "anthropic-bedrock" { + errs = append(errs, fmt.Errorf("llm.url is required for custom provider %q", c.LLM.Provider)) + } + } + if c.GitLab.TokenEnv == "" { + errs = append(errs, errors.New("gitlab.token_env is required")) + } + switch c.OCR.Effort { + case "", "low", "medium", "high": + default: + errs = append(errs, fmt.Errorf("ocr.effort must be low, medium or high, got %q", c.OCR.Effort)) + } + if c.OCR.RuleFile != "" && len(c.OCR.Rules) > 0 { + errs = append(errs, errors.New("set either ocr.rules or ocr.rule_file, not both")) + } + for _, r := range c.OCR.Rules { + if r.Path == "" || r.Rule == "" { + errs = append(errs, errors.New("every ocr.rules entry needs path and rule")) + break + } + } + if SeverityRank(c.Review.MinSeverity) == 0 { + errs = append(errs, fmt.Errorf("review.min_severity must be low, medium, high or critical, got %q", c.Review.MinSeverity)) + } + if c.Review.FailOnSeverity != "" && SeverityRank(c.Review.FailOnSeverity) == 0 { + errs = append(errs, fmt.Errorf("review.fail_on_severity must be empty or low, medium, high, critical, got %q", c.Review.FailOnSeverity)) + } + if c.Review.MaxComments < 0 { + errs = append(errs, errors.New("review.max_comments must be >= 0 (0 = unlimited)")) + } + for _, re := range c.Skip.TitleRegex { + if _, err := regexp.Compile(re); err != nil { + errs = append(errs, fmt.Errorf("skip.title_regex %q: %w", re, err)) + } + } + return errors.Join(errs...) +} + +// builtinProviders mirrors OCR's built-in provider table (docs: configuration.md). +var builtinProviders = map[string]bool{ + "anthropic": true, "bedrock": true, "openai": true, "openai-responses": true, + "openrouter": true, "gemini": true, "dashscope": true, "dashscope-tokenplan": true, + "volcengine": true, "deepseek": true, "tencent-tokenhub": true, "hy-tokenplan": true, + "iflytek": true, "kimi": true, "kimi-global": true, "z-ai": true, "mimo": true, + "minimax": true, "minimax-cn": true, "baidu-qianfan": true, "siliconflow": true, + "siliconflow-cn": true, "novita": true, "xai": true, +} + +func IsBuiltinProvider(name string) bool { return builtinProviders[name] } + +// YAML renders the effective config. Secrets are never stored in Config, so +// only the names of the env vars that hold them appear. +func (c Config) YAML() string { + out, _ := yaml.Marshal(c) + return string(out) +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..61f7cdb --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,146 @@ +package config + +import ( + "strings" + "testing" + "time" +) + +func env(m map[string]string) func(string) (string, bool) { + return func(k string) (string, bool) { v, ok := m[k]; return v, ok } +} + +func TestLayering(t *testing.T) { + cfg := Default() + global := ` +llm: + provider: anthropic + model: claude-sonnet-5 +review: + max_comments: 10 + min_severity: medium +skip: + labels: [skip-me] +` + repo := ` +llm: + model: claude-opus-5-5 +ocr: + effort: high + exclude: ["docs/**"] +review: + min_severity: high +` + if err := cfg.ApplyGlobal([]byte(global)); err != nil { + t.Fatal(err) + } + if err := cfg.ApplyRepo([]byte(repo)); err != nil { + t.Fatal(err) + } + if err := cfg.ApplyEnv(env(map[string]string{ + "PRUEFBYTE_REVIEW_MAX_COMMENTS": "5", + "PRUEFBYTE_OCR_TIMEOUT": "5m", + "PRUEFBYTE_SKIP_AUTHORS": "renovate-bot, dependabot", + "PRUEFBYTE_REVIEW_SUGGESTIONS": "false", + })); err != nil { + t.Fatal(err) + } + if err := cfg.Validate(); err != nil { + t.Fatal(err) + } + checks := []struct { + name string + got, want any + }{ + {"provider from global", cfg.LLM.Provider, "anthropic"}, + {"model from repo", cfg.LLM.Model, "claude-opus-5-5"}, + {"effort from repo", cfg.OCR.Effort, "high"}, + {"min severity from repo", cfg.Review.MinSeverity, "high"}, + {"max comments from env", cfg.Review.MaxComments, 5}, + {"timeout from env", cfg.OCR.Timeout, 5 * time.Minute}, + {"suggestions from env", cfg.Review.Suggestions, false}, + {"authors from env", strings.Join(cfg.Skip.Authors, "|"), "renovate-bot|dependabot"}, + {"labels from global", strings.Join(cfg.Skip.Labels, "|"), "skip-me"}, + {"default kept", cfg.GitLab.TokenEnv, "PRUEFBYTE_GITLAB_TOKEN"}, + {"exclude from repo", strings.Join(cfg.OCR.Exclude, "|"), "docs/**"}, + } + for _, c := range checks { + if c.got != c.want { + t.Errorf("%s: got %v, want %v", c.name, c.got, c.want) + } + } +} + +func TestRepoFileCannotRedirectSecrets(t *testing.T) { + for _, doc := range []string{ + "llm:\n url: https://evil.example/v1\n", + "llm:\n provider: my-proxy\n", + "llm:\n api_key_env: CI_JOB_TOKEN\n", + "gitlab:\n url: https://evil.example\n", + "ocr:\n binary: /bin/sh\n", + "ocr:\n extra_args: [--tools, x.json]\n", + } { + cfg := Default() + if err := cfg.ApplyRepo([]byte(doc)); err == nil { + t.Errorf("ApplyRepo(%q) succeeded, want error", doc) + } + } +} + +func TestUnknownKeyRejected(t *testing.T) { + cfg := Default() + if err := cfg.ApplyGlobal([]byte("review:\n max_coments: 3\n")); err == nil { + t.Fatal("typo in key was accepted") + } +} + +func TestEmptyFiles(t *testing.T) { + cfg := Default() + if err := cfg.ApplyGlobal(nil); err != nil { + t.Fatal(err) + } + if err := cfg.ApplyRepo([]byte("# only a comment\n")); err != nil { + t.Fatal(err) + } +} + +func TestValidate(t *testing.T) { + base := func() Config { + c := Default() + c.LLM.Provider, c.LLM.Model = "openai", "gpt-5" + return c + } + if err := base().Validate(); err != nil { + t.Fatalf("valid config rejected: %v", err) + } + cases := map[string]func(*Config){ + "missing model": func(c *Config) { c.LLM.Model = "" }, + "custom w/o protocol": func(c *Config) { c.LLM.Provider = "gateway"; c.LLM.URL = "https://x" }, + "custom w/o url": func(c *Config) { c.LLM.Provider = "gateway"; c.LLM.Protocol = "openai" }, + "bad severity": func(c *Config) { c.Review.MinSeverity = "urgent" }, + "bad gate": func(c *Config) { c.Review.FailOnSeverity = "bad" }, + "bad effort": func(c *Config) { c.OCR.Effort = "max" }, + "rules and rule file": func(c *Config) { c.OCR.RuleFile = "r.json"; c.OCR.Rules = []Rule{{"**", "x"}} }, + "bad title regex": func(c *Config) { c.Skip.TitleRegex = []string{"("} }, + "negative max comment": func(c *Config) { c.Review.MaxComments = -1 }, + } + for name, mutate := range cases { + c := base() + mutate(&c) + if err := c.Validate(); err == nil { + t.Errorf("%s: accepted", name) + } + } + c := base() + c.LLM.Provider, c.LLM.Protocol, c.LLM.URL = "ollama", "openai", "http://localhost:11434/v1" + if err := c.Validate(); err != nil { + t.Errorf("custom provider rejected: %v", err) + } +} + +func TestEnvBadValue(t *testing.T) { + cfg := Default() + if err := cfg.ApplyEnv(env(map[string]string{"PRUEFBYTE_REVIEW_MAX_COMMENTS": "many"})); err == nil { + t.Fatal("non-numeric int accepted") + } +} diff --git a/internal/config/env.go b/internal/config/env.go new file mode 100644 index 0000000..358427e --- /dev/null +++ b/internal/config/env.go @@ -0,0 +1,87 @@ +package config + +import ( + "fmt" + "reflect" + "strconv" + "strings" + "time" +) + +// EnvPrefix prefixes every override, e.g. PRUEFBYTE_LLM_MODEL or PRUEFBYTE_REVIEW_MAX_COMMENTS. +const EnvPrefix = "PRUEFBYTE_" + +var durationType = reflect.TypeOf(time.Duration(0)) + +// ApplyEnv overrides scalar and list fields from PRUEFBYTE_
_ variables. +// Lists are comma-separated. Map fields (llm.extra_body, llm.extra_headers) and +// ocr.rules can only be set in a config file. +func (c *Config) ApplyEnv(lookup func(string) (string, bool)) error { + return applyEnv(reflect.ValueOf(c).Elem(), EnvPrefix, lookup) +} + +func applyEnv(v reflect.Value, prefix string, lookup func(string) (string, bool)) error { + t := v.Type() + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + tag := strings.Split(f.Tag.Get("yaml"), ",")[0] + if tag == "" || tag == "-" { + continue + } + name := prefix + strings.ToUpper(tag) + fv := v.Field(i) + if f.Type.Kind() == reflect.Struct { + if err := applyEnv(fv, name+"_", lookup); err != nil { + return err + } + continue + } + raw, ok := lookup(name) + if !ok { + continue + } + if err := setFromString(fv, raw); err != nil { + return fmt.Errorf("%s: %w", name, err) + } + } + return nil +} + +func setFromString(fv reflect.Value, raw string) error { + if fv.Type() == durationType { + d, err := time.ParseDuration(raw) + if err != nil { + return err + } + fv.SetInt(int64(d)) + return nil + } + switch fv.Kind() { + case reflect.String: + fv.SetString(raw) + case reflect.Int: + n, err := strconv.Atoi(strings.TrimSpace(raw)) + if err != nil { + return err + } + fv.SetInt(int64(n)) + case reflect.Bool: + b, err := strconv.ParseBool(strings.TrimSpace(raw)) + if err != nil { + return err + } + fv.SetBool(b) + case reflect.Slice: + if fv.Type().Elem().Kind() != reflect.String { + return nil + } + items := []string{} + for _, s := range strings.Split(raw, ",") { + if s = strings.TrimSpace(s); s != "" { + items = append(items, s) + } + } + fv.Set(reflect.ValueOf(items)) + } + return nil +} diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go new file mode 100644 index 0000000..69d6782 --- /dev/null +++ b/internal/gitlab/client.go @@ -0,0 +1,233 @@ +// Package gitlab is a small wrapper around client-go, bound to one merge request. +// It exposes only what the review needs, in plain types that are easy to fake in tests. +package gitlab + +import ( + "context" + "errors" + "fmt" + "net/http" + + gl "gitlab.com/gitlab-org/api/client-go" +) + +type MR struct { + IID int64 + Title string + Description string + SourceBranch string + TargetBranch string + Author string + Draft bool + Labels []string + WebURL string + BaseSHA string + StartSHA string + HeadSHA string +} + +type FileDiff struct { + OldPath, NewPath string + Diff string + NewFile bool + DeletedFile bool + RenamedFile bool +} + +type Position struct { + BaseSHA, StartSHA, HeadSHA string + OldPath, NewPath string + OldLine, NewLine int +} + +type Note struct { + ID int64 + AuthorID int64 + Body string + System bool + Resolvable bool + Resolved bool + Position *Position +} + +type Discussion struct { + ID string + IndividualNote bool + Notes []Note +} + +// API is what the review orchestrator uses. *Client implements it. +type API interface { + CurrentUserID(ctx context.Context) (int64, error) + GetMR(ctx context.Context) (*MR, error) + ListDiffs(ctx context.Context) ([]FileDiff, error) + ListDiscussions(ctx context.Context) ([]Discussion, error) + CreateDiscussion(ctx context.Context, body string, pos *Position) error + CreateNote(ctx context.Context, body string) error + UpdateNote(ctx context.Context, noteID int64, body string) error + ReplyAndResolve(ctx context.Context, discussionID, body string) error + Compare(ctx context.Context, from, to string) ([]FileDiff, error) +} + +type Client struct { + gl *gl.Client + project string + mrIID int64 +} + +// New creates a client authenticated with the bot's personal access token. +// client-go retries 429 and 5xx responses with backoff. +func New(baseURL, token, project string, mrIID int64) (*Client, error) { + c, err := gl.NewClient(token, gl.WithBaseURL(baseURL)) + if err != nil { + return nil, err + } + return &Client{gl: c, project: project, mrIID: mrIID}, nil +} + +// IsBadRequest reports whether err is a GitLab 400, which is how GitLab rejects +// a discussion position it cannot resolve against the diff. +func IsBadRequest(err error) bool { + var er *gl.ErrorResponse + return errors.As(err, &er) && er.Response != nil && er.Response.StatusCode == http.StatusBadRequest +} + +func (c *Client) CurrentUserID(ctx context.Context) (int64, error) { + u, _, err := c.gl.Users.CurrentUser(gl.WithContext(ctx)) + if err != nil { + return 0, fmt.Errorf("resolving bot user (is the token valid?): %w", err) + } + return u.ID, nil +} + +func (c *Client) GetMR(ctx context.Context) (*MR, error) { + m, _, err := c.gl.MergeRequests.GetMergeRequest(c.project, c.mrIID, nil, gl.WithContext(ctx)) + if err != nil { + return nil, fmt.Errorf("fetching merge request !%d: %w", c.mrIID, err) + } + mr := &MR{ + IID: m.IID, + Title: m.Title, + Description: m.Description, + SourceBranch: m.SourceBranch, + TargetBranch: m.TargetBranch, + Draft: m.Draft, + Labels: m.Labels, + WebURL: m.WebURL, + BaseSHA: m.DiffRefs.BaseSha, + StartSHA: m.DiffRefs.StartSha, + HeadSHA: m.DiffRefs.HeadSha, + } + if m.Author != nil { + mr.Author = m.Author.Username + } + return mr, nil +} + +func (c *Client) ListDiffs(ctx context.Context) ([]FileDiff, error) { + var out []FileDiff + opt := &gl.ListMergeRequestDiffsOptions{ListOptions: gl.ListOptions{PerPage: 100, Page: 1}} + for { + page, resp, err := c.gl.MergeRequests.ListMergeRequestDiffs(c.project, c.mrIID, opt, gl.WithContext(ctx)) + if err != nil { + return nil, fmt.Errorf("listing merge request diffs: %w", err) + } + for _, d := range page { + out = append(out, FileDiff{OldPath: d.OldPath, NewPath: d.NewPath, Diff: d.Diff, + NewFile: d.NewFile, DeletedFile: d.DeletedFile, RenamedFile: d.RenamedFile}) + } + if resp.NextPage == 0 { + return out, nil + } + opt.Page = resp.NextPage + } +} + +func (c *Client) ListDiscussions(ctx context.Context) ([]Discussion, error) { + var out []Discussion + opt := &gl.ListMergeRequestDiscussionsOptions{ListOptions: gl.ListOptions{PerPage: 100, Page: 1}} + for { + page, resp, err := c.gl.Discussions.ListMergeRequestDiscussions(c.project, c.mrIID, opt, gl.WithContext(ctx)) + if err != nil { + return nil, fmt.Errorf("listing discussions: %w", err) + } + for _, d := range page { + disc := Discussion{ID: d.ID, IndividualNote: d.IndividualNote} + for _, n := range d.Notes { + note := Note{ID: n.ID, AuthorID: n.Author.ID, Body: n.Body, System: n.System, + Resolvable: n.Resolvable, Resolved: n.Resolved} + if p := n.Position; p != nil { + note.Position = &Position{BaseSHA: p.BaseSHA, StartSHA: p.StartSHA, HeadSHA: p.HeadSHA, + OldPath: p.OldPath, NewPath: p.NewPath, OldLine: int(p.OldLine), NewLine: int(p.NewLine)} + } + disc.Notes = append(disc.Notes, note) + } + out = append(out, disc) + } + if resp.NextPage == 0 { + return out, nil + } + opt.Page = resp.NextPage + } +} + +func (c *Client) CreateDiscussion(ctx context.Context, body string, pos *Position) error { + opt := &gl.CreateMergeRequestDiscussionOptions{Body: gl.Ptr(body)} + if pos != nil { + po := &gl.PositionOptions{ + PositionType: gl.Ptr("text"), + BaseSHA: gl.Ptr(pos.BaseSHA), + StartSHA: gl.Ptr(pos.StartSHA), + HeadSHA: gl.Ptr(pos.HeadSHA), + NewPath: gl.Ptr(pos.NewPath), + OldPath: gl.Ptr(pos.OldPath), + } + if pos.NewLine > 0 { + po.NewLine = gl.Ptr(int64(pos.NewLine)) + } + if pos.OldLine > 0 { + po.OldLine = gl.Ptr(int64(pos.OldLine)) + } + opt.Position = po + } + _, _, err := c.gl.Discussions.CreateMergeRequestDiscussion(c.project, c.mrIID, opt, gl.WithContext(ctx)) + return err +} + +func (c *Client) CreateNote(ctx context.Context, body string) error { + _, _, err := c.gl.Notes.CreateMergeRequestNote(c.project, c.mrIID, + &gl.CreateMergeRequestNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx)) + return err +} + +func (c *Client) UpdateNote(ctx context.Context, noteID int64, body string) error { + _, _, err := c.gl.Notes.UpdateMergeRequestNote(c.project, c.mrIID, noteID, + &gl.UpdateMergeRequestNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx)) + return err +} + +func (c *Client) ReplyAndResolve(ctx context.Context, discussionID, body string) error { + if body != "" { + if _, _, err := c.gl.Discussions.AddMergeRequestDiscussionNote(c.project, c.mrIID, discussionID, + &gl.AddMergeRequestDiscussionNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx)); err != nil { + return err + } + } + _, _, err := c.gl.Discussions.ResolveMergeRequestDiscussion(c.project, c.mrIID, discussionID, + &gl.ResolveMergeRequestDiscussionOptions{Resolved: gl.Ptr(true)}, gl.WithContext(ctx)) + return err +} + +func (c *Client) Compare(ctx context.Context, from, to string) ([]FileDiff, error) { + cmp, _, err := c.gl.Repositories.Compare(c.project, + &gl.CompareOptions{From: gl.Ptr(from), To: gl.Ptr(to), Straight: gl.Ptr(true)}, gl.WithContext(ctx)) + if err != nil { + return nil, fmt.Errorf("comparing %s..%s: %w", from, to, err) + } + out := make([]FileDiff, 0, len(cmp.Diffs)) + for _, d := range cmp.Diffs { + out = append(out, FileDiff{OldPath: d.OldPath, NewPath: d.NewPath, Diff: d.Diff, + NewFile: d.NewFile, DeletedFile: d.DeletedFile, RenamedFile: d.RenamedFile}) + } + return out, nil +} diff --git a/internal/gitlab/client_test.go b/internal/gitlab/client_test.go new file mode 100644 index 0000000..c6b8b2c --- /dev/null +++ b/internal/gitlab/client_test.go @@ -0,0 +1,67 @@ +package gitlab + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" +) + +func TestCreateDiscussionPayload(t *testing.T) { + var got map[string]any + var auth, path string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + auth, path = r.Header.Get("Private-Token"), r.URL.EscapedPath() + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &got) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":"abc","notes":[]}`)) + })) + defer srv.Close() + + c, err := New(srv.URL+"/api/v4", "glpat-bot", "group/proj", 7) + if err != nil { + t.Fatal(err) + } + err = c.CreateDiscussion(context.Background(), "body", &Position{ + BaseSHA: "b", StartSHA: "s", HeadSHA: "h", OldPath: "old.go", NewPath: "new.go", NewLine: 12, + }) + if err != nil { + t.Fatal(err) + } + if auth != "glpat-bot" { + t.Errorf("PRIVATE-TOKEN header = %q", auth) + } + if path != "/api/v4/projects/group%2Fproj/merge_requests/7/discussions" { + t.Errorf("path = %s", path) + } + pos, _ := got["position"].(map[string]any) + want := map[string]any{"position_type": "text", "base_sha": "b", "start_sha": "s", "head_sha": "h", + "old_path": "old.go", "new_path": "new.go", "new_line": float64(12)} + for k, v := range want { + if pos[k] != v { + t.Errorf("position.%s = %v, want %v", k, pos[k], v) + } + } + if _, ok := pos["old_line"]; ok { + t.Error("old_line must be omitted for added lines") + } + if got["body"] != "body" { + t.Errorf("body = %v", got["body"]) + } +} + +func TestIsBadRequest(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"400 Bad request - Note {:line_code=>[\"can't be blank\"]}"}`)) + })) + defer srv.Close() + c, _ := New(srv.URL+"/api/v4", "t", "1", 1) + err := c.CreateDiscussion(context.Background(), "x", &Position{NewPath: "a", NewLine: 1}) + if !IsBadRequest(err) { + t.Fatalf("IsBadRequest(%v) = false", err) + } +} diff --git a/internal/gitlab/dryrun.go b/internal/gitlab/dryrun.go new file mode 100644 index 0000000..cb3f025 --- /dev/null +++ b/internal/gitlab/dryrun.go @@ -0,0 +1,37 @@ +package gitlab + +import ( + "context" + "fmt" + "io" +) + +// DryRun passes reads through to the wrapped API and prints writes instead of sending them. +type DryRun struct { + API + Out io.Writer +} + +func (d DryRun) CreateDiscussion(_ context.Context, body string, pos *Position) error { + loc := "(no position)" + if pos != nil { + loc = fmt.Sprintf("%s new_line=%d old_line=%d", pos.NewPath, pos.NewLine, pos.OldLine) + } + fmt.Fprintf(d.Out, "--- [dry-run] discussion on %s\n%s\n\n", loc, body) + return nil +} + +func (d DryRun) CreateNote(_ context.Context, body string) error { + fmt.Fprintf(d.Out, "--- [dry-run] note\n%s\n\n", body) + return nil +} + +func (d DryRun) UpdateNote(_ context.Context, id int64, body string) error { + fmt.Fprintf(d.Out, "--- [dry-run] update note %d\n%s\n\n", id, body) + return nil +} + +func (d DryRun) ReplyAndResolve(_ context.Context, id, body string) error { + fmt.Fprintf(d.Out, "--- [dry-run] resolve discussion %s: %s\n\n", id, body) + return nil +} diff --git a/internal/gitutil/git.go b/internal/gitutil/git.go new file mode 100644 index 0000000..5e02e22 --- /dev/null +++ b/internal/gitutil/git.go @@ -0,0 +1,67 @@ +// Package gitutil runs the few git commands pruefbyte needs in the CI checkout. +package gitutil + +import ( + "bytes" + "context" + "fmt" + "os/exec" + "strings" +) + +type Repo struct{ Dir string } + +func (r Repo) run(ctx context.Context, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", append([]string{"-c", "safe.directory=*"}, args...)...) + cmd.Dir = r.Dir + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + return out, fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return out, nil +} + +// ShowFile returns path at rev, or (nil, false) if it does not exist there. +func (r Repo) ShowFile(ctx context.Context, rev, path string) ([]byte, bool, error) { + if _, err := r.run(ctx, "cat-file", "-e", rev+":"+path); err != nil { + if _, cerr := r.run(ctx, "cat-file", "-e", rev+"^{commit}"); cerr != nil { + return nil, false, fmt.Errorf("commit %.8s is not in the clone (set GIT_DEPTH: 0): %w", rev, cerr) + } + return nil, false, nil + } + out, err := r.run(ctx, "show", rev+":"+path) + return out, err == nil, err +} + +// EnsureCommits fetches any of the given commits missing from the clone. +func (r Repo) EnsureCommits(ctx context.Context, fetchRefs []string, shas ...string) error { + missing := func() []string { + var m []string + for _, s := range shas { + if _, err := r.run(ctx, "cat-file", "-e", s+"^{commit}"); err != nil { + m = append(m, s) + } + } + return m + } + m := missing() + if len(m) == 0 { + return nil + } + // Shallow clones: deepen to full history; then try fetching the commits and MR refs directly. + if out, _ := r.run(ctx, "rev-parse", "--is-shallow-repository"); strings.TrimSpace(string(out)) == "true" { + _, _ = r.run(ctx, "fetch", "--quiet", "--unshallow", "origin") + } + for _, ref := range append(missing(), fetchRefs...) { + if len(missing()) == 0 { + break + } + _, _ = r.run(ctx, "fetch", "--quiet", "origin", ref) + } + if m = missing(); len(m) > 0 { + return fmt.Errorf("commits %s are not available in %s; use GIT_DEPTH: 0 in the CI job", strings.Join(m, ", "), r.Dir) + } + return nil +} diff --git a/internal/gitutil/git_test.go b/internal/gitutil/git_test.go new file mode 100644 index 0000000..bafd77b --- /dev/null +++ b/internal/gitutil/git_test.go @@ -0,0 +1,56 @@ +package gitutil + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + +func TestShowFileAndEnsureCommits(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + dir := t.TempDir() + git(t, dir, "init", "-q") + os.WriteFile(filepath.Join(dir, ".pruefbyte.yml"), []byte("review:\n max_comments: 3\n"), 0o644) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "base") + base := git(t, dir, "rev-parse", "HEAD") + os.WriteFile(filepath.Join(dir, ".pruefbyte.yml"), []byte("review:\n max_comments: 99\n"), 0o644) + git(t, dir, "commit", "-qam", "head") + head := git(t, dir, "rev-parse", "HEAD") + + r := Repo{Dir: dir} + ctx := context.Background() + data, found, err := r.ShowFile(ctx, base, ".pruefbyte.yml") + if err != nil || !found || !strings.Contains(string(data), "3") { + t.Fatalf("ShowFile at base = %q %v %v", data, found, err) + } + if _, found, err := r.ShowFile(ctx, base, "missing.yml"); err != nil || found { + t.Errorf("missing file: found=%v err=%v", found, err) + } + if _, _, err := r.ShowFile(ctx, strings.Repeat("0", 40), ".pruefbyte.yml"); err == nil { + t.Error("unknown commit should be an error") + } + if err := r.EnsureCommits(ctx, nil, base, head); err != nil { + t.Errorf("EnsureCommits with present commits: %v", err) + } + if err := r.EnsureCommits(ctx, nil, strings.Repeat("1", 40)); err == nil { + t.Error("EnsureCommits with an unknown commit and no remote should fail") + } +} diff --git a/internal/ocr/ocr_test.go b/internal/ocr/ocr_test.go new file mode 100644 index 0000000..c8cd2c5 --- /dev/null +++ b/internal/ocr/ocr_test.go @@ -0,0 +1,143 @@ +package ocr + +import ( + "strings" + "testing" + + "pruefbyte/internal/config" +) + +func TestParseResult(t *testing.T) { + doc := `{ + "status": "success", + "llm": {"provider": "anthropic", "model": "claude-sonnet-5"}, + "summary": {"files_reviewed": 2, "comments": 1, "total_tokens": 100, "input_tokens": 80, "output_tokens": 20, "elapsed": "5s"}, + "comments": [{"path": "a.go", "content": "nil deref", "start_line": 3, "end_line": 4, + "existing_code": "x.y", "suggestion_code": "if x != nil { x.y }", "thinking": "…", + "severity": "high", "category": "bug"}], + "session_id": "abc" +}` + r, err := ParseResult([]byte(doc)) + if err != nil { + t.Fatal(err) + } + if !r.Complete() || r.Failed() { + t.Errorf("status handling wrong for %q", r.Status) + } + c := r.Comments[0] + if c.Path != "a.go" || c.StartLine != 3 || c.EndLine != 4 || c.Severity != "high" || c.Category != "bug" { + t.Errorf("unexpected comment %+v", c) + } + if _, err := ParseResult([]byte(`{"comments": []}`)); err == nil { + t.Error("document without status accepted") + } + if _, err := ParseResult([]byte(`not json`)); err == nil { + t.Error("garbage accepted") + } + partial, _ := ParseResult([]byte(`{"status":"partial","comments":[]}`)) + if partial.Complete() { + t.Error("partial counted as complete") + } +} + +func settingsMap(kv [][2]string) map[string]string { + m := map[string]string{} + for _, p := range kv { + m[p[0]] = p[1] + } + return m +} + +func TestConfigSettingsBuiltin(t *testing.T) { + kv, err := ConfigSettings(config.LLM{ + Provider: "anthropic", Model: "claude-sonnet-5", URL: "https://proxy/v1", + ExtraBody: map[string]any{"thinking": map[string]any{"type": "disabled"}}, + ExtraHeaders: map[string]string{"b": "2", "a": "1"}, + TimeoutSec: 600, + }, "sk-test", "German") + if err != nil { + t.Fatal(err) + } + m := settingsMap(kv) + want := map[string]string{ + "provider": "anthropic", + "model": "claude-sonnet-5", + "providers.anthropic.api_key": "sk-test", + "providers.anthropic.url": "https://proxy/v1", + "providers.anthropic.extra_body": `{"thinking":{"type":"disabled"}}`, + "providers.anthropic.extra_headers": "a=1,b=2", + "providers.anthropic.timeout_sec": "600", + "language": "German", + } + for k, v := range want { + if m[k] != v { + t.Errorf("%s = %q, want %q", k, m[k], v) + } + } + if kv[0][0] != "provider" { + t.Error("provider must be set first") + } +} + +func TestConfigSettingsCustom(t *testing.T) { + m := settingsMap(must(ConfigSettings(config.LLM{ + Provider: "ollama", Model: "qwen3:32b", URL: "http://127.0.0.1:11434/v1", Protocol: "openai", + }, "", ""))) + if m["custom_providers.ollama.protocol"] != "openai" || m["custom_providers.ollama.model"] != "qwen3:32b" || + m["custom_providers.ollama.url"] == "" { + t.Errorf("custom provider settings wrong: %v", m) + } + if m["custom_providers.ollama.api_key"] == "" { + t.Error("custom providers need a placeholder api_key") + } + if _, ok := m["language"]; ok { + t.Error("empty language should not be set") + } +} + +func must[T any](v T, err error) T { + if err != nil { + panic(err) + } + return v +} + +func TestReviewArgs(t *testing.T) { + args := ReviewOptions{ + RepoDir: "/repo", From: "base", To: "head", Provider: "openai", Model: "gpt-5", + Effort: "high", Concurrency: 4, Exclude: []string{"a/**", "b"}, RuleFile: "/tmp/rule.json", + BackgroundFile: "/tmp/bg.md", ExtraArgs: []string{"--no-filter"}, + }.Args("/tmp/out.json") + got := strings.Join(args, " ") + for _, want := range []string{ + "review --repo /repo --from base --to head --format json --audience agent --output /tmp/out.json", + "--provider openai", "--model gpt-5", "--effort high", "--concurrency 4", + "--exclude a/**,b", "--rule /tmp/rule.json", "--background-file /tmp/bg.md", "--no-filter", + } { + if !strings.Contains(got, want) { + t.Errorf("args %q missing %q", got, want) + } + } + if strings.Contains(got, "--max-tokens-budget") { + t.Error("zero budget should not be passed") + } +} + +func TestRuleJSON(t *testing.T) { + b, err := RuleJSON([]config.Rule{{Path: "**/*.go", Rule: "check errors"}}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(b), `"path": "**/*.go"`) || !strings.Contains(string(b), `"rule": "check errors"`) { + t.Errorf("unexpected rule.json: %s", b) + } +} + +func TestTailBuffer(t *testing.T) { + var tb tailBuffer + tb.Write([]byte(strings.Repeat("x", tailSize))) + tb.Write([]byte("END")) + if s := tb.String(); len(s) != tailSize || !strings.HasSuffix(s, "END") { + t.Errorf("tail buffer kept %d bytes", len(s)) + } +} diff --git a/internal/ocr/result.go b/internal/ocr/result.go new file mode 100644 index 0000000..20b7880 --- /dev/null +++ b/internal/ocr/result.go @@ -0,0 +1,74 @@ +package ocr + +import ( + "encoding/json" + "fmt" +) + +// Result is the document printed by `ocr review --format json`. +type Result struct { + Status string `json:"status"` + Message string `json:"message,omitempty"` + LLM LLMInfo `json:"llm"` + Summary *Summary `json:"summary,omitempty"` + Comments []Comment `json:"comments"` + Warnings []Warning `json:"warnings,omitempty"` + Session string `json:"session_id,omitempty"` +} + +type LLMInfo struct { + Provider string `json:"provider,omitempty"` + Model string `json:"model"` +} + +type Summary struct { + FilesReviewed int `json:"files_reviewed"` + Comments int `json:"comments"` + TotalTokens int `json:"total_tokens"` + InputTokens int `json:"input_tokens"` + OutputTokens int `json:"output_tokens"` + Elapsed string `json:"elapsed"` +} + +// Comment is one finding. Severity and Category are not part of OCR's documented +// schema but appear in practice; both may be empty. +type Comment struct { + Path string `json:"path"` + Content string `json:"content"` + StartLine int `json:"start_line"` + EndLine int `json:"end_line"` + ExistingCode string `json:"existing_code,omitempty"` + SuggestionCode string `json:"suggestion_code,omitempty"` + Severity string `json:"severity,omitempty"` + Category string `json:"category,omitempty"` +} + +type Warning struct { + File string `json:"file,omitempty"` + Type string `json:"type,omitempty"` + Message string `json:"message,omitempty"` +} + +func ParseResult(data []byte) (*Result, error) { + var r Result + if err := json.Unmarshal(data, &r); err != nil { + return nil, fmt.Errorf("parsing ocr JSON output: %w", err) + } + if r.Status == "" { + return nil, fmt.Errorf("ocr JSON output has no status field") + } + return &r, nil +} + +// Complete reports whether every file was reviewed, i.e. an absent finding +// means "no issue" rather than "not looked at". +func (r *Result) Complete() bool { + switch r.Status { + case "success", "completed_with_warnings", "complete", "skipped": + return true + } + return false +} + +// Failed reports whether the run produced nothing usable. +func (r *Result) Failed() bool { return r.Status == "failed" } diff --git a/internal/ocr/rules.go b/internal/ocr/rules.go new file mode 100644 index 0000000..2b3b65d --- /dev/null +++ b/internal/ocr/rules.go @@ -0,0 +1,14 @@ +package ocr + +import ( + "encoding/json" + + "pruefbyte/internal/config" +) + +// RuleJSON renders configured rules in OCR's rule.json schema. +func RuleJSON(rules []config.Rule) ([]byte, error) { + return json.MarshalIndent(struct { + Rules []config.Rule `json:"rules"` + }{rules}, "", " ") +} diff --git a/internal/ocr/runner.go b/internal/ocr/runner.go new file mode 100644 index 0000000..1bcef2c --- /dev/null +++ b/internal/ocr/runner.go @@ -0,0 +1,256 @@ +// Package ocr drives the OpenCodeReview CLI (`ocr`) in an isolated home directory. +package ocr + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "sort" + "strconv" + "strings" + + "pruefbyte/internal/config" +) + +type Runner struct { + Binary string + // Home is a private directory used as HOME for ocr, so its config.json and + // session logs never touch (or read from) the runner's real home directory. + Home string + // Log receives ocr's stderr as it runs. + Log io.Writer +} + +// NewRunner creates a runner with a fresh temporary home directory. Call Close to remove it. +func NewRunner(binary string, log io.Writer) (*Runner, error) { + home, err := os.MkdirTemp("", "pruefbyte-ocr-") + if err != nil { + return nil, err + } + return &Runner{Binary: binary, Home: home, Log: log}, nil +} + +func (r *Runner) Close() error { return os.RemoveAll(r.Home) } + +func (r *Runner) env() []string { + env := make([]string, 0, len(os.Environ())+6) + for _, kv := range os.Environ() { + k, _, _ := strings.Cut(kv, "=") + switch strings.ToUpper(k) { + // Keep ocr from reading secrets or config meant for pruefbyte or other tools. + case "HOME", "USERPROFILE", "XDG_CONFIG_HOME": + continue + } + if strings.HasPrefix(k, "PRUEFBYTE_") || strings.HasPrefix(k, "OCR_LLM_") { + continue + } + env = append(env, kv) + } + return append(env, + "HOME="+r.Home, "USERPROFILE="+r.Home, + // The CI checkout is often owned by another uid; git refuses to work in it otherwise. + "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=safe.directory", "GIT_CONFIG_VALUE_0=*", + "TERM=dumb", "NO_COLOR=1", + ) +} + +// ConfigSettings returns the `ocr config set` key/value pairs for the LLM settings. +func ConfigSettings(llm config.LLM, apiKey, language string) ([][2]string, error) { + section := "providers." + llm.Provider + if !config.IsBuiltinProvider(llm.Provider) { + section = "custom_providers." + llm.Provider + } + var kv [][2]string + add := func(k, v string) { + if v != "" { + kv = append(kv, [2]string{k, v}) + } + } + add("provider", llm.Provider) + add("model", llm.Model) + if strings.HasPrefix(section, "custom_") { + add(section+".protocol", llm.Protocol) + add(section+".model", llm.Model) + if apiKey == "" && llm.Protocol != "anthropic-bedrock" { + // OCR requires a non-empty key for custom providers; keyless local endpoints accept anything. + apiKey = "none" + } + } + add(section+".url", llm.URL) + add(section+".api_key", apiKey) + if llm.TimeoutSec > 0 { + add(section+".timeout_sec", strconv.Itoa(llm.TimeoutSec)) + } + add(section+".aws_region", llm.AWSRegion) + add(section+".aws_profile", llm.AWSProfile) + if len(llm.ExtraBody) > 0 { + b, err := json.Marshal(llm.ExtraBody) + if err != nil { + return nil, fmt.Errorf("llm.extra_body: %w", err) + } + add(section+".extra_body", string(b)) + } + if len(llm.ExtraHeaders) > 0 { + keys := make([]string, 0, len(llm.ExtraHeaders)) + for k := range llm.ExtraHeaders { + keys = append(keys, k) + } + sort.Strings(keys) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, k+"="+llm.ExtraHeaders[k]) + } + add(section+".extra_headers", strings.Join(parts, ",")) + } + add("language", language) + return kv, nil +} + +// Configure writes the LLM settings into ocr's config via `ocr config set`, so the +// config file format stays ocr's own business. +func (r *Runner) Configure(ctx context.Context, llm config.LLM, apiKey, language string) error { + settings, err := ConfigSettings(llm, apiKey, language) + if err != nil { + return err + } + for _, s := range settings { + cmd := exec.CommandContext(ctx, r.Binary, "config", "set", s[0], s[1]) + cmd.Env = r.env() + if out, err := cmd.CombinedOutput(); err != nil { + msg := strings.ReplaceAll(string(out), apiKey, "***") + return fmt.Errorf("ocr config set %s: %w: %s", s[0], err, strings.TrimSpace(msg)) + } + } + return nil +} + +type ReviewOptions struct { + RepoDir string + From, To string + Provider, Model string + Effort string + Concurrency int + MaxTokensBudget int + Exclude []string + RuleFile string + BackgroundFile string + ExtraArgs []string +} + +func (o ReviewOptions) Args(outFile string) []string { + args := []string{"review", "--repo", o.RepoDir, "--from", o.From, "--to", o.To, + "--format", "json", "--audience", "agent", "--output", outFile} + if o.Provider != "" { + args = append(args, "--provider", o.Provider) + } + if o.Model != "" { + args = append(args, "--model", o.Model) + } + if o.Effort != "" { + args = append(args, "--effort", o.Effort) + } + if o.Concurrency > 0 { + args = append(args, "--concurrency", strconv.Itoa(o.Concurrency)) + } + if o.MaxTokensBudget > 0 { + args = append(args, "--max-tokens-budget", strconv.Itoa(o.MaxTokensBudget)) + } + if len(o.Exclude) > 0 { + args = append(args, "--exclude", strings.Join(o.Exclude, ",")) + } + if o.RuleFile != "" { + args = append(args, "--rule", o.RuleFile) + } + if o.BackgroundFile != "" { + args = append(args, "--background-file", o.BackgroundFile) + } + return append(args, o.ExtraArgs...) +} + +// RunError carries ocr's stderr tail when the review process fails. +type RunError struct { + Err error + Stderr string +} + +func (e *RunError) Error() string { + return fmt.Sprintf("ocr review failed: %v\n%s", e.Err, e.Stderr) +} + +func (e *RunError) Unwrap() error { return e.Err } + +// Review runs `ocr review` and returns the parsed JSON result. The raw output is +// also returned so callers can keep it as a CI artifact. +func (r *Runner) Review(ctx context.Context, o ReviewOptions) (*Result, []byte, error) { + outFile := filepath.Join(r.Home, "result.json") + cmd := exec.CommandContext(ctx, r.Binary, o.Args(outFile)...) + cmd.Dir = o.RepoDir + cmd.Env = r.env() + var stderr tailBuffer + cmd.Stdout = r.Log + cmd.Stderr = io.MultiWriter(r.Log, &stderr) + runErr := cmd.Run() + + raw, readErr := os.ReadFile(outFile) + if runErr != nil { + if ctx.Err() != nil { + runErr = fmt.Errorf("%w (ocr.timeout reached?)", ctx.Err()) + } + // A failed run can still leave a usable document (e.g. partial results). + if readErr == nil { + if res, err := ParseResult(raw); err == nil && len(res.Comments) > 0 { + res.Status = "partial" + return res, raw, nil + } + } + return nil, raw, &RunError{Err: runErr, Stderr: stderr.String()} + } + if readErr != nil { + return nil, nil, &RunError{Err: fmt.Errorf("ocr wrote no result file: %w", readErr), Stderr: stderr.String()} + } + res, err := ParseResult(raw) + if err != nil { + return nil, raw, &RunError{Err: err, Stderr: stderr.String()} + } + return res, raw, nil +} + +// WriteTemp writes data to a file in the runner's home and returns its path. +func (r *Runner) WriteTemp(name string, data []byte) (string, error) { + p := filepath.Join(r.Home, name) + return p, os.WriteFile(p, data, 0o600) +} + +// tailBuffer keeps the last 8 KiB written to it. +type tailBuffer struct{ buf bytes.Buffer } + +const tailSize = 8 << 10 + +func (t *tailBuffer) Write(p []byte) (int, error) { + t.buf.Write(p) + if extra := t.buf.Len() - tailSize; extra > 0 { + t.buf.Next(extra) + } + return len(p), nil +} + +func (t *tailBuffer) String() string { return strings.TrimSpace(t.buf.String()) } + +var ErrNotFound = errors.New("ocr binary not found") + +// Version returns `ocr version` output, or ErrNotFound. +func (r *Runner) Version(ctx context.Context) (string, error) { + if _, err := exec.LookPath(r.Binary); err != nil { + return "", fmt.Errorf("%w: %q (install @alibaba-group/open-code-review or set ocr.binary)", ErrNotFound, r.Binary) + } + cmd := exec.CommandContext(ctx, r.Binary, "version") + cmd.Env = r.env() + out, err := cmd.CombinedOutput() + return strings.TrimSpace(string(out)), err +} diff --git a/internal/review/diff.go b/internal/review/diff.go new file mode 100644 index 0000000..71ccd5e --- /dev/null +++ b/internal/review/diff.go @@ -0,0 +1,126 @@ +package review + +import ( + "regexp" + "strconv" + "strings" + + "pruefbyte/internal/gitlab" +) + +var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) + +// lineInfo describes a new-file line that is visible in the MR diff. +type lineInfo struct { + added bool + oldLine int // set for unchanged context lines +} + +type fileDiff struct { + oldPath string + // lines maps new-file line numbers to diff positions. nil means GitLab + // returned no patch (too large / collapsed), so lines are unknown. + lines map[int]lineInfo +} + +// diffIndex maps new paths of the MR diff to the lines GitLab can anchor comments on. +type diffIndex map[string]*fileDiff + +func buildDiffIndex(diffs []gitlab.FileDiff) diffIndex { + idx := diffIndex{} + for _, d := range diffs { + if d.DeletedFile { + continue + } + fd := &fileDiff{oldPath: d.OldPath} + if strings.TrimSpace(d.Diff) != "" { + fd.lines = parseNewLines(d.Diff) + } + idx[d.NewPath] = fd + } + return idx +} + +func parseNewLines(patch string) map[int]lineInfo { + lines := map[int]lineInfo{} + oldLn, newLn, inHunk := 0, 0, false + for _, l := range strings.Split(patch, "\n") { + if m := hunkHeader.FindStringSubmatch(l); m != nil { + oldLn, _ = strconv.Atoi(m[1]) + newLn, _ = strconv.Atoi(m[3]) + inHunk = true + continue + } + if !inHunk || strings.HasPrefix(l, `\`) { + continue + } + switch { + case strings.HasPrefix(l, "+"): + lines[newLn] = lineInfo{added: true} + newLn++ + case strings.HasPrefix(l, "-"): + oldLn++ + case strings.HasPrefix(l, " "): + lines[newLn] = lineInfo{oldLine: oldLn} + oldLn++ + newLn++ + } + } + return lines +} + +// anchor picks the line a finding on [start,end] should be attached to: the last +// line of the range that is part of the diff. ok is false when no line qualifies. +func (fd *fileDiff) anchor(start, end int) (line int, info lineInfo, ok bool) { + if end <= 0 { + end = start + } + if start <= 0 || start > end { + start = end + } + if end <= 0 { + return 0, lineInfo{}, false + } + if fd.lines == nil { + // Unknown patch: try the end line as an added line and let GitLab decide. + return end, lineInfo{added: true}, true + } + for l := end; l >= start; l-- { + if info, ok := fd.lines[l]; ok { + return l, info, true + } + } + return 0, lineInfo{}, false +} + +// touchedOldRanges returns, per old path, the old-side line ranges of each hunk in +// a diff. Used to tell whether code a previous comment pointed at has changed. +func touchedOldRanges(diffs []gitlab.FileDiff) map[string][][2]int { + out := map[string][][2]int{} + for _, d := range diffs { + if d.NewFile { + continue + } + if d.DeletedFile || d.RenamedFile || strings.TrimSpace(d.Diff) == "" { + out[d.OldPath] = append(out[d.OldPath], [2]int{1, int(^uint(0) >> 1)}) + continue + } + for _, l := range strings.Split(d.Diff, "\n") { + m := hunkHeader.FindStringSubmatch(l) + if m == nil { + continue + } + start, _ := strconv.Atoi(m[1]) + count := 1 + if m[2] != "" { + count, _ = strconv.Atoi(m[2]) + } + end := start + count - 1 + if count == 0 { + end = start // pure insertion after line `start` + } + out[d.OldPath] = append(out[d.OldPath], [2]int{start, end}) + } + } + return out +} diff --git a/internal/review/diff_test.go b/internal/review/diff_test.go new file mode 100644 index 0000000..209a5d0 --- /dev/null +++ b/internal/review/diff_test.go @@ -0,0 +1,89 @@ +package review + +import ( + "strings" + "testing" + + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +func TestParseNewLines(t *testing.T) { + patch := "@@ -10,4 +10,5 @@ func x() {\n a\n-b\n+B\n+C\n c\n\\ No newline at end of file\n@@ -30,2 +31,2 @@\n d\n-e\n+E\n" + got := parseNewLines(patch) + want := map[int]lineInfo{ + 10: {oldLine: 10}, 11: {added: true}, 12: {added: true}, 13: {oldLine: 12}, + 31: {oldLine: 30}, 32: {added: true}, + } + if len(got) != len(want) { + t.Fatalf("got %v", got) + } + for k, v := range want { + if got[k] != v { + t.Errorf("line %d: got %+v, want %+v", k, got[k], v) + } + } +} + +func TestAnchor(t *testing.T) { + fd := &fileDiff{lines: parseNewLines(patchA)} + cases := []struct { + start, end, want int + ok bool + }{ + {3, 4, 4, true}, + {4, 9, 5, true}, // last diff line inside the range + {8, 9, 0, false}, + {0, 3, 3, true}, + {3, 0, 3, true}, + {0, 0, 0, false}, + } + for _, c := range cases { + got, _, ok := fd.anchor(c.start, c.end) + if got != c.want || ok != c.ok { + t.Errorf("anchor(%d,%d) = %d,%v want %d,%v", c.start, c.end, got, ok, c.want, c.ok) + } + } + unknown := &fileDiff{} + if l, info, ok := unknown.anchor(5, 7); !ok || l != 7 || !info.added { + t.Error("unknown patch should anchor on end line") + } +} + +func TestTouchedOldRanges(t *testing.T) { + r := touchedOldRanges([]gitlab.FileDiff{ + {OldPath: "a.go", Diff: "@@ -3,2 +3,0 @@\n-x\n-y\n@@ -20,0 +19,1 @@\n+z\n"}, + {OldPath: "gone.go", DeletedFile: true}, + {OldPath: "new.go", NewFile: true, Diff: "@@ -0,0 +1 @@\n+n\n"}, + }) + if got := r["a.go"]; len(got) != 2 || got[0] != [2]int{3, 4} || got[1] != [2]int{20, 20} { + t.Errorf("a.go ranges %v", got) + } + if len(r["gone.go"]) != 1 { + t.Error("deleted file should count as fully touched") + } + if _, ok := r["new.go"]; ok { + t.Error("new files have no old lines") + } +} + +func TestFenceAndBodies(t *testing.T) { + c := ocr.Comment{Path: "a.go", Content: "use a fence", StartLine: 2, EndLine: 2, + ExistingCode: "x", SuggestionCode: "```go\ny\n```"} + body := inlineBody(c, "abc", 2, true) + if !strings.Contains(body, "````suggestion:-0+0\n```go") { + t.Errorf("nested fence not escaped:\n%s", body) + } + if extractFingerprint(body) != "abc" { + t.Error("fingerprint not recoverable") + } + // Anchored on a different line than OCR's end line: no applicable suggestion. + c.EndLine = 5 + body = inlineBody(c, "abc", 3, true) + if strings.Contains(body, "suggestion:") || !strings.Contains(body, "Suggested change") || !strings.Contains(body, "lines 2–5") { + t.Errorf("unexpected body:\n%s", body) + } + if body := inlineBody(c, "abc", 3, false); strings.Contains(body, "y\n") { + t.Error("suggestions disabled but code rendered") + } +} diff --git a/internal/review/filter.go b/internal/review/filter.go new file mode 100644 index 0000000..0218b3b --- /dev/null +++ b/internal/review/filter.go @@ -0,0 +1,88 @@ +package review + +import ( + "fmt" + "regexp" + "slices" + "sort" + "strings" + + "pruefbyte/internal/config" + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +// filterFindings drops findings below review.min_severity or outside +// review.categories and orders the rest most severe first. Findings with no +// severity or category are kept: there is nothing to filter them on. +func filterFindings(cs []ocr.Comment, r config.Review) []ocr.Comment { + minRank := config.SeverityRank(r.MinSeverity) + var cats []string + for _, c := range r.Categories { + cats = append(cats, strings.ToLower(strings.TrimSpace(c))) + } + var out []ocr.Comment + for _, c := range cs { + if strings.TrimSpace(c.Path) == "" || strings.TrimSpace(c.Content) == "" { + continue + } + if rank := config.SeverityRank(c.Severity); rank > 0 && rank < minRank { + continue + } + if cat := strings.ToLower(strings.TrimSpace(c.Category)); len(cats) > 0 && cat != "" && !slices.Contains(cats, cat) { + continue + } + out = append(out, c) + } + sort.SliceStable(out, func(i, j int) bool { + ri, rj := config.SeverityRank(out[i].Severity), config.SeverityRank(out[j].Severity) + if ri != rj { + return ri > rj + } + if out[i].Path != out[j].Path { + return out[i].Path < out[j].Path + } + return out[i].StartLine < out[j].StartLine + }) + return out +} + +// skipReason returns why the MR should not be reviewed, or "". +func skipReason(s config.Skip, mr *gitlab.MR) string { + if s.Drafts && mr.Draft { + return "merge request is a draft (skip.drafts)" + } + for _, l := range mr.Labels { + if slices.Contains(s.Labels, l) { + return fmt.Sprintf("label %q (skip.labels)", l) + } + } + if slices.Contains(s.Authors, mr.Author) { + return fmt.Sprintf("author %q (skip.authors)", mr.Author) + } + for _, re := range s.TitleRegex { + if m, _ := regexp.MatchString(re, mr.Title); m { + return fmt.Sprintf("title matches %q (skip.title_regex)", re) + } + } + if matchAny(s.SourceBranches, mr.SourceBranch) { + return fmt.Sprintf("source branch %q (skip.source_branches)", mr.SourceBranch) + } + if matchAny(s.TargetBranches, mr.TargetBranch) { + return fmt.Sprintf("target branch %q (skip.target_branches)", mr.TargetBranch) + } + return "" +} + +// matchAny matches branch names exactly or as regular expressions anchored at both ends. +func matchAny(patterns []string, s string) bool { + for _, p := range patterns { + if p == s { + return true + } + if re, err := regexp.Compile("^(?:" + p + ")$"); err == nil && re.MatchString(s) { + return true + } + } + return false +} diff --git a/internal/review/format.go b/internal/review/format.go new file mode 100644 index 0000000..c3cb279 --- /dev/null +++ b/internal/review/format.go @@ -0,0 +1,175 @@ +package review + +import ( + "crypto/sha1" + "encoding/hex" + "fmt" + "regexp" + "strings" + + "pruefbyte/internal/ocr" +) + +const ( + fpPrefix = "" +) + +var fpPattern = regexp.MustCompile(``) + +// fingerprint identifies a finding independently of its line numbers, so the same +// issue is recognised after unrelated lines shift. +func fingerprint(c ocr.Comment) string { + norm := strings.Join(strings.Fields(strings.ToLower(c.Content)), " ") + h := sha1.Sum([]byte(c.Path + "\x00" + norm)) + return hex.EncodeToString(h[:8]) +} + +func extractFingerprint(body string) string { + if m := fpPattern.FindStringSubmatch(body); m != nil { + return m[1] + } + return "" +} + +var severityIcon = map[string]string{"critical": "🔴", "high": "🟠", "medium": "🟡", "low": "🔵"} + +func badge(c ocr.Comment) string { + sev := strings.ToLower(strings.TrimSpace(c.Severity)) + cat := strings.ToLower(strings.TrimSpace(c.Category)) + var parts []string + if sev != "" { + icon := severityIcon[sev] + if icon == "" { + icon = "⚪" + } + parts = append(parts, icon+" "+strings.ToUpper(sev[:1])+sev[1:]) + } + if cat != "" { + parts = append(parts, cat) + } + if len(parts) == 0 { + return "" + } + return "**" + strings.Join(parts, " · ") + "**" +} + +func lineLabel(c ocr.Comment) string { + switch { + case c.StartLine > 0 && c.EndLine > c.StartLine: + return fmt.Sprintf("lines %d–%d", c.StartLine, c.EndLine) + case c.EndLine > 0: + return fmt.Sprintf("line %d", c.EndLine) + case c.StartLine > 0: + return fmt.Sprintf("line %d", c.StartLine) + } + return "" +} + +// fence returns a backtick fence longer than any backtick run inside code. +func fence(code string) string { + f := "```" + for strings.Contains(code, f) { + f += "`" + } + return f +} + +// inlineBody renders a finding as a diff discussion anchored at anchorLine. +func inlineBody(c ocr.Comment, fp string, anchorLine int, suggestions bool) string { + var b strings.Builder + head := badge(c) + if anchorLine != c.EndLine || c.EndLine > c.StartLine && c.StartLine > 0 { + if l := lineLabel(c); l != "" { + if head != "" { + head += " " + } + head += "(" + l + ")" + } + } + if head != "" { + b.WriteString(head + "\n\n") + } + b.WriteString(strings.TrimSpace(c.Content)) + b.WriteString("\n") + + if code := strings.TrimRight(c.SuggestionCode, "\n"); code != "" && suggestions { + f := fence(code) + // A GitLab suggestion replaces the anchor line plus N lines above it, which + // only matches OCR's range when we anchored on its last line. + if c.ExistingCode != "" && anchorLine == c.EndLine && c.StartLine > 0 && c.StartLine <= c.EndLine { + fmt.Fprintf(&b, "\n%ssuggestion:-%d+0\n%s\n%s\n", f, c.EndLine-c.StartLine, code, f) + } else { + fmt.Fprintf(&b, "\nSuggested change:\n\n%s\n%s\n%s\n", f, code, f) + } + } + fmt.Fprintf(&b, "\n%s%s -->", fpPrefix, fp) + return b.String() +} + +type summaryData struct { + Model string + HeadSHA string + Stats Stats + Unplaced []ocr.Comment + Overflow []ocr.Comment + MaxComments int + Warnings []ocr.Warning + Failure string + Incomplete bool +} + +func summaryBody(d summaryData) string { + var b strings.Builder + b.WriteString(summaryMarker + "\n### 🔍 pruefbyte review\n\n") + sha := d.HeadSHA + if len(sha) > 8 { + sha = sha[:8] + } + if d.Failure != "" { + fmt.Fprintf(&b, "⚠️ The review of `%s` failed.\n\n
Error\n\n```\n%s\n```\n\n
\n", sha, d.Failure) + return b.String() + } + fmt.Fprintf(&b, "Reviewed `%s`", sha) + if d.Model != "" { + fmt.Fprintf(&b, " with `%s`", d.Model) + } + fmt.Fprintf(&b, ": %d new inline comment(s), %d already posted earlier.\n", d.Stats.Posted, d.Stats.Duplicates) + if d.Incomplete { + b.WriteString("\n⚠️ The review did not complete for every file; see the job log.\n") + } + list := func(title string, cs []ocr.Comment) { + if len(cs) == 0 { + return + } + fmt.Fprintf(&b, "\n#### %s\n\n", title) + for _, c := range cs { + loc := c.Path + if l := lineLabel(c); l != "" { + loc += " (" + l + ")" + } + line := "- `" + loc + "`" + if bd := badge(c); bd != "" { + line += " " + bd + } + content := strings.Join(strings.Fields(c.Content), " ") + if r := []rune(content); len(r) > 400 { + content = string(r[:400]) + "…" + } + b.WriteString(line + ": " + content + "\n") + } + } + list("Findings outside the diff", d.Unplaced) + list(fmt.Sprintf("Further findings (over the limit of %d inline comments)", d.MaxComments), d.Overflow) + if len(d.Warnings) > 0 { + b.WriteString("\n
OCR warnings\n\n") + for _, w := range d.Warnings { + fmt.Fprintf(&b, "- %s %s\n", w.File, strings.Join(strings.Fields(w.Message), " ")) + } + b.WriteString("\n
\n") + } + if len(d.Unplaced)+len(d.Overflow)+len(d.Warnings) == 0 && !d.Incomplete { + b.WriteString("\nAll findings are posted inline.\n") + } + return b.String() +} diff --git a/internal/review/review.go b/internal/review/review.go new file mode 100644 index 0000000..72f7e1d --- /dev/null +++ b/internal/review/review.go @@ -0,0 +1,367 @@ +// Package review runs OCR on a merge request and publishes the findings as +// GitLab diff discussions from the bot account. +package review + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + + "pruefbyte/internal/config" + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +// Reviewer runs the review engine. *ocr.Runner implements it. +type Reviewer interface { + Review(ctx context.Context, o ocr.ReviewOptions) (*ocr.Result, []byte, error) + WriteTemp(name string, data []byte) (string, error) +} + +type Deps struct { + GitLab gitlab.API + OCR Reviewer + // LoadConfig returns the effective config, given the MR's base commit + // (where the repository's config file is read from). + LoadConfig func(ctx context.Context, baseSHA string) (config.Config, error) + // EnsureCommits makes sure the given commits exist in the local clone. + EnsureCommits func(ctx context.Context, shas ...string) error + // SaveResult optionally keeps OCR's raw JSON, e.g. as a CI artifact. + SaveResult func(raw []byte) error + // Redact masks secrets in text that gets posted to the MR. + Redact func(string) string + Log io.Writer +} + +type Options struct { + RepoDir string + // ExpectedHeadSHA is the commit the pipeline was started for. If the MR has + // moved on since, the run is skipped: a newer pipeline will review it. + ExpectedHeadSHA string +} + +type Stats struct { + Findings int + Posted int + Duplicates int + Unplaced int + Overflow int + Failed int + Resolved int +} + +type Outcome struct { + Skipped string + Stats Stats + GateFailed bool + // GateReason explains GateFailed. + GateReason string +} + +var ErrReviewFailed = errors.New("review failed") + +func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { + logf := func(format string, a ...any) { fmt.Fprintf(d.Log, "[pruefbyte] "+format+"\n", a...) } + + botID, err := d.GitLab.CurrentUserID(ctx) + if err != nil { + return nil, err + } + mr, err := d.GitLab.GetMR(ctx) + if err != nil { + return nil, err + } + if mr.BaseSHA == "" || mr.HeadSHA == "" { + return nil, fmt.Errorf("merge request !%d has no diff refs yet; retry once GitLab has computed the diff", mr.IID) + } + cfg, err := d.LoadConfig(ctx, mr.BaseSHA) + if err != nil { + return nil, err + } + if reason := skipReason(cfg.Skip, mr); reason != "" { + logf("skipping !%d: %s", mr.IID, reason) + return &Outcome{Skipped: reason}, nil + } + if opts.ExpectedHeadSHA != "" && opts.ExpectedHeadSHA != mr.HeadSHA { + reason := fmt.Sprintf("merge request head moved from %.8s to %.8s; a newer pipeline reviews it", opts.ExpectedHeadSHA, mr.HeadSHA) + logf("skipping !%d: %s", mr.IID, reason) + return &Outcome{Skipped: reason}, nil + } + if d.EnsureCommits != nil { + if err := d.EnsureCommits(ctx, mr.BaseSHA, mr.HeadSHA); err != nil { + return nil, err + } + } + + existing, err := d.GitLab.ListDiscussions(ctx) + if err != nil { + return nil, err + } + state := collectBotState(existing, botID) + + logf("reviewing !%d %s (%.8s..%.8s)", mr.IID, mr.Title, mr.BaseSHA, mr.HeadSHA) + ro, err := reviewOptions(cfg, mr, opts.RepoDir, d.OCR) + if err != nil { + return nil, err + } + reviewCtx, cancel := context.WithTimeout(ctx, cfg.OCR.Timeout) + res, raw, err := d.OCR.Review(reviewCtx, ro) + cancel() + if raw != nil && d.SaveResult != nil { + if serr := d.SaveResult(raw); serr != nil { + logf("warning: saving OCR result: %v", serr) + } + } + if err == nil && res.Failed() { + err = fmt.Errorf("ocr reported status %q: %s", res.Status, res.Message) + } + if err != nil { + if cfg.Review.PostFailures { + msg := err.Error() + var re *ocr.RunError + if errors.As(err, &re) { + msg = re.Err.Error() + "\n\n" + lastLines(re.Stderr, 30) + } + if d.Redact != nil { + msg = d.Redact(msg) + } + if perr := upsertSummary(ctx, d.GitLab, state, summaryBody(summaryData{HeadSHA: mr.HeadSHA, Failure: msg})); perr != nil { + logf("warning: posting failure note: %v", perr) + } + } + return nil, fmt.Errorf("%w: %v", ErrReviewFailed, err) + } + logf("ocr finished with status %s: %d finding(s)", res.Status, len(res.Comments)) + + diffs, err := d.GitLab.ListDiffs(ctx) + if err != nil { + return nil, err + } + idx := buildDiffIndex(diffs) + + findings := filterFindings(res.Comments, cfg.Review) + out := &Outcome{} + st := &out.Stats + st.Findings = len(findings) + current := map[string]bool{} + var unplaced, overflow []ocr.Comment + + for _, c := range findings { + fp := fingerprint(c) + if current[fp] { + continue // the same finding twice in one run + } + current[fp] = true + if state.fingerprints[fp] { + st.Duplicates++ + continue + } + if cfg.Review.MaxComments > 0 && st.Posted >= cfg.Review.MaxComments { + overflow = append(overflow, c) + continue + } + fd := idx[c.Path] + if fd == nil { + unplaced = append(unplaced, c) + continue + } + line, info, ok := fd.anchor(c.StartLine, c.EndLine) + if !ok { + unplaced = append(unplaced, c) + continue + } + pos := &gitlab.Position{ + BaseSHA: mr.BaseSHA, StartSHA: mr.StartSHA, HeadSHA: mr.HeadSHA, + OldPath: fd.oldPath, NewPath: c.Path, NewLine: line, + } + if !info.added { + pos.OldLine = info.oldLine + } + err := d.GitLab.CreateDiscussion(ctx, inlineBody(c, fp, line, cfg.Review.Suggestions), pos) + switch { + case err == nil: + st.Posted++ + case gitlab.IsBadRequest(err): + logf("GitLab rejected the position %s:%d, listing it in the summary: %v", c.Path, line, err) + unplaced = append(unplaced, c) + default: + logf("warning: posting comment on %s:%d: %v", c.Path, line, err) + st.Failed++ + } + } + st.Unplaced, st.Overflow = len(unplaced), len(overflow) + + if cfg.Review.ResolveOutdated { + if res.Complete() && st.Failed == 0 { + st.Resolved = resolveOutdated(ctx, d.GitLab, state, current, idx, mr.HeadSHA, logf) + } else { + logf("not resolving earlier threads: this review was incomplete") + } + } + + summary := summaryData{ + Model: res.LLM.Model, HeadSHA: mr.HeadSHA, Stats: *st, + Unplaced: unplaced, Overflow: overflow, MaxComments: cfg.Review.MaxComments, + Warnings: res.Warnings, Incomplete: !res.Complete(), + } + if len(unplaced)+len(overflow)+len(res.Warnings) > 0 || !res.Complete() || state.summary != nil { + if err := upsertSummary(ctx, d.GitLab, state, summaryBody(summary)); err != nil { + logf("warning: posting summary note: %v", err) + } + } + + if cfg.Review.FailOnSeverity != "" { + gate := config.SeverityRank(cfg.Review.FailOnSeverity) + for _, c := range findings { + if config.SeverityRank(c.Severity) >= gate { + out.GateFailed = true + out.GateReason = fmt.Sprintf("%s finding in %s (%s): review.fail_on_severity is %s", + strings.ToLower(c.Severity), c.Path, lineLabel(c), cfg.Review.FailOnSeverity) + break + } + } + } + if st.Failed > 0 { + return out, fmt.Errorf("%d comment(s) could not be posted", st.Failed) + } + return out, nil +} + +func reviewOptions(cfg config.Config, mr *gitlab.MR, repoDir string, r Reviewer) (ocr.ReviewOptions, error) { + ro := ocr.ReviewOptions{ + RepoDir: repoDir, From: mr.BaseSHA, To: mr.HeadSHA, + Provider: cfg.LLM.Provider, Model: cfg.LLM.Model, + Effort: cfg.OCR.Effort, Concurrency: cfg.OCR.Concurrency, + MaxTokensBudget: cfg.OCR.MaxTokensBudget, Exclude: cfg.OCR.Exclude, + RuleFile: cfg.OCR.RuleFile, ExtraArgs: cfg.OCR.ExtraArgs, + } + if len(cfg.OCR.Rules) > 0 { + data, err := ocr.RuleJSON(cfg.OCR.Rules) + if err != nil { + return ro, err + } + if ro.RuleFile, err = r.WriteTemp("rule.json", data); err != nil { + return ro, err + } + } + if cfg.OCR.UseMRDescriptionAsBackground { + bg := "# " + mr.Title + "\n\n" + mr.Description + if r := []rune(bg); len(r) > 8000 { + bg = string(r[:8000]) + } + p, err := r.WriteTemp("background.md", []byte(bg)) + if err != nil { + return ro, err + } + ro.BackgroundFile = p + } + return ro, nil +} + +type botState struct { + fingerprints map[string]bool + open []botThread + summary *gitlab.Note +} + +type botThread struct { + discussionID string + fingerprint string + pos *gitlab.Position +} + +// collectBotState finds everything the bot posted earlier: fingerprints (resolved +// or not, so a dismissed finding is not raised again), open threads, and the summary note. +func collectBotState(ds []gitlab.Discussion, botID int64) botState { + s := botState{fingerprints: map[string]bool{}} + for _, d := range ds { + if len(d.Notes) == 0 { + continue + } + first := d.Notes[0] + if first.AuthorID != botID || first.System { + continue + } + if strings.HasPrefix(first.Body, summaryMarker) { + n := first + s.summary = &n + continue + } + fp := extractFingerprint(first.Body) + if fp == "" { + continue + } + s.fingerprints[fp] = true + if first.Resolvable && !first.Resolved && first.Position != nil { + s.open = append(s.open, botThread{discussionID: d.ID, fingerprint: fp, pos: first.Position}) + } + } + return s +} + +// resolveOutdated resolves the bot's open threads whose finding was not reported +// again and whose code changed since the comment was made. Requiring the change +// keeps an LLM that merely missed the issue this time from closing the thread. +func resolveOutdated(ctx context.Context, api gitlab.API, s botState, current map[string]bool, idx diffIndex, headSHA string, logf func(string, ...any)) int { + resolved := 0 + compares := map[string]map[string][][2]int{} + for _, t := range s.open { + if current[t.fingerprint] || t.pos.HeadSHA == headSHA { + continue + } + changed := false + if idx[t.pos.NewPath] == nil { + changed = true // the file is no longer part of the MR + } else { + ranges, ok := compares[t.pos.HeadSHA] + if !ok { + diffs, err := api.Compare(ctx, t.pos.HeadSHA, headSHA) + if err != nil { + logf("warning: %v", err) + } + ranges = touchedOldRanges(diffs) + compares[t.pos.HeadSHA] = ranges + } + line := t.pos.NewLine + if line == 0 { + line = t.pos.OldLine + } + for _, r := range ranges[t.pos.NewPath] { + if line >= r[0] && line <= r[1] { + changed = true + break + } + } + } + if !changed { + continue + } + msg := fmt.Sprintf("Resolved automatically: the code changed in `%.8s` and this finding was not reported again.", headSHA) + if err := api.ReplyAndResolve(ctx, t.discussionID, msg); err != nil { + logf("warning: resolving discussion %s: %v", t.discussionID, err) + continue + } + resolved++ + } + return resolved +} + +func upsertSummary(ctx context.Context, api gitlab.API, s botState, body string) error { + if s.summary != nil { + if s.summary.Body == body { + return nil + } + return api.UpdateNote(ctx, s.summary.ID, body) + } + return api.CreateNote(ctx, body) +} + +func lastLines(s string, n int) string { + lines := strings.Split(strings.TrimSpace(s), "\n") + if len(lines) > n { + lines = lines[len(lines)-n:] + } + return strings.Join(lines, "\n") +} diff --git a/internal/review/review_test.go b/internal/review/review_test.go new file mode 100644 index 0000000..f41afa9 --- /dev/null +++ b/internal/review/review_test.go @@ -0,0 +1,403 @@ +package review + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "strings" + "testing" + + gl "gitlab.com/gitlab-org/api/client-go" + + "pruefbyte/internal/config" + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +const botID = 42 + +// fakeGitLab keeps discussions in memory, like a tiny GitLab. +type fakeGitLab struct { + mr gitlab.MR + diffs []gitlab.FileDiff + compare map[string][]gitlab.FileDiff + discussions []gitlab.Discussion + // rejectLine makes CreateDiscussion fail with 400 for this new_line. + rejectLine int + nextID int64 + resolved []string + updates int +} + +func (f *fakeGitLab) CurrentUserID(context.Context) (int64, error) { return botID, nil } +func (f *fakeGitLab) GetMR(context.Context) (*gitlab.MR, error) { m := f.mr; return &m, nil } +func (f *fakeGitLab) ListDiffs(context.Context) ([]gitlab.FileDiff, error) { return f.diffs, nil } +func (f *fakeGitLab) ListDiscussions(context.Context) ([]gitlab.Discussion, error) { + return f.discussions, nil +} + +func (f *fakeGitLab) CreateDiscussion(_ context.Context, body string, pos *gitlab.Position) error { + if pos != nil && pos.NewLine == f.rejectLine { + return &gl.ErrorResponse{Response: &http.Response{StatusCode: 400}, Message: "400 {line_code: [can't be blank]}"} + } + f.nextID++ + p := *pos + f.discussions = append(f.discussions, gitlab.Discussion{ + ID: fmt.Sprintf("d%d", f.nextID), + Notes: []gitlab.Note{{ID: f.nextID, AuthorID: botID, Body: body, Resolvable: true, Position: &p}}, + }) + return nil +} + +func (f *fakeGitLab) CreateNote(_ context.Context, body string) error { + f.nextID++ + f.discussions = append(f.discussions, gitlab.Discussion{ + ID: fmt.Sprintf("d%d", f.nextID), IndividualNote: true, + Notes: []gitlab.Note{{ID: f.nextID, AuthorID: botID, Body: body}}, + }) + return nil +} + +func (f *fakeGitLab) UpdateNote(_ context.Context, id int64, body string) error { + for i := range f.discussions { + for j := range f.discussions[i].Notes { + if f.discussions[i].Notes[j].ID == id { + f.discussions[i].Notes[j].Body = body + f.updates++ + return nil + } + } + } + return errors.New("note not found") +} + +func (f *fakeGitLab) ReplyAndResolve(_ context.Context, id, _ string) error { + f.resolved = append(f.resolved, id) + for i := range f.discussions { + if f.discussions[i].ID == id { + f.discussions[i].Notes[0].Resolved = true + } + } + return nil +} + +func (f *fakeGitLab) Compare(_ context.Context, from, to string) ([]gitlab.FileDiff, error) { + return f.compare[from+".."+to], nil +} + +func (f *fakeGitLab) inline() []gitlab.Discussion { + var out []gitlab.Discussion + for _, d := range f.discussions { + if d.Notes[0].Position != nil { + out = append(out, d) + } + } + return out +} + +func (f *fakeGitLab) summary() string { + for _, d := range f.discussions { + if strings.HasPrefix(d.Notes[0].Body, summaryMarker) { + return d.Notes[0].Body + } + } + return "" +} + +type fakeOCR struct { + res *ocr.Result + err error + opts ocr.ReviewOptions +} + +func (f *fakeOCR) Review(_ context.Context, o ocr.ReviewOptions) (*ocr.Result, []byte, error) { + f.opts = o + return f.res, []byte("{}"), f.err +} +func (f *fakeOCR) WriteTemp(name string, _ []byte) (string, error) { return "/tmp/" + name, nil } + +// a.go: lines 1-2 context, 3-4 added, 5 context. b.go is new. +const patchA = "@@ -1,3 +1,5 @@\n ctx1\n ctx2\n+add3\n+add4\n ctx5\n" + +func newFake() *fakeGitLab { + return &fakeGitLab{ + mr: gitlab.MR{IID: 7, Title: "Add feature", BaseSHA: "base", StartSHA: "start", HeadSHA: "head1"}, + diffs: []gitlab.FileDiff{ + {OldPath: "a.go", NewPath: "a.go", Diff: patchA}, + {OldPath: "b.go", NewPath: "b.go", NewFile: true, Diff: "@@ -0,0 +1,2 @@\n+x\n+y\n"}, + }, + compare: map[string][]gitlab.FileDiff{}, + } +} + +func testConfig() config.Config { + c := config.Default() + c.LLM.Provider, c.LLM.Model = "anthropic", "claude-sonnet-5" + return c +} + +func deps(f *fakeGitLab, o *fakeOCR, cfg config.Config) Deps { + return Deps{ + GitLab: f, + OCR: o, + LoadConfig: func(context.Context, string) (config.Config, error) { return cfg, nil }, + Log: io.Discard, + } +} + +func result(cs ...ocr.Comment) *ocr.Result { + return &ocr.Result{Status: "success", LLM: ocr.LLMInfo{Model: "claude-sonnet-5"}, Comments: cs} +} + +func TestPostsInlineWithCorrectPositions(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "added line issue", StartLine: 3, EndLine: 4, Severity: "high", Category: "bug", + ExistingCode: "add3\nadd4", SuggestionCode: "fixed3\nfixed4"}, + ocr.Comment{Path: "a.go", Content: "context line issue", StartLine: 5, EndLine: 5, Severity: "low"}, + ocr.Comment{Path: "b.go", Content: "new file issue", StartLine: 2, EndLine: 2, Severity: "medium"}, + )} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{RepoDir: "/repo"}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Posted != 3 { + t.Fatalf("posted %d, want 3", out.Stats.Posted) + } + if o.opts.From != "base" || o.opts.To != "head1" || o.opts.BackgroundFile == "" { + t.Errorf("ocr options wrong: %+v", o.opts) + } + byPath := map[string]*gitlab.Position{} + var first string + for _, d := range f.inline() { + p := d.Notes[0].Position + byPath[fmt.Sprintf("%s:%d", p.NewPath, p.NewLine)] = p + if strings.Contains(d.Notes[0].Body, "added line issue") { + first = d.Notes[0].Body + } + if p.BaseSHA != "base" || p.StartSHA != "start" || p.HeadSHA != "head1" { + t.Errorf("bad diff refs in %+v", p) + } + } + if p := byPath["a.go:4"]; p == nil || p.OldLine != 0 { + t.Errorf("added line: want new_line 4 without old_line, got %+v", p) + } + if p := byPath["a.go:5"]; p == nil || p.OldLine != 3 { + t.Errorf("context line: want new_line 5 / old_line 3, got %+v", p) + } + if byPath["b.go:2"] == nil { + t.Error("new file comment missing") + } + if !strings.Contains(first, "```suggestion:-1+0\nfixed3\nfixed4\n```") { + t.Errorf("suggestion block missing:\n%s", first) + } + if !strings.Contains(first, "🟠 High · bug") { + t.Errorf("badge missing:\n%s", first) + } + if f.summary() != "" { + t.Error("summary note posted although everything went inline") + } +} + +func TestSecondRunPostsNoDuplicates(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "Issue One", StartLine: 3, EndLine: 3})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + // Same finding, lines shifted, whitespace and case changed. + o.res = result(ocr.Comment{Path: "a.go", Content: "issue one", StartLine: 4, EndLine: 4}) + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Posted != 0 || out.Stats.Duplicates != 1 || len(f.inline()) != 1 { + t.Errorf("stats %+v, %d threads", out.Stats, len(f.inline())) + } +} + +func TestOthersFingerprintsAreIgnored(t *testing.T) { + f := newFake() + c := ocr.Comment{Path: "a.go", Content: "x", StartLine: 3, EndLine: 3} + // A human pasted the bot's marker; it must not suppress the finding. + f.discussions = []gitlab.Discussion{{ID: "h", Notes: []gitlab.Note{{AuthorID: 1, Body: fpPrefix + fingerprint(c) + " -->"}}}} + out, err := Run(context.Background(), deps(f, &fakeOCR{res: result(c)}, testConfig()), Options{}) + if err != nil || out.Stats.Posted != 1 { + t.Fatalf("posted %d, err %v", out.Stats.Posted, err) + } +} + +func TestOutsideDiffAndRejectedGoToSummary(t *testing.T) { + f := newFake() + f.rejectLine = 2 // b.go:2 + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "far away", StartLine: 40, EndLine: 41}, + ocr.Comment{Path: "untouched.go", Content: "other file", StartLine: 1, EndLine: 1}, + ocr.Comment{Path: "b.go", Content: "rejected", StartLine: 2, EndLine: 2}, + )} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Unplaced != 3 || out.Stats.Posted != 0 { + t.Fatalf("stats %+v", out.Stats) + } + s := f.summary() + for _, want := range []string{"far away", "other file", "rejected", "Findings outside the diff"} { + if !strings.Contains(s, want) { + t.Errorf("summary misses %q:\n%s", want, s) + } + } + // A rerun updates the same note instead of adding one. + if _, err := Run(context.Background(), deps(f, o, testConfig()), Options{}); err != nil { + t.Fatal(err) + } + notes := 0 + for _, d := range f.discussions { + if strings.HasPrefix(d.Notes[0].Body, summaryMarker) { + notes++ + } + } + if notes != 1 { + t.Errorf("%d summary notes, want 1", notes) + } +} + +func TestFilterCapAndGate(t *testing.T) { + f := newFake() + cfg := testConfig() + cfg.Review.MinSeverity = "medium" + cfg.Review.MaxComments = 1 + cfg.Review.FailOnSeverity = "critical" + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "low one", StartLine: 3, EndLine: 3, Severity: "low"}, + ocr.Comment{Path: "a.go", Content: "medium one", StartLine: 4, EndLine: 4, Severity: "medium"}, + ocr.Comment{Path: "b.go", Content: "critical one", StartLine: 1, EndLine: 1, Severity: "critical"}, + )} + out, err := Run(context.Background(), deps(f, o, cfg), Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Findings != 2 || out.Stats.Posted != 1 || out.Stats.Overflow != 1 { + t.Errorf("stats %+v", out.Stats) + } + if !strings.Contains(f.inline()[0].Notes[0].Body, "critical one") { + t.Error("most severe finding should be posted first") + } + if !strings.Contains(f.summary(), "medium one") { + t.Error("overflow finding should be listed in the summary") + } + if !out.GateFailed { + t.Error("fail_on_severity=critical did not trip") + } +} + +func TestResolvesOnlyChangedThreads(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "will be fixed", StartLine: 3, EndLine: 3}, + ocr.Comment{Path: "a.go", Content: "code untouched", StartLine: 5, EndLine: 5}, + ocr.Comment{Path: "a.go", Content: "still there", StartLine: 4, EndLine: 4}, + )} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + // New push: line 3 of a.go changed; only "still there" is reported again. + f.mr.HeadSHA = "head2" + f.compare["head1..head2"] = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -3 +3 @@\n-add3\n+fix3\n"}} + o.res = result(ocr.Comment{Path: "a.go", Content: "still there", StartLine: 4, EndLine: 4}) + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Resolved != 1 || len(f.resolved) != 1 { + t.Fatalf("resolved %v", f.resolved) + } + for _, disc := range f.discussions { + n := disc.Notes[0] + if strings.Contains(n.Body, "will be fixed") != n.Resolved { + t.Errorf("thread %q resolved=%v", n.Body[:30], n.Resolved) + } + } +} + +func TestNoResolveOnIncompleteReview(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "x", StartLine: 3, EndLine: 3})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + f.mr.HeadSHA = "head2" + f.compare["head1..head2"] = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -3 +3 @@\n-a\n+b\n"}} + o.res = &ocr.Result{Status: "completed_with_errors"} + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Resolved != 0 { + t.Error("resolved threads after an incomplete review") + } + if !strings.Contains(f.summary(), "did not complete") { + t.Error("incomplete review not flagged in summary") + } +} + +func TestFailurePostsRedactedNote(t *testing.T) { + f := newFake() + o := &fakeOCR{err: &ocr.RunError{Err: errors.New("exit status 1"), Stderr: "auth failed for key sk-secret-123"}} + d := deps(f, o, testConfig()) + d.Redact = func(s string) string { return strings.ReplaceAll(s, "sk-secret-123", "***") } + _, err := Run(context.Background(), d, Options{}) + if !errors.Is(err, ErrReviewFailed) { + t.Fatalf("err = %v", err) + } + s := f.summary() + if !strings.Contains(s, "failed") || strings.Contains(s, "sk-secret-123") || !strings.Contains(s, "***") { + t.Errorf("failure note wrong:\n%s", s) + } +} + +func TestSkips(t *testing.T) { + f := newFake() + f.mr.Draft = true + o := &fakeOCR{res: result()} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{}) + if err != nil || out.Skipped == "" { + t.Fatalf("draft not skipped: %+v %v", out, err) + } + f.mr.Draft = false + out, _ = Run(context.Background(), deps(f, o, testConfig()), Options{ExpectedHeadSHA: "older"}) + if out.Skipped == "" { + t.Error("stale pipeline not skipped") + } + if o.opts.RepoDir != "" { + t.Error("ocr ran for a skipped MR") + } +} + +func TestSkipReason(t *testing.T) { + s := config.Skip{Labels: []string{"no-review"}, Authors: []string{"renovate"}, TitleRegex: []string{`^\[WIP\]`}, + SourceBranches: []string{"release/.*"}, TargetBranches: []string{"legacy"}} + cases := []struct { + mr gitlab.MR + skip bool + }{ + {gitlab.MR{Title: "ok", Author: "alice", SourceBranch: "feat", TargetBranch: "main"}, false}, + {gitlab.MR{Labels: []string{"no-review"}}, true}, + {gitlab.MR{Author: "renovate"}, true}, + {gitlab.MR{Title: "[WIP] thing"}, true}, + {gitlab.MR{SourceBranch: "release/1.2"}, true}, + {gitlab.MR{SourceBranch: "xrelease/1.2"}, false}, + {gitlab.MR{TargetBranch: "legacy"}, true}, + } + for _, c := range cases { + if got := skipReason(s, &c.mr) != ""; got != c.skip { + t.Errorf("%+v: skip=%v, want %v", c.mr, got, c.skip) + } + } +} diff --git a/pruefbyte.example.yml b/pruefbyte.example.yml new file mode 100644 index 0000000..6c69ebc --- /dev/null +++ b/pruefbyte.example.yml @@ -0,0 +1,49 @@ +# Global pruefbyte configuration (pass with --config or PRUEFBYTE_CONFIG). +# Every key can also be set as PRUEFBYTE_
_, e.g. PRUEFBYTE_REVIEW_MAX_COMMENTS=10. +# Precedence: defaults < this file < .pruefbyte.yml at the MR's base commit < env vars. + +gitlab: + url: "" # default: CI_SERVER_URL + token_env: PRUEFBYTE_GITLAB_TOKEN # env var holding the bot's PAT + +llm: + provider: anthropic # any OCR built-in provider; other names are custom providers + model: claude-sonnet-5 + api_key_env: PRUEFBYTE_LLM_API_KEY # env var holding the API key + url: "" # base URL override; required for custom providers + protocol: "" # custom providers: anthropic | openai | openai-responses | anthropic-bedrock + timeout_sec: 0 + extra_body: {} # e.g. {thinking: {type: disabled}} + extra_headers: {} + aws_region: "" # bedrock only + aws_profile: "" + +ocr: + binary: ocr + effort: medium # low | medium | high (review rounds) + language: English # language of the comments + timeout: 30m # whole OCR run + concurrency: 8 + max_tokens_budget: 0 # 0 = unlimited + exclude: [] # gitignore-style patterns + rules: [] # [{path: "**/*.go", rule: "Errors must be wrapped with %w"}] + rule_file: "" # or an OCR rule.json in the repo + use_mr_description_as_background: true + extra_args: [] # passed verbatim to `ocr review` + +review: + min_severity: low # low | medium | high | critical + categories: [] # bug, security, performance, maintainability, test, style, documentation, other + max_comments: 30 # 0 = unlimited; the rest go to the summary note + suggestions: true # GitLab "apply suggestion" blocks + resolve_outdated: true # resolve own threads once the flagged code changed + fail_on_severity: "" # e.g. critical: fail the job (exit 3) + post_failures: true # post a note when the review fails + +skip: + drafts: true + labels: [no-review] + authors: [] + title_regex: ['^\[WIP\]'] + source_branches: [] # exact names or regexes + target_branches: [] diff --git a/templates/pruefbyte.gitlab-ci.yml b/templates/pruefbyte.gitlab-ci.yml new file mode 100644 index 0000000..31f3b07 --- /dev/null +++ b/templates/pruefbyte.gitlab-ci.yml @@ -0,0 +1,38 @@ +# pruefbyte: AI review of merge requests, posted as inline comments by a bot account. +# +# Usage in a project's .gitlab-ci.yml: +# +# include: +# - project: /pruefbyte +# file: templates/pruefbyte.gitlab-ci.yml +# +# Required CI/CD variables (masked, ideally set at group level): +# PRUEFBYTE_GITLAB_TOKEN personal access token of the bot user (scope: api, role: Developer) +# PRUEFBYTE_LLM_API_KEY API key for the LLM provider +# PRUEFBYTE_LLM_PROVIDER e.g. anthropic, openai, dashscope (or set llm.provider in PRUEFBYTE_CONFIG) +# PRUEFBYTE_LLM_MODEL e.g. claude-sonnet-5 +# Optional: +# PRUEFBYTE_CONFIG path to a global config file (a "File" type CI variable works well) +# PRUEFBYTE_IMAGE image to run (default below) + +variables: + PRUEFBYTE_IMAGE: registry.example.com/tools/pruefbyte:latest + +pruefbyte-review: + stage: test + image: $PRUEFBYTE_IMAGE + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + variables: + GIT_DEPTH: "0" # the MR's base commit must be in the clone + PRUEFBYTE_ARTIFACT_DIR: .pruefbyte + interruptible: true + resource_group: pruefbyte-$CI_MERGE_REQUEST_IID + needs: [] + script: + - pruefbyte review + artifacts: + when: always + expire_in: 1 week + paths: + - .pruefbyte/ From 9be49398a3be7e6329b36e075aaed93d70b7511f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Mon, 28 Sep 2026 19:29:59 +0200 Subject: [PATCH 2/4] Expand .gitignore for Go builds, local config and tool state Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitignore | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/.gitignore b/.gitignore index f21b8f3..bd04c21 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,43 @@ +# Build output /pruefbyte /pruefbyte.exe +/bin/ +/dist/ +*.exe +*.exe~ +*.dll +*.so +*.dylib + +# Go test and profiling artifacts +*.test +*.out +coverage.* +*.coverprofile +/go.work +/go.work.sum + +# pruefbyte runtime artifacts (OCR results kept by the CI job) /.pruefbyte/ +/.ocr/ + +# Local configuration and secrets +.env +.env.* +!.env.example +/pruefbyte.yml +mise.local.toml +.mise.local.toml + +# Editor / tool state +.idea/ +.vscode/ +*.swp +*~ .serena/ +.claude/settings.local.json + +# OS files +.DS_Store +Thumbs.db +desktop.ini From b4e7c5719b3150bb0b53de3121987967b203faa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Mon, 5 Oct 2026 13:43:21 +0200 Subject: [PATCH 3/4] Harden review pipeline and leave run decisions to CI Fixes from code review: - Pass one generated --rule file built from the base commit, so a merge request's own .opencodereview/rule.json cannot exclude files or rewrite review instructions. - Place findings on removed lines when the quoted code was deleted; only offer suggestion blocks when existing_code matches the new lines. - Escape GitLab quick actions in LLM text; do not trust markers inside it. - Fetch the MR diff before running OCR; do not resend discussion POSTs after 5xx; count open bot threads against max_comments; list failed posts in the summary. - Fingerprint findings by path, category and quoted code instead of prose. - Kill the OCR process tree on timeout; never relabel failed runs. - Resolve threads only for files OCR reviewed, against unfiltered findings, on actually changed lines, and never re-resolve reopened ones. - Add missing OCR provider presets, fix arm64 image, cmd.exe argument injection, comma handling, OCR self-update and secret env leakage. - Skip merge-train and fork pipelines in the CI template. Remove pruefbyte's own skip filters (drafts, labels, authors, branches, titles). The CI job's rules decide when pruefbyte runs; the README shows the equivalent rules. Only the stale-head guard remains. Co-Authored-By: Claude Opus 5.5 (1M context) --- Dockerfile | 11 +- README.md | 51 ++++- cmd/pruefbyte/main.go | 10 +- cmd/pruefbyte/main_test.go | 6 +- internal/config/config.go | 43 ++-- internal/config/config_test.go | 36 +++- internal/gitlab/client.go | 65 +++--- internal/gitlab/client_test.go | 17 ++ internal/ocr/ocr_test.go | 114 ++++++++++- internal/ocr/result.go | 31 +++ internal/ocr/rules.go | 72 ++++++- internal/ocr/runner.go | 146 ++++++++++--- internal/review/diff.go | 145 ++++++++++--- internal/review/diff_test.go | 94 +++++++-- internal/review/filter.go | 43 ---- internal/review/format.go | 62 ++++-- internal/review/review.go | 222 ++++++++++++++------ internal/review/review_test.go | 326 ++++++++++++++++++++++++++---- pruefbyte.example.yml | 15 +- templates/pruefbyte.gitlab-ci.yml | 17 ++ 20 files changed, 1197 insertions(+), 329 deletions(-) diff --git a/Dockerfile b/Dockerfile index d271b35..a30f1a5 100644 --- a/Dockerfile +++ b/Dockerfile @@ -11,13 +11,16 @@ RUN CGO_ENABLED=0 go build -trimpath -ldflags "-s -w -X main.version=${VERSION}" FROM alpine:3 AS ocr ARG OCR_VERSION=v1.12.10 -ARG TARGETARCH=amd64 +# BuildKit sets TARGETARCH for the platform being built; a default here would +# override it (e.g. an amd64 binary in an arm64 image). +ARG TARGETARCH RUN apk add --no-cache curl \ + && arch="${TARGETARCH:-amd64}" \ && cd /tmp \ - && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/opencodereview-linux-${TARGETARCH}" \ + && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/opencodereview-linux-${arch}" \ && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/sha256sum.txt" \ - && grep " opencodereview-linux-${TARGETARCH}\$" sha256sum.txt | sha256sum -c - \ - && install -m 0755 "opencodereview-linux-${TARGETARCH}" /out-ocr + && grep " opencodereview-linux-${arch}\$" sha256sum.txt | sha256sum -c - \ + && install -m 0755 "opencodereview-linux-${arch}" /out-ocr FROM alpine:3 # OCR needs git >= 2.41; alpine:3 ships a current git. diff --git a/README.md b/README.md index 518cc2d..38955e7 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,15 @@ from a dedicated bot account. variables: PRUEFBYTE_IMAGE: registry.example.com/tools/pruefbyte:latest ``` + The job runs in merge request pipelines (not in merge train pipelines, and not in pipelines that run in a fork, which lack the CI/CD variables), in the `test` stage: if the project defines `stages:`, keep `test` or override the job's `stage:`. If the project's other jobs run in branch pipelines, switch to merge request pipelines while a merge request is open, as GitLab recommends. Otherwise every push runs two pipelines, and the merge check only looks at the merge request pipeline, which then holds nothing but this job: + ```yaml + workflow: + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + - if: $CI_COMMIT_BRANCH && $CI_OPEN_MERGE_REQUESTS + when: never + - if: $CI_COMMIT_BRANCH + ``` ## Configuration @@ -42,10 +51,10 @@ Settings are layered; later layers win: 1. Built-in defaults. 2. The global file, from `--config` or `PRUEFBYTE_CONFIG`. See [`pruefbyte.example.yml`](pruefbyte.example.yml) for every key. -3. `.pruefbyte.yml` in the repository, **read from the merge request's base commit**. A merge request can't change its own review settings: config changes take effect once they are merged. +3. `.pruefbyte.yml` in the repository, **read from the merge request's base commit**. A merge request can't change its own review settings: config changes take effect once they are merged. The same holds for OCR's own rule files, `.opencodereview/rule.json` and `ocr.rule_file`: pruefbyte reads them at the base commit and passes one rule file that keeps the merge request's copies from applying. 4. Environment variables `PRUEFBYTE_
_`, e.g. `PRUEFBYTE_REVIEW_MIN_SEVERITY=medium`. Lists are comma-separated. -The repository file may only set `llm.model`, `ocr.*` (except `binary` and `extra_args`), `review.*` and `skip.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there. +The repository file may only set `llm.model`, `ocr.*` (except `binary` and `extra_args`), and `review.*`. Anything that decides where credentials are sent, or what gets executed, is rejected there. Secrets are only ever read from the env vars named by `gitlab.token_env` and `llm.api_key_env`. `ocr` runs with a private, temporary `HOME`, so its config file and session logs never touch the runner. @@ -58,23 +67,49 @@ ocr: rules: - path: "**/*.go" rule: "Errors must be wrapped with %w; flag bare returns of err from external calls." + merge_system_rule: true # add to OCR's built-in Go rules instead of replacing them review: min_severity: medium max_comments: 15 -skip: - authors: [renovate-bot] ``` +## When it runs + +pruefbyte reviews the merge request whenever it is run; it has no draft, label, +author or branch filters of its own. When it runs is decided by the CI job's +`rules:`. Override the job in your project to add conditions, for example: + +```yaml +pruefbyte-review: + rules: + - if: $CI_MERGE_REQUEST_EVENT_TYPE == "merge_train" + when: never + - if: $CI_PROJECT_ID != $CI_MERGE_REQUEST_PROJECT_ID + when: never + - if: $CI_MERGE_REQUEST_DRAFT == "true" # skip drafts + when: never + - if: $CI_MERGE_REQUEST_LABELS =~ /(^|,)no-review(,|$)/ + when: never + - if: $GITLAB_USER_LOGIN == "renovate-bot" + when: never + - if: $CI_MERGE_REQUEST_TARGET_BRANCH_NAME =~ /^release\// + when: never + - if: $CI_PIPELINE_SOURCE == "merge_request_event" +``` + +Overriding `rules:` replaces the template's list, so keep its first two entries +(merge trains, fork pipelines). To review on demand only, use `when: manual`. + ## Behaviour | Situation | What happens | |---|---| -| Draft MR, skip label/author/branch/title | Job exits 0 without reviewing. | | MR has new commits since the pipeline started | Skipped; the newer pipeline reviews it. | -| Finding on a line in the diff | Inline discussion. Suggestion block if OCR proposed replacement code. | +| Finding on a line in the diff | Inline discussion. Suggestion block if OCR proposed replacement code for exactly the lines it quotes. | +| Finding about removed code | Inline discussion on the removed line, without a suggestion block. | | Finding outside the diff, or GitLab rejects the position | Listed in the summary note. | -| Same finding as an earlier run (open or resolved) | Not posted again. Matched by a hidden fingerprint of file + text. | -| Earlier bot thread not reported again **and** its code changed | Reply and resolve, but only after a complete review. | +| Same finding as an earlier run (open or resolved) | Not posted again. Matched by a hidden fingerprint of file + the code the finding quotes (its text if it quotes none), so rewording doesn't count as new. | +| Earlier bot thread not reported again **and** its code changed | Reply and resolve, but only after a complete review that covered the file, and only once: a thread someone reopens stays open. | | OCR fails | A failure note with the redacted error; job exits 1. | | `review.fail_on_severity` reached | Comments are posted; job exits 3. | diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index a96c4d1..8185909 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -195,6 +195,7 @@ func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { return err } defer runner.Close() + runner.SecretEnv = []string{base.GitLab.TokenEnv, base.LLM.APIKeyEnv} v, err := runner.Version(ctx) if err != nil { return err @@ -226,17 +227,18 @@ func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { if err := cfg.ApplyEnv(os.LookupEnv); err != nil { return cfg, err } - if err := cfg.Validate(); err != nil { - return cfg, err - } + return cfg, cfg.Validate() + }, + PrepareOCR: func(ctx context.Context, cfg config.Config) error { if apiKey == "" && cfg.LLM.Provider != "bedrock" { fmt.Fprintf(os.Stderr, "[pruefbyte] warning: %s is empty; ocr falls back to the provider's own env var\n", cfg.LLM.APIKeyEnv) } - return cfg, runner.Configure(ctx, cfg.LLM, apiKey, cfg.OCR.Language) + return runner.Configure(ctx, cfg.LLM, apiKey, cfg.OCR.Language) }, EnsureCommits: func(ctx context.Context, shas ...string) error { return repo.EnsureCommits(ctx, []string{fmt.Sprintf("refs/merge-requests/%d/head", mrc.MRIID)}, shas...) }, + ReadFileAt: repo.ShowFile, Redact: func(s string) string { for _, secret := range []string{apiKey, token} { if len(secret) >= 4 { diff --git a/cmd/pruefbyte/main_test.go b/cmd/pruefbyte/main_test.go index 005feac..fb7eb0a 100644 --- a/cmd/pruefbyte/main_test.go +++ b/cmd/pruefbyte/main_test.go @@ -36,7 +36,11 @@ func fakeOCR(args []string) int { // Record settings so the test can check them. f, _ := os.OpenFile(filepath.Join(os.Getenv("HOME"), "settings.txt"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) defer f.Close() - fmt.Fprintf(f, "%s=%s\n", args[2], args[3]) + kv := args[2:] + if len(kv) > 0 && kv[0] == "--" { + kv = kv[1:] + } + fmt.Fprintf(f, "%s=%s\n", kv[0], kv[1]) return 0 case "review": var out string diff --git a/internal/config/config.go b/internal/config/config.go index 3afcb08..dd5e9ee 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "os" - "regexp" "strings" "time" @@ -23,7 +22,6 @@ type Config struct { LLM LLM `yaml:"llm"` OCR OCR `yaml:"ocr"` Review Review `yaml:"review"` - Skip Skip `yaml:"skip"` } type GitLab struct { @@ -67,6 +65,9 @@ type OCR struct { type Rule struct { Path string `yaml:"path" json:"path"` Rule string `yaml:"rule" json:"rule"` + // MergeSystemRule keeps OCR's built-in language rule for matching files and + // adds this one. Without it the entry replaces the built-in rule. + MergeSystemRule bool `yaml:"merge_system_rule" json:"merge_system_rule,omitempty"` } type Review struct { @@ -79,15 +80,6 @@ type Review struct { PostFailures bool `yaml:"post_failures"` } -type Skip struct { - Drafts bool `yaml:"drafts"` - Labels []string `yaml:"labels"` - Authors []string `yaml:"authors"` - TitleRegex []string `yaml:"title_regex"` - SourceBranches []string `yaml:"source_branches"` - TargetBranches []string `yaml:"target_branches"` -} - // repoConfig lists the only keys a repository file may set. Anything that decides // where secrets are sent or what gets executed (gitlab.*, llm endpoint/provider, // ocr.binary, ocr.extra_args) stays under the operator's control. @@ -107,7 +99,6 @@ type repoConfig struct { UseMRDescriptionAsBackground bool `yaml:"use_mr_description_as_background"` } `yaml:"ocr"` Review Review `yaml:"review"` - Skip Skip `yaml:"skip"` } func Default() Config { @@ -129,11 +120,6 @@ func Default() Config { ResolveOutdated: true, PostFailures: true, }, - Skip: Skip{ - Drafts: true, - Labels: []string{"no-review"}, - TitleRegex: []string{`^\[WIP\]`}, - }, } } @@ -147,7 +133,7 @@ func (c *Config) ApplyGlobal(data []byte) error { func (c *Config) ApplyRepo(data []byte) error { var probe repoConfig if err := decodeStrict(data, &probe); err != nil { - return fmt.Errorf("%s: %w (only llm.model, ocr.*, review.* and skip.* are allowed here, except ocr.binary and ocr.extra_args)", RepoConfigFile, err) + return fmt.Errorf("%s: %w (only llm.model, ocr.*, and review.* are allowed here, except ocr.binary and ocr.extra_args)", RepoConfigFile, err) } return decodeStrict(data, c) } @@ -204,6 +190,12 @@ func (c Config) Validate() error { errs = append(errs, fmt.Errorf("llm.url is required for custom provider %q", c.LLM.Provider)) } } + for k, v := range c.LLM.ExtraHeaders { + // ocr takes headers as one `k=v,k="v,w"` string and cannot represent these. + if k == "" || strings.ContainsAny(k, `,="`) || strings.Contains(v, `"`) { + errs = append(errs, fmt.Errorf("llm.extra_headers %q: ocr cannot pass a header whose name contains , = or \" or whose value contains \"", k)) + } + } if c.GitLab.TokenEnv == "" { errs = append(errs, errors.New("gitlab.token_env is required")) } @@ -230,22 +222,19 @@ func (c Config) Validate() error { if c.Review.MaxComments < 0 { errs = append(errs, errors.New("review.max_comments must be >= 0 (0 = unlimited)")) } - for _, re := range c.Skip.TitleRegex { - if _, err := regexp.Compile(re); err != nil { - errs = append(errs, fmt.Errorf("skip.title_regex %q: %w", re, err)) - } - } return errors.Join(errs...) } -// builtinProviders mirrors OCR's built-in provider table (docs: configuration.md). +// builtinProviders mirrors OCR's built-in provider presets (`ocr llm providers`, +// v1.12.10). OCR rejects a custom provider that uses one of these names. var builtinProviders = map[string]bool{ "anthropic": true, "bedrock": true, "openai": true, "openai-responses": true, "openrouter": true, "gemini": true, "dashscope": true, "dashscope-tokenplan": true, "volcengine": true, "deepseek": true, "tencent-tokenhub": true, "hy-tokenplan": true, - "iflytek": true, "kimi": true, "kimi-global": true, "z-ai": true, "mimo": true, - "minimax": true, "minimax-cn": true, "baidu-qianfan": true, "siliconflow": true, - "siliconflow-cn": true, "novita": true, "xai": true, + "iflytek": true, "kimi": true, "kimi-global": true, "z-ai": true, "z-ai-coding": true, + "mimo": true, "minimax": true, "minimax-cn": true, "baidu-qianfan": true, + "siliconflow": true, "siliconflow-cn": true, "novita": true, "xai": true, + "edenai": true, "litellm": true, "mistral": true, "ollama-cloud": true, } func IsBuiltinProvider(name string) bool { return builtinProviders[name] } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 61f7cdb..eec6c4e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -19,8 +19,6 @@ llm: review: max_comments: 10 min_severity: medium -skip: - labels: [skip-me] ` repo := ` llm: @@ -40,7 +38,7 @@ review: if err := cfg.ApplyEnv(env(map[string]string{ "PRUEFBYTE_REVIEW_MAX_COMMENTS": "5", "PRUEFBYTE_OCR_TIMEOUT": "5m", - "PRUEFBYTE_SKIP_AUTHORS": "renovate-bot, dependabot", + "PRUEFBYTE_REVIEW_CATEGORIES": "bug, security", "PRUEFBYTE_REVIEW_SUGGESTIONS": "false", })); err != nil { t.Fatal(err) @@ -59,8 +57,7 @@ review: {"max comments from env", cfg.Review.MaxComments, 5}, {"timeout from env", cfg.OCR.Timeout, 5 * time.Minute}, {"suggestions from env", cfg.Review.Suggestions, false}, - {"authors from env", strings.Join(cfg.Skip.Authors, "|"), "renovate-bot|dependabot"}, - {"labels from global", strings.Join(cfg.Skip.Labels, "|"), "skip-me"}, + {"categories from env", strings.Join(cfg.Review.Categories, "|"), "bug|security"}, {"default kept", cfg.GitLab.TokenEnv, "PRUEFBYTE_GITLAB_TOKEN"}, {"exclude from repo", strings.Join(cfg.OCR.Exclude, "|"), "docs/**"}, } @@ -87,6 +84,17 @@ func TestRepoFileCannotRedirectSecrets(t *testing.T) { } } +func TestRuleMergeSystemRule(t *testing.T) { + cfg := Default() + doc := "ocr:\n rules:\n - path: \"**/*.go\"\n rule: check errors\n merge_system_rule: true\n" + if err := cfg.ApplyRepo([]byte(doc)); err != nil { + t.Fatal(err) + } + if len(cfg.OCR.Rules) != 1 || !cfg.OCR.Rules[0].MergeSystemRule { + t.Errorf("rules %+v", cfg.OCR.Rules) + } +} + func TestUnknownKeyRejected(t *testing.T) { cfg := Default() if err := cfg.ApplyGlobal([]byte("review:\n max_coments: 3\n")); err == nil { @@ -120,9 +128,10 @@ func TestValidate(t *testing.T) { "bad severity": func(c *Config) { c.Review.MinSeverity = "urgent" }, "bad gate": func(c *Config) { c.Review.FailOnSeverity = "bad" }, "bad effort": func(c *Config) { c.OCR.Effort = "max" }, - "rules and rule file": func(c *Config) { c.OCR.RuleFile = "r.json"; c.OCR.Rules = []Rule{{"**", "x"}} }, - "bad title regex": func(c *Config) { c.Skip.TitleRegex = []string{"("} }, + "rules and rule file": func(c *Config) { c.OCR.RuleFile = "r.json"; c.OCR.Rules = []Rule{{Path: "**", Rule: "x"}} }, "negative max comment": func(c *Config) { c.Review.MaxComments = -1 }, + "quote in header": func(c *Config) { c.LLM.ExtraHeaders = map[string]string{"x-a": `a"b`} }, + "comma in header name": func(c *Config) { c.LLM.ExtraHeaders = map[string]string{"x,a": "b"} }, } for name, mutate := range cases { c := base() @@ -136,6 +145,19 @@ func TestValidate(t *testing.T) { if err := c.Validate(); err != nil { t.Errorf("custom provider rejected: %v", err) } + // OCR presets need no protocol or url; OCR refuses them as custom providers. + for _, p := range []string{"litellm", "mistral", "edenai", "ollama-cloud", "z-ai-coding"} { + c := base() + c.LLM.Provider = p + if err := c.Validate(); err != nil || !IsBuiltinProvider(p) { + t.Errorf("preset %s: %v", p, err) + } + } + c = base() + c.LLM.ExtraHeaders = map[string]string{"anthropic-beta": "a,b"} + if err := c.Validate(); err != nil { + t.Errorf("header value with commas rejected: %v", err) + } } func TestEnvBadValue(t *testing.T) { diff --git a/internal/gitlab/client.go b/internal/gitlab/client.go index 69d6782..bd4800d 100644 --- a/internal/gitlab/client.go +++ b/internal/gitlab/client.go @@ -6,24 +6,20 @@ import ( "context" "errors" "fmt" + "net" "net/http" gl "gitlab.com/gitlab-org/api/client-go" ) type MR struct { - IID int64 - Title string - Description string - SourceBranch string - TargetBranch string - Author string - Draft bool - Labels []string - WebURL string - BaseSHA string - StartSHA string - HeadSHA string + IID int64 + Title string + Description string + WebURL string + BaseSHA string + StartSHA string + HeadSHA string } type FileDiff struct { @@ -76,7 +72,8 @@ type Client struct { } // New creates a client authenticated with the bot's personal access token. -// client-go retries 429 and 5xx responses with backoff. +// client-go retries 429 and 5xx responses with backoff; requests that create +// notes use retryCreate instead. func New(baseURL, token, project string, mrIID int64) (*Client, error) { c, err := gl.NewClient(token, gl.WithBaseURL(baseURL)) if err != nil { @@ -85,6 +82,21 @@ func New(baseURL, token, project string, mrIID int64) (*Client, error) { return &Client{gl: c, project: project, mrIID: mrIID}, nil } +// retryCreate is the retry policy for requests that create a note. client-go +// retries 5xx for every method, but GitLab can answer 5xx after it stored the +// note (e.g. a 500 when it fails to build the note's diff file), and resending +// posts it again. Only rate limiting and connection failures are retried. +func retryCreate(ctx context.Context, resp *http.Response, err error) (bool, error) { + if ctx.Err() != nil { + return false, ctx.Err() + } + if err != nil { + var op *net.OpError + return errors.As(err, &op) && op.Op == "dial", nil + } + return resp.StatusCode == http.StatusTooManyRequests, nil +} + // IsBadRequest reports whether err is a GitLab 400, which is how GitLab rejects // a discussion position it cannot resolve against the diff. func IsBadRequest(err error) bool { @@ -106,20 +118,13 @@ func (c *Client) GetMR(ctx context.Context) (*MR, error) { return nil, fmt.Errorf("fetching merge request !%d: %w", c.mrIID, err) } mr := &MR{ - IID: m.IID, - Title: m.Title, - Description: m.Description, - SourceBranch: m.SourceBranch, - TargetBranch: m.TargetBranch, - Draft: m.Draft, - Labels: m.Labels, - WebURL: m.WebURL, - BaseSHA: m.DiffRefs.BaseSha, - StartSHA: m.DiffRefs.StartSha, - HeadSHA: m.DiffRefs.HeadSha, - } - if m.Author != nil { - mr.Author = m.Author.Username + IID: m.IID, + Title: m.Title, + Description: m.Description, + WebURL: m.WebURL, + BaseSHA: m.DiffRefs.BaseSha, + StartSHA: m.DiffRefs.StartSha, + HeadSHA: m.DiffRefs.HeadSha, } return mr, nil } @@ -190,13 +195,13 @@ func (c *Client) CreateDiscussion(ctx context.Context, body string, pos *Positio } opt.Position = po } - _, _, err := c.gl.Discussions.CreateMergeRequestDiscussion(c.project, c.mrIID, opt, gl.WithContext(ctx)) + _, _, err := c.gl.Discussions.CreateMergeRequestDiscussion(c.project, c.mrIID, opt, gl.WithContext(ctx), gl.WithRequestRetry(retryCreate)) return err } func (c *Client) CreateNote(ctx context.Context, body string) error { _, _, err := c.gl.Notes.CreateMergeRequestNote(c.project, c.mrIID, - &gl.CreateMergeRequestNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx)) + &gl.CreateMergeRequestNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx), gl.WithRequestRetry(retryCreate)) return err } @@ -209,7 +214,7 @@ func (c *Client) UpdateNote(ctx context.Context, noteID int64, body string) erro func (c *Client) ReplyAndResolve(ctx context.Context, discussionID, body string) error { if body != "" { if _, _, err := c.gl.Discussions.AddMergeRequestDiscussionNote(c.project, c.mrIID, discussionID, - &gl.AddMergeRequestDiscussionNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx)); err != nil { + &gl.AddMergeRequestDiscussionNoteOptions{Body: gl.Ptr(body)}, gl.WithContext(ctx), gl.WithRequestRetry(retryCreate)); err != nil { return err } } diff --git a/internal/gitlab/client_test.go b/internal/gitlab/client_test.go index c6b8b2c..c24341e 100644 --- a/internal/gitlab/client_test.go +++ b/internal/gitlab/client_test.go @@ -53,6 +53,23 @@ func TestCreateDiscussionPayload(t *testing.T) { } } +func TestCreateIsNotResentAfter5xx(t *testing.T) { + var posts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + posts++ // GitLab may have stored the note before failing + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"500 Internal Server Error"}`)) + })) + defer srv.Close() + c, _ := New(srv.URL+"/api/v4", "t", "1", 1) + if err := c.CreateDiscussion(context.Background(), "x", &Position{NewPath: "a", NewLine: 1}); err == nil { + t.Fatal("500 reported as success") + } + if posts != 1 { + t.Errorf("discussion POSTed %d times", posts) + } +} + func TestIsBadRequest(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusBadRequest) diff --git a/internal/ocr/ocr_test.go b/internal/ocr/ocr_test.go index c8cd2c5..5750f09 100644 --- a/internal/ocr/ocr_test.go +++ b/internal/ocr/ocr_test.go @@ -1,6 +1,9 @@ package ocr import ( + "context" + "os" + "path/filepath" "strings" "testing" @@ -52,7 +55,7 @@ func TestConfigSettingsBuiltin(t *testing.T) { kv, err := ConfigSettings(config.LLM{ Provider: "anthropic", Model: "claude-sonnet-5", URL: "https://proxy/v1", ExtraBody: map[string]any{"thinking": map[string]any{"type": "disabled"}}, - ExtraHeaders: map[string]string{"b": "2", "a": "1"}, + ExtraHeaders: map[string]string{"b": "2", "a": "1", "anthropic-beta": "x-1,y-2"}, TimeoutSec: 600, }, "sk-test", "German") if err != nil { @@ -65,7 +68,7 @@ func TestConfigSettingsBuiltin(t *testing.T) { "providers.anthropic.api_key": "sk-test", "providers.anthropic.url": "https://proxy/v1", "providers.anthropic.extra_body": `{"thinking":{"type":"disabled"}}`, - "providers.anthropic.extra_headers": "a=1,b=2", + "providers.anthropic.extra_headers": `a=1,anthropic-beta="x-1,y-2",b=2`, "providers.anthropic.timeout_sec": "600", "language": "German", } @@ -105,14 +108,14 @@ func must[T any](v T, err error) T { func TestReviewArgs(t *testing.T) { args := ReviewOptions{ RepoDir: "/repo", From: "base", To: "head", Provider: "openai", Model: "gpt-5", - Effort: "high", Concurrency: 4, Exclude: []string{"a/**", "b"}, RuleFile: "/tmp/rule.json", + Effort: "high", Concurrency: 4, RuleFile: "/tmp/rule.json", BackgroundFile: "/tmp/bg.md", ExtraArgs: []string{"--no-filter"}, }.Args("/tmp/out.json") got := strings.Join(args, " ") for _, want := range []string{ "review --repo /repo --from base --to head --format json --audience agent --output /tmp/out.json", "--provider openai", "--model gpt-5", "--effort high", "--concurrency 4", - "--exclude a/**,b", "--rule /tmp/rule.json", "--background-file /tmp/bg.md", "--no-filter", + "--rule /tmp/rule.json", "--background-file /tmp/bg.md", "--no-filter", } { if !strings.Contains(got, want) { t.Errorf("args %q missing %q", got, want) @@ -124,12 +127,69 @@ func TestReviewArgs(t *testing.T) { } func TestRuleJSON(t *testing.T) { - b, err := RuleJSON([]config.Rule{{Path: "**/*.go", Rule: "check errors"}}) + parse := func(b []byte, err error) RuleFile { + t.Helper() + if err != nil { + t.Fatal(err) + } + rf, err := ParseRuleFile(b) + if err != nil { + t.Fatal(err) + } + return rf + } + catchAll := config.Rule{Path: "**/*", MergeSystemRule: true} + + // Nothing configured: still a filter and a catch-all, so the checkout's + // .opencodereview/rule.json cannot apply. + rf := parse(RuleJSON(nil, nil)) + if len(rf.Exclude) != 1 || len(rf.Include) != 0 || len(rf.Rules) != 1 || rf.Rules[0] != catchAll { + t.Errorf("empty rule file: %+v", rf) + } + + // Rules keep layer order; the first layer with a filter wins, like in OCR; + // extra excludes (brace globs included) are added to it. + own := RuleFile{Rules: []config.Rule{{Path: "**/*.go", Rule: "check errors", MergeSystemRule: true}}} + base := RuleFile{Include: []string{"src/**"}, Exclude: []string{"gen/**"}, Rules: []config.Rule{{Path: "**/*.ts", Rule: "ts"}}} + rf = parse(RuleJSON([]RuleFile{own, base, {Exclude: []string{"never/**"}}}, []string{"**/*.{pb,gen}.go"})) + if len(rf.Rules) != 3 || rf.Rules[0].Rule != "check errors" || !rf.Rules[0].MergeSystemRule || rf.Rules[1].Rule != "ts" || rf.Rules[2] != catchAll { + t.Errorf("rules: %+v", rf.Rules) + } + if strings.Join(rf.Include, ",") != "src/**" || strings.Join(rf.Exclude, " ") != "gen/** **/*.{pb,gen}.go" { + t.Errorf("filter: include %v exclude %v", rf.Include, rf.Exclude) + } + if strings.Join(base.Exclude, ",") != "gen/**" { + t.Errorf("input layer modified: %v", base.Exclude) + } +} + +func TestReviewedCoverage(t *testing.T) { + r, err := ParseResult([]byte(`{"status": "complete", "comments": [], + "manifest": {"coverage": {"completed": [{"path": "a.go"}], "reused": [{"path": "b.go"}], "failed": [{"path": "c.go"}]}}}`)) if err != nil { t.Fatal(err) } - if !strings.Contains(string(b), `"path": "**/*.go"`) || !strings.Contains(string(b), `"rule": "check errors"`) { - t.Errorf("unexpected rule.json: %s", b) + if !r.Reviewed("a.go") || !r.Reviewed("b.go") || r.Reviewed("c.go") || r.Reviewed("big.go") { + t.Error("coverage not honoured") + } + skipped := &Result{Status: "skipped"} + if skipped.Reviewed("a.go") || !(&Result{Status: "success"}).Reviewed("a.go") { + t.Error("status fallback wrong") + } +} + +func TestCommandRefusesCmdMetacharacters(t *testing.T) { + dir := t.TempDir() + shim := filepath.Join(dir, "ocr.cmd") + if err := os.WriteFile(shim, []byte("@echo off\r\n"), 0o755); err != nil { + t.Fatal(err) + } + r := &Runner{Binary: shim, Home: dir} + if _, err := r.command(context.Background(), "config", "set", "model", `m" & calc & rem "`); err == nil { + t.Error("cmd.exe metacharacters accepted for a .cmd binary") + } + if _, err := r.command(context.Background(), "config", "set", "model", "claude-sonnet-5"); err != nil { + t.Errorf("plain argument refused: %v", err) } } @@ -141,3 +201,43 @@ func TestTailBuffer(t *testing.T) { t.Errorf("tail buffer kept %d bytes", len(s)) } } + +func TestEnvIsolation(t *testing.T) { + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, ".aws"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(home, ".aws", "config"), []byte("[profile prod]\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("GITLAB_BOT_TOKEN", "glpat-renamed") + t.Setenv("PRUEFBYTE_LLM_API_KEY", "sk-default") + t.Setenv("OCR_LLM_URL", "http://elsewhere") + r := &Runner{Home: t.TempDir(), SecretEnv: []string{"gitlab_bot_token"}} + env := "\n" + strings.Join(r.env(), "\n") + "\n" + for _, leaked := range []string{"GITLAB_BOT_TOKEN=", "PRUEFBYTE_LLM_API_KEY=", "OCR_LLM_URL="} { + if strings.Contains(env, "\n"+leaked) { + t.Errorf("%s passed to ocr", leaked) + } + } + for _, want := range []string{"HOME=" + r.Home, "OCR_NO_UPDATE=1"} { + if !strings.Contains(env, "\n"+want+"\n") { + t.Errorf("env lacks %s", want) + } + } + if _, set := os.LookupEnv("AWS_CONFIG_FILE"); !set && !strings.Contains(env, "\nAWS_CONFIG_FILE="+filepath.Join(home, ".aws", "config")+"\n") { + t.Error("private HOME hides ~/.aws/config from the Bedrock credential chain") + } +} + +func TestExtraBodyWithNonStringKeys(t *testing.T) { + m := settingsMap(must(ConfigSettings(config.LLM{ + Provider: "openai", Model: "gpt-5", + ExtraBody: map[string]any{"logit_bias": map[any]any{50256: -100}}, + }, "sk", ""))) + if got := m["providers.openai.extra_body"]; got != `{"logit_bias":{"50256":-100}}` { + t.Errorf("extra_body = %q", got) + } +} diff --git a/internal/ocr/result.go b/internal/ocr/result.go index 20b7880..d921678 100644 --- a/internal/ocr/result.go +++ b/internal/ocr/result.go @@ -14,6 +14,19 @@ type Result struct { Comments []Comment `json:"comments"` Warnings []Warning `json:"warnings,omitempty"` Session string `json:"session_id,omitempty"` + Manifest *Manifest `json:"manifest,omitempty"` +} + +// Manifest is the part of OCR's run manifest (range reviews) pruefbyte reads. +type Manifest struct { + Coverage struct { + Completed []CoveredItem `json:"completed"` + Reused []CoveredItem `json:"reused"` + } `json:"coverage"` +} + +type CoveredItem struct { + Path string `json:"path"` } type LLMInfo struct { @@ -72,3 +85,21 @@ func (r *Result) Complete() bool { // Failed reports whether the run produced nothing usable. func (r *Result) Failed() bool { return r.Status == "failed" } + +// Reviewed reports whether OCR finished reviewing path in this run, so that a +// missing finding there means "no issue". A complete run only covers the files +// OCR selected: files it left out (too large, excluded, unsupported) are not +// reviewed, and a skipped run reviewed nothing. +func (r *Result) Reviewed(path string) bool { + if r.Manifest == nil { + return r.Complete() && r.Status != "skipped" + } + for _, items := range [][]CoveredItem{r.Manifest.Coverage.Completed, r.Manifest.Coverage.Reused} { + for _, it := range items { + if it.Path == path { + return true + } + } + } + return false +} diff --git a/internal/ocr/rules.go b/internal/ocr/rules.go index 2b3b65d..f6eacff 100644 --- a/internal/ocr/rules.go +++ b/internal/ocr/rules.go @@ -2,13 +2,75 @@ package ocr import ( "encoding/json" + "fmt" + "path/filepath" + "slices" + "strings" "pruefbyte/internal/config" ) -// RuleJSON renders configured rules in OCR's rule.json schema. -func RuleJSON(rules []config.Rule) ([]byte, error) { - return json.MarshalIndent(struct { - Rules []config.Rule `json:"rules"` - }{rules}, "", " ") +// RuleFile is OCR's rule.json schema. +type RuleFile struct { + Include []string `json:"include,omitempty"` + Exclude []string `json:"exclude,omitempty"` + Rules []config.Rule `json:"rules,omitempty"` +} + +// ParseRuleFile reads an OCR rule.json document. +func ParseRuleFile(data []byte) (RuleFile, error) { + var rf RuleFile + if err := json.Unmarshal(data, &rf); err != nil { + return rf, fmt.Errorf("parsing rule.json: %w", err) + } + return rf, nil +} + +var ruleFileExts = map[string]bool{".md": true, ".txt": true, ".markdown": true} + +// InlineFiles replaces rules that name a file instead of holding text (OCR's +// test: one line, no spaces, ending in .md, .txt or .markdown) with the content +// read returns for that name. A file read cannot find leaves the rule empty, as +// in OCR. +func (rf *RuleFile) InlineFiles(read func(name string) ([]byte, bool, error)) error { + for i := range rf.Rules { + name := rf.Rules[i].Rule + if strings.TrimSpace(name) == "" || strings.ContainsAny(name, "\n ") || !ruleFileExts[strings.ToLower(filepath.Ext(name))] { + continue + } + data, found, err := read(name) + if err != nil { + return fmt.Errorf("rule file %s: %w", name, err) + } + rf.Rules[i].Rule = "" + if found { + rf.Rules[i].Rule = strings.TrimRight(string(data), "\n") + } + } + return nil +} + +// RuleJSON merges rule layers, highest priority first, and extra exclude patterns +// into the one file pruefbyte passes with --rule. +// +// OCR also reads /.opencodereview/rule.json from the checkout, which the +// merge request controls. The result keeps that file inert: OCR takes the file +// filter from the highest-priority layer that has include/exclude patterns, so +// this file always has some, and the trailing catch-all matches every path, so +// the checkout's rule entries are never reached. The catch-all has no text and +// keeps OCR's built-in rule, so it changes nothing else. +func RuleJSON(layers []RuleFile, exclude []string) ([]byte, error) { + var out RuleFile + for _, l := range layers { + out.Rules = append(out.Rules, l.Rules...) + if len(out.Include)+len(out.Exclude) == 0 { + out.Include, out.Exclude = slices.Clone(l.Include), slices.Clone(l.Exclude) + } + } + out.Exclude = append(out.Exclude, exclude...) + if len(out.Include)+len(out.Exclude) == 0 { + out.Exclude = []string{".git/**"} // matches nothing a diff contains + } + out.Rules = append(out.Rules, config.Rule{Path: "**/*", MergeSystemRule: true}) + return json.MarshalIndent(out, "", " ") } diff --git a/internal/ocr/runner.go b/internal/ocr/runner.go index 1bcef2c..0dcf7c5 100644 --- a/internal/ocr/runner.go +++ b/internal/ocr/runner.go @@ -11,9 +11,13 @@ import ( "os" "os/exec" "path/filepath" + "runtime" + "slices" "sort" "strconv" "strings" + "syscall" + "time" "pruefbyte/internal/config" ) @@ -25,6 +29,9 @@ type Runner struct { Home string // Log receives ocr's stderr as it runs. Log io.Writer + // SecretEnv names env vars ocr must not inherit, such as the renamed + // gitlab.token_env and llm.api_key_env. + SecretEnv []string } // NewRunner creates a runner with a fresh temporary home directory. Call Close to remove it. @@ -39,25 +46,64 @@ func NewRunner(binary string, log io.Writer) (*Runner, error) { func (r *Runner) Close() error { return os.RemoveAll(r.Home) } func (r *Runner) env() []string { - env := make([]string, 0, len(os.Environ())+6) + env := make([]string, 0, len(os.Environ())+9) for _, kv := range os.Environ() { k, _, _ := strings.Cut(kv, "=") - switch strings.ToUpper(k) { + // Env names are case-insensitive on Windows. + switch k = strings.ToUpper(k); { // Keep ocr from reading secrets or config meant for pruefbyte or other tools. - case "HOME", "USERPROFILE", "XDG_CONFIG_HOME": + case k == "HOME" || k == "USERPROFILE" || k == "XDG_CONFIG_HOME": continue - } - if strings.HasPrefix(k, "PRUEFBYTE_") || strings.HasPrefix(k, "OCR_LLM_") { + case strings.HasPrefix(k, config.EnvPrefix) || strings.HasPrefix(k, "OCR_LLM_"): + continue + case slices.ContainsFunc(r.SecretEnv, func(s string) bool { return strings.EqualFold(s, k) }): continue } env = append(env, kv) } - return append(env, + env = append(env, "HOME="+r.Home, "USERPROFILE="+r.Home, // The CI checkout is often owned by another uid; git refuses to work in it otherwise. "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=safe.directory", "GIT_CONFIG_VALUE_0=*", "TERM=dumb", "NO_COLOR=1", + // The npm launcher would otherwise check for, and install, a newer OCR on every + // run: its once-per-18-minutes stamp lives in the fresh HOME. + "OCR_NO_UPDATE=1", ) + // The private HOME also hides ~/.aws, where the AWS SDK behind OCR's Bedrock + // provider finds profiles and credentials. + if home, err := os.UserHomeDir(); err == nil { + for _, f := range [][2]string{{"AWS_CONFIG_FILE", "config"}, {"AWS_SHARED_CREDENTIALS_FILE", "credentials"}} { + p := filepath.Join(home, ".aws", f[1]) + if _, set := os.LookupEnv(f[0]); !set && fileExists(p) { + env = append(env, f[0]+"="+p) + } + } + } + return env +} + +func fileExists(p string) bool { + st, err := os.Stat(p) + return err == nil && !st.IsDir() +} + +// command builds an ocr invocation with the isolated environment. On Windows npm +// installs ocr as a .cmd shim, which runs through cmd.exe; Go's argument quoting +// does not protect against cmd.exe, so arguments it would interpret are refused. +// (A '"' only moves cmd.exe's quoting, which matters no more once none of these +// characters is left anywhere on the line.) +func (r *Runner) command(ctx context.Context, args ...string) (*exec.Cmd, error) { + cmd := exec.CommandContext(ctx, r.Binary, args...) + if ext := strings.ToLower(filepath.Ext(cmd.Path)); ext == ".cmd" || ext == ".bat" { + for _, a := range args { + if strings.ContainsAny(a, "%&<>^|\r\n") { + return nil, fmt.Errorf("%s runs through cmd.exe, which cannot be given an argument containing any of %% & < > ^ | or a line break; point ocr.binary at the native OCR executable", cmd.Path) + } + } + } + cmd.Env = r.env() + return cmd, nil } // ConfigSettings returns the `ocr config set` key/value pairs for the LLM settings. @@ -90,7 +136,7 @@ func ConfigSettings(llm config.LLM, apiKey, language string) ([][2]string, error add(section+".aws_region", llm.AWSRegion) add(section+".aws_profile", llm.AWSProfile) if len(llm.ExtraBody) > 0 { - b, err := json.Marshal(llm.ExtraBody) + b, err := json.Marshal(jsonKeys(llm.ExtraBody)) if err != nil { return nil, fmt.Errorf("llm.extra_body: %w", err) } @@ -104,7 +150,11 @@ func ConfigSettings(llm config.LLM, apiKey, language string) ([][2]string, error sort.Strings(keys) parts := make([]string, 0, len(keys)) for _, k := range keys { - parts = append(parts, k+"="+llm.ExtraHeaders[k]) + v := llm.ExtraHeaders[k] + if strings.Contains(v, ",") { + v = `"` + v + `"` // ocr splits on commas outside double quotes + } + parts = append(parts, k+"="+v) } add(section+".extra_headers", strings.Join(parts, ",")) } @@ -112,6 +162,32 @@ func ConfigSettings(llm config.LLM, apiKey, language string) ([][2]string, error return kv, nil } +// jsonKeys turns the map[any]any that YAML produces for mappings with non-string +// keys (e.g. logit_bias token ids) into string-keyed maps json can encode. +func jsonKeys(v any) any { + switch v := v.(type) { + case map[string]any: + out := make(map[string]any, len(v)) + for k, e := range v { + out[k] = jsonKeys(e) + } + return out + case map[any]any: + out := make(map[string]any, len(v)) + for k, e := range v { + out[fmt.Sprint(k)] = jsonKeys(e) + } + return out + case []any: + out := make([]any, len(v)) + for i, e := range v { + out[i] = jsonKeys(e) + } + return out + } + return v +} + // Configure writes the LLM settings into ocr's config via `ocr config set`, so the // config file format stays ocr's own business. func (r *Runner) Configure(ctx context.Context, llm config.LLM, apiKey, language string) error { @@ -120,10 +196,16 @@ func (r *Runner) Configure(ctx context.Context, llm config.LLM, apiKey, language return err } for _, s := range settings { - cmd := exec.CommandContext(ctx, r.Binary, "config", "set", s[0], s[1]) - cmd.Env = r.env() + // "--": a value starting with '-' (an API key can) is not a flag. + cmd, err := r.command(ctx, "config", "set", "--", s[0], s[1]) + if err != nil { + return fmt.Errorf("ocr config set %s: %w", s[0], err) + } if out, err := cmd.CombinedOutput(); err != nil { - msg := strings.ReplaceAll(string(out), apiKey, "***") + msg := string(out) + if apiKey != "" { // an empty old string would put *** between every character + msg = strings.ReplaceAll(msg, apiKey, "***") + } return fmt.Errorf("ocr config set %s: %w: %s", s[0], err, strings.TrimSpace(msg)) } } @@ -137,10 +219,11 @@ type ReviewOptions struct { Effort string Concurrency int MaxTokensBudget int - Exclude []string - RuleFile string - BackgroundFile string - ExtraArgs []string + // RuleFile is passed with --rule; exclude patterns go into it too, since + // --exclude is split on commas and would break brace globs. + RuleFile string + BackgroundFile string + ExtraArgs []string } func (o ReviewOptions) Args(outFile string) []string { @@ -161,9 +244,6 @@ func (o ReviewOptions) Args(outFile string) []string { if o.MaxTokensBudget > 0 { args = append(args, "--max-tokens-budget", strconv.Itoa(o.MaxTokensBudget)) } - if len(o.Exclude) > 0 { - args = append(args, "--exclude", strings.Join(o.Exclude, ",")) - } if o.RuleFile != "" { args = append(args, "--rule", o.RuleFile) } @@ -189,9 +269,22 @@ func (e *RunError) Unwrap() error { return e.Err } // also returned so callers can keep it as a CI artifact. func (r *Runner) Review(ctx context.Context, o ReviewOptions) (*Result, []byte, error) { outFile := filepath.Join(r.Home, "result.json") - cmd := exec.CommandContext(ctx, r.Binary, o.Args(outFile)...) + cmd, err := r.command(ctx, o.Args(outFile)...) + if err != nil { + return nil, nil, err + } cmd.Dir = o.RepoDir - cmd.Env = r.env() + // When ctx ends, stop the whole process tree: the npm launcher (node, plus + // cmd.exe on Windows) runs the native binary as a child, and killing only the + // launcher leaves it running. On Unix the launcher forwards SIGTERM. WaitDelay + // bounds the wait for a child that still holds the inherited stderr pipe. + cmd.Cancel = func() error { + if runtime.GOOS == "windows" { + return exec.Command("taskkill", "/T", "/F", "/PID", strconv.Itoa(cmd.Process.Pid)).Run() + } + return cmd.Process.Signal(syscall.SIGTERM) + } + cmd.WaitDelay = 10 * time.Second var stderr tailBuffer cmd.Stdout = r.Log cmd.Stderr = io.MultiWriter(r.Log, &stderr) @@ -200,11 +293,12 @@ func (r *Runner) Review(ctx context.Context, o ReviewOptions) (*Result, []byte, raw, readErr := os.ReadFile(outFile) if runErr != nil { if ctx.Err() != nil { - runErr = fmt.Errorf("%w (ocr.timeout reached?)", ctx.Err()) + return nil, raw, &RunError{Err: fmt.Errorf("%w (ocr.timeout reached?)", ctx.Err()), Stderr: stderr.String()} } - // A failed run can still leave a usable document (e.g. partial results). + // A failed run can still leave a usable document (e.g. partial results), + // but not when OCR itself reports the review as failed. if readErr == nil { - if res, err := ParseResult(raw); err == nil && len(res.Comments) > 0 { + if res, err := ParseResult(raw); err == nil && len(res.Comments) > 0 && !res.Failed() { res.Status = "partial" return res, raw, nil } @@ -249,8 +343,10 @@ func (r *Runner) Version(ctx context.Context) (string, error) { if _, err := exec.LookPath(r.Binary); err != nil { return "", fmt.Errorf("%w: %q (install @alibaba-group/open-code-review or set ocr.binary)", ErrNotFound, r.Binary) } - cmd := exec.CommandContext(ctx, r.Binary, "version") - cmd.Env = r.env() + cmd, err := r.command(ctx, "version") + if err != nil { + return "", err + } out, err := cmd.CombinedOutput() return strings.TrimSpace(string(out)), err } diff --git a/internal/review/diff.go b/internal/review/diff.go index 71ccd5e..f629b09 100644 --- a/internal/review/diff.go +++ b/internal/review/diff.go @@ -6,6 +6,7 @@ import ( "strings" "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" ) var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) @@ -14,13 +15,22 @@ var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@ type lineInfo struct { added bool oldLine int // set for unchanged context lines + text string +} + +// oldLineInfo describes an old-file line that is visible in the MR diff. +type oldLineInfo struct { + removed bool + newLine int // set for unchanged context lines + text string } type fileDiff struct { oldPath string - // lines maps new-file line numbers to diff positions. nil means GitLab - // returned no patch (too large / collapsed), so lines are unknown. - lines map[int]lineInfo + // lines and oldLines map new- and old-file line numbers to diff positions. nil + // means GitLab returned no patch (too large / collapsed), so lines are unknown. + lines map[int]lineInfo + oldLines map[int]oldLineInfo } // diffIndex maps new paths of the MR diff to the lines GitLab can anchor comments on. @@ -34,15 +44,15 @@ func buildDiffIndex(diffs []gitlab.FileDiff) diffIndex { } fd := &fileDiff{oldPath: d.OldPath} if strings.TrimSpace(d.Diff) != "" { - fd.lines = parseNewLines(d.Diff) + fd.lines, fd.oldLines = parsePatch(d.Diff) } idx[d.NewPath] = fd } return idx } -func parseNewLines(patch string) map[int]lineInfo { - lines := map[int]lineInfo{} +func parsePatch(patch string) (map[int]lineInfo, map[int]oldLineInfo) { + lines, oldLines := map[int]lineInfo{}, map[int]oldLineInfo{} oldLn, newLn, inHunk := 0, 0, false for _, l := range strings.Split(patch, "\n") { if m := hunkHeader.FindStringSubmatch(l); m != nil { @@ -56,22 +66,42 @@ func parseNewLines(patch string) map[int]lineInfo { } switch { case strings.HasPrefix(l, "+"): - lines[newLn] = lineInfo{added: true} + lines[newLn] = lineInfo{added: true, text: l[1:]} newLn++ case strings.HasPrefix(l, "-"): + oldLines[oldLn] = oldLineInfo{removed: true, text: l[1:]} oldLn++ case strings.HasPrefix(l, " "): - lines[newLn] = lineInfo{oldLine: oldLn} + lines[newLn] = lineInfo{oldLine: oldLn, text: l[1:]} + oldLines[oldLn] = oldLineInfo{newLine: newLn, text: l[1:]} oldLn++ newLn++ } } - return lines + return lines, oldLines +} + +// placement is where a finding goes on the diff. +type placement struct { + newLine, oldLine int // GitLab position lines; newLine 0 means a removed line + at int // the same line in the finding's own numbering + // suggest is set when the new-file lines start..end are in the diff and hold + // the finding's existing_code, so a suggestion block replaces exactly that code. + suggest bool +} + +func (p placement) line() int { + if p.newLine > 0 { + return p.newLine + } + return p.oldLine } -// anchor picks the line a finding on [start,end] should be attached to: the last -// line of the range that is part of the diff. ok is false when no line qualifies. -func (fd *fileDiff) anchor(start, end int) (line int, info lineInfo, ok bool) { +// place picks the diff line a finding is attached to. OCR numbers lines of the +// new file, except when existing_code only occurs among context and removed +// lines: then they are old-file numbers, and the comment goes on the old side. +func (fd *fileDiff) place(c ocr.Comment) (placement, bool) { + start, end := c.StartLine, c.EndLine if end <= 0 { end = start } @@ -79,22 +109,59 @@ func (fd *fileDiff) anchor(start, end int) (line int, info lineInfo, ok bool) { start = end } if end <= 0 { - return 0, lineInfo{}, false + return placement{}, false } if fd.lines == nil { // Unknown patch: try the end line as an added line and let GitLab decide. - return end, lineInfo{added: true}, true + return placement{newLine: end, at: end}, true + } + if code := codeLines(c.ExistingCode); code != nil { + if matchLines(code, start, end, func(l int) (string, bool) { i, ok := fd.lines[l]; return i.text, ok }) { + return placement{newLine: end, oldLine: fd.lines[end].oldLine, at: end, suggest: true}, true + } + if matchLines(code, start, end, func(l int) (string, bool) { i, ok := fd.oldLines[l]; return i.text, ok }) { + return placement{newLine: fd.oldLines[end].newLine, oldLine: end, at: end}, true + } } for l := end; l >= start; l-- { if info, ok := fd.lines[l]; ok { - return l, info, true + return placement{newLine: l, oldLine: info.oldLine, at: l}, true + } + } + return placement{}, false +} + +// codeLines splits code into whitespace-normalized lines, or nil if it is blank. +func codeLines(code string) []string { + code = strings.Trim(code, "\n") + if strings.TrimSpace(code) == "" { + return nil + } + var out []string + for _, l := range strings.Split(code, "\n") { + out = append(out, strings.Join(strings.Fields(l), " ")) + } + return out +} + +// matchLines reports whether lines start..end all exist and equal code. +func matchLines(code []string, start, end int, get func(int) (string, bool)) bool { + if end-start+1 != len(code) { + return false + } + for i, want := range code { + got, ok := get(start + i) + if !ok || strings.Join(strings.Fields(got), " ") != want { + return false } } - return 0, lineInfo{}, false + return true } -// touchedOldRanges returns, per old path, the old-side line ranges of each hunk in -// a diff. Used to tell whether code a previous comment pointed at has changed. +// touchedOldRanges returns, per old path, the old-side lines a diff removes or +// replaces, plus the line after which a pure insertion happens. Used to tell +// whether code a previous comment pointed at has changed. Context lines do not +// count: GitLab puts a few unchanged lines around every hunk. func touchedOldRanges(diffs []gitlab.FileDiff) map[string][][2]int { out := map[string][][2]int{} for _, d := range diffs { @@ -105,21 +172,43 @@ func touchedOldRanges(diffs []gitlab.FileDiff) map[string][][2]int { out[d.OldPath] = append(out[d.OldPath], [2]int{1, int(^uint(0) >> 1)}) continue } + touch := func(line int) { + if line < 1 { + return + } + rs := out[d.OldPath] + if n := len(rs); n > 0 && line >= rs[n-1][0] && line <= rs[n-1][1]+1 { + rs[n-1][1] = max(rs[n-1][1], line) + return + } + out[d.OldPath] = append(rs, [2]int{line, line}) + } + oldLn, inHunk, removed := 0, false, false for _, l := range strings.Split(d.Diff, "\n") { - m := hunkHeader.FindStringSubmatch(l) - if m == nil { + if m := hunkHeader.FindStringSubmatch(l); m != nil { + oldLn, _ = strconv.Atoi(m[1]) + if m[2] == "0" { + oldLn++ // an empty old side names the line it follows + } + inHunk, removed = true, false continue } - start, _ := strconv.Atoi(m[1]) - count := 1 - if m[2] != "" { - count, _ = strconv.Atoi(m[2]) + if !inHunk || strings.HasPrefix(l, `\`) { + continue } - end := start + count - 1 - if count == 0 { - end = start // pure insertion after line `start` + switch { + case strings.HasPrefix(l, "-"): + touch(oldLn) + oldLn++ + removed = true + case strings.HasPrefix(l, "+"): + if !removed { + touch(oldLn - 1) // pure insertion after this old line + } + case strings.HasPrefix(l, " "): + oldLn++ + removed = false } - out[d.OldPath] = append(out[d.OldPath], [2]int{start, end}) } } return out diff --git a/internal/review/diff_test.go b/internal/review/diff_test.go index 209a5d0..27be819 100644 --- a/internal/review/diff_test.go +++ b/internal/review/diff_test.go @@ -8,12 +8,12 @@ import ( "pruefbyte/internal/ocr" ) -func TestParseNewLines(t *testing.T) { +func TestParsePatch(t *testing.T) { patch := "@@ -10,4 +10,5 @@ func x() {\n a\n-b\n+B\n+C\n c\n\\ No newline at end of file\n@@ -30,2 +31,2 @@\n d\n-e\n+E\n" - got := parseNewLines(patch) + got, old := parsePatch(patch) want := map[int]lineInfo{ - 10: {oldLine: 10}, 11: {added: true}, 12: {added: true}, 13: {oldLine: 12}, - 31: {oldLine: 30}, 32: {added: true}, + 10: {oldLine: 10, text: "a"}, 11: {added: true, text: "B"}, 12: {added: true, text: "C"}, 13: {oldLine: 12, text: "c"}, + 31: {oldLine: 30, text: "d"}, 32: {added: true, text: "E"}, } if len(got) != len(want) { t.Fatalf("got %v", got) @@ -23,10 +23,23 @@ func TestParseNewLines(t *testing.T) { t.Errorf("line %d: got %+v, want %+v", k, got[k], v) } } + wantOld := map[int]oldLineInfo{ + 10: {newLine: 10, text: "a"}, 11: {removed: true, text: "b"}, 12: {newLine: 13, text: "c"}, + 30: {newLine: 31, text: "d"}, 31: {removed: true, text: "e"}, + } + if len(old) != len(wantOld) { + t.Fatalf("old %v", old) + } + for k, v := range wantOld { + if old[k] != v { + t.Errorf("old line %d: got %+v, want %+v", k, old[k], v) + } + } } -func TestAnchor(t *testing.T) { - fd := &fileDiff{lines: parseNewLines(patchA)} +func TestPlace(t *testing.T) { + fd := &fileDiff{} + fd.lines, fd.oldLines = parsePatch(patchA) cases := []struct { start, end, want int ok bool @@ -39,14 +52,37 @@ func TestAnchor(t *testing.T) { {0, 0, 0, false}, } for _, c := range cases { - got, _, ok := fd.anchor(c.start, c.end) - if got != c.want || ok != c.ok { - t.Errorf("anchor(%d,%d) = %d,%v want %d,%v", c.start, c.end, got, ok, c.want, c.ok) + pl, ok := fd.place(ocr.Comment{StartLine: c.start, EndLine: c.end}) + if pl.newLine != c.want || ok != c.ok || pl.suggest { + t.Errorf("place(%d,%d) = %+v,%v want new line %d,%v", c.start, c.end, pl, ok, c.want, c.ok) } } unknown := &fileDiff{} - if l, info, ok := unknown.anchor(5, 7); !ok || l != 7 || !info.added { - t.Error("unknown patch should anchor on end line") + if pl, ok := unknown.place(ocr.Comment{StartLine: 5, EndLine: 7, ExistingCode: "x"}); !ok || pl.newLine != 7 || pl.suggest { + t.Errorf("unknown patch should anchor on end line without a suggestion, got %+v", pl) + } +} + +func TestPlaceChecksExistingCode(t *testing.T) { + // Old 1-5: a b check() c d; new 1-5: a b c d e. + fd := &fileDiff{} + fd.lines, fd.oldLines = parsePatch("@@ -1,5 +1,5 @@\n a\n b\n-check()\n c\n d\n+e\n") + + // Code on the new side: suggestion allowed. + pl, ok := fd.place(ocr.Comment{StartLine: 2, EndLine: 3, ExistingCode: "b\n c"}) + if !ok || pl.newLine != 3 || pl.oldLine != 4 || !pl.suggest { + t.Errorf("new-side match: %+v", pl) + } + // The quoted code was removed, so OCR's numbers are old-file lines: comment on + // the removed line, and never offer to overwrite new lines 1-3 with it. + pl, ok = fd.place(ocr.Comment{StartLine: 1, EndLine: 3, ExistingCode: "a\nb\ncheck()"}) + if !ok || pl.newLine != 0 || pl.oldLine != 3 || pl.suggest { + t.Errorf("old-side match: %+v", pl) + } + // Code that is nowhere at these lines: anchored as before, no suggestion. + pl, ok = fd.place(ocr.Comment{StartLine: 3, EndLine: 3, ExistingCode: "other()"}) + if !ok || pl.newLine != 3 || pl.suggest { + t.Errorf("mismatch: %+v", pl) } } @@ -55,6 +91,10 @@ func TestTouchedOldRanges(t *testing.T) { {OldPath: "a.go", Diff: "@@ -3,2 +3,0 @@\n-x\n-y\n@@ -20,0 +19,1 @@\n+z\n"}, {OldPath: "gone.go", DeletedFile: true}, {OldPath: "new.go", NewFile: true, Diff: "@@ -0,0 +1 @@\n+n\n"}, + // GitLab puts context around a change: only old line 12 changed here. + {OldPath: "ctx.go", Diff: "@@ -9,7 +9,7 @@\n l9\n l10\n l11\n-l12\n+L12\n l13\n l14\n l15\n"}, + // A pure insertion inside context touches the line it follows. + {OldPath: "ins.go", Diff: "@@ -4,4 +4,5 @@\n l4\n l5\n+new\n l6\n l7\n"}, }) if got := r["a.go"]; len(got) != 2 || got[0] != [2]int{3, 4} || got[1] != [2]int{20, 20} { t.Errorf("a.go ranges %v", got) @@ -65,12 +105,18 @@ func TestTouchedOldRanges(t *testing.T) { if _, ok := r["new.go"]; ok { t.Error("new files have no old lines") } + if got := r["ctx.go"]; len(got) != 1 || got[0] != [2]int{12, 12} { + t.Errorf("context lines counted as changed: %v", got) + } + if got := r["ins.go"]; len(got) != 1 || got[0] != [2]int{5, 5} { + t.Errorf("insertion ranges %v", got) + } } func TestFenceAndBodies(t *testing.T) { c := ocr.Comment{Path: "a.go", Content: "use a fence", StartLine: 2, EndLine: 2, ExistingCode: "x", SuggestionCode: "```go\ny\n```"} - body := inlineBody(c, "abc", 2, true) + body := inlineBody(c, "abc", placement{newLine: 2, at: 2, suggest: true}, true) if !strings.Contains(body, "````suggestion:-0+0\n```go") { t.Errorf("nested fence not escaped:\n%s", body) } @@ -79,11 +125,31 @@ func TestFenceAndBodies(t *testing.T) { } // Anchored on a different line than OCR's end line: no applicable suggestion. c.EndLine = 5 - body = inlineBody(c, "abc", 3, true) + body = inlineBody(c, "abc", placement{newLine: 3, at: 3}, true) if strings.Contains(body, "suggestion:") || !strings.Contains(body, "Suggested change") || !strings.Contains(body, "lines 2–5") { t.Errorf("unexpected body:\n%s", body) } - if body := inlineBody(c, "abc", 3, false); strings.Contains(body, "y\n") { + if body := inlineBody(c, "abc", placement{newLine: 3, at: 3}, false); strings.Contains(body, "y\n") { t.Error("suggestions disabled but code rendered") } } + +func TestQuickActionsNeutralized(t *testing.T) { + c := ocr.Comment{Path: "a.go", Content: "Missing check.\n/approve\n /merge\n```sh\n/usr/bin/x\n```\nsee /tmp", StartLine: 2, EndLine: 2} + body := inlineBody(c, "abc", placement{newLine: 2, at: 2}, true) + for _, l := range strings.Split(body, "\n") { + if t2 := strings.TrimLeft(l, " "); strings.HasPrefix(t2, "/approve") || strings.HasPrefix(t2, "/merge") { + t.Errorf("quick action left in body:\n%s", body) + } + } + if !strings.Contains(body, "\n/usr/bin/x\n") || !strings.Contains(body, "see /tmp") { + t.Errorf("code or inline text changed:\n%s", body) + } +} + +func TestFailureNoteFence(t *testing.T) { + s := summaryBody(summaryData{HeadSHA: "abc", Failure: "boom\n```json\n{}\n```\n/close"}) + if !strings.Contains(s, "````\nboom\n```json") || !strings.HasSuffix(strings.TrimSpace(s), "````\n\n") { + t.Errorf("failure text escapes its fence:\n%s", s) + } +} diff --git a/internal/review/filter.go b/internal/review/filter.go index 0218b3b..b690b0a 100644 --- a/internal/review/filter.go +++ b/internal/review/filter.go @@ -1,14 +1,11 @@ package review import ( - "fmt" - "regexp" "slices" "sort" "strings" "pruefbyte/internal/config" - "pruefbyte/internal/gitlab" "pruefbyte/internal/ocr" ) @@ -46,43 +43,3 @@ func filterFindings(cs []ocr.Comment, r config.Review) []ocr.Comment { }) return out } - -// skipReason returns why the MR should not be reviewed, or "". -func skipReason(s config.Skip, mr *gitlab.MR) string { - if s.Drafts && mr.Draft { - return "merge request is a draft (skip.drafts)" - } - for _, l := range mr.Labels { - if slices.Contains(s.Labels, l) { - return fmt.Sprintf("label %q (skip.labels)", l) - } - } - if slices.Contains(s.Authors, mr.Author) { - return fmt.Sprintf("author %q (skip.authors)", mr.Author) - } - for _, re := range s.TitleRegex { - if m, _ := regexp.MatchString(re, mr.Title); m { - return fmt.Sprintf("title matches %q (skip.title_regex)", re) - } - } - if matchAny(s.SourceBranches, mr.SourceBranch) { - return fmt.Sprintf("source branch %q (skip.source_branches)", mr.SourceBranch) - } - if matchAny(s.TargetBranches, mr.TargetBranch) { - return fmt.Sprintf("target branch %q (skip.target_branches)", mr.TargetBranch) - } - return "" -} - -// matchAny matches branch names exactly or as regular expressions anchored at both ends. -func matchAny(patterns []string, s string) bool { - for _, p := range patterns { - if p == s { - return true - } - if re, err := regexp.Compile("^(?:" + p + ")$"); err == nil && re.MatchString(s) { - return true - } - } - return false -} diff --git a/internal/review/format.go b/internal/review/format.go index c3cb279..23d1e3e 100644 --- a/internal/review/format.go +++ b/internal/review/format.go @@ -18,16 +18,24 @@ const ( var fpPattern = regexp.MustCompile(``) // fingerprint identifies a finding independently of its line numbers, so the same -// issue is recognised after unrelated lines shift. +// issue is recognised after unrelated lines shift. The model words a finding +// differently from run to run, so the code it points at (existing_code, which OCR's +// comment tool asks for) identifies it, as in OCR's own session comparison; the +// text is only a fallback. func fingerprint(c ocr.Comment) string { - norm := strings.Join(strings.Fields(strings.ToLower(c.Content)), " ") - h := sha1.Sum([]byte(c.Path + "\x00" + norm)) + key := "text\x00" + strings.ToLower(strings.Join(strings.Fields(c.Content), " ")) + if code := codeLines(c.ExistingCode); code != nil { + key = "code\x00" + strings.ToLower(strings.TrimSpace(c.Category)) + "\x00" + strings.Join(code, "\n") + } + h := sha1.Sum([]byte(c.Path + "\x00" + key)) return hex.EncodeToString(h[:8]) } +// extractFingerprint returns the marker inlineBody appends last; the finding text +// before it comes from the model and may contain marker-like text. func extractFingerprint(body string) string { - if m := fpPattern.FindStringSubmatch(body); m != nil { - return m[1] + if ms := fpPattern.FindAllStringSubmatch(body, -1); len(ms) > 0 { + return ms[len(ms)-1][1] } return "" } @@ -75,11 +83,11 @@ func fence(code string) string { return f } -// inlineBody renders a finding as a diff discussion anchored at anchorLine. -func inlineBody(c ocr.Comment, fp string, anchorLine int, suggestions bool) string { +// inlineBody renders a finding as a diff discussion placed at pl. +func inlineBody(c ocr.Comment, fp string, pl placement, suggestions bool) string { var b strings.Builder head := badge(c) - if anchorLine != c.EndLine || c.EndLine > c.StartLine && c.StartLine > 0 { + if pl.at != c.EndLine || c.EndLine > c.StartLine && c.StartLine > 0 { if l := lineLabel(c); l != "" { if head != "" { head += " " @@ -90,14 +98,14 @@ func inlineBody(c ocr.Comment, fp string, anchorLine int, suggestions bool) stri if head != "" { b.WriteString(head + "\n\n") } - b.WriteString(strings.TrimSpace(c.Content)) + b.WriteString(neutralizeQuickActions(strings.TrimSpace(c.Content))) b.WriteString("\n") if code := strings.TrimRight(c.SuggestionCode, "\n"); code != "" && suggestions { f := fence(code) - // A GitLab suggestion replaces the anchor line plus N lines above it, which - // only matches OCR's range when we anchored on its last line. - if c.ExistingCode != "" && anchorLine == c.EndLine && c.StartLine > 0 && c.StartLine <= c.EndLine { + // A GitLab suggestion replaces the anchor line plus N lines above it. That is + // only safe when those lines hold the code OCR quoted (see place). + if pl.suggest && c.StartLine > 0 && c.StartLine <= c.EndLine { fmt.Fprintf(&b, "\n%ssuggestion:-%d+0\n%s\n%s\n", f, c.EndLine-c.StartLine, code, f) } else { fmt.Fprintf(&b, "\nSuggested change:\n\n%s\n%s\n%s\n", f, code, f) @@ -107,12 +115,36 @@ func inlineBody(c ocr.Comment, fp string, anchorLine int, suggestions bool) stri return b.String() } +// neutralizeQuickActions escapes a leading "/" on lines outside code fences. +// GitLab runs such lines in the bot's comments as quick actions (/approve, +// /merge, /close, ...), and the text comes from a model that reads the MR. +// "\/" renders as "/". +func neutralizeQuickActions(s string) string { + lines := strings.Split(s, "\n") + fence := "" + for i, l := range lines { + t := strings.TrimLeft(l, " \t") + switch { + case fence != "": + if strings.HasPrefix(t, fence) { + fence = "" + } + case strings.HasPrefix(t, "```") || strings.HasPrefix(t, "~~~"): + fence = t[:3] + case strings.HasPrefix(t, "/"): + lines[i] = l[:len(l)-len(t)] + `\` + t + } + } + return strings.Join(lines, "\n") +} + type summaryData struct { Model string HeadSHA string Stats Stats Unplaced []ocr.Comment Overflow []ocr.Comment + Failed []ocr.Comment // could not be posted MaxComments int Warnings []ocr.Warning Failure string @@ -127,7 +159,8 @@ func summaryBody(d summaryData) string { sha = sha[:8] } if d.Failure != "" { - fmt.Fprintf(&b, "⚠️ The review of `%s` failed.\n\n
Error\n\n```\n%s\n```\n\n
\n", sha, d.Failure) + f := fence(d.Failure) + fmt.Fprintf(&b, "⚠️ The review of `%s` failed.\n\n
Error\n\n%s\n%s\n%s\n\n
\n", sha, f, d.Failure, f) return b.String() } fmt.Fprintf(&b, "Reviewed `%s`", sha) @@ -161,6 +194,7 @@ func summaryBody(d summaryData) string { } list("Findings outside the diff", d.Unplaced) list(fmt.Sprintf("Further findings (over the limit of %d inline comments)", d.MaxComments), d.Overflow) + list("Findings that could not be posted (see the job log)", d.Failed) if len(d.Warnings) > 0 { b.WriteString("\n
OCR warnings\n\n") for _, w := range d.Warnings { @@ -168,7 +202,7 @@ func summaryBody(d summaryData) string { } b.WriteString("\n
\n") } - if len(d.Unplaced)+len(d.Overflow)+len(d.Warnings) == 0 && !d.Incomplete { + if len(d.Unplaced)+len(d.Overflow)+len(d.Failed)+len(d.Warnings) == 0 && !d.Incomplete { b.WriteString("\nAll findings are posted inline.\n") } return b.String() diff --git a/internal/review/review.go b/internal/review/review.go index 72f7e1d..7e9538d 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -7,6 +7,10 @@ import ( "errors" "fmt" "io" + "io/fs" + "os" + "path" + "path/filepath" "strings" "pruefbyte/internal/config" @@ -26,8 +30,13 @@ type Deps struct { // LoadConfig returns the effective config, given the MR's base commit // (where the repository's config file is read from). LoadConfig func(ctx context.Context, baseSHA string) (config.Config, error) + // PrepareOCR optionally configures the review engine. + PrepareOCR func(ctx context.Context, cfg config.Config) error // EnsureCommits makes sure the given commits exist in the local clone. EnsureCommits func(ctx context.Context, shas ...string) error + // ReadFileAt returns path at commit rev, or found=false. Rule files OCR would + // read from the merge request's checkout are taken from the base commit instead. + ReadFileAt func(ctx context.Context, rev, path string) (data []byte, found bool, err error) // SaveResult optionally keeps OCR's raw JSON, e.g. as a CI artifact. SaveResult func(raw []byte) error // Redact masks secrets in text that gets posted to the MR. @@ -64,6 +73,10 @@ var ErrReviewFailed = errors.New("review failed") func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { logf := func(format string, a ...any) { fmt.Fprintf(d.Log, "[pruefbyte] "+format+"\n", a...) } + skip := func(mr *gitlab.MR, reason string) (*Outcome, error) { + logf("skipping !%d: %s", mr.IID, reason) + return &Outcome{Skipped: reason}, nil + } botID, err := d.GitLab.CurrentUserID(ctx) if err != nil { @@ -76,33 +89,41 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { if mr.BaseSHA == "" || mr.HeadSHA == "" { return nil, fmt.Errorf("merge request !%d has no diff refs yet; retry once GitLab has computed the diff", mr.IID) } - cfg, err := d.LoadConfig(ctx, mr.BaseSHA) - if err != nil { - return nil, err - } - if reason := skipReason(cfg.Skip, mr); reason != "" { - logf("skipping !%d: %s", mr.IID, reason) - return &Outcome{Skipped: reason}, nil - } if opts.ExpectedHeadSHA != "" && opts.ExpectedHeadSHA != mr.HeadSHA { - reason := fmt.Sprintf("merge request head moved from %.8s to %.8s; a newer pipeline reviews it", opts.ExpectedHeadSHA, mr.HeadSHA) - logf("skipping !%d: %s", mr.IID, reason) - return &Outcome{Skipped: reason}, nil + return skip(mr, fmt.Sprintf("merge request head moved from %.8s to %.8s; a newer pipeline reviews it", opts.ExpectedHeadSHA, mr.HeadSHA)) } + // The repository config is read from the base commit, so fetch before loading it. if d.EnsureCommits != nil { if err := d.EnsureCommits(ctx, mr.BaseSHA, mr.HeadSHA); err != nil { return nil, err } } + cfg, err := d.LoadConfig(ctx, mr.BaseSHA) + if err != nil { + return nil, err + } + if d.PrepareOCR != nil { + if err := d.PrepareOCR(ctx, cfg); err != nil { + return nil, err + } + } existing, err := d.GitLab.ListDiscussions(ctx) if err != nil { return nil, err } state := collectBotState(existing, botID) + // Read the diff now: positions carry the SHAs read above, and once OCR is done + // GitLab may already list the diff of a newer push. (Positions on an older + // version are fine as long as their lines are typed from that version.) + diffs, err := d.GitLab.ListDiffs(ctx) + if err != nil { + return nil, err + } + idx := buildDiffIndex(diffs) logf("reviewing !%d %s (%.8s..%.8s)", mr.IID, mr.Title, mr.BaseSHA, mr.HeadSHA) - ro, err := reviewOptions(cfg, mr, opts.RepoDir, d.OCR) + ro, err := reviewOptions(ctx, cfg, mr, opts.RepoDir, d) if err != nil { return nil, err } @@ -135,30 +156,34 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { } logf("ocr finished with status %s: %d finding(s)", res.Status, len(res.Comments)) - diffs, err := d.GitLab.ListDiffs(ctx) - if err != nil { - return nil, err - } - idx := buildDiffIndex(diffs) - findings := filterFindings(res.Comments, cfg.Review) out := &Outcome{} st := &out.Stats st.Findings = len(findings) + // A finding OCR reports again keeps its thread open, even when it now falls + // below min_severity or outside review.categories. current := map[string]bool{} - var unplaced, overflow []ocr.Comment + for _, c := range res.Comments { + current[fingerprint(c)] = true + } + seen := map[string]bool{} + var unplaced, overflow, failed []ocr.Comment for _, c := range findings { fp := fingerprint(c) - if current[fp] { - continue // the same finding twice in one run + // Two findings share a fingerprint when they quote the same code; only an + // identical report at the same place is the same finding twice in one run. + key := fmt.Sprintf("%s:%d-%d:%s", fp, c.StartLine, c.EndLine, strings.ToLower(strings.Join(strings.Fields(c.Content), " "))) + if seen[key] { + continue } - current[fp] = true + seen[key] = true if state.fingerprints[fp] { st.Duplicates++ continue } - if cfg.Review.MaxComments > 0 && st.Posted >= cfg.Review.MaxComments { + // The limit is for the MR: the bot's open threads from earlier runs count. + if cfg.Review.MaxComments > 0 && len(state.open)+st.Posted >= cfg.Review.MaxComments { overflow = append(overflow, c) continue } @@ -167,35 +192,32 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { unplaced = append(unplaced, c) continue } - line, info, ok := fd.anchor(c.StartLine, c.EndLine) + pl, ok := fd.place(c) if !ok { unplaced = append(unplaced, c) continue } pos := &gitlab.Position{ BaseSHA: mr.BaseSHA, StartSHA: mr.StartSHA, HeadSHA: mr.HeadSHA, - OldPath: fd.oldPath, NewPath: c.Path, NewLine: line, - } - if !info.added { - pos.OldLine = info.oldLine + OldPath: fd.oldPath, NewPath: c.Path, NewLine: pl.newLine, OldLine: pl.oldLine, } - err := d.GitLab.CreateDiscussion(ctx, inlineBody(c, fp, line, cfg.Review.Suggestions), pos) + err := d.GitLab.CreateDiscussion(ctx, inlineBody(c, fp, pl, cfg.Review.Suggestions), pos) switch { case err == nil: st.Posted++ case gitlab.IsBadRequest(err): - logf("GitLab rejected the position %s:%d, listing it in the summary: %v", c.Path, line, err) + logf("GitLab rejected the position %s:%d, listing it in the summary: %v", c.Path, pl.line(), err) unplaced = append(unplaced, c) default: - logf("warning: posting comment on %s:%d: %v", c.Path, line, err) - st.Failed++ + logf("warning: posting comment on %s:%d: %v", c.Path, pl.line(), err) + failed = append(failed, c) } } - st.Unplaced, st.Overflow = len(unplaced), len(overflow) + st.Unplaced, st.Overflow, st.Failed = len(unplaced), len(overflow), len(failed) if cfg.Review.ResolveOutdated { if res.Complete() && st.Failed == 0 { - st.Resolved = resolveOutdated(ctx, d.GitLab, state, current, idx, mr.HeadSHA, logf) + st.Resolved = resolveOutdated(ctx, d.GitLab, state, current, idx, res, mr.HeadSHA, logf) } else { logf("not resolving earlier threads: this review was incomplete") } @@ -203,10 +225,10 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { summary := summaryData{ Model: res.LLM.Model, HeadSHA: mr.HeadSHA, Stats: *st, - Unplaced: unplaced, Overflow: overflow, MaxComments: cfg.Review.MaxComments, + Unplaced: unplaced, Overflow: overflow, Failed: failed, MaxComments: cfg.Review.MaxComments, Warnings: res.Warnings, Incomplete: !res.Complete(), } - if len(unplaced)+len(overflow)+len(res.Warnings) > 0 || !res.Complete() || state.summary != nil { + if len(unplaced)+len(overflow)+len(failed)+len(res.Warnings) > 0 || !res.Complete() || state.summary != nil { if err := upsertSummary(ctx, d.GitLab, state, summaryBody(summary)); err != nil { logf("warning: posting summary note: %v", err) } @@ -229,29 +251,79 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { return out, nil } -func reviewOptions(cfg config.Config, mr *gitlab.MR, repoDir string, r Reviewer) (ocr.ReviewOptions, error) { +func reviewOptions(ctx context.Context, cfg config.Config, mr *gitlab.MR, repoDir string, d Deps) (ocr.ReviewOptions, error) { ro := ocr.ReviewOptions{ RepoDir: repoDir, From: mr.BaseSHA, To: mr.HeadSHA, Provider: cfg.LLM.Provider, Model: cfg.LLM.Model, Effort: cfg.OCR.Effort, Concurrency: cfg.OCR.Concurrency, - MaxTokensBudget: cfg.OCR.MaxTokensBudget, Exclude: cfg.OCR.Exclude, - RuleFile: cfg.OCR.RuleFile, ExtraArgs: cfg.OCR.ExtraArgs, + MaxTokensBudget: cfg.OCR.MaxTokensBudget, ExtraArgs: cfg.OCR.ExtraArgs, } - if len(cfg.OCR.Rules) > 0 { - data, err := ocr.RuleJSON(cfg.OCR.Rules) + // OCR reads rule files, and the files their rules name, from the checkout, + // which the merge request controls. Like .pruefbyte.yml they come from the base + // commit, merged into one --rule file that also keeps the checkout's + // .opencodereview/rule.json from applying. + atBase := func(dir string) func(string) ([]byte, bool, error) { + return func(name string) ([]byte, bool, error) { + p := path.Join(dir, filepath.ToSlash(name)) + if filepath.IsAbs(name) || path.IsAbs(filepath.ToSlash(name)) || p == ".." || strings.HasPrefix(p, "../") { + return nil, false, nil // outside the repository, where OCR does not read either + } + if d.ReadFileAt == nil { + return readFile(filepath.Join(repoDir, filepath.FromSlash(p))) + } + return d.ReadFileAt(ctx, mr.BaseSHA, p) + } + } + layers := []ocr.RuleFile{{Rules: cfg.OCR.Rules}} + for _, p := range []string{cfg.OCR.RuleFile, ".opencodereview/rule.json"} { + if p == "" { + continue + } + // An absolute ocr.rule_file lives on the runner; so do the files it names. + read := atBase(path.Dir(filepath.ToSlash(p))) + if filepath.IsAbs(p) { + read = func(name string) ([]byte, bool, error) { + if !filepath.IsAbs(name) { + name = filepath.Join(filepath.Dir(p), name) + } + return readFile(name) + } + } + data, found, err := read(filepath.Base(p)) if err != nil { - return ro, err + return ro, fmt.Errorf("reading %s: %w", p, err) } - if ro.RuleFile, err = r.WriteTemp("rule.json", data); err != nil { - return ro, err + if !found { + if p == cfg.OCR.RuleFile { + return ro, fmt.Errorf("ocr.rule_file %s not found (relative paths are read at the base commit %.8s)", p, mr.BaseSHA) + } + continue + } + rf, err := ocr.ParseRuleFile(data) + if err == nil { + if p != cfg.OCR.RuleFile { + read = atBase(".") // the project file's rule files are relative to the repository root + } + err = rf.InlineFiles(read) + } + if err != nil { + return ro, fmt.Errorf("%s: %w", p, err) } + layers = append(layers, rf) + } + data, err := ocr.RuleJSON(layers, cfg.OCR.Exclude) + if err != nil { + return ro, err + } + if ro.RuleFile, err = d.OCR.WriteTemp("rule.json", data); err != nil { + return ro, err } if cfg.OCR.UseMRDescriptionAsBackground { bg := "# " + mr.Title + "\n\n" + mr.Description - if r := []rune(bg); len(r) > 8000 { - bg = string(r[:8000]) + if runes := []rune(bg); len(runes) > 8000 { + bg = string(runes[:8000]) } - p, err := r.WriteTemp("background.md", []byte(bg)) + p, err := d.OCR.WriteTemp("background.md", []byte(bg)) if err != nil { return ro, err } @@ -260,6 +332,15 @@ func reviewOptions(cfg config.Config, mr *gitlab.MR, repoDir string, r Reviewer) return ro, nil } +// readFile is os.ReadFile with found=false for a missing file. +func readFile(name string) ([]byte, bool, error) { + data, err := os.ReadFile(name) + if errors.Is(err, fs.ErrNotExist) { + return nil, false, nil + } + return data, err == nil, err +} + type botState struct { fingerprints map[string]bool open []botThread @@ -270,6 +351,9 @@ type botThread struct { discussionID string fingerprint string pos *gitlab.Position + // reopened is set when the bot resolved the thread before and someone + // unresolved it; it is left alone from then on. + reopened bool } // collectBotState finds everything the bot posted earlier: fingerprints (resolved @@ -284,7 +368,8 @@ func collectBotState(ds []gitlab.Discussion, botID int64) botState { if first.AuthorID != botID || first.System { continue } - if strings.HasPrefix(first.Body, summaryMarker) { + // The summary is a plain note; an inline finding could start with the marker text. + if first.Position == nil && strings.HasPrefix(first.Body, summaryMarker) { n := first s.summary = &n continue @@ -295,26 +380,45 @@ func collectBotState(ds []gitlab.Discussion, botID int64) botState { } s.fingerprints[fp] = true if first.Resolvable && !first.Resolved && first.Position != nil { - s.open = append(s.open, botThread{discussionID: d.ID, fingerprint: fp, pos: first.Position}) + s.open = append(s.open, botThread{discussionID: d.ID, fingerprint: fp, pos: first.Position, + reopened: autoResolvedBefore(d.Notes[1:], botID)}) } } return s } +const autoResolvedPrefix = "Resolved automatically:" + +func autoResolvedBefore(replies []gitlab.Note, botID int64) bool { + for _, n := range replies { + if n.AuthorID == botID && strings.HasPrefix(n.Body, autoResolvedPrefix) { + return true + } + } + return false +} + // resolveOutdated resolves the bot's open threads whose finding was not reported -// again and whose code changed since the comment was made. Requiring the change -// keeps an LLM that merely missed the issue this time from closing the thread. -func resolveOutdated(ctx context.Context, api gitlab.API, s botState, current map[string]bool, idx diffIndex, headSHA string, logf func(string, ...any)) int { +// again, in a file this review covered, and whose code changed since the comment +// was made. Requiring the change keeps an LLM that merely missed the issue this +// time from closing the thread. +func resolveOutdated(ctx context.Context, api gitlab.API, s botState, current map[string]bool, idx diffIndex, res *ocr.Result, headSHA string, logf func(string, ...any)) int { resolved := 0 compares := map[string]map[string][][2]int{} for _, t := range s.open { - if current[t.fingerprint] || t.pos.HeadSHA == headSHA { + if current[t.fingerprint] || t.reopened || t.pos.HeadSHA == headSHA { continue } changed := false - if idx[t.pos.NewPath] == nil { + switch { + case idx[t.pos.NewPath] == nil: changed = true // the file is no longer part of the MR - } else { + case !res.Reviewed(t.pos.NewPath): + continue // OCR did not look at the file this time + case t.pos.NewLine == 0: + // A comment on removed code that GitLab no longer traces to the current diff. + changed = true + default: ranges, ok := compares[t.pos.HeadSHA] if !ok { diffs, err := api.Compare(ctx, t.pos.HeadSHA, headSHA) @@ -324,12 +428,8 @@ func resolveOutdated(ctx context.Context, api gitlab.API, s botState, current ma ranges = touchedOldRanges(diffs) compares[t.pos.HeadSHA] = ranges } - line := t.pos.NewLine - if line == 0 { - line = t.pos.OldLine - } for _, r := range ranges[t.pos.NewPath] { - if line >= r[0] && line <= r[1] { + if t.pos.NewLine >= r[0] && t.pos.NewLine <= r[1] { changed = true break } @@ -338,7 +438,7 @@ func resolveOutdated(ctx context.Context, api gitlab.API, s botState, current ma if !changed { continue } - msg := fmt.Sprintf("Resolved automatically: the code changed in `%.8s` and this finding was not reported again.", headSHA) + msg := fmt.Sprintf("%s the code changed in `%.8s` and this finding was not reported again.", autoResolvedPrefix, headSHA) if err := api.ReplyAndResolve(ctx, t.discussionID, msg); err != nil { logf("warning: resolving discussion %s: %v", t.discussionID, err) continue diff --git a/internal/review/review_test.go b/internal/review/review_test.go index f41afa9..a111e8d 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -24,11 +24,11 @@ type fakeGitLab struct { diffs []gitlab.FileDiff compare map[string][]gitlab.FileDiff discussions []gitlab.Discussion - // rejectLine makes CreateDiscussion fail with 400 for this new_line. - rejectLine int - nextID int64 - resolved []string - updates int + // rejectLine makes CreateDiscussion fail with 400 for this new_line, failLine with 500. + rejectLine, failLine int + nextID int64 + resolved []string + updates int } func (f *fakeGitLab) CurrentUserID(context.Context) (int64, error) { return botID, nil } @@ -42,6 +42,9 @@ func (f *fakeGitLab) CreateDiscussion(_ context.Context, body string, pos *gitla if pos != nil && pos.NewLine == f.rejectLine { return &gl.ErrorResponse{Response: &http.Response{StatusCode: 400}, Message: "400 {line_code: [can't be blank]}"} } + if pos != nil && pos.NewLine == f.failLine { + return &gl.ErrorResponse{Response: &http.Response{StatusCode: 500}, Message: "500 Internal Server Error"} + } f.nextID++ p := *pos f.discussions = append(f.discussions, gitlab.Discussion{ @@ -73,16 +76,27 @@ func (f *fakeGitLab) UpdateNote(_ context.Context, id int64, body string) error return errors.New("note not found") } -func (f *fakeGitLab) ReplyAndResolve(_ context.Context, id, _ string) error { +func (f *fakeGitLab) ReplyAndResolve(_ context.Context, id, body string) error { f.resolved = append(f.resolved, id) for i := range f.discussions { if f.discussions[i].ID == id { - f.discussions[i].Notes[0].Resolved = true + f.nextID++ + f.discussions[i].Notes = append(f.discussions[i].Notes, gitlab.Note{ID: f.nextID, AuthorID: botID, Body: body}) + for j := range f.discussions[i].Notes { + f.discussions[i].Notes[j].Resolved = true + } } } return nil } +// setResolved resolves or unresolves thread i, as a person would. +func (f *fakeGitLab) setResolved(i int, resolved bool) { + for j := range f.discussions[i].Notes { + f.discussions[i].Notes[j].Resolved = resolved + } +} + func (f *fakeGitLab) Compare(_ context.Context, from, to string) ([]gitlab.FileDiff, error) { return f.compare[from+".."+to], nil } @@ -107,16 +121,31 @@ func (f *fakeGitLab) summary() string { } type fakeOCR struct { - res *ocr.Result - err error - opts ocr.ReviewOptions + res *ocr.Result + err error + opts ocr.ReviewOptions + calls int + temps map[string][]byte + // during runs inside Review, e.g. to simulate a push while OCR works. + during func() } func (f *fakeOCR) Review(_ context.Context, o ocr.ReviewOptions) (*ocr.Result, []byte, error) { f.opts = o + f.calls++ + if f.during != nil { + f.during() + } return f.res, []byte("{}"), f.err } -func (f *fakeOCR) WriteTemp(name string, _ []byte) (string, error) { return "/tmp/" + name, nil } + +func (f *fakeOCR) WriteTemp(name string, data []byte) (string, error) { + if f.temps == nil { + f.temps = map[string][]byte{} + } + f.temps[name] = data + return "/tmp/" + name, nil +} // a.go: lines 1-2 context, 3-4 added, 5 context. b.go is new. const patchA = "@@ -1,3 +1,5 @@\n ctx1\n ctx2\n+add3\n+add4\n ctx5\n" @@ -362,42 +391,257 @@ func TestFailurePostsRedactedNote(t *testing.T) { } } -func TestSkips(t *testing.T) { +// When to run is the CI job's decision: pruefbyte reviews whatever it is run on. +// It only skips a pipeline whose commit is no longer the MR head. +func TestSkipsOnlyStalePipelines(t *testing.T) { f := newFake() - f.mr.Draft = true - o := &fakeOCR{res: result()} - out, err := Run(context.Background(), deps(f, o, testConfig()), Options{}) - if err != nil || out.Skipped == "" { - t.Fatalf("draft not skipped: %+v %v", out, err) + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "x", StartLine: 3, EndLine: 3})} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{ExpectedHeadSHA: "head1"}) + if err != nil || out.Skipped != "" || out.Stats.Posted != 1 { + t.Fatalf("not reviewed: %+v %v", out, err) } - f.mr.Draft = false + calls := o.calls out, _ = Run(context.Background(), deps(f, o, testConfig()), Options{ExpectedHeadSHA: "older"}) if out.Skipped == "" { t.Error("stale pipeline not skipped") } - if o.opts.RepoDir != "" { - t.Error("ocr ran for a skipped MR") - } -} - -func TestSkipReason(t *testing.T) { - s := config.Skip{Labels: []string{"no-review"}, Authors: []string{"renovate"}, TitleRegex: []string{`^\[WIP\]`}, - SourceBranches: []string{"release/.*"}, TargetBranches: []string{"legacy"}} - cases := []struct { - mr gitlab.MR - skip bool - }{ - {gitlab.MR{Title: "ok", Author: "alice", SourceBranch: "feat", TargetBranch: "main"}, false}, - {gitlab.MR{Labels: []string{"no-review"}}, true}, - {gitlab.MR{Author: "renovate"}, true}, - {gitlab.MR{Title: "[WIP] thing"}, true}, - {gitlab.MR{SourceBranch: "release/1.2"}, true}, - {gitlab.MR{SourceBranch: "xrelease/1.2"}, false}, - {gitlab.MR{TargetBranch: "legacy"}, true}, - } - for _, c := range cases { - if got := skipReason(s, &c.mr) != ""; got != c.skip { - t.Errorf("%+v: skip=%v, want %v", c.mr, got, c.skip) + if o.calls != calls { + t.Error("ocr ran for a stale pipeline") + } +} + +func TestSameTextOnTwoLinesIsPostedTwice(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "Error is ignored.", StartLine: 3, EndLine: 3, ExistingCode: "add3"}, + ocr.Comment{Path: "a.go", Content: "Error is ignored.", StartLine: 4, EndLine: 4, ExistingCode: "add4"}, + ocr.Comment{Path: "a.go", Content: "Error is ignored.", StartLine: 4, EndLine: 4, ExistingCode: "add4"}, // repeated + )} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Posted != 2 || len(f.inline()) != 2 { + t.Errorf("stats %+v, %d threads", out.Stats, len(f.inline())) + } +} + +func TestRewordedFindingIsDuplicate(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "Possible nil dereference.", StartLine: 3, EndLine: 3, ExistingCode: "add3", Category: "bug"})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + o.res = result(ocr.Comment{Path: "a.go", Content: "x may be nil here.", StartLine: 3, EndLine: 3, ExistingCode: " add3 ", Category: "Bug"}) + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Posted != 0 || out.Stats.Duplicates != 1 || len(f.inline()) != 1 { + t.Errorf("stats %+v, %d threads", out.Stats, len(f.inline())) + } +} + +func TestDifferentIssuesOnSameCodeBothPosted(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "Nil dereference.", StartLine: 3, EndLine: 3, ExistingCode: "add3", Category: "bug"}, + ocr.Comment{Path: "a.go", Content: "Error ignored.", StartLine: 3, EndLine: 3, ExistingCode: "add3", Category: "bug"}, + )} + out, err := Run(context.Background(), deps(f, o, testConfig()), Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Posted != 2 { + t.Errorf("stats %+v", out.Stats) + } +} + +func TestMaxCommentsCountsOpenThreads(t *testing.T) { + f := newFake() + cfg := testConfig() + cfg.Review.MaxComments = 1 + o := &fakeOCR{res: result( + ocr.Comment{Path: "a.go", Content: "one", StartLine: 3, EndLine: 3, Severity: "high"}, + ocr.Comment{Path: "a.go", Content: "two", StartLine: 4, EndLine: 4, Severity: "high"}, + ocr.Comment{Path: "a.go", Content: "three", StartLine: 5, EndLine: 5, Severity: "high"}, + )} + d := deps(f, o, cfg) + for i := 0; i < 3; i++ { // the pipeline is retried + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + } + if n := len(f.inline()); n != 1 { + t.Errorf("%d inline threads with max_comments=1", n) + } + if s := f.summary(); !strings.Contains(s, "two") || !strings.Contains(s, "three") { + t.Errorf("overflow missing from summary:\n%s", s) + } +} + +func TestReopenedThreadIsNotResolvedAgain(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "still wrong", StartLine: 3, EndLine: 3})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + f.mr.HeadSHA = "head2" + f.compare["head1..head2"] = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -3 +3 @@\n-add3\n+fix3\n"}} + o.res = result() + if _, err := Run(context.Background(), d, Options{}); err != nil || len(f.resolved) != 1 { + t.Fatalf("first resolve: %v %v", f.resolved, err) + } + f.setResolved(0, false) // a reviewer disagrees and reopens it + f.mr.HeadSHA = "head3" + f.compare["head1..head3"] = f.compare["head1..head2"] + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + if len(f.resolved) != 1 { + t.Errorf("reopened thread resolved again: %v", f.resolved) + } +} + +func TestDiffIsReadBeforeTheReview(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "on ctx5", StartLine: 5, EndLine: 5})} + // A push lands while OCR runs; outside CI nothing re-checks the head. + o.during = func() { + f.diffs = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -1,3 +1,7 @@\n+n1\n+n2\n ctx1\n ctx2\n+add3\n+add4\n ctx5\n"}} + } + if _, err := Run(context.Background(), deps(f, o, testConfig()), Options{}); err != nil { + t.Fatal(err) + } + if p := f.inline()[0].Notes[0].Position; p.HeadSHA != "head1" || p.NewLine != 5 || p.OldLine != 3 { + t.Errorf("position from the wrong diff version: %+v", p) + } +} + +func TestReRatedFindingKeepsThreadOpen(t *testing.T) { + f := newFake() + cfg := testConfig() + cfg.Review.MinSeverity = "medium" + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "Error from Close is ignored.", StartLine: 3, EndLine: 3, Severity: "medium"})} + d := deps(f, o, cfg) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + // Line 3 is edited without a fix; OCR reports the same finding, now as low. + f.mr.HeadSHA = "head2" + f.compare["head1..head2"] = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -3 +3 @@\n-add3\n+add3 // edited\n"}} + o.res = result(ocr.Comment{Path: "a.go", Content: "Error from Close is ignored.", StartLine: 3, EndLine: 3, Severity: "low"}) + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Resolved != 0 { + t.Error("thread resolved although OCR still reports the finding") + } +} + +func TestResolveOnlyInReviewedFiles(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "a.go", Content: "x", StartLine: 3, EndLine: 3})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + f.mr.HeadSHA = "head2" + f.compare["head1..head2"] = []gitlab.FileDiff{{OldPath: "a.go", NewPath: "a.go", Diff: "@@ -3 +3 @@\n-add3\n+fix3\n"}} + // a.go grew too large and OCR left it out: complete, but a.go was not reviewed. + o.res = &ocr.Result{Status: "complete", Manifest: &ocr.Manifest{}} + o.res.Manifest.Coverage.Completed = []ocr.CoveredItem{{Path: "b.go"}} + out, err := Run(context.Background(), d, Options{}) + if err != nil { + t.Fatal(err) + } + if out.Stats.Resolved != 0 { + t.Error("resolved a thread in a file OCR did not review") + } +} + +func TestRuleFileComesFromBaseCommit(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result()} + cfg := testConfig() + cfg.OCR.Exclude = []string{"**/*.{pb,gen}.go"} + d := deps(f, o, cfg) + var reads []string + d.ReadFileAt = func(_ context.Context, rev, path string) ([]byte, bool, error) { + reads = append(reads, rev+":"+path) + switch path { + case ".opencodereview/rule.json": + return []byte(`{"exclude": ["docs/**"], "rules": [ + {"path": "**/*.go", "rule": "base rule"}, + {"path": "web/**", "rule": "docs/web-rules.md"}, + {"path": "x/**", "rule": "../outside.md"}]}`), true, nil + case "docs/web-rules.md": + return []byte("web rule at base\n"), true, nil } + return nil, false, nil + } + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + rf, err := ocr.ParseRuleFile(o.temps["rule.json"]) + if err != nil || o.opts.RuleFile == "" { + t.Fatalf("no rule file passed: %v %+v", err, o.opts) + } + if strings.Join(reads, " ") != "base:.opencodereview/rule.json base:docs/web-rules.md" { + t.Errorf("reads %v", reads) + } + if strings.Join(rf.Exclude, ",") != "docs/**,**/*.{pb,gen}.go" || len(rf.Rules) != 4 || rf.Rules[3].Path != "**/*" { + t.Fatalf("rule file %+v", rf) + } + // Rules naming a file get its content from the base commit; a path outside the + // repository is dropped, as OCR does. + if rf.Rules[0].Rule != "base rule" || rf.Rules[1].Rule != "web rule at base" || rf.Rules[2].Rule != "" { + t.Errorf("rules %+v", rf.Rules) + } +} + +func TestFetchBeforeConfigBeforeOCRSetup(t *testing.T) { + f := newFake() + var steps []string + d := deps(f, &fakeOCR{res: result()}, testConfig()) + d.EnsureCommits = func(context.Context, ...string) error { steps = append(steps, "fetch"); return nil } + load := d.LoadConfig + d.LoadConfig = func(ctx context.Context, sha string) (config.Config, error) { + steps = append(steps, "config") + return load(ctx, sha) + } + d.PrepareOCR = func(context.Context, config.Config) error { steps = append(steps, "prepare"); return nil } + out, err := Run(context.Background(), d, Options{}) + if err != nil || out.Skipped != "" { + t.Fatalf("run: %+v %v", out, err) + } + if strings.Join(steps, ",") != "fetch,config,prepare" { + t.Errorf("steps %v", steps) + } + // A stale pipeline is skipped before anything is fetched or loaded. + steps = nil + if out, _ := Run(context.Background(), d, Options{ExpectedHeadSHA: "older"}); out.Skipped == "" || len(steps) != 0 { + t.Errorf("stale run: %+v, steps %v", out, steps) + } +} + +func TestFailedPostIsListedInSummary(t *testing.T) { + f := newFake() + o := &fakeOCR{res: result(ocr.Comment{Path: "untouched.go", Content: "elsewhere", StartLine: 1, EndLine: 1})} + d := deps(f, o, testConfig()) + if _, err := Run(context.Background(), d, Options{}); err != nil { + t.Fatal(err) + } + f.failLine = 3 + o.res = result(ocr.Comment{Path: "a.go", Content: "cannot be posted", StartLine: 3, EndLine: 3}) + out, err := Run(context.Background(), d, Options{}) + if err == nil || out.Stats.Failed != 1 { + t.Fatalf("stats %+v, err %v", out.Stats, err) + } + if s := f.summary(); !strings.Contains(s, "cannot be posted") || strings.Contains(s, "All findings are posted inline") { + t.Errorf("summary hides the failed finding:\n%s", s) } } diff --git a/pruefbyte.example.yml b/pruefbyte.example.yml index 6c69ebc..27cb56c 100644 --- a/pruefbyte.example.yml +++ b/pruefbyte.example.yml @@ -26,24 +26,19 @@ ocr: concurrency: 8 max_tokens_budget: 0 # 0 = unlimited exclude: [] # gitignore-style patterns - rules: [] # [{path: "**/*.go", rule: "Errors must be wrapped with %w"}] - rule_file: "" # or an OCR rule.json in the repo + rules: [] # [{path: "**/*.go", rule: "Errors must be wrapped with %w", merge_system_rule: true}]; without merge_system_rule a rule replaces OCR's built-in rule for matching files + rule_file: "" # or an OCR rule.json in the repo (read at the MR's base commit) use_mr_description_as_background: true extra_args: [] # passed verbatim to `ocr review` review: min_severity: low # low | medium | high | critical categories: [] # bug, security, performance, maintainability, test, style, documentation, other - max_comments: 30 # 0 = unlimited; the rest go to the summary note + max_comments: 30 # 0 = unlimited; the bot's open threads count; the rest go to the summary note suggestions: true # GitLab "apply suggestion" blocks resolve_outdated: true # resolve own threads once the flagged code changed fail_on_severity: "" # e.g. critical: fail the job (exit 3) post_failures: true # post a note when the review fails -skip: - drafts: true - labels: [no-review] - authors: [] - title_regex: ['^\[WIP\]'] - source_branches: [] # exact names or regexes - target_branches: [] +# pruefbyte reviews whenever it is run. When it runs (drafts, labels, authors, +# branches, ...) is decided by the CI job's rules: see the README. diff --git a/templates/pruefbyte.gitlab-ci.yml b/templates/pruefbyte.gitlab-ci.yml index 31f3b07..4f3e56c 100644 --- a/templates/pruefbyte.gitlab-ci.yml +++ b/templates/pruefbyte.gitlab-ci.yml @@ -6,6 +6,15 @@ # - project: /pruefbyte # file: templates/pruefbyte.gitlab-ci.yml # +# The job runs in merge request pipelines, in the `test` stage: if the project +# defines `stages:`, keep `test` there or override the job's `stage:`. If the +# project's other jobs run in branch pipelines, switch to merge request pipelines +# with workflow:rules (see README); otherwise every push runs two pipelines and +# the merge check only sees the one that holds just this job. +# +# pruefbyte reviews whenever the job runs. To skip drafts, labels, authors or +# branches, override `rules:` on this job (see README, "When it runs"). +# # Required CI/CD variables (masked, ideally set at group level): # PRUEFBYTE_GITLAB_TOKEN personal access token of the bot user (scope: api, role: Developer) # PRUEFBYTE_LLM_API_KEY API key for the LLM provider @@ -22,6 +31,14 @@ pruefbyte-review: stage: test image: $PRUEFBYTE_IMAGE rules: + # A merge train pipeline re-runs an already reviewed head; failing there would + # drop the merge request from the train. + - if: $CI_MERGE_REQUEST_EVENT_TYPE == "merge_train" + when: never + # A fork's pipeline runs in the fork, without this project's CI/CD variables; + # pipelines a maintainer runs in this project for the fork still review it. + - if: $CI_PROJECT_ID != $CI_MERGE_REQUEST_PROJECT_ID + when: never - if: $CI_PIPELINE_SOURCE == "merge_request_event" variables: GIT_DEPTH: "0" # the MR's base commit must be in the clone From fbaf2504bb2c61ae4f456fb2e3875c3cc0a46a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20H=C3=B6gerl?= Date: Mon, 5 Oct 2026 14:08:30 +0200 Subject: [PATCH 4/4] Add CI pipeline and multi-arch image publishing GitHub Actions runs gofmt, go mod tidy, golangci-lint (v2.14.0, pinned in mise.toml), go vet, race-enabled tests and linux/amd64+arm64 builds on pushes to main, v* tags and pull requests. Once those pass, it builds a linux/amd64+arm64 image and pushes it to ghcr.io (latest from main, semver tags from v* tags, sha-); pull requests only build it. The Dockerfile now cross-compiles on the build platform so only the final stage runs under emulation. Lint fixes: wrap both errors with %w, tighter artifact permissions, sha256 for finding fingerprints. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 111 ++++++++++++++++++++++++++++++ .golangci.yml | 26 +++++++ Dockerfile | 12 +++- README.md | 23 +++++-- cmd/pruefbyte/main.go | 4 +- internal/config/config.go | 4 +- internal/review/format.go | 4 +- internal/review/review.go | 2 +- mise.toml | 1 + templates/pruefbyte.gitlab-ci.yml | 2 +- 10 files changed, 172 insertions(+), 17 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 .golangci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..3308335 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,111 @@ +name: CI + +on: + push: + branches: [main] + tags: ["v*"] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +env: + IMAGE: ghcr.io/${{ github.repository }} + +jobs: + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + - name: Check formatting + run: | + unformatted=$(gofmt -l .) + if [ -n "$unformatted" ]; then + echo "::error::Run gofmt -w on these files:" + echo "$unformatted" + exit 1 + fi + - name: Check go.mod and go.sum are tidy + run: | + go mod tidy + git diff --exit-code -- go.mod go.sum + - uses: golangci/golangci-lint-action@v9 + with: + version: v2.14.0 # keep in sync with mise.toml + + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + - run: go vet ./... + - run: go test -race -coverprofile=coverage.out ./... + - run: go tool cover -func=coverage.out | tail -1 + + build: + runs-on: ubuntu-latest + strategy: + matrix: + goarch: [amd64, arm64] + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-go@v7 + with: + go-version-file: go.mod + - run: go build -trimpath -o pruefbyte-linux-${{ matrix.goarch }} ./cmd/pruefbyte + env: + CGO_ENABLED: "0" + GOOS: linux + GOARCH: ${{ matrix.goarch }} + + image: + needs: [lint, test, build] + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + steps: + - uses: actions/checkout@v7 + - uses: docker/setup-qemu-action@v4 + - uses: docker/setup-buildx-action@v4 + - uses: docker/login-action@v4 + if: github.event_name != 'pull_request' + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + - id: meta + uses: docker/metadata-action@v6 + with: + images: ${{ env.IMAGE }} + # latest follows main only; the default (auto) would also move it on every v* tag. + flavor: latest=false + # Lowest priority, so the version passed to the build is sha- on main, not "latest". + tags: | + type=ref,event=pr + type=raw,value=latest,enable={{is_default_branch}},priority=50 + type=semver,pattern={{version}} + type=semver,pattern={{major}}.{{minor}} + type=sha + - uses: docker/build-push-action@v7 + with: + context: . + platforms: linux/amd64,linux/arm64 + # Pull requests only check that the image builds. + push: ${{ github.event_name != 'pull_request' }} + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + build-args: | + VERSION=${{ steps.meta.outputs.version }} + cache-from: type=gha + cache-to: type=gha,mode=max diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..686be12 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,26 @@ +version: "2" + +linters: + default: standard # errcheck, govet, ineffassign, staticcheck, unused + enable: + - bodyclose + - errorlint + - gosec + - misspell + - nolintlint + - unconvert + settings: + gosec: + excludes: + - G204 # subprocess with variable args: running git and ocr is the point + - G304 # reading files from variable paths: config, rule and result files are the point + exclusions: + presets: + - std-error-handling + rules: + - path: _test\.go + linters: [errcheck, gosec] + +formatters: + enable: + - gofmt diff --git a/Dockerfile b/Dockerfile index a30f1a5..534b2b6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,15 +1,21 @@ # syntax=docker/dockerfile:1 ARG GO_VERSION=1.25 -FROM golang:${GO_VERSION}-alpine AS build +# Build stages run on the build machine's platform and cross-compile, so a +# multi-arch build only emulates the final stage's package install. +FROM --platform=$BUILDPLATFORM golang:${GO_VERSION}-alpine AS build WORKDIR /src COPY go.mod go.sum ./ RUN go mod download COPY . . ARG VERSION=dev -RUN CGO_ENABLED=0 go build -trimpath -ldflags "-s -w -X main.version=${VERSION}" -o /out/pruefbyte ./cmd/pruefbyte +# BuildKit sets these for the target platform; defaults would override them. +ARG TARGETOS +ARG TARGETARCH +RUN CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH \ + go build -trimpath -ldflags "-s -w -X main.version=${VERSION}" -o /out/pruefbyte ./cmd/pruefbyte -FROM alpine:3 AS ocr +FROM --platform=$BUILDPLATFORM alpine:3 AS ocr ARG OCR_VERSION=v1.12.10 # BuildKit sets TARGETARCH for the platform being built; a default here would # override it (e.g. an amd64 binary in an arm64 image). diff --git a/README.md b/README.md index 38955e7..f6993c3 100644 --- a/README.md +++ b/README.md @@ -22,10 +22,13 @@ from a dedicated bot account. | `PRUEFBYTE_LLM_API_KEY` | the LLM API key | | `PRUEFBYTE_LLM_PROVIDER` | e.g. `anthropic` | | `PRUEFBYTE_LLM_MODEL` | e.g. `claude-sonnet-5` | -3. **Image.** Build and push it: +3. **Image.** CI publishes `ghcr.io/feinarbyte/pruefbyte` for linux/amd64 and linux/arm64: + `latest` from `main`, `X.Y.Z` and `X.Y` from `vX.Y.Z` tags, and `sha-` for each of these pushes. + If the package is private, give the GitLab runners pull access (a GitHub token with + `read:packages` in `DOCKER_AUTH_CONFIG`). To build your own: ```sh - docker build -t registry.example.com/tools/pruefbyte:latest --build-arg OCR_VERSION=v1.12.10 . - docker push registry.example.com/tools/pruefbyte:latest + docker buildx build --platform linux/amd64,linux/arm64 --build-arg OCR_VERSION=v1.12.10 \ + -t registry.example.com/tools/pruefbyte:latest --push . ``` 4. **Pipeline.** Include the template in each project (or in a shared CI config): ```yaml @@ -33,7 +36,7 @@ from a dedicated bot account. - project: tools/pruefbyte file: templates/pruefbyte.gitlab-ci.yml variables: - PRUEFBYTE_IMAGE: registry.example.com/tools/pruefbyte:latest + PRUEFBYTE_IMAGE: ghcr.io/feinarbyte/pruefbyte:latest ``` The job runs in merge request pipelines (not in merge train pipelines, and not in pipelines that run in a fork, which lack the CI/CD variables), in the `test` stage: if the project defines `stages:`, keep `test` or override the job's `stage:`. If the project's other jobs run in branch pipelines, switch to merge request pipelines while a merge request is open, as GitLab recommends. Otherwise every push runs two pipelines, and the merge check only looks at the merge request pipeline, which then holds nothing but this job: ```yaml @@ -126,10 +129,18 @@ pruefbyte review --config pruefbyte.yml --gitlab-url https://gitlab.example.com ## Development ```sh -mise install # Go toolchain -go test ./... +mise install # Go toolchain and golangci-lint +gofmt -l . # must print nothing +golangci-lint run ./... +go test -race ./... ``` +GitHub Actions (`.github/workflows/ci.yml`) runs these checks plus `go mod tidy` and +linux/amd64 + linux/arm64 builds on pushes to `main` and `v*` tags and on every pull +request. Once they pass, it +builds the multi-arch image. On `main` and `v*` tags the image is pushed to GHCR; +for pull requests it is only built. + Layout: | Path | Purpose | diff --git a/cmd/pruefbyte/main.go b/cmd/pruefbyte/main.go index 8185909..4a242d3 100644 --- a/cmd/pruefbyte/main.go +++ b/cmd/pruefbyte/main.go @@ -250,10 +250,10 @@ func runReview(ctx context.Context, g *globalFlags, f *reviewFlags) error { } if f.artifactDir != "" { deps.SaveResult = func(raw []byte) error { - if err := os.MkdirAll(f.artifactDir, 0o755); err != nil { + if err := os.MkdirAll(f.artifactDir, 0o750); err != nil { return err } - return os.WriteFile(filepath.Join(f.artifactDir, "ocr-result.json"), raw, 0o644) + return os.WriteFile(filepath.Join(f.artifactDir, "ocr-result.json"), raw, 0o600) } } diff --git a/internal/config/config.go b/internal/config/config.go index dd5e9ee..3878ae9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -103,8 +103,8 @@ type repoConfig struct { func Default() Config { return Config{ - GitLab: GitLab{TokenEnv: "PRUEFBYTE_GITLAB_TOKEN"}, - LLM: LLM{APIKeyEnv: "PRUEFBYTE_LLM_API_KEY"}, + GitLab: GitLab{TokenEnv: "PRUEFBYTE_GITLAB_TOKEN"}, //nolint:gosec // G101: the name of an env var, not a credential + LLM: LLM{APIKeyEnv: "PRUEFBYTE_LLM_API_KEY"}, //nolint:gosec // G101: the name of an env var, not a credential OCR: OCR{ Binary: "ocr", Effort: "medium", diff --git a/internal/review/format.go b/internal/review/format.go index 23d1e3e..d66a92c 100644 --- a/internal/review/format.go +++ b/internal/review/format.go @@ -1,7 +1,7 @@ package review import ( - "crypto/sha1" + "crypto/sha256" "encoding/hex" "fmt" "regexp" @@ -27,7 +27,7 @@ func fingerprint(c ocr.Comment) string { if code := codeLines(c.ExistingCode); code != nil { key = "code\x00" + strings.ToLower(strings.TrimSpace(c.Category)) + "\x00" + strings.Join(code, "\n") } - h := sha1.Sum([]byte(c.Path + "\x00" + key)) + h := sha256.Sum256([]byte(c.Path + "\x00" + key)) return hex.EncodeToString(h[:8]) } diff --git a/internal/review/review.go b/internal/review/review.go index 7e9538d..2453ed3 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -152,7 +152,7 @@ func Run(ctx context.Context, d Deps, opts Options) (*Outcome, error) { logf("warning: posting failure note: %v", perr) } } - return nil, fmt.Errorf("%w: %v", ErrReviewFailed, err) + return nil, fmt.Errorf("%w: %w", ErrReviewFailed, err) } logf("ocr finished with status %s: %d finding(s)", res.Status, len(res.Comments)) diff --git a/mise.toml b/mise.toml index ac7816d..172014c 100644 --- a/mise.toml +++ b/mise.toml @@ -1,2 +1,3 @@ [tools] go = "1.25" +golangci-lint = "2.14.0" diff --git a/templates/pruefbyte.gitlab-ci.yml b/templates/pruefbyte.gitlab-ci.yml index 4f3e56c..a3c7167 100644 --- a/templates/pruefbyte.gitlab-ci.yml +++ b/templates/pruefbyte.gitlab-ci.yml @@ -25,7 +25,7 @@ # PRUEFBYTE_IMAGE image to run (default below) variables: - PRUEFBYTE_IMAGE: registry.example.com/tools/pruefbyte:latest + PRUEFBYTE_IMAGE: ghcr.io/feinarbyte/pruefbyte:latest pruefbyte-review: stage: test