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/.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/.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 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 new file mode 100644 index 0000000..534b2b6 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,37 @@ +# syntax=docker/dockerfile:1 +ARG GO_VERSION=1.25 + +# 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 +# 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 --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). +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-${arch}" \ + && curl -fsSLO "https://github.com/alibaba/open-code-review/releases/download/${OCR_VERSION}/sha256sum.txt" \ + && 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. +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..f6993c3 --- /dev/null +++ b/README.md @@ -0,0 +1,155 @@ +# 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.** 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 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 + include: + - project: tools/pruefbyte + file: templates/pruefbyte.gitlab-ci.yml + variables: + 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 + workflow: + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + - if: $CI_COMMIT_BRANCH && $CI_OPEN_MERGE_REQUESTS + when: never + - if: $CI_COMMIT_BRANCH + ``` + +## 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. 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`), 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. + +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." + merge_system_rule: true # add to OCR's built-in Go rules instead of replacing them +review: + min_severity: medium + max_comments: 15 +``` + +## 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 | +|---|---| +| 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 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 + 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. | + +## 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 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 | +|---|---| +| `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..4a242d3 --- /dev/null +++ b/cmd/pruefbyte/main.go @@ -0,0 +1,273 @@ +// 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() + runner.SecretEnv = []string{base.GitLab.TokenEnv, base.LLM.APIKeyEnv} + 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 + } + 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 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 { + s = strings.ReplaceAll(s, secret, "***") + } + } + return s + }, + } + if f.artifactDir != "" { + deps.SaveResult = func(raw []byte) error { + if err := os.MkdirAll(f.artifactDir, 0o750); err != nil { + return err + } + return os.WriteFile(filepath.Join(f.artifactDir, "ocr-result.json"), raw, 0o600) + } + } + + 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..fb7eb0a --- /dev/null +++ b/cmd/pruefbyte/main_test.go @@ -0,0 +1,196 @@ +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() + 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 + 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..3878ae9 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,247 @@ +// 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" + "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"` +} + +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"` + // 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 { + 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"` +} + +// 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"` +} + +func Default() Config { + return Config{ + 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", + Timeout: 30 * time.Minute, + Concurrency: 8, + Exclude: []string{}, + UseMRDescriptionAsBackground: true, + }, + Review: Review{ + MinSeverity: "low", + MaxComments: 30, + Suggestions: true, + ResolveOutdated: true, + PostFailures: true, + }, + } +} + +// 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.*, and review.* 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)) + } + } + 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")) + } + 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)")) + } + return errors.Join(errs...) +} + +// 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, "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] } + +// 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..eec6c4e --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,168 @@ +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 +` + 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_REVIEW_CATEGORIES": "bug, security", + "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}, + {"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/**"}, + } + 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 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 { + 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{{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() + 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) + } + // 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) { + 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..bd4800d --- /dev/null +++ b/internal/gitlab/client.go @@ -0,0 +1,238 @@ +// 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" + "net/http" + + gl "gitlab.com/gitlab-org/api/client-go" +) + +type MR struct { + IID int64 + Title string + Description 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; 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 { + return nil, err + } + 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 { + 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, + WebURL: m.WebURL, + BaseSHA: m.DiffRefs.BaseSha, + StartSHA: m.DiffRefs.StartSha, + HeadSHA: m.DiffRefs.HeadSha, + } + 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), 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.WithRequestRetry(retryCreate)) + 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), gl.WithRequestRetry(retryCreate)); 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..c24341e --- /dev/null +++ b/internal/gitlab/client_test.go @@ -0,0 +1,84 @@ +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 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) + _, _ = 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..5750f09 --- /dev/null +++ b/internal/ocr/ocr_test.go @@ -0,0 +1,243 @@ +package ocr + +import ( + "context" + "os" + "path/filepath" + "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", "anthropic-beta": "x-1,y-2"}, + 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,anthropic-beta="x-1,y-2",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, 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", + "--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) { + 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 !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) + } +} + +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)) + } +} + +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 new file mode 100644 index 0000000..d921678 --- /dev/null +++ b/internal/ocr/result.go @@ -0,0 +1,105 @@ +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"` + 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 { + 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" } + +// 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 new file mode 100644 index 0000000..f6eacff --- /dev/null +++ b/internal/ocr/rules.go @@ -0,0 +1,76 @@ +package ocr + +import ( + "encoding/json" + "fmt" + "path/filepath" + "slices" + "strings" + + "pruefbyte/internal/config" +) + +// 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 new file mode 100644 index 0000000..0dcf7c5 --- /dev/null +++ b/internal/ocr/runner.go @@ -0,0 +1,352 @@ +// 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" + "runtime" + "slices" + "sort" + "strconv" + "strings" + "syscall" + "time" + + "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 + // 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. +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())+9) + for _, kv := range os.Environ() { + k, _, _ := strings.Cut(kv, "=") + // 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 k == "HOME" || k == "USERPROFILE" || k == "XDG_CONFIG_HOME": + continue + 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) + } + 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. +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(jsonKeys(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 { + 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, ",")) + } + add("language", language) + 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 { + settings, err := ConfigSettings(llm, apiKey, language) + if err != nil { + return err + } + for _, s := range settings { + // "--": 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 := 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)) + } + } + return nil +} + +type ReviewOptions struct { + RepoDir string + From, To string + Provider, Model string + Effort string + Concurrency int + MaxTokensBudget int + // 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 { + 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 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, err := r.command(ctx, o.Args(outFile)...) + if err != nil { + return nil, nil, err + } + cmd.Dir = o.RepoDir + // 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) + runErr := cmd.Run() + + raw, readErr := os.ReadFile(outFile) + if runErr != nil { + if ctx.Err() != nil { + 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), + // but not when OCR itself reports the review as failed. + if readErr == nil { + if res, err := ParseResult(raw); err == nil && len(res.Comments) > 0 && !res.Failed() { + 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, 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 new file mode 100644 index 0000000..f629b09 --- /dev/null +++ b/internal/review/diff.go @@ -0,0 +1,215 @@ +package review + +import ( + "regexp" + "strconv" + "strings" + + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +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 + 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 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. +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, fd.oldLines = parsePatch(d.Diff) + } + idx[d.NewPath] = fd + } + return idx +} + +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 { + 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, 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, text: l[1:]} + oldLines[oldLn] = oldLineInfo{newLine: newLn, text: l[1:]} + oldLn++ + newLn++ + } + } + 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 +} + +// 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 + } + if start <= 0 || start > end { + start = end + } + if end <= 0 { + return placement{}, false + } + if fd.lines == nil { + // Unknown patch: try the end line as an added line and let GitLab decide. + 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 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 true +} + +// 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 { + 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 + } + 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") { + 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 + } + if !inHunk || strings.HasPrefix(l, `\`) { + continue + } + 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 + } + } + } + return out +} diff --git a/internal/review/diff_test.go b/internal/review/diff_test.go new file mode 100644 index 0000000..27be819 --- /dev/null +++ b/internal/review/diff_test.go @@ -0,0 +1,155 @@ +package review + +import ( + "strings" + "testing" + + "pruefbyte/internal/gitlab" + "pruefbyte/internal/ocr" +) + +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, old := parsePatch(patch) + want := map[int]lineInfo{ + 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) + } + for k, v := range want { + if got[k] != v { + 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 TestPlace(t *testing.T) { + fd := &fileDiff{} + fd.lines, fd.oldLines = parsePatch(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 { + 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 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) + } +} + +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"}, + // 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) + } + 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") + } + 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", 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) + } + 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", 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", 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 new file mode 100644 index 0000000..b690b0a --- /dev/null +++ b/internal/review/filter.go @@ -0,0 +1,45 @@ +package review + +import ( + "slices" + "sort" + "strings" + + "pruefbyte/internal/config" + "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 +} diff --git a/internal/review/format.go b/internal/review/format.go new file mode 100644 index 0000000..d66a92c --- /dev/null +++ b/internal/review/format.go @@ -0,0 +1,209 @@ +package review + +import ( + "crypto/sha256" + "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. 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 { + 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 := sha256.Sum256([]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 ms := fpPattern.FindAllStringSubmatch(body, -1); len(ms) > 0 { + return ms[len(ms)-1][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 placed at pl. +func inlineBody(c ocr.Comment, fp string, pl placement, suggestions bool) string { + var b strings.Builder + head := badge(c) + if pl.at != 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(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. 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) + } + } + fmt.Fprintf(&b, "\n%s%s -->", fpPrefix, fp) + 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 + 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 != "" { + 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) + 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) + 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 { + 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.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 new file mode 100644 index 0000000..2453ed3 --- /dev/null +++ b/internal/review/review.go @@ -0,0 +1,467 @@ +// 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" + "io/fs" + "os" + "path" + "path/filepath" + "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) + // 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. + 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...) } + 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 { + 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) + } + if opts.ExpectedHeadSHA != "" && opts.ExpectedHeadSHA != mr.HeadSHA { + 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(ctx, cfg, mr, opts.RepoDir, d) + 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: %w", ErrReviewFailed, err) + } + logf("ocr finished with status %s: %d finding(s)", res.Status, len(res.Comments)) + + 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{} + 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) + // 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 + } + seen[key] = true + if state.fingerprints[fp] { + st.Duplicates++ + continue + } + // 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 + } + fd := idx[c.Path] + if fd == nil { + unplaced = append(unplaced, c) + continue + } + 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: pl.newLine, OldLine: pl.oldLine, + } + 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, pl.line(), err) + unplaced = append(unplaced, c) + default: + logf("warning: posting comment on %s:%d: %v", c.Path, pl.line(), err) + failed = append(failed, c) + } + } + 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, res, 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, Failed: failed, MaxComments: cfg.Review.MaxComments, + Warnings: res.Warnings, Incomplete: !res.Complete(), + } + 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) + } + } + + 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(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, ExtraArgs: cfg.OCR.ExtraArgs, + } + // 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, fmt.Errorf("reading %s: %w", p, 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 runes := []rune(bg); len(runes) > 8000 { + bg = string(runes[:8000]) + } + p, err := d.OCR.WriteTemp("background.md", []byte(bg)) + if err != nil { + return ro, err + } + ro.BackgroundFile = p + } + 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 + summary *gitlab.Note +} + +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 +// 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 + } + // 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 + } + 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, + 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, 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.reopened || t.pos.HeadSHA == headSHA { + continue + } + changed := false + switch { + case idx[t.pos.NewPath] == nil: + changed = true // the file is no longer part of the MR + 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) + if err != nil { + logf("warning: %v", err) + } + ranges = touchedOldRanges(diffs) + compares[t.pos.HeadSHA] = ranges + } + for _, r := range ranges[t.pos.NewPath] { + if t.pos.NewLine >= r[0] && t.pos.NewLine <= r[1] { + changed = true + break + } + } + } + if !changed { + continue + } + 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 + } + 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..a111e8d --- /dev/null +++ b/internal/review/review_test.go @@ -0,0 +1,647 @@ +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, failLine with 500. + rejectLine, failLine 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]}"} + } + 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{ + 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, body string) error { + f.resolved = append(f.resolved, id) + for i := range f.discussions { + if f.discussions[i].ID == id { + 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 +} + +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 + 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, 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" + +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) + } +} + +// 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() + 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) + } + 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.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/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/pruefbyte.example.yml b/pruefbyte.example.yml new file mode 100644 index 0000000..27cb56c --- /dev/null +++ b/pruefbyte.example.yml @@ -0,0 +1,44 @@ +# 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", 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 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 + +# 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 new file mode 100644 index 0000000..a3c7167 --- /dev/null +++ b/templates/pruefbyte.gitlab-ci.yml @@ -0,0 +1,55 @@ +# 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 +# +# 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 +# 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: ghcr.io/feinarbyte/pruefbyte:latest + +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 + 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/